Re: [PATCH v2 03/12] refs: stop using mkpath() in refname_match()

["SZEDER Gábor" <szeder.dev@gmail.com>]

On Tue, Oct 13, 2026 at 11:13:38PM -0400, Jon Simons wrote:
> refname_match() formats every ref_rev_parse_rules entry through mkpath()
> to then strcmp() with its given full_name.  It turns out this formatting
> function dominates the client-side cost of matching explicit refspecs
> against a remote that advertises lots of refs.
> 
> Reuse match_parse_rule() instead to efficiently compare each rule
> against its given full_name.  With this change we avoid mkpath()'s
> strbuf_vaddf(), and each match is now a direct prefix + suffix
> comparison, length check, and memcmp of the refname at hand.
> 
> Timings from the test added in the previous commit:
> 
>   Test                           HEAD~1            HEAD
>   -----------------------------------------------------------------------
>   5516.3: empty:refspecs:1       0.15(0.09+0.11)   0.14(0.07+0.11) -6.7%
>   5516.5: empty:refspecs:10      0.38(0.32+0.11)   0.17(0.11+0.11) -55.3%
>   5516.7: empty:refspecs:100     2.50(2.43+0.11)   0.49(0.42+0.11) -80.4%
>   5516.9: mirror:refspecs:1      0.21(0.15+0.11)   0.16(0.09+0.11) -23.8%
>   5516.11: mirror:refspecs:10    0.80(0.73+0.11)   0.26(0.19+0.11) -67.5%
>   5516.13: mirror:refspecs:100   6.68(6.59+0.13)   1.18(1.11+0.11) -82.3%
> 
> Notes on a change in behavior:
> 
>  - mkpath() runs cleanup_path(), which strips leading "./".  So,
>    "./refs/heads/foo" previously matched "refs/heads/foo" through the
>    "%.*s" rule.  As of this commit, this is no longer true.

I was wondering whether we should worry about breaking something out
there that relies on "./refs/heads/foo" working as a fully qualified
ref, according to Hyrum's law...

>  - It's considered a bug that "./"-prefixed strings previously could
>    have been matched with refnames in this way:
> 
>    - cleanup_path() dates back to 26c8a533af (Add "mkpath()" helper
>      function, 2005-07-08), and is intended to be used for filepaths,
>      not refnames.  The leading-"./" strip itself comes from f17a1b1bec
>      (Fix up path-cleanup in git_path() properly, 2005-07-05).
> 
>    - refname_match() has used mkpath() in its matching loop since its
>      inception with 79803322c1 (add refname_match(), 2007-11-11).
> 
>    - 6cd4a8982d (avoid using mksnpath for refs, 2017-03-28) previously
>      removed other cleanup_path() spots reachable from mksnpath() and
>      notes they were "questionable when dealing with refnames, as we
>      could silently canonicalize a syntactically bogus refname into a
>      valid one."

But apprently we didn't worry about it back then with 6cd4a8982d, so I
don't think we should worry about it now, either.

