{"thread":{"id":"36202","subject":"[PATCH v4 0/1] general style: replaces memcmp() with starts_with()","startedAt":"2014-03-17T21:52:23Z","lastAt":"2014-03-18T19:23:31Z","messageCount":12,"participants":["Quint Guvernator","Junio C Hamano","Jeff King","Eric Sunshine"],"isPatch":true,"patchVersion":4,"patchTotal":1},"messages":[{"id":"236922","messageId":"1395093144-6786-1-git-send-email-quintus.public@gmail.com","threadId":"36202","inReplyTo":null,"subject":"[PATCH v4 0/1] general style: replaces memcmp() with starts_with()","fromName":"Quint Guvernator","fromEmail":"quintus.public@gmail.com","sentAt":"2014-03-17T21:52:23Z","receivedAt":"2014-03-17T21:52:23Z","isPatch":true,"sender":{"key":"quintus.public@gmail.com","avatar":"https://gravatar.com/avatar/0805695585d6191e00badd5057b262b501047d9fb2b22dd2495009ffb25d85fd?d=mp&s=160"},"body":"Hi again, all.\n\nI've gone through the patch again to filter for the use of magic numbers so\nthat I can leave those hunks alone. Junio says, and Michael agrees, that:\n\n> The original hunks show that the code knows and relies on magic numbers 7\n> and 8 very clearly and there are rooms for improvement.  The result after\n> the conversion, however, still have the same magic numbers, but one less\n> of them each.  Doesn't it make it harder to later spot the patterns to\n> come up with a better abstraction that does not rely on the magic number?\n\nI cut this one down very sharply; Michael says:\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 wall\n> and asking the reviewer to pick between the good and the bad.\n\nThanks for the guidance, everyone.\n\nThis work is microproject #14 for GSoC.\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/mktag.c        |  4 ++--\n builtin/patch-id.c     | 10 +++++-----\n connect.c              |  4 ++--\n imap-send.c            |  6 +++---\n remote.c               |  2 +-\n 7 files changed, 17 insertions(+), 17 deletions(-)\n\n-- \n1.9.0\n"},{"id":"236923","messageId":"1395093144-6786-2-git-send-email-quintus.public@gmail.com","threadId":"36202","inReplyTo":"1395093144-6786-1-git-send-email-quintus.public@gmail.com","subject":"[PATCH 1/1] general style: replaces memcmp() with starts_with()","fromName":"Quint Guvernator","fromEmail":"quintus.public@gmail.com","sentAt":"2014-03-17T21:52:24Z","receivedAt":"2014-03-17T21:52:24Z","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/mktag.c        |  4 ++--\n builtin/patch-id.c     | 10 +++++-----\n connect.c              |  4 ++--\n imap-send.c            |  6 +++---\n remote.c               |  2 +-\n 7 files changed, 17 insertions(+), 17 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/mktag.c b/builtin/mktag.c\nindex 640ab64..640c28f 100644\n--- a/builtin/mktag.c\n+++ b/builtin/mktag.c\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 \ndiff --git a/builtin/patch-id.c b/builtin/patch-id.c\nindex 3cfe02d..23ef424 100644\n--- a/builtin/patch-id.c\n+++ b/builtin/patch-id.c\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/connect.c b/connect.c\nindex 4150412..5ae2aaa 100644\n--- a/connect.c\n+++ b/connect.c\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/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/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":"236925","messageId":"CALs4jVFY=E3Ejh81hxhH62F=dh31jP7_Tmx0tFmfGqAsr+_n4A@mail.gmail.com","threadId":"36202","inReplyTo":"1395093144-6786-1-git-send-email-quintus.public@gmail.com","subject":"Re: [PATCH v4 0/1] general style: replaces memcmp() with starts_with()","fromName":"Quint Guvernator","fromEmail":"quintus.public@gmail.com","sentAt":"2014-03-17T21:53:45Z","receivedAt":"2014-03-17T21:53:45Z","isPatch":true,"sender":{"key":"quintus.public@gmail.com","avatar":"https://gravatar.com/avatar/0805695585d6191e00badd5057b262b501047d9fb2b22dd2495009ffb25d85fd?d=mp&s=160"},"body":"My mistake, folks. This is [PATCH v4]. Apologies for the confusion.\n\nQuint\n"},{"id":"236935","messageId":"xmqqob14a14s.fsf@gitster.dls.corp.google.com","threadId":"36202","inReplyTo":"1395093144-6786-2-git-send-email-quintus.public@gmail.com","subject":"Re: [PATCH 1/1] general style: replaces memcmp() with starts_with()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-17T22:52:51Z","receivedAt":"2014-03-17T22:52:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Quint Guvernator <quintus.public@gmail.com> writes:\n\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\nThanks.  This probably needs retitled, though (hint: \"replaces\"?\nwho does so?) and the message rewritten (see numerous reviews on\nother GSoC micros from Eric).\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\nThe above two looks sensible.\n\nI sense that there is a bonus point for an independent follow-up\npatch to unify the conflicting definitions of what an incomplete\nline should look like.  Hint, hint...\n\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\nIf there were a variant of starts_with() that works on a counted\nstring, and rewriting this using it to\n\n\twhile (starts_with_counted(line, size, \"@@ -\")) {\n\nwould make perfect sense, but as written above, I do not think it is\nan improvement.\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\nGood.\n\n> diff --git a/builtin/mktag.c b/builtin/mktag.c\n> index 640ab64..640c28f 100644\n> --- a/builtin/mktag.c\n> +++ b/builtin/mktag.c\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\nGood.\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\nNot quite.  I suspect that what actually makes this strange and\ntricky is that this \"no tag found\" check is misplaced.  It found the\ntype line, expects that the next line is a tag line, and instead of\nvalidating the remainder of type line, it does this thing, and then\nverifies the actual type string, and for that, it needs tag_line\nvariable to stay where it is.\n\nIf we flipped the order of things around the codepath a bit, then we\nshould be able to first validate the type line, and then use\nskip-prefix to skip the \"tag \" part (while validating that that line\nactually begins with \"tag \") and check the tag name is a non-empty\nstring that consists of a good character.  All of that is a topic\nfor a separate patch.\n\n> diff --git a/builtin/patch-id.c b/builtin/patch-id.c\n> index 3cfe02d..23ef424 100644\n> --- a/builtin/patch-id.c\n> +++ b/builtin/patch-id.c\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>  \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\nLooks good.\n\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\nLooks good.\n\n> diff --git a/connect.c b/connect.c\n> index 4150412..5ae2aaa 100644\n> --- a/connect.c\n> +++ b/connect.c\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\nLooks good.\n\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\nLooks good.\n\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\nLooks good.\n\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\nLooks good.\n"},{"id":"236937","messageId":"20140317230139.GB19578@sigill.intra.peff.net","threadId":"36202","inReplyTo":"xmqqob14a14s.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/1] general style: replaces memcmp() with starts_with()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-17T23:01:39Z","receivedAt":"2014-03-17T23:01:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 17, 2014 at 03:52:51PM -0700, Junio C Hamano wrote:\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> \n> Good.\n\nActually, I found this one confusing. We are looking for \"color:\", but\nif we find it, we _don't_ skip past and look at what comes after.\nInstead, we compare the whole string. Which works because color_reset\nactually contains \"color:reset\", and we end up just re-comparing the\nfirst bit of the string. So the memcmp here is redundant, and this can\nsimply become:\n\n  need_color_reset_at_eol = !!strcmp(used_atom[at], color_reset);\n\nOr am I missing something?\n\n-Peff\n"},{"id":"236940","messageId":"xmqqd2hka0h7.fsf@gitster.dls.corp.google.com","threadId":"36202","inReplyTo":"20140317230139.GB19578@sigill.intra.peff.net","subject":"Re: [PATCH 1/1] general style: replaces memcmp() with starts_with()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-17T23:07:00Z","receivedAt":"2014-03-17T23:07:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Mon, Mar 17, 2014 at 03:52:51PM -0700, Junio C Hamano wrote:\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>> \n>> Good.\n>\n> Actually, I found this one confusing. We are looking for \"color:\", but\n> if we find it, we _don't_ skip past and look at what comes after.\n> Instead, we compare the whole string. Which works because color_reset\n> actually contains \"color:reset\", and we end up just re-comparing the\n> first bit of the string. So the memcmp here is redundant, and this can\n> simply become:\n>\n>   need_color_reset_at_eol = !!strcmp(used_atom[at], color_reset);\n>\n> Or am I missing something?\n\nWhat if used_atom[at] is not related to color at all?  We do not\nwant to touch the variable.\n"},{"id":"236943","messageId":"CALs4jVGxmirDdO03kaKco7=NrTaXBinY=1U71=fYwX1mbqNDOA@mail.gmail.com","threadId":"36202","inReplyTo":"xmqqob14a14s.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/1] general style: replaces memcmp() with starts_with()","fromName":"Quint Guvernator","fromEmail":"quintus.public@gmail.com","sentAt":"2014-03-17T23:46:12Z","receivedAt":"2014-03-17T23:46:12Z","isPatch":true,"sender":{"key":"quintus.public@gmail.com","avatar":"https://gravatar.com/avatar/0805695585d6191e00badd5057b262b501047d9fb2b22dd2495009ffb25d85fd?d=mp&s=160"},"body":"2014-03-17 18:52 GMT-04:00 Junio C Hamano <gitster@pobox.com>:\n> Thanks.  This probably needs retitled, though (hint: \"replaces\"?\n> who does so?) and the message rewritten (see numerous reviews on\n> other GSoC micros from Eric).\n\nI found some messages [1] by Eric concerning imperative voice (\"simplify\"\nrather than \"simplifies/ed\").\n\nOther than the change of verb, what sort of changes are you looking for in\nthe description? It doesn't look much different than, for instance, this\n[2] commit in the log.\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/243848\n[2]: https://github.com/git/git/commit/0eea5a6e91d3da6932c13f16fdf4b4e5ed91b93c\n\n> I sense that there is a bonus point for an independent follow-up\n> patch to unify the conflicting definitions of what an incomplete\n> line should look like.  Hint, hint...\n\nI'll try to make the time to follow up on that, if I can think of a good\nclear solution for the conflict. I'm also a full-time student, but I will\ncertainly give it a shot.\n\n>> @@ -1673,7 +1673,7 @@ static int parse_single_patch(const char *line, unsigned long size, struct patch\n>>       unsigned long oldlines = 0, newlines = 0, context = 0;\n>>       struct fragment **fragp = &patch->fragments;\n>>\n>> -     while (size > 4 && !memcmp(line, \"@@ -\", 4)) {\n>> +     while (size > 4 && starts_with(line, \"@@ -\")) {\n>\n> If there were a variant of starts_with() that works on a counted\n> string, and rewriting this using it to\n>\n>         while (starts_with_counted(line, size, \"@@ -\")) {\n>\n> would make perfect sense, but as written above, I do not think it is\n> an improvement.\n\nThis still feels to me like an improvement from the !memcmp line, but if\nyou think we need to wait for a full helper-function revamp, let's drop it.\n\n>> @@ -66,7 +66,7 @@ static int verify_tag(char *buffer, unsigned long size)\n>>               return error(\"char%\"PRIuMAX\": could not find next \\\"\\\\n\\\"\",\n>>                               (uintmax_t) (type_line - buffer));\n>>       tag_line++;\n>> -     if (memcmp(tag_line, \"tag \", 4) || tag_line[4] == '\\n')\n>> +     if (!starts_with(tag_line, \"tag \") || tag_line[4] == '\\n')\n>>               return error(\"char%\"PRIuMAX\": no \\\"tag \\\" found\",\n>>                               (uintmax_t) (tag_line - buffer));\n>\n> Not quite.  I suspect that what actually makes this strange and\n> tricky is that this \"no tag found\" check is misplaced.  It found the\n> type line, expects that the next line is a tag line, and instead of\n> validating the remainder of type line, it does this thing, and then\n> verifies the actual type string, and for that, it needs tag_line\n> variable to stay where it is.\n>\n> If we flipped the order of things around the codepath a bit, then we\n> should be able to first validate the type line, and then use\n> skip-prefix to skip the \"tag \" part (while validating that that line\n> actually begins with \"tag \") and check the tag name is a non-empty\n> string that consists of a good character.  All of that is a topic\n> for a separate patch.\n\nThat's tricky. Okay, let's definitely drop this hunk.\n\nShall I submit a new [PATCH v5] with these changes to the mailing list or\ndirectly to you, or is everything in order?\n\nThanks for taking the time to review this. I really appreciate the\nfeedback.\n\nQuint\n"},{"id":"236950","messageId":"CAPig+cRGwBV5CE3X9yGZGRNtKojCagz_F_f4Kd+S+D_qX+ZwJw@mail.gmail.com","threadId":"36202","inReplyTo":"CALs4jVGxmirDdO03kaKco7=NrTaXBinY=1U71=fYwX1mbqNDOA@mail.gmail.com","subject":"Re: [PATCH 1/1] general style: replaces memcmp() with starts_with()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-18T01:59:26Z","receivedAt":"2014-03-18T01:59:26Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Mar 17, 2014 at 7:46 PM, Quint Guvernator\n<quintus.public@gmail.com> wrote:\n> 2014-03-17 18:52 GMT-04:00 Junio C Hamano <gitster@pobox.com>:\n>> Thanks.  This probably needs retitled, though (hint: \"replaces\"?\n>> who does so?) and the message rewritten (see numerous reviews on\n>> other GSoC micros from Eric).\n>\n> I found some messages [1] by Eric concerning imperative voice (\"simplify\"\n> rather than \"simplifies/ed\").\n>\n> Other than the change of verb, what sort of changes are you looking for in\n> the description? It doesn't look much different than, for instance, this\n> [2] commit in the log.\n>\n> [1]: http://article.gmane.org/gmane.comp.version-control.git/243848\n> [2]: https://github.com/git/git/commit/0eea5a6e91d3da6932c13f16fdf4b4e5ed91b93c\n\nI can't speak for Junio, but the description could be made more\nconcise and to-the-point. Aside from using imperative voice, you can\neliminate redundancy, some of which comes from repeating in prose what\nthe patch itself already states more concisely and precisely, and some\nfrom repeating what is implied by the fact that you're making such a\nchange in the first place. Here's your original:\n\n    Subject: general style: replaces memcmp() with starts_with()\n\n    memcmp() is replaced with negated starts_with() when comparing\n    strings from the beginning and when it is logical to do\n    so. starts_with() looks nicer and it saves the extra argument of\n    the length of the comparing string.\n\nIn the subject, \"general style\" is a bit unusual. This isn't just a\nstylistic change; it's intended to improve code clarity.\n\nExamples of redundancy:\n\n\"memcmp() is replaced with ...\": The subject already says this.\n\n\"negated starts_with()\": Having to negate the value is a necessary\nartifact of switching to starts_with(), thus it's a mere\nimplementation detail of the change. There is no mystery here. Anyone\nfamiliar with memcmp() and starts_with() will understand implicitly\nwhy the value is negated.\n\n\"when comparing strings from the beginning\": That's effectively\nimplied by the name starts_with(). (And, if you did happen use\nstarts_with() at a location other than the start-of-string, a reviewer\nwould likely point out that doing so makes the code less readable.)\n\n\"when it is logical to do so\": The scope of the patch already implies\nthat the changes are restricted to cases when it is logical to do so\n(and if it's not, a reviewer will question the illogical changes).\n\n\"starts_with() looks nicer\": Subjective, as written. Reworded to be\nmore forceful, it might make a decent justification for the patch (see\nbelow).\n\n\"saves the extra argument\": This is incidental to the real change,\nwhich is to make the code read more clearly, and is an obvious\nartifact of switching from memcmp() to starts_with().\n\nA patch of this nature doesn't require much more description than\nstating what it does (\"replace memcmp() with starts_with()\") and why\n(\"improve code clarity\"). The following rewrite might be sufficient:\n\n    Subject: replace memcmp() with starts_with()\n\n    starts_with() indicates the intention of the check more clearly\n    than memcmp().\n"},{"id":"236951","messageId":"CALs4jVFVO8Jf-nn0PMtOYeMMKsB869KWQv8vV5kAg1itC1isiQ@mail.gmail.com","threadId":"36202","inReplyTo":"CAPig+cRGwBV5CE3X9yGZGRNtKojCagz_F_f4Kd+S+D_qX+ZwJw@mail.gmail.com","subject":"Re: [PATCH 1/1] general style: replaces memcmp() with starts_with()","fromName":"Quint Guvernator","fromEmail":"quintus.public@gmail.com","sentAt":"2014-03-18T02:33:30Z","receivedAt":"2014-03-18T02:33:30Z","isPatch":true,"sender":{"key":"quintus.public@gmail.com","avatar":"https://gravatar.com/avatar/0805695585d6191e00badd5057b262b501047d9fb2b22dd2495009ffb25d85fd?d=mp&s=160"},"body":"2014-03-17 21:59 GMT-04:00 Eric Sunshine <sunshine@sunshineco.com>:\n> I can't speak for Junio, but the description could be made more\n> concise and to-the-point. Aside from using imperative voice, you can\n> eliminate redundancy, some of which comes from repeating in prose what\n> the patch itself already states more concisely and precisely, and some\n> from repeating what is implied by the fact that you're making such a\n> change in the first place.\n\nWow, thanks for the detailed review. This mail will be something to\nwhich I can refer when making future changes.\n\n> In the subject, \"general style\" is a bit unusual. This isn't just a\n> stylistic change; it's intended to improve code clarity.\n\nIt felt a little awkward, but I was trying to follow our guide [1] for\ncommit messages. It is all right to omit the leading identifier?\n\n[1]: https://github.com/git/git/blob/fca26a/Documentation/SubmittingPatches#L87-L116\n\n> A patch of this nature doesn't require much more description than\n> stating what it does (\"replace memcmp() with starts_with()\") and why\n> (\"improve code clarity\"). The following rewrite might be sufficient:\n>\n>     Subject: replace memcmp() with starts_with()\n>\n>     starts_with() indicates the intention of the check more clearly\n>     than memcmp().\n\nThis is more concise; thank you. I will adapt this as the message for\nthe next version of this patch. Would it be wise to mention magic\nnumbers, as the discussion surrounding the rationale of this patch,\nespecially with Junio and Michael, has centered around that?\n\nThank you for the feedback,\nQuint\n"},{"id":"236952","messageId":"20140318024134.GA644@sigill.intra.peff.net","threadId":"36202","inReplyTo":"xmqqd2hka0h7.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/1] general style: replaces memcmp() with starts_with()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-18T02:41:34Z","receivedAt":"2014-03-18T02:41:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 17, 2014 at 04:07:00PM -0700, Junio C Hamano wrote:\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> [...]\n> What if used_atom[at] is not related to color at all?  We do not\n> want to touch the variable.\n\nThanks, that is what I was missing. It is not \"did we find a reset\" but\n\"toggle on for a non-reset color, toggle off for a reset\".\n\nIt could be written with skip_prefix as:\n\n  if (skip_prefix(used_atom[at], \"color:\", &c))\n          need_color_reset_at_eol = !!strcmp(c, \"reset\");\n\nbut I do not think it is particularly important to do so. Sorry for the\nnoise.\n\n-Peff\n"},{"id":"236953","messageId":"CAPig+cSu=zgXdd55dj-u6mOzHqpUme7r-7OH+jjnOEt2P6C0vw@mail.gmail.com","threadId":"36202","inReplyTo":"CALs4jVFVO8Jf-nn0PMtOYeMMKsB869KWQv8vV5kAg1itC1isiQ@mail.gmail.com","subject":"Re: [PATCH 1/1] general style: replaces memcmp() with starts_with()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-18T04:07:41Z","receivedAt":"2014-03-18T04:07:41Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Mar 17, 2014 at 10:33 PM, Quint Guvernator\n<quintus.public@gmail.com> wrote:\n> 2014-03-17 21:59 GMT-04:00 Eric Sunshine <sunshine@sunshineco.com>:\n>> I can't speak for Junio, but the description could be made more\n>> concise and to-the-point. Aside from using imperative voice, you can\n>> eliminate redundancy, some of which comes from repeating in prose what\n>> the patch itself already states more concisely and precisely, and some\n>> from repeating what is implied by the fact that you're making such a\n>> change in the first place.\n>\n> Wow, thanks for the detailed review. This mail will be something to\n> which I can refer when making future changes.\n>\n>> In the subject, \"general style\" is a bit unusual. This isn't just a\n>> stylistic change; it's intended to improve code clarity.\n>\n> It felt a little awkward, but I was trying to follow our guide [1] for\n> commit messages. It is all right to omit the leading identifier?\n\nSince this particular patch touches so many different files and\nfunctions, it is difficult to craft a suitable leading identifier. The\nalternative would be to split the patch into smaller pieces.\nUltimately, as the project maintainer, Junio must be the one to decide\nif the monolithic patch lacking leading identifier is sufficient or if\nsmaller patches would be preferred.\n\n> [1]: https://github.com/git/git/blob/fca26a/Documentation/SubmittingPatches#L87-L116\n>\n>> A patch of this nature doesn't require much more description than\n>> stating what it does (\"replace memcmp() with starts_with()\") and why\n>> (\"improve code clarity\"). The following rewrite might be sufficient:\n>>\n>>     Subject: replace memcmp() with starts_with()\n>>\n>>     starts_with() indicates the intention of the check more clearly\n>>     than memcmp().\n>\n> This is more concise; thank you. I will adapt this as the message for\n> the next version of this patch. Would it be wise to mention magic\n> numbers, as the discussion surrounding the rationale of this patch,\n> especially with Junio and Michael, has centered around that?\n\nI was thinking of mentioning magic numbers in the example, but decided\nthat their removal was a natural and obvious consequence of the\nimprovement to code clarity, so it wasn't strictly necessary to talk\nabout it. On the other hand, it is a good secondary justification,\nthus it should be perfectly acceptable to mention it. If I was writing\nthe commit message, I might start by saying \"As an additional\nbenefit...\" and then talk a bit about magic number retirement.\n"},{"id":"236999","messageId":"xmqqiorb8g5o.fsf@gitster.dls.corp.google.com","threadId":"36202","inReplyTo":"CAPig+cSu=zgXdd55dj-u6mOzHqpUme7r-7OH+jjnOEt2P6C0vw@mail.gmail.com","subject":"Re: [PATCH 1/1] general style: replaces memcmp() with starts_with()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-18T19:23:31Z","receivedAt":"2014-03-18T19:23:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n>>> A patch of this nature doesn't require much more description than\n>>> stating what it does (\"replace memcmp() with starts_with()\") and why\n>>> (\"improve code clarity\"). The following rewrite might be sufficient:\n>>>\n>>>     Subject: replace memcmp() with starts_with()\n>>>\n>>>     starts_with() indicates the intention of the check more clearly\n>>>     than memcmp().\n>>\n>> This is more concise; thank you. I will adapt this as the message for\n>> the next version of this patch. Would it be wise to mention magic\n>> numbers, as the discussion surrounding the rationale of this patch,\n>> especially with Junio and Michael, has centered around that?\n>\n> I was thinking of mentioning magic numbers in the example, but decided\n> that their removal was a natural and obvious consequence of the\n> improvement to code clarity, so it wasn't strictly necessary to talk\n> about it. On the other hand, it is a good secondary justification,\n> thus it should be perfectly acceptable to mention it. If I was writing\n> the commit message, I might start by saying \"As an additional\n> benefit...\" and then talk a bit about magic number retirement.\n\nI think your subject is good (as you said, it makes it clear that it\nis a project-wide clean-up by not mentioning any specific area), but\n\"indicates the intention of the check more clearly\" would not tell\nnew readers who are unfamiliar with the helper API what \"intention\"\nit is talking about very much, so perhaps\n\n\tSubject: use starts_with() instead of !memcmp()\n\n\tWhen checking if a string begins with a constant string,\n\tusing starts_with() is less error prone than calling\n\t!memcmp() with an explicit byte count.\n\nor something?\n"}]}