Re: [PATCH v3 04/12] path-walk: always emit directly-requested objects

[Derrick Stolee <stolee@gmail.com>]

On 5/11/2026 9:23 PM, Taylor Blau wrote:
> On Mon, May 11, 2026 at 06:13:01PM +0000, Derrick Stolee via GitGitGadget wrote:
>> We can tell that a path is part of the directly-referenced objects if its
>> path name starts with '/' (other paths, including root trees never have this
>> starting character). Create a path_is_for_direct_objects() to make this
>> meaning clear, especially as we add more references in the future as we
>> integrate the path-walk API with partial clone filter options.
> 
> I don't know that I have anything in the way of a better suggestion, but
> I can't help but feel like the `path_is_for_direct_objects()` check is
> somewhat brittle as-is.
> 
> I am not familiar enough with the path-walk.c internals to come up with
> a good suggestion off the top of my head, but I figured I'd raise it
> here in case you had thoughts on alternatives.

The brittle-ness is due to how trees and blobs without known paths are
added to sets that are emitted using distinct paths. The starting '/'
character does prevent the "/tagged-blobs" and "/tagged-trees" sets
from ever colliding with valid paths that were discovered starting from
root trees (with path "").

I could imagine a world where we store a more robust struct that has a
flag member to indicate that these objects were not discovered via
normal tree walking. Changing that could lead to this method not doing
what we expect it to do. But we are not in that future.

>> diff --git a/path-walk.c b/path-walk.c
>> index 6e426af433..59a7670c5b 100644
>> --- a/path-walk.c
>> +++ b/path-walk.c
>> @@ -248,6 +248,16 @@ static int add_tree_entries(struct path_walk_context *ctx,
>>  	return 0;
>>  }
>>
>> +/*
>> + * Paths starting with '/' (e.g., "/tags", "/tagged-blobs") hold objects that
>> + * were directly requested by 'pending' objects rather than discovered during
>> + * tree traversal.
>> + */
>> +static int path_is_for_direct_objects(const char *path)
>> +{
>> +	return path[0] == '/';
> 
> If we do end up keeping this approach, should we have a NULL check on
> path itself here? I think that could even be an ASSERT(), since
> something has gone wrong if we have a NULL at this point, but I'd rather
> die by an assertion rather than a segfault here if so.
The ASSERT() is a good idea to prevent incorrect use.

Thanks,
-Stolee