{"thread":{"id":"36706","subject":"[PATCH v5] format-patch --signature-file ","startedAt":"2014-05-20T08:00:05Z","lastAt":"2014-05-21T21:14:14Z","messageCount":15,"participants":["Jeremiah Mahler","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":5,"patchTotal":null},"messages":[{"id":"242229","messageId":"1400572806-21092-1-git-send-email-jmmahler@gmail.com","threadId":"36706","inReplyTo":null,"subject":"[PATCH v5] format-patch --signature-file ","fromName":"Jeremiah Mahler","fromEmail":"jmmahler@gmail.com","sentAt":"2014-05-20T08:00:05Z","receivedAt":"2014-05-20T08:00:05Z","isPatch":true,"sender":{"key":"jmmahler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/154028?v=4"},"body":"v5 of patch to add format-patch --signature-file option.\n\nThis revision includes more suggestions from Jeff King and Junio C Hamano:\n\n - Use git_config_pathname instead of git_config_string for ~ expansion.\n\n - Eliminated head/tail --lines which is not POSIX compliant.\n Replaced with sed equivalents.\n\n - Used test_config instead of git config which also handles unsetting.\n\n - Use a DEFAULT_SIGNATURE variable to better describe its purpose\n instead of git_version_string which could be confusing.\n\n\nJeremiah Mahler (1):\n format-patch --signature-file \n\n Documentation/config.txt | 4 ++++\n Documentation/git-format-patch.txt | 4 ++++\n builtin/log.c | 19 ++++++++++++++++++-\n t/t4014-format-patch.sh | 32 ++++++++++++++++++++++++++++++++\n 4 files changed, 58 insertions(+), 1 deletion(-)\n\n-- \nJeremiah Mahler\njmmahler@gmail.com\nhttp://github.com/jmahler\n"},{"id":"242233","messageId":"1400572806-21092-2-git-send-email-jmmahler@gmail.com","threadId":"36706","inReplyTo":"1400572806-21092-1-git-send-email-jmmahler@gmail.com","subject":"[PATCH v5] format-patch --signature-file ","fromName":"Jeremiah Mahler","fromEmail":"jmmahler@gmail.com","sentAt":"2014-05-20T08:00:06Z","receivedAt":"2014-05-20T08:00:06Z","isPatch":true,"sender":{"key":"jmmahler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/154028?v=4"},"body":"Added option that allows a signature file to be used with format-patch\nso that signatures with newlines and other special characters can be\neasily included.\n\n $ git format-patch --signature-file ~/.signature -1\n\nThe config variable format.signaturefile is also provided so that it\ncan be added by default.\n\n $ git config format.signaturefile ~/.signature\n\n $ git format-patch -1\n\nSigned-off-by: Jeremiah Mahler \n---\n Documentation/config.txt | 4 ++++\n Documentation/git-format-patch.txt | 4 ++++\n builtin/log.c | 19 ++++++++++++++++++-\n t/t4014-format-patch.sh | 32 ++++++++++++++++++++++++++++++++\n 4 files changed, 58 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 1932e9b..140ed77 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1114,6 +1114,10 @@ format.signature::\n \tSet this variable to the empty string (\"\") to suppress\n \tsignature generation.\n \n+format.signaturefile::\n+\tWorks just like format.signature except the contents of the\n+\tfile specified by this variable will be used as the signature.\n+\n format.suffix::\n \tThe default for format-patch is to output files with the suffix\n \t`.patch`. Use this variable to change that suffix (make sure to\ndiff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt\nindex 5c0a4ab..c0fd470 100644\n--- a/Documentation/git-format-patch.txt\n+++ b/Documentation/git-format-patch.txt\n@@ -14,6 +14,7 @@ SYNOPSIS\n \t\t [(--attach|--inline)[=] | --no-attach]\n \t\t [-s | --signoff]\n \t\t [--signature= | --no-signature]\n+\t\t [--signature-file=]\n \t\t [-n | --numbered | -N | --no-numbered]\n \t\t [--start-number ] [--numbered-files]\n \t\t [--in-reply-to=Message-Id] [--suffix=.]\n@@ -233,6 +234,9 @@ configuration options in linkgit:git-notes[1] to use this workflow).\n \tsignature option is omitted the signature defaults to the Git version\n \tnumber.\n \n+--signature-file=::\n+\tWorks just like --signature except the signature is read from a file.\n+\n --suffix=.::\n \tInstead of using `.patch` as the suffix for generated\n \tfilenames, use specified suffix. A common alternative is\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 39e8836..7ebe302 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -672,7 +672,9 @@ static void add_header(const char *value)\n #define THREAD_DEEP 2\n static int thread;\n static int do_signoff;\n-static const char *signature = git_version_string;\n+#define DEFAULT_SIGNATURE git_version_string\n+static const char *signature = DEFAULT_SIGNATURE;\n+static const char *signature_file;\n static int config_cover_letter;\n \n enum {\n@@ -742,6 +744,8 @@ static int git_format_config(const char *var, const char *value, void *cb)\n \t}\n \tif (!strcmp(var, \"format.signature\"))\n \t\treturn git_config_string(&signature, var, value);\n+\tif (!strcmp(var, \"format.signaturefile\"))\n+\t\treturn git_config_pathname(&signature_file, var, value);\n \tif (!strcmp(var, \"format.coverletter\")) {\n \t\tif (value && !strcasecmp(value, \"auto\")) {\n \t\t\tconfig_cover_letter = COVER_AUTO;\n@@ -1230,6 +1234,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\t\t PARSE_OPT_OPTARG, thread_callback },\n \t\tOPT_STRING(0, \"signature\", &signature, N_(\"signature\"),\n \t\t\t N_(\"add a signature\")),\n+\t\tOPT_FILENAME(0, \"signature-file\", &signature_file,\n+\t\t\t\tN_(\"add a signature from a file\")),\n \t\tOPT__QUIET(&quiet, N_(\"don't print the patch filenames\")),\n \t\tOPT_END()\n \t};\n@@ -1447,6 +1453,17 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\t\tcover_letter = (config_cover_letter == COVER_ON);\n \t}\n \n+\tif (signature_file) {\n+\t\tif (signature && signature != DEFAULT_SIGNATURE)\n+\t\t\tdie(_(\"cannot specify both signature and signature-file\"));\n+\n+\t\tstruct strbuf buf = STRBUF_INIT;\n+\n+\t\tif (strbuf_read_file(&buf, signature_file, 128) < 0)\n+\t\t\tdie_errno(_(\"unable to read signature file '%s'\"), signature_file);\n+\t\tsignature = strbuf_detach(&buf, NULL);\n+\t}\n+\n \tif (in_reply_to || thread || cover_letter)\n \t\trev.ref_message_ids = xcalloc(1, sizeof(struct string_list));\n \tif (in_reply_to) {\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex 9c80633..74f0aec 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -762,6 +762,38 @@ test_expect_success 'format-patch --signature=\"\" suppresses signatures' '\n \t! grep \"^-- \\$\" output\n '\n \n+cat >expect <<-\\EOF\n+\n+Test User \n+http://git.kernel.org/cgit/git/git.git\n+\n+git.kernel.org/?p=git/git.git;a=summary\n+\n+EOF\n+\n+test_expect_success 'format-patch --signature-file=file' '\n+\tgit format-patch --stdout --signature-file=expect -1 >output &&\n+\tcheck_patch output &&\n+\tsed -n -e \"/^-- $/,\\$p\" output2 &&\n+\ttest_cmp expect output2\n+'\n+\n+test_expect_success 'format-patch --signature and --signature-file die' '\n+\ttest_must_fail git format-patch --stdout --signature=\"foo\" --signature-file=expect -1 >output\n+'\n+\n+test_expect_success 'format-patch --no-signature and --signature-file OK' '\n+\tgit format-patch --stdout --no-signature --signature-file=expect -1\n+'\n+\n+test_expect_success 'format-patch with format.signaturefile config' '\n+\ttest_config format.signaturefile expect &&\n+\tgit format-patch --stdout -1 >output &&\n+\tcheck_patch output &&\n+\tsed -n -e \"/^-- $/,\\$p\" output2 &&\n+\ttest_cmp expect output2\n+'\n+\n test_expect_success TTY 'format-patch --stdout paginates' '\n \trm -f pager_used &&\n \ttest_terminal env GIT_PAGER=\"wc >pager_used\" git format-patch --stdout --all &&\n-- \nJeremiah Mahler\njmmahler@gmail.com\nhttp://github.com/jmahler\n"},{"id":"242236","messageId":"20140520082740.GB27590@sigill.intra.peff.net","threadId":"36706","inReplyTo":"1400572806-21092-2-git-send-email-jmmahler@gmail.com","subject":"Re: [PATCH v5] format-patch --signature-file ","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-20T08:27:40Z","receivedAt":"2014-05-20T08:27:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 20, 2014 at 01:00:06AM -0700, Jeremiah Mahler wrote:\n\n> Added option that allows a signature file to be used with format-patch\n> so that signatures with newlines and other special characters can be\n> easily included.\n> \n> $ git format-patch --signature-file ~/.signature -1\n> \n> The config variable format.signaturefile is also provided so that it\n> can be added by default.\n> \n> $ git config format.signaturefile ~/.signature\n> \n> $ git format-patch -1\n> \n> Signed-off-by: Jeremiah Mahler \n\nThanks for keeping at it. This looks good, but I have one question:\n\n> +test_expect_success 'format-patch --signature-file=file' '\n> +\tgit format-patch --stdout --signature-file=expect -1 >output &&\n> +\tcheck_patch output &&\n> +\tsed -n -e \"/^-- $/,\\$p\" output2 &&\n> +\ttest_cmp expect output2\n> +'\n\nHere we drop two final lines from the email output. But if I just use\nvanilla git and try this, I get nothing:\n\n $ git format-patch -1 --stdout |\n sed -n -e \"/^-- $/,\\$p\" | sed -e \"1 d\" | sed -e \"\\$d\" | sed -e \"\\$d\"\n\nRemoving the second dropped line works:\n\n $ git format-patch -1 --stdout |\n sed -n -e \"/^-- $/,\\$p\" | sed -e \"1 d\" | sed -e \"\\$d\"\n 2.0.0.rc1.436.g03cb729\n\nI guess the signature code is adding its own newline, so that the\ndefault signature (or \"--signature=foo\") works. But it adds it\nunconditionally, which is probably not what we want for signatures read\nfrom a file.\n\nDo we maybe want something like this right before your patch?\n\n-- >8 --\nSubject: format-patch: make newline after signature conditional\n\nWhen we print an email signature, we print the divider \"--\n\\n\", then the signature string, then two newlines.\nTraditionally the signature is a one-liner (and the default\nis just the git version), so the extra newline makes sense.\n\nBut one could easily specify a longer, multi-line signature,\nlike:\n\n git format-patch --signature='\n this is my long signature\n\n it has multiple lines\n ' ...\n\nWe should notice that it already has its own trailing\nnewline, and suppress one of ours.\n\nSigned-off-by: Jeff King \n---\nIn the example above, there's also a newline before the signature\nstarts. Should we suppress the first one, too?\n\nAlso, I'm not clear on the purpose of the extra trailing line in the\nfirst place. Emails now end with (\">\" added to show blanks):\n\n > -- \n > 2.0.0-rc3...\n >\n\nIs there are a reason for that final blank line?\n\n builtin/log.c | 9 +++++++--\n 1 file changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 39e8836..5acc048 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -844,8 +844,13 @@ static void gen_message_id(struct rev_info *info, char *base)\n \n static void print_signature(void)\n {\n-\tif (signature && *signature)\n-\t\tprintf(\"-- \\n%s\\n\\n\", signature);\n+\tif (!signature || !*signature)\n+\t\treturn;\n+\n+\tprintf(\"-- \\n%s\", signature);\n+\tif (signature[strlen(signature)-1] != '\\n')\n+\t\tputchar('\\n');\n+\tputchar('\\n');\n }\n \n static void add_branch_description(struct strbuf *buf, const char *branch_name)\n-- \n2.0.0.rc1.436.g03cb729\n"},{"id":"242292","messageId":"xmqq61l01gmw.fsf@gitster.dls.corp.google.com","threadId":"36706","inReplyTo":"20140520082740.GB27590@sigill.intra.peff.net","subject":"Re: [PATCH v5] format-patch --signature-file ","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-20T17:53:11Z","receivedAt":"2014-05-20T17:53:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> But one could easily specify a longer, multi-line signature,\n> like:\n>\n> git format-patch --signature='\n> this is my long signature\n>\n> it has multiple lines\n> ' ...\n>\n> We should notice that it already has its own trailing\n> newline, and suppress one of ours.\n>\n> Signed-off-by: Jeff King \n> ---\n> In the example above, there's also a newline before the signature\n> starts. Should we suppress the first one, too?\n>\n> Also, I'm not clear on the purpose of the extra trailing line in the\n> first place. Emails now end with (\">\" added to show blanks):\n>\n> > -- \n> > 2.0.0-rc3...\n> >\n>\n> Is there are a reason for that final blank line?\n\nI actually think these \"supress extra LFs\" trying to be overly smart\nand inviting unnecessary surprises. Unlike log messages people type\n(in which we do squash runs of blank lines and other prettifying),\nmail-signature string is not something people keep typing, and it\nwould be better to keep it simple and consistent, i.e. we can\ndeclare that the users who use non-default mail-signature can and\nshould learn to:\n\n --signature='this is the first line of my long sig\n\n with a blank line and then it ends here'\n\nand be done with it, I think.\n\nThe trailing blank after the mail-signature is a different issue. I\nthink it is safe to remove it and I also think the result may look\nbetter, but at the same time, it is very close to the \"if we were\nwriting format-patch today, then we would...\" category, I would say.\n\n> builtin/log.c | 9 +++++++--\n> 1 file changed, 7 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/log.c b/builtin/log.c\n> index 39e8836..5acc048 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -844,8 +844,13 @@ static void gen_message_id(struct rev_info *info, char *base)\n> \n> static void print_signature(void)\n> {\n> -\tif (signature && *signature)\n> -\t\tprintf(\"-- \\n%s\\n\\n\", signature);\n> +\tif (!signature || !*signature)\n> +\t\treturn;\n> +\n> +\tprintf(\"-- \\n%s\", signature);\n> +\tif (signature[strlen(signature)-1] != '\\n')\n> +\t\tputchar('\\n');\n> +\tputchar('\\n');\n> }\n> \n> static void add_branch_description(struct strbuf *buf, const char *branch_name)\n"},{"id":"242300","messageId":"20140520182443.GA30533@sigill.intra.peff.net","threadId":"36706","inReplyTo":"xmqq61l01gmw.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v5] format-patch --signature-file ","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-20T18:24:43Z","receivedAt":"2014-05-20T18:24:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 20, 2014 at 10:53:11AM -0700, Junio C Hamano wrote:\n\n> I actually think these \"supress extra LFs\" trying to be overly smart\n> and inviting unnecessary surprises. Unlike log messages people type\n> (in which we do squash runs of blank lines and other prettifying),\n> mail-signature string is not something people keep typing, and it\n> would be better to keep it simple and consistent, i.e. we can\n> declare that the users who use non-default mail-signature can and\n> should learn to:\n> \n> --signature='this is the first line of my long sig\n> \n> with a blank line and then it ends here'\n> \n> and be done with it, I think.\n\nIf it were just \"--signature\", I'd agree. After all, nobody is even\ncomplaining. But this is also in preparation for --signature-file.\nShould the user create a file without a trailing newline?\n\nWe can special-case --signature-file to strip the final newline from the\nread file, but it seems friendlier to handle it at the printing stage\n(and then we handle the unlikely but possible --signature as above for\nfree). I don't think it would adversely impact any real-world case,\nbecause somebody would have to:\n\n 1. already be including an extra trailing newline\n\n 2. really _want_ three newlines at the end\n\n> The trailing blank after the mail-signature is a different issue. I\n> think it is safe to remove it and I also think the result may look\n> better, but at the same time, it is very close to the \"if we were\n> writing format-patch today, then we would...\" category, I would say.\n\nYeah. I think it is probably extraneous and would not hurt to remove.\nBut it may not be worth worrying about (it's really the _two_ lines\ncaused by the unconditional newline above that bugs me).\n\n-Peff\n"},{"id":"242307","messageId":"xmqq7g5gz3s4.fsf@gitster.dls.corp.google.com","threadId":"36706","inReplyTo":"20140520182443.GA30533@sigill.intra.peff.net","subject":"Re: [PATCH v5] format-patch --signature-file ","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-20T18:46:51Z","receivedAt":"2014-05-20T18:46:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> If it were just \"--signature\", I'd agree. After all, nobody is even\n> complaining. But this is also in preparation for --signature-file.\n> Should the user create a file without a trailing newline?\n\nAhh, I missed that part.\n\nI am fine with processing it with stripspace().\n\nBy the way, at some point we may want to move that helper function\nto strbuf.c, but that is a separate issue.\n"},{"id":"242346","messageId":"20140521004244.GA4927@hudson.localdomain","threadId":"36706","inReplyTo":"20140520082740.GB27590@sigill.intra.peff.net","subject":"Re: [PATCH v5] format-patch --signature-file ","fromName":"Jeremiah Mahler","fromEmail":"jmmahler@gmail.com","sentAt":"2014-05-21T00:42:44Z","receivedAt":"2014-05-21T00:42:44Z","isPatch":true,"sender":{"key":"jmmahler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/154028?v=4"},"body":"On Tue, May 20, 2014 at 04:27:40AM -0400, Jeff King wrote:\n> On Tue, May 20, 2014 at 01:00:06AM -0700, Jeremiah Mahler wrote:\n> \n...\n> > +test_expect_success 'format-patch --signature-file=file' '\n> > +\tgit format-patch --stdout --signature-file=expect -1 >output &&\n> > +\tcheck_patch output &&\n> > +\tsed -n -e \"/^-- $/,\\$p\" output2 &&\n> > +\ttest_cmp expect output2\n> > +'\n> \n> Here we drop two final lines from the email output. But if I just use\n> vanilla git and try this, I get nothing:\n> \n> $ git format-patch -1 --stdout |\n> sed -n -e \"/^-- $/,\\$p\" | sed -e \"1 d\" | sed -e \"\\$d\" | sed -e \"\\$d\"\n> \n> Removing the second dropped line works:\n> \n> $ git format-patch -1 --stdout |\n> sed -n -e \"/^-- $/,\\$p\" | sed -e \"1 d\" | sed -e \"\\$d\"\n> 2.0.0.rc1.436.g03cb729\n> \n> I guess the signature code is adding its own newline, so that the\n> default signature (or \"--signature=foo\") works. But it adds it\n> unconditionally, which is probably not what we want for signatures read\n> from a file.\n> \n> Do we maybe want something like this right before your patch?\n> \n> -- >8 --\n> Subject: format-patch: make newline after signature conditional\n> \n> When we print an email signature, we print the divider \"--\n> \\n\", then the signature string, then two newlines.\n> Traditionally the signature is a one-liner (and the default\n> is just the git version), so the extra newline makes sense.\n> \n> But one could easily specify a longer, multi-line signature,\n> like:\n> \n> git format-patch --signature='\n> this is my long signature\n> \n> it has multiple lines\n> ' ...\n> \n> We should notice that it already has its own trailing\n> newline, and suppress one of ours.\n> \n> Signed-off-by: Jeff King \n> ---\n> In the example above, there's also a newline before the signature\n> starts. Should we suppress the first one, too?\n> \n> Also, I'm not clear on the purpose of the extra trailing line in the\n> first place. Emails now end with (\">\" added to show blanks):\n> \n> > -- \n> > 2.0.0-rc3...\n> >\n> \n> Is there are a reason for that final blank line?\n> \n> builtin/log.c | 9 +++++++--\n> 1 file changed, 7 insertions(+), 2 deletions(-)\n> \n> diff --git a/builtin/log.c b/builtin/log.c\n> index 39e8836..5acc048 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -844,8 +844,13 @@ static void gen_message_id(struct rev_info *info, char *base)\n> \n> static void print_signature(void)\n> {\n> -\tif (signature && *signature)\n> -\t\tprintf(\"-- \\n%s\\n\\n\", signature);\n> +\tif (!signature || !*signature)\n> +\t\treturn;\n> +\n> +\tprintf(\"-- \\n%s\", signature);\n> +\tif (signature[strlen(signature)-1] != '\\n')\n> +\t\tputchar('\\n');\n> +\tputchar('\\n');\n> }\n> \n> static void add_branch_description(struct strbuf *buf, const char *branch_name)\n> -- \n> 2.0.0.rc1.436.g03cb729\n> \n\nThe simple solution, leaving things as they are, appeals to me.\nFor the purposes of testing just expect whatever odd number of blank\nlines are produced. Or perhaps slurp all the trailing blank lines and\nthen compare.\n\nIt would be nice if every signature was followed by exactly one blank line.\nBut if it had two it wouldn't bother me much.\n\n-- \nJeremiah Mahler\njmmahler@gmail.com\nhttp://github.com/jmahler\n"},{"id":"242385","messageId":"20140521164255.GA2040@sigill.intra.peff.net","threadId":"36706","inReplyTo":"xmqq7g5gz3s4.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v5] format-patch --signature-file ","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-21T16:42:55Z","receivedAt":"2014-05-21T16:42:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 20, 2014 at 11:46:51AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > If it were just \"--signature\", I'd agree. After all, nobody is even\n> > complaining. But this is also in preparation for --signature-file.\n> > Should the user create a file without a trailing newline?\n> \n> Ahh, I missed that part.\n> \n> I am fine with processing it with stripspace().\n\nI wasn't planning on anything as drastic as stripspace. I really just\nwanted to suppress the one newline, which is almost certainly the right\nthing to include for \"--signature\", but the wrong thing for\n\"--signature-file\" (i.e., the patch I posted earlier).\n\nStripspace() would drop all extra whitespace, and I wondered if people\nwould _want_ it in their sigs (e.g., a blank line after the \"-- \" but\nbefore their .sig content).\n\nI dunno. Maybe it is not worth caring too much about. I don't want to\nhold up Jeremiah's patch for something that I suspect neither of us\ncares _that_ much about (I know I am not planning on using\n--signature-file myself). I just don't want to deal with a patch later\nthat says \"oh, this spacing is wrong\" and have to respond \"yes, but we\nhave to retain it so as not to break existing users\".\n\n> By the way, at some point we may want to move that helper function\n> to strbuf.c, but that is a separate issue.\n\nAgreed. I was touching some string functions earlier today and noticed\nthat strbuf.c actually contains a lot of non-strbuf functions for\ndealing with C strings. That's fine, I guess, but I also wondered if we\nshould have a separate file for C-string functions. I suppose it doesn't\nmatter that much either way, as long as it's in a libgit.a file (and\nstripspace currently is _not_, which I assume is what you were\nindicating above).\n\n-Peff\n"},{"id":"242389","messageId":"20140521165543.GA3743@hudson.localdomain","threadId":"36706","inReplyTo":"20140521164255.GA2040@sigill.intra.peff.net","subject":"Re: [PATCH v5] format-patch --signature-file ","fromName":"Jeremiah Mahler","fromEmail":"jmmahler@gmail.com","sentAt":"2014-05-21T16:55:43Z","receivedAt":"2014-05-21T16:55:43Z","isPatch":true,"sender":{"key":"jmmahler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/154028?v=4"},"body":"On Wed, May 21, 2014 at 12:42:55PM -0400, Jeff King wrote:\n> On Tue, May 20, 2014 at 11:46:51AM -0700, Junio C Hamano wrote:\n> \n> > Jeff King writes:\n> > \n> > > If it were just \"--signature\", I'd agree. After all, nobody is even\n> > > complaining. But this is also in preparation for --signature-file.\n> > > Should the user create a file without a trailing newline?\n> > \n> > Ahh, I missed that part.\n> > \n> > I am fine with processing it with stripspace().\n> \n> I wasn't planning on anything as drastic as stripspace. I really just\n> wanted to suppress the one newline, which is almost certainly the right\n> thing to include for \"--signature\", but the wrong thing for\n> \"--signature-file\" (i.e., the patch I posted earlier).\n> \n> Stripspace() would drop all extra whitespace, and I wondered if people\n> would _want_ it in their sigs (e.g., a blank line after the \"-- \" but\n> before their .sig content).\n> \n> I dunno. Maybe it is not worth caring too much about. I don't want to\n> hold up Jeremiah's patch for something that I suspect neither of us\n> cares _that_ much about (I know I am not planning on using\n> --signature-file myself). I just don't want to deal with a patch later\n> that says \"oh, this spacing is wrong\" and have to respond \"yes, but we\n> have to retain it so as not to break existing users\".\n> \n> > By the way, at some point we may want to move that helper function\n> > to strbuf.c, but that is a separate issue.\n> \n> Agreed. I was touching some string functions earlier today and noticed\n> that strbuf.c actually contains a lot of non-strbuf functions for\n> dealing with C strings. That's fine, I guess, but I also wondered if we\n> should have a separate file for C-string functions. I suppose it doesn't\n> matter that much either way, as long as it's in a libgit.a file (and\n> stripspace currently is _not_, which I assume is what you were\n> indicating above).\n> \n> -Peff\n\nI am fine with including your previous patch.\n\nWould like me to test it out and create another patch set?\n\n-- \nJeremiah Mahler\njmmahler@gmail.com\nhttp://github.com/jmahler\n"},{"id":"242391","messageId":"20140521170010.GE2040@sigill.intra.peff.net","threadId":"36706","inReplyTo":"20140521165543.GA3743@hudson.localdomain","subject":"Re: [PATCH v5] format-patch --signature-file ","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-21T17:00:10Z","receivedAt":"2014-05-21T17:00:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 21, 2014 at 09:55:43AM -0700, Jeremiah Mahler wrote:\n\n> I am fine with including your previous patch.\n\nI think that would be my preference, but we'll see what Junio says.\n\n> Would like me to test it out and create another patch set?\n\nYeah, that would be the logical next step. I think the only thing you\nshould need to adjust in your patch is to strip only one line from the\noutput, rather than two.\n\n-Peff\n"},{"id":"242395","messageId":"xmqq61kzqbi6.fsf@gitster.dls.corp.google.com","threadId":"36706","inReplyTo":"20140521164255.GA2040@sigill.intra.peff.net","subject":"Re: [PATCH v5] format-patch --signature-file ","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-21T17:37:05Z","receivedAt":"2014-05-21T17:37:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, May 20, 2014 at 11:46:51AM -0700, Junio C Hamano wrote:\n>\n>> Jeff King writes:\n>> \n>> > If it were just \"--signature\", I'd agree. After all, nobody is even\n>> > complaining. But this is also in preparation for --signature-file.\n>> > Should the user create a file without a trailing newline?\n>> \n>> Ahh, I missed that part.\n>> \n>> I am fine with processing it with stripspace().\n>\n> I wasn't planning on anything as drastic as stripspace. I really just\n> wanted to suppress the one newline, which is almost certainly the right\n> thing to include for \"--signature\", but the wrong thing for\n> \"--signature-file\" (i.e., the patch I posted earlier).\n> ...\n> I dunno. Maybe it is not worth caring too much about.\n\nI suggested stripspace() because I know we do not care too\nmuch, actually.\n\nCleansing blank lines in one way for many types of user input\n(e.g. commit log messages and tag messages) while doing it in a\ncompletely different way just for \"--signature-file\" is warranted if\nthere is a good reason for them to be different, but I did not think\nof any, and I still don't. So...\n\n>> By the way, at some point we may want to move that helper function\n>> to strbuf.c, but that is a separate issue.\n>\n> Agreed. I was touching some string functions earlier today and noticed\n> that strbuf.c actually contains a lot of non-strbuf functions for\n> dealing with C strings. That's fine, I guess, but I also wondered if we\n> should have a separate file for C-string functions. I suppose it doesn't\n> matter that much either way, as long as it's in a libgit.a file (and\n> stripspace currently is _not_, which I assume is what you were\n> indicating above).\n\nYes, I thought you would have used it in your follow-up patch if it\nwere more prominent.\n"},{"id":"242398","messageId":"20140521175930.GA4588@sigill.intra.peff.net","threadId":"36706","inReplyTo":"xmqq61kzqbi6.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v5] format-patch --signature-file ","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-21T17:59:30Z","receivedAt":"2014-05-21T17:59:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 21, 2014 at 10:37:05AM -0700, Junio C Hamano wrote:\n\n> > I wasn't planning on anything as drastic as stripspace. I really just\n> > wanted to suppress the one newline, which is almost certainly the right\n> > thing to include for \"--signature\", but the wrong thing for\n> > \"--signature-file\" (i.e., the patch I posted earlier).\n> > ...\n> > I dunno. Maybe it is not worth caring too much about.\n> \n> I suggested stripspace() because I know we do not care too\n> much, actually.\n> \n> Cleansing blank lines in one way for many types of user input\n> (e.g. commit log messages and tag messages) while doing it in a\n> completely different way just for \"--signature-file\" is warranted if\n> there is a good reason for them to be different, but I did not think\n> of any, and I still don't. So...\n\nI didn't think of mine as cleansing. It is more like \"do not duplicate a\nnewline ourselves if there is already one there\". But I guess those are\ntwo sides of the same coin.\n\n-Peff\n"},{"id":"242400","messageId":"xmqqsio3ounp.fsf@gitster.dls.corp.google.com","threadId":"36706","inReplyTo":"20140521175930.GA4588@sigill.intra.peff.net","subject":"Re: [PATCH v5] format-patch --signature-file ","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-21T18:26:18Z","receivedAt":"2014-05-21T18:26:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, May 21, 2014 at 10:37:05AM -0700, Junio C Hamano wrote:\n>\n>> > I wasn't planning on anything as drastic as stripspace. I really just\n>> > wanted to suppress the one newline, which is almost certainly the right\n>> > thing to include for \"--signature\", but the wrong thing for\n>> > \"--signature-file\" (i.e., the patch I posted earlier).\n>> > ...\n>> > I dunno. Maybe it is not worth caring too much about.\n>> \n>> I suggested stripspace() because I know we do not care too\n>> much, actually.\n>> \n>> Cleansing blank lines in one way for many types of user input\n>> (e.g. commit log messages and tag messages) while doing it in a\n>> completely different way just for \"--signature-file\" is warranted if\n>> there is a good reason for them to be different, but I did not think\n>> of any, and I still don't. So...\n>\n> I didn't think of mine as cleansing. It is more like \"do not duplicate a\n> newline ourselves if there is already one there\". But I guess those are\n> two sides of the same coin.\n\nYeah, I agree with the last sentence. My mention of \"cleansing\"\ntook into account your \"do we want to omit the leading blank as\nwell?\" as well. In any case, wouldn't reusing stripspace() make the\nfix-up shorter?\n"},{"id":"242408","messageId":"20140521204739.GA8381@sigill.intra.peff.net","threadId":"36706","inReplyTo":"xmqqsio3ounp.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v5] format-patch --signature-file ","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-21T20:47:39Z","receivedAt":"2014-05-21T20:47:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 21, 2014 at 11:26:18AM -0700, Junio C Hamano wrote:\n\n> Yeah, I agree with the last sentence. My mention of \"cleansing\"\n> took into account your \"do we want to omit the leading blank as\n> well?\" as well. In any case, wouldn't reusing stripspace() make the\n> fix-up shorter?\n\nNot really. Doing stripspace() on the file we read would be a one-liner,\nbut it doesn't address the extra line. We _already_ have\nstripspace-compatible output in the file, with a trailing newline. It's\nthe one we add for the cmdline arg or default (which lack a newline\ngenerally) that causes the doubling.\n\nSo I think it would be something like:\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 39e8836..0312402 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -844,8 +844,13 @@ static void gen_message_id(struct rev_info *info, char *base)\n \n static void print_signature(void)\n {\n-\tif (signature && *signature)\n-\t\tprintf(\"-- \\n%s\\n\\n\", signature);\n+\tif (signature && *signature) {\n+\t\tstruct strbuf buf = STRBUF_INIT;\n+\t\tstrbuf_addstr(&buf, signature);\n+\t\tstripspace(&buf, 0);\n+\t\tprintf(\"-- \\n%s\\n\", buf.buf);\n+\t\tstrbuf_release(&buf);\n+\t}\n }\n \n static void add_branch_description(struct strbuf *buf, const char *branch_name)\n\nI.e., make sure we have consistent stripspace output, and then stop\nadding the extra newline in the printf (notice we still include the\n\"extra\" blank at the end, though we can obviously easily drop it here).\nCompare to the previous patch I sent, or to the shortest possible:\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 39e8836..16b8771 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -845,7 +845,8 @@ static void gen_message_id(struct rev_info *info, char *base)\n static void print_signature(void)\n {\n \tif (signature && *signature)\n-\t\tprintf(\"-- \\n%s\\n\\n\", signature);\n+\t\tprintf(\"-- \\n%s%s\\n\", signature,\n+\t\t ends_with(signature, \"\\n\") ? \"\" : \"\\n\");\n }\n \n static void add_branch_description(struct strbuf *buf, const char *branch_name)\n\nBut I'm not sure \"length of code\" is the right metric. They're all\npretty easy to do, if we can decide on which.\n\nI think the ratio of usefulness to number of words in this sub-thread is\ngetting off. I'd be OK with any of:\n\n 1. Leave it as-is. Files get two blank lines.\n\n 2. Conditional newline\n\n 3. Stripspace.\n\n-Peff\n"},{"id":"242414","messageId":"20140521211414.GA13540@hudson.localdomain","threadId":"36706","inReplyTo":"20140521204739.GA8381@sigill.intra.peff.net","subject":"Re: [PATCH v5] format-patch --signature-file ","fromName":"Jeremiah Mahler","fromEmail":"jmmahler@gmail.com","sentAt":"2014-05-21T21:14:14Z","receivedAt":"2014-05-21T21:14:14Z","isPatch":true,"sender":{"key":"jmmahler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/154028?v=4"},"body":"On Wed, May 21, 2014 at 04:47:39PM -0400, Jeff King wrote:\n> On Wed, May 21, 2014 at 11:26:18AM -0700, Junio C Hamano wrote:\n> \n...\n> I think the ratio of usefulness to number of words in this sub-thread is\n> getting off. I'd be OK with any of:\n...\n> \n> -Peff\n\nI think this has become a discussion about the airspeed of an unladen swallow.\n:-)\n\nI think we're close to a final patch. v7 sent.\n\n-- \nJeremiah Mahler\njmmahler@gmail.com\nhttp://github.com/jmahler\n"}]}