Page MenuHomePhabricator

Compare two branches
ClosedPublic

Authored by avivey on Feb 23 2016, 1:38 AM.

Details

Summary

This shows the commits list only (Actual git diff will show up at a later date).
The inputs are left as text-fields, to allow the form to accept anything that can be resolved. The form is GET, to allow sharing URIs.

The conduit method response array is compatible with that of diffusion.historyquery, to make it easy to build
the "history" table.

The hardest part here was, of course, Naming. I think "from" and "onto" are unconfusing, and I'm fairly confident that the "to merge"
instructions are in sync with the actual content of the page.

Test Plan

Look at several "compare" views, with various values of "from" and "onto".

Diff Detail

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

Event Timeline

avivey updated this revision to Diff 36973.Feb 23 2016, 1:38 AM
avivey retitled this revision from to Compare two branches.
avivey updated this object.
avivey edited the test plan for this revision. (Show Details)
avivey added a reviewer: epriestley.
avivey edited the test plan for this revision. (Show Details)Feb 23 2016, 1:39 AM
avivey edited edge metadata.

src/applications/diffusion/controller/DiffusionCompareController.php
235

I've copied all of this code from Browse view, I think, with the TODO.

Another big TODO is "support hg, svn".

chad added a subscriber: chad.Feb 23 2016, 3:40 AM
epriestley edited edge metadata.Feb 23 2016, 11:00 PM
  • Can diffusion.revlist just be diffusion.historyquery with an extra against argument or similar?
  • What is cherryPicked?
  • Can we get all the data in a single command by using --left-right and --format=%m:%H:%P or similar?
  • Do users really not know how to merge?

Also, your custom art is beautiful.

  • Can diffusion.revlist just be diffusion.historyquery with an extra against argument or similar?

It's mostly identical, so I guess so.

  • What is cherryPicked?

The idea is to use --cherry-mark to show that some commits have already been cherry-picked on their own to the destination. But I didn't do the UI and removed the implementation (--cherry-mark was introduced in 1.7.5).
I still think it's a useful signal in this context, and would like to someday (Maybe soon) display it.

  • Can we get all the data in a single command by using --left-right and --format=%m:%H:%P or similar?

I take a bunch of data for this:

  • list of commits (rev-list --right-only) with limit
  • count of commits (rev-list --left-right --count) - we can join these 2 to one rev-list call with no limit (And limit later, or blast the UI).
  • merge-base - rev-list can't expose this, but in theory if we calculate this one first, all rev-list stuff would be faster / can be resolved from it? Or maybe I don't really want it.

In the most "pure" sense, only rev-list --right-only is "really" required, but I think the use-cases I have in mind ("Show me the difference before I merge to production") would generally benefit from the extra 3 value (Size of this merge, size or reverse merge, Last thing we merged). I think.

  • Do users really not know how to merge?

That was an attempt to explain what 'from' and 'onto' means... not sure if it made it any better or worse? More text? Pictures? I'd like a picture, but my skills are what produced that lovely custom art...

avivey planned changes to this revision.Mar 1 2016, 12:54 AM

To be fair, I'm pretty sure that locally I'll render the same information a little differently (Showing related JIRA tickets, names of all reviewers, maybe even pull a custom "Release notes" field), and just use the same conduit method; This view is more of a first-pass than a full-feature.

So, it might makes sense to:

  • Remove the "reverse merge" completely, from the UI and method, as well as the commit count,
  • Move this back into diffusion.historyquery, with against argument,
  • if against argument is provided, also return merge-base value (At the cost of another git call).
avivey updated this revision to Diff 37054.Mar 1 2016, 3:31 AM
avivey edited the test plan for this revision. (Show Details)
avivey edited edge metadata.
  • Remove counts, reverse compare, remove git-merge instructions
  • Remove new method, use diffusion.historyquery
  • Added merge-base to historyquery
  • Paging
avivey updated this revision to Diff 39940.Sep 23 2016, 11:31 PM
  • rebase
  • drop merge-base
  • rename "from/onto" to "head/against", which is slightly more user-friendly, maybe.

@epriestley, does this have a shot at being accepted, or is it completely on the wrong path?

(I'd like to add path argument and some hook to add actions if so.)

epriestley accepted this revision.Dec 6 2016, 12:25 AM
epriestley edited edge metadata.

I'm going to mangle this a bit in the pull but I think it's mostly in reasonable shape.

This revision is now accepted and ready to land.Dec 6 2016, 12:25 AM
This revision was automatically updated to reflect the committed changes.

(I resolved some merge conflicts and removed the actual action from the UI for the moment.)