From c4c02bf16c47f8751958458e540269ec13f4bd98 Mon Sep 17 00:00:00 2001 From: Johannes Sixt Date: Fri, 22 Jul 2016 21:14:38 +0200 Subject: [PATCH 1/2] git-submodule: forward exit code of git-submodule--helper more faithfully 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 Acked-by: Stefan Beller Signed-off-by: Junio C Hamano --- 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 7c62b53cc0..d8765361c5 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -50,7 +50,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") @@ -580,12 +580,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) @@ -993,11 +993,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") @@ -1074,11 +1074,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 06f55a1b8a..112cf40233 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 3570f7bb8c..fba2659a22 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 && From eb09121b745e9aa5bbd3fa438f873c3511f48b33 Mon Sep 17 00:00:00 2001 From: Johannes Sixt Date: Fri, 22 Jul 2016 21:15:39 +0200 Subject: [PATCH 2/2] submodule-helper: fix indexing in clone retry error reporting path '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 Acked-by: Stefan Beller Signed-off-by: Junio C Hamano --- 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 b22352b6e1..6f6d67a469 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 112cf40233..06f55a1b8a 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 fba2659a22..3570f7bb8c 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 &&