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

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

[Steffen Prohaska]

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 <prohaska@zib.de>
---
 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

[PATCH 2/2] checkout: fix attribute handling in checkout all

[Steffen Prohaska]

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 <prohaska@zib.de>
---
 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

Re: [PATCH 2/2] checkout: fix attribute handling in checkout all

[Junio C Hamano]

Steffen Prohaska <prohaska@zib.de> writes:

> 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'

While I think it is _one_ good approach to make things two-pass,
I do not know if this is enough.  A logic similar to this should
be made available to the codepath that switches branches,
shouldn't it?

It feels somewhat bogus to treat only the files that contain
".gitattributes" as substring.  Don't you want to at least say
"is .gitattributes or ends with /.gitattributes"?

I am not 100% convinced that it is "unexpected" that
these two sequences give different results.

 (1) rm -f .gitattributes other
     git-checkout-index -f .gitattributes
     git-checkout-index -f other

 (2) rm -f .gitattributes other
     git-checkout-index -f other
     git-checkout-index -f .gitattributes

And if this is mostly to work around the chicken-and-egg problem
of the initial checkout, I do not know if we would want to
complicate checkout_all() nor prepare_attr_stack().  Perhaps the
_initial_ checkout can do something like:

 * look at index, checkout .gitattributes and */.gitattributes;
 * checkout -f -a

_at the Porcelain level_, without complicating the plumbing?

Both patches are seriously out of existing coding style, by the
way.  Extra spaces after called function names everywhere, etc.

Re: [PATCH 2/2] checkout: fix attribute handling in checkout all

[Steffen Prohaska]

On Aug 12, 2007, at 11:50 PM, Junio C Hamano wrote:

> Steffen Prohaska <prohaska@zib.de> writes:
>
>> 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'
>
> While I think it is _one_ good approach to make things two-pass,
> I do not know if this is enough.  A logic similar to this should
> be made available to the codepath that switches branches,
> shouldn't it?

I think so. I played a bit more and I am pretty sure that
switching branches suffers from the same problem. After I
understood the problem I now remember that I wondered why
I needed '-f' now and then to convince git to do things
that normally just work.


> It feels somewhat bogus to treat only the files that contain
> ".gitattributes" as substring.  Don't you want to at least say
> "is .gitattributes or ends with /.gitattributes"?

Yes, if someone really want to construct a case, he can break
my code. Where should I place a helper function, such as
ends_with_gitattributes()?


> I am not 100% convinced that it is "unexpected" that
> these two sequences give different results.
>
>  (1) rm -f .gitattributes other
>      git-checkout-index -f .gitattributes
>      git-checkout-index -f other
>
>  (2) rm -f .gitattributes other
>      git-checkout-index -f other
>      git-checkout-index -f .gitattributes

Yeah, it's not obvious to me either.

My feeling it that the working tree should match the current
content of .gitattributes. That is after you modified
.gittattributes by whatever means (checkout, editor, ...),
or you modified the global default for autocrlf, you should
have a way to update your working tree. One way would be to
force a fresh checkout of all files in the working tree.
How can I do that?

I'm pretty certain that all files in the working tree resulting
from a single command should match the .gitattributes that
were modified by the same command. This is true for initial
checkout, but also for branch switching.


> And if this is mostly to work around the chicken-and-egg problem
> of the initial checkout, I do not know if we would want to
> complicate checkout_all() nor prepare_attr_stack().  Perhaps the
> _initial_ checkout can do something like:
>
>  * look at index, checkout .gitattributes and */.gitattributes;
>  * checkout -f -a
>
> _at the Porcelain level_, without complicating the plumbing?

I'm not convinced that it's only the initial checkout. As you
already mentioned above, branch switching suffers from the
same problem. But it could perhaps be handled on a porcelain
level.

Maybe we should start with some test cases first?


> Both patches are seriously out of existing coding style, by the
> way.  Extra spaces after called function names everywhere, etc.

Hmm, I see, ... my daytime coding style got burnt into my brain.
I'd rework if needed. But let's first find out what's needed.

	Steffen

Re: [PATCH 2/2] checkout: fix attribute handling in checkout all

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Steffen Prohaska <prohaska@zib.de> writes:
> ...
>> This works only together with the commit
>>
>> 'attr: fix attribute handling if .gitattributes is involved'
>
> While I think it is _one_ good approach to make things two-pass,
> I do not know if this is enough.  A logic similar to this should
> be made available to the codepath that switches branches,
> shouldn't it?

