* Re: [PATCH] diff-tree: read the index so attribute checks work in bare repositories
2017-12-05 22:13 ` [PATCH] diff-tree: read the index so attribute checks work in bare repositories Brandon Williams
@ 2017-12-05 22:29 ` Ben Boeckel
2017-12-05 22:31 ` Brandon Williams
2017-12-05 23:13 ` Junio C Hamano
` (3 subsequent siblings)
4 siblings, 1 reply; 15+ messages in thread
From: Ben Boeckel @ 2017-12-05 22:29 UTC (permalink / raw)
To: Brandon Williams; +Cc: git
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] diff-tree: read the index so attribute checks work in bare repositories
2017-12-05 22:29 ` Ben Boeckel
@ 2017-12-05 22:31 ` Brandon Williams
0 siblings, 0 replies; 15+ messages in thread
From: Brandon Williams @ 2017-12-05 22:31 UTC (permalink / raw)
To: Ben Boeckel; +Cc: git
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] diff-tree: read the index so attribute checks work in bare repositories
2017-12-05 22:13 ` [PATCH] diff-tree: read the index so attribute checks work in bare repositories Brandon Williams
2017-12-05 22:29 ` Ben Boeckel
@ 2017-12-05 23:13 ` Junio C Hamano
2017-12-05 23:14 ` Stefan Beller
` (2 subsequent siblings)
4 siblings, 0 replies; 15+ messages in thread
From: Junio C Hamano @ 2017-12-05 23:13 UTC (permalink / raw)
To: Brandon Williams; +Cc: git, ben.boeckel
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 &&
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] diff-tree: read the index so attribute checks work in bare repositories
2017-12-05 22:13 ` [PATCH] diff-tree: read the index so attribute checks work in bare repositories Brandon Williams
2017-12-05 22:29 ` Ben Boeckel
2017-12-05 23:13 ` Junio C Hamano
@ 2017-12-05 23:14 ` Stefan Beller
2017-12-06 21:47 ` Brandon Williams
2017-12-05 23:25 ` Eric Sunshine
2017-12-06 22:02 ` [PATCH v2] " Brandon Williams
4 siblings, 1 reply; 15+ messages in thread
From: Stefan Beller @ 2017-12-05 23:14 UTC (permalink / raw)
To: Brandon Williams; +Cc: git, ben.boeckel
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] diff-tree: read the index so attribute checks work in bare repositories
2017-12-05 23:14 ` Stefan Beller
@ 2017-12-06 21:47 ` Brandon Williams
0 siblings, 0 replies; 15+ messages in thread
From: Brandon Williams @ 2017-12-06 21:47 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, ben.boeckel
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] diff-tree: read the index so attribute checks work in bare repositories
2017-12-05 22:13 ` [PATCH] diff-tree: read the index so attribute checks work in bare repositories Brandon Williams
` (2 preceding siblings ...)
2017-12-05 23:14 ` Stefan Beller
@ 2017-12-05 23:25 ` Eric Sunshine
2017-12-06 22:00 ` Brandon Williams
2017-12-06 22:02 ` [PATCH v2] " Brandon Williams
4 siblings, 1 reply; 15+ messages in thread
From: Eric Sunshine @ 2017-12-05 23:25 UTC (permalink / raw)
To: Brandon Williams; +Cc: Git List, ben.boeckel
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
> +'
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] diff-tree: read the index so attribute checks work in bare repositories
2017-12-05 23:25 ` Eric Sunshine
@ 2017-12-06 22:00 ` Brandon Williams
2017-12-06 22:07 ` Eric Sunshine
0 siblings, 1 reply; 15+ messages in thread
From: Brandon Williams @ 2017-12-06 22:00 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Git List, ben.boeckel
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] diff-tree: read the index so attribute checks work in bare repositories
2017-12-06 22:00 ` Brandon Williams
@ 2017-12-06 22:07 ` Eric Sunshine
0 siblings, 0 replies; 15+ messages in thread
From: Eric Sunshine @ 2017-12-06 22:07 UTC (permalink / raw)
To: Brandon Williams; +Cc: Git List, Ben Boeckel
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.
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2] diff-tree: read the index so attribute checks work in bare repositories
2017-12-05 22:13 ` [PATCH] diff-tree: read the index so attribute checks work in bare repositories Brandon Williams
` (3 preceding siblings ...)
2017-12-05 23:25 ` Eric Sunshine
@ 2017-12-06 22:02 ` Brandon Williams
4 siblings, 0 replies; 15+ messages in thread
From: Brandon Williams @ 2017-12-06 22:02 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, ben.boeckel, sunshine, sbeller, gitster
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
^ permalink raw reply related [flat|nested] 15+ messages in thread