{"thread":{"id":"36174","subject":"[PATCH v3 0/1] general style: replaces memcmp() with starts_with()","startedAt":"2014-03-15T16:39:12Z","lastAt":"2014-03-17T20:36:59Z","messageCount":4,"participants":["Quint Guvernator","Michael Haggerty"],"isPatch":true,"patchVersion":3,"patchTotal":1},"messages":[{"id":"236796","messageId":"1394901553-69548-1-git-send-email-quintus.public@gmail.com","threadId":"36174","inReplyTo":null,"subject":"[PATCH v3 0/1] general style: replaces memcmp() with starts_with()","fromName":"Quint Guvernator","fromEmail":"quintus.public@gmail.com","sentAt":"2014-03-15T16:39:12Z","receivedAt":"2014-03-15T16:39:12Z","isPatch":true,"sender":{"key":"quintus.public@gmail.com","avatar":"https://gravatar.com/avatar/0805695585d6191e00badd5057b262b501047d9fb2b22dd2495009ffb25d85fd?d=mp&s=160"},"body":"Hi, folks.\n\nI've looked through the list's responses and removed the most objectionable\nhunks from the patch v2, especially in cases where starts_with either hurts\nreadability or further obscures the use of magic numbers. Let me know what you\nall think about the changes.\n\nThank you all again for your help. This is my first patch here, and has been\nquite a microproject indeed!\n\nQuint Guvernator (1):\n  general style: replaces memcmp() with starts_with()\n\n builtin/apply.c                           |  6 +++---\n builtin/for-each-ref.c                    |  2 +-\n builtin/get-tar-commit-id.c               |  2 +-\n builtin/mktag.c                           |  8 ++++----\n builtin/patch-id.c                        | 18 +++++++++---------\n commit.c                                  |  4 ++--\n connect.c                                 |  6 +++---\n contrib/convert-objects/convert-objects.c |  6 +++---\n convert.c                                 |  2 +-\n http-walker.c                             |  2 +-\n imap-send.c                               |  6 +++---\n pack-write.c                              |  2 +-\n remote.c                                  |  2 +-\n 13 files changed, 33 insertions(+), 33 deletions(-)\n\n-- \n1.9.0\n"},{"id":"236797","messageId":"1394901553-69548-2-git-send-email-quintus.public@gmail.com","threadId":"36174","inReplyTo":"1394901553-69548-1-git-send-email-quintus.public@gmail.com","subject":"[PATCH v3 1/1] general style: replaces memcmp() with starts_with()","fromName":"Quint Guvernator","fromEmail":"quintus.public@gmail.com","sentAt":"2014-03-15T16:39:13Z","receivedAt":"2014-03-15T16:39:13Z","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 and when it is logical to do so. starts_with() looks\nnicer and it saves the extra argument of the length of the comparing\nstring.\n\nSigned-off-by: Quint Guvernator <quintus.public@gmail.com>\n---\n builtin/apply.c                           |  6 +++---\n builtin/for-each-ref.c                    |  2 +-\n builtin/get-tar-commit-id.c               |  2 +-\n builtin/mktag.c                           |  8 ++++----\n builtin/patch-id.c                        | 18 +++++++++---------\n commit.c                                  |  4 ++--\n connect.c                                 |  6 +++---\n contrib/convert-objects/convert-objects.c |  6 +++---\n convert.c                                 |  2 +-\n http-walker.c                             |  2 +-\n imap-send.c                               |  6 +++---\n pack-write.c                              |  2 +-\n remote.c                                  |  2 +-\n 13 files changed, 33 insertions(+), 33 deletions(-)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 0189523..de84dce 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@@ -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/for-each-ref.c b/builtin/for-each-ref.c\nindex 3e1d5c3..4135980 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/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 fa401ae..f3ca1db 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 */;\ndiff --git a/connect.c b/connect.c\nindex 4150412..1ff0540 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 */\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 <sha1>\\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/http-walker.c b/http-walker.c\nindex 1516c5e..8ae7d69 100644\n--- a/http-walker.c\n+++ b/http-walker.c\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 <user> <pass>\\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 <user> <pass>\",\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/remote.c b/remote.c\nindex 5f63d55..bd02b0e 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1149,7 +1149,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\",\n-- \n1.9.0\n"},{"id":"236886","messageId":"532722ED.1010500@alum.mit.edu","threadId":"36174","inReplyTo":"1394901553-69548-2-git-send-email-quintus.public@gmail.com","subject":"Re: [PATCH v3 1/1] general style: replaces memcmp() with starts_with()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-03-17T16:29:33Z","receivedAt":"2014-03-17T16:29:33Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Quint,\n\nThanks for v3 of the patch and for sticking with it.  See a few comments\nbelow.\n\nOn 03/15/2014 05:39 PM, Quint Guvernator wrote:\n> memcmp() is replaced with negated starts_with() when comparing strings\n> from the beginning and when it is logical to do so. starts_with() looks\n> nicer and it saves the extra argument of the length of the comparing\n> string.\n> \n> Signed-off-by: Quint Guvernator <quintus.public@gmail.com>\n> ---\n>  builtin/apply.c                           |  6 +++---\n>  builtin/for-each-ref.c                    |  2 +-\n>  builtin/get-tar-commit-id.c               |  2 +-\n>  builtin/mktag.c                           |  8 ++++----\n>  builtin/patch-id.c                        | 18 +++++++++---------\n>  commit.c                                  |  4 ++--\n>  connect.c                                 |  6 +++---\n>  contrib/convert-objects/convert-objects.c |  6 +++---\n>  convert.c                                 |  2 +-\n>  http-walker.c                             |  2 +-\n>  imap-send.c                               |  6 +++---\n>  pack-write.c                              |  2 +-\n>  remote.c                                  |  2 +-\n>  13 files changed, 33 insertions(+), 33 deletions(-)\n> \n> diff --git a/builtin/apply.c b/builtin/apply.c\n> index 0189523..de84dce 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> @@ -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>  \n> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\n> index 3e1d5c3..4135980 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;\n> diff --git a/builtin/get-tar-commit-id.c b/builtin/get-tar-commit-id.c\n> index 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);\n\nThis hunk uses the magic number \"11\" a couple lines later.  Junio (I\nthink rightly) objected [1] to rewrites in these circumstances because\nthey make it even less obvious where the constant came from and thereby\nmake the complete elimination of the hard-coded constant *more* difficult.\n\n> diff --git a/builtin/mktag.c b/builtin/mktag.c\n> index 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\nDitto.\n\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>  \n\nDitto.\n\n> diff --git a/builtin/patch-id.c b/builtin/patch-id.c\n> index 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\nDitto.  OK, I'm starting to see a pattern here so I'll leave you to look\nfor similar problems with later changes.\n\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.  */\n> diff --git a/commit.c b/commit.c\n> index fa401ae..f3ca1db 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\nDitto, except that here the magic number is used *before* your change.\n\nI stopped reading the changes here because I expect there will be a lot\nmore of the same problem.\n\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> diff --git a/connect.c b/connect.c\n> index 4150412..1ff0540 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> diff --git a/contrib/convert-objects/convert-objects.c b/contrib/convert-objects/convert-objects.c\n> index 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 <sha1>\\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}\n> diff --git a/convert.c b/convert.c\n> index 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;\n> diff --git a/http-walker.c b/http-walker.c\n> index 1516c5e..8ae7d69 100644\n> --- a/http-walker.c\n> +++ b/http-walker.c\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 &&\n> diff --git a/imap-send.c b/imap-send.c\n> index 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 <user> <pass>\\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 <user> <pass>\",\n>  \t\t\t\t\t\t\targ, cmd ? cmd : \"\");\n>  \t\t\t}\n> diff --git a/pack-write.c b/pack-write.c\n> index 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\",\n> diff --git a/remote.c b/remote.c\n> index 5f63d55..bd02b0e 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -1149,7 +1149,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\",\n> \n\nIn your cover letter you said:\n\n> I've looked through the list's responses and removed the most objectionable\n> hunks from the patch v2, especially in cases where starts_with either hurts\n> readability or further obscures the use of magic numbers. Let me know what you\n> all think about the changes.\n\nIn any open-source project it is important to optimize for the time of\nthe reviewer, because code-review is a relatively tedious task and is\ntherefore often the bottleneck for progress.  Therefore I suggest that\nyou turn your approach on its head.  Don't \"remove the most\nobjectionable hunks\" but rather *include only the best hunks*--the ones\nthat you can stand behind 100%, which you think are an unambiguous\nimprovement, and that the reviewer can accept without reservations.\n\nIt would be much better for you to submit only a handful of changes (or\neven only one!) that is perfect, rather than throwing a bunch at the\nwall and asking the reviewer to pick between the good and the bad.  As\nyou gain experience and learn about the preferences of the Git project,\nyou will get a better feel for the boundary between\nacceptable/unacceptable patches, and *then* you will be able to start\nsubmitting patches closer to the boundary.\n\nCheers,\nMichael\n\n[1] http://article.gmane.org/gmane.comp.version-control.git/244005\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"236911","messageId":"CALs4jVFFf2cBU920jRcyh1v9KAxkSS+LnycfYo4quuqjNRsp2g@mail.gmail.com","threadId":"36174","inReplyTo":"532722ED.1010500@alum.mit.edu","subject":"Re: [PATCH v3 1/1] general style: replaces memcmp() with starts_with()","fromName":"Quint Guvernator","fromEmail":"quintus.public@gmail.com","sentAt":"2014-03-17T20:36:59Z","receivedAt":"2014-03-17T20:36:59Z","isPatch":true,"sender":{"key":"quintus.public@gmail.com","avatar":"https://gravatar.com/avatar/0805695585d6191e00badd5057b262b501047d9fb2b22dd2495009ffb25d85fd?d=mp&s=160"},"body":"2014-03-17 12:29 GMT-04:00 Michael Haggerty <mhagger@alum.mit.edu>:\n> This hunk uses the magic number \"11\" a couple lines later.  Junio (I\n> think rightly) objected [1] to rewrites in these circumstances because\n> they make it even less obvious where the constant came from and thereby\n> make the complete elimination of the hard-coded constant *more* difficult.\n>\n> [1] http://article.gmane.org/gmane.comp.version-control.git/244005\n\nSure, I can understand that. I'll look through the hunks again with\nmore context in the diff to try to look for more cases where magic\nnumbers are used more than once. One of the goals of this revision is\nto minimize that, see the commit message.\n\n> In any open-source project it is important to optimize for the time of\n> the reviewer, because code-review is a relatively tedious task and is\n> therefore often the bottleneck for progress.  Therefore I suggest that\n> you turn your approach on its head.  Don't \"remove the most\n> objectionable hunks\" but rather *include only the best hunks*--the ones\n> that you can stand behind 100%, which you think are an unambiguous\n> improvement, and that the reviewer can accept without reservations.\n...\n> It would be much better for you to submit only a handful of changes (or\n> even only one!) that is perfect, rather than throwing a bunch at the\n> wall and asking the reviewer to pick between the good and the bad.  As\n> you gain experience and learn about the preferences of the Git project,\n> you will get a better feel for the boundary between\n> acceptable/unacceptable patches, and *then* you will be able to start\n> submitting patches closer to the boundary.\n\nI see. For v4, I will be more discerning as to what I include. I will\ntry to keep the scope of future patches down and err on the side of\ncaution when I review my own changes before submitting. Thank you for\nthe *pointers.\n\nI'm going to give v4 a shot. It should be on the mailing list in an hour or so.\n\nQuint\n"}]}