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

[Matheus Tavares Bernardino <matheus.tavb@gmail.com>]

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.