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

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

[Matheus Tavares]

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

diff --git a/sequencer.c b/sequencer.c
index 0291920f0b..d5824b41c1 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3793,12 +3793,14 @@ 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");
@@ -5013,7 +5015,7 @@ static int pick_commits(struct repository *r,
 			if (!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) {
-- 
2.46.0

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

[Elijah Newren]

On Thu, Aug 15, 2024 at 8:26 PM Matheus Tavares <matheus.tavb@gmail.com> wrote:
>
> `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(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 0291920f0b..d5824b41c1 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -3793,12 +3793,14 @@ 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");
> @@ -5013,7 +5015,7 @@ static int pick_commits(struct repository *r,
>                         if (!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) {
> --
> 2.46.0

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.

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

[Patrick Steinhardt]

On Fri, Aug 16, 2024 at 12:26:19AM -0300, Matheus Tavares wrote:
> `rebase --exec` doesn't obey --quiet and end up printing a few messages

s/end/ends/

> about the cmd being executed:

s/cmd/command/

>   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(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index 0291920f0b..d5824b41c1 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -3793,12 +3793,14 @@ 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);
> +	}

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

>  	cmd.use_shell = 1;
>  	strvec_push(&cmd.args, command_line);
>  	strvec_push(&cmd.env, "GIT_CHERRY_PICK_HELP");
> @@ -5013,7 +5015,7 @@ static int pick_commits(struct repository *r,
>  			if (!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) {

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

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

Patrick

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

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

>>  			if (res) {
>
> Do we also want to add a test for this fix?
>
> Other than those nits the fix looks obviously correct to me, thanks!

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

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

[Matheus Tavares]

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

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

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

[Junio C Hamano]

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.

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

[Matheus Tavares Bernardino]

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.

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

[Phillip Wood]

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

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

[Junio C Hamano]

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.

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

[Matheus Tavares Bernardino]

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

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

[Junio C Hamano]

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.

[PATCH v3] rebase --exec: respect --quiet

[Matheus Tavares]

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

diff --git a/pager.c b/pager.c
index 896f40fcd2..9c24ce6263 100644
--- a/pager.c
+++ b/pager.c
@@ -234,6 +234,8 @@ int term_columns(void)
  */
 void term_clear_line(void)
 {
+	if (!isatty(2))
+		return;
 	if (is_terminal_dumb())
 		/*
 		 * Fall back to print a terminal width worth of space
diff --git a/sequencer.c b/sequencer.c
index 0291920f0b..65c485d783 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");
@@ -5013,7 +5014,7 @@ static int pick_commits(struct repository *r,
 			if (!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..bd8bcc381a 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -235,6 +235,12 @@ 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" &&
-- 
2.46.0

Re: [PATCH v3] rebase --exec: respect --quiet

[Junio C Hamano]

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

> rebase --exec doesn't obey --quiet and ends up printing messages about
> the command being executed:
> ...
> 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.

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.