t7400 broken on pu (Mac OS X)

t7400 broken on pu (Mac OS X)

[Torsten Bögershausen]

The current pu fails on Mac OS, case insensitive FS.


Bisecting points out
commit 3f28e4fafc046284657945798d71c57608bee479
[snip]
Date:   Sun Jan 6 13:21:07 2013 +0700

    Convert add_files_to_cache to take struct pathspec

And I veryfied that the preceeding commit 05647d2d8a5dc456d1f4ef73
is OK.

It fails here:
not ok 38 - gracefully add submodule with a trailing slash

A run of a modified t7400 looks like this:
Is there anything more, that I can do to debug this?


[snip]
ok 37 - do not add files from a submodule

expecting success: 

	git reset --hard &&
	echo 1 >&2 &&
	git commit -m "commit subproject" init &&
	echo 2 >&2 &&
	(cd init &&
	echo 3 >&2 &&
	 echo b > a) &&
	echo 4 >&2 &&
	git add init/ &&
	echo 5 >&2 &&
	git diff --exit-code --cached init &&
	echo 6 >&2 &&
	commit=$(cd init &&
	echo 7 >&2 &&
	 git commit -m update a >/dev/null &&
	echo 8 >&2 &&
	 git rev-parse HEAD) &&
	echo 9 >&2 &&
	git add init/ &&
	echo 10 >&2 &&
	test_must_fail git diff --exit-code --cached init &&
	echo 11 >&2 &&
	test $commit = $(git ls-files --stage |
		sed -n "s/^160000 \([^ ]*\).*/\1/p")


HEAD is now at 57f2622 super commit 1
1
[second 1b8c63f] commit subproject
 Author: A U Thor <author@example.com>
 1 file changed, 1 insertion(+), 1 deletion(-)
2
3
4
5
6
7
8
9
10
test_must_fail: command succeeded: git diff --exit-code --cached init
not ok 38 - gracefully add submodule with a trailing slash

Re: t7400 broken on pu (Mac OS X)

[Junio C Hamano]

Torsten Bögershausen <tboegi@web.de> writes:

> The current pu fails on Mac OS, case insensitive FS.
>
>
> Bisecting points out
> commit 3f28e4fafc046284657945798d71c57608bee479
> [snip]
> Date:   Sun Jan 6 13:21:07 2013 +0700

Next time do not [snip] but please find the author address there,
and Cc such a report.

I think this topic is planned to be rerolled anyway, and your report
would be a valuable input while doing so.

Thanks.

Re: t7400 broken on pu (Mac OS X)

[Duy Nguyen]

On Wed, Jan 09, 2013 at 07:43:03PM +0100, Torsten Bögershausen wrote:
> The current pu fails on Mac OS, case insensitive FS.
> 
> 
> Bisecting points out
> commit 3f28e4fafc046284657945798d71c57608bee479
> [snip]
> Date:   Sun Jan 6 13:21:07 2013 +0700
> 
>     Convert add_files_to_cache to take struct pathspec
> 

I can reproduce it by setting core.ignorecase to true. There is a bug
that I overlooked. Can you verify if this throw-away patch fixes it
for you? A proper fix will be in the reroll later.

-- 8< --
diff --git a/builtin/add.c b/builtin/add.c
index 641037f..61cb8bd 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -155,12 +155,13 @@ static char *prune_directory(struct dir_struct *dir, const char **pathspec, int
 	return seen;
 }
 
-static void treat_gitlinks(const char **pathspec)
+static int treat_gitlinks(const char **pathspec)
 {
 	int i;
+	int modified = 0;
 
 	if (!pathspec || !*pathspec)
-		return;
+		return modified;
 
 	for (i = 0; i < active_nr; i++) {
 		struct cache_entry *ce = active_cache[i];
@@ -171,15 +172,17 @@ static void treat_gitlinks(const char **pathspec)
 				if (len2 <= len || pathspec[j][len] != '/' ||
 				    memcmp(ce->name, pathspec[j], len))
 					continue;
-				if (len2 == len + 1)
+				if (len2 == len + 1) {
 					/* strip trailing slash */
 					pathspec[j] = xstrndup(ce->name, len);
-				else
+					modified = 1;
+				} else
 					die (_("Path '%s' is in submodule '%.*s'"),
 						pathspec[j], len, ce->name);
 			}
 		}
 	}
+	return modified;
 }
 
 static void refresh(int verbose, const struct pathspec *pathspec)
@@ -418,7 +421,16 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 
 	if (read_cache() < 0)
 		die(_("index file corrupt"));
