<div dir="ltr">On 22 November 2017 at 19:15, Ben Gamari <span dir="ltr"><<a href="mailto:ben@well-typed.com" target="_blank">ben@well-typed.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">Simon Marlow <<a href="mailto:marlowsd@gmail.com">marlowsd@gmail.com</a>> writes:<br>
<br>
> On 22 November 2017 at 15:31, Ben Gamari <<a href="mailto:ben@well-typed.com">ben@well-typed.com</a>> wrote:<br>
><br>
>> Manuel M T Chakravarty <<a href="mailto:manuel.chakravarty@tweag.io">manuel.chakravarty@tweag.io</a>> writes:<br>
>> > Why do we need the intermediate builds exactly? Wouldn’t they usually<br>
>> > fail? (When I do PRs with multiple commits, the state of the tree<br>
>> > between this commits will usually not be well-defined.)<br>
>><br>
>> No, every commit should build. This is in part a difference between<br>
>> Phabricator's patch-based model and GitHub's feature branch model.<br>
>> However, many projects using the latter also demand that all<br>
>> intermediate commits must be atomic, buildable changes. Sacrificing this<br>
>> property greatly complicates bisection.<br>
>><br>
>> Building all intermediates is desireable as ultimately we would like to<br>
>> preserve per-commit build artifacts for the last few months of commits<br>
>> to enable easy bisection.<br>
>><br>
><br>
> I don't quite understand this. Yes building all commits is desirable, but<br>
> in the case of Phabricator each revision is going to be a single commit,<br>
> no? So why is this an issue? Or is it an issue only for github PRs?<br>
<br>
</span>The problem is that many contributors, including Simon PJ, Richard, and<br>
me, tend to push batches of work. For instance, when I land<br>
contributors' differentials I first apply a batch, then validate<br>
locally, and then push as a chunk. We can change this if necessary, but<br>
it will need to be via social convention which hasn't worked very well<br>
historically.<span class=""><br></span></blockquote><div><br></div><div>Ah, I see, I thought the problem was just to do with branches. Thanks for the clarification.<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
> I thought we had decided (somewhere, I forget where) that we were going to<br>
> require PRs to be squashed before pushing?<br>
><br>
</span>Please correct me if I'm forgetting something but I do not believe we<br>
have even decided that we would start accepting PRs for larger patches.<br>
I had said during ICFP that I was open to the idea, but experience<br>
with GitHub's reviewing tools has since led me to view the proposal with<br>
a bit more skepticism. Since this was prior to the creation of this<br>
mailing list I summarised these concerns in a private email to Manuel; I<br>
will forward this message to the list in a moment.<br></blockquote><div><br></div><div>Right, we certainly haven't decided to move code reviewing to GitHub (and I also have deep concerns about that). I was thinking about the case where someone submits a PR via GitHub and we convert it into a Phabricator diff, squashing in the process, or if the PR doesn't require reviewing then we squash before pushing.</div><div><br></div><div>Cheers</div><div>Simon<br></div><div> </div></div></div></div>