> Tests are introduced for the observable behavior change:
> 
>  - t5516 now asserts that pushing a "./"-prefixed source fails
> 
>  - t5510 now asserts that fetch does not match a "./"-prefixed
>    branch.<name>.merge value, either via the default fetch refspec
>    or against the remote's advertised refs.
> 
> Signed-off-by: Jon Simons <jon@jonsimons.org>
> ---
>  refs.c                | 88 +++++++++++++++++++++++--------------------
>  t/t5510-fetch.sh      | 44 ++++++++++++++++++++++
>  t/t5516-fetch-push.sh |  6 +++
>  3 files changed, 97 insertions(+), 41 deletions(-)
> 
> diff --git a/refs.c b/refs.c
> index 951db56113..9bf3bc8153 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -652,6 +652,44 @@ static const char *ref_rev_parse_rules[] = {
>  
>  #define NUM_REV_PARSE_RULES (ARRAY_SIZE(ref_rev_parse_rules) - 1)
>  
> +/*
> + * Check that the string refname matches a rule of the form
> + * "{prefix}%.*s{suffix}". So "foo/bar/baz" would match the rule
> + * "foo/%.*s/baz", and return the string "bar".
> + */
> +static const char *match_parse_rule(const char *refname, const char *rule,
> +				    size_t *len)
> +{
> +	/*
> +	 * Check that rule matches refname up to the first percent in the rule.
> +	 * We can bail immediately if not, but otherwise we leave "rule" at the
> +	 * %-placeholder, and "refname" at the start of the potential matched
> +	 * name.
> +	 */
> +	while (*rule != '%') {
> +		if (!*rule)
> +			BUG("rev-parse rule did not have percent");
> +		if (*refname++ != *rule++)
> +			return NULL;
> +	}
> +
> +	/*
> +	 * Check that our "%" is the expected placeholder. This assumes there
> +	 * are no other percents (placeholder or quoted) in the string, but
> +	 * that is sufficient for our rev-parse rules.
> +	 */
> +	if (!skip_prefix(rule, "%.*s", &rule))
> +		return NULL;
> +
> +	/*
> +	 * And now check that our suffix (if any) matches.
> +	 */
> +	if (!strip_suffix(refname, rule, len))
> +		return NULL;
> +
> +	return refname; /* len set by strip_suffix() */
> +}
> +

"--color-moved" shows that this function was moved here without any
modification.  Good.

>  /*
>   * Is it possible that the caller meant full_name with abbrev_name?
>   * If so return a non-zero value to signal "yes"; the magnitude of
> @@ -662,12 +700,18 @@ static const char *ref_rev_parse_rules[] = {
>  int refname_match(const char *abbrev_name, const char *full_name)
>  {
>  	const char **p;
> -	const int abbrev_name_len = strlen(abbrev_name);
> +	const size_t abbrev_name_len = strlen(abbrev_name);
>  	const int num_rules = NUM_REV_PARSE_RULES;
>  
> -	for (p = ref_rev_parse_rules; *p; p++)
> -		if (!strcmp(full_name, mkpath(*p, abbrev_name_len, abbrev_name)))
> +	for (p = ref_rev_parse_rules; *p; p++) {
> +		size_t short_name_len;
> +		const char *short_name = match_parse_rule(full_name, *p,
> +							  &short_name_len);
> +
> +		if (short_name && short_name_len == abbrev_name_len &&
> +		    !memcmp(short_name, abbrev_name, abbrev_name_len))
>  			return &ref_rev_parse_rules[num_rules] - p;
> +	}

Looks good.

>  	return 0;
>  }
> @@ -1615,44 +1659,6 @@ int refs_update_ref(struct ref_store *refs, const char *msg,
>  	return 0;
>  }
>  
> -/*
> - * Check that the string refname matches a rule of the form
> - * "{prefix}%.*s{suffix}". So "foo/bar/baz" would match the rule
> - * "foo/%.*s/baz", and return the string "bar".
> - */
> -static const char *match_parse_rule(const char *refname, const char *rule,
> -				    size_t *len)
> -{
> -	/*
> -	 * Check that rule matches refname up to the first percent in the rule.
> -	 * We can bail immediately if not, but otherwise we leave "rule" at the
> -	 * %-placeholder, and "refname" at the start of the potential matched
> -	 * name.
> -	 */
> -	while (*rule != '%') {
> -		if (!*rule)
> -			BUG("rev-parse rule did not have percent");
> -		if (*refname++ != *rule++)
> -			return NULL;
> -	}
> -
> -	/*
> -	 * Check that our "%" is the expected placeholder. This assumes there
> -	 * are no other percents (placeholder or quoted) in the string, but
> -	 * that is sufficient for our rev-parse rules.
> -	 */
> -	if (!skip_prefix(rule, "%.*s", &rule))
> -		return NULL;
> -
> -	/*
> -	 * And now check that our suffix (if any) matches.
> -	 */
> -	if (!strip_suffix(refname, rule, len))
> -		return NULL;
> -
> -	return refname; /* len set by strip_suffix() */
> -}
> -
>  char *refs_shorten_unambiguous_ref(struct ref_store *refs,
>  				   const char *refname, int strict)
>  {
> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> index 300bd5396d..941213f36a 100755
> --- a/t/t5510-fetch.sh
> +++ b/t/t5510-fetch.sh
> @@ -1074,6 +1074,50 @@ test_expect_success 'LHS of refspec follows ref disambiguation rules' '
>  	)
>  '
>  
> +test_expect_success 'fetch with "./"-prefixed branch.<name>.merge does not mark any ref for merge' '
> +	mkdir dotslash-merge-default-refspec &&
> +	(
> +		cd dotslash-merge-default-refspec &&
> +		git init -b main server &&
> +		test_commit -C server one &&
> +		git -C server branch other &&
> +		test_commit -C server two &&
> +
> +		# The bogus ./refs/heads/main should not match the remote refs/heads/main.
> +		git clone server client &&
> +		git -C client config branch.main.merge ./refs/heads/main &&
> +		git -C client fetch &&
> +		{
> +			echo "$(git -C server rev-parse main)	not-for-merge" &&
> +			echo "$(git -C server rev-parse other)	not-for-merge"
> +		} >expect &&
> +		cut -f -2 client/.git/FETCH_HEAD >actual &&
> +		test_cmp expect actual
> +	)
> +'
> +
> +test_expect_success 'fetch protocol v0 with "./"-prefixed branch.<name>.merge does not match any remote ref' '
> +	mkdir dotslash-merge-fetch-protocol-v0 &&
> +	(
> +		cd dotslash-merge-fetch-protocol-v0 &&
> +		git init -b main server &&
> +		test_commit -C server one &&
> +		git -C server branch other &&
> +		test_commit -C server two &&
> +
> +		# Omit refs/heads/main from the fetch refspec so that the merge
> +		# source is instead looked up among the refs the remote advertised.
> +		git clone server client-v0 &&
> +		git -C client-v0 config remote.origin.fetch \
> +			+refs/heads/other:refs/remotes/origin/other &&
> +		git -C client-v0 config branch.main.merge ./refs/heads/main &&
> +		git -C client-v0 -c protocol.version=0 fetch &&
> +		echo "$(git -C server rev-parse other)	not-for-merge" >expect &&
> +		cut -f -2 client-v0/.git/FETCH_HEAD >actual &&
> +		test_cmp expect actual
> +	)
> +'
> +

Testing the "./"-prefixed "branch.<name>.merge" config is a nice
touch.

>  test_expect_success 'fetch.writeCommitGraph' '
>  	git clone three write &&
>  	(
> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index b982b209bf..81ad6cd52f 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -433,6 +433,12 @@ test_expect_success 'push with onelevel ref' '
>  	test_must_fail git push testrepo HEAD:refs/onelevel
>  '
>  
> +test_expect_success 'push with "./"-prefixed src does not match any ref' '
> +	mk_test testrepo heads/main &&
> +	test_must_fail git push testrepo ./refs/heads/main:refs/heads/frotz 2>err &&
> +	test_grep "src refspec ./refs/heads/main does not match any" err
> +'
> +
>  test_expect_success 'push with colon-less refspec (1)' '
>  	mk_test testrepo heads/frotz tags/frotz &&
>  	git branch -f frotz main &&
> -- 
> 2.55.0
>