It is mentioned in object-store.h that the function
repo_has_object_file() is deprecated. One possible alternative for this
function is has_object() (or atleast that is how I understood it).
The file object-store.h also mentions that repo_has_object_file() and
its fellow functions and macros can be removed once the migrations take
place. This patch therefore is an attempt to reduce the usage of these
functions and macros.
I request for comments as I'm not really sure about the "flags" argument
of the has_object() function and its usage in this patch.
Signed-off-by: Kousik Sanagavarapu <redacted>
---
object.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
From: Jeff King <hidden> Date: 2022-11-16 18:20:29
On Wed, Nov 16, 2022 at 10:09:56PM +0530, Kousik Sanagavarapu wrote:
It is mentioned in object-store.h that the function
repo_has_object_file() is deprecated. One possible alternative for this
function is has_object() (or atleast that is how I understood it).
The file object-store.h also mentions that repo_has_object_file() and
its fellow functions and macros can be removed once the migrations take
place. This patch therefore is an attempt to reduce the usage of these
functions and macros.
I request for comments as I'm not really sure about the "flags" argument
of the has_object() function and its usage in this patch.
So you've stumbled into quite a tricky spot. :)
Yes, without specifying new flags, this patch has a change of behavior
which I think is not what we want. From 1d8d9cb620 (sha1-file: introduce
no-lazy-fetch has_object(), 2020-08-05):
There have been a few bugs wherein Git fetches missing objects
whenever the existence of an object is checked, even though it does
not need to perform such a fetch. To resolve these bugs, we could look
at all the places that has_object_file() (or a similar function) is
used. As a first step, introduce a new function has_object() that
checks for the existence of an object, with a default behavior of not
fetching if the object is missing and the repository is a partial
clone. As we verify each has_object_file() (or similar) usage, we can
replace it with has_object(), and we will know that we are done when
we can delete has_object_file() (and the other similar functions).
Also, the new function has_object() has more appropriate defaults:
besides not fetching, it also does not recheck packed storage.
So the new function will:
- not recheck packed objects unless we pass HAS_OBJECT_RECHECK_PACKED;
this is done in the default paths because a simultaneous git-gc may
be repacking objects (and we would rather double-check than racily
miss it). So it's appropriate behavior if the caller is
speculatively asking "hey, we _might_ have this object", but not if
we expect to have it.
- not lazily fetch objects in a partial-clone repository. This again
depends on the caller being in a situation where they are OK saying
"no we don't have it" for an object we _could_ get if it was worth
spending effort
The call you're touching here is in parse_object_with_flags(), which is
using the check as part of the "is it a blob that we should stream?"
check. If this call returns "no we don't have it", when we could get it
(either due to racy repack or by fetching), then we'll hit the non-blob
path that calls repo_read_object_file(). Where we would do a fresh
lookup, including re-checking packs and/or lazily fetching!
So the new behavior seems strictly worse to me. We don't avoid those
behaviors, _and_ we fail to follow the streaming-blob code path (which
means we may accidentally read a huge blob into memory. I think we'd
want to leave it as-is, or if we really want to eventually drop to a
single interface, we need has_object() to learn a new flag to enable the
lazy-fetch (and then use it along with RECHECK_PACKED).
Now all of that said, I am skeptical that these calls to
repo_has_object_file() are even doing anything useful at all. Looking at
the existing code (dropping the "+" lines from your patch):
we are checking that it exits if either:
- another object (e.g., a tree) referred to it as a blob during this
process, and we created an in-memory "struct object" with that type
- nobody has referred to it, and we want to check its type via
oid_object_info()
In the second case, this seems totally pointless. We can just ask
oid_object_info() what it's type is, and it will say "no, we don't have
it" if appropriate. It will do the usual recheck-pack and lazy-fetch,
but so is repo_has_object_file(), so the short-circuit "&&" is not
helping. _If_ we were to switch to has_object() it would start to do
something, but I think that's a bad idea for the reasons given above.
In the first case, I'd likewise argue it's not doing anything useful. It
is confirming that we have the object, but so would the call to
stream_object_signature() immediately below (assuming skip_hash is not
set). If skip_hash is set, then we could either:
1. Just assume we have it. The point of the caller passing skip_hash
is that we don't care about checking the integrity for this use
case, and "missing" is not really any different than "there's a
file on disk but it might contain garbage bytes".
2. Check repo_has_object_file() in this code path only when skip_hash
is set. That retains the same "do we even have it" check for
skip_hash that is performed now.
I.e., I'd suggest this patch to remove both calls entirely:
But there may be some subtlety I'm missing. I'm cc-ing Jonathan Tan, who
added has_object(), and who added the top call to repo_has_object_file()
via df11e19648 (rev-list: support termination at promisor objects,
2017-12-08). The second call is from 090ea12671 (parse_object: avoid
putting whole blob in core, 2012-03-07), but he's no longer active on
the project. Looking at the commit, I think it was just a case of "let's
be extra careful". But as far as I can tell, it's not helping anything,
and both calls are introducing extra work doing object lookups.
-Peff
But there may be some subtlety I'm missing. I'm cc-ing Jonathan Tan, who
added has_object(), and who added the top call to repo_has_object_file()
via df11e19648 (rev-list: support termination at promisor objects,
2017-12-08).
Thanks for CC-ing me on this. Looking at that commit and the code at that time,
I'm not sure why I added that call either. My best guess is that I was worried
that the streaming interface wouldn't support missing objects, but both then
and now, a call to istream_source() is made before any streaming occurs (which
does perform the lazy fetch).
So yes, I also think that you can remove these calls.
From: Jeff King <hidden> Date: 2022-11-17 22:37:35
On Wed, Nov 16, 2022 at 01:14:18PM -0800, Jonathan Tan wrote:
quoted
But there may be some subtlety I'm missing. I'm cc-ing Jonathan Tan, who
added has_object(), and who added the top call to repo_has_object_file()
via df11e19648 (rev-list: support termination at promisor objects,
2017-12-08).
Thanks for CC-ing me on this. Looking at that commit and the code at that time,
I'm not sure why I added that call either. My best guess is that I was worried
that the streaming interface wouldn't support missing objects, but both then
and now, a call to istream_source() is made before any streaming occurs (which
does perform the lazy fetch).
So yes, I also think that you can remove these calls.
Thanks. After staring at this a bit, I noticed there is an even more
subtle issue with the case you touched back then, which is that it fails
to notice when a think we expect to be a blob isn't one. Your old patch
didn't make anything worse there, but it also wasn't sufficient to catch
the problem. See patch 2 for details.
I'm adding Taylor to the cc as the author of t6102, when we were
tracking down all of these "oops, it's not really a blob" cases. This
fixes one of the lingering cases from that test script.
[1/2]: parse_object(): drop extra "has" check before checking object type
[2/2]: parse_object(): check on-disk type of suspected blob
object.c | 5 ++---
t/t6102-rev-list-unexpected-objects.sh | 4 ++--
2 files changed, 4 insertions(+), 5 deletions(-)
-Peff
From: Jeff King <hidden> Date: 2022-11-17 22:38:03
When parsing an object of unknown type, we check to see if it's a blob,
so we can use our streaming code path. This uses oid_object_info() to
check the type, but before doing so we call repo_has_object_file(). This
latter is pointless, as oid_object_info() will already fail if the
object is missing. Checking it ahead of time just complicates the code
and is a waste of resources (albeit small).
Let's drop the redundant check.
Signed-off-by: Jeff King <redacted>
---
object.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
From: Jeff King <hidden> Date: 2022-11-17 22:41:21
In parse_object(), we try to handle blobs by streaming rather than
loading them entirely into memory. The most common case here will be
that we haven't seen the object yet and check oid_object_info(), which
tells us we have a blob.
But we trigger this code on one other case: when we have an in-memory
object struct with type OBJ_BLOB (and without its "parsed" flag set,
since otherwise we'd return early from the function). This indicates
that some other part of the code suspected we have a blob (e.g., it was
mentioned by a tree or tag) but we haven't yet looked at the on-disk
copy.
In this case before hitting the streaming path, we check if we have the
object on-disk at all. This is mostly pointless extra work, as the
streaming path would complain if it couldn't open the object (albeit
with the message "hash mismatch", which is a little misleading).
But it's also insufficient to catch all problems. The streaming code
will only tell us "yes, the on-disk object matches the oid". But it
doesn't actually confirm that what we found was indeed a blob, and
neither does repo_has_object_file().
One way to improve this would be to teach stream_object_signature() to
check the type (either by returning it to us to check, or taking an
"expected" type). But there's an even simpler fix here: if we suspect
the object is a blob, just call oid_object_info() to confirm that we
have it on-disk, and that it really is a blob.
This is slightly less efficient than teaching stream_object_signature()
to do it (since it has to open the object already). But this case very
rarely comes up. In practice, we usually don't have any clue what the
type is, in which case we already call oid_object_info(). This
"suspected" case happens only when some other code created an object
struct but didn't actually parse the blob, which is actually tricky to
trigger at all (see the discussion of the test below).
I reworked the conditional a bit so that instead of:
if ((suspected_blob && oid_object_info() == OBJ_BLOB)
(no_clue && oid_object_info() == OBJ_BLOB)
we have the simpler:
if ((suspected_blob || no_clue) && oid_object_info() == OBJ_BLOB)
This is shorter, but also reflects what we really want say, which is
"have we ruled out this being a blob; if not, check it on-disk".
In either case, if oid_object_info() fails to tell us it's a blob, we'll
skip the streaming code path and call repo_read_object_file(), just as
before. And if we really do have a mismatch with the existing object
struct, we'll eventually call lookup_commit(), etc, via
parse_object_buffer(), which will complain that it doesn't match our
existing obj->type.
So this fixes one of the lingering expect_failure cases from 0616617c7e
(t: introduce tests for unexpected object types, 2019-04-09). That test
works by peeling a tag that claims to point to a blob (triggering us to
create the struct), but really points to something else, which we later
discover when we call parse_object() as part of the actual traversal).
Prior to this commit, we'd quietly check the sha1 and mark the blob as
"parsed". Now we correctly complain about the mismatch.
Signed-off-by: Jeff King <redacted>
---
As an aside, I found the "this test is marked as success but testing the
wrong thing" pattern here confusing to deal with (since I had to dig in
history to understand what was going on and what the test was _supposed_
to say).
It comes from cf10c5b4cf (rev-list tests: don't hide abort() in
"test_expect_failure", 2022-03-07). I'm skeptical that it was worth
switching those tests for leak detection purposes.
But more importantly, it looks like pw/test-todo would provide us with a
much nicer pattern there. It seems to be stalled on review, so let's see
if we can get that moving again.
object.c | 4 ++--
t/t6102-rev-list-unexpected-objects.sh | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
I reworked the conditional a bit so that instead of:
if ((suspected_blob && oid_object_info() == OBJ_BLOB)
(no_clue && oid_object_info() == OBJ_BLOB)
we have the simpler:
if ((suspected_blob || no_clue) && oid_object_info() == OBJ_BLOB)
[...]
But why:
if ((!x || (x && x->m)) && ...)
Instead of:
if ((!x || x->m)) && ...)
If "!obj" is false then "obj" must be non-NULL, so you don't need to
check it again and can lose the "obj &&".
[...]
So this fixes one of the lingering expect_failure cases from 0616617c7e
(t: introduce tests for unexpected object types, 2019-04-09). That test
works by peeling a tag that claims to point to a blob (triggering us to
create the struct), but really points to something else, which we later
discover when we call parse_object() as part of the actual traversal).
Prior to this commit, we'd quietly check the sha1 and mark the blob as
"parsed". Now we correctly complain about the mismatch.
I applied this on top of "master", and adjusted your test to be this
instead:
test_expect_success 'traverse unexpected non-blob tag (lone)' '
cat >expect <<-EOF &&
error: object $commit is a blob, not a commit
fatal: bad object $commit
EOF
test_must_fail git rev-list --objects $tag >out 2>actual &&
test_must_be_empty out &&
test_cmp expect actual
'
Which passes, showing that we're still not correctly identifying it, but
we are doing it for the purposes of erroring out, but the incorrect type
persists.
Now, this all does seem quite familiar... :) :
https://lore.kernel.org/git/patch-10.11-a84f670ac24-20210328T021238Z-avarab@gmail.com/
I.e. that's the rest of the fix for this issue. I applied this change on
my local branch with that, and they combine nicely. the "test_must_fail"
here works as intended, *and* we'll correctly report & store the type.
As an aside, I found the "this test is marked as success but testing the
wrong thing" pattern here confusing to deal with (since I had to dig in
history to understand what was going on and what the test was _supposed_
to say).
It comes from cf10c5b4cf (rev-list tests: don't hide abort() in
"test_expect_failure", 2022-03-07). I'm skeptical that it was worth
switching those tests for leak detection purposes.
It's not just for leak detection purposes that it was a good idea to
switch it away from test_expect_failure, but also that we've been
ensuring that this didn't turn into a segfault all this time by not
using "test_expect_failure".
But more importantly, it looks like pw/test-todo would provide us with a
much nicer pattern there. It seems to be stalled on review, so let's see
if we can get that moving again.
The "TODO (should fail!)" didn't stand out? But yeah, having a "todo" or
"test_expect_todo" or "test_expect_failure" not suck would be nice.
FWIW I think
https://lore.kernel.org/git/221006.86v8owr986.gmgdl@evledraar.gmail.com/
outlines a good way forward for it that I think should make everyone
happy.
I think you'll either want the test_cmp I noted above, or to do that in
a subsequent test_expect_failure.
I know that your stance is that you prefer not to "test the bad
behavior" as it were. Personally I thought an all-caps TODO comment
might make it less confusing, but anyway.
In this case your commit message claims you're happy with the end
result, so I think you'd want to test what we actually emit on stderr,
as it's quite ... unintuative.
Or, which I think probably makes more sense, add that as a subsequent
test_expect_failure or whatever. FWIW this somewhat un-idiomatic pattern
will get around the current caveats with it:
test_expect_success .... '
... >actual.non-blob-tag
'
test_lazy_prereq HAVE_NON_BLOB_TAG 'test -e actual.non-blob-tag'
test_expect_failure HAVE_NON_BLOB_TAG '...' '
cat >expect.non-blob-tag <<-\EOF &&
...
EOF
test_cmp expect.non-blob-tag actual.non-blob-tag
'
I.e. peel off the 'test_cmp" that should have a known-good state from
the already-good status code.
Fix a memory leak that's been with us ever since c879daa2372 (Make
hash-object more robust against malformed objects, 2011-02-05). With
"HASH_FORMAT_CHECK" (used by "hash-object" and "replace") we'll parse
tags into a throwaway variable on the stack, but weren't freeing the
"item->tag" we might malloc() when doing so.
Mark the tests that now pass in their entirety as passing under
"SANITIZE=leak", which means we'll test them as part of the
"linux-leaks" CI job.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
object-file.c | 1 +
t/t3800-mktag.sh | 1 +
t/t5302-pack-index.sh | 2 ++
3 files changed, 4 insertions(+)
@@ -4,6 +4,8 @@#test_description='pack index with 64-bit offsets and object CRC'++TEST_PASSES_SANITIZE_LEAK=true ../test-lib.sh test_expect_success'setup''
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.
We'll report the wrong object type in these cases, and thus fail on
the "test_cmp", e.g. for the first "error: " output being tested here
we should say "$commit is a tag, not a commit", instead we say
"$commit is a commit, not a tag". This will be fixed in a subsequent
commit.
See the discussion & notes in [1] and downthread of there for test
snippets that are adapted here.
In the case of "fsck" which objects we visit in what order, and if we
report errors on them depends on their OIDs. So the test uses the
technique of extracting the OID/type combinations that fsck does
report, and asserting that those are correct (currently, it's far from
correct).
1. https://lore.kernel.org/git/YGTGgFI19fS7Uv6I@coredump.intra.peff.net/
Helped-by: Jeff King [off-list ref]
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
t/t6102-rev-list-unexpected-objects.sh | 146 +++++++++++++++++++++++++
1 file changed, 146 insertions(+)
@@ -130,4 +130,150 @@ test_expect_success 'traverse unexpected non-blob tag (seen)' 'test_i18ngrep"not a blob"output'+test_expect_success'setup unexpected non-tag tag''+test_when_finished"git tag -d tag-commit tag-tag"&&++gittag-a-m"my tagged commit"tag-commit$commit&&+tag_commit=$(gitrev-parsetag-commit)&&+gittag-a-m"my tagged tag"tag-tagtag-commit&&+tag_tag=$(gitrev-parsetag-tag)&&++gitcat-filetagtag-tag>good-tag-tag&&+gitcat-filetagtag-commit>good-commit-tag&&++sed-e"s/$tag_commit/$commit/"<good-tag-tag>broken-tag-tag-commit&&+sed-e"s/$tag_commit/$tree/"<good-tag-tag>broken-tag-tag-tree&&+sed-e"s/$tag_commit/$blob/"<good-tag-tag>broken-tag-tag-blob&&++sed-e"s/$commit/$tag_commit/"<good-commit-tag>broken-commit-tag-tag&&+sed-e"s/$commit/$tree/"<good-commit-tag>broken-commit-tag-tree&&+sed-e"s/$commit/$blob/"<good-commit-tag>broken-commit-tag-blob&&++tag_tag_commit=$(githash-object-w-ttagbroken-tag-tag-commit)&&+tag_tag_tree=$(githash-object-w-ttagbroken-tag-tag-tree)&&+tag_tag_blob=$(githash-object-w-ttagbroken-tag-tag-blob)&&++gitupdate-refrefs/tags/tag_tag_commit$tag_tag_commit&&+gitupdate-refrefs/tags/tag_tag_tree$tag_tag_tree&&+gitupdate-refrefs/tags/tag_tag_blob$tag_tag_blob&&++commit_tag_tag=$(githash-object-w-ttagbroken-commit-tag-tag)&&+commit_tag_tree=$(githash-object-w-ttagbroken-commit-tag-tree)&&+commit_tag_blob=$(githash-object-w-ttagbroken-commit-tag-blob)&&++gitupdate-refrefs/tags/commit_tag_tag$commit_tag_tag&&+gitupdate-refrefs/tags/commit_tag_tree$commit_tag_tree&&+gitupdate-refrefs/tags/commit_tag_blob$commit_tag_blob+'++test_expect_failure'traverse unexpected incorrectly typed tag (to commit & tag)''+test_must_failgitrev-list--objects$tag_tag_commit2>err&&+cat>expect<<-EOF&&+error:object$commitisacommit,notatag+fatal:badobject$commit+EOF+test_cmpexpecterr&&++test_must_failgitrev-list--objects$commit_tag_tag2>err&&+cat>expect<<-EOF&&+error:object$tag_commitisatag,notacommit+fatal:badobject$tag_commit+EOF+test_cmpexpecterr+'++test_expect_failure'traverse unexpected incorrectly typed tag (to tree)''+test_must_failgitrev-list--objects$tag_tag_tree2>err&&+cat>expect<<-EOF&&+error:object$treeisatree,notatag+fatal:badobject$tree+EOF+test_cmpexpecterr&&++test_must_failgitrev-list--objects$commit_tag_tree2>err&&+cat>expect<<-EOF&&+error:object$treeisatree,notacommit+fatal:badobject$tree+EOF+test_cmpexpecterr+'++test_expect_failure'traverse unexpected incorrectly typed tag (to blob)''+test_must_failgitrev-list--objects$tag_tag_blob2>err&&+cat>expect<<-EOF&&+error:object$blobisablob,notatag+fatal:badobject$blob+EOF+test_cmpexpecterr&&++test_must_failgitrev-list--objects$commit_tag_blob2>err&&+cat>expect<<-EOF&&+error:object$blobisablob,notacommit+fatal:badobject$blob+EOF+test_cmpexpecterr+'++test_expect_failure'traverse unexpected non-tag tag (tree seen to blob)''+test_must_failgitrev-list--objects$tree$commit_tag_blob2>err&&+cat>expect<<-EOF&&+error:object$blobisablob,notacommit+fatal:badobject$blob+EOF+test_cmpexpecterr&&++test_must_failgitrev-list--objects$tree$tag_tag_blob2>err&&+cat>expect<<-EOF&&+error:object$blobisablob,notatag+fatal:badobject$blob+EOF+test_cmpexpecterr+'+++test_expect_failure'traverse unexpected objects with for-each-ref''+cat>expect<<-EOF&&+error:badtagpointerto$treein$tag_tag_tree+fatal:parse_object_bufferfailedon$tag_tag_treeforrefs/tags/tag_tag_tree+EOF+test_must_failgitfor-each-ref--format="%(*objectname)"2>actual&&+test_cmpexpectactual+'++>fsck-object-isa+test_expect_failure'setup: unexpected objects with fsck''+test_must_failgitfsck2>err&&+sed-n-e"/^error: object .* is a .*, not a .*$/ {+s/^error:object\([0-9a-f]*\)isa\([a-z]*\),nota[a-z]*$/\\1\\2/;+p;+}" <err >fsck-object-isa+'++whilereadoidtype+do+test_expect_failure"fsck knows unexpected object $oid is $type"'+gitcat-file-t$oid>expect&&+echo$type>actual&&+test_cmpexpectactual+'+done<fsck-object-isa++test_expect_success'traverse unexpected non-tag tag (blob seen to blob)''+test_must_failgitrev-list--objects$blob$commit_tag_blob2>err&&+cat>expected<<-EOF&&+error:object$blobisablob,notacommit+error:badtagpointerto$blobin$commit_tag_blob+fatal:badobject$commit_tag_blob+EOF+test_cmpexpectederr&&++test_must_failgitrev-list--objects$blob$tag_tag_blob2>err&&+cat>expected<<-EOF&&+error:object$blobisablob,notatag+error:badtagpointerto$blobin$tag_tag_blob+fatal:badobject$tag_tag_blob+EOF+test_cmpexpectederr+'+ test_done
This series fixes a very long-standing issue where we'll get confused
when we parse a tag whose "type" lies about the type of the target
object.
It goes on top of Jeff King's just-submitted [1], and the two
compliment one another. See [2] for my feedback about what was left
over, which this fixes.
Currently we'll parse tags and note what the "type" claims to be. Say
a pointer to a "blob" object that claims to be a "commit" in the
envelope.
Then when we we'd try to parse that supposed "commit' for real we'd
emit a message like:
error: object <oid> is a blob, not a commit
Which is reversed, i.e. we'd remember the first "blob" we saw, and
then get confused about seeing a "commit" when we did the actual
parsing.
This is now fixed in almost all cases by having the one caller of
parse_tag() which actually knows the type tell it "yes, I'm sure this
is a commit".
We'll then be able to see that we have a non-parsed object as
scaffolding, but that it's really a commit, and emit the correct:
error: object <oid> is a commit not a blob
Which goes along with other errors where the tag object itself yells
about being unhappy with the object reference.
I submitted a version of these patches back in early 2021[3], this is
significantly slimmed down since then.
At the time Jeff King noted that this approach inherently can't cover
all possible scenarios. I.e. sometimes our parsing of the envelope
isn't followed up by the "real" parse.
Even in those cases we can "get it right as 4/4 here demonstrates.
But there are going to be cases left where we get it wrong, but
they're all cases where we get it wrong now. It's probably not worth
fixing the long tail of those issues, but now we'll emit a sensible
error on the common case of "log" etc.
1. https://lore.kernel.org/git/Y3a3qcqNG8W3ueeb@coredump.intra.peff.net/
2. https://lore.kernel.org/git/221118.86cz9lgjxu.gmgdl@evledraar.gmail.com/
3. https://lore.kernel.org/git/YGTGgFI19fS7Uv6I@coredump.intra.peff.net/
Ævar Arnfjörð Bjarmason (4):
object-file.c: free the "t.tag" in check_tag()
object tests: add test for unexpected objects in tags
tag: don't misreport type of tagged objects in errors
tag: don't emit potentially incorrect "object is a X, not a Y"
blob.c | 11 +-
blob.h | 3 +
commit.c | 11 +-
commit.h | 2 +
object-file.c | 1 +
object.c | 20 +++-
object.h | 2 +
t/t3800-mktag.sh | 1 +
t/t5302-pack-index.sh | 2 +
t/t6102-rev-list-unexpected-objects.sh | 146 +++++++++++++++++++++++++
tag.c | 22 +++-
tag.h | 2 +
tree.c | 11 +-
tree.h | 2 +
14 files changed, 222 insertions(+), 14 deletions(-)
--
2.38.0.1511.gcdcff1f1dc2
As noted in the preceding commit we weren't handling cases where we
see a reference to a bad "type" in a "tag", but then end up not fully
parsing the object.
In those cases let's only claim that we have a bad tag pointer, but
emit "is a %s, not a %s".
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
t/t6102-rev-list-unexpected-objects.sh | 6 +++---
tag.c | 5 +++--
2 files changed, 6 insertions(+), 5 deletions(-)
@@ -231,7 +231,7 @@ test_expect_success 'traverse unexpected non-tag tag (tree seen to blob)' ''-test_expect_failure'traverse unexpected objects with for-each-ref''+test_expect_success'traverse unexpected objects with for-each-ref''cat>expect<<-EOF&&error:badtagpointerto$treein$tag_tag_treefatal:parse_object_bufferfailedon$tag_tag_treeforrefs/tags/tag_tag_tree
@@ -241,7 +241,7 @@ test_expect_failure 'traverse unexpected objects with for-each-ref' '' >fsck-object-isa-test_expect_failure'setup: unexpected objects with fsck''+test_expect_success'setup: unexpected objects with fsck''test_must_failgitfsck2>err&&sed-n-e"/^error: object .* is a .*, not a .*$/ {s/^error:object\([0-9a-f]*\)isa\([a-z]*\),nota[a-z]*$/\\1\\2/;
@@ -251,7 +251,7 @@ test_expect_failure 'setup: unexpected objects with fsck' 'whilereadoidtypedo-test_expect_failure"fsck knows unexpected object $oid is $type"'+test_expect_success"fsck knows unexpected object $oid is $type"'gitcat-file-t$oid>expect&&echo$type>actual&&test_cmpexpectactual
@@ -193,8 +193,9 @@ int parse_tag_buffer(struct repository *r, struct tag *item, const void *data, u}if(!item->tagged||strcmp(type_name(item->tagged->type),type)){-error(_("object %s is a %s, not a %s"),oid_to_hex(&oid),-type_name(item->tagged->type),type);+if(item->tagged&&item->tagged->parsed)+error(_("object %s is a %s, not a %s"),oid_to_hex(&oid),+type_name(item->tagged->type),type);returnerror("bad tag pointer to %s in %s",oid_to_hex(&oid),oid_to_hex(&item->object.oid));
Fix a regression in 89e4202f982 ([PATCH] Parse tags for absent
objects, 2005-06-21) (yes, that ancient!) and correctly report an
error on a tag like:
object <a tree hash>
type commit
As:
error: object <a tree hash> is tree, not a commit
Instead of our long-standing misbehavior of inverting the two, and
reporting:
error: object <a tree hash> is commit, not a tree
Which, as can be trivially seen with 'git cat-file -t <a tree hash>'
is incorrect.
The reason for this misreporting is that in parse_tag_buffer() we end
up doing a lookup_{blob,commit,tag,tree}() depending on what we read
out of the "type" line.
If we haven't parsed that object before we end up dispatching to the
type-specific lookup functions, e.g. this for commit.c in
lookup_commit_type():
struct object *obj = lookup_object(r, oid);
if (!obj)
return create_object(r, oid, alloc_commit_node(r));
Its allocation will then set the obj->type according to what the tag
told us the type was, but which we've never validated. At this point
we've got an object in memory that hasn't been parsed, and whose type
is incorrect, since we mistrusted a tag to tell us the type.
Then when we actually load the object with parse_object() we read it
and find that it's a "tree". See 8ff226a9d5e (add object_as_type
helper for casting objects, 2014-07-13) for that behavior (that's just
a refactoring commit, but shows all the code involved).
Which explains why we inverted the error report. Normally when
object_as_type() is called it's by the lookup_{blob,commit,tag,tree}()
functions via parse_object(). At that point we can trust the
obj->type.
In the case of parsing objects we've learned about via a tag with an
incorrect type it's the opposite, the obj->type isn't correct and
holds the mislabeled type, but we're parsing the object and know for
sure what object type we're dealing with.
So, let's add "lookup_{blob,commit,tag,tree}_type()" functions to go
with the existing ""lookup_{blob,commit,tag,tree}()", we'll call these
from "parse_object_buffer()" where we actually know the type, as
opposed to the "parse_tag_buffer()" code where we're just guessing
what it might be.
This only help with the cases where we do see the tag reference, and
then end up doing a full parse of the object. But as seen in the
"for-each-ref" and "fsck" tests we have cases where we'll never fully
parse it.
Those will be handled in a subsequent commit, but for now this handles
the common case of "show" etc. running into these.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
blob.c | 11 +++++++++--
blob.h | 3 +++
commit.c | 11 +++++++++--
commit.h | 2 ++
object.c | 20 ++++++++++++++++----
object.h | 2 ++
t/t6102-rev-list-unexpected-objects.sh | 8 ++++----
tag.c | 21 +++++++++++++++++----
tag.h | 2 ++
tree.c | 11 +++++++++--
tree.h | 2 ++
11 files changed, 75 insertions(+), 18 deletions(-)
@@ -177,6 +177,18 @@ void *object_as_type(struct object *obj, enum object_type type, int quiet)}}+void*object_as_type_hint(structobject*obj,enumobject_typetype,+enumobject_typehint)+{+if(hint!=OBJ_NONE&&obj->type!=OBJ_NONE&&obj->type!=type){+error(_("object %s is a %s, not a %s"),oid_to_hex(&obj->oid),+type_name(type),type_name(obj->type));+obj->type=type;+returnNULL;+}+returnobject_as_type(obj,type,0);;+}+structobject*lookup_unknown_object(structrepository*r,conststructobject_id*oid){structobject*obj=lookup_object(r,oid);
@@ -182,7 +182,7 @@ test_expect_failure 'traverse unexpected incorrectly typed tag (to commit & tag)test_cmpexpecterr'-test_expect_failure'traverse unexpected incorrectly typed tag (to tree)''+test_expect_success'traverse unexpected incorrectly typed tag (to tree)''test_must_failgitrev-list--objects$tag_tag_tree2>err&&cat>expect<<-EOF&&error:object$treeisatree,notatag
@@ -198,7 +198,7 @@ test_expect_failure 'traverse unexpected incorrectly typed tag (to tree)' 'test_cmpexpecterr'-test_expect_failure'traverse unexpected incorrectly typed tag (to blob)''+test_expect_success'traverse unexpected incorrectly typed tag (to blob)''test_must_failgitrev-list--objects$tag_tag_blob2>err&&cat>expect<<-EOF&&error:object$blobisablob,notatag
@@ -214,7 +214,7 @@ test_expect_failure 'traverse unexpected incorrectly typed tag (to blob)' 'test_cmpexpecterr'-test_expect_failure'traverse unexpected non-tag tag (tree seen to blob)''+test_expect_success'traverse unexpected non-tag tag (tree seen to blob)''test_must_failgitrev-list--objects$tree$commit_tag_blob2>err&&cat>expect<<-EOF&&error:object$blobisablob,notacommit
@@ -135,6 +141,7 @@ void release_tag_memory(struct tag *t)intparse_tag_buffer(structrepository*r,structtag*item,constvoid*data,unsignedlongsize){+structobject*obj;structobject_idoid;chartype[20];constchar*bufptr=data;
@@ -169,7 +176,10 @@ int parse_tag_buffer(struct repository *r, struct tag *item, const void *data, utype[nl-bufptr]='\0';bufptr=nl+1;-if(!strcmp(type,blob_type)){+obj=lookup_object(r,&oid);+if(obj){+item->tagged=obj;+}elseif(!strcmp(type,blob_type)){item->tagged=(structobject*)lookup_blob(r,&oid);}elseif(!strcmp(type,tree_type)){item->tagged=(structobject*)lookup_tree(r,&oid);
@@ -182,10 +192,13 @@ int parse_tag_buffer(struct repository *r, struct tag *item, const void *data, utype,oid_to_hex(&item->object.oid));}-if(!item->tagged)+if(!item->tagged||strcmp(type_name(item->tagged->type),type)){+error(_("object %s is a %s, not a %s"),oid_to_hex(&oid),+type_name(item->tagged->type),type);returnerror("bad tag pointer to %s in %s",oid_to_hex(&oid),oid_to_hex(&item->object.oid));+}if(bufptr+4<tail&&starts_with(bufptr,"tag "));/* good */
From: Taylor Blau <hidden> Date: 2022-11-18 19:07:13
On Thu, Nov 17, 2022 at 05:37:29PM -0500, Jeff King wrote:
I'm adding Taylor to the cc as the author of t6102, when we were
tracking down all of these "oops, it's not really a blob" cases. This
fixes one of the lingering cases from that test script.
[1/2]: parse_object(): drop extra "has" check before checking object type
[2/2]: parse_object(): check on-disk type of suspected blob
object.c | 5 ++---
t/t6102-rev-list-unexpected-objects.sh | 4 ++--
2 files changed, 4 insertions(+), 5 deletions(-)
A blast from the past :-).
I took a careful look at both of these patches and they looked good to
me, so let's start merging them down.
Thanks,
Taylor
But why:
if ((!x || (x && x->m)) && ...)
Instead of:
if ((!x || x->m)) && ...)
If "!obj" is false then "obj" must be non-NULL, so you don't need to
check it again and can lose the "obj &&".
Just that it was one more round of refactoring than I did. :)
I agree that it's much more readable. It looks like the original hit
'next', so I'll send a patch on top.
I applied this on top of "master", and adjusted your test to be this
instead:
test_expect_success 'traverse unexpected non-blob tag (lone)' '
cat >expect <<-EOF &&
error: object $commit is a blob, not a commit
fatal: bad object $commit
EOF
test_must_fail git rev-list --objects $tag >out 2>actual &&
test_must_be_empty out &&
test_cmp expect actual
'
Which passes, showing that we're still not correctly identifying it, but
we are doing it for the purposes of erroring out, but the incorrect type
persists.
Now, this all does seem quite familiar... :) :
https://lore.kernel.org/git/patch-10.11-a84f670ac24-20210328T021238Z-avarab@gmail.com/
I.e. that's the rest of the fix for this issue. I applied this change on
my local branch with that, and they combine nicely. the "test_must_fail"
here works as intended, *and* we'll correctly report & store the type.
Right. It's hitting the exact same code path as all of the other object
types now. You suggested adding to the test here, but I'd prefer not to
do that. Noticing that we have a type mismatch is what is fixed, and it
now does that just like all the other object types. Dealing with the
message reversal is orthogonal.
quoted
But more importantly, it looks like pw/test-todo would provide us with a
much nicer pattern there. It seems to be stalled on review, so let's see
if we can get that moving again.
The "TODO (should fail!)" didn't stand out? But yeah, having a "todo" or
"test_expect_todo" or "test_expect_failure" not suck would be nice.
I did double-take on the "TODO" just because that is not our usual
pattern, but that was easily fixed. What I really don't like about the
"switch failure to success" pattern is that it requires rewriting the
test to expect the wrong thing! So when somebody later fixes the bug,
they get a confusing failure, but must also rewrite the test back to
what it originally should have been.
That was not too hard here, where it was just replacing a
test_must_fail, but that earlier hunk in cf10c5b4cf that actualy adds in
expected output (that we know is the wrong thing to be printing!) seems
a bit over the top to me. Anybody who encounters it has to dig into the
history to understand what is going on.
-Peff
From: Jeff King <hidden> Date: 2022-11-21 19:27:06
On Fri, Nov 18, 2022 at 02:05:04PM -0500, Taylor Blau wrote:
On Thu, Nov 17, 2022 at 05:37:29PM -0500, Jeff King wrote:
quoted
I'm adding Taylor to the cc as the author of t6102, when we were
tracking down all of these "oops, it's not really a blob" cases. This
fixes one of the lingering cases from that test script.
[1/2]: parse_object(): drop extra "has" check before checking object type
[2/2]: parse_object(): check on-disk type of suspected blob
object.c | 5 ++---
t/t6102-rev-list-unexpected-objects.sh | 4 ++--
2 files changed, 4 insertions(+), 5 deletions(-)
A blast from the past :-).
I took a careful look at both of these patches and they looked good to
me, so let's start merging them down.
I saw this hit 'next', but I think Ævar's simplification suggestion is
worth taking. So here is a patch on top to do so (the original branch is
jk/parse-object-type-mismatch for the benefit of any newly-returned
maintainers).
I was going to do a "helped-by", but since the only thing in the patch
is the suggested change, I just handed over authorship. :)
I didn't forge a signoff, and I think mine is sufficient under DCO's
part (b), but Ævar please indicate if that's OK.
-- >8 --
From: Ævar Arnfjörð Bjarmason <redacted>
Subject: [PATCH] parse_object(): simplify blob conditional
Commit 8db2dad7a0 (parse_object(): check on-disk type of suspected blob,
2022-11-17) simplified the conditional for checking if we might have a
blob. But we can simplify it further. In:
!obj || (obj && obj->type == OBJ_BLOB)
the short-circuit "OR" means "obj" will always be true on the right-hand
side. The compiler almost certainly optimized that out anyway, but
dropping it makes the conditional easier to understand for humans.
Signed-off-by: Jeff King <redacted>
---
object.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
On Fri, Nov 18, 2022 at 02:05:04PM -0500, Taylor Blau wrote:
quoted
On Thu, Nov 17, 2022 at 05:37:29PM -0500, Jeff King wrote:
quoted
I'm adding Taylor to the cc as the author of t6102, when we were
tracking down all of these "oops, it's not really a blob" cases. This
fixes one of the lingering cases from that test script.
[1/2]: parse_object(): drop extra "has" check before checking object type
[2/2]: parse_object(): check on-disk type of suspected blob
object.c | 5 ++---
t/t6102-rev-list-unexpected-objects.sh | 4 ++--
2 files changed, 4 insertions(+), 5 deletions(-)
A blast from the past :-).
I took a careful look at both of these patches and they looked good to
me, so let's start merging them down.
I saw this hit 'next', but I think Ævar's simplification suggestion is
worth taking. So here is a patch on top to do so (the original branch is
jk/parse-object-type-mismatch for the benefit of any newly-returned
maintainers).
I was going to do a "helped-by", but since the only thing in the patch
is the suggested change, I just handed over authorship. :)
I didn't forge a signoff, and I think mine is sufficient under DCO's
part (b), but Ævar please indicate if that's OK.
This looks good to me, thanks for following up. In case my SOB is needed
feel free to add it, but it's fine without that too as far as I'm
concerned.
quoted hunk
-- >8 --
From: Ævar Arnfjörð Bjarmason <redacted>
Subject: [PATCH] parse_object(): simplify blob conditional
Commit 8db2dad7a0 (parse_object(): check on-disk type of suspected blob,
2022-11-17) simplified the conditional for checking if we might have a
blob. But we can simplify it further. In:
!obj || (obj && obj->type == OBJ_BLOB)
the short-circuit "OR" means "obj" will always be true on the right-hand
side. The compiler almost certainly optimized that out anyway, but
dropping it makes the conditional easier to understand for humans.
Signed-off-by: Jeff King <redacted>
---
object.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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.
We'll report the wrong object type in these cases, and thus fail on
the "test_cmp", e.g. for the first "error: " output being tested here
we should say "$commit is a tag, not a commit", instead we say
"$commit is a commit, not a tag". This will be fixed in a subsequent
commit.
See the discussion & notes in [1] and downthread of there for test
snippets that are adapted here.
In the case of "fsck" which objects we visit in what order, and if we
report errors on them depends on their OIDs. So the test uses the
technique of extracting the OID/type combinations that fsck does
report, and asserting that those are correct (currently, it's far from
correct).
As these tests happen to run into a memory leak skip them under
SANITIZE=leak, as the test file was previously marked leak-free in
[3]. There is a concurrent fix for the leak in question[4].
1. https://lore.kernel.org/git/YGTGgFI19fS7Uv6I@coredump.intra.peff.net/
2. https://lore.kernel.org/git/patch-18.20-aa4df0e1b5c-20221228T175512Z-avarab@gmail.com/
3. dd9cede9136 (leak tests: mark some rev-list tests as passing with
SANITIZE=leak, 2021-10-31)
4. https://lore.kernel.org/git/patch-18.20-aa4df0e1b5c-20221228T175512Z-avarab@gmail.com/
Helped-by: Jeff King [off-list ref]
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
t/t6102-rev-list-unexpected-objects.sh | 146 +++++++++++++++++++++++++
1 file changed, 146 insertions(+)
@@ -130,4 +130,150 @@ test_expect_success 'traverse unexpected non-blob tag (seen)' 'test_i18ngrep"not a blob"output'+test_expect_success!SANITIZE_LEAK'setup unexpected non-tag tag''+test_when_finished"git tag -d tag-commit tag-tag"&&++gittag-a-m"my tagged commit"tag-commit$commit&&+tag_commit=$(gitrev-parsetag-commit)&&+gittag-a-m"my tagged tag"tag-tagtag-commit&&+tag_tag=$(gitrev-parsetag-tag)&&++gitcat-filetagtag-tag>good-tag-tag&&+gitcat-filetagtag-commit>good-commit-tag&&++sed-e"s/$tag_commit/$commit/"<good-tag-tag>broken-tag-tag-commit&&+sed-e"s/$tag_commit/$tree/"<good-tag-tag>broken-tag-tag-tree&&+sed-e"s/$tag_commit/$blob/"<good-tag-tag>broken-tag-tag-blob&&++sed-e"s/$commit/$tag_commit/"<good-commit-tag>broken-commit-tag-tag&&+sed-e"s/$commit/$tree/"<good-commit-tag>broken-commit-tag-tree&&+sed-e"s/$commit/$blob/"<good-commit-tag>broken-commit-tag-blob&&++tag_tag_commit=$(githash-object-w-ttagbroken-tag-tag-commit)&&+tag_tag_tree=$(githash-object-w-ttagbroken-tag-tag-tree)&&+tag_tag_blob=$(githash-object-w-ttagbroken-tag-tag-blob)&&++gitupdate-refrefs/tags/tag_tag_commit$tag_tag_commit&&+gitupdate-refrefs/tags/tag_tag_tree$tag_tag_tree&&+gitupdate-refrefs/tags/tag_tag_blob$tag_tag_blob&&++commit_tag_tag=$(githash-object-w-ttagbroken-commit-tag-tag)&&+commit_tag_tree=$(githash-object-w-ttagbroken-commit-tag-tree)&&+commit_tag_blob=$(githash-object-w-ttagbroken-commit-tag-blob)&&++gitupdate-refrefs/tags/commit_tag_tag$commit_tag_tag&&+gitupdate-refrefs/tags/commit_tag_tree$commit_tag_tree&&+gitupdate-refrefs/tags/commit_tag_blob$commit_tag_blob+'++test_expect_failure!SANITIZE_LEAK'traverse unexpected incorrectly typed tag (to commit & tag)''+test_must_failgitrev-list--objects$tag_tag_commit2>err&&+cat>expect<<-EOF&&+error:object$commitisacommit,notatag+fatal:badobject$commit+EOF+test_cmpexpecterr&&++test_must_failgitrev-list--objects$commit_tag_tag2>err&&+cat>expect<<-EOF&&+error:object$tag_commitisatag,notacommit+fatal:badobject$tag_commit+EOF+test_cmpexpecterr+'++test_expect_failure!SANITIZE_LEAK'traverse unexpected incorrectly typed tag (to tree)''+test_must_failgitrev-list--objects$tag_tag_tree2>err&&+cat>expect<<-EOF&&+error:object$treeisatree,notatag+fatal:badobject$tree+EOF+test_cmpexpecterr&&++test_must_failgitrev-list--objects$commit_tag_tree2>err&&+cat>expect<<-EOF&&+error:object$treeisatree,notacommit+fatal:badobject$tree+EOF+test_cmpexpecterr+'++test_expect_failure!SANITIZE_LEAK'traverse unexpected incorrectly typed tag (to blob)''+test_must_failgitrev-list--objects$tag_tag_blob2>err&&+cat>expect<<-EOF&&+error:object$blobisablob,notatag+fatal:badobject$blob+EOF+test_cmpexpecterr&&++test_must_failgitrev-list--objects$commit_tag_blob2>err&&+cat>expect<<-EOF&&+error:object$blobisablob,notacommit+fatal:badobject$blob+EOF+test_cmpexpecterr+'++test_expect_failure!SANITIZE_LEAK'traverse unexpected non-tag tag (tree seen to blob)''+test_must_failgitrev-list--objects$tree$commit_tag_blob2>err&&+cat>expect<<-EOF&&+error:object$blobisablob,notacommit+fatal:badobject$blob+EOF+test_cmpexpecterr&&++test_must_failgitrev-list--objects$tree$tag_tag_blob2>err&&+cat>expect<<-EOF&&+error:object$blobisablob,notatag+fatal:badobject$blob+EOF+test_cmpexpecterr+'+++test_expect_failure!SANITIZE_LEAK'traverse unexpected objects with for-each-ref''+cat>expect<<-EOF&&+error:badtagpointerto$treein$tag_tag_tree+fatal:parse_object_bufferfailedon$tag_tag_treeforrefs/tags/tag_tag_tree+EOF+test_must_failgitfor-each-ref--format="%(*objectname)"2>actual&&+test_cmpexpectactual+'++>fsck-object-isa+test_expect_success'setup: unexpected objects with fsck''+test_must_failgitfsck2>err&&+sed-n-e"/^error: object .* is a .*, not a .*$/ {+s/^error:object\([0-9a-f]*\)isa\([a-z]*\),nota[a-z]*$/\\1\\2/;+p;+}" <err >fsck-object-isa+'++whilereadoidtype+do+test_expect_failure"fsck knows unexpected object $oid is $type"'+gitcat-file-t$oid>expect&&+echo$type>actual&&+test_cmpexpectactual+'+done<fsck-object-isa++test_expect_success!SANITIZE_LEAK'traverse unexpected non-tag tag (blob seen to blob)''+test_must_failgitrev-list--objects$blob$commit_tag_blob2>err&&+cat>expected<<-EOF&&+error:object$blobisablob,notacommit+error:badtagpointerto$blobin$commit_tag_blob+fatal:badobject$commit_tag_blob+EOF+test_cmpexpectederr&&++test_must_failgitrev-list--objects$blob$tag_tag_blob2>err&&+cat>expected<<-EOF&&+error:object$blobisablob,notatag+error:badtagpointerto$blobin$tag_tag_blob+fatal:badobject$tag_tag_blob+EOF+test_cmpexpectederr+'+ test_done
This series fixes a very long-standing issue where we'll get confused
when we parse a tag whose "type" lies about the type of the target
object.
The v1 was on top of jk/parse-object-type-mismatch, which has since
landed on "master". As I noted in [1] this covers remaining
misreporting cases which weren't addressed in that series.
Currently we'll parse tags and note what the "type" claims to be. Say
a pointer to a "blob" object that claims to be a "commit" in the
envelope.
Then when we we'd try to parse that supposed "commit' for real we'd
emit a message like:
error: object <oid> is a blob, not a commit
Which is reversed, i.e. we'd remember the first "blob" we saw, and
then get confused about seeing a "commit" when we did the actual
parsing.
This is now fixed in almost all cases by having the one caller of
parse_tag() which actually knows the type tell it "yes, I'm sure this
is a commit".
We'll then be able to see that we have a non-parsed object as
scaffolding, but that it's really a commit, and emit the correct:
error: object <oid> is a commit not a blob
Which goes along with other errors where the tag object itself yells
about being unhappy with the object reference.
I submitted a version of these patches back in early 2021[2], this is
significantly slimmed down since then.
At the time Jeff King noted[3] that this approach inherently can't cover
all possible scenarios. I.e. sometimes our parsing of the envelope
isn't followed up by the "real" parse.
Even in those cases we can "get it right as 3/3 here demonstrates.
But there are going to be cases left where we get it wrong, but
they're all cases where we get it wrong now. It's probably not worth
fixing the long tail of those issues, but now we'll emit a sensible
error on the common case of "log" etc.
Changes since v1:
* The v1 of this included a fix for the t.tag memory leak, which has
now been ejected. I'm fixing that in another series[4]
As a result we need to mark the new tests with !SANITIZE_LEAK, once
some version of [4] lands we can un-mark these, so we'll test them
under SANITIZE=leak.
* In the previous 1st patch I marked a "setup" test as
"test_expect_failure", which will pass at that point, let's make it
"test_expect_success" from the outset.
CI & branch at [5]. The "win build" CI failure is unrelated, it also
happens when I re-push master, root cause unknown, but unrelated to
this topic.
1. https://lore.kernel.org/git/221118.86cz9lgjxu.gmgdl@evledraar.gmail.com/
2. https://lore.kernel.org/git/cover-00.11-00000000000-20210328T021238Z-avarab@gmail.com/
3. https://lore.kernel.org/git/YGTGgFI19fS7Uv6I@coredump.intra.peff.net/
4. https://lore.kernel.org/git/cover-00.20-00000000000-20221228T175512Z-avarab@gmail.com/
5. https://github.com/avar/git/tree/avar/correct-object-as-type-minimal-2
Ævar Arnfjörð Bjarmason (3):
object tests: add test for unexpected objects in tags
tag: don't misreport type of tagged objects in errors
tag: don't emit potentially incorrect "object is a X, not a Y"
blob.c | 11 +-
blob.h | 3 +
commit.c | 11 +-
commit.h | 2 +
object.c | 20 +++-
object.h | 2 +
t/t6102-rev-list-unexpected-objects.sh | 146 +++++++++++++++++++++++++
tag.c | 22 +++-
tag.h | 2 +
tree.c | 11 +-
tree.h | 2 +
11 files changed, 218 insertions(+), 14 deletions(-)
Range-diff against v1:
1: 2be8477cd78 < -: ----------- object-file.c: free the "t.tag" in check_tag()
2: 1b5544ec868 ! 1: 0abf873f1e3 object tests: add test for unexpected objects in tags
@@ Commit message
report, and asserting that those are correct (currently, it's far from
correct).
+ As these tests happen to run into a memory leak skip them under
+ SANITIZE=leak, as the test file was previously marked leak-free in
+ [3]. There is a concurrent fix for the leak in question[4].
+
1. https://lore.kernel.org/git/YGTGgFI19fS7Uv6I@coredump.intra.peff.net/
+ 2. https://lore.kernel.org/git/patch-18.20-aa4df0e1b5c-20221228T175512Z-avarab@gmail.com/
+ 3. dd9cede9136 (leak tests: mark some rev-list tests as passing with
+ SANITIZE=leak, 2021-10-31)
+ 4. https://lore.kernel.org/git/patch-18.20-aa4df0e1b5c-20221228T175512Z-avarab@gmail.com/
Helped-by: Jeff King [off-list ref]
Signed-off-by: Ævar Arnfjörð Bjarmason [off-list ref]
@@ t/t6102-rev-list-unexpected-objects.sh: test_expect_success 'traverse unexpected
test_i18ngrep "not a blob" output
'
-+test_expect_success 'setup unexpected non-tag tag' '
++test_expect_success !SANITIZE_LEAK 'setup unexpected non-tag tag' '
+ test_when_finished "git tag -d tag-commit tag-tag" &&
+
+ git tag -a -m"my tagged commit" tag-commit $commit &&
@@ t/t6102-rev-list-unexpected-objects.sh: test_expect_success 'traverse unexpected
+ git update-ref refs/tags/commit_tag_blob $commit_tag_blob
+'
+
-+test_expect_failure 'traverse unexpected incorrectly typed tag (to commit & tag)' '
++test_expect_failure !SANITIZE_LEAK 'traverse unexpected incorrectly typed tag (to commit & tag)' '
+ test_must_fail git rev-list --objects $tag_tag_commit 2>err &&
+ cat >expect <<-EOF &&
+ error: object $commit is a commit, not a tag
@@ t/t6102-rev-list-unexpected-objects.sh: test_expect_success 'traverse unexpected
+ test_cmp expect err
+'
+
-+test_expect_failure 'traverse unexpected incorrectly typed tag (to tree)' '
++test_expect_failure !SANITIZE_LEAK 'traverse unexpected incorrectly typed tag (to tree)' '
+ test_must_fail git rev-list --objects $tag_tag_tree 2>err &&
+ cat >expect <<-EOF &&
+ error: object $tree is a tree, not a tag
@@ t/t6102-rev-list-unexpected-objects.sh: test_expect_success 'traverse unexpected
+ test_cmp expect err
+'
+
-+test_expect_failure 'traverse unexpected incorrectly typed tag (to blob)' '
++test_expect_failure !SANITIZE_LEAK 'traverse unexpected incorrectly typed tag (to blob)' '
+ test_must_fail git rev-list --objects $tag_tag_blob 2>err &&
+ cat >expect <<-EOF &&
+ error: object $blob is a blob, not a tag
@@ t/t6102-rev-list-unexpected-objects.sh: test_expect_success 'traverse unexpected
+ test_cmp expect err
+'
+
-+test_expect_failure 'traverse unexpected non-tag tag (tree seen to blob)' '
++test_expect_failure !SANITIZE_LEAK 'traverse unexpected non-tag tag (tree seen to blob)' '
+ test_must_fail git rev-list --objects $tree $commit_tag_blob 2>err &&
+ cat >expect <<-EOF &&
+ error: object $blob is a blob, not a commit
@@ t/t6102-rev-list-unexpected-objects.sh: test_expect_success 'traverse unexpected
+'
+
+
-+test_expect_failure 'traverse unexpected objects with for-each-ref' '
++test_expect_failure !SANITIZE_LEAK 'traverse unexpected objects with for-each-ref' '
+ cat >expect <<-EOF &&
+ error: bad tag pointer to $tree in $tag_tag_tree
+ fatal: parse_object_buffer failed on $tag_tag_tree for refs/tags/tag_tag_tree
@@ t/t6102-rev-list-unexpected-objects.sh: test_expect_success 'traverse unexpected
+'
+
+>fsck-object-isa
-+test_expect_failure 'setup: unexpected objects with fsck' '
++test_expect_success 'setup: unexpected objects with fsck' '
+ test_must_fail git fsck 2>err &&
+ sed -n -e "/^error: object .* is a .*, not a .*$/ {
+ s/^error: object \([0-9a-f]*\) is a \([a-z]*\), not a [a-z]*$/\\1 \\2/;
@@ t/t6102-rev-list-unexpected-objects.sh: test_expect_success 'traverse unexpected
+ '
+done <fsck-object-isa
+
-+test_expect_success 'traverse unexpected non-tag tag (blob seen to blob)' '
++test_expect_success !SANITIZE_LEAK 'traverse unexpected non-tag tag (blob seen to blob)' '
+ test_must_fail git rev-list --objects $blob $commit_tag_blob 2>err &&
+ cat >expected <<-EOF &&
+ error: object $blob is a blob, not a commit
3: 468af961dc4 ! 2: 96398731841 tag: don't misreport type of tagged objects in errors
@@ blob.c
+ return lookup_blob_type(r, oid, OBJ_NONE);
}
- int parse_blob_buffer(struct blob *item, void *buffer, unsigned long size)
+ void parse_blob_buffer(struct blob *item)
## blob.h ##
@@ blob.h: struct blob {
@@ blob.h: struct blob {
+ const struct object_id *oid,
+ enum object_type type);
- int parse_blob_buffer(struct blob *item, void *buffer, unsigned long size);
-
+ /**
+ * Blobs do not contain references to other objects and do not have
## commit.c ##
@@ commit.c: struct commit *lookup_commit_object(struct repository *r,
@@ object.c: struct object *parse_object_buffer(struct repository *r, const struct
- struct blob *blob = lookup_blob(r, oid);
+ struct blob *blob = lookup_blob_type(r, oid, type);
if (blob) {
- if (parse_blob_buffer(blob, buffer, size))
- return NULL;
+ parse_blob_buffer(blob);
obj = &blob->object;
}
} else if (type == OBJ_TREE) {
@@ object.h: struct object *lookup_object(struct repository *r, const struct object
* Returns the object, having parsed it to find out what it is.
## t/t6102-rev-list-unexpected-objects.sh ##
-@@ t/t6102-rev-list-unexpected-objects.sh: test_expect_success 'setup unexpected non-tag tag' '
+@@ t/t6102-rev-list-unexpected-objects.sh: test_expect_success !SANITIZE_LEAK 'setup unexpected non-tag tag' '
git update-ref refs/tags/commit_tag_blob $commit_tag_blob
'
--test_expect_failure 'traverse unexpected incorrectly typed tag (to commit & tag)' '
-+test_expect_success 'traverse unexpected incorrectly typed tag (to commit & tag)' '
+-test_expect_failure !SANITIZE_LEAK 'traverse unexpected incorrectly typed tag (to commit & tag)' '
++test_expect_success !SANITIZE_LEAK 'traverse unexpected incorrectly typed tag (to commit & tag)' '
test_must_fail git rev-list --objects $tag_tag_commit 2>err &&
cat >expect <<-EOF &&
error: object $commit is a commit, not a tag
-@@ t/t6102-rev-list-unexpected-objects.sh: test_expect_failure 'traverse unexpected incorrectly typed tag (to commit & tag)
+@@ t/t6102-rev-list-unexpected-objects.sh: test_expect_failure !SANITIZE_LEAK 'traverse unexpected incorrectly typed tag (t
test_cmp expect err
'
--test_expect_failure 'traverse unexpected incorrectly typed tag (to tree)' '
-+test_expect_success 'traverse unexpected incorrectly typed tag (to tree)' '
+-test_expect_failure !SANITIZE_LEAK 'traverse unexpected incorrectly typed tag (to tree)' '
++test_expect_success !SANITIZE_LEAK 'traverse unexpected incorrectly typed tag (to tree)' '
test_must_fail git rev-list --objects $tag_tag_tree 2>err &&
cat >expect <<-EOF &&
error: object $tree is a tree, not a tag
-@@ t/t6102-rev-list-unexpected-objects.sh: test_expect_failure 'traverse unexpected incorrectly typed tag (to tree)' '
+@@ t/t6102-rev-list-unexpected-objects.sh: test_expect_failure !SANITIZE_LEAK 'traverse unexpected incorrectly typed tag (t
test_cmp expect err
'
--test_expect_failure 'traverse unexpected incorrectly typed tag (to blob)' '
-+test_expect_success 'traverse unexpected incorrectly typed tag (to blob)' '
+-test_expect_failure !SANITIZE_LEAK 'traverse unexpected incorrectly typed tag (to blob)' '
++test_expect_success !SANITIZE_LEAK 'traverse unexpected incorrectly typed tag (to blob)' '
test_must_fail git rev-list --objects $tag_tag_blob 2>err &&
cat >expect <<-EOF &&
error: object $blob is a blob, not a tag
-@@ t/t6102-rev-list-unexpected-objects.sh: test_expect_failure 'traverse unexpected incorrectly typed tag (to blob)' '
+@@ t/t6102-rev-list-unexpected-objects.sh: test_expect_failure !SANITIZE_LEAK 'traverse unexpected incorrectly typed tag (t
test_cmp expect err
'
--test_expect_failure 'traverse unexpected non-tag tag (tree seen to blob)' '
-+test_expect_success 'traverse unexpected non-tag tag (tree seen to blob)' '
+-test_expect_failure !SANITIZE_LEAK 'traverse unexpected non-tag tag (tree seen to blob)' '
++test_expect_success !SANITIZE_LEAK 'traverse unexpected non-tag tag (tree seen to blob)' '
test_must_fail git rev-list --objects $tree $commit_tag_blob 2>err &&
cat >expect <<-EOF &&
error: object $blob is a blob, not a commit
4: 1a9dcb9e05d ! 3: 2493988c41c tag: don't emit potentially incorrect "object is a X, not a Y"
@@ Commit message
Signed-off-by: Ævar Arnfjörð Bjarmason [off-list ref]
## t/t6102-rev-list-unexpected-objects.sh ##
-@@ t/t6102-rev-list-unexpected-objects.sh: test_expect_success 'traverse unexpected non-tag tag (tree seen to blob)' '
+@@ t/t6102-rev-list-unexpected-objects.sh: test_expect_success !SANITIZE_LEAK 'traverse unexpected non-tag tag (tree seen t
'
--test_expect_failure 'traverse unexpected objects with for-each-ref' '
-+test_expect_success 'traverse unexpected objects with for-each-ref' '
+-test_expect_failure !SANITIZE_LEAK 'traverse unexpected objects with for-each-ref' '
++test_expect_success !SANITIZE_LEAK 'traverse unexpected objects with for-each-ref' '
cat >expect <<-EOF &&
error: bad tag pointer to $tree in $tag_tag_tree
fatal: parse_object_buffer failed on $tag_tag_tree for refs/tags/tag_tag_tree
-@@ t/t6102-rev-list-unexpected-objects.sh: test_expect_failure 'traverse unexpected objects with for-each-ref' '
- '
-
- >fsck-object-isa
--test_expect_failure 'setup: unexpected objects with fsck' '
-+test_expect_success 'setup: unexpected objects with fsck' '
- test_must_fail git fsck 2>err &&
- sed -n -e "/^error: object .* is a .*, not a .*$/ {
- s/^error: object \([0-9a-f]*\) is a \([a-z]*\), not a [a-z]*$/\\1 \\2/;
-@@ t/t6102-rev-list-unexpected-objects.sh: test_expect_failure 'setup: unexpected objects with fsck' '
+@@ t/t6102-rev-list-unexpected-objects.sh: test_expect_success 'setup: unexpected objects with fsck' '
while read oid type
do
--
2.39.0.1153.g589e4efe9dc
As noted in the preceding commit we weren't handling cases where we
see a reference to a bad "type" in a "tag", but then end up not fully
parsing the object.
In those cases let's only claim that we have a bad tag pointer, but
emit "is a %s, not a %s".
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
t/t6102-rev-list-unexpected-objects.sh | 4 ++--
tag.c | 5 +++--
2 files changed, 5 insertions(+), 4 deletions(-)
@@ -193,8 +193,9 @@ int parse_tag_buffer(struct repository *r, struct tag *item, const void *data, u}if(!item->tagged||strcmp(type_name(item->tagged->type),type)){-error(_("object %s is a %s, not a %s"),oid_to_hex(&oid),-type_name(item->tagged->type),type);+if(item->tagged&&item->tagged->parsed)+error(_("object %s is a %s, not a %s"),oid_to_hex(&oid),+type_name(item->tagged->type),type);returnerror("bad tag pointer to %s in %s",oid_to_hex(&oid),oid_to_hex(&item->object.oid));
Fix a regression in 89e4202f982 ([PATCH] Parse tags for absent
objects, 2005-06-21) (yes, that ancient!) and correctly report an
error on a tag like:
object <a tree hash>
type commit
As:
error: object <a tree hash> is tree, not a commit
Instead of our long-standing misbehavior of inverting the two, and
reporting:
error: object <a tree hash> is commit, not a tree
Which, as can be trivially seen with 'git cat-file -t <a tree hash>'
is incorrect.
The reason for this misreporting is that in parse_tag_buffer() we end
up doing a lookup_{blob,commit,tag,tree}() depending on what we read
out of the "type" line.
If we haven't parsed that object before we end up dispatching to the
type-specific lookup functions, e.g. this for commit.c in
lookup_commit_type():
struct object *obj = lookup_object(r, oid);
if (!obj)
return create_object(r, oid, alloc_commit_node(r));
Its allocation will then set the obj->type according to what the tag
told us the type was, but which we've never validated. At this point
we've got an object in memory that hasn't been parsed, and whose type
is incorrect, since we mistrusted a tag to tell us the type.
Then when we actually load the object with parse_object() we read it
and find that it's a "tree". See 8ff226a9d5e (add object_as_type
helper for casting objects, 2014-07-13) for that behavior (that's just
a refactoring commit, but shows all the code involved).
Which explains why we inverted the error report. Normally when
object_as_type() is called it's by the lookup_{blob,commit,tag,tree}()
functions via parse_object(). At that point we can trust the
obj->type.
In the case of parsing objects we've learned about via a tag with an
incorrect type it's the opposite, the obj->type isn't correct and
holds the mislabeled type, but we're parsing the object and know for
sure what object type we're dealing with.
So, let's add "lookup_{blob,commit,tag,tree}_type()" functions to go
with the existing ""lookup_{blob,commit,tag,tree}()", we'll call these
from "parse_object_buffer()" where we actually know the type, as
opposed to the "parse_tag_buffer()" code where we're just guessing
what it might be.
This only help with the cases where we do see the tag reference, and
then end up doing a full parse of the object. But as seen in the
"for-each-ref" and "fsck" tests we have cases where we'll never fully
parse it.
Those will be handled in a subsequent commit, but for now this handles
the common case of "show" etc. running into these.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
blob.c | 11 +++++++++--
blob.h | 3 +++
commit.c | 11 +++++++++--
commit.h | 2 ++
object.c | 20 ++++++++++++++++----
object.h | 2 ++
t/t6102-rev-list-unexpected-objects.sh | 8 ++++----
tag.c | 21 +++++++++++++++++----
tag.h | 2 ++
tree.c | 11 +++++++++--
tree.h | 2 ++
11 files changed, 75 insertions(+), 18 deletions(-)
@@ -177,6 +177,18 @@ void *object_as_type(struct object *obj, enum object_type type, int quiet)}}+void*object_as_type_hint(structobject*obj,enumobject_typetype,+enumobject_typehint)+{+if(hint!=OBJ_NONE&&obj->type!=OBJ_NONE&&obj->type!=type){+error(_("object %s is a %s, not a %s"),oid_to_hex(&obj->oid),+type_name(type),type_name(obj->type));+obj->type=type;+returnNULL;+}+returnobject_as_type(obj,type,0);;+}+structobject*lookup_unknown_object(structrepository*r,conststructobject_id*oid){structobject*obj=lookup_object(r,oid);
@@ -182,7 +182,7 @@ test_expect_failure !SANITIZE_LEAK 'traverse unexpected incorrectly typed tag (ttest_cmpexpecterr'-test_expect_failure!SANITIZE_LEAK'traverse unexpected incorrectly typed tag (to tree)''+test_expect_success!SANITIZE_LEAK'traverse unexpected incorrectly typed tag (to tree)''test_must_failgitrev-list--objects$tag_tag_tree2>err&&cat>expect<<-EOF&&error:object$treeisatree,notatag
@@ -198,7 +198,7 @@ test_expect_failure !SANITIZE_LEAK 'traverse unexpected incorrectly typed tag (ttest_cmpexpecterr'-test_expect_failure!SANITIZE_LEAK'traverse unexpected incorrectly typed tag (to blob)''+test_expect_success!SANITIZE_LEAK'traverse unexpected incorrectly typed tag (to blob)''test_must_failgitrev-list--objects$tag_tag_blob2>err&&cat>expect<<-EOF&&error:object$blobisablob,notatag
@@ -214,7 +214,7 @@ test_expect_failure !SANITIZE_LEAK 'traverse unexpected incorrectly typed tag (ttest_cmpexpecterr'-test_expect_failure!SANITIZE_LEAK'traverse unexpected non-tag tag (tree seen to blob)''+test_expect_success!SANITIZE_LEAK'traverse unexpected non-tag tag (tree seen to blob)''test_must_failgitrev-list--objects$tree$commit_tag_blob2>err&&cat>expect<<-EOF&&error:object$blobisablob,notacommit
@@ -135,6 +141,7 @@ void release_tag_memory(struct tag *t)intparse_tag_buffer(structrepository*r,structtag*item,constvoid*data,unsignedlongsize){+structobject*obj;structobject_idoid;chartype[20];constchar*bufptr=data;
@@ -169,7 +176,10 @@ int parse_tag_buffer(struct repository *r, struct tag *item, const void *data, utype[nl-bufptr]='\0';bufptr=nl+1;-if(!strcmp(type,blob_type)){+obj=lookup_object(r,&oid);+if(obj){+item->tagged=obj;+}elseif(!strcmp(type,blob_type)){item->tagged=(structobject*)lookup_blob(r,&oid);}elseif(!strcmp(type,tree_type)){item->tagged=(structobject*)lookup_tree(r,&oid);
@@ -182,10 +192,13 @@ int parse_tag_buffer(struct repository *r, struct tag *item, const void *data, utype,oid_to_hex(&item->object.oid));}-if(!item->tagged)+if(!item->tagged||strcmp(type_name(item->tagged->type),type)){+error(_("object %s is a %s, not a %s"),oid_to_hex(&oid),+type_name(item->tagged->type),type);returnerror("bad tag pointer to %s in %s",oid_to_hex(&oid),oid_to_hex(&item->object.oid));+}if(bufptr+4<tail&&starts_with(bufptr,"tag "));/* good */