226404 - Editing mime type on an attachment flagged with "patch" doesn't work

Status: NEW

(Bugzilla :: Attachments & Requests, defect)

Description

[Jouni Heikniemi]

If I set a mime type on an attachment flagged with "patch", nothing happens.
Perhaps the UI should be tuned so that patch and mime type selections are
exclusive (like in "create attachment").

Comment 1

[victory <[e-mail]>]

content-type isn't needed for patches,
 but it should be able to specify its charset
 so multi-byte src can always be shown correctly
 without using browser's feature

perhaps to make charset field is better solution?

Comment 2

[victory <[e-mail]>]

Attachment: patch for tip

Comment 4

[Teemu Mannermaa (:wicked)]

Comment on attachment 250087 [details] [diff] [review]
patch for tip

While adding new charset field might be a good idea, this still doesn't fix the reported problem.  Maybe in addition fix suggested in comment #0 should be done.

Also Character Set field should be enabled/disabled with JS just like content type field is on the attachment entry page.

Comment 5

[Gerald Combs]

In reply to comment #1, content-type _is_ needed for patches, particularly when someone uploads a gzipped patch, e.g.
http://bugs.wireshark.org/bugzilla/show_bug.cgi?id=1606

When this happens I have to un-mark the attachment as a patch and manually set its content-type, otherwise a bunch of gzipped data gets displayed as text.  Shouldn't this be handled more gracefully?

Comment 6

[timeless]

[e-mail]: what do you mean the attachment is patch when you specify gzip content?

from my perspective, attachment is patch means that patchreader should try to operate on it. no more, no less.

are you actually doing queries to decide which bugs have patches that could need reviews? or what? please indicate your use case.

attachment is gzip/zip means that the bug covers something big and needs more careful manual review than you can possibly do from inside Bugzilla.

- I'm speaking as someone who has posted/reviewed big patches at both sizes. You really don't want to try to review something that can't fit the limit using a web browser.

Comment 7

[Jeff Morriss]

(In reply to comment #6)
> gerald[AT]wireshark.org: what do you mean the attachment is patch when you specify
> gzip content?

We (over at wireshark.org) have reasonably frequently see that people attach patches (that is "a modification to the source code") but they gzip the patch first for whatever reason (because it's big, because they're afraid their browser will DOS(CR/LF)-ify it, whatever) but they still click the "Attachment is a patch" button.
 
> from my perspective, attachment is patch means that patchreader should try to
> operate on it. no more, no less.

I think that's the source of confusion: some people think "patch" means "a patch to the source code" whereas bugzilla means "it's something [probably a patch to the source code] in patch format [that is: text/plain and in diff format]".

Comment 8

[Gerald Combs]

To illustrate Jeff's comments above, suppose I'm about to upload a multi-megabyte compressed patch file. Bugzilla displays the following text:


  Content Type: If the attachment is a patch, check the box below.
                [] patch


Do I click the box or not?

I've modified createformcontents.html.tmpl on bugs.wireshark.org to display "If the attachment is a small, uncompressed patch...", but it would be nice if Bugzilla could verify that files marked "patch" actually looked like patch files.

Comment 9

[Frédéric Buclin]

(In reply to comment #6)
> from my perspective, attachment is patch means that patchreader should try to
> operate on it. no more, no less.

timeless is right, that's what the "patch" attribute is used for. So in this case the MIME type *must* be text/plain. Else PatchReader won't be able to display it.

Comment 11

[Gordon P. Hemsley [:GPHemsley]]

As others in this bug attest, patches are not always text/plain. They could be gzipped for compression, or there may be multiple patches in a single archive file.

It is not intuitively clear that the "patch" checkbox means "PatchReader should operate on this attachment" rather than "this attachment contains a patch".

But either way, checking the "patch" box should not be blinding overriding the specified MIME type after you submit the form. And this goes double if you are clearly also attempting to change the MIME type.

If you intend to maintain the purpose of the checkbox as only a signal to PatchReader, I would recommend improving the label to make that clear.

At the very least, checking the "patch" box should change the MIME type to "Text/plain" before the form is submitted (via JavaScript), and if the form is submitted with the "patch" box checked and with a different MIME type than it was displayed with, the user should be given a message that the attachment was forced to "text/plain" because the "patch" box was checked.

It might also be useful to allow the PatchReader to interpret gzipped patches, though that's probably a different bug.