{"thread":{"id":"61323","subject":"[PATCH] add-patch: response to invalid option","startedAt":"2024-04-15T19:00:31Z","lastAt":"2024-05-02T16:38:37Z","messageCount":55,"participants":["Rubén Justo","Patrick Steinhardt","phillip.wood123@gmail.com","Junio C Hamano","Phillip Wood","Jeff King","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"492962","messageId":"4e2bc660-ee33-4641-aca5-783d0cefcd23@gmail.com","threadId":"61323","inReplyTo":null,"subject":"[PATCH] add-patch: response to invalid option","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-04-15T19:00:28Z","receivedAt":"2024-04-15T19:00:31Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"When the user introduces an invalid option, we respond with the whole\nhelp text.\n\nInstead of displaying the long help description, display a short error\nmessage indicating the incorrectly introduced option with a note on how\nto get the help text.\n\nSigned-off-by: Rubén Justo \n---\n add-patch.c | 5 ++++-\n t/t3701-add-interactive.sh | 10 ++++++++++\n 2 files changed, 14 insertions(+), 1 deletion(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex a06dd18985..c77902fec5 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -1667,7 +1667,7 @@ static int patch_update_file(struct add_p_state *s,\n \t\t\t}\n \t\t} else if (s->answer.buf[0] == 'p') {\n \t\t\trendered_hunk_index = -1;\n-\t\t} else {\n+\t\t} else if (s->answer.buf[0] == '?') {\n \t\t\tconst char *p = _(help_patch_remainder), *eol = p;\n \n \t\t\tcolor_fprintf(stdout, s->s.help_color, \"%s\",\n@@ -1691,6 +1691,9 @@ static int patch_update_file(struct add_p_state *s,\n \t\t\t\tcolor_fprintf_ln(stdout, s->s.help_color,\n \t\t\t\t\t\t \"%.*s\", (int)(eol - p), p);\n \t\t\t}\n+\t\t} else {\n+\t\t\terr(s, _(\"Unknown option '%s' (use '?' for help)\"),\n+\t\t\t s->answer.buf);\n \t\t}\n \t}\n \ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex bc55255b0a..b38fd5388a 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -61,6 +61,16 @@ test_expect_success 'setup (initial)' '\n \techo more >>file &&\n \techo lines >>file\n '\n+\n+test_expect_success 'invalid option' '\n+\tcat >expect <<-EOF &&\n+\tUnknown option ${SQ}W${SQ} (use ${SQ}?${SQ} for help)\n+\tEOF\n+\ttest_write_lines W |\n+\tgit -c core.filemode=true add -p 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'status works (initial)' '\n \tgit add -i output &&\n \tgrep \"+1/-0 *+2/-0 file\" output\n-- \n2.44.0.782.g480309b2c8\n\n\n"},{"id":"492984","messageId":"Zh4R5yonRLBcunCR@tanuki","threadId":"61323","inReplyTo":"4e2bc660-ee33-4641-aca5-783d0cefcd23@gmail.com","subject":"Re: [PATCH] add-patch: response to invalid option","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-16T05:51:35Z","receivedAt":"2024-04-16T05:51:43Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Apr 15, 2024 at 09:00:28PM +0200, Rubén Justo wrote:\n> When the user introduces an invalid option, we respond with the whole\n> help text.\n> \n> Instead of displaying the long help description, display a short error\n> message indicating the incorrectly introduced option with a note on how\n> to get the help text.\n> \n> Signed-off-by: Rubén Justo \n> ---\n> add-patch.c | 5 ++++-\n> t/t3701-add-interactive.sh | 10 ++++++++++\n> 2 files changed, 14 insertions(+), 1 deletion(-)\n> \n> diff --git a/add-patch.c b/add-patch.c\n> index a06dd18985..c77902fec5 100644\n> --- a/add-patch.c\n> +++ b/add-patch.c\n> @@ -1667,7 +1667,7 @@ static int patch_update_file(struct add_p_state *s,\n> \t\t\t}\n> \t\t} else if (s->answer.buf[0] == 'p') {\n> \t\t\trendered_hunk_index = -1;\n> -\t\t} else {\n> +\t\t} else if (s->answer.buf[0] == '?') {\n> \t\t\tconst char *p = _(help_patch_remainder), *eol = p;\n> \n> \t\t\tcolor_fprintf(stdout, s->s.help_color, \"%s\",\n\nI think it would've made sense to describe this change in the commit\nmessage, as well. Currently, the reader is left wondering whether \"?\" is\na new shortcut that you introduce in this patch or whether it is already\ndocumented as such.\n\n> @@ -1691,6 +1691,9 @@ static int patch_update_file(struct add_p_state *s,\n> \t\t\t\tcolor_fprintf_ln(stdout, s->s.help_color,\n> \t\t\t\t\t\t \"%.*s\", (int)(eol - p), p);\n> \t\t\t}\n> +\t\t} else {\n> +\t\t\terr(s, _(\"Unknown option '%s' (use '?' for help)\"),\n> +\t\t\t s->answer.buf);\n> \t\t}\n> \t}\n> \n\nI was wondering why we have `err()` here, and whether we shouldn't\nconvert this to use `error()` instead. Similarly, I was wondering\nwhether we should convert the error message to start with a lower-case\nletter to match our other errors.\n\nBut scanning through \"add-patch.c\" I don't think that makes sense.\n`err()` knows to handle colors, which `error()` doesn't. And given that\nthis is an interactive interface where all the other error messages\nstart with an upper-case letter, too, it would feel out of place to\nadapt the error message.\n\nSo all of this is just to say that this looks sensible to me.\n\n> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\n> index bc55255b0a..b38fd5388a 100755\n> --- a/t/t3701-add-interactive.sh\n> +++ b/t/t3701-add-interactive.sh\n> @@ -61,6 +61,16 @@ test_expect_success 'setup (initial)' '\n> \techo more >>file &&\n> \techo lines >>file\n> '\n> +\n> +test_expect_success 'invalid option' '\n> +\tcat >expect <<-EOF &&\n> +\tUnknown option ${SQ}W${SQ} (use ${SQ}?${SQ} for help)\n> +\tEOF\n> +\ttest_write_lines W |\n> +\tgit -c core.filemode=true add -p 2>actual &&\n\nNit: it might be sensible to write the lines into a temporary file first\nso that Git isn't spawned as part of a pipeline. But on the other hand\nit's fine to have Git on the right-hand side of pipelines, so this way\nto write it is fine, too.\n\nPatrick\n\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_expect_success 'status works (initial)' '\n> \tgit add -i output &&\n> \tgrep \"+1/-0 *+2/-0 file\" output\n> -- \n> 2.44.0.782.g480309b2c8\n> \n> \n> \n"},{"id":"492987","messageId":"15f9252c-212f-43eb-84f3-6046fb2fab38@gmail.com","threadId":"61323","inReplyTo":"4e2bc660-ee33-4641-aca5-783d0cefcd23@gmail.com","subject":"Re: [PATCH] add-patch: response to invalid option","fromName":"","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-04-16T09:41:32Z","receivedAt":"2024-04-16T09:41:37Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Rubén\n\nThanks for working on this, it is a nice follow up to your last series.\n\nOn 15/04/2024 20:00, Rubén Justo wrote:\n>\n> +\t\t} else {\n> +\t\t\terr(s, _(\"Unknown option '%s' (use '?' for help)\"),\n\nAs this is an interactive program I think \"Unknown key\" would be clearer.\n\n> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\n> index bc55255b0a..b38fd5388a 100755\n> --- a/t/t3701-add-interactive.sh\n> +++ b/t/t3701-add-interactive.sh\n> @@ -61,6 +61,16 @@ test_expect_success 'setup (initial)' '\n> \techo more >>file &&\n> \techo lines >>file\n> '\n> +\n> +test_expect_success 'invalid option' '\n> +\tcat >expect <<-EOF &&\n> +\tUnknown option ${SQ}W${SQ} (use ${SQ}?${SQ} for help)\n> +\tEOF\n> +\ttest_write_lines W |\n> +\tgit -c core.filemode=true add -p 2>actual &&\n> +\ttest_cmp expect actual\n> +'\n\nI was confused by this test as \"add -p\" doesn't seem to be printing\nanything apart from the error. That's because it only captures stderr\n(quite why an interactive program is writing its some of its output to\nstderr and the rest to stdout is another question). I think we want to\ncapture the whole output otherwise we can't assert that the help isn't\nprinted. Something like this (which also adds coverage for '?' and 'p')\n\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex bc55255b0a8..0fc7d4b5d89 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -1130,4 +1130,26 @@ test_expect_success 'reset -p with unmerged files' '\n test_must_be_empty staged\n '\n \n+test_expect_success 'invalid key' '\n+ echo changed >file &&\n+ test_write_lines æ \\? p q | force_color git add -p >actual.colored 2>&1 &&\n+ test_decode_color actual &&\n+ force_color git diff >diff.colored &&\n+ test_decode_color diff.decoded &&\n+ cat diff.decoded >expect &&\n+ cat >>expect <<-EOF &&\n+ (1/1) Stage this hunk [y,n,q,a,d,e,p,?]? Unknown key ${SQ}æ${SQ}. Use ${SQ}?${SQ} for help\n+ (1/1) Stage this hunk [y,n,q,a,d,e,p,?]? y - stage this hunk\n+ n - do not stage this hunk\n+ q - quit; do not stage this hunk or any of the remaining ones\n+ a - stage this hunk and all later hunks in the file\n+ d - do not stage this hunk or any of the later hunks in the file\n+ e - manually edit the current hunk\n+ p - print the current hunk\n+ ? - print help\n+ (1/1) Stage this hunk [y,n,q,a,d,e,p,?]? $(sed -n \"/@/,\\$ p\" diff.decoded)\n+ (1/1) Stage this hunk [y,n,q,a,d,e,p,?]? \n+ EOF\n+ test_cmp expect actual\n+'\n+\n test_done\n\nBest Wishes\n\nPhillip\n"},{"id":"492989","messageId":"xmqq5xwhhacm.fsf@gitster.g","threadId":"61323","inReplyTo":"4e2bc660-ee33-4641-aca5-783d0cefcd23@gmail.com","subject":"Re: [PATCH] add-patch: response to invalid option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-16T10:26:33Z","receivedAt":"2024-04-16T10:26:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> When the user introduces an invalid option, we respond with the whole\n> help text.\n\nThe verb \"introduces\" in the first sentence looked weird to me.\n\n add -p: require two steps to get help after mistyping an option\n\n During a \"git add -p\" session, if the user chooses an option\n that is not offered, the help text for the entire available\n choices is given.\n\nor something, perhaps.\n\n> Instead of displaying the long help description, display a short error\n> message indicating the incorrectly introduced option with a note on how\n> to get the help text.\n\nWhen you wanted to 'q'uit but touched the 'w' key that sits next to\nit by mistake, you do not need to be told about what any of the\noptions would do; you may want to be told \"the option to quit is\n'q', not 'w'\", though ;-).\n\nOn the other hand, if you are truly lost and do not know what each\nof the listed choices mean, you'd type '?' anyway because that is\none of the offered choices. So the only change needed here is to\nmake sure that '?' is the only thing that gives the help message,\nand all other unrecognised option 'x' are made to say \"we do not\nknow 'x'\".\n\nThat flow of thought makes sort-of sense, if the choices that are\noffered are too numerous (say, around a dozen or more), but with the\ncurrent command set, I am not sure if this change is an improvement\n(note: I did not say \"I do not think that\"---I simply am not sure).\n\nIf we implemented the UI this way 20 years ago in the first version,\nperhaps we would have had happily been using it since, but given\nthat the way we implemented the UI 20 years ago has been used\nhappily by our users without much complaint, either way must be just\nfine. \n\nIs it worth changing it at this point? Does it improve the end-user\nexperience in any noticeable way? I do not think I can answer these\ntwo questions with confident \"yes\".\n\n> Signed-off-by: Rubén Justo \n> ---\n> add-patch.c | 5 ++++-\n> t/t3701-add-interactive.sh | 10 ++++++++++\n> 2 files changed, 14 insertions(+), 1 deletion(-)\n>\n> diff --git a/add-patch.c b/add-patch.c\n> index a06dd18985..c77902fec5 100644\n> --- a/add-patch.c\n> +++ b/add-patch.c\n> @@ -1667,7 +1667,7 @@ static int patch_update_file(struct add_p_state *s,\n> \t\t\t}\n> \t\t} else if (s->answer.buf[0] == 'p') {\n> \t\t\trendered_hunk_index = -1;\n> -\t\t} else {\n> +\t\t} else if (s->answer.buf[0] == '?') {\n> \t\t\tconst char *p = _(help_patch_remainder), *eol = p;\n> \n> \t\t\tcolor_fprintf(stdout, s->s.help_color, \"%s\",\n> @@ -1691,6 +1691,9 @@ static int patch_update_file(struct add_p_state *s,\n> \t\t\t\tcolor_fprintf_ln(stdout, s->s.help_color,\n> \t\t\t\t\t\t \"%.*s\", (int)(eol - p), p);\n> \t\t\t}\n> +\t\t} else {\n> +\t\t\terr(s, _(\"Unknown option '%s' (use '?' for help)\"),\n> +\t\t\t s->answer.buf);\n> \t\t}\n> \t}\n> \n> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\n> index bc55255b0a..b38fd5388a 100755\n> --- a/t/t3701-add-interactive.sh\n> +++ b/t/t3701-add-interactive.sh\n> @@ -61,6 +61,16 @@ test_expect_success 'setup (initial)' '\n> \techo more >>file &&\n> \techo lines >>file\n> '\n> +\n> +test_expect_success 'invalid option' '\n> +\tcat >expect <<-EOF &&\n> +\tUnknown option ${SQ}W${SQ} (use ${SQ}?${SQ} for help)\n> +\tEOF\n> +\ttest_write_lines W |\n> +\tgit -c core.filemode=true add -p 2>actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_expect_success 'status works (initial)' '\n> \tgit add -i output &&\n> \tgrep \"+1/-0 *+2/-0 file\" output\n"},{"id":"492993","messageId":"64d4ba8e-0dfe-407c-9b32-d02f1ce40393@gmail.com","threadId":"61323","inReplyTo":"xmqq5xwhhacm.fsf@gitster.g","subject":"Re: [PATCH] add-patch: response to invalid option","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-04-16T13:56:37Z","receivedAt":"2024-04-16T13:56:43Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Junio\n\nOn 16/04/2024 11:26, Junio C Hamano wrote:\n> Rubén Justo writes:\n> \n>> When the user introduces an invalid option, we respond with the whole\n>> help text.\n> \n> The verb \"introduces\" in the first sentence looked weird to me.\n> \n> add -p: require two steps to get help after mistyping an option\n> \n> During a \"git add -p\" session, if the user chooses an option\n> that is not offered, the help text for the entire available\n> choices is given.\n\nI find this suggestion clearer as well.\n\n> [...] \n> On the other hand, if you are truly lost and do not know what each\n> of the listed choices mean, you'd type '?' anyway because that is\n> one of the offered choices. So the only change needed here is to\n> make sure that '?' is the only thing that gives the help message,\n> and all other unrecognised option 'x' are made to say \"we do not\n> know 'x'\".\n> \n> That flow of thought makes sort-of sense, if the choices that are\n> offered are too numerous (say, around a dozen or more), but with the\n> current command set, I am not sure if this change is an improvement\n> (note: I did not say \"I do not think that\"---I simply am not sure).\n\nThe complete list of help is 15 lines long (we don't always print \neverything but if you're in the middle of staging several hunks from the \nsame file we do)\n\n y - stage this hunk\n n - do not stage this hunk\n q - quit; do not stage this hunk or any of the remaining ones\n a - stage this hunk and all later hunks in the file\n d - do not stage this hunk or any of the later hunks in the file\n j - leave this hunk undecided, see next undecided hunk\n J - leave this hunk undecided, see next hunk\n k - leave this hunk undecided, see previous undecided hunk\n K - leave this hunk undecided, see previous hunk\n g - select a hunk to go to\n / - search for a hunk matching the given regex\n s - split the current hunk into smaller hunks\n e - manually edit the current hunk\n p - print the current hunk\n ? - print help\n\nI find it long enough to be annoying as it means there is a significant \ndistance between the prompt and the hunk text.\n\n> If we implemented the UI this way 20 years ago in the first version,\n> perhaps we would have had happily been using it since, but given\n> that the way we implemented the UI 20 years ago has been used\n> happily by our users without much complaint, either way must be just\n> fine.\n> \n> Is it worth changing it at this point? Does it improve the end-user\n> experience in any noticeable way? I do not think I can answer these\n> two questions with confident \"yes\".\n\nPersonally I think it would be an improvement but I suspect it is a \nmatter of opinion.\n\nBest Wishes\n\nPhillip\n\n>> Signed-off-by: Rubén Justo \n>> ---\n>> add-patch.c | 5 ++++-\n>> t/t3701-add-interactive.sh | 10 ++++++++++\n>> 2 files changed, 14 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/add-patch.c b/add-patch.c\n>> index a06dd18985..c77902fec5 100644\n>> --- a/add-patch.c\n>> +++ b/add-patch.c\n>> @@ -1667,7 +1667,7 @@ static int patch_update_file(struct add_p_state *s,\n>> \t\t\t}\n>> \t\t} else if (s->answer.buf[0] == 'p') {\n>> \t\t\trendered_hunk_index = -1;\n>> -\t\t} else {\n>> +\t\t} else if (s->answer.buf[0] == '?') {\n>> \t\t\tconst char *p = _(help_patch_remainder), *eol = p;\n>> \n>> \t\t\tcolor_fprintf(stdout, s->s.help_color, \"%s\",\n>> @@ -1691,6 +1691,9 @@ static int patch_update_file(struct add_p_state *s,\n>> \t\t\t\tcolor_fprintf_ln(stdout, s->s.help_color,\n>> \t\t\t\t\t\t \"%.*s\", (int)(eol - p), p);\n>> \t\t\t}\n>> +\t\t} else {\n>> +\t\t\terr(s, _(\"Unknown option '%s' (use '?' for help)\"),\n>> +\t\t\t s->answer.buf);\n>> \t\t}\n>> \t}\n>> \n>> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\n>> index bc55255b0a..b38fd5388a 100755\n>> --- a/t/t3701-add-interactive.sh\n>> +++ b/t/t3701-add-interactive.sh\n>> @@ -61,6 +61,16 @@ test_expect_success 'setup (initial)' '\n>> \techo more >>file &&\n>> \techo lines >>file\n>> '\n>> +\n>> +test_expect_success 'invalid option' '\n>> +\tcat >expect <<-EOF &&\n>> +\tUnknown option ${SQ}W${SQ} (use ${SQ}?${SQ} for help)\n>> +\tEOF\n>> +\ttest_write_lines W |\n>> +\tgit -c core.filemode=true add -p 2>actual &&\n>> +\ttest_cmp expect actual\n>> +'\n>> +\n>> test_expect_success 'status works (initial)' '\n>> \tgit add -i output &&\n>> \tgrep \"+1/-0 *+2/-0 file\" output\n> \n"},{"id":"492994","messageId":"xmqqr0f5fi2b.fsf@gitster.g","threadId":"61323","inReplyTo":"64d4ba8e-0dfe-407c-9b32-d02f1ce40393@gmail.com","subject":"Re: [PATCH] add-patch: response to invalid option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-16T15:22:52Z","receivedAt":"2024-04-16T15:22:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n>> That flow of thought makes sort-of sense, if the choices that are\n>> offered are too numerous (say, around a dozen or more), but with the\n>> current command set, I am not sure if this change is an improvement\n>> (note: I did not say \"I do not think that\"---I simply am not sure).\n>\n> The complete list of help is 15 lines long (we don't always print\n> everything but if you're in the middle of staging several hunks from\n> the same file we do)\n\nAh, OK. Even though I was the one who stole this feature from\nelsewhere, I forgot that we had that many commands, and it makes the\ndecision a lot simpler ;-)\n\n> y - stage this hunk\n> n - do not stage this hunk\n> q - quit; do not stage this hunk or any of the remaining ones\n> a - stage this hunk and all later hunks in the file\n> d - do not stage this hunk or any of the later hunks in the file\n> j - leave this hunk undecided, see next undecided hunk\n> J - leave this hunk undecided, see next hunk\n> k - leave this hunk undecided, see previous undecided hunk\n> K - leave this hunk undecided, see previous hunk\n> g - select a hunk to go to\n> / - search for a hunk matching the given regex\n> s - split the current hunk into smaller hunks\n> e - manually edit the current hunk\n> p - print the current hunk\n> ? - print help\n>\n> I find it long enough to be annoying as it means there is a\n> significant distance between the prompt and the hunk text.\n\nYes, I now agree.\n\nSo the log message may want a bit of rewrite in the title and the\nintroductory part, with a clarification that '?' is what we already\nhad and not what this adds (from Patrick's comment), and there may\nbe other small improvements we want to see that I may have missed,\nbut other than these small-ish points, the basic direction of this\npatch is good?\n\nThis is a tangent, but I think we _mostly_ take these commands case\ninsensitively (e.g., you can say 'Q' to quit the program, as well as\n'q' that is listed above). But 'k' and 'j' (there may be others)\nact differently when given in the uppercase. This would limit our\nability to later add capitalized variant of an already used lower\ncase letter without retraining users---should we find it disturbing,\nor is \"add -p\" mostly done and we do not have to worry?\n\nThanks.\n"},{"id":"492997","messageId":"e2e72c39-2975-4307-a356-19e661cafb75@gmail.com","threadId":"61323","inReplyTo":"xmqqr0f5fi2b.fsf@gitster.g","subject":"Re: [PATCH] add-patch: response to invalid option","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-04-16T15:46:59Z","receivedAt":"2024-04-16T15:47:02Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 16/04/2024 16:22, Junio C Hamano wrote:\n> Phillip Wood writes:\n> \n>>> That flow of thought makes sort-of sense, if the choices that are\n>>> offered are too numerous (say, around a dozen or more), but with the\n>>> current command set, I am not sure if this change is an improvement\n>>> (note: I did not say \"I do not think that\"---I simply am not sure).\n>>\n>> The complete list of help is 15 lines long (we don't always print\n>> everything but if you're in the middle of staging several hunks from\n>> the same file we do)\n> \n> Ah, OK. Even though I was the one who stole this feature from\n> elsewhere, I forgot that we had that many commands, and it makes the\n> decision a lot simpler ;-)\n\nYes, it always surprises me how many there are when the help is printed.\n\n> So the log message may want a bit of rewrite in the title and the\n> introductory part, with a clarification that '?' is what we already\n> had and not what this adds (from Patrick's comment), and there may\n> be other small improvements we want to see that I may have missed,\n> but other than these small-ish points, the basic direction of this\n> patch is good?\n\nI think so.\n\n> This is a tangent, but I think we _mostly_ take these commands case\n> insensitively (e.g., you can say 'Q' to quit the program, as well as\n> 'q' that is listed above). But 'k' and 'j' (there may be others)\n> act differently when given in the uppercase.\n\n'J' and 'K' are the only keys where we have different actions for upper \nand lowers cases. The implementation looks a bit confused though - we \ntreat y,n,q,a,d case insensitively but not g,s,e,p. As no one has \ncomplained about the latter we should perhaps leave them as is in case \nwe ever want to add upper case variants.\n\n> This would limit our\n> ability to later add capitalized variant of an already used lower\n> case letter without retraining users---should we find it disturbing,\n> or is \"add -p\" mostly done and we do not have to worry?\n\nI think its mostly done. I'd like a way of selecting lines from a hunk \nthat does not involve making the user edit the hunk and I wish the \nsearch remembered the last string but I don't think we're in danger of \nrunning out of keys for new commands.\n\nBest Wishes\n\nPhillip\n\n"},{"id":"492999","messageId":"xmqq4jc1ffu9.fsf@gitster.g","threadId":"61323","inReplyTo":"e2e72c39-2975-4307-a356-19e661cafb75@gmail.com","subject":"Re: [PATCH] add-patch: response to invalid option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-16T16:10:54Z","receivedAt":"2024-04-16T16:11:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n>> Ah, OK. Even though I was the one who stole this feature from\n>> elsewhere, I forgot that we had that many commands, and it makes the\n>> decision a lot simpler ;-)\n>\n> Yes, it always surprises me how many there are when the help is printed.\n>\n>> So the log message may want a bit of rewrite in the title and the\n>> introductory part, with a clarification that '?' is what we already\n>> had and not what this adds (from Patrick's comment), and there may\n>> be other small improvements we want to see that I may have missed,\n>> but other than these small-ish points, the basic direction of this\n>> patch is good?\n>\n> I think so.\n\nOK, let me mark the topic as expecting hopefully a final reroll\nin the draft \"What's cooking\" report.\n\nThanks.\n"},{"id":"493001","messageId":"6780c9b6-ece6-4bce-b020-d1f828f3fb73@gmail.com","threadId":"61323","inReplyTo":"Zh4R5yonRLBcunCR@tanuki","subject":"Re: [PATCH] add-patch: response to invalid option","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-04-16T19:11:38Z","receivedAt":"2024-04-16T19:11:41Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Tue, Apr 16, 2024 at 07:51:35AM +0200, Patrick Steinhardt wrote:\n\n> I think it would've made sense to describe this change in the commit\n> message, as well. Currently, the reader is left wondering whether \"?\" is\n> a new shortcut that you introduce in this patch or whether it is already\n> documented as such.\n\nPerhaps, something like?:\n\n\tInstead of displaying the long help description, display a short\n\terror message indicating the incorrectly introduced option with\n\ta note on how to use the '?' option, to get the help text.\n\nThanks.\n"},{"id":"493002","messageId":"20685fa0-815f-4cdf-95e0-7206588552b5@gmail.com","threadId":"61323","inReplyTo":"15f9252c-212f-43eb-84f3-6046fb2fab38@gmail.com","subject":"Re: [PATCH] add-patch: response to invalid option","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-04-16T19:24:39Z","receivedAt":"2024-04-16T19:24:42Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Tue, Apr 16, 2024 at 10:41:32AM +0100, phillip.wood123@gmail.com wrote:\n\n> Thanks for working on this, it is a nice follow up to your last series.\n\nThank you for your interest!\n\n> \n> On 15/04/2024 20:00, Rubén Justo wrote:\n> > \n> > +\t\t} else {\n> > +\t\t\terr(s, _(\"Unknown option '%s' (use '?' for help)\"),\n> \n> As this is an interactive program I think \"Unknown key\" would be clearer.\n\nMaybe you have \"interactive.singleKey\" set to \"true\"?\n\nPerhaps \"key\" refers more to the key pressed by the user. My impression\nis that \"option\" have a clearer and wider audience.\n\n> Something like this (which also adds coverage for '?' and 'p')\n\nI know we don't have a test for the help. I noticed this in my other\nseries. And I decided not to include a test for it then. Maybe in this\nseries it makes sense.\n\nHowever, I would prefer to keep a test only on the new error message.\n\nThanks.\n"},{"id":"493003","messageId":"18640bf4-aefb-4057-a680-3e47f10d88e2@gmail.com","threadId":"61323","inReplyTo":"xmqq5xwhhacm.fsf@gitster.g","subject":"Re: [PATCH] add-patch: response to invalid option","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-04-16T19:31:18Z","receivedAt":"2024-04-16T19:31:21Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Tue, Apr 16, 2024 at 03:26:33AM -0700, Junio C Hamano wrote:\n\n> > When the user introduces an invalid option, we respond with the whole\n> > help text.\n> \n> The verb \"introduces\" in the first sentence looked weird to me.\n\nOK. I'll reword it.\n\n> > Instead of displaying the long help description, display a short error\n> > message indicating the incorrectly introduced option with a note on how\n> > to get the help text.\n\n> Is it worth changing it at this point? Does it improve the end-user\n> experience in any noticeable way? I do not think I can answer these\n> two questions with confident \"yes\".\n\nIndeed, this has been with us for a long time. We're not fixing or\nchanging any normal usage here.\n\nI know you know, but let me put it this way; We are introducing two\nchanges here:\n\n - a new error message to inform the user that an invalid option has\n been entered. And,\n\n - not displaying the help if not requested.\n\nBoth are improvements to the user experience, but especially the former,\nI think.\n\nThis:\n\n\t$ echo W | git add -p \n\tdiff --git a/add-interactive.c b/add-interactive.c\n\t[...]\n\t(1/1) Stage this hunk [y,n,q,a,d,e,?]? y - stage this hunk\n\tn - do not stage this hunk\n\tq - quit; do not stage this hunk or any of the remaining ones\n\ta - stage this hunk and all later hunks in the file\n\t[...]\n\nBecomes:\n\n\t$ echo W | ./git add -p\n\tdiff --git a/add-interactive.c b/add-interactive.c\n\t[...]\n\t(1/1) Stage this hunk [y,n,q,a,d,e,p,?]? Unknown option 'W' (use '?' for help)\n\nThanks.\n"},{"id":"493082","messageId":"1d0e98cb-78a4-40d0-9bfe-390a3a30aad8@gmail.com","threadId":"61323","inReplyTo":"20685fa0-815f-4cdf-95e0-7206588552b5@gmail.com","subject":"Re: [PATCH] add-patch: response to invalid option","fromName":"","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-04-17T09:37:39Z","receivedAt":"2024-04-17T09:37:42Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Rubén\n\nOn 16/04/2024 20:24, Rubén Justo wrote:\n> On Tue, Apr 16, 2024 at 10:41:32AM +0100, phillip.wood123@gmail.com wrote:\n>> On 15/04/2024 20:00, Rubén Justo wrote:\n>>>\n>>> +\t\t} else {\n>>> +\t\t\terr(s, _(\"Unknown option '%s' (use '?' for help)\"),\n>>\n>> As this is an interactive program I think \"Unknown key\" would be clearer.\n> \n> Maybe you have \"interactive.singleKey\" set to \"true\"?\n> \n> Perhaps \"key\" refers more to the key pressed by the user. My impression\n> is that \"option\" have a clearer and wider audience.\n\nI tend to associate \"option\" with a command-line argument, not \ninteractive input to a program.\n\n>> Something like this (which also adds coverage for '?' and 'p')\n> \n> I know we don't have a test for the help. I noticed this in my other\n> series. And I decided not to include a test for it then. Maybe in this\n> series it makes sense.\n> \n> However, I would prefer to keep a test only on the new error message.\n\nWhy? This commit makes three changes\n - it stops printing the help when the user presses an unknown key\n - it starts treating '?' differently to an unknown key\n - it starts printing an error message when the user presses an unknown\n key\n\nThe test you are proposing only tests the last of these changes. We \nshould be aiming to write tests that (a) verify all of the changes \nintroduced by a commit (b) are likely to detect regressions to those \nchanges (c) are reasonably efficient, for example if it is possible to \ntest more than one key with a single \"add -p\" process we should do so. \nAs this is an interactive program I have a strong preference for testing \nwhat the user sees printed to their screen, not just what happens to \ncome out on stderr.\n\nThanks\n\nPhillip\n"},{"id":"493093","messageId":"xmqqh6g0av28.fsf@gitster.g","threadId":"61323","inReplyTo":"1d0e98cb-78a4-40d0-9bfe-390a3a30aad8@gmail.com","subject":"Re: [PATCH] add-patch: response to invalid option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-17T15:05:35Z","receivedAt":"2024-04-17T15:05:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"phillip.wood123@gmail.com writes:\n\n> I tend to associate \"option\" with a command-line argument, not\n> interactive input to a program.\n\n\"git add --help\" is a bit mixed. The choices offered by \"git add\n-i\" are called \"subcommand\" (see \"INTERACTIVE MODE\" section), but\nthe choices you give to the prompt \"patch\" subcommand gives you are\npresented with \"You can select one of the following options and type\nreturn\". So \"option\" is not too wrong, even though it is a word\nused in other contexts as well. I am OK with \"option\", but if I\nwere adding this new error message, I probably would have said\n\"unknown command\".\n\nIn any case, whether you said option, command, or key , it is so\nobvious from the context that we could even say \"error: 'W' not\nknown, use '?' for help\" without any noun there, so it would not\nmatter too much which noun you pick.\n\nI'd still avoid \"key\", though, because to those who do not do\nsingle-key input, myself included, it does not match their user\nexperience, and it is even more so if they forgot or do not even\nknow that they could choose to use single-key input.\n\n> The test you are proposing only tests the last of these changes. We\n> should be aiming to write tests that (a) verify all of the changes\n> introduced by a commit (b) are likely to detect regressions to those\n> changes (c) are reasonably efficient, for example if it is possible to\n> test more than one key with a single \"add -p\" process we should do\n> so. As this is an interactive program I have a strong preference for\n> testing what the user sees printed to their screen, not just what\n> happens to come out on stderr.\n\nI do agree with these three points, but I do not have a strong\nopinion on the new test that was added by the patch when judging\nwith them used as a yardstick.\n\nThanks.\n"},{"id":"493144","messageId":"52102ebc-ae57-4000-9990-7def910ba254@gmail.com","threadId":"61323","inReplyTo":"xmqqh6g0av28.fsf@gitster.g","subject":"Re: [PATCH] add-patch: response to invalid option","fromName":"","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-04-18T15:11:33Z","receivedAt":"2024-04-18T15:11:36Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 17/04/2024 16:05, Junio C Hamano wrote:\n> phillip.wood123@gmail.com writes:\n> \n>> I tend to associate \"option\" with a command-line argument, not\n>> interactive input to a program.\n> \n> \"git add --help\" is a bit mixed. The choices offered by \"git add\n> -i\" are called \"subcommand\" (see \"INTERACTIVE MODE\" section), but\n> the choices you give to the prompt \"patch\" subcommand gives you are\n> presented with \"You can select one of the following options and type\n> return\". So \"option\" is not too wrong, even though it is a word\n> used in other contexts as well. I am OK with \"option\", but if I\n> were adding this new error message, I probably would have said\n> \"unknown command\".\n\nI think \"unknown command\" is a good suggestion, I take your point about \n\"unknown key\" not being so clear for users who do not use single-key input.\n\nBest Wishes\n\nPhillip\n\n> In any case, whether you said option, command, or key , it is so\n> obvious from the context that we could even say \"error: 'W' not\n> known, use '?' for help\" without any noun there, so it would not\n> matter too much which noun you pick.\n> \n> I'd still avoid \"key\", though, because to those who do not do\n> single-key input, myself included, it does not match their user\n> experience, and it is even more so if they forgot or do not even\n> know that they could choose to use single-key input.\n> \n>> The test you are proposing only tests the last of these changes. We\n>> should be aiming to write tests that (a) verify all of the changes\n>> introduced by a commit (b) are likely to detect regressions to those\n>> changes (c) are reasonably efficient, for example if it is possible to\n>> test more than one key with a single \"add -p\" process we should do\n>> so. As this is an interactive program I have a strong preference for\n>> testing what the user sees printed to their screen, not just what\n>> happens to come out on stderr.\n> \n> I do agree with these three points, but I do not have a strong\n> opinion on the new test that was added by the patch when judging\n> with them used as a yardstick.\n> \n> Thanks.\n"},{"id":"493247","messageId":"6d421c67-9e10-4a7b-9782-38ba8e9da915@gmail.com","threadId":"61323","inReplyTo":"4e2bc660-ee33-4641-aca5-783d0cefcd23@gmail.com","subject":"[PATCH v2] add-patch: response to invalid command","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-04-20T11:08:14Z","receivedAt":"2024-04-20T11:08:23Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"When the user enters an invalid command, we respond with a list of\naccepted commands; the response we give to the command '?'.\n\nHowever, the invalid command may be due to either a user input error or\na malfunctioning interface component, rather than the user not knowing\nthe valid command.\n\nOur response is unlikely to provide help in such situations.\n\nTo reduce the likelihood of user confusion and error repetition, if an\nunrecognized command is received, stop displaying the help text and\ndisplay a short error message with the invalid command received, as\nfeedback to the user.\n\nInclude a reminder about the current command '?' in the new message, to\nguide the user if they want help.\n\nSigned-off-by: Rubén Justo \n---\n\nChanges since v1:\n\n - Use a temporary file for the lines.\n - Say explicitly in the message that '?' is an existing command.\n - Instead of \"option\", use \"command\".\n - Test for stdout, not just stderr.\n\n add-patch.c | 5 ++++-\n t/t3701-add-interactive.sh | 23 ++++++++++++++++++++++-\n 2 files changed, 26 insertions(+), 2 deletions(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex a06dd18985..7be142d448 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -1667,7 +1667,7 @@ static int patch_update_file(struct add_p_state *s,\n \t\t\t}\n \t\t} else if (s->answer.buf[0] == 'p') {\n \t\t\trendered_hunk_index = -1;\n-\t\t} else {\n+\t\t} else if (s->answer.buf[0] == '?') {\n \t\t\tconst char *p = _(help_patch_remainder), *eol = p;\n \n \t\t\tcolor_fprintf(stdout, s->s.help_color, \"%s\",\n@@ -1691,6 +1691,9 @@ static int patch_update_file(struct add_p_state *s,\n \t\t\t\tcolor_fprintf_ln(stdout, s->s.help_color,\n \t\t\t\t\t\t \"%.*s\", (int)(eol - p), p);\n \t\t\t}\n+\t\t} else {\n+\t\t\terr(s, _(\"Unknown command '%s' (use '?' for help)\"),\n+\t\t\t s->answer.buf);\n \t\t}\n \t}\n \ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex bc55255b0a..4c3901de17 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -7,6 +7,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-terminal.sh\n \n+SP=\" \"\n+\n diff_cmp () {\n \tfor x\n \tdo\n@@ -61,6 +63,26 @@ test_expect_success 'setup (initial)' '\n \techo more >>file &&\n \techo lines >>file\n '\n+\n+test_expect_success 'unknown command' '\n+\ttest_when_finished \"git reset command && rm command\" &&\n+\techo W >command &&\n+\tgit add -N command &&\n+\tcat >expect <<-EOF &&\n+\tdiff --git a/command b/command\n+\tnew file mode 100644\n+\tindex 0000000..a42d8ff\n+\t--- /dev/null\n+\t+++ b/command\n+\t@@ -0,0 +1 @@\n+\t+W\n+\t(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help)\n+\t(1/1) Stage addition [y,n,q,a,d,e,p,?]?$SP\n+\tEOF\n+\tgit add -p -- command actual 2>&1 &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'status works (initial)' '\n \tgit add -i output &&\n \tgrep \"+1/-0 *+2/-0 file\" output\n@@ -231,7 +253,6 @@ test_expect_success 'setup file' '\n '\n \n test_expect_success 'setup patch' '\n-\tSP=\" \" &&\n \tNULL=\"\" &&\n \tcat >patch <<-EOF\n \t@@ -1,4 +1,4 @@\n-- \n2.45.0.rc0.1.gc94c838fad\n"},{"id":"493251","messageId":"xmqqr0ez3opi.fsf@gitster.g","threadId":"61323","inReplyTo":"6d421c67-9e10-4a7b-9782-38ba8e9da915@gmail.com","subject":"Re: [PATCH v2] add-patch: response to invalid command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-20T17:53:45Z","receivedAt":"2024-04-20T17:53:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> When the user enters an invalid command, we respond with a list of\n> accepted commands; the response we give to the command '?'.\n\nI am guessing that you want to say with the part of the sentence\nafter the semicolon that the list of accepted commands and the\nexplanation for them is the same as what we give when the user asks\n'?', but that is after reading the above twice. I wonder\n\n When the user gives an unknown command to the \"add -p\" prompt,\n the list of accepted commands with their explanation is given.\n This is the same output they get when they say `?`.\n\nis easier to understand?\n\n> However, the invalid command may be due to either a user input error or\n> a malfunctioning interface component, rather than the user not knowing\n> the valid command.\n\nI am not sure readers would understand what you are trying to refer\nto with \"or a malfunctioning interface component\". I don't, at\nleast. Either rewrite it to be understandable, or drop it. I think\ndropping it is sufficient in this case.\n\n> Our response is unlikely to provide help in such situations.\n>\n> To reduce the likelihood of user confusion and error repetition, if an\n> unrecognized command is received, stop displaying the help text and\n> display a short error message with the invalid command received, as\n> feedback to the user.\n\n\"stop ... text and\" -> \"instead of ... text,\"\n\nIt would give a better contrast between the current UI and the\nproposed new one.\n\n> Include a reminder about the current command '?' in the new message, to\n> guide the user if they want help.\n\nOK.\n\n> diff --git a/add-patch.c b/add-patch.c\n> index a06dd18985..7be142d448 100644\n> --- a/add-patch.c\n> +++ b/add-patch.c\n> @@ -1667,7 +1667,7 @@ static int patch_update_file(struct add_p_state *s,\n> \t\t\t}\n> \t\t} else if (s->answer.buf[0] == 'p') {\n> \t\t\trendered_hunk_index = -1;\n> -\t\t} else {\n> +\t\t} else if (s->answer.buf[0] == '?') {\n> \t\t\tconst char *p = _(help_patch_remainder), *eol = p;\n> \n> \t\t\tcolor_fprintf(stdout, s->s.help_color, \"%s\",\n> @@ -1691,6 +1691,9 @@ static int patch_update_file(struct add_p_state *s,\n> \t\t\t\tcolor_fprintf_ln(stdout, s->s.help_color,\n> \t\t\t\t\t\t \"%.*s\", (int)(eol - p), p);\n> \t\t\t}\n> +\t\t} else {\n> +\t\t\terr(s, _(\"Unknown command '%s' (use '?' for help)\"),\n> +\t\t\t s->answer.buf);\n> \t\t}\n> \t}\n\nLooking good.\n\n> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\n> index bc55255b0a..4c3901de17 100755\n> --- a/t/t3701-add-interactive.sh\n> +++ b/t/t3701-add-interactive.sh\n> @@ -7,6 +7,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n> . ./test-lib.sh\n> . \"$TEST_DIRECTORY\"/lib-terminal.sh\n> \n> +SP=\" \"\n\nGood to see what is already used in the existing tests reused.\n\n> diff_cmp () {\n> \tfor x\n> \tdo\n> @@ -61,6 +63,26 @@ test_expect_success 'setup (initial)' '\n> \techo more >>file &&\n> \techo lines >>file\n> '\n> +\n> +test_expect_success 'unknown command' '\n> +\ttest_when_finished \"git reset command && rm command\" &&\n\nGenerally, use of && in test_when_finished is a contradiction. Even\nif other things fail, you want \"command\" to be gone befor ethe next\nstep, and that is why you use test_when_finished to arrange it to be\ncleaned. Similarly, even if \"git reset command\" fails here, you\nwould want command to be cleaned.\n\nWhat is the expected state of the test repository before we start\nthis test? If it is that \"git status -uno\" would be silent (i.e.,\nno paths known to the index and the HEAD are modified in the index\nor in the working tree, but there may be random untracked cruft left\nin the working tree, like \"expect\" and stuff), then it makes more\nsense to make it easier to read that expectation by doing\n\n\ttest_when_finished \"git reset --hard; rm -f command\"\n\nhere. Instead of pretending that we care about making the minimum\ndamage by selectively resetting the index, we make it clear that we\nare giving the _next_ test a pristine state of the index and the\nworking tree as we inherited from the previous test.\n\n> +\techo W >command &&\n> +\tgit add -N command &&\n> +\tcat >expect <<-EOF &&\n> +\tdiff --git a/command b/command\n> +\tnew file mode 100644\n> +\tindex 0000000..a42d8ff\n> +\t--- /dev/null\n> +\t+++ b/command\n> +\t@@ -0,0 +1 @@\n> +\t+W\n> +\t(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help)\n> +\t(1/1) Stage addition [y,n,q,a,d,e,p,?]?$SP\n> +\tEOF\n> +\tgit add -p -- command actual 2>&1 &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_expect_success 'status works (initial)' '\n> \tgit add -i output &&\n> \tgrep \"+1/-0 *+2/-0 file\" output\n> @@ -231,7 +253,6 @@ test_expect_success 'setup file' '\n> '\n> \n> test_expect_success 'setup patch' '\n> -\tSP=\" \" &&\n> \tNULL=\"\" &&\n> \tcat >patch <<-EOF\n> \t@@ -1,4 +1,4 @@\n\nOtherwise, looking very good.\nWill queue.\n\nThanks.\n"},{"id":"493255","messageId":"db1d540f-30ae-4d4c-883b-088bcfe68140@gmail.com","threadId":"61323","inReplyTo":"6d421c67-9e10-4a7b-9782-38ba8e9da915@gmail.com","subject":"[PATCH v3] add-patch: response to unknown command","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-04-21T09:51:45Z","receivedAt":"2024-04-21T09:51:53Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"When the user gives an unknown command to the \"add -p\" prompt, the list\nof accepted commands with their explanation is given. This is the same\noutput they get when they say '?'.\n\nHowever, the unknown command may be due to a user input error rather\nthan the user not knowing the valid command.\n\nTo reduce the likelihood of user confusion and error repetition, instead\nof displaying the list of accepted commands, display a short error\nmessage with the unknown command received, as feedback to the user.\n\nInclude a reminder about the current command '?' in the new message, to\nguide the user if they want help.\n\nSigned-off-by: Rubén Justo \n---\n\nThanks.\n\nRange-diff against v2:\n1: c94c838fad ! 1: ddd8d380a7 add-patch: response to invalid command\n @@ Metadata\n Author: Rubén Justo \n \n ## Commit message ##\n - add-patch: response to invalid command\n + add-patch: response to unknown command\n \n - When the user enters an invalid command, we respond with a list of\n - accepted commands; the response we give to the command '?'.\n + When the user gives an unknown command to the \"add -p\" prompt, the list\n + of accepted commands with their explanation is given. This is the same\n + output they get when they say '?'.\n \n - However, the invalid command may be due to either a user input error or\n - a malfunctioning interface component, rather than the user not knowing\n - the valid command.\n + However, the unknown command may be due to a user input error rather\n + than the user not knowing the valid command.\n \n - Our response is unlikely to provide help in such situations.\n -\n - To reduce the likelihood of user confusion and error repetition, if an\n - unrecognized command is received, stop displaying the help text and\n - display a short error message with the invalid command received, as\n - feedback to the user.\n + To reduce the likelihood of user confusion and error repetition, instead\n + of displaying the list of accepted commands, display a short error\n + message with the unknown command received, as feedback to the user.\n \n Include a reminder about the current command '?' in the new message, to\n guide the user if they want help.\n @@ t/t3701-add-interactive.sh: export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n diff_cmp () {\n \tfor x\n \tdo\n -@@ t/t3701-add-interactive.sh: test_expect_success 'setup (initial)' '\n - \techo more >>file &&\n - \techo lines >>file\n +@@ t/t3701-add-interactive.sh: test_expect_success 'warn about add.interactive.useBuiltin' '\n + \tdone\n '\n -+\n + \n +test_expect_success 'unknown command' '\n -+\ttest_when_finished \"git reset command && rm command\" &&\n ++\ttest_when_finished \"git reset --hard; rm -f command\" &&\n +\techo W >command &&\n +\tgit add -N command &&\n +\tcat >expect <<-EOF &&\n @@ t/t3701-add-interactive.sh: test_expect_success 'setup (initial)' '\n +\ttest_cmp expect actual\n +'\n +\n - test_expect_success 'status works (initial)' '\n - \tgit add -i output &&\n - \tgrep \"+1/-0 *+2/-0 file\" output\n + test_expect_success 'setup (initial)' '\n + \techo content >file &&\n + \tgit add file &&\n @@ t/t3701-add-interactive.sh: test_expect_success 'setup file' '\n '\n \n\n add-patch.c | 5 ++++-\n t/t3701-add-interactive.sh | 22 +++++++++++++++++++++-\n 2 files changed, 25 insertions(+), 2 deletions(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex a06dd18985..7be142d448 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -1667,7 +1667,7 @@ static int patch_update_file(struct add_p_state *s,\n \t\t\t}\n \t\t} else if (s->answer.buf[0] == 'p') {\n \t\t\trendered_hunk_index = -1;\n-\t\t} else {\n+\t\t} else if (s->answer.buf[0] == '?') {\n \t\t\tconst char *p = _(help_patch_remainder), *eol = p;\n \n \t\t\tcolor_fprintf(stdout, s->s.help_color, \"%s\",\n@@ -1691,6 +1691,9 @@ static int patch_update_file(struct add_p_state *s,\n \t\t\t\tcolor_fprintf_ln(stdout, s->s.help_color,\n \t\t\t\t\t\t \"%.*s\", (int)(eol - p), p);\n \t\t\t}\n+\t\t} else {\n+\t\t\terr(s, _(\"Unknown command '%s' (use '?' for help)\"),\n+\t\t\t s->answer.buf);\n \t\t}\n \t}\n \ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex bc55255b0a..d974bddcf2 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -7,6 +7,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-terminal.sh\n \n+SP=\" \"\n+\n diff_cmp () {\n \tfor x\n \tdo\n@@ -55,6 +57,25 @@ test_expect_success 'warn about add.interactive.useBuiltin' '\n \tdone\n '\n \n+test_expect_success 'unknown command' '\n+\ttest_when_finished \"git reset --hard; rm -f command\" &&\n+\techo W >command &&\n+\tgit add -N command &&\n+\tcat >expect <<-EOF &&\n+\tdiff --git a/command b/command\n+\tnew file mode 100644\n+\tindex 0000000..a42d8ff\n+\t--- /dev/null\n+\t+++ b/command\n+\t@@ -0,0 +1 @@\n+\t+W\n+\t(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help)\n+\t(1/1) Stage addition [y,n,q,a,d,e,p,?]?$SP\n+\tEOF\n+\tgit add -p -- command actual 2>&1 &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'setup (initial)' '\n \techo content >file &&\n \tgit add file &&\n@@ -231,7 +252,6 @@ test_expect_success 'setup file' '\n '\n \n test_expect_success 'setup patch' '\n-\tSP=\" \" &&\n \tNULL=\"\" &&\n \tcat >patch <<-EOF\n \t@@ -1,4 +1,4 @@\n-- \n2.45.0.rc0.1.gddd8d380a7\n"},{"id":"493266","messageId":"b1c725f3-0e92-4723-a7ca-8d001d60b89e@gmail.com","threadId":"61323","inReplyTo":"db1d540f-30ae-4d4c-883b-088bcfe68140@gmail.com","subject":"Re: [PATCH v3] add-patch: response to unknown command","fromName":"","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-04-21T13:18:32Z","receivedAt":"2024-04-21T13:18:37Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Rubén\n\nThis is looking good, I've left a couple of comments on the test.\n\nOn 21/04/2024 10:51, Rubén Justo wrote:\n> When the user gives an unknown command to the \"add -p\" prompt, the list\n> of accepted commands with their explanation is given. This is the same\n> output they get when they say '?'.\n> \n> However, the unknown command may be due to a user input error rather\n> than the user not knowing the valid command.\n> \n> To reduce the likelihood of user confusion and error repetition, instead\n> of displaying the list of accepted commands, display a short error\n> message with the unknown command received, as feedback to the user.\n> \n> Include a reminder about the current command '?' in the new message, to\n> guide the user if they want help.\n> \n> Signed-off-by: Rubén Justo \n\n> +test_expect_success 'unknown command' '\n> +\ttest_when_finished \"git reset --hard; rm -f command\" &&\n> +\techo W >command &&\n\nI find separating out writing this file from where it is used makes it \nharder to see the input to \"git add -p\" using test_write_lines as you \ndid in v1 matches the existing tests and is perfectly fine.\n\n> +\tgit add -N command &&\n> +\tcat >expect <<-EOF &&\n> +\tdiff --git a/command b/command\n> +\tnew file mode 100644\n> +\tindex 0000000..a42d8ff\n> +\t--- /dev/null\n> +\t+++ b/command\n> +\t@@ -0,0 +1 @@\n> +\t+W\n> +\t(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help)\n> +\t(1/1) Stage addition [y,n,q,a,d,e,p,?]?$SP\n> +\tEOF\n> +\tgit add -p -- command actual 2>&1 &&\n> +\ttest_cmp expect actual\n\nIt is really good to see us testing the whole of the program output now \nbut have you tested this with GIT_TEST_DEFAULT_HASH=sha256? All the \nother tests use diff_cmp() to avoid test failures caused by the index \nline depending on the hash function. Alternatively you could run \"git \ndiff\" to create that part of the expected output.\n\nIt is a shame we're not testing '?' as we have changed the \nimplementation - before this commit '?' was just another unknown command \nas far as the implementation was concerned, now we treat it differently. \nIt would be nice to see some test coverage for the 'p' command you \nrecently added as well at some point.\n\n> test_expect_success 'setup patch' '\n> -\tSP=\" \" &&\n\nNice attention to detail, this is not needed now SP is defined at the \nbeginning of the test file.\n\nBest Wishes\n\nPhillip\n"},{"id":"493293","messageId":"c8f0f709-4a0d-469f-876a-63940705746c@gmail.com","threadId":"61323","inReplyTo":"b1c725f3-0e92-4723-a7ca-8d001d60b89e@gmail.com","subject":"Re: [PATCH v3] add-patch: response to unknown command","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-04-21T19:37:31Z","receivedAt":"2024-04-21T19:37:38Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Sun, Apr 21, 2024 at 02:18:32PM +0100, phillip.wood123@gmail.com wrote:\n\n> > +\tgit add -N command &&\n> > +\tcat >expect <<-EOF &&\n> > +\tdiff --git a/command b/command\n> > +\tnew file mode 100644\n> > +\tindex 0000000..a42d8ff\n> > +\t--- /dev/null\n> > +\t+++ b/command\n> > +\t@@ -0,0 +1 @@\n> > +\t+W\n> > +\t(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help)\n> > +\t(1/1) Stage addition [y,n,q,a,d,e,p,?]?$SP\n> > +\tEOF\n> > +\tgit add -p -- command actual 2>&1 &&\n> > +\ttest_cmp expect actual\n> \n> It is really good to see us testing the whole of the program output now but\n> have you tested this with GIT_TEST_DEFAULT_HASH=sha256? All the other tests\n> use diff_cmp() to avoid test failures caused by the index line depending on\n> the hash function. Alternatively you could run \"git diff\" to create that\n> part of the expected output.\n\nOops. Thank you for pointing this out. I'll reroll with a fix for it\nlater today.\n\n> It is a shame we're not testing '?' as we have changed the implementation -\n> before this commit '?' was just another unknown command as far as the\n> implementation was concerned, now we treat it differently.\n\nI think testing for '?' deserves its own series.\n\n> It would be nice\n> to see some test coverage for the 'p' command you recently added as well at\n> some point.\n\nYes. I'll send some tests for it soon.\n\nThanks.\n"},{"id":"493295","messageId":"b209a2b8-f98f-4f14-a687-9022d30968dd@gmail.com","threadId":"61323","inReplyTo":"db1d540f-30ae-4d4c-883b-088bcfe68140@gmail.com","subject":"[PATCH v4] add-patch: response to unknown command","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-04-21T21:52:33Z","receivedAt":"2024-04-21T21:52:42Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"When the user gives an unknown command to the \"add -p\" prompt, the list\nof accepted commands with their explanation is given. This is the same\noutput they get when they say '?'.\n\nHowever, the unknown command may be due to a user input error rather\nthan the user not knowing the valid command.\n\nTo reduce the likelihood of user confusion and error repetition, instead\nof displaying the list of accepted commands, display a short error\nmessage with the unknown command received, as feedback to the user.\n\nInclude a reminder about the current command '?' in the new message, to\nguide the user if they want help.\n\nSigned-off-by: Rubén Justo \n---\n\nThe test now pass with GIT_TEST_DEFAULT_HASH=sha256.\n\nThanks.\n\nRange-diff against v3:\n1: 0317594bce ! 1: b418b03f15 add-patch: response to unknown command\n @@ t/t3701-add-interactive.sh: test_expect_success 'warn about add.interactive.useB\n +\ttest_when_finished \"git reset --hard; rm -f command\" &&\n +\techo W >command &&\n +\tgit add -N command &&\n -+\tcat >expect <<-EOF &&\n -+\tdiff --git a/command b/command\n -+\tnew file mode 100644\n -+\tindex 0000000..a42d8ff\n -+\t--- /dev/null\n -+\t+++ b/command\n -+\t@@ -0,0 +1 @@\n -+\t+W\n ++\tgit diff command >expect &&\n ++\tcat >>expect <<-EOF &&\n +\t(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help)\n +\t(1/1) Stage addition [y,n,q,a,d,e,p,?]?$SP\n +\tEOF\n\n add-patch.c | 5 ++++-\n t/t3701-add-interactive.sh | 16 +++++++++++++++-\n 2 files changed, 19 insertions(+), 2 deletions(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex a06dd18985..7be142d448 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -1667,7 +1667,7 @@ static int patch_update_file(struct add_p_state *s,\n \t\t\t}\n \t\t} else if (s->answer.buf[0] == 'p') {\n \t\t\trendered_hunk_index = -1;\n-\t\t} else {\n+\t\t} else if (s->answer.buf[0] == '?') {\n \t\t\tconst char *p = _(help_patch_remainder), *eol = p;\n \n \t\t\tcolor_fprintf(stdout, s->s.help_color, \"%s\",\n@@ -1691,6 +1691,9 @@ static int patch_update_file(struct add_p_state *s,\n \t\t\t\tcolor_fprintf_ln(stdout, s->s.help_color,\n \t\t\t\t\t\t \"%.*s\", (int)(eol - p), p);\n \t\t\t}\n+\t\t} else {\n+\t\t\terr(s, _(\"Unknown command '%s' (use '?' for help)\"),\n+\t\t\t s->answer.buf);\n \t\t}\n \t}\n \ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex bc55255b0a..482d5c117e 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -7,6 +7,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-terminal.sh\n \n+SP=\" \"\n+\n diff_cmp () {\n \tfor x\n \tdo\n@@ -55,6 +57,19 @@ test_expect_success 'warn about add.interactive.useBuiltin' '\n \tdone\n '\n \n+test_expect_success 'unknown command' '\n+\ttest_when_finished \"git reset --hard; rm -f command\" &&\n+\techo W >command &&\n+\tgit add -N command &&\n+\tgit diff command >expect &&\n+\tcat >>expect <<-EOF &&\n+\t(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help)\n+\t(1/1) Stage addition [y,n,q,a,d,e,p,?]?$SP\n+\tEOF\n+\tgit add -p -- command actual 2>&1 &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'setup (initial)' '\n \techo content >file &&\n \tgit add file &&\n@@ -231,7 +246,6 @@ test_expect_success 'setup file' '\n '\n \n test_expect_success 'setup patch' '\n-\tSP=\" \" &&\n \tNULL=\"\" &&\n \tcat >patch <<-EOF\n \t@@ -1,4 +1,4 @@\n-- \n2.45.0.rc0.1.gb418b03f15\n"},{"id":"493309","messageId":"xmqqv849xups.fsf@gitster.g","threadId":"61323","inReplyTo":"b209a2b8-f98f-4f14-a687-9022d30968dd@gmail.com","subject":"Re: [PATCH v4] add-patch: response to unknown command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-22T15:50:23Z","receivedAt":"2024-04-22T15:50:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> 1: 0317594bce ! 1: b418b03f15 add-patch: response to unknown command\n> @@ t/t3701-add-interactive.sh: test_expect_success 'warn about add.interactive.useB\n> +\ttest_when_finished \"git reset --hard; rm -f command\" &&\n> +\techo W >command &&\n> +\tgit add -N command &&\n> -+\tcat >expect <<-EOF &&\n> -+\tdiff --git a/command b/command\n> -+\tnew file mode 100644\n> -+\tindex 0000000..a42d8ff\n> -+\t--- /dev/null\n> -+\t+++ b/command\n> -+\t@@ -0,0 +1 @@\n> -+\t+W\n> ++\tgit diff command >expect &&\n> ++\tcat >>expect <<-EOF &&\n> +\t(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help)\n> +\t(1/1) Stage addition [y,n,q,a,d,e,p,?]?$SP\n> +\tEOF\n\nInteresting.\n\nMy first reaction was \"how is this different from checking just the\nlast line of the actual output? The early part of expect and actual\nboth come from an internal invocation of 'git diff', and they must\nmatch by definition\".\n\nBut that may really be the point of this test.\n\nThat is, we may later decide to tweak the way \"git diff\" hunks are\npresented, and we expect that the way \"git add -p\" presents the\nhunks would change together with it automatically.\n"},{"id":"493396","messageId":"41d23496-0bd3-47c7-a470-39334588f520@gmail.com","threadId":"61323","inReplyTo":"xmqqv849xups.fsf@gitster.g","subject":"Re: [PATCH v4] add-patch: response to unknown command","fromName":"","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-04-24T10:15:57Z","receivedAt":"2024-04-24T10:16:02Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Junio\n\nOn 22/04/2024 16:50, Junio C Hamano wrote:\n> Rubén Justo writes:\n> \n>> 1: 0317594bce ! 1: b418b03f15 add-patch: response to unknown command\n>> @@ t/t3701-add-interactive.sh: test_expect_success 'warn about add.interactive.useB\n>> +\ttest_when_finished \"git reset --hard; rm -f command\" &&\n>> +\techo W >command &&\n>> +\tgit add -N command &&\n>> -+\tcat >expect <<-EOF &&\n>> -+\tdiff --git a/command b/command\n>> -+\tnew file mode 100644\n>> -+\tindex 0000000..a42d8ff\n>> -+\t--- /dev/null\n>> -+\t+++ b/command\n>> -+\t@@ -0,0 +1 @@\n>> -+\t+W\n>> ++\tgit diff command >expect &&\n>> ++\tcat >>expect <<-EOF &&\n>> +\t(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help)\n>> +\t(1/1) Stage addition [y,n,q,a,d,e,p,?]?$SP\n>> +\tEOF\n> \n> Interesting.\n> \n> My first reaction was \"how is this different from checking just the\n> last line of the actual output? The early part of expect and actual\n> both come from an internal invocation of 'git diff', and they must\n> match by definition\".\n> \n> But that may really be the point of this test.\n\nYes - we want to make sure that we are not printing the help and the \nonly way to do that is to check the whole output\n\nBest Wishes\n\nPhillip\n\n> That is, we may later decide to tweak the way \"git diff\" hunks are\n> presented, and we expect that the way \"git add -p\" presents the\n> hunks would change together with it automatically.\n"},{"id":"493408","messageId":"xmqqfrva22py.fsf@gitster.g","threadId":"61323","inReplyTo":"41d23496-0bd3-47c7-a470-39334588f520@gmail.com","subject":"Re: [PATCH v4] add-patch: response to unknown command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-24T15:35:21Z","receivedAt":"2024-04-24T15:35:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"phillip.wood123@gmail.com writes:\n\n> On 22/04/2024 16:50, Junio C Hamano wrote:\n>> Rubén Justo writes:\n>> \n>>> 1: 0317594bce ! 1: b418b03f15 add-patch: response to unknown command\n>>> @@ t/t3701-add-interactive.sh: test_expect_success 'warn about add.interactive.useB\n>>> +\ttest_when_finished \"git reset --hard; rm -f command\" &&\n>>> +\techo W >command &&\n>>> +\tgit add -N command &&\n>>> -+\tcat >expect <<-EOF &&\n>>> -+\tdiff --git a/command b/command\n>>> -+\tnew file mode 100644\n>>> -+\tindex 0000000..a42d8ff\n>>> -+\t--- /dev/null\n>>> -+\t+++ b/command\n>>> -+\t@@ -0,0 +1 @@\n>>> -+\t+W\n>>> ++\tgit diff command >expect &&\n>>> ++\tcat >>expect <<-EOF &&\n>>> +\t(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help)\n>>> +\t(1/1) Stage addition [y,n,q,a,d,e,p,?]?$SP\n>>> +\tEOF\n>> Interesting.\n>> My first reaction was \"how is this different from checking just the\n>> last line of the actual output? The early part of expect and actual\n>> both come from an internal invocation of 'git diff', and they must\n>> match by definition\".\n>> But that may really be the point of this test.\n>\n> Yes - we want to make sure that we are not printing the help and the\n> only way to do that is to check the whole output\n\nI was not questioning that part of the patch. \"My first reaction\"\nwas solely about use of \"git diff\" to replace the golden copy of\nexpected result in the test itself, only to allow for use of\ndifferent hash functions. As you (or somebody else?) mentioned in\nan earlier review, diff_cmp is there for exactly that purpose.\n\n>> That is, we may later decide to tweak the way \"git diff\" hunks are\n>> presented, and we expect that the way \"git add -p\" presents the\n>> hunks would change together with it automatically.\n\nThis argument cuts both ways, by the way.\n\nInsisting that the output match the explicit expectation (not what\n\"git diff\" of the day produces) has a few advantages. It makes the\ntest more explicit and easy to see what output we are expecting, and\nmore importantly, it forces us to update the test when we decide to\ntweak the output from the command being tested (i.e. hunk selection\nUI) and/or the output from \"git diff\" command.\n"},{"id":"493452","messageId":"20240425014432.GA1768914@coredump.intra.peff.net","threadId":"61323","inReplyTo":"b209a2b8-f98f-4f14-a687-9022d30968dd@gmail.com","subject":"Re: [PATCH v4] add-patch: response to unknown command","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-25T01:44:32Z","receivedAt":"2024-04-25T01:44:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Apr 21, 2024 at 11:52:33PM +0200, Rubén Justo wrote:\n\n> +test_expect_success 'unknown command' '\n> +\ttest_when_finished \"git reset --hard; rm -f command\" &&\n> +\techo W >command &&\n> +\tgit add -N command &&\n> +\tgit diff command >expect &&\n> +\tcat >>expect <<-EOF &&\n> +\t(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help)\n> +\t(1/1) Stage addition [y,n,q,a,d,e,p,?]?$SP\n> +\tEOF\n> +\tgit add -p -- command actual 2>&1 &&\n> +\ttest_cmp expect actual\n> +'\n\nI got a test failure on Windows CI from this. The test_cmp output looks\nlike this:\n\n -(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command 'W' (use '?' for help)\n -(1/1) Stage addition [y,n,q,a,d,e,p,?]?\n +(1/1) Stage addition [y,n,q,a,d,e,p,?]? (1/1) Stage addition [y,n,q,a,d,e,p,?]?\n +Unknown command 'W' (use '?' for help)\n\nwhich makes me suspect a race. Perhaps because the prompt is going to\nstdout, but the \"Unknown command\" message goes to stderr? Maybe we\nshould keep stdout and stderr separate and check them independently.\n\n-Peff\n"},{"id":"493453","messageId":"CAPig+cST4_hUwWg+P_drqT-mv8+eLAzBmuMFXm=Dui14kfYZzA@mail.gmail.com","threadId":"61323","inReplyTo":"20240425014432.GA1768914@coredump.intra.peff.net","subject":"Re: [PATCH v4] add-patch: response to unknown command","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-04-25T02:15:25Z","receivedAt":"2024-04-25T02:15:38Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Apr 24, 2024 at 9:44 PM Jeff King wrote:\n> On Sun, Apr 21, 2024 at 11:52:33PM +0200, Rubén Justo wrote:\n> > +test_expect_success 'unknown command' '\n> > + test_when_finished \"git reset --hard; rm -f command\" &&\n> > + echo W >command &&\n> > + git add -N command &&\n> > + git diff command >expect &&\n> > + cat >>expect <<-EOF &&\n> > + (1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help)\n> > + (1/1) Stage addition [y,n,q,a,d,e,p,?]?$SP\n> > + EOF\n> > + git add -p -- command actual 2>&1 &&\n> > + test_cmp expect actual\n> > +'\n>\n> I got a test failure on Windows CI from this. The test_cmp output looks\n> like this:\n>\n> -(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command 'W' (use '?' for help)\n> -(1/1) Stage addition [y,n,q,a,d,e,p,?]?\n> +(1/1) Stage addition [y,n,q,a,d,e,p,?]? (1/1) Stage addition [y,n,q,a,d,e,p,?]?\n> +Unknown command 'W' (use '?' for help)\n>\n> which makes me suspect a race. Perhaps because the prompt is going to\n> stdout, but the \"Unknown command\" message goes to stderr? Maybe we\n> should keep stdout and stderr separate and check them independently.\n\nThat's very reminiscent of [1]. Although, unlike [1], the output\npresented to the user in this case is (I suppose) less likely to be\nmessed up; only the combined captured output is probably affected. So,\ncapturing stdout and stderr separately would indeed be a good idea.\n\n[1]: https://lore.kernel.org/git/CAPig+cTGq-10ZTBts2LXRVdPMf2vNMX8HTuhg_+ZHSiLX-brOQ@mail.gmail.com/\n"},{"id":"493454","messageId":"19ff10e9-12be-4a50-9163-1fe71414fd46@gmail.com","threadId":"61323","inReplyTo":"20240425014432.GA1768914@coredump.intra.peff.net","subject":"Re: [PATCH v4] add-patch: response to unknown command","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-04-25T03:04:45Z","receivedAt":"2024-04-25T03:04:48Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Wed, Apr 24, 2024 at 09:44:32PM -0400, Jeff King wrote:\n> On Sun, Apr 21, 2024 at 11:52:33PM +0200, Rubén Justo wrote:\n> \n> > +test_expect_success 'unknown command' '\n> > +\ttest_when_finished \"git reset --hard; rm -f command\" &&\n> > +\techo W >command &&\n> > +\tgit add -N command &&\n> > +\tgit diff command >expect &&\n> > +\tcat >>expect <<-EOF &&\n> > +\t(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help)\n> > +\t(1/1) Stage addition [y,n,q,a,d,e,p,?]?$SP\n> > +\tEOF\n> > +\tgit add -p -- command actual 2>&1 &&\n> > +\ttest_cmp expect actual\n> > +'\n> \n> I got a test failure on Windows CI from this.\n\nThank you for testing thoroughly.\n\n> The test_cmp output looks\n> like this:\n> \n> -(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command 'W' (use '?' for help)\n> -(1/1) Stage addition [y,n,q,a,d,e,p,?]?\n> +(1/1) Stage addition [y,n,q,a,d,e,p,?]? (1/1) Stage addition [y,n,q,a,d,e,p,?]?\n> +Unknown command 'W' (use '?' for help)\n> \n> which makes me suspect a race. Perhaps because the prompt is going to\n> stdout, but the \"Unknown command\" message goes to stderr?\n\nI have to read the thread pointed by Eric, but my knee-jerk reaction has\nbeen to think in something like:\n\ndiff --git a/add-patch.c b/add-patch.c\nindex 447e8166d2..0090543f89 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -292,6 +292,7 @@ static void err(struct add_p_state *s, const char *fmt, ...)\n {\n va_list args;\n\n va_start(args, fmt);\n+ fflush(stdout);\n fputs(s->s.error_color, stderr);\n vfprintf(stderr, fmt, args);\n"},{"id":"493464","messageId":"02440375-2514-48a3-8f94-11d853f00caa@gmail.com","threadId":"61323","inReplyTo":"20240425014432.GA1768914@coredump.intra.peff.net","subject":"Re: [PATCH v4] add-patch: response to unknown command","fromName":"","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-04-25T08:53:20Z","receivedAt":"2024-04-25T08:53:25Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Peff\n\nOn 25/04/2024 02:44, Jeff King wrote:\n> On Sun, Apr 21, 2024 at 11:52:33PM +0200, Rubén Justo wrote:\n> \n>> +test_expect_success 'unknown command' '\n>> +\ttest_when_finished \"git reset --hard; rm -f command\" &&\n>> +\techo W >command &&\n>> +\tgit add -N command &&\n>> +\tgit diff command >expect &&\n>> +\tcat >>expect <<-EOF &&\n>> +\t(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help)\n>> +\t(1/1) Stage addition [y,n,q,a,d,e,p,?]?$SP\n>> +\tEOF\n>> +\tgit add -p -- command actual 2>&1 &&\n>> +\ttest_cmp expect actual\n>> +'\n> \n> I got a test failure on Windows CI from this. The test_cmp output looks\n> like this:\n> \n> -(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command 'W' (use '?' for help)\n> -(1/1) Stage addition [y,n,q,a,d,e,p,?]?\n> +(1/1) Stage addition [y,n,q,a,d,e,p,?]? (1/1) Stage addition [y,n,q,a,d,e,p,?]?\n> +Unknown command 'W' (use '?' for help)\n> \n> which makes me suspect a race. Perhaps because the prompt is going to\n> stdout, but the \"Unknown command\" message goes to stderr? Maybe we\n> should keep stdout and stderr separate and check them independently.\n\nCan we change err() to print to stdout? I can't really see the advantage \nof separating \"normal output\" and \"error messages\" for an interactive \nprogram like this.\n\nBest Wishes\n\nPhillip\n\n"},{"id":"493480","messageId":"xmqqedatfay3.fsf@gitster.g","threadId":"61323","inReplyTo":"CAPig+cST4_hUwWg+P_drqT-mv8+eLAzBmuMFXm=Dui14kfYZzA@mail.gmail.com","subject":"Re: [PATCH v4] add-patch: response to unknown command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-25T20:23:48Z","receivedAt":"2024-04-25T20:23:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> That's very reminiscent of [1]. Although, unlike [1], the output\n> presented to the user in this case is (I suppose) less likely to be\n> messed up; only the combined captured output is probably affected. So,\n> capturing stdout and stderr separately would indeed be a good idea.\n\nHmph, something along this line?\n\nIt loses to capture how the output should be intermixed, which is\nessential to validate what the end-user should see. As we can see\nin the attached patch, we cannot express that \"Unknown ...\" should\ncome in between two \"Stage addition?\" questions, which is a downside.\n\nBetween adding fflush() before err() writes, and updating err() to\nwrite to the standard output stream, I am in favor of the latter for\nits simplicity (of the mental model of the resulting code, not of\nthe patch that is required to do so).\n\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex ed7e414649..a8dfebd8d7 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -64,11 +64,12 @@ test_expect_success 'unknown command' '\n \tgit add -N command &&\n \tgit diff command >expect &&\n \tcat >>expect <<-EOF &&\n-\t(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help)\n-\t(1/1) Stage addition [y,n,q,a,d,e,p,?]?$SP\n+\t(1/1) Stage addition [y,n,q,a,d,e,p,?]? (1/1) Stage addition [y,n,q,a,d,e,p,?]?$SP\n \tEOF\n-\tgit add -p -- command actual 2>&1 &&\n-\ttest_cmp expect actual\n+\techo \"Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help)\" >expect.error &&\n+\tgit add -p -- command actual 2>actual.error &&\n+\ttest_cmp expect actual &&\n+\ttest_cmp expect.error actual.error\n '\n \n test_expect_success 'setup (initial)' '\n"},{"id":"493481","messageId":"CAPig+cRerw2SnkEK4-_wiPopGii+Eq92UiBmqib_oBgArty-Gg@mail.gmail.com","threadId":"61323","inReplyTo":"xmqqedatfay3.fsf@gitster.g","subject":"Re: [PATCH v4] add-patch: response to unknown command","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-04-25T21:00:27Z","receivedAt":"2024-04-25T21:00:39Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Apr 25, 2024 at 4:23 PM Junio C Hamano wrote:\n> Eric Sunshine writes:\n> > That's very reminiscent of [1]. Although, unlike [1], the output\n> > presented to the user in this case is (I suppose) less likely to be\n> > messed up; only the combined captured output is probably affected. So,\n> > capturing stdout and stderr separately would indeed be a good idea.\n>\n> Between adding fflush() before err() writes, and updating err() to\n> write to the standard output stream, I am in favor of the latter for\n> its simplicity (of the mental model of the resulting code, not of\n> the patch that is required to do so).\n\nWriting to a common stream (stdout, in this case) for this sort of\ninteractive session is indeed probably the way to go, as Phillip\nsuggested.\n\nThat was also the adopted solution to the cited similar example[1];\ngit-worktree was changed to send all its chatty output to stderr[2],\nwhich was appropriate for that (non-interactive) case.\n\n[1]: https://lore.kernel.org/git/CAPig+cTGq-10ZTBts2LXRVdPMf2vNMX8HTuhg_+ZHSiLX-brOQ@mail.gmail.com/\n[2]: https://lore.kernel.org/git/20211203034420.47447-2-sunshine@sunshineco.com/\n"},{"id":"493482","messageId":"xmqqv845dug2.fsf@gitster.g","threadId":"61323","inReplyTo":"xmqqedatfay3.fsf@gitster.g","subject":"Re: [PATCH v4] add-patch: response to unknown command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-25T21:05:33Z","receivedAt":"2024-04-25T21:05:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Eric Sunshine writes:\n>\n>> That's very reminiscent of [1]. Although, unlike [1], the output\n>> presented to the user in this case is (I suppose) less likely to be\n>> messed up; only the combined captured output is probably affected. So,\n>> capturing stdout and stderr separately would indeed be a good idea.\n>\n> Hmph, something along this line?\n>\n> It loses to capture how the output should be intermixed, which is\n> essential to validate what the end-user should see. As we can see\n> in the attached patch, we cannot express that \"Unknown ...\" should\n> come in between two \"Stage addition?\" questions, which is a downside.\n>\n> Between adding fflush() before err() writes, and updating err() to\n> write to the standard output stream, I am in favor of the latter for\n> its simplicity (of the mental model of the resulting code, not of\n> the patch that is required to do so).\n\nThe latter, which I claimed to prefer, would look like this.\n\nThe idea is to perform the interactive session over the standard\noutput (and the standard input). For that, we teach err() to use\nthe standard output and have a few fprintf() to also call err().\n\nA few tests expect certain messages to appear on the standard error\nstream, which needed adjusting.\n\nI know the previous one \"fixes\" the CI job at Windows, but I haven't\ntried this yet.\n\ndiff --git c/add-patch.c w/add-patch.c\nindex 7be142d448..c28ad380ed 100644\n--- c/add-patch.c\n+++ w/add-patch.c\n@@ -293,10 +293,10 @@ static void err(struct add_p_state *s, const char *fmt, ...)\n \tva_list args;\n \n \tva_start(args, fmt);\n-\tfputs(s->s.error_color, stderr);\n-\tvfprintf(stderr, fmt, args);\n-\tfputs(s->s.reset_color, stderr);\n-\tfputc('\\n', stderr);\n+\tfputs(s->s.error_color, stdout);\n+\tvfprintf(stdout, fmt, args);\n+\tfputs(s->s.reset_color, stdout);\n+\tfputc('\\n', stdout);\n \tva_end(args);\n }\n \n@@ -1326,7 +1326,7 @@ static int apply_for_checkout(struct add_p_state *s, struct strbuf *diff,\n \t\terr(s, _(\"Nothing was applied.\\n\"));\n \t} else\n \t\t/* As a last resort, show the diff to the user */\n-\t\tfwrite(diff->buf, diff->len, 1, stderr);\n+\t\tfwrite(diff->buf, diff->len, 1, stdout);\n \n \treturn 0;\n }\n@@ -1780,9 +1780,9 @@ int run_add_p(struct repository *r, enum add_p_mode mode,\n \t\t\tbreak;\n \n \tif (s.file_diff_nr == 0)\n-\t\tfprintf(stderr, _(\"No changes.\\n\"));\n+\t\terr(&s, _(\"No changes.\"));\n \telse if (binary_count == s.file_diff_nr)\n-\t\tfprintf(stderr, _(\"Only binary files changed.\\n\"));\n+\t\terr(&s, _(\"Only binary files changed.\"));\n \n \tadd_p_state_clear(&s);\n \treturn 0;\ndiff --git c/t/t3701-add-interactive.sh w/t/t3701-add-interactive.sh\nindex 482d5c117e..a315ec99a3 100755\n--- c/t/t3701-add-interactive.sh\n+++ w/t/t3701-add-interactive.sh\n@@ -43,17 +43,17 @@ force_color () {\n }\n \n test_expect_success 'warn about add.interactive.useBuiltin' '\n-\tcat >expect <<-\\EOF &&\n+\tcat >expect.error <<-\\EOF &&\n \twarning: the add.interactive.useBuiltin setting has been removed!\n \tSee its entry in '\\''git help config'\\'' for details.\n-\tNo changes.\n \tEOF\n \n \tfor v in = =true =false\n \tdo\n-\t\tgit -c \"add.interactive.useBuiltin$v\" add -p >out 2>actual &&\n-\t\ttest_must_be_empty out &&\n-\t\ttest_cmp expect actual || return 1\n+\t\tgit -c \"add.interactive.useBuiltin$v\" add -p >actual 2>error &&\n+\t\techo \"No changes.\" >expect &&\n+\t\ttest_cmp expect actual &&\n+\t\ttest_cmp expect.error error || return 1\n \tdone\n '\n \n@@ -348,13 +348,13 @@ test_expect_success 'different prompts for mode change/deleted' '\n \n test_expect_success 'correct message when there is nothing to do' '\n \tgit reset --hard &&\n-\tgit add -p 2>err &&\n-\ttest_grep \"No changes\" err &&\n+\tgit add -p >out &&\n+\ttest_grep \"No changes\" out &&\n \tprintf \"\\\\0123\" >binary &&\n \tgit add binary &&\n \tprintf \"\\\\0abc\" >binary &&\n-\tgit add -p 2>err &&\n-\ttest_grep \"Only binary files changed\" err\n+\tgit add -p >out &&\n+\ttest_grep \"Only binary files changed\" out\n '\n \n test_expect_success 'setup again' '\n"},{"id":"493485","messageId":"xmqqr0etdu2c.fsf@gitster.g","threadId":"61323","inReplyTo":"CAPig+cRerw2SnkEK4-_wiPopGii+Eq92UiBmqib_oBgArty-Gg@mail.gmail.com","subject":"Re: [PATCH v4] add-patch: response to unknown command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-25T21:13:47Z","receivedAt":"2024-04-25T21:13:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> That was also the adopted solution to the cited similar example[1];\n> git-worktree was changed to send all its chatty output to stderr[2],\n> which was appropriate for that (non-interactive) case.\n>\n> [1]: https://lore.kernel.org/git/CAPig+cTGq-10ZTBts2LXRVdPMf2vNMX8HTuhg_+ZHSiLX-brOQ@mail.gmail.com/\n> [2]: https://lore.kernel.org/git/20211203034420.47447-2-sunshine@sunshineco.com/\n\nHmph, what I just sent out tries to send everything to the standard\noutput stream. For an interactive sesion, whether the interaction\nis done over stdout or stderr does not really make much difference,\nbut I have a slight preference to use the stderr (because I view\nprompts etc. in the same \"chatty\" class as the progress bar), but I\ndo not care enough to go back and redo the patch ;-)\n\n"},{"id":"493491","messageId":"5854e33c-b84e-47a3-94c6-c5fe8c9a4c98@gmail.com","threadId":"61323","inReplyTo":"xmqqv845dug2.fsf@gitster.g","subject":"Re: [PATCH v4] add-patch: response to unknown command","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-04-25T22:09:20Z","receivedAt":"2024-04-25T22:10:18Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Thu, Apr 25, 2024 at 02:05:33PM -0700, Junio C Hamano wrote:\n\nI am assuming that this change will precede my series.\n\nI leave some nits below.\n\n> diff --git c/add-patch.c w/add-patch.c\n> index 7be142d448..c28ad380ed 100644\n> --- c/add-patch.c\n> +++ w/add-patch.c\n> @@ -293,10 +293,10 @@ static void err(struct add_p_state *s, const char *fmt, ...)\n> \tva_list args;\n> \n> \tva_start(args, fmt);\n> -\tfputs(s->s.error_color, stderr);\n> -\tvfprintf(stderr, fmt, args);\n> -\tfputs(s->s.reset_color, stderr);\n> -\tfputc('\\n', stderr);\n> +\tfputs(s->s.error_color, stdout);\n> +\tvfprintf(stdout, fmt, args);\n> +\tfputs(s->s.reset_color, stdout);\n> +\tfputc('\\n', stdout);\n\nIf we're going to use stdout, perhaps we can be less explicit?\n\ndiff --git a/add-patch.c b/add-patch.c\nindex 447e8166d2..a204224dae 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -293,10 +293,10 @@ static void err(struct add_p_state *s, const char *fmt, ...)\n va_list args;\n\n va_start(args, fmt);\n- fputs(s->s.error_color, stderr);\n- vfprintf(stderr, fmt, args);\n- fputs(s->s.reset_color, stderr);\n- fputc('\\n', stderr);\n+ puts(s->s.error_color);\n+ vprintf(fmt, args);\n+ puts(s->s.reset_color);\n+ putchar('\\n');\n va_end(args);\n }\n\n> @@ -1780,9 +1780,9 @@ int run_add_p(struct repository *r, enum add_p_mode mode,\n> \t\t\tbreak;\n> \n> \tif (s.file_diff_nr == 0)\n> -\t\tfprintf(stderr, _(\"No changes.\\n\"));\n> +\t\terr(&s, _(\"No changes.\"));\n\nNice.\n\n> \telse if (binary_count == s.file_diff_nr)\n> -\t\tfprintf(stderr, _(\"Only binary files changed.\\n\"));\n> +\t\terr(&s, _(\"Only binary files changed.\"));\n\nNice.\n\n> diff --git c/t/t3701-add-interactive.sh w/t/t3701-add-interactive.sh\n> index 482d5c117e..a315ec99a3 100755\n> --- c/t/t3701-add-interactive.sh\n> +++ w/t/t3701-add-interactive.sh\n> @@ -43,17 +43,17 @@ force_color () {\n> }\n> \n> test_expect_success 'warn about add.interactive.useBuiltin' '\n> -\tcat >expect <<-\\EOF &&\n> +\tcat >expect.error <<-\\EOF &&\n\nThis expect.error is what we are going to test with the output on\nstderr ... \n\n> \twarning: the add.interactive.useBuiltin setting has been removed!\n> \tSee its entry in '\\''git help config'\\'' for details.\n> -\tNo changes.\n\nhowever, this line no longer goes to stderr. OK.\n\n> \tEOF\n> \n> \tfor v in = =true =false\n> \tdo\n> -\t\tgit -c \"add.interactive.useBuiltin$v\" add -p >out 2>actual &&\n> -\t\ttest_must_be_empty out &&\n> -\t\ttest_cmp expect actual || return 1\n> +\t\tgit -c \"add.interactive.useBuiltin$v\" add -p >actual 2>error &&\n> +\t\techo \"No changes.\" >expect &&\n\nWhy not prepare this file above, out of the loop?\n\n> +\t\ttest_cmp expect actual &&\n> +\t\ttest_cmp expect.error error || return 1\n> \tdone\n> '\n> \n> @@ -348,13 +348,13 @@ test_expect_success 'different prompts for mode change/deleted' '\n> \n> test_expect_success 'correct message when there is nothing to do' '\n> \tgit reset --hard &&\n> -\tgit add -p 2>err &&\n> -\ttest_grep \"No changes\" err &&\n> +\tgit add -p >out &&\n> +\ttest_grep \"No changes\" out &&\n> \tprintf \"\\\\0123\" >binary &&\n> \tgit add binary &&\n> \tprintf \"\\\\0abc\" >binary &&\n> -\tgit add -p 2>err &&\n> -\ttest_grep \"Only binary files changed\" err\n> +\tgit add -p >out &&\n> +\ttest_grep \"Only binary files changed\" out\n> '\n> \n> test_expect_success 'setup again' '\n"},{"id":"493492","messageId":"xmqq8r11dr64.fsf@gitster.g","threadId":"61323","inReplyTo":"5854e33c-b84e-47a3-94c6-c5fe8c9a4c98@gmail.com","subject":"Re: [PATCH v4] add-patch: response to unknown command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-25T22:16:19Z","receivedAt":"2024-04-25T22:16:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> On Thu, Apr 25, 2024 at 02:05:33PM -0700, Junio C Hamano wrote:\n>\n> I am assuming that this change will precede my series.\n\nNo, this was merely \"if we were to update the series I queued to my\ntree, the squashable fix may look like this\", which you can use to\nupdate _your_ series if you want to.\n\n"},{"id":"493493","messageId":"305296f1-975b-41b9-968c-3984d8056196@gmail.com","threadId":"61323","inReplyTo":"xmqq8r11dr64.fsf@gitster.g","subject":"Re: [PATCH v4] add-patch: response to unknown command","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-04-25T23:46:38Z","receivedAt":"2024-04-25T23:46:49Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"> Rubén Justo writes:\n> \n> > On Thu, Apr 25, 2024 at 02:05:33PM -0700, Junio C Hamano wrote:\n> >\n> > I am assuming that this change will precede my series.\n> \n> No, this was merely \"if we were to update the series I queued to my\n> tree, the squashable fix may look like this\", which you can use to\n> update _your_ series if you want to.\n> \n\nI was not sure what was the expectation. In fact, I'm still not quite\nsure.\n\nI am not sure about the change either.\n\nThe current options are:\n\n\ta.- make the test check for stderr and stdout, separatedly\n\n\tb.- fflush(stdout) in err\n\n\tc.- make err print to stdout\n\nI suspect that similar tests for other commands would produce similar\nerrors, so (a) seems like an easy fix but feels like kicking the can\nforward.\n\nI'm not sure of the implications of (c). Perhaps moving current\nmessages to stdout breaks some workflow out there? The other thread\nabout disabling all hints has made me think.\n\nThe (b) option seems to me the less disturbing change, but it has not\nattracted attention.\n\nThere is even a (d) that is to go back to test just the new error\nmessage, not the whole output.\n\nI will give it some thought.\n"},{"id":"493506","messageId":"xmqqwmokbs3p.fsf@gitster.g","threadId":"61323","inReplyTo":"305296f1-975b-41b9-968c-3984d8056196@gmail.com","subject":"Re: [PATCH v4] add-patch: response to unknown command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-26T05:39:06Z","receivedAt":"2024-04-26T05:39:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> The current options are:\n>\n> \ta.- make the test check for stderr and stdout, separatedly\n\nThere is a downside that we do not check what the users would see,\nas I mentioned in an earlier message.\n\n> \tb.- fflush(stdout) in err\n>\n> \tc.- make err print to stdout\n\nYup. The last one is what I showed in the thread.\n\n> I suspect that similar tests for other commands would produce similar\n> errors, so (a) seems like an easy fix but feels like kicking the can\n> forward.\n>\n> I'm not sure of the implications of (c). Perhaps moving current\n> messages to stdout breaks some workflow out there? The other thread\n> about disabling all hints has made me think.\n\nThat depends on \"other\" commands. As Phillip said, \"git add -p\" and\nthe like that interacts with the end user via terminal, using a\nsingle stream, whether it is the standard error stream or the\nstandard output stream, should not affect any end-user experience.\nIt is very unlikely people capture only one stream and feed it to a\nprogram, and with i18n, these messages are not even designed for\nmachine consumption in the first place.\n\n> The (b) option seems to me the less disturbing change\n\nSticking close to the status quo would certainly _feel_ the safest\nat least in the shorter term.\n\nBut given that there is not really a reasonable justification why\nsome output goes to the standard output stream while others go to\nthe standard error stream in this program (note that I am\nspecifically talking about the terminal interactive session with\n\"add -p\"), the approach will force us to worry about similar gotchas\nand we need to decide which stream the message needs to go every\ntime we add a new one.\n\n> but it has not attracted attention.\n\nI think the same reasoning from the old thread that made us avoid\nthe \"flush() and keep writing to two streams\" in the worktree code\nwould apply here, too.\n"},{"id":"493525","messageId":"xmqqcyqcay4n.fsf@gitster.g","threadId":"61323","inReplyTo":"305296f1-975b-41b9-968c-3984d8056196@gmail.com","subject":"Re: [PATCH v4] add-patch: response to unknown command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-26T16:26:32Z","receivedAt":"2024-04-26T16:26:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> I will give it some thought.\n\nIn the meantime, I'll revert the merge of the topic into 'next',\nsquash in the \"send everything to the standard output stream\" fix,\nand queue the \"tentative\" result to 'seen', to make the CI happy.\n\nI am OK enough with the \"tentative\" one, but if you and others feel\ndifferently, I am perfectly happy to see it replaced with [PATCH v5].\n\nThanks.\n"},{"id":"493528","messageId":"20240426202133.GA13703@coredump.intra.peff.net","threadId":"61323","inReplyTo":"CAPig+cST4_hUwWg+P_drqT-mv8+eLAzBmuMFXm=Dui14kfYZzA@mail.gmail.com","subject":"Re: [PATCH v4] add-patch: response to unknown command","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-26T20:21:33Z","receivedAt":"2024-04-26T20:21:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 24, 2024 at 10:15:25PM -0400, Eric Sunshine wrote:\n\n> > I got a test failure on Windows CI from this. The test_cmp output looks\n> > like this:\n> >\n> > -(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command 'W' (use '?' for help)\n> > -(1/1) Stage addition [y,n,q,a,d,e,p,?]?\n> > +(1/1) Stage addition [y,n,q,a,d,e,p,?]? (1/1) Stage addition [y,n,q,a,d,e,p,?]?\n> > +Unknown command 'W' (use '?' for help)\n> >\n> > which makes me suspect a race. Perhaps because the prompt is going to\n> > stdout, but the \"Unknown command\" message goes to stderr? Maybe we\n> > should keep stdout and stderr separate and check them independently.\n> \n> That's very reminiscent of [1]. Although, unlike [1], the output\n> presented to the user in this case is (I suppose) less likely to be\n> messed up; only the combined captured output is probably affected. So,\n> capturing stdout and stderr separately would indeed be a good idea.\n\nAh, yeah, that is almost certainly the same issue. I could not reproduce\nit on my local system with --stress, so I think it may be unique to\nWindows. What it looks like is that stderr is buffered when output to a\nfile (whereas on most systems it would still be line buffered at most).\nBut I didn't actually run it through a debugger to see.\n\nSo I _suspect_ that yet another possible solution here is to do an\nexplicit:\n\n setvbuf(stderr, NULL, _IOLBF, BUFSIZ);\n\nin common-main. Though grepping for setvbuf() in our code base, it looks\nlike compat/winansi.c already does:\n\n if (fd == 2)\n setvbuf(stderr, NULL, _IONBF, BUFSIZ);\n\nwhich would imply that output should happen even more immediately than\nline-buffering. So maybe there is some other layer at work. There are\nprobably dragons, and definitely pursuing that line would involve some\nexperimentation.\n\nThe \"if you want them in order, make sure they are on the same stream\"\napproach suggested elsewhere is by comparison much simpler. :)\n\n-Peff\n"},{"id":"493529","messageId":"20240426202333.GB13703@coredump.intra.peff.net","threadId":"61323","inReplyTo":"19ff10e9-12be-4a50-9163-1fe71414fd46@gmail.com","subject":"Re: [PATCH v4] add-patch: response to unknown command","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-26T20:23:33Z","receivedAt":"2024-04-26T20:23:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 25, 2024 at 05:04:45AM +0200, Rubén Justo wrote:\n\n> > The test_cmp output looks\n> > like this:\n> > \n> > -(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command 'W' (use '?' for help)\n> > -(1/1) Stage addition [y,n,q,a,d,e,p,?]?\n> > +(1/1) Stage addition [y,n,q,a,d,e,p,?]? (1/1) Stage addition [y,n,q,a,d,e,p,?]?\n> > +Unknown command 'W' (use '?' for help)\n> > \n> > which makes me suspect a race. Perhaps because the prompt is going to\n> > stdout, but the \"Unknown command\" message goes to stderr?\n> \n> I have to read the thread pointed by Eric, but my knee-jerk reaction has\n> been to think in something like:\n> \n> diff --git a/add-patch.c b/add-patch.c\n> index 447e8166d2..0090543f89 100644\n> --- a/add-patch.c\n> +++ b/add-patch.c\n> @@ -292,6 +292,7 @@ static void err(struct add_p_state *s, const char *fmt, ...)\n> {\n> va_list args;\n> \n> va_start(args, fmt);\n> + fflush(stdout);\n> fputs(s->s.error_color, stderr);\n> vfprintf(stderr, fmt, args);\n\nI think the \"just send it all to stdout\" approach makes the most sense\nhere, but in case we don't do that: I don't think this will fix it. In\nthe output above it is the \"Unknown command\" output which is delayed,\nwhich is sent to stderr via err(). So flushing stdout again won't help.\nFlushing stderr after the vfprintf _might_ help (though I'm confused why\nstderr would be fully buffered in the first place).\n\n-Peff\n"},{"id":"493530","messageId":"433fe12c-170c-48b8-9654-f70d238ab120@gmail.com","threadId":"61323","inReplyTo":"20240426202333.GB13703@coredump.intra.peff.net","subject":"Re: [PATCH v4] add-patch: response to unknown command","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-04-26T20:41:13Z","receivedAt":"2024-04-26T20:41:16Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Fri, Apr 26, 2024 at 04:23:33PM -0400, Jeff King wrote:\n> On Thu, Apr 25, 2024 at 05:04:45AM +0200, Rubén Justo wrote:\n> \n> > > The test_cmp output looks\n> > > like this:\n> > > \n> > > -(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command 'W' (use '?' for help)\n> > > -(1/1) Stage addition [y,n,q,a,d,e,p,?]?\n> > > +(1/1) Stage addition [y,n,q,a,d,e,p,?]? (1/1) Stage addition [y,n,q,a,d,e,p,?]?\n> > > +Unknown command 'W' (use '?' for help)\n> > > \n> > > which makes me suspect a race. Perhaps because the prompt is going to\n> > > stdout, but the \"Unknown command\" message goes to stderr?\n> > \n> > I have to read the thread pointed by Eric, but my knee-jerk reaction has\n> > been to think in something like:\n> > \n> > diff --git a/add-patch.c b/add-patch.c\n> > index 447e8166d2..0090543f89 100644\n> > --- a/add-patch.c\n> > +++ b/add-patch.c\n> > @@ -292,6 +292,7 @@ static void err(struct add_p_state *s, const char *fmt, ...)\n> > {\n> > va_list args;\n> > \n> > va_start(args, fmt);\n> > + fflush(stdout);\n> > fputs(s->s.error_color, stderr);\n> > vfprintf(stderr, fmt, args);\n> \n> I think the \"just send it all to stdout\" approach makes the most sense\n> here, but in case we don't do that: I don't think this will fix it. In\n> the output above it is the \"Unknown command\" output which is delayed,\n> which is sent to stderr via err().\n\nVery true.\n\nI'm happy with what Junio has just queued. I do not plan to send a new\niteration, unless a new test break appears :-)\n\nThanks, all.\n"},{"id":"493625","messageId":"33bb69ad-ec79-4863-97b7-51605b94c9c5@gmail.com","threadId":"61323","inReplyTo":"xmqqfrva22py.fsf@gitster.g","subject":"Re: [PATCH v4] add-patch: response to unknown command","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-04-29T09:48:13Z","receivedAt":"2024-04-29T09:48:15Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Junio\n\nOn 24/04/2024 16:35, Junio C Hamano wrote:\n> phillip.wood123@gmail.com writes:\n> \n>> On 22/04/2024 16:50, Junio C Hamano wrote:\n>>> Rubén Justo writes:\n>>>\n>>>> 1: 0317594bce ! 1: b418b03f15 add-patch: response to unknown command\n>>>> @@ t/t3701-add-interactive.sh: test_expect_success 'warn about add.interactive.useB\n>>>> +\ttest_when_finished \"git reset --hard; rm -f command\" &&\n>>>> +\techo W >command &&\n>>>> +\tgit add -N command &&\n>>>> -+\tcat >expect <<-EOF &&\n>>>> -+\tdiff --git a/command b/command\n>>>> -+\tnew file mode 100644\n>>>> -+\tindex 0000000..a42d8ff\n>>>> -+\t--- /dev/null\n>>>> -+\t+++ b/command\n>>>> -+\t@@ -0,0 +1 @@\n>>>> -+\t+W\n>>>> ++\tgit diff command >expect &&\n>>>> ++\tcat >>expect <<-EOF &&\n>>>> +\t(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help)\n>>>> +\t(1/1) Stage addition [y,n,q,a,d,e,p,?]?$SP\n>>>> +\tEOF\n>>> Interesting.\n>>> My first reaction was \"how is this different from checking just the\n>>> last line of the actual output? The early part of expect and actual\n>>> both come from an internal invocation of 'git diff', and they must\n>>> match by definition\".\n>>> But that may really be the point of this test.\n>>\n>> Yes - we want to make sure that we are not printing the help and the\n>> only way to do that is to check the whole output\n> \n> I was not questioning that part of the patch. \"My first reaction\"\n> was solely about use of \"git diff\" to replace the golden copy of\n> expected result in the test itself, only to allow for use of\n> different hash functions. As you (or somebody else?) mentioned in\n> an earlier review, diff_cmp is there for exactly that purpose.\n\nOh sorry I'd misunderstood\n\n>>> That is, we may later decide to tweak the way \"git diff\" hunks are\n>>> presented, and we expect that the way \"git add -p\" presents the\n>>> hunks would change together with it automatically.\n> \n> This argument cuts both ways, by the way.\n> \n> Insisting that the output match the explicit expectation (not what\n> \"git diff\" of the day produces) has a few advantages. It makes the\n> test more explicit and easy to see what output we are expecting, and\n> more importantly, it forces us to update the test when we decide to\n> tweak the output from the command being tested (i.e. hunk selection\n> UI) and/or the output from \"git diff\" command.\n\nThere is also a practical argument against using \"git diff\" to generate \nthe expected output as it only works if the diff contains a single hunk. \nIf the diff contains more than one hunk \"add -p\" displays them separately.\n\nBest Wishes\n\nPhillip\n"},{"id":"493646","messageId":"xmqqttjkxiai.fsf@gitster.g","threadId":"61323","inReplyTo":"33bb69ad-ec79-4863-97b7-51605b94c9c5@gmail.com","subject":"Re: [PATCH v4] add-patch: response to unknown command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-29T16:09:09Z","receivedAt":"2024-04-29T16:09:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> There is also a practical argument against using \"git diff\" to\n> generate the expected output as it only works if the diff contains a\n> single hunk. If the diff contains more than one hunk \"add -p\" displays\n> them separately.\n\nTrue, but spliting the output from \"git diff\" at @@ hunk boundary\nlines would allow you to hardcode the assumption that \"add -p\" shows\neach hunk exactly as \"git diff\" would. So if we really wanted to\nhave the convenience that the tests' expectation automatically\nfollow what \"git diff\" of the day happens to produce, it is still a\nviable approach to use \"git diff\" output with some post-processing.\n\nThanks.\n\n\n\n\n\n"},{"id":"493668","messageId":"952a9514-3cf1-4601-8f0d-db57adc750c3@gmail.com","threadId":"61323","inReplyTo":"b209a2b8-f98f-4f14-a687-9022d30968dd@gmail.com","subject":"[PATCH v5 0/2] add-patch: response to unknown command","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-04-29T18:35:01Z","receivedAt":"2024-04-29T18:35:04Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"I'm happy with the currently queued version v4+fix, and my understanding\nis that it is OK enough. However in the recent 'What's cooking' it is\nmarked with 'Expecting a reroll'. Here it is, in case it is expected. \n\nRubén Justo (2):\n add-patch: do not show UI messages on stderr\n add-patch: response to unknown command\n\n add-patch.c | 18 ++++++++++--------\n t/t3701-add-interactive.sh | 28 +++++++++++++++++++++-------\n 2 files changed, 31 insertions(+), 15 deletions(-)\n\n-- \n2.45.0.2.g84ce137e4a\n\n"},{"id":"493669","messageId":"10905ab3-bb3c-4669-9177-84c8e6759616@gmail.com","threadId":"61323","inReplyTo":"952a9514-3cf1-4601-8f0d-db57adc750c3@gmail.com","subject":"[PATCH v5 1/2] add-patch: do not show UI messages on stderr","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-04-29T18:37:03Z","receivedAt":"2024-04-29T18:37:06Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"There is no need to show some UI messages on stderr, and yet doing so\nmay produce some undesirable results, such as messages appearing in an\nunexpected order.\n\nLet's use stdout for all UI messages, and adjusts the tests accordingly.\n\nSigned-off-by: Rubén Justo \n---\n add-patch.c | 13 ++++++-------\n t/t3701-add-interactive.sh | 12 ++++++------\n 2 files changed, 12 insertions(+), 13 deletions(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex 0997d4af73..fc0eed4fd4 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -293,10 +293,9 @@ static void err(struct add_p_state *s, const char *fmt, ...)\n \tva_list args;\n \n \tva_start(args, fmt);\n-\tfputs(s->s.error_color, stderr);\n-\tvfprintf(stderr, fmt, args);\n-\tfputs(s->s.reset_color, stderr);\n-\tfputc('\\n', stderr);\n+\tfputs(s->s.error_color, stdout);\n+\tvprintf(fmt, args);\n+\tputs(s->s.reset_color);\n \tva_end(args);\n }\n \n@@ -1326,7 +1325,7 @@ static int apply_for_checkout(struct add_p_state *s, struct strbuf *diff,\n \t\terr(s, _(\"Nothing was applied.\\n\"));\n \t} else\n \t\t/* As a last resort, show the diff to the user */\n-\t\tfwrite(diff->buf, diff->len, 1, stderr);\n+\t\tfwrite(diff->buf, diff->len, 1, stdout);\n \n \treturn 0;\n }\n@@ -1778,9 +1777,9 @@ int run_add_p(struct repository *r, enum add_p_mode mode,\n \t\t\tbreak;\n \n \tif (s.file_diff_nr == 0)\n-\t\tfprintf(stderr, _(\"No changes.\\n\"));\n+\t\terr(&s, _(\"No changes.\"));\n \telse if (binary_count == s.file_diff_nr)\n-\t\tfprintf(stderr, _(\"Only binary files changed.\\n\"));\n+\t\terr(&s, _(\"Only binary files changed.\"));\n \n \tadd_p_state_clear(&s);\n \treturn 0;\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 04d8333373..c5531520cb 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -45,13 +45,13 @@ test_expect_success 'warn about add.interactive.useBuiltin' '\n \tcat >expect <<-\\EOF &&\n \twarning: the add.interactive.useBuiltin setting has been removed!\n \tSee its entry in '\\''git help config'\\'' for details.\n-\tNo changes.\n \tEOF\n+\techo \"No changes.\" >expect.out &&\n \n \tfor v in = =true =false\n \tdo\n \t\tgit -c \"add.interactive.useBuiltin$v\" add -p >out 2>actual &&\n-\t\ttest_must_be_empty out &&\n+\t\ttest_cmp expect.out out &&\n \t\ttest_cmp expect actual || return 1\n \tdone\n '\n@@ -335,13 +335,13 @@ test_expect_success 'different prompts for mode change/deleted' '\n \n test_expect_success 'correct message when there is nothing to do' '\n \tgit reset --hard &&\n-\tgit add -p 2>err &&\n-\ttest_grep \"No changes\" err &&\n+\tgit add -p >out &&\n+\ttest_grep \"No changes\" out &&\n \tprintf \"\\\\0123\" >binary &&\n \tgit add binary &&\n \tprintf \"\\\\0abc\" >binary &&\n-\tgit add -p 2>err &&\n-\ttest_grep \"Only binary files changed\" err\n+\tgit add -p >out &&\n+\ttest_grep \"Only binary files changed\" out\n '\n \n test_expect_success 'setup again' '\n-- \n2.45.0.2.g84ce137e4a\n"},{"id":"493670","messageId":"95c0dfff-3b11-4156-a502-c741fc0b76f7@gmail.com","threadId":"61323","inReplyTo":"952a9514-3cf1-4601-8f0d-db57adc750c3@gmail.com","subject":"[PATCH v5 2/2] add-patch: response to unknown command","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-04-29T18:37:12Z","receivedAt":"2024-04-29T18:37:14Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"When the user gives an unknown command to the \"add -p\" prompt, the list\nof accepted commands with their explanation is given. This is the same\noutput they get when they say '?'.\n\nHowever, the unknown command may be due to a user input error rather\nthan the user not knowing the valid command.\n\nTo reduce the likelihood of user confusion and error repetition, instead\nof displaying the list of accepted commands, display a short error\nmessage with the unknown command received, as feedback to the user.\n\nInclude a reminder about the current command '?' in the new message, to\nguide the user if they want help.\n\nSigned-off-by: Rubén Justo \n---\n add-patch.c | 5 ++++-\n t/t3701-add-interactive.sh | 16 +++++++++++++++-\n 2 files changed, 19 insertions(+), 2 deletions(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex fc0eed4fd4..2252895c28 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -1667,7 +1667,7 @@ static int patch_update_file(struct add_p_state *s,\n \t\t\t}\n \t\t} else if (s->answer.buf[0] == 'p') {\n \t\t\trendered_hunk_index = -1;\n-\t\t} else {\n+\t\t} else if (s->answer.buf[0] == '?') {\n \t\t\tconst char *p = _(help_patch_remainder), *eol = p;\n \n \t\t\tcolor_fprintf(stdout, s->s.help_color, \"%s\",\n@@ -1691,6 +1691,9 @@ static int patch_update_file(struct add_p_state *s,\n \t\t\t\tcolor_fprintf_ln(stdout, s->s.help_color,\n \t\t\t\t\t\t \"%.*s\", (int)(eol - p), p);\n \t\t\t}\n+\t\t} else {\n+\t\t\terr(s, _(\"Unknown command '%s' (use '?' for help)\"),\n+\t\t\t s->answer.buf);\n \t\t}\n \t}\n \ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex c5531520cb..28a95a775d 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -8,6 +8,8 @@ TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-terminal.sh\n \n+SP=\" \"\n+\n diff_cmp () {\n \tfor x\n \tdo\n@@ -56,6 +58,19 @@ test_expect_success 'warn about add.interactive.useBuiltin' '\n \tdone\n '\n \n+test_expect_success 'unknown command' '\n+\ttest_when_finished \"git reset --hard; rm -f command\" &&\n+\techo W >command &&\n+\tgit add -N command &&\n+\tgit diff command >expect &&\n+\tcat >>expect <<-EOF &&\n+\t(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help)\n+\t(1/1) Stage addition [y,n,q,a,d,e,p,?]?$SP\n+\tEOF\n+\tgit add -p -- command actual 2>&1 &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'setup (initial)' '\n \techo content >file &&\n \tgit add file &&\n@@ -232,7 +247,6 @@ test_expect_success 'setup file' '\n '\n \n test_expect_success 'setup patch' '\n-\tSP=\" \" &&\n \tNULL=\"\" &&\n \tcat >patch <<-EOF\n \t@@ -1,4 +1,4 @@\n-- \n2.45.0.2.g84ce137e4a\n"},{"id":"493677","messageId":"xmqqfrv4ug3l.fsf@gitster.g","threadId":"61323","inReplyTo":"10905ab3-bb3c-4669-9177-84c8e6759616@gmail.com","subject":"Re: [PATCH v5 1/2] add-patch: do not show UI messages on stderr","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-29T19:24:46Z","receivedAt":"2024-04-29T19:24:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> diff --git a/add-patch.c b/add-patch.c\n> index 0997d4af73..fc0eed4fd4 100644\n> --- a/add-patch.c\n> +++ b/add-patch.c\n> @@ -293,10 +293,9 @@ static void err(struct add_p_state *s, const char *fmt, ...)\n> \tva_list args;\n> \n> \tva_start(args, fmt);\n> -\tfputs(s->s.error_color, stderr);\n> -\tvfprintf(stderr, fmt, args);\n> -\tfputs(s->s.reset_color, stderr);\n> -\tfputc('\\n', stderr);\n> +\tfputs(s->s.error_color, stdout);\n> +\tvprintf(fmt, args);\n> +\tputs(s->s.reset_color);\n\nI like the attention of the detail here ;-).\n"},{"id":"493752","messageId":"20240430105256.GH1279403@coredump.intra.peff.net","threadId":"61323","inReplyTo":"xmqqfrv4ug3l.fsf@gitster.g","subject":"Re: [PATCH v5 1/2] add-patch: do not show UI messages on stderr","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-30T10:52:56Z","receivedAt":"2024-04-30T10:52:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 29, 2024 at 12:24:46PM -0700, Junio C Hamano wrote:\n\n> Rubén Justo writes:\n> \n> > diff --git a/add-patch.c b/add-patch.c\n> > index 0997d4af73..fc0eed4fd4 100644\n> > --- a/add-patch.c\n> > +++ b/add-patch.c\n> > @@ -293,10 +293,9 @@ static void err(struct add_p_state *s, const char *fmt, ...)\n> > \tva_list args;\n> > \n> > \tva_start(args, fmt);\n> > -\tfputs(s->s.error_color, stderr);\n> > -\tvfprintf(stderr, fmt, args);\n> > -\tfputs(s->s.reset_color, stderr);\n> > -\tfputc('\\n', stderr);\n> > +\tfputs(s->s.error_color, stdout);\n> > +\tvprintf(fmt, args);\n> > +\tputs(s->s.reset_color);\n> \n> I like the attention of the detail here ;-).\n\nIndeed. I had to read this several times to wonder why it was not a\nmistake to leave the first fputs() but use vprintf() and puts() for the\nother two (for those just reading, the answer is that puts() prints an\nextra newline, so we can only use it at the end). Which IMHO really just\npoints to how inconsistent the stdio interfaces are, but there is\nnothing we can do about that. ;)\n\nI am tempted to suggest that it borders on too-clever, and writing out\n\"stdout\" everywhere with vfprintf() and fputs() would be more obvious.\nBut in a little self-contained function like this I don't know that it\nmatters much.\n\n-Peff\n"},{"id":"493786","messageId":"27fbb12a-f2d2-459c-a27b-519f69242105@gmail.com","threadId":"61323","inReplyTo":"10905ab3-bb3c-4669-9177-84c8e6759616@gmail.com","subject":"Re: [PATCH v5 1/2] add-patch: do not show UI messages on stderr","fromName":"","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-04-30T14:47:04Z","receivedAt":"2024-04-30T14:47:08Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Rubén\n\nOn 29/04/2024 19:37, Rubén Justo wrote:\n> There is no need to show some UI messages on stderr, and yet doing so\n> may produce some undesirable results, such as messages appearing in an\n> unexpected order.\n> \n> Let's use stdout for all UI messages, and adjusts the tests accordingly.\n\nThe test changes in \"warn add.interactive.useBultin\" show that we still \nprint a warning to stderr, I don't think that particular case is worth \nworrying about though.\n\n> Signed-off-by: Rubén Justo \n> ---\n> add-patch.c | 13 ++++++-------\n> t/t3701-add-interactive.sh | 12 ++++++------\n> 2 files changed, 12 insertions(+), 13 deletions(-)\n> \n> diff --git a/add-patch.c b/add-patch.c\n> index 0997d4af73..fc0eed4fd4 100644\n> --- a/add-patch.c\n> +++ b/add-patch.c\n> @@ -293,10 +293,9 @@ static void err(struct add_p_state *s, const char *fmt, ...)\n> \tva_list args;\n> \n> \tva_start(args, fmt);\n> -\tfputs(s->s.error_color, stderr);\n> -\tvfprintf(stderr, fmt, args);\n> -\tfputs(s->s.reset_color, stderr);\n> -\tfputc('\\n', stderr);\n> +\tfputs(s->s.error_color, stdout);\n> +\tvprintf(fmt, args);\n> +\tputs(s->s.reset_color);\n> \tva_end(args);\n> }\n\nThis looks like a good change\n\n> @@ -1326,7 +1325,7 @@ static int apply_for_checkout(struct add_p_state *s, struct strbuf *diff,\n> \t\terr(s, _(\"Nothing was applied.\\n\"));\n> \t} else\n> \t\t/* As a last resort, show the diff to the user */\n> -\t\tfwrite(diff->buf, diff->len, 1, stderr);\n> +\t\tfwrite(diff->buf, diff->len, 1, stdout);\n\nThis seems reasonable as we'd print the \"Nothing was applied\" error \nmessage above to stdout now. Anything \"git apply\" writes to stderr \nshould be flushed when it exits before we print these messages.\n\n> \n> \treturn 0;\n> }\n> @@ -1778,9 +1777,9 @@ int run_add_p(struct repository *r, enum add_p_mode mode,\n> \t\t\tbreak;\n> \n> \tif (s.file_diff_nr == 0)\n> -\t\tfprintf(stderr, _(\"No changes.\\n\"));\n> +\t\terr(&s, _(\"No changes.\"));\n> \telse if (binary_count == s.file_diff_nr)\n> -\t\tfprintf(stderr, _(\"Only binary files changed.\\n\"));\n> +\t\terr(&s, _(\"Only binary files changed.\"));\n\nThese two mean we'll now color these messages which we didn't do before. \nI think if we hit this code we don't print anything else (apart from the \nwarning about add.interactive.useBuiltin being removed) so it probably \ndoes not matter whether we use stdout or stderr here and I don't have a \nstrong opinion either way.\n\nBest Wishes\n\nPhillip\n\n> \tadd_p_state_clear(&s);\n> \treturn 0;\n> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\n> index 04d8333373..c5531520cb 100755\n> --- a/t/t3701-add-interactive.sh\n> +++ b/t/t3701-add-interactive.sh\n> @@ -45,13 +45,13 @@ test_expect_success 'warn about add.interactive.useBuiltin' '\n> \tcat >expect <<-\\EOF &&\n> \twarning: the add.interactive.useBuiltin setting has been removed!\n> \tSee its entry in '\\''git help config'\\'' for details.\n> -\tNo changes.\n> \tEOF\n> +\techo \"No changes.\" >expect.out &&\n> \n> \tfor v in = =true =false\n> \tdo\n> \t\tgit -c \"add.interactive.useBuiltin$v\" add -p >out 2>actual &&\n> -\t\ttest_must_be_empty out &&\n> +\t\ttest_cmp expect.out out &&\n> \t\ttest_cmp expect actual || return 1\n> \tdone\n> '\n> @@ -335,13 +335,13 @@ test_expect_success 'different prompts for mode change/deleted' '\n> \n> test_expect_success 'correct message when there is nothing to do' '\n> \tgit reset --hard &&\n> -\tgit add -p 2>err &&\n> -\ttest_grep \"No changes\" err &&\n> +\tgit add -p >out &&\n> +\ttest_grep \"No changes\" out &&\n> \tprintf \"\\\\0123\" >binary &&\n> \tgit add binary &&\n> \tprintf \"\\\\0abc\" >binary &&\n> -\tgit add -p 2>err &&\n> -\ttest_grep \"Only binary files changed\" err\n> +\tgit add -p >out &&\n> +\ttest_grep \"Only binary files changed\" out\n> '\n> \n> test_expect_success 'setup again' '\n"},{"id":"493791","messageId":"90464326-0d0a-4cc6-84cd-eb1e5489a91a@gmail.com","threadId":"61323","inReplyTo":"20240430105256.GH1279403@coredump.intra.peff.net","subject":"Re: [PATCH v5 1/2] add-patch: do not show UI messages on stderr","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-04-30T16:35:12Z","receivedAt":"2024-04-30T16:35:15Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Tue, Apr 30, 2024 at 06:52:56AM -0400, Jeff King wrote:\n> On Mon, Apr 29, 2024 at 12:24:46PM -0700, Junio C Hamano wrote:\n> \n> > Rubén Justo writes:\n> > \n> > > diff --git a/add-patch.c b/add-patch.c\n> > > index 0997d4af73..fc0eed4fd4 100644\n> > > --- a/add-patch.c\n> > > +++ b/add-patch.c\n> > > @@ -293,10 +293,9 @@ static void err(struct add_p_state *s, const char *fmt, ...)\n> > > \tva_list args;\n> > > \n> > > \tva_start(args, fmt);\n> > > -\tfputs(s->s.error_color, stderr);\n> > > -\tvfprintf(stderr, fmt, args);\n> > > -\tfputs(s->s.reset_color, stderr);\n> > > -\tfputc('\\n', stderr);\n> > > +\tfputs(s->s.error_color, stdout);\n> > > +\tvprintf(fmt, args);\n> > > +\tputs(s->s.reset_color);\n> > \n> > I like the attention of the detail here ;-).\n> \n> Indeed. I had to read this several times to wonder why it was not a\n> mistake to leave the first fputs() but use vprintf() and puts() for the\n> other two (for those just reading, the answer is that puts() prints an\n> extra newline, so we can only use it at the end).\n\nI did not know the details (or had happily forgotten them) but Junio\nignoring my comments in [1] intrigued me :-). A simple test would have\nbeen quick, but \"man puts\" was quicker; my comments were not correct.\n\nJust to be clear, and to extend your comment, in case any reader is\ninterested, this:\n\n\tvfprintf(stdout, fmt, args);\n\ncan be written as follows:\n\n\tvprintf(fmt, args);\n\nAnd this:\n\n\tfputs(s->s.reset_color, stdout);\n\tfputc('\\n', stdout);\n\ncan be shortened to:\n\n\tputs(s->s.reset_color);\n\nThe difference in vfprintf and vprintf is quite obvious IMHO, but not so\nobvious with puts. The documentation says:\n\n SYNOPSIS\n\n int puts(const char *s);\n\n DESCRIPTION\n\n puts() writes the string s and a trailing newline to stdout.\n\n\n\n [1] - https://lore.kernel.org/git/4e2bc660-ee33-4641-aca5-783d0cefcd23@gmail.com/T/#m644a682364212729a2b21c052ef744039c26f972\n"},{"id":"493793","messageId":"ce11355e-25f3-4d76-91ae-bd561143dd49@gmail.com","threadId":"61323","inReplyTo":"27fbb12a-f2d2-459c-a27b-519f69242105@gmail.com","subject":"Re: [PATCH v5 1/2] add-patch: do not show UI messages on stderr","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-04-30T16:38:32Z","receivedAt":"2024-04-30T16:38:36Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Tue, Apr 30, 2024 at 03:47:04PM +0100, phillip.wood123@gmail.com wrote:\n\n> > @@ -1778,9 +1777,9 @@ int run_add_p(struct repository *r, enum add_p_mode mode,\n> > \t\t\tbreak;\n> > \tif (s.file_diff_nr == 0)\n> > -\t\tfprintf(stderr, _(\"No changes.\\n\"));\n> > +\t\terr(&s, _(\"No changes.\"));\n> > \telse if (binary_count == s.file_diff_nr)\n> > -\t\tfprintf(stderr, _(\"Only binary files changed.\\n\"));\n> > +\t\terr(&s, _(\"Only binary files changed.\"));\n> \n> These two mean we'll now color these messages which we didn't do before. I\n> think if we hit this code we don't print anything else (apart from the\n> warning about add.interactive.useBuiltin being removed) so it probably does\n> not matter whether we use stdout or stderr here and I don't have a strong\n> opinion either way.\n\nCan we consider those messages not part of the UI? IIUC, if we hit that\ncode we haven't entered in the interactive UI. Maybe we should:\n\ndiff --git a/add-patch.c b/add-patch.c\nindex c28ad380ed..b11a435738 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -1780,9 +1780,9 @@ int run_add_p(struct repository *r, enum add_p_mode mode,\n \t\t\tbreak;\n \n \tif (s.file_diff_nr == 0)\n-\t\terr(&s, _(\"No changes.\"));\n+\t\terror(_(\"no changes\"));\n \telse if (binary_count == s.file_diff_nr)\n-\t\terr(&s, _(\"Only binary files changed.\"));\n+\t\terror(_(\"only binary files changed\"));\n \n \tadd_p_state_clear(&s);\n \treturn 0;\n\nOr, simply leave them untouched in this series.\n\n> \n> Best Wishes\n> \n> Phillip\n\nThanks.\n"},{"id":"493800","messageId":"xmqqikzyrd1m.fsf@gitster.g","threadId":"61323","inReplyTo":"20240430105256.GH1279403@coredump.intra.peff.net","subject":"Re: [PATCH v5 1/2] add-patch: do not show UI messages on stderr","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-30T17:11:17Z","receivedAt":"2024-04-30T17:11:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I am tempted to suggest that it borders on too-clever, and writing out\n> \"stdout\" everywhere with vfprintf() and fputs() would be more obvious.\n> But in a little self-contained function like this I don't know that it\n> matters much.\n\nI share your preference (and that was the way I wrote \"it would look\nlike this\" version earlier) but I agree that this is too small to\nmatter.\n\nThanks.\n"},{"id":"493801","messageId":"xmqqedamrcqj.fsf@gitster.g","threadId":"61323","inReplyTo":"90464326-0d0a-4cc6-84cd-eb1e5489a91a@gmail.com","subject":"Re: [PATCH v5 1/2] add-patch: do not show UI messages on stderr","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-30T17:17:56Z","receivedAt":"2024-04-30T17:17:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n>> Indeed. I had to read this several times to wonder why it was not a\n>> mistake to leave the first fputs() but use vprintf() and puts() for the\n>> other two (for those just reading, the answer is that puts() prints an\n>> extra newline, so we can only use it at the end).\n>\n> I did not know the details (or had happily forgotten them) but Junio\n> ignoring my comments in [1] intrigued me :-). A simple test would have\n> been quick, but \"man puts\" was quicker; my comments were not correct.\n\nI thought you were suggesting to fold fputs()+putchar('\\n') into\nputs(), which is a change that does not break. I didn't even know\nthat you were suggesting to make more changes that would break the\noutput. After all, you said only \"can be less explicit\" without\nsaying which use of \"stdout\" you meant to make \"less explicit\" ;-).\n"},{"id":"493830","messageId":"7a3fcf9c-a70f-47e8-9624-40b407f90242@gmail.com","threadId":"61323","inReplyTo":"ce11355e-25f3-4d76-91ae-bd561143dd49@gmail.com","subject":"Re: [PATCH v5 1/2] add-patch: do not show UI messages on stderr","fromName":"","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-05-01T15:39:53Z","receivedAt":"2024-05-01T15:39:56Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Rubén\n\nOn 30/04/2024 17:38, Rubén Justo wrote:\n> On Tue, Apr 30, 2024 at 03:47:04PM +0100, phillip.wood123@gmail.com wrote:\n> \n>>> @@ -1778,9 +1777,9 @@ int run_add_p(struct repository *r, enum add_p_mode mode,\n>>> \t\t\tbreak;\n>>> \tif (s.file_diff_nr == 0)\n>>> -\t\tfprintf(stderr, _(\"No changes.\\n\"));\n>>> +\t\terr(&s, _(\"No changes.\"));\n>>> \telse if (binary_count == s.file_diff_nr)\n>>> -\t\tfprintf(stderr, _(\"Only binary files changed.\\n\"));\n>>> +\t\terr(&s, _(\"Only binary files changed.\"));\n>>\n>> These two mean we'll now color these messages which we didn't do before. I\n>> think if we hit this code we don't print anything else (apart from the\n>> warning about add.interactive.useBuiltin being removed) so it probably does\n>> not matter whether we use stdout or stderr here and I don't have a strong\n>> opinion either way.\n> \n> Can we consider those messages not part of the UI? IIUC, if we hit that\n> code we haven't entered in the interactive UI. \n\nI thikn so\n\n> Maybe we should:\n> \n> diff --git a/add-patch.c b/add-patch.c\n> index c28ad380ed..b11a435738 100644\n> --- a/add-patch.c\n> +++ b/add-patch.c\n> @@ -1780,9 +1780,9 @@ int run_add_p(struct repository *r, enum add_p_mode mode,\n> \t\t\tbreak;\n> \n> \tif (s.file_diff_nr == 0)\n> -\t\terr(&s, _(\"No changes.\"));\n> +\t\terror(_(\"no changes\"));\n> \telse if (binary_count == s.file_diff_nr)\n> -\t\terr(&s, _(\"Only binary files changed.\"));\n> +\t\terror(_(\"only binary files changed\"));\n> \n> \tadd_p_state_clear(&s);\n> \treturn 0;\n> \n> Or, simply leave them untouched in this series.\n\nEither option sounds good to me\n\nBest Wishes\n\nPhillip\n"},{"id":"493831","messageId":"xmqq5xvx5x1u.fsf@gitster.g","threadId":"61323","inReplyTo":"7a3fcf9c-a70f-47e8-9624-40b407f90242@gmail.com","subject":"Re: [PATCH v5 1/2] add-patch: do not show UI messages on stderr","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-01T16:14:37Z","receivedAt":"2024-05-01T16:14:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"phillip.wood123@gmail.com writes:\n\n>>>> -\t\tfprintf(stderr, _(\"No changes.\\n\"));\n>>>> +\t\terr(&s, _(\"No changes.\"));\n>>>> \telse if (binary_count == s.file_diff_nr)\n>>>> -\t\tfprintf(stderr, _(\"Only binary files changed.\\n\"));\n>>>> +\t\terr(&s, _(\"Only binary files changed.\"));\n>>>\n>>> These two mean we'll now color these messages which we didn't do before. I\n> ...\n> I think so\n> ...\n>> -\t\terr(&s, _(\"Only binary files changed.\"));\n>> +\t\terror(_(\"only binary files changed\"));\n>> \tadd_p_state_clear(&s);\n>> \treturn 0;\n>> Or, simply leave them untouched in this series.\n>\n> Either option sounds good to me\n\nWe are returning \"success\" to the caller, so using error() here is a\nbit strong. Judging from how other messages emitted with err() in\nthis program is meant to help the end user, they are all to tell the\nuser why their input caused no actual change, and showing these two\nmessages the same way as these other messages would be the best for\nconsistency.\n\nSo I'm inclined to say that what was posted is good. If it paints\nthese two messages in the same color as others, that is even getter.\n"},{"id":"493845","messageId":"0dc1f9f9-02ec-4394-9c25-7a7fee6147ee@gmail.com","threadId":"61323","inReplyTo":"xmqq5xvx5x1u.fsf@gitster.g","subject":"Re: [PATCH v5 1/2] add-patch: do not show UI messages on stderr","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-05-01T21:13:03Z","receivedAt":"2024-05-01T21:13:10Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Wed, May 01, 2024 at 09:14:37AM -0700, Junio C Hamano wrote:\n> phillip.wood123@gmail.com writes:\n> \n> >>>> -\t\tfprintf(stderr, _(\"No changes.\\n\"));\n> >>>> +\t\terr(&s, _(\"No changes.\"));\n> >>>> \telse if (binary_count == s.file_diff_nr)\n> >>>> -\t\tfprintf(stderr, _(\"Only binary files changed.\\n\"));\n> >>>> +\t\terr(&s, _(\"Only binary files changed.\"));\n> >>>\n> >>> These two mean we'll now color these messages which we didn't do before. I\n> > ...\n> > I think so\n> > ...\n> >> -\t\terr(&s, _(\"Only binary files changed.\"));\n> >> +\t\terror(_(\"only binary files changed\"));\n> >> \tadd_p_state_clear(&s);\n> >> \treturn 0;\n> >> Or, simply leave them untouched in this series.\n> >\n> > Either option sounds good to me\n> \n> We are returning \"success\" to the caller, so using error() here is a\n> bit strong. Judging from how other messages emitted with err() in\n> this program is meant to help the end user, they are all to tell the\n> user why their input caused no actual change, and showing these two\n> messages the same way as these other messages would be the best for\n> consistency.\n> \n> So I'm inclined to say that what was posted is good. If it paints\n> these two messages in the same color as others, that is even getter.\n\nHaving:\n\n $ printf \"\\0\" >binary\n $ git add -N binary\n\nDisplaying the message in RED does not seem as a bad change:\n\n $ git add -i\n staged unstaged path\n 1: +0/-0 binary binary\n \n *** Commands ***\n 1: status 2: update 3: revert 4: add untracked\n 5: patch 6: diff 7: quit 8: help\n What now> p\n Only binary files changed.\n\nHowever, here could be disturbing:\n\n $ git add -p\n Only binary files changed.\n\nAnd the \"No changes.\" message is going to have even more audience, I\nthink.\n\nPerhaps this seems sensible:\n\nRange-diff against v5:\n2: b0f042f4ff ! 1: 94c3f80041 add-patch: do not show UI messages on stderr\n @@ add-patch.c: int run_add_p(struct repository *r, enum add_p_mode mode,\n\n if (s.file_diff_nr == 0)\n - fprintf(stderr, _(\"No changes.\\n\"));\n -+ err(&s, _(\"No changes.\"));\n ++ fprintf(stdout, _(\"No changes.\\n\"));\n else if (binary_count == s.file_diff_nr)\n - fprintf(stderr, _(\"Only binary files changed.\\n\"));\n -+ err(&s, _(\"Only binary files changed.\"));\n ++ fprintf(stdout, _(\"Only binary files changed.\\n\"));\n\n add_p_state_clear(&s);\n return 0;\n1: 4a0eb1337f = 2: abaf904e8c add-patch: response to unknown command\n\nBut I'm not sure and I do not want to send a new iteration unless it is\nnecessary. I was already happy with previous versions ;-).\n"},{"id":"493922","messageId":"xmqqcyq4f9th.fsf@gitster.g","threadId":"61323","inReplyTo":"0dc1f9f9-02ec-4394-9c25-7a7fee6147ee@gmail.com","subject":"Re: [PATCH v5 1/2] add-patch: do not show UI messages on stderr","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-02T16:38:34Z","receivedAt":"2024-05-02T16:38:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> But I'm not sure and I do not want to send a new iteration unless it is\n> necessary. I was already happy with previous versions ;-).\n\nLet's call it done-well-enough and merge it down.\n\nThanks.\n"}]}