threads / discuss / 10403

.gittattributes handling has deficiencies

Subject: .gittattributes handling has deficiencies

## tl;dr

14 messages between Oct 21, 2007 and Oct 22, 2007.

replies: 13people: 4as markdown or json

Steffen Prohaska· Oct 21, 2007, 08:48 UTC · lore

If a .gitattributes is in the work tree and we checkout a different head, the .gitattributes of the head we are switching to must have precedence. Files are expected to be converted as configured in the .gitattributes that is available in the head we're switching to.

This adds a test case revealing deficiencies of the current handling of .gitattributes.

At a first glance, I saw two possible resolutions:
1) .gitattributes from the index has precedence. It's unclear
   how merging can be handled appropriately.
2) .gitattributes are handled as a special file. Checkout is a
   two pass process. In the first pass only the special file
   .gitattributes is checked out. In th second pass the remaining
   files are added. Maybe this gives a perspective how to handle
   merges.
But actually the issue is much harder to solve.

Here is what needs to be done: Whenever the attributes of a file change the file must be freshly checked out according to the attributes of the head we switch to. The file itself does not necessarily change between the two commits. A fresh checkout is already needed if only .gitattributes change.

But this is really hard to solve. We would need to compare attributes before and after for _all_ files that have attributes in one of the two commits and check if they changed. If so, we need to do a fresh checkout according to the new attributes.

Maybe the gitattributes of a file should be part of the per-file flags in the index. Thus we could verify if the flags changed and if so, adjust the work tree accordig to the new flags. I'm lacking a deeper insight into the git internals. Therefore, I can't really say if the index is the right place. But it looks to me as if changing an attribute should be treated similar to a changing sha1, as far as the work tree is concerned.

So, I need some help.
    Steffen
---
 t/t0020-crlf.sh |   23 +++++++++++++++++++++++
 1 files changed, 23 insertions(+), 0 deletions(-)
diff --git a/t/t0020-crlf.sh b/t/t0020-crlf.sh
index 62bc4bb..5d7e033 100755
--- a/t/t0020-crlf.sh
+++ b/t/t0020-crlf.sh
@@ -371,6 +371,29 @@ test_expect_success 'in-tree .gitattributes (4)' '
 	}
 '
 
+test_expect_success 'in-tree .gitattributes (5)' '
+
+	git reset --hard master &&
+	echo >.gitattributes &&
+	git add .gitattributes &&
+	git commit -m "empty .gitattributes" &&
+	rm -rf tmp one dir .gitattributes patch.file three &&
+	git reset --hard master &&
+	git checkout master^ &&
+
+	if remove_cr one >/dev/null
+	then
+		echo "Eh? one should not have CRLF"
+		false
+	else
+		: happy
+	fi &&
+	remove_cr three >/dev/null || {
+		echo "Eh? three should still have CRLF"
+		false
+	}
+'
+
 test_expect_success 'invalid .gitattributes (must not crash)' '
 
 	echo "three +crlf" >>.gitattributes &&

-- 
1.5.3.mingw.1.138.g7bf9d
david@lang.hm· Oct 21, 2007, 09:19 UTC · re: Steffen Prohaska · lore

Re: .gittattributes handling has deficiencies

On Sun, 21 Oct 2007, Steffen Prohaska wrote:
Show 23 quoted lines
> If a .gitattributes is in the work tree and we checkout a
> different head, the .gitattributes of the head we are switching
> to must have precedence. Files are expected to be converted as
> configured in the .gitattributes that is available in the head
> we're switching to.
>
> This adds a test case revealing deficiencies of the current
> handling of .gitattributes.
>
> At a first glance, I saw two possible resolutions:
> 1) .gitattributes from the index has precedence. It's unclear
>   how merging can be handled appropriately.
> 2) .gitattributes are handled as a special file. Checkout is a
>   two pass process. In the first pass only the special file
>   .gitattributes is checked out. In th second pass the remaining
>   files are added. Maybe this gives a perspective how to handle
>   merges.
>
> But actually the issue is much harder to solve.
>
> Here is what needs to be done: Whenever the attributes of a file
> change the file must be freshly checked out according to the
> attributes of the head we switch to.

this is the same problem that we would have with the extended file attributes, in the discussion for those it was suggested that I refrence the file from the index rather then from the file system to avoid needing to do two passes.

