On Fri, Oct 25, 2024 at 5:52 AM karthik nayak <karthik.188@xxxxxxxxx> wrote:
>
> Eric Ju <eric.peijian@xxxxxxxxx> writes:
>
> [snip]
>
> > +
> > +void send_object_info_request(int fd_out, struct object_info_args *args)
> > +{
> > + struct strbuf req_buf = STRBUF_INIT;
> > +
> > + write_command_and_capabilities(&req_buf, "object-info", args->server_options);
> > +
> > + if (unsorted_string_list_has_string(args->object_info_options, "size"))
> > + packet_buf_write(&req_buf, "size");
> > +
> > + if (args->oids) {
> > + for (size_t i = 0; i < args->oids->nr; i++)
> > + packet_buf_write(&req_buf, "oid %s", oid_to_hex(&args->oids->oid[i]));
> > + }
> > +
> > + packet_buf_flush(&req_buf);
> > + if (write_in_full(fd_out, req_buf.buf, req_buf.len) < 0)
> > + die_errno(_("unable to write request to remote"));
> > +
> > + strbuf_release(&req_buf);
> > +}
> > +
>
> Was this function meant to be added here? I mean, there is no reference
> to it in the commit message or anywhere else.
>
Thank you.
The `send_object_info_request` function is used in `transport.c`
`fetch_object_info()` in patch 4/6. Its functionality is similar to
`send_fetch_request()`: sending the object-info command along with
sub-command (e.g. size) and arguments (e.g. oids) to the remote.
I guess Clavin put it here because
1. it has similar functionality as `send_fetch_request()`
2. `write_command_and_capabilities()` is only visible within `fecth-pack.c`.
However, I believe your comment is valid. Adding everything to
`fetch-pack.c` makes the file overly bloated with functionality
unrelated to fetch-pack. For v5, I plan to address this by:
I will:
1. move `write_command_and_capabilities()` a level up to connect.c
2. add a new file f`ecth-object-info.c` at the same level of
`fetch-pack.c`. This new file contains the logic related to
object-info command, i.e. `send_object_info_request()` and
`fetch_object_info()`
3. move `fetch_object_info()` away from `transport.c`
The dependency WAS like this, as I pointed out in a previous reply at
https://lore.kernel.org/git/CAN2LT1AM5rYpwjZ+rhYerxDkL6mbxr7iDc=wvuhvNKS8VVXQ8w@xxxxxxxxxxxxxx/#t
`transport.c` -> `fetch-pack.c` -> `connect.c`, where "->" means
"depends on".
In v5, it would be like this:
`transport.c` -> `fetch-pack.c` -> `connect.c`
| /
-> fecth-object-info.c ->
Let me know if that makes sense.
> [snip]
