[PATCH 0/7] Make "$remote/$branch" work with unconventional refspecs

[PATCH 0/7] Make "$remote/$branch" work with unconventional refspecs

[Johan Herland]

The "$remote/$branch" syntax can be interpreted in two subtly different
ways:

 1. A shorthand name for the remote-tracking branch corresponding to a
    specific $branch from a specific $remote.

 2. A refname fragment, which - when appended to "refs/remotes/" -
    yields the remote-tracking branch corresponding to a specific
    $branch from a specific $remote.

For the current default/conventional refspecs, both interpretations are
true and equally valid. However, when configuring non-default refspecs
(such as the +refs/heads/*:refs/remotes/origin/heads/*), it becomes
obvious that the current code follows the latter interpretation: The
"$remote/$branch" shorthand will no longer work, and you are forced to
use "$remote/heads/$branch" instead.

I argue that the former interpretation is what most users intuitively
expect, and that we should work towards making Git follow the same
interpretation:

Instead of prepending "refs/remotes/" to convert "$remote/$branch" into
a full refname, we should find "$remote" in the repo config, and then
map "refs/heads/$branch" through $remote's fetch refspec(s), to find
the remote-tracking branch that properly corresponds to the specific
$branch in the $remote repo.

This goal is achieved by the final patch in this series, and most of
the preceding patches serve as preliminary changes and refactoring to
support this.

Patch #1 is the exception in that it is a self-contained bugfix for a
scanf-related problem I ran across while working on the patch series.

Patches #2 and #3 introduce a new test intended to verify Git's
usability when working with remote ref namespaces (which rely on
setting up unconventional refspecs). For now, this test is fairly
thin, but it should be expanded as we find more problems with these
kinds of setups.

Patches #4 and #5 are pure refactorings to reorganize the code that
expands shorthand names to full refnames and vice versa. The idea
is to associate the patterns that are used to expand/shorten ref
names with the actual function that does the expansion/shortening,
so that we can later add patterns that uses different expand/shorten
functions.

Patch #6 teaches Git to realize when - in the context of communication
with a remote repo - it's expanding shorthand refs into either local
refnames, or remote refnames. It is important that any expansion rules
relying on local repo configuration are not allowed to expand shorthand
names on behalf of the remote repo.

Finally, patch #7 introduces a new rule and associated expand/shorten
functions mapping "$remote/$branch"-type shorthand names to/from their
remote-tracking branch counterparts, by using the configured refspecs
as described above. This rule is obviously only applied to local refs,
as it would be wrong for a repo to use its local config to dictate a
ref expansion in a remote repo.

The series has been build on recent 'next', and although it also
applies cleanly to v1.8.3-rc1, it will cause a test failure in
t7900, since it depends on the jh/checkout-auto-tracking topic, which
is currently cooking.


Have fun! :)

...Johan


Johan Herland (7):
  shorten_unambiguous_ref(): Allow shortening refs/remotes/origin/HEAD to origin
  t7900: Start testing usability of namespaced remote refs
  t7900: Demonstrate failure to expand "$remote/$branch" according to refspecs
  refs.c: Refactor rules for expanding shorthand names into full refnames
  refs.c: Refactor code for shortening full refnames into shorthand names
  refname_match(): Caller must declare if we're matching local or remote refs
  refs.c: Add rules for resolving refs using remote refspecs

 cache.h                                        |   4 -
 refs.c                                         | 260 +++++++++++++++++--------
 refs.h                                         |  14 ++
 remote.c                                       |  15 +-
 t/t6300-for-each-ref.sh                        |  12 ++
 t/t7900-working-with-namespaced-remote-refs.sh | 133 +++++++++++++
 6 files changed, 342 insertions(+), 96 deletions(-)
 create mode 100755 t/t7900-working-with-namespaced-remote-refs.sh

-- 
1.8.1.3.704.g33f7d4f

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

[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 <bert.wesarg@googlemail.com>
Signed-off-by: Johan Herland <johan@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

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

[Bert Wesarg]

On Sun, May 5, 2013 at 1:55 AM, Johan Herland <johan@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.wesarg@googlemail.com>

Looks good, thanks.

Reviewed-by: Bert Wesarg <bert.wesarg@googlemail.com>

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

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

[Junio C Hamano]

Johan Herland <johan@herland.net> 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@googlemail.com>
> Signed-off-by: Johan Herland <johan@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;
>  }
>  
> +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

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

[Johan Herland]

On Mon, May 6, 2013 at 7:52 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Johan Herland <johan@herland.net> 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 <bert.wesarg@googlemail.com>
>> Signed-off-by: Johan Herland <johan@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;
>>  }
>>
>> +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 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"?

True. Good catch.

>>
>>               /*
>>                * check if the short name resolves to a valid ref,
>
> Could you add a test to trigger the "strict" codepath?

I imagined the strict codepath was already being tested by the
addition to t6300, seeing as core.warnAmbiguousRef defaults to true.
Obviously I will have to add some more tests to make sure I'm not
screwing things up.

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


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

[PATCHv2 1/3] t1514: Add tests of shortening refnames in strict/loose mode

[Johan Herland]

These tests verify the correct behavior of "git rev-parse --abbrev-ref"
in both "strict" and "loose" modes. Really, it tests the correct behavior
of refs.c:shorten_unambiguous_ref() with its 'strict' argument set to
either true of false.

Signed-off-by: Johan Herland <johan@herland.net>
---
 t/t1514-rev-parse-shorten_unambiguous_ref.sh | 75 ++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)
 create mode 100755 t/t1514-rev-parse-shorten_unambiguous_ref.sh

diff --git a/t/t1514-rev-parse-shorten_unambiguous_ref.sh b/t/t1514-rev-parse-shorten_unambiguous_ref.sh
new file mode 100755
index 0000000..41e0162
--- /dev/null
+++ b/t/t1514-rev-parse-shorten_unambiguous_ref.sh
@@ -0,0 +1,75 @@
+#!/bin/sh
+
+test_description='short refname disambiguation
+
+Create refs that share the same name, and make sure
+"git rev-parse --abbrev-ref" can present them all with as short a name
+as possible, while still being unambiguous.
+'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	test_commit master_a &&
+	git remote add origin . &&
+	git fetch origin &&
+	test_commit master_b &&
+	git branch origin/master &&
+	test_commit master_c &&
+	git tag master &&
+	test_commit master_d &&
+	git update-ref refs/master master_d &&
+	test_commit master_e
+	test_commit master_f
+'
+
+cat > expect.show-ref << EOF
+$(git rev-parse master_f) refs/heads/master
+$(git rev-parse master_b) refs/heads/origin/master
+$(git rev-parse master_d) refs/master
+$(git rev-parse master_a) refs/remotes/origin/master
+$(git rev-parse master_c) refs/tags/master
+$(git rev-parse master_a) refs/tags/master_a
+$(git rev-parse master_b) refs/tags/master_b
+$(git rev-parse master_c) refs/tags/master_c
+$(git rev-parse master_d) refs/tags/master_d
+$(git rev-parse master_e) refs/tags/master_e
+$(git rev-parse master_f) refs/tags/master_f
+EOF
+
+test_expect_success 'we have the expected ref layout' '
+	git show-ref > actual.show-ref &&
+	test_cmp expect.show-ref actual.show-ref
+'
+
+test_shortname () {
+	refname=$1
+	mode=$2
+	expect_shortname=$3
+	expect_tag=$4
+	echo "$expect_shortname" > expect.shortname &&
+	actual_shortname="$(git rev-parse --abbrev-ref="$mode" "$refname")" &&
+	echo "$actual_shortname" > actual.shortname &&
+	test_cmp expect.shortname actual.shortname &&
+	git rev-parse --verify "$expect_tag" > expect.sha1 &&
+	git rev-parse --verify "$actual_shortname" > actual.sha1 &&
+	test_cmp expect.sha1 actual.sha1
+}
+
+test_expect_success 'shortening refnames in strict mode' '
+	test_shortname refs/heads/master strict heads/master master_f &&
+	test_shortname refs/heads/origin/master strict heads/origin/master master_b &&
+	test_shortname refs/master strict refs/master master_d &&
+	test_shortname refs/remotes/origin/master strict remotes/origin/master master_a &&
+	test_shortname refs/tags/master strict tags/master master_c
+'
+
+test_expect_success 'shortening refnames in loose mode' '
+	test_shortname refs/heads/master loose heads/master master_f &&
+	test_shortname refs/heads/origin/master loose origin/master master_b &&
+	test_shortname refs/master loose master master_d &&
+	test_shortname refs/remotes/origin/master loose remotes/origin/master master_a &&
+	test_shortname refs/tags/master loose tags/master master_c
+'
+
+test_done
-- 
1.8.1.3.704.g33f7d4f

[PATCHv2 2/3] t1514: Demonstrate failure to correctly shorten "refs/remotes/origin/HEAD"

[Johan Herland]

There is currently a bug in refs.c:shorten_unambiguous_ref() that causes
"refs/remotes/origin/HEAD" to be shortened to "origin/HEAD" instead of
"origin" (which is expected from matching against the "refs/remotes/%.*s"
pattern from refs.c:ref_rev_parse_rules).

Signed-off-by: Johan Herland <johan@herland.net>
---
 t/t1514-rev-parse-shorten_unambiguous_ref.sh |  8 ++++++--
 t/t6300-for-each-ref.sh                      | 12 ++++++++++++
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/t/t1514-rev-parse-shorten_unambiguous_ref.sh b/t/t1514-rev-parse-shorten_unambiguous_ref.sh
index 41e0162..ad75436 100755
--- a/t/t1514-rev-parse-shorten_unambiguous_ref.sh
+++ b/t/t1514-rev-parse-shorten_unambiguous_ref.sh
@@ -20,6 +20,7 @@ test_expect_success 'setup' '
 	test_commit master_d &&
 	git update-ref refs/master master_d &&
 	test_commit master_e
+	git update-ref refs/remotes/origin/HEAD master_e &&
 	test_commit master_f
 '
 
@@ -27,6 +28,7 @@ cat > expect.show-ref << EOF
 $(git rev-parse master_f) refs/heads/master
 $(git rev-parse master_b) refs/heads/origin/master
 $(git rev-parse master_d) refs/master
+$(git rev-parse master_e) refs/remotes/origin/HEAD
 $(git rev-parse master_a) refs/remotes/origin/master
 $(git rev-parse master_c) refs/tags/master
 $(git rev-parse master_a) refs/tags/master_a
@@ -56,18 +58,20 @@ test_shortname () {
 	test_cmp expect.sha1 actual.sha1
 }
 
-test_expect_success 'shortening refnames in strict mode' '
+test_expect_failure 'shortening refnames in strict mode' '
 	test_shortname refs/heads/master strict heads/master master_f &&
 	test_shortname refs/heads/origin/master strict heads/origin/master master_b &&
 	test_shortname refs/master strict refs/master master_d &&
+	test_shortname refs/remotes/origin/HEAD strict origin master_e &&
 	test_shortname refs/remotes/origin/master strict remotes/origin/master master_a &&
 	test_shortname refs/tags/master strict tags/master master_c
 '
 
-test_expect_success 'shortening refnames in loose mode' '
+test_expect_failure 'shortening refnames in loose mode' '
 	test_shortname refs/heads/master loose heads/master master_f &&
 	test_shortname refs/heads/origin/master loose origin/master master_b &&
 	test_shortname refs/master loose master master_d &&
+	test_shortname refs/remotes/origin/HEAD loose origin master_e &&
 	test_shortname refs/remotes/origin/master loose remotes/origin/master master_a &&
 	test_shortname refs/tags/master loose tags/master master_c
 '
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 752f5cb..5d716c8 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_failure '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

[PATCHv2 3/3] shorten_unambiguous_ref(): Fix shortening refs/remotes/origin/HEAD to origin

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

[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.wesarg@googlemail.com>
Improved-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Johan Herland <johan@herland.net>
---
 refs.c                                       | 77 ++++++++++------------------
 t/t1514-rev-parse-shorten_unambiguous_ref.sh |  4 +-
 t/t6300-for-each-ref.sh                      |  2 +-
 3 files changed, 29 insertions(+), 54 deletions(-)

diff --git a/refs.c b/refs.c
index d17931a..a0ba2fd 100644
--- a/refs.c
+++ b/refs.c
@@ -2945,80 +2945,55 @@ 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)
+static 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]". If refname
+	 * starts with "[pre]" and ends with "[post]", extract the middle
+	 * part into short_name, and return the number of chars in the
+	 * middle part (not counting the added NUL-terminator). Otherwise,
+	 * if refname does not match pattern, return 0.
+	 */
+	int match_len;
+	const char *match_start, *sep = strstr(pattern, "%.*s");
+	if (!sep || strstr(sep + 4, "%.*s"))
+		die("invalid pattern in ref_rev_parse_rules: %s", pattern);
+	match_start = refname + (sep - pattern);
+	match_len = strlen(refname) - (strlen(pattern) - 4);
+	if (match_len <= 0 ||
+	    strncmp(refname, pattern, match_start - refname) ||
+	    strcmp(match_start + match_len, sep + 4))
+		return 0; /* refname does not match */
+	memcpy(short_name, match_start, 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) - 2; i > 0 ; --i) {
 		int j;
 		int rules_to_fail = i;
 		int short_name_len;
 
-		if (1 != sscanf(refname, scanf_fmts[i], short_name))
+		if (!(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) - 1;
 
 		/*
 		 * check if the short name resolves to a valid ref,
diff --git a/t/t1514-rev-parse-shorten_unambiguous_ref.sh b/t/t1514-rev-parse-shorten_unambiguous_ref.sh
index ad75436..fd87ce3 100755
--- a/t/t1514-rev-parse-shorten_unambiguous_ref.sh
+++ b/t/t1514-rev-parse-shorten_unambiguous_ref.sh
@@ -58,7 +58,7 @@ test_shortname () {
 	test_cmp expect.sha1 actual.sha1
 }
 
-test_expect_failure 'shortening refnames in strict mode' '
+test_expect_success 'shortening refnames in strict mode' '
 	test_shortname refs/heads/master strict heads/master master_f &&
 	test_shortname refs/heads/origin/master strict heads/origin/master master_b &&
 	test_shortname refs/master strict refs/master master_d &&
@@ -67,7 +67,7 @@ test_expect_failure 'shortening refnames in strict mode' '
 	test_shortname refs/tags/master strict tags/master master_c
 '
 
-test_expect_failure 'shortening refnames in loose mode' '
+test_expect_success 'shortening refnames in loose mode' '
 	test_shortname refs/heads/master loose heads/master master_f &&
 	test_shortname refs/heads/origin/master loose origin/master master_b &&
 	test_shortname refs/master loose master master_d &&
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 5d716c8..57e3109 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -472,7 +472,7 @@ origin
 origin/master
 EOF
 
-test_expect_failure 'Check refs/remotes/origin/HEAD shortens to origin' '
+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
-- 
1.8.1.3.704.g33f7d4f

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

[Junio C Hamano]

Johan Herland <johan@herland.net> 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.

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

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> On Mon, May 6, 2013 at 7:52 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Johan Herland <johan@herland.net> 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

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

[Johan Herland]

On Tue, May 7, 2013 at 11:31 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Johan Herland <johan@herland.net> writes:
>> On Mon, May 6, 2013 at 7:52 PM, Junio C Hamano <gitster@pobox.com> 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, <johan@herland.net>
www.herland.net

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

[Junio C Hamano]

Johan Herland <johan@herland.net> 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".

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

[Johan Herland]

On Wed, May 8, 2013 at 12:06 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Johan Herland <johan@herland.net> 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, <johan@herland.net>
www.herland.net

[PATCH 2/7] t7900: Start testing usability of namespaced remote refs

[Johan Herland]

Some users are interested in fetching remote refs into a separate namespace
in the local repo. E.g. instead of the usual remote config:

  [remote "origin"]
	fetch = +refs/heads/*:refs/remotes/origin/*
	url = ...

they want to keep remote tags separate from local tags, and they may also
want to fetch other ref types:

  [remote "origin"]
	fetch = +refs/heads/*:refs/remotes/origin/heads/*
	fetch = +refs/tags/*:refs/remotes/origin/tags/*
	fetch = +refs/notes/*:refs/remotes/origin/notes/*
	fetch = +refs/replace/*:refs/remotes/origin/replace/*
	tagopt = "--no-tags"
	url = ...

This configuration creates a separate namespace under refs/remotes/origin/*
mirroring the structure of local refs (under refs/*) where all the relevant
refs from the 'origin' remote can be found.

This patch introduces a test whose main purpose is to verify that git will
work comfortably with this kind of setup. For now, we only verify that it
is possible (though not exactly easy) to establish a clone with the above
configuration, and that fetching into it yields the expected result.

Signed-off-by: Johan Herland <johan@herland.net>
---
 t/t7900-working-with-namespaced-remote-refs.sh | 88 ++++++++++++++++++++++++++
 1 file changed, 88 insertions(+)
 create mode 100755 t/t7900-working-with-namespaced-remote-refs.sh

diff --git a/t/t7900-working-with-namespaced-remote-refs.sh b/t/t7900-working-with-namespaced-remote-refs.sh
new file mode 100755
index 0000000..af03ac9
--- /dev/null
+++ b/t/t7900-working-with-namespaced-remote-refs.sh
@@ -0,0 +1,88 @@
+#!/bin/sh
+
+test_description='testing end-user usability of namespaced remote refs
+
+Set up a local repo with namespaced remote refs, like this:
+
+[remote "origin"]
+	fetch = +refs/heads/*:refs/remotes/origin/heads/*
+	fetch = +refs/tags/*:refs/remotes/origin/tags/*
+	fetch = +refs/notes/*:refs/remotes/origin/notes/*
+	fetch = +refs/replace/*:refs/remotes/origin/replace/*
+	tagopt = "--no-tags"
+	url = ...
+
+Test that the usual end-user operations work as expected with this setup.
+'
+
+. ./test-lib.sh
+
+test_expect_success 'setup server repo' '
+	git init server &&
+	(
+		cd server &&
+		test_commit server_master_a &&
+		git checkout -b other &&
+		test_commit server_other_b &&
+		git checkout master &&
+		test_commit server_master_b
+	)
+'
+
+server_master_a=$(git --git-dir=server/.git rev-parse --verify server_master_a)
+server_master_b=$(git --git-dir=server/.git rev-parse --verify server_master_b)
+server_other_b=$(git --git-dir=server/.git rev-parse --verify server_other_b)
+
+cat > expect.refspecs << EOF
++refs/heads/*:refs/remotes/origin/heads/*
++refs/tags/*:refs/remotes/origin/tags/*
++refs/notes/*:refs/remotes/origin/notes/*
++refs/replace/*:refs/remotes/origin/replace/*
+EOF
+
+cat > expect.show-ref << EOF
+$server_master_b refs/heads/master
+$server_master_b refs/remotes/origin/heads/master
+$server_other_b refs/remotes/origin/heads/other
+$server_master_a refs/remotes/origin/tags/server_master_a
+$server_master_b refs/remotes/origin/tags/server_master_b
+$server_other_b refs/remotes/origin/tags/server_other_b
+EOF
+
+test_clone() {
+	( cd $1 && git config --get-all remote.origin.fetch ) > actual.refspecs &&
+	test_cmp expect.refspecs actual.refspecs &&
+	( cd $1 && git show-ref ) > actual.show-ref &&
+	test_cmp expect.show-ref actual.show-ref
+}
+
+test_expect_failure 'clone with namespaced remote refs' '
+	git clone server client \
+		--config remote.origin.fetch="+refs/heads/*:refs/remotes/origin/heads/*" \
+		--config remote.origin.fetch="+refs/tags/*:refs/remotes/origin/tags/*" \
+		--config remote.origin.fetch="+refs/notes/*:refs/remotes/origin/notes/*" \
+		--config remote.origin.fetch="+refs/replace/*:refs/remotes/origin/replace/*" \
+		--config remote.origin.tagopt "--no-tags" &&
+	test_clone client
+'
+
+# Work-around for the above failure
+test_expect_success 'work-around "clone" with namespaced remote refs' '
+	rm -rf client &&
+	git init client &&
+	(
+		cd client &&
+		git remote add origin ../server &&
+		git config --unset-all remote.origin.fetch &&
+		git config --add remote.origin.fetch "+refs/heads/*:refs/remotes/origin/heads/*" &&
+		git config --add remote.origin.fetch "+refs/tags/*:refs/remotes/origin/tags/*" &&
+		git config --add remote.origin.fetch "+refs/notes/*:refs/remotes/origin/notes/*" &&
+		git config --add remote.origin.fetch "+refs/replace/*:refs/remotes/origin/replace/*" &&
+		git config remote.origin.tagopt "--no-tags" &&
+		git fetch &&
+		git checkout master
+	) &&
+	test_clone client
+'
+
+test_done
-- 
1.8.1.3.704.g33f7d4f

Re: [PATCH 2/7] t7900: Start testing usability of namespaced remote refs

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> +test_expect_success 'work-around "clone" with namespaced remote refs' '
> +	rm -rf client &&
> +	git init client &&
> +	(
> +		cd client &&
> +		git remote add origin ../server &&
> +		git config --unset-all remote.origin.fetch &&
> +		git config --add remote.origin.fetch "+refs/heads/*:refs/remotes/origin/heads/*" &&

If you were to do this, I think you should drop the "remote add
origin" step and illustrate what configuration variables should be
prepared (at least minimally---the final implementation of "git
clone --separate-remote-layout" may add some other configuration
variable as a hint to say "this remote is using the new layout" or
somesuch) in this "client" repository.

That would make the test more self documenting.

I am not convinced that it is a good idea to reuse "remotes/origin"
hierarchy which traditionally has been branches-only like this,
though.  It may be better to use

	refs/$remotes_new_layout/origin/{heads,tags,...}/*

for a value of $remotes_new_layout that is different from "remote",
and teach the dwim_ref() machinery to pay attention to it, to avoid
confusion.  Otherwise, you wouldn't be able to tell between a topic
branch that works on tags named "tags/refactor" under the old layout,
and a tag that marks a good point in a refactoring effort "refactor"
under the new layout.

> +		git config --add remote.origin.fetch "+refs/tags/*:refs/remotes/origin/tags/*" &&
> +		git config --add remote.origin.fetch "+refs/notes/*:refs/remotes/origin/notes/*" &&
> +		git config --add remote.origin.fetch "+refs/replace/*:refs/remotes/origin/replace/*" &&
> +		git config remote.origin.tagopt "--no-tags" &&
> +		git fetch &&
> +		git checkout master
> +	) &&
> +	test_clone client
> +'
> +
> +test_done

Re: [PATCH 2/7] t7900: Start testing usability of namespaced remote refs

[Johan Herland]

On Tue, May 7, 2013 at 3:29 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Johan Herland <johan@herland.net> writes:
>> +test_expect_success 'work-around "clone" with namespaced remote refs' '
>> +     rm -rf client &&
>> +     git init client &&
>> +     (
>> +             cd client &&
>> +             git remote add origin ../server &&
>> +             git config --unset-all remote.origin.fetch &&
>> +             git config --add remote.origin.fetch "+refs/heads/*:refs/remotes/origin/heads/*" &&
>
> If you were to do this, I think you should drop the "remote add
> origin" step and illustrate what configuration variables should be
> prepared (at least minimally---the final implementation of "git
> clone --separate-remote-layout" may add some other configuration
> variable as a hint to say "this remote is using the new layout" or
> somesuch) in this "client" repository.

Sure, I can change the test into doing:

	cd client &&
	git config remote.origin.url ../server &&
	git config --add remote.origin.fetch
"+refs/heads/*:refs/remotes/origin/heads/*" &&
	git config --add remote.origin.fetch
"+refs/tags/*:refs/remotes/origin/tags/*" &&
	git config --add remote.origin.fetch
"+refs/notes/*:refs/remotes/origin/notes/*" &&
	git config --add remote.origin.fetch
"+refs/replace/*:refs/remotes/origin/replace/*" &&
	git config remote.origin.tagopt "--no-tags" &&
	git fetch &&
	git checkout master

> That would make the test more self documenting.
>
> I am not convinced that it is a good idea to reuse "remotes/origin"
> hierarchy which traditionally has been branches-only like this,
> though.  It may be better to use
>
>         refs/$remotes_new_layout/origin/{heads,tags,...}/*
>
> for a value of $remotes_new_layout that is different from "remote",
> and teach the dwim_ref() machinery to pay attention to it, to avoid
> confusion.  Otherwise, you wouldn't be able to tell between a topic
> branch that works on tags named "tags/refactor" under the old layout,
> and a tag that marks a good point in a refactoring effort "refactor"
> under the new layout.

I see your point, although I'm not convinced it is common among users
to have branch names of the "tags/*" form (or tag names of the
"heads/*" form, for that matter). I'm also not sure it's worth messing
with the "remotes" name which has had a long time to work its way into
our brains and into git's user interface.

That said, I could have a go at using "refs/peers/*" instead of
"refs/remotes/*", and see how that works out.

If it sticks, how pervasive do we want this renaming to be? I guess we
don't want to rename the "git remote" command to "git peer" just
yet... What about the config? Do we rename "remote.origin.url" to
"peer.origin.url" for new-style remotes? For how long do you
anticipate having "peers" and "remotes" living side-by-side as
concepts in git?


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCH 2/7] t7900: Start testing usability of namespaced remote refs

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> That said, I could have a go at using "refs/peers/*" instead of
> "refs/remotes/*", and see how that works out.

Hmm, I had "refs/track/" in mind.  Perhaps "peers" may work as well.

> If it sticks, how pervasive do we want this renaming to be? I guess we
> don't want to rename the "git remote" command to "git peer" just
> yet.

If we were to do this, I would expect that the transition would be
similar to the way we introduced the separate remote layout.  The
effort was started at around v1.3.0 era and we allowed the users to
choose the layout when they make a new clone for quite some time,
until we made it the default at v1.5.0 boundary, IIRC.  Let the user
opt into using the new layout first, and then if the new layout
turns out to be vastly more useful than the current one, then the
userbase will welcome it as the new default (and otherwise, it won't
become the new default).

We _should_ be able to tell the layout being used by checking which
of refs/peers/ or refs/remotes/ is populated, but I do not mind if
we added core.remoteLayout configuration variable that explicitly
tells us which, if such an explicit clue turns out necessary.

[PATCH 3/7] t7900: Demonstrate failure to expand "$remote/$branch" according to refspecs

[Johan Herland]

This test verifies that the following expressions all evaluate to the
full refname "refs/remotes/origin/heads/master":

 - refs/remotes/origin/heads/master
 - remotes/origin/heads/master
 - origin/heads/master
 - origin/master

Currently the last of these fail, because the "$remote/$branch" syntax only
works for remotes with conventional (refs/heads/*:refs/remotes/origin/*)
refspecs.

In order for users of namespaced remote refs (or any other unconventional
refspec configuration) to be able to use the "$remote/$branch" syntax, we
need to extend the parsing of "$remote/$branch" expressions to take the
configured refspecs into account (i.e. look up the fetch refspecs for
$remote, and map "refs/heads/$branch" through the refspecs to find the
corresponding remote-tracking branch name).

Mirroring the expansion of the above 4 expressions into the full refname,
the same 4 expression should also be shortened into "origin/master" when
abbreviating them into their shortest unambiguous representation, e.g.
when running "git rev-parse --abbrev-ref" on them. A (currently failing)
test verifying this behavior is also added by this patch.

Signed-off-by: Johan Herland <johan@herland.net>
---
 t/t7900-working-with-namespaced-remote-refs.sh | 28 ++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/t/t7900-working-with-namespaced-remote-refs.sh b/t/t7900-working-with-namespaced-remote-refs.sh
index af03ac9..cc25e76 100755
--- a/t/t7900-working-with-namespaced-remote-refs.sh
+++ b/t/t7900-working-with-namespaced-remote-refs.sh
@@ -85,4 +85,32 @@ test_expect_success 'work-around "clone" with namespaced remote refs' '
 	test_clone client
 '
 
+test_expect_success 'enter client repo' '
+	cd client
+'
+
+test_expect_failure 'short-hand notation expands correctly for remote-tracking branches' '
+	echo refs/remotes/origin/heads/master > expect &&
+	git rev-parse --symbolic-full-name refs/remotes/origin/heads/master > actual &&
+	test_cmp expect actual &&
+	git rev-parse --symbolic-full-name remotes/origin/heads/master > actual &&
+	test_cmp expect actual &&
+	git rev-parse --symbolic-full-name origin/heads/master > actual &&
+	test_cmp expect actual &&
+	git rev-parse --symbolic-full-name origin/master > actual &&
+	test_cmp expect actual
+'
+
+test_expect_failure 'remote-tracking branches are shortened correctly' '
+	echo origin/master > expect &&
+	git rev-parse --abbrev-ref refs/remotes/origin/heads/master > actual &&
+	test_cmp expect actual &&
+	git rev-parse --abbrev-ref remotes/origin/heads/master > actual &&
+	test_cmp expect actual &&
+	git rev-parse --abbrev-ref origin/heads/master > actual &&
+	test_cmp expect actual &&
+	git rev-parse --abbrev-ref origin/master > actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
1.8.1.3.704.g33f7d4f

Re: [PATCH 3/7] t7900: Demonstrate failure to expand "$remote/$branch" according to refspecs

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> This test verifies that the following expressions all evaluate to the
> full refname "refs/remotes/origin/heads/master":

As I've aleady said, I am not convinced that local refname
resolution should pay attention to refspec mapping, so I won't look
at this step.

[PATCH 4/7] refs.c: Refactor rules for expanding shorthand names into full refnames

[Johan Herland]

In preparation for allowing alternative ways of expanding shorthand refs
(like "master") into full refnames (like "refs/heads/master"): Expand
the current ref_rev_parse_rules list into ref_expand_rules, a list of
struct ref_expand_rule objects that encode both an expansion pattern
(e.g. "refs/heads/%.*s") and an associated expansion function
(e.g. the code that applies "master" to "refs/heads/%.*s" to produce
"refs/heads/master"). This allows us to later add expansion rules that
do something other than the current purely textual expansion.

The current expansion behavior is encoded in the new ref_expand_txtly()
helper function, which does the mksnpath() call that were previously
performed by all users of ref_rev_parse_rules.

The end result is identical in behavior to the existing code, but makes
it easier to adjust the way ref expansion happens for remote-tracking
branches in future patches

Most of the existing code that uses ref_rev_parse_rules to expand
shorthand refs are converted to use ref_expand_rules instead.

Signed-off-by: Johan Herland <johan@herland.net>
---
 cache.h  |  4 ----
 refs.c   | 46 +++++++++++++++++++++++++++++++++-------------
 refs.h   | 11 +++++++++++
 remote.c |  6 +++---
 4 files changed, 47 insertions(+), 20 deletions(-)

diff --git a/cache.h b/cache.h
index 7ce9061..6adab04 100644
--- a/cache.h
+++ b/cache.h
@@ -875,10 +875,6 @@ extern int dwim_log(const char *str, int len, unsigned char *sha1, char **ref);
 extern int interpret_branch_name(const char *str, struct strbuf *);
 extern int get_sha1_mb(const char *str, unsigned char *sha1);
 
-extern int refname_match(const char *abbrev_name, const char *full_name, const char **rules);
-extern const char *ref_rev_parse_rules[];
-#define ref_fetch_rules ref_rev_parse_rules
-
 extern int create_symref(const char *ref, const char *refs_heads_master, const char *logmsg);
 extern int validate_headref(const char *ref);
 
diff --git a/refs.c b/refs.c
index 7231f54..8b02140 100644
--- a/refs.c
+++ b/refs.c
@@ -1724,7 +1724,24 @@ const char *prettify_refname(const char *name)
 		0);
 }
 
-const char *ref_rev_parse_rules[] = {
+static void ref_expand_txtly(const struct ref_expand_rule *rule,
+			     char *dst, size_t dst_len,
+			     const char *shortname, size_t shortname_len)
+{
+	mksnpath(dst, dst_len, rule->pattern, shortname_len, shortname);
+}
+
+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 }
+};
+
+static const char *ref_rev_parse_rules[] = {
 	"%.*s",
 	"refs/%.*s",
 	"refs/tags/%.*s",
@@ -1734,15 +1751,17 @@ const char *ref_rev_parse_rules[] = {
 	NULL
 };
 
-int refname_match(const char *abbrev_name, const char *full_name, const char **rules)
+int refname_match(const char *abbrev_name, const char *full_name,
+		  const struct ref_expand_rule *rules)
 {
-	const char **p;
+	const struct ref_expand_rule *p;
 	const int abbrev_name_len = strlen(abbrev_name);
+	char n[PATH_MAX];
 
-	for (p = rules; *p; p++) {
-		if (!strcmp(full_name, mkpath(*p, abbrev_name_len, abbrev_name))) {
+	for (p = rules; p->expand; p++) {
+		p->expand(p, n, sizeof(n), abbrev_name, abbrev_name_len);
+		if (!strcmp(full_name, n))
 			return 1;
-		}
 	}
 
 	return 0;
@@ -1807,21 +1826,22 @@ static char *substitute_branch_name(const char **string, int *len)
 int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref)
 {
 	char *last_branch = substitute_branch_name(&str, &len);
-	const char **p, *r;
+	const struct ref_expand_rule *p;
+	const char *r;
 	int refs_found = 0;
 
 	*ref = NULL;
-	for (p = ref_rev_parse_rules; *p; p++) {
+	for (p = ref_expand_rules; p->expand; p++) {
 		char fullref[PATH_MAX];
 		unsigned char sha1_from_ref[20];
 		unsigned char *this_result;
 		int flag;
 
 		this_result = refs_found ? sha1_from_ref : sha1;
-		mksnpath(fullref, sizeof(fullref), *p, len, str);
+		p->expand(p, fullref, sizeof(fullref), str, len);
 		r = resolve_ref_unsafe(fullref, this_result, 1, &flag);
 		if (r) {
-			if (!refs_found++)
+			if ((!*ref || strcmp(*ref, r)) && !refs_found++)
 				*ref = xstrdup(r);
 			if (!warn_ambiguous_refs)
 				break;
@@ -1838,17 +1858,17 @@ int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref)
 int dwim_log(const char *str, int len, unsigned char *sha1, char **log)
 {
 	char *last_branch = substitute_branch_name(&str, &len);
-	const char **p;
+	const struct ref_expand_rule *p;
 	int logs_found = 0;
 
 	*log = NULL;
-	for (p = ref_rev_parse_rules; *p; p++) {
+	for (p = ref_expand_rules; p->expand; p++) {
 		struct stat st;
 		unsigned char hash[20];
 		char path[PATH_MAX];
 		const char *ref, *it;
 
-		mksnpath(path, sizeof(path), *p, len, str);
+		p->expand(p, path, sizeof(path), str, len);
 		ref = resolve_ref_unsafe(path, hash, 1, NULL);
 		if (!ref)
 			continue;
diff --git a/refs.h b/refs.h
index 8060ed8..85710cb 100644
--- a/refs.h
+++ b/refs.h
@@ -164,6 +164,17 @@ extern int for_each_reflog(each_ref_fn, void *);
 extern int check_refname_format(const char *refname, int flags);
 
 extern const char *prettify_refname(const char *refname);
+
+struct ref_expand_rule {
+	void (*expand)(const struct ref_expand_rule *rule,
+		       char *dst, size_t dst_len,
+		       const char *shortname, size_t shortname_len);
+	const char *pattern;
+};
+extern const struct ref_expand_rule ref_expand_rules[];
+extern int refname_match(const char *abbrev_name, const char *full_name,
+			 const struct ref_expand_rule *rules);
+
 extern char *shorten_unambiguous_ref(const char *refname, int strict);
 
 /** rename ref, return 0 on success **/
diff --git a/remote.c b/remote.c
index 68eb99b..5ef34c9 100644
--- a/remote.c
+++ b/remote.c
@@ -981,7 +981,7 @@ static int count_refspec_match(const char *pattern,
 		char *name = refs->name;
 		int namelen = strlen(name);
 
-		if (!refname_match(pattern, name, ref_rev_parse_rules))
+		if (!refname_match(pattern, name, ref_expand_rules))
 			continue;
 
 		/* A match is "weak" if it is with refs outside
@@ -1499,7 +1499,7 @@ int branch_merge_matches(struct branch *branch,
 {
 	if (!branch || i < 0 || i >= branch->merge_nr)
 		return 0;
-	return refname_match(branch->merge[i]->src, refname, ref_fetch_rules);
+	return refname_match(branch->merge[i]->src, refname, ref_expand_rules);
 }
 
 static int ignore_symref_update(const char *refname)
@@ -1545,7 +1545,7 @@ static const struct ref *find_ref_by_name_abbrev(const struct ref *refs, const c
 {
 	const struct ref *ref;
 	for (ref = refs; ref; ref = ref->next) {
-		if (refname_match(name, ref->name, ref_fetch_rules))
+		if (refname_match(name, ref->name, ref_expand_rules))
 			return ref;
 	}
 	return NULL;
-- 
1.8.1.3.704.g33f7d4f

Re: [PATCH 4/7] refs.c: Refactor rules for expanding shorthand names into full refnames

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> diff --git a/refs.c b/refs.c
> index 7231f54..8b02140 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1724,7 +1724,24 @@ const char *prettify_refname(const char *name)
>  		0);
>  }
>  
> -const char *ref_rev_parse_rules[] = {
> +static void ref_expand_txtly(const struct ref_expand_rule *rule,
> +			     char *dst, size_t dst_len,
> +			     const char *shortname, size_t shortname_len)
> +{
> +	mksnpath(dst, dst_len, rule->pattern, shortname_len, shortname);
> +}
> +
> +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 }
> +};
> +
> +static const char *ref_rev_parse_rules[] = {
>  	"%.*s",
>  	"refs/%.*s",
>  	"refs/tags/%.*s",
> @@ -1734,15 +1751,17 @@ const char *ref_rev_parse_rules[] = {
>  	NULL
>  };
>  
> -int refname_match(const char *abbrev_name, const char *full_name, const char **rules)
> +int refname_match(const char *abbrev_name, const char *full_name,
> +		  const struct ref_expand_rule *rules)
>  {
> -	const char **p;
> +	const struct ref_expand_rule *p;
>  	const int abbrev_name_len = strlen(abbrev_name);
> +	char n[PATH_MAX];

Hmmm, is it too much to ask to do this without a fixed length
buffer?  I think we have long learned the value of using strbuf to
avoid having to worry about buffer overruns.

I am OK with the idea to make ref_expand_rules[] customizable, and
the overall strategy taken by this step to refactor the current code
looks reasonably sensible.

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

[Johan Herland]

shorten_unambiguous_ref() provides the reverse functionality to the
expansion of shorthand names into full refnames, i.e. it takes a full
refname (like "refs/heads/master"), and matches it against a pattern
(like "refs/heads/%.*s") to produce a shorthand name (like "master").

Being the last remaining user of ref_rev_parse_rules list, this patch
converts it to use ref_expand_rules instead. However, as we associated
an expansion function with each expansion rule - to allow for alternative
expansion behavior in the future - we also need to associate a
"shortening" function with each rule, to allow for the reverse operation
to be customized as well.

Therefore, we add a "shorten" function to struct ref_expand_rule, to
encode the shortening of a full refname into a shorthand name. The
relevant rule and the full refname is passed as arguments, and the
resulting shorthand name is returned as an allocated string. If the
refname could not be shortened according to the given rule, NULL is
returned.

The reason for moving the allocation of the shorthand name into the
shortening function, is that one assumes the shortening function itself
will best know exactly how much memory is needed to hold the shorthand
string.

Naturally, we provide a shortening function that encodes the current
textual shortening algorithm - called ref_shorten_txtly() - which is
merely a slight refactoring of the former shorten_ref() function.

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.

Signed-off-by: Johan Herland <johan@herland.net>
---
 refs.c | 110 ++++++++++++++++++++++++++++-------------------------------------
 refs.h |   2 ++
 2 files changed, 50 insertions(+), 62 deletions(-)

diff --git a/refs.c b/refs.c
index 8b02140..a866489 100644
--- a/refs.c
+++ b/refs.c
@@ -1731,24 +1731,41 @@ static void ref_expand_txtly(const struct ref_expand_rule *rule,
 	mksnpath(dst, dst_len, rule->pattern, shortname_len, shortname);
 }
 
-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 }
-};
+static char *ref_shorten_txtly(const struct ref_expand_rule *rule,
+			       const char *refname)
+{
+	/*
+	 * rule->pattern must be of the form "[pre]%.*s[post]". Check if
+	 * refname starts with "[pre]" and ends with "[post]". If so,
+	 * extract the middle part into a newly-allocated buffer, and
+	 * return it. Else - if refname does not match rule->pattern -
+	 * return NULL.
+	 */
+	size_t pre_len, post_start, post_len, match_len;
+	size_t ref_len = strlen(refname);
+	char *sep = strstr(rule->pattern, "%.*s");
+	if (!sep || strstr(sep + 4, "%.*s"))
+		die("invalid pattern in ref_rev_parse_rules_alt: %s", rule->pattern);
+	pre_len = sep - rule->pattern;
+	post_start = pre_len + 4;
+	post_len = strlen(rule->pattern + post_start);
+	if (pre_len + post_len >= ref_len)
+		return NULL; /* refname too short */
+	match_len = ref_len - (pre_len + post_len);
+	if (strncmp(refname, rule->pattern, pre_len) ||
+	    strncmp(refname + ref_len - post_len, rule->pattern + post_start, post_len))
+		return NULL; /* refname does not match */
+	return xstrndup(refname + pre_len, match_len);
+}
 
-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,
@@ -2965,68 +2982,35 @@ 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)
-{
-	/*
-	 * 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;
 	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) {
 		int j;
 		int rules_to_fail = i;
 		int short_name_len;
+		const struct ref_expand_rule *p = ref_expand_rules + i;
 
-		if (!ref_rev_parse_rules[i] ||
-		    !(short_name_len = shorten_ref(refname,
-						   ref_rev_parse_rules[i],
-						   short_name)))
+		if (!p->shorten || !(short_name = p->shorten(p, refname)))
 			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 = ARRAY_SIZE(ref_rev_parse_rules);
+			rules_to_fail = ARRAY_SIZE(ref_expand_rules);
 
 		/*
 		 * check if the short name resolves to a valid ref,
 		 * but use only rules prior to the matched one
 		 */
 		for (j = 0; j < rules_to_fail; j++) {
-			const char *rule = ref_rev_parse_rules[j];
-			char refname[PATH_MAX];
+			const struct ref_expand_rule *q = ref_expand_rules + j;
+			char resolved[PATH_MAX];
 
 			/* skip matched rule */
 			if (i == j)
@@ -3037,10 +3021,12 @@ char *shorten_unambiguous_ref(const char *refname, int strict)
 			 * (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 (ref_exists(refname))
-				break;
+			if (q->expand) {
+				q->expand(q, resolved, sizeof(resolved),
+					  short_name, short_name_len);
+				if (ref_exists(resolved))
+					break;
+			}
 		}
 
 		/*
@@ -3049,9 +3035,9 @@ char *shorten_unambiguous_ref(const char *refname, int strict)
 		 */
 		if (j == rules_to_fail)
 			return short_name;
+		free(short_name);
 	}
 
-	free(short_name);
 	return xstrdup(refname);
 }
 
diff --git a/refs.h b/refs.h
index 85710cb..245af6f 100644
--- a/refs.h
+++ b/refs.h
@@ -169,6 +169,8 @@ struct ref_expand_rule {
 	void (*expand)(const struct ref_expand_rule *rule,
 		       char *dst, size_t dst_len,
 		       const char *shortname, size_t shortname_len);
+	char *(*shorten)(const struct ref_expand_rule *rule,
+			 const char *refname);
 	const char *pattern;
 };
 extern const struct ref_expand_rule ref_expand_rules[];
-- 
1.8.1.3.704.g33f7d4f

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

[Junio C Hamano]

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.

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

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

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

[PATCH 6/7] refname_match(): Caller must declare if we're matching local or remote refs

[Johan Herland]

refname_match() is used to check whether a given shorthand name matches a
given full refname, but that full refname does not always belong in the
local repo, rather it is sometimes taken from list of refs sent over from
a remote repo.

In preparation for adding alternative ways of expanding refspecs, we
need to take into account that such alternative ways may depend on
local repo configuration, and it would therefore be wrong to use them
when expanding refnames for a remote repo.

To resolve this, we split the ref_expand_rules list into a local and a
remote variant, and teach the callers of refname_match() to pass the
correct list as third argument to refname_match().

For now, the remote and local lists are identical, but this will change
in a subsequent patch.

The other functions that use ref_expand_rules, all use it in a local
context, so they hardcode the use of the local variant.

Signed-off-by: Johan Herland <johan@herland.net>
---
 refs.c   | 24 +++++++++++++++++-------
 refs.h   |  3 ++-
 remote.c | 15 +++++++++------
 3 files changed, 28 insertions(+), 14 deletions(-)

diff --git a/refs.c b/refs.c
index a866489..98997c4 100644
--- a/refs.c
+++ b/refs.c
@@ -1758,7 +1758,17 @@ static char *ref_shorten_txtly(const struct ref_expand_rule *rule,
 	return xstrndup(refname + pre_len, match_len);
 }
 
-const struct ref_expand_rule ref_expand_rules[] = {
+const struct ref_expand_rule ref_expand_rules_local[] = {
+	{ 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 }
+};
+
+const struct ref_expand_rule ref_expand_rules_remote[] = {
 	{ ref_expand_txtly, NULL, "%.*s" },
 	{ ref_expand_txtly, ref_shorten_txtly, "refs/%.*s" },
 	{ ref_expand_txtly, ref_shorten_txtly, "refs/tags/%.*s" },
@@ -1848,7 +1858,7 @@ int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref)
 	int refs_found = 0;
 
 	*ref = NULL;
-	for (p = ref_expand_rules; p->expand; p++) {
+	for (p = ref_expand_rules_local; p->expand; p++) {
 		char fullref[PATH_MAX];
 		unsigned char sha1_from_ref[20];
 		unsigned char *this_result;
@@ -1879,7 +1889,7 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log)
 	int logs_found = 0;
 
 	*log = NULL;
-	for (p = ref_expand_rules; p->expand; p++) {
+	for (p = ref_expand_rules_local; p->expand; p++) {
 		struct stat st;
 		unsigned char hash[20];
 		char path[PATH_MAX];
@@ -2987,11 +2997,11 @@ char *shorten_unambiguous_ref(const char *refname, int strict)
 	int i;
 	char *short_name;
 
-	for (i = ARRAY_SIZE(ref_expand_rules) - 1; i >= 0 ; --i) {
+	for (i = ARRAY_SIZE(ref_expand_rules_local) - 1; i >= 0 ; --i) {
 		int j;
 		int rules_to_fail = i;
 		int short_name_len;
-		const struct ref_expand_rule *p = ref_expand_rules + i;
+		const struct ref_expand_rule *p = ref_expand_rules_local + i;
 
 		if (!p->shorten || !(short_name = p->shorten(p, refname)))
 			continue;
@@ -3002,14 +3012,14 @@ char *shorten_unambiguous_ref(const char *refname, int strict)
 		 * must fail to resolve to a valid non-ambiguous ref
 		 */
 		if (strict)
-			rules_to_fail = ARRAY_SIZE(ref_expand_rules);
+			rules_to_fail = ARRAY_SIZE(ref_expand_rules_local);
 
 		/*
 		 * check if the short name resolves to a valid ref,
 		 * but use only rules prior to the matched one
 		 */
 		for (j = 0; j < rules_to_fail; j++) {
-			const struct ref_expand_rule *q = ref_expand_rules + j;
+			const struct ref_expand_rule *q = ref_expand_rules_local + j;
 			char resolved[PATH_MAX];
 
 			/* skip matched rule */
diff --git a/refs.h b/refs.h
index 245af6f..15b5ec2 100644
--- a/refs.h
+++ b/refs.h
@@ -173,7 +173,8 @@ struct ref_expand_rule {
 			 const char *refname);
 	const char *pattern;
 };
-extern const struct ref_expand_rule ref_expand_rules[];
+extern const struct ref_expand_rule ref_expand_rules_local[];
+extern const struct ref_expand_rule ref_expand_rules_remote[];
 extern int refname_match(const char *abbrev_name, const char *full_name,
 			 const struct ref_expand_rule *rules);
 
diff --git a/remote.c b/remote.c
index 5ef34c9..379577c 100644
--- a/remote.c
+++ b/remote.c
@@ -969,7 +969,8 @@ void sort_ref_list(struct ref **l, int (*cmp)(const void *, const void *))
 
 static int count_refspec_match(const char *pattern,
 			       struct ref *refs,
-			       struct ref **matched_ref)
+			       struct ref **matched_ref,
+			       int refs_from_remote)
 {
 	int patlen = strlen(pattern);
 	struct ref *matched_weak = NULL;
@@ -981,7 +982,9 @@ static int count_refspec_match(const char *pattern,
 		char *name = refs->name;
 		int namelen = strlen(name);
 
-		if (!refname_match(pattern, name, ref_expand_rules))
+		if (!refname_match(pattern, name, refs_from_remote ?
+				   ref_expand_rules_remote :
+				   ref_expand_rules_local))
 			continue;
 
 		/* A match is "weak" if it is with refs outside
@@ -1091,7 +1094,7 @@ static int match_explicit(struct ref *src, struct ref *dst,
 		return 0;
 
 	matched_src = matched_dst = NULL;
-	switch (count_refspec_match(rs->src, src, &matched_src)) {
+	switch (count_refspec_match(rs->src, src, &matched_src, 0)) {
 	case 1:
 		copy_src = 1;
 		break;
@@ -1121,7 +1124,7 @@ static int match_explicit(struct ref *src, struct ref *dst,
 			    matched_src->name);
 	}
 
-	switch (count_refspec_match(dst_value, dst, &matched_dst)) {
+	switch (count_refspec_match(dst_value, dst, &matched_dst, 1)) {
 	case 1:
 		break;
 	case 0:
@@ -1499,7 +1502,7 @@ int branch_merge_matches(struct branch *branch,
 {
 	if (!branch || i < 0 || i >= branch->merge_nr)
 		return 0;
-	return refname_match(branch->merge[i]->src, refname, ref_expand_rules);
+	return refname_match(branch->merge[i]->src, refname, ref_expand_rules_local);
 }
 
 static int ignore_symref_update(const char *refname)
@@ -1545,7 +1548,7 @@ static const struct ref *find_ref_by_name_abbrev(const struct ref *refs, const c
 {
 	const struct ref *ref;
 	for (ref = refs; ref; ref = ref->next) {
-		if (refname_match(name, ref->name, ref_expand_rules))
+		if (refname_match(name, ref->name, ref_expand_rules_remote))
 			return ref;
 	}
 	return NULL;
-- 
1.8.1.3.704.g33f7d4f

Re: [PATCH 6/7] refname_match(): Caller must declare if we're matching local or remote refs

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> refname_match() is used to check whether a given shorthand name matches a
> given full refname, but that full refname does not always belong in the
> local repo, rather it is sometimes taken from list of refs sent over from
> a remote repo.

That "local vs remote" is a wrong way to think about things.

All refs you can feed to resolve_ref() and dwim_ref() are local, and
"remotes" is not all that special.  It is just a convention to store
many different kind of things per different hierarchy, and the only
special hierarchy we have is refs/heads/* that can be updated by
making new commits through the index and the working tree.

[PATCH 7/7] refs.c: Add rules for resolving refs using remote refspecs

[Johan Herland]

The "$remote/$branch" expression is often used as a shorthand with the
intention of referring to the remote-tracking branch corresponding to
the given $branch in the $remote repo.

Currently, Git resolves this by prepending "refs/remotes/" to the given
"$remote/$branch" expression, resulting in "refs/remotes/$remote/$branch",
which is equivalent to the above intention only when conventional
refspecs are being used (e.g. refs/heads/*:refs/remotes/origin/*).
Correspondingly, a full remote-tracking branch name, can only be
shortened to the "$remote/$branch" form if it textually matches the
"refs/remotes/$remote/$branch" format.

If unconventional refspecs (e.g. refs/heads/*:refs/remotes/origin/heads/*)
are being used, then the current expansion ("refs/remotes/$remote/$branch")
fails to match the remote-tracking branches from the configured remotes.
Ditto for the corresponding shortening.

Instead of doing pure textual expansion (from "$remote/$branch" to
"refs/remotes/$remote/$branch"), we should expand by finding all remotes
matching "$remote", look up their fetch refspecs, and map
"refs/heads/$branch" through them to find the appropriate remote-tracking
branch name corresponding to "$remote/$branch". This would yield the
correct remote-tracking branch in all cases, both for conventional and
unconventional refspecs.

Likewise, when shortening full refnames for remote-tracking branches into
their shorthand form ("$remote/$branch"), we should find the refspec with
the RHS that matches the full remote-tracking branch name, and map
through that refspec to produce the corresponding LHS (minus the leading
"refs/heads/" part), and from that construct the corresponding
"$remote/$branch" shorthand.

This patch adds a new expansion method - ref_expand_refspec() - and a
corresponding shortening method - ref_shorten_refspec() - that implements
the remote refspec traversal and expanding/shortening logic described
above. These expand/shorten methods complement the existing
ref_expand_txtly() and ref_shorten_txtly() methods that implement the
current textual expanding/shortening logic.

Implementing the proper expanding/shortening of "$remote/$branch" is now
a simple matter of adding another entry to the ref_expand_rules list,
using the new ref_expand_refspec() and ref_shorten_refspec() functions.

Note that the existing "refs/remotes/*" textual expansion/shortening rule
is kept to preserve backwards compatibility for refs under refs/remotes/*
that are not covered by any configured refspec.

Signed-off-by: Johan Herland <johan@herland.net>
---
 refs.c                                         | 98 +++++++++++++++++++++++++-
 t/t7900-working-with-namespaced-remote-refs.sh | 21 +++++-
 2 files changed, 114 insertions(+), 5 deletions(-)

diff --git a/refs.c b/refs.c
index 98997c4..18d7188 100644
--- a/refs.c
+++ b/refs.c
@@ -4,6 +4,7 @@
 #include "tag.h"
 #include "dir.h"
 #include "string-list.h"
+#include "remote.h"
 
 /*
  * Make sure "ref" is something reasonable to have under ".git/refs/";
@@ -1758,12 +1759,102 @@ static char *ref_shorten_txtly(const struct ref_expand_rule *rule,
 	return xstrndup(refname + pre_len, match_len);
 }
 
+struct ref_expand_refspec_helper_data {
+	const struct ref_expand_rule *rule;
+	char *dst;
+	size_t dst_len;
+	const char *src;
+	size_t src_len;
+};
+
+static int ref_expand_refspec_helper(struct remote *remote, void *cb_data)
+{
+	struct ref_expand_refspec_helper_data *cb = cb_data;
+	struct refspec query;
+	char refspec_src[PATH_MAX];
+	size_t ref_start = strlen(remote->name) + 1;
+	if (prefixcmp(cb->src, remote->name) ||
+	    cb->src_len <= ref_start ||
+	    cb->src[ref_start - 1] != '/')
+		return 0;
+
+	mksnpath(refspec_src, sizeof(refspec_src), cb->rule->pattern,
+		 cb->src_len - ref_start, cb->src + ref_start);
+
+	memset(&query, 0, sizeof(struct refspec));
+	query.src = refspec_src;
+	if ((!remote_find_tracking(remote, &query)) &&
+	    strlen(query.dst) < cb->dst_len) {
+		strcpy(cb->dst, query.dst);
+		return 1;
+	}
+	return 0;
+}
+
+static void ref_expand_refspec(const struct ref_expand_rule *rule,
+			       char *dst, size_t dst_len,
+			       const char *shortname, size_t shortname_len)
+{
+	/*
+	 * Given shortname of the form "$remote/$ref", see if there is a
+	 * fetch refspec configured for $remote whose lhs matches
+	 * rule->pattern % $ref, and use the corresponding rhs of that
+	 * mapping as the expanded result of "$remote/$ref".
+	 */
+	const void *has_slash = memchr(shortname, '/', shortname_len);
+	struct ref_expand_refspec_helper_data cb = {
+		rule, dst, dst_len, shortname, shortname_len };
+	dst[0] = '\0';
+	if (has_slash)
+		for_each_remote(ref_expand_refspec_helper, &cb);
+}
+
+static int ref_shorten_refspec_helper(struct remote *remote, void *cb_data)
+{
+	struct ref_expand_refspec_helper_data *cb = cb_data;
+	struct refspec query;
+	char *lhs_ref;
+	int ret = 0;
+
+	memset(&query, 0, sizeof(struct refspec));
+	query.dst = (char *) cb->src;
+	if (!remote_find_tracking(remote, &query) &&
+	    (lhs_ref = ref_shorten_txtly(cb->rule, query.src))) {
+		/* refname matches rhs and rule->pattern matches lhs */
+		cb->dst_len = strlen(remote->name) + 1 + strlen(lhs_ref);
+		/* "$remote/$lhs_ref" should be shorter than src */
+		if (cb->dst_len < cb->src_len) {
+			cb->dst = xmalloc(cb->dst_len + 1);
+			snprintf(cb->dst, cb->dst_len + 1,
+				 "%s/%s", remote->name, lhs_ref);
+			ret = 1;
+		}
+		free(lhs_ref);
+	}
+	return ret;
+}
+
+static char *ref_shorten_refspec(const struct ref_expand_rule *rule,
+				 const char *refname)
+{
+	/*
+	 * See if there is a $remote with a fetch refspec that matches the
+	 * given refname on rhs, and will produce refs/heads/$ref on the
+	 * lhs. If so, construct "$remote/$ref" as the shorthand.
+	 */
+	struct ref_expand_refspec_helper_data cb = {
+		rule, NULL, 0, refname, strlen(refname) };
+	for_each_remote(ref_shorten_refspec_helper, &cb);
+	return cb.dst;
+}
+
 const struct ref_expand_rule ref_expand_rules_local[] = {
 	{ 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_refspec, ref_shorten_refspec, "refs/heads/%.*s" },
 	{ ref_expand_txtly, ref_shorten_txtly, "refs/remotes/%.*s/HEAD" },
 	{ NULL, NULL, NULL }
 };
@@ -3028,13 +3119,14 @@ char *shorten_unambiguous_ref(const char *refname, int strict)
 
 			/*
 			 * the short name is ambiguous, if it resolves
-			 * (with this previous rule) to a valid ref
-			 * read_ref() returns 0 on success
+			 * (with this previous rule) to a valid
+			 * (but different) ref
 			 */
 			if (q->expand) {
 				q->expand(q, resolved, sizeof(resolved),
 					  short_name, short_name_len);
-				if (ref_exists(resolved))
+				if (strcmp(refname, resolved) &&
+				    ref_exists(resolved))
 					break;
 			}
 		}
diff --git a/t/t7900-working-with-namespaced-remote-refs.sh b/t/t7900-working-with-namespaced-remote-refs.sh
index cc25e76..302083e 100755
--- a/t/t7900-working-with-namespaced-remote-refs.sh
+++ b/t/t7900-working-with-namespaced-remote-refs.sh
@@ -89,7 +89,7 @@ test_expect_success 'enter client repo' '
 	cd client
 '
 
-test_expect_failure 'short-hand notation expands correctly for remote-tracking branches' '
+test_expect_success 'short-hand notation expands correctly for remote-tracking branches' '
 	echo refs/remotes/origin/heads/master > expect &&
 	git rev-parse --symbolic-full-name refs/remotes/origin/heads/master > actual &&
 	test_cmp expect actual &&
@@ -101,7 +101,7 @@ test_expect_failure 'short-hand notation expands correctly for remote-tracking b
 	test_cmp expect actual
 '
 
-test_expect_failure 'remote-tracking branches are shortened correctly' '
+test_expect_success 'remote-tracking branches are shortened correctly' '
 	echo origin/master > expect &&
 	git rev-parse --abbrev-ref refs/remotes/origin/heads/master > actual &&
 	test_cmp expect actual &&
@@ -113,4 +113,21 @@ test_expect_failure 'remote-tracking branches are shortened correctly' '
 	test_cmp expect actual
 '
 
+cat > expect.origin_master << EOF
+$server_master_b
+$server_master_a
+EOF
+
+cat > expect.origin_other << EOF
+$server_other_b
+$server_master_a
+EOF
+
+test_expect_success 'rev-list machinery should work with $remote/$branch' '
+	git rev-list origin/master > actual.origin_master &&
+	test_cmp expect.origin_master actual.origin_master &&
+	git rev-list origin/other > actual.origin_other &&
+	test_cmp expect.origin_other actual.origin_other
+'
+
 test_done
-- 
1.8.1.3.704.g33f7d4f

Re: [PATCH 0/7] Make "$remote/$branch" work with unconventional refspecs

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> The "$remote/$branch" syntax can be interpreted in two subtly different
> ways:
>
>  1. A shorthand name for the remote-tracking branch corresponding to a
>     specific $branch from a specific $remote.
>
>  2. A refname fragment, which - when appended to "refs/remotes/" -
>     yields the remote-tracking branch corresponding to a specific
>     $branch from a specific $remote.

I think both of the above are somewhat distorted views and they go
against all the documentation we have so far.  The real definition
is:

   3. $string (which may happen to have one or more slashes) is used
      by prepending a few common prefixes to see if the result forms
      a full refname, and refs/remotes/ is one of the prefixes.
      origin/master ends up referring refs/remotes/origin/master
      because of this.

> However, when configuring non-default refspecs
> (such as the +refs/heads/*:refs/remotes/origin/heads/*), it becomes
> obvious that the current code follows the latter interpretation: The
> "$remote/$branch" shorthand will no longer work, and you are forced to
> use "$remote/heads/$branch" instead.

While I do _not_ think it is _wrong_ to use remotes/origin/heads/*
as a namespace for branches you copy from the 'origin' remote, my
gut feeling is that it is myopic to redefine that origin/master
resolves to refs/remotes/origin/heads/master [*1*].

Step back a bit.

There must be a reason why somebody wants remotes/origin/heads/*
instead of the traditional remotes/origin/* to keep the copies of
branches taken from the origin.

It is because she wants to use the parts of remotes/origin/ that are
outside remote/origin/heads/ to store other things taken from that
remote, no?  They may be "changes", "pull-requests", "notes", etc.

If origin/master were to map to refs/remotes/origin/heads/master and
origin/jh/rtrack were to map to refs/remotes/origin/heads/jh/rtrack,
[*2*] what short-hands hierarchies in refs/remotes/origin/ other
than "heads/" would have?

If you do not special case "heads/",

    $ git merge origin/pull-requests/4

is very straightforward to understand and explain when you use the
definition #3 above.  But if you do, then the above may refer to
origin/heads/pull-requests/4, or perhaps there is no pull-requests/4
branch in the origin and the resolution may have to error out.

While I do not reject refs/remotes/origin/heads/* layout as a
possibility, I am somewhat skeptical that any "solution" that starts
from the "two interpretations" above (both of which are flawed, that
only consider what happens to the branches) will yield a generally
useful result.

If the final end result you are shooting for is to introduce an
extra level between the remote name and the branch names, i.e.
"heads/", any solution needs to at least have a plan (not necessarily
a detailed design or implementation) for the other hierarchies.  The
possibility to have these other hierarchies per remote is the true
progress that the "heads/" at that level can give us; there is not
much point to have heads/ after refs/remotes/origin/, if heads/ is
the only thing that can come there.


[Footnotes]

*1* Unlike the usual cautions from me, this does not have anything
    to do with backward compatibility; it is more about forward
    thinking.

*2* Wait.
    Does origin/jh/rtrack map to refs/remotes/origin/jh/heads/rtrack
    which is rtrack branch taken from the origin/jh remote?

Re: [PATCH 0/7] Make "$remote/$branch" work with unconventional refspecs

[Johan Herland]

On Sun, May 5, 2013 at 6:28 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Johan Herland <johan@herland.net> writes:
>
>> The "$remote/$branch" syntax can be interpreted in two subtly different
>> ways:
>>
>>  1. A shorthand name for the remote-tracking branch corresponding to a
>>     specific $branch from a specific $remote.
>>
>>  2. A refname fragment, which - when appended to "refs/remotes/" -
>>     yields the remote-tracking branch corresponding to a specific
>>     $branch from a specific $remote.
>
> I think both of the above are somewhat distorted views and they go
> against all the documentation we have so far.  The real definition
> is:
>
>    3. $string (which may happen to have one or more slashes) is used
>       by prepending a few common prefixes to see if the result forms
>       a full refname, and refs/remotes/ is one of the prefixes.
>       origin/master ends up referring refs/remotes/origin/master
>       because of this.

True, but from the POV of remote-tracking branches (which is what this
series is concerned about, more on that below), #2 and #3 are equivalent.

>> However, when configuring non-default refspecs
>> (such as the +refs/heads/*:refs/remotes/origin/heads/*), it becomes
>> obvious that the current code follows the latter interpretation: The
>> "$remote/$branch" shorthand will no longer work, and you are forced to
>> use "$remote/heads/$branch" instead.
>
> While I do _not_ think it is _wrong_ to use remotes/origin/heads/*
> as a namespace for branches you copy from the 'origin' remote, my
> gut feeling is that it is myopic to redefine that origin/master
> resolves to refs/remotes/origin/heads/master [*1*].

I guess my point is to "raise" the origin/master notation to a higher
level of abstraction. Instead of it being purely a string that is
appended to some common prefixes to yielf a (hopefully unambiguous) match,
I want Git to support the idea that it refers to the "master" shorthand
from the "origin" remote. In other words, just as you can write
"$something" to refer to a ref in the local repo, you can also write
"$remote/$something" to refer to a remote-tracking ref from another repo.

> Step back a bit.
>
> There must be a reason why somebody wants remotes/origin/heads/*
> instead of the traditional remotes/origin/* to keep the copies of
> branches taken from the origin.
>
> It is because she wants to use the parts of remotes/origin/ that are
> outside remote/origin/heads/ to store other things taken from that
> remote, no?  They may be "changes", "pull-requests", "notes", etc.

Indeed.

> If origin/master were to map to refs/remotes/origin/heads/master and
> origin/jh/rtrack were to map to refs/remotes/origin/heads/jh/rtrack,
> [*2*] what short-hands hierarchies in refs/remotes/origin/ other
> than "heads/" would have?

The same. Today, you can use "eggs" to refer to either "refs/heads/eggs"
or "refs/tags/eggs", depending on which exists in your repo. (Having both
exist is also possible, but then you must express more explicitly which of
them you mean). In the same way, I want "origin/eggs" to refer to either
"refs/remotes/origin/heads/eggs" or "refs/remotes/origin/tags/eggs",
depending on which is available (with the same behavior when both exist).

This series only gets us halfway there, the other half consists of making
Git work properly with "remote tags" (i.e. teaching Git to handle
"refs/remotes/$remote/tags/*" as an extension of "refs/tags/*").

Part of that would surely be to add another rule that maps "$remote/$tag"
through $remote's fetch refspec for tags to yield the corresponding
remote-tracking tag (e.g. "refs/remotes/$remote/tags/$tag" if you're
using namespaced remote refs). This rule would should cause problems for
current users since it would map to _nothing_ if you're using the default
refspec, alternatively "refs/tags/$tag" if you have explicitly set up
mirroring of tags from the remote (refs/tags/*:refs/tags/*).

> If you do not special case "heads/",
>
>     $ git merge origin/pull-requests/4
>
> is very straightforward to understand and explain when you use the
> definition #3 above.  But if you do, then the above may refer to
> origin/heads/pull-requests/4, or perhaps there is no pull-requests/4
> branch in the origin and the resolution may have to error out.

Let's say that in the origin repo there may be either refs/pull-requests/4
or refs/heads/pull-requests/4, and they are fetched into our repo, and
placed at either refs/remotes/origin/pull-requests/4 or
refs/remotes/origin/heads/pull-requests/4, respectively. (If both are
present, and fetched, we will get both in our local repo).

When doing your

    $ git merge origin/pull-requests/4

we would either expand to refs/remotes/origin/pull-requests/4 (with the
existing "refs/remotes/%.*s" rule) if that exists, or alternatively expand
to refs/remotes/origin/heads/pull-requests/4 (with the new refspec-mapping
rule from this series) if that exists. (If both exist, you would have to
be more explicit to resolve the ambiguity - the same ambiguity that would
have to be resolved if you did "git merge pull-requests/4" in the origin
repo.

The rules that expand shorthand names into full refnames, are all about
balancing convenience against ambiguity. We want users to be able use as
short names as possible, as long as they are unambiguous. As part of that
we have determined that "foo" can be auto-completed into any of

  foo
  refs/foo
  refs/tags/foo
  refs/heads/foo

without causing ambiguity in the common case (obviously all of these could
exist, and the user would have to be more explicit, but in most repos, at
most one of these exist, so we are fine).

I want to extend the same reasoning to remote-tracking refs, i.e.
"$remote/$name" could be auto-completed into any of

  refs/remotes/$remote/$name
  refs/remotes/$remote/tags/$name
  refs/remotes/$remote/heads/$name

without causing ambiguity in the common case. When there is ambiguity, we
would resolve that in the same manner as for local refs.

> While I do not reject refs/remotes/origin/heads/* layout as a
> possibility, I am somewhat skeptical that any "solution" that starts
> from the "two interpretations" above (both of which are flawed, that
> only consider what happens to the branches) will yield a generally
> useful result.

The current series only concerns itself with the branches, but the larger
intention is to make it work for tags and other refs as well.

> If the final end result you are shooting for is to introduce an
> extra level between the remote name and the branch names, i.e.
> "heads/", any solution needs to at least have a plan (not necessarily
> a detailed design or implementation) for the other hierarchies.  The
> possibility to have these other hierarchies per remote is the true
> progress that the "heads/" at that level can give us; there is not
> much point to have heads/ after refs/remotes/origin/, if heads/ is
> the only thing that can come there.

I fully agree. This series was meant as the first step in that direction
(sorry for not describing my intentions more clearly).

I believe "the plan" that you request might have been largely described in
the large "[1.8.0] Provide proper remote ref namespaces" thread, but this
thread is now more than 2 years old, and although I re-read some of it to
prepare for this series, it was stupid of me to assume that it would still
be fresh in the minds of everyone else... I hope my answers above have
clarified the plan somewhat.

> [Footnotes]
>
> *1* Unlike the usual cautions from me, this does not have anything
>     to do with backward compatibility; it is more about forward
>     thinking.
>
> *2* Wait.
>     Does origin/jh/rtrack map to refs/remotes/origin/jh/heads/rtrack
>     which is rtrack branch taken from the origin/jh remote?

There is an inherent ambiguity when interpreting "foo/bar/baz" as
"$remote/$ref", whether to do

  $remote = foo/bar and $ref = baz

or

  $remote = foo and $ref = bar/baz

My opinion on this is:

 1. Having both remotes "foo" and "foo/bar" in the same repo is _insane_,
    and this scenario would cause problems for current Git as well.

 2. Remote ref namespaces marginally improves the situation by forcing the
    middle fragment ("bar" in the above example) to be one of "heads",
    "tags", etc.; otherwise the refs won't clobber eachother.

 3. Currently, this series does not explicitly handle this insanity.
    The code in patch 7/7 (refs.c:ref_expand_refspec()) stops at the
    first remote that matches a prefix of the given shorthand, so both
    alternatives won't be tested. This could be fixed by moving the ref
    verification (does the refname actually exist/resolve?) into the
    expansion, but I'm not sure it's worth it.

 4. The proper way to deal with this is probably to detect when a new
    remote name is a prefix of another remote name (or vice versa) in
    "git remote add" and "git remote rename", and warn or error out.


Hope this helps,

...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCH 0/7] Make "$remote/$branch" work with unconventional refspecs

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> I want to extend the same reasoning to remote-tracking refs, i.e.
> "$remote/$name" could be auto-completed into any of
>
>   refs/remotes/$remote/$name
>   refs/remotes/$remote/tags/$name
>   refs/remotes/$remote/heads/$name
>
> without causing ambiguity in the common case. When there is ambiguity, we
> would resolve that in the same manner as for local refs.
>
> The current series only concerns itself with the branches, but the larger
> intention is to make it work for tags and other refs as well.

Good ;-).

So another issue that remains is the following, I think.

When interpreting $nick/$name, assuming that we can tell where $nick
for a remote ends and $name for the ref we take from the remote
begins [*1*], how would we determine which refs/remotes/$remote/ is
used for $nick?

My gut feeling is that we should ignore any "remote.$nick.fetch"
wildcard mapping, e.g.

    [remote "foo"]
        fetch = +refs/heads/*:refs/remotes/bar/heads/*
        fetch = +refs/tags/*:refs/remotes/baz/tags/*

so that we look always in refs/remotes/$nick/ somewhere, for at
least two reasons:

 * For sane people, "bar" and "baz" in the above example are both
   "foo", so ignoring remote.foo.fetch mapping is a no-op for them.

 * For people who deliberately wanted to move "foo"'s refs to
   different hierarchies depending on the hierarchies at the origin
   (i.e. branches to "bar", tags to "baz"), they wanted to do so for
   a reason to group related things in "bar" (and "baz") [*2*].  For
   them, mapping with remote.$nick.fetch" means not allowing them to
   use the real name of the group (i.e. "bar") they chose to name
   their refs.

>> If the final end result you are shooting for is to introduce an
>> extra level between the remote name and the branch names, i.e.
>> "heads/", any solution needs to at least have a plan (not necessarily
>> a detailed design or implementation) for the other hierarchies.  The
>> possibility to have these other hierarchies per remote is the true
>> progress that the "heads/" at that level can give us; there is not
>> much point to have heads/ after refs/remotes/origin/, if heads/ is
>> the only thing that can come there.
>
> I fully agree. This series was meant as the first step in that direction
> (sorry for not describing my intentions more clearly).

And I do not think we mind terribly if we extend the ref_rev_parse_rules[]
used in dwim_ref() to also look at these

	refs/remotes/$nick/$name
	refs/remotes/$nick/tags/$name
	refs/remotes/$nick/heads/$name

(the first of the above is existing "refs/remotes/%.*s").  I think
it is going too far if you extend it further to

	refs/remotes/$nick/*/$name

where the code does not control what an acceptable match for '*' is
(i.e. origin/foo matching origin/changes/foo might be OK, but
matching it with origin/randomstring/foo is not, unless the canned
ref_rev_parse_rules[] knows about the "randomstring", or there is a
configuration mechanism for the user to tell us she cares about the
"randomstring" hierarchy in her project).


[Footnotes]

*1* I offhand do not remember if we even allow multi-level remote
    nicks, but I do know we support multi-level branch names, so it
    may turn out that the only valid split of origin/jh/rbranch is
    topic 'jh/rbranch' from remote 'origin' and not topic 'rbranch'
    from remote 'origin/jh'.

*2* Perhaps "bar" in the above is spelled "topics", and the
    hierarchy may be used to collect non-integration single topic
    branches from more than one remote.  An example that is more in
    line with such a usage might be:

    [remote "jh"]
        fetch = +refs/heads/*:refs/remotes/topics/heads/jh/*
    [remote "jk"]
        fetch = +refs/heads/*:refs/remotes/topics/heads/jk/*
    [remote "fc"]
        fetch = +refs/heads/*:refs/remotes/topics/heads/fc/*

    and I would expect "git merge topics/jh/rbranch" to merge the
    "refs/remotes/topics/heads/jh/rbranch" topic branch.

Re: [PATCH 0/7] Make "$remote/$branch" work with unconventional refspecs

[Johan Herland]

On Sun, May 5, 2013 at 9:02 PM, Junio C Hamano <gitster@pobox.com> wrote:
> So another issue that remains is the following, I think.
>
> When interpreting $nick/$name, assuming that we can tell where $nick
> for a remote ends and $name for the ref we take from the remote
> begins [*1*], how would we determine which refs/remotes/$remote/ is
> used for $nick?

My code currently iterates for_each_remote() until we find a remote whose
name == $nick. After all, the remote name is what the user interacts with
when doing things like "git fetch $remote" and "git push $remote ...".
Hence, I believe that the remote name is the most natural name to use for
the "$nick/$name" shorthand.

> My gut feeling is that we should ignore any "remote.$nick.fetch"
> wildcard mapping, e.g.
>
>     [remote "foo"]
>         fetch = +refs/heads/*:refs/remotes/bar/heads/*
>         fetch = +refs/tags/*:refs/remotes/baz/tags/*
>
> so that we look always in refs/remotes/$nick/ somewhere, for at
> least two reasons:
>
>  * For sane people, "bar" and "baz" in the above example are both
>    "foo", so ignoring remote.foo.fetch mapping is a no-op for them.

Agreed.

>  * For people who deliberately wanted to move "foo"'s refs to
>    different hierarchies depending on the hierarchies at the origin
>    (i.e. branches to "bar", tags to "baz"), they wanted to do so for
>    a reason to group related things in "bar" (and "baz") [*2*].  For
>    them, mapping with remote.$nick.fetch" means not allowing them to
>    use the real name of the group (i.e. "bar") they chose to name
>    their refs.

Actually, this series currently accepts _both_. Observe (using your config
from above):

  Shorthand         -> Expansion                         Matching rule
  --------------------------------------------------------------------
  foo/$branch       -> refs/remotes/bar/heads/$branch    foo's refspec
  foo/$tag          -> refs/remotes/baz/tags/$branch     foo's refspec
  bar/heads/$branch -> refs/remotes/bar/heads/$branch    refs/remotes/%.*s
  baz/tags/$tag     -> refs/remotes/baz/tags/$tag        refs/remotes/%.*s

(If you want to lose the "heads/" from the "bar/heads/$branch" shorthand
 (or lose "tags/" from "baz/tags/$tag"), use this config instead:

    [remote "foo"]
        fetch = +refs/heads/*:refs/remotes/bar/*
        fetch = +refs/tags/*:refs/remotes/baz/tags/*
)

Now, IINM you seem to mean that we shouldn't look at refspecs (or the
config file) at all, but should instead require that $nick matches a
subdirectory of "refs/remotes", and then allow for some flexibility as
to what comes between "refs/remotes/$nick" and "$name" (for example, one
of "/", "/heads/", and "/tags/").

This would not allow the user to use the relevant $remote_name for $nick,
which I argue might be the more natural name for the user to use, since
it's the same name that is used for otherwise interacting with the remote.

>>> If the final end result you are shooting for is to introduce an
>>> extra level between the remote name and the branch names, i.e.
>>> "heads/", any solution needs to at least have a plan (not necessarily
>>> a detailed design or implementation) for the other hierarchies.  The
>>> possibility to have these other hierarchies per remote is the true
>>> progress that the "heads/" at that level can give us; there is not
>>> much point to have heads/ after refs/remotes/origin/, if heads/ is
>>> the only thing that can come there.
>>
>> I fully agree. This series was meant as the first step in that direction
>> (sorry for not describing my intentions more clearly).
>
> And I do not think we mind terribly if we extend the ref_rev_parse_rules[]
> used in dwim_ref() to also look at these
>
>         refs/remotes/$nick/$name
>         refs/remotes/$nick/tags/$name
>         refs/remotes/$nick/heads/$name
>
> (the first of the above is existing "refs/remotes/%.*s").

This is not what I have in mind with this series (although the behavior
will be identical for the refspecs suggested in the remote ref namespace
discussion). Rather I would have the following three rules:

        refs/remotes/%.*s
            (which is - as you say - identical to your first rule)
        RHS from remote $nick's refspec matching LHS = refs/tags/$name
        RHS from remote $nick's refspec matching LHS = refs/heads/$name

and indeed, my series implements the last of these (in addition to
retaining the first). The middle one (for tags) would be added by a future
patch series.

The whole point of mapping through the refspecs, is that we DON'T hardcode
any preference as to where remote-tracking branches (or tags, or whatever)
should be located. It's _purely_ up to how the user has configured her
refspecs, and any expansion for remote-tracking refs should obey those
refspecs.

> I think it is going too far if you extend it further to
>
>         refs/remotes/$nick/*/$name
>
> where the code does not control what an acceptable match for '*' is
> (i.e. origin/foo matching origin/changes/foo might be OK, but
> matching it with origin/randomstring/foo is not, unless the canned
> ref_rev_parse_rules[] knows about the "randomstring", or there is a
> configuration mechanism for the user to tell us she cares about the
> "randomstring" hierarchy in her project).

Fully agreed, although this is somewhat irrelevant to my objective.
I don't want a list of "blessed" strings that will be automatically
traversed upon expansion at all (much less a configuration mechanism
to customize it). I want the _refspec_ to be the king of deciding where
remote-tracking branches and tags are

Let me try to summarize my views on how refnames should work in Git, to
see if we can identify where we differ on the principles (or if we, in
fact, differ at all):

0. refnames must obviously follow the check-ref-format syntactic rules.

1. refs should generally be placed within the refs/ hierarchy. Anything
   outside refs/ is only for Git's own special use (e.g. HEAD, FETCH_HEAD,
   etc.).

2. refs may in general be placed anywhere within refs/, but there are some
   places that are interpreted specially by Git:
     - refs/heads/* hold local branches
     - refs/tags/* hold local tags
     - refs/replace/* hold local replace refs
     - refs/notes/* hold local notes
     - (there may be more here, for other ref types, e.g. refs/original/)

3. refs that originate from a remote repository (a.k.a. remote-tracking
   refs) are typically placed within refs/remotes/*, however there are NO
   _hardcoded_ rules dictating how remote-tracking refs are to be
   organized.

4. The organization of remote-tracking refs is determined by the
   configured (fetch) refspecs, which map remote refs to their remote-
   tracking counterparts (typically - but not necessarily - located
   within refs/remotes/*).

5. Ideally, all refs within refs/remotes/* should correspond to exactly
   one refspec (i.e. be matched by the RHS side of that refspec) in one
   remote, although this is not a requirement.

6. The user should be able to use shorthand notation for local refs when
   doing so is natural and unambiguous in the current context. For example
   in a branch context, "$name" should automatically be expanded into
   "refs/heads/$name", if doing so is unambiguous. Likewise for tags,
   notes and other contexts corresponding to the local ref types mentioned
   in #2.

7. When there is no context limiting us to a specific ref type, we should
   still allow the shorthands from #6, as long as they are unambiguous.

8. For remote-tracking refs, there is no _hardcoded_ structure along which
   we can expand shorthand notations. However, since the structure is
   defined by the configured (fetch) refspecs, we can use those same
   refspecs to expand the "$nick/$name" shorthand notation into the
   remote-tracking ref from remote "$nick" that (unambiguously) matches
   "$name". (When matching "$name" against $nick's refspecs, we should use
   the same expansions as in #6 to expand "$name" into something matching
   the LHS of a refspec (e.g. "refs/heads/$name", "refs/tags/$name"), and
   then use the corresponding RHS as the resulting expansion.)

9. In addition to the refspec-sensitive expansion of shorthand notations
   described in #8, we probably also want to allow a blanket expansion of
   "$anything" into "refs/remotes/$anything", since that seems generally
   useful.

Some notes:

- Both the current default refspecs, and the refspecs suggested in the
  remote ref namespace discussion follow the above principles. Also, your
  non-integration single topic hierarchy mentioned below should work with
  these principles.

- My rationale for #3 is that hardcoding a structure within refs/remotes/*
  will likely resist future inventions for how to deal with remote refs,
  and the likely result will be to create ad hoc solutions outside the
  refs/remotes/* hierarchy, like we have already seen with e.g.
  "refs/remote-notes/*". By leaving the organizing of refs/remotes/* up to
  the refspecs (#4), we are infinitely more customizable and future-proof
  when it comes to new ways of organizing remote refs.

> [Footnotes]
>
> *1* I offhand do not remember if we even allow multi-level remote
>     nicks, but I do know we support multi-level branch names, so it
>     may turn out that the only valid split of origin/jh/rbranch is
>     topic 'jh/rbranch' from remote 'origin' and not topic 'rbranch'
>     from remote 'origin/jh'.

We do currently allow this. Observe:

$ git init clobbering_remotes
Initialized empty Git repository in ./clobbering_remotes/.git/
$ cd clobbering_remotes/
$ echo foo > foo
$ git add foo
$ git commit -m foo
[master (root-commit) 5b5db04] foo
 1 file changed, 1 insertion(+)
 create mode 100644 foo
$ git checkout -b bar/master
Switched to a new branch 'bar/master'
$ echo bar > foo
$ git commit -am bar
[bar/master 03f1d10] bar
 1 file changed, 1 insertion(+), 1 deletion(-)
$ git remote add foo .
$ git fetch foo
From .
 * [new branch]      bar/master -> foo/bar/master
 * [new branch]      master     -> foo/master
$ git rev-parse refs/remotes/foo/bar/master
03f1d10c1456aec8b42e2432cb7726e92d0dc17a
$ git remote add foo/bar .
$ git fetch foo/bar
From .
 * [new branch]      bar/master -> foo/bar/bar/master
 + 03f1d10...5b5db04 master     -> foo/bar/master  (forced update)
$ git rev-parse refs/remotes/foo/bar/master
5b5db042e0af6f10bef7a3c82ce53e5c0ac9ab8a

I would support disallowing multi-level remote names, although I don't
know if it is commonly used, and would break many existing users.

> *2* Perhaps "bar" in the above is spelled "topics", and the
>     hierarchy may be used to collect non-integration single topic
>     branches from more than one remote.  An example that is more in
>     line with such a usage might be:
>
>     [remote "jh"]
>         fetch = +refs/heads/*:refs/remotes/topics/heads/jh/*
>     [remote "jk"]
>         fetch = +refs/heads/*:refs/remotes/topics/heads/jk/*
>     [remote "fc"]
>         fetch = +refs/heads/*:refs/remotes/topics/heads/fc/*
>
>     and I would expect "git merge topics/jh/rbranch" to merge the
>     "refs/remotes/topics/heads/jh/rbranch" topic branch.

I like the use case, but not necessarily your expectation. ;-)

With the above configuration, and my series as-is, you could simply do
"git merge jh/rbranch" to merge the "refs/remotes/topics/heads/jh/rbranch"
topic branch. Furthermore, I don't see why you want/need the extra
"heads/" level in the refspec. If you do this instead:

    [remote "jh"]
        fetch = +refs/heads/*:refs/remotes/topics/jh/*
    [remote "jk"]
        fetch = +refs/heads/*:refs/remotes/topics/jk/*
    [remote "fc"]
        fetch = +refs/heads/*:refs/remotes/topics/fc/*

your "git merge topics/jh/rbranch" should work with current Git. With my
patch series on top, it would also be equivalent to "git merge jh/rbranch".


...Johan

--
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCH 0/7] Make "$remote/$branch" work with unconventional refspecs

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> This would not allow the user to use the relevant $remote_name for $nick,
> which I argue might be the more natural name for the user to use, since
> it's the same name that is used for otherwise interacting with the remote.

That is where we differ.

The thing is, when you name a local ref (be it "refs/heads/master"
or "refs/remotes/origin/master") with a short-hand, you are still
dealing with a refname, not interacting with the remote at all.

Taking notice of remote.$nick.fetch mappings only to complicate the
refname resolution logic is absolutely unacceptable, at least to
somebody who comes from the "we are interacting with refs, not with
remotes" school, like me.

Re: [PATCH 0/7] Make "$remote/$branch" work with unconventional refspecs

[Santi Béjar]

El 06/05/2013 00:36, "Junio C Hamano" <gitster@pobox.com> escribió:
>
> Johan Herland <johan@herland.net> writes:
>
> > This would not allow the user to use the relevant $remote_name for $nick,
> > which I argue might be the more natural name for the user to use, since
> > it's the same name that is used for otherwise interacting with the remote.
>
> That is where we differ.
>
> The thing is, when you name a local ref (be it "refs/heads/master"
> or "refs/remotes/origin/master") with a short-hand, you are still
> dealing with a refname, not interacting with the remote at all.
>
> Taking notice of remote.$nick.fetch mappings only to complicate the
> refname resolution logic is absolutely unacceptable, at least to
> somebody who comes from the "we are interacting with refs, not with
> remotes" school, like me.

Maybe we could mark it explicity with a double slash: "$remote//$branch",
or similar. And it even allows a slash in the remote nick: "bar/baz//foo".

See you,
Santi

P.D: Resend because the list rejected it, sorry for the duplicate.

Re: [PATCH 0/7] Make "$remote/$branch" work with unconventional refspecs

[Santi Béjar]

2013/5/6 Santi Béjar <santi@agolina.net>:
> El 06/05/2013 00:36, "Junio C Hamano" <gitster@pobox.com> escribió:
>>
>> Johan Herland <johan@herland.net> writes:
>>
>> > This would not allow the user to use the relevant $remote_name for $nick,
>> > which I argue might be the more natural name for the user to use, since
>> > it's the same name that is used for otherwise interacting with the remote.
>>
>> That is where we differ.
>>
>> The thing is, when you name a local ref (be it "refs/heads/master"
>> or "refs/remotes/origin/master") with a short-hand, you are still
>> dealing with a refname, not interacting with the remote at all.
>>
>> Taking notice of remote.$nick.fetch mappings only to complicate the
>> refname resolution logic is absolutely unacceptable, at least to
>> somebody who comes from the "we are interacting with refs, not with
>> remotes" school, like me.
>
> Maybe we could mark it explicity with a double slash: "$remote//$branch",
> or similar. And it even allows a slash in the remote nick: "bar/baz//foo".

The next question could be: why not make it work with $url too? As in:

$ git merge git://git.kernel.org/pub/scm/git/git.git//master

But I don't know if it can be problematic...

I remember that there was a discussion about the remote#branch
notation used in cogito, and at the end it was rejected.

See you,
Santi

Re: [PATCH 0/7] Make "$remote/$branch" work with unconventional refspecs

[Junio C Hamano]

Santi Béjar <santi@agolina.net> writes:

> The next question could be: why not make it work with $url too? As in:
>
> $ git merge git://git.kernel.org/pub/scm/git/git.git//master

You need to remember "merge" is a local operation, working in your
local repository.  Like it or not, the UI of Git makes distinction
between operations that are local and those that go to other
repositories (e.g. "git pull").

That of course does _not_ prevent you from writing an alternative UI
"got merge <anything>" that interprets <anthing> part and invokes
"git merge" or "git pull" as its helper (after all, Git is designed
to be scripted).

Re: [PATCH 0/7] Make "$remote/$branch" work with unconventional refspecs

[Santi Béjar]

El 06/05/2013 19:11, "Junio C Hamano" <gitster@pobox.com> va escriure:
>
> Santi Béjar <santi@agolina.net> writes:
>
> > The next question could be: why not make it work with $url too? As in:
> >
> > $ git merge git://git.kernel.org/pub/scm/git/git.git//master
>
> You need to remember "merge" is a local operation, working in your
> local repository.  Like it or not, the UI of Git makes distinction
> between operations that are local and those that go to other
> repositories (e.g. "git pull").
>

Of course! In fact I wanted to say "git pull".

Santi

Re: [PATCH 0/7] Make "$remote/$branch" work with unconventional refspecs

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> Let me try to summarize my views on how refnames should work in Git, to
> see if we can identify where we differ on the principles (or if we, in
> fact, differ at all):

Thanks; I think I already said where I think we differ in a separate
message, but a short version is that the point of remote.$nick.fetch
mapping is to solve "The remote may call a ref $this, which is not
the refname I want to or can use in my repository, so here is the
rule to use when importing it in my local namespace.  With the
mapping, I can name the ref in my local namespace conveniently."
E.g. their "refs/heads/master" cannot be our "refs/heads/master" at
the same time, so use "refs/heads/origin/master".

The result of the above mapping, be it remotes/origin/master or
remotes/origin/heads/master, should be designed to be useful for the
local use of the ref in question.  If you further need to remap it
when using it locally, there is something wrong in the mapping you
defined in your remote.$nick.fetch mapping in the first place.

We do not force any structure under refs/remotes/; it is left
entirely up to the user, even though we would like to suggest the
best current practice by teaching "clone" and "remote add" to lay
them out in a certain way.

Another thing is that refs/remotes/ is not special at all.  If notes
hierarchies taken from a remote need to be somewhere other than
refs/notes/, it is perfectly fine to introduce refs/remote-notes/ if
that is the best layout when using them locally.  What is special is
refs/heads/ in that they are the _only_ refs you can check out to
the working tree and directly advance them by working on the working
tree files.

> I would support disallowing multi-level remote names, although I don't
> know if it is commonly used, and would break many existing users.

I somewhat doubt it.

We very much anticipated the use of multi-level branch names from
the very beginning and have support (e.g. in "for-each-ref" and
"branch --list") to group/filter them according to prefixes, but I
do not think there is anywhere we consciously try to give support
for multi-level remote names to treat groups of remotes that share
the same prefix.

>> *2* Perhaps "bar" in the above is spelled "topics", and the
>>     hierarchy may be used to collect non-integration single topic
>>     branches from more than one remote.  An example that is more in
>>     line with such a usage might be:
>>
>>     [remote "jh"]
>>         fetch = +refs/heads/*:refs/remotes/topics/heads/jh/*
>>     [remote "jk"]
>>         fetch = +refs/heads/*:refs/remotes/topics/heads/jk/*
>>     [remote "fc"]
>>         fetch = +refs/heads/*:refs/remotes/topics/heads/fc/*
>>
>>     and I would expect "git merge topics/jh/rbranch" to merge the
>>     "refs/remotes/topics/heads/jh/rbranch" topic branch.
>
> I like the use case, but not necessarily your expectation. ;-)
>
> With the above configuration, and my series as-is, you could simply do
> "git merge jh/rbranch" to merge the "refs/remotes/topics/heads/jh/rbranch"
> topic branch.

That dropping of 'topics/' is the issue.  The user wanted to group
them under 'topics/' hierarchy and made a conscous effort to set up
the fetch refspec to map these refs there.  These are done all for
convenience when she deals with refs in her namespace in the
repository.  What justification do we have to second guess the user
and force her to drop it when naming these refs?

> Furthermore, I don't see why you want/need the extra
> "heads/" level in the refspec.

Just like you wanted to have separate kinds of refs under a single
remote, the layout is grouping kinds of refs other than branch heads
related to the "topics" (as opposed to "integration branches").

Re: [PATCH 0/7] Make "$remote/$branch" work with unconventional refspecs

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

Just a few typofixes...

> Johan Herland <johan@herland.net> writes:
>
>> Let me try to summarize my views on how refnames should work in Git, to
>> see if we can identify where we differ on the principles (or if we, in
>> fact, differ at all):
>
> Thanks; I think I already said where I think we differ in a separate
> message, but a short version is that the point of remote.$nick.fetch
> mapping is to solve "The remote may call a ref $this, which is not
> the refname I want to or can use in my repository, so here is the
> rule to use when importing it in my local namespace.  With the
> mapping, I can name the ref in my local namespace conveniently."
> E.g. their "refs/heads/master" cannot be our "refs/heads/master" at
> the same time, so use "refs/heads/origin/master".

The last should be "refs/remotes/origin/master".

>
> The result of the above mapping, be it remotes/origin/master or
> remotes/origin/heads/master, should be designed to be useful for the
> local use of the ref in question.  If you further need to remap it
> when using it locally, there is something wrong in the mapping you
> defined in your remote.$nick.fetch mapping in the first place.
>
> We do not force any structure under refs/remotes/; it is left
> entirely up to the user, even though we would like to suggest the
> best current practice by teaching "clone" and "remote add" to lay
> them out in a certain way.
>
> Another thing is that refs/remotes/ is not special at all.  If notes
> hierarchies taken from a remote need to be somewhere other than
> refs/notes/, it is perfectly fine to introduce refs/remote-notes/ if
> that is the best layout when using them locally.  What is special is

s/the best/the most convenient/;

> refs/heads/ in that they are the _only_ refs you can check out to
> the working tree and directly advance them by working on the working
> tree files.

Re: [PATCH 0/7] Make "$remote/$branch" work with unconventional refspecs

[Johan Herland]

On Mon, May 6, 2013 at 7:06 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Johan Herland <johan@herland.net> writes:
>> Let me try to summarize my views on how refnames should work in Git, to
>> see if we can identify where we differ on the principles (or if we, in
>> fact, differ at all):
>
> Thanks; I think I already said where I think we differ in a separate
> message, but a short version is that the point of remote.$nick.fetch
> mapping is to solve "The remote may call a ref $this, which is not
> the refname I want to or can use in my repository, so here is the
> rule to use when importing it in my local namespace.  With the
> mapping, I can name the ref in my local namespace conveniently."
> E.g. their "refs/heads/master" cannot be our "refs/heads/master" at
> the same time, so use "refs/remotes/origin/master".
>
> The result of the above mapping, be it remotes/origin/master or
> remotes/origin/heads/master, should be designed to be useful for the
> local use of the ref in question.  If you further need to remap it
> when using it locally, there is something wrong in the mapping you
> defined in your remote.$nick.fetch mapping in the first place.

Ok, so whereas I consider the refspec to be "king", and that the expansion
from convenient shorthands to full remote-tracking refnames should be
derived from the chosen refspec, you would (if I understand you correctly)
rather have a constant (i.e. independent of remotes and refspecs) set of
rules for expanding shorthands to full refnames, and if the user chooses
refspecs that don't mesh well with those rules, then that is the user's
problem, and not Git's.

> We do not force any structure under refs/remotes/; it is left
> entirely up to the user, even though we would like to suggest the
> best current practice by teaching "clone" and "remote add" to lay
> them out in a certain way.

If we were to suggest +refs/heads/*:refs/remotes/origin/heads/* as the
best practice, I assume you do want "origin/master" to keep working. And
since you do not want to use the configured refspec when expanding
"origin/master" into "refs/remotes/origin/heads/master", then I assume
you would rather add a hardcoded (what I call a "textual expansion" in
my patches) rule that would map "$nick/$name" into

  /refs/remotes/$nick/heads/$name

But isn't the existence of such a rule evidence of us trying to impose
(or at least hint) at a certain structure for refs/remotes/*?

In light of this, I'm interested in your thoughts about the following
related problem that I've just started looking at:

git branch -r shows the remote-tracking branches in this repo. Currently,
AFAICS, this just spits out all refs under refs/remotes/*. This behavior
must clearly be modified if we are to allow remote-tracking tags at
refs/remotes/$remote/tags/* (they currently show up in "git branch -r",
but shouldn't). One could say that the filter should merely change from
refs/remotes/* to refs/remotes/*/heads/*, but this would break for
existing (old-style) remotes. Should we add a heuristic for detecting when
to use refs/remotes/* vs. refs/remotes/*/heads/* as a filter?

My approach would be to iterate through the configured remotes, and for
each remote list all refs that match the RHS of the refspec whose LHS is
refs/heads/*. This would work for both old- and new-style remotes with
no heuristics.

If you agree that my approach is correct for enumerating remote-tracking
branches, then what is different about using the refspec when expanding
remote-tracking refs in general?

In other words, given the following configuration:

  [remote "origin"]
          +refs/heads/*:refs/foo/bar/baz/*
  [remote "foo"]
          +refs/heads/*:refs/remotes/origin/heads/*

1. In your opininon, is refs/foo/bar/baz/master a remote-tracking branch?

2. Should refs/foo/bar/baz/master be listed by "git branch -r"?

3. Should the "origin/master" shorthand notation expand to
   refs/remotes/origin/heads/master from remote foo, or
   refs/foo/bar/baz/master from remote origin?


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: [PATCH 0/7] Make "$remote/$branch" work with unconventional refspecs

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> Ok, so whereas I consider the refspec to be "king", and that the expansion
> from convenient shorthands to full remote-tracking refnames should be
> derived from the chosen refspec, you would (if I understand you correctly)
> rather have a constant (i.e. independent of remotes and refspecs) set of
> rules for expanding shorthands to full refnames, and if the user chooses
> refspecs that don't mesh well with those rules, then that is the user's
> problem, and not Git's.

You need to dig your rhetoric from the other end of the tunnel.  I
would consider the local namespace for the refs to be the "king",
and we would design how they are resolved to be useful for user's
local usage pattern.  How the remote refs are copied by fetch
following refspecs is one of the many components (e.g. the dwimming
machinery "checkout foo" uses to guess that the user may want to
fork from and integrate later with one of the refs under refs/remotes
is one of them and it is not "fetch") to complement the refname
resolution rule to support the local usage of refs.

> In light of this, I'm interested in your thoughts about the following
> related problem that I've just started looking at:
>
> git branch -r shows the remote-tracking branches in this repo. Currently,
> .... Should we add a heuristic for detecting when
> to use refs/remotes/* vs. refs/remotes/*/heads/* as a filter?

Didn't I already said that I do not think repurposing refs/remotes/
for these "unified" copies is the best approach?

A change that I think is a good thing to add on top of your [45]/7
refactoring is to allow the user to add custom expansion/contraction
rules.  Then the user can group refs regardless of "remotes" and
give meaningful shortening.

As I said in a very early review, viewing "fetch refspec" to be
"king" and refspecs are the only way the user may want to group
things locally is myopic.  The every-day usage of the local names
ought to be the king, and everything else should serve to make it
easier to use.