diff mbox series

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

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

Commit Message

Matheus Tavares Aug. 16, 2024, 10:48 p.m. UTC
`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(-)

Comments

Junio C Hamano Aug. 17, 2024, 11:22 a.m. UTC | #1
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.
Matheus Tavares Aug. 18, 2024, 1:03 p.m. UTC | #2
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.
Phillip Wood Aug. 19, 2024, 1:57 p.m. UTC | #3
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
Junio C Hamano Aug. 19, 2024, 3:41 p.m. UTC | #4
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.
Junio C Hamano Aug. 19, 2024, 8:17 p.m. UTC | #5
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.
Matheus Tavares Aug. 20, 2024, 10:23 p.m. UTC | #6
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 mbox series

Patch

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" &&