gitattributes not read for diff-tree anymore in 2.15?

gitattributes not read for diff-tree anymore in 2.15?

[Ben Boeckel]

Hi,

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

    commit 557a5998df19faf8641acfc5b6b1c3c2ba64dca9 (HEAD, refs/bisect/bad)
    Author: Brandon Williams <bmwill@google.com>
    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 <bmwill@google.com>
        Signed-off-by: Junio C Hamano <gitster@pobox.com>

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

Re: gitattributes not read for diff-tree anymore in 2.15?

[Brandon Williams]

On 12/04, Ben Boeckel wrote:
> Hi,
> 
> I've bisected a failure in our test suite to this commit:
> 
>     commit 557a5998df19faf8641acfc5b6b1c3c2ba64dca9 (HEAD, refs/bisect/bad)
>     Author: Brandon Williams <bmwill@google.com>
>     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 <bmwill@google.com>
>         Signed-off-by: Junio C Hamano <gitster@pobox.com>
> 
> 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.

-- 
Brandon Williams

Re: gitattributes not read for diff-tree anymore in 2.15?

[Ben Boeckel]

On Mon, Dec 04, 2017 at 15:03:55 -0800, Brandon Williams wrote:
> 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*.

> 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

Re: gitattributes not read for diff-tree anymore in 2.15?

[Brandon Williams]

On 12/05, Ben Boeckel wrote:
> On Mon, Dec 04, 2017 at 15:03:55 -0800, Brandon Williams wrote:
> > 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*.

While it doesn't make the most sense, I still wouldn't be surprised if I
missed something when writing that patch that inadvertently caused an
issue.

> 
> > 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!

-- 
Brandon Williams

Re: gitattributes not read for diff-tree anymore in 2.15?

[Ben Boeckel]