Show 8 quoted lines
> The file itself does not
> necessarily change between the two commits. A fresh checkout is
> already needed if only .gitattributes change.
>
> But this is really hard to solve. We would need to compare
> attributes before and after for _all_ files that have attributes
> in one of the two commits and check if they changed. If so, we
> need to do a fresh checkout according to the new attributes.

if you know that you will get the new .gitattributes if it changes, setup a post-checkout hook to checkout everything if it has changed. it's far from ideal, but it should be a good, safe, first approximation.

you shouldn't need to check all files of all attributes, only on any that match the lines that get changed. the hook for this is exactly the type of thing that I was talking about in the metastore thread.

Show 7 quoted lines
> Maybe the gitattributes of a file should be part of the per-file
> flags in the index. Thus we could verify if the flags changed and
> if so, adjust the work tree accordig to the new flags.  I'm
> lacking a deeper insight into the git internals.  Therefore, I
> can't really say if the index is the right place.  But it looks
> to me as if changing an attribute should be treated similar to a
> changing sha1, as far as the work tree is concerned.

the problem with this is that each attribute ends up needing it's own flag, which severely limits extending things (see the discussions on file permissions for examples). it's also much harder to manipulate them then in a file.

David Lang
Show 42 quoted lines
> So, I need some help.
>    Steffen
>
> ---
> t/t0020-crlf.sh |   23 +++++++++++++++++++++++
> 1 files changed, 23 insertions(+), 0 deletions(-)
>
> diff --git a/t/t0020-crlf.sh b/t/t0020-crlf.sh
> index 62bc4bb..5d7e033 100755
> --- a/t/t0020-crlf.sh
> +++ b/t/t0020-crlf.sh
> @@ -371,6 +371,29 @@ test_expect_success 'in-tree .gitattributes (4)' '
> 	}
> '
>
> +test_expect_success 'in-tree .gitattributes (5)' '
> +
> +	git reset --hard master &&
> +	echo >.gitattributes &&
> +	git add .gitattributes &&
> +	git commit -m "empty .gitattributes" &&
> +	rm -rf tmp one dir .gitattributes patch.file three &&
> +	git reset --hard master &&
> +	git checkout master^ &&
> +
> +	if remove_cr one >/dev/null
> +	then
> +		echo "Eh? one should not have CRLF"
> +		false
> +	else
> +		: happy
> +	fi &&
> +	remove_cr three >/dev/null || {
> +		echo "Eh? three should still have CRLF"
> +		false
> +	}
> +'
> +
> test_expect_success 'invalid .gitattributes (must not crash)' '
>
> 	echo "three +crlf" >>.gitattributes &&
>
Steffen Prohaska· Oct 21, 2007, 10:27 UTC · re: david@lang.hm · lore

Re: .gittattributes handling has deficiencies

On Oct 21, 2007, at 11:19 AM, david@lang.hm wrote:
Show 9 quoted lines
>> But this is really hard to solve. We would need to compare
>> attributes before and after for _all_ files that have attributes
>> in one of the two commits and check if they changed. If so, we
>> need to do a fresh checkout according to the new attributes.
>
> if you know that you will get the new .gitattributes if it changes,  
> setup a post-checkout hook to checkout everything if it has  
> changed. it's far from ideal, but it should be a good, safe, first  
> approximation.

That's not good enough. I'll stop using .gitattributes. I need to teach >40 devs how to use git on Windows. I only use features that work flawlessly. .gitattributes doesn't. It bit me twice now.

Luckily, core.autocrlf works if you set it before the first checkout and never change it. This seems sufficient for me if all files that have mixed line endings are fixed right away.

	Steffen
david@lang.hm· Oct 21, 2007, 17:09 UTC · re: Steffen Prohaska · lore

Re: .gittattributes handling has deficiencies

On Sun, 21 Oct 2007, Steffen Prohaska wrote:
Show 16 quoted lines
> On Oct 21, 2007, at 11:19 AM, david@lang.hm wrote:
>
>>> But this is really hard to solve. We would need to compare
>>> attributes before and after for _all_ files that have attributes
>>> in one of the two commits and check if they changed. If so, we
>>> need to do a fresh checkout according to the new attributes.
>> 
>> if you know that you will get the new .gitattributes if it changes, setup a 
>> post-checkout hook to checkout everything if it has changed. it's far from 
>> ideal, but it should be a good, safe, first approximation.
>
>
> That's not good enough. I'll stop using .gitattributes. I
> need to teach >40 devs how to use git on Windows. I only use
> features that work flawlessly. .gitattributes doesn't. It bit
> me twice now.

