On Sun, Jun 29, 2014 at 07:56:25AM +0200, René Scharfe wrote:
> Avoid overrunning the existing pack name (p->pack_name, a C string) in
> the case that the new path is longer by using strncmp instead of memcmp
> for comparing. While at it, replace the magic constant 4 with a
> strlen call to document its meaning.
An unwritten assumption with this code is that we have "foo.idx" and we
want to make sure that we are matching "foo.pack" from the existing pack
list. Both before and after your patch, I think we would match
"foobar.pack". It's probably not a big deal, as we don't expect random
junk in the pack directory, but I wonder if it would be better to be
explicit, like:
/*
* like ends_with, but return the truncated size of str via
* the "len" parameter.
*/
int strip_suffix(const char *str, const char *suffix, size_t *len)
{
size_t suflen = strlen(suffix);
*len = strlen(str);
if (len < suflen)
return 0;
else if (strcmp(str + len - suflen, suffix))
return 0;
else {
*len -= suflen;
return 1;
}
}
int idx_matches_pack(const char *idx, const char *pack)[
{
size_t idx_len, pack_len;
if (!strip_suffix(idx, ".idx", &idx_len) ||
!strip_suffix(pack, ".pack", &pack_len))
return 0;
if (idx_len != pack_len)
return 0;
return !memcmp(idx, pack, idx_len);
}
You'd perhaps want to split idx_matches_pack across the loop below (our
idx is actually a strbuf, so you can reuse path->len to avoid a strlen,
and you do not have to verify idx_len each time through the loop).
I think strip_suffix would have other uses, too. It's a more featureful
version of ends_with, as skip_prefix is to starts_with. E.g.,
builtin/remote.c:config_read_branches could use it to avoid some magic
numbers.
> diff --git a/sha1_file.c b/sha1_file.c
> index 394fa45..8adab14 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -1207,7 +1207,8 @@ static void prepare_packed_git_one(char *objdir, int
> local)
> if (has_extension(de->d_name, ".idx")) {
> /* Don't reopen a pack we already have. */
If we don't follow my suggestion above, we still have this
has_extension. This is a reimplementation of ends_with, isn't it? We can
probably drop it and just use ends_with.
-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
