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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Jun 3, 2020, 23:06 UTC
Message-ID: <20200603230611.GA1925@gmail.com>
In-Reply-To: <20200603221453.GA36237@syl.local>

Taylor Blau wrote:

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

Show 17 quoted lines

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

Regards, Jonathan