Re: [PATCH v4 1/6] fetch-pack: refactor packet writing

From: Peijian Ju <eric.peijian@gmail.com>
Date: Oct 25, 2024, 16:06 UTC
Message-ID: <CAN2LT1CEPdTAxCEpKtd+8-5zKYSnh0PMqEXgAZ++TTMPPKrD1g@mail.gmail.com>
In-Reply-To: <CAOLa=ZSb6p_z8jJHSWCfqD6bdTNNXQw-Dq6DsQWt+o+cyQ3x9Q@mail.gmail.com>

On Fri, Oct 25, 2024 at 5:52 AM karthik nayak <karthik.188@gmail.com> wrote:

Show 31 quoted lines

>
> Eric Ju <eric.peijian@gmail.com> 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@mail.gmail.com/#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]