[PATCH 1/3] for-each-ref: utilize core.warnambiguousrefs for strict refname:short format

[PATCH 1/3] for-each-ref: utilize core.warnambiguousrefs for strict refname:short format

[Bert Wesarg]

core.warnambiguousrefs is used to enable strict mode for the
abbreviation.

In strict mode, the abbreviated ref will never trigger the
'warn_ambiguous_refs' warning. I.e. for these refs:

  refs/heads/xyzzy
  refs/tags/xyzzy

the abbreviated forms are:

  heads/xyzzy
  tags/xyzzy


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

---

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.

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             |   43 ++++++++++++++++++++++-------------
 2 files changed, 29 insertions(+), 16 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 5061d3e..265bbf3 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -75,6 +75,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`.
+	The option core.warnambiguousrefs is used to enable the strict mode
+	for the abbretiation.
 
 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 9b44092..e7b7712 100644
--- a/builtin-for-each-ref.c
+++ b/builtin-for-each-ref.c
@@ -571,7 +571,7 @@ static void gen_scanf_fmt(char *scanf_fmt, const char *rule)
 /*
  * Shorten the refname to an non-ambiguous form
  */
-static char *get_short_ref(struct refinfo *ref)
+static void get_short_ref(struct refinfo *ref, int strict, char **short_ref)
 {
 	int i;
 	static char **scanf_fmts;
@@ -598,16 +598,16 @@ static char *get_short_ref(struct refinfo *ref)
 		}
 	}
 
-	/* 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);
+	*short_ref = short_name;
 
-	/* skip first rule, it will always match */
-	for (i = nr_rules - 1; i > 0 ; --i) {
-		int j;
+	/* bail out if there are no rules */
+	if (!nr_rules)
+		return;
+
+	for (i = nr_rules - 1; i >= 0 ; --i) {
+		int j, rules_to_fail = i;
 		int short_name_len;
 
 		if (1 != sscanf(ref->refname, scanf_fmts[i], short_name))
@@ -616,13 +616,23 @@ static char *get_short_ref(struct refinfo *ref)
 		short_name_len = strlen(short_name);
 
 		/*
+		 * in strict mode, all (except the matched one) rules
+		 * must fail to resolve to a valid ref
+		 */
+		if (strict)
+			rules_to_fail = nr_rules;
+		/*
 		 * 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++) {
+		for (j = 0; j < rules_to_fail; j++) {
 			const char *rule = ref_rev_parse_rules[j];
 			unsigned char short_objectname[20];
 
+			/* skip matched rule */
+			if (i == j)
+				continue;
+
 			/*
 			 * the short name is ambiguous, if it resolves
 			 * (with this previous rule) to a valid ref
@@ -635,14 +645,14 @@ static char *get_short_ref(struct refinfo *ref)
 
 		/*
 		 * short name is non-ambiguous if all previous rules
-		 * haven't resolved to a valid ref
+		 * doesn't resolved to a valid ref
 		 */
-		if (j == i)
-			return short_name;
+		if (j == rules_to_fail)
+			return;
 	}
 
-	free(short_name);
-	return ref->refname;
+	/* can't abbreviate refname, return full name */
+	strcpy(short_name, ref->refname);
 }
 
 
@@ -678,13 +688,14 @@ static void populate_value(struct refinfo *ref)
 		}
 		if (!prefixcmp(name, "refname")) {
 			const char *formatp = strchr(name, ':');
-			const char *refname = ref->refname;
+			char *refname = ref->refname;
 
 			/* look for "short" refname format */
 			if (formatp) {
 				formatp++;
 				if (!strcmp(formatp, "short"))
-					refname = get_short_ref(ref);
+					get_short_ref(ref, warn_ambiguous_refs,
+						      &refname);
 				else
 					die("unknown refname format %s",
 					    formatp);
-- 
1.6.0.1

[PATCH 2/3] for-each-ref: factor out get_short_ref() into refs.c:abbreviate_refname()

[Bert Wesarg]

Moves the function get_short_ref() from builtin-for-each-ref.c into refs.c
as function abbreviate_refname().

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>

 builtin-for-each-ref.c |  116 +----------------------------------------------
 cache.h                |    1 +
 refs.c                 |  110 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 114 insertions(+), 113 deletions(-)

diff --git a/builtin-for-each-ref.c b/builtin-for-each-ref.c
index e7b7712..06fd6a2 100644
--- a/builtin-for-each-ref.c
+++ b/builtin-for-each-ref.c
@@ -546,117 +546,6 @@ 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 void get_short_ref(struct refinfo *ref, int strict, char **short_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]);
-		}
-	}
-
-	/* buffer for scanf result, at most ref->refname must fit */
-	short_name = xstrdup(ref->refname);
-	*short_ref = short_name;
-
-	/* bail out if there are no rules */
-	if (!nr_rules)
-		return;
-
-	for (i = nr_rules - 1; i >= 0 ; --i) {
-		int j, rules_to_fail = i;
-		int short_name_len;
-
-		if (1 != sscanf(ref->refname, scanf_fmts[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 ref
-		 */
-		if (strict)
-			rules_to_fail = nr_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];
-			unsigned char short_objectname[20];
-
-			/* skip matched rule */
-			if (i == j)
-				continue;
-
-			/*
-			 * 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
-		 * doesn't resolved to a valid ref
-		 */
-		if (j == rules_to_fail)
-			return;
-	}
-
-	/* can't abbreviate refname, return full name */
-	strcpy(short_name, ref->refname);
-}
-
-
-/*
  * Parse the object referred by ref, and grab needed value.
  */
 static void populate_value(struct refinfo *ref)
@@ -694,8 +583,9 @@ static void populate_value(struct refinfo *ref)
 			if (formatp) {
 				formatp++;
 				if (!strcmp(formatp, "short"))
-					get_short_ref(ref, warn_ambiguous_refs,
-						      &refname);
+					abbreviate_refname(ref->refname,
+							   warn_ambiguous_refs,
+							   &refname);
 				else
 					die("unknown refname format %s",
 					    formatp);
diff --git a/cache.h b/cache.h
index de8c2b6..8a128a5 100644
--- a/cache.h
+++ b/cache.h
@@ -582,6 +582,7 @@ extern int dwim_log(const char *str, int len, unsigned char *sha1, char **ref);
 extern int refname_match(const char *abbrev_name, const char *full_name, const char **rules);
 extern const char *ref_rev_parse_rules[];
 extern const char *ref_fetch_rules[];
+extern void abbreviate_refname(const char *refname, int strict, char **abbrev_name);
 
 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 b680750..f370081 100644
--- a/refs.c
+++ b/refs.c
@@ -730,6 +730,116 @@ int refname_match(const char *abbrev_name, const char *full_name, const char **r
 	return 0;
 }
 
+/*
+ * 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);
+}
+
+/*
+ * Abbreviate refname to an non-ambiguous form, undefined if refname is not
+ * a fully quallified refname
+ */
+void abbreviate_refname(const char *refname, int strict, char **abbrev_name)
+{
+	int i;
+	static char **scanf_fmts;
+	static int nr_rules;
+	char *abbrev;
+
+	/* 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]);
+		}
+	}
+
+	/* buffer for scanf result, at most refname must fit */
+	abbrev = xstrdup(refname);
+	*abbrev_name = abbrev;
+
+	/* bail out if there are no rules */
+	if (!nr_rules)
+		return;
+
+	for (i = nr_rules - 1; i >= 0 ; --i) {
+		int j, rules_to_fail = i;
+		int abbrev_len;
+
+		if (1 != sscanf(refname, scanf_fmts[i], abbrev))
+			continue;
+
+		abbrev_len = strlen(abbrev);
+
+		/*
+		 * in strict mode, all (except the matched one) rules
+		 * must fail to resolve to a valid ref
+		 */
+		if (strict)
+			rules_to_fail = nr_rules;
+
+		/*
+		 * check if the short name resolves to a valid ref,
+		 */
+		for (j = 0; j < rules_to_fail; j++) {
+			const char *rule = ref_rev_parse_rules[j];
+			unsigned char abbrev_objectname[20];
+
+			/* skip matched rule */
+			if (i == j)
+				continue;
+
+			/*
+			 * the abbreviated name is ambiguous,
+			 * if it resolves to a valid ref
+			 *
+			 * read_ref() returns 0 on success
+			 */
+			if (!read_ref(mkpath(rule, abbrev_len, abbrev),
+				      abbrev_objectname))
+				break;
+		}
+
+		/*
+		 * abbrev is non-ambiguous if all rules
+		 * doesn't resolved to a valid ref
+		 */
+		if (j == rules_to_fail)
+			return;
+	}
+
+	/* can't abbreviate refname, return full name */
+	strcpy(abbrev, refname);
+}
+
 static struct ref_lock *verify_lock(struct ref_lock *lock,
 	const unsigned char *old_sha1, int mustexist)
 {
-- 
1.6.0.1

[PATCH 3/3] git abbref-ref: new porcelain for abbreviate_ref()

[Bert Wesarg]

This gives direct access to the abbreviate_ref() function. The operation
mode defaults to the core.warnambiguousrefs value, like the refname:short
format, but can be explicitly changed with the --{,no}-strict option.

The bash completion script utilizes this new command.

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

Junio, if this is not a porcelain, tell me.

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

 .gitignore                             |    1 +
 Documentation/git-abbrev-ref.txt       |   34 +++++++++++++++++++++++++++
 Makefile                               |    1 +
 builtin-abbrev-ref.c                   |   40 ++++++++++++++++++++++++++++++++
 builtin.h                              |    1 +
 contrib/completion/git-completion.bash |   16 ++++++------
 git.c                                  |    1 +
 7 files changed, 86 insertions(+), 8 deletions(-)
 create mode 100644 Documentation/git-abbrev-ref.txt
 create mode 100644 builtin-abbrev-ref.c

diff --git a/.gitignore b/.gitignore
index bbaf9de..c2d0ce4 100644
--- a/.gitignore
+++ b/.gitignore
@@ -3,6 +3,7 @@ GIT-CFLAGS
 GIT-GUI-VARS
 GIT-VERSION-FILE
 git
+git-abbrev-ref
 git-add
 git-add--interactive
 git-am
diff --git a/Documentation/git-abbrev-ref.txt b/Documentation/git-abbrev-ref.txt
new file mode 100644
index 0000000..67f6733
--- /dev/null
+++ b/Documentation/git-abbrev-ref.txt
@@ -0,0 +1,34 @@
+git-abbrev-ref(1)
+=================
+
+NAME
+----
+git-abbrev-ref - Abbreviate a named ref
+
+SYNOPSIS
+--------
+'git abbrev-ref' [--strict] <ref>...
+
+DESCRIPTION
+-----------
+
+
+OPTIONS
+-------
+<ref>...::
+	Refnames to be abbreviated.
+
+--strict::
+	Operates in strict mode. Defaults to core.warnambiguousrefs.
+
+Author
+------
+Written by Bert Wesarg <bert.wesarg@googlemail.com>
+
+Documentation
+--------------
+Documentation by Bert Wesarg and the git-list <git@vger.kernel.org>.
+
+GIT
+---
+Part of the linkgit:git[1] suite
diff --git a/Makefile b/Makefile
index 3c0664a..f78e75f 100644
--- a/Makefile
+++ b/Makefile
@@ -493,6 +493,7 @@ LIB_OBJS += ws.o
 LIB_OBJS += wt-status.o
 LIB_OBJS += xdiff-interface.o
 
+BUILTIN_OBJS += builtin-abbrev-ref.o
 BUILTIN_OBJS += builtin-add.o
 BUILTIN_OBJS += builtin-annotate.o
 BUILTIN_OBJS += builtin-apply.o
diff --git a/builtin-abbrev-ref.c b/builtin-abbrev-ref.c
new file mode 100644
index 0000000..4c4d42c
--- /dev/null
+++ b/builtin-abbrev-ref.c
@@ -0,0 +1,40 @@
+#include "builtin.h"
+#include "cache.h"
+#include "refs.h"
+#include "parse-options.h"
+
+static const char * const git_abbrev_ref_usage[] = {
+	"git abbrev-ref [options] ref...",
+	NULL
+};
+
+int cmd_abbrev_ref(int argc, const char **argv, const char *prefix)
+{
+	int i;
+	int strict = 0;
+	struct option options[] = {
+		OPT_BOOLEAN( 0 , "strict", &strict, "use strict mode"),
+		OPT_END(),
+	};
+
+	git_config(git_default_config, NULL);
+	strict = warn_ambiguous_refs;
+	argc = parse_options(argc, argv, options, git_abbrev_ref_usage, 0);
+
+	for (i = 0; i < argc; i++) {
+		unsigned char sha1[20];
+		char *ref;
+		char *abbrev_ref;
+
+		if (!dwim_ref(argv[i], strlen(argv[i]), sha1, &ref))
+			die("No such ref %s", argv[i]);
+
+		abbreviate_refname(ref, strict, &abbrev_ref);
+		puts(abbrev_ref);
+
+		free(ref);
+		free(abbrev_ref);
+	}
+
+	return 0;
+}
diff --git a/builtin.h b/builtin.h
index e67cb20..8271a4e 100644
--- a/builtin.h
+++ b/builtin.h
@@ -20,6 +20,7 @@ extern int commit_tree(const char *msg, unsigned char *tree,
 		struct commit_list *parents, unsigned char *ret);
 extern int check_pager_config(const char *cmd);
 
+extern int cmd_abbrev_ref(int argc, const char **argv, const char *prefix);
 extern int cmd_add(int argc, const char **argv, const char *prefix);
 extern int cmd_annotate(int argc, const char **argv, const char *prefix);
 extern int cmd_apply(int argc, const char **argv, const char *prefix);
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 93f0881..7f002c0 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -112,9 +112,9 @@ __git_ps1 ()
 		fi
 
 		if [ -n "$1" ]; then
-			printf "$1" "${b##refs/heads/}$r"
+			printf "$1" "$(git abbrev-ref $b)$r"
 		else
-			printf " (%s)" "${b##refs/heads/}$r"
+			printf " (%s)" "$(git abbrev-ref $b)$r"
 		fi
 	fi
 }
@@ -162,7 +162,7 @@ __git_heads ()
 		case "$is_hash,$i" in
 		y,*) is_hash=n ;;
 		n,*^{}) is_hash=y ;;
-		n,refs/heads/*) is_hash=y; echo "${i#refs/heads/}" ;;
+		n,refs/heads/*) is_hash=y; echo "$(git abbrev-ref $i)" ;;
 		n,*) is_hash=y; echo "$i" ;;
 		esac
 	done
@@ -180,7 +180,7 @@ __git_tags ()
 		case "$is_hash,$i" in
 		y,*) is_hash=n ;;
 		n,*^{}) is_hash=y ;;
-		n,refs/tags/*) is_hash=y; echo "${i#refs/tags/}" ;;
+		n,refs/tags/*) is_hash=y; echo "$(git abbrev-ref $i)" ;;
 		n,*) is_hash=y; echo "$i" ;;
 		esac
 	done
@@ -199,9 +199,9 @@ __git_refs ()
 		case "$is_hash,$i" in
 		y,*) is_hash=n ;;
 		n,*^{}) is_hash=y ;;
-		n,refs/tags/*) is_hash=y; echo "${i#refs/tags/}" ;;
-		n,refs/heads/*) is_hash=y; echo "${i#refs/heads/}" ;;
-		n,refs/remotes/*) is_hash=y; echo "${i#refs/remotes/}" ;;
+		n,refs/tags/*) is_hash=y; echo "$(git abbrev-ref $i)" ;;
+		n,refs/heads/*) is_hash=y; echo "$(git abbrev-ref $i)" ;;
+		n,refs/remotes/*) is_hash=y; echo "$(git abbrev-ref $i)" ;;
 		n,*) is_hash=y; echo "$i" ;;
 		esac
 	done
@@ -222,7 +222,7 @@ __git_refs_remotes ()
 		case "$is_hash,$i" in
 		n,refs/heads/*)
 			is_hash=y
-			echo "$i:refs/remotes/$1/${i#refs/heads/}"
+			echo "$i:refs/remotes/$1/$(git abbrev-ref $i)"
 			;;
 		y,*) is_hash=n ;;
 		n,*^{}) is_hash=y ;;
diff --git a/git.c b/git.c
index 905acc2..052ebb5 100644
--- a/git.c
+++ b/git.c
@@ -263,6 +263,7 @@ static void handle_internal_command(int argc, const char **argv)
 {
 	const char *cmd = argv[0];
 	static struct cmd_struct commands[] = {
+		{ "abbrev-ref", cmd_abbrev_ref, RUN_SETUP },
 		{ "add", cmd_add, RUN_SETUP | NEED_WORK_TREE },
 		{ "annotate", cmd_annotate, RUN_SETUP },
 		{ "apply", cmd_apply },
-- 
1.6.0.1

Re: [PATCH 3/3] git abbref-ref: new porcelain for abbreviate_ref()

[Shawn O. Pearce]

Bert Wesarg <bert.wesarg@googlemail.com> wrote:
> This gives direct access to the abbreviate_ref() function. The operation
> mode defaults to the core.warnambiguousrefs value, like the refname:short
> format, but can be explicitly changed with the --{,no}-strict option.
> 
> The bash completion script utilizes this new command.

And it slows down too, doesn't it?  Now we are doing a fork per
branch during completion.  Yikes.  Didn't you just post a series
about making completion faster?
 
> Junio, if this is not a porcelain, tell me.

IMHO its plumbing.  Porcelain is used by a human.  Plumbing is the
bits needed to make human interfaces.
 
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 93f0881..7f002c0 100755
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -112,9 +112,9 @@ __git_ps1 ()
>  		fi
>  
>  		if [ -n "$1" ]; then
> -			printf "$1" "${b##refs/heads/}$r"
> +			printf "$1" "$(git abbrev-ref $b)$r"
>  		else
> -			printf " (%s)" "${b##refs/heads/}$r"
> +			printf " (%s)" "$(git abbrev-ref $b)$r"
>  		fi
>  	fi
>  }
> @@ -162,7 +162,7 @@ __git_heads ()
>  		case "$is_hash,$i" in
>  		y,*) is_hash=n ;;
>  		n,*^{}) is_hash=y ;;
> -		n,refs/heads/*) is_hash=y; echo "${i#refs/heads/}" ;;
> +		n,refs/heads/*) is_hash=y; echo "$(git abbrev-ref $i)" ;;
>  		n,*) is_hash=y; echo "$i" ;;
>  		esac
>  	done
> @@ -180,7 +180,7 @@ __git_tags ()
>  		case "$is_hash,$i" in
>  		y,*) is_hash=n ;;
>  		n,*^{}) is_hash=y ;;
> -		n,refs/tags/*) is_hash=y; echo "${i#refs/tags/}" ;;
> +		n,refs/tags/*) is_hash=y; echo "$(git abbrev-ref $i)" ;;
>  		n,*) is_hash=y; echo "$i" ;;
>  		esac
>  	done
> @@ -199,9 +199,9 @@ __git_refs ()
>  		case "$is_hash,$i" in
>  		y,*) is_hash=n ;;
>  		n,*^{}) is_hash=y ;;
> -		n,refs/tags/*) is_hash=y; echo "${i#refs/tags/}" ;;
> -		n,refs/heads/*) is_hash=y; echo "${i#refs/heads/}" ;;
> -		n,refs/remotes/*) is_hash=y; echo "${i#refs/remotes/}" ;;
> +		n,refs/tags/*) is_hash=y; echo "$(git abbrev-ref $i)" ;;
> +		n,refs/heads/*) is_hash=y; echo "$(git abbrev-ref $i)" ;;
> +		n,refs/remotes/*) is_hash=y; echo "$(git abbrev-ref $i)" ;;
>  		n,*) is_hash=y; echo "$i" ;;
>  		esac
>  	done
> @@ -222,7 +222,7 @@ __git_refs_remotes ()
>  		case "$is_hash,$i" in
>  		n,refs/heads/*)
>  			is_hash=y
> -			echo "$i:refs/remotes/$1/${i#refs/heads/}"
> +			echo "$i:refs/remotes/$1/$(git abbrev-ref $i)"
>  			;;
>  		y,*) is_hash=n ;;
>  		n,*^{}) is_hash=y ;;

-- 
Shawn.

Re: [PATCH 3/3] git abbref-ref: new porcelain for abbreviate_ref()

[Junio C Hamano]

"Shawn O. Pearce" <spearce@spearce.org> writes:

> Bert Wesarg <bert.wesarg@googlemail.com> wrote:
>> This gives direct access to the abbreviate_ref() function. The operation
>> mode defaults to the core.warnambiguousrefs value, like the refname:short
>> format, but can be explicitly changed with the --{,no}-strict option.
>> 
>> The bash completion script utilizes this new command.
>
> And it slows down too, doesn't it?  Now we are doing a fork per
> branch during completion.  Yikes.  Didn't you just post a series
> about making completion faster?
>  
>> Junio, if this is not a porcelain, tell me.
>
> IMHO its plumbing.  Porcelain is used by a human.  Plumbing is the
> bits needed to make human interfaces.

Shawn is right.

I wouldn't be taking this patch to add a new command, but I suspect that
this could be an option to rev-parse that is similar to --symbolic.

Teach SHOW_SYMBOLIC_SHORTEST to builtin-rev-parse.c::show_rev(), teach the
parser cmd_rev_parse() a new option --symbolic-abbrev and you are done,
right?

By the way, I found it amusing to see Cc: lines _after_ three dashes to
control send-email --- nice trick I didn't think of ;-)

Re: [PATCH 1/3] for-each-ref: utilize core.warnambiguousrefs for strict refname:short format

[Junio C Hamano]

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

> diff --git a/builtin-for-each-ref.c b/builtin-for-each-ref.c
> index 9b44092..e7b7712 100644
> --- a/builtin-for-each-ref.c
> +++ b/builtin-for-each-ref.c
> @@ -571,7 +571,7 @@ static void gen_scanf_fmt(char *scanf_fmt, const char *rule)
>  /*
>   * Shorten the refname to an non-ambiguous form
>   */
> -static char *get_short_ref(struct refinfo *ref)
> +static void get_short_ref(struct refinfo *ref, int strict, char **short_ref)
>  {
>  	int i;
>  	static char **scanf_fmts;

As far as I can tell this changing of function signature does not help its
existing caller nor the function's implementation.

Why?  And especially why do this in a patch that does something else?

Re: [PATCH 3/3] git abbref-ref: new porcelain for abbreviate_ref()

[Bert Wesarg]

On Mon, Sep 22, 2008 at 17:32, Shawn O. Pearce <spearce@spearce.org> wrote:
> Bert Wesarg <bert.wesarg@googlemail.com> wrote:
>> This gives direct access to the abbreviate_ref() function. The operation
>> mode defaults to the core.warnambiguousrefs value, like the refname:short
>> format, but can be explicitly changed with the --{,no}-strict option.
>>
>> The bash completion script utilizes this new command.
>
> And it slows down too, doesn't it?  Now we are doing a fork per
> branch during completion.  Yikes.  Didn't you just post a series
> about making completion faster?
No, correctness was and is my concern, so this patch fits well into this.

Bert

Re: [PATCH 3/3] git abbref-ref: new porcelain for abbreviate_ref()

[Bert Wesarg]

On Mon, Sep 22, 2008 at 17:55, Junio C Hamano <gitster@pobox.com> wrote:
> "Shawn O. Pearce" <spearce@spearce.org> writes:
>
>> Bert Wesarg <bert.wesarg@googlemail.com> wrote:
>>> This gives direct access to the abbreviate_ref() function. The operation
>>> mode defaults to the core.warnambiguousrefs value, like the refname:short
>>> format, but can be explicitly changed with the --{,no}-strict option.
>>>
>>> The bash completion script utilizes this new command.
>>
>> And it slows down too, doesn't it?  Now we are doing a fork per
>> branch during completion.  Yikes.  Didn't you just post a series
>> about making completion faster?
>>
>>> Junio, if this is not a porcelain, tell me.
>>
>> IMHO its plumbing.  Porcelain is used by a human.  Plumbing is the
>> bits needed to make human interfaces.
>
> Shawn is right.
>
> I wouldn't be taking this patch to add a new command, but I suspect that
> this could be an option to rev-parse that is similar to --symbolic.
>
> Teach SHOW_SYMBOLIC_SHORTEST to builtin-rev-parse.c::show_rev(), teach the
> parser cmd_rev_parse() a new option --symbolic-abbrev and you are done,
> right?
>
You are probably right, that this small functionality could fit into
an existing program.
But I haven't look for one.

> By the way, I found it amusing to see Cc: lines _after_ three dashes to
> control send-email --- nice trick I didn't think of ;-)
The only problem is, that git format-patch outputs '---\n'
unconditionally, so I had to remove the second one by hand.

Bert
>
>
>
>

Re: [PATCH 1/3] for-each-ref: utilize core.warnambiguousrefs for strict refname:short format

[Bert Wesarg]

On Mon, Sep 22, 2008 at 18:27, Junio C Hamano <gitster@pobox.com> wrote:
> Bert Wesarg <bert.wesarg@googlemail.com> writes:
>
>> diff --git a/builtin-for-each-ref.c b/builtin-for-each-ref.c
>> index 9b44092..e7b7712 100644
>> --- a/builtin-for-each-ref.c
>> +++ b/builtin-for-each-ref.c
>> @@ -571,7 +571,7 @@ static void gen_scanf_fmt(char *scanf_fmt, const char *rule)
>>  /*
>>   * Shorten the refname to an non-ambiguous form
>>   */
>> -static char *get_short_ref(struct refinfo *ref)
>> +static void get_short_ref(struct refinfo *ref, int strict, char **short_ref)
>>  {
>>       int i;
>>       static char **scanf_fmts;
>
> As far as I can tell this changing of function signature does not help its
> existing caller nor the function's implementation.
>
> Why?  And especially why do this in a patch that does something else?
Good point.  I think it had something to do that I first want a
reverse of dwim_ref().  And this has this 'char **' argument.  Than I
changed it back to the old signature which makes this line:

-                                       refname = get_short_ref(ref);
+                                       refname = get_short_ref(ref,
warn_ambiguous_refs);

unbreakable or screams for an out factoring of all inside the 'if
(!prefixcmp(name, "refname")) {'

I would prefer the out factory.

Bert

Re: [PATCH 1/3] for-each-ref: utilize core.warnambiguousrefs for strict refname:short format

[Junio C Hamano]

Shawn, was there any issue with this one?  The patch changes the function
signature for no good reason (at least, it does not have anything to do
with the stated purpose of the change), but other than that, I think what
it attempts to do makes sense.

Re: [PATCH 1/3] for-each-ref: utilize core.warnambiguousrefs for strict refname:short format

[Shawn O. Pearce]

Junio C Hamano <gitster@pobox.com> wrote:
> Shawn, was there any issue with this one?  The patch changes the function
> signature for no good reason (at least, it does not have anything to do
> with the stated purpose of the change), but other than that, I think what
> it attempts to do makes sense.

No, aside from the signature issue I think its reasonable.

-- 
Shawn.

Re: [PATCH 1/3] for-each-ref: utilize core.warnambiguousrefs for strict refname:short format

[Bert Wesarg]

On Sat, Oct 18, 2008 at 03:50, Shawn O. Pearce <spearce@spearce.org> wrote:
> Junio C Hamano <gitster@pobox.com> wrote:
>> Shawn, was there any issue with this one?  The patch changes the function
>> signature for no good reason (at least, it does not have anything to do
>> with the stated purpose of the change), but other than that, I think what
>> it attempts to do makes sense.
>
> No, aside from the signature issue I think its reasonable.
Ok, I will post a new single patch, that does only this.

Bert
>
> --
> Shawn.
>

[RFC/PATCH 0/5] making upstream branch information accessible

[Jeff King]

Here are slightly more cleaned-up versions of patches I've thrown out in
the last few days. The aim is to make the information on
upstream/tracking relationships more accessible both to scripts and to
users.

  1/5: for-each-ref: refactor get_short_ref function
  2/5: for-each-ref: refactor refname handling

    Cleanup for 3/5.

  3/5: for-each-ref: add "upstream" format field

    Plumbing support.

  4/5: make get_short_ref a public function

    Clean up for 4/5. Builds on 1/5.

  5/5: branch: show upstream branch when double verbose

-Peff

[PATCH 1/5] for-each-ref: refactor get_short_ref function

[Jeff King]

This function took a "refinfo" object which is unnecessarily
restrictive; it only ever looked at the refname field. This
patch refactors it to take just the ref name as a string.

While we're touching the relevant lines, let's give it
consistent memory semantics. Previously, some code paths
would return an allocated string and some would return the
original string; now it will always return a malloc'd
string.

This doesn't actually fix a bug or a leak, because
for-each-ref doesn't clean up its memory, but it makes the
function a lot less surprising for reuse (which will
happen in a later patch).

Signed-off-by: Jeff King <peff@peff.net>
---
Actually, as the modified version is always prefix-shortened,
it should be possible to rewrite this in a way that never allocates
memory. I picked the least-invasive and most lazy approach.

 builtin-for-each-ref.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/builtin-for-each-ref.c b/builtin-for-each-ref.c
index 5cbb4b0..4aaf75c 100644
--- a/builtin-for-each-ref.c
+++ b/builtin-for-each-ref.c
@@ -569,7 +569,7 @@ static void gen_scanf_fmt(char *scanf_fmt, const char *rule)
 /*
  * Shorten the refname to an non-ambiguous form
  */
-static char *get_short_ref(struct refinfo *ref)
+static char *get_short_ref(const char *ref)
 {
 	int i;
 	static char **scanf_fmts;
@@ -598,17 +598,17 @@ static char *get_short_ref(struct refinfo *ref)
 
 	/* bail out if there are no rules */
 	if (!nr_rules)
-		return ref->refname;
+		return xstrdup(ref);
 
-	/* buffer for scanf result, at most ref->refname must fit */
-	short_name = xstrdup(ref->refname);
+	/* buffer for scanf result, at most ref must fit */
+	short_name = xstrdup(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))
+		if (1 != sscanf(ref, scanf_fmts[i], short_name))
 			continue;
 
 		short_name_len = strlen(short_name);
@@ -642,7 +642,7 @@ static char *get_short_ref(struct refinfo *ref)
 	}
 
 	free(short_name);
-	return ref->refname;
+	return xstrdup(ref);
 }
 
 
@@ -684,7 +684,7 @@ static void populate_value(struct refinfo *ref)
 			if (formatp) {
 				formatp++;
 				if (!strcmp(formatp, "short"))
-					refname = get_short_ref(ref);
+					refname = get_short_ref(ref->refname);
 				else
 					die("unknown refname format %s",
 					    formatp);
-- 
1.6.2.2.450.gd6aa9.dirty

[PATCH 2/5] for-each-ref: refactor refname handling

[Jeff King]

This code handles some special magic like *-deref and the
:short formatting specifier. The next patch will add another
field which outputs a ref and wants to use the same code.

This patch splits the "which ref are we outputting" from the
actual formatting. There should be no behavioral change.

Signed-off-by: Jeff King <peff@peff.net>
---
The diff is scary, but it is mostly reindentation.

 builtin-for-each-ref.c |   47 ++++++++++++++++++++++++++---------------------
 1 files changed, 26 insertions(+), 21 deletions(-)

diff --git a/builtin-for-each-ref.c b/builtin-for-each-ref.c
index 4aaf75c..b50c93b 100644
--- a/builtin-for-each-ref.c
+++ b/builtin-for-each-ref.c
@@ -672,32 +672,37 @@ static void populate_value(struct refinfo *ref)
 		const char *name = used_atom[i];
 		struct atom_value *v = &ref->value[i];
 		int deref = 0;
+		const char *refname;
+		const char *formatp;
+
 		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->refname);
-				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;
-			}
+		if (!prefixcmp(name, "refname"))
+			refname = ref->refname;
+		else
+			continue;
+
+		formatp = strchr(name, ':');
+		/* look for "short" refname format */
+		if (formatp) {
+			formatp++;
+			if (!strcmp(formatp, "short"))
+				refname = get_short_ref(refname);
+			else
+				die("unknown %.*s format %s",
+					formatp - name, name, formatp);
+		}
+
+		if (!deref)
+			v->s = refname;
+		else {
+			int len = strlen(refname);
+			char *s = xmalloc(len + 4);
+			sprintf(s, "%s^{}", refname);
+			v->s = s;
 		}
 	}
 
-- 
1.6.2.2.450.gd6aa9.dirty

[PATCH 3/5] for-each-ref: add "upstream" format field

[Jeff King]

The logic for determining the upstream ref of a branch is
somewhat complex to perform in a shell script. This patch
provides a plumbing mechanism for scripts to access the C
logic used internally by git-status, git-branch, etc.

For example:

  $ git for-each-ref \
       --format='%(refname:short) %(upstream:short)' \
       refs/heads/
  master origin/master

Signed-off-by: Jeff King <peff@peff.net>
---
This is a cleaned-up version of what I sent previously. Mainly just code
cleanups, and it no longer frees the branch struct, which seems to be
allocated from semi-permanent storage during branch_get.

Should the documentation explain the concept of "upstream" more fully? I
noticed Santi sent a glossary patch earlier today, so maybe that is
enough.

 Documentation/git-for-each-ref.txt |    5 +++++
 builtin-for-each-ref.c             |   14 ++++++++++++++
 t/t6300-for-each-ref.sh            |   22 ++++++++++++++++++++++
 3 files changed, 41 insertions(+), 0 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 5061d3e..b362e9e 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -85,6 +85,11 @@ objectsize::
 objectname::
 	The object name (aka SHA-1).
 
+upstream::
+	The name of a local ref which can be considered ``upstream''
+	from the displayed ref. Respects `:short` in the same way as
+	`refname` above.
+
 In addition to the above, for commit and tag objects, the header
 field names (`tree`, `parent`, `object`, `type`, and `tag`) can
 be used to specify the value in the header field.
diff --git a/builtin-for-each-ref.c b/builtin-for-each-ref.c
index b50c93b..277d1fb 100644
--- a/builtin-for-each-ref.c
+++ b/builtin-for-each-ref.c
@@ -8,6 +8,7 @@
 #include "blob.h"
 #include "quote.h"
 #include "parse-options.h"
+#include "remote.h"
 
 /* Quoting styles */
 #define QUOTE_NONE 0
@@ -66,6 +67,7 @@ static struct {
 	{ "subject" },
 	{ "body" },
 	{ "contents" },
+	{ "upstream" },
 };
 
 /*
@@ -682,6 +684,18 @@ static void populate_value(struct refinfo *ref)
 
 		if (!prefixcmp(name, "refname"))
 			refname = ref->refname;
+		else if(!prefixcmp(name, "upstream")) {
+			struct branch *branch;
+			/* only local branches may have an upstream */
+			if (prefixcmp(ref->refname, "refs/heads/"))
+				continue;
+			branch = branch_get(ref->refname + 11);
+
+			if (!branch || !branch->merge || !branch->merge[0] ||
+			    !branch->merge[0]->dst)
+				continue;
+			refname = branch->merge[0]->dst;
+		}
 		else
 			continue;
 
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 8bfae44..daf02d5 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -26,6 +26,13 @@ test_expect_success 'Create sample commit with known timestamp' '
 	git tag -a -m "Tagging at $datestamp" testtag
 '
 
+test_expect_success 'Create upstream config' '
+	git update-ref refs/remotes/origin/master master &&
+	git remote add origin nowhere &&
+	git config branch.master.remote origin &&
+	git config branch.master.merge refs/heads/master
+'
+
 test_atom() {
 	case "$1" in
 		head) ref=refs/heads/master ;;
@@ -39,6 +46,7 @@ test_atom() {
 }
 
 test_atom head refname refs/heads/master
+test_atom head upstream refs/remotes/origin/master
 test_atom head objecttype commit
 test_atom head objectsize 171
 test_atom head objectname 67a36f10722846e891fbada1ba48ed035de75581
@@ -68,6 +76,7 @@ test_atom head contents 'Initial
 '
 
 test_atom tag refname refs/tags/testtag
+test_atom tag upstream ''
 test_atom tag objecttype tag
 test_atom tag objectsize 154
 test_atom tag objectname 98b46b1d36e5b07909de1b3886224e3e81e87322
@@ -203,6 +212,7 @@ test_expect_success 'Check format "rfc2822" date fields output' '
 
 cat >expected <<\EOF
 refs/heads/master
+refs/remotes/origin/master
 refs/tags/testtag
 EOF
 
@@ -214,6 +224,7 @@ test_expect_success 'Verify ascending sort' '
 
 cat >expected <<\EOF
 refs/tags/testtag
+refs/remotes/origin/master
 refs/heads/master
 EOF
 
@@ -224,6 +235,7 @@ test_expect_success 'Verify descending sort' '
 
 cat >expected <<\EOF
 'refs/heads/master'
+'refs/remotes/origin/master'
 'refs/tags/testtag'
 EOF
 
@@ -244,6 +256,7 @@ test_expect_success 'Quoting style: python' '
 
 cat >expected <<\EOF
 "refs/heads/master"
+"refs/remotes/origin/master"
 "refs/tags/testtag"
 EOF
 
@@ -273,6 +286,15 @@ test_expect_success 'Check short refname format' '
 	test_cmp expected actual
 '
 
+cat >expected <<EOF
+origin/master
+EOF
+
+test_expect_success 'Check short upstream format' '
+	git for-each-ref --format="%(upstream:short)" refs/heads >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'Check for invalid refname format' '
 	test_must_fail git for-each-ref --format="%(refname:INVALID)"
 '
-- 
1.6.2.2.450.gd6aa9.dirty

[PATCH 4/5] make get_short_ref a public function

[Jeff King]

Often we want to shorten a full ref name to something "prettier"
to show a user. For example, "refs/heads/master" is often shown
simply as "master", or "refs/remotes/origin/master" is shown as
"origin/master".

Many places in the code use a very simple formula: skip common
prefixes like refs/heads, refs/remotes, etc. This is codified in
the prettify_ref function.

for-each-ref has a more correct (but more expensive) approach:
consider the ref lookup rules, and try shortening as much as
possible while remaining unambiguous.

This patch makes the latter strategy globally available as
shorten_unambiguous_ref.

Signed-off-by: Jeff King <peff@peff.net>
---
Actually, I am not quite sure that this function is "more correct". It
looks at the rev-parsing rules as a hierarchy, so if you have
"refs/remotes/foo" and "refs/heads/foo", then it will abbreviate the
first to "remotes/foo" (as expected) and the latter to just "foo".

This is technically correct, as "refs/heads/foo" will be selected by
"foo", but it will warn about ambiguity. Should we actually try to avoid
reporting refs which would be ambiguous?

Should this simply replace prettify_ref (and other places which should
be using prettify_ref but aren't)? It is definitely more expensive, as
it has to resolve refs to look for ambiguities, but I don't know if we
care in most code paths.

 builtin-for-each-ref.c |  105 +-----------------------------------------------
 refs.c                 |   99 +++++++++++++++++++++++++++++++++++++++++++++
 refs.h                 |    1 +
 3 files changed, 101 insertions(+), 104 deletions(-)

diff --git a/builtin-for-each-ref.c b/builtin-for-each-ref.c
index 277d1fb..8c82484 100644
--- a/builtin-for-each-ref.c
+++ b/builtin-for-each-ref.c
@@ -546,109 +546,6 @@ 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(const char *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 xstrdup(ref);
-
-	/* buffer for scanf result, at most ref must fit */
-	short_name = xstrdup(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, 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];
-			char refname[PATH_MAX];
-
-			/*
-			 * the short name is ambiguous, if it resolves
-			 * (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 (!read_ref(refname, 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 xstrdup(ref);
-}
-
-
-/*
  * Parse the object referred by ref, and grab needed value.
  */
 static void populate_value(struct refinfo *ref)
@@ -704,7 +601,7 @@ static void populate_value(struct refinfo *ref)
 		if (formatp) {
 			formatp++;
 			if (!strcmp(formatp, "short"))
-				refname = get_short_ref(refname);
+				refname = shorten_unambiguous_ref(refname);
 			else
 				die("unknown %.*s format %s",
 					formatp - name, name, formatp);
diff --git a/refs.c b/refs.c
index 59c373f..1e5e7b4 100644
--- a/refs.c
+++ b/refs.c
@@ -1652,3 +1652,102 @@ struct ref *find_ref_by_name(const struct ref *list, const char *name)
 			return (struct ref *)list;
 	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)
+{
+	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;
+}
+
+char *shorten_unambiguous_ref(const char *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 xstrdup(ref);
+
+	/* buffer for scanf result, at most ref must fit */
+	short_name = xstrdup(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, 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];
+			char refname[PATH_MAX];
+
+			/*
+			 * the short name is ambiguous, if it resolves
+			 * (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 (!read_ref(refname, 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 xstrdup(ref);
+}
diff --git a/refs.h b/refs.h
index 68c2d16..2d0f961 100644
--- a/refs.h
+++ b/refs.h
@@ -80,6 +80,7 @@ extern int for_each_reflog(each_ref_fn, void *);
 extern int check_ref_format(const char *target);
 
 extern const char *prettify_ref(const struct ref *ref);
+extern char *shorten_unambiguous_ref(const char *ref);
 
 /** rename ref, return 0 on success **/
 extern int rename_ref(const char *oldref, const char *newref, const char *logmsg);
-- 
1.6.2.2.450.gd6aa9.dirty

[PATCH 5/5] branch: show upstream branch when double verbose

[Jeff King]

This information is easily accessible when we are
calculating the relationship. The only reason not to print
it all the time is that it consumes a fair bit of screen
space, and may not be of interest to the user.

Signed-off-by: Jeff King <peff@peff.net>
---
This one is very RFC. Should this information be part of the regular
"-v"? Should it be part of "git branch" with regular verbosity?

Should the format be different? I wonder if

  master 1234abcd [origin/master: ahead 5, behind 6] whatever

will be interpreted as "origin/master is ahead 5, behind 6" when it is
really the reverse. Maybe "[ahead 5, behind 6 from origin/master]" would
be better?

 Documentation/git-branch.txt |    4 +++-
 builtin-branch.c             |   23 +++++++++++++++++------
 2 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index 31ba7f2..ba3dea6 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -100,7 +100,9 @@ OPTIONS
 
 -v::
 --verbose::
-	Show sha1 and commit subject line for each head.
+	Show sha1 and commit subject line for each head, along with
+	relationship to upstream branch (if any). If given twice, print
+	the name of the upstream branch, as well.
 
 --abbrev=<length>::
 	Alter the sha1's minimum display length in the output listing.
diff --git a/builtin-branch.c b/builtin-branch.c
index ca81d72..3275821 100644
--- a/builtin-branch.c
+++ b/builtin-branch.c
@@ -301,19 +301,30 @@ static int ref_cmp(const void *r1, const void *r2)
 	return strcmp(c1->name, c2->name);
 }
 
-static void fill_tracking_info(struct strbuf *stat, const char *branch_name)
+static void fill_tracking_info(struct strbuf *stat, const char *branch_name,
+		int show_upstream_ref)
 {
 	int ours, theirs;
 	struct branch *branch = branch_get(branch_name);
 
-	if (!stat_tracking_info(branch, &ours, &theirs) || (!ours && !theirs))
+	if (!stat_tracking_info(branch, &ours, &theirs)) {
+		if (branch && branch->merge && branch->merge[0]->dst &&
+		    show_upstream_ref)
+			strbuf_addf(stat, "[%s] ",
+			    shorten_unambiguous_ref(branch->merge[0]->dst));
 		return;
+	}
+
+	strbuf_addch(stat, '[');
+	if (show_upstream_ref)
+		strbuf_addf(stat, "%s: ",
+			shorten_unambiguous_ref(branch->merge[0]->dst));
 	if (!ours)
-		strbuf_addf(stat, "[behind %d] ", theirs);
+		strbuf_addf(stat, "behind %d] ", theirs);
 	else if (!theirs)
-		strbuf_addf(stat, "[ahead %d] ", ours);
+		strbuf_addf(stat, "ahead %d] ", ours);
 	else
-		strbuf_addf(stat, "[ahead %d, behind %d] ", ours, theirs);
+		strbuf_addf(stat, "ahead %d, behind %d] ", ours, theirs);
 }
 
 static int matches_merge_filter(struct commit *commit)
@@ -379,7 +390,7 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
 		}
 
 		if (item->kind == REF_LOCAL_BRANCH)
-			fill_tracking_info(&stat, item->name);
+			fill_tracking_info(&stat, item->name, verbose > 1);
 
 		strbuf_addf(&out, " %s %s%s",
 			find_unique_abbrev(item->commit->object.sha1, abbrev),
-- 
1.6.2.2.450.gd6aa9.dirty

[PATCH] for-each-ref: remove multiple xstrdup() in get_short_ref()

[Bert Wesarg]

Now that get_short_ref() always return an malloced string, consolidate to
one xstrcpy() call.

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

---
Also an 

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

for Jeffs patch.

 builtin-for-each-ref.c |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/builtin-for-each-ref.c b/builtin-for-each-ref.c
index 4aaf75c..108c128 100644
--- a/builtin-for-each-ref.c
+++ b/builtin-for-each-ref.c
@@ -596,13 +596,13 @@ static char *get_short_ref(const char *ref)
 		}
 	}
 
-	/* bail out if there are no rules */
-	if (!nr_rules)
-		return xstrdup(ref);
-
 	/* buffer for scanf result, at most ref must fit */
 	short_name = xstrdup(ref);
 
+	/* bail out if there are no rules */
+	if (!nr_rules)
+		return short_name;
+
 	/* skip first rule, it will always match */
 	for (i = nr_rules - 1; i > 0 ; --i) {
 		int j;
@@ -641,8 +641,7 @@ static char *get_short_ref(const char *ref)
 			return short_name;
 	}
 
-	free(short_name);
-	return xstrdup(ref);
+	return strcpy(short_name, ref);
 }
 
 
-- 
tg: (e9786d7..) bw/ammend-jk/refactor-get_short_ref (depends on: jk/refactor-get_short_ref)

Re: [PATCH 4/5] make get_short_ref a public function

[Bert Wesarg]

On Tue, Apr 7, 2009 at 09:14, Jeff King <peff@peff.net> wrote:
> Often we want to shorten a full ref name to something "prettier"
> to show a user. For example, "refs/heads/master" is often shown
> simply as "master", or "refs/remotes/origin/master" is shown as
> "origin/master".
>
> Many places in the code use a very simple formula: skip common
> prefixes like refs/heads, refs/remotes, etc. This is codified in
> the prettify_ref function.
>
> for-each-ref has a more correct (but more expensive) approach:
> consider the ref lookup rules, and try shortening as much as
> possible while remaining unambiguous.
>
> This patch makes the latter strategy globally available as
> shorten_unambiguous_ref.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> Actually, I am not quite sure that this function is "more correct". It
> looks at the rev-parsing rules as a hierarchy, so if you have
> "refs/remotes/foo" and "refs/heads/foo", then it will abbreviate the
> first to "remotes/foo" (as expected) and the latter to just "foo".
>
> This is technically correct, as "refs/heads/foo" will be selected by
> "foo", but it will warn about ambiguity. Should we actually try to avoid
> reporting refs which would be ambiguous?
Back than, there was the idea that the core.warnAmbiguousRefs config
could be used for this.

Anyway

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

Re: [PATCH] for-each-ref: remove multiple xstrdup() in get_short_ref()

[Jeff King]

On Tue, Apr 07, 2009 at 09:33:19AM +0200, Bert Wesarg wrote:

> Now that get_short_ref() always return an malloced string, consolidate to
> one xstrcpy() call.

Makes sense to squash in on top of what I have. But I think it actually
is pretty easy to always return a pointer into the existing string
(patch based on current master):

---
diff --git a/builtin-for-each-ref.c b/builtin-for-each-ref.c
index 5cbb4b0..8b24a4a 100644
--- a/builtin-for-each-ref.c
+++ b/builtin-for-each-ref.c
@@ -569,7 +569,7 @@ static void gen_scanf_fmt(char *scanf_fmt, const char *rule)
 /*
  * Shorten the refname to an non-ambiguous form
  */
-static char *get_short_ref(struct refinfo *ref)
+static const char *get_short_ref(const char *ref)
 {
 	int i;
 	static char **scanf_fmts;
@@ -598,17 +598,17 @@ static char *get_short_ref(struct refinfo *ref)
 
 	/* bail out if there are no rules */
 	if (!nr_rules)
-		return ref->refname;
+		return ref;
 
 	/* buffer for scanf result, at most ref->refname must fit */
-	short_name = xstrdup(ref->refname);
+	short_name = xstrdup(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))
+		if (1 != sscanf(ref, scanf_fmts[i], short_name))
 			continue;
 
 		short_name_len = strlen(short_name);
@@ -637,12 +637,14 @@ static char *get_short_ref(struct refinfo *ref)
 		 * short name is non-ambiguous if all previous rules
 		 * haven't resolved to a valid ref
 		 */
-		if (j == i)
-			return short_name;
+		if (j == i) {
+			ref += strlen(ref) - strlen(short_name);
+			break;
+		}
 	}
 
 	free(short_name);
-	return ref->refname;
+	return ref;
 }
 
 
@@ -684,7 +686,7 @@ static void populate_value(struct refinfo *ref)
 			if (formatp) {
 				formatp++;
 				if (!strcmp(formatp, "short"))
-					refname = get_short_ref(ref);
+					refname = get_short_ref(ref->refname);
 				else
 					die("unknown refname format %s",
 					    formatp);

Re: [PATCH] for-each-ref: remove multiple xstrdup() in get_short_ref()

[Bert Wesarg]

On Tue, Apr 7, 2009 at 09:33, Bert Wesarg <bert.wesarg@googlemail.com> wrote:
> Now that get_short_ref() always return an malloced string, consolidate to
> one xstrcpy() call.
>
> Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
>
> ---
> Also an
>
> Acked-by: Bert Wesarg <bert.wesarg@googlemail.com>
Sorry, wrong Message-ID for In-Reply-To, this Ack is for:

[PATCH 1/5] for-each-ref: refactor get_short_ref function

with Message-ID <20090407070501.GA2924@coredump.intra.peff.net>

Bert

Re: [PATCH] for-each-ref: remove multiple xstrdup() in get_short_ref()

[Bert Wesarg]

On Tue, Apr 7, 2009 at 09:44, Jeff King <peff@peff.net> wrote:
> On Tue, Apr 07, 2009 at 09:33:19AM +0200, Bert Wesarg wrote:
>
>> Now that get_short_ref() always return an malloced string, consolidate to
>> one xstrcpy() call.
>
> Makes sense to squash in on top of what I have. But I think it actually
> is pretty easy to always return a pointer into the existing string
> (patch based on current master):
Yes, thats probably a good idea. The caller can always do a
xstrdup(get_short_ref(ref)).

> @@ -637,12 +637,14 @@ static char *get_short_ref(struct refinfo *ref)
>                 * short name is non-ambiguous if all previous rules
>                 * haven't resolved to a valid ref
>                 */
> -               if (j == i)
> -                       return short_name;
> +               if (j == i) {
> +                       ref += strlen(ref) - strlen(short_name);
we have strlen(short_name) in short_name_len already.

Bert

Re: [PATCH 4/5] make get_short_ref a public function

[Michael J Gruber]

Jeff King venit, vidit, dixit 07.04.2009 09:14:
> Often we want to shorten a full ref name to something "prettier"
> to show a user. For example, "refs/heads/master" is often shown
> simply as "master", or "refs/remotes/origin/master" is shown as
> "origin/master".
> 
> Many places in the code use a very simple formula: skip common
> prefixes like refs/heads, refs/remotes, etc. This is codified in
> the prettify_ref function.
> 
> for-each-ref has a more correct (but more expensive) approach:
> consider the ref lookup rules, and try shortening as much as
> possible while remaining unambiguous.
> 
> This patch makes the latter strategy globally available as
> shorten_unambiguous_ref.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> Actually, I am not quite sure that this function is "more correct". It
> looks at the rev-parsing rules as a hierarchy, so if you have
> "refs/remotes/foo" and "refs/heads/foo", then it will abbreviate the
> first to "remotes/foo" (as expected) and the latter to just "foo".
> 
> This is technically correct, as "refs/heads/foo" will be selected by
> "foo", but it will warn about ambiguity. Should we actually try to avoid
> reporting refs which would be ambiguous?
> 
> Should this simply replace prettify_ref (and other places which should
> be using prettify_ref but aren't)? It is definitely more expensive, as
> it has to resolve refs to look for ambiguities, but I don't know if we
> care in most code paths.

I would think that as long as the default is to warn about ambiguous
refs we should not generate ambiguous refs...

Other than that it's very nice, it can be used in many places.

> 
>  builtin-for-each-ref.c |  105 +-----------------------------------------------
>  refs.c                 |   99 +++++++++++++++++++++++++++++++++++++++++++++
>  refs.h                 |    1 +
>  3 files changed, 101 insertions(+), 104 deletions(-)
> 
> diff --git a/builtin-for-each-ref.c b/builtin-for-each-ref.c
> index 277d1fb..8c82484 100644
> --- a/builtin-for-each-ref.c
> +++ b/builtin-for-each-ref.c
> @@ -546,109 +546,6 @@ 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(const char *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 xstrdup(ref);
> -
> -	/* buffer for scanf result, at most ref must fit */
> -	short_name = xstrdup(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, 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];
> -			char refname[PATH_MAX];
> -
> -			/*
> -			 * the short name is ambiguous, if it resolves
> -			 * (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 (!read_ref(refname, 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 xstrdup(ref);
> -}
> -
> -
> -/*
>   * Parse the object referred by ref, and grab needed value.
>   */
>  static void populate_value(struct refinfo *ref)
> @@ -704,7 +601,7 @@ static void populate_value(struct refinfo *ref)
>  		if (formatp) {
>  			formatp++;
>  			if (!strcmp(formatp, "short"))
> -				refname = get_short_ref(refname);
> +				refname = shorten_unambiguous_ref(refname);
>  			else
>  				die("unknown %.*s format %s",
>  					formatp - name, name, formatp);
> diff --git a/refs.c b/refs.c
> index 59c373f..1e5e7b4 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1652,3 +1652,102 @@ struct ref *find_ref_by_name(const struct ref *list, const char *name)
>  			return (struct ref *)list;
>  	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)
> +{
> +	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;
> +}
> +
> +char *shorten_unambiguous_ref(const char *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 xstrdup(ref);
> +
> +	/* buffer for scanf result, at most ref must fit */
> +	short_name = xstrdup(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, 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];
> +			char refname[PATH_MAX];
> +
> +			/*
> +			 * the short name is ambiguous, if it resolves
> +			 * (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 (!read_ref(refname, 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 xstrdup(ref);
> +}
> diff --git a/refs.h b/refs.h
> index 68c2d16..2d0f961 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -80,6 +80,7 @@ extern int for_each_reflog(each_ref_fn, void *);
>  extern int check_ref_format(const char *target);
>  
>  extern const char *prettify_ref(const struct ref *ref);
> +extern char *shorten_unambiguous_ref(const char *ref);
>  
>  /** rename ref, return 0 on success **/
>  extern int rename_ref(const char *oldref, const char *newref, const char *logmsg);

Re: [PATCH 5/5] branch: show upstream branch when double verbose

[Michael J Gruber]

Jeff King venit, vidit, dixit 07.04.2009 09:16:
> This information is easily accessible when we are
> calculating the relationship. The only reason not to print
> it all the time is that it consumes a fair bit of screen
> space, and may not be of interest to the user.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> This one is very RFC. Should this information be part of the regular
> "-v"? Should it be part of "git branch" with regular verbosity?
> 
> Should the format be different? I wonder if
> 
>   master 1234abcd [origin/master: ahead 5, behind 6] whatever
> 
> will be interpreted as "origin/master is ahead 5, behind 6" when it is
> really the reverse. Maybe "[ahead 5, behind 6 from origin/master]" would
> be better?

Maybe [origin/master +5 -6]? That should be short enough for sticking it
into -v. We could even use [origin/master +0 -0] for an up-to-date
branch then.

In any case, I think often one is interested in one branch only. I would
expect "git branch -v foo" to give me the -v info just for branch foo.
Currently it does not. But that would be an independent patch on top.

>  Documentation/git-branch.txt |    4 +++-
>  builtin-branch.c             |   23 +++++++++++++++++------
>  2 files changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
> index 31ba7f2..ba3dea6 100644
> --- a/Documentation/git-branch.txt
> +++ b/Documentation/git-branch.txt
> @@ -100,7 +100,9 @@ OPTIONS
>  
>  -v::
>  --verbose::
> -	Show sha1 and commit subject line for each head.
> +	Show sha1 and commit subject line for each head, along with
> +	relationship to upstream branch (if any). If given twice, print
> +	the name of the upstream branch, as well.
>  
>  --abbrev=<length>::
>  	Alter the sha1's minimum display length in the output listing.
> diff --git a/builtin-branch.c b/builtin-branch.c
> index ca81d72..3275821 100644
> --- a/builtin-branch.c
> +++ b/builtin-branch.c
> @@ -301,19 +301,30 @@ static int ref_cmp(const void *r1, const void *r2)
>  	return strcmp(c1->name, c2->name);
>  }
>  
> -static void fill_tracking_info(struct strbuf *stat, const char *branch_name)
> +static void fill_tracking_info(struct strbuf *stat, const char *branch_name,
> +		int show_upstream_ref)
>  {
>  	int ours, theirs;
>  	struct branch *branch = branch_get(branch_name);
>  
> -	if (!stat_tracking_info(branch, &ours, &theirs) || (!ours && !theirs))
> +	if (!stat_tracking_info(branch, &ours, &theirs)) {
> +		if (branch && branch->merge && branch->merge[0]->dst &&
> +		    show_upstream_ref)
> +			strbuf_addf(stat, "[%s] ",
> +			    shorten_unambiguous_ref(branch->merge[0]->dst));
>  		return;
> +	}
> +
> +	strbuf_addch(stat, '[');
> +	if (show_upstream_ref)
> +		strbuf_addf(stat, "%s: ",
> +			shorten_unambiguous_ref(branch->merge[0]->dst));
>  	if (!ours)
> -		strbuf_addf(stat, "[behind %d] ", theirs);
> +		strbuf_addf(stat, "behind %d] ", theirs);
>  	else if (!theirs)
> -		strbuf_addf(stat, "[ahead %d] ", ours);
> +		strbuf_addf(stat, "ahead %d] ", ours);
>  	else
> -		strbuf_addf(stat, "[ahead %d, behind %d] ", ours, theirs);
> +		strbuf_addf(stat, "ahead %d, behind %d] ", ours, theirs);
>  }
>  
>  static int matches_merge_filter(struct commit *commit)
> @@ -379,7 +390,7 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
>  		}
>  
>  		if (item->kind == REF_LOCAL_BRANCH)
> -			fill_tracking_info(&stat, item->name);
> +			fill_tracking_info(&stat, item->name, verbose > 1);
>  
>  		strbuf_addf(&out, " %s %s%s",
>  			find_unique_abbrev(item->commit->object.sha1, abbrev),

Re: [PATCH 5/5] branch: show upstream branch when double verbose

[Paolo Ciarrocchi]

On Tue, Apr 7, 2009 at 9:16 AM, Jeff King <peff@peff.net> wrote:
> This information is easily accessible when we are
> calculating the relationship. The only reason not to print
> it all the time is that it consumes a fair bit of screen
> space, and may not be of interest to the user.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> This one is very RFC. Should this information be part of the regular
> "-v"? Should it be part of "git branch" with regular verbosity?
>
> Should the format be different? I wonder if
>
>  master 1234abcd [origin/master: ahead 5, behind 6] whatever
>
> will be interpreted as "origin/master is ahead 5, behind 6" when it is
> really the reverse. Maybe "[ahead 5, behind 6 from origin/master]" would
> be better?

Yes I think so.
Thanks a lot for your work!

-Paolo

Re: [PATCH] for-each-ref: remove multiple xstrdup() in get_short_ref()

[Jeff King]

On Tue, Apr 07, 2009 at 03:44:35AM -0400, Jeff King wrote:

> +		if (1 != sscanf(ref, scanf_fmts[i], short_name))
> [...]
> +		if (j == i) {
> +			ref += strlen(ref) - strlen(short_name);
> +			break;
> +		}

Actually, I am not sure this is correct, either. It is making the
assumption that the short_name is always a suffix of the ref. But one of
the rev_parse_rules is:

     "refs/remotes/%.*s/HEAD"

for which this is not true. _But_ as it happens, this rule doesn't
actually work in reverse because of the way scanf works with %s. The
code:

    sscanf("refs/remotes/origin/HEAD", "refs/remotes/%s/HEAD", buf);

will put "origin/HEAD" in buf, not "origin"; %s eats until it sees
whitespace (which should not be occuring in a ref, fortunately).

So it actually _is_ correct to assume with the current code that the
short_name is always a suffix, but I am not sure if that is what we
actually want. We will always see "$remote/HEAD" instead of "$remote".

Part of me actually thinks the "incorrect" behavior we are doing now is
actually more explicit and readable. But if that is the case, we should
perhaps simply be excluding that final rule explicitly, and then our
"suffix" assumption will hold.

-Peff

Re: [PATCH 2/5] for-each-ref: refactor refname handling

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> This code handles some special magic like *-deref and the
> :short formatting specifier. The next patch will add another
> field which outputs a ref and wants to use the same code.
>
> This patch splits the "which ref are we outputting" from the
> actual formatting. There should be no behavioral change.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> The diff is scary, but it is mostly reindentation.

... and an introduction of a bug ;-)

>  builtin-for-each-ref.c |   47 ++++++++++++++++++++++++++---------------------
>  1 files changed, 26 insertions(+), 21 deletions(-)
>
> diff --git a/builtin-for-each-ref.c b/builtin-for-each-ref.c
> index 4aaf75c..b50c93b 100644
> --- a/builtin-for-each-ref.c
> +++ b/builtin-for-each-ref.c
> @@ -672,32 +672,37 @@ static void populate_value(struct refinfo *ref)
> ...
> +		/* look for "short" refname format */
> +		if (formatp) {
> +			formatp++;
> +			if (!strcmp(formatp, "short"))
> +				refname = get_short_ref(refname);
> +			else
> +				die("unknown %.*s format %s",
> +					formatp - name, name, formatp);

				die("unknown %.*s format %s",
                                    (int)(formatp - name), name, formatp);

Re: [PATCH 2/5] for-each-ref: refactor refname handling

[Jeff King]

On Tue, Apr 07, 2009 at 11:22:51PM -0700, Junio C Hamano wrote:

> > The diff is scary, but it is mostly reindentation.
> ... and an introduction of a bug ;-)

Oops. 

> > +				die("unknown %.*s format %s",
> > +					formatp - name, name, formatp);
> 				die("unknown %.*s format %s",
>                                     (int)(formatp - name), name, formatp);

Hey, it's all 32 bits, right? ;)

Thanks for spotting it.

-Peff

Re: [PATCH 4/5] make get_short_ref a public function

[Jeff King]

On Tue, Apr 07, 2009 at 09:39:58AM +0200, Bert Wesarg wrote:

> > Actually, I am not quite sure that this function is "more correct". It
> > looks at the rev-parsing rules as a hierarchy, so if you have
> > "refs/remotes/foo" and "refs/heads/foo", then it will abbreviate the
> > first to "remotes/foo" (as expected) and the latter to just "foo".
> >
> > This is technically correct, as "refs/heads/foo" will be selected by
> > "foo", but it will warn about ambiguity. Should we actually try to avoid
> > reporting refs which would be ambiguous?
> Back than, there was the idea that the core.warnAmbiguousRefs config
> could be used for this.

I'm not quite sure what you mean. Using this function, we may shorten an
unambiguous name to one that will complain if core.warnAmbiguousRefs is
set. So what I'm wondering is if it should use a different algorithm
that produces a shortened ref which will not cause a warning.

E.g., right now if we have:

  refs/heads/master
  refs/remotes/master

showing %(refname:short) gets you:

  master
  remotes/master

but "git show master" will warn about the ambiguous ref (but still show
you the one you want). An alternative would be to show:

  heads/master
  remotes/master

in this case.

-Peff

Re: [PATCH 5/5] branch: show upstream branch when double verbose

[Jeff King]

On Tue, Apr 07, 2009 at 10:02:34AM +0200, Michael J Gruber wrote:

> > will be interpreted as "origin/master is ahead 5, behind 6" when it is
> > really the reverse. Maybe "[ahead 5, behind 6 from origin/master]" would
> > be better?
> 
> Maybe [origin/master +5 -6]? That should be short enough for sticking it
> into -v. We could even use [origin/master +0 -0] for an up-to-date
> branch then.

I am not opposed to that format, but I don't feel strongly. And not many
people are voicing an opinion in this thread (strange, given that it is
an opportunity for bikeshedding :) ). My patches are in next, so I think
I am done on the topic for now. But feel free to submit a followup
patch.

> In any case, I think often one is interested in one branch only. I would
> expect "git branch -v foo" to give me the -v info just for branch foo.
> Currently it does not. But that would be an independent patch on top.

Hmm. I think that is a little counterintuitive because we think of "-v"
as simply "increase verbosity" (because that is what it means in just
about every program). But here you are fundamentally changing the action
that is occurring (from "create branch" to "show branch").  I think it
would make more sense to have a "show branch" mode like:

  git branch -s foo

which would probably have the more detailed output by default.

-Peff

Re: [PATCH 4/5] make get_short_ref a public function

[Bert Wesarg]

On Thu, Apr 9, 2009 at 10:18, Jeff King <peff@peff.net> wrote:
> On Tue, Apr 07, 2009 at 09:39:58AM +0200, Bert Wesarg wrote:
>
>> > Actually, I am not quite sure that this function is "more correct". It
>> > looks at the rev-parsing rules as a hierarchy, so if you have
>> > "refs/remotes/foo" and "refs/heads/foo", then it will abbreviate the
>> > first to "remotes/foo" (as expected) and the latter to just "foo".
>> >
>> > This is technically correct, as "refs/heads/foo" will be selected by
>> > "foo", but it will warn about ambiguity. Should we actually try to avoid
>> > reporting refs which would be ambiguous?
>> Back than, there was the idea that the core.warnAmbiguousRefs config
>> could be used for this.
>
> I'm not quite sure what you mean. Using this function, we may shorten an
> unambiguous name to one that will complain if core.warnAmbiguousRefs is
> set. So what I'm wondering is if it should use a different algorithm
> that produces a shortened ref which will not cause a warning.
>
> E.g., right now if we have:
>
>  refs/heads/master
>  refs/remotes/master
>
> showing %(refname:short) gets you:
>
>  master
>  remotes/master
>
> but "git show master" will warn about the ambiguous ref (but still show
> you the one you want). An alternative would be to show:
>
>  heads/master
>  remotes/master
>
> in this case.
Right, and the idea was to choose the alternatives based on the
core.warnAmbiguousRefs setting, i.e. the former for false, the latter
for true.

For what I posted a patch some time ago:

http://thread.gmane.org/gmane.comp.version-control.git/96464

(which I read though now)

Bert
>
> -Peff
>

Re: [PATCH 5/5] branch: show upstream branch when double verbose

[Santi Béjar]

2009/4/9 Jeff King <peff@peff.net>:
> On Tue, Apr 07, 2009 at 10:02:34AM +0200, Michael J Gruber wrote:
>
>> > will be interpreted as "origin/master is ahead 5, behind 6" when it is
>> > really the reverse. Maybe "[ahead 5, behind 6 from origin/master]" would
>> > be better?
>>
>> Maybe [origin/master +5 -6]? That should be short enough for sticking it
>> into -v. We could even use [origin/master +0 -0] for an up-to-date
>> branch then.
>
> I am not opposed to that format, but I don't feel strongly. And not many
> people are voicing an opinion in this thread (strange, given that it is
> an opportunity for bikeshedding :) ).

I've been thinking about this and both formats seems OK for me,
although using the +5 -6 format for just -v seems a good point.

Just to bikeshed a bit more :) we could use a format more similar to
the "git fetch" output, like:

  next         c4628f8 [4...6 origin/next] Merge branch 'jk/no-perl' into next
  next         c4628f8 [4.. origin/next] Merge branch 'jk/no-perl' into next
  next         c4628f8 [..6 origin/next] Merge branch 'jk/no-perl' into next

(three dots when they have diverged and two otherwise)

It can suggest that the left number you know it is about things in
next and the right number about things in origin/next. The problem is
that is also looks like revisions.

Just my 2cents, but feel free to ignore ;-)
Santi

[PATCH&RFC] get_short_ref(): add strict mode

[Bert Wesarg]

Add the strict mode of abbreviation to get_short_ref(), i.e. the resulting ref
won't trigger the ambiguous ref warning.

The only user of this function ("refname:short") still uses the loose mode.

Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
---
Cc: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org

I think of 3 alternatives to use this mode for the "refname" format (and
probably others):

  a) Use core.warnAmbiguousRefs to control strict mode.
     This would change the current default behaviour, because this is true
     by default.

  b) Introduce a new core config variable to control this, either for
     for-each-ref alone ore globally.

  c) Introduce a "refname:short-strict" format to get the strict abbreviation.

I'm currently slighty in favour for option b).

Regards,
Bert

 builtin-for-each-ref.c |   22 +++++++++++++++++-----
 1 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/builtin-for-each-ref.c b/builtin-for-each-ref.c
index 5cbb4b0..2f323c6 100644
--- a/builtin-for-each-ref.c
+++ b/builtin-for-each-ref.c
@@ -569,7 +569,7 @@ static void gen_scanf_fmt(char *scanf_fmt, const char *rule)
 /*
  * Shorten the refname to an non-ambiguous form
  */
-static char *get_short_ref(struct refinfo *ref)
+static char *get_short_ref(struct refinfo *ref, int strict)
 {
 	int i;
 	static char **scanf_fmts;
@@ -606,6 +606,7 @@ 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 rules_to_fail = i;
 		int short_name_len;
 
 		if (1 != sscanf(ref->refname, scanf_fmts[i], short_name))
@@ -614,14 +615,25 @@ static char *get_short_ref(struct refinfo *ref)
 		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;
+
+		/*
 		 * 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++) {
+		for (j = 0; j < rules_to_fail; j++) {
 			const char *rule = ref_rev_parse_rules[j];
 			unsigned char short_objectname[20];
 			char refname[PATH_MAX];
 
+			/* skip matched rule */
+			if (i == j)
+				continue;
+
 			/*
 			 * the short name is ambiguous, if it resolves
 			 * (with this previous rule) to a valid ref
@@ -635,9 +647,9 @@ static char *get_short_ref(struct refinfo *ref)
 
 		/*
 		 * short name is non-ambiguous if all previous rules
-		 * haven't resolved to a valid ref
+		 * doesn't resolved to a valid ref
 		 */
-		if (j == i)
+		if (j == rules_to_fail)
 			return short_name;
 	}
 
@@ -684,7 +696,7 @@ static void populate_value(struct refinfo *ref)
 			if (formatp) {
 				formatp++;
 				if (!strcmp(formatp, "short"))
-					refname = get_short_ref(ref);
+					refname = get_short_ref(ref, 0);
 				else
 					die("unknown refname format %s",
 					    formatp);
-- 
tg: (e37347b..) bw/short_ref-warnAmbiguousRefs (depends on: master)

Re: [PATCH&RFC] get_short_ref(): add strict mode

[Junio C Hamano]

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

> I think of 3 alternatives to use this mode for the "refname" format (and
> probably others):
>
>   a) Use core.warnAmbiguousRefs to control strict mode.
>      This would change the current default behaviour, because this is true
>      by default.
>
>   b) Introduce a new core config variable to control this, either for
>      for-each-ref alone ore globally.
>
>   c) Introduce a "refname:short-strict" format to get the strict abbreviation.
>
> I'm currently slighty in favour for option b).

Your earlier http://thread.gmane.org/gmane.comp.version-control.git/96464
made a lot of sense to me.  The request "refname:short" cannot be for use
by scripts (well, scripts may pass it to for-each-ref but that has to be
for final consumption by humans wanting to view the names in a format not
overly long, as opposed to scripts using for-each-ref to extract
unambiguous names to be used for further processing, in which case they
would be using "refname" without ":short"), so I do not see "change the
current default behaviour" is a bad thing at all.  If anything, it is an
improvement, isn't it?

Re: [PATCH&RFC] get_short_ref(): add strict mode

[Bert Wesarg]

On Sat, Apr 11, 2009 at 21:23, Junio C Hamano <gitster@pobox.com> wrote:
> Bert Wesarg <bert.wesarg@googlemail.com> writes:
>
>> I think of 3 alternatives to use this mode for the "refname" format (and
>> probably others):
>>
>>   a) Use core.warnAmbiguousRefs to control strict mode.
>>      This would change the current default behaviour, because this is true
>>      by default.
>>
>>   b) Introduce a new core config variable to control this, either for
>>      for-each-ref alone ore globally.
>>
>>   c) Introduce a "refname:short-strict" format to get the strict abbreviation.
>>
>> I'm currently slighty in favour for option b).
>
> Your earlier http://thread.gmane.org/gmane.comp.version-control.git/96464
> made a lot of sense to me.  The request "refname:short" cannot be for use
> by scripts (well, scripts may pass it to for-each-ref but that has to be
> for final consumption by humans wanting to view the names in a format not
> overly long, as opposed to scripts using for-each-ref to extract
> unambiguous names to be used for further processing, in which case they
> would be using "refname" without ":short"), so I do not see "change the
> current default behaviour" is a bad thing at all.  If anything, it is an
> improvement, isn't it?
Sure it is. So you're still with option a) and I'm ok with this. I
prepare a patch.

Bert

[PATCH] for-each-ref: refname:short utilize core.warnAmbiguousRefs

[Bert Wesarg]

core.warnAmbiguousRefs is used to select strict mode for the
abbreviation for the "refname:short" format.

In strict mode, the abbreviated ref will never trigger the
'warn_ambiguous_refs' warning. I.e. for these refs:

  refs/heads/xyzzy
  refs/tags/xyzzy

the abbreviated forms are:

  heads/xyzzy
  tags/xyzzy

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

Cc: "Jeff King" <peff@peff.net>
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             |    6 +++++-
 t/t6300-for-each-ref.sh            |   18 +++++++++++++++---
 3 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 5061d3e..42cfad9 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -75,6 +75,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`.
