[PATCH] sequencer: rectify empty hint in call of require_clean_work_tree()

[PATCH] sequencer: rectify empty hint in call of require_clean_work_tree()

[Oswald Buddenhagen]

The canonical way to represent "no error hint" is making it null, which
shortcuts the error() call altogether.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 sequencer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index 3be23d7ca2..7c275c9a65 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -6168,7 +6168,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);
-- 
2.40.0.152.g15d061e6df

Re: [PATCH] sequencer: rectify empty hint in call of require_clean_work_tree()

[Oswald Buddenhagen]

ping!

On Thu, Mar 23, 2023 at 05:22:34PM +0100, Oswald Buddenhagen wrote:
>The canonical way to represent "no error hint" is making it null, which
>shortcuts the error() call altogether.
>

Re: [PATCH] sequencer: rectify empty hint in call of require_clean_work_tree()

[Junio C Hamano]

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

> ping!
>
> On Thu, Mar 23, 2023 at 05:22:34PM +0100, Oswald Buddenhagen wrote:
>>The canonical way to represent "no error hint" is making it null, which
>>shortcuts the error() call altogether.

I won't repeat what Peff already said on another ping! we saw
recently on the list.

The call to require_clean_work_tree() with "" hint existed ever
since this part of the "rebase" machinery was rewritten in C by
b97e1873 (rebase -i: rewrite complete_action() in C, 2018-08-28).
I added the author of that change to the Cc: list.

The original implementation of require_clean_work_tree looked like
this:

require_clean_work_tree () {
	git rev-parse --verify HEAD >/dev/null || exit 1
	git update-index -q --ignore-submodules --refresh
	err=0

	if ! git diff-files --quiet --ignore-submodules
	then
		...
		err=1
	fi

	if ! git diff-index --cached --quiet --ignore-submodules HEAD --
	then
		...
		err=1
	fi

	if test $err = 1
	then
		test -n "$2" && echo "$2" >&2
		exit 1
	fi
}

I.e. the second argument, "hint", is shown only when it was a
non-empty string.  It did not add "error:" prefix before the
message.

In contrast, this is what wt-status.c has:

int require_clean_work_tree(struct repository *r,
			    const char *action,
			    const char *hint,
			    int ignore_submodules,
			    int gently)
{
	struct lock_file lock_file = LOCK_INIT;
	int err = 0, fd;

	...
	if (err) {
		if (hint)
			error("%s", hint);
		if (!gently)
			exit(128);
	}

	return err;
}

Arguably, using error() as a replacement for 'echo "$2" >&2' was a
sloppy conversion made back in ea63b393 (pull: make code more
similar to the shell script again, 2016-10-07), but I suspect that
in-tree callers that do have something to say, and the end-users who
are used to see the messages these callers produce, expect to see
the "error:" prefix these days, so it needs further study if we
wanted to "fix" the misuse of error() there.  In any case, the
observation that motivated your patch is not error() vs fputs().

For squelching a useless "hint" that is empty (other than that
mistaken "error:" prefix), however, I think you can and should do
better than replacing "" with NULL on the callers' side.  As we can
see from the comparison between the original, scripted version and
the verison in C that is in wt-status.c of require_clean_work_tree,
checking for NULL-ness of hint is another misconversion made when
it was rewritten in C.

I think the right fix would be more like the attached patch, which
will fix any other callsites that pass "" at the same time.  Of
course, you can fix the callers on top, but that is secondary.

 wt-status.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git i/wt-status.c w/wt-status.c
