Fix ORIG_HEAD behavior of the built-in rebase
PATCH 3/4 built-in rebase: demonstrate that ORIG_HEAD is not set correctly
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
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.
--- t/t3400-rebase.sh | 8 ++++++++ 1 file changed, 8 insertions(+)
t/t3400-rebase.sh +8 -0
@@ -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 <onto> and <upstream> specified as :/quuxery' ' test_when_finished "git branch -D torebase" && git checkout -b torebase my-topic-branch^ &&
Signature · 1 line
gitgitgadget
PATCH 4/4 built-in rebase: set ORIG_HEAD just once, before the rebase
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
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.
--- builtin/rebase.c | 31 ++++++++++++++++++------------- t/t3400-rebase.sh | 2 +- 2 files changed, 19 insertions(+), 14 deletions(-)
builtin/rebase.c +18 -13
@@ -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 @@ static int reset_head(struct object_id *oid, const char *action,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);
t/t3400-rebase.sh +1 -1
@@ -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 &&
Signature · 1 line
gitgitgadget
PATCH 2/4 built-in rebase: use the correct reflog when switching branches
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
By mistake, we used the reflog intended for ORIG_HEAD.
--- builtin/rebase.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
builtin/rebase.c +1 -1
@@ -475,7 +475,7 @@ static int reset_head(struct object_id *oid, const char *action, 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,
Signature · 1 line
gitgitgadget
cover Fix ORIG_HEAD behavior of the built-in rebase
It was reported by Nazri Ramliy that ORIG_HEAD was set incorrectly again, this time caused by the shortcut to call git am directly, without detouring to a Unix shell script.
Patch 2/4 might look like something completely unrelated, but without it, the update to HEAD might use an incorrect reflog message.
Patch 1/4 is more a "while at it" patch; while looking which code paths might need to update ORIG_HEAD, I noticed that this reset_head() call did unnecessary work.
Johannes Schindelin (4):
built-in rebase: no need to check out onto twice built-in rebase: use the correct reflog when switching branches built-in rebase: demonstrate that ORIG_HEAD is not set correctly built-in rebase: set ORIG_HEAD just once, before the rebase
builtin/rebase.c | 37 +++++++++++++++++++++---------------- t/t3400-rebase.sh | 8 ++++++++ 2 files changed, 29 insertions(+), 16 deletions(-)
base-commit: 21853626eac565dd42572d90724b29863f61eb3b
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-153%2Fdscho%2Frebase-am-and-orig-head-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-153/dscho/rebase-am-and-orig-head-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/153
Signature · 1 line
gitgitgadget
PATCH 1/4 built-in rebase: no need to check out `onto` twice
From: Johannes Schindelin <[email protected]>
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 HEAD was an ancestor, 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. So let's avoid that unnecessary work.
--- builtin/rebase.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
builtin/rebase.c +2 -2
@@ -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;
Signature · 1 line
gitgitgadget
Re: [PATCH 1/4] built-in rebase: no need to check out `onto` twice
Hi Johannes
Quoted 10 lines · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]>
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 HEAD was an ancestor,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. So let's avoid that unnecessary work.
I'm confused by this I think I must be missing something. If we've checked out onto then why does the working copy not need updating when we fast forward. (also why to we checkout onto before seeing if we can fast-forward but that's not related to this patch series)
Best Wishes
Phillip
Quoted 22 lines
Signed-off-by Johannes Schindelin <[email protected]>--- builtin/rebase.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)builtin/rebase.c +2
-2@@ -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;
Re: [PATCH 1/4] built-in rebase: no need to check out `onto` twice
Hi Phillip,
Quoted 8 lines · Phillip Wood
Quoted 7 lines · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]>
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 HEAD was an ancestor,reset_head()was called to update the original ref and to point HEAD back to it.
Quoted 8 lines
Quoted 3 lines
That second
reset_head()call does not need to touch the working tree, though, as it does not change the actual tip commit. So let's avoid that unnecessary work.I'm confused by this I think I must be missing something. If we've checked out onto then why does the working copy not need updating when we fast forward. (also why to we checkout onto before seeing if we can fast-forward but that's not related to this patch series)
Sorry, I really try to learn how to express things clearly. Still learning a lot.
So what happens is this: we detect the situation where the pre-rebase HEAD is an ancestor of onto. We do this *after* checking out onto. So we now know that we essentially fast-forwarded to the post-rebase state.
What we still need to do is to update the original ref (unless we were on a detached HEAD when the rebase was started).
The original shell code updates the original ref to the current HEAD, and the updates HEAD to point to that symbolic ref instead of being detached.
In the C code, we abused reset_head() to do the same. But reset_head() does more: it does a two-way merge (in this instance, because RESET_HEAD_HARD was not part of the flags). Which is unnecessary.
That's all this commit is about.
Ciao,
Dscho
Quoted 29 lines
Best Wishes
Phillip
Quoted 23 lines
Signed-off-by Johannes Schindelin <[email protected]>--- builtin/rebase.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)builtin/rebase.c +2
-2@@ -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;
Re: [PATCH 1/4] built-in rebase: no need to check out `onto` twice
Hi Dscho
Quoted 21 lines · Johannes Schindelin
Hi Phillip,
Quoted 8 lines · Phillip Wood
Quoted 7 lines · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]>
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 HEAD was an ancestor,reset_head()was called to update the original ref and to point HEAD back to it.Quoted 8 lines
Quoted 3 lines
That second
reset_head()call does not need to touch the working tree, though, as it does not change the actual tip commit. So let's avoid that unnecessary work.I'm confused by this I think I must be missing something. If we've checked out onto then why does the working copy not need updating when we fast forward. (also why to we checkout onto before seeing if we can fast-forward but that's not related to this patch series)
Quoted 7 lines
Sorry, I really try to learn how to express things clearly. Still learning a lot.
So what happens is this: we detect the situation where the pre-rebase
HEADis an ancestor ofonto. We do this *after* checking outonto. So we now know that we essentially fast-forwarded to the post-rebase state.
Ah that makes sense, for some reason I misread the commit message and thought that we were dealing with the case where onto was an ancestor of the original HEAD and we were fast-forwarding from onto back to the original HEAD.
Quoted 11 lines
What we still need to do is to update the original ref (unless we were on a detached HEAD when the rebase was started).
The original shell code updates the original ref to the current HEAD, and the updates HEAD to point to that symbolic ref instead of being detached.
In the C code, we abused
reset_head()to do the same. Butreset_head()does more: it does a two-way merge (in this instance, becauseRESET_HEAD_HARDwas not part of the flags). Which is unnecessary.That's all this commit is about.
Thanks for explaining, it all makes sense to me now
Best Wishes
Phillip
Quoted 32 lines
Ciao,
DschoQuoted 29 lines
Best Wishes
Phillip
Quoted 23 lines
Signed-off-by Johannes Schindelin <[email protected]>--- builtin/rebase.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)builtin/rebase.c +2
-2@@ -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;
Re: [PATCH 1/4] built-in rebase: no need to check out `onto` twice
Quoted 1 line · Phillip Wood
Thanks for explaining, it all makes sense to me now
It would be necessary to make sure that it all makes sense to all future readers. Are they patches good enough as-is for that, or do they need some updates before I take a look at them to pick up?
Re: [PATCH 1/4] built-in rebase: no need to check out `onto` twice
Hi Junio,
Quoted 7 lines · Junio C Hamano
Quoted 1 line · Phillip Wood
Thanks for explaining, it all makes sense to me now
It would be necessary to make sure that it all makes sense to all future readers. Are they patches good enough as-is for that, or do they need some updates before I take a look at them to pick up?
As you frequently say: if a reviewer gets puzzled, it is often a good indicator that the commit message needs to be improved. I did exactly that and will send out the next iteration in a minute.
Ciao,
Dscho
v2 cover Fix ORIG_HEAD behavior of the built-in rebase
In reply to Johannes Schindelin via GitGitGadgetIt was reported by Nazri Ramliy that ORIG_HEAD was set incorrectly again, this time caused by the shortcut to call git am directly, without detouring to a Unix shell script.
Patch 2/4 might look like something completely unrelated, but without it, the update to HEAD might use an incorrect reflog message.
Patch 1/4 is more a "while at it" patch; while looking which code paths might need to update ORIG_HEAD, I noticed that this reset_head() call did unnecessary work.
Changes since v1:
* Reworded the unclear commit message of patch 1/4.
Johannes Schindelin (4):
built-in rebase: no need to check out onto twice built-in rebase: use the correct reflog when switching branches built-in rebase: demonstrate that ORIG_HEAD is not set correctly built-in rebase: set ORIG_HEAD just once, before the rebase
builtin/rebase.c | 37 +++++++++++++++++++++---------------- t/t3400-rebase.sh | 8 ++++++++ 2 files changed, 29 insertions(+), 16 deletions(-)
base-commit: 21853626eac565dd42572d90724b29863f61eb3b
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-153%2Fdscho%2Frebase-am-and-orig-head-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-153/dscho/rebase-am-and-orig-head-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/153
Range-diff vs v1:
1: 2d99429387 ! 1: 59c3266ed5 built-in rebase: no need to check out onto twice
@@ -4,12 +4,21 @@
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 HEAD was an ancestor, reset_head()
- was called to update the original ref and to point HEAD back to it.
+ 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. So let's avoid that
- unnecessary work.
+ 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.
2: b92d76065d = 2: 67e41c032a built-in rebase: use the correct reflog when switching branches 3: 1bc3cbad26 = 3: 17737998f4 built-in rebase: demonstrate that ORIG_HEAD is not set correctly 4: 1786c172ef = 4: 7891c05f51 built-in rebase: set ORIG_HEAD just once, before the rebase
Signature · 1 line
gitgitgadget
PATCH v2 1/4 built-in rebase: no need to check out `onto` twice
From: Johannes Schindelin <[email protected]>
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.
--- builtin/rebase.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
builtin/rebase.c +2 -2
@@ -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;
Signature · 1 line
gitgitgadget
PATCH v2 2/4 built-in rebase: use the correct reflog when switching branches
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
By mistake, we used the reflog intended for ORIG_HEAD.
--- builtin/rebase.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
builtin/rebase.c +1 -1
@@ -475,7 +475,7 @@ static int reset_head(struct object_id *oid, const char *action, 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,
Signature · 1 line
gitgitgadget
PATCH v2 3/4 built-in rebase: demonstrate that ORIG_HEAD is not set correctly
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
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.
--- t/t3400-rebase.sh | 8 ++++++++ 1 file changed, 8 insertions(+)
t/t3400-rebase.sh +8 -0
@@ -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 <onto> and <upstream> specified as :/quuxery' ' test_when_finished "git branch -D torebase" && git checkout -b torebase my-topic-branch^ &&
Signature · 1 line
gitgitgadget
PATCH v2 4/4 built-in rebase: set ORIG_HEAD just once, before the rebase
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
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.
--- builtin/rebase.c | 31 ++++++++++++++++++------------- t/t3400-rebase.sh | 2 +- 2 files changed, 19 insertions(+), 14 deletions(-)
builtin/rebase.c +18 -13
@@ -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 @@ static int reset_head(struct object_id *oid, const char *action,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);
t/t3400-rebase.sh +1 -1
@@ -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 &&
Signature · 1 line
gitgitgadget
Re: [PATCH 1/4] built-in rebase: no need to check out `onto` twice
In reply to Junio C HamanoHi Junio
Quoted 8 lines · Junio C Hamano
Quoted 1 line · Phillip Wood
Thanks for explaining, it all makes sense to me now
It would be necessary to make sure that it all makes sense to all future readers. Are they patches good enough as-is for that, or do they need some updates before I take a look at them to pick up?
I've just re-read them and I think they're fine as is, though the first
paragraph of the first commit message might be clearer if "HEAD was an
ancestor and" was changed to "HEAD was an ancestor of onto and".
Best Wishes
Phillip