Closed Bug 365756 Opened 18 years ago Closed 17 years ago

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

Categories

(Bugzilla :: Attachments & Requests, enhancement)

2.23.3
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 3.0

People

(Reporter: timeless, Assigned: timeless)

Details

Attachments

(1 file, 2 obsolete files)

if someone tries to attach a file and their browser tells us it's a patch/diff, we should automatically mark it as such.
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
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.
Attached patch autodetect patches (obsolete) — Splinter Review
Assignee: attach-and-request → timeless
Status: NEW → ASSIGNED
Attachment #251469 - Flags: review?
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
Attachment #251469 - Flags: review? → review?(LpSolit)
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-
Attached patch autodetect patches (obsolete) — Splinter Review
Attachment #251469 - Attachment is obsolete: true
Attachment #271436 - Flags: review?(LpSolit)
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-
Attachment #271436 - Attachment is obsolete: true
Attachment #272752 - Flags: review?(LpSolit)
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+
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+
tip:

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

3.0:

mozilla/webtools/bugzilla/Bugzilla/Attachment.pm 	1.45.2.1
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Keywords: relnote
Whiteboard: [relnote 3.0]
Added to 3.0 relnotes in bug 391233.
Whiteboard: [relnote 3.0]
Keywords: relnote
You need to log in before you can comment on or make changes to this bug.