Note: There are a few cases of duplicates in user autocompletion which are being worked on.

text/x-patch and text/x-diff should be mapped to [x] patch

RESOLVED FIXED in Bugzilla 3.0

Status

()

Bugzilla
Attachments & Requests
--
enhancement
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: timeless, Assigned: timeless)

Tracking

2.23.3
Bugzilla 3.0
Bug Flags:
approval +
approval3.0 +

Details

Attachments

(1 attachment, 2 obsolete attachments)

1.26 KB, patch
Frédéric Buclin
: review+
Details | Diff | Splinter Review
(Assignee)

Description

11 years ago
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

11 years ago
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...
OS: Windows XP → All
Hardware: PC → All
(Assignee)

Comment 2

11 years ago
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.
(Assignee)

Comment 3

11 years ago
Created attachment 251469 [details] [diff] [review]
autodetect patches
Assignee: attach-and-request → timeless
Status: NEW → ASSIGNED
Attachment #251469 - Flags: review?

Comment 4

11 years ago
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.
Target Milestone: --- → Bugzilla 3.0
(Assignee)

Updated

11 years ago
Attachment #251469 - Flags: review? → review?(LpSolit)

Comment 5

11 years ago
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.
Attachment #251469 - Flags: review?(LpSolit) → review-
(Assignee)

Comment 6

10 years ago
Created attachment 271436 [details] [diff] [review]
autodetect patches
Attachment #251469 - Attachment is obsolete: true
Attachment #271436 - Flags: review?(LpSolit)

Comment 7

10 years ago
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)
{
    ...
}
Attachment #271436 - Flags: review?(LpSolit) → review-
(Assignee)

Comment 8

10 years ago
Created attachment 272752 [details] [diff] [review]
autodetect patches with style
Attachment #271436 - Attachment is obsolete: true
Attachment #272752 - Flags: review?(LpSolit)

Comment 9

10 years ago
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.
Attachment #272752 - Flags: review?(LpSolit) → review+

Comment 10

10 years ago
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.
Flags: approval3.0+
Flags: approval+

Comment 11

10 years ago
tip:

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

3.0:

mozilla/webtools/bugzilla/Bugzilla/Attachment.pm 	1.45.2.1
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Updated

10 years ago
Keywords: relnote
Whiteboard: [relnote 3.0]

Comment 12

10 years ago
Added to 3.0 relnotes in bug 391233.
Whiteboard: [relnote 3.0]

Updated

10 years ago
Keywords: relnote
You need to log in before you can comment on or make changes to this bug.