Hi Fabian
Impressive analysis!
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.
[...]
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?
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!
@@ -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
t...@thomasrast.ch
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html