{"thread":{"id":"41501","subject":"[PATCH] submodule helper list: Respect correct path prefix","startedAt":"2016-02-24T21:15:02Z","lastAt":"2016-02-24T22:18:16Z","messageCount":5,"participants":["Stefan Beller","Junio C Hamano","Caleb Jorden"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"279176","messageId":"1456348502-4529-1-git-send-email-sbeller@google.com","threadId":"41501","inReplyTo":null,"subject":"[PATCH] submodule helper list: Respect correct path prefix","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-24T21:15:02Z","receivedAt":"2016-02-24T21:15:02Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This is a regression introduced by 74703a1e4d (submodule: rewrite\n`module_list` shell function in C, 2015-09-02).\n\nAdd a test to ensure we list the right submodule when giving a specific\npath spec.\n\nReported-By: Caleb Jorden <cjorden@gmail.com>\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\n I developed this on top of current origin/master, though I can backport it\n to 2.7 as well if desired.\n \n I do not remember the cause why we started to ignore a common prefix.\n \n Thanks,\n Stefan\n  \n builtin/submodule--helper.c | 10 ++--------\n t/t7400-submodule-basic.sh  | 25 +++++++++++++++++++++++++\n 2 files changed, 27 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex f4c3eff..ed764c9 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -22,17 +22,12 @@ static int module_list_compute(int argc, const char **argv,\n \t\t\t       struct module_list *list)\n {\n \tint i, result = 0;\n-\tchar *max_prefix, *ps_matched = NULL;\n-\tint max_prefix_len;\n+\tchar *ps_matched = NULL;\n \tparse_pathspec(pathspec, 0,\n \t\t       PATHSPEC_PREFER_FULL |\n \t\t       PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,\n \t\t       prefix, argv);\n \n-\t/* Find common prefix for all pathspec's */\n-\tmax_prefix = common_prefix(pathspec);\n-\tmax_prefix_len = max_prefix ? strlen(max_prefix) : 0;\n-\n \tif (pathspec->nr)\n \t\tps_matched = xcalloc(pathspec->nr, 1);\n \n@@ -44,7 +39,7 @@ static int module_list_compute(int argc, const char **argv,\n \n \t\tif (!S_ISGITLINK(ce->ce_mode) ||\n \t\t    !match_pathspec(pathspec, ce->name, ce_namelen(ce),\n-\t\t\t\t    max_prefix_len, ps_matched, 1))\n+\t\t\t\t    0, ps_matched, 1))\n \t\t\tcontinue;\n \n \t\tALLOC_GROW(list->entries, list->nr + 1, list->alloc);\n@@ -57,7 +52,6 @@ static int module_list_compute(int argc, const char **argv,\n \t\t\t */\n \t\t\ti++;\n \t}\n-\tfree(max_prefix);\n \n \tif (ps_matched && report_path_error(ps_matched, pathspec, prefix))\n \t\tresult = -1;\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex 540771c..be82a75 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -999,5 +999,30 @@ test_expect_success 'submodule add clone shallow submodule' '\n \t)\n '\n \n+test_expect_success 'submodule helper list is not confused by common prefixes' '\n+\tmkdir -p dir1/b &&\n+\t(\n+\t\tcd dir1/b &&\n+\t\tgit init &&\n+\t\techo hi >testfile2 &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"test1\"\n+\t) &&\n+\tmkdir -p dir2/b &&\n+\t(\n+\t\tcd dir2/b &&\n+\t\tgit init &&\n+\t\techo hello >testfile1 &&\n+\t\tgit add .  &&\n+\t\tgit commit -m \"test2\"\n+\t) &&\n+\tgit submodule add /dir1/b dir1/b &&\n+\tgit submodule add /dir2/b dir2/b &&\n+\tgit commit -m \"first submodule commit\" &&\n+\tgit submodule--helper list dir1/b |cut -c51- >actual &&\n+\techo \"dir1/b\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n \n test_done\n-- \n2.7.2.334.g7c0da37.dirty\n"},{"id":"279179","messageId":"xmqqy4a9st2v.fsf@gitster.mtv.corp.google.com","threadId":"41501","inReplyTo":"1456348502-4529-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] submodule helper list: Respect correct path prefix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-24T21:21:12Z","receivedAt":"2016-02-24T21:21:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> This is a regression introduced by 74703a1e4d (submodule: rewrite\n> `module_list` shell function in C, 2015-09-02).\n>\n> Add a test to ensure we list the right submodule when giving a specific\n> path spec.\n>\n> Reported-By: Caleb Jorden <cjorden@gmail.com>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>\n>  I developed this on top of current origin/master, though I can backport it\n>  to 2.7 as well if desired.\n>  \n>  I do not remember the cause why we started to ignore a common prefix.\n\nThe code you are removing with this patch is probably an\noptimization you copied from builtin/ls-files.c.  When the\noptimization is used, the original also limits the list of paths to\nthose that match the prefix by calling prune_cache(), but perhaps\nyou didn't have a corresponding code in your copy?\n\n>  \n>  Thanks,\n>  Stefan\n>   \n>  builtin/submodule--helper.c | 10 ++--------\n>  t/t7400-submodule-basic.sh  | 25 +++++++++++++++++++++++++\n>  2 files changed, 27 insertions(+), 8 deletions(-)\n>\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index f4c3eff..ed764c9 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -22,17 +22,12 @@ static int module_list_compute(int argc, const char **argv,\n>  \t\t\t       struct module_list *list)\n>  {\n>  \tint i, result = 0;\n> -\tchar *max_prefix, *ps_matched = NULL;\n> -\tint max_prefix_len;\n> +\tchar *ps_matched = NULL;\n>  \tparse_pathspec(pathspec, 0,\n>  \t\t       PATHSPEC_PREFER_FULL |\n>  \t\t       PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,\n>  \t\t       prefix, argv);\n>  \n> -\t/* Find common prefix for all pathspec's */\n> -\tmax_prefix = common_prefix(pathspec);\n> -\tmax_prefix_len = max_prefix ? strlen(max_prefix) : 0;\n> -\n>  \tif (pathspec->nr)\n>  \t\tps_matched = xcalloc(pathspec->nr, 1);\n>  \n> @@ -44,7 +39,7 @@ static int module_list_compute(int argc, const char **argv,\n>  \n>  \t\tif (!S_ISGITLINK(ce->ce_mode) ||\n>  \t\t    !match_pathspec(pathspec, ce->name, ce_namelen(ce),\n> -\t\t\t\t    max_prefix_len, ps_matched, 1))\n> +\t\t\t\t    0, ps_matched, 1))\n>  \t\t\tcontinue;\n>  \n>  \t\tALLOC_GROW(list->entries, list->nr + 1, list->alloc);\n> @@ -57,7 +52,6 @@ static int module_list_compute(int argc, const char **argv,\n>  \t\t\t */\n>  \t\t\ti++;\n>  \t}\n> -\tfree(max_prefix);\n>  \n>  \tif (ps_matched && report_path_error(ps_matched, pathspec, prefix))\n>  \t\tresult = -1;\n> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\n> index 540771c..be82a75 100755\n> --- a/t/t7400-submodule-basic.sh\n> +++ b/t/t7400-submodule-basic.sh\n> @@ -999,5 +999,30 @@ test_expect_success 'submodule add clone shallow submodule' '\n>  \t)\n>  '\n>  \n> +test_expect_success 'submodule helper list is not confused by common prefixes' '\n> +\tmkdir -p dir1/b &&\n> +\t(\n> +\t\tcd dir1/b &&\n> +\t\tgit init &&\n> +\t\techo hi >testfile2 &&\n> +\t\tgit add . &&\n> +\t\tgit commit -m \"test1\"\n> +\t) &&\n> +\tmkdir -p dir2/b &&\n> +\t(\n> +\t\tcd dir2/b &&\n> +\t\tgit init &&\n> +\t\techo hello >testfile1 &&\n> +\t\tgit add .  &&\n> +\t\tgit commit -m \"test2\"\n> +\t) &&\n> +\tgit submodule add /dir1/b dir1/b &&\n> +\tgit submodule add /dir2/b dir2/b &&\n> +\tgit commit -m \"first submodule commit\" &&\n> +\tgit submodule--helper list dir1/b |cut -c51- >actual &&\n> +\techo \"dir1/b\" >expect &&\n> +\ttest_cmp expect actual\n> +'\n> +\n>  \n>  test_done\n"},{"id":"279181","messageId":"CAGZ79kbbKnips12CU6KZX39rAZ_O-pYy20nsSGCCf+1w5LzgxA@mail.gmail.com","threadId":"41501","inReplyTo":"xmqqy4a9st2v.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] submodule helper list: Respect correct path prefix","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-24T21:32:22Z","receivedAt":"2016-02-24T21:32:22Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Feb 24, 2016 at 1:21 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> This is a regression introduced by 74703a1e4d (submodule: rewrite\n>> `module_list` shell function in C, 2015-09-02).\n>>\n>> Add a test to ensure we list the right submodule when giving a specific\n>> path spec.\n>>\n>> Reported-By: Caleb Jorden <cjorden@gmail.com>\n>> Signed-off-by: Stefan Beller <sbeller@google.com>\n>> ---\n>>\n>>  I developed this on top of current origin/master, though I can backport it\n>>  to 2.7 as well if desired.\n>>\n>>  I do not remember the cause why we started to ignore a common prefix.\n>\n> The code you are removing with this patch is probably an\n> optimization you copied from builtin/ls-files.c.  When the\n> optimization is used, the original also limits the list of paths to\n> those that match the prefix by calling prune_cache(), but perhaps\n> you didn't have a corresponding code in your copy?\n\nI think that is a good explanation. So do we want to add the pruning\nor use this patch to fixup the regression and wait until someone complains\nabout the speed penalty due to no optimization?\n\n>\n>>\n>>  Thanks,\n>>  Stefan\n>>\n>>  builtin/submodule--helper.c | 10 ++--------\n>>  t/t7400-submodule-basic.sh  | 25 +++++++++++++++++++++++++\n>>  2 files changed, 27 insertions(+), 8 deletions(-)\n>>\n>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n>> index f4c3eff..ed764c9 100644\n>> --- a/builtin/submodule--helper.c\n>> +++ b/builtin/submodule--helper.c\n>> @@ -22,17 +22,12 @@ static int module_list_compute(int argc, const char **argv,\n>>                              struct module_list *list)\n>>  {\n>>       int i, result = 0;\n>> -     char *max_prefix, *ps_matched = NULL;\n>> -     int max_prefix_len;\n>> +     char *ps_matched = NULL;\n>>       parse_pathspec(pathspec, 0,\n>>                      PATHSPEC_PREFER_FULL |\n>>                      PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,\n>>                      prefix, argv);\n>>\n>> -     /* Find common prefix for all pathspec's */\n>> -     max_prefix = common_prefix(pathspec);\n>> -     max_prefix_len = max_prefix ? strlen(max_prefix) : 0;\n>> -\n>>       if (pathspec->nr)\n>>               ps_matched = xcalloc(pathspec->nr, 1);\n>>\n>> @@ -44,7 +39,7 @@ static int module_list_compute(int argc, const char **argv,\n>>\n>>               if (!S_ISGITLINK(ce->ce_mode) ||\n>>                   !match_pathspec(pathspec, ce->name, ce_namelen(ce),\n>> -                                 max_prefix_len, ps_matched, 1))\n>> +                                 0, ps_matched, 1))\n>>                       continue;\n>>\n>>               ALLOC_GROW(list->entries, list->nr + 1, list->alloc);\n>> @@ -57,7 +52,6 @@ static int module_list_compute(int argc, const char **argv,\n>>                        */\n>>                       i++;\n>>       }\n>> -     free(max_prefix);\n>>\n>>       if (ps_matched && report_path_error(ps_matched, pathspec, prefix))\n>>               result = -1;\n>> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\n>> index 540771c..be82a75 100755\n>> --- a/t/t7400-submodule-basic.sh\n>> +++ b/t/t7400-submodule-basic.sh\n>> @@ -999,5 +999,30 @@ test_expect_success 'submodule add clone shallow submodule' '\n>>       )\n>>  '\n>>\n>> +test_expect_success 'submodule helper list is not confused by common prefixes' '\n>> +     mkdir -p dir1/b &&\n>> +     (\n>> +             cd dir1/b &&\n>> +             git init &&\n>> +             echo hi >testfile2 &&\n>> +             git add . &&\n>> +             git commit -m \"test1\"\n>> +     ) &&\n>> +     mkdir -p dir2/b &&\n>> +     (\n>> +             cd dir2/b &&\n>> +             git init &&\n>> +             echo hello >testfile1 &&\n>> +             git add .  &&\n>> +             git commit -m \"test2\"\n>> +     ) &&\n>> +     git submodule add /dir1/b dir1/b &&\n>> +     git submodule add /dir2/b dir2/b &&\n>> +     git commit -m \"first submodule commit\" &&\n>> +     git submodule--helper list dir1/b |cut -c51- >actual &&\n>> +     echo \"dir1/b\" >expect &&\n>> +     test_cmp expect actual\n>> +'\n>> +\n>>\n>>  test_done\n"},{"id":"279182","messageId":"xmqqpovlssar.fsf@gitster.mtv.corp.google.com","threadId":"41501","inReplyTo":"CAGZ79kbbKnips12CU6KZX39rAZ_O-pYy20nsSGCCf+1w5LzgxA@mail.gmail.com","subject":"Re: [PATCH] submodule helper list: Respect correct path prefix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-24T21:38:04Z","receivedAt":"2016-02-24T21:38:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> On Wed, Feb 24, 2016 at 1:21 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Stefan Beller <sbeller@google.com> writes:\n>>\n>>> This is a regression introduced by 74703a1e4d (submodule: rewrite\n>>> `module_list` shell function in C, 2015-09-02).\n>>>\n>>> Add a test to ensure we list the right submodule when giving a specific\n>>> path spec.\n>>>\n>>> Reported-By: Caleb Jorden <cjorden@gmail.com>\n>>> Signed-off-by: Stefan Beller <sbeller@google.com>\n>>> ---\n>>>\n>>>  I developed this on top of current origin/master, though I can backport it\n>>>  to 2.7 as well if desired.\n>>>\n>>>  I do not remember the cause why we started to ignore a common prefix.\n>>\n>> The code you are removing with this patch is probably an\n>> optimization you copied from builtin/ls-files.c.  When the\n>> optimization is used, the original also limits the list of paths to\n>> those that match the prefix by calling prune_cache(), but perhaps\n>> you didn't have a corresponding code in your copy?\n>\n> I think that is a good explanation. So do we want to add the pruning\n> or use this patch to fixup the regression and wait until someone complains\n> about the speed penalty due to no optimization?\n\nAs I do not know offhand if the optimization, especially the pruning\npart, applies to the context of this code the same way ls-files does\nthings (which treats the index read into core as a throw-away data),\nwe shouldn't even attempt to salvage the faulty half-optimization\nuntil we understand what it involves to make it work.  So \"disable\nbroken optimization and make simple way work correctly\" is the good\nfirst step, especially for a fix that is meant to go to 2.7.x\nseries.\n\nWe must first be sure that removing the faulty half-optimization is\nthe only thing we need to fix this breakage, though ;-)\n\nThanks.\n"},{"id":"279185","messageId":"CABD8wQkU5J7ZYDuNW9=M=2dzR2tJ5v1zcSUnUHPeo=2aed4V_g@mail.gmail.com","threadId":"41501","inReplyTo":"1456348502-4529-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] submodule helper list: Respect correct path prefix","fromName":"Caleb Jorden","fromEmail":"cjorden@gmail.com","sentAt":"2016-02-24T22:18:16Z","receivedAt":"2016-02-24T22:18:16Z","isPatch":true,"sender":{"key":"cjorden@gmail.com","avatar":null},"body":"Stefan,\n\nThank you for the exceptionally quick response, and the patch!  As\nyour added test case verifies, this fixes my use case.  I will\ncontinue to test this, and let you know if I see any other problems.\n\nThanks again.\n\nCaleb Jorden\ncjorden@gmail.com\n\nOn Wed, Feb 24, 2016 at 3:15 PM, Stefan Beller <sbeller@google.com> wrote:\n> This is a regression introduced by 74703a1e4d (submodule: rewrite\n> `module_list` shell function in C, 2015-09-02).\n>\n> Add a test to ensure we list the right submodule when giving a specific\n> path spec.\n>\n> Reported-By: Caleb Jorden <cjorden@gmail.com>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>\n>  I developed this on top of current origin/master, though I can backport it\n>  to 2.7 as well if desired.\n>\n>  I do not remember the cause why we started to ignore a common prefix.\n>\n>  Thanks,\n>  Stefan\n>\n>  builtin/submodule--helper.c | 10 ++--------\n>  t/t7400-submodule-basic.sh  | 25 +++++++++++++++++++++++++\n>  2 files changed, 27 insertions(+), 8 deletions(-)\n>\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index f4c3eff..ed764c9 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -22,17 +22,12 @@ static int module_list_compute(int argc, const char **argv,\n>                                struct module_list *list)\n>  {\n>         int i, result = 0;\n> -       char *max_prefix, *ps_matched = NULL;\n> -       int max_prefix_len;\n> +       char *ps_matched = NULL;\n>         parse_pathspec(pathspec, 0,\n>                        PATHSPEC_PREFER_FULL |\n>                        PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,\n>                        prefix, argv);\n>\n> -       /* Find common prefix for all pathspec's */\n> -       max_prefix = common_prefix(pathspec);\n> -       max_prefix_len = max_prefix ? strlen(max_prefix) : 0;\n> -\n>         if (pathspec->nr)\n>                 ps_matched = xcalloc(pathspec->nr, 1);\n>\n> @@ -44,7 +39,7 @@ static int module_list_compute(int argc, const char **argv,\n>\n>                 if (!S_ISGITLINK(ce->ce_mode) ||\n>                     !match_pathspec(pathspec, ce->name, ce_namelen(ce),\n> -                                   max_prefix_len, ps_matched, 1))\n> +                                   0, ps_matched, 1))\n>                         continue;\n>\n>                 ALLOC_GROW(list->entries, list->nr + 1, list->alloc);\n> @@ -57,7 +52,6 @@ static int module_list_compute(int argc, const char **argv,\n>                          */\n>                         i++;\n>         }\n> -       free(max_prefix);\n>\n>         if (ps_matched && report_path_error(ps_matched, pathspec, prefix))\n>                 result = -1;\n> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\n> index 540771c..be82a75 100755\n> --- a/t/t7400-submodule-basic.sh\n> +++ b/t/t7400-submodule-basic.sh\n> @@ -999,5 +999,30 @@ test_expect_success 'submodule add clone shallow submodule' '\n>         )\n>  '\n>\n> +test_expect_success 'submodule helper list is not confused by common prefixes' '\n> +       mkdir -p dir1/b &&\n> +       (\n> +               cd dir1/b &&\n> +               git init &&\n> +               echo hi >testfile2 &&\n> +               git add . &&\n> +               git commit -m \"test1\"\n> +       ) &&\n> +       mkdir -p dir2/b &&\n> +       (\n> +               cd dir2/b &&\n> +               git init &&\n> +               echo hello >testfile1 &&\n> +               git add .  &&\n> +               git commit -m \"test2\"\n> +       ) &&\n> +       git submodule add /dir1/b dir1/b &&\n> +       git submodule add /dir2/b dir2/b &&\n> +       git commit -m \"first submodule commit\" &&\n> +       git submodule--helper list dir1/b |cut -c51- >actual &&\n> +       echo \"dir1/b\" >expect &&\n> +       test_cmp expect actual\n> +'\n> +\n>\n>  test_done\n> --\n> 2.7.2.334.g7c0da37.dirty\n>\n"}]}