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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 17, 2024, 11:22 UTC
Message-ID: <xmqq34n3jswh.fsf@gitster.g>
In-Reply-To: <be3c968b0d9085843cd9ce67e85aadfaaafa69c8.1723848510.git.matheus.tavb@gmail.com>

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

Show 12 quoted lines

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

Show 7 quoted lines

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

Show 14 quoted lines

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