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

From: Jonathan Tan <jonathantanmy@google.com>
Date: Mar 24, 2022, 22:15 UTC
Message-ID: <20220324221519.1324898-1-jonathantanmy@google.com>
In-Reply-To: <34da1688-7d94-d54c-6b3a-3106d0f15380@github.com>

Derrick Stolee <derrickstolee@github.com> writes:

Show 11 quoted lines

> On 3/23/2022 7:30 PM, Junio C Hamano wrote:
> > 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?
>  
> I agree that the adjustment to paint_down_to_common() is a bit too
> coupled to this scenario, when we should just trust our commits to
> be valid if they have been parsed. We should always be able to
> parse our parents.

Thanks for the comments from both of you. I think Stolee's comment squarely hits the relevant points: it is precisely this scenario (revision walk to remove unreachable shallow commits) that must be careful of what it parses, and we *must not* parse the shallow boundary commit's parents.

I think that there are 2 questions. First, whether we should parse the shallow boundary commit's parents, and second, whether we should parse the shallow boundary commit itself. In the commit message, I only discussed the second (because that implies the first), but perhaps I should have discussed both. Anyway, the discussion:

(a) Should we parse the shallow boundary commit's parents? I think the
    obvious answer is no, because the remote probably wouldn't have sent
    them. But the code currently does: in paint_down_to_common(), they
    are parsed before being added to the priority queue (and parsing is
    necessary because the priority queue requires their date).
    Incidentally, this results in an error message from
    repo_parse_commit_internal() being printed, but
    repo_in_merge_bases_many() swallows the error by not checking the
    return value (it only checks whether a certain commit has a certain
    flag, which is true by the time the failing parent parse occurs). So
    we should have some sort of one_is_at_min_generation anyway, at
    least so that we can prevent its parents from being parsed.

(b) Should we parse the shallow boundary commit itself? If we don't
    care, then we should unparse commits when grafts are cleared.

    In this case, though, I think that it is the responsibility of the
    shallow code to be careful with what it does with the commits. It is
    performing operations on commits that it alone knows shallow
    information about (because the shallow information is still being
    checked and thus not yet written to the repo). As I wrote in the
    commit message (which is admittedly long and perhaps hard to
    understand), I think that in the typical case, we only have a commit
    when its graft information is already present, so we don't need to
    worry about graft information changing from under it.

> Finding this bug is interesting, but I agree with Junio that a
> better fix would be to "unparse" a commit when modifying its graft
> in any way. That will universally fix it for any potential future
> commit walks that might be hit due to future code changes.

Unparsing also means that code can't rely on commits being already parsed, even if they would expect it to be true (for example, a commit in a priority queue would be expected to be parsed, since we would have needed the date for it to enter the queue in the first place), but maybe this is not a big problem in practice.