git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Johannes Schindelin <johannes.schindelin@gmx.de>
Date
Jun 23, 2014, 16:09 UTC
Message-ID
<alpine.DEB.1.00.1406231801190.14982@s15462909.onlinehome-server.info>
In-Reply-To
<53A6137A.7040803@gmail.com>
Hi,
On Sun, 22 Jun 2014, Fabian Ruch wrote:
Show 69 quoted lines
> 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.
> >>
> >> [...]
> >> +		*)
> >> +			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.

I agree that this is a good change, but I would like to point out that it would be a good change even if it pick_one_preserving_merged could return with a non-zero exit code, even if it could be established easier that it cannot return with a non-zero exit code: it is plainly confusing. (My fault, of course.)

Show 20 quoted lines
> 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.

I do not resent you. Nor do I resent that the seven-year-old change is coming to light again. But I have to admit that my recollection as to the rationale for the way the code was written is very faint indeed.

Also, my commit messages were substantially inferior back then, so I guess that the commit message of f09c9b8 does not quite help to jog my memory.

Looking at that revision of git-rebase--interactive.sh, however, I see three call sites of pick_one, and I would suspect that I wanted to maintain DRY code. Having said that, today I would replace

	@@ -68,6 +71,8 @@ die_abort () {
	 pick_one () {
		case "$1" in -n) sha1=$2 ;; *) sha1=$1 ;; esac
		git rev-parse --verify $sha1 || die "Invalid commit name: $sha1"
	+       test -d "$REWRITTEN" &&
	+               pick_one_preserving_merges "$@" && return
		parent_sha1=$(git rev-parse --verify $sha1^ 2>/dev/null)
		current_sha1=$(git rev-parse --verify HEAD)
		if [ $current_sha1 = $parent_sha1 ]; then
by
	@@ -68,6 +71,8 @@ die_abort () {
	 pick_one () {
		case "$1" in -n) sha1=$2 ;; *) sha1=$1 ;; esac
		git rev-parse --verify $sha1 || die "Invalid commit name: $sha1"
	+       if test -d "$REWRITTEN"
	+       then
	+               pick_one_preserving_merges "$@"
	+               return
	+       fi
		parent_sha1=$(git rev-parse --verify $sha1^ 2>/dev/null)
		current_sha1=$(git rev-parse --verify HEAD)
		if [ $current_sha1 = $parent_sha1 ]; then
instead.

Hope this clarifies things! Dscho

Previous: Fabian RuchNext: Fabian Ruch
Message 6 of 16 in “rebase -i: Make option handling in pick_one more flexible”
  1. 1/7 rebase -i: Make option handling in pick_one more flexibleFabian Ruch, Jun 19, 2014
  2. Michael HaggertyJun 20, 2014
  3. Junio C HamanoJun 20, 2014
  4. Fabian RuchJun 23, 2014
  5. Fabian RuchJun 21, 2014
  6. Johannes SchindelinJun 23, 2014
  7. 2/7 rebase -i: Teach do_pick the option --editFabian Ruch, Jun 19, 2014
  8. Michael HaggertyJun 20, 2014
  9. Fabian RuchJun 22, 2014
  10. 3/7 rebase -i: Stop on root commits with empty log messagesFabian Ruch, Jun 19, 2014
  11. Eric SunshineJun 21, 2014
  12. Fabian RuchJun 22, 2014
  13. 4/7 rebase -i: Commit only once when rewriting picksFabian Ruch, Jun 19, 2014
  14. 5/7 rebase -i: Do not die in do_pickFabian Ruch, Jun 19, 2014
  15. 6/7 rebase -i: Prepare for squash in terms of do_pick --amendFabian Ruch, Jun 19, 2014
  16. 7/7 rebase -i: Teach do_pick the options --amend and --fileFabian Ruch, Jun 19, 2014

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.