Accidental modification of mercurial.changelog._defaultextra

[Pierre-Yves David]

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 ?

-- 

Pierre-Yves David
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.mercurial-scm.org/pipermail/mercurial-devel/attachments/20240201/8b64607a/attachment.html>