Git Mailing List Archive on lore.kernel.org
 help / color / Atom feed
From: Jeff King <peff@peff.net>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
	Taylor Blau <me@ttaylorr.com>, Elijah Newren <newren@gmail.com>,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>
Subject: Re: [PATCH 6/7] object tests: add test for unexpected objects in tags
Date: Sun, 28 Mar 2021 05:06:39 -0400
Message-ID: <YGBHH7sAVsPpVKWd@coredump.intra.peff.net> (raw)
In-Reply-To: <87h7kwnjpp.fsf@evledraar.gmail.com>

On Sun, Mar 28, 2021 at 03:35:46AM +0200, Ævar Arnfjörð Bjarmason wrote:

> >> Fix a blind spot in the tests added in 0616617c7e1 (t: introduce tests
> >> for unexpected object types, 2019-04-09), there were no meaningful
> >> tests for checking how we reported on finding the incorrect object
> >> type in a tag, i.e. one that broke the "type" promise in the tag
> >> header.
> >
> > Isn't this covered by tests 16 and 17 ("traverse unexpected non-commit
> > tag", both "lone" and "seen")? And likewise the matching "non-tree" and
> > "non-blob" variants afterwards?
> 
> Barely, those tests are mainly testing that rev-list doesn't die, and
> only do a very fuzzy match on the output. E.g. checking `grep "not a
> commit" err`, not a full test_cmp that'll check what OID is reported
> etc.

I thought the "blind spot" you meant was not testing these cases. But I
guess you mean that we are not checking stderr.

So OK, but...

> > The only thing we don't seem to cover is an unexpected non-tag. I don't
> > mind adding that, but why wouldn't we just follow the template of the
> > existing tests?
> 
> I am following that template to some extent, e.g. using
> ${commit,tree,blob}. It just didn't seem worth it to refactor an earlier
> test in the file just to re-use a single hash-object invocation, those
> tests e.g. clobber the $tag variable, so bending over backwards to
> re-use anything set up in them would mean some refactoring.
> 
> I think it's much clearer just do do all the different kinds of setup in
> the new setup function.

It does not seem to make the resulting test script more clear at all to
create the same situation twice, but test stderr only in the second
case. I.e., why is the change to the test script not just:

diff --git a/t/t6102-rev-list-unexpected-objects.sh b/t/t6102-rev-list-unexpected-objects.sh
index 52cde097dd..4cdc87c913 100755
--- a/t/t6102-rev-list-unexpected-objects.sh
+++ b/t/t6102-rev-list-unexpected-objects.sh
@@ -82,12 +82,13 @@ test_expect_success 'setup unexpected non-commit tag' '
 '
 
 test_expect_success 'traverse unexpected non-commit tag (lone)' '
-	test_must_fail git rev-list --objects $tag
+	test_must_fail git rev-list --objects $tag >output 2>&1 &&
+	test_i18ngrep "is a blob, not a commit" output
 '
 
 test_expect_success 'traverse unexpected non-commit tag (seen)' '
 	test_must_fail git rev-list --objects $blob $tag >output 2>&1 &&
-	test_i18ngrep "not a commit" output
+	test_i18ngrep "is a blob, not a commit" output
 '
 
 test_expect_success 'setup unexpected non-tree tag' '

and so forth (or you can replace it with a full test_cmp if you really
prefer). That does not seem like bending over backwards to me, but
rather keeping the test script tidy and readable.

But I wonder if there is something more going on that led you to have
trouble demonstrating your problem with the existing tests. Looking at
your follow-on patch that flips the "is an X, not a Y", I am not sure
this is something that we can actually handle reliably. At least not
without further access to the object database.

Because when we call, say, lookup_blob() and find that the object is
already in memory as a non-blob, we don't know who the culprit is.
Perhaps an earlier part of the code called parse_object(), found that it
really is a blob on disk, and used that type. But it may equally have
been the case that we saw a reference to the object as a commit, called
lookup_commit() on it, and now our lookup_blob() call is unhappy,
thinking it is really a commit. In that case, one of those references is
wrong, but we don't know which.

