Git no longer reads attributes from the index properly

Git no longer reads attributes from the index properly

[Brian Downing]

As of 34110cd4e394e3f92c01a4709689b384c34645d8, (2008-03-06, just over a
year ago), Git no longer reads attributes from the index properly in all
cases.  This breaks initial checkouts where .gitattribute information is
required to get a good checkout.  [I'm also copying the msysgit guys on
this, since they are probably the primary customers of autocrlf and the
crlf attributes, though the problem exists in core Git.]

This support for reading attributes from the index in addition to the
working directory was added in 1a9d7e9b484e77436edc7f5cacd39c24ec605e6d,
on 2007-08-14:

commit 1a9d7e9b484e77436edc7f5cacd39c24ec605e6d
Author: Junio C Hamano <gitster@pobox.com>
Date:   Tue Aug 14 01:41:02 2007 -0700

    attr.c: read .gitattributes from index as well.

    This makes .gitattributes files to be read from the index when
    they are not checked out to the work tree.  This is in line with
    the way we always allowed low-level tools to operate in sparsely
    checked out work tree in a reasonable way.

    It swaps the order of new file creation and converting the blob
    to work tree representation; otherwise when we are in the middle
    of checking out .gitattributes we would notice an empty but
    unwritten .gitattributes file in the work tree and will ignore
    the copy in the index.

    Signed-off-by: Junio C Hamano <gitster@pobox.com>

This worked until the following commit:

34110cd4e394e3f92c01a4709689b384c34645d8 is first bad commit
commit 34110cd4e394e3f92c01a4709689b384c34645d8
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date:   Thu Mar 6 18:12:28 2008 -0800

    Make 'unpack_trees()' have a separate source and destination index

    We will always unpack into our own internal index, but we will take the
    source from wherever specified, and we will optionally write the result
    to a specified index (optionally, because not everybody even _wants_ any
    result: the index diffing really wants to just walk the tree and index
    in parallel).

    This ends up removing a fair number more lines than it adds, for the
    simple reason that we can now skip all the crud that tried to be
    oh-so-careful about maintaining our position in the index as we were
    traversing and modifying it.  Since we don't actually modify the source
    index any more, we can just update the 'o->pos' pointer without worrying
    about whether an index entry got removed or replaced or added to.

    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

Unfortunately I don't have the foggiest on how to fix this.  However, I
did write a new test case to catch this bug, as the existing ones were
insufficiently destructive to trigger it.  This test case is ugly, uses
porcelain, and is probably overly-destructive, but it does show the
problem.  It fails on 34110cd4 and succeeds on 34110cd4^.

I don't expect the test case can be accepted as-is, but regardless:

Signed-off-by: Brian Downing <bdowning@lavos.net>
---
 t/t0020-crlf.sh |   19 +++++++++++++++++++
 1 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/t/t0020-crlf.sh b/t/t0020-crlf.sh
index 1be7446..749cd09 100755
--- a/t/t0020-crlf.sh
+++ b/t/t0020-crlf.sh
@@ -429,6 +429,25 @@ test_expect_success 'in-tree .gitattributes (4)' '
 	}
 '
 
+test_expect_success 'in-tree .gitattributes (5)' '
+
+	rm .git/index &&
+	git clean -d -x -f &&
+	git checkout &&
+
+	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.6.3

Re: Git no longer reads attributes from the index properly

[Junio C Hamano]

bdowning@lavos.net (Brian Downing) writes:

> As of 34110cd4e394e3f92c01a4709689b384c34645d8, (2008-03-06, just over a
> year ago), Git no longer reads attributes from the index properly in all
> cases....

Perhaps you would want to try it on 06f33c1 (Read attributes from the
index that is being checked out, 2009-03-13) that is part of 'pu'?

Re: Git no longer reads attributes from the index properly

[Brian Downing]

On Fri, Mar 20, 2009 at 01:27:31AM -0700, Junio C Hamano wrote:
> bdowning@lavos.net (Brian Downing) writes:
> > As of 34110cd4e394e3f92c01a4709689b384c34645d8, (2008-03-06, just over a
> > year ago), Git no longer reads attributes from the index properly in all
> > cases....
> 
> Perhaps you would want to try it on 06f33c1 (Read attributes from the
> index that is being checked out, 2009-03-13) that is part of 'pu'?

