{"thread":{"id":"18347","subject":"[PATCH] builtin-tag.c: remove global variable to use the callback data of git-config.","startedAt":"2009-03-17T14:43:51Z","lastAt":"2009-03-18T00:50:52Z","messageCount":6,"participants":["Carlos Rica","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"108231","messageId":"1237301031.10001.13.camel@equipo-loli","threadId":"18347","inReplyTo":null,"subject":"[PATCH] builtin-tag.c: remove global variable to use the callback data of git-config.","fromName":"Carlos Rica","fromEmail":"jasampler@gmail.com","sentAt":"2009-03-17T14:43:51Z","receivedAt":"2009-03-17T14:43:51Z","isPatch":true,"sender":{"key":"jasampler@gmail.com","avatar":null},"body":"By using strbuf to save the signing-key id, it also imposes no limit\nto the length of the string obtained from the config or command-line.\nThis string is then passed to gpg to sign the tag, when appropriate.\n\nSigned-off-by: Carlos Rica \n---\n\n\nQUESTION: Is it safe to remove this limit?\n\n\n builtin-tag.c | 39 ++++++++++++++++-----------------------\n 1 files changed, 16 insertions(+), 23 deletions(-)\n\ndiff --git a/builtin-tag.c b/builtin-tag.c\nindex 01e7374..ed8b24f 100644\n--- a/builtin-tag.c\n+++ b/builtin-tag.c\n@@ -21,8 +21,6 @@ static const char * const git_tag_usage[] = {\n \tNULL\n };\n \n-static char signingkey[1000];\n-\n struct tag_filter {\n \tconst char *pattern;\n \tint lines;\n@@ -156,7 +154,7 @@ static int verify_tag(const char *name, const char *ref,\n \treturn 0;\n }\n \n-static int do_sign(struct strbuf *buffer)\n+static int do_sign(struct strbuf *signingkey, struct strbuf *buffer)\n {\n \tstruct child_process gpg;\n \tconst char *args[4];\n@@ -164,11 +162,10 @@ static int do_sign(struct strbuf *buffer)\n \tint len;\n \tint i, j;\n \n-\tif (!*signingkey) {\n-\t\tif (strlcpy(signingkey, git_committer_info(IDENT_ERROR_ON_NO_NAME),\n-\t\t\t\tsizeof(signingkey)) > sizeof(signingkey) - 1)\n-\t\t\treturn error(\"committer info too long.\");\n-\t\tbracket = strchr(signingkey, '>');\n+\tif (!signingkey->buf[0]) {\n+\t\tstrbuf_addstr(signingkey,\n+\t\t\t\tgit_committer_info(IDENT_ERROR_ON_NO_NAME));\n+\t\tbracket = strchr(signingkey->buf, '>');\n \t\tif (bracket)\n \t\t\tbracket[1] = '\\0';\n \t}\n@@ -183,7 +180,7 @@ static int do_sign(struct strbuf *buffer)\n \tgpg.out = -1;\n \targs[0] = \"gpg\";\n \targs[1] = \"-bsau\";\n-\targs[2] = signingkey;\n+\targs[2] = signingkey->buf;\n \targs[3] = NULL;\n \n \tif (start_command(&gpg))\n@@ -220,18 +217,12 @@ static const char tag_template[] =\n \t\"# Write a tag message\\n\"\n \t\"#\\n\";\n \n-static void set_signingkey(const char *value)\n-{\n-\tif (strlcpy(signingkey, value, sizeof(signingkey)) >= sizeof(signingkey))\n-\t\tdie(\"signing key value too long (%.10s...)\", value);\n-}\n-\n static int git_tag_config(const char *var, const char *value, void *cb)\n {\n \tif (!strcmp(var, \"user.signingkey\")) {\n \t\tif (!value)\n \t\t\treturn config_error_nonbool(var);\n-\t\tset_signingkey(value);\n+\t\tstrbuf_addstr((struct strbuf *) cb, value);\n \t\treturn 0;\n \t}\n \n@@ -266,9 +257,10 @@ static void write_tag_body(int fd, const unsigned char *sha1)\n \tfree(buf);\n }\n \n-static int build_tag_object(struct strbuf *buf, int sign, unsigned char *result)\n+static int build_tag_object(struct strbuf *buf, int sign,\n+\t\t\tstruct strbuf *signingkey, unsigned char *result)\n {\n-\tif (sign && do_sign(buf) < 0)\n+\tif (sign && do_sign(signingkey, buf) < 0)\n \t\treturn error(\"unable to sign the tag\");\n \tif (write_sha1_file(buf->buf, buf->len, tag_type, result) < 0)\n \t\treturn error(\"unable to write tag file\");\n@@ -277,6 +269,7 @@ static int build_tag_object(struct strbuf *buf, int sign, unsigned char *result)\n \n static void create_tag(const unsigned char *object, const char *tag,\n \t\t struct strbuf *buf, int message, int sign,\n+\t\t struct strbuf *signingkey,\n \t\t unsigned char *prev, unsigned char *result)\n {\n \tenum object_type type;\n@@ -331,7 +324,7 @@ static void create_tag(const unsigned char *object, const char *tag,\n \n \tstrbuf_insert(buf, 0, header_buf, header_len);\n \n-\tif (build_tag_object(buf, sign, result) < 0) {\n+\tif (build_tag_object(buf, sign, signingkey, result) < 0) {\n \t\tif (path)\n \t\t\tfprintf(stderr, \"The tag message has been left in %s\\n\",\n \t\t\t\tpath);\n@@ -363,7 +356,7 @@ static int parse_msg_arg(const struct option *opt, const char *arg, int unset)\n \n int cmd_tag(int argc, const char **argv, const char *prefix)\n {\n-\tstruct strbuf buf = STRBUF_INIT;\n+\tstruct strbuf buf = STRBUF_INIT, signingkey = STRBUF_INIT;\n \tunsigned char object[20], prev[20];\n \tchar ref[PATH_MAX];\n \tconst char *object_ref, *tag;\n@@ -403,14 +396,14 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\tOPT_END()\n \t};\n \n-\tgit_config(git_tag_config, NULL);\n+\tgit_config(git_tag_config, &signingkey);\n \n \targc = parse_options(argc, argv, options, git_tag_usage, 0);\n \tmsgfile = parse_options_fix_filename(prefix, msgfile);\n \n \tif (keyid) {\n \t\tsign = 1;\n-\t\tset_signingkey(keyid);\n+\t\tstrbuf_addstr(&signingkey, keyid);\n \t}\n \tif (sign)\n \t\tannotate = 1;\n@@ -474,7 +467,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \n \tif (annotate)\n \t\tcreate_tag(object, tag, &buf, msg.given || msgfile,\n-\t\t\t sign, prev, object);\n+\t\t\t sign, &signingkey, prev, object);\n \n \tlock = lock_any_ref_for_update(ref, prev, 0);\n \tif (!lock)\n-- \n1.6.0.5\n"},{"id":"108240","messageId":"alpine.DEB.1.00.0903171646140.6393@intel-tinevez-2-302","threadId":"18347","inReplyTo":"1237301031.10001.13.camel@equipo-loli","subject":"Re: [PATCH] builtin-tag.c: remove global variable to use the callback data of git-config.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-03-17T15:47:45Z","receivedAt":"2009-03-17T15:47:45Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 17 Mar 2009, Carlos Rica wrote:\n\n> By using strbuf to save the signing-key id, it also imposes no limit\n> to the length of the string obtained from the config or command-line.\n> This string is then passed to gpg to sign the tag, when appropriate.\n> \n> Signed-off-by: Carlos Rica \n> ---\n> \n> \n> QUESTION: Is it safe to remove this limit?\n\nI think so. GPG should return an error if it thinks it is too large.\n\n> @@ -164,11 +162,10 @@ static int do_sign(struct strbuf *buffer)\n> \tint len;\n> \tint i, j;\n> \n> -\tif (!*signingkey) {\n> -\t\tif (strlcpy(signingkey, git_committer_info(IDENT_ERROR_ON_NO_NAME),\n> -\t\t\t\tsizeof(signingkey)) > sizeof(signingkey) - 1)\n> -\t\t\treturn error(\"committer info too long.\");\n> -\t\tbracket = strchr(signingkey, '>');\n> +\tif (!signingkey->buf[0]) {\n\nIt is probably better to ask for !signingkey->len (think of trying to \nunderstand the code in 6 months from now).\n\nOther than that, very nice!\n\nCiao,\nDscho\n"},{"id":"108268","messageId":"1b46aba20903171057r4fb4697eo3b8abc62a45fe858@mail.gmail.com","threadId":"18347","inReplyTo":"alpine.DEB.1.00.0903171646140.6393@intel-tinevez-2-302","subject":"Re: [PATCH] builtin-tag.c: remove global variable to use the callback data of git-config.","fromName":"Carlos Rica","fromEmail":"jasampler@gmail.com","sentAt":"2009-03-17T17:57:28Z","receivedAt":"2009-03-17T17:57:28Z","isPatch":true,"sender":{"key":"jasampler@gmail.com","avatar":null},"body":"On Tue, Mar 17, 2009 at 4:47 PM, Johannes Schindelin\n wrote:\n> Hi,\n> On Tue, 17 Mar 2009, Carlos Rica wrote:\n>> @@ -164,11 +162,10 @@ static int do_sign(struct strbuf *buffer)\n>> int len;\n>> int i, j;\n>>\n>> - if (!*signingkey) {\n>> - if (strlcpy(signingkey, git_committer_info(IDENT_ERROR_ON_NO_NAME),\n>> - sizeof(signingkey)) > sizeof(signingkey) - 1)\n>> - return error(\"committer info too long.\");\n>> - bracket = strchr(signingkey, '>');\n>> + if (!signingkey->buf[0]) {\n>\n> It is probably better to ask for !signingkey->len (think of trying to\n> understand the code in 6 months from now).\n\nI was in doubt here. By avoiding the use of signingkey->len I was\ntrying to say that you cannot rely in such field if we touch the\nbuffer directly, as it happens below:\n\n bracket = strchr(signingkey->buf, '>');\n if (bracket)\n bracket[1] = '\\0';\n"},{"id":"108272","messageId":"7vljr4q97u.fsf@gitster.siamese.dyndns.org","threadId":"18347","inReplyTo":"1b46aba20903171057r4fb4697eo3b8abc62a45fe858@mail.gmail.com","subject":"Re: [PATCH] builtin-tag.c: remove global variable to use the callback data of git-config.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-17T18:45:41Z","receivedAt":"2009-03-17T18:45:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlos Rica writes:\n\n> On Tue, Mar 17, 2009 at 4:47 PM, Johannes Schindelin\n> wrote:\n>> Hi,\n>> On Tue, 17 Mar 2009, Carlos Rica wrote:\n>>> @@ -164,11 +162,10 @@ static int do_sign(struct strbuf *buffer)\n>>> int len;\n>>> int i, j;\n>>>\n>>> - if (!*signingkey) {\n>>> - if (strlcpy(signingkey, git_committer_info(IDENT_ERROR_ON_NO_NAME),\n>>> - sizeof(signingkey)) > sizeof(signingkey) - 1)\n>>> - return error(\"committer info too long.\");\n>>> - bracket = strchr(signingkey, '>');\n>>> + if (!signingkey->buf[0]) {\n>>\n>> It is probably better to ask for !signingkey->len (think of trying to\n>> understand the code in 6 months from now).\n>\n> I was in doubt here. By avoiding the use of signingkey->len I was\n> trying to say that you cannot rely in such field if we touch the\n> buffer directly, as it happens below:\n>\n> bracket = strchr(signingkey->buf, '>');\n> if (bracket)\n> bracket[1] = '\\0';\n\nThat's a wrong use of strbuf, isn't it?\n"},{"id":"108287","messageId":"alpine.DEB.1.00.0903172326250.10279@pacific.mpi-cbg.de","threadId":"18347","inReplyTo":"1b46aba20903171057r4fb4697eo3b8abc62a45fe858@mail.gmail.com","subject":"Re: [PATCH] builtin-tag.c: remove global variable to use the callback data of git-config.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-03-17T22:27:23Z","receivedAt":"2009-03-17T22:27:23Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 17 Mar 2009, Carlos Rica wrote:\n\n> On Tue, Mar 17, 2009 at 4:47 PM, Johannes Schindelin\n> wrote:\n> > Hi,\n> > On Tue, 17 Mar 2009, Carlos Rica wrote:\n> >> @@ -164,11 +162,10 @@ static int do_sign(struct strbuf *buffer)\n> >> int len;\n> >> int i, j;\n> >>\n> >> - if (!*signingkey) {\n> >> - if (strlcpy(signingkey, git_committer_info(IDENT_ERROR_ON_NO_NAME),\n> >> - sizeof(signingkey)) > sizeof(signingkey) - 1)\n> >> - return error(\"committer info too long.\");\n> >> - bracket = strchr(signingkey, '>');\n> >> + if (!signingkey->buf[0]) {\n> >\n> > It is probably better to ask for !signingkey->len (think of trying to\n> > understand the code in 6 months from now).\n> \n> I was in doubt here. By avoiding the use of signingkey->len I was\n> trying to say that you cannot rely in such field if we touch the\n> buffer directly, as it happens below:\n> \n> bracket = strchr(signingkey->buf, '>');\n> if (bracket)\n> bracket[1] = '\\0';\n\nOh, I missed that. It should read\n\n\tif (bracket)\n\t\tstrbuf_setlen(signingkey, bracket + 1 - signingkey->buf);\n\ninstead.\n\nCiao,\nDscho\n"},{"id":"108303","messageId":"1b46aba20903171750p5d63d94bq347db4ce06bda95b@mail.gmail.com","threadId":"18347","inReplyTo":"alpine.DEB.1.00.0903172326250.10279@pacific.mpi-cbg.de","subject":"Re: [PATCH] builtin-tag.c: remove global variable to use the callback data of git-config.","fromName":"Carlos Rica","fromEmail":"jasampler@gmail.com","sentAt":"2009-03-18T00:50:52Z","receivedAt":"2009-03-18T00:50:52Z","isPatch":true,"sender":{"key":"jasampler@gmail.com","avatar":null},"body":"On Tue, Mar 17, 2009 at 11:27 PM, Johannes Schindelin\n wrote:\n> Hi,\n>\n> On Tue, 17 Mar 2009, Carlos Rica wrote:\n>\n>> On Tue, Mar 17, 2009 at 4:47 PM, Johannes Schindelin\n>> wrote:\n>> > Hi,\n>> > On Tue, 17 Mar 2009, Carlos Rica wrote:\n>> >> @@ -164,11 +162,10 @@ static int do_sign(struct strbuf *buffer)\n>> >> int len;\n>> >> int i, j;\n>> >>\n>> >> - if (!*signingkey) {\n>> >> - if (strlcpy(signingkey, git_committer_info(IDENT_ERROR_ON_NO_NAME),\n>> >> - sizeof(signingkey)) > sizeof(signingkey) - 1)\n>> >> - return error(\"committer info too long.\");\n>> >> - bracket = strchr(signingkey, '>');\n>> >> + if (!signingkey->buf[0]) {\n>> >\n>> > It is probably better to ask for !signingkey->len (think of trying to\n>> > understand the code in 6 months from now).\n>>\n>> I was in doubt here. By avoiding the use of signingkey->len I was\n>> trying to say that you cannot rely in such field if we touch the\n>> buffer directly, as it happens below:\n>>\n>> bracket = strchr(signingkey->buf, '>');\n>> if (bracket)\n>> bracket[1] = '\\0';\n>\n> Oh, I missed that. It should read\n>\n> if (bracket)\n> strbuf_setlen(signingkey, bracket + 1 - signingkey->buf);\n>\n> instead.\n\nI agree! That's much better. Thanks Dscho and Junio.\nTell me if you need me to resend the patch with both changes.\n"}]}