threads / patch / 26442

patchmergetool: don't skip modify/remove conflicts

Subject: [PATCH] mergetool: don't skip modify/remove conflicts

## tl;dr

13 messages between Feb 8, 2011 and Feb 16, 2011. Diffs are folded; open one to read it.

replies: 12people: 2as markdown or json

Martin von Zweigbergk· Feb 8, 2011, 03:08 UTC · lore

Since bb0a484 (mergetool: Skip autoresolved paths, 2010-08-17), mergetool uses different ways of figuring out the list of files with merge conflicts depending on whether rerere is active. If rerere is active, mergetool will use 'git rerere status' to list the files with remaining conflicts. However, the output from that command does not list conflicts of types that rerere does not handle, such as modify/remove conflicts.

Another problem with solely relying on the output from 'git rerere status' is that, for new conflicts that are not yet known to rerere, the output from the command will list the files even after adding them to the index. This means that if the conflicts in some files have been resolved and 'git mergetool' is run again, it will ask the user something like the following for each of those files.

 file1: file does not need merging
 Continue merging other unresolved paths (y/n) ?

Solve the first of these problems by replacing the call to 'git rerere status' with a call to the new 'git rerere remaining' that was introduced in the previous commit. Solve the second problem by modifying 'git rerere remaining' not to output files that have already been staged.

Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
---
This applies on top of jc/rerere-remaining.

This is my first patch that touches any C code and I haven't written anything in C for about 10 years. I hope it doesn't look too bad.

I also have another version of this patch that doesn't modify 'git rerere remaining', but instead combines the output with the output from 'git ls-files'. Tell me if you think that is better.

Is it correct to call "yes" with multiple arguments in the test script the way I did?

 builtin/rerere.c     |   10 +++++++---
 git-mergetool.sh     |    2 +-
 rerere.c             |   31 ++++++++++++++++++++-----------
 rerere.h             |    3 +++
 t/t7610-mergetool.sh |   40 ++++++++++++++++++++++++++++++++++------
 5 files changed, 65 insertions(+), 21 deletions(-)
Show changes to 5 files +65 −21

builtin/rerere.c, git-mergetool.sh, rerere.c, rerere.h, t/t7610-mergetool.sh

diff --git a/builtin/rerere.c b/builtin/rerere.c
index 081fccc..9cbab27 100644
--- a/builtin/rerere.c
+++ b/builtin/rerere.c
@@ -147,7 +147,7 @@ int cmd_rerere(int argc, const char **argv, const char *prefix)
 	if (!strcmp(argv[0], "clear")) {
 		for (i = 0; i < merge_rr.nr; i++) {
 			const char *name = (const char *)merge_rr.items[i].util;
-			if (!name)
+			if (name == RERERE_UTIL_PUNTED || name == RERERE_UTIL_STAGED)
 				continue;
 			if (!has_rerere_resolution(name))
 				unlink_rr_item(name);
@@ -162,13 +162,17 @@ int cmd_rerere(int argc, const char **argv, const char *prefix)
 			printf("%s\n", merge_rr.items[i].string);
 		}
 	else if (!strcmp(argv[0], "remaining"))
-		for (i = 0; i < merge_rr.nr; i++)
+		for (i = 0; i < merge_rr.nr; i++) {
+			const char *name = (const char *)merge_rr.items[i].util;
+			if (name == RERERE_UTIL_STAGED)
+				continue;
 			printf("%s\n", merge_rr.items[i].string);
+		}
 	else if (!strcmp(argv[0], "diff"))
 		for (i = 0; i < merge_rr.nr; i++) {
 			const char *path = merge_rr.items[i].string;
 			const char *name = (const char *)merge_rr.items[i].util;
-			if (!name)
+			if (name == RERERE_UTIL_PUNTED || name == RERERE_UTIL_STAGED)
 				continue;
 			diff_two(rerere_path(name, "preimage"), path, path, path);
 		}
diff --git a/git-mergetool.sh b/git-mergetool.sh
index 2f8dc44..bacbda2 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -269,7 +269,7 @@ rerere=false
 files_to_merge() {
     if test "$rerere" = true
     then
-	git rerere status
+	git rerere remaining
     else
 	git ls-files -u | sed -e 's/^[^	]*	//' | sort -u
     fi
diff --git a/rerere.c b/rerere.c
index eb47f97..61cac54 100644
--- a/rerere.c
+++ b/rerere.c
@@ -7,6 +7,10 @@
 #include "ll-merge.h"
 #include "attr.h"
 
+#define RERERE_UTIL_ELIGIBLE NULL
+void *RERERE_UTIL_PUNTED = &RERERE_UTIL_PUNTED;
+void *RERERE_UTIL_STAGED = &RERERE_UTIL_STAGED;
+
 /* if rerere_enabled == -1, fall back to detection of .git/rr-cache */
 static int rerere_enabled = -1;
 
@@ -352,18 +356,20 @@ static int find_conflict(struct string_list *conflict)
 		return error("Could not read index");
 
 	/*
-	 * Collect paths with conflict, mark them with NULL (punted) or
-	 * !NULL (eligible) in their ->util field.
+	 * Collect paths with conflict, mark them according to type in
+	 * their ->util field.
 	 */
 	for (i = 0; i < active_nr; i++) {
 		struct cache_entry *e = active_cache[i];
 		struct string_list_item *it;
 
-		if (!ce_stage(e))
+		if (!ce_stage(e)) {
 			continue;
+		}
 		it = string_list_insert(conflict, (const char *)e->name);
-		it->util = NULL;
+		it->util = RERERE_UTIL_PUNTED;
 		if (ce_stage(e) == 1) {
+			it->util = RERERE_UTIL_STAGED;
 			if (active_nr <= ++i)
 				break;
 		}
@@ -377,7 +383,7 @@ static int find_conflict(struct string_list *conflict)
 			    ce_same_name(e, e3) &&
 			    S_ISREG(e2->ce_mode) &&
 			    S_ISREG(e3->ce_mode))
-				it->util = (void *) 1;
+				it->util = RERERE_UTIL_ELIGIBLE;
 		}
 
 		/* Skip the entries with the same name */
@@ -395,9 +401,10 @@ static void add_punted(struct string_list *merge_rr)
 
 	find_conflict(&conflict);
 	for (i = 0; i < conflict.nr; i++) {
-		if (conflict.items[i].util)
+		if (conflict.items[i].util == RERERE_UTIL_ELIGIBLE)
 			continue;
-		string_list_insert(merge_rr, conflict.items[i].string);
+		string_list_insert(merge_rr, conflict.items[i].string)->util =
+			conflict.items[i].util;
 	}
 }
 
