[PATCH 0/9] port branch.c to use ref-filter printing APIs

[PATCH 0/9] port branch.c to use ref-filter printing APIs

[Karthik Nayak]

This series ports over branch.c to use ref-filter APIs
This is the last part of unification of 'git branch -l', 'git tag -l'
and 'git for-each-ref'.

There are a few topics worth discussing here :

1. [7/9] This ensures that when we use the "%(upstream:track)" atom we
print "[gone]" for upstreams which are invalid. We either implement
this and try to mimic what branch.c does when we use the verbose
option or we can drop this in branch.c.

2. [8/9] The usage of get_format() function in branch.c which provides
us the needed format for printing. This seems overly complex to the
uninitiated as it uses a complex atom combinations to mimic the
existing branch printing format.

Karthik Nayak (9):
  ref-filter: implement %(if), %(then), and %(else) atoms
  ref-filter: implement %(if:equals=<string>) and
    %(if:notequals=<string>)
  ref-filter: add support for %(path) atom
  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
  branch: use ref-filter printing APIs
  branch: implement '--format' option

 Documentation/git-branch.txt       |   7 +-
 Documentation/git-for-each-ref.txt |  32 ++++-
 builtin/branch.c                   | 262 ++++++++++---------------------------
 ref-filter.c                       | 250 +++++++++++++++++++++++++++++++----
 ref-filter.h                       |   3 +
 t/t3203-branch-output.sh           |  11 ++
 t/t6040-tracking-info.sh           |  12 +-
 t/t6300-for-each-ref.sh            |  24 +++-
 t/t6302-for-each-ref-filter.sh     | 105 +++++++++++++++
 9 files changed, 480 insertions(+), 226 deletions(-)

-- 
2.6.0

[PATCH 1/9] 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 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. If the string contains only
whitespaces, then it is not considered. 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 |  23 +++++++-
 ref-filter.c                       | 116 ++++++++++++++++++++++++++++++++++---
 t/t6302-for-each-ref-filter.sh     |  48 +++++++++++++++
 3 files changed, 177 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 16b4ac5..768a512 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -134,9 +134,14 @@ 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.
 
 In addition to the above, for commit and tag objects, the header
 field names (`tree`, `parent`, `object`, `type`, and `tag`) can
@@ -169,6 +174,18 @@ 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, 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
 --------
diff --git a/ref-filter.c b/ref-filter.c
index dbd8fce..93b07f5 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,17 @@ struct contents {
 	struct object_id oid;
 };
 
+struct if_then_else {
+	unsigned int if_atom : 1,
+		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 +226,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 +247,85 @@ 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 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);
+	} else if (!if_then_else->condition_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);
+
+	if_then_else->if_atom = 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 = (struct if_then_else *)cur->at_end_data;
+
+	if (!if_then_else)
+		die(_("format: %%(then) atom used without an %%(if) atom"));
+	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 = (struct if_then_else *)state->stack->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_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 +333,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, 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 +1013,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..7ce1cc4 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -255,4 +255,52 @@ 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_done
-- 
2.6.0

[PATCH 2/9] 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 768a512..5c12c2f 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -141,7 +141,9 @@ 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.
+	printed. 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 93b07f5..da7723b 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 if_atom : 1,
 		then_atom : 1,
 		else_atom : 1,
@@ -277,8 +279,16 @@ 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_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))
+		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;
 	new->at_end = if_then_else_handler;
@@ -302,11 +312,19 @@ static void then_atom_handler(struct atom_value *atomv, struct ref_formatting_st
 	if (!if_then_else)
 		die(_("format: %%(then) atom used without an %%(if) atom"));
 	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);
 }
@@ -1013,8 +1031,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 7ce1cc4..d7f7a18 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -303,4 +303,22 @@ test_expect_success 'check %(if)...%(then)...%(else)...%(end) atoms' '
 	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 3/9] ref-filter: add support for %(path) atom

[Karthik Nayak]

This adds %(path) and %(path:short) atoms. The %(path) atom will print
the path of the given ref, while %(path:short) will only print the
subdirectory of the given ref.

