| Message ID | be3c968b0d9085843cd9ce67e85aadfaaafa69c8.1723848510.git.matheus.tavb@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] rebase -x: don't print "Executing:" msgs with --quiet | expand |
Matheus Tavares <matheus.tavb@gmail.com> writes: > `rebase --exec` doesn't obey --quiet and ends up printing a few messages > about the command being executed: > ... > -static int do_exec(struct repository *r, const char *command_line) > +static int do_exec(struct repository *r, const char *command_line, int quiet) > { > struct child_process cmd = CHILD_PROCESS_INIT; > int dirty, status; > > - fprintf(stderr, _("Executing: %s\n"), command_line); > + if (!quiet) > + fprintf(stderr, _("Executing: %s\n"), command_line); This is very much understandable and match what the proposed log message explained. > @@ -4902,7 +4903,7 @@ static int pick_one_commit(struct repository *r, > if (item->command == TODO_EDIT) { > struct commit *commit = item->commit; > if (!res) { > - if (!opts->verbose) > + if (!opts->quiet && !opts->verbose) > term_clear_line(); This is not, though. The original says "if not verbose, clear the line", so presumably calling the term_clear_line() makes it _less_ verbose. The reasoning needs to be explained. I actually would have expected that this message ... > fprintf(stderr, _("Stopped at %s... %.*s\n"), > short_commit_name(r, commit), item->arg_len, arg); ... goes away when opts->quiet is in effect ;-). Another thing, if _all_ calls to term_clear_line() is done under the same "not quiet, and not verbose" condition, perhaps it is easier to follow the resulting code if a helper function that takes a single argument, opts, and does eomthing like: static void helper(struct replay_opts *opts) { /* * explain why we shouldn't call term_clear_line() * under opts->quiet or opts->verbose here. */ if (opts->quiet || opts->verbose) return; term_clear_line(); } Once we understand why it makes sense to treat quiet and verbose the same way with repect to clearing the line, we can properly fill the "explain" above, and give an intuitive name to the helper, which will help readers understand the callers, too. > diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh > index ae34bfad60..15b3228c6e 100755 > --- a/t/t3400-rebase.sh > +++ b/t/t3400-rebase.sh > @@ -235,6 +235,13 @@ test_expect_success 'rebase --merge -q is quiet' ' > test_must_be_empty output.out > ' > > +test_expect_success 'rebase --exec -q is quiet' ' > + git checkout -B quiet topic && > + git rebase --exec true -q main >output.out 2>&1 && > + test_must_be_empty output.out > + > +' Thanks.
On Sat, Aug 17, 2024 at 8:22 AM Junio C Hamano <gitster@pobox.com> wrote: > > Matheus Tavares <matheus.tavb@gmail.com> writes: > > > > > - fprintf(stderr, _("Executing: %s\n"), command_line); > > + if (!quiet) > > + fprintf(stderr, _("Executing: %s\n"), command_line); > > This is very much understandable and match what the proposed log > message explained. > > > @@ -4902,7 +4903,7 @@ static int pick_one_commit(struct repository *r, > > if (item->command == TODO_EDIT) { > > struct commit *commit = item->commit; > > if (!res) { > > - if (!opts->verbose) > > + if (!opts->quiet && !opts->verbose) > > term_clear_line(); > > This is not, though. The original says "if not verbose, clear the > line", so presumably calling the term_clear_line() makes it _less_ > verbose. The reasoning needs to be explained. The idea is that, when running in --quiet mode, we don't want to print anything, not even a line-cleaning char sequence. Nonetheless, since these are invisible chars (assuming we haven't printed anything to be "cleaned" before them), printing them doesn't actually make a difference to the user running rebase in the terminal, as they won't see the chars anyways. The actual issue is when piping/redirecting the rebase output, which will include these invisible chars... So perhaps, instead of modifying the sequencer.c to use "if (!opts->quiet && !opts->verbose) term_clean_line()", the correct approach would be to modify "term_clean_line()" to return earlier "if (!isatty(1))". What do you think? > I actually would have expected that this message ... > > > fprintf(stderr, _("Stopped at %s... %.*s\n"), > > short_commit_name(r, commit), item->arg_len, arg); > > ... goes away when opts->quiet is in effect ;-). Sure, I can add that :) I was mostly focused on the "Executing ..." lines, so that's why I haven't seen/touched this one.
Hi Matheus On 18/08/2024 14:03, Matheus Tavares Bernardino wrote: > On Sat, Aug 17, 2024 at 8:22 AM Junio C Hamano <gitster@pobox.com> wrote: > The idea is that, when running in --quiet mode, we don't want to print > anything, not even a line-cleaning char sequence. > > Nonetheless, since these are invisible chars (assuming we haven't > printed anything to be "cleaned" before them), printing them doesn't > actually make a difference to the user running rebase in the terminal, > as they won't see the chars anyways. > > The actual issue is when piping/redirecting the rebase output, which > will include these invisible chars... So perhaps, instead of modifying > the sequencer.c to use "if (!opts->quiet && !opts->verbose) > term_clean_line()", the correct approach would be to modify > "term_clean_line()" to return earlier "if (!isatty(1))". What do you > think? On the face of it that sounds like a good idea but I haven't thought too much about it. These messages are all going to stderr rather than stdout. If we do go that way we'll need to adjust launch_specified_editor() in editor.c to either suppress the hint or terminate it with '\n' if stderr is not a terminal. >> I actually would have expected that this message ... >> >>> fprintf(stderr, _("Stopped at %s... %.*s\n"), >>> short_commit_name(r, commit), item->arg_len, arg); >> >> ... goes away when opts->quiet is in effect ;-). > > Sure, I can add that :) I was mostly focused on the "Executing ..." > lines, so that's why I haven't seen/touched this one. If we're going to suppress this we should probably suppress the message about amending the commit that gets printed after this by error_with_patch(). There are a number of other places that we ignore "--quiet". stopped_at_head() prints a similar message to the one above when we stop for a "break" command and currently ignores "--quiet". Should the messages from "--autostash" be suppressed by "--quiet"? What about when a commit is dropped because it is has become empty in do_pick_commit()? Thanks for working on this, it would be nice to have the sequencer respect "--quiet" better. Phillip
Matheus Tavares Bernardino <matheus.tavb@gmail.com> writes: > Nonetheless, since these are invisible chars (assuming we haven't > printed anything to be "cleaned" before them), printing them doesn't > actually make a difference to the user running rebase in the terminal, > as they won't see the chars anyways. > > The actual issue is when piping/redirecting the rebase output, which > will include these invisible chars... So perhaps, instead of modifying > the sequencer.c to use "if (!opts->quiet && !opts->verbose) > term_clean_line()", the correct approach would be to modify > "term_clean_line()" to return earlier "if (!isatty(1))". What do you > think? So, term_clear_line() assumes that there were something already on the line, goes back to the beginning of the line and then makes what was on the line invisible, either by overwriting them with enough spaces or with "clear to the end of line" sequence, and then go back to the beginning of the line. None of that really makes much sense if the output is not going to the human user sitting in front of the terminal, so isatty(1) (or isatty(2)[*]) based guard does sound like the right thing to do. I certainly would have suggested us do so if we were inventing this code anew today, and offhand my gut feeling is that it is unlikely if such a behaviour change causes breakage of any existing scripted use. But people do "interesting" things, and because there are sufficiently large number of Git users, I would not be totally surprised if there are people who "double check" by, say, counting "Rebasing" and "Executing" and making sure these match what they fed in the todo file---their use case will certainly be broken. >> I actually would have expected that this message ... >> >> > fprintf(stderr, _("Stopped at %s... %.*s\n"), >> > short_commit_name(r, commit), item->arg_len, arg); >> >> ... goes away when opts->quiet is in effect ;-). > > Sure, I can add that :) I was mostly focused on the "Executing ..." > lines, so that's why I haven't seen/touched this one. It would make the user experience horrible if we removed this "Stopped at", especially with the "Rebasing..." indicator that is given at each step squelched with the "opts->quiet" flag, because the user would totally really lose where they are if we did't give this message. As it is the norm for sequencer operations to advance without human intervention, stopping at somewhere ought to be given a bit more special status and deserves to be marked as such. With the same yardstick, removing "Executing:" message while running under the --quiet option, when these "exec" insn were automatically inserted via "rebase -x", does make sense, because it is just "a stream of insns given in the todo file, we execute one step at a time, and we stay quiet unless some exceptional thing happens". Because we give a warning if the execution fails or the execution leaves the working tree dirty, and we include what command we attempted to run with the "exec" insn, it is unlikely that users will lose their place and get confused. If a user of "rebase -i" inserted an "exec" insn at a selected place in the todo file, the above argument to sequelch "Executing" becomes a bit weaker, but I think it still is OK.
Phillip Wood <phillip.wood123@gmail.com> writes: > On 18/08/2024 14:03, Matheus Tavares Bernardino wrote: >> ... >> term_clean_line()", the correct approach would be to modify >> "term_clean_line()" to return earlier "if (!isatty(1))". What do you >> think? > > On the face of it that sounds like a good idea but I haven't thought > too much about it. These messages are all going to stderr rather than > stdout. If we do go that way we'll need to adjust > launch_specified_editor() in editor.c to either suppress the hint or > terminate it with '\n' if stderr is not a terminal. Right. The true reason why I brought it up was because (1) it looked really funny to avoid doing that term_clean_line() under "--verbose" as well as under "--quiet" and the code should explain what reasoning backs such decision but it did not, and (2) that unexplained funny pattern repeated, which probably was a sign that it needed to become a small helper function with descriptive name to encapsulate the logic to decide when to call and when not to call the clean-line, which as a bonus would give a central place for us to explain the reason behind not cleaning the line under "--verbose" and the same for "--quiet" (as I suspect that these two want to omit the call for different reasons). Thanks.
On Mon, Aug 19, 2024 at 10:57 AM Phillip Wood <phillip.wood123@gmail.com> wrote: > > Hi Matheus > > On 18/08/2024 14:03, Matheus Tavares Bernardino wrote: > > On Sat, Aug 17, 2024 at 8:22 AM Junio C Hamano <gitster@pobox.com> wrote: > > The idea is that, when running in --quiet mode, we don't want to print > > anything, not even a line-cleaning char sequence. > > > > Nonetheless, since these are invisible chars (assuming we haven't > > printed anything to be "cleaned" before them), printing them doesn't > > actually make a difference to the user running rebase in the terminal, > > as they won't see the chars anyways. > > > > The actual issue is when piping/redirecting the rebase output, which > > will include these invisible chars... So perhaps, instead of modifying > > the sequencer.c to use "if (!opts->quiet && !opts->verbose) > > term_clean_line()", the correct approach would be to modify > > "term_clean_line()" to return earlier "if (!isatty(1))". What do you > > think? > > On the face of it that sounds like a good idea but I haven't thought too > much about it. These messages are all going to stderr rather than > stdout. Oh, good point. So `isatty(2)`, actually. > If we do go that way we'll need to adjust > launch_specified_editor() in editor.c to either suppress the hint or > terminate it with '\n' if stderr is not a terminal. Hmm, isn't that what we do already? The hint printing is conditional on `print_waiting_for_editor` which, in turn, is conditional on `isatty(2)`.
diff --git a/sequencer.c b/sequencer.c index 0291920f0b..79d577e676 100644 --- a/sequencer.c +++ b/sequencer.c @@ -3793,12 +3793,13 @@ static int error_failed_squash(struct repository *r, return error_with_patch(r, commit, subject, subject_len, opts, 1, 0); } -static int do_exec(struct repository *r, const char *command_line) +static int do_exec(struct repository *r, const char *command_line, int quiet) { struct child_process cmd = CHILD_PROCESS_INIT; int dirty, status; - fprintf(stderr, _("Executing: %s\n"), command_line); + if (!quiet) + fprintf(stderr, _("Executing: %s\n"), command_line); cmd.use_shell = 1; strvec_push(&cmd.args, command_line); strvec_push(&cmd.env, "GIT_CHERRY_PICK_HELP"); @@ -4902,7 +4903,7 @@ static int pick_one_commit(struct repository *r, if (item->command == TODO_EDIT) { struct commit *commit = item->commit; if (!res) { - if (!opts->verbose) + if (!opts->quiet && !opts->verbose) term_clear_line(); fprintf(stderr, _("Stopped at %s... %.*s\n"), short_commit_name(r, commit), item->arg_len, arg); @@ -4994,7 +4995,7 @@ static int pick_commits(struct repository *r, NULL, REF_NO_DEREF); if (item->command == TODO_BREAK) { - if (!opts->verbose) + if (!opts->quiet && !opts->verbose) term_clear_line(); return stopped_at_head(r); } @@ -5010,10 +5011,10 @@ static int pick_commits(struct repository *r, char *end_of_arg = (char *)(arg + item->arg_len); int saved = *end_of_arg; - if (!opts->verbose) + if (!opts->quiet && !opts->verbose) term_clear_line(); *end_of_arg = '\0'; - res = do_exec(r, arg); + res = do_exec(r, arg, opts->quiet); *end_of_arg = saved; if (res) { diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh index ae34bfad60..15b3228c6e 100755 --- a/t/t3400-rebase.sh +++ b/t/t3400-rebase.sh @@ -235,6 +235,13 @@ test_expect_success 'rebase --merge -q is quiet' ' test_must_be_empty output.out ' +test_expect_success 'rebase --exec -q is quiet' ' + git checkout -B quiet topic && + git rebase --exec true -q main >output.out 2>&1 && + test_must_be_empty output.out + +' + test_expect_success 'Rebase a commit that sprinkles CRs in' ' ( echo "One" &&
`rebase --exec` doesn't obey --quiet and ends up printing a few messages about the command being executed: git rebase HEAD~3 --quiet --exec "printf foo >/dev/null" Executing: printf foo >/dev/null Executing: printf foo >/dev/null Executing: printf foo >/dev/null Let's fix that. Reported-by: Lincoln Yuji <lincolnyuji@hotmail.com> Reported-by: Rodrigo Siqueira <siqueirajordao@riseup.net> Signed-off-by: Matheus Tavares <matheus.tavb@gmail.com> --- Changes in v2: - Applied commit message fixes by Patrick. - Fixed codestyle. - Added regression test. - Also checked "!opt->quiet" before calling term_clear_line() (this would only print whitspaces, so no direct impact for users, but the bytes are still there when the output is captured by scripts, like the test script :) - Added Lincoln as one of the reporters. sequencer.c | 13 +++++++------ t/t3400-rebase.sh | 7 +++++++ 2 files changed, 14 insertions(+), 6 deletions(-)