diff mbox series

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

Message ID 8d1dec51b704c45bf36af24f657cc40f006989e2.1542065154.git.gitgitgadget@gmail.com
State New, archived
Headers show
Series Assorted fixes revolving around rebase and merges | expand

Commit Message

Elijah Newren via GitGitGadget Nov. 12, 2018, 11:26 p.m. UTC
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(+)

Comments

Junio C Hamano Nov. 13, 2018, 2:11 a.m. UTC | #1
"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?  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.

>  		ret = finish_rebase(&options);
>  		goto cleanup;
>  	}
Johannes Schindelin Nov. 13, 2018, 10:14 a.m. UTC | #2
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;
> >  	}
>
diff mbox series

Patch

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();
 		ret = finish_rebase(&options);
 		goto cleanup;
 	}