[PATCH 1/5] mktag.c: adjust verify_tag parameters

[PATCH 1/5] mktag.c: adjust verify_tag parameters

[drafnel]

From: Brandon Casey <casey@nrlssc.navy.mil>

The size parameter should be a size_t since it is a string length.

There is no reason that the buffer argument should not be constant except
for the nul termination that is performed in the first few lines of this
function. This is not necessary, since a valid c string must always be
nul terminated and we can check whether we have exceeded the caller's
size parameter at the end of parsing the buffer.

Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---
 mktag.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/mktag.c b/mktag.c
index 0b34341..5489ad6 100644
--- a/mktag.c
+++ b/mktag.c
@@ -39,7 +39,7 @@ static int verify_object(unsigned char *sha1, const char *expected_type)
 #define PD_FMT "%td"
 #endif
 
-static int verify_tag(char *buffer, unsigned long size)
+static int verify_tag(const char *buffer, size_t size)
 {
 	int typelen;
 	char type[20];
@@ -50,8 +50,6 @@ static int verify_tag(char *buffer, unsigned long size)
 	if (size < 84)
 		return error("wanna fool me ? you obviously got the size wrong !");
 
-	buffer[size] = 0;
-
 	/* Verify object line */
 	object = buffer;
 	if (memcmp(object, "object ", 7))
@@ -145,6 +143,13 @@ static int verify_tag(char *buffer, unsigned long size)
 		return error("char" PD_FMT ": trailing garbage in tag header",
 			tagger_line - buffer);
 
+	/*
+	 * Make sure we haven't advanced past what the caller said the
+	 * buffer size was.
+	 */
+	if (tagger_line - buffer >= size)
+		return error("char" PD_FMT ": tag truncated", size);
+
 	/* The actual stuff afterwards we don't care about.. */
 	return 0;
 }
-- 
1.5.5.67.g9a49

Re: [PATCH 1/5] mktag.c: adjust verify_tag parameters

[Junio C Hamano]

drafnel@gmail.com writes:

> From: Brandon Casey <casey@nrlssc.navy.mil>
>
> The size parameter should be a size_t since it is a string length.

Correct.

> There is no reason that the buffer argument should not be constant except
> for the nul termination that is performed in the first few lines of this
> function. This is not necessary, since a valid c string must always be
> nul terminated and we can check whether we have exceeded the caller's
> size parameter at the end of parsing the buffer.

Wait a minute.  The point of passing a stringlet as a tuple of <pointer to
the beginning, length> is that you may not have a valid C string to begin
with, isn't it?  We shouldn't be assuming that reading past the given size
is Ok --- you may not be lucky enough to have any NUL byte after the given
string before you hit the page boundary and encounter unmapped page.

The generic-looking argument you made is bogus, but for this particular
code it is true, as the parameter you are passing to the function is
prepared by strbuf_read() which gives you a NUL terminated buffer.

So the code is correct -- justification is not.

> Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
> ---
>  mktag.c |   11 ++++++++---
>  1 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/mktag.c b/mktag.c
> index 0b34341..5489ad6 100644
> --- a/mktag.c
> +++ b/mktag.c
> @@ -39,7 +39,7 @@ static int verify_object(unsigned char *sha1, const char *expected_type)
>  #define PD_FMT "%td"
>  #endif
>  
> -static int verify_tag(char *buffer, unsigned long size)
> +static int verify_tag(const char *buffer, size_t size)
>  {
>  	int typelen;
>  	char type[20];
> @@ -50,8 +50,6 @@ static int verify_tag(char *buffer, unsigned long size)
>  	if (size < 84)
>  		return error("wanna fool me ? you obviously got the size wrong !");
>  
> -	buffer[size] = 0;
> -
>  	/* Verify object line */
>  	object = buffer;
>  	if (memcmp(object, "object ", 7))
> @@ -145,6 +143,13 @@ static int verify_tag(char *buffer, unsigned long size)
>  		return error("char" PD_FMT ": trailing garbage in tag header",
>  			tagger_line - buffer);
>  
> +	/*
> +	 * Make sure we haven't advanced past what the caller said the
> +	 * buffer size was.
> +	 */
> +	if (tagger_line - buffer >= size)
> +		return error("char" PD_FMT ": tag truncated", size);
> +
>  	/* The actual stuff afterwards we don't care about.. */
>  	return 0;
>  }
> -- 
> 1.5.5.67.g9a49

Re: [PATCH 1/5] mktag.c: adjust verify_tag parameters

[Brandon Casey]

Junio C Hamano wrote:
> drafnel@gmail.com writes:
> 
>> From: Brandon Casey <casey@nrlssc.navy.mil>
>>
>> The size parameter should be a size_t since it is a string length.
> 
> Correct.
> 
>> There is no reason that the buffer argument should not be constant except
>> for the nul termination that is performed in the first few lines of this
>> function. This is not necessary, since a valid c string must always be
>> nul terminated and we can check whether we have exceeded the caller's
>> size parameter at the end of parsing the buffer.
> 
> Wait a minute.  The point of passing a stringlet as a tuple of <pointer to
> the beginning, length> is that you may not have a valid C string to begin
> with, isn't it?

