In Mercurial 3.2 the locate command was deprecated in favor of files command. This change updates the DiffusionLowLevelMercurialPathsQuery command to conditionally use locate or files based on the version of Mercurial used.
Closes T7375
Details
Details
- Reviewers
epriestley - Group Reviewers
Blessed Reviewers - Maniphest Tasks
- T7375: Replace `hg locate` with `hg files` if `hg` is 3.2 or newer
- Commits
- Restricted Diffusion Commit
rP812c41a18abd: Conditionally use `hg files` vs. `hg locate` depending on version of Mercurial
My test/develop Phabricator instance is setup to run Mercurial 3.5.1.
The test procedure to verify valid file listings are being returned:
- I navigated to http://192.168.0.133/conduit/method/diffusion.querypaths/
- I populated the following fields:
- path: "/"
- commit: "d721d5b57fc9ef72e47ff9d4e0c583d74a46590c"
- callsign: "HGTEST"
- I submitted request and verified that result contained all files in the repository:
{
"0": "README",
"1": "alpha/beta/trifle",
"2": "test/Chupacabra.cow",
"3": "test/socket.ks"
}I repeated the above steps after setting up Mercurial 2.6.2, which I installed in the following manner:
- I downloaded Mercurial 2.6.2 source and run make local which will only compile it to work from its own directory (/opt/mercurial-2.6.2)
- I linked /usr/local/bin/hg -> /opt/mercurial-2.6.2/hg (there's also a /usr/bin/hg which is a link to /usr/local/bin/hg)
- I navigated to my home directory and verify that hg --version returns 2.6.2.
- I restarted phabricator services (probably unnecessary).
With the Multimeter application active
- I verified that /usr/local/bin/hg referred to version 2.6
- I ran the same conduit call from the conduit application
- I verified that http://192.168.0.133/multimeter/?type=2&group=label incremented values for bin.hg locate.
- I swapped out mercurial versions for 3.5.1
- I ran the same conduit call from the conduit application
- I verified that http://192.168.0.133/multimeter/?type=2&group=label incremented values for bin.hg files
Diff Detail
Diff Detail
- Repository
- rP Phabricator
- Branch
- master
- Lint
Lint Passed - Unit
Tests Passed - Build Status
Buildable 8230 Build 9413: arc lint + arc unit
Event Timeline
Comment Actions
- Changed unit test so cases are per-version, to allow for easily adding versions to check
Comment Actions
I believe I have tested this successfully. I will update the Test Plan to describe what steps I took.
I used the following commit to understand where hg locate is used:
rPd1936711a07724761c8862e404764ecf718c5df9
I started searching around for other areas where it might be used and came across:
https://secure.phabricator.com/diffusion/P/browse/master/src/applications/multimeter/data/MultimeterControl.php;bb37ad65a2b757891cc47efb32e9a2b9d63699e0$262
I'm not familiar with Multimeter -- wasn't sure what would be needed to update that, if at all.
Comment Actions
For Multimeter, I think you should just add files to the list:
'files' => true,
What that's doing is whitelisting CLI arguments for building this view:
By default, we show only the name of the binary, because we want to: (a) aggregate similar commands together and (b) not leak sensitive information. For example, a hypothetical rm <path> command would have a different path every time and prevent meaningful aggregation, and a hypothetical set-password <password> would leak sensitive information. By default, we would record these as bin.rm and bin.set-password.
So, by default, hg files will be lumped into all other default invocations of hg that aren't explicitly whitelisted. For comparison, see how git diff and git log are separate, but git is also present. Those are separated because of the whitelisting of diff and log near the code you linked to: when we run git diff or git log it gets split out, and when we run git <anything not on the whitelist> it gets bucked into git.
Since hg files is useful to separate, meaningful for aggregation, and not sensitive, adding files to the map will improve the quality of the Multimeter samples a little bit.
You don't need to bother testing that, but theoretically you'd do it like this:
- Set debug.sample-rate to 1 to record a Multimeter sample for every request.
- Load some pages running hg files.
- Observe that Multimeter records events with generic label bin.hg (you have to fish around right now, there's no "most recent events" view).
- Add files to the map.
- Reload the hg files pages.
- Observe that Multimeter now separates those commands out into samples with the label bin.hg files.
Comment Actions
Maybe a slightly cleaner way to structure this is to expose a method like SomethingSomething::isMercurialFilesCommandAvailable($mercurial_version), and then keep the files vs locate logic private inside DiffusionLowLevelMercurialPathsQuery?
Structuring the version test like that lets you write a more pure unit test (no preg_match stuff), have a cleaner API (no implicit "expects to be run with 2 arguments" stuff), and might be useful to other callsites in the future.
Structured like the current diff, the files vs locate logic is a little better localized, but the API and test feel a lot messier to me, and I can't imagine any other callsite ever invoking this method.
| src/applications/diffusion/query/lowlevel/DiffusionLowLevelMercurialPathsQuery.php | ||
|---|---|---|
| 39 | Minor, but, for consistency, prefer Javadoc-style comments which put text on the first * line, not the initial /** line: /** * Comment starts here... | |
Comment Actions
OK - when adding these unit tests I did wonder a bit about how to make it testable, but also wasn't sure about needing to make the method public in order to be callable by the UT -- I struggled for a few minutes not sure whether the actual command string should be returned from the method or defined by the caller. I like the structure you propose.
| src/applications/diffusion/query/lowlevel/DiffusionLowLevelMercurialPathsQuery.php | ||
|---|---|---|
| 39 | Hah I mainly develop in Java and apparently I've been manually fixing the auto-javadoc style this whole time... Will fix. | |
Comment Actions
Updated based on feedback
- Added 'files' to multimeter commands
- Restructured method for determining which command to use by returning a boolean value instead of a format string
- Moved the method for testing whether files is available into PhabricatorRepositoryVersion
- Updated unit test appropriately for the refactored method