Re: [PATCH 1/6] fetch-pack: refactor packet writing
- From
Karthik Nayak <karthik.188@gmail.com>
- Date
- Jul 10, 2024, 09:39 UTC
- Message-ID
- <CAOLa=ZRi2w_CXovpg3h1mfMsOiLWrpS8vL+NzS5o_NwhFdysTg@mail.gmail.com>
- In-Reply-To
- <CAN2LT1Ctwdij9-DujKeuzPX71mzEoMVbdrTkJ8bpRnX9NAZy9Q@mail.gmail.com>
Peijian Ju <eric.peijian@gmail.com> writes: [snip]
Show 27 quoted lines
>> 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?