[PATCH] add-patch: response to invalid option

[PATCH] add-patch: response to invalid option

[Rubén Justo]

When the user introduces an invalid option, we respond with the whole
help text.

Instead of displaying the long help description, display a short error
message indicating the incorrectly introduced option with a note on how
to get the help text.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 add-patch.c                |  5 ++++-
 t/t3701-add-interactive.sh | 10 ++++++++++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/add-patch.c b/add-patch.c
index a06dd18985..c77902fec5 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1667,7 +1667,7 @@ static int patch_update_file(struct add_p_state *s,
 			}
 		} else if (s->answer.buf[0] == 'p') {
 			rendered_hunk_index = -1;
-		} else {
+		} else if (s->answer.buf[0] == '?') {
 			const char *p = _(help_patch_remainder), *eol = p;
 
 			color_fprintf(stdout, s->s.help_color, "%s",
@@ -1691,6 +1691,9 @@ static int patch_update_file(struct add_p_state *s,
 				color_fprintf_ln(stdout, s->s.help_color,
 						 "%.*s", (int)(eol - p), p);
 			}
+		} else {
+			err(s, _("Unknown option '%s' (use '?' for help)"),
+			    s->answer.buf);
 		}
 	}
 
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index bc55255b0a..b38fd5388a 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -61,6 +61,16 @@ test_expect_success 'setup (initial)' '
 	echo more >>file &&
 	echo lines >>file
 '
+
+test_expect_success 'invalid option' '
+	cat >expect <<-EOF &&
+	Unknown option ${SQ}W${SQ} (use ${SQ}?${SQ} for help)
+	EOF
+	test_write_lines W |
+	git -c core.filemode=true add -p 2>actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'status works (initial)' '
 	git add -i </dev/null >output &&
 	grep "+1/-0 *+2/-0 file" output
-- 
2.44.0.782.g480309b2c8

Re: [PATCH] add-patch: response to invalid option

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3138 bytes --]

On Mon, Apr 15, 2024 at 09:00:28PM +0200, Rubén Justo wrote:
> When the user introduces an invalid option, we respond with the whole
> help text.
> 
> Instead of displaying the long help description, display a short error
> message indicating the incorrectly introduced option with a note on how
> to get the help text.
> 
> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
>  add-patch.c                |  5 ++++-
>  t/t3701-add-interactive.sh | 10 ++++++++++
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/add-patch.c b/add-patch.c
> index a06dd18985..c77902fec5 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -1667,7 +1667,7 @@ static int patch_update_file(struct add_p_state *s,
>  			}
>  		} else if (s->answer.buf[0] == 'p') {
>  			rendered_hunk_index = -1;
> -		} else {
> +		} else if (s->answer.buf[0] == '?') {
>  			const char *p = _(help_patch_remainder), *eol = p;
>  
>  			color_fprintf(stdout, s->s.help_color, "%s",

I think it would've made sense to describe this change in the commit
message, as well. Currently, the reader is left wondering whether "?" is
a new shortcut that you introduce in this patch or whether it is already
documented as such.

> @@ -1691,6 +1691,9 @@ static int patch_update_file(struct add_p_state *s,
>  				color_fprintf_ln(stdout, s->s.help_color,
>  						 "%.*s", (int)(eol - p), p);
>  			}
> +		} else {
> +			err(s, _("Unknown option '%s' (use '?' for help)"),
> +			    s->answer.buf);
>  		}
>  	}
>  

I was wondering why we have `err()` here, and whether we shouldn't
convert this to use `error()` instead. Similarly, I was wondering
whether we should convert the error message to start with a lower-case
letter to match our other errors.

But scanning through "add-patch.c" I don't think that makes sense.
`err()` knows to handle colors, which `error()` doesn't. And given that
this is an interactive interface where all the other error messages
start with an upper-case letter, too, it would feel out of place to
adapt the error message.

So all of this is just to say that this looks sensible to me.

> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index bc55255b0a..b38fd5388a 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -61,6 +61,16 @@ test_expect_success 'setup (initial)' '
>  	echo more >>file &&
>  	echo lines >>file
>  '
> +
> +test_expect_success 'invalid option' '
> +	cat >expect <<-EOF &&
> +	Unknown option ${SQ}W${SQ} (use ${SQ}?${SQ} for help)
> +	EOF
> +	test_write_lines W |
> +	git -c core.filemode=true add -p 2>actual &&

Nit: it might be sensible to write the lines into a temporary file first
so that Git isn't spawned as part of a pipeline. But on the other hand
it's fine to have Git on the right-hand side of pipelines, so this way
to write it is fine, too.

Patrick

> +	test_cmp expect actual
> +'
> +
>  test_expect_success 'status works (initial)' '
>  	git add -i </dev/null >output &&
>  	grep "+1/-0 *+2/-0 file" output
> -- 
> 2.44.0.782.g480309b2c8
> 
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] add-patch: response to invalid option

[Rubén Justo]

On Tue, Apr 16, 2024 at 07:51:35AM +0200, Patrick Steinhardt wrote:

> I think it would've made sense to describe this change in the commit
> message, as well. Currently, the reader is left wondering whether "?" is
> a new shortcut that you introduce in this patch or whether it is already
> documented as such.

Perhaps, something like?:

	Instead of displaying the long help description, display a short
	error message indicating the incorrectly introduced option with
	a note on how to use the '?' option, to get the help text.

Thanks.

Re: [PATCH] add-patch: response to invalid option

[phillip.wood123]

Hi Rubén

Thanks for working on this, it is a nice follow up to your last series.

