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

Lasse Makholm [off-list ref] 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 },

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

 * 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

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

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

Hi,

Junio C Hamano wrote:

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");
 	}

Hi again,

Jonathan Nieder wrote:

Junio C Hamano wrote:

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

On 6 February 2014 18:54, Junio C Hamano [off-list ref] wrote:

Lasse Makholm [off-list ref] writes:

quoted

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

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/