From 91e07058c5663d01d39a00418c91415d0227d53c Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Mon, 31 Jul 2023 15:44:03 -0700 Subject: [PATCH 1/7] update-index: do not read HEAD and MERGE_HEAD unconditionally When "update-index --unresolve $path" cannot find the resolve-undo record for the path the user requested to unresolve, it stuffs the blobs from HEAD and MERGE_HEAD to stage #2 and stage #3 as a fallback. For this reason, the operation does not even start unless both "HEAD" and "MERGE_HEAD" exist. This is suboptimal in a few ways: * It does not recreate stage #1. Even though it is a correct design decision not to do so (because it is impossible to recreate in general cases, without knowing how we got there, including what merge strategy was used), it is much less useful not to have that information in the index. * It limits the "unresolve" operation only during a conflicted "git merge" and nothing else. Other operations like "rebase", "cherry-pick", and "switch -m" may result in conflicts, and the user may want to unresolve the conflict that they incorrectly resolved in order to redo the resolution, but the fallback would not kick in. * Most importantly, the entire "unresolve" operation is disabled after a conflicted merge is committed and MERGE_HEAD is removed, even though the index has perfectly usable resolve-undo records. By lazily reading the HEAD and MERGE_HEAD only when we need to go to the fallback codepath, we will allow cases where resolve-undo records are available (which is 100% of the time, unless the user is reading from an index file created by Git more than 10 years ago) to proceed even after a conflicted merge was committed, during other mergy operations that do not use MERGE_HEAD, or after the result of such mergy operations has been committed. Signed-off-by: Junio C Hamano --- builtin/update-index.c | 46 ++++++++++++++++++++++++--------------- t/t2030-unresolve-info.sh | 8 +++++++ 2 files changed, 36 insertions(+), 18 deletions(-) diff --git a/builtin/update-index.c b/builtin/update-index.c index 5fab9ad2ec..47cd68e9d5 100644 --- a/builtin/update-index.c +++ b/builtin/update-index.c @@ -639,6 +639,21 @@ static struct cache_entry *read_one_ent(const char *which, return ce; } +static int read_head_pointers(void) +{ + static int result = -2; /* unknown yet */ + + if (result == -2) { + result = -1; + if (read_ref("HEAD", &head_oid)) + return error("No HEAD -- no initial commit yet?"); + if (read_ref("MERGE_HEAD", &merge_head_oid)) + return error("Not in the middle of a merge"); + result = 0; + } + return result; +} + static int unresolve_one(const char *path) { int namelen = strlen(path); @@ -677,10 +692,20 @@ static int unresolve_one(const char *path) } } - /* Grab blobs from given path from HEAD and MERGE_HEAD, - * stuff HEAD version in stage #2, - * stuff MERGE_HEAD version in stage #3. + /* + * We are not using resolve-undo information but just + * populating the stages #2 and #3 from HEAD and MERGE_HEAD. + * + * This is a flawed replacement of true "unresolve", as we do + * not have a way to recreate the stage #1 for the common + * ancestor (which may not be a unique merge-base between the + * two). */ + if (read_head_pointers()) { + ret = -1; + goto free_return; + } + ce_2 = read_one_ent("our", &head_oid, path, namelen, 2); ce_3 = read_one_ent("their", &merge_head_oid, path, namelen, 3); @@ -711,27 +736,12 @@ static int unresolve_one(const char *path) return ret; } -static void read_head_pointers(void) -{ - if (read_ref("HEAD", &head_oid)) - die("No HEAD -- no initial commit yet?"); - if (read_ref("MERGE_HEAD", &merge_head_oid)) { - fprintf(stderr, "Not in the middle of a merge.\n"); - exit(0); - } -} - static int do_unresolve(int ac, const char **av, const char *prefix, int prefix_length) { int i; int err = 0; - /* Read HEAD and MERGE_HEAD; if MERGE_HEAD does not exist, we - * are not doing a merge, so exit with success status. - */ - read_head_pointers(); - for (i = 1; i < ac; i++) { const char *arg = av[i]; char *p = prefix_path(prefix, prefix_length, arg); diff --git a/t/t2030-unresolve-info.sh b/t/t2030-unresolve-info.sh index 2d8c70b03a..d4e7760df5 100755 --- a/t/t2030-unresolve-info.sh +++ b/t/t2030-unresolve-info.sh @@ -126,6 +126,14 @@ test_expect_success 'unmerge with plumbing' ' test_line_count = 3 actual ' +test_expect_success 'unmerge can be done even after committing' ' + prime_resolve_undo && + git commit -m "record to nuke MERGE_HEAD" && + git update-index --unresolve fi/le && + git ls-files -u >actual && + test_line_count = 3 actual +' + test_expect_success 'rerere and rerere forget' ' mkdir .git/rr-cache && prime_resolve_undo && From fe83269e16c1fd57df42c2a22a2b9ee621175a75 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Mon, 31 Jul 2023 15:44:04 -0700 Subject: [PATCH 2/7] resolve-undo: allow resurrecting conflicted state that resolved to deletion The resolve-undo index extension records up to three (mode, object name) tuples for non-zero stages for each path that was resolved, to be used to recreate the original conflicted state later when the user requests. The unmerge_index_entry_at() function uses the resolve-undo data to do so, but it assumes that the path for which the conflicted state needs to be recreated can be specified by the position in the active_cache[] array. This obviously cannot salvage the state of conflicted paths that were resolved by removing them. For example, a delete-modify conflict, in which the change whose "modify" side made is a trivial typofix, may legitimately be resolved to remove the path, and resolve-undo extension does record the two (mode, object name) tuples for the common ancestor version and their version, lacking our version. But after recording such a removal of the path, you should be able to use resolve-undo data to recreate the conflicted state. Introduce a new unmerge_index_entry() helper function that takes the path (which does not necessarily have to exist in the active_cache[] array) and resolve-undo data, and use it to reimplement unmerge_index() public function that is used by "git rerere". The limited interface is still kept for now, as it is used by "git checkout -m" and "git update-index --unmerge", but these two codepaths will be updated to lift the assumption to allow conflicts that resolved to deletion can be recreated. Signed-off-by: Junio C Hamano --- resolve-undo.c | 48 +++++++++++++++++++++++++++++++++++++++++++----- resolve-undo.h | 1 + 2 files changed, 44 insertions(+), 5 deletions(-) diff --git a/resolve-undo.c b/resolve-undo.c index 70a6db526d..3b0244e210 100644 --- a/resolve-undo.c +++ b/resolve-undo.c @@ -182,19 +182,57 @@ void unmerge_marked_index(struct index_state *istate) } } +int unmerge_index_entry(struct index_state *istate, const char *path, + struct resolve_undo_info *ru) +{ + int i = index_name_pos(istate, path, strlen(path)); + + if (i < 0) { + /* unmerged? */ + i = -i - 1; + if (i < istate->cache_nr && + !strcmp(istate->cache[i]->name, path)) + /* yes, it is already unmerged */ + return 0; + /* fallthru: resolved to removal */ + } else { + /* merged - remove it to replace it with unmerged entries */ + remove_index_entry_at(istate, i); + } + + for (i = 0; i < 3; i++) { + struct cache_entry *ce; + if (!ru->mode[i]) + continue; + ce = make_cache_entry(istate, ru->mode[i], &ru->oid[i], + path, i + 1, 0); + if (add_index_entry(istate, ce, ADD_CACHE_OK_TO_ADD)) + return error("cannot unmerge '%s'", path); + } + return 0; +} + void unmerge_index(struct index_state *istate, const struct pathspec *pathspec) { - int i; + struct string_list_item *item; if (!istate->resolve_undo) return; /* TODO: audit for interaction with sparse-index. */ ensure_full_index(istate); - for (i = 0; i < istate->cache_nr; i++) { - const struct cache_entry *ce = istate->cache[i]; - if (!ce_path_match(istate, ce, pathspec, NULL)) + + for_each_string_list_item(item, istate->resolve_undo) { + const char *path = item->string; + struct resolve_undo_info *ru = item->util; + if (!item->util) continue; - i = unmerge_index_entry_at(istate, i); + if (!match_pathspec(istate, pathspec, + item->string, strlen(item->string), + 0, NULL, 0)) + continue; + unmerge_index_entry(istate, path, ru); + free(ru); + item->util = NULL; } } diff --git a/resolve-undo.h b/resolve-undo.h index c5deafc92f..1ae321c88b 100644 --- a/resolve-undo.h +++ b/resolve-undo.h @@ -18,6 +18,7 @@ void resolve_undo_write(struct strbuf *, struct string_list *); struct string_list *resolve_undo_read(const char *, unsigned long); void resolve_undo_clear_index(struct index_state *); int unmerge_index_entry_at(struct index_state *, int); +int unmerge_index_entry(struct index_state *, const char *, struct resolve_undo_info *); void unmerge_index(struct index_state *, const struct pathspec *); void unmerge_marked_index(struct index_state *); From 35901f1c2461640019e70bbefecdce1f59f660d2 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Mon, 31 Jul 2023 15:44:05 -0700 Subject: [PATCH 3/7] update-index: use unmerge_index_entry() to support removal "update-index --unresolve" uses the unmerge_index_entry_at() that assumes that the path to be unresolved must be in the index, which makes it impossible to unresolve a path that was resolved as removal. Rewrite unresolve_one() to use the unmerge_index_entry() to support unresolving such a path. Existing tests for "update-index --unresolve" forgot to check one thing that tests for "checkout --merge -- paths" tested, which is to make sure that resolve-undo record that has already been used to recreate higher-stage index entries is removed. Add new invocations of "ls-files --resolve-undo" after running "update-index --unresolve" to make sure that unresolving with update-index does remove the used resolve-undo records. Signed-off-by: Junio C Hamano --- builtin/update-index.c | 38 ++++++++++++++++++++++---------------- t/t2030-unresolve-info.sh | 37 +++++++++++++++++++++++++++++++++---- 2 files changed, 55 insertions(+), 20 deletions(-) diff --git a/builtin/update-index.c b/builtin/update-index.c index 47cd68e9d5..ecd1c0c2d3 100644 --- a/builtin/update-index.c +++ b/builtin/update-index.c @@ -660,26 +660,31 @@ static int unresolve_one(const char *path) int pos; int ret = 0; struct cache_entry *ce_2 = NULL, *ce_3 = NULL; + struct resolve_undo_info *ru = NULL; + + if (the_index.resolve_undo) { + struct string_list_item *item; + item = string_list_lookup(the_index.resolve_undo, path); + if (item) { + ru = item->util; + item->util = NULL; + } + } + + /* resolve-undo record exists for the path */ + if (ru) { + ret = unmerge_index_entry(&the_index, path, ru); + free(ru); + return ret; + } /* See if there is such entry in the index. */ pos = index_name_pos(&the_index, path, namelen); if (0 <= pos) { - /* already merged */ - pos = unmerge_index_entry_at(&the_index, pos); - if (pos < the_index.cache_nr) { - const struct cache_entry *ce = the_index.cache[pos]; - if (ce_stage(ce) && - ce_namelen(ce) == namelen && - !memcmp(ce->name, path, namelen)) - return 0; - } - /* no resolve-undo information; fall back */ + ; /* resolve-undo record was used already -- fall back */ } else { - /* If there isn't, either it is unmerged, or - * resolved as "removed" by mistake. We do not - * want to do anything in the former case. - */ - pos = -pos-1; + /* Is it unmerged? */ + pos = -pos - 1; if (pos < the_index.cache_nr) { const struct cache_entry *ce = the_index.cache[pos]; if (ce_namelen(ce) == namelen && @@ -687,9 +692,10 @@ static int unresolve_one(const char *path) fprintf(stderr, "%s: skipping still unmerged path.\n", path); - goto free_return; } + goto free_return; } + /* No, such a path does not exist -- removed */ } /* diff --git a/t/t2030-unresolve-info.sh b/t/t2030-unresolve-info.sh index d4e7760df5..3eda385ca2 100755 --- a/t/t2030-unresolve-info.sh +++ b/t/t2030-unresolve-info.sh @@ -37,11 +37,17 @@ prime_resolve_undo () { git checkout second^0 && test_tick && test_must_fail git merge third^0 && - echo merge does not leave anything && check_resolve_undo empty && - echo different >fi/le && - git add fi/le && - echo resolving records && + + # how should the conflict be resolved? + case "$1" in + remove) + rm -f file/le && git rm fi/le + ;; + *) # modify + echo different >fi/le && git add fi/le + ;; + esac check_resolve_undo recorded fi/le initial:fi/le second:fi/le third:fi/le } @@ -122,6 +128,8 @@ test_expect_success 'add records checkout -m undoes' ' test_expect_success 'unmerge with plumbing' ' prime_resolve_undo && git update-index --unresolve fi/le && + git ls-files --resolve-undo fi/le >actual && + test_must_be_empty actual && git ls-files -u >actual && test_line_count = 3 actual ' @@ -130,6 +138,27 @@ test_expect_success 'unmerge can be done even after committing' ' prime_resolve_undo && git commit -m "record to nuke MERGE_HEAD" && git update-index --unresolve fi/le && + git ls-files --resolve-undo fi/le >actual && + test_must_be_empty actual && + git ls-files -u >actual && + test_line_count = 3 actual +' + +test_expect_success 'unmerge removal' ' + prime_resolve_undo remove && + git update-index --unresolve fi/le && + git ls-files --resolve-undo fi/le >actual && + test_must_be_empty actual && + git ls-files -u >actual && + test_line_count = 3 actual +' + +test_expect_success 'unmerge removal after committing' ' + prime_resolve_undo remove && + git commit -m "record to nuke MERGE_HEAD" && + git update-index --unresolve fi/le && + git ls-files --resolve-undo fi/le >actual && + test_must_be_empty actual && git ls-files -u >actual && test_line_count = 3 actual ' From c0a4ae7f4e010204f8341dcc3b9217dd42baee14 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Mon, 31 Jul 2023 15:44:06 -0700 Subject: [PATCH 4/7] update-index: remove stale fallback code for "--unresolve" The "update-index --unresolve" is a relatively old feature that was introduced in Git v1.4.1 (June 2006), which predates the resolve-undo extension introduced in Git v1.7.0 (February 2010). The original code that was limited only to work during a merge (and not during a rebase or a cherry-pick) has been kept as the fallback codepath to be used as a transition measure. By now, for more than 10 years we have stored resolve-undo extension in the index file, and the fallback code way outlived its usefulness. Remove it, together with two file-scope static global variables. One of these variables is still used by surviving function, but it does not have to be a global at all, so move it to local to that function. Signed-off-by: Junio C Hamano --- builtin/update-index.c | 112 ++++------------------------------------- 1 file changed, 11 insertions(+), 101 deletions(-) diff --git a/builtin/update-index.c b/builtin/update-index.c index ecd1c0c2d3..def7f98504 100644 --- a/builtin/update-index.c +++ b/builtin/update-index.c @@ -608,9 +608,6 @@ static const char * const update_index_usage[] = { NULL }; -static struct object_id head_oid; -static struct object_id merge_head_oid; - static struct cache_entry *read_one_ent(const char *which, struct object_id *ent, const char *path, int namelen, int stage) @@ -639,107 +636,19 @@ static struct cache_entry *read_one_ent(const char *which, return ce; } -static int read_head_pointers(void) -{ - static int result = -2; /* unknown yet */ - - if (result == -2) { - result = -1; - if (read_ref("HEAD", &head_oid)) - return error("No HEAD -- no initial commit yet?"); - if (read_ref("MERGE_HEAD", &merge_head_oid)) - return error("Not in the middle of a merge"); - result = 0; - } - return result; -} - static int unresolve_one(const char *path) { - int namelen = strlen(path); - int pos; - int ret = 0; - struct cache_entry *ce_2 = NULL, *ce_3 = NULL; - struct resolve_undo_info *ru = NULL; + struct string_list_item *item; + int res = 0; - if (the_index.resolve_undo) { - struct string_list_item *item; - item = string_list_lookup(the_index.resolve_undo, path); - if (item) { - ru = item->util; - item->util = NULL; - } - } - - /* resolve-undo record exists for the path */ - if (ru) { - ret = unmerge_index_entry(&the_index, path, ru); - free(ru); - return ret; - } - - /* See if there is such entry in the index. */ - pos = index_name_pos(&the_index, path, namelen); - if (0 <= pos) { - ; /* resolve-undo record was used already -- fall back */ - } else { - /* Is it unmerged? */ - pos = -pos - 1; - if (pos < the_index.cache_nr) { - const struct cache_entry *ce = the_index.cache[pos]; - if (ce_namelen(ce) == namelen && - !memcmp(ce->name, path, namelen)) { - fprintf(stderr, - "%s: skipping still unmerged path.\n", - path); - } - goto free_return; - } - /* No, such a path does not exist -- removed */ - } - - /* - * We are not using resolve-undo information but just - * populating the stages #2 and #3 from HEAD and MERGE_HEAD. - * - * This is a flawed replacement of true "unresolve", as we do - * not have a way to recreate the stage #1 for the common - * ancestor (which may not be a unique merge-base between the - * two). - */ - if (read_head_pointers()) { - ret = -1; - goto free_return; - } - - ce_2 = read_one_ent("our", &head_oid, path, namelen, 2); - ce_3 = read_one_ent("their", &merge_head_oid, path, namelen, 3); - - if (!ce_2 || !ce_3) { - ret = -1; - goto free_return; - } - if (oideq(&ce_2->oid, &ce_3->oid) && - ce_2->ce_mode == ce_3->ce_mode) { - fprintf(stderr, "%s: identical in both, skipping.\n", - path); - goto free_return; - } - - remove_file_from_index(&the_index, path); - if (add_index_entry(&the_index, ce_2, ADD_CACHE_OK_TO_ADD)) { - error("%s: cannot add our version to the index.", path); - ret = -1; - goto free_return; - } - if (!add_index_entry(&the_index, ce_3, ADD_CACHE_OK_TO_ADD)) - return 0; - error("%s: cannot add their version to the index.", path); - ret = -1; - free_return: - discard_cache_entry(ce_2); - discard_cache_entry(ce_3); - return ret; + if (!the_index.resolve_undo) + return res; + item = string_list_lookup(the_index.resolve_undo, path); + if (!item) + return res; /* no resolve-undo record for the path */ + res = unmerge_index_entry(&the_index, path, item->util); + FREE_AND_NULL(item->util); + return res; } static int do_unresolve(int ac, const char **av, @@ -766,6 +675,7 @@ static int do_reupdate(const char **paths, int pos; int has_head = 1; struct pathspec pathspec; + struct object_id head_oid; parse_pathspec(&pathspec, 0, PATHSPEC_PREFER_CWD, From 54f98fee5069f50c6e96687504b82a7695c8648a Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Mon, 31 Jul 2023 15:44:07 -0700 Subject: [PATCH 5/7] checkout/restore: refuse unmerging paths unless checking out of the index Recreating unmerged index entries using resolve-undo data, recreating conflicted working tree files using unmerged index entries, and writing data out of unmerged index entries, make sense only when we are checking paths out of the index and not when we are checking paths out of a tree-ish. Add an extra check to make sure "--merge" and "--ours/--theirs" options are rejected when checking out from a tree-ish, update the document (especially the SYNOPSIS section) to highlight that they are incompatible, and add a few tests to make sure the combination fails. Signed-off-by: Junio C Hamano --- Documentation/git-checkout.txt | 9 ++++++--- Documentation/git-restore.txt | 4 ++++ builtin/checkout.c | 9 +++++++++ t/t2070-restore.sh | 7 +++++-- t/t7201-co.sh | 5 +++++ 5 files changed, 29 insertions(+), 5 deletions(-) diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt index 4af0904f47..a30e3ebc51 100644 --- a/Documentation/git-checkout.txt +++ b/Documentation/git-checkout.txt @@ -12,8 +12,10 @@ SYNOPSIS 'git checkout' [-q] [-f] [-m] --detach [] 'git checkout' [-q] [-f] [-m] [--detach] 'git checkout' [-q] [-f] [-m] [[-b|-B|--orphan] ] [] -'git checkout' [-f|--ours|--theirs|-m|--conflict=