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

[Junio C Hamano]

Eric Sunshine sunsh...@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?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Quint Guvernator quintus.pub...@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.pub...@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 ))

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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Jeff King p...@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.

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Quint Guvernator]

2014-03-17 18:52 GMT-04:00 Junio C Hamano gits...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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.pub...@gmail.com wrote:
 2014-03-17 18:52 GMT-04:00 Junio C Hamano gits...@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().
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Quint Guvernator]

2014-03-17 21:59 GMT-04:00 Eric Sunshine sunsh...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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.pub...@gmail.com wrote:
 2014-03-17 21:59 GMT-04:00 Eric Sunshine sunsh...@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.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html