Page MenuHomePhabricator

Initial documentation of example Differential workflows (with Gerrit equivalents)
Closed, ResolvedPublic

Description

Meta task purpose

The purpose of this task is to track the initial big dump of documentation for code-review in Phabricator. Documentation is, by its very nature, on-going.

This task will track the major documentation work needed to be completed in FY1516-Q4 (April - June 2016). Documentation work will continue after that but this task will probably be resolved (if we can get the basics documented).

Submitting a differential diff (D36)

$ git checkout master
$ git checkout -b sync_flag
$ vim scap/main.py
$ git add scap/main.py
$ git commit

# write commit message...

[sync_flag 94d1df5] abort sync if a file named sync.flag exists
 1 file changed, 16 insertions(+)

$ arc diff

Linting...
 LINT OKAY  No lint problems.
Running unit tests...
 UNIT OKAY  No unit test failures.
 SKIP STAGING  No staging area is configured for this repository.
Updating commit message...
Created a new Differential revision:
        Revision URI: https://phabricator.wikimedia.org/D36

Included changes:
  M       scap/main.py

Submitting a draft diff

This allows you to start differential revision that is still a work in progress and not yet ready for review. The only difference from regular revisions is that you add --plan-changes argument to arc diff, to signify that your revision is a work in progress/draft. This also allows you to submit a diff without explicitly naming reviewers. Optionally, you may name interested parties via the --cc argument as I have done here:

$ git checkout master
$ git checkout -b wip
$ vim some_new_thing.py
$ git commit -a -m 'WIP: some cool new thing'
$ arc diff --plan-changes -cc bd808

Landing a revision

The arc land command has been greatly improved by rARCa03c6079bb71: Make "arc land" great again. Now the workflow is absolutely straightforward and arc land will almost always do the right thing.

To land your own revision that exists in a local branch, use arc land [branch-name]. To land a revision that you don't have in a local branch, pass the differential revision as the argument to arc land [revision], e.g. arc land D123

Here is a complete real-world example, including the output from the arc land command:

$ cd /path/to/local-copy/
$ arc land T119200

 TARGET  Landing onto "master", the default target under git.
 REMOTE  Using remote "origin", the default remote under git.
 FETCH  Fetching origin/master...
These commits will be landed:

      - 763b3e6 search the environment specific path if specified
      - 4d264e9 Change search_path arg to a list
      - b6f3207 Add a search_path for finding target hosts files

Landing revision 'D59: Add a search_path for finding target hosts files'...
Harbormaster is still building the active diff for this revision:

     BUILDING  Build 279: test harbormaster with jenkins
     PASSED  Build 278: arc lint + arc unit

You can review build details here:

    Harbormaster URI: https://phabricator.wikimedia.org/B217

    Land revision anyway, despite ongoing build? [y/N] y

 PUSHING  Pushing changes to "origin/master".
Counting objects: 5, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (5/5), done.
Writing objects: 100% (5/5), 1.19 KiB | 0 bytes/s, done.
Total 5 (delta 4), reused 0 (delta 0)
To ssh://vcs@git-ssh.wikimedia.org/diffusion/MSCA/scap.git
   b62f945..578cce9  578cce980ac8f78ababaed18f34c65e31851ed29 -> master
 UPDATE  Local "master" tracks target remote "origin/master", checking out and pulling changes.
 PULL  Checking out and pulling "master".
Cleaning up branch "T119200"...
(Use `git checkout -b T119200 763b3e6026ebe032e25f0eaf50a938ca733a69bb` if you want it back.)
 DONE  Landed changes.

Related Objects

Event Timeline

mmodell created this task.Oct 29 2015, 4:53 PM
mmodell claimed this task.
mmodell raised the priority of this task from to Medium.
mmodell updated the task description. (Show Details)
mmodell added subscribers: mmodell, demon, greg, Aklapper.
greg awarded a token.Oct 29 2015, 4:55 PM
hashar awarded a token.
hashar added a subscriber: hashar.
greg set Security to None.

Repository access controls:

some discussion about differential "sticky accept" T164

Qgil added a subscriber: Qgil.
greg moved this task from To Triage to Backlog on the Differential board.Nov 4 2015, 7:03 PM
mmodell updated the task description. (Show Details)Nov 5 2015, 11:47 AM
mmodell updated the task description. (Show Details)Nov 5 2015, 11:52 AM
mmodell updated the task description. (Show Details)Nov 30 2015, 8:51 PM
mmodell removed a subscriber: chasemp.
greg moved this task from To Triage to Documentation on the Gerrit-Migration board.Dec 1 2015, 4:37 PM
greg added a comment.Dec 2 2015, 12:22 AM

I created a stub of a page at https://www.mediawiki.org/wiki/Phabricator/Differential, we could probably document this stuff there? (or on a subpage(s)?) There's also https://www.mediawiki.org/wiki/Phabricator/Arcanist though.... we need an Information Architect :)

@greg: you couldn't be more right. The information architecture of mediawiki documentation is horrendous.

Maybe another problem:
I don't know, how you want to control, who can review a patch. You can use blocking reviewers, added via herald, but the problem is, that everybody who can edit a revision can remove the blocking reviewer(s), so, if we use a system like gerrit, where a bot will merge the commits, this could be a problem. Otherwise some people have to merge it manually (better access control).

@Luke081515: I don't think we should have any bots doing merges with differential.

mmodell renamed this task from Create example workflows for differential showing old way and new way side by side to Document gerrit workflows and demonstrate equivalent in arcanist workflows.Dec 2 2015, 8:07 AM
mmodell added a subscriber: cscott.
hashar added a comment.Dec 2 2015, 8:58 AM

@Luke081515: I don't think we should have any bots doing merges with differential.

