From: Derrick Stolee <stolee@gmail.com>
Date: Fri, 16 May 2025 15:27:17 GMT
Subject: Re: [PATCH v2 02/13] pack-objects: add --path-walk option
Message-ID: <6087094c-2639-4262-8026-89e4d8aa8c8f@gmail.com>
In-Reply-To: <052c534a-c2a4-4ff6-9130-4c1e96e1ef72@gmail.com>

On 5/6/25 3:39 PM, Derrick Stolee wrote:
> On 5/2/25 7:21 PM, Taylor Blau wrote:
>> On Mon, Mar 24, 2025 at 03:22:38PM +0000, Derrick Stolee via GitGitGadget wrote:

>>> +static int add_objects_by_path(const char *path,
>>> +                   struct oid_array *oids,
>>> +                   enum object_type type,
>>> +                   void *data)
>>> +{
>>> +    struct object_entry **delta_list;
>>> +    size_t oe_start = to_pack.nr_objects;
>>> +    size_t oe_end;
>>> +    unsigned int sub_list_size;
>>
>> Could you help me understand why sub_list_size is an unsigned int and
>> not a size_t here?
> 
> This is based on a similar "list_size" used in ll_find_deltas(), but
> since we are starting from scratch here it would be better to use
> size_t.

Sorry for the delay, but the find_deltas() method called in both
places expects unsigned ints, so we are stuck with this until we
decide to upgrade it in both places.


>>> +    unsigned int *processed = data;
>>> +
>>> +    /*
>>> +     * First, add all objects to the packing data, including the ones
>>> +     * marked UNINTERESTING (translated to 'exclude') as they can be
>>> +     * used as delta bases.
>>> +     */
>>> +    for (size_t i = 0; i < oids->nr; i++) {
>>> +        int exclude;
>>> +        struct object_info oi = OBJECT_INFO_INIT;
>>> +        struct object_id *oid = &oids->oid[i];
>>> +
>>> +        /* Skip objects that do not exist locally. */
>>> +        if (exclude_promisor_objects &&
>>> +            oid_object_info_extended(the_repository, oid, &oi,
>>> +                         OBJECT_INFO_FOR_PREFETCH) < 0)
>>> +            continue;
>>> +
>>> +        exclude = !is_oid_interesting(the_repository, oid);
>>> +
>>> +        if (exclude && !thin)
>>> +            continue;
>>
>> ...here we only skip calling add_object_entry() if the object is
>> excluded (which lookup_object() returning NULL would cause above) *and*
>> we're not using thin packs. That makes sense, since if we have a missing
>> object, but we're using thin packs, then it's OK to skip.
>>
>> So I think all of this looks good. However, I find the interface here a
>> little awkward. The function is called "is_oid_interesting()", but it
>> determines that fact by checking whether or not the object has the
>> UNINTERSTING bit set on its flag, and then negating the result. But then
>> we negate it again here at the caller to obtain whether or not we should
>> "exclude" the object.
>>
>> I was wondering if there were other callers of is_oid_interesting() that
>> don't negate its result. But it doesn't look like there are (at least in
>> this patch). I wonder if it would be cleaner just to inline this instead
>> of having the result negated at multiple levels.

Double-checked now and there seem to be no other callers, so this will be
renamed to the opposite case.

>>> +    return 0;
>>> +}
>>> +
>>> +static void get_object_list_path_walk(struct rev_info *revs)
>>> +{
>>> +    struct path_walk_info info = PATH_WALK_INFO_INIT;
>>> +    unsigned int processed = 0;
>>> +
>>> +    info.revs = revs;
>>> +    info.path_fn = add_objects_by_path;
>>> +    info.path_fn_data = &processed;
>>> +    revs->tag_objects = 1;
>>
>> What is the purpose of setting revs->tag_objects to 1 here? Do we need
>> to restore it back to its original value before returning? A comment
>> here would be useful, I think.

I think this is not necessary and should be removed.

Thanks,
-Stolee