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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jun 9, 2018, 21:45 UTC
Message-ID: <nycvar.QRO.7.76.6.1806092336320.77@tvgsbejvaqbjf.bet>
In-Reply-To: <20180607171344.23331-2-newren@gmail.com>

Hi Elijah,

On Thu, 7 Jun 2018, Elijah Newren wrote:

Show 12 quoted lines

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

Show 25 quoted lines

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

Show 10 quoted lines

> 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