Re: [PATCH 5/7] refs.c: Refactor code for shortening full refnames into shorthand names

From: Junio C Hamano <gitster@pobox.com>
Date: May 7, 2013, 01:44 UTC
Message-ID: <7vwqrbo97g.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1367711749-8812-6-git-send-email-johan@herland.net>

Johan Herland <johan@herland.net> writes:

> This patch removes the only remaining user of ref_rev_parse_rules.
> It has now been fully replaced by ref_expand_rules. Hence this patch
> also removes ref_rev_parse_rules.

Yeah, I was wondering when you would do this while reading [4/7], as removing the parse_rules[] would break shortener side, and leaving it in for long would risk allowing parse_rules[] and expand_rules[] to drift apart.

Show 9 quoted lines

> -const struct ref_expand_rule ref_expand_rules[] = {
> -	{ ref_expand_txtly, "%.*s" },
> -	{ ref_expand_txtly, "refs/%.*s" },
> -	{ ref_expand_txtly, "refs/tags/%.*s" },
> -	{ ref_expand_txtly, "refs/heads/%.*s" },
> -	{ ref_expand_txtly, "refs/remotes/%.*s" },
> -	{ ref_expand_txtly, "refs/remotes/%.*s/HEAD" },
> -	{ NULL, NULL }
> -};

I wonder if you planned the previous step a bit better, this removal of a large block of text could have come next to the replacement of it we see after the addition of ref_shorten_txtly() function.

Show 25 quoted lines

> +static char *ref_shorten_txtly(const struct ref_expand_rule *rule,
> +			       const char *refname)
> +{
> +...
> +}
>  
> -static const char *ref_rev_parse_rules[] = {
> -	"%.*s",
> -	"refs/%.*s",
> -	"refs/tags/%.*s",
> -	"refs/heads/%.*s",
> -	"refs/remotes/%.*s",
> -	"refs/remotes/%.*s/HEAD",
> -	NULL
> +const struct ref_expand_rule ref_expand_rules[] = {
> +	{ ref_expand_txtly, NULL, "%.*s" },
> +	{ ref_expand_txtly, ref_shorten_txtly, "refs/%.*s" },
> +	{ ref_expand_txtly, ref_shorten_txtly, "refs/tags/%.*s" },
> +	{ ref_expand_txtly, ref_shorten_txtly, "refs/heads/%.*s" },
> +	{ ref_expand_txtly, ref_shorten_txtly, "refs/remotes/%.*s" },
> +	{ ref_expand_txtly, ref_shorten_txtly, "refs/remotes/%.*s/HEAD" },
> +	{ NULL, NULL, NULL }
>  };
>  
>  int refname_match(const char *abbrev_name, const char *full_name,

Show 19 quoted lines

>  char *shorten_unambiguous_ref(const char *refname, int strict)
>  {
>  	int i;
>  	char *short_name;
>  
> -	/* buffer for scanf result, at most refname must fit */
> -	short_name = xstrdup(refname);
> -
> -	/* skip first rule, it will always match */
> -	for (i = ARRAY_SIZE(ref_rev_parse_rules) - 1; i > 0 ; --i) {
> +	for (i = ARRAY_SIZE(ref_expand_rules) - 1; i >= 0 ; --i) {
> ...
>  		/*
>  		 * in strict mode, all (except the matched one) rules
>  		 * must fail to resolve to a valid non-ambiguous ref
>  		 */
>  		if (strict)
> -			rules_to_fail = ARRAY_SIZE(ref_rev_parse_rules);
> +			rules_to_fail = ARRAY_SIZE(ref_expand_rules);

This part obviously depends on 1/7; do we still have an off-by-one change from the original, or did I miscount when I reviewed 1/7?

Again, the overall strategy to refactor sounds sound.

It may be a lot simpler if you have ref_expand/shorten_append() and ref_expand/shortn_append_with_HEAD() built-in helper functions. Then you can perform the expansion and contraction without "%.*s" at all.