Honoring a checked out gitattributes file

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

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

Kristian Amlie venit, vidit, dixit 28.01.2009 16:25:

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

I agree.

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

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

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

new file mode 100755
index 0000000..cb997a8
@@ -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

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

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

new file mode 100755
index 0000000..cb997a8
@@ -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

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(-)

index 17f6a4d..7deb51a 100644
@@ -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;
+}

index f1c2038..8f4135b 100644
@@ -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 */

index 05aa58d..121c979 100644
@@ -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

...

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

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

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

new file mode 100755
index 0000000..d9d6465
@@ -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

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(-)

index 17f6a4d..2bc0847 100644
@@ -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;
+	}
+}

index f1c2038..8f4135b 100644
@@ -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 */

index 05aa58d..7fb27fd 100644
@@ -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);

index d9d6465..e704c93 100755
@@ -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 &&

1.6.1.1

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(-)

index 17f6a4d..72f6807 100644
@@ -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;
+}

index f1c2038..3a2f4ec 100644
@@ -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 */

index e547282..661218c 100644
@@ -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;
 }
 

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.

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

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.

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

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(-)

index 1be7446..4e72b53 100755
@@ -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

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

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

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

--
Kristian