Re: [PATCH GSoC v15 05/13] fetch-pack: drop static `advertise_sid` variable

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

Pablo Sabater <pabloosabaterr@gmail.com> writes:

> `write_fetch_command_and_capabilities()` is moved to `connect.c` in a
> subsequent commit. To prepare for that, drop the static variable usage
> of `advertise_sid`. Currently `advertise_sid` is used in two places:
>
> 1. In function `do_fetch_pack()`:
>         if (!server_supports("session_id"))
>                advertise_sid = 0;
> 2. In function `fetch_pack_config()`:
>         repo_config_get_bool("transfer.advertisesid", &advertise_sid);
>
> Since `do_fetch_pack()` is only relevant for protocol v1, it can be
> ignored because `write_fetch_command_and_capabilities()` is only used in
> protocol v2.
>
> About 2, call `repo_config_get_bool()` directly inside of the function.

Puzzled.  The patch does introduce a local on-stack variable with
the same name, but does not remove the file-scope global one.

> diff --git a/fetch-pack.c b/fetch-pack.c
> index f13951d154..ad07603755 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -1380,6 +1380,9 @@ static void write_fetch_command_and_capabilities(struct strbuf *req_buf,
>  						 const struct string_list *server_options)
>  {
>  	const char *hash_name;
> +	int advertise_sid;
> +
> +	repo_config_get_bool(the_repository, "transfer.advertisesid", &advertise_sid);

If there is no such configuration variable defined anywhere in the
system, advertise_sid will be left uninitialied.  Wouldn't it cause
problems later.

Initialize it to 0 (if the default is not to advertise, which I
think is the case but please double check), and you would be OK,
probably?

>  	ensure_server_supports_v2("fetch");
>  	packet_buf_write(req_buf, "command=fetch");
> @@ -1395,7 +1398,7 @@ static void write_fetch_command_and_capabilities(struct strbuf *req_buf,
>  	}
>  
>  	if (server_feature_v2("object-format", &hash_name)) {
> -		int hash_algo = hash_algo_by_name(hash_name);
> +		const unsigned int hash_algo = hash_algo_by_name(hash_name);
>  		if (hash_algo_by_ptr(the_hash_algo) != hash_algo)
>  			die(_("mismatched algorithms: client %s; server %s"),
>  			    the_hash_algo->name, hash_name);

> While at it, change `hash_algo`'s type to match `hash_algo_by_name()`'s
> actual return type (`unsigned int`) and make it `const`.

This one makes sense, but is better left out to a separate step.
Especially when the primary focus of the change is iffy (see above).