On Sun, May 5, 2013 at 1:55 AM, Johan Herland <jo...@herland.net> wrote:
> When expanding shorthand refs to full ref names (e.g. in dwim_ref()),
> we use the ref_rev_parse_rules list of expansion patterns. This list
> allows "origin" to be expanded into "refs/remotes/origin/HEAD", by
> using the "refs/remotes/%.*s/HEAD" pattern from that list.
>
> shorten_unambiguous_ref() exists to provide the reverse operation:
> turning a full ref name into a shorter (but still unambiguous) name.
> It does so by matching the given refname against each pattern from
> the ref_rev_parse_rules list (in reverse), and extracting the short-
> hand name from the matching rule.
>
> However, when given "refs/remotes/origin/HEAD" it fails to shorten it
> into "origin", because we misuse the sscanf() function when matching
> "refs/remotes/origin/HEAD" against "refs/remotes/%.*s/HEAD": We end
> up calling sscanf like this:
>
> sscanf("refs/remotes/origin/HEAD", "refs/remotes/%s/HEAD", short_name)
>
> In this case, sscanf() will match the initial "refs/remotes/" part, and
> then match the remainder of the refname against the "%s", and place it
> ("origin/HEAD") into short_name. The part of the pattern following the
> "%s" format is never verified, because sscanf() apparently does not
> need to do that (it has performed the one expected format extraction,
> and will return 1 correspondingly; see [1] for more details).
>
> This patch replaces the misuse of sscanf() with a fairly simple function
> that manually matches the refname against patterns, and extracts the
> shorthand name.
>
> Also a testcase verifying "refs/remotes/origin/HEAD" -> "origin" has
> been added.
>
> [1]: If we assume that sscanf() does not do a verification pass prior
> to format extraction, there is AFAICS _no_ way for sscanf() - having
> already done one or more format extractions - to indicate to its caller
> that the input fails to match the trailing part of the format string.
> In other words, AFAICS, the scanf() family of function will only verify
> matching input up to and including the last format specifier in the
> format string. Any data following the last format specifier will not be
> verified. Yet another reason to consider the scanf functions harmful...
>
> Cc: Bert Wesarg <bert.wes...@googlemail.com>
Looks good, thanks.
Reviewed-by: Bert Wesarg <bert.wes...@googlemail.com>
> Signed-off-by: Johan Herland <jo...@herland.net>
> ---
> refs.c | 82
> +++++++++++++++++++------------------------------
> t/t6300-for-each-ref.sh | 12 ++++++++
> 2 files changed, 43 insertions(+), 51 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index d17931a..7231f54 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2945,80 +2945,60 @@ struct ref *find_ref_by_name(const struct ref *list,
> const char *name)
> 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)
> +int shorten_ref(const char *refname, const char *pattern, char *short_name)
> {
> - 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;
> + /*
> + * pattern must be of the form "[pre]%.*s[post]". Check if refname
> + * starts with "[pre]" and ends with "[post]". If so, write the
> + * middle part into short_name, and return the number of chars
> + * written (not counting the added NUL-terminator). Otherwise,
> + * if refname does not match pattern, return 0.
> + */
> + size_t pre_len, post_start, post_len, match_len;
> + size_t ref_len = strlen(refname);
> + char *sep = strstr(pattern, "%.*s");
> + if (!sep || strstr(sep + 4, "%.*s"))
> + die("invalid pattern in ref_rev_parse_rules: %s", pattern);
> + pre_len = sep - pattern;
> + post_start = pre_len + 4;
> + post_len = strlen(pattern + post_start);
> + if (pre_len + post_len >= ref_len)
> + return 0; /* refname too short */
> + match_len = ref_len - (pre_len + post_len);
> + if (strncmp(refname, pattern, pre_len) ||
> + strncmp(refname + ref_len - post_len, pattern + post_start,
> post_len))
> + return 0; /* refname does not match */
> + memcpy(short_name, refname + pre_len, match_len);
> + short_name[match_len] = '\0';
> + return match_len;
> }
>
> char *shorten_unambiguous_ref(const char *refname, int strict)
> {
> 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(refname);
> -
> /* buffer for scanf result, at most refname must fit */
> short_name = xstrdup(refname);
>
> /* skip first rule, it will always match */
> - for (i = nr_rules - 1; i > 0 ; --i) {
> + for (i = ARRAY_SIZE(ref_rev_parse_rules) - 1; i > 0 ; --i) {
> int j;
> int rules_to_fail = i;
> int short_name_len;
>
> - if (1 != sscanf(refname, scanf_fmts[i], short_name))
> + if (!ref_rev_parse_rules[i] ||
> + !(short_name_len = shorten_ref(refname,
> + ref_rev_parse_rules[i],
> + short_name)))
> continue;
>
> - short_name_len = strlen(short_name);
> -
> /*
> * in strict mode, all (except the matched one) rules
> * must fail to resolve to a valid non-ambiguous ref
> */
> if (strict)
> - rules_to_fail = nr_rules;
> + rules_to_fail = ARRAY_SIZE(ref_rev_parse_rules);
>
> /*
> * check if the short name resolves to a valid ref,
> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index 752f5cb..57e3109 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -466,4 +466,16 @@ test_expect_success 'Verify sort with multiple keys' '
> refs/tags/bogo refs/tags/master > actual &&
> test_cmp expected actual
> '
> +
> +cat >expected <<\EOF
> +origin
> +origin/master
> +EOF
> +
> +test_expect_success 'Check refs/remotes/origin/HEAD shortens to origin' '
> + git remote set-head origin master &&
> + git for-each-ref --format="%(refname:short)" refs/remotes >actual &&
> + test_cmp expected actual
> +'
> +
> test_done
> --
> 1.8.1.3.704.g33f7d4f
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
