{"thread":{"id":"36685","subject":"[PATCH v3] format-patch --signature-file <file>","startedAt":"2014-05-17T16:02:21Z","lastAt":"2014-05-18T17:49:54Z","messageCount":4,"participants":["Jeremiah Mahler","Jeff King"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"242067","messageId":"1400342542-11256-1-git-send-email-jmmahler@gmail.com","threadId":"36685","inReplyTo":null,"subject":"[PATCH v3] format-patch --signature-file <file>","fromName":"Jeremiah Mahler","fromEmail":"jmmahler@gmail.com","sentAt":"2014-05-17T16:02:21Z","receivedAt":"2014-05-17T16:02:21Z","isPatch":true,"sender":{"key":"jmmahler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/154028?v=4"},"body":"v3 patch to add format-patch --signature-file <file> option.\n\nThis revision includes changes suggested by Jeff King:\n\n  - Instead of a custom OPTION_CALLBACK, use OPT_FILENAME which\n\tcorrectly resolves file names which are not evaluated by the\n\tshell such as \"--signature-file=<file>\"\n\n  - Use strbuf_read_file() which eliminates the arbitrary size limit\n\tand simplifies the code.\n\n  - Generate an error if the file cannot be read instead of quietly\n\tcontinuing.  This is accomplished with strbuf_read_file().\n\n  - Die if both --signature and --signature-file are used.\n\nJeremiah Mahler (1):\n  format-patch --signature-file <file>\n\n Documentation/git-format-patch.txt |  4 ++++\n builtin/log.c                      | 13 +++++++++++++\n t/t4014-format-patch.sh            | 28 ++++++++++++++++++++++++++++\n 3 files changed, 45 insertions(+)\n"},{"id":"242068","messageId":"1400342542-11256-2-git-send-email-jmmahler@gmail.com","threadId":"36685","inReplyTo":"1400342542-11256-1-git-send-email-jmmahler@gmail.com","subject":"[PATCH v3] format-patch --signature-file <file>","fromName":"Jeremiah Mahler","fromEmail":"jmmahler@gmail.com","sentAt":"2014-05-17T16:02:22Z","receivedAt":"2014-05-17T16:02:22Z","isPatch":true,"sender":{"key":"jmmahler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/154028?v=4"},"body":"Added feature that allows a signature file to be used with format-patch.\n\n  $ git format-patch --signature-file ~/.signature -1\n\nNow signatures with newlines and other special characters can be\neasily included.\n\nSigned-off-by: Jeremiah Mahler <jmmahler@gmail.com>\n---\n Documentation/git-format-patch.txt |  4 ++++\n builtin/log.c                      | 13 +++++++++++++\n t/t4014-format-patch.sh            | 28 ++++++++++++++++++++++++++++\n 3 files changed, 45 insertions(+)\n\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)[=<boundary>] | --no-attach]\n \t\t   [-s | --signoff]\n \t\t   [--signature=<signature> | --no-signature]\n+\t\t   [--signature-file=<file>]\n \t\t   [-n | --numbered | -N | --no-numbered]\n \t\t   [--start-number <n>] [--numbered-files]\n \t\t   [--in-reply-to=Message-Id] [--suffix=.<sfx>]\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=<file>::\n+\tWorks just like --signature except the signature is read from a file.\n+\n --suffix=.<sfx>::\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..af7d610 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -673,6 +673,7 @@ static void add_header(const char *value)\n static int thread;\n static int do_signoff;\n static const char *signature = git_version_string;\n+static const char *signature_file;\n static int config_cover_letter;\n \n enum {\n@@ -1230,6 +1231,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 +1450,16 @@ 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 != git_version_string)\n+\t\t\tdie(_(\"--signature and --signature-file are mutually exclusive\"));\n+\n+\t\tstruct strbuf buf = STRBUF_INIT;\n+\n+\t\tstrbuf_read_file(&buf, signature_file, 128);\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..fb3dc1b 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -762,6 +762,34 @@ test_expect_success 'format-patch --signature=\"\" suppresses signatures' '\n \t! grep \"^-- \\$\" output\n '\n \n+cat > expect << EOF\n+Test User <test.email@kernel.org>\n+http://git.kernel.org/cgit/git/git.git\n+git.kernel.org/?p=git/git.git;a=summary\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+\tfgrep -x -f output expect >output2 &&\n+\tdiff expect output2\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+\tfgrep -x -f output expect >output2 &&\n+\tdiff expect output2\n+'\n+\n+test_expect_success 'format-patch --signature and --signature-file die' '\n+\t! 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 >output\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"},{"id":"242082","messageId":"20140518112020.GA2153@sigill.intra.peff.net","threadId":"36685","inReplyTo":"1400342542-11256-2-git-send-email-jmmahler@gmail.com","subject":"Re: [PATCH v3] format-patch --signature-file <file>","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-18T11:20:20Z","receivedAt":"2014-05-18T11:20:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, May 17, 2014 at 09:02:22AM -0700, Jeremiah Mahler wrote:\n\n> Added feature that allows a signature file to be used with format-patch.\n> \n>   $ git format-patch --signature-file ~/.signature -1\n> \n> Now signatures with newlines and other special characters can be\n> easily included.\n> \n> Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>\n\nThis is looking much better. I have a few comments still, inline below.\n\nBy the way, did you want to add a format.signaturefile config, too? I do\nnot care myself, but I would imagine most workflows would end up\nspecifying it every time.\n\n> @@ -1447,6 +1450,16 @@ 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 != git_version_string)\n> +\t\t\tdie(_(\"--signature and --signature-file are mutually exclusive\"));\n\nTechnically \"signature\" might have come from config, not \"--signature\"\non the command-line.  But I don't know if that's even worth worrying\nabout; presumably the user can figure it out if they set the config.\n\n> +\t\tstruct strbuf buf = STRBUF_INIT;\n> +\n> +\t\tstrbuf_read_file(&buf, signature_file, 128);\n> +\t\tsignature = strbuf_detach(&buf, NULL);\n\nYour cover letter mentioned generating an error here. Did you want:\n\n  if (strbuf_read_file(&buf, signature_file, 128) < 0)\n\tdie_errno(\"unable to read signature file '%s'\", signature_file);\n\nor similar?\n\n> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\n> index 9c80633..fb3dc1b 100755\n> --- a/t/t4014-format-patch.sh\n> +++ b/t/t4014-format-patch.sh\n> @@ -762,6 +762,34 @@ test_expect_success 'format-patch --signature=\"\" suppresses signatures' '\n>  \t! grep \"^-- \\$\" output\n>  '\n>  \n> +cat > expect << EOF\n\nMinor style nits, but we usually omit the whitespace between redirection\noperations, and we always quote our here-doc endings unless they\nexplicitly want to interpolate. So:\n\n  cat >expect <<\\EOF\n\n(we also tend to use \"<<-\\EOF\" to drop leading tabs, and then include\nthem inside the test_expect_success properly indented, but as this\nexpectation is used in multiple places, it's not unreasonable to keep it\nseparate).\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> +\tfgrep -x -f output expect >output2 &&\n\nBoth of these fgrep options are in POSIX, but it looks like this will be\nthe first use for either of them. I'm not sure if they will give us any\nportability problems.\n\nWe could probably do something like:\n\n  sed -n '/^-- $/,$p'\n\nif we have to.\n\n> +\tdiff expect output2\n\nPlease use test_cmp here, which adjusts automatically for less-abled\nsystems where diff is not available.\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> +\tfgrep -x -f output expect >output2 &&\n> +\tdiff expect output2\n> +'\n\nSame comments as above; I note that this is just checking \"--foo=bar\"\nrather than \"--foo bar\". I don't mind being thorough, but for the most\npart we just assume this is tested as part of the parse-options tests,\nand don't check it explicitly for each option.\n\n> +test_expect_success 'format-patch --signature and --signature-file die' '\n> +\t! git format-patch --stdout --signature=\"foo\" --signature-file=expect -1 >output\n> +'\n\nPlease use test_must_fail instead of \"!\" here; it is more thorough in\nchecking that we exited with a non-zero error code (and didn't die from\nsignal death, for example).\n\n-Peff\n"},{"id":"242084","messageId":"20140518174954.GA30558@hudson.localdomain","threadId":"36685","inReplyTo":"20140518112020.GA2153@sigill.intra.peff.net","subject":"Re: [PATCH v3] format-patch --signature-file <file>","fromName":"Jeremiah Mahler","fromEmail":"jmmahler@gmail.com","sentAt":"2014-05-18T17:49:54Z","receivedAt":"2014-05-18T17:49:54Z","isPatch":true,"sender":{"key":"jmmahler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/154028?v=4"},"body":"Peff,\n\nLots of good suggestions.  I had to expand upon the signature\npattern match to get to work.\n\nOn Sun, May 18, 2014 at 07:20:20AM -0400, Jeff King wrote:\n> On Sat, May 17, 2014 at 09:02:22AM -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> > +\tfgrep -x -f output expect >output2 &&\n> \n> Both of these fgrep options are in POSIX, but it looks like this will be\n> the first use for either of them. I'm not sure if they will give us any\n> portability problems.\n> \n> We could probably do something like:\n> \n>   sed -n '/^-- $/,$p'\n> \n\nThis gets the signature out but it will have '--' and\nsome trailing blank lines which were not in the original signature.\nSo then test_cmp won't work directly.\n\nWhat I came up with was to use head and tail to remove the first line\nand the last two lines.  Then test_cmp can be used normally.\n\ntest_expect_success 'format-patch --signature-file=file' '\n\tgit format-patch --stdout --signature-file=expect -1 >output &&\n\tcheck_patch_output output &&\n\tsed -n \"/^-- $/,\\$p\" <output | head --lines=-2 | tail --lines=+2 >output2 &&\n\ttest_cmp expect output2\n'\n\n-- \nJeremiah Mahler\njmmahler@gmail.com\nhttp://github.com/jmahler\n"}]}