Re: [PATCH 4/5] built-in rebase --skip/--abort: clean up stale .git/<name> files

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

Hi Junio,

On Tue, 13 Nov 2018, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
> 
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > The scripted version of the rebase used to execute `git reset --hard`
> > when skipping or aborting. When we ported this to C, we did update the
> > worktree and some reflogs, but we failed to imitate `git reset --hard`'s
> > behavior regarding files in .git/ such as MERGE_HEAD.
> >
> > Let's address this oversight.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  builtin/rebase.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/builtin/rebase.c b/builtin/rebase.c
> > index 0ee06aa363..017dce1b50 100644
> > --- a/builtin/rebase.c
> > +++ b/builtin/rebase.c
> > @@ -23,6 +23,7 @@
> >  #include "revision.h"
> >  #include "commit-reach.h"
> >  #include "rerere.h"
> > +#include "branch.h"
> >  
> >  static char const * const builtin_rebase_usage[] = {
> >  	N_("git rebase [-i] [options] [--exec <cmd>] [--onto <newbase>] "
> > @@ -1002,6 +1003,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> >  
> >  		if (reset_head(NULL, "reset", NULL, 0, NULL, NULL) < 0)
> >  			die(_("could not discard worktree changes"));
> > +		remove_branch_state();
> >  		if (read_basic_state(&options))
> >  			exit(1);
> >  		goto run_rebase;
> > @@ -1019,6 +1021,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> >  			       options.head_name, 0, NULL, NULL) < 0)
> >  			die(_("could not move back to %s"),
> >  			    oid_to_hex(&options.orig_head));
> > +		remove_branch_state();
> 
> Hmph.  Among 5 or so callsites of reset_head(), only these two
> places need it, so reset_head() is clearly not a substitute for
> "reset --hard HEAD", and it sometimes used to switch branches or
> something?

Indeed. There is also the `git reset --hard` call in the scripted
version's autostash code path. But that definitely does not need to remove
the branch state, as it is not recovering from a merge or cherry-pick or
revert.

> Perhaps we may need to rename it to avoid confusion but it can wait
> until we actually decide to make it externally available.  Until then,
> it's OK as-is, I would think.

Okay.

Ciao,
Dscho

> 
> >  		ret = finish_rebase(&options);
> >  		goto cleanup;
> >  	}
>