<div dir="ltr">On Wed, Jun 6, 2018 at 9:40 PM, Eric Rescorla <span dir="ltr"><<a href="mailto:ekr@rtfm.com" target="_blank">ekr@rtfm.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><div><div class="gmail-h5">On Wed, Jun 6, 2018 at 9:27 PM, Cameron McCormack <span dir="ltr"><<a href="mailto:cam@mcc.id.au" target="_blank">cam@mcc.id.au</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><u></u>
<div><span><div>On Thu, Jun 7, 2018, at 2:15 PM, Eric Rescorla wrote:<br></div>
<blockquote type="cite"><div dir="ltr"><div>So, say for the sake of argument you are working for some feature, and<br></div>
<div>the git history consists of:<br></div>
<div><br></div>
<div> origin/master -> A1 -> A2 -> A3<br></div>
<div><br></div>
<div>When you do |arc diff|, this will create a single Revision (say D1)<br></div>
<div>which contains all of A1, A2, A3.<br></div>
</div>
</blockquote><div><br></div>
</span><div>Are A1, A2, and A3 all separately viewable and reviewable in Phabricator when I open D1, or do they get squashed down to a single (let's call it) patch? Can I give say r+ to A1 and r- to A2 and A3?<br></div></div></blockquote><div><br></div></div></div><div>They're basically squashed down to a single patch [0]. You can't review them independently. However, you can do interdiffs between the various Diffs (i.e., versions) of a Revision.</div><div><br></div><div>To be a little more concrete about it, In my work I usually end up with a git history that looks like:</div><div><br></div><div>- Checkpoint: function written but doesn't compile</div><div>- Checkpoint: compiles</div><div>- Checkpoint: most tests pass</div><div>- Checkpoint: OK, all tests pass now</div><div>- Clang-formatted</div><div><br></div><div>These are all part of the same conceptual unit and I don't want them independently reviewed, so in Phabricator you'd just make those one Diff (which happens on its own without squashing them down to a single commit).</div><div><br></div><div><br></div><div>And then once it's been reviewed, my git history would look like:</div><div><br></div><div><div>- Checkpoint: function written but doesn't compile</div><div>- Checkpoint: compiles</div><div>- Checkpoint: most tests pass</div><div>- Checkpoint: OK, all tests pass now</div><div>- Clang-formatted</div>- Responses to review comments <- New<br></div><div>- Re-clang-formatted <- New</div><div><br></div><div>And when I updated the revision, Phabricator would have two diffs, corresponding to:</div><div><div><br></div><div><div>- Checkpoint: function written but doesn't compile</div><div>- Checkpoint: compiles</div><div>- Checkpoint: most tests pass</div><div>- Checkpoint: OK, all tests pass now</div><div>- Clang-formatted <wbr> <- R01<br></div>- Responses to review comments <br></div><div>- Re-clang-formatted <wbr> <- R02</div><div><br></div>And so the views it gives you are: base -> R01, base->R02, R01 -> R02</div></div></div></div></blockquote><div><br></div><div>This is essentially correct: when you submit a series of commits with `arc diff` using the normal settings, it squashes everything to a single Phabricator revision. It does upload the information about the squashed commits, so you can see things like commit messages in the Phabricator UI. However, it loses the diffs from individual commits.</div><div><br></div><div>The MozReview model is to preserve your local commits as separate reviewable entities in ReviewBoard. There is little to no data loss.</div><div><br></div><div>The GitHub pull request model does preserve the original commits. However, the UI encourages you to operate against the overall diff. It is possible to leave reviews on individual commits. But the UX is lacking. When you do things like force pushes, inline comments can lose their context and other wonkiness occurs. The experience is poor enough that most people steer clear of reviewing individual commits and focus on the overall diff.</div><div><br></div><div>In Phabricator, you can link up reviewable revisions to each other. They then show up in the "Stack" view of Phabricator. See e.g. <a href="https://phabricator.services.mozilla.com/D1575">https://phabricator.services.mozilla.com/D1575</a>. This allows you to get the MozReview behavior which preserves a series of commits.<br></div><div><br></div><div>However, unlike MozReview or GitHub, Phabricator doesn't have a strong concept of a "series" (merely loosely linked revisions constituting a "stack") so you can't see a diff over multiple revisions if you submit reviews this way. This sounds scary since it is a loss of functionality. But since we generally review commits individually, I don't think it is that severe of a regression. If you really need to look at a diff over multiple revisions, you can always import the patches into your local repository and use `hg diff` or `git diff` to construct arbitrary diffs.</div><div><br></div><div>I think the concern voiced by various people on this thread is that the default behavior of `arc diff` collapsing commits doesn't jive with our workflow of "the commit that is authored is the commit that is reviewed is the commit that lands." It changes the model to "the commits that are authored are collapsed into something that is reviewed and that something is landed." That *is* a substantial departure from the status quo - especially since many of us like to author more, smaller, easier-to-review-in-isolation commits with descriptive commit messages and collapsing those commits undermines that workflow. It is possible to achieve the MozReview behavior with Phabricator. But the end-user UX for submitting revisions this way isn't the default nor is it a simple one-liner like `hg push review`. This is certainly a solvable problem - either via a new feature of `arc diff` (which I believe mcote said is being discussed with upstream) or a custom tool or wrapper for submitting to Phabricator.</div><div><br></div><div>Of course, if all you ever do is author single commits in isolation, then all this discussion about a series of commits is irrelevant to you. I believe a significant percentage of Firefox developers do things this way.<br></div><div><br></div><div>I hope this summary is helpful.<br></div></div></div></div>