[v2,0/5] mktag tests & fix for-each-ref segfault

Message

Ævar Arnfjörð Bjarmason April 1, 2021, 1:56 p.m. UTC

On Thu, Apr 01 2021, Jeff King wrote:

> On Thu, Apr 01, 2021 at 03:54:56AM -0400, Jeff King wrote:
>
>> On Wed, Mar 31, 2021 at 10:46:22PM +0200, Ævar Arnfjörð Bjarmason wrote:
>> 
>> > > Neither of those types is the correct one. And the segfault is just a
>> > > bonus! :)
>> > >
>> > > I'd expect similar cases with parsing commit parents and tree pointers.
>> > > And probably tree entries whose modes are wrong.
>> > 
>> > So the segfault happens without my patches,
>> 
>> Yeah, sorry if that was unclear. It is definitely a pre-existing bug.
>
> Here's a patch to fix it. This is mostly orthogonal to your patch
> series. It happens to use a similar recipe to reproduce, but that is not
> the only way to do it, and the fix and the test shouldn't conflict
> textually or semantically.

Here's a proposed v2. We test the same case, but I thought it made
sense to test this more exhaustively.

The v1 will also leave t6300 in a bad state for whoever adds the next
test, trivial to fix with a test_create_repo, but this seems better.

Jeff King (1):
  ref-filter: fix NULL check for parse object failure

Ævar Arnfjörð Bjarmason (4):
  mktag tests: parse out options in helper
  mktag tests: invert --no-strict test
  mktag tests: do fsck on failure
  mktag tests: test for maybe segfaulting for-each-ref

 ref-filter.c     |  2 +-
 t/t3800-mktag.sh | 90 +++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 75 insertions(+), 17 deletions(-)

Range-diff:
-:  ----------- > 1:  45e0f100613 mktag tests: parse out options in helper
-:  ----------- > 2:  dd71740447d mktag tests: invert --no-strict test
-:  ----------- > 3:  688d7456843 mktag tests: do fsck on failure
-:  ----------- > 4:  403024b1cca mktag tests: test for maybe segfaulting for-each-ref
1:  9358541ce1f ! 5:  2ffe8f9fe3c ref-filter: fix NULL check for parse object failure
    @@ Commit message
     
         There are many ways a parse could fail, but most of them are hard to set
         up in the tests (it's easy to make a bogus object, but update-ref will
    -    refuse to point to it). The test here uses a tag which points to a wrong
    -    object type. A parse of just the broken tag object will succeed, but
    -    seeing both tag objects in the same process will lead to a parse error
    -    (since we'll see the pointed-to object as both types).
    +    refuse to point to it).
    +
    +    A minimal stand-alone test can be found at, but let's use the newly
    +    amended t3800-mktag.sh tests to test these cases exhaustively on all
    +    sorts of bad tags.
    +
    +    1. http://lore.kernel.org/git/[e-mail]
     
         Signed-off-by: Jeff King <[e-mail]>
    +    Signed-off-by: Ævar Arnfjörð Bjarmason <[e-mail]>
     
      ## ref-filter.c ##
     @@ ref-filter.c: static int get_object(struct ref_array_item *ref, int deref, struct object **obj
    @@ ref-filter.c: static int get_object(struct ref_array_item *ref, int deref, struc
      				free(oi->content);
      			return strbuf_addf_ret(err, -1, _("parse_object_buffer failed on %s for %s"),
     
    - ## t/t6300-for-each-ref.sh ##
    -@@ t/t6300-for-each-ref.sh: test_expect_success 'for-each-ref --ignore-case works on multiple sort keys' '
    - 	test_cmp expect actual
    - '
    + ## t/t3800-mktag.sh ##
    +@@ t/t3800-mktag.sh: check_verify_failure () {
    + 		git -C bad-tag for-each-ref "$tag_ref" >actual &&
    + 		test_cmp expected actual &&
    + 		# segfaults!
    +-		! git -C bad-tag for-each-ref --format="%(*objectname)"
    ++		test_must_fail git -C bad-tag for-each-ref --format="%(*objectname)"
    + 	'
    + }
      
    -+test_expect_success 'for-each-ref reports broken tags' '
    -+	git tag -m "good tag" broken-tag-good HEAD &&
    -+	git cat-file tag broken-tag-good >good &&
    -+	sed s/commit/blob/ <good >bad &&
    -+	bad=$(git hash-object -w -t tag bad) &&
    -+	git update-ref refs/tags/broken-tag-bad $bad &&
    -+	test_must_fail git for-each-ref --format="%(*objectname)" \
    -+		refs/tags/broken-tag-*
    -+'
    -+
    - test_done

Comments

Junio C Hamano April 1, 2021, 7:56 p.m. UTC | #1

Ævar Arnfjörð Bjarmason  <[e-mail]> writes:

> Here's a proposed v2. We test the same case, but I thought it made
> sense to test this more exhaustively.

Let's first make a targetted fix that can be applied to maint and
below.  After that is merged to 'master', you are free to add more
tests on top, but let's avoid to have more and more topics that go
overboard.

Thanks.

Ævar Arnfjörð Bjarmason April 2, 2021, 11:37 a.m. UTC | #2

On Thu, Apr 01 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <[e-mail]> writes:
>
>> Here's a proposed v2. We test the same case, but I thought it made
>> sense to test this more exhaustively.
>
> Let's first make a targetted fix that can be applied to maint and
> below.  After that is merged to 'master', you are free to add more
> tests on top, 

Makes sense. I based Jeff's patch on top of mine to demonstrate that
those tests also catch the segfault.

> but let's avoid to have more and more topics that go overboard.

So "submit a new version on-top" or "maybe deal with your existing
topics first as you're overflowing my inbox" ? :) I suspect the latter..

Junio C Hamano April 2, 2021, 8:51 p.m. UTC | #3

Ævar Arnfjörð Bjarmason <[e-mail]> writes:

> On Thu, Apr 01 2021, Junio C Hamano wrote:
>
>> Ævar Arnfjörð Bjarmason  <[e-mail]> writes:
>>
>>> Here's a proposed v2. We test the same case, but I thought it made
>>> sense to test this more exhaustively.
>>
>> Let's first make a targetted fix that can be applied to maint and
>> below.  After that is merged to 'master', you are free to add more
>> tests on top, 
>
> Makes sense. I based Jeff's patch on top of mine to demonstrate that
> those tests also catch the segfault.
>
>> but let's avoid to have more and more topics that go overboard.
>
> So "submit a new version on-top" or "maybe deal with your existing
> topics first as you're overflowing my inbox" ? :) I suspect the latter..

What I meant is...

Comparing the five-patch series with Peff's small fix that is more
to the point, I have a feeling that the five-patch series, like many
other series from you, may be made unnecessarily large by not
resisting the temptation to including unessential "while at it"
changes.