Re: Editing the root commit

Chris Webb [off-list ref] writes:

Chris Webb [off-list ref] writes:

quoted

Junio C Hamano [off-list ref] writes:

quoted

Even though I wouldn't bother doing this myself, I wouldn't mind
reviewing a patch series ;-)

Okay, I'll take a look when I finish my current project!

I had a bit of spare time this morning and had a quick look through
git-rebase--interactive.sh.

Apart from the validation, message and reflog code in git-rebase.sh and
git-rebase--interactive.sh that would need fixing up to know about this
case, the essence of this seems to be starting with an orphan commit instead
of a commit descended from $onto right at the end of --interactive.

I'd love to write something like

  git checkout ${onto:---orphan}

(or a variant) but can git be persuaded to have an orphan detached HEAD like
that?

For the root commit in the history, you check it out on the detached
HEAD.  Under "--interactive" if the insn sheet tells you to allow
the user to "edit/amend/reword" it, give control back the user after
you have detached HEAD at that commit.  The user experience should
be identical to the case you are replaying on an existing commit
after that point.

But lets step back a bit and look at the whole picture, to make sure
we are on the same page.

    $ git rebase [-i] frotz

looks at where you are, finds where you forked from 'frotz', and
replays everything you have done since you forked onto the tip of
'frotz'.

    $ git rebase [-i] --onto nitfol frotz

replays the same history onto the tip of 'nitfol' instead.

    $ git rebase [-i] --root --onto nitfol

looks at the entire history leading to where you are, and replays
everything you have done onto the tip of 'nitfol'.

