35

Here is the scenario that is pretty much annoying me.

Jack works in foobar a software house, Jack is a Working Programmer, he loves coding and commits frequently. Paul who is jack's manager tells him that we are going to start using a new code review tool, phabricator. Jack complies, Jack creates a local branch and starts working. He adds features and commits to his local branch very frequently. Now at the end of the day he sends a phabricator request.

arc diff development

John who is jacks team member reviews his code and accepts his changes. Now Jack opens the terminal and moves to his repository directory. Jack types the following command to close the revision and merge his code with the development branch.

arc land --onto development

He sees the following message

Landing current branch 'feature-awesome-features'.
Switched to branch development. Updating branch...
The following commit(s) will be landed:

b2ff76e  Added the foo to bar
33f33ba  Added a really important check which can destroy the project or save it
31a4c9a Added that new awesome feature
8cae3bf rewrote that awful code john wrote
bc54afb  bug fixes

Switched to branch feature-awesome-features. Identifying and merging...
Landing revision 'D1067: Added the awesome feature'...
Rebasing feature-awesome-features onto development
Already up-to-date.
Pushing change...

Now jack opens Github to see his code , his beautiful commits. but what he sees is pure horror all of his commits have been replaced by a single commit which basically says something like this

Summary: Added the awesome feature

Test Plan:  do foo bar testing

Reviewers: John

Reviewed By: John

CC: Paul

Differential Revision: http://phabricator.foobar.com/D1067

Now jack is sad, because he wants to see all of his commits, jack thinks that this commit makes him look like The Hoarder which he is not. He wants to fix this so he goes to ask a question on stackoverflow.

That how may he prevent phabricator from eating his commit history.
Matthias
  • 133
  • 2
  • 10
xeon111
  • 1,025
  • 9
  • 20
  • If that's how everyone's merges look, why do you think you will be singled out? Anyway, the workaround is to have more smaller branches. In particular, don't commit unrelated changes on feature branches. – tripleee Dec 24 '13 at 07:12

7 Answers7

20

asherkin's answer explains the rationale for this behavior, and why this is the default.

If you don't find that argument compelling, you can use the --merge flag to arc land to perform --no-ff merges instead of --squash merges. These merges will not destroy local commits.

If you set history.immutable to true in your .arcconfig, arc land will --no-ff merge by default.

You can also use raw git commands if you don't like the behavior of arc land; it is provided only for convenience.

In your example, we recommend creating five separate reviews -- there are multiple different ideas being implemented, and they are not related and seem easily separable. See Writing Reviewable Code. Combining bugfixes, style changes and new features into one change is hoarding.

Thomas
  • 3,348
  • 4
  • 35
  • 49
Evan Priestley
  • 3,287
  • 23
  • 16
  • 4
    When I write code, I generally have a single task I want to accomplish it and I separate it into smaller tasks that preserve the correctness of the codebase, do exactly one thing codebase-wise, but don't necessary change functionality in any meaningful way in a single commit. I expect that the acceptance of the commitset is atomic which isn't possible under the broken 1-idea=1-commit that *the developers of git themselves don't use*. Five separate reviews implies that they are five separate branches that are mutually interchangable when this is not the case in the typical patch series. – alternative Jun 06 '15 at 22:17
10

You should use the native git flow such as git merge and git push directly instead. From phabricator arc documentation:

After changes have been accepted, you generally push them and close the revision. arc has several workflows which help with this, by:

* squashing or merging changes from a feature branch into a master branch
* formatting a good commit message with all the information from Differential
* and automatically closing the revision.

You don't need to use any of these workflows: you can just run git push, hg push or svn commit and then manually close the revision from the web.

arc is squashing your commits on purpose.

Avi Flax
  • 50,872
  • 9
  • 47
  • 64
mockinterface
  • 14,452
  • 5
  • 28
  • 49
  • 5
    Because screw documentation, right? How such a humongous brain-fart ever made it into production code is deeply worrying. "Then let's just stick to normal Git-style pull requests?", one might say. Nope, can't be done using Phabricator. A module is on its way, but it is still in beta. Git's whole "Commit early and commit often" matra goes right out the window... – Kafoso Oct 05 '16 at 14:55
4

history.immutable: Configures arc to use workflows which never rewrite history in the working copy. By default, arc will perform some rewriting of unpublished history (amending commit messages, squash merging) on some workflows in Git. The distinctions are covered in detail below.

so just add line in your .arcconfig

"history.immutable": true
Phiter
  • 14,570
  • 14
  • 50
  • 84
Vyachesla D.
  • 41
  • 3
3

There is some documentation that explains why this is the default setup for arc land.

