Message ID | 20180816215726.GA20526@ziepe.ca (mailing list archive) |
---|---|
State | Mainlined |
Commit | 8a8c600de5dc1d9a7f4b83269fddc80ebd3dd045 |
Headers | show |
Series | [GIT,PULL] Please pull RDMA subsystem changes | expand |
On Thu, Aug 16, 2018 at 2:57 PM Jason Gunthorpe <jgg@mellanox.com> wrote: > > This time there are many merge conflicts, all due to various tree-wide or RDMA > subystem wide changes done by various people. The resolution is tricky, as git > does not highlight two areas that need revision. I was confused by this, because there were no merge conflicts. Then I looked at the history, and the reason is that you actually pushed the for-next branch into the for-linus branch, and so I got the pre-merged version. Oh well. I'll look at what the conflicts were, but may end up just taking your resolution. Linus
On Fri, Aug 17, 2018 at 12:31 PM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > Oh well. I'll look at what the conflicts were, but may end up just > taking your resolution. Ok, everything but the max_sge thing was trivial. I just took yours. Linus
On Fri, Aug 17, 2018 at 12:31:00PM -0700, Linus Torvalds wrote: > On Thu, Aug 16, 2018 at 2:57 PM Jason Gunthorpe <jgg@mellanox.com> wrote: > > > > This time there are many merge conflicts, all due to various tree-wide or RDMA > > subystem wide changes done by various people. The resolution is tricky, as git > > does not highlight two areas that need revision. > > I was confused by this, because there were no merge conflicts. > > Then I looked at the history, and the reason is that you actually > pushed the for-next branch into the for-linus branch, and so I got the > pre-merged version. Er, yes, sorry, I made a typo in my email, it should have read "As before I've resolved the conflicts in the for-linus tag and provided the unmerged tree in the for-linus-unmerged tag" > Oh well. I'll look at what the conflicts were, but may end up just > taking your resolution. We often have merge conflicts with RDMA, how do you prefer to get the PR? I'm actually not very clear on this part of the process. Historically, when we have conflicts, I've given you a tag 'for-linus-unmerged', a text description of the conflicts with merge diff hunks, and then a tag 'for-linus' which contains the full resolution I used and checked (based on linux-next). In the past you've taken from both tags, I think, depending on how the resolution looks? I generate the 'git request-pull' against the 'for-linus' tag only because the diffstat of the resolved branch is closer to what you'll actually see during merge. Otherwise there are always misleading netdev changes due to the shared branches with netdev. If you don't care about the diffstat I'll switch to 'for-linus' and 'for-linus-resolved' tags so you won't accidentally get any resolution commit unless you want it. Thanks, Jason
On Fri, Aug 17, 2018 at 1:15 PM Jason Gunthorpe <jgg@mellanox.com> wrote: > > We often have merge conflicts with RDMA, how do you prefer to get the > PR? I'm actually not very clear on this part of the process. I generally prefer the non-merged version, but with comments about the conflicts. In fact, the really preferred model is generally to send me a non-merged pull request (say "tags/for-linus") but if the conflicts look even half-way nasty - or simply because you did the merge anyway just to get the proper diffstat because history is complex - mention that you have a merged branch for verification (say "branch/for-linus-merged"). When I get that kind of pull request, I'll just do the merge resolution myself, and after I've done it I'll generally then do git checkout -b test-merge git pull <repoaddress> for-linus-merged and then just compare the end result with my resolution as an extra verification step. I may end up skipping the verification step if everything looks entirely trivial (and really, if you have no real reason for the pre-merged branch, don't bother even mentioning it even if you did it for the diffstat), but in practice whenever somebody does that, I have no reason not to just do that simple extra verification. Most of the time the merges are exactly the same, possibly with whitespace or trivial ordering differences (things like Makefiles and Kconfig files often have add-add conflicts where the ordering really doesn't matter between two config options). And then sometimes it shows that I missed something in my mmerge. And sometimes it shows that I do so many merges that I actually ended up noticing something that the submaintainer didn't. So it can be uninteresting, and when it isn't uninteresting it can go either way, but so far for the people who do this, I've never been in the situation where I was *sorry* for the double merge and extra verification step. Because when mis-merges happen (they are happily pretty rare) they are _so_ annoying and can be so hard to figure out later, that I'd rather spend a bit more time on the merge than have a dodgy one. Linus
On Fri, Aug 17, 2018 at 12:44 PM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > Ok, everything but the max_sge thing was trivial. I just took yours. Oh, and doing the full test compile, I notice there are new warnings. That is *NOT* ok. The whole "x is deprecated" is not a useful warning. If you can't remove something, making it a reminder for yourself for later is not an acceptable excuse for bothering everybody else with the warning, and possibly having other issues hidden because by the time there are expected warnings, people will start ignoring the unexpected ones too. So "__deprecated" is for stuff that really isn't used any more, to catch possible new users. People have tried to use it for anything else, but it's always been a much bigger pain than it is worth. I've considered just removing the whole deprecation infrastructure. It has never really worked. Either its used, in which case deprecation warnings only hide real issues, or it's not used, in which case the function should just be removed. Linus
On Fri, Aug 17, 2018 at 01:50:05PM -0700, Linus Torvalds wrote: > On Fri, Aug 17, 2018 at 12:44 PM Linus Torvalds > <torvalds@linux-foundation.org> wrote: > > > > Ok, everything but the max_sge thing was trivial. I just took yours. > > Oh, and doing the full test compile, I notice there are new warnings. > > That is *NOT* ok. I expect to send you a pull request to remove all of this next week. This is fall out from the SMC merge conflict reversion. > The whole "x is deprecated" is not a useful warning. If you can't > remove something, making it a reminder for yourself for later is not > an acceptable excuse for bothering everybody else with the warning, > and possibly having other issues hidden because by the time there are > expected warnings, people will start ignoring the unexpected ones too. > > So "__deprecated" is for stuff that really isn't used any more, to > catch possible new users. People have tried to use it for anything > else, but it's always been a much bigger pain than it is worth. In this case it has become confusing what CONFIG_ENABLE_WARN_DEPRECATED is for. If the kernel is supposed to compile warning free with that config set, then why have the config at all - it does nothing, by definition? Would you like a patch to remove that CONFIG option? Jason
On Fri, Aug 17, 2018 at 01:27:02PM -0700, Linus Torvalds wrote: > On Fri, Aug 17, 2018 at 1:15 PM Jason Gunthorpe <jgg@mellanox.com> wrote: > > > > We often have merge conflicts with RDMA, how do you prefer to get the > > PR? I'm actually not very clear on this part of the process. > > I generally prefer the non-merged version, but with comments about > the conflicts. > > In fact, the really preferred model is generally to send me a > non-merged pull request (say "tags/for-linus") but if the conflicts > look even half-way nasty - or simply because you did the merge anyway > just to get the proper diffstat because history is complex - mention > that you have a merged branch for verification (say > "branch/for-linus-merged"). > > When I get that kind of pull request, I'll just do the merge > resolution myself, and after I've done it I'll generally then do > > git checkout -b test-merge > git pull <repoaddress> for-linus-merged > > and then just compare the end result with my resolution as an extra > verification step. > > I may end up skipping the verification step if everything looks > entirely trivial (and really, if you have no real reason for the > pre-merged branch, don't bother even mentioning it even if you did it > for the diffstat), but in practice whenever somebody does that, I have > no reason not to just do that simple extra verification. > > Most of the time the merges are exactly the same, possibly with > whitespace or trivial ordering differences (things like Makefiles and > Kconfig files often have add-add conflicts where the ordering really > doesn't matter between two config options). > > And then sometimes it shows that I missed something in my mmerge. > > And sometimes it shows that I do so many merges that I actually ended > up noticing something that the submaintainer didn't. > > So it can be uninteresting, and when it isn't uninteresting it can go > either way, but so far for the people who do this, I've never been in > the situation where I was *sorry* for the double merge and extra > verification step. > > Because when mis-merges happen (they are happily pretty rare) they are > _so_ annoying and can be so hard to figure out later, that I'd rather > spend a bit more time on the merge than have a dodgy one. Thanks for the explanation. Doug and I will do this in future. Jason
On Fri, Aug 17, 2018 at 2:17 PM Jason Gunthorpe <jgg@mellanox.com> wrote: > > Would you like a patch to remove that CONFIG option? I've definitely considered it and would almost certainly just apply a patch that removed it. We haven't had this issue for a while (because people stopped using the deprecated thing), so it ended up not having been an issue lately. Yours was the first case of deprecation warnings I've seen in a longish while. Linus