What if we do not say --onto here?  I am not asking what the current
implementation does (we get an error message saying "I want 'onto'
specified").  What _should_ this command mean to a naïve user?

    $ git rebase [-i] --root

I think it should mean "replay all my history down to root".  The
original root commit should become the new root commit in the
rewritten history.

On Wed, Jun 20, 2012 at 11:25:32AM -0700, Junio C Hamano wrote:

What if we do not say --onto here?  I am not asking what the current
implementation does (we get an error message saying "I want 'onto'
specified").  What _should_ this command mean to a naïve user?

    $ git rebase [-i] --root

I think it should mean "replay all my history down to root".  The
original root commit should become the new root commit in the
rewritten history.

I think that is the only thing that makes sense. And it should be easy
with non-interactive rebase, because we always start with the root commit,
and it always applies cleanly.

For interactive rebase, though, it's a little trickier. Earlier you
said:

For the root commit in the history, you check it out on the detached
HEAD.  Under "--interactive" if the insn sheet tells you to allow
the user to "edit/amend/reword" it, give control back the user after
you have detached HEAD at that commit.  The user experience should
be identical to the case you are replaying on an existing commit
after that point.

That makes sense if the first instruction involves picking that first
commit. But if the first commit is deleted (or reordered), then it is
not appropriate to detach to the root; we must detach to the first
picked commit, which we can only do after we see the final instruction
sheet.

Git-rebase tries to do the rewind before dropping to run_specific_rebase.
However, I think we might be OK, as it seems that there is a special
exception for "interactive", and we drop to run_specific_rebase early in
that case.

-Peff

PS I have no clue how multiple roots would do. Without --preserve merges,
   I would except history to be flattened, and that should be OK (the root
   commit will become a non-root, just as it would if you were actually
   going --onto something else).  But I suspect --preserve-merges might be
   tricky, as you might have to create several root commits during the
   course of processing the instruction sheet.

Junio C Hamano [off-list ref] writes:

What if we do not say --onto here?  I am not asking what the current
implementation does (we get an error message saying "I want 'onto'
specified").  What _should_ this command mean to a na??ve user?

    $ git rebase [-i] --root

I think it should mean "replay all my history down to root".  The
original root commit should become the new root commit in the
rewritten history.

Yes, I definitely agree with everything you say here. That's exactly what
I'd expect from git rebase --root without --onto, and what I'd hope to be
able to implement in a patch series.

For the root commit in the history, you check it out on the detached
HEAD.  Under "--interactive" if the insn sheet tells you to allow
the user to "edit/amend/reword" it, give control back the user after
you have detached HEAD at that commit.  The user experience should
be identical to the case you are replaying on an existing commit
after that point.

I think it might a little more complicated than detecting when we have to do
a commit --amend on the root commit though? The user might have reordered
the first commit (introducing A and B, say) with the second commit
(introducing C and D), or dropped the original root commit entirely.

My understanding of the way --interactive works at the moment is that it
checks out the starting commit (whether given explicitly by --onto or taken
from <upstream>) and then 'plays out' the commits as described in the
instruction sheet.

I could re-use this unchanged if I could do a git checkout --orphan without
having to create a new branch, but I don't think this is allowed: I can have
a detached head or make an orphan checkout onto a new branch, but not both
at the same time? Would you prefer that I create a temporary branch just to
be able to git checkout --orphan onto it here, or that I add support for
this kind of 'detached HEAD with no parent' state, or is there a natural way
to rework --interactive without needing to do this which I'm missing?

Best wishes,

Chris.

Jeff King [off-list ref] writes:

But if the first commit is deleted (or reordered), then it is not
appropriate to detach to the root; we must detach to the first picked
commit, which we can only do after we see the final instruction sheet.

It's worse than that isn't it? If you have

  A -- B -- C

and the sheet says drop A, pick B, pick C, you can't detach to B. You want
the commit B as a root (i.e. with no parent), not the commit B with parent
A. You need to have the patch from A to B replayed as the first commit on an
empty branch (only without the branch).

Cheers,

Chris.

On Wed, Jun 20, 2012 at 08:39:23PM +0100, Chris Webb wrote:

Jeff King [off-list ref] writes:

quoted

But if the first commit is deleted (or reordered), then it is not
appropriate to detach to the root; we must detach to the first picked
commit, which we can only do after we see the final instruction sheet.

It's worse than that isn't it? If you have

  A -- B -- C

and the sheet says drop A, pick B, pick C, you can't detach to B. You want
the commit B as a root (i.e. with no parent), not the commit B with parent
A. You need to have the patch from A to B replayed as the first commit on an
empty branch (only without the branch).

Oh, you're right. I think you would have to do some custom magic to make
the first commit without looking at HEAD, like:

  1. Erase the index and working tree.

  2. Apply the patch from B^ to B (where B is the first picked commit).

  3. git-commit-tree manually with no parents.

  4. Point HEAD to the newly made commit.

But there is a very good chance of step (2) causing conflicts, at which
point you would have to give control back to the user. And what is in
HEAD at that point that would give them a meaningful answer to "git diff
--cached" or similar?

I think the only thing you can do is make a fake sentinel commit (with
an empty tree) to put in HEAD, and then remove the sentinel immediately
after the first commit is put in place (making sure not to include it in
the first commit's parent list). Yuck.

-Peff

Jeff King [off-list ref] writes:

I think the only thing you can do is make a fake sentinel commit (with
an empty tree) to put in HEAD, and then remove the sentinel immediately
after the first commit is put in place (making sure not to include it in
the first commit's parent list). Yuck.

If I do this:

diff --git a/path.c b/path.c
index 6f2aa69..1b3b6f3 100644
--- a/path.c
+++ b/path.c

@@ -169,8 +169,9 @@ int validate_headref(const char *path)
 	int fd;
 	ssize_t len;
 
+	/* Allow HEAD to be entirely missing for detached orphan state */
 	if (lstat(path, &st) < 0)
-		return -1;
+		return errno == ENOENT ? 0 : -1;
 
 	/* Make sure it is a "refs/.." symlink */
 	if (S_ISLNK(st.st_mode)) {

to thwart the sanity check, I can do 'rm $GIT_DIR/HEAD' to put my HEAD into a
state where it is both detached and unborn, i.e. so that my next commit will
result in a detached HEAD pointing at a root commit.

Surprisingly, this check appears to be the only thing disallowing such a state,
and the result behaves as sanely as a normal git-checkout --orphan <branch>
does! Using a detached unborn HEAD like this would avoid any need for sentinel
commits or the like in generalising rebase: we'd just do

  git rm -rf .
  rm -f $GIT_DIR/index $GIT_DIR/HEAD

instead of git checkout $onto, and be away replaying the commits or executing
the instruction sheet as normal.

If I prepared a proper patch series with docs and tests, would allowing this be
acceptable? I don't want to work on it if there's an intentional design
decision to explicitly disallow it. However, apart from just rebase and
rebase--interactive, I suspect other scripts which operate on history will be
more easily generalised to work on history right up to the root commit if such
a state were allowed.

PS Whilst experimenting, I also noticed a (presumably unintentional) behaviour:

  $ git init .
  Initialized empty Git repository in /tmp/foo/.git/
  $ git checkout --detach
  $ touch bar
  $ git add bar
  $ git commit -m test
  [(null) (root-commit) 17b5bf9] test
   0 files changed
   create mode 100644 bar
  $ ls .git/refs/heads/
  (null)
  $

Here we've created a branch with the strange name '(null)' instead of actually
detaching, or refusing to detach because we're on an unborn branch.

Assuming this is a bug, I'll cook up a patch to fix it either way, either by
entering a detached unborn state if we're allowing that, or to refuse to detach
if we're not allowing that state.

Best wishes,

Chris.

On Wed, Jun 20, 2012 at 11:25 AM, Junio C Hamano [off-list ref] wrote:

   $ git rebase [-i] --root --onto nitfol

looks at the entire history leading to where you are, and replays
everything you have done onto the tip of 'nitfol'.

What if we do not say --onto here?

Or even leaving out the --onto for that matter. If I understand
correctly, "git rebase --root --onto nitfol" is (more-or-less) just
syntactic sugar for "git rebase nitfol", see
http://thread.gmane.org/gmane.comp.version-control.git/181024/focus=182054.

I have an old branch from around the time of that thread that make
this fact (that "--root --onto" is syntactic sugar) a little clearer
in the code. I'll try to finally send out some of those patches as
soon as I can.

 What _should_ this command mean to a naïve user?

   $ git rebase [-i] --root

I think it should mean "replay all my history down to root".  The
original root commit should become the new root commit in the
rewritten history.

I agree, and in this case, the --root flag would not just be syntactic sugar.

Martin

Jeff King [off-list ref] writes:

I think the only thing you can do is make a fake sentinel commit (with
an empty tree) to put in HEAD, and then remove the sentinel immediately
after the first commit is put in place (making sure not to include it in
the first commit's parent list). Yuck.

I thought this would turn out far more horrible than it actually did in
practice.

The follow-up patch adds this empty sentinel and then automatically squashes
it into the root commit(s) as they're picked. The resulting rebase -i --root
can handle re-ordering of commits (picking a new root), editing of the root
commit, dropping the root commit, and so on.

In the second patch, I've written some simple tests to demonstrate this and
to cover cases that seem particularly likely to break, as well as removing a
test_must_fail for --root without --onto, because it no longer does!

Cheers,

Chris.

Test for likely breakages in t3404, including successful reordering of
non-conflicting changes with a new root, correct preservation of commit
message and author in a root commit when it is squashed with the
sentinel, and presence of the sentinel following a conflicting
cherry-pick of a new root.

Remove test_must_fail for git rebase --root without --onto from t3412 as
this case will now be successfully handled by an implicit git rebase -i.

Signed-off-by: Chris Webb <redacted>
---
 t/t3404-rebase-interactive.sh |   27 +++++++++++++++++++++++++++
 t/t3412-rebase-root.sh        |    4 ----
 2 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 025c1c6..6ffc9c2 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh

@@ -755,4 +755,31 @@ test_expect_success 'rebase-i history with funny messages' '
 	test_cmp expect actual
 '
 
+test_expect_success 'rebase -i --root re-order and drop commits' '
+	git checkout E &&
+	FAKE_LINES="3 1 2 5" git rebase -i --root &&
+	test E = $(git cat-file commit HEAD | sed -ne \$p) &&
+	test B = $(git cat-file commit HEAD^ | sed -ne \$p) &&
+	test A = $(git cat-file commit HEAD^^ | sed -ne \$p) &&
+	test C = $(git cat-file commit HEAD^^^ | sed -ne \$p) &&
+	test 0 = $(git cat-file commit HEAD^^^ | grep -c ^parent\ )
+'
+
+test_expect_success 'rebase -i --root retain root commit author and message' '
+	git checkout A &&
+	echo B >file7 &&
+	git add file7 &&
+	GIT_AUTHOR_NAME="Twerp Snog" git commit -m "different author" &&
+	FAKE_LINES="2" git rebase -i --root &&
+	git cat-file commit HEAD | grep -q "^author Twerp Snog" &&
+	git cat-file commit HEAD | grep -q "^different author$"
+'
+
+test_expect_success 'rebase -i --root temporary sentinel commit' '
+	git checkout B &&
+	FAKE_LINES="2" test_must_fail git rebase -i --root &&
+	git cat-file commit HEAD | grep "^tree 4b825dc642cb" &&
+	git rebase --abort
+'
+
 test_done

diff --git a/t/t3412-rebase-root.sh b/t/t3412-rebase-root.sh
index 086c91c..e4f9da8 100755
--- a/t/t3412-rebase-root.sh
+++ b/t/t3412-rebase-root.sh

@@ -22,10 +22,6 @@ test_expect_success 'prepare repository' '
 	test_commit 4 B
 '
 
-test_expect_success 'rebase --root expects --onto' '
-	test_must_fail git rebase --root
-'
-
 test_expect_success 'setup pre-rebase hook' '
 	mkdir -p .git/hooks &&
 	cat >.git/hooks/pre-rebase <<EOF &&

-- 
1.7.10

Allow --root to be specified to rebase -i without --onto, making it
possible to edit and re-order all commits right back to the root(s).

If there is a conflict to be resolved when applying the first change,
the user will expect a sane index and working tree to get sensible
behaviour from git-diff and friends, so create a sentinel commit with an
empty tree to rebase onto. Automatically squash the sentinel with any
commits rebased directly onto it, so they end up as root commits in
their own right and retain their authorship and commit message.

Signed-off-by: Chris Webb <redacted>
---
 Documentation/git-rebase.txt |    9 +++++----
 git-rebase--interactive.sh   |   24 ++++++++++++++++++------
 git-rebase.sh                |   14 ++++++++++++--
 3 files changed, 35 insertions(+), 12 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 147fa1a..85b5e44 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt

@@ -10,7 +10,7 @@ SYNOPSIS
 [verse]
 'git rebase' [-i | --interactive] [options] [--onto <newbase>]
 	[<upstream>] [<branch>]
-'git rebase' [-i | --interactive] [options] --onto <newbase>
+'git rebase' [-i | --interactive] [options] [--onto <newbase>]
 	--root [<branch>]
 'git rebase' --continue | --skip | --abort
 

@@ -348,10 +348,11 @@ idea unless you know what you are doing (see BUGS below).
 --root::
 	Rebase all commits reachable from <branch>, instead of
 	limiting them with an <upstream>.  This allows you to rebase
-	the root commit(s) on a branch.  Must be used with --onto, and
+	the root commit(s) on a branch.  When used with --onto, it
 	will skip changes already contained in <newbase> (instead of
-	<upstream>).  When used together with --preserve-merges, 'all'
-	root commits will be rewritten to have <newbase> as parent
+	<upstream>) whereas without --onto it will operate on every change.
+	When used together with both --onto and --preserve-merges,
+	'all' root commits will be rewritten to have <newbase> as parent
 	instead.
 
 --autosquash::

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 0c19b7c..ed5a6ba 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh

@@ -417,6 +417,21 @@ record_in_rewritten() {
 	esac
 }
 
+do_pick () {
+	if test "$(git rev-parse HEAD)" = "$squash_onto"
+	then
+		git commit --allow-empty --allow-empty-message --amend \
+			   --no-post-rewrite -n -q -C $1 &&
+			pick_one -n $1 &&
+			git commit --allow-empty --allow-empty-message \
+				   --amend --no-post-rewrite -n -q -C $1 ||
+			die_with_patch $1 "Could not apply $1... $2"
+	else
+		pick_one $1 ||
+			die_with_patch $1 "Could not apply $1... $2"
+	fi
+}
+
 do_next () {
 	rm -f "$msg" "$author_script" "$amend" || exit
 	read -r command sha1 rest < "$todo"

@@ -428,16 +443,14 @@ do_next () {
 		comment_for_reflog pick
 
 		mark_action_done
-		pick_one $sha1 ||
-			die_with_patch $sha1 "Could not apply $sha1... $rest"
+		do_pick $sha1 "$rest"
 		record_in_rewritten $sha1
 		;;
 	reword|r)
 		comment_for_reflog reword
 
 		mark_action_done
-		pick_one $sha1 ||
-			die_with_patch $sha1 "Could not apply $sha1... $rest"
+		do_pick $sha1 "$rest"
 		git commit --amend --no-post-rewrite || {
 			warn "Could not amend commit after successfully picking $sha1... $rest"
 			warn "This is most likely due to an empty commit message, or the pre-commit hook"

@@ -451,8 +464,7 @@ do_next () {
 		comment_for_reflog edit
 
 		mark_action_done
-		pick_one $sha1 ||
-			die_with_patch $sha1 "Could not apply $sha1... $rest"
+		do_pick $sha1 "$rest"
 		warn "Stopped at $sha1... $rest"
 		exit_with_patch $sha1 0
 		;;

diff --git a/git-rebase.sh b/git-rebase.sh
index e616737..bde2be8 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh

@@ -31,7 +31,7 @@ SUBDIRECTORY_OK=Yes
 OPTIONS_KEEPDASHDASH=
 OPTIONS_SPEC="\
 git rebase [-i] [options] [--onto <newbase>] [<upstream>] [<branch>]
-git rebase [-i] [options] --onto <newbase> --root [<branch>]
+git rebase [-i] [options] [--onto <newbase>] --root [<branch>]
 git-rebase [-i] --continue | --abort | --skip
 --
  Available options are

@@ -364,6 +364,11 @@ and run me again.  I am stopping in case you still have something
 valuable there.'
 fi
 
+if test -n "$rebase_root" && test -z "$onto"
+then
+	test -z "$interactive_rebase" && interactive_rebase=implied
+fi
+
 if test -n "$interactive_rebase"
 then
 	type=interactive

@@ -397,7 +402,12 @@ then
 	die "invalid upstream $upstream_name"
 	upstream_arg="$upstream_name"
 else
-	test -z "$onto" && die "You must specify --onto when using --root"
+	if test -z "$onto"
+	then
+		empty_tree=`git hash-object -t tree /dev/null`
+		onto=`git commit-tree $empty_tree </dev/null`
+		squash_onto="$onto"
+	fi
 	unset upstream_name
 	unset upstream
 	upstream_arg=--root

-- 
1.7.10

Chris Webb [off-list ref] writes:

PS Whilst experimenting, I also noticed a (presumably unintentional)
behaviour:

  $ git init .
  Initialized empty Git repository in /tmp/foo/.git/
  $ git checkout --detach
  $ touch bar
  $ git add bar
  $ git commit -m test
  [(null) (root-commit) 17b5bf9] test
   0 files changed
   create mode 100644 bar
  $ ls .git/refs/heads/
  (null)
  $

Here we've created a branch with the strange name '(null)' instead of
actually detaching, or refusing to detach because we're on an unborn
branch.

This was introduced by abe199808c, which is intended to allow

  git init . && git checkout --orphan newbranch

but presumably wasn't also meant to enable

  git checkout --orphan foo
  git checkout --detach

This leads to a printf("%s", NULL) and thus

  $ git symbolic-ref HEAD
  refs/heads/(null)

I've followed up to this message with a patch including a test to catch this
in future.

Best wishes,

Chris.

abe199808c (git checkout -b: allow switching out of an unborn branch)
introduced a bug demonstrated by

  git checkout --orphan foo
  git checkout --detach
  git symbolic-ref HEAD

which gives 'refs/heads/(null)'.

This happens because we strbuf_addf(&branch_ref, "refs/heads/%s",
opts->new_branch) when opts->new_branch can be NULL for --detach.

Catch and forbid this case, adding a test to t2017 to catch it in
future.

Signed-off-by: Chris Webb <redacted>
---
 builtin/checkout.c         |    2 ++
 t/t2017-checkout-orphan.sh |    6 ++++++
 2 files changed, 8 insertions(+)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index e8c1b1f..3980d5d 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c

@@ -915,6 +915,8 @@ static int switch_unborn_to_new_branch(struct checkout_opts *opts)
 	int status;
 	struct strbuf branch_ref = STRBUF_INIT;
 
+	if (!opts->new_branch)
+		die(_("You are on a branch yet to be born"));
 	strbuf_addf(&branch_ref, "refs/heads/%s", opts->new_branch);
 	status = create_symref("HEAD", branch_ref.buf, "checkout -b");
 	strbuf_release(&branch_ref);

diff --git a/t/t2017-checkout-orphan.sh b/t/t2017-checkout-orphan.sh
index 0e3b858..655f278 100755
--- a/t/t2017-checkout-orphan.sh
+++ b/t/t2017-checkout-orphan.sh

@@ -116,4 +116,10 @@ test_expect_success '--orphan refuses to switch if a merge is needed' '
 	git reset --hard
 '
 
+test_expect_success 'cannot --detach on an unborn branch' '
+	git checkout master &&
+	git checkout --orphan new &&
+	test_must_fail git checkout --detach
+'
+
 test_done

-- 
1.7.10