[PATCH v4 0/1] general style: replaces memcmp() with starts_with()

[PATCH v4 0/1] general style: replaces memcmp() with starts_with()

[Quint Guvernator]

Hi again, all.

I've gone through the patch again to filter for the use of magic numbers so
that I can leave those hunks alone. Junio says, and Michael agrees, that:

> The original hunks show that the code knows and relies on magic numbers 7
> and 8 very clearly and there are rooms for improvement.  The result after
> the conversion, however, still have the same magic numbers, but one less
> of them each.  Doesn't it make it harder to later spot the patterns to
> come up with a better abstraction that does not rely on the magic number?

I cut this one down very sharply; Michael says:

> It would be much better for you to submit only a handful of changes (or
> even only one!) that is perfect, rather than throwing a bunch at the wall
> and asking the reviewer to pick between the good and the bad.

Thanks for the guidance, everyone.

This work is microproject #14 for GSoC.

Quint Guvernator (1):
  general style: replaces memcmp() with starts_with()

 builtin/apply.c        |  6 +++---
 builtin/for-each-ref.c |  2 +-
 builtin/mktag.c        |  4 ++--
 builtin/patch-id.c     | 10 +++++-----
 connect.c              |  4 ++--
 imap-send.c            |  6 +++---
 remote.c               |  2 +-
 7 files changed, 17 insertions(+), 17 deletions(-)

-- 
1.9.0

[PATCH 1/1] general style: replaces memcmp() with starts_with()

[Quint Guvernator]

memcmp() is replaced with negated starts_with() when comparing strings
from the beginning and when it is logical to do so. starts_with() looks
nicer and it saves the extra argument of the length of the comparing
string.

Signed-off-by: Quint Guvernator <quintus.public@gmail.com>
---
 builtin/apply.c        |  6 +++---
 builtin/for-each-ref.c |  2 +-
 builtin/mktag.c        |  4 ++--
 builtin/patch-id.c     | 10 +++++-----
 connect.c              |  4 ++--
 imap-send.c            |  6 +++---
 remote.c               |  2 +-
 7 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 0189523..de84dce 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -1631,7 +1631,7 @@ static int parse_fragment(const char *line, unsigned long size,
 		 * l10n of "\ No newline..." is at least that long.
 		 */
 		case '\\':
-			if (len < 12 || memcmp(line, "\\ ", 2))
+			if (len < 12 || !starts_with(line, "\\ "))
 				return -1;
 			break;
 		}
