Last Comment Bug 365756 - text/x-patch and text/x-diff should be mapped to [x] patch
: text/x-patch and text/x-diff should be mapped to [x] patch
Status: RESOLVED FIXED
:
Product: Bugzilla
Classification: Server Software
Component: Attachments & Requests (show other bugs)
: 2.23.3
: All All
: -- enhancement (vote)
: Bugzilla 3.0
Assigned To: timeless
: default-qa
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-02 23:34 PST by timeless
Modified: 2008-01-04 06:15 PST (History)
2 users (show)
LpSolit: approval+
LpSolit: approval3.0+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
autodetect patches (1.04 KB, patch)
2007-01-14 15:09 PST, timeless
LpSolit: review-
Details | Diff | Splinter Review
autodetect patches (1.17 KB, patch)
2007-07-08 11:54 PDT, timeless
LpSolit: review-
Details | Diff | Splinter Review
autodetect patches with style (1.26 KB, patch)
2007-07-18 01:16 PDT, timeless
LpSolit: review+
Details | Diff | Splinter Review

Description timeless 2007-01-02 23:34:43 PST
if someone tries to attach a file and their browser tells us it's a patch/diff, we should automatically mark it as such.
Comment 1 Max Kanat-Alexander 2007-01-03 14:50:43 PST
I think sometimes people want to attach patches and don't want them to be treated as patches. I don't know why they'd want to do this, though...
Comment 2 timeless 2007-01-14 06:49:20 PST
i don't think that's relevant. if they want to do that, they can check the manual mime type. this is only for automatic detection.

it really bothers me that I can't use the diff action unless i mark an attachment as a patch.
Comment 3 timeless 2007-01-14 15:09:39 PST
Created attachment 251469 [details] [diff] [review]
autodetect patches
Comment 4 Frédéric Buclin 2007-01-23 10:43:28 PST
I'm with timeless. The only reason I always check the "patch" checkbox is because Firefox detects my patch as text/x-patch and I cannot view it from the browser, not use the Diff feature.
Comment 5 Frédéric Buclin 2007-01-25 08:10:21 PST
Comment on attachment 251469 [details] [diff] [review]
autodetect patches

>+    if (defined $cgi->param('contenttypemethod') &&
>+        defined $cgi->param('contenttype') &&
>+        $cgi->param('contenttypemethod') eq 'autodetect' &&
>+        $cgi->param('contenttype') =~ m{text/x-(?:diff|patch)}

validate_is_patch() is called before validate_content_type() and so at this point $cgi->param('contenttype') is still undefined.
Comment 6 timeless 2007-07-08 11:54:00 PDT
Created attachment 271436 [details] [diff] [review]
autodetect patches
Comment 7 Frédéric Buclin 2007-07-09 10:11:37 PDT
Comment on attachment 271436 [details] [diff] [review]
autodetect patches

>+            if (defined $cgi->param('contenttypemethod') &&
>+                defined $cgi->param('contenttype') &&
>+                $cgi->param('contenttypemethod') eq 'autodetect' &&
>+                $cgi->param('contenttype') =~ m{text/x-(?:diff|patch)}
>+            ) {
>+                $cgi->param('ispatch', 1);
>+            }

You have to set 'contenttype' to 'text/plain', else when you go to the 'Details' page, Bugzilla tries to download the patch instead of displaying it. So you can view the patch in 'Diff' mode, but you cannot review/edit it. Not really useful.

Nit: also, please use the Bugzilla standards and put && at the beginning of lines, instead of at the end, and also put the { braket on a new line:

if ($a_very_long_line
    && $another_very_long_line)
{
    ...
}
Comment 8 timeless 2007-07-18 01:16:05 PDT
Created attachment 272752 [details] [diff] [review]
autodetect patches with style
Comment 9 Frédéric Buclin 2007-07-19 06:27:23 PDT
Comment on attachment 272752 [details] [diff] [review]
autodetect patches with style

>Index: mozilla/webtools/bugzilla/Bugzilla/Attachment.pm

>+            if (defined $cgi->param('contenttypemethod')
>+                && defined $cgi->param('contenttype')
>+                && $cgi->param('contenttypemethod') eq 'autodetect'
>+                && $cgi->param('contenttype') =~ m{text/x-(?:diff|patch)})

I forgot to mention this in my previous patch (because I didn't check), but you don't need to check if 'contenttypemethod' and 'contenttype' are defined. They are in all cases, else an error is thrown by validate_content_type() the line above this condition. So you can safely remove these two lines.

Your patch is working fine. r=LpSolit with the two lines above removed.
Comment 10 Frédéric Buclin 2007-07-19 06:33:08 PDT
This patch doesn't apply cleanly on the 3.0 branch. All you have to do is to append '0' after 'return' the line above your IF condition: || return 0. Here too, do not forget to remove the two lines checking for "defined $cgi->param(foo)", we already know they are defined.
Comment 11 Frédéric Buclin 2007-07-20 05:03:30 PDT
tip:

mozilla/webtools/bugzilla/Bugzilla/Attachment.pm 	1.47

3.0:

mozilla/webtools/bugzilla/Bugzilla/Attachment.pm 	1.45.2.1
Comment 12 Max Kanat-Alexander 2007-08-13 13:19:28 PDT
Added to 3.0 relnotes in bug 391233.

Note You need to log in before you can comment on or make changes to this bug.