[PATCH v2 00/10] port branch.c to use ref-filter's printing options

[PATCH v2 00/10] port branch.c to use ref-filter's printing options

[Karthik Nayak]

This is part of unification of the commands 'git tag -l, git branch -l
and git for-each-ref'. This ports over branch.c to use ref-filter's
printing options. The previous version of the patch can be found at:
http://article.gmane.org/gmane.comp.version-control.git/278926

Karthik Nayak (10):
  ref-filter: implement %(if), %(then), and %(else) atoms
  ref-filter: implement %(if:equals=<string>) and
    %(if:notequals=<string>)
  ref-filter: add support for %(refname:dir) and %(refname:base)
  ref-filter: modify "%(objectname:short)" to take length
  ref-filter: adopt get_head_description() from branch.c
  ref-filter: introduce format_ref_array_item()
  ref-filter: make %(upstream:track) prints "[gone]" for invalid
    upstreams
  ref-filter: add support for %(upstream:track,nobracket)
  branch: use ref-filter printing APIs
  branch: implement '--format' option

 Documentation/git-branch.txt       |   7 +-
 Documentation/git-for-each-ref.txt |  63 +++++++-
 builtin/branch.c                   | 265 +++++++++-------------------------
 ref-filter.c                       | 285 +++++++++++++++++++++++++++++++++----
 ref-filter.h                       |   5 +
 t/t3203-branch-output.sh           |  11 ++
 t/t6040-tracking-info.sh           |   2 +-
 t/t6300-for-each-ref.sh            |  33 ++++-
 t/t6302-for-each-ref-filter.sh     | 132 +++++++++++++++++
 9 files changed, 573 insertions(+), 230 deletions(-)

Changes in this version:

1. Use %(refname:dir) and %(refname:base) instead of %(path) and
%(path:short).
2. Make it %(objectname:short=<len>) rather than %(objectname:short,<len>).
3. Add %(upstream:track,nobracket) which removes the brackets from tracking
information.
4. calc_maxwidth() now also considers head ref descriptions, this wasn't done
in the last patch and would cause detached head description to be not aligned with the other ref-names.
5. remove the if_atom field in if_then_else struct.
6. introduce better error checking for the %(if), %(then), %(else) atoms
as suggested by Matthieu (when used along with %(align) atom and so on).
7. refactor code and improve documentation.

Interdiff:

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 5097915..a38cbf6 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -92,7 +92,9 @@ 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.
+	abbreviation mode. For the base directory of the ref (i.e. foo
+	in refs/foo/bar/boz) append `:base`. For the entire directory
+	path append `:dir`.
 
 objecttype::
 	The type of the object (`blob`, `tree`, `commit`, `tag`).
@@ -102,9 +104,9 @@ objectsize::
 
 objectname::
 	The object name (aka SHA-1).
-	For a non-ambiguous abbreviation of the object name append `:short`.
-	The length can be explicitly specified by appending either
-	`:short,<length>` or `:<length>,short`.  Minimum length is 4.
+	For a non-ambiguous abbreviation of the object name append
+	`:short`.  The length can be explicitly specified by appending
+	`:short=<length>`.  Minimum length being 4.
 
 upstream::
 	The name of a local ref which can be considered ``upstream''
@@ -112,8 +114,10 @@ upstream::
 	`refname` above.  Additionally respects `:track` to show
 	"[ahead N, behind M]" and `:trackshort` to show the terse
 	version: ">" (ahead), "<" (behind), "<>" (ahead and behind),
-	or "=" (in sync).  Has no effect if the ref does not have
-	tracking information associated with it.
+	or "=" (in sync).  Append `:track,nobracket` to show tracking
+	information without brackets (i.e "ahead N, behind M").  Has
+	no effect if the ref does not have tracking information
+	associated with it.
 
 push::
 	The name of a local ref which represents the `@{push}` location
@@ -143,14 +147,12 @@ if::
 	If there is an atom with value or string literal after the
 	%(if) then everything after the %(then) is printed, else if
 	the %(else) atom is used, then everything after %(else) is
-	printed. Append ":equals=<string>" or ":notequals=<string>" to
-	compare the value between the %(if:...) and %(then) atoms with the
-	given string.
-
-path::
-	The path of the given ref. For a shortened version listing
-	only the name of the directory the ref is under append
-	`:short`.
+	printed. We ignore space when evaluating the string before
+	%(then), this is useful when we use the %(HEAD) atom which
+	prints either "*" or " " and we want to apply the 'if'
+	condition only on the 'HEAD' ref. Append ":equals=<string>" or
+	":notequals=<string>" to compare the value between the
+	%(if:...) and %(then) atoms with the given string.
 
 In addition to the above, for commit and tag objects, the header
 field names (`tree`, `parent`, `object`, `type`, and `tag`) can
@@ -191,8 +193,9 @@ opening atoms, replacement from every %(atom) is quoted when and only
 when it appears at the top-level (that is, when it appears outside
 %($open)...%(end)).
 
-When a scripting language specific quoting is in effect, everything
-between a top-level opening atom and its matching %(end) is evaluated
+When a scripting language specific quoting is in effect (i.e. one of
+`--shell`, `--perl`, `--python`, `--tcl` is used), everything between
+a top-level opening atom and its matching %(end) is evaluated
 according to the semantics of the opening atom and its result is
 quoted.
 
@@ -282,6 +285,26 @@ eval=`git for-each-ref --shell --format="$fmt" \
 eval "$eval"
 ------------
 
+
+An example to show the usage of %(if)...%(then)...%(else)...%(end).
+This prefixes the current branch with a star.
+
+------------
+#!/bin/sh
+
+git for-each-ref --format="%(if)%(HEAD)%(then)* %(else)  %(end)%(refname:short)" refs/heads/
+------------
+
+
+An example to show the usage of %(if)...%(then)...%(end).
+This adds a red color to authorname, if present
+
+------------
+#!/bin/sh
+
+git for-each-ref --format="%(refname)%(if)%(authorname)%(then)%(color:red)%(end) %(authorname)"
+------------
+
 SEE ALSO
 --------
 linkgit:git-show-ref[1]
diff --git a/builtin/branch.c b/builtin/branch.c
index ae3ecfb..2356e72 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -282,7 +282,10 @@ static int calc_maxwidth(struct ref_array *refs, int remote_bonus)
 
 		skip_prefix(it->refname, "refs/heads/", &desc);
 		skip_prefix(it->refname, "refs/remotes/", &desc);
-		w = utf8_strwidth(desc);
+		if (it->kind == FILTER_REFS_DETACHED_HEAD)
+			w = strlen(get_head_description());
+		else
+			w = utf8_strwidth(desc);
 
 		if (it->kind == FILTER_REFS_REMOTES)
 			w += remote_bonus;
@@ -292,44 +295,41 @@ static int calc_maxwidth(struct ref_array *refs, int remote_bonus)
 	return max;
 }
 
-static char *get_format(struct ref_filter *filter, int maxwidth, const char *remote_prefix)
+static char *build_format(struct ref_filter *filter, int maxwidth, const char *remote_prefix)
 {
-	char *remote = NULL;
-	char *local = NULL;
-	char *final = NULL;
+	struct strbuf fmt = STRBUF_INIT;
+	struct strbuf local = STRBUF_INIT;
+	struct strbuf remote = STRBUF_INIT;
+
+	strbuf_addf(&fmt, "%%(if)%%(HEAD)%%(then)* %s%%(else)  %%(end)", branch_get_color(BRANCH_COLOR_CURRENT));
 
 	if (filter->verbose) {
-		if (filter->verbose > 1)
-			local = xstrfmt("%%(if)%%(HEAD)%%(then)* %s%%(else)  %%(end)%%(align:%d,left)%%(refname:short)%%(end)%s"
-					" %%(objectname:short,7) %%(if)%%(upstream)%%(then)[%s%%(upstream:short)%s] %%(end)"
-					"%%(if)%%(upstream:track)%%(then)%%(upstream:track) %%(end)%%(contents:subject)",
-					branch_get_color(BRANCH_COLOR_CURRENT), maxwidth, branch_get_color(BRANCH_COLOR_RESET),
-					branch_get_color(BRANCH_COLOR_UPSTREAM), branch_get_color(BRANCH_COLOR_RESET));
+		strbuf_addf(&local, "%%(align:%d,left)%%(refname:short)%%(end)", maxwidth);
+		strbuf_addf(&local, "%s", branch_get_color(BRANCH_COLOR_RESET));
+		strbuf_addf(&local, " %%(objectname:short=7) ");
 
+		if (filter->verbose > 1)
+			strbuf_addf(&local, "%%(if)%%(upstream)%%(then)[%s%%(upstream:short)%s%%(if)%%(upstream:track)"
+				    "%%(then): %%(upstream:track,nobracket)%%(end)] %%(end)%%(contents:subject)",
+				    branch_get_color(BRANCH_COLOR_UPSTREAM), branch_get_color(BRANCH_COLOR_RESET));
 		else
-			local = xstrfmt("%%(if)%%(HEAD)%%(then)* %s%%(else)  %%(end)%%(align:%d,left)%"
-					"%(refname:short)%%(end)%s %%(objectname:short,7) %%(if)%%(upstream:track)%"
-					"%(then)%%(upstream:track) %%(end)%%(contents:subject)",
-					branch_get_color(BRANCH_COLOR_CURRENT), maxwidth, branch_get_color(BRANCH_COLOR_RESET));
-
-		remote = xstrfmt("  %s%%(align:%d,left)%s%%(refname:short)%%(end)%s%%(if)%%(symref)%%(then) -> %%(symref:short)"
-				 "%%(else) %%(objectname:short,7) %%(contents:subject)%%(end)",
-				 branch_get_color(BRANCH_COLOR_REMOTE), maxwidth,
-				 remote_prefix, branch_get_color(BRANCH_COLOR_RESET));
-		final = xstrfmt("%%(if:notequals=remotes)%%(path:short)%%(then)%s%%(else)%s%%(end)", local, remote);
+			strbuf_addf(&local, "%%(if)%%(upstream:track)%%(then)%%(upstream:track) %%(end)%%(contents:subject)");
 
+		strbuf_addf(&remote, "%s%%(align:%d,left)%s%%(refname:short)%%(end)%s%%(if)%%(symref)%%(then) -> %%(symref:short)"
+			    "%%(else) %%(objectname:short=7) %%(contents:subject)%%(end)",
+			    branch_get_color(BRANCH_COLOR_REMOTE), maxwidth,
+			    remote_prefix, branch_get_color(BRANCH_COLOR_RESET));
 	} else {
-		local = xstrfmt("%%(if)%%(HEAD)%%(then)* %s%%(else)  %%(end)%%(refname:short)%s",
-				branch_get_color(BRANCH_COLOR_CURRENT), branch_get_color(BRANCH_COLOR_RESET));
-		remote = xstrfmt("  %s%s%%(refname:short)%s%%(if)%%(symref)%%(then) -> %%(symref:short)%%(end)",
-				 branch_get_color(BRANCH_COLOR_REMOTE), remote_prefix, branch_get_color(BRANCH_COLOR_RESET));
-		final = xstrfmt("%%(if:notequals=remotes)%%(path:short)%%(then)%s%%(else)%s%%(end)", local, remote);
+		strbuf_addf(&local, "%%(refname:short)%s", branch_get_color(BRANCH_COLOR_RESET));
+		strbuf_addf(&remote, "%s%s%%(refname:short)%s%%(if)%%(symref)%%(then) -> %%(symref:short)%%(end)",
+			    branch_get_color(BRANCH_COLOR_REMOTE), remote_prefix, branch_get_color(BRANCH_COLOR_RESET));
 	}
 
-	free(local);
-	free(remote);
+	strbuf_addf(&fmt, "%%(if:notequals=remotes)%%(refname:base)%%(then)%s%%(else)%s%%(end)", local.buf, remote.buf);
 
-	return final;
+	strbuf_release(&local);
+	strbuf_release(&remote);
+	return strbuf_detach(&fmt, NULL);
 }
 
 static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sorting, const char *format)
@@ -357,7 +357,7 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
 		maxwidth = calc_maxwidth(&array, strlen(remote_prefix));
 
 	if (!format)
-		format = to_free = get_format(filter, maxwidth, remote_prefix);
+		format = to_free = build_format(filter, maxwidth, remote_prefix);
 	verify_ref_format(format);
 
 	/*
@@ -375,6 +375,7 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
 		format_ref_array_item(array.items[i], format, 0, &out);
 		if (column_active(colopts)) {
 			assert(!filter->verbose && "--column and --verbose are incompatible");
+			 /* format to a string_list to let print_columns() do its job */
 			string_list_append(&output, out.buf);
 		} else {
 			fwrite(out.buf, 1, out.len, stdout);
diff --git a/ref-filter.c b/ref-filter.c
index 48b06e3..6044eb0 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -59,7 +59,6 @@ static struct {
 	{ "if" },
 	{ "then" },
 	{ "else" },
-	{ "path" },
 };
 
 #define REF_FORMATTING_STATE_INIT  { 0, NULL }
@@ -77,8 +76,7 @@ struct contents {
 struct if_then_else {
 	const char *if_equals,
 		*not_equals;
-	unsigned int if_atom : 1,
-		then_atom : 1,
+	unsigned int then_atom : 1,
 		else_atom : 1,
 		condition_satisfied : 1;
 };
@@ -257,22 +255,27 @@ static void if_then_else_handler(struct ref_formatting_stack **stack)
 	struct ref_formatting_stack *prev = cur->prev;
 	struct if_then_else *if_then_else = (struct if_then_else *)cur->at_end_data;
 
-	/*
-	 * If the 'if' condition is satisfied, then if there exists an
-	 * 'else' atom drop the 'else' atom's state. Else we swap the
-	 * buffer of the 'else' atom with the previous state and drop
-	 * that state. If the condition is satisfied and no 'else' atom
-	 * exists, then just clear the buffer.
-	 */
-	if (if_then_else->condition_satisfied && if_then_else->else_atom) {
-		strbuf_reset(&cur->output);
-		pop_stack_element(&cur);
-	} else if (if_then_else->else_atom) {
-		strbuf_swap(&cur->output, &prev->output);
-		strbuf_reset(&cur->output);
-		pop_stack_element(&cur);
+	if (if_then_else->else_atom) {
+		/*
+		 * There is an %(else) atom: we need to drop one state from the
+		 * stack, either the %(else) branch if the condition is satisfied, or
+		 * the %(then) branch if it isn't.
+		 */
+		if (if_then_else->condition_satisfied) {
+			strbuf_reset(&cur->output);
+			pop_stack_element(&cur);
+		} else {
+			strbuf_swap(&cur->output, &prev->output);
+			strbuf_reset(&cur->output);
+			pop_stack_element(&cur);
+		}
 	} else if (!if_then_else->condition_satisfied)
+		/*
+		 * No %(else) atom: just drop the %(then) branch if the
+		 * condition is not satisfied.
+		 */
 		strbuf_reset(&cur->output);
+
 	*stack = cur;
 	free(if_then_else);
 }
@@ -283,7 +286,6 @@ static void if_atom_handler(struct atom_value *atomv, struct ref_formatting_stat
 	struct if_then_else *if_then_else = xcalloc(sizeof(struct if_then_else), 1);
 	const char *valp;
 
-	if_then_else->if_atom = 1;
 	if (skip_prefix(atomv->s, "equals=", &valp))
 		if_then_else->if_equals = valp;
 	else if (skip_prefix(atomv->s, "notequals=", &valp))
@@ -297,7 +299,7 @@ static void if_atom_handler(struct atom_value *atomv, struct ref_formatting_stat
 	new->at_end_data = if_then_else;
 }
 
-static int is_empty(const char * s){
+static int is_empty(const char *s){
 	while (*s != '\0') {
 		if (!isspace(*s))
 			return 0;
@@ -309,10 +311,14 @@ static int is_empty(const char * s){
 static void then_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
 {
 	struct ref_formatting_stack *cur = state->stack;
-	struct if_then_else *if_then_else = (struct if_then_else *)cur->at_end_data;
+	struct if_then_else *if_then_else = NULL;
 
+	if (cur->at_end == if_then_else_handler)
+		if_then_else = (struct if_then_else *)cur->at_end_data;
 	if (!if_then_else)
 		die(_("format: %%(then) atom used without an %%(if) atom"));
+	if (if_then_else->then_atom)
+		die(_("format: %%(then) atom used more than once"));
 	if_then_else->then_atom = 1;
 
 	/*
@@ -334,12 +340,16 @@ static void then_atom_handler(struct atom_value *atomv, struct ref_formatting_st
 static void else_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
 {
 	struct ref_formatting_stack *prev = state->stack;
-	struct if_then_else *if_then_else = (struct if_then_else *)state->stack->at_end_data;
+	struct if_then_else *if_then_else = NULL;
 
+	if (prev->at_end == if_then_else_handler)
+		if_then_else = (struct if_then_else *)prev->at_end_data;
 	if (!if_then_else)
 		die(_("format: %%(else) atom used without an %%(if) atom"));
 	if (!if_then_else->then_atom)
 		die(_("format: %%(else) atom used without a %%(then) atom"));
+	if (if_then_else->else_atom)
+		die(_("format: %%(else) atom used more than once"));
 	if_then_else->else_atom = 1;
 	push_stack_element(&state->stack);
 	state->stack->at_end_data = prev->at_end_data;
@@ -355,7 +365,7 @@ static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_sta
 		die(_("format: %%(end) atom used without corresponding atom"));
 	current->at_end(&state->stack);
 
-	/*  Stack may have been popped, hence reset the current pointer */
+	/*  Stack may have been popped within at_end(), hence reset the current pointer */
 	current = state->stack;
 
 	/*
@@ -458,33 +468,24 @@ static int grab_objectname(const char *name, const unsigned char *sha1,
 	const char *p;
 
 	if (match_atom_name(name, "objectname", &p)) {
-		struct strbuf **s, **to_free;
-		int length = DEFAULT_ABBREV;
-
 		/*  No arguments given, copy the entire objectname */
 		if (!p) {
 			char *s = xmalloc(41);
 			strcpy(s, sha1_to_hex(sha1));
 			v->s = s;
 		} else {
-			s = to_free = strbuf_split_str(p, ',', 0);
-			while (*s) {
-				/*  Strip trailing comma */
-				if (s[1])
-					strbuf_setlen(s[0], s[0]->len - 1);
-				if (!strtoul_ui(s[0]->buf, 10, (unsigned int *)&length))
-					;
-				/*  The `short` argument uses the default length */
-				else if (!strcmp("short", s[0]->buf))
-					;
-				else
-					die(_("format: unknown option entered with objectname:%s"), s[0]->buf);
-				s++;
-			}
+			unsigned int length = DEFAULT_ABBREV;
+
+			if (skip_prefix(p, "short", &p)) {
+				if (p[0] == '=')
+					if (strtoul_ui(p + 1, 10, &length))
+						die(_("positive length expected with objectname:%s"), p + 1);
+			} else
+				die(_("format: unknown option entered with objectname:%s"), p);
+
 			if (length < MINIMUM_ABBREV)
 				length = MINIMUM_ABBREV;
 			v->s = xstrdup(find_unique_abbrev(sha1, length));
-			free(to_free);
 		}
 		return 1;
 	}
@@ -916,7 +917,7 @@ static inline char *copy_advance(char *dst, const char *src)
 	return dst;
 }
 
-static char *get_head_description(void)
+char *get_head_description(void)
 {
 	struct strbuf desc = STRBUF_INIT;
 	struct wt_status_state state;
@@ -1100,22 +1101,6 @@ static void populate_value(struct ref_array_item *ref)
 		} else if (!strcmp(name, "else")) {
 			v->handler = else_atom_handler;
 			continue;
-		} else if (match_atom_name(name, "path", &valp)) {
-			const char *sp, *ep;
-
-			if (ref->kind & FILTER_REFS_DETACHED_HEAD)
-				continue;
-
-			sp = strchr(ref->refname, '/');
-			ep = strchr(++sp, '/');
-
-			if (!valp)
-				sp = ref->refname;
-			else if (strcmp(valp, "short"))
-				die(_("format: invalid value given path:%s"), valp);
-
-			v->s = xstrndup(sp, ep - sp);
-			continue;
 		} else
 			continue;
 
@@ -1127,27 +1112,45 @@ static void populate_value(struct ref_array_item *ref)
 			if (!strcmp(formatp, "short"))
 				refname = shorten_unambiguous_ref(refname,
 						      warn_ambiguous_refs);
-			else if (!strcmp(formatp, "track") &&
+			else if (skip_prefix(formatp, "track", &valp) &&
+				 strcmp(formatp, "trackshort") &&
 				 (starts_with(name, "upstream") ||
 				  starts_with(name, "push"))) {
 				char buf[40];
+				unsigned int nobracket = 0;
+
+				if (!strcmp(valp, ",nobracket"))
+					nobracket = 1;
 
 				if (stat_tracking_info(branch, &num_ours,
 						       &num_theirs, NULL)) {
-					v->s = xstrdup("[gone]");
+					if (nobracket)
+						v->s = "gone";
+					else
+						v->s = "[gone]";
 					continue;
 				}
 
 				if (!num_ours && !num_theirs)
 					v->s = "";
 				else if (!num_ours) {
-					sprintf(buf, "[behind %d]", num_theirs);
+					if (nobracket)
+						sprintf(buf, "behind %d", num_theirs);
+					else
+						sprintf(buf, "[behind %d]", num_theirs);
 					v->s = xstrdup(buf);
 				} else if (!num_theirs) {
-					sprintf(buf, "[ahead %d]", num_ours);
+					if (nobracket)
+						sprintf(buf, "ahead %d", num_ours);
+					else
+						sprintf(buf, "[ahead %d]", num_ours);
 					v->s = xstrdup(buf);
 				} else {
-					sprintf(buf, "[ahead %d, behind %d]",
+					if (nobracket)
+						sprintf(buf, "ahead %d, behind %d",
+							num_ours, num_theirs);
+					else
+						sprintf(buf, "[ahead %d, behind %d]",
 						num_ours, num_theirs);
 					v->s = xstrdup(buf);
 				}
@@ -1170,6 +1173,29 @@ static void populate_value(struct ref_array_item *ref)
 				else
 					v->s = "<>";
 				continue;
+			} else if (!strcmp(formatp, "dir") &&
+				   (starts_with(name, "refname"))) {
+				const char *sp, *ep, *tmp;
+
+				sp = tmp = ref->refname;
+				/*  Obtain refs/foo/bar/ from refname refs/foo/bar/abc */
+				do {
+					ep = tmp;
+					tmp = strchr(ep + 1, '/');
+				} while (tmp);
+				v->s = xstrndup(sp, ep - sp);
+				continue;
+			} else if (!strcmp(formatp, "base") &&
+				   (starts_with(name, "refname"))) {
+				const char *sp, *ep;
+
+				if (skip_prefix(ref->refname, "refs/", &sp)) {
+					ep = strchr(sp, '/');
+					if (!ep)
+						continue;
+					v->s = xstrndup(sp, ep - sp);
+				}
+				continue;
 			} else
 				die("unknown %.*s format %s",
 				    (int)(formatp - name), name, formatp);
diff --git a/ref-filter.h b/ref-filter.h
index e0b26e8..0014b92 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -109,5 +109,7 @@ int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset);
 struct ref_sorting *ref_default_sorting(void);
 /*  Function to parse --merged and --no-merged options */
 int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset);
+/*  Get the current HEAD's description */
+char *get_head_description(void);
 
 #endif /*  REF_FILTER_H  */
diff --git a/t/t6040-tracking-info.sh b/t/t6040-tracking-info.sh
index d110f26..97a0765 100755
--- a/t/t6040-tracking-info.sh
+++ b/t/t6040-tracking-info.sh
@@ -58,11 +58,11 @@ test_expect_success 'branch -v' '
 '
 
 cat >expect <<\EOF
-b1 [origin/master] [ahead 1, behind 1] d
-b2 [origin/master] [ahead 1, behind 1] d
-b3 [origin/master] [behind 1] b
-b4 [origin/master] [ahead 2] f
-b5 [brokenbase] [gone] g
+b1 [origin/master: ahead 1, behind 1] d
+b2 [origin/master: ahead 1, behind 1] d
+b3 [origin/master: behind 1] b
+b4 [origin/master: ahead 2] f
+b5 [brokenbase: gone] g
 b6 [origin/master] c
 EOF
 
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 2a01645..7ab8bf8 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -344,6 +344,15 @@ test_expect_success 'Check upstream:track format' '
 '
 
 cat >expected <<EOF
+ahead 1
+EOF
+
+test_expect_success 'Check upstream:track,nobracket format' '
+	git for-each-ref --format="%(upstream:track,nobracket)" refs/heads >actual &&
+	test_cmp expected actual
+'
+
+cat >expected <<EOF
 >
 EOF
 
@@ -390,7 +399,7 @@ $(git rev-parse --short=1 HEAD)
 EOF
 
 test_expect_success 'Check objectname:short format when size is less than MINIMUM_ABBREV' '
-	git for-each-ref --format="%(objectname:short,1)" refs/heads >actual &&
+	git for-each-ref --format="%(objectname:short=1)" refs/heads >actual &&
 	test_cmp expected actual
 '
 
@@ -399,12 +408,12 @@ $(git rev-parse --short=10 HEAD)
 EOF
 
 test_expect_success 'Check objectname:short format' '
-	git for-each-ref --format="%(objectname:short,10)" refs/heads >actual &&
+	git for-each-ref --format="%(objectname:short=10)" refs/heads >actual &&
 	test_cmp expected actual
 '
 
 test_expect_success 'Check objectname:short format for invalid input' '
-	test_must_fail git for-each-ref --format="%(objectname:short,-1)" refs/heads
+	test_must_fail git for-each-ref --format="%(objectname:short=-1)" refs/heads
 '
 
 test_expect_success 'Check for invalid refname format' '
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index 5557657..19a5075 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -303,6 +303,25 @@ test_expect_success 'check %(if)...%(then)...%(else)...%(end) atoms' '
 	test_cmp expect actual
 '
 
+test_expect_success 'ignore spaces in %(if) atom usage' '
+	git for-each-ref --format="%(refname:short): %(if)%(HEAD)%(then)Head ref%(else)Not Head ref%(end)" >actual &&
+	cat >expect <<-\EOF &&
+	master: Head ref
+	side: Not Head ref
+	odd/spot: Not Head ref
+	double-tag: Not Head ref
+	foo1.10: Not Head ref
+	foo1.3: Not Head ref
+	foo1.6: Not Head ref
+	four: Not Head ref
+	one: Not Head ref
+	signed-tag: Not Head ref
+	three: Not Head ref
+	two: Not Head ref
+	EOF
+	test_cmp expect actual
+'
+
 test_expect_success 'check %(if:equals=<string>)' '
 	git for-each-ref --format="%(if:equals=master)%(refname:short)%(then)Found master%(else)Not master%(end)" refs/heads/ >actual &&
 	cat >expect <<-\EOF &&
@@ -312,7 +331,6 @@ test_expect_success 'check %(if:equals=<string>)' '
 	test_cmp expect actual
 '
 
-
 test_expect_success 'check %(if:notequals=<string>)' '
 	git for-each-ref --format="%(if:notequals=master)%(refname:short)%(then)Not master%(else)Found master%(end)" refs/heads/ >actual &&
 	cat >expect <<-\EOF &&
@@ -322,9 +340,16 @@ test_expect_success 'check %(if:notequals=<string>)' '
 	test_cmp expect actual
 '
 
-test_expect_success 'check %(path)' '
-	git for-each-ref --format="%(path)" >actual &&
+test_expect_success 'update refs for %(refname:dir) and %(refname:base)' '
+	git update-ref refs/foo HEAD &&
+	git update-ref refs/foodir/bar/boz HEAD
+'
+
+test_expect_success 'check %(refname:dir)' '
+	git for-each-ref --format="%(refname:dir)" >actual &&
 	cat >expect <<-\EOF &&
+	refs
+	refs/foodir/bar
 	refs/heads
 	refs/heads
 	refs/odd
@@ -341,9 +366,11 @@ test_expect_success 'check %(path)' '
 	test_cmp expect actual
 '
 
-test_expect_success 'check %(path:short)' '
-	git for-each-ref --format="%(path:short)" >actual &&
+test_expect_success 'check %(refname:base)' '
+	git for-each-ref --format="%(refname:base)" >actual &&
 	cat >expect <<-\EOF &&
+	
+	foodir
 	heads
 	heads
 	odd

--
2.6.0

[PATCH v2 01/10] ref-filter: implement %(if), %(then), and %(else) atoms

[Karthik Nayak]

Implement %(if), %(then) and %(else) atoms. Used as
%(if)...%(then)...%(end) or %(if)...%(then)...%(else)...%(end). If the
format string between %(if) and %(then) expands to an empty string, or
to only whitespaces, then the whole %(if)...%(end) expands to the
string following %(then). Otherwise, it expands to the string
following %(else), if any. Nesting of this construct is possible.

This is in preperation for porting over `git branch -l` to use
ref-filter APIs for printing.

Add Documentation and tests regarding the same.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 Documentation/git-for-each-ref.txt |  47 +++++++++++++-
 ref-filter.c                       | 127 +++++++++++++++++++++++++++++++++++--
 t/t6302-for-each-ref-filter.sh     |  67 +++++++++++++++++++
 3 files changed, 231 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 16b4ac5..7345acb 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -134,9 +134,17 @@ align::
 	`<position>` is either left, right or middle, default being
 	left and `<width>` is the total length of the content with
 	alignment. If the contents length is more than the width then
-	no alignment is performed. If used with '--quote' everything
-	in between %(align:...) and %(end) is quoted, but if nested
-	then only the topmost level performs quoting.
+	no alignment is performed.
+
+if::
+	Used as %(if)..%(then)..(%end) or %(if)..%(then)..%(else)..%(end).
+	If there is an atom with value or string literal after the
+	%(if) then everything after the %(then) is printed, else if
+	the %(else) atom is used, then everything after %(else) is
+	printed. We ignore space when evaluating the string before
+	%(then), this is useful when we use the %(HEAD) atom which
+	prints either "*" or " " and we want to apply the 'if'
+	condition only on the 'HEAD' ref.
 
 In addition to the above, for commit and tag objects, the header
 field names (`tree`, `parent`, `object`, `type`, and `tag`) can
@@ -169,6 +177,19 @@ the date by adding one of `:default`, `:relative`, `:short`, `:local`,
 `:iso8601`, `:rfc2822` or `:raw` to the end of the fieldname; e.g.
 `%(taggerdate:relative)`.
 
+Some atoms like %(align) and %(if) always require a matching %(end).
+We call them "opening atoms" and sometimes denote them as %($open).
+
+When a scripting language specific quoting is in effect, except for
+opening atoms, replacement from every %(atom) is quoted when and only
+when it appears at the top-level (that is, when it appears outside
+%($open)...%(end)).
+
+When a scripting language specific quoting is in effect (i.e. one of
+`--shell`, `--perl`, `--python`, `--tcl` is used), everything between
+a top-level opening atom and its matching %(end) is evaluated
+according to the semantics of the opening atom and its result is
+quoted.
 
 EXAMPLES
 --------
@@ -256,6 +277,26 @@ eval=`git for-each-ref --shell --format="$fmt" \
 eval "$eval"
 ------------
 
