Re: [PATCH 2/2] checkout: fix attribute handling in checkout all

From: Steffen Prohaska <prohaska@zib.de>
Date: Aug 12, 2007, 22:26 UTC
Message-ID: <BC69C8EA-7486-4542-B866-EC52A5BF4B1F@zib.de>
In-Reply-To: <7veji8ifs2.fsf@assigned-by-dhcp.cox.net>

On Aug 12, 2007, at 11:50 PM, Junio C Hamano wrote:

Show 16 quoted lines

> Steffen Prohaska <prohaska@zib.de> writes:
>
>> We need to check out .gitattributes files first to have
>> them in place when we check out the remaining files. This
>> is needed to get the right attributes during checkout,
>> for example having the right crlf conversion on the first
>> checkout if crlf is controlled by a .gitattribute file.
>>
>> This works only together with the commit
>>
>> 'attr: fix attribute handling if .gitattributes is involved'
>
> While I think it is _one_ good approach to make things two-pass,
> I do not know if this is enough.  A logic similar to this should
> be made available to the codepath that switches branches,
> shouldn't it?

I think so. I played a bit more and I am pretty sure that switching branches suffers from the same problem. After I understood the problem I now remember that I wondered why I needed '-f' now and then to convince git to do things that normally just work.

> It feels somewhat bogus to treat only the files that contain
> ".gitattributes" as substring.  Don't you want to at least say
> "is .gitattributes or ends with /.gitattributes"?

Yes, if someone really want to construct a case, he can break my code. Where should I place a helper function, such as ends_with_gitattributes()?

Show 10 quoted lines

> I am not 100% convinced that it is "unexpected" that
> these two sequences give different results.
>
>  (1) rm -f .gitattributes other
>      git-checkout-index -f .gitattributes
>      git-checkout-index -f other
>
>  (2) rm -f .gitattributes other
>      git-checkout-index -f other
>      git-checkout-index -f .gitattributes

Yeah, it's not obvious to me either.

My feeling it that the working tree should match the current content of .gitattributes. That is after you modified .gittattributes by whatever means (checkout, editor, ...), or you modified the global default for autocrlf, you should have a way to update your working tree. One way would be to force a fresh checkout of all files in the working tree. How can I do that?

I'm pretty certain that all files in the working tree resulting from a single command should match the .gitattributes that were modified by the same command. This is true for initial checkout, but also for branch switching.

Show 9 quoted lines

> And if this is mostly to work around the chicken-and-egg problem
> of the initial checkout, I do not know if we would want to
> complicate checkout_all() nor prepare_attr_stack().  Perhaps the
> _initial_ checkout can do something like:
>
>  * look at index, checkout .gitattributes and */.gitattributes;
>  * checkout -f -a
>
> _at the Porcelain level_, without complicating the plumbing?

I'm not convinced that it's only the initial checkout. As you already mentioned above, branch switching suffers from the same problem. But it could perhaps be handled on a porcelain level.

Maybe we should start with some test cases first?

> Both patches are seriously out of existing coding style, by the
> way.  Extra spaces after called function names everywhere, etc.

Hmm, I see, ... my daytime coding style got burnt into my brain. I'd rework if needed. But let's first find out what's needed.

	Steffen