+	The option core.warnAmbiguousRefs is used to select the strict
+	abbreviation mode.
 
 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 2f323c6..f2af55a 100644
--- a/builtin-for-each-ref.c
+++ b/builtin-for-each-ref.c
@@ -696,7 +696,8 @@ static void populate_value(struct refinfo *ref)
 			if (formatp) {
 				formatp++;
 				if (!strcmp(formatp, "short"))
-					refname = get_short_ref(ref, 0);
+					refname = get_short_ref(ref,
+						warn_ambiguous_refs);
 				else
 					die("unknown refname format %s",
 					    formatp);
@@ -1013,6 +1014,9 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 		sort = default_sort();
 	sort_atom_limit = used_atom_cnt;
 
+	/* for warn_ambiguous_refs */
+	git_config(git_default_config, NULL);
+
 	memset(&cbdata, 0, sizeof(cbdata));
 	cbdata.grab_pattern = argv;
 	for_each_ref(grab_single_ref, &cbdata);
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 8bfae44..f83be5d 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -279,10 +279,11 @@ test_expect_success 'Check for invalid refname format' '
 
 cat >expected <<\EOF
 heads/master
-master
+tags/master
 EOF
 
-test_expect_success 'Check ambiguous head and tag refs' '
+test_expect_success 'Check ambiguous head and tag refs (strict)' '
+	git config --bool core.warnambiguousrefs true &&
 	git checkout -b newtag &&
 	echo "Using $datestamp" > one &&
 	git add one &&
@@ -294,11 +295,22 @@ test_expect_success 'Check ambiguous head and tag refs' '
 '
 
 cat >expected <<\EOF
+heads/master
+master
+EOF
+
+test_expect_success 'Check ambiguous head and tag refs (loose)' '
+	git config --bool core.warnambiguousrefs false &&
+	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' '
+test_expect_success 'Check ambiguous head and tag refs II (loose)' '
 	git checkout master &&
 	git tag ambiguous testtag^0 &&
 	git branch ambiguous testtag^0 &&
-- 
tg: (6750239..) bw/utilize-it (depends on: bw/short_ref-warnAmbiguousRefs)

Re: [PATCH 4/5] make get_short_ref a public function

[Jeff King]

On Thu, Apr 09, 2009 at 11:05:06AM +0200, Bert Wesarg wrote:

> > you the one you want). An alternative would be to show:
> >
> >  heads/master
> >  remotes/master
> >
> > in this case.
> Right, and the idea was to choose the alternatives based on the
> core.warnAmbiguousRefs setting, i.e. the former for false, the latter
> for true.
> 
> For what I posted a patch some time ago:
> 
> http://thread.gmane.org/gmane.comp.version-control.git/96464