+
+An example to show the usage of %(if)...%(then)...%(else)...%(end).
+This prefixes the current branch with a star.
+
+------------
+#!/bin/sh
+
+git for-each-ref --format="%(if)%(HEAD)%(then)* %(else)  %(end)%(refname:short)" refs/heads/
+------------
+
+
+An example to show the usage of %(if)...%(then)...%(end).
+This adds a red color to authorname, if present
+
+------------
+#!/bin/sh
+
+git for-each-ref --format="%(refname)%(if)%(authorname)%(then)%(color:red)%(end) %(authorname)"
+------------
+
 SEE ALSO
 --------
 linkgit:git-show-ref[1]
diff --git a/ref-filter.c b/ref-filter.c
index dbd8fce..95f007c 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -55,6 +55,9 @@ static struct {
 	{ "color" },
 	{ "align" },
 	{ "end" },
+	{ "if" },
+	{ "then" },
+	{ "else" },
 };
 
 #define REF_FORMATTING_STATE_INIT  { 0, NULL }
@@ -69,10 +72,16 @@ struct contents {
 	struct object_id oid;
 };
 
+struct if_then_else {
+	unsigned int then_atom : 1,
+		else_atom : 1,
+		condition_satisfied : 1;
+};
+
 struct ref_formatting_stack {
 	struct ref_formatting_stack *prev;
 	struct strbuf output;
-	void (*at_end)(struct ref_formatting_stack *stack);
+	void (*at_end)(struct ref_formatting_stack **stack);
 	void *at_end_data;
 };
 
@@ -216,13 +225,14 @@ static void pop_stack_element(struct ref_formatting_stack **stack)
 	*stack = prev;
 }
 
-static void end_align_handler(struct ref_formatting_stack *stack)
+static void end_align_handler(struct ref_formatting_stack **stack)
 {
-	struct align *align = (struct align *)stack->at_end_data;
+	struct ref_formatting_stack *cur = *stack;
+	struct align *align = (struct align *)cur->at_end_data;
 	struct strbuf s = STRBUF_INIT;
 
-	strbuf_utf8_align(&s, align->position, align->width, stack->output.buf);
-	strbuf_swap(&stack->output, &s);
+	strbuf_utf8_align(&s, align->position, align->width, cur->output.buf);
+	strbuf_swap(&cur->output, &s);
 	strbuf_release(&s);
 }
 
@@ -236,6 +246,97 @@ static void align_atom_handler(struct atom_value *atomv, struct ref_formatting_s
 	new->at_end_data = &atomv->u.align;
 }
 
