threads / patch / 14913

patchMake cherry-pick use rerere for conflict resolution.

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

## tl;dr

12 messages between Aug 10, 2008 and Aug 12, 2008. Diffs are folded; open one to read it.

replies: 11people: 5as markdown or json

Abhijit Menon-Sen· Aug 10, 2008, 11:48 UTC · lore
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
Show changes to 2 files +47 −0

builtin-revert.c, 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
Johannes Schindelin· Aug 10, 2008, 23:12 UTC · re: Abhijit Menon-Sen · lore

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

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

Abhijit Menon-Sen· Aug 11, 2008, 02:30 UTC · re: Johannes Schindelin · lore

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

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
Show changes to 2 files +47 −0

builtin-revert.c, 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
Johannes Schindelin· Aug 11, 2008, 10:19 UTC · re: Abhijit Menon-Sen · lore

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

Hi,
On Mon, 11 Aug 2008, Abhijit Menon-Sen wrote:
Show 16 quoted lines
> 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

Petr Baudis· Aug 11, 2008, 10:40 UTC · re: Johannes Schindelin · lore

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

  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:
Show 27 quoted lines
> 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
Johannes Schindelin· Aug 11, 2008, 11:32 UTC · re: Petr Baudis · lore

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

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

Johannes Sixt· Aug 11, 2008, 11:49 UTC · re: Johannes Schindelin · lore

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

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
Johannes Schindelin· Aug 11, 2008, 15:54 UTC · re: Johannes Sixt · lore

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

Hi,
On Mon, 11 Aug 2008, Johannes Sixt wrote:
Show 6 quoted lines
> 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

Johannes Sixt· Aug 12, 2008, 07:02 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin schrieb:
Show 7 quoted lines
> 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
Junio C Hamano· Aug 11, 2008, 18:47 UTC · re: Abhijit Menon-Sen · lore

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

Abhijit Menon-Sen <ams@toroid.org> writes:
Show 16 quoted lines
> 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.

Abhijit Menon-Sen· Aug 12, 2008, 02:34 UTC · re: Junio C Hamano · lore

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

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
Junio C Hamano· Aug 12, 2008, 06:59 UTC · re: Abhijit Menon-Sen · lore

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

Abhijit Menon-Sen <ams@toroid.org> writes:
Show 13 quoted lines
> 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.

← back to recent threads