Ah, OK, now I understand what you meant. I think that is the right
solution. Thanks.

-Peff

Re: [PATCH 5/5] branch: show upstream branch when double verbose

[Jeff King]

On Thu, Apr 09, 2009 at 12:15:08PM +0200, Santi Béjar wrote:

> I've been thinking about this and both formats seems OK for me,
> although using the +5 -6 format for just -v seems a good point.

The trivial patch for this is below:

---
diff --git a/builtin-branch.c b/builtin-branch.c
index 3275821..c056a4d 100644
--- a/builtin-branch.c
+++ b/builtin-branch.c
@@ -317,14 +317,14 @@ static void fill_tracking_info(struct strbuf *stat, const char *branch_name,
 
 	strbuf_addch(stat, '[');
 	if (show_upstream_ref)
-		strbuf_addf(stat, "%s: ",
+		strbuf_addf(stat, "%s ",
 			shorten_unambiguous_ref(branch->merge[0]->dst));
 	if (!ours)
-		strbuf_addf(stat, "behind %d] ", theirs);
+		strbuf_addf(stat, "-%d] ", theirs);
 	else if (!theirs)
-		strbuf_addf(stat, "ahead %d] ", ours);
+		strbuf_addf(stat, "+%d] ", ours);
 	else
-		strbuf_addf(stat, "ahead %d, behind %d] ", ours, theirs);
+		strbuf_addf(stat, "+%d -%d] ", ours, theirs);
 }
 
 static int matches_merge_filter(struct commit *commit)


I actually think it looks a bit ugly without the upstream name, as the
short bit in brackets blends in with the commit subject:

  next        1412037 [+2] shorter tracking format for branch -v

whereas I think this is better:

  next        1412037 [origin/next +2] shorter tracking format for branch -v

but I am still lukewarm on the concept personally (i.e., I am not
opposed, but I am not taking it further than the "how about this" patch
below, so if somebody thinks it is a good idea they should speak up and
advocate for it).

> Just to bikeshed a bit more :) we could use a format more similar to
> the "git fetch" output, like:
> 
>   next         c4628f8 [4...6 origin/next] Merge branch 'jk/no-perl' into next
>   next         c4628f8 [4.. origin/next] Merge branch 'jk/no-perl' into next
>   next         c4628f8 [..6 origin/next] Merge branch 'jk/no-perl' into next

