Git Mailing List Archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
Cc: git@vger.kernel.org
Subject: Re: [PATCH v2] sequencer: rectify empty hint in call of require_clean_work_tree()
Date: Mon, 07 Aug 2023 13:19:37 -0700	[thread overview]
Message-ID: <xmqqa5v2ehba.fsf@gitster.g> (raw)
In-Reply-To: <20230807170935.2336730-1-oswald.buddenhagen@gmx.de> (Oswald Buddenhagen's message of "Mon, 7 Aug 2023 19:09:35 +0200")

Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:

> The canonical way to represent "no error hint" is making it null, which
> shortcuts the error() call altogether. This fixes the output by removing
> the line which said just "error:".
>
> Alternatively, one could make the function treat empty strings like null
> strings, which would make it resemble its original script implementation
> more closely, but that doesn't seem like a worthwhile goal. If anything,

The analysis I gave you in the previous round was primarily to
explain how this bug came to be, i.e. a misconversion of the
scripted original, and that would be a lot more pertinent
information than "is this closer to or further from the scripted
original?"  IOW, "equating NULL and an empty string is how the
original implemented the logic correctly, but the misconversion made
the current source to fail to do so" is the take-home message.  

You could correct the current version by making it follow the same
"NULL and an empty string are the same" convention to implement the
logic correctly, or you can build on the misconverted callee that
treats only NULL specially hence pass NULL instead of "".  Either
one is a valid implementation, but the reason to choose the former
would not be to "make it resemble the original".

IOW, "doesn't seem like a worthwhile goal" is beating a strawman.
Nobody advocates "if (!hint || !*hint)" because "the code must look
exactly like the original".  It is merely that it is one known way
to achieve correctness, as it was how the original achieved its
correctness.

> I'd go in the opposite direction and assert() that the argument is not
> an empty string.

If we were writing this program from scratch, "NULL means something
different from any sttring, and an empty string does not have
anything special compared to any other strings" would probably be a
good semantics to give to this helper function.  I'd be OK with the
change.  Also, adding "if (!*hint) BUG(...)" would be a good future
direction, I would think.

> diff --git a/sequencer.c b/sequencer.c
> index cc9821ece2..d15a7409d8 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -6182,7 +6182,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
>  	if (checkout_onto(r, opts, onto_name, &oid, orig_head))
>  		goto cleanup;
>  
> -	if (require_clean_work_tree(r, "rebase", "", 1, 1))
> +	if (require_clean_work_tree(r, "rebase", NULL, 1, 1))
>  		goto cleanup;
>  
>  	todo_list_write_total_nr(&new_todo);

  reply	other threads:[~2023-08-07 20:19 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-03-23 16:22 [PATCH] sequencer: rectify empty hint in call of require_clean_work_tree() Oswald Buddenhagen
2023-04-27  7:58 ` Oswald Buddenhagen
2023-04-27 21:13   ` Junio C Hamano
2023-04-27 22:33     ` Oswald Buddenhagen
2023-05-02 18:57       ` Felipe Contreras
2023-05-03  7:15         ` Oswald Buddenhagen
2023-08-07 17:09 ` [PATCH v2] " Oswald Buddenhagen
2023-08-07 20:19   ` Junio C Hamano [this message]
2023-08-09 17:15     ` [PATCH v3] " Oswald Buddenhagen
2023-08-09 21:44       ` Junio C Hamano
2023-08-10 11:24         ` Oswald Buddenhagen
2023-08-10 16:04           ` Junio C Hamano
2023-08-24 15:00           ` [PATCH v4] " Oswald Buddenhagen
2023-08-24 15:59             ` Junio C Hamano
2023-08-09  1:24   ` [PATCH v2] " Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqa5v2ehba.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=oswald.buddenhagen@gmx.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).