[PATCH 1/2] git-submodule: forward exit code of git-submodule--helper more faithfully

[PATCH 1/2] git-submodule: forward exit code of git-submodule--helper more faithfully

[Johannes Sixt]

git-submodule--helper is invoked as the upstream of a pipe in several
places. Usually, the failure of a program in this position is not
detected by the shell. For this reason, the code inserts a token in the
output stream when git-submodule--helper fails that is detected
downstream, where the shell script is quit with exit code 1.

There happens to be a bug in git-submodule--helper that leads to a
segmentation fault. The test suite triggers the crash in several places,
all of which are protected by 'test_must_fail'. But due to the inspecific
exit code 1, the crash remains undiagnosed.

Extend the failure protocol such that git-submodule--helper's exit code
is passed downstream (only in the case of failure). This enables the
downstream to use it as its own exit code, and 'test_must_fail' to
identify the segmentation fault as an unexpected failure.

The bug itself is fixed in the next commit.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
When you run ./t7400-submodule-basic.sh -v, you will notice this output:

fatal: destination path '/home/jsixt/Src/git/git/t/trash directory.t7400-submodule-basic/init' already exists and is not an empty directory.
fatal: clone of './.subrepo' into submodule path '/home/jsixt/Src/git/git/t/trash directory.t7400-submodule-basic/init' failed
Failed to clone 'init'. Retry scheduled
fatal: destination path '/home/jsixt/Src/git/git/t/trash directory.t7400-submodule-basic/init' already exists and is not an empty directory.
fatal: clone of './.subrepo' into submodule path '/home/jsixt/Src/git/git/t/trash directory.t7400-submodule-basic/init' failed
/home/jsixt/Src/git/git/git-submodule: line 494: 21757 Segmentation fault      git submodule--helper update-clone ${GIT_QUIET:+--quiet} ${wt_prefix:+--prefix "$wt_prefix"} ${prefix:+--recursive-prefix "$prefix"} ${update:+--update "$update"} ${reference:+--reference "$reference"} ${depth:+--depth "$depth"} ${recommend_shallow:+"$recommend_shallow"} ${jobs:+$jobs} "$@"
ok 32 - update should fail when path is used by a file

Note the segmentation fault. This mini-series addresses the issue.

Noticed on Windows because it "visualizes" segfaults even for
command line programs.

 git-submodule.sh            | 22 +++++++++++-----------
 t/t5815-submodule-protos.sh |  4 ++--
 t/t7400-submodule-basic.sh  |  4 ++--
 3 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 4ec7546..0a0e12d 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -49,7 +49,7 @@ die_if_unmatched ()
 {
 	if test "$1" = "#unmatched"
 	then
-		exit 1
+		exit ${2:-1}
 	fi
 }
 
@@ -312,11 +312,11 @@ cmd_foreach()
 
 	{
 		git submodule--helper list --prefix "$wt_prefix" ||
-		echo "#unmatched"
+		echo "#unmatched" $?
 	} |
 	while read mode sha1 stage sm_path
 	do
-		die_if_unmatched "$mode"
+		die_if_unmatched "$mode" "$sha1"
 		if test -e "$sm_path"/.git
 		then
 			displaypath=$(git submodule--helper relative-path "$prefix$sm_path" "$wt_prefix")
@@ -423,11 +423,11 @@ cmd_deinit()
 
 	{
 		git submodule--helper list --prefix "$wt_prefix" "$@" ||
-		echo "#unmatched"
+		echo "#unmatched" $?
 	} |
 	while read mode sha1 stage sm_path
 	do
-		die_if_unmatched "$mode"
+		die_if_unmatched "$mode" "$sha1"
 		name=$(git submodule--helper name "$sm_path") || exit
 
 		displaypath=$(git submodule--helper relative-path "$sm_path" "$wt_prefix")
@@ -581,12 +581,12 @@ cmd_update()
 		${depth:+--depth "$depth"} \
 		${recommend_shallow:+"$recommend_shallow"} \
 		${jobs:+$jobs} \