+static void if_then_else_handler(struct ref_formatting_stack **stack)
+{
+	struct ref_formatting_stack *cur = *stack;
+	struct ref_formatting_stack *prev = cur->prev;
+	struct if_then_else *if_then_else = (struct if_then_else *)cur->at_end_data;
+
+	if (if_then_else->else_atom) {
+		/*
+		 * There is an %(else) atom: we need to drop one state from the
+		 * stack, either the %(else) branch if the condition is satisfied, or
+		 * the %(then) branch if it isn't.
+		 */
+		if (if_then_else->condition_satisfied) {
+			strbuf_reset(&cur->output);
+			pop_stack_element(&cur);
+		} else {
+			strbuf_swap(&cur->output, &prev->output);
+			strbuf_reset(&cur->output);
+			pop_stack_element(&cur);
+		}
+	} else if (!if_then_else->condition_satisfied)
+		/*
+		 * No %(else) atom: just drop the %(then) branch if the
+		 * condition is not satisfied.
+		 */
+		strbuf_reset(&cur->output);
+
+	*stack = cur;
+	free(if_then_else);
+}
+
+static void if_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
+{
+	struct ref_formatting_stack *new;
+	struct if_then_else *if_then_else = xcalloc(sizeof(struct if_then_else), 1);
+
+	push_stack_element(&state->stack);
+	new = state->stack;
+	new->at_end = if_then_else_handler;
+	new->at_end_data = if_then_else;
+}
+
+static int is_empty(const char *s){
+	while (*s != '\0') {
+		if (!isspace(*s))
+			return 0;
+		s++;
+	}
+	return 1;
+}
+
+static void then_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
+{
+	struct ref_formatting_stack *cur = state->stack;
+	struct if_then_else *if_then_else = NULL;
+
+	if (cur->at_end == if_then_else_handler)
+		if_then_else = (struct if_then_else *)cur->at_end_data;
+	if (!if_then_else)
+		die(_("format: %%(then) atom used without an %%(if) atom"));
+	if (if_then_else->then_atom)
+		die(_("format: %%(then) atom used more than once"));
+	if_then_else->then_atom = 1;
+	/*
+	 * If there exists non-empty string between the 'if' and
+	 * 'then' atom then the 'if' condition is satisfied.
+	 */
+	if (cur->output.len && !is_empty(cur->output.buf))
+		if_then_else->condition_satisfied = 1;
+	strbuf_reset(&cur->output);
+}
+
+static void else_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
+{
+	struct ref_formatting_stack *prev = state->stack;
+	struct if_then_else *if_then_else = NULL;
+
+	if (prev->at_end == if_then_else_handler)
+		if_then_else = (struct if_then_else *)prev->at_end_data;
+	if (!if_then_else)
+		die(_("format: %%(else) atom used without an %%(if) atom"));
+	if (!if_then_else->then_atom)
+		die(_("format: %%(else) atom used without a %%(then) atom"));
+	if (if_then_else->else_atom)
+		die(_("format: %%(else) atom used more than once"));
+	if_then_else->else_atom = 1;
+	push_stack_element(&state->stack);
+	state->stack->at_end_data = prev->at_end_data;
+	state->stack->at_end = prev->at_end;
+}
+
 static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
 {
 	struct ref_formatting_stack *current = state->stack;
@@ -243,14 +344,17 @@ static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_sta
 
 	if (!current->at_end)
 		die(_("format: %%(end) atom used without corresponding atom"));
-	current->at_end(current);
+	current->at_end(&state->stack);
+
+	/*  Stack may have been popped within at_end(), hence reset the current pointer */
+	current = state->stack;
 
 	/*
 	 * Perform quote formatting when the stack element is that of
 	 * a supporting atom. If nested then perform quote formatting
 	 * only on the topmost supporting atom.
 	 */
-	if (!state->stack->prev->prev) {
+	if (!current->prev->prev) {
 		quote_formatting(&s, current->output.buf, state->quote_style);
 		strbuf_swap(&current->output, &s);
 	}
@@ -920,6 +1024,15 @@ static void populate_value(struct ref_array_item *ref)
 		} else if (!strcmp(name, "end")) {
 			v->handler = end_atom_handler;
 			continue;
+		} else if (!strcmp(name, "if")) {
+			v->handler = if_atom_handler;
+			continue;
+		} else if (!strcmp(name, "then")) {
+			v->handler = then_atom_handler;
+			continue;
+		} else if (!strcmp(name, "else")) {
+			v->handler = else_atom_handler;
+			continue;
 		} else
 			continue;
 
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index fe4796c..617fa1f 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -255,4 +255,71 @@ test_expect_success 'reverse version sort' '
 	test_cmp expect actual
 '
 
+test_expect_success 'improper usage of %(if), %(then), %(else) and %(end) atoms' '
+	test_must_fail git for-each-ref --format="%(if)" &&
+	test_must_fail git for-each-ref --format="%(then)" &&
+	test_must_fail git for-each-ref --format="%(else)" &&
+	test_must_fail git for-each-ref --format="%(if) %(else)" &&
+	test_must_fail git for-each-ref --format="%(then) %(else)" &&
+	test_must_fail git for-each-ref --format="%(if) %(else)" &&
+	test_must_fail git for-each-ref --format="%(if) %(then) %(else)"
+'
+
+test_expect_success 'check %(if)...%(then)...%(end) atoms' '
+	git for-each-ref --format="%(if)%(authorname)%(then)%(authorname): %(refname)%(end)" >actual &&
+	cat >expect <<-\EOF &&
+	A U Thor: refs/heads/master
+	A U Thor: refs/heads/side
+	A U Thor: refs/odd/spot
+	
+	A U Thor: refs/tags/foo1.10
+	A U Thor: refs/tags/foo1.3
+	A U Thor: refs/tags/foo1.6
+	A U Thor: refs/tags/four
+	A U Thor: refs/tags/one
+	
+	A U Thor: refs/tags/three
+	A U Thor: refs/tags/two
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'check %(if)...%(then)...%(else)...%(end) atoms' '
+	git for-each-ref --format="%(if)%(authorname)%(then)%(authorname)%(else)No author%(end): %(refname)" >actual &&
+	cat >expect <<-\EOF &&
+	A U Thor: refs/heads/master
+	A U Thor: refs/heads/side
+	A U Thor: refs/odd/spot
+	No author: refs/tags/double-tag
+	A U Thor: refs/tags/foo1.10
+	A U Thor: refs/tags/foo1.3
+	A U Thor: refs/tags/foo1.6
+	A U Thor: refs/tags/four
+	A U Thor: refs/tags/one
+	No author: refs/tags/signed-tag
+	A U Thor: refs/tags/three
+	A U Thor: refs/tags/two
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'ignore spaces in %(if) atom usage' '
+	git for-each-ref --format="%(refname:short): %(if)%(HEAD)%(then)Head ref%(else)Not Head ref%(end)" >actual &&
+	cat >expect <<-\EOF &&
+	master: Head ref
+	side: Not Head ref
+	odd/spot: Not Head ref
+	double-tag: Not Head ref
+	foo1.10: Not Head ref
+	foo1.3: Not Head ref
+	foo1.6: Not Head ref
+	four: Not Head ref
+	one: Not Head ref
+	signed-tag: Not Head ref
+	three: Not Head ref
+	two: Not Head ref
+	EOF
+	test_cmp expect actual
+'
+
 test_done
-- 
2.6.0

[PATCH v2 02/10] ref-filter: implement %(if:equals=<string>) and %(if:notequals=<string>)

[Karthik Nayak]

Implement %(if:equals=<string>) wherein the if condition is only
satisfied if the value obtained between the %(if:...) and %(then) atom
is the same as the given '<string>'.

Similarly, implement (if:notequals=<string>) wherein the if condition
is only satisfied if the value obtained between the %(if:...) and
%(then) atom is differnt from the given '<string>'.

Add tests and Documentation for the same.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 Documentation/git-for-each-ref.txt |  4 +++-
 ref-filter.c                       | 28 ++++++++++++++++++++++++----
 t/t6302-for-each-ref-filter.sh     | 18 ++++++++++++++++++
 3 files changed, 45 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 7345acb..206d646 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -144,7 +144,9 @@ if::
 	printed. We ignore space when evaluating the string before
 	%(then), this is useful when we use the %(HEAD) atom which
 	prints either "*" or " " and we want to apply the 'if'
-	condition only on the 'HEAD' ref.
+	condition only on the 'HEAD' ref. Append ":equals=<string>" or
+	":notequals=<string>" to compare the value between the
+	%(if:...) and %(then) atoms with the given string.
 
 In addition to the above, for commit and tag objects, the header
 field names (`tree`, `parent`, `object`, `type`, and `tag`) can
diff --git a/ref-filter.c b/ref-filter.c
index 95f007c..359c76d 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -73,6 +73,8 @@ struct contents {
 };
 
 struct if_then_else {
+	const char *if_equals,
+		*not_equals;
 	unsigned int then_atom : 1,
 		else_atom : 1,
 		condition_satisfied : 1;
@@ -281,6 +283,14 @@ static void if_atom_handler(struct atom_value *atomv, struct ref_formatting_stat
 {
 	struct ref_formatting_stack *new;
 	struct if_then_else *if_then_else = xcalloc(sizeof(struct if_then_else), 1);
+	const char *valp;
+
+	if (skip_prefix(atomv->s, "equals=", &valp))
+		if_then_else->if_equals = valp;
+	else if (skip_prefix(atomv->s, "notequals=", &valp))
+		if_then_else->not_equals = valp;
+	else if (atomv->s[0])
+		die(_("format: unknown format if:%s"), atomv->s);
 
 	push_stack_element(&state->stack);
 	new = state->stack;
@@ -309,11 +319,19 @@ static void then_atom_handler(struct atom_value *atomv, struct ref_formatting_st
 	if (if_then_else->then_atom)
 		die(_("format: %%(then) atom used more than once"));
 	if_then_else->then_atom = 1;
+
 	/*
-	 * If there exists non-empty string between the 'if' and
-	 * 'then' atom then the 'if' condition is satisfied.
+	 * If the 'equals' or 'notequals' attribute is used then
+	 * perform the required comparison. If not, only non-empty
+	 * strings satisfy the 'if' condition.
 	 */
-	if (cur->output.len && !is_empty(cur->output.buf))
+	if (if_then_else->if_equals) {
+		if (!strcmp(if_then_else->if_equals, cur->output.buf))
+			if_then_else->condition_satisfied = 1;
+	} else 	if (if_then_else->not_equals) {
+		if (strcmp(if_then_else->not_equals, cur->output.buf))
+			if_then_else->condition_satisfied = 1;
+	} else if (cur->output.len && !is_empty(cur->output.buf))
 		if_then_else->condition_satisfied = 1;
 	strbuf_reset(&cur->output);
 }
@@ -1024,8 +1042,10 @@ static void populate_value(struct ref_array_item *ref)
 		} else if (!strcmp(name, "end")) {
 			v->handler = end_atom_handler;
 			continue;
-		} else if (!strcmp(name, "if")) {
+		} else if (match_atom_name(name, "if", &valp)) {
 			v->handler = if_atom_handler;
+			if (valp)
+				v->s = xstrdup(valp);
 			continue;
 		} else if (!strcmp(name, "then")) {
 			v->handler = then_atom_handler;
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index 617fa1f..f45ac1f 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -322,4 +322,22 @@ test_expect_success 'ignore spaces in %(if) atom usage' '
 	test_cmp expect actual
 '
 
+test_expect_success 'check %(if:equals=<string>)' '
+	git for-each-ref --format="%(if:equals=master)%(refname:short)%(then)Found master%(else)Not master%(end)" refs/heads/ >actual &&
+	cat >expect <<-\EOF &&
+	Found master
+	Not master
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'check %(if:notequals=<string>)' '
+	git for-each-ref --format="%(if:notequals=master)%(refname:short)%(then)Not master%(else)Found master%(end)" refs/heads/ >actual &&
+	cat >expect <<-\EOF &&
+	Found master
+	Not master
+	EOF
+	test_cmp expect actual
+'
+
 test_done
-- 
2.6.0

[PATCH v2 03/10] ref-filter: add support for %(refname:dir) and %(refname:base)

[Karthik Nayak]

Add the options `:dir` and `:base` to the %(refname) atom. The `:dir`
option gives the directory (the part after $GIT_DIR/) of the ref
without the refname. The `:base` option gives the base directory of
the given ref (i.e. the directory following $GIT_DIR/refs/).

Add tests and documentation for the same.
---
 Documentation/git-for-each-ref.txt |  4 +++-
 ref-filter.c                       | 23 +++++++++++++++++++
 t/t6302-for-each-ref-filter.sh     | 47 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 73 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 206d646..b8d33a1 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -92,7 +92,9 @@ 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.
+	abbreviation mode. For the base directory of the ref (i.e. foo
+	in refs/foo/bar/boz) append `:base`. For the entire directory
+	path append `:dir`.
 
 objecttype::
 	The type of the object (`blob`, `tree`, `commit`, `tag`).
diff --git a/ref-filter.c b/ref-filter.c
index 359c76d..94d8754 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1105,6 +1105,29 @@ static void populate_value(struct ref_array_item *ref)
 				else
 					v->s = "<>";
 				continue;
+			} else if (!strcmp(formatp, "dir") &&
+				   (starts_with(name, "refname"))) {
+				const char *sp, *ep, *tmp;
+
+				sp = tmp = ref->refname;
+				/*  Obtain refs/foo/bar/ from refname refs/foo/bar/abc */
+				do {
+					ep = tmp;
+					tmp = strchr(ep + 1, '/');
+				} while (tmp);
+				v->s = xstrndup(sp, ep - sp);
+				continue;
+			} else if (!strcmp(formatp, "base") &&
+				   (starts_with(name, "refname"))) {
+				const char *sp, *ep;
+
+				if (skip_prefix(ref->refname, "refs/", &sp)) {
+					ep = strchr(sp, '/');
+					if (!ep)
+						continue;
+					v->s = xstrndup(sp, ep - sp);
+				}
+				continue;
 			} else
 				die("unknown %.*s format %s",
 				    (int)(formatp - name), name, formatp);
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index f45ac1f..19a5075 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -340,4 +340,51 @@ test_expect_success 'check %(if:notequals=<string>)' '
 	test_cmp expect actual
 '
 
+test_expect_success 'update refs for %(refname:dir) and %(refname:base)' '
+	git update-ref refs/foo HEAD &&
+	git update-ref refs/foodir/bar/boz HEAD
+'
+
+test_expect_success 'check %(refname:dir)' '
+	git for-each-ref --format="%(refname:dir)" >actual &&
+	cat >expect <<-\EOF &&
+	refs
+	refs/foodir/bar
+	refs/heads
+	refs/heads
+	refs/odd
+	refs/tags
+	refs/tags
+	refs/tags
+	refs/tags
+	refs/tags
+	refs/tags
+	refs/tags
+	refs/tags
+	refs/tags
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'check %(refname:base)' '
+	git for-each-ref --format="%(refname:base)" >actual &&
+	cat >expect <<-\EOF &&
+	
+	foodir
+	heads
+	heads
+	odd
+	tags
+	tags
+	tags
+	tags
+	tags
+	tags
+	tags
+	tags
+	tags
+	EOF
+	test_cmp expect actual
+'
+
 test_done
-- 
2.6.0

[PATCH v2 04/10] ref-filter: modify "%(objectname:short)" to take length

[Karthik Nayak]

Add support for %(objectname:short=<length>) which would print the
abbreviated unique objectname of given length. When no length is
specified 7 is used. The minimum length is 4.

Add tests and documentation for the same.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 Documentation/git-for-each-ref.txt |  4 +++-
 ref-filter.c                       | 30 ++++++++++++++++++++++--------
 t/t6300-for-each-ref.sh            | 22 ++++++++++++++++++++++
 3 files changed, 47 insertions(+), 9 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index b8d33a1..c713ec0 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -104,7 +104,9 @@ objectsize::
 
 objectname::
 	The object name (aka SHA-1).
-	For a non-ambiguous abbreviation of the object name append `:short`.
+	For a non-ambiguous abbreviation of the object name append
+	`:short`.  The length can be explicitly specified by appending
+	`:short=<length>`.  Minimum length being 4.
 
 upstream::
 	The name of a local ref which can be considered ``upstream''
diff --git a/ref-filter.c b/ref-filter.c
index 94d8754..de4d451 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -464,14 +464,28 @@ static void *get_obj(const unsigned char *sha1, struct object **obj, unsigned lo
 static int grab_objectname(const char *name, const unsigned char *sha1,
 			    struct atom_value *v)
 {
-	if (!strcmp(name, "objectname")) {
-		char *s = xmalloc(41);
-		strcpy(s, sha1_to_hex(sha1));
-		v->s = s;
-		return 1;
-	}
-	if (!strcmp(name, "objectname:short")) {
-		v->s = xstrdup(find_unique_abbrev(sha1, DEFAULT_ABBREV));
+	const char *p;
+
+	if (match_atom_name(name, "objectname", &p)) {
+		/*  No arguments given, copy the entire objectname */
+		if (!p) {
+			char *s = xmalloc(41);
+			strcpy(s, sha1_to_hex(sha1));
+			v->s = s;
+		} else {
+			unsigned int length = DEFAULT_ABBREV;
+
+			if (skip_prefix(p, "short", &p)) {
+				if (p[0] == '=')
+					if (strtoul_ui(p + 1, 10, &length))
+						die(_("positive length expected with objectname:%s"), p + 1);
+			} else
+				die(_("format: unknown option entered with objectname:%s"), p);
+
+			if (length < MINIMUM_ABBREV)
+				length = MINIMUM_ABBREV;
+			v->s = xstrdup(find_unique_abbrev(sha1, length));
+		}
 		return 1;
 	}
 	return 0;
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 7c9bec7..6fc569e 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -385,6 +385,28 @@ test_expect_success 'Check short objectname format' '
 	test_cmp expected actual
 '
 
+cat >expected <<EOF
+$(git rev-parse --short=1 HEAD)
+EOF
+
+test_expect_success 'Check objectname:short format when size is less than MINIMUM_ABBREV' '
+	git for-each-ref --format="%(objectname:short=1)" refs/heads >actual &&
+	test_cmp expected actual
+'
+
+cat >expected <<EOF
+$(git rev-parse --short=10 HEAD)
+EOF
+
+test_expect_success 'Check objectname:short format' '
+	git for-each-ref --format="%(objectname:short=10)" refs/heads >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'Check objectname:short format for invalid input' '
+	test_must_fail git for-each-ref --format="%(objectname:short=-1)" refs/heads
+'
+
 test_expect_success 'Check for invalid refname format' '
 	test_must_fail git for-each-ref --format="%(refname:INVALID)"
 '
-- 
2.6.0

[PATCH v2 05/10] ref-filter: adopt get_head_description() from branch.c

[Karthik Nayak]

Copy the implementation of get_head_description() from branch.c.  This
gives a description of the HEAD ref if called. This is used as the
refname for the HEAD ref whenever the FILTER_REFS_DETACHED_HEAD option
is used. Make it public because we need it to calculate the length of
the HEAD refs description in branch.c:calc_maxwidth() when we port
branch.c to use ref-filter APIs.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 builtin/branch.c |  4 ++++
 ref-filter.c     | 38 ++++++++++++++++++++++++++++++++++++--
 ref-filter.h     |  2 ++
 3 files changed, 42 insertions(+), 2 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index b7a60f4..67ef9f1 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -355,6 +355,10 @@ static void add_verbose_info(struct strbuf *out, struct ref_array_item *item,
 	strbuf_release(&subject);
 }
 
+/*
+ * This is duplicated in ref-filter.c, will be removed when we adopt
+ * ref-filter's printing APIs.
+ */
 static char *get_head_description(void)
 {
 	struct strbuf desc = STRBUF_INIT;
diff --git a/ref-filter.c b/ref-filter.c
index de4d451..a3cc3af 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -13,6 +13,7 @@
 #include "utf8.h"
 #include "git-compat-util.h"
 #include "version.h"
+#include "wt-status.h"
 
 typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
 
@@ -916,6 +917,37 @@ static inline char *copy_advance(char *dst, const char *src)
 	return dst;
 }
 
+char *get_head_description(void)
+{
+	struct strbuf desc = STRBUF_INIT;
+	struct wt_status_state state;
+	memset(&state, 0, sizeof(state));
+	wt_status_get_state(&state, 1);
+	if (state.rebase_in_progress ||
+	    state.rebase_interactive_in_progress)
+		strbuf_addf(&desc, _("(no branch, rebasing %s)"),
+			    state.branch);
+	else if (state.bisect_in_progress)
+		strbuf_addf(&desc, _("(no branch, bisect started on %s)"),
+			    state.branch);
+	else if (state.detached_from) {
+		/* TRANSLATORS: make sure these match _("HEAD detached at ")
+		   and _("HEAD detached from ") in wt-status.c */
+		if (state.detached_at)
+			strbuf_addf(&desc, _("(HEAD detached at %s)"),
+				state.detached_from);
+		else
+			strbuf_addf(&desc, _("(HEAD detached from %s)"),
+				state.detached_from);
+	}
+	else
+		strbuf_addstr(&desc, _("(no branch)"));
+	free(state.branch);
+	free(state.onto);
+	free(state.detached_from);
+	return strbuf_detach(&desc, NULL);
+}
+
 /*
  * Parse the object referred by ref, and grab needed value.
  */
@@ -954,9 +986,11 @@ static void populate_value(struct ref_array_item *ref)
 			name++;
 		}
 
-		if (starts_with(name, "refname"))
+		if (starts_with(name, "refname")) {
 			refname = ref->refname;
-		else if (starts_with(name, "symref"))
+			if (ref->kind & FILTER_REFS_DETACHED_HEAD)
+				refname = get_head_description();
+		} else if (starts_with(name, "symref"))
 			refname = ref->symref ? ref->symref : "";
 		else if (starts_with(name, "upstream")) {
 			const char *branch_name;
diff --git a/ref-filter.h b/ref-filter.h
index 14d435e..4aea594 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -106,5 +106,7 @@ int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset);
 struct ref_sorting *ref_default_sorting(void);
 /*  Function to parse --merged and --no-merged options */
 int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset);
+/*  Get the current HEAD's description */
+char *get_head_description(void);
 
 #endif /*  REF_FILTER_H  */
-- 
2.6.0

[PATCH v2 06/10] ref-filter: introduce format_ref_array_item()

[Karthik Nayak]

To allow column display, we will need to first render the output in a
string list to allow print_columns() to compute the proper size of
each column before starting the actual output. Introduce the function
format_ref_array_item() that does the formatting of a ref_array_item
to an strbuf.

show_ref_array_item() is kept as a convenience wrapper around it which
obtains the strbuf and prints it the standard output.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 ref-filter.c | 16 ++++++++++++----
 ref-filter.h |  3 +++
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index a3cc3af..38e4424 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1756,10 +1756,10 @@ static void append_literal(const char *cp, const char *ep, struct ref_formatting
 	}
 }
 
-void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style)
+void format_ref_array_item(struct ref_array_item *info, const char *format,
+			   int quote_style, struct strbuf *final_buf)
 {
 	const char *cp, *sp, *ep;
-	struct strbuf *final_buf;
 	struct ref_formatting_state state = REF_FORMATTING_STATE_INIT;
 
 	state.quote_style = quote_style;
@@ -1789,9 +1789,17 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu
 	}
 	if (state.stack->prev)
 		die(_("format: %%(end) atom missing"));
-	final_buf = &state.stack->output;
-	fwrite(final_buf->buf, 1, final_buf->len, stdout);
+	strbuf_addbuf(final_buf, &state.stack->output);
 	pop_stack_element(&state.stack);
+}
+
+void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style)
+{
+	struct strbuf final_buf = STRBUF_INIT;
+
+	format_ref_array_item(info, format, quote_style, &final_buf);
+	fwrite(final_buf.buf, 1, final_buf.len, stdout);
+	strbuf_release(&final_buf);
 	putchar('\n');
 }
 
diff --git a/ref-filter.h b/ref-filter.h
index 4aea594..0014b92 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -98,6 +98,9 @@ int parse_ref_filter_atom(const char *atom, const char *ep);
 int verify_ref_format(const char *format);
 /*  Sort the given ref_array as per the ref_sorting provided */
 void ref_array_sort(struct ref_sorting *sort, struct ref_array *array);
+/*  Based on the given format and quote_style, fill the strbuf */
+void format_ref_array_item(struct ref_array_item *info, const char *format,
+			   int quote_style, struct strbuf *final_buf);
 /*  Print the ref using the given format and quote_style */
 void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style);
 /*  Callback function for parsing the sort option */
-- 
2.6.0

[PATCH v2 07/10] ref-filter: make %(upstream:track) prints "[gone]" for invalid upstreams

[Karthik Nayak]

Borrowing from branch.c's implementation print "[gone]" whenever an
unknown upstream ref is encountered instead of just ignoring it.

This makes sure that when branch.c is ported over to using ref-filter
APIs for printing, this feature is not lost.

Make changes to t/t6300-for-each-ref.sh to reflect this change.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 ref-filter.c            | 4 +++-
 t/t6300-for-each-ref.sh | 2 +-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 38e4424..6a38089 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1118,8 +1118,10 @@ static void populate_value(struct ref_array_item *ref)
 				char buf[40];
 
 				if (stat_tracking_info(branch, &num_ours,
-						       &num_theirs, NULL))
+						       &num_theirs, NULL)) {
+					v->s = "[gone]";
 					continue;
+				}
 
 				if (!num_ours && !num_theirs)
 					v->s = "";
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 6fc569e..4f620bf 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -359,7 +359,7 @@ test_expect_success 'Check that :track[short] cannot be used with other atoms' '
 
 test_expect_success 'Check that :track[short] works when upstream is invalid' '
 	cat >expected <<-\EOF &&
-
+	[gone]
 
 	EOF
 	test_when_finished "git config branch.master.merge refs/heads/master" &&
-- 
2.6.0

[PATCH v2 08/10] ref-filter: add support for %(upstream:track,nobracket)

[Karthik Nayak]

Add support for %(upstream:track,nobracket) which will print the
tracking information without the brackets (i.e. "ahead N, behind M").

Add test and documentation for the same.
---
 Documentation/git-for-each-ref.txt |  6 ++++--
 ref-filter.c                       | 28 +++++++++++++++++++++++-----
 t/t6300-for-each-ref.sh            |  9 +++++++++
 3 files changed, 36 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index c713ec0..a38cbf6 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -114,8 +114,10 @@ upstream::
 	`refname` above.  Additionally respects `:track` to show
 	"[ahead N, behind M]" and `:trackshort` to show the terse
 	version: ">" (ahead), "<" (behind), "<>" (ahead and behind),
-	or "=" (in sync).  Has no effect if the ref does not have
-	tracking information associated with it.
+	or "=" (in sync).  Append `:track,nobracket` to show tracking
+	information without brackets (i.e "ahead N, behind M").  Has
+	no effect if the ref does not have tracking information
+	associated with it.
 
 push::
 	The name of a local ref which represents the `@{push}` location
diff --git a/ref-filter.c b/ref-filter.c
index 6a38089..6044eb0 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1112,27 +1112,45 @@ static void populate_value(struct ref_array_item *ref)
 			if (!strcmp(formatp, "short"))
 				refname = shorten_unambiguous_ref(refname,
 						      warn_ambiguous_refs);
