{"thread":{"id":"42906","subject":"[PATCH 1/2] git-submodule: forward exit code of git-submodule--helper more faithfully","startedAt":"2016-07-22T19:14:47Z","lastAt":"2016-07-22T20:00:59Z","messageCount":9,"participants":["Johannes Sixt","Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"291990","messageId":"8c0e116b-b604-ee83-197a-538eedf6e0ea@kdbg.org","threadId":"42906","inReplyTo":null,"subject":"[PATCH 1/2] git-submodule: forward exit code of git-submodule--helper more faithfully","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-07-22T19:14:38Z","receivedAt":"2016-07-22T19:14:47Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"git-submodule--helper is invoked as the upstream of a pipe in several\nplaces. Usually, the failure of a program in this position is not\ndetected by the shell. For this reason, the code inserts a token in the\noutput stream when git-submodule--helper fails that is detected\ndownstream, where the shell script is quit with exit code 1.\n\nThere happens to be a bug in git-submodule--helper that leads to a\nsegmentation fault. The test suite triggers the crash in several places,\nall of which are protected by 'test_must_fail'. But due to the inspecific\nexit code 1, the crash remains undiagnosed.\n\nExtend the failure protocol such that git-submodule--helper's exit code\nis passed downstream (only in the case of failure). This enables the\ndownstream to use it as its own exit code, and 'test_must_fail' to\nidentify the segmentation fault as an unexpected failure.\n\nThe bug itself is fixed in the next commit.\n\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n---\nWhen you run ./t7400-submodule-basic.sh -v, you will notice this output:\n\nfatal: destination path '/home/jsixt/Src/git/git/t/trash directory.t7400-submodule-basic/init' already exists and is not an empty directory.\nfatal: clone of './.subrepo' into submodule path '/home/jsixt/Src/git/git/t/trash directory.t7400-submodule-basic/init' failed\nFailed to clone 'init'. Retry scheduled\nfatal: destination path '/home/jsixt/Src/git/git/t/trash directory.t7400-submodule-basic/init' already exists and is not an empty directory.\nfatal: clone of './.subrepo' into submodule path '/home/jsixt/Src/git/git/t/trash directory.t7400-submodule-basic/init' failed\n/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} \"$@\"\nok 32 - update should fail when path is used by a file\n\nNote the segmentation fault. This mini-series addresses the issue.\n\nNoticed on Windows because it \"visualizes\" segfaults even for\ncommand line programs.\n\n git-submodule.sh            | 22 +++++++++++-----------\n t/t5815-submodule-protos.sh |  4 ++--\n t/t7400-submodule-basic.sh  |  4 ++--\n 3 files changed, 15 insertions(+), 15 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 4ec7546..0a0e12d 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -49,7 +49,7 @@ die_if_unmatched ()\n {\n \tif test \"$1\" = \"#unmatched\"\n \tthen\n-\t\texit 1\n+\t\texit ${2:-1}\n \tfi\n }\n \n@@ -312,11 +312,11 @@ cmd_foreach()\n \n \t{\n \t\tgit submodule--helper list --prefix \"$wt_prefix\" ||\n-\t\techo \"#unmatched\"\n+\t\techo \"#unmatched\" $?\n \t} |\n \twhile read mode sha1 stage sm_path\n \tdo\n-\t\tdie_if_unmatched \"$mode\"\n+\t\tdie_if_unmatched \"$mode\" \"$sha1\"\n \t\tif test -e \"$sm_path\"/.git\n \t\tthen\n \t\t\tdisplaypath=$(git submodule--helper relative-path \"$prefix$sm_path\" \"$wt_prefix\")\n@@ -423,11 +423,11 @@ cmd_deinit()\n \n \t{\n \t\tgit submodule--helper list --prefix \"$wt_prefix\" \"$@\" ||\n-\t\techo \"#unmatched\"\n+\t\techo \"#unmatched\" $?\n \t} |\n \twhile read mode sha1 stage sm_path\n \tdo\n-\t\tdie_if_unmatched \"$mode\"\n+\t\tdie_if_unmatched \"$mode\" \"$sha1\"\n \t\tname=$(git submodule--helper name \"$sm_path\") || exit\n \n \t\tdisplaypath=$(git submodule--helper relative-path \"$sm_path\" \"$wt_prefix\")\n@@ -581,12 +581,12 @@ cmd_update()\n \t\t${depth:+--depth \"$depth\"} \\\n \t\t${recommend_shallow:+\"$recommend_shallow\"} \\\n \t\t${jobs:+$jobs} \\\n-\t\t\"$@\" || echo \"#unmatched\"\n+\t\t\"$@\" || echo \"#unmatched\" $?\n \t} | {\n \terr=\n \twhile read mode sha1 stage just_cloned sm_path\n \tdo\n-\t\tdie_if_unmatched \"$mode\"\n+\t\tdie_if_unmatched \"$mode\" \"$sha1\"\n \n \t\tname=$(git submodule--helper name \"$sm_path\") || exit\n \t\turl=$(git config submodule.\"$name\".url)\n@@ -994,11 +994,11 @@ cmd_status()\n \n \t{\n \t\tgit submodule--helper list --prefix \"$wt_prefix\" \"$@\" ||\n-\t\techo \"#unmatched\"\n+\t\techo \"#unmatched\" $?\n \t} |\n \twhile read mode sha1 stage sm_path\n \tdo\n-\t\tdie_if_unmatched \"$mode\"\n+\t\tdie_if_unmatched \"$mode\" \"$sha1\"\n \t\tname=$(git submodule--helper name \"$sm_path\") || exit\n \t\turl=$(git config submodule.\"$name\".url)\n \t\tdisplaypath=$(git submodule--helper relative-path \"$prefix$sm_path\" \"$wt_prefix\")\n@@ -1075,11 +1075,11 @@ cmd_sync()\n \tcd_to_toplevel\n \t{\n \t\tgit submodule--helper list --prefix \"$wt_prefix\" \"$@\" ||\n-\t\techo \"#unmatched\"\n+\t\techo \"#unmatched\" $?\n \t} |\n \twhile read mode sha1 stage sm_path\n \tdo\n-\t\tdie_if_unmatched \"$mode\"\n+\t\tdie_if_unmatched \"$mode\" \"$sha1\"\n \t\tname=$(git submodule--helper name \"$sm_path\")\n \t\turl=$(git config -f .gitmodules --get submodule.\"$name\".url)\n \ndiff --git a/t/t5815-submodule-protos.sh b/t/t5815-submodule-protos.sh\nindex 06f55a1..112cf40 100755\n--- a/t/t5815-submodule-protos.sh\n+++ b/t/t5815-submodule-protos.sh\n@@ -18,7 +18,7 @@ test_expect_success 'setup repository with submodules' '\n \tgit commit -m \"add submodules\"\n '\n \n-test_expect_success 'clone with recurse-submodules fails' '\n+test_expect_failure 'clone with recurse-submodules fails' '\n \ttest_must_fail git clone --recurse-submodules . dst\n '\n \n@@ -32,7 +32,7 @@ test_expect_success 'update of ssh allowed' '\n \tgit -C dst submodule update ssh-module\n '\n \n-test_expect_success 'update of ext not allowed' '\n+test_expect_failure 'update of ext not allowed' '\n \ttest_must_fail git -C dst submodule update ext-module\n '\n \ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex b77cce8..7c8b90b 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -352,7 +352,7 @@ test_expect_success 'sync should fail with unknown submodule' '\n \ttest_failure_with_unknown_submodule sync\n '\n \n-test_expect_success 'update should fail when path is used by a file' '\n+test_expect_failure 'update should fail when path is used by a file' '\n \techo hello >expect &&\n \n \techo \"hello\" >init &&\n@@ -361,7 +361,7 @@ test_expect_success 'update should fail when path is used by a file' '\n \ttest_cmp expect init\n '\n \n-test_expect_success 'update should fail when path is used by a nonempty directory' '\n+test_expect_failure 'update should fail when path is used by a nonempty directory' '\n \techo hello >expect &&\n \n \trm -fr init &&\n-- \n2.9.0.443.ga8520ad\n"},{"id":"291991","messageId":"4d40da99-2f66-a380-840f-1828dc5b9324@kdbg.org","threadId":"42906","inReplyTo":"8c0e116b-b604-ee83-197a-538eedf6e0ea@kdbg.org","subject":"[PATCH 2/2] submodule-helper: fix indexing in clone retry error reporting path","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-07-22T19:15:39Z","receivedAt":"2016-07-22T19:15:46Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"'git submodule--helper update-clone' has logic to retry failed clones\na second time. For this purpose, there is a list of submodules to clone,\nand a second list that is filled with the submodules to retry. Within\nthese lists, the submodules are identified by an index as if both lists\nwere just appended.\n\nThis works nicely except when the second clone attempt fails as well. To\nreport an error, the identifying index must be adjusted by an offset so\nthat it can be used as an index into the second list. However, the\ncalculation uses the logical total length of the lists so that the result\nalways points one past the end of the second list.\n\nPick the correct index.\n\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n---\n builtin/submodule--helper.c | 2 +-\n t/t5815-submodule-protos.sh | 4 ++--\n t/t7400-submodule-basic.sh  | 4 ++--\n 3 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex b22352b..6f6d67a 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -795,7 +795,7 @@ static int update_clone_task_finished(int result,\n \t\tsuc->failed_clones[suc->failed_clones_nr++] = ce;\n \t\treturn 0;\n \t} else {\n-\t\tidx = suc->current - suc->list.nr;\n+\t\tidx -= suc->list.nr;\n \t\tce  = suc->failed_clones[idx];\n \t\tstrbuf_addf(err, _(\"Failed to clone '%s' a second time, aborting\"),\n \t\t\t    ce->name);\ndiff --git a/t/t5815-submodule-protos.sh b/t/t5815-submodule-protos.sh\nindex 112cf40..06f55a1 100755\n--- a/t/t5815-submodule-protos.sh\n+++ b/t/t5815-submodule-protos.sh\n@@ -18,7 +18,7 @@ test_expect_success 'setup repository with submodules' '\n \tgit commit -m \"add submodules\"\n '\n \n-test_expect_failure 'clone with recurse-submodules fails' '\n+test_expect_success 'clone with recurse-submodules fails' '\n \ttest_must_fail git clone --recurse-submodules . dst\n '\n \n@@ -32,7 +32,7 @@ test_expect_success 'update of ssh allowed' '\n \tgit -C dst submodule update ssh-module\n '\n \n-test_expect_failure 'update of ext not allowed' '\n+test_expect_success 'update of ext not allowed' '\n \ttest_must_fail git -C dst submodule update ext-module\n '\n \ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex 7c8b90b..b77cce8 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -352,7 +352,7 @@ test_expect_success 'sync should fail with unknown submodule' '\n \ttest_failure_with_unknown_submodule sync\n '\n \n-test_expect_failure 'update should fail when path is used by a file' '\n+test_expect_success 'update should fail when path is used by a file' '\n \techo hello >expect &&\n \n \techo \"hello\" >init &&\n@@ -361,7 +361,7 @@ test_expect_failure 'update should fail when path is used by a file' '\n \ttest_cmp expect init\n '\n \n-test_expect_failure 'update should fail when path is used by a nonempty directory' '\n+test_expect_success 'update should fail when path is used by a nonempty directory' '\n \techo hello >expect &&\n \n \trm -fr init &&\n-- \n2.9.0.443.ga8520ad\n\n"},{"id":"291993","messageId":"CAGZ79ka0K6bFPt8gYNeKvA_B-ggHHETD=EtwRYj7F_5XTJ+n8g@mail.gmail.com","threadId":"42906","inReplyTo":"8c0e116b-b604-ee83-197a-538eedf6e0ea@kdbg.org","subject":"Re: [PATCH 1/2] git-submodule: forward exit code of git-submodule--helper more faithfully","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-07-22T19:27:04Z","receivedAt":"2016-07-22T19:27:13Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Jul 22, 2016 at 12:14 PM, Johannes Sixt <j6t@kdbg.org> wrote:\n> git-submodule--helper is invoked as the upstream of a pipe in several\n> places. Usually, the failure of a program in this position is not\n> detected by the shell. For this reason, the code inserts a token in the\n> output stream when git-submodule--helper fails that is detected\n> downstream, where the shell script is quit with exit code 1.\n>\n> There happens to be a bug in git-submodule--helper that leads to a\n> segmentation fault. The test suite triggers the crash in several places,\n> all of which are protected by 'test_must_fail'. But due to the inspecific\n> exit code 1, the crash remains undiagnosed.\n>\n> Extend the failure protocol such that git-submodule--helper's exit code\n> is passed downstream (only in the case of failure). This enables the\n> downstream to use it as its own exit code, and 'test_must_fail' to\n> identify the segmentation fault as an unexpected failure.\n>\n> The bug itself is fixed in the next commit.\n>\n> Signed-off-by: Johannes Sixt <j6t@kdbg.org>\n> ---\n> When you run ./t7400-submodule-basic.sh -v, you will notice this output:\n>\n> fatal: destination path '/home/jsixt/Src/git/git/t/trash directory.t7400-submodule-basic/init' already exists and is not an empty directory.\n> fatal: clone of './.subrepo' into submodule path '/home/jsixt/Src/git/git/t/trash directory.t7400-submodule-basic/init' failed\n> Failed to clone 'init'. Retry scheduled\n> fatal: destination path '/home/jsixt/Src/git/git/t/trash directory.t7400-submodule-basic/init' already exists and is not an empty directory.\n> fatal: clone of './.subrepo' into submodule path '/home/jsixt/Src/git/git/t/trash directory.t7400-submodule-basic/init' failed\n> /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} \"$@\"\n> ok 32 - update should fail when path is used by a file\n>\n> Note the segmentation fault. This mini-series addresses the issue.\n\nThe segfault has been addressed in\nhttp://thread.gmane.org/gmane.comp.version-control.git/299995\nbut received no attention yet.\nThe propagation of the exit code makes sense nevertheless.\n\nThanks!\n\n\n>\n> Noticed on Windows because it \"visualizes\" segfaults even for\n> command line programs.\n>\n>  git-submodule.sh            | 22 +++++++++++-----------\n>  t/t5815-submodule-protos.sh |  4 ++--\n>  t/t7400-submodule-basic.sh  |  4 ++--\n>  3 files changed, 15 insertions(+), 15 deletions(-)\n>\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index 4ec7546..0a0e12d 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -49,7 +49,7 @@ die_if_unmatched ()\n>  {\n>         if test \"$1\" = \"#unmatched\"\n>         then\n> -               exit 1\n> +               exit ${2:-1}\n>         fi\n>  }\n>\n> @@ -312,11 +312,11 @@ cmd_foreach()\n>\n>         {\n>                 git submodule--helper list --prefix \"$wt_prefix\" ||\n> -               echo \"#unmatched\"\n> +               echo \"#unmatched\" $?\n>         } |\n>         while read mode sha1 stage sm_path\n>         do\n> -               die_if_unmatched \"$mode\"\n> +               die_if_unmatched \"$mode\" \"$sha1\"\n>                 if test -e \"$sm_path\"/.git\n>                 then\n>                         displaypath=$(git submodule--helper relative-path \"$prefix$sm_path\" \"$wt_prefix\")\n> @@ -423,11 +423,11 @@ cmd_deinit()\n>\n>         {\n>                 git submodule--helper list --prefix \"$wt_prefix\" \"$@\" ||\n> -               echo \"#unmatched\"\n> +               echo \"#unmatched\" $?\n>         } |\n>         while read mode sha1 stage sm_path\n>         do\n> -               die_if_unmatched \"$mode\"\n> +               die_if_unmatched \"$mode\" \"$sha1\"\n>                 name=$(git submodule--helper name \"$sm_path\") || exit\n>\n>                 displaypath=$(git submodule--helper relative-path \"$sm_path\" \"$wt_prefix\")\n> @@ -581,12 +581,12 @@ cmd_update()\n>                 ${depth:+--depth \"$depth\"} \\\n>                 ${recommend_shallow:+\"$recommend_shallow\"} \\\n>                 ${jobs:+$jobs} \\\n> -               \"$@\" || echo \"#unmatched\"\n> +               \"$@\" || echo \"#unmatched\" $?\n>         } | {\n>         err=\n>         while read mode sha1 stage just_cloned sm_path\n>         do\n> -               die_if_unmatched \"$mode\"\n> +               die_if_unmatched \"$mode\" \"$sha1\"\n>\n>                 name=$(git submodule--helper name \"$sm_path\") || exit\n>                 url=$(git config submodule.\"$name\".url)\n> @@ -994,11 +994,11 @@ cmd_status()\n>\n>         {\n>                 git submodule--helper list --prefix \"$wt_prefix\" \"$@\" ||\n> -               echo \"#unmatched\"\n> +               echo \"#unmatched\" $?\n>         } |\n>         while read mode sha1 stage sm_path\n>         do\n> -               die_if_unmatched \"$mode\"\n> +               die_if_unmatched \"$mode\" \"$sha1\"\n>                 name=$(git submodule--helper name \"$sm_path\") || exit\n>                 url=$(git config submodule.\"$name\".url)\n>                 displaypath=$(git submodule--helper relative-path \"$prefix$sm_path\" \"$wt_prefix\")\n> @@ -1075,11 +1075,11 @@ cmd_sync()\n>         cd_to_toplevel\n>         {\n>                 git submodule--helper list --prefix \"$wt_prefix\" \"$@\" ||\n> -               echo \"#unmatched\"\n> +               echo \"#unmatched\" $?\n>         } |\n>         while read mode sha1 stage sm_path\n>         do\n> -               die_if_unmatched \"$mode\"\n> +               die_if_unmatched \"$mode\" \"$sha1\"\n>                 name=$(git submodule--helper name \"$sm_path\")\n>                 url=$(git config -f .gitmodules --get submodule.\"$name\".url)\n>\n> diff --git a/t/t5815-submodule-protos.sh b/t/t5815-submodule-protos.sh\n> index 06f55a1..112cf40 100755\n> --- a/t/t5815-submodule-protos.sh\n> +++ b/t/t5815-submodule-protos.sh\n> @@ -18,7 +18,7 @@ test_expect_success 'setup repository with submodules' '\n>         git commit -m \"add submodules\"\n>  '\n>\n> -test_expect_success 'clone with recurse-submodules fails' '\n> +test_expect_failure 'clone with recurse-submodules fails' '\n>         test_must_fail git clone --recurse-submodules . dst\n>  '\n>\n> @@ -32,7 +32,7 @@ test_expect_success 'update of ssh allowed' '\n>         git -C dst submodule update ssh-module\n>  '\n>\n> -test_expect_success 'update of ext not allowed' '\n> +test_expect_failure 'update of ext not allowed' '\n>         test_must_fail git -C dst submodule update ext-module\n>  '\n>\n> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\n> index b77cce8..7c8b90b 100755\n> --- a/t/t7400-submodule-basic.sh\n> +++ b/t/t7400-submodule-basic.sh\n> @@ -352,7 +352,7 @@ test_expect_success 'sync should fail with unknown submodule' '\n>         test_failure_with_unknown_submodule sync\n>  '\n>\n> -test_expect_success 'update should fail when path is used by a file' '\n> +test_expect_failure 'update should fail when path is used by a file' '\n>         echo hello >expect &&\n>\n>         echo \"hello\" >init &&\n> @@ -361,7 +361,7 @@ test_expect_success 'update should fail when path is used by a file' '\n>         test_cmp expect init\n>  '\n>\n> -test_expect_success 'update should fail when path is used by a nonempty directory' '\n> +test_expect_failure 'update should fail when path is used by a nonempty directory' '\n>         echo hello >expect &&\n>\n>         rm -fr init &&\n> --\n> 2.9.0.443.ga8520ad\n"},{"id":"291996","messageId":"CAGZ79kbiVwyyTZLxQP+ioLBoC6r8dcJV4SdDUL_bn58bFUbsWg@mail.gmail.com","threadId":"42906","inReplyTo":"4d40da99-2f66-a380-840f-1828dc5b9324@kdbg.org","subject":"Re: [PATCH 2/2] submodule-helper: fix indexing in clone retry error reporting path","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-07-22T19:30:17Z","receivedAt":"2016-07-22T19:30:23Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Jul 22, 2016 at 12:15 PM, Johannes Sixt <j6t@kdbg.org> wrote:\n> 'git submodule--helper update-clone' has logic to retry failed clones\n> a second time. For this purpose, there is a list of submodules to clone,\n> and a second list that is filled with the submodules to retry. Within\n> these lists, the submodules are identified by an index as if both lists\n> were just appended.\n>\n> This works nicely except when the second clone attempt fails as well. To\n> report an error, the identifying index must be adjusted by an offset so\n> that it can be used as an index into the second list. However, the\n> calculation uses the logical total length of the lists so that the result\n> always points one past the end of the second list.\n>\n> Pick the correct index.\n>\n> Signed-off-by: Johannes Sixt <j6t@kdbg.org>\n> ---\n>  builtin/submodule--helper.c | 2 +-\n>  t/t5815-submodule-protos.sh | 4 ++--\n>  t/t7400-submodule-basic.sh  | 4 ++--\n>  3 files changed, 5 insertions(+), 5 deletions(-)\n>\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index b22352b..6f6d67a 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -795,7 +795,7 @@ static int update_clone_task_finished(int result,\n>                 suc->failed_clones[suc->failed_clones_nr++] = ce;\n>                 return 0;\n>         } else {\n> -               idx = suc->current - suc->list.nr;\n> +               idx -= suc->list.nr;\n\nThe fix is the same as in\nhttp://thread.gmane.org/gmane.comp.version-control.git/299995\nThere we have an additional check, which may make sense to use here as well,\nspecifically when having the patch 1 which propagates the exit code.\n\nThe approach to tests is different though. I like yours better than mine,\nas it doesn't add more tests, but strengthens existing tests.\n\n>                 ce  = suc->failed_clones[idx];\n>                 strbuf_addf(err, _(\"Failed to clone '%s' a second time, aborting\"),\n>                             ce->name);\n> diff --git a/t/t5815-submodule-protos.sh b/t/t5815-submodule-protos.sh\n> index 112cf40..06f55a1 100755\n> --- a/t/t5815-submodule-protos.sh\n> +++ b/t/t5815-submodule-protos.sh\n> @@ -18,7 +18,7 @@ test_expect_success 'setup repository with submodules' '\n>         git commit -m \"add submodules\"\n>  '\n>\n> -test_expect_failure 'clone with recurse-submodules fails' '\n> +test_expect_success 'clone with recurse-submodules fails' '\n>         test_must_fail git clone --recurse-submodules . dst\n>  '\n>\n> @@ -32,7 +32,7 @@ test_expect_success 'update of ssh allowed' '\n>         git -C dst submodule update ssh-module\n>  '\n>\n> -test_expect_failure 'update of ext not allowed' '\n> +test_expect_success 'update of ext not allowed' '\n>         test_must_fail git -C dst submodule update ext-module\n>  '\n>\n> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\n> index 7c8b90b..b77cce8 100755\n> --- a/t/t7400-submodule-basic.sh\n> +++ b/t/t7400-submodule-basic.sh\n> @@ -352,7 +352,7 @@ test_expect_success 'sync should fail with unknown submodule' '\n>         test_failure_with_unknown_submodule sync\n>  '\n>\n> -test_expect_failure 'update should fail when path is used by a file' '\n> +test_expect_success 'update should fail when path is used by a file' '\n>         echo hello >expect &&\n>\n>         echo \"hello\" >init &&\n> @@ -361,7 +361,7 @@ test_expect_failure 'update should fail when path is used by a file' '\n>         test_cmp expect init\n>  '\n>\n> -test_expect_failure 'update should fail when path is used by a nonempty directory' '\n> +test_expect_success 'update should fail when path is used by a nonempty directory' '\n>         echo hello >expect &&\n>\n>         rm -fr init &&\n> --\n> 2.9.0.443.ga8520ad\n>\n"},{"id":"291999","messageId":"xmqqeg6la2da.fsf@gitster.mtv.corp.google.com","threadId":"42906","inReplyTo":"4d40da99-2f66-a380-840f-1828dc5b9324@kdbg.org","subject":"Re: [PATCH 2/2] submodule-helper: fix indexing in clone retry error reporting path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-22T19:39:45Z","receivedAt":"2016-07-22T19:39:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\n> 'git submodule--helper update-clone' has logic to retry failed clones\n> a second time. For this purpose, there is a list of submodules to clone,\n> and a second list that is filled with the submodules to retry. Within\n> these lists, the submodules are identified by an index as if both lists\n> were just appended.\n>\n> This works nicely except when the second clone attempt fails as well. To\n> report an error, the identifying index must be adjusted by an offset so\n> that it can be used as an index into the second list. However, the\n> calculation uses the logical total length of the lists so that the result\n> always points one past the end of the second list.\n>\n> Pick the correct index.\n>\n> Signed-off-by: Johannes Sixt <j6t@kdbg.org>\n> ---\n\nLooks very similar to\n\n  http://public-inbox.org/git/20160721013923.17435-1-sbeller%40google.com/\n\nbut these two patch series looks more thorough.\n\nI expect I'd queue these two instead, after seeing Acks from Stefan.\n\nThanks.\n"},{"id":"292000","messageId":"CAGZ79kZCt2iZNc7VkkcW1bTnwF9YrDi9zPi0p7=-7aDVe=0dzg@mail.gmail.com","threadId":"42906","inReplyTo":"xmqqeg6la2da.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/2] submodule-helper: fix indexing in clone retry error reporting path","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-07-22T19:50:13Z","receivedAt":"2016-07-22T19:50:20Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Jul 22, 2016 at 12:39 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Johannes Sixt <j6t@kdbg.org> writes:\n>\n>> 'git submodule--helper update-clone' has logic to retry failed clones\n>> a second time. For this purpose, there is a list of submodules to clone,\n>> and a second list that is filled with the submodules to retry. Within\n>> these lists, the submodules are identified by an index as if both lists\n>> were just appended.\n>>\n>> This works nicely except when the second clone attempt fails as well. To\n>> report an error, the identifying index must be adjusted by an offset so\n>> that it can be used as an index into the second list. However, the\n>> calculation uses the logical total length of the lists so that the result\n>> always points one past the end of the second list.\n>>\n>> Pick the correct index.\n>>\n>> Signed-off-by: Johannes Sixt <j6t@kdbg.org>\n>> ---\n>\n> Looks very similar to\n>\n>   http://public-inbox.org/git/20160721013923.17435-1-sbeller%40google.com/\n>\n> but these two patch series looks more thorough.\n>\n> I expect I'd queue these two instead, after seeing Acks from Stefan.\n>\n> Thanks.\n\nSure. Please take these 2 patches instead of mine.\n\nThanks,\nStefan\n"},{"id":"292009","messageId":"xmqq60rxa1sl.fsf@gitster.mtv.corp.google.com","threadId":"42906","inReplyTo":"CAGZ79kbiVwyyTZLxQP+ioLBoC6r8dcJV4SdDUL_bn58bFUbsWg@mail.gmail.com","subject":"Re: [PATCH 2/2] submodule-helper: fix indexing in clone retry error reporting path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-22T19:52:10Z","receivedAt":"2016-07-22T19:52:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> The approach to tests is different though. I like yours better than mine,\n> as it doesn't add more tests, but strengthens existing tests.\n\nSo... are you retracting\nhttp://thread.gmane.org/gmane.comp.version-control.git/299995 and\ninstead giving an Ack to these two?\n\n"},{"id":"292010","messageId":"xmqq1t2la1f0.fsf@gitster.mtv.corp.google.com","threadId":"42906","inReplyTo":"CAGZ79kZCt2iZNc7VkkcW1bTnwF9YrDi9zPi0p7=-7aDVe=0dzg@mail.gmail.com","subject":"Re: [PATCH 2/2] submodule-helper: fix indexing in clone retry error reporting path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-22T20:00:19Z","receivedAt":"2016-07-22T20:00:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n>> I expect I'd queue these two instead, after seeing Acks from Stefan.\n>>\n>> Thanks.\n>\n> Sure. Please take these 2 patches instead of mine.\n\nOK, I've already queued yours but I haven't started today's\nintegration cycle yet (I first pick up new topics and updates and\nreview them in isolation, and then re-review them after rebuilding\njch/pu branches in a separate phase), so I'll replace it with these\ntwo patches.\n\nThanks.\n"},{"id":"292011","messageId":"CAGZ79kaoxo9qM-r9nNzG8GV4aCfun4RaK8jpsdSgbyix1bJBFg@mail.gmail.com","threadId":"42906","inReplyTo":"xmqq60rxa1sl.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/2] submodule-helper: fix indexing in clone retry error reporting path","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-07-22T20:00:53Z","receivedAt":"2016-07-22T20:00:59Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Jul 22, 2016 at 12:52 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> The approach to tests is different though. I like yours better than mine,\n>> as it doesn't add more tests, but strengthens existing tests.\n>\n> So... are you retracting\n> http://thread.gmane.org/gmane.comp.version-control.git/299995 and\n> instead giving an Ack to these two?\n>\n\nI like this series better\n* for the approach\n* for the tests\n* for the commit message\n\nSo I do think this should be applied instead of what I sent.\n\nI am tempted to send a squash proposal like:\n(broken whitespaces)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 6f6d67a..77be97e 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -796,6 +796,8 @@ static int update_clone_task_finished(int result,\n  return 0;\n  } else {\n  idx -= suc->list.nr;\n+ if (idx >= suc->failed_clones_nr)\n+ die(\"BUG: idx too large???\");\n  ce  = suc->failed_clones[idx];\n  strbuf_addf(err, _(\"Failed to clone '%s' a second time, aborting\"),\n     ce->name);\n"}]}