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

Re: [PATCH 2/7] revert: allow cherry-pick --continue to commit before resuming

From
Ramkumar Ramachandra <artagnon@gmail.com>
Date
Dec 14, 2011, 14:26 UTC
Message-ID
<CALkWK0miBzT4BXRDYOhz8JqF2jeyz1L3=pwaGKVm654oHtQbtQ@mail.gmail.com>
In-Reply-To
<20111210124925.GC22035@elie.hsd1.il.comcast.net>
Hi again,
Jonathan Nieder wrote:
> When "git cherry-pick ..bar" encounters conflicts, permit the operator
> to use cherry-pick --continue after resolving them as a shortcut for
> "git commit && git cherry-pick --continue" to record the resolution
> and carry on with the rest of the sequence.

Sounds good. I remember my implementation being quite complicated; let's see how you've done this.

Show 19 quoted lines
> Example: after encountering a conflict from running "git cherry-pick
> foo bar baz":
>
>        CONFLICT (content): Merge conflict in main.c
>        error: could not apply f78a8d98c... bar!
>        hint: after resolving the conflicts, mark the corrected paths
>        hint: with 'git add <paths>' or 'git rm <paths>'
>        hint: and commit the result with 'git commit'
>
> We edit main.c to resolve the conflict, mark it acceptable with "git
> add main.c", and can run "cherry-pick --continue" to resume the
> sequence.
>
>        $ git cherry-pick --continue
>        [editor opens to confirm commit message]
>        [master 78c8a8c98] bar!
>         1 files changed, 1 insertions(+), 1 deletions(-)
>        [master 87ca8798c] baz!
>         1 files changed, 1 insertions(+), 1 deletions(-)
I like the presentation of this example: much clearer than my examples.
>  builtin/revert.c                |   23 ++++++-
>  t/t3510-cherry-pick-sequence.sh |  139 ++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 156 insertions(+), 6 deletions(-)
Oh, good -- lots of new tests :)
Show 17 quoted lines
> diff --git a/builtin/revert.c b/builtin/revert.c
> index 9f6c85c1..a43b4d85 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -1038,18 +1038,35 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
>        return 0;
>  }
>
> +static int continue_single_pick(void)
> +{
> +       const char *argv[] = { "commit", NULL };
> +
> +       if (!file_exists(git_path("CHERRY_PICK_HEAD")) &&
> +           !file_exists(git_path("REVERT_HEAD")))
> +               return error(_("no cherry-pick or revert in progress"));
> +       return run_command_v_opt(argv, RUN_GIT_CMD);
> +}

Very nice! I can see how the introduction of REVERT_HEAD simplifies things :) I'm totally embarrassed by the horribly convoluted logic in the "New sequencer workflow!" I posted earlier.

Note to self: don't capitalize error() messages.
Show 24 quoted lines
>  static int sequencer_continue(struct replay_opts *opts)
>  {
>        struct commit_list *todo_list = NULL;
>
>        if (!file_exists(git_path(SEQ_TODO_FILE)))
> -               return error(_("No %s in progress"), action_name(opts));
> +               return continue_single_pick();
>        read_populate_opts(&opts);
>        read_populate_todo(&todo_list, opts);
>
>        /* Verify that the conflict has been resolved */
> -       if (!index_differs_from("HEAD", 0))
> -               todo_list = todo_list->next;
> +       if (file_exists(git_path("CHERRY_PICK_HEAD")) ||
> +           file_exists(git_path("REVERT_HEAD"))) {
> +               int ret = continue_single_pick();
> +               if (ret)
> +                       return ret;
> +       }
> +       if (index_differs_from("HEAD", 0))
> +               return error_dirty_index(opts);
> +       todo_list = todo_list->next;
>        return pick_commits(todo_list, opts);
>  }

Very nicely done. I can see why 1/7 makes so much sense now: it helps think of different operations independently.

Show 12 quoted lines
> diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
> index 2c4c1c85..4d1883b7 100755
> --- a/t/t3510-cherry-pick-sequence.sh
> +++ b/t/t3510-cherry-pick-sequence.sh
> @@ -2,6 +2,7 @@
>
>  test_description='Test cherry-pick continuation features
>
> + +  conflicting: rewrites unrelated to conflicting
>   + yetanotherpick: rewrites foo to e
>   + anotherpick: rewrites foo to d
>   + picked: rewrites foo to c

