{"thread":{"id":"47375","subject":"gitattributes not read for diff-tree anymore in 2.15?","startedAt":"2017-12-04T21:23:07Z","lastAt":"2017-12-06T22:07:27Z","messageCount":15,"participants":["Ben Boeckel","Brandon Williams","Junio C Hamano","Stefan Beller","Eric Sunshine"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"334087","messageId":"20171204212255.GA19059@megas.kitware.com","threadId":"47375","inReplyTo":null,"subject":"gitattributes not read for diff-tree anymore in 2.15?","fromName":"Ben Boeckel","fromEmail":"ben.boeckel@kitware.com","sentAt":"2017-12-04T21:22:55Z","receivedAt":"2017-12-04T21:23:07Z","isPatch":false,"sender":{"key":"ben.boeckel@kitware.com","avatar":null},"body":"Hi,\n\nI've bisected a failure in our test suite to this commit:\n\n commit 557a5998df19faf8641acfc5b6b1c3c2ba64dca9 (HEAD, refs/bisect/bad)\n Author: Brandon Williams \n Date: Thu Aug 3 11:20:00 2017 -0700\n\n submodule: remove gitmodules_config\n\n Now that the submodule-config subsystem can lazily read the gitmodules\n file we no longer need to explicitly pre-read the gitmodules by calling\n 'gitmodules_config()' so let's remove it.\n\n Signed-off-by: Brandon Williams \n Signed-off-by: Junio C Hamano \n\nWhich is tags/v2.15.0-rc0~120^2.\n\nOur test suite is in a Rust project here:\n\n https://gitlab.kitware.com/utils/rust-git-checks\n\nand the failing test(s) can be run using:\n\n cargo test whitespace_all_ignored\n\nThe test checks that when `.gitattributes` says that whitespace errors\nshould be ignored, they aren't reported as errors. My guess is that not\nreading the gitmodules configuration also skips reading attributes\nfiles. Is this reasoning correct?\n\nThanks,\n\n--Ben\n"},{"id":"334106","messageId":"20171204230355.GA52452@google.com","threadId":"47375","inReplyTo":"20171204212255.GA19059@megas.kitware.com","subject":"Re: gitattributes not read for diff-tree anymore in 2.15?","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-12-04T23:03:55Z","receivedAt":"2017-12-04T23:04:03Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 12/04, Ben Boeckel wrote:\n> Hi,\n> \n> I've bisected a failure in our test suite to this commit:\n> \n> commit 557a5998df19faf8641acfc5b6b1c3c2ba64dca9 (HEAD, refs/bisect/bad)\n> Author: Brandon Williams \n> Date: Thu Aug 3 11:20:00 2017 -0700\n> \n> submodule: remove gitmodules_config\n> \n> Now that the submodule-config subsystem can lazily read the gitmodules\n> file we no longer need to explicitly pre-read the gitmodules by calling\n> 'gitmodules_config()' so let's remove it.\n> \n> Signed-off-by: Brandon Williams \n> Signed-off-by: Junio C Hamano \n> \n> Which is tags/v2.15.0-rc0~120^2.\n> \n> Our test suite is in a Rust project here:\n> \n> https://gitlab.kitware.com/utils/rust-git-checks\n> \n> and the failing test(s) can be run using:\n> \n> cargo test whitespace_all_ignored\n> \n> The test checks that when `.gitattributes` says that whitespace errors\n> should be ignored, they aren't reported as errors. My guess is that not\n> reading the gitmodules configuration also skips reading attributes\n> files. Is this reasoning correct?\n> \n> Thanks,\n> \n> --Ben\n\nReading the attributes files should be done regardless if the gitmodules\nfile is read. The gitmodules file should only come into play if you are\ndealing with submodules.\n\nDo you mind providing a reproduction recipe with expected outcome vs\nactual outcome and I can take a closer look.\n\n-- \nBrandon Williams\n"},{"id":"334157","messageId":"20171205154244.GA16581@megas.kitware.com","threadId":"47375","inReplyTo":"20171204230355.GA52452@google.com","subject":"Re: gitattributes not read for diff-tree anymore in 2.15?","fromName":"Ben Boeckel","fromEmail":"ben.boeckel@kitware.com","sentAt":"2017-12-05T15:42:44Z","receivedAt":"2017-12-05T15:42:53Z","isPatch":false,"sender":{"key":"ben.boeckel@kitware.com","avatar":null},"body":"On Mon, Dec 04, 2017 at 15:03:55 -0800, Brandon Williams wrote:\n> Reading the attributes files should be done regardless if the gitmodules\n> file is read. The gitmodules file should only come into play if you are\n> dealing with submodules.\n\nYeah, it doesn't seem to make sense why this commit breaks us, but there\nit is *shrug*.\n\n> Do you mind providing a reproduction recipe with expected outcome vs\n> actual outcome and I can take a closer look.\n\nI'll work on one. It isn't as simple as I thought it was :) . The setup\nwe do before running the checks is apparently involved as running it\nfrom the command line is not exhibiting the difference.\n\n--Ben\n"},{"id":"334208","messageId":"20171205181645.GA159917@google.com","threadId":"47375","inReplyTo":"20171205154244.GA16581@megas.kitware.com","subject":"Re: gitattributes not read for diff-tree anymore in 2.15?","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-12-05T18:16:45Z","receivedAt":"2017-12-05T18:16:53Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 12/05, Ben Boeckel wrote:\n> On Mon, Dec 04, 2017 at 15:03:55 -0800, Brandon Williams wrote:\n> > Reading the attributes files should be done regardless if the gitmodules\n> > file is read. The gitmodules file should only come into play if you are\n> > dealing with submodules.\n> \n> Yeah, it doesn't seem to make sense why this commit breaks us, but there\n> it is *shrug*.\n\nWhile it doesn't make the most sense, I still wouldn't be surprised if I\nmissed something when writing that patch that inadvertently caused an\nissue.\n\n> \n> > Do you mind providing a reproduction recipe with expected outcome vs\n> > actual outcome and I can take a closer look.\n> \n> I'll work on one. It isn't as simple as I thought it was :) . The setup\n> we do before running the checks is apparently involved as running it\n> from the command line is not exhibiting the difference.\n> \n> --Ben\n\nPerfect, thanks!\n\n-- \nBrandon Williams\n"},{"id":"334213","messageId":"20171205194801.GA31721@megas.kitware.com","threadId":"47375","inReplyTo":"20171205181645.GA159917@google.com","subject":"Re: gitattributes not read for diff-tree anymore in 2.15?","fromName":"Ben Boeckel","fromEmail":"ben.boeckel@kitware.com","sentAt":"2017-12-05T19:48:01Z","receivedAt":"2017-12-05T19:48:12Z","isPatch":false,"sender":{"key":"ben.boeckel@kitware.com","avatar":null},"body":"On Tue, Dec 05, 2017 at 10:16:45 -0800, Brandon Williams wrote:\n> Perfect, thanks!\n\nOK, attached is a shell script which recreates the issue. I haven't been\nable to get it to happen without the `GIT_WORK_TREE` and `GIT_INDEX_FILE`\nsetup involved, so that seems to be important.\n\nI reran the bisect using this script and came up with this commit:\n\n commit be4ca290570f9173db64ea1f925b5b3831c6efed\n Author: David Turner \n Date: Thu Apr 20 16:41:18 2017 -0400\n\n Increase core.packedGitLimit\n\n \n\nwhich seems even less relevant…\n\nThanks,\n\n--Ben\n"},{"id":"334225","messageId":"20171205221337.140548-1-bmwill@google.com","threadId":"47375","inReplyTo":"20171205194801.GA31721@megas.kitware.com","subject":"[PATCH] diff-tree: read the index so attribute checks work in bare repositories","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-12-05T22:13:37Z","receivedAt":"2017-12-05T22:13:48Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"A regression was introduced in 557a5998d (submodule: remove\ngitmodules_config, 2017-08-03) to how attribute processing was handled\nin bare repositories when running the diff-tree command.\n\nBy default the attribute system will first try to read \".gitattribute\"\nfiles from the working tree and then falls back to reading them from the\nindex if there isn't a copy checked out in the worktree. Prior to\n557a5998d the index was read as a side effect of the call to\n'gitmodules_config()' which ensured that the index was already populated\nbefore entering the attribute subsystem.\n\nSince the call to 'gitmodules_config()' was removed the index is no\nlonger being read so when the attribute system tries to read from the\nin-memory index it doesn't find any \".gitattribute\" entries effectively\nignoring any configured attributes.\n\nFix this by explicitly reading the index during the setup of diff-tree.\n\nReported-by: Ben Boeckel \nSigned-off-by: Brandon Williams \n---\n\nThis patch should fix the regression. Let me know if it doesn't solve the\nissue and I'll investigate some more.\n\n builtin/diff-tree.c | 1 +\n t/t4015-diff-whitespace.sh | 17 +++++++++++++++++\n 2 files changed, 18 insertions(+)\n\ndiff --git a/builtin/diff-tree.c b/builtin/diff-tree.c\nindex d66499909..cfe7d0281 100644\n--- a/builtin/diff-tree.c\n+++ b/builtin/diff-tree.c\n@@ -110,6 +110,7 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)\n \n \tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n \tinit_revisions(opt, prefix);\n+\tread_cache();\n \topt->abbrev = 0;\n \topt->diff = 1;\n \topt->disable_stdin = 1;\ndiff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nindex 559a7541a..6e061a002 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -636,6 +636,23 @@ test_expect_success 'check with space before tab in indent (diff-tree)' '\n \ttest_must_fail git diff-tree --check HEAD^ HEAD\n '\n \n+test_expect_success 'check with ignored trailing whitespace attr (diff-tree)' '\n+\ttest_when_finished \"git reset --hard HEAD^\" &&\n+\n+\t# Create a whitespace error that should be ignored.\n+\techo \"* -whitespace\" > \".gitattributes\" &&\n+\tgit add \".gitattributes\" &&\n+\techo \"trailing space -> \" > \"trailing-space\" &&\n+\tgit add \"trailing-space\" &&\n+\tgit commit -m \"trailing space\" &&\n+\n+\t# With a worktree diff-tree ignores the whitespace error\n+\tgit diff-tree --root --check HEAD &&\n+\n+\t# Without a worktree diff-tree still ignores the whitespace error\n+\tgit -C .git diff-tree --root --check HEAD\n+'\n+\n test_expect_success 'check trailing whitespace (trailing-space: off)' '\n \tgit config core.whitespace \"-trailing-space\" &&\n \techo \"foo (); \" >x &&\n-- \n2.15.1.424.g9478a66081-goog\n\n"},{"id":"334229","messageId":"20171205222954.GA9217@megas.kitware.com","threadId":"47375","inReplyTo":"20171205221337.140548-1-bmwill@google.com","subject":"Re: [PATCH] diff-tree: read the index so attribute checks work in bare repositories","fromName":"Ben Boeckel","fromEmail":"ben.boeckel@kitware.com","sentAt":"2017-12-05T22:29:54Z","receivedAt":"2017-12-05T22:30:01Z","isPatch":true,"sender":{"key":"ben.boeckel@kitware.com","avatar":null},"body":"On Tue, Dec 05, 2017 at 14:13:37 -0800, Brandon Williams wrote:\n> This patch should fix the regression. Let me know if it doesn't solve the\n> issue and I'll investigate some more.\n\nOur test suite passes again. Thanks!\n\n Acked-by: Ben Boeckel \n\n--Ben\n"},{"id":"334230","messageId":"20171205223115.GA36335@google.com","threadId":"47375","inReplyTo":"20171205222954.GA9217@megas.kitware.com","subject":"Re: [PATCH] diff-tree: read the index so attribute checks work in bare repositories","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-12-05T22:31:15Z","receivedAt":"2017-12-05T22:31:34Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 12/05, Ben Boeckel wrote:\n> On Tue, Dec 05, 2017 at 14:13:37 -0800, Brandon Williams wrote:\n> > This patch should fix the regression. Let me know if it doesn't solve the\n> > issue and I'll investigate some more.\n> \n> Our test suite passes again. Thanks!\n\nOf course! Glad I could help :)\n\n> \n> Acked-by: Ben Boeckel \n> \n> --Ben\n\n-- \nBrandon Williams\n"},{"id":"334231","messageId":"xmqqy3mghiot.fsf@gitster.mtv.corp.google.com","threadId":"47375","inReplyTo":"20171205221337.140548-1-bmwill@google.com","subject":"Re: [PATCH] diff-tree: read the index so attribute checks work in bare repositories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-05T23:13:38Z","receivedAt":"2017-12-05T23:13:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams writes:\n\n> A regression was introduced in 557a5998d (submodule: remove\n> gitmodules_config, 2017-08-03) to how attribute processing was handled\n> in bare repositories when running the diff-tree command.\n>\n> By default the attribute system will first try to read \".gitattribute\"\n> files from the working tree and then falls back to reading them from the\n> index if there isn't a copy checked out in the worktree. Prior to\n> 557a5998d the index was read as a side effect of the call to\n> 'gitmodules_config()' which ensured that the index was already populated\n> before entering the attribute subsystem.\n>\n> Since the call to 'gitmodules_config()' was removed the index is no\n> longer being read so when the attribute system tries to read from the\n> in-memory index it doesn't find any \".gitattribute\" entries effectively\n> ignoring any configured attributes.\n>\n> Fix this by explicitly reading the index during the setup of diff-tree.\n\nThanks, both. Will queue.\n\n\n\n\n> Reported-by: Ben Boeckel \n> Signed-off-by: Brandon Williams \n> ---\n>\n> This patch should fix the regression. Let me know if it doesn't solve the\n> issue and I'll investigate some more.\n>\n> builtin/diff-tree.c | 1 +\n> t/t4015-diff-whitespace.sh | 17 +++++++++++++++++\n> 2 files changed, 18 insertions(+)\n>\n> diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c\n> index d66499909..cfe7d0281 100644\n> --- a/builtin/diff-tree.c\n> +++ b/builtin/diff-tree.c\n> @@ -110,6 +110,7 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)\n> \n> \tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n> \tinit_revisions(opt, prefix);\n> +\tread_cache();\n> \topt->abbrev = 0;\n> \topt->diff = 1;\n> \topt->disable_stdin = 1;\n> diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\n> index 559a7541a..6e061a002 100755\n> --- a/t/t4015-diff-whitespace.sh\n> +++ b/t/t4015-diff-whitespace.sh\n> @@ -636,6 +636,23 @@ test_expect_success 'check with space before tab in indent (diff-tree)' '\n> \ttest_must_fail git diff-tree --check HEAD^ HEAD\n> '\n> \n> +test_expect_success 'check with ignored trailing whitespace attr (diff-tree)' '\n> +\ttest_when_finished \"git reset --hard HEAD^\" &&\n> +\n> +\t# Create a whitespace error that should be ignored.\n> +\techo \"* -whitespace\" > \".gitattributes\" &&\n> +\tgit add \".gitattributes\" &&\n> +\techo \"trailing space -> \" > \"trailing-space\" &&\n> +\tgit add \"trailing-space\" &&\n> +\tgit commit -m \"trailing space\" &&\n> +\n> +\t# With a worktree diff-tree ignores the whitespace error\n> +\tgit diff-tree --root --check HEAD &&\n> +\n> +\t# Without a worktree diff-tree still ignores the whitespace error\n> +\tgit -C .git diff-tree --root --check HEAD\n> +'\n> +\n> test_expect_success 'check trailing whitespace (trailing-space: off)' '\n> \tgit config core.whitespace \"-trailing-space\" &&\n> \techo \"foo (); \" >x &&\n"},{"id":"334232","messageId":"CAGZ79kbvkopatFZi64Hxoa=wX6CJxJw6V+9RnQqrx6gTBL-78w@mail.gmail.com","threadId":"47375","inReplyTo":"20171205221337.140548-1-bmwill@google.com","subject":"Re: [PATCH] diff-tree: read the index so attribute checks work in bare repositories","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-12-05T23:14:00Z","receivedAt":"2017-12-05T23:14:06Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Dec 5, 2017 at 2:13 PM, Brandon Williams wrote:\n> A regression was introduced in 557a5998d (submodule: remove\n> gitmodules_config, 2017-08-03) to how attribute processing was handled\n> in bare repositories when running the diff-tree command.\n>\n> By default the attribute system will first try to read \".gitattribute\"\n> files from the working tree and then falls back to reading them from the\n> index if there isn't a copy checked out in the worktree. Prior to\n> 557a5998d the index was read as a side effect of the call to\n> 'gitmodules_config()' which ensured that the index was already populated\n> before entering the attribute subsystem.\n>\n> Since the call to 'gitmodules_config()' was removed the index is no\n> longer being read so when the attribute system tries to read from the\n> in-memory index it doesn't find any \".gitattribute\" entries effectively\n> ignoring any configured attributes.\n>\n> Fix this by explicitly reading the index during the setup of diff-tree.\n>\n> Reported-by: Ben Boeckel \n> Signed-off-by: Brandon Williams \n> ---\n>\n> This patch should fix the regression. Let me know if it doesn't solve the\n> issue and I'll investigate some more.\n>\n\nThanks for fixing this bug! The commit message is helpful\nto understand how this bug could slip in!\n\n> diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c\n> index d66499909..cfe7d0281 100644\n> --- a/builtin/diff-tree.c\n> +++ b/builtin/diff-tree.c\n> @@ -110,6 +110,7 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)\n>\n> git_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n> init_revisions(opt, prefix);\n> + read_cache();\n\n\nAlthough we do have very few unchecked calls to read_cache, I'd suggest\nto avoid spreading them. Most of the read_cache calls are guarded via:\n\n if (read_cache() < 0)\n die(_(\"index file corrupt\"));\n\nI wonder if this hints at a bad API, and we'd rather have read_cache\ndie() on errors, and the few callers that try to get out of trouble might\nneed to use read_cache_gently() instead.\n(While this potentially large refactoring may be deferred, I'd ask for\nan if at least)\n\nThanks,\nStefan\n"},{"id":"334233","messageId":"CAPig+cS6zf3AvQVi7PZf=ignL-7JZKzYUyN9RoJSPZzL=Dj7FA@mail.gmail.com","threadId":"47375","inReplyTo":"20171205221337.140548-1-bmwill@google.com","subject":"Re: [PATCH] diff-tree: read the index so attribute checks work in bare repositories","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2017-12-05T23:25:12Z","receivedAt":"2017-12-05T23:25:19Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Dec 5, 2017 at 5:13 PM, Brandon Williams wrote:\n> A regression was introduced in 557a5998d (submodule: remove\n> gitmodules_config, 2017-08-03) to how attribute processing was handled\n> in bare repositories when running the diff-tree command.\n>\n> By default the attribute system will first try to read \".gitattribute\"\n> files from the working tree and then falls back to reading them from the\n> index if there isn't a copy checked out in the worktree. Prior to\n> 557a5998d the index was read as a side effect of the call to\n> 'gitmodules_config()' which ensured that the index was already populated\n> before entering the attribute subsystem.\n>\n> Since the call to 'gitmodules_config()' was removed the index is no\n> longer being read so when the attribute system tries to read from the\n> in-memory index it doesn't find any \".gitattribute\" entries effectively\n> ignoring any configured attributes.\n>\n> Fix this by explicitly reading the index during the setup of diff-tree.\n\nThis commit message does a good job of explaining the issue, so\nsomeone who hasn't followed the thread (or has not followed it\nclosely, like me) can understand the problem and solution. Thanks.\n\nA few comments below...\n\n> Reported-by: Ben Boeckel \n> Signed-off-by: Brandon Williams \n> ---\n> diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c\n> @@ -110,6 +110,7 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)\n>\n> git_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n> init_revisions(opt, prefix);\n> + read_cache();\n\n557a5998d (submodule: remove gitmodules_config, 2017-08-03) touched a\nfair number of built-in commands. It's not clear from the current\npatch's commit message if diff-tree is the only command which\nregressed. Is it? Or are other commands also likely to have regressed?\nPerhaps the commit message could say something about this. For\ninstance: \"All other commands touched by 557a5998d have been audited\nand were found to be regression-free\" or \"Other commands may regress\nin the same way, but we will take a wait-and-see attitude and fix them\nas needed because \".\n\n> diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\n> @@ -636,6 +636,23 @@ test_expect_success 'check with space before tab in indent (diff-tree)' '\n> +test_expect_success 'check with ignored trailing whitespace attr (diff-tree)' '\n> + test_when_finished \"git reset --hard HEAD^\" &&\n\nA few style nits...\n\n> + # Create a whitespace error that should be ignored.\n\nComments in nearby tests are not capitalized and do not end with period.\n\n> + echo \"* -whitespace\" > \".gitattributes\" &&\n\nPlease drop unnecessary quotes around the filename, as the extra noise\nmakes it a bit harder to read. Also, lose space after redirection\noperator:\n\n echo \"* -whitespace\" >.gitattributes &&\n\n> + git add \".gitattributes\" &&\n> + echo \"trailing space -> \" > \"trailing-space\" &&\n\nAll the nearby tests use some variation of:\n\n echo \"foo (); \" >x &&\n\nwhich differs from the \"trailing space ->\" and filename\n'trailing-space' used in this test. Lack of consistency makes this new\ntest stick out like a sore thumb.\n\n> + git add \"trailing-space\" &&\n> + git commit -m \"trailing space\" &&\n> +\n> + # With a worktree diff-tree ignores the whitespace error\n> + git diff-tree --root --check HEAD &&\n> +\n> + # Without a worktree diff-tree still ignores the whitespace error\n> + git -C .git diff-tree --root --check HEAD\n> +'\n"},{"id":"334276","messageId":"20171206214722.GA118027@google.com","threadId":"47375","inReplyTo":"CAGZ79kbvkopatFZi64Hxoa=wX6CJxJw6V+9RnQqrx6gTBL-78w@mail.gmail.com","subject":"Re: [PATCH] diff-tree: read the index so attribute checks work in bare repositories","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-12-06T21:47:22Z","receivedAt":"2017-12-06T21:47:51Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 12/05, Stefan Beller wrote:\n> On Tue, Dec 5, 2017 at 2:13 PM, Brandon Williams wrote:\n> > A regression was introduced in 557a5998d (submodule: remove\n> > gitmodules_config, 2017-08-03) to how attribute processing was handled\n> > in bare repositories when running the diff-tree command.\n> >\n> > By default the attribute system will first try to read \".gitattribute\"\n> > files from the working tree and then falls back to reading them from the\n> > index if there isn't a copy checked out in the worktree. Prior to\n> > 557a5998d the index was read as a side effect of the call to\n> > 'gitmodules_config()' which ensured that the index was already populated\n> > before entering the attribute subsystem.\n> >\n> > Since the call to 'gitmodules_config()' was removed the index is no\n> > longer being read so when the attribute system tries to read from the\n> > in-memory index it doesn't find any \".gitattribute\" entries effectively\n> > ignoring any configured attributes.\n> >\n> > Fix this by explicitly reading the index during the setup of diff-tree.\n> >\n> > Reported-by: Ben Boeckel \n> > Signed-off-by: Brandon Williams \n> > ---\n> >\n> > This patch should fix the regression. Let me know if it doesn't solve the\n> > issue and I'll investigate some more.\n> >\n> \n> Thanks for fixing this bug! The commit message is helpful\n> to understand how this bug could slip in!\n> \n> > diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c\n> > index d66499909..cfe7d0281 100644\n> > --- a/builtin/diff-tree.c\n> > +++ b/builtin/diff-tree.c\n> > @@ -110,6 +110,7 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)\n> >\n> > git_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n> > init_revisions(opt, prefix);\n> > + read_cache();\n> \n> \n> Although we do have very few unchecked calls to read_cache, I'd suggest\n> to avoid spreading them. Most of the read_cache calls are guarded via:\n> \n> if (read_cache() < 0)\n> die(_(\"index file corrupt\"));\n\nThanks, I'll add this change.\n\n> \n> I wonder if this hints at a bad API, and we'd rather have read_cache\n> die() on errors, and the few callers that try to get out of trouble might\n> need to use read_cache_gently() instead.\n> (While this potentially large refactoring may be deferred, I'd ask for\n> an if at least)\n> \n> Thanks,\n> Stefan\n\n-- \nBrandon Williams\n"},{"id":"334278","messageId":"20171206220055.GB118027@google.com","threadId":"47375","inReplyTo":"CAPig+cS6zf3AvQVi7PZf=ignL-7JZKzYUyN9RoJSPZzL=Dj7FA@mail.gmail.com","subject":"Re: [PATCH] diff-tree: read the index so attribute checks work in bare repositories","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-12-06T22:00:55Z","receivedAt":"2017-12-06T22:01:05Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 12/05, Eric Sunshine wrote:\n> On Tue, Dec 5, 2017 at 5:13 PM, Brandon Williams wrote:\n> > A regression was introduced in 557a5998d (submodule: remove\n> > gitmodules_config, 2017-08-03) to how attribute processing was handled\n> > in bare repositories when running the diff-tree command.\n> >\n> > By default the attribute system will first try to read \".gitattribute\"\n> > files from the working tree and then falls back to reading them from the\n> > index if there isn't a copy checked out in the worktree. Prior to\n> > 557a5998d the index was read as a side effect of the call to\n> > 'gitmodules_config()' which ensured that the index was already populated\n> > before entering the attribute subsystem.\n> >\n> > Since the call to 'gitmodules_config()' was removed the index is no\n> > longer being read so when the attribute system tries to read from the\n> > in-memory index it doesn't find any \".gitattribute\" entries effectively\n> > ignoring any configured attributes.\n> >\n> > Fix this by explicitly reading the index during the setup of diff-tree.\n> \n> This commit message does a good job of explaining the issue, so\n> someone who hasn't followed the thread (or has not followed it\n> closely, like me) can understand the problem and solution. Thanks.\n> \n> A few comments below...\n> \n> > Reported-by: Ben Boeckel \n> > Signed-off-by: Brandon Williams \n> > ---\n> > diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c\n> > @@ -110,6 +110,7 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)\n> >\n> > git_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n> > init_revisions(opt, prefix);\n> > + read_cache();\n> \n> 557a5998d (submodule: remove gitmodules_config, 2017-08-03) touched a\n> fair number of built-in commands. It's not clear from the current\n> patch's commit message if diff-tree is the only command which\n> regressed. Is it? Or are other commands also likely to have regressed?\n> Perhaps the commit message could say something about this. For\n> instance: \"All other commands touched by 557a5998d have been audited\n> and were found to be regression-free\" or \"Other commands may regress\n> in the same way, but we will take a wait-and-see attitude and fix them\n> as needed because \".\n\nI don't know if any other commands have regressed. This was such an odd\nregression that I think it would be difficult to say for certain that\nthere couldn't be others. I did go through the affected builtins to see\nif I could find anything. I came up empty handed so I think we should\nbe ok.\n\n> \n> > diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\n> > @@ -636,6 +636,23 @@ test_expect_success 'check with space before tab in indent (diff-tree)' '\n> > +test_expect_success 'check with ignored trailing whitespace attr (diff-tree)' '\n> > + test_when_finished \"git reset --hard HEAD^\" &&\n> \n> A few style nits...\n> \n> > + # Create a whitespace error that should be ignored.\n> \n> Comments in nearby tests are not capitalized and do not end with period.\n> \n> > + echo \"* -whitespace\" > \".gitattributes\" &&\n> \n> Please drop unnecessary quotes around the filename, as the extra noise\n> makes it a bit harder to read. Also, lose space after redirection\n> operator:\n> \n> echo \"* -whitespace\" >.gitattributes &&\n> \n> > + git add \".gitattributes\" &&\n> > + echo \"trailing space -> \" > \"trailing-space\" &&\n> \n> All the nearby tests use some variation of:\n> \n> echo \"foo (); \" >x &&\n> \n> which differs from the \"trailing space ->\" and filename\n> 'trailing-space' used in this test. Lack of consistency makes this new\n> test stick out like a sore thumb.\n\nYou're right, when writing the tests I didn't really consider the\nsurrounding ones. I'll make the requested changes.\n\n> \n> > + git add \"trailing-space\" &&\n> > + git commit -m \"trailing space\" &&\n> > +\n> > + # With a worktree diff-tree ignores the whitespace error\n> > + git diff-tree --root --check HEAD &&\n> > +\n> > + # Without a worktree diff-tree still ignores the whitespace error\n> > + git -C .git diff-tree --root --check HEAD\n> > +'\n\n-- \nBrandon Williams\n"},{"id":"334279","messageId":"20171206220256.44482-1-bmwill@google.com","threadId":"47375","inReplyTo":"20171205221337.140548-1-bmwill@google.com","subject":"[PATCH v2] diff-tree: read the index so attribute checks work in bare repositories","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-12-06T22:02:56Z","receivedAt":"2017-12-06T22:03:15Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"A regression was introduced in 557a5998d (submodule: remove\ngitmodules_config, 2017-08-03) to how attribute processing was handled\nin bare repositories when running the diff-tree command.\n\nBy default the attribute system will first try to read \".gitattribute\"\nfiles from the working tree and then falls back to reading them from the\nindex if there isn't a copy checked out in the worktree. Prior to\n557a5998d the index was read as a side effect of the call to\n'gitmodules_config()' which ensured that the index was already populated\nbefore entering the attribute subsystem.\n\nSince the call to 'gitmodules_config()' was removed the index is no\nlonger being read so when the attribute system tries to read from the\nin-memory index it doesn't find any \".gitattribute\" entries effectively\nignoring any configured attributes.\n\nFix this by explicitly reading the index during the setup of diff-tree.\n\nReported-by: Ben Boeckel \nSigned-off-by: Brandon Williams \n---\n builtin/diff-tree.c | 2 ++\n t/t4015-diff-whitespace.sh | 17 +++++++++++++++++\n 2 files changed, 19 insertions(+)\n\ndiff --git a/builtin/diff-tree.c b/builtin/diff-tree.c\nindex d66499909..b775a7564 100644\n--- a/builtin/diff-tree.c\n+++ b/builtin/diff-tree.c\n@@ -110,6 +110,8 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)\n \n \tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n \tinit_revisions(opt, prefix);\n+\tif (read_cache() < 0)\n+\t\tdie(_(\"index file corrupt\"));\n \topt->abbrev = 0;\n \topt->diff = 1;\n \topt->disable_stdin = 1;\ndiff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nindex 559a7541a..17df491a3 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -636,6 +636,23 @@ test_expect_success 'check with space before tab in indent (diff-tree)' '\n \ttest_must_fail git diff-tree --check HEAD^ HEAD\n '\n \n+test_expect_success 'check with ignored trailing whitespace attr (diff-tree)' '\n+\ttest_when_finished \"git reset --hard HEAD^\" &&\n+\n+\t# create a whitespace error that should be ignored\n+\techo \"* -whitespace\" >.gitattributes &&\n+\tgit add .gitattributes &&\n+\techo \"foo(); \" >x &&\n+\tgit add x &&\n+\tgit commit -m \"add trailing space\" &&\n+\n+\t# with a worktree diff-tree ignores the whitespace error\n+\tgit diff-tree --root --check HEAD &&\n+\n+\t# without a worktree diff-tree still ignores the whitespace error\n+\tgit -C .git diff-tree --root --check HEAD\n+'\n+\n test_expect_success 'check trailing whitespace (trailing-space: off)' '\n \tgit config core.whitespace \"-trailing-space\" &&\n \techo \"foo (); \" >x &&\n-- \n2.15.1.424.g9478a66081-goog\n\n"},{"id":"334281","messageId":"CAPig+cSNwPJkVNNbnp-rh-jXgwrEXhvzVHZoQM6wQMQNjn5n5Q@mail.gmail.com","threadId":"47375","inReplyTo":"20171206220055.GB118027@google.com","subject":"Re: [PATCH] diff-tree: read the index so attribute checks work in bare repositories","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2017-12-06T22:07:21Z","receivedAt":"2017-12-06T22:07:27Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Dec 6, 2017 at 5:00 PM, Brandon Williams wrote:\n> On 12/05, Eric Sunshine wrote:\n>> 557a5998d (submodule: remove gitmodules_config, 2017-08-03) touched a\n>> fair number of built-in commands. It's not clear from the current\n>> patch's commit message if diff-tree is the only command which\n>> regressed. Is it? Or are other commands also likely to have regressed?\n>> Perhaps the commit message could say something about this. For\n>> instance: \"All other commands touched by 557a5998d have been audited\n>> and were found to be regression-free\" or \"Other commands may regress\n>> in the same way, but we will take a wait-and-see attitude and fix them\n>> as needed because \".\n>\n> I don't know if any other commands have regressed. This was such an odd\n> regression that I think it would be difficult to say for certain that\n> there couldn't be others. I did go through the affected builtins to see\n> if I could find anything. I came up empty handed so I think we should\n> be ok.\n\nThanks for the response. It would be nice to have this explained in\nthe commit message so that future readers don't have to wonder about\nit.\n"}]}