[PATCH 1/2] attr: fix attribute handling if .gitattributes is involved

If we checkout .gitattributes, we must not try to parse
.gitattributes. If we tried to read it, it may not be
present because checkout_entry unlinks files before checkout.
But we would record that no attributes are present for this
directory, which is wrong. And worse, we would never try again.

This fix skips read_attr_from_file if the path triggering
the read ends with .gitattributes. This is a bit more than we
need, but it helps to checkout all .gitattributes before any
other file, without starting the attr machinery.

This solves part of the problem of correct attributes during
checkout. For example

   rm .gitattributes other
   git-checkout-index -f .gitattributes other

will work as expected, while

   rm .gitattributes other
   git-checkout-index -f other .gitattributes

will not. The problem in the second case is that .gitattributes
is not yet present when it is needed.

The second case could be fixed by ordering files such that
all .gitattributes are checked out before any other file. But
this is perhaps too expensive and not really needed. If the user
deliberately chooses to checkout .gitattributes after another
file, he will not benefit from the attr machinery.

However, for checkout-index --all we should fix the problem,
which is done by the next patch, building on top of this one.

Signed-off-by: Steffen Prohaska <redacted>
---
 attr.c |   58 ++++++++++++++++++++++++++++++++++++++++++----------------
 1 files changed, 42 insertions(+), 16 deletions(-)

This together with the patch that follows fixes a problem, which
is most likely to bite Windows users. I recognized it by setting
autocrlf globally to true and doing a fresh checkout of msysgit.
The checkout contained etc/termcap converted to CRLF, although
is was marked as '-crlf' in etc/.gitattributes.

If we believe autocrlf is a reasonable default for Windows users,
we really should use it ourselves, to find such problems. 

The fixed problem is not really critical but may be quite annoying,
and complex to understand.

    Steffen

diff --git a/attr.c b/attr.c
index a071254..e942f6c 100644
--- a/attr.c
+++ b/attr.c

@@ -383,6 +383,18 @@ static void bootstrap_attr_stack(void)
 	}
 }
 