In Gerrit almost all changes are merged via bots. Either via Zuul or the l10n bot that send localization changes from translatewiki.

Luke081515 added a comment.EditedDec 2 2015, 10:13 AM

Another problem: Do we got a function, that we don't make all unit tests for patches from new users?

@Luke081515: I don't think we should have any bots doing merges with differential.

In Gerrit almost all changes are merged via bots. Either via Zuul or the l10n bot that send localization changes from translatewiki.

But why? is it really a good idea?

I'd like a document for those of us who use git as much as possible and will use arcanist as little as possible: Here are the 5 places when you absolutely must use arcanist because e.g. pasting your diff into the UI is horrible. I understand that this list will change over time and eventually be empty, but for folks who want to be early adopters/testers, it would be extremely helpful. Thanks.

Paladox added a subscriber: Paladox.Jan 6 2016, 8:53 PM

Could we also document on to easily use Arcanist to update three successive change?

Use case

For example, we want to modernize SomeOldExtension two successive changes, for example to:

  1. refactor the code to move code into classes
  2. some other modernize change (e.g. switch from magic numbers to class contants)
  3. use extension registration

We would craft small changes to make them more documented reviewable than a only change Modernize extension.

Workflows

In Gerrit, we can upload the three commits. It will handle automatically the deps. If we amend one of these three commits, we have the Change-Id unique identifier, so Gerrit knows how to rebase the whole set.

In Phabricator, it seems the easiest way is to:

  • send them to Differential with arc diff HEAD^
  • use the web UI to track the dependencies between the three changes.
  • amend the change and instruct explicitely Arcanist to select only the current commit with arc diff HEAD^ --update
  • check the history to avoid an history where a rebase operation included both the original change and the amended version

But there is maybe an easiest way?

mmodell added a comment.EditedMar 7 2016, 11:33 PM

@ArielGlenn: really arc diff and arc patch are the only arcanist commands that are absolutely critical. arc diff is needed for the reason you mentioned: because currently the only alternative is pasting a diff into a web form. Phabricator doesn't yet support "push to create revision" functionality. From my understanding of upstream plans, I believe that they will eventually implement a way to create a diff by simply git push HEAD:some/virtual/ref just like gerrit does.

arc patch D123 is the command you would run to apply someone else's patch to your repo.

Finally arc land is useful but if you are avoiding arcanist you can do the git wrangling manually (git merge or rebase, git commit --amend, git push)

Could we also document on to easily use Arcanist to update three successive change?

...

In Phabricator, it seems the easiest way is to:

  • send them to Differential with arc diff HEAD^
  • use the web UI to track the dependencies between the three changes.
  • amend the change and instruct explicitely Arcanist to select only the current commit with arc diff HEAD^ --update
  • check the history to avoid an history where a rebase operation included both the original change and the amended version

But there is maybe an easiest way?

I think there is an easier way but I'll have to spend a little time describing it and get back to you.

greg added a comment.Mar 23 2016, 7:12 PM

Workflow examples to be created (copied from the RFC page, mostly to clean it up/consolidate):

  • General code review workflow
  • WMF / SWAT deploy workflow (aka wmf branches / the cherry-pick button in Gerrit)
  • Post commit review workflow
  • Siebrand's workflow?
greg renamed this task from Document gerrit workflows and demonstrate equivalent in arcanist workflows to Document example Differential workflows (with Gerrit equivalents).Mar 23 2016, 7:14 PM
greg updated the task description. (Show Details)
greg added a comment.Mar 23 2016, 7:20 PM

(NB: title change was just for clarity and to be clear we aren't going to document all possible Gerrit workflows (there's almost as many workflows as developers) but instead the most commonly used ones, and of course more can be added as needed.)

greg added a comment.Mar 23 2016, 7:49 PM

From the RFC talk page:

Private patches
Maybe the way they are implemented in Gerrit they are a bad feature (I had no idea it even has such a feature), but a way to review security patches with proper code review tools instead of the current "upload as a file attachment and talk about it in comments" workflow would be quite valuable. --Tgr (WMF) (talk) 09:07, 11 December 2015 (UTC)

Reply from the mainspace page:

  • private repos / patches
    • Bad feature of Gerrit, will not be re-doing. (Chad, I believe)
demon added a comment.Mar 23 2016, 7:54 PM

From the RFC talk page:
Private patches
Maybe the way they are implemented in Gerrit they are a bad feature (I had no idea it even has such a feature), but a way to review security patches with proper code review tools instead of the current "upload as a file attachment and talk about it in comments" workflow would be quite valuable. --Tgr (WMF) (talk) 09:07, 11 December 2015 (UTC)

Ideally yes, it would be nice to submit something with arc diff and have it be secured for review by default. I'll have to look into this...

Reply from the mainspace page:

  • private repos / patches
    • Bad feature of Gerrit, will not be re-doing. (Chad, I believe)

Yes. The refs/drafts/* implementation in Gerrit is terrible and that was me. The reason it's not used for security is because the refs/* leak all over the place to people who shouldn't use them. Really, it's only useful for submitting a patch you're embarrassed to submit yet.

As far as private repos go: other than puppet's secrets we've never really had a usecase for them...

it should be possible to submit a secured diff...maybe :D

Qgil removed a subscriber: Qgil.Mar 30 2016, 9:40 AM
greg renamed this task from Document example Differential workflows (with Gerrit equivalents) to Initial documentation of example Differential workflows (with Gerrit equivalents).Apr 7 2016, 6:50 PM
greg updated the task description. (Show Details)
mmodell raised the priority of this task from Medium to High.Jun 14 2016, 5:54 AM
Qgil awarded a token.Jul 4 2016, 12:05 PM