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

[Junio C Hamano <gitster@pobox.com>]

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.