Re: [PATCH v3 1/3] fetch-pack: refactor packet writing and fetch options

[Junio C Hamano <gitster@pobox.com>]

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");