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

From: Stefan Beller <sbeller@google.com>
Date: Jul 22, 2016, 19:30 UTC
Message-ID: <CAGZ79kbiVwyyTZLxQP+ioLBoC6r8dcJV4SdDUL_bn58bFUbsWg@mail.gmail.com>
In-Reply-To: <4d40da99-2f66-a380-840f-1828dc5b9324@kdbg.org>

On Fri, Jul 22, 2016 at 12:15 PM, Johannes Sixt <j6t@kdbg.org> wrote:

Show 31 quoted lines

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

Show 50 quoted lines

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