Re: [PATCH 4/5] make get_short_ref a public function

> Often we want to shorten a full ref name to something "prettier"
> to show a user. For example, "refs/heads/master" is often shown
> simply as "master", or "refs/remotes/origin/master" is shown as
> "origin/master".
> 
> Many places in the code use a very simple formula: skip common
> prefixes like refs/heads, refs/remotes, etc. This is codified in
> the prettify_ref function.
> 
> for-each-ref has a more correct (but more expensive) approach:
> consider the ref lookup rules, and try shortening as much as
> possible while remaining unambiguous.
> 
> This patch makes the latter strategy globally available as
> shorten_unambiguous_ref.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> Actually, I am not quite sure that this function is "more correct". It
> looks at the rev-parsing rules as a hierarchy, so if you have
> "refs/remotes/foo" and "refs/heads/foo", then it will abbreviate the
> first to "remotes/foo" (as expected) and the latter to just "foo".
> 
> This is technically correct, as "refs/heads/foo" will be selected by
> "foo", but it will warn about ambiguity. Should we actually try to avoid
> reporting refs which would be ambiguous?
> 
> Should this simply replace prettify_ref (and other places which should
> be using prettify_ref but aren't)? It is definitely more expensive, as
> it has to resolve refs to look for ambiguities, but I don't know if we
> care in most code paths.

> 
>  builtin-for-each-ref.c |  105 +-----------------------------------------------
>  refs.c                 |   99 +++++++++++++++++++++++++++++++++++++++++++++
>  refs.h                 |    1 +
>  3 files changed, 101 insertions(+), 104 deletions(-)
> 
> diff --git a/builtin-for-each-ref.c b/builtin-for-each-ref.c
> index 277d1fb..8c82484 100644
> --- a/builtin-for-each-ref.c
> +++ b/builtin-for-each-ref.c
> @@ -546,109 +546,6 @@ static void grab_values(struct atom_value *val, int deref, struct object *obj, v
>  }
>  
>  /*
> - * generate a format suitable for scanf from a ref_rev_parse_rules
> - * rule, that is replace the "%.*s" spec with a "%s" spec
> - */
> -static void gen_scanf_fmt(char *scanf_fmt, const char *rule)
> -{
> -	char *spec;
> -
> -	spec = strstr(rule, "%.*s");
> -	if (!spec || strstr(spec + 4, "%.*s"))
> -		die("invalid rule in ref_rev_parse_rules: %s", rule);
> -
> -	/* copy all until spec */
> -	strncpy(scanf_fmt, rule, spec - rule);
> -	scanf_fmt[spec - rule] = '\0';
> -	/* copy new spec */
> -	strcat(scanf_fmt, "%s");
> -	/* copy remaining rule */
> -	strcat(scanf_fmt, spec + 4);
> -
> -	return;
> -}
> -
> -/*
> - * Shorten the refname to an non-ambiguous form
> - */
> -static char *get_short_ref(const char *ref)
> -{
> -	int i;
> -	static char **scanf_fmts;
> -	static int nr_rules;
> -	char *short_name;
> -
> -	/* pre generate scanf formats from ref_rev_parse_rules[] */
> -	if (!nr_rules) {
> -		size_t total_len = 0;
> -
> -		/* the rule list is NULL terminated, count them first */
> -		for (; ref_rev_parse_rules[nr_rules]; nr_rules++)
> -			/* no +1 because strlen("%s") < strlen("%.*s") */
> -			total_len += strlen(ref_rev_parse_rules[nr_rules]);
> -
> -		scanf_fmts = xmalloc(nr_rules * sizeof(char *) + total_len);
> -
> -		total_len = 0;
> -		for (i = 0; i < nr_rules; i++) {
> -			scanf_fmts[i] = (char *)&scanf_fmts[nr_rules]
> -					+ total_len;
> -			gen_scanf_fmt(scanf_fmts[i], ref_rev_parse_rules[i]);
> -			total_len += strlen(ref_rev_parse_rules[i]);
> -		}
> -	}
> -
> -	/* bail out if there are no rules */
> -	if (!nr_rules)
> -		return xstrdup(ref);
> -
> -	/* buffer for scanf result, at most ref must fit */
> -	short_name = xstrdup(ref);
> -
> -	/* skip first rule, it will always match */
> -	for (i = nr_rules - 1; i > 0 ; --i) {
> -		int j;
> -		int short_name_len;
> -
> -		if (1 != sscanf(ref, scanf_fmts[i], short_name))
> -			continue;
> -
> -		short_name_len = strlen(short_name);
> -
> -		/*
> -		 * check if the short name resolves to a valid ref,
> -		 * but use only rules prior to the matched one
> -		 */
> -		for (j = 0; j < i; j++) {
> -			const char *rule = ref_rev_parse_rules[j];
> -			unsigned char short_objectname[20];
> -			char refname[PATH_MAX];
> -
> -			/*
> -			 * the short name is ambiguous, if it resolves
> -			 * (with this previous rule) to a valid ref
> -			 * read_ref() returns 0 on success
> -			 */
> -			mksnpath(refname, sizeof(refname),
> -				 rule, short_name_len, short_name);
> -			if (!read_ref(refname, short_objectname))
> -				break;
> -		}
> -
> -		/*
> -		 * short name is non-ambiguous if all previous rules
> -		 * haven't resolved to a valid ref
> -		 */
> -		if (j == i)
> -			return short_name;
> -	}
> -
> -	free(short_name);
> -	return xstrdup(ref);
> -}
> -
> -
> -/*
>   * Parse the object referred by ref, and grab needed value.
>   */
>  static void populate_value(struct refinfo *ref)
> @@ -704,7 +601,7 @@ static void populate_value(struct refinfo *ref)
>  		if (formatp) {
>  			formatp++;
>  			if (!strcmp(formatp, "short"))
> -				refname = get_short_ref(refname);
> +				refname = shorten_unambiguous_ref(refname);
>  			else
>  				die("unknown %.*s format %s",
>  					formatp - name, name, formatp);
> diff --git a/refs.c b/refs.c
> index 59c373f..1e5e7b4 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1652,3 +1652,102 @@ struct ref *find_ref_by_name(const struct ref *list, const char *name)
>  			return (struct ref *)list;
>  	return NULL;
>  }
> +
> +/*
> + * generate a format suitable for scanf from a ref_rev_parse_rules
> + * rule, that is replace the "%.*s" spec with a "%s" spec
> + */
> +static void gen_scanf_fmt(char *scanf_fmt, const char *rule)
> +{
> +	char *spec;
> +
> +	spec = strstr(rule, "%.*s");
> +	if (!spec || strstr(spec + 4, "%.*s"))
> +		die("invalid rule in ref_rev_parse_rules: %s", rule);
> +
> +	/* copy all until spec */
> +	strncpy(scanf_fmt, rule, spec - rule);
> +	scanf_fmt[spec - rule] = '\0';
> +	/* copy new spec */
> +	strcat(scanf_fmt, "%s");
> +	/* copy remaining rule */
> +	strcat(scanf_fmt, spec + 4);
> +
> +	return;
> +}
> +
> +char *shorten_unambiguous_ref(const char *ref)
> +{
> +	int i;
> +	static char **scanf_fmts;
> +	static int nr_rules;
> +	char *short_name;
> +
> +	/* pre generate scanf formats from ref_rev_parse_rules[] */
> +	if (!nr_rules) {
> +		size_t total_len = 0;
> +
> +		/* the rule list is NULL terminated, count them first */
> +		for (; ref_rev_parse_rules[nr_rules]; nr_rules++)
> +			/* no +1 because strlen("%s") < strlen("%.*s") */
> +			total_len += strlen(ref_rev_parse_rules[nr_rules]);
> +
> +		scanf_fmts = xmalloc(nr_rules * sizeof(char *) + total_len);
> +
> +		total_len = 0;
> +		for (i = 0; i < nr_rules; i++) {
> +			scanf_fmts[i] = (char *)&scanf_fmts[nr_rules]
> +					+ total_len;
> +			gen_scanf_fmt(scanf_fmts[i], ref_rev_parse_rules[i]);
> +			total_len += strlen(ref_rev_parse_rules[i]);
> +		}
> +	}
> +
> +	/* bail out if there are no rules */
> +	if (!nr_rules)
> +		return xstrdup(ref);
> +
> +	/* buffer for scanf result, at most ref must fit */
> +	short_name = xstrdup(ref);
> +
> +	/* skip first rule, it will always match */
> +	for (i = nr_rules - 1; i > 0 ; --i) {
> +		int j;
> +		int short_name_len;
> +
> +		if (1 != sscanf(ref, scanf_fmts[i], short_name))
> +			continue;
> +
> +		short_name_len = strlen(short_name);
> +
> +		/*
> +		 * check if the short name resolves to a valid ref,
> +		 * but use only rules prior to the matched one
> +		 */
> +		for (j = 0; j < i; j++) {
> +			const char *rule = ref_rev_parse_rules[j];
> +			unsigned char short_objectname[20];
> +			char refname[PATH_MAX];
> +
> +			/*
> +			 * the short name is ambiguous, if it resolves
> +			 * (with this previous rule) to a valid ref
> +			 * read_ref() returns 0 on success
> +			 */
> +			mksnpath(refname, sizeof(refname),
> +				 rule, short_name_len, short_name);
> +			if (!read_ref(refname, short_objectname))
> +				break;
> +		}
> +
> +		/*
> +		 * short name is non-ambiguous if all previous rules
> +		 * haven't resolved to a valid ref
> +		 */
> +		if (j == i)
> +			return short_name;
> +	}
> +
> +	free(short_name);
> +	return xstrdup(ref);
> +}
> diff --git a/refs.h b/refs.h
> index 68c2d16..2d0f961 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -80,6 +80,7 @@ extern int for_each_reflog(each_ref_fn, void *);
>  extern int check_ref_format(const char *target);
>  
>  extern const char *prettify_ref(const struct ref *ref);
> +extern char *shorten_unambiguous_ref(const char *ref);
>  
>  /** rename ref, return 0 on success **/
>  extern int rename_ref(const char *oldref, const char *newref, const char *logmsg);