read_packed_refs: avoid double-checking sane refs

Prior to d0f810f (refs.c: allow listing and deleting badly named refs, 2014-09-03), read_packed_refs would barf on any malformed refnames by virtue of calling create_ref_entry with the "check" parameter set to 1. That commit loosened our reading so that we call check_refname_format ourselves and just set a REF_BAD_NAME flag. We then call create_ref_entry with the check parameter set to 0. That function learned to do an extra safety check even when the check parameter is 0, so that we don't load any dangerous refnames (like "../../../etc/passwd"). This is implemented by calling refname_is_safe() in create_ref_entry(). However, we can observe that refname_is_safe() can only be true if check_refname_format() also failed. So in the common case of a sanely named ref, we perform _both_ checks, even though we know that the latter will never trigger. This has a noticeable performance impact when the packed-refs file is large. Let's drop the refname_is_safe check from create_ref_entry(), and make it the responsibility of the caller. Of the three callers that pass a check parameter of "0", two will have just called check_refname_format(), and can check the refname-safety only when it fails. The third case, pack_if_possible_fn, is copying from an existing ref entry, which must have previously passed our safety check. With this patch, running "git rev-parse refs/heads/does-not-exist" on a repo with a large (1.6GB) packed-refs file went from: real 0m6.768s user 0m6.340s sys 0m0.432s to: real 0m5.703s user 0m5.276s sys 0m0.432s for a wall-clock speedup of 15%. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Apr 16, 2015 at 05:03 UTC 03afcbee9b9246f5ec9b013f2a699b151f0ba7ab
1 file changed +4 -2
refs.c
+4 -2
@@ -344,8 +344,6 @@ static struct ref_entry *create_ref_entry(const char *refname,
344 if (check_name &&
345 check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))
346 die("Reference has invalid format: '%s'", refname);
347 - if (!check_name && !refname_is_safe(refname))
348 - die("Reference has invalid name: '%s'", refname);
347 len = strlen(refname) + 1;
348 ref = xmalloc(sizeof(struct ref_entry) + len);
349 hashcpy(ref->u.value.sha1, sha1);
@@ -1178,6 +1176,8 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)
1176 int flag = REF_ISPACKED;
1177
1178 if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
1179 + if (!refname_is_safe(refname))
1180 + die("packed refname is dangerous: %s", refname);
1181 hashclr(sha1);
1182 flag |= REF_BAD_NAME | REF_ISBROKEN;
1183 }
@@ -1323,6 +1323,8 @@ static void read_loose_refs(const char *dirname, struct ref_dir *dir)
1323 }
1324 if (check_refname_format(refname.buf,
1325 REFNAME_ALLOW_ONELEVEL)) {
1326 + if (!refname_is_safe(refname.buf))
1327 + die("loose refname is dangerous: %s", refname.buf);
1328 hashclr(sha1);
1329 flag |= REF_BAD_NAME | REF_ISBROKEN;
1330 }