Re: [RFC PATCH 1/7] rebase -i: Make option handling in pick_one more flexible

[Michael Haggerty <mhagger@alum.mit.edu>]

On 06/19/2014 05:28 AM, Fabian Ruch wrote:
> `pick_one` and `pick_one_preserving_merges` are wrappers around
> `cherry-pick` in `rebase --interactive`. They take the hash of a commit
> and build a `cherry-pick` command line that
> 
>  - respects the user-supplied merge options
>  - disables complaints about empty commits
>  - tries to fast-forward the rebase head unless rebase is forced
>  - suppresses output unless the user requested higher verbosity
>  - rewrites merge commits to point to their rebased parents.
> 
> `pick_one` is used to implement not only `pick` but also `squash`, which
> amends the previous commit rather than creating a new one. When
> `pick_one` is called from `squash`, it receives a second argument `-n`.
> This tells `pick_one` to apply the changes to the index without
> committing them. Since the argument is almost directly passed to
> `cherry-pick`, we might want to do the same with other `cherry-pick`
> options. Currently, `pick_one` expects `-n` to be the first and only
> argument except for the commit hash.
> 
> Prepare `pick_one` for additional `cherry-pick` options by allowing `-n`
> to appear anywhere before the commit hash in the argument list. Loop
> over the argument list and pop each handled item until the commit hash
> is the only parameter left on the list. If an option is not supported,
> ignore it and issue a warning on the console. Construct a new arguments
> list `extra_args` of recognized options that shall be passed to
> `cherry-pick` on the command line.
> 
> Signed-off-by: Fabian Ruch <bafain@gmail.com>
> ---
>  git-rebase--interactive.sh | 61 ++++++++++++++++++++++++++++++++++------------
>  1 file changed, 45 insertions(+), 16 deletions(-)
> 
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index f267d8b..ea5514e 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -237,8 +237,26 @@ git_sequence_editor () {
>  
>  pick_one () {
>  	ff=--ff
> +	extra_args=
> +	while test $# -gt 0
> +	do
> +		case "$1" in
> +		-n)
> +			ff=
> +			extra_args="$extra_args -n"
> +			;;
> +		-*)
> +			warn "pick_one: ignored option -- $1"
> +			;;

This is an internal interface, right?  I.e., user input isn't being
processed here?  If so, then the presence of an unrecognized option is a
bug and it is preferable to "die" here rather than "warn".

The same below and in at least one later commit.

> +		*)
> +			break
> +			;;
> +		esac
> +		shift
> +	done
> +	test $# -ne 1 && die "pick_one: wrong number of arguments"
> +	sha1=$1
>  
> -	case "$1" in -n) sha1=$2; ff= ;; *) sha1=$1 ;; esac
>  	case "$force_rebase" in '') ;; ?*) ff= ;; esac
>  	output git rev-parse --verify $sha1 || die "Invalid commit name: $sha1"
>  
> @@ -248,24 +266,35 @@ pick_one () {
>  	fi
>  
>  	test -d "$rewritten" &&
> -		pick_one_preserving_merges "$@" && return
> +		pick_one_preserving_merges $extra_args $sha1 && return
>  	output eval git cherry-pick \
>  			${gpg_sign_opt:+$(git rev-parse --sq-quote "$gpg_sign_opt")} \
> -			"$strategy_args" $empty_args $ff "$@"
> +			"$strategy_args" $empty_args $ff $extra_args $sha1
>  }

It might be confusing that extra_args is used both in pick_one and in
pick_one_preserving_merges.  Since these are not local variables, the
call to the latter changes the value of the variable in the former.  I
don't know if that could be a problem now (can
pick_one_preserving_merges return with a nonzero exit code?) but even if
not, it is a trap for future developers.  I recommend giving the two
variables different names.

>  pick_one_preserving_merges () {
>  	fast_forward=t
> -	case "$1" in
> -	-n)
> -		fast_forward=f
> -		sha1=$2
> -		;;
> -	*)
> -		sha1=$1
> -		;;
> -	esac
> -	sha1=$(git rev-parse $sha1)
> +	no_commit=
> +	extra_args=
> +	while test $# -gt 0
> +	do
> +		case "$1" in
> +		-n)
> +			fast_forward=f
> +			extra_args="$extra_args -n"
> +			no_commit=y
> +			;;
> +		-*)
> +			warn "pick_one_preserving_merges: ignored option -- $1"
> +			;;
> +		*)
> +			break
> +			;;
> +		esac
> +		shift
> +	done
> +	test $# -ne 1 && die "pick_one_preserving_merges: wrong number of arguments"
> +	sha1=$(git rev-parse $1)
>  
>  	if test -f "$state_dir"/current-commit
>  	then
> @@ -335,7 +364,7 @@ pick_one_preserving_merges () {
>  	f)
>  		first_parent=$(expr "$new_parents" : ' \([^ ]*\)')
>  
> -		if [ "$1" != "-n" ]
> +		if test -z "$no_commit"
>  		then
>  			# detach HEAD to current parent
>  			output git checkout $first_parent 2> /dev/null ||
> @@ -344,7 +373,7 @@ pick_one_preserving_merges () {
>  
>  		case "$new_parents" in
>  		' '*' '*)
> -			test "a$1" = a-n && die "Refusing to squash a merge: $sha1"
> +			test -n "$no_commit" && die "Refusing to squash a merge: $sha1"
>  
>  			# redo merge
>  			author_script_content=$(get_author_ident_from_commit $sha1)
> @@ -365,7 +394,7 @@ pick_one_preserving_merges () {
>  		*)
>  			output eval git cherry-pick \
>  				${gpg_sign_opt:+$(git rev-parse --sq-quote "$gpg_sign_opt")} \
> -				"$strategy_args" "$@" ||
> +				"$strategy_args" $extra_args $sha1 ||
>  				die_with_patch $sha1 "Could not pick $sha1"
>  			;;
>  		esac
> 


-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/