This is an archive of the discontinued Mercurial Phabricator instance.

merge: use constants for actions
ClosedPublic

Authored by indygreg on Mar 5 2018, 7:15 PM.
Tags
None
Subscribers

Details

Summary

We finish up establishing named constants in this file with
actions.

I remember scratching my head trying to figure out what this
code was doing as part of addressing a recent security issue with
subrepos. Having the named constants in place definitely makes
things easier to read.

I'm not convinced the new constants have the best names (I'm not
an expert in this code). But they can be changed easily enough.

Also, since these constants are internal only, we might want
to change their values to something more human readable to
facilitate debugging. Or maybe we could employ an enum type
some day...

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

indygreg created this revision.Mar 5 2018, 7:15 PM
pulkit added a subscriber: pulkit.Mar 5 2018, 7:29 PM

I also faced difficulty in past understanding these magic single or double
character values. I am +1 on this series.

phillco accepted this revision.Mar 5 2018, 9:06 PM
phillco added a subscriber: phillco.

Strong +1

mercurial/merge.py
909–910

Update this too?

indygreg marked an inline comment as done.Mar 5 2018, 9:11 PM
indygreg added inline comments.
mercurial/merge.py
909–910

Done.

What's... amusing is that `dr and rd` are not actions in the current code base.

phillco added inline comments.Mar 5 2018, 10:02 PM
mercurial/merge.py
909–910

lol

btw, @quark mentioned there could be a perf hit (@sid0 mentioned this too a long time ago iirc)

indygreg marked an inline comment as done.Mar 6 2018, 11:53 AM

btw, @quark mentioned there could be a perf hit (@sid0 mentioned this too a long time ago iirc)

Are we sure about that? I could see a perf hit for dirstate. But I don't think merge things are in any critical paths. If you are performing a merge, you need to do working directory updates and/or read data from storage. I would expect these things to dwarf the time spent resolving Python variables to values.

durin42 accepted this revision.Mar 24 2018, 3:08 PM
This revision is now accepted and ready to land.Mar 24 2018, 3:08 PM
This revision was automatically updated to reflect the committed changes.
yuja added a subscriber: yuja.Mar 26 2018, 10:11 AM

If the perf hit was because of lookup of global variables, maybe we can alias
them in functions where hot loop exists.