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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html