-	treat_gitlinks(pathspec.raw);
+	if (treat_gitlinks(pathspec.raw))
+		/*
+		 * HACK: treat_gitlinks strips the trailing slashes
+		 * out of submodule entries but it only affects
+		 * raw[]. Everything in pathspec.items is not touched.
+		 * Re-init it to propagate the change. Long term, this
+		 * function should be moved to pathspec.c and update
+		 * everything in a consistent way.
+		 */
+		init_pathspec(&pathspec, pathspec.raw);
 
 	if (add_new_files) {
 		int baselen;
-- 8< --

Re: t7400 broken on pu (Mac OS X)

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Wed, Jan 09, 2013 at 07:43:03PM +0100, Torsten Bögershausen wrote:
>> The current pu fails on Mac OS, case insensitive FS.
>> 
>> 
>> Bisecting points out
>> commit 3f28e4fafc046284657945798d71c57608bee479
>> [snip]
>> Date:   Sun Jan 6 13:21:07 2013 +0700
>> 
>>     Convert add_files_to_cache to take struct pathspec
>> 
>
> I can reproduce it by setting core.ignorecase to true. There is a bug
> that I overlooked. Can you verify if this throw-away patch fixes it
> for you? A proper fix will be in the reroll later.

I can see why it is wrong to let pathspec.raw be rewritten without
making matching change to the containing pathspec, but I find it
strange why it matters only on case-insensitive codepath.

I agree with the "Hack" comment that the canonicalization should be
done at a higher level upfront.  Then ls-files does not need its own
strip_trailing_slash_from_submodules(), and check_path_for_gitlink()
can (and should---the callers of "check_anything" would not expect
the function to change things) stop rewriting its parameter.

Thanks for a quick response.

> -- 8< --
> diff --git a/builtin/add.c b/builtin/add.c
> index 641037f..61cb8bd 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -155,12 +155,13 @@ static char *prune_directory(struct dir_struct *dir, const char **pathspec, int
>  	return seen;
>  }
>  
> -static void treat_gitlinks(const char **pathspec)
> +static int treat_gitlinks(const char **pathspec)
>  {
>  	int i;
> +	int modified = 0;
>  
>  	if (!pathspec || !*pathspec)
> -		return;
> +		return modified;
>  
>  	for (i = 0; i < active_nr; i++) {
>  		struct cache_entry *ce = active_cache[i];
> @@ -171,15 +172,17 @@ static void treat_gitlinks(const char **pathspec)
>  				if (len2 <= len || pathspec[j][len] != '/' ||
>  				    memcmp(ce->name, pathspec[j], len))
>  					continue;
> -				if (len2 == len + 1)
> +				if (len2 == len + 1) {
>  					/* strip trailing slash */
>  					pathspec[j] = xstrndup(ce->name, len);
> -				else
> +					modified = 1;
> +				} else
>  					die (_("Path '%s' is in submodule '%.*s'"),
>  						pathspec[j], len, ce->name);
>  			}
>  		}
>  	}
> +	return modified;
>  }
>  
>  static void refresh(int verbose, const struct pathspec *pathspec)
> @@ -418,7 +421,16 @@ int cmd_add(int argc, const char **argv, const char *prefix)
>  
>  	if (read_cache() < 0)
>  		die(_("index file corrupt"));
> -	treat_gitlinks(pathspec.raw);
> +	if (treat_gitlinks(pathspec.raw))
> +		/*
> +		 * HACK: treat_gitlinks strips the trailing slashes
> +		 * out of submodule entries but it only affects
> +		 * raw[]. Everything in pathspec.items is not touched.
> +		 * Re-init it to propagate the change. Long term, this
> +		 * function should be moved to pathspec.c and update
> +		 * everything in a consistent way.
> +		 */
> +		init_pathspec(&pathspec, pathspec.raw);
>  
>  	if (add_new_files) {
>  		int baselen;
> -- 8< --

Re: t7400 broken on pu (Mac OS X)

[Torsten Bögershausen]

On 10.01.13 18:58, Junio C Hamano wrote:
> Duy Nguyen <pclouds@gmail.com> writes:
>
>> On Wed, Jan 09, 2013 at 07:43:03PM +0100, Torsten Bögershausen wrote:
>>> The current pu fails on Mac OS, case insensitive FS.
>>>
>>>
>>> Bisecting points out
>>> commit 3f28e4fafc046284657945798d71c57608bee479
>>> [snip]
>>> Date:   Sun Jan 6 13:21:07 2013 +0700
>>>
>>>     Convert add_files_to_cache to take struct pathspec
>>>
>> I can reproduce it by setting core.ignorecase to true. There is a bug
>> that I overlooked. Can you verify if this throw-away patch fixes it
>> for you? A proper fix will be in the reroll later.
> I can see why it is wrong to let pathspec.raw be rewritten without
> making matching change to the containing pathspec, but I find it
> strange why it matters only on case-insensitive codepath.
>
> I agree with the "Hack" comment that the canonicalization should be
> done at a higher level upfront.  Then ls-files does not need its own
> strip_trailing_slash_from_submodules(), and check_path_for_gitlink()
> can (and should---the callers of "check_anything" would not expect
> the function to change things) stop rewriting its parameter.
>
> Thanks for a quick response.
>
The patch fixes t7400.
Thanks from my side as well
/Torsten

Re: t7400 broken on pu (Mac OS X)

[Duy Nguyen]

On Fri, Jan 11, 2013 at 12:58 AM, Junio C Hamano <gitster@pobox.com> wrote:
> I can see why it is wrong to let pathspec.raw be rewritten without
> making matching change to the containing pathspec, but I find it
> strange why it matters only on case-insensitive codepath.

Yeah, I don't get it either. I can see that core.ignorecase exercises
some more code, but still fail to see the link. I should get to the
bottom of this and write some tests to for core.ignorecase-only code.
-- 
Duy