From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: Wed, 14 Dec 2011 14:26:20 GMT
Subject: Re: [PATCH 2/7] revert: allow cherry-pick --continue to commit before resuming
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.

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

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

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

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

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

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

> @@ -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 :\

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

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

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

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

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