Add tests and documentation for the same.
---
 Documentation/git-for-each-ref.txt |  5 +++++
 ref-filter.c                       | 17 +++++++++++++++++
 t/t6302-for-each-ref-filter.sh     | 39 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 61 insertions(+)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 5c12c2f..6a476ba 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -145,6 +145,11 @@ if::
 	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`.
+
 In addition to the above, for commit and tag objects, the header
 field names (`tree`, `parent`, `object`, `type`, and `tag`) can
 be used to specify the value in the header field.
diff --git a/ref-filter.c b/ref-filter.c
index da7723b..b0e86ae 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -58,6 +58,7 @@ static struct {
 	{ "if" },
 	{ "then" },
 	{ "else" },
+	{ "path" },
 };
 
 #define REF_FORMATTING_STATE_INIT  { 0, NULL }
@@ -1042,6 +1043,22 @@ 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;
 
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index d7f7a18..5557657 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -312,6 +312,7 @@ 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 &&
@@ -321,4 +322,42 @@ test_expect_success 'check %(if:notequals=<string>)' '
 	test_cmp expect actual
 '
 
+test_expect_success 'check %(path)' '
+	git for-each-ref --format="%(path)" >actual &&
+	cat >expect <<-\EOF &&
+	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 %(path:short)' '
+	git for-each-ref --format="%(path:short)" >actual &&
+	cat >expect <<-\EOF &&
+	heads
+	heads
+	odd
+	tags
+	tags
+	tags
+	tags
+	tags
+	tags
+	tags
+	tags
+	tags
+	EOF
+	test_cmp expect actual
+'
+
 test_done
-- 
2.6.0

[PATCH 4/9] 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 |  2 ++
 ref-filter.c                       | 39 ++++++++++++++++++++++++++++++--------
 t/t6300-for-each-ref.sh            | 22 +++++++++++++++++++++
 3 files changed, 55 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 6a476ba..5097915 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -103,6 +103,8 @@ 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.
 
 upstream::
 	The name of a local ref which can be considered ``upstream''
diff --git a/ref-filter.c b/ref-filter.c
index b0e86ae..223daeb 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -454,14 +454,37 @@ 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)) {
+		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++;
+			}
+			if (length < MINIMUM_ABBREV)
+				length = MINIMUM_ABBREV;
+			v->s = xstrdup(find_unique_abbrev(sha1, length));
+			free(to_free);
+		}
 		return 1;
 	}
 	return 0;
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 7c9bec7..7f675d2 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 5/9] 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.

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 ++++++++++++++++++++++++++++++++++++--
 2 files changed, 40 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 223daeb..15df421 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;
 
@@ -915,6 +916,37 @@ static inline char *copy_advance(char *dst, const char *src)
 	return dst;
 }
 
+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);
+}
+
 /*
  * Parse the object referred by ref, and grab needed value.
  */
@@ -953,9 +985,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;
-- 
2.6.0

[PATCH 6/9] ref-filter: introduce format_ref_array_item()

[Karthik Nayak]

Introduce format_ref_array_item() which will output the details of a
given ref_array_item as per the given format and quote_style to the
given strbuf.

This is derived from show_ref_array_item() which is now reduced to a
wrapper around format_ref_array_item().  show_ref_array_item() obtains
the strbuf and prints it the standard output.

This is needed when we port over the printing options of ref-filter to
branch.c.

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 15df421..099acd6 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1748,10 +1748,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;
@@ -1781,9 +1781,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 14d435e..e0b26e8 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 7/9] 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 099acd6..48b06e3 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1133,8 +1133,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 = xstrdup("[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 7f675d2..2a01645 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 8/9] 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.

Since branch.c is being ported to use ref-filter APIs to print the
required data, it is constricted to the constraints of printing as per
ref-filter. Which means branch.c can only print as per the atoms
available in ref-filter. Hence the "-vv" option of 'git branch' now
prints the upstream and its tracking details separately as
"[<upstream>] [<tracking info>]" instead of "[<upstream>: <tracking
info>]".

Make changes in /t/t6040-tracking-info.sh to reflect the 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>
---
 builtin/branch.c         | 258 ++++++++++++-----------------------------------
 t/t6040-tracking-info.sh |  12 +--
 2 files changed, 69 insertions(+), 201 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 67ef9f1..48fbca1 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;
@@ -477,12 +291,54 @@ 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)
+{
+	char *remote = NULL;
+	char *local = NULL;
+	char *final = NULL;
+
+	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));
+
+		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);
+
+	} 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);
+	}
+
+	free(local);
+	free(remote);
+
+	return final;
+}
+
 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 = get_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,20 @@ 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");
+			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..d110f26 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
 
@@ -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
 
-- 
2.6.0

[PATCH 9/9] 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 48fbca1..ae3ecfb 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 *get_format(struct ref_filter *filter, int maxwidth, const char *rem
 	return final;
 }
 
-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 = get_format(filter, maxwidth, remote_prefix);
+	if (!format)
+		format = to_free = get_format(filter, maxwidth, remote_prefix);
 	verify_ref_format(format);
 
 	/*
@@ -382,7 +384,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)
@@ -483,6 +485,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")),
@@ -523,6 +526,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(),
 	};
 
@@ -581,7 +585,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 1/9] ref-filter: implement %(if), %(then), and %(else) atoms

[Junio C Hamano]

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

> +static int is_empty(const char * s){
> +	while (*s != '\0') {
> +		if (!isspace(*s))
> +			return 0;
> +		s++;
> +	}
> +	return 1;
> +}

My knee-jerk reaction was "why is space so special?", but if a
caller really cared, it can do "%(if:not_equal=)%(something)%(then)"
to unignore spaces in %(something), so it is not a huge deal.  It
may be that ignoring spaces when checking if something is empty is
so common that this default is useful---I cannot tell offhand.


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

I notice that "we ignore space when evaluating the string before
%(then)" is not mentioned here.  That fact, and an example or two
that illustrates the situation where this "ignore spaces" behaviour
is useful, would be a good thing to document here.

Thanks.

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

[Junio C Hamano]

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

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

The fast that the patch touches only the narrow parts that are
specific to %(if),%(then) and %(else) and does not have to touch any
generic part (other than the populate_value() parser for obvious
reasons) is a good signal that tells us that the basic structure of
the code is very sound.  I very much like the direction in which
this series is going ;-)

Re: [PATCH 4/9] ref-filter: modify "%(objectname:short)" to take length

[Junio C Hamano]

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

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

It would have to be "short=<length>", not "short,<length>", if I
recall the previous discussion on width=<w>, etc., on the %(align)
atom.

Re: [PATCH 1/9] ref-filter: implement %(if), %(then), and %(else) atoms

[Karthik Nayak]

On Sat, Oct 3, 2015 at 2:15 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> +static int is_empty(const char * s){
>> +     while (*s != '\0') {
>> +             if (!isspace(*s))
>> +                     return 0;
>> +             s++;
>> +     }
>> +     return 1;
>> +}
>
> My knee-jerk reaction was "why is space so special?", but if a
> caller really cared, it can do "%(if:not_equal=)%(something)%(then)"
> to unignore spaces in %(something), so it is not a huge deal.  It
> may be that ignoring spaces when checking if something is empty is
> so common that this default is useful---I cannot tell offhand.
>
>

My reason for doing this was the "HEAD" atom which prints "*" or " ",
hence I thought strings with only spaces in general shouldn't be accepted.

>> +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.
>
> I notice that "we ignore space when evaluating the string before
> %(then)" is not mentioned here.  That fact, and an example or two
> that illustrates the situation where this "ignore spaces" behaviour
> is useful, would be a good thing to document here.
>

Oh Yes! Will do that :)

-- 
Regards,
Karthik Nayak

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

[Karthik Nayak]

On Sat, Oct 3, 2015 at 2:24 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> 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>
>> ---
>
> The fast that the patch touches only the narrow parts that are
> specific to %(if),%(then) and %(else) and does not have to touch any
> generic part (other than the populate_value() parser for obvious
> reasons) is a good signal that tells us that the basic structure of
> the code is very sound.  I very much like the direction in which
> this series is going ;-)
>

Thats nice to hear :)

-- 
Regards,
Karthik Nayak

Re: [PATCH 4/9] ref-filter: modify "%(objectname:short)" to take length

[Karthik Nayak]

On Sat, Oct 3, 2015 at 2:32 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> 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.
>
> It would have to be "short=<length>", not "short,<length>", if I
> recall the previous discussion on width=<w>, etc., on the %(align)
> atom.

Yea, I got confused by "The "align:" is followed by `<width>` and
`<position>` in any order
separated by a comma". Will change, Thanks.

-- 
Regards,
Karthik Nayak

Re: [PATCH 1/9] ref-filter: implement %(if), %(then), and %(else) atoms

[Matthieu Moy]

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

> Implement %(if), %(then) and %(else) atoms. Used as
> %(if)..%(then)..%(end) or %(if)..%(then)..%(else)..%(end).

I prefer ... to .., which often means "interval" as in HEAD^^..HEAD.

> If there is an atom with value or string literal after the %(if)

I find this explanation hard to read, and ambiguous: what does "atom
with value" mean?

> then everything after the %(then) is printed, else if the %(else) atom
> is used, then everything after %(else) is printed. If the string
> contains only whitespaces, then it is not considered.

"the string" is ambiguous again. I guess it's "what's between %(if) and
%(then)", but it could be clearer. And it's not clear what "not
considered" means.

My take on it:

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 string following %(then) is printed.
Otherwise, the string following %(else), if any, is printed.

> +When a scripting language specific quoting is in effect,

This may not be immediately clear to the reader. I'd add explicitly:

When a scripting language specific quoting is in effect (i.e. one of
`--shell`, `--perl`, `--python`, `--tcl` is used), ...

>  EXAMPLES
>  --------

This is just the context of the patch, but I read it as a hint that we
could add some examples with complex --format usage to illustrate the
theory above.

> +	if (if_then_else->condition_satisfied && if_then_else->else_atom) {
// cs && else
> +		strbuf_reset(&cur->output);
> +		pop_stack_element(&cur);
> +	} else if (if_then_else->else_atom) {
// !cs && else
> +		strbuf_swap(&cur->output, &prev->output);
> +		strbuf_reset(&cur->output);
> +		pop_stack_element(&cur);
> +	} else if (!if_then_else->condition_satisfied)
// !cs && !else
> +		strbuf_reset(&cur->output);

This if/else if/else if looks hard to read to me. I had to add the
comments above as a note to myself to get the actual full condition for
3 branches.

The reasoning would be clearer to me as:

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

> +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);
> +
> +	if_then_else->if_atom = 1;

Do you ever use this "if_atom"? It doesn't seem so in the current patch,
and it seems like a tautology to me: if you have a struct if_then_else,
then you have seen the %(if).

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

char * s -> 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;
> +
> +	if (!if_then_else)
> +		die(_("format: %%(then) atom used without an %%(if) atom"));

You've just casted at_end_data to if_then_else. if_then_else being not
NULL does not mean that it is properly typed. It can be the at_end_data
of another opening atom. What happens if you use
%(align)foo%(then)bar%(end)?

One way to be safer would be to check that cur->at_end does point to
if_then_else_handler. Or add information to struct ref_formatting_stack
(a Boolean is_if_then_else or an enum).

Also, you need to check that if_then_else->then_atom is not 1.

> +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;
> +
> +	if (!if_then_else)
> +		die(_("format: %%(else) atom used without an %%(if) atom"));

Same as above, I guess (not tested) %(align)...%(else) is accepted.

> +	if (!if_then_else->then_atom)
> +		die(_("format: %%(else) atom used without a %%(then) atom"));
> +	if_then_else->else_atom = 1;
> +	push_stack_element(&state->stack);

So, while parsing the %(else)...%(end), the stack contains both the
%(then)...%(else) part, and the %(else)...%(end).

I'm wondering if we can simplify this. We already know if the condition
is satisfied, and if it's not, we can just drop the %(then) part right
now, and write to the top of the stack normally (the %(end) atom will
only have to pop the string normally). But if the condition is not
satisfied, we need to preserve the %(then) part and need to do something
about the %(else).

> -	current->at_end(current);
> +	current->at_end(&state->stack);
> +
> +	/*  Stack may have been popped, hence reset the current pointer */

I'd say explicitly "... may have been popped within at_end, hence ..."

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

Re: [PATCH 3/9] ref-filter: add support for %(path) atom

[Matthieu Moy]

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

> This adds %(path) and %(path:short) atoms. The %(path) atom will print
> the path of the given ref, while %(path:short) will only print the
> subdirectory of the given ref.

What does "path" mean in this context? How is it different from
%(refname)?

I found the answer below, but I could not guess from the doc and commit
message. Actually, I'm not sure %(path) is the right name. If you want
the "file/directory" analogy, then %(dirname) would be better.

> +		} 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, '/');

This assumes you have two / in the fullrefname. It is normally the case,
but one can also create eg. refs/foo references. AFAIK, in this case sp
will be NULL, and you're then calling strchr(++NULL) which may segfault.

> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
> index d7f7a18..5557657 100755
> --- a/t/t6302-for-each-ref-filter.sh
> +++ b/t/t6302-for-each-ref-filter.sh
> @@ -312,6 +312,7 @@ test_expect_success 'check %(if:equals=<string>)' '
>  	test_cmp expect actual
>  '
>  
> +
>  test_expect_success 'check %(if:notequals=<string>)' '

Useless new blank line.

> +test_expect_success 'check %(path)' '
> +	git for-each-ref --format="%(path)" >actual &&
> +	cat >expect <<-\EOF &&
> +	refs/heads

You should add eg.

git update-ref refs/foo HEAD
git update-ref refs/foodir/bar/boz HEAD

before the test to check and document the behavior for such refnames.

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

Re: [PATCH 6/9] ref-filter: introduce format_ref_array_item()

[Matthieu Moy]

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

> Introduce format_ref_array_item() which will output the details of a
> given ref_array_item as per the given format and quote_style to the
> given strbuf.

Why do you need it in this series and you could do without for tag?

Going through PATCH 8/9, I guess there's something related to --column,
but tag also has --column so I'm puzzled.

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

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

[Matthieu Moy]

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

> diff --git a/ref-filter.c b/ref-filter.c
> index 099acd6..48b06e3 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1133,8 +1133,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 = xstrdup("[gone]");

I think just "[gone]" without the xstrdup is OK, and avoids leaking one
string.

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

Re: [PATCH 8/9] branch: use ref-filter printing APIs

[Matthieu Moy]

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

> -	if (upstream_is_gone) {
> -		if (show_upstream_ref)
> -			strbuf_addf(stat, _("[%s: gone]"), fancy.buf);

The old string was translated, and you're replacing it with one which
isn't.

I'm not a big fan of translation, so that change doesn't disturb me, but
people used to having "git branch" talk their native language here may
care.

Be careful: translation should happen only in porcelain. Typicall,
"for-each-ref" should anyway continue saying "gone" in english whatever
the configuration is.

See e.g. 7a76c28 (status: disable translation when --porcelain is used,
2014-03-20) for an example of translation only for porcelain (that was
for status, but also for ahead/behind/gone).

> +static char *get_format(struct ref_filter *filter, int maxwidth, const char *remote_prefix)

I'd call that "build_format", since "get_..." usually implies that the
value exists already and you're retrieving it.

> +{
> +	char *remote = NULL;
> +	char *local = NULL;
> +	char *final = NULL;
> +
> +	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));

OK, that is a hell of a block of code ;-).

You should factor the common portions out of these if/else. C allows you
to write several string litteral next to each other, so you can eg.

#define STAR_IF_HEAD "%%(if)%%(HEAD)%%(then)* %s%%(else)  %%(end)"
#define ALIGNED_REFNAME "%%(align:%d,left)%%(refname:short)%%(end)"

and then

STAR_IF_HEAD ALIGNED_REFNAME ...

Actually, this is not a performance-cricical piece of code at all, so I
think it's even better to build an strbuf little by little using
repeated strbuf_addf calls. This way you can do things like

strbuf_addf(fmt, "%%(if)%%(HEAD)%%(then)* %s%%(else)  %%(end)",
	    branch_get_color(BRANCH_COLOR_CURRENT));
strbuf_addf(fmt, "%%(align:%d,left)%%(refname:short)%%(end)", maxwidth);

which makes it much easier to pair the %s and the corresponding
branch_get_color() or the %d and maxwidth.

But fundamentally, I think this function is doing the right thing. The
function is a bit complex (I think it will be much nicer to read when
better factored), but 1) it allows getting rid of a lot of specific code
and 2) it's a proof that --format is expressive enough.

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

This corresponds to this part of the commit message:

> Since branch.c is being ported to use ref-filter APIs to print the
> required data, it is constricted to the constraints of printing as per
> ref-filter. Which means branch.c can only print as per the atoms
> available in ref-filter. Hence the "-vv" option of 'git branch' now
> prints the upstream and its tracking details separately as
> "[<upstream>] [<tracking info>]" instead of "[<upstream>: <tracking
> info>]".
> 
> Make changes in /t/t6040-tracking-info.sh to reflect the change.

nit: /t/t... -> t/t... (remove leading /)

That is a behavior change, and I actually prefered the previous one.

I actually don't understand why you can't get the old syntax: the []
around the refname come from the format string it get_format(), so you
could decide to change "[%s%%(upstream:short)%s]" to
"[%s%%(upstream:short)%s: ". The brackets around the status are a bit
more tricky, since they were here before your series. But we could have
a %(upstream:track,nobracket) to display just the status without the
brackets around.

Then, the code must deal with up-to-date branches for which no
" :ahead/behind" must be displayed. Probably one more if/then/else
nested in the first one.

So, it seems feasible to me to keep the old behavior with the new
system, even though it's going to be a bit tricky.

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

Re: [PATCH 1/9] ref-filter: implement %(if), %(then), and %(else) atoms

[Junio C Hamano]

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

> My take on it:
>
> 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 string following %(then) is printed.
> Otherwise, the string following %(else), if any, is printed.
>
>> +When a scripting language specific quoting is in effect,
>
> This may not be immediately clear to the reader. I'd add explicitly:
>
> When a scripting language specific quoting is in effect (i.e. one of
> `--shell`, `--perl`, `--python`, `--tcl` is used), ...

I found all the suggestions very good, except that the distinction
between "expands to" and "is printed" bothers me a bit, as they want
to mean exactly the same thing (imagine this whole thing were inside
another %(if)...%(then)).

>>  EXAMPLES
>>  --------
>
> This is just the context of the patch, but I read it as a hint that we
> could add some examples with complex --format usage to illustrate the
> theory above.

Yes ;-)

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

Also "{" must sit alone on the next line.

>> +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;
>> +
>> +	if (!if_then_else)
>> +		die(_("format: %%(then) atom used without an %%(if) atom"));
>
> You've just casted at_end_data to if_then_else. if_then_else being not
> NULL does not mean that it is properly typed. It can be the at_end_data
> of another opening atom. What happens if you use
> %(align)foo%(then)bar%(end)?
>
> One way to be safer would be to check that cur->at_end does point to
> if_then_else_handler.

Good eyes.  Thanks.

> So, while parsing the %(else)...%(end), the stack contains both the
> %(then)...%(else) part, and the %(else)...%(end).
>
> I'm wondering if we can simplify this. We already know if the condition
> is satisfied, and if it's not, we can just drop the %(then) part right
> now, and write to the top of the stack normally (the %(end) atom will
> only have to pop the string normally). But if the condition is not
> satisfied, we need to preserve the %(then) part and need to do something
> about the %(else).

Good suggestion.

Thanks.

Re: [PATCH 1/9] ref-filter: implement %(if), %(then), and %(else) atoms

[Matthieu Moy]

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

> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>> My take on it:
>>
>> 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 string following %(then) is printed.
>> Otherwise, the string following %(else), if any, is printed.
>
> I found all the suggestions very good, except that the distinction
> between "expands to" and "is printed" bothers me a bit, as they want
> to mean exactly the same thing (imagine this whole thing were inside
> another %(if)...%(then)).

True. Then let me try again:

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.

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

Re: [PATCH 1/9] ref-filter: implement %(if), %(then), and %(else) atoms

[Karthik Nayak]

On Sat, Oct 3, 2015 at 3:09 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Implement %(if), %(then) and %(else) atoms. Used as
>> %(if)..%(then)..%(end) or %(if)..%(then)..%(else)..%(end).
>
> I prefer ... to .., which often means "interval" as in HEAD^^..HEAD.
>

Seems good, will change.

>> If there is an atom with value or string literal after the %(if)
>
> I find this explanation hard to read, and ambiguous: what does "atom
> with value" mean?
>
>> then everything after the %(then) is printed, else if the %(else) atom
>> is used, then everything after %(else) is printed. If the string
>> contains only whitespaces, then it is not considered.
>
> "the string" is ambiguous again. I guess it's "what's between %(if) and
> %(then)", but it could be clearer. And it's not clear what "not
> considered" means.
>
> My take on it:
>
> 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 string following %(then) is printed.
> Otherwise, the string following %(else), if any, is printed.
>

This (the one you sent again after Junio's suggestion, looks better),
I'll use this thanks.

>> +When a scripting language specific quoting is in effect,
>
> This may not be immediately clear to the reader. I'd add explicitly:
>
> When a scripting language specific quoting is in effect (i.e. one of
> `--shell`, `--perl`, `--python`, `--tcl` is used), ...
>

Makes sense.

>>  EXAMPLES
>>  --------
>
> This is just the context of the patch, but I read it as a hint that we
> could add some examples with complex --format usage to illustrate the
> theory above.
>

I was thinking about adding :

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

>> +     if (if_then_else->condition_satisfied && if_then_else->else_atom) {
> // cs && else
>> +             strbuf_reset(&cur->output);
>> +             pop_stack_element(&cur);
>> +     } else if (if_then_else->else_atom) {
> // !cs && else
>> +             strbuf_swap(&cur->output, &prev->output);
>> +             strbuf_reset(&cur->output);
>> +             pop_stack_element(&cur);
>> +     } else if (!if_then_else->condition_satisfied)
> // !cs && !else
>> +             strbuf_reset(&cur->output);
>
> This if/else if/else if looks hard to read to me. I had to add the
> comments above as a note to myself to get the actual full condition for
> 3 branches.
>
> The reasoning would be clearer to me as:
>
> 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);
>

This looks neater thanks.

>> +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);
>> +
>> +     if_then_else->if_atom = 1;
>
> Do you ever use this "if_atom"? It doesn't seem so in the current patch,
> and it seems like a tautology to me: if you have a struct if_then_else,
> then you have seen the %(if).
>

Yea I'll remove that.

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

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 = (struct if_then_else *)cur->at_end_data;
>> +
>> +     if (!if_then_else)
>> +             die(_("format: %%(then) atom used without an %%(if) atom"));
>
> You've just casted at_end_data to if_then_else. if_then_else being not
> NULL does not mean that it is properly typed. It can be the at_end_data
> of another opening atom. What happens if you use
> %(align)foo%(then)bar%(end)?
>

Nice catch, didn't see that possibility.

> One way to be safer would be to check that cur->at_end does point to
> if_then_else_handler. Or add information to struct ref_formatting_stack
> (a Boolean is_if_then_else or an enum).
>

Checking cur->at_end with if_then_else_handler seems good to me.

> Also, you need to check that if_then_else->then_atom is not 1.
>

Ah! multiple usage of the same atom.

>> +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;
>> +
>> +     if (!if_then_else)
>> +             die(_("format: %%(else) atom used without an %%(if) atom"));
>
> Same as above, I guess (not tested) %(align)...%(else) is accepted.
>

Will change.

>> +     if (!if_then_else->then_atom)
>> +             die(_("format: %%(else) atom used without a %%(then) atom"));
>> +     if_then_else->else_atom = 1;
>> +     push_stack_element(&state->stack);
>
> So, while parsing the %(else)...%(end), the stack contains both the
> %(then)...%(else) part, and the %(else)...%(end).
>
> I'm wondering if we can simplify this. We already know if the condition
> is satisfied, and if it's not, we can just drop the %(then) part right
> now, and write to the top of the stack normally (the %(end) atom will
> only have to pop the string normally). But if the condition is not
> satisfied, we need to preserve the %(then) part and need to do something
> about the %(else).
>

I wanted to do something like this the problem is append_atom() and
append_literal()
would need to be informed about which part to ignore, and this moves
the code's logic
from the current handlers to append_atom() and append_literal(). Which I didn't
think was a nice way of doing this.

>> -     current->at_end(current);
>> +     current->at_end(&state->stack);
>> +
>> +     /*  Stack may have been popped, hence reset the current pointer */
>
> I'd say explicitly "... may have been popped within at_end, hence ..."
>

Will do.

-- 
Regards,
Karthik Nayak

Re: [PATCH 3/9] ref-filter: add support for %(path) atom

[Karthik Nayak]

On Sat, Oct 3, 2015 at 3:32 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> This adds %(path) and %(path:short) atoms. The %(path) atom will print
>> the path of the given ref, while %(path:short) will only print the
>> subdirectory of the given ref.
>
> What does "path" mean in this context? How is it different from
> %(refname)?
>
> I found the answer below, but I could not guess from the doc and commit
> message. Actually, I'm not sure %(path) is the right name. If you want
> the "file/directory" analogy, then %(dirname) would be better.
>

Noted will change.

>> +             } 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, '/');
>
> This assumes you have two / in the fullrefname. It is normally the case,
> but one can also create eg. refs/foo references. AFAIK, in this case sp
> will be NULL, and you're then calling strchr(++NULL) which may segfault.
>

Not really, but you made me think of another possible issue.

Assume refs/foo "sp = strchr(ref->refname, '/');" would set sp to point at
'/' and ++sp will now point at 'f'.

The problem now is refs/foo is supposed to print just "refs" whereas it'll
print "refs/foo". and %(dirname:short) is supposed to print "" whereas it'll
print "foo". Will look into this :)

>> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
>> index d7f7a18..5557657 100755
>> --- a/t/t6302-for-each-ref-filter.sh
>> +++ b/t/t6302-for-each-ref-filter.sh
>> @@ -312,6 +312,7 @@ test_expect_success 'check %(if:equals=<string>)' '
>>       test_cmp expect actual
>>  '
>>
>> +
>>  test_expect_success 'check %(if:notequals=<string>)' '
>
> Useless new blank line.
>

Will remove.

>> +test_expect_success 'check %(path)' '
>> +     git for-each-ref --format="%(path)" >actual &&
>> +     cat >expect <<-\EOF &&
>> +     refs/heads
>
> You should add eg.
>
> git update-ref refs/foo HEAD
> git update-ref refs/foodir/bar/boz HEAD
>
> before the test to check and document the behavior for such refnames.

Yeah makes sense.

-- 
Regards,
Karthik Nayak

Re: [PATCH 1/9] ref-filter: implement %(if), %(then), and %(else) atoms

[Junio C Hamano]

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

>> I found all the suggestions very good, except that the distinction
>> between "expands to" and "is printed" bothers me a bit, as they want
>> to mean exactly the same thing (imagine this whole thing were inside
>> another %(if)...%(then)).
>
> True. Then let me try again:
>
> 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.

Nice.  Thanks.

Re: [PATCH 3/9] ref-filter: add support for %(path) atom

[Matthieu Moy]

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

> On Sat, Oct 3, 2015 at 3:32 PM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>
>>> This adds %(path) and %(path:short) atoms. The %(path) atom will print
>>> the path of the given ref, while %(path:short) will only print the
>>> subdirectory of the given ref.
>>
>> What does "path" mean in this context? How is it different from
>> %(refname)?
>>
>> I found the answer below, but I could not guess from the doc and commit
>> message. Actually, I'm not sure %(path) is the right name. If you want
>> the "file/directory" analogy, then %(dirname) would be better.
>>
>
> Noted will change.

Note: I don't completely like %(dirname) either. I'm convinced it's
better than %(path), but there may be a better option.

>>> +             } 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, '/');
>>
>> This assumes you have two / in the fullrefname. It is normally the case,
>> but one can also create eg. refs/foo references. AFAIK, in this case sp
>> will be NULL, and you're then calling strchr(++NULL) which may segfault.
>
> Not really,

Oops, indeed, for refs/foo, sp points to / and ep is NULL. It would
segfault for any ref without a /. Currently, the only such ref is HEAD
and it is dealt with by the 'if' above, but that still sounds a bit
fragile to me. Ifever we allow listing refs like FETCH_HEAD, I'm not
sure we'll remember that and test %(path) on it.

Adding something like

if (!sp)
	continue;

after the first strchr would make me feel safer.

> but you made me think of another possible issue.
>
> Assume refs/foo "sp = strchr(ref->refname, '/');" would set sp to point at
> '/' and ++sp will now point at 'f'.
>
> The problem now is refs/foo is supposed to print just "refs" whereas it'll
> print "refs/foo". and %(dirname:short) is supposed to print "" whereas it'll
> print "foo". Will look into this :)

I think it's worse than that because ep will be NULL, and then you call

	v->s = xstrndup(sp, ep - sp);

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

Re: [PATCH 3/9] ref-filter: add support for %(path) atom

[Junio C Hamano]

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

>>>> This adds %(path) and %(path:short) atoms. The %(path) atom will print
>>>> the path of the given ref, while %(path:short) will only print the
>>>> subdirectory of the given ref.
>>>
>>> What does "path" mean in this context? How is it different from
>>> %(refname)?
>>>
>>> I found the answer below, but I could not guess from the doc and commit
>>> message. Actually, I'm not sure %(path) is the right name. If you want
>>> the "file/directory" analogy, then %(dirname) would be better.
>>>
>>
>> Noted will change.
>
> Note: I don't completely like %(dirname) either. I'm convinced it's
> better than %(path), but there may be a better option.

Is that a derived form of the refname, just like %(refname:short)
that is 'master' for a ref whose %(refname) is 'refs/heads/master'
is a derived form of %(refname), and ":short" is what tells the
formatting machinery what kind of derivation is desired?

If so would %(refname:dir) & %(refname:base) be more in line with
the overall structure?

Re: [PATCH 3/9] ref-filter: add support for %(path) atom

[Karthik Nayak]

On Mon, Oct 5, 2015 at 12:14 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Is that a derived form of the refname, just like %(refname:short)
> that is 'master' for a ref whose %(refname) is 'refs/heads/master'
> is a derived form of %(refname), and ":short" is what tells the
> formatting machinery what kind of derivation is desired?
>
> If so would %(refname:dir) & %(refname:base) be more in line with
> the overall structure?

This seems way better, I like these names more.

On Sun, Oct 4, 2015 at 11:21 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> On Sat, Oct 3, 2015 at 3:32 PM, Matthieu Moy
>> <Matthieu.Moy@grenoble-inp.fr> wrote:
>>> Karthik Nayak <karthik.188@gmail.com> writes:
>>>
>>>> This adds %(path) and %(path:short) atoms. The %(path) atom will print
>>>> the path of the given ref, while %(path:short) will only print the
>>>> subdirectory of the given ref.
>>>
>>> What does "path" mean in this context? How is it different from
>>> %(refname)?
>>>
>>> I found the answer below, but I could not guess from the doc and commit
>>> message. Actually, I'm not sure %(path) is the right name. If you want
>>> the "file/directory" analogy, then %(dirname) would be better.
>>>
>>
>> Noted will change.
>
> Note: I don't completely like %(dirname) either. I'm convinced it's
> better than %(path), but there may be a better option.
>

I like Junio's suggestion, stick with that?

>>>> +             } 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, '/');
>>>
>>> This assumes you have two / in the fullrefname. It is normally the case,
>>> but one can also create eg. refs/foo references. AFAIK, in this case sp
>>> will be NULL, and you're then calling strchr(++NULL) which may segfault.
>>
>> Not really,
>
> Oops, indeed, for refs/foo, sp points to / and ep is NULL. It would
> segfault for any ref without a /. Currently, the only such ref is HEAD
> and it is dealt with by the 'if' above, but that still sounds a bit
> fragile to me. Ifever we allow listing refs like FETCH_HEAD, I'm not
> sure we'll remember that and test %(path) on it.
>
> Adding something like
>
> if (!sp)
>         continue;
>
> after the first strchr would make me feel safer.
>

Good point! I'll add that.

>> but you made me think of another possible issue.
>>
>> Assume refs/foo "sp = strchr(ref->refname, '/');" would set sp to point at
>> '/' and ++sp will now point at 'f'.
>>
>> The problem now is refs/foo is supposed to print just "refs" whereas it'll
>> print "refs/foo". and %(dirname:short) is supposed to print "" whereas it'll
>> print "foo". Will look into this :)
>
> I think it's worse than that because ep will be NULL, and then you call
>
>         v->s = xstrndup(sp, ep - sp);
>

Ah yes, weirdly though my test didn't yield a problem.

~ git update-ref refs/foo master
~ git for-each-ref --format="%(refname) %(color:red)%(dirname:short)"
refs/foo foo
~ git for-each-ref --format="%(refname) %(color:red)%(dirname)"
refs/foo refs/foo

-- 
Regards,
Karthik Nayak

Re: [PATCH 3/9] ref-filter: add support for %(path) atom

[Matthieu Moy]

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

> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>>>>> This adds %(path) and %(path:short) atoms. The %(path) atom will print
>>>>> the path of the given ref, while %(path:short) will only print the
>>>>> subdirectory of the given ref.
>>>>
>>>> What does "path" mean in this context? How is it different from
>>>> %(refname)?
>>>>
>>>> I found the answer below, but I could not guess from the doc and commit
>>>> message. Actually, I'm not sure %(path) is the right name. If you want
>>>> the "file/directory" analogy, then %(dirname) would be better.
>>>>
>>>
>>> Noted will change.
>>
>> Note: I don't completely like %(dirname) either. I'm convinced it's
>> better than %(path), but there may be a better option.
>
> Is that a derived form of the refname, just like %(refname:short)
> that is 'master' for a ref whose %(refname) is 'refs/heads/master'
> is a derived form of %(refname), and ":short" is what tells the
> formatting machinery what kind of derivation is desired?
>
> If so would %(refname:dir) & %(refname:base) be more in line with
> the overall structure?

Yes, indeed much better. It's still about the refnames, so a specialized
version of %(refname) makes much more sense than a new atom.

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

Re: [PATCH 6/9] ref-filter: introduce format_ref_array_item()

[Karthik Nayak]

On Sat, Oct 3, 2015 at 5:32 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Introduce format_ref_array_item() which will output the details of a
>> given ref_array_item as per the given format and quote_style to the
>> given strbuf.
>
> Why do you need it in this series and you could do without for tag?
>
> Going through PATCH 8/9, I guess there's something related to --column,
> but tag also has --column so I'm puzzled.
>

The problem is with colors which tag.c doesn't use by default.
Here we need to print colors only if we're printing to a tty which supports
colors. which does not play well with the implementation of --column as
done in tag.c. Where, If I'm not wrong the --column option captures all
output, formats it and then prints it to stdout. Hence when using colors, we're
told that the printing isn't done to a tty which supports colors, hence we lose
out on colors.

I was trying something where we have a variable which knows when the column
option is used and hence explicitly lets us use colors, but seemed like a mess.
This was a clean of going about it.

-- 
Regards,
Karthik Nayak

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

[Karthik Nayak]

On Sat, Oct 3, 2015 at 5:38 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> diff --git a/ref-filter.c b/ref-filter.c
>> index 099acd6..48b06e3 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -1133,8 +1133,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 = xstrdup("[gone]");
>
> I think just "[gone]" without the xstrdup is OK, and avoids leaking one
> string.

Will do.

-- 
Regards,
Karthik Nayak

Re: [PATCH 6/9] ref-filter: introduce format_ref_array_item()

[Matthieu Moy]

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

> The problem is with colors which tag.c doesn't use by default.
> Here we need to print colors only if we're printing to a tty which supports
> colors.

Ah, and tag --format '%(color:...)' does not do the tty autodetection.

> which does not play well with the implementation of --column as done
> in tag.c. Where, If I'm not wrong the --column option captures all
> output, formats it and then prints it to stdout. Hence when using
> colors, we're told that the printing isn't done to a tty which
> supports colors, hence we lose out on colors.

What I don't understand is how --column is different from --no-column
wrt colors.

In any case, this should be explained better in comments.

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

Re: [PATCH 8/9] branch: use ref-filter printing APIs

[Karthik Nayak]

On Sat, Oct 3, 2015 at 6:11 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> -     if (upstream_is_gone) {
>> -             if (show_upstream_ref)
>> -                     strbuf_addf(stat, _("[%s: gone]"), fancy.buf);
>
> The old string was translated, and you're replacing it with one which
> isn't.
>
> I'm not a big fan of translation, so that change doesn't disturb me, but
> people used to having "git branch" talk their native language here may
> care.
>
> Be careful: translation should happen only in porcelain. Typicall,
> "for-each-ref" should anyway continue saying "gone" in english whatever
> the configuration is.
>
> See e.g. 7a76c28 (status: disable translation when --porcelain is used,
> 2014-03-20) for an example of translation only for porcelain (that was
> for status, but also for ahead/behind/gone).
>

I'm not how translation works, thanks for the hint, I'll look around.

>> +static char *get_format(struct ref_filter *filter, int maxwidth, const char *remote_prefix)
>
> I'd call that "build_format", since "get_..." usually implies that the
> value exists already and you're retrieving it.
>

Makes sense.

>> +{
>> +     char *remote = NULL;
>> +     char *local = NULL;
>> +     char *final = NULL;
>> +
>> +     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));
>
> OK, that is a hell of a block of code ;-).
>
> You should factor the common portions out of these if/else. C allows you
> to write several string litteral next to each other, so you can eg.
>
> #define STAR_IF_HEAD "%%(if)%%(HEAD)%%(then)* %s%%(else)  %%(end)"
> #define ALIGNED_REFNAME "%%(align:%d,left)%%(refname:short)%%(end)"
>
> and then
>
> STAR_IF_HEAD ALIGNED_REFNAME ...
>
> Actually, this is not a performance-cricical piece of code at all, so I
> think it's even better to build an strbuf little by little using
> repeated strbuf_addf calls. This way you can do things like
>
> strbuf_addf(fmt, "%%(if)%%(HEAD)%%(then)* %s%%(else)  %%(end)",
>             branch_get_color(BRANCH_COLOR_CURRENT));
> strbuf_addf(fmt, "%%(align:%d,left)%%(refname:short)%%(end)", maxwidth);
>
> which makes it much easier to pair the %s and the corresponding
> branch_get_color() or the %d and maxwidth.
>
> But fundamentally, I think this function is doing the right thing. The
> function is a bit complex (I think it will be much nicer to read when
> better factored), but 1) it allows getting rid of a lot of specific code
> and 2) it's a proof that --format is expressive enough.
>

I like the idea of using "#define" it might make things simpler.

Not sure about using a strbuf cause I don't see how that could make things
simpler, we'll end up with something similar to what we have currently. Just
that instead of using variables like "local", "remote" and "final" we'd just
make multiple calls to strbuf_addf().

>> @@ -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
>
> This corresponds to this part of the commit message:
>
>> Since branch.c is being ported to use ref-filter APIs to print the
>> required data, it is constricted to the constraints of printing as per
>> ref-filter. Which means branch.c can only print as per the atoms
>> available in ref-filter. Hence the "-vv" option of 'git branch' now
>> prints the upstream and its tracking details separately as
>> "[<upstream>] [<tracking info>]" instead of "[<upstream>: <tracking
>> info>]".
>>
>> Make changes in /t/t6040-tracking-info.sh to reflect the change.
>
> nit: /t/t... -> t/t... (remove leading /)
>
> That is a behavior change, and I actually prefered the previous one.
>
> I actually don't understand why you can't get the old syntax: the []
> around the refname come from the format string it get_format(), so you
> could decide to change "[%s%%(upstream:short)%s]" to
> "[%s%%(upstream:short)%s: ". The brackets around the status are a bit
> more tricky, since they were here before your series. But we could have
> a %(upstream:track,nobracket) to display just the status without the
> brackets around.
>
> Then, the code must deal with up-to-date branches for which no
> " :ahead/behind" must be displayed. Probably one more if/then/else
> nested in the first one.
>
> So, it seems feasible to me to keep the old behavior with the new
> system, even though it's going to be a bit tricky.
>

Wasn't that hard to do, I got this working, I was just reluctant on making
a new subvalue like "%(upstream:track,nobracket)" (no real reason, just didn't
want to ).

-- 
Regards,
Karthik Nayak

Re: [PATCH 8/9] branch: use ref-filter printing APIs

[Matthieu Moy]

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

> On Sat, Oct 3, 2015 at 6:11 PM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>> Actually, this is not a performance-cricical piece of code at all, so I
>> think it's even better to build an strbuf little by little using
>> repeated strbuf_addf calls. This way you can do things like
>>
>> strbuf_addf(fmt, "%%(if)%%(HEAD)%%(then)* %s%%(else)  %%(end)",
>>             branch_get_color(BRANCH_COLOR_CURRENT));
>> strbuf_addf(fmt, "%%(align:%d,left)%%(refname:short)%%(end)", maxwidth);
>>
>> which makes it much easier to pair the %s and the corresponding
>> branch_get_color() or the %d and maxwidth.
>>
>> But fundamentally, I think this function is doing the right thing. The
>> function is a bit complex (I think it will be much nicer to read when
>> better factored), but 1) it allows getting rid of a lot of specific code
>> and 2) it's a proof that --format is expressive enough.
>>
>
> I like the idea of using "#define" it might make things simpler.
>
> Not sure about using a strbuf cause I don't see how that could make things
> simpler, we'll end up with something similar to what we have
> currently.

It allows you to really break the way you build the string into several
small steps, and use if/then locally on steps which require it.

For example, you currently have

+        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));
+
+                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);
+
+        } else {
+                local = xstrfmt("%%(if)%%(HEAD)%%(then)* %s%%(else)  %%(end)%%(refname:short)%s",

The first bits of local are identical in all branches of the two-level
if/else. You could use something like

	strbuf_addf(format, "%%(if)%%(HEAD)%%(then)* %s%%(else)  %%(end)",
		    branch_get_color(BRANCH_COLOR_CURRENT));
	if (filter->verbose) {
		...
	}

to factor it out of the if/else. Similarly, the "if (filter->verbose >
1)" could be much smaller, and probably doesn't require an else branch
(just say "if very verbose, then add XYZ at this point in the format
string").

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

Re: [PATCH 8/9] branch: use ref-filter printing APIs

[Karthik Nayak]

On Tue, Oct 6, 2015 at 12:13 AM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> On Sat, Oct 3, 2015 at 6:11 PM, Matthieu Moy
>> <Matthieu.Moy@grenoble-inp.fr> wrote:
>>> Actually, this is not a performance-cricical piece of code at all, so I
>>> think it's even better to build an strbuf little by little using
>>> repeated strbuf_addf calls. This way you can do things like
>>>
>>> strbuf_addf(fmt, "%%(if)%%(HEAD)%%(then)* %s%%(else)  %%(end)",
>>>             branch_get_color(BRANCH_COLOR_CURRENT));
>>> strbuf_addf(fmt, "%%(align:%d,left)%%(refname:short)%%(end)", maxwidth);
>>>
>>> which makes it much easier to pair the %s and the corresponding
>>> branch_get_color() or the %d and maxwidth.
>>>
>>> But fundamentally, I think this function is doing the right thing. The
>>> function is a bit complex (I think it will be much nicer to read when
>>> better factored), but 1) it allows getting rid of a lot of specific code
>>> and 2) it's a proof that --format is expressive enough.
>>>
>>
>> I like the idea of using "#define" it might make things simpler.
>>
>> Not sure about using a strbuf cause I don't see how that could make things
>> simpler, we'll end up with something similar to what we have
>> currently.
>
> It allows you to really break the way you build the string into several
> small steps, and use if/then locally on steps which require it.
>
> For example, you currently have
>
> +        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));
> +
> +                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);
> +
> +        } else {
> +                local = xstrfmt("%%(if)%%(HEAD)%%(then)* %s%%(else)  %%(end)%%(refname:short)%s",
>
> The first bits of local are identical in all branches of the two-level
> if/else. You could use something like
>
>         strbuf_addf(format, "%%(if)%%(HEAD)%%(then)* %s%%(else)  %%(end)",
>                     branch_get_color(BRANCH_COLOR_CURRENT));
>         if (filter->verbose) {
>                 ...
>         }
>
> to factor it out of the if/else. Similarly, the "if (filter->verbose >
> 1)" could be much smaller, and probably doesn't require an else branch
> (just say "if very verbose, then add XYZ at this point in the format
> string").
>

If you look closely, thats only for the local branches, the remotes
have `align` atom when
printing in verbose.

So like I said, I couldn't really make it quite simpler. Maybe a line
or two, but using `#define`
I could cook up this:

char *remote = NULL;
char *final = NULL;

struct strbuf local = STRBUF_INIT;

strbuf_addf(&local, "%%(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)");

        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));
} else {
        strbuf_addf(&local, "%%(refname:short)%s",
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)%%(refname:base)%%(then)%s%%(else)%s%%(end)",
local.buf, remote);

strbuf_release(&local);
free(remote);

return final;

Couldn't see anything else I could break down.


-- 
Regards,
Karthik Nayak

Re: [PATCH 8/9] branch: use ref-filter printing APIs

[Matthieu Moy]

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

> If you look closely, thats only for the local branches, the remotes
> have `align` atom when
> printing in verbose.

Yes, but that's already one thing factored out of the if, even if it's
just for local.

Actually, I think you can also factor some parts out of the
%(if:notequals=remotes). In 'local', you have an %(if) to display either
"* " or "  ", and in remote you always start with "  ". Why not always
apply the %(if), and let it display "  " if not displaying the current
branch? Similarly, the "verbose" part of remote branches seems like a
particular case of the one for local ones (remotes don't have tracking
branches, so the tracking info should expand to the empty string).

To go a bit further, you can pre-build a string or strbuf aligned_short
with value like "%%(align:20,left)%%(refname:short)%%(end)" and use it
where needed (it's not a constant because you have to introduce maxwidth
into the string, so it's not a candidate for #define).

> I could cook up this:

Your mailer broke the formatting, so it looks terrible, but from what I
could parse, it's already much better than the previous one. It's not a
matter of size of the function, I very much prefer reading 10 lines of
nice code than 4 lines like

> +                        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));

