Hi Jonathan,

On Wed, Jun 03, 2020 at 04:06:11PM -0700, Jonathan Nieder wrote:

Taylor Blau wrote:

quoted

On Wed, Jun 03, 2020 at 01:51:51PM -0700, Jonathan Nieder wrote:

quoted

quoted

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