@@ -1646,7 +1646,7 @@ static int parse_fragment(const char *line, unsigned long size,
 	 * it in the above loop because we hit oldlines == newlines == 0
 	 * before seeing it.
 	 */
-	if (12 < size && !memcmp(line, "\\ ", 2))
+	if (12 < size && starts_with(line, "\\ "))
 		offset += linelen(line, size);
 
 	patch->lines_added += added;
@@ -1673,7 +1673,7 @@ static int parse_single_patch(const char *line, unsigned long size, struct patch
 	unsigned long oldlines = 0, newlines = 0, context = 0;
 	struct fragment **fragp = &patch->fragments;
 
-	while (size > 4 && !memcmp(line, "@@ -", 4)) {
+	while (size > 4 && starts_with(line, "@@ -")) {
 		struct fragment *fragment;
 		int len;
 
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 3e1d5c3..4135980 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -193,7 +193,7 @@ static int verify_format(const char *format)
 		at = parse_atom(sp + 2, ep);
 		cp = ep + 1;
 
-		if (!memcmp(used_atom[at], "color:", 6))
+		if (starts_with(used_atom[at], "color:"))
 			need_color_reset_at_eol = !!strcmp(used_atom[at], color_reset);
 	}
 	return 0;
diff --git a/builtin/mktag.c b/builtin/mktag.c
index 640ab64..640c28f 100644
--- a/builtin/mktag.c
+++ b/builtin/mktag.c
@@ -57,7 +57,7 @@ static int verify_tag(char *buffer, unsigned long size)
 
 	/* Verify type line */
 	type_line = object + 48;
-	if (memcmp(type_line - 1, "\ntype ", 6))
+	if (!starts_with(type_line - 1, "\ntype "))
 		return error("char%d: could not find \"\\ntype \"", 47);
 
 	/* Verify tag-line */
@@ -66,7 +66,7 @@ static int verify_tag(char *buffer, unsigned long size)
 		return error("char%"PRIuMAX": could not find next \"\\n\"",
 				(uintmax_t) (type_line - buffer));
 	tag_line++;
-	if (memcmp(tag_line, "tag ", 4) || tag_line[4] == '\n')
+	if (!starts_with(tag_line, "tag ") || tag_line[4] == '\n')
 		return error("char%"PRIuMAX": no \"tag \" found",
 				(uintmax_t) (tag_line - buffer));
 
diff --git a/builtin/patch-id.c b/builtin/patch-id.c
index 3cfe02d..23ef424 100644
--- a/builtin/patch-id.c
+++ b/builtin/patch-id.c
@@ -81,14 +81,14 @@ static int get_one_patchid(unsigned char *next_sha1, git_SHA_CTX *ctx, struct st
 		}
 
 		/* Ignore commit comments */
-		if (!patchlen && memcmp(line, "diff ", 5))
+		if (!patchlen && !starts_with(line, "diff "))
 			continue;
 
 		/* Parsing diff header?  */
 		if (before == -1) {
-			if (!memcmp(line, "index ", 6))
+			if (starts_with(line, "index "))
 				continue;
-			else if (!memcmp(line, "--- ", 4))
+			else if (starts_with(line, "--- "))
 				before = after = 1;
 			else if (!isalpha(line[0]))
 				break;
@@ -96,14 +96,14 @@ static int get_one_patchid(unsigned char *next_sha1, git_SHA_CTX *ctx, struct st
 
 		/* Looking for a valid hunk header?  */
 		if (before == 0 && after == 0) {
-			if (!memcmp(line, "@@ -", 4)) {
+			if (starts_with(line, "@@ -")) {
 				/* Parse next hunk, but ignore line numbers.  */
 				scan_hunk_header(line, &before, &after);
 				continue;
 			}
 
 			/* Split at the end of the patch.  */
-			if (memcmp(line, "diff ", 5))
+			if (!starts_with(line, "diff "))
 				break;
 
 			/* Else we're parsing another header.  */
diff --git a/connect.c b/connect.c
index 4150412..5ae2aaa 100644
--- a/connect.c
+++ b/connect.c
@@ -30,11 +30,11 @@ static int check_ref(const char *name, int len, unsigned int flags)
 		return 0;
 
 	/* REF_HEADS means that we want regular branch heads */
-	if ((flags & REF_HEADS) && !memcmp(name, "heads/", 6))
+	if ((flags & REF_HEADS) && starts_with(name, "heads/"))
 		return 1;
 
 	/* REF_TAGS means that we want tags */
-	if ((flags & REF_TAGS) && !memcmp(name, "tags/", 5))
+	if ((flags & REF_TAGS) && starts_with(name, "tags/"))
 		return 1;
 
 	/* All type bits clear means that we are ok with anything */
diff --git a/imap-send.c b/imap-send.c
index 0bc6f7f..019de18 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -524,7 +524,7 @@ static struct imap_cmd *v_issue_imap_cmd(struct imap_store *ctx,
 	if (Verbose) {
 		if (imap->num_in_progress)
 			printf("(%d in progress) ", imap->num_in_progress);
-		if (memcmp(cmd->cmd, "LOGIN", 5))
+		if (!starts_with(cmd->cmd, "LOGIN"))
 			printf(">>> %s", buf);
 		else
 			printf(">>> %d LOGIN <user> <pass>\n", cmd->tag);
@@ -802,7 +802,7 @@ static int get_cmd_result(struct imap_store *ctx, struct imap_cmd *tcmd)
 				resp = DRV_OK;
 			else {
 				if (!strcmp("NO", arg)) {
-					if (cmdp->cb.create && cmd && (cmdp->cb.trycreate || !memcmp(cmd, "[TRYCREATE]", 11))) { /* SELECT, APPEND or UID COPY */
+					if (cmdp->cb.create && cmd && (cmdp->cb.trycreate || starts_with(cmd, "[TRYCREATE]"))) { /* SELECT, APPEND or UID COPY */
 						p = strchr(cmdp->cmd, '"');
 						if (!issue_imap_cmd(ctx, NULL, "CREATE \"%.*s\"", (int)(strchr(p + 1, '"') - p + 1), p)) {
 							resp = RESP_BAD;
@@ -827,7 +827,7 @@ static int get_cmd_result(struct imap_store *ctx, struct imap_cmd *tcmd)
 				} else /*if (!strcmp("BAD", arg))*/
 					resp = RESP_BAD;
 				fprintf(stderr, "IMAP command '%s' returned response (%s) - %s\n",
-					 memcmp(cmdp->cmd, "LOGIN", 5) ?
+					 !starts_with(cmdp->cmd, "LOGIN") ?
 							cmdp->cmd : "LOGIN <user> <pass>",
 							arg, cmd ? cmd : "");
 			}
diff --git a/remote.c b/remote.c
index 5f63d55..bd02b0e 100644
--- a/remote.c
+++ b/remote.c
@@ -1149,7 +1149,7 @@ static int match_explicit(struct ref *src, struct ref *dst,
 	case 1:
 		break;
 	case 0:
-		if (!memcmp(dst_value, "refs/", 5))
+		if (starts_with(dst_value, "refs/"))
 			matched_dst = make_linked_ref(dst_value, dst_tail);
 		else if (is_null_sha1(matched_src->new_sha1))
 			error("unable to delete '%s': remote ref does not exist",
-- 
1.9.0

Re: [PATCH v4 0/1] general style: replaces memcmp() with starts_with()

[Quint Guvernator]

My mistake, folks. This is [PATCH v4]. Apologies for the confusion.

Quint

Re: [PATCH 1/1] general style: replaces memcmp() with starts_with()

[Junio C Hamano]

Quint Guvernator <quintus.public@gmail.com> writes:

> memcmp() is replaced with negated starts_with() when comparing strings
> from the beginning and when it is logical to do so. starts_with() looks
> nicer and it saves the extra argument of the length of the comparing
> string.
>
> Signed-off-by: Quint Guvernator <quintus.public@gmail.com>
> ---

Thanks.  This probably needs retitled, though (hint: "replaces"?
who does so?) and the message rewritten (see numerous reviews on
other GSoC micros from Eric).

> diff --git a/builtin/apply.c b/builtin/apply.c
> index 0189523..de84dce 100644
> --- a/builtin/apply.c
> +++ b/builtin/apply.c
> @@ -1631,7 +1631,7 @@ static int parse_fragment(const char *line, unsigned long size,
>  		 * l10n of "\ No newline..." is at least that long.
>  		 */
>  		case '\\':
> -			if (len < 12 || memcmp(line, "\\ ", 2))
> +			if (len < 12 || !starts_with(line, "\\ "))
>  				return -1;
>  			break;
>  		}
> @@ -1646,7 +1646,7 @@ static int parse_fragment(const char *line, unsigned long size,
>  	 * it in the above loop because we hit oldlines == newlines == 0
>  	 * before seeing it.
>  	 */
> -	if (12 < size && !memcmp(line, "\\ ", 2))
> +	if (12 < size && starts_with(line, "\\ "))
>  		offset += linelen(line, size);
>  
>  	patch->lines_added += added;

The above two looks sensible.

I sense that there is a bonus point for an independent follow-up
patch to unify the conflicting definitions of what an incomplete
line should look like.  Hint, hint...

> @@ -1673,7 +1673,7 @@ static int parse_single_patch(const char *line, unsigned long size, struct patch
>  	unsigned long oldlines = 0, newlines = 0, context = 0;
>  	struct fragment **fragp = &patch->fragments;
>  
> -	while (size > 4 && !memcmp(line, "@@ -", 4)) {
> +	while (size > 4 && starts_with(line, "@@ -")) {

If there were a variant of starts_with() that works on a counted
string, and rewriting this using it to

	while (starts_with_counted(line, size, "@@ -")) {

would make perfect sense, but as written above, I do not think it is
an improvement.

> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
> index 3e1d5c3..4135980 100644
> --- a/builtin/for-each-ref.c
> +++ b/builtin/for-each-ref.c
> @@ -193,7 +193,7 @@ static int verify_format(const char *format)
>  		at = parse_atom(sp + 2, ep);
>  		cp = ep + 1;
>  
> -		if (!memcmp(used_atom[at], "color:", 6))
> +		if (starts_with(used_atom[at], "color:"))
>  			need_color_reset_at_eol = !!strcmp(used_atom[at], color_reset);
>  	}
>  	return 0;

Good.

> diff --git a/builtin/mktag.c b/builtin/mktag.c
> index 640ab64..640c28f 100644
> --- a/builtin/mktag.c
> +++ b/builtin/mktag.c
> @@ -57,7 +57,7 @@ static int verify_tag(char *buffer, unsigned long size)
>  
>  	/* Verify type line */
>  	type_line = object + 48;
> -	if (memcmp(type_line - 1, "\ntype ", 6))
> +	if (!starts_with(type_line - 1, "\ntype "))
>  		return error("char%d: could not find \"\\ntype \"", 47);
>  
>  	/* Verify tag-line */

Good.

> @@ -66,7 +66,7 @@ static int verify_tag(char *buffer, unsigned long size)
>  		return error("char%"PRIuMAX": could not find next \"\\n\"",
>  				(uintmax_t) (type_line - buffer));
>  	tag_line++;
> -	if (memcmp(tag_line, "tag ", 4) || tag_line[4] == '\n')
> +	if (!starts_with(tag_line, "tag ") || tag_line[4] == '\n')
>  		return error("char%"PRIuMAX": no \"tag \" found",
>  				(uintmax_t) (tag_line - buffer));

Not quite.  I suspect that what actually makes this strange and
tricky is that this "no tag found" check is misplaced.  It found the
type line, expects that the next line is a tag line, and instead of
validating the remainder of type line, it does this thing, and then
verifies the actual type string, and for that, it needs tag_line
variable to stay where it is.

If we flipped the order of things around the codepath a bit, then we
should be able to first validate the type line, and then use
skip-prefix to skip the "tag " part (while validating that that line
actually begins with "tag ") and check the tag name is a non-empty
string that consists of a good character.  All of that is a topic
for a separate patch.

> diff --git a/builtin/patch-id.c b/builtin/patch-id.c
> index 3cfe02d..23ef424 100644
> --- a/builtin/patch-id.c
> +++ b/builtin/patch-id.c
> @@ -81,14 +81,14 @@ static int get_one_patchid(unsigned char *next_sha1, git_SHA_CTX *ctx, struct st
>  		}
>  
>  		/* Ignore commit comments */
> -		if (!patchlen && memcmp(line, "diff ", 5))
> +		if (!patchlen && !starts_with(line, "diff "))
>  			continue;
>  		/* Parsing diff header?  */
>  		if (before == -1) {
> -			if (!memcmp(line, "index ", 6))
> +			if (starts_with(line, "index "))
>  				continue;
> -			else if (!memcmp(line, "--- ", 4))
> +			else if (starts_with(line, "--- "))
>  				before = after = 1;
>  			else if (!isalpha(line[0]))
>  				break;

Looks good.

> @@ -96,14 +96,14 @@ static int get_one_patchid(unsigned char *next_sha1, git_SHA_CTX *ctx, struct st
>  
>  		/* Looking for a valid hunk header?  */
>  		if (before == 0 && after == 0) {
> -			if (!memcmp(line, "@@ -", 4)) {
> +			if (starts_with(line, "@@ -")) {
>  				/* Parse next hunk, but ignore line numbers.  */
>  				scan_hunk_header(line, &before, &after);
>  				continue;
>  			}
>  
>  			/* Split at the end of the patch.  */
> -			if (memcmp(line, "diff ", 5))
> +			if (!starts_with(line, "diff "))
>  				break;
>  
>  			/* Else we're parsing another header.  */

Looks good.

> diff --git a/connect.c b/connect.c
> index 4150412..5ae2aaa 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -30,11 +30,11 @@ static int check_ref(const char *name, int len, unsigned int flags)
>  		return 0;
>  
>  	/* REF_HEADS means that we want regular branch heads */
> -	if ((flags & REF_HEADS) && !memcmp(name, "heads/", 6))
> +	if ((flags & REF_HEADS) && starts_with(name, "heads/"))
>  		return 1;
>  
>  	/* REF_TAGS means that we want tags */
> -	if ((flags & REF_TAGS) && !memcmp(name, "tags/", 5))
> +	if ((flags & REF_TAGS) && starts_with(name, "tags/"))
>  		return 1;
>  
>  	/* All type bits clear means that we are ok with anything */

Looks good.

> diff --git a/imap-send.c b/imap-send.c
> index 0bc6f7f..019de18 100644
> --- a/imap-send.c
> +++ b/imap-send.c
> @@ -524,7 +524,7 @@ static struct imap_cmd *v_issue_imap_cmd(struct imap_store *ctx,
>  	if (Verbose) {
>  		if (imap->num_in_progress)
>  			printf("(%d in progress) ", imap->num_in_progress);
> -		if (memcmp(cmd->cmd, "LOGIN", 5))
> +		if (!starts_with(cmd->cmd, "LOGIN"))
>  			printf(">>> %s", buf);
>  		else
>  			printf(">>> %d LOGIN <user> <pass>\n", cmd->tag);

Looks good.

> @@ -802,7 +802,7 @@ static int get_cmd_result(struct imap_store *ctx, struct imap_cmd *tcmd)
>  				resp = DRV_OK;
>  			else {
>  				if (!strcmp("NO", arg)) {
> -					if (cmdp->cb.create && cmd && (cmdp->cb.trycreate || !memcmp(cmd, "[TRYCREATE]", 11))) { /* SELECT, APPEND or UID COPY */
> +					if (cmdp->cb.create && cmd && (cmdp->cb.trycreate || starts_with(cmd, "[TRYCREATE]"))) { /* SELECT, APPEND or UID COPY */
>  						p = strchr(cmdp->cmd, '"');
>  						if (!issue_imap_cmd(ctx, NULL, "CREATE \"%.*s\"", (int)(strchr(p + 1, '"') - p + 1), p)) {
>  							resp = RESP_BAD;
> @@ -827,7 +827,7 @@ static int get_cmd_result(struct imap_store *ctx, struct imap_cmd *tcmd)
>  				} else /*if (!strcmp("BAD", arg))*/
>  					resp = RESP_BAD;
>  				fprintf(stderr, "IMAP command '%s' returned response (%s) - %s\n",
> -					 memcmp(cmdp->cmd, "LOGIN", 5) ?
> +					 !starts_with(cmdp->cmd, "LOGIN") ?
>  							cmdp->cmd : "LOGIN <user> <pass>",
>  							arg, cmd ? cmd : "");
>  			}

Looks good.

> diff --git a/remote.c b/remote.c
> index 5f63d55..bd02b0e 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -1149,7 +1149,7 @@ static int match_explicit(struct ref *src, struct ref *dst,
>  	case 1:
>  		break;
>  	case 0:
> -		if (!memcmp(dst_value, "refs/", 5))
> +		if (starts_with(dst_value, "refs/"))
>  			matched_dst = make_linked_ref(dst_value, dst_tail);
>  		else if (is_null_sha1(matched_src->new_sha1))
>  			error("unable to delete '%s': remote ref does not exist",

Looks good.

Re: [PATCH 1/1] general style: replaces memcmp() with starts_with()

[Jeff King]

On Mon, Mar 17, 2014 at 03:52:51PM -0700, Junio C Hamano wrote:

> > diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
> > index 3e1d5c3..4135980 100644
> > --- a/builtin/for-each-ref.c
> > +++ b/builtin/for-each-ref.c
> > @@ -193,7 +193,7 @@ static int verify_format(const char *format)
> >  		at = parse_atom(sp + 2, ep);
> >  		cp = ep + 1;
> >  
> > -		if (!memcmp(used_atom[at], "color:", 6))
> > +		if (starts_with(used_atom[at], "color:"))
> >  			need_color_reset_at_eol = !!strcmp(used_atom[at], color_reset);
> >  	}
> >  	return 0;
> 
> Good.

Actually, I found this one confusing. We are looking for "color:", but
if we find it, we _don't_ skip past and look at what comes after.
Instead, we compare the whole string. Which works because color_reset
actually contains "color:reset", and we end up just re-comparing the
first bit of the string. So the memcmp here is redundant, and this can
simply become:

  need_color_reset_at_eol = !!strcmp(used_atom[at], color_reset);

Or am I missing something?

-Peff

Re: [PATCH 1/1] general style: replaces memcmp() with starts_with()

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Mar 17, 2014 at 03:52:51PM -0700, Junio C Hamano wrote:
>
>> > diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
>> > index 3e1d5c3..4135980 100644
>> > --- a/builtin/for-each-ref.c
>> > +++ b/builtin/for-each-ref.c
>> > @@ -193,7 +193,7 @@ static int verify_format(const char *format)
>> >  		at = parse_atom(sp + 2, ep);
>> >  		cp = ep + 1;
>> >  
>> > -		if (!memcmp(used_atom[at], "color:", 6))
>> > +		if (starts_with(used_atom[at], "color:"))
>> >  			need_color_reset_at_eol = !!strcmp(used_atom[at], color_reset);
>> >  	}
>> >  	return 0;
>> 
>> Good.
>
> Actually, I found this one confusing. We are looking for "color:", but
> if we find it, we _don't_ skip past and look at what comes after.
> Instead, we compare the whole string. Which works because color_reset
> actually contains "color:reset", and we end up just re-comparing the
> first bit of the string. So the memcmp here is redundant, and this can
> simply become:
>
>   need_color_reset_at_eol = !!strcmp(used_atom[at], color_reset);
>
> Or am I missing something?

What if used_atom[at] is not related to color at all?  We do not
want to touch the variable.

Re: [PATCH 1/1] general style: replaces memcmp() with starts_with()

[Quint Guvernator]

2014-03-17 18:52 GMT-04:00 Junio C Hamano <gitster@pobox.com>:
> Thanks.  This probably needs retitled, though (hint: "replaces"?
> who does so?) and the message rewritten (see numerous reviews on
> other GSoC micros from Eric).

I found some messages [1] by Eric concerning imperative voice ("simplify"
rather than "simplifies/ed").

Other than the change of verb, what sort of changes are you looking for in
the description? It doesn't look much different than, for instance, this
[2] commit in the log.

[1]: http://article.gmane.org/gmane.comp.version-control.git/243848
[2]: https://github.com/git/git/commit/0eea5a6e91d3da6932c13f16fdf4b4e5ed91b93c

> I sense that there is a bonus point for an independent follow-up
> patch to unify the conflicting definitions of what an incomplete
> line should look like.  Hint, hint...

I'll try to make the time to follow up on that, if I can think of a good
clear solution for the conflict. I'm also a full-time student, but I will
certainly give it a shot.

>> @@ -1673,7 +1673,7 @@ static int parse_single_patch(const char *line, unsigned long size, struct patch
>>       unsigned long oldlines = 0, newlines = 0, context = 0;
>>       struct fragment **fragp = &patch->fragments;
>>
>> -     while (size > 4 && !memcmp(line, "@@ -", 4)) {
>> +     while (size > 4 && starts_with(line, "@@ -")) {
>
> If there were a variant of starts_with() that works on a counted
> string, and rewriting this using it to
>
>         while (starts_with_counted(line, size, "@@ -")) {
>
> would make perfect sense, but as written above, I do not think it is
> an improvement.

This still feels to me like an improvement from the !memcmp line, but if
you think we need to wait for a full helper-function revamp, let's drop it.

>> @@ -66,7 +66,7 @@ static int verify_tag(char *buffer, unsigned long size)
>>               return error("char%"PRIuMAX": could not find next \"\\n\"",
>>                               (uintmax_t) (type_line - buffer));
>>       tag_line++;
>> -     if (memcmp(tag_line, "tag ", 4) || tag_line[4] == '\n')
>> +     if (!starts_with(tag_line, "tag ") || tag_line[4] == '\n')
>>               return error("char%"PRIuMAX": no \"tag \" found",
>>                               (uintmax_t) (tag_line - buffer));
>
> Not quite.  I suspect that what actually makes this strange and
> tricky is that this "no tag found" check is misplaced.  It found the
> type line, expects that the next line is a tag line, and instead of
> validating the remainder of type line, it does this thing, and then
> verifies the actual type string, and for that, it needs tag_line
> variable to stay where it is.
>
> If we flipped the order of things around the codepath a bit, then we
> should be able to first validate the type line, and then use
> skip-prefix to skip the "tag " part (while validating that that line
> actually begins with "tag ") and check the tag name is a non-empty
> string that consists of a good character.  All of that is a topic
> for a separate patch.

That's tricky. Okay, let's definitely drop this hunk.

Shall I submit a new [PATCH v5] with these changes to the mailing list or
directly to you, or is everything in order?

Thanks for taking the time to review this. I really appreciate the
feedback.

Quint

Re: [PATCH 1/1] general style: replaces memcmp() with starts_with()

[Eric Sunshine]

On Mon, Mar 17, 2014 at 7:46 PM, Quint Guvernator
<quintus.public@gmail.com> wrote:
> 2014-03-17 18:52 GMT-04:00 Junio C Hamano <gitster@pobox.com>:
>> Thanks.  This probably needs retitled, though (hint: "replaces"?
>> who does so?) and the message rewritten (see numerous reviews on
>> other GSoC micros from Eric).
>
> I found some messages [1] by Eric concerning imperative voice ("simplify"
> rather than "simplifies/ed").
>
> Other than the change of verb, what sort of changes are you looking for in
> the description? It doesn't look much different than, for instance, this
> [2] commit in the log.
>
> [1]: http://article.gmane.org/gmane.comp.version-control.git/243848
> [2]: https://github.com/git/git/commit/0eea5a6e91d3da6932c13f16fdf4b4e5ed91b93c

I can't speak for Junio, but the description could be made more
concise and to-the-point. Aside from using imperative voice, you can
eliminate redundancy, some of which comes from repeating in prose what
the patch itself already states more concisely and precisely, and some
from repeating what is implied by the fact that you're making such a
change in the first place. Here's your original:

    Subject: general style: replaces memcmp() with starts_with()

    memcmp() is replaced with negated starts_with() when comparing
    strings from the beginning and when it is logical to do
    so. starts_with() looks nicer and it saves the extra argument of
    the length of the comparing string.

In the subject, "general style" is a bit unusual. This isn't just a
stylistic change; it's intended to improve code clarity.

Examples of redundancy:

"memcmp() is replaced with ...": The subject already says this.

"negated starts_with()": Having to negate the value is a necessary
artifact of switching to starts_with(), thus it's a mere
implementation detail of the change. There is no mystery here. Anyone
familiar with memcmp() and starts_with() will understand implicitly
why the value is negated.

"when comparing strings from the beginning": That's effectively
implied by the name starts_with(). (And, if you did happen use
starts_with() at a location other than the start-of-string, a reviewer
would likely point out that doing so makes the code less readable.)

"when it is logical to do so": The scope of the patch already implies
that the changes are restricted to cases when it is logical to do so
(and if it's not, a reviewer will question the illogical changes).

"starts_with() looks nicer": Subjective, as written. Reworded to be
more forceful, it might make a decent justification for the patch (see
below).

"saves the extra argument": This is incidental to the real change,
which is to make the code read more clearly, and is an obvious
artifact of switching from memcmp() to starts_with().

A patch of this nature doesn't require much more description than
stating what it does ("replace memcmp() with starts_with()") and why
("improve code clarity"). The following rewrite might be sufficient:

    Subject: replace memcmp() with starts_with()

    starts_with() indicates the intention of the check more clearly
    than memcmp().

Re: [PATCH 1/1] general style: replaces memcmp() with starts_with()

[Quint Guvernator]

2014-03-17 21:59 GMT-04:00 Eric Sunshine <sunshine@sunshineco.com>:
> I can't speak for Junio, but the description could be made more
> concise and to-the-point. Aside from using imperative voice, you can
> eliminate redundancy, some of which comes from repeating in prose what
> the patch itself already states more concisely and precisely, and some
> from repeating what is implied by the fact that you're making such a
> change in the first place.

Wow, thanks for the detailed review. This mail will be something to
which I can refer when making future changes.

> In the subject, "general style" is a bit unusual. This isn't just a
> stylistic change; it's intended to improve code clarity.

It felt a little awkward, but I was trying to follow our guide [1] for
commit messages. It is all right to omit the leading identifier?

[1]: https://github.com/git/git/blob/fca26a/Documentation/SubmittingPatches#L87-L116

> A patch of this nature doesn't require much more description than
> stating what it does ("replace memcmp() with starts_with()") and why
> ("improve code clarity"). The following rewrite might be sufficient:
>
>     Subject: replace memcmp() with starts_with()
>
>     starts_with() indicates the intention of the check more clearly
>     than memcmp().

This is more concise; thank you. I will adapt this as the message for
the next version of this patch. Would it be wise to mention magic
numbers, as the discussion surrounding the rationale of this patch,
especially with Junio and Michael, has centered around that?

Thank you for the feedback,
Quint

Re: [PATCH 1/1] general style: replaces memcmp() with starts_with()

[Jeff King]

On Mon, Mar 17, 2014 at 04:07:00PM -0700, Junio C Hamano wrote:

> >> > -		if (!memcmp(used_atom[at], "color:", 6))
> >> > +		if (starts_with(used_atom[at], "color:"))
> >> >  			need_color_reset_at_eol = !!strcmp(used_atom[at], color_reset);
> [...]
> What if used_atom[at] is not related to color at all?  We do not
> want to touch the variable.

Thanks, that is what I was missing. It is not "did we find a reset" but
"toggle on for a non-reset color, toggle off for a reset".

It could be written with skip_prefix as:

  if (skip_prefix(used_atom[at], "color:", &c))
          need_color_reset_at_eol = !!strcmp(c, "reset");

but I do not think it is particularly important to do so. Sorry for the
noise.

-Peff

Re: [PATCH 1/1] general style: replaces memcmp() with starts_with()

[Eric Sunshine]

On Mon, Mar 17, 2014 at 10:33 PM, Quint Guvernator
<quintus.public@gmail.com> wrote:
> 2014-03-17 21:59 GMT-04:00 Eric Sunshine <sunshine@sunshineco.com>:
>> I can't speak for Junio, but the description could be made more
>> concise and to-the-point. Aside from using imperative voice, you can
>> eliminate redundancy, some of which comes from repeating in prose what
>> the patch itself already states more concisely and precisely, and some
>> from repeating what is implied by the fact that you're making such a
>> change in the first place.
>
> Wow, thanks for the detailed review. This mail will be something to
> which I can refer when making future changes.
>
>> In the subject, "general style" is a bit unusual. This isn't just a
>> stylistic change; it's intended to improve code clarity.
>
> It felt a little awkward, but I was trying to follow our guide [1] for
> commit messages. It is all right to omit the leading identifier?

Since this particular patch touches so many different files and
functions, it is difficult to craft a suitable leading identifier. The
alternative would be to split the patch into smaller pieces.
Ultimately, as the project maintainer, Junio must be the one to decide
if the monolithic patch lacking leading identifier is sufficient or if
smaller patches would be preferred.

> [1]: https://github.com/git/git/blob/fca26a/Documentation/SubmittingPatches#L87-L116
>
>> A patch of this nature doesn't require much more description than
>> stating what it does ("replace memcmp() with starts_with()") and why
>> ("improve code clarity"). The following rewrite might be sufficient:
>>
>>     Subject: replace memcmp() with starts_with()
>>
>>     starts_with() indicates the intention of the check more clearly
>>     than memcmp().
>
> This is more concise; thank you. I will adapt this as the message for
> the next version of this patch. Would it be wise to mention magic
> numbers, as the discussion surrounding the rationale of this patch,
> especially with Junio and Michael, has centered around that?

I was thinking of mentioning magic numbers in the example, but decided
that their removal was a natural and obvious consequence of the
improvement to code clarity, so it wasn't strictly necessary to talk
about it. On the other hand, it is a good secondary justification,
thus it should be perfectly acceptable to mention it. If I was writing
the commit message, I might start by saying "As an additional
benefit..." and then talk a bit about magic number retirement.

Re: [PATCH 1/1] general style: replaces memcmp() with starts_with()

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>>> A patch of this nature doesn't require much more description than
>>> stating what it does ("replace memcmp() with starts_with()") and why
>>> ("improve code clarity"). The following rewrite might be sufficient:
>>>
>>>     Subject: replace memcmp() with starts_with()
>>>
>>>     starts_with() indicates the intention of the check more clearly
>>>     than memcmp().
>>
>> This is more concise; thank you. I will adapt this as the message for
>> the next version of this patch. Would it be wise to mention magic
>> numbers, as the discussion surrounding the rationale of this patch,
>> especially with Junio and Michael, has centered around that?
>
> I was thinking of mentioning magic numbers in the example, but decided
> that their removal was a natural and obvious consequence of the
> improvement to code clarity, so it wasn't strictly necessary to talk
> about it. On the other hand, it is a good secondary justification,
> thus it should be perfectly acceptable to mention it. If I was writing
> the commit message, I might start by saying "As an additional
> benefit..." and then talk a bit about magic number retirement.

I think your subject is good (as you said, it makes it clear that it
is a project-wide clean-up by not mentioning any specific area), but
"indicates the intention of the check more clearly" would not tell
new readers who are unfamiliar with the helper API what "intention"
it is talking about very much, so perhaps

	Subject: use starts_with() instead of !memcmp()

	When checking if a string begins with a constant string,
	using starts_with() is less error prone than calling
	!memcmp() with an explicit byte count.

or something?