Note to self: this list of commits is becoming quite unwieldy. We should probably refactor these sometime.

Show 8 quoted lines
> @@ -27,6 +28,7 @@ test_cmp_rev () {
>  }
>
>  test_expect_success setup '
> +       git config advice.detachedhead false
>        echo unrelated >unrelated &&
>        git add unrelated &&
>        test_commit initial foo a &&

Huh, why are you moving this line up? Oh, right: there are "test_commit" statements in the setup- good catch. This is unrelated to your patch and should be a separate commit though.

Show 9 quoted lines
> @@ -35,8 +37,8 @@ test_expect_success setup '
>        test_commit picked foo c &&
>        test_commit anotherpick foo d &&
>        test_commit yetanotherpick foo e &&
> -       git config advice.detachedhead false
> -
> +       pristine_detach initial &&
> +       test_commit conflicting unrelated
>  '
Looks fishy- I wonder why you're doing this.  Let's read ahead and find out.
Show 18 quoted lines
> @@ -243,7 +245,66 @@ test_expect_success '--continue complains when there are unresolved conflicts' '
>        test_must_fail git cherry-pick --continue
>  '
>
> -test_expect_success '--continue continues after conflicts are resolved' '
> +test_expect_success '--continue of single cherry-pick' '
> +       pristine_detach initial &&
> +       echo c >expect &&
> +       test_must_fail git cherry-pick picked &&
> +       echo c >foo &&
> +       git add foo &&
> +       git cherry-pick --continue &&
> +
> +       test_cmp expect foo &&
> +       test_cmp_rev initial HEAD^ &&
> +       git diff --exit-code HEAD &&
> +       test_must_fail git rev-parse --verify CHERRY_PICK_HEAD
> +'
Beautiful.  The tests I wrote are ugly in comparison :\
Show 8 quoted lines
> +test_expect_success '--continue of single revert' '
> +       pristine_detach initial &&
> +       echo resolved >expect &&
> +       echo "Revert \"picked\"" >expect.msg &&
> +       test_must_fail git revert picked &&
> +       echo resolved >foo &&
> +       git add foo &&
> +       git cherry-pick --continue &&

Huh? You're continuing a "git revert" with a a "git cherry-pick --continue"? The current 'master' still uses a commit_list, and doesn't allow mixed "pick" and "revert" instructions yet.

Show 8 quoted lines
> +       git diff --exit-code HEAD &&
> +       test_cmp expect foo &&
> +       test_cmp_rev initial HEAD^ &&
> +       git diff-tree -s --pretty=tformat:%s HEAD >msg &&
> +       test_cmp expect.msg msg &&
> +       test_must_fail git rev-parse --verify CHERRY_PICK_HEAD &&
> +       test_must_fail git rev-parse --verify REVERT_HEAD
> +'
A couple of notes:
1. I haven't used the "-s" flag of "git diff-tree" before, so I opened
up the documentation to find this:
        By default, 'git diff-tree --stdin' shows differences,
        either in machine-readable form (without '-p') or in patch
        form (with '-p').  This output can be suppressed.  It is
        only useful with '-v' flag.
Very misleading.  TODO: Fix this.
2. Why did you use "diff-tree" to get the commit message?  Isn't
"cat-file commit" much more straightforward?
Show 26 quoted lines
> +test_expect_success '--continue after resolving conflicts' '
> +       pristine_detach initial &&
> +       echo d >expect &&
> +       cat >expect.log <<-\EOF &&
> +       OBJID
> +       :100644 100644 OBJID OBJID M    foo
> +       OBJID
> +       :100644 100644 OBJID OBJID M    foo
> +       OBJID
> +       :100644 100644 OBJID OBJID M    unrelated
> +       OBJID
> +       :000000 100644 OBJID OBJID A    foo
> +       :000000 100644 OBJID OBJID A    unrelated
> +       EOF
> +       test_must_fail git cherry-pick base..anotherpick &&
> +       echo c >foo &&
> +       git add foo &&
> +       git cherry-pick --continue &&
> +       {
> +               git rev-list HEAD |
> +               git diff-tree --root --stdin |
> +               sed "s/$_x40/OBJID/g"
> +       } >actual.log &&
> +       test_cmp expect foo &&
> +       test_cmp expect.log actual.log
> +'

