Re: [PATCH 2/2] format-patch: --inline-single

2013-02-22 Thread Adam Spiers 
On Thu, Feb 21, 2013 at 06:13:28PM -0500, Jeff King wrote:
 On Thu, Feb 21, 2013 at 12:26:22PM -0800, Junio C Hamano wrote:
 
  Some people may find it convenient to append a simple patch at the
  bottom of a discussion e-mail separated by a scissors mark, ready
  to be applied with git am -c.  Introduce --inline-single option
  to format-patch to do so.  A typical usage example might be to start
  'F'ollow-up to a discussion, write your message, conclude with a
  patch to do so may look like this., and
  
  \C-u M-! git format-patch --inline-single -1 HEAD ENTER
  
  if you are an Emacs user.  Users of other MUA's may want to consult
  their manuals to find equivalent command to append output from an
  external command to the message being composed.
 
 Interesting. I usually just do this by hand, but this could save a few
 keystrokes in my workflow.

Same here.  This is great; thanks a lot both for working on it!
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] format-patch: --inline-single

2013-02-22 Thread Junio C Hamano 
Jeff King p...@peff.net writes:

 ... helper function to see if the user is the author ...
 +}

 Nice, I'm glad you handled this case properly. I've wondered if we
 should have an option to do a similar test when writing out the real
 message format. I.e., to put the extra From line in the body of the
 message when !is_current_user(). Traditionally we have just said that
 is the responsibility of the MUA you use, and let send-email handle it.
 But it means people who do not use send-email have to reimplement the
 feature themselves.

I am not sure if I follow.  Do you mean that you have to remove
fewer lines if you omit Date/From when it is from you in the first
place?  People who do not use send-email (like me) slurp the output
0001-have-gostak-distim-doshes.patch into their MUA editor, tell the
MUA to use the contents on the Subject: line as the subject, and
remove what is redundant, including the Subject.  Because the output
cannot be used as-is anyway, I do not think it is such a big deal.

And those who have a custom mechanism to stuff our output in their
MUA's outbox, similar to what imap-send does, would already have to
have a trivial parser to read the first part of our output up to the
first blank line (i.e. parsing out the header part) and formatting
the information it finds into a form that is understood by their
MUA.  Omitting From: or Date: lines would not help those people who
already have established the procedure to handle the Oh, this one
is from me case, or to send the output always with the Sender: and
keeping the From: intact.  So,...

 
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] format-patch: --inline-single

2013-02-22 Thread Jeff King 
On Fri, Feb 22, 2013 at 08:47:39AM -0800, Junio C Hamano wrote:

 Jeff King p...@peff.net writes:
 
  ... helper function to see if the user is the author ...
  +}
 
  Nice, I'm glad you handled this case properly. I've wondered if we
  should have an option to do a similar test when writing out the real
  message format. I.e., to put the extra From line in the body of the
  message when !is_current_user(). Traditionally we have just said that
  is the responsibility of the MUA you use, and let send-email handle it.
  But it means people who do not use send-email have to reimplement the
  feature themselves.
 
 I am not sure if I follow.  Do you mean that you have to remove
 fewer lines if you omit Date/From when it is from you in the first
 place?

Sorry, I think I confused you by going off on a tangent. The rest of my
email was about dropping unnecessary lines from the inline view.  But
here I was talking about another possible use of the is user the
author function. For the existing view, we show:

  From: A U Thor aut...@example.com
  Date: ...
  Subject: [PATCH] whatever

  body

and if committer != author, we expect the MUA to convert that to:

  From: C O Mitter commit...@example.com
  Date: ...
  Subject: [PATCH] whatever

  From: A U Thor aut...@example.com

  body

That logic happens in git-send-email right now, but given that your
patch adds the are we the author? function, it would be trivial to add
a --sender-is-committer option to format-patch to have it do it
automatically. That saves the MUA from having to worry about it.

 People who do not use send-email (like me) slurp the output
 0001-have-gostak-distim-doshes.patch into their MUA editor, tell the
 MUA to use the contents on the Subject: line as the subject, and
 remove what is redundant, including the Subject.  Because the output
 cannot be used as-is anyway, I do not think it is such a big deal.

