gitattributes not read for diff-tree anymore in 2.15?

Hi,

I've bisected a failure in our test suite to this commit:

commit 557a5998df19faf8641acfc5b6b1c3c2ba64dca9 (HEAD, refs/bisect/bad)
Author: Brandon Williams <[email protected]>
Date: Thu Aug 3 11:20:00 2017 -0700

submodule: remove gitmodules_config

Now that the submodule-config subsystem can lazily read the gitmodules file we no longer need to explicitly pre-read the gitmodules by calling 'gitmodules_config()' so let's remove it.

Signed-off-by Brandon Williams <[email protected]>

Signed-off-by Junio C Hamano <[email protected]>

Which is tags/v2.15.0-rc0~120^2.

Our test suite is in a Rust project here:

https://gitlab.kitware.com/utils/rust-git-checks

and the failing test(s) can be run using:

cargo test whitespace_all_ignored

The test checks that when .gitattributes says that whitespace errors should be ignored, they aren't reported as errors. My guess is that not reading the gitmodules configuration also skips reading attributes files. Is this reasoning correct?

Thanks,

--Ben

Reading the attributes files should be done regardless if the gitmodules file is read. The gitmodules file should only come into play if you are dealing with submodules.

Do you mind providing a reproduction recipe with expected outcome vs actual outcome and I can take a closer look.

Quoted 3 lines · Brandon Williams

Reading the attributes files should be done regardless if the gitmodules file is read. The gitmodules file should only come into play if you are dealing with submodules.

Yeah, it doesn't seem to make sense why this commit breaks us, but there it is *shrug*.

Quoted 3 lines

Do you mind providing a reproduction recipe with expected outcome vs actual outcome and I can take a closer look.

I'll work on one. It isn't as simple as I thought it was :) . The setup we do before running the checks is apparently involved as running it from the command line is not exhibiting the difference.

--Ben

Perfect, thanks!

This patch should fix the regression. Let me know if it doesn't solve the issue and I'll investigate some more.

Quoted 2 lines · Brandon Williams

This patch should fix the regression. Let me know if it doesn't solve the issue and I'll investigate some more.

Our test suite passes again. Thanks!

Acked-by Ben Boeckel <[email protected]>

--Ben

A regression was introduced in 557a5998d (submodule: remove gitmodules_config, 2017-08-03) to how attribute processing was handled in bare repositories when running the diff-tree command.

By default the attribute system will first try to read ".gitattribute" files from the working tree and then falls back to reading them from the index if there isn't a copy checked out in the worktree. Prior to 557a5998d the index was read as a side effect of the call to 'gitmodules_config()' which ensured that the index was already populated before entering the attribute subsystem.

Since the call to 'gitmodules_config()' was removed the index is no longer being read so when the attribute system tries to read from the in-memory index it doesn't find any ".gitattribute" entries effectively ignoring any configured attributes.

Fix this by explicitly reading the index during the setup of diff-tree.

Reported-by Ben Boeckel <[email protected]>

Signed-off-by Brandon Williams <[email protected]>

---

This patch should fix the regression. Let me know if it doesn't solve the issue and I'll investigate some more.

 builtin/diff-tree.c        |  1 +
 t/t4015-diff-whitespace.sh | 17 +++++++++++++++++
 2 files changed, 18 insertions(+)

builtin/diff-tree.c +1 -0

index d66499909..cfe7d0281 100644
@@ -110,6 +110,7 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
 
 	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
 	init_revisions(opt, prefix);
+	read_cache();
 	opt->abbrev = 0;
 	opt->diff = 1;
 	opt->disable_stdin = 1;

t/t4015-diff-whitespace.sh +17 -0

index 559a7541a..6e061a002 100755
@@ -636,6 +636,23 @@ test_expect_success 'check with space before tab in indent (diff-tree)' '
 	test_must_fail git diff-tree --check HEAD^ HEAD
 '
 
+test_expect_success 'check with ignored trailing whitespace attr (diff-tree)' '
+	test_when_finished "git reset --hard HEAD^" &&
+
+	# Create a whitespace error that should be ignored.
+	echo "* -whitespace" > ".gitattributes" &&
+	git add ".gitattributes" &&
+	echo "trailing space -> " > "trailing-space" &&
+	git add "trailing-space" &&
+	git commit -m "trailing space" &&
+
+	# With a worktree diff-tree ignores the whitespace error
+	git diff-tree --root --check HEAD &&
+
+	# Without a worktree diff-tree still ignores the whitespace error
+	git -C .git diff-tree --root --check HEAD
+'
+
 test_expect_success 'check trailing whitespace (trailing-space: off)' '
 	git config core.whitespace "-trailing-space" &&
 	echo "foo ();   " >x &&

