Re: [PATCH 17/17] revert: Introduce --continue to continue the operation

From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: Jul 17, 2011, 16:11 UTC
Message-ID: <CALkWK0==A1a-kzqXuEkjS+FMR1CAzJox8MBdkQGMLukoU15L9w@mail.gmail.com>
In-Reply-To: <20110712204650.GG14909@elie>

Hi Jonathan,

Jonathan Nieder writes:

Show 26 quoted lines

> Ramkumar Ramachandra wrote:
>> Using the information in ".git/sequencer", it is now possible to
>> continue a cherry-pick or continue after a conflict.  To do this, we
>> have to parse the information in ".git/sequencer/opts" into the
>> replay_opts structure and
> [...]
>
> Might be simpler to say:
>
>        Introduce a new "git cherry-pick --continue" command which uses
>        the information in ".git/sequencer" to continue a cherry-pick that
>        stopped because of a conflict or other error.  It works by dropping
>        the first instruction from .git/sequencer/todo and performing the
>        remaining cherry-picks listed there, with options (think "-s" and
>        "-X") from the initial command listed in .git/sequencer/opts.
>
>        So now you can do:
>
>                $ git cherry-pick -Xpatience foo..bar
>                ... description conflict in commit moo ...
>                $ git cherry-pick --continue
>                error: 'cherry-pick' is not possible because you have unmerged files.
>                fatal: failed to resume cherry-pick
>                $ echo resolved >conflictingfile
>                $ git add conflictingfile && git commit
>                $ git cherry-pick --continue; # resumes with the commit after "moo"

Yep, thanks for the pretty commit message :)

Show 16 quoted lines

>> During the "git commit" stage, CHERRY_PICK_HEAD will aid by providing
>> the commit message from the conflicting "moo" commit.  Note that the
>> cherry-pick mechanism has no control at this stage, so the user is
>> free to violate anything that was specified during the first
>> cherry-pick invocation.  For example, if "-x" was specified during the
>> first cherry-pick invocation, the user is free to edit out the message
>> during commit time.  One glitch to note is that the "--signoff" option
>> specified at cherry-pick invocation time is not reflected in the
>> commit message provided by CHERRY_PICK_HEAD; the user must take care
>> to add "--signoff" during the "git commit" invocation.
>
> The -s thing doesn't have much to do with this change.  But is it a
> bug or not?  If it's not a bug, then this is not so much a glitch to
> note as an important feature to ensure people don't sign off on a
> conflict resolution without thinking about it.  (I guess I think it's
> a bug.  It's hard to decide.)

Bug, definitely. It happens because unlike "-x" where the cherry-picking machinery appends to the commit message, "-s" is handled at commit-time (when it spawns `git commit`). Ofcourse, if I were never to write the sequencing features, this would never been seen as a bug -- hence the term "glitch"; an implementation detail that doesn't suit our future plans (namely, this series) very well.

Show 11 quoted lines

>> --- a/t/t3510-cherry-pick-sequence.sh
>> +++ b/t/t3510-cherry-pick-sequence.sh
>> @@ -101,4 +101,97 @@ test_expect_success '--reset cleans up sequencer state' '
> [...]
>> +test_expect_success '--continue complains when no cherry-pick is in progress' '
>> +     pristine_detach initial &&
>> +     test_must_fail git cherry-pick --continue >actual 2>&1 &&
>> +     test_i18ngrep "error" actual
>
> This would start to fail if the message ever changed from "error" to
> "fatal", right?  I don't think that's a good thing.

Okay, removed this and all similar checks.

Show 13 quoted lines

>> +test_expect_success '--continue complains when there are unresolved conflicts' '
>> +     pristine_detach initial &&
>> +     head=$(git rev-parse HEAD) &&
>> +     test_must_fail git cherry-pick base..picked &&
>> +     test_must_fail git cherry-pick --continue &&
>> +     git cherry-pick --reset
>> +'
>> +
>> +test_expect_success '--continue continues after conflicts are resolved' '
>> +     pristine_detach initial &&
>> +     head=$(git rev-parse HEAD) &&
>
> How is $head used?

Stray line -- copy-pasting from an older version on my own tests. Removed everywhere.

Show 13 quoted lines

>> +     test_must_fail git cherry-pick base..anotherpick &&
>> +     echo "c" >foo &&
>> +     git add foo &&
>> +     git commit &&
>> +     git cherry-pick --continue &&
>> +     test_path_is_missing .git/sequencer &&
>> +     {
>> +             git rev-list HEAD |
>> +             git diff-tree --root --stdin |
>> +             sed "s/[0-9a-f]\{40\}/OBJID/g"
>> +     } >actual &&
>
> $_x40 is idiomatic and safer with old seds.

I see. I actually thought '[0-9a-f]\{40\}' would be more traditional, and that's why I picked in the first place :p

Show 23 quoted lines

>> +test_expect_success '--continue respects opts' '
>> +     pristine_detach initial &&
>> +     head=$(git rev-parse HEAD) &&
>> +     test_must_fail git cherry-pick -s -x base..anotherpick &&
>> +     echo "c" >foo &&
>> +     git add foo &&
>> +     git commit -s &&
>> +     git cherry-pick --continue &&
>> +     test_path_is_missing .git/sequencer &&
>> +     git cat-file commit HEAD >anotherpick_msg &&
>> +     git cat-file commit HEAD~1 >picked_msg &&
>> +     git cat-file commit HEAD~2 >unrelatedpick_msg &&
>> +     git cat-file commit HEAD~3 >initial_msg &&
>> +     test_must_fail test_i18ngrep "Signed-off-by:" initial_msg &&
>
> This will break when GETTEXT_POISON is set --- test_i18ngrep
> automatically succeeds in that case.
>
> Is "Signed-off-by" meant to be translated anyway?  I would use
>
>        ! grep
>
> if testing that.

My bad.  Replaced with grep.  Thanks.

> By the way, that probably should go in a separate test assertion
> ("-s is not automatically propagated to resolved conflict") to make
> it easier to change the behavior later.

Separated into another test.  Thanks.

-- Ram