This is an archive of the discontinued LLVM Phabricator instance.

Improve the documentation on committing code reviewed on Phabricator to trunk.
ClosedPublic

Authored by delcypher on Dec 28 2015, 11:29 PM.

Details

Summary

[docs] Improve the documentation on committing code reviewed on
Phabricator to trunk.

The previous documentation had a few issues:

  • It did not make it explicit that code could be

committed without using the Arcanist tool and how this should be done.

  • There was also an implicit assumption on using Subversion

rather than git-svn in the example using Arcanist. The documentation now
explicitly mentions both cases and details how to commit to trunk in
each case.

Diff Detail

Event Timeline

delcypher updated this revision to Diff 43714.Dec 28 2015, 11:29 PM
delcypher retitled this revision from to Improve the documentation on committing code reviewed on Phabricator to trunk. .
delcypher updated this object.
delcypher added reviewers: klimek, chandlerc.
delcypher added subscribers: llvm-commits, reames, nwilson.

Thanks for describing the git-svn sequence. Some inline comments.

docs/Phabricator.rst
132

"Whichever" is one word.

146

Remove the last "this" on the line.

148

I'd write that last sentence in a more active-voice style: "If you don't want to use Arcanist, you will need to add the Differential Revision line to the commit message yourself."

Did you also want to add a pointer to the Phab page on the markup language? It isn't necessarily obvious that Phabricator is actually a separate project with its own documentation; the code-review pages don't have any "help" links for example. (The only docs I was ever able to find were pretty generic "code reviews are good for you" pages, never did find any UI help of any kind.)

nwilson added inline comments.Dec 29 2015, 2:58 PM
docs/Phabricator.rst
146

Maybe replace the first this with something like hyperlinked commit line since that's what we're talking about and is being referenced in the previous paragraph.

delcypher updated this revision to Diff 43774.Dec 29 2015, 8:19 PM
  • Address probinson comments
  • Address nwilson comments
  • Introduce how arcanist can help commit code in a slightly more verbose way.
delcypher marked 3 inline comments as done.Dec 29 2015, 8:20 PM

Mark comments as done.

delcypher marked an inline comment as done.Dec 29 2015, 8:20 PM

Did you also want to add a pointer to the Phab page on the markup language? It isn't necessarily obvious that Phabricator is actually a separate project with its own documentation; the code-review pages don't have any "help" links for example. (The only docs I was ever able to find were pretty generic "code reviews are good for you" pages, never did find any UI help of any kind.)

This is out of scope for what this commit is trying to address but I could certainly address it in a later commit. There doesn't seem to be a relevant section for this sort of information. Perhaps there should be a Tips on usiing Phabricator section or something?

Did you also want to add a pointer to the Phab page on the markup language? It isn't necessarily obvious that Phabricator is actually a separate project with its own documentation; the code-review pages don't have any "help" links for example. (The only docs I was ever able to find were pretty generic "code reviews are good for you" pages, never did find any UI help of any kind.)

This is out of scope for what this commit is trying to address but I could certainly address it in a later commit. There doesn't seem to be a relevant section for this sort of information. Perhaps there should be a Tips on usiing Phabricator section or something?

The "Reviewing code with Phabricator" section seems like it would be a reasonable place to link to the markup description. Or a new "tips" section could work too. Deferring that to a followup is fine.

Current revision LGTM.

klimek added inline comments.Jan 5 2016, 3:43 AM
docs/Phabricator.rst
151–158

I would phrase that less strongly. Remember that email is still the only system of record, so there's no requirement to make phab auto-close issues.

I'd probably rephrase to something like:
Note that you can get phabricator to automatically close the review when it finds the following line in the commit message:
...

166–168

Drop the must, it really is optional.

delcypher added inline comments.Jan 5 2016, 4:16 AM
docs/Phabricator.rst
151–158

I would phrase that less strongly. Remember that email is still the only system of record, so there's no requirement to make phab auto-close issues.

I don't really agree with that. Although there is no requirement to use Phabricator for reviews if the decision has been made to use it then I think we should be documenting the way Phabricator should be used that is most helpful to LLVM:

  • Having closed reviews makes searching for un-committed patches easier
  • Having a link in a commit message to the Phabricator review is useful when looking back at LLVM's history

I'm happy to weaken the language a little (e.g. turning must into should) if that helps.

166–168

I'm happy to weaken the language a little (e.g. turning must into should) here if that helps.

klimek added inline comments.Jan 5 2016, 5:08 AM
docs/Phabricator.rst
151–158

How about saying "... consider to ..."?

Phab is currently really just there to make it easier to do email based code reviews. If you want to propose a change to that policy, feel free to propose it on the llvm-dev/cfe-dev mailing lists.

delcypher updated this revision to Diff 44375.Jan 8 2016, 1:57 PM
  • Weaken "must" as requested by Manuel. It now says "recommended"
  • Rebase on current trunk to encorporate the changes in r257180 which would not merge cleanly.

@klimek

Is this new version satisfactory to you?

  • Weaken "must" as requested by Manuel. It now says "recommended"
  • Rebase on current trunk to encorporate the changes in r257180 which would not merge cleanly.

Sorry! Forgot this was still in progress...

klimek added inline comments.Jan 13 2016, 7:13 AM
docs/Phabricator.rst
151–158

s/you follow you/you follow/

klimek accepted this revision.Jan 13 2016, 7:13 AM
klimek edited edge metadata.

LG apart from the superfluous "you".

This revision is now accepted and ready to land.Jan 13 2016, 7:13 AM
delcypher edited reviewers, added: probinson; removed: chandlerc.Jan 14 2016, 5:30 AM
delcypher updated this revision to Diff 44866.Jan 14 2016, 5:38 AM
  • Fix typo
This revision was automatically updated to reflect the committed changes.