Ok, let's step back a bit and I'll suggest an alternative
approach to your 1/2.  This would hopefully solve 2/2 without
any code change your patch 2/2 has.

I think this approach is very much in line with how the git
plumbing works, but you would need to know how the world is
designed to work in order to appreciate it fully.  Let's have a
few paragraphs to give the readers some background.

The work tree side of git is primarily about the index, and what
is on the work tree is more or less secondary.  At the lower
level, often we deliberately treat not having a working tree
file as equivalent to having an unmodified work tree file.  We
can apply the same principle to this "missing .gitattributes
file" case.

People who only know modern git may not be aware of this, but
you can apply patches and perform a merge in a work tree that
does not have any file checked out, as long as your index is
fully populated.  For example, you can do something like this:

    $ git clone -n git://.../git.git v.git
    $ cd v.git
    $ git update-ref --no-deref HEAD $(git rev-parse v1.5.3-rc4^0)
    $ git read-tree HEAD
    $ git apply --index patch.txt

You will have the files that are patched in the resulting work
tree, so that you can inspect the result.  If you like the
result, you can even make a commit in such a sparsely populated
tree:

    $ git commit

Of course, "git commit -a" and "git add -u" Porcelain options
are more recent inventions, and they would not work with such a
sparsely populated work tree.  But the above demonstration shows
that at the plumbing level the index is the king and the work
tree is secondary, and this is very much as designed.  The merge
operation has similar characteristics:

    $ git merge master

... will check out the paths that need file-level 3-way merge,
so that you can inspect the result, but what you will have is a
sparsely populated work tree, and this is as designed.

Currently, the attr_stack code reads only from the work tree
and work tree alone.  We could change it to:

 - If the directory on the work tree has .gitattributes, use it
   (this is what the current code does);

 - Otherwise if the index has .gitattributes at the
   corresponding path, use that instead.

This essentially treats not having .gitattributes files checked
out as equivalent to having these files checked out unmodified,
which is very much in line with how the world is designed to
work.

Re: [PATCH 2/2] checkout: fix attribute handling in checkout all

[Marius Storm-Olsen]

[-- Attachment #1: Type: text/plain, Size: 1421 bytes --]

Junio C Hamano said the following on 13.08.2007 08:14:
> Ok, let's step back a bit and I'll suggest an alternative
> approach to your 1/2.  This would hopefully solve 2/2 without
> any code change your patch 2/2 has.
(..snip..)
> I think this approach is very much in line with how the git
> plumbing works, but you would need to know how the world is
> designed to work in order to appreciate it fully.  Let's have a
> few paragraphs to give the readers some background.
(..snip..)
> Currently, the attr_stack code reads only from the work tree
> and work tree alone.  We could change it to:
> 
>  - If the directory on the work tree has .gitattributes, use it
>    (this is what the current code does);
> 
>  - Otherwise if the index has .gitattributes at the
>    corresponding path, use that instead.
> 
> This essentially treats not having .gitattributes files checked
> out as equivalent to having these files checked out unmodified,
> which is very much in line with how the world is designed to
> work.

ACK! We really need this! :-)

In msysgit.git/etc/.gitattributes we have 'termcap -crlf', to avoid 
the termcaps being checked out with Windows EOL, if the user happens 
to have 'autocrlf = true'. However, when you checkout the working dir 
the first time it still has Windows EOL due to exactly this problem.

The above algorithm would alleviate this issue.

-- 
.marius


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 187 bytes --]

Re: [PATCH 2/2] checkout: fix attribute handling in checkout all

[Steffen Prohaska]

On Aug 13, 2007, at 8:32 AM, Marius Storm-Olsen wrote:

> Junio C Hamano said the following on 13.08.2007 08:14:
>> Ok, let's step back a bit and I'll suggest an alternative
>> approach to your 1/2.  This would hopefully solve 2/2 without
>> any code change your patch 2/2 has.
> (..snip..)
>> I think this approach is very much in line with how the git
>> plumbing works, but you would need to know how the world is
>> designed to work in order to appreciate it fully.  Let's have a
>> few paragraphs to give the readers some background.
> (..snip..)
>> Currently, the attr_stack code reads only from the work tree
>> and work tree alone.  We could change it to:
>>  - If the directory on the work tree has .gitattributes, use it
>>    (this is what the current code does);
>>  - Otherwise if the index has .gitattributes at the
>>    corresponding path, use that instead.
>> This essentially treats not having .gitattributes files checked
>> out as equivalent to having these files checked out unmodified,
>> which is very much in line with how the world is designed to
>> work.
>
> ACK! We really need this! :-)
>
> In msysgit.git/etc/.gitattributes we have 'termcap -crlf', to avoid  
> the termcaps being checked out with Windows EOL, if the user  
> happens to have 'autocrlf = true'. However, when you checkout the  
> working dir the first time it still has Windows EOL due to exactly  
> this problem.

