patchrebase -x: don't print "Executing:" msgs with --quiet

`rebase --exec` doesn't obey --quiet and end up printing a few messages about the cmd 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.

Suggested-by: Rodrigo Siqueira <siqueirajordao@riseup.net>
Signed-off-by: Matheus Tavares <matheus.tavb@gmail.com>
---
 sequencer.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

On Thu, Aug 15, 2024 at 8:26 PM Matheus Tavares <matheus.tavb@gmail.com> wrote:

Makes sense and looks good to me. It's kind surprising just how many places we've ignored --quiet over the years...anyway, thanks for fixing another one of them.

On Fri, Aug 16, 2024 at 12:26:19AM -0300, Matheus Tavares wrote:

s/end/ends/

s/cmd/command/

We don't typically use braces around single-line statements, so they should be removed here.

Do we also want to add a test for this fix?

Other than those nits the fix looks obviously correct to me, thanks!

Patrick

Patrick Steinhardt <ps@pks.im> writes:

Thanks for a review. I agree that this is almost there but would want a test.

`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(-)

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

This is very much understandable and match what the proposed log message explained.

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

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

Thanks.

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?

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

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

Phillip Wood <phillip.wood123@gmail.com> writes:

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:

Oh, good point. So `isatty(2)`, actually.

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

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

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.

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.

rebase --exec doesn't obey --quiet and ends up printing messages about the command being executed:

  git rebase HEAD~3 --quiet --exec true
  Executing: true
  Executing: true
  Executing: true

Let's fix that by omitting the "Executing" messages when using --quiet.

Furthermore, the sequencer code includes a few calls to term_clear_line(), which prints a special character sequence to erase the previous line displayed on stderr (even when nothing was printed yet). For an user running the command interactively, the net effect of calling this function with or without --quiet is the same as the characters are invisible in the terminal. However, when redirecting the output to a file or piping to another command, the presence of these invisible characters is noticeable, and it may break user expectation as --quiet is not being respected.

We could skip the term_clear_line() calls when --quiet is used, like we are doing with the "Executing" messages, but it makes much more sense to condition the line cleaning upon stderr being TTY, since these characters are really only useful for TTY outputs.

The added test checks for both these two changes.

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 v3:
- Skipped term_clean_line() when stderr is not a TTY.
- Removed the !opts->quiet condition when calling term_clean_line().
- Reworded commit message to better explain the proposed changes.

 pager.c           | 2 ++
 sequencer.c       | 7 ++++---
 t/t3400-rebase.sh | 6 ++++++
 3 files changed, 12 insertions(+), 3 deletions(-)

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

The updated title and log message, and the new approach to squelch term_clear_line() when it is not needed. Both look quite sensible.

Will replace.  Thanks.