{"thread":{"id":"26768","subject":"[PATCH] diffcore-rename: don't consider unmerged path as source","startedAt":"2011-03-18T01:42:28Z","lastAt":"2011-03-24T02:41:01Z","messageCount":6,"participants":["Martin von Zweigbergk","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"163605","messageId":"1300412548-1724-1-git-send-email-martin.von.zweigbergk@gmail.com","threadId":"26768","inReplyTo":null,"subject":"[PATCH] diffcore-rename: don't consider unmerged path as source","fromName":"Martin von Zweigbergk","fromEmail":"martin.von.zweigbergk@gmail.com","sentAt":"2011-03-18T01:42:28Z","receivedAt":"2011-03-18T01:42:28Z","isPatch":true,"sender":{"key":"martinvonz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/891642?v=4"},"body":"The output from 'git status' and 'git diff-index --cached -M' is\nbroken when there are unmerged files as well as new files similar to\nthe unmerged file (in stage 1).\n\nWhen two copies have been created the output is:\n\n$ git status -s\nC  original -> exact-copy\nR  original -> modified-copy\n U original\n\nThere are several errors here: 1) \"original\" has invalid status \" U\",\n2) \"modified-copy\" is listed as a rename, even though its source\n(\"original\") still exists, and 3) \"exact-copy\" is detected as a copy,\neven though 'git status' is only supposed to show renames\n\nIn the verbose 'git status' output, the unmerged path is completely\nmissing. 'git diff-index --cached -M' show similar output.\n\nFix these problems by making diffcore-rename not consider unmerged\npaths as source for rename detection. For simplicty, don't consider\nthe path independent of the type of merge conflict, even if the path\nis deleted on at least one side. Still consider unmerged paths as\nsource for copy detection.\n\nWhile at it, update the users of diff_options to use\nDIFF_DETECT_RENAME instead of directly using the value 1 to enable\nrename detection.\n\nSigned-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>\n---\n\nThis is the first time I look at the diff code, so please review\ncarefully. I think the changes make sense, but I really don't know the\ncode enough to be sure.\n\nAlso not sure about the \"while at it\" stuff...\n\nThe test cases assume that the paths will be printed in a certain\norder. Can I rely on that?\n\n builtin/commit.c                    |    2 +-\n diffcore-rename.c                   |    7 +++-\n t/t7510-status-merge-rename-copy.sh |   68 +++++++++++++++++++++++++++++++++++\n wt-status.c                         |    6 ++--\n 4 files changed, 77 insertions(+), 6 deletions(-)\n create mode 100755 t/t7510-status-merge-rename-copy.sh\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 82092e5..1f9f314 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1284,7 +1284,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1)\n \trev.show_root_diff = 1;\n \tget_commit_format(format.buf, &rev);\n \trev.always_show_header = 0;\n-\trev.diffopt.detect_rename = 1;\n+\trev.diffopt.detect_rename = DIFF_DETECT_RENAME;\n \trev.diffopt.rename_limit = 100;\n \trev.diffopt.break_opt = 0;\n \tdiff_setup_done(&rev.diffopt);\ndiff --git a/diffcore-rename.c b/diffcore-rename.c\nindex 0cd4c13..c53ca36 100644\n--- a/diffcore-rename.c\n+++ b/diffcore-rename.c\n@@ -464,7 +464,7 @@ void diffcore_rename(struct diff_options *options)\n \t\t\telse\n \t\t\t\tlocate_rename_dst(p->two, 1);\n \t\t}\n-\t\telse if (!DIFF_FILE_VALID(p->two)) {\n+\t\telse if (!DIFF_PAIR_UNMERGED(p) && !DIFF_FILE_VALID(p->two)) {\n \t\t\t/*\n \t\t\t * If the source is a broken \"delete\", and\n \t\t\t * they did not really want to get broken,\n@@ -575,7 +575,10 @@ void diffcore_rename(struct diff_options *options)\n \t\tstruct diff_filepair *p = q->queue[i];\n \t\tstruct diff_filepair *pair_to_free = NULL;\n \n-\t\tif (!DIFF_FILE_VALID(p->one) && DIFF_FILE_VALID(p->two)) {\n+\t\tif (DIFF_PAIR_UNMERGED(p)) {\n+\t\t\tdiff_q(&outq, p);\n+\t\t}\n+\t\telse if (!DIFF_FILE_VALID(p->one) && DIFF_FILE_VALID(p->two)) {\n \t\t\t/*\n \t\t\t * Creation\n \t\t\t *\ndiff --git a/t/t7510-status-merge-rename-copy.sh b/t/t7510-status-merge-rename-copy.sh\nnew file mode 100755\nindex 0000000..8c7a4d2\n--- /dev/null\n+++ b/t/t7510-status-merge-rename-copy.sh\n@@ -0,0 +1,68 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2011 Martin von Zweigbergk\n+#\n+\n+test_description='git status during merge with copies'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\techo a b c | xargs -n1 >original &&\n+\tgit add original &&\n+\tgit commit -m initial &&\n+\n+\tcp original exact-copy &&\n+\tcp original modified-copy &&\n+\techo x >>original &&\n+\techo y >>modified-copy &&\n+\tgit add original exact-copy modified-copy &&\n+\tgit commit -m \"create copies and modify original on master\" &&\n+\n+\tgit checkout -b topic HEAD^ &&\n+\techo z >>original &&\n+\tgit add original &&\n+\tgit commit -m \"modify original on topic\" &&\n+\ttest_must_fail git merge master\n+'\n+\n+cat >expected <<\\EOF\n+A  exact-copy\n+A  modified-copy\n+UU original\n+EOF\n+\n+test_expect_success 'git status -s shows 2 added + 1 unmerged' '\n+\tgit status -s -uno >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+cat >expected <<\\EOF\n+A\texact-copy\n+A\tmodified-copy\n+U\toriginal\n+EOF\n+\n+test_expect_success 'git diff-index --cached shows 2 added + 1 unmerged' '\n+\tgit diff-index --cached --name-status HEAD >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'git diff-index --cached -M shows 2 added + 1 unmerged' '\n+\tgit diff-index --cached --name-status HEAD >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+cat >expected <<\\EOF\n+C\toriginal\texact-copy\n+C\toriginal\tmodified-copy\n+U\toriginal\n+EOF\n+\n+test_expect_success 'git diff-index --cached -C shows 2 copies + 1 unmerged' '\n+\tgit diff-index --cached -C --name-status HEAD |\n+\t\tsed \"s/^C[0-9]*/C/g\" >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_done\ndiff --git a/wt-status.c b/wt-status.c\nindex 4daa8bb..532bbcc 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -320,7 +320,7 @@ static void wt_status_collect_changes_worktree(struct wt_status *s)\n \tif (s->ignore_submodule_arg) {\n \t\tDIFF_OPT_SET(&rev.diffopt, OVERRIDE_SUBMODULE_CONFIG);\n \t\thandle_ignore_submodules_arg(&rev.diffopt, s->ignore_submodule_arg);\n-    }\n+\t}\n \trev.diffopt.format_callback = wt_status_collect_changed_cb;\n \trev.diffopt.format_callback_data = s;\n \tinit_pathspec(&rev.prune_data, s->pathspec);\n@@ -345,7 +345,7 @@ static void wt_status_collect_changes_index(struct wt_status *s)\n \trev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;\n \trev.diffopt.format_callback = wt_status_collect_updated_cb;\n \trev.diffopt.format_callback_data = s;\n-\trev.diffopt.detect_rename = 1;\n+\trev.diffopt.detect_rename = DIFF_DETECT_RENAME;\n \trev.diffopt.rename_limit = 200;\n \trev.diffopt.break_opt = 0;\n \tinit_pathspec(&rev.prune_data, s->pathspec);\n@@ -597,7 +597,7 @@ static void wt_status_print_verbose(struct wt_status *s)\n \tsetup_revisions(0, NULL, &rev, &opt);\n \n \trev.diffopt.output_format |= DIFF_FORMAT_PATCH;\n-\trev.diffopt.detect_rename = 1;\n+\trev.diffopt.detect_rename = DIFF_DETECT_RENAME;\n \trev.diffopt.file = s->fp;\n \trev.diffopt.close_file = 0;\n \t/*\n-- \n1.7.4.79.gcbe20\n"},{"id":"163608","messageId":"7vmxkszzw5.fsf@alter.siamese.dyndns.org","threadId":"26768","inReplyTo":"1300412548-1724-1-git-send-email-martin.von.zweigbergk@gmail.com","subject":"Re: [PATCH] diffcore-rename: don't consider unmerged path as source","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-18T06:49:46Z","receivedAt":"2011-03-18T06:49:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:\n\n> The output from 'git status' and 'git diff-index --cached -M' is\n> broken when there are unmerged files as well as new files similar to\n> the unmerged file (in stage 1).\n>\n> When two copies have been created the output is:\n>\n> $ git status -s\n> C  original -> exact-copy\n> R  original -> modified-copy\n>  U original\n\nI think the above actually sort-of makes sense.  The first column of\nstatus output is about comparing the HEAD and the index, and the second\ncolumn is about comparing the index and the working tree, but I'll begin\nby explaining the latter.\n\nWhen comparing an unmerged index with the working tree, the comparison\nitself does not make much sense.  When producing textual diff,\n\"diff-index\" tends to give the difference between stage #2 and the working\ntree in order to show the difference from \"our\" version and the result of\nthe incomplete merge, but when we need to show the result concisely in the\n\"status -s\" output, the fact that the index is unmerged is more important\nthan the incomplete merge result in the working tree is different from the\noriginal, so we show \"U\".\n\nSo I think \"U\" is perfectly good there.\n\nAbout the comparison between HEAD and index, \"original\" in HEAD is copied\nto \"exact-copy\" in the index, and \"modified-copy\" in the index has a very\nsimilar contents as \"original\" in HEAD. It may be a bug that the latter is\nshown as \"R\" and I suspect that is because the code mistook the unmerged\nentry in the index as missing.  Turning that \"R\" to \"C\" may be worth\ndoing. Change the code that says \"ah, the index is unmerged at this path,\nso treat it as if it is not there\" to \"if the unmerged path does not have\nstage #2 entry, it is missing\".\n\n> There are several errors here: 1) \"original\" has invalid status \" U\",\n> 2) \"modified-copy\" is listed as a rename, even though its source\n> (\"original\") still exists, and 3) \"exact-copy\" is detected as a copy,\n> even though 'git status' is only supposed to show renames\n\nThe prose is good but if you illustrated a bug with a command output,\nplease follow it up with another command output that you think is the\nright output.  It becomes easier to point out potential flaws in the\ndesign and the thought behind it, like I just did above about \"U\".\n\nI don't think anybody said \"only supposed to show renames\", but I suspect\nthat the recent lt/diff-rename topic may affect this part of the output.\n\n> Fix these problems by making diffcore-rename not consider unmerged\n> paths as source for rename detection. For simplicty, don't consider\n> the path independent of the type of merge conflict, even if the path\n> is deleted on at least one side. Still consider unmerged paths as\n> source for copy detection.\n\nI don't think the part after \"Still...\" is justified enough.\n\nFor that matter, everything after \"For simplicity...\" is not justified\nenough. What are you sacrificing in return for that simplicity?  \"Ideally\nwe should show X but because we don't consider these we end up getting Y\nbut that still is better than what we get today which is Z\" is missing.\n\n> Also not sure about the \"while at it\" stuff...\n\nBecause \"while at it\" is by definition stuff that is not essential, don't\ndo \"white at it\" if you are not sure, if it adds unnecessary noise and\nburden on reviewers.\n"},{"id":"163634","messageId":"alpine.DEB.2.00.1103180717230.7736@debian","threadId":"26768","inReplyTo":"7vmxkszzw5.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] diffcore-rename: don't consider unmerged path as source","fromName":"Martin von Zweigbergk","fromEmail":"martin.von.zweigbergk@gmail.com","sentAt":"2011-03-18T14:00:44Z","receivedAt":"2011-03-18T14:00:44Z","isPatch":true,"sender":{"key":"martinvonz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/891642?v=4"},"body":"On Thu, 17 Mar 2011, Junio C Hamano wrote:\n\n> Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:\n> \n> > The output from 'git status' and 'git diff-index --cached -M' is\n> > broken when there are unmerged files as well as new files similar to\n> > the unmerged file (in stage 1).\n> >\n> > When two copies have been created the output is:\n> >\n> > $ git status -s\n> > C  original -> exact-copy\n> > R  original -> modified-copy\n> >  U original\n> \n> The first column of\n> status output is about comparing the HEAD and the index, and the second\n> column is about comparing the index and the working tree\n\nAccording to git-status's man page, that is true for paths without\nmerge conflicts only. For paths with merge conflicts, the two letters\nrepresent each side of the merge. That is why I thought \" U\" (note the\nSP) was invalid. Reading the documentation again, I see that ' '\nactually means 'unmodified'. I'm still not sure it is correct\nthough. First, \"original\" was modified on both sides in this case,\nwhich is of course the reason it had a merge conflict. Second, the\nmeaning of \" U\" is not described in the table in the man page. I have\nalways assumed that the table is supposed to be complete.\n\nEither way, the fact that \"original\" is completetly missing from the\nverbose 'git status' output must surely be incorrect.\n\n> When comparing an unmerged index with the working tree, the comparison\n> itself does not make much sense.  When producing textual diff,\n> \"diff-index\" tends to give the difference between stage #2 and the working\n> tree in order to show the difference from \"our\" version and the result of\n> the incomplete merge, but when we need to show the result concisely in the\n> \"status -s\" output, the fact that the index is unmerged is more important\n> than the incomplete merge result in the working tree is different from the\n> original, so we show \"U\".\n> \n> So I think \"U\" is perfectly good there.\n\nFor completeness, I should say that I think the output from 'git\ndiff-files' is correct (and unchanged by this patch); only 'git\nstatus' and 'git diff-index' showed incorrect (as far as I can see)\noutput.\n\n$ git diff-files --name-status\nU\toriginal\nM\toriginal\n\nSame output with '-M' or '-C'.\n\n> About the comparison between HEAD and index, \"original\" in HEAD is copied\n> to \"exact-copy\" in the index, and \"modified-copy\" in the index has a very\n> similar contents as \"original\" in HEAD. It may be a bug that the latter is\n> shown as \"R\" and I suspect that is because the code mistook the unmerged\n> entry in the index as missing.  Turning that \"R\" to \"C\" may be worth\n> doing. Change the code that says \"ah, the index is unmerged at this path,\n> so treat it as if it is not there\" to \"if the unmerged path does not have\n> stage #2 entry, it is missing\".\n\nExactly. That is what this patch tries to do (although not for 'git\nstatus', but for 'git diff-index --cached -C', see later answers).\n\nSpeaking of stage #2, I think my references to stage #1 in the commit\nmessage should have been referring to stage #2.\n\n> > There are several errors here: 1) \"original\" has invalid status \" U\",\n> > 2) \"modified-copy\" is listed as a rename, even though its source\n> > (\"original\") still exists, and 3) \"exact-copy\" is detected as a copy,\n> > even though 'git status' is only supposed to show renames\n> \n> The prose is good but if you illustrated a bug with a command output,\n> please follow it up with another command output that you think is the\n> right output.\n\nMakes sense. This is the output after this patch:\n\n$ git status -s\nA  exact-copy\nA  modified-copy\nUU original\n\n> I don't think anybody said \"only supposed to show renames\", but I suspect\n> that the recent lt/diff-rename topic may affect this part of the output.\n\nwt-status.c sets rev.diffopt.detect_rename = 1 (==\nDIFF_DETECT_RENAME). I think that means that it should only detect\nrename, not copies. With the patch applies, the files show up as added\n(as show in above). I tried changing it to DIFF_DETECT_COPY and then,\nnot surprisingly, 'git status' showed them as copies instead (even\nwith the patch applied).\n\nWhile on the subject, I almost asked why 'git status' does not detect\ncopies, but I never did. I kind of suspected that it might be too\nslow. Could there be some other reason?\n\n> > Fix these problems by making diffcore-rename not consider unmerged\n> > paths as source for rename detection. For simplicty, don't consider\n> > the path independent of the type of merge conflict, even if the path\n> > is deleted on at least one side. Still consider unmerged paths as\n> > source for copy detection.\n> \n> I don't think the part after \"Still...\" is justified enough.\n> \n> For that matter, everything after \"For simplicity...\" is not justified\n> enough. What are you sacrificing in return for that simplicity?  \"Ideally\n> we should show X but because we don't consider these we end up getting Y\n> but that still is better than what we get today which is Z\" is missing.\n\nI wasn't really sure how it should work, so I just punted and said\n\"For simplicity\" :-). What I was not sure about was what stage to\ncompare to. For example, for \"deleted by us\" conflicts, the most\nlikely outcome is that the file will be deleted, so it could make some\nsense to make it eligible for rename detection. This would mean that\nif a similar file was added by \"them\", it would be described as a\nrename in the git-status output. The same applies to \"deleted by us\",\nbut if I understand correctly, we don't have the stage #3 content\navailable to compare to at this point in the code, and even if we did,\nit would be inconsistent with how the diff is done with stage #2\neverywhere else.\n\nI don't have time to think more about this right now, but I hope the\nabove explains what I simplfied from. I'll think a bit more about this\nlater and might get back with another mail.\n\n> > Also not sure about the \"while at it\" stuff...\n> \n> Because \"while at it\" is by definition stuff that is not essential, don't\n> do \"white at it\" if you are not sure, if it adds unnecessary noise and\n> burden on reviewers.\n\nI changed it because I thought it is slightly more clear that\nDIFF_DETECT_RENAME, as opposed to 1, does not detect renames. I could\neither explain that reason in the commit message, do it in a separate\npatch, or just drop it completely. I don't have a strong\nopinion. Which would you prefer?\n\nThanks for reviewing. I will try to include much of the above in the\ncommit message in a re-roll.\n\n\n/Martin\n"},{"id":"163870","messageId":"7vk4fsucce.fsf@alter.siamese.dyndns.org","threadId":"26768","inReplyTo":"1300412548-1724-1-git-send-email-martin.von.zweigbergk@gmail.com","subject":"Re: [PATCH] diffcore-rename: don't consider unmerged path as source","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-21T08:06:25Z","receivedAt":"2011-03-21T08:06:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:\n\n> This is the first time I look at the diff code, so please review\n> carefully. I think the changes make sense, but I really don't know the\n> code enough to be sure.\n>\n> Also not sure about the \"while at it\" stuff...\n>\n> The test cases assume that the paths will be printed in a certain\n> order. Can I rely on that?\n\nI re-read the patch and found that the core of the patch, i.e. the change\nto diffcore-rename.c, is basically sound.\n\nI'd like to see the commit log message to describe the cause of the\nbreakage better.  Perhaps something like this:\n\n  Since e9c8409 (diff-index --cached --raw: show tree entry on the LHS for\n  unmerged entries., 2007-01-05), an unmerged entry should be detected by\n  using DIFF_PAIR_UNMERGED(p), not by noticing both one and two sides of\n  the filepair records mode=0 entries. However, it forgot to update some\n  parts of the rename detection logic.\n\n  This only makes difference in the \"diff --cached\" codepath where an\n  unmerged filepair carries information on the entries that came from the\n  tree.  It probably hasn't been noticed for a long time because nobody\n  would run \"diff -M\" during a conflict resolution, but \"git status\" uses\n  rename detection when it internally runs \"diff-index\" and \"diff-files\"\n  and gives nonsense results.\n\n  In an unmerged pair, \"one\" side can have a valid filespec to record the\n  tree entry (e.g. what's in HEAD) when running \"diff --cached\". This can\n  be used as a rename source to other paths in the index that are not\n  unmerged. The path that is unmerged by definition does not have the\n  final content yet (i.e. \"two\" side cannot have a valid filespec), so it\n  can never be a rename destination.\n\n  Use the DIFF_PAIR_UNMERGED() to detect unmerged filepair correctly, and\n  allow the valid \"one\" side of an unmerged filepair to be considered a\n  potential rename source, but never to be considered a rename destination.\n\nPlease split changes to wt-status.c (indentation and symbolic constant)\nand builtin/commit.c (symbolic constant) to a single commit that is\nseparate from this fix, as we would want to backport the fix to older\nmaintenance tracks.\n\nAlso, please don't add a new test script 7510 that conflicts what is\nalready in flight ('pu' has t/t7510-commit-notes.sh).  Instead, tack\nsomething like the following (and your diff-index tests) at the end of\nt/t7060-wtstatus.sh, as you should be able to do this without actually\nrunning a merge.\n\nAnswering the last question in the comment part of your message, the paths\nare supposed to be shown in the name order, so your comparison (and the\ncomparison below) should be the right thing to do.\n\nThanks.\n\n\n\ntest_expect_success 'rename & unmerged setup' '\n\tgit rm -f -r . &&\n\tcat \"$TEST_DIRECTORY/README\" >ONE &&\n\tgit add ONE &&\n\ttest_tick &&\n\tgit commit -m \"One commit with ONE\" &&\n\n\techo Modified >TWO &&\n\tcat ONE >>TWO &&\n\tcat ONE >>THREE &&\n\tgit add TWO THREE &&\n\tsha1=$(git rev-parse :ONE) &&\n\tgit rm --cached ONE &&\n\t(\n\t\techo \"100644 $sha1 1\tONE\" &&\n\t\techo \"100644 $sha1 2\tONE\" &&\n\t\techo \"100644 $sha1 3\tONE\"\n\t) | git update-index --index-info &&\n\techo Further >>THREE\n'\n\ntest_expect_success 'rename & unmerged status' '\n\tgit status -suno >actual &&\n\tcat >expect <<-EOF &&\n\tUU ONE\n\tAM THREE\n\tA  TWO\n\tEOF\n\ttest_cmp expect actual\n'\n"},{"id":"164196","messageId":"alpine.DEB.2.00.1103232043530.30954@debian","threadId":"26768","inReplyTo":"7vk4fsucce.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] diffcore-rename: don't consider unmerged path as source","fromName":"Martin von Zweigbergk","fromEmail":"martin.von.zweigbergk@gmail.com","sentAt":"2011-03-24T00:52:46Z","receivedAt":"2011-03-24T00:52:46Z","isPatch":true,"sender":{"key":"martinvonz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/891642?v=4"},"body":"On Mon, 21 Mar 2011, Junio C Hamano wrote:\n\n> Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:\n> \n> > This is the first time I look at the diff code, so please review\n> > carefully. I think the changes make sense, but I really don't know the\n> > code enough to be sure.\n> >\n> > Also not sure about the \"while at it\" stuff...\n> >\n> > The test cases assume that the paths will be printed in a certain\n> > order. Can I rely on that?\n> \n> I re-read the patch and found that the core of the patch, i.e. the change\n> to diffcore-rename.c, is basically sound.\n> \n> I'd like to see the commit log message to describe the cause of the\n> breakage better.  Perhaps something like this:\n> \n>   Since e9c8409 (diff-index --cached --raw: show tree entry on the LHS for\n>   unmerged entries., 2007-01-05), an unmerged entry should be detected by\n>   using DIFF_PAIR_UNMERGED(p), not by noticing both one and two sides of\n>   the filepair records mode=0 entries. However, it forgot to update some\n>   parts of the rename detection logic.\n> \n>   This only makes difference in the \"diff --cached\" codepath where an\n>   unmerged filepair carries information on the entries that came from the\n>   tree.  It probably hasn't been noticed for a long time because nobody\n>   would run \"diff -M\" during a conflict resolution, but \"git status\" uses\n>   rename detection when it internally runs \"diff-index\" and \"diff-files\"\n>   and gives nonsense results.\n> \n>   In an unmerged pair, \"one\" side can have a valid filespec to record the\n>   tree entry (e.g. what's in HEAD) when running \"diff --cached\". This can\n>   be used as a rename source to other paths in the index that are not\n>   unmerged. The path that is unmerged by definition does not have the\n>   final content yet (i.e. \"two\" side cannot have a valid filespec), so it\n>   can never be a rename destination.\n> \n>   Use the DIFF_PAIR_UNMERGED() to detect unmerged filepair correctly, and\n>   allow the valid \"one\" side of an unmerged filepair to be considered a\n>   potential rename source, but never to be considered a rename destination.\n\nThanks. I really appreciate it. My commit message did feel a little\nlazy and I didn't know the history.\n\n> Please split changes to wt-status.c (indentation and symbolic constant)\n> and builtin/commit.c (symbolic constant) to a single commit that is\n> separate from this fix, as we would want to backport the fix to older\n> maintenance tracks.\n\nAh, of course. I did think this should go to maint, so it seems stupid\nnot to have split it. Will do.\n\n> Also, please don't add a new test script 7510 that conflicts what is\n> already in flight ('pu' has t/t7510-commit-notes.sh).\n\nSorry, I missed that and thanks for the help below. I have not had\nmuch time lately, but I will do my best to send a re-roll in not too\nlong.\n\n>  Instead, tack\n> something like the following (and your diff-index tests) at the end of\n> t/t7060-wtstatus.sh, as you should be able to do this without actually\n> running a merge.\n> \n> Answering the last question in the comment part of your message, the paths\n> are supposed to be shown in the name order, so your comparison (and the\n> comparison below) should be the right thing to do.\n> \n> Thanks.\n> \n> \n> \n> test_expect_success 'rename & unmerged setup' '\n> \tgit rm -f -r . &&\n> \tcat \"$TEST_DIRECTORY/README\" >ONE &&\n> \tgit add ONE &&\n> \ttest_tick &&\n> \tgit commit -m \"One commit with ONE\" &&\n> \n> \techo Modified >TWO &&\n> \tcat ONE >>TWO &&\n> \tcat ONE >>THREE &&\n> \tgit add TWO THREE &&\n> \tsha1=$(git rev-parse :ONE) &&\n> \tgit rm --cached ONE &&\n> \t(\n> \t\techo \"100644 $sha1 1\tONE\" &&\n> \t\techo \"100644 $sha1 2\tONE\" &&\n> \t\techo \"100644 $sha1 3\tONE\"\n> \t) | git update-index --index-info &&\n> \techo Further >>THREE\n> '\n> \n> test_expect_success 'rename & unmerged status' '\n> \tgit status -suno >actual &&\n> \tcat >expect <<-EOF &&\n> \tUU ONE\n> \tAM THREE\n> \tA  TWO\n> \tEOF\n> \ttest_cmp expect actual\n> '\n> \n"},{"id":"164198","messageId":"1300934461-28867-1-git-send-email-martin.von.zweigbergk@gmail.com","threadId":"26768","inReplyTo":"1300412548-1724-1-git-send-email-martin.von.zweigbergk@gmail.com","subject":"[PATCH v2] diffcore-rename: don't consider unmerged path as source","fromName":"Martin von Zweigbergk","fromEmail":"martin.von.zweigbergk@gmail.com","sentAt":"2011-03-24T02:41:01Z","receivedAt":"2011-03-24T02:41:01Z","isPatch":true,"sender":{"key":"martinvonz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/891642?v=4"},"body":"Since e9c8409 (diff-index --cached --raw: show tree entry on the LHS for\nunmerged entries., 2007-01-05), an unmerged entry should be detected by\nusing DIFF_PAIR_UNMERGED(p), not by noticing both one and two sides of\nthe filepair records mode=0 entries. However, it forgot to update some\nparts of the rename detection logic.\n\nThis only makes difference in the \"diff --cached\" codepath where an\nunmerged filepair carries information on the entries that came from the\ntree.  It probably hasn't been noticed for a long time because nobody\nwould run \"diff -M\" during a conflict resolution, but \"git status\" uses\nrename detection when it internally runs \"diff-index\" and \"diff-files\"\nand gives nonsense results.\n\nIn an unmerged pair, \"one\" side can have a valid filespec to record the\ntree entry (e.g. what's in HEAD) when running \"diff --cached\". This can\nbe used as a rename source to other paths in the index that are not\nunmerged. The path that is unmerged by definition does not have the\nfinal content yet (i.e. \"two\" side cannot have a valid filespec), so it\ncan never be a rename destination.\n\nUse the DIFF_PAIR_UNMERGED() to detect unmerged filepair correctly, and\nallow the valid \"one\" side of an unmerged filepair to be considered a\npotential rename source, but never to be considered a rename destination.\n\nCommit message and first two test cases by Junio, the rest by Martin.\n\nHelped-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>\n---\nI think I mean something slightly different when I say \"don't consider\nunmerged path as source\" than you do when you say \"allow the valid\n\"one\" side of an unmerged filepair to be considered a potential rename\nsource\". I mean that they shouldn't show up on the LHS of a \"copied: A\n-> B\" line in git-status output. I think you mean that they should be\nregistered as rename sources, even though the destination will always\nbe the file itself, so other files will only end up having status\nCOPIED. I didn't know how phrase the subject line in a better way, but\nfeel free to change it as you like.\n\n\n diffcore-rename.c   |    7 ++++-\n t/t7060-wtstatus.sh |   59 +++++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 64 insertions(+), 2 deletions(-)\n\ndiff --git a/diffcore-rename.c b/diffcore-rename.c\nindex 0cd4c13..c53ca36 100644\n--- a/diffcore-rename.c\n+++ b/diffcore-rename.c\n@@ -464,7 +464,7 @@ void diffcore_rename(struct diff_options *options)\n \t\t\telse\n \t\t\t\tlocate_rename_dst(p->two, 1);\n \t\t}\n-\t\telse if (!DIFF_FILE_VALID(p->two)) {\n+\t\telse if (!DIFF_PAIR_UNMERGED(p) && !DIFF_FILE_VALID(p->two)) {\n \t\t\t/*\n \t\t\t * If the source is a broken \"delete\", and\n \t\t\t * they did not really want to get broken,\n@@ -575,7 +575,10 @@ void diffcore_rename(struct diff_options *options)\n \t\tstruct diff_filepair *p = q->queue[i];\n \t\tstruct diff_filepair *pair_to_free = NULL;\n \n-\t\tif (!DIFF_FILE_VALID(p->one) && DIFF_FILE_VALID(p->two)) {\n+\t\tif (DIFF_PAIR_UNMERGED(p)) {\n+\t\t\tdiff_q(&outq, p);\n+\t\t}\n+\t\telse if (!DIFF_FILE_VALID(p->one) && DIFF_FILE_VALID(p->two)) {\n \t\t\t/*\n \t\t\t * Creation\n \t\t\t *\ndiff --git a/t/t7060-wtstatus.sh b/t/t7060-wtstatus.sh\nindex fcac472..48b751e 100755\n--- a/t/t7060-wtstatus.sh\n+++ b/t/t7060-wtstatus.sh\n@@ -56,4 +56,63 @@ test_expect_success 'M/D conflict does not segfault' '\n \t)\n '\n \n+test_expect_success 'rename & unmerged setup' '\n+\tgit rm -f -r . &&\n+\tcat \"$TEST_DIRECTORY/README\" >ONE &&\n+\tgit add ONE &&\n+\ttest_tick &&\n+\tgit commit -m \"One commit with ONE\" &&\n+\n+\techo Modified >TWO &&\n+\tcat ONE >>TWO &&\n+\tcat ONE >>THREE &&\n+\tgit add TWO THREE &&\n+\tsha1=$(git rev-parse :ONE) &&\n+\tgit rm --cached ONE &&\n+\t(\n+\t\techo \"100644 $sha1 1\tONE\" &&\n+\t\techo \"100644 $sha1 2\tONE\" &&\n+\t\techo \"100644 $sha1 3\tONE\"\n+\t) | git update-index --index-info &&\n+\techo Further >>THREE\n+'\n+\n+test_expect_success 'rename & unmerged status' '\n+\tgit status -suno >actual &&\n+\tcat >expect <<-EOF &&\n+\tUU ONE\n+\tAM THREE\n+\tA  TWO\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+cat >expected <<\\EOF\n+U\tONE\n+A\tTHREE\n+A\tTWO\n+EOF\n+\n+test_expect_success 'git diff-index --cached shows 2 added + 1 unmerged' '\n+\tgit diff-index --cached --name-status HEAD >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'git diff-index --cached -M shows 2 added + 1 unmerged' '\n+\tgit diff-index --cached --name-status HEAD >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+cat >expected <<\\EOF\n+U\tONE\n+C\tONE\tTHREE\n+C\tONE\tTWO\n+EOF\n+\n+test_expect_success 'git diff-index --cached -C shows 2 copies + 1 unmerged' '\n+\tgit diff-index --cached -C --name-status HEAD |\n+\t\tsed \"s/^C[0-9]*/C/g\" >actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_done\n-- \n1.7.4.79.gcbe20\n"}]}