Re: [RFC PATCH 1/3] git-rebase, sequencer: add a --quiet option for the interactive machinery

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Elijah,

On Thu, 7 Jun 2018, Elijah Newren wrote:

> While 'quiet' and 'interactive' may sound like antonyms, the interactive
> machinery actually has logic that implements several
> interactive_rebase=implied cases (--exec, --keep-empty, --rebase-merges)
> which won't pop up an editor.  Further, we want to make the interactive
> machinery also take over for git-rebase--merge and become the default
> merge strategy, so it makes sense for these other cases to have a quiet
> option.
> 
> git-rebase--interactive was already somewhat quieter than
> git-rebase--merge and git-rebase--am, possibly because cherry-pick has
> just traditionally been quieter.  As such, we only drop a few
> informational messages -- "Rebasing (n/m)" and "Succesfully rebased..."

Makes sense. As long as it is coordinated with Alban and Pratik, as both
of their GSoC projects are affected by this.

In particular Pratik's project, I think, would actually *benefit* from
your work, as it might even make it possible to turn all modes but
--preserve-merges into pure builtin code, which would be awesome.

> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 06a7b79307..1f2401f702 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -139,8 +139,12 @@ mark_action_done () {
>  	if test "$last_count" != "$new_count"
>  	then
>  		last_count=$new_count
> -		eval_gettext "Rebasing (\$new_count/\$total)"; printf "\r"
> -		test -z "$verbose" || echo
> +		if test -z "$GIT_QUIET"
> +		then
> +			eval_gettext "Rebasing (\$new_count/\$total)";
> +			printf "\r"
> +			test -z "$verbose" || echo
> +		fi
>  	fi
>  }
>  
> @@ -713,6 +717,7 @@ Commit or stash your changes, and then run
>  		"$hook" rebase < "$rewritten_list"
>  		true # we don't care if this hook failed
>  	fi &&
> +		test -z "$GIT_QUIET" &&
>  		warn "$(eval_gettext "Successfully rebased and updated \$head_name.")"

In general, I tried the statements to return success at all times. That
means that

		test -n "$GIT_QUIET" ||

would be better in this case.

> diff --git a/git-rebase.sh b/git-rebase.sh
> index 7d1612b31b..b639c0d4fe 100755
> --- a/git-rebase.sh
> +++ b/git-rebase.sh
> @@ -136,7 +136,7 @@ write_basic_state () {
>  	echo "$head_name" > "$state_dir"/head-name &&
>  	echo "$onto" > "$state_dir"/onto &&
>  	echo "$orig_head" > "$state_dir"/orig-head &&
> -	echo "$GIT_QUIET" > "$state_dir"/quiet &&
> +	test t = "$GIT_QUIET" && : > "$state_dir"/quiet

Maybe it would be better to `echo t` into that file? That way, scripts
that used the value in that file would continue to work. (But maybe there
were no scripts that could use it, as only the interactive rebase allows
scripting, and it did not handle that flag before?)

The rest looks obviously good.

Thank you!
Dscho