[PATCH] Make cherry-pick use rerere for conflict resolution.

[PATCH] Make cherry-pick use rerere for conflict resolution.

[Abhijit Menon-Sen]

Trivial change plus test, as requested by Johannes Schindelin.

Signed-off-by: Abhijit Menon-Sen <ams@toroid.org>
---
 builtin-revert.c              |    2 +
 t/t3504-cherry-pick-rerere.sh |   45 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+), 0 deletions(-)
 create mode 100755 t/t3504-cherry-pick-rerere.sh

diff --git a/builtin-revert.c b/builtin-revert.c
index 27881e9..3667705 100644
--- a/builtin-revert.c
+++ b/builtin-revert.c
@@ -11,6 +11,7 @@
 #include "cache-tree.h"
 #include "diff.h"
 #include "revision.h"
+#include "rerere.h"
 
 /*
  * This implements the builtins revert and cherry-pick.
@@ -395,6 +396,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 			die ("Error wrapping up %s", defmsg);
 		fprintf(stderr, "Automatic %s failed.%s\n",
 			me, help_msg(commit->object.sha1));
+		rerere();
 		exit(1);
 	}
 	if (commit_lock_file(&msg_file) < 0)
diff --git a/t/t3504-cherry-pick-rerere.sh b/t/t3504-cherry-pick-rerere.sh
new file mode 100755
index 0000000..957b298
--- /dev/null
+++ b/t/t3504-cherry-pick-rerere.sh
@@ -0,0 +1,45 @@
+#!/bin/sh
+
+test_description='cherry-pick should rerere for conflicts'
+
+. ./test-lib.sh
+
+echo foo > foo
+git add foo && git commit -q -m 1
+
+echo foo-master > foo
+git add foo && git commit -q -m 2
+
+git checkout -b dev HEAD^
+
+echo foo-dev > foo
+git add foo && git commit -q -m 3
+
+git config rerere.enabled true
+
+test_expect_success 'conflicting merge' '
+	test_must_fail git merge master
+'
+
+echo foo-dev > foo
+git add foo && git commit -q -m 4
+
+git reset --hard HEAD^
+
+echo foo-dev > expect
+
+test_expect_success 'cherry-pick conflict' '
+	test_must_fail git cherry-pick master &&
+	test_cmp expect foo
+'
+
+git config rerere.enabled false
+
+git reset --hard
+
+test_expect_success 'cherry-pick conflict without rerere' '
+	test_must_fail git cherry-pick master &&
+	test_must_fail test_cmp expect foo
+'
+
+test_done
-- 
1.6.0.rc2

Re: [PATCH] Make cherry-pick use rerere for conflict resolution.

[Johannes Schindelin]

Hi,

On Sun, 10 Aug 2008, Abhijit Menon-Sen wrote:

> Trivial change plus test, as requested by Johannes Schindelin.

I would have preferred some convincing rationale for the change instead.

Ciao,
Dscho

[PATCH v2] Make cherry-pick use rerere for conflict resolution.

[Abhijit Menon-Sen]

It was a dark and stormy night. Sam struggled to keep his eyelids open
as he integrated yet another gigantic patch series. Ever the optimist,
he'd pulled in the changes, only to discover several merge conflicts.
But the night was young then, and he'd fixed them all by hand.

It was only later that he noticed many lousy, one-line commit messages.
Undaunted, he reset his branch and began to cherry-pick patches, giving
them a once-over, writing a comment here, squashing the odd grotesque
hack there, and writing sensible commit messages more often than not.

But even that was hours ago, and each new but oh-so-familiar conflict
ate into his determination like maggots through decaying meat; and Sam
was beginning to question the wisdom of staying in this fruit business.
His whiskey was running low, and time was running out.

"If only", thought Sam, "If only cherry-pick would..."

Signed-off-by: Abhijit Menon-Sen <ams@toroid.org>
---
 builtin-revert.c              |    2 +
 t/t3504-cherry-pick-rerere.sh |   45 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+), 0 deletions(-)
 create mode 100755 t/t3504-cherry-pick-rerere.sh

diff --git a/builtin-revert.c b/builtin-revert.c
index 27881e9..3667705 100644
--- a/builtin-revert.c
+++ b/builtin-revert.c
@@ -11,6 +11,7 @@
 #include "cache-tree.h"
 #include "diff.h"
 #include "revision.h"
+#include "rerere.h"
 
 /*
  * This implements the builtins revert and cherry-pick.
@@ -395,6 +396,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 			die ("Error wrapping up %s", defmsg);
 		fprintf(stderr, "Automatic %s failed.%s\n",
 			me, help_msg(commit->object.sha1));
+		rerere();
 		exit(1);
 	}
 	if (commit_lock_file(&msg_file) < 0)
diff --git a/t/t3504-cherry-pick-rerere.sh b/t/t3504-cherry-pick-rerere.sh
new file mode 100755
index 0000000..957b298
--- /dev/null
+++ b/t/t3504-cherry-pick-rerere.sh
@@ -0,0 +1,45 @@
+#!/bin/sh
+
+test_description='cherry-pick should rerere for conflicts'
+
+. ./test-lib.sh
+
+echo foo > foo
+git add foo && git commit -q -m 1
+
+echo foo-master > foo
+git add foo && git commit -q -m 2
+
+git checkout -b dev HEAD^
+
+echo foo-dev > foo
+git add foo && git commit -q -m 3
+
+git config rerere.enabled true
+
+test_expect_success 'conflicting merge' '
+	test_must_fail git merge master
+'
+
+echo foo-dev > foo
+git add foo && git commit -q -m 4
+
+git reset --hard HEAD^
+
+echo foo-dev > expect
+
+test_expect_success 'cherry-pick conflict' '
+	test_must_fail git cherry-pick master &&
+	test_cmp expect foo
+'
+
+git config rerere.enabled false
+
+git reset --hard
+
+test_expect_success 'cherry-pick conflict without rerere' '
+	test_must_fail git cherry-pick master &&
+	test_must_fail test_cmp expect foo
+'
+
+test_done
-- 
1.6.0.rc2

Re: [PATCH v2] Make cherry-pick use rerere for conflict resolution.

[Johannes Schindelin]

Hi,

On Mon, 11 Aug 2008, Abhijit Menon-Sen wrote:

> It was a dark and stormy night. Sam struggled to keep his eyelids open
> as he integrated yet another gigantic patch series. Ever the optimist,
> he'd pulled in the changes, only to discover several merge conflicts.
> But the night was young then, and he'd fixed them all by hand.
> 
> It was only later that he noticed many lousy, one-line commit messages.
> Undaunted, he reset his branch and began to cherry-pick patches, giving
> them a once-over, writing a comment here, squashing the odd grotesque
> hack there, and writing sensible commit messages more often than not.
> 
> But even that was hours ago, and each new but oh-so-familiar conflict
> ate into his determination like maggots through decaying meat; and Sam
> was beginning to question the wisdom of staying in this fruit business.
> His whiskey was running low, and time was running out.
> 
> "If only", thought Sam, "If only cherry-pick would..."

Nice try.

I have tried the whole dark and lonely night to find where in the git.git 
history we have some equally enlightening commit message.

So in essence, it is nice what you wrote, but not a commit message.  
Please imitate the style of existing commit messages, especially if you 
want to have your patch applied.

Ciao,
Dscho

Re: [PATCH v2] Make cherry-pick use rerere for conflict resolution.

[Petr Baudis]

  Hi,

  this makes revert use rerere too, right? Maybe use

	Make cherry-pick and revert call rerere for conflicts

instead?

  For janitors looking for a cleanup job, it would be nice to share this
code with suggest_conflicts() in the future.

On Mon, Aug 11, 2008 at 12:19:50PM +0200, Johannes Schindelin wrote:
> On Mon, 11 Aug 2008, Abhijit Menon-Sen wrote:
> 
> > It was a dark and stormy night. Sam struggled to keep his eyelids open
> > as he integrated yet another gigantic patch series. Ever the optimist,
> > he'd pulled in the changes, only to discover several merge conflicts.
> > But the night was young then, and he'd fixed them all by hand.
> > 
> > It was only later that he noticed many lousy, one-line commit messages.
> > Undaunted, he reset his branch and began to cherry-pick patches, giving
> > them a once-over, writing a comment here, squashing the odd grotesque
> > hack there, and writing sensible commit messages more often than not.
> > 
> > But even that was hours ago, and each new but oh-so-familiar conflict
> > ate into his determination like maggots through decaying meat; and Sam
> > was beginning to question the wisdom of staying in this fruit business.
> > His whiskey was running low, and time was running out.
> > 
> > "If only", thought Sam, "If only cherry-pick would..."
> 
> Nice try.
> 
> I have tried the whole dark and lonely night to find where in the git.git 
> history we have some equally enlightening commit message.
> 
> So in essence, it is nice what you wrote, but not a commit message.  
> Please imitate the style of existing commit messages, especially if you 
> want to have your patch applied.

  come on. :-)  I think it's harmless and amusing. If there was some
useful information lost because of this, that would be troublesome, but
what kind of "rationale" do you want here? The point seems obvious.

-- 
				Petr "Pasky" Baudis
The next generation of interesting software will be done
on the Macintosh, not the IBM PC.  -- Bill Gates

Re: [PATCH v2] Make cherry-pick use rerere for conflict resolution.

[Johannes Schindelin]

Hi,

On Mon, 11 Aug 2008, Petr Baudis wrote:

>   this makes revert use rerere too, right?

That would actually be a problem, no?  I am not sure that resolutions for 
reverts make sense for cherry-picks, so I am not sure if resolutions 
should be recorded for reverts.

Thanks for bringing up that point,
Dscho

Re: [PATCH v2] Make cherry-pick use rerere for conflict resolution.

[Johannes Sixt]

Johannes Schindelin schrieb:
> That would actually be a problem, no?  I am not sure that resolutions for 
> reverts make sense for cherry-picks, so I am not sure if resolutions 
> should be recorded for reverts.

Of course they should. If the reversal is part of a topic branch that you
rebase at least once, then you want to have the resolutions recorded,
don't you?

-- Hannes

Re: [PATCH v2] Make cherry-pick use rerere for conflict resolution.

[Johannes Schindelin]

Hi,

On Mon, 11 Aug 2008, Johannes Sixt wrote:

> Johannes Schindelin schrieb:
> > That would actually be a problem, no?  I am not sure that resolutions 
> > for reverts make sense for cherry-picks, so I am not sure if 
> > resolutions should be recorded for reverts.
> 
> Of course they should.

Are you sure?

> If the reversal is part of a topic branch that you rebase at least once, 
> then you want to have the resolutions recorded, don't you?

That is not the revert we are talking about.  The revert we are talking 
about is a literal "git revert <commit>".  Not a replay of a commit (that 
might have been a revert originally).

I am a little worried that these reverts (being negative changes) could 
interfer with the common operation: positive changes.  Although I haven't 
been able to come up with a scenario where the recorded revert would 
actively be wrong in a subsequent rebase/cherry-pick.

Yes, I see your point that a revert on a topic branch which is then 
rebased would be nice to have its resolution recorded; that will happen 
with the first rebase, though.

However, if my suspicion is true, recording the resolution only with the 
first rebase could make things safer overall, because an occasional 
temporary revert would not affect later cherry-picks in an unintuitive 
way.

Thinking about a good example, or a counterexample,
Dscho

Re: [PATCH v2] Make cherry-pick use rerere for conflict resolution.

[Johannes Sixt]

Johannes Schindelin schrieb:
> On Mon, 11 Aug 2008, Johannes Sixt wrote:
>> If the reversal is part of a topic branch that you rebase at least once, 
>> then you want to have the resolutions recorded, don't you?
> 
> That is not the revert we are talking about.  The revert we are talking 
> about is a literal "git revert <commit>".  Not a replay of a commit (that 
> might have been a revert originally).

You are right. My example misses the point.

Another example is when you have to repeat the revert, say, you find out
you did it on the wrong branch. When you repeat the 'git revert' on the
correct branch, you want to have the resolutions replayed.

> I am a little worried that these reverts (being negative changes) could 
> interfer with the common operation: positive changes.  Although I haven't 
> been able to come up with a scenario where the recorded revert would 
> actively be wrong in a subsequent rebase/cherry-pick.

I think that your worries are not justified.  A 'git revert' is not a
"negative" change; it a change like any other. 'git revert' is just a
short hand for a more sequence of diff+apply+commit.

-- Hannes

Re: [PATCH v2] Make cherry-pick use rerere for conflict resolution.

[Junio C Hamano]

Abhijit Menon-Sen <ams@toroid.org> writes:

> It was a dark and stormy night. Sam struggled to keep his eyelids open
> as he integrated yet another gigantic patch series. Ever the optimist,
> he'd pulled in the changes, only to discover several merge conflicts.
> But the night was young then, and he'd fixed them all by hand.
>
> It was only later that he noticed many lousy, one-line commit messages.
> Undaunted, he reset his branch and began to cherry-pick patches, giving
> them a once-over, writing a comment here, squashing the odd grotesque
> hack there, and writing sensible commit messages more often than not.
>
> But even that was hours ago, and each new but oh-so-familiar conflict
> ate into his determination like maggots through decaying meat; and Sam
> was beginning to question the wisdom of staying in this fruit business.
> His whiskey was running low, and time was running out.
>
> "If only", thought Sam, "If only cherry-pick would..."

That's cute, but I do not think that story is a good example.

By "pulled in the changes" do you mean "he merged somebody else's work"?
If so, the cherry-pick would be doing rebase of the series manually, and
as you already may know, you are not supposed to be rebasing other
people's work.  And if you are indeed rebasing, that would not be a good
example of cherry-pick, either.

Do you mean instead "he applied many patches, but there were conflicts and
he wiggled them in?"  If so, at the resolution time rerere() wouldn't have
recorded them in the first place, and more importantly, what you would be
cherry-picking won't have conflicts.  What the second paragraph describes
is what he would do with "git rebase -i" on top of the same base, so there
won't be merge conflicts, and even if there were, the use case is again
about rebase and not cherry-pick.

A better example would be if you have two (or more) maintenance tracks
from similarly old vintage and a far more advanced development track, and
cherry-picking from that development track to backport a fix down to one
of the maintenance track would have conflicts you need to fix.  Then you
would face the same conflict while propagating the same fix to another
maintenance track.  But even then, you would most likely cherry-pick the
cherry-picked fix from the maintenance track, which would be conflict
free, instead of cherry-picking it from the development track.

Re: [PATCH v2] Make cherry-pick use rerere for conflict resolution.

[Abhijit Menon-Sen]

At 2008-08-11 11:47:01 -0700, gitster@pobox.com wrote:
>
> I do not think that story is a good example.

I agree, it's a stretch.

I can't think of any better rationale for the change than "It might
conceivably be convenient to someone at some point", which falls a
fair bit short of being convincing.

To be honest, it took so little time to implement this suggestion that
I didn't realise until later that there was no realistic use-case and
nothing to say about the patch.

Sorry for the noise.

-- ams

Re: [PATCH v2] Make cherry-pick use rerere for conflict resolution.

[Junio C Hamano]

Abhijit Menon-Sen <ams@toroid.org> writes:

> At 2008-08-11 11:47:01 -0700, gitster@pobox.com wrote:
>>
>> I do not think that story is a good example.
>
> I agree, it's a stretch.
>
> I can't think of any better rationale for the change than "It might
> conceivably be convenient to someone at some point", which falls a
> fair bit short of being convincing.
>
> To be honest, it took so little time to implement this suggestion that
> I didn't realise until later that there was no realistic use-case and
> nothing to say about the patch.

Oh, that's Ok.  I think my "cherry-picking from devel to maint1 and then
cherry-picking the same change to maint2" example already shows the
potential usefulness of the patch.  Yes, cherry-picking the change from
maint1 would avoid conflicts, but we do not _have to_ force the user to
think about it.  If the user somehow chose to cherry-pick from devel to
maint2, it is certainly better if we allowed the earlier resolution
applied.

In any case, thanks for the patch --- queued in 'pu' for now.