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

2014-07-20 Thread Chris Webb
Thomas Rast t...@thomasrast.ch 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

2014-07-18 Thread 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


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

2014-07-16 Thread Fabian Ruch
git-rebase supports the option `--root` both with and without
`--onto`. In rebase root mode it replays all commits reachable from a
branch on top of another branch, including the very first commit. In
case `--onto` is not specified, that other branch is temporarily
provided by creating an empty commit. When the first commit on the
to-do list is being replayed, the so-called sentinel commit is
amended using the log message and patch of the replayed commit. Since
the sentinel commit is empty, this results in a replacement of the
sentinel commit with the new root commit of the rebased branch.

The combination of `--root` and no `--onto` implies an interactive
rebase. When `--preserve-merges` is not specified on the command
line, git-rebase--interactive uses `--cherry-pick` with git-rev-list
to put the initial to-do list together. The left side is given by the
fake base and the right side by the branch being rebased. What
happens now is that any empty commit on the original branch is
treated as a cherry-pick of the sentinel commit and subsequently
omitted from the to-do list. This is a bug if `--keep-empty` is
specified also.

Even without `--keep-empty`, using the sentinel commit as left side
with git-rev-list can result in a faulty rebased branch. Indeed, in
the unlikely case that the original branch consists solely of empty
commits, the bug crops up in the strangest fashion as all commits are
skipped and the sentinel commit is not replaced. As a result,
git-rebase produces a branch with a single empty commit.

To trigger the replacement of the sentinel commit, git-rebase assigns
the variable `squash_onto`. Special case a second time regarding
`squash_onto` and run git-rev-list without a left side if the
variable is assigned. The latter is the case if and only if `--root`
is used without `--onto`, that is `upstream` points to the sentinel
commit and `$upstream...$orig_head` would subtract a commit that is
not actually there from the original branch.

Fix a typo in `is_empty_commit`. It always found root commits
non-empty so that empty root commits were scheduled even without
`--keep-empty`. The POSIX specification states that command
substitutions are to be executed in sub-shells, which makes exit(1)
and variable assignments not affect the script execution state. That
was the reason why `ptree` was null for parentless commits and the
test `$tree = $ptree` always false for them.

Add tests.

Signed-off-by: Fabian Ruch baf...@gmail.com
---
Hi,

Three test cases were added to the bug report to account for the
additional cases in which the bug strikes (raised by Michael on the
other sub-thread). A bugfix is included now as well.

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`). However, the following two bugs, not related to
empty commits, seem to suggest that git-rebase--interactive cannot
work obliviously to non-existent bases.

 1) git-rebase--interactive when used with `--root` and the to-do
list `noop` results in the original branch's history being
rewritten to contain only the sentinel commit.

git-rebase--interactive correctly checkouts `$onto` and replays
no commits on top of it but git-rebase has forgotten that `$onto`
was fake.

 2) git-rebase--interactive when used with `--root` always creates a
fresh root commit, regardless of `--no-ff` being specified.

With the current meaning of `squash_onto`,
git-rebase--interactive cannot just reset the branch to the old
root commit. It is really the fault of git-rebase to start off
with a new commit.

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.

Thanks for your time,
   Fabian

 git-rebase--interactive.sh | 20 ++-
 t/t3412-rebase-root.sh | 49 ++
 2 files changed, 64 insertions(+), 5 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index f267d8b..71ca0f0 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -201,10 +201,10 @@ has_action () {
 }
 
 is_empty_commit() {
-   tree=$(git rev-parse -q --verify $1^{tree} 2/dev/null ||
-   die $1: not a commit that can be picked)
-