And exactly this is where I recognized the issue.

msysgit devs,
We should really make autocrlf = true the default for us and fix all
problems that we'll encounter. There may be more tricky stuff ahead,
like merges, cherry-picks, ...

	Steffen

Re: [PATCH 2/2] checkout: fix attribute handling in checkout all

[Marius Storm-Olsen]

[-- Attachment #1: Type: text/plain, Size: 862 bytes --]

Steffen Prohaska said the following on 13.08.2007 08:50:
> On Aug 13, 2007, at 8:32 AM, Marius Storm-Olsen wrote:
>> In msysgit.git/etc/.gitattributes we have 'termcap -crlf', to avoid  
>> the termcaps being checked out with Windows EOL, if the user  
>> happens to have 'autocrlf = true'. However, when you checkout the  
>> working dir the first time it still has Windows EOL due to exactly  
>> this problem.
> 
> And exactly this is where I recognized the issue.
> 
> msysgit devs,
> We should really make autocrlf = true the default for us and fix
> all problems that we'll encounter. There may be more tricky stuff
> ahead, like merges, cherry-picks, ...

I'm more leaning towards having the installer give you the option to 
choose what kind of line-endings you want Git to work with; just like 
the Cygwin installer.

-- 
.marius


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 187 bytes --]

Re: [PATCH 2/2] checkout: fix attribute handling in checkout all

[Steffen Prohaska]

On Aug 13, 2007, at 9:15 AM, Marius Storm-Olsen wrote:

> Steffen Prohaska said the following on 13.08.2007 08:50:
>> On Aug 13, 2007, at 8:32 AM, Marius Storm-Olsen wrote:
>>> In msysgit.git/etc/.gitattributes we have 'termcap -crlf', to  
>>> avoid  the termcaps being checked out with Windows EOL, if the  
>>> user  happens to have 'autocrlf = true'. However, when you  
>>> checkout the  working dir the first time it still has Windows EOL  
>>> due to exactly  this problem.
>> And exactly this is where I recognized the issue.
>> msysgit devs,
>> We should really make autocrlf = true the default for us and fix
>> all problems that we'll encounter. There may be more tricky stuff
>> ahead, like merges, cherry-picks, ...
>
> I'm more leaning towards having the installer give you the option  
> to choose what kind of line-endings you want Git to work with; just  
> like the Cygwin installer.

Which is the root of much trouble with Cygwin. People now say,
git works perfectly in Cygwin but forget to mention that they
mean Cygwin A (in binmode) but not Cygwin B (in textmode).

Better choose the right default and work hard to make the
default choice work perfectly. I am strongly against an option
in the installer. An option _will_ cause confusion. Better give
people a hint how they can override the default for a single
user, or for a single repo. Then they recognize that they move
to a non-default configuration and hopefully think twice. And we
never need to talk about msysgit A vs. msysgit B, but only about
msysgit with repo specific or user specific options.

For me, the question comes down to the following: What would the
average Windows user (real Windows user, not Linux user who was
forced to work in Cygwin!) expect git to do with line endings?
The answer to this question should be the default.

	Steffen

Re: [PATCH 2/2] checkout: fix attribute handling in checkout all

[Marius Storm-Olsen]