A strategy where one idea is one commit has no real advantage over any other strategy until your repository hits a velocity where it becomes critical. In particular:

  • Essentially all operations against the master/remote repository are about ideas, not commits. When one idea is many commits, everything you do is more complicated because you need to figure out which commits represent an idea ("the foo widget is broken, what do I need to revert?") or what idea is ultimately represented by a commit ("commit af3291029 makes no sense, what goal is this change trying to accomplish?").
  • Release engineering is greatly simplified. Release engineers can pick or drop ideas easily when each idea corresponds to one commit. When an idea is several commits, it becomes easier to accidentally pick or drop half of an idea and end up in a state which is virtually guaranteed to be wrong.
  • Automated testing is greatly simplified. If each idea is one commit, you can run automated tests against every commit and test failures indicate a serious problem. If each idea is many commits, most of those commits represent a known broken state of the codebase (e.g., a checkpoint with a syntax error which was fixed in the next checkpoint, or with a half-implemented idea).
  • Understanding changes is greatly simplified. You can bisect to a break and identify the entire idea trivially, without fishing forward and backward in the log to identify the extents of the idea. And you can be confident in what you need to revert to remove the entire idea.
  • There is no clear value in having checkpoint commits (some of which are guaranteed to be known broken versions of the repository) persist into the remote. Consider a theoretical VCS which automatically creates a checkpoint commit for every keystroke. This VCS would obviously be unusable. But many checkpoint commits aren't much different, and conceptually represent some relatively arbitrary point in the sequence of keystrokes that went into writing a larger idea. Get rid of them or create an abstraction layer (merge commits) which allows you to ignore them when you are trying to understand the repository in terms of ideas (which is almost always).

All of these become problems only at scale. Facebook pushes dozens of ideas every day and thousands on a weekly basis, and could not do this (at least, not without more people or more errors) without choosing a repository strategy where one idea is one commit.

For git, the recommended solution is to push feature branches into the upstream, then merge rather than rebase onto master - if you agree with the points above but still want your checkpoint commits in the upstream.

If this is only for during review (as your question is worded - but it also sounds like you're doing no code review or only post-push code review with Audit), note that Differential will already display all your local commits with the original commit message for the reviewer to see.

arc land is designed to only be used to push to the "final" branch of a repository (i.e. production or some branch representing changes pending release). If you're doing post-push commit review (i.e. on merges from development to master), you can just bypass arc land (and it is generally accepted that you need to in many cases) and git push your changes directly.

asherkin
  • 377
  • 5
  • 9
  • 5
    To squash commits is the opposite of "one idea per commit". It forms "mega commits" that can't be reverted, passed around or discussed easily. It seriously hampers git bisect and git blame. Use proper branches if you want to collect multiple commits into a single merge point. – Alexander Ljungberg Mar 03 '14 at 14:38
  • 1
    All of what you've said implies that you're squashing far too much together. Squashing checkpoint commits is most certainly not the opposite of "one idea per commit". Please read the linked source if you want more context. – asherkin Mar 03 '14 at 19:09
  • 1
    Okay I see now that the linked article defines "one idea" in a way that I would describe as "many ideas". I stand by this: one commit should be one specific, easily describable change with a tight purpose. A string of commits can be a part of a grander scheme, a "feature", and that's a branch. – Alexander Ljungberg Mar 03 '14 at 21:50
  • 1
    What the hell is a checkpoint commit? Do people actually use that? When I write code, I split it into several commits as a patchset. It is a fundamental flaw that a single differential revision in Phabricator only represents a single diff, when what I really want is to show all diffs, in order, and have them be reviewed atomically. This is the workflow git itself uses - see git format-patch - and anything that diverges from this is *wrong*. – alternative Jun 06 '15 at 22:12
1

The modern answer to this question after some of the changes in the most recent updates to arcanist

In order to prevent the squash you have to use the new strategy syntax

arc land Dxyz --strategy merge

The old syntax of --merge or --squash has been replaced by --strategy

( See some background info in : https://secure.phabricator.com/T13547 )

Eric
  • 28
  • 4
0

--merge doesn't work as it creates a merge commit. I'v patched mine to have a --rebase that will do the right thing but I'm unsure if arc people would like to have this.

Tomaz Canabrava
  • 2,320
  • 15
  • 20
0

Here is what I do. It doesn't prevent the commit history from being squashed, but it allows you to rebase subsequent features onto the branches that were lost.

$ arc land --revision <feature1>

$ git branch -u master <feature2>

$ arc cascade

arc cascade will rebase all feature branches that were branched off of feature1

Miguel Coder
  • 1,896
  • 19
  • 36