{"thread":{"id":"36140","subject":"[PATCH] general style: replaces memcmp() with proper starts_with()","startedAt":"2014-03-12T14:43:54Z","lastAt":"2014-03-14T16:56:49Z","messageCount":22,"participants":["Quint Guvernator","Jeff King","Junio C Hamano","David Kastrup","René Scharfe"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"236579","messageId":"1394635434-44979-1-git-send-email-quintus.public@gmail.com","threadId":"36140","inReplyTo":null,"subject":"[PATCH] general style: replaces memcmp() with proper starts_with()","fromName":"Quint Guvernator","fromEmail":"quintus.public@gmail.com","sentAt":"2014-03-12T14:43:54Z","receivedAt":"2014-03-12T14:43:54Z","isPatch":true,"sender":{"key":"quintus.public@gmail.com","avatar":"https://gravatar.com/avatar/0805695585d6191e00badd5057b262b501047d9fb2b22dd2495009ffb25d85fd?d=mp&s=160"},"body":"memcmp() is replaced with negated starts_with() when comparing strings\nfrom the beginning. starts_with() looks nicer and it saves the extra\nargument of the length of the comparing string.\n\nnote: this commit properly handles negation in starts_with()\n\nSigned-off-by: Quint Guvernator \n---\n builtin/apply.c | 10 +++++-----\n builtin/cat-file.c | 2 +-\n builtin/commit-tree.c | 2 +-\n builtin/for-each-ref.c | 2 +-\n builtin/get-tar-commit-id.c | 2 +-\n builtin/mailinfo.c | 10 +++++-----\n builtin/mktag.c | 8 ++++----\n builtin/patch-id.c | 18 +++++++++---------\n commit.c | 18 +++++++++---------\n connect.c | 8 ++++----\n contrib/convert-objects/convert-objects.c | 6 +++---\n convert.c | 2 +-\n credential-cache.c | 2 +-\n fetch-pack.c | 2 +-\n fsck.c | 8 ++++----\n http-walker.c | 4 ++--\n imap-send.c | 6 +++---\n pack-write.c | 2 +-\n path.c | 2 +-\n refs.c | 2 +-\n remote.c | 2 +-\n submodule.c | 2 +-\n transport.c | 2 +-\n 23 files changed, 61 insertions(+), 61 deletions(-)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex a7e72d5..16c20af 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -846,8 +846,8 @@ static int has_epoch_timestamp(const char *nameline)\n \t * YYYY-MM-DD hh:mm:ss must be from either 1969-12-31\n \t * (west of GMT) or 1970-01-01 (east of GMT)\n \t */\n-\tif ((zoneoffset < 0 && memcmp(timestamp, \"1969-12-31\", 10)) ||\n-\t (0 <= zoneoffset && memcmp(timestamp, \"1970-01-01\", 10)))\n+\tif ((zoneoffset < 0 && !starts_with(timestamp, \"1969-12-31\")) ||\n+\t (0 <= zoneoffset && !starts_with(timestamp, \"1970-01-01\")))\n \t\treturn 0;\n \n \thourminute = (strtol(timestamp + 11, NULL, 10) * 60 +\n@@ -1631,7 +1631,7 @@ static int parse_fragment(const char *line, unsigned long size,\n \t\t * l10n of \"\\ No newline...\" is at least that long.\n \t\t */\n \t\tcase '\\\\':\n-\t\t\tif (len < 12 || memcmp(line, \"\\\\ \", 2))\n+\t\t\tif (len < 12 || !starts_with(line, \"\\\\ \"))\n \t\t\t\treturn -1;\n \t\t\tbreak;\n \t\t}\n@@ -1646,7 +1646,7 @@ static int parse_fragment(const char *line, unsigned long size,\n \t * it in the above loop because we hit oldlines == newlines == 0\n \t * before seeing it.\n \t */\n-\tif (12 < size && !memcmp(line, \"\\\\ \", 2))\n+\tif (12 < size && starts_with(line, \"\\\\ \"))\n \t\toffset += linelen(line, size);\n \n \tpatch->lines_added += added;\n@@ -1673,7 +1673,7 @@ static int parse_single_patch(const char *line, unsigned long size, struct patch\n \tunsigned long oldlines = 0, newlines = 0, context = 0;\n \tstruct fragment **fragp = &patch->fragments;\n \n-\twhile (size > 4 && !memcmp(line, \"@@ -\", 4)) {\n+\twhile (size > 4 && starts_with(line, \"@@ -\")) {\n \t\tstruct fragment *fragment;\n \t\tint len;\n \ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex d5a93e0..6266bbb 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -82,7 +82,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name)\n \t\t\t\tenum object_type type;\n \t\t\t\tunsigned long size;\n \t\t\t\tchar *buffer = read_sha1_file(sha1, &type, &size);\n-\t\t\t\tif (memcmp(buffer, \"object \", 7) ||\n+\t\t\t\tif (!starts_with(buffer, \"object \") ||\n \t\t\t\t get_sha1_hex(buffer + 7, blob_sha1))\n \t\t\t\t\tdie(\"%s not a valid tag\", sha1_to_hex(sha1));\n \t\t\t\tfree(buffer);\ndiff --git a/builtin/commit-tree.c b/builtin/commit-tree.c\nindex 987a4c3..2777519 100644\n--- a/builtin/commit-tree.c\n+++ b/builtin/commit-tree.c\n@@ -66,7 +66,7 @@ int cmd_commit_tree(int argc, const char **argv, const char *prefix)\n \t\t\tcontinue;\n \t\t}\n \n-\t\tif (!memcmp(arg, \"-S\", 2)) {\n+\t\tif (starts_with(arg, \"-S\")) {\n \t\t\tsign_commit = arg + 2;\n \t\t\tcontinue;\n \t\t}\ndiff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex 51798b4..fe198fd 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -193,7 +193,7 @@ static int verify_format(const char *format)\n \t\tat = parse_atom(sp + 2, ep);\n \t\tcp = ep + 1;\n \n-\t\tif (!memcmp(used_atom[at], \"color:\", 6))\n+\t\tif (starts_with(used_atom[at], \"color:\"))\n \t\t\tneed_color_reset_at_eol = !!strcmp(used_atom[at], color_reset);\n \t}\n \treturn 0;\ndiff --git a/builtin/get-tar-commit-id.c b/builtin/get-tar-commit-id.c\nindex aa72596..6409c26 100644\n--- a/builtin/get-tar-commit-id.c\n+++ b/builtin/get-tar-commit-id.c\n@@ -29,7 +29,7 @@ int cmd_get_tar_commit_id(int argc, const char **argv, const char *prefix)\n \t\tdie(\"git get-tar-commit-id: read error\");\n \tif (header->typeflag[0] != 'g')\n \t\treturn 1;\n-\tif (memcmp(content, \"52 comment=\", 11))\n+\tif (!starts_with(content, \"52 comment=\"))\n \t\treturn 1;\n \n \tn = write_in_full(1, content + 11, 41);\ndiff --git a/builtin/mailinfo.c b/builtin/mailinfo.c\nindex 2c3cd8e..1e76dac 100644\n--- a/builtin/mailinfo.c\n+++ b/builtin/mailinfo.c\n@@ -626,7 +626,7 @@ static int handle_boundary(void)\n \tstrbuf_addch(&newline, '\\n');\n again:\n \tif (line.len >= (*content_top)->len + 2 &&\n-\t !memcmp(line.buf + (*content_top)->len, \"--\", 2)) {\n+\t starts_with(line.buf + (*content_top)->len, \"--\")) {\n \t\t/* we hit an end boundary */\n \t\t/* pop the current boundary off the stack */\n \t\tstrbuf_release(*content_top);\n@@ -727,8 +727,8 @@ static int is_scissors_line(const struct strbuf *line)\n \t\t\tcontinue;\n \t\t}\n \t\tif (i + 1 < len &&\n-\t\t (!memcmp(buf + i, \">8\", 2) || !memcmp(buf + i, \"8<\", 2) ||\n-\t\t !memcmp(buf + i, \">%\", 2) || !memcmp(buf + i, \"%<\", 2))) {\n+\t\t (starts_with(buf + i, \">8\") || starts_with(buf + i, \"8<\") ||\n+\t\t starts_with(buf + i, \">%\") || starts_with(buf + i, \"%<\"))) {\n \t\t\tin_perforation = 1;\n \t\t\tperforation += 2;\n \t\t\tscissors += 2;\n@@ -929,13 +929,13 @@ static void handle_info(void)\n \t\telse\n \t\t\tcontinue;\n \n-\t\tif (!memcmp(header[i], \"Subject\", 7)) {\n+\t\tif (starts_with(header[i], \"Subject\")) {\n \t\t\tif (!keep_subject) {\n \t\t\t\tcleanup_subject(hdr);\n \t\t\t\tcleanup_space(hdr);\n \t\t\t}\n \t\t\toutput_header_lines(fout, \"Subject\", hdr);\n-\t\t} else if (!memcmp(header[i], \"From\", 4)) {\n+\t\t} else if (starts_with(header[i], \"From\")) {\n \t\t\tcleanup_space(hdr);\n \t\t\thandle_from(hdr);\n \t\t\tfprintf(fout, \"Author: %s\\n\", name.buf);\ndiff --git a/builtin/mktag.c b/builtin/mktag.c\nindex 640ab64..70385ac 100644\n--- a/builtin/mktag.c\n+++ b/builtin/mktag.c\n@@ -49,7 +49,7 @@ static int verify_tag(char *buffer, unsigned long size)\n \n \t/* Verify object line */\n \tobject = buffer;\n-\tif (memcmp(object, \"object \", 7))\n+\tif (!starts_with(object, \"object \"))\n \t\treturn error(\"char%d: does not start with \\\"object \\\"\", 0);\n \n \tif (get_sha1_hex(object + 7, sha1))\n@@ -57,7 +57,7 @@ static int verify_tag(char *buffer, unsigned long size)\n \n \t/* Verify type line */\n \ttype_line = object + 48;\n-\tif (memcmp(type_line - 1, \"\\ntype \", 6))\n+\tif (!starts_with(type_line - 1, \"\\ntype \"))\n \t\treturn error(\"char%d: could not find \\\"\\\\ntype \\\"\", 47);\n \n \t/* Verify tag-line */\n@@ -66,7 +66,7 @@ static int verify_tag(char *buffer, unsigned long size)\n \t\treturn error(\"char%\"PRIuMAX\": could not find next \\\"\\\\n\\\"\",\n \t\t\t\t(uintmax_t) (type_line - buffer));\n \ttag_line++;\n-\tif (memcmp(tag_line, \"tag \", 4) || tag_line[4] == '\\n')\n+\tif (!starts_with(tag_line, \"tag \") || tag_line[4] == '\\n')\n \t\treturn error(\"char%\"PRIuMAX\": no \\\"tag \\\" found\",\n \t\t\t\t(uintmax_t) (tag_line - buffer));\n \n@@ -98,7 +98,7 @@ static int verify_tag(char *buffer, unsigned long size)\n \t/* Verify the tagger line */\n \ttagger_line = tag_line;\n \n-\tif (memcmp(tagger_line, \"tagger \", 7))\n+\tif (!starts_with(tagger_line, \"tagger \"))\n \t\treturn error(\"char%\"PRIuMAX\": could not find \\\"tagger \\\"\",\n \t\t\t(uintmax_t) (tagger_line - buffer));\n \ndiff --git a/builtin/patch-id.c b/builtin/patch-id.c\nindex 3cfe02d..7e02f2c 100644\n--- a/builtin/patch-id.c\n+++ b/builtin/patch-id.c\n@@ -66,13 +66,13 @@ static int get_one_patchid(unsigned char *next_sha1, git_SHA_CTX *ctx, struct st\n \t\tchar *p = line;\n \t\tint len;\n \n-\t\tif (!memcmp(line, \"diff-tree \", 10))\n+\t\tif (starts_with(line, \"diff-tree \"))\n \t\t\tp += 10;\n-\t\telse if (!memcmp(line, \"commit \", 7))\n+\t\telse if (starts_with(line, \"commit \"))\n \t\t\tp += 7;\n-\t\telse if (!memcmp(line, \"From \", 5))\n+\t\telse if (starts_with(line, \"From \"))\n \t\t\tp += 5;\n-\t\telse if (!memcmp(line, \"\\\\ \", 2) && 12 < strlen(line))\n+\t\telse if (starts_with(line, \"\\\\ \") && 12 < strlen(line))\n \t\t\tcontinue;\n \n \t\tif (!get_sha1_hex(p, next_sha1)) {\n@@ -81,14 +81,14 @@ static int get_one_patchid(unsigned char *next_sha1, git_SHA_CTX *ctx, struct st\n \t\t}\n \n \t\t/* Ignore commit comments */\n-\t\tif (!patchlen && memcmp(line, \"diff \", 5))\n+\t\tif (!patchlen && !starts_with(line, \"diff \"))\n \t\t\tcontinue;\n \n \t\t/* Parsing diff header? */\n \t\tif (before == -1) {\n-\t\t\tif (!memcmp(line, \"index \", 6))\n+\t\t\tif (starts_with(line, \"index \"))\n \t\t\t\tcontinue;\n-\t\t\telse if (!memcmp(line, \"--- \", 4))\n+\t\t\telse if (starts_with(line, \"--- \"))\n \t\t\t\tbefore = after = 1;\n \t\t\telse if (!isalpha(line[0]))\n \t\t\t\tbreak;\n@@ -96,14 +96,14 @@ static int get_one_patchid(unsigned char *next_sha1, git_SHA_CTX *ctx, struct st\n \n \t\t/* Looking for a valid hunk header? */\n \t\tif (before == 0 && after == 0) {\n-\t\t\tif (!memcmp(line, \"@@ -\", 4)) {\n+\t\t\tif (starts_with(line, \"@@ -\")) {\n \t\t\t\t/* Parse next hunk, but ignore line numbers. */\n \t\t\t\tscan_hunk_header(line, &before, &after);\n \t\t\t\tcontinue;\n \t\t\t}\n \n \t\t\t/* Split at the end of the patch. */\n-\t\t\tif (memcmp(line, \"diff \", 5))\n+\t\t\tif (!starts_with(line, \"diff \"))\n \t\t\t\tbreak;\n \n \t\t\t/* Else we're parsing another header. */\ndiff --git a/commit.c b/commit.c\nindex 6bf4fe0..0a6ce49 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -90,13 +90,13 @@ static unsigned long parse_commit_date(const char *buf, const char *tail)\n \n \tif (buf + 6 >= tail)\n \t\treturn 0;\n-\tif (memcmp(buf, \"author\", 6))\n+\tif (!starts_with(buf, \"author\"))\n \t\treturn 0;\n \twhile (buf < tail && *buf++ != '\\n')\n \t\t/* nada */;\n \tif (buf + 9 >= tail)\n \t\treturn 0;\n-\tif (memcmp(buf, \"committer\", 9))\n+\tif (!starts_with(buf, \"committer\"))\n \t\treturn 0;\n \twhile (buf < tail && *buf++ != '>')\n \t\t/* nada */;\n@@ -269,7 +269,7 @@ int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long s\n \t\treturn 0;\n \titem->object.parsed = 1;\n \ttail += size;\n-\tif (tail <= bufptr + 46 || memcmp(bufptr, \"tree \", 5) || bufptr[45] != '\\n')\n+\tif (tail <= bufptr + 46 || !starts_with(bufptr, \"tree \") || bufptr[45] != '\\n')\n \t\treturn error(\"bogus commit object %s\", sha1_to_hex(item->object.sha1));\n \tif (get_sha1_hex(bufptr + 5, parent) < 0)\n \t\treturn error(\"bad tree pointer in commit %s\",\n@@ -279,7 +279,7 @@ int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long s\n \tpptr = &item->parents;\n \n \tgraft = lookup_commit_graft(item->object.sha1);\n-\twhile (bufptr + 48 < tail && !memcmp(bufptr, \"parent \", 7)) {\n+\twhile (bufptr + 48 < tail && starts_with(bufptr, \"parent \")) {\n \t\tstruct commit *new_parent;\n \n \t\tif (tail <= bufptr + 48 ||\n@@ -1279,11 +1279,11 @@ struct commit_extra_header *read_commit_extra_headers(struct commit *commit,\n \n static inline int standard_header_field(const char *field, size_t len)\n {\n-\treturn ((len == 4 && !memcmp(field, \"tree \", 5)) ||\n-\t\t(len == 6 && !memcmp(field, \"parent \", 7)) ||\n-\t\t(len == 6 && !memcmp(field, \"author \", 7)) ||\n-\t\t(len == 9 && !memcmp(field, \"committer \", 10)) ||\n-\t\t(len == 8 && !memcmp(field, \"encoding \", 9)));\n+\treturn ((len == 4 && starts_with(field, \"tree \")) ||\n+\t\t(len == 6 && starts_with(field, \"parent \")) ||\n+\t\t(len == 6 && starts_with(field, \"author \")) ||\n+\t\t(len == 9 && starts_with(field, \"committer \")) ||\n+\t\t(len == 8 && starts_with(field, \"encoding \")));\n }\n \n static int excluded_header_field(const char *field, size_t len, const char **exclude)\ndiff --git a/connect.c b/connect.c\nindex 4150412..ce47631 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -18,7 +18,7 @@ static int check_ref(const char *name, int len, unsigned int flags)\n \tif (!flags)\n \t\treturn 1;\n \n-\tif (len < 5 || memcmp(name, \"refs/\", 5))\n+\tif (len < 5 || !starts_with(name, \"refs/\"))\n \t\treturn 0;\n \n \t/* Skip the \"refs/\" part */\n@@ -30,11 +30,11 @@ static int check_ref(const char *name, int len, unsigned int flags)\n \t\treturn 0;\n \n \t/* REF_HEADS means that we want regular branch heads */\n-\tif ((flags & REF_HEADS) && !memcmp(name, \"heads/\", 6))\n+\tif ((flags & REF_HEADS) && starts_with(name, \"heads/\"))\n \t\treturn 1;\n \n \t/* REF_TAGS means that we want tags */\n-\tif ((flags & REF_TAGS) && !memcmp(name, \"tags/\", 5))\n+\tif ((flags & REF_TAGS) && starts_with(name, \"tags/\"))\n \t\treturn 1;\n \n \t/* All type bits clear means that we are ok with anything */\n@@ -514,7 +514,7 @@ static int git_proxy_command_options(const char *var, const char *value,\n \t\tif (0 <= matchlen) {\n \t\t\t/* core.gitproxy = none for kernel.org */\n \t\t\tif (matchlen == 4 &&\n-\t\t\t !memcmp(value, \"none\", 4))\n+\t\t\t starts_with(value, \"none\"))\n \t\t\t\tmatchlen = 0;\n \t\t\tgit_proxy_command = xmemdupz(value, matchlen);\n \t\t}\ndiff --git a/contrib/convert-objects/convert-objects.c b/contrib/convert-objects/convert-objects.c\nindex f3b57bf..9fdc730 100644\n--- a/contrib/convert-objects/convert-objects.c\n+++ b/contrib/convert-objects/convert-objects.c\n@@ -245,7 +245,7 @@ static void convert_date(void *buffer, unsigned long size, unsigned char *result\n \tsize -= 46;\n \n \t/* \"parent \\n\" */\n-\twhile (!memcmp(buffer, \"parent \", 7)) {\n+\twhile (starts_with(buffer, \"parent \")) {\n \t\tmemcpy(new + newlen, buffer, 48);\n \t\tnewlen += 48;\n \t\tbuffer = (char *) buffer + 48;\n@@ -270,11 +270,11 @@ static void convert_commit(void *buffer, unsigned long size, unsigned char *resu\n \tvoid *orig_buffer = buffer;\n \tunsigned long orig_size = size;\n \n-\tif (memcmp(buffer, \"tree \", 5))\n+\tif (!starts_with(buffer, \"tree \"))\n \t\tdie(\"Bad commit '%s'\", (char *) buffer);\n \tconvert_ascii_sha1((char *) buffer + 5);\n \tbuffer = (char *) buffer + 46; /* \"tree \" + \"hex sha1\" + \"\\n\" */\n-\twhile (!memcmp(buffer, \"parent \", 7)) {\n+\twhile (starts_with(buffer, \"parent \")) {\n \t\tconvert_ascii_sha1((char *) buffer + 7);\n \t\tbuffer = (char *) buffer + 48;\n \t}\ndiff --git a/convert.c b/convert.c\nindex ab80b72..30882e2 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -543,7 +543,7 @@ static int ident_to_git(const char *path, const char *src, size_t len,\n \t\tlen -= dollar + 1 - src;\n \t\tsrc = dollar + 1;\n \n-\t\tif (len > 3 && !memcmp(src, \"Id:\", 3)) {\n+\t\tif (len > 3 && starts_with(src, \"Id:\")) {\n \t\t\tdollar = memchr(src + 3, '$', len - 3);\n \t\t\tif (!dollar)\n \t\t\t\tbreak;\ndiff --git a/credential-cache.c b/credential-cache.c\nindex 9a03792..23b153c 100644\n--- a/credential-cache.c\n+++ b/credential-cache.c\n@@ -54,7 +54,7 @@ static void spawn_daemon(const char *socket)\n \tr = read_in_full(daemon.out, buf, sizeof(buf));\n \tif (r < 0)\n \t\tdie_errno(\"unable to read result code from cache daemon\");\n-\tif (r != 3 || memcmp(buf, \"ok\\n\", 3))\n+\tif (r != 3 || !starts_with(buf, \"ok\\n\"))\n \t\tdie(\"cache daemon did not start: %.*s\", r, buf);\n \tclose(daemon.out);\n }\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex f061f1f..17823ab 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -506,7 +506,7 @@ static void filter_refs(struct fetch_pack_args *args,\n \t\tint keep = 0;\n \t\tnext = ref->next;\n \n-\t\tif (!memcmp(ref->name, \"refs/\", 5) &&\n+\t\tif (starts_with(ref->name, \"refs/\") &&\n \t\t check_refname_format(ref->name, 0))\n \t\t\t; /* trash */\n \t\telse {\ndiff --git a/fsck.c b/fsck.c\nindex 99c0497..3bd69da 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -290,12 +290,12 @@ static int fsck_commit(struct commit *commit, fsck_error error_func)\n \tif (commit->date == ULONG_MAX)\n \t\treturn error_func(&commit->object, FSCK_ERROR, \"invalid author/committer line\");\n \n-\tif (memcmp(buffer, \"tree \", 5))\n+\tif (!starts_with(buffer, \"tree \"))\n \t\treturn error_func(&commit->object, FSCK_ERROR, \"invalid format - expected 'tree' line\");\n \tif (get_sha1_hex(buffer+5, tree_sha1) || buffer[45] != '\\n')\n \t\treturn error_func(&commit->object, FSCK_ERROR, \"invalid 'tree' line format - bad sha1\");\n \tbuffer += 46;\n-\twhile (!memcmp(buffer, \"parent \", 7)) {\n+\twhile (starts_with(buffer, \"parent \")) {\n \t\tif (get_sha1_hex(buffer+7, sha1) || buffer[47] != '\\n')\n \t\t\treturn error_func(&commit->object, FSCK_ERROR, \"invalid 'parent' line format - bad sha1\");\n \t\tbuffer += 48;\n@@ -322,13 +322,13 @@ static int fsck_commit(struct commit *commit, fsck_error error_func)\n \t\tif (p || parents)\n \t\t\treturn error_func(&commit->object, FSCK_ERROR, \"parent objects missing\");\n \t}\n-\tif (memcmp(buffer, \"author \", 7))\n+\tif (!starts_with(buffer, \"author \"))\n \t\treturn error_func(&commit->object, FSCK_ERROR, \"invalid format - expected 'author' line\");\n \tbuffer += 7;\n \terr = fsck_ident(&buffer, &commit->object, error_func);\n \tif (err)\n \t\treturn err;\n-\tif (memcmp(buffer, \"committer \", strlen(\"committer \")))\n+\tif (!starts_with(buffer, \"committer \"))\n \t\treturn error_func(&commit->object, FSCK_ERROR, \"invalid format - expected 'committer' line\");\n \tbuffer += strlen(\"committer \");\n \terr = fsck_ident(&buffer, &commit->object, error_func);\ndiff --git a/http-walker.c b/http-walker.c\nindex 1516c5e..9a8d412 100644\n--- a/http-walker.c\n+++ b/http-walker.c\n@@ -245,7 +245,7 @@ static void process_alternates_response(void *callback_data)\n \t\t\t\t\t\t - base);\n \t\t\t\t\tokay = 1;\n \t\t\t\t}\n-\t\t\t} else if (!memcmp(data + i, \"../\", 3)) {\n+\t\t\t} else if (starts_with(data + i, \"../\")) {\n \t\t\t\t/*\n \t\t\t\t * Relative URL; chop the corresponding\n \t\t\t\t * number of subpath from base (and ../\n@@ -267,7 +267,7 @@ static void process_alternates_response(void *callback_data)\n \t\t\t\ti += 3;\n \t\t\t\tserverlen = strlen(base);\n \t\t\t\twhile (i + 2 < posn &&\n-\t\t\t\t !memcmp(data + i, \"../\", 3)) {\n+\t\t\t\t starts_with(data + i, \"../\")) {\n \t\t\t\t\tdo {\n \t\t\t\t\t\tserverlen--;\n \t\t\t\t\t} while (serverlen &&\ndiff --git a/imap-send.c b/imap-send.c\nindex 0bc6f7f..019de18 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -524,7 +524,7 @@ static struct imap_cmd *v_issue_imap_cmd(struct imap_store *ctx,\n \tif (Verbose) {\n \t\tif (imap->num_in_progress)\n \t\t\tprintf(\"(%d in progress) \", imap->num_in_progress);\n-\t\tif (memcmp(cmd->cmd, \"LOGIN\", 5))\n+\t\tif (!starts_with(cmd->cmd, \"LOGIN\"))\n \t\t\tprintf(\">>> %s\", buf);\n \t\telse\n \t\t\tprintf(\">>> %d LOGIN \\n\", cmd->tag);\n@@ -802,7 +802,7 @@ static int get_cmd_result(struct imap_store *ctx, struct imap_cmd *tcmd)\n \t\t\t\tresp = DRV_OK;\n \t\t\telse {\n \t\t\t\tif (!strcmp(\"NO\", arg)) {\n-\t\t\t\t\tif (cmdp->cb.create && cmd && (cmdp->cb.trycreate || !memcmp(cmd, \"[TRYCREATE]\", 11))) { /* SELECT, APPEND or UID COPY */\n+\t\t\t\t\tif (cmdp->cb.create && cmd && (cmdp->cb.trycreate || starts_with(cmd, \"[TRYCREATE]\"))) { /* SELECT, APPEND or UID COPY */\n \t\t\t\t\t\tp = strchr(cmdp->cmd, '\"');\n \t\t\t\t\t\tif (!issue_imap_cmd(ctx, NULL, \"CREATE \\\"%.*s\\\"\", (int)(strchr(p + 1, '\"') - p + 1), p)) {\n \t\t\t\t\t\t\tresp = RESP_BAD;\n@@ -827,7 +827,7 @@ static int get_cmd_result(struct imap_store *ctx, struct imap_cmd *tcmd)\n \t\t\t\t} else /*if (!strcmp(\"BAD\", arg))*/\n \t\t\t\t\tresp = RESP_BAD;\n \t\t\t\tfprintf(stderr, \"IMAP command '%s' returned response (%s) - %s\\n\",\n-\t\t\t\t\t memcmp(cmdp->cmd, \"LOGIN\", 5) ?\n+\t\t\t\t\t !starts_with(cmdp->cmd, \"LOGIN\") ?\n \t\t\t\t\t\t\tcmdp->cmd : \"LOGIN \",\n \t\t\t\t\t\t\targ, cmd ? cmd : \"\");\n \t\t\t}\ndiff --git a/pack-write.c b/pack-write.c\nindex 9b8308b..c3bfa16 100644\n--- a/pack-write.c\n+++ b/pack-write.c\n@@ -289,7 +289,7 @@ char *index_pack_lockfile(int ip_out)\n \t * later on. If we don't get that then tough luck with it.\n \t */\n \tif (read_in_full(ip_out, packname, 46) == 46 && packname[45] == '\\n' &&\n-\t memcmp(packname, \"keep\\t\", 5) == 0) {\n+\t !starts_with(packname, \"keep\\t\") == 0) {\n \t\tchar path[PATH_MAX];\n \t\tpackname[45] = 0;\n \t\tsnprintf(path, sizeof(path), \"%s/pack/pack-%s.keep\",\ndiff --git a/path.c b/path.c\nindex f9c5062..b4b07d6 100644\n--- a/path.c\n+++ b/path.c\n@@ -26,7 +26,7 @@ static char *get_pathname(void)\n static char *cleanup_path(char *path)\n {\n \t/* Clean it up */\n-\tif (!memcmp(path, \"./\", 2)) {\n+\tif (starts_with(path, \"./\")) {\n \t\tpath += 2;\n \t\twhile (*path == '/')\n \t\t\tpath++;\ndiff --git a/refs.c b/refs.c\nindex 89228e2..d167dfd 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -63,7 +63,7 @@ static int check_refname_component(const char *refname, int flags)\n \t\tif (refname[1] == '\\0')\n \t\t\treturn -1; /* Component equals \".\". */\n \t}\n-\tif (cp - refname >= 5 && !memcmp(cp - 5, \".lock\", 5))\n+\tif (cp - refname >= 5 && starts_with(cp - 5, \".lock\"))\n \t\treturn -1; /* Refname ends with \".lock\". */\n \treturn cp - refname;\n }\ndiff --git a/remote.c b/remote.c\nindex e41251e..8a03b4a 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1144,7 +1144,7 @@ static int match_explicit(struct ref *src, struct ref *dst,\n \tcase 1:\n \t\tbreak;\n \tcase 0:\n-\t\tif (!memcmp(dst_value, \"refs/\", 5))\n+\t\tif (starts_with(dst_value, \"refs/\"))\n \t\t\tmatched_dst = make_linked_ref(dst_value, dst_tail);\n \t\telse if (is_null_sha1(matched_src->new_sha1))\n \t\t\terror(\"unable to delete '%s': remote ref does not exist\",\ndiff --git a/submodule.c b/submodule.c\nindex b80ecac..3de9ae5 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -203,7 +203,7 @@ void gitmodules_config(void)\n \t\t\tif (active_nr > pos) { /* there is a .gitmodules */\n \t\t\t\tconst struct cache_entry *ce = active_cache[pos];\n \t\t\t\tif (ce_namelen(ce) == 11 &&\n-\t\t\t\t !memcmp(ce->name, \".gitmodules\", 11))\n+\t\t\t\t starts_with(ce->name, \".gitmodules\"))\n \t\t\t\t\tgitmodules_is_unmerged = 1;\n \t\t\t}\n \t\t} else if (pos < active_nr) {\ndiff --git a/transport.c b/transport.c\nindex ca7bb44..a267822 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -1364,7 +1364,7 @@ static int refs_from_alternate_cb(struct alternate_object_database *e,\n \n \twhile (other[len-1] == '/')\n \t\tother[--len] = '\\0';\n-\tif (len < 8 || memcmp(other + len - 8, \"/objects\", 8))\n+\tif (len < 8 || !starts_with(other + len - 8, \"/objects\"))\n \t\treturn 0;\n \t/* Is this a git repository with refs? */\n \tmemcpy(other + len - 8, \"/refs\", 6);\n-- \n1.9.0\n"},{"id":"236588","messageId":"20140312175624.GA7982@sigill.intra.peff.net","threadId":"36140","inReplyTo":"1394635434-44979-1-git-send-email-quintus.public@gmail.com","subject":"Re: [PATCH] general style: replaces memcmp() with proper starts_with()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-12T17:56:24Z","receivedAt":"2014-03-12T17:56:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 12, 2014 at 10:43:54AM -0400, Quint Guvernator wrote:\n\n> memcmp() is replaced with negated starts_with() when comparing strings\n> from the beginning. starts_with() looks nicer and it saves the extra\n> argument of the length of the comparing string.\n\nThanks, I think this is a real readability improvement in most cases.\n\nOne of the things to do when reviewing a patch like this is make sure\nthat there aren't any silly arithmetic mistakes. Checking that can be\ntedious, so I thought about how _I_ would do it programatically, and\nthen compared our results.\n\nI tried:\n\n perl -i -lpe '\n s/memcmp\\(([^,]+), \"(.*?)\", (\\d+)\\)/\n length($2) == $3 ?\n qq{!starts_with($1, \"$2\")} :\n $&\n /ge\n ' $(git ls-files '*.c')\n\nThat comes up with almost the same results as yours, but misses a few\ncases that you caught (in addition to the need to simplify\n!!starts_with()):\n\n - memcmp(foo, \"bar\", strlen(\"bar\"))\n\n - strings with interpolation, like \"foo\\n\", which is actually 4\n characters in length, not 5.\n\nBut there were only a few of those, and I hand-verified them. So I feel\npretty good that the changes are all correct transformations.\n\nThat leaves the question of whether they are all improvements in\nreadability (more on that below).\n\n> note: this commit properly handles negation in starts_with()\n> \n> Signed-off-by: Quint Guvernator \n> ---\n\nThis note should go after the \"---\" line so that it is not part of the\ncommit message (it is only interesting to people seeing v2 and wondering\nwhat changed from v1 earlier on the list, not people reading the history\nmuch later).\n\n> diff --git a/builtin/commit-tree.c b/builtin/commit-tree.c\n> index 987a4c3..2777519 100644\n> --- a/builtin/commit-tree.c\n> +++ b/builtin/commit-tree.c\n> @@ -66,7 +66,7 @@ int cmd_commit_tree(int argc, const char **argv, const char *prefix)\n> \t\t\tcontinue;\n> \t\t}\n> \n> -\t\tif (!memcmp(arg, \"-S\", 2)) {\n> +\t\tif (starts_with(arg, \"-S\")) {\n> \t\t\tsign_commit = arg + 2;\n> \t\t\tcontinue;\n> \t\t}\n\nThis is an improvement, but we still have the magic \"2\" below. Would\nskip_prefix be a better match here, like:\n\n if ((val = skip_prefix(arg, \"-S\"))) {\n sign_commit = val;\n continue;\n }\n\nWe've also talked about refactoring skip_prefix to return a boolean, and\nput the value in an out-parameter. Which would make this even more\nreadable:\n\n if (skip_prefix(arg, \"-S\", &sign_commit))\n continue;\n\nMaybe the time has come to do that.\n\n> --- a/builtin/mailinfo.c\n> +++ b/builtin/mailinfo.c\n> @@ -626,7 +626,7 @@ static int handle_boundary(void)\n> \tstrbuf_addch(&newline, '\\n');\n> again:\n> \tif (line.len >= (*content_top)->len + 2 &&\n> -\t !memcmp(line.buf + (*content_top)->len, \"--\", 2)) {\n> +\t starts_with(line.buf + (*content_top)->len, \"--\")) {\n\nI'm not sure if this improves readability or not. It's certainly nice to\nget rid of the magic \"2\", but starts_with is a bit of a misnomer, since\nwe are indexing deep into the buffer anyway. And we still have the magic\n\"2\" above anyway.\n\nI'm on the fence.\n\n> @@ -727,8 +727,8 @@ static int is_scissors_line(const struct strbuf *line)\n> \t\t\tcontinue;\n> \t\t}\n> \t\tif (i + 1 < len &&\n> -\t\t (!memcmp(buf + i, \">8\", 2) || !memcmp(buf + i, \"8<\", 2) ||\n> -\t\t !memcmp(buf + i, \">%\", 2) || !memcmp(buf + i, \"%<\", 2))) {\n> +\t\t (starts_with(buf + i, \">8\") || starts_with(buf + i, \"8<\") ||\n> +\t\t starts_with(buf + i, \">%\") || starts_with(buf + i, \"%<\"))) {\n\nSame as above.\n\n> @@ -66,7 +66,7 @@ static int verify_tag(char *buffer, unsigned long size)\n> \t\treturn error(\"char%\"PRIuMAX\": could not find next \\\"\\\\n\\\"\",\n> \t\t\t\t(uintmax_t) (type_line - buffer));\n> \ttag_line++;\n> -\tif (memcmp(tag_line, \"tag \", 4) || tag_line[4] == '\\n')\n> +\tif (!starts_with(tag_line, \"tag \") || tag_line[4] == '\\n')\n> \t\treturn error(\"char%\"PRIuMAX\": no \\\"tag \\\" found\",\n> \t\t\t\t(uintmax_t) (tag_line - buffer));\n\nI think this is another that could benefit from an enhanced skip_prefix:\n\n if (!skip_prefix(tag_line, \"tag \", &tag_name) || *tag_name == '\\n')\n ...\n\nand then we can get rid of the \"tag_line += 4\" that is used much later\n(in fact, I suspect that whole function could be improved in this\nrespect).\n\n> @@ -269,7 +269,7 @@ int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long s\n> \t\treturn 0;\n> \titem->object.parsed = 1;\n> \ttail += size;\n> -\tif (tail <= bufptr + 46 || memcmp(bufptr, \"tree \", 5) || bufptr[45] != '\\n')\n> +\tif (tail <= bufptr + 46 || !starts_with(bufptr, \"tree \") || bufptr[45] != '\\n')\n> \t\treturn error(\"bogus commit object %s\", sha1_to_hex(item->object.sha1));\n> \tif (get_sha1_hex(bufptr + 5, parent) < 0)\n\nAgain, we just use \"bufptr + 5\" a bit later here. So a candidate for\nskip_prefix.\n\n> \tgraft = lookup_commit_graft(item->object.sha1);\n> -\twhile (bufptr + 48 < tail && !memcmp(bufptr, \"parent \", 7)) {\n> +\twhile (bufptr + 48 < tail && starts_with(bufptr, \"parent \")) {\n\nDitto here.\n\n> static inline int standard_header_field(const char *field, size_t len)\n> {\n> -\treturn ((len == 4 && !memcmp(field, \"tree \", 5)) ||\n> -\t\t(len == 6 && !memcmp(field, \"parent \", 7)) ||\n> -\t\t(len == 6 && !memcmp(field, \"author \", 7)) ||\n> -\t\t(len == 9 && !memcmp(field, \"committer \", 10)) ||\n> -\t\t(len == 8 && !memcmp(field, \"encoding \", 9)));\n> +\treturn ((len == 4 && starts_with(field, \"tree \")) ||\n> +\t\t(len == 6 && starts_with(field, \"parent \")) ||\n> +\t\t(len == 6 && starts_with(field, \"author \")) ||\n> +\t\t(len == 9 && starts_with(field, \"committer \")) ||\n> +\t\t(len == 8 && starts_with(field, \"encoding \")));\n\nThese extra \"len\" checks are interesting. They look like an attempt to\noptimize lookup, since the caller will already have scanned forward to\nthe space. I wonder if that makes a measurable difference, or is simply\npremature optimization.\n\n> @@ -514,7 +514,7 @@ static int git_proxy_command_options(const char *var, const char *value,\n> \t\tif (0 <= matchlen) {\n> \t\t\t/* core.gitproxy = none for kernel.org */\n> \t\t\tif (matchlen == 4 &&\n> -\t\t\t !memcmp(value, \"none\", 4))\n> +\t\t\t starts_with(value, \"none\"))\n\nWhile technically correct, I'm not sure starts_with really expresses\nwhat's going on here. We're matching a full string, but we cannot use\nstrcmp because the buffer isn't terminated. I'd think a more readable\nalternative would be something like:\n\n int mem_equals(const char *buf, size_t len, const char *match)\n {\n int match_len = strlen(match);\n return len == match_len && !memcmp(buf, match, len);\n }\n\n ...\n\n if (mem_equals(value, matchlen, \"none\"))\n ...\n\nThat might also apply to some other cases (e.g., the case above with\nstandard_header_field). The name mem_equals might not be descriptive\nenough, but hopefully you get the idea.\n\n> --- a/credential-cache.c\n> +++ b/credential-cache.c\n> @@ -54,7 +54,7 @@ static void spawn_daemon(const char *socket)\n> \tr = read_in_full(daemon.out, buf, sizeof(buf));\n> \tif (r < 0)\n> \t\tdie_errno(\"unable to read result code from cache daemon\");\n> -\tif (r != 3 || memcmp(buf, \"ok\\n\", 3))\n> +\tif (r != 3 || !starts_with(buf, \"ok\\n\"))\n> \t\tdie(\"cache daemon did not start: %.*s\", r, buf);\n\nAnother mem_equals case?\n\n> \tclose(daemon.out);\n> }\n> diff --git a/fetch-pack.c b/fetch-pack.c\n> index f061f1f..17823ab 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -506,7 +506,7 @@ static void filter_refs(struct fetch_pack_args *args,\n> \t\tint keep = 0;\n> \t\tnext = ref->next;\n> \n> -\t\tif (!memcmp(ref->name, \"refs/\", 5) &&\n> +\t\tif (starts_with(ref->name, \"refs/\") &&\n> \t\t check_refname_format(ref->name, 0))\n> \t\t\t; /* trash */\n> \t\telse {\n> diff --git a/fsck.c b/fsck.c\n> index 99c0497..3bd69da 100644\n> --- a/fsck.c\n> +++ b/fsck.c\n> @@ -290,12 +290,12 @@ static int fsck_commit(struct commit *commit, fsck_error error_func)\n> \tif (commit->date == ULONG_MAX)\n> \t\treturn error_func(&commit->object, FSCK_ERROR, \"invalid author/committer line\");\n> \n> -\tif (memcmp(buffer, \"tree \", 5))\n> +\tif (!starts_with(buffer, \"tree \"))\n> \t\treturn error_func(&commit->object, FSCK_ERROR, \"invalid format - expected 'tree' line\");\n> \tif (get_sha1_hex(buffer+5, tree_sha1) || buffer[45] != '\\n')\n\nAnother skip_prefix case.\n\n> -\twhile (!memcmp(buffer, \"parent \", 7)) {\n> +\twhile (starts_with(buffer, \"parent \")) {\n> \t\tif (get_sha1_hex(buffer+7, sha1) || buffer[47] != '\\n')\n\nDitto.\n\n> -\tif (memcmp(buffer, \"author \", 7))\n> +\tif (!starts_with(buffer, \"author \"))\n> \t\treturn error_func(&commit->object, FSCK_ERROR, \"invalid format - expected 'author' line\");\n> \tbuffer += 7;\n\nDitto.\n\n> -\tif (memcmp(buffer, \"committer \", strlen(\"committer \")))\n> +\tif (!starts_with(buffer, \"committer \"))\n> \t\treturn error_func(&commit->object, FSCK_ERROR, \"invalid format - expected 'committer' line\");\n> \tbuffer += strlen(\"committer \");\n\nDitto.\n\n> diff --git a/http-walker.c b/http-walker.c\n> index 1516c5e..9a8d412 100644\n> --- a/http-walker.c\n> +++ b/http-walker.c\n> @@ -245,7 +245,7 @@ static void process_alternates_response(void *callback_data)\n> \t\t\t\t\t\t - base);\n> \t\t\t\t\tokay = 1;\n> \t\t\t\t}\n> -\t\t\t} else if (!memcmp(data + i, \"../\", 3)) {\n> +\t\t\t} else if (starts_with(data + i, \"../\")) {\n> \t\t\t\t/*\n> \t\t\t\t * Relative URL; chop the corresponding\n> \t\t\t\t * number of subpath from base (and ../\n> @@ -267,7 +267,7 @@ static void process_alternates_response(void *callback_data)\n> \t\t\t\ti += 3;\n\nAnother skip_prefix?\n\n> --- a/path.c\n> +++ b/path.c\n> @@ -26,7 +26,7 @@ static char *get_pathname(void)\n> static char *cleanup_path(char *path)\n> {\n> \t/* Clean it up */\n> -\tif (!memcmp(path, \"./\", 2)) {\n> +\tif (starts_with(path, \"./\")) {\n> \t\tpath += 2;\n\nAnother skip_prefix.\n\n> @@ -63,7 +63,7 @@ static int check_refname_component(const char *refname, int flags)\n> \t\tif (refname[1] == '\\0')\n> \t\t\treturn -1; /* Component equals \".\". */\n> \t}\n> -\tif (cp - refname >= 5 && !memcmp(cp - 5, \".lock\", 5))\n> +\tif (cp - refname >= 5 && starts_with(cp - 5, \".lock\"))\n> \t\treturn -1; /* Refname ends with \".lock\". */\n\nShould this actually become ends_with? I guess it needs refname to be\nNUL-terminated, though, which it is not. I don't know if it is worth\nhaving a mem_ends_with variant, like:\n\n if (mem_ends_with(refname, cp - refname, \".lock\"))\n ...\n\n> diff --git a/submodule.c b/submodule.c\n> index b80ecac..3de9ae5 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -203,7 +203,7 @@ void gitmodules_config(void)\n> \t\t\tif (active_nr > pos) { /* there is a .gitmodules */\n> \t\t\t\tconst struct cache_entry *ce = active_cache[pos];\n> \t\t\t\tif (ce_namelen(ce) == 11 &&\n> -\t\t\t\t !memcmp(ce->name, \".gitmodules\", 11))\n> +\t\t\t\t starts_with(ce->name, \".gitmodules\"))\n> \t\t\t\t\tgitmodules_is_unmerged = 1;\n\nJens singled this one out earlier, and I agree. I think this is a\n\"mem_equals\".\n\n> diff --git a/transport.c b/transport.c\n> index ca7bb44..a267822 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -1364,7 +1364,7 @@ static int refs_from_alternate_cb(struct alternate_object_database *e,\n> \n> \twhile (other[len-1] == '/')\n> \t\tother[--len] = '\\0';\n> -\tif (len < 8 || memcmp(other + len - 8, \"/objects\", 8))\n> +\tif (len < 8 || !starts_with(other + len - 8, \"/objects\"))\n> \t\treturn 0;\n\nThis is another ends_with candidate (maybe mem_ends_with? I didn't\ncheck).\n\nPhew. That's a lot of changes. The ones I didn't mention are all\nhands-down readability improvements, though I suspect some of them may\nalso be skip_prefix candidates (I didn't investigate each one, but only\nmentioned the ones where I saw the obvious magic number in the diff\ncontext).\n\nI think we can take most of them as-is, though, and perhaps even some of\nthe ones I mentioned (even if skip_prefix can be used, switching to\nstarts_with may be an intermediate improvement).\n\nIf you don't feel like going further on this topic, that's OK, but I\nthink with a few more helpers we could really further clean up some of\nthese callsites.\n\n-Peff\n"},{"id":"236610","messageId":"xmqqiorjky0a.fsf@gitster.dls.corp.google.com","threadId":"36140","inReplyTo":"20140312175624.GA7982@sigill.intra.peff.net","subject":"Re: [PATCH] general style: replaces memcmp() with proper starts_with()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-12T19:39:01Z","receivedAt":"2014-03-12T19:39:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> static inline int standard_header_field(const char *field, size_t len)\n>> {\n>> -\treturn ((len == 4 && !memcmp(field, \"tree \", 5)) ||\n>> -\t\t(len == 6 && !memcmp(field, \"parent \", 7)) ||\n>> -\t\t(len == 6 && !memcmp(field, \"author \", 7)) ||\n>> -\t\t(len == 9 && !memcmp(field, \"committer \", 10)) ||\n>> -\t\t(len == 8 && !memcmp(field, \"encoding \", 9)));\n>> +\treturn ((len == 4 && starts_with(field, \"tree \")) ||\n>> +\t\t(len == 6 && starts_with(field, \"parent \")) ||\n>> +\t\t(len == 6 && starts_with(field, \"author \")) ||\n>> +\t\t(len == 9 && starts_with(field, \"committer \")) ||\n>> +\t\t(len == 8 && starts_with(field, \"encoding \")));\n>\n> These extra \"len\" checks are interesting. They look like an attempt to\n> optimize lookup, since the caller will already have scanned forward to\n> the space.\n\nIf one really wants to remove the magic constants from this, then\none must take advantage of the pattern\n\n\tlen == strlen(S) - 1 && !memcmp(field, S, strlen(S))\n\nthat appears here, and come up with a simple abstraction to express\nthat we are only using the string S (e.g. \"tree \"), length len and\nlocation field of the counted string.\n\nBlindly replacing starts_with() with !memcmp() in the above part is\na readability regression otherwise.\n\n> ... I\n> think with a few more helpers we could really further clean up some of\n> these callsites.\n\nYes.\n"},{"id":"236612","messageId":"20140312194943.GA2912@sigill.intra.peff.net","threadId":"36140","inReplyTo":"xmqqiorjky0a.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] general style: replaces memcmp() with proper starts_with()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-12T19:49:44Z","receivedAt":"2014-03-12T19:49:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 12, 2014 at 12:39:01PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> >> static inline int standard_header_field(const char *field, size_t len)\n> >> {\n> >> -\treturn ((len == 4 && !memcmp(field, \"tree \", 5)) ||\n> >> -\t\t(len == 6 && !memcmp(field, \"parent \", 7)) ||\n> >> -\t\t(len == 6 && !memcmp(field, \"author \", 7)) ||\n> >> -\t\t(len == 9 && !memcmp(field, \"committer \", 10)) ||\n> >> -\t\t(len == 8 && !memcmp(field, \"encoding \", 9)));\n> >> +\treturn ((len == 4 && starts_with(field, \"tree \")) ||\n> >> +\t\t(len == 6 && starts_with(field, \"parent \")) ||\n> >> +\t\t(len == 6 && starts_with(field, \"author \")) ||\n> >> +\t\t(len == 9 && starts_with(field, \"committer \")) ||\n> >> +\t\t(len == 8 && starts_with(field, \"encoding \")));\n> >\n> > These extra \"len\" checks are interesting. They look like an attempt to\n> > optimize lookup, since the caller will already have scanned forward to\n> > the space.\n> \n> If one really wants to remove the magic constants from this, then\n> one must take advantage of the pattern\n> \n> \tlen == strlen(S) - 1 && !memcmp(field, S, strlen(S))\n> \n> that appears here, and come up with a simple abstraction to express\n> that we are only using the string S (e.g. \"tree \"), length len and\n> location field of the counted string.\n> \n> Blindly replacing starts_with() with !memcmp() in the above part is\n> a readability regression otherwise.\n\nI actually think the right solution is:\n\n static inline int standard_header_field(const char *field, size_t len)\n {\n return mem_equals(field, len, \"tree \") ||\n mem_equals(field, len, \"parent \") ||\n ...;\n }\n\nand the caller should tell us it's OK to look at field[len]:\n\n standard_header_field(line, eof - line + 1)\n\nWe could also omit the space from the standard_header_field. The caller\njust ran strchr() looking for the space, so we know that either it is\nthere, or we are at the end of the line/buffer. Arguably a string like\n\"parent\\n\" should be \"a parent header with no data\" (but right now it is\nnot matched by this function). I'm not aware of an implementation that\nwrites such a thing, but it seems to fall in the \"be liberal in what you\naccept\" category.\n\n-Peff\n"},{"id":"236621","messageId":"xmqq61njkwnw.fsf@gitster.dls.corp.google.com","threadId":"36140","inReplyTo":"20140312194943.GA2912@sigill.intra.peff.net","subject":"Re: [PATCH] general style: replaces memcmp() with proper starts_with()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-12T20:08:03Z","receivedAt":"2014-03-12T20:08:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> Blindly replacing starts_with() with !memcmp() in the above part is\n>> a readability regression otherwise.\n>\n> I actually think the right solution is:\n>\n> static inline int standard_header_field(const char *field, size_t len)\n> {\n> return mem_equals(field, len, \"tree \") ||\n> mem_equals(field, len, \"parent \") ||\n> ...;\n> }\n>\n> and the caller should tell us it's OK to look at field[len]:\n>\n> standard_header_field(line, eof - line + 1)\n>\n> We could also omit the space from the standard_header_field.\n\nYes, that was what I had in mind. The only reason why the callee\n(over-)optimizes the \"SP must follow these know keywords\" part by\nusing the extra \"len\" parameter is because the caller has to do a\nsingle strchr() to skip an arbitrary field name anyway so computing\n\"len\" is essentially free.\n\n> The caller\n> just ran strchr() looking for the space, so we know that either it is\n> there, or we are at the end of the line/buffer. Arguably a string like\n> \"parent\\n\" should be \"a parent header with no data\" (but right now it is\n> not matched by this function). I'm not aware of an implementation that\n> writes such a thing, but it seems to fall in the \"be liberal in what you\n> accept\" category.\n\nIt is _not_ a standard header field, so it will be read by the logic\nin the caller as a non-standard header field without getting lost.\n"},{"id":"236630","messageId":"5320C8CC.3010404@web.de","threadId":"36140","inReplyTo":"xmqqiorjky0a.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] general style: replaces memcmp() with proper starts_with()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2014-03-12T20:51:24Z","receivedAt":"2014-03-12T20:51:24Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 12.03.2014 20:39, schrieb Junio C Hamano:\n> Jeff King writes:\n>\n>>> static inline int standard_header_field(const char *field, size_t len)\n>>> {\n>>> -\treturn ((len == 4 && !memcmp(field, \"tree \", 5)) ||\n>>> -\t\t(len == 6 && !memcmp(field, \"parent \", 7)) ||\n>>> -\t\t(len == 6 && !memcmp(field, \"author \", 7)) ||\n>>> -\t\t(len == 9 && !memcmp(field, \"committer \", 10)) ||\n>>> -\t\t(len == 8 && !memcmp(field, \"encoding \", 9)));\n>>> +\treturn ((len == 4 && starts_with(field, \"tree \")) ||\n>>> +\t\t(len == 6 && starts_with(field, \"parent \")) ||\n>>> +\t\t(len == 6 && starts_with(field, \"author \")) ||\n>>> +\t\t(len == 9 && starts_with(field, \"committer \")) ||\n>>> +\t\t(len == 8 && starts_with(field, \"encoding \")));\n>>\n>> These extra \"len\" checks are interesting. They look like an attempt to\n>> optimize lookup, since the caller will already have scanned forward to\n>> the space.\n\nI wonder what the performance impact might be. The length checks are \nalso needed for correctness, however, to avoid running over the end of \nthe buffer.\n\n> If one really wants to remove the magic constants from this, then\n> one must take advantage of the pattern\n>\n> \tlen == strlen(S) - 1 && !memcmp(field, S, strlen(S))\n>\n> that appears here, and come up with a simple abstraction to express\n> that we are only using the string S (e.g. \"tree \"), length len and\n> location field of the counted string.\n\nThis might be a job for kwset.\n\nRené\n"},{"id":"236629","messageId":"87ob1bi1hp.fsf@fencepost.gnu.org","threadId":"36140","inReplyTo":"xmqqiorjky0a.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] general style: replaces memcmp() with proper starts_with()","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2014-03-12T20:52:02Z","receivedAt":"2014-03-12T20:52:02Z","isPatch":true,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"Junio C Hamano writes:\n\n> Jeff King writes:\n>\n>>> static inline int standard_header_field(const char *field, size_t len)\n>>> {\n>>> -\treturn ((len == 4 && !memcmp(field, \"tree \", 5)) ||\n>>> -\t\t(len == 6 && !memcmp(field, \"parent \", 7)) ||\n>>> -\t\t(len == 6 && !memcmp(field, \"author \", 7)) ||\n>>> -\t\t(len == 9 && !memcmp(field, \"committer \", 10)) ||\n>>> -\t\t(len == 8 && !memcmp(field, \"encoding \", 9)));\n>>> +\treturn ((len == 4 && starts_with(field, \"tree \")) ||\n>>> +\t\t(len == 6 && starts_with(field, \"parent \")) ||\n>>> +\t\t(len == 6 && starts_with(field, \"author \")) ||\n>>> +\t\t(len == 9 && starts_with(field, \"committer \")) ||\n>>> +\t\t(len == 8 && starts_with(field, \"encoding \")));\n>>\n>> These extra \"len\" checks are interesting. They look like an attempt to\n>> optimize lookup, since the caller will already have scanned forward to\n>> the space.\n>\n> If one really wants to remove the magic constants from this, then\n> one must take advantage of the pattern\n>\n> \tlen == strlen(S) - 1 && !memcmp(field, S, strlen(S))\n\nIf the caller has already scanned forward to the space, then there is no\nactual need to let the comparison compare the space again after checking\nlen, is there? That makes for a more consistent look in the memcmp\ncase.\n\nOne could do this in a switch on len instead. Not that it seems\nterribly more efficient.\n\n> that appears here, and come up with a simple abstraction to express\n> that we are only using the string S (e.g. \"tree \"), length len and\n> location field of the counted string.\n>\n> Blindly replacing starts_with() with !memcmp() in the above part is\n> a readability regression otherwise.\n\nDon't really think so: if the len at the front and the back is the same\nand the space is not explicitly compared any more, both look pretty much\nthe same to me.\n\n>> ... I think with a few more helpers we could really further clean up\n>> some of these callsites.\n>\n> Yes.\n\nPossibly. But it does seem like overkill.\n\n-- \nDavid Kastrup\n"},{"id":"236632","messageId":"20140312211415.GA10305@sigill.intra.peff.net","threadId":"36140","inReplyTo":"xmqq61njkwnw.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] general style: replaces memcmp() with proper starts_with()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-12T21:14:15Z","receivedAt":"2014-03-12T21:14:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 12, 2014 at 01:08:03PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> >> Blindly replacing starts_with() with !memcmp() in the above part is\n> >> a readability regression otherwise.\n> >\n> > I actually think the right solution is:\n> >\n> > static inline int standard_header_field(const char *field, size_t len)\n> > {\n> > return mem_equals(field, len, \"tree \") ||\n> > mem_equals(field, len, \"parent \") ||\n> > ...;\n> > }\n> >\n> > and the caller should tell us it's OK to look at field[len]:\n> >\n> > standard_header_field(line, eof - line + 1)\n> >\n> > We could also omit the space from the standard_header_field.\n> \n> Yes, that was what I had in mind. The only reason why the callee\n> (over-)optimizes the \"SP must follow these know keywords\" part by\n> using the extra \"len\" parameter is because the caller has to do a\n> single strchr() to skip an arbitrary field name anyway so computing\n> \"len\" is essentially free.\n\nOne thing that bugs me about the current code is that the sub-function\nlooks one past the end of the length given to it by the caller.\nSwitching it to pass \"eof - line + 1\" resolves that, but is it right?\n\nThe character pointed at by \"eof\" is either:\n\n 1. space, if our strchr(line, ' ') found something\n\n 2. the first character of the next line, if our\n memchr(line, '\\n', eob - line) found something\n\n 3. Whatever is at eob (end-of-buffer)\n\nThere are two questionable things here. In (1), we use strchr on a\nsized buffer. And in (3), we look one past the size that was passed in.\n\nIn both cases, we are saved by the fact that the buffer is actually NUL\nterminated at the end of \"size\" (because it comes from read_sha1_file).\nBut we may find a space much further than the line ending which is\nsupposed to be our boundary, and end up having to do a comparison to\ncover this case.\n\nSo I think the current code is correct, but we could make it more\nobvious by:\n\n 1. Restricting our search for the field separator to the current line.\n\n 2. Explicitly avoid looking for headers when we did not find a space,\n since we cannot match anything anyway.\n\nLike:\n\ndiff --git a/commit.c b/commit.c\nindex 6bf4fe0..9383cc1 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1325,14 +1325,14 @@ static struct commit_extra_header *read_commit_extra_header_lines(\n \t\tstrbuf_reset(&buf);\n \t\tit = NULL;\n \n-\t\teof = strchr(line, ' ');\n-\t\tif (next <= eof)\n+\t\teof = memchr(line, ' ', next - line);\n+\t\tif (eof) {\n+\t\t\tif (standard_header_field(line, eof - line + 1) ||\n+\t\t\t excluded_header_field(line, eof - line, exclude))\n+\t\t\t\tcontinue;\n+\t\t} else\n \t\t\teof = next;\n \n-\t\tif (standard_header_field(line, eof - line) ||\n-\t\t excluded_header_field(line, eof - line, exclude))\n-\t\t\tcontinue;\n-\n \t\tit = xcalloc(1, sizeof(*it));\n \t\tit->key = xmemdupz(line, eof-line);\n \t\t*tail = it;\n\nI also think that \"eof = next\" (which I retained here) is off-by-one.\n\"next\" here is not the newline, but the start of the next line. And I'm\nguessing the code actually wanted the newline (otherwise \"it->key\" ends\nup with the newline in it). But we cannot just subtract one, because if\nwe hit \"eob\", it really is in the right spot.\n\n-Peff\n"},{"id":"236633","messageId":"87fvmni0de.fsf@fencepost.gnu.org","threadId":"36140","inReplyTo":"5320C8CC.3010404@web.de","subject":"Re: [PATCH] general style: replaces memcmp() with proper starts_with()","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2014-03-12T21:16:13Z","receivedAt":"2014-03-12T21:16:13Z","isPatch":true,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"René Scharfe writes:\n\n> Am 12.03.2014 20:39, schrieb Junio C Hamano:\n>> Jeff King writes:\n>>\n>>>> static inline int standard_header_field(const char *field, size_t len)\n>>>> {\n>>>> -\treturn ((len == 4 && !memcmp(field, \"tree \", 5)) ||\n>>>> -\t\t(len == 6 && !memcmp(field, \"parent \", 7)) ||\n>>>> -\t\t(len == 6 && !memcmp(field, \"author \", 7)) ||\n>>>> -\t\t(len == 9 && !memcmp(field, \"committer \", 10)) ||\n>>>> -\t\t(len == 8 && !memcmp(field, \"encoding \", 9)));\n>>>> +\treturn ((len == 4 && starts_with(field, \"tree \")) ||\n>>>> +\t\t(len == 6 && starts_with(field, \"parent \")) ||\n>>>> +\t\t(len == 6 && starts_with(field, \"author \")) ||\n>>>> +\t\t(len == 9 && starts_with(field, \"committer \")) ||\n>>>> +\t\t(len == 8 && starts_with(field, \"encoding \")));\n>>>\n>>> These extra \"len\" checks are interesting. They look like an attempt to\n>>> optimize lookup, since the caller will already have scanned forward to\n>>> the space.\n>\n> I wonder what the performance impact might be. The length checks are\n> also needed for correctness, however, to avoid running over the end of\n> the buffer.\n\nDepends on whether memcmp is guaranteed to stop immediately on mismatch.\nThen memcmp(field, \"tree \", 5) cannot walk across a NUL byte in field.\n\n-- \nDavid Kastrup\n"},{"id":"236640","messageId":"20140312213944.GA12818@sigill.intra.peff.net","threadId":"36140","inReplyTo":"20140312211415.GA10305@sigill.intra.peff.net","subject":"Re: [PATCH] general style: replaces memcmp() with proper starts_with()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-12T21:39:44Z","receivedAt":"2014-03-12T21:39:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 12, 2014 at 05:14:15PM -0400, Jeff King wrote:\n\n> I also think that \"eof = next\" (which I retained here) is off-by-one.\n> \"next\" here is not the newline, but the start of the next line. And I'm\n> guessing the code actually wanted the newline (otherwise \"it->key\" ends\n> up with the newline in it). But we cannot just subtract one, because if\n> we hit \"eob\", it really is in the right spot.\n\nI started on a patch for this, but found another interesting corner\ncase. If we do not find a newline and hit end-of-buffer (which\n_shouldn't_ happen, as that is a malformed commit object), we set \"next\"\nto \"eob\". But then we copy the whole string, including *next into the\n\"value\" of the header.\n\nSo we intend to capture the trailing newline in the value, and do in the\nnormal case. But in the end-of-buffer case, we capture an extra NUL. I\nthink that's OK, because the eventual fate of the buffer is to become a\nC-string. But it does mean that the result sometimes has a\nnewline-terminator and sometimes doesn't, and the calling code needs to\nhandle this when printing, or risk lines running together.\n\nShould this function append a newline if there is not already one? If\nit's a mergetag header, we feed the result to gpg, etc, and expect to\nget the data out verbatim. We would not want to mess that up. OTOH, the\ncommit object is bogus (and possibly truncated) in the first place, so\nit probably doesn't really matter. And the GPG signature on tag objects\nhas its own delimiters, so a stray newline present or not at the end\nshould not matter.\n\n-Peff\n"},{"id":"236641","messageId":"5320D56B.8060101@web.de","threadId":"36140","inReplyTo":"87fvmni0de.fsf@fencepost.gnu.org","subject":"Re: [PATCH] general style: replaces memcmp() with proper starts_with()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2014-03-12T21:45:15Z","receivedAt":"2014-03-12T21:45:15Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 12.03.2014 22:16, schrieb David Kastrup:\n> René Scharfe writes:\n>\n>> Am 12.03.2014 20:39, schrieb Junio C Hamano:\n>>> Jeff King writes:\n>>>\n>>>>> static inline int standard_header_field(const char *field, size_t len)\n>>>>> {\n>>>>> -\treturn ((len == 4 && !memcmp(field, \"tree \", 5)) ||\n>>>>> -\t\t(len == 6 && !memcmp(field, \"parent \", 7)) ||\n>>>>> -\t\t(len == 6 && !memcmp(field, \"author \", 7)) ||\n>>>>> -\t\t(len == 9 && !memcmp(field, \"committer \", 10)) ||\n>>>>> -\t\t(len == 8 && !memcmp(field, \"encoding \", 9)));\n>>>>> +\treturn ((len == 4 && starts_with(field, \"tree \")) ||\n>>>>> +\t\t(len == 6 && starts_with(field, \"parent \")) ||\n>>>>> +\t\t(len == 6 && starts_with(field, \"author \")) ||\n>>>>> +\t\t(len == 9 && starts_with(field, \"committer \")) ||\n>>>>> +\t\t(len == 8 && starts_with(field, \"encoding \")));\n>>>>\n>>>> These extra \"len\" checks are interesting. They look like an attempt to\n>>>> optimize lookup, since the caller will already have scanned forward to\n>>>> the space.\n>>\n>> I wonder what the performance impact might be. The length checks are\n>> also needed for correctness, however, to avoid running over the end of\n>> the buffer.\n>\n> Depends on whether memcmp is guaranteed to stop immediately on mismatch.\n> Then memcmp(field, \"tree \", 5) cannot walk across a NUL byte in field.\n\nI'm not sure we can rely on that property. But anyway -- if field \npoints to, say, a one-byte buffer containing the letter t, then memcmp \nwould overrun that buffer. If field was guaranteed to be NUL-terminated \nthen at least starts_with would stop at the end, but the signature of \nstandard_header_field() suggests that it can be used with arbitrary \nbuffers, not just C strings.\n\nRené\n"},{"id":"236643","messageId":"20140312220640.GA14802@sigill.intra.peff.net","threadId":"36140","inReplyTo":"20140312211415.GA10305@sigill.intra.peff.net","subject":"Re: [PATCH] general style: replaces memcmp() with proper starts_with()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-12T22:06:40Z","receivedAt":"2014-03-12T22:06:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 12, 2014 at 05:14:15PM -0400, Jeff King wrote:\n\n> One thing that bugs me about the current code is that the sub-function\n> looks one past the end of the length given to it by the caller.\n> Switching it to pass \"eof - line + 1\" resolves that, but is it right?\n> \n> The character pointed at by \"eof\" is either:\n> \n> 1. space, if our strchr(line, ' ') found something\n> \n> 2. the first character of the next line, if our\n> memchr(line, '\\n', eob - line) found something\n> \n> 3. Whatever is at eob (end-of-buffer)\n\nThis misses a case. We might find no space at all, and eof is NULL. We\nnever dereference it, so we don't segfault, but it does cause a bogus\ngiant allocation.\n\nI'm out of time for the day, but here is a test I started on that\ndemonstrates the failure:\n\ndiff --git a/t/t7513-commit-bogus-extra-headers.sh b/t/t7513-commit-bogus-extra-headers.sh\nindex e69de29..834db08 100755\n--- a/t/t7513-commit-bogus-extra-headers.sh\n+++ b/t/t7513-commit-bogus-extra-headers.sh\n@@ -0,0 +1,26 @@\n+#!/bin/sh\n+\n+test_description='check parsing of badly formed commit objects'\n+. ./test-lib.sh\n+\n+EMPTY_TREE=4b825dc642cb6eb9a060e54bf8d69288fbee4904\n+\n+test_expect_success 'newline right after mergetag header' '\n+\ttest_tick &&\n+\tcat >commit <<-EOF &&\n+\ttree $EMPTY_TREE\n+\tauthor $GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL> $GIT_AUTHOR_DATE\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tmergetag\n+\n+\tfoo\n+\tEOF\n+\tcommit=$(git hash-object -w -t commit commit) &&\n+\tcat >expect <<-EOF &&\n+\ttodo...\n+\tEOF\n+\tgit --no-pager show --show-signature $commit >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n\nThe \"git show\" fails with:\n\n fatal: Out of memory, malloc failed (tried to allocate 18446744073699764927 bytes)\n\nSo I think the whole function could use some refactoring to handle\ncorner cases better. I'll try to take a look tomorrow, but please feel\nfree if somebody else wants to take a crack at it.\n\n-Peff\n"},{"id":"236644","messageId":"xmqqha73jb6q.fsf@gitster.dls.corp.google.com","threadId":"36140","inReplyTo":"20140312175624.GA7982@sigill.intra.peff.net","subject":"Re: [PATCH] general style: replaces memcmp() with proper starts_with()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-12T22:37:17Z","receivedAt":"2014-03-12T22:37:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Thanks, I think this is a real readability improvement in most cases.\n> ...\n>\n> I tried:\n>\n> perl -i -lpe '\n> s/memcmp\\(([^,]+), \"(.*?)\", (\\d+)\\)/\n> length($2) == $3 ?\n> qq{!starts_with($1, \"$2\")} :\n> $&\n> /ge\n> ' $(git ls-files '*.c')\n>\n> That comes up with almost the same results as yours, but misses a few\n> cases that you caught (in addition to the need to simplify\n> !!starts_with()):\n>\n> - memcmp(foo, \"bar\", strlen(\"bar\"))\n>\n> - strings with interpolation, like \"foo\\n\", which is actually 4\n> characters in length, not 5.\n>\n> But there were only a few of those, and I hand-verified them. So I feel\n> pretty good that the changes are all correct transformations.\n>\n> That leaves the question of whether they are all improvements in\n> readability (more on that below).\n\nAfter reading the patch and the result of your Perl replacement, I'd\nagree with the \"correctness\" but I am not as convinced as you seem\nto be about the \"real readability improvement in most cases.\" \"some\ncases\", perhaps, though.\n\nTaking two random examples from an early and a late parts of the\npatch:\n\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -82,7 +82,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name)\n \t\t\t\tenum object_type type;\n \t\t\t\tunsigned long size;\n \t\t\t\tchar *buffer = read_sha1_file(sha1, &type, &size);\n-\t\t\t\tif (memcmp(buffer, \"object \", 7) ||\n+\t\t\t\tif (!starts_with(buffer, \"object \") ||\n \t\t\t\t get_sha1_hex(buffer + 7, blob_sha1))\n \t\t\t\t\tdie(\"%s not a valid tag\", sha1_to_hex(sha1));\n \t\t\t\tfree(buffer);\n\ndiff --git a/transport.c b/transport.c\nindex ca7bb44..a267822 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -1364,7 +1364,7 @@ static int refs_from_alternate_cb(struct alternate_object_database *e,\n \n \twhile (other[len-1] == '/')\n \t\tother[--len] = '\\0';\n-\tif (len < 8 || memcmp(other + len - 8, \"/objects\", 8))\n+\tif (len < 8 || !starts_with(other + len - 8, \"/objects\"))\n \t\treturn 0;\n \t/* Is this a git repository with refs? */\n \tmemcpy(other + len - 8, \"/refs\", 6);\n\n\nThe original hunks show that the code knows and relies on magic\nnumbers 7 and 8 very clearly and there are rooms for improvement.\nThe result after the conversion, however, still have the same magic\nnumbers, but one less of them each. Doesn't it make it harder to\nlater spot the patterns to come up with a better abstraction that\ndoes not rely on the magic number? Especially in the first hunk, we\ncan spot the repeated 7s in the original that make it glaringly\nclear that we might want a better abstraction there, but in the text\nafter the patch, there is less clue that encourages us to take a\nlook at that \"buffer + 7\" further to make sure we do not feed a\nwrong string to get_sha1_hex() by mistake when we update the\nsurrounding code or the data format this codepath parses.\n\nI think grepping for \"memcmp()\" that compares the same number of\nbytes as a constant string, like you showed in that Perl scriptlet,\nis a very good first step to locate where we might want to look for\nimprovements. I however think that a mechanical replacement of all\nsuch memcmp() with !start_with() is of a dubious value.\n\nAfter finding the hunk in the cat-file.c shown above, and after\nseeing many other similar patterns, one may realize that it is a\ngood use case for a variant of skip_prefix() that returns boolean,\nwhich we discussed earlier, perhaps like so:\n\n\tif (!skip_over(buffer, \"object \", &object_name) ||\n get_sha1_hex(object_name, blob_sha1))\n\t\tdie(...);\n\nand such a solution would be what truly eradicates the reliance of\nmagic constants that might break by miscounting bytes in string\nconstants. I do not think mechanical replacement to !start_with()\nis \"going in the right direction and reaching a halfway to that\ngoal\". I honestly think that it instead is taking us into a wrong\ndirection, without really solving the use of brittle magic constants\nand making remaining reliance of them even harder to spot.\n\nSo....\n"},{"id":"236645","messageId":"xmqqd2hrjb4d.fsf@gitster.dls.corp.google.com","threadId":"36140","inReplyTo":"20140312220640.GA14802@sigill.intra.peff.net","subject":"Re: [PATCH] general style: replaces memcmp() with proper starts_with()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-12T22:38:42Z","receivedAt":"2014-03-12T22:38:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> So I think the whole function could use some refactoring to handle\n> corner cases better. I'll try to take a look tomorrow, but please\n> feel free if somebody else wants to take a crack at it.\n\nYup, thanks.\n"},{"id":"236655","messageId":"CALs4jVHTBH3wTAQsv8+jb15Do1_oy0pcThsDL8ssE7fgrx5NxA@mail.gmail.com","threadId":"36140","inReplyTo":"xmqqd2hrjb4d.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] general style: replaces memcmp() with proper starts_with()","fromName":"Quint Guvernator","fromEmail":"quintus.public@gmail.com","sentAt":"2014-03-13T03:33:50Z","receivedAt":"2014-03-13T03:33:50Z","isPatch":true,"sender":{"key":"quintus.public@gmail.com","avatar":"https://gravatar.com/avatar/0805695585d6191e00badd5057b262b501047d9fb2b22dd2495009ffb25d85fd?d=mp&s=160"},"body":">From what I can gather, there seems to be opposition to specific\npieces of this patch.\n\nThe following area is clearly the most controversial:\n\n> static inline int standard_header_field(const char *field, size_t len)\n> {\n> - return ((len == 4 && !memcmp(field, \"tree \", 5)) ||\n> - (len == 6 && !memcmp(field, \"parent \", 7)) ||\n> - (len == 6 && !memcmp(field, \"author \", 7)) ||\n> - (len == 9 && !memcmp(field, \"committer \", 10)) ||\n> - (len == 8 && !memcmp(field, \"encoding \", 9)));\n> + return ((len == 4 && starts_with(field, \"tree \")) ||\n> + (len == 6 && starts_with(field, \"parent \")) ||\n> + (len == 6 && starts_with(field, \"author \")) ||\n> + (len == 9 && starts_with(field, \"committer \")) ||\n> + (len == 8 && starts_with(field, \"encoding \")));\n\nI am happy to submit a version of this patch excluding this section\n(and/or others), but it seems I've stumbled into a more fundamental\nconversation about the place for helper functions in general (and\nabout refactoring skip_prefix()). I am working on this particular\nchange as a microproject, #14 on the list [1], and am not as familiar\nwith the conventions of the Git codebase as many of you on this list\nare.\n\nJunio said:\n\n> The result after the conversion, however, still have the same magic\n> numbers, but one less of them each. Doesn't it make it harder to\n> later spot the patterns to come up with a better abstraction that\n> does not rely on the magic number?\n\nIt is _not_ my goal to make the code harder to maintain down the road.\nSo, at this point, which hunks (if any) are worth patching?\n\nQuint\n\n\n[1]: http://git.github.io/SoC-2014-Microprojects.html\n"},{"id":"236660","messageId":"877g7yipf9.fsf@fencepost.gnu.org","threadId":"36140","inReplyTo":"xmqqha73jb6q.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] general style: replaces memcmp() with proper starts_with()","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2014-03-13T06:27:22Z","receivedAt":"2014-03-13T06:27:22Z","isPatch":true,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"Junio C Hamano writes:\n\n> Taking two random examples from an early and a late parts of the\n> patch:\n>\n> --- a/builtin/cat-file.c\n> +++ b/builtin/cat-file.c\n> @@ -82,7 +82,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name)\n> \t\t\t\tenum object_type type;\n> \t\t\t\tunsigned long size;\n> \t\t\t\tchar *buffer = read_sha1_file(sha1, &type, &size);\n> -\t\t\t\tif (memcmp(buffer, \"object \", 7) ||\n> +\t\t\t\tif (!starts_with(buffer, \"object \") ||\n\n[...]\n\n> The original hunks show that the code knows and relies on magic\n> numbers 7 and 8 very clearly and there are rooms for improvement.\n\nLike: what if the file is empty?\n\n-- \nDavid Kastrup\n"},{"id":"236675","messageId":"xmqq8usej8km.fsf@gitster.dls.corp.google.com","threadId":"36140","inReplyTo":"CALs4jVHTBH3wTAQsv8+jb15Do1_oy0pcThsDL8ssE7fgrx5NxA@mail.gmail.com","subject":"Re: [PATCH] general style: replaces memcmp() with proper starts_with()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-13T17:46:01Z","receivedAt":"2014-03-13T17:46:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Quint Guvernator writes:\n\n>> The result after the conversion, however, still have the same magic\n>> numbers, but one less of them each. Doesn't it make it harder to\n>> later spot the patterns to come up with a better abstraction that\n>> does not rely on the magic number?\n>\n> It is _not_ my goal to make the code harder to maintain down the road.\n\nGood.\n\n> So, at this point, which hunks (if any) are worth patching?\n\nWill, I am not going through all the mechanical hits to memcmp() and\njudge each and every one if it is a good idea to convert. Anybody\nwho does so in order to tell you \"which hunks are worth patching\"\nwould end up being the one doing the real work, and at that point\nthere is nothing left to be credited as your work anymore ;-)\n\nBut as Peff said, there are good bits, like these ones just for a\nfew examples.\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex a7e72d5..16c20af 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -1631,7 +1631,7 @@ static int parse_fragment(const char *line, unsigned long size,\n \t\t * l10n of \"\\ No newline...\" is at least that long.\n \t\t */\n \t\tcase '\\\\':\n-\t\t\tif (len < 12 || memcmp(line, \"\\\\ \", 2))\n+\t\t\tif (len < 12 || !starts_with(line, \"\\\\ \"))\n \t\t\t\treturn -1;\n \t\t\tbreak;\n \t\t}\n@@ -1646,7 +1646,7 @@ static int parse_fragment(const char *line, unsigned long size,\n \t * it in the above loop because we hit oldlines == newlines == 0\n \t * before seeing it.\n \t */\n-\tif (12 < size && !memcmp(line, \"\\\\ \", 2))\n+\tif (12 < size && starts_with(line, \"\\\\ \"))\n \t\toffset += linelen(line, size);\n \n \tpatch->lines_added += added;\n\nThese two are about \"An incomplete line marker begins with a\nbackslash and a SP\" and there is no other significance in the\nconstant 2 (like, \"after we recognise the match, we start scanning\nthe remainder of the line starting at the offset 2\").\n\nIt is a tangent but I notice that these two parts (both in the\noriginal and in the version after patch) contradict what the\nincomplete last line marker should look like in a minor detail.\n\nOn the other hand, I think this one from nearby is iffy.\n\n@@ -846,8 +846,8 @@ static int has_epoch_timestamp(const char *nameline)\n \t * YYYY-MM-DD hh:mm:ss must be from either 1969-12-31\n \t * (west of GMT) or 1970-01-01 (east of GMT)\n \t */\n-\tif ((zoneoffset < 0 && memcmp(timestamp, \"1969-12-31\", 10)) ||\n-\t (0 <= zoneoffset && memcmp(timestamp, \"1970-01-01\", 10)))\n+\tif ((zoneoffset < 0 && !starts_with(timestamp, \"1969-12-31\")) ||\n+\t (0 <= zoneoffset && !starts_with(timestamp, \"1970-01-01\")))\n \t\treturn 0;\n \n \thourminute = (strtol(timestamp + 11, NULL, 10) * 60 +\n\nIf one looks at the post-context of the hunk, one would realize that\nthis codepath very intimately knows how the timestamp should look\nlike at the byte-offset level, not just \"YYYY-MM-DD ought to be\n10-byte long\", but \"there should be two-digit hour part after\nskipping one byte after that YYYY-MM-DD part, followed by two-digit\nminute part after further skipping one byte\", knowing that these\ndetails are guaranteed by the stamp_regexp[] pattern it earlier made\nsure the given string would match.\n\nI do not think it is a good idea to reduce this kind of precise\nformat knowledge from this function in the first place (after all,\nthis is parsing a header line in a traditional diff whose format is\nknown to us), and even if it were our eventual goal to reduce the\nprecise format knowledge, it would not help very much to get rid of\nconstant 10 only from these two memcmp() calls, and that is why I\nthink this hunk is iffy.\n\nHope the above shows what kind of thinking is needed to judge each\nchange from memcmp() to !starts_with().\n\nThanks.\n"},{"id":"236676","messageId":"xmqq4n32j8i7.fsf@gitster.dls.corp.google.com","threadId":"36140","inReplyTo":"877g7yipf9.fsf@fencepost.gnu.org","subject":"Re: [PATCH] general style: replaces memcmp() with proper starts_with()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-13T17:47:28Z","receivedAt":"2014-03-13T17:47:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Kastrup writes:\n\n> Junio C Hamano writes:\n>\n>> Taking two random examples from an early and a late parts of the\n>> patch:\n>>\n>> --- a/builtin/cat-file.c\n>> +++ b/builtin/cat-file.c\n>> @@ -82,7 +82,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name)\n>> \t\t\t\tenum object_type type;\n>> \t\t\t\tunsigned long size;\n>> \t\t\t\tchar *buffer = read_sha1_file(sha1, &type, &size);\n>> -\t\t\t\tif (memcmp(buffer, \"object \", 7) ||\n>> +\t\t\t\tif (!starts_with(buffer, \"object \") ||\n>\n> [...]\n>\n>> The original hunks show that the code knows and relies on magic\n>> numbers 7 and 8 very clearly and there are rooms for improvement.\n>\n> Like: what if the file is empty?\n\nYes.\n"},{"id":"236678","messageId":"20140313175515.GA18897@sigill.intra.peff.net","threadId":"36140","inReplyTo":"xmqq4n32j8i7.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] general style: replaces memcmp() with proper starts_with()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-13T17:55:15Z","receivedAt":"2014-03-13T17:55:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 13, 2014 at 10:47:28AM -0700, Junio C Hamano wrote:\n\n> >> --- a/builtin/cat-file.c\n> >> +++ b/builtin/cat-file.c\n> >> @@ -82,7 +82,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name)\n> >> \t\t\t\tenum object_type type;\n> >> \t\t\t\tunsigned long size;\n> >> \t\t\t\tchar *buffer = read_sha1_file(sha1, &type, &size);\n> >> -\t\t\t\tif (memcmp(buffer, \"object \", 7) ||\n> >> +\t\t\t\tif (!starts_with(buffer, \"object \") ||\n> >\n> > [...]\n> >\n> >> The original hunks show that the code knows and relies on magic\n> >> numbers 7 and 8 very clearly and there are rooms for improvement.\n> >\n> > Like: what if the file is empty?\n> \n> Yes.\n\nI think this one is actually OK. The result of read_sha1_file is\nNUL-terminated, and we know that starts_with reads byte-by-byte (the\nprior memcmp is wrong, but only if you care about accessing bytes past\nthe end of the NUL).\n\nThat whole piece of code seems silly, though, IMHO. It should be using\nparse_tag or peel_to_type.\n\n-Peff\n"},{"id":"236718","messageId":"20140314045758.GA32748@sigill.intra.peff.net","threadId":"36140","inReplyTo":"CALs4jVHTBH3wTAQsv8+jb15Do1_oy0pcThsDL8ssE7fgrx5NxA@mail.gmail.com","subject":"Re: [PATCH] general style: replaces memcmp() with proper starts_with()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-14T04:57:58Z","receivedAt":"2014-03-14T04:57:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 12, 2014 at 11:33:50PM -0400, Quint Guvernator wrote:\n\n> It is _not_ my goal to make the code harder to maintain down the road.\n> So, at this point, which hunks (if any) are worth patching?\n\nThis discussion ended up encompassing a lot of other related cleanups. I\nhope we didn't scare you away. :)\n\nMy understanding is that you were approaching this as a micro-project\nfor GSoC. I'd love it if you want to pick up and run with some of the\nideas discussed here. But as far as a microproject goes, I think it\nwould make sense to identify one or two no-brainer improvement spots by\nhand, and submit a patch with just those (and I think Junio gave some\ngood guidelines in his reply).\n\n-Peff\n"},{"id":"236727","messageId":"CALs4jVHcPFjf-12mXvvSxctNJZgLD=P3=57u6_FYePDOB7txQQ@mail.gmail.com","threadId":"36140","inReplyTo":"20140314045758.GA32748@sigill.intra.peff.net","subject":"Re: [PATCH] general style: replaces memcmp() with proper starts_with()","fromName":"Quint Guvernator","fromEmail":"quintus.public@gmail.com","sentAt":"2014-03-14T14:51:53Z","receivedAt":"2014-03-14T14:51:53Z","isPatch":true,"sender":{"key":"quintus.public@gmail.com","avatar":"https://gravatar.com/avatar/0805695585d6191e00badd5057b262b501047d9fb2b22dd2495009ffb25d85fd?d=mp&s=160"},"body":"2014-03-14 0:57 GMT-04:00 Jeff King :\n> This discussion ended up encompassing a lot of other related cleanups. I\n> hope we didn't scare you away. :)\n\nI don't think you could; this community is much more accepting than\nother software communities around the web. The fact that I received\nconstructive feedback rather than a lecture when formatting issues\nslipped my mind (i.e. forgetting [PATCH v2]) is reason enough to stick\naround!\n\n> My understanding is that you were approaching this as a micro-project\n> for GSoC. I'd love it if you want to pick up and run with some of the\n> ideas discussed here. But as far as a microproject goes, I think it\n> would make sense to identify one or two no-brainer improvement spots by\n> hand, and submit a patch with just those (and I think Junio gave some\n> good guidelines in his reply).\n\nI agree with trying to push a few uncontroversial changes through. I'd\nlove to take a deeper look at these helper functions and related\ncleanups…perhaps it would be worth it to identify a few key areas to\nwork on in addition to a main GSoC project? In fact, the project I'm\nlooking to take on (rebase --interactive) also involves code cleanup\nand might not take all summer, so I could see how those could work\nwell together in a proposal.\n\nI'll be re-reading this thread and working on this patch over the\nweekend to try to identify the more straightforward hunks I could\nsubmit in a patch.\n\nThanks Peff and everyone else for your help.\nQuint\n"},{"id":"236733","messageId":"xmqqeh24hg6m.fsf@gitster.dls.corp.google.com","threadId":"36140","inReplyTo":"CALs4jVHcPFjf-12mXvvSxctNJZgLD=P3=57u6_FYePDOB7txQQ@mail.gmail.com","subject":"Re: [PATCH] general style: replaces memcmp() with proper starts_with()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-14T16:56:49Z","receivedAt":"2014-03-14T16:56:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Quint Guvernator writes:\n\n> I'll be re-reading this thread and working on this patch over the\n> weekend to try to identify the more straightforward hunks I could\n> submit in a patch.\n\nThanks.\n"}]}