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

From: Derrick Stolee <stolee@gmail.com>
Date: Dec 18, 2024, 14:21 UTC
Message-ID: <eda7d3b7-8c3b-4486-b292-2507ca7f3bf9@gmail.com>
In-Reply-To: <Z1whcUJE-MHAhULO@pks.im>

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:

Show 5 quoted lines

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

Show 6 quoted lines

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

Show 8 quoted lines

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

Show 9 quoted lines

>> +		/* 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.

Show 9 quoted lines

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