[-- Attachment #1: Type: text/plain, Size: 2640 bytes --]

Steffen Prohaska said the following on 13.08.2007 09:32:
> On Aug 13, 2007, at 9:15 AM, Marius Storm-Olsen wrote:
>> Steffen Prohaska said the following on 13.08.2007 08:50:
>>> We should really make autocrlf = true the default for us and
>>> fix all problems that we'll encounter. There may be more tricky
>>> stuff ahead, like merges, cherry-picks, ...
>> I'm more leaning towards having the installer give you the option
>>  to choose what kind of line-endings you want Git to work with;
>> just like the Cygwin installer.
> 
> Which is the root of much trouble with Cygwin. People now say, git
> works perfectly in Cygwin but forget to mention that they mean
> Cygwin A (in binmode) but not Cygwin B (in textmode).
> 
> Better choose the right default and work hard to make the default
> choice work perfectly. I am strongly against an option in the
> installer. An option _will_ cause confusion. Better give people a
> hint how they can override the default for a single user, or for a
> single repo. Then they recognize that they move to a non-default
> configuration and hopefully think twice. And we never need to talk
> about msysgit A vs. msysgit B, but only about msysgit with repo
> specific or user specific options.
> 
> For me, the question comes down to the following: What would the 
> average Windows user (real Windows user, not Linux user who was 
> forced to work in Cygwin!) expect git to do with line endings? The
> answer to this question should be the default.

If we were talking about a huge amount (real) Windows users I would 
agree with you. However, currently most of the users using Git on 
Windows are Unix users which for some reason have to work on Windows 
every now and then. And changing the default option to autocrlf=true 
would be stepping on their toes, which we probably don't want to do :-)

I'm a Windows developer myself, so I naturally have autocrlf=true in 
my global settings. I don't think having the option in the installer 
(together with other things, like setting the global username, and 
email for example) would be such a bad thing. The problem with the way 
the Cygwin installer presents it is that it doesn't explain the pros 
and cons of the two options; it just recommends Linux EOL, which leads 
to confusion with some Windows developers. If we properly explain the 
issue in the installer, and say we recommend Windows EOL for Windows 
developers, I think it's OK. It would in any case be better than the 
current state where you have no option, or stepping on all the current 
msysgit/mingw-git maintainers toes.

-- 
.marius


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 187 bytes --]

Re: [PATCH 2/2] checkout: fix attribute handling in checkout all

[Steffen Prohaska]

On Aug 13, 2007, at 10:39 AM, Marius Storm-Olsen wrote:

> Steffen Prohaska said the following on 13.08.2007 09:32:
>> On Aug 13, 2007, at 9:15 AM, Marius Storm-Olsen wrote:
>>> Steffen Prohaska said the following on 13.08.2007 08:50:
>>>> We should really make autocrlf = true the default for us and
>>>> fix all problems that we'll encounter. There may be more tricky
>>>> stuff ahead, like merges, cherry-picks, ...
>>> I'm more leaning towards having the installer give you the option
>>>  to choose what kind of line-endings you want Git to work with;
>>> just like the Cygwin installer.
>> Which is the root of much trouble with Cygwin. People now say, git
>> works perfectly in Cygwin but forget to mention that they mean
>> Cygwin A (in binmode) but not Cygwin B (in textmode).
>> Better choose the right default and work hard to make the default
>> choice work perfectly. I am strongly against an option in the
>> installer. An option _will_ cause confusion. Better give people a
>> hint how they can override the default for a single user, or for a
>> single repo. Then they recognize that they move to a non-default
>> configuration and hopefully think twice. And we never need to talk
>> about msysgit A vs. msysgit B, but only about msysgit with repo
>> specific or user specific options.
>> For me, the question comes down to the following: What would the  
>> average Windows user (real Windows user, not Linux user who was  
>> forced to work in Cygwin!) expect git to do with line endings? The
>> answer to this question should be the default.
>
> If we were talking about a huge amount (real) Windows users I would  
> agree with you. However, currently most of the users using Git on  
> Windows are Unix users which for some reason have to work on  
> Windows every now and then. And changing the default option to  
> autocrlf=true would be stepping on their toes, which we probably  
> don't want to do :-)

My target audience of Git on Windows are Windows users and, frankly,
that is the only reasonable way to think about Windows. Why else should
I boot Windows, if I don't have real Windows users in mind? I mean,
Windows is not the superior platform to build Unix on top. The reason
to boot Windows is Windows itself, including its real users.


> I'm a Windows developer myself, so I naturally have autocrlf=true  
> in my global settings. I don't think having the option in the  
> installer (together with other things, like setting the global  
> username, and email for example) would be such a bad thing. The  
> problem with the way the Cygwin installer presents it is that it  
> doesn't explain the pros and cons of the two options; it just  
> recommends Linux EOL, which leads to confusion with some Windows  
> developers.

The problem is that Cygwin doesn't really support textmode. It
offers a choice, where there is no choice. After selecting textmode,
I still can install git. But git doesn't work.


> If we properly explain the issue in the installer, and say we  
> recommend Windows EOL for Windows developers, I think it's OK. It  
> would in any case be better than the current state where you have  
> no option, or stepping on all the current msysgit/mingw-git  
> maintainers toes.

Maybe I don't fully understand what msysgit is about. I thought it would
be about real Windows support, which I think requires to accept what
Windows users expect to be the right thing: Windows EOL.

In the long run it will also be easier for us, because other Windows
tools expect Windows EOL. I'm pretty sure that git plays better on
Windows if it has Window EOL on by default.

	Steffen

