Re: [PATCH 3/4] rebase: teach `reset_head()` to optionally skip the worktree

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Junio,

On Fri, 4 Jan 2019, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
> 
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > This is what the legacy (scripted) rebase does in
> > `move_to_original_branch`, and we will need this functionality in the
> > next commit.
> 
> The move-to-original-branch helper does:
> 
>     - point $head_name to the commit pointed at by HEAD
>     - point HEAD symref to $head_name
> 
> without touching the index or the working tree files.  It's not
> exactly "reset --soft" but more like "switch-branch --soft" ;-)

Right, but it does reset the HEAD softly.

> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  builtin/rebase.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/builtin/rebase.c b/builtin/rebase.c
> > index 768bea0da8..303175fdf1 100644
> > --- a/builtin/rebase.c
> > +++ b/builtin/rebase.c
> > @@ -337,6 +337,7 @@ static void add_var(struct strbuf *buf, const char *name, const char *value)
> >  
> >  #define RESET_HEAD_DETACH (1<<0)
> >  #define RESET_HEAD_HARD (1<<1)
> > +#define RESET_HEAD_REFS_ONLY (1<<2)
> 
> In the future codebase in 'pu', we have 1<<2 already taken by
> another topic, so I'll tell my rerere database that the bit
> assignment needs to be adjusted.

Okay.

> >  static int reset_head(struct object_id *oid, const char *action,
> >  		      const char *switch_to_branch, unsigned flags,
> > @@ -344,6 +345,7 @@ static int reset_head(struct object_id *oid, const char *action,
> >  {
> >  	unsigned detach_head = flags & RESET_HEAD_DETACH;
> >  	unsigned reset_hard = flags & RESET_HEAD_HARD;
> > +	unsigned refs_only = flags & RESET_HEAD_REFS_ONLY;
> >  	struct object_id head_oid;
> >  	struct tree_desc desc[2] = { { NULL }, { NULL } };
> >  	struct lock_file lock = LOCK_INIT;
> > @@ -359,7 +361,7 @@ static int reset_head(struct object_id *oid, const char *action,
> >  	if (switch_to_branch && !starts_with(switch_to_branch, "refs/"))
> >  		BUG("Not a fully qualified branch: '%s'", switch_to_branch);
> >  
> > -	if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0) {
> > +	if (!refs_only && hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0) {
> >  		ret = -1;
> >  		goto leave_reset_head;
> >  	}
> 
> Not touching the index, so no need to lock the index.  Makes sense.
> 
> > @@ -372,6 +374,9 @@ static int reset_head(struct object_id *oid, const char *action,
> >  	if (!oid)
> >  		oid = &head_oid;
> >  
> > +	if (flags & RESET_HEAD_REFS_ONLY)
> > +		goto reset_head_refs;
> > +
> 
> Why not "refs_only" that we already prepared above???  Are we
> munging that secondary variable before control comes here?

No, just an oversight. Will fix.

> In any case, not touching the index nor the working tree, so no need
> to call into the unpack_trees machinery.  Makes sense.
> 
> >  	memset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));
> >  	setup_unpack_trees_porcelain(&unpack_tree_opts, action);
> >  	unpack_tree_opts.head_idx = 1;
> > @@ -412,6 +417,7 @@ static int reset_head(struct object_id *oid, const char *action,
> >  		goto leave_reset_head;
> >  	}
> >  
> > +reset_head_refs:
> >  	reflog_action = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);
> 
> And the control continues from the point we update the reflog.
> Makes sense.
> 
> >  	strbuf_addf(&msg, "%s: ", reflog_action ? reflog_action : "rebase");
> >  	prefix_len = msg.len;
> 
> This helper is touched by two other topics in flight, and that was
> one of the reason why it took a bit longer than usual for me to
> merge this topic.  Please sanity-check the result of the conflict
> resolution at the tip of 'pu' branch.

Sorry, I *just now* had time to take care of this patch series. I will
gladly check once you integrate the new patch series iteration.

Thanks,
Dscho