{"thread":{"id":"31056","subject":"[RFC/PATCH] Only call record_resolve_undo() when coming from add/rm","startedAt":"2012-07-20T21:28:48Z","lastAt":"2012-07-22T18:37:19Z","messageCount":3,"participants":["Thomas Rast","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"195387","messageId":"e0e9ce3599d898c974c56c818d9858638e343f5b.1342819276.git.trast@student.ethz.ch","threadId":"31056","inReplyTo":null,"subject":"[RFC/PATCH] Only call record_resolve_undo() when coming from add/rm","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2012-07-20T21:28:48Z","receivedAt":"2012-07-20T21:28:48Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"The REUC extension stores the stage 1/2/3 data of entries which were\nmarked resolved by the user, to enable 'git checkout -m <name>' to\nrestore the conflicted state later.\n\nWhen a file was deleted on one side of the merge and unmodified on the\nother, merge-recursive uses remove_file_from_cache() to remove it from\nthe result index.  This uses remove_index_entry_at(), which calls\nrecord_resolve_undo().\n\nSuch REUC entries are in fact even useless: neither 'git checkout -m'\nnor 'git update-index --unresolve' can resurrect the file (the former\nbecause there is no corresponding index entry, the latter because the\nfile is missing one side).\n\nSolve this by taking a more specific approach to record_resolve_undo():\n\n* git-rm and 'git update-index --remove' go through\n  remove_file_from_cache(), just like merge-recursive.  Make them use\n  a new _extended version that optionally records REUC.\n\n* git-add and 'git update-index conflicted_file' go through the\n  add_index_entry() call chain.  git-apply and git-read-tree use\n  add_index_entry() too.  However, they insert stage-0 entries, which\n  already means resolving.  So even if these cases were not caught\n  earlier, saving the unresolved state would be correct.\n\n  So we can unconditionally record REUC deeper in the call chain.\n\n(git-mv calls remove_index_entry_at() through rename_index_entry_at();\nhowever, it refuses to work on unresolved files.  So that's okay too.)\n\nSigned-off-by: Thomas Rast <trast@student.ethz.ch>\n---\n\nThomas and me discovered this while hacking on index-v5.  It would be\na bit tricky to handle there: the index is structured according to the\ndirectory layout of the files it contains, and the REUC data is the\nsame as the conflict (stages) data plus a flag bit, for (future)\neasier toggling between them.  With the behavior before this patch, it\nis possible to have directories that do not have any \"live\" files, yet\ncontain REUC records.  For example, in git.git,\n\n  $ git checkout 8e17b34c33^1\n  $ git merge 8e17b34c33^2\n  $ g ls-files -s | grep arpa\n  $ g ls-files --resolve-undo | grep arpa\n  100644 0d8552a2c6dc3a5fee5360ea2ff468463461e3bf 1       compat/vcbuild/include/arpa/inet.h\n  100644 0d8552a2c6dc3a5fee5360ea2ff468463461e3bf 3       compat/vcbuild/include/arpa/inet.h\n\n(and several other files in the same commit).  So this patch would let\nus keep the complexity of that handling down, by not running into such\ncases.\n\n builtin/rm.c           |  2 +-\n builtin/update-index.c |  2 +-\n cache.h                |  2 ++\n read-cache.c           | 14 +++++++++++---\n 4 files changed, 15 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/rm.c b/builtin/rm.c\nindex 90c8a50..145d368 100644\n--- a/builtin/rm.c\n+++ b/builtin/rm.c\n@@ -226,7 +226,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n \t\tif (!quiet)\n \t\t\tprintf(\"rm '%s'\\n\", path);\n \n-\t\tif (remove_file_from_cache(path))\n+\t\tif (remove_file_from_cache_extended(path, 1))\n \t\t\tdie(_(\"git rm: unable to remove %s\"), path);\n \t}\n \ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex 5f038d6..2e58979 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -196,7 +196,7 @@ static int process_path(const char *path)\n \t\t * so updating it does not make sense.\n \t\t * On the other hand, removing it from index should work\n \t\t */\n-\t\tif (allow_remove && remove_file_from_cache(path))\n+\t\tif (allow_remove && remove_file_from_cache_extended(path, 1))\n \t\t\treturn error(\"%s: cannot remove from the index\", path);\n \t\treturn 0;\n \t}\ndiff --git a/cache.h b/cache.h\nindex 14d8fd4..616a355 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -378,6 +378,7 @@ static inline void remove_name_hash(struct cache_entry *ce)\n #define rename_cache_entry_at(pos, new_name) rename_index_entry_at(&the_index, (pos), (new_name))\n #define remove_cache_entry_at(pos) remove_index_entry_at(&the_index, (pos))\n #define remove_file_from_cache(path) remove_file_from_index(&the_index, (path))\n+#define remove_file_from_cache_extended(path, flags) remove_file_from_index_extended(&the_index, (path), (flags))\n #define add_to_cache(path, st, flags) add_to_index(&the_index, (path), (st), (flags))\n #define add_file_to_cache(path, flags) add_file_to_index(&the_index, (path), (flags))\n #define refresh_cache(flags) refresh_index(&the_index, (flags), NULL, NULL, NULL)\n@@ -522,6 +523,7 @@ static inline enum object_type object_type(unsigned int mode)\n extern int remove_index_entry_at(struct index_state *, int pos);\n extern void remove_marked_cache_entries(struct index_state *istate);\n extern int remove_file_from_index(struct index_state *, const char *path);\n+extern int remove_file_from_index_extended(struct index_state *, const char *path, int save_reuc);\n #define ADD_CACHE_VERBOSE 1\n #define ADD_CACHE_PRETEND 2\n #define ADD_CACHE_IGNORE_ERRORS\t4\ndiff --git a/read-cache.c b/read-cache.c\nindex ee4e633..e172143 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -540,7 +540,6 @@ int remove_index_entry_at(struct index_state *istate, int pos)\n {\n \tstruct cache_entry *ce = istate->cache[pos];\n \n-\trecord_resolve_undo(istate, ce);\n \tremove_name_hash(ce);\n \tistate->cache_changed = 1;\n \tistate->cache_nr--;\n@@ -572,17 +571,25 @@ void remove_marked_cache_entries(struct index_state *istate)\n \tistate->cache_nr = j;\n }\n \n-int remove_file_from_index(struct index_state *istate, const char *path)\n+int remove_file_from_index_extended(struct index_state *istate, const char *path, int save_reuc)\n {\n \tint pos = index_name_pos(istate, path, strlen(path));\n \tif (pos < 0)\n \t\tpos = -pos-1;\n \tcache_tree_invalidate_path(istate->cache_tree, path);\n-\twhile (pos < istate->cache_nr && !strcmp(istate->cache[pos]->name, path))\n+\twhile (pos < istate->cache_nr && !strcmp(istate->cache[pos]->name, path)) {\n+\t\tif (save_reuc)\n+\t\t\trecord_resolve_undo(istate, istate->cache[pos]);\n \t\tremove_index_entry_at(istate, pos);\n+\t}\n \treturn 0;\n }\n \n+int remove_file_from_index(struct index_state *istate, const char *path)\n+{\n+\treturn remove_file_from_index_extended(istate, path, 0);\n+}\n+\n static int compare_name(struct cache_entry *ce, const char *path, int namelen)\n {\n \treturn namelen != ce_namelen(ce) || memcmp(path, ce->name, namelen);\n@@ -1020,6 +1027,7 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e\n \tif (pos < istate->cache_nr && ce_stage(ce) == 0) {\n \t\twhile (ce_same_name(istate->cache[pos], ce)) {\n \t\t\tok_to_add = 1;\n+\t\t\trecord_resolve_undo(istate, istate->cache[pos]);\n \t\t\tif (!remove_index_entry_at(istate, pos))\n \t\t\t\tbreak;\n \t\t}\n-- \n1.7.11.2.589.gcf84a3b\n"},{"id":"195389","messageId":"87liievzr8.fsf@thomas.inf.ethz.ch","threadId":"31056","inReplyTo":"e0e9ce3599d898c974c56c818d9858638e343f5b.1342819276.git.trast@student.ethz.ch","subject":"Re: [RFC/PATCH] Only call record_resolve_undo() when coming from add/rm","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2012-07-20T22:22:51Z","receivedAt":"2012-07-20T22:22:51Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Thomas Rast <trast@student.ethz.ch> writes:\n\n> Thomas and me discovered this while hacking on index-v5.  It would be\n> a bit tricky to handle there: the index is structured according to the\n> directory layout of the files it contains, and the REUC data is the\n> same as the conflict (stages) data plus a flag bit, for (future)\n> easier toggling between them.  With the behavior before this patch, it\n> is possible to have directories that do not have any \"live\" files, yet\n> contain REUC records.  For example, in git.git,\n>\n>   $ git checkout 8e17b34c33^1\n>   $ git merge 8e17b34c33^2\n>   $ g ls-files -s | grep arpa\n>   $ g ls-files --resolve-undo | grep arpa\n>   100644 0d8552a2c6dc3a5fee5360ea2ff468463461e3bf 1       compat/vcbuild/include/arpa/inet.h\n>   100644 0d8552a2c6dc3a5fee5360ea2ff468463461e3bf 3       compat/vcbuild/include/arpa/inet.h\n\nWe just discovered that this merge is no longer in current git.git.  It\nwent into master as b7f7c079, and the example works with that.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"195447","messageId":"7vtxwzskv4.fsf@alter.siamese.dyndns.org","threadId":"31056","inReplyTo":"e0e9ce3599d898c974c56c818d9858638e343f5b.1342819276.git.trast@student.ethz.ch","subject":"Re: [RFC/PATCH] Only call record_resolve_undo() when coming from add/rm","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-22T18:37:19Z","receivedAt":"2012-07-22T18:37:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast <trast@student.ethz.ch> writes:\n\n> The REUC extension stores the stage 1/2/3 data of entries which were\n> marked resolved by the user, to enable 'git checkout -m <name>' to\n> restore the conflicted state later.\n\nYes.\n\n> When a file was deleted on one side of the merge and unmodified on the\n> other, merge-recursive uses remove_file_from_cache() to remove it from\n> the result index.  This uses remove_index_entry_at(), which calls\n> record_resolve_undo().\n\nWhat is missing from here which confused me during my initial read\nof this patch is \"But such a removal is the natural resolution for\nthe three-way merge. It is not a resolution by the end user, and\nshould not be unresolved with 'checkout -m'. Recording REUC entry\nfor such a path is wrong.\"\n\nPerhaps you thought it was so obvious that it can be left unsaid,\nbut combined with the \"in fact _even_ useless\" below, I had to\nre-read it to find something that says why it was _wrong_, did not\nfind any, and had to scratch my head.\n\n> Such REUC entries are in fact even useless: neither 'git checkout -m'\n> nor 'git update-index --unresolve' can resurrect the file (the former\n> because there is no corresponding index entry, the latter because the\n> file is missing one side).\n\nI do not think that \"they are not used the current implementation of\nthe commands that are supposed to use the information\" automatically\nqualifies as a good reason to remove them. If the conflict were \"we\nmodified while they removed\", the resolution may either be \"keep\nsome stuff we added\" or \"delete the path\", we may want to be able to\nresurrect the conflicted state with \"checkout -m\" for them, and we\nmay want to fix \"checkout -m\" and \"update-index --unresolve\" to deal\nwith such a case if they don't already, which is an independent topic.\n\nFor the \"one side untouched, the other side removed\" case, removing\nis the natural resolution, so we do not want to have REUC entry to\nbegin with, so there is nothing to fix in \"checkout -m\" for that\ncase.\n\n> Solve this by taking a more specific approach to record_resolve_undo():\n\nJust a sanity check.\n\nAre there cases we would want to have _any_ REUC entries in the\nindex, after running any mergy operation, not just merge-recursive,\nbut cherry-pick and friends that share the same machinery?\n\n> * git-rm and 'git update-index --remove' go through\n>   remove_file_from_cache(), just like merge-recursive.  Make them use\n>   a new _extended version that optionally records REUC.\n\nI wonder if it is better have two functions, one records REUC (and\ndoes nothing else) and the other that removes a path from the\nin-core index (and does nothing else), instead of two functions that\nboth remove a path from the in-core index (one with REUC and the\nother without).  Would it be less error-prone for the callers and\nmake the resulting code easier to follow?\n\n\tif (path is conflicted and we are resolving as removal)\n\t\trecord_REUC(&the_index, path);\n\tremove_file_from_cache(&the_index, path);\n\nNot a suggestion \"I think it is better\", but just a question.\n\n> * git-add and 'git update-index conflicted_file' go through the\n>   add_index_entry() call chain.  git-apply and git-read-tree use\n>   add_index_entry() too.  However, they insert stage-0 entries, which\n>   already means resolving.  So even if these cases were not caught\n>   earlier, saving the unresolved state would be correct.\n>   So we can unconditionally record REUC deeper in the call chain.\n\nAre there cases where an automerge use add_index_entry() to insert a\nstage#0 entry to the index (which already means resolving) to record\na clean automerge?  Doesn't the same \"a natural three-way resolution\nshould not be unresolved with 'checkout -m'\" reasoning as the original\nmotivation of this patch apply to it?\n"}]}