[pinpoint] Calculate distances between Changes.

[pinpoint] Calculate distances between Changes.

We want to know the distances between the Changes so we can show them at the
proper distances on the chart. This lets users know how big the regression range
is, how far apart the regressions are, and how far along the Job is.

Change.Midpoint() and Commit.Midpoint() already do the legwork of downloading
DEPS files and calling out to gitiles_service.CommitRange(), so we can avoid
extra API calls by making Midpoint() provide the distance information as well.

BUG=catapult:#3869

[dtu, 2017-09-15 22:01:40]

dtu@chromium.org changed reviewers:
+ perezju@chromium.org, simonhatch@chromium.org, sullivan@chromium.org

[dtu, 2017-09-18 17:01:38]

The CQ bit was checked by dtu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-09-18 17:01:43]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-09-18 18:26:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-09-18 18:26:28]

Dry run: Try jobs failed on following builders:
  Catapult Mac Tryserver on master.tryserver.client.catapult (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Ma...)

[perezju, 2017-09-19 15:58:05]

Added a few comments for now; would still like to have a careful pass over the
Change.Midpoint logic.

https://codereview.chromium.org/3013713002/diff/1/dashboard/dashboard/pinpoin...
File dashboard/dashboard/pinpoint/models/change/change.py (right):

https://codereview.chromium.org/3013713002/diff/1/dashboard/dashboard/pinpoin...
dashboard/dashboard/pinpoint/models/change/change.py:102: A tuple of (Change,
(left, right)). left and right are the distances
I think it's cleaner if it returns just (change, left, right)

https://codereview.chromium.org/3013713002/diff/1/dashboard/dashboard/pinpoin...
File dashboard/dashboard/pinpoint/models/change/change_test.py (right):

https://codereview.chromium.org/3013713002/diff/1/dashboard/dashboard/pinpoin...
dashboard/dashboard/pinpoint/models/change/change_test.py:139:
file_contents.side_effect = _FileContents
I would like to see a test with nesteded DEPS, at least one extra level deeper.

Also lets add a comment describing the layout of commits being used for these
tests (e.g. which ones are rolls, in which order they fall).

Also wondering; it's a pretty rare scenario. But how would this treat a DEPS
rolling backwards? (i.e. reverting a DEPS roll.)

https://codereview.chromium.org/3013713002/diff/1/dashboard/dashboard/pinpoin...
File dashboard/dashboard/pinpoint/models/change/commit.py (right):

https://codereview.chromium.org/3013713002/diff/1/dashboard/dashboard/pinpoin...
dashboard/dashboard/pinpoint/models/change/commit.py:137: # Include both
commit_a and commit_b in the range.
nit: Add a short reminder that the list of commits is in reverse order (i.e.
from commit_b to commit_a); took me a small bit to figure out why the lengths
below seemed to be in the wrong order.

https://codereview.chromium.org/3013713002/diff/1/dashboard/dashboard/pinpoin...
dashboard/dashboard/pinpoint/models/change/commit.py:138:
commits.append({'commit': commit_a.git_hash})
I'm wondering whether gitiles_service.CommitRange should have an
"inclusive=True" option that basically returns '+log/{hash_a}^..{hash_b}'; so
this would be cleaner.

https://codereview.chromium.org/3013713002/diff/1/dashboard/dashboard/pinpoin...
File dashboard/dashboard/pinpoint/models/change/commit_test.py (right):

https://codereview.chromium.org/3013713002/diff/1/dashboard/dashboard/pinpoin...
dashboard/dashboard/pinpoint/models/change/commit_test.py:120: {'commit':
'1ef4789'},
these are a bit hard to think about, can you replace them with "hash_1",
"hash_2", etc.? (in reverse order, I assume?)

And same for other MidpointTest's below.

https://codereview.chromium.org/3013713002/diff/1/dashboard/dashboard/pinpoin...
dashboard/dashboard/pinpoint/models/change/commit_test.py:127: 
Can you also add a success test with an even number of commits in between?

[perezju, 2017-09-20 15:29:41]

https://codereview.chromium.org/3013713002/diff/1/dashboard/dashboard/pinpoin...
File dashboard/dashboard/pinpoint/models/change/change.py (right):

https://codereview.chromium.org/3013713002/diff/1/dashboard/dashboard/pinpoin...
dashboard/dashboard/pinpoint/models/change/change.py:76: """Returns a Change
halfway between the two given Changes.
After thinking through this, I think the important invariant that makes this
work (and why you complained about my example on your previous CL that: "These A
and B are only possible if the user explicitly set it that way") is that:

 for each pair (commit_a, commit_b) in zip(change_a.commits, change_b.commits)

 at most one of those pairs has a distance > 1; specifically the pair of commits
for the deepest nested DEP'ed repo which we most recently expanded into; commit
pairs for all other repos should be adjacent to each other (distance == 1).

Does that make sense? Is it correct?

If so, we should probably make that requirement explicit in the code. Should
also help to simplify the logic (I think) and make it easier to understand.

https://codereview.chromium.org/3013713002/diff/1/dashboard/dashboard/pinpoin...
dashboard/dashboard/pinpoint/models/change/change.py:201: deps_b =
commit_b.Deps()
The logic would be clearer if we would raise the error here in case of
mismatching DEPS. So the check for mismatching length above is not needed.

Actually, would it make sense to ignore added or removed DEPS, and just add to
both lists of commits the changed DEPS that both have?