[PATCH v5] use starts_with() instead of !memcmp()
Another version, this time very in line with the review and commentary of Junio, Eric, and Michael. This version boasts a revamped commit message and fewer but surer hunks changed. Thanks again for the guidance. Quint Guvernator (1): use starts_with() instead of !memcmp() builtin/apply.c| 4 ++-- builtin/for-each-ref.c | 2 +- builtin/mktag.c| 2 +- builtin/patch-id.c | 10 +- connect.c | 4 ++-- imap-send.c| 6 +++--- remote.c | 2 +- 7 files changed, 15 insertions(+), 15 deletions(-) -- 1.9.0 -- 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
[PATCH v5] use starts_with() instead of !memcmp()
When checking if a string begins with a constant string, using
starts_with() indicates the intention of the check more clearly and is
less error-prone than calling !memcmp() with an explicit byte count.
Signed-off-by: Quint Guvernator quintus.pub...@gmail.com
---
builtin/apply.c| 4 ++--
builtin/for-each-ref.c | 2 +-
builtin/mktag.c| 2 +-
builtin/patch-id.c | 10 +-
connect.c | 4 ++--
imap-send.c| 6 +++---
remote.c | 2 +-
7 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/builtin/apply.c b/builtin/apply.c
index 0189523..826b3e2 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;
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..d2d9310 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 */
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
Re: [PATCH v3 1/1] general style: replaces memcmp() with starts_with()
2014-03-17 12:29 GMT-04:00 Michael Haggerty mhag...@alum.mit.edu: This hunk uses the magic number 11 a couple lines later. Junio (I think rightly) objected [1] to rewrites in these circumstances because they make it even less obvious where the constant came from and thereby make the complete elimination of the hard-coded constant *more* difficult. [1] http://article.gmane.org/gmane.comp.version-control.git/244005 Sure, I can understand that. I'll look through the hunks again with more context in the diff to try to look for more cases where magic numbers are used more than once. One of the goals of this revision is to minimize that, see the commit message. In any open-source project it is important to optimize for the time of the reviewer, because code-review is a relatively tedious task and is therefore often the bottleneck for progress. Therefore I suggest that you turn your approach on its head. Don't remove the most objectionable hunks but rather *include only the best hunks*--the ones that you can stand behind 100%, which you think are an unambiguous improvement, and that the reviewer can accept without reservations. ... 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. As you gain experience and learn about the preferences of the Git project, you will get a better feel for the boundary between acceptable/unacceptable patches, and *then* you will be able to start submitting patches closer to the boundary. I see. For v4, I will be more discerning as to what I include. I will try to keep the scope of future patches down and err on the side of caution when I review my own changes before submitting. Thank you for the *pointers. I'm going to give v4 a shot. It should be on the mailing list in an hour or so. 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] GSoC Change multiple if-else statements to be table-driven
2014-03-17 15:27 GMT-04:00 Eric Sunshine sunsh...@sunshineco.com: A quick (perhaps inaccurate) search of the mailing list shows that, of the remaining untaken items, #10, 11, 12, 15, 16, and 18 have had just one submission, and #13 had two, so we're okay. I am still working on #14: Change fetch-pack.c:filter_refs() to use starts_with() instead of memcmp(). Try to find other sites that could be rewritten similarly. Another version of the patch should be on this list within the hour. 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
[PATCH v4 0/1] general style: replaces memcmp() with starts_with()
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 -- 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
[PATCH 1/1] 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.
Signed-off-by: Quint Guvernator quintus.pub...@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
Re: [PATCH v4 0/1] general style: replaces memcmp() with starts_with()
My mistake, folks. This is [PATCH v4]. Apologies for the confusion. 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()
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()
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
[GSoC 2014] Refactoring git rebase --interactive
Hi again, Git community! My name is Quint Guvernator, and I am participating in the Google Summer of Code program. I am in university at the College of William and Mary in Williamsburg, VA and plan to major in Computer Science and Linguistics. I have been working on a microproject [1][2] to get the hang of submitting patches and working with the mailing list. I have just submitted my proposal for git rebase --interactive through the google-melange website. In brief, I plan to refactor the shell script, cleaning up parts where the code is incohesive or difficult to decipher; add functionality to the script; and improve documentation by describing the script in comments and improving our user docs. I think it is important not to rush into new features, however, and detail in my proposal the extent to which I will stay in contact with the community through this list and on IRC. Should the work on rebase --interactive not take all summer, I would work to improve the quality of Git documentation. I am a native English speaker and copy-edit a local newspaper, so I feel I am qualified to edit and extend the Git documentation. I am happy to receive private mail, so please send any issues or questions you may have either to the list or directly to my inbox. Thanks for your consideration for GSoC. --Quint [1]: http://thread.gmane.org/gmane.comp.version-control.git/243940 [2]: http://thread.gmane.org/gmane.comp.version-control.git/244159 -- 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
[PATCH v3 0/1] general style: replaces memcmp() with starts_with()
Hi, folks. I've looked through the list's responses and removed the most objectionable hunks from the patch v2, especially in cases where starts_with either hurts readability or further obscures the use of magic numbers. Let me know what you all think about the changes. Thank you all again for your help. This is my first patch here, and has been quite a microproject indeed! Quint Guvernator (1): general style: replaces memcmp() with starts_with() builtin/apply.c | 6 +++--- builtin/for-each-ref.c| 2 +- builtin/get-tar-commit-id.c | 2 +- builtin/mktag.c | 8 builtin/patch-id.c| 18 +- commit.c | 4 ++-- connect.c | 6 +++--- contrib/convert-objects/convert-objects.c | 6 +++--- convert.c | 2 +- http-walker.c | 2 +- imap-send.c | 6 +++--- pack-write.c | 2 +- remote.c | 2 +- 13 files changed, 33 insertions(+), 33 deletions(-) -- 1.9.0 -- 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
[PATCH v3 1/1] 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.
Signed-off-by: Quint Guvernator quintus.pub...@gmail.com
---
builtin/apply.c | 6 +++---
builtin/for-each-ref.c| 2 +-
builtin/get-tar-commit-id.c | 2 +-
builtin/mktag.c | 8
builtin/patch-id.c| 18 +-
commit.c | 4 ++--
connect.c | 6 +++---
contrib/convert-objects/convert-objects.c | 6 +++---
convert.c | 2 +-
http-walker.c | 2 +-
imap-send.c | 6 +++---
pack-write.c | 2 +-
remote.c | 2 +-
13 files changed, 33 insertions(+), 33 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/get-tar-commit-id.c b/builtin/get-tar-commit-id.c
index aa72596..6409c26 100644
--- a/builtin/get-tar-commit-id.c
+++ b/builtin/get-tar-commit-id.c
@@ -29,7 +29,7 @@ int cmd_get_tar_commit_id(int argc, const char **argv, const
char *prefix)
die(git get-tar-commit-id: read error);
if (header-typeflag[0] != 'g')
return 1;
- if (memcmp(content, 52 comment=, 11))
+ if (!starts_with(content, 52 comment=))
return 1;
n = write_in_full(1, content + 11, 41);
diff --git a/builtin/mktag.c b/builtin/mktag.c
index 640ab64..70385ac 100644
--- a/builtin/mktag.c
+++ b/builtin/mktag.c
@@ -49,7 +49,7 @@ static int verify_tag(char *buffer, unsigned long size)
/* Verify object line */
object = buffer;
- if (memcmp(object, object , 7))
+ if (!starts_with(object, object ))
return error(char%d: does not start with \object \, 0);
if (get_sha1_hex(object + 7, sha1))
@@ -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));
@@ -98,7 +98,7 @@ static int verify_tag(char *buffer, unsigned long size)
/* Verify the tagger line */
tagger_line = tag_line;
- if (memcmp(tagger_line, tagger , 7))
+ if (!starts_with(tagger_line, tagger ))
return error(char%PRIuMAX: could not find
Re: [PATCH] general style: replaces memcmp() with proper starts_with()
2014-03-14 0:57 GMT-04:00 Jeff King p...@peff.net: This discussion ended up encompassing a lot of other related cleanups. I hope we didn't scare you away. :) I don't think you could; this community is much more accepting than other software communities around the web. The fact that I received constructive feedback rather than a lecture when formatting issues slipped my mind (i.e. forgetting [PATCH v2]) is reason enough to stick around! My understanding is that you were approaching this as a micro-project for GSoC. I'd love it if you want to pick up and run with some of the ideas discussed here. But as far as a microproject goes, I think it would make sense to identify one or two no-brainer improvement spots by hand, and submit a patch with just those (and I think Junio gave some good guidelines in his reply). I agree with trying to push a few uncontroversial changes through. I'd love to take a deeper look at these helper functions and related cleanups…perhaps it would be worth it to identify a few key areas to work on in addition to a main GSoC project? In fact, the project I'm looking to take on (rebase --interactive) also involves code cleanup and might not take all summer, so I could see how those could work well together in a proposal. I'll be re-reading this thread and working on this patch over the weekend to try to identify the more straightforward hunks I could submit in a patch. Thanks Peff and everyone else for your help. 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] general style: replaces memcmp() with starts_with()
2014-03-13 12:05 GMT-04:00 Michael Haggerty mhag...@alum.mit.edu: It is very, very unlikely that you inverted the sense of dozens of tests throughout the Git code base and the tests ran correctly. I rather think that you made a mistake when testing. You should double- and triple-check that you really ran the tests and ran them correctly. It looks like HEAD was in the wrong place when I ran the tests. They do not in fact pass. Apologies, 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
[PATCH] general style: replaces memcmp() with starts_with()
memcmp() is replaced with starts_with() when comparing strings from the
beginning. 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
---
builtin/apply.c | 10 +-
builtin/cat-file.c| 2 +-
builtin/commit-tree.c | 2 +-
builtin/for-each-ref.c| 2 +-
builtin/get-tar-commit-id.c | 2 +-
builtin/mailinfo.c| 10 +-
builtin/mktag.c | 8
builtin/patch-id.c| 18 +-
commit.c | 18 +-
connect.c | 8
contrib/convert-objects/convert-objects.c | 6 +++---
convert.c | 2 +-
credential-cache.c| 2 +-
fetch-pack.c | 2 +-
fsck.c| 8
http-walker.c | 4 ++--
imap-send.c | 2 +-
pack-write.c | 2 +-
path.c| 2 +-
refs.c| 2 +-
remote.c | 2 +-
submodule.c | 2 +-
transport.c | 2 +-
xdiff-interface.c | 2 +-
24 files changed, 60 insertions(+), 60 deletions(-)
diff --git a/builtin/apply.c b/builtin/apply.c
index a7e72d5..8f21957 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -846,8 +846,8 @@ static int has_epoch_timestamp(const char *nameline)
* -MM-DD hh:mm:ss must be from either 1969-12-31
* (west of GMT) or 1970-01-01 (east of GMT)
*/
- if ((zoneoffset 0 memcmp(timestamp, 1969-12-31, 10)) ||
- (0 = zoneoffset memcmp(timestamp, 1970-01-01, 10)))
+ if ((zoneoffset 0 starts_with(timestamp, 1969-12-31)) ||
+ (0 = zoneoffset starts_with(timestamp, 1970-01-01)))
return 0;
hourminute = (strtol(timestamp + 11, NULL, 10) * 60 +
@@ -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/cat-file.c b/builtin/cat-file.c
index d5a93e0..be83345 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -82,7 +82,7 @@ static int cat_one_file(int opt, const char *exp_type, const
char *obj_name)
enum object_type type;
unsigned long size;
char *buffer = read_sha1_file(sha1, type,
size);
- if (memcmp(buffer, object , 7) ||
+ if (starts_with(buffer, object ) ||
get_sha1_hex(buffer + 7, blob_sha1))
die(%s not a valid tag,
sha1_to_hex(sha1));
free(buffer);
diff --git a/builtin/commit-tree.c b/builtin/commit-tree.c
index 987a4c3..2d995a2 100644
--- a/builtin/commit-tree.c
+++ b/builtin/commit-tree.c
@@ -66,7 +66,7 @@ int cmd_commit_tree(int argc, const char **argv, const char
*prefix)
continue;
}
- if (!memcmp(arg, -S, 2)) {
+ if (!starts_with(arg, -S)) {
sign_commit = arg + 2;
continue;
}
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 51798b4..be14d71 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
Re: [PATCH] general style: replaces memcmp() with starts_with()
2014-03-12 9:51 GMT-04:00 Duy Nguyen pclo...@gmail.com: starts_with(..) == !memcmp(...). So you need to negate every replacement. My apologies--it doesn't look like the tests caught it either. I will fix this and submit a new patch. -- 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
[PATCH] general style: replaces memcmp() with proper starts_with()
memcmp() is replaced with negated starts_with() when comparing strings
from the beginning. starts_with() looks nicer and it saves the extra
argument of the length of the comparing string.
note: this commit properly handles negation in starts_with()
Signed-off-by: Quint Guvernator quintus.pub...@gmail.com
---
builtin/apply.c | 10 +-
builtin/cat-file.c| 2 +-
builtin/commit-tree.c | 2 +-
builtin/for-each-ref.c| 2 +-
builtin/get-tar-commit-id.c | 2 +-
builtin/mailinfo.c| 10 +-
builtin/mktag.c | 8
builtin/patch-id.c| 18 +-
commit.c | 18 +-
connect.c | 8
contrib/convert-objects/convert-objects.c | 6 +++---
convert.c | 2 +-
credential-cache.c| 2 +-
fetch-pack.c | 2 +-
fsck.c| 8
http-walker.c | 4 ++--
imap-send.c | 6 +++---
pack-write.c | 2 +-
path.c| 2 +-
refs.c| 2 +-
remote.c | 2 +-
submodule.c | 2 +-
transport.c | 2 +-
23 files changed, 61 insertions(+), 61 deletions(-)
diff --git a/builtin/apply.c b/builtin/apply.c
index a7e72d5..16c20af 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -846,8 +846,8 @@ static int has_epoch_timestamp(const char *nameline)
* -MM-DD hh:mm:ss must be from either 1969-12-31
* (west of GMT) or 1970-01-01 (east of GMT)
*/
- if ((zoneoffset 0 memcmp(timestamp, 1969-12-31, 10)) ||
- (0 = zoneoffset memcmp(timestamp, 1970-01-01, 10)))
+ if ((zoneoffset 0 !starts_with(timestamp, 1969-12-31)) ||
+ (0 = zoneoffset !starts_with(timestamp, 1970-01-01)))
return 0;
hourminute = (strtol(timestamp + 11, NULL, 10) * 60 +
@@ -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/cat-file.c b/builtin/cat-file.c
index d5a93e0..6266bbb 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -82,7 +82,7 @@ static int cat_one_file(int opt, const char *exp_type, const
char *obj_name)
enum object_type type;
unsigned long size;
char *buffer = read_sha1_file(sha1, type,
size);
- if (memcmp(buffer, object , 7) ||
+ if (!starts_with(buffer, object ) ||
get_sha1_hex(buffer + 7, blob_sha1))
die(%s not a valid tag,
sha1_to_hex(sha1));
free(buffer);
diff --git a/builtin/commit-tree.c b/builtin/commit-tree.c
index 987a4c3..2777519 100644
--- a/builtin/commit-tree.c
+++ b/builtin/commit-tree.c
@@ -66,7 +66,7 @@ int cmd_commit_tree(int argc, const char **argv, const char
*prefix)
continue;
}
- if (!memcmp(arg, -S, 2)) {
+ if (starts_with(arg, -S)) {
sign_commit = arg + 2;
continue;
}
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 51798b4..fe198fd 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
Re: [PATCH] general style: replaces memcmp() with starts_with()
2014-03-12 11:47 GMT-04:00 Jens Lehmann jens.lehm...@web.de: I think this hunk should be dropped as the memcmp() here doesn't mean starts with but is identical (due to the ce_namelen(ce) == 11 in the line above). There is an issue with negation in this patch. I've submitted a new one [1] to the mailing list. The subject line of the new patch is [PATCH] general style: replaces memcmp() with proper starts_with(). Let me know if you still think the hunk should be dropped there. [1]: http://thread.gmane.org/gmane.comp.version-control.git/243940 -- 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] general style: replaces memcmp() with proper starts_with()
From what I can gather, there seems to be opposition to specific
pieces of this patch.
The following area is clearly the most controversial:
static inline int standard_header_field(const char *field, size_t len)
{
-return ((len == 4 !memcmp(field, tree , 5)) ||
-(len == 6 !memcmp(field, parent , 7)) ||
-(len == 6 !memcmp(field, author , 7)) ||
-(len == 9 !memcmp(field, committer , 10)) ||
-(len == 8 !memcmp(field, encoding , 9)));
+return ((len == 4 starts_with(field, tree )) ||
+(len == 6 starts_with(field, parent )) ||
+(len == 6 starts_with(field, author )) ||
+(len == 9 starts_with(field, committer )) ||
+(len == 8 starts_with(field, encoding )));
I am happy to submit a version of this patch excluding this section
(and/or others), but it seems I've stumbled into a more fundamental
conversation about the place for helper functions in general (and
about refactoring skip_prefix()). I am working on this particular
change as a microproject, #14 on the list [1], and am not as familiar
with the conventions of the Git codebase as many of you on this list
are.
Junio said:
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?
It is _not_ my goal to make the code harder to maintain down the road.
So, at this point, which hunks (if any) are worth patching?
Quint
[1]: http://git.github.io/SoC-2014-Microprojects.html
--
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
