{"thread":{"id":"56396","subject":"[PATCH] merge-recursive: use fspathcmp() in path_hashmap_cmp()","startedAt":"2021-08-28T21:30:57Z","lastAt":"2021-09-14T14:11:48Z","messageCount":15,"participants":["René Scharfe","Taylor Blau","Jeff King","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"434022","messageId":"512abaef-d71c-9308-6a62-f37b4de69e60@web.de","threadId":"56396","inReplyTo":null,"subject":"[PATCH] merge-recursive: use fspathcmp() in path_hashmap_cmp()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-08-28T21:30:49Z","receivedAt":"2021-08-28T21:30:57Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Call fspathcmp() instead of open-coding it.  This shortens the code and\nmakes it less repetitive.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n merge-recursive.c | 5 +----\n 1 file changed, 1 insertion(+), 4 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 3355d50e8a..840599fd53 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -55,10 +55,7 @@ static int path_hashmap_cmp(const void *cmp_data,\n \ta = container_of(eptr, const struct path_hashmap_entry, e);\n \tb = container_of(entry_or_key, const struct path_hashmap_entry, e);\n\n-\tif (ignore_case)\n-\t\treturn strcasecmp(a->path, key ? key : b->path);\n-\telse\n-\t\treturn strcmp(a->path, key ? key : b->path);\n+\treturn fspathcmp(a->path, key ? key : b->path);\n }\n\n /*\n--\n2.33.0\n\n"},{"id":"434028","messageId":"YSvsQcGNpCMZwS8o@nand.local","threadId":"56396","inReplyTo":"512abaef-d71c-9308-6a62-f37b4de69e60@web.de","subject":"Re: [PATCH] merge-recursive: use fspathcmp() in path_hashmap_cmp()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-08-29T20:21:21Z","receivedAt":"2021-08-29T20:21:25Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi René,\n\nOn Sat, Aug 28, 2021 at 11:30:49PM +0200, René Scharfe wrote:\n> Call fspathcmp() instead of open-coding it.  This shortens the code and\n> makes it less repetitive.\n>\n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n>  merge-recursive.c | 5 +----\n>  1 file changed, 1 insertion(+), 4 deletions(-)\n>\n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index 3355d50e8a..840599fd53 100644\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -55,10 +55,7 @@ static int path_hashmap_cmp(const void *cmp_data,\n>  \ta = container_of(eptr, const struct path_hashmap_entry, e);\n>  \tb = container_of(entry_or_key, const struct path_hashmap_entry, e);\n>\n> -\tif (ignore_case)\n> -\t\treturn strcasecmp(a->path, key ? key : b->path);\n> -\telse\n> -\t\treturn strcmp(a->path, key ? key : b->path);\n> +\treturn fspathcmp(a->path, key ? key : b->path);\n>  }\n\nLooks obviously right to me. I found another spot in\nt/helper/test-hashmap.c:test_entry_cmp() that could be cleaned up in the\nsame way. But this looks fine with or without the following diff:\n\ndiff --git a/t/helper/test-hashmap.c b/t/helper/test-hashmap.c\nindex 36ff07bd4b..ab34bdfecd 100644\n--- a/t/helper/test-hashmap.c\n+++ b/t/helper/test-hashmap.c\n@@ -28,10 +28,7 @@ static int test_entry_cmp(const void *cmp_data,\n        e1 = container_of(eptr, const struct test_entry, ent);\n        e2 = container_of(entry_or_key, const struct test_entry, ent);\n\n-       if (ignore_case)\n-               return strcasecmp(e1->key, key ? key : e2->key);\n-       else\n-               return strcmp(e1->key, key ? key : e2->key);\n+       return fspathcmp(e1->key, key ? key : e2->key);\n }\n\n static struct test_entry *alloc_test_entry(unsigned int hash,\n"},{"id":"434029","messageId":"YSv1XZJvYdYj1JtP@coredump.intra.peff.net","threadId":"56396","inReplyTo":"YSvsQcGNpCMZwS8o@nand.local","subject":"Re: [PATCH] merge-recursive: use fspathcmp() in path_hashmap_cmp()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-08-29T21:00:13Z","receivedAt":"2021-08-29T21:00:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Aug 29, 2021 at 04:21:21PM -0400, Taylor Blau wrote:\n\n> > +++ b/merge-recursive.c\n> > @@ -55,10 +55,7 @@ static int path_hashmap_cmp(const void *cmp_data,\n> [...]\n> \n> Looks obviously right to me. I found another spot in\n> t/helper/test-hashmap.c:test_entry_cmp() that could be cleaned up in the\n> same way. But this looks fine with or without the following diff:\n> \n> diff --git a/t/helper/test-hashmap.c b/t/helper/test-hashmap.c\n> index 36ff07bd4b..ab34bdfecd 100644\n> --- a/t/helper/test-hashmap.c\n> +++ b/t/helper/test-hashmap.c\n> @@ -28,10 +28,7 @@ static int test_entry_cmp(const void *cmp_data,\n>         e1 = container_of(eptr, const struct test_entry, ent);\n>         e2 = container_of(entry_or_key, const struct test_entry, ent);\n> \n> -       if (ignore_case)\n> -               return strcasecmp(e1->key, key ? key : e2->key);\n> -       else\n> -               return strcmp(e1->key, key ? key : e2->key);\n> +       return fspathcmp(e1->key, key ? key : e2->key);\n>  }\n> \n>  static struct test_entry *alloc_test_entry(unsigned int hash,\n\nMaybe also:\n\ndiff --git a/dir.c b/dir.c\nindex 03c4d21267..ee46290cbb 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -669,9 +669,7 @@ int pl_hashmap_cmp(const void *unused_cmp_data,\n \t\t\t ? ee1->patternlen\n \t\t\t : ee2->patternlen;\n \n-\tif (ignore_case)\n-\t\treturn strncasecmp(ee1->pattern, ee2->pattern, min_len);\n-\treturn strncmp(ee1->pattern, ee2->pattern, min_len);\n+\treturn fspathncmp(ee1->pattern, ee2->pattern, min_len);\n }\n \n static char *dup_and_filter_pattern(const char *pattern)\n\nThis is fun. :)\n\n-Peff\n"},{"id":"434038","messageId":"xmqqo89f7pxq.fsf@gitster.g","threadId":"56396","inReplyTo":"YSv1XZJvYdYj1JtP@coredump.intra.peff.net","subject":"Re: [PATCH] merge-recursive: use fspathcmp() in path_hashmap_cmp()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-30T00:10:25Z","receivedAt":"2021-08-30T00:10:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> -       if (ignore_case)\n>> -               return strcasecmp(e1->key, key ? key : e2->key);\n>> -       else\n>> -               return strcmp(e1->key, key ? key : e2->key);\n>> +       return fspathcmp(e1->key, key ? key : e2->key);\n>>  }\n>> \n>>  static struct test_entry *alloc_test_entry(unsigned int hash,\n>\n> Maybe also:\n>\n> diff --git a/dir.c b/dir.c\n> index 03c4d21267..ee46290cbb 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -669,9 +669,7 @@ int pl_hashmap_cmp(const void *unused_cmp_data,\n>  \t\t\t ? ee1->patternlen\n>  \t\t\t : ee2->patternlen;\n>  \n> -\tif (ignore_case)\n> -\t\treturn strncasecmp(ee1->pattern, ee2->pattern, min_len);\n> -\treturn strncmp(ee1->pattern, ee2->pattern, min_len);\n> +\treturn fspathncmp(ee1->pattern, ee2->pattern, min_len);\n>  }\n>  \n>  static char *dup_and_filter_pattern(const char *pattern)\n>\n> This is fun. :)\n\nYes.\n\nSo we found three of them in the existing code.  A Coccinelle rule\nmay be an overkill, I guess.\n"},{"id":"434110","messageId":"60e9baf4-4e76-71a8-e6bb-1af87486994d@web.de","threadId":"56396","inReplyTo":"YSvsQcGNpCMZwS8o@nand.local","subject":"Re: [PATCH] merge-recursive: use fspathcmp() in path_hashmap_cmp()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-08-30T15:09:34Z","receivedAt":"2021-08-30T15:09:44Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 29.08.21 um 22:21 schrieb Taylor Blau:\n> Hi René,\n>\n> On Sat, Aug 28, 2021 at 11:30:49PM +0200, René Scharfe wrote:\n>> Call fspathcmp() instead of open-coding it.  This shortens the code and\n>> makes it less repetitive.\n>>\n>> Signed-off-by: René Scharfe <l.s.r@web.de>\n>> ---\n>>  merge-recursive.c | 5 +----\n>>  1 file changed, 1 insertion(+), 4 deletions(-)\n>>\n>> diff --git a/merge-recursive.c b/merge-recursive.c\n>> index 3355d50e8a..840599fd53 100644\n>> --- a/merge-recursive.c\n>> +++ b/merge-recursive.c\n>> @@ -55,10 +55,7 @@ static int path_hashmap_cmp(const void *cmp_data,\n>>  \ta = container_of(eptr, const struct path_hashmap_entry, e);\n>>  \tb = container_of(entry_or_key, const struct path_hashmap_entry, e);\n>>\n>> -\tif (ignore_case)\n>> -\t\treturn strcasecmp(a->path, key ? key : b->path);\n>> -\telse\n>> -\t\treturn strcmp(a->path, key ? key : b->path);\n>> +\treturn fspathcmp(a->path, key ? key : b->path);\n>>  }\n>\n> Looks obviously right to me. I found another spot in\n> t/helper/test-hashmap.c:test_entry_cmp() that could be cleaned up in the\n> same way. But this looks fine with or without the following diff:\n>\n> diff --git a/t/helper/test-hashmap.c b/t/helper/test-hashmap.c\n> index 36ff07bd4b..ab34bdfecd 100644\n> --- a/t/helper/test-hashmap.c\n> +++ b/t/helper/test-hashmap.c\n> @@ -28,10 +28,7 @@ static int test_entry_cmp(const void *cmp_data,\n>         e1 = container_of(eptr, const struct test_entry, ent);\n>         e2 = container_of(entry_or_key, const struct test_entry, ent);\n>\n> -       if (ignore_case)\n> -               return strcasecmp(e1->key, key ? key : e2->key);\n> -       else\n> -               return strcmp(e1->key, key ? key : e2->key);\n> +       return fspathcmp(e1->key, key ? key : e2->key);\n>  }\n>\n>  static struct test_entry *alloc_test_entry(unsigned int hash,\n>\n\nThat's a local variable named \"ignore_case\", not the one declared in\nenvironment.c that fspathcmp() uses, so this would change the behavior.\nThe helper code does not include cache.h, so this is not even a case of\nvariable shadowing, just two different variables for similar purposes\nin different places having the same name.\n\nRené\n"},{"id":"434119","messageId":"xmqqeeaa6fey.fsf@gitster.g","threadId":"56396","inReplyTo":"YSvsQcGNpCMZwS8o@nand.local","subject":"Re: [PATCH] merge-recursive: use fspathcmp() in path_hashmap_cmp()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-30T16:55:17Z","receivedAt":"2021-08-30T16:55:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> Looks obviously right to me. I found another spot in\n> t/helper/test-hashmap.c:test_entry_cmp() that could be cleaned up in the\n> same way. But this looks fine with or without the following diff:\n\n> diff --git a/t/helper/test-hashmap.c b/t/helper/test-hashmap.c\n> index 36ff07bd4b..ab34bdfecd 100644\n> --- a/t/helper/test-hashmap.c\n> +++ b/t/helper/test-hashmap.c\n> @@ -28,10 +28,7 @@ static int test_entry_cmp(const void *cmp_data,\n>         e1 = container_of(eptr, const struct test_entry, ent);\n>         e2 = container_of(entry_or_key, const struct test_entry, ent);\n>\n> -       if (ignore_case)\n> -               return strcasecmp(e1->key, key ? key : e2->key);\n> -       else\n> -               return strcmp(e1->key, key ? key : e2->key);\n> +       return fspathcmp(e1->key, key ? key : e2->key);\n\nSorry but I think this patch is wrong.  Before the precontext of the\npatch, there is a local variable decl for ignore_case---the existing\ncode looks at ignore_case that is different from the global\nignore_case fspathcmp() looks at.\n\nAdmittedly, it was probably not an excellent idea to give a name so\nbland and unremarkable, 'ignore_case', to a global that affects so\nmany code paths in the system.  But the variable is already very\nestablished that renaming it would not contribute to improving the\ncode at all.\n\nIt however may not be a bad idea to catch these code paths where a\nlocal variable masks 'ignore_case' (and possibly other globals) and\nrename these local ones to avoid a mistake like this.\n\nThanks.\n"},{"id":"434141","messageId":"YS0hMILaN2ODfM3y@coredump.intra.peff.net","threadId":"56396","inReplyTo":"60e9baf4-4e76-71a8-e6bb-1af87486994d@web.de","subject":"Re: [PATCH] merge-recursive: use fspathcmp() in path_hashmap_cmp()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-08-30T18:19:28Z","receivedAt":"2021-08-30T18:20:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 30, 2021 at 05:09:34PM +0200, René Scharfe wrote:\n\n> > diff --git a/t/helper/test-hashmap.c b/t/helper/test-hashmap.c\n> > index 36ff07bd4b..ab34bdfecd 100644\n> > --- a/t/helper/test-hashmap.c\n> > +++ b/t/helper/test-hashmap.c\n> > @@ -28,10 +28,7 @@ static int test_entry_cmp(const void *cmp_data,\n> >         e1 = container_of(eptr, const struct test_entry, ent);\n> >         e2 = container_of(entry_or_key, const struct test_entry, ent);\n> >\n> > -       if (ignore_case)\n> > -               return strcasecmp(e1->key, key ? key : e2->key);\n> > -       else\n> > -               return strcmp(e1->key, key ? key : e2->key);\n> > +       return fspathcmp(e1->key, key ? key : e2->key);\n> >  }\n> >\n> >  static struct test_entry *alloc_test_entry(unsigned int hash,\n> >\n> \n> That's a local variable named \"ignore_case\", not the one declared in\n> environment.c that fspathcmp() uses, so this would change the behavior.\n> The helper code does not include cache.h, so this is not even a case of\n> variable shadowing, just two different variables for similar purposes\n> in different places having the same name.\n\nYikes, good catch. Perhaps it's overkill, but I wonder if a comment\nlike:\n\n  /*\n   * Do not use fspathcmp() here; our behavior depends on the local\n   * ignore_case variable, not the usual Git-wide global.\n   */\n\nwould help.\n\nI double-checked the spot I suggested. I think it is actually using the\nglobal (though I got it right through sheer luck).\n\n-Peff\n"},{"id":"434142","messageId":"8d2e0876-9441-9665-ebb1-8cb28902014b@web.de","threadId":"56396","inReplyTo":"xmqqeeaa6fey.fsf@gitster.g","subject":"Re: [PATCH] merge-recursive: use fspathcmp() in path_hashmap_cmp()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-08-30T18:22:25Z","receivedAt":"2021-08-30T18:22:40Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 30.08.21 um 18:55 schrieb Junio C Hamano:\n> Taylor Blau <me@ttaylorr.com> writes:\n>\n>> Looks obviously right to me. I found another spot in\n>> t/helper/test-hashmap.c:test_entry_cmp() that could be cleaned up in the\n>> same way. But this looks fine with or without the following diff:\n>\n>> diff --git a/t/helper/test-hashmap.c b/t/helper/test-hashmap.c\n>> index 36ff07bd4b..ab34bdfecd 100644\n>> --- a/t/helper/test-hashmap.c\n>> +++ b/t/helper/test-hashmap.c\n>> @@ -28,10 +28,7 @@ static int test_entry_cmp(const void *cmp_data,\n>>         e1 = container_of(eptr, const struct test_entry, ent);\n>>         e2 = container_of(entry_or_key, const struct test_entry, ent);\n>>\n>> -       if (ignore_case)\n>> -               return strcasecmp(e1->key, key ? key : e2->key);\n>> -       else\n>> -               return strcmp(e1->key, key ? key : e2->key);\n>> +       return fspathcmp(e1->key, key ? key : e2->key);\n>\n> Sorry but I think this patch is wrong.  Before the precontext of the\n> patch, there is a local variable decl for ignore_case---the existing\n> code looks at ignore_case that is different from the global\n> ignore_case fspathcmp() looks at.\n>\n> Admittedly, it was probably not an excellent idea to give a name so\n> bland and unremarkable, 'ignore_case', to a global that affects so\n> many code paths in the system.  But the variable is already very\n> established that renaming it would not contribute to improving the\n> code at all.\n>\n> It however may not be a bad idea to catch these code paths where a\n> local variable masks 'ignore_case' (and possibly other globals) and\n> rename these local ones to avoid a mistake like this.\n\nThe name itself is OK, I think, but using it at global scope is\nconfusing.  -Wshadow can help find such cases, but not this one, as\ntest-hashmap.c doesn't include the global declaration.  Moving the\nglobal into a struct to provide a poor man's namespace would fix this\nfor all namesakes, assisted by the compiler.  We'd then access it as\nthe_config.ignore_case or even the_config.core.ignore_case.\n\nMoving all config-related variables would be quite noisy, I guess,\nand probably conflict with lots of in-flight patches, but might be\nworth it.\n\nRené\n"},{"id":"434163","messageId":"YS1EVq2Gz+sPhw3c@coredump.intra.peff.net","threadId":"56396","inReplyTo":"8d2e0876-9441-9665-ebb1-8cb28902014b@web.de","subject":"Re: [PATCH] merge-recursive: use fspathcmp() in path_hashmap_cmp()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-08-30T20:49:26Z","receivedAt":"2021-08-30T20:49:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 30, 2021 at 08:22:25PM +0200, René Scharfe wrote:\n\n> > It however may not be a bad idea to catch these code paths where a\n> > local variable masks 'ignore_case' (and possibly other globals) and\n> > rename these local ones to avoid a mistake like this.\n> \n> The name itself is OK, I think, but using it at global scope is\n> confusing.  -Wshadow can help find such cases, but not this one, as\n> test-hashmap.c doesn't include the global declaration.  Moving the\n> global into a struct to provide a poor man's namespace would fix this\n> for all namesakes, assisted by the compiler.  We'd then access it as\n> the_config.ignore_case or even the_config.core.ignore_case.\n> \n> Moving all config-related variables would be quite noisy, I guess,\n> and probably conflict with lots of in-flight patches, but might be\n> worth it.\n\nReally most of these ought to be in the repository struct anyway, I\nwould think. The value of ignore_case comes from core.ignorecase, which\nis going to be repository-specific. We are probably doing the wrong\nthing already by looking at the parent core.ignorecase value when\noperating in an in-process submodule, but nobody noticed because it's\nquite unlikely for a submodule to have a different setting than the\nparent.\n\n-Peff\n"},{"id":"435560","messageId":"8bd13e99-208b-4c22-90e6-28227593e9c4@web.de","threadId":"56396","inReplyTo":"YS1EVq2Gz+sPhw3c@coredump.intra.peff.net","subject":"Re: [PATCH] merge-recursive: use fspathcmp() in path_hashmap_cmp()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-09-11T16:08:46Z","receivedAt":"2021-09-11T16:09:02Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 30.08.21 um 22:49 schrieb Jeff King:\n> On Mon, Aug 30, 2021 at 08:22:25PM +0200, René Scharfe wrote:\n>\n>>> It however may not be a bad idea to catch these code paths where a\n>>> local variable masks 'ignore_case' (and possibly other globals) and\n>>> rename these local ones to avoid a mistake like this.\n>>\n>> The name itself is OK, I think, but using it at global scope is\n>> confusing.  -Wshadow can help find such cases, but not this one, as\n>> test-hashmap.c doesn't include the global declaration.  Moving the\n>> global into a struct to provide a poor man's namespace would fix this\n>> for all namesakes, assisted by the compiler.  We'd then access it as\n>> the_config.ignore_case or even the_config.core.ignore_case.\n>>\n>> Moving all config-related variables would be quite noisy, I guess,\n>> and probably conflict with lots of in-flight patches, but might be\n>> worth it.\n>\n> Really most of these ought to be in the repository struct anyway, I\n> would think. The value of ignore_case comes from core.ignorecase, which\n> is going to be repository-specific. We are probably doing the wrong\n> thing already by looking at the parent core.ignorecase value when\n> operating in an in-process submodule, but nobody noticed because it's\n> quite unlikely for a submodule to have a different setting than the\n> parent.\n\nGood point.  So fspathcmp() and friends would need a repo parameter. :-|\n\nRené\n"},{"id":"435708","messageId":"nycvar.QRO.7.76.6.2109131335260.55@tvgsbejvaqbjf.bet","threadId":"56396","inReplyTo":"8bd13e99-208b-4c22-90e6-28227593e9c4@web.de","subject":"Re: [PATCH] merge-recursive: use fspathcmp() in path_hashmap_cmp()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-09-13T11:37:48Z","receivedAt":"2021-09-13T11:38:07Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi René,\n\nOn Sat, 11 Sep 2021, René Scharfe wrote:\n\n> Am 30.08.21 um 22:49 schrieb Jeff King:\n> > On Mon, Aug 30, 2021 at 08:22:25PM +0200, René Scharfe wrote:\n> >\n> >>> It however may not be a bad idea to catch these code paths where a\n> >>> local variable masks 'ignore_case' (and possibly other globals) and\n> >>> rename these local ones to avoid a mistake like this.\n> >>\n> >> The name itself is OK, I think, but using it at global scope is\n> >> confusing.  -Wshadow can help find such cases, but not this one, as\n> >> test-hashmap.c doesn't include the global declaration.  Moving the\n> >> global into a struct to provide a poor man's namespace would fix this\n> >> for all namesakes, assisted by the compiler.  We'd then access it as\n> >> the_config.ignore_case or even the_config.core.ignore_case.\n> >>\n> >> Moving all config-related variables would be quite noisy, I guess,\n> >> and probably conflict with lots of in-flight patches, but might be\n> >> worth it.\n> >\n> > Really most of these ought to be in the repository struct anyway, I\n> > would think. The value of ignore_case comes from core.ignorecase, which\n> > is going to be repository-specific. We are probably doing the wrong\n> > thing already by looking at the parent core.ignorecase value when\n> > operating in an in-process submodule, but nobody noticed because it's\n> > quite unlikely for a submodule to have a different setting than the\n> > parent.\n>\n> Good point.  So fspathcmp() and friends would need a repo parameter. :-|\n\nYes, we will eventually have to pass `struct repository *r` into a _lot_\nof call chains. It'll be a disruptive change, yet if the submodule folks\ntruly want to aim for in-process recursive treatment of submodules, there\nis no alternative.\n\nFWIW on Windows there are other potentially repository-specific settings\nthat are relevant in similar situations. For example, there is\n`core.symlinks`.\n\nCiao,\nDscho\n"},{"id":"435747","messageId":"YT+FzPT0RCP2PdNL@coredump.intra.peff.net","threadId":"56396","inReplyTo":"nycvar.QRO.7.76.6.2109131335260.55@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] merge-recursive: use fspathcmp() in path_hashmap_cmp()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-13T17:09:32Z","receivedAt":"2021-09-13T17:09:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 13, 2021 at 01:37:48PM +0200, Johannes Schindelin wrote:\n\n> > Good point.  So fspathcmp() and friends would need a repo parameter. :-|\n> \n> Yes, we will eventually have to pass `struct repository *r` into a _lot_\n> of call chains. It'll be a disruptive change, yet if the submodule folks\n> truly want to aim for in-process recursive treatment of submodules, there\n> is no alternative.\n> \n> FWIW on Windows there are other potentially repository-specific settings\n> that are relevant in similar situations. For example, there is\n> `core.symlinks`.\n\nAnother approach is to stuff the appropriate globals into the repository\nstruct, and then \"push\" onto the global the_repository pointer, treating\nit like a stack. And then low-level code is free to use that global\ncontext, even if it wasn't passed in.\n\nThat helps the primary use case of \"now I need to do something in a\nsub-module, but I'd like to do it in-process\". But it's not without\nchallenges:\n\n  - code which acts at the boundary of a submodule and a superproject\n    may be more awkward (since only one of them can be \"the current\n    repository\" at a time).\n\n  - it's a challenge with threading (an obvious problem would be a\n    multi-threaded grep which wanted to descend into a submodule). Using\n    a thread-local global for the_repository might solve that.\n\nIt's possible that this is a terrible direction to go, so I'm not\nnecessarily endorsing it, but just offering it as a possibility to think\nabout. The trickiest thing is that any devil would likely be in the\ndetails, and we wouldn't know until proceeding for a while along that\npath. Whereas passing around a context struct, while verbose and\nannoying, is a well-understood construct.\n\n-Peff\n"},{"id":"435785","messageId":"xmqqy280dz8s.fsf@gitster.g","threadId":"56396","inReplyTo":"YT+FzPT0RCP2PdNL@coredump.intra.peff.net","subject":"Re: [PATCH] merge-recursive: use fspathcmp() in path_hashmap_cmp()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-13T19:58:11Z","receivedAt":"2021-09-13T19:58:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Another approach is to stuff the appropriate globals into the repository\n> struct, and then \"push\" onto the global the_repository pointer, treating\n> it like a stack. And then low-level code is free to use that global\n> context, even if it wasn't passed in.\n> ...\n>   - it's a challenge with threading (an obvious problem would be a\n>     multi-threaded grep which wanted to descend into a submodule). Using\n>     a thread-local global for the_repository might solve that.\n>\n> It's possible that this is a terrible direction to go, so I'm not\n> necessarily endorsing it, but just offering it as a possibility to think\n> about. The trickiest thing is that any devil would likely be in the\n> details, and we wouldn't know until proceeding for a while along that\n> path. Whereas passing around a context struct, while verbose and\n> annoying, is a well-understood construct.\n\nI agree that it is cute, thread-unsafe, and tricky.  Let's leave it\nat an interesting thought experiment for now.\n\nThanks.\n\n"},{"id":"435836","messageId":"nycvar.QRO.7.76.6.2109141215140.55@tvgsbejvaqbjf.bet","threadId":"56396","inReplyTo":"YT+FzPT0RCP2PdNL@coredump.intra.peff.net","subject":"Re: [PATCH] merge-recursive: use fspathcmp() in path_hashmap_cmp()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-09-14T10:18:35Z","receivedAt":"2021-09-14T10:19:07Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Mon, 13 Sep 2021, Jeff King wrote:\n\n> On Mon, Sep 13, 2021 at 01:37:48PM +0200, Johannes Schindelin wrote:\n>\n> > > Good point.  So fspathcmp() and friends would need a repo parameter. :-|\n> >\n> > Yes, we will eventually have to pass `struct repository *r` into a _lot_\n> > of call chains. It'll be a disruptive change, yet if the submodule folks\n> > truly want to aim for in-process recursive treatment of submodules, there\n> > is no alternative.\n> >\n> > FWIW on Windows there are other potentially repository-specific settings\n> > that are relevant in similar situations. For example, there is\n> > `core.symlinks`.\n>\n> Another approach is to stuff the appropriate globals into the repository\n> struct, and then \"push\" onto the global the_repository pointer, treating\n> it like a stack. And then low-level code is free to use that global\n> context, even if it wasn't passed in.\n>\n> That helps the primary use case of \"now I need to do something in a\n> sub-module, but I'd like to do it in-process\". But it's not without\n> challenges:\n>\n>   - code which acts at the boundary of a submodule and a superproject\n>     may be more awkward (since only one of them can be \"the current\n>     repository\" at a time).\n>\n>   - it's a challenge with threading (an obvious problem would be a\n>     multi-threaded grep which wanted to descend into a submodule). Using\n>     a thread-local global for the_repository might solve that.\n>\n> It's possible that this is a terrible direction to go, so I'm not\n> necessarily endorsing it, but just offering it as a possibility to think\n> about. The trickiest thing is that any devil would likely be in the\n> details, and we wouldn't know until proceeding for a while along that\n> path. Whereas passing around a context struct, while verbose and\n> annoying, is a well-understood construct.\n\nI would not so far as to call it a terrible direction. It is definitely\nworth a thought or two.\n\nAt the end of the day, I fear that it is too tricky in practice, though.\n\nSeeing as there seems to be some appetite for refactoring Git's code on\nthis list, I am thinking that the `struct repository *r` direction might\nbe the one to go for. And I mean like \"move the globals into that struct\"\nas opposed to introducing that stack you talked about. It would even be a\nrefactoring where I would understand the motivation, and agree with it,\ntoo.\n\nCiao,\nDscho\n"},{"id":"435850","messageId":"YUCtoa4Wv1GWT1LE@coredump.intra.peff.net","threadId":"56396","inReplyTo":"nycvar.QRO.7.76.6.2109141215140.55@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] merge-recursive: use fspathcmp() in path_hashmap_cmp()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-14T14:11:45Z","receivedAt":"2021-09-14T14:11:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 14, 2021 at 12:18:35PM +0200, Johannes Schindelin wrote:\n\n> Seeing as there seems to be some appetite for refactoring Git's code on\n> this list, I am thinking that the `struct repository *r` direction might\n> be the one to go for. And I mean like \"move the globals into that struct\"\n> as opposed to introducing that stack you talked about. It would even be a\n> refactoring where I would understand the motivation, and agree with it,\n> too.\n\nOh, definitely. Regardless of whether step 2 is \"pass around the\nrepository struct\" or \"treat the global repository struct as a stack\",\nstep 1 must be putting repository-related globals into the struct. I\ndon't think there can be any solution that doesn't start with that. :)\n\nAnd I think it can even be done incrementally with very little impact.\nJust s/ignore_case/the_repository->ignore_case/ in the use-sites is an\nimprovement over the status quo. Even though it doesn't _fix_ anything,\nnow we can easily see where the dependencies on repo-variables are. And\nof course follow-on steps to make sure we are passing around and\naccessing the right repository struct are then welcome.\n\n-Peff\n"}]}