Re: [WIP RFC 5/5] upload-pack: send part of packfile response as uri

From: Stefan Beller <sbeller@google.com>
Date: Dec 4, 2018, 20:09 UTC
Message-ID: <CAGZ79kYhemdUjxQcOZ6ryofh6XddfFZB6zG29grQwstC=VeV-g@mail.gmail.com>
In-Reply-To: <707a8181a0e8cd2e21989fa6da59cee38d9fadde.1543879256.git.jonathantanmy@google.com>

On Mon, Dec 3, 2018 at 3:38 PM Jonathan Tan <jonathantanmy@google.com> wrote:

>
> This is a partial implementation of upload-pack sending part of its
> packfile response as URIs.

It does so by implementing a new flag `--exclude-configured-blobs` in pack-objects, which would change the output of pack-objects to output a list of URLs (of the excluded blobs) followed by the pack to be asked for.

This design seems easy to implement as then upload-pack can just parse the output and only needs to insert "packfile" and "packfile-uris\n" at the appropriate places of the stream, otherwise it just passes through the information obtained from pack-objects.

The design as-is would make for hard documentation of pack-objects (its output is not just a pack anymore when that flag is given, but a highly optimized byte stream).

Initially I did not anticipate this to be one of the major design problems as I assumed we'd want to use this pack feature over broadly (e.g. eventually by offloading most of the objects into a base pack that is just always included as the likelihood for any object in there is very high on initial clone), but it makes total sense to only output the URIs that we actually need.

An alternative that comes very close to the current situation would be to either pass a file path or file descriptor (that upload-pack listens to in parallel) to pack-objects as an argument of the new flag. Then we would not need to splice the protocol sections into the single output stream, but we could announce the sections, then flush the URIs and then flush the pack afterwards.

I looked at this quickly, but that would need either extensions in run-command.c for setting up the new fd for us, as there we already have OS specific code for these setups, or we'd have to duplicate some of the logic here, which doesn't enthuse me either.

So maybe we'd create a temp file via mkstemp and pass the file name to pack-objects for writing the URIs and then we'd just need to stream that file?

Show 10 quoted lines

> +       # NEEDSWORK: "git clone" fails here because it ignores the URI provided
> +       # instead of fetching it.
> +       test_must_fail env GIT_TRACE_PACKET="$(pwd)/log" \
> +               git -c protocol.version=2 clone \
> +               "$HTTPD_URL/smart/http_parent" http_child 2>err &&
> +       # Although "git clone" fails, we can still check that the server
> +       # provided the URI we requested and that the error message pinpoints
> +       # the object that is missing.
> +       grep "clone< uri https://example.com/a-uri" log &&
> +       test_i18ngrep "did not receive expected object $(cat h)" err

That is a nice test!