;-).

One obvious issue with the initial version was this big hard-to-parse
block, but another one is that the code did not make it easy to
understand what was changing depending on which branch of the if, and
depending on local/remote. It's getting much easier already.

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

Re: [PATCH 6/9] ref-filter: introduce format_ref_array_item()

[Karthik Nayak]

On Mon, Oct 5, 2015 at 2:19 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>> which does not play well with the implementation of --column as done
>> in tag.c. Where, If I'm not wrong the --column option captures all
>> output, formats it and then prints it to stdout. Hence when using
>> colors, we're told that the printing isn't done to a tty which
>> supports colors, hence we lose out on colors.
>
> What I don't understand is how --column is different from --no-column
> wrt colors.
>
> In any case, this should be explained better in comments.

Well, If we use column the way we do in tag.c then it'll replace the tty
and color will not print color because it will assume the output tty doesn't
support colors.

I hope that's what you're asking

-- 
Regards,
Karthik Nayak

Re: [PATCH 6/9] ref-filter: introduce format_ref_array_item()

[Matthieu Moy]

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

> On Mon, Oct 5, 2015 at 2:19 PM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>> which does not play well with the implementation of --column as done
>>> in tag.c. Where, If I'm not wrong the --column option captures all
>>> output, formats it and then prints it to stdout. Hence when using
>>> colors, we're told that the printing isn't done to a tty which
>>> supports colors, hence we lose out on colors.
>>
>> What I don't understand is how --column is different from --no-column
>> wrt colors.
>>
>> In any case, this should be explained better in comments.
>
> Well, If we use column the way we do in tag.c then it'll replace the tty
> and color will not print color because it will assume the output tty doesn't
> support colors.
>
> I hope that's what you're asking