index 97b9c1c035..2b6dc4d6ac 100644
--- i/wt-status.c
+++ w/wt-status.c
@@ -2650,7 +2650,7 @@ int require_clean_work_tree(struct repository *r,
 	}
 
 	if (err) {
-		if (hint)
+		if (hint && *hint)
 			error("%s", hint);
 		if (!gently)
 			exit(128);

Re: [PATCH] sequencer: rectify empty hint in call of require_clean_work_tree()

[Oswald Buddenhagen]

On Thu, Apr 27, 2023 at 02:13:29PM -0700, Junio C Hamano wrote:
>I think the right fix would be more like the attached patch, which
>will fix any other callsites that pass "" at the same time.  Of
>course, you can fix the callers on top, but that is secondary.
>
there is only that one incorrect (in-tree) call.
i don't think that making the behavior more compliant with the shell 
implementation is particularly elegant or even useful.
if i wanted to be super-pedantic about it, i'd assert that non-null 
strings are non-empty. but that would only help if all error paths 
actually have test coverage.

>--- i/wt-status.c
>+++ w/wt-status.c
>@@ -2650,7 +2650,7 @@ int require_clean_work_tree(struct repository *r,
> 
>-		if (hint)
>+		if (hint && *hint)
> 			error("%s", hint);

-- ossi

Re: [PATCH] sequencer: rectify empty hint in call of require_clean_work_tree()

[Felipe Contreras]

Oswald Buddenhagen wrote:
> if i wanted to be super-pedantic about it, i'd assert that non-null 
> strings are non-empty.

I would disagree. "" is empty but not null, not just in C but in many
other languages, including shell.

-- 
Felipe Contreras

Re: [PATCH] sequencer: rectify empty hint in call of require_clean_work_tree()

[Oswald Buddenhagen]

On Tue, May 02, 2023 at 12:57:03PM -0600, Felipe Contreras wrote:
>Oswald Buddenhagen wrote:
>> if i wanted to be super-pedantic about it, i'd assert that non-null 
>> strings are non-empty.
>
>I would disagree. "" is empty but not null, not just in C but in many
>other languages, including shell.
>
yes. that's kinda the point the assert would make.

-- ossi

[PATCH v2] sequencer: rectify empty hint in call of require_clean_work_tree()

[Oswald Buddenhagen]

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,
I'd go in the opposite direction and assert() that the argument is not
an empty string.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
v2:
- expanded commit message

Cc: Junio C Hamano <gitster@pobox.com>
---
 sequencer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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);
-- 
2.40.0.152.g15d061e6df

Re: [PATCH v2] sequencer: rectify empty hint in call of require_clean_work_tree()

[Junio C Hamano]

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);

[PATCH v3] sequencer: rectify empty hint in call of require_clean_work_tree()

[Oswald Buddenhagen]

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:", which would appear when starting a
rebase whose initial checkout worked fine despite a dirty worktree. This
was introduced by 97e1873 (rebase -i: rewrite complete_action() in C,
2018-08-28), which did a somewhat inaccurate conversion from shell.

To avoid that such bugs re-appear, test for the condition in
require_clean_work_tree().

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
v3:
- added BUG()
- rewrote commit message again
v2:
- expanded commit message

Cc: Junio C Hamano <gitster@pobox.com>
---
 sequencer.c | 2 +-
 wt-status.c | 6 +++++-
 2 files changed, 6 insertions(+), 2 deletions(-)

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);
diff --git a/wt-status.c b/wt-status.c
index 8a1a4fb1f0..c8c1780566 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -2654,8 +2654,12 @@ int require_clean_work_tree(struct repository *r,
 	}
 
 	if (err) {
-		if (hint)
+		if (hint) {
+			if (!*hint)
+				BUG("empty hint passed to require_clean_work_tree();"
+				    " use NULL instead");
 			error("%s", hint);
+		}
 		if (!gently)
 			exit(128);
 	}
-- 
2.40.0.152.g15d061e6df

Re: [PATCH v3] sequencer: rectify empty hint in call of require_clean_work_tree()

[Junio C Hamano]

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:", which would appear when starting a
> rebase whose initial checkout worked fine despite a dirty worktree. This

Thanks for adding this info.  

