{"thread":{"id":"26212","subject":"git mergetool broken when rerere active","startedAt":"2011-01-06T03:39:07Z","lastAt":"2011-01-07T02:50:00Z","messageCount":6,"participants":["Martin von Zweigbergk","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"159018","messageId":"alpine.DEB.1.10.1101052119530.26654@debian","threadId":"26212","inReplyTo":null,"subject":"git mergetool broken when rerere active","fromName":"Martin von Zweigbergk","fromEmail":"martin.von.zweigbergk@gmail.com","sentAt":"2011-01-06T03:39:07Z","receivedAt":"2011-01-06T03:39:07Z","isPatch":false,"sender":{"key":"martinvonz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/891642?v=4"},"body":"Hi,\n\nWhen rerere is enabled, git mergetool uses 'git rerere status' to find\nout which files to run the merge tool on. This was introduced in\nbb0a484 (mergetool: Skip autoresolved paths, 2010-08-17). Before that,\n'git ls-files -u' was used, whether or not rerere was active.\n\nThis change caused two problems:\n\n (1) Before this change, it used to be that case that all conflicts\n     would be resolved and added to the index after running 'git\n     mergetool' without arguments, i.e. on all files. After the\n     change, conflicts of type 'deleted by them' or 'deleted by us'\n     would be ignored, since they are not listed shown by 'git rerere\n     status'. Previously, git mergetool would ask whether to pick the\n     modified file or to delete the file.\n\n (2) When running mergetool again after resolving some (or all)\n     conflicts, so that some of the files have already been added to\n     the index, mergetool will now print something like\n\n     file1: file does not need merging\n     Continue merging other unresolved paths (y/n) ?\n\n     Before the change, any files that were already added to the index\n     would just be skipped, without mergetool asking the user whether\n     to continue.\n\nI would like to have both the original properties in (1) and (2) back,\ni.e. being ready for commit once 'git mergetool' has been successfully\ncompleted, and having it ignore any files that have already been added\nto the index.\n\nI was reading the original thread [1], but I didn't quite understand\nwhy just enabling rerere.autoupdate would not solve the problem. Maybe\nit was just that the goal was a solution that works even with\nrerere.autoupdate disabled? Can we fix it in some way by combining the\noutput of 'git rerere status' and 'git ls-files -u'?\n\n\nRegards,\nMartin\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/153420\n"},{"id":"159026","messageId":"alpine.DEB.1.10.1101060634480.26654@debian","threadId":"26212","inReplyTo":"alpine.DEB.1.10.1101052119530.26654@debian","subject":"Re: git mergetool broken when rerere active","fromName":"Martin von Zweigbergk","fromEmail":"martin.von.zweigbergk@gmail.com","sentAt":"2011-01-06T12:47:28Z","receivedAt":"2011-01-06T12:47:28Z","isPatch":false,"sender":{"key":"martinvonz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/891642?v=4"},"body":"On Wed, 5 Jan 2011, Martin von Zweigbergk wrote:\n\n>  (2) When running mergetool again after resolving some (or all)\n>      conflicts, so that some of the files have already been added to\n>      the index, mergetool will now print something like\n> \n>      file1: file does not need merging\n>      Continue merging other unresolved paths (y/n) ?\n> \n\nThe same problem should arise even the first time mergetool if run if\nrerere autoupdate is active and has added some files to the index.\n\n> I would like to have both the original properties in (1) and (2) back,\n> i.e. being ready for commit once 'git mergetool' has been successfully\n> completed, and having it ignore any files that have already been added\n> to the index.\n\nWell, if rerere.autoupdate is disabled, any files resolved by rerere\nshould not be touched by 'git mergetool' (this was the original\nrequest that lead to the change), but they would not be added to the\nindex, so we would not be ready for commit. I take that part of my\nrequest back. I guess it would be ok (or even necessary) if, after\nresolving all the conflicts reported by 'git mergetool', the user\nwould still have to explicitly add any files resolved by rerere and\nthus ignored by 'git mergetool'.\n\nSo I guess what I'm saying here, is that I think only the problem with\n'delete/modify' conflicts should be fixed regarding issue (1).\n"},{"id":"159050","messageId":"7vbp3tc142.fsf@alter.siamese.dyndns.org","threadId":"26212","inReplyTo":"alpine.DEB.1.10.1101052119530.26654@debian","subject":"Re: git mergetool broken when rerere active","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-01-06T18:56:29Z","receivedAt":"2011-01-06T18:56:29Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:\n\n> When rerere is enabled, git mergetool uses 'git rerere status' to find\n> out which files to run the merge tool on. This was introduced in\n> bb0a484 (mergetool: Skip autoresolved paths, 2010-08-17). Before that,\n> 'git ls-files -u' was used, whether or not rerere was active.\n>\n> This change caused two problems:\n>\n>  (1) Before this change, it used to be that case that all conflicts\n>      would be resolved and added to the index after running 'git\n>      mergetool' without arguments, i.e. on all files. After the\n>      change, conflicts of type 'deleted by them' or 'deleted by us'\n>      would be ignored, since they are not listed shown by 'git rerere\n>      status'.\n\nGood point.  We used to say \"everything that had conflict after a mergy\noperation\", now we say \"everything that rerere attempted resolution but\ndidn't succeed\".  Missing are paths that rerere didn't even attempt to\napply previous resolution at all.\n\nProbably we would need a \"git rerere remaining\" sobcommand that is similar\nto status but also includes the \"punted\" paths.\n"},{"id":"159053","messageId":"7v62u1bzeg.fsf@alter.siamese.dyndns.org","threadId":"26212","inReplyTo":"7vbp3tc142.fsf@alter.siamese.dyndns.org","subject":"Re: git mergetool broken when rerere active","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-01-06T19:33:27Z","receivedAt":"2011-01-06T19:33:27Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Probably we would need a \"git rerere remaining\" sobcommand that is similar\n> to status but also includes the \"punted\" paths.\n\n... which may look like this.\n\nReplace use of \"git rerere status\" in bb0a484 (mergetool: Skip\nautoresolved paths, 2010-08-17) with \"git rerere remainder\" and see what\nhappens.\n\n builtin/rerere.c |   12 +++++++++-\n rerere.c         |   63 +++++++++++++++++++++++++++++++++++++++++++++--------\n 2 files changed, 64 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/rerere.c b/builtin/rerere.c\nindex 642bf35..081fccc 100644\n--- a/builtin/rerere.c\n+++ b/builtin/rerere.c\n@@ -8,7 +8,7 @@\n #include \"xdiff-interface.h\"\n \n static const char * const rerere_usage[] = {\n-\t\"git rerere [clear | status | diff | gc]\",\n+\t\"git rerere [clear | status | remaining | diff | gc]\",\n \tNULL,\n };\n \n@@ -147,6 +147,8 @@ int cmd_rerere(int argc, const char **argv, const char *prefix)\n \tif (!strcmp(argv[0], \"clear\")) {\n \t\tfor (i = 0; i < merge_rr.nr; i++) {\n \t\t\tconst char *name = (const char *)merge_rr.items[i].util;\n+\t\t\tif (!name)\n+\t\t\t\tcontinue;\n \t\t\tif (!has_rerere_resolution(name))\n \t\t\t\tunlink_rr_item(name);\n \t\t}\n@@ -154,12 +156,20 @@ int cmd_rerere(int argc, const char **argv, const char *prefix)\n \t} else if (!strcmp(argv[0], \"gc\"))\n \t\tgarbage_collect(&merge_rr);\n \telse if (!strcmp(argv[0], \"status\"))\n+\t\tfor (i = 0; i < merge_rr.nr; i++) {\n+\t\t\tif (!merge_rr.items[i].util)\n+\t\t\t\tcontinue;\n+\t\t\tprintf(\"%s\\n\", merge_rr.items[i].string);\n+\t\t}\n+\telse if (!strcmp(argv[0], \"remaining\"))\n \t\tfor (i = 0; i < merge_rr.nr; i++)\n \t\t\tprintf(\"%s\\n\", merge_rr.items[i].string);\n \telse if (!strcmp(argv[0], \"diff\"))\n \t\tfor (i = 0; i < merge_rr.nr; i++) {\n \t\t\tconst char *path = merge_rr.items[i].string;\n \t\t\tconst char *name = (const char *)merge_rr.items[i].util;\n+\t\t\tif (!name)\n+\t\t\t\tcontinue;\n \t\t\tdiff_two(rerere_path(name, \"preimage\"), path, path, path);\n \t\t}\n \telse\ndiff --git a/rerere.c b/rerere.c\nindex d260843..eb47f97 100644\n--- a/rerere.c\n+++ b/rerere.c\n@@ -350,21 +350,57 @@ static int find_conflict(struct string_list *conflict)\n \tint i;\n \tif (read_cache() < 0)\n \t\treturn error(\"Could not read index\");\n-\tfor (i = 0; i+1 < active_nr; i++) {\n-\t\tstruct cache_entry *e2 = active_cache[i];\n-\t\tstruct cache_entry *e3 = active_cache[i+1];\n-\t\tif (ce_stage(e2) == 2 &&\n-\t\t    ce_stage(e3) == 3 &&\n-\t\t    ce_same_name(e2, e3) &&\n-\t\t    S_ISREG(e2->ce_mode) &&\n-\t\t    S_ISREG(e3->ce_mode)) {\n-\t\t\tstring_list_insert(conflict, (const char *)e2->name);\n-\t\t\ti++; /* skip over both #2 and #3 */\n+\n+\t/*\n+\t * Collect paths with conflict, mark them with NULL (punted) or\n+\t * !NULL (eligible) in their ->util field.\n+\t */\n+\tfor (i = 0; i < active_nr; i++) {\n+\t\tstruct cache_entry *e = active_cache[i];\n+\t\tstruct string_list_item *it;\n+\n+\t\tif (!ce_stage(e))\n+\t\t\tcontinue;\n+\t\tit = string_list_insert(conflict, (const char *)e->name);\n+\t\tit->util = NULL;\n+\t\tif (ce_stage(e) == 1) {\n+\t\t\tif (active_nr <= ++i)\n+\t\t\t\tbreak;\n \t\t}\n+\n+\t\t/* Only handle regular files with both stages #2 and #3 */\n+\t\tif (i + 1 < active_nr) {\n+\t\t\tstruct cache_entry *e2 = active_cache[i];\n+\t\t\tstruct cache_entry *e3 = active_cache[i + 1];\n+\t\t\tif (ce_stage(e2) == 2 &&\n+\t\t\t    ce_stage(e3) == 3 &&\n+\t\t\t    ce_same_name(e, e3) &&\n+\t\t\t    S_ISREG(e2->ce_mode) &&\n+\t\t\t    S_ISREG(e3->ce_mode))\n+\t\t\t\tit->util = (void *) 1;\n+\t\t}\n+\n+\t\t/* Skip the entries with the same name */\n+\t\twhile (i < active_nr && ce_same_name(e, active_cache[i]))\n+\t\t\ti++;\n+\t\ti--; /* compensate for the outer loop */\n \t}\n \treturn 0;\n }\n \n+static void add_punted(struct string_list *merge_rr)\n+{\n+\tint i;\n+\tstruct string_list conflict = STRING_LIST_INIT_DUP;\n+\n+\tfind_conflict(&conflict);\n+\tfor (i = 0; i < conflict.nr; i++) {\n+\t\tif (conflict.items[i].util)\n+\t\t\tcontinue;\n+\t\tstring_list_insert(merge_rr, conflict.items[i].string);\n+\t}\n+}\n+\n static int merge(const char *name, const char *path)\n {\n \tint ret;\n@@ -451,6 +487,8 @@ static int do_plain_rerere(struct string_list *rr, int fd)\n \n \tfor (i = 0; i < conflict.nr; i++) {\n \t\tconst char *path = conflict.items[i].string;\n+\t\tif (!conflict.items[i].util)\n+\t\t\tcontinue; /* punted */\n \t\tif (!string_list_has_string(rr, path)) {\n \t\t\tunsigned char sha1[20];\n \t\t\tchar *hex;\n@@ -478,6 +516,8 @@ static int do_plain_rerere(struct string_list *rr, int fd)\n \t\tconst char *path = rr->items[i].string;\n \t\tconst char *name = (const char *)rr->items[i].util;\n \n+\t\tif (!name)\n+\t\t\tcontinue;\n \t\tif (has_rerere_resolution(name)) {\n \t\t\tif (!merge(name, path)) {\n \t\t\t\tif (rerere_autoupdate)\n@@ -552,6 +592,7 @@ int setup_rerere(struct string_list *merge_rr, int flags)\n \tfd = hold_lock_file_for_update(&write_lock, merge_rr_path,\n \t\t\t\t       LOCK_DIE_ON_ERROR);\n \tread_rr(merge_rr);\n+\tadd_punted(merge_rr);\n \treturn fd;\n }\n \n@@ -607,6 +648,8 @@ int rerere_forget(const char **pathspec)\n \tfind_conflict(&conflict);\n \tfor (i = 0; i < conflict.nr; i++) {\n \t\tstruct string_list_item *it = &conflict.items[i];\n+\t\tif (!conflict.items[i].util)\n+\t\t\tcontinue; /* punted */\n \t\tif (!match_pathspec(pathspec, it->string, strlen(it->string),\n \t\t\t\t    0, NULL))\n \t\t\tcontinue;\n"},{"id":"159057","messageId":"AANLkTimOzwaw6VN+NPfCJPH9t0G64r=WrffNpfHQ0h-F@mail.gmail.com","threadId":"26212","inReplyTo":"7v62u1bzeg.fsf@alter.siamese.dyndns.org","subject":"Re: git mergetool broken when rerere active","fromName":"Martin von Zweigbergk","fromEmail":"martin.von.zweigbergk@gmail.com","sentAt":"2011-01-06T19:51:34Z","receivedAt":"2011-01-06T19:51:34Z","isPatch":false,"sender":{"key":"martinvonz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/891642?v=4"},"body":"On Thu, Jan 6, 2011 at 2:33 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> Probably we would need a \"git rerere remaining\" sobcommand that is similar\n>> to status but also includes the \"punted\" paths.\n>\n> ... which may look like this.\n\nThanks! Will have a look at this tonight when I get home.\n"},{"id":"159091","messageId":"alpine.DEB.1.10.1101062058380.3800@debian","threadId":"26212","inReplyTo":"7v62u1bzeg.fsf@alter.siamese.dyndns.org","subject":"Re: git mergetool broken when rerere active","fromName":"Martin von Zweigbergk","fromEmail":"martin.von.zweigbergk@gmail.com","sentAt":"2011-01-07T02:50:00Z","receivedAt":"2011-01-07T02:50:00Z","isPatch":false,"sender":{"key":"martinvonz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/891642?v=4"},"body":"On Thu, 6 Jan 2011, Junio C Hamano wrote:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > Probably we would need a \"git rerere remaining\" sobcommand that is similar\n> > to status but also includes the \"punted\" paths.\n> \n> ... which may look like this.\n> \n> Replace use of \"git rerere status\" in bb0a484 (mergetool: Skip\n> autoresolved paths, 2010-08-17) with \"git rerere remainder\" and see what\n> happens.\n\nI think it almost works. The only thing I can see that is not handled\nis the case where a file that had conflicts that have now been\nresolved and the file has been added to the index. These files still\nshow up in 'git rerere remaining' and thus in 'git mergetool'. It is\nof course possible in mergetool to calculate the set difference\nbetween `git rerere remaining` and the set of added files, but would\nthat be the right thing to do? Should 'git rerere remaining' rather be\nchanged?\n\nWould it be easier to add a 'git rerere resolved' that lists the files\nfor which all conflicts have been resolved by rerere?x We could then\nmake mergetool use the usual list of files from 'git ls-files -u' and\nsubtract these files.\n"}]}