Honoring a checked out gitattributes file

Honoring a checked out gitattributes file

[Kristian Amlie]

Hi!

We currently use msysGit in our company test farm to checkout the latest
development branch and do autotests. However, we have one problem:
Certain files require UNIX line endings, even though this is a Windows
system. For this we use .gitattributes.

However, if the .gitattributes file is also checked in to the branch, it
will not always be honored. I browsed the code a bit, and it seems to
happen whenever there is an existing .gitattributes file, but the
checkout adds new files to it. These new files will not get the correct
line endings (although I'm not sure if it happens *every* time, it could
depend on the order they are checked out).

I think this should be fairly straightforward to fix, by ensuring that
if there is a file called .gitattributes in the index of the current
directory, check it out first, before all the other files that may be
affected by it. I can produce a patch and testcase for it, but I wanted
to hear the opinions of some developers in case there is an obvious flaw
in my solution.

What do you think?

--
Kristian Amlie

Re: Honoring a checked out gitattributes file

[Michael J Gruber]

Kristian Amlie venit, vidit, dixit 28.01.2009 16:25:
> Hi!
> 
> We currently use msysGit in our company test farm to checkout the latest
> development branch and do autotests. However, we have one problem:
> Certain files require UNIX line endings, even though this is a Windows
> system. For this we use .gitattributes.
> 
> However, if the .gitattributes file is also checked in to the branch, it
> will not always be honored. I browsed the code a bit, and it seems to
> happen whenever there is an existing .gitattributes file, but the
> checkout adds new files to it. These new files will not get the correct
> line endings (although I'm not sure if it happens *every* time, it could
> depend on the order they are checked out).
> 
> I think this should be fairly straightforward to fix, by ensuring that
> if there is a file called .gitattributes in the index of the current
> directory, check it out first, before all the other files that may be
> affected by it. I can produce a patch and testcase for it, but I wanted
> to hear the opinions of some developers in case there is an obvious flaw
> in my solution.
> 
> What do you think?

I think there's a general time ordering problem. Say you do the
following commits:

A: add a.txt
B: add a .gitattributes file covering *.txt (say with crlf or any filter)
C: add c.txt

Now, with an empty dir, you do either

1) checkout A, B, C sequentially
2) checkout C

The contents of the checkout will be different in cases 1) and 2):
1) a.txt is checked out out as is, c.txt according to the attributes
2) with current git: probably like 1), with your suggestion: both a.txt
and c.txt filtered according to the attributes.

If you add a file and .gitattributes covering it in the same commit
there is an ordering ambiguity which can be solved (patched away)
easily, but I think the difference between 1) and 2) is still
problematic, and would need to be dealt with.

The main problem seems to be that changing a file like gitattributes can
potentially change (by changing filters) the contents which should be
stored for a different file even if the checkout of that file doesn't
change.

Cheers,
Michael

Re: Honoring a checked out gitattributes file

[Kristian Amlie]

Michael J Gruber wrote:
> I think there's a general time ordering problem. Say you do the
> following commits:
> 
> A: add a.txt
> B: add a .gitattributes file covering *.txt (say with crlf or any filter)
> C: add c.txt
> 
> Now, with an empty dir, you do either
> 
> 1) checkout A, B, C sequentially
> 2) checkout C
> 
> The contents of the checkout will be different in cases 1) and 2):
> 1) a.txt is checked out out as is, c.txt according to the attributes
> 2) with current git: probably like 1), with your suggestion: both a.txt
> and c.txt filtered according to the attributes.
> 
> If you add a file and .gitattributes covering it in the same commit
> there is an ordering ambiguity which can be solved (patched away)
> easily, but I think the difference between 1) and 2) is still
> problematic, and would need to be dealt with.

I agree.

> 
> The main problem seems to be that changing a file like gitattributes can
> potentially change (by changing filters) the contents which should be
> stored for a different file even if the checkout of that file doesn't
> change.

Yes, that is a problem. Ideally, the crlf attribute would be tied to the
file entry itself rather than a separate file (so changing the attribute
would mean a change to the file), but I guess we are stuck with what we
have.

I still think that case 2) is the most common, and fixing it has the
appealing property that if the repository line endings are broken for
some reason (because of case 1 or something else), then recloning or
checking out from scratch is guaranteed to bring the working tree into
the "correct" state.

Since fixing both cases is a pretty big task and fixing only case 2 is
small, I propose that we go ahead with that.

--
Kristian Amlie

Re: Honoring a checked out gitattributes file

[Jeff King]

On Wed, Jan 28, 2009 at 04:25:37PM +0100, Kristian Amlie wrote:

> However, if the .gitattributes file is also checked in to the branch, it
> will not always be honored. I browsed the code a bit, and it seems to
> happen whenever there is an existing .gitattributes file, but the
> checkout adds new files to it. These new files will not get the correct
> line endings (although I'm not sure if it happens *every* time, it could
> depend on the order they are checked out).

This is a known limitation of gitattributes. There has been some
discussion in the past on how it should work, but I don't recall the
specifics; try searching the list archive. I think it is really just
waiting for somebody to step up and write some patches.

As a workaround, you might be able to do something like:

  branch=master
  git show $branch:.gitattributes >.git/info/attributes
  git checkout $branch

which is very hacky, but might work depending on your setup. Notably it
will overwrite any actual use you were making of .git/info/attributes,
and it will not respect any .gitattributes files in subdirectories.

-Peff

Re: Honoring a checked out gitattributes file

[Kristian Amlie]

I have now created a test case which demonstrates the problem.

I want to create a patch for it too, but I am unfortunately not so
familiar with the Git source code.

To put it short, I want to make sure that when checking out a tree,
.gitattributes will be checked out before all other files, so that
files that are affected by it will be guaranteed to get the correct
attributes. Maybe someone from this list could point me in the right
direction in the source code for something like this?