why would checking everything out if .gitattributes has changed not work? I can see why _not_ doing so would cause problems, and I freely acknowledge that this approach imposes a performance hit by checking everything out twice, but I don't see how it would not be reliable.

David Lang
Show 5 quoted lines
> Luckily, core.autocrlf works if you set it before the first
> checkout and never change it. This seems sufficient for me if
> all files that have mixed line endings are fixed right away.
>
> 	Steffen
Steffen Prohaska· Oct 21, 2007, 17:25 UTC · re: david@lang.hm · lore

Re: .gittattributes handling has deficiencies

On Oct 21, 2007, at 7:09 PM, david@lang.hm wrote:
Show 24 quoted lines
> On Sun, 21 Oct 2007, Steffen Prohaska wrote:
>
>> On Oct 21, 2007, at 11:19 AM, david@lang.hm wrote:
>>
>>>> But this is really hard to solve. We would need to compare
>>>> attributes before and after for _all_ files that have attributes
>>>> in one of the two commits and check if they changed. If so, we
>>>> need to do a fresh checkout according to the new attributes.
>>> if you know that you will get the new .gitattributes if it  
>>> changes, setup a post-checkout hook to checkout everything if it  
>>> has changed. it's far from ideal, but it should be a good, safe,  
>>> first approximation.
>>
>>
>> That's not good enough. I'll stop using .gitattributes. I
>> need to teach >40 devs how to use git on Windows. I only use
>> features that work flawlessly. .gitattributes doesn't. It bit
>> me twice now.
>
> why would checking everything out if .gitattributes has changed not  
> work? I can see why _not_ doing so would cause problems, and I  
> freely acknowledge that this approach imposes a performance hit by  
> checking everything out twice, but I don't see how it would not be  
> reliable.

What do you mean by "checking out everything"? Which command do you propose?

	Steffen
david@lang.hm· Oct 21, 2007, 17:57 UTC · re: Steffen Prohaska · lore

Re: .gittattributes handling has deficiencies

On Sun, 21 Oct 2007, Steffen Prohaska wrote:
Show 27 quoted lines
> On Oct 21, 2007, at 7:09 PM, david@lang.hm wrote:
>
>> On Sun, 21 Oct 2007, Steffen Prohaska wrote:
>> 
>>> On Oct 21, 2007, at 11:19 AM, david@lang.hm wrote:
>>> 
>>>>> But this is really hard to solve. We would need to compare
>>>>> attributes before and after for _all_ files that have attributes
>>>>> in one of the two commits and check if they changed. If so, we
>>>>> need to do a fresh checkout according to the new attributes.
>>>> if you know that you will get the new .gitattributes if it changes, setup 
>>>> a post-checkout hook to checkout everything if it has changed. it's far 
>>>> from ideal, but it should be a good, safe, first approximation.
>>> 
>>> 
>>> That's not good enough. I'll stop using .gitattributes. I
>>> need to teach >40 devs how to use git on Windows. I only use
>>> features that work flawlessly. .gitattributes doesn't. It bit
>>> me twice now.
>> 
>> why would checking everything out if .gitattributes has changed not work? I 
>> can see why _not_ doing so would cause problems, and I freely acknowledge 
>> that this approach imposes a performance hit by checking everything out 
>> twice, but I don't see how it would not be reliable.
>
> What do you mean by "checking out everything"?
> Which command do you propose?
something like git checkout -f
David Lang
Steffen Prohaska· Oct 21, 2007, 20:21 UTC · re: david@lang.hm · lore

Re: .gittattributes handling has deficiencies

