git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Stefan Beller <sbeller@google.com>
Date
Aug 15, 2018, 17:09 UTC
Message-ID
<CAGZ79kYo-icUHfezTMg77s_2GtKcgH6jysNTu11XOoL4UT-9bQ@mail.gmail.com>
In-Reply-To
<20180815162308.17747-2-pclouds@gmail.com>
On Wed, Aug 15, 2018 at 9:23 AM Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
Show 37 quoted lines
>
> --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 <pclouds@gmail.com>
> ---
>  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

Previous: Nguyễn Thái Ngọc DuyNext: Duy Nguyen
Message 3 of 18 in “branch.c: remove explicit reference to the_repository”
  1. 1/2 branch.c: remove explicit reference to the_repositoryNguyễn Thái Ngọc Duy, Aug 15, 2018
  2. 2/2 cherry-pick: fix --quit not deleting CHERRY_PICK_HEADNguyễn Thái Ngọc Duy, Aug 15, 2018
  3. Stefan BellerAug 15, 2018
  4. Duy NguyenAug 15, 2018
  5. Stefan BellerAug 15, 2018
  6. Junio C HamanoAug 15, 2018
  7. Duy NguyenAug 15, 2018
  8. Junio C HamanoAug 15, 2018
  9. cherry-pick: fix --quit not deleting CHERRY_PICK_HEADNguyễn Thái Ngọc Duy, Aug 16, 2018
  10. Junio C HamanoAug 16, 2018
  11. Elijah NewrenAug 15, 2018
  12. Duy NguyenAug 15, 2018
  13. Stefan BellerAug 15, 2018
  14. Duy NguyenAug 15, 2018
  15. Junio C HamanoAug 15, 2018
  16. Duy NguyenAug 15, 2018
  17. Junio C HamanoAug 15, 2018
  18. Duy NguyenAug 15, 2018

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.