I thought it was just to limit the scope of parsing.

Other functions that deal with non-nul-terminated memory segments (like memchr)
do not use char*, they instead use void*.

If we want to handle non-nul-terminated memory segments here, we should
probably allocate a new string to copy the memory segment into so we can
ensure it is nul-terminated. Otherwise we can't use any of the str*()
functions safely.

-brandon

[PATCH 2/5] Make mktag a builtin.

[drafnel]

From: Brandon Casey <drafnel@gmail.com>

Signed-off-by: Brandon Casey <drafnel@gmail.com>
---
 Makefile  |    3 ++-
 builtin.h |    1 +
 git.c     |    1 +
 mktag.c   |    4 +---
 4 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/Makefile b/Makefile
index 9d84c8d..1128cef 100644
--- a/Makefile
+++ b/Makefile
@@ -282,7 +282,6 @@ PROGRAMS += git-imap-send$X
 PROGRAMS += git-index-pack$X
 PROGRAMS += git-merge-index$X
 PROGRAMS += git-merge-tree$X
-PROGRAMS += git-mktag$X
 PROGRAMS += git-mktree$X
 PROGRAMS += git-pack-redundant$X
 PROGRAMS += git-patch-id$X
@@ -306,6 +305,7 @@ BUILT_INS += git-fsck-objects$X
 BUILT_INS += git-get-tar-commit-id$X
 BUILT_INS += git-init$X
 BUILT_INS += git-merge-subtree$X
+BUILT_INS += git-mktag$X
 BUILT_INS += git-peek-remote$X
 BUILT_INS += git-repo-config$X
 BUILT_INS += git-show$X
@@ -423,6 +423,7 @@ LIB_OBJS += log-tree.o
 LIB_OBJS += mailmap.o
 LIB_OBJS += match-trees.o
 LIB_OBJS += merge-file.o
+LIB_OBJS += mktag.o
 LIB_OBJS += object.o
 LIB_OBJS += pack-check.o
 LIB_OBJS += pack-revindex.o
diff --git a/builtin.h b/builtin.h
index 95126fd..a8d3a11 100644
--- a/builtin.h
+++ b/builtin.h
@@ -58,6 +58,7 @@ extern int cmd_merge_base(int argc, const char **argv, const char *prefix);
 extern int cmd_merge_ours(int argc, const char **argv, const char *prefix);
 extern int cmd_merge_file(int argc, const char **argv, const char *prefix);
 extern int cmd_merge_recursive(int argc, const char **argv, const char *prefix);
+extern int cmd_mktag(int argc, const char **argv, const char *prefix);
 extern int cmd_mv(int argc, const char **argv, const char *prefix);
 extern int cmd_name_rev(int argc, const char **argv, const char *prefix);
 extern int cmd_pack_objects(int argc, const char **argv, const char *prefix);
diff --git a/git.c b/git.c
index 89b431f..81aa31c 100644
--- a/git.c
+++ b/git.c
@@ -325,6 +325,7 @@ static void handle_internal_command(int argc, const char **argv)
 		{ "merge-ours", cmd_merge_ours, RUN_SETUP },
 		{ "merge-recursive", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE },
 		{ "merge-subtree", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE },
+		{ "mktag", cmd_mktag, RUN_SETUP },
 		{ "mv", cmd_mv, RUN_SETUP | NEED_WORK_TREE },
 		{ "name-rev", cmd_name_rev, RUN_SETUP },
 		{ "pack-objects", cmd_pack_objects, RUN_SETUP },
diff --git a/mktag.c b/mktag.c
index 5489ad6..352747b 100644
--- a/mktag.c
+++ b/mktag.c
@@ -156,7 +156,7 @@ static int verify_tag(const char *buffer, size_t size)
 
 #undef PD_FMT
 
