shallow.c: use '{commit,rollback}_shallow_file'

In bd0b42aed3 (fetch-pack: do not take shallow lock unnecessarily,
2019-01-10), the author noted that 'is_repository_shallow' produces
visible side-effect(s) by setting 'is_shallow' and 'shallow_stat'.

This is a problem for e.g., fetching with '--update-shallow' in a
shallow repository with 'fetch.writeCommitGraph' enabled, since the
update to '.git/shallow' will cause Git to think that the repository
isn't shallow when it is, thereby circumventing the commit-graph
compatibility check.

This causes problems in shallow repositories with at least shallow refs
that have at least one ancestor (since the client won't have those
objects, and therefore can't take the reachability closure over commits
when writing a commit-graph).

Address this by introducing thin wrappers over 'commit_lock_file' and
'rollback_lock_file' for use specifically when the lock is held over
'.git/shallow'. These wrappers (appropriately called
'commit_shallow_file' and 'rollback_shallow_file') call into their
respective functions in 'lockfile.h', but additionally reset validity
checks used by the shallow machinery.

Replace each instance of 'commit_lock_file' and 'rollback_lock_file'
with 'commit_shallow_file' and 'rollback_shallow_file' when the lock
being held is over the '.git/shallow' file.

As a result, 'prune_shallow' can now only be called once (since
'check_shallow_file_for_update' will die after calling
'reset_repository_shallow'). But, this is OK since we only call
'prune_shallow' at most once per process.

Helped-by: Jonathan Tan <jonathantanmy@google.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
Reviewed-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 2cc18bbffdcf41cf2510becf6fcdd74c5946494a..652661fa993c4f3fdde2d30115e5e0131cf57103 100644 (file)
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -872,12 +872,12 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)
        opt.env = tmp_objdir_env(tmp_objdir);
        setup_alternate_shallow(&shallow_lock, &opt.shallow_file, &extra);
        if (check_connected(command_singleton_iterator, cmd, &opt)) {
-               rollback_lock_file(&shallow_lock);
+               rollback_shallow_file(the_repository, &shallow_lock);
                oid_array_clear(&extra);
                return -1;
        }
 
-       commit_lock_file(&shallow_lock);
+       commit_shallow_file(the_repository, &shallow_lock);
 
        /*
         * Make sure setup_alternate_shallow() for the next ref does

diff --git a/commit.h b/commit.h
index 008a0fa4a01d06c0e191e5988823ff630931d30d..ab91d21131c8dfbc5386a3c5c9ac538f64d94601 100644 (file)
--- a/commit.h
+++ b/commit.h
@@ -249,6 +249,8 @@ struct oid_array;
 struct ref;
 int register_shallow(struct repository *r, const struct object_id *oid);
 int unregister_shallow(const struct object_id *oid);
+int commit_shallow_file(struct repository *r, struct lock_file *lk);
+void rollback_shallow_file(struct repository *r, struct lock_file *lk);
 int for_each_commit_graft(each_commit_graft_fn, void *);
 int is_repository_shallow(struct repository *r);
 struct commit_list *get_shallow_commits(struct object_array *heads,

diff --git a/fetch-pack.c b/fetch-pack.c
index 1734a573b010dd2f87541154cc70e5bb71b00463..a618f3b0293730d063850345a282805cb08b8233 100644 (file)
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1629,9 +1629,9 @@ static void update_shallow(struct fetch_pack_args *args,
        if (args->deepen && alternate_shallow_file) {
                if (*alternate_shallow_file == '\0') { /* --unshallow */
                        unlink_or_warn(git_path_shallow(the_repository));
-                       rollback_lock_file(&shallow_lock);
+                       rollback_shallow_file(the_repository, &shallow_lock);
                } else
-                       commit_lock_file(&shallow_lock);
+                       commit_shallow_file(the_repository, &shallow_lock);
                alternate_shallow_file = NULL;
                return;
        }
@@ -1655,7 +1655,7 @@ static void update_shallow(struct fetch_pack_args *args,
                        setup_alternate_shallow(&shallow_lock,
                                                &alternate_shallow_file,
                                                &extra);
-                       commit_lock_file(&shallow_lock);
+                       commit_shallow_file(the_repository, &shallow_lock);
                        alternate_shallow_file = NULL;
                }
                oid_array_clear(&extra);
