threads / patch / 41501

patchsubmodule helper list: Respect correct path prefix

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

## tl;dr

5 messages between Feb 24, 2016 and Feb 24, 2016. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

Stefan Beller· Feb 24, 2016, 21:15 UTC · lore

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

Show changes to 2 files +27 −8

builtin/submodule--helper.c, t/t7400-submodule-basic.sh

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

Junio C Hamano· Feb 24, 2016, 21:21 UTC · re: Stefan Beller · lore

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

Stefan Beller <sbeller@google.com> writes:

Show 14 quoted lines

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

Show 83 quoted lines

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

Stefan Beller· Feb 24, 2016, 21:32 UTC · re: Junio C Hamano · lore

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

On Wed, Feb 24, 2016 at 1:21 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 22 quoted lines

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

Show 84 quoted lines

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

Junio C Hamano· Feb 24, 2016, 21:38 UTC · re: Stefan Beller · lore

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

Stefan Beller <sbeller@google.com> writes:

Show 27 quoted lines

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

Caleb Jorden· Feb 24, 2016, 22:18 UTC · re: Stefan Beller · lore

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

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:

Show 100 quoted lines

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