On Fri, Jul 22, 2016 at 12:14 PM, Johannes Sixt <j...@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 <j...@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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