On Oct 21, 2007, at 7:57 PM, david@lang.hm wrote:
Show 28 quoted lines
> On Sun, 21 Oct 2007, Steffen Prohaska wrote:
>
>> On Oct 21, 2007, at 7:09 PM, david@lang.hm wrote:
>>
>>> On Sun, 21 Oct 2007, Steffen Prohaska wrote:
>>>> On Oct 21, 2007, at 11:19 AM, david@lang.hm wrote:
>>>>>> But this is really hard to solve. We would need to compare
>>>>>> attributes before and after for _all_ files that have attributes
>>>>>> in one of the two commits and check if they changed. If so, we
>>>>>> need to do a fresh checkout according to the new attributes.
>>>>> if you know that you will get the new .gitattributes if it  
>>>>> changes, setup a post-checkout hook to checkout everything if  
>>>>> it has changed. it's far from ideal, but it should be a good,  
>>>>> safe, first approximation.
>>>> That's not good enough. I'll stop using .gitattributes. I
>>>> need to teach >40 devs how to use git on Windows. I only use
>>>> features that work flawlessly. .gitattributes doesn't. It bit
>>>> me twice now.
>>> why would checking everything out if .gitattributes has changed  
>>> not work? I can see why _not_ doing so would cause problems, and  
>>> I freely acknowledge that this approach imposes a performance hit  
>>> by checking everything out twice, but I don't see how it would  
>>> not be reliable.
>>
>> What do you mean by "checking out everything"?
>> Which command do you propose?
>
> something like git checkout -f
I suspected this. I see two problems:
1) it's too dangerous: I throws away _all_ changes, not only
changes that are related to gitattributes.
2) it doesn't work reliably. git checkout -f will only update
files that git detects as changed. But you could have files that
should have crlf in the working copy but actually have only lf.
Those would not be updated.

I'll not recommend this. Not using .gitattributes is the only sane solution.

	Steffen
david@lang.hm· Oct 21, 2007, 23:49 UTC · re: Steffen Prohaska · lore

Re: .gittattributes handling has deficiencies

On Sun, 21 Oct 2007, Steffen Prohaska wrote:
Show 34 quoted lines
> On Oct 21, 2007, at 7:57 PM, david@lang.hm wrote:
>
>> On Sun, 21 Oct 2007, Steffen Prohaska wrote:
>> 
>>> On Oct 21, 2007, at 7:09 PM, david@lang.hm wrote:
>>> 
>>>> On Sun, 21 Oct 2007, Steffen Prohaska wrote:
>>>>> On Oct 21, 2007, at 11:19 AM, david@lang.hm wrote:
>>>>>>> But this is really hard to solve. We would need to compare
>>>>>>> attributes before and after for _all_ files that have attributes
>>>>>>> in one of the two commits and check if they changed. If so, we
>>>>>>> need to do a fresh checkout according to the new attributes.
>>>>>> if you know that you will get the new .gitattributes if it changes, 
>>>>>> setup a post-checkout hook to checkout everything if it has changed. 
>>>>>> it's far from ideal, but it should be a good, safe, first 
>>>>>> approximation.
>>>>> That's not good enough. I'll stop using .gitattributes. I
>>>>> need to teach >40 devs how to use git on Windows. I only use
>>>>> features that work flawlessly. .gitattributes doesn't. It bit
>>>>> me twice now.
>>>> why would checking everything out if .gitattributes has changed not work? 
>>>> I can see why _not_ doing so would cause problems, and I freely 
>>>> acknowledge that this approach imposes a performance hit by checking 
>>>> everything out twice, but I don't see how it would not be reliable.
>>> 
>>> What do you mean by "checking out everything"?
>>> Which command do you propose?
>> 
>> something like git checkout -f
>
> I suspected this. I see two problems:
>
> 1) it's too dangerous: I throws away _all_ changes, not only
> changes that are related to gitattributes.

this is true, the question of if this is 'too dangerous' depends on what workflow you teach as safe. if you teach that checking out a new version will loose any modifications you have made (which is useually the sane thing to do by default anyway) then this is just more of the same

> 2) it doesn't work reliably. git checkout -f will only update
> files that git detects as changed. But you could have files that
> should have crlf in the working copy but actually have only lf.
> Those would not be updated.

