Re: [PATCH v1] rebase --root: sentinel commit cloaks empty commits

From: Thomas Rast <tr@thomasrast.ch>
Date: Jul 18, 2014, 12:10 UTC
Message-ID: <871tti50l8.fsf@thomasrast.ch>
In-Reply-To: <8d5cf2e1ff45e2e60072bf6c6e05371e4b265709.1405539123.git.bafain@gmail.com>

Hi Fabian

Impressive analysis!

Show 6 quoted lines

> Concerning the bugfix: Obviously, the patch misuses the `squash_onto`
> flag because it assumes that the new base is empty except for the
> sentinel commit. The variable name does not imply anything close to
> that. An additional flag to disable the use of the git-rev-list
> option `--cherry-pick` would work and make sense again (for instance,
> `keep_redundant`).

Seeing as there are only two existing uses of the variable, you could also rename it to make it more obvious what is going on. I think either way is fine.

[...]

Show 9 quoted lines

> Please take a closer look at the last two test cases that specify the
> expected behaviour of rebasing a branch that tracks the empty tree.
> At this point they expect the "Nothing to do" error (aborts with
> untouched history). This is consistent with rebasing only empty
> commits without `--root`, which also doesn't just delete them from
> the history. Furthermore, I think the two alternatives adding a note
> that all commits in the range were empty, and removing the empty
> commits (thus making the branch empty) are better discussed in a
> separate bug report.

Makes sense to me, though I have never thought much about rebasing empty commits. Maybe Chris has a more informed opinion?

Show 11 quoted lines

>  is_empty_commit() {
> -	tree=$(git rev-parse -q --verify "$1"^{tree} 2>/dev/null ||
> -		die "$1: not a commit that can be picked")
> -	ptree=$(git rev-parse -q --verify "$1"^^{tree} 2>/dev/null ||
> -		ptree=4b825dc642cb6eb9a060e54bf8d69288fbee4904)
> +	tree=$(git rev-parse -q --verify "$1"^{tree} 2>/dev/null) ||
> +		die "$1: not a commit that can be picked"
> +	ptree=$(git rev-parse -q --verify "$1"^^{tree} 2>/dev/null) ||
> +		ptree=4b825dc642cb6eb9a060e54bf8d69288fbee4904
>  	test "$tree" = "$ptree"
>  }

Nice catch!

Show 17 quoted lines

> @@ -958,7 +958,17 @@ then
>  	revisions=$upstream...$orig_head
>  	shortrevisions=$shortupstream..$shorthead
>  else
> -	revisions=$onto...$orig_head
> +	if test -n "$squash_onto"
> +	then
> +		# $onto points to an empty commit (the sentinel
> +		# commit) which was not created by the user.
> +		# Exclude it from the rev list to avoid skipping
> +		# empty user commits prematurely, i. e. before
> +		# --keep-empty can take effect.
> +		revisions=$orig_head
> +	else
> +		revisions=$onto...$orig_head
> +	fi
>  	shortrevisions=$shorthead

Nit: I think this would be clearer if you phrased it using an 'elif',
instead of nesting (but keep the comment!).

-- 
Thomas Rast
tr@thomasrast.ch