Re: [PATCH v3 2/3] negative-refspec: fix segfault on : refspec

[Eric Sunshine <sunshine@sunshineco.com>]

On Sun, Dec 20, 2020 at 9:05 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
>
> Tell git to handle matching refspec by adding the needle to the
> set of positively matched refspecs, since matching ":" refspecs
> match anything as src.
>
> Added testing for matching refspec pushes fetch-negative-refspec

s/Added testing/Add test/

> both individually and in combination with a negative refspec
>
> Signed-off-by: Nipunn Koorapati <nipunn@dropbox.com>
> ---
> diff --git a/t/t5582-fetch-negative-refspec.sh b/t/t5582-fetch-negative-refspec.sh
> @@ -186,4 +186,26 @@ test_expect_success "fetch --prune with negative refspec" '
> +test_expect_success "push with matching ':' refspec" '
> +       test_config -C two remote.one.push : &&
> +       # Fails w/ tip behind counterpart - but should not segfault
> +       test_must_fail git -C two push one
> +'

Nit: It is understood implicitly that Git should not segfault (or
indeed any software). That's also implied by use of test_must_fail()
which explicitly distinguishes expected failures from unexpected
failures (where segfault falls in the category of unexpected failure).
Therefore, it doesn't really add value to say "but should not
segfault" in the comment.

Same observation applies to the other similarly-worded comments in
this patch. Not alone worth a re-roll, but perhaps worth changing if
you do re-roll.