Re: [RFC/WIP PATCH 04/11] upload-pack-2: Implement the version 2 of upload-pack
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- May 27, 2015, 02:30 UTC
- Message-ID
- <CAPig+cQHQFrabqkXUaRHw7HU+URL4QBzzLZYko5GrRHFpYctng@mail.gmail.com>
- In-Reply-To
- <1432677675-5118-5-git-send-email-sbeller@google.com>
On Tue, May 26, 2015 at 6:01 PM, Stefan Beller <sbeller@google.com> wrote:
Show 30 quoted lines
> In upload-pack-2 we send each capability in its own packet.
> By reusing the advertise_capabilities and eventually setting it to
> NULL we will be able to reuse the methods for refs advertisement.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> diff --git a/upload-pack-2.c b/upload-pack-2.c
> new file mode 120000
> index 0000000..e30a871
> --- /dev/null
> +++ b/upload-pack-2.c
> @@ -716,10 +716,47 @@ static void format_symref_info(struct strbuf *buf, struct string_list *symref)
> strbuf_addf(buf, " symref=%s:%s", item->string, (char *)item->util);
> }
>
> -static const char *advertise_capabilities = "multi_ack thin-pack side-band"
> +static char *advertise_capabilities = "multi_ack thin-pack side-band"
> " side-band-64k ofs-delta shallow no-progress"
> " include-tag multi_ack_detailed";
>
> +/*
> + * Reads the next capability and puts it into dst as a null terminated string.
> + * Returns true if more capabilities can be read.
> + * */
> +static int next_capability(char *dst)
> +{
> + int len = 0;
> + if (!*advertise_capabilities) {
> + /* make sure to not advertise capabilities afterwards */
> + advertise_capabilities = NULL;You set advertise_capabilities to NULL here but then unconditionally dereference that NULL in the if-statement just above (if someone calls next_capability() again). Seems fishy (and crashy) to me. Either don't dereference the NULL or don't bother setting it to NULL (in which case, you'll dereference and find '\0', which should serve the same purpose).
Show 6 quoted lines
> + return 0; > + } > + > + while (advertise_capabilities[len] != '\0' && > + advertise_capabilities[len] != ' ') > + len ++;
Style: len++
Show 6 quoted lines
> + strncpy(dst, advertise_capabilities, len); > + dst[len] = '\0'; > + > + advertise_capabilities += len; > + if (*advertise_capabilities == ' ') > + advertise_capabilities++;
If for some reason, someone happens to add an extra space between capabilities in advertise_capabilities, then the capability returned by the next invocation of next_capability() be zero-length, which is probably undesirable, and certainly not expected by the caller. Skipping whitespace in a loop would be more robust.
> + return 1; > +}
I realize that this is RFC, but my overall reaction to next_capability() in its current form is that it's ugly . A somewhat cleaner approach would be to pass some state into next_capability() and have it modify that state rather than the global advertise_capabilities. The passed-in state could be as simple as a 'const char *' which initially points at the start of advertise_capabilities and then gets advanced; until, finally, it points at the '\0' at the end of advertise_capabilities.
Show 23 quoted lines
> +static void send_capabilities(void)
> +{
> + char buf[100];
> +
> + while (next_capability(buf))
> + packet_write(1, "capability:%s\n", buf);
> +
> + packet_write(1, "agent:%s\n", git_user_agent_sanitized());
> + packet_flush(1);
> +}
> +
> static int send_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)
> {
>
> @@ -794,6 +831,28 @@ static void upload_pack(void)
> }
> }
>
> +static void receive_capabilities(void)
> +{
> + int done = 0;
> + while (1) {
> + char *line = packet_read_line(0, NULL);If you declare this 'const char *', then it becomes much more obvious that it's not your responsibility to free() the result (and, tangentially, that you have no intention of modifying the content).
> + if (!line)
> + break;
> + if (starts_with(line, "capability:"))
> + parse_features(line + strlen("capability:"));See skip_prefix().
Show 10 quoted lines
> + }
> +}
> +
> +static void upload_pack_version_2(void)
> +{
> + send_capabilities();
> + receive_capabilities();
> +
> + /* The rest of the protocol stays the same, capabilities advertising
> + is disabled though. */ /*
* This is a multi-line
* comment.
*/Show 14 quoted lines
> + upload_pack();
> +}
> +
> static int upload_pack_config(const char *var, const char *value, void *unused)
> {
> if (!strcmp("uploadpack.allowtipsha1inwant", var))
> @@ -806,16 +865,24 @@ static int upload_pack_config(const char *var, const char *value, void *unused)
> return parse_hide_refs_config(var, value, "uploadpack");
> }
>
> +static int endswith(const char *teststring, const char *ending)
> +{
> + int slen = strlen(teststring);
> + int elen = strlen(ending);You may be confident today that no callers are supplying an 'ending' which is longer than 'teststring', but someone may some day break that assumption. At the very least, for robustness, add an assert() or die() if 'elen' is greater than 'slen'.
Show 32 quoted lines
> + return !strcmp(teststring + slen - elen, ending);
> +}
> +
> int main(int argc, char **argv)
> {
> char *dir;
> + const char *cmd;
> int i;
> int strict = 0;
>
> git_setup_gettext();
>
> packet_trace_identity("upload-pack");
> - git_extract_argv0_path(argv[0]);
> + cmd = git_extract_argv0_path(argv[0]);
> check_replace_refs = 0;
>
> for (i = 1; i < argc; i++) {
> @@ -857,6 +924,10 @@ int main(int argc, char **argv)
> die("'%s' does not appear to be a git repository", dir);
>
> git_config(upload_pack_config, NULL);
> - upload_pack();
> +
> + if (endswith(cmd, "-2"))
> + upload_pack_version_2();
> + else
> + upload_pack();
> return 0;
> }
> --
> 2.4.1.345.gab207b6.dirty