{"thread":{"id":"44616","subject":"[PATCH] diff: fix up SHA-1 abbreviations outside of repository","startedAt":"2016-12-04T19:48:53Z","lastAt":"2016-12-05T22:43:53Z","messageCount":6,"participants":["Jack Bates","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"306898","messageId":"20161204194747.7100-1-jack@nottheoilrig.com","threadId":"44616","inReplyTo":null,"subject":"[PATCH] diff: fix up SHA-1 abbreviations outside of repository","fromName":"Jack Bates","fromEmail":"bk874k@nottheoilrig.com","sentAt":"2016-12-04T19:47:47Z","receivedAt":"2016-12-04T19:48:53Z","isPatch":true,"sender":{"key":"bk874k@nottheoilrig.com","avatar":null},"body":"The three cases where \"git diff\" operates outside of a repository are 1)\nwhen we run it outside of a repository, 2) when one of the files we're\ncomparing is outside of the repository we're in, and 3) the --no-index\noption. Commit 4f03666 (\"diff: handle sha1 abbreviations outside of\nrepository\", 2016-10-20) only worked in the first case.\n---\n builtin/diff.c                  |  4 +++-\n t/t4063-diff-no-index-abbrev.sh | 50 +++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 53 insertions(+), 1 deletion(-)\n create mode 100755 t/t4063-diff-no-index-abbrev.sh\n\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 7f91f6d..ec7c432 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -342,9 +342,11 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\t       \"--no-index\" : \"[--no-index]\");\n \n \t}\n-\tif (no_index)\n+\tif (no_index) {\n \t\t/* If this is a no-index diff, just run it and exit there. */\n+\t\tstartup_info->have_repository = 0;\n \t\tdiff_no_index(&rev, argc, argv);\n+\t}\n \n \t/* Otherwise, we are doing the usual \"git\" diff */\n \trev.diffopt.skip_stat_unmatch = !!diff_auto_refresh_index;\ndiff --git a/t/t4063-diff-no-index-abbrev.sh b/t/t4063-diff-no-index-abbrev.sh\nnew file mode 100755\nindex 0000000..d1d6302\n--- /dev/null\n+++ b/t/t4063-diff-no-index-abbrev.sh\n@@ -0,0 +1,50 @@\n+#!/bin/sh\n+\n+test_description='don'\\'' peek into .git when operating outside of a repository\n+\n+When abbreviating SHA-1s, if another object in the repository has the\n+same abbreviation, we normally lengthen the abbreviation until it'\\''s\n+unique. Commit 4f03666 (\"diff: handle sha1 abbreviations outside of\n+repository\", 2016-10-20) addressed the case of abbreviating SHA-1s\n+outside the context of a repository. In that case we shouldn'\\''t peek\n+into a .git directory to make an abbreviation unique.\n+\n+To check that we don'\\''t, create an blob with a SHA-1 that starts with\n+0000. (Outside of a repository, SHA-1s are all zeros.) Then make an\n+abbreviation and check that Git doesn'\\''t lengthen it.\n+\n+The three cases where \"git diff\" operates outside of a repository are\n+1) when we run it outside of a repository, 2) when one of the files\n+we'\\''re comparing is outside of the repository we'\\''re in,\n+and 3) the --no-index option.\n+'\n+\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\techo 1 >a &&\n+\techo 2 >b &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\t# Create a blob\n+\t\t# 00002e907f44c3881822c473d8842405cfd96362\n+\t\techo 119132 >collision &&\n+\t\tgit add collision\n+\t)\n+'\n+\n+cat >expect <<EOF\n+:100644 100644 0000... 0000... M\t../a\n+EOF\n+\n+test_expect_success 'don'\\''t peek into .git when operating outside of a repository' '\n+\t(\n+\t\tcd repo &&\n+\t\ttest_must_fail git diff --raw --abbrev=4 ../a ../b >actual &&\n+\t\ttest_cmp ../expect actual\n+\t)\n+'\n+\n+test_done\n-- \n2.10.2\n"},{"id":"306913","messageId":"20161205065523.yspqt34p3dp5g5fk@sigill.intra.peff.net","threadId":"44616","inReplyTo":"20161204194747.7100-1-jack@nottheoilrig.com","subject":"Re: [PATCH] diff: fix up SHA-1 abbreviations outside of repository","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-05T06:55:23Z","receivedAt":"2016-12-05T06:55:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Dec 04, 2016 at 12:47:47PM -0700, Jack Bates wrote:\n\n> The three cases where \"git diff\" operates outside of a repository are 1)\n> when we run it outside of a repository, 2) when one of the files we're\n> comparing is outside of the repository we're in, and 3) the --no-index\n> option. Commit 4f03666 (\"diff: handle sha1 abbreviations outside of\n> repository\", 2016-10-20) only worked in the first case.\n\nYou didn't define \"worked\" here. From looking at your patch, it looks\nlike we look look in the object database for abbreviations in the\n--no-index case, but you think we shouldn't.\n\nI'm not sure I agree. The \"--no-index\" option asks git not to treat the\narguments as pathspecs, but instead as two filesystem files to diff.\nBut should it ignore the repository entirely? One use case is to just\nask for the diff of two files:\n\n  git diff --no-index foo bar\n\nwhich may or may not be tracked in the repository. For abbreviations, I\ndoubt that people would care much, but see below.\n\n> diff --git a/builtin/diff.c b/builtin/diff.c\n> index 7f91f6d..ec7c432 100644\n> --- a/builtin/diff.c\n> +++ b/builtin/diff.c\n> @@ -342,9 +342,11 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n>  \t\t       \"--no-index\" : \"[--no-index]\");\n>  \n>  \t}\n> -\tif (no_index)\n> +\tif (no_index) {\n>  \t\t/* If this is a no-index diff, just run it and exit there. */\n> +\t\tstartup_info->have_repository = 0;\n>  \t\tdiff_no_index(&rev, argc, argv);\n> +\t}\n\nThis reset of have_repository would affect more than just abbreviations.\nWe may also look at the repository for attributes, config, etc.  For\ninstance, right now:\n\n  echo \"*.pdf diff=pdf\" >.git/info/attributes\n  git config diff.pdf.textconv pdftotext\n  git diff --no-index --textconv foo.pdf bar.pdf\n\nwill show a text diff of the two files, but wouldn't after your patch.\n\n(I actually think even needing to say --textconv is actually a bug;\nnormally the diff porcelain defaults to --textconv, but that setup is\nskipped in the no-index case).\n\n> +To check that we don'\\''t, create an blob with a SHA-1 that starts with\n> +0000. (Outside of a repository, SHA-1s are all zeros.) Then make an\n> +abbreviation and check that Git doesn'\\''t lengthen it.\n\nThat's not always true. If we actually diff the file contents, the sha1s\nare correct (which you can see in the default --patch output). It's only\nin the --raw case that the sha1 is all zeroes.\n\nI'm not 100% sure that isn't a bug.  In a normal git diff, we can show\nthe sha1s without actually looking at the file content, because we get\nthem from either the containing tree or the index. In a --no-index diff,\nwe create the diff_filespec structs without a valid sha1. But we can't\nget away from reading the files eventually. The magic happens in\ndiffcore_skip_stat_unmatch(), which actually does a series of checks,\nculminating in a size-check and then a comparison of the contents.\n\nI suppose it _is_ faster than computing the actual sha1, because we can\nsometimes show \"modified\" by only looking at the size, or the first few\nbytes. But any time two files are identical, we pay the full cost. So if\nyou're comparing two hierarchies, say, like:\n\n  git diff --raw one/ two/\n\nit's probably not much more expensive to compare with the full sha1s,\nbecause we're already reading the entire contents of every file that's\nthe same.\n\nSo I dunno. It kind of surprised me that anybody was using \"--no-index\n--raw\" in the first place, so maybe there is some use case I'm missing.\n\n-Peff\n"},{"id":"306917","messageId":"xmqqr35m6dwt.fsf@gitster.mtv.corp.google.com","threadId":"44616","inReplyTo":"20161204194747.7100-1-jack@nottheoilrig.com","subject":"Re: [PATCH] diff: fix up SHA-1 abbreviations outside of repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-05T07:19:46Z","receivedAt":"2016-12-05T07:20:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jack Bates <bk874k@nottheoilrig.com> writes:\n\n> The three cases where \"git diff\" operates outside of a repository are 1)\n> when we run it outside of a repository, 2) when one of the files we're\n> comparing is outside of the repository we're in, and 3) the --no-index\n> option. Commit 4f03666 (\"diff: handle sha1 abbreviations outside of\n> repository\", 2016-10-20) only worked in the first case.\n> ---\n>  builtin/diff.c                  |  4 +++-\n>  t/t4063-diff-no-index-abbrev.sh | 50 +++++++++++++++++++++++++++++++++++++++++\n>  2 files changed, 53 insertions(+), 1 deletion(-)\n>  create mode 100755 t/t4063-diff-no-index-abbrev.sh\n>\n> diff --git a/builtin/diff.c b/builtin/diff.c\n> index 7f91f6d..ec7c432 100644\n> --- a/builtin/diff.c\n> +++ b/builtin/diff.c\n> @@ -342,9 +342,11 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n>  \t\t       \"--no-index\" : \"[--no-index]\");\n>  \n>  \t}\n> -\tif (no_index)\n> +\tif (no_index) {\n>  \t\t/* If this is a no-index diff, just run it and exit there. */\n> +\t\tstartup_info->have_repository = 0;\n>  \t\tdiff_no_index(&rev, argc, argv);\n> +\t}\n\nThis kind of change makes me nervous (partly because I am not seeing\nthe whole code but only this part of the patch).\n\nSome code may react to \"have_repository\" being zero and do the right\nthing (which I think is what you are using from your previous \"we\ndid one of the three cases\" change here), but the codepath that led\nto \"have_repository\" being set to non-zero previously must have done\na lot more than just flipping that field to non-zero, and setting\nzero to this field alone would not \"undo\" what it did.\n\nI wonder if we're better off if we made sure that diff_no_index()\nworks the same way regardless of the value of \"have_repository\"\nfield?\n\n\n\n"},{"id":"306919","messageId":"20161205072614.zg6yglqnznna65vf@sigill.intra.peff.net","threadId":"44616","inReplyTo":"xmqqr35m6dwt.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] diff: fix up SHA-1 abbreviations outside of repository","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-05T07:26:14Z","receivedAt":"2016-12-05T07:26:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Dec 04, 2016 at 11:19:46PM -0800, Junio C Hamano wrote:\n\n> > -\tif (no_index)\n> > +\tif (no_index) {\n> >  \t\t/* If this is a no-index diff, just run it and exit there. */\n> > +\t\tstartup_info->have_repository = 0;\n> >  \t\tdiff_no_index(&rev, argc, argv);\n> > +\t}\n> \n> This kind of change makes me nervous (partly because I am not seeing\n> the whole code but only this part of the patch).\n> \n> Some code may react to \"have_repository\" being zero and do the right\n> thing (which I think is what you are using from your previous \"we\n> did one of the three cases\" change here), but the codepath that led\n> to \"have_repository\" being set to non-zero previously must have done\n> a lot more than just flipping that field to non-zero, and setting\n> zero to this field alone would not \"undo\" what it did.\n\nI _think_ it's OK because the only substantive change would be the\nchdir() to the top of the working tree. But that information is carried\nthrough by revs->prefix, which we act on regardless of the value of\nstartup_info->have_repository when we call prefix_filename().\n\nI agree that it may be an accident waiting to happen, though, as soon as\nsome buried sub-function needs to care about the distinction.\n\n> I wonder if we're better off if we made sure that diff_no_index()\n> works the same way regardless of the value of \"have_repository\"\n> field?\n\nIf you mean adding a diffopt flag like \"just abbreviate everything to\nFALLBACK_DEFAULT_ABBREV even if we're in a repository\", and then setting\nthat in diff_no_index(), I agree that is a lot cleaner.\n\nI'm still not 100% convinced that it's actually the correct behavior,\nbut at least doing a more contained version wouldn't take away other\nfunctionality like reading config.\n\n-Peff\n"},{"id":"306938","messageId":"66e03fae-d78e-2577-3f27-40e36fdc6ee1@nottheoilrig.com","threadId":"44616","inReplyTo":"20161205072614.zg6yglqnznna65vf@sigill.intra.peff.net","subject":"Re: [PATCH] diff: fix up SHA-1 abbreviations outside of repository","fromName":"Jack Bates","fromEmail":"bk874k@nottheoilrig.com","sentAt":"2016-12-05T17:45:08Z","receivedAt":"2016-12-05T17:45:15Z","isPatch":true,"sender":{"key":"bk874k@nottheoilrig.com","avatar":null},"body":"On 05/12/16 12:26 AM, Jeff King wrote:\n> On Sun, Dec 04, 2016 at 11:19:46PM -0800, Junio C Hamano wrote:\n>>> -\tif (no_index)\n>>> +\tif (no_index) {\n>>>  \t\t/* If this is a no-index diff, just run it and exit there. */\n>>> +\t\tstartup_info->have_repository = 0;\n>>>  \t\tdiff_no_index(&rev, argc, argv);\n>>> +\t}\n>>\n>> This kind of change makes me nervous (partly because I am not seeing\n>> the whole code but only this part of the patch).\n>>\n>> Some code may react to \"have_repository\" being zero and do the right\n>> thing (which I think is what you are using from your previous \"we\n>> did one of the three cases\" change here), but the codepath that led\n>> to \"have_repository\" being set to non-zero previously must have done\n>> a lot more than just flipping that field to non-zero, and setting\n>> zero to this field alone would not \"undo\" what it did.\n>\n> I _think_ it's OK because the only substantive change would be the\n> chdir() to the top of the working tree. But that information is carried\n> through by revs->prefix, which we act on regardless of the value of\n> startup_info->have_repository when we call prefix_filename().\n>\n> I agree that it may be an accident waiting to happen, though, as soon as\n> some buried sub-function needs to care about the distinction.\n>\n>> I wonder if we're better off if we made sure that diff_no_index()\n>> works the same way regardless of the value of \"have_repository\"\n>> field?\n>\n> If you mean adding a diffopt flag like \"just abbreviate everything to\n> FALLBACK_DEFAULT_ABBREV even if we're in a repository\", and then setting\n> that in diff_no_index(), I agree that is a lot cleaner.\n>\n> I'm still not 100% convinced that it's actually the correct behavior,\n> but at least doing a more contained version wouldn't take away other\n> functionality like reading config.\n\nI don't have a strong reason for wanting these three cases to behave \nidentically, I was merely surprised that they don't. I think you \nexpected them to behave the same as well? I'll withdraw this patch.\n\nConceptually I do think of \"git diff\" as having two separate modes, \"in \nrepository\" and \"out of repository\", with the --no-index option forcing \nthe \"out of repository\" mode. But maybe there are good reasons why this \nisn't accurate, or maybe it doesn't matter that it's not 100% accurate.\n\nIn summary, currently all of the three cases are \"no index\" but only the \nfirst case doesn't \"have repository\".\n\nThank you for your thoughtful feedback!\n"},{"id":"306976","messageId":"xmqqmvga2dzx.fsf@gitster.mtv.corp.google.com","threadId":"44616","inReplyTo":"20161205072614.zg6yglqnznna65vf@sigill.intra.peff.net","subject":"Re: [PATCH] diff: fix up SHA-1 abbreviations outside of repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-05T22:43:46Z","receivedAt":"2016-12-05T22:43:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I agree that it may be an accident waiting to happen, though, as soon as\n> some buried sub-function needs to care about the distinction.\n\nYes to that.\n\n>> I wonder if we're better off if we made sure that diff_no_index()\n>> works the same way regardless of the value of \"have_repository\"\n>> field?\n>\n> If you mean adding a diffopt flag like \"just abbreviate everything to\n> FALLBACK_DEFAULT_ABBREV even if we're in a repository\", and then setting\n> that in diff_no_index(), I agree that is a lot cleaner.\n\nI am not sure if that is what I meant (I no longer sure what exactly\nI meant to say there TBH), but this is probably not limited to the\ndefault abbrev length aka core.abbrev configuration.  Don't we have\nother configuration settings we may read from $HOME/.gitconfig (and\npossibly per-repository .git/config, if we did discovery but were\nexplicitly given \"--no-index\") that want to affect the behaviour of\nthe command?  \n\nI guess what I wanted, with \"the same way\", to see happen was that\n\"have_repository\" should be only controling how and from what files\nthe configuration is read, and the behaviour of the command should\nbe controlled by the values read from the configuration after that.\n\nSpecifically, even if we were running with \"--no-index\", if we know\nwe have access to the current repository discovered by setting it up\ngently, I do not think it is bad to ask find_unique_abbrev() to come\nup with an appropriate abbreviation.  So the fact that patch in\nquestion has to flip the have_repository bit off, if it is done in\norder to affect what diff_abbrev_oid() does, smells quite fishy from\nthat point of view.\n\n"}]}