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

[Junio C Hamano <gitster@pobox.com>]

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

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

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