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

[Junio C Hamano <gitster@pobox.com>]

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.