| Message ID | 20240514123039.1029081-1-knayak@gitlab.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | SubmittingPatches: add section for iterating patches | expand |
On Tue, May 14, 2024 at 02:30:39PM +0200, Karthik Nayak wrote: > From: Karthik Nayak <karthik.188@gmail.com> > > Add a section to explain how to work around other in-flight patches and > how to navigate conflicts which arise as a series is being iterated. > This will provide the necessary steps that users can follow to reduce s/This will/This provides/ > friction with other ongoing topics and also provides guidelines on how > the users can also communicate this to the list efficiently. > > Co-authored-by: Junio C Hamano <gitster@pobox.com> > Suggested-by: Junio C Hamano <gitster@pobox.com> > Signed-off-by: Karthik Nayak <karthik.188@gmail.com> > --- > > This came off a discussion wherein I sent a series based on `next` > instead of merging in conflicts [1]. This is mostly worded by Junio and > I've just put it together into a patch. > > This is based off master, with 'jc/patch-flow-updates' merged in. :) > > [1]: https://lore.kernel.org/git/xmqqy18lpoqg.fsf@gitster.g/ > > Documentation/SubmittingPatches | 79 +++++++++++++++++++++++++++++++++ > 1 file changed, 79 insertions(+) > > diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches > index 8332073e27..2fd94dc8de 100644 > --- a/Documentation/SubmittingPatches > +++ b/Documentation/SubmittingPatches > @@ -608,6 +608,85 @@ patch, format it as "multipart/signed", not a text/plain message > that starts with `-----BEGIN PGP SIGNED MESSAGE-----`. That is > not a text/plain, it's something else. > > +=== Handling Conflicts and Iterating Patches > + > +When revising changes made to your patches, it's important to > +acknowledge the possibility of conflicts with other ongoing topics. To > +navigate these potential conflicts effectively, follow the recommended > +steps outlined below: Okay. I was first wondering why we only mention conflicts when revising changes. But I see there are other parts in the document where we already mention the potential for conflicts, so this is fine. > +. Build on a suitable base branch, see the <<choose-starting-point, section above>>, > +and format-patch the series. If you are doing "rebase -i" in-place to > +update from the previous round, this will reuse the previous base so > +(2) and (3) may become trivial. > + > +. Find the base of where the last round was queued It's somewhat unusual for bulleted lists to start with a dot, but this is consistent with the remainder of this document. [snip] > +Do not forget to write in the cover letter you did this, including the > +topics you have in your base on top of 'master'. Then go to (4). > + > +. Make a trial merge of your topic into 'next' and 'seen', e.g. > ++ > + $ git checkout --detach 'origin/seen' && > + $ git revert -m 1 <the merge of the previous iteration into seen> && > + $ git merge kn/ref-transaction-symref Let's remove the trailing '&&' lines. The leading dollar indicates that this is interactive, so you wouldn't concatenate the commands like this. Also, preceding code didn't have it. > +The "revert" is needed if the previous iteration of your topic is > +already in 'seen' (like in this case). You could choose to rebuild > +master..origin/seen from scratch while excluding your previous > +iteration, which may emulate what happens on the maintainers end more > +closely. > ++ > +This trial merge may conflict. It is primarily to see what conflicts > +_other_ topics may have with your topic. In other words, you do not > +have to depend on to make your topic work on 'master'. It may become I think there's either a word too many or missing -- depend on what? > +the job of the other topic owners to resolve conflicts if your topic > +goes to 'next' before theirs. > ++ > +Make a note on what conflict you saw in the cover letter. You do not > +necessarily have to resolve them, but it would be a good opportunity to > +learn what others are doing in an related area. s/an/a > ++ > + $ git checkout --detach 'origin/next' && > + $ git merge kn/ref-transaction-symref Same comment here regarding the ampersands. Other than that the additions look good to me, thanks! Patrick
Patrick Steinhardt <ps@pks.im> writes: > On Tue, May 14, 2024 at 02:30:39PM +0200, Karthik Nayak wrote: >> From: Karthik Nayak <karthik.188@gmail.com> >> >> Add a section to explain how to work around other in-flight patches and >> how to navigate conflicts which arise as a series is being iterated. >> This will provide the necessary steps that users can follow to reduce > > s/This will/This provides/ > Thanks, will change! >> friction with other ongoing topics and also provides guidelines on how >> the users can also communicate this to the list efficiently. >> >> Co-authored-by: Junio C Hamano <gitster@pobox.com> >> Suggested-by: Junio C Hamano <gitster@pobox.com> >> Signed-off-by: Karthik Nayak <karthik.188@gmail.com> >> --- >> >> This came off a discussion wherein I sent a series based on `next` >> instead of merging in conflicts [1]. This is mostly worded by Junio and >> I've just put it together into a patch. >> >> This is based off master, with 'jc/patch-flow-updates' merged in. > > :) > >> >> [1]: https://lore.kernel.org/git/xmqqy18lpoqg.fsf@gitster.g/ >> >> Documentation/SubmittingPatches | 79 +++++++++++++++++++++++++++++++++ >> 1 file changed, 79 insertions(+) >> >> diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches >> index 8332073e27..2fd94dc8de 100644 >> --- a/Documentation/SubmittingPatches >> +++ b/Documentation/SubmittingPatches >> @@ -608,6 +608,85 @@ patch, format it as "multipart/signed", not a text/plain message >> that starts with `-----BEGIN PGP SIGNED MESSAGE-----`. That is >> not a text/plain, it's something else. >> >> +=== Handling Conflicts and Iterating Patches >> + >> +When revising changes made to your patches, it's important to >> +acknowledge the possibility of conflicts with other ongoing topics. To >> +navigate these potential conflicts effectively, follow the recommended >> +steps outlined below: > > Okay. I was first wondering why we only mention conflicts when revising > changes. But I see there are other parts in the document where we > already mention the potential for conflicts, so this is fine. > >> +. Build on a suitable base branch, see the <<choose-starting-point, section above>>, >> +and format-patch the series. If you are doing "rebase -i" in-place to >> +update from the previous round, this will reuse the previous base so >> +(2) and (3) may become trivial. >> + >> +. Find the base of where the last round was queued > > It's somewhat unusual for bulleted lists to start with a dot, but this > is consistent with the remainder of this document. > Yeah, that's mostly why I added dots instead of asterisks here. > > [snip] >> +Do not forget to write in the cover letter you did this, including the >> +topics you have in your base on top of 'master'. Then go to (4). >> + >> +. Make a trial merge of your topic into 'next' and 'seen', e.g. >> ++ >> + $ git checkout --detach 'origin/seen' && >> + $ git revert -m 1 <the merge of the previous iteration into seen> && >> + $ git merge kn/ref-transaction-symref > > Let's remove the trailing '&&' lines. The leading dollar indicates that > this is interactive, so you wouldn't concatenate the commands like this. > Also, preceding code didn't have it. > Yeah, agreed, I will remove here and below >> +The "revert" is needed if the previous iteration of your topic is >> +already in 'seen' (like in this case). You could choose to rebuild >> +master..origin/seen from scratch while excluding your previous >> +iteration, which may emulate what happens on the maintainers end more >> +closely. >> ++ >> +This trial merge may conflict. It is primarily to see what conflicts >> +_other_ topics may have with your topic. In other words, you do not >> +have to depend on to make your topic work on 'master'. It may become > > I think there's either a word too many or missing -- depend on what? > 's/depend on/depend on it/' should do I think. >> +the job of the other topic owners to resolve conflicts if your topic >> +goes to 'next' before theirs. >> ++ >> +Make a note on what conflict you saw in the cover letter. You do not >> +necessarily have to resolve them, but it would be a good opportunity to >> +learn what others are doing in an related area. > > s/an/a > I think it makes sense to make it 's/an related area/related areas/'. >> ++ >> + $ git checkout --detach 'origin/next' && >> + $ git merge kn/ref-transaction-symref > > Same comment here regarding the ampersands. > > Other than that the additions look good to me, thanks! > > Patrick Thanks for the review. Will send a follow up version!
Karthik Nayak <karthik.188@gmail.com> writes: >>> +. Build on a suitable base branch, see the <<choose-starting-point, section above>>, >>> +and format-patch the series. If you are doing "rebase -i" in-place to >>> +update from the previous round, this will reuse the previous base so >>> +(2) and (3) may become trivial. >>> + >>> +. Find the base of where the last round was queued >> >> It's somewhat unusual for bulleted lists to start with a dot, but this >> is consistent with the remainder of this document. > > Yeah, that's mostly why I added dots instead of asterisks here. Here we want a list of numbered items so that we can refer to other items in the same list like "(2) and (3) may become trivial". Wouldn't asterisks give us an unordered list that is typically rendered as a list of unordered items instead?
On Fri, May 17, 2024 at 10:23:40AM -0700, Junio C Hamano wrote: > Karthik Nayak <karthik.188@gmail.com> writes: > > >>> +. Build on a suitable base branch, see the <<choose-starting-point, section above>>, > >>> +and format-patch the series. If you are doing "rebase -i" in-place to > >>> +update from the previous round, this will reuse the previous base so > >>> +(2) and (3) may become trivial. > >>> + > >>> +. Find the base of where the last round was queued > >> > >> It's somewhat unusual for bulleted lists to start with a dot, but this > >> is consistent with the remainder of this document. > > > > Yeah, that's mostly why I added dots instead of asterisks here. > > Here we want a list of numbered items so that we can refer to other > items in the same list like "(2) and (3) may become trivial". > Wouldn't asterisks give us an unordered list that is typically > rendered as a list of unordered items instead? I didn't even know that dots are rendered as numbered items. TIL, I guess? Patrick
diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches index 8332073e27..2fd94dc8de 100644 --- a/Documentation/SubmittingPatches +++ b/Documentation/SubmittingPatches @@ -608,6 +608,85 @@ patch, format it as "multipart/signed", not a text/plain message that starts with `-----BEGIN PGP SIGNED MESSAGE-----`. That is not a text/plain, it's something else. +=== Handling Conflicts and Iterating Patches + +When revising changes made to your patches, it's important to +acknowledge the possibility of conflicts with other ongoing topics. To +navigate these potential conflicts effectively, follow the recommended +steps outlined below: + +. Build on a suitable base branch, see the <<choose-starting-point, section above>>, +and format-patch the series. If you are doing "rebase -i" in-place to +update from the previous round, this will reuse the previous base so +(2) and (3) may become trivial. + +. Find the base of where the last round was queued ++ + $ mine='kn/ref-transaction-symref' + $ git checkout "origin/seen^{/^Merge branch '$mine'}...master" + +. Apply your format-patch result. There are two cases +.. Things apply cleanly and tests fine. Go to (4). +.. Things apply cleanly but does not build or test fails, or things do +not apply cleanly. ++ +In the latter case, you have textual or semantic conflicts coming from +the difference between the old base and the base you used to build in +(1). Identify what caused the breakages (e.g., a topic or two may have +merged since the base used by (2) until the base used by (1)). ++ +Check out the latest 'origin/master' (which may be newer than the base +used by (2)), "merge --no-ff" the topics you newly depend on in there, +and use the result of the merge(s) as the base, rebuild the series and +test again. Run format-patch from the last such merges to the tip of +your topic. If you did ++ + $ git checkout origin/master + $ git merge --no-ff --into-name kn/ref-transaction-symref fo/obar + $ git merge --no-ff --into-name kn/ref-transaction-symref ba/zqux + ... rebuild the topic ... ++ +Then you'd just format your topic above these "preparing the ground" +merges, e.g. ++ + $ git format-patch "HEAD^{/^Merge branch 'ba/zqux'}"..HEAD ++ +Do not forget to write in the cover letter you did this, including the +topics you have in your base on top of 'master'. Then go to (4). + +. Make a trial merge of your topic into 'next' and 'seen', e.g. ++ + $ git checkout --detach 'origin/seen' && + $ git revert -m 1 <the merge of the previous iteration into seen> && + $ git merge kn/ref-transaction-symref ++ +The "revert" is needed if the previous iteration of your topic is +already in 'seen' (like in this case). You could choose to rebuild +master..origin/seen from scratch while excluding your previous +iteration, which may emulate what happens on the maintainers end more +closely. ++ +This trial merge may conflict. It is primarily to see what conflicts +_other_ topics may have with your topic. In other words, you do not +have to depend on to make your topic work on 'master'. It may become +the job of the other topic owners to resolve conflicts if your topic +goes to 'next' before theirs. ++ +Make a note on what conflict you saw in the cover letter. You do not +necessarily have to resolve them, but it would be a good opportunity to +learn what others are doing in an related area. ++ + $ git checkout --detach 'origin/next' && + $ git merge kn/ref-transaction-symref ++ +This is to see what conflicts your topic has with other topics that are +already cooking. This should not conflict if (3)-2 prepared a base on +top of updated master plus dependent topics taken from 'next'. Unless +the context is severe (one way to tell is try the same trial merge with +your old iteration, which may conflict in a similar way), expect that it +will be handled on maintainers end (if it gets unmanageable, I'll ask to +rebase when I receive your patches). + == Subsystems with dedicated maintainers Some parts of the system have dedicated maintainers with their own