Re: [PATCH 2/2] checkout: fix attribute handling in checkout all

[Dmitry Kakurin]

On 8/13/07, Steffen Prohaska <prohaska@zib.de> wrote:
> My target audience of Git on Windows are Windows users and, frankly,
> that is the only reasonable way to think about Windows. Why else should
> I boot Windows, if I don't have real Windows users in mind? I mean,
> Windows is not the superior platform to build Unix on top. The reason
> to boot Windows is Windows itself, including its real users.

I agree with this approach.

> Maybe I don't fully understand what msysgit is about. I thought it would
> be about real Windows support, which I think requires to accept what
> Windows users expect to be the right thing: Windows EOL.

msysgit is Build Environment for Git on Windows. It's purpose is to
facilitate Git development. This is not what end-user wants.
Another installer (WinGit) is targeting end users. It is just so
happens that msysgit is in better shape right now and more useful. But
long term it will not be the case.

Here is another consideration: let's say I've started a new git repo
under Windows with no autocrlf set. Then my repo will contain crlf
line endings.
Now let's say that someone else checks out this repo with
autocrlf=true. What would happen then? Will they get cr cr lf?
-- 
- Dmitry

[PATCH 1/2] attr.c: refactoring

[Junio C Hamano]

This splits out a common routine that parses a single line of
attribute file and adds it to the attr_stack.  It should not
change any behaviour, other than attrs array in the attr_stack
structure is now grown with alloc_nr() macro, instead of one by
one, which relied on xrealloc() to give enough slack to be
efficient enough.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 Marius Storm-Olsen <marius@trolltech.com> writes:

 >> This essentially treats not having .gitattributes files checked
 >> out as equivalent to having these files checked out unmodified,
 >> which is very much in line with how the world is designed to
 >> work.
 >
 > ACK! We really need this! :-)

 So this is the first part, which does not do much.  The second
 one is interesting.

 attr.c |   67 +++++++++++++++++++++++++++++++++++++++------------------------
 1 files changed, 41 insertions(+), 26 deletions(-)

diff --git a/attr.c b/attr.c
index a071254..8d778f1 100644
--- a/attr.c
+++ b/attr.c
@@ -257,6 +257,7 @@ static struct attr_stack {
 	struct attr_stack *prev;
 	char *origin;
 	unsigned num_matches;
+	unsigned alloc;
 	struct match_attr **attrs;
 } *attr_stack;
 
@@ -287,6 +288,26 @@ static const char *builtin_attr[] = {
 	NULL,
 };
 
+static void handle_attr_line(struct attr_stack *res,
+			     const char *line,
+			     const char *src,
+			     int lineno,
+			     int macro_ok)
+{
+	struct match_attr *a;
+
+	a = parse_attr_line(line, src, lineno, macro_ok);
+	if (!a)
+		return;
+	if (res->alloc <= res->num_matches) {
+		res->alloc = alloc_nr(res->num_matches);
+		res->attrs = xrealloc(res->attrs,
+				      sizeof(struct match_attr *) *
+				      res->alloc);
+	}
+	res->attrs[res->num_matches++] = a;
+}
+
 static struct attr_stack *read_attr_from_array(const char **list)
 {
 	struct attr_stack *res;
@@ -294,42 +315,34 @@ static struct attr_stack *read_attr_from_array(const char **list)
 	int lineno = 0;
 
 	res = xcalloc(1, sizeof(*res));
-	while ((line = *(list++)) != NULL) {
-		struct match_attr *a;
-
-		a = parse_attr_line(line, "[builtin]", ++lineno, 1);
-		if (!a)
-			continue;
-		res->attrs = xrealloc(res->attrs,
-			sizeof(struct match_attr *) * (res->num_matches + 1));
-		res->attrs[res->num_matches++] = a;
-	}
+	while ((line = *(list++)) != NULL)
+		handle_attr_line(res, line, "[builtin]", ++lineno, 1);
 	return res;
 }
 
 static struct attr_stack *read_attr_from_file(const char *path, int macro_ok)
 {
-	FILE *fp;
+	FILE *fp = fopen(path, "r");
 	struct attr_stack *res;
 	char buf[2048];
 	int lineno = 0;
 
-	res = xcalloc(1, sizeof(*res));
-	fp = fopen(path, "r");
 	if (!fp)
-		return res;
+		return NULL;
+	res = xcalloc(1, sizeof(*res));
+	while (fgets(buf, sizeof(buf), fp))
+		handle_attr_line(res, buf, path, ++lineno, macro_ok);
+	fclose(fp);
+	return res;
+}
 
