requiring text format for patch attachments?

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

requiring text format for patch attachments?

Chris Metcalf-3
On 6/28/2013 6:39 AM, Marcus Shawcroft wrote:
> This patch fixes the AArch64 implementation of elf_machine_dynamic() to find
> _DYNAMIC via _GLOBAL_OFFSET_TABLE_ as discussed here:

I'm all for using attachments for patches, to preserve whitespace, etc., but I wonder whether it might make sense to require attachments for libc-alpha or libc-ports to be text/plain or text/x-patch rather than application/octet-stream or other non-text types (as Marcus did in the quoted email)?  Otherwise the patch doesn't get generally displayed inline by the MUA, and one has to separately run some external tool to view it.  Perhaps a mailing list filtering step that bounces emails with non-text attachments, with a helpful explanation?

It does seem like with the rise of GUI MUAs it has become harder to reliably paste patches into emails.  I end up writing emails that include patches in a text editor and then running "sendmail -f < myemail", which frankly, isn't a great user experience. :-)   To be fair, I just checked Thunderbird, and my configuration does seem to use "text/" types for diff attachments, so perhaps I'll switch to using that going forward.  I did double-check the wiki contribution checklist to confirm it says "patches inline or as attachments", so from that point of view we seem to be OK.

--
Chris Metcalf, Tilera Corp.
http://www.tilera.com

Reply | Threaded
Open this post in threaded view
|

Re: requiring text format for patch attachments?

Richard Henderson
On 06/28/2013 07:27 AM, Chris Metcalf wrote:
> It does seem like with the rise of GUI MUAs it has become harder to reliably
> paste patches into emails.  I end up writing emails that include patches in
> a text editor and then running "sendmail -f < myemail", which frankly, isn't
> a great user experience. :-)   To be fair, I just checked Thunderbird, and
> my configuration does seem to use "text/" types for diff attachments, so
> perhaps I'll switch to using that going forward.  I did double-check the
> wiki contribution checklist to confirm it says "patches inline or as
> attachments", so from that point of view we seem to be OK.

I'm using git-send-email for almost all of my patches these days.

Given that you can put a

[sendemail]
        to = "[hidden email]"

clause in the .git/config, I know that I'm sending the patches to
the right place if I do the send-email from within the repository
of the project.


r~
Reply | Threaded
Open this post in threaded view
|

Re: requiring text format for patch attachments?

Carlos O'Donell-2
In reply to this post by Chris Metcalf-3
On 06/28/2013 10:27 AM, Chris Metcalf wrote:

> On 6/28/2013 6:39 AM, Marcus Shawcroft wrote:
>> This patch fixes the AArch64 implementation of
>> elf_machine_dynamic() to find _DYNAMIC via _GLOBAL_OFFSET_TABLE_ as
>> discussed here:
>
> I'm all for using attachments for patches, to preserve whitespace,
> etc., but I wonder whether it might make sense to require attachments
> for libc-alpha or libc-ports to be text/plain or text/x-patch rather
> than application/octet-stream or other non-text types (as Marcus did
> in the quoted email)?  Otherwise the patch doesn't get generally
> displayed inline by the MUA, and one has to separately run some
> external tool to view it.  Perhaps a mailing list filtering step that
> bounces emails with non-text attachments, with a helpful
> explanation?

No, we must accept application/octet-stream because we post tarballs
of things to the list.

The solution is to remind the author that attaching patches to
issues is not correct per the contribution checklist.

> It does seem like with the rise of GUI MUAs it has become harder to
> reliably paste patches into emails.  I end up writing emails that
> include patches in a text editor and then running "sendmail -f <
> myemail", which frankly, isn't a great user experience. :-)   To be
> fair, I just checked Thunderbird, and my configuration does seem to
> use "text/" types for diff attachments, so perhaps I'll switch to
> using that going forward.  I did double-check the wiki contribution
> checklist to confirm it says "patches inline or as attachments", so
> from that point of view we seem to be OK.

If we want to support patches as attachments please send an email
to libc-alpha and ask for consensus around that, then document it
in the contribution checklist.

Then beat up people who submit non-text patches :-)

Cheers,
CArlos.
Reply | Threaded
Open this post in threaded view
|

Re: requiring text format for patch attachments?

