Re: [PATCH v2 1/2] negative-refspec: fix segfault on : refspec

[Eric Sunshine <sunshine@sunshineco.com>]

On Sat, Dec 19, 2020 at 5:00 PM Nipunn Koorapati via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> The logic added to check for negative pathspec match by c0192df630
> (refspec: add support for negative refspecs, 2020-09-30) looks at
> refspec->src assuming it is never NULL, however when
> remote.origin.push is set to ":", then refspec->src is NULL,
> causing a segfault within strcmp
>
> Added testing for this case in fetch-negative-refspec

A couple minor comments below...

> Signed-off-by: Nipunn Koorapati <nipunn@dropbox.com>
> ---
> diff --git a/remote.c b/remote.c
> @@ -751,9 +751,13 @@ static int query_matches_negative_refspec(struct refspec *rs, struct refspec_ite
> +               } else if (refspec->matching) {
> +                       /* For the special matching refspec, any query should match */
> +                       string_list_append(&reversed, needle);
> +               } else if (refspec->src == NULL) {
> +                       BUG("refspec->src should not be null here");

I realize that you copied Junio's example, but style on this project
is to write this as:

    } else if (!refspec->src) {
        ...

> diff --git a/t/t5582-fetch-negative-refspec.sh b/t/t5582-fetch-negative-refspec.sh
> @@ -186,4 +186,19 @@ test_expect_success "fetch --prune with negative refspec" '
> +test_expect_success "push with matching ':' refspec" '
> +       (
> +               cd two &&
> +               git config remote.one.push : &&
> +               # Fails w/ tip behind counterpart - but should not segfault
> +               test_must_fail git push one master &&
> +
> +               git config remote.one.push +: &&
> +               # Fails w/ tip behind counterpart - but should not segfault
> +               test_must_fail git push one master &&
> +
> +               git config --unset remote.one.push
> +       )
> +'

If anything in this test fails prior to the final `git config
--unset`, then that cleanup command won't be executed, which might
negatively impact tests which follow. To ensure cleanup whether the
test succeeds or fails, use test_config(). Unfortunately,
test_config() has the limitation that it can't be used in subshells,
so you may have to restructure the test a bit, perhaps like this:

    test_config remote.one.push : &&
    (
        cd two &&
        test_must_fail git push one master &&

        git config remote.one.push +: &&
        test_must_fail git push one master
    )

Driving the test with a for-loop and taking advantage of -C to avoid
the subshell is also an option:

    for v in : +:
    do
        test_config -C two remote.one.push $v &&
        test_must_fail git -C two push one master || return 1
    done