-	while (fgets(buf, sizeof(buf), fp)) {
-		struct match_attr *a;
+static struct attr_stack *read_attr(const char *path, int macro_ok)
+{
+	struct attr_stack *res;
 
-		a = parse_attr_line(buf, path, ++lineno, macro_ok);
-		if (!a)
-			continue;
-		res->attrs = xrealloc(res->attrs,
-			sizeof(struct match_attr *) * (res->num_matches + 1));
-		res->attrs[res->num_matches++] = a;
-	}
-	fclose(fp);
+	res = read_attr_from_file(path, macro_ok);
+	if (!res)
+		res = xcalloc(1, sizeof(*res));
 	return res;
 }
 
@@ -370,13 +383,15 @@ static void bootstrap_attr_stack(void)
 		elem->prev = attr_stack;
 		attr_stack = elem;
 
-		elem = read_attr_from_file(GITATTRIBUTES_FILE, 1);
+		elem = read_attr(GITATTRIBUTES_FILE, 1);
 		elem->origin = strdup("");
 		elem->prev = attr_stack;
 		attr_stack = elem;
 		debug_push(elem);
 
 		elem = read_attr_from_file(git_path(INFOATTRIBUTES_FILE), 1);
+		if (!elem)
+			elem = xcalloc(1, sizeof(*elem));
 		elem->origin = NULL;
 		elem->prev = attr_stack;
 		attr_stack = elem;
@@ -441,7 +456,7 @@ static void prepare_attr_stack(const char *path, int dirlen)
 		memcpy(pathbuf + dirlen, "/", 2);
 		cp = strchr(pathbuf + len + 1, '/');
 		strcpy(cp + 1, GITATTRIBUTES_FILE);
-		elem = read_attr_from_file(pathbuf, 0);
+		elem = read_attr(pathbuf, 0);
 		*cp = '\0';
 		elem->origin = strdup(pathbuf);
 		elem->prev = attr_stack;
-- 
1.5.3.rc4.89.g18078

[PATCH 2/2] attr.c: read .gitattributes from index as well.

[Junio C Hamano]

This makes .gitattributes files to be read from the index when
they are not checked out to the work tree.  This is in line with
the way we always allowed low-level tools to operate in sparsely
checked out work tree in a reasonable way.

It swaps the order of new file creation and converting the blob
to work tree representation; otherwise when we are in the middle
of checking out .gitattributes we would notice an empty but
unwritten .gitattributes file in the work tree and will ignore
the copy in the index.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 attr.c          |   61 ++++++++++++++++++++++++++++++++++++++++-
 entry.c         |   19 +++++++------
 t/t0020-crlf.sh |   81 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 150 insertions(+), 11 deletions(-)

diff --git a/attr.c b/attr.c
index 8d778f1..1293993 100644
--- a/attr.c
+++ b/attr.c
@@ -336,13 +336,70 @@ static struct attr_stack *read_attr_from_file(const char *path, int macro_ok)
 	return res;
 }
 
+static void *read_index_data(const char *path)
+{
+	int pos, len;
+	unsigned long sz;
+	enum object_type type;
+	void *data;
+
+	len = strlen(path);
+	pos = cache_name_pos(path, len);
+	if (pos < 0) {
+		/*
+		 * We might be in the middle of a merge, in which
+		 * case we would read stage #2 (ours).
+		 */
+		int i;
+		for (i = -pos - 1;
+		     (pos < 0 && i < active_nr &&
+		      !strcmp(active_cache[i]->name, path));
+		     i++)
+			if (ce_stage(active_cache[i]) == 2)
+				pos = i;
+	}
+	if (pos < 0)
+		return NULL;
+	data = read_sha1_file(active_cache[pos]->sha1, &type, &sz);
+	if (!data || type != OBJ_BLOB) {
+		free(data);
+		return NULL;
+	}
+	return data;
+}
+
 static struct attr_stack *read_attr(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)
-		res = xcalloc(1, sizeof(*res));
+	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
+	 * sparsely checked out work tree, so read from it.
+	 */
+	buf = read_index_data(path);
+	if (!buf)
+		return res;
+
+	for (sp = buf; *sp; ) {
+		char *ep;
+		int more;
+		for (ep = sp; *ep && *ep != '\n'; ep++)
+			;
+		more = (*ep == '\n');
+		*ep = '\0';
+		handle_attr_line(res, sp, path, ++lineno, macro_ok);
+		sp = ep + more;
+	}
+	free(buf);
 	return res;
 }
 