--
Kristian Amlie

[PATCH] Add a test for checking whether gitattributes is honored by checkout.

[Kristian Amlie]

The original bug will not honor new entries in gitattributes if they
are changed in the same checkout as the files they affect.
---
 t/t2012-checkout-crlf.sh |   29 +++++++++++++++++++++++++++++
 1 files changed, 29 insertions(+), 0 deletions(-)
 create mode 100755 t/t2012-checkout-crlf.sh

diff --git a/t/t2012-checkout-crlf.sh b/t/t2012-checkout-crlf.sh
new file mode 100755
index 0000000..cb997a8
--- /dev/null
+++ b/t/t2012-checkout-crlf.sh
@@ -0,0 +1,29 @@
+#!/bin/sh
+
+test_description='checkout should honor .gitattributes'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+
+	git config core.autocrlf true &&
+	printf "dummy dummy=true\r\n" > .gitattributes &&
+	git add .gitattributes &&
+	git commit -m initial &&
+	printf "file -crlf\n" >> .gitattributes &&
+	printf "contents\n" > file &&
+	git add .gitattributes file &&
+	git commit -m second
+
+'
+
+test_expect_success 'checkout with existing .gitattributes' '
+
+	git checkout master~1 &&
+	git checkout master &&
+	test "$(git diff-files --raw)" = ""
+
+'
+
+test_done
+
-- 
1.6.0.3

Re: Honoring a checked out gitattributes file

[Kristian Amlie]

Ok, it took a bit longer than I thought to get around to this, but I
finally have a patch for the problem. I am not so familiar with the
Git source code, so I apologize if I do something incredibly stupid.

Hopefully the commit message should explain what I do, but please ask
questions if it's not clear. I'm sure there's room for improvements.

--
Kristian

[PATCH 1/2] Add a test for checking whether gitattributes is honored by checkout.

[Kristian Amlie]

The original bug will not honor new entries in gitattributes if they
are changed in the same checkout as the files they affect.
---
 t/t2013-checkout-crlf.sh |   29 +++++++++++++++++++++++++++++
 1 files changed, 29 insertions(+), 0 deletions(-)
 create mode 100755 t/t2013-checkout-crlf.sh

diff --git a/t/t2013-checkout-crlf.sh b/t/t2013-checkout-crlf.sh
new file mode 100755
index 0000000..cb997a8
--- /dev/null
+++ b/t/t2013-checkout-crlf.sh
@@ -0,0 +1,29 @@
+#!/bin/sh
+
+test_description='checkout should honor .gitattributes'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+
+	git config core.autocrlf true &&
+	printf "dummy dummy=true\r\n" > .gitattributes &&
+	git add .gitattributes &&
+	git commit -m initial &&
+	printf "file -crlf\n" >> .gitattributes &&
+	printf "contents\n" > file &&
+	git add .gitattributes file &&
+	git commit -m second
+
+'
+
+test_expect_success 'checkout with existing .gitattributes' '
+
+	git checkout master~1 &&
+	git checkout master &&
+	test "$(git diff-files --raw)" = ""
+
+'
+
+test_done
+
-- 
1.6.2.105.g16bc7.dirty

[PATCH 2/2] Make Git respect changes to .gitattributes during checkout.

[Kristian Amlie]

We do this by popping off elements on the attribute stack, until we
reach the level where a new .gitattributes was checked out. The next
time someone calls git_checkattr(), it will reconstruct the
attributes from that point.
---
 attr.c  |   72 ++++++++++++++++++++++++++++++++++++++++++++++++++++----------
 attr.h  |    1 +
 entry.c |   23 ++++++++++++++++++++
 3 files changed, 84 insertions(+), 12 deletions(-)

diff --git a/attr.c b/attr.c
index 17f6a4d..7deb51a 100644
--- a/attr.c
+++ b/attr.c
@@ -455,6 +455,23 @@ static void bootstrap_attr_stack(void)
 	}
 }
 
+static void pop_attr_stack(const char *path, int dirlen)
+{
+	struct attr_stack *elem;
+	while (attr_stack && attr_stack->origin) {
+		int namelen = strlen(attr_stack->origin);
+
+		elem = attr_stack;
+		if (namelen <= dirlen &&
+			!strncmp(elem->origin, path, namelen))
+			break;
+
+		debug_pop(elem);
+		attr_stack = elem->prev;
+		free_attr_elem(elem);
+	}
+}
+
 static void prepare_attr_stack(const char *path, int dirlen)
 {
 	struct attr_stack *elem, *info;
@@ -489,18 +506,7 @@ static void prepare_attr_stack(const char *path, int dirlen)
 	 * Pop the ones from directories that are not the prefix of
 	 * the path we are checking.
 	 */
-	while (attr_stack && attr_stack->origin) {
-		int namelen = strlen(attr_stack->origin);
-
-		elem = attr_stack;
-		if (namelen <= dirlen &&
-		    !strncmp(elem->origin, path, namelen))
-			break;
-
-		debug_pop(elem);
-		attr_stack = elem->prev;
-		free_attr_elem(elem);
-	}
+	pop_attr_stack(path, dirlen);
 
 	/*
 	 * Read from parent directories and push them down
@@ -642,3 +648,45 @@ int git_checkattr(const char *path, int num, struct git_attr_check *check)
 
 	return 0;
 }
+
+void git_attr_invalidate_path(const char *path)
+{
+	int dirlen;
+	const char *cp;
+	struct attr_stack *info, *elem;
+
+	bootstrap_attr_stack();
+
+	/*
+	 * Pop the "info" one that is always at the top of the stack.
+	 */
+	info = attr_stack;
+	attr_stack = info->prev;
+
+	cp = strrchr(path, '/');
+	dirlen = cp ? cp - path : 0;
+	/* Pop everything up to, and including, path. */
+	pop_attr_stack(path, dirlen);
+
+	if (!strcmp(path, "") && attr_stack->origin && !strcmp(attr_stack->origin, "")) {
+		/* Special handling when the root attributes must be invalidated. */
+		elem = attr_stack;
+		debug_pop(elem);
+		attr_stack = elem->prev;
+		free_attr_elem(elem);
+
+		if (!is_bare_repository()) {
+			elem = read_attr(GITATTRIBUTES_FILE, 1);
+			elem->origin = strdup("");
+			elem->prev = attr_stack;
+			attr_stack = elem;
+			debug_push(elem);
+		}
+	}
+
+	/*
+	 * Finally push the "info" one at the top of the stack.
+	 */
+	info->prev = attr_stack;
+	attr_stack = info;
+}
diff --git a/attr.h b/attr.h
index f1c2038..8f4135b 100644
--- a/attr.h
+++ b/attr.h
@@ -30,5 +30,6 @@ struct git_attr_check {
 };
 
 int git_checkattr(const char *path, int, struct git_attr_check *);