That is one way to do it. Another way is to hand the output of
format-patch to your MUA as a template, making it a starting point for a
message we are about to send. No manual editing is necessary in that
case, unless the From header does not match the sender identity.

 And those who have a custom mechanism to stuff our output in their
 MUA's outbox, similar to what imap-send does, would already have to
 have a trivial parser to read the first part of our output up to the
 first blank line (i.e. parsing out the header part) and formatting
 the information it finds into a form that is understood by their
 MUA.

Not necessarily. The existing format is an rfc822 message, which mailers
understand already. It's perfectly cromulent to do:

  git format-patch --stdout $@ mbox 
  mutt -f mbox

and use mutt's resend-message as a starting point for sending each
message. No editing is necessary except for adding recipients (which you
can also do on the command-line to format-patch).

 Omitting From: or Date: lines would not help those people who
 already have established the procedure to handle the Oh, this one
 is from me case, or to send the output always with the Sender: and
 keeping the From: intact.  So,...

Right, my point was to help people who _should_ have implemented the
oh, this one is from me case, but were too lazy to do so (and it's
actually a little tricky to get right, because you might have to adjust
the mime headers to account for encoded author names).

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] format-patch: --inline-single

2013-02-21 Thread Junio C Hamano 
Some people may find it convenient to append a simple patch at the
bottom of a discussion e-mail separated by a scissors mark, ready
to be applied with git am -c.  Introduce --inline-single option
to format-patch to do so.  A typical usage example might be to start
'F'ollow-up to a discussion, write your message, conclude with a
patch to do so may look like this., and 

\C-u M-! git format-patch --inline-single -1 HEAD ENTER

if you are an Emacs user.  Users of other MUA's may want to consult
their manuals to find equivalent command to append output from an
external command to the message being composed.

It does not make any sense to use this mode when formatting multiple
patches, or to combine this with options such as --attach, --inline,
and --cover-letter, so some of such uses are forbidden.  There may
be more insane combination the check in this patch may not even
bother to reject.  Caveat emptor.

Signed-off-by: Junio C Hamano gits...@pobox.com
---

 * I did this as a lunch-time hack, but I'll leave it to interested
   readers as an exercise to find corner case bugs, e.g. some
   insane combinations of options may not be diagnosed as usage
   errors, and to update the tests and documentation.

   Personally, git format-patch --stdout -1 HEAD with manual
   editing is more flexible, so I am not interested in spending
   cycles to polish this further myself.

   The preliminary patch 1/2 I sent earlier is worth doing, though.

 builtin/log.c | 32 
 commit.h  |  1 +
 log-tree.c|  7 ++-
 pretty.c  | 27 ++-
 revision.h|  1 +
 5 files changed, 66 insertions(+), 2 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 30265d8..5ad0837 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1000,6 +1000,19 @@ static int inline_callback(const struct option *opt, 
const char *arg, int unset)
return 0;
 }
 
