Re: [PATCH v2 2/2] shallow.c: use '{commit,rollback}_shallow_file'

[Taylor Blau <me@ttaylorr.com>]

Hi Jonathan,

On Wed, Jun 03, 2020 at 04:06:11PM -0700, Jonathan Nieder wrote:
> Taylor Blau wrote:
> > On Wed, Jun 03, 2020 at 01:51:51PM -0700, Jonathan Nieder wrote:
>
> >> --- i/commit-graph.c
> >> +++ w/commit-graph.c
> >> @@ -149,7 +149,8 @@ static int commit_graph_compatible(struct repository *r)
> >>  	}
> >>
> >>  	prepare_commit_graft(r);
> >> -	if (r->parsed_objects && r->parsed_objects->grafts_nr)
> >> +	if (r->parsed_objects &&
> >> +	    (r->parsed_objects->grafts_nr || r->parsed_objects->substituted_parent))
> >
> > This is a little tricky. Why would we set substituted_parent without
> > also incrementing grafts_nr? That seems like the real bug here: if we
> > incremented grafts_nr, then we would return a non-zero value from
> > 'commit_graph_compatible' and rightly stop even without this sticky-bit.
> >
> > I don't quite understand this myself. If it's an oversight, it's a
> > remarkably long-lived one. Do you have a better sense of this?
>
> The idea here is to check for two different things:
>
>  1. Do we have grafts (either from a grafts file or from a shallow
>     file)?  If so, this repository is not commit graph compatible
>     because we could encounter one of them.
>
>  2. Have cached parsed objects taken any history modifications into
>     account?  If so, this in-memory state is not commit graph
>     compatible because we could encounter one of them.

Ah, breaking it up like this makes it clearer. The gist is that it is
possible to generate a situation which has (1) no grafts explicitly, but
(2) *does* have history modifications.

> The check (1) might seem sufficient if the set of grafts is constant
> for the lifetime of a git process.  But since 37b9dcabfc (shallow.c:
> use '{commit,rollback}_shallow_file', 2020-04-22), it is not constant
> for the process lifetime, so we need the check (2) as well.
>
> We might want a similar check for replace refs as well some day, but
> not today (there is not a way to remove entries from replace_map
> during the life of a process).

Right.

> I can try sending a proper patch with commit message and tests
> tomorrow morning (or if someone else takes care of it, that's fine,
> too).  Thanks, both, for your help --- it was nice seeing a clear
> explanation of the cause already figured out and explained when I woke
> up.

If you are interested, by all means -- I'd certainly be appreciative.
I'm on vacation next week, and so it may be better if you are able to
shepherd the patches through. Otherwise, I'd be happy to do it the week
after when I get back.

> Regards,
> Jonathan

Thanks,
Taylor