{"thread":{"id":"32574","subject":"t7400 broken on pu (Mac OS X)","startedAt":"2013-01-09T18:43:03Z","lastAt":"2013-01-11T11:03:47Z","messageCount":6,"participants":["Torsten Bögershausen","Junio C Hamano","Duy Nguyen"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"206399","messageId":"50EDBA37.30205@web.de","threadId":"32574","inReplyTo":null,"subject":"t7400 broken on pu (Mac OS X)","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2013-01-09T18:43:03Z","receivedAt":"2013-01-09T18:43:03Z","isPatch":false,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"The current pu fails on Mac OS, case insensitive FS.\n\n\nBisecting points out\ncommit 3f28e4fafc046284657945798d71c57608bee479\n[snip]\nDate:   Sun Jan 6 13:21:07 2013 +0700\n\n    Convert add_files_to_cache to take struct pathspec\n\nAnd I veryfied that the preceeding commit 05647d2d8a5dc456d1f4ef73\nis OK.\n\nIt fails here:\nnot ok 38 - gracefully add submodule with a trailing slash\n\nA run of a modified t7400 looks like this:\nIs there anything more, that I can do to debug this?\n\n\n[snip]\nok 37 - do not add files from a submodule\n\nexpecting success: \n\n\tgit reset --hard &&\n\techo 1 >&2 &&\n\tgit commit -m \"commit subproject\" init &&\n\techo 2 >&2 &&\n\t(cd init &&\n\techo 3 >&2 &&\n\t echo b > a) &&\n\techo 4 >&2 &&\n\tgit add init/ &&\n\techo 5 >&2 &&\n\tgit diff --exit-code --cached init &&\n\techo 6 >&2 &&\n\tcommit=$(cd init &&\n\techo 7 >&2 &&\n\t git commit -m update a >/dev/null &&\n\techo 8 >&2 &&\n\t git rev-parse HEAD) &&\n\techo 9 >&2 &&\n\tgit add init/ &&\n\techo 10 >&2 &&\n\ttest_must_fail git diff --exit-code --cached init &&\n\techo 11 >&2 &&\n\ttest $commit = $(git ls-files --stage |\n\t\tsed -n \"s/^160000 \\([^ ]*\\).*/\\1/p\")\n\n\nHEAD is now at 57f2622 super commit 1\n1\n[second 1b8c63f] commit subproject\n Author: A U Thor <author@example.com>\n 1 file changed, 1 insertion(+), 1 deletion(-)\n2\n3\n4\n5\n6\n7\n8\n9\n10\ntest_must_fail: command succeeded: git diff --exit-code --cached init\nnot ok 38 - gracefully add submodule with a trailing slash\n"},{"id":"206402","messageId":"7v38ya6ubq.fsf@alter.siamese.dyndns.org","threadId":"32574","inReplyTo":"50EDBA37.30205@web.de","subject":"Re: t7400 broken on pu (Mac OS X)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-09T19:16:09Z","receivedAt":"2013-01-09T19:16:09Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen <tboegi@web.de> writes:\n\n> The current pu fails on Mac OS, case insensitive FS.\n>\n>\n> Bisecting points out\n> commit 3f28e4fafc046284657945798d71c57608bee479\n> [snip]\n> Date:   Sun Jan 6 13:21:07 2013 +0700\n\nNext time do not [snip] but please find the author address there,\nand Cc such a report.\n\nI think this topic is planned to be rerolled anyway, and your report\nwould be a valuable input while doing so.\n\nThanks.\n"},{"id":"206442","messageId":"20130110062838.GA11634@duynguyen-vnpc.dek-tpc.internal","threadId":"32574","inReplyTo":"50EDBA37.30205@web.de","subject":"Re: t7400 broken on pu (Mac OS X)","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-01-10T06:28:38Z","receivedAt":"2013-01-10T06:28:38Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Jan 09, 2013 at 07:43:03PM +0100, Torsten Bögershausen wrote:\n> The current pu fails on Mac OS, case insensitive FS.\n> \n> \n> Bisecting points out\n> commit 3f28e4fafc046284657945798d71c57608bee479\n> [snip]\n> Date:   Sun Jan 6 13:21:07 2013 +0700\n> \n>     Convert add_files_to_cache to take struct pathspec\n> \n\nI can reproduce it by setting core.ignorecase to true. There is a bug\nthat I overlooked. Can you verify if this throw-away patch fixes it\nfor you? A proper fix will be in the reroll later.\n\n-- 8< --\ndiff --git a/builtin/add.c b/builtin/add.c\nindex 641037f..61cb8bd 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -155,12 +155,13 @@ static char *prune_directory(struct dir_struct *dir, const char **pathspec, int\n \treturn seen;\n }\n \n-static void treat_gitlinks(const char **pathspec)\n+static int treat_gitlinks(const char **pathspec)\n {\n \tint i;\n+\tint modified = 0;\n \n \tif (!pathspec || !*pathspec)\n-\t\treturn;\n+\t\treturn modified;\n \n \tfor (i = 0; i < active_nr; i++) {\n \t\tstruct cache_entry *ce = active_cache[i];\n@@ -171,15 +172,17 @@ static void treat_gitlinks(const char **pathspec)\n \t\t\t\tif (len2 <= len || pathspec[j][len] != '/' ||\n \t\t\t\t    memcmp(ce->name, pathspec[j], len))\n \t\t\t\t\tcontinue;\n-\t\t\t\tif (len2 == len + 1)\n+\t\t\t\tif (len2 == len + 1) {\n \t\t\t\t\t/* strip trailing slash */\n \t\t\t\t\tpathspec[j] = xstrndup(ce->name, len);\n-\t\t\t\telse\n+\t\t\t\t\tmodified = 1;\n+\t\t\t\t} else\n \t\t\t\t\tdie (_(\"Path '%s' is in submodule '%.*s'\"),\n \t\t\t\t\t\tpathspec[j], len, ce->name);\n \t\t\t}\n \t\t}\n \t}\n+\treturn modified;\n }\n \n static void refresh(int verbose, const struct pathspec *pathspec)\n@@ -418,7 +421,16 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \n \tif (read_cache() < 0)\n \t\tdie(_(\"index file corrupt\"));\n-\ttreat_gitlinks(pathspec.raw);\n+\tif (treat_gitlinks(pathspec.raw))\n+\t\t/*\n+\t\t * HACK: treat_gitlinks strips the trailing slashes\n+\t\t * out of submodule entries but it only affects\n+\t\t * raw[]. Everything in pathspec.items is not touched.\n+\t\t * Re-init it to propagate the change. Long term, this\n+\t\t * function should be moved to pathspec.c and update\n+\t\t * everything in a consistent way.\n+\t\t */\n+\t\tinit_pathspec(&pathspec, pathspec.raw);\n \n \tif (add_new_files) {\n \t\tint baselen;\n-- 8< --\n"},{"id":"206461","messageId":"7v38y83ooo.fsf@alter.siamese.dyndns.org","threadId":"32574","inReplyTo":"20130110062838.GA11634@duynguyen-vnpc.dek-tpc.internal","subject":"Re: t7400 broken on pu (Mac OS X)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-10T17:58:31Z","receivedAt":"2013-01-10T17:58:31Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen <pclouds@gmail.com> writes:\n\n> On Wed, Jan 09, 2013 at 07:43:03PM +0100, Torsten Bögershausen wrote:\n>> The current pu fails on Mac OS, case insensitive FS.\n>> \n>> \n>> Bisecting points out\n>> commit 3f28e4fafc046284657945798d71c57608bee479\n>> [snip]\n>> Date:   Sun Jan 6 13:21:07 2013 +0700\n>> \n>>     Convert add_files_to_cache to take struct pathspec\n>> \n>\n> I can reproduce it by setting core.ignorecase to true. There is a bug\n> that I overlooked. Can you verify if this throw-away patch fixes it\n> for you? A proper fix will be in the reroll later.\n\nI can see why it is wrong to let pathspec.raw be rewritten without\nmaking matching change to the containing pathspec, but I find it\nstrange why it matters only on case-insensitive codepath.\n\nI agree with the \"Hack\" comment that the canonicalization should be\ndone at a higher level upfront.  Then ls-files does not need its own\nstrip_trailing_slash_from_submodules(), and check_path_for_gitlink()\ncan (and should---the callers of \"check_anything\" would not expect\nthe function to change things) stop rewriting its parameter.\n\nThanks for a quick response.\n\n> -- 8< --\n> diff --git a/builtin/add.c b/builtin/add.c\n> index 641037f..61cb8bd 100644\n> --- a/builtin/add.c\n> +++ b/builtin/add.c\n> @@ -155,12 +155,13 @@ static char *prune_directory(struct dir_struct *dir, const char **pathspec, int\n>  \treturn seen;\n>  }\n>  \n> -static void treat_gitlinks(const char **pathspec)\n> +static int treat_gitlinks(const char **pathspec)\n>  {\n>  \tint i;\n> +\tint modified = 0;\n>  \n>  \tif (!pathspec || !*pathspec)\n> -\t\treturn;\n> +\t\treturn modified;\n>  \n>  \tfor (i = 0; i < active_nr; i++) {\n>  \t\tstruct cache_entry *ce = active_cache[i];\n> @@ -171,15 +172,17 @@ static void treat_gitlinks(const char **pathspec)\n>  \t\t\t\tif (len2 <= len || pathspec[j][len] != '/' ||\n>  \t\t\t\t    memcmp(ce->name, pathspec[j], len))\n>  \t\t\t\t\tcontinue;\n> -\t\t\t\tif (len2 == len + 1)\n> +\t\t\t\tif (len2 == len + 1) {\n>  \t\t\t\t\t/* strip trailing slash */\n>  \t\t\t\t\tpathspec[j] = xstrndup(ce->name, len);\n> -\t\t\t\telse\n> +\t\t\t\t\tmodified = 1;\n> +\t\t\t\t} else\n>  \t\t\t\t\tdie (_(\"Path '%s' is in submodule '%.*s'\"),\n>  \t\t\t\t\t\tpathspec[j], len, ce->name);\n>  \t\t\t}\n>  \t\t}\n>  \t}\n> +\treturn modified;\n>  }\n>  \n>  static void refresh(int verbose, const struct pathspec *pathspec)\n> @@ -418,7 +421,16 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n>  \n>  \tif (read_cache() < 0)\n>  \t\tdie(_(\"index file corrupt\"));\n> -\ttreat_gitlinks(pathspec.raw);\n> +\tif (treat_gitlinks(pathspec.raw))\n> +\t\t/*\n> +\t\t * HACK: treat_gitlinks strips the trailing slashes\n> +\t\t * out of submodule entries but it only affects\n> +\t\t * raw[]. Everything in pathspec.items is not touched.\n> +\t\t * Re-init it to propagate the change. Long term, this\n> +\t\t * function should be moved to pathspec.c and update\n> +\t\t * everything in a consistent way.\n> +\t\t */\n> +\t\tinit_pathspec(&pathspec, pathspec.raw);\n>  \n>  \tif (add_new_files) {\n>  \t\tint baselen;\n> -- 8< --\n"},{"id":"206464","messageId":"50EF1145.6020002@web.de","threadId":"32574","inReplyTo":"7v38y83ooo.fsf@alter.siamese.dyndns.org","subject":"Re: t7400 broken on pu (Mac OS X)","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2013-01-10T19:06:45Z","receivedAt":"2013-01-10T19:06:45Z","isPatch":false,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 10.01.13 18:58, Junio C Hamano wrote:\n> Duy Nguyen <pclouds@gmail.com> writes:\n>\n>> On Wed, Jan 09, 2013 at 07:43:03PM +0100, Torsten Bögershausen wrote:\n>>> The current pu fails on Mac OS, case insensitive FS.\n>>>\n>>>\n>>> Bisecting points out\n>>> commit 3f28e4fafc046284657945798d71c57608bee479\n>>> [snip]\n>>> Date:   Sun Jan 6 13:21:07 2013 +0700\n>>>\n>>>     Convert add_files_to_cache to take struct pathspec\n>>>\n>> I can reproduce it by setting core.ignorecase to true. There is a bug\n>> that I overlooked. Can you verify if this throw-away patch fixes it\n>> for you? A proper fix will be in the reroll later.\n> I can see why it is wrong to let pathspec.raw be rewritten without\n> making matching change to the containing pathspec, but I find it\n> strange why it matters only on case-insensitive codepath.\n>\n> I agree with the \"Hack\" comment that the canonicalization should be\n> done at a higher level upfront.  Then ls-files does not need its own\n> strip_trailing_slash_from_submodules(), and check_path_for_gitlink()\n> can (and should---the callers of \"check_anything\" would not expect\n> the function to change things) stop rewriting its parameter.\n>\n> Thanks for a quick response.\n>\nThe patch fixes t7400.\nThanks from my side as well\n/Torsten\n"},{"id":"206505","messageId":"CACsJy8C4GQVQzwP3K1N_vP25MHkaJ4-oAbwrpfqEsqKHO4UXsQ@mail.gmail.com","threadId":"32574","inReplyTo":"7v38y83ooo.fsf@alter.siamese.dyndns.org","subject":"Re: t7400 broken on pu (Mac OS X)","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-01-11T11:03:47Z","receivedAt":"2013-01-11T11:03:47Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Jan 11, 2013 at 12:58 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> I can see why it is wrong to let pathspec.raw be rewritten without\n> making matching change to the containing pathspec, but I find it\n> strange why it matters only on case-insensitive codepath.\n\nYeah, I don't get it either. I can see that core.ignorecase exercises\nsome more code, but still fail to see the link. I should get to the\nbottom of this and write some tests to for core.ignorecase-only code.\n-- \nDuy\n"}]}