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:

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

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

Use 'test' rather than '['. Drop the semicolon. Place 'then' on its own line.

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

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.

So, 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
'

You typically don't need to cleanup after your test, so this boilerplate can go away.