ok, you could do rm -r * before doing the checkout -f (or there's probably a option to git to tell it not to preserve changes to the working area, I am not a git guru.

> I'll not recommend this. Not using .gitattributes is the only
> sane solution.

it may be the best thing to do for you and your users, that's not the same thing as saying that it's the only sane solution.

David Lang
david@lang.hm· Oct 22, 2007, 00:05 UTC · re: david@lang.hm · lore

Re: .gittattributes handling has deficiencies

On Sun, 21 Oct 2007, david@lang.hm wrote:
Show 30 quoted lines
>>>> 
>>>> What do you mean by "checking out everything"?
>>>> Which command do you propose?
>>> 
>>> something like git checkout -f
>> 
>> I suspected this. I see two problems:
>> 
>> 1) it's too dangerous: I throws away _all_ changes, not only
>> changes that are related to gitattributes.
>
> this is true, the question of if this is 'too dangerous' depends on what 
> workflow you teach as safe. if you teach that checking out a new version will 
> loose any modifications you have made (which is useually the sane thing to do 
> by default anyway) then this is just more of the same
>
>> 2) it doesn't work reliably. git checkout -f will only update
>> files that git detects as changed. But you could have files that
>> should have crlf in the working copy but actually have only lf.
>> Those would not be updated.
>
> ok, you could do rm -r * before doing the checkout -f (or there's probably a 
> option to git to tell it not to preserve changes to the working area, I am 
> not a git guru.
>
>> I'll not recommend this. Not using .gitattributes is the only
>> sane solution.
>
> it may be the best thing to do for you and your users, that's not the same 
> thing as saying that it's the only sane solution.

by the way, I am not saying that my suggestion is the right way for things to be (especially long term), but I'm trying to figure out a work-around for the short term.

I'm very interested to see the logn-term suggestions, becouse I suspect that modt of them could be leveraged for the metastore jobs.

David Lang
Steffen Prohaska· Oct 22, 2007, 05:38 UTC · re: david@lang.hm · lore

Re: .gittattributes handling has deficiencies

On Oct 22, 2007, at 2:05 AM, david@lang.hm wrote:
>
> by the way, I am not saying that my suggestion is the right way for  
> things to be (especially long term), but I'm trying to figure out a  
> work-around for the short term.

And I'm saying your proposed workaround is dangerous and doesn't work reliably.

> I'm very interested to see the logn-term suggestions, becouse I  
> suspect that modt of them could be leveraged for the metastore jobs.
I'd prefer this, too.
	
	Steffen
Shawn O. Pearce· Oct 22, 2007, 05:01 UTC · re: david@lang.hm · lore

Re: .gittattributes handling has deficiencies

david@lang.hm wrote:
Show 17 quoted lines
> On Sun, 21 Oct 2007, Steffen Prohaska wrote:
> >If a .gitattributes is in the work tree and we checkout a
> >different head, the .gitattributes of the head we are switching
> >to must have precedence.
> >
> >Maybe the gitattributes of a file should be part of the per-file
> >flags in the index. Thus we could verify if the flags changed and
> >if so, adjust the work tree accordig to the new flags.  I'm
> >lacking a deeper insight into the git internals.  Therefore, I
> >can't really say if the index is the right place.  But it looks
> >to me as if changing an attribute should be treated similar to a
> >changing sha1, as far as the work tree is concerned.
> 
> the problem with this is that each attribute ends up needing it's own 
> flag, which severely limits extending things (see the discussions on file 
> permissions for examples). it's also much harder to manipulate them then 
> in a file.

Yea, you really don't want to copy .gitattributes into the per-file records in the index. That's not going to scale as more types of attributes are defined.

Fortunately the .gitattributes file format was designed to be readable even when there's merge conflicts; that is it is a very simple line-oriented record format. One could difference the old .gitattributes currently found in the index against the .gitattributes we are switching to (from the target tree-ish), scan the lines removed/added, find which files those match against in the target tree-ish, and just add those files to the list of things we need to checkout.

If any of those files is dirty then we just refuse the checkout, just as if the file was modified and we were switching branches. The user then needs to decide how to continue (probably stash the file and then restart the checkout).

Rather simple IMHO. Of course I haven't gone into that part of read-tree recently, and the .gitattribute parser reads from the working directory, so you need to make sure you checkout the target .gitattributes file before anything else in the "to process list".

-- 
Shawn.
Steffen Prohaska· Oct 22, 2007, 05:45 UTC · re: Shawn O. Pearce · lore

Re: .gittattributes handling has deficiencies

On Oct 22, 2007, at 7:01 AM, Shawn O. Pearce wrote:
Show 38 quoted lines
> david@lang.hm wrote:
>> On Sun, 21 Oct 2007, Steffen Prohaska wrote:
>>> If a .gitattributes is in the work tree and we checkout a
>>> different head, the .gitattributes of the head we are switching
>>> to must have precedence.
>>>
>>> Maybe the gitattributes of a file should be part of the per-file
>>> flags in the index. Thus we could verify if the flags changed and
>>> if so, adjust the work tree accordig to the new flags.  I'm
>>> lacking a deeper insight into the git internals.  Therefore, I
>>> can't really say if the index is the right place.  But it looks
>>> to me as if changing an attribute should be treated similar to a
>>> changing sha1, as far as the work tree is concerned.
>>
>> the problem with this is that each attribute ends up needing it's own
>> flag, which severely limits extending things (see the discussions  
>> on file
>> permissions for examples). it's also much harder to manipulate  
>> them then
>> in a file.
>
> Yea, you really don't want to copy .gitattributes into the per-file
> records in the index.  That's not going to scale as more types of
> attributes are defined.
>
> Fortunately the .gitattributes file format was designed to be
> readable even when there's merge conflicts; that is it is a
> very simple line-oriented record format.  One could difference
> the old .gitattributes currently found in the index against the
> .gitattributes we are switching to (from the target tree-ish),
> scan the lines removed/added, find which files those match against
> in the target tree-ish, and just add those files to the list of
> things we need to checkout.
>
> If any of those files is dirty then we just refuse the checkout,
> just as if the file was modified and we were switching branches.
> The user then needs to decide how to continue (probably stash the
> file and then restart the checkout).

Sounds like a reasonable plan. I have no time to look into this right away.

> Rather simple IMHO.  Of course I haven't gone into that part of
> read-tree recently, and the .gitattribute parser reads from the
> working directory, so you need to make sure you checkout the target
> .gitattributes file before anything else in the "to process list".

.gitattributes is first looked for in the working directory, and if not there, .gitattributes is read from the index.

	Steffen
Johannes Schindelin· Oct 22, 2007, 10:29 UTC · re: Steffen Prohaska · lore

Re: .gittattributes handling has deficiencies

Hi,
On Mon, 22 Oct 2007, Steffen Prohaska wrote:
> .gitattributes is first looked for in the working directory, and if not 
> there, .gitattributes is read from the index.

Of course we could change that to do it the other way round. But this would contradict expectations when you edit .gitattributes and then checkout single files without having git-add'ed .gitattributes first.

The biggest problem in your setup, however, is not if .gitattributes is read from the index or the working directory. The biggest problem is that files are not touched when their contents have not changed.

IOW if you have .gitattributes in the to-be-checked-out branch which say that README is crlf, and in the current branch it is not, and README's _contents_ are identical in both branches, a "git checkout <that-other-branch>" will not rewrite README, and consequently not change the working copy to crlf.

Ciao, Dscho

Steffen Prohaska· Oct 22, 2007, 13:04 UTC · re: Johannes Schindelin · lore

Re: .gittattributes handling has deficiencies

On Oct 22, 2007, at 12:29 PM, Johannes Schindelin wrote:
Show 24 quoted lines
>
> On Mon, 22 Oct 2007, Steffen Prohaska wrote:
>
>> .gitattributes is first looked for in the working directory, and  
>> if not
>> there, .gitattributes is read from the index.
>
> Of course we could change that to do it the other way round.  But this
> would contradict expectations when you edit .gitattributes and then
> checkout single files without having git-add'ed .gitattributes first.
>
> The biggest problem in your setup, however, is not  
> if .gitattributes is
> read from the index or the working directory.  The biggest problem  
> is that
> files are not touched when their contents have not changed.
>
> IOW if you have .gitattributes in the to-be-checked-out branch  
> which say
> that README is crlf, and in the current branch it is not, and README's
> _contents_ are identical in both branches, a "git checkout
> <that-other-branch>" will not rewrite README, and consequently not  
> change
> the working copy to crlf.

Exactly. The order of reading .gitattributes from the working directory or the index doesn't matter. The current mechanism has a more fundamental deficiency.

Changes of.gitattributes can influence how the same content is checked out to the work tree. If .gitattributes change the checkout may need to be updated, even if the real content did not change.

	Steffen

← back to recent threads