On Wed, Jul 10, 2024 at 5:39 AM Karthik Nayak <karthik.188@xxxxxxxxx> wrote: > > Peijian Ju <eric.peijian@xxxxxxxxx> writes: > [snip] > > >> 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/ > > > > " 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.
