Arc diff doesn't pick the right base commit and creates a new revision by mistake when working topic branch with merges
Closed, InvalidPublic
Actions

Assigned To

None

Authored By

	swisspol
	Feb 28 2017, 6:04 PM

Description

NOTE: Observed on Phacility and with very latest Arcanist on OS X.

Circumstances

I have a topic branch test branched off master
2 commits went on it and I called arc diff a couple times: the generated diffs are OK
I merged master into it (there was an intentional conflict which I solved)
I pushed the topic branch to the repo
I made another commit on the topic branch but didn't push (so the topic branch is 1 commit behind its remote ref)

Bug

When you call arc which, it indicates 2 problems:

It picks the wrong base commit to diff against: it says "07042b690e7af034 Merge branch 'master' into test" instead of using the branch point of the topic branch
It says a new revision will be created even though I'm still on the same topic branch which already has a revision

Repo topology

Output of `arc which`

$ arc which
REPOSITORY
To identify the repository associated with this working copy, arc followed this process:

    Configuration value "repository.callsign" is empty.

    This repository has no VCS UUID (this is normal for git/hg).

    The remote URI for this working copy is
    "git@github.com:lifeonairteam/test.git".

    Found a unique matching repository.

This working copy is associated with the Test repository.

COMMIT RANGE
If you run 'arc diff', changes between the commit:

    07042b690e7af034  Merge branch 'master' into test

...and the current working copy state will be sent to Differential, because
it is the merge-base of 'origin/test' (the Git upstream of the current
branch) HEAD.

You can see the exact changes that will be sent by running this command:

    $ git diff 07042b690e7af034..HEAD

These commits will be included in the diff:

    0131fbd6025dd3d1  Update


MATCHING REVISIONS
These Differential revisions match the changes in this working copy:

    (No revisions match.)

Since there are no revisions in Differential which match this working copy, a
new revision will be created if you run 'arc diff'.

Event Timeline

swisspol created this task.Feb 28 2017, 6:04 PM

Oh, it looks like pushing the topic branch when it was at the merge commit resulted in Diffusion auto-closing the Differential revision? This would explain the arc diff behavior reported above, but then the question becomes, why did it auto-close?

Clicking on "Explain Why" shows:

We didn't find a "Differential Revision" field in the commit message. This commit and the active diff of D127 had the same commit hash (70bfb4dd58c56a37296b00632113bd01d9facb83) so we linked this commit to D127.

Is this behavior clear after finding "Explain Why"? Which parts remain unclear?

I understand the "why" but not the "why of the why" if that makes sense? What this means is that revisions might be closed in what appears to be unpredictable to users if they happen to push their topic branches to the repo AND have merge commits on them (that's the issue no?).

By default, Phabricator assumes that git push means "publish", and that it is ONLY used to publish the final version of complete, reviewed changes. If you haven't already seen it, this post describes this in more detail:

https://secure.phabricator.com/phame/post/view/766/write_review_merge_publish_phabricator_review_workflow/

If you or your users sometimes use git push to mean "save changes", "make a backup", "share", etc., you can configure "Autoclose" in Diffusion. For example, you can configure only the "master" branch to Autoclose. If you configure Phabricator like this, it will never close revisions when changes are pushed to other branches.

The default configuration makes the assumption "when users push changes, they are always publishing them forever". It sounds like this isn't the right assumption in your environment. You should adjust "Autoclose" to list only branches where changes are considered to be truly published.

(Note that we may change this assumption soon because of the prevalence of workflows where users frequently push epriestley-tmp-3-bugfix2typo into the remote. These workflows do not scale well, and we've seen several larger organizations hit major scalability issues because they adopted these workflows before they grew, but they are sufficiently prevalent that the "push = publish" rule often defies expectations.)

The general "why of the why" is that if "git push = publish" is a valid assumption, and users "git push" a series of changes, they expect corresponding revisions to close -- even if they've rebased, amended, etc., those changes. We try very hard to find revisions to close when we discover new commits to meet that expectation.

In this case, it sounds like the revision contained a commit with hash 70bfb4dd58c56a37296b00632113bd01d9facb83, and a commit with that same hash was pushed to the remote, to an "Autoclose" branch. Since the commits match exactly and the assumption is "git push = publish", we close the associated revision.

If you or your users sometimes use git push to mean "save changes", "make a backup", "share", etc., you can configure "Autoclose" in Diffusion. For example, you can configure only the "master" branch to Autoclose. If you configure Phabricator like this, it will never close revisions when changes are pushed to other branches.

That sounds reasonable, thanks.

Arc diff doesn't pick the right base commit and creates a new revision by mistake when working topic branch with merges Closed, InvalidPublicActions