Re: [RFC PATCH] upload-pack: expand capability advertises additional refs
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:47:20
"Shawn O. Pearce" [off-list ref] writes:
quoted hunk
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...