On Wed, Feb 24, 2016 at 1:21 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Stefan Beller <sbeller@xxxxxxxxxx> writes:
>
>> This is a regression introduced by 74703a1e4d (submodule: rewrite
>> `module_list` shell function in C, 2015-09-02).
>>
>> Add a test to ensure we list the right submodule when giving a specific
>> path spec.
>>
>> Reported-By: Caleb Jorden <cjorden@xxxxxxxxx>
>> Signed-off-by: Stefan Beller <sbeller@xxxxxxxxxx>
>> ---
>>
>> I developed this on top of current origin/master, though I can backport it
>> to 2.7 as well if desired.
>>
>> I do not remember the cause why we started to ignore a common prefix.
>
> The code you are removing with this patch is probably an
> optimization you copied from builtin/ls-files.c. When the
> optimization is used, the original also limits the list of paths to
> those that match the prefix by calling prune_cache(), but perhaps
> you didn't have a corresponding code in your copy?
I think that is a good explanation. So do we want to add the pruning
or use this patch to fixup the regression and wait until someone complains
about the speed penalty due to no optimization?
>
>>
>> Thanks,
>> Stefan
>>
>> builtin/submodule--helper.c | 10 ++--------
>> t/t7400-submodule-basic.sh | 25 +++++++++++++++++++++++++
>> 2 files changed, 27 insertions(+), 8 deletions(-)
>>
>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
>> index f4c3eff..ed764c9 100644
>> --- a/builtin/submodule--helper.c
>> +++ b/builtin/submodule--helper.c
>> @@ -22,17 +22,12 @@ static int module_list_compute(int argc, const char **argv,
>> struct module_list *list)
>> {
>> int i, result = 0;
>> - char *max_prefix, *ps_matched = NULL;
>> - int max_prefix_len;
>> + char *ps_matched = NULL;
>> parse_pathspec(pathspec, 0,
>> PATHSPEC_PREFER_FULL |
>> PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
>> prefix, argv);
>>
>> - /* Find common prefix for all pathspec's */
>> - max_prefix = common_prefix(pathspec);
>> - max_prefix_len = max_prefix ? strlen(max_prefix) : 0;
>> -
>> if (pathspec->nr)
>> ps_matched = xcalloc(pathspec->nr, 1);
>>
>> @@ -44,7 +39,7 @@ static int module_list_compute(int argc, const char **argv,
>>
>> if (!S_ISGITLINK(ce->ce_mode) ||
>> !match_pathspec(pathspec, ce->name, ce_namelen(ce),
>> - max_prefix_len, ps_matched, 1))
>> + 0, ps_matched, 1))
>> continue;
>>
>> ALLOC_GROW(list->entries, list->nr + 1, list->alloc);
>> @@ -57,7 +52,6 @@ static int module_list_compute(int argc, const char **argv,
>> */
>> i++;
>> }
>> - free(max_prefix);
>>
>> if (ps_matched && report_path_error(ps_matched, pathspec, prefix))
>> result = -1;
>> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
>> index 540771c..be82a75 100755
>> --- a/t/t7400-submodule-basic.sh
>> +++ b/t/t7400-submodule-basic.sh
>> @@ -999,5 +999,30 @@ test_expect_success 'submodule add clone shallow submodule' '
>> )
>> '
>>
>> +test_expect_success 'submodule helper list is not confused by common prefixes' '
>> + mkdir -p dir1/b &&
>> + (
>> + cd dir1/b &&
>> + git init &&
>> + echo hi >testfile2 &&
>> + git add . &&
>> + git commit -m "test1"
>> + ) &&
>> + mkdir -p dir2/b &&
>> + (
>> + cd dir2/b &&
>> + git init &&
>> + echo hello >testfile1 &&
>> + git add . &&
>> + git commit -m "test2"
>> + ) &&
>> + git submodule add /dir1/b dir1/b &&
>> + git submodule add /dir2/b dir2/b &&
>> + git commit -m "first submodule commit" &&
>> + git submodule--helper list dir1/b |cut -c51- >actual &&
>> + echo "dir1/b" >expect &&
>> + test_cmp expect actual
>> +'
>> +
>>
>> test_done
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html