git mergetool broken when rerere active

git mergetool broken when rerere active

[Martin von Zweigbergk]

Hi,

When rerere is enabled, git mergetool uses 'git rerere status' to find
out which files to run the merge tool on. This was introduced in
bb0a484 (mergetool: Skip autoresolved paths, 2010-08-17). Before that,
'git ls-files -u' was used, whether or not rerere was active.

This change caused two problems:

 (1) Before this change, it used to be that case that all conflicts
     would be resolved and added to the index after running 'git
     mergetool' without arguments, i.e. on all files. After the
     change, conflicts of type 'deleted by them' or 'deleted by us'
     would be ignored, since they are not listed shown by 'git rerere
     status'. Previously, git mergetool would ask whether to pick the
     modified file or to delete the file.

 (2) When running mergetool again after resolving some (or all)
     conflicts, so that some of the files have already been added to
     the index, mergetool will now print something like

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

     Before the change, any files that were already added to the index
     would just be skipped, without mergetool asking the user whether
     to continue.

I would like to have both the original properties in (1) and (2) back,
i.e. being ready for commit once 'git mergetool' has been successfully
completed, and having it ignore any files that have already been added
to the index.

I was reading the original thread [1], but I didn't quite understand
why just enabling rerere.autoupdate would not solve the problem. Maybe
it was just that the goal was a solution that works even with
rerere.autoupdate disabled? Can we fix it in some way by combining the
output of 'git rerere status' and 'git ls-files -u'?


Regards,
Martin

[1] http://thread.gmane.org/gmane.comp.version-control.git/153420

Re: git mergetool broken when rerere active

[Martin von Zweigbergk]

On Wed, 5 Jan 2011, Martin von Zweigbergk wrote:

>  (2) When running mergetool again after resolving some (or all)
>      conflicts, so that some of the files have already been added to
>      the index, mergetool will now print something like
> 
>      file1: file does not need merging
>      Continue merging other unresolved paths (y/n) ?
> 

The same problem should arise even the first time mergetool if run if
rerere autoupdate is active and has added some files to the index.

> I would like to have both the original properties in (1) and (2) back,
> i.e. being ready for commit once 'git mergetool' has been successfully
> completed, and having it ignore any files that have already been added
> to the index.

Well, if rerere.autoupdate is disabled, any files resolved by rerere
should not be touched by 'git mergetool' (this was the original
request that lead to the change), but they would not be added to the
index, so we would not be ready for commit. I take that part of my
request back. I guess it would be ok (or even necessary) if, after
resolving all the conflicts reported by 'git mergetool', the user
would still have to explicitly add any files resolved by rerere and
thus ignored by 'git mergetool'.

So I guess what I'm saying here, is that I think only the problem with
'delete/modify' conflicts should be fixed regarding issue (1).

Re: git mergetool broken when rerere active

[Junio C Hamano]

Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:

> When rerere is enabled, git mergetool uses 'git rerere status' to find
> out which files to run the merge tool on. This was introduced in
> bb0a484 (mergetool: Skip autoresolved paths, 2010-08-17). Before that,
> 'git ls-files -u' was used, whether or not rerere was active.
>
> This change caused two problems:
>
>  (1) Before this change, it used to be that case that all conflicts
>      would be resolved and added to the index after running 'git
>      mergetool' without arguments, i.e. on all files. After the
>      change, conflicts of type 'deleted by them' or 'deleted by us'
>      would be ignored, since they are not listed shown by 'git rerere
>      status'.

Good point.  We used to say "everything that had conflict after a mergy
operation", now we say "everything that rerere attempted resolution but
didn't succeed".  Missing are paths that rerere didn't even attempt to
apply previous resolution at all.

Probably we would need a "git rerere remaining" sobcommand that is similar
to status but also includes the "punted" paths.

Re: git mergetool broken when rerere active

[Junio C Hamano]

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

> Probably we would need a "git rerere remaining" sobcommand that is similar
> to status but also includes the "punted" paths.

... which may look like this.

Replace use of "git rerere status" in bb0a484 (mergetool: Skip
autoresolved paths, 2010-08-17) with "git rerere remainder" and see what
happens.

 builtin/rerere.c |   12 +++++++++-
 rerere.c         |   63 +++++++++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 64 insertions(+), 11 deletions(-)

diff --git a/builtin/rerere.c b/builtin/rerere.c
index 642bf35..081fccc 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,
 };
 
@@ -147,6 +147,8 @@ 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);
 		}
@@ -154,12 +156,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++) {
+			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"))
 		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/rerere.c b/rerere.c
index d260843..eb47f97 100644
--- a/rerere.c
+++ b/rerere.c
@@ -350,21 +350,57 @@ static int find_conflict(struct string_list *conflict)
 	int i;
 	if (read_cache() < 0)
 		return error("Could not read index");
-	for (i = 0; i+1 < active_nr; i++) {
-		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(e2, 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 */
+
+	/*
+	 * Collect paths with conflict, mark them with NULL (punted) or
+	 * !NULL (eligible) 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))
+			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 */
 	}
 	return 0;
 }
 
+static void add_punted(struct string_list *merge_rr)
+{
+	int i;
+	struct string_list conflict = STRING_LIST_INIT_DUP;
+
+	find_conflict(&conflict);
+	for (i = 0; i < conflict.nr; i++) {
+		if (conflict.items[i].util)
+			continue;
+		string_list_insert(merge_rr, conflict.items[i].string);
+	}
+}
+
 static int merge(const char *name, const char *path)
 {
 	int ret;
@@ -451,6 +487,8 @@ 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;
@@ -478,6 +516,8 @@ 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)
@@ -552,6 +592,7 @@ 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;
 }
 
@@ -607,6 +648,8 @@ 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;

Re: git mergetool broken when rerere active

[Martin von Zweigbergk]

On Thu, Jan 6, 2011 at 2:33 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Probably we would need a "git rerere remaining" sobcommand that is similar
>> to status but also includes the "punted" paths.
>
> ... which may look like this.

Thanks! Will have a look at this tonight when I get home.

Re: git mergetool broken when rerere active

[Martin von Zweigbergk]

On Thu, 6 Jan 2011, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Probably we would need a "git rerere remaining" sobcommand that is similar
> > to status but also includes the "punted" paths.
> 
> ... which may look like this.
> 
> Replace use of "git rerere status" in bb0a484 (mergetool: Skip
> autoresolved paths, 2010-08-17) with "git rerere remainder" and see what
> happens.

I think it almost works. The only thing I can see that is not handled
is the case where a file that had conflicts that have now been
resolved and the file has been added to the index. These files still
show up in 'git rerere remaining' and thus in 'git mergetool'. It is
of course possible in mergetool to calculate the set difference
between `git rerere remaining` and the set of added files, but would
that be the right thing to do? Should 'git rerere remaining' rather be
changed?

Would it be easier to add a 'git rerere resolved' that lists the files
for which all conflicts have been resolved by rerere?x We could then
make mergetool use the usual list of files from 'git ls-files -u' and
subtract these files.