CODEWITHSUNDEEP
gitmergephabricator
ugoren
ugoren

Reputation: 16441

Phabricator - How to Review a Merge

We have two branches, and I want to merge the tip of one onto the other. Like any code change, this needs to be reviewed in Phabricator/Differential.

It all seems like very normal workflow to me, but I can't figure out how to get it done.

The Merge We Do

We have two branches - master and beta. Most development is done on master. Occasionally, we merge from master to beta. With plain git, we would do this:

% git checkout beta
% git merge master
% git commit
% git push

But with Phabricator, we can't just do git push (and don't want to - code review is important).

Problem with arc diff

I do the merge as above, but replace git push with arc diff.

First, the message it offers to use is the commit message of one of the commits I've merged. This isn't a good message, so I replace it with merge master into beta.

Next, I get this message:

You don't own revision D97: "Some change".
Normally, you should only update revisions you own. You can "Commandeer"
this revision from the web interface if you want to become the owner.

Update this revision anyway? [y/N]

Revision D97 is a change which was already landed into master. I certainly don't want to commandeer it. I answer no and arc diff exits.

Running arc diff --create behaves the same.

Problem with arc land

I can use arc diff --preview to create a diff, and then a revision from the web UI. I don't know if it's a good idea, but I can proceed with the review.

Afrer the patch is approved, arc land squashes all changes into one commit. For normal development, I agree with the rationale - I want one change to enter as one commit.

But here the result is awful - there's no record of a merge between master and beta. It seems as if all master's changes were rewritten over beta. Future merges will suffer because the merge isn't recorded.

Upvotes: 1

Views: 3553

Answers (2)

ugoren
ugoren

Reputation: 16441

I have a method that works, end to end. It's quite annoying.

  1. Merge in git - a normal merge.
  2. Create diff with arc diff --preview.
  3. Go to the link printed by the last command and create a revision based on the diff.
  4. A reviewer can now review the change. To review properly, he'll need to run arc patch and compare the various related versions (source branch, target branch, merge result, common ancestor).
  5. Land the diff with arc land --merge --revision D123. --merge (as suggested in JSON's answer prevents squashing the commits, --revision prevents confusion with the revisions used to land the merged content.

I didn't check what happens if the reviewer requests changes, and another arc diff is needed before I can land.

Upvotes: 1

JSON
JSON

Reputation: 4606

You seem to have found a workaround for the arc diff step, and I can't see a better one unfortunately.

arc land --merge will land the changes as a merge instead of squash.

Upvotes: 1

Related Questions