-		"$@" || echo "#unmatched"
+		"$@" || echo "#unmatched" $?
 	} | {
 	err=
 	while read mode sha1 stage just_cloned sm_path
 	do
-		die_if_unmatched "$mode"
+		die_if_unmatched "$mode" "$sha1"
 
 		name=$(git submodule--helper name "$sm_path") || exit
 		url=$(git config submodule."$name".url)
@@ -994,11 +994,11 @@ cmd_status()
 
 	{
 		git submodule--helper list --prefix "$wt_prefix" "$@" ||
-		echo "#unmatched"
+		echo "#unmatched" $?
 	} |
 	while read mode sha1 stage sm_path
 	do
-		die_if_unmatched "$mode"
+		die_if_unmatched "$mode" "$sha1"
 		name=$(git submodule--helper name "$sm_path") || exit
 		url=$(git config submodule."$name".url)
 		displaypath=$(git submodule--helper relative-path "$prefix$sm_path" "$wt_prefix")
@@ -1075,11 +1075,11 @@ cmd_sync()
 	cd_to_toplevel
 	{
 		git submodule--helper list --prefix "$wt_prefix" "$@" ||
-		echo "#unmatched"
+		echo "#unmatched" $?
 	} |
 	while read mode sha1 stage sm_path
 	do
-		die_if_unmatched "$mode"
+		die_if_unmatched "$mode" "$sha1"
 		name=$(git submodule--helper name "$sm_path")
 		url=$(git config -f .gitmodules --get submodule."$name".url)
 
diff --git a/t/t5815-submodule-protos.sh b/t/t5815-submodule-protos.sh
index 06f55a1..112cf40 100755
--- a/t/t5815-submodule-protos.sh
+++ b/t/t5815-submodule-protos.sh
@@ -18,7 +18,7 @@ test_expect_success 'setup repository with submodules' '
 	git commit -m "add submodules"
 '
 
-test_expect_success 'clone with recurse-submodules fails' '
+test_expect_failure 'clone with recurse-submodules fails' '
 	test_must_fail git clone --recurse-submodules . dst
 '
 
@@ -32,7 +32,7 @@ test_expect_success 'update of ssh allowed' '
 	git -C dst submodule update ssh-module
 '
 
-test_expect_success 'update of ext not allowed' '
+test_expect_failure 'update of ext not allowed' '
 	test_must_fail git -C dst submodule update ext-module
 '
 
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index b77cce8..7c8b90b 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -352,7 +352,7 @@ test_expect_success 'sync should fail with unknown submodule' '
 	test_failure_with_unknown_submodule sync
 '
 
-test_expect_success 'update should fail when path is used by a file' '
+test_expect_failure 'update should fail when path is used by a file' '
 	echo hello >expect &&
 
 	echo "hello" >init &&
@@ -361,7 +361,7 @@ test_expect_success 'update should fail when path is used by a file' '
 	test_cmp expect init
 '
 
-test_expect_success 'update should fail when path is used by a nonempty directory' '
+test_expect_failure 'update should fail when path is used by a nonempty directory' '
 	echo hello >expect &&
 
 	rm -fr init &&
-- 
2.9.0.443.ga8520ad

[PATCH 2/2] submodule-helper: fix indexing in clone retry error reporting path

[Johannes Sixt]

'git submodule--helper update-clone' has logic to retry failed clones
a second time. For this purpose, there is a list of submodules to clone,
and a second list that is filled with the submodules to retry. Within
these lists, the submodules are identified by an index as if both lists
were just appended.

This works nicely except when the second clone attempt fails as well. To
report an error, the identifying index must be adjusted by an offset so
that it can be used as an index into the second list. However, the
calculation uses the logical total length of the lists so that the result
always points one past the end of the second list.

Pick the correct index.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 builtin/submodule--helper.c | 2 +-
 t/t5815-submodule-protos.sh | 4 ++--
 t/t7400-submodule-basic.sh  | 4 ++--
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index b22352b..6f6d67a 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -795,7 +795,7 @@ static int update_clone_task_finished(int result,
 		suc->failed_clones[suc->failed_clones_nr++] = ce;
 		return 0;
 	} else {
-		idx = suc->current - suc->list.nr;
+		idx -= suc->list.nr;
 		ce  = suc->failed_clones[idx];
 		strbuf_addf(err, _("Failed to clone '%s' a second time, aborting"),
 			    ce->name);
diff --git a/t/t5815-submodule-protos.sh b/t/t5815-submodule-protos.sh
index 112cf40..06f55a1 100755
--- a/t/t5815-submodule-protos.sh
+++ b/t/t5815-submodule-protos.sh
@@ -18,7 +18,7 @@ test_expect_success 'setup repository with submodules' '
 	git commit -m "add submodules"
 '
 
-test_expect_failure 'clone with recurse-submodules fails' '
+test_expect_success 'clone with recurse-submodules fails' '
 	test_must_fail git clone --recurse-submodules . dst
 '
 
@@ -32,7 +32,7 @@ test_expect_success 'update of ssh allowed' '
 	git -C dst submodule update ssh-module
 '
 
-test_expect_failure 'update of ext not allowed' '
+test_expect_success 'update of ext not allowed' '
 	test_must_fail git -C dst submodule update ext-module
 '
 
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 7c8b90b..b77cce8 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -352,7 +352,7 @@ test_expect_success 'sync should fail with unknown submodule' '
 	test_failure_with_unknown_submodule sync
 '
 
-test_expect_failure 'update should fail when path is used by a file' '
+test_expect_success 'update should fail when path is used by a file' '
 	echo hello >expect &&
 
 	echo "hello" >init &&
@@ -361,7 +361,7 @@ test_expect_failure 'update should fail when path is used by a file' '
 	test_cmp expect init
 '
 
-test_expect_failure 'update should fail when path is used by a nonempty directory' '
+test_expect_success 'update should fail when path is used by a nonempty directory' '
 	echo hello >expect &&
 
 	rm -fr init &&
-- 
2.9.0.443.ga8520ad

Re: [PATCH 1/2] git-submodule: forward exit code of git-submodule--helper more faithfully

[Stefan Beller]

On Fri, Jul 22, 2016 at 12:14 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> git-submodule--helper is invoked as the upstream of a pipe in several
> places. Usually, the failure of a program in this position is not
> detected by the shell. For this reason, the code inserts a token in the
> output stream when git-submodule--helper fails that is detected
> downstream, where the shell script is quit with exit code 1.
>
> There happens to be a bug in git-submodule--helper that leads to a
> segmentation fault. The test suite triggers the crash in several places,
> all of which are protected by 'test_must_fail'. But due to the inspecific
> exit code 1, the crash remains undiagnosed.
>
> Extend the failure protocol such that git-submodule--helper's exit code
> is passed downstream (only in the case of failure). This enables the
> downstream to use it as its own exit code, and 'test_must_fail' to
> identify the segmentation fault as an unexpected failure.
>
> The bug itself is fixed in the next commit.
>
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
> When you run ./t7400-submodule-basic.sh -v, you will notice this output:
>
> fatal: destination path '/home/jsixt/Src/git/git/t/trash directory.t7400-submodule-basic/init' already exists and is not an empty directory.
> fatal: clone of './.subrepo' into submodule path '/home/jsixt/Src/git/git/t/trash directory.t7400-submodule-basic/init' failed
> Failed to clone 'init'. Retry scheduled
> fatal: destination path '/home/jsixt/Src/git/git/t/trash directory.t7400-submodule-basic/init' already exists and is not an empty directory.
> fatal: clone of './.subrepo' into submodule path '/home/jsixt/Src/git/git/t/trash directory.t7400-submodule-basic/init' failed
> /home/jsixt/Src/git/git/git-submodule: line 494: 21757 Segmentation fault      git submodule--helper update-clone ${GIT_QUIET:+--quiet} ${wt_prefix:+--prefix "$wt_prefix"} ${prefix:+--recursive-prefix "$prefix"} ${update:+--update "$update"} ${reference:+--reference "$reference"} ${depth:+--depth "$depth"} ${recommend_shallow:+"$recommend_shallow"} ${jobs:+$jobs} "$@"
> ok 32 - update should fail when path is used by a file
>
> Note the segmentation fault. This mini-series addresses the issue.

The segfault has been addressed in
http://thread.gmane.org/gmane.comp.version-control.git/299995
but received no attention yet.
The propagation of the exit code makes sense nevertheless.

Thanks!


>
> Noticed on Windows because it "visualizes" segfaults even for
> command line programs.
>
>  git-submodule.sh            | 22 +++++++++++-----------
>  t/t5815-submodule-protos.sh |  4 ++--
>  t/t7400-submodule-basic.sh  |  4 ++--
>  3 files changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 4ec7546..0a0e12d 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -49,7 +49,7 @@ die_if_unmatched ()
>  {
>         if test "$1" = "#unmatched"
>         then
> -               exit 1
> +               exit ${2:-1}
>         fi
>  }
>
> @@ -312,11 +312,11 @@ cmd_foreach()
>
>         {
>                 git submodule--helper list --prefix "$wt_prefix" ||
> -               echo "#unmatched"
> +               echo "#unmatched" $?
>         } |
>         while read mode sha1 stage sm_path
>         do
> -               die_if_unmatched "$mode"
> +               die_if_unmatched "$mode" "$sha1"
>                 if test -e "$sm_path"/.git
>                 then
>                         displaypath=$(git submodule--helper relative-path "$prefix$sm_path" "$wt_prefix")
> @@ -423,11 +423,11 @@ cmd_deinit()
>
>         {
>                 git submodule--helper list --prefix "$wt_prefix" "$@" ||
> -               echo "#unmatched"
> +               echo "#unmatched" $?
>         } |
>         while read mode sha1 stage sm_path
>         do
> -               die_if_unmatched "$mode"
> +               die_if_unmatched "$mode" "$sha1"
>                 name=$(git submodule--helper name "$sm_path") || exit
>
>                 displaypath=$(git submodule--helper relative-path "$sm_path" "$wt_prefix")
> @@ -581,12 +581,12 @@ cmd_update()
>                 ${depth:+--depth "$depth"} \
>                 ${recommend_shallow:+"$recommend_shallow"} \
>                 ${jobs:+$jobs} \
> -               "$@" || echo "#unmatched"
> +               "$@" || echo "#unmatched" $?
>         } | {
>         err=
>         while read mode sha1 stage just_cloned sm_path
>         do
> -               die_if_unmatched "$mode"
> +               die_if_unmatched "$mode" "$sha1"
>
>                 name=$(git submodule--helper name "$sm_path") || exit
>                 url=$(git config submodule."$name".url)
> @@ -994,11 +994,11 @@ cmd_status()
>
>         {
>                 git submodule--helper list --prefix "$wt_prefix" "$@" ||
> -               echo "#unmatched"
> +               echo "#unmatched" $?
>         } |
>         while read mode sha1 stage sm_path
>         do
> -               die_if_unmatched "$mode"
> +               die_if_unmatched "$mode" "$sha1"
>                 name=$(git submodule--helper name "$sm_path") || exit
>                 url=$(git config submodule."$name".url)
>                 displaypath=$(git submodule--helper relative-path "$prefix$sm_path" "$wt_prefix")
> @@ -1075,11 +1075,11 @@ cmd_sync()
>         cd_to_toplevel
>         {
>                 git submodule--helper list --prefix "$wt_prefix" "$@" ||
> -               echo "#unmatched"
> +               echo "#unmatched" $?
>         } |
>         while read mode sha1 stage sm_path
>         do
> -               die_if_unmatched "$mode"
> +               die_if_unmatched "$mode" "$sha1"
>                 name=$(git submodule--helper name "$sm_path")
>                 url=$(git config -f .gitmodules --get submodule."$name".url)
>
> diff --git a/t/t5815-submodule-protos.sh b/t/t5815-submodule-protos.sh
> index 06f55a1..112cf40 100755
> --- a/t/t5815-submodule-protos.sh
> +++ b/t/t5815-submodule-protos.sh
> @@ -18,7 +18,7 @@ test_expect_success 'setup repository with submodules' '
>         git commit -m "add submodules"
>  '
>
> -test_expect_success 'clone with recurse-submodules fails' '
> +test_expect_failure 'clone with recurse-submodules fails' '
>         test_must_fail git clone --recurse-submodules . dst
>  '
>
> @@ -32,7 +32,7 @@ test_expect_success 'update of ssh allowed' '
>         git -C dst submodule update ssh-module
>  '
>
> -test_expect_success 'update of ext not allowed' '
> +test_expect_failure 'update of ext not allowed' '
>         test_must_fail git -C dst submodule update ext-module
>  '
>
> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> index b77cce8..7c8b90b 100755
> --- a/t/t7400-submodule-basic.sh
> +++ b/t/t7400-submodule-basic.sh
> @@ -352,7 +352,7 @@ test_expect_success 'sync should fail with unknown submodule' '
>         test_failure_with_unknown_submodule sync
>  '
>
> -test_expect_success 'update should fail when path is used by a file' '
> +test_expect_failure 'update should fail when path is used by a file' '
>         echo hello >expect &&
>
>         echo "hello" >init &&
> @@ -361,7 +361,7 @@ test_expect_success 'update should fail when path is used by a file' '
>         test_cmp expect init
>  '
>
> -test_expect_success 'update should fail when path is used by a nonempty directory' '
> +test_expect_failure 'update should fail when path is used by a nonempty directory' '
>         echo hello >expect &&
>
>         rm -fr init &&
> --
> 2.9.0.443.ga8520ad

Re: [PATCH 2/2] submodule-helper: fix indexing in clone retry error reporting path

[Stefan Beller]

On Fri, Jul 22, 2016 at 12:15 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> 'git submodule--helper update-clone' has logic to retry failed clones
> a second time. For this purpose, there is a list of submodules to clone,
> and a second list that is filled with the submodules to retry. Within
> these lists, the submodules are identified by an index as if both lists
> were just appended.
>
> This works nicely except when the second clone attempt fails as well. To
> report an error, the identifying index must be adjusted by an offset so
> that it can be used as an index into the second list. However, the
> calculation uses the logical total length of the lists so that the result
> always points one past the end of the second list.
>
> Pick the correct index.
>
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
>  builtin/submodule--helper.c | 2 +-
>  t/t5815-submodule-protos.sh | 4 ++--
>  t/t7400-submodule-basic.sh  | 4 ++--
>  3 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index b22352b..6f6d67a 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -795,7 +795,7 @@ static int update_clone_task_finished(int result,
>                 suc->failed_clones[suc->failed_clones_nr++] = ce;
>                 return 0;
>         } else {
> -               idx = suc->current - suc->list.nr;
> +               idx -= suc->list.nr;

The fix is the same as in
http://thread.gmane.org/gmane.comp.version-control.git/299995
There we have an additional check, which may make sense to use here as well,
specifically when having the patch 1 which propagates the exit code.

The approach to tests is different though. I like yours better than mine,
as it doesn't add more tests, but strengthens existing tests.

>                 ce  = suc->failed_clones[idx];
>                 strbuf_addf(err, _("Failed to clone '%s' a second time, aborting"),
>                             ce->name);
> diff --git a/t/t5815-submodule-protos.sh b/t/t5815-submodule-protos.sh
> index 112cf40..06f55a1 100755
> --- a/t/t5815-submodule-protos.sh
> +++ b/t/t5815-submodule-protos.sh
> @@ -18,7 +18,7 @@ test_expect_success 'setup repository with submodules' '
>         git commit -m "add submodules"
>  '
>
> -test_expect_failure 'clone with recurse-submodules fails' '
> +test_expect_success 'clone with recurse-submodules fails' '
>         test_must_fail git clone --recurse-submodules . dst
>  '
>
> @@ -32,7 +32,7 @@ test_expect_success 'update of ssh allowed' '
>         git -C dst submodule update ssh-module
>  '
>
> -test_expect_failure 'update of ext not allowed' '
> +test_expect_success 'update of ext not allowed' '
>         test_must_fail git -C dst submodule update ext-module
>  '
>
> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> index 7c8b90b..b77cce8 100755
> --- a/t/t7400-submodule-basic.sh
> +++ b/t/t7400-submodule-basic.sh
> @@ -352,7 +352,7 @@ test_expect_success 'sync should fail with unknown submodule' '
>         test_failure_with_unknown_submodule sync
>  '
>
> -test_expect_failure 'update should fail when path is used by a file' '
> +test_expect_success 'update should fail when path is used by a file' '
>         echo hello >expect &&
>
>         echo "hello" >init &&
> @@ -361,7 +361,7 @@ test_expect_failure 'update should fail when path is used by a file' '
>         test_cmp expect init
>  '
>
> -test_expect_failure 'update should fail when path is used by a nonempty directory' '
> +test_expect_success 'update should fail when path is used by a nonempty directory' '
>         echo hello >expect &&
>
>         rm -fr init &&
> --
> 2.9.0.443.ga8520ad
>

Re: [PATCH 2/2] submodule-helper: fix indexing in clone retry error reporting path

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> 'git submodule--helper update-clone' has logic to retry failed clones
> a second time. For this purpose, there is a list of submodules to clone,
> and a second list that is filled with the submodules to retry. Within
> these lists, the submodules are identified by an index as if both lists
> were just appended.
>
> This works nicely except when the second clone attempt fails as well. To
> report an error, the identifying index must be adjusted by an offset so
> that it can be used as an index into the second list. However, the
> calculation uses the logical total length of the lists so that the result
> always points one past the end of the second list.
>
> Pick the correct index.
>
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---

Looks very similar to

  http://public-inbox.org/git/20160721013923.17435-1-sbeller%40google.com/

but these two patch series looks more thorough.

I expect I'd queue these two instead, after seeing Acks from Stefan.

Thanks.

Re: [PATCH 2/2] submodule-helper: fix indexing in clone retry error reporting path

[Stefan Beller]

On Fri, Jul 22, 2016 at 12:39 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Johannes Sixt <j6t@kdbg.org> writes:
>
>> 'git submodule--helper update-clone' has logic to retry failed clones
>> a second time. For this purpose, there is a list of submodules to clone,
>> and a second list that is filled with the submodules to retry. Within
>> these lists, the submodules are identified by an index as if both lists
>> were just appended.
>>
>> This works nicely except when the second clone attempt fails as well. To
>> report an error, the identifying index must be adjusted by an offset so
>> that it can be used as an index into the second list. However, the
>> calculation uses the logical total length of the lists so that the result
>> always points one past the end of the second list.
>>
>> Pick the correct index.
>>
>> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
>> ---
>
> Looks very similar to
>
>   http://public-inbox.org/git/20160721013923.17435-1-sbeller%40google.com/
>
> but these two patch series looks more thorough.
>
> I expect I'd queue these two instead, after seeing Acks from Stefan.
>
> Thanks.

Sure. Please take these 2 patches instead of mine.

Thanks,
Stefan

Re: [PATCH 2/2] submodule-helper: fix indexing in clone retry error reporting path

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> The approach to tests is different though. I like yours better than mine,
> as it doesn't add more tests, but strengthens existing tests.

So... are you retracting
http://thread.gmane.org/gmane.comp.version-control.git/299995 and
instead giving an Ack to these two?

Re: [PATCH 2/2] submodule-helper: fix indexing in clone retry error reporting path

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

>> I expect I'd queue these two instead, after seeing Acks from Stefan.
>>
>> Thanks.
>
> Sure. Please take these 2 patches instead of mine.

OK, I've already queued yours but I haven't started today's
integration cycle yet (I first pick up new topics and updates and
review them in isolation, and then re-review them after rebuilding
jch/pu branches in a separate phase), so I'll replace it with these
two patches.

Thanks.

Re: [PATCH 2/2] submodule-helper: fix indexing in clone retry error reporting path

[Stefan Beller]

On Fri, Jul 22, 2016 at 12:52 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> The approach to tests is different though. I like yours better than mine,
>> as it doesn't add more tests, but strengthens existing tests.
>
> So... are you retracting
> http://thread.gmane.org/gmane.comp.version-control.git/299995 and
> instead giving an Ack to these two?
>

I like this series better
* for the approach
* for the tests
* for the commit message

So I do think this should be applied instead of what I sent.

I am tempted to send a squash proposal like:
(broken whitespaces)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 6f6d67a..77be97e 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -796,6 +796,8 @@ static int update_clone_task_finished(int result,
  return 0;
  } else {
  idx -= suc->list.nr;
+ if (idx >= suc->failed_clones_nr)
+ die("BUG: idx too large???");
  ce  = suc->failed_clones[idx];
  strbuf_addf(err, _("Failed to clone '%s' a second time, aborting"),
     ce->name);