Re: [PATCH] commit-reach: do not parse and iterate minima

[Junio C Hamano <gitster@pobox.com>]

Jonathan Tan <jonathantanmy@google.com> writes:

> 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.
>
> However, this is not the case when fetching with --update-shallow. In
> post_assign_shallow() in shallow.c, a revision walk is done that also
> parses commits at the shallow boundary before updating the shallow
> information (and hence, the graft information). (This revision walk
> needs to be done before the update because the nature of the update
> depends on the outcome of the revision walk.) If we were to
> revision-walk such a commit (at the shallow boundary), we would end up
> trying and failing to parse its parents because its list of parents is
> not empty (since it was parsed before there was any graft information
> telling us to conceal its parents). This revision walk will happen if
> the client has submodules, as it will revision-walk the fetched commits
> to check for new submodules, triggering this bug.
>
> This revision walk in post_assign_shallow() actually does not need to go
> beyond the shallow boundaries, so the solution is twofold: (1) do not
> iterate beyond such commits, and (2) in doing so, we no longer need to
> parse them, so do not parse them.

This sounds quite tricky.  In this case we may know which commit we
need to avoid (re)parsing to avoid the bug, but would it always be
the case?  It feels almost like we want to unparse the commit
objects when we clear the grafts information in the previous patch,
doesn't it?