diff --git a/entry.c b/entry.c
index 0625112..fc3a506 100644
--- a/entry.c
+++ b/entry.c
@@ -112,6 +112,16 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout
 		if (!new)
 			return error("git-checkout-index: unable to read sha1 file of %s (%s)",
 				path, sha1_to_hex(ce->sha1));
+
+		/*
+		 * Convert from git internal format to working tree format
+		 */
+		buf = convert_to_working_tree(ce->name, new, &size);
+		if (buf) {
+			free(new);
+			new = buf;
+		}
+
 		if (to_tempfile) {
 			strcpy(path, ".merge_file_XXXXXX");
 			fd = mkstemp(path);
@@ -123,15 +133,6 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout
 				path, strerror(errno));
 		}
 
-		/*
-		 * Convert from git internal format to working tree format
-		 */
-		buf = convert_to_working_tree(ce->name, new, &size);
-		if (buf) {
-			free(new);
-			new = buf;
-		}
-
 		wrote = write_in_full(fd, new, size);
 		close(fd);
 		free(new);
diff --git a/t/t0020-crlf.sh b/t/t0020-crlf.sh
index fe1dfd0..0807d9f 100755
--- a/t/t0020-crlf.sh
+++ b/t/t0020-crlf.sh
@@ -290,4 +290,85 @@ test_expect_success '.gitattributes says two and three are text' '
 	fi
 '
 
+test_expect_success 'in-tree .gitattributes (1)' '
+
+	echo "one -crlf" >>.gitattributes &&
+	git add .gitattributes &&
+	git commit -m "Add .gitattributes" &&
+
+	rm -rf tmp one dir .gitattributes patch.file three &&
+	git read-tree --reset -u HEAD &&
+
+	if remove_cr one >/dev/null
+	then
+		echo "Eh? one should not have CRLF"
+		false
+	else
+		: happy
+	fi &&
+	remove_cr three >/dev/null || {
+		echo "Eh? three should still have CRLF"
+		false
+	}
+'
+
+test_expect_success 'in-tree .gitattributes (2)' '
+
+	rm -rf tmp one dir .gitattributes patch.file three &&
+	git read-tree --reset HEAD &&
+	git checkout-index -f -q -u -a &&
+
+	if remove_cr one >/dev/null
+	then
+		echo "Eh? one should not have CRLF"
+		false
+	else
+		: happy
+	fi &&
+	remove_cr three >/dev/null || {
+		echo "Eh? three should still have CRLF"
+		false
+	}
+'
+
+test_expect_success 'in-tree .gitattributes (3)' '
+
+	rm -rf tmp one dir .gitattributes patch.file three &&
+	git read-tree --reset HEAD &&
+	git checkout-index -u .gitattributes &&
+	git checkout-index -u one dir/two three &&
+
+	if remove_cr one >/dev/null
+	then
+		echo "Eh? one should not have CRLF"
+		false
+	else
+		: happy
+	fi &&
+	remove_cr three >/dev/null || {
+		echo "Eh? three should still have CRLF"
+		false
+	}
+'
+
+test_expect_success 'in-tree .gitattributes (4)' '
+
+	rm -rf tmp one dir .gitattributes patch.file three &&
+	git read-tree --reset HEAD &&
+	git checkout-index -u one dir/two three &&
+	git checkout-index -u .gitattributes &&
+
+	if remove_cr one >/dev/null
+	then
+		echo "Eh? one should not have CRLF"
+		false
+	else
+		: happy
+	fi &&
+	remove_cr three >/dev/null || {
+		echo "Eh? three should still have CRLF"
+		false
+	}
+'
+
 test_done
-- 
1.5.3.rc4.89.g18078

Re: [PATCH 2/2] checkout: fix attribute handling in checkout all

[Steffen Prohaska]

On Aug 13, 2007, at 8:14 AM, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Steffen Prohaska <prohaska@zib.de> writes:
>> ...
>>> This works only together with the commit
>>>
>>> 'attr: fix attribute handling if .gitattributes is involved'
>>
>> While I think it is _one_ good approach to make things two-pass,
>> I do not know if this is enough.  A logic similar to this should
>> be made available to the codepath that switches branches,
>> shouldn't it?
>
> Ok, let's step back a bit and I'll suggest an alternative
> approach to your 1/2.  This would hopefully solve 2/2 without
> any code change your patch 2/2 has.

That would be great.


