Re: [RFC 7/7] upload-pack: ack version 2

[Brandon Williams]

On 09/01, Bryan Turner wrote:
> On Thu, Aug 24, 2017 at 3:53 PM, Brandon Williams  wrote:
> > +
> > +   version = getenv("GIT_PROTOCOL");
> > +   if (!strcmp(version, "2"))
> > +   upload_pack_v2();
> > +
> 
> I think the "if" check here needs some type of NULL check for
> "version" before calling "strcmp". Without that, if the "GIT_PROTOCOL"
> environment variable isn't set, git-upload-pack SEGVs.
> 
> This came up when I was testing the "protocol-v2" branch with
> Bitbucket Server. For performance reasons, we sometimes run ref
> advertisements as a separate process, when serving fetches, to allow
> throttling the ref advertisement separately from actually generating
> and sending a packfile. Since CI servers continuously poll for
> changes, but usually don't find any, we want to be able to serve ref
> advertisements, which have minimal overhead, with much higher
> parallelism than serving packs. To do that, we run "git-upload-pack
> --advertize-refs" directly, and that command has never *required*
> "GIT_PROTOCOL" before this change.

Thanks for pointing this out.  Since this was an RFC I wasn't being
careful with doing these sorts of checks :).  I'm currently working on
the non-RFC version of this series and it is getting close to being in a
state where I can send it out for more careful review.

-- 
Brandon Williams

Re: [RFC 7/7] upload-pack: ack version 2

[Bryan Turner]

On Thu, Aug 24, 2017 at 3:53 PM, Brandon Williams  wrote:
> +
> +   version = getenv("GIT_PROTOCOL");
> +   if (!strcmp(version, "2"))
> +   upload_pack_v2();
> +

I think the "if" check here needs some type of NULL check for
"version" before calling "strcmp". Without that, if the "GIT_PROTOCOL"
environment variable isn't set, git-upload-pack SEGVs.

This came up when I was testing the "protocol-v2" branch with
Bitbucket Server. For performance reasons, we sometimes run ref
advertisements as a separate process, when serving fetches, to allow
throttling the ref advertisement separately from actually generating
and sending a packfile. Since CI servers continuously poll for
changes, but usually don't find any, we want to be able to serve ref
advertisements, which have minimal overhead, with much higher
parallelism than serving packs. To do that, we run "git-upload-pack
--advertize-refs" directly, and that command has never *required*
"GIT_PROTOCOL" before this change.

[RFC 7/7] upload-pack: ack version 2

[Brandon Williams]

Signed-off-by: Brandon Williams 
---
 upload-pack.c | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/upload-pack.c b/upload-pack.c
index 7efff2fbf..0f853152f 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -1032,9 +1032,15 @@ static int upload_pack_config(const char *var, const 
char *value, void *unused)
return parse_hide_refs_config(var, value, "uploadpack");
 }
 
+void upload_pack_v2(void)
+{
+   packet_write_fmt(1, "%s\n", "version 2");
+}
+
 int cmd_main(int argc, const char **argv)
 {
const char *dir;
+   const char *version;
int strict = 0;
struct option options[] = {
OPT_BOOL(0, "stateless-rpc", _rpc,
@@ -1067,6 +1073,11 @@ int cmd_main(int argc, const char **argv)
die("'%s' does not appear to be a git repository", dir);
 
git_config(upload_pack_config, NULL);
+
+   version = getenv("GIT_PROTOCOL");
+   if (!strcmp(version, "2"))
+   upload_pack_v2();
+
upload_pack();
return 0;
 }
-- 
2.14.1.342.g6490525c54-goog