-			else if (!strcmp(formatp, "track") &&
+			else if (skip_prefix(formatp, "track", &valp) &&
+				 strcmp(formatp, "trackshort") &&
 				 (starts_with(name, "upstream") ||
 				  starts_with(name, "push"))) {
 				char buf[40];
+				unsigned int nobracket = 0;
+
+				if (!strcmp(valp, ",nobracket"))
+					nobracket = 1;
 
 				if (stat_tracking_info(branch, &num_ours,
 						       &num_theirs, NULL)) {
-					v->s = "[gone]";
+					if (nobracket)
+						v->s = "gone";
+					else
+						v->s = "[gone]";
 					continue;
 				}
 
 				if (!num_ours && !num_theirs)
 					v->s = "";
 				else if (!num_ours) {
-					sprintf(buf, "[behind %d]", num_theirs);
+					if (nobracket)
+						sprintf(buf, "behind %d", num_theirs);
+					else
+						sprintf(buf, "[behind %d]", num_theirs);
 					v->s = xstrdup(buf);
 				} else if (!num_theirs) {
-					sprintf(buf, "[ahead %d]", num_ours);
+					if (nobracket)
+						sprintf(buf, "ahead %d", num_ours);
+					else
+						sprintf(buf, "[ahead %d]", num_ours);
 					v->s = xstrdup(buf);
 				} else {
-					sprintf(buf, "[ahead %d, behind %d]",
+					if (nobracket)
+						sprintf(buf, "ahead %d, behind %d",
+							num_ours, num_theirs);
+					else
+						sprintf(buf, "[ahead %d, behind %d]",
 						num_ours, num_theirs);
 					v->s = xstrdup(buf);
 				}
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 4f620bf..7ab8bf8 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -344,6 +344,15 @@ test_expect_success 'Check upstream:track format' '
 '
 
 cat >expected <<EOF
+ahead 1
+EOF
+
+test_expect_success 'Check upstream:track,nobracket format' '
+	git for-each-ref --format="%(upstream:track,nobracket)" refs/heads >actual &&
+	test_cmp expected actual
+'
+
+cat >expected <<EOF
 >
 EOF
 
-- 
2.6.0

[PATCH v2 09/10] branch: use ref-filter printing APIs

[Karthik Nayak]

Port branch.c to use ref-filter APIs for printing. This clears out
most of the code used in branch.c for printing and replaces them with
calls made to the ref-filter library.

Introduce get_format() which gets the format required for printing of
refs. Make amendments to print_ref_list() to reflect these changes.

Change calc_maxwidth() to also account for the length of HEAD ref, by
calling ref-filter:get_head_discription().

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 builtin/branch.c         | 261 ++++++++++++-----------------------------------
 t/t6040-tracking-info.sh |   2 +-
 2 files changed, 66 insertions(+), 197 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 67ef9f1..4d8b570 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -35,12 +35,12 @@ static unsigned char head_sha1[20];
 
 static int branch_use_color = -1;
 static char branch_colors[][COLOR_MAXLEN] = {
-	GIT_COLOR_RESET,
-	GIT_COLOR_NORMAL,	/* PLAIN */
-	GIT_COLOR_RED,		/* REMOTE */
-	GIT_COLOR_NORMAL,	/* LOCAL */
-	GIT_COLOR_GREEN,	/* CURRENT */
-	GIT_COLOR_BLUE,		/* UPSTREAM */
+	"%(color:reset)",
+	"%(color:reset)",	/* PLAIN */
+	"%(color:red)",		/* REMOTE */
+	"%(color:reset)",	/* LOCAL */
+	"%(color:green)",	/* CURRENT */
+	"%(color:blue)",	/* UPSTREAM */
 };
 enum color_branch {
 	BRANCH_COLOR_RESET = 0,
@@ -271,192 +271,6 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 	return(ret);
 }
 
-static void fill_tracking_info(struct strbuf *stat, const char *branch_name,
-		int show_upstream_ref)
-{
-	int ours, theirs;
-	char *ref = NULL;
-	struct branch *branch = branch_get(branch_name);
-	const char *upstream;
-	struct strbuf fancy = STRBUF_INIT;
-	int upstream_is_gone = 0;
-	int added_decoration = 1;
-
-	if (stat_tracking_info(branch, &ours, &theirs, &upstream) < 0) {
-		if (!upstream)
-			return;
-		upstream_is_gone = 1;
-	}
-
-	if (show_upstream_ref) {
-		ref = shorten_unambiguous_ref(upstream, 0);
-		if (want_color(branch_use_color))
-			strbuf_addf(&fancy, "%s%s%s",
-					branch_get_color(BRANCH_COLOR_UPSTREAM),
-					ref, branch_get_color(BRANCH_COLOR_RESET));
-		else
-			strbuf_addstr(&fancy, ref);
-	}
-
-	if (upstream_is_gone) {
-		if (show_upstream_ref)
-			strbuf_addf(stat, _("[%s: gone]"), fancy.buf);
-		else
-			added_decoration = 0;
-	} else if (!ours && !theirs) {
-		if (show_upstream_ref)
-			strbuf_addf(stat, _("[%s]"), fancy.buf);
-		else
-			added_decoration = 0;
-	} else if (!ours) {
-		if (show_upstream_ref)
-			strbuf_addf(stat, _("[%s: behind %d]"), fancy.buf, theirs);
-		else
-			strbuf_addf(stat, _("[behind %d]"), theirs);
-
-	} else if (!theirs) {
-		if (show_upstream_ref)
-			strbuf_addf(stat, _("[%s: ahead %d]"), fancy.buf, ours);
-		else
-			strbuf_addf(stat, _("[ahead %d]"), ours);
-	} else {
-		if (show_upstream_ref)
-			strbuf_addf(stat, _("[%s: ahead %d, behind %d]"),
-				    fancy.buf, ours, theirs);
-		else
-			strbuf_addf(stat, _("[ahead %d, behind %d]"),
-				    ours, theirs);
-	}
-	strbuf_release(&fancy);
-	if (added_decoration)
-		strbuf_addch(stat, ' ');
-	free(ref);
-}
-
-static void add_verbose_info(struct strbuf *out, struct ref_array_item *item,
-			     struct ref_filter *filter, const char *refname)
-{
-	struct strbuf subject = STRBUF_INIT, stat = STRBUF_INIT;
-	const char *sub = _(" **** invalid ref ****");
-	struct commit *commit = item->commit;
-
-	if (!parse_commit(commit)) {
-		pp_commit_easy(CMIT_FMT_ONELINE, commit, &subject);
-		sub = subject.buf;
-	}
-
-	if (item->kind == FILTER_REFS_BRANCHES)
-		fill_tracking_info(&stat, refname, filter->verbose > 1);
-
-	strbuf_addf(out, " %s %s%s",
-		find_unique_abbrev(item->commit->object.sha1, filter->abbrev),
-		stat.buf, sub);
-	strbuf_release(&stat);
-	strbuf_release(&subject);
-}
-
-/*
- * This is duplicated in ref-filter.c, will be removed when we adopt
- * ref-filter's printing APIs.
- */
-static char *get_head_description(void)
-{
-	struct strbuf desc = STRBUF_INIT;
-	struct wt_status_state state;
-	memset(&state, 0, sizeof(state));
-	wt_status_get_state(&state, 1);
-	if (state.rebase_in_progress ||
-	    state.rebase_interactive_in_progress)
-		strbuf_addf(&desc, _("(no branch, rebasing %s)"),
-			    state.branch);
-	else if (state.bisect_in_progress)
-		strbuf_addf(&desc, _("(no branch, bisect started on %s)"),
-			    state.branch);
-	else if (state.detached_from) {
-		/* TRANSLATORS: make sure these match _("HEAD detached at ")
-		   and _("HEAD detached from ") in wt-status.c */
-		if (state.detached_at)
-			strbuf_addf(&desc, _("(HEAD detached at %s)"),
-				state.detached_from);
-		else
-			strbuf_addf(&desc, _("(HEAD detached from %s)"),
-				state.detached_from);
-	}
-	else
-		strbuf_addstr(&desc, _("(no branch)"));
-	free(state.branch);
-	free(state.onto);
-	free(state.detached_from);
-	return strbuf_detach(&desc, NULL);
-}
-
-static void format_and_print_ref_item(struct ref_array_item *item, int maxwidth,
-				      struct ref_filter *filter, const char *remote_prefix)
-{
-	char c;
-	int current = 0;
-	int color;
-	struct strbuf out = STRBUF_INIT, name = STRBUF_INIT;
-	const char *prefix = "";
-	const char *desc = item->refname;
-	char *to_free = NULL;
-
-	switch (item->kind) {
-	case FILTER_REFS_BRANCHES:
-		skip_prefix(desc, "refs/heads/", &desc);
-		if (!filter->detached && !strcmp(desc, head))
-			current = 1;
-		else
-			color = BRANCH_COLOR_LOCAL;
-		break;
-	case FILTER_REFS_REMOTES:
-		skip_prefix(desc, "refs/remotes/", &desc);
-		color = BRANCH_COLOR_REMOTE;
-		prefix = remote_prefix;
-		break;
-	case FILTER_REFS_DETACHED_HEAD:
-		desc = to_free = get_head_description();
-		current = 1;
-		break;
-	default:
-		color = BRANCH_COLOR_PLAIN;
-		break;
-	}
-
-	c = ' ';
-	if (current) {
-		c = '*';
-		color = BRANCH_COLOR_CURRENT;
-	}
-
-	strbuf_addf(&name, "%s%s", prefix, desc);
-	if (filter->verbose) {
-		int utf8_compensation = strlen(name.buf) - utf8_strwidth(name.buf);
-		strbuf_addf(&out, "%c %s%-*s%s", c, branch_get_color(color),
-			    maxwidth + utf8_compensation, name.buf,
-			    branch_get_color(BRANCH_COLOR_RESET));
-	} else
-		strbuf_addf(&out, "%c %s%s%s", c, branch_get_color(color),
-			    name.buf, branch_get_color(BRANCH_COLOR_RESET));
-
-	if (item->symref) {
-		skip_prefix(item->symref, "refs/remotes/", &desc);
-		strbuf_addf(&out, " -> %s", desc);
-	}
-	else if (filter->verbose)
-		/* " f7c0c00 [ahead 58, behind 197] vcs-svn: drop obj_pool.h" */
-		add_verbose_info(&out, item, filter, desc);
-	if (column_active(colopts)) {
-		assert(!filter->verbose && "--column and --verbose are incompatible");
-		string_list_append(&output, out.buf);
-	} else {
-		printf("%s\n", out.buf);
-	}
-	strbuf_release(&name);
-	strbuf_release(&out);
-	free(to_free);
-}
-
 static int calc_maxwidth(struct ref_array *refs, int remote_bonus)
 {
 	int i, max = 0;
@@ -467,7 +281,10 @@ static int calc_maxwidth(struct ref_array *refs, int remote_bonus)
 
 		skip_prefix(it->refname, "refs/heads/", &desc);
 		skip_prefix(it->refname, "refs/remotes/", &desc);
-		w = utf8_strwidth(desc);
+		if (it->kind == FILTER_REFS_DETACHED_HEAD)
+			w = strlen(get_head_description());
+		else
+			w = utf8_strwidth(desc);
 
 		if (it->kind == FILTER_REFS_REMOTES)
 			w += remote_bonus;
@@ -477,12 +294,51 @@ static int calc_maxwidth(struct ref_array *refs, int remote_bonus)
 	return max;
 }
 
+static char *build_format(struct ref_filter *filter, int maxwidth, const char *remote_prefix)
+{
+	struct strbuf fmt = STRBUF_INIT;
+	struct strbuf local = STRBUF_INIT;
+	struct strbuf remote = STRBUF_INIT;
+
+	strbuf_addf(&fmt, "%%(if)%%(HEAD)%%(then)* %s%%(else)  %%(end)", branch_get_color(BRANCH_COLOR_CURRENT));
+
+	if (filter->verbose) {
+		strbuf_addf(&local, "%%(align:%d,left)%%(refname:short)%%(end)", maxwidth);
+		strbuf_addf(&local, "%s", branch_get_color(BRANCH_COLOR_RESET));
+		strbuf_addf(&local, " %%(objectname:short=7) ");
+
+		if (filter->verbose > 1)
+			strbuf_addf(&local, "%%(if)%%(upstream)%%(then)[%s%%(upstream:short)%s%%(if)%%(upstream:track)"
+				    "%%(then): %%(upstream:track,nobracket)%%(end)] %%(end)%%(contents:subject)",
+				    branch_get_color(BRANCH_COLOR_UPSTREAM), branch_get_color(BRANCH_COLOR_RESET));
+		else
+			strbuf_addf(&local, "%%(if)%%(upstream:track)%%(then)%%(upstream:track) %%(end)%%(contents:subject)");
+
+		strbuf_addf(&remote, "%s%%(align:%d,left)%s%%(refname:short)%%(end)%s%%(if)%%(symref)%%(then) -> %%(symref:short)"
+			    "%%(else) %%(objectname:short=7) %%(contents:subject)%%(end)",
+			    branch_get_color(BRANCH_COLOR_REMOTE), maxwidth,
+			    remote_prefix, branch_get_color(BRANCH_COLOR_RESET));
+	} else {
+		strbuf_addf(&local, "%%(refname:short)%s", branch_get_color(BRANCH_COLOR_RESET));
+		strbuf_addf(&remote, "%s%s%%(refname:short)%s%%(if)%%(symref)%%(then) -> %%(symref:short)%%(end)",
+			    branch_get_color(BRANCH_COLOR_REMOTE), remote_prefix, branch_get_color(BRANCH_COLOR_RESET));
+	}
+
+	strbuf_addf(&fmt, "%%(if:notequals=remotes)%%(refname:base)%%(then)%s%%(else)%s%%(end)", local.buf, remote.buf);
+
+	strbuf_release(&local);
+	strbuf_release(&remote);
+	return strbuf_detach(&fmt, NULL);
+}
+
 static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sorting)
 {
 	int i;
 	struct ref_array array;
 	int maxwidth = 0;
 	const char *remote_prefix = "";
+	struct strbuf out = STRBUF_INIT;
+	char *format;
 
 	/*
 	 * If we are listing more than just remote branches,
@@ -494,12 +350,14 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
 
 	memset(&array, 0, sizeof(array));
 
-	verify_ref_format("%(refname)%(symref)");
 	filter_refs(&array, filter, filter->kind | FILTER_REFS_INCLUDE_BROKEN);
 
 	if (filter->verbose)
 		maxwidth = calc_maxwidth(&array, strlen(remote_prefix));
 
+	format = build_format(filter, maxwidth, remote_prefix);
+	verify_ref_format(format);
+
 	/*
 	 * If no sorting parameter is given then we default to sorting
 	 * by 'refname'. This would give us an alphabetically sorted
@@ -511,10 +369,21 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
 		sorting = ref_default_sorting();
 	ref_array_sort(sorting, &array);
 
-	for (i = 0; i < array.nr; i++)
-		format_and_print_ref_item(array.items[i], maxwidth, filter, remote_prefix);
+	for (i = 0; i < array.nr; i++) {
+		format_ref_array_item(array.items[i], format, 0, &out);
+		if (column_active(colopts)) {
+			assert(!filter->verbose && "--column and --verbose are incompatible");
+			 /* format to a string_list to let print_columns() do its job */
+			string_list_append(&output, out.buf);
+		} else {
+			fwrite(out.buf, 1, out.len, stdout);
+			putchar('\n');
+		}
+		strbuf_release(&out);
+	}
 
 	ref_array_clear(&array);
+	free(format);
 }
 
 static void rename_branch(const char *oldname, const char *newname, int force)
diff --git a/t/t6040-tracking-info.sh b/t/t6040-tracking-info.sh
index 3d5c238..97a0765 100755
--- a/t/t6040-tracking-info.sh
+++ b/t/t6040-tracking-info.sh
@@ -44,7 +44,7 @@ b1 [ahead 1, behind 1] d
 b2 [ahead 1, behind 1] d
 b3 [behind 1] b
 b4 [ahead 2] f
-b5 g
+b5 [gone] g
 b6 c
 EOF
 
-- 
2.6.0

[PATCH v2 10/10] branch: implement '--format' option

[Karthik Nayak]

Implement the '--format' option provided by 'ref-filter'.
This lets the user list tags as per desired format similar
to the implementation in 'git for-each-ref'.

Add tests and documentation for the same.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 Documentation/git-branch.txt |  7 ++++++-
 builtin/branch.c             | 14 +++++++++-----
 t/t3203-branch-output.sh     | 11 +++++++++++
 3 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index 03c7af1..5227206 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -12,7 +12,7 @@ SYNOPSIS
 	[--list] [-v [--abbrev=<length> | --no-abbrev]]
 	[--column[=<options>] | --no-column]
 	[(--merged | --no-merged | --contains) [<commit>]] [--sort=<key>]
-	[--points-at <object>] [<pattern>...]
+	[--points-at <object>] [--format=<format>] [<pattern>...]
 'git branch' [--set-upstream | --track | --no-track] [-l] [-f] <branchname> [<start-point>]
 'git branch' (--set-upstream-to=<upstream> | -u <upstream>) [<branchname>]
 'git branch' --unset-upstream [<branchname>]
@@ -244,6 +244,11 @@ start-point is either a local or remote-tracking branch.
 --points-at <object>::
 	Only list branches of the given object.
 
+--format <format>::
+	A string that interpolates `%(fieldname)` from the object
+	pointed at by a ref being shown.  The format is the same as
+	that of linkgit:git-for-each-ref[1].
+
 Examples
 --------
 
diff --git a/builtin/branch.c b/builtin/branch.c
index 4d8b570..2356e72 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -27,6 +27,7 @@ static const char * const builtin_branch_usage[] = {
 	N_("git branch [<options>] [-r] (-d | -D) <branch-name>..."),
 	N_("git branch [<options>] (-m | -M) [<old-branch>] <new-branch>"),
 	N_("git branch [<options>] [-r | -a] [--points-at]"),
+	N_("git branch [<options>] [-r | -a] [--format]"),
 	NULL
 };
 
@@ -331,14 +332,14 @@ static char *build_format(struct ref_filter *filter, int maxwidth, const char *r
 	return strbuf_detach(&fmt, NULL);
 }
 
-static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sorting)
+static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sorting, const char *format)
 {
 	int i;
 	struct ref_array array;
 	int maxwidth = 0;
 	const char *remote_prefix = "";
 	struct strbuf out = STRBUF_INIT;
-	char *format;
+	char *to_free = NULL;
 
 	/*
 	 * If we are listing more than just remote branches,
@@ -355,7 +356,8 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
 	if (filter->verbose)
 		maxwidth = calc_maxwidth(&array, strlen(remote_prefix));
 
-	format = build_format(filter, maxwidth, remote_prefix);
+	if (!format)
+		format = to_free = build_format(filter, maxwidth, remote_prefix);
 	verify_ref_format(format);
 
 	/*
@@ -383,7 +385,7 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
 	}
 
 	ref_array_clear(&array);
-	free(format);
+	free(to_free);
 }
 
 static void rename_branch(const char *oldname, const char *newname, int force)
@@ -484,6 +486,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	enum branch_track track;
 	struct ref_filter filter;
 	static struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;
+	const char *format = NULL;
 
 	struct option options[] = {
 		OPT_GROUP(N_("Generic options")),
@@ -524,6 +527,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 			OPTION_CALLBACK, 0, "points-at", &filter.points_at, N_("object"),
 			N_("print only branches of the object"), 0, parse_opt_object_name
 		},
+		OPT_STRING(  0 , "format", &format, N_("format"), N_("format to use for the output")),
 		OPT_END(),
 	};
 
@@ -582,7 +586,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		if ((filter.kind & FILTER_REFS_BRANCHES) && filter.detached)
 			filter.kind |= FILTER_REFS_DETACHED_HEAD;
 		filter.name_patterns = argv;
-		print_ref_list(&filter, sorting);
+		print_ref_list(&filter, sorting, format);
 		print_columns(&output, colopts, NULL);
 		string_list_clear(&output, 0);
 		return 0;
diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
index f1ae5ff..a475ff1 100755
--- a/t/t3203-branch-output.sh
+++ b/t/t3203-branch-output.sh
@@ -163,4 +163,15 @@ test_expect_success 'git branch --points-at option' '
 	test_cmp expect actual
 '
 
+test_expect_success 'git branch --format option' '
+	cat >expect <<-\EOF &&
+	Refname is (HEAD detached from fromtag)
+	Refname is refs/heads/branch-one
+	Refname is refs/heads/branch-two
+	Refname is refs/heads/master
+	EOF
+	git branch --format="Refname is %(refname)" >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.6.0

Re: [PATCH v2 00/10] port branch.c to use ref-filter's printing options

[Matthieu Moy]

Karthik Nayak <karthik.188@gmail.com> writes:

> +An example to show the usage of %(if)...%(then)...%(else)...%(end).
> +This prefixes the current branch with a star.
> +
> +------------
> +#!/bin/sh
> +
> +git for-each-ref --format="%(if)%(HEAD)%(then)* %(else)  %(end)%(refname:short)" refs/heads/

I don't think the #!/bin/sh adds any value here. Just the 'git
for-each-ref' line is sufficient IMHO.

> +An example to show the usage of %(if)...%(then)...%(end).
> +This adds a red color to authorname, if present

I don't think this is such a good example.
%(color:red)%(authorname)%(color:reset) just works even if %(authorname)
is empty.

A better example would be

git for-each-ref --format='%(if)%(authorname)%(then)Authored by %(authorname)%(end)'

which avoids writting "Authored by " with no author.

> -static int is_empty(const char * s){
> +static int is_empty(const char *s){

You still have the { on the declaration line, it should be on the next
line.

> @@ -309,10 +311,14 @@ static int is_empty(const char * s){
>  static void then_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
>  {
>  	struct ref_formatting_stack *cur = state->stack;
> -	struct if_then_else *if_then_else = (struct if_then_else *)cur->at_end_data;
> +	struct if_then_else *if_then_else = NULL;
>  
> +	if (cur->at_end == if_then_else_handler)
> +		if_then_else = (struct if_then_else *)cur->at_end_data;

OK, now the cast is safe since at_end_data has to be of type struct
if_then_else * if at_end is if_then_else_handler.

> +				unsigned int nobracket = 0;
> +
> +				if (!strcmp(valp, ",nobracket"))
> +					nobracket = 1;

The code to parse comma-separated values is different here and
elsewhere. I'd rather have the same code (possibly factored into a
helper function), both to get consistent behavior (you're not allowing
%(upstream:nobracket,track) for example, right?) and consistent code.

>  				if (!num_ours && !num_theirs)
>  					v->s = "";
>  				else if (!num_ours) {
> -					sprintf(buf, "[behind %d]", num_theirs);
> +					if (nobracket)
> +						sprintf(buf, "behind %d", num_theirs);
> +					else
> +						sprintf(buf, "[behind %d]", num_theirs);

Perhaps use sprintf(buf, "%sbehind %d%s", obracket, num_their, cbracket)
unconditionnally, and set obracket = "" or obracket = "[" once and for
all when you test for "nobracket" above. This avoids these "if
(nobracket)" spread accross the code, but at the price of extra %s in
the format strings.

> @@ -1170,6 +1173,29 @@ static void populate_value(struct ref_array_item *ref)
>  				else
>  					v->s = "<>";
>  				continue;
> +			} else if (!strcmp(formatp, "dir") &&
> +				   (starts_with(name, "refname"))) {
> +				const char *sp, *ep, *tmp;
> +
> +				sp = tmp = ref->refname;
> +				/*  Obtain refs/foo/bar/ from refname refs/foo/bar/abc */
> +				do {
> +					ep = tmp;
> +					tmp = strchr(ep + 1, '/');
> +				} while (tmp);

To look for the last occurence of '/' you can also use strrchr().
Doesn't it do what you want here?

> --- a/t/t6040-tracking-info.sh
> +++ b/t/t6040-tracking-info.sh
> @@ -58,11 +58,11 @@ test_expect_success 'branch -v' '
>  '
>  
>  cat >expect <<\EOF
> -b1 [origin/master] [ahead 1, behind 1] d
> -b2 [origin/master] [ahead 1, behind 1] d
> -b3 [origin/master] [behind 1] b
> -b4 [origin/master] [ahead 2] f
> -b5 [brokenbase] [gone] g
> +b1 [origin/master: ahead 1, behind 1] d
> +b2 [origin/master: ahead 1, behind 1] d
> +b3 [origin/master: behind 1] b
> +b4 [origin/master: ahead 2] f
> +b5 [brokenbase: gone] g
>  b6 [origin/master] c
>  EOF

Cool!

I didn't go through the patches themselves, but modulo my remarks above
the interdiff looks good. Thanks.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v2 00/10] port branch.c to use ref-filter's printing options

[Karthik Nayak]

On Thu, Oct 8, 2015 at 5:57 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> +An example to show the usage of %(if)...%(then)...%(else)...%(end).
>> +This prefixes the current branch with a star.
>> +
>> +------------
>> +#!/bin/sh
>> +
>> +git for-each-ref --format="%(if)%(HEAD)%(then)* %(else)  %(end)%(refname:short)" refs/heads/
>
> I don't think the #!/bin/sh adds any value here. Just the 'git
> for-each-ref' line is sufficient IMHO.
>

Sure, we can remove that.

>> +An example to show the usage of %(if)...%(then)...%(end).
>> +This adds a red color to authorname, if present
>
> I don't think this is such a good example.
> %(color:red)%(authorname)%(color:reset) just works even if %(authorname)
> is empty.
>
> A better example would be
>
> git for-each-ref --format='%(if)%(authorname)%(then)Authored by %(authorname)%(end)'
>
> which avoids writting "Authored by " with no author.
>

Yeah, will include this.

