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

[Fabian Ruch <bafain@gmail.com>]

Hi Michael,

On 06/20/2014 03:40 PM, Michael Haggerty wrote:
> 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.

Ok.

>> +		*)
>> +			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.

Please correct me if I missed something. At the moment (786a89d),
pick_one_preserving_merges will not and cannot return with a non-zero
exit code because it doesn't explicitly return and all possibly failing
last statements are guarded with a ... || die "...". Since this can only
be established by carefully looking at the code, I will change the reuse
of extra_args.

Now that we're at it, what I didn't understand when creating this patch
was why the code doesn't explicitly say "one or the other" in the first
place:

> if test -d "$rewritten"
> then
> 	pick_one_preserving_merges "$@"
> else
> 	output eval git cherry-pick \
> 			${gpg_sign_opt:+$(git rev-parse --sq-quote "$gpg_sign_opt")} \
> 			"$strategy_args" $empty_args $ff "$@"
> fi

At least that is how I interpreted it. After all, if
pick_one_preserving_merges failed to recreate a merge commit, wouldn't
it be a bug to record the changes in a single-parent commit?

Johannes, I cc'd you this email since you're the author of f09c9b8 -
"Teach rebase -i about --preserve-merges". I hope you don't resent me
digging out this seven-year-old patch.

   Fabian

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