+void git_attr_invalidate_path(const char *path);
 
 #endif /* ATTR_H */
diff --git a/entry.c b/entry.c
index 05aa58d..121c979 100644
--- a/entry.c
+++ b/entry.c
@@ -1,6 +1,7 @@
 #include "cache.h"
 #include "blob.h"
 #include "dir.h"
+#include "attr.h"
 
 static void create_directories(const char *path, const struct checkout *state)
 {
@@ -91,6 +92,9 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout
 {
 	int fd;
 	long wrote;
+	int gitattrlen;
+	int pathlen;
+	char *inv_path;
 
 	switch (ce->ce_mode & S_IFMT) {
 		char *new;
@@ -171,6 +175,25 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout
 		return error("git checkout-index: unknown file mode for %s", path);
 	}
 
+	gitattrlen = strlen(GITATTRIBUTES_FILE);
+	pathlen = strlen(path);
+	if (!strncmp(path + pathlen - gitattrlen, GITATTRIBUTES_FILE, gitattrlen)) {
+		/* Invalidate attributes if a new .gitattributes file was checked out. */
+		inv_path = strrchr(path, '/');
+		if (!inv_path) {
+			pathlen = 0;
+			inv_path = xmalloc(1);
+			*inv_path = '\0';
+		} else {
+			pathlen = inv_path - path;
+			inv_path = xmalloc(pathlen + 1);
+			strncpy(inv_path, path, pathlen);
+			inv_path[pathlen] = '\0';
+		}
+		git_attr_invalidate_path(inv_path);
+		free(inv_path);
+	}
+
 	if (state->refresh_cache) {
 		struct stat st;
 		lstat(ce->name, &st);
-- 
1.6.2.105.g16bc7.dirty

Re: [PATCH 1/2] Add a test for checking whether gitattributes is honored by checkout.

[Matthieu Moy]

Kristian Amlie <kristian.amlie@nokia.com> writes:

> +test_expect_success 'setup' '

If you have two separate patches for test and bugfix, you probably
want the first to introduce test_expect_failure, and the second to
change it to test_expect_success. This way: 1) the test-suite passes
for all commits (and git-bisect will be your friend again), and 2) the
second patch is self-explanatory about the bug it fixes.

-- 
Matthieu

Re: [PATCH 1/2] Add a test for checking whether gitattributes is honored by checkout.

[Kristian Amlie]

ext Matthieu Moy wrote:
> Kristian Amlie <kristian.amlie@nokia.com> writes:
> 
>> +test_expect_success 'setup' '
> 
> If you have two separate patches for test and bugfix, you probably
> want the first to introduce test_expect_failure, and the second to
> change it to test_expect_success. This way: 1) the test-suite passes
> for all commits (and git-bisect will be your friend again), and 2) the
> second patch is self-explanatory about the bug it fixes.

Good point. I'll fix that in my next batch.

--
Kristian

Re: [PATCH 2/2] Make Git respect changes to .gitattributes during checkout.

[Johannes Sixt]

