Page MenuHomePhabricator

Post-push workflow for code reviews
Closed, DuplicatePublic

Description

Our workflow goes something like this (it's similar to git-flow):

  • Create a branch off master for what we're working on (e.g. case-12345 or blah-123).
  • We makes commits and push these branches to the central repository (so that others can easily fetch them).
  • Then we want to do a code review based on the commits that are pushed.

At the moment "arc diff" has some strange semantics (that are often more suited to centralized VCS and not DVCS, even when git-branch workflow is used):

  • Working directory changes are included.
  • It creates a commit that is a copy of the last commit. I'm guessing this is because we're doing "git stash" / "git stash pop" to work around the working directory issues.
  • When we push the branch to Phabricator, it considers that to be approval of the commit, even though they are not merged onto master. In our workflow, we only consider them to be approved when they're actually merged into something (such as master), not just pushed to the server.

Is there any similar existing workflow that can be done, or is there a way we can integrate this workflow into Phabricator?

Related Objects

Event Timeline

Unknown Object (User) added a project: Differential.Feb 5 2013, 6:36 AM
Unknown Object (User) added a subscriber: Unknown Object (User).

Phabricator does not currently support a Gerrit-like remote-branch-based workflow. It can accommodate something similar, but trying to combine the two will result in some rough edges, at least for now. We will likely support this workflow eventually, but it's many iterations out (optimistically, maybe later this year).

On the specific issues you're hitting:

Working Directory Changes: arc diff normally submits a commit range for review which must end in the working directory state, so it can run linters (and fix errors they detect) and run unit tests. If you have uncommitted changes in your working copy, lint and unit tests would sometimes get the wrong result: for example, an uncommitted change might fix a broken test, and then arc would report that everything was OK when a change actually bad.

If you want to send arbitrary commit ranges for review, you can use git diff X..Y | arc diff --raw instead. However, this will disable a bunch of features that rely on access to a working copy.

Personally, I isolate related changes by using local branches (e.g., those working copy changes would be in another branch off the first one, not uncommitted in the working copy).

Another workflow is to stack commits in a single branch and use git rebase -i with "edit" to move the working copy's state through history.

(Do you avoid amend/rebase/reset --soft, which is why you have dirty working copies?)

Commit Copies: I can't come up with any theories on how this is happening, under strict readings of "copy". Can you explain this in more detail, and/or give me an example or a repro case? If it's literally creating a copy, that seems like it should be impossible (you can't make the same changes twice in a row, usually, since if commit A deleted a line, commit B can hardly delete it again). Is it creating an empty commit with the a copy of the message? Or some other commit (e.g., working directory changes) with a copy of the message?

Pushing: Go to the Repositories tool in the web UI, edit the repository, and set it to "Track Only: master".

With regards to the working directory required; I suggest taking a look at a script I wrote once at https://github.com/DCPUTeam/DCPUToolchain/blob/master/contrib/sanitize-tree.sh. It effectively runs arbitrary commands over the staging index, but the same logic could easily be adapted for any non-working-directory linting. Unit testing is a little more difficult, but we won't be using that (every fix goes through a QA process which involves a VM being spun up for running the web services with a particular fix so that QA can test it).

The reason that we have dirty working directories is that we have developer-local configuration for the code; unfortunately this is being migrated from TFS where the actual deployment configurations are checked-in. We have to modify those files for our local development environment, but we don't want them to get sent as part of code reviews nor as parts of commits.

By commit copies I mean that it looks something like this:

a - b - c
       \ - d

Where c is the original head and d is the commit that "arc diff" creates. We're using the git-branch(master) workflo and it should be replicatable by doing "git push", "git stash", "arc diff" and "git stash pop". Commit d is an identical copy of commit c; although c is pointed to be origin/blah branch and d is put onto blah branch. I'm guessing most of the time you don't see this because c is not already pushed to a remote repository.

We'd like to track all branches (so they're all accessible via the web UI too). Is there a way to just list the branches to consider as being pushed?

I don't have an aversion to implementing this stuff myself, but I have no idea of the correct way to hook into all of these things (and my changes would end up just being hacks to get things working). For example, I'm not quite sure how different workflows are registered (is this part of the libphutil auto-include magic that goes on?) nor whether it's possible to have that pushed down from the server with 'arc liberate' automatically.

  1. That d branch should be coming off b.
  2. It seems I'm logged in with my personal email address and not my work email address (but both hach-que and jamesr are the same person)

Because lint can make changes and apply them to the working copy and/or HEAD, it's not a readonly operation we can run on some arbitrary commit (we could do git stash in arc, but we try to avoid ever leaving the working copy in a state the user doesn't understand if arc dies).

Phabricator and Facebook solved the "local configuration" problem by having .gitignore'd "local.conf" files; I believe this is the common approach. This also means "git status", "git add .", etc., will work correctly. If this is possible in your environment, it has worked well for us.

I think the c/d problem is because arc amends by default. You can set "history.immutable": true in your .arcconfig to disable this behavior (arc diff will no longer amend, and arc land will use --no-ff merges instead of squash merges).

You can leave "Track Only" empty (track everything) and set "Autoclose Only" to master if you want to track developer branches but not automatically close revisions pushed to them.

Actually with the script I linked, it could be adapted to run lint over the specified range and any changes to that range and be placed straight into the staging index, without touching the working directory at all. I think it would be perfectly fine to have "arc diff" in this case required a clean staging index, since in this workflow at least you're basing what you want diff'd on commits you've just made (and thus the staging index would be clean). I am unsure as to whether "git stash" actually stashes the index as well, since that again might be an alternative.

Unfortunately local configuration files aren't possible with ASP.NET's Web.config system and we can't ignore them since they're required to be correct for deployment (.gitignore'ing them would cause more trouble with people forgetting to commit changes required for production servers).

The history.immutable sounds like something we want to turn on though; we do want --no-ff merges anyway rather than commit squashing for "git land". The "Autoclose Only" option also sounds like what we want.

Unknown Object (User) added a comment.Feb 5 2013, 8:14 PM

What I've basically done is enabled immutable history and created a small wrapper script like so:

#!/bin/bash

if [ "$1" == "diff" ]; then
        git stash;
        arc $*;
        A=$?
        git stash pop;
        exit $A;
else
        echo "Must have 'diff' argument.";
fi

So then people can run this script as "arc-blah diff ..." to work.

epriestley triaged this task as Wishlist priority.May 23 2013, 6:27 PM
asherkin changed the visibility from "All Users" to "Public (No Login Required)".Jan 31 2014, 10:16 PM