fetch-pack: do not take shallow lock unnecessarily

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.

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.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/fetch-pack.c b/fetch-pack.c
index dd6700bda9240f2278e663f9147df1c0808d2efc..5885623eceb0de359bf8eb45af9370497cbe46b9 100644 (file)
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1232,6 +1232,8 @@ static int process_acks(struct fetch_negotiator *negotiator,
 static void receive_shallow_info(struct fetch_pack_args *args,
                                 struct packet_reader *reader)
 {
+       int line_received = 0;
+
        process_section_header(reader, "shallow-info", 0);
        while (packet_reader_read(reader) == PACKET_READ_NORMAL) {
                const char *arg;
@@ -1241,6 +1243,7 @@ static void receive_shallow_info(struct fetch_pack_args *args,
                        if (get_oid_hex(arg, &oid))
                                die(_("invalid shallow line: %s"), reader->line);
                        register_shallow(the_repository, &oid);
+                       line_received = 1;
                        continue;
                }
                if (skip_prefix(reader->line, "unshallow ", &arg)) {
@@ -1253,6 +1256,7 @@ static void receive_shallow_info(struct fetch_pack_args *args,
                                die(_("error in object: %s"), reader->line);
                        if (unregister_shallow(&oid))
                                die(_("no shallow found: %s"), reader->line);
+                       line_received = 1;
                        continue;
                }
                die(_("expected shallow/unshallow, got %s"), reader->line);
@@ -1262,8 +1266,11 @@ static void receive_shallow_info(struct fetch_pack_args *args,
            reader->status != PACKET_READ_DELIM)
                die(_("error processing shallow info: %d"), reader->status);
 
-       setup_alternate_shallow(&shallow_lock, &alternate_shallow_file, NULL);
-       args->deepen = 1;
+       if (line_received) {
+               setup_alternate_shallow(&shallow_lock, &alternate_shallow_file,
+                                       NULL);
+               args->deepen = 1;
+       }
 }
 
 static void receive_wanted_refs(struct packet_reader *reader,

diff --git a/shallow.c b/shallow.c
index 02fdbfc554c462c1eecdf1ddc6d17edbc1d8d853..ce45297940d417e3454b08d3f0c29f5cc6d93658 100644 (file)
--- a/shallow.c
+++ b/shallow.c
@@ -43,6 +43,13 @@ int register_shallow(struct repository *r, const struct object_id *oid)
 
 int is_repository_shallow(struct repository *r)
 {
+       /*
+        * NEEDSWORK: This function updates
+        * r->parsed_objects->{is_shallow,shallow_stat} as a side effect but
+        * there is no corresponding function to clear them when the shallow
+        * file is updated.
+        */
+
        FILE *fp;
        char buf[1024];
        const char *path = r->parsed_objects->alternate_shallow_file;

diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 0f2b09ebb8d6625b527ccc771c4725d2a314d4ee..fd164d414dad9cfe02fa435fd88dfe6ba6cfcc7b 100755 (executable)
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -471,6 +471,24 @@ test_expect_success 'upload-pack respects client shallows' '
        grep "fetch< version 2" trace
 '
 
+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
+'
+
 # Test protocol v2 with 'http://' transport
 #
 . "$TEST_DIRECTORY"/lib-httpd.sh