On 15/04/2024 20:00, Rubén Justo wrote:
>
> +		} else {
> +			err(s, _("Unknown option '%s' (use '?' for help)"),

As this is an interactive program I think "Unknown key" would be clearer.

> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index bc55255b0a..b38fd5388a 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -61,6 +61,16 @@ test_expect_success 'setup (initial)' '
>   	echo more >>file &&
>   	echo lines >>file
>   '
> +
> +test_expect_success 'invalid option' '
> +	cat >expect <<-EOF &&
> +	Unknown option ${SQ}W${SQ} (use ${SQ}?${SQ} for help)
> +	EOF
> +	test_write_lines W |
> +	git -c core.filemode=true add -p 2>actual &&
> +	test_cmp expect actual
> +'

I was confused by this test as "add -p" doesn't seem to be printing
anything apart from the error. That's because it only captures stderr
(quite why an interactive program is writing its some of its output to
stderr and the rest to stdout is another question). I think we want to
capture the whole output otherwise we can't assert that the help isn't
printed. Something like this (which also adds coverage for '?' and 'p')

diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index bc55255b0a8..0fc7d4b5d89 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -1130,4 +1130,26 @@ test_expect_success 'reset -p with unmerged files' '
          test_must_be_empty staged
  '
  
+test_expect_success 'invalid key' '
+        echo changed >file &&
+        test_write_lines æ \? p q | force_color git add -p >actual.colored 2>&1 &&
+        test_decode_color <actual.colored >actual &&
+        force_color git diff >diff.colored &&
+        test_decode_color <diff.colored >diff.decoded &&
+        cat diff.decoded >expect &&
+        cat >>expect <<-EOF &&
+        <BOLD;BLUE>(1/1) Stage this hunk [y,n,q,a,d,e,p,?]? <RESET><BOLD;RED>Unknown key ${SQ}æ${SQ}. Use ${SQ}?${SQ} for help<RESET>
+        <BOLD;BLUE>(1/1) Stage this hunk [y,n,q,a,d,e,p,?]? <RESET><BOLD;RED>y - stage this hunk
+        n - do not stage this hunk
+        q - quit; do not stage this hunk or any of the remaining ones
+        a - stage this hunk and all later hunks in the file
+        d - do not stage this hunk or any of the later hunks in the file
+        <RESET><BOLD;RED>e - manually edit the current hunk<RESET>
+        <BOLD;RED>p - print the current hunk<RESET>
+        <BOLD;RED>? - print help<RESET>
+        <BOLD;BLUE>(1/1) Stage this hunk [y,n,q,a,d,e,p,?]? <RESET>$(sed -n "/@/,\$ p" diff.decoded)
+        <BOLD;BLUE>(1/1) Stage this hunk [y,n,q,a,d,e,p,?]? <RESET>
+        EOF
+        test_cmp expect actual
+'
+
  test_done

Best Wishes

Phillip

Re: [PATCH] add-patch: response to invalid option

[Rubén Justo]

On Tue, Apr 16, 2024 at 10:41:32AM +0100, phillip.wood123@gmail.com wrote:

> Thanks for working on this, it is a nice follow up to your last series.

Thank you for your interest!

> 
> On 15/04/2024 20:00, Rubén Justo wrote:
> > 
> > +		} else {
> > +			err(s, _("Unknown option '%s' (use '?' for help)"),
> 
> As this is an interactive program I think "Unknown key" would be clearer.

Maybe you have "interactive.singleKey" set to "true"?

Perhaps "key" refers more to the key pressed by the user.  My impression
is that "option" have a clearer and wider audience.

> Something like this (which also adds coverage for '?' and 'p')

I know we don't have a test for the help.  I noticed this in my other
series.  And I decided not to include a test for it then.  Maybe in this
series it makes sense.

However, I would prefer to keep a test only on the new error message.

Thanks.

Re: [PATCH] add-patch: response to invalid option

[phillip.wood123]

Hi Rubén

On 16/04/2024 20:24, Rubén Justo wrote:
> On Tue, Apr 16, 2024 at 10:41:32AM +0100, phillip.wood123@gmail.com wrote:
>> On 15/04/2024 20:00, Rubén Justo wrote:
>>>
>>> +		} else {
>>> +			err(s, _("Unknown option '%s' (use '?' for help)"),
>>
>> As this is an interactive program I think "Unknown key" would be clearer.
> 
> Maybe you have "interactive.singleKey" set to "true"?
> 
> Perhaps "key" refers more to the key pressed by the user.  My impression
> is that "option" have a clearer and wider audience.

I tend to associate "option" with a command-line argument, not 
interactive input to a program.

>> Something like this (which also adds coverage for '?' and 'p')
> 
> I know we don't have a test for the help.  I noticed this in my other
> series.  And I decided not to include a test for it then.  Maybe in this
> series it makes sense.
> 
> However, I would prefer to keep a test only on the new error message.

Why? This commit makes three changes
  - it stops printing the help when the user presses an unknown key
  - it starts treating '?' differently to an unknown key
  - it starts printing an error message when the user presses an unknown
    key

The test you are proposing only tests the last of these changes. We 
should be aiming to write tests that (a) verify all of the changes 
introduced by a commit (b) are likely to detect regressions to those 
changes (c) are reasonably efficient, for example if it is possible to 
test more than one key with a single "add -p" process we should do so. 
As this is an interactive program I have a strong preference for testing 
what the user sees printed to their screen, not just what happens to 
come out on stderr.

Thanks

Phillip

Re: [PATCH] add-patch: response to invalid option

[Junio C Hamano]

phillip.wood123@gmail.com writes:

> I tend to associate "option" with a command-line argument, not
> interactive input to a program.

"git add --help" is a bit mixed.  The choices offered by "git add
-i" are called "subcommand" (see "INTERACTIVE MODE" section), but
the choices you give to the prompt "patch" subcommand gives you are
presented with "You can select one of the following options and type
return".  So "option" is not too wrong, even though it is a word
used in other contexts as well.  I am OK with "option", but if I
were adding this new error message, I probably would have said
"unknown command".

In any case, whether you said option, command, or key , it is so
obvious from the context that we could even say "error: 'W' not
known, use '?' for help" without any noun there, so it would not
matter too much which noun you pick.

I'd still avoid "key", though, because to those who do not do
single-key input, myself included, it does not match their user
experience, and it is even more so if they forgot or do not even
know that they could choose to use single-key input.

> The test you are proposing only tests the last of these changes. We
> should be aiming to write tests that (a) verify all of the changes
> introduced by a commit (b) are likely to detect regressions to those
> changes (c) are reasonably efficient, for example if it is possible to
> test more than one key with a single "add -p" process we should do
> so. As this is an interactive program I have a strong preference for
> testing what the user sees printed to their screen, not just what
> happens to come out on stderr.

I do agree with these three points, but I do not have a strong
opinion on the new test that was added by the patch when judging
with them used as a yardstick.

Thanks.

Re: [PATCH] add-patch: response to invalid option

[phillip.wood123]

On 17/04/2024 16:05, Junio C Hamano wrote:
> phillip.wood123@gmail.com writes:
> 
>> I tend to associate "option" with a command-line argument, not
>> interactive input to a program.
> 
> "git add --help" is a bit mixed.  The choices offered by "git add
> -i" are called "subcommand" (see "INTERACTIVE MODE" section), but
> the choices you give to the prompt "patch" subcommand gives you are
> presented with "You can select one of the following options and type
> return".  So "option" is not too wrong, even though it is a word
> used in other contexts as well.  I am OK with "option", but if I
> were adding this new error message, I probably would have said
> "unknown command".

I think "unknown command" is a good suggestion, I take your point about 
"unknown key" not being so clear for users who do not use single-key input.

Best Wishes

Phillip

> In any case, whether you said option, command, or key , it is so
> obvious from the context that we could even say "error: 'W' not
> known, use '?' for help" without any noun there, so it would not
> matter too much which noun you pick.
> 
> I'd still avoid "key", though, because to those who do not do
> single-key input, myself included, it does not match their user
> experience, and it is even more so if they forgot or do not even
> know that they could choose to use single-key input.
> 
>> The test you are proposing only tests the last of these changes. We
>> should be aiming to write tests that (a) verify all of the changes
>> introduced by a commit (b) are likely to detect regressions to those
>> changes (c) are reasonably efficient, for example if it is possible to
>> test more than one key with a single "add -p" process we should do
>> so. As this is an interactive program I have a strong preference for
>> testing what the user sees printed to their screen, not just what
>> happens to come out on stderr.
> 
> I do agree with these three points, but I do not have a strong
> opinion on the new test that was added by the patch when judging
> with them used as a yardstick.
> 
> Thanks.

Re: [PATCH] add-patch: response to invalid option

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

> When the user introduces an invalid option, we respond with the whole
> help text.

The verb "introduces" in the first sentence looked weird to me.

    add -p: require two steps to get help after mistyping an option

    During a "git add -p" session, if the user chooses an option
    that is not offered, the help text for the entire available
    choices is given.

or something, perhaps.

> Instead of displaying the long help description, display a short error
> message indicating the incorrectly introduced option with a note on how
> to get the help text.

When you wanted to 'q'uit but touched the 'w' key that sits next to
it by mistake, you do not need to be told about what any of the
options would do; you may want to be told "the option to quit is
'q', not 'w'", though ;-).

On the other hand, if you are truly lost and do not know what each
of the listed choices mean, you'd type '?' anyway because that is
one of the offered choices.  So the only change needed here is to
make sure that '?' is the only thing that gives the help message,
and all other unrecognised option 'x' are made to say "we do not
know 'x'".

That flow of thought makes sort-of sense, if the choices that are
offered are too numerous (say, around a dozen or more), but with the
current command set, I am not sure if this change is an improvement
(note: I did not say "I do not think that"---I simply am not sure).

If we implemented the UI this way 20 years ago in the first version,
perhaps we would have had happily been using it since, but given
that the way we implemented the UI 20 years ago has been used
happily by our users without much complaint, either way must be just
fine.  

Is it worth changing it at this point?  Does it improve the end-user
experience in any noticeable way?  I do not think I can answer these
two questions with confident "yes".

> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
>  add-patch.c                |  5 ++++-
>  t/t3701-add-interactive.sh | 10 ++++++++++
>  2 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/add-patch.c b/add-patch.c
> index a06dd18985..c77902fec5 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -1667,7 +1667,7 @@ static int patch_update_file(struct add_p_state *s,
>  			}
>  		} else if (s->answer.buf[0] == 'p') {
>  			rendered_hunk_index = -1;
> -		} else {
> +		} else if (s->answer.buf[0] == '?') {
>  			const char *p = _(help_patch_remainder), *eol = p;
>  
>  			color_fprintf(stdout, s->s.help_color, "%s",
> @@ -1691,6 +1691,9 @@ static int patch_update_file(struct add_p_state *s,
>  				color_fprintf_ln(stdout, s->s.help_color,
>  						 "%.*s", (int)(eol - p), p);
>  			}
> +		} else {
> +			err(s, _("Unknown option '%s' (use '?' for help)"),
> +			    s->answer.buf);
>  		}
>  	}
>  
> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index bc55255b0a..b38fd5388a 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -61,6 +61,16 @@ test_expect_success 'setup (initial)' '
>  	echo more >>file &&
>  	echo lines >>file
>  '
> +
> +test_expect_success 'invalid option' '
> +	cat >expect <<-EOF &&
> +	Unknown option ${SQ}W${SQ} (use ${SQ}?${SQ} for help)
> +	EOF
> +	test_write_lines W |
> +	git -c core.filemode=true add -p 2>actual &&
> +	test_cmp expect actual
> +'
> +
>  test_expect_success 'status works (initial)' '
>  	git add -i </dev/null >output &&
>  	grep "+1/-0 *+2/-0 file" output

Re: [PATCH] add-patch: response to invalid option

[Phillip Wood]

Hi Junio

On 16/04/2024 11:26, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:
> 
>> When the user introduces an invalid option, we respond with the whole
>> help text.
> 
> The verb "introduces" in the first sentence looked weird to me.
> 
>      add -p: require two steps to get help after mistyping an option
> 
>      During a "git add -p" session, if the user chooses an option
>      that is not offered, the help text for the entire available
>      choices is given.

I find this suggestion clearer as well.

> [...] 
> On the other hand, if you are truly lost and do not know what each
> of the listed choices mean, you'd type '?' anyway because that is
> one of the offered choices.  So the only change needed here is to
> make sure that '?' is the only thing that gives the help message,
> and all other unrecognised option 'x' are made to say "we do not
> know 'x'".
> 
> That flow of thought makes sort-of sense, if the choices that are
> offered are too numerous (say, around a dozen or more), but with the
> current command set, I am not sure if this change is an improvement
> (note: I did not say "I do not think that"---I simply am not sure).

The complete list of help is 15 lines long (we don't always print 
everything but if you're in the middle of staging several hunks from the 
same file we do)

   y - stage this hunk
   n - do not stage this hunk
   q - quit; do not stage this hunk or any of the remaining ones
   a - stage this hunk and all later hunks in the file
   d - do not stage this hunk or any of the later hunks in the file
   j - leave this hunk undecided, see next undecided hunk
   J - leave this hunk undecided, see next hunk
   k - leave this hunk undecided, see previous undecided hunk
   K - leave this hunk undecided, see previous hunk
   g - select a hunk to go to
   / - search for a hunk matching the given regex
   s - split the current hunk into smaller hunks
   e - manually edit the current hunk
   p - print the current hunk
   ? - print help

I find it long enough to be annoying as it means there is a significant 
distance between the prompt and the hunk text.

> If we implemented the UI this way 20 years ago in the first version,
> perhaps we would have had happily been using it since, but given
> that the way we implemented the UI 20 years ago has been used
> happily by our users without much complaint, either way must be just
> fine.
> 
> Is it worth changing it at this point?  Does it improve the end-user
> experience in any noticeable way?  I do not think I can answer these
> two questions with confident "yes".

Personally I think it would be an improvement but I suspect it is a 
matter of opinion.

Best Wishes

Phillip

>> Signed-off-by: Rubén Justo <rjusto@gmail.com>
>> ---
>>   add-patch.c                |  5 ++++-
>>   t/t3701-add-interactive.sh | 10 ++++++++++
>>   2 files changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/add-patch.c b/add-patch.c
>> index a06dd18985..c77902fec5 100644
>> --- a/add-patch.c
>> +++ b/add-patch.c
>> @@ -1667,7 +1667,7 @@ static int patch_update_file(struct add_p_state *s,
>>   			}
>>   		} else if (s->answer.buf[0] == 'p') {
>>   			rendered_hunk_index = -1;
>> -		} else {
>> +		} else if (s->answer.buf[0] == '?') {
>>   			const char *p = _(help_patch_remainder), *eol = p;
>>   
>>   			color_fprintf(stdout, s->s.help_color, "%s",
>> @@ -1691,6 +1691,9 @@ static int patch_update_file(struct add_p_state *s,
>>   				color_fprintf_ln(stdout, s->s.help_color,
>>   						 "%.*s", (int)(eol - p), p);
>>   			}
>> +		} else {
>> +			err(s, _("Unknown option '%s' (use '?' for help)"),
>> +			    s->answer.buf);
>>   		}
>>   	}
>>   
>> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
>> index bc55255b0a..b38fd5388a 100755
>> --- a/t/t3701-add-interactive.sh
>> +++ b/t/t3701-add-interactive.sh
>> @@ -61,6 +61,16 @@ test_expect_success 'setup (initial)' '
>>   	echo more >>file &&
>>   	echo lines >>file
>>   '
>> +
>> +test_expect_success 'invalid option' '
>> +	cat >expect <<-EOF &&
>> +	Unknown option ${SQ}W${SQ} (use ${SQ}?${SQ} for help)
>> +	EOF
>> +	test_write_lines W |
>> +	git -c core.filemode=true add -p 2>actual &&
>> +	test_cmp expect actual
>> +'
>> +
>>   test_expect_success 'status works (initial)' '
>>   	git add -i </dev/null >output &&
>>   	grep "+1/-0 *+2/-0 file" output
> 

Re: [PATCH] add-patch: response to invalid option

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

>> That flow of thought makes sort-of sense, if the choices that are
>> offered are too numerous (say, around a dozen or more), but with the
>> current command set, I am not sure if this change is an improvement
>> (note: I did not say "I do not think that"---I simply am not sure).
>
> The complete list of help is 15 lines long (we don't always print
> everything but if you're in the middle of staging several hunks from
> the same file we do)

Ah, OK.  Even though I was the one who stole this feature from
elsewhere, I forgot that we had that many commands, and it makes the
decision a lot simpler ;-)

>   y - stage this hunk
>   n - do not stage this hunk
>   q - quit; do not stage this hunk or any of the remaining ones
>   a - stage this hunk and all later hunks in the file
>   d - do not stage this hunk or any of the later hunks in the file
>   j - leave this hunk undecided, see next undecided hunk
>   J - leave this hunk undecided, see next hunk
>   k - leave this hunk undecided, see previous undecided hunk
>   K - leave this hunk undecided, see previous hunk
>   g - select a hunk to go to
>   / - search for a hunk matching the given regex
>   s - split the current hunk into smaller hunks
>   e - manually edit the current hunk
>   p - print the current hunk
>   ? - print help
>
> I find it long enough to be annoying as it means there is a
> significant distance between the prompt and the hunk text.

Yes, I now agree.

So the log message may want a bit of rewrite in the title and the
introductory part, with a clarification that '?' is what we already
had and not what this adds (from Patrick's comment), and there may
be other small improvements we want to see that I may have missed,
but other than these small-ish points, the basic direction of this
patch is good?

This is a tangent, but I think we _mostly_ take these commands case
insensitively (e.g., you can say 'Q' to quit the program, as well as
'q' that is listed above).  But 'k' and 'j' (there may be others)
act differently when given in the uppercase.  This would limit our
ability to later add capitalized variant of an already used lower
case letter without retraining users---should we find it disturbing,
or is "add -p" mostly done and we do not have to worry?

Thanks.

Re: [PATCH] add-patch: response to invalid option

[Phillip Wood]

On 16/04/2024 16:22, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>>> That flow of thought makes sort-of sense, if the choices that are
>>> offered are too numerous (say, around a dozen or more), but with the
>>> current command set, I am not sure if this change is an improvement
>>> (note: I did not say "I do not think that"---I simply am not sure).
>>
>> The complete list of help is 15 lines long (we don't always print
>> everything but if you're in the middle of staging several hunks from
>> the same file we do)
> 
> Ah, OK.  Even though I was the one who stole this feature from
> elsewhere, I forgot that we had that many commands, and it makes the
> decision a lot simpler ;-)

Yes, it always surprises me how many there are when the help is printed.

> So the log message may want a bit of rewrite in the title and the
> introductory part, with a clarification that '?' is what we already
> had and not what this adds (from Patrick's comment), and there may
> be other small improvements we want to see that I may have missed,
> but other than these small-ish points, the basic direction of this
> patch is good?

I think so.

> This is a tangent, but I think we _mostly_ take these commands case
> insensitively (e.g., you can say 'Q' to quit the program, as well as
> 'q' that is listed above).  But 'k' and 'j' (there may be others)
> act differently when given in the uppercase.

'J' and 'K' are the only keys where we have different actions for upper 
and lowers cases. The implementation looks a bit confused though - we 
treat y,n,q,a,d case insensitively but not g,s,e,p. As no one has 
complained about the latter we should perhaps leave them as is in case 
we ever want to add upper case variants.

>  This would limit our
> ability to later add capitalized variant of an already used lower
> case letter without retraining users---should we find it disturbing,
> or is "add -p" mostly done and we do not have to worry?

I think its mostly done. I'd like a way of selecting lines from a hunk 
that does not involve making the user edit the hunk and I wish the 
search remembered the last string but I don't think we're in danger of 
running out of keys for new commands.

Best Wishes

Phillip

Re: [PATCH] add-patch: response to invalid option

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

>> Ah, OK.  Even though I was the one who stole this feature from
>> elsewhere, I forgot that we had that many commands, and it makes the
>> decision a lot simpler ;-)
>
> Yes, it always surprises me how many there are when the help is printed.
>
>> So the log message may want a bit of rewrite in the title and the
>> introductory part, with a clarification that '?' is what we already
>> had and not what this adds (from Patrick's comment), and there may
>> be other small improvements we want to see that I may have missed,
>> but other than these small-ish points, the basic direction of this
>> patch is good?
>
> I think so.

OK, let me mark the topic as expecting hopefully a final reroll
in the draft "What's cooking" report.

Thanks.

Re: [PATCH] add-patch: response to invalid option

[Rubén Justo]

On Tue, Apr 16, 2024 at 03:26:33AM -0700, Junio C Hamano wrote:

> > When the user introduces an invalid option, we respond with the whole
> > help text.
> 
> The verb "introduces" in the first sentence looked weird to me.

OK.  I'll reword it.

> > Instead of displaying the long help description, display a short error
> > message indicating the incorrectly introduced option with a note on how
> > to get the help text.

> Is it worth changing it at this point?  Does it improve the end-user
> experience in any noticeable way?  I do not think I can answer these
> two questions with confident "yes".

Indeed, this has been with us for a long time.  We're not fixing or
changing any normal usage here.

I know you know, but let me put it this way;  We are introducing two
changes here:

   - a new error message to inform the user that an invalid option has
     been entered.  And,

   - not displaying the help if not requested.

Both are improvements to the user experience, but especially the former,
I think.

This:

	$ echo W | git add -p 
	diff --git a/add-interactive.c b/add-interactive.c
	[...]
	(1/1) Stage this hunk [y,n,q,a,d,e,?]? y - stage this hunk
	n - do not stage this hunk
	q - quit; do not stage this hunk or any of the remaining ones
	a - stage this hunk and all later hunks in the file
	[...]

Becomes:

	$ echo W | ./git add -p
	diff --git a/add-interactive.c b/add-interactive.c
	[...]
	(1/1) Stage this hunk [y,n,q,a,d,e,p,?]? Unknown option 'W' (use '?' for help)

Thanks.

[PATCH v2] add-patch: response to invalid command

[Rubén Justo]

When the user enters an invalid command, we respond with a list of
accepted commands; the response we give to the command '?'.

However, the invalid command may be due to either a user input error or
a malfunctioning interface component, rather than the user not knowing
the valid command.

Our response is unlikely to provide help in such situations.

To reduce the likelihood of user confusion and error repetition, if an
unrecognized command is received, stop displaying the help text and
display a short error message with the invalid command received, as
feedback to the user.

Include a reminder about the current command '?' in the new message, to
guide the user if they want help.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---

Changes since v1:

  - Use a temporary file for the lines.
  - Say explicitly in the message that '?' is an existing command.
  - Instead of "option", use "command".
  - Test for stdout, not just stderr.

 add-patch.c                |  5 ++++-
 t/t3701-add-interactive.sh | 23 ++++++++++++++++++++++-
 2 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index a06dd18985..7be142d448 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1667,7 +1667,7 @@ static int patch_update_file(struct add_p_state *s,
 			}
 		} else if (s->answer.buf[0] == 'p') {
 			rendered_hunk_index = -1;
-		} else {
+		} else if (s->answer.buf[0] == '?') {
 			const char *p = _(help_patch_remainder), *eol = p;
 
 			color_fprintf(stdout, s->s.help_color, "%s",
@@ -1691,6 +1691,9 @@ static int patch_update_file(struct add_p_state *s,
 				color_fprintf_ln(stdout, s->s.help_color,
 						 "%.*s", (int)(eol - p), p);
 			}
+		} else {
+			err(s, _("Unknown command '%s' (use '?' for help)"),
+			    s->answer.buf);
 		}
 	}
 
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index bc55255b0a..4c3901de17 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -7,6 +7,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-terminal.sh
 
+SP=" "
+
 diff_cmp () {
 	for x
 	do
@@ -61,6 +63,26 @@ test_expect_success 'setup (initial)' '
 	echo more >>file &&
 	echo lines >>file
 '
+
+test_expect_success 'unknown command' '
+	test_when_finished "git reset command && rm command" &&
+	echo W >command &&
+	git add -N command &&
+	cat >expect <<-EOF &&
+	diff --git a/command b/command
+	new file mode 100644
+	index 0000000..a42d8ff
+	--- /dev/null
+	+++ b/command
+	@@ -0,0 +1 @@
+	+W
+	(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help)
+	(1/1) Stage addition [y,n,q,a,d,e,p,?]?$SP
+	EOF
+	git add -p -- command <command >actual 2>&1 &&
+	test_cmp expect actual
+'
+
 test_expect_success 'status works (initial)' '
 	git add -i </dev/null >output &&
 	grep "+1/-0 *+2/-0 file" output
@@ -231,7 +253,6 @@ test_expect_success 'setup file' '
 '
 
 test_expect_success 'setup patch' '
-	SP=" " &&
 	NULL="" &&
 	cat >patch <<-EOF
 	@@ -1,4 +1,4 @@
-- 
2.45.0.rc0.1.gc94c838fad

Re: [PATCH v2] add-patch: response to invalid command

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

> When the user enters an invalid command, we respond with a list of
> accepted commands; the response we give to the command '?'.

I am guessing that you want to say with the part of the sentence
after the semicolon that the list of accepted commands and the
explanation for them is the same as what we give when the user asks
'?', but that is after reading the above twice.  I wonder

    When the user gives an unknown command to the "add -p" prompt,
    the list of accepted commands with their explanation is given.
    This is the same output they get when they say `?`.

is easier to understand?

> However, the invalid command may be due to either a user input error or
> a malfunctioning interface component, rather than the user not knowing
> the valid command.

I am not sure readers would understand what you are trying to refer
to with "or a malfunctioning interface component".  I don't, at
least.  Either rewrite it to be understandable, or drop it.  I think
dropping it is sufficient in this case.

> Our response is unlikely to provide help in such situations.
>
> To reduce the likelihood of user confusion and error repetition, if an
> unrecognized command is received, stop displaying the help text and
> display a short error message with the invalid command received, as
> feedback to the user.

"stop ... text and" -> "instead of ... text,"

It would give a better contrast between the current UI and the
proposed new one.

> Include a reminder about the current command '?' in the new message, to
> guide the user if they want help.

OK.

> diff --git a/add-patch.c b/add-patch.c
> index a06dd18985..7be142d448 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -1667,7 +1667,7 @@ static int patch_update_file(struct add_p_state *s,
>  			}
>  		} else if (s->answer.buf[0] == 'p') {
>  			rendered_hunk_index = -1;
> -		} else {
> +		} else if (s->answer.buf[0] == '?') {
>  			const char *p = _(help_patch_remainder), *eol = p;
>  
>  			color_fprintf(stdout, s->s.help_color, "%s",
> @@ -1691,6 +1691,9 @@ static int patch_update_file(struct add_p_state *s,
>  				color_fprintf_ln(stdout, s->s.help_color,
>  						 "%.*s", (int)(eol - p), p);
>  			}
> +		} else {
> +			err(s, _("Unknown command '%s' (use '?' for help)"),
> +			    s->answer.buf);
>  		}
>  	}

Looking good.

> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index bc55255b0a..4c3901de17 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -7,6 +7,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>  . ./test-lib.sh
>  . "$TEST_DIRECTORY"/lib-terminal.sh
>  
> +SP=" "

Good to see what is already used in the existing tests reused.

>  diff_cmp () {
>  	for x
>  	do
> @@ -61,6 +63,26 @@ test_expect_success 'setup (initial)' '
>  	echo more >>file &&
>  	echo lines >>file
>  '
> +
> +test_expect_success 'unknown command' '
> +	test_when_finished "git reset command && rm command" &&

Generally, use of && in test_when_finished is a contradiction.  Even
if other things fail, you want "command" to be gone befor ethe next
step, and that is why you use test_when_finished to arrange it to be
cleaned.  Similarly, even if "git reset command" fails here, you
would want command to be cleaned.

What is the expected state of the test repository before we start
this test?  If it is that "git status -uno" would be silent (i.e.,
no paths known to the index and the HEAD are modified in the index
or in the working tree, but there may be random untracked cruft left
in the working tree, like "expect" and stuff), then it makes more
sense to make it easier to read that expectation by doing

	test_when_finished "git reset --hard; rm -f command"

here.  Instead of pretending that we care about making the minimum
damage by selectively resetting the index, we make it clear that we
are giving the _next_ test a pristine state of the index and the
working tree as we inherited from the previous test.

> +	echo W >command &&
> +	git add -N command &&
> +	cat >expect <<-EOF &&
> +	diff --git a/command b/command
> +	new file mode 100644
> +	index 0000000..a42d8ff
> +	--- /dev/null
> +	+++ b/command
> +	@@ -0,0 +1 @@
> +	+W
> +	(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help)
> +	(1/1) Stage addition [y,n,q,a,d,e,p,?]?$SP
> +	EOF
> +	git add -p -- command <command >actual 2>&1 &&
> +	test_cmp expect actual
> +'
> +
>  test_expect_success 'status works (initial)' '
>  	git add -i </dev/null >output &&
>  	grep "+1/-0 *+2/-0 file" output
> @@ -231,7 +253,6 @@ test_expect_success 'setup file' '
>  '
>  
>  test_expect_success 'setup patch' '
> -	SP=" " &&
>  	NULL="" &&
>  	cat >patch <<-EOF
>  	@@ -1,4 +1,4 @@

Otherwise, looking very good.
Will queue.

Thanks.

[PATCH v3] add-patch: response to unknown command

[Rubén Justo]

When the user gives an unknown command to the "add -p" prompt, the list
of accepted commands with their explanation is given.  This is the same
output they get when they say '?'.

However, the unknown command may be due to a user input error rather
than the user not knowing the valid command.

To reduce the likelihood of user confusion and error repetition, instead
of displaying the list of accepted commands, display a short error
message with the unknown command received, as feedback to the user.

Include a reminder about the current command '?' in the new message, to
guide the user if they want help.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---

Thanks.

Range-diff against v2:
1:  c94c838fad ! 1:  ddd8d380a7 add-patch: response to invalid command
    @@ Metadata
     Author: Rubén Justo <rjusto@gmail.com>
     
      ## Commit message ##
    -    add-patch: response to invalid command
    +    add-patch: response to unknown command
     
    -    When the user enters an invalid command, we respond with a list of
    -    accepted commands; the response we give to the command '?'.
    +    When the user gives an unknown command to the "add -p" prompt, the list
    +    of accepted commands with their explanation is given.  This is the same
    +    output they get when they say '?'.
     
    -    However, the invalid command may be due to either a user input error or
    -    a malfunctioning interface component, rather than the user not knowing
    -    the valid command.
    +    However, the unknown command may be due to a user input error rather
    +    than the user not knowing the valid command.
     
    -    Our response is unlikely to provide help in such situations.
    -
    -    To reduce the likelihood of user confusion and error repetition, if an
    -    unrecognized command is received, stop displaying the help text and
    -    display a short error message with the invalid command received, as
    -    feedback to the user.
    +    To reduce the likelihood of user confusion and error repetition, instead
    +    of displaying the list of accepted commands, display a short error
    +    message with the unknown command received, as feedback to the user.
     
         Include a reminder about the current command '?' in the new message, to
         guide the user if they want help.
    @@ t/t3701-add-interactive.sh: export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
      diff_cmp () {
      	for x
      	do
    -@@ t/t3701-add-interactive.sh: test_expect_success 'setup (initial)' '
    - 	echo more >>file &&
    - 	echo lines >>file
    +@@ t/t3701-add-interactive.sh: test_expect_success 'warn about add.interactive.useBuiltin' '
    + 	done
      '
    -+
    + 
     +test_expect_success 'unknown command' '
    -+	test_when_finished "git reset command && rm command" &&
    ++	test_when_finished "git reset --hard; rm -f command" &&
     +	echo W >command &&
     +	git add -N command &&
     +	cat >expect <<-EOF &&
    @@ t/t3701-add-interactive.sh: test_expect_success 'setup (initial)' '
     +	test_cmp expect actual
     +'
     +
    - test_expect_success 'status works (initial)' '
    - 	git add -i </dev/null >output &&
    - 	grep "+1/-0 *+2/-0 file" output
    + test_expect_success 'setup (initial)' '
    + 	echo content >file &&
    + 	git add file &&
     @@ t/t3701-add-interactive.sh: test_expect_success 'setup file' '
      '
      

 add-patch.c                |  5 ++++-
 t/t3701-add-interactive.sh | 22 +++++++++++++++++++++-
 2 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index a06dd18985..7be142d448 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1667,7 +1667,7 @@ static int patch_update_file(struct add_p_state *s,
 			}
 		} else if (s->answer.buf[0] == 'p') {
 			rendered_hunk_index = -1;
-		} else {
+		} else if (s->answer.buf[0] == '?') {
 			const char *p = _(help_patch_remainder), *eol = p;
 
 			color_fprintf(stdout, s->s.help_color, "%s",
@@ -1691,6 +1691,9 @@ static int patch_update_file(struct add_p_state *s,
 				color_fprintf_ln(stdout, s->s.help_color,
 						 "%.*s", (int)(eol - p), p);
 			}
+		} else {
+			err(s, _("Unknown command '%s' (use '?' for help)"),
+			    s->answer.buf);
 		}
 	}
 
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index bc55255b0a..d974bddcf2 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -7,6 +7,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-terminal.sh
 
+SP=" "
+
 diff_cmp () {
 	for x
 	do
@@ -55,6 +57,25 @@ test_expect_success 'warn about add.interactive.useBuiltin' '
 	done
 '
 
+test_expect_success 'unknown command' '
+	test_when_finished "git reset --hard; rm -f command" &&
+	echo W >command &&
+	git add -N command &&
+	cat >expect <<-EOF &&
+	diff --git a/command b/command
+	new file mode 100644
+	index 0000000..a42d8ff
+	--- /dev/null
+	+++ b/command
+	@@ -0,0 +1 @@
+	+W
+	(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help)
+	(1/1) Stage addition [y,n,q,a,d,e,p,?]?$SP
+	EOF
+	git add -p -- command <command >actual 2>&1 &&
+	test_cmp expect actual
+'
+
 test_expect_success 'setup (initial)' '
 	echo content >file &&
 	git add file &&
@@ -231,7 +252,6 @@ test_expect_success 'setup file' '
 '
 
 test_expect_success 'setup patch' '
-	SP=" " &&
 	NULL="" &&
 	cat >patch <<-EOF
 	@@ -1,4 +1,4 @@
-- 
2.45.0.rc0.1.gddd8d380a7

Re: [PATCH v3] add-patch: response to unknown command

[phillip.wood123]

Hi Rubén

This is looking good, I've left a couple of comments on the test.

On 21/04/2024 10:51, Rubén Justo wrote:
> When the user gives an unknown command to the "add -p" prompt, the list
> of accepted commands with their explanation is given.  This is the same
> output they get when they say '?'.
> 
> However, the unknown command may be due to a user input error rather
> than the user not knowing the valid command.
> 
> To reduce the likelihood of user confusion and error repetition, instead
> of displaying the list of accepted commands, display a short error
> message with the unknown command received, as feedback to the user.
> 
> Include a reminder about the current command '?' in the new message, to
> guide the user if they want help.
> 
> Signed-off-by: Rubén Justo <rjusto@gmail.com>

> +test_expect_success 'unknown command' '
> +	test_when_finished "git reset --hard; rm -f command" &&
> +	echo W >command &&

I find separating out writing this file from where it is used makes it 
harder to see the input to "git add -p" using test_write_lines as you 
did in v1 matches the existing tests and is perfectly fine.

> +	git add -N command &&
> +	cat >expect <<-EOF &&
> +	diff --git a/command b/command
> +	new file mode 100644
> +	index 0000000..a42d8ff
> +	--- /dev/null
> +	+++ b/command
> +	@@ -0,0 +1 @@
> +	+W
> +	(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help)
> +	(1/1) Stage addition [y,n,q,a,d,e,p,?]?$SP
> +	EOF
> +	git add -p -- command <command >actual 2>&1 &&
> +	test_cmp expect actual

It is really good to see us testing the whole of the program output now 
but have you tested this with GIT_TEST_DEFAULT_HASH=sha256? All the 
other tests use diff_cmp() to avoid test failures caused by the index 
line depending on the hash function. Alternatively you could run "git 
diff" to create that part of the expected output.

It is a shame we're not testing '?' as we have changed the 
implementation - before this commit '?' was just another unknown command 
as far as the implementation was concerned, now we treat it differently. 
It would be nice to see some test coverage for the 'p' command you 
recently added as well at some point.

>   test_expect_success 'setup patch' '
> -	SP=" " &&

Nice attention to detail, this is not needed now SP is defined at the 
beginning of the test file.

Best Wishes

Phillip

Re: [PATCH v3] add-patch: response to unknown command

[Rubén Justo]

On Sun, Apr 21, 2024 at 02:18:32PM +0100, phillip.wood123@gmail.com wrote:

> > +	git add -N command &&
> > +	cat >expect <<-EOF &&
> > +	diff --git a/command b/command
> > +	new file mode 100644
> > +	index 0000000..a42d8ff
> > +	--- /dev/null
> > +	+++ b/command
> > +	@@ -0,0 +1 @@
> > +	+W
> > +	(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help)
> > +	(1/1) Stage addition [y,n,q,a,d,e,p,?]?$SP
> > +	EOF
> > +	git add -p -- command <command >actual 2>&1 &&
> > +	test_cmp expect actual
> 
> It is really good to see us testing the whole of the program output now but
> have you tested this with GIT_TEST_DEFAULT_HASH=sha256? All the other tests
> use diff_cmp() to avoid test failures caused by the index line depending on
> the hash function. Alternatively you could run "git diff" to create that
> part of the expected output.

Oops.  Thank you for pointing this out.  I'll reroll with a fix for it
later today.

> It is a shame we're not testing '?' as we have changed the implementation -
> before this commit '?' was just another unknown command as far as the
> implementation was concerned, now we treat it differently.

I think testing for '?' deserves its own series.

> It would be nice
> to see some test coverage for the 'p' command you recently added as well at
> some point.

Yes.  I'll send some tests for it soon.

Thanks.

[PATCH v4] add-patch: response to unknown command

[Rubén Justo]

When the user gives an unknown command to the "add -p" prompt, the list
of accepted commands with their explanation is given.  This is the same
output they get when they say '?'.

However, the unknown command may be due to a user input error rather
than the user not knowing the valid command.

To reduce the likelihood of user confusion and error repetition, instead
of displaying the list of accepted commands, display a short error
message with the unknown command received, as feedback to the user.

Include a reminder about the current command '?' in the new message, to
guide the user if they want help.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---

The test now pass with GIT_TEST_DEFAULT_HASH=sha256.

Thanks.

Range-diff against v3:
1:  0317594bce ! 1:  b418b03f15 add-patch: response to unknown command
    @@ t/t3701-add-interactive.sh: test_expect_success 'warn about add.interactive.useB
     +	test_when_finished "git reset --hard; rm -f command" &&
     +	echo W >command &&
     +	git add -N command &&
    -+	cat >expect <<-EOF &&
    -+	diff --git a/command b/command
    -+	new file mode 100644
    -+	index 0000000..a42d8ff
    -+	--- /dev/null
    -+	+++ b/command
    -+	@@ -0,0 +1 @@
    -+	+W
    ++	git diff command >expect &&
    ++	cat >>expect <<-EOF &&
     +	(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help)
     +	(1/1) Stage addition [y,n,q,a,d,e,p,?]?$SP
     +	EOF

 add-patch.c                |  5 ++++-
 t/t3701-add-interactive.sh | 16 +++++++++++++++-
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index a06dd18985..7be142d448 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1667,7 +1667,7 @@ static int patch_update_file(struct add_p_state *s,
 			}
 		} else if (s->answer.buf[0] == 'p') {
 			rendered_hunk_index = -1;
-		} else {
+		} else if (s->answer.buf[0] == '?') {
 			const char *p = _(help_patch_remainder), *eol = p;
 
 			color_fprintf(stdout, s->s.help_color, "%s",
@@ -1691,6 +1691,9 @@ static int patch_update_file(struct add_p_state *s,
 				color_fprintf_ln(stdout, s->s.help_color,
 						 "%.*s", (int)(eol - p), p);
 			}
+		} else {
+			err(s, _("Unknown command '%s' (use '?' for help)"),
+			    s->answer.buf);
 		}
 	}
 
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index bc55255b0a..482d5c117e 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -7,6 +7,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-terminal.sh
 
+SP=" "
+
 diff_cmp () {
 	for x
 	do
@@ -55,6 +57,19 @@ test_expect_success 'warn about add.interactive.useBuiltin' '
 	done
 '
 
+test_expect_success 'unknown command' '
+	test_when_finished "git reset --hard; rm -f command" &&
+	echo W >command &&
+	git add -N command &&
+	git diff command >expect &&
+	cat >>expect <<-EOF &&
+	(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help)
+	(1/1) Stage addition [y,n,q,a,d,e,p,?]?$SP
+	EOF
+	git add -p -- command <command >actual 2>&1 &&
+	test_cmp expect actual
+'
+
 test_expect_success 'setup (initial)' '
 	echo content >file &&
 	git add file &&
@@ -231,7 +246,6 @@ test_expect_success 'setup file' '
 '
 
 test_expect_success 'setup patch' '
-	SP=" " &&
 	NULL="" &&
 	cat >patch <<-EOF
 	@@ -1,4 +1,4 @@
-- 
2.45.0.rc0.1.gb418b03f15

Re: [PATCH v4] add-patch: response to unknown command

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

> 1:  0317594bce ! 1:  b418b03f15 add-patch: response to unknown command
>     @@ t/t3701-add-interactive.sh: test_expect_success 'warn about add.interactive.useB
>      +	test_when_finished "git reset --hard; rm -f command" &&
>      +	echo W >command &&
>      +	git add -N command &&
>     -+	cat >expect <<-EOF &&
>     -+	diff --git a/command b/command
>     -+	new file mode 100644
>     -+	index 0000000..a42d8ff
>     -+	--- /dev/null
>     -+	+++ b/command
>     -+	@@ -0,0 +1 @@
>     -+	+W
>     ++	git diff command >expect &&
>     ++	cat >>expect <<-EOF &&
>      +	(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help)
>      +	(1/1) Stage addition [y,n,q,a,d,e,p,?]?$SP
>      +	EOF

Interesting.

My first reaction was "how is this different from checking just the
last line of the actual output?  The early part of expect and actual
both come from an internal invocation of 'git diff', and they must
match by definition".

But that may really be the point of this test.

That is, we may later decide to tweak the way "git diff" hunks are
presented, and we expect that the way "git add -p" presents the
hunks would change together with it automatically.

Re: [PATCH v4] add-patch: response to unknown command

[phillip.wood123]

Hi Junio

On 22/04/2024 16:50, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:
> 
>> 1:  0317594bce ! 1:  b418b03f15 add-patch: response to unknown command
>>      @@ t/t3701-add-interactive.sh: test_expect_success 'warn about add.interactive.useB
>>       +	test_when_finished "git reset --hard; rm -f command" &&
>>       +	echo W >command &&
>>       +	git add -N command &&
>>      -+	cat >expect <<-EOF &&
>>      -+	diff --git a/command b/command
>>      -+	new file mode 100644
>>      -+	index 0000000..a42d8ff
>>      -+	--- /dev/null
>>      -+	+++ b/command
>>      -+	@@ -0,0 +1 @@
>>      -+	+W
>>      ++	git diff command >expect &&
>>      ++	cat >>expect <<-EOF &&
>>       +	(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help)
>>       +	(1/1) Stage addition [y,n,q,a,d,e,p,?]?$SP
>>       +	EOF
> 
> Interesting.
> 
> My first reaction was "how is this different from checking just the
> last line of the actual output?  The early part of expect and actual
> both come from an internal invocation of 'git diff', and they must
> match by definition".
> 
> But that may really be the point of this test.

Yes - we want to make sure that we are not printing the help and the 
only way to do that is to check the whole output

Best Wishes

Phillip

> That is, we may later decide to tweak the way "git diff" hunks are
> presented, and we expect that the way "git add -p" presents the
> hunks would change together with it automatically.

Re: [PATCH v4] add-patch: response to unknown command

[Junio C Hamano]

phillip.wood123@gmail.com writes:

> On 22/04/2024 16:50, Junio C Hamano wrote:
>> Rubén Justo <rjusto@gmail.com> writes:
>> 
>>> 1:  0317594bce ! 1:  b418b03f15 add-patch: response to unknown command
>>>      @@ t/t3701-add-interactive.sh: test_expect_success 'warn about add.interactive.useB
>>>       +	test_when_finished "git reset --hard; rm -f command" &&
>>>       +	echo W >command &&
>>>       +	git add -N command &&
>>>      -+	cat >expect <<-EOF &&
>>>      -+	diff --git a/command b/command
>>>      -+	new file mode 100644
>>>      -+	index 0000000..a42d8ff
>>>      -+	--- /dev/null
>>>      -+	+++ b/command
>>>      -+	@@ -0,0 +1 @@
>>>      -+	+W
>>>      ++	git diff command >expect &&
>>>      ++	cat >>expect <<-EOF &&
>>>       +	(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help)
>>>       +	(1/1) Stage addition [y,n,q,a,d,e,p,?]?$SP
>>>       +	EOF
>> Interesting.
>> My first reaction was "how is this different from checking just the
>> last line of the actual output?  The early part of expect and actual
>> both come from an internal invocation of 'git diff', and they must
>> match by definition".
>> But that may really be the point of this test.
>
> Yes - we want to make sure that we are not printing the help and the
> only way to do that is to check the whole output

I was not questioning that part of the patch.  "My first reaction"
was solely about use of "git diff" to replace the golden copy of
expected result in the test itself, only to allow for use of
different hash functions.  As you (or somebody else?) mentioned in
an earlier review, diff_cmp is there for exactly that purpose.

>> That is, we may later decide to tweak the way "git diff" hunks are
>> presented, and we expect that the way "git add -p" presents the
>> hunks would change together with it automatically.

This argument cuts both ways, by the way.

Insisting that the output match the explicit expectation (not what
"git diff" of the day produces) has a few advantages.  It makes the
test more explicit and easy to see what output we are expecting, and
more importantly, it forces us to update the test when we decide to
tweak the output from the command being tested (i.e. hunk selection
UI) and/or the output from "git diff" command.

Re: [PATCH v4] add-patch: response to unknown command

[Phillip Wood]

Hi Junio

On 24/04/2024 16:35, Junio C Hamano wrote:
> phillip.wood123@gmail.com writes:
> 
>> On 22/04/2024 16:50, Junio C Hamano wrote:
>>> Rubén Justo <rjusto@gmail.com> writes:
>>>
>>>> 1:  0317594bce ! 1:  b418b03f15 add-patch: response to unknown command
>>>>       @@ t/t3701-add-interactive.sh: test_expect_success 'warn about add.interactive.useB
>>>>        +	test_when_finished "git reset --hard; rm -f command" &&
>>>>        +	echo W >command &&
>>>>        +	git add -N command &&
>>>>       -+	cat >expect <<-EOF &&
>>>>       -+	diff --git a/command b/command
>>>>       -+	new file mode 100644
>>>>       -+	index 0000000..a42d8ff
>>>>       -+	--- /dev/null
>>>>       -+	+++ b/command
>>>>       -+	@@ -0,0 +1 @@
>>>>       -+	+W
>>>>       ++	git diff command >expect &&
>>>>       ++	cat >>expect <<-EOF &&
>>>>        +	(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help)
>>>>        +	(1/1) Stage addition [y,n,q,a,d,e,p,?]?$SP
>>>>        +	EOF
>>> Interesting.
>>> My first reaction was "how is this different from checking just the
>>> last line of the actual output?  The early part of expect and actual
>>> both come from an internal invocation of 'git diff', and they must
>>> match by definition".
>>> But that may really be the point of this test.
>>
>> Yes - we want to make sure that we are not printing the help and the
>> only way to do that is to check the whole output
> 
> I was not questioning that part of the patch.  "My first reaction"
> was solely about use of "git diff" to replace the golden copy of
> expected result in the test itself, only to allow for use of
> different hash functions.  As you (or somebody else?) mentioned in
> an earlier review, diff_cmp is there for exactly that purpose.

Oh sorry I'd misunderstood

>>> That is, we may later decide to tweak the way "git diff" hunks are
>>> presented, and we expect that the way "git add -p" presents the
>>> hunks would change together with it automatically.
> 
> This argument cuts both ways, by the way.
> 
> Insisting that the output match the explicit expectation (not what
> "git diff" of the day produces) has a few advantages.  It makes the
> test more explicit and easy to see what output we are expecting, and
> more importantly, it forces us to update the test when we decide to
> tweak the output from the command being tested (i.e. hunk selection
> UI) and/or the output from "git diff" command.

There is also a practical argument against using "git diff" to generate 
the expected output as it only works if the diff contains a single hunk. 
If the diff contains more than one hunk "add -p" displays them separately.

Best Wishes

Phillip

Re: [PATCH v4] add-patch: response to unknown command

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> There is also a practical argument against using "git diff" to
> generate the expected output as it only works if the diff contains a
> single hunk. If the diff contains more than one hunk "add -p" displays
> them separately.

True, but spliting the output from "git diff" at @@ hunk boundary
lines would allow you to hardcode the assumption that "add -p" shows
each hunk exactly as "git diff" would.  So if we really wanted to
have the convenience that the tests' expectation automatically
follow what "git diff" of the day happens to produce, it is still a
viable approach to use "git diff" output with some post-processing.

Thanks.

Re: [PATCH v4] add-patch: response to unknown command

[Jeff King]

On Sun, Apr 21, 2024 at 11:52:33PM +0200, Rubén Justo wrote:

> +test_expect_success 'unknown command' '
> +	test_when_finished "git reset --hard; rm -f command" &&
> +	echo W >command &&
> +	git add -N command &&
> +	git diff command >expect &&
> +	cat >>expect <<-EOF &&
> +	(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help)
> +	(1/1) Stage addition [y,n,q,a,d,e,p,?]?$SP
> +	EOF
> +	git add -p -- command <command >actual 2>&1 &&
> +	test_cmp expect actual
> +'

I got a test failure on Windows CI from this. The test_cmp output looks
like this:

  -(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command 'W' (use '?' for help)
  -(1/1) Stage addition [y,n,q,a,d,e,p,?]?
  +(1/1) Stage addition [y,n,q,a,d,e,p,?]? (1/1) Stage addition [y,n,q,a,d,e,p,?]?
  +Unknown command 'W' (use '?' for help)

which makes me suspect a race. Perhaps because the prompt is going to
stdout, but the "Unknown command" message goes to stderr? Maybe we
should keep stdout and stderr separate and check them independently.

-Peff

Re: [PATCH v4] add-patch: response to unknown command

[Eric Sunshine]

On Wed, Apr 24, 2024 at 9:44 PM Jeff King <peff@peff.net> wrote:
> On Sun, Apr 21, 2024 at 11:52:33PM +0200, Rubén Justo wrote:
> > +test_expect_success 'unknown command' '
> > +     test_when_finished "git reset --hard; rm -f command" &&
> > +     echo W >command &&
> > +     git add -N command &&
> > +     git diff command >expect &&
> > +     cat >>expect <<-EOF &&
> > +     (1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help)
> > +     (1/1) Stage addition [y,n,q,a,d,e,p,?]?$SP
> > +     EOF
> > +     git add -p -- command <command >actual 2>&1 &&
> > +     test_cmp expect actual
> > +'
>
> I got a test failure on Windows CI from this. The test_cmp output looks
> like this:
>
>   -(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command 'W' (use '?' for help)
>   -(1/1) Stage addition [y,n,q,a,d,e,p,?]?
>   +(1/1) Stage addition [y,n,q,a,d,e,p,?]? (1/1) Stage addition [y,n,q,a,d,e,p,?]?
>   +Unknown command 'W' (use '?' for help)
>
> which makes me suspect a race. Perhaps because the prompt is going to
> stdout, but the "Unknown command" message goes to stderr? Maybe we
> should keep stdout and stderr separate and check them independently.

That's very reminiscent of [1]. Although, unlike [1], the output
presented to the user in this case is (I suppose) less likely to be
messed up; only the combined captured output is probably affected. So,
capturing stdout and stderr separately would indeed be a good idea.

[1]: https://lore.kernel.org/git/CAPig+cTGq-10ZTBts2LXRVdPMf2vNMX8HTuhg_+ZHSiLX-brOQ@mail.gmail.com/

Re: [PATCH v4] add-patch: response to unknown command

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> That's very reminiscent of [1]. Although, unlike [1], the output
> presented to the user in this case is (I suppose) less likely to be
> messed up; only the combined captured output is probably affected. So,
> capturing stdout and stderr separately would indeed be a good idea.

Hmph, something along this line?

It loses to capture how the output should be intermixed, which is
essential to validate what the end-user should see.  As we can see
in the attached patch, we cannot express that "Unknown ..." should
come in between two "Stage addition?" questions, which is a downside.

Between adding fflush() before err() writes, and updating err() to
write to the standard output stream, I am in favor of the latter for
its simplicity (of the mental model of the resulting code, not of
the patch that is required to do so).

diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index ed7e414649..a8dfebd8d7 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -64,11 +64,12 @@ test_expect_success 'unknown command' '
 	git add -N command &&
 	git diff command >expect &&
 	cat >>expect <<-EOF &&
-	(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help)
-	(1/1) Stage addition [y,n,q,a,d,e,p,?]?$SP
+	(1/1) Stage addition [y,n,q,a,d,e,p,?]? (1/1) Stage addition [y,n,q,a,d,e,p,?]?$SP
 	EOF
-	git add -p -- command <command >actual 2>&1 &&
-	test_cmp expect actual
+	echo "Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help)" >expect.error &&
+	git add -p -- command <command >actual 2>actual.error &&
+	test_cmp expect actual &&
+	test_cmp expect.error actual.error
 '
 
 test_expect_success 'setup (initial)' '

Re: [PATCH v4] add-patch: response to unknown command

[Eric Sunshine]

On Thu, Apr 25, 2024 at 4:23 PM Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> > That's very reminiscent of [1]. Although, unlike [1], the output
> > presented to the user in this case is (I suppose) less likely to be
> > messed up; only the combined captured output is probably affected. So,
> > capturing stdout and stderr separately would indeed be a good idea.
>
> Between adding fflush() before err() writes, and updating err() to
> write to the standard output stream, I am in favor of the latter for
> its simplicity (of the mental model of the resulting code, not of
> the patch that is required to do so).

Writing to a common stream (stdout, in this case) for this sort of
interactive session is indeed probably the way to go, as Phillip
suggested.

That was also the adopted solution to the cited similar example[1];
git-worktree was changed to send all its chatty output to stderr[2],
which was appropriate for that (non-interactive) case.

[1]: https://lore.kernel.org/git/CAPig+cTGq-10ZTBts2LXRVdPMf2vNMX8HTuhg_+ZHSiLX-brOQ@mail.gmail.com/
[2]: https://lore.kernel.org/git/20211203034420.47447-2-sunshine@sunshineco.com/

Re: [PATCH v4] add-patch: response to unknown command

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> That was also the adopted solution to the cited similar example[1];
> git-worktree was changed to send all its chatty output to stderr[2],
> which was appropriate for that (non-interactive) case.
>
> [1]: https://lore.kernel.org/git/CAPig+cTGq-10ZTBts2LXRVdPMf2vNMX8HTuhg_+ZHSiLX-brOQ@mail.gmail.com/
> [2]: https://lore.kernel.org/git/20211203034420.47447-2-sunshine@sunshineco.com/

Hmph, what I just sent out tries to send everything to the standard
output stream.  For an interactive sesion, whether the interaction
is done over stdout or stderr does not really make much difference,
but I have a slight preference to use the stderr (because I view
prompts etc. in the same "chatty" class as the progress bar), but I
do not care enough to go back and redo the patch ;-)

Re: [PATCH v4] add-patch: response to unknown command

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> That's very reminiscent of [1]. Although, unlike [1], the output
>> presented to the user in this case is (I suppose) less likely to be
>> messed up; only the combined captured output is probably affected. So,
>> capturing stdout and stderr separately would indeed be a good idea.
>
> Hmph, something along this line?
>
> It loses to capture how the output should be intermixed, which is
> essential to validate what the end-user should see.  As we can see
> in the attached patch, we cannot express that "Unknown ..." should
> come in between two "Stage addition?" questions, which is a downside.
>
> Between adding fflush() before err() writes, and updating err() to
> write to the standard output stream, I am in favor of the latter for
> its simplicity (of the mental model of the resulting code, not of
> the patch that is required to do so).

The latter, which I claimed to prefer, would look like this.

The idea is to perform the interactive session over the standard
output (and the standard input).  For that, we teach err() to use
the standard output and have a few fprintf() to also call err().

A few tests expect certain messages to appear on the standard error
stream, which needed adjusting.

I know the previous one "fixes" the CI job at Windows, but I haven't
tried this yet.

diff --git c/add-patch.c w/add-patch.c
index 7be142d448..c28ad380ed 100644
--- c/add-patch.c
+++ w/add-patch.c
@@ -293,10 +293,10 @@ static void err(struct add_p_state *s, const char *fmt, ...)
 	va_list args;
 
 	va_start(args, fmt);
-	fputs(s->s.error_color, stderr);
-	vfprintf(stderr, fmt, args);
-	fputs(s->s.reset_color, stderr);
-	fputc('\n', stderr);
+	fputs(s->s.error_color, stdout);
+	vfprintf(stdout, fmt, args);
+	fputs(s->s.reset_color, stdout);
+	fputc('\n', stdout);
 	va_end(args);
 }
 
@@ -1326,7 +1326,7 @@ static int apply_for_checkout(struct add_p_state *s, struct strbuf *diff,
 		err(s, _("Nothing was applied.\n"));
 	} else
 		/* As a last resort, show the diff to the user */
-		fwrite(diff->buf, diff->len, 1, stderr);
+		fwrite(diff->buf, diff->len, 1, stdout);
 
 	return 0;
 }
@@ -1780,9 +1780,9 @@ int run_add_p(struct repository *r, enum add_p_mode mode,
 			break;
 
 	if (s.file_diff_nr == 0)
-		fprintf(stderr, _("No changes.\n"));
+		err(&s, _("No changes."));
 	else if (binary_count == s.file_diff_nr)
-		fprintf(stderr, _("Only binary files changed.\n"));
+		err(&s, _("Only binary files changed."));
 
 	add_p_state_clear(&s);
 	return 0;
diff --git c/t/t3701-add-interactive.sh w/t/t3701-add-interactive.sh
index 482d5c117e..a315ec99a3 100755
--- c/t/t3701-add-interactive.sh
+++ w/t/t3701-add-interactive.sh
@@ -43,17 +43,17 @@ force_color () {
 }
 
 test_expect_success 'warn about add.interactive.useBuiltin' '
-	cat >expect <<-\EOF &&
+	cat >expect.error <<-\EOF &&
 	warning: the add.interactive.useBuiltin setting has been removed!
 	See its entry in '\''git help config'\'' for details.
-	No changes.
 	EOF
 
 	for v in = =true =false
 	do
-		git -c "add.interactive.useBuiltin$v" add -p >out 2>actual &&
-		test_must_be_empty out &&
-		test_cmp expect actual || return 1
+		git -c "add.interactive.useBuiltin$v" add -p >actual 2>error &&
+		echo "No changes." >expect &&
+		test_cmp expect actual &&
+		test_cmp expect.error error || return 1
 	done
 '
 
@@ -348,13 +348,13 @@ test_expect_success 'different prompts for mode change/deleted' '
 
 test_expect_success 'correct message when there is nothing to do' '
 	git reset --hard &&
-	git add -p 2>err &&
-	test_grep "No changes" err &&
+	git add -p >out &&
+	test_grep "No changes" out &&
 	printf "\\0123" >binary &&
 	git add binary &&
 	printf "\\0abc" >binary &&
-	git add -p 2>err &&
-	test_grep "Only binary files changed" err
+	git add -p >out &&
+	test_grep "Only binary files changed" out
 '
 
 test_expect_success 'setup again' '

Re: [PATCH v4] add-patch: response to unknown command

[Rubén Justo]

On Thu, Apr 25, 2024 at 02:05:33PM -0700, Junio C Hamano wrote:

I am assuming that this change will precede my series.

I leave some nits below.

> diff --git c/add-patch.c w/add-patch.c
> index 7be142d448..c28ad380ed 100644
> --- c/add-patch.c
> +++ w/add-patch.c
> @@ -293,10 +293,10 @@ static void err(struct add_p_state *s, const char *fmt, ...)
>  	va_list args;
>  
>  	va_start(args, fmt);
> -	fputs(s->s.error_color, stderr);
> -	vfprintf(stderr, fmt, args);
> -	fputs(s->s.reset_color, stderr);
> -	fputc('\n', stderr);
> +	fputs(s->s.error_color, stdout);
> +	vfprintf(stdout, fmt, args);
> +	fputs(s->s.reset_color, stdout);
> +	fputc('\n', stdout);

If we're going to use stdout, perhaps we can be less explicit?

diff --git a/add-patch.c b/add-patch.c
index 447e8166d2..a204224dae 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -293,10 +293,10 @@ static void err(struct add_p_state *s, const char *fmt, ...)
        va_list args;

        va_start(args, fmt);
-       fputs(s->s.error_color, stderr);
-       vfprintf(stderr, fmt, args);
-       fputs(s->s.reset_color, stderr);
-       fputc('\n', stderr);
+       puts(s->s.error_color);
+       vprintf(fmt, args);
+       puts(s->s.reset_color);
+       putchar('\n');
        va_end(args);
 }

> @@ -1780,9 +1780,9 @@ int run_add_p(struct repository *r, enum add_p_mode mode,
>  			break;
>  
>  	if (s.file_diff_nr == 0)
> -		fprintf(stderr, _("No changes.\n"));
> +		err(&s, _("No changes."));

Nice.

>  	else if (binary_count == s.file_diff_nr)
> -		fprintf(stderr, _("Only binary files changed.\n"));
> +		err(&s, _("Only binary files changed."));

Nice.

> diff --git c/t/t3701-add-interactive.sh w/t/t3701-add-interactive.sh
> index 482d5c117e..a315ec99a3 100755
> --- c/t/t3701-add-interactive.sh
> +++ w/t/t3701-add-interactive.sh
> @@ -43,17 +43,17 @@ force_color () {
>  }
>  
>  test_expect_success 'warn about add.interactive.useBuiltin' '
> -	cat >expect <<-\EOF &&
> +	cat >expect.error <<-\EOF &&

This expect.error is what we are going to test with the output on
stderr ... 

>  	warning: the add.interactive.useBuiltin setting has been removed!
>  	See its entry in '\''git help config'\'' for details.
> -	No changes.

however, this line no longer goes to stderr.  OK.

>  	EOF
>  
>  	for v in = =true =false
>  	do
> -		git -c "add.interactive.useBuiltin$v" add -p >out 2>actual &&
> -		test_must_be_empty out &&
> -		test_cmp expect actual || return 1
> +		git -c "add.interactive.useBuiltin$v" add -p >actual 2>error &&
> +		echo "No changes." >expect &&

Why not prepare this file above, out of the loop?

> +		test_cmp expect actual &&
> +		test_cmp expect.error error || return 1
>  	done
>  '
>  
> @@ -348,13 +348,13 @@ test_expect_success 'different prompts for mode change/deleted' '
>  
>  test_expect_success 'correct message when there is nothing to do' '
>  	git reset --hard &&
> -	git add -p 2>err &&
> -	test_grep "No changes" err &&
> +	git add -p >out &&
> +	test_grep "No changes" out &&
>  	printf "\\0123" >binary &&
>  	git add binary &&
>  	printf "\\0abc" >binary &&
> -	git add -p 2>err &&
> -	test_grep "Only binary files changed" err
> +	git add -p >out &&
> +	test_grep "Only binary files changed" out
>  '
>  
>  test_expect_success 'setup again' '

Re: [PATCH v4] add-patch: response to unknown command

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

> On Thu, Apr 25, 2024 at 02:05:33PM -0700, Junio C Hamano wrote:
>
> I am assuming that this change will precede my series.

No, this was merely "if we were to update the series I queued to my
tree, the squashable fix may look like this", which you can use to
update _your_ series if you want to.

Re: [PATCH v4] add-patch: response to unknown command

[Rubén Justo]

> Rubén Justo <rjusto@gmail.com> writes:
> 
> > On Thu, Apr 25, 2024 at 02:05:33PM -0700, Junio C Hamano wrote:
> >
> > I am assuming that this change will precede my series.
> 
> No, this was merely "if we were to update the series I queued to my
> tree, the squashable fix may look like this", which you can use to
> update _your_ series if you want to.
> 

I was not sure what was the expectation.  In fact, I'm still not quite
sure.

I am not sure about the change either.

The current options are:

	a.- make the test check for stderr and stdout, separatedly

	b.- fflush(stdout) in err

	c.- make err print to stdout

I suspect that similar tests for other commands would produce similar
errors, so (a) seems like an easy fix but feels like kicking the can
forward.

I'm not sure of the implications of (c).  Perhaps moving current
messages to stdout breaks some workflow out there?  The other thread
about disabling all hints has made me think.

The (b) option seems to me the less disturbing change, but it has not
attracted attention.

There is even a (d) that is to go back to test just the new error
message, not the whole output.

I will give it some thought.

Re: [PATCH v4] add-patch: response to unknown command

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

> The current options are:
>
> 	a.- make the test check for stderr and stdout, separatedly

There is a downside that we do not check what the users would see,
as I mentioned in an earlier message.

> 	b.- fflush(stdout) in err
>
> 	c.- make err print to stdout

Yup.  The last one is what I showed in the thread.

> I suspect that similar tests for other commands would produce similar
> errors, so (a) seems like an easy fix but feels like kicking the can
> forward.
>
> I'm not sure of the implications of (c).  Perhaps moving current
> messages to stdout breaks some workflow out there?  The other thread
> about disabling all hints has made me think.

That depends on "other" commands.  As Phillip said, "git add -p" and
the like that interacts with the end user via terminal, using a
single stream, whether it is the standard error stream or the
standard output stream, should not affect any end-user experience.
It is very unlikely people capture only one stream and feed it to a
program, and with i18n, these messages are not even designed for
machine consumption in the first place.

> The (b) option seems to me the less disturbing change

Sticking close to the status quo would certainly _feel_ the safest
at least in the shorter term.

But given that there is not really a reasonable justification why
some output goes to the standard output stream while others go to
the standard error stream in this program (note that I am
specifically talking about the terminal interactive session with
"add -p"), the approach will force us to worry about similar gotchas
and we need to decide which stream the message needs to go every
time we add a new one.

> but it has not attracted attention.

I think the same reasoning from the old thread that made us avoid
the "flush() and keep writing to two streams" in the worktree code
would apply here, too.

Re: [PATCH v4] add-patch: response to unknown command

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

> I will give it some thought.

In the meantime, I'll revert the merge of the topic into 'next',
squash in the "send everything to the standard output stream" fix,
and queue the "tentative" result to 'seen', to make the CI happy.

I am OK enough with the "tentative" one, but if you and others feel
differently, I am perfectly happy to see it replaced with [PATCH v5].

Thanks.

Re: [PATCH v4] add-patch: response to unknown command

[Jeff King]

On Wed, Apr 24, 2024 at 10:15:25PM -0400, Eric Sunshine wrote:

> > I got a test failure on Windows CI from this. The test_cmp output looks
> > like this:
> >
> >   -(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command 'W' (use '?' for help)
> >   -(1/1) Stage addition [y,n,q,a,d,e,p,?]?
> >   +(1/1) Stage addition [y,n,q,a,d,e,p,?]? (1/1) Stage addition [y,n,q,a,d,e,p,?]?
> >   +Unknown command 'W' (use '?' for help)
> >
> > which makes me suspect a race. Perhaps because the prompt is going to
> > stdout, but the "Unknown command" message goes to stderr? Maybe we
> > should keep stdout and stderr separate and check them independently.
> 
> That's very reminiscent of [1]. Although, unlike [1], the output
> presented to the user in this case is (I suppose) less likely to be
> messed up; only the combined captured output is probably affected. So,
> capturing stdout and stderr separately would indeed be a good idea.

Ah, yeah, that is almost certainly the same issue. I could not reproduce
it on my local system with --stress, so I think it may be unique to
Windows. What it looks like is that stderr is buffered when output to a
file (whereas on most systems it would still be line buffered at most).
But I didn't actually run it through a debugger to see.

So I _suspect_ that yet another possible solution here is to do an
explicit:

  setvbuf(stderr, NULL, _IOLBF, BUFSIZ);

in common-main. Though grepping for setvbuf() in our code base, it looks
like compat/winansi.c already does:

          if (fd == 2)
                setvbuf(stderr, NULL, _IONBF, BUFSIZ);

which would imply that output should happen even more immediately than
line-buffering. So maybe there is some other layer at work. There are
probably dragons, and definitely pursuing that line would involve some
experimentation.

The "if you want them in order, make sure they are on the same stream"
approach suggested elsewhere is by comparison much simpler. :)

-Peff

Re: [PATCH v4] add-patch: response to unknown command

[Rubén Justo]

On Wed, Apr 24, 2024 at 09:44:32PM -0400, Jeff King wrote:
> On Sun, Apr 21, 2024 at 11:52:33PM +0200, Rubén Justo wrote:
> 
> > +test_expect_success 'unknown command' '
> > +	test_when_finished "git reset --hard; rm -f command" &&
> > +	echo W >command &&
> > +	git add -N command &&
> > +	git diff command >expect &&
> > +	cat >>expect <<-EOF &&
> > +	(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help)
> > +	(1/1) Stage addition [y,n,q,a,d,e,p,?]?$SP
> > +	EOF
> > +	git add -p -- command <command >actual 2>&1 &&
> > +	test_cmp expect actual
> > +'
> 
> I got a test failure on Windows CI from this.

Thank you for testing thoroughly.

> The test_cmp output looks
> like this:
> 
>   -(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command 'W' (use '?' for help)
>   -(1/1) Stage addition [y,n,q,a,d,e,p,?]?
>   +(1/1) Stage addition [y,n,q,a,d,e,p,?]? (1/1) Stage addition [y,n,q,a,d,e,p,?]?
>   +Unknown command 'W' (use '?' for help)
> 
> which makes me suspect a race. Perhaps because the prompt is going to
> stdout, but the "Unknown command" message goes to stderr?

I have to read the thread pointed by Eric, but my knee-jerk reaction has
been to think in something like:

diff --git a/add-patch.c b/add-patch.c
index 447e8166d2..0090543f89 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -292,6 +292,7 @@ static void err(struct add_p_state *s, const char *fmt, ...)
 {
        va_list args;

        va_start(args, fmt);
+       fflush(stdout);
        fputs(s->s.error_color, stderr);
        vfprintf(stderr, fmt, args);

Re: [PATCH v4] add-patch: response to unknown command

[Jeff King]

On Thu, Apr 25, 2024 at 05:04:45AM +0200, Rubén Justo wrote:

> > The test_cmp output looks
> > like this:
> > 
> >   -(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command 'W' (use '?' for help)
> >   -(1/1) Stage addition [y,n,q,a,d,e,p,?]?
> >   +(1/1) Stage addition [y,n,q,a,d,e,p,?]? (1/1) Stage addition [y,n,q,a,d,e,p,?]?
> >   +Unknown command 'W' (use '?' for help)
> > 
> > which makes me suspect a race. Perhaps because the prompt is going to
> > stdout, but the "Unknown command" message goes to stderr?
> 
> I have to read the thread pointed by Eric, but my knee-jerk reaction has
> been to think in something like:
> 
> diff --git a/add-patch.c b/add-patch.c
> index 447e8166d2..0090543f89 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -292,6 +292,7 @@ static void err(struct add_p_state *s, const char *fmt, ...)
>  {
>         va_list args;
> 
>         va_start(args, fmt);
> +       fflush(stdout);
>         fputs(s->s.error_color, stderr);
>         vfprintf(stderr, fmt, args);

I think the "just send it all to stdout" approach makes the most sense
here, but in case we don't do that: I don't think this will fix it. In
the output above it is the "Unknown command" output which is delayed,
which is sent to stderr via err(). So flushing stdout again won't help.
Flushing stderr after the vfprintf _might_ help (though I'm confused why
stderr would be fully buffered in the first place).

-Peff

Re: [PATCH v4] add-patch: response to unknown command

[Rubén Justo]

On Fri, Apr 26, 2024 at 04:23:33PM -0400, Jeff King wrote:
> On Thu, Apr 25, 2024 at 05:04:45AM +0200, Rubén Justo wrote:
> 
> > > The test_cmp output looks
> > > like this:
> > > 
> > >   -(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command 'W' (use '?' for help)
> > >   -(1/1) Stage addition [y,n,q,a,d,e,p,?]?
> > >   +(1/1) Stage addition [y,n,q,a,d,e,p,?]? (1/1) Stage addition [y,n,q,a,d,e,p,?]?
> > >   +Unknown command 'W' (use '?' for help)
> > > 
> > > which makes me suspect a race. Perhaps because the prompt is going to
> > > stdout, but the "Unknown command" message goes to stderr?
> > 
> > I have to read the thread pointed by Eric, but my knee-jerk reaction has
> > been to think in something like:
> > 
> > diff --git a/add-patch.c b/add-patch.c
> > index 447e8166d2..0090543f89 100644
> > --- a/add-patch.c
> > +++ b/add-patch.c
> > @@ -292,6 +292,7 @@ static void err(struct add_p_state *s, const char *fmt, ...)
> >  {
> >         va_list args;
> > 
> >         va_start(args, fmt);
> > +       fflush(stdout);
> >         fputs(s->s.error_color, stderr);
> >         vfprintf(stderr, fmt, args);
> 
> I think the "just send it all to stdout" approach makes the most sense
> here, but in case we don't do that: I don't think this will fix it. In
> the output above it is the "Unknown command" output which is delayed,
> which is sent to stderr via err().

Very true.

I'm happy with what Junio has just queued.  I do not plan to send a new
iteration, unless a new test break appears :-)

Thanks, all.

Re: [PATCH v4] add-patch: response to unknown command

[phillip.wood123]

Hi Peff

On 25/04/2024 02:44, Jeff King wrote:
> On Sun, Apr 21, 2024 at 11:52:33PM +0200, Rubén Justo wrote:
> 
>> +test_expect_success 'unknown command' '
>> +	test_when_finished "git reset --hard; rm -f command" &&
>> +	echo W >command &&
>> +	git add -N command &&
>> +	git diff command >expect &&
>> +	cat >>expect <<-EOF &&
>> +	(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help)
>> +	(1/1) Stage addition [y,n,q,a,d,e,p,?]?$SP
>> +	EOF
>> +	git add -p -- command <command >actual 2>&1 &&
>> +	test_cmp expect actual
>> +'
> 
> I got a test failure on Windows CI from this. The test_cmp output looks
> like this:
> 
>    -(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command 'W' (use '?' for help)
>    -(1/1) Stage addition [y,n,q,a,d,e,p,?]?
>    +(1/1) Stage addition [y,n,q,a,d,e,p,?]? (1/1) Stage addition [y,n,q,a,d,e,p,?]?
>    +Unknown command 'W' (use '?' for help)
> 
> which makes me suspect a race. Perhaps because the prompt is going to
> stdout, but the "Unknown command" message goes to stderr? Maybe we
> should keep stdout and stderr separate and check them independently.

Can we change err() to print to stdout? I can't really see the advantage 
of separating "normal output" and "error messages" for an interactive 
program like this.

Best Wishes

Phillip

[PATCH v5 0/2] add-patch: response to unknown command

[Rubén Justo]

I'm happy with the currently queued version v4+fix, and my understanding
is that it is OK enough.  However in the recent 'What's cooking' it is
marked with 'Expecting a reroll'.  Here it is, in case it is expected. 

Rubén Justo (2):
  add-patch: do not show UI messages on stderr
  add-patch: response to unknown command

 add-patch.c                | 18 ++++++++++--------
 t/t3701-add-interactive.sh | 28 +++++++++++++++++++++-------
 2 files changed, 31 insertions(+), 15 deletions(-)

-- 
2.45.0.2.g84ce137e4a

[PATCH v5 1/2] add-patch: do not show UI messages on stderr

[Rubén Justo]

There is no need to show some UI messages on stderr, and yet doing so
may produce some undesirable results, such as messages appearing in an
unexpected order.

Let's use stdout for all UI messages, and adjusts the tests accordingly.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 add-patch.c                | 13 ++++++-------
 t/t3701-add-interactive.sh | 12 ++++++------
 2 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index 0997d4af73..fc0eed4fd4 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -293,10 +293,9 @@ static void err(struct add_p_state *s, const char *fmt, ...)
 	va_list args;
 
 	va_start(args, fmt);
-	fputs(s->s.error_color, stderr);
-	vfprintf(stderr, fmt, args);
-	fputs(s->s.reset_color, stderr);
-	fputc('\n', stderr);
+	fputs(s->s.error_color, stdout);
+	vprintf(fmt, args);
+	puts(s->s.reset_color);
 	va_end(args);
 }
 
@@ -1326,7 +1325,7 @@ static int apply_for_checkout(struct add_p_state *s, struct strbuf *diff,
 		err(s, _("Nothing was applied.\n"));
 	} else
 		/* As a last resort, show the diff to the user */
-		fwrite(diff->buf, diff->len, 1, stderr);
+		fwrite(diff->buf, diff->len, 1, stdout);
 
 	return 0;
 }
@@ -1778,9 +1777,9 @@ int run_add_p(struct repository *r, enum add_p_mode mode,
 			break;
 
 	if (s.file_diff_nr == 0)
-		fprintf(stderr, _("No changes.\n"));
+		err(&s, _("No changes."));
 	else if (binary_count == s.file_diff_nr)
-		fprintf(stderr, _("Only binary files changed.\n"));
+		err(&s, _("Only binary files changed."));
 
 	add_p_state_clear(&s);
 	return 0;
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 04d8333373..c5531520cb 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -45,13 +45,13 @@ test_expect_success 'warn about add.interactive.useBuiltin' '
 	cat >expect <<-\EOF &&
 	warning: the add.interactive.useBuiltin setting has been removed!
 	See its entry in '\''git help config'\'' for details.
-	No changes.
 	EOF
+	echo "No changes." >expect.out &&
 
 	for v in = =true =false
 	do
 		git -c "add.interactive.useBuiltin$v" add -p >out 2>actual &&
-		test_must_be_empty out &&
+		test_cmp expect.out out &&
 		test_cmp expect actual || return 1
 	done
 '
@@ -335,13 +335,13 @@ test_expect_success 'different prompts for mode change/deleted' '
 
 test_expect_success 'correct message when there is nothing to do' '
 	git reset --hard &&
-	git add -p 2>err &&
-	test_grep "No changes" err &&
+	git add -p >out &&
+	test_grep "No changes" out &&
 	printf "\\0123" >binary &&
 	git add binary &&
 	printf "\\0abc" >binary &&
-	git add -p 2>err &&
-	test_grep "Only binary files changed" err
+	git add -p >out &&
+	test_grep "Only binary files changed" out
 '
 
 test_expect_success 'setup again' '
-- 
2.45.0.2.g84ce137e4a

Re: [PATCH v5 1/2] add-patch: do not show UI messages on stderr

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

> diff --git a/add-patch.c b/add-patch.c
> index 0997d4af73..fc0eed4fd4 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -293,10 +293,9 @@ static void err(struct add_p_state *s, const char *fmt, ...)
>  	va_list args;
>  
>  	va_start(args, fmt);
> -	fputs(s->s.error_color, stderr);
> -	vfprintf(stderr, fmt, args);
> -	fputs(s->s.reset_color, stderr);
> -	fputc('\n', stderr);
> +	fputs(s->s.error_color, stdout);
> +	vprintf(fmt, args);
> +	puts(s->s.reset_color);

I like the attention of the detail here ;-).

Re: [PATCH v5 1/2] add-patch: do not show UI messages on stderr

[Jeff King]

On Mon, Apr 29, 2024 at 12:24:46PM -0700, Junio C Hamano wrote:

> Rubén Justo <rjusto@gmail.com> writes:
> 
> > diff --git a/add-patch.c b/add-patch.c
> > index 0997d4af73..fc0eed4fd4 100644
> > --- a/add-patch.c
> > +++ b/add-patch.c
> > @@ -293,10 +293,9 @@ static void err(struct add_p_state *s, const char *fmt, ...)
> >  	va_list args;
> >  
> >  	va_start(args, fmt);
> > -	fputs(s->s.error_color, stderr);
> > -	vfprintf(stderr, fmt, args);
> > -	fputs(s->s.reset_color, stderr);
> > -	fputc('\n', stderr);
> > +	fputs(s->s.error_color, stdout);
> > +	vprintf(fmt, args);
> > +	puts(s->s.reset_color);
> 
> I like the attention of the detail here ;-).

Indeed. I had to read this several times to wonder why it was not a
mistake to leave the first fputs() but use vprintf() and puts() for the
other two (for those just reading, the answer is that puts() prints an
extra newline, so we can only use it at the end). Which IMHO really just
points to how inconsistent the stdio interfaces are, but there is
nothing we can do about that. ;)

I am tempted to suggest that it borders on too-clever, and writing out
"stdout" everywhere with vfprintf() and fputs() would be more obvious.
But in a little self-contained function like this I don't know that it
matters much.

-Peff

Re: [PATCH v5 1/2] add-patch: do not show UI messages on stderr

[Rubén Justo]

On Tue, Apr 30, 2024 at 06:52:56AM -0400, Jeff King wrote:
> On Mon, Apr 29, 2024 at 12:24:46PM -0700, Junio C Hamano wrote:
> 
> > Rubén Justo <rjusto@gmail.com> writes:
> > 
> > > diff --git a/add-patch.c b/add-patch.c
> > > index 0997d4af73..fc0eed4fd4 100644
> > > --- a/add-patch.c
> > > +++ b/add-patch.c
> > > @@ -293,10 +293,9 @@ static void err(struct add_p_state *s, const char *fmt, ...)
> > >  	va_list args;
> > >  
> > >  	va_start(args, fmt);
> > > -	fputs(s->s.error_color, stderr);
> > > -	vfprintf(stderr, fmt, args);
> > > -	fputs(s->s.reset_color, stderr);
> > > -	fputc('\n', stderr);
> > > +	fputs(s->s.error_color, stdout);
> > > +	vprintf(fmt, args);
> > > +	puts(s->s.reset_color);
> > 
> > I like the attention of the detail here ;-).
> 
> Indeed. I had to read this several times to wonder why it was not a
> mistake to leave the first fputs() but use vprintf() and puts() for the
> other two (for those just reading, the answer is that puts() prints an
> extra newline, so we can only use it at the end).

I did not know the details (or had happily forgotten them) but Junio
ignoring my comments in [1] intrigued me :-).  A simple test would have
been quick, but "man puts" was quicker;  my comments were not correct.

Just to be clear, and to extend your comment, in case any reader is
interested, this:

	vfprintf(stdout, fmt, args);

can be written as follows:

	vprintf(fmt, args);

And this:

	fputs(s->s.reset_color, stdout);
	fputc('\n', stdout);

can be shortened to:

	puts(s->s.reset_color);

The difference in vfprintf and vprintf is quite obvious IMHO, but not so
obvious with puts.  The documentation says:

  SYNOPSIS

       int puts(const char *s);

  DESCRIPTION

       puts() writes the string s and a trailing newline to stdout.



  [1] - https://lore.kernel.org/git/4e2bc660-ee33-4641-aca5-783d0cefcd23@gmail.com/T/#m644a682364212729a2b21c052ef744039c26f972

Re: [PATCH v5 1/2] add-patch: do not show UI messages on stderr

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

>> Indeed. I had to read this several times to wonder why it was not a
>> mistake to leave the first fputs() but use vprintf() and puts() for the
>> other two (for those just reading, the answer is that puts() prints an
>> extra newline, so we can only use it at the end).
>
> I did not know the details (or had happily forgotten them) but Junio
> ignoring my comments in [1] intrigued me :-).  A simple test would have
> been quick, but "man puts" was quicker;  my comments were not correct.

I thought you were suggesting to fold fputs()+putchar('\n') into
puts(), which is a change that does not break.  I didn't even know
that you were suggesting to make more changes that would break the
output.  After all, you said only "can be less explicit" without
saying which use of "stdout" you meant to make "less explicit" ;-).

Re: [PATCH v5 1/2] add-patch: do not show UI messages on stderr

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I am tempted to suggest that it borders on too-clever, and writing out
> "stdout" everywhere with vfprintf() and fputs() would be more obvious.
> But in a little self-contained function like this I don't know that it
> matters much.

I share your preference (and that was the way I wrote "it would look
like this" version earlier) but I agree that this is too small to
matter.

Thanks.

Re: [PATCH v5 1/2] add-patch: do not show UI messages on stderr

[phillip.wood123]

Hi Rubén

On 29/04/2024 19:37, Rubén Justo wrote:
> There is no need to show some UI messages on stderr, and yet doing so
> may produce some undesirable results, such as messages appearing in an
> unexpected order.
> 
> Let's use stdout for all UI messages, and adjusts the tests accordingly.

The test changes in "warn add.interactive.useBultin" show that we still 
print a warning to stderr, I don't think that particular case is worth 
worrying about though.

> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
>   add-patch.c                | 13 ++++++-------
>   t/t3701-add-interactive.sh | 12 ++++++------
>   2 files changed, 12 insertions(+), 13 deletions(-)
> 
> diff --git a/add-patch.c b/add-patch.c
> index 0997d4af73..fc0eed4fd4 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -293,10 +293,9 @@ static void err(struct add_p_state *s, const char *fmt, ...)
>   	va_list args;
>   
>   	va_start(args, fmt);
> -	fputs(s->s.error_color, stderr);
> -	vfprintf(stderr, fmt, args);
> -	fputs(s->s.reset_color, stderr);
> -	fputc('\n', stderr);
> +	fputs(s->s.error_color, stdout);
> +	vprintf(fmt, args);
> +	puts(s->s.reset_color);
>   	va_end(args);
>   }

This looks like a good change

> @@ -1326,7 +1325,7 @@ static int apply_for_checkout(struct add_p_state *s, struct strbuf *diff,
>   		err(s, _("Nothing was applied.\n"));
>   	} else
>   		/* As a last resort, show the diff to the user */
> -		fwrite(diff->buf, diff->len, 1, stderr);
> +		fwrite(diff->buf, diff->len, 1, stdout);

This seems reasonable as we'd print the "Nothing was applied" error 
message above to stdout now. Anything "git apply" writes to stderr 
should be flushed when it exits before we print these messages.

>   
>   	return 0;
>   }
> @@ -1778,9 +1777,9 @@ int run_add_p(struct repository *r, enum add_p_mode mode,
>   			break;
>   
>   	if (s.file_diff_nr == 0)
> -		fprintf(stderr, _("No changes.\n"));
> +		err(&s, _("No changes."));
>   	else if (binary_count == s.file_diff_nr)
> -		fprintf(stderr, _("Only binary files changed.\n"));
> +		err(&s, _("Only binary files changed."));

These two mean we'll now color these messages which we didn't do before. 
I think if we hit this code we don't print anything else (apart from the 
warning about add.interactive.useBuiltin being removed) so it probably 
does not matter whether we use stdout or stderr here and I don't have a 
strong opinion either way.

