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

On Wed, Jul 10, 2024 at 5:39 AM Karthik Nayak [off-list ref] wrote:

Peijian Ju [off-list ref] writes:
[snip]

quoted

quoted

Right, this commit in itself looks good. But I was curious why we need
this, so I did a sneak peak into the following commits.

To summarize, we want to call:
   `write_command_and_capabilities(..., "object-info");`
in the upcoming patches to get the object-info details from the server.
But isn't this function too specific to the "fetch" command to be
generalized to be for "object-info" too?

Wouldn't it make sense to add a custom function for 'object-info' in
'connect.c'? Like how we currently have `get_remote_bundle_uri()` for
'bundle-uri' and `get_remote_refs` for 'ls-refs'?

Thank you. I am reading through the old comments left by Taylor
at https://lore.kernel.org/git/YkOPyc9tUfe2Tozx@nand.local/ (local)

  " Makes obvious sense, and this was something that jumped out to me when I
  looked at the first and second versions of this patch. I'm glad that
  this is getting factored out."


It seems refactoring this into a more general function is on purpose.
It is encouraged to use this general function to request capability
rather than adding a custom function.
Taylor’s comment was 2 years ago, but I think refactoring this into a
more general function to
enforce DRY still makes sense.

It would make sense then to move the existing users to also use
`write_command_and_capabilities` eventually. I guess this could be done
in a follow up series.

Then I would say `write_command_and_capabilities()` should be moved to
`transport.c`, no?

Thank you. I am not sure about this. Currently, the file dependency is
like this:
`transport.c`  -> `fetch-pack.c` -> `connect.c`  where "->" means "depends on".

Moving `write_command_and_capabilities()` to `transport.c` would make
circle dependency.

If we want `write_command_and_capabilities()` to be a more general
utility function,
it seems make more sense to move it to `connect.c`. I saw a bunch of
these general utility functions
in `connect.c` such as `send_capabilities()`. Some custom functions
such as `get_remote_bundle_uri()` and `get_remote_refs`also lives in
it.

Please let me know what you think. Thanks.