{"thread":{"id":"42852","subject":"[PATCH] blame: Allow to blame paths freshly added to the index","startedAt":"2016-07-15T02:43:15Z","lastAt":"2016-07-18T18:52:20Z","messageCount":15,"participants":["Mike Hommey","Johannes Schindelin","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"291501","messageId":"20160715024254.29186-1-mh@glandium.org","threadId":"42852","inReplyTo":null,"subject":"[PATCH] blame: Allow to blame paths freshly added to the index","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-07-15T02:42:54Z","receivedAt":"2016-07-15T02:43:15Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"When blaming files, changes in the work tree are taken into account\nand displayed as being \"Not Committed Yet\".\n\nHowever, when blaming a file that is not known to the current HEAD,\ngit blame fails with `no such path 'foo' in HEAD`, even when the file\nwas git add'ed.\n\nThis would seem uninteresting with the plain `git blame` case, which\nit is, but it becomes useful when using copy detection, and the new file\nwas created from pieces already in HEAD, moved or copied from other\nfiles.\n---\n builtin/blame.c               |  4 +++-\n t/t8003-blame-corner-cases.sh | 23 +++++++++++++++++++++++\n 2 files changed, 26 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 1e214bd..0858b18 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -2240,7 +2240,9 @@ static void verify_working_tree_path(struct commit *work_tree, const char *path)\n \t\t    sha1_object_info(blob_sha1, NULL) == OBJ_BLOB)\n \t\t\treturn;\n \t}\n-\tdie(\"no such path '%s' in HEAD\", path);\n+\n+\tif (cache_name_pos(path, strlen(path)) < 0)\n+\t\tdie(\"no such path '%s' in HEAD\", path);\n }\n \n static struct commit_list **append_parent(struct commit_list **tail, const unsigned char *sha1)\ndiff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh\nindex a9b266f..a0a09e2 100755\n--- a/t/t8003-blame-corner-cases.sh\n+++ b/t/t8003-blame-corner-cases.sh\n@@ -137,6 +137,29 @@ test_expect_success 'blame wholesale copy and more' '\n \n '\n \n+test_expect_success 'blame wholesale copy and more in the index' '\n+\n+\t{\n+\t\techo ABC\n+\t\techo DEF\n+\t\techo XXXX\n+\t\techo YYYY\n+\t\techo GHIJK\n+\t} >horse &&\n+\tgit add horse &&\n+\tgit blame -f -C -C1 -- horse | sed -e \"$pick_fc\" >current &&\n+\t{\n+\t\techo mouse-Initial\n+\t\techo mouse-Second\n+\t\techo cow-Fifth\n+\t\techo horse-Not\n+\t\techo mouse-Third\n+\t} >expected &&\n+\ttest_cmp expected current &&\n+\tgit rm -f horse\n+\n+'\n+\n test_expect_success 'blame path that used to be a directory' '\n \tmkdir path &&\n \techo A A A A A >path/file &&\n-- \n2.9.1.276.geea30e8\n\n"},{"id":"291526","messageId":"alpine.DEB.2.20.1607151242020.6426@virtualbox","threadId":"42852","inReplyTo":"20160715024254.29186-1-mh@glandium.org","subject":"Re: [PATCH] blame: Allow to blame paths freshly added to the index","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-07-15T10:45:15Z","receivedAt":"2016-07-15T10:45:40Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Mike,\n\nOn Fri, 15 Jul 2016, Mike Hommey wrote:\n\n> When blaming files, changes in the work tree are taken into account\n> and displayed as being \"Not Committed Yet\".\n> \n> However, when blaming a file that is not known to the current HEAD,\n> git blame fails with `no such path 'foo' in HEAD`, even when the file\n> was git add'ed.\n> \n> This would seem uninteresting with the plain `git blame` case, which\n> it is, but it becomes useful when using copy detection, and the new file\n> was created from pieces already in HEAD, moved or copied from other\n> files.\n> ---\n\nWell explained.\n\nPlease add your sign-off.\n\n>  static struct commit_list **append_parent(struct commit_list **tail, const unsigned char *sha1)\n> diff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh\n> index a9b266f..a0a09e2 100755\n> --- a/t/t8003-blame-corner-cases.sh\n> +++ b/t/t8003-blame-corner-cases.sh\n> @@ -137,6 +137,29 @@ test_expect_success 'blame wholesale copy and more' '\n>  \n>  '\n>  \n> +test_expect_success 'blame wholesale copy and more in the index' '\n> +\n> +\t{\n> +\t\techo ABC\n> +\t\techo DEF\n> +\t\techo XXXX\n> +\t\techo YYYY\n> +\t\techo GHIJK\n> +\t} >horse &&\n\nA more common way to do this in our test scripts is by using here\ndocuments. However, in this case I would suggest\n\n\ttest_write_lines ABC DEF XXXX YYYY GHIJK >horse\n\ninstead. The equivalent applies to the 'expected' file below:\n\n> +\tgit add horse &&\n> +\tgit blame -f -C -C1 -- horse | sed -e \"$pick_fc\" >current &&\n> +\t{\n> +\t\techo mouse-Initial\n> +\t\techo mouse-Second\n> +\t\techo cow-Fifth\n> +\t\techo horse-Not\n> +\t\techo mouse-Third\n> +\t} >expected &&\n> +\ttest_cmp expected current &&\n> +\tgit rm -f horse\n\nShould this not be a\n\n\ttest_when_finished \"git rm -f horse\"\n\nat the beginning?\n\nOtherwise it looks really good to me.\n\nCiao,\nDscho\n"},{"id":"291531","messageId":"20160715123245.rdpbl4tmqscyf2tx@glandium.org","threadId":"42852","inReplyTo":"alpine.DEB.2.20.1607151242020.6426@virtualbox","subject":"Re: [PATCH] blame: Allow to blame paths freshly added to the index","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-07-15T12:32:45Z","receivedAt":"2016-07-15T12:32:57Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Fri, Jul 15, 2016 at 12:45:15PM +0200, Johannes Schindelin wrote:\n> Hi Mike,\n> \n> On Fri, 15 Jul 2016, Mike Hommey wrote:\n> \n> > When blaming files, changes in the work tree are taken into account\n> > and displayed as being \"Not Committed Yet\".\n> > \n> > However, when blaming a file that is not known to the current HEAD,\n> > git blame fails with `no such path 'foo' in HEAD`, even when the file\n> > was git add'ed.\n> > \n> > This would seem uninteresting with the plain `git blame` case, which\n> > it is, but it becomes useful when using copy detection, and the new file\n> > was created from pieces already in HEAD, moved or copied from other\n> > files.\n> > ---\n> \n> Well explained.\n> \n> Please add your sign-off.\n\nFacepalm, forgot to sign-off again.\n\n> >  static struct commit_list **append_parent(struct commit_list **tail, const unsigned char *sha1)\n> > diff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh\n> > index a9b266f..a0a09e2 100755\n> > --- a/t/t8003-blame-corner-cases.sh\n> > +++ b/t/t8003-blame-corner-cases.sh\n> > @@ -137,6 +137,29 @@ test_expect_success 'blame wholesale copy and more' '\n> >  \n> >  '\n> >  \n> > +test_expect_success 'blame wholesale copy and more in the index' '\n> > +\n> > +\t{\n> > +\t\techo ABC\n> > +\t\techo DEF\n> > +\t\techo XXXX\n> > +\t\techo YYYY\n> > +\t\techo GHIJK\n> > +\t} >horse &&\n> \n> A more common way to do this in our test scripts is by using here\n> documents. However, in this case I would suggest\n> \n> \ttest_write_lines ABC DEF XXXX YYYY GHIJK >horse\n\nI merely copied the pattern used in other places in the same test file.\nUsing test_write_lines or something else (what are \"here documents\"?)\nwould break consistency. I can also change the other similar blocks at\nthe same time, though, whichever you prefer.\n\n> instead. The equivalent applies to the 'expected' file below:\n> \n> > +\tgit add horse &&\n> > +\tgit blame -f -C -C1 -- horse | sed -e \"$pick_fc\" >current &&\n> > +\t{\n> > +\t\techo mouse-Initial\n> > +\t\techo mouse-Second\n> > +\t\techo cow-Fifth\n> > +\t\techo horse-Not\n> > +\t\techo mouse-Third\n> > +\t} >expected &&\n> > +\ttest_cmp expected current &&\n> > +\tgit rm -f horse\n> \n> Should this not be a\n> \n> \ttest_when_finished \"git rm -f horse\"\n> \n> at the beginning?\n\nIndeed.\n\nThanks\n\nMike\n"},{"id":"291532","messageId":"20160715123758.GA22395@sigill.intra.peff.net","threadId":"42852","inReplyTo":"20160715123245.rdpbl4tmqscyf2tx@glandium.org","subject":"Re: [PATCH] blame: Allow to blame paths freshly added to the index","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-15T12:37:59Z","receivedAt":"2016-07-15T12:38:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 15, 2016 at 09:32:45PM +0900, Mike Hommey wrote:\n\n> > > +test_expect_success 'blame wholesale copy and more in the index' '\n> > > +\n> > > +\t{\n> > > +\t\techo ABC\n> > > +\t\techo DEF\n> > > +\t\techo XXXX\n> > > +\t\techo YYYY\n> > > +\t\techo GHIJK\n> > > +\t} >horse &&\n> > \n> > A more common way to do this in our test scripts is by using here\n> > documents. However, in this case I would suggest\n> > \n> > \ttest_write_lines ABC DEF XXXX YYYY GHIJK >horse\n> \n> I merely copied the pattern used in other places in the same test file.\n> Using test_write_lines or something else (what are \"here documents\"?)\n> would break consistency. I can also change the other similar blocks at\n> the same time, though, whichever you prefer.\n\nA here document is this:\n\n   cat <<-\\EOF\n   ABC\n   DEF\n   XXXX\n   YYYY\n   GHIJK\n   EOF\n\nThe \"<<\" starts the here-doc. The \"-\" tells the shell to strip leading\ntabs (so you can keep it indented with the rest of the code. The \"\\\"\ntells the shell not to interpolate (not a big deal here, but great for\nmore complicated input). The \"EOF\" tells it where to stop.\n\nMatching surrounding style is always reasonable, though I do think this\nparticular file is a bit of an oddball. Most of our scripts use here\ndocuments. Either is OK in this case, IMHO.\n\nPersonally I do not find test_write_lines particularly readable, but I\nguess some people do, which is why it exists.\n\n-Peff\n"},{"id":"291533","messageId":"20160715124201.mrz5ze32s7koevww@glandium.org","threadId":"42852","inReplyTo":"20160715123758.GA22395@sigill.intra.peff.net","subject":"Re: [PATCH] blame: Allow to blame paths freshly added to the index","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-07-15T12:42:01Z","receivedAt":"2016-07-15T12:42:18Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Fri, Jul 15, 2016 at 08:37:59AM -0400, Jeff King wrote:\n> On Fri, Jul 15, 2016 at 09:32:45PM +0900, Mike Hommey wrote:\n> \n> > > > +test_expect_success 'blame wholesale copy and more in the index' '\n> > > > +\n> > > > +\t{\n> > > > +\t\techo ABC\n> > > > +\t\techo DEF\n> > > > +\t\techo XXXX\n> > > > +\t\techo YYYY\n> > > > +\t\techo GHIJK\n> > > > +\t} >horse &&\n> > > \n> > > A more common way to do this in our test scripts is by using here\n> > > documents. However, in this case I would suggest\n> > > \n> > > \ttest_write_lines ABC DEF XXXX YYYY GHIJK >horse\n> > \n> > I merely copied the pattern used in other places in the same test file.\n> > Using test_write_lines or something else (what are \"here documents\"?)\n> > would break consistency. I can also change the other similar blocks at\n> > the same time, though, whichever you prefer.\n> \n> A here document is this:\n> \n>    cat <<-\\EOF\n>    ABC\n>    DEF\n>    XXXX\n>    YYYY\n>    GHIJK\n>    EOF\n> \n> The \"<<\" starts the here-doc. The \"-\" tells the shell to strip leading\n> tabs (so you can keep it indented with the rest of the code. The \"\\\"\n> tells the shell not to interpolate (not a big deal here, but great for\n> more complicated input). The \"EOF\" tells it where to stop.\n\nOh, so that's what they are called! I've used them for 20 years\nwithout knowing :) TIL.\n\nMike\n"},{"id":"291535","messageId":"20160715125550.24883-1-mh@glandium.org","threadId":"42852","inReplyTo":"alpine.DEB.2.20.1607151242020.6426@virtualbox","subject":"[PATCH v2] blame: Allow to blame paths freshly added to the index","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-07-15T12:55:50Z","receivedAt":"2016-07-15T12:56:04Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"When blaming files, changes in the work tree are taken into account\nand displayed as being \"Not Committed Yet\".\n\nHowever, when blaming a file that is not known to the current HEAD,\ngit blame fails with `no such path 'foo' in HEAD`, even when the file\nwas git add'ed.\n\nThis would seem uninteresting with the plain `git blame` case, which\nit is, but it becomes useful when using copy detection, and the new file\nwas created from pieces already in HEAD, moved or copied from other\nfiles.\n\nSigned-off-by: Mike Hommey <mh@glandium.org>\n---\n builtin/blame.c               |  4 ++-\n t/t8003-blame-corner-cases.sh | 57 ++++++++++++++++++++++++++++++-------------\n 2 files changed, 43 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 1e214bd..0858b18 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -2240,7 +2240,9 @@ static void verify_working_tree_path(struct commit *work_tree, const char *path)\n \t\t    sha1_object_info(blob_sha1, NULL) == OBJ_BLOB)\n \t\t\treturn;\n \t}\n-\tdie(\"no such path '%s' in HEAD\", path);\n+\n+\tif (cache_name_pos(path, strlen(path)) < 0)\n+\t\tdie(\"no such path '%s' in HEAD\", path);\n }\n \n static struct commit_list **append_parent(struct commit_list **tail, const unsigned char *sha1)\ndiff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh\nindex a9b266f..2812d7c 100755\n--- a/t/t8003-blame-corner-cases.sh\n+++ b/t/t8003-blame-corner-cases.sh\n@@ -41,12 +41,12 @@ test_expect_success setup '\n \ttest_tick &&\n \tGIT_AUTHOR_NAME=Fourth git commit -m Fourth &&\n \n-\t{\n-\t\techo ABC\n-\t\techo DEF\n-\t\techo XXXX\n-\t\techo GHIJK\n-\t} >cow &&\n+\tcat >cow <<-\\EOF &&\n+\tABC\n+\tDEF\n+\tXXXX\n+\tGHIJK\n+\tEOF\n \tgit add cow &&\n \ttest_tick &&\n \tGIT_AUTHOR_NAME=Fifth git commit -m Fifth\n@@ -115,11 +115,11 @@ test_expect_success 'append with -C -C -C' '\n test_expect_success 'blame wholesale copy' '\n \n \tgit blame -f -C -C1 HEAD^ -- cow | sed -e \"$pick_fc\" >current &&\n-\t{\n-\t\techo mouse-Initial\n-\t\techo mouse-Second\n-\t\techo mouse-Third\n-\t} >expected &&\n+\tcat >expected <<-\\EOF &&\n+\tmouse-Initial\n+\tmouse-Second\n+\tmouse-Third\n+\tEOF\n \ttest_cmp expected current\n \n '\n@@ -127,12 +127,35 @@ test_expect_success 'blame wholesale copy' '\n test_expect_success 'blame wholesale copy and more' '\n \n \tgit blame -f -C -C1 HEAD -- cow | sed -e \"$pick_fc\" >current &&\n-\t{\n-\t\techo mouse-Initial\n-\t\techo mouse-Second\n-\t\techo cow-Fifth\n-\t\techo mouse-Third\n-\t} >expected &&\n+\tcat >expected <<-\\EOF &&\n+\tmouse-Initial\n+\tmouse-Second\n+\tcow-Fifth\n+\tmouse-Third\n+\tEOF\n+\ttest_cmp expected current\n+\n+'\n+\n+test_expect_success 'blame wholesale copy and more in the index' '\n+\n+\tcat >horse <<-\\EOF &&\n+\tABC\n+\tDEF\n+\tXXXX\n+\tYYYY\n+\tGHIJK\n+\tEOF\n+\tgit add horse &&\n+\ttest_when_finished \"git rm -f horse\" &&\n+\tgit blame -f -C -C1 -- horse | sed -e \"$pick_fc\" >current &&\n+\tcat >expected <<-\\EOF &&\n+\tmouse-Initial\n+\tmouse-Second\n+\tcow-Fifth\n+\thorse-Not\n+\tmouse-Third\n+\tEOF\n \ttest_cmp expected current\n \n '\n-- \n2.9.1.276.geea30e8\n\n"},{"id":"291541","messageId":"alpine.DEB.2.20.1607151726370.6426@virtualbox","threadId":"42852","inReplyTo":"20160715125550.24883-1-mh@glandium.org","subject":"Re: [PATCH v2] blame: Allow to blame paths freshly added to the index","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-07-15T15:28:30Z","receivedAt":"2016-07-15T15:28:55Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Mike,\n\nOn Fri, 15 Jul 2016, Mike Hommey wrote:\n\n> diff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh\n> index a9b266f..2812d7c 100755\n> --- a/t/t8003-blame-corner-cases.sh\n> +++ b/t/t8003-blame-corner-cases.sh\n> @@ -41,12 +41,12 @@ test_expect_success setup '\n>  \ttest_tick &&\n>  \tGIT_AUTHOR_NAME=Fourth git commit -m Fourth &&\n>  \n> -\t{\n> -\t\techo ABC\n> -\t\techo DEF\n> -\t\techo XXXX\n> -\t\techo GHIJK\n> -\t} >cow &&\n> +\tcat >cow <<-\\EOF &&\n> +\tABC\n> +\tDEF\n> +\tXXXX\n> +\tGHIJK\n> +\tEOF\n>  \tgit add cow &&\n\nSorry, I did not realize that there was precedent for this awkward\nparadigm in the same test script.\n\nI like that you fix them, but I would prefer that to be done in a separate\npatch (does not even need to be the same patch series).\n\nApart from that (i.e. apart from touching unrelated parts of the test\nscript), the patch looks good to me.\n\nThanks,\nDscho\n"},{"id":"291565","messageId":"xmqqvb06lita.fsf@gitster.mtv.corp.google.com","threadId":"42852","inReplyTo":"20160715024254.29186-1-mh@glandium.org","subject":"Re: [PATCH] blame: Allow to blame paths freshly added to the index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-15T20:58:41Z","receivedAt":"2016-07-15T20:58:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mike Hommey <mh@glandium.org> writes:\n\n> When blaming files, changes in the work tree are taken into account\n> and displayed as being \"Not Committed Yet\".\n>\n> However, when blaming a file that is not known to the current HEAD,\n> git blame fails with `no such path 'foo' in HEAD`, even when the file\n> was git add'ed.\n>\n> This would seem uninteresting with the plain `git blame` case, which\n> it is, but it becomes useful when using copy detection, and the new file\n> was created from pieces already in HEAD, moved or copied from other\n> files.\n\nI suspect that this would be useful without copy detection.  If you\n\"git mv fileA fileB\" (optionally followed by \"edit fileB\"), fileB\nwould not be in HEAD but you should be able to trace the lineage of\nthe lines in it back through the renaming event, and this change\nalso allows that use case, no?\n\n> ---\n\nMissing sign-off?\n\n>  builtin/blame.c               |  4 +++-\n>  t/t8003-blame-corner-cases.sh | 23 +++++++++++++++++++++++\n>  2 files changed, 26 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index 1e214bd..0858b18 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -2240,7 +2240,9 @@ static void verify_working_tree_path(struct commit *work_tree, const char *path)\n>  \t\t    sha1_object_info(blob_sha1, NULL) == OBJ_BLOB)\n>  \t\t\treturn;\n>  \t}\n> -\tdie(\"no such path '%s' in HEAD\", path);\n> +\n> +\tif (cache_name_pos(path, strlen(path)) < 0)\n> +\t\tdie(\"no such path '%s' in HEAD\", path);\n>  }\n\nThis is a surprisingly small change to bring a big difference at the\nusage level.  I am sort-of surprised that the \"fake working tree\ncommit\" mechanism was already prepared to handle this, even though I\nam responsible for the introduction of it at 1cfe7733 (git-blame: no\nrev means start from the working tree file., 2007-01-30).\n\nHaving said that, do we act differently when the index is unmerged\nat path in any way?  When path exists but is unmerged, you get\nnegative index from cache_name_pos(), and IIUC, \"blaming of working\ntree file\" does not care if the index is unmerged.  It creates the\nfake working tree file commit, pretends HEAD is its parent, and digs\nthe lineage from there.\n\nI suspect that the above change needs to be updated further if the\nuser wants to run \"blame path\" during a conflicted renaming merge,\ni.e.\n\n 0. Before two histories diverged, there was old_path.\n 1. Our side updated contents of that file and kept it at old_path.\n 2. Their side updated contents of that file and moved it to path.\n 3. \"git merge\" notcied the rename, created three stages at \"path\",\n    with the result of conflicted content-level merge in the working\n    tree at \"path\".\n 4. The user edits \"path\" and resolves the conflict, but wants to\n    double check before running \"git add path\".\n 5. \"git blame path\"\n\nPerhaps something like this should suffice:\n\n    pos = cache_name_pos(path, strlen(path));\n    if (0 <= pos)\n    \t; /* ok */\n    else if (!strcmp(active_cache[-1 - pos]->name), path)\n        ; /* ok -- just unmerged */\n    else\n    \tdie(\"no such path in HEAD\");\n\n"},{"id":"291566","messageId":"xmqqr3auli2x.fsf@gitster.mtv.corp.google.com","threadId":"42852","inReplyTo":"xmqqvb06lita.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] blame: Allow to blame paths freshly added to the index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-15T21:14:30Z","receivedAt":"2016-07-15T21:14:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> I suspect that the above change needs to be updated further if the\n> user wants to run \"blame path\" during a conflicted renaming merge,\n> i.e.\n>\n>  0. Before two histories diverged, there was old_path.\n>  1. Our side updated contents of that file and kept it at old_path.\n>  2. Their side updated contents of that file and moved it to path.\n>  3. \"git merge\" notcied the rename, created three stages at \"path\",\n>     with the result of conflicted content-level merge in the working\n>     tree at \"path\".\n>  4. The user edits \"path\" and resolves the conflict, but wants to\n>     double check before running \"git add path\".\n>  5. \"git blame path\"\n>\n> Perhaps something like this should suffice:\n>\n>     pos = cache_name_pos(path, strlen(path));\n>     if (0 <= pos)\n>     \t; /* ok */\n>     else if (!strcmp(active_cache[-1 - pos]->name), path)\n>         ; /* ok -- just unmerged */\n>     else\n>     \tdie(\"no such path in HEAD\");\n\nI do not think the \"conflicted renaming merge\" example would not be\na problem in practice iff \"git merge\" was used, because the fake\nworking tree commit would look at both our tree and their tree, and\nfind \"path\" in theirs inside the loop above this \"die\".\n\nBut the user can be in the same conflicted rename situation with\n\"git am -3\" or cherry-pick, and in these cases there won't be extra\nparent commits for the fake work tree commit, hence the conclusion\ndoes not change.\n\nThanks.\n"},{"id":"291571","messageId":"20160715231656.iztvtftk5ttrzeow@glandium.org","threadId":"42852","inReplyTo":"xmqqr3auli2x.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] blame: Allow to blame paths freshly added to the index","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-07-15T23:16:56Z","receivedAt":"2016-07-15T23:17:09Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"> I suspect that this would be useful without copy detection.  If you\n> \"git mv fileA fileB\" (optionally followed by \"edit fileB\"), fileB\n> would not be in HEAD but you should be able to trace the lineage of\n> the lines in it back through the renaming event, and this change\n> also allows that use case, no?\n\nIt should, but that'd be copy/move detection, wouldn't it? :)\n\n> > I suspect that the above change needs to be updated further if the\n> > user wants to run \"blame path\" during a conflicted renaming merge,\n> > i.e.\n> >\n> >  0. Before two histories diverged, there was old_path.\n> >  1. Our side updated contents of that file and kept it at old_path.\n> >  2. Their side updated contents of that file and moved it to path.\n> >  3. \"git merge\" notcied the rename, created three stages at \"path\",\n> >     with the result of conflicted content-level merge in the working\n> >     tree at \"path\".\n> >  4. The user edits \"path\" and resolves the conflict, but wants to\n> >     double check before running \"git add path\".\n> >  5. \"git blame path\"\n> >\n> > Perhaps something like this should suffice:\n> >\n> >     pos = cache_name_pos(path, strlen(path));\n> >     if (0 <= pos)\n> >     \t; /* ok */\n> >     else if (!strcmp(active_cache[-1 - pos]->name), path)\n> >         ; /* ok -- just unmerged */\n> >     else\n> >     \tdie(\"no such path in HEAD\");\n> \n> I do not think the \"conflicted renaming merge\" example would not be\n> a problem in practice iff \"git merge\" was used, because the fake\n> working tree commit would look at both our tree and their tree, and\n> find \"path\" in theirs inside the loop above this \"die\".\n\nMore than that, it seems that's a case that already works without the\npatch (it doesn't complain that \"no such path in HEAD\", although it\nonly identifies the \"theirs\" part of the merge conflict when blaming\nthe file straight out of the merge failure, but that'd be a separate\nissue.\n\n> But the user can be in the same conflicted rename situation with\n> \"git am -3\" or cherry-pick, and in these cases there won't be extra\n> parent commits for the fake work tree commit, hence the conclusion\n> does not change.\n\nIndeed, with cherry-pick, the \"no such path in HEAD\" error is happening\nwith the patch.\n\nMike\n"},{"id":"291572","messageId":"20160715232346.12176-2-mh@glandium.org","threadId":"42852","inReplyTo":"20160715232346.12176-1-mh@glandium.org","subject":"[PATCH v3 2/2] t/t8003-blame-corner-cases.sh: Use here documents","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-07-15T23:23:46Z","receivedAt":"2016-07-15T23:23:56Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"Somehow, this test was using:\n\n{\n\techo A\n\techo B\n} > file\n\nblock to feed file contents. This changes those to the form most common\nin git test scripts:\n\ncat >file <<-\\EOF\nA\nB\nEOF\n\nSigned-off-by: Mike Hommey <mh@glandium.org>\n---\n t/t8003-blame-corner-cases.sh | 34 +++++++++++++++++-----------------\n 1 file changed, 17 insertions(+), 17 deletions(-)\n\ndiff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh\nindex eab2e28..e48370d 100755\n--- a/t/t8003-blame-corner-cases.sh\n+++ b/t/t8003-blame-corner-cases.sh\n@@ -41,12 +41,12 @@ test_expect_success setup '\n \ttest_tick &&\n \tGIT_AUTHOR_NAME=Fourth git commit -m Fourth &&\n \n-\t{\n-\t\techo ABC\n-\t\techo DEF\n-\t\techo XXXX\n-\t\techo GHIJK\n-\t} >cow &&\n+\tcat >cow <<-\\EOF &&\n+\tABC\n+\tDEF\n+\tXXXX\n+\tGHIJK\n+\tEOF\n \tgit add cow &&\n \ttest_tick &&\n \tGIT_AUTHOR_NAME=Fifth git commit -m Fifth\n@@ -115,11 +115,11 @@ test_expect_success 'append with -C -C -C' '\n test_expect_success 'blame wholesale copy' '\n \n \tgit blame -f -C -C1 HEAD^ -- cow | sed -e \"$pick_fc\" >current &&\n-\t{\n-\t\techo mouse-Initial\n-\t\techo mouse-Second\n-\t\techo mouse-Third\n-\t} >expected &&\n+\tcat >expected <<-\\EOF &&\n+\tmouse-Initial\n+\tmouse-Second\n+\tmouse-Third\n+\tEOF\n \ttest_cmp expected current\n \n '\n@@ -127,12 +127,12 @@ test_expect_success 'blame wholesale copy' '\n test_expect_success 'blame wholesale copy and more' '\n \n \tgit blame -f -C -C1 HEAD -- cow | sed -e \"$pick_fc\" >current &&\n-\t{\n-\t\techo mouse-Initial\n-\t\techo mouse-Second\n-\t\techo cow-Fifth\n-\t\techo mouse-Third\n-\t} >expected &&\n+\tcat >expected <<-\\EOF &&\n+\tmouse-Initial\n+\tmouse-Second\n+\tcow-Fifth\n+\tmouse-Third\n+\tEOF\n \ttest_cmp expected current\n \n '\n-- \n2.9.1.277.gdbc86c9.dirty\n\n"},{"id":"291573","messageId":"20160715232346.12176-1-mh@glandium.org","threadId":"42852","inReplyTo":"xmqqr3auli2x.fsf@gitster.mtv.corp.google.com","subject":"[PATCH v3 1/2] blame: Allow to blame paths freshly added to the index","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-07-15T23:23:45Z","receivedAt":"2016-07-15T23:24:00Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"When blaming files, changes in the work tree are taken into account\nand displayed as being \"Not Committed Yet\".\n\nHowever, when blaming a file that is not known to the current HEAD,\ngit blame fails with `no such path 'foo' in HEAD`, even when the file\nwas git add'ed.\n\nThis would seem uninteresting with the plain `git blame` case, which\nit is, but it becomes useful when using copy detection, and the new file\nwas created from pieces already in HEAD, moved or copied from other\nfiles.\n\nSigned-off-by: Mike Hommey <mh@glandium.org>\n---\n builtin/blame.c               | 10 +++++++++-\n t/t8003-blame-corner-cases.sh | 45 +++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 54 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 1e214bd..f297a7f 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -2230,6 +2230,7 @@ static int git_blame_config(const char *var, const char *value, void *cb)\n static void verify_working_tree_path(struct commit *work_tree, const char *path)\n {\n \tstruct commit_list *parents;\n+\tint pos;\n \n \tfor (parents = work_tree->parents; parents; parents = parents->next) {\n \t\tconst unsigned char *commit_sha1 = parents->item->object.oid.hash;\n@@ -2240,7 +2241,14 @@ static void verify_working_tree_path(struct commit *work_tree, const char *path)\n \t\t    sha1_object_info(blob_sha1, NULL) == OBJ_BLOB)\n \t\t\treturn;\n \t}\n-\tdie(\"no such path '%s' in HEAD\", path);\n+\n+\tpos = cache_name_pos(path, strlen(path));\n+\tif (pos >= 0)\n+\t\t; /* path is in the index */\n+\telse if (!strcmp(active_cache[-1 - pos]->name, path))\n+\t\t; /* path is in the index, unmerged */\n+\telse\n+\t\tdie(\"no such path '%s' in HEAD\", path);\n }\n \n static struct commit_list **append_parent(struct commit_list **tail, const unsigned char *sha1)\ndiff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh\nindex a9b266f..eab2e28 100755\n--- a/t/t8003-blame-corner-cases.sh\n+++ b/t/t8003-blame-corner-cases.sh\n@@ -137,6 +137,51 @@ test_expect_success 'blame wholesale copy and more' '\n \n '\n \n+test_expect_success 'blame wholesale copy and more in the index' '\n+\n+\tcat >horse <<-\\EOF &&\n+\tABC\n+\tDEF\n+\tXXXX\n+\tYYYY\n+\tGHIJK\n+\tEOF\n+\tgit add horse &&\n+\ttest_when_finished \"git rm -f horse\" &&\n+\tgit blame -f -C -C1 -- horse | sed -e \"$pick_fc\" >current &&\n+\tcat >expected <<-\\EOF &&\n+\tmouse-Initial\n+\tmouse-Second\n+\tcow-Fifth\n+\thorse-Not\n+\tmouse-Third\n+\tEOF\n+\ttest_cmp expected current\n+\n+'\n+\n+test_expect_success 'blame during cherry-pick with file rename conflict' '\n+\n+\ttest_when_finished \"git reset --hard && git checkout master\" &&\n+\tgit checkout HEAD~3 &&\n+\techo MOUSE >> mouse &&\n+\tgit mv mouse rodent &&\n+\tgit add rodent &&\n+\tGIT_AUTHOR_NAME=Rodent git commit -m \"rodent\" &&\n+\tgit checkout --detach master &&\n+\t(git cherry-pick HEAD@{1} || test $? -eq 1) &&\n+\tgit show HEAD@{1}:rodent > rodent &&\n+\tgit add rodent &&\n+\tgit blame -f -C -C1 rodent | sed -e \"$pick_fc\" >current &&\n+\tcat current &&\n+\tcat >expected <<-\\EOF &&\n+\tmouse-Initial\n+\tmouse-Second\n+\trodent-Not\n+\tEOF\n+\ttest_cmp expected current\n+'\n+\n test_expect_success 'blame path that used to be a directory' '\n \tmkdir path &&\n \techo A A A A A >path/file &&\n-- \n2.9.1.277.gdbc86c9.dirty\n\n"},{"id":"291595","messageId":"alpine.DEB.2.20.1607161505100.28832@virtualbox","threadId":"42852","inReplyTo":"20160715232346.12176-2-mh@glandium.org","subject":"Re: [PATCH v3 2/2] t/t8003-blame-corner-cases.sh: Use here documents","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-07-16T13:05:52Z","receivedAt":"2016-07-16T13:06:23Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Mike,\n\nOn Sat, 16 Jul 2016, Mike Hommey wrote:\n\n> Somehow, this test was using:\n> \n> {\n> \techo A\n> \techo B\n> } > file\n> \n> block to feed file contents. This changes those to the form most common\n> in git test scripts:\n> \n> cat >file <<-\\EOF\n> A\n> B\n> EOF\n> \n> Signed-off-by: Mike Hommey <mh@glandium.org>\n\nI reviewed both patches and like them.\n\nThanks,\nDscho\n"},{"id":"291667","messageId":"xmqqbn1ukchz.fsf@gitster.mtv.corp.google.com","threadId":"42852","inReplyTo":"20160715231656.iztvtftk5ttrzeow@glandium.org","subject":"Re: [PATCH] blame: Allow to blame paths freshly added to the index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-18T18:49:28Z","receivedAt":"2016-07-18T18:50:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mike Hommey <mh@glandium.org> writes:\n\n>> I suspect that this would be useful without copy detection.  If you\n>> \"git mv fileA fileB\" (optionally followed by \"edit fileB\"), fileB\n>> would not be in HEAD but you should be able to trace the lineage of\n>> the lines in it back through the renaming event, and this change\n>> also allows that use case, no?\n>\n> It should, but that'd be copy/move detection, wouldn't it? :)\n\nActually, in the context of \"git blame\", there is no extra\n\"detection\" needed for following a whole file rename.\n\n>> But the user can be in the same conflicted rename situation with\n>> \"git am -3\" or cherry-pick, and in these cases there won't be extra\n>> parent commits for the fake work tree commit, hence the conclusion\n>> does not change.\n>\n> Indeed, with cherry-pick, the \"no such path in HEAD\" error is happening\n> with the patch.\n\nOK.\n"},{"id":"291668","messageId":"xmqq7fcikcdf.fsf@gitster.mtv.corp.google.com","threadId":"42852","inReplyTo":"20160715232346.12176-2-mh@glandium.org","subject":"Re: [PATCH v3 2/2] t/t8003-blame-corner-cases.sh: Use here documents","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-18T18:52:12Z","receivedAt":"2016-07-18T18:52:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mike Hommey <mh@glandium.org> writes:\n\n> Somehow, this test was using:\n>\n> {\n> \techo A\n> \techo B\n> } > file\n>\n> block to feed file contents. This changes those to the form most common\n> in git test scripts:\n>\n> cat >file <<-\\EOF\n> A\n> B\n> EOF\n>\n> Signed-off-by: Mike Hommey <mh@glandium.org>\n> ---\n\nIt is not so strong a preference to ask you to re-roll, but for\nfuture reference, I'd prefer to see this preparatory clean-up early\nin the series, followed by the primary thing, IOW, I would have\nliked more if the two patches were swapped.\n\nThanks.  Will queue.\n\n>  t/t8003-blame-corner-cases.sh | 34 +++++++++++++++++-----------------\n>  1 file changed, 17 insertions(+), 17 deletions(-)\n>\n> diff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh\n> index eab2e28..e48370d 100755\n> --- a/t/t8003-blame-corner-cases.sh\n> +++ b/t/t8003-blame-corner-cases.sh\n> @@ -41,12 +41,12 @@ test_expect_success setup '\n>  \ttest_tick &&\n>  \tGIT_AUTHOR_NAME=Fourth git commit -m Fourth &&\n>  \n> -\t{\n> -\t\techo ABC\n> -\t\techo DEF\n> -\t\techo XXXX\n> -\t\techo GHIJK\n> -\t} >cow &&\n> +\tcat >cow <<-\\EOF &&\n> +\tABC\n> +\tDEF\n> +\tXXXX\n> +\tGHIJK\n> +\tEOF\n>  \tgit add cow &&\n>  \ttest_tick &&\n>  \tGIT_AUTHOR_NAME=Fifth git commit -m Fifth\n> @@ -115,11 +115,11 @@ test_expect_success 'append with -C -C -C' '\n>  test_expect_success 'blame wholesale copy' '\n>  \n>  \tgit blame -f -C -C1 HEAD^ -- cow | sed -e \"$pick_fc\" >current &&\n> -\t{\n> -\t\techo mouse-Initial\n> -\t\techo mouse-Second\n> -\t\techo mouse-Third\n> -\t} >expected &&\n> +\tcat >expected <<-\\EOF &&\n> +\tmouse-Initial\n> +\tmouse-Second\n> +\tmouse-Third\n> +\tEOF\n>  \ttest_cmp expected current\n>  \n>  '\n> @@ -127,12 +127,12 @@ test_expect_success 'blame wholesale copy' '\n>  test_expect_success 'blame wholesale copy and more' '\n>  \n>  \tgit blame -f -C -C1 HEAD -- cow | sed -e \"$pick_fc\" >current &&\n> -\t{\n> -\t\techo mouse-Initial\n> -\t\techo mouse-Second\n> -\t\techo cow-Fifth\n> -\t\techo mouse-Third\n> -\t} >expected &&\n> +\tcat >expected <<-\\EOF &&\n> +\tmouse-Initial\n> +\tmouse-Second\n> +\tcow-Fifth\n> +\tmouse-Third\n> +\tEOF\n>  \ttest_cmp expected current\n>  \n>  '\n"}]}