| Message ID | 20220328191112.3092139-2-calvinwan@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | object-info: add option for retrieving object info | expand |
Calvin Wan <calvinwan@google.com> writes: > -static void write_fetch_command_and_capabilities(struct strbuf *req_buf, > - const struct string_list *server_options) > +static void write_command_and_capabilities(struct strbuf *req_buf, > + const struct string_list *server_options, const char* command) Reindent the second line to take advantage of the fact that the function name is now slightly shorter? It is named "command" and "capabilities", but the parameters it takes has the "command" at the end. We probably want to swap them, i.e. static void write_command_and_capabilities(struct strbuf *req_buf, const char *command, const struct string_list *server_options) Note that asterisk sticks to identifier, not to type, in this code base. > { > const char *hash_name; > > - if (server_supports_v2("fetch", 1)) > - packet_buf_write(req_buf, "command=fetch"); > + if (server_supports_v2(command, 1)) > + packet_buf_write(req_buf, "command=%s", command); > if (server_supports_v2("agent", 0)) > packet_buf_write(req_buf, "agent=%s", git_user_agent_sanitized()); > if (advertise_sid && server_supports_v2("session-id", 0)) > @@ -1279,7 +1279,7 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out, > int done_sent = 0; > struct strbuf req_buf = STRBUF_INIT; > > - write_fetch_command_and_capabilities(&req_buf, args->server_options); > + write_command_and_capabilities(&req_buf, args->server_options, "fetch"); > > if (args->use_thin_pack) > packet_buf_write(&req_buf, "thin-pack"); The above two hunks (and there is another one at the end) are trivial no-op refactoring, and the first paragraph of the proposed log message clearly explains it. > @@ -1598,18 +1598,18 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args, > reader.me = "fetch-pack"; > } > > + /* v2 supports these by default */ > + allow_unadvertised_object_request |= ALLOW_REACHABLE_SHA1; > + use_sideband = 2; > + if (args->depth > 0 || args->deepen_since || args->deepen_not) > + args->deepen = 1; > + > while (state != FETCH_DONE) { > switch (state) { > case FETCH_CHECK_LOCAL: > sort_ref_list(&ref, ref_compare_name); > QSORT(sought, nr_sought, cmp_ref_by_name); > > - /* v2 supports these by default */ > - allow_unadvertised_object_request |= ALLOW_REACHABLE_SHA1; > - use_sideband = 2; > - if (args->depth > 0 || args->deepen_since || args->deepen_not) > - args->deepen = 1; > - > /* Filter 'ref' by 'sought' and those that aren't local */ > mark_complete_and_common_ref(negotiator, args, &ref); > filter_refs(args, &ref, sought, nr_sought); The second paragraph of the proposed log message touched briefly about the existence of this change. It is a no-op for the current code if two conditions are met: (1) the initial state is CHECK_LOCAL, (2) we only enter CHECK_LOCAL once and never return to it. And they both hold true, so this is a no-op change. But I am not sure if it is a good idea to have this in this patch. Possibilities are (1) this patch as-is is OK, (2) move it in a separate patch as it has nothing to do with the other refactoring, or (3) do it as part of the change that wants this change. I may form an opinion once I see the step that wants this change, but not yet. > @@ -2060,7 +2060,7 @@ void negotiate_using_fetch(const struct oid_array *negotiation_tips, > int received_ready = 0; > > strbuf_reset(&req_buf); > - write_fetch_command_and_capabilities(&req_buf, server_options); > + write_command_and_capabilities(&req_buf, server_options, "fetch"); > > packet_buf_write(&req_buf, "wait-for-done");
On Mon, Mar 28, 2022 at 07:11:10PM +0000, Calvin Wan wrote: > A subsequent commit will need to write capabilities for another > command, so refactor write_fetch_command_and_capabilities() to be able > to used by both fetch and future command. 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. > Move fetch options set in `FETCH_CHECK_LOCAL` from the fetch state > machine to above the state machine so it is set by default. The > initial state of the state machine is always `FETCH_CHECK_LOCAL` so > this does not affect any current functionality. This change prepares > for a subsequent commit that doesn't need to check the local state, but > still requires those options to be set before sending the fetch request. This took a little more thinking, but I agree that this is behaviorally equivalent. The key points are (and here I'm basically restating what you already wrote): - when the state machine in do_fetch_pack_v2() is in FETCH_CHECK_LOCAL, we set a few v2-specific defaults - it will be helpful for a future patch to have these defaults set _before_ we enter the main loop - and we can safely make that change, since the initial state is always FETCH_CHECK_LOCAL anyway, so we're guaranteed to hit that code whether it's in the loop or not. So this seems safe to me, too. I'll have to see why this is necessary, but I suspect we'll find out in subsequent patches, so let's read on... Thanks, Taylor
Calvin Wan <calvinwan@google.com> writes: > A subsequent commit will need to write capabilities for another > command, so refactor write_fetch_command_and_capabilities() to be able > to used by both fetch and future command. > > Move fetch options set in `FETCH_CHECK_LOCAL` from the fetch state > machine to above the state machine so it is set by default. The > initial state of the state machine is always `FETCH_CHECK_LOCAL` so > this does not affect any current functionality. This change prepares > for a subsequent commit that doesn't need to check the local state, but > still requires those options to be set before sending the fetch request. I think it would be clearer to rephrase "that doesn't need to check the local state" to "that will skip FETCH_CHECK_LOCAL". It does literally mean the same thing, but I had a mental block with "initial state...is always FETCH_CHECK_LOCAL" in the preceding sentence. I agree with some of the others who said that this change should go into its own patch. Otherwise, this patch makes sense.
diff --git a/fetch-pack.c b/fetch-pack.c index 87657907e7..b709a61baf 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -1237,13 +1237,13 @@ static int add_haves(struct fetch_negotiator *negotiator, return haves_added; } -static void write_fetch_command_and_capabilities(struct strbuf *req_buf, - const struct string_list *server_options) +static void write_command_and_capabilities(struct strbuf *req_buf, + const struct string_list *server_options, const char* command) { const char *hash_name; - if (server_supports_v2("fetch", 1)) - packet_buf_write(req_buf, "command=fetch"); + if (server_supports_v2(command, 1)) + packet_buf_write(req_buf, "command=%s", command); if (server_supports_v2("agent", 0)) packet_buf_write(req_buf, "agent=%s", git_user_agent_sanitized()); if (advertise_sid && server_supports_v2("session-id", 0)) @@ -1279,7 +1279,7 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out, int done_sent = 0; struct strbuf req_buf = STRBUF_INIT; - write_fetch_command_and_capabilities(&req_buf, args->server_options); + write_command_and_capabilities(&req_buf, args->server_options, "fetch"); if (args->use_thin_pack) packet_buf_write(&req_buf, "thin-pack"); @@ -1598,18 +1598,18 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args, reader.me = "fetch-pack"; } + /* v2 supports these by default */ + allow_unadvertised_object_request |= ALLOW_REACHABLE_SHA1; + use_sideband = 2; + if (args->depth > 0 || args->deepen_since || args->deepen_not) + args->deepen = 1; + while (state != FETCH_DONE) { switch (state) { case FETCH_CHECK_LOCAL: sort_ref_list(&ref, ref_compare_name); QSORT(sought, nr_sought, cmp_ref_by_name); - /* v2 supports these by default */ - allow_unadvertised_object_request |= ALLOW_REACHABLE_SHA1; - use_sideband = 2; - if (args->depth > 0 || args->deepen_since || args->deepen_not) - args->deepen = 1; - /* Filter 'ref' by 'sought' and those that aren't local */ mark_complete_and_common_ref(negotiator, args, &ref); filter_refs(args, &ref, sought, nr_sought); @@ -2060,7 +2060,7 @@ void negotiate_using_fetch(const struct oid_array *negotiation_tips, int received_ready = 0; strbuf_reset(&req_buf); - write_fetch_command_and_capabilities(&req_buf, server_options); + write_command_and_capabilities(&req_buf, server_options, "fetch"); packet_buf_write(&req_buf, "wait-for-done");