| Message ID | b209a2b8-f98f-4f14-a687-9022d30968dd@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Commit | ea7d103d395c07404861999ceca51e3267bf7e74 |
| Headers | show |
| Series | [v4] add-patch: response to unknown command | expand |
Rubén Justo <rjusto@gmail.com> writes: > 1: 0317594bce ! 1: b418b03f15 add-patch: response to unknown command > @@ t/t3701-add-interactive.sh: test_expect_success 'warn about add.interactive.useB > + test_when_finished "git reset --hard; rm -f command" && > + echo W >command && > + git add -N command && > -+ cat >expect <<-EOF && > -+ diff --git a/command b/command > -+ new file mode 100644 > -+ index 0000000..a42d8ff > -+ --- /dev/null > -+ +++ b/command > -+ @@ -0,0 +1 @@ > -+ +W > ++ git diff command >expect && > ++ cat >>expect <<-EOF && > + (1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help) > + (1/1) Stage addition [y,n,q,a,d,e,p,?]?$SP > + EOF Interesting. My first reaction was "how is this different from checking just the last line of the actual output? The early part of expect and actual both come from an internal invocation of 'git diff', and they must match by definition". But that may really be the point of this test. That is, we may later decide to tweak the way "git diff" hunks are presented, and we expect that the way "git add -p" presents the hunks would change together with it automatically.
Hi Junio On 22/04/2024 16:50, Junio C Hamano wrote: > Rubén Justo <rjusto@gmail.com> writes: > >> 1: 0317594bce ! 1: b418b03f15 add-patch: response to unknown command >> @@ t/t3701-add-interactive.sh: test_expect_success 'warn about add.interactive.useB >> + test_when_finished "git reset --hard; rm -f command" && >> + echo W >command && >> + git add -N command && >> -+ cat >expect <<-EOF && >> -+ diff --git a/command b/command >> -+ new file mode 100644 >> -+ index 0000000..a42d8ff >> -+ --- /dev/null >> -+ +++ b/command >> -+ @@ -0,0 +1 @@ >> -+ +W >> ++ git diff command >expect && >> ++ cat >>expect <<-EOF && >> + (1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help) >> + (1/1) Stage addition [y,n,q,a,d,e,p,?]?$SP >> + EOF > > Interesting. > > My first reaction was "how is this different from checking just the > last line of the actual output? The early part of expect and actual > both come from an internal invocation of 'git diff', and they must > match by definition". > > But that may really be the point of this test. Yes - we want to make sure that we are not printing the help and the only way to do that is to check the whole output Best Wishes Phillip > That is, we may later decide to tweak the way "git diff" hunks are > presented, and we expect that the way "git add -p" presents the > hunks would change together with it automatically.
phillip.wood123@gmail.com writes: > On 22/04/2024 16:50, Junio C Hamano wrote: >> Rubén Justo <rjusto@gmail.com> writes: >> >>> 1: 0317594bce ! 1: b418b03f15 add-patch: response to unknown command >>> @@ t/t3701-add-interactive.sh: test_expect_success 'warn about add.interactive.useB >>> + test_when_finished "git reset --hard; rm -f command" && >>> + echo W >command && >>> + git add -N command && >>> -+ cat >expect <<-EOF && >>> -+ diff --git a/command b/command >>> -+ new file mode 100644 >>> -+ index 0000000..a42d8ff >>> -+ --- /dev/null >>> -+ +++ b/command >>> -+ @@ -0,0 +1 @@ >>> -+ +W >>> ++ git diff command >expect && >>> ++ cat >>expect <<-EOF && >>> + (1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help) >>> + (1/1) Stage addition [y,n,q,a,d,e,p,?]?$SP >>> + EOF >> Interesting. >> My first reaction was "how is this different from checking just the >> last line of the actual output? The early part of expect and actual >> both come from an internal invocation of 'git diff', and they must >> match by definition". >> But that may really be the point of this test. > > Yes - we want to make sure that we are not printing the help and the > only way to do that is to check the whole output I was not questioning that part of the patch. "My first reaction" was solely about use of "git diff" to replace the golden copy of expected result in the test itself, only to allow for use of different hash functions. As you (or somebody else?) mentioned in an earlier review, diff_cmp is there for exactly that purpose. >> That is, we may later decide to tweak the way "git diff" hunks are >> presented, and we expect that the way "git add -p" presents the >> hunks would change together with it automatically. This argument cuts both ways, by the way. Insisting that the output match the explicit expectation (not what "git diff" of the day produces) has a few advantages. It makes the test more explicit and easy to see what output we are expecting, and more importantly, it forces us to update the test when we decide to tweak the output from the command being tested (i.e. hunk selection UI) and/or the output from "git diff" command.
On Sun, Apr 21, 2024 at 11:52:33PM +0200, Rubén Justo wrote: > +test_expect_success 'unknown command' ' > + test_when_finished "git reset --hard; rm -f command" && > + echo W >command && > + git add -N command && > + git diff command >expect && > + cat >>expect <<-EOF && > + (1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help) > + (1/1) Stage addition [y,n,q,a,d,e,p,?]?$SP > + EOF > + git add -p -- command <command >actual 2>&1 && > + test_cmp expect actual > +' I got a test failure on Windows CI from this. The test_cmp output looks like this: -(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command 'W' (use '?' for help) -(1/1) Stage addition [y,n,q,a,d,e,p,?]? +(1/1) Stage addition [y,n,q,a,d,e,p,?]? (1/1) Stage addition [y,n,q,a,d,e,p,?]? +Unknown command 'W' (use '?' for help) which makes me suspect a race. Perhaps because the prompt is going to stdout, but the "Unknown command" message goes to stderr? Maybe we should keep stdout and stderr separate and check them independently. -Peff
On Wed, Apr 24, 2024 at 9:44 PM Jeff King <peff@peff.net> wrote: > On Sun, Apr 21, 2024 at 11:52:33PM +0200, Rubén Justo wrote: > > +test_expect_success 'unknown command' ' > > + test_when_finished "git reset --hard; rm -f command" && > > + echo W >command && > > + git add -N command && > > + git diff command >expect && > > + cat >>expect <<-EOF && > > + (1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help) > > + (1/1) Stage addition [y,n,q,a,d,e,p,?]?$SP > > + EOF > > + git add -p -- command <command >actual 2>&1 && > > + test_cmp expect actual > > +' > > I got a test failure on Windows CI from this. The test_cmp output looks > like this: > > -(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command 'W' (use '?' for help) > -(1/1) Stage addition [y,n,q,a,d,e,p,?]? > +(1/1) Stage addition [y,n,q,a,d,e,p,?]? (1/1) Stage addition [y,n,q,a,d,e,p,?]? > +Unknown command 'W' (use '?' for help) > > which makes me suspect a race. Perhaps because the prompt is going to > stdout, but the "Unknown command" message goes to stderr? Maybe we > should keep stdout and stderr separate and check them independently. That's very reminiscent of [1]. Although, unlike [1], the output presented to the user in this case is (I suppose) less likely to be messed up; only the combined captured output is probably affected. So, capturing stdout and stderr separately would indeed be a good idea. [1]: https://lore.kernel.org/git/CAPig+cTGq-10ZTBts2LXRVdPMf2vNMX8HTuhg_+ZHSiLX-brOQ@mail.gmail.com/
On Wed, Apr 24, 2024 at 09:44:32PM -0400, Jeff King wrote: > On Sun, Apr 21, 2024 at 11:52:33PM +0200, Rubén Justo wrote: > > > +test_expect_success 'unknown command' ' > > + test_when_finished "git reset --hard; rm -f command" && > > + echo W >command && > > + git add -N command && > > + git diff command >expect && > > + cat >>expect <<-EOF && > > + (1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help) > > + (1/1) Stage addition [y,n,q,a,d,e,p,?]?$SP > > + EOF > > + git add -p -- command <command >actual 2>&1 && > > + test_cmp expect actual > > +' > > I got a test failure on Windows CI from this. Thank you for testing thoroughly. > The test_cmp output looks > like this: > > -(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command 'W' (use '?' for help) > -(1/1) Stage addition [y,n,q,a,d,e,p,?]? > +(1/1) Stage addition [y,n,q,a,d,e,p,?]? (1/1) Stage addition [y,n,q,a,d,e,p,?]? > +Unknown command 'W' (use '?' for help) > > which makes me suspect a race. Perhaps because the prompt is going to > stdout, but the "Unknown command" message goes to stderr? I have to read the thread pointed by Eric, but my knee-jerk reaction has been to think in something like: diff --git a/add-patch.c b/add-patch.c index 447e8166d2..0090543f89 100644 --- a/add-patch.c +++ b/add-patch.c @@ -292,6 +292,7 @@ static void err(struct add_p_state *s, const char *fmt, ...) { va_list args; va_start(args, fmt); + fflush(stdout); fputs(s->s.error_color, stderr); vfprintf(stderr, fmt, args);
Hi Peff On 25/04/2024 02:44, Jeff King wrote: > On Sun, Apr 21, 2024 at 11:52:33PM +0200, Rubén Justo wrote: > >> +test_expect_success 'unknown command' ' >> + test_when_finished "git reset --hard; rm -f command" && >> + echo W >command && >> + git add -N command && >> + git diff command >expect && >> + cat >>expect <<-EOF && >> + (1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help) >> + (1/1) Stage addition [y,n,q,a,d,e,p,?]?$SP >> + EOF >> + git add -p -- command <command >actual 2>&1 && >> + test_cmp expect actual >> +' > > I got a test failure on Windows CI from this. The test_cmp output looks > like this: > > -(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command 'W' (use '?' for help) > -(1/1) Stage addition [y,n,q,a,d,e,p,?]? > +(1/1) Stage addition [y,n,q,a,d,e,p,?]? (1/1) Stage addition [y,n,q,a,d,e,p,?]? > +Unknown command 'W' (use '?' for help) > > which makes me suspect a race. Perhaps because the prompt is going to > stdout, but the "Unknown command" message goes to stderr? Maybe we > should keep stdout and stderr separate and check them independently. Can we change err() to print to stdout? I can't really see the advantage of separating "normal output" and "error messages" for an interactive program like this. Best Wishes Phillip
Eric Sunshine <sunshine@sunshineco.com> writes: > That's very reminiscent of [1]. Although, unlike [1], the output > presented to the user in this case is (I suppose) less likely to be > messed up; only the combined captured output is probably affected. So, > capturing stdout and stderr separately would indeed be a good idea. Hmph, something along this line? It loses to capture how the output should be intermixed, which is essential to validate what the end-user should see. As we can see in the attached patch, we cannot express that "Unknown ..." should come in between two "Stage addition?" questions, which is a downside. Between adding fflush() before err() writes, and updating err() to write to the standard output stream, I am in favor of the latter for its simplicity (of the mental model of the resulting code, not of the patch that is required to do so). diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index ed7e414649..a8dfebd8d7 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -64,11 +64,12 @@ test_expect_success 'unknown command' ' git add -N command && git diff command >expect && cat >>expect <<-EOF && - (1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help) - (1/1) Stage addition [y,n,q,a,d,e,p,?]?$SP + (1/1) Stage addition [y,n,q,a,d,e,p,?]? (1/1) Stage addition [y,n,q,a,d,e,p,?]?$SP EOF - git add -p -- command <command >actual 2>&1 && - test_cmp expect actual + echo "Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help)" >expect.error && + git add -p -- command <command >actual 2>actual.error && + test_cmp expect actual && + test_cmp expect.error actual.error ' test_expect_success 'setup (initial)' '
On Thu, Apr 25, 2024 at 4:23 PM Junio C Hamano <gitster@pobox.com> wrote: > Eric Sunshine <sunshine@sunshineco.com> writes: > > That's very reminiscent of [1]. Although, unlike [1], the output > > presented to the user in this case is (I suppose) less likely to be > > messed up; only the combined captured output is probably affected. So, > > capturing stdout and stderr separately would indeed be a good idea. > > Between adding fflush() before err() writes, and updating err() to > write to the standard output stream, I am in favor of the latter for > its simplicity (of the mental model of the resulting code, not of > the patch that is required to do so). Writing to a common stream (stdout, in this case) for this sort of interactive session is indeed probably the way to go, as Phillip suggested. That was also the adopted solution to the cited similar example[1]; git-worktree was changed to send all its chatty output to stderr[2], which was appropriate for that (non-interactive) case. [1]: https://lore.kernel.org/git/CAPig+cTGq-10ZTBts2LXRVdPMf2vNMX8HTuhg_+ZHSiLX-brOQ@mail.gmail.com/ [2]: https://lore.kernel.org/git/20211203034420.47447-2-sunshine@sunshineco.com/
Junio C Hamano <gitster@pobox.com> writes: > Eric Sunshine <sunshine@sunshineco.com> writes: > >> That's very reminiscent of [1]. Although, unlike [1], the output >> presented to the user in this case is (I suppose) less likely to be >> messed up; only the combined captured output is probably affected. So, >> capturing stdout and stderr separately would indeed be a good idea. > > Hmph, something along this line? > > It loses to capture how the output should be intermixed, which is > essential to validate what the end-user should see. As we can see > in the attached patch, we cannot express that "Unknown ..." should > come in between two "Stage addition?" questions, which is a downside. > > Between adding fflush() before err() writes, and updating err() to > write to the standard output stream, I am in favor of the latter for > its simplicity (of the mental model of the resulting code, not of > the patch that is required to do so). The latter, which I claimed to prefer, would look like this. The idea is to perform the interactive session over the standard output (and the standard input). For that, we teach err() to use the standard output and have a few fprintf() to also call err(). A few tests expect certain messages to appear on the standard error stream, which needed adjusting. I know the previous one "fixes" the CI job at Windows, but I haven't tried this yet. diff --git c/add-patch.c w/add-patch.c index 7be142d448..c28ad380ed 100644 --- c/add-patch.c +++ w/add-patch.c @@ -293,10 +293,10 @@ static void err(struct add_p_state *s, const char *fmt, ...) va_list args; va_start(args, fmt); - fputs(s->s.error_color, stderr); - vfprintf(stderr, fmt, args); - fputs(s->s.reset_color, stderr); - fputc('\n', stderr); + fputs(s->s.error_color, stdout); + vfprintf(stdout, fmt, args); + fputs(s->s.reset_color, stdout); + fputc('\n', stdout); va_end(args); } @@ -1326,7 +1326,7 @@ static int apply_for_checkout(struct add_p_state *s, struct strbuf *diff, err(s, _("Nothing was applied.\n")); } else /* As a last resort, show the diff to the user */ - fwrite(diff->buf, diff->len, 1, stderr); + fwrite(diff->buf, diff->len, 1, stdout); return 0; } @@ -1780,9 +1780,9 @@ int run_add_p(struct repository *r, enum add_p_mode mode, break; if (s.file_diff_nr == 0) - fprintf(stderr, _("No changes.\n")); + err(&s, _("No changes.")); else if (binary_count == s.file_diff_nr) - fprintf(stderr, _("Only binary files changed.\n")); + err(&s, _("Only binary files changed.")); add_p_state_clear(&s); return 0; diff --git c/t/t3701-add-interactive.sh w/t/t3701-add-interactive.sh index 482d5c117e..a315ec99a3 100755 --- c/t/t3701-add-interactive.sh +++ w/t/t3701-add-interactive.sh @@ -43,17 +43,17 @@ force_color () { } test_expect_success 'warn about add.interactive.useBuiltin' ' - cat >expect <<-\EOF && + cat >expect.error <<-\EOF && warning: the add.interactive.useBuiltin setting has been removed! See its entry in '\''git help config'\'' for details. - No changes. EOF for v in = =true =false do - git -c "add.interactive.useBuiltin$v" add -p >out 2>actual && - test_must_be_empty out && - test_cmp expect actual || return 1 + git -c "add.interactive.useBuiltin$v" add -p >actual 2>error && + echo "No changes." >expect && + test_cmp expect actual && + test_cmp expect.error error || return 1 done ' @@ -348,13 +348,13 @@ test_expect_success 'different prompts for mode change/deleted' ' test_expect_success 'correct message when there is nothing to do' ' git reset --hard && - git add -p 2>err && - test_grep "No changes" err && + git add -p >out && + test_grep "No changes" out && printf "\\0123" >binary && git add binary && printf "\\0abc" >binary && - git add -p 2>err && - test_grep "Only binary files changed" err + git add -p >out && + test_grep "Only binary files changed" out ' test_expect_success 'setup again' '
Eric Sunshine <sunshine@sunshineco.com> writes: > That was also the adopted solution to the cited similar example[1]; > git-worktree was changed to send all its chatty output to stderr[2], > which was appropriate for that (non-interactive) case. > > [1]: https://lore.kernel.org/git/CAPig+cTGq-10ZTBts2LXRVdPMf2vNMX8HTuhg_+ZHSiLX-brOQ@mail.gmail.com/ > [2]: https://lore.kernel.org/git/20211203034420.47447-2-sunshine@sunshineco.com/ Hmph, what I just sent out tries to send everything to the standard output stream. For an interactive sesion, whether the interaction is done over stdout or stderr does not really make much difference, but I have a slight preference to use the stderr (because I view prompts etc. in the same "chatty" class as the progress bar), but I do not care enough to go back and redo the patch ;-)
On Thu, Apr 25, 2024 at 02:05:33PM -0700, Junio C Hamano wrote: I am assuming that this change will precede my series. I leave some nits below. > diff --git c/add-patch.c w/add-patch.c > index 7be142d448..c28ad380ed 100644 > --- c/add-patch.c > +++ w/add-patch.c > @@ -293,10 +293,10 @@ static void err(struct add_p_state *s, const char *fmt, ...) > va_list args; > > va_start(args, fmt); > - fputs(s->s.error_color, stderr); > - vfprintf(stderr, fmt, args); > - fputs(s->s.reset_color, stderr); > - fputc('\n', stderr); > + fputs(s->s.error_color, stdout); > + vfprintf(stdout, fmt, args); > + fputs(s->s.reset_color, stdout); > + fputc('\n', stdout); If we're going to use stdout, perhaps we can be less explicit? diff --git a/add-patch.c b/add-patch.c index 447e8166d2..a204224dae 100644 --- a/add-patch.c +++ b/add-patch.c @@ -293,10 +293,10 @@ static void err(struct add_p_state *s, const char *fmt, ...) va_list args; va_start(args, fmt); - fputs(s->s.error_color, stderr); - vfprintf(stderr, fmt, args); - fputs(s->s.reset_color, stderr); - fputc('\n', stderr); + puts(s->s.error_color); + vprintf(fmt, args); + puts(s->s.reset_color); + putchar('\n'); va_end(args); } > @@ -1780,9 +1780,9 @@ int run_add_p(struct repository *r, enum add_p_mode mode, > break; > > if (s.file_diff_nr == 0) > - fprintf(stderr, _("No changes.\n")); > + err(&s, _("No changes.")); Nice. > else if (binary_count == s.file_diff_nr) > - fprintf(stderr, _("Only binary files changed.\n")); > + err(&s, _("Only binary files changed.")); Nice. > diff --git c/t/t3701-add-interactive.sh w/t/t3701-add-interactive.sh > index 482d5c117e..a315ec99a3 100755 > --- c/t/t3701-add-interactive.sh > +++ w/t/t3701-add-interactive.sh > @@ -43,17 +43,17 @@ force_color () { > } > > test_expect_success 'warn about add.interactive.useBuiltin' ' > - cat >expect <<-\EOF && > + cat >expect.error <<-\EOF && This expect.error is what we are going to test with the output on stderr ... > warning: the add.interactive.useBuiltin setting has been removed! > See its entry in '\''git help config'\'' for details. > - No changes. however, this line no longer goes to stderr. OK. > EOF > > for v in = =true =false > do > - git -c "add.interactive.useBuiltin$v" add -p >out 2>actual && > - test_must_be_empty out && > - test_cmp expect actual || return 1 > + git -c "add.interactive.useBuiltin$v" add -p >actual 2>error && > + echo "No changes." >expect && Why not prepare this file above, out of the loop? > + test_cmp expect actual && > + test_cmp expect.error error || return 1 > done > ' > > @@ -348,13 +348,13 @@ test_expect_success 'different prompts for mode change/deleted' ' > > test_expect_success 'correct message when there is nothing to do' ' > git reset --hard && > - git add -p 2>err && > - test_grep "No changes" err && > + git add -p >out && > + test_grep "No changes" out && > printf "\\0123" >binary && > git add binary && > printf "\\0abc" >binary && > - git add -p 2>err && > - test_grep "Only binary files changed" err > + git add -p >out && > + test_grep "Only binary files changed" out > ' > > test_expect_success 'setup again' '
Rubén Justo <rjusto@gmail.com> writes: > On Thu, Apr 25, 2024 at 02:05:33PM -0700, Junio C Hamano wrote: > > I am assuming that this change will precede my series. No, this was merely "if we were to update the series I queued to my tree, the squashable fix may look like this", which you can use to update _your_ series if you want to.
> Rubén Justo <rjusto@gmail.com> writes: > > > On Thu, Apr 25, 2024 at 02:05:33PM -0700, Junio C Hamano wrote: > > > > I am assuming that this change will precede my series. > > No, this was merely "if we were to update the series I queued to my > tree, the squashable fix may look like this", which you can use to > update _your_ series if you want to. > I was not sure what was the expectation. In fact, I'm still not quite sure. I am not sure about the change either. The current options are: a.- make the test check for stderr and stdout, separatedly b.- fflush(stdout) in err c.- make err print to stdout I suspect that similar tests for other commands would produce similar errors, so (a) seems like an easy fix but feels like kicking the can forward. I'm not sure of the implications of (c). Perhaps moving current messages to stdout breaks some workflow out there? The other thread about disabling all hints has made me think. The (b) option seems to me the less disturbing change, but it has not attracted attention. There is even a (d) that is to go back to test just the new error message, not the whole output. I will give it some thought.
Rubén Justo <rjusto@gmail.com> writes: > The current options are: > > a.- make the test check for stderr and stdout, separatedly There is a downside that we do not check what the users would see, as I mentioned in an earlier message. > b.- fflush(stdout) in err > > c.- make err print to stdout Yup. The last one is what I showed in the thread. > I suspect that similar tests for other commands would produce similar > errors, so (a) seems like an easy fix but feels like kicking the can > forward. > > I'm not sure of the implications of (c). Perhaps moving current > messages to stdout breaks some workflow out there? The other thread > about disabling all hints has made me think. That depends on "other" commands. As Phillip said, "git add -p" and the like that interacts with the end user via terminal, using a single stream, whether it is the standard error stream or the standard output stream, should not affect any end-user experience. It is very unlikely people capture only one stream and feed it to a program, and with i18n, these messages are not even designed for machine consumption in the first place. > The (b) option seems to me the less disturbing change Sticking close to the status quo would certainly _feel_ the safest at least in the shorter term. But given that there is not really a reasonable justification why some output goes to the standard output stream while others go to the standard error stream in this program (note that I am specifically talking about the terminal interactive session with "add -p"), the approach will force us to worry about similar gotchas and we need to decide which stream the message needs to go every time we add a new one. > but it has not attracted attention. I think the same reasoning from the old thread that made us avoid the "flush() and keep writing to two streams" in the worktree code would apply here, too.
Rubén Justo <rjusto@gmail.com> writes:
> I will give it some thought.
In the meantime, I'll revert the merge of the topic into 'next',
squash in the "send everything to the standard output stream" fix,
and queue the "tentative" result to 'seen', to make the CI happy.
I am OK enough with the "tentative" one, but if you and others feel
differently, I am perfectly happy to see it replaced with [PATCH v5].
Thanks.
On Wed, Apr 24, 2024 at 10:15:25PM -0400, Eric Sunshine wrote: > > I got a test failure on Windows CI from this. The test_cmp output looks > > like this: > > > > -(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command 'W' (use '?' for help) > > -(1/1) Stage addition [y,n,q,a,d,e,p,?]? > > +(1/1) Stage addition [y,n,q,a,d,e,p,?]? (1/1) Stage addition [y,n,q,a,d,e,p,?]? > > +Unknown command 'W' (use '?' for help) > > > > which makes me suspect a race. Perhaps because the prompt is going to > > stdout, but the "Unknown command" message goes to stderr? Maybe we > > should keep stdout and stderr separate and check them independently. > > That's very reminiscent of [1]. Although, unlike [1], the output > presented to the user in this case is (I suppose) less likely to be > messed up; only the combined captured output is probably affected. So, > capturing stdout and stderr separately would indeed be a good idea. Ah, yeah, that is almost certainly the same issue. I could not reproduce it on my local system with --stress, so I think it may be unique to Windows. What it looks like is that stderr is buffered when output to a file (whereas on most systems it would still be line buffered at most). But I didn't actually run it through a debugger to see. So I _suspect_ that yet another possible solution here is to do an explicit: setvbuf(stderr, NULL, _IOLBF, BUFSIZ); in common-main. Though grepping for setvbuf() in our code base, it looks like compat/winansi.c already does: if (fd == 2) setvbuf(stderr, NULL, _IONBF, BUFSIZ); which would imply that output should happen even more immediately than line-buffering. So maybe there is some other layer at work. There are probably dragons, and definitely pursuing that line would involve some experimentation. The "if you want them in order, make sure they are on the same stream" approach suggested elsewhere is by comparison much simpler. :) -Peff
On Thu, Apr 25, 2024 at 05:04:45AM +0200, Rubén Justo wrote: > > The test_cmp output looks > > like this: > > > > -(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command 'W' (use '?' for help) > > -(1/1) Stage addition [y,n,q,a,d,e,p,?]? > > +(1/1) Stage addition [y,n,q,a,d,e,p,?]? (1/1) Stage addition [y,n,q,a,d,e,p,?]? > > +Unknown command 'W' (use '?' for help) > > > > which makes me suspect a race. Perhaps because the prompt is going to > > stdout, but the "Unknown command" message goes to stderr? > > I have to read the thread pointed by Eric, but my knee-jerk reaction has > been to think in something like: > > diff --git a/add-patch.c b/add-patch.c > index 447e8166d2..0090543f89 100644 > --- a/add-patch.c > +++ b/add-patch.c > @@ -292,6 +292,7 @@ static void err(struct add_p_state *s, const char *fmt, ...) > { > va_list args; > > va_start(args, fmt); > + fflush(stdout); > fputs(s->s.error_color, stderr); > vfprintf(stderr, fmt, args); I think the "just send it all to stdout" approach makes the most sense here, but in case we don't do that: I don't think this will fix it. In the output above it is the "Unknown command" output which is delayed, which is sent to stderr via err(). So flushing stdout again won't help. Flushing stderr after the vfprintf _might_ help (though I'm confused why stderr would be fully buffered in the first place). -Peff
On Fri, Apr 26, 2024 at 04:23:33PM -0400, Jeff King wrote: > On Thu, Apr 25, 2024 at 05:04:45AM +0200, Rubén Justo wrote: > > > > The test_cmp output looks > > > like this: > > > > > > -(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command 'W' (use '?' for help) > > > -(1/1) Stage addition [y,n,q,a,d,e,p,?]? > > > +(1/1) Stage addition [y,n,q,a,d,e,p,?]? (1/1) Stage addition [y,n,q,a,d,e,p,?]? > > > +Unknown command 'W' (use '?' for help) > > > > > > which makes me suspect a race. Perhaps because the prompt is going to > > > stdout, but the "Unknown command" message goes to stderr? > > > > I have to read the thread pointed by Eric, but my knee-jerk reaction has > > been to think in something like: > > > > diff --git a/add-patch.c b/add-patch.c > > index 447e8166d2..0090543f89 100644 > > --- a/add-patch.c > > +++ b/add-patch.c > > @@ -292,6 +292,7 @@ static void err(struct add_p_state *s, const char *fmt, ...) > > { > > va_list args; > > > > va_start(args, fmt); > > + fflush(stdout); > > fputs(s->s.error_color, stderr); > > vfprintf(stderr, fmt, args); > > I think the "just send it all to stdout" approach makes the most sense > here, but in case we don't do that: I don't think this will fix it. In > the output above it is the "Unknown command" output which is delayed, > which is sent to stderr via err(). Very true. I'm happy with what Junio has just queued. I do not plan to send a new iteration, unless a new test break appears :-) Thanks, all.
Hi Junio On 24/04/2024 16:35, Junio C Hamano wrote: > phillip.wood123@gmail.com writes: > >> On 22/04/2024 16:50, Junio C Hamano wrote: >>> Rubén Justo <rjusto@gmail.com> writes: >>> >>>> 1: 0317594bce ! 1: b418b03f15 add-patch: response to unknown command >>>> @@ t/t3701-add-interactive.sh: test_expect_success 'warn about add.interactive.useB >>>> + test_when_finished "git reset --hard; rm -f command" && >>>> + echo W >command && >>>> + git add -N command && >>>> -+ cat >expect <<-EOF && >>>> -+ diff --git a/command b/command >>>> -+ new file mode 100644 >>>> -+ index 0000000..a42d8ff >>>> -+ --- /dev/null >>>> -+ +++ b/command >>>> -+ @@ -0,0 +1 @@ >>>> -+ +W >>>> ++ git diff command >expect && >>>> ++ cat >>expect <<-EOF && >>>> + (1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help) >>>> + (1/1) Stage addition [y,n,q,a,d,e,p,?]?$SP >>>> + EOF >>> Interesting. >>> My first reaction was "how is this different from checking just the >>> last line of the actual output? The early part of expect and actual >>> both come from an internal invocation of 'git diff', and they must >>> match by definition". >>> But that may really be the point of this test. >> >> Yes - we want to make sure that we are not printing the help and the >> only way to do that is to check the whole output > > I was not questioning that part of the patch. "My first reaction" > was solely about use of "git diff" to replace the golden copy of > expected result in the test itself, only to allow for use of > different hash functions. As you (or somebody else?) mentioned in > an earlier review, diff_cmp is there for exactly that purpose. Oh sorry I'd misunderstood >>> That is, we may later decide to tweak the way "git diff" hunks are >>> presented, and we expect that the way "git add -p" presents the >>> hunks would change together with it automatically. > > This argument cuts both ways, by the way. > > Insisting that the output match the explicit expectation (not what > "git diff" of the day produces) has a few advantages. It makes the > test more explicit and easy to see what output we are expecting, and > more importantly, it forces us to update the test when we decide to > tweak the output from the command being tested (i.e. hunk selection > UI) and/or the output from "git diff" command. There is also a practical argument against using "git diff" to generate the expected output as it only works if the diff contains a single hunk. If the diff contains more than one hunk "add -p" displays them separately. Best Wishes Phillip
Phillip Wood <phillip.wood123@gmail.com> writes: > There is also a practical argument against using "git diff" to > generate the expected output as it only works if the diff contains a > single hunk. If the diff contains more than one hunk "add -p" displays > them separately. True, but spliting the output from "git diff" at @@ hunk boundary lines would allow you to hardcode the assumption that "add -p" shows each hunk exactly as "git diff" would. So if we really wanted to have the convenience that the tests' expectation automatically follow what "git diff" of the day happens to produce, it is still a viable approach to use "git diff" output with some post-processing. Thanks.
diff --git a/add-patch.c b/add-patch.c index a06dd18985..7be142d448 100644 --- a/add-patch.c +++ b/add-patch.c @@ -1667,7 +1667,7 @@ static int patch_update_file(struct add_p_state *s, } } else if (s->answer.buf[0] == 'p') { rendered_hunk_index = -1; - } else { + } else if (s->answer.buf[0] == '?') { const char *p = _(help_patch_remainder), *eol = p; color_fprintf(stdout, s->s.help_color, "%s", @@ -1691,6 +1691,9 @@ static int patch_update_file(struct add_p_state *s, color_fprintf_ln(stdout, s->s.help_color, "%.*s", (int)(eol - p), p); } + } else { + err(s, _("Unknown command '%s' (use '?' for help)"), + s->answer.buf); } } diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index bc55255b0a..482d5c117e 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -7,6 +7,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME . ./test-lib.sh . "$TEST_DIRECTORY"/lib-terminal.sh +SP=" " + diff_cmp () { for x do @@ -55,6 +57,19 @@ test_expect_success 'warn about add.interactive.useBuiltin' ' done ' +test_expect_success 'unknown command' ' + test_when_finished "git reset --hard; rm -f command" && + echo W >command && + git add -N command && + git diff command >expect && + cat >>expect <<-EOF && + (1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help) + (1/1) Stage addition [y,n,q,a,d,e,p,?]?$SP + EOF + git add -p -- command <command >actual 2>&1 && + test_cmp expect actual +' + test_expect_success 'setup (initial)' ' echo content >file && git add file && @@ -231,7 +246,6 @@ test_expect_success 'setup file' ' ' test_expect_success 'setup patch' ' - SP=" " && NULL="" && cat >patch <<-EOF @@ -1,4 +1,4 @@
When the user gives an unknown command to the "add -p" prompt, the list of accepted commands with their explanation is given. This is the same output they get when they say '?'. However, the unknown command may be due to a user input error rather than the user not knowing the valid command. To reduce the likelihood of user confusion and error repetition, instead of displaying the list of accepted commands, display a short error message with the unknown command received, as feedback to the user. Include a reminder about the current command '?' in the new message, to guide the user if they want help. Signed-off-by: Rubén Justo <rjusto@gmail.com> --- The test now pass with GIT_TEST_DEFAULT_HASH=sha256. Thanks. Range-diff against v3: 1: 0317594bce ! 1: b418b03f15 add-patch: response to unknown command @@ t/t3701-add-interactive.sh: test_expect_success 'warn about add.interactive.useB + test_when_finished "git reset --hard; rm -f command" && + echo W >command && + git add -N command && -+ cat >expect <<-EOF && -+ diff --git a/command b/command -+ new file mode 100644 -+ index 0000000..a42d8ff -+ --- /dev/null -+ +++ b/command -+ @@ -0,0 +1 @@ -+ +W ++ git diff command >expect && ++ cat >>expect <<-EOF && + (1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help) + (1/1) Stage addition [y,n,q,a,d,e,p,?]?$SP + EOF add-patch.c | 5 ++++- t/t3701-add-interactive.sh | 16 +++++++++++++++- 2 files changed, 19 insertions(+), 2 deletions(-)