Re: [PATCH v3 05/10] upload-pack, receive-pack: introduce protocol version 1

[Brandon Williams]

On 10/10, Jonathan Tan wrote:
> On Tue,  3 Oct 2017 13:15:02 -0700
> Brandon Williams  wrote:
> 
> > +   switch (determine_protocol_version_server()) {
> > +   case protocol_v1:
> > +   if (advertise_refs || !stateless_rpc)
> > +   packet_write_fmt(1, "version 1\n");
> > +   /*
> > +* v1 is just the original protocol with a version string,
> > +* so just fall through after writing the version string.
> > +*/
> 
> Peff sent out at least one patch [1] that reformats fallthrough comments
> to be understood by GCC. It's probably worth doing here too.
> 
> In this case, I would do the 2-comment system that Peff suggested:
> 
>   /*
>* v1 is just the original protocol with a version string,
>* so just fall through after writing the version string.
>*/
>   if (advertise_refs || !stateless_rpc)
>   packet_write_fmt(1, "version 1\n");
>   /* fallthrough */
> 
> (I put the first comment before the code, so it doesn't look so weird.)

Sounds good.

> 
> [1] 
> https://public-inbox.org/git/20170921062541.ew67gyvrmb2ot...@sigill.intra.peff.net/
> 
> > +   case protocol_v0:
> > +   break;
> > +   default:

I'm also going to change this to from default to
'protocol_unknown_version' that way we get a compiler error instead of a
run-time BUG when introducing a new protocol version number.

> > +   BUG("unknown protocol version");
> > +   }

-- 
Brandon Williams

Re: [PATCH v3 05/10] upload-pack, receive-pack: introduce protocol version 1

[Jonathan Tan]

On Tue,  3 Oct 2017 13:15:02 -0700
Brandon Williams  wrote:

> + switch (determine_protocol_version_server()) {
> + case protocol_v1:
> + if (advertise_refs || !stateless_rpc)
> + packet_write_fmt(1, "version 1\n");
> + /*
> +  * v1 is just the original protocol with a version string,
> +  * so just fall through after writing the version string.
> +  */

Peff sent out at least one patch [1] that reformats fallthrough comments
to be understood by GCC. It's probably worth doing here too.

In this case, I would do the 2-comment system that Peff suggested:

/*
 * v1 is just the original protocol with a version string,
 * so just fall through after writing the version string.
 */
if (advertise_refs || !stateless_rpc)
packet_write_fmt(1, "version 1\n");
/* fallthrough */

(I put the first comment before the code, so it doesn't look so weird.)

[1] 
https://public-inbox.org/git/20170921062541.ew67gyvrmb2ot...@sigill.intra.peff.net/

> + case protocol_v0:
> + break;
> + default:
> + BUG("unknown protocol version");
> + }

[PATCH v3 05/10] upload-pack, receive-pack: introduce protocol version 1

[Brandon Williams]

Teach upload-pack and receive-pack to understand and respond using
protocol version 1, if requested.

Protocol version 1 is simply the original and current protocol (what I'm
calling version 0) with the addition of a single packet line, which
precedes the ref advertisement, indicating the protocol version being
spoken.

Signed-off-by: Brandon Williams 
---
 builtin/receive-pack.c | 15 +++
 upload-pack.c  | 18 +-
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index dd06b3fb4..94b7d29ea 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -24,6 +24,7 @@
 #include "tmp-objdir.h"
 #include "oidset.h"
 #include "packfile.h"
+#include "protocol.h"
 
 static const char * const receive_pack_usage[] = {
N_("git receive-pack "),
@@ -1963,6 +1964,20 @@ int cmd_receive_pack(int argc, const char **argv, const 
char *prefix)
else if (0 <= receive_unpack_limit)
unpack_limit = receive_unpack_limit;
 
+   switch (determine_protocol_version_server()) {
+   case protocol_v1:
+   if (advertise_refs || !stateless_rpc)
+   packet_write_fmt(1, "version 1\n");
+   /*
+* v1 is just the original protocol with a version string,
+* so just fall through after writing the version string.
+*/
+   case protocol_v0:
+   break;
+   default:
+   BUG("unknown protocol version");
+   }
+
if (advertise_refs || !stateless_rpc) {
write_head_info();
}
diff --git a/upload-pack.c b/upload-pack.c
index 7efff2fbf..ef438e9c2 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -18,6 +18,7 @@
 #include "parse-options.h"
 #include "argv-array.h"
 #include "prio-queue.h"
+#include "protocol.h"
 
 static const char * const upload_pack_usage[] = {
N_("git upload-pack [] "),
@@ -1067,6 +1068,21 @@ 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);
-   upload_pack();
+
+   switch (determine_protocol_version_server()) {
+   case protocol_v1:
+   if (advertise_refs || !stateless_rpc)
+   packet_write_fmt(1, "version 1\n");
+   /*
+* v1 is just the original protocol with a version string,
+* so just fall through after writing the version string.
+*/
+   case protocol_v0:
+   upload_pack();
+   break;
+   default:
+   BUG("unknown protocol version");
+   }
+
return 0;
 }
-- 
2.14.2.920.gcf0c67979c-goog