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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Dec 21, 2020, 07:20 UTC
Message-ID: <CAPig+cStb8a7QW-TDit2mfodEQMcPQTsCB0eJX=BMZJzo-TmUQ@mail.gmail.com>
In-Reply-To: <20cff2f5c59adb1076c845657aabed7ebbf0a6b5.1608516320.git.gitgitgadget@gmail.com>

On Sun, Dec 20, 2020 at 9:05 PM Nipunn Koorapati via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 11 quoted lines

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

Show 11 quoted lines

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