threads / patch / 30816

patchrebase -i -p: use rerere to resolve conflicts if enabled

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

## tl;dr

12 messages between Jun 15, 2012 and Jun 18, 2012. Diffs are folded; open one to read it.

replies: 11people: 3as markdown or json

David D. Kilzer· Jun 15, 2012, 14:17 UTC · lore
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
Show changes to 2 files +80 −2

git-rebase--interactive.sh, 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)
Junio C Hamano· Jun 15, 2012, 15:52 UTC · re: David D. Kilzer · lore

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

"David D. Kilzer" <ddkilzer@kilzer.net> writes:
Show 5 quoted lines
> 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.

David Kilzer· Jun 16, 2012, 04:08 UTC · re: Junio C Hamano · lore

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

On Jun 15, 2012, at 8:52 AM, Junio C Hamano wrote:
Show 21 quoted lines
> "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
Junio C Hamano· Jun 16, 2012, 05:19 UTC · re: David Kilzer · lore

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

David Kilzer <ddkilzer@kilzer.net> writes:
Show 9 quoted lines
>> 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?
David Kilzer· Jun 17, 2012, 03:58 UTC · re: Junio C Hamano · lore

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

On Jun 15, 2012, at 10:19 PM, Junio C Hamano wrote:
Show 8 quoted lines
> 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?
Show 14 quoted lines
> 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
Junio C Hamano· Jun 17, 2012, 05:49 UTC · re: David Kilzer · lore

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

David Kilzer <ddkilzer@kilzer.net> writes:
Show 10 quoted lines
> 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.

David Kilzer· Jun 17, 2012, 13:46 UTC · re: Junio C Hamano · lore

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

+ 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:
Show 21 quoted lines
> 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.
Show 7 quoted lines
> 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
Johannes Sixt· Jun 17, 2012, 18:30 UTC · re: David Kilzer · lore

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

Am 17.06.2012 15:46, schrieb David Kilzer:
Show 6 quoted lines
> 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
David Kilzer· Jun 17, 2012, 21:30 UTC · re: Johannes Sixt · lore

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

On Jun 17, 2012, at 11:30 AM, Johannes Sixt wrote:
Show 15 quoted lines
> 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
Johannes Sixt· Jun 18, 2012, 19:06 UTC · re: David Kilzer · lore

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

Am 17.06.2012 23:30, schrieb David Kilzer:
Show 28 quoted lines
> 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
Junio C Hamano· Jun 17, 2012, 22:00 UTC · re: David Kilzer · lore

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

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 ;-).

Junio C Hamano· Jun 17, 2012, 22:12 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 7 quoted lines
> 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.

← back to recent threads