Re: [PATCH v2 3/3] archive: support remote archive from stateless transport

From: Jiang Xin <worldhello.net@gmail.com>
Date: Sep 24, 2023, 23:36 UTC
Message-ID: <CANYiYbHjk4CX4Uswn4sX-tH3e22uLSHk_4bwjqVO=9MWcfoHnw@mail.gmail.com>
In-Reply-To: <f4877c36-ff26-4f81-b5dd-63c929ba30c9@gmail.com>

On Sun, Sep 24, 2023 at 9:41 PM Phillip Wood <phillip.wood123@gmail.com> wrote:

Show 27 quoted lines

>
> On 23/09/2023 16:22, Jiang Xin wrote:
> > From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> >
> > Even though we can establish a stateless connection, we still cannot
> > archive the remote repository using a stateless HTTP protocol. Try the
> > following steps to make it work.
> >
> >   1. Add support for "git-upload-archive" service in "http-backend".
> >
> >   2. Use the URL ".../info/refs?service=git-upload-pack" to detect the
> >      protocol version, instead of use the "git-upload-archive" service.
> >
> >   3. "git-archive" does not expect to see protocol version and
> >      capabilities when connecting to remote-helper, so do not send them
> >      in "remote-curl.c" for the "git-upload-archive" service.
>
> I'm not familiar enough with the server side of git to comment on
> whether this patch is a good idea, but I did notice one C language issue
> below.
>
> >   static struct string_list *get_parameters(void)
> > @@ -639,10 +640,19 @@ static void check_content_type(struct strbuf *hdr, const char *accepted_type)
> >
> >   static void service_rpc(struct strbuf *hdr, char *service_name)
> >   {
> > -     const char *argv[] = {NULL, "--stateless-rpc", ".", NULL};

For the original implementation, the first NULL is used as a placeholder, and will be initialized somewhere below.

Show 18 quoted lines

> In the pre-image argv[0] is initialized to NULL
>
> > +     const char *argv[4];
>
> In the post-image argv is not initialized and the first element is not
> set in the code below.
>
> >       struct rpc_service *svc = select_service(hdr, service_name);
> >       struct strbuf buf = STRBUF_INIT;
> >
> > +     if (!strcmp(service_name, "git-upload-archive")) {
> > +             argv[1] = ".";
> > +             argv[2] = NULL;
> > +     } else {
> > +             argv[1] = "--stateless-rpc";
> > +             argv[2] = ".";
> > +             argv[3] = NULL;
> > +     }

It will be initialized in the code further below, see http-backend.c:668.

        argv[0] = svc->name;
        run_service(argv, svc->buffer_input);
        strbuf_release(&buf);

Anyway, I will rewrite these code in reroll v3 to follow Eric's suggestion.

> Best Wishes
>
> Phillip
>