Re: [RFC/WIP PATCH 04/11] upload-pack-2: Implement the version 2 of upload-pack

[Eric Sunshine <sunshine@sunshineco.com>]

On Tue, May 26, 2015 at 6:01 PM, Stefan Beller <sbeller@google.com> wrote:
> 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).

> +               return 0;
> +       }
> +
> +       while (advertise_capabilities[len] != '\0' &&
> +              advertise_capabilities[len] != ' ')
> +               len ++;

Style: len++

> +       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.

> +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().

> +       }
> +}
> +
> +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.
     */

> +       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'.

> +       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