{"thread":{"id":"13448","subject":"[PATCH 5/5] git-tag: call verify_tag_buffer to validate the generated tag object","startedAt":"2008-05-09T02:19:45Z","lastAt":"2008-05-12T18:41:13Z","messageCount":13,"participants":["drafnel@gmail.com","Junio C Hamano","Brandon Casey","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"76434","messageId":"3712867.1210299524581.JavaMail.teamon@b301.teamon.com","threadId":"13448","inReplyTo":"1210299589-10448-1-git-send-email-drafnel@example.com","subject":"[PATCH 1/5] mktag.c: adjust verify_tag parameters","fromName":"","fromEmail":"drafnel@gmail.com","sentAt":"2008-05-09T02:19:45Z","receivedAt":"2008-05-09T02:19:45Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"From: Brandon Casey <casey@nrlssc.navy.mil>\n\nThe size parameter should be a size_t since it is a string length.\n\nThere is no reason that the buffer argument should not be constant except\nfor the nul termination that is performed in the first few lines of this\nfunction. This is not necessary, since a valid c string must always be\nnul terminated and we can check whether we have exceeded the caller's\nsize parameter at the end of parsing the buffer.\n\nSigned-off-by: Brandon Casey <casey@nrlssc.navy.mil>\n---\n mktag.c |   11 ++++++++---\n 1 files changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/mktag.c b/mktag.c\nindex 0b34341..5489ad6 100644\n--- a/mktag.c\n+++ b/mktag.c\n@@ -39,7 +39,7 @@ static int verify_object(unsigned char *sha1, const char *expected_type)\n #define PD_FMT \"%td\"\n #endif\n \n-static int verify_tag(char *buffer, unsigned long size)\n+static int verify_tag(const char *buffer, size_t size)\n {\n \tint typelen;\n \tchar type[20];\n@@ -50,8 +50,6 @@ static int verify_tag(char *buffer, unsigned long size)\n \tif (size < 84)\n \t\treturn error(\"wanna fool me ? you obviously got the size wrong !\");\n \n-\tbuffer[size] = 0;\n-\n \t/* Verify object line */\n \tobject = buffer;\n \tif (memcmp(object, \"object \", 7))\n@@ -145,6 +143,13 @@ static int verify_tag(char *buffer, unsigned long size)\n \t\treturn error(\"char\" PD_FMT \": trailing garbage in tag header\",\n \t\t\ttagger_line - buffer);\n \n+\t/*\n+\t * Make sure we haven't advanced past what the caller said the\n+\t * buffer size was.\n+\t */\n+\tif (tagger_line - buffer >= size)\n+\t\treturn error(\"char\" PD_FMT \": tag truncated\", size);\n+\n \t/* The actual stuff afterwards we don't care about.. */\n \treturn 0;\n }\n-- \n1.5.5.67.g9a49\n"},{"id":"76433","messageId":"7689656.1210299528037.JavaMail.teamon@b301.teamon.com","threadId":"13448","inReplyTo":"1210299589-10448-2-git-send-email-drafnel@example.com","subject":"[PATCH 2/5] Make mktag a builtin.","fromName":"","fromEmail":"drafnel@gmail.com","sentAt":"2008-05-09T02:19:46Z","receivedAt":"2008-05-09T02:19:46Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"From: Brandon Casey <drafnel@gmail.com>\n\nSigned-off-by: Brandon Casey <drafnel@gmail.com>\n---\n Makefile  |    3 ++-\n builtin.h |    1 +\n git.c     |    1 +\n mktag.c   |    4 +---\n 4 files changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 9d84c8d..1128cef 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -282,7 +282,6 @@ PROGRAMS += git-imap-send$X\n PROGRAMS += git-index-pack$X\n PROGRAMS += git-merge-index$X\n PROGRAMS += git-merge-tree$X\n-PROGRAMS += git-mktag$X\n PROGRAMS += git-mktree$X\n PROGRAMS += git-pack-redundant$X\n PROGRAMS += git-patch-id$X\n@@ -306,6 +305,7 @@ BUILT_INS += git-fsck-objects$X\n BUILT_INS += git-get-tar-commit-id$X\n BUILT_INS += git-init$X\n BUILT_INS += git-merge-subtree$X\n+BUILT_INS += git-mktag$X\n BUILT_INS += git-peek-remote$X\n BUILT_INS += git-repo-config$X\n BUILT_INS += git-show$X\n@@ -423,6 +423,7 @@ LIB_OBJS += log-tree.o\n LIB_OBJS += mailmap.o\n LIB_OBJS += match-trees.o\n LIB_OBJS += merge-file.o\n+LIB_OBJS += mktag.o\n LIB_OBJS += object.o\n LIB_OBJS += pack-check.o\n LIB_OBJS += pack-revindex.o\ndiff --git a/builtin.h b/builtin.h\nindex 95126fd..a8d3a11 100644\n--- a/builtin.h\n+++ b/builtin.h\n@@ -58,6 +58,7 @@ extern int cmd_merge_base(int argc, const char **argv, const char *prefix);\n extern int cmd_merge_ours(int argc, const char **argv, const char *prefix);\n extern int cmd_merge_file(int argc, const char **argv, const char *prefix);\n extern int cmd_merge_recursive(int argc, const char **argv, const char *prefix);\n+extern int cmd_mktag(int argc, const char **argv, const char *prefix);\n extern int cmd_mv(int argc, const char **argv, const char *prefix);\n extern int cmd_name_rev(int argc, const char **argv, const char *prefix);\n extern int cmd_pack_objects(int argc, const char **argv, const char *prefix);\ndiff --git a/git.c b/git.c\nindex 89b431f..81aa31c 100644\n--- a/git.c\n+++ b/git.c\n@@ -325,6 +325,7 @@ static void handle_internal_command(int argc, const char **argv)\n \t\t{ \"merge-ours\", cmd_merge_ours, RUN_SETUP },\n \t\t{ \"merge-recursive\", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE },\n \t\t{ \"merge-subtree\", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE },\n+\t\t{ \"mktag\", cmd_mktag, RUN_SETUP },\n \t\t{ \"mv\", cmd_mv, RUN_SETUP | NEED_WORK_TREE },\n \t\t{ \"name-rev\", cmd_name_rev, RUN_SETUP },\n \t\t{ \"pack-objects\", cmd_pack_objects, RUN_SETUP },\ndiff --git a/mktag.c b/mktag.c\nindex 5489ad6..352747b 100644\n--- a/mktag.c\n+++ b/mktag.c\n@@ -156,7 +156,7 @@ static int verify_tag(const char *buffer, size_t size)\n \n #undef PD_FMT\n \n-int main(int argc, char **argv)\n+int cmd_mktag(int argc, const char **argv, const char *prefix)\n {\n \tstruct strbuf buf;\n \tunsigned char result_sha1[20];\n@@ -164,8 +164,6 @@ int main(int argc, char **argv)\n \tif (argc != 1)\n \t\tusage(\"git-mktag < signaturefile\");\n \n-\tsetup_git_directory();\n-\n \tstrbuf_init(&buf, 0);\n \tif (strbuf_read(&buf, 0, 4096) < 0) {\n \t\tdie(\"could not read from stdin\");\n-- \n1.5.5.67.g9a49\n"},{"id":"76436","messageId":"5164765.1210299531781.JavaMail.teamon@b301.teamon.com","threadId":"13448","inReplyTo":"1210299589-10448-3-git-send-email-drafnel@example.com","subject":"[PATCH 3/5] mktag.c: rename verify_tag to verify_tag_buffer","fromName":"","fromEmail":"drafnel@gmail.com","sentAt":"2008-05-09T02:19:47Z","receivedAt":"2008-05-09T02:19:47Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"From: Brandon Casey <drafnel@gmail.com>\n\nThis is in preparation for merging mktag.c with builtin-tag.c and\nusing verify_tag_buffer within the git-tag code path.\n\nSigned-off-by: Brandon Casey <drafnel@gmail.com>\n---\n mktag.c |    4 ++--\n 1 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/mktag.c b/mktag.c\nindex 352747b..6204001 100644\n--- a/mktag.c\n+++ b/mktag.c\n@@ -39,7 +39,7 @@ static int verify_object(unsigned char *sha1, const char *expected_type)\n #define PD_FMT \"%td\"\n #endif\n \n-static int verify_tag(const char *buffer, size_t size)\n+static int verify_tag_buffer(const char *buffer, size_t size)\n {\n \tint typelen;\n \tchar type[20];\n@@ -171,7 +171,7 @@ int cmd_mktag(int argc, const char **argv, const char *prefix)\n \n \t/* Verify it for some basic sanity: it needs to start with\n \t   \"object <sha1>\\ntype\\ntagger \" */\n-\tif (verify_tag(buf.buf, buf.len) < 0)\n+\tif (verify_tag_buffer(buf.buf, buf.len) < 0)\n \t\tdie(\"invalid tag signature file\");\n \n \tif (write_sha1_file(buf.buf, buf.len, tag_type, result_sha1) < 0)\n-- \n1.5.5.67.g9a49\n"},{"id":"76435","messageId":"2582798.1210299536197.JavaMail.teamon@b301.teamon.com","threadId":"13448","inReplyTo":"1210299589-10448-4-git-send-email-drafnel@example.com","subject":"[PATCH 4/5] mktag.c: consolidate tag functions by merging mktag.c into builtin-tag.c","fromName":"","fromEmail":"drafnel@gmail.com","sentAt":"2008-05-09T02:19:48Z","receivedAt":"2008-05-09T02:19:48Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"From: Brandon Casey <drafnel@gmail.com>\n\nSigned-off-by: Brandon Casey <drafnel@gmail.com>\n---\n Makefile      |    1 -\n builtin-tag.c |  181 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n mktag.c       |  183 ---------------------------------------------------------\n 3 files changed, 181 insertions(+), 184 deletions(-)\n delete mode 100644 mktag.c\n\ndiff --git a/Makefile b/Makefile\nindex 1128cef..572459f 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -423,7 +423,6 @@ LIB_OBJS += log-tree.o\n LIB_OBJS += mailmap.o\n LIB_OBJS += match-trees.o\n LIB_OBJS += merge-file.o\n-LIB_OBJS += mktag.o\n LIB_OBJS += object.o\n LIB_OBJS += pack-check.o\n LIB_OBJS += pack-revindex.o\ndiff --git a/builtin-tag.c b/builtin-tag.c\nindex 129ff57..85ec2f3 100644\n--- a/builtin-tag.c\n+++ b/builtin-tag.c\n@@ -296,6 +296,161 @@ static void write_tag_body(int fd, const unsigned char *sha1)\n \tfree(buf);\n }\n \n+/*\n+ * We refuse to tag something we can't verify. Just because.\n+ */\n+static int verify_object(unsigned char *sha1, const char *expected_type)\n+{\n+\tint ret = -1;\n+\tenum object_type type;\n+\tunsigned long size;\n+\tvoid *buffer = read_sha1_file(sha1, &type, &size);\n+\n+\tif (buffer) {\n+\t\tif (type == type_from_string(expected_type))\n+\t\t\tret = check_sha1_signature(sha1, buffer, size, expected_type);\n+\t\tfree(buffer);\n+\t}\n+\treturn ret;\n+}\n+\n+#ifdef NO_C99_FORMAT\n+#define PD_FMT \"%d\"\n+#else\n+#define PD_FMT \"%td\"\n+#endif\n+\n+/*\n+ * A signature file has a very simple fixed format: four lines\n+ * of \"object <sha1>\" + \"type <typename>\" + \"tag <tagname>\" +\n+ * \"tagger <committer>\", followed by a blank line, a free-form tag\n+ * message and a signature block that git itself doesn't care about,\n+ * but that can be verified with gpg or similar.\n+ *\n+ * The first four lines are guaranteed to be at least 83 bytes:\n+ * \"object <sha1>\\n\" is 48 bytes, \"type tag\\n\" at 9 bytes is the\n+ * shortest possible type-line, \"tag .\\n\" at 6 bytes is the shortest\n+ * single-character-tag line, and \"tagger . <> 0 +0000\\n\" at 20 bytes is\n+ * the shortest possible tagger-line.\n+ */\n+\n+static int verify_tag_buffer(const char *buffer, size_t size)\n+{\n+\tint typelen;\n+\tchar type[20];\n+\tunsigned char sha1[20];\n+\tconst char *object, *type_line, *tag_line, *tagger_line, *lb, *rb;\n+\tsize_t len;\n+\n+\tif (size < 84)\n+\t\treturn error(\"wanna fool me ? you obviously got the size wrong !\");\n+\n+\t/* Verify object line */\n+\tobject = buffer;\n+\tif (memcmp(object, \"object \", 7))\n+\t\treturn error(\"char%d: does not start with \\\"object \\\"\", 0);\n+\n+\tif (get_sha1_hex(object + 7, sha1))\n+\t\treturn error(\"char%d: could not get SHA1 hash\", 7);\n+\n+\t/* Verify type line */\n+\ttype_line = object + 48;\n+\tif (memcmp(type_line - 1, \"\\ntype \", 6))\n+\t\treturn error(\"char%d: could not find \\\"\\\\ntype \\\"\", 47);\n+\n+\t/* Verify tag-line */\n+\ttag_line = strchr(type_line, '\\n');\n+\tif (!tag_line)\n+\t\treturn error(\"char\" PD_FMT \": could not find next \\\"\\\\n\\\"\", type_line - buffer);\n+\ttag_line++;\n+\tif (memcmp(tag_line, \"tag \", 4) || tag_line[4] == '\\n')\n+\t\treturn error(\"char\" PD_FMT \": no \\\"tag \\\" found\", tag_line - buffer);\n+\n+\t/* Get the actual type */\n+\ttypelen = tag_line - type_line - strlen(\"type \\n\");\n+\tif (typelen >= sizeof(type))\n+\t\treturn error(\"char\" PD_FMT \": type too long\", type_line+5 - buffer);\n+\n+\tmemcpy(type, type_line+5, typelen);\n+\ttype[typelen] = 0;\n+\n+\t/* Verify that the object matches */\n+\tif (verify_object(sha1, type))\n+\t\treturn error(\"char%d: could not verify object %s\", 7, sha1_to_hex(sha1));\n+\n+\t/* Verify the tag-name: we don't allow control characters or spaces in it */\n+\ttag_line += 4;\n+\tfor (;;) {\n+\t\tunsigned char c = *tag_line++;\n+\t\tif (c == '\\n')\n+\t\t\tbreak;\n+\t\tif (c > ' ')\n+\t\t\tcontinue;\n+\t\treturn error(\"char\" PD_FMT \": could not verify tag name\", tag_line - buffer);\n+\t}\n+\n+\t/* Verify the tagger line */\n+\ttagger_line = tag_line;\n+\n+\tif (memcmp(tagger_line, \"tagger \", 7))\n+\t\treturn error(\"char\" PD_FMT \": could not find \\\"tagger \\\"\",\n+\t\t\ttagger_line - buffer);\n+\n+\t/*\n+\t * Check for correct form for name and email\n+\t * i.e. \" <\" followed by \"> \" on _this_ line\n+\t * No angle brackets within the name or email address fields.\n+\t * No spaces within the email address field.\n+\t */\n+\ttagger_line += 7;\n+\tif (!(lb = strstr(tagger_line, \" <\")) || !(rb = strstr(lb+2, \"> \")) ||\n+\t\tstrpbrk(tagger_line, \"<>\\n\") != lb+1 ||\n+\t\tstrpbrk(lb+2, \"><\\n \") != rb)\n+\t\treturn error(\"char\" PD_FMT \": malformed tagger field\",\n+\t\t\ttagger_line - buffer);\n+\n+\t/* Check for author name, at least one character, space is acceptable */\n+\tif (lb == tagger_line)\n+\t\treturn error(\"char\" PD_FMT \": missing tagger name\",\n+\t\t\ttagger_line - buffer);\n+\n+\t/* timestamp, 1 or more digits followed by space */\n+\ttagger_line = rb + 2;\n+\tif (!(len = strspn(tagger_line, \"0123456789\")))\n+\t\treturn error(\"char\" PD_FMT \": missing tag timestamp\",\n+\t\t\ttagger_line - buffer);\n+\ttagger_line += len;\n+\tif (*tagger_line != ' ')\n+\t\treturn error(\"char\" PD_FMT \": malformed tag timestamp\",\n+\t\t\ttagger_line - buffer);\n+\ttagger_line++;\n+\n+\t/* timezone, 5 digits [+-]hhmm, max. 1400 */\n+\tif (!((tagger_line[0] == '+' || tagger_line[0] == '-') &&\n+\t      strspn(tagger_line+1, \"0123456789\") == 4 &&\n+\t      tagger_line[5] == '\\n' && atoi(tagger_line+1) <= 1400))\n+\t\treturn error(\"char\" PD_FMT \": malformed tag timezone\",\n+\t\t\ttagger_line - buffer);\n+\ttagger_line += 6;\n+\n+\t/* Verify the blank line separating the header from the body */\n+\tif (*tagger_line != '\\n')\n+\t\treturn error(\"char\" PD_FMT \": trailing garbage in tag header\",\n+\t\t\ttagger_line - buffer);\n+\n+\t/*\n+\t * Make sure we haven't advanced past what the caller said the\n+\t * buffer size was.\n+\t */\n+\tif (tagger_line - buffer >= size)\n+\t\treturn error(\"char\" PD_FMT \": tag truncated\", size);\n+\n+\t/* The actual stuff afterwards we don't care about.. */\n+\treturn 0;\n+}\n+\n+#undef PD_FMT\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       unsigned char *prev, unsigned char *result)\n@@ -482,3 +637,29 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \tstrbuf_release(&buf);\n \treturn 0;\n }\n+\n+int cmd_mktag(int argc, const char **argv, const char *prefix)\n+{\n+\tstruct strbuf buf;\n+\tunsigned char result_sha1[20];\n+\n+\tif (argc != 1)\n+\t\tusage(\"git-mktag < signaturefile\");\n+\n+\tstrbuf_init(&buf, 0);\n+\tif (strbuf_read(&buf, 0, 4096) < 0) {\n+\t\tdie(\"could not read from stdin\");\n+\t}\n+\n+\t/* Verify it for some basic sanity: it needs to start with\n+\t   \"object <sha1>\\ntype\\ntagger \" */\n+\tif (verify_tag_buffer(buf.buf, buf.len) < 0)\n+\t\tdie(\"invalid tag signature file\");\n+\n+\tif (write_sha1_file(buf.buf, buf.len, tag_type, result_sha1) < 0)\n+\t\tdie(\"unable to write tag file\");\n+\n+\tstrbuf_release(&buf);\n+\tprintf(\"%s\\n\", sha1_to_hex(result_sha1));\n+\treturn 0;\n+}\ndiff --git a/mktag.c b/mktag.c\ndeleted file mode 100644\nindex 6204001..0000000\n--- a/mktag.c\n+++ /dev/null\n@@ -1,183 +0,0 @@\n-#include \"cache.h\"\n-#include \"tag.h\"\n-\n-/*\n- * A signature file has a very simple fixed format: four lines\n- * of \"object <sha1>\" + \"type <typename>\" + \"tag <tagname>\" +\n- * \"tagger <committer>\", followed by a blank line, a free-form tag\n- * message and a signature block that git itself doesn't care about,\n- * but that can be verified with gpg or similar.\n- *\n- * The first four lines are guaranteed to be at least 83 bytes:\n- * \"object <sha1>\\n\" is 48 bytes, \"type tag\\n\" at 9 bytes is the\n- * shortest possible type-line, \"tag .\\n\" at 6 bytes is the shortest\n- * single-character-tag line, and \"tagger . <> 0 +0000\\n\" at 20 bytes is\n- * the shortest possible tagger-line.\n- */\n-\n-/*\n- * We refuse to tag something we can't verify. Just because.\n- */\n-static int verify_object(unsigned char *sha1, const char *expected_type)\n-{\n-\tint ret = -1;\n-\tenum object_type type;\n-\tunsigned long size;\n-\tvoid *buffer = read_sha1_file(sha1, &type, &size);\n-\n-\tif (buffer) {\n-\t\tif (type == type_from_string(expected_type))\n-\t\t\tret = check_sha1_signature(sha1, buffer, size, expected_type);\n-\t\tfree(buffer);\n-\t}\n-\treturn ret;\n-}\n-\n-#ifdef NO_C99_FORMAT\n-#define PD_FMT \"%d\"\n-#else\n-#define PD_FMT \"%td\"\n-#endif\n-\n-static int verify_tag_buffer(const char *buffer, size_t size)\n-{\n-\tint typelen;\n-\tchar type[20];\n-\tunsigned char sha1[20];\n-\tconst char *object, *type_line, *tag_line, *tagger_line, *lb, *rb;\n-\tsize_t len;\n-\n-\tif (size < 84)\n-\t\treturn error(\"wanna fool me ? you obviously got the size wrong !\");\n-\n-\t/* Verify object line */\n-\tobject = buffer;\n-\tif (memcmp(object, \"object \", 7))\n-\t\treturn error(\"char%d: does not start with \\\"object \\\"\", 0);\n-\n-\tif (get_sha1_hex(object + 7, sha1))\n-\t\treturn error(\"char%d: could not get SHA1 hash\", 7);\n-\n-\t/* Verify type line */\n-\ttype_line = object + 48;\n-\tif (memcmp(type_line - 1, \"\\ntype \", 6))\n-\t\treturn error(\"char%d: could not find \\\"\\\\ntype \\\"\", 47);\n-\n-\t/* Verify tag-line */\n-\ttag_line = strchr(type_line, '\\n');\n-\tif (!tag_line)\n-\t\treturn error(\"char\" PD_FMT \": could not find next \\\"\\\\n\\\"\", type_line - buffer);\n-\ttag_line++;\n-\tif (memcmp(tag_line, \"tag \", 4) || tag_line[4] == '\\n')\n-\t\treturn error(\"char\" PD_FMT \": no \\\"tag \\\" found\", tag_line - buffer);\n-\n-\t/* Get the actual type */\n-\ttypelen = tag_line - type_line - strlen(\"type \\n\");\n-\tif (typelen >= sizeof(type))\n-\t\treturn error(\"char\" PD_FMT \": type too long\", type_line+5 - buffer);\n-\n-\tmemcpy(type, type_line+5, typelen);\n-\ttype[typelen] = 0;\n-\n-\t/* Verify that the object matches */\n-\tif (verify_object(sha1, type))\n-\t\treturn error(\"char%d: could not verify object %s\", 7, sha1_to_hex(sha1));\n-\n-\t/* Verify the tag-name: we don't allow control characters or spaces in it */\n-\ttag_line += 4;\n-\tfor (;;) {\n-\t\tunsigned char c = *tag_line++;\n-\t\tif (c == '\\n')\n-\t\t\tbreak;\n-\t\tif (c > ' ')\n-\t\t\tcontinue;\n-\t\treturn error(\"char\" PD_FMT \": could not verify tag name\", tag_line - buffer);\n-\t}\n-\n-\t/* Verify the tagger line */\n-\ttagger_line = tag_line;\n-\n-\tif (memcmp(tagger_line, \"tagger \", 7))\n-\t\treturn error(\"char\" PD_FMT \": could not find \\\"tagger \\\"\",\n-\t\t\ttagger_line - buffer);\n-\n-\t/*\n-\t * Check for correct form for name and email\n-\t * i.e. \" <\" followed by \"> \" on _this_ line\n-\t * No angle brackets within the name or email address fields.\n-\t * No spaces within the email address field.\n-\t */\n-\ttagger_line += 7;\n-\tif (!(lb = strstr(tagger_line, \" <\")) || !(rb = strstr(lb+2, \"> \")) ||\n-\t\tstrpbrk(tagger_line, \"<>\\n\") != lb+1 ||\n-\t\tstrpbrk(lb+2, \"><\\n \") != rb)\n-\t\treturn error(\"char\" PD_FMT \": malformed tagger field\",\n-\t\t\ttagger_line - buffer);\n-\n-\t/* Check for author name, at least one character, space is acceptable */\n-\tif (lb == tagger_line)\n-\t\treturn error(\"char\" PD_FMT \": missing tagger name\",\n-\t\t\ttagger_line - buffer);\n-\n-\t/* timestamp, 1 or more digits followed by space */\n-\ttagger_line = rb + 2;\n-\tif (!(len = strspn(tagger_line, \"0123456789\")))\n-\t\treturn error(\"char\" PD_FMT \": missing tag timestamp\",\n-\t\t\ttagger_line - buffer);\n-\ttagger_line += len;\n-\tif (*tagger_line != ' ')\n-\t\treturn error(\"char\" PD_FMT \": malformed tag timestamp\",\n-\t\t\ttagger_line - buffer);\n-\ttagger_line++;\n-\n-\t/* timezone, 5 digits [+-]hhmm, max. 1400 */\n-\tif (!((tagger_line[0] == '+' || tagger_line[0] == '-') &&\n-\t      strspn(tagger_line+1, \"0123456789\") == 4 &&\n-\t      tagger_line[5] == '\\n' && atoi(tagger_line+1) <= 1400))\n-\t\treturn error(\"char\" PD_FMT \": malformed tag timezone\",\n-\t\t\ttagger_line - buffer);\n-\ttagger_line += 6;\n-\n-\t/* Verify the blank line separating the header from the body */\n-\tif (*tagger_line != '\\n')\n-\t\treturn error(\"char\" PD_FMT \": trailing garbage in tag header\",\n-\t\t\ttagger_line - buffer);\n-\n-\t/*\n-\t * Make sure we haven't advanced past what the caller said the\n-\t * buffer size was.\n-\t */\n-\tif (tagger_line - buffer >= size)\n-\t\treturn error(\"char\" PD_FMT \": tag truncated\", size);\n-\n-\t/* The actual stuff afterwards we don't care about.. */\n-\treturn 0;\n-}\n-\n-#undef PD_FMT\n-\n-int cmd_mktag(int argc, const char **argv, const char *prefix)\n-{\n-\tstruct strbuf buf;\n-\tunsigned char result_sha1[20];\n-\n-\tif (argc != 1)\n-\t\tusage(\"git-mktag < signaturefile\");\n-\n-\tstrbuf_init(&buf, 0);\n-\tif (strbuf_read(&buf, 0, 4096) < 0) {\n-\t\tdie(\"could not read from stdin\");\n-\t}\n-\n-\t/* Verify it for some basic sanity: it needs to start with\n-\t   \"object <sha1>\\ntype\\ntagger \" */\n-\tif (verify_tag_buffer(buf.buf, buf.len) < 0)\n-\t\tdie(\"invalid tag signature file\");\n-\n-\tif (write_sha1_file(buf.buf, buf.len, tag_type, result_sha1) < 0)\n-\t\tdie(\"unable to write tag file\");\n-\n-\tstrbuf_release(&buf);\n-\tprintf(\"%s\\n\", sha1_to_hex(result_sha1));\n-\treturn 0;\n-}\n-- \n1.5.5.67.g9a49\n"},{"id":"76432","messageId":"859967.1210299539642.JavaMail.teamon@b301.teamon.com","threadId":"13448","inReplyTo":"1210299589-10448-5-git-send-email-drafnel@example.com","subject":"[PATCH 5/5] git-tag: call verify_tag_buffer to validate the generated tag object","fromName":"","fromEmail":"drafnel@gmail.com","sentAt":"2008-05-09T02:19:49Z","receivedAt":"2008-05-09T02:19:49Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"From: Brandon Casey <drafnel@gmail.com>\n\nThis will ensure that both git-tag and git-mktag \"produce valid tags,\nwith the same definition of validity.\"\n\nSigned-off-by: Brandon Casey <drafnel@gmail.com>\n---\n builtin-tag.c |    2 ++\n 1 files changed, 2 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin-tag.c b/builtin-tag.c\nindex 85ec2f3..0f79d47 100644\n--- a/builtin-tag.c\n+++ b/builtin-tag.c\n@@ -508,6 +508,8 @@ static void create_tag(const unsigned char *object, const char *tag,\n \n \tif (sign && do_sign(buf) < 0)\n \t\tdie(\"unable to sign the tag\");\n+\tif (verify_tag_buffer(buf->buf, buf->len) < 0)\n+\t\tdie(\"invalid tag object\");\n \tif (write_sha1_file(buf->buf, buf->len, tag_type, result) < 0)\n \t\tdie(\"unable to write tag file\");\n }\n-- \n1.5.5.67.g9a49\n"},{"id":"76643","messageId":"7v63tk6992.fsf@gitster.siamese.dyndns.org","threadId":"13448","inReplyTo":"7689656.1210299528037.JavaMail.teamon@b301.teamon.com","subject":"Re: [PATCH 2/5] Make mktag a builtin.","fromName":"Junio C Hamano","fromEmail":"junio@pobox.com","sentAt":"2008-05-11T17:28:09Z","receivedAt":"2008-05-11T17:28:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"drafnel@gmail.com writes:\n\n> From: Brandon Casey <drafnel@gmail.com>\n>\n> Signed-off-by: Brandon Casey <drafnel@gmail.com>\n\n> @@ -306,6 +305,7 @@ BUILT_INS += git-fsck-objects$X\n>  BUILT_INS += git-get-tar-commit-id$X\n>  BUILT_INS += git-init$X\n>  BUILT_INS += git-merge-subtree$X\n> +BUILT_INS += git-mktag$X\n>  BUILT_INS += git-peek-remote$X\n>  BUILT_INS += git-repo-config$X\n>  BUILT_INS += git-show$X\n> @@ -423,6 +423,7 @@ LIB_OBJS += log-tree.o\n>  LIB_OBJS += mailmap.o\n>  LIB_OBJS += match-trees.o\n>  LIB_OBJS += merge-file.o\n> +LIB_OBJS += mktag.o\n\nThis is unusual for a builtin.  Why didn't it migrate to builtin-mktag?\n"},{"id":"76645","messageId":"7vprrs4uaa.fsf@gitster.siamese.dyndns.org","threadId":"13448","inReplyTo":"7v63tk6992.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/5] Make mktag a builtin.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-05-11T17:36:45Z","receivedAt":"2008-05-11T17:36:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <junio@pobox.com> writes:\n\n> drafnel@gmail.com writes:\n>\n>> From: Brandon Casey <drafnel@gmail.com>\n>>\n>> Signed-off-by: Brandon Casey <drafnel@gmail.com>\n>\n>> @@ -306,6 +305,7 @@ BUILT_INS += git-fsck-objects$X\n>>  BUILT_INS += git-get-tar-commit-id$X\n>>  BUILT_INS += git-init$X\n>>  BUILT_INS += git-merge-subtree$X\n>> +BUILT_INS += git-mktag$X\n>>  BUILT_INS += git-peek-remote$X\n>>  BUILT_INS += git-repo-config$X\n>>  BUILT_INS += git-show$X\n>> @@ -423,6 +423,7 @@ LIB_OBJS += log-tree.o\n>>  LIB_OBJS += mailmap.o\n>>  LIB_OBJS += match-trees.o\n>>  LIB_OBJS += merge-file.o\n>> +LIB_OBJS += mktag.o\n>\n> This is unusual for a builtin.  Why didn't it migrate to builtin-mktag?\n\nThat is, something along this line, instead.\n\n-- >8 --\nFrom: Brandon Casey <drafnel@gmail.com>\nDate: Thu, 8 May 2008 21:19:46 -0500\nSubject: [PATCH] Make mktag a builtin\n\nSigned-off-by: Brandon Casey <drafnel@gmail.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n Makefile                   |    2 +-\n mktag.c => builtin-mktag.c |    4 +---\n builtin.h                  |    1 +\n git.c                      |    1 +\n 4 files changed, 4 insertions(+), 4 deletions(-)\n rename mktag.c => builtin-mktag.c (98%)\n\ndiff --git a/Makefile b/Makefile\nindex 9d84c8d..3980dc9 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -282,7 +282,6 @@ PROGRAMS += git-imap-send$X\n PROGRAMS += git-index-pack$X\n PROGRAMS += git-merge-index$X\n PROGRAMS += git-merge-tree$X\n-PROGRAMS += git-mktag$X\n PROGRAMS += git-mktree$X\n PROGRAMS += git-pack-redundant$X\n PROGRAMS += git-patch-id$X\n@@ -510,6 +509,7 @@ BUILTIN_OBJS += builtin-merge-base.o\n BUILTIN_OBJS += builtin-merge-file.o\n BUILTIN_OBJS += builtin-merge-ours.o\n BUILTIN_OBJS += builtin-merge-recursive.o\n+BUILTIN_OBJS += builtin-mktag.o\n BUILTIN_OBJS += builtin-mv.o\n BUILTIN_OBJS += builtin-name-rev.o\n BUILTIN_OBJS += builtin-pack-objects.o\ndiff --git a/mktag.c b/builtin-mktag.c\nsimilarity index 98%\nrename from mktag.c\nrename to builtin-mktag.c\nindex a45a5e7..1596900 100644\n--- a/mktag.c\n+++ b/builtin-mktag.c\n@@ -149,7 +149,7 @@ static int verify_tag(const char *buffer, size_t size)\n \n #undef PD_FMT\n \n-int main(int argc, char **argv)\n+int cmd_mktag(int argc, const char **argv, const char *prefix)\n {\n \tstruct strbuf buf;\n \tunsigned char result_sha1[20];\n@@ -157,8 +157,6 @@ int main(int argc, char **argv)\n \tif (argc != 1)\n \t\tusage(\"git-mktag < signaturefile\");\n \n-\tsetup_git_directory();\n-\n \tstrbuf_init(&buf, 0);\n \tif (strbuf_read(&buf, 0, 4096) < 0) {\n \t\tdie(\"could not read from stdin\");\ndiff --git a/builtin.h b/builtin.h\nindex 95126fd..a8d3a11 100644\n--- a/builtin.h\n+++ b/builtin.h\n@@ -58,6 +58,7 @@ extern int cmd_merge_base(int argc, const char **argv, const char *prefix);\n extern int cmd_merge_ours(int argc, const char **argv, const char *prefix);\n extern int cmd_merge_file(int argc, const char **argv, const char *prefix);\n extern int cmd_merge_recursive(int argc, const char **argv, const char *prefix);\n+extern int cmd_mktag(int argc, const char **argv, const char *prefix);\n extern int cmd_mv(int argc, const char **argv, const char *prefix);\n extern int cmd_name_rev(int argc, const char **argv, const char *prefix);\n extern int cmd_pack_objects(int argc, const char **argv, const char *prefix);\ndiff --git a/git.c b/git.c\nindex 89b431f..81aa31c 100644\n--- a/git.c\n+++ b/git.c\n@@ -325,6 +325,7 @@ static void handle_internal_command(int argc, const char **argv)\n \t\t{ \"merge-ours\", cmd_merge_ours, RUN_SETUP },\n \t\t{ \"merge-recursive\", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE },\n \t\t{ \"merge-subtree\", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE },\n+\t\t{ \"mktag\", cmd_mktag, RUN_SETUP },\n \t\t{ \"mv\", cmd_mv, RUN_SETUP | NEED_WORK_TREE },\n \t\t{ \"name-rev\", cmd_name_rev, RUN_SETUP },\n \t\t{ \"pack-objects\", cmd_pack_objects, RUN_SETUP },\n-- \n1.5.5.1.295.g4c42f\n"},{"id":"76654","messageId":"7vmymw3cst.fsf@gitster.siamese.dyndns.org","threadId":"13448","inReplyTo":"3712867.1210299524581.JavaMail.teamon@b301.teamon.com","subject":"Re: [PATCH 1/5] mktag.c: adjust verify_tag parameters","fromName":"Junio C Hamano","fromEmail":"junio@pobox.com","sentAt":"2008-05-11T18:39:46Z","receivedAt":"2008-05-11T18:39:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"drafnel@gmail.com writes:\n\n> From: Brandon Casey <casey@nrlssc.navy.mil>\n>\n> The size parameter should be a size_t since it is a string length.\n\nCorrect.\n\n> There is no reason that the buffer argument should not be constant except\n> for the nul termination that is performed in the first few lines of this\n> function. This is not necessary, since a valid c string must always be\n> nul terminated and we can check whether we have exceeded the caller's\n> size parameter at the end of parsing the buffer.\n\nWait a minute.  The point of passing a stringlet as a tuple of <pointer to\nthe beginning, length> is that you may not have a valid C string to begin\nwith, isn't it?  We shouldn't be assuming that reading past the given size\nis Ok --- you may not be lucky enough to have any NUL byte after the given\nstring before you hit the page boundary and encounter unmapped page.\n\nThe generic-looking argument you made is bogus, but for this particular\ncode it is true, as the parameter you are passing to the function is\nprepared by strbuf_read() which gives you a NUL terminated buffer.\n\nSo the code is correct -- justification is not.\n\n> Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>\n> ---\n>  mktag.c |   11 ++++++++---\n>  1 files changed, 8 insertions(+), 3 deletions(-)\n>\n> diff --git a/mktag.c b/mktag.c\n> index 0b34341..5489ad6 100644\n> --- a/mktag.c\n> +++ b/mktag.c\n> @@ -39,7 +39,7 @@ static int verify_object(unsigned char *sha1, const char *expected_type)\n>  #define PD_FMT \"%td\"\n>  #endif\n>  \n> -static int verify_tag(char *buffer, unsigned long size)\n> +static int verify_tag(const char *buffer, size_t size)\n>  {\n>  \tint typelen;\n>  \tchar type[20];\n> @@ -50,8 +50,6 @@ static int verify_tag(char *buffer, unsigned long size)\n>  \tif (size < 84)\n>  \t\treturn error(\"wanna fool me ? you obviously got the size wrong !\");\n>  \n> -\tbuffer[size] = 0;\n> -\n>  \t/* Verify object line */\n>  \tobject = buffer;\n>  \tif (memcmp(object, \"object \", 7))\n> @@ -145,6 +143,13 @@ static int verify_tag(char *buffer, unsigned long size)\n>  \t\treturn error(\"char\" PD_FMT \": trailing garbage in tag header\",\n>  \t\t\ttagger_line - buffer);\n>  \n> +\t/*\n> +\t * Make sure we haven't advanced past what the caller said the\n> +\t * buffer size was.\n> +\t */\n> +\tif (tagger_line - buffer >= size)\n> +\t\treturn error(\"char\" PD_FMT \": tag truncated\", size);\n> +\n>  \t/* The actual stuff afterwards we don't care about.. */\n>  \treturn 0;\n>  }\n> -- \n> 1.5.5.67.g9a49\n"},{"id":"76715","messageId":"48285DAB.2040707@nrlssc.navy.mil","threadId":"13448","inReplyTo":"7v63tk6992.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/5] Make mktag a builtin.","fromName":"Brandon Casey","fromEmail":"casey@nrlssc.navy.mil","sentAt":"2008-05-12T15:09:31Z","receivedAt":"2008-05-12T15:09:31Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"Junio C Hamano wrote:\n> drafnel@gmail.com writes:\n> \n>> From: Brandon Casey <drafnel@gmail.com>\n>>\n>> Signed-off-by: Brandon Casey <drafnel@gmail.com>\n> \n>> @@ -306,6 +305,7 @@ BUILT_INS += git-fsck-objects$X\n>>  BUILT_INS += git-get-tar-commit-id$X\n>>  BUILT_INS += git-init$X\n>>  BUILT_INS += git-merge-subtree$X\n>> +BUILT_INS += git-mktag$X\n>>  BUILT_INS += git-peek-remote$X\n>>  BUILT_INS += git-repo-config$X\n>>  BUILT_INS += git-show$X\n>> @@ -423,6 +423,7 @@ LIB_OBJS += log-tree.o\n>>  LIB_OBJS += mailmap.o\n>>  LIB_OBJS += match-trees.o\n>>  LIB_OBJS += merge-file.o\n>> +LIB_OBJS += mktag.o\n> \n> This is unusual for a builtin.  Why didn't it migrate to builtin-mktag?\n\nI didn't know how to do it.\n\nI was trying not to do a code move and a code change at the same time.\nI didn't think I should move the non-builtin mktag.c to builtin-mktag.c,\nand then after I modified mktag to be a builtin I knew I was moving it\nto builtin-tag.c so I didn't see a point to renaming it.\n\nAlso, I decided about those things _before_ I realized how small the changes\nwould be to mktag to make it a builtin.\n\nDo you think the modified patch you posted conflicts with the idea that\n\"code move should be separate from code change\"?\n\n-brandon\n"},{"id":"76719","messageId":"482865A1.7040207@nrlssc.navy.mil","threadId":"13448","inReplyTo":"7vmymw3cst.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 1/5] mktag.c: adjust verify_tag parameters","fromName":"Brandon Casey","fromEmail":"casey@nrlssc.navy.mil","sentAt":"2008-05-12T15:43:29Z","receivedAt":"2008-05-12T15:43:29Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"Junio C Hamano wrote:\n> drafnel@gmail.com writes:\n> \n>> From: Brandon Casey <casey@nrlssc.navy.mil>\n>>\n>> The size parameter should be a size_t since it is a string length.\n> \n> Correct.\n> \n>> There is no reason that the buffer argument should not be constant except\n>> for the nul termination that is performed in the first few lines of this\n>> function. This is not necessary, since a valid c string must always be\n>> nul terminated and we can check whether we have exceeded the caller's\n>> size parameter at the end of parsing the buffer.\n> \n> Wait a minute.  The point of passing a stringlet as a tuple of <pointer to\n> the beginning, length> is that you may not have a valid C string to begin\n> with, isn't it?\n\nI thought it was just to limit the scope of parsing.\n\nOther functions that deal with non-nul-terminated memory segments (like memchr)\ndo not use char*, they instead use void*.\n\nIf we want to handle non-nul-terminated memory segments here, we should\nprobably allocate a new string to copy the memory segment into so we can\nensure it is nul-terminated. Otherwise we can't use any of the str*()\nfunctions safely.\n\n-brandon\n"},{"id":"76723","messageId":"alpine.DEB.1.00.0805121803320.30431@racer","threadId":"13448","inReplyTo":"48285DAB.2040707@nrlssc.navy.mil","subject":"Re: [PATCH 2/5] Make mktag a builtin.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-05-12T17:04:42Z","receivedAt":"2008-05-12T17:04:42Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 12 May 2008, Brandon Casey wrote:\n\n> Junio C Hamano wrote:\n> > drafnel@gmail.com writes:\n> > \n> >> From: Brandon Casey <drafnel@gmail.com>\n> >>\n> >> Signed-off-by: Brandon Casey <drafnel@gmail.com>\n> > \n> >> @@ -306,6 +305,7 @@ BUILT_INS += git-fsck-objects$X\n> >>  BUILT_INS += git-get-tar-commit-id$X\n> >>  BUILT_INS += git-init$X\n> >>  BUILT_INS += git-merge-subtree$X\n> >> +BUILT_INS += git-mktag$X\n> >>  BUILT_INS += git-peek-remote$X\n> >>  BUILT_INS += git-repo-config$X\n> >>  BUILT_INS += git-show$X\n> >> @@ -423,6 +423,7 @@ LIB_OBJS += log-tree.o\n> >>  LIB_OBJS += mailmap.o\n> >>  LIB_OBJS += match-trees.o\n> >>  LIB_OBJS += merge-file.o\n> >> +LIB_OBJS += mktag.o\n> > \n> > This is unusual for a builtin.  Why didn't it migrate to builtin-mktag?\n> \n> I didn't know how to do it.\n> \n> I was trying not to do a code move and a code change at the same time.\n\nWhy did you not consult Git's own history for guidance?  See e.g.\n\n\t$ git log next --diff-filter=A builtin-*.c\n\nCiao,\nDscho\n"},{"id":"76728","messageId":"48287F14.9040105@nrlssc.navy.mil","threadId":"13448","inReplyTo":"alpine.DEB.1.00.0805121803320.30431@racer","subject":"Re: [PATCH 2/5] Make mktag a builtin.","fromName":"Brandon Casey","fromEmail":"casey@nrlssc.navy.mil","sentAt":"2008-05-12T17:32:04Z","receivedAt":"2008-05-12T17:32:04Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"Johannes Schindelin wrote:\n> Hi,\n> \n> On Mon, 12 May 2008, Brandon Casey wrote:\n> \n>> Junio C Hamano wrote:\n>>> drafnel@gmail.com writes:\n>>>\n>>>> From: Brandon Casey <drafnel@gmail.com>\n>>>>\n>>>> Signed-off-by: Brandon Casey <drafnel@gmail.com>\n>>>> @@ -306,6 +305,7 @@ BUILT_INS += git-fsck-objects$X\n>>>>  BUILT_INS += git-get-tar-commit-id$X\n>>>>  BUILT_INS += git-init$X\n>>>>  BUILT_INS += git-merge-subtree$X\n>>>> +BUILT_INS += git-mktag$X\n>>>>  BUILT_INS += git-peek-remote$X\n>>>>  BUILT_INS += git-repo-config$X\n>>>>  BUILT_INS += git-show$X\n>>>> @@ -423,6 +423,7 @@ LIB_OBJS += log-tree.o\n>>>>  LIB_OBJS += mailmap.o\n>>>>  LIB_OBJS += match-trees.o\n>>>>  LIB_OBJS += merge-file.o\n>>>> +LIB_OBJS += mktag.o\n>>> This is unusual for a builtin.  Why didn't it migrate to builtin-mktag?\n>> I didn't know how to do it.\n>>\n>> I was trying not to do a code move and a code change at the same time.\n> \n> Why did you not consult Git's own history for guidance?  See e.g.\n\nI did, but not exhaustively. I found only examples of converting a\nshell/perl script to a c version. That is straight-forward.\n\n> \t$ git log next --diff-filter=A builtin-*.c\n\nAnd indeed the first two results here are examples of converting scripts\nto builtin c versions.\n\nThe third result is applicable and converts merge-recursive from a\nstandalone c version to builtin.\n\nIf I had known about (or looked for) the --diff-filter option I would\nhave used the 'R' argument, since I am interested in a rename event,\nbut it produces no results.\n\n\t$ git log next --diff-filter=R builtin-*.c\n\nAdding -M does not help.\n\n-brandon\n"},{"id":"76741","messageId":"7vej87z7p2.fsf@gitster.siamese.dyndns.org","threadId":"13448","inReplyTo":"48285DAB.2040707@nrlssc.navy.mil","subject":"Re: [PATCH 2/5] Make mktag a builtin.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-05-12T18:41:13Z","receivedAt":"2008-05-12T18:41:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Casey <casey@nrlssc.navy.mil> writes:\n\n> I didn't think I should move the non-builtin mktag.c to builtin-mktag.c,\n> and then after I modified mktag to be a builtin I knew I was moving it\n> to builtin-tag.c so I didn't see a point to renaming it.\n\nI see.  I did not realize that the eventual shape would be to have both\nmktag and tag to be in builtin-tag.c, just like builtin-log.c supports\nmany other commands from the log family, and that was where my question\ncame from.\n\nBut I think the arrangement to have both in builtin-tag.c actually makes\nsense --- mktag needs to be kept supported but most of its internal should\nbe shared with tag anyway.\n\n> Also, I decided about those things _before_ I realized how small the changes\n> would be to mktag to make it a builtin.\n>\n> Do you think the modified patch you posted conflicts with the idea that\n> \"code move should be separate from code change\"?\n\nYes, it does, but I do not subscribe to the \"idea\".  Therefor I do not see\nany problem.\n\nIf you were to stop at making mktag a builtin, then the patch I sent would\nbe the change that is necessary to do so.  A code movement can and often\ndoes need some adjustment (e.g. if you move \"a.c\" to \"src/a.c\", its\n'#include \"a.h\"' may need to become '#include \"../a.h\"' (or preferably to\n'#include <a.h>' with appropriate -I.. option in the Makefile).  It does\nnot help anybody to insist on a blanket dogma that forbids modification\nand movement at the same time.\n\nWe do discourage rolling unrelated things in one commit, but creating a\nbuiltin \"foo\" typically involves creation of builtin-foo.c and associated\nchanges to the Makefile and builtin.h, and in this case the initial\ncontents of builtin-mktag.c happens to come from an existing file mktag.c,\nwhile making the original mktag.c obsolete and unnecessary along the way.\nThat is a single logical change, and I do not think there is anything\nwrong to do it in one commit. In fact, splitting such a change into more\nthan one commit is just plain silly and wrong, isn't it?\n"}]}