Please update your install to a current version of Phabricator (we require all bug reports be against HEAD of stable or master).

Alternatively, you can use a test instance at Phacility to verify your reproduction steps against a current version.

That's the expected behavior - see https://secure.phabricator.com/book/phabricator/article/differential_faq/

arc land squashes all the revision's changes into a single commit, marks it with the revision's message, and pushes that; Herald then identifies the single commit as "reviewed as revision X", and accepts it.

@avivey Thank you for the explanation!

We would like all code being pushed to show up in the code review,. Our main concern with this behavior is that these additional commits can be pushed to develop with arc land without actually showing up in the code review that approved the land.

Obviously best practice to address this concern is to arc diff anytime between making a commit and using arc land, but people are not perfect.

Do you know if there is a different Herald Rule we could use or setting we could change that would enforce all the code being pushed showing up in the associated code review?

arc land mutates the code (adds details, squashes), so it's not possible. I'd recommend restricting who can land code and have the accepted reviewers manually land with "Land Revision" from the web ui. In general we expect people to trust co-workers, or use Audit for post-commit coverage.

I don't think there's any way to check for that via Herald. There's the Land Revision button( T182), which I ❤, but it's still a prototype and has some surprising behavior built into it (If you're doing anything complex, it can sometime land stuff you didn't want it to).

You can set history.immutable to true in your repository to prevent arc from performing mutation operations (amends, squash-merges).

If you set this option, arc will use a merge commit to merge instead of a squash-merge. But this won't really solve your problem.

Particularly, this will not guarantee that what lands is what was reviewed. Most often, users may need to resolve conflicts during the merge, which will cause additional changes to be contained in the merge commit. The conflict resolutions are not reviewed, and they are free to resolve the conflicts however they want, including by introducing entirely new code, and we can not prevent this (there is no automatic way to tell when a merge is faithful vs malicious). This will also prevent them from merging master to their branch first without getting re-review, because the merge commit will be a new commit which is not part of the revision.

If we did exactly what you ask more generally (prevent code from landing unless it is exactly identical to reviewed changes) users would need to re-review for every conflict, rebase, and merge, because rebasing and merging [almost] always introduces new code.

T182 discusses the idea of "chain of custody", which allows you to make this guarantee by having Phabricator perform merges. In this case, we know the merge is faithful because we perform it faithfully ourselves. This is the only way to guarantee that reviewed code and committed code are the same while still allowing merges and rebases (that is, guarantee that reviewed code and published code differ by only faithful merge/rebase actions).

This isn't a priority because installs generally have not been interested in it.

Thanks for the information @epriestley.

To be clear I am not requesting that users should have to re-review every conflict, rebase, and merge. We do not hold our users to that standard.

For example, if you want to push directly (not land) with the Herald Rule we are using, you just need to arc diff any new changes since the approval before pushing, but the code review does not need re-approval. I agree that that re-approval would be too arduous.

I think our solution is to proceduralize users to always arc diff before landing and have the reviewer close the review after looking over the changes since the last review iteration. This code will already have been pushed, of course, but at least they can make a ticket if they see any problems. We could also use audit more as @chad suggested.