{"thread":{"id":"45209","subject":"fatal error when diffing changed symlinks","startedAt":"2017-02-24T11:48:25Z","lastAt":"2017-03-14T05:52:11Z","messageCount":17,"participants":["Christophe Macabiau","Junio C Hamano","Jeff King","Johannes Schindelin","David Aguilar"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"312510","messageId":"CAC0icTZ87yeYndPFyjD4nkJBcw5SC-bgUJYbEzYqH7YhSFZvuQ@mail.gmail.com","threadId":"45209","inReplyTo":null,"subject":"fatal error when diffing changed symlinks","fromName":"Christophe Macabiau","fromEmail":"christophemacabiau@gmail.com","sentAt":"2017-02-24T11:47:38Z","receivedAt":"2017-02-24T11:48:25Z","isPatch":false,"sender":{"key":"christophemacabiau@gmail.com","avatar":null},"body":"Hi,\n\nwith the commands below, you will get :\n\n> fatal: bad object 0000000000000000000000000000000000000000\n> show 0000000000000000000000000000000000000000: command returned error: 128\n>\n\nI am using version 2.5.5 fedora 23\n\ncd /tmp\nmkdir a\ncd a\ngit init\ntouch b\nln -s b c\ngit add .\ngit commit -m 'first'\ntouch d\nrm c\nln -s d c\ngit difftool --dir-diff\n\nThanks for your feedback,\n\nChristophe\n"},{"id":"312526","messageId":"xmqqshn34gsh.fsf@gitster.mtv.corp.google.com","threadId":"45209","inReplyTo":"CAC0icTZ87yeYndPFyjD4nkJBcw5SC-bgUJYbEzYqH7YhSFZvuQ@mail.gmail.com","subject":"Re: fatal error when diffing changed symlinks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-24T17:53:02Z","receivedAt":"2017-02-24T17:53:12Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christophe Macabiau writes:\n\n> with the commands below, you will get :\n>\n>> fatal: bad object 0000000000000000000000000000000000000000\n>> show 0000000000000000000000000000000000000000: command returned error: 128\n>>\n>\n> I am using version 2.5.5 fedora 23\n>\n> cd /tmp\n> mkdir a\n> cd a\n> git init\n> touch b\n> ln -s b c\n> git add .\n> git commit -m 'first'\n> touch d\n> rm c\n> ln -s d c\n> git difftool --dir-diff\n\nA slightly worse is that the upcoming Git will ship with a rewritten\n\"difftool\" that makes the above sequence segfault. As either way is\nbad, I do not particularly think the rewritten one needs to be\nreverted (it would give \"fatal: bad object\" then, and would not fix\nyour problem), but it needs to be looked at.\n\nThanks for a report.\n\n"},{"id":"312544","messageId":"xmqqtw7j1i6d.fsf@gitster.mtv.corp.google.com","threadId":"45209","inReplyTo":"xmqqshn34gsh.fsf@gitster.mtv.corp.google.com","subject":"Re: fatal error when diffing changed symlinks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-24T19:51:22Z","receivedAt":"2017-02-24T19:51:57Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> cd /tmp\n>> mkdir a\n>> cd a\n>> git init\n>> touch b\n>> ln -s b c\n>> git add .\n>> git commit -m 'first'\n>> touch d\n>> rm c\n>> ln -s d c\n>> git difftool --dir-diff\n>\n> A slightly worse is that the upcoming Git will ship with a rewritten\n> \"difftool\" that makes the above sequence segfault.\n\nThe culprit seems to be these lines in run_dir_diff():\n\n\t\tif (S_ISLNK(lmode)) {\n\t\t\tchar *content = read_sha1_file(loid.hash, &type, &size);\n\t\t\tadd_left_or_right(&symlinks2, src_path, content, 0);\n\t\t\tfree(content);\n\t\t}\n\n\t\tif (S_ISLNK(rmode)) {\n\t\t\tchar *content = read_sha1_file(roid.hash, &type, &size);\n\t\t\tadd_left_or_right(&symlinks2, dst_path, content, 1);\n\t\t\tfree(content);\n\t\t}\n\nWhen viewing a working tree file, oid.hash could be 0{40} and\nread_sha1_file() is not the right function to use to obtain the\ncontents.\n\nBoth of these two need to pay attention to 0{40}, I think, as the\nuser may be running \"difftool -R --dir-diff\" in which case the\nworking tree would appear in the left hand side instead.\n\nI didn't follow the codepath for regular files closely, but the code\nthat follows the above excerpt does quite different things to lstate\nand rstate, which makes me suspect that the code is not prepared to\nsee \"-R\"(everse) diff (and I further suspect that these issues were\ninherited from the original scripted Porcelain).\n\n"},{"id":"312551","messageId":"20170224203523.mdoh4ivhwflmpr6j@sigill.intra.peff.net","threadId":"45209","inReplyTo":"xmqqtw7j1i6d.fsf@gitster.mtv.corp.google.com","subject":"Re: fatal error when diffing changed symlinks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-24T20:35:23Z","receivedAt":"2017-02-24T20:35:43Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 24, 2017 at 11:51:22AM -0800, Junio C Hamano wrote:\n\n> > A slightly worse is that the upcoming Git will ship with a rewritten\n> > \"difftool\" that makes the above sequence segfault.\n> \n> The culprit seems to be these lines in run_dir_diff():\n> \n> \t\tif (S_ISLNK(lmode)) {\n> \t\t\tchar *content = read_sha1_file(loid.hash, &type, &size);\n> \t\t\tadd_left_or_right(&symlinks2, src_path, content, 0);\n> \t\t\tfree(content);\n> \t\t}\n> \n> \t\tif (S_ISLNK(rmode)) {\n> \t\t\tchar *content = read_sha1_file(roid.hash, &type, &size);\n> \t\t\tadd_left_or_right(&symlinks2, dst_path, content, 1);\n> \t\t\tfree(content);\n> \t\t}\n> \n> When viewing a working tree file, oid.hash could be 0{40} and\n> read_sha1_file() is not the right function to use to obtain the\n> contents.\n> \n> Both of these two need to pay attention to 0{40}, I think, as the\n> user may be running \"difftool -R --dir-diff\" in which case the\n> working tree would appear in the left hand side instead.\n\nAs a side note, I think even outside of 0{40}, this should be checking\nthe return value of read_sha1_file(). A corrupted repo should die(), not\nsegfault.\n\n-Peff\n"},{"id":"312636","messageId":"alpine.DEB.2.20.1702251336420.3767@virtualbox","threadId":"45209","inReplyTo":"20170224203523.mdoh4ivhwflmpr6j@sigill.intra.peff.net","subject":"Re: fatal error when diffing changed symlinks","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-02-25T12:36:58Z","receivedAt":"2017-02-25T12:38:11Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff & Junio,\n\nOn Fri, 24 Feb 2017, Jeff King wrote:\n\n> On Fri, Feb 24, 2017 at 11:51:22AM -0800, Junio C Hamano wrote:\n> \n> > > A slightly worse is that the upcoming Git will ship with a rewritten\n> > > \"difftool\" that makes the above sequence segfault.\n> > \n> > The culprit seems to be these lines in run_dir_diff():\n> > \n> > \t\tif (S_ISLNK(lmode)) {\n> > \t\t\tchar *content = read_sha1_file(loid.hash, &type, &size);\n> > \t\t\tadd_left_or_right(&symlinks2, src_path, content, 0);\n> > \t\t\tfree(content);\n> > \t\t}\n> > \n> > \t\tif (S_ISLNK(rmode)) {\n> > \t\t\tchar *content = read_sha1_file(roid.hash, &type, &size);\n> > \t\t\tadd_left_or_right(&symlinks2, dst_path, content, 1);\n> > \t\t\tfree(content);\n> > \t\t}\n> > \n> > When viewing a working tree file, oid.hash could be 0{40} and\n> > read_sha1_file() is not the right function to use to obtain the\n> > contents.\n> > \n> > Both of these two need to pay attention to 0{40}, I think, as the\n> > user may be running \"difftool -R --dir-diff\" in which case the\n> > working tree would appear in the left hand side instead.\n> \n> As a side note, I think even outside of 0{40}, this should be checking\n> the return value of read_sha1_file(). A corrupted repo should die(), not\n> segfault.\n\nI agree. I am on it.\n\nCiao,\nDscho\n"},{"id":"313437","messageId":"xmqqfuip7y07.fsf@gitster.mtv.corp.google.com","threadId":"45209","inReplyTo":"alpine.DEB.2.20.1702251336420.3767@virtualbox","subject":"Re: fatal error when diffing changed symlinks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-07T18:16:56Z","receivedAt":"2017-03-07T18:34:55Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> > When viewing a working tree file, oid.hash could be 0{40} and\n>> > read_sha1_file() is not the right function to use to obtain the\n>> > contents.\n>> > \n>> > Both of these two need to pay attention to 0{40}, I think, as the\n>> > user may be running \"difftool -R --dir-diff\" in which case the\n>> > working tree would appear in the left hand side instead.\n>> \n>> As a side note, I think even outside of 0{40}, this should be checking\n>> the return value of read_sha1_file(). A corrupted repo should die(), not\n>> segfault.\n>\n> I agree. I am on it.\n\nFriendly ping, if only to make sure that we can keep a piece of this\nthread in the more \"recent\" pile.\n\nIf you have other topics you need to perfect, I think it is OK to\npostpone the fix on this topic a bit longer, but I'd hate to ship\ntwo releases with a known breakage without an attempt to fix it, so\nif you are otherwise occupied, I may encourage others (including me)\nto take a look at this. The new \"difftool\" also has a reported\nregression somebody else expressed willingness to work on, which is\nsort of blocked by everybody else not knowing the timeline on this\none. cf. <20170303212836.GB13790@arch-attack.localdomain>\n\nA patch series would be very welcome, but \"Please go ahead if\nsomebody else has time, and I'll help reviewing\" would be also\ngood.\n\nThanks.\n"},{"id":"313461","messageId":"alpine.DEB.2.20.1703072332370.3767@virtualbox","threadId":"45209","inReplyTo":"xmqqfuip7y07.fsf@gitster.mtv.corp.google.com","subject":"Re: fatal error when diffing changed symlinks","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-03-07T22:34:08Z","receivedAt":"2017-03-07T23:07:16Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 7 Mar 2017, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> >> > When viewing a working tree file, oid.hash could be 0{40} and\n> >> > read_sha1_file() is not the right function to use to obtain the\n> >> > contents.\n> >> > \n> >> > Both of these two need to pay attention to 0{40}, I think, as the\n> >> > user may be running \"difftool -R --dir-diff\" in which case the\n> >> > working tree would appear in the left hand side instead.\n> >> \n> >> As a side note, I think even outside of 0{40}, this should be checking\n> >> the return value of read_sha1_file(). A corrupted repo should die(), not\n> >> segfault.\n> >\n> > I agree. I am on it.\n> \n> Friendly ping, if only to make sure that we can keep a piece of this\n> thread in the more \"recent\" pile.\n> \n> If you have other topics you need to perfect, I think it is OK to\n> postpone the fix on this topic a bit longer, but I'd hate to ship\n> two releases with a known breakage without an attempt to fix it, so\n> if you are otherwise occupied, I may encourage others (including me)\n> to take a look at this. The new \"difftool\" also has a reported\n> regression somebody else expressed willingness to work on, which is\n> sort of blocked by everybody else not knowing the timeline on this\n> one. cf. <20170303212836.GB13790@arch-attack.localdomain>\n> \n> A patch series would be very welcome, but \"Please go ahead if\n> somebody else has time, and I'll help reviewing\" would be also\n> good.\n\nUnfortunately, the obvious fix was not enough. My current work in progress\nis in the `difftool-null-sha1` branch on https://gihub.com/dscho/git. I\nstill have a few other things to tend to, but hope to come back to the\ndifftool before the end of the week.\n\nCiao,\nJohannes\n"},{"id":"313910","messageId":"20170313175640.14106-1-davvid@gmail.com","threadId":"45209","inReplyTo":"alpine.DEB.2.20.1703072332370.3767@virtualbox","subject":"[PATCH] difftool: handle changing symlinks in dir-diff mode","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2017-03-13T17:56:40Z","receivedAt":"2017-03-13T17:56:50Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Detect the null object ID for symlinks in dir-diff so that difftool can\nprepare temporary files that matches how git handles symlinks.\n\nPreviously, a null object ID would crash difftool. We now detect null\nobject IDs and write the symlink's content into the temporary symlink\nstand-in file.\n\nOriginal-patch-by: Johannes Schindelin \nSigned-off-by: David Aguilar \n---\n builtin/difftool.c | 36 +++++++++++++++++++++++++++++++++---\n t/t7800-difftool.sh | 40 ++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 73 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex d13350ce83..6c20e20b45 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -254,6 +254,31 @@ static int ensure_leading_directories(char *path)\n \t}\n }\n \n+static int create_symlink_file(struct cache_entry* ce, struct checkout* state)\n+{\n+\t/*\n+\t * Dereference a worktree symlink and writes its contents\n+\t * into the checkout state's path.\n+\t */\n+\tstruct strbuf path = STRBUF_INIT;\n+\tstruct strbuf link = STRBUF_INIT;\n+\n+\tint ok = 0;\n+\n+\tif (strbuf_readlink(&link, ce->name, ce_namelen(ce)) == 0) {\n+\t\tstrbuf_add(&path, state->base_dir, state->base_dir_len);\n+\t\tstrbuf_add(&path, ce->name, ce_namelen(ce));\n+\n+\t\twrite_file_buf(path.buf, link.buf, link.len);\n+\t\tok = 1;\n+\t}\n+\n+\tstrbuf_release(&path);\n+\tstrbuf_release(&link);\n+\n+\treturn ok;\n+}\n+\n static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \t\t\tint argc, const char **argv)\n {\n@@ -376,13 +401,13 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \t\t\tcontinue;\n \t\t}\n \n-\t\tif (S_ISLNK(lmode)) {\n+\t\tif (S_ISLNK(lmode) && !is_null_oid(&loid)) {\n \t\t\tchar *content = read_sha1_file(loid.hash, &type, &size);\n \t\t\tadd_left_or_right(&symlinks2, src_path, content, 0);\n \t\t\tfree(content);\n \t\t}\n \n-\t\tif (S_ISLNK(rmode)) {\n+\t\tif (S_ISLNK(rmode) && !is_null_oid(&roid)) {\n \t\t\tchar *content = read_sha1_file(roid.hash, &type, &size);\n \t\t\tadd_left_or_right(&symlinks2, dst_path, content, 1);\n \t\t\tfree(content);\n@@ -414,7 +439,12 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \t\t\t\toidcpy(&ce->oid, &roid);\n \t\t\t\tstrcpy(ce->name, dst_path);\n \t\t\t\tce->ce_namelen = dst_path_len;\n-\t\t\t\tif (checkout_entry(ce, &rstate, NULL))\n+\n+\t\t\t\tif (S_ISLNK(rmode) && is_null_oid(&roid)) {\n+\t\t\t\t\tif (!create_symlink_file(ce, &rstate))\n+\t\t\t\t\t\treturn error(\"unable to create symlink file %s\",\n+\t\t\t\t\t\t\t dst_path);\n+\t\t\t\t} else if (checkout_entry(ce, &rstate, NULL))\n \t\t\t\t\treturn error(\"could not write '%s'\",\n \t\t\t\t\t\t dst_path);\n \t\t\t} else if (!is_null_oid(&roid)) {\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex e1ec292718..64f8e451b5 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -623,4 +623,44 @@ test_expect_success SYMLINKS 'difftool --dir-diff symlinked directories' '\n \t)\n '\n \n+test_expect_success SYMLINKS 'difftool --dir-diff' '\n+\ttouch b &&\n+\tln -s b c &&\n+\tgit add . &&\n+\ttest_tick &&\n+\tgit commit -m initial &&\n+\ttouch d &&\n+\trm c &&\n+\tln -s d c &&\n+\n+\tgit difftool --dir-diff --extcmd ls >output &&\n+\tgrep -v ^/ output >actual &&\n+\tcat >expect <<-EOF &&\n+\t\tb\n+\t\tc\n+\t\tdirlinks\n+\t\toutput\n+\t\tsubmod\n+\n+\t\tc\n+\t\tdirlinks\n+\t\toutput\n+\t\tsubmod\n+\tEOF\n+\ttest_cmp expect actual &&\n+\n+\t# The left side contains symlink \"c\" that points to \"b\"\n+\ttest_config difftool.cat.cmd \"cat \\$LOCAL/c\" &&\n+\tgit difftool --dir-diff --tool cat >actual &&\n+\techo b >expect &&\n+\ttest_cmp expect actual &&\n+\n+\t# The right side contains symlink \"c\" that points to \"d\",\n+\t# which mimics the state of the worktree.\n+\ttest_config difftool.cat.cmd \"cat \\$REMOTE/c\" &&\n+\tgit difftool --dir-diff --tool cat >actual &&\n+\techo -n d >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.12.0.266.g44c9eec009\n\n"},{"id":"313920","messageId":"xmqqlgs9rprt.fsf@gitster.mtv.corp.google.com","threadId":"45209","inReplyTo":"20170313175640.14106-1-davvid@gmail.com","subject":"Re: [PATCH] difftool: handle changing symlinks in dir-diff mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-13T18:32:38Z","receivedAt":"2017-03-13T18:33:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar writes:\n\n> Detect the null object ID for symlinks in dir-diff so that difftool can\n> prepare temporary files that matches how git handles symlinks.\n>\n> Previously, a null object ID would crash difftool. We now detect null\n> object IDs and write the symlink's content into the temporary symlink\n> stand-in file.\n>\n> Original-patch-by: Johannes Schindelin \n> Signed-off-by: David Aguilar \n> ---\n\nI would have appreciated (and I suspect other reviewers would, too)\na bit of back-story wrt how \"Original-patch-by\" resulted in this\npatch after the three-dashes line. It is perfectly fine if you two\ncoordinated privately; I mostly wanted to hear something like \"Dscho\nhas been working on this and I asked him if it is OK to take over\nhis WIP to produce a quick-fix we can ship on the maint branch, here\nis the result of that collaboration.\" IOW, the person who is named\nas the original author is fine to be named like so (I care only\nbecause I do not think we saw the \"original\" here on the list).\n\n> +static int create_symlink_file(struct cache_entry* ce, struct checkout* state)\n\nAsterisk sticks to variable, not type.\n\n> +{\n> +\t/*\n> +\t * Dereference a worktree symlink and writes its contents\n\ns/writes/write/\n\n> +\t * into the checkout state's path.\n> +\t */\n> +\tstruct strbuf path = STRBUF_INIT;\n> +\tstruct strbuf link = STRBUF_INIT;\n> +\n> +\tint ok = 0;\n> +\n> +\tif (strbuf_readlink(&link, ce->name, ce_namelen(ce)) == 0) {\n> +\t\tstrbuf_add(&path, state->base_dir, state->base_dir_len);\n> +\t\tstrbuf_add(&path, ce->name, ce_namelen(ce));\n> +\n> +\t\twrite_file_buf(path.buf, link.buf, link.len);\n\nThis does \"write content into symlink stand-in file\", but why? A\nsymbolic link that is not changed on the right-hand side of the\ncomparison (i.e. S_ISLNK(rmode) && !is_null_oid(&roid)) would go to\ncheckout_entry() which\n\n - creates a symbolic link, on a filesystem that supports symlink; or\n - writes the stand-in file on a filesystem that does not.\n\nwhich is the right thing. It seems that the other side of \"if (!use_wt_file())\"\nif/elseif/... cascade also does the right thing manually.\n\nShouldn't this helper also do the same (i.e. create symlink and fall\nback to stand-in)?\n\nAlso, I am not sure if strbuf_readlink() can unconditionally used\nhere. On a filesystem without symbolic link, the working tree\nentity that corresponds to the ce that represents a symlink is a\nstand-in regular file, so shouldn't we be opening it as a regular\nfile and reading its contents in that case?\n\n"},{"id":"313926","messageId":"xmqqh92xrodr.fsf@gitster.mtv.corp.google.com","threadId":"45209","inReplyTo":"20170313175640.14106-1-davvid@gmail.com","subject":"Re: [PATCH] difftool: handle changing symlinks in dir-diff mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-13T19:02:40Z","receivedAt":"2017-03-13T19:03:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar writes:\n\n> -\t\tif (S_ISLNK(lmode)) {\n> +\t\tif (S_ISLNK(lmode) && !is_null_oid(&loid)) {\n> \t\t\tchar *content = read_sha1_file(loid.hash, &type, &size);\n> \t\t\tadd_left_or_right(&symlinks2, src_path, content, 0);\n> \t\t\tfree(content);\n> \t\t}\n> \n> -\t\tif (S_ISLNK(rmode)) {\n> +\t\tif (S_ISLNK(rmode) && !is_null_oid(&roid)) {\n> \t\t\tchar *content = read_sha1_file(roid.hash, &type, &size);\n> \t\t\tadd_left_or_right(&symlinks2, dst_path, content, 1);\n> \t\t\tfree(content);\n\nOn this part I didn't comment in my previous message, but what is\nthe implication of omitting add-left-or-right and not registering\nthis symbolic link modified in the working tree to the symlinks2\ntable?\n\nI am wondering if these should be more like\n\n\tif (S_ISLNK(lmode) {\n\t\tchar *content = get_symlink(src_path, &loid);\n\t\tadd_left_or_right(&symlinks2, src_path, content, 0);\n free(content);\n\t} \n\nwith get_symlink() helper that does\n\n - if the object name is not 0{40}, read from the object store\n\n - if the object name is 0{40}, that means we need to read the real\n contents from the working tree file, so do the \"readlink(2) if\n symbolic link is supported, otherwise open/read/close the stub\n file sitting there\" thing.\n\nSimilary to the right hand side tree.\n\nDiscarding \"content\" after reading feels wasteful, as that is the\ninformation we would be using when populating the rstate and lstaten\nworking trees later in the loop, but that would probably need a\nlarger surgery to the code to optimize, I would imagine.\n\n\n"},{"id":"313968","messageId":"alpine.DEB.2.20.1703132200290.3767@virtualbox","threadId":"45209","inReplyTo":"xmqqlgs9rprt.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] difftool: handle changing symlinks in dir-diff mode","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-03-13T21:04:11Z","receivedAt":"2017-03-13T21:04:29Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 13 Mar 2017, Junio C Hamano wrote:\n\n> David Aguilar writes:\n> \n> > Detect the null object ID for symlinks in dir-diff so that difftool\n> > can prepare temporary files that matches how git handles symlinks.\n> >\n> > Previously, a null object ID would crash difftool. We now detect null\n> > object IDs and write the symlink's content into the temporary symlink\n> > stand-in file.\n> >\n> > Original-patch-by: Johannes Schindelin \n> > Signed-off-by: David Aguilar \n> > ---\n> \n> I would have appreciated (and I suspect other reviewers would, too)\n> a bit of back-story wrt how \"Original-patch-by\" resulted in this\n> patch after the three-dashes line. It is perfectly fine if you two\n> coordinated privately; I mostly wanted to hear something like \"Dscho\n> has been working on this and I asked him if it is OK to take over\n> his WIP to produce a quick-fix we can ship on the maint branch, here\n> is the result of that collaboration.\" IOW, the person who is named\n> as the original author is fine to be named like so (I care only\n> because I do not think we saw the \"original\" here on the list).\n\nThe story is more like: Johannes started working on this but got pulled\naway by other tasks and sent out a link to the initial patch, along with a\nnote that he hopes to be able to get back to working on that patch before\nlong (but of course he did not get the chance):\n\nhttp://public-inbox.org/git/alpine.DEB.2.20.1703072332370.3767@virtualbox/\n\nThere was no private exchange. I am happy that David picked up the\nproject.\n\nCiao,\nJohannes\n"},{"id":"313971","messageId":"alpine.DEB.2.20.1703132204410.3767@virtualbox","threadId":"45209","inReplyTo":"xmqqlgs9rprt.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] difftool: handle changing symlinks in dir-diff mode","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-03-13T21:27:30Z","receivedAt":"2017-03-13T21:27:46Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 13 Mar 2017, Junio C Hamano wrote:\n\n> David Aguilar writes:\n> \n> > +static int create_symlink_file(struct cache_entry* ce, struct checkout* state)\n> \n> Asterisk sticks to variable, not type.\n\nIf only we had tools to format the code so that authors as well as\nreviewers could concentrate on essential parts of the patches :-)\n\n> > +\t * into the checkout state's path.\n> > +\t */\n> > +\tstruct strbuf path = STRBUF_INIT;\n> > +\tstruct strbuf link = STRBUF_INIT;\n> > +\n> > +\tint ok = 0;\n> > +\n> > +\tif (strbuf_readlink(&link, ce->name, ce_namelen(ce)) == 0) {\n> > +\t\tstrbuf_add(&path, state->base_dir, state->base_dir_len);\n> > +\t\tstrbuf_add(&path, ce->name, ce_namelen(ce));\n> > +\n> > +\t\twrite_file_buf(path.buf, link.buf, link.len);\n> \n> This does \"write content into symlink stand-in file\", but why?\n\nFrom the commit message:\n\n\t> Detect the null object ID for symlinks in dir-diff so that\n\t> difftool can prepare temporary files that matches how git\n\t> handles symlinks.\n\nThe obvious connection: when core.symlinks = false, Git already falls back\nto writing plain files with the link target as contents. This function\ndoes the same, for the same motivation: it is the best we can do in this\ncase.\n\n> Also, I am not sure if strbuf_readlink() can unconditionally used\n> here. On a filesystem without symbolic link, the working tree\n> entity that corresponds to the ce that represents a symlink is a\n> stand-in regular file, so shouldn't we be opening it as a regular\n> file and reading its contents in that case?\n\nI think you are right, we cannot simply call strbuf_readlink(), we would\nhave to check the core_symlinks variable to maybe read the file contents\ninstead.\n\nBut then, it may not be appropriate to read the worktree to begin with...\nsee my reply to the patch that I will send out in a couple of minutes.\n\nCiao,\nJohannes\n"},{"id":"313974","messageId":"xmqqinncrhey.fsf@gitster.mtv.corp.google.com","threadId":"45209","inReplyTo":"alpine.DEB.2.20.1703132204410.3767@virtualbox","subject":"Re: [PATCH] difftool: handle changing symlinks in dir-diff mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-13T21:33:09Z","receivedAt":"2017-03-13T21:33:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> > +\tif (strbuf_readlink(&link, ce->name, ce_namelen(ce)) == 0) {\n>> > +\t\tstrbuf_add(&path, state->base_dir, state->base_dir_len);\n>> > +\t\tstrbuf_add(&path, ce->name, ce_namelen(ce));\n>> > +\n>> > +\t\twrite_file_buf(path.buf, link.buf, link.len);\n>> \n>> This does \"write content into symlink stand-in file\", but why?\n>\n> From the commit message:\n>\n> \t> Detect the null object ID for symlinks in dir-diff so that\n> \t> difftool can prepare temporary files that matches how git\n> \t> handles symlinks.\n\nYes I read what the proposed log message said, and noticed that\nsymbolic link is _always_ written as a regular file.\n\nI was questioning why. I know Git falls back to such behaviour when\nthe filesystem does not support symbolic links. \"Why do so\nunconditionally, even when the filesystem does?\" was the question.\n\n> The obvious connection: when core.symlinks = false, Git already falls back\n> to writing plain files with the link target as contents. This function\n> does the same, for the same motivation: it is the best we can do in this\n> case.\n\nAnd that \"obvious connection\" does not answer the question.\n"},{"id":"313976","messageId":"alpine.DEB.2.20.1703132212430.3767@virtualbox","threadId":"45209","inReplyTo":"20170313175640.14106-1-davvid@gmail.com","subject":"Re: [PATCH] difftool: handle changing symlinks in dir-diff mode","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-03-13T21:36:22Z","receivedAt":"2017-03-13T21:36:42Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi David,\n\nThank you very much for picking this up!\n\nOn Mon, 13 Mar 2017, David Aguilar wrote:\n\n> Detect the null object ID for symlinks in dir-diff so that difftool can\n> prepare temporary files that matches how git handles symlinks.\n\nMaybe a description is needed how the OID can be null in that case. I have\nto admit that I failed to wrap my head around this so far.\n\n> Previously, a null object ID would crash difftool. We now detect null\n> object IDs and write the symlink's content into the temporary symlink\n> stand-in file.\n> \n> Original-patch-by: Johannes Schindelin \n> Signed-off-by: David Aguilar \n> ---\n> diff --git a/builtin/difftool.c b/builtin/difftool.c\n> index d13350ce83..6c20e20b45 100644\n> --- a/builtin/difftool.c\n> +++ b/builtin/difftool.c\n> @@ -254,6 +254,31 @@ static int ensure_leading_directories(char *path)\n> \t}\n> }\n> \n> +static int create_symlink_file(struct cache_entry* ce, struct checkout* state)\n> +{\n> +\t/*\n> +\t * Dereference a worktree symlink and writes its contents\n> +\t * into the checkout state's path.\n> +\t */\n> +\tstruct strbuf path = STRBUF_INIT;\n> +\tstruct strbuf link = STRBUF_INIT;\n> +\n> +\tint ok = 0;\n\nIt would appear that the convention in Git's C code is for functions to\nreturn 0 upon success and -1 upon error, and to use `int ret` for that\npurpose.\n\n> +\tif (strbuf_readlink(&link, ce->name, ce_namelen(ce)) == 0) {\n\nLooking at the calling site, I would have expected the code to read the\ncontents as per ce->hash... After all, we are calling this in the\n!use_wt_file() case.\n\nBut that is exactly that null hash, isn't it?\n\n> @@ -414,7 +439,12 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n\nThe lines before this hunk read:\n\n> if (!use_wt_file(workdir, dst_path, &roid)) {\n> ce->ce_mode = rmode;\n\n... and then follow these lines:\n\n> \t\t\t\toidcpy(&ce->oid, &roid);\n> \t\t\t\tstrcpy(ce->name, dst_path);\n> \t\t\t\tce->ce_namelen = dst_path_len;\n> -\t\t\t\tif (checkout_entry(ce, &rstate, NULL))\n> +\n> +\t\t\t\tif (S_ISLNK(rmode) && is_null_oid(&roid)) {\n> +\t\t\t\t\tif (!create_symlink_file(ce, &rstate))\n> +\t\t\t\t\t\treturn error(\"unable to create symlink file %s\",\n> +\t\t\t\t\t\t\t dst_path);\n> +\t\t\t\t} else if (checkout_entry(ce, &rstate, NULL))\n> \t\t\t\t\treturn error(\"could not write '%s'\",\n> \t\t\t\t\t\t dst_path);\n> \t\t\t} else if (!is_null_oid(&roid)) {\n\nGiven that we explicitly ask use_wt_file() whether we can use the\nworktree's file, and we get the answer \"no\" before we enter the modified\ncode block, I would really expect us *not* to want to read the link from\ndisk at all.\n\nFurther, reading the code of use_wt_file(), there seems to be another case\nwhere roid is left alone: when the file could not be lstat()ed. So I\nwonder whether the create_symlink_file() is the correct solution, or\nwhether we could somehow fill roid correctly instead, and keep the\ncheckout_entry() call?\n\nCiao,\nDscho\n"},{"id":"314023","messageId":"20170314022030.pqqgtaavttzb7rhq@gmail.com","threadId":"45209","inReplyTo":"xmqqinncrhey.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] difftool: handle changing symlinks in dir-diff mode","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2017-03-14T02:20:30Z","receivedAt":"2017-03-14T02:20:41Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Mon, Mar 13, 2017 at 02:33:09PM -0700, Junio C Hamano wrote:\n> Johannes Schindelin writes:\n> \n> >> > +\tif (strbuf_readlink(&link, ce->name, ce_namelen(ce)) == 0) {\n> >> > +\t\tstrbuf_add(&path, state->base_dir, state->base_dir_len);\n> >> > +\t\tstrbuf_add(&path, ce->name, ce_namelen(ce));\n> >> > +\n> >> > +\t\twrite_file_buf(path.buf, link.buf, link.len);\n> >> \n> >> This does \"write content into symlink stand-in file\", but why?\n> >\n> > From the commit message:\n> >\n> > \t> Detect the null object ID for symlinks in dir-diff so that\n> > \t> difftool can prepare temporary files that matches how git\n> > \t> handles symlinks.\n> \n> Yes I read what the proposed log message said, and noticed that\n> symbolic link is _always_ written as a regular file.\n> \n> I was questioning why. I know Git falls back to such behaviour when\n> the filesystem does not support symbolic links. \"Why do so\n> unconditionally, even when the filesystem does?\" was the question.\n> \n> > The obvious connection: when core.symlinks = false, Git already falls back\n> > to writing plain files with the link target as contents. This function\n> > does the same, for the same motivation: it is the best we can do in this\n> > case.\n> \n> And that \"obvious connection\" does not answer the question.\n\nThanks for the thorough review.\n\nI'll try to answer questions from the various emails in just this\none spot in case it helps.\n\nDscho wrote:\n> Given that we explicitly ask use_wt_file() whether we can use the\n> worktree's file, and we get the answer \"no\" before we enter the modified\n> code block, I would really expect us *not* to want to read the link from\n> disk at all.\n\nThat probably deserves a comment on its own.\n\nThe use_wt_file() function really answers the question --\n\"does the worktree contain content that does is unknown to\n Git, and thus we should symlink to the worktree?\"\n\nSince these are symlinks, and symlinks are already used to\npoint back to the worktree (so that difftools will write\nback to the worktree in case the user edited in-tool)\nthen the meaning of use_wt_file() in this context\ncan be misleading.\n\nWhat we're trying to do is handle the case where Git knows it's dealing\nwith an entry that it wants to checkout into a temporary area\nbut it has no way to do so. These entries always have the\n0{40} null SHA1 because that is what git diff-files reports\nfor content that exists in the worktree only.\n\nDscho wrote:\n> I think you are right, we cannot simply call strbuf_readlink(), we would\n> have to check the core_symlinks variable to maybe read the file contents\n> instead.\n>\n> But then, it may not be appropriate to read the worktree to begin\n> with...\n> see my reply to the patch that I will send out in a couple of minutes.\n\nIn this case, where the null SHA1 indicates that it is unknown content,\nthen I believe we must read from the worktree to simulate what Git\nwould have checked out. Checking for core.symlinks should probably be\ndone before calling strbuf_readlink, though.\n\nJunio wrote:\n> > > Detect the null object ID for symlinks in dir-diff so that\n> > > difftool can prepare temporary files that matches how git\n> > > handles symlinks.\n>\n> Yes I read what the proposed log message said, and noticed that\n> symbolic link is _always_ written as a regular file.\n>\n> I was questioning why. I know Git falls back to such behaviour when\n> the filesystem does not support symbolic links. \"Why do so\n> unconditionally, even when the filesystem does?\" was the question.\n\nI have to re-read the code to see where this is special-cased, but\nit seems that symlinks are always written as raw files by the\ndir-diff code for the purposes of full-tree diffing.\n\nI think the \"why\" is tied up in the implementation details of\nthe symlink-back-to-the-worktree-to-allow-editing feature.\nBy special-casing in-tree symlinks and writing them as raw\nfiles we can hijack on-disk symlinks. We use on-disk symlinks to\nlink back to the worktree so that external diff tools can write\nto the worktree through the symlink.\n\nJunio wrote:\n> On this part I didn't comment in my previous message, but what is\n> the implication of omitting add-left-or-right and not registering\n> this symbolic link modified in the working tree to the symlinks2\n> table?\n>\n> I am wondering if these should be more like\n>\n> if (S_ISLNK(lmode) {\n> char *content = get_symlink(src_path, &loid);\n> add_left_or_right(&symlinks2, src_path, content, 0);\n> free(content);\n> }\n>\n> with get_symlink() helper that does\n>\n> - if the object name is not 0{40}, read from the object store\n>\n> - if the object name is 0{40}, that means we need to read the real\n> contents from the working tree file, so do the \"readlink(2) if\n> symbolic link is supported, otherwise open/read/close the stub\n> file sitting there\" thing.\n>\n> Similary to the right hand side tree.\n\nI'll take a look at trying this out.\n\nReading the code again, the point of add_left_or_right()\nis to populate the worktree (done later in the loop) with\nthe stuff we read from Git. Thus, if we changed just this\nsection to call get_symlink() then we should not even try\nto checkout any symlink entries in !use_wt_file(...)\nblock where checkout_entry() / create_symlink_file() are called.\n\nSince the symlinks2 hashmap already populates the worktree\nthen that code should instead simply skip symlinks.\n\n\nLater, once we get to the part where we copy stuff back\ninto the worktree, it should be noted that we always\nskip over symlinks. We simply do not handle them,\nnever have, and I don't think we really should.\n\nThe use case that we're trying to handle is a common\nuse case where the user is using dir-diff and uses the\ndifftool to edit a file with content that exists in the\nworktree only. For that use case, a symlink to the worktree\nis created in the temp area, and Git does not need to do\nanything special.\n\nI do not think the use case of a user editing a symlink\nstand-in file, and then having Git update the worktree\nwith the updated content, is common or something we\nwant to support. I'd prefer to keep the use case simple\nsince the code is already complicated enough.\n\nI'll take a stab at adding a get_symlink() helper, adjust\nthe code so that add_left_or_right() is populated, and\nspecial-case the checkout_entry() code path to simply skip\nover null SHA1s.\n\nI'll also address the review notes and try to add more\ncomments to describe what exactly this code does and why\nit does it.\n\nDo the tests make sense?\n\nOne minor thing I noticed is that I had to use \"echo -n\"\nfor the stuff coming out of strbuf_readlink(), and\nplain \"echo\" for entries that come in via read_sha1_file()\ncontent passed to add_left_or_right().\n\nThat suggests that maybe I should append a newline to the\noutput from strbuf_readlink() so that it matches Git.\nDoes that sound right? Does Git store symlink entries\nwith a terminating newline?\n\nPlease let me know if I'm missing something.\n\ncheers,\n-- \nDavid\n"},{"id":"314024","messageId":"xmqq1su0lcm3.fsf@gitster.mtv.corp.google.com","threadId":"45209","inReplyTo":"xmqqlgs9rprt.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] difftool: handle changing symlinks in dir-diff mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-14T04:13:24Z","receivedAt":"2017-03-14T04:15:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> +\tstruct strbuf path = STRBUF_INIT;\n>> +\tstruct strbuf link = STRBUF_INIT;\n>> +\n>> +\tint ok = 0;\n>> +\n>> +\tif (strbuf_readlink(&link, ce->name, ce_namelen(ce)) == 0) {\n>> +\t\tstrbuf_add(&path, state->base_dir, state->base_dir_len);\n>> +\t\tstrbuf_add(&path, ce->name, ce_namelen(ce));\n>> +\n>> +\t\twrite_file_buf(path.buf, link.buf, link.len);\n>\n> This does \"write content into symlink stand-in file\", but why? A\n> symbolic link that is not changed on the right-hand side of the\n> comparison (i.e. S_ISLNK(rmode) && !is_null_oid(&roid)) would go to\n> checkout_entry() which\n>\n> - creates a symbolic link, on a filesystem that supports symlink; or\n> - writes the stand-in file on a filesystem that does not.\n>\n> which is the right thing. It seems that the other side of \"if (!use_wt_file())\"\n> if/elseif/... cascade also does the right thing manually.\n>\n> Shouldn't this helper also do the same (i.e. create symlink and fall\n> back to stand-in)?\n>\n> Also, I am not sure if strbuf_readlink() can unconditionally used\n> here. On a filesystem without symbolic link, the working tree\n> entity that corresponds to the ce that represents a symlink is a\n> stand-in regular file, so shouldn't we be opening it as a regular\n> file and reading its contents in that case?\n\nI _think_ I was mistaken (please correct me again if my reasoning\nbelow is wrong) and unconditional writing of a plain regular file is\nwhat this codepath wants to do, because we are preparing two temporary\ndirectories, to be fed to a Git-unaware tool that knows how to show\na diff of two directories (e.g. \"diff -r A B\"). Because the tool is\nGit-unaware, if a symbolic link appears in either of these temporary\ndirectories, it will try to dereference and show the difference of\nthe target of the symbolic link, which is not what we want, as the\ngoal of the dir-diff mode is to produce an output that is logically\nequivalent to what \"git diff\" produces---most importantly, we want\nto get textual comparison of the result of the readlink(2). And\nwrite_file_buf() used here is exactly that---we write the contents\nof symlink to a regular file to force the external tool to compare\nthe readlink(2) result as if it is a text. Even on a filesystem\nthat is capable of doing a symbolic link.\n\nThe strbuf_readlink() that read the contents of symlink, however,\nis wrong on a filesystem that is not capable of a symbolic link; if\ncore.symlinks is false, we do need to read them as a regular file.\n\n"},{"id":"314025","messageId":"xmqqwpbsjthb.fsf@gitster.mtv.corp.google.com","threadId":"45209","inReplyTo":"20170314022030.pqqgtaavttzb7rhq@gmail.com","subject":"Re: [PATCH] difftool: handle changing symlinks in dir-diff mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-14T05:52:00Z","receivedAt":"2017-03-14T05:52:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar writes:\n\n> Reading the code again, the point of add_left_or_right()\n> is to populate the worktree (done later in the loop) with\n> the stuff we read from Git. Thus, if we changed just this\n> section to call get_symlink() then we should not even try\n> to checkout any symlink entries in !use_wt_file(...)\n> block where checkout_entry() / create_symlink_file() are called.\n> \n> Since the symlinks2 hashmap already populates the worktree\n> then that code should instead simply skip symlinks.\n\nOK, that would simplify things, certainly.\n\n> I'll take a stab at adding a get_symlink() helper, adjust\n> the code so that add_left_or_right() is populated, and\n> special-case the checkout_entry() code path to simply skip\n> over null SHA1s.\n\nDid you mean s/null SHA1s/S_ISLNK()s/?\n\n> One minor thing I noticed is that I had to use \"echo -n\"\n> for the stuff coming out of strbuf_readlink(), and\n> plain \"echo\" for entries that come in via read_sha1_file()\n> content passed to add_left_or_right().\n>\n> That suggests that maybe I should append a newline to the\n> output from strbuf_readlink() so that it matches Git.\n> Does that sound right? Does Git store symlink entries\n> with a terminating newline?\n\nDo not append a newline. Unless the pathname of the target you are\nsymlinking to ends with LF, readlink() won't end with LF, and the\nstand-in file shouldn't, either.\n\nBy the way, avoid \"echo -n\"; use \"printf '%s'\" instead.\n\nThanks.\n\n"}]}