Re: [PATCH v2] commit,shallow: unparse commits if grafts changed

[Derrick Stolee <derrickstolee@github.com>]

On 6/3/2022 5:30 AM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Thu, Jun 02 2022, Jonathan Tan wrote:
> 
>> diff --git a/commit.c b/commit.c
>> index 59b6c3e455..1537ea73d0 100644
>> --- a/commit.c
>> +++ b/commit.c
>> @@ -120,6 +120,17 @@ int commit_graft_pos(struct repository *r, const struct object_id *oid)
>>  		       commit_graft_oid_access);
>>  }
>>  
>> +static void unparse_commit(struct repository *r, const struct object_id *oid)
>> +{
>> +	struct commit *c = lookup_commit(r, oid);
>> +
>> +	if (!c->object.parsed)
>> +		return;
>> +	free_commit_list(c->parents);
>> +	c->parents = NULL;
>> +	c->object.parsed = 0;
>> +}

This looks good. I took a quick inventory of 'struct commit' and
agree that this is all we need. There is no need to clear the date
or maybe_tree members. The commit_graph_data slab might have some
information if there were no grafts before (but are now), but the
existence of grafts should clear the commit-graph already and stop
that slab from being used.

>>  int register_commit_graft(struct repository *r, struct commit_graft *graft,
>>  			  int ignore_dups)
>>  {
>> @@ -145,6 +156,7 @@ int register_commit_graft(struct repository *r, struct commit_graft *graft,
>>  			(r->parsed_objects->grafts_nr - pos - 1) *
>>  			sizeof(*r->parsed_objects->grafts));
>>  	r->parsed_objects->grafts[pos] = graft;
>> +	unparse_commit(r, &graft->oid);
>>  	return 0;
>>  }
>>  
>> @@ -253,8 +265,10 @@ void reset_commit_grafts(struct repository *r)
>>  {
>>  	int i;
>>  
>> -	for (i = 0; i < r->parsed_objects->grafts_nr; i++)
>> +	for (i = 0; i < r->parsed_objects->grafts_nr; i++) {
>> +		unparse_commit(r, &r->parsed_objects->grafts[i]->oid);
>>  		free(r->parsed_objects->grafts[i]);
>> +	}
>>  	r->parsed_objects->grafts_nr = 0;
>>  	r->parsed_objects->commit_graft_prepared = 0;
>>  }
> 
> Are we going to have the same issue with tags, c.f. parse_tag() and
> there being no unparse_tag()?
> 
> (I don't know offhand, just asking)

Grafts are only on commits. You cannot replace what a tag is pointing
at with a graft. Replace-objects is a different thing and changes it at
the OID level (and I don't think this can happen during the process
without concurrent external changes to the filesystem).

Thanks,
-Stolee