[PATCH] builtin-tag.c: remove global variable to use the callback data of git-config.

[PATCH] builtin-tag.c: remove global variable to use the callback data of git-config.

[Carlos Rica]

By using strbuf to save the signing-key id, now there's no limit
to the length of the string obtained from the config or command-line.
This string is then passed to gpg to sign the tag, when appropriate.

Signed-off-by: Carlos Rica <jasampler@gmail.com>
---


NOTES: As Johannes Schindelin suggested, now strbuf is correctly used,
       using setlen() for chopping the end data of the committer info,
       instead of just setting '\0' and forgetting the len field.

       Although git_committer_info() seems to return always the email
       field between <angle brackets>, I left unchanged the "if (bracket)"
       condition (as in builtin-log.c) to prevent errors or future changes.

       Trying to test it, I found that gpg accepts any non-empty substring
       of the name or email to identify the signing key ID, just as it is
       stated in its manpage at end of section HOW TO SPECIFY A USER ID.


 builtin-tag.c |   41 +++++++++++++++++------------------------
 1 files changed, 17 insertions(+), 24 deletions(-)

diff --git a/builtin-tag.c b/builtin-tag.c
index 01e7374..ad54e4a 100644
--- a/builtin-tag.c
+++ b/builtin-tag.c
@@ -21,8 +21,6 @@ static const char * const git_tag_usage[] = {
 	NULL
 };
 
