Re: [PATCH v2 3/3] archive: do not read .gitattributes in working directory

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 16, 2009, 00:26 UTC
Message-ID: <7vr5ztpho3.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <49E4FCF8.3050207@lsrfire.ath.cx>

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

Show 10 quoted lines

> Junio C Hamano schrieb:
> ...
>> Ah, bootstrap_attr_stack() and prepare_attr_stack() still assume that you
>> won't be doing any per-level attributes in a bare repository because the
>> concept of attributes is inherently tied to having a work tree from their
>> point of view.
>> 
>> How about this "mostly re-indent with four line removal" patch?
>
> Plus the following (on top of Duy's GIT_ATTR_INDEX patch)?

Actually, the "mostly re-indent" patch breaks things for normal users that expect the attributes never kicks in when you are doing things in a bare repository as-is.

Show 13 quoted lines

> diff --git a/attr.c b/attr.c
> index f5917de..cce561b 100644
> --- a/attr.c
> +++ b/attr.c
> @@ -672,6 +672,8 @@ int git_checkattr(const char *path, int num, struct git_attr_check *check)
>  void git_attr_set_direction(enum git_attr_direction new, struct index_state *istate)
>  {
>  	enum git_attr_direction old = direction;
> +	if (is_bare_repository())
> +		new = GIT_ATTR_INDEX;
>  	direction = new;
>  	if (new != old)
>  		drop_attr_stack();

This looks _conceptually_ wrong.

I think your patch came from the fact that check_updates() unconditionally calls git_attr_set_direction() without checking o->update, and I think it is a bug in check_updates().

A bare repository is by definition without a work tree, and we shouldn't be reading the index in general. I wouldn't go as far to say that a bare repository should not have the index file, because people often clone forgetting the --bare option and manually convert that to a bare repository, and they forget to remove the index that is never used.

	... thinks a bit more ...

I think codepaths that make calls to git_attr_set_direction() inside is_bare_repository() are special already. If we were to teach check-attr how to check attributes for paths _inside a given tree-ish_, most likely the implementation would be similar to what Duy did for archive; read the tree into in-core index, set direction to the INDEX, and start using the attribute mechanism.

So I think we'd rather not have the patch to force GIT_ATTR_INDEX in set_direction(); if anything, the patch should say something like "if we are in a bare repository and the new direction is not INDEX, it is a programming error".

Instead, such specialized codepaths should call set_direction() itself, perhaps after reading the tree-ish into the in-core index. And we should fix the "mostly re-indent" patch not to remove the conditional, but make the conditional to check "If in a bare repository and the direction is not explicitly set to INDEX, do not use the attributes".