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

> 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.

> 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).

> 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.