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

From: Lasse Makholm <lasse.makholm@gmail.com>
Date: Feb 10, 2014, 12:57 UTC
Message-ID: <CAB7pA0-dhFS_QOqxTdLmHAnsOUXSrmGi+dRR=+fMyY4M9m-XLA@mail.gmail.com>
In-Reply-To: <xmqqvbwsyvn8.fsf@gitster.dls.corp.google.com>

On 6 February 2014 18:54, Junio C Hamano <gitster@pobox.com> wrote:

Show 17 quoted lines

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

Show 7 quoted lines

> 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

Show 16 quoted lines

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