From: Phillip Wood via GitGitGadget <hidden> Date: 2021-10-01 10:05:07
Fix some issues with the implementation and use of reset_head(). The last
patch was previously posted as [1], I have updated the commit message and
rebased it onto the fixes in this series. There are a couple of small
conflicts merging this into seen, I think they should be easy to resolve (in
rebase.c take both sides in reset.c take the changed lines from each side).
These patches are based on pw/rebase-of-a-tag-fix
[1]
https://lore.kernel.org/git/39ad40c9297531a2d42b7263a1d41b1ecbc23c0a.1631108472.git.gitgitgadget@gmail.com/
Phillip Wood (11):
rebase: factor out checkout for up to date branch
reset_head(): fix checkout
reset_head(): don't run checkout hook if there is an error
reset_head(): remove action parameter
reset_head(): factor out ref updates
reset_head(): make default_reflog_action optional
rebase: cleanup reset_head() calls
reset_head(): take struct rebase_head_opts
rebase --apply: fix reflog
rebase --apply: set ORIG_HEAD correctly
rebase -m: don't fork git checkout
builtin/merge.c | 6 +-
builtin/rebase.c | 97 +++++++++++++++----------
reset.c | 143 ++++++++++++++++++++++---------------
reset.h | 33 +++++++--
sequencer.c | 48 ++++---------
sequencer.h | 3 +-
t/t3406-rebase-message.sh | 23 ++++++
t/t3418-rebase-continue.sh | 26 +++++++
8 files changed, 240 insertions(+), 139 deletions(-)
base-commit: 7740ac691d8e7f1bed67bcbdb1ee5c5c618f7373
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1049%2Fphillipwood%2Fwip%2Frebase-reset-head-fixes-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1049/phillipwood/wip/rebase-reset-head-fixes-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1049
--
gitgitgadget
From: Phillip Wood via GitGitGadget <hidden> Date: 2021-10-01 10:05:08
From: Phillip Wood <redacted>
This code is heavily indented and it will be convenient later in the
series to have it in its own function.
Signed-off-by: Phillip Wood <redacted>
---
builtin/rebase.c | 33 +++++++++++++++++++--------------
1 file changed, 19 insertions(+), 14 deletions(-)
@@ -1099,6 +1099,23 @@ static int rebase_config(const char *var, const char *value, void *data)returngit_default_config(var,value,data);}+staticintcheckout_up_to_date(structrebase_options*options)+{+structstrbufbuf=STRBUF_INIT;+intret=0;++strbuf_addf(&buf,"%s: checkout %s",+getenv(GIT_REFLOG_ACTION_ENVIRONMENT),+options->switch_to);+if(reset_head(the_repository,&options->orig_head,"checkout",+options->head_name,RESET_HEAD_RUN_POST_CHECKOUT_HOOK,+NULL,buf.buf,DEFAULT_REFLOG_ACTION)<0)+ret=error(_("could not switch to %s"),options->switch_to);+strbuf_release(&buf);++returnret;+}+/**Determineswhetherthecommitsinfrom..toarelinear,i.e.contain*nomergecommits.Thisfunction*expects*`from`tobeanancestorof
@@ -1978,21 +1995,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)if(!(options.flags&REBASE_FORCE)){/* Lazily switch to the target branch if needed... */if(options.switch_to){-strbuf_reset(&buf);-strbuf_addf(&buf,"%s: checkout %s",-getenv(GIT_REFLOG_ACTION_ENVIRONMENT),-options.switch_to);-if(reset_head(the_repository,-&options.orig_head,"checkout",-options.head_name,-RESET_HEAD_RUN_POST_CHECKOUT_HOOK,-NULL,buf.buf,-DEFAULT_REFLOG_ACTION)<0){-ret=error(_("could not switch to "-"%s"),-options.switch_to);+ret=checkout_up_to_date(&options);+if(ret)gotocleanup;-}}if(!(options.flags&REBASE_NO_QUIET))
From: Phillip Wood via GitGitGadget <hidden> Date: 2021-10-01 10:05:11
From: Phillip Wood <redacted>
The hook should only be run if the worktree and refs were successfully
updated.
Signed-off-by: Phillip Wood <redacted>
---
reset.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Phillip Wood via GitGitGadget <hidden> Date: 2021-10-01 10:05:13
From: Phillip Wood <redacted>
The action parameter is passed as the command name to
setup_unpack_trees_porcelain(). All but two cases pass either
"checkout" or "reset". The case that passes "reset --hard" should be
passing "reset" instead. The case that passes "Fast-forwarded" is only
updating HEAD and so does not call unpack_trees(). The value can be
determined by checking whether flags contains RESET_HEAD_HARD so it
does not need to be specified by the caller.
Signed-off-by: Phillip Wood <redacted>
---
builtin/rebase.c | 14 +++++++-------
reset.c | 5 +++--
reset.h | 2 +-
sequencer.c | 3 +--
4 files changed, 12 insertions(+), 12 deletions(-)
@@ -789,7 +789,7 @@ static int move_to_original_branch(struct rebase_options *opts)opts->head_name,oid_to_hex(&opts->onto->object.oid));strbuf_addf(&head_reflog,"rebase finished: returning to %s",opts->head_name);-ret=reset_head(the_repository,NULL,"",opts->head_name,+ret=reset_head(the_repository,NULL,opts->head_name,RESET_HEAD_REFS_ONLY,orig_head_reflog.buf,head_reflog.buf,DEFAULT_REFLOG_ACTION);
@@ -880,7 +880,7 @@ static int run_am(struct rebase_options *opts)free(rebased_patches);strvec_clear(&am.args);-reset_head(the_repository,&opts->orig_head,"checkout",+reset_head(the_repository,&opts->orig_head,opts->head_name,0,"HEAD",NULL,DEFAULT_REFLOG_ACTION);error(_("\ngit encountered an error while preparing the "
@@ -1107,7 +1107,7 @@ static int checkout_up_to_date(struct rebase_options *options)strbuf_addf(&buf,"%s: checkout %s",getenv(GIT_REFLOG_ACTION_ENVIRONMENT),options->switch_to);-if(reset_head(the_repository,&options->orig_head,"checkout",+if(reset_head(the_repository,&options->orig_head,options->head_name,RESET_HEAD_RUN_POST_CHECKOUT_HOOK,NULL,buf.buf,DEFAULT_REFLOG_ACTION)<0)ret=error(_("could not switch to %s"),options->switch_to);
@@ -1557,7 +1557,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)rerere_clear(the_repository,&merge_rr);string_list_clear(&merge_rr,1);-if(reset_head(the_repository,NULL,"reset",NULL,RESET_HEAD_HARD,+if(reset_head(the_repository,NULL,NULL,RESET_HEAD_HARD,NULL,NULL,DEFAULT_REFLOG_ACTION)<0)die(_("could not discard worktree changes"));remove_branch_state(the_repository,0);
@@ -1575,7 +1575,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)if(read_basic_state(&options))exit(1);-if(reset_head(the_repository,&options.orig_head,"reset",+if(reset_head(the_repository,&options.orig_head,options.head_name,RESET_HEAD_HARD,NULL,NULL,DEFAULT_REFLOG_ACTION)<0)die(_("could not move back to %s"),
From: Phillip Wood via GitGitGadget <hidden> Date: 2021-10-01 10:05:14
From: Phillip Wood <redacted>
In the next commit we will stop trying to update HEAD when we are just
clearing changes from the working tree. Move the code that updates the
refs to its own function in preparation for that.
Signed-off-by: Phillip Wood <redacted>
---
reset.c | 112 +++++++++++++++++++++++++++++++-------------------------
1 file changed, 62 insertions(+), 50 deletions(-)
From: Phillip Wood via GitGitGadget <hidden> Date: 2021-10-01 10:05:15
From: Phillip Wood <redacted>
This parameter is only needed when a ref is going to be updated and
the caller does not pass an explicit reflog message. Callers that are
just discarding changes in the working tree like create_autostash() do
not update any refs so should not have to worry about passing this
parameter.
Signed-off-by: Phillip Wood <redacted>
---
builtin/merge.c | 6 ++----
builtin/rebase.c | 14 ++++++--------
reset.c | 16 ++++++++++++----
sequencer.c | 6 +++---
sequencer.h | 3 +--
5 files changed, 24 insertions(+), 21 deletions(-)
@@ -1632,8 +1631,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)if(autostash)create_autostash(the_repository,-git_path_merge_autostash(the_repository),-"merge");+git_path_merge_autostash(the_repository));/* We are going to make a new commit. */git_committer_info(IDENT_STRICT);
@@ -791,8 +791,7 @@ static int move_to_original_branch(struct rebase_options *opts)opts->head_name);ret=reset_head(the_repository,NULL,opts->head_name,RESET_HEAD_REFS_ONLY,-orig_head_reflog.buf,head_reflog.buf,-DEFAULT_REFLOG_ACTION);+orig_head_reflog.buf,head_reflog.buf,NULL);strbuf_release(&orig_head_reflog);strbuf_release(&head_reflog);
@@ -1109,7 +1108,7 @@ static int checkout_up_to_date(struct rebase_options *options)options->switch_to);if(reset_head(the_repository,&options->orig_head,options->head_name,RESET_HEAD_RUN_POST_CHECKOUT_HOOK,-NULL,buf.buf,DEFAULT_REFLOG_ACTION)<0)+NULL,buf.buf,NULL)<0)ret=error(_("could not switch to %s"),options->switch_to);strbuf_release(&buf);
@@ -1558,7 +1557,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)string_list_clear(&merge_rr,1);if(reset_head(the_repository,NULL,NULL,RESET_HEAD_HARD,-NULL,NULL,DEFAULT_REFLOG_ACTION)<0)+NULL,NULL,NULL)<0)die(_("could not discard worktree changes"));remove_branch_state(the_repository,0);if(read_basic_state(&options))
@@ -1964,8 +1963,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)die(_("could not read index"));if(options.autostash){-create_autostash(the_repository,state_dir_path("autostash",&options),-DEFAULT_REFLOG_ACTION);+create_autostash(the_repository,+state_dir_path("autostash",&options));}if(require_clean_work_tree(the_repository,"rebase",
@@ -21,8 +21,13 @@ static int update_refs(const struct object_id *oid, const char *switch_to_branchsize_tprefix_len;intret;-reflog_action=getenv(GIT_REFLOG_ACTION_ENVIRONMENT);-strbuf_addf(&msg,"%s: ",reflog_action?reflog_action:default_reflog_action);+if((update_orig_head&&!reflog_orig_head)||!reflog_head){+if(!default_reflog_action)+BUG("default_reflog_action must be given when reflog messages are omitted");+reflog_action=getenv(GIT_REFLOG_ACTION_ENVIRONMENT);+strbuf_addf(&msg,"%s: ",reflog_action?reflog_action:+default_reflog_action);+}prefix_len=msg.len;if(update_orig_head){
From: Phillip Wood via GitGitGadget <hidden> Date: 2021-10-01 10:05:17
From: Phillip Wood <redacted>
move_to_original_branch() passes the message intended for the branch
reflog as `orig_head_msg`. Fix this by adding a `branch_msg` member to
struct reset_head_opts and add a regression test. Note that these
reflog messages do not respect GIT_REFLOG_ACTION. They are not alone
in that and will be fixed in a future series.
The "merge" backend already has tests that check both the branch and
HEAD reflogs.
Signed-off-by: Phillip Wood <redacted>
---
builtin/rebase.c | 8 ++++----
reset.c | 12 ++++++++++--
reset.h | 2 ++
t/t3406-rebase-message.sh | 23 +++++++++++++++++++++++
4 files changed, 39 insertions(+), 6 deletions(-)
@@ -15,6 +15,7 @@ static int update_refs(const struct reset_head_opts *opts,unsignedrun_hook=opts->flags&RESET_HEAD_RUN_POST_CHECKOUT_HOOK;unsignedupdate_orig_head=opts->flags&RESET_ORIG_HEAD;constchar*switch_to_branch=opts->branch;+constchar*reflog_branch=opts->branch_msg;constchar*reflog_head=opts->head_msg;constchar*reflog_orig_head=opts->orig_head_msg;constchar*default_reflog_action=opts->default_reflog_action;
@@ -58,8 +59,9 @@ static int update_refs(const struct reset_head_opts *opts,detach_head?REF_NO_DEREF:0,UPDATE_REFS_MSG_ON_ERR);else{-ret=update_ref(reflog_head,switch_to_branch,oid,-NULL,0,UPDATE_REFS_MSG_ON_ERR);+ret=update_ref(reflog_branch?reflog_branch:reflog_head,+switch_to_branch,oid,NULL,0,+UPDATE_REFS_MSG_ON_ERR);if(!ret)ret=create_symref("HEAD",switch_to_branch,reflog_head);
@@ -90,6 +92,12 @@ int reset_head(struct repository *r, const struct reset_head_opts *opts)if(switch_to_branch&&!starts_with(switch_to_branch,"refs/"))BUG("Not a fully qualified branch: '%s'",switch_to_branch);+if(opts->orig_head_msg&&!update_orig_head)+BUG("ORIG_HEAD reflog message given without updating ORIG_HEAD");++if(opts->branch_msg&&!opts->branch)+BUG("branch reflog message given without a branch");+if(!refs_only&&repo_hold_locked_index(r,&lock,LOCK_REPORT_ON_ERROR)<0){ret=-1;gotoleave_reset_head;
From: Phillip Wood via GitGitGadget <hidden> Date: 2021-10-01 10:05:18
From: Phillip Wood <redacted>
This function already takes a confusingly large number of parameters
some of which are optional or not always required. The following
commits will add a couple more parameters so change it to take a
struct of options first.
Signed-off-by: Phillip Wood <redacted>
---
builtin/rebase.c | 57 ++++++++++++++++++++++++++++++------------------
reset.c | 36 +++++++++++++++---------------
reset.h | 29 ++++++++++++++++++++----
sequencer.c | 5 ++---
4 files changed, 80 insertions(+), 47 deletions(-)
@@ -789,9 +790,11 @@ static int move_to_original_branch(struct rebase_options *opts)opts->head_name,oid_to_hex(&opts->onto->object.oid));strbuf_addf(&head_reflog,"rebase finished: returning to %s",opts->head_name);-ret=reset_head(the_repository,NULL,opts->head_name,-RESET_HEAD_REFS_ONLY,-orig_head_reflog.buf,head_reflog.buf,NULL);+ropts.branch=opts->head_name;+ropts.flags=RESET_HEAD_REFS_ONLY;+ropts.orig_head_msg=orig_head_reflog.buf;+ropts.head_msg=head_reflog.buf;+ret=reset_head(the_repository,&ropts);strbuf_release(&orig_head_reflog);strbuf_release(&head_reflog);
@@ -875,13 +878,15 @@ static int run_am(struct rebase_options *opts)status=run_command(&format_patch);if(status){+structreset_head_optsropts={0};unlink(rebased_patches);free(rebased_patches);strvec_clear(&am.args);-reset_head(the_repository,&opts->orig_head,-opts->head_name,0,-NULL,NULL,DEFAULT_REFLOG_ACTION);+ropts.oid=&opts->orig_head;+ropts.branch=opts->head_name;+ropts.default_reflog_action=DEFAULT_REFLOG_ACTION;+reset_head(the_repository,&ropts);error(_("\ngit encountered an error while preparing the ""patches to replay\n""these revisions:\n"
@@ -1101,14 +1106,17 @@ static int rebase_config(const char *var, const char *value, void *data)staticintcheckout_up_to_date(structrebase_options*options){structstrbufbuf=STRBUF_INIT;+structreset_head_optsropts={0};intret=0;strbuf_addf(&buf,"%s: checkout %s",getenv(GIT_REFLOG_ACTION_ENVIRONMENT),options->switch_to);-if(reset_head(the_repository,&options->orig_head,-options->head_name,RESET_HEAD_RUN_POST_CHECKOUT_HOOK,-NULL,buf.buf,NULL)<0)+ropts.oid=&options->orig_head;+ropts.branch=options->head_name;+ropts.flags=RESET_HEAD_RUN_POST_CHECKOUT_HOOK;+ropts.head_msg=buf.buf;+if(reset_head(the_repository,&ropts)<0)ret=error(_("could not switch to %s"),options->switch_to);strbuf_release(&buf);
@@ -1555,9 +1564,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)rerere_clear(the_repository,&merge_rr);string_list_clear(&merge_rr,1);--if(reset_head(the_repository,NULL,NULL,RESET_HEAD_HARD,-NULL,NULL,NULL)<0)+ropts.flags=RESET_HEAD_HARD;+if(reset_head(the_repository,&ropts)<0)die(_("could not discard worktree changes"));remove_branch_state(the_repository,0);if(read_basic_state(&options))
@@ -1574,9 +1582,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)if(read_basic_state(&options))exit(1);-if(reset_head(the_repository,&options.orig_head,-options.head_name,RESET_HEAD_HARD,-NULL,NULL,DEFAULT_REFLOG_ACTION)<0)+ropts.oid=&options.orig_head;+ropts.branch=options.head_name;+ropts.flags=RESET_HEAD_HARD;+ropts.default_reflog_action=DEFAULT_REFLOG_ACTION;+if(reset_head(the_repository,&ropts)<0)die(_("could not move back to %s"),oid_to_hex(&options.orig_head));remove_branch_state(the_repository,0);
@@ -2063,10 +2073,12 @@ 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(the_repository,&options.onto->object.oid,NULL,-RESET_HEAD_DETACH|RESET_ORIG_HEAD|-RESET_HEAD_RUN_POST_CHECKOUT_HOOK,-NULL,msg.buf,DEFAULT_REFLOG_ACTION))+ropts.oid=&options.onto->object.oid;+ropts.flags=RESET_HEAD_DETACH|RESET_ORIG_HEAD|+RESET_HEAD_RUN_POST_CHECKOUT_HOOK;+ropts.head_msg=msg.buf;+ropts.default_reflog_action=DEFAULT_REFLOG_ACTION;+if(reset_head(the_repository,&ropts))die(_("Could not detach HEAD"));strbuf_release(&msg);
@@ -12,9 +12,30 @@#define RESET_HEAD_REFS_ONLY (1<<3)#define RESET_ORIG_HEAD (1<<4)-intreset_head(structrepository*r,structobject_id*oid,-constchar*switch_to_branch,unsignedflags,-constchar*reflog_orig_head,constchar*reflog_head,-constchar*default_reflog_action);+structreset_head_opts{+/* The oid of the commit to checkout/reset to. Defaults to HEAD */+conststructobject_id*oid;+/* Optional branch to switch to */+constchar*branch;+/* Flags defined above */+unsignedflags;+/*+*OptionalreflogmessageforHEAD,ifthisisnotsetthen+*default_reflog_actionmustbe.+*/+constchar*head_msg;+/*+*OptionalreflogmessageforORIG_HEAD,ifthisisnotsetandflags+*containsRESET_ORIG_HEADthendefault_reflog_actionmustbeset.+*/+constchar*orig_head_msg;+/*+*Actiontouseindefaultreflogmessages,onlyrequiredifarefis+*beingupdatedandthereflogmessagesaboveareomitted.+*/+constchar*default_reflog_action;+};++intreset_head(structrepository*r,conststructreset_head_opts*opts);#endif
From: Phillip Wood via GitGitGadget <hidden> Date: 2021-10-01 10:05:20
From: Phillip Wood <redacted>
At the start of a rebase ORIG_HEAD is updated to tip of the branch
being rebased. Unfortunately reset_head() always uses the current
value of HEAD for this which is incorrect if the rebase is started
with 'git rebase <upstream> <branch>' as in that case ORIG_HEAD should
be updated to <branch>. This only affects the "apply" backend as the
"merge" backend does not yet use reset_head() for the initial
checkout. Fix this by passing in orig_head when calling reset_head()
and add some regression tests.
Signed-off-by: Phillip Wood <redacted>
---
builtin/rebase.c | 1 +
reset.c | 4 +++-
reset.h | 2 ++
t/t3418-rebase-continue.sh | 26 ++++++++++++++++++++++++++
4 files changed, 32 insertions(+), 1 deletion(-)
@@ -15,6 +15,8 @@structreset_head_opts{/* The oid of the commit to checkout/reset to. Defaults to HEAD */conststructobject_id*oid;+/* Optional commit when setting ORIG_HEAD. Defaults to HEAD */+conststructobject_id*orig_head;/* Optional branch to switch to */constchar*branch;/* Flags defined above */
From: Phillip Wood via GitGitGadget <hidden> Date: 2021-10-01 10:05:21
From: Phillip Wood <redacted>
The reset bit should only be set if flags contains RESET_HEAD_HARD.
The test for `!deatch_head` dates back to the original implementation
of reset_head() in ac7f467fef ("builtin/rebase: support running "git
rebase <upstream>"", 2018-08-07) and was correct until e65123a71d
("builtin rebase: support `git rebase <upstream> <switch-to>`",
2018-09-04) started using reset_head() to checkout <switch-to> when
fast-forwarding.
Signed-off-by: Phillip Wood <redacted>
---
reset.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Phillip Wood via GitGitGadget <hidden> Date: 2021-10-01 10:05:26
From: Phillip Wood <redacted>
Now that reset_head() can handle the initial checkout of onto
correctly use it in the "merge" backend instead of forking 'git
checkout'. This opens the way for us to stop calling the
post-checkout hook in the future. Not running 'git checkout' means
that 'rebase -i/m' no longer recurse submodules when checking out
'onto' (thanks to Philippe Blain for pointing this out). As the rest
of rebase does not know what to do with submodules this is probably a
good thing. When using merge-ort rebase ought be able to handle
submodules correctly if it parsed the submodule config, such a change
is left for a future patch series.
The "apply" based rebase has avoided forking git checkout
since ac7f467fef ("builtin/rebase: support running "git rebase
<upstream>"", 2018-08-07). The code that handles the checkout was
moved into libgit by b309a97108 ("reset: extract reset_head() from
rebase", 2020-04-07).
Signed-off-by: Phillip Wood <redacted>
---
sequencer.c | 38 +++++++++++---------------------------
1 file changed, 11 insertions(+), 27 deletions(-)
From: Phillip Wood via GitGitGadget <hidden> Date: 2021-10-01 10:07:23
From: Phillip Wood <redacted>
If ORIG_HEAD is not set by passing RESET_ORIG_HEAD then there is no
need to pass anything for reflog_orig_head. In addition to the callers
fixed in this commit move_to_original_branch() also passes
reflog_orig_head without setting ORIG_HEAD. That caller is mistakenly
passing the message it wants to put in the branch reflog which is not
currently possible so we delay fixing that caller until we can pass
the message as the branch reflog.
Signed-off-by: Phillip Wood <redacted>
---
builtin/rebase.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
@@ -881,7 +881,7 @@ static int run_am(struct rebase_options *opts)reset_head(the_repository,&opts->orig_head,opts->head_name,0,-"HEAD",NULL,DEFAULT_REFLOG_ACTION);+NULL,NULL,DEFAULT_REFLOG_ACTION);error(_("\ngit encountered an error while preparing the ""patches to replay\n""these revisions:\n"
From: Eric Sunshine <hidden> Date: 2021-10-01 22:47:35
On Fri, Oct 1, 2021 at 6:06 AM Phillip Wood via GitGitGadget
[off-list ref] wrote:
The reset bit should only be set if flags contains RESET_HEAD_HARD.
The test for `!deatch_head` dates back to the original implementation
s/deatch_head/detach_head/
of reset_head() in ac7f467fef ("builtin/rebase: support running "git
rebase <upstream>"", 2018-08-07) and was correct until e65123a71d
("builtin rebase: support `git rebase <upstream> <switch-to>`",
2018-09-04) started using reset_head() to checkout <switch-to> when
fast-forwarding.
Signed-off-by: Phillip Wood <redacted>
From: Phillip Wood via GitGitGadget <hidden> Date: 2021-12-08 14:58:07
From: Phillip Wood <redacted>
This code is heavily indented and it will be convenient later in the
series to have it in its own function.
Signed-off-by: Phillip Wood <redacted>
---
builtin/rebase.c | 33 +++++++++++++++++++--------------
1 file changed, 19 insertions(+), 14 deletions(-)
@@ -812,6 +812,23 @@ static int rebase_config(const char *var, const char *value, void *data)returngit_default_config(var,value,data);}+staticintcheckout_up_to_date(structrebase_options*options)+{+structstrbufbuf=STRBUF_INIT;+intret=0;++strbuf_addf(&buf,"%s: checkout %s",+getenv(GIT_REFLOG_ACTION_ENVIRONMENT),+options->switch_to);+if(reset_head(the_repository,&options->orig_head,"checkout",+options->head_name,RESET_HEAD_RUN_POST_CHECKOUT_HOOK,+NULL,buf.buf,DEFAULT_REFLOG_ACTION)<0)+ret=error(_("could not switch to %s"),options->switch_to);+strbuf_release(&buf);++returnret;+}+/**Determineswhetherthecommitsinfrom..toarelinear,i.e.contain*nomergecommits.Thisfunction*expects*`from`tobeanancestorof
@@ -1673,21 +1690,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)if(!(options.flags&REBASE_FORCE)){/* Lazily switch to the target branch if needed... */if(options.switch_to){-strbuf_reset(&buf);-strbuf_addf(&buf,"%s: checkout %s",-getenv(GIT_REFLOG_ACTION_ENVIRONMENT),-options.switch_to);-if(reset_head(the_repository,-&options.orig_head,"checkout",-options.head_name,-RESET_HEAD_RUN_POST_CHECKOUT_HOOK,-NULL,buf.buf,-DEFAULT_REFLOG_ACTION)<0){-ret=error(_("could not switch to "-"%s"),-options.switch_to);+ret=checkout_up_to_date(&options);+if(ret)gotocleanup;-}}if(!(options.flags&REBASE_NO_QUIET))
From: Phillip Wood via GitGitGadget <hidden> Date: 2021-12-08 14:58:08
From: Phillip Wood <redacted>
These tests only test the default backend and do not check that the
arguments passed to the hook are correct. Fix this by running the
tests with both backends and adding checks for the hook arguments.
Signed-off-by: Phillip Wood <redacted>
---
t/t5403-post-checkout-hook.sh | 42 ++++++++++++++++++++++-------------
1 file changed, 26 insertions(+), 16 deletions(-)
@@ -49,23 +49,33 @@ test_expect_success 'post-checkout receives the right args when not switching brtest$old=$new&&test$flag=0'-test_expect_success'post-checkout is triggered on rebase''-test_when_finished"rm -f .git/post-checkout.args"&&-gitcheckout-brebase-testmain&&-rm-f.git/post-checkout.args&&-gitrebaserebase-on-me&&-readoldnewflag<.git/post-checkout.args&&-test$old!=$new&&test$flag=1-'+test_rebase(){+args="$*"&&+test_expect_success"post-checkout is triggered on rebase $args"'+test_when_finished"rm -f .git/post-checkout.args"&&+gitcheckout-Brebase-testmain&&+rm-f.git/post-checkout.args&&+gitrebase$argsrebase-on-me&&+readoldnewflag<.git/post-checkout.args&&+test_cmp_revmain$old&&+test_cmp_revrebase-on-me$new&&+test$flag=1+'-test_expect_success'post-checkout is triggered on rebase with fast-forward''-test_when_finished"rm -f .git/post-checkout.args"&&-gitcheckout-bff-rebase-testrebase-on-me^&&-rm-f.git/post-checkout.args&&-gitrebaserebase-on-me&&-readoldnewflag<.git/post-checkout.args&&-test$old!=$new&&test$flag=1-'+test_expect_success"post-checkout is triggered on rebase $args with fast-forward"'+test_when_finished"rm -f .git/post-checkout.args"&&+gitcheckout-Bff-rebase-testrebase-on-me^&&+rm-f.git/post-checkout.args&&+gitrebase$argsrebase-on-me&&+readoldnewflag<.git/post-checkout.args&&+test_cmp_revrebase-on-me^$old&&+test_cmp_revrebase-on-me$new&&+test$flag=1+'+}++test_rebase--apply&&+test_rebase--merge test_expect_success'post-checkout hook is triggered by clone''mkdir-ptemplates/hooks&&
From: Phillip Wood via GitGitGadget <hidden> Date: 2021-12-08 14:58:08
Thanks for the comments on V1. I have tried to improve the commit messages
to explain better the motivation and implications of the changes in this
series and I have added some more tests. I have rebased onto v2.34.0 to
avoid some merges conflicts.
Changes since V1:
* Patch 1 - unchanged.
* Patches 2, 3 - these are new and fix an bug I noticed while adding a test
to patch 4.
* Patches 4, 5 - improved commit messages and added tests.
* Patch 6 - reworded commit message.
* Patch 7 - split out some changes that used to be in patch 9.
* Patch 8 - in principle the same but the range-diff is noisy due to the
addition of patch 3.
* Patch 9 - reworded commit message.
* Patch 10 - unchanged.
* Patch 11 - reworded commit message and a couple of comments.
* Patch 12 - minor changes to comments.
* Patch 13 - cosmetic changes to commit message and tests.
* Patch 14 - cosmetic changes to commit message.
Cover letter for V1: Fix some issues with the implementation and use of
reset_head(). The last patch was previously posted as [1], I have updated
the commit message and rebased it onto the fixes in this series. There are a
couple of small conflicts merging this into seen, I think they should be
easy to resolve (in rebase.c take both sides in reset.c take the changed
lines from each side). These patches are based on pw/rebase-of-a-tag-fix
[1]
https://lore.kernel.org/git/39ad40c9297531a2d42b7263a1d41b1ecbc23c0a.1631108472.git.gitgitgadget@gmail.com/
Phillip Wood (14):
rebase: factor out checkout for up to date branch
t5403: refactor rebase post-checkout hook tests
rebase: pass correct arguments to post-checkout hook
rebase: do not remove untracked files on checkout
rebase --apply: don't run post-checkout hook if there is an error
reset_head(): remove action parameter
create_autostash(): remove unneeded parameter
reset_head(): factor out ref updates
reset_head(): make default_reflog_action optional
rebase: cleanup reset_head() calls
reset_head(): take struct rebase_head_opts
rebase --apply: fix reflog
rebase --apply: set ORIG_HEAD correctly
rebase -m: don't fork git checkout
builtin/merge.c | 6 +-
builtin/rebase.c | 101 +++++++++++++----------
reset.c | 149 ++++++++++++++++++++--------------
reset.h | 48 ++++++++++-
sequencer.c | 47 ++++-------
sequencer.h | 3 +-
t/t3406-rebase-message.sh | 23 ++++++
t/t3418-rebase-continue.sh | 26 ++++++
t/t5403-post-checkout-hook.sh | 67 +++++++++++----
9 files changed, 312 insertions(+), 158 deletions(-)
base-commit: cd3e606211bb1cf8bc57f7d76bab98cc17a150bc
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1049%2Fphillipwood%2Fwip%2Frebase-reset-head-fixes-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1049/phillipwood/wip/rebase-reset-head-fixes-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1049
Range-diff vs v1:
1: 4d3441c2b25 = 1: 0e84d00572e rebase: factor out checkout for up to date branch
-: ----------- > 2: a67a5a03b94 t5403: refactor rebase post-checkout hook tests
-: ----------- > 3: 07867760e68 rebase: pass correct arguments to post-checkout hook
2: c8f64113216 ! 4: 2b499704c8f reset_head(): fix checkout
@@ Metadata
Author: Phillip Wood [off-list ref]
## Commit message ##
- reset_head(): fix checkout
+ rebase: do not remove untracked files on checkout
- The reset bit should only be set if flags contains RESET_HEAD_HARD.
- The test for `!deatch_head` dates back to the original implementation
- of reset_head() in ac7f467fef ("builtin/rebase: support running "git
- rebase <upstream>"", 2018-08-07) and was correct until e65123a71d
+ If "git rebase [--apply|--merge] <upstream> <branch>" detects that
+ <upstream> is an ancestor of <branch> then it will fast-forward and
+ checkout <branch>. Normally a checkout or picking a commit during a
+ rebase will refuse to overwrite untracked files, however rebase does
+ overwrite untracked files when checking <branch>.
+
+ The fix is to only set reset in `unpack_tree_opts` if flags contains
+ `RESET_HEAD_HARD`. t5403 may seem like an odd home for the new test
+ but it will be extended in the next commit to check that the
+ post-checkout hook is not run when the checkout fails.
+
+ The test for `!deatch_head` dates back to the
+ original implementation of reset_head() in
+ ac7f467fef ("builtin/rebase: support running "git rebase <upstream>"",
+ 2018-08-07) and was correct until e65123a71d
("builtin rebase: support `git rebase <upstream> <switch-to>`",
2018-09-04) started using reset_head() to checkout <switch-to> when
fast-forwarding.
+ Note that 480d3d6bf9 ("Change unpack_trees' 'reset' flag into an
+ enum", 2021-09-27) also fixes this bug as it changes reset_head() to
+ never remove untracked files. I think this fix is still worthwhile as
+ it makes it clear that the same settings are used for detached and
+ non-detached checkouts.
+
Signed-off-by: Phillip Wood [off-list ref]
## reset.c ##
@@ reset.c: int reset_head(struct repository *r, struct object_id *oid, const char *action,
- unpack_tree_opts.update = 1;
unpack_tree_opts.merge = 1;
+ unpack_tree_opts.preserve_ignored = 0; /* FIXME: !overwrite_ignore */
init_checkout_metadata(&unpack_tree_opts.meta, switch_to_branch, oid, NULL);
- if (!detach_head)
+ if (reset_hard)
- unpack_tree_opts.reset = 1;
+ unpack_tree_opts.reset = UNPACK_RESET_PROTECT_UNTRACKED;
if (repo_read_index_unmerged(r) < 0) {
+
+ ## t/t5403-post-checkout-hook.sh ##
+@@ t/t5403-post-checkout-hook.sh: test_rebase () {
+ test_cmp_rev three $new &&
+ test $flag = 1
+ '
++
++ test_expect_success "rebase $args checkout does not remove untracked files" '
++ test_when_finished "test_might_fail git rebase --abort" &&
++ git update-ref refs/heads/rebase-fast-forward three &&
++ git checkout two &&
++ echo untracked >three.t &&
++ test_when_finished "rm three.t" &&
++ test_must_fail git rebase $args HEAD rebase-fast-forward 2>err &&
++ grep "untracked working tree files would be overwritten by checkout" err
++'
+ }
+
+ test_rebase --apply &&
3: 28872cbca68 ! 5: 04e7340a7e7 reset_head(): don't run checkout hook if there is an error
@@ Metadata
Author: Phillip Wood [off-list ref]
## Commit message ##
- reset_head(): don't run checkout hook if there is an error
+ rebase --apply: don't run post-checkout hook if there is an error
The hook should only be run if the worktree and refs were successfully
- updated.
+ updated. This primarily affects "rebase --apply" but also "rebase
+ --merge" when it fast-forwards.
Signed-off-by: Phillip Wood [off-list ref]
@@ reset.c: reset_head_refs:
- if (run_hook)
+ if (!ret && run_hook)
run_hook_le(NULL, "post-checkout",
- oid_to_hex(orig ? orig : null_oid()),
+ oid_to_hex(head ? head : null_oid()),
oid_to_hex(oid), "1", NULL);
+
+ ## t/t5403-post-checkout-hook.sh ##
+@@ t/t5403-post-checkout-hook.sh: test_rebase () {
+
+ test_expect_success "rebase $args checkout does not remove untracked files" '
+ test_when_finished "test_might_fail git rebase --abort" &&
++ test_when_finished "rm -f .git/post-checkout.args" &&
+ git update-ref refs/heads/rebase-fast-forward three &&
+ git checkout two &&
++ rm -f .git/post-checkout.args &&
+ echo untracked >three.t &&
+ test_when_finished "rm three.t" &&
+ test_must_fail git rebase $args HEAD rebase-fast-forward 2>err &&
+- grep "untracked working tree files would be overwritten by checkout" err
++ grep "untracked working tree files would be overwritten by checkout" err &&
++ test_path_is_missing .git/post-checkout.args
++
+ '
+ }
+
4: fbaf64d6b28 ! 6: 32ffa98c1bc reset_head(): remove action parameter
@@ Metadata
## Commit message ##
reset_head(): remove action parameter
- The action parameter is passed as the command name to
- setup_unpack_trees_porcelain(). All but two cases pass either
- "checkout" or "reset". The case that passes "reset --hard" should be
- passing "reset" instead. The case that passes "Fast-forwarded" is only
- updating HEAD and so does not call unpack_trees(). The value can be
- determined by checking whether flags contains RESET_HEAD_HARD so it
- does not need to be specified by the caller.
+ The only use of the action parameter is to setup the error messages
+ for unpack_trees(). All but two cases pass either "checkout" or
+ "reset". The case that passes "reset --hard" would be better passing
+ "reset" so that the error messages match the builtin reset command
+ like all the other callers that are doing a reset. The case that
+ passes "Fast-forwarded" is only updating HEAD and so the parameter is
+ unused in that case as it does not call unpack_trees(). The value to
+ pass to setup_unpack_trees_porcelain() can be determined by checking
+ whether flags contains RESET_HEAD_HARD without the caller having to
+ specify it.
Signed-off-by: Phillip Wood [off-list ref]
@@ reset.c: int reset_head(struct repository *r, struct object_id *oid, const char
+ const char *action, *reflog_action;
struct strbuf msg = STRBUF_INIT;
size_t prefix_len;
- struct object_id *orig = NULL, oid_orig,
+ struct object_id *old_orig = NULL, oid_old_orig;
@@ reset.c: int reset_head(struct repository *r, struct object_id *oid, const char *action,
if (refs_only)
goto reset_head_refs;
-: ----------- > 7: 341fe183c18 create_autostash(): remove unneeded parameter
5: 0744c3d143b ! 8: 29e06e7d36d reset_head(): factor out ref updates
@@ Metadata
## Commit message ##
reset_head(): factor out ref updates
- In the next commit we will stop trying to update HEAD when we are just
- clearing changes from the working tree. Move the code that updates the
- refs to its own function in preparation for that.
+ In the next commit we will stop trying to update HEAD when we are
+ removing uncommitted changes from the working tree. Move the code that
+ updates the refs to its own function in preparation for that.
Signed-off-by: Phillip Wood [off-list ref]
@@ reset.c
#include "unpack-trees.h"
+static int update_refs(const struct object_id *oid, const char *switch_to_branch,
-+ const char *reflog_head, const char *reflog_orig_head,
++ const struct object_id *head, const char *reflog_head,
++ const char *reflog_orig_head,
+ const char *default_reflog_action, unsigned flags)
+{
+ unsigned detach_head = flags & RESET_HEAD_DETACH;
+ unsigned run_hook = flags & RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
+ unsigned update_orig_head = flags & RESET_ORIG_HEAD;
-+ struct object_id *orig = NULL, oid_orig, *old_orig = NULL, oid_old_orig;
++ struct object_id *old_orig = NULL, oid_old_orig;
+ struct strbuf msg = STRBUF_INIT;
+ const char *reflog_action;
+ size_t prefix_len;
@@ reset.c
+ 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 (head) {
+ if (!reflog_orig_head) {
+ strbuf_addstr(&msg, "updating ORIG_HEAD");
+ reflog_orig_head = msg.buf;
+ }
-+ update_ref(reflog_orig_head, "ORIG_HEAD", orig,
++ update_ref(reflog_orig_head, "ORIG_HEAD", head,
+ old_orig, 0, UPDATE_REFS_MSG_ON_ERR);
+ } else if (old_orig)
+ delete_ref(NULL, "ORIG_HEAD", old_orig, 0);
@@ reset.c
+ reflog_head = msg.buf;
+ }
+ if (!switch_to_branch)
-+ ret = update_ref(reflog_head, "HEAD", oid, orig,
++ ret = update_ref(reflog_head, "HEAD", oid, head,
+ detach_head ? REF_NO_DEREF : 0,
+ UPDATE_REFS_MSG_ON_ERR);
+ else {
@@ reset.c
+ }
+ if (!ret && run_hook)
+ run_hook_le(NULL, "post-checkout",
-+ oid_to_hex(orig ? orig : null_oid()),
++ oid_to_hex(head ? head : null_oid()),
+ oid_to_hex(oid), "1", NULL);
+ strbuf_release(&msg);
+ return ret;
@@ reset.c
- unsigned run_hook = flags & RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
unsigned refs_only = flags & RESET_HEAD_REFS_ONLY;
- unsigned update_orig_head = flags & RESET_ORIG_HEAD;
- struct object_id head_oid;
+ struct object_id *head = NULL, head_oid;
struct tree_desc desc[2] = { { NULL }, { NULL } };
struct lock_file lock = LOCK_INIT;
struct unpack_trees_options unpack_tree_opts = { 0 };
@@ reset.c
- const char *action, *reflog_action;
- struct strbuf msg = STRBUF_INIT;
- size_t prefix_len;
-- struct object_id *orig = NULL, oid_orig,
-- *old_orig = NULL, oid_old_orig;
+- struct object_id *old_orig = NULL, oid_old_orig;
+ const char *action;
int ret = 0, nr = 0;
@@ reset.c: int reset_head(struct repository *r, struct object_id *oid,
if (refs_only)
- goto reset_head_refs;
-+ return update_refs(oid, switch_to_branch, reflog_head,
++ return update_refs(oid, switch_to_branch, head, reflog_head,
+ reflog_orig_head, default_reflog_action,
+ flags);
@@ reset.c: int reset_head(struct repository *r, struct object_id *oid,
- 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 (head) {
- if (!reflog_orig_head) {
- strbuf_addstr(&msg, "updating ORIG_HEAD");
- reflog_orig_head = msg.buf;
- }
-- update_ref(reflog_orig_head, "ORIG_HEAD", orig,
+- update_ref(reflog_orig_head, "ORIG_HEAD", head,
- old_orig, 0, UPDATE_REFS_MSG_ON_ERR);
- } else if (old_orig)
- delete_ref(NULL, "ORIG_HEAD", old_orig, 0);
@@ reset.c: int reset_head(struct repository *r, struct object_id *oid,
- reflog_head = msg.buf;
- }
- if (!switch_to_branch)
-- ret = update_ref(reflog_head, "HEAD", oid, orig,
+- ret = update_ref(reflog_head, "HEAD", oid, head,
- detach_head ? REF_NO_DEREF : 0,
- UPDATE_REFS_MSG_ON_ERR);
- else {
@@ reset.c: int reset_head(struct repository *r, struct object_id *oid,
- }
- if (!ret && run_hook)
- run_hook_le(NULL, "post-checkout",
-- oid_to_hex(orig ? orig : null_oid()),
+- oid_to_hex(head ? head : null_oid()),
- oid_to_hex(oid), "1", NULL);
-+ ret = update_refs(oid, switch_to_branch, reflog_head, reflog_orig_head,
-+ default_reflog_action, flags);
++ ret = update_refs(oid, switch_to_branch, head, reflog_head,
++ reflog_orig_head, default_reflog_action, flags);
leave_reset_head:
- strbuf_release(&msg);
6: 4503defe591 ! 9: 9d00a218daf reset_head(): make default_reflog_action optional
@@ Commit message
This parameter is only needed when a ref is going to be updated and
the caller does not pass an explicit reflog message. Callers that are
- just discarding changes in the working tree like create_autostash() do
- not update any refs so should not have to worry about passing this
- parameter.
+ only discarding uncommitted changes in the working tree such as such
+ as "rebase --skip" or create_autostash() do not update any refs so
+ should not have to worry about passing this parameter.
- Signed-off-by: Phillip Wood [off-list ref]
+ This change is not intended to have any user visible changes. The
+ pointer comparison between `oid` and `&head_oid` checks that the
+ caller did not pass an oid to be checked out. As no callers pass
+ RESET_HEAD_RUN_POST_CHECKOUT_HOOK without passing an oid there are
+ no changes to when the post-checkout hook is run. As update_ref() only
+ updates the ref if the oid passed to it differs from the current ref
+ there are no changes to when HEAD is updated.
- ## builtin/merge.c ##
-@@ builtin/merge.c: int cmd_merge(int argc, const char **argv, const char *prefix)
-
- if (autostash)
- create_autostash(the_repository,
-- git_path_merge_autostash(the_repository),
-- "merge");
-+ git_path_merge_autostash(the_repository));
- if (checkout_fast_forward(the_repository,
- &head_commit->object.oid,
- &commit->object.oid,
-@@ builtin/merge.c: int cmd_merge(int argc, const char **argv, const char *prefix)
-
- if (autostash)
- create_autostash(the_repository,
-- git_path_merge_autostash(the_repository),
-- "merge");
-+ git_path_merge_autostash(the_repository));
-
- /* We are going to make a new commit. */
- git_committer_info(IDENT_STRICT);
+ Signed-off-by: Phillip Wood [off-list ref]
## builtin/rebase.c ##
@@ builtin/rebase.c: static int move_to_original_branch(struct rebase_options *opts)
@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix
die(_("could not discard worktree changes"));
remove_branch_state(the_repository, 0);
if (read_basic_state(&options))
-@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
- die(_("could not read index"));
-
- if (options.autostash) {
-- create_autostash(the_repository, state_dir_path("autostash", &options),
-- DEFAULT_REFLOG_ACTION);
-+ create_autostash(the_repository,
-+ state_dir_path("autostash", &options));
- }
-
- if (require_clean_work_tree(the_repository, "rebase",
@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
options.head_name ? options.head_name : "detached HEAD",
oid_to_hex(&options.onto->object.oid));
@@ reset.c: int reset_head(struct repository *r, struct object_id *oid,
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 object_id *head = NULL, head_oid;
struct tree_desc desc[2] = { { NULL }, { NULL } };
struct lock_file lock = LOCK_INIT;
@@ reset.c: int reset_head(struct repository *r, struct object_id *oid,
goto leave_reset_head;
}
-- ret = update_refs(oid, switch_to_branch, reflog_head, reflog_orig_head,
-- default_reflog_action, flags);
+- ret = update_refs(oid, switch_to_branch, head, reflog_head,
+- reflog_orig_head, default_reflog_action, flags);
+ if (oid != &head_oid || update_orig_head || switch_to_branch)
-+ ret = update_refs(oid, switch_to_branch, reflog_head,
++ ret = update_refs(oid, switch_to_branch, head, reflog_head,
+ reflog_orig_head, default_reflog_action,
+ flags);
@@ reset.c: int reset_head(struct repository *r, struct object_id *oid,
rollback_lock_file(&lock);
## sequencer.c ##
-@@ sequencer.c: static enum todo_command peek_command(struct todo_list *todo_list, int offset)
- return -1;
- }
-
--void create_autostash(struct repository *r, const char *path,
-- const char *default_reflog_action)
-+void create_autostash(struct repository *r, const char *path)
-+
- {
- struct strbuf buf = STRBUF_INIT;
- struct lock_file lock_file = LOCK_INIT;
-@@ sequencer.c: void create_autostash(struct repository *r, const char *path,
+@@ sequencer.c: void create_autostash(struct repository *r, const char *path)
write_file(path, "%s", oid_to_hex(&oid));
printf(_("Created autostash: %s\n"), buf.buf);
if (reset_head(r, NULL, NULL, RESET_HEAD_HARD, NULL, NULL,
-- default_reflog_action) < 0)
+- "") < 0)
+ NULL) < 0)
die(_("could not reset --hard"));
if (discard_index(r->index) < 0 ||
-
- ## sequencer.h ##
-@@ sequencer.h: void commit_post_rewrite(struct repository *r,
- const struct commit *current_head,
- const struct object_id *new_head);
-
--void create_autostash(struct repository *r, const char *path,
-- const char *default_reflog_action);
-+void create_autostash(struct repository *r, const char *path);
- int save_autostash(const char *path);
- int apply_autostash(const char *path);
- int apply_autostash_oid(const char *stash_oid);
7: 5ffc7e64ff1 = 10: 5ea636009e7 rebase: cleanup reset_head() calls
8: 267e074e6db ! 11: 24b0566aba5 reset_head(): take struct rebase_head_opts
@@ Metadata
## Commit message ##
reset_head(): take struct rebase_head_opts
- This function already takes a confusingly large number of parameters
- some of which are optional or not always required. The following
- commits will add a couple more parameters so change it to take a
- struct of options first.
+ This function takes a confusingly large number of parameters which
+ makes it difficult to remember which order to pass them in. The
+ following commits will add a couple more parameters which makes the
+ problem worse. To address this change the function to take a struct of
+ options. Using a struct means that it is no longer necessary to
+ remember which order to pass the parameters in and anyone reading the
+ code can easily see which value is passed to each parameter.
Signed-off-by: Phillip Wood [off-list ref]
## builtin/rebase.c ##
-@@ builtin/rebase.c: static void add_var(struct strbuf *buf, const char *name, const char *value)
+@@ builtin/rebase.c: static int finish_rebase(struct rebase_options *opts)
static int move_to_original_branch(struct rebase_options *opts)
{
struct strbuf orig_head_reflog = STRBUF_INIT, head_reflog = STRBUF_INIT;
@@ builtin/rebase.c: static int rebase_config(const char *var, const char *value, v
strbuf_release(&buf);
@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
- char *squash_onto_name = NULL;
int reschedule_failed_exec = -1;
int allow_preemptive_ff = 1;
+ int preserve_merges_selected = 0;
+ struct reset_head_opts ropts = { 0 };
struct option builtin_rebase_options[] = {
OPT_STRING(0, "onto", &options.onto_name,
@@ reset.c
#include "unpack-trees.h"
-static int update_refs(const struct object_id *oid, const char *switch_to_branch,
-- const char *reflog_head, const char *reflog_orig_head,
+- const struct object_id *head, const char *reflog_head,
+- const char *reflog_orig_head,
- const char *default_reflog_action, unsigned flags)
+static int update_refs(const struct reset_head_opts *opts,
-+ const struct object_id *oid)
++ const struct object_id *oid,
++ const struct object_id *head)
{
- unsigned detach_head = flags & RESET_HEAD_DETACH;
- unsigned run_hook = flags & RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
@@ reset.c
+ const char *reflog_head = opts->head_msg;
+ const char *reflog_orig_head = opts->orig_head_msg;
+ const char *default_reflog_action = opts->default_reflog_action;
- struct object_id *orig = NULL, oid_orig, *old_orig = NULL, oid_old_orig;
+ struct object_id *old_orig = NULL, oid_old_orig;
struct strbuf msg = STRBUF_INIT;
const char *reflog_action;
@@ reset.c: static int update_refs(const struct object_id *oid, const char *switch_to_branch
@@ reset.c: static int update_refs(const struct object_id *oid, const char *switch_
+ unsigned reset_hard = opts->flags & RESET_HEAD_HARD;
+ unsigned refs_only = opts->flags & RESET_HEAD_REFS_ONLY;
+ unsigned update_orig_head = opts->flags & RESET_ORIG_HEAD;
- struct object_id head_oid;
+ struct object_id *head = NULL, head_oid;
struct tree_desc desc[2] = { { NULL }, { NULL } };
struct lock_file lock = LOCK_INIT;
@@ reset.c: int reset_head(struct repository *r, struct object_id *oid,
oid = &head_oid;
if (refs_only)
-- return update_refs(oid, switch_to_branch, reflog_head,
+- return update_refs(oid, switch_to_branch, head, reflog_head,
- reflog_orig_head, default_reflog_action,
- flags);
-+ return update_refs(opts, oid);
++ return update_refs(opts, oid, head);
action = reset_hard ? "reset" : "checkout";
setup_unpack_trees_porcelain(&unpack_tree_opts, action);
@@ reset.c: int reset_head(struct repository *r, struct object_id *oid,
}
if (oid != &head_oid || update_orig_head || switch_to_branch)
-- ret = update_refs(oid, switch_to_branch, reflog_head,
+- ret = update_refs(oid, switch_to_branch, head, reflog_head,
- reflog_orig_head, default_reflog_action,
- flags);
-+ ret = update_refs(opts, oid);
++ ret = update_refs(opts, oid, head);
leave_reset_head:
rollback_lock_file(&lock);
## reset.h ##
@@
+
+ #define GIT_REFLOG_ACTION_ENVIRONMENT "GIT_REFLOG_ACTION"
+
++/* Request a detached checkout */
+ #define RESET_HEAD_DETACH (1<<0)
++/* Request a reset rather than a checkout */
+ #define RESET_HEAD_HARD (1<<1)
++/* Run the post-checkout hook */
+ #define RESET_HEAD_RUN_POST_CHECKOUT_HOOK (1<<2)
++/* Only update refs, do not touch the worktree */
#define RESET_HEAD_REFS_ONLY (1<<3)
++/* Update ORIG_HEAD as well as HEAD */
#define RESET_ORIG_HEAD (1<<4)
-int reset_head(struct repository *r, struct object_id *oid,
@@ reset.h
- const char *reflog_orig_head, const char *reflog_head,
- const char *default_reflog_action);
+struct reset_head_opts {
-+ /* The oid of the commit to checkout/reset to. Defaults to HEAD */
++ /*
++ * The commit to checkout/reset to. Defaults to HEAD.
++ */
+ const struct object_id *oid;
-+ /* Optional branch to switch to */
++ /*
++ * Optional branch to switch to.
++ */
+ const char *branch;
-+ /* Flags defined above */
++ /*
++ * Flags defined above.
++ */
+ unsigned flags;
-+ /*
-+ * Optional reflog message for HEAD, if this is not set then
-+ * default_reflog_action must be.
-+ */
++ /*
++ * Optional reflog message for HEAD, if this omitted but oid or branch
++ * are given then default_reflog_action must be given.
++ */
+ const char *head_msg;
+ /*
-+ * Optional reflog message for ORIG_HEAD, if this is not set and flags
-+ * contains RESET_ORIG_HEAD then default_reflog_action must be set.
++ * Optional reflog message for ORIG_HEAD, if this omitted and flags
++ * contains RESET_ORIG_HEAD then default_reflog_action must be given.
+ */
+ const char *orig_head_msg;
+ /*
9: cdb0de221d5 ! 12: dc5d11291e7 rebase --apply: fix reflog
@@ Commit message
Signed-off-by: Phillip Wood [off-list ref]
## builtin/rebase.c ##
-@@ builtin/rebase.c: static void add_var(struct strbuf *buf, const char *name, const char *value)
+@@ builtin/rebase.c: static int finish_rebase(struct rebase_options *opts)
static int move_to_original_branch(struct rebase_options *opts)
{
@@ reset.c: int reset_head(struct repository *r, const struct reset_head_opts *opts
## reset.h ##
@@ reset.h: struct reset_head_opts {
- const char *branch;
- /* Flags defined above */
+ * Flags defined above.
+ */
unsigned flags;
-+ /* Optional reflog message for branch, defaults to head_msg. */
++ /*
++ * Optional reflog message for branch, defaults to head_msg.
++ */
+ const char *branch_msg;
- /*
- * Optional reflog message for HEAD, if this is not set then
- * default_reflog_action must be.
+ /*
+ * Optional reflog message for HEAD, if this omitted but oid or branch
+ * are given then default_reflog_action must be given.
## t/t3406-rebase-message.sh ##
@@ t/t3406-rebase-message.sh: test_expect_success 'GIT_REFLOG_ACTION' '
10: e8884efcc83 ! 13: 45a5b5e9818 rebase --apply: set ORIG_HEAD correctly
@@ Commit message
At the start of a rebase ORIG_HEAD is updated to tip of the branch
being rebased. Unfortunately reset_head() always uses the current
value of HEAD for this which is incorrect if the rebase is started
- with 'git rebase <upstream> <branch>' as in that case ORIG_HEAD should
+ with "git rebase <upstream> <branch>" as in that case ORIG_HEAD should
be updated to <branch>. This only affects the "apply" backend as the
"merge" backend does not yet use reset_head() for the initial
checkout. Fix this by passing in orig_head when calling reset_head()
@@ reset.c: static int update_refs(const struct reset_head_opts *opts,
strbuf_addstr(&msg, "updating ORIG_HEAD");
reflog_orig_head = msg.buf;
}
-- update_ref(reflog_orig_head, "ORIG_HEAD", orig,
+- update_ref(reflog_orig_head, "ORIG_HEAD", head,
+ update_ref(reflog_orig_head, "ORIG_HEAD",
-+ orig_head ? orig_head : orig,
++ orig_head ? orig_head : head,
old_orig, 0, UPDATE_REFS_MSG_ON_ERR);
} else if (old_orig)
delete_ref(NULL, "ORIG_HEAD", old_orig, 0);
## reset.h ##
-@@
- struct reset_head_opts {
- /* The oid of the commit to checkout/reset to. Defaults to HEAD */
+@@ reset.h: struct reset_head_opts {
+ * The commit to checkout/reset to. Defaults to HEAD.
+ */
const struct object_id *oid;
-+ /* Optional commit when setting ORIG_HEAD. Defaults to HEAD */
++ /*
++ * Optional value to set ORIG_HEAD. Defaults to HEAD.
++ */
+ const struct object_id *orig_head;
- /* Optional branch to switch to */
- const char *branch;
- /* Flags defined above */
+ /*
+ * Optional branch to switch to.
+ */
## t/t3418-rebase-continue.sh ##
@@ t/t3418-rebase-continue.sh: test_expect_success 'there is no --no-reschedule-failed-exec in an ongoing rebas
test_expect_code 129 git rebase --edit-todo --no-reschedule-failed-exec
'
-+test_orig_head_helper() {
++test_orig_head_helper () {
+ test_when_finished 'git rebase --abort &&
+ git checkout topic &&
+ git reset --hard commit-new-file-F2-on-topic-branch' &&
@@ t/t3418-rebase-continue.sh: test_expect_success 'there is no --no-reschedule-fai
+ test_cmp_rev ORIG_HEAD commit-new-file-F2-on-topic-branch
+}
+
-+test_orig_head() {
++test_orig_head () {
+ type=$1
+ test_expect_success "rebase $type sets ORIG_HEAD correctly" '
+ git checkout topic &&
11: 2c8c60c3f31 ! 14: 3f64b9274b5 rebase -m: don't fork git checkout
@@ Commit message
rebase -m: don't fork git checkout
Now that reset_head() can handle the initial checkout of onto
- correctly use it in the "merge" backend instead of forking 'git
- checkout'. This opens the way for us to stop calling the
- post-checkout hook in the future. Not running 'git checkout' means
- that 'rebase -i/m' no longer recurse submodules when checking out
- 'onto' (thanks to Philippe Blain for pointing this out). As the rest
+ correctly use it in the "merge" backend instead of forking "git
+ checkout". This opens the way for us to stop calling the
+ post-checkout hook in the future. Not running "git checkout" means
+ that "rebase -i/m" no longer recurse submodules when checking out
+ "onto" (thanks to Philippe Blain for pointing this out). As the rest
of rebase does not know what to do with submodules this is probably a
good thing. When using merge-ort rebase ought be able to handle
submodules correctly if it parsed the submodule config, such a change
--
gitgitgadget
From: Phillip Wood via GitGitGadget <hidden> Date: 2021-12-08 14:58:10
From: Phillip Wood <redacted>
If a rebase started with "rebase [--apply|--merge] <upstream> <branch>"
detects that <upstream> is an ancestor of <branch> then it fast-forwards
and checks out <branch>. Unfortunately in that case it passed the null
oid as the first argument to the post-checkout hook rather than the oid
of HEAD.
A side effect of this change is that the call to update_ref() which
updates HEAD now always receives the old value of HEAD. This provides
protection against another process updating HEAD during the checkout.
Signed-off-by: Phillip Wood <redacted>
---
reset.c | 18 +++++++++---------
t/t5403-post-checkout-hook.sh | 13 +++++++++++++
2 files changed, 22 insertions(+), 9 deletions(-)
From: Phillip Wood via GitGitGadget <hidden> Date: 2021-12-08 14:58:11
From: Phillip Wood <redacted>
If "git rebase [--apply|--merge] <upstream> <branch>" detects that
<upstream> is an ancestor of <branch> then it will fast-forward and
checkout <branch>. Normally a checkout or picking a commit during a
rebase will refuse to overwrite untracked files, however rebase does
overwrite untracked files when checking <branch>.
The fix is to only set reset in `unpack_tree_opts` if flags contains
`RESET_HEAD_HARD`. t5403 may seem like an odd home for the new test
but it will be extended in the next commit to check that the
post-checkout hook is not run when the checkout fails.
The test for `!deatch_head` dates back to the
original implementation of reset_head() in
ac7f467fef ("builtin/rebase: support running "git rebase <upstream>"",
2018-08-07) and was correct until e65123a71d
("builtin rebase: support `git rebase <upstream> <switch-to>`",
2018-09-04) started using reset_head() to checkout <switch-to> when
fast-forwarding.
Note that 480d3d6bf9 ("Change unpack_trees' 'reset' flag into an
enum", 2021-09-27) also fixes this bug as it changes reset_head() to
never remove untracked files. I think this fix is still worthwhile as
it makes it clear that the same settings are used for detached and
non-detached checkouts.
Signed-off-by: Phillip Wood <redacted>
---
reset.c | 2 +-
t/t5403-post-checkout-hook.sh | 10 ++++++++++
2 files changed, 11 insertions(+), 1 deletion(-)
@@ -85,6 +85,16 @@ test_rebase () {test_cmp_revthree$new&&test$flag=1'++test_expect_success"rebase $args checkout does not remove untracked files"'+test_when_finished"test_might_fail git rebase --abort"&&+gitupdate-refrefs/heads/rebase-fast-forwardthree&&+gitcheckouttwo&&+echountracked>three.t&&+test_when_finished"rm three.t"&&+test_must_failgitrebase$argsHEADrebase-fast-forward2>err&&+grep"untracked working tree files would be overwritten by checkout"err+'} test_rebase--apply&&
From: Phillip Wood via GitGitGadget <hidden> Date: 2021-12-08 14:58:14
From: Phillip Wood <redacted>
The hook should only be run if the worktree and refs were successfully
updated. This primarily affects "rebase --apply" but also "rebase
--merge" when it fast-forwards.
Signed-off-by: Phillip Wood <redacted>
---
reset.c | 2 +-
t/t5403-post-checkout-hook.sh | 6 +++++-
2 files changed, 6 insertions(+), 2 deletions(-)
@@ -88,12 +88,16 @@ test_rebase () {test_expect_success"rebase $args checkout does not remove untracked files"'test_when_finished"test_might_fail git rebase --abort"&&+test_when_finished"rm -f .git/post-checkout.args"&&gitupdate-refrefs/heads/rebase-fast-forwardthree&&gitcheckouttwo&&+rm-f.git/post-checkout.args&&echountracked>three.t&&test_when_finished"rm three.t"&&test_must_failgitrebase$argsHEADrebase-fast-forward2>err&&-grep"untracked working tree files would be overwritten by checkout"err+grep"untracked working tree files would be overwritten by checkout"err&&+test_path_is_missing.git/post-checkout.args+'}
From: Phillip Wood via GitGitGadget <hidden> Date: 2021-12-08 14:58:16
From: Phillip Wood <redacted>
The only use of the action parameter is to setup the error messages
for unpack_trees(). All but two cases pass either "checkout" or
"reset". The case that passes "reset --hard" would be better passing
"reset" so that the error messages match the builtin reset command
like all the other callers that are doing a reset. The case that
passes "Fast-forwarded" is only updating HEAD and so the parameter is
unused in that case as it does not call unpack_trees(). The value to
pass to setup_unpack_trees_porcelain() can be determined by checking
whether flags contains RESET_HEAD_HARD without the caller having to
specify it.
Signed-off-by: Phillip Wood <redacted>
---
builtin/rebase.c | 14 +++++++-------
reset.c | 5 +++--
reset.h | 2 +-
sequencer.c | 3 +--
4 files changed, 12 insertions(+), 12 deletions(-)
@@ -583,7 +583,7 @@ static int move_to_original_branch(struct rebase_options *opts)opts->head_name,oid_to_hex(&opts->onto->object.oid));strbuf_addf(&head_reflog,"rebase finished: returning to %s",opts->head_name);-ret=reset_head(the_repository,NULL,"",opts->head_name,+ret=reset_head(the_repository,NULL,opts->head_name,RESET_HEAD_REFS_ONLY,orig_head_reflog.buf,head_reflog.buf,DEFAULT_REFLOG_ACTION);
@@ -674,7 +674,7 @@ static int run_am(struct rebase_options *opts)free(rebased_patches);strvec_clear(&am.args);-reset_head(the_repository,&opts->orig_head,"checkout",+reset_head(the_repository,&opts->orig_head,opts->head_name,0,"HEAD",NULL,DEFAULT_REFLOG_ACTION);error(_("\ngit encountered an error while preparing the "
@@ -820,7 +820,7 @@ static int checkout_up_to_date(struct rebase_options *options)strbuf_addf(&buf,"%s: checkout %s",getenv(GIT_REFLOG_ACTION_ENVIRONMENT),options->switch_to);-if(reset_head(the_repository,&options->orig_head,"checkout",+if(reset_head(the_repository,&options->orig_head,options->head_name,RESET_HEAD_RUN_POST_CHECKOUT_HOOK,NULL,buf.buf,DEFAULT_REFLOG_ACTION)<0)ret=error(_("could not switch to %s"),options->switch_to);
@@ -1272,7 +1272,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)rerere_clear(the_repository,&merge_rr);string_list_clear(&merge_rr,1);-if(reset_head(the_repository,NULL,"reset",NULL,RESET_HEAD_HARD,+if(reset_head(the_repository,NULL,NULL,RESET_HEAD_HARD,NULL,NULL,DEFAULT_REFLOG_ACTION)<0)die(_("could not discard worktree changes"));remove_branch_state(the_repository,0);
@@ -1290,7 +1290,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)if(read_basic_state(&options))exit(1);-if(reset_head(the_repository,&options.orig_head,"reset",+if(reset_head(the_repository,&options.orig_head,options.head_name,RESET_HEAD_HARD,NULL,NULL,DEFAULT_REFLOG_ACTION)<0)die(_("could not move back to %s"),
From: Phillip Wood via GitGitGadget <hidden> Date: 2021-12-08 14:58:17
From: Phillip Wood <redacted>
The default_reflog parameter of create_autostash() is passed to
reset_head(). However as creating a stash does not involve updating
any refs the parameter is not used by reset_head(). Removing the
parameter from create_autostash() simplifies the callers.
Signed-off-by: Phillip Wood <redacted>
---
builtin/merge.c | 6 ++----
builtin/rebase.c | 8 ++++----
sequencer.c | 5 ++---
sequencer.h | 3 +--
4 files changed, 9 insertions(+), 13 deletions(-)
@@ -1637,8 +1636,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)if(autostash)create_autostash(the_repository,-git_path_merge_autostash(the_repository),-"merge");+git_path_merge_autostash(the_repository));/* We are going to make a new commit. */git_committer_info(IDENT_STRICT);
From: Phillip Wood via GitGitGadget <hidden> Date: 2021-12-08 14:58:18
From: Phillip Wood <redacted>
In the next commit we will stop trying to update HEAD when we are
removing uncommitted changes from the working tree. Move the code that
updates the refs to its own function in preparation for that.
Signed-off-by: Phillip Wood <redacted>
---
reset.c | 110 +++++++++++++++++++++++++++++++-------------------------
1 file changed, 62 insertions(+), 48 deletions(-)
From: Phillip Wood via GitGitGadget <hidden> Date: 2021-12-08 14:58:20
From: Phillip Wood <redacted>
This parameter is only needed when a ref is going to be updated and
the caller does not pass an explicit reflog message. Callers that are
only discarding uncommitted changes in the working tree such as such
as "rebase --skip" or create_autostash() do not update any refs so
should not have to worry about passing this parameter.
This change is not intended to have any user visible changes. The
pointer comparison between `oid` and `&head_oid` checks that the
caller did not pass an oid to be checked out. As no callers pass
RESET_HEAD_RUN_POST_CHECKOUT_HOOK without passing an oid there are
no changes to when the post-checkout hook is run. As update_ref() only
updates the ref if the oid passed to it differs from the current ref
there are no changes to when HEAD is updated.
Signed-off-by: Phillip Wood <redacted>
---
builtin/rebase.c | 10 ++++------
reset.c | 16 ++++++++++++----
sequencer.c | 2 +-
3 files changed, 17 insertions(+), 11 deletions(-)
@@ -585,8 +585,7 @@ static int move_to_original_branch(struct rebase_options *opts)opts->head_name);ret=reset_head(the_repository,NULL,opts->head_name,RESET_HEAD_REFS_ONLY,-orig_head_reflog.buf,head_reflog.buf,-DEFAULT_REFLOG_ACTION);+orig_head_reflog.buf,head_reflog.buf,NULL);strbuf_release(&orig_head_reflog);strbuf_release(&head_reflog);
@@ -822,7 +821,7 @@ static int checkout_up_to_date(struct rebase_options *options)options->switch_to);if(reset_head(the_repository,&options->orig_head,options->head_name,RESET_HEAD_RUN_POST_CHECKOUT_HOOK,-NULL,buf.buf,DEFAULT_REFLOG_ACTION)<0)+NULL,buf.buf,NULL)<0)ret=error(_("could not switch to %s"),options->switch_to);strbuf_release(&buf);
@@ -1273,7 +1272,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)string_list_clear(&merge_rr,1);if(reset_head(the_repository,NULL,NULL,RESET_HEAD_HARD,-NULL,NULL,DEFAULT_REFLOG_ACTION)<0)+NULL,NULL,NULL)<0)die(_("could not discard worktree changes"));remove_branch_state(the_repository,0);if(read_basic_state(&options))
@@ -22,8 +22,13 @@ static int update_refs(const struct object_id *oid, const char *switch_to_branchsize_tprefix_len;intret;-reflog_action=getenv(GIT_REFLOG_ACTION_ENVIRONMENT);-strbuf_addf(&msg,"%s: ",reflog_action?reflog_action:default_reflog_action);+if((update_orig_head&&!reflog_orig_head)||!reflog_head){+if(!default_reflog_action)+BUG("default_reflog_action must be given when reflog messages are omitted");+reflog_action=getenv(GIT_REFLOG_ACTION_ENVIRONMENT);+strbuf_addf(&msg,"%s: ",reflog_action?reflog_action:+default_reflog_action);+}prefix_len=msg.len;if(update_orig_head){
From: Phillip Wood via GitGitGadget <hidden> Date: 2021-12-08 14:58:21
From: Phillip Wood <redacted>
If ORIG_HEAD is not set by passing RESET_ORIG_HEAD then there is no
need to pass anything for reflog_orig_head. In addition to the callers
fixed in this commit move_to_original_branch() also passes
reflog_orig_head without setting ORIG_HEAD. That caller is mistakenly
passing the message it wants to put in the branch reflog which is not
currently possible so we delay fixing that caller until we can pass
the message as the branch reflog.
Signed-off-by: Phillip Wood <redacted>
---
builtin/rebase.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
@@ -675,7 +675,7 @@ static int run_am(struct rebase_options *opts)reset_head(the_repository,&opts->orig_head,opts->head_name,0,-"HEAD",NULL,DEFAULT_REFLOG_ACTION);+NULL,NULL,DEFAULT_REFLOG_ACTION);error(_("\ngit encountered an error while preparing the ""patches to replay\n""these revisions:\n"
From: Phillip Wood via GitGitGadget <hidden> Date: 2021-12-08 14:58:22
From: Phillip Wood <redacted>
This function takes a confusingly large number of parameters which
makes it difficult to remember which order to pass them in. The
following commits will add a couple more parameters which makes the
problem worse. To address this change the function to take a struct of
options. Using a struct means that it is no longer necessary to
remember which order to pass the parameters in and anyone reading the
code can easily see which value is passed to each parameter.
Signed-off-by: Phillip Wood <redacted>
---
builtin/rebase.c | 57 ++++++++++++++++++++++++++++++------------------
reset.c | 38 +++++++++++++++-----------------
reset.h | 40 +++++++++++++++++++++++++++++----
sequencer.c | 5 ++---
4 files changed, 92 insertions(+), 48 deletions(-)
@@ -571,6 +571,7 @@ static int finish_rebase(struct rebase_options *opts)staticintmove_to_original_branch(structrebase_options*opts){structstrbuforig_head_reflog=STRBUF_INIT,head_reflog=STRBUF_INIT;+structreset_head_optsropts={0};intret;if(!opts->head_name)
@@ -583,9 +584,11 @@ static int move_to_original_branch(struct rebase_options *opts)opts->head_name,oid_to_hex(&opts->onto->object.oid));strbuf_addf(&head_reflog,"rebase finished: returning to %s",opts->head_name);-ret=reset_head(the_repository,NULL,opts->head_name,-RESET_HEAD_REFS_ONLY,-orig_head_reflog.buf,head_reflog.buf,NULL);+ropts.branch=opts->head_name;+ropts.flags=RESET_HEAD_REFS_ONLY;+ropts.orig_head_msg=orig_head_reflog.buf;+ropts.head_msg=head_reflog.buf;+ret=reset_head(the_repository,&ropts);strbuf_release(&orig_head_reflog);strbuf_release(&head_reflog);
@@ -669,13 +672,15 @@ static int run_am(struct rebase_options *opts)status=run_command(&format_patch);if(status){+structreset_head_optsropts={0};unlink(rebased_patches);free(rebased_patches);strvec_clear(&am.args);-reset_head(the_repository,&opts->orig_head,-opts->head_name,0,-NULL,NULL,DEFAULT_REFLOG_ACTION);+ropts.oid=&opts->orig_head;+ropts.branch=opts->head_name;+ropts.default_reflog_action=DEFAULT_REFLOG_ACTION;+reset_head(the_repository,&ropts);error(_("\ngit encountered an error while preparing the ""patches to replay\n""these revisions:\n"
@@ -814,14 +819,17 @@ static int rebase_config(const char *var, const char *value, void *data)staticintcheckout_up_to_date(structrebase_options*options){structstrbufbuf=STRBUF_INIT;+structreset_head_optsropts={0};intret=0;strbuf_addf(&buf,"%s: checkout %s",getenv(GIT_REFLOG_ACTION_ENVIRONMENT),options->switch_to);-if(reset_head(the_repository,&options->orig_head,-options->head_name,RESET_HEAD_RUN_POST_CHECKOUT_HOOK,-NULL,buf.buf,NULL)<0)+ropts.oid=&options->orig_head;+ropts.branch=options->head_name;+ropts.flags=RESET_HEAD_RUN_POST_CHECKOUT_HOOK;+ropts.head_msg=buf.buf;+if(reset_head(the_repository,&ropts)<0)ret=error(_("could not switch to %s"),options->switch_to);strbuf_release(&buf);
@@ -1270,9 +1279,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)rerere_clear(the_repository,&merge_rr);string_list_clear(&merge_rr,1);--if(reset_head(the_repository,NULL,NULL,RESET_HEAD_HARD,-NULL,NULL,NULL)<0)+ropts.flags=RESET_HEAD_HARD;+if(reset_head(the_repository,&ropts)<0)die(_("could not discard worktree changes"));remove_branch_state(the_repository,0);if(read_basic_state(&options))
@@ -1289,9 +1297,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)if(read_basic_state(&options))exit(1);-if(reset_head(the_repository,&options.orig_head,-options.head_name,RESET_HEAD_HARD,-NULL,NULL,DEFAULT_REFLOG_ACTION)<0)+ropts.oid=&options.orig_head;+ropts.branch=options.head_name;+ropts.flags=RESET_HEAD_HARD;+ropts.default_reflog_action=DEFAULT_REFLOG_ACTION;+if(reset_head(the_repository,&ropts)<0)die(_("could not move back to %s"),oid_to_hex(&options.orig_head));remove_branch_state(the_repository,0);
@@ -1758,10 +1768,12 @@ 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(the_repository,&options.onto->object.oid,NULL,-RESET_HEAD_DETACH|RESET_ORIG_HEAD|-RESET_HEAD_RUN_POST_CHECKOUT_HOOK,-NULL,msg.buf,DEFAULT_REFLOG_ACTION))+ropts.oid=&options.onto->object.oid;+ropts.flags=RESET_HEAD_DETACH|RESET_ORIG_HEAD|+RESET_HEAD_RUN_POST_CHECKOUT_HOOK;+ropts.head_msg=msg.buf;+ropts.default_reflog_action=DEFAULT_REFLOG_ACTION;+if(reset_head(the_repository,&ropts))die(_("Could not detach HEAD"));strbuf_release(&msg);
@@ -6,15 +6,47 @@#define GIT_REFLOG_ACTION_ENVIRONMENT "GIT_REFLOG_ACTION"+/* Request a detached checkout */#define RESET_HEAD_DETACH (1<<0)+/* Request a reset rather than a checkout */#define RESET_HEAD_HARD (1<<1)+/* Run the post-checkout hook */#define RESET_HEAD_RUN_POST_CHECKOUT_HOOK (1<<2)+/* Only update refs, do not touch the worktree */#define RESET_HEAD_REFS_ONLY (1<<3)+/* Update ORIG_HEAD as well as HEAD */#define RESET_ORIG_HEAD (1<<4)-intreset_head(structrepository*r,structobject_id*oid,-constchar*switch_to_branch,unsignedflags,-constchar*reflog_orig_head,constchar*reflog_head,-constchar*default_reflog_action);+structreset_head_opts{+/*+*Thecommittocheckout/resetto.DefaultstoHEAD.+*/+conststructobject_id*oid;+/*+*Optionalbranchtoswitchto.+*/+constchar*branch;+/*+*Flagsdefinedabove.+*/+unsignedflags;+/*+*OptionalreflogmessageforHEAD,ifthisomittedbutoidorbranch+*aregiventhendefault_reflog_actionmustbegiven.+*/+constchar*head_msg;+/*+*OptionalreflogmessageforORIG_HEAD,ifthisomittedandflags+*containsRESET_ORIG_HEADthendefault_reflog_actionmustbegiven.+*/+constchar*orig_head_msg;+/*+*Actiontouseindefaultreflogmessages,onlyrequiredifarefis+*beingupdatedandthereflogmessagesaboveareomitted.+*/+constchar*default_reflog_action;+};++intreset_head(structrepository*r,conststructreset_head_opts*opts);#endif
From: Phillip Wood via GitGitGadget <hidden> Date: 2021-12-08 14:58:23
From: Phillip Wood <redacted>
move_to_original_branch() passes the message intended for the branch
reflog as `orig_head_msg`. Fix this by adding a `branch_msg` member to
struct reset_head_opts and add a regression test. Note that these
reflog messages do not respect GIT_REFLOG_ACTION. They are not alone
in that and will be fixed in a future series.
The "merge" backend already has tests that check both the branch and
HEAD reflogs.
Signed-off-by: Phillip Wood <redacted>
---
builtin/rebase.c | 8 ++++----
reset.c | 12 ++++++++++--
reset.h | 4 ++++
t/t3406-rebase-message.sh | 23 +++++++++++++++++++++++
4 files changed, 41 insertions(+), 6 deletions(-)
@@ -16,6 +16,7 @@ static int update_refs(const struct reset_head_opts *opts,unsignedrun_hook=opts->flags&RESET_HEAD_RUN_POST_CHECKOUT_HOOK;unsignedupdate_orig_head=opts->flags&RESET_ORIG_HEAD;constchar*switch_to_branch=opts->branch;+constchar*reflog_branch=opts->branch_msg;constchar*reflog_head=opts->head_msg;constchar*reflog_orig_head=opts->orig_head_msg;constchar*default_reflog_action=opts->default_reflog_action;
@@ -58,8 +59,9 @@ static int update_refs(const struct reset_head_opts *opts,detach_head?REF_NO_DEREF:0,UPDATE_REFS_MSG_ON_ERR);else{-ret=update_ref(reflog_head,switch_to_branch,oid,-NULL,0,UPDATE_REFS_MSG_ON_ERR);+ret=update_ref(reflog_branch?reflog_branch:reflog_head,+switch_to_branch,oid,NULL,0,+UPDATE_REFS_MSG_ON_ERR);if(!ret)ret=create_symref("HEAD",switch_to_branch,reflog_head);
@@ -90,6 +92,12 @@ int reset_head(struct repository *r, const struct reset_head_opts *opts)if(switch_to_branch&&!starts_with(switch_to_branch,"refs/"))BUG("Not a fully qualified branch: '%s'",switch_to_branch);+if(opts->orig_head_msg&&!update_orig_head)+BUG("ORIG_HEAD reflog message given without updating ORIG_HEAD");++if(opts->branch_msg&&!opts->branch)+BUG("branch reflog message given without a branch");+if(!refs_only&&repo_hold_locked_index(r,&lock,LOCK_REPORT_ON_ERROR)<0){ret=-1;gotoleave_reset_head;
From: Phillip Wood via GitGitGadget <hidden> Date: 2021-12-08 14:58:32
From: Phillip Wood <redacted>
At the start of a rebase ORIG_HEAD is updated to tip of the branch
being rebased. Unfortunately reset_head() always uses the current
value of HEAD for this which is incorrect if the rebase is started
with "git rebase <upstream> <branch>" as in that case ORIG_HEAD should
be updated to <branch>. This only affects the "apply" backend as the
"merge" backend does not yet use reset_head() for the initial
checkout. Fix this by passing in orig_head when calling reset_head()
and add some regression tests.
Signed-off-by: Phillip Wood <redacted>
---
builtin/rebase.c | 1 +
reset.c | 4 +++-
reset.h | 4 ++++
t/t3418-rebase-continue.sh | 26 ++++++++++++++++++++++++++
4 files changed, 34 insertions(+), 1 deletion(-)
From: Phillip Wood via GitGitGadget <hidden> Date: 2021-12-08 14:59:24
From: Phillip Wood <redacted>
Now that reset_head() can handle the initial checkout of onto
correctly use it in the "merge" backend instead of forking "git
checkout". This opens the way for us to stop calling the
post-checkout hook in the future. Not running "git checkout" means
that "rebase -i/m" no longer recurse submodules when checking out
"onto" (thanks to Philippe Blain for pointing this out). As the rest
of rebase does not know what to do with submodules this is probably a
good thing. When using merge-ort rebase ought be able to handle
submodules correctly if it parsed the submodule config, such a change
is left for a future patch series.
The "apply" based rebase has avoided forking git checkout
since ac7f467fef ("builtin/rebase: support running "git rebase
<upstream>"", 2018-08-07). The code that handles the checkout was
moved into libgit by b309a97108 ("reset: extract reset_head() from
rebase", 2020-04-07).
Signed-off-by: Phillip Wood <redacted>
---
sequencer.c | 38 +++++++++++---------------------------
1 file changed, 11 insertions(+), 27 deletions(-)
From: Junio C Hamano <hidden> Date: 2021-12-09 21:04:24
"Phillip Wood via GitGitGadget" [off-list ref] writes:
Thanks for the comments on V1. I have tried to improve the commit messages
to explain better the motivation and implications of the changes in this
series and I have added some more tests. I have rebased onto v2.34.0 to
avoid some merges conflicts.
Thanks.
It still is not clear in the cover letter what the overall theme of
the topic is, and the original cover letter was deliberately vague
by saying "Fix *some* issues". Random assortment of changes to
various code paths, the only common trait among them being they are
somehow related to reset_head()?
Cover letter for V1: Fix some issues with the implementation and use of
reset_head(). The last patch was previously posted as [1], I have updated
the commit message and rebased it onto the fixes in this series. There are a
couple of small conflicts merging this into seen, I think they should be
easy to resolve (in rebase.c take both sides in reset.c take the changed
lines from each side). These patches are based on pw/rebase-of-a-tag-fix
I've read the patches through. It does revolve around the use of
reset_head(). I would have appreciated if the cover letter said
something along this line:
reset.c::reset_head() started its life at ac7f467f
(builtin/rebase: support running "git rebase <upstream>",
2018-08-07) as a way to detach the HEAD to replay the commits
during "git rebase", but over time it learned to do many things,
like switching the tip of the branch to another commit,
recording the old value of HEAD in ORIG_HEAD while it does so,
recording reflog entries for both HEAD and for the branch.
The API into the function got clunky and it is harder than
necessary for the callers to use the function correctly, which
led to a handful of bugs that this series is going to fix.
... list of bugs here ...
Later steps of this series revamps the API so that it is harder
to use it incorrectly to prevent future bugs.
Anyway, I think the series is more or less in a very good shape,
even though a few comments I threw at this round may result in a
further improvement.
Thanks for working on this.
From: Junio C Hamano <hidden> Date: 2021-12-09 21:04:37
"Phillip Wood via GitGitGadget" [off-list ref] writes:
From: Phillip Wood <redacted>
This code is heavily indented and it will be convenient later in the
series to have it in its own function.
Looks good in the sense that it is straight code movement without
changing any behaviour that won't hurt.
Without a clear overall direction given in the cover letter, however,
it is hard to judge if "being convenient later in the series" is a
good thing in the first place, though.
@@ -812,6 +812,23 @@ static int rebase_config(const char *var, const char *value, void *data)returngit_default_config(var,value,data);}+staticintcheckout_up_to_date(structrebase_options*options)+{+structstrbufbuf=STRBUF_INIT;+intret=0;++strbuf_addf(&buf,"%s: checkout %s",+getenv(GIT_REFLOG_ACTION_ENVIRONMENT),+options->switch_to);+if(reset_head(the_repository,&options->orig_head,"checkout",+options->head_name,RESET_HEAD_RUN_POST_CHECKOUT_HOOK,+NULL,buf.buf,DEFAULT_REFLOG_ACTION)<0)+ret=error(_("could not switch to %s"),options->switch_to);+strbuf_release(&buf);++returnret;+}+/**Determineswhetherthecommitsinfrom..toarelinear,i.e.contain*nomergecommits.Thisfunction*expects*`from`tobeanancestorof
@@ -1673,21 +1690,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)if(!(options.flags&REBASE_FORCE)){/* Lazily switch to the target branch if needed... */if(options.switch_to){-strbuf_reset(&buf);-strbuf_addf(&buf,"%s: checkout %s",-getenv(GIT_REFLOG_ACTION_ENVIRONMENT),-options.switch_to);-if(reset_head(the_repository,-&options.orig_head,"checkout",-options.head_name,-RESET_HEAD_RUN_POST_CHECKOUT_HOOK,-NULL,buf.buf,-DEFAULT_REFLOG_ACTION)<0){-ret=error(_("could not switch to "-"%s"),-options.switch_to);+ret=checkout_up_to_date(&options);+if(ret)gotocleanup;-}}if(!(options.flags&REBASE_NO_QUIET))
On Wed, Dec 8, 2021 at 6:58 AM Phillip Wood via GitGitGadget
[off-list ref] wrote:
From: Phillip Wood <redacted>
At the start of a rebase ORIG_HEAD is updated to tip of the branch
Could we insert a comma between "rebase" and "ORIG_HEAD"? Otherwise I
try to read it as "At the start of a `rebase ORIG_HEAD` is updated..."
and get totally lost. I had to re-read the sentence a few times
before I understood what it was trying to say.
Also, perhaps s/to tip of/to the tip of/ ?
quoted hunk
being rebased. Unfortunately reset_head() always uses the current
value of HEAD for this which is incorrect if the rebase is started
with "git rebase <upstream> <branch>" as in that case ORIG_HEAD should
be updated to <branch>. This only affects the "apply" backend as the
"merge" backend does not yet use reset_head() for the initial
checkout. Fix this by passing in orig_head when calling reset_head()
and add some regression tests.
Signed-off-by: Phillip Wood <redacted>
---
builtin/rebase.c | 1 +
reset.c | 4 +++-
reset.h | 4 ++++
t/t3418-rebase-continue.sh | 26 ++++++++++++++++++++++++++
4 files changed, 34 insertions(+), 1 deletion(-)
On Wed, Dec 8, 2021 at 6:58 AM Phillip Wood via GitGitGadget
[off-list ref] wrote:
Thanks for the comments on V1. I have tried to improve the commit messages
to explain better the motivation and implications of the changes in this
series and I have added some more tests. I have rebased onto v2.34.0 to
avoid some merges conflicts.
Changes since V1:
* Patch 1 - unchanged.
* Patches 2, 3 - these are new and fix an bug I noticed while adding a test
to patch 4.
* Patches 4, 5 - improved commit messages and added tests.
* Patch 6 - reworded commit message.
* Patch 7 - split out some changes that used to be in patch 9.
* Patch 8 - in principle the same but the range-diff is noisy due to the
addition of patch 3.
* Patch 9 - reworded commit message.
* Patch 10 - unchanged.
* Patch 11 - reworded commit message and a couple of comments.
* Patch 12 - minor changes to comments.
* Patch 13 - cosmetic changes to commit message and tests.
* Patch 14 - cosmetic changes to commit message.
I don't know why, but I seem to have gotten interrupted a lot more
reviewing this series than others; I've come back to it multiple
times. Most of the stuff I found had already been noted by Junio; the
only new thing was some tiny grammatical comments I left on the commit
message in Patch 13.
Overall, the series looks very nice; thanks for working on this.
Hi Junio
On 09/12/2021 21:04, Junio C Hamano wrote:
"Phillip Wood via GitGitGadget" [off-list ref] writes:
quoted
Thanks for the comments on V1. I have tried to improve the commit messages
to explain better the motivation and implications of the changes in this
series and I have added some more tests. I have rebased onto v2.34.0 to
avoid some merges conflicts.
Thanks.
It still is not clear in the cover letter what the overall theme of
the topic is, and the original cover letter was deliberately vague
by saying "Fix *some* issues". Random assortment of changes to
various code paths, the only common trait among them being they are
somehow related to reset_head()?
So the theme started out as "convert 'rebase -i' to use reset_head() and
stop forking 'git checkout'" which I thought would be one to two
patches. Then I started looking at the code and realized that
reset_head() needed some work before we could use it for 'rebase -i' and
that work ended up dominating the series. I've updated the cover letter
as you suggested.
Best Wishes
Phillip
quoted
Cover letter for V1: Fix some issues with the implementation and use of
reset_head(). The last patch was previously posted as [1], I have updated
the commit message and rebased it onto the fixes in this series. There are a
couple of small conflicts merging this into seen, I think they should be
easy to resolve (in rebase.c take both sides in reset.c take the changed
lines from each side). These patches are based on pw/rebase-of-a-tag-fix
I've read the patches through. It does revolve around the use of
reset_head(). I would have appreciated if the cover letter said
something along this line:
reset.c::reset_head() started its life at ac7f467f
(builtin/rebase: support running "git rebase <upstream>",
2018-08-07) as a way to detach the HEAD to replay the commits
during "git rebase", but over time it learned to do many things,
like switching the tip of the branch to another commit,
recording the old value of HEAD in ORIG_HEAD while it does so,
recording reflog entries for both HEAD and for the branch.
The API into the function got clunky and it is harder than
necessary for the callers to use the function correctly, which
led to a handful of bugs that this series is going to fix.
... list of bugs here ...
Later steps of this series revamps the API so that it is harder
to use it incorrectly to prevent future bugs.
Anyway, I think the series is more or less in a very good shape,
even though a few comments I threw at this round may result in a
further improvement.
Thanks for working on this.
From: Phillip Wood via GitGitGadget <hidden> Date: 2022-01-26 13:05:55
Thanks to Junio and Elijah for their comments on V2. There aren't too many
changes this time.
This series started out with the aim of converting 'rebase -i' to use
reset_head() instead of forking 'git checkout'. As 'rebase --apply' already
uses reset_head() I assumed this would be straight forward. However it has
morphed into a series of fixes for reset_head() followed by the intended
conversion of 'rebase -i'.
reset.c::reset_head() started its life at ac7f467f (builtin/rebase: support
running "git rebase ", 2018-08-07) as a way to detach the HEAD to replay the
commits during "git rebase", but over time it learned to do many things,
like switching the tip of the branch to another commit, recording the old
value of HEAD in ORIG_HEAD while it does so, recording reflog entries for
both HEAD and for the branch.
The API into the function got clunky and it is harder than necessary for the
callers to use the function correctly, which led to a handful of bugs that
are fixed by this series. The bugs include
* passing the wrong oid to the post-checkout hook
* removing untracked files on checkout
* running the post-checkout hook if the checkout fails
* passing parameters to reset_head() that it does not use
* incorrect reflog messages for 'rebase --apply'
* sometimes setting ORIG_HEAD incorrectly at the start 'rebase --apply'
Later steps of this series revamps the API so that it is harder to use it
incorrectly to prevent future bugs and finally convert 'rebase -i' to use
reset_head()
Changes since V2:
* Updated cover letter as suggested by Junio
* Patch 4 - fixed typos in the commit message spotted by Junio
* Patch 9 - Moved later in the series to simplify the autostash canges as
suggested by Junio. This used to be patch 7
* Patch 10 - Added a comment to the commit message explaining why we cannot
BUG() on an invalid parameter until a change is made in a later commit
* Patch 13 - Reworded the first sentence commit message as suggest by
Elijah.
Cover letter for V2:
Thanks for the comments on V1. I have tried to improve the commit messages
to explain better the motivation and implications of the changes in this
series and I have added some more tests. I have rebased onto v2.34.0 to
avoid some merges conflicts.
Changes since V1:
* Patch 1 - unchanged.
* Patches 2, 3 - these are new and fix an bug I noticed while adding a test
to patch 4.
* Patches 4, 5 - improved commit messages and added tests.
* Patch 6 - reworded commit message.
* Patch 7 - split out some changes that used to be in patch 9.
* Patch 8 - in principle the same but the range-diff is noisy due to the
addition of patch 3.
* Patch 9 - reworded commit message.
* Patch 10 - unchanged.
* Patch 11 - reworded commit message and a couple of comments.
* Patch 12 - minor changes to comments.
* Patch 13 - cosmetic changes to commit message and tests.
* Patch 14 - cosmetic changes to commit message.
Cover letter for V1: Fix some issues with the implementation and use of
reset_head(). The last patch was previously posted as [1], I have updated
the commit message and rebased it onto the fixes in this series. There are a
couple of small conflicts merging this into seen, I think they should be
easy to resolve (in rebase.c take both sides in reset.c take the changed
lines from each side). These patches are based on pw/rebase-of-a-tag-fix
[1]
https://lore.kernel.org/git/39ad40c9297531a2d42b7263a1d41b1ecbc23c0a.1631108472.git.gitgitgadget@gmail.com/
Phillip Wood (14):
rebase: factor out checkout for up to date branch
t5403: refactor rebase post-checkout hook tests
rebase: pass correct arguments to post-checkout hook
rebase: do not remove untracked files on checkout
rebase --apply: don't run post-checkout hook if there is an error
reset_head(): remove action parameter
reset_head(): factor out ref updates
reset_head(): make default_reflog_action optional
create_autostash(): remove unneeded parameter
rebase: cleanup reset_head() calls
reset_head(): take struct rebase_head_opts
rebase --apply: fix reflog
rebase --apply: set ORIG_HEAD correctly
rebase -m: don't fork git checkout
builtin/merge.c | 6 +-
builtin/rebase.c | 101 +++++++++++++----------
reset.c | 149 ++++++++++++++++++++--------------
reset.h | 48 ++++++++++-
sequencer.c | 47 ++++-------
sequencer.h | 3 +-
t/t3406-rebase-message.sh | 23 ++++++
t/t3418-rebase-continue.sh | 26 ++++++
t/t5403-post-checkout-hook.sh | 67 +++++++++++----
9 files changed, 312 insertions(+), 158 deletions(-)
base-commit: cd3e606211bb1cf8bc57f7d76bab98cc17a150bc
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1049%2Fphillipwood%2Fwip%2Frebase-reset-head-fixes-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1049/phillipwood/wip/rebase-reset-head-fixes-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1049
Range-diff vs v2:
1: 0e84d00572e = 1: 0e84d00572e rebase: factor out checkout for up to date branch
2: a67a5a03b94 = 2: a67a5a03b94 t5403: refactor rebase post-checkout hook tests
3: 07867760e68 = 3: 07867760e68 rebase: pass correct arguments to post-checkout hook
4: 2b499704c8f ! 4: f4b925508e7 rebase: do not remove untracked files on checkout
@@ Commit message
<upstream> is an ancestor of <branch> then it will fast-forward and
checkout <branch>. Normally a checkout or picking a commit during a
rebase will refuse to overwrite untracked files, however rebase does
- overwrite untracked files when checking <branch>.
+ overwrite untracked files when checking out <branch>.
The fix is to only set reset in `unpack_tree_opts` if flags contains
`RESET_HEAD_HARD`. t5403 may seem like an odd home for the new test
but it will be extended in the next commit to check that the
post-checkout hook is not run when the checkout fails.
- The test for `!deatch_head` dates back to the
+ The test for `!detach_head` dates back to the
original implementation of reset_head() in
ac7f467fef ("builtin/rebase: support running "git rebase <upstream>"",
2018-08-07) and was correct until e65123a71d
5: 04e7340a7e7 = 5: 4de5104d22d rebase --apply: don't run post-checkout hook if there is an error
6: 32ffa98c1bc = 6: ff23498e93e reset_head(): remove action parameter
8: 29e06e7d36d = 7: 688ebc45bf7 reset_head(): factor out ref updates
9: 9d00a218daf ! 8: a5cc7eaa925 reset_head(): make default_reflog_action optional
@@ reset.c: int reset_head(struct repository *r, struct object_id *oid,
leave_reset_head:
rollback_lock_file(&lock);
-
- ## sequencer.c ##
-@@ sequencer.c: void create_autostash(struct repository *r, const char *path)
- write_file(path, "%s", oid_to_hex(&oid));
- printf(_("Created autostash: %s\n"), buf.buf);
- if (reset_head(r, NULL, NULL, RESET_HEAD_HARD, NULL, NULL,
-- "") < 0)
-+ NULL) < 0)
- die(_("could not reset --hard"));
-
- if (discard_index(r->index) < 0 ||
7: 341fe183c18 ! 9: dd3a22384d2 create_autostash(): remove unneeded parameter
@@ sequencer.c: void create_autostash(struct repository *r, const char *path,
printf(_("Created autostash: %s\n"), buf.buf);
if (reset_head(r, NULL, NULL, RESET_HEAD_HARD, NULL, NULL,
- default_reflog_action) < 0)
-+ "") < 0)
++ NULL) < 0)
die(_("could not reset --hard"));
if (discard_index(r->index) < 0 ||
10: 5ea636009e7 ! 10: ad7c6467987 rebase: cleanup reset_head() calls
@@ Commit message
currently possible so we delay fixing that caller until we can pass
the message as the branch reflog.
+ A later commit will make it a BUG() to pass reflog_orig_head without
+ RESET_ORIG_HEAD, that changes cannot be done here as it needs to wait
+ for move_to_original_branch() to be fixed first.
+
Signed-off-by: Phillip Wood [off-list ref]
## builtin/rebase.c ##
11: 24b0566aba5 = 11: d170703e833 reset_head(): take struct rebase_head_opts
12: dc5d11291e7 = 12: 4973892561e rebase --apply: fix reflog
13: 45a5b5e9818 ! 13: 0ef0e978112 rebase --apply: set ORIG_HEAD correctly
@@ Metadata
## Commit message ##
rebase --apply: set ORIG_HEAD correctly
- At the start of a rebase ORIG_HEAD is updated to tip of the branch
- being rebased. Unfortunately reset_head() always uses the current
- value of HEAD for this which is incorrect if the rebase is started
- with "git rebase <upstream> <branch>" as in that case ORIG_HEAD should
- be updated to <branch>. This only affects the "apply" backend as the
- "merge" backend does not yet use reset_head() for the initial
- checkout. Fix this by passing in orig_head when calling reset_head()
- and add some regression tests.
+ At the start of a rebase, ORIG_HEAD is updated to the tip of the
+ branch being rebased. Unfortunately reset_head() always uses the
+ current value of HEAD for this which is incorrect if the rebase is
+ started with "git rebase <upstream> <branch>" as in that case
+ ORIG_HEAD should be updated to <branch>. This only affects the "apply"
+ backend as the "merge" backend does not yet use reset_head() for the
+ initial checkout. Fix this by passing in orig_head when calling
+ reset_head() and add some regression tests.
Signed-off-by: Phillip Wood [off-list ref]
14: 3f64b9274b5 = 14: 9b9560ef676 rebase -m: don't fork git checkout
--
gitgitgadget
From: Phillip Wood via GitGitGadget <hidden> Date: 2022-01-26 13:05:56
From: Phillip Wood <redacted>
This code is heavily indented and it will be convenient later in the
series to have it in its own function.
Signed-off-by: Phillip Wood <redacted>
---
builtin/rebase.c | 33 +++++++++++++++++++--------------
1 file changed, 19 insertions(+), 14 deletions(-)
@@ -812,6 +812,23 @@ static int rebase_config(const char *var, const char *value, void *data)returngit_default_config(var,value,data);}+staticintcheckout_up_to_date(structrebase_options*options)+{+structstrbufbuf=STRBUF_INIT;+intret=0;++strbuf_addf(&buf,"%s: checkout %s",+getenv(GIT_REFLOG_ACTION_ENVIRONMENT),+options->switch_to);+if(reset_head(the_repository,&options->orig_head,"checkout",+options->head_name,RESET_HEAD_RUN_POST_CHECKOUT_HOOK,+NULL,buf.buf,DEFAULT_REFLOG_ACTION)<0)+ret=error(_("could not switch to %s"),options->switch_to);+strbuf_release(&buf);++returnret;+}+/**Determineswhetherthecommitsinfrom..toarelinear,i.e.contain*nomergecommits.Thisfunction*expects*`from`tobeanancestorof
@@ -1673,21 +1690,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)if(!(options.flags&REBASE_FORCE)){/* Lazily switch to the target branch if needed... */if(options.switch_to){-strbuf_reset(&buf);-strbuf_addf(&buf,"%s: checkout %s",-getenv(GIT_REFLOG_ACTION_ENVIRONMENT),-options.switch_to);-if(reset_head(the_repository,-&options.orig_head,"checkout",-options.head_name,-RESET_HEAD_RUN_POST_CHECKOUT_HOOK,-NULL,buf.buf,-DEFAULT_REFLOG_ACTION)<0){-ret=error(_("could not switch to "-"%s"),-options.switch_to);+ret=checkout_up_to_date(&options);+if(ret)gotocleanup;-}}if(!(options.flags&REBASE_NO_QUIET))
From: Phillip Wood via GitGitGadget <hidden> Date: 2022-01-26 13:05:57
From: Phillip Wood <redacted>
These tests only test the default backend and do not check that the
arguments passed to the hook are correct. Fix this by running the
tests with both backends and adding checks for the hook arguments.
Signed-off-by: Phillip Wood <redacted>
---
t/t5403-post-checkout-hook.sh | 42 ++++++++++++++++++++++-------------
1 file changed, 26 insertions(+), 16 deletions(-)
@@ -49,23 +49,33 @@ test_expect_success 'post-checkout receives the right args when not switching brtest$old=$new&&test$flag=0'-test_expect_success'post-checkout is triggered on rebase''-test_when_finished"rm -f .git/post-checkout.args"&&-gitcheckout-brebase-testmain&&-rm-f.git/post-checkout.args&&-gitrebaserebase-on-me&&-readoldnewflag<.git/post-checkout.args&&-test$old!=$new&&test$flag=1-'+test_rebase(){+args="$*"&&+test_expect_success"post-checkout is triggered on rebase $args"'+test_when_finished"rm -f .git/post-checkout.args"&&+gitcheckout-Brebase-testmain&&+rm-f.git/post-checkout.args&&+gitrebase$argsrebase-on-me&&+readoldnewflag<.git/post-checkout.args&&+test_cmp_revmain$old&&+test_cmp_revrebase-on-me$new&&+test$flag=1+'-test_expect_success'post-checkout is triggered on rebase with fast-forward''-test_when_finished"rm -f .git/post-checkout.args"&&-gitcheckout-bff-rebase-testrebase-on-me^&&-rm-f.git/post-checkout.args&&-gitrebaserebase-on-me&&-readoldnewflag<.git/post-checkout.args&&-test$old!=$new&&test$flag=1-'+test_expect_success"post-checkout is triggered on rebase $args with fast-forward"'+test_when_finished"rm -f .git/post-checkout.args"&&+gitcheckout-Bff-rebase-testrebase-on-me^&&+rm-f.git/post-checkout.args&&+gitrebase$argsrebase-on-me&&+readoldnewflag<.git/post-checkout.args&&+test_cmp_revrebase-on-me^$old&&+test_cmp_revrebase-on-me$new&&+test$flag=1+'+}++test_rebase--apply&&+test_rebase--merge test_expect_success'post-checkout hook is triggered by clone''mkdir-ptemplates/hooks&&
From: Phillip Wood via GitGitGadget <hidden> Date: 2022-01-26 13:05:57
From: Phillip Wood <redacted>
If a rebase started with "rebase [--apply|--merge] <upstream> <branch>"
detects that <upstream> is an ancestor of <branch> then it fast-forwards
and checks out <branch>. Unfortunately in that case it passed the null
oid as the first argument to the post-checkout hook rather than the oid
of HEAD.
A side effect of this change is that the call to update_ref() which
updates HEAD now always receives the old value of HEAD. This provides
protection against another process updating HEAD during the checkout.
Signed-off-by: Phillip Wood <redacted>
---
reset.c | 18 +++++++++---------
t/t5403-post-checkout-hook.sh | 13 +++++++++++++
2 files changed, 22 insertions(+), 9 deletions(-)
From: Phillip Wood via GitGitGadget <hidden> Date: 2022-01-26 13:05:58
From: Phillip Wood <redacted>
If "git rebase [--apply|--merge] <upstream> <branch>" detects that
<upstream> is an ancestor of <branch> then it will fast-forward and
checkout <branch>. Normally a checkout or picking a commit during a
rebase will refuse to overwrite untracked files, however rebase does
overwrite untracked files when checking out <branch>.
The fix is to only set reset in `unpack_tree_opts` if flags contains
`RESET_HEAD_HARD`. t5403 may seem like an odd home for the new test
but it will be extended in the next commit to check that the
post-checkout hook is not run when the checkout fails.
The test for `!detach_head` dates back to the
original implementation of reset_head() in
ac7f467fef ("builtin/rebase: support running "git rebase <upstream>"",
2018-08-07) and was correct until e65123a71d
("builtin rebase: support `git rebase <upstream> <switch-to>`",
2018-09-04) started using reset_head() to checkout <switch-to> when
fast-forwarding.
Note that 480d3d6bf9 ("Change unpack_trees' 'reset' flag into an
enum", 2021-09-27) also fixes this bug as it changes reset_head() to
never remove untracked files. I think this fix is still worthwhile as
it makes it clear that the same settings are used for detached and
non-detached checkouts.
Signed-off-by: Phillip Wood <redacted>
---
reset.c | 2 +-
t/t5403-post-checkout-hook.sh | 10 ++++++++++
2 files changed, 11 insertions(+), 1 deletion(-)
@@ -85,6 +85,16 @@ test_rebase () {test_cmp_revthree$new&&test$flag=1'++test_expect_success"rebase $args checkout does not remove untracked files"'+test_when_finished"test_might_fail git rebase --abort"&&+gitupdate-refrefs/heads/rebase-fast-forwardthree&&+gitcheckouttwo&&+echountracked>three.t&&+test_when_finished"rm three.t"&&+test_must_failgitrebase$argsHEADrebase-fast-forward2>err&&+grep"untracked working tree files would be overwritten by checkout"err+'} test_rebase--apply&&
From: Phillip Wood via GitGitGadget <hidden> Date: 2022-01-26 13:06:03
From: Phillip Wood <redacted>
The hook should only be run if the worktree and refs were successfully
updated. This primarily affects "rebase --apply" but also "rebase
--merge" when it fast-forwards.
Signed-off-by: Phillip Wood <redacted>
---
reset.c | 2 +-
t/t5403-post-checkout-hook.sh | 6 +++++-
2 files changed, 6 insertions(+), 2 deletions(-)
@@ -88,12 +88,16 @@ test_rebase () {test_expect_success"rebase $args checkout does not remove untracked files"'test_when_finished"test_might_fail git rebase --abort"&&+test_when_finished"rm -f .git/post-checkout.args"&&gitupdate-refrefs/heads/rebase-fast-forwardthree&&gitcheckouttwo&&+rm-f.git/post-checkout.args&&echountracked>three.t&&test_when_finished"rm three.t"&&test_must_failgitrebase$argsHEADrebase-fast-forward2>err&&-grep"untracked working tree files would be overwritten by checkout"err+grep"untracked working tree files would be overwritten by checkout"err&&+test_path_is_missing.git/post-checkout.args+'}
From: Phillip Wood via GitGitGadget <hidden> Date: 2022-01-26 13:06:08
From: Phillip Wood <redacted>
The only use of the action parameter is to setup the error messages
for unpack_trees(). All but two cases pass either "checkout" or
"reset". The case that passes "reset --hard" would be better passing
"reset" so that the error messages match the builtin reset command
like all the other callers that are doing a reset. The case that
passes "Fast-forwarded" is only updating HEAD and so the parameter is
unused in that case as it does not call unpack_trees(). The value to
pass to setup_unpack_trees_porcelain() can be determined by checking
whether flags contains RESET_HEAD_HARD without the caller having to
specify it.
Signed-off-by: Phillip Wood <redacted>
---
builtin/rebase.c | 14 +++++++-------
reset.c | 5 +++--
reset.h | 2 +-
sequencer.c | 3 +--
4 files changed, 12 insertions(+), 12 deletions(-)
@@ -583,7 +583,7 @@ static int move_to_original_branch(struct rebase_options *opts)opts->head_name,oid_to_hex(&opts->onto->object.oid));strbuf_addf(&head_reflog,"rebase finished: returning to %s",opts->head_name);-ret=reset_head(the_repository,NULL,"",opts->head_name,+ret=reset_head(the_repository,NULL,opts->head_name,RESET_HEAD_REFS_ONLY,orig_head_reflog.buf,head_reflog.buf,DEFAULT_REFLOG_ACTION);
@@ -674,7 +674,7 @@ static int run_am(struct rebase_options *opts)free(rebased_patches);strvec_clear(&am.args);-reset_head(the_repository,&opts->orig_head,"checkout",+reset_head(the_repository,&opts->orig_head,opts->head_name,0,"HEAD",NULL,DEFAULT_REFLOG_ACTION);error(_("\ngit encountered an error while preparing the "
@@ -820,7 +820,7 @@ static int checkout_up_to_date(struct rebase_options *options)strbuf_addf(&buf,"%s: checkout %s",getenv(GIT_REFLOG_ACTION_ENVIRONMENT),options->switch_to);-if(reset_head(the_repository,&options->orig_head,"checkout",+if(reset_head(the_repository,&options->orig_head,options->head_name,RESET_HEAD_RUN_POST_CHECKOUT_HOOK,NULL,buf.buf,DEFAULT_REFLOG_ACTION)<0)ret=error(_("could not switch to %s"),options->switch_to);
@@ -1272,7 +1272,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)rerere_clear(the_repository,&merge_rr);string_list_clear(&merge_rr,1);-if(reset_head(the_repository,NULL,"reset",NULL,RESET_HEAD_HARD,+if(reset_head(the_repository,NULL,NULL,RESET_HEAD_HARD,NULL,NULL,DEFAULT_REFLOG_ACTION)<0)die(_("could not discard worktree changes"));remove_branch_state(the_repository,0);
@@ -1290,7 +1290,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)if(read_basic_state(&options))exit(1);-if(reset_head(the_repository,&options.orig_head,"reset",+if(reset_head(the_repository,&options.orig_head,options.head_name,RESET_HEAD_HARD,NULL,NULL,DEFAULT_REFLOG_ACTION)<0)die(_("could not move back to %s"),
From: Phillip Wood via GitGitGadget <hidden> Date: 2022-01-26 13:06:09
From: Phillip Wood <redacted>
In the next commit we will stop trying to update HEAD when we are
removing uncommitted changes from the working tree. Move the code that
updates the refs to its own function in preparation for that.
Signed-off-by: Phillip Wood <redacted>
---
reset.c | 110 +++++++++++++++++++++++++++++++-------------------------
1 file changed, 62 insertions(+), 48 deletions(-)
From: Phillip Wood via GitGitGadget <hidden> Date: 2022-01-26 13:06:10
From: Phillip Wood <redacted>
This parameter is only needed when a ref is going to be updated and
the caller does not pass an explicit reflog message. Callers that are
only discarding uncommitted changes in the working tree such as such
as "rebase --skip" or create_autostash() do not update any refs so
should not have to worry about passing this parameter.
This change is not intended to have any user visible changes. The
pointer comparison between `oid` and `&head_oid` checks that the
caller did not pass an oid to be checked out. As no callers pass
RESET_HEAD_RUN_POST_CHECKOUT_HOOK without passing an oid there are
no changes to when the post-checkout hook is run. As update_ref() only
updates the ref if the oid passed to it differs from the current ref
there are no changes to when HEAD is updated.
Signed-off-by: Phillip Wood <redacted>
---
builtin/rebase.c | 10 ++++------
reset.c | 16 ++++++++++++----
2 files changed, 16 insertions(+), 10 deletions(-)
@@ -585,8 +585,7 @@ static int move_to_original_branch(struct rebase_options *opts)opts->head_name);ret=reset_head(the_repository,NULL,opts->head_name,RESET_HEAD_REFS_ONLY,-orig_head_reflog.buf,head_reflog.buf,-DEFAULT_REFLOG_ACTION);+orig_head_reflog.buf,head_reflog.buf,NULL);strbuf_release(&orig_head_reflog);strbuf_release(&head_reflog);
@@ -822,7 +821,7 @@ static int checkout_up_to_date(struct rebase_options *options)options->switch_to);if(reset_head(the_repository,&options->orig_head,options->head_name,RESET_HEAD_RUN_POST_CHECKOUT_HOOK,-NULL,buf.buf,DEFAULT_REFLOG_ACTION)<0)+NULL,buf.buf,NULL)<0)ret=error(_("could not switch to %s"),options->switch_to);strbuf_release(&buf);
@@ -1273,7 +1272,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)string_list_clear(&merge_rr,1);if(reset_head(the_repository,NULL,NULL,RESET_HEAD_HARD,-NULL,NULL,DEFAULT_REFLOG_ACTION)<0)+NULL,NULL,NULL)<0)die(_("could not discard worktree changes"));remove_branch_state(the_repository,0);if(read_basic_state(&options))
@@ -22,8 +22,13 @@ static int update_refs(const struct object_id *oid, const char *switch_to_branchsize_tprefix_len;intret;-reflog_action=getenv(GIT_REFLOG_ACTION_ENVIRONMENT);-strbuf_addf(&msg,"%s: ",reflog_action?reflog_action:default_reflog_action);+if((update_orig_head&&!reflog_orig_head)||!reflog_head){+if(!default_reflog_action)+BUG("default_reflog_action must be given when reflog messages are omitted");+reflog_action=getenv(GIT_REFLOG_ACTION_ENVIRONMENT);+strbuf_addf(&msg,"%s: ",reflog_action?reflog_action:+default_reflog_action);+}prefix_len=msg.len;if(update_orig_head){
From: Phillip Wood via GitGitGadget <hidden> Date: 2022-01-26 13:06:13
From: Phillip Wood <redacted>
The default_reflog parameter of create_autostash() is passed to
reset_head(). However as creating a stash does not involve updating
any refs the parameter is not used by reset_head(). Removing the
parameter from create_autostash() simplifies the callers.
Signed-off-by: Phillip Wood <redacted>
---
builtin/merge.c | 6 ++----
builtin/rebase.c | 8 ++++----
sequencer.c | 5 ++---
sequencer.h | 3 +--
4 files changed, 9 insertions(+), 13 deletions(-)
@@ -1637,8 +1636,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)if(autostash)create_autostash(the_repository,-git_path_merge_autostash(the_repository),-"merge");+git_path_merge_autostash(the_repository));/* We are going to make a new commit. */git_committer_info(IDENT_STRICT);
From: Phillip Wood via GitGitGadget <hidden> Date: 2022-01-26 13:06:15
From: Phillip Wood <redacted>
If ORIG_HEAD is not set by passing RESET_ORIG_HEAD then there is no
need to pass anything for reflog_orig_head. In addition to the callers
fixed in this commit move_to_original_branch() also passes
reflog_orig_head without setting ORIG_HEAD. That caller is mistakenly
passing the message it wants to put in the branch reflog which is not
currently possible so we delay fixing that caller until we can pass
the message as the branch reflog.
A later commit will make it a BUG() to pass reflog_orig_head without
RESET_ORIG_HEAD, that changes cannot be done here as it needs to wait
for move_to_original_branch() to be fixed first.
Signed-off-by: Phillip Wood <redacted>
---
builtin/rebase.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
@@ -675,7 +675,7 @@ static int run_am(struct rebase_options *opts)reset_head(the_repository,&opts->orig_head,opts->head_name,0,-"HEAD",NULL,DEFAULT_REFLOG_ACTION);+NULL,NULL,DEFAULT_REFLOG_ACTION);error(_("\ngit encountered an error while preparing the ""patches to replay\n""these revisions:\n"
From: Phillip Wood via GitGitGadget <hidden> Date: 2022-01-26 13:06:17
From: Phillip Wood <redacted>
This function takes a confusingly large number of parameters which
makes it difficult to remember which order to pass them in. The
following commits will add a couple more parameters which makes the
problem worse. To address this change the function to take a struct of
options. Using a struct means that it is no longer necessary to
remember which order to pass the parameters in and anyone reading the
code can easily see which value is passed to each parameter.
Signed-off-by: Phillip Wood <redacted>
---
builtin/rebase.c | 57 ++++++++++++++++++++++++++++++------------------
reset.c | 38 +++++++++++++++-----------------
reset.h | 40 +++++++++++++++++++++++++++++----
sequencer.c | 5 ++---
4 files changed, 92 insertions(+), 48 deletions(-)
@@ -571,6 +571,7 @@ static int finish_rebase(struct rebase_options *opts)staticintmove_to_original_branch(structrebase_options*opts){structstrbuforig_head_reflog=STRBUF_INIT,head_reflog=STRBUF_INIT;+structreset_head_optsropts={0};intret;if(!opts->head_name)
@@ -583,9 +584,11 @@ static int move_to_original_branch(struct rebase_options *opts)opts->head_name,oid_to_hex(&opts->onto->object.oid));strbuf_addf(&head_reflog,"rebase finished: returning to %s",opts->head_name);-ret=reset_head(the_repository,NULL,opts->head_name,-RESET_HEAD_REFS_ONLY,-orig_head_reflog.buf,head_reflog.buf,NULL);+ropts.branch=opts->head_name;+ropts.flags=RESET_HEAD_REFS_ONLY;+ropts.orig_head_msg=orig_head_reflog.buf;+ropts.head_msg=head_reflog.buf;+ret=reset_head(the_repository,&ropts);strbuf_release(&orig_head_reflog);strbuf_release(&head_reflog);
@@ -669,13 +672,15 @@ static int run_am(struct rebase_options *opts)status=run_command(&format_patch);if(status){+structreset_head_optsropts={0};unlink(rebased_patches);free(rebased_patches);strvec_clear(&am.args);-reset_head(the_repository,&opts->orig_head,-opts->head_name,0,-NULL,NULL,DEFAULT_REFLOG_ACTION);+ropts.oid=&opts->orig_head;+ropts.branch=opts->head_name;+ropts.default_reflog_action=DEFAULT_REFLOG_ACTION;+reset_head(the_repository,&ropts);error(_("\ngit encountered an error while preparing the ""patches to replay\n""these revisions:\n"
@@ -814,14 +819,17 @@ static int rebase_config(const char *var, const char *value, void *data)staticintcheckout_up_to_date(structrebase_options*options){structstrbufbuf=STRBUF_INIT;+structreset_head_optsropts={0};intret=0;strbuf_addf(&buf,"%s: checkout %s",getenv(GIT_REFLOG_ACTION_ENVIRONMENT),options->switch_to);-if(reset_head(the_repository,&options->orig_head,-options->head_name,RESET_HEAD_RUN_POST_CHECKOUT_HOOK,-NULL,buf.buf,NULL)<0)+ropts.oid=&options->orig_head;+ropts.branch=options->head_name;+ropts.flags=RESET_HEAD_RUN_POST_CHECKOUT_HOOK;+ropts.head_msg=buf.buf;+if(reset_head(the_repository,&ropts)<0)ret=error(_("could not switch to %s"),options->switch_to);strbuf_release(&buf);
@@ -1270,9 +1279,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)rerere_clear(the_repository,&merge_rr);string_list_clear(&merge_rr,1);--if(reset_head(the_repository,NULL,NULL,RESET_HEAD_HARD,-NULL,NULL,NULL)<0)+ropts.flags=RESET_HEAD_HARD;+if(reset_head(the_repository,&ropts)<0)die(_("could not discard worktree changes"));remove_branch_state(the_repository,0);if(read_basic_state(&options))
@@ -1289,9 +1297,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)if(read_basic_state(&options))exit(1);-if(reset_head(the_repository,&options.orig_head,-options.head_name,RESET_HEAD_HARD,-NULL,NULL,DEFAULT_REFLOG_ACTION)<0)+ropts.oid=&options.orig_head;+ropts.branch=options.head_name;+ropts.flags=RESET_HEAD_HARD;+ropts.default_reflog_action=DEFAULT_REFLOG_ACTION;+if(reset_head(the_repository,&ropts)<0)die(_("could not move back to %s"),oid_to_hex(&options.orig_head));remove_branch_state(the_repository,0);
@@ -1758,10 +1768,12 @@ 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(the_repository,&options.onto->object.oid,NULL,-RESET_HEAD_DETACH|RESET_ORIG_HEAD|-RESET_HEAD_RUN_POST_CHECKOUT_HOOK,-NULL,msg.buf,DEFAULT_REFLOG_ACTION))+ropts.oid=&options.onto->object.oid;+ropts.flags=RESET_HEAD_DETACH|RESET_ORIG_HEAD|+RESET_HEAD_RUN_POST_CHECKOUT_HOOK;+ropts.head_msg=msg.buf;+ropts.default_reflog_action=DEFAULT_REFLOG_ACTION;+if(reset_head(the_repository,&ropts))die(_("Could not detach HEAD"));strbuf_release(&msg);
@@ -6,15 +6,47 @@#define GIT_REFLOG_ACTION_ENVIRONMENT "GIT_REFLOG_ACTION"+/* Request a detached checkout */#define RESET_HEAD_DETACH (1<<0)+/* Request a reset rather than a checkout */#define RESET_HEAD_HARD (1<<1)+/* Run the post-checkout hook */#define RESET_HEAD_RUN_POST_CHECKOUT_HOOK (1<<2)+/* Only update refs, do not touch the worktree */#define RESET_HEAD_REFS_ONLY (1<<3)+/* Update ORIG_HEAD as well as HEAD */#define RESET_ORIG_HEAD (1<<4)-intreset_head(structrepository*r,structobject_id*oid,-constchar*switch_to_branch,unsignedflags,-constchar*reflog_orig_head,constchar*reflog_head,-constchar*default_reflog_action);+structreset_head_opts{+/*+*Thecommittocheckout/resetto.DefaultstoHEAD.+*/+conststructobject_id*oid;+/*+*Optionalbranchtoswitchto.+*/+constchar*branch;+/*+*Flagsdefinedabove.+*/+unsignedflags;+/*+*OptionalreflogmessageforHEAD,ifthisomittedbutoidorbranch+*aregiventhendefault_reflog_actionmustbegiven.+*/+constchar*head_msg;+/*+*OptionalreflogmessageforORIG_HEAD,ifthisomittedandflags+*containsRESET_ORIG_HEADthendefault_reflog_actionmustbegiven.+*/+constchar*orig_head_msg;+/*+*Actiontouseindefaultreflogmessages,onlyrequiredifarefis+*beingupdatedandthereflogmessagesaboveareomitted.+*/+constchar*default_reflog_action;+};++intreset_head(structrepository*r,conststructreset_head_opts*opts);#endif
From: Phillip Wood via GitGitGadget <hidden> Date: 2022-01-26 13:06:20
From: Phillip Wood <redacted>
move_to_original_branch() passes the message intended for the branch
reflog as `orig_head_msg`. Fix this by adding a `branch_msg` member to
struct reset_head_opts and add a regression test. Note that these
reflog messages do not respect GIT_REFLOG_ACTION. They are not alone
in that and will be fixed in a future series.
The "merge" backend already has tests that check both the branch and
HEAD reflogs.
Signed-off-by: Phillip Wood <redacted>
---
builtin/rebase.c | 8 ++++----
reset.c | 12 ++++++++++--
reset.h | 4 ++++
t/t3406-rebase-message.sh | 23 +++++++++++++++++++++++
4 files changed, 41 insertions(+), 6 deletions(-)
@@ -16,6 +16,7 @@ static int update_refs(const struct reset_head_opts *opts,unsignedrun_hook=opts->flags&RESET_HEAD_RUN_POST_CHECKOUT_HOOK;unsignedupdate_orig_head=opts->flags&RESET_ORIG_HEAD;constchar*switch_to_branch=opts->branch;+constchar*reflog_branch=opts->branch_msg;constchar*reflog_head=opts->head_msg;constchar*reflog_orig_head=opts->orig_head_msg;constchar*default_reflog_action=opts->default_reflog_action;
@@ -58,8 +59,9 @@ static int update_refs(const struct reset_head_opts *opts,detach_head?REF_NO_DEREF:0,UPDATE_REFS_MSG_ON_ERR);else{-ret=update_ref(reflog_head,switch_to_branch,oid,-NULL,0,UPDATE_REFS_MSG_ON_ERR);+ret=update_ref(reflog_branch?reflog_branch:reflog_head,+switch_to_branch,oid,NULL,0,+UPDATE_REFS_MSG_ON_ERR);if(!ret)ret=create_symref("HEAD",switch_to_branch,reflog_head);
@@ -90,6 +92,12 @@ int reset_head(struct repository *r, const struct reset_head_opts *opts)if(switch_to_branch&&!starts_with(switch_to_branch,"refs/"))BUG("Not a fully qualified branch: '%s'",switch_to_branch);+if(opts->orig_head_msg&&!update_orig_head)+BUG("ORIG_HEAD reflog message given without updating ORIG_HEAD");++if(opts->branch_msg&&!opts->branch)+BUG("branch reflog message given without a branch");+if(!refs_only&&repo_hold_locked_index(r,&lock,LOCK_REPORT_ON_ERROR)<0){ret=-1;gotoleave_reset_head;
From: Phillip Wood via GitGitGadget <hidden> Date: 2022-01-26 13:06:21
From: Phillip Wood <redacted>
At the start of a rebase, ORIG_HEAD is updated to the tip of the
branch being rebased. Unfortunately reset_head() always uses the
current value of HEAD for this which is incorrect if the rebase is
started with "git rebase <upstream> <branch>" as in that case
ORIG_HEAD should be updated to <branch>. This only affects the "apply"
backend as the "merge" backend does not yet use reset_head() for the
initial checkout. Fix this by passing in orig_head when calling
reset_head() and add some regression tests.
Signed-off-by: Phillip Wood <redacted>
---
builtin/rebase.c | 1 +
reset.c | 4 +++-
reset.h | 4 ++++
t/t3418-rebase-continue.sh | 26 ++++++++++++++++++++++++++
4 files changed, 34 insertions(+), 1 deletion(-)
From: Phillip Wood via GitGitGadget <hidden> Date: 2022-01-26 13:06:23
From: Phillip Wood <redacted>
Now that reset_head() can handle the initial checkout of onto
correctly use it in the "merge" backend instead of forking "git
checkout". This opens the way for us to stop calling the
post-checkout hook in the future. Not running "git checkout" means
that "rebase -i/m" no longer recurse submodules when checking out
"onto" (thanks to Philippe Blain for pointing this out). As the rest
of rebase does not know what to do with submodules this is probably a
good thing. When using merge-ort rebase ought be able to handle
submodules correctly if it parsed the submodule config, such a change
is left for a future patch series.
The "apply" based rebase has avoided forking git checkout
since ac7f467fef ("builtin/rebase: support running "git rebase
<upstream>"", 2018-08-07). The code that handles the checkout was
moved into libgit by b309a97108 ("reset: extract reset_head() from
rebase", 2020-04-07).
Signed-off-by: Phillip Wood <redacted>
---
sequencer.c | 38 +++++++++++---------------------------
1 file changed, 11 insertions(+), 27 deletions(-)
...and then for some of the ones like this "ropts.head_msg = buf.buf"
assignment you just do that one immediately after the strbuf_addf() or
whatever modifies it.
That way it's clear what options we get from the function arguments and
can populate right away, and which ones we need to run some code in the
function before we can update "ropts".
[Ditto for the elided parts below]
#define GIT_REFLOG_ACTION_ENVIRONMENT "GIT_REFLOG_ACTION"
+/* Request a detached checkout */
#define RESET_HEAD_DETACH (1<<0)
+/* Request a reset rather than a checkout */
#define RESET_HEAD_HARD (1<<1)
+/* Run the post-checkout hook */
#define RESET_HEAD_RUN_POST_CHECKOUT_HOOK (1<<2)
+/* Only update refs, do not touch the worktree */
#define RESET_HEAD_REFS_ONLY (1<<3)
+/* Update ORIG_HEAD as well as HEAD */
#define RESET_ORIG_HEAD (1<<4)
-int reset_head(struct repository *r, struct object_id *oid,
- const char *switch_to_branch, unsigned flags,
- const char *reflog_orig_head, const char *reflog_head,
- const char *default_reflog_action);
+struct reset_head_opts {
+ /*
+ * The commit to checkout/reset to. Defaults to HEAD.
+ */
+ const struct object_id *oid;
+ /*
+ * Optional branch to switch to.
+ */
+ const char *branch;
+ /*
+ * Flags defined above.
+ */
+ unsigned flags;
It's nice to make these sort of things an enum type for the reasons
explained in 3f9ab7ccdea (parse-options.[ch]: consistently use "enum
parse_opt_flags", 2021-10-08), i.e. gdb and the like will give you the
labels in the debugger.
Hi Ævar
On 26/01/2022 13:35, Ævar Arnfjörð Bjarmason wrote:
On Wed, Jan 26 2022, Phillip Wood via GitGitGadget wrote:
quoted
@@ -669,13 +672,15 @@ static int run_am(struct rebase_options *opts) status = run_command(&format_patch); if (status) {+ struct reset_head_opts ropts = { 0 }; unlink(rebased_patches); free(rebased_patches); strvec_clear(&am.args);- reset_head(the_repository, &opts->orig_head,- opts->head_name, 0,- NULL, NULL, DEFAULT_REFLOG_ACTION);+ ropts.oid = &opts->orig_head;+ ropts.branch = opts->head_name;+ ropts.default_reflog_action = DEFAULT_REFLOG_ACTION;+ reset_head(the_repository, &ropts); error(_("\ngit encountered an error while preparing the " "patches to replay\n" "these revisions:\n"
Wouldn't these and the rest be easier to read as:
struct reset_head_opts ropts = {
.oid = &opts->orig_head,
.branch = opts->head_name,
.default_reflog_action = DEFAULT_REFLOG_ACTION,
};
I did start out doing something like that but changed to the current
style as I felt it made it easier to convert the calls correctly and for
reviewers to verify that the conversion is correct when the deletion of
the old function arguments is adjacent to the insertion of the new
struct assignments and the assignments are in the same order as the old
function arguments.
...and then for some of the ones like this "ropts.head_msg = buf.buf"
assignment you just do that one immediately after the strbuf_addf() or
whatever modifies it.
That way it's clear what options we get from the function arguments and
can populate right away, and which ones we need to run some code in the
function before we can update "ropts".
I'm not immediately clear why that matters. My priority was to keep the
assignments in the same order an the old function arguments to make the
conversion and review easier.
[Ditto for the elided parts below]
quoted
#define GIT_REFLOG_ACTION_ENVIRONMENT "GIT_REFLOG_ACTION"
+/* Request a detached checkout */
#define RESET_HEAD_DETACH (1<<0)
+/* Request a reset rather than a checkout */
#define RESET_HEAD_HARD (1<<1)
+/* Run the post-checkout hook */
#define RESET_HEAD_RUN_POST_CHECKOUT_HOOK (1<<2)
+/* Only update refs, do not touch the worktree */
#define RESET_HEAD_REFS_ONLY (1<<3)
+/* Update ORIG_HEAD as well as HEAD */
#define RESET_ORIG_HEAD (1<<4)
-int reset_head(struct repository *r, struct object_id *oid,
- const char *switch_to_branch, unsigned flags,
- const char *reflog_orig_head, const char *reflog_head,
- const char *default_reflog_action);
+struct reset_head_opts {
+ /*
+ * The commit to checkout/reset to. Defaults to HEAD.
+ */
+ const struct object_id *oid;
+ /*
+ * Optional branch to switch to.
+ */
+ const char *branch;
+ /*
+ * Flags defined above.
+ */
+ unsigned flags;
It's nice to make these sort of things an enum type for the reasons
explained in 3f9ab7ccdea (parse-options.[ch]: consistently use "enum
parse_opt_flags", 2021-10-08), i.e. gdb and the like will give you the
labels in the debugger.
Yeah that's true but I'm not actually touching the flags here, just
adding comments.
Best Wishes
Phillip
On Wed, Jan 26, 2022 at 5:05 AM Phillip Wood via GitGitGadget
[off-list ref] wrote:
Thanks to Junio and Elijah for their comments on V2. There aren't too many
changes this time.
This series started out with the aim of converting 'rebase -i' to use
reset_head() instead of forking 'git checkout'. As 'rebase --apply' already
uses reset_head() I assumed this would be straight forward. However it has
morphed into a series of fixes for reset_head() followed by the intended
conversion of 'rebase -i'.
reset.c::reset_head() started its life at ac7f467f (builtin/rebase: support
running "git rebase ", 2018-08-07) as a way to detach the HEAD to replay the
commits during "git rebase", but over time it learned to do many things,
like switching the tip of the branch to another commit, recording the old
value of HEAD in ORIG_HEAD while it does so, recording reflog entries for
both HEAD and for the branch.
The API into the function got clunky and it is harder than necessary for the
callers to use the function correctly, which led to a handful of bugs that
are fixed by this series. The bugs include
* passing the wrong oid to the post-checkout hook
* removing untracked files on checkout
* running the post-checkout hook if the checkout fails
* passing parameters to reset_head() that it does not use
* incorrect reflog messages for 'rebase --apply'
* sometimes setting ORIG_HEAD incorrectly at the start 'rebase --apply'
Later steps of this series revamps the API so that it is harder to use it
incorrectly to prevent future bugs and finally convert 'rebase -i' to use
reset_head()
Changes since V2:
* Updated cover letter as suggested by Junio
* Patch 4 - fixed typos in the commit message spotted by Junio
* Patch 9 - Moved later in the series to simplify the autostash canges as
suggested by Junio. This used to be patch 7
* Patch 10 - Added a comment to the commit message explaining why we cannot
BUG() on an invalid parameter until a change is made in a later commit
* Patch 13 - Reworded the first sentence commit message as suggest by
Elijah.
This round looks good to me. Thanks!
Cover letter for V2:
Thanks for the comments on V1. I have tried to improve the commit messages
to explain better the motivation and implications of the changes in this
series and I have added some more tests. I have rebased onto v2.34.0 to
avoid some merges conflicts.
Changes since V1:
* Patch 1 - unchanged.
* Patches 2, 3 - these are new and fix an bug I noticed while adding a test
to patch 4.
* Patches 4, 5 - improved commit messages and added tests.
* Patch 6 - reworded commit message.
* Patch 7 - split out some changes that used to be in patch 9.
* Patch 8 - in principle the same but the range-diff is noisy due to the
addition of patch 3.
* Patch 9 - reworded commit message.
* Patch 10 - unchanged.
* Patch 11 - reworded commit message and a couple of comments.
* Patch 12 - minor changes to comments.
* Patch 13 - cosmetic changes to commit message and tests.
* Patch 14 - cosmetic changes to commit message.
Cover letter for V1: Fix some issues with the implementation and use of
reset_head(). The last patch was previously posted as [1], I have updated
the commit message and rebased it onto the fixes in this series. There are a
couple of small conflicts merging this into seen, I think they should be
easy to resolve (in rebase.c take both sides in reset.c take the changed
lines from each side). These patches are based on pw/rebase-of-a-tag-fix
[1]
https://lore.kernel.org/git/39ad40c9297531a2d42b7263a1d41b1ecbc23c0a.1631108472.git.gitgitgadget@gmail.com/
Phillip Wood (14):
rebase: factor out checkout for up to date branch
t5403: refactor rebase post-checkout hook tests
rebase: pass correct arguments to post-checkout hook
rebase: do not remove untracked files on checkout
rebase --apply: don't run post-checkout hook if there is an error
reset_head(): remove action parameter
reset_head(): factor out ref updates
reset_head(): make default_reflog_action optional
create_autostash(): remove unneeded parameter
rebase: cleanup reset_head() calls
reset_head(): take struct rebase_head_opts
rebase --apply: fix reflog
rebase --apply: set ORIG_HEAD correctly
rebase -m: don't fork git checkout
builtin/merge.c | 6 +-
builtin/rebase.c | 101 +++++++++++++----------
reset.c | 149 ++++++++++++++++++++--------------
reset.h | 48 ++++++++++-
sequencer.c | 47 ++++-------
sequencer.h | 3 +-
t/t3406-rebase-message.sh | 23 ++++++
t/t3418-rebase-continue.sh | 26 ++++++
t/t5403-post-checkout-hook.sh | 67 +++++++++++----
9 files changed, 312 insertions(+), 158 deletions(-)
base-commit: cd3e606211bb1cf8bc57f7d76bab98cc17a150bc
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1049%2Fphillipwood%2Fwip%2Frebase-reset-head-fixes-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1049/phillipwood/wip/rebase-reset-head-fixes-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1049
Range-diff vs v2:
1: 0e84d00572e = 1: 0e84d00572e rebase: factor out checkout for up to date branch
2: a67a5a03b94 = 2: a67a5a03b94 t5403: refactor rebase post-checkout hook tests
3: 07867760e68 = 3: 07867760e68 rebase: pass correct arguments to post-checkout hook
4: 2b499704c8f ! 4: f4b925508e7 rebase: do not remove untracked files on checkout
@@ Commit message
<upstream> is an ancestor of <branch> then it will fast-forward and
checkout <branch>. Normally a checkout or picking a commit during a
rebase will refuse to overwrite untracked files, however rebase does
- overwrite untracked files when checking <branch>.
+ overwrite untracked files when checking out <branch>.
The fix is to only set reset in `unpack_tree_opts` if flags contains
`RESET_HEAD_HARD`. t5403 may seem like an odd home for the new test
but it will be extended in the next commit to check that the
post-checkout hook is not run when the checkout fails.
- The test for `!deatch_head` dates back to the
+ The test for `!detach_head` dates back to the
original implementation of reset_head() in
ac7f467fef ("builtin/rebase: support running "git rebase <upstream>"",
2018-08-07) and was correct until e65123a71d
5: 04e7340a7e7 = 5: 4de5104d22d rebase --apply: don't run post-checkout hook if there is an error
6: 32ffa98c1bc = 6: ff23498e93e reset_head(): remove action parameter
8: 29e06e7d36d = 7: 688ebc45bf7 reset_head(): factor out ref updates
9: 9d00a218daf ! 8: a5cc7eaa925 reset_head(): make default_reflog_action optional
@@ reset.c: int reset_head(struct repository *r, struct object_id *oid,
leave_reset_head:
rollback_lock_file(&lock);
-
- ## sequencer.c ##
-@@ sequencer.c: void create_autostash(struct repository *r, const char *path)
- write_file(path, "%s", oid_to_hex(&oid));
- printf(_("Created autostash: %s\n"), buf.buf);
- if (reset_head(r, NULL, NULL, RESET_HEAD_HARD, NULL, NULL,
-- "") < 0)
-+ NULL) < 0)
- die(_("could not reset --hard"));
-
- if (discard_index(r->index) < 0 ||
7: 341fe183c18 ! 9: dd3a22384d2 create_autostash(): remove unneeded parameter
@@ sequencer.c: void create_autostash(struct repository *r, const char *path,
printf(_("Created autostash: %s\n"), buf.buf);
if (reset_head(r, NULL, NULL, RESET_HEAD_HARD, NULL, NULL,
- default_reflog_action) < 0)
-+ "") < 0)
++ NULL) < 0)
die(_("could not reset --hard"));
if (discard_index(r->index) < 0 ||
10: 5ea636009e7 ! 10: ad7c6467987 rebase: cleanup reset_head() calls
@@ Commit message
currently possible so we delay fixing that caller until we can pass
the message as the branch reflog.
+ A later commit will make it a BUG() to pass reflog_orig_head without
+ RESET_ORIG_HEAD, that changes cannot be done here as it needs to wait
+ for move_to_original_branch() to be fixed first.
+
Signed-off-by: Phillip Wood [off-list ref]
## builtin/rebase.c ##
11: 24b0566aba5 = 11: d170703e833 reset_head(): take struct rebase_head_opts
12: dc5d11291e7 = 12: 4973892561e rebase --apply: fix reflog
13: 45a5b5e9818 ! 13: 0ef0e978112 rebase --apply: set ORIG_HEAD correctly
@@ Metadata
## Commit message ##
rebase --apply: set ORIG_HEAD correctly
- At the start of a rebase ORIG_HEAD is updated to tip of the branch
- being rebased. Unfortunately reset_head() always uses the current
- value of HEAD for this which is incorrect if the rebase is started
- with "git rebase <upstream> <branch>" as in that case ORIG_HEAD should
- be updated to <branch>. This only affects the "apply" backend as the
- "merge" backend does not yet use reset_head() for the initial
- checkout. Fix this by passing in orig_head when calling reset_head()
- and add some regression tests.
+ At the start of a rebase, ORIG_HEAD is updated to the tip of the
+ branch being rebased. Unfortunately reset_head() always uses the
+ current value of HEAD for this which is incorrect if the rebase is
+ started with "git rebase <upstream> <branch>" as in that case
+ ORIG_HEAD should be updated to <branch>. This only affects the "apply"
+ backend as the "merge" backend does not yet use reset_head() for the
+ initial checkout. Fix this by passing in orig_head when calling
+ reset_head() and add some regression tests.
Signed-off-by: Phillip Wood [off-list ref]
14: 3f64b9274b5 = 14: 9b9560ef676 rebase -m: don't fork git checkout
--
gitgitgadget