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

[Chris Webb]

Thomas Rast  wrote:

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

I definitely agree with you both that --root should be (and isn't)
consistent with normal interactive rebasing. The difference isn't deliberate
on my part.

On a personal note, I've always disliked the way interactive rebase stops
when you pick an existing empty commit or empty log message rather than
preserving it. Jumping through a few hoops is perhaps sensible when you
create that kind of strange commit, but just annoying when picking an
existing empty/logless commit as part of a series. But as you say, that's
a separate issue than --root behaving differently to non --root.

Cheers,

Chris.

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

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

[Thomas Rast]

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