{"thread":{"id":"17417","subject":"Honoring a checked out gitattributes file","startedAt":"2009-01-28T15:25:37Z","lastAt":"2009-03-20T09:32:09Z","messageCount":22,"participants":["Kristian Amlie","Michael J Gruber","Jeff King","Matthieu Moy","Johannes Sixt","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"102315","messageId":"498078F1.20807@trolltech.com","threadId":"17417","inReplyTo":null,"subject":"Honoring a checked out gitattributes file","fromName":"Kristian Amlie","fromEmail":"kristian.amlie@trolltech.com","sentAt":"2009-01-28T15:25:37Z","receivedAt":"2009-01-28T15:25:37Z","isPatch":false,"sender":{"key":"kristian.amlie@trolltech.com","avatar":null},"body":"Hi!\n\nWe currently use msysGit in our company test farm to checkout the latest\ndevelopment branch and do autotests. However, we have one problem:\nCertain files require UNIX line endings, even though this is a Windows\nsystem. For this we use .gitattributes.\n\nHowever, if the .gitattributes file is also checked in to the branch, it\nwill not always be honored. I browsed the code a bit, and it seems to\nhappen whenever there is an existing .gitattributes file, but the\ncheckout adds new files to it. These new files will not get the correct\nline endings (although I'm not sure if it happens *every* time, it could\ndepend on the order they are checked out).\n\nI think this should be fairly straightforward to fix, by ensuring that\nif there is a file called .gitattributes in the index of the current\ndirectory, check it out first, before all the other files that may be\naffected by it. I can produce a patch and testcase for it, but I wanted\nto hear the opinions of some developers in case there is an obvious flaw\nin my solution.\n\nWhat do you think?\n\n--\nKristian Amlie\n"},{"id":"102322","messageId":"49808B53.6040907@drmicha.warpmail.net","threadId":"17417","inReplyTo":"498078F1.20807@trolltech.com","subject":"Re: Honoring a checked out gitattributes file","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2009-01-28T16:44:03Z","receivedAt":"2009-01-28T16:44:03Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Kristian Amlie venit, vidit, dixit 28.01.2009 16:25:\n> Hi!\n> \n> We currently use msysGit in our company test farm to checkout the latest\n> development branch and do autotests. However, we have one problem:\n> Certain files require UNIX line endings, even though this is a Windows\n> system. For this we use .gitattributes.\n> \n> However, if the .gitattributes file is also checked in to the branch, it\n> will not always be honored. I browsed the code a bit, and it seems to\n> happen whenever there is an existing .gitattributes file, but the\n> checkout adds new files to it. These new files will not get the correct\n> line endings (although I'm not sure if it happens *every* time, it could\n> depend on the order they are checked out).\n> \n> I think this should be fairly straightforward to fix, by ensuring that\n> if there is a file called .gitattributes in the index of the current\n> directory, check it out first, before all the other files that may be\n> affected by it. I can produce a patch and testcase for it, but I wanted\n> to hear the opinions of some developers in case there is an obvious flaw\n> in my solution.\n> \n> What do you think?\n\nI think there's a general time ordering problem. Say you do the\nfollowing commits:\n\nA: add a.txt\nB: add a .gitattributes file covering *.txt (say with crlf or any filter)\nC: add c.txt\n\nNow, with an empty dir, you do either\n\n1) checkout A, B, C sequentially\n2) checkout C\n\nThe contents of the checkout will be different in cases 1) and 2):\n1) a.txt is checked out out as is, c.txt according to the attributes\n2) with current git: probably like 1), with your suggestion: both a.txt\nand c.txt filtered according to the attributes.\n\nIf you add a file and .gitattributes covering it in the same commit\nthere is an ordering ambiguity which can be solved (patched away)\neasily, but I think the difference between 1) and 2) is still\nproblematic, and would need to be dealt with.\n\nThe main problem seems to be that changing a file like gitattributes can\npotentially change (by changing filters) the contents which should be\nstored for a different file even if the checkout of that file doesn't\nchange.\n\nCheers,\nMichael\n"},{"id":"102325","messageId":"498094F9.5070201@trolltech.com","threadId":"17417","inReplyTo":"49808B53.6040907@drmicha.warpmail.net","subject":"Re: Honoring a checked out gitattributes file","fromName":"Kristian Amlie","fromEmail":"kristian.amlie@trolltech.com","sentAt":"2009-01-28T17:25:13Z","receivedAt":"2009-01-28T17:25:13Z","isPatch":false,"sender":{"key":"kristian.amlie@trolltech.com","avatar":null},"body":"Michael J Gruber wrote:\n> I think there's a general time ordering problem. Say you do the\n> following commits:\n> \n> A: add a.txt\n> B: add a .gitattributes file covering *.txt (say with crlf or any filter)\n> C: add c.txt\n> \n> Now, with an empty dir, you do either\n> \n> 1) checkout A, B, C sequentially\n> 2) checkout C\n> \n> The contents of the checkout will be different in cases 1) and 2):\n> 1) a.txt is checked out out as is, c.txt according to the attributes\n> 2) with current git: probably like 1), with your suggestion: both a.txt\n> and c.txt filtered according to the attributes.\n> \n> If you add a file and .gitattributes covering it in the same commit\n> there is an ordering ambiguity which can be solved (patched away)\n> easily, but I think the difference between 1) and 2) is still\n> problematic, and would need to be dealt with.\n\nI agree.\n\n> \n> The main problem seems to be that changing a file like gitattributes can\n> potentially change (by changing filters) the contents which should be\n> stored for a different file even if the checkout of that file doesn't\n> change.\n\nYes, that is a problem. Ideally, the crlf attribute would be tied to the\nfile entry itself rather than a separate file (so changing the attribute\nwould mean a change to the file), but I guess we are stuck with what we\nhave.\n\nI still think that case 2) is the most common, and fixing it has the\nappealing property that if the repository line endings are broken for\nsome reason (because of case 1 or something else), then recloning or\nchecking out from scratch is guaranteed to bring the working tree into\nthe \"correct\" state.\n\nSince fixing both cases is a pretty big task and fixing only case 2 is\nsmall, I propose that we go ahead with that.\n\n--\nKristian Amlie\n"},{"id":"102327","messageId":"20090128175545.GE8863@coredump.intra.peff.net","threadId":"17417","inReplyTo":"498078F1.20807@trolltech.com","subject":"Re: Honoring a checked out gitattributes file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-01-28T17:55:45Z","receivedAt":"2009-01-28T17:55:45Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 28, 2009 at 04:25:37PM +0100, Kristian Amlie wrote:\n\n> However, if the .gitattributes file is also checked in to the branch, it\n> will not always be honored. I browsed the code a bit, and it seems to\n> happen whenever there is an existing .gitattributes file, but the\n> checkout adds new files to it. These new files will not get the correct\n> line endings (although I'm not sure if it happens *every* time, it could\n> depend on the order they are checked out).\n\nThis is a known limitation of gitattributes. There has been some\ndiscussion in the past on how it should work, but I don't recall the\nspecifics; try searching the list archive. I think it is really just\nwaiting for somebody to step up and write some patches.\n\nAs a workaround, you might be able to do something like:\n\n  branch=master\n  git show $branch:.gitattributes >.git/info/attributes\n  git checkout $branch\n\nwhich is very hacky, but might work depending on your setup. Notably it\nwill overwrite any actual use you were making of .git/info/attributes,\nand it will not respect any .gitattributes files in subdirectories.\n\n-Peff\n"},{"id":"102581","messageId":"1233320413-28069-1-git-send-email-kristian.amlie@trolltech.com","threadId":"17417","inReplyTo":"498094F9.5070201@trolltech.com","subject":"Re: Honoring a checked out gitattributes file","fromName":"Kristian Amlie","fromEmail":"kristian.amlie@trolltech.com","sentAt":"2009-01-30T13:00:12Z","receivedAt":"2009-01-30T13:00:12Z","isPatch":false,"sender":{"key":"kristian.amlie@trolltech.com","avatar":null},"body":"I have now created a test case which demonstrates the problem.\n\nI want to create a patch for it too, but I am unfortunately not so\nfamiliar with the Git source code.\n\nTo put it short, I want to make sure that when checking out a tree,\n.gitattributes will be checked out before all other files, so that\nfiles that are affected by it will be guaranteed to get the correct\nattributes. Maybe someone from this list could point me in the right\ndirection in the source code for something like this?\n\n--\nKristian Amlie\n"},{"id":"102580","messageId":"1233320413-28069-2-git-send-email-kristian.amlie@trolltech.com","threadId":"17417","inReplyTo":"1233320413-28069-1-git-send-email-kristian.amlie@trolltech.com","subject":"[PATCH] Add a test for checking whether gitattributes is honored by checkout.","fromName":"Kristian Amlie","fromEmail":"kristian.amlie@trolltech.com","sentAt":"2009-01-30T13:00:13Z","receivedAt":"2009-01-30T13:00:13Z","isPatch":true,"sender":{"key":"kristian.amlie@trolltech.com","avatar":null},"body":"The original bug will not honor new entries in gitattributes if they\nare changed in the same checkout as the files they affect.\n---\n t/t2012-checkout-crlf.sh |   29 +++++++++++++++++++++++++++++\n 1 files changed, 29 insertions(+), 0 deletions(-)\n create mode 100755 t/t2012-checkout-crlf.sh\n\ndiff --git a/t/t2012-checkout-crlf.sh b/t/t2012-checkout-crlf.sh\nnew file mode 100755\nindex 0000000..cb997a8\n--- /dev/null\n+++ b/t/t2012-checkout-crlf.sh\n@@ -0,0 +1,29 @@\n+#!/bin/sh\n+\n+test_description='checkout should honor .gitattributes'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\n+\tgit config core.autocrlf true &&\n+\tprintf \"dummy dummy=true\\r\\n\" > .gitattributes &&\n+\tgit add .gitattributes &&\n+\tgit commit -m initial &&\n+\tprintf \"file -crlf\\n\" >> .gitattributes &&\n+\tprintf \"contents\\n\" > file &&\n+\tgit add .gitattributes file &&\n+\tgit commit -m second\n+\n+'\n+\n+test_expect_success 'checkout with existing .gitattributes' '\n+\n+\tgit checkout master~1 &&\n+\tgit checkout master &&\n+\ttest \"$(git diff-files --raw)\" = \"\"\n+\n+'\n+\n+test_done\n+\n-- \n1.6.0.3\n"},{"id":"107789","messageId":"1236850575-27973-1-git-send-email-kristian.amlie@nokia.com","threadId":"17417","inReplyTo":"1233320413-28069-1-git-send-email-kristian.amlie@trolltech.com","subject":"Re: Honoring a checked out gitattributes file","fromName":"Kristian Amlie","fromEmail":"kristian.amlie@nokia.com","sentAt":"2009-03-12T09:36:13Z","receivedAt":"2009-03-12T09:36:13Z","isPatch":false,"sender":{"key":"kristian.amlie@nokia.com","avatar":null},"body":"\nOk, it took a bit longer than I thought to get around to this, but I\nfinally have a patch for the problem. I am not so familiar with the\nGit source code, so I apologize if I do something incredibly stupid.\n\nHopefully the commit message should explain what I do, but please ask\nquestions if it's not clear. I'm sure there's room for improvements.\n\n--\nKristian\n"},{"id":"107790","messageId":"1236850575-27973-2-git-send-email-kristian.amlie@nokia.com","threadId":"17417","inReplyTo":"1236850575-27973-1-git-send-email-kristian.amlie@nokia.com","subject":"[PATCH 1/2] Add a test for checking whether gitattributes is honored by checkout.","fromName":"Kristian Amlie","fromEmail":"kristian.amlie@nokia.com","sentAt":"2009-03-12T09:36:14Z","receivedAt":"2009-03-12T09:36:14Z","isPatch":true,"sender":{"key":"kristian.amlie@nokia.com","avatar":null},"body":"The original bug will not honor new entries in gitattributes if they\nare changed in the same checkout as the files they affect.\n---\n t/t2013-checkout-crlf.sh |   29 +++++++++++++++++++++++++++++\n 1 files changed, 29 insertions(+), 0 deletions(-)\n create mode 100755 t/t2013-checkout-crlf.sh\n\ndiff --git a/t/t2013-checkout-crlf.sh b/t/t2013-checkout-crlf.sh\nnew file mode 100755\nindex 0000000..cb997a8\n--- /dev/null\n+++ b/t/t2013-checkout-crlf.sh\n@@ -0,0 +1,29 @@\n+#!/bin/sh\n+\n+test_description='checkout should honor .gitattributes'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\n+\tgit config core.autocrlf true &&\n+\tprintf \"dummy dummy=true\\r\\n\" > .gitattributes &&\n+\tgit add .gitattributes &&\n+\tgit commit -m initial &&\n+\tprintf \"file -crlf\\n\" >> .gitattributes &&\n+\tprintf \"contents\\n\" > file &&\n+\tgit add .gitattributes file &&\n+\tgit commit -m second\n+\n+'\n+\n+test_expect_success 'checkout with existing .gitattributes' '\n+\n+\tgit checkout master~1 &&\n+\tgit checkout master &&\n+\ttest \"$(git diff-files --raw)\" = \"\"\n+\n+'\n+\n+test_done\n+\n-- \n1.6.2.105.g16bc7.dirty\n"},{"id":"107791","messageId":"1236850575-27973-3-git-send-email-kristian.amlie@nokia.com","threadId":"17417","inReplyTo":"1236850575-27973-2-git-send-email-kristian.amlie@nokia.com","subject":"[PATCH 2/2] Make Git respect changes to .gitattributes during checkout.","fromName":"Kristian Amlie","fromEmail":"kristian.amlie@nokia.com","sentAt":"2009-03-12T09:36:15Z","receivedAt":"2009-03-12T09:36:15Z","isPatch":true,"sender":{"key":"kristian.amlie@nokia.com","avatar":null},"body":"We do this by popping off elements on the attribute stack, until we\nreach the level where a new .gitattributes was checked out. The next\ntime someone calls git_checkattr(), it will reconstruct the\nattributes from that point.\n---\n attr.c  |   72 ++++++++++++++++++++++++++++++++++++++++++++++++++++----------\n attr.h  |    1 +\n entry.c |   23 ++++++++++++++++++++\n 3 files changed, 84 insertions(+), 12 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex 17f6a4d..7deb51a 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -455,6 +455,23 @@ static void bootstrap_attr_stack(void)\n \t}\n }\n \n+static void pop_attr_stack(const char *path, int dirlen)\n+{\n+\tstruct attr_stack *elem;\n+\twhile (attr_stack && attr_stack->origin) {\n+\t\tint namelen = strlen(attr_stack->origin);\n+\n+\t\telem = attr_stack;\n+\t\tif (namelen <= dirlen &&\n+\t\t\t!strncmp(elem->origin, path, namelen))\n+\t\t\tbreak;\n+\n+\t\tdebug_pop(elem);\n+\t\tattr_stack = elem->prev;\n+\t\tfree_attr_elem(elem);\n+\t}\n+}\n+\n static void prepare_attr_stack(const char *path, int dirlen)\n {\n \tstruct attr_stack *elem, *info;\n@@ -489,18 +506,7 @@ static void prepare_attr_stack(const char *path, int dirlen)\n \t * Pop the ones from directories that are not the prefix of\n \t * the path we are checking.\n \t */\n-\twhile (attr_stack && attr_stack->origin) {\n-\t\tint namelen = strlen(attr_stack->origin);\n-\n-\t\telem = attr_stack;\n-\t\tif (namelen <= dirlen &&\n-\t\t    !strncmp(elem->origin, path, namelen))\n-\t\t\tbreak;\n-\n-\t\tdebug_pop(elem);\n-\t\tattr_stack = elem->prev;\n-\t\tfree_attr_elem(elem);\n-\t}\n+\tpop_attr_stack(path, dirlen);\n \n \t/*\n \t * Read from parent directories and push them down\n@@ -642,3 +648,45 @@ int git_checkattr(const char *path, int num, struct git_attr_check *check)\n \n \treturn 0;\n }\n+\n+void git_attr_invalidate_path(const char *path)\n+{\n+\tint dirlen;\n+\tconst char *cp;\n+\tstruct attr_stack *info, *elem;\n+\n+\tbootstrap_attr_stack();\n+\n+\t/*\n+\t * Pop the \"info\" one that is always at the top of the stack.\n+\t */\n+\tinfo = attr_stack;\n+\tattr_stack = info->prev;\n+\n+\tcp = strrchr(path, '/');\n+\tdirlen = cp ? cp - path : 0;\n+\t/* Pop everything up to, and including, path. */\n+\tpop_attr_stack(path, dirlen);\n+\n+\tif (!strcmp(path, \"\") && attr_stack->origin && !strcmp(attr_stack->origin, \"\")) {\n+\t\t/* Special handling when the root attributes must be invalidated. */\n+\t\telem = attr_stack;\n+\t\tdebug_pop(elem);\n+\t\tattr_stack = elem->prev;\n+\t\tfree_attr_elem(elem);\n+\n+\t\tif (!is_bare_repository()) {\n+\t\t\telem = read_attr(GITATTRIBUTES_FILE, 1);\n+\t\t\telem->origin = strdup(\"\");\n+\t\t\telem->prev = attr_stack;\n+\t\t\tattr_stack = elem;\n+\t\t\tdebug_push(elem);\n+\t\t}\n+\t}\n+\n+\t/*\n+\t * Finally push the \"info\" one at the top of the stack.\n+\t */\n+\tinfo->prev = attr_stack;\n+\tattr_stack = info;\n+}\ndiff --git a/attr.h b/attr.h\nindex f1c2038..8f4135b 100644\n--- a/attr.h\n+++ b/attr.h\n@@ -30,5 +30,6 @@ struct git_attr_check {\n };\n \n int git_checkattr(const char *path, int, struct git_attr_check *);\n+void git_attr_invalidate_path(const char *path);\n \n #endif /* ATTR_H */\ndiff --git a/entry.c b/entry.c\nindex 05aa58d..121c979 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -1,6 +1,7 @@\n #include \"cache.h\"\n #include \"blob.h\"\n #include \"dir.h\"\n+#include \"attr.h\"\n \n static void create_directories(const char *path, const struct checkout *state)\n {\n@@ -91,6 +92,9 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout\n {\n \tint fd;\n \tlong wrote;\n+\tint gitattrlen;\n+\tint pathlen;\n+\tchar *inv_path;\n \n \tswitch (ce->ce_mode & S_IFMT) {\n \t\tchar *new;\n@@ -171,6 +175,25 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout\n \t\treturn error(\"git checkout-index: unknown file mode for %s\", path);\n \t}\n \n+\tgitattrlen = strlen(GITATTRIBUTES_FILE);\n+\tpathlen = strlen(path);\n+\tif (!strncmp(path + pathlen - gitattrlen, GITATTRIBUTES_FILE, gitattrlen)) {\n+\t\t/* Invalidate attributes if a new .gitattributes file was checked out. */\n+\t\tinv_path = strrchr(path, '/');\n+\t\tif (!inv_path) {\n+\t\t\tpathlen = 0;\n+\t\t\tinv_path = xmalloc(1);\n+\t\t\t*inv_path = '\\0';\n+\t\t} else {\n+\t\t\tpathlen = inv_path - path;\n+\t\t\tinv_path = xmalloc(pathlen + 1);\n+\t\t\tstrncpy(inv_path, path, pathlen);\n+\t\t\tinv_path[pathlen] = '\\0';\n+\t\t}\n+\t\tgit_attr_invalidate_path(inv_path);\n+\t\tfree(inv_path);\n+\t}\n+\n \tif (state->refresh_cache) {\n \t\tstruct stat st;\n \t\tlstat(ce->name, &st);\n-- \n1.6.2.105.g16bc7.dirty\n"},{"id":"107792","messageId":"vpqr613jcoa.fsf@bauges.imag.fr","threadId":"17417","inReplyTo":"1236850575-27973-2-git-send-email-kristian.amlie@nokia.com","subject":"Re: [PATCH 1/2] Add a test for checking whether gitattributes is honored by checkout.","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2009-03-12T09:47:01Z","receivedAt":"2009-03-12T09:47:01Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"Kristian Amlie <kristian.amlie@nokia.com> writes:\n\n> +test_expect_success 'setup' '\n\nIf you have two separate patches for test and bugfix, you probably\nwant the first to introduce test_expect_failure, and the second to\nchange it to test_expect_success. This way: 1) the test-suite passes\nfor all commits (and git-bisect will be your friend again), and 2) the\nsecond patch is self-explanatory about the bug it fixes.\n\n-- \nMatthieu\n"},{"id":"107793","messageId":"49B8DB81.7020100@nokia.com","threadId":"17417","inReplyTo":"vpqr613jcoa.fsf@bauges.imag.fr","subject":"Re: [PATCH 1/2] Add a test for checking whether gitattributes is honored by checkout.","fromName":"Kristian Amlie","fromEmail":"kristian.amlie@nokia.com","sentAt":"2009-03-12T09:53:05Z","receivedAt":"2009-03-12T09:53:05Z","isPatch":true,"sender":{"key":"kristian.amlie@nokia.com","avatar":null},"body":"ext Matthieu Moy wrote:\n> Kristian Amlie <kristian.amlie@nokia.com> writes:\n> \n>> +test_expect_success 'setup' '\n> \n> If you have two separate patches for test and bugfix, you probably\n> want the first to introduce test_expect_failure, and the second to\n> change it to test_expect_success. This way: 1) the test-suite passes\n> for all commits (and git-bisect will be your friend again), and 2) the\n> second patch is self-explanatory about the bug it fixes.\n\nGood point. I'll fix that in my next batch.\n\n--\nKristian\n"},{"id":"107794","messageId":"49B8DD1D.3060908@viscovery.net","threadId":"17417","inReplyTo":"1236850575-27973-3-git-send-email-kristian.amlie@nokia.com","subject":"Re: [PATCH 2/2] Make Git respect changes to .gitattributes during checkout.","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-03-12T09:59:57Z","receivedAt":"2009-03-12T09:59:57Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Kristian Amlie schrieb:\n> We do this by popping off elements on the attribute stack, until we\n> reach the level where a new .gitattributes was checked out. The next\n> time someone calls git_checkattr(), it will reconstruct the\n> attributes from that point.\n...\n> +\tgitattrlen = strlen(GITATTRIBUTES_FILE);\n> +\tpathlen = strlen(path);\n> +\tif (!strncmp(path + pathlen - gitattrlen, GITATTRIBUTES_FILE, gitattrlen)) {\n> +\t\t/* Invalidate attributes if a new .gitattributes file was checked out. */\n\nBut if there was file .abc.txt that was checked out before .gitattributes\nwas checked out, then the new .gitattributes won't have been used for\n.abc.txt, yet, right?\n\n(Disclaimer: I didn't read the code, just your description and this comment.)\n\n-- Hannes\n"},{"id":"107797","messageId":"49B8E29C.5080107@nokia.com","threadId":"17417","inReplyTo":"49B8DD1D.3060908@viscovery.net","subject":"Re: [PATCH 2/2] Make Git respect changes to .gitattributes during checkout.","fromName":"Kristian Amlie","fromEmail":"kristian.amlie@nokia.com","sentAt":"2009-03-12T10:23:24Z","receivedAt":"2009-03-12T10:23:24Z","isPatch":true,"sender":{"key":"kristian.amlie@nokia.com","avatar":null},"body":"ext Johannes Sixt wrote:\n> Kristian Amlie schrieb:\n>> We do this by popping off elements on the attribute stack, until we\n>> reach the level where a new .gitattributes was checked out. The next\n>> time someone calls git_checkattr(), it will reconstruct the\n>> attributes from that point.\n> ...\n>> +\tgitattrlen = strlen(GITATTRIBUTES_FILE);\n>> +\tpathlen = strlen(path);\n>> +\tif (!strncmp(path + pathlen - gitattrlen, GITATTRIBUTES_FILE, gitattrlen)) {\n>> +\t\t/* Invalidate attributes if a new .gitattributes file was checked out. */\n> \n> But if there was file .abc.txt that was checked out before .gitattributes\n> was checked out, then the new .gitattributes won't have been used for\n> .abc.txt, yet, right?\n\nYou're right, that fails.\n\nI'm guessing my initial patch (and testcase) works because the sorting\nis alphabetical and .gitattributes happens to be checked out before\nfiles with letters. I would need to make sure that .gitattributes gets\nchecked out first.\n\n--\nKristian\n"},{"id":"107941","messageId":"1236950656-1967-1-git-send-email-kristian.amlie@nokia.com","threadId":"17417","inReplyTo":"49B8DD1D.3060908@viscovery.net","subject":"Re: Honoring a checked out gitattributes file","fromName":"Kristian Amlie","fromEmail":"kristian.amlie@nokia.com","sentAt":"2009-03-13T13:24:14Z","receivedAt":"2009-03-13T13:24:14Z","isPatch":false,"sender":{"key":"kristian.amlie@nokia.com","avatar":null},"body":"\nOk, here's another round. I fixed the test case to expect failure, as\npointed out by Matthieu, and I also added code to make sure that\n.gitattributes gets checked out first.\n\nI also added a test case and some code to support the case where\n.gitattributes is removed in a commit, but this doesn't work properly\nyet (see commit message). I'm not sure how to solve this without\nresolving to ugly hacks like passing the new index_state* all the way\ndown to where git_checkattr gets called. If anybody has any\nsuggestions, do share.\n\nThe main usecase where .gitattributes is modified, is anyway covered\nby this patch.\n\n--\nKristian\n"},{"id":"107943","messageId":"1236950656-1967-2-git-send-email-kristian.amlie@nokia.com","threadId":"17417","inReplyTo":"1236950656-1967-1-git-send-email-kristian.amlie@nokia.com","subject":"[PATCH 1/2] Add a test for checking whether gitattributes is honored by checkout.","fromName":"Kristian Amlie","fromEmail":"kristian.amlie@nokia.com","sentAt":"2009-03-13T13:24:15Z","receivedAt":"2009-03-13T13:24:15Z","isPatch":true,"sender":{"key":"kristian.amlie@nokia.com","avatar":null},"body":"The original bug will not honor new entries in gitattributes if they\nare changed in the same checkout as the files they affect.\n\nIt will also keep using .gitattributes, even if it is deleted in the\nsame commit as the files it affects.\n---\n t/t2013-checkout-crlf.sh |   41 +++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 41 insertions(+), 0 deletions(-)\n create mode 100755 t/t2013-checkout-crlf.sh\n\ndiff --git a/t/t2013-checkout-crlf.sh b/t/t2013-checkout-crlf.sh\nnew file mode 100755\nindex 0000000..d9d6465\n--- /dev/null\n+++ b/t/t2013-checkout-crlf.sh\n@@ -0,0 +1,41 @@\n+#!/bin/sh\n+\n+test_description='checkout should honor .gitattributes'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\n+        git config core.autocrlf true &&\n+        printf \".file2 -crlf\\r\\n\" > .gitattributes &&\n+        git add .gitattributes &&\n+        git commit -m initial &&\n+        printf \".file -crlf\\n\" >> .gitattributes &&\n+        printf \"contents\\n\" > .file &&\n+        git add .gitattributes .file &&\n+        git commit -m second &&\n+        git rm .gitattributes &&\n+        printf \"contents\\r\\n\" > .file2 &&\n+        git add .file2 &&\n+        git commit -m third\n+\n+'\n+\n+test_expect_failure 'checkout with existing .gitattributes' '\n+\n+        git checkout master~2 &&\n+        git checkout master~1 &&\n+        test \"$(git diff-files --raw)\" = \"\"\n+\n+'\n+\n+test_expect_failure 'checkout when deleting .gitattributes' '\n+\n+        git checkout master~1 &&\n+        git checkout master &&\n+        test \"$(stat -c %s .file2)\" = \"10\"\n+\n+'\n+\n+test_done\n+\n-- \n1.6.1.1\n"},{"id":"107942","messageId":"1236950656-1967-3-git-send-email-kristian.amlie@nokia.com","threadId":"17417","inReplyTo":"1236950656-1967-2-git-send-email-kristian.amlie@nokia.com","subject":"[PATCH 2/2] Make Git respect changes to .gitattributes during checkout.","fromName":"Kristian Amlie","fromEmail":"kristian.amlie@nokia.com","sentAt":"2009-03-13T13:24:16Z","receivedAt":"2009-03-13T13:24:16Z","isPatch":true,"sender":{"key":"kristian.amlie@nokia.com","avatar":null},"body":"The fix is twofold:\n\nFirst, we force .gitattributes files to always be the first ones\nchecked out. This is the part in check_updates().\n\nSecond, we make sure that the checked out attributes get honored by\npopping off elements on the attribute stack, until we reach the level\nwhere a new .gitattributes was checked out. The next time someone\ncalls git_checkattr(), it will reconstruct the attributes from that\npoint.\n\nNote that in unlink_entry() there is code to support the case where\n.gitattributes is removed (test case #3 in t2013-checkout-crlf.sh),\nbut this doesn't work properly because Git tries to read new\nattributes from the index, but the old index (where .gitattributes\nstill exists) is still active.\n---\n attr.c                   |   86 +++++++++++++++++++++++++++++++++++++++------\n attr.h                   |    1 +\n entry.c                  |   19 ++++++++++\n t/t2013-checkout-crlf.sh |    2 +-\n unpack-trees.c           |   77 +++++++++++++++++++++++++++++++----------\n 5 files changed, 154 insertions(+), 31 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex 17f6a4d..2bc0847 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -455,6 +455,23 @@ static void bootstrap_attr_stack(void)\n \t}\n }\n \n+static void pop_attr_stack(const char *path, int dirlen)\n+{\n+\tstruct attr_stack *elem;\n+\twhile (attr_stack && attr_stack->origin) {\n+\t\tint namelen = strlen(attr_stack->origin);\n+\n+\t\telem = attr_stack;\n+\t\tif (namelen <= dirlen &&\n+\t\t\t!strncmp(elem->origin, path, namelen))\n+\t\t\tbreak;\n+\n+\t\tdebug_pop(elem);\n+\t\tattr_stack = elem->prev;\n+\t\tfree_attr_elem(elem);\n+\t}\n+}\n+\n static void prepare_attr_stack(const char *path, int dirlen)\n {\n \tstruct attr_stack *elem, *info;\n@@ -489,18 +506,7 @@ static void prepare_attr_stack(const char *path, int dirlen)\n \t * Pop the ones from directories that are not the prefix of\n \t * the path we are checking.\n \t */\n-\twhile (attr_stack && attr_stack->origin) {\n-\t\tint namelen = strlen(attr_stack->origin);\n-\n-\t\telem = attr_stack;\n-\t\tif (namelen <= dirlen &&\n-\t\t    !strncmp(elem->origin, path, namelen))\n-\t\t\tbreak;\n-\n-\t\tdebug_pop(elem);\n-\t\tattr_stack = elem->prev;\n-\t\tfree_attr_elem(elem);\n-\t}\n+\tpop_attr_stack(path, dirlen);\n \n \t/*\n \t * Read from parent directories and push them down\n@@ -642,3 +648,59 @@ int git_checkattr(const char *path, int num, struct git_attr_check *check)\n \n \treturn 0;\n }\n+\n+void git_attr_invalidate_path(const char *path)\n+{\n+\tint dirlen;\n+\tconst char *cp;\n+\tstruct attr_stack *info, *elem;\n+\n+\tbootstrap_attr_stack();\n+\n+\t/*\n+\t * Pop the \"info\" one that is always at the top of the stack.\n+\t */\n+\tinfo = attr_stack;\n+\tattr_stack = info->prev;\n+\n+\tcp = strrchr(path, '/');\n+\tdirlen = cp ? cp - path : 0;\n+\t/* Pop everything up to, and including, path. */\n+\tpop_attr_stack(path, dirlen);\n+\n+\tif (!strcmp(path, \"\") && attr_stack->origin && !strcmp(attr_stack->origin, \"\")) {\n+\t\t/*\n+\t\t * Special handling when the root attributes must be invalidated.\n+\t\t * Note that we cannot reread the attributes here. The reason is that\n+\t\t * when unlinking the root .gitattributes file, the index entry is\n+\t\t * removed after unlinking. If we reread the attributes here, we\n+\t\t * would get the same attributes as before because .gitattributes\n+\t\t * is still in the index. Thus we must wait until the next call to\n+\t\t * read_attr, when the index has been updated.\n+\t\t */\n+\t\t/* $GIT_DIR/info/attributes */\n+\t\telem = info;\n+\t\tdebug_pop(elem);\n+\t\tfree_attr_elem(elem);\n+\n+\t\t/* $GIT_WORK_TREE/.gitattributes */\n+\t\telem = attr_stack;\n+\t\tdebug_pop(elem);\n+\t\tattr_stack = elem->prev;\n+\t\tfree_attr_elem(elem);\n+\n+\t\t/* builtins */\n+\t\telem = attr_stack;\n+\t\tdebug_pop(elem);\n+\t\tattr_stack = elem->prev;\n+\t\tfree_attr_elem(elem);\n+\n+\t\tassert(!attr_stack);\n+\t} else {\n+\t\t/*\n+\t\t * Finally push the \"info\" one at the top of the stack.\n+\t\t */\n+\t\tinfo->prev = attr_stack;\n+\t\tattr_stack = info;\n+\t}\n+}\ndiff --git a/attr.h b/attr.h\nindex f1c2038..8f4135b 100644\n--- a/attr.h\n+++ b/attr.h\n@@ -30,5 +30,6 @@ struct git_attr_check {\n };\n \n int git_checkattr(const char *path, int, struct git_attr_check *);\n+void git_attr_invalidate_path(const char *path);\n \n #endif /* ATTR_H */\ndiff --git a/entry.c b/entry.c\nindex 05aa58d..7fb27fd 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -1,6 +1,7 @@\n #include \"cache.h\"\n #include \"blob.h\"\n #include \"dir.h\"\n+#include \"attr.h\"\n \n static void create_directories(const char *path, const struct checkout *state)\n {\n@@ -91,6 +92,9 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout\n {\n \tint fd;\n \tlong wrote;\n+\tint gitattrlen;\n+\tint pathlen;\n+\tchar *cp;\n \n \tswitch (ce->ce_mode & S_IFMT) {\n \t\tchar *new;\n@@ -171,6 +175,21 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout\n \t\treturn error(\"git checkout-index: unknown file mode for %s\", path);\n \t}\n \n+\tgitattrlen = strlen(GITATTRIBUTES_FILE);\n+\tpathlen = strlen(path);\n+\tif (pathlen >= gitattrlen && !strncmp(path + pathlen - gitattrlen,\n+\t\t\t\tGITATTRIBUTES_FILE, gitattrlen)) {\n+\t\t/* Invalidate attributes if a new .gitattributes file was checked out. */\n+\t\tcp = strrchr(path, '/');\n+\t\tif (!cp) {\n+\t\t\tgit_attr_invalidate_path(\"\");\n+\t\t} else {\n+\t\t\t*cp = '\\0';\n+\t\t\tgit_attr_invalidate_path(path);\n+\t\t\t*cp = '/';\n+\t\t}\n+\t}\n+\n \tif (state->refresh_cache) {\n \t\tstruct stat st;\n \t\tlstat(ce->name, &st);\ndiff --git a/t/t2013-checkout-crlf.sh b/t/t2013-checkout-crlf.sh\nindex d9d6465..e704c93 100755\n--- a/t/t2013-checkout-crlf.sh\n+++ b/t/t2013-checkout-crlf.sh\n@@ -21,7 +21,7 @@ test_expect_success 'setup' '\n \n '\n \n-test_expect_failure 'checkout with existing .gitattributes' '\n+test_expect_success 'checkout with existing .gitattributes' '\n \n         git checkout master~2 &&\n         git checkout master~1 &&\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex e547282..63a41f8 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -7,6 +7,7 @@\n #include \"unpack-trees.h\"\n #include \"progress.h\"\n #include \"refs.h\"\n+#include \"attr.h\"\n \n /*\n  * Error messages expected by scripts out of plumbing commands such as\n@@ -60,6 +61,7 @@ static void unlink_entry(struct cache_entry *ce)\n {\n \tchar *cp, *prev;\n \tchar *name = ce->name;\n+\tint namelen, gitattrlen;\n \n \tif (has_symlink_or_noent_leading_path(ce_namelen(ce), ce->name))\n \t\treturn;\n@@ -82,6 +84,21 @@ static void unlink_entry(struct cache_entry *ce)\n \t\t}\n \t\tprev = cp;\n \t}\n+\n+\tif (!prev) {\n+\t\tgitattrlen = strlen(GITATTRIBUTES_FILE);\n+\t\tnamelen = strlen(name);\n+\t\tif (namelen >= gitattrlen && !strncmp(name + namelen - gitattrlen,\n+\t\t\t\tGITATTRIBUTES_FILE, gitattrlen)) {\n+\t\t\tif (cp) {\n+\t\t\t\t*cp = '\\0';\n+\t\t\t\tgit_attr_invalidate_path(name);\n+\t\t\t\t*cp = '/';\n+\t\t\t} else {\n+\t\t\t\tgit_attr_invalidate_path(\"\");\n+\t\t\t}\n+\t\t}\n+\t}\n }\n \n static struct checkout state;\n@@ -92,6 +109,9 @@ static int check_updates(struct unpack_trees_options *o)\n \tstruct index_state *index = &o->result;\n \tint i;\n \tint errs = 0;\n+\tint attr;\n+\tint gitattrlen;\n+\tint namelen;\n \n \tif (o->update && o->verbose_update) {\n \t\tfor (total = cnt = 0; cnt < index->cache_nr; cnt++) {\n@@ -105,27 +125,48 @@ static int check_updates(struct unpack_trees_options *o)\n \t\tcnt = 0;\n \t}\n \n-\tfor (i = 0; i < index->cache_nr; i++) {\n-\t\tstruct cache_entry *ce = index->cache[i];\n+\tgitattrlen = strlen(GITATTRIBUTES_FILE);\n \n-\t\tif (ce->ce_flags & CE_REMOVE) {\n-\t\t\tdisplay_progress(progress, ++cnt);\n-\t\t\tif (o->update)\n-\t\t\t\tunlink_entry(ce);\n-\t\t\tremove_index_entry_at(&o->result, i);\n-\t\t\ti--;\n-\t\t\tcontinue;\n+\t/*\n+\t * We want to checkout .gitattributes before everything else. So:\n+\t * attr == 0 -> .gitattributes\n+\t * attr == 1 -> everything else\n+\t */\n+\tfor (attr = 0; attr < 2; attr++) {\n+\t\tfor (i = 0; i < index->cache_nr; i++) {\n+\t\t\tstruct cache_entry *ce = index->cache[i];\n+\n+\t\t\tnamelen = strlen(ce->name);\n+\t\t\tif ((attr == 0) != (namelen >= gitattrlen && strncmp(\n+\t\t\t\t\tce->name + namelen - gitattrlen,\n+\t\t\t\t\tGITATTRIBUTES_FILE, gitattrlen) == 0))\n+\t\t\t\tcontinue;\n+\t\t\tif (ce->ce_flags & CE_REMOVE) {\n+\t\t\t\tdisplay_progress(progress, ++cnt);\n+\t\t\t\tif (o->update)\n+\t\t\t\t\tunlink_entry(ce);\n+\t\t\t\tremove_index_entry_at(&o->result, i);\n+\t\t\t\ti--;\n+\t\t\t\tcontinue;\n+\t\t\t}\n \t\t}\n \t}\n \n-\tfor (i = 0; i < index->cache_nr; i++) {\n-\t\tstruct cache_entry *ce = index->cache[i];\n+\tfor (attr = 0; attr < 2; attr++) {\n+\t\tfor (i = 0; i < index->cache_nr; i++) {\n+\t\t\tstruct cache_entry *ce = index->cache[i];\n \n-\t\tif (ce->ce_flags & CE_UPDATE) {\n-\t\t\tdisplay_progress(progress, ++cnt);\n-\t\t\tce->ce_flags &= ~CE_UPDATE;\n-\t\t\tif (o->update) {\n-\t\t\t\terrs |= checkout_entry(ce, &state, NULL);\n+\t\t\tnamelen = strlen(ce->name);\n+\t\t\tif ((attr == 0) != (namelen >= gitattrlen && strncmp(\n+\t\t\t\t\tce->name + namelen - gitattrlen,\n+\t\t\t\t\tGITATTRIBUTES_FILE, gitattrlen) == 0))\n+\t\t\t\tcontinue;\n+\t\t\tif (ce->ce_flags & CE_UPDATE) {\n+\t\t\t\tdisplay_progress(progress, ++cnt);\n+\t\t\t\tce->ce_flags &= ~CE_UPDATE;\n+\t\t\t\tif (o->update) {\n+\t\t\t\t\terrs |= checkout_entry(ce, &state, NULL);\n+\t\t\t\t}\n \t\t\t}\n \t\t}\n \t}\n-- \n1.6.1.1\n"},{"id":"107993","messageId":"7vprgkg2kx.fsf@gitster.siamese.dyndns.org","threadId":"17417","inReplyTo":"1236950656-1967-3-git-send-email-kristian.amlie@nokia.com","subject":"Re: [PATCH 2/2] Make Git respect changes to .gitattributes during checkout.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-14T04:17:50Z","receivedAt":"2009-03-14T04:17:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kristian Amlie <kristian.amlie@nokia.com> writes:\n\n> The fix is twofold:\n>\n> First, we force .gitattributes files to always be the first ones\n> checked out. This is the part in check_updates().\n>\n> Second, we make sure that the checked out attributes get honored by\n> popping off elements on the attribute stack, until we reach the level\n> where a new .gitattributes was checked out. The next time someone\n> calls git_checkattr(), it will reconstruct the attributes from that\n> point.\n\nYikes.  The patch is too ugly beyond words.\n\nIn a well structured git program, we always read from the work tree and\nthe index (to see if there is something changed---you need to be able to\napply convert_to_git when you do this), internally compute what should\nhappen (e.g. decide that the new result needs to be checked out for a\npath), and then write it out (you apply convert_to_working_tree while you\ndo this).  So how about doing something like the attached patch?\n\nThe patch allows the caller to tell the usual \"read from the working tree,\nif not use the version from the index as the fallback\" logic to be swapped\naround, and take the index version.  It may or may not pass your new tests\n(I haven't even compile tested it), but I think the damage is minimized\ncompared to your version.\n\nIt is great that you are trying to fix this issue for the most obvious\n\"switching between two branches while not having a local change\" case, but\nfrankly, I do not think this is solvable in more general cases, and that\nis why it was kept \"known to be broken, not worth fixing\" state.\n\nFor example, you may notice that, after making a clean checkout, one path\nhas a wrong attribute assigned to it, and may try to correct it.  But how?\n\n $ edit .gitattributes ;# mark foo.dat as binary\n $ rm foo.dat\n $ git checkout foo.dat ;# make sure the new settings is correct???\n\nWithout this patch, this would have worked as expected, because we always\nuse the one from the work tree if available.\n\nWe have not \"git add\"ed .gitattributes yet because we would want to make\nsure the change is correct before doing so.  The patch takes attributes\nfor foo.dat from the old .gitattributes that is sitting in the index,\nwhich still has the wrong information, so in that sense, it is a\nregression.\n\nTo fix that, I _think_ you can further hack read_attr_from_index() to see\nif the attribute file is marked with CE_UPDATE, i.e. it is something we\nare checking out in this invocation of \"check_updates()\", and use it only\nif it is, or something like that, but there probably are more corner cases\nin which whichever one between the work tree and the index we take, it is\na wrong one.\n\nThere are other codepaths that call checkout_entry() that needs a\ntreatment similar to the one done to check_updates() by this patch, and I\nsuspect there are a few places that we do not even use checkout_entry().\nI think you can fairly easily wrap write_out_results() in builtin-apply.c\nin a similar way.  I do not know what merge-recursive does offhand, but I\nsuspect it would be a lot harder to fix.\n\nAnyway, here is the patch.\n\n attr.c         |   63 ++++++++++++++++++++++++++++++++++++++++++++-----------\n attr.h         |    6 +++++\n unpack-trees.c |    3 ++\n 3 files changed, 59 insertions(+), 13 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex 17f6a4d..72f6807 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -1,3 +1,4 @@\n+#define NO_THE_INDEX_COMPATIBILITY_MACROS\n #include \"cache.h\"\n #include \"attr.h\"\n \n@@ -318,6 +319,9 @@ static struct attr_stack *read_attr_from_array(const char **list)\n \treturn res;\n }\n \n+static enum git_attr_direction direction;\n+static struct index_state *use_index;\n+\n static struct attr_stack *read_attr_from_file(const char *path, int macro_ok)\n {\n \tFILE *fp = fopen(path, \"r\");\n@@ -340,9 +344,10 @@ static void *read_index_data(const char *path)\n \tunsigned long sz;\n \tenum object_type type;\n \tvoid *data;\n+\tstruct index_state *istate = use_index ? use_index : &the_index;\n \n \tlen = strlen(path);\n-\tpos = cache_name_pos(path, len);\n+\tpos = index_name_pos(istate, path, len);\n \tif (pos < 0) {\n \t\t/*\n \t\t * We might be in the middle of a merge, in which\n@@ -350,15 +355,15 @@ static void *read_index_data(const char *path)\n \t\t */\n \t\tint i;\n \t\tfor (i = -pos - 1;\n-\t\t     (pos < 0 && i < active_nr &&\n-\t\t      !strcmp(active_cache[i]->name, path));\n+\t\t     (pos < 0 && i < istate->cache_nr &&\n+\t\t      !strcmp(istate->cache[i]->name, path));\n \t\t     i++)\n-\t\t\tif (ce_stage(active_cache[i]) == 2)\n+\t\t\tif (ce_stage(istate->cache[i]) == 2)\n \t\t\t\tpos = i;\n \t}\n \tif (pos < 0)\n \t\treturn NULL;\n-\tdata = read_sha1_file(active_cache[pos]->sha1, &type, &sz);\n+\tdata = read_sha1_file(istate->cache[pos]->sha1, &type, &sz);\n \tif (!data || type != OBJ_BLOB) {\n \t\tfree(data);\n \t\treturn NULL;\n@@ -366,18 +371,12 @@ static void *read_index_data(const char *path)\n \treturn data;\n }\n \n-static struct attr_stack *read_attr(const char *path, int macro_ok)\n+static struct attr_stack *read_attr_from_index(const char *path, int macro_ok)\n {\n \tstruct attr_stack *res;\n \tchar *buf, *sp;\n \tint lineno = 0;\n \n-\tres = read_attr_from_file(path, macro_ok);\n-\tif (res)\n-\t\treturn res;\n-\n-\tres = xcalloc(1, sizeof(*res));\n-\n \t/*\n \t * There is no checked out .gitattributes file there, but\n \t * we might have it in the index.  We allow operation in a\n@@ -385,8 +384,9 @@ static struct attr_stack *read_attr(const char *path, int macro_ok)\n \t */\n \tbuf = read_index_data(path);\n \tif (!buf)\n-\t\treturn res;\n+\t\treturn NULL;\n \n+\tres = xcalloc(1, sizeof(*res));\n \tfor (sp = buf; *sp; ) {\n \t\tchar *ep;\n \t\tint more;\n@@ -401,6 +401,25 @@ static struct attr_stack *read_attr(const char *path, int macro_ok)\n \treturn res;\n }\n \n+static struct attr_stack *read_attr(const char *path, int macro_ok)\n+{\n+\tstruct attr_stack *res;\n+\n+\tif (direction == GIT_ATTR_CHECKOUT) {\n+\t\tres = read_attr_from_index(path, macro_ok);\n+\t\tif (!res)\n+\t\t\tres = read_attr_from_file(path, macro_ok);\n+\t}\n+\telse {\n+\t\tres = read_attr_from_file(path, macro_ok);\n+\t\tif (!res)\n+\t\t\tres = read_attr_from_index(path, macro_ok);\n+\t}\n+\tif (!res)\n+\t\tres = xcalloc(1, sizeof(*res));\n+\treturn res;\n+}\n+\n #if DEBUG_ATTR\n static void debug_info(const char *what, struct attr_stack *elem)\n {\n@@ -428,6 +447,15 @@ static void debug_set(const char *what, const char *match, struct git_attr *attr\n #define debug_set(a,b,c,d) do { ; } while (0)\n #endif\n \n+static void drop_attr_stack(void)\n+{\n+\twhile (attr_stack) {\n+\t\tstruct attr_stack *elem = attr_stack;\n+\t\tattr_stack = elem->prev;\n+\t\tfree_attr_elem(elem);\n+\t}\n+}\n+\n static void bootstrap_attr_stack(void)\n {\n \tif (!attr_stack) {\n@@ -642,3 +670,12 @@ int git_checkattr(const char *path, int num, struct git_attr_check *check)\n \n \treturn 0;\n }\n+\n+void git_attr_set_direction(enum git_attr_direction new, struct index_state *istate)\n+{\n+\tenum git_attr_direction old = direction;\n+\tdirection = new;\n+\tif (new != old)\n+\t\tdrop_attr_stack();\n+\tuse_index = istate;\n+}\ndiff --git a/attr.h b/attr.h\nindex f1c2038..3a2f4ec 100644\n--- a/attr.h\n+++ b/attr.h\n@@ -31,4 +31,10 @@ struct git_attr_check {\n \n int git_checkattr(const char *path, int, struct git_attr_check *);\n \n+enum git_attr_direction {\n+\tGIT_ATTR_CHECKIN,\n+\tGIT_ATTR_CHECKOUT\n+};\n+void git_attr_set_direction(enum git_attr_direction, struct index_state *);\n+\n #endif /* ATTR_H */\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@@ -7,6 +7,7 @@\n #include \"unpack-trees.h\"\n #include \"progress.h\"\n #include \"refs.h\"\n+#include \"attr.h\"\n \n /*\n  * Error messages expected by scripts out of plumbing commands such as\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@@ -130,6 +132,7 @@ static int check_updates(struct unpack_trees_options *o)\n \t\t}\n \t}\n \tstop_progress(&progress);\n+\tgit_attr_set_direction(GIT_ATTR_CHECKIN, NULL);\n \treturn errs != 0;\n }\n \n"},{"id":"107994","messageId":"7vljr8g1qe.fsf@gitster.siamese.dyndns.org","threadId":"17417","inReplyTo":"1236950656-1967-2-git-send-email-kristian.amlie@nokia.com","subject":"Re: [PATCH 1/2] Add a test for checking whether gitattributes is honored by checkout.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-14T04:36:09Z","receivedAt":"2009-03-14T04:36:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Please do not waste new tests for these.  I think they can be added to\nt0020-crlf.sh or t0021-conversion.sh.\n"},{"id":"108540","messageId":"49C267E3.1020205@nokia.com","threadId":"17417","inReplyTo":"7vprgkg2kx.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/2] Make Git respect changes to .gitattributes during checkout.","fromName":"Kristian Amlie","fromEmail":"kristian.amlie@nokia.com","sentAt":"2009-03-19T15:42:27Z","receivedAt":"2009-03-19T15:42:27Z","isPatch":true,"sender":{"key":"kristian.amlie@nokia.com","avatar":null},"body":"ext Junio C Hamano wrote:\n> In a well structured git program, we always read from the work tree and\n> the index (to see if there is something changed---you need to be able to\n> apply convert_to_git when you do this), internally compute what should\n> happen (e.g. decide that the new result needs to be checked out for a\n> path), and then write it out (you apply convert_to_working_tree while you\n> do this).  So how about doing something like the attached patch?\n> \n> The patch allows the caller to tell the usual \"read from the working tree,\n> if not use the version from the index as the fallback\" logic to be swapped\n> around, and take the index version.  It may or may not pass your new tests\n> (I haven't even compile tested it), but I think the damage is minimized\n> compared to your version.\n\nYou're right, that's a much cleaner approach. The patch is really good\nas far as I can see. It fixes both my test cases (including the one I\nwasn't able to fix), and there are no additional failures when running\nthe autotests. Do you have second thoughts about it, or is it good\nenough to apply?\n\nIf it is, I'll move the test case into t0021-conversion.sh like you said\nand give you a new patch for that.\n\n> It is great that you are trying to fix this issue for the most obvious\n> \"switching between two branches while not having a local change\" case, but\n> frankly, I do not think this is solvable in more general cases, and that\n> is why it was kept \"known to be broken, not worth fixing\" state.\n> \n> For example, you may notice that, after making a clean checkout, one path\n> has a wrong attribute assigned to it, and may try to correct it.  But how?\n> \n>  $ edit .gitattributes ;# mark foo.dat as binary\n>  $ rm foo.dat\n>  $ git checkout foo.dat ;# make sure the new settings is correct???\n\nAs far as I can see, this works without any modifications to the patch.\nIs that maybe because git_attr_set_direction() is not called if you use\nthat form of checkout?\n\n--\nKristian\n"},{"id":"108585","messageId":"7v8wn18bo2.fsf@gitster.siamese.dyndns.org","threadId":"17417","inReplyTo":"49C267E3.1020205@nokia.com","subject":"Re: [PATCH 2/2] Make Git respect changes to .gitattributes during checkout.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-19T21:06:53Z","receivedAt":"2009-03-19T21:06:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kristian Amlie <kristian.amlie@nokia.com> writes:\n\n> ext Junio C Hamano wrote:\n> ...\n>> For example, you may notice that, after making a clean checkout, one path\n>> has a wrong attribute assigned to it, and may try to correct it.  But how?\n>> \n>>  $ edit .gitattributes ;# mark foo.dat as binary\n>>  $ rm foo.dat\n>>  $ git checkout foo.dat ;# make sure the new settings is correct???\n>\n> As far as I can see, this works without any modifications to the patch.\n> Is that maybe because git_attr_set_direction() is not called if you use\n> that form of checkout?\n\nBut that in itself can be seen as a bug, right?  In another use case,\nsuppose you botched your .gitattributes in HEAD version and noticed that\nfoo.dat is checked out with a wrong attribute.  You try to fix it like\nthis:\n\n    $ git reset HEAD^ .gitattributes\n    $ rm foo.dat\n    $ git checkout foo.dat\n\nIf you do not flip the direction, the one from the work tree is used which\nis not what you want.  If you do, then you break the other use case.\n\nEither way, you cannot win.\n\nIn any case, I think I already queued the patch in 'pu' but without\ndocumentation updates nor additional tests, so no need to include the\npatch itself when you send in an update.\n"},{"id":"108669","messageId":"49C34FB1.4040100@nokia.com","threadId":"17417","inReplyTo":"7v8wn18bo2.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/2] Make Git respect changes to .gitattributes during checkout.","fromName":"Kristian Amlie","fromEmail":"kristian.amlie@nokia.com","sentAt":"2009-03-20T08:11:29Z","receivedAt":"2009-03-20T08:11:29Z","isPatch":true,"sender":{"key":"kristian.amlie@nokia.com","avatar":null},"body":"ext Junio C Hamano wrote:\n> Kristian Amlie <kristian.amlie@nokia.com> writes:\n> \n>> ext Junio C Hamano wrote:\n>> ...\n>>> For example, you may notice that, after making a clean checkout, one path\n>>> has a wrong attribute assigned to it, and may try to correct it.  But how?\n>>>\n>>>  $ edit .gitattributes ;# mark foo.dat as binary\n>>>  $ rm foo.dat\n>>>  $ git checkout foo.dat ;# make sure the new settings is correct???\n>> As far as I can see, this works without any modifications to the patch.\n>> Is that maybe because git_attr_set_direction() is not called if you use\n>> that form of checkout?\n> \n> But that in itself can be seen as a bug, right?  In another use case,\n> suppose you botched your .gitattributes in HEAD version and noticed that\n> foo.dat is checked out with a wrong attribute.  You try to fix it like\n> this:\n> \n>     $ git reset HEAD^ .gitattributes\n>     $ rm foo.dat\n>     $ git checkout foo.dat\n> \n> If you do not flip the direction, the one from the work tree is used which\n> is not what you want.  If you do, then you break the other use case.\n\nRight, I didn't even think about that case. My idea of gitattributes was\nthat the working tree copy is always the master version, and takes\nprecedence. The only reason that the index takes precedence in the \"git\ncheckout <branch>\" case is that there is no other way to get it checked\nout correctly, so I see this as an implementation detail. I'm sure some\npeople would disagree though.\n\nBut you're right, there is no way to make both cases correct. From my\nstandpoint I'd say that the behavior of your patch is the most intuitive.\n\nI'll follow up with the test case.\n\n--\nKristian\n"},{"id":"108679","messageId":"1237541529-29477-1-git-send-email-kristian.amlie@nokia.com","threadId":"17417","inReplyTo":"49C34FB1.4040100@nokia.com","subject":"[PATCH] Add a test for checking whether gitattributes is honored by checkout.","fromName":"Kristian Amlie","fromEmail":"kristian.amlie@nokia.com","sentAt":"2009-03-20T09:32:09Z","receivedAt":"2009-03-20T09:32:09Z","isPatch":true,"sender":{"key":"kristian.amlie@nokia.com","avatar":null},"body":"The original bug will not honor new entries in gitattributes if they\nare changed in the same checkout as the files they affect.\n\nIt will also keep using .gitattributes, even if it is deleted in the\nsame commit as the files it affects.\n---\n t/t0020-crlf.sh |   31 +++++++++++++++++++++++++++++++\n 1 files changed, 31 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t0020-crlf.sh b/t/t0020-crlf.sh\nindex 1be7446..4e72b53 100755\n--- a/t/t0020-crlf.sh\n+++ b/t/t0020-crlf.sh\n@@ -429,6 +429,37 @@ test_expect_success 'in-tree .gitattributes (4)' '\n \t}\n '\n \n+test_expect_success 'checkout with existing .gitattributes' '\n+\n+\tgit config core.autocrlf true &&\n+\tgit config --unset core.safecrlf &&\n+\techo \".file2 -crlfQ\" | q_to_cr >> .gitattributes &&\n+\tgit add .gitattributes &&\n+\tgit commit -m initial &&\n+\techo \".file -crlfQ\" | q_to_cr >> .gitattributes &&\n+\techo \"contents\" > .file &&\n+\tgit add .gitattributes .file &&\n+\tgit commit -m second &&\n+\n+\tgit checkout master~1 &&\n+\tgit checkout master &&\n+\ttest \"$(git diff-files --raw)\" = \"\"\n+\n+'\n+\n+test_expect_success 'checkout when deleting .gitattributes' '\n+\n+\tgit rm .gitattributes &&\n+\techo \"contentsQ\" | q_to_cr > .file2 &&\n+\tgit add .file2 &&\n+\tgit commit -m third\n+\n+\tgit checkout master~1 &&\n+\tgit checkout master &&\n+\tremove_cr .file2 >/dev/null\n+\n+'\n+\n test_expect_success 'invalid .gitattributes (must not crash)' '\n \n \techo \"three +crlf\" >>.gitattributes &&\n-- \n1.6.2.1.222.g570cc.dirty\n"}]}