{"thread":{"id":"18428","subject":"Git no longer reads attributes from the index properly","startedAt":"2009-03-20T07:35:27Z","lastAt":"2009-03-20T10:16:45Z","messageCount":5,"participants":["Brian Downing","Junio C Hamano","Michael J Gruber"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"108664","messageId":"20090320073527.GC1037@lavos.net","threadId":"18428","inReplyTo":null,"subject":"Git no longer reads attributes from the index properly","fromName":"Brian Downing","fromEmail":"bdowning@lavos.net","sentAt":"2009-03-20T07:35:27Z","receivedAt":"2009-03-20T07:35:27Z","isPatch":false,"sender":{"key":"bdowning@lavos.net","avatar":"https://avatars.githubusercontent.com/u/366426?v=4"},"body":"\nAs of 34110cd4e394e3f92c01a4709689b384c34645d8, (2008-03-06, just over a\nyear ago), Git no longer reads attributes from the index properly in all\ncases. This breaks initial checkouts where .gitattribute information is\nrequired to get a good checkout. [I'm also copying the msysgit guys on\nthis, since they are probably the primary customers of autocrlf and the\ncrlf attributes, though the problem exists in core Git.]\n\nThis support for reading attributes from the index in addition to the\nworking directory was added in 1a9d7e9b484e77436edc7f5cacd39c24ec605e6d,\non 2007-08-14:\n\ncommit 1a9d7e9b484e77436edc7f5cacd39c24ec605e6d\nAuthor: Junio C Hamano \nDate: Tue Aug 14 01:41:02 2007 -0700\n\n attr.c: read .gitattributes from index as well.\n\n This makes .gitattributes files to be read from the index when\n they are not checked out to the work tree. This is in line with\n the way we always allowed low-level tools to operate in sparsely\n checked out work tree in a reasonable way.\n\n It swaps the order of new file creation and converting the blob\n to work tree representation; otherwise when we are in the middle\n of checking out .gitattributes we would notice an empty but\n unwritten .gitattributes file in the work tree and will ignore\n the copy in the index.\n\n Signed-off-by: Junio C Hamano \n\nThis worked until the following commit:\n\n34110cd4e394e3f92c01a4709689b384c34645d8 is first bad commit\ncommit 34110cd4e394e3f92c01a4709689b384c34645d8\nAuthor: Linus Torvalds \nDate: Thu Mar 6 18:12:28 2008 -0800\n\n Make 'unpack_trees()' have a separate source and destination index\n\n We will always unpack into our own internal index, but we will take the\n source from wherever specified, and we will optionally write the result\n to a specified index (optionally, because not everybody even _wants_ any\n result: the index diffing really wants to just walk the tree and index\n in parallel).\n\n This ends up removing a fair number more lines than it adds, for the\n simple reason that we can now skip all the crud that tried to be\n oh-so-careful about maintaining our position in the index as we were\n traversing and modifying it. Since we don't actually modify the source\n index any more, we can just update the 'o->pos' pointer without worrying\n about whether an index entry got removed or replaced or added to.\n\n Signed-off-by: Linus Torvalds \n Signed-off-by: Junio C Hamano \n\nUnfortunately I don't have the foggiest on how to fix this. However, I\ndid write a new test case to catch this bug, as the existing ones were\ninsufficiently destructive to trigger it. This test case is ugly, uses\nporcelain, and is probably overly-destructive, but it does show the\nproblem. It fails on 34110cd4 and succeeds on 34110cd4^.\n\nI don't expect the test case can be accepted as-is, but regardless:\n\nSigned-off-by: Brian Downing \n---\n t/t0020-crlf.sh | 19 +++++++++++++++++++\n 1 files changed, 19 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t0020-crlf.sh b/t/t0020-crlf.sh\nindex 1be7446..749cd09 100755\n--- a/t/t0020-crlf.sh\n+++ b/t/t0020-crlf.sh\n@@ -429,6 +429,25 @@ test_expect_success 'in-tree .gitattributes (4)' '\n \t}\n '\n \n+test_expect_success 'in-tree .gitattributes (5)' '\n+\n+\trm .git/index &&\n+\tgit clean -d -x -f &&\n+\tgit checkout &&\n+\n+\tif remove_cr one >/dev/null\n+\tthen\n+\t\techo \"Eh? one should not have CRLF\"\n+\t\tfalse\n+\telse\n+\t\t: happy\n+\tfi &&\n+\tremove_cr three >/dev/null || {\n+\t\techo \"Eh? three should still have CRLF\"\n+\t\tfalse\n+\t}\n+'\n+\n test_expect_success 'invalid .gitattributes (must not crash)' '\n \n \techo \"three +crlf\" >>.gitattributes &&\n-- \n1.5.6.3\n"},{"id":"108670","messageId":"7vab7gk39o.fsf@gitster.siamese.dyndns.org","threadId":"18428","inReplyTo":"20090320073527.GC1037@lavos.net","subject":"Re: Git no longer reads attributes from the index properly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-20T08:27:31Z","receivedAt":"2009-03-20T08:27:31Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"bdowning@lavos.net (Brian Downing) writes:\n\n> As of 34110cd4e394e3f92c01a4709689b384c34645d8, (2008-03-06, just over a\n> year ago), Git no longer reads attributes from the index properly in all\n> cases....\n\nPerhaps you would want to try it on 06f33c1 (Read attributes from the\nindex that is being checked out, 2009-03-13) that is part of 'pu'?\n"},{"id":"108673","messageId":"20090320084031.GD1037@lavos.net","threadId":"18428","inReplyTo":"7vab7gk39o.fsf@gitster.siamese.dyndns.org","subject":"Re: Git no longer reads attributes from the index properly","fromName":"Brian Downing","fromEmail":"bdowning@lavos.net","sentAt":"2009-03-20T08:40:31Z","receivedAt":"2009-03-20T08:40:31Z","isPatch":false,"sender":{"key":"bdowning@lavos.net","avatar":"https://avatars.githubusercontent.com/u/366426?v=4"},"body":"\nOn Fri, Mar 20, 2009 at 01:27:31AM -0700, Junio C Hamano wrote:\n> bdowning@lavos.net (Brian Downing) writes:\n> > As of 34110cd4e394e3f92c01a4709689b384c34645d8, (2008-03-06, just over a\n> > year ago), Git no longer reads attributes from the index properly in all\n> > cases....\n> \n> Perhaps you would want to try it on 06f33c1 (Read attributes from the\n> index that is being checked out, 2009-03-13) that is part of 'pu'?\n\nI only tried it on next, groan. Yes, it works there, thanks.\n\nHowever, that commit looks like it's solving a different problem\nentirely (supporting changing between two branches where .gitattributes\nexists in both cases) and happens to fix the no .gitattributes -> read\nfrom index regression at the same time. I don't know enough about the\nguts to tell, but does this also fix the core problem of the regression\n(I assume something about trying to read from the wrong index, given the\ncommit that broke it), or does it just happen to work around it?\n\nSpecifically, it would be nice to have a fix for the regression that\ncould land on maint relatively soon, as the initial checkout case is\nbreaking a real repository I use, whereas the switching branches case is\nsomething I don't care about as much at the moment.\n\nOf course, I don't know how to fix it at the moment, and beggars can't\nbe choosers. :)\n\n-bcd\n"},{"id":"108682","messageId":"49C365A2.5070607@drmicha.warpmail.net","threadId":"18428","inReplyTo":"20090320084031.GD1037@lavos.net","subject":"Re: Git no longer reads attributes from the index properly","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2009-03-20T09:45:06Z","receivedAt":"2009-03-20T09:45:06Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Brian Downing venit, vidit, dixit 20.03.2009 09:40:\n> \n> On Fri, Mar 20, 2009 at 01:27:31AM -0700, Junio C Hamano wrote:\n>> bdowning@lavos.net (Brian Downing) writes:\n>>> As of 34110cd4e394e3f92c01a4709689b384c34645d8, (2008-03-06, just over a\n>>> year ago), Git no longer reads attributes from the index properly in all\n>>> cases....\n>>\n>> Perhaps you would want to try it on 06f33c1 (Read attributes from the\n>> index that is being checked out, 2009-03-13) that is part of 'pu'?\n> \n> I only tried it on next, groan. Yes, it works there, thanks.\n> \n> However, that commit looks like it's solving a different problem\n> entirely (supporting changing between two branches where .gitattributes\n> exists in both cases) and happens to fix the no .gitattributes -> read\n> from index regression at the same time. I don't know enough about the\n> guts to tell, but does this also fix the core problem of the regression\n> (I assume something about trying to read from the wrong index, given the\n> commit that broke it), or does it just happen to work around it?\n> \n> Specifically, it would be nice to have a fix for the regression that\n> could land on maint relatively soon, as the initial checkout case is\n> breaking a real repository I use, whereas the switching branches case is\n> something I don't care about as much at the moment.\n> \n> Of course, I don't know how to fix it at the moment, and beggars can't\n> be choosers. :)\n\nYou're testing whether a checkout without index and with empty work tree\nworks, right?\n\nIn that case, the checkout needs to make sure that .gitattributes is\nchecked out (or at least respected) before all other files, and that is\nexactly what the patch in pu does. [If I remember right that great\nsimplification patch you bisected as bad played a role there, unless I'm\nmixing up threads...]\n\nMichael\n"},{"id":"108689","messageId":"7v4oxojy7m.fsf@gitster.siamese.dyndns.org","threadId":"18428","inReplyTo":"20090320084031.GD1037@lavos.net","subject":"Re: Git no longer reads attributes from the index properly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-20T10:16:45Z","receivedAt":"2009-03-20T10:16:45Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"bdowning@lavos.net (Brian Downing) writes:\n\n> However, that commit looks like it's solving a different problem\n> entirely (supporting changing between two branches where .gitattributes\n> exists in both cases) and happens to fix the no .gitattributes -> read\n> from index regression at the same time. I don't know enough about the\n> guts to tell, but does this also fix the core problem of the regression\n> (I assume something about trying to read from the wrong index, given the\n> commit that broke it), or does it just happen to work around it?\n\nActually the commit solves both.\n\nNotice that the second hunk of the patch to unpack-trees passes o->result\nto the new git_attr_set_direction() function to tell it to read from the\nnew index, instead of reading from the wrong one. In addition, by setting\nthe direction to CHECKOUT, it favors to read the attribute data from the\nindex over from the work tree.\n\nNote that this is merely a \"good enough\" approximation and arguing that we\nshould only read from the in-index attributes during checkout (and read\nonly from work tree attributes during checkin) is futile. Look at other\nthread with Kristian Amlie for details.\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex e547282..661218c 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -105,6 +106,7 @@ static int check_updates(struct unpack_trees_options *o)\n \t\tcnt = 0;\n \t}\n \n+\tgit_attr_set_direction(GIT_ATTR_CHECKOUT, &o->result);\n \tfor (i = 0; i < index->cache_nr; i++) {\n \t\tstruct cache_entry *ce = index->cache[i];\n \n"}]}