{"thread":{"id":"60034","subject":"[PATCH 1/2] resolve-undo: allow resurrecting conflicted state that resolved to deletion","startedAt":"2023-07-27T21:46:30Z","lastAt":"2023-07-28T19:49:28Z","messageCount":3,"participants":["Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"479944","messageId":"xmqq3519auz5.fsf@gitster.g","threadId":"60034","inReplyTo":null,"subject":"[PATCH 1/2] resolve-undo: allow resurrecting conflicted state that resolved to deletion","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-27T21:46:22Z","receivedAt":"2023-07-27T21:46:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The resolve-undo index extension records up to three (mode, object\nname) tuples for non-zero stages for each path that was resolved,\nto be used to recreate the original conflicted state later when the\nuser requests.\n\nThe unmerge_index_entry_at() function uses the resolve-undo data to\ndo so, but it assumes that the path for which the conflicted state\nneeds to be recreated can be specified by the position in the\nactive_cache[] array.  This obviously cannot salvage a conflicted\nstate that was resolved by removing them.  A delete-modify conflict,\nin which the change whose \"modify\" side made is a trivial typofix,\nmay legitimately be resolved to remove the path, and resolve-undo\nextension does record the two (mode, object name) tuples for the\ncommon ancestor version and their version, lacking our version.\n\nIntroduce a new unmerge_index_entry() helper function that takes the\npath (which does not necessarily have to exist in the active_cache[]\narray) and resolve-undo data, and use it to reimplement unmerge_index()\npublic function that is used by \"git rerere\".\n\nThe limited interface is still kept for now, as it is used by \"git\ncheckout -m\" and \"git update-index --unmerge\", but these two codepaths\nwill be updated to lift the assumption to allow conflicts that resolved\nto deletion can be recreated.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * No new tests, as unmerge_index() that is used by \"git rerere\"\n   already exercises the codepath.  We'll use the new function also\n   in \"update-index\" in a future patch.\n\n resolve-undo.c | 46 +++++++++++++++++++++++++++++++++++++++++-----\n resolve-undo.h |  1 +\n 2 files changed, 42 insertions(+), 5 deletions(-)\n\ndiff --git a/resolve-undo.c b/resolve-undo.c\nindex 7817f5d6db..9ceab129ac 100644\n--- a/resolve-undo.c\n+++ b/resolve-undo.c\n@@ -184,19 +184,55 @@ void unmerge_marked_index(struct index_state *istate)\n \t}\n }\n \n+int unmerge_index_entry(struct index_state *istate, const char *path,\n+\t\t\tstruct resolve_undo_info *ru)\n+{\n+\tint i = index_name_pos(istate, path, strlen(path));\n+\n+\tif (i < 0) {\n+\t\t/* unmerged? */\n+\t\ti = -i - 1;\n+\t\tif (i < istate->cache_nr &&\n+\t\t    !strcmp(istate->cache[i]->name, path))\n+\t\t\t/* yes, it is already unmerged */\n+\t\t\treturn 0;\n+\t\t/* fallthru: resolved to removal */\n+\t} else {\n+\t\t/* merged - remove it to replace it with unmerged entries */\n+\t\tremove_index_entry_at(istate, i);\n+\t}\n+\n+\tfor (i = 0; i < 3; i++) {\n+\t\tstruct cache_entry *ce;\n+\t\tif (!ru->mode[i])\n+\t\t\tcontinue;\n+\t\tce = make_cache_entry(istate, ru->mode[i], &ru->oid[i],\n+\t\t\t\t      path, i + 1, 0);\n+\t\tif (add_index_entry(istate, ce, ADD_CACHE_OK_TO_ADD))\n+\t\t\treturn error(\"cannot unmerge '%s'\", path);\n+\t}\n+\treturn 0;\n+}\n+\n void unmerge_index(struct index_state *istate, const struct pathspec *pathspec)\n {\n-\tint i;\n+\tstruct string_list_item *item;\n \n \tif (!istate->resolve_undo)\n \t\treturn;\n \n \t/* TODO: audit for interaction with sparse-index. */\n \tensure_full_index(istate);\n-\tfor (i = 0; i < istate->cache_nr; i++) {\n-\t\tconst struct cache_entry *ce = istate->cache[i];\n-\t\tif (!ce_path_match(istate, ce, pathspec, NULL))\n+\n+\tfor_each_string_list_item(item, istate->resolve_undo) {\n+\t\tconst char *path = item->string;\n+\t\tstruct resolve_undo_info *ru = item->util;\n+\t\tif (!item->util)\n+\t\t\tcontinue;\n+\t\tif (!match_pathspec(istate, pathspec,\n+\t\t\t\t    item->string, strlen(item->string),\n+\t\t\t\t    0, NULL, 0))\n \t\t\tcontinue;\n-\t\ti = unmerge_index_entry_at(istate, i);\n+\t\tunmerge_index_entry(istate, path, ru);\n \t}\n }\ndiff --git a/resolve-undo.h b/resolve-undo.h\nindex c5deafc92f..1ae321c88b 100644\n--- a/resolve-undo.h\n+++ b/resolve-undo.h\n@@ -18,6 +18,7 @@ void resolve_undo_write(struct strbuf *, struct string_list *);\n struct string_list *resolve_undo_read(const char *, unsigned long);\n void resolve_undo_clear_index(struct index_state *);\n int unmerge_index_entry_at(struct index_state *, int);\n+int unmerge_index_entry(struct index_state *, const char *, struct resolve_undo_info *);\n void unmerge_index(struct index_state *, const struct pathspec *);\n void unmerge_marked_index(struct index_state *);\n \n-- \n2.41.0-459-gb4fce4b6e4\n\n"},{"id":"479945","messageId":"xmqqtttp9g6r.fsf@gitster.g","threadId":"60034","inReplyTo":"xmqq3519auz5.fsf@gitster.g","subject":"[PATCH 2/2] update-index: use unmerge_index_entry() to support removal","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-27T21:51:08Z","receivedAt":"2023-07-27T21:51:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"update-index --unresolve\" used the unmerge_index_entry_at() that\nassumed that the path to be unresolved must be in the index, which\nmade it impossible to unresolve a path that was resolved as removal.\n\nRewrite unresolve_one() to use the unmerge_index_entry() to support\nunresolving such a path.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n * Now getting rid of unmerge_index_entry_at() is a bigger task.  The\n   only remaining user of it is the \"checkout -m -- $path\" codepath.\n\n builtin/update-index.c    | 33 +++++++++++++++++----------------\n t/t2030-unresolve-info.sh | 29 +++++++++++++++++++++++++----\n 2 files changed, 42 insertions(+), 20 deletions(-)\n\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex 853ec9eb7a..d02ac55313 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -661,26 +661,26 @@ static int unresolve_one(const char *path)\n \tint pos;\n \tint ret = 0;\n \tstruct cache_entry *ce_2 = NULL, *ce_3 = NULL;\n+\tstruct resolve_undo_info *ru = NULL;\n+\n+\tif (the_index.resolve_undo) {\n+\t\tstruct string_list_item *item;\n+\t\titem = string_list_lookup(the_index.resolve_undo, path);\n+\t\tif (item)\n+\t\t\tru = item->util;\n+\t}\n+\n+\t/* resolve-undo record exists for the path */\n+\tif (ru)\n+\t\treturn unmerge_index_entry(&the_index, path, ru);\n \n \t/* See if there is such entry in the index. */\n \tpos = index_name_pos(&the_index, path, namelen);\n \tif (0 <= pos) {\n-\t\t/* already merged */\n-\t\tpos = unmerge_index_entry_at(&the_index, pos);\n-\t\tif (pos < the_index.cache_nr) {\n-\t\t\tconst struct cache_entry *ce = the_index.cache[pos];\n-\t\t\tif (ce_stage(ce) &&\n-\t\t\t    ce_namelen(ce) == namelen &&\n-\t\t\t    !memcmp(ce->name, path, namelen))\n-\t\t\t\treturn 0;\n-\t\t}\n-\t\t/* no resolve-undo information; fall back */\n+\t\t; /* resolve-undo record was used already -- fall back */\n \t} else {\n-\t\t/* If there isn't, either it is unmerged, or\n-\t\t * resolved as \"removed\" by mistake.  We do not\n-\t\t * want to do anything in the former case.\n-\t\t */\n-\t\tpos = -pos-1;\n+\t\t/* Is it unmerged? */\n+\t\tpos = -pos - 1;\n \t\tif (pos < the_index.cache_nr) {\n \t\t\tconst struct cache_entry *ce = the_index.cache[pos];\n \t\t\tif (ce_namelen(ce) == namelen &&\n@@ -688,9 +688,10 @@ static int unresolve_one(const char *path)\n \t\t\t\tfprintf(stderr,\n \t\t\t\t\t\"%s: skipping still unmerged path.\\n\",\n \t\t\t\t\tpath);\n-\t\t\t\tgoto free_return;\n \t\t\t}\n+\t\t\tgoto free_return;\n \t\t}\n+\t\t/* No, such a path does not exist -- removed */\n \t}\n \n \t/*\ndiff --git a/t/t2030-unresolve-info.sh b/t/t2030-unresolve-info.sh\nindex d4e7760df5..6a3af64e0f 100755\n--- a/t/t2030-unresolve-info.sh\n+++ b/t/t2030-unresolve-info.sh\n@@ -37,11 +37,17 @@ prime_resolve_undo () {\n \tgit checkout second^0 &&\n \ttest_tick &&\n \ttest_must_fail git merge third^0 &&\n-\techo merge does not leave anything &&\n \tcheck_resolve_undo empty &&\n-\techo different >fi/le &&\n-\tgit add fi/le &&\n-\techo resolving records &&\n+\n+\t# how should the conflict be resolved?\n+\tcase \"$1\" in\n+\tremove)\n+\t\trm -f file/le && git rm fi/le\n+\t\t;;\n+\t*) # modify\n+\t\techo different >fi/le && git add fi/le\n+\t\t;;\n+\tesac\n \tcheck_resolve_undo recorded fi/le initial:fi/le second:fi/le third:fi/le\n }\n \n@@ -134,6 +140,21 @@ test_expect_success 'unmerge can be done even after committing' '\n \ttest_line_count = 3 actual\n '\n \n+test_expect_success 'unmerge removal' '\n+\tprime_resolve_undo remove &&\n+\tgit update-index --unresolve fi/le &&\n+\tgit ls-files -u actual &&\n+\ttest_line_count = 3 actual\n+'\n+\n+test_expect_success 'unmerge removal after committing' '\n+\tprime_resolve_undo remove &&\n+\tgit commit -m \"record to nuke MERGE_HEAD\" &&\n+\tgit update-index --unresolve fi/le &&\n+\tgit ls-files -u actual &&\n+\ttest_line_count = 3 actual\n+'\n+\n test_expect_success 'rerere and rerere forget' '\n \tmkdir .git/rr-cache &&\n \tprime_resolve_undo &&\n-- \n2.41.0-459-gb4fce4b6e4\n\n"},{"id":"479974","messageId":"xmqqo7jv4y0t.fsf_-_@gitster.g","threadId":"60034","inReplyTo":"xmqqtttp9g6r.fsf@gitster.g","subject":"[PATCH 3/2] update-index: remove stale fallback code for \"--unresolve\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-28T19:49:22Z","receivedAt":"2023-07-28T19:49:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The \"update-index --unresolve\" is a relatively old feature that was\nintroduced in Git v1.4.1 (June 2006), which predates the\nresolve-undo extension introduced in Git v1.7.0 (February 2010).\nThe original code that was limited only to work during a merge (and\nnot during a rebase or a cherry-pick) has been kept as the fallback\ncodepath to be used as a transition measure.\n\nBy now, for more than 10 years we have stored resolve-undo extension\nin the index file, and the fallback code way outlived its usefulness.\n\nRemove it, together with two file-scope static global variables.\nOne of these variables is still used by surviving function, but it\ndoes not have to be a global at all, so move it to local to that\nfunction.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/update-index.c | 103 ++++-------------------------------------\n 1 file changed, 8 insertions(+), 95 deletions(-)\n\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex d02ac55313..79c25a2a58 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -609,9 +609,6 @@ static const char * const update_index_usage[] = {\n \tNULL\n };\n \n-static struct object_id head_oid;\n-static struct object_id merge_head_oid;\n-\n static struct cache_entry *read_one_ent(const char *which,\n \t\t\t\t\tstruct object_id *ent, const char *path,\n \t\t\t\t\tint namelen, int stage)\n@@ -640,102 +637,17 @@ static struct cache_entry *read_one_ent(const char *which,\n \treturn ce;\n }\n \n-static int read_head_pointers(void)\n-{\n-\tstatic int result = -2; /* unknown yet */\n-\n-\tif (result == -2) {\n-\t\tresult = -1;\n-\t\tif (read_ref(\"HEAD\", &head_oid))\n-\t\t\treturn error(\"No HEAD -- no initial comit yet?\");\n-\t\tif (read_ref(\"MERGE_HEAD\", &merge_head_oid))\n-\t\t\treturn error(\"Not in the middle of a merge\");\n-\t\tresult = 0;\n-\t}\n-\treturn result;\n-}\n-\n static int unresolve_one(const char *path)\n {\n-\tint namelen = strlen(path);\n-\tint pos;\n-\tint ret = 0;\n-\tstruct cache_entry *ce_2 = NULL, *ce_3 = NULL;\n-\tstruct resolve_undo_info *ru = NULL;\n-\n-\tif (the_index.resolve_undo) {\n-\t\tstruct string_list_item *item;\n-\t\titem = string_list_lookup(the_index.resolve_undo, path);\n-\t\tif (item)\n-\t\t\tru = item->util;\n-\t}\n-\n-\t/* resolve-undo record exists for the path */\n-\tif (ru)\n-\t\treturn unmerge_index_entry(&the_index, path, ru);\n+\tstruct string_list_item *item;\n \n-\t/* See if there is such entry in the index. */\n-\tpos = index_name_pos(&the_index, path, namelen);\n-\tif (0 <= pos) {\n-\t\t; /* resolve-undo record was used already -- fall back */\n-\t} else {\n-\t\t/* Is it unmerged? */\n-\t\tpos = -pos - 1;\n-\t\tif (pos < the_index.cache_nr) {\n-\t\t\tconst struct cache_entry *ce = the_index.cache[pos];\n-\t\t\tif (ce_namelen(ce) == namelen &&\n-\t\t\t    !memcmp(ce->name, path, namelen)) {\n-\t\t\t\tfprintf(stderr,\n-\t\t\t\t\t\"%s: skipping still unmerged path.\\n\",\n-\t\t\t\t\tpath);\n-\t\t\t}\n-\t\t\tgoto free_return;\n-\t\t}\n-\t\t/* No, such a path does not exist -- removed */\n-\t}\n-\n-\t/*\n-\t * We are not using resolve-undo information but just\n-\t * populating the stages #2 and #3 from HEAD and MERGE_HEAD.\n-\t *\n-\t * This is a flawed replacement of true \"unresolve\", as we do\n-\t * not have a way to recreate the stage #1 for the common\n-\t * ancestor (which may not be a unique merge-base between the\n-\t * two).\n-\t */\n-\tif (read_head_pointers()) {\n-\t\tret = -1;\n-\t\tgoto free_return;\n-\t}\n-\n-\tce_2 = read_one_ent(\"our\", &head_oid, path, namelen, 2);\n-\tce_3 = read_one_ent(\"their\", &merge_head_oid, path, namelen, 3);\n-\n-\tif (!ce_2 || !ce_3) {\n-\t\tret = -1;\n-\t\tgoto free_return;\n-\t}\n-\tif (oideq(&ce_2->oid, &ce_3->oid) &&\n-\t    ce_2->ce_mode == ce_3->ce_mode) {\n-\t\tfprintf(stderr, \"%s: identical in both, skipping.\\n\",\n-\t\t\tpath);\n-\t\tgoto free_return;\n-\t}\n-\n-\tremove_file_from_index(&the_index, path);\n-\tif (add_index_entry(&the_index, ce_2, ADD_CACHE_OK_TO_ADD)) {\n-\t\terror(\"%s: cannot add our version to the index.\", path);\n-\t\tret = -1;\n-\t\tgoto free_return;\n-\t}\n-\tif (!add_index_entry(&the_index, ce_3, ADD_CACHE_OK_TO_ADD))\n+\tif (!the_index.resolve_undo)\n \t\treturn 0;\n-\terror(\"%s: cannot add their version to the index.\", path);\n-\tret = -1;\n- free_return:\n-\tdiscard_cache_entry(ce_2);\n-\tdiscard_cache_entry(ce_3);\n-\treturn ret;\n+\titem = string_list_lookup(the_index.resolve_undo, path);\n+\tif (!item)\n+\t\treturn 0; /* no resolve-undo record for the path */\n+\n+\treturn unmerge_index_entry(&the_index, path, item->util);\n }\n \n static int do_unresolve(int ac, const char **av,\n@@ -762,6 +674,7 @@ static int do_reupdate(const char **paths,\n \tint pos;\n \tint has_head = 1;\n \tstruct pathspec pathspec;\n+\tstruct object_id head_oid;\n \n \tparse_pathspec(&pathspec, 0,\n \t\t       PATHSPEC_PREFER_CWD,\n-- \n2.41.0-478-gee48e70a82\n\n"}]}