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

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.

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 :)

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.

 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.

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

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.

Looks fishy- I wonder why you're doing this.  Let's read ahead and find out.

Beautiful.  The tests I wrote are ugly in comparison :\

+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.

+       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?

+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

+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?

+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?

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

+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?

Unrelated.

+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