A regression was introduced in 557a5998d (submodule: remove gitmodules_config, 2017-08-03) to how attribute processing was handled in bare repositories when running the diff-tree command.

By default the attribute system will first try to read ".gitattribute" files from the working tree and then falls back to reading them from the index if there isn't a copy checked out in the worktree. Prior to 557a5998d the index was read as a side effect of the call to 'gitmodules_config()' which ensured that the index was already populated before entering the attribute subsystem.

Since the call to 'gitmodules_config()' was removed the index is no longer being read so when the attribute system tries to read from the in-memory index it doesn't find any ".gitattribute" entries effectively ignoring any configured attributes.

Fix this by explicitly reading the index during the setup of diff-tree.

Reported-by Ben Boeckel <[email protected]>

Signed-off-by Brandon Williams <[email protected]>

---

This patch should fix the regression. Let me know if it doesn't solve the issue and I'll investigate some more.

Quoted 16 lines · Brandon Williams

A regression was introduced in 557a5998d (submodule: remove gitmodules_config, 2017-08-03) to how attribute processing was handled in bare repositories when running the diff-tree command.

By default the attribute system will first try to read ".gitattribute" files from the working tree and then falls back to reading them from the index if there isn't a copy checked out in the worktree. Prior to 557a5998d the index was read as a side effect of the call to 'gitmodules_config()' which ensured that the index was already populated before entering the attribute subsystem.

Since the call to 'gitmodules_config()' was removed the index is no longer being read so when the attribute system tries to read from the in-memory index it doesn't find any ".gitattribute" entries effectively ignoring any configured attributes.

Quoted 9 lines

Fix this by explicitly reading the index during the setup of diff-tree.

Reported-by Ben Boeckel <[email protected]>

Signed-off-by Brandon Williams <[email protected]>

---

This patch should fix the regression. Let me know if it doesn't solve the issue and I'll investigate some more.

Thanks for fixing this bug! The commit message is helpful to understand how this bug could slip in!

builtin/diff-tree.c int cmd_diff_tree(int argc, const char **argv, const char *prefix) · line 113

Quoted 9 lines

builtin/diff-tree.c +1 -0

index d66499909..cfe7d0281 100644
@@ -110,6 +110,7 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
 
        git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
        init_revisions(opt, prefix);
+       read_cache();

Although we do have very few unchecked calls to read_cache, I'd suggest to avoid spreading them. Most of the read_cache calls are guarded via:

    if (read_cache() < 0)
        die(_("index file corrupt"));

