Johan Herland <johan@xxxxxxxxxxx> writes:
> ... 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.
Yeah, we can detect when we did not have enough, but we cannot tell
where it stopped matching.
It is interesting that this bug has stayed so long with us, which
may indicate that nobody actually uses the feature at all.
Good eyes.
>
> Cc: Bert Wesarg <bert.wesarg@xxxxxxxxxxxxxx>
> Signed-off-by: Johan Herland <johan@xxxxxxxxxxx>
> ---
> 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;
> }
>
> +int shorten_ref(const char *refname, const char *pattern, char *short_name)
Does this need to be an extern?
> {
> + /*
> + * 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;
> }
OK. Looks correct, even though I suspect some people might come up
with a more concise way to express the above.
> char *shorten_unambiguous_ref(const char *refname, int strict)
> {
> int i;
> char *short_name;
>
> /* 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 (!ref_rev_parse_rules[i] ||
What is this skippage about? Isn't it what you already compensated
away by starting from "ARRAY_SIZE() - 1"?
Ahh, no. But wait. Isn't there a larger issue here?
> + !(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);
Isn't nr_rules in the original is "ARRAY_SIZE()-1"?
>
> /*
> * check if the short name resolves to a valid ref,
Could you add a test to trigger the "strict" codepath?
Thanks.
> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html