Page MenuHomePhabricator

Conditionally use `hg files` vs. `hg locate` depending on version of Mercurial
ClosedPublic

Authored by cspeckmim on Oct 11 2015, 6:59 PM.
Tags
None
Referenced Files
F10883207: D14253.id34433.diff
Fri, Jul 1, 4:20 AM
Unknown Object (File)
Sat, Jun 18, 10:36 AM
Unknown Object (File)
Thu, Jun 16, 6:04 AM
Unknown Object (File)
Sat, Jun 11, 11:44 AM
Unknown Object (File)
Fri, Jun 10, 4:25 PM
Unknown Object (File)
Fri, Jun 10, 12:35 PM
Unknown Object (File)
Fri, Jun 10, 12:01 PM
Unknown Object (File)
Fri, Jun 10, 9:43 AM
Subscribers

Details

Summary

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

Test Plan

My test/develop Phabricator instance is setup to run Mercurial 3.5.1.

The test procedure to verify valid file listings are being returned:

  1. I navigated to http://192.168.0.133/conduit/method/diffusion.querypaths/
  2. I populated the following fields:
    • path: "/"
    • commit: "d721d5b57fc9ef72e47ff9d4e0c583d74a46590c"
    • callsign: "HGTEST"
  3. 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:

  1. 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)
  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)
  3. I navigated to my home directory and verify that hg --version returns 2.6.2.
  4. I restarted phabricator services (probably unnecessary).

With the Multimeter application active

  1. I verified that /usr/local/bin/hg referred to version 2.6
  2. I ran the same conduit call from the conduit application
  3. I verified that http://192.168.0.133/multimeter/?type=2&group=label incremented values for bin.hg locate.
  4. I swapped out mercurial versions for 3.5.1
  5. I ran the same conduit call from the conduit application
  6. I verified that http://192.168.0.133/multimeter/?type=2&group=label incremented values for bin.hg files

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

cspeckmim retitled this revision from to Conditionally use `hg files` vs. `hg locate` depending on version of Mercurial.
cspeckmim updated this object.
cspeckmim edited the test plan for this revision. (Show Details)
cspeckmim added a reviewer: epriestley.
cspeckmim edited the test plan for this revision. (Show Details)
cspeckmim edited edge metadata.
  • Changed unit test so cases are per-version, to allow for easily adding versions to check

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.

cspeckmim edited the test plan for this revision. (Show Details)
cspeckmim edited the test plan for this revision. (Show Details)
cspeckmim edited the test plan for this revision. (Show Details)

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:

Screen Shot 2015-10-12 at 6.31.44 AM.png (1×1 px, 177 KB)

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.

Will update the Multimeter code.

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
43

Minor, but, for consistency, prefer Javadoc-style comments which put text on the first * line, not the initial /** line:

/**
 * Comment starts here...

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.

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
43

Hah I mainly develop in Java and apparently I've been manually fixing the auto-javadoc style this whole time... Will fix.

cspeckmim edited edge metadata.

Updated based on feedback

  1. Added 'files' to multimeter commands
  2. Restructured method for determining which command to use by returning a boolean value instead of a format string
  3. Moved the method for testing whether files is available into PhabricatorRepositoryVersion
  4. Updated unit test appropriately for the refactored method
This revision is now accepted and ready to land.Oct 13 2015, 12:50 AM
This revision was automatically updated to reflect the committed changes.