Kristian Amlie schrieb:
> We do this by popping off elements on the attribute stack, until we
> reach the level where a new .gitattributes was checked out. The next
> time someone calls git_checkattr(), it will reconstruct the
> attributes from that point.
...
> +	gitattrlen = strlen(GITATTRIBUTES_FILE);
> +	pathlen = strlen(path);
> +	if (!strncmp(path + pathlen - gitattrlen, GITATTRIBUTES_FILE, gitattrlen)) {
> +		/* Invalidate attributes if a new .gitattributes file was checked out. */

But if there was file .abc.txt that was checked out before .gitattributes
was checked out, then the new .gitattributes won't have been used for
.abc.txt, yet, right?

(Disclaimer: I didn't read the code, just your description and this comment.)

-- Hannes

Re: [PATCH 2/2] Make Git respect changes to .gitattributes during checkout.

[Kristian Amlie]

ext Johannes Sixt wrote:
> Kristian Amlie schrieb:
>> We do this by popping off elements on the attribute stack, until we
>> reach the level where a new .gitattributes was checked out. The next
>> time someone calls git_checkattr(), it will reconstruct the
>> attributes from that point.
> ...
>> +	gitattrlen = strlen(GITATTRIBUTES_FILE);
>> +	pathlen = strlen(path);
>> +	if (!strncmp(path + pathlen - gitattrlen, GITATTRIBUTES_FILE, gitattrlen)) {
>> +		/* Invalidate attributes if a new .gitattributes file was checked out. */
> 
> But if there was file .abc.txt that was checked out before .gitattributes
> was checked out, then the new .gitattributes won't have been used for
> .abc.txt, yet, right?

You're right, that fails.

I'm guessing my initial patch (and testcase) works because the sorting
is alphabetical and .gitattributes happens to be checked out before
files with letters. I would need to make sure that .gitattributes gets
checked out first.

--
Kristian

Re: Honoring a checked out gitattributes file

[Kristian Amlie]

Ok, here's another round. I fixed the test case to expect failure, as
pointed out by Matthieu, and I also added code to make sure that
.gitattributes gets checked out first.

I also added a test case and some code to support the case where
.gitattributes is removed in a commit, but this doesn't work properly
yet (see commit message). I'm not sure how to solve this without
resolving to ugly hacks like passing the new index_state* all the way
down to where git_checkattr gets called. If anybody has any
suggestions, do share.

The main usecase where .gitattributes is modified, is anyway covered
by this patch.

--
Kristian

[PATCH 1/2] Add a test for checking whether gitattributes is honored by checkout.

[Kristian Amlie]

The original bug will not honor new entries in gitattributes if they
are changed in the same checkout as the files they affect.

It will also keep using .gitattributes, even if it is deleted in the
same commit as the files it affects.
---
 t/t2013-checkout-crlf.sh |   41 +++++++++++++++++++++++++++++++++++++++++
 1 files changed, 41 insertions(+), 0 deletions(-)
 create mode 100755 t/t2013-checkout-crlf.sh

diff --git a/t/t2013-checkout-crlf.sh b/t/t2013-checkout-crlf.sh
new file mode 100755
index 0000000..d9d6465
--- /dev/null
+++ b/t/t2013-checkout-crlf.sh
@@ -0,0 +1,41 @@
+#!/bin/sh
+
+test_description='checkout should honor .gitattributes'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+
+        git config core.autocrlf true &&
+        printf ".file2 -crlf\r\n" > .gitattributes &&
+        git add .gitattributes &&
+        git commit -m initial &&
+        printf ".file -crlf\n" >> .gitattributes &&
+        printf "contents\n" > .file &&
+        git add .gitattributes .file &&
+        git commit -m second &&
+        git rm .gitattributes &&
+        printf "contents\r\n" > .file2 &&
+        git add .file2 &&
+        git commit -m third
+
+'
+
+test_expect_failure 'checkout with existing .gitattributes' '
+
+        git checkout master~2 &&
+        git checkout master~1 &&
+        test "$(git diff-files --raw)" = ""
+
+'
+
+test_expect_failure 'checkout when deleting .gitattributes' '
+
+        git checkout master~1 &&
+        git checkout master &&
+        test "$(stat -c %s .file2)" = "10"
+
+'
+
+test_done
+
-- 
1.6.1.1

[PATCH 2/2] Make Git respect changes to .gitattributes during checkout.

[Kristian Amlie]

The fix is twofold:

First, we force .gitattributes files to always be the first ones
checked out. This is the part in check_updates().

Second, we make sure that the checked out attributes get honored by
popping off elements on the attribute stack, until we reach the level
where a new .gitattributes was checked out. The next time someone
calls git_checkattr(), it will reconstruct the attributes from that
point.

Note that in unlink_entry() there is code to support the case where
.gitattributes is removed (test case #3 in t2013-checkout-crlf.sh),
but this doesn't work properly because Git tries to read new
attributes from the index, but the old index (where .gitattributes
still exists) is still active.
---
 attr.c                   |   86 +++++++++++++++++++++++++++++++++++++++------
 attr.h                   |    1 +
 entry.c                  |   19 ++++++++++
 t/t2013-checkout-crlf.sh |    2 +-
 unpack-trees.c           |   77 +++++++++++++++++++++++++++++++----------
 5 files changed, 154 insertions(+), 31 deletions(-)

diff --git a/attr.c b/attr.c
index 17f6a4d..2bc0847 100644
--- a/attr.c
+++ b/attr.c
@@ -455,6 +455,23 @@ static void bootstrap_attr_stack(void)
 	}
 }
 
+static void pop_attr_stack(const char *path, int dirlen)
+{
+	struct attr_stack *elem;
+	while (attr_stack && attr_stack->origin) {
+		int namelen = strlen(attr_stack->origin);
+
+		elem = attr_stack;
+		if (namelen <= dirlen &&
+			!strncmp(elem->origin, path, namelen))
+			break;
+
+		debug_pop(elem);
+		attr_stack = elem->prev;
+		free_attr_elem(elem);
+	}
+}
+
 static void prepare_attr_stack(const char *path, int dirlen)
 {
 	struct attr_stack *elem, *info;
@@ -489,18 +506,7 @@ static void prepare_attr_stack(const char *path, int dirlen)
 	 * Pop the ones from directories that are not the prefix of
 	 * the path we are checking.
 	 */
-	while (attr_stack && attr_stack->origin) {
-		int namelen = strlen(attr_stack->origin);
-
-		elem = attr_stack;
-		if (namelen <= dirlen &&
-		    !strncmp(elem->origin, path, namelen))
-			break;
-
-		debug_pop(elem);
-		attr_stack = elem->prev;
-		free_attr_elem(elem);
-	}
+	pop_attr_stack(path, dirlen);
 
 	/*
 	 * Read from parent directories and push them down
@@ -642,3 +648,59 @@ int git_checkattr(const char *path, int num, struct git_attr_check *check)
 
 	return 0;
 }
+
+void git_attr_invalidate_path(const char *path)
+{
+	int dirlen;
+	const char *cp;
+	struct attr_stack *info, *elem;
+
+	bootstrap_attr_stack();
+
+	/*
+	 * Pop the "info" one that is always at the top of the stack.
+	 */
+	info = attr_stack;
+	attr_stack = info->prev;
+
+	cp = strrchr(path, '/');
+	dirlen = cp ? cp - path : 0;
+	/* Pop everything up to, and including, path. */
+	pop_attr_stack(path, dirlen);
+
+	if (!strcmp(path, "") && attr_stack->origin && !strcmp(attr_stack->origin, "")) {
+		/*
+		 * Special handling when the root attributes must be invalidated.
+		 * Note that we cannot reread the attributes here. The reason is that
+		 * when unlinking the root .gitattributes file, the index entry is
+		 * removed after unlinking. If we reread the attributes here, we
+		 * would get the same attributes as before because .gitattributes
+		 * is still in the index. Thus we must wait until the next call to
+		 * read_attr, when the index has been updated.
+		 */
+		/* $GIT_DIR/info/attributes */
+		elem = info;
+		debug_pop(elem);
+		free_attr_elem(elem);
+
+		/* $GIT_WORK_TREE/.gitattributes */
+		elem = attr_stack;
+		debug_pop(elem);
+		attr_stack = elem->prev;
+		free_attr_elem(elem);
+
+		/* builtins */
+		elem = attr_stack;
+		debug_pop(elem);
+		attr_stack = elem->prev;
+		free_attr_elem(elem);
+
+		assert(!attr_stack);
+	} else {
+		/*
+		 * Finally push the "info" one at the top of the stack.
+		 */
+		info->prev = attr_stack;
+		attr_stack = info;
+	}
+}
diff --git a/attr.h b/attr.h
index f1c2038..8f4135b 100644
--- a/attr.h
+++ b/attr.h
@@ -30,5 +30,6 @@ struct git_attr_check {
 };
 
 int git_checkattr(const char *path, int, struct git_attr_check *);
+void git_attr_invalidate_path(const char *path);
 
 #endif /* ATTR_H */
diff --git a/entry.c b/entry.c
index 05aa58d..7fb27fd 100644
--- a/entry.c
+++ b/entry.c
@@ -1,6 +1,7 @@
 #include "cache.h"
 #include "blob.h"
 #include "dir.h"
+#include "attr.h"
 
 static void create_directories(const char *path, const struct checkout *state)
 {
@@ -91,6 +92,9 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout
 {
 	int fd;
 	long wrote;
+	int gitattrlen;
+	int pathlen;
+	char *cp;
 
 	switch (ce->ce_mode & S_IFMT) {
 		char *new;
@@ -171,6 +175,21 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout
 		return error("git checkout-index: unknown file mode for %s", path);
 	}
 
+	gitattrlen = strlen(GITATTRIBUTES_FILE);
+	pathlen = strlen(path);
+	if (pathlen >= gitattrlen && !strncmp(path + pathlen - gitattrlen,
+				GITATTRIBUTES_FILE, gitattrlen)) {
+		/* Invalidate attributes if a new .gitattributes file was checked out. */
+		cp = strrchr(path, '/');
+		if (!cp) {
+			git_attr_invalidate_path("");
+		} else {
+			*cp = '\0';
+			git_attr_invalidate_path(path);
+			*cp = '/';
+		}
+	}
+
 	if (state->refresh_cache) {
 		struct stat st;
 		lstat(ce->name, &st);
diff --git a/t/t2013-checkout-crlf.sh b/t/t2013-checkout-crlf.sh
index d9d6465..e704c93 100755
--- a/t/t2013-checkout-crlf.sh
+++ b/t/t2013-checkout-crlf.sh
@@ -21,7 +21,7 @@ test_expect_success 'setup' '
 
 '
 
-test_expect_failure 'checkout with existing .gitattributes' '
+test_expect_success 'checkout with existing .gitattributes' '
 
         git checkout master~2 &&
         git checkout master~1 &&
diff --git a/unpack-trees.c b/unpack-trees.c
index e547282..63a41f8 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -7,6 +7,7 @@
 #include "unpack-trees.h"
 #include "progress.h"
 #include "refs.h"
+#include "attr.h"
 
 /*
  * Error messages expected by scripts out of plumbing commands such as
@@ -60,6 +61,7 @@ static void unlink_entry(struct cache_entry *ce)
 {
 	char *cp, *prev;
 	char *name = ce->name;
+	int namelen, gitattrlen;
 
 	if (has_symlink_or_noent_leading_path(ce_namelen(ce), ce->name))
 		return;
@@ -82,6 +84,21 @@ static void unlink_entry(struct cache_entry *ce)
 		}
 		prev = cp;
 	}
+
+	if (!prev) {
+		gitattrlen = strlen(GITATTRIBUTES_FILE);
+		namelen = strlen(name);
+		if (namelen >= gitattrlen && !strncmp(name + namelen - gitattrlen,
+				GITATTRIBUTES_FILE, gitattrlen)) {
+			if (cp) {
+				*cp = '\0';
+				git_attr_invalidate_path(name);
+				*cp = '/';
+			} else {
+				git_attr_invalidate_path("");
+			}
+		}
+	}
 }
 
 static struct checkout state;
@@ -92,6 +109,9 @@ static int check_updates(struct unpack_trees_options *o)
 	struct index_state *index = &o->result;
 	int i;
 	int errs = 0;
+	int attr;
+	int gitattrlen;
+	int namelen;
 
 	if (o->update && o->verbose_update) {
 		for (total = cnt = 0; cnt < index->cache_nr; cnt++) {
@@ -105,27 +125,48 @@ static int check_updates(struct unpack_trees_options *o)
 		cnt = 0;
 	}
 
-	for (i = 0; i < index->cache_nr; i++) {
-		struct cache_entry *ce = index->cache[i];
+	gitattrlen = strlen(GITATTRIBUTES_FILE);
 
-		if (ce->ce_flags & CE_REMOVE) {
-			display_progress(progress, ++cnt);
-			if (o->update)
-				unlink_entry(ce);
-			remove_index_entry_at(&o->result, i);
-			i--;
-			continue;
+	/*
+	 * We want to checkout .gitattributes before everything else. So:
+	 * attr == 0 -> .gitattributes
+	 * attr == 1 -> everything else
+	 */
+	for (attr = 0; attr < 2; attr++) {
+		for (i = 0; i < index->cache_nr; i++) {
+			struct cache_entry *ce = index->cache[i];
+
+			namelen = strlen(ce->name);
+			if ((attr == 0) != (namelen >= gitattrlen && strncmp(
+					ce->name + namelen - gitattrlen,
+					GITATTRIBUTES_FILE, gitattrlen) == 0))
+				continue;
+			if (ce->ce_flags & CE_REMOVE) {
+				display_progress(progress, ++cnt);
+				if (o->update)
+					unlink_entry(ce);
+				remove_index_entry_at(&o->result, i);
+				i--;
+				continue;
+			}
 		}
 	}
 
-	for (i = 0; i < index->cache_nr; i++) {
-		struct cache_entry *ce = index->cache[i];
+	for (attr = 0; attr < 2; attr++) {
+		for (i = 0; i < index->cache_nr; i++) {
+			struct cache_entry *ce = index->cache[i];
 
-		if (ce->ce_flags & CE_UPDATE) {
-			display_progress(progress, ++cnt);
-			ce->ce_flags &= ~CE_UPDATE;
-			if (o->update) {
-				errs |= checkout_entry(ce, &state, NULL);
+			namelen = strlen(ce->name);
+			if ((attr == 0) != (namelen >= gitattrlen && strncmp(
+					ce->name + namelen - gitattrlen,
+					GITATTRIBUTES_FILE, gitattrlen) == 0))
+				continue;
+			if (ce->ce_flags & CE_UPDATE) {
+				display_progress(progress, ++cnt);
+				ce->ce_flags &= ~CE_UPDATE;
+				if (o->update) {
+					errs |= checkout_entry(ce, &state, NULL);
+				}
 			}
 		}
 	}
-- 
1.6.1.1

Re: [PATCH 2/2] Make Git respect changes to .gitattributes during checkout.

[Junio C Hamano]

Kristian Amlie <kristian.amlie@nokia.com> writes:

> The fix is twofold:
>
> First, we force .gitattributes files to always be the first ones
> checked out. This is the part in check_updates().
>
> Second, we make sure that the checked out attributes get honored by
> popping off elements on the attribute stack, until we reach the level
> where a new .gitattributes was checked out. The next time someone
> calls git_checkattr(), it will reconstruct the attributes from that
> point.

Yikes.  The patch is too ugly beyond words.

In a well structured git program, we always read from the work tree and
the index (to see if there is something changed---you need to be able to
apply convert_to_git when you do this), internally compute what should
happen (e.g. decide that the new result needs to be checked out for a
path), and then write it out (you apply convert_to_working_tree while you
do this).  So how about doing something like the attached patch?

The patch allows the caller to tell the usual "read from the working tree,
if not use the version from the index as the fallback" logic to be swapped
around, and take the index version.  It may or may not pass your new tests
(I haven't even compile tested it), but I think the damage is minimized
compared to your version.

It is great that you are trying to fix this issue for the most obvious
"switching between two branches while not having a local change" case, but
frankly, I do not think this is solvable in more general cases, and that
is why it was kept "known to be broken, not worth fixing" state.

For example, you may notice that, after making a clean checkout, one path
has a wrong attribute assigned to it, and may try to correct it.  But how?

 $ edit .gitattributes ;# mark foo.dat as binary
 $ rm foo.dat
 $ git checkout foo.dat ;# make sure the new settings is correct???

Without this patch, this would have worked as expected, because we always
use the one from the work tree if available.

We have not "git add"ed .gitattributes yet because we would want to make
sure the change is correct before doing so.  The patch takes attributes
for foo.dat from the old .gitattributes that is sitting in the index,
which still has the wrong information, so in that sense, it is a
regression.

To fix that, I _think_ you can further hack read_attr_from_index() to see
if the attribute file is marked with CE_UPDATE, i.e. it is something we
are checking out in this invocation of "check_updates()", and use it only
if it is, or something like that, but there probably are more corner cases
in which whichever one between the work tree and the index we take, it is
a wrong one.

There are other codepaths that call checkout_entry() that needs a
treatment similar to the one done to check_updates() by this patch, and I
suspect there are a few places that we do not even use checkout_entry().
I think you can fairly easily wrap write_out_results() in builtin-apply.c
in a similar way.  I do not know what merge-recursive does offhand, but I
suspect it would be a lot harder to fix.

Anyway, here is the patch.

 attr.c         |   63 ++++++++++++++++++++++++++++++++++++++++++++-----------
 attr.h         |    6 +++++
 unpack-trees.c |    3 ++
 3 files changed, 59 insertions(+), 13 deletions(-)

diff --git a/attr.c b/attr.c
index 17f6a4d..72f6807 100644
--- a/attr.c
+++ b/attr.c
@@ -1,3 +1,4 @@
+#define NO_THE_INDEX_COMPATIBILITY_MACROS
 #include "cache.h"
 #include "attr.h"
 
@@ -318,6 +319,9 @@ static struct attr_stack *read_attr_from_array(const char **list)
 	return res;
 }
 
+static enum git_attr_direction direction;
+static struct index_state *use_index;
+
 static struct attr_stack *read_attr_from_file(const char *path, int macro_ok)
 {
 	FILE *fp = fopen(path, "r");
@@ -340,9 +344,10 @@ static void *read_index_data(const char *path)
 	unsigned long sz;
 	enum object_type type;
 	void *data;
+	struct index_state *istate = use_index ? use_index : &the_index;
 
 	len = strlen(path);
-	pos = cache_name_pos(path, len);
+	pos = index_name_pos(istate, path, len);
 	if (pos < 0) {
 		/*
 		 * We might be in the middle of a merge, in which
@@ -350,15 +355,15 @@ static void *read_index_data(const char *path)
 		 */
 		int i;
 		for (i = -pos - 1;
-		     (pos < 0 && i < active_nr &&
-		      !strcmp(active_cache[i]->name, path));
+		     (pos < 0 && i < istate->cache_nr &&
+		      !strcmp(istate->cache[i]->name, path));
 		     i++)
-			if (ce_stage(active_cache[i]) == 2)
+			if (ce_stage(istate->cache[i]) == 2)
 				pos = i;
 	}
 	if (pos < 0)
 		return NULL;
-	data = read_sha1_file(active_cache[pos]->sha1, &type, &sz);
+	data = read_sha1_file(istate->cache[pos]->sha1, &type, &sz);
 	if (!data || type != OBJ_BLOB) {
 		free(data);
 		return NULL;
@@ -366,18 +371,12 @@ static void *read_index_data(const char *path)
 	return data;
 }
 
-static struct attr_stack *read_attr(const char *path, int macro_ok)
+static struct attr_stack *read_attr_from_index(const char *path, int macro_ok)
 {
 	struct attr_stack *res;
 	char *buf, *sp;
 	int lineno = 0;
 
-	res = read_attr_from_file(path, macro_ok);
-	if (res)
-		return res;
-
-	res = xcalloc(1, sizeof(*res));
-
 	/*
 	 * There is no checked out .gitattributes file there, but
 	 * we might have it in the index.  We allow operation in a
@@ -385,8 +384,9 @@ static struct attr_stack *read_attr(const char *path, int macro_ok)
 	 */
 	buf = read_index_data(path);
 	if (!buf)
-		return res;
+		return NULL;
 
+	res = xcalloc(1, sizeof(*res));
 	for (sp = buf; *sp; ) {
 		char *ep;
 		int more;
@@ -401,6 +401,25 @@ static struct attr_stack *read_attr(const char *path, int macro_ok)
 	return res;
 }
 
+static struct attr_stack *read_attr(const char *path, int macro_ok)
+{
+	struct attr_stack *res;
+
+	if (direction == GIT_ATTR_CHECKOUT) {
+		res = read_attr_from_index(path, macro_ok);
+		if (!res)
+			res = read_attr_from_file(path, macro_ok);
+	}
+	else {
+		res = read_attr_from_file(path, macro_ok);
+		if (!res)
+			res = read_attr_from_index(path, macro_ok);
+	}
+	if (!res)
+		res = xcalloc(1, sizeof(*res));
+	return res;
+}
+
 #if DEBUG_ATTR
 static void debug_info(const char *what, struct attr_stack *elem)
 {
@@ -428,6 +447,15 @@ static void debug_set(const char *what, const char *match, struct git_attr *attr
 #define debug_set(a,b,c,d) do { ; } while (0)
 #endif
 
+static void drop_attr_stack(void)
+{
+	while (attr_stack) {
+		struct attr_stack *elem = attr_stack;
+		attr_stack = elem->prev;
+		free_attr_elem(elem);
+	}
+}
+
 static void bootstrap_attr_stack(void)
 {
 	if (!attr_stack) {
@@ -642,3 +670,12 @@ int git_checkattr(const char *path, int num, struct git_attr_check *check)
 
 	return 0;
 }
+
+void git_attr_set_direction(enum git_attr_direction new, struct index_state *istate)
+{
+	enum git_attr_direction old = direction;
+	direction = new;
+	if (new != old)
+		drop_attr_stack();
+	use_index = istate;
+}
diff --git a/attr.h b/attr.h
index f1c2038..3a2f4ec 100644
--- a/attr.h
+++ b/attr.h
@@ -31,4 +31,10 @@ struct git_attr_check {
 
 int git_checkattr(const char *path, int, struct git_attr_check *);
 
+enum git_attr_direction {
+	GIT_ATTR_CHECKIN,
+	GIT_ATTR_CHECKOUT
+};
+void git_attr_set_direction(enum git_attr_direction, struct index_state *);
+
 #endif /* ATTR_H */
diff --git a/unpack-trees.c b/unpack-trees.c
index e547282..661218c 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -7,6 +7,7 @@
 #include "unpack-trees.h"
 #include "progress.h"
 #include "refs.h"
+#include "attr.h"
 
 /*
  * Error messages expected by scripts out of plumbing commands such as
@@ -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];
 
@@ -130,6 +132,7 @@ static int check_updates(struct unpack_trees_options *o)
 		}
 	}
 	stop_progress(&progress);
+	git_attr_set_direction(GIT_ATTR_CHECKIN, NULL);
 	return errs != 0;
 }
 

Re: [PATCH 1/2] Add a test for checking whether gitattributes is honored by checkout.

[Junio C Hamano]

Please do not waste new tests for these.  I think they can be added to
t0020-crlf.sh or t0021-conversion.sh.

Re: [PATCH 2/2] Make Git respect changes to .gitattributes during checkout.

[Kristian Amlie]

ext Junio C Hamano wrote:
> In a well structured git program, we always read from the work tree and
> the index (to see if there is something changed---you need to be able to
> apply convert_to_git when you do this), internally compute what should
> happen (e.g. decide that the new result needs to be checked out for a
> path), and then write it out (you apply convert_to_working_tree while you
> do this).  So how about doing something like the attached patch?
> 
> The patch allows the caller to tell the usual "read from the working tree,
> if not use the version from the index as the fallback" logic to be swapped
> around, and take the index version.  It may or may not pass your new tests
> (I haven't even compile tested it), but I think the damage is minimized
> compared to your version.

You're right, that's a much cleaner approach. The patch is really good
as far as I can see. It fixes both my test cases (including the one I
wasn't able to fix), and there are no additional failures when running
the autotests. Do you have second thoughts about it, or is it good
enough to apply?

If it is, I'll move the test case into t0021-conversion.sh like you said
and give you a new patch for that.

> It is great that you are trying to fix this issue for the most obvious
> "switching between two branches while not having a local change" case, but
> frankly, I do not think this is solvable in more general cases, and that
> is why it was kept "known to be broken, not worth fixing" state.
> 
> For example, you may notice that, after making a clean checkout, one path
> has a wrong attribute assigned to it, and may try to correct it.  But how?
> 
>  $ edit .gitattributes ;# mark foo.dat as binary
>  $ rm foo.dat
>  $ git checkout foo.dat ;# make sure the new settings is correct???

As far as I can see, this works without any modifications to the patch.
Is that maybe because git_attr_set_direction() is not called if you use
that form of checkout?

--
Kristian

Re: [PATCH 2/2] Make Git respect changes to .gitattributes during checkout.

[Junio C Hamano]

Kristian Amlie <kristian.amlie@nokia.com> writes:

> ext Junio C Hamano wrote:
> ...
>> For example, you may notice that, after making a clean checkout, one path
>> has a wrong attribute assigned to it, and may try to correct it.  But how?
>> 
>>  $ edit .gitattributes ;# mark foo.dat as binary
>>  $ rm foo.dat
>>  $ git checkout foo.dat ;# make sure the new settings is correct???
>
> As far as I can see, this works without any modifications to the patch.
> Is that maybe because git_attr_set_direction() is not called if you use
> that form of checkout?

But that in itself can be seen as a bug, right?  In another use case,
suppose you botched your .gitattributes in HEAD version and noticed that
foo.dat is checked out with a wrong attribute.  You try to fix it like
this:

    $ git reset HEAD^ .gitattributes
    $ rm foo.dat
    $ git checkout foo.dat

If you do not flip the direction, the one from the work tree is used which
is not what you want.  If you do, then you break the other use case.

Either way, you cannot win.

In any case, I think I already queued the patch in 'pu' but without
documentation updates nor additional tests, so no need to include the
patch itself when you send in an update.

Re: [PATCH 2/2] Make Git respect changes to .gitattributes during checkout.

[Kristian Amlie]

ext Junio C Hamano wrote:
> Kristian Amlie <kristian.amlie@nokia.com> writes:
> 
>> ext Junio C Hamano wrote:
>> ...
>>> For example, you may notice that, after making a clean checkout, one path
>>> has a wrong attribute assigned to it, and may try to correct it.  But how?
>>>
>>>  $ edit .gitattributes ;# mark foo.dat as binary
>>>  $ rm foo.dat
>>>  $ git checkout foo.dat ;# make sure the new settings is correct???
>> As far as I can see, this works without any modifications to the patch.
>> Is that maybe because git_attr_set_direction() is not called if you use
>> that form of checkout?
> 
> But that in itself can be seen as a bug, right?  In another use case,
> suppose you botched your .gitattributes in HEAD version and noticed that
> foo.dat is checked out with a wrong attribute.  You try to fix it like
> this:
> 
>     $ git reset HEAD^ .gitattributes
>     $ rm foo.dat
>     $ git checkout foo.dat
> 
> If you do not flip the direction, the one from the work tree is used which
> is not what you want.  If you do, then you break the other use case.

Right, I didn't even think about that case. My idea of gitattributes was
that the working tree copy is always the master version, and takes
precedence. The only reason that the index takes precedence in the "git
checkout <branch>" case is that there is no other way to get it checked
out correctly, so I see this as an implementation detail. I'm sure some
people would disagree though.

But you're right, there is no way to make both cases correct. From my
standpoint I'd say that the behavior of your patch is the most intuitive.

I'll follow up with the test case.

--
Kristian

[PATCH] Add a test for checking whether gitattributes is honored by checkout.

[Kristian Amlie]

The original bug will not honor new entries in gitattributes if they
are changed in the same checkout as the files they affect.

It will also keep using .gitattributes, even if it is deleted in the
same commit as the files it affects.
---
 t/t0020-crlf.sh |   31 +++++++++++++++++++++++++++++++
 1 files changed, 31 insertions(+), 0 deletions(-)

diff --git a/t/t0020-crlf.sh b/t/t0020-crlf.sh
index 1be7446..4e72b53 100755
--- a/t/t0020-crlf.sh
+++ b/t/t0020-crlf.sh
@@ -429,6 +429,37 @@ test_expect_success 'in-tree .gitattributes (4)' '
 	}
 '
 
+test_expect_success 'checkout with existing .gitattributes' '
+
+	git config core.autocrlf true &&
+	git config --unset core.safecrlf &&
+	echo ".file2 -crlfQ" | q_to_cr >> .gitattributes &&
+	git add .gitattributes &&
+	git commit -m initial &&
+	echo ".file -crlfQ" | q_to_cr >> .gitattributes &&
+	echo "contents" > .file &&
+	git add .gitattributes .file &&
+	git commit -m second &&
+
+	git checkout master~1 &&
+	git checkout master &&
+	test "$(git diff-files --raw)" = ""
+
+'
+
+test_expect_success 'checkout when deleting .gitattributes' '
+
+	git rm .gitattributes &&
+	echo "contentsQ" | q_to_cr > .file2 &&
+	git add .file2 &&
+	git commit -m third
+
+	git checkout master~1 &&
+	git checkout master &&
+	remove_cr .file2 >/dev/null
+
+'
+
 test_expect_success 'invalid .gitattributes (must not crash)' '
 
 	echo "three +crlf" >>.gitattributes &&
-- 
1.6.2.1.222.g570cc.dirty