From e6aac8177d02691bebedec56376ed17239793f9f Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sun, 3 Mar 2019 09:11:53 -0800 Subject: built-in rebase: no need to check out `onto` twice In the case that the rebase boils down to a fast-forward, the built-in rebase reset the working tree twice: once to start the rebase at `onto`, then realizing that the original (pre-rebase) HEAD was an ancestor and we basically already fast-forwarded to the post-rebase HEAD, `reset_head()` was called to update the original ref and to point HEAD back to it. That second `reset_head()` call does not need to touch the working tree, though, as it does not change the actual tip commit (and therefore the working tree should stay unchanged anyway): only the ref needs to be updated (because the rebase detached the HEAD, and we want to go back to the branch on which the rebase was started). But that second `reset_head()` was called without the flag to leave the working tree alone (the reason: when that call was introduced, that flag was not yet even thought of). Let's avoid that unnecessary work by passing that flag. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano diff --git a/builtin/rebase.c b/builtin/rebase.c index 08ec4d5..813ec28 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -1740,8 +1740,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) strbuf_addf(&msg, "rebase finished: %s onto %s", options.head_name ? options.head_name : "detached HEAD", oid_to_hex(&options.onto->object.oid)); - reset_head(NULL, "Fast-forwarded", options.head_name, 0, - "HEAD", msg.buf); + reset_head(NULL, "Fast-forwarded", options.head_name, + RESET_HEAD_REFS_ONLY, "HEAD", msg.buf); strbuf_release(&msg); ret = !!finish_rebase(&options); goto cleanup; -- cgit v0.10.2-6-g49f6 From eaf81605b8d17cc15a4a72ffd99d71111b70815b Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sun, 3 Mar 2019 09:11:54 -0800 Subject: built-in rebase: use the correct reflog when switching branches By mistake, we used the reflog intended for ORIG_HEAD. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano diff --git a/builtin/rebase.c b/builtin/rebase.c index 813ec28..aa469ec 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -475,7 +475,7 @@ reset_head_refs: detach_head ? REF_NO_DEREF : 0, UPDATE_REFS_MSG_ON_ERR); else { - ret = update_ref(reflog_orig_head, switch_to_branch, oid, + ret = update_ref(reflog_head, switch_to_branch, oid, NULL, 0, UPDATE_REFS_MSG_ON_ERR); if (!ret) ret = create_symref("HEAD", switch_to_branch, -- cgit v0.10.2-6-g49f6 From c2d96293602bf6e8ffa1002cff8cb6faa3854815 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sun, 3 Mar 2019 09:11:54 -0800 Subject: built-in rebase: demonstrate that ORIG_HEAD is not set correctly The ORIG_HEAD pseudo ref is supposed to refer to the original, pre-rebase state after a successful rebase. Let's add a regression test to prove that this regressed: With GIT_TEST_REBASE_USE_BUILTIN=false, this test case passes, with GIT_TEST_REBASE_USE_BUILTIN=true (or unset), it fails. Reported by Nazri Ramliy. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh index 3e73f75..7e8d5bb 100755 --- a/t/t3400-rebase.sh +++ b/t/t3400-rebase.sh @@ -59,6 +59,14 @@ test_expect_success 'rebase against master' ' git rebase master ' +test_expect_failure 'rebase sets ORIG_HEAD to pre-rebase state' ' + git checkout -b orig-head topic && + pre="$(git rev-parse --verify HEAD)" && + git rebase master && + test_cmp_rev "$pre" ORIG_HEAD && + ! test_cmp_rev "$pre" HEAD +' + test_expect_success 'rebase, with and specified as :/quuxery' ' test_when_finished "git branch -D torebase" && git checkout -b torebase my-topic-branch^ && -- cgit v0.10.2-6-g49f6 From cbd29ead92d8555f2b918c7f9d53913f26f7a872 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sun, 3 Mar 2019 09:11:55 -0800 Subject: built-in rebase: set ORIG_HEAD just once, before the rebase Technically, the scripted version set ORIG_HEAD only in two spots (which really could have been one, because it called `git checkout $onto^0` to start the rebase and also if it could take a shortcut, and in both cases it called `git update-ref $orig_head`). Practically, it *implicitly* reset ORIG_HEAD whenever `git reset --hard` was called. However, what we really want is that it is set exactly once, at the beginning of the rebase. So let's do that. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano diff --git a/builtin/rebase.c b/builtin/rebase.c index aa469ec..0f4e1ea 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -369,6 +369,7 @@ static void add_var(struct strbuf *buf, const char *name, const char *value) #define RESET_HEAD_DETACH (1<<0) #define RESET_HEAD_HARD (1<<1) #define RESET_HEAD_REFS_ONLY (1<<2) +#define RESET_ORIG_HEAD (1<<3) static int reset_head(struct object_id *oid, const char *action, const char *switch_to_branch, unsigned flags, @@ -377,6 +378,7 @@ static int reset_head(struct object_id *oid, const char *action, unsigned detach_head = flags & RESET_HEAD_DETACH; unsigned reset_hard = flags & RESET_HEAD_HARD; unsigned refs_only = flags & RESET_HEAD_REFS_ONLY; + unsigned update_orig_head = flags & RESET_ORIG_HEAD; struct object_id head_oid; struct tree_desc desc[2] = { { NULL }, { NULL } }; struct lock_file lock = LOCK_INIT; @@ -453,18 +455,21 @@ reset_head_refs: strbuf_addf(&msg, "%s: ", reflog_action ? reflog_action : "rebase"); prefix_len = msg.len; - if (!get_oid("ORIG_HEAD", &oid_old_orig)) - old_orig = &oid_old_orig; - if (!get_oid("HEAD", &oid_orig)) { - orig = &oid_orig; - if (!reflog_orig_head) { - strbuf_addstr(&msg, "updating ORIG_HEAD"); - reflog_orig_head = msg.buf; - } - update_ref(reflog_orig_head, "ORIG_HEAD", orig, old_orig, 0, - UPDATE_REFS_MSG_ON_ERR); - } else if (old_orig) - delete_ref(NULL, "ORIG_HEAD", old_orig, 0); + if (update_orig_head) { + if (!get_oid("ORIG_HEAD", &oid_old_orig)) + old_orig = &oid_old_orig; + if (!get_oid("HEAD", &oid_orig)) { + orig = &oid_orig; + if (!reflog_orig_head) { + strbuf_addstr(&msg, "updating ORIG_HEAD"); + reflog_orig_head = msg.buf; + } + update_ref(reflog_orig_head, "ORIG_HEAD", orig, + old_orig, 0, UPDATE_REFS_MSG_ON_ERR); + } else if (old_orig) + delete_ref(NULL, "ORIG_HEAD", old_orig, 0); + } + if (!reflog_head) { strbuf_setlen(&msg, prefix_len); strbuf_addstr(&msg, "updating HEAD"); @@ -1725,7 +1730,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) strbuf_addf(&msg, "%s: checkout %s", getenv(GIT_REFLOG_ACTION_ENVIRONMENT), options.onto_name); if (reset_head(&options.onto->object.oid, "checkout", NULL, - RESET_HEAD_DETACH, NULL, msg.buf)) + RESET_HEAD_DETACH | RESET_ORIG_HEAD, NULL, msg.buf)) die(_("Could not detach HEAD")); strbuf_release(&msg); diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh index 7e8d5bb..460d052 100755 --- a/t/t3400-rebase.sh +++ b/t/t3400-rebase.sh @@ -59,7 +59,7 @@ test_expect_success 'rebase against master' ' git rebase master ' -test_expect_failure 'rebase sets ORIG_HEAD to pre-rebase state' ' +test_expect_success 'rebase sets ORIG_HEAD to pre-rebase state' ' git checkout -b orig-head topic && pre="$(git rev-parse --verify HEAD)" && git rebase master && -- cgit v0.10.2-6-g49f6