Re: [PATCH 1/7] shorten_unambiguous_ref(): Allow shortening refs/remotes/origin/HEAD to origin

2013-05-07 Thread Johan Herland
On Wed, May 8, 2013 at 12:06 AM, Junio C Hamano  wrote:
> Johan Herland  writes:
>
>> ...oops, I see I forgot the trailing && on this line. Do you want a
>> resend, or fix up yourself?
>
> I've pushed out a heavily fixed-up version on 'pu', mostly for
> styles and some log message changes to describe "when it is not a
> symref".

Looks good to me.

Thanks!

-- 
Johan Herland, 
www.herland.net
--
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


Re: [PATCH 1/7] shorten_unambiguous_ref(): Allow shortening refs/remotes/origin/HEAD to origin

2013-05-07 Thread Junio C Hamano
Johan Herland  writes:

> ...oops, I see I forgot the trailing && on this line. Do you want a
> resend, or fix up yourself?

I've pushed out a heavily fixed-up version on 'pu', mostly for
styles and some log message changes to describe "when it is not a
symref".
--
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


Re: [PATCH 1/7] shorten_unambiguous_ref(): Allow shortening refs/remotes/origin/HEAD to origin

2013-05-07 Thread Johan Herland
On Tue, May 7, 2013 at 11:31 PM, Junio C Hamano  wrote:
> Johan Herland  writes:
>> On Mon, May 6, 2013 at 7:52 PM, Junio C Hamano  wrote:
>>> It is interesting that this bug has stayed so long with us, which
>>> may indicate that nobody actually uses the feature at all.
>>
>> I don't know if people really care about whether
>> "refs/remotes/origin/HEAD" shortens to "origin/HEAD" or "origin". I'm
>> guessing that people _do_ depend on the reverse - having "origin"
>> expand into "refs/remotes/origin/HEAD", so we probably cannot rip out
>> the "refs/remotes/%.*s/HEAD" rule altogether...
>
> Oh, no doubt about that reverse conversion.
>
> The real reason nobody cared about refs/remotes/origin/HEAD is that
> nobody sane has anything but non-symbolic ref there.  Your t1514
> does this:
>
> ...
> git update-ref refs/master master_d &&
> test_commit master_e

...oops, I see I forgot the trailing && on this line. Do you want a
resend, or fix up yourself?

> git update-ref refs/remotes/origin/HEAD master_e &&
> ...
>
> Nowhere in the set-up sequence, you see anything that does
>
> git symbolic-ref refs/remotes/origin/HEAD refs/remotes/origin/master
>
> or any other branch we copied from the remote.

Correct. I first did a "git remote set-head origin master", but
quickly discovered that rev-parse resolved the symref as part of
--abbrev-ref, so I had to fake up a non-symref to trigger the
shortening logic I wanted to test.

> And the shortening is done after dereferencing the symbolic ref.
> Because of this, refs/remotes/origin/HEAD usually resolves to
> origin/master, not origin.
>
>  t/t1514-rev-parse-shorten-unambiguous-ref.sh | 7 +++
>  1 file changed, 7 insertions(+)
>
> diff --git a/t/t1514-rev-parse-shorten-unambiguous-ref.sh 
> b/t/t1514-rev-parse-shorten-unambiguous-ref.sh
> index fd87ce3..556ad16 100755
> --- a/t/t1514-rev-parse-shorten-unambiguous-ref.sh
> +++ b/t/t1514-rev-parse-shorten-unambiguous-ref.sh
> @@ -76,4 +76,11 @@ test_expect_success 'shortening refnames in loose mode' '
> test_shortname refs/tags/master loose tags/master master_c
>  '
>
> +test_expect_success 'shortening is done after dereferencing a symref' '
> +   git update-ref refs/remotes/frotz/master master_e &&
> +   git symbolic-ref refs/remotes/frotz/HEAD refs/remotes/frotz/master &&
> +   test_shortname refs/remotes/frotz/HEAD strict frotz/master master_e &&
> +   test_shortname refs/remotes/frotz/HEAD loose frotz/master master_e
> +'
> +
>  test_done

True. I'm not sure whether that's a feature or a bug in --abbrev-ref,
probably a feature.


...Johan

-- 
Johan Herland, 
www.herland.net
--
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


Re: [PATCH 1/7] shorten_unambiguous_ref(): Allow shortening refs/remotes/origin/HEAD to origin

2013-05-07 Thread Junio C Hamano
Johan Herland  writes:

> On Mon, May 6, 2013 at 7:52 PM, Junio C Hamano  wrote:
>> Johan Herland  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.
>
> I don't know if people really care about whether
> "refs/remotes/origin/HEAD" shortens to "origin/HEAD" or "origin". I'm
> guessing that people _do_ depend on the reverse - having "origin"
> expand into "refs/remotes/origin/HEAD", so we probably cannot rip out
> the "refs/remotes/%.*s/HEAD" rule altogether...

Oh, no doubt about that reverse conversion.

