attr.c doesn't honor --work-tree option

attr.c doesn't honor --work-tree option

[Lasse Makholm]

Hi,

It seems that code in attr.c does not honor the current work tree path
(set by e.g. --work-tree ...) and simply always assumes CWD. When the
current dir is not in the work tree, git will attempt to find
.gitattributes under ./ instead of under the correct work tree.

Here's a repro with -DDEBUG_ATTR=1 and a printf() in read_attr_from_file():

$ cd /tmp/
$ mkdir -p attr-test/repo
$ cd attr-test/repo
$ git init
Initialized empty Git repository in /tmp/attr-test/repo/.git/
$ echo 'dir/* filter=foo' >.gitattributes
$

Inside the working tree, it works:

$ ~/src/git.git/git check-attr -a dir/file
read_attr_from_file: /home/lasse/etc/gitattributes
read_attr_from_file: /home/lasse/.config/git/attributes
read_attr_from_file: .gitattributes
push:
read_attr_from_file: .git/info/attributes
read_attr_from_file: dir/.gitattributes
push: dir
fill: filter => foo (dir/*)
dir/file: filter: foo
$

Outside, it fails to find the .gitattributes file:

$ cd ..
$ ~/src/git.git/git --work-tree /tmp/attr-test/repo --git-dir
/tmp/attr-test/repo/.git check-attr -a dir/file
read_attr_from_file: /home/lasse/etc/gitattributes
read_attr_from_file: /home/lasse/.config/git/attributes
read_attr_from_file: .gitattributes
push:
read_attr_from_file: /tmp/attr-test/repo/.git/info/attributes
read_attr_from_file: dir/.gitattributes
push: dir
$

This is with the latest rev on master:

$ ~/src/git.git/git --version
git version 1.8.5.2.192.g7794a68.dirty
$

It (sort of) works with a committed .gitattributes file because git
will find it in the index, but that will still yield incorrect results
if the .gitattributes file happens to be dirty.

Looking at the code, I'm not really sure if this can be fixed in
read_attr_from_file() by resolving relative paths against
get_git_work_tree(). I doubt it's that simple though...

Thoughts?

/Lasse

Re: attr.c doesn't honor --work-tree option

[Junio C Hamano]

Lasse Makholm <lasse.makholm@gmail.com> writes:

> Here's a repro with -DDEBUG_ATTR=1 and a printf() in read_attr_from_file():
>
> $ cd /tmp/
> $ mkdir -p attr-test/repo
> $ cd attr-test/repo
> $ git init
> Initialized empty Git repository in /tmp/attr-test/repo/.git/
> $ echo 'dir/* filter=foo' >.gitattributes
> $
>
> Inside the working tree, it works:
>
> $ ~/src/git.git/git check-attr -a dir/file

Does check-ignore misbehave the same way?

I suspect that is this because check-attr is not a command that
requires a working tree.  The command was written primarily as a
debugging aid that can be used anywhere as long as you have a
repository to read strings from either its standard input or its
arguments, and gives them directly to check_attr(), but it does so
without first going to the top of the real working tree like
check-ignore does.

Forcing it to go to the top of the working tree (see the attached
one-liner, but note that I didn't test it) may give you want you
want.

 git.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git.c b/git.c
index 7cf2953..314ec9f 100644
--- a/git.c
+++ b/git.c
@@ -342,7 +342,7 @@ static struct cmd_struct commands[] = {
 	{ "branch", cmd_branch, RUN_SETUP },
 	{ "bundle", cmd_bundle, RUN_SETUP_GENTLY },
 	{ "cat-file", cmd_cat_file, RUN_SETUP },
-	{ "check-attr", cmd_check_attr, RUN_SETUP },
+	{ "check-attr", cmd_check_attr, RUN_SETUP | NEED_WORK_TREE },
 	{ "check-ignore", cmd_check_ignore, RUN_SETUP | NEED_WORK_TREE },
 	{ "check-mailmap", cmd_check_mailmap, RUN_SETUP },
 	{ "check-ref-format", cmd_check_ref_format },

[PATCH 1/2] t0003: do not chdir the whole test process

[Junio C Hamano]

Moving to some other directory and letting the remainder of the test
pieces to expect that they start there is a bad practice.  The test
that contains chdir itself may fail (or by mistake skipped via the
GIT_SKIP_TESTS mechanism) in which case the remainder may operate on
files in unexpected places.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * This is purely a preparatory clean-up in the test script I'll be
   adding a new test to in the next patch.

 t/t0003-attributes.sh | 52 +++++++++++++++++++++++++++++----------------------
 1 file changed, 30 insertions(+), 22 deletions(-)

diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index febc45c..0554b13 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -197,39 +197,47 @@ test_expect_success 'root subdir attribute test' '
 '
 
 test_expect_success 'setup bare' '
-	git clone --bare . bare.git &&
-	cd bare.git
+	git clone --bare . bare.git
 '
 
 test_expect_success 'bare repository: check that .gitattribute is ignored' '
 	(
-		echo "f	test=f"
-		echo "a/i test=a/i"
-	) >.gitattributes &&
-	attr_check f unspecified &&
-	attr_check a/f unspecified &&
-	attr_check a/c/f unspecified &&
-	attr_check a/i unspecified &&
-	attr_check subdir/a/i unspecified
+		cd bare.git &&
+		(
+			echo "f	test=f"
+			echo "a/i test=a/i"
+		) >.gitattributes &&
+		attr_check f unspecified &&
+		attr_check a/f unspecified &&
+		attr_check a/c/f unspecified &&
+		attr_check a/i unspecified &&
+		attr_check subdir/a/i unspecified
+	)
 '
 
 test_expect_success 'bare repository: check that --cached honors index' '
-	GIT_INDEX_FILE=../.git/index \
-	git check-attr --cached --stdin --all <../stdin-all |
-	sort >actual &&
-	test_cmp ../specified-all actual
+	(
+		cd bare.git &&
+		GIT_INDEX_FILE=../.git/index \
+		git check-attr --cached --stdin --all <../stdin-all |
+		sort >actual &&
+		test_cmp ../specified-all actual
+	)
 '
 
 test_expect_success 'bare repository: test info/attributes' '
 	(
-		echo "f	test=f"
-		echo "a/i test=a/i"
-	) >info/attributes &&
-	attr_check f f &&
-	attr_check a/f f &&
-	attr_check a/c/f f &&
-	attr_check a/i a/i &&
-	attr_check subdir/a/i unspecified
+		cd bare.git &&
+		(
+			echo "f	test=f"
+			echo "a/i test=a/i"
+		) >info/attributes &&
+		attr_check f f &&
+		attr_check a/f f &&
+		attr_check a/c/f f &&
+		attr_check a/i a/i &&
+		attr_check subdir/a/i unspecified
+	)
 '
 
 test_done
-- 
1.9-rc2-233-ged4ee9f

[PATCH 2/2] check-attr: move to the top of working tree when in non-bare repository

[Junio C Hamano]

Lasse Makholm noticed that running "git check-attr" from a place
totally unrelated to $GIT_DIR and $GIT_WORK_TREE does not give
expected results.  I think it is because the command does not say it
wants to call setup_work_tree().

We still need to support use cases where only a bare repository is
involved, so unconditionally requiring a working tree would not work
well.  Instead, make a call only in a non-bare repository.

We may want to see if we want to do a similar fix in the opposite
direction to check-ignore.  The command unconditionally requires a
working tree, but it should be usable in a bare repository just like
check-attr attempts to be.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/check-attr.c  |  3 +++
 t/t0003-attributes.sh | 10 ++++++++++
 2 files changed, 13 insertions(+)

diff --git a/builtin/check-attr.c b/builtin/check-attr.c
index 075d01d..f29d6c3 100644
--- a/builtin/check-attr.c
+++ b/builtin/check-attr.c
@@ -94,6 +94,9 @@ int cmd_check_attr(int argc, const char **argv, const char *prefix)
 	struct git_attr_check *check;
 	int cnt, i, doubledash, filei;
 
+	if (!is_bare_repository())
+		setup_work_tree();
+
 	git_config(git_default_config, NULL);
 
 	argc = parse_options(argc, argv, prefix, check_attr_options,
diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index 0554b13..6e6aef5 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -196,6 +196,16 @@ test_expect_success 'root subdir attribute test' '
 	attr_check subdir/a/i unspecified
 '
 
+test_expect_success 'using --git-dir and --work-tree' '
+	mkdir unreal real &&
+	git init real &&
+	echo "file test=in-real" >real/.gitattributes &&
+	(
+		cd unreal &&
+		attr_check file in-real "--git-dir ../real/.git --work-tree ../real"
+	)
+'
+
 test_expect_success 'setup bare' '
 	git clone --bare . bare.git
 '
-- 
1.9-rc2-233-ged4ee9f

Re: [PATCH 2/2] check-attr: move to the top of working tree when in non-bare repository

[Jonathan Nieder]

Hi,

Junio C Hamano wrote:

> --- a/builtin/check-attr.c
> +++ b/builtin/check-attr.c
> @@ -94,6 +94,9 @@ int cmd_check_attr(int argc, const char **argv, const char *prefix)
>  	struct git_attr_check *check;
>  	int cnt, i, doubledash, filei;
>  
> +	if (!is_bare_repository())
> +		setup_work_tree();

Hm.  Shouldn't check-attr error out when run without a worktree and
without --cached?

That would mean something like

diff --git i/builtin/check-attr.c w/builtin/check-attr.c
index e9af7b2..c34b6ee 100644
--- i/builtin/check-attr.c
+++ w/builtin/check-attr.c
@@ -107,6 +107,9 @@ int cmd_check_attr(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, check_attr_options,
 			     check_attr_usage, PARSE_OPT_KEEP_DASHDASH);
 
+	if (!cached_attrs)
+		setup_work_tree();
+
 	if (read_cache() < 0) {
 		die("invalid cache");
 	}

Re: [PATCH 2/2] check-attr: move to the top of working tree when in non-bare repository

[Jonathan Nieder]

Hi again,

Jonathan Nieder wrote:
> Junio C Hamano wrote:

>> +	if (!is_bare_repository())
>> +		setup_work_tree();
>
> Hm.  Shouldn't check-attr error out when run without a worktree and
> without --cached?
> 
> That would mean something like
> 
> diff --git i/builtin/check-attr.c w/builtin/check-attr.c
> index e9af7b2..c34b6ee 100644
> --- i/builtin/check-attr.c
> +++ w/builtin/check-attr.c
> @@ -107,6 +107,9 @@ int cmd_check_attr(int argc, const char **argv, const char *prefix)
>  	argc = parse_options(argc, argv, prefix, check_attr_options,
>  			     check_attr_usage, PARSE_OPT_KEEP_DASHDASH);
>  
> +	if (!cached_attrs)
> +		setup_work_tree();

Someone asked in a private reply how this interacts with t0003.

t0003 tries check-attr in a bare repository.  The question is, is that
a desirable feature, and are people relying on it?  If people are
relying on it, perhaps the intuitive behavior would be to make
check-attr use an only-look-at-HEAD mode by default when running in a
bare repo.

How do I use the only-look-at-HEAD mode from a non-bare repo?  If I
want attributes with respect to some other commit instead of HEAD, is
there a syntax for that?  The command doesn't seem to have been well
thought out.

Hope that helps,
Jonathan

Re: [PATCH 2/2] check-attr: move to the top of working tree when in non-bare repository

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Someone asked in a private reply how this interacts with t0003.

It was me mistakenly using "reply" not "reply all".

> t0003 tries check-attr in a bare repository.  The question is, is that
> a desirable feature, and are people relying on it?

Running tar-tree from a public distribution point comes to mind.
bootstrap-attr-stack seems to have reference to is-bare-repository
to validate the attribute direction to read from the index, but I
tend to think what it really wants is to read from HEAD not the
index.

> How do I use the only-look-at-HEAD mode from a non-bare repo?

Is "You don't" a good answer?

Use of --cached when your index matches HEAD is the equivalent, and
if the index differs from HEAD, you must have had a reason to add
that change to .gitattributes to the index, so I think it is
reasonable to honour that change.

Re: [PATCH 2/2] check-attr: move to the top of working tree when in non-bare repository

[Michael Haggerty]

On 02/06/2014 09:17 PM, Jonathan Nieder wrote:
> How do I use the only-look-at-HEAD mode from a non-bare repo?  If I
> want attributes with respect to some other commit instead of HEAD, is
> there a syntax for that?  The command doesn't seem to have been well
> thought out.

I agree that it would be nice for "git check-attr" to handle this case.
 Currently, I believe that one has to resort to a temporary index file
via something like

    (
        export GIT_INDEX_FILE="$(mktemp)"
        git read-tree HEAD
        git check-attr --cached ...
        rm "$GIT_INDEX_FILE"
    )

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH 1/2] t0003: do not chdir the whole test process

[Jonathan Nieder]

Junio C Hamano wrote:

> Moving to some other directory and letting the remainder of the test
> pieces to expect that they start there is a bad practice.

I agree with the above, and I like the patch...

>                                                            The test
> that contains chdir itself may fail (or by mistake skipped via the
> GIT_SKIP_TESTS mechanism) in which case the remainder may operate on
> files in unexpected places.

... but this logic seems wrong.  I don't think we've ever supported
setup tests failing or being skipped in the past.

Thanks,
Jonathan

Re: [PATCH 1/2] t0003: do not chdir the whole test process

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Junio C Hamano wrote:
>
>> Moving to some other directory and letting the remainder of the test
>> pieces to expect that they start there is a bad practice.
>
> I agree with the above, and I like the patch...
>
>>                                                            The test
>> that contains chdir itself may fail (or by mistake skipped via the
>> GIT_SKIP_TESTS mechanism) in which case the remainder may operate on
>> files in unexpected places.
>
> ... but this logic seems wrong.  I don't think we've ever supported
> setup tests failing or being skipped in the past.

The first set-up test, yes, but something in the middle added as an
afterthought?

Re: [PATCH 1/2] t0003: do not chdir the whole test process

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>>>                                                            The test
>>> that contains chdir itself may fail (or by mistake skipped via the
>>> GIT_SKIP_TESTS mechanism) in which case the remainder may operate on
>>> files in unexpected places.
>>
>> ... but this logic seems wrong.  I don't think we've ever supported
>> setup tests failing or being skipped in the past.
>
> The first set-up test, yes, but something in the middle added as an
> afterthought?

Even set-up in the middle added as an afterthought, yes.

For a while I've been wanting to teach GIT_SKIP_TESTS not to skip
tests with 'setup' or 'set up' in their name, but I never got around
to it.  If I try to skip the setup test this patch touches, then there
is no bare.git and lots of later tests fail.  Perhaps it would be
better for each test to do

	rm -fr bare.git &&
	git clone --bare . bare.git &&
	(
		cd bare.git &&
		...
	)

for itself to make the state easier to think about.

On the other hand I agree that the 'cd' here is a bad practice.  I
just don't think it's about skipping setup --- instead, it's about it
being hard to remember the cwd in general.

Thanks,
Jonathan

Re: [PATCH 1/2] t0003: do not chdir the whole test process

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> For a while I've been wanting to teach GIT_SKIP_TESTS not to skip
> tests with 'setup' or 'set up' in their name, but I never got around
> to it.

Yeah, that would be a good thing.  As part of doing so, we might
want to come up with a way to test the tests, randomly skipping
pieces that are not "setup" and find ones that break the later tests
when skipped, and mark test scripts that fail such a test for fixing.

> If I try to skip the setup test this patch touches, then there
> is no bare.git and lots of later tests fail.  Perhaps it would be
> better for each test to do
>
> 	rm -fr bare.git &&
> 	git clone --bare . bare.git &&
> 	(
> 		cd bare.git &&
> 		...
> 	)
>
> for itself to make the state easier to think about.

That is a better and worse way to do it at the same time ;-)  It
definitely is better from maintainability POV to keep each test as
independent as possible.  It however also is worse if it forces us
to be repetitive X-<.

> On the other hand I agree that the 'cd' here is a bad practice.  I
> just don't think it's about skipping setup --- instead, it's about it
> being hard to remember the cwd in general.

Exactly.

Re: attr.c doesn't honor --work-tree option

[Lasse Makholm]

On 6 February 2014 18:54, Junio C Hamano <gitster@pobox.com> wrote:
> Lasse Makholm <lasse.makholm@gmail.com> writes:
>
>> Here's a repro with -DDEBUG_ATTR=1 and a printf() in read_attr_from_file():
>>
>> $ cd /tmp/
>> $ mkdir -p attr-test/repo
>> $ cd attr-test/repo
>> $ git init
>> Initialized empty Git repository in /tmp/attr-test/repo/.git/
>> $ echo 'dir/* filter=foo' >.gitattributes
>> $
>>
>> Inside the working tree, it works:
>>
>> $ ~/src/git.git/git check-attr -a dir/file
>
> Does check-ignore misbehave the same way?

No, check-ignore works but also has NEED_WORK_TREE set. And that
actually also feels a bit wrong to me because check-attr and
check-ignore both seem like reasonable things to do in a bare repo
because .git(attributes|ignore) files are likely to be committed in
the repo.

> I suspect that is this because check-attr is not a command that
> requires a working tree.  The command was written primarily as a
> debugging aid that can be used anywhere as long as you have a
> repository to read strings from either its standard input or its
> arguments, and gives them directly to check_attr(), but it does so
> without first going to the top of the real working tree like
> check-ignore does.

Fair point. I actually stumbled across this because a git cat-file
--textconv ... was failing, so that's at least one other (and arguably
more real) use case that is broken in the same way.

> Forcing it to go to the top of the working tree (see the attached
> one-liner, but note that I didn't test it) may give you want you
> want.

For this case, it does, yes. But it also breaks check-attr in bare
repos with attributes defined in $GIT_DIR/info/attributes because it
will refuse to run without a work tree...

In any case the current state seems broken because --work-tree clearly
doesn't work for all commands...

Setting NEED_WORK_TREE for check-attr risks breaking existing scripts
but on the other hand there doesn't seem to be any good reason why
check-attr and check-ignore should differ in this regard...

It seems like the ideal solution would be an optional NEED_WORK_TREE
of some sort that would let these commands work correctly both with
--work-tree, without it and in bare repos but I get that that might
not be easy to fix...

Another approach might be to deprecate --work-tree and tell people to
use -C instead...

/L

>  git.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/git.c b/git.c
> index 7cf2953..314ec9f 100644
> --- a/git.c
> +++ b/git.c
> @@ -342,7 +342,7 @@ static struct cmd_struct commands[] = {
>         { "branch", cmd_branch, RUN_SETUP },
>         { "bundle", cmd_bundle, RUN_SETUP_GENTLY },
>         { "cat-file", cmd_cat_file, RUN_SETUP },
> -       { "check-attr", cmd_check_attr, RUN_SETUP },
> +       { "check-attr", cmd_check_attr, RUN_SETUP | NEED_WORK_TREE },
>         { "check-ignore", cmd_check_ignore, RUN_SETUP | NEED_WORK_TREE },
>         { "check-mailmap", cmd_check_mailmap, RUN_SETUP },
>         { "check-ref-format", cmd_check_ref_format },