I only tried it on next, groan.  Yes, it works there, thanks.

However, that commit looks like it's solving a different problem
entirely (supporting changing between two branches where .gitattributes
exists in both cases) and happens to fix the no .gitattributes -> read
from index regression at the same time.  I don't know enough about the
guts to tell, but does this also fix the core problem of the regression
(I assume something about trying to read from the wrong index, given the
commit that broke it), or does it just happen to work around it?

Specifically, it would be nice to have a fix for the regression that
could land on maint relatively soon, as the initial checkout case is
breaking a real repository I use, whereas the switching branches case is
something I don't care about as much at the moment.

Of course, I don't know how to fix it at the moment, and beggars can't
be choosers.  :)

-bcd

Re: Git no longer reads attributes from the index properly

[Michael J Gruber]

Brian Downing venit, vidit, dixit 20.03.2009 09:40:
> 
> On Fri, Mar 20, 2009 at 01:27:31AM -0700, Junio C Hamano wrote:
>> bdowning@lavos.net (Brian Downing) writes:
>>> As of 34110cd4e394e3f92c01a4709689b384c34645d8, (2008-03-06, just over a
>>> year ago), Git no longer reads attributes from the index properly in all
>>> cases....
>>
>> Perhaps you would want to try it on 06f33c1 (Read attributes from the
>> index that is being checked out, 2009-03-13) that is part of 'pu'?
> 
> I only tried it on next, groan.  Yes, it works there, thanks.
> 
> However, that commit looks like it's solving a different problem
> entirely (supporting changing between two branches where .gitattributes
> exists in both cases) and happens to fix the no .gitattributes -> read
> from index regression at the same time.  I don't know enough about the
> guts to tell, but does this also fix the core problem of the regression
> (I assume something about trying to read from the wrong index, given the
> commit that broke it), or does it just happen to work around it?
> 
> Specifically, it would be nice to have a fix for the regression that
> could land on maint relatively soon, as the initial checkout case is
> breaking a real repository I use, whereas the switching branches case is
> something I don't care about as much at the moment.
> 
> Of course, I don't know how to fix it at the moment, and beggars can't
> be choosers.  :)

You're testing whether a checkout without index and with empty work tree
works, right?

In that case, the checkout needs to make sure that .gitattributes is
checked out (or at least respected) before all other files, and that is
exactly what the patch in pu does. [If I remember right that great
simplification patch you bisected as bad played a role there, unless I'm
mixing up threads...]

Michael

Re: Git no longer reads attributes from the index properly

[Junio C Hamano]

bdowning@lavos.net (Brian Downing) writes:

> However, that commit looks like it's solving a different problem
> entirely (supporting changing between two branches where .gitattributes
> exists in both cases) and happens to fix the no .gitattributes -> read
> from index regression at the same time.  I don't know enough about the
> guts to tell, but does this also fix the core problem of the regression
> (I assume something about trying to read from the wrong index, given the
> commit that broke it), or does it just happen to work around it?

Actually the commit solves both.

Notice that the second hunk of the patch to unpack-trees passes o->result
to the new git_attr_set_direction() function to tell it to read from the
new index, instead of reading from the wrong one.  In addition, by setting
the direction to CHECKOUT, it favors to read the attribute data from the
index over from the work tree.

Note that this is merely a "good enough" approximation and arguing that we
should only read from the in-index attributes during checkout (and read
only from work tree attributes during checkin) is futile.  Look at other
thread with Kristian Amlie for details.

diff --git a/unpack-trees.c b/unpack-trees.c
index e547282..661218c 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -105,6 +106,7 @@ static int check_updates(struct unpack_trees_options *o)
 		cnt = 0;
 	}
 
+	git_attr_set_direction(GIT_ATTR_CHECKOUT, &o->result);
 	for (i = 0; i < index->cache_nr; i++) {
 		struct cache_entry *ce = index->cache[i];