-int main(int argc, char **argv)
+int cmd_mktag(int argc, const char **argv, const char *prefix)
 {
 	struct strbuf buf;
 	unsigned char result_sha1[20];
@@ -164,8 +164,6 @@ int main(int argc, char **argv)
 	if (argc != 1)
 		usage("git-mktag < signaturefile");
 
-	setup_git_directory();
-
 	strbuf_init(&buf, 0);
 	if (strbuf_read(&buf, 0, 4096) < 0) {
 		die("could not read from stdin");
-- 
1.5.5.67.g9a49

Re: [PATCH 2/5] Make mktag a builtin.

[Junio C Hamano]

drafnel@gmail.com writes:

> From: Brandon Casey <drafnel@gmail.com>
>
> Signed-off-by: Brandon Casey <drafnel@gmail.com>

> @@ -306,6 +305,7 @@ BUILT_INS += git-fsck-objects$X
>  BUILT_INS += git-get-tar-commit-id$X
>  BUILT_INS += git-init$X
>  BUILT_INS += git-merge-subtree$X
> +BUILT_INS += git-mktag$X
>  BUILT_INS += git-peek-remote$X
>  BUILT_INS += git-repo-config$X
>  BUILT_INS += git-show$X
> @@ -423,6 +423,7 @@ LIB_OBJS += log-tree.o
>  LIB_OBJS += mailmap.o
>  LIB_OBJS += match-trees.o
>  LIB_OBJS += merge-file.o
> +LIB_OBJS += mktag.o

This is unusual for a builtin.  Why didn't it migrate to builtin-mktag?

Re: [PATCH 2/5] Make mktag a builtin.

[Junio C Hamano]

Junio C Hamano <junio@pobox.com> writes:

> drafnel@gmail.com writes:
>
>> From: Brandon Casey <drafnel@gmail.com>
>>
>> Signed-off-by: Brandon Casey <drafnel@gmail.com>
>
>> @@ -306,6 +305,7 @@ BUILT_INS += git-fsck-objects$X
>>  BUILT_INS += git-get-tar-commit-id$X
>>  BUILT_INS += git-init$X
>>  BUILT_INS += git-merge-subtree$X
>> +BUILT_INS += git-mktag$X
>>  BUILT_INS += git-peek-remote$X
>>  BUILT_INS += git-repo-config$X
>>  BUILT_INS += git-show$X
>> @@ -423,6 +423,7 @@ LIB_OBJS += log-tree.o
>>  LIB_OBJS += mailmap.o
>>  LIB_OBJS += match-trees.o
>>  LIB_OBJS += merge-file.o
>> +LIB_OBJS += mktag.o
>
> This is unusual for a builtin.  Why didn't it migrate to builtin-mktag?

That is, something along this line, instead.

-- >8 --
From: Brandon Casey <drafnel@gmail.com>
Date: Thu, 8 May 2008 21:19:46 -0500
Subject: [PATCH] Make mktag a builtin

Signed-off-by: Brandon Casey <drafnel@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Makefile                   |    2 +-
 mktag.c => builtin-mktag.c |    4 +---
 builtin.h                  |    1 +
 git.c                      |    1 +
 4 files changed, 4 insertions(+), 4 deletions(-)
 rename mktag.c => builtin-mktag.c (98%)

diff --git a/Makefile b/Makefile
index 9d84c8d..3980dc9 100644
--- a/Makefile
+++ b/Makefile
@@ -282,7 +282,6 @@ PROGRAMS += git-imap-send$X
 PROGRAMS += git-index-pack$X
 PROGRAMS += git-merge-index$X
 PROGRAMS += git-merge-tree$X
-PROGRAMS += git-mktag$X
 PROGRAMS += git-mktree$X
 PROGRAMS += git-pack-redundant$X
 PROGRAMS += git-patch-id$X
@@ -510,6 +509,7 @@ BUILTIN_OBJS += builtin-merge-base.o
 BUILTIN_OBJS += builtin-merge-file.o
 BUILTIN_OBJS += builtin-merge-ours.o
 BUILTIN_OBJS += builtin-merge-recursive.o
+BUILTIN_OBJS += builtin-mktag.o
 BUILTIN_OBJS += builtin-mv.o
 BUILTIN_OBJS += builtin-name-rev.o
 BUILTIN_OBJS += builtin-pack-objects.o
diff --git a/mktag.c b/builtin-mktag.c
similarity index 98%
rename from mktag.c
rename to builtin-mktag.c
index a45a5e7..1596900 100644
--- a/mktag.c
+++ b/builtin-mktag.c
@@ -149,7 +149,7 @@ static int verify_tag(const char *buffer, size_t size)
 
 #undef PD_FMT
 
-int main(int argc, char **argv)
+int cmd_mktag(int argc, const char **argv, const char *prefix)
 {
 	struct strbuf buf;
 	unsigned char result_sha1[20];
@@ -157,8 +157,6 @@ int main(int argc, char **argv)
 	if (argc != 1)
 		usage("git-mktag < signaturefile");
 
-	setup_git_directory();
-
 	strbuf_init(&buf, 0);
 	if (strbuf_read(&buf, 0, 4096) < 0) {
 		die("could not read from stdin");
diff --git a/builtin.h b/builtin.h
index 95126fd..a8d3a11 100644
--- a/builtin.h
+++ b/builtin.h
@@ -58,6 +58,7 @@ extern int cmd_merge_base(int argc, const char **argv, const char *prefix);
 extern int cmd_merge_ours(int argc, const char **argv, const char *prefix);
 extern int cmd_merge_file(int argc, const char **argv, const char *prefix);
 extern int cmd_merge_recursive(int argc, const char **argv, const char *prefix);
+extern int cmd_mktag(int argc, const char **argv, const char *prefix);
 extern int cmd_mv(int argc, const char **argv, const char *prefix);
 extern int cmd_name_rev(int argc, const char **argv, const char *prefix);
 extern int cmd_pack_objects(int argc, const char **argv, const char *prefix);
diff --git a/git.c b/git.c
index 89b431f..81aa31c 100644
--- a/git.c
+++ b/git.c
@@ -325,6 +325,7 @@ static void handle_internal_command(int argc, const char **argv)
 		{ "merge-ours", cmd_merge_ours, RUN_SETUP },
 		{ "merge-recursive", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE },
 		{ "merge-subtree", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE },
+		{ "mktag", cmd_mktag, RUN_SETUP },
 		{ "mv", cmd_mv, RUN_SETUP | NEED_WORK_TREE },
 		{ "name-rev", cmd_name_rev, RUN_SETUP },
 		{ "pack-objects", cmd_pack_objects, RUN_SETUP },
-- 
1.5.5.1.295.g4c42f

Re: [PATCH 2/5] Make mktag a builtin.

[Brandon Casey]

Junio C Hamano wrote:
> drafnel@gmail.com writes:
> 
>> From: Brandon Casey <drafnel@gmail.com>
>>
>> Signed-off-by: Brandon Casey <drafnel@gmail.com>
> 
>> @@ -306,6 +305,7 @@ BUILT_INS += git-fsck-objects$X
>>  BUILT_INS += git-get-tar-commit-id$X
>>  BUILT_INS += git-init$X
>>  BUILT_INS += git-merge-subtree$X
>> +BUILT_INS += git-mktag$X
>>  BUILT_INS += git-peek-remote$X
>>  BUILT_INS += git-repo-config$X
>>  BUILT_INS += git-show$X
>> @@ -423,6 +423,7 @@ LIB_OBJS += log-tree.o
>>  LIB_OBJS += mailmap.o
>>  LIB_OBJS += match-trees.o
>>  LIB_OBJS += merge-file.o
>> +LIB_OBJS += mktag.o
> 
> This is unusual for a builtin.  Why didn't it migrate to builtin-mktag?

I didn't know how to do it.

I was trying not to do a code move and a code change at the same time.
I didn't think I should move the non-builtin mktag.c to builtin-mktag.c,
and then after I modified mktag to be a builtin I knew I was moving it
to builtin-tag.c so I didn't see a point to renaming it.

Also, I decided about those things _before_ I realized how small the changes
would be to mktag to make it a builtin.

Do you think the modified patch you posted conflicts with the idea that
"code move should be separate from code change"?

-brandon

Re: [PATCH 2/5] Make mktag a builtin.

[Johannes Schindelin]

Hi,

On Mon, 12 May 2008, Brandon Casey wrote:

> Junio C Hamano wrote:
> > drafnel@gmail.com writes:
> > 
> >> From: Brandon Casey <drafnel@gmail.com>
> >>
> >> Signed-off-by: Brandon Casey <drafnel@gmail.com>
> > 
> >> @@ -306,6 +305,7 @@ BUILT_INS += git-fsck-objects$X
> >>  BUILT_INS += git-get-tar-commit-id$X
> >>  BUILT_INS += git-init$X
> >>  BUILT_INS += git-merge-subtree$X
> >> +BUILT_INS += git-mktag$X
> >>  BUILT_INS += git-peek-remote$X
> >>  BUILT_INS += git-repo-config$X
> >>  BUILT_INS += git-show$X
> >> @@ -423,6 +423,7 @@ LIB_OBJS += log-tree.o
> >>  LIB_OBJS += mailmap.o
> >>  LIB_OBJS += match-trees.o
> >>  LIB_OBJS += merge-file.o
> >> +LIB_OBJS += mktag.o
> > 
> > This is unusual for a builtin.  Why didn't it migrate to builtin-mktag?
> 
> I didn't know how to do it.
> 
> I was trying not to do a code move and a code change at the same time.

Why did you not consult Git's own history for guidance?  See e.g.

	$ git log next --diff-filter=A builtin-*.c

Ciao,
Dscho

Re: [PATCH 2/5] Make mktag a builtin.

[Brandon Casey]

Johannes Schindelin wrote:
> Hi,
> 
> On Mon, 12 May 2008, Brandon Casey wrote:
> 
>> Junio C Hamano wrote:
>>> drafnel@gmail.com writes:
>>>
>>>> From: Brandon Casey <drafnel@gmail.com>
>>>>
>>>> Signed-off-by: Brandon Casey <drafnel@gmail.com>
>>>> @@ -306,6 +305,7 @@ BUILT_INS += git-fsck-objects$X
>>>>  BUILT_INS += git-get-tar-commit-id$X
>>>>  BUILT_INS += git-init$X
>>>>  BUILT_INS += git-merge-subtree$X
>>>> +BUILT_INS += git-mktag$X
>>>>  BUILT_INS += git-peek-remote$X
>>>>  BUILT_INS += git-repo-config$X
>>>>  BUILT_INS += git-show$X
>>>> @@ -423,6 +423,7 @@ LIB_OBJS += log-tree.o
>>>>  LIB_OBJS += mailmap.o
>>>>  LIB_OBJS += match-trees.o
>>>>  LIB_OBJS += merge-file.o
>>>> +LIB_OBJS += mktag.o
>>> This is unusual for a builtin.  Why didn't it migrate to builtin-mktag?
>> I didn't know how to do it.
>>
>> I was trying not to do a code move and a code change at the same time.
> 
> Why did you not consult Git's own history for guidance?  See e.g.

I did, but not exhaustively. I found only examples of converting a
shell/perl script to a c version. That is straight-forward.

> 	$ git log next --diff-filter=A builtin-*.c

And indeed the first two results here are examples of converting scripts
to builtin c versions.

The third result is applicable and converts merge-recursive from a
standalone c version to builtin.

If I had known about (or looked for) the --diff-filter option I would
have used the 'R' argument, since I am interested in a rename event,
but it produces no results.

	$ git log next --diff-filter=R builtin-*.c

Adding -M does not help.

-brandon

Re: [PATCH 2/5] Make mktag a builtin.

[Junio C Hamano]

Brandon Casey <casey@nrlssc.navy.mil> writes:

> I didn't think I should move the non-builtin mktag.c to builtin-mktag.c,
> and then after I modified mktag to be a builtin I knew I was moving it
> to builtin-tag.c so I didn't see a point to renaming it.

I see.  I did not realize that the eventual shape would be to have both
mktag and tag to be in builtin-tag.c, just like builtin-log.c supports
many other commands from the log family, and that was where my question
came from.

But I think the arrangement to have both in builtin-tag.c actually makes
sense --- mktag needs to be kept supported but most of its internal should
be shared with tag anyway.

> Also, I decided about those things _before_ I realized how small the changes
> would be to mktag to make it a builtin.
>
> Do you think the modified patch you posted conflicts with the idea that
> "code move should be separate from code change"?

Yes, it does, but I do not subscribe to the "idea".  Therefor I do not see
any problem.

If you were to stop at making mktag a builtin, then the patch I sent would
be the change that is necessary to do so.  A code movement can and often
does need some adjustment (e.g. if you move "a.c" to "src/a.c", its
'#include "a.h"' may need to become '#include "../a.h"' (or preferably to
'#include <a.h>' with appropriate -I.. option in the Makefile).  It does
not help anybody to insist on a blanket dogma that forbids modification
and movement at the same time.

We do discourage rolling unrelated things in one commit, but creating a
builtin "foo" typically involves creation of builtin-foo.c and associated
changes to the Makefile and builtin.h, and in this case the initial
contents of builtin-mktag.c happens to come from an existing file mktag.c,
while making the original mktag.c obsolete and unnecessary along the way.
That is a single logical change, and I do not think there is anything
wrong to do it in one commit. In fact, splitting such a change into more
than one commit is just plain silly and wrong, isn't it?

[PATCH 3/5] mktag.c: rename verify_tag to verify_tag_buffer

[drafnel]

From: Brandon Casey <drafnel@gmail.com>

This is in preparation for merging mktag.c with builtin-tag.c and
using verify_tag_buffer within the git-tag code path.

Signed-off-by: Brandon Casey <drafnel@gmail.com>
---
 mktag.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/mktag.c b/mktag.c
index 352747b..6204001 100644
--- a/mktag.c
+++ b/mktag.c
@@ -39,7 +39,7 @@ static int verify_object(unsigned char *sha1, const char *expected_type)
 #define PD_FMT "%td"
 #endif
 
-static int verify_tag(const char *buffer, size_t size)
+static int verify_tag_buffer(const char *buffer, size_t size)
 {
 	int typelen;
 	char type[20];
@@ -171,7 +171,7 @@ int cmd_mktag(int argc, const char **argv, const char *prefix)
 
 	/* Verify it for some basic sanity: it needs to start with
 	   "object <sha1>\ntype\ntagger " */
-	if (verify_tag(buf.buf, buf.len) < 0)
+	if (verify_tag_buffer(buf.buf, buf.len) < 0)
 		die("invalid tag signature file");
 
 	if (write_sha1_file(buf.buf, buf.len, tag_type, result_sha1) < 0)
-- 
1.5.5.67.g9a49

[PATCH 4/5] mktag.c: consolidate tag functions by merging mktag.c into builtin-tag.c

[drafnel]

From: Brandon Casey <drafnel@gmail.com>

Signed-off-by: Brandon Casey <drafnel@gmail.com>
---
 Makefile      |    1 -
 builtin-tag.c |  181 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 mktag.c       |  183 ---------------------------------------------------------
 3 files changed, 181 insertions(+), 184 deletions(-)
 delete mode 100644 mktag.c

diff --git a/Makefile b/Makefile
index 1128cef..572459f 100644
--- a/Makefile
+++ b/Makefile
@@ -423,7 +423,6 @@ LIB_OBJS += log-tree.o
 LIB_OBJS += mailmap.o
 LIB_OBJS += match-trees.o
 LIB_OBJS += merge-file.o
-LIB_OBJS += mktag.o
 LIB_OBJS += object.o
 LIB_OBJS += pack-check.o
 LIB_OBJS += pack-revindex.o
diff --git a/builtin-tag.c b/builtin-tag.c
index 129ff57..85ec2f3 100644
--- a/builtin-tag.c
+++ b/builtin-tag.c
@@ -296,6 +296,161 @@ static void write_tag_body(int fd, const unsigned char *sha1)
 	free(buf);
 }
 
+/*
+ * We refuse to tag something we can't verify. Just because.
+ */
+static int verify_object(unsigned char *sha1, const char *expected_type)
+{
+	int ret = -1;
+	enum object_type type;
+	unsigned long size;
+	void *buffer = read_sha1_file(sha1, &type, &size);
+
+	if (buffer) {
+		if (type == type_from_string(expected_type))
+			ret = check_sha1_signature(sha1, buffer, size, expected_type);
+		free(buffer);
+	}
+	return ret;
+}
+
+#ifdef NO_C99_FORMAT
+#define PD_FMT "%d"
+#else
+#define PD_FMT "%td"
+#endif
+
+/*
+ * A signature file has a very simple fixed format: four lines
+ * of "object <sha1>" + "type <typename>" + "tag <tagname>" +
+ * "tagger <committer>", followed by a blank line, a free-form tag
+ * message and a signature block that git itself doesn't care about,
+ * but that can be verified with gpg or similar.
+ *
+ * The first four lines are guaranteed to be at least 83 bytes:
+ * "object <sha1>\n" is 48 bytes, "type tag\n" at 9 bytes is the
+ * shortest possible type-line, "tag .\n" at 6 bytes is the shortest
+ * single-character-tag line, and "tagger . <> 0 +0000\n" at 20 bytes is
+ * the shortest possible tagger-line.
+ */
+
+static int verify_tag_buffer(const char *buffer, size_t size)
+{
+	int typelen;
+	char type[20];
+	unsigned char sha1[20];
+	const char *object, *type_line, *tag_line, *tagger_line, *lb, *rb;
+	size_t len;
+
+	if (size < 84)
+		return error("wanna fool me ? you obviously got the size wrong !");
+
+	/* Verify object line */
+	object = buffer;
+	if (memcmp(object, "object ", 7))
+		return error("char%d: does not start with \"object \"", 0);
+
+	if (get_sha1_hex(object + 7, sha1))
+		return error("char%d: could not get SHA1 hash", 7);
+
+	/* Verify type line */
+	type_line = object + 48;
+	if (memcmp(type_line - 1, "\ntype ", 6))
+		return error("char%d: could not find \"\\ntype \"", 47);
+
+	/* Verify tag-line */
+	tag_line = strchr(type_line, '\n');
+	if (!tag_line)
+		return error("char" PD_FMT ": could not find next \"\\n\"", type_line - buffer);
+	tag_line++;
+	if (memcmp(tag_line, "tag ", 4) || tag_line[4] == '\n')
+		return error("char" PD_FMT ": no \"tag \" found", tag_line - buffer);
+
+	/* Get the actual type */
+	typelen = tag_line - type_line - strlen("type \n");
+	if (typelen >= sizeof(type))
+		return error("char" PD_FMT ": type too long", type_line+5 - buffer);
+
+	memcpy(type, type_line+5, typelen);
+	type[typelen] = 0;
+
+	/* Verify that the object matches */
+	if (verify_object(sha1, type))
+		return error("char%d: could not verify object %s", 7, sha1_to_hex(sha1));
+
+	/* Verify the tag-name: we don't allow control characters or spaces in it */
+	tag_line += 4;
+	for (;;) {
+		unsigned char c = *tag_line++;
+		if (c == '\n')
+			break;
+		if (c > ' ')
+			continue;
+		return error("char" PD_FMT ": could not verify tag name", tag_line - buffer);
+	}
+
+	/* Verify the tagger line */
+	tagger_line = tag_line;
+
+	if (memcmp(tagger_line, "tagger ", 7))
+		return error("char" PD_FMT ": could not find \"tagger \"",
+			tagger_line - buffer);
+
+	/*
+	 * Check for correct form for name and email
+	 * i.e. " <" followed by "> " on _this_ line
+	 * No angle brackets within the name or email address fields.
+	 * No spaces within the email address field.
+	 */
+	tagger_line += 7;
+	if (!(lb = strstr(tagger_line, " <")) || !(rb = strstr(lb+2, "> ")) ||
+		strpbrk(tagger_line, "<>\n") != lb+1 ||
+		strpbrk(lb+2, "><\n ") != rb)
+		return error("char" PD_FMT ": malformed tagger field",
+			tagger_line - buffer);
+
+	/* Check for author name, at least one character, space is acceptable */
+	if (lb == tagger_line)
+		return error("char" PD_FMT ": missing tagger name",
+			tagger_line - buffer);
+
+	/* timestamp, 1 or more digits followed by space */
+	tagger_line = rb + 2;
+	if (!(len = strspn(tagger_line, "0123456789")))
+		return error("char" PD_FMT ": missing tag timestamp",
+			tagger_line - buffer);
+	tagger_line += len;
+	if (*tagger_line != ' ')
+		return error("char" PD_FMT ": malformed tag timestamp",
+			tagger_line - buffer);
+	tagger_line++;
+
+	/* timezone, 5 digits [+-]hhmm, max. 1400 */
+	if (!((tagger_line[0] == '+' || tagger_line[0] == '-') &&
+	      strspn(tagger_line+1, "0123456789") == 4 &&
+	      tagger_line[5] == '\n' && atoi(tagger_line+1) <= 1400))
+		return error("char" PD_FMT ": malformed tag timezone",
+			tagger_line - buffer);
+	tagger_line += 6;
+
+	/* Verify the blank line separating the header from the body */
+	if (*tagger_line != '\n')
+		return error("char" PD_FMT ": trailing garbage in tag header",
+			tagger_line - buffer);
+
+	/*
+	 * Make sure we haven't advanced past what the caller said the
+	 * buffer size was.
+	 */
+	if (tagger_line - buffer >= size)
+		return error("char" PD_FMT ": tag truncated", size);
+
+	/* The actual stuff afterwards we don't care about.. */
+	return 0;
+}
+
+#undef PD_FMT
+
 static void create_tag(const unsigned char *object, const char *tag,
 		       struct strbuf *buf, int message, int sign,
 		       unsigned char *prev, unsigned char *result)
@@ -482,3 +637,29 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	strbuf_release(&buf);
 	return 0;
 }
+
+int cmd_mktag(int argc, const char **argv, const char *prefix)
+{
+	struct strbuf buf;
+	unsigned char result_sha1[20];
+
+	if (argc != 1)
+		usage("git-mktag < signaturefile");
+
+	strbuf_init(&buf, 0);
+	if (strbuf_read(&buf, 0, 4096) < 0) {
+		die("could not read from stdin");
+	}
+
+	/* Verify it for some basic sanity: it needs to start with
+	   "object <sha1>\ntype\ntagger " */
+	if (verify_tag_buffer(buf.buf, buf.len) < 0)
+		die("invalid tag signature file");
+
+	if (write_sha1_file(buf.buf, buf.len, tag_type, result_sha1) < 0)
+		die("unable to write tag file");
+
+	strbuf_release(&buf);
+	printf("%s\n", sha1_to_hex(result_sha1));
+	return 0;
+}
diff --git a/mktag.c b/mktag.c
deleted file mode 100644
index 6204001..0000000
--- a/mktag.c
+++ /dev/null
@@ -1,183 +0,0 @@
-#include "cache.h"
-#include "tag.h"
-
-/*
- * A signature file has a very simple fixed format: four lines
- * of "object <sha1>" + "type <typename>" + "tag <tagname>" +
- * "tagger <committer>", followed by a blank line, a free-form tag
- * message and a signature block that git itself doesn't care about,
- * but that can be verified with gpg or similar.
- *
- * The first four lines are guaranteed to be at least 83 bytes:
- * "object <sha1>\n" is 48 bytes, "type tag\n" at 9 bytes is the
- * shortest possible type-line, "tag .\n" at 6 bytes is the shortest
- * single-character-tag line, and "tagger . <> 0 +0000\n" at 20 bytes is
- * the shortest possible tagger-line.
- */
-
-/*
- * We refuse to tag something we can't verify. Just because.
- */
-static int verify_object(unsigned char *sha1, const char *expected_type)
-{
-	int ret = -1;
-	enum object_type type;
-	unsigned long size;
-	void *buffer = read_sha1_file(sha1, &type, &size);
-
-	if (buffer) {
-		if (type == type_from_string(expected_type))
-			ret = check_sha1_signature(sha1, buffer, size, expected_type);
-		free(buffer);
-	}
-	return ret;
-}
-
-#ifdef NO_C99_FORMAT
-#define PD_FMT "%d"
-#else
-#define PD_FMT "%td"
-#endif
-
-static int verify_tag_buffer(const char *buffer, size_t size)
-{
-	int typelen;
-	char type[20];
-	unsigned char sha1[20];
-	const char *object, *type_line, *tag_line, *tagger_line, *lb, *rb;
-	size_t len;
-
-	if (size < 84)
-		return error("wanna fool me ? you obviously got the size wrong !");
-
-	/* Verify object line */
-	object = buffer;
-	if (memcmp(object, "object ", 7))
-		return error("char%d: does not start with \"object \"", 0);
-
-	if (get_sha1_hex(object + 7, sha1))
-		return error("char%d: could not get SHA1 hash", 7);
-
-	/* Verify type line */
-	type_line = object + 48;
-	if (memcmp(type_line - 1, "\ntype ", 6))
-		return error("char%d: could not find \"\\ntype \"", 47);
-
-	/* Verify tag-line */
-	tag_line = strchr(type_line, '\n');
-	if (!tag_line)
-		return error("char" PD_FMT ": could not find next \"\\n\"", type_line - buffer);
-	tag_line++;
-	if (memcmp(tag_line, "tag ", 4) || tag_line[4] == '\n')
-		return error("char" PD_FMT ": no \"tag \" found", tag_line - buffer);
-
-	/* Get the actual type */
-	typelen = tag_line - type_line - strlen("type \n");
-	if (typelen >= sizeof(type))
-		return error("char" PD_FMT ": type too long", type_line+5 - buffer);
-
-	memcpy(type, type_line+5, typelen);
-	type[typelen] = 0;
-
-	/* Verify that the object matches */
-	if (verify_object(sha1, type))
-		return error("char%d: could not verify object %s", 7, sha1_to_hex(sha1));
-
-	/* Verify the tag-name: we don't allow control characters or spaces in it */
-	tag_line += 4;
-	for (;;) {
-		unsigned char c = *tag_line++;
-		if (c == '\n')
-			break;
-		if (c > ' ')
-			continue;
-		return error("char" PD_FMT ": could not verify tag name", tag_line - buffer);
-	}
-
-	/* Verify the tagger line */
-	tagger_line = tag_line;
-
-	if (memcmp(tagger_line, "tagger ", 7))
-		return error("char" PD_FMT ": could not find \"tagger \"",
-			tagger_line - buffer);
-
-	/*
-	 * Check for correct form for name and email
-	 * i.e. " <" followed by "> " on _this_ line
-	 * No angle brackets within the name or email address fields.
-	 * No spaces within the email address field.
-	 */
-	tagger_line += 7;
-	if (!(lb = strstr(tagger_line, " <")) || !(rb = strstr(lb+2, "> ")) ||
-		strpbrk(tagger_line, "<>\n") != lb+1 ||
-		strpbrk(lb+2, "><\n ") != rb)
-		return error("char" PD_FMT ": malformed tagger field",
-			tagger_line - buffer);
-
-	/* Check for author name, at least one character, space is acceptable */
-	if (lb == tagger_line)
-		return error("char" PD_FMT ": missing tagger name",
-			tagger_line - buffer);
-
-	/* timestamp, 1 or more digits followed by space */
-	tagger_line = rb + 2;
-	if (!(len = strspn(tagger_line, "0123456789")))
-		return error("char" PD_FMT ": missing tag timestamp",
-			tagger_line - buffer);
-	tagger_line += len;
-	if (*tagger_line != ' ')
-		return error("char" PD_FMT ": malformed tag timestamp",
-			tagger_line - buffer);
-	tagger_line++;
-
-	/* timezone, 5 digits [+-]hhmm, max. 1400 */
-	if (!((tagger_line[0] == '+' || tagger_line[0] == '-') &&
-	      strspn(tagger_line+1, "0123456789") == 4 &&
-	      tagger_line[5] == '\n' && atoi(tagger_line+1) <= 1400))
-		return error("char" PD_FMT ": malformed tag timezone",
-			tagger_line - buffer);
-	tagger_line += 6;
-
-	/* Verify the blank line separating the header from the body */
-	if (*tagger_line != '\n')
-		return error("char" PD_FMT ": trailing garbage in tag header",
-			tagger_line - buffer);
-
-	/*
-	 * Make sure we haven't advanced past what the caller said the
-	 * buffer size was.
-	 */
-	if (tagger_line - buffer >= size)
-		return error("char" PD_FMT ": tag truncated", size);
-
-	/* The actual stuff afterwards we don't care about.. */
-	return 0;
-}
-
-#undef PD_FMT
-
-int cmd_mktag(int argc, const char **argv, const char *prefix)
-{
-	struct strbuf buf;
-	unsigned char result_sha1[20];
-
-	if (argc != 1)
-		usage("git-mktag < signaturefile");
-
-	strbuf_init(&buf, 0);
-	if (strbuf_read(&buf, 0, 4096) < 0) {
-		die("could not read from stdin");
-	}
-
-	/* Verify it for some basic sanity: it needs to start with
-	   "object <sha1>\ntype\ntagger " */
-	if (verify_tag_buffer(buf.buf, buf.len) < 0)
-		die("invalid tag signature file");
-
-	if (write_sha1_file(buf.buf, buf.len, tag_type, result_sha1) < 0)
-		die("unable to write tag file");
-
-	strbuf_release(&buf);
-	printf("%s\n", sha1_to_hex(result_sha1));
-	return 0;
-}
-- 
1.5.5.67.g9a49

[PATCH 5/5] git-tag: call verify_tag_buffer to validate the generated tag object

[drafnel]

From: Brandon Casey <drafnel@gmail.com>

This will ensure that both git-tag and git-mktag "produce valid tags,
with the same definition of validity."

Signed-off-by: Brandon Casey <drafnel@gmail.com>
---
 builtin-tag.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/builtin-tag.c b/builtin-tag.c
index 85ec2f3..0f79d47 100644
--- a/builtin-tag.c
+++ b/builtin-tag.c
@@ -508,6 +508,8 @@ static void create_tag(const unsigned char *object, const char *tag,
 
 	if (sign && do_sign(buf) < 0)
 		die("unable to sign the tag");
+	if (verify_tag_buffer(buf->buf, buf->len) < 0)
+		die("invalid tag object");
 	if (write_sha1_file(buf->buf, buf->len, tag_type, result) < 0)
 		die("unable to write tag file");
 }
-- 
1.5.5.67.g9a49