[PATCH] rebase -i -p: use rerere to resolve conflicts if enabled

[PATCH] rebase -i -p: use rerere to resolve conflicts if enabled

[David D. Kilzer]

From: "David D. Kilzer" <ddkilzer@kilzer.net>

When performing an interactive rebase that preserves merges with
rerere enabled, the --rerere-autoupdate switch should be passed
to git-merge.

Signed-off-by: David D. Kilzer <ddkilzer@kilzer.net>
---
 git-rebase--interactive.sh                    |    7 ++-
 t/t3420-rebase-preserve-merges-with-rerere.sh |   75 +++++++++++++++++++++++++
 2 files changed, 80 insertions(+), 2 deletions(-)
 create mode 100755 t/t3420-rebase-preserve-merges-with-rerere.sh

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 2e13258..958bbf8 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -297,9 +297,12 @@ pick_one_preserving_merges () {
 			msg_content="$(commit_message $sha1)"
 			# No point in merging the first parent, that's HEAD
 			new_parents=${new_parents# $first_parent}
+			# If rerere is enabled, pass the --rerere-autoupdate flag
+			test "$(git config --bool rerere.enabled)" = "true" &&
+				rerere_autoupdate=--rerere-autoupdate || rerere_autoupdate=
 			if ! do_with_author output \
-				git merge --no-ff ${strategy:+-s $strategy} -m \
-					"$msg_content" $new_parents
+				git merge --no-ff ${strategy:+-s $strategy} $rerere_autoupdate \
+					-m "$msg_content" $new_parents
 			then
 				printf "%s\n" "$msg_content" > "$GIT_DIR"/MERGE_MSG
 				die_with_patch $sha1 "Error redoing merge $sha1"
diff --git a/t/t3420-rebase-preserve-merges-with-rerere.sh b/t/t3420-rebase-preserve-merges-with-rerere.sh
new file mode 100755
index 0000000..679937d
--- /dev/null
+++ b/t/t3420-rebase-preserve-merges-with-rerere.sh
@@ -0,0 +1,75 @@
+#!/bin/sh
+#
+# Copyright (c) 2007 Johannes E. Schindelin
+# Copyright (c) 2012 David D. Kilzer
+#
+
+test_description='git rebase -i -p should use rerere to resolve conflicts if enabled'
+. ./test-lib.sh
+
+. "$TEST_DIRECTORY"/lib-rebase.sh
+
+set_fake_editor
+
+# Setup
+#
+# A--AA--B    <-- master
+#     \
+#      \
+#       \
+#        C    <-- topic1
+
+test_expect_success 'setup' '
+	test_commit A file1 &&
+	test_commit AA file2 &&
+	test_commit B file1 &&
+	git checkout -b topic1 HEAD^ &&
+	test_commit C file1 &&
+	git checkout master
+'
+
+# Use rerere to resolve conflicts
+#
+# Before interactive rebase:
+#
+# A--AA--B    <-- master
+#     \   \
+#      \   M  <-- merge1-baseline, merge1
+#       \ /
+#        C    <-- topic1
+#
+# After interactive rebase:
+#
+# A--AA--B    <-- master
+#    |\   \
+#    | \   M  <-- merge1-baseline
+#    |  \ /
+#    |   C    <-- topic1
+#     \   \
+#      \   M' <-- merge1
+#       \ /
+#        B'
+
+test_expect_success 'rebase -i -p uses rerere to resolve conflicts' '
+	git config rerere.enabled true &&
+	git rerere clear &&
+
+	git checkout -b merge1 master &&
+	test_must_fail git merge topic1 &&
+	test "`git rerere status`" = "file1" &&
+	printf "B\nC\n" > file1 &&
+	git add file1 &&
+	git commit -m "M: Merge with conflict resolved." &&
+	git branch merge1-baseline &&
+
+	FAKE_LINES="edit 1 2 3" git rebase -i -p HEAD~2 &&
+	echo BB >> file2 &&
+	git add file2 &&
+	git commit -m "B'\'': Edit file2 to prevent fast-forward." --amend &&
+	test_must_fail git rebase --continue &&
+	git commit -m "M'\'': Merge with conflict resolved by rerere." &&
+	git rebase --continue &&
+	git diff --exit-code merge1-baseline..merge1 file1
+'
+
+test_done
-- 
1.7.9.6 (Apple Git-31)

Re: [PATCH] rebase -i -p: use rerere to resolve conflicts if enabled

[Junio C Hamano]

"David D. Kilzer" <ddkilzer@kilzer.net> writes:

> From: "David D. Kilzer" <ddkilzer@kilzer.net>
>
> When performing an interactive rebase that preserves merges with
> rerere enabled, the --rerere-autoupdate switch should be passed
> to git-merge.

I do not understand the above reasoning.

"rerere" is enabled in "merge" used in this codepath already, so
after it runs, you will see the result of automatically replaying
a previous resolution without your patch.

The configuration rerere.enabled *never* meant that the user blindly
trusts the result of replaying a previous resolution.  If you were
checking rerere.autoupdate configuration variable, the patch may
have made some sense, but basing the decision on rerere.enabled
(which by the way is not necessary to trigger the rerere machinery
these days, as long as $GIT_DIR/rr-cache/ directory exists) sounds
very wrong.

Re: [PATCH] rebase -i -p: use rerere to resolve conflicts if enabled

[David Kilzer]

On Jun 15, 2012, at 8:52 AM, Junio C Hamano wrote:

> "David D. Kilzer" <ddkilzer@kilzer.net> writes:
> 
>> From: "David D. Kilzer" <ddkilzer@kilzer.net>
>> 
>> When performing an interactive rebase that preserves merges with
>> rerere enabled, the --rerere-autoupdate switch should be passed
>> to git-merge.
> 
> I do not understand the above reasoning.
> 
> "rerere" is enabled in "merge" used in this codepath already, so
> after it runs, you will see the result of automatically replaying
> a previous resolution without your patch.
> 
> The configuration rerere.enabled *never* meant that the user blindly
> trusts the result of replaying a previous resolution.  If you were
> checking rerere.autoupdate configuration variable, the patch may
> have made some sense, but basing the decision on rerere.enabled
> (which by the way is not necessary to trigger the rerere machinery
> these days, as long as $GIT_DIR/rr-cache/ directory exists) sounds
> very wrong.

Thanks!  I'll repost the patch based on rerere.autoupdate for further discussion.

Dave

Re: [PATCH] rebase -i -p: use rerere to resolve conflicts if enabled

[Junio C Hamano]

David Kilzer <ddkilzer@kilzer.net> writes:

>> The configuration rerere.enabled *never* meant that the user blindly
>> trusts the result of replaying a previous resolution.  If you were
>> checking rerere.autoupdate configuration variable, the patch may
>> have made some sense, but basing the decision on rerere.enabled
>> (which by the way is not necessary to trigger the rerere machinery
>> these days, as long as $GIT_DIR/rr-cache/ directory exists) sounds
>> very wrong.
>
> Thanks!  I'll repost the patch based on rerere.autoupdate for further discussion.

I do not use the configuration variable myself, and I didn't check
the code, but if you had rerere.autoupdate set, doesn't "git merge"
in the codepath you are touching (or anywhere for that matter)
already blindly take the replayed resolution and commit the result?

In other words, do you need to do anything special to make the
command honour rerere.autoupdate?

Assuming that your patch does not need to do anything special based
on the rerere.autoupdate configuration (because the underlying
"merge" may automatically take care of it), I think what you need
may be a mechanism to give --[no-]rerere-autoupdate option to "git
rebase -m/-i/-p" and pass that option to the invocation of
underlying "git merge", so that the user who does not usually want
to blindly trust the replayed resolution (hence rerere.autoupdate
configured to false) can choose to tell the "git rebase -m/-i/-p"
command that "for this single invocation it is OK to trust the
replayed resolution".  Or the other way around, i.e. "Even though I
have rerere.autoupdate configured to true, for this single
invocation of 'rebase', I am giving the '--no-rerere-autoupdate'
option to tell you that you should _not_ blindly replay the
resolution."

Hrm?

Re: [PATCH] rebase -i -p: use rerere to resolve conflicts if enabled

[David Kilzer]

On Jun 15, 2012, at 10:19 PM, Junio C Hamano wrote:

> David Kilzer <ddkilzer@kilzer.net> writes:
> 
>> Thanks!  I'll repost the patch based on rerere.autoupdate for further discussion.
> 
> I do not use the configuration variable myself, and I didn't check
> the code, but if you had rerere.autoupdate set, doesn't "git merge"
> in the codepath you are touching (or anywhere for that matter)
> already blindly take the replayed resolution and commit the result?

No, it does not.  That is what I originally expected to happen when I used "git rebase -i -p" through a merge with conflicts, but it currently does not behave this way.

> In other words, do you need to do anything special to make the
> command honour rerere.autoupdate?

Yes, there are two changes required to make it behave this way, both in git-rebase--interactive.sh in the same locality:

1. Pass --rerere-autoupdate to git-merge if rerere.autoupdate is true.
2. Run git-update-index (before dying) to determine if all conflicts were successfully resolved and commit the result if so, else die as before.

There is one big caveat to #2, though.  If the original (pre-rebase) merge commit contained changes to a non-conflicted file, those changes will be lost if all of the conflicted files are auto-updated using rerere.

This is actually a real concern in a particular git repository that I maintain where I'm merging individual commits from two different git-svn repositories into a third pure-git tree.  (One svn tree is essentially a branch of the other.)  I merge individual commits from both git-svn trees to provide the highest fidelity for (potential future) git-bisect operations.  When I hit a build failure after ~100 commits, I determine how best to fix it, then run "git rebase -i -p" to inject the fix in the proper merge commit.  Occasionally I catch a build failure when resolving a conflict, which may also cause me to change a non-conflicted file.

I now have a patch series for #1 and #2 (including a failing test that provides an example of a change to a non-conflicted file getting lost during "rebase -i -p").  Would it be helpful to post this patch series?

> Assuming that your patch does not need to do anything special based
> on the rerere.autoupdate configuration (because the underlying
> "merge" may automatically take care of it), I think what you need
> may be a mechanism to give --[no-]rerere-autoupdate option to "git
> rebase -m/-i/-p" and pass that option to the invocation of
> underlying "git merge", so that the user who does not usually want
> to blindly trust the replayed resolution (hence rerere.autoupdate
> configured to false) can choose to tell the "git rebase -m/-i/-p"
> command that "for this single invocation it is OK to trust the
> replayed resolution".  Or the other way around, i.e. "Even though I
> have rerere.autoupdate configured to true, for this single
> invocation of 'rebase', I am giving the '--no-rerere-autoupdate'
> option to tell you that you should _not_ blindly replay the
> resolution."


Yes, that sounds reasonable.  What would be the best way to store this rebase-only switch?

Does git-config have a per-rebase-operation mode where config options can be read/written for the duration of a specific rebase operation such that these config settings override all the other config files?  That has the potential to provide a better separation of concerns rather than creating yet another one-off file in .git/rebase-*/.  (May want to add an extra flag to git-config like --check-rebase or --rebase to make it check for .git/rebase-*/config before .git/config since that probably shouldn't be the default behavior when the user invokes git-config.)

Or would it be best just to touch an empty file in .git/rebase-*/ for this purpose?

Dave

Re: [PATCH] rebase -i -p: use rerere to resolve conflicts if enabled

[Junio C Hamano]

David Kilzer <ddkilzer@kilzer.net> writes:

> On Jun 15, 2012, at 10:19 PM, Junio C Hamano wrote:
>
>> I do not use the configuration variable myself, and I didn't check
>> the code, but if you had rerere.autoupdate set, doesn't "git merge"
>> in the codepath you are touching (or anywhere for that matter)
>> already blindly take the replayed resolution and commit the result?
>
> No, it does not.  That is what I originally expected to happen
> when I used "git rebase -i -p" through a merge with conflicts, but
> it currently does not behave this way.

After looking at what is done in t/t4200-rerere.sh, I think "git
merge" (or anything that use --rerere-autoupdate, that is) is meant
to exit with an error code after allowing rerere to add the result
of replayed resolution to the index, so that the user can deal with
any remaining paths that may be still in conflict.

Are you sure that the autoresolved paths are not "git add"ed when
you have rerere.autoupdate set by "git merge" in "git rebase -i/-p"?

Or are you only talking about the error exit from "git merge" that
would cause "git rebase -i" to stop and give control back to the end
user?

I suspect that the latter behaviour to stop "rebase" in the middle
is in line with the spirit of --rerere-autoupdate, and it is not
likely that we would want to change it.

Re: [PATCH] rebase -i -p: use rerere to resolve conflicts if enabled

[David Kilzer]

+ Johannes Schindelin  [sorry, should have added you at the beginning of the thread]

On Jun 16, 2012, at 10:49 PM, Junio C Hamano wrote:

> David Kilzer <ddkilzer@kilzer.net> writes:
> 
>> On Jun 15, 2012, at 10:19 PM, Junio C Hamano wrote:
>> 
>>> I do not use the configuration variable myself, and I didn't check
>>> the code, but if you had rerere.autoupdate set, doesn't "git merge"
>>> in the codepath you are touching (or anywhere for that matter)
>>> already blindly take the replayed resolution and commit the result?
>> 
>> No, it does not.  That is what I originally expected to happen
>> when I used "git rebase -i -p" through a merge with conflicts, but
>> it currently does not behave this way.
> 
> After looking at what is done in t/t4200-rerere.sh, I think "git
> merge" (or anything that use --rerere-autoupdate, that is) is meant
> to exit with an error code after allowing rerere to add the result
> of replayed resolution to the index, so that the user can deal with
> any remaining paths that may be still in conflict.
> 
> Are you sure that the autoresolved paths are not "git add"ed when
> you have rerere.autoupdate set by "git merge" in "git rebase -i/-p"?

You are correct, autoresolved paths are "git add"ed when rerere.autoupdate is true.

Argh...in my original patch, I wasn't setting rerere.autoupdate.  After fixing that in the test, it's clear that the patch is no longer needed.

> Or are you only talking about the error exit from "git merge" that
> would cause "git rebase -i" to stop and give control back to the end
> user?
> 
> I suspect that the latter behaviour to stop "rebase" in the middle
> is in line with the spirit of --rerere-autoupdate, and it is not
> likely that we would want to change it.

If it could be guaranteed that all changes in a merge commit would be preserved when running "git rebase -i -p" with rerere.autoupdate enabled, I think that would be an argument for not returning control to the user during the rebase operation.  However, changes to non-conflicted files in a merge commit are currently lost in this case, so it would be too dangerous to enable this behavior now.

Dave

Re: [PATCH] rebase -i -p: use rerere to resolve conflicts if enabled

[Johannes Sixt]

Am 17.06.2012 15:46, schrieb David Kilzer:
> If it could be guaranteed that all changes in a merge commit would be
> preserved when running "git rebase -i -p" with rerere.autoupdate
> enabled, I think that would be an argument for not returning control
> to the user during the rebase operation.  However, changes to
> non-conflicted files in a merge commit are currently lost in this
> case, so it would be too dangerous to enable this behavior now.

You can test this patch:

  git://repo.or.cz/git/mingw/j6t.git preserve-merges-by-cherry-pick

I think it suits you needs unless you run into the one use-case where
the patch is a regression (as documented by the new test_expect_failure
in the test suite).

-- Hannes

Re: [PATCH] rebase -i -p: use rerere to resolve conflicts if enabled

[David Kilzer]

On Jun 17, 2012, at 11:30 AM, Johannes Sixt wrote:

> Am 17.06.2012 15:46, schrieb David Kilzer:
>> If it could be guaranteed that all changes in a merge commit would be
>> preserved when running "git rebase -i -p" with rerere.autoupdate
>> enabled, I think that would be an argument for not returning control
>> to the user during the rebase operation.  However, changes to
>> non-conflicted files in a merge commit are currently lost in this
>> case, so it would be too dangerous to enable this behavior now.
> 
> You can test this patch:
> 
>  git://repo.or.cz/git/mingw/j6t.git preserve-merges-by-cherry-pick
> 
> I think it suits you needs unless you run into the one use-case where
> the patch is a regression (as documented by the new test_expect_failure
> in the test suite).


I'd love to try it, but I'm running into an issue pulling the repository:

$ git clone git://repo.or.cz/git/mingw/j6t.git -b preserve-merges-by-cherry-pick j6t.git
Cloning into 'j6t.git'...
remote: error: Could not read 942cf39b9a36ae27a4377d22093827ef4df25239
remote: fatal: Failed to traverse parents of commit 051ba02462dd65a0ceb3e527a75f24416378880f
remote: aborting due to possible repository corruption on the remote side.
fatal: early EOF
fatal: index-pack failed

$ git --version
git version 1.7.9.6 (Apple Git-31)

Dave

Re: [PATCH] rebase -i -p: use rerere to resolve conflicts if enabled

[Junio C Hamano]

David Kilzer <ddkilzer@kilzer.net> writes:

> + Johannes Schindelin  [sorry, should have added you at the beginning of the thread]

Side note: Dscho, I do not mind hearing from you from time to time,
but if the only reason David summoned you is because I mentioned
t4200 and your name appears at the beginning of that file, and
unless you are interested in rerere.autoupdate yourself, I am fine
if you to treat this thread as low priority.  Your code in t4200
does not have much to do with rerere.autoupdate which this
discussion thread is about.

I vaguely recall doing the 5-patch series that ends with 121c813
(rerere.autoupdate, 2008-06-22) after somebody asked if there is a
way to tell paths that have been resolved by rerere already and
paths that still need to be sorted out manually (back then I think
we had "rerere status" but not "rerere remaining"), so that "git
ls-files -u" can be a more useful command to find out which paths
need further work, but I do not seem to be able to find the thread.
I also think that somebody was a regular in the kernel mailing list,
but I do not remember the details.  Hopefully somebody with better
research skills (or better memory) than I have can help digging the
context up for us ;-).

Re: [PATCH] rebase -i -p: use rerere to resolve conflicts if enabled

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Or are you only talking about the error exit from "git merge" that
> would cause "git rebase -i" to stop and give control back to the end
> user?
>
> I suspect that the latter behaviour to stop "rebase" in the middle
> is in line with the spirit of --rerere-autoupdate, and it is not
> likely that we would want to change it.

See this for the background:

  http://thread.gmane.org/gmane.comp.version-control.git/85176/focus=85231

The entire thread (except for articles from a few uninformed
noisemakers) is worth a read, as some issues that were discussed
have resulted in more recent "rerere", and the original inquiry by
Ingo illustrates the motivation behind them quite clearly.

And we can see that I was right when I wrote in the above that not
committing is the right thing to do for --rerere-autoupdate /
rerere.autoupdate even though I did not remember that thread.  The
second step described as "the way forward" in the quoted article
hasn't happened yet.

Re: [PATCH] rebase -i -p: use rerere to resolve conflicts if enabled

[Johannes Sixt]

Am 17.06.2012 23:30, schrieb David Kilzer:
> On Jun 17, 2012, at 11:30 AM, Johannes Sixt wrote:
> 
>> Am 17.06.2012 15:46, schrieb David Kilzer:
>>> If it could be guaranteed that all changes in a merge commit would be
>>> preserved when running "git rebase -i -p" with rerere.autoupdate
>>> enabled, I think that would be an argument for not returning control
>>> to the user during the rebase operation.  However, changes to
>>> non-conflicted files in a merge commit are currently lost in this
>>> case, so it would be too dangerous to enable this behavior now.
>>
>> You can test this patch:
>>
>>  git://repo.or.cz/git/mingw/j6t.git preserve-merges-by-cherry-pick
>>
>> I think it suits you needs unless you run into the one use-case where
>> the patch is a regression (as documented by the new test_expect_failure
>> in the test suite).
> 
> 
> I'd love to try it, but I'm running into an issue pulling the repository:
> 
> $ git clone git://repo.or.cz/git/mingw/j6t.git -b preserve-merges-by-cherry-pick j6t.git
> Cloning into 'j6t.git'...
> remote: error: Could not read 942cf39b9a36ae27a4377d22093827ef4df25239
> remote: fatal: Failed to traverse parents of commit 051ba02462dd65a0ceb3e527a75f24416378880f
> remote: aborting due to possible repository corruption on the remote side.
> fatal: early EOF
> fatal: index-pack failed

Don't do that then ;) Enter your favorite git.git clone, then

  git pull git://repo.or.cz/git/mingw/j6t.git preserve-merges-by-cherry-pick

should work like a charm. But I've now removed the branches that
needed the missing commit, and your command should work as well.

-- Hannes