Re: [PATCH v2] rebase -x: don't print "Executing:" msgs with --quiet

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 19, 2024, 15:41 UTC
Message-ID: <xmqqwmkcfrk6.fsf@gitster.g>
In-Reply-To: <CAGdrTFhZ6KeDPDUoCsV3h5myPuoYf7RR8eFdbFFXGrUGCdEkEw@mail.gmail.com>

Matheus Tavares Bernardino <matheus.tavb@gmail.com> writes:

Show 11 quoted lines

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

Show 9 quoted lines

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