Looking a bit more closely at the code, I think I understand what's
going on. branch.c used print_columns which does all the clever things
wrt columns display. tag.c used run_column_filter which pipes the output
to "git column" (hence, indeed, color detection does not work since
we're writting to a pipe).

The branch.c way seems good to me (why fork another process when we can
format the list ourselves ?). Probably tag.c could (should?) be modified
to use it too. It should just be justified, like, replacing this commit
message with:

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

and adding a comment next to if (column_active(colopts)) { in patch
8/9:

	/* format to a string_list to let print_columns() do its job */

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

Re: [PATCH 6/9] ref-filter: introduce format_ref_array_item()

[Karthik Nayak]

On Wed, Oct 7, 2015 at 1:08 AM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> On Mon, Oct 5, 2015 at 2:19 PM, Matthieu Moy
>> <Matthieu.Moy@grenoble-inp.fr> wrote:
>>> Karthik Nayak <karthik.188@gmail.com> writes:
>>>> which does not play well with the implementation of --column as done
>>>> in tag.c. Where, If I'm not wrong the --column option captures all
>>>> output, formats it and then prints it to stdout. Hence when using
>>>> colors, we're told that the printing isn't done to a tty which
>>>> supports colors, hence we lose out on colors.
>>>
>>> What I don't understand is how --column is different from --no-column
>>> wrt colors.
>>>
>>> In any case, this should be explained better in comments.
>>
>> Well, If we use column the way we do in tag.c then it'll replace the tty
>> and color will not print color because it will assume the output tty doesn't
>> support colors.
>>
>> I hope that's what you're asking
>
> Looking a bit more closely at the code, I think I understand what's
> going on. branch.c used print_columns which does all the clever things
> wrt columns display. tag.c used run_column_filter which pipes the output
> to "git column" (hence, indeed, color detection does not work since
> we're writting to a pipe).
>
> The branch.c way seems good to me (why fork another process when we can
> format the list ourselves ?). Probably tag.c could (should?) be modified
> to use it too. It should just be justified, like, replacing this commit
> message with:
>

Yes! this :)


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

Looks Good.

> and adding a comment next to if (column_active(colopts)) { in patch
> 8/9:
>
>         /* format to a string_list to let print_columns() do its job */
>

Will do this, thanks a bunch

-- 
Regards,
Karthik Nayak

Re: [PATCH 8/9] branch: use ref-filter printing APIs

[Karthik Nayak]

On Wed, Oct 7, 2015 at 12:33 AM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> If you look closely, thats only for the local branches, the remotes
>> have `align` atom when
>> printing in verbose.
>
> Yes, but that's already one thing factored out of the if, even if it's
> just for local.
>
> Actually, I think you can also factor some parts out of the
> %(if:notequals=remotes). In 'local', you have an %(if) to display either
> "* " or "  ", and in remote you always start with "  ". Why not always
> apply the %(if), and let it display "  " if not displaying the current
> branch? Similarly, the "verbose" part of remote branches seems like a
> particular case of the one for local ones (remotes don't have tracking
> branches, so the tracking info should expand to the empty string).

We could factor out the " " and the "* " printing. I'll do that

>
> To go a bit further, you can pre-build a string or strbuf aligned_short
> with value like "%%(align:20,left)%%(refname:short)%%(end)" and use it
> where needed (it's not a constant because you have to introduce maxwidth
> into the string, so it's not a candidate for #define).
>

Again, the remote has a remote prefix here, so we can't really factor
it out much.

>> I could cook up this:
>
> Your mailer broke the formatting, so it looks terrible, but from what I
> could parse, it's already much better than the previous one. It's not a
> matter of size of the function, I very much prefer reading 10 lines of
> nice code than 4 lines like
>

I get what you're saying I'll see if I can factor out more.

>> +                        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));
>
> ;-).
>
> One obvious issue with the initial version was this big hard-to-parse
> block, but another one is that the code did not make it easy to
> understand what was changing depending on which branch of the if, and
> depending on local/remote. It's getting much easier already.
>

Yea, we can say that :)

