Page MenuHomePhabricator

Moar functionality for `arc browse`!
Closed, ResolvedPublic

Description

arc browse is useful for popping up a file in the web browser, but wouldn't it be even awesomer if it could pop up other phabricator artifacts as well?

  • arc browse <sha1> would pop up http://phabricator/r<callsign><sha1> (for git and hg, at least)
  • arc browse rCCCxxx would pop up http://phabricator/rCCCxxxx
  • arc browse Dxxx, Txxx, Kxxxx, etc would pop up http://phabricator/Dxxx etc.

Well, we think it would be awesomer that way. (We'd be happy if there were a separate arc command that did that as well; we don't need it to be part of browse.)

One piece of functionality that would be particularly handy would be to have something like arc browse --differential <commitish> which would look at the given commit, and if it was part of a differential review would take you to http://phabricator/Dxxx. (Probably the syntax could be a lot better.)

The common use case would be to do something like this:

  1. arc diff
  2. arc browse --differential HEAD
  3. <go to the browser and add a comment or an image or whatever for the review you just created>

Event Timeline

csilvers raised the priority of this task from to Needs Triage.
csilvers updated the task description. (Show Details)
csilvers added a project: Arcanist.
csilvers added subscribers: csilvers, benkomalo.

For the latter case, maybe arc diff --browse would make sense?

Are you thinking arc diff + arc diff --browse, or just arc diff --browse which does an arc diff and then loads it in the browser?

I'm guessing the latter, which does seem like it would be useful functionality. But sometimes what happens is I run arc diff and as soon as it goes in -- isn't that always the way? -- I realize I left someone off the Cc or something. Having another command I could run right afterwards to get that diff in a browser, could be useful.

Of course, arc diff --browse would maybe have that behavior as well, since already arc diff has different behavior depending on whether there's a "current diff" on the branch or not. I guess arc diff --browse could just do the browser step, in the case there's already a diff at HEAD. Dunno if that makes sense.

Yeah, I was thinking the latter.

It's easy to implement arc browse T123 and it's easy to implement arc diff --and-then-open-the-revision-in-my-browser, but arc browse --the-related-revision-for <something> is a little more fuzzy and nongeneral. It would be nice if there was a more clear syntax extension path for --the-related-task-for, --the-related-pull-request-for (both of which may match more than one object), and so on.

D10140 implements the easy stuff (arc browse T123, arc browse D123).

It doesn't implement arc browse HEAD, arc browse a1b2c3, arc diff --browse, or arc browse --the-related-revision-for <something>.

arc browse --the-related-revision-for <something> is a little more fuzzy and nongeneral. It would be nice if there was a more clear syntax extension path for --the-related-task-for, --the-related-pull-request-for (both of which may match more than one object), and so on.

Agreed. I'm not sure of the best syntax for that, if any. (I think if there are multiple objects matched, you just load multiple tabs.)

@benkomalo (cc-ed) is the one who requested this feature, and here are his most common use-cases:

$ git phab HEAD^  # Opens the diff of the parent commit

$ arc diff  # Send out a review
$ git phab   # defaults to HEAD so takes you to diff you just opened, in case you wanted to add screenshots

# Or you can specify a commit explicitly if you're investigating a bug and
# something in the commit log looks fishy
$ git phab bd09c44 

I think in the last case, his preference would be to pop up a review if bd09c44 was part of a review, or just pop up that commit in isolation if it's not (because it's an audit, say).

I'm arguing git phab should actually be an arc command, but I don't have great insights into the best way to add all this functionality (assuming it's even deemed desirable to do so).

I think --the-related-review-for is desirable functionality, I just ideally want to try to come up with an extensible/modular way to build it.

In particular, the arc browse <object> added in D10140 works for any object, including objects we add in the future and objects third-party applications eventually add once all software is a layer on top of Phabricator.

I don't see a reasonable way to build --the-related-review-for that will work with everything for free.

I don't see a reasonable way to build --the-related-review-for that will work with everything for free.

What, you don't want to just parse every word in the commit message and see if it looks like a phabricator object? :^)

(That may not be so ridiculous actually, since you must do that anyway to parse out fixes Txxxx and the like. Though I guess it's not enough -- you'd also need to look at parent commits to see if the given commit is part of a review, and there you get into application-specific logic like you're hoping to avoid.)

Oh, it's easy to build --the-related-review. I mean it's hard to build --instead-of-this-object-open-the-related-object-I-want.

D10140 broke arc browse filename:<line-numbers>
It returns Unable to find an object named heading.js:66, no such commit exists in the remote, and no such path exists in the working copy. Use --force to treat this as a path anyway.

chad triaged this task as Low priority.Nov 1 2014, 4:05 PM
epriestley claimed this task.

T10895 is a reasonable followup for --revision / --do-what-I-mean-instead-of-what-I-say.