I wonder if this hints at a bad API, and we'd rather have read_cache die() on errors, and the few callers that try to get out of trouble might need to use read_cache_gently() instead.
(While this potentially large refactoring may be deferred, I'd ask for an if at least)

Thanks,
Stefan

A regression was introduced in 557a5998d (submodule: remove gitmodules_config, 2017-08-03) to how attribute processing was handled in bare repositories when running the diff-tree command.

By default the attribute system will first try to read ".gitattribute" files from the working tree and then falls back to reading them from the index if there isn't a copy checked out in the worktree. Prior to 557a5998d the index was read as a side effect of the call to 'gitmodules_config()' which ensured that the index was already populated before entering the attribute subsystem.

Since the call to 'gitmodules_config()' was removed the index is no longer being read so when the attribute system tries to read from the in-memory index it doesn't find any ".gitattribute" entries effectively ignoring any configured attributes.

Fix this by explicitly reading the index during the setup of diff-tree.

t/t4015-diff-whitespace.sh +9 -0

+       git add "trailing-space" &&
+       git commit -m "trailing space" &&
+
+       # With a worktree diff-tree ignores the whitespace error
+       git diff-tree --root --check HEAD &&
+
+       # Without a worktree diff-tree still ignores the whitespace error
+       git -C .git diff-tree --root --check HEAD
+'

Quoted 11 lines · Brandon Williams

A regression was introduced in 557a5998d (submodule: remove gitmodules_config, 2017-08-03) to how attribute processing was handled in bare repositories when running the diff-tree command.

By default the attribute system will first try to read ".gitattribute" files from the working tree and then falls back to reading them from the index if there isn't a copy checked out in the worktree. Prior to 557a5998d the index was read as a side effect of the call to 'gitmodules_config()' which ensured that the index was already populated before entering the attribute subsystem.

Quoted 6 lines

Since the call to 'gitmodules_config()' was removed the index is no longer being read so when the attribute system tries to read from the in-memory index it doesn't find any ".gitattribute" entries effectively ignoring any configured attributes.

Fix this by explicitly reading the index during the setup of diff-tree.

This commit message does a good job of explaining the issue, so someone who hasn't followed the thread (or has not followed it closely, like me) can understand the problem and solution. Thanks.

A few comments below...

Quoted 6 lines

Reported-by Ben Boeckel <[email protected]>

Signed-off-by Brandon Williams <[email protected]>

---

builtin/diff-tree.c +0 -0

@@ -110,6 +110,7 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
 

builtin/diff-tree.c int cmd_diff_tree(int argc, const char **argv, const char *prefix) · line 113

Quoted 3 lines

builtin/diff-tree.c +1 -0

        git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
        init_revisions(opt, prefix);
+       read_cache();

557a5998d (submodule: remove gitmodules_config, 2017-08-03) touched a fair number of built-in commands. It's not clear from the current patch's commit message if diff-tree is the only command which regressed. Is it? Or are other commands also likely to have regressed? Perhaps the commit message could say something about this. For
instance: "All other commands touched by 557a5998d have been audited and were found to be regression-free" or "Other commands may regress in the same way, but we will take a wait-and-see attitude and fix them as needed because <fill-in-reason>".

t/t4015-diff-whitespace.sh test_expect_success 'check with space before tab in indent (diff-tree)' ' · line 637

Quoted 5 lines

t/t4015-diff-whitespace.sh +2 -0

@@ -636,6 +636,23 @@ test_expect_success 'check with space before tab in indent (diff-tree)' '
+test_expect_success 'check with ignored trailing whitespace attr (diff-tree)' '
+       test_when_finished "git reset --hard HEAD^" &&

A few style nits...

t/t4015-diff-whitespace.sh test_expect_success 'check with space before tab in indent (diff-tree)' ' · line 642

Quoted 1 line

t/t4015-diff-whitespace.sh +1 -0

+       # Create a whitespace error that should be ignored.

Comments in nearby tests are not capitalized and do not end with period.

t/t4015-diff-whitespace.sh test_expect_success 'check with space before tab in indent (diff-tree)' ' · line 643

Quoted 1 line

t/t4015-diff-whitespace.sh +1 -0

+       echo "* -whitespace" > ".gitattributes" &&

Please drop unnecessary quotes around the filename, as the extra noise makes it a bit harder to read. Also, lose space after redirection operator:

    echo "* -whitespace" >.gitattributes &&

t/t4015-diff-whitespace.sh test_expect_success 'check with space before tab in indent (diff-tree)' ' · line 645

Quoted 2 lines

t/t4015-diff-whitespace.sh +2 -0

+       git add ".gitattributes" &&
+       echo "trailing space -> " > "trailing-space" &&

All the nearby tests use some variation of:

    echo "foo ();  " >x &&

which differs from the "trailing space ->" and filename 'trailing-space' used in this test. Lack of consistency makes this new test stick out like a sore thumb.

Quoted 10 lines

t/t4015-diff-whitespace.sh +9 -0

+       git add "trailing-space" &&
+       git commit -m "trailing space" &&
+
+       # With a worktree diff-tree ignores the whitespace error
+       git diff-tree --root --check HEAD &&
+
+       # Without a worktree diff-tree still ignores the whitespace error
+       git -C .git diff-tree --root --check HEAD
+'

Quoted 9 lines · Eric Sunshine

557a5998d (submodule: remove gitmodules_config, 2017-08-03) touched a fair number of built-in commands. It's not clear from the current patch's commit message if diff-tree is the only command which regressed. Is it? Or are other commands also likely to have regressed? Perhaps the commit message could say something about this. For
instance: "All other commands touched by 557a5998d have been audited and were found to be regression-free" or "Other commands may regress in the same way, but we will take a wait-and-see attitude and fix them as needed because <fill-in-reason>".

I don't know if any other commands have regressed. This was such an odd regression that I think it would be difficult to say for certain that there couldn't be others. I did go through the affected builtins to see if I could find anything. I came up empty handed so I think we should be ok.