Re: [PATCH] Fixing segmentation fault when merging FETCH_HEAD
On Sun, Mar 20, 2016 at 8:11 PM, Jose Ivan B. Vilarouca Filho <joseivan@lavid.ufpb.br> wrote:
> Hello, Eric.
>
> Thanks for suggestions. I've added a test in commit replacing git fetch origin by a fake FETCH_HEAD content.
Thanks for the re-roll. To be "git am"-friendly, you should either place a "-- >8 --" scissor line after the above commentary, or use "git send-email" to send the patch, and place the commentary just below the "---" line following your sign-off.
More below...
Show 14 quoted lines
> merge: don't dereference NULL pointer
>
> A segmentaion fault is raised when trying to merge FETCH_HEAD
> formed only by "not-for-merge" refs.
>
> Ex:
> git init .
> git remote add origin ...
> git fetch origin
> git merge FETCH_HEAD
>
> Signed-off-by: Jose Ivan B. Vilarouca Filho <joseivan@lavid.ufpb.br>
> ---
> diff --git a/test-merge-fetchhead.sh b/test-merge-fetchhead.sh
As you're new around here, I probably should have been more specific in my previous review and explained that you'd want your new test to be incorporated into the existing test suite so that it actually gets exercised regularly. A good place for the new test might be at the bottom of t/t7600-merge.sh.
Writing a new test is pretty simple, especially when you already have a recipe for reproduction. Take a look at existing tests in that file to get a feel for how it should be done. Rather than all the "|| exit 1"'s, chain your test statements together with &&. And, because you'll be incorporating the new test into an existing script, you can drop a lot of the boilerplate below and just focus on the recipe.
Some style comments below (most of which won't matter after you drop the boilerplate but are generally good to know)...
Show 6 quoted lines
> @@ -0,0 +1,23 @@
> +#!/bin/bash
> +GIT=$(pwd)/git
> +REPO_DIR=./test-fetch-head-repo
> +
> +if [ ! -x "${GIT}" ]; thenUse 'test' rather than '['. Drop the semicolon. Place 'then' on its own line.
Show 6 quoted lines
> + echo "Missing git binary"
> + exit 1
> +fi
> +
> +${GIT} init ${REPO_DIR} || rm -rf ${REPO_DIR} || exit 1
> +pushd ${REPO_DIR} || rm -rf ${REPO_DIR} || exit 1In test scripts you need to take extra care when switching directories since failure of a command following 'pushd' will prevent the subsequent 'popd' from executing, and then tests later in the script will be invoked in the wrong directory. The typical way of dealing with this is to use a subhsell since 'cd' within a subshell doesn't affect the parent, and the parent continues at its own working directory when the subshell exits. For example:
some-command &&
(
cd somewhere &&
command-which-might-fail &&
another-possible-failure
)> +#Let's fake a FETCH_HEAD only formed by not-for-merge (git fetch origin)
> +echo -ne "f954fc9919c9ec33179e11aa1af562384677e174\tnot-for-merge\tbranch 'master' of https://github.com/git/git.git" > .git/FETCH_HEAD
Non-portable 'echo' options. Use 'printf' instead. And, you'll need to take extra care with the single-quotes since the test body itself will be within single-quotes.
> +${GIT} merge FETCH_HEAD
> +GRET=$?
> +popd
> +if [ ${GRET} -eq 139 ]; thenSo, both before and after this patch, this git-merge fails, and you want the test to detect that it's failing in a controlled way (via die()) rather than the previous uncontrolled way (via segfault). You could use test_expect_code() for this (from t/test-lib-functions.sh), or you could capture stderr and verify that it has the expected failure message from die(). The latter is probably preferable, and may look something like this:
test_expect_success 'my test title' '
...setup stuff... &&
test_must_fail git merge FETCH_HEAD 2>err &&
test_i18ngrep "not something we can merge" err
'Show 6 quoted lines
> + rm -rf ${REPO_DIR}
> + exit 1
> +fi
> +
> +rm -rf ${REPO_DIR}
> +exit 0You typically don't need to cleanup after your test, so this boilerplate can go away.
