[PATCH] submodule helper list: Respect correct path prefix

[PATCH] submodule helper list: Respect correct path prefix

[Stefan Beller]

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@gmail.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---

 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.
 
 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
-- 
2.7.2.334.g7c0da37.dirty

Re: [PATCH] submodule helper list: Respect correct path prefix

[Junio C Hamano]

Stefan Beller <sbeller@google.com> 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@gmail.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>
>  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?

>  
>  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

Re: [PATCH] submodule helper list: Respect correct path prefix

[Stefan Beller]

On Wed, Feb 24, 2016 at 1:21 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> 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@gmail.com>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>
>>  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

Re: [PATCH] submodule helper list: Respect correct path prefix

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> On Wed, Feb 24, 2016 at 1:21 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Stefan Beller <sbeller@google.com> 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@gmail.com>
>>> Signed-off-by: Stefan Beller <sbeller@google.com>
>>> ---
>>>
>>>  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?

As I do not know offhand if the optimization, especially the pruning
part, applies to the context of this code the same way ls-files does
things (which treats the index read into core as a throw-away data),
we shouldn't even attempt to salvage the faulty half-optimization
until we understand what it involves to make it work.  So "disable
broken optimization and make simple way work correctly" is the good
first step, especially for a fix that is meant to go to 2.7.x
series.

We must first be sure that removing the faulty half-optimization is
the only thing we need to fix this breakage, though ;-)

Thanks.

Re: [PATCH] submodule helper list: Respect correct path prefix

[Caleb Jorden]

Stefan,

Thank you for the exceptionally quick response, and the patch!  As
your added test case verifies, this fixes my use case.  I will
continue to test this, and let you know if I see any other problems.

Thanks again.

Caleb Jorden
cjorden@gmail.com

On Wed, Feb 24, 2016 at 3:15 PM, Stefan Beller <sbeller@google.com> wrote:
> 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@gmail.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>
>  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.
>
>  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
> --
> 2.7.2.334.g7c0da37.dirty
>