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
Re: [PATCH] general style: replaces memcmp() with starts_with()
On 03/12/2014 03:06 PM, Quint Guvernator wrote: 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. 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. Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- 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;
- if
Re: [PATCH] general style: replaces memcmp() with starts_with()
On Wed, Mar 12, 2014 at 8:44 PM, Quint Guvernator quintus.pub...@gmail.com wrote: 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; It is not a plain search/replace. starts_with(..) == !memcmp(...). So you need to negate every replacement. -- Duy -- 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-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
Re: [PATCH] general style: replaces memcmp() with starts_with()
Am 12.03.2014 14:44, schrieb Quint Guvernator:
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
---
...
diff --git a/submodule.c b/submodule.c
index b80ecac..1edebc1 100644
--- a/submodule.c
+++ b/submodule.c
@@ -203,7 +203,7 @@ void gitmodules_config(void)
if (active_nr pos) { /* there is a .gitmodules */
const struct cache_entry *ce =
active_cache[pos];
if (ce_namelen(ce) == 11
- !memcmp(ce-name, .gitmodules, 11))
+ !starts_with(ce-name, .gitmodules))
gitmodules_is_unmerged = 1;
}
} else if (pos active_nr) {
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).
--
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-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 starts_with()
Am 12.03.2014 17:46, schrieb Quint Guvernator: 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(). Thanks, I missed that one (please use [PATCH v2] in the subject line of a second patch to make follow-ups easily distinguishable from the initial one ;-). Let me know if you still think the hunk should be dropped there. Yes, I think so. That spot uses memcmp() because ce-name may not be 0-terminated. If that assumption isn't correct, it should be replaced with a plain strcmp() instead (while also dropping the ce_namelen() comparison in the line above). But starts_with() points into the wrong direction 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 starts_with()
On Wed, Mar 12, 2014 at 06:22:24PM +0100, Jens Lehmann wrote: Let me know if you still think the hunk should be dropped there. Yes, I think so. That spot uses memcmp() because ce-name may not be 0-terminated. If that assumption isn't correct, it should be replaced with a plain strcmp() instead (while also dropping the ce_namelen() comparison in the line above). But starts_with() points into the wrong direction there. I think the length-check and memcmp is an optimization[1] here. But we should be able to encapsulate that pattern and avoid magic numbers entirely with something like mem_equals(). See my other response for more details. -Peff [1] Getting rid of the magic number entirely means we have to call strlen(.gitmodules), which seems like it is working against this optimization. But I think past experiments have shown that decent compilers will optimize strlen on a string literal to a constant, so as long as mem_equals is an inline, it should be equivalent. -- 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()
On Thu, Mar 13, 2014 at 12:22 AM, Jens Lehmann jens.lehm...@web.de wrote: That spot uses memcmp() because ce-name may not be 0-terminated. ce-name is 0-terminated (at least if it's created the normal way, I haven't checked where this ce in submodule.c comes from). ce_namelen() is just an optimization because we happen to store name's length if it's shorter than 4096, so there's no need to strlen(ce-name) again. -- Duy -- 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