>> -static int is_empty(const char * s){
>> +static int is_empty(const char *s){
>
> You still have the { on the declaration line, it should be on the next
> line.
>

Oops, will change that.

>> @@ -309,10 +311,14 @@ static int is_empty(const char * s){
>>  static void then_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
>>  {
>>       struct ref_formatting_stack *cur = state->stack;
>> -     struct if_then_else *if_then_else = (struct if_then_else *)cur->at_end_data;
>> +     struct if_then_else *if_then_else = NULL;
>>
>> +     if (cur->at_end == if_then_else_handler)
>> +             if_then_else = (struct if_then_else *)cur->at_end_data;
>
> OK, now the cast is safe since at_end_data has to be of type struct
> if_then_else * if at_end is if_then_else_handler.
>
>> +                             unsigned int nobracket = 0;
>> +
>> +                             if (!strcmp(valp, ",nobracket"))
>> +                                     nobracket = 1;
>
> The code to parse comma-separated values is different here and
> elsewhere. I'd rather have the same code (possibly factored into a
> helper function), both to get consistent behavior (you're not allowing
> %(upstream:nobracket,track) for example, right?) and consistent code.
>

Eh, okay, will do that!

>>                               if (!num_ours && !num_theirs)
>>                                       v->s = "";
>>                               else if (!num_ours) {
>> -                                     sprintf(buf, "[behind %d]", num_theirs);
>> +                                     if (nobracket)
>> +                                             sprintf(buf, "behind %d", num_theirs);
>> +                                     else
>> +                                             sprintf(buf, "[behind %d]", num_theirs);
>
> Perhaps use sprintf(buf, "%sbehind %d%s", obracket, num_their, cbracket)
> unconditionnally, and set obracket = "" or obracket = "[" once and for
> all when you test for "nobracket" above. This avoids these "if
> (nobracket)" spread accross the code, but at the price of extra %s in
> the format strings.
>

Okay! will do that.

>> @@ -1170,6 +1173,29 @@ static void populate_value(struct ref_array_item *ref)
>>                               else
>>                                       v->s = "<>";
>>                               continue;
>> +                     } else if (!strcmp(formatp, "dir") &&
>> +                                (starts_with(name, "refname"))) {
>> +                             const char *sp, *ep, *tmp;
>> +
>> +                             sp = tmp = ref->refname;
>> +                             /*  Obtain refs/foo/bar/ from refname refs/foo/bar/abc */
>> +                             do {
>> +                                     ep = tmp;
>> +                                     tmp = strchr(ep + 1, '/');
>> +                             } while (tmp);
>
> To look for the last occurence of '/' you can also use strrchr().
> Doesn't it do what you want here?
>

Didn't know that, thanks will do that.

>> --- a/t/t6040-tracking-info.sh
>> +++ b/t/t6040-tracking-info.sh
>> @@ -58,11 +58,11 @@ test_expect_success 'branch -v' '
>>  '
>>
>>  cat >expect <<\EOF
>> -b1 [origin/master] [ahead 1, behind 1] d
>> -b2 [origin/master] [ahead 1, behind 1] d
>> -b3 [origin/master] [behind 1] b
>> -b4 [origin/master] [ahead 2] f
>> -b5 [brokenbase] [gone] g
>> +b1 [origin/master: ahead 1, behind 1] d
>> +b2 [origin/master: ahead 1, behind 1] d
>> +b3 [origin/master: behind 1] b
>> +b4 [origin/master: ahead 2] f
>> +b5 [brokenbase: gone] g
>>  b6 [origin/master] c
>>  EOF
>
> Cool!
>
> I didn't go through the patches themselves, but modulo my remarks above
> the interdiff looks good. Thanks.
>

Thanks for the review.

-- 
Regards,
Karthik Nayak

Re: [PATCH v2 00/10] port branch.c to use ref-filter's printing options

[Karthik Nayak]

On Thu, Oct 8, 2015 at 5:57 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
>> +                             unsigned int nobracket = 0;
>> +
>> +                             if (!strcmp(valp, ",nobracket"))
>> +                                     nobracket = 1;
>
> The code to parse comma-separated values is different here and
> elsewhere. I'd rather have the same code (possibly factored into a
> helper function), both to get consistent behavior (you're not allowing
> %(upstream:nobracket,track) for example, right?) and consistent code.
>

Speaking of comma-separated values, the only other place we use that is
in the align atom. Also I find this very specific to get a function out of.
Somehow I think this is the simplest way to go about this.

-- 
Regards,
Karthik Nayak

Re: [PATCH v2 00/10] port branch.c to use ref-filter's printing options

[Matthieu Moy]

Karthik Nayak <karthik.188@gmail.com> writes:

> On Thu, Oct 8, 2015 at 5:57 PM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>>> +                             unsigned int nobracket = 0;
>>> +
>>> +                             if (!strcmp(valp, ",nobracket"))
>>> +                                     nobracket = 1;
>>
>> The code to parse comma-separated values is different here and
>> elsewhere. I'd rather have the same code (possibly factored into a
>> helper function), both to get consistent behavior (you're not allowing
>> %(upstream:nobracket,track) for example, right?) and consistent code.
>>
>
> Speaking of comma-separated values, the only other place we use that is
> in the align atom. Also I find this very specific to get a function out of.
> Somehow I think this is the simplest way to go about this.

Well, most pieces of code start with one instance, then two, then
more ;-). When the second instance starts being different from the
first, it doesn't give a good example for the future third instance.

This particular piece of code is so important and I won't fight for a
better factored one, but in general "there are only two instances" is a
dubious argument to avoid refactoring.

Still, I find it weird to force the nobracket to be always at the same
position.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v2 00/10] port branch.c to use ref-filter's printing options

[Karthik Nayak]

On Thu, Oct 8, 2015 at 10:40 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> On Thu, Oct 8, 2015 at 5:57 PM, Matthieu Moy
>> <Matthieu.Moy@grenoble-inp.fr> wrote:
>>>> +                             unsigned int nobracket = 0;
>>>> +
>>>> +                             if (!strcmp(valp, ",nobracket"))
>>>> +                                     nobracket = 1;
>>>
>>> The code to parse comma-separated values is different here and
>>> elsewhere. I'd rather have the same code (possibly factored into a
>>> helper function), both to get consistent behavior (you're not allowing
>>> %(upstream:nobracket,track) for example, right?) and consistent code.
>>>
>>
>> Speaking of comma-separated values, the only other place we use that is
>> in the align atom. Also I find this very specific to get a function out of.
>> Somehow I think this is the simplest way to go about this.
>
> Well, most pieces of code start with one instance, then two, then
> more ;-). When the second instance starts being different from the
> first, it doesn't give a good example for the future third instance.
>

Totally agree with you here.

> This particular piece of code is so important and I won't fight for a
> better factored one, but in general "there are only two instances" is a
> dubious argument to avoid refactoring.
>
> Still, I find it weird to force the nobracket to be always at the same
> position.
>

No i mean I could follow up with the way we use it in align, but I don't see
how I can make a function out of this.

-- 
Regards,
Karthik Nayak

Re: [PATCH v2 00/10] port branch.c to use ref-filter's printing options

[Matthieu Moy]

Karthik Nayak <karthik.188@gmail.com> writes:

> On Thu, Oct 8, 2015 at 10:40 PM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>
>> This particular piece of code is so important and I won't fight for a
>> better factored one, but in general "there are only two instances" is a
>> dubious argument to avoid refactoring.
>>
>> Still, I find it weird to force the nobracket to be always at the same
>> position.
>>
>
> No i mean I could follow up with the way we use it in align, but I don't see
> how I can make a function out of this.

Ah, OK. Well, there are already two instances of

	/*  Strip trailing comma */
	if (s[1])
		strbuf_setlen(s[0], s[0]->len - 1);


(for objectname and align), one of which says

	/*
	 * TODO: Implement a function similar to strbuf_split_str()
	 * which would omit the separator from the end of each value.
	 */

I guess it's time to replace this TODO with actual code. A
straightforward implementation would be to call strbuf_split_str and
then use the same "if (s[1])" as you already have. There's probably a
more efficient way.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v2 00/10] port branch.c to use ref-filter's printing options

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> No i mean I could follow up with the way we use it in align, but I don't see
> how I can make a function out of this.

At least you should be able to pre-parse the %(<atom>:<modifier>)
construct, instead of doing strcmp() every time populate_value() is
called, no?  Then your parser would not be doing

>>>> +                             if (!strcmp(valp, ",nobracket"))
>>>> +                                     nobracket = 1;

with 'valp' that scans over 'name' (which is an element of
used_atom[], i.e. a name from valid_atom[] followed by ":<modifier>"
for each customization).  Instead your used_atom[] would be an array
of a data structure that is richer than just a string.

The <modifier> would be split at comma boundary and made into an
array of two field structure, as the most general form of it is

    <modifier> ::= <attrval> | <attrval> ',' <modifier>
    <attrval> ::= <attr> '=' <value> | <attr> | <value>

if we recall the discussion we had while designing %(align), i.e.

 * The most general form is "%(align:position=left,width=32)"

 * The value domain of attributes may be distinct, in which case the
   value itself could imply what attr it is talking about,
   e.g. "%(align:32,left)" is sufficiently clear as '32' cannot be
   position and 'left' cannot be width.

And clearly there can be an attr whose presense alone can imply its
value (iow, a boolean), even though your %(align) does not yet have
such a modifier.  It is easy to imagine that you would later want to
add %(align:position=left,width=32,truncate) that truncates the value
when its display width exceeds '32'.  The 'nobracket' above would be
an another example of a boolean (whose name is negative, which is
not a very good design in general, though).

Then used_atom[] could become something like

    struct {
    	const char *str; /* e.g. "align:position=left,32" */
	struct {
        	const char *part0; /* everything before '=' */
                const char *part1; /* optional */
	} *modifier;
        int modifier_nr;
    } *used_atom;

and "align:position=left,32" would be parsed into

	.str = "align:position=left,32";
        .modifier = { { "position", "left" }, { "32", NULL } };
        .modifier_nr = 2;

when the format string is read, which is done only once.

The looping over all the refs done in populate_value() could just
use the pre-parsed representation.

Hmm?

Re: [PATCH v2 07/10] ref-filter: make %(upstream:track) prints "[gone]" for invalid upstreams

[Matthieu Moy]

Karthik Nayak <karthik.188@gmail.com> writes:

> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1118,8 +1118,10 @@ static void populate_value(struct ref_array_item *ref)
>  				char buf[40];
>  
>  				if (stat_tracking_info(branch, &num_ours,
> -						       &num_theirs, NULL))
> +						       &num_theirs, NULL)) {
> +					v->s = "[gone]";

My remark about translation still holds. The string was previously
translated in "branch" and you are removing this translation (well, not
here, but when 09/10 starts using this code).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v2 01/10] ref-filter: implement %(if), %(then), and %(else) atoms

[Matthieu Moy]

Karthik Nayak <karthik.188@gmail.com> writes:

> +static void if_then_else_handler(struct ref_formatting_stack **stack)
> +{
> +	struct ref_formatting_stack *cur = *stack;
> +	struct ref_formatting_stack *prev = cur->prev;
> +	struct if_then_else *if_then_else = (struct if_then_else *)cur->at_end_data;
> +

You should add

	if (!if_then_else->then_atom)
		die(_("format: %%(if) atom used without a %%(then) atom"));

here ...

> +static void then_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
> +{
> +	struct ref_formatting_stack *cur = state->stack;
> +	struct if_then_else *if_then_else = NULL;
> +
> +	if (cur->at_end == if_then_else_handler)
> +		if_then_else = (struct if_then_else *)cur->at_end_data;
> +	if (!if_then_else)
> +		die(_("format: %%(then) atom used without an %%(if) atom"));
> +	if (if_then_else->then_atom)
> +		die(_("format: %%(then) atom used more than once"));
> +	if_then_else->then_atom = 1;

... and

	if (if_then_else->else_atom)
		die(_("format: %%(then) atom used after %%(else)"));

here, just in case (adding the two corresponding test_must_fail wouldn't
harm of course).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v2 02/10] ref-filter: implement %(if:equals=<string>) and %(if:notequals=<string>)

[Matthieu Moy]

Karthik Nayak <karthik.188@gmail.com> writes:

> @@ -309,11 +319,19 @@ static void then_atom_handler(struct atom_value *atomv, struct ref_formatting_st
>  	if (if_then_else->then_atom)
>  		die(_("format: %%(then) atom used more than once"));
>  	if_then_else->then_atom = 1;
> +
>  	/*

Useless new blank line.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v2 04/10] ref-filter: modify "%(objectname:short)" to take length

[Matthieu Moy]

Karthik Nayak <karthik.188@gmail.com> writes:

> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -464,14 +464,28 @@ static void *get_obj(const unsigned char *sha1, struct object **obj, unsigned lo
>  static int grab_objectname(const char *name, const unsigned char *sha1,
>  			    struct atom_value *v)
>  {
> -	if (!strcmp(name, "objectname")) {
> -		char *s = xmalloc(41);
> -		strcpy(s, sha1_to_hex(sha1));
> -		v->s = s;
> -		return 1;
> -	}
> -	if (!strcmp(name, "objectname:short")) {
> -		v->s = xstrdup(find_unique_abbrev(sha1, DEFAULT_ABBREV));
> +	const char *p;
> +
> +	if (match_atom_name(name, "objectname", &p)) {
> +		/*  No arguments given, copy the entire objectname */
> +		if (!p) {
> +			char *s = xmalloc(41);

While you're there, you may want to get rid of this hardcoded 41 and
write GIT_SHA1_HEXSZ + 1 instead.

> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index 7c9bec7..6fc569e 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -385,6 +385,28 @@ test_expect_success 'Check short objectname format' '
>  	test_cmp expected actual
>  '
>  
> +cat >expected <<EOF
> +$(git rev-parse --short=1 HEAD)
> +EOF

Please write all code within test_expect_success including this
(t/README:

 - Put all code inside test_expect_success and other assertions.

   Even code that isn't a test per se, but merely some setup code
   should be inside a test assertion.
).

> +test_expect_success 'Check objectname:short format when size is less than MINIMUM_ABBREV' '
> +	git for-each-ref --format="%(objectname:short=1)" refs/heads >actual &&
> +	test_cmp expected actual
> +'
> +
> +cat >expected <<EOF
> +$(git rev-parse --short=10 HEAD)
> +EOF

Likewise.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v2 05/10] ref-filter: adopt get_head_description() from branch.c

[Matthieu Moy]

Karthik Nayak <karthik.188@gmail.com> writes:

> Copy the implementation of get_head_description() from branch.c.  This
> gives a description of the HEAD ref if called. This is used as the
> refname for the HEAD ref whenever the FILTER_REFS_DETACHED_HEAD option
> is used. Make it public because we need it to calculate the length of
> the HEAD refs description in branch.c:calc_maxwidth() when we port
> branch.c to use ref-filter APIs.

If it's made public, then it could be simpler to just _move_ the
function instead of copying it. You'd need to add a #include
<ref-filter.c> to branch.c, but you're going to add one anyway.

Code movement is more "git blame" friendly than code copy, and as a
reviewer I'd rather see the code movement here and not hear about it
later in the series.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v2 08/10] ref-filter: add support for %(upstream:track,nobracket)

[Matthieu Moy]

Karthik Nayak <karthik.188@gmail.com> writes:

> Add support for %(upstream:track,nobracket) which will print the
> tracking information without the brackets (i.e. "ahead N, behind M").
>
> Add test and documentation for the same.
> ---
>  Documentation/git-for-each-ref.txt |  6 ++++--
>  ref-filter.c                       | 28 +++++++++++++++++++++++-----
>  t/t6300-for-each-ref.sh            |  9 +++++++++
>  3 files changed, 36 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index c713ec0..a38cbf6 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -114,8 +114,10 @@ upstream::
>  	`refname` above.  Additionally respects `:track` to show
>  	"[ahead N, behind M]" and `:trackshort` to show the terse
>  	version: ">" (ahead), "<" (behind), "<>" (ahead and behind),
> -	or "=" (in sync).  Has no effect if the ref does not have
> -	tracking information associated with it.
> +	or "=" (in sync).  Append `:track,nobracket` to show tracking
> +	information without brackets (i.e "ahead N, behind M").  Has
> +	no effect if the ref does not have tracking information
> +	associated with it.
>  
>  push::
>  	The name of a local ref which represents the `@{push}` location
> diff --git a/ref-filter.c b/ref-filter.c
> index 6a38089..6044eb0 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1112,27 +1112,45 @@ static void populate_value(struct ref_array_item *ref)
>  			if (!strcmp(formatp, "short"))
>  				refname = shorten_unambiguous_ref(refname,
>  						      warn_ambiguous_refs);
> -			else if (!strcmp(formatp, "track") &&
> +			else if (skip_prefix(formatp, "track", &valp) &&
> +				 strcmp(formatp, "trackshort") &&
>  				 (starts_with(name, "upstream") ||
>  				  starts_with(name, "push"))) {
>  				char buf[40];
> +				unsigned int nobracket = 0;
> +
> +				if (!strcmp(valp, ",nobracket"))
> +					nobracket = 1;
>  
>  				if (stat_tracking_info(branch, &num_ours,
>  						       &num_theirs, NULL)) {
> -					v->s = "[gone]";
> +					if (nobracket)
> +						v->s = "gone";
> +					else
> +						v->s = "[gone]";
>  					continue;
>  				}
>  
>  				if (!num_ours && !num_theirs)
>  					v->s = "";
>  				else if (!num_ours) {
> -					sprintf(buf, "[behind %d]", num_theirs);
> +					if (nobracket)
> +						sprintf(buf, "behind %d", num_theirs);
> +					else
> +						sprintf(buf, "[behind %d]", num_theirs);
>  					v->s = xstrdup(buf);
>  				} else if (!num_theirs) {
> -					sprintf(buf, "[ahead %d]", num_ours);
> +					if (nobracket)
> +						sprintf(buf, "ahead %d", num_ours);
> +					else
> +						sprintf(buf, "[ahead %d]", num_ours);
>  					v->s = xstrdup(buf);
>  				} else {
> -					sprintf(buf, "[ahead %d, behind %d]",
> +					if (nobracket)
> +						sprintf(buf, "ahead %d, behind %d",
> +							num_ours, num_theirs);
> +					else
> +						sprintf(buf, "[ahead %d, behind %d]",
>  						num_ours, num_theirs);
>  					v->s = xstrdup(buf);
>  				}
> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index 4f620bf..7ab8bf8 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -344,6 +344,15 @@ test_expect_success 'Check upstream:track format' '
>  '
>  
>  cat >expected <<EOF
> +ahead 1
> +EOF
> +
> +test_expect_success 'Check upstream:track,nobracket format' '
> +	git for-each-ref --format="%(upstream:track,nobracket)" refs/heads >actual &&
> +	test_cmp expected actual
> +'
> +
> +cat >expected <<EOF
>  >
>  EOF

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v2 08/10] ref-filter: add support for %(upstream:track,nobracket)

[Matthieu Moy]

Oops, sorry, I sent the wrong message, this one is empty. Please ignore.

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Add support for %(upstream:track,nobracket) which will print the
>> tracking information without the brackets (i.e. "ahead N, behind M").
>>
>> Add test and documentation for the same.
>> ---
>>  Documentation/git-for-each-ref.txt |  6 ++++--
>>  ref-filter.c                       | 28 +++++++++++++++++++++++-----
>>  t/t6300-for-each-ref.sh            |  9 +++++++++
>>  3 files changed, 36 insertions(+), 7 deletions(-)
>>
>> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
>> index c713ec0..a38cbf6 100644
>> --- a/Documentation/git-for-each-ref.txt
>> +++ b/Documentation/git-for-each-ref.txt
>> @@ -114,8 +114,10 @@ upstream::
>>  	`refname` above.  Additionally respects `:track` to show
>>  	"[ahead N, behind M]" and `:trackshort` to show the terse
>>  	version: ">" (ahead), "<" (behind), "<>" (ahead and behind),
>> -	or "=" (in sync).  Has no effect if the ref does not have
>> -	tracking information associated with it.
>> +	or "=" (in sync).  Append `:track,nobracket` to show tracking
>> +	information without brackets (i.e "ahead N, behind M").  Has
>> +	no effect if the ref does not have tracking information
>> +	associated with it.
>>  
>>  push::
>>  	The name of a local ref which represents the `@{push}` location
>> diff --git a/ref-filter.c b/ref-filter.c
>> index 6a38089..6044eb0 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -1112,27 +1112,45 @@ static void populate_value(struct ref_array_item *ref)
>>  			if (!strcmp(formatp, "short"))
>>  				refname = shorten_unambiguous_ref(refname,
>>  						      warn_ambiguous_refs);
>> -			else if (!strcmp(formatp, "track") &&
>> +			else if (skip_prefix(formatp, "track", &valp) &&
>> +				 strcmp(formatp, "trackshort") &&
>>  				 (starts_with(name, "upstream") ||
>>  				  starts_with(name, "push"))) {
>>  				char buf[40];
>> +				unsigned int nobracket = 0;
>> +
>> +				if (!strcmp(valp, ",nobracket"))
>> +					nobracket = 1;
>>  
>>  				if (stat_tracking_info(branch, &num_ours,
>>  						       &num_theirs, NULL)) {
>> -					v->s = "[gone]";
>> +					if (nobracket)
>> +						v->s = "gone";
>> +					else
>> +						v->s = "[gone]";
>>  					continue;
>>  				}
>>  
>>  				if (!num_ours && !num_theirs)
>>  					v->s = "";
>>  				else if (!num_ours) {
>> -					sprintf(buf, "[behind %d]", num_theirs);
>> +					if (nobracket)
>> +						sprintf(buf, "behind %d", num_theirs);
>> +					else
>> +						sprintf(buf, "[behind %d]", num_theirs);
>>  					v->s = xstrdup(buf);
>>  				} else if (!num_theirs) {
>> -					sprintf(buf, "[ahead %d]", num_ours);
>> +					if (nobracket)
>> +						sprintf(buf, "ahead %d", num_ours);
>> +					else
>> +						sprintf(buf, "[ahead %d]", num_ours);
>>  					v->s = xstrdup(buf);
>>  				} else {
>> -					sprintf(buf, "[ahead %d, behind %d]",
>> +					if (nobracket)
>> +						sprintf(buf, "ahead %d, behind %d",
>> +							num_ours, num_theirs);
>> +					else
>> +						sprintf(buf, "[ahead %d, behind %d]",
>>  						num_ours, num_theirs);
>>  					v->s = xstrdup(buf);
>>  				}
>> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
>> index 4f620bf..7ab8bf8 100755
>> --- a/t/t6300-for-each-ref.sh
>> +++ b/t/t6300-for-each-ref.sh
>> @@ -344,6 +344,15 @@ test_expect_success 'Check upstream:track format' '
>>  '
>>  
>>  cat >expected <<EOF
>> +ahead 1
>> +EOF
>> +
>> +test_expect_success 'Check upstream:track,nobracket format' '
>> +	git for-each-ref --format="%(upstream:track,nobracket)" refs/heads >actual &&
>> +	test_cmp expected actual
>> +'
>> +
>> +cat >expected <<EOF
>>  >
>>  EOF

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v2 01/10] ref-filter: implement %(if), %(then), and %(else) atoms

[Matthieu Moy]

Karthik Nayak <karthik.188@gmail.com> writes:

> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -134,9 +134,17 @@ align::
>  	`<position>` is either left, right or middle, default being
>  	left and `<width>` is the total length of the content with
>  	alignment. If the contents length is more than the width then
> -	no alignment is performed. If used with '--quote' everything
> -	in between %(align:...) and %(end) is quoted, but if nested
> -	then only the topmost level performs quoting.
> +	no alignment is performed.
> +
> +if::
> +	Used as %(if)..%(then)..(%end) or %(if)..%(then)..%(else)..%(end).

I guess you forgot to replace .. with ... (I think you agreed with me
that it was better).

> @@ -69,10 +72,16 @@ struct contents {
>  	struct object_id oid;
>  };
>  
> +struct if_then_else {
> +	unsigned int then_atom : 1,
> +		else_atom : 1,

Maybe "then_atom_seen" and "else_atom_seen" would be better names. Or
maybe they'd be too long, I leave it up to you.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v2 00/10] port branch.c to use ref-filter's printing options

[Matthieu Moy]

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

> Then used_atom[] could become something like
>
>     struct {
>     	const char *str; /* e.g. "align:position=left,32" */
> 	struct {
>         	const char *part0; /* everything before '=' */
>                 const char *part1; /* optional */
> 	} *modifier;
>         int modifier_nr;
>     } *used_atom;

If the goal is to prepare as much as possible when parsing the format
string, I'd even push it one step further and have stg like

     struct {
     	const char *str; /* e.g. "align:position=left,32" */
 	union {
         	struct {
			int position;
			enum { left, right, center } kind;
		} align;
                struct {
                	....;
                } objectname;
        int modifier_nr;
     } *used_atom;

Just a thought, I'm not sure how useful this would be, and this may be
too much change for this series (so it may deserve a separate topic).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v2 00/10] port branch.c to use ref-filter's printing options

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Then used_atom[] could become something like
>>
>>     struct {
>>     	const char *str; /* e.g. "align:position=left,32" */
>> 	struct {
>>         	const char *part0; /* everything before '=' */
>>                 const char *part1; /* optional */
>> 	} *modifier;
>>         int modifier_nr;
>>     } *used_atom;
>
> If the goal is to prepare as much as possible when parsing the format
> string, I'd even push it one step further and have stg like
>
>      struct {
>      	const char *str; /* e.g. "align:position=left,32" */
>  	union {
>          	struct {
> 			int position;
> 			enum { left, right, center } kind;
> 		} align;
>                 struct {
>                 	....;
>                 } objectname;
>         int modifier_nr;
>      } *used_atom;
>
> Just a thought, I'm not sure how useful this would be, and this may be
> too much change for this series (so it may deserve a separate topic).

Yes, if we are willing to enrich the element of valid_atom[] array
with a type-specific parsing functions, we could even do that.  Then
populate_value() would not have to do any parsing and just do the
filling.

I was shooting for a middle ground, but certainly with an eye
towards such an endgame state in the future.

Re: [PATCH v2 01/10] ref-filter: implement %(if), %(then), and %(else) atoms

[Karthik Nayak]

On Fri, Oct 9, 2015 at 12:18 AM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> +static void if_then_else_handler(struct ref_formatting_stack **stack)
>> +{
>> +     struct ref_formatting_stack *cur = *stack;
>> +     struct ref_formatting_stack *prev = cur->prev;
>> +     struct if_then_else *if_then_else = (struct if_then_else *)cur->at_end_data;
>> +
>
> You should add
>
>         if (!if_then_else->then_atom)
>                 die(_("format: %%(if) atom used without a %%(then) atom"));
>
> here ...
>

Will do.

>> +static void then_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
>> +{
>> +     struct ref_formatting_stack *cur = state->stack;
>> +     struct if_then_else *if_then_else = NULL;
>> +
>> +     if (cur->at_end == if_then_else_handler)
>> +             if_then_else = (struct if_then_else *)cur->at_end_data;
>> +     if (!if_then_else)
>> +             die(_("format: %%(then) atom used without an %%(if) atom"));
>> +     if (if_then_else->then_atom)
>> +             die(_("format: %%(then) atom used more than once"));
>> +     if_then_else->then_atom = 1;
>
> ... and
>
>         if (if_then_else->else_atom)
>                 die(_("format: %%(then) atom used after %%(else)"));
>
> here, just in case (adding the two corresponding test_must_fail wouldn't
> harm of course).
>

Will do, thanks!

-- 
Regards,
Karthik Nayak

Re: [PATCH v2 01/10] ref-filter: implement %(if), %(then), and %(else) atoms

[Karthik Nayak]

On Fri, Oct 9, 2015 at 12:49 AM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> --- a/Documentation/git-for-each-ref.txt
>> +++ b/Documentation/git-for-each-ref.txt
>> @@ -134,9 +134,17 @@ align::
>>       `<position>` is either left, right or middle, default being
>>       left and `<width>` is the total length of the content with
>>       alignment. If the contents length is more than the width then
>> -     no alignment is performed. If used with '--quote' everything
>> -     in between %(align:...) and %(end) is quoted, but if nested
>> -     then only the topmost level performs quoting.
>> +     no alignment is performed.
>> +
>> +if::
>> +     Used as %(if)..%(then)..(%end) or %(if)..%(then)..%(else)..%(end).
>
> I guess you forgot to replace .. with ... (I think you agreed with me
> that it was better).
>

Oops! I missed this.

>> @@ -69,10 +72,16 @@ struct contents {
>>       struct object_id oid;
>>  };
>>
>> +struct if_then_else {
>> +     unsigned int then_atom : 1,
>> +             else_atom : 1,
>
> Maybe "then_atom_seen" and "else_atom_seen" would be better names. Or
> maybe they'd be too long, I leave it up to you.
>

They do describe the usage in a better way, so why not.

-- 
Regards,
Karthik Nayak

Re: [PATCH v2 02/10] ref-filter: implement %(if:equals=<string>) and %(if:notequals=<string>)

[Karthik Nayak]

On Fri, Oct 9, 2015 at 12:21 AM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> @@ -309,11 +319,19 @@ static void then_atom_handler(struct atom_value *atomv, struct ref_formatting_st
>>       if (if_then_else->then_atom)
>>               die(_("format: %%(then) atom used more than once"));
>>       if_then_else->then_atom = 1;
>> +
>>       /*
>
> Useless new blank line.
>
> --
> Matthieu Moy
> http://www-verimag.imag.fr/~moy/

Will remove it, thanks!

-- 
Regards,
Karthik Nayak

Re: [PATCH v2 04/10] ref-filter: modify "%(objectname:short)" to take length

[Karthik Nayak]

On Fri, Oct 9, 2015 at 12:28 AM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -464,14 +464,28 @@ static void *get_obj(const unsigned char *sha1, struct object **obj, unsigned lo
>>  static int grab_objectname(const char *name, const unsigned char *sha1,
>>                           struct atom_value *v)
>>  {
>> -     if (!strcmp(name, "objectname")) {
>> -             char *s = xmalloc(41);
>> -             strcpy(s, sha1_to_hex(sha1));
>> -             v->s = s;
>> -             return 1;
>> -     }
>> -     if (!strcmp(name, "objectname:short")) {
>> -             v->s = xstrdup(find_unique_abbrev(sha1, DEFAULT_ABBREV));
>> +     const char *p;
>> +
>> +     if (match_atom_name(name, "objectname", &p)) {
>> +             /*  No arguments given, copy the entire objectname */
>> +             if (!p) {
>> +                     char *s = xmalloc(41);
>
> While you're there, you may want to get rid of this hardcoded 41 and
> write GIT_SHA1_HEXSZ + 1 instead.
>

Sure, will change that.

>> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
>> index 7c9bec7..6fc569e 100755
>> --- a/t/t6300-for-each-ref.sh
>> +++ b/t/t6300-for-each-ref.sh
>> @@ -385,6 +385,28 @@ test_expect_success 'Check short objectname format' '
>>       test_cmp expected actual
>>  '
>>
>> +cat >expected <<EOF
>> +$(git rev-parse --short=1 HEAD)
>> +EOF
>
> Please write all code within test_expect_success including this
> (t/README:
>
>  - Put all code inside test_expect_success and other assertions.
>
>    Even code that isn't a test per se, but merely some setup code
>    should be inside a test assertion.
> ).
>

Was just following the previous syntax, should have read that. fixed it

>> +test_expect_success 'Check objectname:short format when size is less than MINIMUM_ABBREV' '
>> +     git for-each-ref --format="%(objectname:short=1)" refs/heads >actual &&
>> +     test_cmp expected actual
>> +'
>> +
>> +cat >expected <<EOF
>> +$(git rev-parse --short=10 HEAD)
>> +EOF
>
> Likewise.
>

fixed it here too. Thanks

-- 
Regards,
Karthik Nayak

Re: [PATCH v2 05/10] ref-filter: adopt get_head_description() from branch.c

[Karthik Nayak]

On Fri, Oct 9, 2015 at 12:31 AM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Copy the implementation of get_head_description() from branch.c.  This
>> gives a description of the HEAD ref if called. This is used as the
>> refname for the HEAD ref whenever the FILTER_REFS_DETACHED_HEAD option
>> is used. Make it public because we need it to calculate the length of
>> the HEAD refs description in branch.c:calc_maxwidth() when we port
>> branch.c to use ref-filter APIs.
>
> If it's made public, then it could be simpler to just _move_ the
> function instead of copying it. You'd need to add a #include
> <ref-filter.c> to branch.c, but you're going to add one anyway.
>
> Code movement is more "git blame" friendly than code copy, and as a
> reviewer I'd rather see the code movement here and not hear about it
> later in the series.
>

Also this needs to be done, cause now compiling branch.c would give a
multiple declaration error. Will do as you suggested. Thanks

-- 
Regards,
Karthik Nayak

Re: [PATCH v2 07/10] ref-filter: make %(upstream:track) prints "[gone]" for invalid upstreams

[Karthik Nayak]

On Fri, Oct 9, 2015 at 12:10 AM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -1118,8 +1118,10 @@ static void populate_value(struct ref_array_item *ref)
>>                               char buf[40];
>>
>>                               if (stat_tracking_info(branch, &num_ours,
>> -                                                    &num_theirs, NULL))
>> +                                                    &num_theirs, NULL)) {
>> +                                     v->s = "[gone]";
>
> My remark about translation still holds. The string was previously
> translated in "branch" and you are removing this translation (well, not
> here, but when 09/10 starts using this code).
>

I should have mentioned in my cover letter, I didn't really understand
what has to be done about this, couldn't find much reference to go
about this. What do you suggest?

-- 
Regards,
Karthik Nayak

Re: [PATCH v2 00/10] port branch.c to use ref-filter's printing options

[Karthik Nayak]

On Fri, Oct 9, 2015 at 11:59 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> Then used_atom[] could become something like
>>>
>>>     struct {
>>>      const char *str; /* e.g. "align:position=left,32" */
>>>      struct {
>>>              const char *part0; /* everything before '=' */
>>>                 const char *part1; /* optional */
>>>      } *modifier;
>>>         int modifier_nr;
>>>     } *used_atom;
>>
>> If the goal is to prepare as much as possible when parsing the format
>> string, I'd even push it one step further and have stg like
>>
>>      struct {
>>       const char *str; /* e.g. "align:position=left,32" */
>>       union {
>>               struct {
>>                       int position;
>>                       enum { left, right, center } kind;
>>               } align;
>>                 struct {
>>                       ....;
>>                 } objectname;
>>         int modifier_nr;
>>      } *used_atom;
>>
>> Just a thought, I'm not sure how useful this would be, and this may be
>> too much change for this series (so it may deserve a separate topic).
>
> Yes, if we are willing to enrich the element of valid_atom[] array
> with a type-specific parsing functions, we could even do that.  Then
> populate_value() would not have to do any parsing and just do the
> filling.
>
> I was shooting for a middle ground, but certainly with an eye
> towards such an endgame state in the future.

I like the idea's here, I was trying out what you suggested before
Matthieu suggestion.

We could have:

static void parse_atom_modifiers(struct used_atom *atom)
{
    const char *sp = NULL;

    atom->modifier = NULL;
    atom->modifier_nr = 0;

    if ((sp = strchr(atom->str, ':'))) {
        while (sp[1]) {
            const char *equal, *comma, *ep;
            int no = atom->modifier_nr;

            atom->modifier_nr++;
            sp++;
            REALLOC_ARRAY(atom->modifier, atom->modifier_nr);

            equal = strchr(sp, '=');
            comma = strchr(sp, ',');

            if (comma)
                ep = comma;
            else
                ep = sp + strlen(sp);

            if (!equal) {
                atom->modifier[no].part0 = xstrndup(sp, ep - sp);
                atom->modifier[no].part1 = NULL;
            } else {
                atom->modifier[no].part0 = xstrndup(sp, equal - sp);
                atom->modifier[no].part1 = xstrndup(equal + 1, ep - equal - 1);
            }
            sp = ep;
        }
    }
}

or something on these lines for what you suggested. We could improve by
having a special parsing function for selected atoms and leave this to
be default.

Also does it make sense to integrate these changes here? Or would you like to
have another series on this?

-- 
Regards,
Karthik Nayak

Re: [PATCH v2 04/10] ref-filter: modify "%(objectname:short)" to take length

[Matthieu Moy]

Karthik Nayak <karthik.188@gmail.com> writes:

>>> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
>>> index 7c9bec7..6fc569e 100755
>>> --- a/t/t6300-for-each-ref.sh
>>> +++ b/t/t6300-for-each-ref.sh
>>> @@ -385,6 +385,28 @@ test_expect_success 'Check short objectname format' '
>>>       test_cmp expected actual
>>>  '
>>>
>>> +cat >expected <<EOF
>>> +$(git rev-parse --short=1 HEAD)
>>> +EOF
>>
>> Please write all code within test_expect_success including this
>> (t/README:
>>
>>  - Put all code inside test_expect_success and other assertions.
>>
>>    Even code that isn't a test per se, but merely some setup code
>>    should be inside a test assertion.
>> ).
>>
>
> Was just following the previous syntax, should have read that. fixed it

The common practice (not necessarily a rule, though) when you write code
next to other code that does not follow the style is:

* If it's not too disturbing, adopt the new style and keep the old code
  as-is. I think we are in this case.

* If the new and the old style do not mix well, prepend a "modernize
  style" patch to the series, and adopt the new style in the patch
  itself.

* If you're too lazy to do a "modernize style", adopt the old style for
  consistency.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v2 07/10] ref-filter: make %(upstream:track) prints "[gone]" for invalid upstreams

[Matthieu Moy]

Karthik Nayak <karthik.188@gmail.com> writes:

> On Fri, Oct 9, 2015 at 12:10 AM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>
>>> --- a/ref-filter.c
>>> +++ b/ref-filter.c
>>> @@ -1118,8 +1118,10 @@ static void populate_value(struct ref_array_item *ref)
>>>                               char buf[40];
>>>
>>>                               if (stat_tracking_info(branch, &num_ours,
>>> -                                                    &num_theirs, NULL))
>>> +                                                    &num_theirs, NULL)) {
>>> +                                     v->s = "[gone]";
>>
>> My remark about translation still holds. The string was previously
>> translated in "branch" and you are removing this translation (well, not
>> here, but when 09/10 starts using this code).
>>
>
> I should have mentioned in my cover letter, I didn't really understand
> what has to be done about this, couldn't find much reference to go
> about this. What do you suggest?

>From the user point of view :

git for-each-ref --format '%(upstream:track)' => Should always be the
same, because this may be parsed by scripts (plumbing). Should not
depend on $LANG, and shouldn't change from a version of Git to another.

git branch --format '%(upstream:track)' => Should show what is most
pleasant to the user (porcelain): translated according to $LANG and
friends, and may be improved in the future.

I already pointed out a fix where a string was translated in a plumbing
command. Another example is setup_unpack_trees_porcelain() in
unpack-trees.c which solves exactly the same problem.

I'll followup with a small series on top of yours to show the way. I did
not try to polish it since I guess you have local changes on the same
part of the code. Feel free to squash patches together or to squash them
with yours. The commit messages are not meant to be final either.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

[PATCH 1/3] fixup: use xstrfmt instead of fixed-size buf + sprintf + xstrdup

[Matthieu Moy]

The char buf[40] is safe (at least while the strings are not
translated), but I'd rather avoid magic numbers like this 40 in the
code, and use a construct that does not have this size limitation.
Especially if it makes the code shorter.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 ref-filter.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 6044eb0..7932c21 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1116,7 +1116,6 @@ static void populate_value(struct ref_array_item *ref)
 				 strcmp(formatp, "trackshort") &&
 				 (starts_with(name, "upstream") ||
 				  starts_with(name, "push"))) {
-				char buf[40];
 				unsigned int nobracket = 0;
 
 				if (!strcmp(valp, ",nobracket"))
@@ -1135,24 +1134,21 @@ static void populate_value(struct ref_array_item *ref)
 					v->s = "";
 				else if (!num_ours) {
 					if (nobracket)
-						sprintf(buf, "behind %d", num_theirs);
+						v->s = xstrfmt("behind %d", num_theirs);
 					else
-						sprintf(buf, "[behind %d]", num_theirs);
-					v->s = xstrdup(buf);
+						v->s = xstrfmt("[behind %d]", num_theirs);
 				} else if (!num_theirs) {
 					if (nobracket)
-						sprintf(buf, "ahead %d", num_ours);
+						v->s = xstrfmt("ahead %d", num_ours);
 					else
-						sprintf(buf, "[ahead %d]", num_ours);
-					v->s = xstrdup(buf);
+						v->s = xstrfmt("[ahead %d]", num_ours);
 				} else {
 					if (nobracket)
-						sprintf(buf, "ahead %d, behind %d",
-							num_ours, num_theirs);
+						v->s = xstrfmt("ahead %d, behind %d",
+							       num_ours, num_theirs);
 					else
-						sprintf(buf, "[ahead %d, behind %d]",
-						num_ours, num_theirs);
-					v->s = xstrdup(buf);
+						v->s = xstrfmt("[ahead %d, behind %d]",
+							       num_ours, num_theirs);
 				}
 				continue;
 			} else if (!strcmp(formatp, "trackshort") &&
-- 
2.6.0.rc2.24.gb06d8e9.dirty

[PATCH 2/3] ref-filter: allow porcelain to translate messages in the output

[Matthieu Moy]

This patch shows the way, but is obviously incomplete as it works only
for "nobracket" version. Actually, I think the code should first build
the unbracketed output string and then do something like

if (!nobracket) {
	const char *to_free = v->s;
	v->s = xstrfmt("[%s]", v->s);
	free(to_free);
}

so we don't have to worry about brackets anywhere else in the code.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 ref-filter.c | 28 ++++++++++++++++++++++++----
 ref-filter.h |  3 +++
 2 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 7932c21..c2ee8c9 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -15,6 +15,26 @@
 #include "version.h"
 #include "wt-status.h"
 
+static struct ref_msg {
+	const char *gone;
+	const char *ahead;
+	const char *behind;
+	const char *ahead_behind;
+} msgs = {
+	"gone",
+	"ahead %d",
+	"behind %d",
+	"ahead %d, behind %d"
+};
+
+void setup_ref_filter_porcelain_msg(void)
+{
+	msgs.gone = _("gone");
+	msgs.ahead = _("ahead %d");
+	msgs.behind = _("behind %d");
+	msgs.ahead_behind = _("ahead %d, behind %d");
+}
+
 typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
 
 static struct {
@@ -1124,7 +1144,7 @@ static void populate_value(struct ref_array_item *ref)
 				if (stat_tracking_info(branch, &num_ours,
 						       &num_theirs, NULL)) {
 					if (nobracket)
-						v->s = "gone";
+						v->s = msgs.gone;
 					else
 						v->s = "[gone]";
 					continue;
@@ -1134,17 +1154,17 @@ static void populate_value(struct ref_array_item *ref)
 					v->s = "";
 				else if (!num_ours) {
 					if (nobracket)
-						v->s = xstrfmt("behind %d", num_theirs);
+						v->s = xstrfmt(msgs.behind, num_theirs);
 					else
 						v->s = xstrfmt("[behind %d]", num_theirs);
 				} else if (!num_theirs) {
 					if (nobracket)
-						v->s = xstrfmt("ahead %d", num_ours);
+						v->s = xstrfmt(msgs.ahead, num_ours);
 					else
 						v->s = xstrfmt("[ahead %d]", num_ours);
 				} else {
 					if (nobracket)
-						v->s = xstrfmt("ahead %d, behind %d",
+						v->s = xstrfmt(msgs.ahead_behind,
 							       num_ours, num_theirs);
 					else
 						v->s = xstrfmt("[ahead %d, behind %d]",
diff --git a/ref-filter.h b/ref-filter.h
index 0014b92..2cce02c 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -112,4 +112,7 @@ int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset)
 /*  Get the current HEAD's description */
 char *get_head_description(void);
 
+/*  Set up translated strings in the output. */
+void setup_ref_filter_porcelain_msg(void);
+
 #endif /*  REF_FILTER_H  */
-- 
2.6.0.rc2.24.gb06d8e9.dirty

[PATCH 3/3] branch, tag: use porcelain output

[Matthieu Moy]

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 builtin/branch.c | 2 ++
 builtin/tag.c    | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/builtin/branch.c b/builtin/branch.c
index 9d6c062..041c649 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -531,6 +531,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		OPT_END(),
 	};
 
+	setup_ref_filter_porcelain_msg();
+
 	memset(&filter, 0, sizeof(filter));
 	filter.kind = FILTER_REFS_BRANCHES;
 	filter.abbrev = -1;
diff --git a/builtin/tag.c b/builtin/tag.c
index 9e17dca..b6d262f 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -372,6 +372,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
+	setup_ref_filter_porcelain_msg();
+
 	git_config(git_tag_config, sorting_tail);
 
 	memset(&opt, 0, sizeof(opt));
-- 
2.6.0.rc2.24.gb06d8e9.dirty

Re: [PATCH v2 00/10] port branch.c to use ref-filter's printing options

[Matthieu Moy]

Karthik Nayak <karthik.188@gmail.com> writes:

> Also does it make sense to integrate these changes here? Or would you like to
> have another series on this?

To me, the important in this series is to avoid introducing duplicated
and inconsistent code, because it would make further refactoring harder.

But this series starts getting close to finished, so you should not try
to add to much to it (at least for me: I spend some time reviewing v2
and I have an idea of what the interdiff should look like, I'd rather
avoid having new distraction in the v2->v3 interdiff).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v2 04/10] ref-filter: modify "%(objectname:short)" to take length

[Karthik Nayak]

On Sun, Oct 11, 2015 at 9:35 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>>>> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
>>>> index 7c9bec7..6fc569e 100755
>>>> --- a/t/t6300-for-each-ref.sh
>>>> +++ b/t/t6300-for-each-ref.sh
>>>> @@ -385,6 +385,28 @@ test_expect_success 'Check short objectname format' '
>>>>       test_cmp expected actual
>>>>  '
>>>>
>>>> +cat >expected <<EOF
>>>> +$(git rev-parse --short=1 HEAD)
>>>> +EOF
>>>
>>> Please write all code within test_expect_success including this
>>> (t/README:
>>>
>>>  - Put all code inside test_expect_success and other assertions.
>>>
>>>    Even code that isn't a test per se, but merely some setup code
>>>    should be inside a test assertion.
>>> ).
>>>
>>
>> Was just following the previous syntax, should have read that. fixed it
>
> The common practice (not necessarily a rule, though) when you write code
> next to other code that does not follow the style is:
>
> * If it's not too disturbing, adopt the new style and keep the old code
>   as-is. I think we are in this case.
>
> * If the new and the old style do not mix well, prepend a "modernize
>   style" patch to the series, and adopt the new style in the patch
>   itself.
>
> * If you're too lazy to do a "modernize style", adopt the old style for
>   consistency.
>

Ah thanks, Makes sense :)

-- 
Regards,
Karthik Nayak

Re: [PATCH v2 07/10] ref-filter: make %(upstream:track) prints "[gone]" for invalid upstreams

[Karthik Nayak]

On Sun, Oct 11, 2015 at 9:42 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> On Fri, Oct 9, 2015 at 12:10 AM, Matthieu Moy
>> <Matthieu.Moy@grenoble-inp.fr> wrote:
>>> Karthik Nayak <karthik.188@gmail.com> writes:
>>>
>>>> --- a/ref-filter.c
>>>> +++ b/ref-filter.c
>>>> @@ -1118,8 +1118,10 @@ static void populate_value(struct ref_array_item *ref)
>>>>                               char buf[40];
>>>>
>>>>                               if (stat_tracking_info(branch, &num_ours,
>>>> -                                                    &num_theirs, NULL))
>>>> +                                                    &num_theirs, NULL)) {
>>>> +                                     v->s = "[gone]";
>>>
>>> My remark about translation still holds. The string was previously
>>> translated in "branch" and you are removing this translation (well, not
>>> here, but when 09/10 starts using this code).
>>>
>>
>> I should have mentioned in my cover letter, I didn't really understand
>> what has to be done about this, couldn't find much reference to go
>> about this. What do you suggest?
>
> From the user point of view :
>
> git for-each-ref --format '%(upstream:track)' => Should always be the
> same, because this may be parsed by scripts (plumbing). Should not
> depend on $LANG, and shouldn't change from a version of Git to another.
>

A little blurry on how this works, as in how translation takes place,
probably need to look at some code.

> git branch --format '%(upstream:track)' => Should show what is most
> pleasant to the user (porcelain): translated according to $LANG and
> friends, and may be improved in the future.
>
> I already pointed out a fix where a string was translated in a plumbing
> command. Another example is setup_unpack_trees_porcelain() in
> unpack-trees.c which solves exactly the same problem.
>

You did. I was just too clueless.

> I'll followup with a small series on top of yours to show the way. I did
> not try to polish it since I guess you have local changes on the same
> part of the code. Feel free to squash patches together or to squash them
> with yours. The commit messages are not meant to be final either.
>

Thanks a lot for this. This should help me out. I'll probably squash them along.
Thanks :)

-- 
Regards,
Karthik Nayak

Re: [PATCH v2 07/10] ref-filter: make %(upstream:track) prints "[gone]" for invalid upstreams

[Matthieu Moy]

Karthik Nayak <karthik.188@gmail.com> writes:

> A little blurry on how this works, as in how translation takes place,
> probably need to look at some code.

What you really need to understand is: _("foo") is translated, "foo" is
not and will always be "foo". Technically, _ is a macro, it could be
called get_the_translated_string_for(...) but that would be too long.

In git, _("foo") should be used when talking to a user (porcelain), and
"foo" when talking to a program (plumbing). This way a user running

git <plumbing-command> | grep "some-plumbing-message"

will always get the same result regardless of the current locale.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v2 00/10] port branch.c to use ref-filter's printing options

[Karthik Nayak]

On Sun, Oct 11, 2015 at 9:51 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Also does it make sense to integrate these changes here? Or would you like to
>> have another series on this?
>
> To me, the important in this series is to avoid introducing duplicated
> and inconsistent code, because it would make further refactoring harder.
>

Would you suggest duplicating whats done with %(align) here?

> But this series starts getting close to finished, so you should not try
> to add to much to it (at least for me: I spend some time reviewing v2
> and I have an idea of what the interdiff should look like, I'd rather
> avoid having new distraction in the v2->v3 interdiff).
>

Yes! makes sense, thanks :)

-- 
Regards,
Karthik Nayak

Re: [PATCH 2/3] ref-filter: allow porcelain to translate messages in the output

[Karthik Nayak]

On Sun, Oct 11, 2015 at 9:46 PM, Matthieu Moy <Matthieu.Moy@imag.fr> wrote:
> This patch shows the way, but is obviously incomplete as it works only
> for "nobracket" version. Actually, I think the code should first build
> the unbracketed output string and then do something like
>
> if (!nobracket) {
>         const char *to_free = v->s;
>         v->s = xstrfmt("[%s]", v->s);
>         free(to_free);
> }
>

Makes sense.

> so we don't have to worry about brackets anywhere else in the code.
>
> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
> ---
>  ref-filter.c | 28 ++++++++++++++++++++++++----
>  ref-filter.h |  3 +++
>  2 files changed, 27 insertions(+), 4 deletions(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index 7932c21..c2ee8c9 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -15,6 +15,26 @@
>  #include "version.h"
>  #include "wt-status.h"
>
> +static struct ref_msg {
> +       const char *gone;
> +       const char *ahead;
> +       const char *behind;
> +       const char *ahead_behind;
> +} msgs = {
> +       "gone",
> +       "ahead %d",
> +       "behind %d",
> +       "ahead %d, behind %d"
> +};
> +
> +void setup_ref_filter_porcelain_msg(void)
> +{
> +       msgs.gone = _("gone");
> +       msgs.ahead = _("ahead %d");
> +       msgs.behind = _("behind %d");
> +       msgs.ahead_behind = _("ahead %d, behind %d");
> +}
> +
>  typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
>
>  static struct {
> @@ -1124,7 +1144,7 @@ static void populate_value(struct ref_array_item *ref)
>                                 if (stat_tracking_info(branch, &num_ours,
>                                                        &num_theirs, NULL)) {
>                                         if (nobracket)
> -                                               v->s = "gone";
> +                                               v->s = msgs.gone;
>                                         else
>                                                 v->s = "[gone]";
>                                         continue;
> @@ -1134,17 +1154,17 @@ static void populate_value(struct ref_array_item *ref)
>                                         v->s = "";
>                                 else if (!num_ours) {
>                                         if (nobracket)
> -                                               v->s = xstrfmt("behind %d", num_theirs);
> +                                               v->s = xstrfmt(msgs.behind, num_theirs);
>                                         else
>                                                 v->s = xstrfmt("[behind %d]", num_theirs);
>                                 } else if (!num_theirs) {
>                                         if (nobracket)
> -                                               v->s = xstrfmt("ahead %d", num_ours);
> +                                               v->s = xstrfmt(msgs.ahead, num_ours);
>                                         else
>                                                 v->s = xstrfmt("[ahead %d]", num_ours);
>                                 } else {
>                                         if (nobracket)
> -                                               v->s = xstrfmt("ahead %d, behind %d",
> +                                               v->s = xstrfmt(msgs.ahead_behind,
>                                                                num_ours, num_theirs);
>                                         else
>                                                 v->s = xstrfmt("[ahead %d, behind %d]",
> diff --git a/ref-filter.h b/ref-filter.h
> index 0014b92..2cce02c 100644
> --- a/ref-filter.h
> +++ b/ref-filter.h
> @@ -112,4 +112,7 @@ int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset)
>  /*  Get the current HEAD's description */
>  char *get_head_description(void);
>
> +/*  Set up translated strings in the output. */
> +void setup_ref_filter_porcelain_msg(void);
> +
>  #endif /*  REF_FILTER_H  */
> --
> 2.6.0.rc2.24.gb06d8e9.dirty
>

I'm squashing this and the other two patches into my series. Thanks :)

-- 
Regards,
Karthik Nayak

Re: [PATCH v2 00/10] port branch.c to use ref-filter's printing options

[Matthieu Moy]

Karthik Nayak <karthik.188@gmail.com> writes:

> On Sun, Oct 11, 2015 at 9:51 PM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>
>>> Also does it make sense to integrate these changes here? Or would you like to
>>> have another series on this?
>>
>> To me, the important in this series is to avoid introducing duplicated
>> and inconsistent code, because it would make further refactoring harder.
>>
>
> Would you suggest duplicating whats done with %(align) here?

I think introducing a function to split according to commas and remove
commas would make sense, but I won't insist on that. Mimicking what's
done with %(align) is acceptable to me: we'll have several instances of
the same pattern, not ideal but easy enough to refactor later.
Especially if you actually plan to work on that :-).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v2 00/10] port branch.c to use ref-filter's printing options

[Karthik Nayak]

On Mon, Oct 12, 2015 at 1:02 AM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> On Sun, Oct 11, 2015 at 9:51 PM, Matthieu Moy
>> <Matthieu.Moy@grenoble-inp.fr> wrote:
>>> Karthik Nayak <karthik.188@gmail.com> writes:
>>>
>>>> Also does it make sense to integrate these changes here? Or would you like to
>>>> have another series on this?
>>>
>>> To me, the important in this series is to avoid introducing duplicated
>>> and inconsistent code, because it would make further refactoring harder.
>>>
>>
>> Would you suggest duplicating whats done with %(align) here?
>
> I think introducing a function to split according to commas and remove
> commas would make sense, but I won't insist on that. Mimicking what's
> done with %(align) is acceptable to me: we'll have several instances of
> the same pattern, not ideal but easy enough to refactor later.
> Especially if you actually plan to work on that :-).
>

I was planning on working on what Junio and you suggested after this ;-).
So I didn't see the need of " introducing a function to split
according to commas"
if we plan to rewrite the whole parsing part of ref-filter.

-- 
Regards,
Karthik Nayak

Re: [PATCH v2 07/10] ref-filter: make %(upstream:track) prints "[gone]" for invalid upstreams

[Karthik Nayak]

On Sun, Oct 11, 2015 at 11:40 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> A little blurry on how this works, as in how translation takes place,
>> probably need to look at some code.
>
> What you really need to understand is: _("foo") is translated, "foo" is
> not and will always be "foo". Technically, _ is a macro, it could be
> called get_the_translated_string_for(...) but that would be too long.
>
> In git, _("foo") should be used when talking to a user (porcelain), and
> "foo" when talking to a program (plumbing). This way a user running
>
> git <plumbing-command> | grep "some-plumbing-message"
>
> will always get the same result regardless of the current locale.

That helped a lot, thanks a bunch.

-- 
Regards,
Karthik Nayak

Re: [PATCH v2 00/10] port branch.c to use ref-filter's printing options

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> On Fri, Oct 9, 2015 at 11:59 PM, Junio C Hamano <gitster@pobox.com> wrote:
> ...
> Also does it make sense to integrate these changes here? Or would you like to
> have another series on this?

I do not think you would want to ask that question, as my answer
would most likely be "The most preferable would be a series to clean
up the existing codepath that deals with %(align) first, on top of
which everything in flight that is not yet in 'next' is rebased."

Re: [PATCH 1/3] fixup: use xstrfmt instead of fixed-size buf + sprintf + xstrdup

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

> The char buf[40] is safe (at least while the strings are not
> translated), but I'd rather avoid magic numbers like this 40 in the
> code, and use a construct that does not have this size limitation.
> Especially if it makes the code shorter.
>
> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
> ---

The construct being fixed with this change looks very similar to
Peff's a5e03bf5 (ref-filter: drop sprintf and strcpy calls,
2015-09-24) on jk/war-on-sprintf topic, but the new code since that
commit cleaned up.

I'd expect that this will be rolled into Karthik's series in the
next reroll?  

Looking good.  Thanks.

>  ref-filter.c | 20 ++++++++------------
>  1 file changed, 8 insertions(+), 12 deletions(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index 6044eb0..7932c21 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1116,7 +1116,6 @@ static void populate_value(struct ref_array_item *ref)
>  				 strcmp(formatp, "trackshort") &&
>  				 (starts_with(name, "upstream") ||
>  				  starts_with(name, "push"))) {
> -				char buf[40];
>  				unsigned int nobracket = 0;
>  
>  				if (!strcmp(valp, ",nobracket"))
> @@ -1135,24 +1134,21 @@ static void populate_value(struct ref_array_item *ref)
>  					v->s = "";
>  				else if (!num_ours) {
>  					if (nobracket)
> -						sprintf(buf, "behind %d", num_theirs);
> +						v->s = xstrfmt("behind %d", num_theirs);
>  					else
> -						sprintf(buf, "[behind %d]", num_theirs);
> -					v->s = xstrdup(buf);
> +						v->s = xstrfmt("[behind %d]", num_theirs);
>  				} else if (!num_theirs) {
>  					if (nobracket)
> -						sprintf(buf, "ahead %d", num_ours);
> +						v->s = xstrfmt("ahead %d", num_ours);
>  					else
> -						sprintf(buf, "[ahead %d]", num_ours);
> -					v->s = xstrdup(buf);
> +						v->s = xstrfmt("[ahead %d]", num_ours);
>  				} else {
>  					if (nobracket)
> -						sprintf(buf, "ahead %d, behind %d",
> -							num_ours, num_theirs);
> +						v->s = xstrfmt("ahead %d, behind %d",
> +							       num_ours, num_theirs);
>  					else
> -						sprintf(buf, "[ahead %d, behind %d]",
> -						num_ours, num_theirs);
> -					v->s = xstrdup(buf);
> +						v->s = xstrfmt("[ahead %d, behind %d]",
> +							       num_ours, num_theirs);
>  				}
>  				continue;
>  			} else if (!strcmp(formatp, "trackshort") &&

Re: [PATCH v2 00/10] port branch.c to use ref-filter's printing options

[Karthik Nayak]

On Mon, Oct 12, 2015 at 6:06 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> On Fri, Oct 9, 2015 at 11:59 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> ...
>> Also does it make sense to integrate these changes here? Or would you like to
>> have another series on this?
>
> I do not think you would want to ask that question, as my answer
> would most likely be "The most preferable would be a series to clean
> up the existing codepath that deals with %(align) first, on top of
> which everything in flight that is not yet in 'next' is rebased."
>

Ah, but I might take a while to get there, So I'd rather push code which
is almost ready and work on that slowly, if that's ok?

-- 
Regards,
Karthik Nayak

Re: [PATCH 1/3] fixup: use xstrfmt instead of fixed-size buf + sprintf + xstrdup

[Karthik Nayak]

On Mon, Oct 12, 2015 at 11:15 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Matthieu Moy <Matthieu.Moy@imag.fr> writes:
>
>> The char buf[40] is safe (at least while the strings are not
>> translated), but I'd rather avoid magic numbers like this 40 in the
>> code, and use a construct that does not have this size limitation.
>> Especially if it makes the code shorter.
>>
>> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
>> ---
>
> The construct being fixed with this change looks very similar to
> Peff's a5e03bf5 (ref-filter: drop sprintf and strcpy calls,
> 2015-09-24) on jk/war-on-sprintf topic, but the new code since that
> commit cleaned up.
>

Yes, pretty much the same.

> I'd expect that this will be rolled into Karthik's series in the
> next reroll?
>
> Looking good.  Thanks.

Yes, I've adding this into my series.

-- 
Regards,
Karthik Nayak

Re: [PATCH v2 00/10] port branch.c to use ref-filter's printing options

[Matthieu Moy]

Karthik Nayak <karthik.188@gmail.com> writes:

> On Mon, Oct 12, 2015 at 6:06 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>
>>> On Fri, Oct 9, 2015 at 11:59 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> ...
>>> Also does it make sense to integrate these changes here? Or would you like to
>>> have another series on this?
>>
>> I do not think you would want to ask that question, as my answer
>> would most likely be "The most preferable would be a series to clean
>> up the existing codepath that deals with %(align) first, on top of
>> which everything in flight that is not yet in 'next' is rebased."
>
> Ah, but I might take a while to get there, So I'd rather push code which
> is almost ready and work on that slowly, if that's ok?

That's OK to me. The "most preferable way" above would lead to a cleaner
history, but also more work for you and for me as a reviewer.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v2 00/10] port branch.c to use ref-filter's printing options

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> On Mon, Oct 12, 2015 at 6:06 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Karthik Nayak <karthik.188@gmail.com> writes:
>>>
>>>> On Fri, Oct 9, 2015 at 11:59 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>>> ...
>>>> Also does it make sense to integrate these changes here? Or would you like to
>>>> have another series on this?
>>>
>>> I do not think you would want to ask that question, as my answer
>>> would most likely be "The most preferable would be a series to clean
>>> up the existing codepath that deals with %(align) first, on top of
>>> which everything in flight that is not yet in 'next' is rebased."
>>
>> Ah, but I might take a while to get there, So I'd rather push code which
>> is almost ready and work on that slowly, if that's ok?
>
> That's OK to me. The "most preferable way" above would lead to a cleaner
> history, but also more work for you and for me as a reviewer.

I do not think the cleanliness of the resulting history is of prime
concern.  At least, that was not where my preference came from.

If you design a new infrastructure to help refactoring early
(i.e. before adding many copies of code that need to be cleaned up
later), it would make the work of reviewing of the design of the
helper and refactoring using that helper smaller, not larger.  And
the new codepaths that use the helper would become easier to follow
(otherwise we wouldn't be doing such a refactoring in the first
place), making the reviews easier.  That is where my preference
comes from.

There _is_ a sunk-cost that has already been invested in reviewing
the older round that added code that needs to be cleaned up; there
are some parts other than the "need to be cleaned-up" parts that we
would feel confortable having in the reroll without having to
re-review them.  I do not know if that is a very high cost, though,
especially for those who have already seen the previous rounds.

Anyway, I wouldn't insist and we can go either way.

Re: [PATCH v2 00/10] port branch.c to use ref-filter's printing options

[Matthieu Moy]

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

> If you design a new infrastructure to help refactoring early
> (i.e. before adding many copies of code that need to be cleaned up
> later), it would make the work of reviewing of the design of the
> helper and refactoring using that helper smaller, not larger.

But most of the code concerned is already reviewed. The first instances
of the pattern to refactor is already in next. With a real time machine,
we could go back in past, refactor and then have cleaner series, but
with Git as our only tool we can't ;-).

The current series will just add one more instance of sub-optimal code,
it isn't hard to review. Inserting new code before them would make the
interdiff far bigger.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v2 09/10] branch: use ref-filter printing APIs

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> Port branch.c to use ref-filter APIs for printing. This clears out
> most of the code used in branch.c for printing and replaces them with
> calls made to the ref-filter library.
>
> Introduce get_format() which gets the format required for printing of
> refs. Make amendments to print_ref_list() to reflect these changes.

Is it get_format() still?

>
> Change calc_maxwidth() to also account for the length of HEAD ref, by
> calling ref-filter:get_head_discription().
>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  builtin/branch.c         | 261 ++++++++++++-----------------------------------
>  t/t6040-tracking-info.sh |   2 +-
>  2 files changed, 66 insertions(+), 197 deletions(-)
>
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 67ef9f1..4d8b570 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -35,12 +35,12 @@ static unsigned char head_sha1[20];
>  
>  static int branch_use_color = -1;
>  static char branch_colors[][COLOR_MAXLEN] = {
> -	GIT_COLOR_RESET,
> -	GIT_COLOR_NORMAL,	/* PLAIN */
> -	GIT_COLOR_RED,		/* REMOTE */
> -	GIT_COLOR_NORMAL,	/* LOCAL */
> -	GIT_COLOR_GREEN,	/* CURRENT */
> -	GIT_COLOR_BLUE,		/* UPSTREAM */
> +	"%(color:reset)",
> +	"%(color:reset)",	/* PLAIN */
> +	"%(color:red)",		/* REMOTE */
> +	"%(color:reset)",	/* LOCAL */
> +	"%(color:green)",	/* CURRENT */
> +	"%(color:blue)",	/* UPSTREAM */
>  };
>  enum color_branch {
>  	BRANCH_COLOR_RESET = 0,
> @@ -271,192 +271,6 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
>  	return(ret);
>  }
>  
> -static void fill_tracking_info(struct strbuf *stat, const char *branch_name,
> -		int show_upstream_ref)
> -{
> -	int ours, theirs;
> -	char *ref = NULL;
> -	struct branch *branch = branch_get(branch_name);
> -	const char *upstream;
> -	struct strbuf fancy = STRBUF_INIT;
> -	int upstream_is_gone = 0;
> -	int added_decoration = 1;
> -
> -	if (stat_tracking_info(branch, &ours, &theirs, &upstream) < 0) {
> -		if (!upstream)
> -			return;
> -		upstream_is_gone = 1;
> -	}
> -
> -	if (show_upstream_ref) {
> -		ref = shorten_unambiguous_ref(upstream, 0);
> -		if (want_color(branch_use_color))
> -			strbuf_addf(&fancy, "%s%s%s",
> -					branch_get_color(BRANCH_COLOR_UPSTREAM),
> -					ref, branch_get_color(BRANCH_COLOR_RESET));
> -		else
> -			strbuf_addstr(&fancy, ref);
> -	}
> -
> -	if (upstream_is_gone) {
> -		if (show_upstream_ref)
> -			strbuf_addf(stat, _("[%s: gone]"), fancy.buf);
> -		else
> -			added_decoration = 0;
> -	} else if (!ours && !theirs) {
> -		if (show_upstream_ref)
> -			strbuf_addf(stat, _("[%s]"), fancy.buf);
> -		else
> -			added_decoration = 0;
> -	} else if (!ours) {
> -		if (show_upstream_ref)
> -			strbuf_addf(stat, _("[%s: behind %d]"), fancy.buf, theirs);
> -		else
> -			strbuf_addf(stat, _("[behind %d]"), theirs);
> -
> -	} else if (!theirs) {
> -		if (show_upstream_ref)
> -			strbuf_addf(stat, _("[%s: ahead %d]"), fancy.buf, ours);
> -		else
> -			strbuf_addf(stat, _("[ahead %d]"), ours);
> -	} else {
> -		if (show_upstream_ref)
> -			strbuf_addf(stat, _("[%s: ahead %d, behind %d]"),
> -				    fancy.buf, ours, theirs);
> -		else
> -			strbuf_addf(stat, _("[ahead %d, behind %d]"),
> -				    ours, theirs);
> -	}
> -	strbuf_release(&fancy);
> -	if (added_decoration)
> -		strbuf_addch(stat, ' ');
> -	free(ref);
> -}
> -
> -static void add_verbose_info(struct strbuf *out, struct ref_array_item *item,
> -			     struct ref_filter *filter, const char *refname)
> -{
> -	struct strbuf subject = STRBUF_INIT, stat = STRBUF_INIT;
> -	const char *sub = _(" **** invalid ref ****");
> -	struct commit *commit = item->commit;
> -
> -	if (!parse_commit(commit)) {
> -		pp_commit_easy(CMIT_FMT_ONELINE, commit, &subject);
> -		sub = subject.buf;
> -	}
> -
> -	if (item->kind == FILTER_REFS_BRANCHES)
> -		fill_tracking_info(&stat, refname, filter->verbose > 1);
> -
> -	strbuf_addf(out, " %s %s%s",
> -		find_unique_abbrev(item->commit->object.sha1, filter->abbrev),
> -		stat.buf, sub);
> -	strbuf_release(&stat);
> -	strbuf_release(&subject);
> -}
> -
> -/*
> - * This is duplicated in ref-filter.c, will be removed when we adopt
> - * ref-filter's printing APIs.
> - */
> -static char *get_head_description(void)
> -{
> -	struct strbuf desc = STRBUF_INIT;
> -	struct wt_status_state state;
> -	memset(&state, 0, sizeof(state));
> -	wt_status_get_state(&state, 1);
> -	if (state.rebase_in_progress ||
> -	    state.rebase_interactive_in_progress)
> -		strbuf_addf(&desc, _("(no branch, rebasing %s)"),
> -			    state.branch);
> -	else if (state.bisect_in_progress)
> -		strbuf_addf(&desc, _("(no branch, bisect started on %s)"),
> -			    state.branch);
> -	else if (state.detached_from) {
> -		/* TRANSLATORS: make sure these match _("HEAD detached at ")
> -		   and _("HEAD detached from ") in wt-status.c */
> -		if (state.detached_at)
> -			strbuf_addf(&desc, _("(HEAD detached at %s)"),
> -				state.detached_from);
> -		else
> -			strbuf_addf(&desc, _("(HEAD detached from %s)"),
> -				state.detached_from);
> -	}
> -	else
> -		strbuf_addstr(&desc, _("(no branch)"));
> -	free(state.branch);
> -	free(state.onto);
> -	free(state.detached_from);
> -	return strbuf_detach(&desc, NULL);
> -}
> -
> -static void format_and_print_ref_item(struct ref_array_item *item, int maxwidth,
> -				      struct ref_filter *filter, const char *remote_prefix)
> -{
> -	char c;
> -	int current = 0;
> -	int color;
> -	struct strbuf out = STRBUF_INIT, name = STRBUF_INIT;
> -	const char *prefix = "";
> -	const char *desc = item->refname;
> -	char *to_free = NULL;
> -
> -	switch (item->kind) {
> -	case FILTER_REFS_BRANCHES:
> -		skip_prefix(desc, "refs/heads/", &desc);
> -		if (!filter->detached && !strcmp(desc, head))
> -			current = 1;
> -		else
> -			color = BRANCH_COLOR_LOCAL;
> -		break;
> -	case FILTER_REFS_REMOTES:
> -		skip_prefix(desc, "refs/remotes/", &desc);
> -		color = BRANCH_COLOR_REMOTE;
> -		prefix = remote_prefix;
> -		break;
> -	case FILTER_REFS_DETACHED_HEAD:
> -		desc = to_free = get_head_description();
> -		current = 1;
> -		break;
> -	default:
> -		color = BRANCH_COLOR_PLAIN;
> -		break;
> -	}
> -
> -	c = ' ';
> -	if (current) {
> -		c = '*';
> -		color = BRANCH_COLOR_CURRENT;
> -	}
> -
> -	strbuf_addf(&name, "%s%s", prefix, desc);
> -	if (filter->verbose) {
> -		int utf8_compensation = strlen(name.buf) - utf8_strwidth(name.buf);
> -		strbuf_addf(&out, "%c %s%-*s%s", c, branch_get_color(color),
> -			    maxwidth + utf8_compensation, name.buf,
> -			    branch_get_color(BRANCH_COLOR_RESET));
> -	} else
> -		strbuf_addf(&out, "%c %s%s%s", c, branch_get_color(color),
> -			    name.buf, branch_get_color(BRANCH_COLOR_RESET));
> -
> -	if (item->symref) {
> -		skip_prefix(item->symref, "refs/remotes/", &desc);
> -		strbuf_addf(&out, " -> %s", desc);
> -	}
> -	else if (filter->verbose)
> -		/* " f7c0c00 [ahead 58, behind 197] vcs-svn: drop obj_pool.h" */
> -		add_verbose_info(&out, item, filter, desc);
> -	if (column_active(colopts)) {
> -		assert(!filter->verbose && "--column and --verbose are incompatible");
> -		string_list_append(&output, out.buf);
> -	} else {
> -		printf("%s\n", out.buf);
> -	}
> -	strbuf_release(&name);
> -	strbuf_release(&out);
> -	free(to_free);
> -}
> -
>  static int calc_maxwidth(struct ref_array *refs, int remote_bonus)
>  {
>  	int i, max = 0;
> @@ -467,7 +281,10 @@ static int calc_maxwidth(struct ref_array *refs, int remote_bonus)
>  
>  		skip_prefix(it->refname, "refs/heads/", &desc);
>  		skip_prefix(it->refname, "refs/remotes/", &desc);
> -		w = utf8_strwidth(desc);
> +		if (it->kind == FILTER_REFS_DETACHED_HEAD)
> +			w = strlen(get_head_description());
> +		else
> +			w = utf8_strwidth(desc);
>  
>  		if (it->kind == FILTER_REFS_REMOTES)
>  			w += remote_bonus;
> @@ -477,12 +294,51 @@ static int calc_maxwidth(struct ref_array *refs, int remote_bonus)
>  	return max;
>  }
>  
> +static char *build_format(struct ref_filter *filter, int maxwidth, const char *remote_prefix)
> +{
> +	struct strbuf fmt = STRBUF_INIT;
> +	struct strbuf local = STRBUF_INIT;
> +	struct strbuf remote = STRBUF_INIT;
> +
> +	strbuf_addf(&fmt, "%%(if)%%(HEAD)%%(then)* %s%%(else)  %%(end)", branch_get_color(BRANCH_COLOR_CURRENT));
> +
> +	if (filter->verbose) {
> +		strbuf_addf(&local, "%%(align:%d,left)%%(refname:short)%%(end)", maxwidth);
> +		strbuf_addf(&local, "%s", branch_get_color(BRANCH_COLOR_RESET));
> +		strbuf_addf(&local, " %%(objectname:short=7) ");
> +
> +		if (filter->verbose > 1)
> +			strbuf_addf(&local, "%%(if)%%(upstream)%%(then)[%s%%(upstream:short)%s%%(if)%%(upstream:track)"
> +				    "%%(then): %%(upstream:track,nobracket)%%(end)] %%(end)%%(contents:subject)",
> +				    branch_get_color(BRANCH_COLOR_UPSTREAM), branch_get_color(BRANCH_COLOR_RESET));
> +		else
> +			strbuf_addf(&local, "%%(if)%%(upstream:track)%%(then)%%(upstream:track) %%(end)%%(contents:subject)");
> +
> +		strbuf_addf(&remote, "%s%%(align:%d,left)%s%%(refname:short)%%(end)%s%%(if)%%(symref)%%(then) -> %%(symref:short)"
> +			    "%%(else) %%(objectname:short=7) %%(contents:subject)%%(end)",
> +			    branch_get_color(BRANCH_COLOR_REMOTE), maxwidth,
> +			    remote_prefix, branch_get_color(BRANCH_COLOR_RESET));
> +	} else {
> +		strbuf_addf(&local, "%%(refname:short)%s", branch_get_color(BRANCH_COLOR_RESET));
> +		strbuf_addf(&remote, "%s%s%%(refname:short)%s%%(if)%%(symref)%%(then) -> %%(symref:short)%%(end)",
> +			    branch_get_color(BRANCH_COLOR_REMOTE), remote_prefix, branch_get_color(BRANCH_COLOR_RESET));
> +	}
> +
> +	strbuf_addf(&fmt, "%%(if:notequals=remotes)%%(refname:base)%%(then)%s%%(else)%s%%(end)", local.buf, remote.buf);
> +
> +	strbuf_release(&local);
> +	strbuf_release(&remote);
> +	return strbuf_detach(&fmt, NULL);
> +}
> +
>  static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sorting)
>  {
>  	int i;
>  	struct ref_array array;
>  	int maxwidth = 0;
>  	const char *remote_prefix = "";
> +	struct strbuf out = STRBUF_INIT;
> +	char *format;
>  
>  	/*
>  	 * If we are listing more than just remote branches,
> @@ -494,12 +350,14 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
>  
>  	memset(&array, 0, sizeof(array));
>  
> -	verify_ref_format("%(refname)%(symref)");
>  	filter_refs(&array, filter, filter->kind | FILTER_REFS_INCLUDE_BROKEN);
>  
>  	if (filter->verbose)
>  		maxwidth = calc_maxwidth(&array, strlen(remote_prefix));
>  
> +	format = build_format(filter, maxwidth, remote_prefix);
> +	verify_ref_format(format);
> +
>  	/*
>  	 * If no sorting parameter is given then we default to sorting
>  	 * by 'refname'. This would give us an alphabetically sorted
> @@ -511,10 +369,21 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
>  		sorting = ref_default_sorting();
>  	ref_array_sort(sorting, &array);
>  
> -	for (i = 0; i < array.nr; i++)
> -		format_and_print_ref_item(array.items[i], maxwidth, filter, remote_prefix);
> +	for (i = 0; i < array.nr; i++) {
> +		format_ref_array_item(array.items[i], format, 0, &out);
> +		if (column_active(colopts)) {
> +			assert(!filter->verbose && "--column and --verbose are incompatible");
> +			 /* format to a string_list to let print_columns() do its job */
> +			string_list_append(&output, out.buf);
> +		} else {
> +			fwrite(out.buf, 1, out.len, stdout);
> +			putchar('\n');
> +		}
> +		strbuf_release(&out);
> +	}
>  
>  	ref_array_clear(&array);
> +	free(format);
>  }
>  
>  static void rename_branch(const char *oldname, const char *newname, int force)
> diff --git a/t/t6040-tracking-info.sh b/t/t6040-tracking-info.sh
> index 3d5c238..97a0765 100755
> --- a/t/t6040-tracking-info.sh
> +++ b/t/t6040-tracking-info.sh
> @@ -44,7 +44,7 @@ b1 [ahead 1, behind 1] d
>  b2 [ahead 1, behind 1] d
>  b3 [behind 1] b
>  b4 [ahead 2] f
> -b5 g
> +b5 [gone] g
>  b6 c
>  EOF

Re: [PATCH v2 09/10] branch: use ref-filter printing APIs

[Karthik Nayak]

On Tue, Oct 13, 2015 at 10:01 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Port branch.c to use ref-filter APIs for printing. This clears out
>> most of the code used in branch.c for printing and replaces them with
>> calls made to the ref-filter library.
>>
>> Introduce get_format() which gets the format required for printing of
>> refs. Make amendments to print_ref_list() to reflect these changes.
>
> Is it get_format() still?

Will change that, thanks :)

-- 
Regards,
Karthik Nayak

Re: [PATCH v2 08/10] ref-filter: add support for %(upstream:track,nobracket)

[Karthik Nayak]

On Thu, Oct 8, 2015 at 2:48 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> Add support for %(upstream:track,nobracket) which will print the
> tracking information without the brackets (i.e. "ahead N, behind M").
>
> Add test and documentation for the same.
> ---
>  Documentation/git-for-each-ref.txt |  6 ++++--
>  ref-filter.c                       | 28 +++++++++++++++++++++++-----
>  t/t6300-for-each-ref.sh            |  9 +++++++++
>  3 files changed, 36 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index c713ec0..a38cbf6 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -114,8 +114,10 @@ upstream::
>         `refname` above.  Additionally respects `:track` to show
>         "[ahead N, behind M]" and `:trackshort` to show the terse
>         version: ">" (ahead), "<" (behind), "<>" (ahead and behind),
> -       or "=" (in sync).  Has no effect if the ref does not have
> -       tracking information associated with it.
> +       or "=" (in sync).  Append `:track,nobracket` to show tracking
> +       information without brackets (i.e "ahead N, behind M").  Has
> +       no effect if the ref does not have tracking information
> +       associated with it.
>
>  push::
>         The name of a local ref which represents the `@{push}` location
> diff --git a/ref-filter.c b/ref-filter.c
> index 6a38089..6044eb0 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1112,27 +1112,45 @@ static void populate_value(struct ref_array_item *ref)
>                         if (!strcmp(formatp, "short"))
>                                 refname = shorten_unambiguous_ref(refname,
>                                                       warn_ambiguous_refs);
> -                       else if (!strcmp(formatp, "track") &&
> +                       else if (skip_prefix(formatp, "track", &valp) &&
> +                                strcmp(formatp, "trackshort") &&
>                                  (starts_with(name, "upstream") ||
>                                   starts_with(name, "push"))) {

If you see here, we detect "track" first for
%(upstream:track,nobracket) so although
the idea was to use something similar to %(align:...) I don't see a
good way of going
about this. If we want %(upstream:nobracket,track) to be supported then, I think
we'll have to change this whole layout and have the detection done up where we
locat "upstream" / "push", what would be a nice way to go around this?

What I could think of:
1. Do the cleanup that Junio and Matthieu suggested, where we
basically parse the
atoms and store them into a used_atom struct. I could either work on
those patches
before this and then rebase this on top.
2. Let this be and come back on it when implementing the above series.
After reading Matthieu's and Junio's discussion, I lean towards the latter.

>                                 char buf[40];
> +                               unsigned int nobracket = 0;
> +
> +                               if (!strcmp(valp, ",nobracket"))
> +                                       nobracket = 1;
>
>                                 if (stat_tracking_info(branch, &num_ours,
>                                                        &num_theirs, NULL)) {
> -                                       v->s = "[gone]";
> +                                       if (nobracket)
> +                                               v->s = "gone";
> +                                       else
> +                                               v->s = "[gone]";
>                                         continue;
>                                 }
>
>                                 if (!num_ours && !num_theirs)
>                                         v->s = "";
>                                 else if (!num_ours) {
> -                                       sprintf(buf, "[behind %d]", num_theirs);
> +                                       if (nobracket)
> +                                               sprintf(buf, "behind %d", num_theirs);
> +                                       else
> +                                               sprintf(buf, "[behind %d]", num_theirs);
>                                         v->s = xstrdup(buf);
>                                 } else if (!num_theirs) {
> -                                       sprintf(buf, "[ahead %d]", num_ours);
> +                                       if (nobracket)
> +                                               sprintf(buf, "ahead %d", num_ours);
> +                                       else
> +                                               sprintf(buf, "[ahead %d]", num_ours);
>                                         v->s = xstrdup(buf);
>                                 } else {
> -                                       sprintf(buf, "[ahead %d, behind %d]",
> +                                       if (nobracket)
> +                                               sprintf(buf, "ahead %d, behind %d",
> +                                                       num_ours, num_theirs);
> +                                       else
> +                                               sprintf(buf, "[ahead %d, behind %d]",
>                                                 num_ours, num_theirs);
>                                         v->s = xstrdup(buf);
>                                 }
> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index 4f620bf..7ab8bf8 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -344,6 +344,15 @@ test_expect_success 'Check upstream:track format' '
>  '
>
>  cat >expected <<EOF
> +ahead 1
> +EOF
> +
> +test_expect_success 'Check upstream:track,nobracket format' '
> +       git for-each-ref --format="%(upstream:track,nobracket)" refs/heads >actual &&
> +       test_cmp expected actual
> +'
> +
> +cat >expected <<EOF
>  >
>  EOF
>
> --
> 2.6.0
>



-- 
Regards,
Karthik Nayak

Re: [PATCH v2 08/10] ref-filter: add support for %(upstream:track,nobracket)

[Matthieu Moy]

Karthik Nayak <karthik.188@gmail.com> writes:

> On Thu, Oct 8, 2015 at 2:48 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> Add support for %(upstream:track,nobracket) which will print the
>> tracking information without the brackets (i.e. "ahead N, behind M").
>>
>> Add test and documentation for the same.
>> ---
>>  Documentation/git-for-each-ref.txt |  6 ++++--
>>  ref-filter.c                       | 28 +++++++++++++++++++++++-----
>>  t/t6300-for-each-ref.sh            |  9 +++++++++
>>  3 files changed, 36 insertions(+), 7 deletions(-)
>>
>> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
>> index c713ec0..a38cbf6 100644
>> --- a/Documentation/git-for-each-ref.txt
>> +++ b/Documentation/git-for-each-ref.txt
>> @@ -114,8 +114,10 @@ upstream::
>>         `refname` above.  Additionally respects `:track` to show
>>         "[ahead N, behind M]" and `:trackshort` to show the terse
>>         version: ">" (ahead), "<" (behind), "<>" (ahead and behind),
>> -       or "=" (in sync).  Has no effect if the ref does not have
>> -       tracking information associated with it.
>> +       or "=" (in sync).  Append `:track,nobracket` to show tracking
>> +       information without brackets (i.e "ahead N, behind M").  Has
>> +       no effect if the ref does not have tracking information
>> +       associated with it.
>>
>>  push::
>>         The name of a local ref which represents the `@{push}` location
>> diff --git a/ref-filter.c b/ref-filter.c
>> index 6a38089..6044eb0 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -1112,27 +1112,45 @@ static void populate_value(struct ref_array_item *ref)
>>                         if (!strcmp(formatp, "short"))
>>                                 refname = shorten_unambiguous_ref(refname,
>>                                                       warn_ambiguous_refs);
>> -                       else if (!strcmp(formatp, "track") &&
>> +                       else if (skip_prefix(formatp, "track", &valp) &&
>> +                                strcmp(formatp, "trackshort") &&
>>                                  (starts_with(name, "upstream") ||
>>                                   starts_with(name, "push"))) {
>
> If you see here, we detect "track" first for
> %(upstream:track,nobracket)

Yes, but I still think that this was a bad idea. If you want nobracket
to apply to "track", then the syntax should be
%(upstream:track=nobracket). I think the "nobracket" should apply to
"upstream" (i.e. be global to the atom), hence
%(upstream:nobracket,track) and %(upstream:track,nobracket) should both
be accepted. Possibly %(upstream:<not track>,nobracket) could complain,
but just ignoring "nobracket" in this case would make sense to me.

Special-casing the implementation of "nobracket" also means you're
special-casing its user-visible behavior. And as a user, I hate it when
the behavior is subtely different for things that look like each other
(in this case, %(align:...) and %(upstream:...) ).

> %(upstream:nobracket,track) to be supported then, I think we'll have
> to change this whole layout and have the detection done up where we
> locat "upstream" / "push", what would be a nice way to go around this?

You mean, below

		else if (starts_with(name, "upstream")) {

within populate_value()?

I think it would, yes.

> What I could think of:
> 1. Do the cleanup that Junio and Matthieu suggested, where we
> basically parse the
> atoms and store them into a used_atom struct. I could either work on
> those patches
> before this and then rebase this on top.
> 2. Let this be and come back on it when implementing the above series.
> After reading Matthieu's and Junio's discussion, I lean towards the latter.

Leaving it as-is does not fit in my arguments to do the refactoring
later. It's not introducing "another instance of an existing pattern",
but actually a new pattern.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v2 08/10] ref-filter: add support for %(upstream:track,nobracket)

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

>> If you see here, we detect "track" first for
>> %(upstream:track,nobracket)
>
> Yes, but I still think that this was a bad idea. If you want
> nobracket to apply to "track", then the syntax should be
> %(upstream:track=nobracket). I think the "nobracket" should apply
> to "upstream" (i.e. be global to the atom), hence
> %(upstream:nobracket,track) and %(upstream:track,nobracket) should
> both be accepted.

That makes sense to me, as I think what is being discussed would be
%(upstream:track=yes,bracket=no) when it is fully spelled out.

Re: [PATCH v2 08/10] ref-filter: add support for %(upstream:track,nobracket)

[Karthik Nayak]

 Wed, Oct 14, 2015 at 12:24 AM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Yes, but I still think that this was a bad idea. If you want nobracket
> to apply to "track", then the syntax should be
> %(upstream:track=nobracket). I think the "nobracket" should apply to
> "upstream" (i.e. be global to the atom), hence
> %(upstream:nobracket,track) and %(upstream:track,nobracket) should both
> be accepted. Possibly %(upstream:<not track>,nobracket) could complain,
> but just ignoring "nobracket" in this case would make sense to me.
>

Oh okay, was thinking only WRT the "track" option.

> Special-casing the implementation of "nobracket" also means you're
> special-casing its user-visible behavior. And as a user, I hate it when
> the behavior is subtely different for things that look like each other
> (in this case, %(align:...) and %(upstream:...) ).
>

Makes sense, was just looking for opinions.

>> %(upstream:nobracket,track) to be supported then, I think we'll have
>> to change this whole layout and have the detection done up where we
>> locat "upstream" / "push", what would be a nice way to go around this?
>
> You mean, below
>
>                 else if (starts_with(name, "upstream")) {
>
> within populate_value()?
>
> I think it would, yes.
>

Yes, that's what I meant.

>> What I could think of:
>> 1. Do the cleanup that Junio and Matthieu suggested, where we
>> basically parse the
>> atoms and store them into a used_atom struct. I could either work on
>> those patches
>> before this and then rebase this on top.
>> 2. Let this be and come back on it when implementing the above series.
>> After reading Matthieu's and Junio's discussion, I lean towards the latter.
>
> Leaving it as-is does not fit in my arguments to do the refactoring
> later. It's not introducing "another instance of an existing pattern",
> but actually a new pattern.
>

I meant after changing whatever we discussed above.

-- 
Regards,
Karthik Nayak

Re: [PATCH v2 00/10] port branch.c to use ref-filter's printing options

[Karthik Nayak]

On Tue, Oct 13, 2015 at 12:37 AM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> If you design a new infrastructure to help refactoring early
>> (i.e. before adding many copies of code that need to be cleaned up
>> later), it would make the work of reviewing of the design of the
>> helper and refactoring using that helper smaller, not larger.
>
> But most of the code concerned is already reviewed. The first instances
> of the pattern to refactor is already in next. With a real time machine,
> we could go back in past, refactor and then have cleaner series, but
> with Git as our only tool we can't ;-).
>
> The current series will just add one more instance of sub-optimal code,
> it isn't hard to review. Inserting new code before them would make the
> interdiff far bigger.
>

Sorry for the delay, was a bit busy with college work. For the most
part I've been
trying to integrate the %(upstream) and its options 'track',
'trachshort' and 'short'
so we could implement %(upstream:track,nobracket) or
%(upstream:nobracket,track).
While doing so I realized I was moving a lot of code around and this
had me thinking it's
perhaps easier to do the cleaning up first as Junio suggested.

Maybe then it'd be simpler to do implement this rather than move code around now
and then move code around when we implement the parsing methods we
spoke about earlier?

-- 
Regards,
Karthik Nayak

Re: [PATCH v2 00/10] port branch.c to use ref-filter's printing options

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> ... While doing so I realized I was moving a lot of code around
> and this had me thinking it's perhaps easier to do the cleaning up
> first as Junio suggested.
> 
> Maybe then it'd be simpler to do implement this rather than move
> code around now and then move code around when we implement the
> parsing methods we spoke about earlier?

When I suggested the approach, I hadn't have done any of what you
actually did, so I was primarily talking out of "hunch" (perhaps
that is what some people call "taste") but I expected something like
that may happen ;-).

Right now, you practically own the topic, in the sense that nobody
seems to be in a very urgent need to compete with you in parallel to
implement what we have discussed before you do, so I personally
would suggest whichever order you feel more comfortable and less
error-prone.

Thanks.

Re: [PATCH v2 00/10] port branch.c to use ref-filter's printing options

[Matthieu Moy]

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

> I personally would suggest whichever order you feel more comfortable
> and less error-prone.

This is a good summary, and I fully agree with it.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v2 00/10] port branch.c to use ref-filter's printing options

[Karthik Nayak]

On Mon, Oct 19, 2015 at 1:42 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> I personally would suggest whichever order you feel more comfortable
>> and less error-prone.
>
> This is a good summary, and I fully agree with it.

Well then, I'm working on the parsing part of it before this series :)

-- 
Regards,
Karthik Nayak