"git rebase" does not seem to start (i.e. does not perform "initial
checkout") from a dirty working tree, with

	error: cannot rebase: You have unstaged changes.
	error: Please commit or stash them.

at least from my quick attempt.  I am not sure if this is actually
triggerble?

In any case, I've replaced v2 that I had with this version, as the
description is much better.  The change to the code does look
correct but now it does not seem to trigger, it is unclear to me if
the fix is merely theoretical (a command-by-command transcript would
have helped here).

Thanks.

Re: [PATCH v3] sequencer: rectify empty hint in call of require_clean_work_tree()

[Oswald Buddenhagen]

On Wed, Aug 09, 2023 at 02:44:25PM -0700, Junio C Hamano wrote:
>"git rebase" does not seem to start (i.e. does not perform "initial
>checkout") from a dirty working tree, with
>
>	error: cannot rebase: You have unstaged changes.
>	error: Please commit or stash them.
>
>at least from my quick attempt.  I am not sure if this is actually
>triggerble?
>
hmm, unless i'm missing something, it is apparently not ... currently.

it becomes actually relevant only after my "rebase: improve resumption 
from incorrect initial todo list" patch, which still needs reworking.

so there are several possibilities how to proceed with this:
- you merge it as a merely theoretical fix (adjust the commit message 
   yourself if saving the round-trip is more convenient for you)
- we postpone the change until it becomes not theoretical
- we make sure that the code really is not triggerable and delete it.  
   this would obviously introduce some churn, as i'll need to 
   re-introduce it. but then, i remember that something was wrong with it 
   anyway (i.e., it didn't actually trigger in some cases i expected it 
   to, but i haven't figured out yet why), so the new code would be 
   slightly different anyway.

regards

Re: [PATCH v3] sequencer: rectify empty hint in call of require_clean_work_tree()

[Junio C Hamano]

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

> it becomes actually relevant only after my "rebase: improve resumption
> from incorrect initial todo list" patch, which still needs reworking.

We are already past -rc1 by the time you see this response, so this
fix is not of immediate urgency.  It has been with us for more than
several cycles, it does not seem to be readily triggerable, and its
effect is merely a single extra empty error message when other things
are mentioned there.

Even if it may not be triggerable, consistent use of the API is
something that is worth aiming for.  

If I were working on a separate change (i.e. your "resume from bad
initial todo") that will make this triggerable, I would explain this
patch as a pure clean-up patch to use the API function consistently,
and tell readers that there is currently no way to trigger it
(assuming that it is the case---I only poked a bit yesterday and
would not claim that I did a thorough investigation), and that it
will start mattering with a later step in the series.  And make it
an early part of the series that will contain the "resume from bad
initial todo" patch.

Thanks.

[PATCH v4] sequencer: rectify empty hint in call of require_clean_work_tree()

[Oswald Buddenhagen]

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:", which would appear when the worktree
is dirtied while editing the initial rebase todo file. This was
introduced by 97e1873 (rebase -i: rewrite complete_action() in C,
2018-08-28), which did a somewhat inaccurate conversion from shell.

To avoid that such bugs re-appear, test for the condition in
require_clean_work_tree().

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>

---
yeah, so i _did_ miss something.

v4:
- fixed reproduction instructions
v3:
- added BUG()
- rewrote commit message again
v2:
- expanded commit message

Cc: Junio C Hamano <gitster@pobox.com>
---
 sequencer.c | 2 +-
 wt-status.c | 6 +++++-
 2 files changed, 6 insertions(+), 2 deletions(-)

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);
diff --git a/wt-status.c b/wt-status.c
index 8a1a4fb1f0..c8c1780566 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -2654,8 +2654,12 @@ int require_clean_work_tree(struct repository *r,
 	}
 
 	if (err) {
-		if (hint)
+		if (hint) {
+			if (!*hint)
+				BUG("empty hint passed to require_clean_work_tree();"
+				    " use NULL instead");
 			error("%s", hint);
+		}
 		if (!gently)
 			exit(128);
 	}
-- 
2.40.0.152.g15d061e6df

Re: [PATCH v4] sequencer: rectify empty hint in call of require_clean_work_tree()

[Junio C Hamano]

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:", which would appear when the worktree
> is dirtied while editing the initial rebase todo file.
> ...
> v4:
> - fixed reproduction instructions

Nice.  Will queue and mark it for 'next'.  Thanks.

Re: [PATCH v2] sequencer: rectify empty hint in call of require_clean_work_tree()

[Junio C Hamano]

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:".

Sorry, but I forgot to point out a rather obvious thing in my
review.  We would want to see a reproduction recipe described here
in the proposed log message at least.  Even better is an addition to
an existing test to ensure that there is no such empty "error:"
line, which will make sure that we will notice when anybody by
mistake (this includes a mismerge of other topics) breaks this fix.

Thanks.

>  sequencer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> 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);