The real reason nobody cared about refs/remotes/origin/HEAD is that
nobody sane has anything but non-symbolic ref there.  Your t1514
does this:

...
git update-ref refs/master master_d &&
test_commit master_e
git update-ref refs/remotes/origin/HEAD master_e &&
...

Nowhere in the set-up sequence, you see anything that does

git symbolic-ref refs/remotes/origin/HEAD refs/remotes/origin/master

or any other branch we copied from the remote.

And the shortening is done after dereferencing the synbolic ref.
Because of this, refs/remotes/origin/HEAD usually resolves to
origin/master, not origin.

 t/t1514-rev-parse-shorten-unambiguous-ref.sh | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/t/t1514-rev-parse-shorten-unambiguous-ref.sh 
b/t/t1514-rev-parse-shorten-unambiguous-ref.sh
index fd87ce3..556ad16 100755
--- a/t/t1514-rev-parse-shorten-unambiguous-ref.sh
+++ b/t/t1514-rev-parse-shorten-unambiguous-ref.sh
@@ -76,4 +76,11 @@ test_expect_success 'shortening refnames in loose mode' '
test_shortname refs/tags/master loose tags/master master_c
 '
 
+test_expect_success 'shortening is done after dereferencing a symref' '
+   git update-ref refs/remotes/frotz/master master_e &&
+   git symbolic-ref refs/remotes/frotz/HEAD refs/remotes/frotz/master &&
+   test_shortname refs/remotes/frotz/HEAD strict frotz/master master_e &&
+   test_shortname refs/remotes/frotz/HEAD loose frotz/master master_e
+'
+
 test_done


--
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


Re: [PATCH 1/7] shorten_unambiguous_ref(): Allow shortening refs/remotes/origin/HEAD to origin

2013-05-07 Thread Junio C Hamano
Johan Herland  writes:

> New version coming up. I'm going to rip this patch out of the
> surrounding series, since it doesn't really belong there anyway.

Thanks; will queue.
--
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


Re: [PATCH 1/7] shorten_unambiguous_ref(): Allow shortening refs/remotes/origin/HEAD to origin

2013-05-07 Thread Johan Herland
On Mon, May 6, 2013 at 7:52 PM, Junio C Hamano  wrote:
> Johan Herland  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.

I don't know if people really care about whether
"refs/remotes/origin/HEAD" shortens to "origin/HEAD" or "origin". I'm
guessing that people _do_ depend on the reverse - having "origin"
expand into "refs/remotes/origin/HEAD", so we probably cannot rip out
the "refs/remotes/%.*s/HEAD" rule altogether...

> Good eyes.
>
>> Cc: Bert Wesarg 
>> Signed-off-by: Johan Herland 
>> ---
>>  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?

Nope, it should be static. Will fix.

>>  {
>> + /*
>> +  * 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.

Yeah, I made it sort of explicit to convince myself I'd gotten it
right. I'm sure the same can be expressed in fewer lines of code.

>>  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"?

There are various things being skipped at various points... The
ref_rev_parse_rules array looks like this:

const char *ref_rev_parse_rules[] = {
"%.*s",
"refs/%.*s",
"refs/tags/%.*s",
"refs/heads/%.*s",
"refs/remotes/%.*s",
"refs/remotes/%.*s/HEAD",
NULL
};

Obviously we want to skip looking at the last (sentinel) entry. But
there's also no point in looking at the first, since it trivially
"shortens" to itself.

The for loop in this function:
>> - for (i = nr_rules - 1; i > 0 ; --i) {
>> + for (i = ARRAY_SIZE(ref_rev_parse_rules) - 1; i > 0 ; --i) {

is about skipping the _first_ array entry (we start at the last index,
and stop _before_ we reach 0).

The current line:
>> + if (!ref_rev_parse_rules[i] ||

is about skipping the last (sentinel) entry. The previous code did
this by doing a pre-pass where nr_rules is set to
ARRAY_SIZE(ref_rev_parse_rules) - 1. I should have obviously done the
same by initializing i to ARRAY_SIZE(ref_rev_parse_rules) - 2 in the
above for loop.

> 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 mod

Re: [PATCH 1/7] shorten_unambiguous_ref(): Allow shortening refs/remotes/origin/HEAD to origin

2013-05-06 Thread Junio C Hamano
Johan Herland  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 
> Signed-off-by: Johan Herland 
> ---
>  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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 1/7] shorten_unambiguous_ref(): Allow shortening refs/remotes/origin/HEAD to origin

2013-05-05 Thread Bert Wesarg
On Sun, May 5, 2013 at 1:55 AM, Johan Herland  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 

Looks good, thanks.

Reviewed-by: Bert Wesarg 

> Signed-off-by: Johan Herland 
> ---
>  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 forma

[PATCH 1/7] shorten_unambiguous_ref(): Allow shortening refs/remotes/origin/HEAD to origin

2013-05-04 Thread Johan Herland
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 
Signed-off-by: Johan Herland 
---
 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") */
-