Page MenuHomePhabricator

Oddity with git branch/merge when using arc diff
Closed, ResolvedPublic

Description

I'm experiencing something with Differential and Git that seems odd to me, but I suspect is just down to my lack of understanding of how these interact.

I am using gitflow branching and am working on a feature branch (branched from develop). I create the branch and over the course or hours or days make a number of commits and publish/pushes to remote prior to being ready for the feature to be complete.
At the point I believe it is complete I push and then run arc diff develop, which kicks off a review of all the changes in the feature nicely. However at this point is when things are odd, while before I ran the diff git status showed everything good, after this git status shows that my local feature branch has diverged from the remote, 1 and 1 commit. I assume that this has something to do with what arc diff has done? This means I have to merge via pull and then push again which looks ugly in the branch view, but I can handle that if that is just life.
However, upon performing this merge and pushing to remote the code review is automatically closed even if the reviewer has never even seen it.
In an example when I make a single change, commit and push prior to running the arc diff, the review ends up being closed with 3 commits when there should only be one really:

rTODObb5905f72a95: changed license
rTODOfade9184c53c: changed license
rTODO8a0de3639143: Merge branch 'feature/test2' into develop

Now I'm sure I'm doing something in this process incorrectly, but it all seems like normal process for a developer that is working on a feature so I'm not sure what I've done wrong. Are you able to point me at it?
Also the revision history of the differential task is incorrect afterwards, showing no changes between 1st and last commits.

I've included a screenshot of the differential task page:

pasted_file (869×1 px, 150 KB)

Event Timeline

cpa199 raised the priority of this task from to Needs Triage.
cpa199 updated the task description. (Show Details)
cpa199 added a subscriber: cpa199.

The expectation in a Differential workflow is changes are reviewed before they are pushed to the remote branch. It sounds like you're pushing to the remote after each commit? What is your arc workflow precisely? A primer is here, but I suspect maybe I just misunderstanding your question.

https://secure.phabricator.com/book/phabricator/article/arcanist_diff/

I thought that may be the case which is why I made sure I mentioned my pushing and my full workflow. I did read all the articles first, but they weren't particularly clear on the fact that you must not push or it messes things up.
In reality a developer may work on a branch for a small number of days (or more, though not ideal it does happen) before wanting it reviewed and merged, if they can't push at all during that time then it means they are liable to lose their work it anything goes wrong on their machine (which is not as rare as it sounds). Are you saying that no-one can push their feature branch to remote without also reviewing it? If so am I missing something because that sounds like it might be a problem for anyone who wants to work on branch for more than a very short period.
It is also a problem (and possibly a more severe one) when more than one person work on a feature branch, which is a common occurrence and allowed for in gitflow.

Also my gitflow and arc workflow ideally is:

git flow feature start <name>
do some work
commit
git flow feature publish <name> (this pushes and allows others to also work on this branch)
do more work
commit
git push (the publish set the branch to track)
do more work
commit
git feature review (this actually just calls arc diff develop)
review is done
do any changes I need to from review
git feature finish <name> (this does an arc amend as part of the process)

Differential (arc diff) is for pre-commit (or push) review. If you wish to push to a remote, then have it reviewed later, you can either use Audit or run arc diff before the branch is merged to master.

When you say "before the branch is merged" to master, do you mean to do the merge, then arc diff that, then push the merge once done?

And thanks for your help by the way, it is much appreciated and it sounds like there is a way to do what I want, I just misunderstood the process and differential a little.

I have @epriestley land branches, so I've never done it personally, but I believe what you state is correct.

I'll try it out and update here with results so if anyone else happens upon this then there is hopefully an answer.

If you like having "unfinished work" branches in your repository, you can set the Phabricator to only consider some branches for closing of revisions: Repository Edit Edit Branches Autoclose Only.

With Autoclose set to "master" (or "develop", in your case), only commits that make it to master will attempt to close revisions.

The "diverged... 1 and 1 commits" you see is just arc diff amending the last commit message, so it can find the revision next time you run arc diff. You can maybe setup "git feature review" to add an (empty?) commit just for that, or something (Maybe have "git flow feature publish" create the initial diff, amending the first commit?).

Thanks, I'll take a look into doing those things too, especially the autoclose settings. Hopefully I'll be able to try it all out tomorrow morning.

I should also note that if you skip pushing and just arc diff your code is saved up to your Phabricator instance (available via arc patch, so if that's the only reason for your workflow, it's not so much an issue.

epriestley claimed this task.

Yeah, @chad / @avivey have this pretty much covered.

  • By default, we assume that code which appears in the remote is final/published/reviewed/done/complete, not working/draft/in-progress. You can use "Autoclose Only" or the adjacent "Track Only" setting to ignore some branches, or import them but not close objects in response to commits.
  • By default, we use a mutable-history workflow in git, which involves amending commits and performing rebases. You can set history.immutable in your .arcconfig or local settings to make arc avoid history mutation operations. See https://secure.phabricator.com/book/phabricator/article/arcanist_new_project/#advanced-arcconfig
  • We encourage pre-publish/pre-push code review (see https://secure.phabricator.com/book/phabricator/article/reviews_vs_audit/) and the defaults generally align well with a pre-publish review workflow. If you don't want to use this workflow, you can adjust the settings mentioned above.

Thank you all for the very helpful comments, sounds like it'll help me do what I need.

So the change to autoclose worked nicely, thanks. However I'd like to be able to use arc diff and arc diff --update, but that means I need to use mutable history it looks (otherwise it can't update the original diff, with an odd error message too, something like can't edit and emtpy diff rather than saying it's because immutable history is turned on). The problem with this is if I use mutable history it automatically assumes I am squashing the commits, and if I don't do that the diff displays are totally incorrectly and I don't want to enforce squashing of commits.

From what I've seen I assume there no way to update revisions (arc diff --update) with new commits if I use immutable history, so every time a review requires changes, I have to raise a new review for the changes and close the original one and just link them via a comment or something? Is this correct?

Alternatively is there a way to get the mutable version to work when commits aren't squashed and to treat commits as sequential? If you just do this anyway the diff displays go very wrong. I'm assuming that there isn't from what I've seen.