| Message ID | 20180826141413.3033-1-dja@axtens.net |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Find patches, cover letters and comments by msgid | expand |
On Sun, Aug 26, 2018 at 7:14 AM Daniel Axtens <dja@axtens.net> wrote: > > This patch doesn't expose these URLs in the UI anywhere. I'd love to > know where people would like them to go. I hesitate to make the > message-id clickable in the patch detail view, as I think that would > break workflows for people who want to copy the msgid to the clipboard. So just to start off: I'm not personally a heavy patchwork user, but I end up occasionally using it particularly when there's a bug report and the issue bisects down to something that then has a patchwork link. I've also mostly seen old versions of patchwork becasue of kernel.org not having been up-to-date. Take this to mean that my opinions on this may be skewed and not necessarily those of heavy users. But given that, I'd really like to see a pointer to the actual email in the header line not just of the original email with the patch, but of every comment. Right now patchwork (well, at least on kernel.org) has that separate header area for that particular patchwork entry, I don't mean that. I mean the line that right now has a pointer to the author of each comment, and a date. Add a clickable link to just after the name of the person making the comment? Or even make the existing (mostly useless) date clickable? Also, since the first email is likely the one you care about most, also make the "Message ID" string itself clickable (in _addition_ to the one-line header for the "Commit message" like all the "Comments"). I agree that the actual string that contains the message ID itself might be best not be clickable for the whole copy-paste reason you mention, but the actual "Message ID" string itself could be clickable, no? IOW (sorry for the ugly ASCII barfic), in the header of the whole thing, you'd have: Message ID 20180826141413.3033-1-dja@axtens.net ^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | | | +--- not clickable to make | copy-paste easy | +------ clickable, to get to the email and then in the header for the individual "Commit message" and "Comments", you'd have Daniel Axtens [ msg ] Mon, 27 Aug 2018 00:14:13 ^^^^^^^^^^^^^ ^^^^^^^ | | | +--- clickable, goes to the message | +------ clickable, goes to the person Or _something_ along these lines. Maybe instead of that added "[msg]" thing, just make the date clickable and get to the email instead. Linus
On Mon, 2018-08-27 at 00:14 +1000, Daniel Axtens wrote: > Add /message/<msgid> and /project/<project>/message/<msgid> URLs. > > /project/<project>/message/<msgid> finds the patch, cover-letter or > comment in <project> that matches <msgid> and redirects you to it. > This is guaranteed to be unique. > > /message/<msgid> does not require a project, but therefore if a mail > has been sent to multiple projects, the project that you will be > redirected to is arbitrary. Personally, I'm not a fan of the latter as I don't see what value it would bring. Is there any reason to keep it or could we drop it. > This patch doesn't expose these URLs in the UI anywhere. I'd love to > know where people would like them to go. I hesitate to make the > message-id clickable in the patch detail view, as I think that would > break workflows for people who want to copy the msgid to the clipboard. > > These URLs also just redirect to the usual canonical patchwork URL - > it doesn't supplant or replace them. However, I'd be open to moving to > this scheme as the 'normal'/default URL in the future. Personally, I'd reverse the order of things and make these the new canonical URLs. There are issues with using the autoincrement ID column, as Konstantin has pointed out, but we could continue to use these for shortlinks until such a time as we come up with something better. > I also haven't attempted to do anything meaningful with series. I think this needs to be addressed too. At a minimum, we should do cover letters. Stephen > Partially-closes: #106 > Reported-by: Konstantin Ryabitsev <konstantin@linuxfoundation.org> > Reported-by: Linus Torvalds <torvalds@linux-foundation.org> > Reported-by: Stephen Finucane <stephen@that.guru> > Signed-off-by: Daniel Axtens <dja@axtens.net> > --- > patchwork/urls.py | 7 ++++++ > patchwork/views/patch.py | 49 ++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 56 insertions(+) > > diff --git a/patchwork/urls.py b/patchwork/urls.py > index 6b1ef511ef15..90391b90e413 100644 > --- a/patchwork/urls.py > +++ b/patchwork/urls.py > @@ -74,6 +74,13 @@ urlpatterns = [ > url(r'^series/(?P<series_id>\d+)/mbox/$', series_views.series_mbox, > name='series-mbox'), > > + # message-id searches > + url(r'^project/(?P<project_id>[^/]+)/message/(?P<msg_id>[^/]+)$', > + patch_views.patch_by_project_msgid, > + name='msgid-project-redirect'), Given my above comments, I wonder if we should split this into 'patch' and 'cover' again? I realize it's more work, but I really do think these should become the canonical links. Stephen > + url(r'^message/(?P<msg_id>[^/]+)$', patch_views.patch_by_msgid, > + name='msgid-redirect'), > + > # logged-in user stuff > url(r'^user/$', user_views.profile, name='user-profile'), > url(r'^user/todo/$', user_views.todo_lists, > diff --git a/patchwork/views/patch.py b/patchwork/views/patch.py > index 6921882e71d1..53441b39cc57 100644 > --- a/patchwork/views/patch.py > +++ b/patchwork/views/patch.py > @@ -28,6 +28,7 @@ from django.urls import reverse > > from patchwork.forms import CreateBundleForm > from patchwork.forms import PatchForm > +from patchwork.models import Comment > from patchwork.models import Bundle > from patchwork.models import Patch > from patchwork.models import Project > @@ -150,3 +151,51 @@ def patch_mbox(request, patch_id): > patch.filename) > > return response > + > + > +def patch_by_msgid(request, msg_id): > + msgid = ('<%s>' % msg_id) > + > + patches = Patch.objects.filter(msgid=msgid) > + if patches: > + return HttpResponseRedirect( > + reverse('patch-detail', kwargs={'patch_id': patches.first().id})) > + > + subs = Submission.objects.filter(msgid=msgid) > + if subs: > + return HttpResponseRedirect( > + reverse('cover-detail', kwargs={'cover_id': subs.first().id})) > + > + comments = Comment.objects.filter(msgid=msgid) > + if comments: > + return HttpResponseRedirect(comments.first().get_absolute_url()) > + > + raise Http404("No patch, cover letter of comment matching %s found." % msg_id) > + > + > +def patch_by_project_msgid(request, project_id, msg_id): > + project = get_object_or_404(Project, linkname=project_id) > + project_id = project.id > + msgid = ('<%s>' % msg_id) > + > + try: > + patch = Patch.objects.get(patch_project_id=project_id, msgid=msgid) > + return HttpResponseRedirect( > + reverse('patch-detail', kwargs={'patch_id': patch.id})) > + except Patch.DoesNotExist: > + pass > + > + try: > + sub = Submission.objects.get(project_id=project_id, msgid=msgid) > + return HttpResponseRedirect( > + reverse('cover-detail', kwargs={'cover_id': sub.id})) > + except Submission.DoesNotExist: > + pass > + > + try: > + comment = Comment.objects.get(submission__project_id=project_id, msgid=msgid) > + return HttpResponseRedirect(comment.get_absolute_url()) > + except Comment.DoesNotExist: > + pass > + > + raise Http404("No patch, cover letter or comment matching %s found in %s" % (msg_id, project.name))
On Wed, Aug 29, 2018 at 01:50:11PM +0100, Stephen Finucane wrote: > On Mon, 2018-08-27 at 00:14 +1000, Daniel Axtens wrote: > > Add /message/<msgid> and /project/<project>/message/<msgid> URLs. > > > > /project/<project>/message/<msgid> finds the patch, cover-letter or > > comment in <project> that matches <msgid> and redirects you to it. > > This is guaranteed to be unique. > > > > /message/<msgid> does not require a project, but therefore if a mail > > has been sent to multiple projects, the project that you will be > > redirected to is arbitrary. > > Personally, I'm not a fan of the latter as I don't see what value it > would bring. Is there any reason to keep it or could we drop it. With the kernel git tree being the culmination of about 100 different mailing lists/projects, it is hard to map a msg-id found in the commit log to the right project. So the former could be difficult unless you have the decoder ring for the mailing list it went through. Now the msg-id could map to nothing to as there is no guarantee it is in patchwork either. Just a different perspective. Cheers, Don > > > This patch doesn't expose these URLs in the UI anywhere. I'd love to > > know where people would like them to go. I hesitate to make the > > message-id clickable in the patch detail view, as I think that would > > break workflows for people who want to copy the msgid to the clipboard. > > > > These URLs also just redirect to the usual canonical patchwork URL - > > it doesn't supplant or replace them. However, I'd be open to moving to > > this scheme as the 'normal'/default URL in the future. > > Personally, I'd reverse the order of things and make these the new > canonical URLs. There are issues with using the autoincrement ID > column, as Konstantin has pointed out, but we could continue to use > these for shortlinks until such a time as we come up with something > better. > > > I also haven't attempted to do anything meaningful with series. > > I think this needs to be addressed too. At a minimum, we should do > cover letters. > > Stephen > > > Partially-closes: #106 > > Reported-by: Konstantin Ryabitsev <konstantin@linuxfoundation.org> > > Reported-by: Linus Torvalds <torvalds@linux-foundation.org> > > Reported-by: Stephen Finucane <stephen@that.guru> > > Signed-off-by: Daniel Axtens <dja@axtens.net> > > --- > > patchwork/urls.py | 7 ++++++ > > patchwork/views/patch.py | 49 ++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 56 insertions(+) > > > > diff --git a/patchwork/urls.py b/patchwork/urls.py > > index 6b1ef511ef15..90391b90e413 100644 > > --- a/patchwork/urls.py > > +++ b/patchwork/urls.py > > @@ -74,6 +74,13 @@ urlpatterns = [ > > url(r'^series/(?P<series_id>\d+)/mbox/$', series_views.series_mbox, > > name='series-mbox'), > > > > + # message-id searches > > + url(r'^project/(?P<project_id>[^/]+)/message/(?P<msg_id>[^/]+)$', > > + patch_views.patch_by_project_msgid, > > + name='msgid-project-redirect'), > > Given my above comments, I wonder if we should split this into 'patch' > and 'cover' again? I realize it's more work, but I really do think > these should become the canonical links. > > Stephen > > > + url(r'^message/(?P<msg_id>[^/]+)$', patch_views.patch_by_msgid, > > + name='msgid-redirect'), > > + > > # logged-in user stuff > > url(r'^user/$', user_views.profile, name='user-profile'), > > url(r'^user/todo/$', user_views.todo_lists, > > diff --git a/patchwork/views/patch.py b/patchwork/views/patch.py > > index 6921882e71d1..53441b39cc57 100644 > > --- a/patchwork/views/patch.py > > +++ b/patchwork/views/patch.py > > @@ -28,6 +28,7 @@ from django.urls import reverse > > > > from patchwork.forms import CreateBundleForm > > from patchwork.forms import PatchForm > > +from patchwork.models import Comment > > from patchwork.models import Bundle > > from patchwork.models import Patch > > from patchwork.models import Project > > @@ -150,3 +151,51 @@ def patch_mbox(request, patch_id): > > patch.filename) > > > > return response > > + > > + > > +def patch_by_msgid(request, msg_id): > > + msgid = ('<%s>' % msg_id) > > + > > + patches = Patch.objects.filter(msgid=msgid) > > + if patches: > > + return HttpResponseRedirect( > > + reverse('patch-detail', kwargs={'patch_id': patches.first().id})) > > + > > + subs = Submission.objects.filter(msgid=msgid) > > + if subs: > > + return HttpResponseRedirect( > > + reverse('cover-detail', kwargs={'cover_id': subs.first().id})) > > + > > + comments = Comment.objects.filter(msgid=msgid) > > + if comments: > > + return HttpResponseRedirect(comments.first().get_absolute_url()) > > + > > + raise Http404("No patch, cover letter of comment matching %s found." % msg_id) > > + > > + > > +def patch_by_project_msgid(request, project_id, msg_id): > > + project = get_object_or_404(Project, linkname=project_id) > > + project_id = project.id > > + msgid = ('<%s>' % msg_id) > > + > > + try: > > + patch = Patch.objects.get(patch_project_id=project_id, msgid=msgid) > > + return HttpResponseRedirect( > > + reverse('patch-detail', kwargs={'patch_id': patch.id})) > > + except Patch.DoesNotExist: > > + pass > > + > > + try: > > + sub = Submission.objects.get(project_id=project_id, msgid=msgid) > > + return HttpResponseRedirect( > > + reverse('cover-detail', kwargs={'cover_id': sub.id})) > > + except Submission.DoesNotExist: > > + pass > > + > > + try: > > + comment = Comment.objects.get(submission__project_id=project_id, msgid=msgid) > > + return HttpResponseRedirect(comment.get_absolute_url()) > > + except Comment.DoesNotExist: > > + pass > > + > > + raise Http404("No patch, cover letter or comment matching %s found in %s" % (msg_id, project.name)) > > > _______________________________________________ > Patchwork mailing list > Patchwork@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/patchwork
Hi Stephen, >> /message/<msgid> does not require a project, but therefore if a mail >> has been sent to multiple projects, the project that you will be >> redirected to is arbitrary. > > Personally, I'm not a fan of the latter as I don't see what value it > would bring. Is there any reason to keep it or could we drop it. As Don points out, without this the feature is basically useless for kernel work. Given that kernel work represents the bulk of both OzLabs PW and kernel.org PW, it's reasonably critical. >> These URLs also just redirect to the usual canonical patchwork URL - >> it doesn't supplant or replace them. However, I'd be open to moving to >> this scheme as the 'normal'/default URL in the future. > > Personally, I'd reverse the order of things and make these the new > canonical URLs. There are issues with using the autoincrement ID > column, as Konstantin has pointed out, but we could continue to use > these for shortlinks until such a time as we come up with something > better. > >> I also haven't attempted to do anything meaningful with series. > > I think this needs to be addressed too. At a minimum, we should do > cover letters. >> + # message-id searches >> + url(r'^project/(?P<project_id>[^/]+)/message/(?P<msg_id>[^/]+)$', >> + patch_views.patch_by_project_msgid, >> + name='msgid-project-redirect'), > > Given my above comments, I wonder if we should split this into 'patch' > and 'cover' again? I realize it's more work, but I really do think > these should become the canonical links. I'm not 100% sure what you had in mind here; I'm guessing you mean that you want the ultimate URL scheme to be: For patches: The canonical URL - /project/N/patch/<msg_id> Accepted, redirects you to canonical URL - /project/N/patch/NNN Accepted, guesses a project and redirects - /patch/<msg_id> For covers: The canonical URL - /project/N/cover/<msg_id> Accepted, redirects you to canonical URL - /project/N/cover/NNN Accepted, guesses a project and redirects - /cover/<msg_id> (And I suppose if you get patch vs cover wrong it'll just redirect you to the right one.) For series, I'm even less clear on what you would like. My guess would be that we could have /project/N/series/<msg_id> just show you an abbreviated version of the entire series that msg_id is a part of. I think that would be super useful but would require some UI work (designing a template to show you a whole series) and ideally for the 1:N series change to land. It might be better for a follow up. Does that line up with what you had in mind? Regards, Daniel > > Stephen > >> + url(r'^message/(?P<msg_id>[^/]+)$', patch_views.patch_by_msgid, >> + name='msgid-redirect'), >> + >> # logged-in user stuff >> url(r'^user/$', user_views.profile, name='user-profile'), >> url(r'^user/todo/$', user_views.todo_lists, >> diff --git a/patchwork/views/patch.py b/patchwork/views/patch.py >> index 6921882e71d1..53441b39cc57 100644 >> --- a/patchwork/views/patch.py >> +++ b/patchwork/views/patch.py >> @@ -28,6 +28,7 @@ from django.urls import reverse >> >> from patchwork.forms import CreateBundleForm >> from patchwork.forms import PatchForm >> +from patchwork.models import Comment >> from patchwork.models import Bundle >> from patchwork.models import Patch >> from patchwork.models import Project >> @@ -150,3 +151,51 @@ def patch_mbox(request, patch_id): >> patch.filename) >> >> return response >> + >> + >> +def patch_by_msgid(request, msg_id): >> + msgid = ('<%s>' % msg_id) >> + >> + patches = Patch.objects.filter(msgid=msgid) >> + if patches: >> + return HttpResponseRedirect( >> + reverse('patch-detail', kwargs={'patch_id': patches.first().id})) >> + >> + subs = Submission.objects.filter(msgid=msgid) >> + if subs: >> + return HttpResponseRedirect( >> + reverse('cover-detail', kwargs={'cover_id': subs.first().id})) >> + >> + comments = Comment.objects.filter(msgid=msgid) >> + if comments: >> + return HttpResponseRedirect(comments.first().get_absolute_url()) >> + >> + raise Http404("No patch, cover letter of comment matching %s found." % msg_id) >> + >> + >> +def patch_by_project_msgid(request, project_id, msg_id): >> + project = get_object_or_404(Project, linkname=project_id) >> + project_id = project.id >> + msgid = ('<%s>' % msg_id) >> + >> + try: >> + patch = Patch.objects.get(patch_project_id=project_id, msgid=msgid) >> + return HttpResponseRedirect( >> + reverse('patch-detail', kwargs={'patch_id': patch.id})) >> + except Patch.DoesNotExist: >> + pass >> + >> + try: >> + sub = Submission.objects.get(project_id=project_id, msgid=msgid) >> + return HttpResponseRedirect( >> + reverse('cover-detail', kwargs={'cover_id': sub.id})) >> + except Submission.DoesNotExist: >> + pass >> + >> + try: >> + comment = Comment.objects.get(submission__project_id=project_id, msgid=msgid) >> + return HttpResponseRedirect(comment.get_absolute_url()) >> + except Comment.DoesNotExist: >> + pass >> + >> + raise Http404("No patch, cover letter or comment matching %s found in %s" % (msg_id, project.name))
diff --git a/patchwork/urls.py b/patchwork/urls.py index 6b1ef511ef15..90391b90e413 100644 --- a/patchwork/urls.py +++ b/patchwork/urls.py @@ -74,6 +74,13 @@ urlpatterns = [ url(r'^series/(?P<series_id>\d+)/mbox/$', series_views.series_mbox, name='series-mbox'), + # message-id searches + url(r'^project/(?P<project_id>[^/]+)/message/(?P<msg_id>[^/]+)$', + patch_views.patch_by_project_msgid, + name='msgid-project-redirect'), + url(r'^message/(?P<msg_id>[^/]+)$', patch_views.patch_by_msgid, + name='msgid-redirect'), + # logged-in user stuff url(r'^user/$', user_views.profile, name='user-profile'), url(r'^user/todo/$', user_views.todo_lists, diff --git a/patchwork/views/patch.py b/patchwork/views/patch.py index 6921882e71d1..53441b39cc57 100644 --- a/patchwork/views/patch.py +++ b/patchwork/views/patch.py @@ -28,6 +28,7 @@ from django.urls import reverse from patchwork.forms import CreateBundleForm from patchwork.forms import PatchForm +from patchwork.models import Comment from patchwork.models import Bundle from patchwork.models import Patch from patchwork.models import Project @@ -150,3 +151,51 @@ def patch_mbox(request, patch_id): patch.filename) return response + + +def patch_by_msgid(request, msg_id): + msgid = ('<%s>' % msg_id) + + patches = Patch.objects.filter(msgid=msgid) + if patches: + return HttpResponseRedirect( + reverse('patch-detail', kwargs={'patch_id': patches.first().id})) + + subs = Submission.objects.filter(msgid=msgid) + if subs: + return HttpResponseRedirect( + reverse('cover-detail', kwargs={'cover_id': subs.first().id})) + + comments = Comment.objects.filter(msgid=msgid) + if comments: + return HttpResponseRedirect(comments.first().get_absolute_url()) + + raise Http404("No patch, cover letter of comment matching %s found." % msg_id) + + +def patch_by_project_msgid(request, project_id, msg_id): + project = get_object_or_404(Project, linkname=project_id) + project_id = project.id + msgid = ('<%s>' % msg_id) + + try: + patch = Patch.objects.get(patch_project_id=project_id, msgid=msgid) + return HttpResponseRedirect( + reverse('patch-detail', kwargs={'patch_id': patch.id})) + except Patch.DoesNotExist: + pass + + try: + sub = Submission.objects.get(project_id=project_id, msgid=msgid) + return HttpResponseRedirect( + reverse('cover-detail', kwargs={'cover_id': sub.id})) + except Submission.DoesNotExist: + pass + + try: + comment = Comment.objects.get(submission__project_id=project_id, msgid=msgid) + return HttpResponseRedirect(comment.get_absolute_url()) + except Comment.DoesNotExist: + pass + + raise Http404("No patch, cover letter or comment matching %s found in %s" % (msg_id, project.name))
Add /message/<msgid> and /project/<project>/message/<msgid> URLs. /project/<project>/message/<msgid> finds the patch, cover-letter or comment in <project> that matches <msgid> and redirects you to it. This is guaranteed to be unique. /message/<msgid> does not require a project, but therefore if a mail has been sent to multiple projects, the project that you will be redirected to is arbitrary. This patch doesn't expose these URLs in the UI anywhere. I'd love to know where people would like them to go. I hesitate to make the message-id clickable in the patch detail view, as I think that would break workflows for people who want to copy the msgid to the clipboard. These URLs also just redirect to the usual canonical patchwork URL - it doesn't supplant or replace them. However, I'd be open to moving to this scheme as the 'normal'/default URL in the future. I also haven't attempted to do anything meaningful with series. Partially-closes: #106 Reported-by: Konstantin Ryabitsev <konstantin@linuxfoundation.org> Reported-by: Linus Torvalds <torvalds@linux-foundation.org> Reported-by: Stephen Finucane <stephen@that.guru> Signed-off-by: Daniel Axtens <dja@axtens.net> --- patchwork/urls.py | 7 ++++++ patchwork/views/patch.py | 49 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+)