Chris Metcalf-3
On 6/28/2013 3:01 PM, Carlos O'Donell wrote:
> On 06/28/2013 10:27 AM, Chris Metcalf wrote:
>> I did double-check the wiki contribution
>> checklist to confirm it says "patches inline or as attachments", so
>> from that point of view we seem to be OK.
> If we want to support patches as attachments please send an email
> to libc-alpha and ask for consensus around that, then document it
> in the contribution checklist.

Attachments are already supported per the contribution checklist (quoted below).  It might be useful to mention something about attachments being helpful to avoid whitespace damage too, in addition to wrapping damage.

> 13. Proper Formatted Unified diff of the Changes
>
>     - Only unified diff (-uNr) format is acceptable. Patches which are context diffs will not be reviewed.
>     - Unified diff must be in a format that can be applied with patch -p1.
>     - Included inline with the text of the email or as a separate attachment if your mailer wraps long lines.
>
> If you have any questions regarding these criteria please email [hidden email] .

--
Chris Metcalf, Tilera Corp.
http://www.tilera.com

Reply | Threaded
Open this post in threaded view
|

Re: requiring text format for patch attachments?

Paul Eggert
On 06/28/2013 12:19 PM, Chris Metcalf wrote:
> It might be useful to mention something about attachments being helpful to avoid
> whitespace damage too, in addition to wrapping damage.

I boldly installed the following wording into the checklist;
please feel free to fix it if I messed up.

 * You can include the patch as part of the text of the email; that's
 often easier.

 * If your mailer insists on wrapping long lines or otherwise altering
 patch contents, or if the diff might cause problems due to binary
 data or mixed encodings, attach the patch to the email.  The
 attachment should normally employ a Content-Type of text/x-diff or
 text/x-patch (or at least text/plain) and a charset of UTF-8 or
 US-ASCII; if it contains binary data or mixed encoding it may need to
 specify a Content-Type of application/octet-stream instead.  Please
 use an attachment file name that ends in ".diff" or ".patch" (or at
 least ".txt").

Reply | Threaded
Open this post in threaded view
|

Re: requiring text format for patch attachments?

Carlos O'Donell-6
On 06/28/2013 05:09 PM, Paul Eggert wrote:

> On 06/28/2013 12:19 PM, Chris Metcalf wrote:
>> It might be useful to mention something about attachments being helpful to avoid
>> whitespace damage too, in addition to wrapping damage.
>
> I boldly installed the following wording into the checklist;
> please feel free to fix it if I messed up.
>
>  * You can include the patch as part of the text of the email; that's
>  often easier.
>
>  * If your mailer insists on wrapping long lines or otherwise altering
>  patch contents, or if the diff might cause problems due to binary
>  data or mixed encodings, attach the patch to the email.  The
>  attachment should normally employ a Content-Type of text/x-diff or
>  text/x-patch (or at least text/plain) and a charset of UTF-8 or
>  US-ASCII; if it contains binary data or mixed encoding it may need to
>  specify a Content-Type of application/octet-stream instead.  Please
>  use an attachment file name that ends in ".diff" or ".patch" (or at
>  least ".txt").
>

+1.

c.
Reply | Threaded
Open this post in threaded view
|

Re: requiring text format for patch attachments?

Roland McGrath-4
> >  * You can include the patch as part of the text of the email; that's
> >  often easier.

It's the strong preference that people not use attachments.  People should
include patches directly in the text unless their MUA unavoidably munges
them.
Reply | Threaded
Open this post in threaded view
|

Re: requiring text format for patch attachments?

Paul Eggert
On 06/28/2013 02:50 PM, Roland McGrath wrote:

> It's the strong preference that people not use attachments.

OK, thanks, I changed the text so that it now reads as follows.

 * The strong preference is that you include the patch as part of
   the text of the email.  However, if your mailer unavoidably
   wraps long lines or otherwise alters patch contents, or if the
   diff might cause problems due to binary data or mixed
   encodings, attach the patch to the email.  The attachment
   should normally employ a Content-Type of text/x-diff or
   text/x-patch (or at least text/plain) and a charset of UTF-8 or
   US-ASCII; if it contains binary data or mixed encoding it may
   need to specify a Content-Type of application/octet-stream
   instead.  Please use an attachment file name that ends in
   ".diff" or ".patch" (or at least ".txt").