Thread (9 messages) flat view 9 messages, 3 authors, 2018-08-15

Re: [PATCH 2/2] cherry-pick: fix --quit not deleting CHERRY_PICK_HEAD

From: Stefan Beller <hidden>
Date: 2018-08-15 17:09:57

On Wed, Aug 15, 2018 at 9:23 AM Nguyễn Thái Ngọc Duy [off-list ref] wrote:
quoted hunk ↗ jump to hunk
--quit is supposed to be --abort but without restoring HEAD. Leaving
CHERRY_PICK_HEAD behind could make other commands mistake that
cherry-pick is still ongoing (e.g. "git commit --amend" will refuse to
work). Clean it too.

For abort, this job of deleting CHERRY_PICK_HEAD is on "git reset" so
we don't need to do anything else.

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 builtin/revert.c                | 9 +++++++--
 t/t3510-cherry-pick-sequence.sh | 3 ++-
 2 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/builtin/revert.c b/builtin/revert.c
index 76f0a35b07..e94a4ead2b 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -7,6 +7,7 @@
 #include "rerere.h"
 #include "dir.h"
 #include "sequencer.h"
+#include "branch.h"

 /*
  * This implements the builtins revert and cherry-pick.
@@ -191,8 +192,12 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
        opts->gpg_sign = xstrdup_or_null(opts->gpg_sign);
        opts->strategy = xstrdup_or_null(opts->strategy);

-       if (cmd == 'q')
-               return sequencer_remove_state(opts);
+       if (cmd == 'q') {
+               int ret = sequencer_remove_state(opts);
+               if (!ret)
+                       remove_branch_state(the_repository);
Technically you would not need patch 1 in this series, as you could call
remove_branch_state(void) as before that patch to do the same thing here.
I guess that patch 1 is more of a drive by cleanup, then?

It looks a bit interesting as sequencer_remove_state actually
takes no arguments and assumes the_repository, but I guess that is fine.

I wondered if we need to have this patch for 'a' as well, and it looks like
which does a sequencer_rollback, which is just some logic before
attempting sequencer_remove_state. So I'd think remove_branch_state
could be done in sequencer_remove_state as well?

Or are there functions that would definitely not want sequencer_remove_state
after sequencer_remove_state?

Going down on that I just realize sequencer_remove_state could also
be returning void, as of now it always returns 0, so the condition here
with !ret is just confusing the reader?

Thanks,
Stefan
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help