<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Jan 16, 2020 at 10:30 AM David Greene via cfe-dev <<a href="mailto:cfe-dev@lists.llvm.org">cfe-dev@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Joerg Sonnenberger via cfe-dev <<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a>> writes:<br>
<br>
> One typical case for a patch series is if you need infrastructure in a<br>
> number of places in place first. Sending all changes at once allow<br>
> others to see where you are going, independent of whether the individual<br>
> pieces are acceptable immediately or not. Yes, this can mean later<br>
> patches need to be reworked for larger structural changes, but that<br>
> doesn't necessarily invalidate review remarks already made.<br>
<br>
I can certainly see the utility of this. I've never tried it since<br>
posting patches via the web interface is not at all convenient</blockquote><div><br>Perhaps this is covered elsewhere but I'm still not super clear on "not at all convenient" - there's a form you fill in and attach the patch file. <br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> and as<br>
I've mentioned before I can't get arcanist to work. Does arcanist take<br>
care of organizing the series?<br></blockquote><div><br>(I haven't made patch series myself)<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">How do reviews work? Does each patch get a separate review page like<br>
any other patch?<br></blockquote><div><br>Yes, see <a href="https://reviews.llvm.org/D69785">https://reviews.llvm.org/D69785</a> and <a href="https://reviews.llvm.org/D69922">https://reviews.llvm.org/D69922</a> as an example of two patches in a series.<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Let's say patch 3 in a series is approved and it could be committed<br>
without patches 1 and 2 (like Renato I question why this is a patch<br>
series if so, but let's assume it for argument's sake). That means I<br>
need to rebase -i my branch in order to get patch 3 committed, right?<br></blockquote><div><br>If patch 3 can be committed without 1 and 2, it's not a series. It's a bunch of related reviews I'd do on separate branches - and I imght send them out at the same time to illustrate the broader design - but they're still effectively 3 separate reviews.<br><br>If they're a series they have ordering and having tools to help review them separately while understanding that one patch is built on top of the other is useful.<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Like Renato, I actually find that more burdensome than just putting<br>
patch 3 in its own branch in the first place and doing a separate review<br>
of it. In other words, my definition of "patch series" is the same as<br>
Renato's: a linear series of commits each of which is dependent on the<br>
previous commits. With that definition, "early approval" of later<br>
patches really doesn't help get things committed earlier.<br></blockquote><div><br>Yeah, I don't /think/ anyone is using "series" To describe a set of independent but related patches. I think everyone would send those out as separate patches not in a single branch of stacked commits on git, or using phab's stack handling functionality.<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">I definitely see the advantage of not invalidating existing review<br>
comments if the series is rebased/changed. I confess I don't know how<br>
GitHub handles rebases (see my other post with questions about that).<br>
<br>
> The other case is reworking a pass to handle a couple of similar, but not<br>
> identical cases. It might not be possible to take out patch 2 in a<br>
> series in this case, but most often the review changes here are smaller<br>
> "stylistic" issues without huge impact on later steps.<br>
<br>
I'm sorry I don't really follow this. How does it differ from the first<br>
case? Are you saying you're grouping two similar but basically<br>
independent changes together? I would think doing so violates the "one<br>
topic per review" principle (that's maybe not an LLVM policy per se,<br>
just the way I think to think about things).<br></blockquote><div><br>So the first scenario Joerg was talking about was what I think almost everyone in this thread are talking about when they say "series" - dependent patches, built on top of each other, where later patches cannot be committed without earlier patches. Usually because they are all in service of some singular goal - but broken into incremental changes that we all know and love.<br><br>This second scenario is one in which the dependence is smaller - you want to make 3 changes to an optimization, each one doesn't require the others - but the code is close enough together that they touch the same bits of code. If the overlap is small enough - you could send these as 3 separate/unrelated reviews/patches - and whichever gets committed first you rebase your other two outstanding ones. But sometimes they might be a bit too connected ("might not be possible to take out patch 2" - that's what makes it a series, not 3 independent patches - because the API overlap is too close, so your 3 patches that are sort of independent, but overlap with each other such that they are ordered - you'd like to get them all reviewed, so you send them all out and use parent/child relationships so reviewers realize they are looking at increments that aren't from top of tree (because one patch is based on the incidental/nearby API changes caused by the earlier patch in the series))<br><br>It's still essentially a dependent patch series - just a more loosely connected one. 3 independent goals, but so close that they're dependent. Rather than 3 incremental steps towards 1 core goal.<br><br>Multiple cases where dependent patch series are useful/might be used.<br><br>- Dave<br><br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
-David<br>
_______________________________________________<br>
cfe-dev mailing list<br>
<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev</a><br>
</blockquote></div></div>