-static char signingkey[1000];
-
 struct tag_filter {
 	const char *pattern;
 	int lines;
@@ -156,7 +154,7 @@ static int verify_tag(const char *name, const char *ref,
 	return 0;
 }
 
-static int do_sign(struct strbuf *buffer)
+static int do_sign(struct strbuf *signingkey, struct strbuf *buffer)
 {
 	struct child_process gpg;
 	const char *args[4];
@@ -164,13 +162,12 @@ static int do_sign(struct strbuf *buffer)
 	int len;
 	int i, j;
 
-	if (!*signingkey) {
-		if (strlcpy(signingkey, git_committer_info(IDENT_ERROR_ON_NO_NAME),
-				sizeof(signingkey)) > sizeof(signingkey) - 1)
-			return error("committer info too long.");
-		bracket = strchr(signingkey, '>');
+	if (!signingkey->len) {
+		strbuf_addstr(signingkey,
+				git_committer_info(IDENT_ERROR_ON_NO_NAME));
+		bracket = strchr(signingkey->buf, '>');
 		if (bracket)
-			bracket[1] = '\0';
+			strbuf_setlen(signingkey, bracket + 1 - signingkey->buf);
 	}
 
 	/* When the username signingkey is bad, program could be terminated
@@ -183,7 +180,7 @@ static int do_sign(struct strbuf *buffer)
 	gpg.out = -1;
 	args[0] = "gpg";
 	args[1] = "-bsau";
-	args[2] = signingkey;
+	args[2] = signingkey->buf;
 	args[3] = NULL;
 
 	if (start_command(&gpg))
@@ -220,18 +217,12 @@ static const char tag_template[] =
 	"# Write a tag message\n"
 	"#\n";
 
-static void set_signingkey(const char *value)
-{
-	if (strlcpy(signingkey, value, sizeof(signingkey)) >= sizeof(signingkey))
-		die("signing key value too long (%.10s...)", value);
-}
-
 static int git_tag_config(const char *var, const char *value, void *cb)
 {
 	if (!strcmp(var, "user.signingkey")) {
 		if (!value)
 			return config_error_nonbool(var);
-		set_signingkey(value);
+		strbuf_addstr((struct strbuf *) cb, value);
 		return 0;
 	}
 
@@ -266,9 +257,10 @@ static void write_tag_body(int fd, const unsigned char *sha1)
 	free(buf);
 }
 
-static int build_tag_object(struct strbuf *buf, int sign, unsigned char *result)
+static int build_tag_object(struct strbuf *buf, int sign,
+			struct strbuf *signingkey, unsigned char *result)
 {
-	if (sign && do_sign(buf) < 0)
+	if (sign && do_sign(signingkey, buf) < 0)
 		return error("unable to sign the tag");
 	if (write_sha1_file(buf->buf, buf->len, tag_type, result) < 0)
 		return error("unable to write tag file");
@@ -277,6 +269,7 @@ static int build_tag_object(struct strbuf *buf, int sign, unsigned char *result)
 
 static void create_tag(const unsigned char *object, const char *tag,
 		       struct strbuf *buf, int message, int sign,
+		       struct strbuf *signingkey,
 		       unsigned char *prev, unsigned char *result)
 {
 	enum object_type type;
@@ -331,7 +324,7 @@ static void create_tag(const unsigned char *object, const char *tag,
 
 	strbuf_insert(buf, 0, header_buf, header_len);
 
-	if (build_tag_object(buf, sign, result) < 0) {
+	if (build_tag_object(buf, sign, signingkey, result) < 0) {
 		if (path)
 			fprintf(stderr, "The tag message has been left in %s\n",
 				path);
@@ -363,7 +356,7 @@ static int parse_msg_arg(const struct option *opt, const char *arg, int unset)
 
 int cmd_tag(int argc, const char **argv, const char *prefix)
 {
-	struct strbuf buf = STRBUF_INIT;
+	struct strbuf buf = STRBUF_INIT, signingkey = STRBUF_INIT;
 	unsigned char object[20], prev[20];
 	char ref[PATH_MAX];
 	const char *object_ref, *tag;
@@ -403,14 +396,14 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
-	git_config(git_tag_config, NULL);
+	git_config(git_tag_config, &signingkey);
 
 	argc = parse_options(argc, argv, options, git_tag_usage, 0);
 	msgfile = parse_options_fix_filename(prefix, msgfile);
 
 	if (keyid) {
 		sign = 1;
-		set_signingkey(keyid);
+		strbuf_addstr(&signingkey, keyid);
 	}
 	if (sign)
 		annotate = 1;
@@ -474,7 +467,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 
 	if (annotate)
 		create_tag(object, tag, &buf, msg.given || msgfile,
-			   sign, prev, object);
+			   sign, &signingkey, prev, object);
 
 	lock = lock_any_ref_for_update(ref, prev, 0);
 	if (!lock)
-- 
1.5.4.3

[PATCH] builtin-tag.c: remove global variable to use the callback data of git-config.

[Carlos Rica]

By using strbuf to save the signing-key id, it also imposes no limit
to the length of the string obtained from the config or command-line.
This string is then passed to gpg to sign the tag, when appropriate.

Signed-off-by: Carlos Rica <jasampler@gmail.com>
---


QUESTION: Is it safe to remove this limit?


 builtin-tag.c |   39 ++++++++++++++++-----------------------
 1 files changed, 16 insertions(+), 23 deletions(-)

diff --git a/builtin-tag.c b/builtin-tag.c
index 01e7374..ed8b24f 100644
--- a/builtin-tag.c
+++ b/builtin-tag.c
@@ -21,8 +21,6 @@ static const char * const git_tag_usage[] = {
 	NULL
 };
 
-static char signingkey[1000];
-
 struct tag_filter {
 	const char *pattern;
 	int lines;
@@ -156,7 +154,7 @@ static int verify_tag(const char *name, const char *ref,
 	return 0;
 }
 
-static int do_sign(struct strbuf *buffer)
+static int do_sign(struct strbuf *signingkey, struct strbuf *buffer)
 {
 	struct child_process gpg;
 	const char *args[4];
@@ -164,11 +162,10 @@ static int do_sign(struct strbuf *buffer)
 	int len;
 	int i, j;
 
-	if (!*signingkey) {
-		if (strlcpy(signingkey, git_committer_info(IDENT_ERROR_ON_NO_NAME),
-				sizeof(signingkey)) > sizeof(signingkey) - 1)
-			return error("committer info too long.");
-		bracket = strchr(signingkey, '>');
+	if (!signingkey->buf[0]) {
+		strbuf_addstr(signingkey,
+				git_committer_info(IDENT_ERROR_ON_NO_NAME));
+		bracket = strchr(signingkey->buf, '>');
 		if (bracket)
 			bracket[1] = '\0';
 	}
@@ -183,7 +180,7 @@ static int do_sign(struct strbuf *buffer)
 	gpg.out = -1;
 	args[0] = "gpg";
 	args[1] = "-bsau";
-	args[2] = signingkey;
+	args[2] = signingkey->buf;
 	args[3] = NULL;
 
 	if (start_command(&gpg))
@@ -220,18 +217,12 @@ static const char tag_template[] =
 	"# Write a tag message\n"
 	"#\n";
 
-static void set_signingkey(const char *value)
-{
-	if (strlcpy(signingkey, value, sizeof(signingkey)) >= sizeof(signingkey))
-		die("signing key value too long (%.10s...)", value);
-}
-
 static int git_tag_config(const char *var, const char *value, void *cb)
 {
 	if (!strcmp(var, "user.signingkey")) {
 		if (!value)
 			return config_error_nonbool(var);
-		set_signingkey(value);
+		strbuf_addstr((struct strbuf *) cb, value);
 		return 0;
 	}
 
@@ -266,9 +257,10 @@ static void write_tag_body(int fd, const unsigned char *sha1)
 	free(buf);
 }
 
-static int build_tag_object(struct strbuf *buf, int sign, unsigned char *result)
+static int build_tag_object(struct strbuf *buf, int sign,
+			struct strbuf *signingkey, unsigned char *result)
 {
-	if (sign && do_sign(buf) < 0)
+	if (sign && do_sign(signingkey, buf) < 0)
 		return error("unable to sign the tag");
 	if (write_sha1_file(buf->buf, buf->len, tag_type, result) < 0)
 		return error("unable to write tag file");
@@ -277,6 +269,7 @@ static int build_tag_object(struct strbuf *buf, int sign, unsigned char *result)
 
 static void create_tag(const unsigned char *object, const char *tag,
 		       struct strbuf *buf, int message, int sign,
+		       struct strbuf *signingkey,
 		       unsigned char *prev, unsigned char *result)
 {
 	enum object_type type;
@@ -331,7 +324,7 @@ static void create_tag(const unsigned char *object, const char *tag,
 
 	strbuf_insert(buf, 0, header_buf, header_len);
 
-	if (build_tag_object(buf, sign, result) < 0) {
+	if (build_tag_object(buf, sign, signingkey, result) < 0) {
 		if (path)
 			fprintf(stderr, "The tag message has been left in %s\n",
 				path);
@@ -363,7 +356,7 @@ static int parse_msg_arg(const struct option *opt, const char *arg, int unset)
 
 int cmd_tag(int argc, const char **argv, const char *prefix)
 {
-	struct strbuf buf = STRBUF_INIT;
+	struct strbuf buf = STRBUF_INIT, signingkey = STRBUF_INIT;
 	unsigned char object[20], prev[20];
 	char ref[PATH_MAX];
 	const char *object_ref, *tag;
@@ -403,14 +396,14 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
-	git_config(git_tag_config, NULL);
+	git_config(git_tag_config, &signingkey);
 
 	argc = parse_options(argc, argv, options, git_tag_usage, 0);
 	msgfile = parse_options_fix_filename(prefix, msgfile);
 
 	if (keyid) {
 		sign = 1;
-		set_signingkey(keyid);
+		strbuf_addstr(&signingkey, keyid);
 	}
 	if (sign)
 		annotate = 1;
@@ -474,7 +467,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 
 	if (annotate)
 		create_tag(object, tag, &buf, msg.given || msgfile,
-			   sign, prev, object);
+			   sign, &signingkey, prev, object);
 
 	lock = lock_any_ref_for_update(ref, prev, 0);
 	if (!lock)
-- 
1.6.0.5

Re: [PATCH] builtin-tag.c: remove global variable to use the callback data of git-config.

[Johannes Schindelin]

Hi,

On Tue, 17 Mar 2009, Carlos Rica wrote:

> By using strbuf to save the signing-key id, it also imposes no limit
> to the length of the string obtained from the config or command-line.
> This string is then passed to gpg to sign the tag, when appropriate.
> 
> Signed-off-by: Carlos Rica <jasampler@gmail.com>
> ---
> 
> 
> QUESTION: Is it safe to remove this limit?

I think so.  GPG should return an error if it thinks it is too large.

> @@ -164,11 +162,10 @@ static int do_sign(struct strbuf *buffer)
>  	int len;
>  	int i, j;
>  
> -	if (!*signingkey) {
> -		if (strlcpy(signingkey, git_committer_info(IDENT_ERROR_ON_NO_NAME),
> -				sizeof(signingkey)) > sizeof(signingkey) - 1)
> -			return error("committer info too long.");
> -		bracket = strchr(signingkey, '>');
> +	if (!signingkey->buf[0]) {

It is probably better to ask for !signingkey->len (think of trying to 
understand the code in 6 months from now).

Other than that, very nice!

Ciao,
Dscho

Re: [PATCH] builtin-tag.c: remove global variable to use the callback data of git-config.

[Carlos Rica]

On Tue, Mar 17, 2009 at 4:47 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
> On Tue, 17 Mar 2009, Carlos Rica wrote:
>> @@ -164,11 +162,10 @@ static int do_sign(struct strbuf *buffer)
>>       int len;
>>       int i, j;
>>
>> -     if (!*signingkey) {
>> -             if (strlcpy(signingkey, git_committer_info(IDENT_ERROR_ON_NO_NAME),
>> -                             sizeof(signingkey)) > sizeof(signingkey) - 1)
>> -                     return error("committer info too long.");
>> -             bracket = strchr(signingkey, '>');
>> +     if (!signingkey->buf[0]) {
>
> It is probably better to ask for !signingkey->len (think of trying to
> understand the code in 6 months from now).

I was in doubt here. By avoiding the use of signingkey->len  I was
trying to say that you cannot rely in such field if we touch the
buffer directly, as it happens below:

   bracket = strchr(signingkey->buf, '>');
   if (bracket)
      bracket[1] = '\0';

Re: [PATCH] builtin-tag.c: remove global variable to use the callback data of git-config.

[Junio C Hamano]

Carlos Rica <jasampler@gmail.com> writes:

> On Tue, Mar 17, 2009 at 4:47 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
>> Hi,
>> On Tue, 17 Mar 2009, Carlos Rica wrote:
>>> @@ -164,11 +162,10 @@ static int do_sign(struct strbuf *buffer)
>>>       int len;
>>>       int i, j;
>>>
>>> -     if (!*signingkey) {
>>> -             if (strlcpy(signingkey, git_committer_info(IDENT_ERROR_ON_NO_NAME),
>>> -                             sizeof(signingkey)) > sizeof(signingkey) - 1)
>>> -                     return error("committer info too long.");
>>> -             bracket = strchr(signingkey, '>');
>>> +     if (!signingkey->buf[0]) {
>>
>> It is probably better to ask for !signingkey->len (think of trying to
>> understand the code in 6 months from now).
>
> I was in doubt here. By avoiding the use of signingkey->len  I was
> trying to say that you cannot rely in such field if we touch the
> buffer directly, as it happens below:
>
>    bracket = strchr(signingkey->buf, '>');
>    if (bracket)
>       bracket[1] = '\0';

That's a wrong use of strbuf, isn't it?

Re: [PATCH] builtin-tag.c: remove global variable to use the callback data of git-config.

[Johannes Schindelin]

Hi,

On Tue, 17 Mar 2009, Carlos Rica wrote:

> On Tue, Mar 17, 2009 at 4:47 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > Hi,
> > On Tue, 17 Mar 2009, Carlos Rica wrote:
> >> @@ -164,11 +162,10 @@ static int do_sign(struct strbuf *buffer)
> >>       int len;
> >>       int i, j;
> >>
> >> -     if (!*signingkey) {
> >> -             if (strlcpy(signingkey, git_committer_info(IDENT_ERROR_ON_NO_NAME),
> >> -                             sizeof(signingkey)) > sizeof(signingkey) - 1)
> >> -                     return error("committer info too long.");
> >> -             bracket = strchr(signingkey, '>');
> >> +     if (!signingkey->buf[0]) {
> >
> > It is probably better to ask for !signingkey->len (think of trying to
> > understand the code in 6 months from now).
> 
> I was in doubt here. By avoiding the use of signingkey->len  I was
> trying to say that you cannot rely in such field if we touch the
> buffer directly, as it happens below:
> 
>    bracket = strchr(signingkey->buf, '>');
>    if (bracket)
>       bracket[1] = '\0';

Oh, I missed that.  It should read

	if (bracket)
		strbuf_setlen(signingkey, bracket + 1 - signingkey->buf);

instead.

Ciao,
Dscho

Re: [PATCH] builtin-tag.c: remove global variable to use the callback data of git-config.

[Carlos Rica]

On Tue, Mar 17, 2009 at 11:27 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Tue, 17 Mar 2009, Carlos Rica wrote:
>
>> On Tue, Mar 17, 2009 at 4:47 PM, Johannes Schindelin
>> <Johannes.Schindelin@gmx.de> wrote:
>> > Hi,
>> > On Tue, 17 Mar 2009, Carlos Rica wrote:
>> >> @@ -164,11 +162,10 @@ static int do_sign(struct strbuf *buffer)
>> >>       int len;
>> >>       int i, j;
>> >>
>> >> -     if (!*signingkey) {
>> >> -             if (strlcpy(signingkey, git_committer_info(IDENT_ERROR_ON_NO_NAME),
>> >> -                             sizeof(signingkey)) > sizeof(signingkey) - 1)
>> >> -                     return error("committer info too long.");
>> >> -             bracket = strchr(signingkey, '>');
>> >> +     if (!signingkey->buf[0]) {
>> >
>> > It is probably better to ask for !signingkey->len (think of trying to
>> > understand the code in 6 months from now).
>>
>> I was in doubt here. By avoiding the use of signingkey->len  I was
>> trying to say that you cannot rely in such field if we touch the
>> buffer directly, as it happens below:
>>
>>    bracket = strchr(signingkey->buf, '>');
>>    if (bracket)
>>       bracket[1] = '\0';
>
> Oh, I missed that.  It should read
>
>        if (bracket)
>                strbuf_setlen(signingkey, bracket + 1 - signingkey->buf);
>
> instead.

I agree! That's much better. Thanks Dscho and Junio.
Tell me if you need me to resend the patch with both changes.

[PATCH] builtin-tag.c: remove global variable to use the callback data of git-config.

[Carlos Rica]

Signed-off-by: Carlos Rica <jasampler@gmail.com>
---

Here I declare a struct to wrap the new local array along with its size.
QUESTION: An alternative to this is strbuf, would it be preferable?


builtin-tag.c |   43 ++++++++++++++++++++++++++-----------------
 1 files changed, 26 insertions(+), 17 deletions(-)

diff --git a/builtin-tag.c b/builtin-tag.c
index 01e7374..2b2d728 100644
--- a/builtin-tag.c
+++ b/builtin-tag.c
@@ -21,8 +21,6 @@ static const char * const git_tag_usage[] = {
 	NULL
 };
 
-static char signingkey[1000];
-
 struct tag_filter {
 	const char *pattern;
 	int lines;
@@ -156,7 +154,12 @@ static int verify_tag(const char *name, const char *ref,
 	return 0;
 }
 
-static int do_sign(struct strbuf *buffer)
+struct char_array {
+	char *buf;
+	size_t size;
+};
+
+static int do_sign(struct char_array *signingkey, struct strbuf *buffer)
 {
 	struct child_process gpg;
 	const char *args[4];
@@ -164,11 +167,12 @@ static int do_sign(struct strbuf *buffer)
 	int len;
 	int i, j;
 
-	if (!*signingkey) {
-		if (strlcpy(signingkey, git_committer_info(IDENT_ERROR_ON_NO_NAME),
-				sizeof(signingkey)) > sizeof(signingkey) - 1)
+	if (!signingkey->buf[0]) {
+		if (strlcpy(signingkey->buf,
+				git_committer_info(IDENT_ERROR_ON_NO_NAME),
+				signingkey->size) > signingkey->size - 1)
 			return error("committer info too long.");
-		bracket = strchr(signingkey, '>');
+		bracket = strchr(signingkey->buf, '>');
 		if (bracket)
 			bracket[1] = '\0';
 	}
@@ -183,7 +187,7 @@ static int do_sign(struct strbuf *buffer)
 	gpg.out = -1;
 	args[0] = "gpg";
 	args[1] = "-bsau";
-	args[2] = signingkey;
+	args[2] = signingkey->buf;
 	args[3] = NULL;
 
 	if (start_command(&gpg))
@@ -220,9 +224,10 @@ static const char tag_template[] =
 	"# Write a tag message\n"
 	"#\n";
 
-static void set_signingkey(const char *value)
+static void set_signingkey(struct char_array *signingkey, const char *value)
 {
-	if (strlcpy(signingkey, value, sizeof(signingkey)) >= sizeof(signingkey))
+	if (strlcpy(signingkey->buf, value, signingkey->size)
+					>= signingkey->size)
 		die("signing key value too long (%.10s...)", value);
 }
 
@@ -231,7 +236,7 @@ static int git_tag_config(const char *var, const char *value, void *cb)
 	if (!strcmp(var, "user.signingkey")) {
 		if (!value)
 			return config_error_nonbool(var);
-		set_signingkey(value);
+		set_signingkey((struct char_array *) cb, value);
 		return 0;
 	}
 
@@ -266,9 +271,10 @@ static void write_tag_body(int fd, const unsigned char *sha1)
 	free(buf);
 }
 
-static int build_tag_object(struct strbuf *buf, int sign, unsigned char *result)
+static int build_tag_object(struct strbuf *buf, int sign,
+			struct char_array *signingkey, unsigned char *result)
 {
-	if (sign && do_sign(buf) < 0)
+	if (sign && do_sign(signingkey, buf) < 0)
 		return error("unable to sign the tag");
 	if (write_sha1_file(buf->buf, buf->len, tag_type, result) < 0)
 		return error("unable to write tag file");
@@ -277,6 +283,7 @@ static int build_tag_object(struct strbuf *buf, int sign, unsigned char *result)
 
 static void create_tag(const unsigned char *object, const char *tag,
 		       struct strbuf *buf, int message, int sign,
+		       struct char_array *signingkey,
 		       unsigned char *prev, unsigned char *result)
 {
 	enum object_type type;
@@ -331,7 +338,7 @@ static void create_tag(const unsigned char *object, const char *tag,
 
 	strbuf_insert(buf, 0, header_buf, header_len);
 
-	if (build_tag_object(buf, sign, result) < 0) {
+	if (build_tag_object(buf, sign, signingkey, result) < 0) {
 		if (path)
 			fprintf(stderr, "The tag message has been left in %s\n",
 				path);
@@ -374,6 +381,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	const char *msgfile = NULL, *keyid = NULL;
 	struct msg_arg msg = { 0, STRBUF_INIT };
 	struct commit_list *with_commit = NULL;
+	char keyarr[1000] = {'\0'};
+	struct char_array signingkey = { keyarr, sizeof(keyarr) };
 	struct option options[] = {
 		OPT_BOOLEAN('l', NULL, &list, "list tag names"),
 		{ OPTION_INTEGER, 'n', NULL, &lines, NULL,
@@ -403,14 +412,14 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
-	git_config(git_tag_config, NULL);
+	git_config(git_tag_config, &signingkey);
 
 	argc = parse_options(argc, argv, options, git_tag_usage, 0);
 	msgfile = parse_options_fix_filename(prefix, msgfile);
 
 	if (keyid) {
 		sign = 1;
-		set_signingkey(keyid);
+		set_signingkey(&signingkey, keyid);
 	}
 	if (sign)
 		annotate = 1;
@@ -474,7 +483,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 
 	if (annotate)
 		create_tag(object, tag, &buf, msg.given || msgfile,
-			   sign, prev, object);
+			   sign, &signingkey, prev, object);
 
 	lock = lock_any_ref_for_update(ref, prev, 0);
 	if (!lock)
-- 
1.5.4.3

Re: [PATCH] builtin-tag.c: remove global variable to use the callback data of git-config.

[Junio C Hamano]

Carlos Rica <jasampler@gmail.com> writes:

> Signed-off-by: Carlos Rica <jasampler@gmail.com>
> ---
>
> Here I declare a struct to wrap the new local array along with its size.
> QUESTION: An alternative to this is strbuf, would it be preferable?

The command already uses strbuf for other purposes, so why not?

Re: [PATCH] builtin-tag.c: remove global variable to use the callback data of git-config.

[Carlos Rica]

On Sat, Mar 14, 2009 at 9:54 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Carlos Rica <jasampler@gmail.com> writes:
>
>> Signed-off-by: Carlos Rica <jasampler@gmail.com>
>> ---
>>
>> Here I declare a struct to wrap the new local array along with its size.
>> QUESTION: An alternative to this is strbuf, would it be preferable?
>
> The command already uses strbuf for other purposes, so why not?

strbuf is designed as an unlimited length buffer, and now the user
signing-key id (obtained from the config or as a command's argument)
is limited to the current static array size.

It is right to remove this limit? I haven't found something like
strlcpy for strbuf and I'm not sure if it would be a nice adition:

size_t strbuf_lcpy(struct strbuf *dest,
        const char *src,  size_t max);

[PATCH] builtin-tag.c: remove global variable to use the callback data of git-config.

[Carlos Rica]

Signed-off-by: Carlos Rica <jasampler@yahoo.es>
---

This way the data flow is much clearer.

Here I declare a struct to wrap the new local array along with its size.
An alternative to this is strbuf, would it be preferable?

 builtin-tag.c |   43 ++++++++++++++++++++++++++-----------------
 1 files changed, 26 insertions(+), 17 deletions(-)

diff --git a/builtin-tag.c b/builtin-tag.c
index 01e7374..2b2d728 100644
--- a/builtin-tag.c
+++ b/builtin-tag.c
@@ -21,8 +21,6 @@ static const char * const git_tag_usage[] = {
 	NULL
 };
 
-static char signingkey[1000];
-
 struct tag_filter {
 	const char *pattern;
 	int lines;
@@ -156,7 +154,12 @@ static int verify_tag(const char *name, const char
*ref,
 	return 0;
 }
 
-static int do_sign(struct strbuf *buffer)
+struct char_array {
+	char *buf;
+	size_t size;
+};
+
+static int do_sign(struct char_array *signingkey, struct strbuf
*buffer)
 {
 	struct child_process gpg;
 	const char *args[4];
@@ -164,11 +167,12 @@ static int do_sign(struct strbuf *buffer)
 	int len;
 	int i, j;
 
-	if (!*signingkey) {
-		if (strlcpy(signingkey, git_committer_info(IDENT_ERROR_ON_NO_NAME),
-				sizeof(signingkey)) > sizeof(signingkey) - 1)
+	if (!signingkey->buf[0]) {
+		if (strlcpy(signingkey->buf,
+				git_committer_info(IDENT_ERROR_ON_NO_NAME),
+				signingkey->size) > signingkey->size - 1)
 			return error("committer info too long.");
-		bracket = strchr(signingkey, '>');
+		bracket = strchr(signingkey->buf, '>');
 		if (bracket)
 			bracket[1] = '\0';
 	}
@@ -183,7 +187,7 @@ static int do_sign(struct strbuf *buffer)
 	gpg.out = -1;
 	args[0] = "gpg";
 	args[1] = "-bsau";
-	args[2] = signingkey;
+	args[2] = signingkey->buf;
 	args[3] = NULL;
 
 	if (start_command(&gpg))
@@ -220,9 +224,10 @@ static const char tag_template[] =
 	"# Write a tag message\n"
 	"#\n";
 
-static void set_signingkey(const char *value)
+static void set_signingkey(struct char_array *signingkey, const char
*value)
 {
-	if (strlcpy(signingkey, value, sizeof(signingkey)) >=
sizeof(signingkey))
+	if (strlcpy(signingkey->buf, value, signingkey->size)
+					>= signingkey->size)
 		die("signing key value too long (%.10s...)", value);
 }
 
@@ -231,7 +236,7 @@ static int git_tag_config(const char *var, const
char *value, void *cb)
 	if (!strcmp(var, "user.signingkey")) {
 		if (!value)
 			return config_error_nonbool(var);
-		set_signingkey(value);
+		set_signingkey((struct char_array *) cb, value);
 		return 0;
 	}
 
@@ -266,9 +271,10 @@ static void write_tag_body(int fd, const unsigned
char *sha1)
 	free(buf);
 }
 
-static int build_tag_object(struct strbuf *buf, int sign, unsigned char
*result)
+static int build_tag_object(struct strbuf *buf, int sign,
+			struct char_array *signingkey, unsigned char *result)
 {
-	if (sign && do_sign(buf) < 0)
+	if (sign && do_sign(signingkey, buf) < 0)
 		return error("unable to sign the tag");
 	if (write_sha1_file(buf->buf, buf->len, tag_type, result) < 0)
 		return error("unable to write tag file");
@@ -277,6 +283,7 @@ static int build_tag_object(struct strbuf *buf, int
sign, unsigned char *result)
 
 static void create_tag(const unsigned char *object, const char *tag,
 		       struct strbuf *buf, int message, int sign,
+		       struct char_array *signingkey,
 		       unsigned char *prev, unsigned char *result)
 {
 	enum object_type type;
@@ -331,7 +338,7 @@ static void create_tag(const unsigned char *object,
const char *tag,
 
 	strbuf_insert(buf, 0, header_buf, header_len);
 
-	if (build_tag_object(buf, sign, result) < 0) {
+	if (build_tag_object(buf, sign, signingkey, result) < 0) {
 		if (path)
 			fprintf(stderr, "The tag message has been left in %s\n",
 				path);
@@ -374,6 +381,8 @@ int cmd_tag(int argc, const char **argv, const char
*prefix)
 	const char *msgfile = NULL, *keyid = NULL;
 	struct msg_arg msg = { 0, STRBUF_INIT };
 	struct commit_list *with_commit = NULL;
+	char keyarr[1000] = {'\0'};
+	struct char_array signingkey = { keyarr, sizeof(keyarr) };
 	struct option options[] = {
 		OPT_BOOLEAN('l', NULL, &list, "list tag names"),
 		{ OPTION_INTEGER, 'n', NULL, &lines, NULL,
@@ -403,14 +412,14 @@ int cmd_tag(int argc, const char **argv, const
char *prefix)
 		OPT_END()
 	};
 
-	git_config(git_tag_config, NULL);
+	git_config(git_tag_config, &signingkey);
 
 	argc = parse_options(argc, argv, options, git_tag_usage, 0);
 	msgfile = parse_options_fix_filename(prefix, msgfile);
 
 	if (keyid) {
 		sign = 1;
-		set_signingkey(keyid);
+		set_signingkey(&signingkey, keyid);
 	}
 	if (sign)
 		annotate = 1;
@@ -474,7 +483,7 @@ int cmd_tag(int argc, const char **argv, const char
*prefix)
 
 	if (annotate)
 		create_tag(object, tag, &buf, msg.given || msgfile,
-			   sign, prev, object);
+			   sign, &signingkey, prev, object);
 
 	lock = lock_any_ref_for_update(ref, prev, 0);
 	if (!lock)
-- 
1.5.4.3

Re: [PATCH] builtin-tag.c: remove global variable to use the callback data of git-config.

[Johannes Schindelin]

Hi,

On Tue, 10 Mar 2009, Carlos Rica wrote:

> 
> Signed-off-by: Carlos Rica <jasampler@yahoo.es>

Good to see you again!

BTW do you want to be recorded with a different email address in the 
author line than in the S-O-B?

> -static int do_sign(struct strbuf *buffer)
> +struct char_array {
> +	char *buf;
> +	size_t size;
> +};

That looks very much like you want a struct strbuf, no?

Ciao,
Dscho

Re: [PATCH] builtin-tag.c: remove global variable to use the callback data of git-config.

[Carlos Rica]

Please, don't even try to apply this patch, since long lines are wrapped.

I will send it fixed, sorry by the inconvenience.

On Tue, Mar 10, 2009 at 2:34 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Tue, 10 Mar 2009, Carlos Rica wrote:
>
>>
>> Signed-off-by: Carlos Rica <jasampler@yahoo.es>
>
> Good to see you again!

Thanks!
>
> BTW do you want to be recorded with a different email address in the
> author line than in the S-O-B?

Thank you for notify it, Johannes! That's was another mistake here,
I wanted to use the gmail account for this, I will change it when I
send this fixed.

>> -static int do_sign(struct strbuf *buffer)
>> +struct char_array {
>> +     char *buf;
>> +     size_t size;
>> +};
>
> That looks very much like you want a struct strbuf, no?

I was asking exactly that, in the hidden lines of the patch,
so if everybody prefers the strbuf solution I will use it, and then,
we should choose if there must be a limit for the signing key id max length
(now 1000) since by using dynamic memory it would not be required.

> Ciao,
> Dscho
>
>

Re: [PATCH] builtin-tag.c: remove global variable to use the callback data of git-config.

[Junio C Hamano]

Carlos Rica <jasampler@gmail.com> writes:

> Signed-off-by: Carlos Rica <jasampler@yahoo.es>
> ---
>
> This way the data flow is much clearer.

Good.  I think the Subject is backwards, though.