Unchanged from the original: I suspect you've moved the generation of expectation messages up to produce a clean diff.

> +test_expect_success '--continue after resolving conflicts and committing' '
>        pristine_detach initial &&
>        test_must_fail git cherry-pick base..anotherpick &&
>        echo "c" >foo &&
Okay, the diff isn't all that clean :P
Show 9 quoted lines
> +test_expect_success '--continue asks for help after resolving patch to nil' '
> +       pristine_detach conflicting &&
> +       test_must_fail git cherry-pick initial..picked &&
> +
> +       test_cmp_rev unrelatedpick CHERRY_PICK_HEAD &&
> +       git checkout HEAD -- unrelated &&
> +       test_must_fail git cherry-pick --continue 2>msg &&
> +       test_i18ngrep "The previous cherry-pick is now empty" msg
> +'

I thought it was a bad idea to grep for specific output messages, because of their volatile nature? Remind me what this test has to do with the rest of your patch?

Show 12 quoted lines
> +test_expect_failure 'follow advice and skip nil patch' '
> +       pristine_detach conflicting &&
> +       test_must_fail git cherry-pick initial..picked &&
> +
> +       git checkout HEAD -- unrelated &&
> +       test_must_fail git cherry-pick --continue &&
> +       git reset &&
> +       git cherry-pick --continue &&
> +
> +       git rev-list initial..HEAD >commits &&
> +       test_line_count = 3 commits
> +'
Again, what does this test have to do with the rest of your patch?
Show 17 quoted lines
>  test_expect_success '--continue respects opts' '
>        pristine_detach initial &&
>        test_must_fail git cherry-pick -x base..anotherpick &&
> @@ -288,6 +372,29 @@ test_expect_success '--continue respects opts' '
>        grep "cherry picked from" anotherpick_msg
>  '
>
> +test_expect_success '--continue of single-pick respects -x' '
> +       pristine_detach initial &&
> +       test_must_fail git cherry-pick -x picked &&
> +       echo c >foo &&
> +       git add foo &&
> +       git cherry-pick --continue &&
> +       test_path_is_missing .git/sequencer &&
> +       git cat-file commit HEAD >msg &&
> +       grep "cherry picked from" msg
> +'

I'd have liked s/respects -x/respects opts/ here for symmetry with the previous test.

Show 11 quoted lines
> +test_expect_success '--continue respects -x in first commit in multi-pick' '
> +       pristine_detach initial &&
> +       test_must_fail git cherry-pick -x picked anotherpick &&
> +       echo c >foo &&
> +       git add foo &&
> +       git cherry-pick --continue &&
> +       test_path_is_missing .git/sequencer &&
> +       git cat-file commit HEAD^ >msg &&
> +       picked=$(git rev-parse --verify picked) &&
> +       grep "cherry picked from.*$picked" msg
> +'
Can you explain why "first commit in a multi-pick" is a special case?
Show 16 quoted lines
> @@ -306,6 +413,32 @@ test_expect_success '--signoff is not automatically propagated to resolved confl
>        grep "Signed-off-by:" anotherpick_msg
>  '
>
> +test_expect_success '--signoff dropped for implicit commit of resolution, multi-pick case' '
> +       pristine_detach initial &&
> +       test_must_fail git cherry-pick -s picked anotherpick &&
> +       echo c >foo &&
> +       git add foo &&
> +       git cherry-pick --continue &&
> +
> +       git diff --exit-code HEAD &&
> +       test_cmp_rev initial HEAD^^ &&
> +       git cat-file commit HEAD^ >msg &&
> +       ! grep Signed-off-by: msg
> +'
Unrelated.
Show 12 quoted lines
> +test_expect_success 'sign-off needs to be reaffirmed after conflict resolution, single-pick case' '
> +       pristine_detach initial &&
> +       test_must_fail git cherry-pick -s picked &&
> +       echo c >foo &&
> +       git add foo &&
> +       git cherry-pick --continue &&
> +
> +       git diff --exit-code HEAD &&
> +       test_cmp_rev initial HEAD^ &&
> +       git cat-file commit HEAD >msg &&
> +       ! grep Signed-off-by: msg
> +'

