Re: [PATCH v3 1/7] path-walk: introduce an object walk by path

[Derrick Stolee <stolee@gmail.com>]

On 12/13/24 6:58 AM, Patrick Steinhardt wrote:
> On Fri, Dec 06, 2024 at 07:45:52PM +0000, Derrick Stolee via GitGitGadget wrote:

>> +	} else if (parse_tree_gently(tree, 1)) {
>> +		die("bad tree object %s", oid_to_hex(oid));
> 
> I wonder whether we maybe shouldn't die but instead return an error in
> the spirit of libification.

This is in fact something that is being tested when 'git pack-objects' has
the --path-walk feature. See "get an error for missing tree object" in
t5317 as an example.

It's not enough to fail, but we need to fail with this error message.

Has there been enough progress in the libification effort to establish a
pattern for returning an error message like "bad tree object %s" from an
API like this to the caller?

I will try using a "error(); return -1;" and consider that as the best
option for right now.

>> +		} else {
>> +			/* Wrong type? */
>> +			continue;
> 
> This code is unreachable, so we could make this a `BUG()`. Might also
> use a switch instead, but that's more of a stylistic question.

I think a BUG() would be good here.

>> +		}
>> +
>> +		if (!o) /* report error?*/
>> +			continue;
> 
> So this can only happen in case `lookup_tree()` or `lookup_blob()`
> run into an error. I think this error should definitely be bubbled up so
> that we don't silently skip tree entries in case of repo corruption.

Looks like I agreed with you but didn't follow through.

>> +		/* Skip this object if already seen. */
>> +		if (o->flags & SEEN)
>> +			continue;
>> +		o->flags |= SEEN;
> 
> This made me wonder: why do we only skip the object this late? Couldn't
> we already have done so immediately after we have looked up the object
> to avoid some work? If not, it might be useful to add a comment why it
> has to come this late.

I went to look to see if there was a reason, and at this point there is
not a good reason. This should be moved up to avoid some checks and path
manipulation.

I think that in a later patch, the use of the UNINTERESTING flag is
important to pass the flag even when the object is already SEEN. This is
probably cruft from an earlier version that passed the UNINTERESTING bits
in this part of the code.

>> +		if (!t) {
>> +			warning("could not find tree %s", oid_to_hex(oid));
>> +			continue;
>> +		}
> 
> Is this error something we should bubble up to the caller? As mentioned
> above, I'm being cautious about silently accepting potentially-corrupt
> data. Silent in the spirit of the caller not noticing it, not in the
> sense of the user not noticing it.
Can do.

Thanks for the careful review!
-Stolee