Personally, I find that pretty ugly, and a bit confusing. The ".."
notation would make more sense if there were actual revisions on the end
and not numbers. But then of course you have to show the numbers
separately. Something like this makes at least has some logic to it:

  origin/next...next: 4,6
  origin/next..next: 4
  next..origin/next: 6

but it looks horribly ugly and is way more confusing than it needs to
be.

-Peff

Re: [PATCH 5/5] branch: show upstream branch when double verbose

[Wincent Colaiuta]

El 13/4/2009, a las 10:34, Jeff King escribió:

> On Thu, Apr 09, 2009 at 12:15:08PM +0200, Santi Béjar wrote:
>
>> I've been thinking about this and both formats seems OK for me,
>> although using the +5 -6 format for just -v seems a good point.
>
> The trivial patch for this is below:
>
> ---
> diff --git a/builtin-branch.c b/builtin-branch.c
> index 3275821..c056a4d 100644
> --- a/builtin-branch.c
> +++ b/builtin-branch.c
> @@ -317,14 +317,14 @@ static void fill_tracking_info(struct strbuf  
> *stat, const char *branch_name,
>
> 	strbuf_addch(stat, '[');
> 	if (show_upstream_ref)
> -		strbuf_addf(stat, "%s: ",
> +		strbuf_addf(stat, "%s ",
> 			shorten_unambiguous_ref(branch->merge[0]->dst));
> 	if (!ours)
> -		strbuf_addf(stat, "behind %d] ", theirs);
> +		strbuf_addf(stat, "-%d] ", theirs);
> 	else if (!theirs)
> -		strbuf_addf(stat, "ahead %d] ", ours);
> +		strbuf_addf(stat, "+%d] ", ours);
> 	else
> -		strbuf_addf(stat, "ahead %d, behind %d] ", ours, theirs);
> +		strbuf_addf(stat, "+%d -%d] ", ours, theirs);
> }
>
> static int matches_merge_filter(struct commit *commit)

I'm skeptical that people will know at a glance that "-" and "+" in  
this context map onto "behind" and "ahead". I think that trading off  
the clarity and obviousness of the words "behind" and "ahead" for the  
brevity of the symbols "-" and "+" wouldn't be a very good idea.

Cheers,
Wincent