Re: [PATCH v2 2/2] sha1_file: use strncmp for string comparison

On Sun, Jun 29, 2014 at 07:56:25AM +0200, René Scharfe wrote:

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.

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