{"thread":{"id":"16671","subject":"[PATCH] modify/delete conflict resolution overwrites untracked file","startedAt":"2008-12-10T20:12:59Z","lastAt":"2008-12-28T23:53:17Z","messageCount":24,"participants":["Clemens Buchacher","Junio C Hamano","Johannes Schindelin","Mike Ralphson","Jakub Narebski"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"97513","messageId":"20081210201259.GA12928@localhost","threadId":"16671","inReplyTo":null,"subject":"[PATCH] modify/delete conflict resolution overwrites untracked file","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2008-12-10T20:12:59Z","receivedAt":"2008-12-10T20:12:59Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"If a file was removed in HEAD, but modified in MERGE_HEAD, recursive merge\nwill result in a \"CONFLICT (delete/modify)\". If the (now untracked) file\nalready exists and was not added to the index, it is overwritten with the\nconflict resolution contents.\n\nIn similar situations (cf. test 2), the merge would abort with\n\n\"error: Untracked working tree 'file' would be overwritten by merge.\"\n\nThe same should happen in this case.\n\nSigned-off-by: Clemens Buchacher \n---\n\nIf it's a regression, it dates far back, since 1.5.0 fails as well.\n\nI don't have time to look into this until Saturday. I won't complain if\nsomeone beats me to it. ;-)\n\nClemens\n\n t/t7607-merge-overwrite.sh | 87 ++++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 87 insertions(+), 0 deletions(-)\n create mode 100755 t/t7607-merge-overwrite.sh\n\ndiff --git a/t/t7607-merge-overwrite.sh b/t/t7607-merge-overwrite.sh\nnew file mode 100755\nindex 0000000..513097c\n--- /dev/null\n+++ b/t/t7607-merge-overwrite.sh\n@@ -0,0 +1,87 @@\n+#!/bin/sh\n+\n+test_description='git-merge\n+\n+Do not overwrite changes.'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\techo c0 > c0.c &&\n+\tgit add c0.c &&\n+\tgit commit -m c0 &&\n+\tgit tag c0 &&\n+\techo c1 > c1.c &&\n+\tgit add c1.c &&\n+\tgit commit -m c1 &&\n+\tgit tag c1 &&\n+\tgit reset --hard c0 &&\n+\techo c2 > c2.c &&\n+\tgit add c2.c &&\n+\tgit commit -m c2 &&\n+\tgit tag c2 &&\n+\tgit reset --hard c1 &&\n+\techo \"c1 a\" > c1.c &&\n+\tgit add c1.c &&\n+\tgit commit -m \"c1 a\" &&\n+\tgit tag c1a &&\n+\techo \"VERY IMPORTANT CHANGES\" > important\n+'\n+\n+test_expect_success 'will not overwrite untracked file' '\n+\tgit reset --hard c1 &&\n+\tcat important > c2.c &&\n+\t! git merge c2 &&\n+\ttest_cmp important c2.c\n+'\n+\n+test_expect_success 'will not overwrite new file' '\n+\tgit reset --hard c1 &&\n+\tcat important > c2.c &&\n+\tgit add c2.c &&\n+\t! git merge c2 &&\n+\ttest_cmp important c2.c\n+'\n+\n+test_expect_success 'will not overwrite staged changes' '\n+\tgit reset --hard c1 &&\n+\tcat important > c2.c &&\n+\tgit add c2.c &&\n+\trm c2.c &&\n+\t! git merge c2 &&\n+\tgit checkout c2.c &&\n+\ttest_cmp important c2.c\n+'\n+\n+test_expect_failure 'will not overwrite removed file' '\n+\tgit reset --hard c1 &&\n+\tgit rm c1.c &&\n+\tgit commit -m \"rm c1.c\" &&\n+\tcat important > c1.c &&\n+\t! git merge c1a &&\n+\ttest_cmp important c1.c\n+'\n+\n+test_expect_success 'will not overwrite re-added file' '\n+\tgit reset --hard c1 &&\n+\tgit rm c1.c &&\n+\tgit commit -m \"rm c1.c\" &&\n+\tcat important > c1.c &&\n+\tgit add c1.c &&\n+\t! git merge c1a &&\n+\ttest_cmp important c1.c\n+'\n+\n+test_expect_success 'will not overwrite removed file with staged changes' '\n+\tgit reset --hard c1 &&\n+\tgit rm c1.c &&\n+\tgit commit -m \"rm c1.c\" &&\n+\tcat important > c1.c &&\n+\tgit add c1.c &&\n+\trm c1.c &&\n+\t! git merge c1a &&\n+\tgit checkout c1.c &&\n+\ttest_cmp important c1.c\n+'\n+\n+test_done\n-- \n1.6.0\n"},{"id":"97519","messageId":"7vzlj3ycr4.fsf@gitster.siamese.dyndns.org","threadId":"16671","inReplyTo":"20081210201259.GA12928@localhost","subject":"Re: [PATCH] modify/delete conflict resolution overwrites untracked file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-10T20:51:59Z","receivedAt":"2008-12-10T20:51:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Clemens Buchacher writes:\n\n> If it's a regression, it dates far back, since 1.5.0 fails as well.\n\nA good lit(h)mus test to see if it is a regression or just a plain bug in\nthe recursive strategy would be to see what 'resolve' strategy does\n(replace \"merge\" with \"merge -s resolve\" in your test).\n"},{"id":"97521","messageId":"20081210211147.GA13624@localhost","threadId":"16671","inReplyTo":"7vzlj3ycr4.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] modify/delete conflict resolution overwrites untracked file","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2008-12-10T21:11:47Z","receivedAt":"2008-12-10T21:11:47Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Wed, Dec 10, 2008 at 12:51:59PM -0800, Junio C Hamano wrote:\n> Clemens Buchacher writes:\n> \n> > If it's a regression, it dates far back, since 1.5.0 fails as well.\n> \n> A good lit(h)mus test to see if it is a regression or just a plain bug in\n> the recursive strategy would be to see what 'resolve' strategy does\n> (replace \"merge\" with \"merge -s resolve\" in your test).\n\n\"merge -s resolve\" fails with\n\nTrying really trivial in-index merge...\nerror: Merge requires file-level merging\nNope.\nTrying simple merge.\nSimple merge failed, trying Automatic merge.\nERROR: c1.c: Not handling case ae9304576a6ec3419b231b2b9c8e33a06f97f9fb ->\n-> 8173b675dc61bb578b411c769c9fb654625a7c4e\nfatal: merge program failed\nAutomatic merge failed; fix conflicts and then commit the result.\n\nand therefore passes the test.\n"},{"id":"97540","messageId":"7voczjy55g.fsf@gitster.siamese.dyndns.org","threadId":"16671","inReplyTo":"20081210211147.GA13624@localhost","subject":"Re: [PATCH] modify/delete conflict resolution overwrites untracked file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-10T23:36:11Z","receivedAt":"2008-12-10T23:36:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Clemens Buchacher writes:\n\n> On Wed, Dec 10, 2008 at 12:51:59PM -0800, Junio C Hamano wrote:\n>> Clemens Buchacher writes:\n>> \n>> > If it's a regression, it dates far back, since 1.5.0 fails as well.\n>> \n>> A good lit(h)mus test to see if it is a regression or just a plain bug in\n>> the recursive strategy would be to see what 'resolve' strategy does\n>> (replace \"merge\" with \"merge -s resolve\" in your test).\n>\n> \"merge -s resolve\" fails with\n>\n> Trying really trivial in-index merge...\n> error: Merge requires file-level merging\n> Nope.\n> Trying simple merge.\n> Simple merge failed, trying Automatic merge.\n> ERROR: c1.c: Not handling case ae9304576a6ec3419b231b2b9c8e33a06f97f9fb ->\n> -> 8173b675dc61bb578b411c769c9fb654625a7c4e\n> fatal: merge program failed\n> Automatic merge failed; fix conflicts and then commit the result.\n>\n> and therefore passes the test.\n\nAre you saying that:\n\n (1) the step should result in conflict and the merge should fail, but it\n should not clobber c1.c nevertheless; and\n\n (2) resolve fails to merge (as expected), and it does not clobber c1.c\n (as expected); therefore it passes the test.\n\nIf so, then you now established that it is a bug in merge-recursive,\nright [implementors of recursive-in-C CC'ed]?\n\nOr are you saying that the step should not fail to begin with?\n"},{"id":"97586","messageId":"20081211080752.GA26709@localhost","threadId":"16671","inReplyTo":"7voczjy55g.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] modify/delete conflict resolution overwrites untracked file","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2008-12-11T08:07:52Z","receivedAt":"2008-12-11T08:07:52Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Wed, Dec 10, 2008 at 03:36:11PM -0800, Junio C Hamano wrote:\n> Clemens Buchacher writes:\n> >\n> > \"merge -s resolve\" fails with\n> >\n> > Trying really trivial in-index merge...\n> > error: Merge requires file-level merging\n> > Nope.\n> > Trying simple merge.\n> > Simple merge failed, trying Automatic merge.\n> > ERROR: c1.c: Not handling case ae9304576a6ec3419b231b2b9c8e33a06f97f9fb ->\n> > -> 8173b675dc61bb578b411c769c9fb654625a7c4e\n> > fatal: merge program failed\n> > Automatic merge failed; fix conflicts and then commit the result.\n> >\n> > and therefore passes the test.\n> \n> Are you saying that:\n> \n> (1) the step should result in conflict and the merge should fail, but it\n> should not clobber c1.c nevertheless; and\n> \n> (2) resolve fails to merge (as expected), and it does not clobber c1.c\n> (as expected); therefore it passes the test.\n\nThe latter.\n\n> If so, then you now established that it is a bug in merge-recursive,\n> right [implementors of recursive-in-C CC'ed]?\n\nCorrect.\n\n> Or are you saying that the step should not fail to begin with?\n\nNo. IMO, merge should fail and abort. That is, it should not modify the\nworking tree at all and tell the user that an untracked file is in the way.\n\nThe tests check that merge returns an error code and c1.c is not modified.\nTest number 5 fails, unless the merge strategy resolve is used. While this\nindicates a bug in the recursive strategy, I am not satisfied with the error\noutput of the resolve strategy either. It should output\n\n\"error: Untracked working tree file '...' would be overwritten by merge.\"\n\njust like test number 2 does.\n"},{"id":"97588","messageId":"7vtz9bt9ih.fsf@gitster.siamese.dyndns.org","threadId":"16671","inReplyTo":"20081211080752.GA26709@localhost","subject":"Re: [PATCH] modify/delete conflict resolution overwrites untracked file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-11T08:13:10Z","receivedAt":"2008-12-11T08:13:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Clemens Buchacher writes:\n\n> On Wed, Dec 10, 2008 at 03:36:11PM -0800, Junio C Hamano wrote:\n>> Clemens Buchacher writes:\n>> >\n>> > \"merge -s resolve\" fails with\n>> >\n>> > Trying really trivial in-index merge...\n>> > error: Merge requires file-level merging\n>> > Nope.\n>> > Trying simple merge.\n>> > Simple merge failed, trying Automatic merge.\n>> > ERROR: c1.c: Not handling case ae9304576a6ec3419b231b2b9c8e33a06f97f9fb ->\n>> > -> 8173b675dc61bb578b411c769c9fb654625a7c4e\n>> > fatal: merge program failed\n>> > Automatic merge failed; fix conflicts and then commit the result.\n>> >\n>> > and therefore passes the test.\n>> \n>> Are you saying that:\n>> \n>> (1) the step should result in conflict and the merge should fail, but it\n>> should not clobber c1.c nevertheless; and\n>> \n>> (2) resolve fails to merge (as expected), and it does not clobber c1.c\n>> (as expected); therefore it passes the test.\n>\n> The latter.\n\nI said \"and\" at the end of (1) so I do not understand your answer, but I\ntake it to mean that both (1) and (2) are true, judging from the rest of\nyour message.\n\n>> If so, then you now established that it is a bug in merge-recursive,\n>> right [implementors of recursive-in-C CC'ed]?\n>\n> Correct.\n>\n>> Or are you saying that the step should not fail to begin with?\n>\n> No...\n\nOk, thanks. Unfortunately my git day for this week is over, so I'll try\nto find time to take a look at what recursive strategy does wrong over the\nweekend, if nobody gets to it before me.\n"},{"id":"97912","messageId":"20081215004651.GA16205@localhost","threadId":"16671","inReplyTo":"20081210201259.GA12928@localhost","subject":"Re: [PATCH] modify/delete conflict resolution overwrites untracked file","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2008-12-15T00:46:51Z","receivedAt":"2008-12-15T00:46:51Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Wed, Dec 10, 2008 at 09:12:59PM +0100, Clemens Buchacher wrote:\n> If a file was removed in HEAD, but modified in MERGE_HEAD, recursive merge\n> will result in a \"CONFLICT (delete/modify)\". If the (now untracked) file\n> already exists and was not added to the index, it is overwritten with the\n> conflict resolution contents.\n\nThe following patch fixes the problem described above, but it also breaks\nt6023-merge-rename-nocruft.sh, which tries to merge \"A\" renamed to \"B\" in\nHEAD and \"A\" modified in MERGE_HEAD, while ignoring an untracked file \"A\" in\nthe working tree. If we want to be able to do this, we have to handle the\nother case after rename detection.\n\n---\n unpack-trees.c | 3 +++\n 1 files changed, 3 insertions(+), 0 deletions(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 54f301d..de5b616 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -859,6 +859,9 @@ int threeway_merge(struct cache_entry **stages, struct unpack_trees_options *o)\n \tif (index) {\n \t\tif (verify_uptodate(index, o))\n \t\t\treturn -1;\n+\t} else if (remote && !remote_match) {\n+\t\tif (verify_absent(remote, \"overwritten\", o))\n+\t\t\treturn -1;\n \t}\n \n \to->nontrivial_merge = 1;\n-- \n1.6.1.rc2\n"},{"id":"97913","messageId":"7v63lm1c76.fsf@gitster.siamese.dyndns.org","threadId":"16671","inReplyTo":"20081215004651.GA16205@localhost","subject":"Re: [PATCH] modify/delete conflict resolution overwrites untracked file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-15T01:03:57Z","receivedAt":"2008-12-15T01:03:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Clemens Buchacher writes:\n\n> On Wed, Dec 10, 2008 at 09:12:59PM +0100, Clemens Buchacher wrote:\n>> If a file was removed in HEAD, but modified in MERGE_HEAD, recursive merge\n>> will result in a \"CONFLICT (delete/modify)\". If the (now untracked) file\n>> already exists and was not added to the index, it is overwritten with the\n>> conflict resolution contents.\n>\n> The following patch fixes the problem described above, but it also breaks\n> t6023-merge-rename-nocruft.sh, which tries to merge \"A\" renamed to \"B\" in\n> HEAD and \"A\" modified in MERGE_HEAD, while ignoring an untracked file \"A\" in\n> the working tree. If we want to be able to do this, we have to handle the\n> other case after rename detection.\n\nIf the breakage is in merge-recursive but not in merge-resolve, my gut\nfeeling is that we should not be touching unpack-trees at all. With luck\nI may be able to find some time to take a look at this myself but right\nnow we are entertaining a guest, so....\n"},{"id":"97921","messageId":"7vmyeyyuuh.fsf@gitster.siamese.dyndns.org","threadId":"16671","inReplyTo":"7v63lm1c76.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] modify/delete conflict resolution overwrites untracked file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-15T03:34:46Z","receivedAt":"2008-12-15T03:34:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Clemens Buchacher writes:\n>\n>> On Wed, Dec 10, 2008 at 09:12:59PM +0100, Clemens Buchacher wrote:\n>>> If a file was removed in HEAD, but modified in MERGE_HEAD, recursive merge\n>>> will result in a \"CONFLICT (delete/modify)\". If the (now untracked) file\n>>> already exists and was not added to the index, it is overwritten with the\n>>> conflict resolution contents.\n>>\n>> The following patch fixes the problem described above, but it also breaks\n>> t6023-merge-rename-nocruft.sh, which tries to merge \"A\" renamed to \"B\" in\n>> HEAD and \"A\" modified in MERGE_HEAD, while ignoring an untracked file \"A\" in\n>> the working tree. If we want to be able to do this, we have to handle the\n>> other case after rename detection.\n>\n> If the breakage is in merge-recursive but not in merge-resolve, my gut\n> feeling is that we should not be touching unpack-trees at all. With luck\n> I may be able to find some time to take a look at this myself but right\n> now we are entertaining a guest, so....\n\n-- >8 --\nmerge-recursive: do not clobber untracked working tree garbage\n\nWhen merge-recursive wanted to create a new file in the work tree (either\nas the final result, or a hint for reference purposes while delete/modify\nconflicts), it unconditionally overwrote an untracked file in the working\ntree. Be careful not to lose whatever the user has that is not tracked.\n\nSigned-off-by: Junio C Hamano \n---\n merge-recursive.c | 32 ++++++++++++++++++++++++++++++++\n 1 files changed, 32 insertions(+), 0 deletions(-)\n\ndiff --git c/merge-recursive.c w/merge-recursive.c\nindex a0c804c..2da4333 100644\n--- c/merge-recursive.c\n+++ w/merge-recursive.c\n@@ -447,6 +447,30 @@ static void flush_buffer(int fd, const char *buf, unsigned long size)\n \t}\n }\n \n+static int would_lose_untracked(const char *path)\n+{\n+\tint pos = cache_name_pos(path, strlen(path));\n+\n+\tif (pos < 0)\n+\t\tpos = -1 - pos;\n+\twhile (pos < active_nr &&\n+\t !strcmp(path, active_cache[pos]->name)) {\n+\t\t/*\n+\t\t * If stage #0, it is definitely tracked.\n+\t\t * If it has stage #2 then it was tracked\n+\t\t * before this merge started. All other\n+\t\t * cases the path was not tracked.\n+\t\t */\n+\t\tswitch (ce_stage(active_cache[pos])) {\n+\t\tcase 0:\n+\t\tcase 2:\n+\t\t\treturn 0;\n+\t\t}\n+\t\tpos++;\n+\t}\n+\treturn file_exists(path);\n+}\n+\n static int make_room_for_path(const char *path)\n {\n \tint status;\n@@ -462,6 +486,14 @@ static int make_room_for_path(const char *path)\n \t\tdie(msg, path, \"\");\n \t}\n \n+\t/*\n+\t * Do not unlink a file in the work tree if we are not\n+\t * tracking it.\n+\t */\n+\tif (would_lose_untracked(path))\n+\t\treturn error(\"refusing to lose untracked file at '%s'\",\n+\t\t\t path);\n+\n \t/* Successful unlink is good.. */\n \tif (!unlink(path))\n \t\treturn 0;\n"},{"id":"97937","messageId":"alpine.DEB.1.00.0812151032230.14632@racer","threadId":"16671","inReplyTo":"7vmyeyyuuh.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] modify/delete conflict resolution overwrites untracked file","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-12-15T09:34:05Z","receivedAt":"2008-12-15T09:34:05Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 14 Dec 2008, Junio C Hamano wrote:\n\n\n> merge-recursive: do not clobber untracked working tree garbage\n> \n> When merge-recursive wanted to create a new file in the work tree (either\n> as the final result, or a hint for reference purposes while delete/modify\n> conflicts), it unconditionally overwrote an untracked file in the working\n> tree. Be careful not to lose whatever the user has that is not tracked.\n> \n> Signed-off-by: Junio C Hamano \n> ---\n\nThanks, I had no time at all to look into this issue.\n\n> diff --git c/merge-recursive.c w/merge-recursive.c\n> index a0c804c..2da4333 100644\n> --- c/merge-recursive.c\n> +++ w/merge-recursive.c\n> @@ -447,6 +447,30 @@ static void flush_buffer(int fd, const char *buf, unsigned long size)\n> \t}\n> }\n> \n> +static int would_lose_untracked(const char *path)\n> +{\n> +\tint pos = cache_name_pos(path, strlen(path));\n> +\n> +\tif (pos < 0)\n> +\t\tpos = -1 - pos;\n> +\twhile (pos < active_nr &&\n> +\t !strcmp(path, active_cache[pos]->name)) {\n> +\t\t/*\n> +\t\t * If stage #0, it is definitely tracked.\n> +\t\t * If it has stage #2 then it was tracked\n> +\t\t * before this merge started. All other\n> +\t\t * cases the path was not tracked.\n> +\t\t */\n> +\t\tswitch (ce_stage(active_cache[pos])) {\n> +\t\tcase 0:\n> +\t\tcase 2:\n> +\t\t\treturn 0;\n> +\t\t}\n> +\t\tpos++;\n> +\t}\n> +\treturn file_exists(path);\n\nI wonder if it is cheaper to test file_exists() when the index contains a \nlot of files...\n\nThanks,\nDscho\n"},{"id":"97940","messageId":"20081215095949.GA7403@localhost","threadId":"16671","inReplyTo":"7vmyeyyuuh.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] modify/delete conflict resolution overwrites untracked file","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2008-12-15T09:59:49Z","receivedAt":"2008-12-15T09:59:49Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Sun, Dec 14, 2008 at 07:34:46PM -0800, Junio C Hamano wrote:\n> merge-recursive: do not clobber untracked working tree garbage\n> \n> When merge-recursive wanted to create a new file in the work tree (either\n> as the final result, or a hint for reference purposes while delete/modify\n> conflicts), it unconditionally overwrote an untracked file in the working\n> tree. Be careful not to lose whatever the user has that is not tracked.\n\nThis leaves the index in an unmerged state, however, so that a subsequent\ngit reset --hard still kills the file. And I just realized that the same\ngoes for merge-resolve. So I'd prefer to abort the merge, leave everything\nunchanged and tell the user to clean up first.\n"},{"id":"97946","messageId":"7vskopwxej.fsf@gitster.siamese.dyndns.org","threadId":"16671","inReplyTo":"20081215095949.GA7403@localhost","subject":"Re: [PATCH] modify/delete conflict resolution overwrites untracked file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-15T10:22:28Z","receivedAt":"2008-12-15T10:22:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Clemens Buchacher writes:\n\n> On Sun, Dec 14, 2008 at 07:34:46PM -0800, Junio C Hamano wrote:\n>> merge-recursive: do not clobber untracked working tree garbage\n>> \n>> When merge-recursive wanted to create a new file in the work tree (either\n>> as the final result, or a hint for reference purposes while delete/modify\n>> conflicts), it unconditionally overwrote an untracked file in the working\n>> tree. Be careful not to lose whatever the user has that is not tracked.\n>\n> This leaves the index in an unmerged state, however, so that a subsequent\n> git reset --hard still kills the file. And I just realized that the same\n> goes for merge-resolve. So I'd prefer to abort the merge, leave everything\n> unchanged and tell the user to clean up first.\n\nThat is unfortunately asking for a moon, I am afraid.\n\nIt needs a major restructuring of the code so that the recursive works\nmore like the way resolve works, namely, changing the final \"writeout\"\ninto two phase thing (the first phase making sure nothing is clobbered in\nthe work tree, and then the second phase actually touching the work tree).\n"},{"id":"97947","messageId":"7vljuhwwtg.fsf@gitster.siamese.dyndns.org","threadId":"16671","inReplyTo":"alpine.DEB.1.00.0812151032230.14632@racer","subject":"Re: [PATCH] modify/delete conflict resolution overwrites untracked file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-15T10:35:07Z","receivedAt":"2008-12-15T10:35:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> merge-recursive: do not clobber untracked working tree garbage\n>> ...\n>> +static int would_lose_untracked(const char *path)\n>> +{\n>> +\tint pos = cache_name_pos(path, strlen(path));\n>> +\n>> +\tif (pos < 0)\n>> +\t\tpos = -1 - pos;\n>> +\twhile (pos < active_nr &&\n>> +\t !strcmp(path, active_cache[pos]->name)) {\n>> +\t\t/*\n>> +\t\t * If stage #0, it is definitely tracked.\n>> +\t\t * If it has stage #2 then it was tracked\n>> +\t\t * before this merge started. All other\n>> +\t\t * cases the path was not tracked.\n>> +\t\t */\n>> +\t\tswitch (ce_stage(active_cache[pos])) {\n>> +\t\tcase 0:\n>> +\t\tcase 2:\n>> +\t\t\treturn 0;\n>> +\t\t}\n>> +\t\tpos++;\n>> +\t}\n>> +\treturn file_exists(path);\n>\n> I wonder if it is cheaper to test file_exists() when the index contains a \n> lot of files...\n\n\"cheaper\" than what?\n\nThe test with the index is not about efficiency, but is all about\ncorrectness.\n\nIf the file in the work tree came from the index that represents \"our\"\nbranch, we do not want to say \"yes\" from this function, even when\n(actually, \"especially when\") the path exists in the work tree.\nunpack-trees decided that the path matches the index (otherwise you are\nalready guaranteeing that we wouldn't have come this far, right?) and we\nare about to write out the (potentially partial) merge result to the\nworking tree file.\n"},{"id":"97948","messageId":"e2b179460812150250t6e028330xf0e0ff626c1b6b3c@mail.gmail.com","threadId":"16671","inReplyTo":"7vskopwxej.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] modify/delete conflict resolution overwrites untracked file","fromName":"Mike Ralphson","fromEmail":"mike.ralphson@gmail.com","sentAt":"2008-12-15T10:50:37Z","receivedAt":"2008-12-15T10:50:37Z","isPatch":true,"sender":{"key":"mike.ralphson@gmail.com","avatar":"https://avatars.githubusercontent.com/u/21603?v=4"},"body":"2008/12/15 Junio C Hamano :\n> Clemens Buchacher writes:\n>\n>> On Sun, Dec 14, 2008 at 07:34:46PM -0800, Junio C Hamano wrote:\n>>> merge-recursive: do not clobber untracked working tree garbage\n>>>\n>>> When merge-recursive wanted to create a new file in the work tree (either\n>>> as the final result, or a hint for reference purposes while delete/modify\n>>> conflicts), it unconditionally overwrote an untracked file in the working\n>>> tree. Be careful not to lose whatever the user has that is not tracked.\n>>\n>> This leaves the index in an unmerged state, however, so that a subsequent\n>> git reset --hard still kills the file. And I just realized that the same\n>> goes for merge-resolve. So I'd prefer to abort the merge, leave everything\n>> unchanged and tell the user to clean up first.\n>\n> That is unfortunately asking for a moon, I am afraid.\n>\n> It needs a major restructuring of the code so that the recursive works\n> more like the way resolve works, namely, changing the final \"writeout\"\n> into two phase thing (the first phase making sure nothing is clobbered in\n> the work tree, and then the second phase actually touching the work tree).\n\nI wonder if another approach is workable... to read 'vulnerable'\nuntracked working tree files into a new (temporary, uncommittable)\nstage in the index, perform whatever merging is required, then\nreinstate all entries from the new stage.\n\nThus the merge should normally succeed under the covers, and the\npreviously untracked file(s) would now show up as modified against the\ntracked content.\n\nTwo problems I foresee - potential loss of untracked metadata, and\nensuring we reinstate the untracked contents in all possible paths the\nuser can use to abort or resolve the merge.\n\nMike\n"},{"id":"97950","messageId":"alpine.DEB.1.00.0812151201300.30933@intel-tinevez-2-302","threadId":"16671","inReplyTo":"7vljuhwwtg.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] modify/delete conflict resolution overwrites untracked file","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-12-15T11:03:15Z","receivedAt":"2008-12-15T11:03:15Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 15 Dec 2008, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> >> merge-recursive: do not clobber untracked working tree garbage\n> >> ...\n> >> +static int would_lose_untracked(const char *path)\n> >> +{\n> >> +\tint pos = cache_name_pos(path, strlen(path));\n> >> +\n> >> +\tif (pos < 0)\n> >> +\t\tpos = -1 - pos;\n> >> +\twhile (pos < active_nr &&\n> >> +\t !strcmp(path, active_cache[pos]->name)) {\n> >> +\t\t/*\n> >> +\t\t * If stage #0, it is definitely tracked.\n> >> +\t\t * If it has stage #2 then it was tracked\n> >> +\t\t * before this merge started. All other\n> >> +\t\t * cases the path was not tracked.\n> >> +\t\t */\n> >> +\t\tswitch (ce_stage(active_cache[pos])) {\n> >> +\t\tcase 0:\n> >> +\t\tcase 2:\n> >> +\t\t\treturn 0;\n> >> +\t\t}\n> >> +\t\tpos++;\n> >> +\t}\n> >> +\treturn file_exists(path);\n> >\n> > I wonder if it is cheaper to test file_exists() when the index contains a \n> > lot of files...\n> \n> \"cheaper\" than what?\n\nOops. I meant \"cheaper to test file_exists() _first_\". But thinking \nabout it again, it is probably way more expensive, especially in the cold \ncache case.\n\nSorry for the noise,\nDscho\n"},{"id":"97951","messageId":"alpine.DEB.1.00.0812151206360.30933@intel-tinevez-2-302","threadId":"16671","inReplyTo":"e2b179460812150250t6e028330xf0e0ff626c1b6b3c@mail.gmail.com","subject":"Re: [PATCH] modify/delete conflict resolution overwrites untracked file","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-12-15T11:09:04Z","receivedAt":"2008-12-15T11:09:04Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 15 Dec 2008, Mike Ralphson wrote:\n\n> 2008/12/15 Junio C Hamano :\n> > Clemens Buchacher writes:\n> >\n> >> On Sun, Dec 14, 2008 at 07:34:46PM -0800, Junio C Hamano wrote:\n> >>> merge-recursive: do not clobber untracked working tree garbage\n> >>>\n> >>> When merge-recursive wanted to create a new file in the work tree \n> >>> (either as the final result, or a hint for reference purposes while \n> >>> delete/modify conflicts), it unconditionally overwrote an untracked \n> >>> file in the working tree. Be careful not to lose whatever the user \n> >>> has that is not tracked.\n> >>\n> >> This leaves the index in an unmerged state, however, so that a \n> >> subsequent git reset --hard still kills the file. And I just realized \n> >> that the same goes for merge-resolve. So I'd prefer to abort the \n> >> merge, leave everything unchanged and tell the user to clean up \n> >> first.\n> >\n> > That is unfortunately asking for a moon, I am afraid.\n> >\n> > It needs a major restructuring of the code so that the recursive works \n> > more like the way resolve works, namely, changing the final \"writeout\" \n> > into two phase thing (the first phase making sure nothing is clobbered \n> > in the work tree, and then the second phase actually touching the work \n> > tree).\n> \n> I wonder if another approach is workable... to read 'vulnerable' \n> untracked working tree files into a new (temporary, uncommittable) stage \n> in the index, perform whatever merging is required, then reinstate all \n> entries from the new stage.\n\nI think the solution is not in making things more complicated, but \nsimpler. I agree with Junio that the recursive merge needs a major \nrewrite which respects d/f conflicts and renames in the _design_, not as \nan afterthought.\n\nBesides, I really do not want untracked files to be inserted into a stage. \nRemember, adding something to the index means to hash it, and I do have \nhalf-a-gigabyte untracked data in some of my worktrees.\n\nCiao,\nDscho\n"},{"id":"97952","messageId":"e2b179460812150345i29222edesc64f544a9e4db42c@mail.gmail.com","threadId":"16671","inReplyTo":"alpine.DEB.1.00.0812151206360.30933@intel-tinevez-2-302","subject":"Re: [PATCH] modify/delete conflict resolution overwrites untracked file","fromName":"Mike Ralphson","fromEmail":"mike.ralphson@gmail.com","sentAt":"2008-12-15T11:45:50Z","receivedAt":"2008-12-15T11:45:50Z","isPatch":true,"sender":{"key":"mike.ralphson@gmail.com","avatar":"https://avatars.githubusercontent.com/u/21603?v=4"},"body":"2008/12/15 Johannes Schindelin :\n> On Mon, 15 Dec 2008, Mike Ralphson wrote:\n>> I wonder if another approach is workable... to read 'vulnerable'\n>> untracked working tree files into a new (temporary, uncommittable) stage\n>> in the index, perform whatever merging is required, then reinstate all\n>> entries from the new stage.\n>\n> I think the solution is not in making things more complicated, but\n> simpler. I agree with Junio that the recursive merge needs a major\n> rewrite which respects d/f conflicts and renames in the _design_, not as\n> an afterthought.\n\nYes, it might also score low on not surprising the user. I bow to more\nexperienced heads on matters of clean design and merge strategies.\n\n> Besides, I really do not want untracked files to be inserted into a stage.\n> Remember, adding something to the index means to hash it, and I do have\n> half-a-gigabyte untracked data in some of my worktrees.\n\nGranted, but here we are only talking about files (or clashing\nfile-names) which someone [else] has already added/removed/modified in\nanother branch etc.\n\nMike\n"},{"id":"98005","messageId":"7vej09w0hy.fsf@gitster.siamese.dyndns.org","threadId":"16671","inReplyTo":"7vskopwxej.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] modify/delete conflict resolution overwrites untracked file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-15T22:13:13Z","receivedAt":"2008-12-15T22:13:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Clemens Buchacher writes:\n>\n>> On Sun, Dec 14, 2008 at 07:34:46PM -0800, Junio C Hamano wrote:\n>>> merge-recursive: do not clobber untracked working tree garbage\n>>> \n>>> When merge-recursive wanted to create a new file in the work tree (either\n>>> as the final result, or a hint for reference purposes while delete/modify\n>>> conflicts), it unconditionally overwrote an untracked file in the working\n>>> tree. Be careful not to lose whatever the user has that is not tracked.\n>>\n>> This leaves the index in an unmerged state, however, so that a subsequent\n>> git reset --hard still kills the file. And I just realized that the same\n>> goes for merge-resolve. So I'd prefer to abort the merge, leave everything\n>> unchanged and tell the user to clean up first.\n>\n> That is unfortunately asking for a moon, I am afraid.\n>\n> It needs a major restructuring of the code so that the recursive works\n> more like the way resolve works, namely, changing the final \"writeout\"\n> into two phase thing (the first phase making sure nothing is clobbered in\n> the work tree, and then the second phase actually touching the work tree).\n\nActually, the more I think about it, I do not think this is not something\nwe would even want to do.\n\nBy this, I do not mean the restructuring to bring some sanity to\nmerge-recursive. That is necessary. What I do not think is unnecessary\nis the issue you raise about \"git reset --hard\".\n\nYou can do a merge inside a dirty work tree, and the merge will fail\nwithout clobbering your work tree files that are dirty when it needs to be\nable to overwrite to do its job. The set of \"dirty files\" in this\nsentence of course includes paths that are modified since HEAD, but it\nalso includes also paths that do not exist in HEAD (i.e. \"new files\").\n\nBut we already caution users that you need to know what you are doing when\nworking in such a dirty work tree. Namely, after a failed merge, your\nnext \"git reset --hard\" will blow away your local modifications. And\nlocal modifications in this context includes the files you could have\nadded to the index but you haven't.\n\nBy the way, I think the patch I sent earlier is too complex and\nsuboptimal for an entirely different reason.\n\nThe only reason the codepath for delete/modify in process_entry() wants to\nleave the modified side in the result is because the internal merge done\nwhen the algorithm is coming up with a merged merge bases _must_ be fully\nresolved. There is no such requirement for the final round of the merge\nwhose result is written out to the work tree. Whether the path that was\ninvolved in delete/modify conflict was originally in the index or not, we\nshould just leave it alone in the work tree. The logic I implemented as\nthe would_lose_untracked() function is just overkill.\n"},{"id":"98010","messageId":"20081215230205.GA19538@localhost","threadId":"16671","inReplyTo":"7vej09w0hy.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] modify/delete conflict resolution overwrites untracked file","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2008-12-15T23:02:05Z","receivedAt":"2008-12-15T23:02:05Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Mon, Dec 15, 2008 at 02:13:13PM -0800, Junio C Hamano wrote:\n> You can do a merge inside a dirty work tree, and the merge will fail\n> without clobbering your work tree files that are dirty when it needs to be\n> able to overwrite to do its job. The set of \"dirty files\" in this\n> sentence of course includes paths that are modified since HEAD, but it\n> also includes also paths that do not exist in HEAD (i.e. \"new files\").\n> \n> But we already caution users that you need to know what you are doing when\n> working in such a dirty work tree. Namely, after a failed merge, your\n> next \"git reset --hard\" will blow away your local modifications. And\n> local modifications in this context includes the files you could have\n> added to the index but you haven't.\n\nI strongly disagree. With the suggested behavior I would have to\ndouble-check every single untracked file in my tree for conflicts before\ntrying a throw-away merge followed by git reset --hard, for example. The\nfile could even be ignored! Whatever happened to git reset doesn't touch\nuntracked files?\n\nI would even prefer breaking t6023 (until we can properly implement this\nfeature) in order to avoid that.\n"},{"id":"98013","messageId":"7vprjtug8u.fsf@gitster.siamese.dyndns.org","threadId":"16671","inReplyTo":"20081215230205.GA19538@localhost","subject":"Re: [PATCH] modify/delete conflict resolution overwrites untracked file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-16T00:16:01Z","receivedAt":"2008-12-16T00:16:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Clemens Buchacher writes:\n\n> I strongly disagree. With the suggested behavior I would have to\n> double-check every single untracked file in my tree for conflicts before\n> trying a throw-away merge followed by git reset --hard, for example.\n\nAFAICS, that's not \"suggested behaviour\" for merge, but it is the\nbehaviour that has been true for eternity.\n\nYou can suggest to fix \"reset --hard\", though.\n"},{"id":"98020","messageId":"m3r649c4e7.fsf@localhost.localdomain","threadId":"16671","inReplyTo":"7vprjtug8u.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] modify/delete conflict resolution overwrites untracked file","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-12-16T01:09:22Z","receivedAt":"2008-12-16T01:09:22Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Junio C Hamano writes:\n\n> Clemens Buchacher writes:\n> \n> > I strongly disagree. With the suggested behavior I would have to\n> > double-check every single untracked file in my tree for conflicts before\n> > trying a throw-away merge followed by git reset --hard, for example.\n> \n> AFAICS, that's not \"suggested behaviour\" for merge, but it is the\n> behaviour that has been true for eternity.\n> \n> You can suggest to fix \"reset --hard\", though.\n\nErrr... what about using \"reset --merge\" instead?\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"98836","messageId":"20081228114445.GA8511@localhost","threadId":"16671","inReplyTo":"7vskopwxej.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] modify/delete conflict resolution overwrites untracked file","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2008-12-28T11:44:45Z","receivedAt":"2008-12-28T11:44:45Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Mon, Dec 15, 2008 at 02:22:28AM -0800, Junio C Hamano wrote:\n> Clemens Buchacher writes:\n> > On Sun, Dec 14, 2008 at 07:34:46PM -0800, Junio C Hamano wrote:\n> >> merge-recursive: do not clobber untracked working tree garbage\n> >> \n> >> When merge-recursive wanted to create a new file in the work tree (either\n> >> as the final result, or a hint for reference purposes while delete/modify\n> >> conflicts), it unconditionally overwrote an untracked file in the working\n> >> tree. Be careful not to lose whatever the user has that is not tracked.\n> >\n> > This leaves the index in an unmerged state, however, so that a subsequent\n> > git reset --hard still kills the file. And I just realized that the same\n> > goes for merge-resolve. So I'd prefer to abort the merge, leave everything\n> > unchanged and tell the user to clean up first.\n> \n> That is unfortunately asking for a moon, I am afraid.\n> \n> It needs a major restructuring of the code so that the recursive works\n> more like the way resolve works, namely, changing the final \"writeout\"\n> into two phase thing (the first phase making sure nothing is clobbered in\n> the work tree, and then the second phase actually touching the work tree).\n\nI've been giving this some thought and I would like to propose the following\nsolution:\n\n1. Save index state.\n2. Merge using whichever strategy, in index only, ignoring work tree.\n This step includes rename detection.\n3. Check that work tree files match original index, if index does not match\n HEAD. Otherwise abort, restore index and leave everything unchanged.\n4. Checkout index, overwriting work tree files, and removing files which are\n in HEAD, but not in the index.\n5. If the merge was clean, commit.\n\nAFAICS, this is largely the behavior right now, except that steps 3 and 4\nare intermingled with step 2, which makes it impossible to abort the merge\nif an untracked file is in the way after rename detection.\n\nThe idea at step 3 is that we can decide whether or not to proceed, based\nonly on the merge result, irrespective of the strategy used, possible rename\ndetection, or conflict resolution.\n\nApart from the fact that this seems like the sane thing to do, I want this\nbehavior because it allows me to do\n\ngit merge \n# Conflicts? I don't have time for that now.\ngit reset --hard HEAD\n\nunder all circumstances, without touching any untracked files.\n\nDo you agree that this is a desireable goal?\n\nI have not looked into d/f conflicts, but I am under the impression that\nthis could also be handled at step 3, as far as the work tree is concerned.\nIs the above proposal a workable approach, which I can pursue independently\nof the major rewrite wrt. d/f conflicts Johannes indicated?\n"},{"id":"98849","messageId":"7v3ag89c0b.fsf@gitster.siamese.dyndns.org","threadId":"16671","inReplyTo":"20081228114445.GA8511@localhost","subject":"Re: [PATCH] modify/delete conflict resolution overwrites untracked file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-28T22:21:40Z","receivedAt":"2008-12-28T22:21:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Clemens Buchacher writes:\n\n> I've been giving this some thought and I would like to propose the following\n> solution:\n>\n> 1. Save index state.\n> 2. Merge using whichever strategy, in index only, ignoring work tree.\n> This step includes rename detection.\n> 3. Check that work tree files match original index, if index does not match\n> HEAD. Otherwise abort, restore index and leave everything unchanged.\n> 4. Checkout index, overwriting work tree files, and removing files which are\n> in HEAD, but not in the index.\n> 5. If the merge was clean, commit.\n>\n> AFAICS, this is largely the behavior right now, except that steps 3 and 4\n> are intermingled with step 2, which makes it impossible to abort the merge\n> if an untracked file is in the way after rename detection.\n\nThe description of step 3 above may be a bit too sketchy and simplistic,\nbut I agree, provided if you are talking about what merge-recursive\nbackend does, that is how it ought to do things. The strategy should\ninstead:\n\n - have trivial merge done inside the index, likely leveraging\n unpack_trees() three-way merge mechanism we already have, and it will\n allow you to notice and abort upon seeing local modifications in the\n work tree at this stage, except for renamed paths;\n\n - notice renames for the unmerged ones and deal with them inside the\n index, still without resolving. E.g. in a merge between A and B, if\n side A deleted \"path1\" and created a \"path2\" that is similar to deleted\n \"path1\", have the ancestor version at stage #1 of \"path2\", hoist\n \"path2\" of side A to stage #2, and move the version of \"path1\" from\n side B and have it at stage #3 of \"path2\", which would result in an\n index without \"path1\" in any stage, and \"path2\" in three stages.\n\n If the other side's rename causes a path to be moved, you can/should\n check and notice local modifications to it at this stage.\n\n You can also notice D/F conflict at this phase (e.g. side B in the\n above example may have created a new path \"path2/file1\" and the index\n may have stage #3 entry without any other stages for \"path2/file1\") and\n abort.\n\n - try to resolve unmerged paths inside the index.\n\n Because you matched up possible renames and and adjusted the entries,\n you can resolve aggressively, e.g. a path with identical stage #1 and\n #3 but missing stage #2 results in delete, a path with missing stage #1\n and #2 but with stage #3 results in create, etc.\n\n - notice the paths that need to be written out to the work tree. Paths\n with local changes should have been noticed already in the above, but\n while debugging your new algorithm, you may want to have a logic to\n double check so that you won't miss any. Abort if you find any local\n change that can be clobbered.\n\n - Then, finally, you commit to writing the results out, both the index\n and the work tree. The changes to the work tree may be cleanly merged,\n or an unmerged results with three-way conflict markers.\n\nThis way, you will never have \"merge stops in halfway because it notices\nit will clobber a local change too late after it has already written a\npartial merge results to the work tree\" problem, I think.\n\n> git merge \n> # Conflicts? I don't have time for that now.\n> git reset --hard HEAD\n>\n> under all circumstances, without touching any untracked files.\n\nEven though I agree the above without the second line is a good thing to\nhave, I think your description is a wrong way to present the desirable\ngoal.\n\n\"Conflicts?\" is an indication that you had a lot of other things going on\ninside your work tree as set of local changes that you \"don't have time\nfor that now\", and \"reset --hard\" is a sure way to lose them. You\nshouldn't train your users to say \"--hard\" lightly.\n\nThe issue of unreliable D/F conflict detection and local change detection\nthe current merge-recursive has at its corner case is not about\n\"Conflicts? I don't have time for that now\" at all. It is not about \" I\nhad something precious in my work tree, but the merge turned out to be\nunmanageable for me\", either.\n\nThe issue is about \"Crap, if merge has to abort in the middle because it\nhas to refrain from writing out a half-merged state out, not to overwrite\nmy local changes, it shouldn't have written out anything else to the work\ntree at all. Don't abort in the middle but abort upfront without touching\nmy work tree!\".\n\nI think the correct statement of the goal is:\n\n\t$ git merge ..\n # Ah, I have \"path2\" locally modified so it cannot proceed?\n # I did not lose anything, nor it did anything to my index nor\n # work tree. Good.\n\nBy this, you realize that you should wrap up what you have been doing\nfirst, because the above is an indication that other end has made some\noverlapping changes. You can either (1) decide not to merge at this point\nbecause you are not ready, keep working on what you were doing first and\nlater merge, or (2) stash away what you have been doing and do the merge\nfirst, and then unstash.\n"},{"id":"98851","messageId":"20081228235317.GA32714@localhost","threadId":"16671","inReplyTo":"7v3ag89c0b.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] modify/delete conflict resolution overwrites untracked file","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2008-12-28T23:53:17Z","receivedAt":"2008-12-28T23:53:17Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Sun, Dec 28, 2008 at 02:21:40PM -0800, Junio C Hamano wrote:\n> Clemens Buchacher writes:\n> \n> > I've been giving this some thought and I would like to propose the following\n> > solution:\n> >\n> > 1. Save index state.\n> > 2. Merge using whichever strategy, in index only, ignoring work tree.\n> > This step includes rename detection.\n> > 3. Check that work tree files match original index, if index does not match\n> > HEAD. Otherwise abort, restore index and leave everything unchanged.\n> > 4. Checkout index, overwriting work tree files, and removing files which are\n> > in HEAD, but not in the index.\n> > 5. If the merge was clean, commit.\n> >\n> > AFAICS, this is largely the behavior right now, except that steps 3 and 4\n> > are intermingled with step 2, which makes it impossible to abort the merge\n> > if an untracked file is in the way after rename detection.\n> \n> The description of step 3 above may be a bit too sketchy and simplistic,\n> but I agree, provided if you are talking about what merge-recursive\n> backend does, that is how it ought to do things. The strategy should\n> instead:\n\nActually, I was hoping to find a solution that's independent of the merge\nstrategy. I think the merge strategy should not have to concern itself with\nthe work tree at all.\n\nRegarding my description of step 3, I worded it carefully to cover all\ncases. My assumption at this stage is that the merge strategy took care not\nto overwrite any staged changes, unless the merge is trivial. It is\ntherefore safe to overwrite the work tree file if it matches either the\noriginal index, or HEAD.\n\n> - have trivial merge done inside the index, likely leveraging\n> unpack_trees() three-way merge mechanism we already have, and it will\n> allow you to notice and abort upon seeing local modifications in the\n> work tree at this stage, except for renamed paths;\n\nRight, but why would we want to do it this early? Is there a performance\nissue? If not, I argue that we don't care about the work tree at this stage.\nOnly the end result matters.\n\n> - notice renames for the unmerged ones and deal with them inside the\n> index, still without resolving. E.g. in a merge between A and B, if\n> side A deleted \"path1\" and created a \"path2\" that is similar to deleted\n> \"path1\", have the ancestor version at stage #1 of \"path2\", hoist\n> \"path2\" of side A to stage #2, and move the version of \"path1\" from\n> side B and have it at stage #3 of \"path2\", which would result in an\n> index without \"path1\" in any stage, and \"path2\" in three stages.\n\nRight. So in this case we do not abort if \"path1\" had unstaged\nchanges, because \"path1\" of B was moved to \"path2\" anyways. Conversely, if\n\"path2\" had unstaged changes, we do abort, because it would be overwritten\nby the merge (or conflict resolution). All this can be decided without any\nknowledge about detected renames or the merge strategy used.\n\n> If the other side's rename causes a path to be moved, you can/should\n> check and notice local modifications to it at this stage.\n> \n> You can also notice D/F conflict at this phase (e.g. side B in the\n> above example may have created a new path \"path2/file1\" and the index\n> may have stage #3 entry without any other stages for \"path2/file1\") and\n> abort.\n> \n> - try to resolve unmerged paths inside the index.\n> \n> Because you matched up possible renames and and adjusted the entries,\n> you can resolve aggressively, e.g. a path with identical stage #1 and\n> #3 but missing stage #2 results in delete, a path with missing stage #1\n> and #2 but with stage #3 results in create, etc.\n\nThis should be done before checking for D/F conflicts. If a file ends up\ngetting removed, the D/F conflict is avoided.\n\n> - notice the paths that need to be written out to the work tree. Paths\n> with local changes should have been noticed already in the above, but\n> while debugging your new algorithm, you may want to have a logic to\n> double check so that you won't miss any. Abort if you find any local\n> change that can be clobbered.\n> \n> - Then, finally, you commit to writing the results out, both the index\n> and the work tree. The changes to the work tree may be cleanly merged,\n> or an unmerged results with three-way conflict markers.\n>\n> This way, you will never have \"merge stops in halfway because it notices\n> it will clobber a local change too late after it has already written a\n> partial merge results to the work tree\" problem, I think.\n\nRight. So all we have to do is keep unpack_trees() and process_entry() in\nmerge_trees() from writing to the work tree and delay until after\ntry_merge_strategy() has completed. Then we check for D/F conflicts and\ndirty work tree files before finally checking out the merge result.\n\nThinking about this now, I wonder what happens if recursive merge has a D/F\nconflict in one of its recursions. There is nothing smart it can do, so it\ncould continue with a D/F conflict in the index, hoping that it goes away in\nthe final result. So again, D/F conflicts should not matter for the merge\nstrategy.\n\n> > git merge \n> > # Conflicts? I don't have time for that now.\n> > git reset --hard HEAD\n> >\n> > under all circumstances, without touching any untracked files.\n> \n> Even though I agree the above without the second line is a good thing to\n> have, I think your description is a wrong way to present the desirable\n> goal.\n[...]\n> The issue is about \"Crap, if merge has to abort in the middle because it\n> has to refrain from writing out a half-merged state out, not to overwrite\n> my local changes, it shouldn't have written out anything else to the work\n> tree at all. Don't abort in the middle but abort upfront without touching\n> my work tree!\".\n\nPoint taken.\n"}]}