> I think this approach is very much in line with how the git
> plumbing works, but you would need to know how the world is
> designed to work in order to appreciate it fully.  Let's have a
> few paragraphs to give the readers some background.
>
> The work tree side of git is primarily about the index, and what
> is on the work tree is more or less secondary.  At the lower
> level, often we deliberately treat not having a working tree
> file as equivalent to having an unmodified work tree file.  We
> can apply the same principle to this "missing .gitattributes
> file" case.
>
> People who only know modern git may not be aware of this, but
> you can apply patches and perform a merge in a work tree that
> does not have any file checked out, as long as your index is
> fully populated.  For example, you can do something like this:
>
>     $ git clone -n git://.../git.git v.git
>     $ cd v.git
>     $ git update-ref --no-deref HEAD $(git rev-parse v1.5.3-rc4^0)
>     $ git read-tree HEAD
>     $ git apply --index patch.txt
>
> You will have the files that are patched in the resulting work
> tree, so that you can inspect the result.  If you like the
> result, you can even make a commit in such a sparsely populated
> tree:
>
>     $ git commit
>
> Of course, "git commit -a" and "git add -u" Porcelain options
> are more recent inventions, and they would not work with such a
> sparsely populated work tree.  But the above demonstration shows
> that at the plumbing level the index is the king and the work
> tree is secondary, and this is very much as designed.  The merge
> operation has similar characteristics:
>
>     $ git merge master
>
> ... will check out the paths that need file-level 3-way merge,
> so that you can inspect the result, but what you will have is a
> sparsely populated work tree, and this is as designed.

Ah, merge ...


> Currently, the attr_stack code reads only from the work tree
> and work tree alone.  We could change it to:
>
>  - If the directory on the work tree has .gitattributes, use it
>    (this is what the current code does);
>
>  - Otherwise if the index has .gitattributes at the
>    corresponding path, use that instead.
>
> This essentially treats not having .gitattributes files checked
> out as equivalent to having these files checked out unmodified,
> which is very much in line with how the world is designed to
> work.
>

We may have conflicts in the .gitattributes file during a merge.
.gitattributes may be present in different stages, and with
conflict markers in the work tree.

Could we drop reading the file in the work tree completely?
.gitattributes would be a property of the index alone. To control
attributes you first need to add them to the index, before adding
the file that has attributes set in .gitattributes.

If we have .gitattributes in different stages, the right one
should be chosen to checkout corresponding files in the same stage.

	Steffen

Re: [PATCH 2/2] checkout: fix attribute handling in checkout all

[Johannes Schindelin]

Hi,

On Mon, 13 Aug 2007, Steffen Prohaska wrote:

> Could we drop reading the file [.gitattributes] in the work tree 
> completely?

NACK.

It is not good to hide things away from the working tree.  It is much 
easier to just edit a file than to edit it and put it into the index.

Ciao,
Dscho

Re: [PATCH 2/2] checkout: fix attribute handling in checkout all

[David Kastrup]

Junio C Hamano <gitster@pobox.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>     $ git update-ref --no-deref HEAD $(git rev-parse v1.5.3-rc4^0)

Is there a fundamental difference to using

git-symbolic-ref HEAD $(git rev-parse v1.5.3-rc4^0)

here?

-- 
David Kastrup

git-update-ref bug? (was: [PATCH 2/2] checkout: fix attribute handling in checkout all)

[David Kastrup]

David Kastrup <dak@gnu.org> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>     $ git update-ref --no-deref HEAD $(git rev-parse v1.5.3-rc4^0)
>
> Is there a fundamental difference to using
>
> git-symbolic-ref HEAD $(git rev-parse v1.5.3-rc4^0)
>
> here?

Apart from the fact that the latter works, and the former doesn't
because "--no-deref" is actually ignored?

-- 
David Kastrup

Re: [PATCH 2/2] checkout: fix attribute handling in checkout all

[Junio C Hamano]

David Kastrup <dak@gnu.org> writes:

>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>     $ git update-ref --no-deref HEAD $(git rev-parse v1.5.3-rc4^0)
>
> Is there a fundamental difference to using
>
> git-symbolic-ref HEAD $(git rev-parse v1.5.3-rc4^0)
>
> here?

The symbolic-ref command is about setting the HEAD to "point at
a(nother) ref".  There is no point talking about "fundamental
difference" here --- the latter is plain wrong, feeding
rev-parse output (which is an object name) as its second
parameter.

Did you mean to ask about the difference between "git-update-ref
HEAD $param" with or without --no-deref?

With --no-deref, it makes the HEAD detached even when HEAD is a
symref that points at a ref, e.g. "refs/heads/master".  Without
that option, it updates the ref that is pointed at by the HEAD
symref.

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

[Brian Downing]

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