[PATCH] for-each-ref: `:short` format for `refname`

[PATCH] for-each-ref: `:short` format for `refname`

[Bert Wesarg]

This strips from the refname the common directory prefix with the
matched pattern.

This is particular usefull for bash completion, to get refs without
`refs/heads` or `refs/tags`.

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

---
 Documentation/git-for-each-ref.txt |    5 ++
 builtin-for-each-ref.c             |   74 +++++++++++++++++++++++++++++++----
 t/t6300-for-each-ref.sh            |   61 +++++++++++++++++++++++++++++
 3 files changed, 131 insertions(+), 9 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index eae6c0e..deeae79 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -74,6 +74,11 @@ For all objects, the following names can be used:
 
 refname::
 	The name of the ref (the part after $GIT_DIR/).
+	For a short name of the ref append `:short`. This will strip
+	the common directory prefix with the pattern which matches this ref.
+	I.e. for a the pattern `refs/heads` you get `master`, or for
+	`refs/tags/v1.5.[01].*` you get `v1.5.[01].*`.
+	This is particular usefull for bash completion.
 
 objecttype::
 	The type of the object (`blob`, `tree`, `commit`, `tag`).
diff --git a/builtin-for-each-ref.c b/builtin-for-each-ref.c
index 21e92bb..946f79b 100644
--- a/builtin-for-each-ref.c
+++ b/builtin-for-each-ref.c
@@ -31,6 +31,7 @@ struct ref_sort {
 
 struct refinfo {
 	char *refname;
+	const char *pattern; /* the pattern which matched this ref */
 	unsigned char objectname[20];
 	struct atom_value *value;
 };
@@ -546,6 +547,40 @@ static void grab_values(struct atom_value *val, int deref, struct object *obj, v
 }
 
 /*
+ * Use the matched pattern from ref to shorten the refname
+ */
+static char *get_short_ref(struct refinfo *ref)
+{
+	int rlen, plen, common = 0;
+
+	if (!ref->pattern)
+		return ref->refname;
+
+	rlen = strlen(ref->refname);
+	plen = strlen(ref->pattern);
+
+	if ((plen <= rlen) &&
+	    !strncmp(ref->refname, ref->pattern, plen) &&
+	    (ref->refname[plen] == '\0' ||
+	     ref->refname[plen] == '/' ||
+	     ref->pattern[plen - 1] == '/')) {
+		common = plen + (ref->refname[plen] == '/');
+		/* prevent stripping the whole refname */
+		if (common == rlen)
+			common = 0;
+	} else {
+		/* find the first wildcard and go back to the previous '/' */
+		common = strcspn(ref->pattern, "*?[");
+		while (common >= 0 && ref->pattern[common] != '/')
+			--common;
+		common++;
+	}
+
+	return ref->refname + common;
+}
+
+
+/*
  * Parse the object referred by ref, and grab needed value.
  */
 static void populate_value(struct refinfo *ref)
@@ -570,13 +605,33 @@ static void populate_value(struct refinfo *ref)
 	for (i = 0; i < used_atom_cnt; i++) {
 		const char *name = used_atom[i];
 		struct atom_value *v = &ref->value[i];
-		if (!strcmp(name, "refname"))
-			v->s = ref->refname;
-		else if (!strcmp(name, "*refname")) {
-			int len = strlen(ref->refname);
-			char *s = xmalloc(len + 4);
-			sprintf(s, "%s^{}", ref->refname);
-			v->s = s;
+		int deref = 0;
+		if (*name == '*') {
+			deref = 1;
+			name++;
+		}
+		if (!prefixcmp(name, "refname")) {
+			const char *formatp = strchr(name, ':');
+			const char *refname = ref->refname;
+
+			/* look for "short" refname format */
+			if (formatp) {
+				formatp++;
+				if (!strcmp(formatp, "short"))
+					refname = get_short_ref(ref);
+				else
+					die("unknown refname format %s",
+					    formatp);
+			}
+
+			if (!deref)
+				v->s = refname;
+			else {
+				int len = strlen(refname);
+				char *s = xmalloc(len + 4);
+				sprintf(s, "%s^{}", refname);
+				v->s = s;
+			}
 		}
 	}
 
@@ -641,9 +696,9 @@ static int grab_single_ref(const char *refname, const unsigned char *sha1, int f
 	struct grab_ref_cbdata *cb = cb_data;
 	struct refinfo *ref;
 	int cnt;
+	const char **pattern = cb->grab_pattern;
 
-	if (*cb->grab_pattern) {
-		const char **pattern;
+	if (*pattern) {
 		int namelen = strlen(refname);
 		for (pattern = cb->grab_pattern; *pattern; pattern++) {
 			const char *p = *pattern;
@@ -668,6 +723,7 @@ static int grab_single_ref(const char *refname, const unsigned char *sha1, int f
 	 */
 	ref = xcalloc(1, sizeof(*ref));
 	ref->refname = xstrdup(refname);
+	ref->pattern = *pattern;
 	hashcpy(ref->objectname, sha1);
 
 	cnt = cb->grab_cnt;
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 8ced593..a4a2fd3 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -262,6 +262,67 @@ for i in "--perl --shell" "-s --python" "--python --tcl" "--tcl --perl"; do
 	"
 done
 
+cat >expected <<\EOF
+master
+testtag
+master
+testtag
+EOF
+
+test_expect_success 'Check short refname format' '
+	(git for-each-ref --format="%(refname:short)" refs/heads &&
+	git for-each-ref --format="%(refname:short)" refs/tags &&
+	git for-each-ref --format="%(refname:short)" refs/heads/ &&
+	git for-each-ref --format="%(refname:short)" refs/tags/) >actual &&
+	test_cmp expected actual
+'
+
+cat >expected <<\EOF
+master
+testtag
+EOF
+
+test_expect_success 'Check short refname format with multiple patterns' '
+	(git for-each-ref --format="%(refname:short)" refs/heads refs/tags) >actual &&
+	test_cmp expected actual
+'
+
+cat >expected <<\EOF
+refs/heads/master
+refs/tags/testtag
+EOF
+
+test_expect_success 'Check short refname format without patterns' '
+	git for-each-ref --format="%(refname:short)" >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'Check for invalid refname format' '
+	test_must_fail git for-each-ref --format="%(refname:INVALID)"
+'
+
+cat >expected <<\EOF
+heads/master
+master
+testtag
+EOF
+
+test_expect_success 'Check short refname format with wildcard pattern' '
+	(git for-each-ref --format="%(refname:short)" refs/*/m* &&
+	git for-each-ref --format="%(refname:short)" refs/heads/?aster &&
+	git for-each-ref --format="%(refname:short)" refs/tags/t[aeiou]sttag) >actual &&
+	test_cmp expected actual
+'
+
+cat >expected <<\EOF
+refs/heads/master
+EOF
+
+test_expect_success 'Check disabled short refname format with exact pattern' '
+	(git for-each-ref --format="%(refname:short)" refs/heads/master) >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'an unusual tag with an incomplete line' '
 
 	git tag -m "bogo" bogo &&
-- 
tg: (445cac1..) t/for-each-ref-refshort (depends on: master)

Re: [PATCH] for-each-ref: `:short` format for `refname`

[SZEDER Gábor]

Hi,

On Sun, Aug 31, 2008 at 02:41:07PM +0200, Bert Wesarg wrote:
> This strips from the refname the common directory prefix with the
> matched pattern.
> 
> This is particular usefull for bash completion, to get refs without
> `refs/heads` or `refs/tags`.

>  refname::
>  	The name of the ref (the part after $GIT_DIR/).
> +	For a short name of the ref append `:short`. This will strip
> +	the common directory prefix with the pattern which matches this ref.
> +	I.e. for a the pattern `refs/heads` you get `master`, or for
> +	`refs/tags/v1.5.[01].*` you get `v1.5.[01].*`.
> +	This is particular usefull for bash completion.
Should this last sentence really belong to the documentation?

Furthermore, I think ':strip' better describes what this format
actually does.  Even you have used the word 'strip' in the commit
message and in the documentation as well.


As far as bash completion is concerned, I'm for it, as it does exactly
what the completion script needs to perform better, it doesn't have
those conceptual issues 'refbasename' has, and it's only a tad slower
than 'refbasename'.

However, if we consider possible use cases other than bash completion,
I don't know which one is more useful.  For example, if you have two
branches 'foo/bar' and 'foo/baz', then 'git merge $(git for-each-ref
--format=%(refbasename) refs/heads/foo)' will work as expected, but
'refname:short' not, as it will output only 'bar' and 'baz' which 'git
merge' can not find.


Best,
Gábor

Re: [PATCH] for-each-ref: `:short` format for `refname`

[Bert Wesarg]

On Mon, Sep 1, 2008 at 15:15, SZEDER Gábor <szeder@ira.uka.de> wrote:
> Hi,
>
> On Sun, Aug 31, 2008 at 02:41:07PM +0200, Bert Wesarg wrote:
>> This strips from the refname the common directory prefix with the
>> matched pattern.
>>
>> This is particular usefull for bash completion, to get refs without
>> `refs/heads` or `refs/tags`.
>
>>  refname::
>>       The name of the ref (the part after $GIT_DIR/).
>> +     For a short name of the ref append `:short`. This will strip
>> +     the common directory prefix with the pattern which matches this ref.
>> +     I.e. for a the pattern `refs/heads` you get `master`, or for
>> +     `refs/tags/v1.5.[01].*` you get `v1.5.[01].*`.
>> +     This is particular usefull for bash completion.
> Should this last sentence really belong to the documentation?
At least it is not the only example in the documentation.

>
> Furthermore, I think ':strip' better describes what this format
> actually does.  Even you have used the word 'strip' in the commit
> message and in the documentation as well.
True, I'm ok with this proposal.

>
>
> As far as bash completion is concerned, I'm for it, as it does exactly
> what the completion script needs to perform better, it doesn't have
> those conceptual issues 'refbasename' has, and it's only a tad slower
> than 'refbasename'.
>
> However, if we consider possible use cases other than bash completion,
> I don't know which one is more useful.  For example, if you have two
> branches 'foo/bar' and 'foo/baz', then 'git merge $(git for-each-ref
> --format=%(refbasename) refs/heads/foo)' will work as expected, but
> 'refname:short' not, as it will output only 'bar' and 'baz' which 'git
> merge' can not find.
Yeah, thats an disadvantage and I thought about this, too. But I have
no particular opinion about it.

Regards,
Bert

> Best,
> Gábor

Re: [PATCH] for-each-ref: `:short` format for `refname`

[Bert Wesarg]

On Mon, Sep 1, 2008 at 16:13, Bert Wesarg <bert.wesarg@googlemail.com> wrote:
> On Mon, Sep 1, 2008 at 15:15, SZEDER Gábor <szeder@ira.uka.de> wrote:
>> However, if we consider possible use cases other than bash completion,
>> I don't know which one is more useful.  For example, if you have two
>> branches 'foo/bar' and 'foo/baz', then 'git merge $(git for-each-ref
>> --format=%(refbasename) refs/heads/foo)' will work as expected, but
>> 'refname:short' not, as it will output only 'bar' and 'baz' which 'git
>> merge' can not find.
> Yeah, thats an disadvantage and I thought about this, too. But I have
> no particular opinion about it.
Ok, I have a new idea, which could be made all happy:

IMHO the goal of this new format for refname should be, that it can be
used as an ref on the command line. This isn't given with my current
:short proposal (which I call :strip as of now), as Gábor showed. What
we need is the reverse of what happened with refnames given on the
command line to commands like checkout/merge/... The only thing that
comes near to this is this from refs.c:

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

Which doesn't look very useful, because every ref from for_each_ref
would match rule one. So we probably need to try the reverse of this
list. Now my knowledge from git internals is really low, I don't know
if this is sane. I know that this can't be bijective but at least the
bash completion would be happy with this idea.

Comments, thoughts, brown paper bags...

Thanks,
Bert

Re: [PATCH] for-each-ref: `:short` format for `refname`

[Shawn O. Pearce]

Bert Wesarg <bert.wesarg@googlemail.com> wrote:
> IMHO the goal of this new format for refname should be, that it can be
> used as an ref on the command line. This isn't given with my current
> :short proposal (which I call :strip as of now), as Gábor showed. What
> we need is the reverse of what happened with refnames given on the
> command line to commands like checkout/merge/... The only thing that
> comes near to this is this from refs.c:
> 
>     const char *ref_rev_parse_rules[] = {
>             "%.*s",
>             "refs/%.*s",
>             "refs/tags/%.*s",
>             "refs/heads/%.*s",
>             "refs/remotes/%.*s",
>             "refs/remotes/%.*s/HEAD",
>             NULL
>     };
> 
> Which doesn't look very useful, because every ref from for_each_ref
> would match rule one. So we probably need to try the reverse of this
> list.

Yup.  If you search the list backwards and extract the part of the
ref that matches %.*s you'll get a name that other tools can find,
and which is the shortest name possible.

You can still get ambiguous names.  Avoiding them requires going
through all refs and building their short forms, then using the
full ref name for any ref which had more than one name shorten to
the same string.  Ugly, but implementable, and probably something
that should be considered.

-- 
Shawn.

Re: [PATCH] for-each-ref: `:short` format for `refname`

[Bert Wesarg]

On Mon, Sep 1, 2008 at 21:10, Shawn O. Pearce <spearce@spearce.org> wrote:
> Bert Wesarg <bert.wesarg@googlemail.com> wrote:
>> IMHO the goal of this new format for refname should be, that it can be
>> used as an ref on the command line. This isn't given with my current
>> :short proposal (which I call :strip as of now), as Gábor showed. What
>> we need is the reverse of what happened with refnames given on the
>> command line to commands like checkout/merge/... The only thing that
>> comes near to this is this from refs.c:
>>
>>     const char *ref_rev_parse_rules[] = {
>>             "%.*s",
>>             "refs/%.*s",
>>             "refs/tags/%.*s",
>>             "refs/heads/%.*s",
>>             "refs/remotes/%.*s",
>>             "refs/remotes/%.*s/HEAD",
>>             NULL
>>     };
>>
>> Which doesn't look very useful, because every ref from for_each_ref
>> would match rule one. So we probably need to try the reverse of this
>> list.
>
> Yup.  If you search the list backwards and extract the part of the
> ref that matches %.*s you'll get a name that other tools can find,
> and which is the shortest name possible.
>
> You can still get ambiguous names.  Avoiding them requires going
> through all refs and building their short forms, then using the
> full ref name for any ref which had more than one name shorten to
> the same string.  Ugly, but implementable, and probably something
> that should be considered.
What about: try the list backwards until the first match, than try the
matched part (this what %.*s matched) with the forward list, if both
give the same pattern, its not disambiguous. If not try the next
pattern backwards.

Its quadratic in the number of patterns, but this is maybe smaller
than scanning all refs (which may include a sort phase).

Bert
> --
> Shawn.
>

Re: [PATCH] for-each-ref: `:short` format for `refname`

[Junio C Hamano]

"Bert Wesarg" <bert.wesarg@googlemail.com> writes:

> On Mon, Sep 1, 2008 at 21:10, Shawn O. Pearce <spearce@spearce.org> wrote:
> ...
>> You can still get ambiguous names.  Avoiding them requires going
>> through all refs and building their short forms, then using the
>> full ref name for any ref which had more than one name shorten to
>> the same string.  Ugly, but implementable, and probably something
>> that should be considered.
>
> What about: try the list backwards until the first match, than try the
> matched part (this what %.*s matched) with the forward list, if both
> give the same pattern, its not disambiguous. If not try the next
> pattern backwards.

How does it catch the case where you have both 'xyzzy' branch and 'xyzzy'
tag, which is the point of disambiguation issue Shawn raised?

Re: [PATCH] for-each-ref: `:short` format for `refname`

[Bert Wesarg]

On Mon, Sep 1, 2008 at 23:28, Junio C Hamano <gitster@pobox.com> wrote:
> "Bert Wesarg" <bert.wesarg@googlemail.com> writes:
>
>> On Mon, Sep 1, 2008 at 21:10, Shawn O. Pearce <spearce@spearce.org> wrote:
>> ...
>>> You can still get ambiguous names.  Avoiding them requires going
>>> through all refs and building their short forms, then using the
>>> full ref name for any ref which had more than one name shorten to
>>> the same string.  Ugly, but implementable, and probably something
>>> that should be considered.
>>
>> What about: try the list backwards until the first match, than try the
>> matched part (this what %.*s matched) with the forward list, if both
>> give the same pattern, its not disambiguous. If not try the next
>> pattern backwards.
>
> How does it catch the case where you have both 'xyzzy' branch and 'xyzzy'
> tag, which is the point of disambiguation issue Shawn raised?
Right.

Re: [PATCH] for-each-ref: `:short` format for `refname`

[Bert Wesarg]

On Mon, Sep 1, 2008 at 23:44, Bert Wesarg <bert.wesarg@googlemail.com> wrote:
> On Mon, Sep 1, 2008 at 23:28, Junio C Hamano <gitster@pobox.com> wrote:
>> "Bert Wesarg" <bert.wesarg@googlemail.com> writes:
>>
>>> On Mon, Sep 1, 2008 at 21:10, Shawn O. Pearce <spearce@spearce.org> wrote:
>>> ...
>>>> You can still get ambiguous names.  Avoiding them requires going
>>>> through all refs and building their short forms, then using the
>>>> full ref name for any ref which had more than one name shorten to
>>>> the same string.  Ugly, but implementable, and probably something
>>>> that should be considered.
>>>
>>> What about: try the list backwards until the first match, than try the
>>> matched part (this what %.*s matched) with the forward list, if both
>>> give the same pattern, its not disambiguous. If not try the next
>>> pattern backwards.
>>
>> How does it catch the case where you have both 'xyzzy' branch and 'xyzzy'
>> tag, which is the point of disambiguation issue Shawn raised?
> Right.
I was wrong:

given these two refs:

  refs/heads/xyzzy
  refs/tags/xyzzy

first try to shorten "refs/heads/xyzzy":

  first (from the end) matched pattern is "refs/heads/%.*s" with
"xyzzy" as result

  but resolved ref for "xyzzy" is "refs/tags/xyzzy" => continue

  next matched pattern is "%.*s" with "refs/heads/xyzzy" as result

  end result is therefore: "refs/heads/xyzzy"

second try to shorten "refs/tags/xyzzy":

  first (from the end) matched pattern is "refs/tags/%.*s" with
"xyzzy" as result

  resolved ref for "xyzzy" is "refs/tags/xyzzy" => end

  end result is therefore: "xyzzy"

the output would be:

  refs/heads/xyzzy
  xyzzy

The question is now, if this is usable for bash completion? Current
bash completion would handle this case wrong, because you get two
xyzzy.

Bert

Re: [PATCH] for-each-ref: `:short` format for `refname`

[Shawn O. Pearce]

Bert Wesarg <bert.wesarg@googlemail.com> wrote:
> 
> given these two refs:
> 
>   refs/heads/xyzzy
>   refs/tags/xyzzy
> 
> first try to shorten "refs/heads/xyzzy":
> 
>   first (from the end) matched pattern is "refs/heads/%.*s" with
> "xyzzy" as result
> 
>   but resolved ref for "xyzzy" is "refs/tags/xyzzy" => continue
> 
>   next matched pattern is "%.*s" with "refs/heads/xyzzy" as result
> 
>   end result is therefore: "refs/heads/xyzzy"
> 
> second try to shorten "refs/tags/xyzzy":
> 
>   first (from the end) matched pattern is "refs/tags/%.*s" with
> "xyzzy" as result
> 
>   resolved ref for "xyzzy" is "refs/tags/xyzzy" => end
> 
>   end result is therefore: "xyzzy"
> 
> the output would be:
> 
>   refs/heads/xyzzy
>   xyzzy
> 
> The question is now, if this is usable for bash completion? Current
> bash completion would handle this case wrong, because you get two
> xyzzy.

I think this is reasonable.  Its better than what we have today,
which is ambiguous completion.  So this looks reasoanble to me.
Usually people don't have ambiguous names, but it happens.  I've
been known to do something stupid like this:

  git checkout -b v1.0 v1.0
  git reset --hard v1.2
  git log v1.0
  # wtf?!?!!?!

;-)

-- 
Shawn.

[PATCH v2] for-each-ref: `:short` format for `refname`

[Bert Wesarg]

Tries to shorten the refname to a non-ambiguous name.
I.e. the full and the short refname points to the same object.

Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
Cc: git@vger.kernel.org
Cc: szeder@ira.uka.de
Cc: Shawn O. Pearce <spearce@spearce.org>
---
 Documentation/git-for-each-ref.txt |    2 +
 builtin-for-each-ref.c             |  133 ++++++++++++++++++++++++++++++++++--
 t/t6300-for-each-ref.sh            |   31 ++++++++
 3 files changed, 159 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index eae6c0e..89158d9 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -74,6 +74,8 @@ For all objects, the following names can be used:
 
 refname::
 	The name of the ref (the part after $GIT_DIR/).
+	For a non-ambiguous short name of the ref append `:short`.
+	I.e. both the full and short name will resolve to the same object.
 
 objecttype::
 	The type of the object (`blob`, `tree`, `commit`, `tag`).
diff --git a/builtin-for-each-ref.c b/builtin-for-each-ref.c
index 21e92bb..6359ad7 100644
--- a/builtin-for-each-ref.c
+++ b/builtin-for-each-ref.c
@@ -546,6 +546,105 @@ static void grab_values(struct atom_value *val, int deref, struct object *obj, v
 }
 
 /*
+ * generate a format suitable for scanf from a ref_rev_parse_rules
+ * rule, that is replace the "%.*s" spec with a "%s" spec
+ */
+static void gen_scanf_fmt(char *scanf_fmt, const char *rule)
+{
+	char *spec;
+
+	spec = strstr(rule, "%.*s");
+	if (!spec || strstr(spec + 4, "%.*s"))
+		die("invalid rule in ref_rev_parse_rules: %s", rule);
+
+	/* copy all until spec */
+	strncpy(scanf_fmt, rule, spec - rule);
+	scanf_fmt[spec - rule] = '\0';
+	/* copy new spec */
+	strcat(scanf_fmt, "%s");
+	/* copy remaining rule */
+	strcat(scanf_fmt, spec + 4);
+
+	return;
+}
+
+/*
+ * Shorten the refname to an non-ambiguous form
+ */
+static char *get_short_ref(struct refinfo *ref)
+{
+	int i;
+	static char **scanf_fmts;
+	static int nr_rules;
+	char *short_name;
+	unsigned char fullref_sha1[20];
+
+	/* 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 ref->refname;
+
+	read_ref(ref->refname, fullref_sha1);
+
+	/* buffer for scanf result, at most ref->refname must fit */
+	short_name = strdup(ref->refname);
+
+	/* skip first rule, will always match */
+	for (i = 0; i < nr_rules - 1; i++) {
+		const char **p;
+		int short_name_len;
+
+		if (1 != sscanf(ref->refname, scanf_fmts[nr_rules - 1 - i],
+				short_name))
+			continue;
+
+		short_name_len = strlen(short_name);
+
+		/* check if full and short point to the same object
+		 * by checking all rules in forward direction
+		 */
+		for (p = ref_rev_parse_rules; *p; p++) {
+			unsigned char short_sha1[20];
+
+			/* check for valid ref */
+			if (read_ref(mkpath(*p, short_name_len, short_name),
+				     short_sha1))
+				continue;
+
+			/* if the objects differ the short name is ambiguous */
+			if (hashcmp(fullref_sha1, short_sha1))
+				break;
+
+			/* ok, short and full ref point to the same object */
+			return short_name;
+		}
+	}
+
+	free(short_name);
+	return ref->refname;
+}
+
+
+/*
  * Parse the object referred by ref, and grab needed value.
  */
 static void populate_value(struct refinfo *ref)
@@ -570,13 +669,33 @@ static void populate_value(struct refinfo *ref)
 	for (i = 0; i < used_atom_cnt; i++) {
 		const char *name = used_atom[i];
 		struct atom_value *v = &ref->value[i];
-		if (!strcmp(name, "refname"))
-			v->s = ref->refname;
-		else if (!strcmp(name, "*refname")) {
-			int len = strlen(ref->refname);
-			char *s = xmalloc(len + 4);
-			sprintf(s, "%s^{}", ref->refname);
-			v->s = s;
+		int deref = 0;
+		if (*name == '*') {
+			deref = 1;
+			name++;
+		}
+		if (!prefixcmp(name, "refname")) {
+			const char *formatp = strchr(name, ':');
+			const char *refname = ref->refname;
+
+			/* look for "short" refname format */
+			if (formatp) {
+				formatp++;
+				if (!strcmp(formatp, "short"))
+					refname = get_short_ref(ref);
+				else
+					die("unknown refname format %s",
+					    formatp);
+			}
+
+			if (!deref)
+				v->s = refname;
+			else {
+				int len = strlen(refname);
+				char *s = xmalloc(len + 4);
+				sprintf(s, "%s^{}", refname);
+				v->s = s;
+			}
 		}
 	}
 
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 8ced593..ad8d48e 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -262,6 +262,37 @@ for i in "--perl --shell" "-s --python" "--python --tcl" "--tcl --perl"; do
 	"
 done
 
+cat >expected <<\EOF
+master
+testtag
+EOF
+
+test_expect_success 'Check short refname format' '
+	(git for-each-ref --format="%(refname:short)" refs/heads &&
+	git for-each-ref --format="%(refname:short)" refs/tags) >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'Check for invalid refname format' '
+	test_must_fail git for-each-ref --format="%(refname:INVALID)"
+'
+
+cat >expected <<\EOF
+heads/master
+master
+EOF
+
+test_expect_success 'Check ambiguous head and tag refs' '
+	git checkout -b newtag &&
+	echo "Using $datestamp" > one &&
+	git add one &&
+	git commit -m "Branch" &&
+	setdate_and_increment &&
+	git tag -m "Tagging at $datestamp" master &&
+	git for-each-ref --format "%(refname:short)" refs/heads/master refs/tags/master >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'an unusual tag with an incomplete line' '
 
 	git tag -m "bogo" bogo &&
-- 
1.6.0

Re: [PATCH v2] for-each-ref: `:short` format for `refname`

[Junio C Hamano]

Bert Wesarg <bert.wesarg@googlemail.com> writes:

> Tries to shorten the refname to a non-ambiguous name.
> I.e. the full and the short refname points to the same object.

The definition of "ambiguity" here is wrong (see below).

> +	/* skip first rule, will always match */
> +	for (i = 0; i < nr_rules - 1; i++) {
> +		const char **p;
> +		int short_name_len;
> +
> +		if (1 != sscanf(ref->refname, scanf_fmts[nr_rules - 1 - i],
> +				short_name))
> +			continue;
> +
> +		short_name_len = strlen(short_name);
> +
> +		/* check if full and short point to the same object

Style?

> +		 * by checking all rules in forward direction
> +		 */

I think this part of the code is wrong, in that it talks about what object
the ref points at.  That is not what ref ambiguity is about.

Given a tag that points at a version 1.0.0 commit, this sequence will
create:

	$ git tag foo v1.0.0^0
        $ git branch foo v1.0.0^0

ambiguous branch and tag whose names are both 'foo', even though they
point at the same thing.  The right API to use would be resolve_ref(), I
think.

Other than that, it is well done.

Although I was initially a bit surprised by the size of the patch to
implement something so (conceptually) simple, the code was easy and
straightforward to follow.

Thanks.

Re: [PATCH v2] for-each-ref: `:short` format for `refname`

[Bert Wesarg]

On Wed, Sep 3, 2008 at 01:10, Junio C Hamano <gitster@pobox.com> wrote:
> Bert Wesarg <bert.wesarg@googlemail.com> writes:
>
>> Tries to shorten the refname to a non-ambiguous name.
>> I.e. the full and the short refname points to the same object.
>
> The definition of "ambiguity" here is wrong (see below).
>
>> +              * by checking all rules in forward direction
>> +              */
>
> I think this part of the code is wrong, in that it talks about what object
> the ref points at.  That is not what ref ambiguity is about.
>
> Given a tag that points at a version 1.0.0 commit, this sequence will
> create:
>
>        $ git tag foo v1.0.0^0
>        $ git branch foo v1.0.0^0
>
> ambiguous branch and tag whose names are both 'foo', even though they
> point at the same thing.  The right API to use would be resolve_ref(), I
> think.
I use resolve_ref():

int read_ref(const char *ref, unsigned char *sha1)
{
	if (resolve_ref(ref, sha1, 1, NULL))
		return 0;
	return -1;
}

Ok, I think I get your point: it doesn't matter if the full and short
point to different objects, it's enough for ambiguity that the short
name resolves to more than one ref.

New patch in reply.

>
> Other than that, it is well done.
>
> Although I was initially a bit surprised by the size of the patch to
> implement something so (conceptually) simple, the code was easy and
> straightforward to follow.
>
> Thanks.
Your welcome.

Bert
>

[PATCH v3] for-each-ref: `:short` format for `refname`

[Bert Wesarg]

Try to shorten the refname to a non-ambiguous name.

Changes in v3:
 * don't compare sha1's, its ambiguous if the short name
   resovles to more than one ref
 * use xstrdup()
 * use indexes for the loops to clarify backward/forward
   direction


Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
Cc: git@vger.kernel.org
Cc: szeder@ira.uka.de
Cc: Shawn O. Pearce <spearce@spearce.org>
---
 Documentation/git-for-each-ref.txt |    1 +
 builtin-for-each-ref.c             |  138 ++++++++++++++++++++++++++++++++++--
 t/t6300-for-each-ref.sh            |   44 +++++++++++
 3 files changed, 176 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index eae6c0e..4016f59 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -74,6 +74,7 @@ For all objects, the following names can be used:
 
 refname::
 	The name of the ref (the part after $GIT_DIR/).
+	For a non-ambiguous short name of the ref append `:short`.
 
 objecttype::
 	The type of the object (`blob`, `tree`, `commit`, `tag`).
diff --git a/builtin-for-each-ref.c b/builtin-for-each-ref.c
index 21e92bb..abe0804 100644
--- a/builtin-for-each-ref.c
+++ b/builtin-for-each-ref.c
@@ -546,6 +546,110 @@ static void grab_values(struct atom_value *val, int deref, struct object *obj, v
 }
 
 /*
+ * generate a format suitable for scanf from a ref_rev_parse_rules
+ * rule, that is replace the "%.*s" spec with a "%s" spec
+ */
+static void gen_scanf_fmt(char *scanf_fmt, const char *rule)
+{
+	char *spec;
+
+	spec = strstr(rule, "%.*s");
+	if (!spec || strstr(spec + 4, "%.*s"))
+		die("invalid rule in ref_rev_parse_rules: %s", rule);
+
+	/* copy all until spec */
+	strncpy(scanf_fmt, rule, spec - rule);
+	scanf_fmt[spec - rule] = '\0';
+	/* copy new spec */
+	strcat(scanf_fmt, "%s");
+	/* copy remaining rule */
+	strcat(scanf_fmt, spec + 4);
+
+	return;
+}
+
+/*
+ * Shorten the refname to an non-ambiguous form
+ */
+static char *get_short_ref(struct refinfo *ref)
+{
+	int i;
+	static char **scanf_fmts;
+	static int nr_rules;
+	char *short_name;
+
+	/* pre generate scanf formats from ref_rev_parse_rules[] */
+	if (!nr_rules) {
+		size_t total_len = 0;
+
+		/* the rule list is NULL terminated, count them first */
+		for (; ref_rev_parse_rules[nr_rules]; nr_rules++)
+			/* no +1 because strlen("%s") < strlen("%.*s") */
+			total_len += strlen(ref_rev_parse_rules[nr_rules]);
+
+		scanf_fmts = xmalloc(nr_rules * sizeof(char *) + total_len);
+
+		total_len = 0;
+		for (i = 0; i < nr_rules; i++) {
+			scanf_fmts[i] = (char *)&scanf_fmts[nr_rules]
+					+ total_len;
+			gen_scanf_fmt(scanf_fmts[i], ref_rev_parse_rules[i]);
+			total_len += strlen(ref_rev_parse_rules[i]);
+		}
+	}
+
+	/* bail out if there are no rules */
+	if (!nr_rules)
+		return ref->refname;
+
+	/* buffer for scanf result, at most ref->refname must fit */
+	short_name = xstrdup(ref->refname);
+
+	/* skip first rule, it will always match */
+	for (i = nr_rules - 1; i > 0 ; --i) {
+		int j;
+		int short_name_len;
+
+		if (1 != sscanf(ref->refname, scanf_fmts[i], short_name))
+			continue;
+
+		short_name_len = strlen(short_name);
+
+		/*
+		 * check if the short name resolves to a valid ref,
+		 * but use only rules prior to the matched one
+		 */
+		for (j = 0; j < i; j++) {
+			const char *p = ref_rev_parse_rules[j];
+			unsigned char short_objectname[20];
+
+			/* check for valid ref */
+			if (read_ref(mkpath(p, short_name_len, short_name),
+				     short_objectname))
+				continue;
+
+			/*
+			 * short_name resolves to a valid ref,
+			 * but its a differnet path than that of ref->refname,
+			 * therefore the short name is ambiguous
+			 */
+			break;
+		}
+
+		/*
+		 * short name is non-ambiguous if all previous rules
+		 * don't resolve to a valid ref
+		 */
+		if (j == i)
+			return short_name;
+	}
+
+	free(short_name);
+	return ref->refname;
+}
+
+
+/*
  * Parse the object referred by ref, and grab needed value.
  */
 static void populate_value(struct refinfo *ref)
@@ -570,13 +674,33 @@ static void populate_value(struct refinfo *ref)
 	for (i = 0; i < used_atom_cnt; i++) {
 		const char *name = used_atom[i];
 		struct atom_value *v = &ref->value[i];
-		if (!strcmp(name, "refname"))
-			v->s = ref->refname;
-		else if (!strcmp(name, "*refname")) {
-			int len = strlen(ref->refname);
-			char *s = xmalloc(len + 4);
-			sprintf(s, "%s^{}", ref->refname);
-			v->s = s;
+		int deref = 0;
+		if (*name == '*') {
+			deref = 1;
+			name++;
+		}
+		if (!prefixcmp(name, "refname")) {
+			const char *formatp = strchr(name, ':');
+			const char *refname = ref->refname;
+
+			/* look for "short" refname format */
+			if (formatp) {
+				formatp++;
+				if (!strcmp(formatp, "short"))
+					refname = get_short_ref(ref);
+				else
+					die("unknown refname format %s",
+					    formatp);
+			}
+
+			if (!deref)
+				v->s = refname;
+			else {
+				int len = strlen(refname);
+				char *s = xmalloc(len + 4);
+				sprintf(s, "%s^{}", refname);
+				v->s = s;
+			}
 		}
 	}
 
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 8ced593..4f247dd 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -262,6 +262,50 @@ for i in "--perl --shell" "-s --python" "--python --tcl" "--tcl --perl"; do
 	"
 done
 
+cat >expected <<\EOF
+master
+testtag
+EOF
+
+test_expect_success 'Check short refname format' '
+	(git for-each-ref --format="%(refname:short)" refs/heads &&
+	git for-each-ref --format="%(refname:short)" refs/tags) >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'Check for invalid refname format' '
+	test_must_fail git for-each-ref --format="%(refname:INVALID)"
+'
+
+cat >expected <<\EOF
+heads/master
+master
+EOF
+
+test_expect_success 'Check ambiguous head and tag refs' '
+	git checkout -b newtag &&
+	echo "Using $datestamp" > one &&
+	git add one &&
+	git commit -m "Branch" &&
+	setdate_and_increment &&
+	git tag -m "Tagging at $datestamp" master &&
+	git for-each-ref --format "%(refname:short)" refs/heads/master refs/tags/master >actual &&
+	test_cmp expected actual
+'
+
+cat >expected <<\EOF
+heads/ambiguous
+ambiguous
+EOF
+
+test_expect_success 'Check ambiguous head and tag refs II' '
+	git checkout master &&
+	git tag ambiguous testtag^0 &&
+	git branch ambiguous testtag^0 &&
+	git for-each-ref --format "%(refname:short)" refs/heads/ambiguous refs/tags/ambiguous >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'an unusual tag with an incomplete line' '
 
 	git tag -m "bogo" bogo &&
-- 
1.6.0

Re: [PATCH v3] for-each-ref: `:short` format for `refname`

[Shawn O. Pearce]

Bert Wesarg <bert.wesarg@googlemail.com> wrote:
> Try to shorten the refname to a non-ambiguous name.
> 
> Changes in v3:
>  * don't compare sha1's, its ambiguous if the short name
>    resovles to more than one ref
>  * use xstrdup()
>  * use indexes for the loops to clarify backward/forward
>    direction
> 
> 
> Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
> Cc: git@vger.kernel.org
> Cc: szeder@ira.uka.de
> Cc: Shawn O. Pearce <spearce@spearce.org>
> ---

Looks good.  But the commit messages shouldn't have the "Changes
in v3" section or probably the "Cc:" lines.  The changes in v3
part usually goes below the ---.

I don't usually work on for-each-ref, but I'll toss an ack in anyway:

Acked-by: Shawn O. Pearce <spearce@spearce.org>

-- 
Shawn.

Re: [PATCH v3] for-each-ref: `:short` format for `refname`

[Bert Wesarg]

On Wed, Sep 3, 2008 at 17:18, Shawn O. Pearce <spearce@spearce.org> wrote:
> Bert Wesarg <bert.wesarg@googlemail.com> wrote:
>> Try to shorten the refname to a non-ambiguous name.
>>
>> Changes in v3:
>>  * don't compare sha1's, its ambiguous if the short name
>>    resovles to more than one ref
>>  * use xstrdup()
>>  * use indexes for the loops to clarify backward/forward
>>    direction
>>
>>
>> Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
>> Cc: git@vger.kernel.org
>> Cc: szeder@ira.uka.de
>> Cc: Shawn O. Pearce <spearce@spearce.org>
>> ---
>
> Looks good.  But the commit messages shouldn't have the "Changes
> in v3" section or probably the "Cc:" lines.  The changes in v3
> part usually goes below the ---.
Ok, but shouldn't git format-patch detect that there is already a ---
in the commit message, or is it illegal to have more than one ---?

And while we are at it, shouldn't git format-patch detect, that there
is already a [*PATCH*]-prefix in the first line of the commit?

I always need to edit these two things after format-patch.

Bert
>
> --
> Shawn.
>

Re: [PATCH v3] for-each-ref: `:short` format for `refname`

[Bert Wesarg]

On Wed, Sep 3, 2008 at 18:33, Bert Wesarg <bert.wesarg@googlemail.com> wrote:
> On Wed, Sep 3, 2008 at 17:18, Shawn O. Pearce <spearce@spearce.org> wrote:
>> Bert Wesarg <bert.wesarg@googlemail.com> wrote:
>>> Try to shorten the refname to a non-ambiguous name.
>>>
>>> Changes in v3:
>>>  * don't compare sha1's, its ambiguous if the short name
>>>    resovles to more than one ref
>>>  * use xstrdup()
>>>  * use indexes for the loops to clarify backward/forward
>>>    direction
>>>
>>>
>>> Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
>>> Cc: git@vger.kernel.org
>>> Cc: szeder@ira.uka.de
>>> Cc: Shawn O. Pearce <spearce@spearce.org>
>>> ---
>>
>> Looks good.  But the commit messages shouldn't have the "Changes
>> in v3" section or probably the "Cc:" lines.  The changes in v3
>> part usually goes below the ---.
> Ok, but shouldn't git format-patch detect that there is already a ---
> in the commit message, or is it illegal to have more than one ---?
>
> And while we are at it, shouldn't git format-patch detect, that there
> is already a [*PATCH*]-prefix in the first line of the commit?
Ok, I was too fast, -k is obviously the right option for this.

>
> I always need to edit these two things after format-patch.
>
> Bert
>>
>> --
>> Shawn.
>>
>

Re: [PATCH v3] for-each-ref: `:short` format for `refname`

[Junio C Hamano]

Bert Wesarg <bert.wesarg@googlemail.com> writes:

> Try to shorten the refname to a non-ambiguous name.
>
> Changes in v3:
>  * don't compare sha1's, its ambiguous if the short name
>    resovles to more than one ref
>  * use xstrdup()
>  * use indexes for the loops to clarify backward/forward
>    direction

Please don't do "Changes in v3" in the commit log message; do so after
"---" is very welcome as it gives people on the discussion _right now_
to understand the context of the change.

Also please do not omit discussion in the commit log message, to help
people _not_ on the discussion right now.

Your earlier versions will not appear on the history anywhere, and v3 is
the _only_ variant available to the people who need to understand what
this change was made for and why the code to implement the change was done
in the way the patch shows.  Write your commit log message to help people
who have "git show $this" and nothing else to figure these things out in
year 2010.

Namely:

 * Why "shortening" is a good thing?  For what use case this new feature
   was invented for?

 * What are the definitions of "shortening" and "non-ambiguous" in this
   feature's context?

In addition, for a complex feature with different possiblilities, it is a
good practice to describe what alternatatives and pros-and-cons among them
were considered while reaching this final version.  This is to help people
who would want to explore enhancement possibilities later; you beforehand
warn them about dead ends you have already explored.  For this particular
patch/feature, I do not think it is necessary, though.

[PATCH v4] for-each-ref: `:short` format for `refname`

[Bert Wesarg]

Tries to shorten the refname to a non-ambiguous name.

Szeder Gábor noticed that the git bash completion takes a
tremendous amount of time to strip leading components from
heads and tags refs (i.e. refs/heads, refs/tags, ...). He
proposed a new atom called 'refbasename' which removes at
most two leading components from the ref name.

I myself, proposed a more dynamic solution, which strips off
common leading components with the matched pattern.

But the current bash solution and both proposals suffer from
one mayor problem: ambiguous refs.

A ref is ambiguous, if it resolves to more than one full refs.
I.e. given the refs refs/heads/xyzzy and refs/tags/xyzzy. The
(short) ref xyzzy can point to both refs.

( Note: Its irrelevant whether the referenced objects are the
  same or not. )

This proposal solves this by checking for ambiguity of the
shorten ref name.

The shortening is done with the same rules for resolving refs
but in the reverse order. The short name is checked if it
resolves to a different ref.

To continue the above example, the output would be like this:

heads/xyzzy
xyzzy

So, if you want just tags, xyzzy is not ambiguous, because it
will resolve to a tag. If you need the heads you get a also
a non-ambiguous short form of the ref.

To integrate this new format into the bash completion to get
only non-ambiguous refs is beyond the scope of this patch.

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

---
Cc: git@vger.kernel.org
Cc: szeder@ira.uka.de
Cc: Shawn O. Pearce <spearce@spearce.org>

 Documentation/git-for-each-ref.txt |    1 +
 builtin-for-each-ref.c             |  135 ++++++++++++++++++++++++++++++++++--
 t/t6300-for-each-ref.sh            |   44 ++++++++++++
 3 files changed, 173 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index eae6c0e..4016f59 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -74,6 +74,7 @@ For all objects, the following names can be used:
 
 refname::
 	The name of the ref (the part after $GIT_DIR/).
+	For a non-ambiguous short name of the ref append `:short`.
 
 objecttype::
 	The type of the object (`blob`, `tree`, `commit`, `tag`).
diff --git a/builtin-for-each-ref.c b/builtin-for-each-ref.c
index 21e92bb..9b44092 100644
--- a/builtin-for-each-ref.c
+++ b/builtin-for-each-ref.c
@@ -546,6 +546,107 @@ static void grab_values(struct atom_value *val, int deref, struct object *obj, v
 }
 
 /*
+ * generate a format suitable for scanf from a ref_rev_parse_rules
+ * rule, that is replace the "%.*s" spec with a "%s" spec
+ */
+static void gen_scanf_fmt(char *scanf_fmt, const char *rule)
+{
+	char *spec;
+
+	spec = strstr(rule, "%.*s");
+	if (!spec || strstr(spec + 4, "%.*s"))
+		die("invalid rule in ref_rev_parse_rules: %s", rule);
+
+	/* copy all until spec */
+	strncpy(scanf_fmt, rule, spec - rule);
+	scanf_fmt[spec - rule] = '\0';
+	/* copy new spec */
+	strcat(scanf_fmt, "%s");
+	/* copy remaining rule */
+	strcat(scanf_fmt, spec + 4);
+
+	return;
+}
+
+/*
+ * Shorten the refname to an non-ambiguous form
+ */
+static char *get_short_ref(struct refinfo *ref)
+{
+	int i;
+	static char **scanf_fmts;
+	static int nr_rules;
+	char *short_name;
+
+	/* pre generate scanf formats from ref_rev_parse_rules[] */
+	if (!nr_rules) {
+		size_t total_len = 0;
+
+		/* the rule list is NULL terminated, count them first */
+		for (; ref_rev_parse_rules[nr_rules]; nr_rules++)
+			/* no +1 because strlen("%s") < strlen("%.*s") */
+			total_len += strlen(ref_rev_parse_rules[nr_rules]);
+
+		scanf_fmts = xmalloc(nr_rules * sizeof(char *) + total_len);
+
+		total_len = 0;
+		for (i = 0; i < nr_rules; i++) {
+			scanf_fmts[i] = (char *)&scanf_fmts[nr_rules]
+					+ total_len;
+			gen_scanf_fmt(scanf_fmts[i], ref_rev_parse_rules[i]);
+			total_len += strlen(ref_rev_parse_rules[i]);
+		}
+	}
+
+	/* bail out if there are no rules */
+	if (!nr_rules)
+		return ref->refname;
+
+	/* buffer for scanf result, at most ref->refname must fit */
+	short_name = xstrdup(ref->refname);
+
+	/* skip first rule, it will always match */
+	for (i = nr_rules - 1; i > 0 ; --i) {
+		int j;
+		int short_name_len;
+
+		if (1 != sscanf(ref->refname, scanf_fmts[i], short_name))
+			continue;
+
+		short_name_len = strlen(short_name);
+
+		/*
+		 * check if the short name resolves to a valid ref,
+		 * but use only rules prior to the matched one
+		 */
+		for (j = 0; j < i; j++) {
+			const char *rule = ref_rev_parse_rules[j];
+			unsigned char short_objectname[20];
+
+			/*
+			 * the short name is ambiguous, if it resolves
+			 * (with this previous rule) to a valid ref
+			 * read_ref() returns 0 on success
+			 */
+			if (!read_ref(mkpath(rule, short_name_len, short_name),
+				      short_objectname))
+				break;
+		}
+
+		/*
+		 * short name is non-ambiguous if all previous rules
+		 * haven't resolved to a valid ref
+		 */
+		if (j == i)
+			return short_name;
+	}
+
+	free(short_name);
+	return ref->refname;
+}
+
+
+/*
  * Parse the object referred by ref, and grab needed value.
  */
 static void populate_value(struct refinfo *ref)
@@ -570,13 +671,33 @@ static void populate_value(struct refinfo *ref)
 	for (i = 0; i < used_atom_cnt; i++) {
 		const char *name = used_atom[i];
 		struct atom_value *v = &ref->value[i];
-		if (!strcmp(name, "refname"))
-			v->s = ref->refname;
-		else if (!strcmp(name, "*refname")) {
-			int len = strlen(ref->refname);
-			char *s = xmalloc(len + 4);
-			sprintf(s, "%s^{}", ref->refname);
-			v->s = s;
+		int deref = 0;
+		if (*name == '*') {
+			deref = 1;
+			name++;
+		}
+		if (!prefixcmp(name, "refname")) {
+			const char *formatp = strchr(name, ':');
+			const char *refname = ref->refname;
+
+			/* look for "short" refname format */
+			if (formatp) {
+				formatp++;
+				if (!strcmp(formatp, "short"))
+					refname = get_short_ref(ref);
+				else
+					die("unknown refname format %s",
+					    formatp);
+			}
+
+			if (!deref)
+				v->s = refname;
+			else {
+				int len = strlen(refname);
+				char *s = xmalloc(len + 4);
+				sprintf(s, "%s^{}", refname);
+				v->s = s;
+			}
 		}
 	}
 
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 8ced593..4f247dd 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -262,6 +262,50 @@ for i in "--perl --shell" "-s --python" "--python --tcl" "--tcl --perl"; do
 	"
 done
 
+cat >expected <<\EOF
+master
+testtag
+EOF
+
+test_expect_success 'Check short refname format' '
+	(git for-each-ref --format="%(refname:short)" refs/heads &&
+	git for-each-ref --format="%(refname:short)" refs/tags) >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'Check for invalid refname format' '
+	test_must_fail git for-each-ref --format="%(refname:INVALID)"
+'
+
+cat >expected <<\EOF
+heads/master
+master
+EOF
+
+test_expect_success 'Check ambiguous head and tag refs' '
+	git checkout -b newtag &&
+	echo "Using $datestamp" > one &&
+	git add one &&
+	git commit -m "Branch" &&
+	setdate_and_increment &&
+	git tag -m "Tagging at $datestamp" master &&
+	git for-each-ref --format "%(refname:short)" refs/heads/master refs/tags/master >actual &&
+	test_cmp expected actual
+'
+
+cat >expected <<\EOF
+heads/ambiguous
+ambiguous
+EOF
+
+test_expect_success 'Check ambiguous head and tag refs II' '
+	git checkout master &&
+	git tag ambiguous testtag^0 &&
+	git branch ambiguous testtag^0 &&
+	git for-each-ref --format "%(refname:short)" refs/heads/ambiguous refs/tags/ambiguous >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'an unusual tag with an incomplete line' '
 
 	git tag -m "bogo" bogo &&
-- 
1.6.0

Re: [PATCH v4] for-each-ref: `:short` format for `refname`

[Junio C Hamano]

Bert Wesarg <bert.wesarg@googlemail.com> writes:

> ...
> To integrate this new format into the bash completion to get
> only non-ambiguous refs is beyond the scope of this patch.
>
> Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
>
> ---
> Cc: git@vger.kernel.org
> Cc: szeder@ira.uka.de
> Cc: Shawn O. Pearce <spearce@spearce.org>

Nice writeup of the history of this patch, if on tad-too-verbose side.

> diff --git a/builtin-for-each-ref.c b/builtin-for-each-ref.c
> index 21e92bb..9b44092 100644
> --- a/builtin-for-each-ref.c
> +++ b/builtin-for-each-ref.c
> @@ -546,6 +546,107 @@ static void grab_values(struct atom_value *val, int deref, struct object *obj, v
> +/*
> + * Shorten the refname to an non-ambiguous form
> + */
> +static char *get_short_ref(struct refinfo *ref)
> +{
> ...
> +	/* skip first rule, it will always match */
> +	for (i = nr_rules - 1; i > 0 ; --i) {
> +		int j;
> +		int short_name_len;
> +
> +		if (1 != sscanf(ref->refname, scanf_fmts[i], short_name))
> +			continue;
> +
> +		short_name_len = strlen(short_name);
> +
> +		/*
> +		 * check if the short name resolves to a valid ref,
> +		 * but use only rules prior to the matched one
> +		 */
> +		for (j = 0; j < i; j++) {
> ...
> +		}
> +		/*
> +		 * short name is non-ambiguous if all previous rules
> +		 * haven't resolved to a valid ref
> +		 */
> +		if (j == i)
> +			return short_name;

Is this inner loop essentially the same as calling dwim_ref(), while
temporarily turning warn_ambiguous_refs on, and checking for return value
of one?

> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index 8ced593..4f247dd 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -262,6 +262,50 @@ for i in "--perl --shell" "-s --python" "--python --tcl" "--tcl --perl"; do
>  	"
>  done
>  
> +cat >expected <<\EOF
> +master
> +testtag
> +EOF
> +
> +test_expect_success 'Check short refname format' '
> +	(git for-each-ref --format="%(refname:short)" refs/heads &&
> +	git for-each-ref --format="%(refname:short)" refs/tags) >actual &&
> +	test_cmp expected actual
> +'

Not a complaint nor objection but mere curiosity.  Why does this run two
for-each-ref, not just one with two patterns?

> +test_expect_success 'Check for invalid refname format' '
> +	test_must_fail git for-each-ref --format="%(refname:INVALID)"
> +'

Good.

> +cat >expected <<\EOF
> +heads/master
> +master
> +EOF
> +
> +test_expect_success 'Check ambiguous head and tag refs' '
> +	git checkout -b newtag &&
> +	echo "Using $datestamp" > one &&
> +	git add one &&
> +	git commit -m "Branch" &&
> +	setdate_and_increment &&
> +	git tag -m "Tagging at $datestamp" master &&
> +	git for-each-ref --format "%(refname:short)" refs/heads/master refs/tags/master >actual &&
> +	test_cmp expected actual
> +'
> +
> +cat >expected <<\EOF
> +heads/ambiguous
> +ambiguous
> +EOF
> +
> +test_expect_success 'Check ambiguous head and tag refs II' '
> +	git checkout master &&
> +	git tag ambiguous testtag^0 &&
> +	git branch ambiguous testtag^0 &&
> +	git for-each-ref --format "%(refname:short)" refs/heads/ambiguous refs/tags/ambiguous >actual &&
> +	test_cmp expected actual
> +'
> +

Can we also try first creating a clone of some repo and run:

	for-each-ref --format="%(refname:short)" refs/remotes

I am unsure how "remotes/origin" when "refs/remotes/origin/HEAD" points at
their 'master' branch behaves with your code, and/or how it should behave.

Other than that, nicely done.

Re: [PATCH v4] for-each-ref: `:short` format for `refname`

[Bert Wesarg]

On Sat, Sep 6, 2008 at 00:20, Junio C Hamano <gitster@pobox.com> wrote:
> Bert Wesarg <bert.wesarg@googlemail.com> writes:
>
>> ...
>> To integrate this new format into the bash completion to get
>> only non-ambiguous refs is beyond the scope of this patch.
>>
>> Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
>>
>> ---
>> Cc: git@vger.kernel.org
>> Cc: szeder@ira.uka.de
>> Cc: Shawn O. Pearce <spearce@spearce.org>
>
> Nice writeup of the history of this patch, if on tad-too-verbose side.
>
>> diff --git a/builtin-for-each-ref.c b/builtin-for-each-ref.c
>> index 21e92bb..9b44092 100644
>> --- a/builtin-for-each-ref.c
>> +++ b/builtin-for-each-ref.c
>> @@ -546,6 +546,107 @@ static void grab_values(struct atom_value *val, int deref, struct object *obj, v
>> +/*
>> + * Shorten the refname to an non-ambiguous form
>> + */
>> +static char *get_short_ref(struct refinfo *ref)
>> +{
>> ...
>> +     /* skip first rule, it will always match */
>> +     for (i = nr_rules - 1; i > 0 ; --i) {
>> +             int j;
>> +             int short_name_len;
>> +
>> +             if (1 != sscanf(ref->refname, scanf_fmts[i], short_name))
>> +                     continue;
>> +
>> +             short_name_len = strlen(short_name);
>> +
>> +             /*
>> +              * check if the short name resolves to a valid ref,
>> +              * but use only rules prior to the matched one
>> +              */
>> +             for (j = 0; j < i; j++) {
>> ...
>> +             }
>> +             /*
>> +              * short name is non-ambiguous if all previous rules
>> +              * haven't resolved to a valid ref
>> +              */
>> +             if (j == i)
>> +                     return short_name;
>
> Is this inner loop essentially the same as calling dwim_ref(), while
> temporarily turning warn_ambiguous_refs on, and checking for return value
> of one?
Not exactly.

Short version:

To follow my above example, with dwim_ref() we would get this:

heads/xyzzy
tags/xyzzy

Long version:

Currently we consider only rules prior to the matched rule (the rule
which gives us the short name). That is, if any of these rules will
also resolve to a valid ref, the short name is  ambiguous, else its
unambiguous. If we would consider subsequent rules past the matched
one, we may find more valid refs for this short name. Because the
current rule would match first if we try to resolve the short name, we
don't have to check these rules. We get only a "ambiguous refs"
warning.

I have no opinion if we want this 'strict unambiguousness'.

>
>> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
>> index 8ced593..4f247dd 100755
>> --- a/t/t6300-for-each-ref.sh
>> +++ b/t/t6300-for-each-ref.sh
>> @@ -262,6 +262,50 @@ for i in "--perl --shell" "-s --python" "--python --tcl" "--tcl --perl"; do
>>       "
>>  done
>>
>> +cat >expected <<\EOF
>> +master
>> +testtag
>> +EOF
>> +
>> +test_expect_success 'Check short refname format' '
>> +     (git for-each-ref --format="%(refname:short)" refs/heads &&
>> +     git for-each-ref --format="%(refname:short)" refs/tags) >actual &&
>> +     test_cmp expected actual
>> +'
>
> Not a complaint nor objection but mere curiosity.  Why does this run two
> for-each-ref, not just one with two patterns?
Its more a leftover from the :strip version, where the pattern was the point of
interest.

>
>> +test_expect_success 'Check for invalid refname format' '
>> +     test_must_fail git for-each-ref --format="%(refname:INVALID)"
>> +'
>
> Good.
>
>> +cat >expected <<\EOF
>> +heads/master
>> +master
>> +EOF
>> +
>> +test_expect_success 'Check ambiguous head and tag refs' '
>> +     git checkout -b newtag &&
>> +     echo "Using $datestamp" > one &&
>> +     git add one &&
>> +     git commit -m "Branch" &&
>> +     setdate_and_increment &&
>> +     git tag -m "Tagging at $datestamp" master &&
>> +     git for-each-ref --format "%(refname:short)" refs/heads/master refs/tags/master >actual &&
>> +     test_cmp expected actual
>> +'
>> +
>> +cat >expected <<\EOF
>> +heads/ambiguous
>> +ambiguous
>> +EOF
>> +
>> +test_expect_success 'Check ambiguous head and tag refs II' '
>> +     git checkout master &&
>> +     git tag ambiguous testtag^0 &&
>> +     git branch ambiguous testtag^0 &&
>> +     git for-each-ref --format "%(refname:short)" refs/heads/ambiguous refs/tags/ambiguous >actual &&
>> +     test_cmp expected actual
>> +'
>> +
>
> Can we also try first creating a clone of some repo and run:
>
>        for-each-ref --format="%(refname:short)" refs/remotes
>
> I am unsure how "remotes/origin" when "refs/remotes/origin/HEAD" points at
> their 'master' branch behaves with your code, and/or how it should behave.
I will look at this.


>
> Other than that, nicely done.
>
>

Bert

Re: [PATCH v4] for-each-ref: `:short` format for `refname`

[Junio C Hamano]

Any followup on this topic since:

  http://thread.gmane.org/gmane.comp.version-control.git/94478/focus=95041

Re: [PATCH v4] for-each-ref: `:short` format for `refname`

[Shawn O. Pearce]

Junio C Hamano <gitster@pobox.com> wrote:
> Any followup on this topic since:
> 
>   http://thread.gmane.org/gmane.comp.version-control.git/94478/focus=95041
> 

Sorry.  This patch looked good to me, but the commit message was
perhaps a bit long on the history of the patch.  I don't quite have
the time to give it enough of a read to offer an Ack'd-by.  But it
seems correct on the first read.

-- 
Shawn.

Re: [PATCH v4] for-each-ref: `:short` format for `refname`

[Bert Wesarg]

On Tue, Sep 9, 2008 at 00:57, Junio C Hamano <gitster@pobox.com> wrote:
> Any followup on this topic since:
>
>  http://thread.gmane.org/gmane.comp.version-control.git/94478/focus=95041
>
>
I plan on looking at the refs/remote test today.

Any opinions, whether we want the 'strict' mode? i.e.:

for refs/heads/xyzzy and refs/tags/xyzzy:

loose mode (current implementation):

  refs/heads/xyzzy => heads/xyzzy
  refs/tags/xyzzy  => xyzzy

there would be a ambiguous warning (if enabled) if you use xyzzy as a
tag, but it resolves correctly to the tag.

strict mode:

  refs/heads/xyzzy => heads/xyzzy
  refs/tags/xyzzy  => tags/xyzzy

will always produce a non-ambiguous short forms.

Bert

Re: [PATCH v4] for-each-ref: `:short` format for `refname`

[Junio C Hamano]

"Bert Wesarg" <bert.wesarg@googlemail.com> writes:

> Any opinions, whether we want the 'strict' mode? i.e.:
>
> for refs/heads/xyzzy and refs/tags/xyzzy:
>
> loose mode (current implementation):
>
>   refs/heads/xyzzy => heads/xyzzy
>   refs/tags/xyzzy  => xyzzy
>
> there would be a ambiguous warning (if enabled) if you use xyzzy as a
> tag, but it resolves correctly to the tag.
>
> strict mode:
>
>   refs/heads/xyzzy => heads/xyzzy
>   refs/tags/xyzzy  => tags/xyzzy
>
> will always produce a non-ambiguous short forms.

I have no strong opinions either way, but if we want to pick only one, I
suspect that the loose mode would be more appropriate for bash completion
purposes exactly because:

 (1) the shorter form would match the users' expectations, and;

 (2) if it triggers ambiguity warning to use that result that matches
     users' expectations, it is a *good thing* --- it reminds the user
     that s/he is playing with fire _if_ the user is of careful type who
     enables the ambiguity warning.

Thinking about it from a different angle, it would make more sense to use
loose mode if the user does not have ambiguity warning configured, and use
strict mode if the warning is enabled.  Then people who will get warnings
from ambiguity will not get an ambiguous completion, and people who won't
will get shorter but still unambiguous completion.

Which means, despite what I said earlier, now I have a mild preference to
tie the choice to core.wawrnambigousrefs configuration.

Re: [PATCH v4] for-each-ref: `:short` format for `refname`

[Bert Wesarg]

On Tue, Sep 9, 2008 at 10:05, Junio C Hamano <gitster@pobox.com> wrote:
> "Bert Wesarg" <bert.wesarg@googlemail.com> writes:
>
>> Any opinions, whether we want the 'strict' mode? i.e.:
>>
>> for refs/heads/xyzzy and refs/tags/xyzzy:
>>
>> loose mode (current implementation):
>>
>>   refs/heads/xyzzy => heads/xyzzy
>>   refs/tags/xyzzy  => xyzzy
>>
>> there would be a ambiguous warning (if enabled) if you use xyzzy as a
>> tag, but it resolves correctly to the tag.
>>
>> strict mode:
>>
>>   refs/heads/xyzzy => heads/xyzzy
>>   refs/tags/xyzzy  => tags/xyzzy
>>
>> will always produce a non-ambiguous short forms.
>
> I have no strong opinions either way, but if we want to pick only one, I
> suspect that the loose mode would be more appropriate for bash completion
> purposes exactly because:
>
>  (1) the shorter form would match the users' expectations, and;
>
>  (2) if it triggers ambiguity warning to use that result that matches
>     users' expectations, it is a *good thing* --- it reminds the user
>     that s/he is playing with fire _if_ the user is of careful type who
>     enables the ambiguity warning.
>
> Thinking about it from a different angle, it would make more sense to use
> loose mode if the user does not have ambiguity warning configured, and use
> strict mode if the warning is enabled.  Then people who will get warnings
> from ambiguity will not get an ambiguous completion, and people who won't
> will get shorter but still unambiguous completion.
>
> Which means, despite what I said earlier, now I have a mild preference to
> tie the choice to core.wawrnambigousrefs configuration.
A really nice idea.  Await PATCH v5.  I had to change my plans for
today, unfortunately.

Bert
>