Best Wishes

Phillip

>   	add_p_state_clear(&s);
>   	return 0;
> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index 04d8333373..c5531520cb 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -45,13 +45,13 @@ test_expect_success 'warn about add.interactive.useBuiltin' '
>   	cat >expect <<-\EOF &&
>   	warning: the add.interactive.useBuiltin setting has been removed!
>   	See its entry in '\''git help config'\'' for details.
> -	No changes.
>   	EOF
> +	echo "No changes." >expect.out &&
>   
>   	for v in = =true =false
>   	do
>   		git -c "add.interactive.useBuiltin$v" add -p >out 2>actual &&
> -		test_must_be_empty out &&
> +		test_cmp expect.out out &&
>   		test_cmp expect actual || return 1
>   	done
>   '
> @@ -335,13 +335,13 @@ test_expect_success 'different prompts for mode change/deleted' '
>   
>   test_expect_success 'correct message when there is nothing to do' '
>   	git reset --hard &&
> -	git add -p 2>err &&
> -	test_grep "No changes" err &&
> +	git add -p >out &&
> +	test_grep "No changes" out &&
>   	printf "\\0123" >binary &&
>   	git add binary &&
>   	printf "\\0abc" >binary &&
> -	git add -p 2>err &&
> -	test_grep "Only binary files changed" err
> +	git add -p >out &&
> +	test_grep "Only binary files changed" out
>   '
>   
>   test_expect_success 'setup again' '

