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

Ramkumar Ramachandra wrote:

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

I have to confess that I don't remember the implementation you are
referring to.  Maybe I could have taken inspiration from it.

The rest of this message is about tests.

[...]

Jonathan Nieder wrote:

quoted

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.

To clarify, "conflicting" is sitting on a separate branch from the
rest of the commits.  This --help text uses "git show-branch"-style
output, which is perhaps out of fashion but compact and used by some
existing tests.

quoted

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

Nah, it's for the pristine_detach, not the test_commit.

I did miss an &&, though.  Not enough to justify rerolling on its own
but seems worth fixing if resending anyway.

quoted

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

Do you mean that you'd prefer this "conflicting" commit not to be
part of the setup shared between tests?

[...]

quoted

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

Yep, works fine.

[...]

1. I haven't used the "-s" flag of "git diff-tree" before, so I opened
up the documentation to find this:

Yeah, that documentation sucks.  I'll keep this message marked as a
reminder to look at it.

Just like "git show" is the porcelain command to show a commit, "git
diff-tree" is the corresponding plumbing.

[...]

quoted

+test_expect_success '--continue after resolving conflicts' '

[...]

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

It's just a new test.  If rerolling, I'll make it imitate the style of
the existing test following it better.

[...]

quoted

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

This test is about --continue asking for help instead of succeeding or
failing in some uncontrolled way, so it seemed useful to check that
the message actually pertains to that.

Remind me what this test has to do
with the rest of your patch?

With this change in how --continue works, I wanted to make sure the
semantics that were not supposed to be changed were still intact.

[...]

quoted

+test_expect_failure 'follow advice and skip nil patch' '

[...]

Again, what does this test have to do with the rest of your patch?

Likewise.

[...]

quoted

+test_expect_success '--continue of single-pick respects -x' '

[...]

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

Maybe the previous one should say "respects -x".

I am not sure what it would mean for --continue of a single-pick to
respect --strategy, for example.

quoted

+test_expect_success '--continue respects -x in first commit in multi-pick' '

[...]

Can you explain why "first commit in a multi-pick" is a special case?

I guess you mean "how does this differ from the existing '--continue
respects opts' test?".

Good question.  In the existing "--continue respects opts" test, we
explicitly run "git commit" before "git cherry-pick --continue".  This
test checks that running "git cherry-pick --continue" without
commiting first does not cause the commit message to be clobbered.

[...]

quoted

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

[...]

Unrelated.

[...]

quoted

+test_expect_success 'sign-off needs to be reaffirmed after conflict resolution, single-pick case' '

There was no implicit commit of resolution before this patch, so how
can it be unrelated?

[...]

Thanks for working on this.

Thanks for your attention to detail.

Sincerely,
Jonathan