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
___
Mercurial-devel mailing list
Mercurial-devel@lists.mercurial-scm.org
https://lists.mercurial-scm.org/mailman/listinfo/mercurial-devel
