Re: [PATCH 1/1] general style: replaces memcmp() with starts_with()
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()
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()
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()
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()
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()
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()
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()
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()
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