{"thread":{"id":"36718","subject":"[PATCH v6] format-patch --signature-file ","startedAt":"2014-05-21T01:02:49Z","lastAt":"2014-05-21T23:18:19Z","messageCount":14,"participants":["Jeremiah Mahler","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":6,"patchTotal":null},"messages":[{"id":"242352","messageId":"1400634170-18266-1-git-send-email-jmmahler@gmail.com","threadId":"36718","inReplyTo":null,"subject":"[PATCH v6] format-patch --signature-file ","fromName":"Jeremiah Mahler","fromEmail":"jmmahler@gmail.com","sentAt":"2014-05-21T01:02:49Z","receivedAt":"2014-05-21T01:02:49Z","isPatch":true,"sender":{"key":"jmmahler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/154028?v=4"},"body":"v6 of patch to add format-patch --signature-file option.\n\nThis revision includes more suggestions from Jeff King and Junio C Hamano:\n\n - Adding #define DEFAULT_SIGNATURE was a good idea but it could be\n used in a way that nullifies the pointer comparison used to see if\n the default has changed. So this was removed.\n\n - The test cases depend on there being two blank lines after a\n signature from a file. This is an odd problem because if --signature\n or the default is used only one blank line is produced.\n This problem can be addressed in another patch.\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 | 16 ++++++++++++++++\n t/t4014-format-patch.sh | 32 ++++++++++++++++++++++++++++++++\n 4 files changed, 56 insertions(+)\n\n-- \nJeremiah Mahler\njmmahler@gmail.com\nhttp://github.com/jmahler\n"},{"id":"242353","messageId":"1400634170-18266-2-git-send-email-jmmahler@gmail.com","threadId":"36718","inReplyTo":"1400634170-18266-1-git-send-email-jmmahler@gmail.com","subject":"[PATCH v6] format-patch --signature-file ","fromName":"Jeremiah Mahler","fromEmail":"jmmahler@gmail.com","sentAt":"2014-05-21T01:02:50Z","receivedAt":"2014-05-21T01:02:50Z","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 | 16 ++++++++++++++++\n t/t4014-format-patch.sh | 32 ++++++++++++++++++++++++++++++++\n 4 files changed, 56 insertions(+)\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..ab4718b 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@@ -742,6 +743,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 +1233,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 +1452,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 != git_version_string)\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..049493d 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 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 '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 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":"242413","messageId":"xmqq61kyq1i5.fsf@gitster.dls.corp.google.com","threadId":"36718","inReplyTo":"1400634170-18266-2-git-send-email-jmmahler@gmail.com","subject":"Re: [PATCH v6] format-patch --signature-file ","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-21T21:13:06Z","receivedAt":"2014-05-21T21:13:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeremiah Mahler writes:\n\n> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\n> index 9c80633..049493d 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\nWe have been trying not to do the above in recent test updates. It\nwould be nice if this set-up did not have to be outside of the usual\ntest_expect_success structure.\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\nOverlong line. If we can't make this pipeline shorter, at least\nfold it to a reasonable length, e.g.\n\n\tsed -n -e ... output2 &&\n\nor something.\n\nThe SP between the address \"1\" and insn \"d\" looks somewhat funny and\nugly, especially given that you write the other one \"$d\" without\nsuch a SP. Was there a specific reason why it was needed?\n\nI would further think that renaming a few files and updating the way\nthe check is done may make the whole thing easier to understand.\n\n * rename the input for --signature-file to \"mail-signature\".\n\n * keep the name \"output\" to hold the format-patch output, i.e.\n\n git format-patch -1 --stdout --signature-file=mail-signature >output\n\n * Instead of munging the \"mail signature\" part of the output too\n excessively to match the input, formulate the expected output\n using \"mail-signature\" as an input, i.e.\n\n sed -e '1,/^-- $/d' actual &&\n {\n\tcat mail-signature && echo && echo\n } >expect &&\n test_cmp expect actual\n\nAlternatively, the third-bullet point above may want to be further\nfuture-proofed by using stripspace, e.g.\n\n sed -e '1/^-- $/d' actual &&\n git stripspace expect &&\n test_cmp expect actual\n\nThanks.\n"},{"id":"242416","messageId":"xmqqwqdeomes.fsf@gitster.dls.corp.google.com","threadId":"36718","inReplyTo":"xmqq61kyq1i5.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v6] format-patch --signature-file ","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-21T21:24:27Z","receivedAt":"2014-05-21T21:24:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jeremiah Mahler writes:\n>\n>> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\n>> index 9c80633..049493d 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> We have been trying not to do the above in recent test updates. It\n> would be nice if this set-up did not have to be outside of the usual\n> test_expect_success structure.\n\nIt seems you sent v7 before I had a chance to comment on this one.\nThe above was merely \"it would be nicer...\" and I am OK as-is. The\ncomments on the rest are a bit more serious, though.\n\nThanks.\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>\n> Overlong line. If we can't make this pipeline shorter, at least\n> fold it to a reasonable length, e.g.\n>\n> \tsed -n -e ... sed -e '1d' -e \"\\$d\" |\n> sed -e \"\\$d\" >output2 &&\n>\n> or something.\n>\n> The SP between the address \"1\" and insn \"d\" looks somewhat funny and\n> ugly, especially given that you write the other one \"$d\" without\n> such a SP. Was there a specific reason why it was needed?\n>\n> I would further think that renaming a few files and updating the way\n> the check is done may make the whole thing easier to understand.\n>\n> * rename the input for --signature-file to \"mail-signature\".\n>\n> * keep the name \"output\" to hold the format-patch output, i.e.\n>\n> git format-patch -1 --stdout --signature-file=mail-signature >output\n>\n> * Instead of munging the \"mail signature\" part of the output too\n> excessively to match the input, formulate the expected output\n> using \"mail-signature\" as an input, i.e.\n>\n> sed -e '1,/^-- $/d' actual &&\n> {\n> \tcat mail-signature && echo && echo\n> } >expect &&\n> test_cmp expect actual\n>\n> Alternatively, the third-bullet point above may want to be further\n> future-proofed by using stripspace, e.g.\n>\n> sed -e '1/^-- $/d' actual &&\n> git stripspace expect &&\n> test_cmp expect actual\n>\n> Thanks.\n"},{"id":"242418","messageId":"20140521213206.GA13956@hudson.localdomain","threadId":"36718","inReplyTo":"xmqqwqdeomes.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v6] format-patch --signature-file ","fromName":"Jeremiah Mahler","fromEmail":"jmmahler@gmail.com","sentAt":"2014-05-21T21:32:06Z","receivedAt":"2014-05-21T21:32:06Z","isPatch":true,"sender":{"key":"jmmahler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/154028?v=4"},"body":"On Wed, May 21, 2014 at 02:24:27PM -0700, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n...\n> \n> It seems you sent v7 before I had a chance to comment on this one.\n> The above was merely \"it would be nicer...\" and I am OK as-is. The\n> comments on the rest are a bit more serious, though.\n> \n> Thanks.\n> \n\nIt is not a problem. Thanks again for all your feedback.\n\n-- \nJeremiah Mahler\njmmahler@gmail.com\nhttp://github.com/jmahler\n"},{"id":"242421","messageId":"20140521215059.GB13956@hudson.localdomain","threadId":"36718","inReplyTo":"xmqq61kyq1i5.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v6] format-patch --signature-file ","fromName":"Jeremiah Mahler","fromEmail":"jmmahler@gmail.com","sentAt":"2014-05-21T21:50:59Z","receivedAt":"2014-05-21T21:50:59Z","isPatch":true,"sender":{"key":"jmmahler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/154028?v=4"},"body":"On Wed, May 21, 2014 at 02:13:06PM -0700, Junio C Hamano wrote:\n> Jeremiah Mahler writes:\n> \n...\n> > \t! grep \"^-- \\$\" output\n...\n> \n> We have been trying not to do the above in recent test updates. It\n> would be nice if this set-up did not have to be outside of the usual\n> test_expect_success structure.\n> \n\nJeff caught those \"! grep\" instances in my patch.\n\nI can submit a separate patch to fix instances like those in test cases\nunrelated to this signature-file patch.\n\n-- \nJeremiah Mahler\njmmahler@gmail.com\nhttp://github.com/jmahler\n"},{"id":"242422","messageId":"xmqqoayqoktp.fsf@gitster.dls.corp.google.com","threadId":"36718","inReplyTo":"20140521215059.GB13956@hudson.localdomain","subject":"Re: [PATCH v6] format-patch --signature-file ","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-21T21:58:42Z","receivedAt":"2014-05-21T21:58:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeremiah Mahler writes:\n\n> On Wed, May 21, 2014 at 02:13:06PM -0700, Junio C Hamano wrote:\n>> Jeremiah Mahler writes:\n>> \n> ...\n>> > \t! grep \"^-- \\$\" output\n> ...\n>> \n>> We have been trying not to do the above in recent test updates. It\n>> would be nice if this set-up did not have to be outside of the usual\n>> test_expect_success structure.\n>> \n>\n> Jeff caught those \"! grep\" instances in my patch.\n\nHmm, I didn't mean that one, and I do not offhand what is wrong\nabout \"! grep\" that says \"output should not contain this string\".\n\nThe problem is a \"cat\" you added outside test_expect_*; the recent\npush is to have as little executable outside them, especially the\n\"set-up\" code to prepare for the real tests.\n\ni.e. we have been trying to write new tests (and convert old ones)\nlike this:\n\n test_expect_success 'I test such and such ' '\n cat >input-for-test <<-\\EOF &&\n here comes input\n EOF\n git command-to-be-tested actual &&\n cat >expected <<-\\EOF &&\n here comes expected output\n EOF\n test_cmp expected actual\n '\n\nnot like this:\n\n cat >input-for-test <<-\\EOF &&\n here comes input\n EOF\n test_expect_success 'I test such and such ' '\n git command-to-be-tested actual &&\n cat >expected <<-\\EOF &&\n here comes expected output\n EOF\n test_cmp expected actual\n '\n"},{"id":"242426","messageId":"20140521220250.GA11970@sigill.intra.peff.net","threadId":"36718","inReplyTo":"xmqqoayqoktp.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v6] format-patch --signature-file ","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-21T22:02:50Z","receivedAt":"2014-05-21T22:02:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 21, 2014 at 02:58:42PM -0700, Junio C Hamano wrote:\n\n> >> > \t! grep \"^-- \\$\" output\n> > ...\n> >> \n> >> We have been trying not to do the above in recent test updates. It\n> >> would be nice if this set-up did not have to be outside of the usual\n> >> test_expect_success structure.\n> >> \n> >\n> > Jeff caught those \"! grep\" instances in my patch.\n> \n> Hmm, I didn't mean that one, and I do not offhand what is wrong\n> about \"! grep\" that says \"output should not contain this string\".\n\nI think he is responding to my earlier request to use test_must_fail\ninstead of \"!\". But there is a subtlety there he does not know, which\nis that we typically only use the former for git programs, and rely on\n\"!\" for normal Unix commands.\n\n> The problem is a \"cat\" you added outside test_expect_*; the recent\n> push is to have as little executable outside them, especially the\n> \"set-up\" code to prepare for the real tests.\n> \n> i.e. we have been trying to write new tests (and convert old ones)\n> like this:\n> \n> test_expect_success 'I test such and such ' '\n> cat >input-for-test <<-\\EOF &&\n> here comes input\n> EOF\n> git command-to-be-tested actual &&\n> cat >expected <<-\\EOF &&\n> here comes expected output\n> EOF\n> test_cmp expected actual\n> '\n> \n> not like this:\n> \n> cat >input-for-test <<-\\EOF &&\n> here comes input\n> EOF\n> test_expect_success 'I test such and such ' '\n> git command-to-be-tested actual &&\n> cat >expected <<-\\EOF &&\n> here comes expected output\n> EOF\n> test_cmp expected actual\n> '\n\nYeah, I noticed and gave a pass on this in earlier review, because the\nfile is used across many tests. So burying it in the first test that\nuses it is probably a bad thing. However, it could go in its own setup\ntest.\n\n-Peff\n"},{"id":"242430","messageId":"20140521221221.GC13956@hudson.localdomain","threadId":"36718","inReplyTo":"xmqqoayqoktp.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v6] format-patch --signature-file ","fromName":"Jeremiah Mahler","fromEmail":"jmmahler@gmail.com","sentAt":"2014-05-21T22:12:21Z","receivedAt":"2014-05-21T22:12:21Z","isPatch":true,"sender":{"key":"jmmahler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/154028?v=4"},"body":"On Wed, May 21, 2014 at 02:58:42PM -0700, Junio C Hamano wrote:\n> Jeremiah Mahler writes:\n> \n...\n> The problem is a \"cat\" you added outside test_expect_*; the recent\n> push is to have as little executable outside them, especially the\n> \"set-up\" code to prepare for the real tests.\n> \n> i.e. we have been trying to write new tests (and convert old ones)\n> like this:\n> \n> test_expect_success 'I test such and such ' '\n> cat >input-for-test <<-\\EOF &&\n> here comes input\n> EOF\n> git command-to-be-tested actual &&\n> cat >expected <<-\\EOF &&\n> here comes expected output\n> EOF\n> test_cmp expected actual\n> '\n> \n> not like this:\n> \n> cat >input-for-test <<-\\EOF &&\n> here comes input\n> EOF\n> test_expect_success 'I test such and such ' '\n> git command-to-be-tested actual &&\n> cat >expected <<-\\EOF &&\n> here comes expected output\n> EOF\n> test_cmp expected actual\n> '\n\nNow I understand.\n\nBelow is one of the updated test cases.\n\ntest_expect_success 'format-patch --signature-file=mail-signature' '\n\tcat >mail-signature <<-\\EOF\n\n\tTest User \n\thttp://git.kernel.org/cgit/git/git.git\n\n\tgit.kernel.org/?p=git/git.git;a=summary\n\n\tEOF\n\tgit format-patch --stdout --signature-file=mail-signature -1 >output &&\n\tcheck_patch output &&\n\tsed -n -e \"/^-- $/,\\$p\" output2 &&\n\ttest_cmp mail-signature output2\n'\n\n-- \nJeremiah Mahler\njmmahler@gmail.com\nhttp://github.com/jmahler\n"},{"id":"242432","messageId":"xmqqfvk2ok10.fsf@gitster.dls.corp.google.com","threadId":"36718","inReplyTo":"20140521220250.GA11970@sigill.intra.peff.net","subject":"Re: [PATCH v6] format-patch --signature-file ","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-21T22:15:55Z","receivedAt":"2014-05-21T22:15:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I think he is responding to my earlier request to use test_must_fail\n> instead of \"!\". But there is a subtlety there he does not know, which\n> is that we typically only use the former for git programs, and rely on\n> \"!\" for normal Unix commands.\n\nOk, that explains it. Thanks.\n\n> Yeah, I noticed and gave a pass on this in earlier review, because the\n> file is used across many tests. So burying it in the first test that\n> uses it is probably a bad thing. However, it could go in its own setup\n> test.\n\nYeah, placing it in its own setup may be the best. There are quite\na many set-ups outside the tests in this script from the olden days,\nso I am OK if left it as-is and have a separate clean-up patch after\nthis topic settles. I am also OK to add a new one \"the new right way\"\nso that a later clean-up patch does not have to change what is added\nin this step.\n"},{"id":"242435","messageId":"20140521222701.GA24537@hudson.localdomain","threadId":"36718","inReplyTo":"xmqqfvk2ok10.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v6] format-patch --signature-file ","fromName":"Jeremiah Mahler","fromEmail":"jmmahler@gmail.com","sentAt":"2014-05-21T22:27:01Z","receivedAt":"2014-05-21T22:27:01Z","isPatch":true,"sender":{"key":"jmmahler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/154028?v=4"},"body":"On Wed, May 21, 2014 at 03:15:55PM -0700, Junio C Hamano wrote:\n> Jeff King writes:\n> \n> Yeah, placing it in its own setup may be the best. There are quite\n> a many set-ups outside the tests in this script from the olden days,\n> so I am OK if left it as-is and have a separate clean-up patch after\n> this topic settles. I am also OK to add a new one \"the new right way\"\n> so that a later clean-up patch does not have to change what is added\n> in this step.\n\nI like the idea of limiting the scope of this data so it couldn't\ninadvertently impact later tests.\n\nBut placing the same data inside multiple test cases creates duplication.\n\nIs there a way to define data once for a limited set of tests?\n\n-- \nJeremiah Mahler\njmmahler@gmail.com\nhttp://github.com/jmahler\n"},{"id":"242436","messageId":"xmqqbnuqoj1k.fsf@gitster.dls.corp.google.com","threadId":"36718","inReplyTo":"20140521221221.GC13956@hudson.localdomain","subject":"Re: [PATCH v6] format-patch --signature-file ","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-21T22:37:11Z","receivedAt":"2014-05-21T22:37:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeremiah Mahler writes:\n\n> Below is one of the updated test cases.\n>\n> test_expect_success 'format-patch --signature-file=mail-signature' '\n> \tcat >mail-signature <<-\\EOF\n>\n> \tTest User \n> \thttp://git.kernel.org/cgit/git/git.git\n>\n> \tgit.kernel.org/?p=git/git.git;a=summary\n>\n> \tEOF\n> \tgit format-patch --stdout --signature-file=mail-signature -1 >output &&\n> \tcheck_patch output &&\n> \tsed -n -e \"/^-- $/,\\$p\" output2 &&\n> \ttest_cmp mail-signature output2\n> '\n\nHmph, there are still few things I do not understand in the above.\n\n * Why does mail-signature file have a leading blank line? Is it\n typical users would want to have one there?\n\n * Similarly, why does mail-signature file need a trailing blank\n line? Is it usual users would want to have one there?\n\n * Why three sed in the pipeline? Wouldn't\n\n sed -e \"1,/^-- \\$/d\" actual &&\n {\n do something to turn mail-signature to what we expect\n } >expect &&\n test_cmp expect actual\n\n If that \"do something\" is \"to append an extra newline\", then it\n would make it a lot clear to do\n\n {\n cat mail-signature && echo\n } >expect\n\n than a 'sed -e \"\\$d\"' tucked at the end of the pipeline that only\n tells us we are removing one line that has _something_ without\n explicitly saying what we are removing, no?\n\nThanks.\n\n\n\n \n"},{"id":"242437","messageId":"xmqq7g5eoiia.fsf@gitster.dls.corp.google.com","threadId":"36718","inReplyTo":"20140521222701.GA24537@hudson.localdomain","subject":"Re: [PATCH v6] format-patch --signature-file ","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-21T22:48:45Z","receivedAt":"2014-05-21T22:48:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeremiah Mahler writes:\n\n> On Wed, May 21, 2014 at 03:15:55PM -0700, Junio C Hamano wrote:\n>> Jeff King writes:\n>> \n>> Yeah, placing it in its own setup may be the best. There are quite\n>> a many set-ups outside the tests in this script from the olden days,\n>> so I am OK if left it as-is and have a separate clean-up patch after\n>> this topic settles. I am also OK to add a new one \"the new right way\"\n>> so that a later clean-up patch does not have to change what is added\n>> in this step.\n>\n> I like the idea of limiting the scope of this data so it couldn't\n> inadvertently impact later tests.\n>\n> But placing the same data inside multiple test cases creates duplication.\n>\n> Is there a way to define data once for a limited set of tests?\n\nThat is what Jeff ment by \"used across many tests. ... it could go\nin its own setup\".\n\nIn other words,\n\n\ttest_expect_success 'prepare mail-signature input' '\n \tcat >mail-signature <<-\\EOF\n ...\n EOF\n\t'\n\n\ttest_expect_success 'one test that uses mail-signature' '\n \tuse mail-signature &&\n test the output\n\t'\n\n\ttest_expect_success 'another test that uses mail-signature' '\n \tuse mail-signature in a different way &&\n test the output\n\t'\n"},{"id":"242439","messageId":"20140521231819.GA24756@hudson.localdomain","threadId":"36718","inReplyTo":"xmqqbnuqoj1k.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v6] format-patch --signature-file ","fromName":"Jeremiah Mahler","fromEmail":"jmmahler@gmail.com","sentAt":"2014-05-21T23:18:19Z","receivedAt":"2014-05-21T23:18:19Z","isPatch":true,"sender":{"key":"jmmahler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/154028?v=4"},"body":"On Wed, May 21, 2014 at 03:37:11PM -0700, Junio C Hamano wrote:\n> Jeremiah Mahler writes:\n> \n> > Below is one of the updated test cases.\n> >\n> > test_expect_success 'format-patch --signature-file=mail-signature' '\n> > \tcat >mail-signature <<-\\EOF\n> >\n> > \tTest User \n> > \thttp://git.kernel.org/cgit/git/git.git\n> >\n> > \tgit.kernel.org/?p=git/git.git;a=summary\n> >\n> > \tEOF\n> > \tgit format-patch --stdout --signature-file=mail-signature -1 >output &&\n> > \tcheck_patch output &&\n> > \tsed -n -e \"/^-- $/,\\$p\" output2 &&\n> > \ttest_cmp mail-signature output2\n> > '\n> \n> Hmph, there are still few things I do not understand in the above.\n> \n> * Why does mail-signature file have a leading blank line? Is it\n> typical users would want to have one there?\n> \n> * Similarly, why does mail-signature file need a trailing blank\n> line? Is it usual users would want to have one there?\n> \n\nI think whatever the user puts in their signature should be reproduced.\nIt is probably isn't common to have leading or trailing blank lines.\nBut if they are there they should be reproduced.\n\n> * Why three sed in the pipeline? Wouldn't\n> \n> sed -e \"1,/^-- \\$/d\" \n> be more direct way to start the pipeline without having to strip\n> the \"-- \\n\" line with the extra one?\n> \n\nYes, that is much cleaner.\n\n> * Given a mail-signature file that does not end with an incomplete\n> line (i.e. we did not have to add the newline to make it\n> complete), what are the expected lines in the output after the\n> \"-- \\n\" separator?\n> \n> Whatever it is, I think it is easier to read the tests if done\n> like so:\n> \n> sed -e \"1,/^-- \\$/d\" actual &&\n> {\n> do something to turn mail-signature to what we expect\n> } >expect &&\n> test_cmp expect actual\n> \n> If that \"do something\" is \"to append an extra newline\", then it\n> would make it a lot clear to do\n> \n> {\n> cat mail-signature && echo\n> } >expect\n> \n> than a 'sed -e \"\\$d\"' tucked at the end of the pipeline that only\n> tells us we are removing one line that has _something_ without\n> explicitly saying what we are removing, no?\n> \n\nThis does make more sense. Since Git prints a blank line when it prints\nthe signature, this test also adds a blank line to produce the same\nexpected result. This is much better than deleting two lines for no\nobvious reason.\n\n-- \nJeremiah Mahler\njmmahler@gmail.com\nhttp://github.com/jmahler\n"}]}