+static int ends_with_gitattributes (const char* path)
+{
+	int attributes_len = strlen (GITATTRIBUTES_FILE);
+	int path_len = strlen (path);
+	if (path_len >= attributes_len
+	     && strcmp (path + path_len - attributes_len, GITATTRIBUTES_FILE) == 0)
+	{
+		return 1;
+	}
+	return 0;
+}
+
 static void prepare_attr_stack(const char *path, int dirlen)
 {
 	struct attr_stack *elem, *info;

@@ -430,23 +442,37 @@ static void prepare_attr_stack(const char *path, int dirlen)
 
 	/*
 	 * Read from parent directories and push them down
+	 *
+	 * But don't try to read if path ends with .gitattributes.
+	 * In this case we could fail and record no attributes for
+	 * a directory. We better wait and see if we need the
+	 * attributes later.
+	 *
+	 * We skip all .gitattributes, even in higher directories.
+	 * Thus, we can checkout all .gitattributes in any order
+	 * before the attr machinery starts to work. .gitattributes
+	 * should not be controlled by .gitattributes in the working
+	 * tree anyway.
+	 *
 	 */
-	while (1) {
-		char *cp;
-
-		len = strlen(attr_stack->origin);
-		if (dirlen <= len)
-			break;
-		memcpy(pathbuf, path, dirlen);
-		memcpy(pathbuf + dirlen, "/", 2);
-		cp = strchr(pathbuf + len + 1, '/');
-		strcpy(cp + 1, GITATTRIBUTES_FILE);
-		elem = read_attr_from_file(pathbuf, 0);
-		*cp = '\0';
-		elem->origin = strdup(pathbuf);
-		elem->prev = attr_stack;
-		attr_stack = elem;
-		debug_push(elem);
+	if (!ends_with_gitattributes (path)) {
+		while (1) {
+			char *cp;
+
+			len = strlen(attr_stack->origin);
+			if (dirlen <= len)
+				break;
+			memcpy(pathbuf, path, dirlen);
+			memcpy(pathbuf + dirlen, "/", 2);
+			cp = strchr(pathbuf + len + 1, '/');
+			strcpy(cp + 1, GITATTRIBUTES_FILE);
+			elem = read_attr_from_file(pathbuf, 0);
+			*cp = '\0';
+			elem->origin = strdup(pathbuf);
+			elem->prev = attr_stack;
+			attr_stack = elem;
+			debug_push(elem);
+		}
 	}
 
 	/*

-- 
1.5.3.rc4.96.g6ceb

We need to check out .gitattributes files first to have
them in place when we check out the remaining files. This
is needed to get the right attributes during checkout,
for example having the right crlf conversion on the first
checkout if crlf is controlled by a .gitattribute file.

This works only together with the commit

'attr: fix attribute handling if .gitattributes is involved'

which ensures that .gitattributes files do not trigger the
attribute machinery too early.

Signed-off-by: Steffen Prohaska <redacted>
---
 builtin-checkout-index.c |   47 ++++++++++++++++++++++++++++-----------------
 1 files changed, 29 insertions(+), 18 deletions(-)

diff --git a/builtin-checkout-index.c b/builtin-checkout-index.c
index 75377b9..5e87a39 100644
--- a/builtin-checkout-index.c
+++ b/builtin-checkout-index.c

@@ -125,27 +125,38 @@ static int checkout_file(const char *name, int prefix_length)
 
 static void checkout_all(const char *prefix, int prefix_length)
 {
-	int i, errs = 0;
+	int i, pass, errs = 0;
 	struct cache_entry* last_ce = NULL;
 
-	for (i = 0; i < active_nr ; i++) {
-		struct cache_entry *ce = active_cache[i];
-		if (ce_stage(ce) != checkout_stage
-		    && (CHECKOUT_ALL != checkout_stage || !ce_stage(ce)))
-			continue;
-		if (prefix && *prefix &&
-		    (ce_namelen(ce) <= prefix_length ||
-		     memcmp(prefix, ce->name, prefix_length)))
-			continue;
-		if (last_ce && to_tempfile) {
-			if (ce_namelen(last_ce) != ce_namelen(ce)
-			    || memcmp(last_ce->name, ce->name, ce_namelen(ce)))
-				write_tempfile_record(last_ce->name, prefix_length);
+	/* pass 0: check out only .gitattribute files
+	   pass 1: check out every file
+
+	   This is needed to have all .gitattributes in place before
+	   checking out files, and thus do the right conversion.
+	 */
+	for (pass = 0; pass < 2; pass++) {
+		for (i = 0; i < active_nr ; i++) {
+			struct cache_entry *ce = active_cache[i];
+			if (pass == 0 && strstr (ce->name, GITATTRIBUTES_FILE) == 0) {
+			    continue;
+			}
+			if (ce_stage(ce) != checkout_stage
+			    && (CHECKOUT_ALL != checkout_stage || !ce_stage(ce)))
+				continue;
+			if (prefix && *prefix &&
+			    (ce_namelen(ce) <= prefix_length ||
+			     memcmp(prefix, ce->name, prefix_length)))
+				continue;
+			if (last_ce && to_tempfile) {
+				if (ce_namelen(last_ce) != ce_namelen(ce)
+				    || memcmp(last_ce->name, ce->name, ce_namelen(ce)))
+					write_tempfile_record(last_ce->name, prefix_length);
+			}
+			if (checkout_entry(ce, &state,
+			    to_tempfile ? topath[ce_stage(ce)] : NULL) < 0)
+				errs++;
+			last_ce = ce;
 		}
-		if (checkout_entry(ce, &state,
-		    to_tempfile ? topath[ce_stage(ce)] : NULL) < 0)
-			errs++;
-		last_ce = ce;
 	}
 	if (last_ce && to_tempfile)
 		write_tempfile_record(last_ce->name, prefix_length);

-- 
1.5.3.rc4.96.g6ceb

On Sun, Aug 12, 2007 at 10:34:34PM +0200, Steffen Prohaska wrote:

This together with the patch that follows fixes a problem, which
is most likely to bite Windows users. I recognized it by setting
autocrlf globally to true and doing a fresh checkout of msysgit.
The checkout contained etc/termcap converted to CRLF, although
is was marked as '-crlf' in etc/.gitattributes.

If we believe autocrlf is a reasonable default for Windows users,
we really should use it ourselves, to find such problems. 

The fixed problem is not really critical but may be quite annoying,
and complex to understand.

I have a case in a live repository where this is not merely annoying.

I have a test data in one of my repositories that must never be
converted.  It has a mix of Unix and Windows line-endings in it.  I
marked the appropriate files with the "-crlf" attribute.

With the current git behavior, cloning this repository with "autocrlf"
globally set irreversably corrupts the files (converting the LF
line-enders to CRLF) on checkout.  If the user is careless upon commit,
these corrupted files will then be committed back (probably with all
CRLFs, since at that point the .gitattributes is present and the -crlf
attribute will be honored.)

There is kind of an ugly chicken-and-egg problem here, but I think it
would be good to figure it out to avoid this kind of broken behavior.

I would also vote for this handling to be in the plumbing, since the
autocrlf processing is in the plumbing as well.

Another thing to consider with respect to attribute access -- It would
be nice for git-cvsserver to be able to send the correct -k option to
the remote side for line endings.  Doing this correctly involves
accessing attributes, but git-cvsserver never has a full working
directory.  Having the attribute machinery work without a working
directory (either directly from trees or from an index) would be a great
benefit here.

-bcd