Re: [GIT PULL] virtio: last minute fixup
On Thu, May 12, 2022 at 10:10:34AM -0700, Linus Torvalds wrote: > On Thu, May 12, 2022 at 6:30 AM Michael Ellerman wrote: > > > > Links to other random places don't serve that function. > > What "function"? > > This is my argument. Those Link: things need to have a *reason*. > > Saying "they are a change ID" is not a reason. That's just a random > word-salad. You need to have an active reason that you can explain, > not just say "look, I want to add a message ID to every commit". So I want to go to my inbox and compare the patch as received with what is in my tree. What did I change? And I tweak both the patch content when applying and the subject so these are not good indicators. Is this at all convincing? -- MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [GIT PULL] virtio: last minute fixup
Linus Torvalds writes: > On Thu, May 12, 2022 at 10:10 AM Linus Torvalds > wrote: >> >> And most definitely not just random data that can be trivially >> auto-generated after-the-fact. > > Put another way: when people asked for change ID's and I said "we have > links", I by no means meant that "you can just add random worthless > links to commits". > > For example, if you have a (public-facing) Gerrit system that tracks a > patch before it gets committed, BY ALL MEANS add a link to that as the > "change ID" that you tracked in Gerrit. > > That's a Link: that actually adds *information*. It shows some real > history to the commit, and shows who approved it and when, and gives > you all the Gerrit background. > > But a link to the email on lkml that just contains the patch and the > same commentary that was introduced into the commit? Useless garbage. > It adds no actual information. > > THAT is my argument. Why do people think I'm arguing against the Link: > tag? No. I'm arguing against adding links with no relevant new > information behind them. > > I don't argue against links to lore. Not at all. If those links are > about the background that caused the patch, they are great. Maybe they > are to a long thread about the original problem and how to solve it. > Thats WONDERFUL. > > But here's the deal: when I look at a commit that I wonder "why is it > doing this, it seems wrong" (possibly after there's been a bug report > about it, but possibly just because I'm reviewing it as part of doing > the pull), and I see a "Link:" tag, and it just points back to the > SAME DAMN DATA that I already have in the commit, then that Link: tag > not only wasn't helpful, it was ACTIVELY DETRIMENTAL and made me waste > time and just get irritated. > > And if you waste my time with useless links, why would you expect me > to be supportive of that behavior? You know. I have exactly the same reaction about your proposal to remove the Link tag. I hate it when v1, v2, v3, v4, etc are not part of the same thread. I find it very useful to go directly to the patch submission by following a single url and see the whole entire conversation right there. I don't relish the need to instead perform a search and waste my time filtering through similar submissions just to find the thread where things happened and what people were thinking. It is human and messy and unstructured so we probably need a search engine to find parts of historical conversations, but gosh darn search engines can take a lot of work to get useful results out of. As for finding the original problem that can be very hard. I recently had someone report a problem in code that had not changed in a decade or so that had just appeared. They had just happened to run ltp on a big enough machine where a poorly written test stressed the hardware on a large enough machine in just the right way that things started falling over. It took asking several times to find that out. So sure let's aim at getting more and better information in commits and in the urls that we place in commits. But let's not throw the baby out with the bath water and stop doing the part we can automate, because we have done such a good job of automating the indexing that we can usually find it with a simple search. Let's instead aim to keep the conversation connected, and the threads not broken so that following the url that is the easy thing to create gives us much more information. Eric ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [GIT PULL] virtio: last minute fixup
On Wed, May 11, 2022 at 02:24:23PM +0200, Jörg Rödel wrote: > On Tue, May 10, 2022 at 11:23:11AM -0700, Linus Torvalds wrote: > > And - once again - I want to complain about the "Link:" in that commit. > > I have to say that for me (probably for others as well) those Link tags > pointing to the patch submission have quite some value: > > 1) First of all it is an easy proof that the patch was actually > submitted somewhere for public review before it went into a > maintainers tree. > > 2) The patch submission is often the entry point to the > discussion which lead to this patch. From that email I can > see what was discussed and often there is even a link to > previous versions and the discussions that happened there. It > helps to better understand how a patch came to be the way it > is. I know this should ideally be part of the commit message, > but in reality this is what I also use the link tag for. > > 3) When backporting a patch to a downstream kernel it often > helps a lot to see the whole patch-set the change was > submitted in, especially when it comes to fixes. With the > Link: tag the whole submission thread is easy to find. > > I can stop adding them to patches if you want, but as I said, I think > there is some value in them which make me want to keep them. > > Regards, > > Joerg Yea, me too ... Linus, will it be less problematic if it's a different tag, other than Link? What if it's Message-Id: ? Still a problem? -- MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [GIT PULL] virtio: last minute fixup
On Thu, May 12, 2022 at 10:10 AM Linus Torvalds wrote: > > And most definitely not just random data that can be trivially > auto-generated after-the-fact. Put another way: when people asked for change ID's and I said "we have links", I by no means meant that "you can just add random worthless links to commits". For example, if you have a (public-facing) Gerrit system that tracks a patch before it gets committed, BY ALL MEANS add a link to that as the "change ID" that you tracked in Gerrit. That's a Link: that actually adds *information*. It shows some real history to the commit, and shows who approved it and when, and gives you all the Gerrit background. But a link to the email on lkml that just contains the patch and the same commentary that was introduced into the commit? Useless garbage. It adds no actual information. THAT is my argument. Why do people think I'm arguing against the Link: tag? No. I'm arguing against adding links with no relevant new information behind them. I don't argue against links to lore. Not at all. If those links are about the background that caused the patch, they are great. Maybe they are to a long thread about the original problem and how to solve it. Thats WONDERFUL. But here's the deal: when I look at a commit that I wonder "why is it doing this, it seems wrong" (possibly after there's been a bug report about it, but possibly just because I'm reviewing it as part of doing the pull), and I see a "Link:" tag, and it just points back to the SAME DAMN DATA that I already have in the commit, then that Link: tag not only wasn't helpful, it was ACTIVELY DETRIMENTAL and made me waste time and just get irritated. And if you waste my time with useless links, why would you expect me to be supportive of that behavior? Linus ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [GIT PULL] virtio: last minute fixup
On Thu, May 12, 2022 at 6:30 AM Michael Ellerman wrote: > > Links to other random places don't serve that function. What "function"? This is my argument. Those Link: things need to have a *reason*. Saying "they are a change ID" is not a reason. That's just a random word-salad. You need to have an active reason that you can explain, not just say "look, I want to add a message ID to every commit". Here's the thing. There's a difference between "data" and "information". We should add information to the commits, not random data. And most definitely not just random data that can be trivially auto-generated after-the-fact. Linus ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [GIT PULL] virtio: last minute fixup
Linus Torvalds writes: > On Wed, May 11, 2022 at 3:12 AM Michael Ellerman wrote: >> >> Which I read as you endorsing Link: tags :) > > I absolutely adore "Link:" tags. They've been great. > > But they've been great for links that are *usedful*. > > They are wonderful when they link to the original problem. > > They are *really* wonderful when they link to some long discussion > about how to solve the problem. > > They are completely useless when they link to "this is the patch > submission of the SAME DAMN PATCH THAT THE COMMIT IS". Folks wanted to add Change-Id: tags to every commit. You said we didn't need to, because we have the Link: to the original patch submission, which includes the Message-Id and therefore is a de facto change id. Links to other random places don't serve that function. cheers ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [GIT PULL] virtio: last minute fixup
On Wed, May 11, 2022 at 12:31:16PM -0400, Konstantin Ryabitsev wrote: > > But my mailer, editor and terminal don't know what to do with a Message-Id. > > > > Whereas they can all open an https link. > > > > Making people paste message ids into lore to see the original submission > > is not a win. People make enough fun of us already for still using email > > to submit patches, let's not make their job any easier :) > > Okay, I'm fine with using a dedicated trailer for this purpose, perhaps an > "Archived-At"? That's a real header that was proposed by IETF for similar > purposes. E.g.: > > Archived-at: > https://lore.kernel.org/r/CAHk-=wgAk3NEJ2PHtb0jXzCUOGytiHLq=rzjkfkfpiuh-sr...@mail.gmail.com > I'd suggest is "Patch-Link". Then we can also have "Bug-Link:", "Test-Link:", etc. "Patch-Link" is a tad bit shorter "Archived-at", and ultimately, it's not actually not the patch which is being archived. It's the fact that it's a pointer to the patch review which is of most interest. - Ted ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [GIT PULL] virtio: last minute fixup
On Wed, May 11, 2022 at 3:12 AM Michael Ellerman wrote: > > Which I read as you endorsing Link: tags :) I absolutely adore "Link:" tags. They've been great. But they've been great for links that are *usedful*. They are wonderful when they link to the original problem. They are *really* wonderful when they link to some long discussion about how to solve the problem. They are completely useless when they link to "this is the patch submission of the SAME DAMN PATCH THAT THE COMMIT IS". See the difference? The two first links add actual new information. That last link adds absolutely nothing. It's a link to the same email that was just applied. Linus ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [GIT PULL] virtio: last minute fixup
Konstantin Ryabitsev writes: ... > > I think we should simply disambiguate the trailer added by tooling like b4. > Instead of using Link:, it can go back to using Message-Id, which is already > standard with git -- it's trivial for git.kernel.org to link them to > lore.kernel.org. But my mailer, editor and terminal don't know what to do with a Message-Id. Whereas they can all open an https link. Making people paste message ids into lore to see the original submission is not a win. People make enough fun of us already for still using email to submit patches, let's not make their job any easier :) > Before: > > Signed-off-by: Main Tainer > Link: > https://lore.kernel.org/r/CAHk-=wgAk3NEJ2PHtb0jXzCUOGytiHLq=rzjkfkfpiuh-sr...@mail.gmail.com > > After: > > Signed-off-by: Main Tainer > Message-Id: > > > This would allow people to still use Link: for things like linking to actual > ML discussions. I know this pollutes commits a bit, but I would argue that > this is a worthwhile trade-off that allows us to improve our automation and > better scale maintainers. I went back through the history and I'm pretty sure that the original use for "Link:" was to link to the original submission, done by tip-bot starting back in 2011: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f994d99cf140dbb637e49882891c89b3fd84becd Prior to that there were no "Link:" tags, only "BugLink:". But if people want to reclaim "Link:" for generic links then fine, but let's still use a https link, just give it a different name. eg. "PatchLink:", or "Submitted:" etc. cheers ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [GIT PULL] virtio: last minute fixup
On Tue, May 10, 2022 at 11:23:11AM -0700, Linus Torvalds wrote: > And - once again - I want to complain about the "Link:" in that commit. I have to say that for me (probably for others as well) those Link tags pointing to the patch submission have quite some value: 1) First of all it is an easy proof that the patch was actually submitted somewhere for public review before it went into a maintainers tree. 2) The patch submission is often the entry point to the discussion which lead to this patch. From that email I can see what was discussed and often there is even a link to previous versions and the discussions that happened there. It helps to better understand how a patch came to be the way it is. I know this should ideally be part of the commit message, but in reality this is what I also use the link tag for. 3) When backporting a patch to a downstream kernel it often helps a lot to see the whole patch-set the change was submitted in, especially when it comes to fixes. With the Link: tag the whole submission thread is easy to find. I can stop adding them to patches if you want, but as I said, I think there is some value in them which make me want to keep them. Regards, Joerg ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [GIT PULL] virtio: last minute fixup
Linus Torvalds writes: > On Tue, May 10, 2022 at 5:24 AM Michael S. Tsirkin wrote: >> >> A last minute fixup of the transitional ID numbers. >> Important to get these right - if users start to depend on the >> wrong ones they are very hard to fix. > > Hmm. I've pulled this, but those numbers aren't exactly "new". > > They've been that way since 5.14, so what makes you think people > haven't already started depending on them? > > And - once again - I want to complain about the "Link:" in that commit. > > It points to a completely useless patch submission. It doesn't point > to anything useful at all. > > I think it's a disease that likely comes from "b4", and people decided > that "hey, I can use the -l parameter to add that Link: field", and it > looks better that way. Some folks have been doing it since the early 2010s. But I think it really took off after the Change-Id discussion a few years back: https://lore.kernel.org/all/CAHk-=whFbgy4RXG11c_=S7O-248oWmwB_aZOcWzWMVh3w7=r...@mail.gmail.com/ Which I read as you endorsing Link: tags :) > And then they add it all the time, whether it makes any sense or not. For me it always makes sense, because it means I can easily go from a commit back to the original submission. That's useful for automating various things like replies and patchwork status updates. It allows me to find the exact patch I applied, even if what I committed is slightly different (due to fuzz or editing), which would be harder with a search based approach. It gives us a way to essentially augment the change log after the fact, by replying to the original patch with things we didn't know at the time of commit - eg. this patch was reverted because it caused a bug, etc. If you follow the Link: and there's nothing useful there explaining what motivated the change then that's a bug in the patch submission, not the Link: tag. Really important information should be in the change log itself, but the space below the "---" is perfect for added context that would be too verbose for the committed change log. And anyone can reply to the original submission to add even more useful information. cheers ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [GIT PULL] virtio: last minute fixup
On Tue, May 10, 2022 at 04:50:47PM -0700, Linus Torvalds wrote:
> On Tue, May 10, 2022 at 4:12 PM Nathan Chancellor wrote:
> >
> > For what it's worth, as someone who is frequently tracking down and
> > reporting issues, a link to the mailing list post in the commit message
> > makes it much easier to get these reports into the right hands, as the
> > original posting is going to have all relevant parties in one location
> > and it will usually have all the context necessary to triage the
> > problem.
>
> Honestly, I think such a thing would be trivial to automate with
> something like just a patch-id lookup, rather than a "Link:".
>
> And such a lookup model ("where was this patch posted") would work for
> any patch (and often also find previous unmodified versions of
> it when it has been posted multiple times).
>
> I suspect that most of the building blocks of such automation
> effectively already exists, since I think the lore infrastructure
> already integrates with patchwork, and patchwork already has a "look
> up by patch id".
>
> Wouldn't it be cool if you had some webby interface to just go from
> commit SHA1 to patch ID to a lore.kernel.org lookup of where said
> patch was done?
Yes, that would be cool!
> Of course, I personally tend to just search by the commit contents
> instead, which works just about as well. If the first line of the
> commit isn't very unique, add a "f:author" to the search.
>
> IOW, I really don't find much value in the "Link to original
> submission", because that thing is *already* trivial to find, and the
> lore search is actually better in many ways (it also tends to find
> people *reporting* that commit, which is often what you really want -
> the reason you're doing the search is that there's something going on
> with it).
>
> My argument here really is that "find where this commit was posted" is
>
> (a) not generally the most interesting thing
>
> (b) doesn't even need that "Link:" line.
>
> but what *is* interesting, and where the "Link:" line is very useful,
> is finding where the original problem that *caused* that patch to be
> posted in the first place.
>
> Yes, obviously you can find that original problem by searching too if
> the commit message has enough other information.
>
> For example, if there is an oops quoted in the commit message, I have
> personally searched for parts of that kind of information to find the
> original report and discussion.
>
> So that whole "searching is often an option" is true for pretty much
> _any_ Link:, but I think that for the whole "original submission" it's
> so mindless and can be automated that it really doesn't add much real
> value at all.
>
> Linus
For me a problematic use-case is multiple versions of the patchset.
So I have a tree and I apply a patchset, start testing etc. Meanwhile author
posts another version. At that point I want to know which version
did I apply. Since people put that within [] in the subject, it
gets stripped off.
Thinking about it some more, how about sticking a link to the *cover
letter* in the commit, instead? That would serve an extra useful purpose of
being able to figure out which patches are part of the same patchset.
And maybe Change "Link:" to "Patchset:" or "Cover-letter:"?
--
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [GIT PULL] virtio: last minute fixup
On Tue, May 10, 2022 at 11:23:11AM -0700, Linus Torvalds wrote: > On Tue, May 10, 2022 at 5:24 AM Michael S. Tsirkin wrote: > > > > A last minute fixup of the transitional ID numbers. > > Important to get these right - if users start to depend on the > > wrong ones they are very hard to fix. > > Hmm. I've pulled this, but those numbers aren't exactly "new". > > They've been that way since 5.14, so what makes you think people > haven't already started depending on them? Yes they have been in the header but they are not used by *Linux* yet. My worry is for when we start using them and then someone backports the patches without backporting the macro fix. Maybe we should just drop these until there's a user, but I am a bit wary of a step like this so late in the cycle. > And - once again - I want to complain about the "Link:" in that commit. > > It points to a completely useless patch submission. It doesn't point > to anything useful at all. > > I think it's a disease that likely comes from "b4", and people decided > that "hey, I can use the -l parameter to add that Link: field", and it > looks better that way. > > And then they add it all the time, whether it makes any sense or not. > > I've mainly noticed it with the -tip tree, but maybe that's just > because I've happened to look at it. > > I really hate those worthless links that basically add zero actual > information to the commit. > > The "Link" field is for _useful_ links. Not "let's add a link just > because we can". > >Linus OK I will stop doing this. I thought they are handy for when there are several versions of the patch. It helps me make sure I applied the latest one. Saving the message ID of the original mail in some other way would also be ok. Any suggestions for a better way to do this? -- MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [GIT PULL] virtio: last minute fixup
On Tue, May 10, 2022 at 4:12 PM Nathan Chancellor wrote:
>
> For what it's worth, as someone who is frequently tracking down and
> reporting issues, a link to the mailing list post in the commit message
> makes it much easier to get these reports into the right hands, as the
> original posting is going to have all relevant parties in one location
> and it will usually have all the context necessary to triage the
> problem.
Honestly, I think such a thing would be trivial to automate with
something like just a patch-id lookup, rather than a "Link:".
And such a lookup model ("where was this patch posted") would work for
any patch (and often also find previous unmodified versions of
it when it has been posted multiple times).
I suspect that most of the building blocks of such automation
effectively already exists, since I think the lore infrastructure
already integrates with patchwork, and patchwork already has a "look
up by patch id".
Wouldn't it be cool if you had some webby interface to just go from
commit SHA1 to patch ID to a lore.kernel.org lookup of where said
patch was done?
Of course, I personally tend to just search by the commit contents
instead, which works just about as well. If the first line of the
commit isn't very unique, add a "f:author" to the search.
IOW, I really don't find much value in the "Link to original
submission", because that thing is *already* trivial to find, and the
lore search is actually better in many ways (it also tends to find
people *reporting* that commit, which is often what you really want -
the reason you're doing the search is that there's something going on
with it).
My argument here really is that "find where this commit was posted" is
(a) not generally the most interesting thing
(b) doesn't even need that "Link:" line.
but what *is* interesting, and where the "Link:" line is very useful,
is finding where the original problem that *caused* that patch to be
posted in the first place.
Yes, obviously you can find that original problem by searching too if
the commit message has enough other information.
For example, if there is an oops quoted in the commit message, I have
personally searched for parts of that kind of information to find the
original report and discussion.
So that whole "searching is often an option" is true for pretty much
_any_ Link:, but I think that for the whole "original submission" it's
so mindless and can be automated that it really doesn't add much real
value at all.
Linus
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [GIT PULL] virtio: last minute fixup
The pull request you sent on Tue, 10 May 2022 08:23:51 -0400: > https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git tags/for_linus has been merged into torvalds/linux.git: https://git.kernel.org/torvalds/c/feb9c5e19e913b53cb536a7aa7c9f20107bb51ec Thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/prtracker.html ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [GIT PULL] virtio: last minute fixup
On Tue, May 10, 2022 at 5:24 AM Michael S. Tsirkin wrote: > > A last minute fixup of the transitional ID numbers. > Important to get these right - if users start to depend on the > wrong ones they are very hard to fix. Hmm. I've pulled this, but those numbers aren't exactly "new". They've been that way since 5.14, so what makes you think people haven't already started depending on them? And - once again - I want to complain about the "Link:" in that commit. It points to a completely useless patch submission. It doesn't point to anything useful at all. I think it's a disease that likely comes from "b4", and people decided that "hey, I can use the -l parameter to add that Link: field", and it looks better that way. And then they add it all the time, whether it makes any sense or not. I've mainly noticed it with the -tip tree, but maybe that's just because I've happened to look at it. I really hate those worthless links that basically add zero actual information to the commit. The "Link" field is for _useful_ links. Not "let's add a link just because we can". Linus ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[GIT PULL] virtio: last minute fixup
The following changes since commit 1c80cf031e0204fde471558ee40183695773ce13: vdpa: mlx5: synchronize driver status with CVQ (2022-03-30 04:18:14 -0400) are available in the Git repository at: https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git tags/for_linus for you to fetch changes up to 7ff960a6fe399fdcbca6159063684671ae57eee9: virtio: fix virtio transitional ids (2022-05-10 07:22:28 -0400) virtio: last minute fixup A last minute fixup of the transitional ID numbers. Important to get these right - if users start to depend on the wrong ones they are very hard to fix. Signed-off-by: Michael S. Tsirkin Shunsuke Mie (1): virtio: fix virtio transitional ids include/uapi/linux/virtio_ids.h | 14 +++--- 1 file changed, 7 insertions(+), 7 deletions(-) ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
