Re: [PATCH v5 5/6] transport: add client support for object-info

From: Calvin Wan <calvinwan@google.com>
Date: Aug 4, 2022, 20:28 UTC
Message-ID: <CAFySSZAJMUw30GCs8Etvi1D0hm3Rna2C8K_EMxKQtXea9fqVcg@mail.gmail.com>
In-Reply-To: <xmqqbkt77q07.fsf@gitster.g>

Show 10 quoted lines

> If !args->oids then we say "we want to request object-info to learn
> size and type for the following objects: oh, there are no objects we
> are interested in".  I wonder if an early return
>
>         if (!args->oids)
>                 return;
>
> at the beginning of the function that turns it into a benign no-op,
> may make more sense?  Calling "send_X()" helper and seeing nothing
> come out on the wire might make it look awkward, though.

If I add this change, then I should also add a check to ensure args->object_info_options is also not empty. The question I think we should answer is whether checking for oids and options should happen inside this function or not. I'm leaning towards outside the function for the reason you stated above about nothing coming out of the wire being awkward.

> > +     struct ref *object_info_refs = xcalloc(1, sizeof (struct ref));
>
> Style: no SP between "sizeof" and "(".

ack