@@ -487,8 +494,9 @@ static int do_plain_rerere(struct string_list *rr, int fd)
 
 	for (i = 0; i < conflict.nr; i++) {
 		const char *path = conflict.items[i].string;
-		if (!conflict.items[i].util)
-			continue; /* punted */
+		if (conflict.items[i].util == RERERE_UTIL_PUNTED ||
+			conflict.items[i].util == RERERE_UTIL_STAGED)
+			continue;
 		if (!string_list_has_string(rr, path)) {
 			unsigned char sha1[20];
 			char *hex;
@@ -648,8 +656,9 @@ int rerere_forget(const char **pathspec)
 	find_conflict(&conflict);
 	for (i = 0; i < conflict.nr; i++) {
 		struct string_list_item *it = &conflict.items[i];
-		if (!conflict.items[i].util)
-			continue; /* punted */
+		if (conflict.items[i].util == RERERE_UTIL_PUNTED ||
+			conflict.items[i].util == RERERE_UTIL_STAGED)
+			continue;
 		if (!match_pathspec(pathspec, it->string, strlen(it->string),
 				    0, NULL))
 			continue;
diff --git a/rerere.h b/rerere.h
index eaa9004..107a2bc 100644
--- a/rerere.h
+++ b/rerere.h
@@ -6,6 +6,9 @@
 #define RERERE_AUTOUPDATE   01
 #define RERERE_NOAUTOUPDATE 02
 
+extern void *RERERE_UTIL_PUNTED;
+extern void *RERERE_UTIL_STAGED;
+
 extern int setup_rerere(struct string_list *, int);
 extern int rerere(int);
 extern const char *rerere_path(const char *hex, const char *file);
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index d78bdec..dc838c9 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -16,23 +16,33 @@ Testing basic merge tool invocation'
 test_expect_success 'setup' '
     git config rerere.enabled true &&
     echo master >file1 &&
+    echo master file11 >file11 &&
+    echo master file12 >file12 &&
+    echo master file13 >file13 &&
+    echo master file14 >file14 &&
     mkdir subdir &&
     echo master sub >subdir/file3 &&
-    git add file1 subdir/file3 &&
-    git commit -m "added file1" &&
+    git add file1 file1[1-4] subdir/file3 &&
+    git commit -m "add initial versions" &&
 
     git checkout -b branch1 master &&
     echo branch1 change >file1 &&
     echo branch1 newfile >file2 &&
+    echo branch1 change file11 >file11 &&
+    echo branch1 change file13 >file13 &&
     echo branch1 sub >subdir/file3 &&
-    git add file1 file2 subdir/file3 &&
+    git add file1 file11 file13 file2 subdir/file3 &&
+    git rm file12 &&
     git commit -m "branch1 changes" &&
 
     git checkout master &&
     echo master updated >file1 &&
     echo master new >file2 &&
+    echo master updated file12 >file12 &&
+    echo master updated file14 >file14 &&
     echo master new sub >subdir/file3 &&
-    git add file1 file2 subdir/file3 &&
+    git add file1 file12 file14 file2 subdir/file3 &&
+    git rm file11 &&
     git commit -m "master updates" &&
 
     git config merge.tool mytool &&
@@ -46,6 +56,8 @@ test_expect_success 'custom mergetool' '
     ( yes "" | git mergetool file1 >/dev/null 2>&1 ) &&
     ( yes "" | git mergetool file2 >/dev/null 2>&1 ) &&
     ( yes "" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&
+    ( yes "d" | git mergetool file11 >/dev/null 2>&1 ) &&
+    ( yes "d" | git mergetool file12 >/dev/null 2>&1 ) &&
     test "$(cat file1)" = "master updated" &&
     test "$(cat file2)" = "master new" &&
     test "$(cat subdir/file3)" = "master new sub" &&
@@ -59,6 +71,8 @@ test_expect_success 'mergetool crlf' '
     ( yes "" | git mergetool file1 >/dev/null 2>&1 ) &&
     ( yes "" | git mergetool file2 >/dev/null 2>&1 ) &&
     ( yes "" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&
+    ( yes "d" | git mergetool file11 >/dev/null 2>&1 ) &&
+    ( yes "d" | git mergetool file12 >/dev/null 2>&1 ) &&
     test "$(printf x | cat file1 -)" = "$(printf "master updated\r\nx")" &&
     test "$(printf x | cat file2 -)" = "$(printf "master new\r\nx")" &&
     test "$(printf x | cat subdir/file3 -)" = "$(printf "master new sub\r\nx")" &&
@@ -82,6 +96,8 @@ test_expect_success 'mergetool on file in parent dir' '
 	cd subdir &&
 	( yes "" | git mergetool ../file1 >/dev/null 2>&1 ) &&
 	( yes "" | git mergetool ../file2 >/dev/null 2>&1 ) &&
+	( yes "d" | git mergetool ../file11 >/dev/null 2>&1 ) &&
+	( yes "d" | git mergetool ../file12 >/dev/null 2>&1 ) &&
 	test "$(cat ../file1)" = "master updated" &&
 	test "$(cat ../file2)" = "master new" &&
 	git commit -m "branch1 resolved with mergetool - subdir"
@@ -92,6 +108,8 @@ test_expect_success 'mergetool skips autoresolved' '
     git checkout -b test4 branch1 &&
     test_must_fail git merge master &&
     test -n "$(git ls-files -u)" &&
+    ( yes "d" | git mergetool file11 >/dev/null 2>&1 ) &&
+    ( yes "d" | git mergetool file12 >/dev/null 2>&1 ) &&
     output="$(git mergetool --no-prompt)" &&
     test "$output" = "No files need merging" &&
     git reset --hard
@@ -102,13 +120,23 @@ test_expect_success 'mergetool merges all from subdir' '
 	cd subdir &&
 	git config rerere.enabled false &&
 	test_must_fail git merge master &&
-	git mergetool --no-prompt &&
+	( yes "d" "d" | git mergetool --no-prompt ) &&
 	test "$(cat ../file1)" = "master updated" &&
 	test "$(cat ../file2)" = "master new" &&
 	test "$(cat file3)" = "master new sub" &&
-	git add ../file1 ../file2 file3 &&
 	git commit -m "branch2 resolved by mergetool from subdir"
     )
 '
 
+test_expect_success 'mergetool skips resolved paths when rerere is active' '
+    git config rerere.enabled true &&
+    rm -rf .git/rr-cache &&
+    git checkout -b test5 branch1
+    test_must_fail git merge master >/dev/null 2>&1 &&
+    ( yes "d" "d" | git mergetool --no-prompt >/dev/null 2>&1 ) &&
+    output="$(yes "n" | git mergetool --no-prompt)" &&
+    test "$output" = "No files need merging" &&
+    git reset --hard
+'
+
 test_done
-- 
1.7.4.rc2.33.g8a14f
Junio C Hamano· Feb 9, 2011, 21:45 UTC · re: Martin von Zweigbergk · lore

Re: [PATCH] mergetool: don't skip modify/remove conflicts

Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:
Show 9 quoted lines
> diff --git a/rerere.h b/rerere.h
> index eaa9004..107a2bc 100644
> --- a/rerere.h
> +++ b/rerere.h
> @@ -6,6 +6,9 @@
>  #define RERERE_AUTOUPDATE   01
>  #define RERERE_NOAUTOUPDATE 02
>  
> +extern void *RERERE_UTIL_PUNTED;

This is for paths without three-stages, i.e. ones rerere won't help. Needs some comment for these two symbols.

Leading "RERERE_" is necessary for clarifying the namespace, PUNTED is necessary because it defines what the symbols means, but why do we need UTIL in the name? Drop it.

> +extern void *RERERE_UTIL_STAGED;

This is for what kind? The contents on the filesystem is ready to go, added to the index, but still in MERGE_RR (i.e. "git rerere" not run yet)?

Is the real problem that git-mergetool is not running rerere when it should, I wonder...

Show 12 quoted lines
> diff --git a/rerere.c b/rerere.c
> index eb47f97..61cac54 100644
> --- a/rerere.c
> +++ b/rerere.c
> @@ -7,6 +7,10 @@
>  #include "ll-merge.h"
>  #include "attr.h"
>  
> +#define RERERE_UTIL_ELIGIBLE NULL
> +void *RERERE_UTIL_PUNTED = &RERERE_UTIL_PUNTED;
> +void *RERERE_UTIL_STAGED = &RERERE_UTIL_STAGED;
> +
Same for "ELIGIBLE"; describe what it means.
Show 8 quoted lines
>  	for (i = 0; i < active_nr; i++) {
>  		struct cache_entry *e = active_cache[i];
>  		struct string_list_item *it;
>  
> -		if (!ce_stage(e))
> +		if (!ce_stage(e)) {
>  			continue;
> +		}
Unnecessary change.
Show 5 quoted lines
>  		it = string_list_insert(conflict, (const char *)e->name);
> -		it->util = NULL;
> +		it->util = RERERE_UTIL_PUNTED;
>  		if (ce_stage(e) == 1) {
> +			it->util = RERERE_UTIL_STAGED;

Hmm, I thought that you were taling about paths that the user hand-resolved and then ran "git add" on. Why is this marked "STAGED"?

Either your logic is wrong, or the name of the symbol is.

In any case, the original code is letting rerere handle both two-way merge (stage #2 and #3 exist without state #1) and three-way merge (all three stages exist) case, and changing only the body of this if statement smells there is extremely fishy going on.

Show 21 quoted lines
> @@ -487,8 +494,9 @@ static int do_plain_rerere(struct string_list *rr, int fd)
>  
>  	for (i = 0; i < conflict.nr; i++) {
>  		const char *path = conflict.items[i].string;
> -		if (!conflict.items[i].util)
> -			continue; /* punted */
> +		if (conflict.items[i].util == RERERE_UTIL_PUNTED ||
> +			conflict.items[i].util == RERERE_UTIL_STAGED)
> +			continue;
>  		if (!string_list_has_string(rr, path)) {
>  			unsigned char sha1[20];
>  			char *hex;
> @@ -648,8 +656,9 @@ int rerere_forget(const char **pathspec)
>  	find_conflict(&conflict);
>  	for (i = 0; i < conflict.nr; i++) {
>  		struct string_list_item *it = &conflict.items[i];
> -		if (!conflict.items[i].util)
> -			continue; /* punted */
> +		if (conflict.items[i].util == RERERE_UTIL_PUNTED ||
> +			conflict.items[i].util == RERERE_UTIL_STAGED)
> +			continue;

There are a few repetition of "if it is marked with PUNTED or STAGED"; can you make it into a small helper function and give it a _meaningful_ name? What does it mean for an entry to be marked with either of these marks?

Thanks.
Martin von Zweigbergk· Feb 11, 2011, 02:46 UTC · re: Junio C Hamano · lore

Re: [PATCH] mergetool: don't skip modify/remove conflicts

On Wed, 9 Feb 2011, Junio C Hamano wrote:
Show 6 quoted lines
> Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:
> 
> > +extern void *RERERE_UTIL_STAGED;
> 
> This is for what kind?  The contents on the filesystem is ready to go,
> added to the index, but still in MERGE_RR (i.e. "git rerere" not run yet)?
Correct.
> Is the real problem that git-mergetool is not running rerere when it
> should, I wonder...

You mean that maybe it should call rerere for files that are added to the index?

Show 8 quoted lines
> >  		it = string_list_insert(conflict, (const char *)e->name);
> > -		it->util = NULL;
> > +		it->util = RERERE_UTIL_PUNTED;
> >  		if (ce_stage(e) == 1) {
> > +			it->util = RERERE_UTIL_STAGED;
> 
> Hmm, I thought that you were taling about paths that the user
> hand-resolved and then ran "git add" on.  Why is this marked "STAGED"?
Very good question. It shouldn't.
> Either your logic is wrong, or the name of the symbol is.

The former. What I really wanted to check is if the file has been added to the index. I am currently struggling with how to do this correctly. My inexperience with the git internals makes it a very slow process with a lot of guesses and trial and error.

Show 9 quoted lines
> > -		if (!conflict.items[i].util)
> > -			continue; /* punted */
> > +		if (conflict.items[i].util == RERERE_UTIL_PUNTED ||
> > +			conflict.items[i].util == RERERE_UTIL_STAGED)
> > +			continue;
> 
> There are a few repetition of "if it is marked with PUNTED or STAGED"; can
> you make it into a small helper function and give it a _meaningful_ name?
> What does it mean for an entry to be marked with either of these marks?

There is another big problem here. I had not realized that the util field would be freed later (being spoilt by GCing languages, I guess) and that will of course fail if it is one of the two constants I defined. I stupidly did not run the commands manually, but relied only on the test cases, so I didn't notice the segfault at the end. Somehow all the test cases still pass.

I think this means that anyone intending to run rerere should not build pu at the moment, right? I'm really sorry about the inconvenience :-(.

/Martin
Martin von Zweigbergk· Feb 13, 2011, 04:09 UTC · re: Martin von Zweigbergk · lore

[PATCH v2 0/2] mergetool: don't skip modify/remove conflicts

Trying again. I hope it looks better this time. The second patch could be squashed with the first one.

This applies on top of jc/rerere-remaining.
Martin von Zweigbergk (2):
  mergetool: don't skip modify/remove conflicts
  rerere: factor out common conflict search code
 builtin/rerere.c     |   21 +++++-----
 git-mergetool.sh     |    2 +-
 rerere.c             |  109 +++++++++++++++++++++++++++++---------------------
 rerere.h             |    8 ++++
 t/t7610-mergetool.sh |   40 +++++++++++++++---
 5 files changed, 117 insertions(+), 63 deletions(-)
-- 
1.7.4.rc2.33.g8a14f
Martin von Zweigbergk· Feb 13, 2011, 04:09 UTC · re: Martin von Zweigbergk · lore

[PATCH v2 1/2] mergetool: don't skip modify/remove conflicts

Since bb0a484 (mergetool: Skip autoresolved paths, 2010-08-17), mergetool uses different ways of figuring out the list of files with merge conflicts depending on whether rerere is active. If rerere is active, mergetool will use 'git rerere status' to list the files with remaining conflicts. However, the output from that command does not list conflicts of types that rerere does not handle, such as modify/remove conflicts.

Another problem with solely relying on the output from 'git rerere status' is that, for new conflicts that are not yet known to rerere, the output from the command will list the files even after adding them to the index. This means that if the conflicts in some files have been resolved and 'git mergetool' is run again, it will ask the user something like the following for each of those files.

 file1: file does not need merging
 Continue merging other unresolved paths (y/n) ?

Solve the first of these problems by replacing the call to 'git rerere status' with a call to the new 'git rerere remaining' that was introduced in the previous commit. Solve the second problem by modifying 'git rerere remaining' not to output files that have already been staged.

Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
---

Ideally, I would have liked to just remove the resolved elements from the string_list, but since there is no function that drops an element, I marked it by pointing its util field to a special address. Is this a good way of doing it or can/should I manually unlink the element from the list instead?

 builtin/rerere.c     |   21 +++++++++++----------
 git-mergetool.sh     |    2 +-
 rerere.c             |   24 ++++++++++++++++++++----
 rerere.h             |    8 ++++++++
 t/t7610-mergetool.sh |   40 ++++++++++++++++++++++++++++++++++------
 5 files changed, 74 insertions(+), 21 deletions(-)
Show changes to 5 files +74 −21

builtin/rerere.c, git-mergetool.sh, rerere.c, rerere.h, t/t7610-mergetool.sh

diff --git a/builtin/rerere.c b/builtin/rerere.c
index 081fccc..7b9fe18 100644
--- a/builtin/rerere.c
+++ b/builtin/rerere.c
@@ -147,8 +147,6 @@ int cmd_rerere(int argc, const char **argv, const char *prefix)
 	if (!strcmp(argv[0], "clear")) {
 		for (i = 0; i < merge_rr.nr; i++) {
 			const char *name = (const char *)merge_rr.items[i].util;
-			if (!name)
-				continue;
 			if (!has_rerere_resolution(name))
 				unlink_rr_item(name);
 		}
@@ -157,19 +155,22 @@ int cmd_rerere(int argc, const char **argv, const char *prefix)
 		garbage_collect(&merge_rr);
 	else if (!strcmp(argv[0], "status"))
 		for (i = 0; i < merge_rr.nr; i++) {
-			if (!merge_rr.items[i].util)
-				continue;
 			printf("%s\n", merge_rr.items[i].string);
 		}
-	else if (!strcmp(argv[0], "remaining"))
-		for (i = 0; i < merge_rr.nr; i++)
-			printf("%s\n", merge_rr.items[i].string);
-	else if (!strcmp(argv[0], "diff"))
+	else if (!strcmp(argv[0], "remaining")) {
+		rerere_remaining(&merge_rr);
+		for (i = 0; i < merge_rr.nr; i++) {
+			if (merge_rr.items[i].util != RERERE_RESOLVED)
+				printf("%s\n", merge_rr.items[i].string);
+			else
+				/* prepare for later call to
+				 * string_list_clear() */
+				merge_rr.items[i].util = NULL;
+		}
+	} else if (!strcmp(argv[0], "diff"))
 		for (i = 0; i < merge_rr.nr; i++) {
 			const char *path = merge_rr.items[i].string;
 			const char *name = (const char *)merge_rr.items[i].util;
-			if (!name)
-				continue;
 			diff_two(rerere_path(name, "preimage"), path, path, path);
 		}
 	else
diff --git a/git-mergetool.sh b/git-mergetool.sh
index 2f8dc44..bacbda2 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -269,7 +269,7 @@ rerere=false
 files_to_merge() {
     if test "$rerere" = true
     then
-	git rerere status
+	git rerere remaining
     else
 	git ls-files -u | sed -e 's/^[^	]*	//' | sort -u
     fi
diff --git a/rerere.c b/rerere.c
index eb47f97..e5bccd5 100644
--- a/rerere.c
+++ b/rerere.c
@@ -7,6 +7,8 @@
 #include "ll-merge.h"
 #include "attr.h"
 
+void *RERERE_RESOLVED = &RERERE_RESOLVED;
+
 /* if rerere_enabled == -1, fall back to detection of .git/rr-cache */
 static int rerere_enabled = -1;
 
@@ -388,16 +390,31 @@ static int find_conflict(struct string_list *conflict)
 	return 0;
 }
 
-static void add_punted(struct string_list *merge_rr)
+void rerere_remaining(struct string_list *merge_rr)
 {
 	int i;
 	struct string_list conflict = STRING_LIST_INIT_DUP;
 
+	/* Add punted paths */
 	find_conflict(&conflict);
 	for (i = 0; i < conflict.nr; i++) {
-		if (conflict.items[i].util)
+		if (!conflict.items[i].util)
+			string_list_insert(merge_rr, conflict.items[i].string);
+	}
+
+	/* Mark already resolved paths */
+	for (i = 0; i < active_nr; i++) {
+		struct cache_entry *e = active_cache[i];
+
+		if (!ce_stage(e)) {
+			struct string_list_item *it;
+			it = string_list_lookup(merge_rr, (const char *)e->name);
+			if (it != NULL) {
+				free(it->util);
+				it->util = RERERE_RESOLVED;
+			}
 			continue;
-		string_list_insert(merge_rr, conflict.items[i].string);
+		}
 	}
 }
 
@@ -592,7 +609,6 @@ int setup_rerere(struct string_list *merge_rr, int flags)
 	fd = hold_lock_file_for_update(&write_lock, merge_rr_path,
 				       LOCK_DIE_ON_ERROR);
 	read_rr(merge_rr);
-	add_punted(merge_rr);
 	return fd;
 }
 
diff --git a/rerere.h b/rerere.h
index eaa9004..ea2618b 100644
--- a/rerere.h
+++ b/rerere.h
@@ -6,11 +6,19 @@
 #define RERERE_AUTOUPDATE   01
 #define RERERE_NOAUTOUPDATE 02
 
+/*
+ * Marks paths that have been hand-resolved and added to the
+ * index. Set in the util field of such paths after calling
+ * rerere_remaining.
+ */
+extern void *RERERE_RESOLVED;
+
 extern int setup_rerere(struct string_list *, int);
 extern int rerere(int);
 extern const char *rerere_path(const char *hex, const char *file);
 extern int has_rerere_resolution(const char *hex);
 extern int rerere_forget(const char **);
+extern void rerere_remaining(struct string_list *);
 
 #define OPT_RERERE_AUTOUPDATE(v) OPT_UYN(0, "rerere-autoupdate", (v), \
 	"update the index with reused conflict resolution if possible")
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index d78bdec..dc838c9 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -16,23 +16,33 @@ Testing basic merge tool invocation'
 test_expect_success 'setup' '
     git config rerere.enabled true &&
     echo master >file1 &&
+    echo master file11 >file11 &&
+    echo master file12 >file12 &&
+    echo master file13 >file13 &&
+    echo master file14 >file14 &&
     mkdir subdir &&
     echo master sub >subdir/file3 &&
-    git add file1 subdir/file3 &&
-    git commit -m "added file1" &&
+    git add file1 file1[1-4] subdir/file3 &&
+    git commit -m "add initial versions" &&
 
     git checkout -b branch1 master &&
     echo branch1 change >file1 &&
     echo branch1 newfile >file2 &&
+    echo branch1 change file11 >file11 &&
+    echo branch1 change file13 >file13 &&
     echo branch1 sub >subdir/file3 &&
-    git add file1 file2 subdir/file3 &&
+    git add file1 file11 file13 file2 subdir/file3 &&
+    git rm file12 &&
     git commit -m "branch1 changes" &&
 
     git checkout master &&
     echo master updated >file1 &&
     echo master new >file2 &&
+    echo master updated file12 >file12 &&
+    echo master updated file14 >file14 &&
     echo master new sub >subdir/file3 &&
-    git add file1 file2 subdir/file3 &&
+    git add file1 file12 file14 file2 subdir/file3 &&
+    git rm file11 &&
     git commit -m "master updates" &&
 
     git config merge.tool mytool &&
@@ -46,6 +56,8 @@ test_expect_success 'custom mergetool' '
     ( yes "" | git mergetool file1 >/dev/null 2>&1 ) &&
     ( yes "" | git mergetool file2 >/dev/null 2>&1 ) &&
     ( yes "" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&
+    ( yes "d" | git mergetool file11 >/dev/null 2>&1 ) &&
+    ( yes "d" | git mergetool file12 >/dev/null 2>&1 ) &&
     test "$(cat file1)" = "master updated" &&
     test "$(cat file2)" = "master new" &&
     test "$(cat subdir/file3)" = "master new sub" &&
@@ -59,6 +71,8 @@ test_expect_success 'mergetool crlf' '
     ( yes "" | git mergetool file1 >/dev/null 2>&1 ) &&
     ( yes "" | git mergetool file2 >/dev/null 2>&1 ) &&
     ( yes "" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&
+    ( yes "d" | git mergetool file11 >/dev/null 2>&1 ) &&
+    ( yes "d" | git mergetool file12 >/dev/null 2>&1 ) &&
     test "$(printf x | cat file1 -)" = "$(printf "master updated\r\nx")" &&
     test "$(printf x | cat file2 -)" = "$(printf "master new\r\nx")" &&
     test "$(printf x | cat subdir/file3 -)" = "$(printf "master new sub\r\nx")" &&
@@ -82,6 +96,8 @@ test_expect_success 'mergetool on file in parent dir' '
 	cd subdir &&
 	( yes "" | git mergetool ../file1 >/dev/null 2>&1 ) &&
 	( yes "" | git mergetool ../file2 >/dev/null 2>&1 ) &&
+	( yes "d" | git mergetool ../file11 >/dev/null 2>&1 ) &&
+	( yes "d" | git mergetool ../file12 >/dev/null 2>&1 ) &&
 	test "$(cat ../file1)" = "master updated" &&
 	test "$(cat ../file2)" = "master new" &&
 	git commit -m "branch1 resolved with mergetool - subdir"
@@ -92,6 +108,8 @@ test_expect_success 'mergetool skips autoresolved' '
     git checkout -b test4 branch1 &&
     test_must_fail git merge master &&
     test -n "$(git ls-files -u)" &&
+    ( yes "d" | git mergetool file11 >/dev/null 2>&1 ) &&
+    ( yes "d" | git mergetool file12 >/dev/null 2>&1 ) &&
     output="$(git mergetool --no-prompt)" &&
     test "$output" = "No files need merging" &&
     git reset --hard
@@ -102,13 +120,23 @@ test_expect_success 'mergetool merges all from subdir' '
 	cd subdir &&
 	git config rerere.enabled false &&
 	test_must_fail git merge master &&
-	git mergetool --no-prompt &&
+	( yes "d" "d" | git mergetool --no-prompt ) &&
 	test "$(cat ../file1)" = "master updated" &&
 	test "$(cat ../file2)" = "master new" &&
 	test "$(cat file3)" = "master new sub" &&
-	git add ../file1 ../file2 file3 &&
 	git commit -m "branch2 resolved by mergetool from subdir"
     )
 '
 
+test_expect_success 'mergetool skips resolved paths when rerere is active' '
+    git config rerere.enabled true &&
+    rm -rf .git/rr-cache &&
+    git checkout -b test5 branch1
+    test_must_fail git merge master >/dev/null 2>&1 &&
+    ( yes "d" "d" | git mergetool --no-prompt >/dev/null 2>&1 ) &&
+    output="$(yes "n" | git mergetool --no-prompt)" &&
+    test "$output" = "No files need merging" &&
+    git reset --hard
+'
+
 test_done
-- 
1.7.4.rc2.33.g8a14f
Junio C Hamano· Feb 15, 2011, 00:54 UTC · re: Martin von Zweigbergk · lore

Re: [PATCH v2 1/2] mergetool: don't skip modify/remove conflicts

Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:
Show 44 quoted lines
> diff --git a/builtin/rerere.c b/builtin/rerere.c
> index 081fccc..7b9fe18 100644
> --- a/builtin/rerere.c
> +++ b/builtin/rerere.c
> @@ -147,8 +147,6 @@ int cmd_rerere(int argc, const char **argv, const char *prefix)
>  	if (!strcmp(argv[0], "clear")) {
>  		for (i = 0; i < merge_rr.nr; i++) {
>  			const char *name = (const char *)merge_rr.items[i].util;
> -			if (!name)
> -				continue;
>  			if (!has_rerere_resolution(name))
>  				unlink_rr_item(name);
>  		}
> @@ -157,19 +155,22 @@ int cmd_rerere(int argc, const char **argv, const char *prefix)
>  		garbage_collect(&merge_rr);
>  	else if (!strcmp(argv[0], "status"))
>  		for (i = 0; i < merge_rr.nr; i++) {
> -			if (!merge_rr.items[i].util)
> -				continue;
>  			printf("%s\n", merge_rr.items[i].string);
>  		}
> -	else if (!strcmp(argv[0], "remaining"))
> -		for (i = 0; i < merge_rr.nr; i++)
> -			printf("%s\n", merge_rr.items[i].string);
> -	else if (!strcmp(argv[0], "diff"))
> +	else if (!strcmp(argv[0], "remaining")) {
> +		rerere_remaining(&merge_rr);
> +		for (i = 0; i < merge_rr.nr; i++) {
> +			if (merge_rr.items[i].util != RERERE_RESOLVED)
> +				printf("%s\n", merge_rr.items[i].string);
> +			else
> +				/* prepare for later call to
> +				 * string_list_clear() */
> +				merge_rr.items[i].util = NULL;
> +		}
> +	} else if (!strcmp(argv[0], "diff"))
>  		for (i = 0; i < merge_rr.nr; i++) {
>  			const char *path = merge_rr.items[i].string;
>  			const char *name = (const char *)merge_rr.items[i].util;
> -			if (!name)
> -				continue;
>  			diff_two(rerere_path(name, "preimage"), path, path, path);
>  		}
>  	else

By looking at the diff between the parent of f322b35 (my earlier "rerere remaining" patch) and the result of applying this patch on top of the patch, I think the basic idea of this is to correct my stupid mistake of contaminating merge_rr unconditionally, and instead adding the "remaining" entries only when handling the "rerere remaining" command (hence many removal of special cases "if (!name) continue" introduced by my patch. The result looks _much_ cleaner than the above patch shows and I am happy with it.

But shouldn't you also revert the parts of my patch to do_plain_rerere() and rerere_forget() that have similar special cases?

Martin von Zweigbergk· Feb 15, 2011, 03:30 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 1/2] mergetool: don't skip modify/remove conflicts

On Mon, 14 Feb 2011, Junio C Hamano wrote:
> But shouldn't you also revert the parts of my patch to do_plain_rerere()
> and rerere_forget() that have similar special cases?
Yep, done in patch 2/2.

Your changes in find_conflicts() are still there after patch 1/2 and since do_plain_rerere() and rerere_forget() call find_conflicts(), I think they need to check for the punted paths. Only the callers of setup_rerere() no longer need to care.

In patch 2/2, I refactored a bit more, so that find_conflicts() behaves the way it did before your first patch, and then the checks for punted paths are removed from the callers.

If you think the second one is good, it could easily be squashed with the first one. I left it separate because I wasn't quite sure. For example, could the interface to check_one_conflict() be improved? As you said, part of the changes from your patch are now undone, so I guess all three patches could even be squashed.

/Martin
Junio C Hamano· Feb 15, 2011, 20:12 UTC · re: Martin von Zweigbergk · lore

Re: [PATCH v2 1/2] mergetool: don't skip modify/remove conflicts

Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:
Show 18 quoted lines
> On Mon, 14 Feb 2011, Junio C Hamano wrote:
>
>> But shouldn't you also revert the parts of my patch to do_plain_rerere()
>> and rerere_forget() that have similar special cases?
>
> Yep, done in patch 2/2.
>
> Your changes in find_conflicts() are still there after patch 1/2 and
> since do_plain_rerere() and rerere_forget() call find_conflicts(), I
> think they need to check for the punted paths. Only the callers of
> setup_rerere() no longer need to care.
>
> In patch 2/2, I refactored a bit more, so that find_conflicts()
> behaves the way it did before your first patch, and then the checks
> for punted paths are removed from the callers.
>
> If you think the second one is good, it could easily be squashed with
> the first one.  I left it separate because I wasn't quite sure.

Probably the changes to rerere proper from all three patches should be squashed into one, and then the patch to the script to use the new feature can be applied on top of it.

Martin von Zweigbergk· Feb 16, 2011, 10:47 UTC · re: Junio C Hamano · lore

[PATCH v3 0/2] mergetool: don't skip modify/remove conflicts

Third round. Reorganized the patches a bit as suggested by Junio. The first patch now contains all the changes to rerere and the second one only makes mergetool call 'rerere remaining'.

The only tiny change to the end result is the removal of a blank line.
The series applies on top of master.
Junio C Hamano (1):
  rerere "remaining"
Martin von Zweigbergk (1):
  mergetool: don't skip modify/remove conflicts
 builtin/rerere.c     |   17 +++++++++--
 git-mergetool.sh     |    2 +-
 rerere.c             |   79 +++++++++++++++++++++++++++++++++++++++++++------
 rerere.h             |    8 +++++
 t/t7610-mergetool.sh |   40 +++++++++++++++++++++----
 5 files changed, 126 insertions(+), 20 deletions(-)
-- 
1.7.4.rc2.33.g8a14f
Martin von Zweigbergk· Feb 16, 2011, 10:47 UTC · re: Martin von Zweigbergk · lore

[PATCH v3 1/2] rerere "remaining"

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

After "rerere" resolves conflicts by reusing old resolution, there would be three kinds of paths with conflict in the index:

 * paths that have been resolved in the working tree by rerere;
 * paths that need further work whose resolution could be recorded;
 * paths that need resolving that rerere won't help.

When the user wants a list of paths that need hand-resolving, output from "rerere status" does not help, as it shows only the second category, but the paths in the third category still needs work (rerere only makes sense for regular files that have both our side and their side, and does not help other kinds of conflicts, e.g. "we modified, they deleted").

The new subcommand "rerere remaining" can be used to show both. As opposed to "rerere status", this subcommand also skips printing paths that have been added to the index, since these paths are already resolved and are no longer "remaining".

Initial patch provided by Junio. Refactored and modified to skip resolved paths by Martin. Commit message mostly by Junio.

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
---

Junio, I wasn't sure how to handle the sign-off etc, so feel free to modify those as appropriate.

 builtin/rerere.c |   17 +++++++++--
 rerere.c         |   79 +++++++++++++++++++++++++++++++++++++++++++++++-------
 rerere.h         |    8 +++++
 3 files changed, 91 insertions(+), 13 deletions(-)
Show changes to 3 files +91 −13

builtin/rerere.c, rerere.c, rerere.h

diff --git a/builtin/rerere.c b/builtin/rerere.c
index 642bf35..7b9fe18 100644
--- a/builtin/rerere.c
+++ b/builtin/rerere.c
@@ -8,7 +8,7 @@
 #include "xdiff-interface.h"
 
 static const char * const rerere_usage[] = {
-	"git rerere [clear | status | diff | gc]",
+	"git rerere [clear | status | remaining | diff | gc]",
 	NULL,
 };
 
@@ -154,9 +154,20 @@ int cmd_rerere(int argc, const char **argv, const char *prefix)
 	} else if (!strcmp(argv[0], "gc"))
 		garbage_collect(&merge_rr);
 	else if (!strcmp(argv[0], "status"))
-		for (i = 0; i < merge_rr.nr; i++)
+		for (i = 0; i < merge_rr.nr; i++) {
 			printf("%s\n", merge_rr.items[i].string);
-	else if (!strcmp(argv[0], "diff"))
+		}
+	else if (!strcmp(argv[0], "remaining")) {
+		rerere_remaining(&merge_rr);
+		for (i = 0; i < merge_rr.nr; i++) {
+			if (merge_rr.items[i].util != RERERE_RESOLVED)
+				printf("%s\n", merge_rr.items[i].string);
+			else
+				/* prepare for later call to
+				 * string_list_clear() */
+				merge_rr.items[i].util = NULL;
+		}
+	} else if (!strcmp(argv[0], "diff"))
 		for (i = 0; i < merge_rr.nr; i++) {
 			const char *path = merge_rr.items[i].string;
 			const char *name = (const char *)merge_rr.items[i].util;
diff --git a/rerere.c b/rerere.c
index d260843..1240d06 100644
--- a/rerere.c
+++ b/rerere.c
@@ -7,6 +7,11 @@
 #include "ll-merge.h"
 #include "attr.h"
 
+#define RESOLVED 0
+#define PUNTED 1
+#define THREE_STAGED 2
+void *RERERE_RESOLVED = &RERERE_RESOLVED;
+
 /* if rerere_enabled == -1, fall back to detection of .git/rr-cache */
 static int rerere_enabled = -1;
 
@@ -345,21 +350,75 @@ static int handle_cache(const char *path, unsigned char *sha1, const char *outpu
 	return hunk_no;
 }
 
-static int find_conflict(struct string_list *conflict)
+static int check_one_conflict(int i, int *type)
 {
-	int i;
-	if (read_cache() < 0)
-		return error("Could not read index");
-	for (i = 0; i+1 < active_nr; i++) {
+	struct cache_entry *e = active_cache[i];
+	struct string_list_item *it;
+
+	if (!ce_stage(e)) {
+		*type = RESOLVED;
+		return i + 1;
+	}
+
+	*type = PUNTED;
+	if (ce_stage(e) == 1) {
+		if (active_nr <= ++i)
+			return i + 1;
+	}
+
+	/* Only handle regular files with both stages #2 and #3 */
+	if (i + 1 < active_nr) {
 		struct cache_entry *e2 = active_cache[i];
-		struct cache_entry *e3 = active_cache[i+1];
+		struct cache_entry *e3 = active_cache[i + 1];
 		if (ce_stage(e2) == 2 &&
 		    ce_stage(e3) == 3 &&
-		    ce_same_name(e2, e3) &&
+		    ce_same_name(e, e3) &&
 		    S_ISREG(e2->ce_mode) &&
-		    S_ISREG(e3->ce_mode)) {
-			string_list_insert(conflict, (const char *)e2->name);
-			i++; /* skip over both #2 and #3 */
+		    S_ISREG(e3->ce_mode))
+			*type = THREE_STAGED;
+	}
+
+	/* Skip the entries with the same name */
+	while (i < active_nr && ce_same_name(e, active_cache[i]))
+		i++;
+	return i;
+}
+
+static int find_conflict(struct string_list *conflict)
+{
+	int i;
+	if (read_cache() < 0)
+		return error("Could not read index");
+
+	for (i = 0; i < active_nr;) {
+		int conflict_type;
+		struct cache_entry *e = active_cache[i];
+		i = check_one_conflict(i, &conflict_type);
+		if (conflict_type == THREE_STAGED)
+			string_list_insert(conflict, (const char *)e->name);
+	}
+	return 0;
+}
+
+int rerere_remaining(struct string_list *merge_rr)
+{
+	int i;
+	if (read_cache() < 0)
+		return error("Could not read index");
+
+	for (i = 0; i < active_nr;) {
+		int conflict_type;
+		struct cache_entry *e = active_cache[i];
+		i = check_one_conflict(i, &conflict_type);
+		if (conflict_type == PUNTED)
+			string_list_insert(merge_rr, (const char *)e->name);
+		else if (conflict_type == RESOLVED) {
+			struct string_list_item *it;
+			it = string_list_lookup(merge_rr, (const char *)e->name);
+			if (it != NULL) {
+				free(it->util);
+				it->util = RERERE_RESOLVED;
+			}
 		}
 	}
 	return 0;
diff --git a/rerere.h b/rerere.h
index eaa9004..595f49f 100644
--- a/rerere.h
+++ b/rerere.h
@@ -6,11 +6,19 @@
 #define RERERE_AUTOUPDATE   01
 #define RERERE_NOAUTOUPDATE 02
 
+/*
+ * Marks paths that have been hand-resolved and added to the
+ * index. Set in the util field of such paths after calling
+ * rerere_remaining.
+ */
+extern void *RERERE_RESOLVED;
+
 extern int setup_rerere(struct string_list *, int);
 extern int rerere(int);
 extern const char *rerere_path(const char *hex, const char *file);
 extern int has_rerere_resolution(const char *hex);
 extern int rerere_forget(const char **);
+extern int rerere_remaining(struct string_list *);
 
 #define OPT_RERERE_AUTOUPDATE(v) OPT_UYN(0, "rerere-autoupdate", (v), \
 	"update the index with reused conflict resolution if possible")
-- 
1.7.4.rc2.33.g8a14f
Junio C Hamano· Feb 16, 2011, 21:14 UTC · re: Martin von Zweigbergk · lore

Re: [PATCH v3 1/2] rerere "remaining"

Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:
> Junio, I wasn't sure how to handle the sign-off etc, so feel free to
> modify those as appropriate.

At this point I think the primary value of the change is your adding "rerere_remaining()" to only where it matters (i.e. the isolation of the change) and you should get the credit as the primary author. Mine was impossible to read because I contaminated merge_rr in a wrong place and had to work it around in unrelated codepaths all over the place.

This was much cleaner and easier to read.
Show 27 quoted lines
>  builtin/rerere.c |   17 +++++++++--
>  rerere.c         |   79 +++++++++++++++++++++++++++++++++++++++++++++++-------
>  rerere.h         |    8 +++++
>  3 files changed, 91 insertions(+), 13 deletions(-)
>
> diff --git a/builtin/rerere.c b/builtin/rerere.c
> index 642bf35..7b9fe18 100644
> --- a/builtin/rerere.c
> +++ b/builtin/rerere.c
> @@ -8,7 +8,7 @@
>  #include "xdiff-interface.h"
>  
>  static const char * const rerere_usage[] = {
> -	"git rerere [clear | status | diff | gc]",
> +	"git rerere [clear | status | remaining | diff | gc]",
>  	NULL,
>  };
>  
> @@ -154,9 +154,20 @@ int cmd_rerere(int argc, const char **argv, const char *prefix)
>  	} else if (!strcmp(argv[0], "gc"))
>  		garbage_collect(&merge_rr);
>  	else if (!strcmp(argv[0], "status"))
> -		for (i = 0; i < merge_rr.nr; i++)
> +		for (i = 0; i < merge_rr.nr; i++) {
>  			printf("%s\n", merge_rr.items[i].string);
> -	else if (!strcmp(argv[0], "diff"))
> +		}
Unnecessary {} around a single printf().  Will clean-up.
Thanks.
Martin von Zweigbergk· Feb 16, 2011, 10:47 UTC · re: Martin von Zweigbergk · lore

[PATCH v3 2/2] mergetool: don't skip modify/remove conflicts

Since bb0a484 (mergetool: Skip autoresolved paths, 2010-08-17), mergetool uses different ways of figuring out the list of files with merge conflicts depending on whether rerere is active. If rerere is active, mergetool will use 'git rerere status' to list the files with remaining conflicts. However, the output from that command does not list conflicts of types that rerere does not handle, such as modify/remove conflicts.

Another problem with solely relying on the output from 'git rerere status' is that, for new conflicts that are not yet known to rerere, the output from the command will list the files even after adding them to the index. This means that if the conflicts in some files have been resolved and 'git mergetool' is run again, it will ask the user something like the following for each of those files.

 file1: file does not need merging
 Continue merging other unresolved paths (y/n) ?

Solve both of these problems by replacing the call to 'git rerere status' with a call to the new 'git rerere remaining' that was introduced in the previous commit.

Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
---
 git-mergetool.sh     |    2 +-
 t/t7610-mergetool.sh |   40 ++++++++++++++++++++++++++++++++++------
 2 files changed, 35 insertions(+), 7 deletions(-)
Show changes to 2 files +35 −7

git-mergetool.sh, t/t7610-mergetool.sh

diff --git a/git-mergetool.sh b/git-mergetool.sh
index 2f8dc44..bacbda2 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -269,7 +269,7 @@ rerere=false
 files_to_merge() {
     if test "$rerere" = true
     then
-	git rerere status
+	git rerere remaining
     else
 	git ls-files -u | sed -e 's/^[^	]*	//' | sort -u
     fi
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index d78bdec..dc838c9 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -16,23 +16,33 @@ Testing basic merge tool invocation'
 test_expect_success 'setup' '
     git config rerere.enabled true &&
     echo master >file1 &&
+    echo master file11 >file11 &&
+    echo master file12 >file12 &&
+    echo master file13 >file13 &&
+    echo master file14 >file14 &&
     mkdir subdir &&
     echo master sub >subdir/file3 &&
-    git add file1 subdir/file3 &&
-    git commit -m "added file1" &&
+    git add file1 file1[1-4] subdir/file3 &&
+    git commit -m "add initial versions" &&
 
     git checkout -b branch1 master &&
     echo branch1 change >file1 &&
     echo branch1 newfile >file2 &&
+    echo branch1 change file11 >file11 &&
+    echo branch1 change file13 >file13 &&
     echo branch1 sub >subdir/file3 &&
-    git add file1 file2 subdir/file3 &&
+    git add file1 file11 file13 file2 subdir/file3 &&
+    git rm file12 &&
     git commit -m "branch1 changes" &&
 
     git checkout master &&
     echo master updated >file1 &&
     echo master new >file2 &&
+    echo master updated file12 >file12 &&
+    echo master updated file14 >file14 &&
     echo master new sub >subdir/file3 &&
-    git add file1 file2 subdir/file3 &&
+    git add file1 file12 file14 file2 subdir/file3 &&
+    git rm file11 &&
     git commit -m "master updates" &&
 
     git config merge.tool mytool &&
@@ -46,6 +56,8 @@ test_expect_success 'custom mergetool' '
     ( yes "" | git mergetool file1 >/dev/null 2>&1 ) &&
     ( yes "" | git mergetool file2 >/dev/null 2>&1 ) &&
     ( yes "" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&
+    ( yes "d" | git mergetool file11 >/dev/null 2>&1 ) &&
+    ( yes "d" | git mergetool file12 >/dev/null 2>&1 ) &&
     test "$(cat file1)" = "master updated" &&
     test "$(cat file2)" = "master new" &&
     test "$(cat subdir/file3)" = "master new sub" &&
@@ -59,6 +71,8 @@ test_expect_success 'mergetool crlf' '
     ( yes "" | git mergetool file1 >/dev/null 2>&1 ) &&
     ( yes "" | git mergetool file2 >/dev/null 2>&1 ) &&
     ( yes "" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&
+    ( yes "d" | git mergetool file11 >/dev/null 2>&1 ) &&
+    ( yes "d" | git mergetool file12 >/dev/null 2>&1 ) &&
     test "$(printf x | cat file1 -)" = "$(printf "master updated\r\nx")" &&
     test "$(printf x | cat file2 -)" = "$(printf "master new\r\nx")" &&
     test "$(printf x | cat subdir/file3 -)" = "$(printf "master new sub\r\nx")" &&
@@ -82,6 +96,8 @@ test_expect_success 'mergetool on file in parent dir' '
 	cd subdir &&
 	( yes "" | git mergetool ../file1 >/dev/null 2>&1 ) &&
 	( yes "" | git mergetool ../file2 >/dev/null 2>&1 ) &&
+	( yes "d" | git mergetool ../file11 >/dev/null 2>&1 ) &&
+	( yes "d" | git mergetool ../file12 >/dev/null 2>&1 ) &&
 	test "$(cat ../file1)" = "master updated" &&
 	test "$(cat ../file2)" = "master new" &&
 	git commit -m "branch1 resolved with mergetool - subdir"
@@ -92,6 +108,8 @@ test_expect_success 'mergetool skips autoresolved' '
     git checkout -b test4 branch1 &&
     test_must_fail git merge master &&
     test -n "$(git ls-files -u)" &&
+    ( yes "d" | git mergetool file11 >/dev/null 2>&1 ) &&
+    ( yes "d" | git mergetool file12 >/dev/null 2>&1 ) &&
     output="$(git mergetool --no-prompt)" &&
     test "$output" = "No files need merging" &&
     git reset --hard
@@ -102,13 +120,23 @@ test_expect_success 'mergetool merges all from subdir' '
 	cd subdir &&
 	git config rerere.enabled false &&
 	test_must_fail git merge master &&
-	git mergetool --no-prompt &&
+	( yes "d" "d" | git mergetool --no-prompt ) &&
 	test "$(cat ../file1)" = "master updated" &&
 	test "$(cat ../file2)" = "master new" &&
 	test "$(cat file3)" = "master new sub" &&
-	git add ../file1 ../file2 file3 &&
 	git commit -m "branch2 resolved by mergetool from subdir"
     )
 '
 
+test_expect_success 'mergetool skips resolved paths when rerere is active' '
+    git config rerere.enabled true &&
+    rm -rf .git/rr-cache &&
+    git checkout -b test5 branch1
+    test_must_fail git merge master >/dev/null 2>&1 &&
+    ( yes "d" "d" | git mergetool --no-prompt >/dev/null 2>&1 ) &&
+    output="$(yes "n" | git mergetool --no-prompt)" &&
+    test "$output" = "No files need merging" &&
+    git reset --hard
+'
+
 test_done
-- 
1.7.4.rc2.33.g8a14f
Martin von Zweigbergk· Feb 13, 2011, 04:09 UTC · re: Martin von Zweigbergk · lore

[PATCH v2 2/2] rerere: factor out common conflict search code

A little bit of the code in the new rerere_remaining function is shared with find_conflicts. Factor this out into a new function that is called from the two mentioned functions. Apart from reducing code duplication, it also means that the callers of find_conflict do not need to handle the case when the util pointer is NULL, because that is only set in rerere_remaining now.

Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
---
 rerere.c |  105 +++++++++++++++++++++++++++++++------------------------------
 rerere.h |    2 +-
 2 files changed, 54 insertions(+), 53 deletions(-)
Show changes to 2 files +54 −53

rerere.c, rerere.h

diff --git a/rerere.c b/rerere.c
index e5bccd5..d14fd9a 100644
--- a/rerere.c
+++ b/rerere.c
@@ -7,6 +7,9 @@
 #include "ll-merge.h"
 #include "attr.h"
 
+#define RESOLVED 0
+#define PUNTED 1
+#define THREE_STAGED 2
 void *RERERE_RESOLVED = &RERERE_RESOLVED;
 
 /* if rerere_enabled == -1, fall back to detection of .git/rr-cache */
@@ -347,75 +350,79 @@ static int handle_cache(const char *path, unsigned char *sha1, const char *outpu
 	return hunk_no;
 }
 
+
+static int check_one_conflict(int i, int *type)
+{
+	struct cache_entry *e = active_cache[i];
+	struct string_list_item *it;
+
+	if (!ce_stage(e)) {
+		*type = RESOLVED;
+		return i + 1;
+	}
+
+	*type = PUNTED;
+	if (ce_stage(e) == 1) {
+		if (active_nr <= ++i)
+			return i + 1;
+	}
+
+	/* Only handle regular files with both stages #2 and #3 */
+	if (i + 1 < active_nr) {
+		struct cache_entry *e2 = active_cache[i];
+		struct cache_entry *e3 = active_cache[i + 1];
+		if (ce_stage(e2) == 2 &&
+		    ce_stage(e3) == 3 &&
+		    ce_same_name(e, e3) &&
+		    S_ISREG(e2->ce_mode) &&
+		    S_ISREG(e3->ce_mode))
+			*type = THREE_STAGED;
+	}
+
+	/* Skip the entries with the same name */
+	while (i < active_nr && ce_same_name(e, active_cache[i]))
+		i++;
+	return i;
+}
+
 static int find_conflict(struct string_list *conflict)
 {
 	int i;
 	if (read_cache() < 0)
 		return error("Could not read index");
 
-	/*
-	 * Collect paths with conflict, mark them with NULL (punted) or
-	 * !NULL (eligible) in their ->util field.
-	 */
-	for (i = 0; i < active_nr; i++) {
+	for (i = 0; i < active_nr;) {
+		int conflict_type;
 		struct cache_entry *e = active_cache[i];
-		struct string_list_item *it;
-
-		if (!ce_stage(e))
-			continue;
-		it = string_list_insert(conflict, (const char *)e->name);
-		it->util = NULL;
-		if (ce_stage(e) == 1) {
-			if (active_nr <= ++i)
-				break;
-		}
-
-		/* Only handle regular files with both stages #2 and #3 */
-		if (i + 1 < active_nr) {
-			struct cache_entry *e2 = active_cache[i];
-			struct cache_entry *e3 = active_cache[i + 1];
-			if (ce_stage(e2) == 2 &&
-			    ce_stage(e3) == 3 &&
-			    ce_same_name(e, e3) &&
-			    S_ISREG(e2->ce_mode) &&
-			    S_ISREG(e3->ce_mode))
-				it->util = (void *) 1;
-		}
-
-		/* Skip the entries with the same name */
-		while (i < active_nr && ce_same_name(e, active_cache[i]))
-			i++;
-		i--; /* compensate for the outer loop */
+		i = check_one_conflict(i, &conflict_type);
+		if (conflict_type == THREE_STAGED)
+			string_list_insert(conflict, (const char *)e->name);
 	}
 	return 0;
 }
 
-void rerere_remaining(struct string_list *merge_rr)
+int rerere_remaining(struct string_list *merge_rr)
 {
 	int i;
-	struct string_list conflict = STRING_LIST_INIT_DUP;
-
-	/* Add punted paths */
-	find_conflict(&conflict);
-	for (i = 0; i < conflict.nr; i++) {
-		if (!conflict.items[i].util)
-			string_list_insert(merge_rr, conflict.items[i].string);
-	}
+	if (read_cache() < 0)
+		return error("Could not read index");
 
-	/* Mark already resolved paths */
-	for (i = 0; i < active_nr; i++) {
+	for (i = 0; i < active_nr;) {
+		int conflict_type;
 		struct cache_entry *e = active_cache[i];
-
-		if (!ce_stage(e)) {
+		i = check_one_conflict(i, &conflict_type);
+		if (conflict_type == PUNTED)
+			string_list_insert(merge_rr, (const char *)e->name);
+		else if (conflict_type == RESOLVED) {
 			struct string_list_item *it;
 			it = string_list_lookup(merge_rr, (const char *)e->name);
 			if (it != NULL) {
 				free(it->util);
 				it->util = RERERE_RESOLVED;
 			}
-			continue;
 		}
 	}
+	return 0;
 }
 
 static int merge(const char *name, const char *path)
@@ -504,8 +511,6 @@ static int do_plain_rerere(struct string_list *rr, int fd)
 
 	for (i = 0; i < conflict.nr; i++) {
 		const char *path = conflict.items[i].string;
-		if (!conflict.items[i].util)
-			continue; /* punted */
 		if (!string_list_has_string(rr, path)) {
 			unsigned char sha1[20];
 			char *hex;
@@ -533,8 +538,6 @@ static int do_plain_rerere(struct string_list *rr, int fd)
 		const char *path = rr->items[i].string;
 		const char *name = (const char *)rr->items[i].util;
 
-		if (!name)
-			continue;
 		if (has_rerere_resolution(name)) {
 			if (!merge(name, path)) {
 				if (rerere_autoupdate)
@@ -664,8 +667,6 @@ int rerere_forget(const char **pathspec)
 	find_conflict(&conflict);
 	for (i = 0; i < conflict.nr; i++) {
 		struct string_list_item *it = &conflict.items[i];
-		if (!conflict.items[i].util)
-			continue; /* punted */
 		if (!match_pathspec(pathspec, it->string, strlen(it->string),
 				    0, NULL))
 			continue;
diff --git a/rerere.h b/rerere.h
index ea2618b..595f49f 100644
--- a/rerere.h
+++ b/rerere.h
@@ -18,7 +18,7 @@ extern int rerere(int);
 extern const char *rerere_path(const char *hex, const char *file);
 extern int has_rerere_resolution(const char *hex);
 extern int rerere_forget(const char **);
-extern void rerere_remaining(struct string_list *);
+extern int rerere_remaining(struct string_list *);
 
 #define OPT_RERERE_AUTOUPDATE(v) OPT_UYN(0, "rerere-autoupdate", (v), \
 	"update the index with reused conflict resolution if possible")
-- 
1.7.4.rc2.33.g8a14f

← back to recent threads