+static int inline_single_callback(const struct option *opt, const char *arg, 
int unset)
+{
+   struct rev_info *rev = (struct rev_info *)opt-value;
+   rev-mime_boundary = NULL;
+   rev-inline_single = 1;
+
+   /* defeat configured format.attach, format.thread, etc. */
+   free(default_attach);
+   default_attach = NULL;
+   thread = 0;
+   return 0;
+}
+
 static int header_callback(const struct option *opt, const char *arg, int 
unset)
 {
if (unset) {
@@ -1149,6 +1162,10 @@ int cmd_format_patch(int argc, const char **argv, const 
char *prefix)
PARSE_OPT_OPTARG, thread_callback },
OPT_STRING(0, signature, signature, N_(signature),
N_(add a signature)),
+   { OPTION_CALLBACK, 0, inline-single, rev, NULL,
+ N_(single patch appendable to the end of an e-mail body),
+ PARSE_OPT_NOARG | PARSE_OPT_NONEG,
+ inline_single_callback },
OPT_BOOLEAN(0, quiet, quiet,
N_(don't print the patch filenames)),
OPT_END()
@@ -1185,6 +1202,17 @@ int cmd_format_patch(int argc, const char **argv, const 
char *prefix)
 PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN |
 PARSE_OPT_KEEP_DASHDASH);
 
+   /* Set defaults and check incompatible options */
+   if (rev.inline_single) {
+   use_stdout = 1;
+   if (cover_letter)
+   die(_(inline-single and cover-letter are 
incompatible.));
+   if (thread)
+   die(_(inline-single and thread are incompatible.));
+   if (output_directory)
+   die(_(inline-single and output-directory are 
incompatible.));
+   }
+
if (0  reroll_count) {
struct strbuf sprefix = STRBUF_INIT;
strbuf_addf(sprefix, %s v%d,
@@ -1373,6 +1401,10 @@ int cmd_format_patch(int argc, const char **argv, const 
char *prefix)
list[nr - 1] = commit;
}
total = nr;
+
+   if (rev.inline_single  total != 1)
+   die(_(inline-single is only for a single commit));
+
if (!keep_subject  auto_number  total  1)
numbered = 1;
if (numbered)
diff --git a/commit.h b/commit.h
index 4138bb4..f3d9959 100644
--- a/commit.h
+++ b/commit.h
@@ -85,6 +85,7 @@ struct pretty_print_context {
int preserve_subject;
enum date_mode date_mode;
unsigned date_mode_explicit:1;
+   unsigned inline_single:1;
int need_8bit_cte;
char *notes_message;
struct reflog_walk_info *reflog_info;
diff --git a/log-tree.c b/log-tree.c
index 34ec20d..15c9749 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -358,7 +358,11 @@ void log_write_email_headers(struct rev_info *opt, struct 
commit *commit,
subject = Subject: ;
}
 
-   printf(From %s Mon Sep 17 00:00:00 2001\n, name);
+   if

Re: [PATCH 2/2] format-patch: --inline-single

2013-02-21 Thread Jeff King 
On Thu, Feb 21, 2013 at 12:26:22PM -0800, Junio C Hamano wrote:

 Some people may find it convenient to append a simple patch at the
 bottom of a discussion e-mail separated by a scissors mark, ready
 to be applied with git am -c.  Introduce --inline-single option
 to format-patch to do so.  A typical usage example might be to start
 'F'ollow-up to a discussion, write your message, conclude with a
 patch to do so may look like this., and
 
 \C-u M-! git format-patch --inline-single -1 HEAD ENTER
 
 if you are an Emacs user.  Users of other MUA's may want to consult
 their manuals to find equivalent command to append output from an
 external command to the message being composed.

Interesting. I usually just do this by hand, but this could save a few
keystrokes in my workflow.

 +static int is_current_user(const struct pretty_print_context *pp,
 +const char *email, size_t emaillen,
 +const char *name, size_t namelen)
 +{
 + const char *me = git_committer_info(0);
 + const char *myname, *mymail;
 + size_t mynamelen, mymaillen;
 + struct ident_split ident;
 +
 + if (split_ident_line(ident, me, strlen(me)))
 + return 0; /* play safe, as we do not know */
 + mymail = ident.mail_begin;
 + mymaillen = ident.mail_end - ident.mail_begin;
 + myname = ident.name_begin;
 + mynamelen = ident.name_end - ident.name_begin;
 + if (pp-mailmap)
 + map_user(pp-mailmap, mymail, mymaillen, myname, mynamelen);
 + return (mymaillen == emaillen 
 + mynamelen == namelen 
 + !memcmp(mymail, email, emaillen) 
 + !memcmp(myname, name, namelen));
 +}

Nice, I'm glad you handled this case properly. I've wondered if we
should have an option to do a similar test when writing out the real
message format. I.e., to put the extra From line in the body of the
message when !is_current_user(). Traditionally we have just said that
is the responsibility of the MUA you use, and let send-email handle it.
But it means people who do not use send-email have to reimplement the
feature themselves.

 @@ -421,6 +443,9 @@ void pp_user_info(const struct pretty_print_context *pp,
   if (pp-mailmap)
   map_user(pp-mailmap, mailbuf, maillen, namebuf, namelen);
  
 + if (pp-inline_single  is_current_user(pp, mailbuf, maillen, namebuf, 
 namelen))
 + return;
 +
   strbuf_init(mail, 0);
   strbuf_init(name, 0);

This makes sense to suppress the user line when it is not necessary. But
we should probably always be suppressing the Date line, as it is almost
always useless.

I also wonder if we should suppress the subject-prefix in such a case,
as it is not adding anything (it is not the subject of the email, so it
does not need to grab attention there, and it will not make it into the
final commit). On the other hand, having tried it, the Subject: looks
a little lonely without it. Perhaps the [PATCH] is still necessary to
grab attention after the scissors line. I dunno.

Patch for both below if you want to pick up either suggestion.

diff --git a/log-tree.c b/log-tree.c
index 15c9749..8994354 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -348,7 +348,8 @@ void log_write_email_headers(struct rev_info *opt, struct 
commit *commit,
 digits_in_number(opt-total),
 opt-nr, opt-total);
subject = buffer;
-   } else if (opt-total == 0  opt-subject_prefix  
*opt-subject_prefix) {
+   } else if (opt-total == 0  !opt-inline_single 
+  opt-subject_prefix  *opt-subject_prefix) {
static char buffer[256];
snprintf(buffer, sizeof(buffer),
 Subject: [%s] ,
diff --git a/pretty.c b/pretty.c
index 363b3d9..1a7352c 100644
--- a/pretty.c
+++ b/pretty.c
@@ -490,7 +490,8 @@ void pp_user_info(const struct pretty_print_context *pp,
strbuf_addf(sb, Date:   %s\n, show_date(time, tz, 
pp-date_mode));
break;
case CMIT_FMT_EMAIL:
-   strbuf_addf(sb, Date: %s\n, show_date(time, tz, 
DATE_RFC2822));
+   if (!pp-inline_single)
+   strbuf_addf(sb, Date: %s\n, show_date(time, tz, 
DATE_RFC2822));
break;
case CMIT_FMT_FULLER:
strbuf_addf(sb, %sDate: %s\n, what, show_date(time, tz, 
pp-date_mode));
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] format-patch: --inline-single

2013-02-21 Thread Junio C Hamano 
Jeff King p...@peff.net writes:

 @@ -421,6 +443,9 @@ void pp_user_info(const struct pretty_print_context *pp,
  if (pp-mailmap)
  map_user(pp-mailmap, mailbuf, maillen, namebuf, namelen);
  
 +if (pp-inline_single  is_current_user(pp, mailbuf, maillen, namebuf, 
 namelen))
 +return;
 +
  strbuf_init(mail, 0);
  strbuf_init(name, 0);

 This makes sense to suppress the user line when it is not necessary. But
 we should probably always be suppressing the Date line, as it is almost
 always useless.

When I (figuratively) am sending my patch in a discussion, saying
You could do it this way, on the other hand, I agree that the date
is uninteresting.

I however think I would prefer to keep the Date: line when I am
relaying somebody else's work during a discussion.  It is more like
Yeah, Peff already did that with this commit; here it is for
reference. The fact that I have _your_ patch makes it more done,
than the case I send out my own patch.

Besides, removing an extra line in the MUA editor is far easier than
having to type what the tool helpfully omitted, guided by an it
is almost always useless that is not backed by the user preference.
I'd rather err on the side of giving extra than omitting too much.

 I also wonder if we should suppress the subject-prefix in such a case,
 as it is not adding anything (it is not the subject of the email, so it
 does not need to grab attention there, and it will not make it into the
 final commit).

If the user does not want to waste too much space in the message,
not passing the --subject-prefix=foo from the command line, or
editing it out in the editor buffer if for some reason the user ran
the command with the option, are both easy things to do.  I do not
think extra lines to excise subject prefix is not worth it, and who
knows what the user's preferences are.

But there is something more important.

We should make sure that we disable MIMEy stuff (i.e. MIME-Version,
C-T-E: 8bit/quoted-printable, Content-type, etc.) when producing the
output to be appended to the body, which should be just a straight
8-bit text.  I do not think the posted patch tries to do anything to
that effect.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] format-patch: --inline-single

2013-02-21 Thread Junio C Hamano 
Junio C Hamano gits...@pobox.com writes:

 Jeff King p...@peff.net writes:

 @@ -421,6 +443,9 @@ void pp_user_info(const struct pretty_print_context *pp,
 if (pp-mailmap)
 map_user(pp-mailmap, mailbuf, maillen, namebuf, namelen);
  
 +   if (pp-inline_single  is_current_user(pp, mailbuf, maillen, namebuf, 
 namelen))
 +   return;
 +
 strbuf_init(mail, 0);
 strbuf_init(name, 0);

 This makes sense to suppress the user line when it is not necessary. But
 we should probably always be suppressing the Date line, as it is almost
 always useless.

 When I (figuratively) am sending my patch in a discussion, saying
 You could do it this way, on the other hand, I agree that the date
 is uninteresting.

Just in case somebody is wondering, please s/, on the other hand//;
above.  I swapped the paragraphs after I wrote them X-.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html