-- 
Regards,
Karthik Nayak

Re: [PATCH 8/9] branch: use ref-filter printing APIs

[Matthieu Moy]

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

> On Wed, Oct 7, 2015 at 12:33 AM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>
>> To go a bit further, you can pre-build a string or strbuf aligned_short
>> with value like "%%(align:20,left)%%(refname:short)%%(end)" and use it
>> where needed (it's not a constant because you have to introduce maxwidth
>> into the string, so it's not a candidate for #define).
>>
>
> Again, the remote has a remote prefix here, so we can't really factor
> it out much.

Ah, OK, there's a %s containing the remote prefix in "remote" that is
not there in "local", and in the last version you sent it's already
factored for verbose > 1 and verbose == 1. Indeed, there's nothing more
to gain here.

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

Re: [PATCH 8/9] branch: use ref-filter printing APIs

[Karthik Nayak]

On Wed, Oct 7, 2015 at 11:28 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> On Wed, Oct 7, 2015 at 12:33 AM, Matthieu Moy
>> <Matthieu.Moy@grenoble-inp.fr> wrote:
>>
>>> To go a bit further, you can pre-build a string or strbuf aligned_short
>>> with value like "%%(align:20,left)%%(refname:short)%%(end)" and use it
>>> where needed (it's not a constant because you have to introduce maxwidth
>>> into the string, so it's not a candidate for #define).
>>>
>>
>> Again, the remote has a remote prefix here, so we can't really factor
>> it out much.
>
> Ah, OK, there's a %s containing the remote prefix in "remote" that is
> not there in "local", and in the last version you sent it's already
> factored for verbose > 1 and verbose == 1. Indeed, there's nothing more
> to gain here.

I'll send version2 of the patch series to the list then :)

-- 
Regards,
Karthik Nayak