@@ -1693,7 +1693,7 @@ static void update_shallow(struct fetch_pack_args *args,
                setup_alternate_shallow(&shallow_lock,
                                        &alternate_shallow_file,
                                        &extra);
-               commit_lock_file(&shallow_lock);
+               commit_shallow_file(the_repository, &shallow_lock);
                oid_array_clear(&extra);
                oid_array_clear(&ref);
                alternate_shallow_file = NULL;
@@ -1785,7 +1785,7 @@ struct ref *fetch_pack(struct fetch_pack_args *args,
                        error(_("remote did not send all necessary objects"));
                        free_refs(ref_cpy);
                        ref_cpy = NULL;
-                       rollback_lock_file(&shallow_lock);
+                       rollback_shallow_file(the_repository, &shallow_lock);
                        goto cleanup;
                }
                args->connectivity_checked = 1;

diff --git a/shallow.c b/shallow.c
index 7fd04afed19af7de9dc1438b328567d0b7f2b2b4..5010a6c73202b65f16edaadd261d034c6f009005 100644 (file)
--- a/shallow.c
+++ b/shallow.c
@@ -40,13 +40,6 @@ 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;
@@ -79,6 +72,25 @@ int is_repository_shallow(struct repository *r)
        return r->parsed_objects->is_shallow;
 }
 
+static void reset_repository_shallow(struct repository *r)
+{
+       r->parsed_objects->is_shallow = -1;
+       stat_validity_clear(r->parsed_objects->shallow_stat);
+}
+
+int commit_shallow_file(struct repository *r, struct lock_file *lk)
+{
+       int res = commit_lock_file(lk);
+       reset_repository_shallow(r);
+       return res;
+}
+
+void rollback_shallow_file(struct repository *r, struct lock_file *lk)
+{
+       rollback_lock_file(lk);
+       reset_repository_shallow(r);
+}
+
 /*
  * TODO: use "int" elemtype instead of "int *" when/if commit-slab
  * supports a "valid" flag.
@@ -410,10 +422,10 @@ void prune_shallow(unsigned options)
                if (write_in_full(fd, sb.buf, sb.len) < 0)
                        die_errno("failed to write to %s",
                                  get_lock_file_path(&shallow_lock));
-               commit_lock_file(&shallow_lock);
+               commit_shallow_file(the_repository, &shallow_lock);
        } else {
                unlink(git_path_shallow(the_repository));
-               rollback_lock_file(&shallow_lock);
+               rollback_shallow_file(the_repository, &shallow_lock);
        }
        strbuf_release(&sb);
 }

diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
index d02f9b5ae8bb14e2915439a870e12dbf9a54e3bb..126654817d3186704024e09b5459f52809ec46f0 100755 (executable)
--- a/t/t5537-fetch-shallow.sh
+++ b/t/t5537-fetch-shallow.sh
@@ -144,6 +144,35 @@ test_expect_success 'fetch --update-shallow' '
        )
 '
 
+test_expect_success 'fetch --update-shallow (with fetch.writeCommitGraph)' '
+       (
+       cd shallow &&
+       git checkout master &&
+       commit 8 &&
+       git tag -m foo heavy-tag-for-graph HEAD^ &&
+       git tag light-tag-for-graph HEAD^:tracked
+       ) &&
+       test_config -C notshallow fetch.writeCommitGraph true &&
+       (
+       cd notshallow &&
+       git fetch --update-shallow ../shallow/.git refs/heads/*:refs/remotes/shallow/* &&
+       git fsck &&
+       git for-each-ref --sort=refname --format="%(refname)" >actual.refs &&
+       cat <<-EOF >expect.refs &&
+       refs/remotes/shallow/master
+       refs/remotes/shallow/no-shallow
+       refs/tags/heavy-tag
+       refs/tags/heavy-tag-for-graph
+       refs/tags/light-tag
+       refs/tags/light-tag-for-graph
+       EOF
+       test_cmp expect.refs actual.refs &&
+       git log --format=%s shallow/master >actual &&
+       test_write_lines 8 7 6 5 4 3 >expect &&
+       test_cmp expect actual
+       )
+'
+
 test_expect_success POSIXPERM,SANITY 'shallow fetch from a read-only repo' '
        cp -R .git read-only.git &&
        test_when_finished "find read-only.git -type d -print | xargs chmod +w" &&