[PATCH v3] commit,shallow: unparse commits if grafts changed

[Jonathan Tan <jonathantanmy@google.com>]

When a commit is parsed, it pretends to have a different (possibly
empty) list of parents if there is graft information for that commit.
But there is a bug that could occur when a commit is parsed, the graft
information is updated (for example, when a shallow file is rewritten),
and the same commit is subsequently used: the parents of the commit do
not conform to the updated graft information, but the information at the
time of parsing.

This is usually not an issue, as a commit is usually introduced into the
repository at the same time as its graft information. That means that
when we try to parse that commit, we already have its graft information.

But it is an issue when fetching a shallow point directly into a
repository with submodules. The function
assign_shallow_commits_to_refs() parses all sought objects (including
the shallow point, which we are directly fetching). In update_shallow()
in fetch-pack.c, assign_shallow_commits_to_refs() is called before
commit_shallow_file(), which means that the shallow point would have
been parsed before graft information is updated. Once a commit is
parsed, it is no longer sensitive to any graft information updates. This
parsed commit is subsequently used when we do a revision walk to search
for submodules to fetch, meaning that the commit is considered to have
parents even though it is a shallow point (and therefore should be
treated as having no parents).

Therefore, whenever graft information is updated, mark the commits that
were previously grafts and the commits that are newly grafts as
unparsed.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
Thanks everyone for your reviews. I've updated the test following Ævar's
suggestion.
---
Range-diff against v2:
1:  368e40d660 ! 1:  a22cc1b02b commit,shallow: unparse commits if grafts changed
    @@ Commit message
         unparsed.
     
         Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
    +    ---
    +    Thanks everyone for your reviews. I've updated the test following Ævar's
    +    suggestion.
     
      ## commit.c ##
     @@ commit.c: int commit_graft_pos(struct repository *r, const struct object_id *oid)
    @@ shallow.c: int commit_shallow_file(struct repository *r, struct shallow_lock *lk
      
     
      ## t/t5537-fetch-shallow.sh ##
    -@@ t/t5537-fetch-shallow.sh: test_expect_success 'fetch --update-shallow into a repo with submodules' '
    +@@ t/t5537-fetch-shallow.sh: test_expect_success 'fetch --update-shallow' '
    + test_expect_success 'fetch --update-shallow into a repo with submodules' '
    + 	git init a-submodule &&
    + 	test_commit -C a-submodule foo &&
    ++
    ++	test_when_finished "rm -rf repo-with-sub" &&
    + 	git init repo-with-sub &&
    + 	git -C repo-with-sub submodule add ../a-submodule a-submodule &&
    + 	git -C repo-with-sub commit -m "added submodule" &&
      	git -C repo-with-sub fetch --update-shallow ../shallow/.git refs/heads/*:refs/remotes/shallow/*
      '
      
     +test_expect_success 'fetch --update-shallow a commit that is also a shallow point into a repo with submodules' '
    -+	git init repo-with-unreachable-upstream-shallow &&
    -+	git -C repo-with-unreachable-upstream-shallow submodule add ../a-submodule a-submodule &&
    -+	git -C repo-with-unreachable-upstream-shallow commit -m "added submodule" &&
    ++	test_when_finished "rm -rf repo-with-sub" &&
    ++	git init repo-with-sub &&
    ++	git -C repo-with-sub submodule add ../a-submodule a-submodule &&
    ++	git -C repo-with-sub commit -m "added submodule" &&
     +
     +	SHALLOW=$(cat shallow/.git/shallow) &&
    -+	git -C repo-with-unreachable-upstream-shallow fetch --update-shallow ../shallow/.git "$SHALLOW":refs/heads/a-shallow
    ++	git -C repo-with-sub fetch --update-shallow ../shallow/.git "$SHALLOW":refs/heads/a-shallow
     +'
     +
      test_expect_success 'fetch --update-shallow (with fetch.writeCommitGraph)' '

 commit.c                 | 16 +++++++++++++++-
 shallow.c                |  7 +++++++
 t/t5537-fetch-shallow.sh | 12 ++++++++++++
 3 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/commit.c b/commit.c
index 59b6c3e455..1537ea73d0 100644
--- a/commit.c
+++ b/commit.c
@@ -120,6 +120,17 @@ int commit_graft_pos(struct repository *r, const struct object_id *oid)
 		       commit_graft_oid_access);
 }
 
+static void unparse_commit(struct repository *r, const struct object_id *oid)
+{
+	struct commit *c = lookup_commit(r, oid);
+
+	if (!c->object.parsed)
+		return;
+	free_commit_list(c->parents);
+	c->parents = NULL;
+	c->object.parsed = 0;
+}
+
 int register_commit_graft(struct repository *r, struct commit_graft *graft,
 			  int ignore_dups)
 {
@@ -145,6 +156,7 @@ int register_commit_graft(struct repository *r, struct commit_graft *graft,
 			(r->parsed_objects->grafts_nr - pos - 1) *
 			sizeof(*r->parsed_objects->grafts));
 	r->parsed_objects->grafts[pos] = graft;
+	unparse_commit(r, &graft->oid);
 	return 0;
 }
 
@@ -253,8 +265,10 @@ void reset_commit_grafts(struct repository *r)
 {
 	int i;
 
-	for (i = 0; i < r->parsed_objects->grafts_nr; i++)
+	for (i = 0; i < r->parsed_objects->grafts_nr; i++) {
+		unparse_commit(r, &r->parsed_objects->grafts[i]->oid);
 		free(r->parsed_objects->grafts[i]);
+	}
 	r->parsed_objects->grafts_nr = 0;
 	r->parsed_objects->commit_graft_prepared = 0;
 }
diff --git a/shallow.c b/shallow.c
index 8ad5f22832..cf289a4c6d 100644
--- a/shallow.c
+++ b/shallow.c
@@ -97,6 +97,13 @@ int commit_shallow_file(struct repository *r, struct shallow_lock *lk)
 {
 	int res = commit_lock_file(&lk->lock);
 	reset_repository_shallow(r);
+
+	/*
+	 * Update in-memory data structures with the new shallow information,
+	 * including unparsing all commits that now have grafts.
+	 */
+	is_repository_shallow(r);
+
 	return res;
 }
 
diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
index 92948de7a0..10e9a7ff26 100755
--- a/t/t5537-fetch-shallow.sh
+++ b/t/t5537-fetch-shallow.sh
@@ -164,12 +164,24 @@ test_expect_success 'fetch --update-shallow' '
 test_expect_success 'fetch --update-shallow into a repo with submodules' '
 	git init a-submodule &&
 	test_commit -C a-submodule foo &&
+
+	test_when_finished "rm -rf repo-with-sub" &&
 	git init repo-with-sub &&
 	git -C repo-with-sub submodule add ../a-submodule a-submodule &&
 	git -C repo-with-sub commit -m "added submodule" &&
 	git -C repo-with-sub fetch --update-shallow ../shallow/.git refs/heads/*:refs/remotes/shallow/*
 '
 
+test_expect_success 'fetch --update-shallow a commit that is also a shallow point into a repo with submodules' '
+	test_when_finished "rm -rf repo-with-sub" &&
+	git init repo-with-sub &&
+	git -C repo-with-sub submodule add ../a-submodule a-submodule &&
+	git -C repo-with-sub commit -m "added submodule" &&
+
+	SHALLOW=$(cat shallow/.git/shallow) &&
+	git -C repo-with-sub fetch --update-shallow ../shallow/.git "$SHALLOW":refs/heads/a-shallow
+'
+
 test_expect_success 'fetch --update-shallow (with fetch.writeCommitGraph)' '
 	(
 	cd shallow &&
-- 
2.36.1.255.ge46751e96f-goog