Re: [PATCH] fetch-pack: do not take shallow lock unnecessarily

[Stefan Beller <sbeller@google.com>]

On Thu, Jan 10, 2019 at 11:36 AM Jonathan Tan <jonathantanmy@google.com> wrote:
>
> When fetching using protocol v2, the remote may send a "shallow-info"
> section if the client is shallow. If so, Git as the client currently
> takes the shallow file lock, even if the "shallow-info" section is
> empty.
>
> This is not a problem except that Git does not support taking the
> shallow file lock after modifying the shallow file, because
> is_repository_shallow() stores information that is never cleared. And
> this take-after-modify occurs when Git does a tag-following fetch from a
> shallow repository on a transport that does not support tag following
> (since in this case, 2 fetches are performed).
>
> To solve this issue, take the shallow file lock (and perform all other
> shallow processing) only if the "shallow-info" section is non-empty;
> otherwise, behave as if it were empty.

In other parts of the code we often have an early exit instead of
setting a variable and reacting to that in the end, i.e. what
do you think about:

static void receive_shallow_info(struct fetch_pack_args *args,
    struct packet_reader *reader)
{
     process_section_header(reader, "shallow-info", 0);
+    if (reader->status == PACKET_READ_FLUSH ||
+        reader->status == PACKET_READ_DELIM)
+            /* useful comment why empty sections appear */
+            return;
    while (packet_reader_read(reader) == PACKET_READ_NORMAL) {
    ...

instead? This would allow us to keep the rest of the function
relatively simple as well as we'd have a dedicated space where
we can explain why empty sections need to be treated specially.

> A full solution (probably, ensuring that any action of committing
> shallow file locks also includes clearing the information stored by
> is_repository_shallow()) would solve the issue without need for this
> patch, but this patch is independently useful (as an optimization to
> prevent writing a file in an unnecessary case), hence why I wrote it. I
> have included a NEEDSWORK outlining the full solution.

I like this patch..

> +test_expect_success 'ensure that multiple fetches in same process from a shallow repo works' '
> +       rm -rf server client trace &&
> +
> +       test_create_repo server &&
> +       test_commit -C server one &&
> +       test_commit -C server two &&
> +       test_commit -C server three &&
> +       git clone --shallow-exclude two "file://$(pwd)/server" client &&
> +
> +       git -C server tag -a -m "an annotated tag" twotag two &&
> +
> +       # Triggers tag following (thus, 2 fetches in one process)
> +       GIT_TRACE_PACKET="$(pwd)/trace" git -C client -c protocol.version=2 \
> +               fetch --shallow-exclude one origin &&
> +       # Ensure that protocol v2 is used
> +       grep "fetch< version 2" trace
> +'

Would we also need to ensure tags 'one' and 'three',
but not 'two' are present?
(What error condition do we see without this patch?)