Re: [PATCH v5 1/2] add-patch: do not show UI messages on stderr

[Rubén Justo]

On Tue, Apr 30, 2024 at 03:47:04PM +0100, phillip.wood123@gmail.com wrote:

> > @@ -1778,9 +1777,9 @@ int run_add_p(struct repository *r, enum add_p_mode mode,
> >   			break;
> >   	if (s.file_diff_nr == 0)
> > -		fprintf(stderr, _("No changes.\n"));
> > +		err(&s, _("No changes."));
> >   	else if (binary_count == s.file_diff_nr)
> > -		fprintf(stderr, _("Only binary files changed.\n"));
> > +		err(&s, _("Only binary files changed."));
> 
> These two mean we'll now color these messages which we didn't do before. I
> think if we hit this code we don't print anything else (apart from the
> warning about add.interactive.useBuiltin being removed) so it probably does
> not matter whether we use stdout or stderr here and I don't have a strong
> opinion either way.

Can we consider those messages not part of the UI?  IIUC, if we hit that
code we haven't entered in the interactive UI.  Maybe we should:

diff --git a/add-patch.c b/add-patch.c
index c28ad380ed..b11a435738 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1780,9 +1780,9 @@ int run_add_p(struct repository *r, enum add_p_mode mode,
 			break;
 
 	if (s.file_diff_nr == 0)
-		err(&s, _("No changes."));
+		error(_("no changes"));
 	else if (binary_count == s.file_diff_nr)
-		err(&s, _("Only binary files changed."));
+		error(_("only binary files changed"));
 
 	add_p_state_clear(&s);
 	return 0;

Or, simply leave them untouched in this series.

> 
> Best Wishes
> 
> Phillip

Thanks.

Re: [PATCH v5 1/2] add-patch: do not show UI messages on stderr

[phillip.wood123]

Hi Rubén

On 30/04/2024 17:38, Rubén Justo wrote:
> On Tue, Apr 30, 2024 at 03:47:04PM +0100, phillip.wood123@gmail.com wrote:
> 
>>> @@ -1778,9 +1777,9 @@ int run_add_p(struct repository *r, enum add_p_mode mode,
>>>    			break;
>>>    	if (s.file_diff_nr == 0)
>>> -		fprintf(stderr, _("No changes.\n"));
>>> +		err(&s, _("No changes."));
>>>    	else if (binary_count == s.file_diff_nr)
>>> -		fprintf(stderr, _("Only binary files changed.\n"));
>>> +		err(&s, _("Only binary files changed."));
>>
>> These two mean we'll now color these messages which we didn't do before. I
>> think if we hit this code we don't print anything else (apart from the
>> warning about add.interactive.useBuiltin being removed) so it probably does
>> not matter whether we use stdout or stderr here and I don't have a strong
>> opinion either way.
> 
> Can we consider those messages not part of the UI?  IIUC, if we hit that
> code we haven't entered in the interactive UI. 

I thikn so

> Maybe we should:
> 
> diff --git a/add-patch.c b/add-patch.c
> index c28ad380ed..b11a435738 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -1780,9 +1780,9 @@ int run_add_p(struct repository *r, enum add_p_mode mode,
>   			break;
>   
>   	if (s.file_diff_nr == 0)
> -		err(&s, _("No changes."));
> +		error(_("no changes"));
>   	else if (binary_count == s.file_diff_nr)
> -		err(&s, _("Only binary files changed."));
> +		error(_("only binary files changed"));
>   
>   	add_p_state_clear(&s);
>   	return 0;
> 
> Or, simply leave them untouched in this series.

Either option sounds good to me

Best Wishes

Phillip

Re: [PATCH v5 1/2] add-patch: do not show UI messages on stderr

[Junio C Hamano]

phillip.wood123@gmail.com writes:

>>>> -		fprintf(stderr, _("No changes.\n"));
>>>> +		err(&s, _("No changes."));
>>>>    	else if (binary_count == s.file_diff_nr)
>>>> -		fprintf(stderr, _("Only binary files changed.\n"));
>>>> +		err(&s, _("Only binary files changed."));
>>>
>>> These two mean we'll now color these messages which we didn't do before. I
> ...
> I think so
> ...
>> -		err(&s, _("Only binary files changed."));
>> +		error(_("only binary files changed"));
>>     	add_p_state_clear(&s);
>>   	return 0;
>> Or, simply leave them untouched in this series.
>
> Either option sounds good to me

We are returning "success" to the caller, so using error() here is a
bit strong.  Judging from how other messages emitted with err() in
this program is meant to help the end user, they are all to tell the
user why their input caused no actual change, and showing these two
messages the same way as these other messages would be the best for
consistency.

So I'm inclined to say that what was posted is good.  If it paints
these two messages in the same color as others, that is even getter.

Re: [PATCH v5 1/2] add-patch: do not show UI messages on stderr

[Rubén Justo]

On Wed, May 01, 2024 at 09:14:37AM -0700, Junio C Hamano wrote:
> phillip.wood123@gmail.com writes:
> 
> >>>> -		fprintf(stderr, _("No changes.\n"));
> >>>> +		err(&s, _("No changes."));
> >>>>    	else if (binary_count == s.file_diff_nr)
> >>>> -		fprintf(stderr, _("Only binary files changed.\n"));
> >>>> +		err(&s, _("Only binary files changed."));
> >>>
> >>> These two mean we'll now color these messages which we didn't do before. I
> > ...
> > I think so
> > ...
> >> -		err(&s, _("Only binary files changed."));
> >> +		error(_("only binary files changed"));
> >>     	add_p_state_clear(&s);
> >>   	return 0;
> >> Or, simply leave them untouched in this series.
> >
> > Either option sounds good to me
> 
> We are returning "success" to the caller, so using error() here is a
> bit strong.  Judging from how other messages emitted with err() in
> this program is meant to help the end user, they are all to tell the
> user why their input caused no actual change, and showing these two
> messages the same way as these other messages would be the best for
> consistency.
> 
> So I'm inclined to say that what was posted is good.  If it paints
> these two messages in the same color as others, that is even getter.

Having:

    $ printf "\0" >binary
    $ git add -N binary

Displaying the message in RED does not seem as a bad change:

    $ git add -i
               staged     unstaged path
      1:        +0/-0       binary binary
    
    *** Commands ***
      1: status       2: update       3: revert       4: add untracked
      5: patch        6: diff         7: quit         8: help
    What now> p
    <RED>Only binary files changed.<RED>

However, here could be disturbing:

    $ git add -p
    <RED>Only binary files changed.<RED>

And the "No changes." message is going to have even more audience, I
think.

Perhaps this seems sensible:

Range-diff against v5:
2:  b0f042f4ff ! 1:  94c3f80041 add-patch: do not show UI messages on stderr
    @@ add-patch.c: int run_add_p(struct repository *r, enum add_p_mode mode,

        if (s.file_diff_nr == 0)
     -          fprintf(stderr, _("No changes.\n"));
    -+          err(&s, _("No changes."));
    ++          fprintf(stdout, _("No changes.\n"));
        else if (binary_count == s.file_diff_nr)
     -          fprintf(stderr, _("Only binary files changed.\n"));
    -+          err(&s, _("Only binary files changed."));
    ++          fprintf(stdout, _("Only binary files changed.\n"));

        add_p_state_clear(&s);
        return 0;
1:  4a0eb1337f = 2:  abaf904e8c add-patch: response to unknown command

But I'm not sure and I do not want to send a new iteration unless it is
necessary.  I was already happy with previous versions ;-).

Re: [PATCH v5 1/2] add-patch: do not show UI messages on stderr

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

> But I'm not sure and I do not want to send a new iteration unless it is
> necessary.  I was already happy with previous versions ;-).

Let's call it done-well-enough and merge it down.

Thanks.

[PATCH v5 2/2] add-patch: response to unknown command

[Rubén Justo]

When the user gives an unknown command to the "add -p" prompt, the list
of accepted commands with their explanation is given.  This is the same
output they get when they say '?'.

However, the unknown command may be due to a user input error rather
than the user not knowing the valid command.

To reduce the likelihood of user confusion and error repetition, instead
of displaying the list of accepted commands, display a short error
message with the unknown command received, as feedback to the user.

Include a reminder about the current command '?' in the new message, to
guide the user if they want help.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 add-patch.c                |  5 ++++-
 t/t3701-add-interactive.sh | 16 +++++++++++++++-
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index fc0eed4fd4..2252895c28 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1667,7 +1667,7 @@ static int patch_update_file(struct add_p_state *s,
 			}
 		} else if (s->answer.buf[0] == 'p') {
 			rendered_hunk_index = -1;
-		} else {
+		} else if (s->answer.buf[0] == '?') {
 			const char *p = _(help_patch_remainder), *eol = p;
 
 			color_fprintf(stdout, s->s.help_color, "%s",
@@ -1691,6 +1691,9 @@ static int patch_update_file(struct add_p_state *s,
 				color_fprintf_ln(stdout, s->s.help_color,
 						 "%.*s", (int)(eol - p), p);
 			}
+		} else {
+			err(s, _("Unknown command '%s' (use '?' for help)"),
+			    s->answer.buf);
 		}
 	}
 
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index c5531520cb..28a95a775d 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -8,6 +8,8 @@ TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-terminal.sh
 
+SP=" "
+
 diff_cmp () {
 	for x
 	do
@@ -56,6 +58,19 @@ test_expect_success 'warn about add.interactive.useBuiltin' '
 	done
 '
 
+test_expect_success 'unknown command' '
+	test_when_finished "git reset --hard; rm -f command" &&
+	echo W >command &&
+	git add -N command &&
+	git diff command >expect &&
+	cat >>expect <<-EOF &&
+	(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help)
+	(1/1) Stage addition [y,n,q,a,d,e,p,?]?$SP
+	EOF
+	git add -p -- command <command >actual 2>&1 &&
+	test_cmp expect actual
+'
+
 test_expect_success 'setup (initial)' '
 	echo content >file &&
 	git add file &&
@@ -232,7 +247,6 @@ test_expect_success 'setup file' '
 '
 
 test_expect_success 'setup patch' '
-	SP=" " &&
 	NULL="" &&
 	cat >patch <<-EOF
 	@@ -1,4 +1,4 @@
-- 
2.45.0.2.g84ce137e4a