If the previous test were a separate patch preceding this one, this'd belong here.

Thanks for working on this.
-- Ram
Previous: Jonathan NiederNext: Jonathan Nieder
Message 27 of 52 in “cherry-pick/revert error messages”
  1. Jonathan NiederNov 20, 2011
  2. Ramkumar RamachandraNov 20, 2011
  3. 0/3 Re: cherry-pick/revert error messagesJonathan Nieder, Nov 20, 2011
  4. 1/3 revert: rename --reset option to --quitJonathan Nieder, Nov 20, 2011
  5. Junio C HamanoNov 21, 2011
  6. Jakub NarebskiNov 21, 2011
  7. Jonathan NiederNov 21, 2011
  8. 2/3 revert: rearrange pick_revisions() for clarityJonathan Nieder, Nov 20, 2011
  9. 3/3 revert: improve error message for cherry-pick during cherry-pickJonathan Nieder, Nov 20, 2011
  10. 0/3 Re: cherry-pick/revert error messagesJonathan Nieder, Nov 22, 2011
  11. 1/3 revert: rename --reset option to --quitJonathan Nieder, Nov 22, 2011
  12. 2/3 revert: rearrange pick_revisions() for clarityJonathan Nieder, Nov 22, 2011
  13. 3/3 revert: improve error message for cherry-pick during cherry-pickJonathan Nieder, Nov 22, 2011
  14. 4/3 revert: write REVERT_HEAD pseudoref during conflicted revertJonathan Nieder, Nov 22, 2011
  15. Thiago FarinaNov 22, 2011
  16. Ramkumar RamachandraDec 1, 2011
  17. 5/3 revert: introduce --abort to cancel a failed cherry-pickJonathan Nieder, Nov 22, 2011
  18. Junio C HamanoNov 23, 2011
  19. Jonathan NiederNov 23, 2011
  20. Fix revert --abort on WindowsJohannes Sixt, Nov 23, 2011
  21. Jonathan NiederNov 23, 2011
  22. Johannes SixtNov 23, 2011
  23. 0/7 some sequencer loose ends (Re: Fix revert --abort on Windows)Jonathan Nieder, Dec 10, 2011
  24. 1/7 revert: give --continue handling its own functionJonathan Nieder, Dec 10, 2011
  25. Ramkumar RamachandraDec 14, 2011
  26. 2/7 revert: allow cherry-pick --continue to commit before resumingJonathan Nieder, Dec 10, 2011
  27. Ramkumar RamachandraDec 14, 2011
  28. Jonathan NiederDec 14, 2011
  29. 3/7 revert: pass around rev-list args in already-parsed formJonathan Nieder, Dec 10, 2011
  30. Ramkumar RamachandraDec 14, 2011
  31. 4/7 revert: allow single-pick in the middle of cherry-pick sequenceJonathan Nieder, Dec 10, 2011
  32. Ramkumar RamachandraDec 14, 2011
  33. Jonathan NiederDec 14, 2011
  34. Ævar Arnfjörð BjarmasonApr 5, 2012
  35. Jonathan NiederApr 5, 2012
  36. 5/7 revert: do not remove state until sequence is finishedJonathan Nieder, Dec 10, 2011
  37. Ramkumar RamachandraDec 14, 2011
  38. 6/7 Revert "reset: Make reset remove the sequencer state"Jonathan Nieder, Dec 10, 2011
  39. Ramkumar RamachandraDec 14, 2011
  40. 7/7 revert: stop creating and removing sequencer-old directoryJonathan Nieder, Dec 10, 2011
  41. Ramkumar RamachandraDec 14, 2011
  42. Jonathan NiederDec 11, 2011
  43. Junio C HamanoDec 12, 2011
  44. Junio C HamanoDec 12, 2011
  45. Jonathan NiederDec 14, 2011
  46. Alex RiesenNov 23, 2011
  47. Junio C HamanoNov 30, 2011
  48. 6/3 revert: remove --reset compatibility optionJonathan Nieder, Nov 22, 2011
  49. Junio C HamanoNov 22, 2011
  50. Jonathan NiederNov 22, 2011
  51. Junio C HamanoNov 22, 2011
  52. Jonathan NiederNov 22, 2011

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.