Re: [RFC PATCH] upload-pack: expand capability advertises additional refs

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 28, 2009, 19:07 UTC
Message-ID: <7vr5uvlpue.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20090828173007.GX1033@spearce.org>

"Shawn O. Pearce" <spearce@spearce.org> writes:

Show 36 quoted lines

>  static void upload_pack(void)
>  {
> +	git_config(upload_pack_config, NULL);
> +	if (!configured_advertise)
> +		push_advertise("refs/*");
> +
> +	head_ref(scan_ref, NULL);
> +	for_each_ref(scan_ref, NULL);
> +
>  	reset_timeout();
> -	head_ref(send_ref, NULL);
> -	for_each_ref(send_ref, NULL);
> -	packet_flush(1);
> +	send_refs();
>  	receive_needs();
>  	if (want_obj.nr) {
>  		get_common_commits();
> @@ -652,6 +764,7 @@ int main(int argc, char **argv)
>  
>  	git_extract_argv0_path(argv[0]);
>  	read_replace_refs = 0;
> +	push_advertise("HEAD");
>  
>  	for (i = 1; i < argc; i++) {
>  		char *arg = argv[i];
> @@ -667,6 +780,10 @@ int main(int argc, char **argv)
>  			daemon_mode = 1;
>  			continue;
>  		}
> +		if (!prefixcmp(arg, "--expand=")) {
> +			push_advertise(arg + 9);
> +			continue;
> +		}
>  		if (!strcmp(arg, "--")) {
>  			i++;
>  			break;

This arrangement of push_advertise() calls are rather curious. I think your design guidelines are:

 - We do want to advertise HEAD, so it can (and should) be unconditional;

 - We may want to restrict with configuration, or use "refs/*" as the
   fallback default;

 - We may want to (extend|replace) advertised set configured in the
   configuration file.

I would naively expect the above to be implemented in this order:

 - In main, first thing is to do the git_config() bit; if to_advertise is
   non-empty after git_config() returns, we have seen upload.advertise.
   Otherwise we haven't.  Push "refs/*" in the latter case.

 - Then, parse the command line.  Do we see "--expand="?  If so:

   . When handling the first "--expand=", clear to_advertise list.  This
     step is optional---necessary only if we want to make config variable
     override-able ;-);

   . Then, push the pattern in.

 - And finally push "HEAD" in unconditionally.

Which would mean a few things:

 - Your version always extends what is read from the configuration with
   --expand=, but I think replacing would be the right thing to do.

 - configured_advertise global variable can go.

 - Instead, to implement "--expand= replaces", we need a local variable in
   main() to remember if we already cleared to_advertise of the patterns
   read from the configuration file, and use it in the part that parses
   "--expand=".

But I may not be thinking clearly; I was up all night because I couldn't sleep under loud noises of fire-fighting helicopters...