mktag.c: consolidate tag functions by merging mktag.c into builtin-tag.c

From: Brandon Casey <[email protected]>

Signed-off-by Brandon Casey <[email protected]>

Makefile +1 -0

@@ -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

Quoted 3 lines · [email protected] writes:

From: Brandon Casey <[email protected]>

Signed-off-by Brandon Casey <[email protected]>

Review LIB_OBJS += log-tree.o · line 426

Quoted 13 lines

(quoted diff) +2 -0

@@ -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?

Quoted 3 lines · [email protected] writes:

From: Brandon Casey <[email protected]>

Signed-off-by Brandon Casey <[email protected]>

Makefile LIB_OBJS += log-tree.o · line 426

Quoted 13 lines

Makefile +2 -0

@@ -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 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"?

Quoted 6 lines · Junio C Hamano

Quoted 3 lines · [email protected] writes:

From: Brandon Casey <[email protected]>

Signed-off-by Brandon Casey <[email protected]>

Quoted 15 lines

Makefile LIB_OBJS += log-tree.o · line 426

Quoted 13 lines

Makefile +2 -0

@@ -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.

Hi,

Quoted 20 lines · Brandon Casey

Quoted 19 lines · Junio C Hamano

Quoted 12 lines · [email protected] writes:

From: Brandon Casey <[email protected]>

Signed-off-by Brandon Casey <[email protected]>

Makefile +1 -0

@@ -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

Makefile LIB_OBJS += log-tree.o · line 426

Quoted 4 lines

Makefile +1 -0

@@ -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?

Quoted 3 lines

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

From: Brandon Casey <[email protected]>

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 <[email protected]>

---
 mktag.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

mktag.c +8 -3

index 0b34341..5489ad6 100644
@@ -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;
 }

Signature · 1 line

1.5.5.67.g9a49

Quoted 3 lines · [email protected] writes:

From: Brandon Casey <[email protected]>

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

Correct.

Quoted 5 lines

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?