[-- Attachment #1: Type: text/plain, Size: 595 bytes --]

On Tue, Dec 05, 2017 at 10:16:45 -0800, Brandon Williams wrote:
> Perfect, thanks!

OK, attached is a shell script which recreates the issue. I haven't been
able to get it to happen without the `GIT_WORK_TREE` and `GIT_INDEX_FILE`
setup involved, so that seems to be important.

I reran the bisect using this script and came up with this commit:

    commit be4ca290570f9173db64ea1f925b5b3831c6efed
    Author: David Turner <dturner@twosigma.com>
    Date:   Thu Apr 20 16:41:18 2017 -0400

        Increase core.packedGitLimit

        <snip>

which seems even less relevant…

Thanks,

--Ben

[-- Attachment #2: diff-tree-break.sh --]
[-- Type: application/x-sh, Size: 933 bytes --]

[PATCH] diff-tree: read the index so attribute checks work in bare repositories

[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.

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

Reported-by: Ben Boeckel <ben.boeckel@kitware.com>
Signed-off-by: Brandon Williams <bmwill@google.com>
---

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(+)

diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
index d66499909..cfe7d0281 100644
--- a/builtin/diff-tree.c
+++ b/builtin/diff-tree.c
@@ -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;
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 559a7541a..6e061a002 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -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 &&
-- 
2.15.1.424.g9478a66081-goog

Re: [PATCH] diff-tree: read the index so attribute checks work in bare repositories

[Ben Boeckel]

On Tue, Dec 05, 2017 at 14:13:37 -0800, Brandon Williams wrote:
> 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 <ben.boeckel@kitware.com>

--Ben

Re: [PATCH] diff-tree: read the index so attribute checks work in bare repositories

[Brandon Williams]

On 12/05, Ben Boeckel wrote:
> On Tue, Dec 05, 2017 at 14:13:37 -0800, Brandon Williams wrote:
> > 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!

Of course! Glad I could help :)

> 
>     Acked-by: Ben Boeckel <ben.boeckel@kitware.com>
> 
> --Ben

-- 
Brandon Williams

Re: [PATCH] diff-tree: read the index so attribute checks work in bare repositories

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

> 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.

Thanks, both.  Will queue.




> Reported-by: Ben Boeckel <ben.boeckel@kitware.com>
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
>
> 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(+)
>
> diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
> index d66499909..cfe7d0281 100644
> --- a/builtin/diff-tree.c
> +++ b/builtin/diff-tree.c
> @@ -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;
> diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
> index 559a7541a..6e061a002 100755
> --- a/t/t4015-diff-whitespace.sh
> +++ b/t/t4015-diff-whitespace.sh
> @@ -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 &&

Re: [PATCH] diff-tree: read the index so attribute checks work in bare repositories

[Stefan Beller]

On Tue, Dec 5, 2017 at 2:13 PM, Brandon Williams <bmwill@google.com> wrote:
> 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 <ben.boeckel@kitware.com>
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
>
> 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!

> diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
> index d66499909..cfe7d0281 100644
> --- a/builtin/diff-tree.c
> +++ b/builtin/diff-tree.c
> @@ -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

Re: [PATCH] diff-tree: read the index so attribute checks work in bare repositories

[Eric Sunshine]

On Tue, Dec 5, 2017 at 5:13 PM, Brandon Williams <bmwill@google.com> wrote:
> 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.

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...

> Reported-by: Ben Boeckel <ben.boeckel@kitware.com>
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
> diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
> @@ -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();

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>".

> diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
> @@ -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...

> +       # Create a whitespace error that should be ignored.

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

> +       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 &&

> +       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.

> +       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
> +'

Re: [PATCH] diff-tree: read the index so attribute checks work in bare repositories

[Brandon Williams]

On 12/05, Stefan Beller wrote:
> On Tue, Dec 5, 2017 at 2:13 PM, Brandon Williams <bmwill@google.com> wrote:
> > 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 <ben.boeckel@kitware.com>
> > Signed-off-by: Brandon Williams <bmwill@google.com>
> > ---
> >
> > 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!
> 
> > diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
> > index d66499909..cfe7d0281 100644
> > --- a/builtin/diff-tree.c
> > +++ b/builtin/diff-tree.c
> > @@ -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"));

Thanks, I'll add this change.

> 
> 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

-- 
Brandon Williams

Re: [PATCH] diff-tree: read the index so attribute checks work in bare repositories

[Brandon Williams]

On 12/05, Eric Sunshine wrote:
> On Tue, Dec 5, 2017 at 5:13 PM, Brandon Williams <bmwill@google.com> wrote:
> > 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.
> 
> 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...
> 
> > Reported-by: Ben Boeckel <ben.boeckel@kitware.com>
> > Signed-off-by: Brandon Williams <bmwill@google.com>
> > ---
> > diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
> > @@ -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();
> 
> 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.

> 
> > diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
> > @@ -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...
> 
> > +       # Create a whitespace error that should be ignored.
> 
> Comments in nearby tests are not capitalized and do not end with period.
> 
> > +       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 &&
> 
> > +       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.

You're right, when writing the tests I didn't really consider the
surrounding ones.  I'll make the requested changes.

> 
> > +       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
> > +'

-- 
Brandon Williams

[PATCH v2] diff-tree: read the index so attribute checks work in bare repositories

[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.

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

Reported-by: Ben Boeckel <ben.boeckel@kitware.com>
Signed-off-by: Brandon Williams <bmwill@google.com>
---
 builtin/diff-tree.c        |  2 ++
 t/t4015-diff-whitespace.sh | 17 +++++++++++++++++
 2 files changed, 19 insertions(+)

diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
index d66499909..b775a7564 100644
--- a/builtin/diff-tree.c
+++ b/builtin/diff-tree.c
@@ -110,6 +110,8 @@ 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);
+	if (read_cache() < 0)
+		die(_("index file corrupt"));
 	opt->abbrev = 0;
 	opt->diff = 1;
 	opt->disable_stdin = 1;
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 559a7541a..17df491a3 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -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 "foo(); " >x &&
+	git add x &&
+	git commit -m "add 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 &&
-- 
2.15.1.424.g9478a66081-goog

Re: [PATCH] diff-tree: read the index so attribute checks work in bare repositories

[Eric Sunshine]

On Wed, Dec 6, 2017 at 5:00 PM, Brandon Williams <bmwill@google.com> wrote:
> On 12/05, Eric Sunshine wrote:
>> 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.

Thanks for the response. It would be nice to have this explained in
the commit message so that future readers don't have to wonder about
it.