{"thread":{"id":"36673","subject":"[PATCH v2] format-patch --signature-file ","startedAt":"2014-05-16T01:31:20Z","lastAt":"2014-05-21T00:45:59Z","messageCount":13,"participants":["Jeremiah Mahler","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"241852","messageId":"1400203881-2794-1-git-send-email-jmmahler@gmail.com","threadId":"36673","inReplyTo":null,"subject":"[PATCH v2] format-patch --signature-file ","fromName":"Jeremiah Mahler","fromEmail":"jmmahler@gmail.com","sentAt":"2014-05-16T01:31:20Z","receivedAt":"2014-05-16T01:31:20Z","isPatch":true,"sender":{"key":"jmmahler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/154028?v=4"},"body":"On Tue, May 13, 2014 at 09:07:12AM -0700, Jonathan Nieder wrote:\n> Hi,\n> \n> Jeremiah Mahler wrote:\n> \n> > # from a string\n> > $ git format-patch --signature \"from a string\" origin\n> >\n> > # or from a file\n> > $ git format-patch --signature ~/.signature origin\n> \n> Interesting. But... what if I want my patch to end with\n> \n> \t-- \n> \t/home/jrnieder/.signature\n> \n> ? It seems safer to introduce a separate --signature-file option.\n> \n\nIt is probably smarter to avoid that corner case entirely.\nGood idea.\n\n> [...]\n> > builtin/log.c | 26 ++++++++++++++++++++++++--\n> > 1 file changed, 24 insertions(+), 2 deletions(-)\n> \n> Tests?\n> \n\nI added a test which checks that a valid patch is produced and that\nthe signature from the file appears in the output.\n\n> Thanks and hope that helps,\n> Jonathan\n\nIn addition to addressing the suggestions from Jonathan I also\nupdated the Documentation.\n\nThis solution uses a static buffer to store the signature which does\ncreate a size limitation (1024 bytes). I considered a solution\nusing malloc but I could not figure out a clean way of determining when\nto free the memory.\n\nThanks for the help and suggestions.\n\nJeremiah Mahler (1):\n format-patch --signature-file \n\n Documentation/git-format-patch.txt | 7 +++++++\n builtin/log.c | 24 ++++++++++++++++++++++++\n t/t4014-format-patch.sh | 13 +++++++++++++\n 3 files changed, 44 insertions(+)\n\n-- \nJeremiah Mahler\njmmahler@gmail.com\nhttp://github.com/jmahler\n"},{"id":"241853","messageId":"1400203881-2794-2-git-send-email-jmmahler@gmail.com","threadId":"36673","inReplyTo":"1400203881-2794-1-git-send-email-jmmahler@gmail.com","subject":"[PATCH v2] format-patch --signature-file ","fromName":"Jeremiah Mahler","fromEmail":"jmmahler@gmail.com","sentAt":"2014-05-16T01:31:21Z","receivedAt":"2014-05-16T01:31:21Z","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 \n---\n Documentation/git-format-patch.txt | 7 +++++++\n builtin/log.c | 24 ++++++++++++++++++++++++\n t/t4014-format-patch.sh | 13 +++++++++++++\n 3 files changed, 44 insertions(+)\n\ndiff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt\nindex 5c0a4ab..dd57841 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,12 @@ 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+\tAdd a signature, by including the contents of a file, to each message\n+\tproduced. Per RFC 3676 the signature is separated from the body by a\n+\tline with '-- ' on it. If the signature option is omitted the signature\n+\tdefaults to the Git version number.\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..4a7308d 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1147,6 +1147,27 @@ static int from_callback(const struct option *opt, const char *arg, int unset)\n \treturn 0;\n }\n \n+static int signature_file_callback(const struct option *opt, const char *arg,\n+\t\t\t\t\t\t\tint unset)\n+{\n+\tconst char **signature = opt->value;\n+\tstatic char buf[1024];\n+\tsize_t sz;\n+\tFILE *fd;\n+\n+\tfd = fopen(arg, \"r\");\n+\tif (fd) {\n+\t\tsz = sizeof(buf);\n+\t\tsz = fread(buf, 1, sz - 1, fd);\n+\t\tif (sz) {\n+\t\t\tbuf[sz] = '\\0';\n+\t\t\t*signature = buf;\n+\t\t}\n+\t\tfclose(fd);\n+\t}\n+\treturn 0;\n+}\n+\n int cmd_format_patch(int argc, const char **argv, const char *prefix)\n {\n \tstruct commit *commit;\n@@ -1230,6 +1251,9 @@ 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\t{ OPTION_CALLBACK, 0, \"signature-file\", &signature, N_(\"signature-file\"),\n+\t\t\t\tN_(\"add a signature from contents of a file\"),\n+\t\t\t PARSE_OPT_NONEG, signature_file_callback },\n \t\tOPT__QUIET(&quiet, N_(\"don't print the patch filenames\")),\n \t\tOPT_END()\n \t};\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex 9c80633..19b67e3 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -762,6 +762,19 @@ test_expect_success 'format-patch --signature=\"\" suppresses signatures' '\n \t! grep \"^-- \\$\" output\n '\n \n+cat > expect << EOF\n+Test User \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 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":"241856","messageId":"20140516081445.GA21468@sigill.intra.peff.net","threadId":"36673","inReplyTo":"1400203881-2794-2-git-send-email-jmmahler@gmail.com","subject":"Re: [PATCH v2] format-patch --signature-file ","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-16T08:14:45Z","receivedAt":"2014-05-16T08:14:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 15, 2014 at 06:31:21PM -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\nI think this version looks nicer than the original.\n\nA few questions/comments:\n\n> +static int signature_file_callback(const struct option *opt, const char *arg,\n> +\t\t\t\t\t\t\tint unset)\n> +{\n> +\tconst char **signature = opt->value;\n> +\tstatic char buf[1024];\n> +\tsize_t sz;\n> +\tFILE *fd;\n> +\n> +\tfd = fopen(arg, \"r\");\n> +\tif (fd) {\n> +\t\tsz = sizeof(buf);\n> +\t\tsz = fread(buf, 1, sz - 1, fd);\n> +\t\tif (sz) {\n> +\t\t\tbuf[sz] = '\\0';\n> +\t\t\t*signature = buf;\n> +\t\t}\n> +\t\tfclose(fd);\n> +\t}\n> +\treturn 0;\n> +}\n\nWe have routines for reading directly into a strbuf, which eliminates\nthe need for this 1024-byte limit. We even have a wrapper that can make\nthis much shorter:\n\n struct strbuf buf = STRBUF_INIT;\n\n strbuf_read_file(&buf, arg, 128);\n *signature = strbuf_detach(&buf, NULL);\n\nI notice that you ignore any errors. Is that intentional (so that we\nsilently ignore missing --signature files)? If so, should we actually\ntreat it as an empty file (e.g., in my code above, we always set\n*signature, even if the file was missing)?\n\nFinally, I suspect that:\n\n cd path/in/repo &&\n git format-patch --signature-file=foo\n\nwill not work, as we chdir() to the toplevel before evaluating the\narguments. You can fix that either by using parse-option's OPT_FILENAME\nto save the filename, followed by opening the file after all arguments\nare processed; or by manually fixing up the filename.\n\nSince parse-options already knows how to do this fixup (it does it for\nOPT_FILENAME), it would be nice if it were a flag rather than a full\ntype, so you could specify at as an option to your callback here:\n\n> +\t\t{ OPTION_CALLBACK, 0, \"signature-file\", &signature, N_(\"signature-file\"),\n> +\t\t\t\tN_(\"add a signature from contents of a file\"),\n> +\t\t\t PARSE_OPT_NONEG, signature_file_callback },\n\nNoticing your OPT_NONEG, though, I wonder if you should simply use an\nOPT_FILENAME. I would expect --no-signature-file to countermand any\nearlier --signature-file on the command-line (or if we eventually grow a\nconfig option, which seems sensible, it would tell git to ignore the\noption). The usual ordering for that is:\n\n 1. Read config and store format.signatureFile as a string\n \"signature_file\".\n\n 2. Parse arguments. --signature-file=... sets signature_file, and\n --no-signature-file sets it to NULL.\n\n 3. If signature_file is non-NULL, load it.\n\nAnd I believe OPT_FILENAME will implement (2) for you.\n\nOne downside of doing it this way is that you need to specify what will\nhappen when both \"--signature\" (or format.signature) and\n\"--signature-file\" are set. With your current code, I think\n\"--signature=foo --signature-file=bar\" will take the second one. I think\nit would be fine to prefer one to the other, or to just notice that both\nare set and complain.\n\n-Peff\n"},{"id":"242039","messageId":"20140517072548.GA18239@hudson.localdomain","threadId":"36673","inReplyTo":"20140516081445.GA21468@sigill.intra.peff.net","subject":"Re: [PATCH v2] format-patch --signature-file ","fromName":"Jeremiah Mahler","fromEmail":"jmmahler@gmail.com","sentAt":"2014-05-17T07:25:48Z","receivedAt":"2014-05-17T07:25:48Z","isPatch":true,"sender":{"key":"jmmahler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/154028?v=4"},"body":"\nOn Fri, May 16, 2014 at 04:14:45AM -0400, Jeff King wrote:\n> On Thu, May 15, 2014 at 06:31:21PM -0700, Jeremiah Mahler wrote:\n...\n> \n> A few questions/comments:\n> \n> > +static int signature_file_callback(const struct option *opt, const char *arg,\n> > +\t\t\t\t\t\t\tint unset)\n> > +{\n> > +\tconst char **signature = opt->value;\n> > +\tstatic char buf[1024];\n> > +\tsize_t sz;\n> > +\tFILE *fd;\n> > +\n> > +\tfd = fopen(arg, \"r\");\n> > +\tif (fd) {\n> > +\t\tsz = sizeof(buf);\n> > +\t\tsz = fread(buf, 1, sz - 1, fd);\n> > +\t\tif (sz) {\n> > +\t\t\tbuf[sz] = '\\0';\n> > +\t\t\t*signature = buf;\n> > +\t\t}\n> > +\t\tfclose(fd);\n> > +\t}\n> > +\treturn 0;\n> > +}\n> \n> We have routines for reading directly into a strbuf, which eliminates\n> the need for this 1024-byte limit. We even have a wrapper that can make\n> this much shorter:\n> \n> struct strbuf buf = STRBUF_INIT;\n> \n> strbuf_read_file(&buf, arg, 128);\n> *signature = strbuf_detach(&buf, NULL);\n> \n\nYes, that is much cleaner.\nThe memory returned by strbuf_detach() will have to be freed as well.\n\n> I notice that you ignore any errors. Is that intentional (so that we\n> silently ignore missing --signature files)? If so, should we actually\n> treat it as an empty file (e.g., in my code above, we always set\n> *signature, even if the file was missing)?\n> \n> Finally, I suspect that:\n> \n> cd path/in/repo &&\n> git format-patch --signature-file=foo\n> \n> will not work, as we chdir() to the toplevel before evaluating the\n> arguments. You can fix that either by using parse-option's OPT_FILENAME\n> to save the filename, followed by opening the file after all arguments\n> are processed; or by manually fixing up the filename.\n> \n\nYes, it wouldn't have worked.\nUsing OPT_FILENAME is a much better solution.\n\n> Since parse-options already knows how to do this fixup (it does it for\n> OPT_FILENAME), it would be nice if it were a flag rather than a full\n> type, so you could specify at as an option to your callback here:\n> \n> > +\t\t{ OPTION_CALLBACK, 0, \"signature-file\", &signature, N_(\"signature-file\"),\n> > +\t\t\t\tN_(\"add a signature from contents of a file\"),\n> > +\t\t\t PARSE_OPT_NONEG, signature_file_callback },\n> \n> Noticing your OPT_NONEG, though, I wonder if you should simply use an\n> OPT_FILENAME. I would expect --no-signature-file to countermand any\n> earlier --signature-file on the command-line (or if we eventually grow a\n> config option, which seems sensible, it would tell git to ignore the\n> option). The usual ordering for that is:\n> \n\nAnother case is when both --signature=\"foo\" and --no-signature-file are used.\nCurrently this would only negate the file option which would allow\nthe --signature option to be used.\n\n> 1. Read config and store format.signatureFile as a string\n> \"signature_file\".\n> \n> 2. Parse arguments. --signature-file=... sets signature_file, and\n> --no-signature-file sets it to NULL.\n> \n> 3. If signature_file is non-NULL, load it.\n> \n> And I believe OPT_FILENAME will implement (2) for you.\n> \n> One downside of doing it this way is that you need to specify what will\n> happen when both \"--signature\" (or format.signature) and\n> \"--signature-file\" are set. With your current code, I think\n> \"--signature=foo --signature-file=bar\" will take the second one. I think\n> it would be fine to prefer one to the other, or to just notice that both\n> are set and complain.\n> \n> -Peff\n\nHaving --signature-file override --signature seems simpler to implement.\nThe signature variable has a default value which complicates\ndetermining whether it was set or not.\n\nThanks for the great suggestions.\n\n-- \nJeremiah Mahler\njmmahler@gmail.com\nhttp://github.com/jmahler\n"},{"id":"242040","messageId":"20140517074224.GA16697@sigill.intra.peff.net","threadId":"36673","inReplyTo":"20140517072548.GA18239@hudson.localdomain","subject":"Re: [PATCH v2] format-patch --signature-file ","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-17T07:42:24Z","receivedAt":"2014-05-17T07:42:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, May 17, 2014 at 12:25:48AM -0700, Jeremiah Mahler wrote:\n\n> > We have routines for reading directly into a strbuf, which eliminates\n> > the need for this 1024-byte limit. We even have a wrapper that can make\n> > this much shorter:\n> > \n> > struct strbuf buf = STRBUF_INIT;\n> > \n> > strbuf_read_file(&buf, arg, 128);\n> > *signature = strbuf_detach(&buf, NULL);\n> > \n> \n> Yes, that is much cleaner.\n> The memory returned by strbuf_detach() will have to be freed as well.\n\nIn cases like this, we often let the memory leak. It's in a global that\nstays valid through the whole program, so we just let the program's exit\nclean it up.\n\n> Having --signature-file override --signature seems simpler to implement.\n> The signature variable has a default value which complicates\n> determining whether it was set or not.\n\nYeah, the default value complicates it. I think you can handle that just\nby moving the default to the main logic, like:\n\n static const char *signature;\n static const char *signature_file;\n\n ...\n\n if (signature) {\n\tif (signature_file)\n\t\tdie(\"you cannot specify both a signature and a signature-file\");\n\t/* otherwise, we already have the value */\n } else if (signature_file) {\n\tstruct strbuf buf = STRBUF_INIT;\n\tstrbuf_read(&buf, signature_file, 128);\n\tsignature = strbuf_detach(&buf);\n } else\n\tsignature = git_version_string;\n\nand as a bonus, that keeps all of the logic together in one (fairly\nreadable) chain.\n\n-Peff\n"},{"id":"242042","messageId":"20140517085911.GA18862@hudson.localdomain","threadId":"36673","inReplyTo":"20140517074224.GA16697@sigill.intra.peff.net","subject":"Re: [PATCH v2] format-patch --signature-file ","fromName":"Jeremiah Mahler","fromEmail":"jmmahler@gmail.com","sentAt":"2014-05-17T08:59:11Z","receivedAt":"2014-05-17T08:59:11Z","isPatch":true,"sender":{"key":"jmmahler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/154028?v=4"},"body":"On Sat, May 17, 2014 at 03:42:24AM -0400, Jeff King wrote:\n> On Sat, May 17, 2014 at 12:25:48AM -0700, Jeremiah Mahler wrote:\n> \n> > > We have routines for reading directly into a strbuf, which eliminates\n> > > the need for this 1024-byte limit. We even have a wrapper that can make\n> > > this much shorter:\n> > > \n> > > struct strbuf buf = STRBUF_INIT;\n> > > \n> > > strbuf_read_file(&buf, arg, 128);\n> > > *signature = strbuf_detach(&buf, NULL);\n> > > \n> > \n> > Yes, that is much cleaner.\n> > The memory returned by strbuf_detach() will have to be freed as well.\n> \n> In cases like this, we often let the memory leak. It's in a global that\n> stays valid through the whole program, so we just let the program's exit\n> clean it up.\n> \n\nIt bugs me but I see your point.\nIt works just fine in this situation.\n\n> > Having --signature-file override --signature seems simpler to implement.\n> > The signature variable has a default value which complicates\n> > determining whether it was set or not.\n> \n> Yeah, the default value complicates it. I think you can handle that just\n> by moving the default to the main logic, like:\n> \n> static const char *signature;\n> static const char *signature_file;\n> \n> ...\n> \n> if (signature) {\n> \tif (signature_file)\n> \t\tdie(\"you cannot specify both a signature and a signature-file\");\n> \t/* otherwise, we already have the value */\n> } else if (signature_file) {\n> \tstruct strbuf buf = STRBUF_INIT;\n> \tstrbuf_read(&buf, signature_file, 128);\n> \tsignature = strbuf_detach(&buf);\n> } else\n> \tsignature = git_version_string;\n> \n\nBefore, --no-signature would clear the &signature.\nWith this code it sees it as not being set and assigns\nthe default version string.\n\n> and as a bonus, that keeps all of the logic together in one (fairly\n> readable) chain.\n> \n> -Peff\n\n-- \nJeremiah Mahler\njmmahler@gmail.com\nhttp://github.com/jmahler\n"},{"id":"242043","messageId":"20140517100013.GA18087@sigill.intra.peff.net","threadId":"36673","inReplyTo":"20140517085911.GA18862@hudson.localdomain","subject":"Re: [PATCH v2] format-patch --signature-file ","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-17T10:00:14Z","receivedAt":"2014-05-17T10:00:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, May 17, 2014 at 01:59:11AM -0700, Jeremiah Mahler wrote:\n\n> > if (signature) {\n> > \tif (signature_file)\n> > \t\tdie(\"you cannot specify both a signature and a signature-file\");\n> > \t/* otherwise, we already have the value */\n> > } else if (signature_file) {\n> > \tstruct strbuf buf = STRBUF_INIT;\n> > \tstrbuf_read(&buf, signature_file, 128);\n> > \tsignature = strbuf_detach(&buf);\n> > } else\n> > \tsignature = git_version_string;\n> > \n> \n> Before, --no-signature would clear the &signature.\n> With this code it sees it as not being set and assigns\n> the default version string.\n\nAh, you're right. Thanks for catching it.\n\nIf you wanted to know whether it was set, I guess you'd have to compare\nit to the default, like:\n\n if (signature_file) {\n\tif (signature && signature != git_version_string)\n\t\tdie(\"you cannot specify both a signature and a signature-file\");\n\t... read signature file ...\n }\n\nthough it's a bit ugly that this code has to know what the default is.\nHaving signature-file take precedence is OK with me, but it feels\nsomewhat arbitrary to me from the user's perspective.\n\n-Peff\n"},{"id":"242055","messageId":"20140517153943.GB31912@hudson.localdomain","threadId":"36673","inReplyTo":"20140517100013.GA18087@sigill.intra.peff.net","subject":"Re: [PATCH v2] format-patch --signature-file ","fromName":"Jeremiah Mahler","fromEmail":"jmmahler@gmail.com","sentAt":"2014-05-17T15:39:43Z","receivedAt":"2014-05-17T15:39:43Z","isPatch":true,"sender":{"key":"jmmahler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/154028?v=4"},"body":"On Sat, May 17, 2014 at 06:00:14AM -0400, Jeff King wrote:\n> On Sat, May 17, 2014 at 01:59:11AM -0700, Jeremiah Mahler wrote:\n> \n> > > if (signature) {\n> > > \tif (signature_file)\n> > > \t\tdie(\"you cannot specify both a signature and a signature-file\");\n> > > \t/* otherwise, we already have the value */\n> > > } else if (signature_file) {\n> > > \tstruct strbuf buf = STRBUF_INIT;\n> > > \tstrbuf_read(&buf, signature_file, 128);\n> > > \tsignature = strbuf_detach(&buf);\n> > > } else\n> > > \tsignature = git_version_string;\n> > > \n> > \n> > Before, --no-signature would clear the &signature.\n> > With this code it sees it as not being set and assigns\n> > the default version string.\n> \n> Ah, you're right. Thanks for catching it.\n> \n> If you wanted to know whether it was set, I guess you'd have to compare\n> it to the default, like:\n> \n> if (signature_file) {\n> \tif (signature && signature != git_version_string)\n> \t\tdie(\"you cannot specify both a signature and a signature-file\");\n> \t... read signature file ...\n> }\n> \n\nThat works until someone changes the default value.\nBut if they did that then some tests should fail.\n\nI like the address comparision which avoids a string comparision.\n\n> though it's a bit ugly that this code has to know what the default is.\n> Having signature-file take precedence is OK with me, but it feels\n> somewhat arbitrary to me from the user's perspective.\n> \n> -Peff\n\n-- \nJeremiah Mahler\njmmahler@gmail.com\nhttp://github.com/jmahler\n"},{"id":"242179","messageId":"xmqqd2f93e0m.fsf@gitster.dls.corp.google.com","threadId":"36673","inReplyTo":"20140517153943.GB31912@hudson.localdomain","subject":"Re: [PATCH v2] format-patch --signature-file ","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-19T16:54:33Z","receivedAt":"2014-05-19T16:54:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeremiah Mahler writes:\n\n> On Sat, May 17, 2014 at 06:00:14AM -0400, Jeff King wrote:\n>> \n>> If you wanted to know whether it was set, I guess you'd have to compare\n>> it to the default, like:\n>> \n>> if (signature_file) {\n>> \tif (signature && signature != git_version_string)\n>> \t\tdie(\"you cannot specify both a signature and a signature-file\");\n>> \t... read signature file ...\n>> }\n>> \n>\n> That works until someone changes the default value.\n> But if they did that then some tests should fail.\n>\n> I like the address comparision which avoids a string comparision.\n\nWell, \"avoids\" is not quite a correct phrasing, because !strcmp()\nwould be wrong there. You cannot tell \"the user did not set\nanything and the variable stayed as the default\" and \"the user\nexplicitly gave us a string but it happened to be the same as the\ndefault\" apart with !strcmp(). Address comparison is not just\n\"avoids\" but is the right thing to do in this case.\n\n>> though it's a bit ugly that this code has to know what the default is.\n\nAvoiding that is easy with an indirection, no? Something like this\nat the top:\n\n static const char *the_default_signature = git_version_string;\n static const char *signature = the_default_signature;\n\nand comparing to see if signature points at the same address as\nthe_default_signature would give you what you want, I think.\n"},{"id":"242220","messageId":"20140520054621.GA28317@hudson.localdomain","threadId":"36673","inReplyTo":"xmqqd2f93e0m.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2] format-patch --signature-file ","fromName":"Jeremiah Mahler","fromEmail":"jmmahler@gmail.com","sentAt":"2014-05-20T05:46:21Z","receivedAt":"2014-05-20T05:46:21Z","isPatch":true,"sender":{"key":"jmmahler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/154028?v=4"},"body":"\nOn Mon, May 19, 2014 at 09:54:33AM -0700, Junio C Hamano wrote:\n> Jeremiah Mahler writes:\n> \n> > On Sat, May 17, 2014 at 06:00:14AM -0400, Jeff King wrote:\n> >> \n>\n> Avoiding that is easy with an indirection, no? Something like this\n> at the top:\n> \n> static const char *the_default_signature = git_version_string;\n> static const char *signature = the_default_signature;\n> \n> and comparing to see if signature points at the same address as\n> the_default_signature would give you what you want, I think.\n\nI like your suggestion for a default signature variable.\nUnfortunately, C doesn't like it as much.\n\nstatic const char *the_default_signature = git_version_string;\nstatic const char *signature = the_default_signature; // ERROR\n// initializer element is not constant\n\n-- \nJeremiah Mahler\njmmahler@gmail.com\nhttp://github.com/jmahler\n"},{"id":"242235","messageId":"20140520062132.GA5222@sigill.intra.peff.net","threadId":"36673","inReplyTo":"20140520054621.GA28317@hudson.localdomain","subject":"Re: [PATCH v2] format-patch --signature-file ","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-20T06:21:32Z","receivedAt":"2014-05-20T06:21:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 19, 2014 at 10:46:21PM -0700, Jeremiah Mahler wrote:\n\n> > Avoiding that is easy with an indirection, no? Something like this\n> > at the top:\n> > \n> > static const char *the_default_signature = git_version_string;\n> > static const char *signature = the_default_signature;\n> > \n> > and comparing to see if signature points at the same address as\n> > the_default_signature would give you what you want, I think.\n> \n> I like your suggestion for a default signature variable.\n> Unfortunately, C doesn't like it as much.\n> \n> static const char *the_default_signature = git_version_string;\n> static const char *signature = the_default_signature; // ERROR\n> // initializer element is not constant\n\nYou could do:\n\n #define DEFAULT_SIGNATURE git_version_string\n static const char *signature = DEFAULT_SIGNATURE;\n\n ...\n\n if (signature == DEFAULT_SIGNATURE)\n ...\n\nbut maybe that is getting a little unwieldy, considering the scope of\nthe original problem.\n\n-Peff\n"},{"id":"242273","messageId":"xmqqzjic1oc5.fsf@gitster.dls.corp.google.com","threadId":"36673","inReplyTo":"20140520062132.GA5222@sigill.intra.peff.net","subject":"Re: [PATCH v2] format-patch --signature-file ","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-20T15:06:50Z","receivedAt":"2014-05-20T15:06:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> You could do:\n>\n> #define DEFAULT_SIGNATURE git_version_string\n> static const char *signature = DEFAULT_SIGNATURE;\n>\n> ...\n>\n> if (signature == DEFAULT_SIGNATURE)\n> ...\n>\n> but maybe that is getting a little unwieldy, considering the scope of\n> the original problem.\n\nI agree. It is an invitation for doing something like\n\n\t#define DEFAULT_SIGNATURE \"-- \\nfoo bar\\n\"\n\nwhich defeats the purpose.\n\nI have an unrelated and larger design level suggestion, but it may\nbe better for me to refrain from sending it during the -rc freeze.\n"},{"id":"242347","messageId":"20140521004559.GB4927@hudson.localdomain","threadId":"36673","inReplyTo":"xmqqzjic1oc5.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2] format-patch --signature-file ","fromName":"Jeremiah Mahler","fromEmail":"jmmahler@gmail.com","sentAt":"2014-05-21T00:45:59Z","receivedAt":"2014-05-21T00:45:59Z","isPatch":true,"sender":{"key":"jmmahler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/154028?v=4"},"body":"On Tue, May 20, 2014 at 08:06:50AM -0700, Junio C Hamano wrote:\n> Jeff King writes:\n> \n> > You could do:\n> >\n> > #define DEFAULT_SIGNATURE git_version_string\n> > static const char *signature = DEFAULT_SIGNATURE;\n> >\n> > ...\n> >\n> > if (signature == DEFAULT_SIGNATURE)\n> > ...\n> >\n> > but maybe that is getting a little unwieldy, considering the scope of\n> > the original problem.\n> \n> I agree. It is an invitation for doing something like\n> \n> \t#define DEFAULT_SIGNATURE \"-- \\nfoo bar\\n\"\n> \n> which defeats the purpose.\n> \n> I have an unrelated and larger design level suggestion, but it may\n> be better for me to refrain from sending it during the -rc freeze.\n\nI will take DEFAULT_SIGNATURE out of the next patch revision.\n\n-- \nJeremiah Mahler\njmmahler@gmail.com\nhttp://github.com/jmahler\n"}]}