I think a robust solution would be one of:

  - make the message more precise: "saw object X as a commit, but
    previously it was referred to as a blob". Or vice versa.

  - when we see such a mismatch, go to the object database to say "aha,
    on disk it is really a blob". That's expensive, but this is an error
    case, so we can afford to be slow. But it does produce unsatisfying
    results when it was the earlier lookup_commit() call that was wrong.
    Because we have to say "object X is really a blob, but some object
    earlier referred to it as a commit. No idea who did that, though!".

-Peff

  reply index

Thread overview: 142+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2005-06-22  0:33 [PATCH 0/2] Pull objects of various types Daniel Barkalow
2005-06-22  0:35 ` [PATCH 1/2] Parse tags for absent objects Daniel Barkalow
2021-03-08 20:04   ` [PATCH 0/7] improve reporting of unexpected objects Ævar Arnfjörð Bjarmason
2021-03-28  2:13     ` [PATCH v2 00/10] " Ævar Arnfjörð Bjarmason
2021-03-28  2:13       ` [PATCH v2 01/10] object.c: stop supporting len == -1 in type_from_string_gently() Ævar Arnfjörð Bjarmason
2021-03-28  5:35         ` Junio C Hamano
2021-03-28 15:46           ` Ævar Arnfjörð Bjarmason
2021-03-28 18:25             ` Junio C Hamano
2021-04-22 18:09               ` Felipe Contreras
2021-03-28  2:13       ` [PATCH v2 02/10] object.c: refactor type_from_string_gently() Ævar Arnfjörð Bjarmason
2021-03-28  2:13       ` [PATCH v2 03/10] object.c: make type_from_string() return "enum object_type" Ævar Arnfjörð Bjarmason
2021-03-28  2:13       ` [PATCH v2 04/10] object-file.c: make oid_object_info() " Ævar Arnfjörð Bjarmason
2021-03-28  2:13       ` [PATCH v2 05/10] object-name.c: make dependency on object_type order more obvious Ævar Arnfjörð Bjarmason
2021-03-28  2:13       ` [PATCH v2 06/10] tree.c: fix misindentation in parse_tree_gently() Ævar Arnfjörð Bjarmason
2021-03-28  2:13       ` [PATCH v2 07/10] object.c: add a utility function for "expected type X, got Y" Ævar Arnfjörð Bjarmason
2021-03-28  2:13       ` [PATCH v2 08/10] object.c: add and use oid_is_type_or_die_msg() function Ævar Arnfjörð Bjarmason
2021-03-28  2:13       ` [PATCH v2 09/10] object tests: add test for unexpected objects in tags Ævar Arnfjörð Bjarmason
2021-03-28  2:13       ` [PATCH v2 10/10] tag: don't misreport type of tagged objects in errors Ævar Arnfjörð Bjarmason
2021-03-30  5:50         ` Junio C Hamano
2021-03-31 11:02           ` Jeff King
2021-03-31 18:05             ` Junio C Hamano
2021-03-31 18:31             ` Ævar Arnfjörð Bjarmason
2021-03-31 18:59               ` Jeff King
2021-03-31 20:46                 ` Ævar Arnfjörð Bjarmason
2021-04-01  7:54                   ` Jeff King
2021-04-01  8:32                     ` [PATCH] ref-filter: fix NULL check for parse object failure Jeff King
2021-04-01 13:56                       ` [PATCH v2 0/5] mktag tests & fix for-each-ref segfault Ævar Arnfjörð Bjarmason
2021-04-01 13:56                         ` [PATCH v2 1/5] mktag tests: parse out options in helper Ævar Arnfjörð Bjarmason
2021-04-01 13:56                         ` [PATCH v2 2/5] mktag tests: invert --no-strict test Ævar Arnfjörð Bjarmason
2021-04-01 13:56                         ` [PATCH v2 3/5] mktag tests: do fsck on failure Ævar Arnfjörð Bjarmason
2021-04-01 13:56                         ` [PATCH v2 4/5] mktag tests: test for maybe segfaulting for-each-ref Ævar Arnfjörð Bjarmason
2021-04-01 13:56                         ` [PATCH v2 5/5] ref-filter: fix NULL check for parse object failure Ævar Arnfjörð Bjarmason
2021-04-01 19:19                           ` Ramsay Jones
2021-04-01 19:56                         ` [PATCH v2 0/5] mktag tests & fix for-each-ref segfault Junio C Hamano
2021-04-02 11:37                           ` Ævar Arnfjörð Bjarmason
2021-04-02 20:51                             ` Junio C Hamano
2021-04-01 19:52                       ` [PATCH] ref-filter: fix NULL check for parse object failure Junio C Hamano
2021-03-31 18:41             ` [PATCH v2 10/10] tag: don't misreport type of tagged objects in errors Junio C Hamano
2021-03-31 19:00               ` Jeff King
2021-03-28  9:27       ` [PATCH v2 00/10] improve reporting of unexpected objects Jeff King
2021-03-29 13:34         ` Ævar Arnfjörð Bjarmason
2021-03-31 10:43           ` Jeff King
2021-04-09  8:07       ` [PATCH 0/2] blob/object.c: trivial readability improvements Ævar Arnfjörð Bjarmason
2021-04-09  8:07         ` [PATCH 1/2] blob.c: remove buffer & size arguments to parse_blob_buffer() Ævar Arnfjörð Bjarmason
2021-04-09 17:51           ` Jeff King
2021-04-09 22:31             ` Junio C Hamano
2021-04-10 12:57             ` Ævar Arnfjörð Bjarmason
2021-04-10 13:01               ` Ævar Arnfjörð Bjarmason
2021-04-13  8:25               ` Jeff King
2021-04-09  8:07         ` [PATCH 2/2] object.c: initialize automatic variable in lookup_object() Ævar Arnfjörð Bjarmason
2021-04-09 17:53           ` Jeff King
2021-04-09 22:32             ` Junio C Hamano
2021-04-09  8:32         ` [PATCH 0/6] {tag,object}*.c: refactorings + prep for a larger change Ævar Arnfjörð Bjarmason
2021-04-09  8:32           ` [PATCH 1/6] object.c: stop supporting len == -1 in type_from_string_gently() Ævar Arnfjörð Bjarmason
2021-04-09 18:06             ` Jeff King
2021-04-09 18:10               ` Jeff King
2021-04-09  8:32           ` [PATCH 2/6] object.c: remove "gently" argument to type_from_string_gently() Ævar Arnfjörð Bjarmason
2021-04-09 18:10             ` Jeff King
2021-04-09  8:32           ` [PATCH 3/6] object.c: make type_from_string() return "enum object_type" Ævar Arnfjörð Bjarmason
2021-04-09 18:14             ` Jeff King
2021-04-09 19:42               ` Ævar Arnfjörð Bjarmason
2021-04-09 21:29                 ` Jeff King
2021-04-09  8:32           ` [PATCH 4/6] object-file.c: make oid_object_info() " Ævar Arnfjörð Bjarmason
2021-04-09 18:24             ` Jeff King
2021-04-09  8:32           ` [PATCH 5/6] object-name.c: make dependency on object_type order more obvious Ævar Arnfjörð Bjarmason
2021-04-09 18:36             ` Jeff King
2021-04-09  8:32           ` [PATCH 6/6] tag.c: use type_from_string_gently() when parsing tags Ævar Arnfjörð Bjarmason
2021-04-09 18:42             ` Jeff King
2021-04-09  8:49           ` [PATCH 0/7] object.c: add and use "is expected" utility function + object_as_type() use Ævar Arnfjörð Bjarmason
2021-04-09  8:49             ` [PATCH 1/7] tree.c: fix misindentation in parse_tree_gently() Ævar Arnfjörð Bjarmason
2021-04-09  8:49             ` [PATCH 2/7] object.c: add a utility function for "expected type X, got Y" Ævar Arnfjörð Bjarmason
2021-04-09  8:49             ` [PATCH 3/7] object.c: add and use oid_is_type_or_die_msg() function Ævar Arnfjörð Bjarmason
2021-04-09  8:49             ` [PATCH 4/7] commit-graph: use obj->type, not object_as_type() Ævar Arnfjörð Bjarmason
2021-04-09  8:50             ` [PATCH 5/7] commit.c: don't use deref_tag() -> object_as_type() Ævar Arnfjörð Bjarmason
2021-04-09  8:50             ` [PATCH 6/7] object.c: normalize brace style in object_as_type() Ævar Arnfjörð Bjarmason
2021-04-09  8:50             ` [PATCH 7/7] object.c: remove "quiet" parameter from object_as_type() Ævar Arnfjörð Bjarmason
2021-04-20 13:36             ` [PATCH v2 0/8] object.c: add and use "is expected" utility function + object_as_type() use Ævar Arnfjörð Bjarmason
2021-04-20 13:36               ` [PATCH v2 1/8] tree.c: fix misindentation in parse_tree_gently() Ævar Arnfjörð Bjarmason
2021-04-20 13:36               ` [PATCH v2 2/8] object.c: add a utility function for "expected type X, got Y" Ævar Arnfjörð Bjarmason
2021-04-21 22:02                 ` Jonathan Tan
2021-04-22  6:10                   ` Ævar Arnfjörð Bjarmason
2021-04-20 13:36               ` [PATCH v2 3/8] object.c: add and use oid_is_type_or_die_msg() function Ævar Arnfjörð Bjarmason
2021-04-21 22:07                 ` Jonathan Tan
2021-04-21 23:28                 ` Josh Steadmon
2021-04-28  4:12                   ` Junio C Hamano
2021-04-20 13:36               ` [PATCH v2 4/8] commit-graph: use obj->type, not object_as_type() Ævar Arnfjörð Bjarmason
2021-04-20 13:36               ` [PATCH v2 5/8] branch tests: assert lookup_commit_reference_gently() error Ævar Arnfjörð Bjarmason
2021-04-20 13:36               ` [PATCH v2 6/8] commit.c: don't use deref_tag() -> object_as_type() Ævar Arnfjörð Bjarmason
2021-04-21 22:26                 ` Jonathan Tan
2021-04-20 13:36               ` [PATCH v2 7/8] object.c: normalize brace style in object_as_type() Ævar Arnfjörð Bjarmason
2021-04-20 13:37               ` [PATCH v2 8/8] object.c: remove "quiet" parameter from object_as_type() Ævar Arnfjörð Bjarmason
2021-04-20 13:00           ` [PATCH v2 00/10] {tag,object}*.c: refactorings + prep for a larger change Ævar Arnfjörð Bjarmason
2021-04-20 13:00             ` [PATCH v2 01/10] object.c: stop supporting len == -1 in type_from_string_gently() Ævar Arnfjörð Bjarmason
2021-04-20 13:00             ` [PATCH v2 02/10] object.c: remove "gently" argument to type_from_string_gently() Ævar Arnfjörð Bjarmason
2021-04-20 13:00             ` [PATCH v2 03/10] object.c: make type_from_string() return "enum object_type" Ævar Arnfjörð Bjarmason
2021-04-20 13:00             ` [PATCH v2 04/10] object-file.c: make oid_object_info() " Ævar Arnfjörð Bjarmason
2021-04-20 13:00             ` [PATCH v2 05/10] object-name.c: make dependency on object_type order more obvious Ævar Arnfjörð Bjarmason
2021-04-20 13:00             ` [PATCH v2 06/10] tag.c: use type_from_string_gently() when parsing tags Ævar Arnfjörð Bjarmason
2021-04-20 13:00             ` [PATCH v2 07/10] hash-object: pass along type length to object.c Ævar Arnfjörð Bjarmason
2021-04-20 13:00             ` [PATCH v2 08/10] hash-object: refactor nested else/if/if into else if/else if Ævar Arnfjörð Bjarmason
2021-04-20 13:00             ` [PATCH v2 09/10] hash-object: show usage on invalid --type Ævar Arnfjörð Bjarmason
2021-04-20 13:00             ` [PATCH v2 10/10] object.c: move type_from_string() code to its last user Ævar Arnfjörð Bjarmason
2021-04-20 12:50         ` [PATCH v2 00/10] object.c et al: tests, small bug fixes etc Ævar Arnfjörð Bjarmason
2021-04-20 12:50           ` [PATCH v2 01/10] cat-file tests: test for bogus type name handling Ævar Arnfjörð Bjarmason
2021-04-29  4:15             ` Junio C Hamano
2021-04-20 12:50           ` [PATCH v2 02/10] hash-object tests: more detailed test for invalid type Ævar Arnfjörð Bjarmason
2021-04-20 12:50           ` [PATCH v2 03/10] mktree tests: add test for invalid object type Ævar Arnfjörð Bjarmason
2021-04-20 12:50           ` [PATCH v2 04/10] object-file.c: take type id, not string, in read_object_with_reference() Ævar Arnfjörð Bjarmason
2021-04-29  4:37             ` Junio C Hamano
2021-04-20 12:50           ` [PATCH v2 05/10] {commit,tree,blob,tag}.c: add a create_{commit,tree,blob,tag}() Ævar Arnfjörð Bjarmason
2021-04-29  4:45             ` Junio C Hamano
2021-04-29 12:01               ` Ævar Arnfjörð Bjarmason
2021-04-20 12:50           ` [PATCH v2 06/10] blob.c: remove parse_blob_buffer() Ævar Arnfjörð Bjarmason
2021-04-29  4:51             ` Junio C Hamano
2021-04-20 12:50           ` [PATCH v2 07/10] object.c: simplify return semantic of parse_object_buffer() Ævar Arnfjörð Bjarmason
2021-04-20 12:50           ` [PATCH v2 08/10] object.c: don't go past "len" under die() in type_from_string_gently() Ævar Arnfjörð Bjarmason
2021-04-29  4:55             ` Junio C Hamano
2021-04-20 12:50           ` [PATCH v2 09/10] mktree: stop setting *ntr++ to NIL Ævar Arnfjörð Bjarmason
2021-04-29  5:01             ` Junio C Hamano
2021-04-20 12:50           ` [PATCH v2 10/10] mktree: emit a more detailed error when the <type> is invalid Ævar Arnfjörð Bjarmason
2021-03-08 20:04   ` [PATCH 1/7] object.c: refactor type_from_string_gently() Ævar Arnfjörð Bjarmason
2021-03-08 20:52     ` Taylor Blau
2021-03-09 10:46     ` Jeff King
2021-03-08 20:04   ` [PATCH 2/7] object.c: make type_from_string() return "enum object_type" Ævar Arnfjörð Bjarmason
2021-03-08 20:56     ` Taylor Blau
2021-03-08 21:48     ` Junio C Hamano
2021-03-08 20:04   ` [PATCH 3/7] oid_object_info(): " Ævar Arnfjörð Bjarmason
2021-03-08 21:54     ` Junio C Hamano
2021-03-08 22:32       ` Junio C Hamano
2021-03-09 10:34     ` Jeff King
2021-03-08 20:04   ` [PATCH 4/7] tree.c: fix misindentation in parse_tree_gently() Ævar Arnfjörð Bjarmason
2021-03-08 20:04   ` [PATCH 5/7] object.c: add a utility function for "expected type X, got Y" Ævar Arnfjörð Bjarmason
2021-03-08 20:59     ` Taylor Blau
2021-03-08 22:15     ` Junio C Hamano
2021-03-08 20:04   ` [PATCH 6/7] object tests: add test for unexpected objects in tags Ævar Arnfjörð Bjarmason
2021-03-09 10:44     ` Jeff King
2021-03-28  1:35       ` Ævar Arnfjörð Bjarmason
2021-03-28  9:06         ` Jeff King [this message]
2021-03-28 15:39           ` Ævar Arnfjörð Bjarmason
2021-03-29  9:16             ` Jeff King
2021-03-08 20:04   ` [PATCH 7/7] tag: don't misreport type of tagged objects in errors Ævar Arnfjörð Bjarmason
2005-06-22  0:35 ` [PATCH 2/2] Pull misc objects Daniel Barkalow

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YGBHH7sAVsPpVKWd@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=me@ttaylorr.com \
    --cc=newren@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Git Mailing List Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/git/0 git/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 git git/ https://lore.kernel.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.git


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git