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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Thu, Jun 02 2022, Jonathan Tan wrote:

> 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;
>  }

Are we going to have the same issue with tags, c.f. parse_tag() and
there being no unparse_tag()?

(I don't know offhand, just asking)

I have some semi-related (test) changes locally where we do have blind
spots in tag v.s. commit parsing semi-related to this, i.e. in the whole
"unparsed" stage.

So I wonder what happens with a tag that's pointing to a shallow object
that's parsed, but its underlying commit becomes un-parsed.

Or maybe that's impossible, I'm not too familiar with "shallow"...

> diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
> index 92948de7a0..ba0a4c5d15 100755
> --- a/t/t5537-fetch-shallow.sh
> +++ b/t/t5537-fetch-shallow.sh
> @@ -170,6 +170,15 @@ test_expect_success 'fetch --update-shallow into a repo with submodules' '
>  	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" &&
> +
> +	SHALLOW=$(cat shallow/.git/shallow) &&
> +	git -C repo-with-unreachable-upstream-shallow fetch --update-shallow ../shallow/.git "$SHALLOW":refs/heads/a-shallow
> +'

Nit: Can this be
e.g. s/repo-with-unreachable-upstream-shallow/repo/. The overly long
repo name makes this much harder to follow. Compare this one which would
clean up after itself too:

	test_when_finished "rm -rf repo" && 
	git init repo &&
	git -C repo submodule add ../a-submodule a-submodule &&
	git -C repo commit -m "added submodule" &&

	SHALLOW=$(cat shallow/.git/shallow) &&
	git -C repo fetch --update-shallow ../shallow/.git "$SHALLOW":refs/heads/a-shallow

(I didn't check if that test really works, i.e. do we have a "repo"
already, but you get the idea...