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

Commit Message

Matheus Tavares Aug. 21, 2024, 1:31 a.m. UTC

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 <[e-mail]>
Reported-by: Rodrigo Siqueira <[e-mail]>
Signed-off-by: Matheus Tavares <[e-mail]>
---

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

Comments

Junio C Hamano Aug. 21, 2024, 4 p.m. UTC | #1

Matheus Tavares <[e-mail]> 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.

Patch

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