Accidental modification of mercurial.changelog._defaultextra

[Manuel Jacob]

On 01/02/2024 13.56, Pierre-Yves David wrote:
> 
> On 2/1/24 10:08, Manuel Jacob wrote:
>> When the raw extra field on a changeset is missing, the 
>> mercurial.changelog.changelogrevision.extra property returns the 
>> _defaultextra dictionary as-is. Method 
>> mercurial.context.changectx.extra() returns that property as-is. The 
>> problem is that if someone changes the return value in this case, all 
>> later calls to mercurial.context.changectx.extra() on changesets 
>> without the raw extra field will return a dictionary containing these 
>> changes.
>>
>> To check whether the problem results in bugs in practice, I changed 
>> the mercurial.changelog.changelogrevision.extra property to return a 
>> read-only proxy to _defaultextra. Three tests fail because of this: 
>> https://foss.heptapod.net/mercurial/mercurial-devel/-/jobs/2253663
>>
>> We could change the mercurial.changelog.changelogrevision.extra 
>> property to always return a new dictionary. This is already the case 
>> if a changeset has the raw extra field.
>>
>> If we don’t want to return a copied dictionary when a changeset has no 
>> raw extra field, we would need to fix all callers that modify the 
>> dictionary to copy it first instead of modifying the returned 
>> dictionary. In the cases when the returned dictionary is already a 
>> copied dictionary (i.e. if the changeset has the raw extra field), it 
>> would be redundant.
>>
>> If we make it part of the contract that the returned extra dictionary 
>> should not be modified, should that contract be enforced somehow? We 
>> could return types.MappingProxyType (1) never, (2) always or (3) only 
>> when _defaultextra is returned.
>>
>> What do you think?
> 
> 
> I think this is a pretty good catch, thanks you.
> 
> I don't believe this is reasonable to assume or enforce that caller will 
> not modify the returned dictionnary, and we should return a new 
> dictionnary in all cases. I don't expect any significant performance 
> impact from this.
> 
> Would you send a patch in that direction ?

https://foss.heptapod.net/mercurial/mercurial-devel/-/merge_requests/758

I added the patch against the default branch to ensure compatibility 
with extensions that depend on the buggy behavior. For example, evolve 
requires fixes like that: 
https://foss.heptapod.net/mercurial/evolve/-/merge_requests/554