[RFC/PATCH] Port branch.c to use ref-filter APIs

[RFC/PATCH] Port branch.c to use ref-filter APIs

[Karthik Nayak]

This is part of my GSoC project to unify git tag -l, git branch -l,
git for-each-ref.  This patch series is continued from: Gmane (v6)
http://article.gmane.org/gmane.comp.version-control.git/274726

This is a RFC version and I'm sending to ensure that I'm on the right path.

This version also doesn't use the printing options provided by
branch.c. I wanted to discuss how exactly to go about that, because in
branch.c, we might need to change the --format based on attributes
such as abbrev, verbose and so on. But ref-filter expects us to verify
the format before filtering.

Any tips or suggestions are welcome, thanks all :)

-- 
Regards,
Karthik Nayak

[RFC/PATCH 01/11] ref-filter: add "%(objectname:size=X)" option

[Karthik Nayak]

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

Add support for %(objectname:size=X) where X is a number.
This will print the first X characters of an objectname.
The minimum value for X is 5. Hence any value lesser than
5 will default to 5 characters.

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 | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/ref-filter.c b/ref-filter.c
index 0a34924..4c5e3f8 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -196,6 +196,8 @@ 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)
 {
+	const char *p;
+
 	if (!strcmp(name, "objectname")) {
 		char *s = xmalloc(41);
 		strcpy(s, sha1_to_hex(sha1));
@@ -206,6 +208,13 @@ static int grab_objectname(const char *name, const unsigned char *sha1,
 		v->s = xstrdup(find_unique_abbrev(sha1, DEFAULT_ABBREV));
 		return 1;
 	}
+	if (skip_prefix(name, "objectname:size=", &p)) {
+		unsigned int size = atoi(p);
+		if (size < 5)
+			size = 5;
+		v->s = xstrdup(find_unique_abbrev(sha1, size));
+		return 1;
+	}
 	return 0;
 }
 
-- 
2.4.6

[RFC/PATCH 02/11] ref-filter: add 'colornext' atom

[Karthik Nayak]

The 'colornext' atom allows us to color only the succeeding atom with
a particular color. This resets any previous color preferences set. It
is not compatible with `padright`. This is required for printing
formats like `git branch -vv` where the upstream is colored in blue.

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 |  5 +++++
 ref-filter.c                       | 20 +++++++++++++++++++-
 ref-filter.h                       |  4 +++-
 t/t6302-for-each-ref-filter.sh     | 16 ++++++++++++++++
 4 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 2b60aee..9dc02aa 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -133,6 +133,11 @@ padright::
 	padding. If the `value` is greater than the atom length, then
 	no padding is performed.
 
+colornext::
+	Change output color only for the next atom. Followed by
+	`<:colorname>`.  Not compatible with `padright` and resets any
+	previous `color`, if set.
+
 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 4c5e3f8..3ab4ab9 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -57,6 +57,7 @@ static struct {
 	{ "HEAD" },
 	{ "color" },
 	{ "padright" },
+	{ "colornext" },
 };
 
 /*
@@ -712,6 +713,15 @@ static void populate_value(struct ref_array_item *ref)
 			v->modifier_atom = 1;
 			v->pad_to_right = 1;
 			continue;
+		} else if (starts_with(name, "colornext:")) {
+			char color[COLOR_MAXLEN] = "";
+
+			if (color_parse(name + 10, color) < 0)
+				die(_("unable to parse format"));
+			v->s = xstrdup(color);
+			v->modifier_atom = 1;
+			v->color_next = 1;
+			continue;
 		} else
 			continue;
 
@@ -1256,7 +1266,13 @@ void ref_array_sort(struct ref_sorting *sorting, struct ref_array *array)
 static void apply_formatting_state(struct ref_formatting_state *state,
 				   struct atom_value *v, struct strbuf *value)
 {
-	if (state->pad_to_right) {
+	if (state->color_next && state->pad_to_right)
+		die(_("cannot use `colornext` and `padright` together"));
+	if (state->color_next) {
+		strbuf_addf(value, "%s%s%s", state->color_next, v->s, GIT_COLOR_RESET);
+		return;
+	}
+	else if (state->pad_to_right) {
 		if (!is_utf8(v->s))
 			strbuf_addf(value, "%-*s", state->pad_to_right, v->s);
 		else {
@@ -1346,6 +1362,8 @@ static void emit(const char *cp, const char *ep)
 static void store_formatting_state(struct ref_formatting_state *state,
 				   struct atom_value *atomv)
 {
+	if (atomv->color_next)
+		state->color_next = atomv->s;
 	if (atomv->pad_to_right)
 		state->pad_to_right = atomv->ul;
 }
diff --git a/ref-filter.h b/ref-filter.h
index fcf469e..8c82fd1 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -21,12 +21,14 @@ struct atom_value {
 	const char *s;
 	unsigned long ul; /* used for sorting when not FIELD_STR */
 	unsigned int modifier_atom : 1, /*  atoms which act as modifiers for the next atom */
-		pad_to_right : 1;
+		pad_to_right : 1,
+		color_next : 1;
 };
 
 struct ref_formatting_state {
 	int quote_style;
 	unsigned int pad_to_right;
+	const char *color_next;
 };
 
 struct ref_sorting {
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index 68688a9..6aad069 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -133,4 +133,20 @@ test_expect_success 'reverse version sort' '
 	test_cmp expect actual
 '
 
+get_color ()
+{
+	git config --get-color no.such.slot "$1"
+}
+
+cat >expect <<EOF &&
+$(get_color green)foo1.10$(get_color reset)||
+$(get_color green)foo1.3$(get_color reset)||
+$(get_color green)foo1.6$(get_color reset)||
+EOF
+
+test_expect_success 'check `colornext` format option' '
+	git for-each-ref --format="%(colornext:green)%(refname:short)||" | grep "foo" >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.4.6

[RFC/PATCH 03/11] ref-filter: add option to filter only branches

[Karthik Nayak]

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

Add an option in 'filter_refs()' to use 'for_each_branch_ref()'
and filter refs. This type checking is done by adding a
'FILTER_REFS_BRANCHES' in 'ref-filter.h'.

Add an option in 'ref_filter_handler()' to filter different
types of branches by calling 'filter_branch_kind()' which
checks for the type of branch needed.

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 | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 ref-filter.h | 10 ++++++++--
 2 files changed, 55 insertions(+), 2 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 3ab4ab9..3f40144 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1054,6 +1054,46 @@ static const unsigned char *match_points_at(struct sha1_array *points_at,
 	return NULL;
 }
 
+/*
+ * Checks if a given refname is a branch and returns the kind of
+ * branch it is. If not a branch, 0 is returned.
+ */
+static int filter_branch_kind(struct ref_filter *filter, const char *refname)
+{
+	int kind, i;
+
+	static struct {
+		int kind;
+		const char *prefix;
+	} ref_kind[] = {
+		{ REF_LOCAL_BRANCH, "refs/heads/" },
+		{ REF_REMOTE_BRANCH, "refs/remotes/" },
+	};
+
+	/*  If no kind is specified, no need to filter */
+	if (!filter->branch_kind)
+		return REF_NO_BRANCH_FILTERING;
+
+	for (i = 0; i < ARRAY_SIZE(ref_kind); i++) {
+		if (starts_with(refname, ref_kind[i].prefix)) {
+			kind = ref_kind[i].kind;
+			break;
+		}
+	}
+
+	if (ARRAY_SIZE(ref_kind) <= i) {
+		if (!strcmp(refname, "HEAD"))
+			kind = REF_DETACHED_HEAD;
+		else
+			return 0;
+	}
+
+	if ((filter->branch_kind & kind) == 0)
+		return 0;
+
+	return kind;
+}
+
 /* Allocate space for a new ref_array_item and copy the objectname and flag to it */
 static struct ref_array_item *new_ref_array_item(const char *refname,
 						 const unsigned char *objectname,
@@ -1079,6 +1119,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
 	struct ref_filter *filter = ref_cbdata->filter;
 	struct ref_array_item *ref;
 	struct commit *commit = NULL;
+	unsigned int kind;
 
 	if (flag & REF_BAD_NAME) {
 		  warning("ignoring ref with broken name %s", refname);
@@ -1090,6 +1131,9 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
 		return 0;
 	}
 
+	if (!(kind = filter_branch_kind(filter, refname)))
+		return 0;
+
 	if (!filter_pattern_match(filter, refname))
 		return 0;
 
@@ -1118,6 +1162,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
 	 */
 	ref = new_ref_array_item(refname, oid->hash, flag);
 	ref->commit = commit;
+	ref->kind = kind;
 
 	REALLOC_ARRAY(ref_cbdata->array->items, ref_cbdata->array->nr + 1);
 	ref_cbdata->array->items[ref_cbdata->array->nr++] = ref;
@@ -1208,6 +1253,8 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int
 		ret = for_each_ref(ref_filter_handler, &ref_cbdata);
 	else if (type & FILTER_REFS_TAGS)
 		ret = for_each_tag_ref_fullpath(ref_filter_handler, &ref_cbdata);
+	else if (type & FILTER_REFS_BRANCHES)
+		ret = for_each_rawref(ref_filter_handler, &ref_cbdata);
 	else if (type)
 		die("filter_refs: invalid type");
 
diff --git a/ref-filter.h b/ref-filter.h
index 8c82fd1..a021b04 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -16,6 +16,12 @@
 #define FILTER_REFS_INCLUDE_BROKEN 0x1
 #define FILTER_REFS_ALL 0x2
 #define FILTER_REFS_TAGS 0x4
+#define FILTER_REFS_BRANCHES 0x8
+
+#define REF_DETACHED_HEAD   0x01
+#define REF_LOCAL_BRANCH    0x02
+#define REF_REMOTE_BRANCH   0x04
+#define REF_NO_BRANCH_FILTERING 0x08
 
 struct atom_value {
 	const char *s;
@@ -40,7 +46,7 @@ struct ref_sorting {
 
 struct ref_array_item {
 	unsigned char objectname[20];
-	int flag;
+	int flag, kind;
 	const char *symref;
 	struct commit *commit;
 	struct atom_value *value;
@@ -66,7 +72,7 @@ struct ref_filter {
 
 	unsigned int with_commit_tag_algo : 1,
 		match_as_path : 1;
-	unsigned int lines;
+	unsigned int lines, branch_kind;
 };
 
 struct ref_filter_cbdata {
-- 
2.4.6

[RFC/PATCH 04/11] ref-filter: add 'ifexists' atom

[Karthik Nayak]

The 'ifexists' atom allows us to print a required format if the
preceeding atom has a value. If the preceeding atom has no value then
the format given is not printed. e.g. to print "[<refname>]" we can
now use the format "%(ifexists:[%s])%(refname)".

Add documentation and test 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 |  8 ++++++++
 ref-filter.c                       | 37 ++++++++++++++++++++++++++++++++++---
 ref-filter.h                       |  5 +++--
 t/t6302-for-each-ref-filter.sh     | 21 +++++++++++++++++++++
 4 files changed, 66 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 9dc02aa..4424020 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -138,6 +138,14 @@ colornext::
 	`<:colorname>`.  Not compatible with `padright` and resets any
 	previous `color`, if set.
 
+ifexists::
+	Print required string only if the next atom specified in the
+	'--format' option exists.
+	e.g. --format="%(ifexists:[%s])%(symref)" prints the symref
+	like "[<symref>]" only if the ref has a symref.  This was
+	incorporated to simulate the output of 'git branch -vv', where
+	we need to display the upstream branch in square brackets.
+
 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 3f40144..ff5a16b 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -58,6 +58,7 @@ static struct {
 	{ "color" },
 	{ "padright" },
 	{ "colornext" },
+	{ "ifexists" },
 };
 
 /*
@@ -722,6 +723,13 @@ static void populate_value(struct ref_array_item *ref)
 			v->modifier_atom = 1;
 			v->color_next = 1;
 			continue;
+		} else if (starts_with(name, "ifexists:")) {
+			skip_prefix(name, "ifexists:", &v->s);
+			if (!*v->s)
+				die(_("no string given with 'ifexists:'"));
+			v->modifier_atom = 1;
+			v->ifexists = 1;
+			continue;
 		} else
 			continue;
 
@@ -1315,11 +1323,32 @@ static void apply_formatting_state(struct ref_formatting_state *state,
 {
 	if (state->color_next && state->pad_to_right)
 		die(_("cannot use `colornext` and `padright` together"));
-	if (state->color_next) {
+	if (state->pad_to_right && state->ifexists)
+		die(_("cannot use 'align' and 'ifexists' together"));
+	if (state->color_next && !state->ifexists) {
 		strbuf_addf(value, "%s%s%s", state->color_next, v->s, GIT_COLOR_RESET);
 		return;
-	}
-	else if (state->pad_to_right) {
+	} else if (state->ifexists) {
+		const char *sp = state->ifexists;
+
+		while (*sp) {
+			if (*sp != '%') {
+				strbuf_addch(value, *sp++);
+				continue;
+			} else if (sp[1] == '%') {
+				strbuf_addch(value, *sp++);
+				continue;
+			} else if (sp[1] == 's') {
+				if (state->color_next)
+					strbuf_addf(value, "%s%s%s", state->color_next, v->s, GIT_COLOR_RESET);
+				else
+					strbuf_addstr(value, v->s);
+				sp += 2;
+			}
+		}
+
+		return;
+	} else if (state->pad_to_right) {
 		if (!is_utf8(v->s))
 			strbuf_addf(value, "%-*s", state->pad_to_right, v->s);
 		else {
@@ -1413,6 +1442,8 @@ static void store_formatting_state(struct ref_formatting_state *state,
 		state->color_next = atomv->s;
 	if (atomv->pad_to_right)
 		state->pad_to_right = atomv->ul;
+	if (atomv->ifexists)
+		state->ifexists = atomv->s;
 }
 
 static void reset_formatting_state(struct ref_formatting_state *state)
diff --git a/ref-filter.h b/ref-filter.h
index a021b04..7d1871d 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -28,13 +28,14 @@ struct atom_value {
 	unsigned long ul; /* used for sorting when not FIELD_STR */
 	unsigned int modifier_atom : 1, /*  atoms which act as modifiers for the next atom */
 		pad_to_right : 1,
-		color_next : 1;
+		color_next : 1,
+		ifexists : 1;
 };
 
 struct ref_formatting_state {
 	int quote_style;
 	unsigned int pad_to_right;
-	const char *color_next;
+	const char *color_next, *ifexists;
 };
 
 struct ref_sorting {
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index 6aad069..29ed97b 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -149,4 +149,25 @@ test_expect_success 'check `colornext` format option' '
 	test_cmp expect actual
 '
 
+test_expect_success 'check `ifexists` format option' '
+	cat >expect <<-\EOF &&
+	[foo1.10]
+	[foo1.3]
+	[foo1.6]
+	EOF
+	git for-each-ref --format="%(ifexists:[%s])%(refname:short)" | grep "foo" >actual &&
+	test_cmp expect actual
+'
+
+cat >expect <<EOF &&
+[$(get_color green)foo1.10$(get_color reset)]||foo1.10
+[$(get_color green)foo1.3$(get_color reset)]||foo1.3
+[$(get_color green)foo1.6$(get_color reset)]||foo1.6
+EOF
+
+test_expect_success 'check `ifexists` with `colornext` format option' '
+	git for-each-ref --format="%(ifexists:[%s])%(colornext:green)%(refname:short)||%(refname:short)" | grep "foo" >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.4.6

[RFC/PATCH 05/11] branch: fix width computation

[Karthik Nayak]

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

Remove unnecessary variables from ref_list and ref_item
which were used for width computation. Make other changes
accordingly. This patch is a precursor for porting branch.c
to use ref-filter APIs.

Based-on-patch-by: Jeff King <peff@peff.net>
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 | 61 +++++++++++++++++++++++++++++---------------------------
 1 file changed, 32 insertions(+), 29 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 4fc8beb..b058b74 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -282,14 +282,14 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 struct ref_item {
 	char *name;
 	char *dest;
-	unsigned int kind, width;
+	unsigned int kind;
 	struct commit *commit;
 	int ignore;
 };
 
 struct ref_list {
 	struct rev_info revs;
-	int index, alloc, maxwidth, verbose, abbrev;
+	int index, alloc, verbose, abbrev;
 	struct ref_item *list;
 	struct commit_list *with_commit;
 	int kinds;
@@ -386,15 +386,8 @@ static int append_ref(const char *refname, const struct object_id *oid, int flag
 	newitem->name = xstrdup(refname);
 	newitem->kind = kind;
 	newitem->commit = commit;
-	newitem->width = utf8_strwidth(refname);
 	newitem->dest = resolve_symref(orig_refname, prefix);
 	newitem->ignore = 0;
-	/* adjust for "remotes/" */
-	if (newitem->kind == REF_REMOTE_BRANCH &&
-	    ref_list->kinds != REF_REMOTE_BRANCH)
-		newitem->width += 8;
-	if (newitem->width > ref_list->maxwidth)
-		ref_list->maxwidth = newitem->width;
 
 	return 0;
 }
@@ -505,11 +498,12 @@ static void add_verbose_info(struct strbuf *out, struct ref_item *item,
 }
 
 static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
-			   int abbrev, int current, char *prefix)
+			   int abbrev, int current, const char *remote_prefix)
 {
 	char c;
 	int color;
 	struct strbuf out = STRBUF_INIT, name = STRBUF_INIT;
+	const char *prefix = "";
 
 	if (item->ignore)
 		return;
@@ -520,6 +514,7 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
 		break;
 	case REF_REMOTE_BRANCH:
 		color = BRANCH_COLOR_REMOTE;
+		prefix = remote_prefix;
 		break;
 	default:
 		color = BRANCH_COLOR_PLAIN;
@@ -557,16 +552,21 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
 	strbuf_release(&out);
 }
 
-static int calc_maxwidth(struct ref_list *refs)
+static int calc_maxwidth(struct ref_list *refs, int remote_bonus)
 {
-	int i, w = 0;
+	int i, max = 0;
 	for (i = 0; i < refs->index; i++) {
+		struct ref_item *it = &refs->list[i];
+		int w = utf8_strwidth(it->name);
+
 		if (refs->list[i].ignore)
 			continue;
-		if (refs->list[i].width > w)
-			w = refs->list[i].width;
+		if (it->kind == REF_REMOTE_BRANCH)
+			w += remote_bonus;
+		if (w > max)
+			max = w;
 	}
-	return w;
+	return max;
 }
 
 static char *get_head_description(void)
@@ -600,21 +600,18 @@ static char *get_head_description(void)
 	return strbuf_detach(&desc, NULL);
 }
 
-static void show_detached(struct ref_list *ref_list)
+static void show_detached(struct ref_list *ref_list, int maxwidth)
 {
 	struct commit *head_commit = lookup_commit_reference_gently(head_sha1, 1);
 
 	if (head_commit && is_descendant_of(head_commit, ref_list->with_commit)) {
 		struct ref_item item;
 		item.name = get_head_description();
-		item.width = utf8_strwidth(item.name);
 		item.kind = REF_LOCAL_BRANCH;
 		item.dest = NULL;
 		item.commit = head_commit;
 		item.ignore = 0;
-		if (item.width > ref_list->maxwidth)
-			ref_list->maxwidth = item.width;
-		print_ref_item(&item, ref_list->maxwidth, ref_list->verbose, ref_list->abbrev, 1, "");
+		print_ref_item(&item, maxwidth, ref_list->verbose, ref_list->abbrev, 1, "");
 		free(item.name);
 	}
 }
@@ -624,6 +621,16 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru
 	int i;
 	struct append_ref_cb cb;
 	struct ref_list ref_list;
+	int maxwidth = 0;
+	const char *remote_prefix = "";
+
+	/*
+	 * If we are listing more than just remote branches,
+	 * then remote branches will have a "remotes/" prefix.
+	 * We need to account for this in the width.
+	 */
+	if (kinds != REF_REMOTE_BRANCH)
+		remote_prefix = "remotes/";
 
 	memset(&ref_list, 0, sizeof(ref_list));
 	ref_list.kinds = kinds;
@@ -667,26 +674,22 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru
 			clear_commit_marks(item->commit, ALL_REV_FLAGS);
 		}
 		clear_commit_marks(filter, ALL_REV_FLAGS);
-
-		if (verbose)
-			ref_list.maxwidth = calc_maxwidth(&ref_list);
 	}
+	if (verbose)
+		maxwidth = calc_maxwidth(&ref_list, strlen(remote_prefix));
 
 	qsort(ref_list.list, ref_list.index, sizeof(struct ref_item), ref_cmp);
 
 	detached = (detached && (kinds & REF_LOCAL_BRANCH));
 	if (detached && match_patterns(pattern, "HEAD"))
-		show_detached(&ref_list);
+		show_detached(&ref_list, maxwidth);
 
 	for (i = 0; i < ref_list.index; i++) {
 		int current = !detached &&
 			(ref_list.list[i].kind == REF_LOCAL_BRANCH) &&
 			!strcmp(ref_list.list[i].name, head);
-		char *prefix = (kinds != REF_REMOTE_BRANCH &&
-				ref_list.list[i].kind == REF_REMOTE_BRANCH)
-				? "remotes/" : "";
-		print_ref_item(&ref_list.list[i], ref_list.maxwidth, verbose,
-			       abbrev, current, prefix);
+		print_ref_item(&ref_list.list[i], maxwidth, verbose,
+			       abbrev, current, remote_prefix);
 	}
 
 	free_ref_list(&ref_list);
-- 
2.4.6

[RFC/PATCH 06/11] branch: roll show_detached HEAD into regular ref_list

[Karthik Nayak]

Remove show_detached() and make detached HEAD to be rolled into
regular ref_list by adding REF_DETACHED_HEAD as a kind of branch and
supporting the same in append_ref().

Bump get_head_description() to the top.

Based-on-patch-by: Jeff King <peff@peff.net>
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 | 122 ++++++++++++++++++++++++++++---------------------------
 1 file changed, 63 insertions(+), 59 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index b058b74..f3d83d0 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -30,6 +30,7 @@ static const char * const builtin_branch_usage[] = {
 
 #define REF_LOCAL_BRANCH    0x01
 #define REF_REMOTE_BRANCH   0x02
+#define REF_DETACHED_HEAD   0x04
 
 static const char *head;
 static unsigned char head_sha1[20];
@@ -352,8 +353,12 @@ static int append_ref(const char *refname, const struct object_id *oid, int flag
 			break;
 		}
 	}
-	if (ARRAY_SIZE(ref_kind) <= i)
-		return 0;
+	if (ARRAY_SIZE(ref_kind) <= i) {
+		if (!strcmp(refname, "HEAD"))
+			kind = REF_DETACHED_HEAD;
+		else
+			return 0;
+	}
 
 	/* Don't add types the caller doesn't want */
 	if ((kind & ref_list->kinds) == 0)
@@ -497,6 +502,37 @@ static void add_verbose_info(struct strbuf *out, struct ref_item *item,
 	strbuf_release(&subject);
 }
 
+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 print_ref_item(struct ref_item *item, int maxwidth, int verbose,
 			   int abbrev, int current, const char *remote_prefix)
 {
@@ -504,6 +540,7 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
 	int color;
 	struct strbuf out = STRBUF_INIT, name = STRBUF_INIT;
 	const char *prefix = "";
+	const char *desc = item->name;
 
 	if (item->ignore)
 		return;
@@ -516,6 +553,10 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
 		color = BRANCH_COLOR_REMOTE;
 		prefix = remote_prefix;
 		break;
+	case REF_DETACHED_HEAD:
+		color = BRANCH_COLOR_CURRENT;
+		desc = get_head_description();
+		break;
 	default:
 		color = BRANCH_COLOR_PLAIN;
 		break;
@@ -527,7 +568,7 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
 		color = BRANCH_COLOR_CURRENT;
 	}
 
-	strbuf_addf(&name, "%s%s", prefix, item->name);
+	strbuf_addf(&name, "%s%s", prefix, desc);
 	if (verbose) {
 		int utf8_compensation = strlen(name.buf) - utf8_strwidth(name.buf);
 		strbuf_addf(&out, "%c %s%-*s%s", c, branch_get_color(color),
@@ -569,56 +610,9 @@ static int calc_maxwidth(struct ref_list *refs, int remote_bonus)
 	return max;
 }
 
-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 show_detached(struct ref_list *ref_list, int maxwidth)
-{
-	struct commit *head_commit = lookup_commit_reference_gently(head_sha1, 1);
-
-	if (head_commit && is_descendant_of(head_commit, ref_list->with_commit)) {
-		struct ref_item item;
-		item.name = get_head_description();
-		item.kind = REF_LOCAL_BRANCH;
-		item.dest = NULL;
-		item.commit = head_commit;
-		item.ignore = 0;
-		print_ref_item(&item, maxwidth, ref_list->verbose, ref_list->abbrev, 1, "");
-		free(item.name);
-	}
-}
-
 static int print_ref_list(int kinds, int detached, int verbose, int abbrev, struct commit_list *with_commit, const char **pattern)
 {
-	int i;
+	int i, index;
 	struct append_ref_cb cb;
 	struct ref_list ref_list;
 	int maxwidth = 0;
@@ -643,6 +637,8 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru
 	cb.pattern = pattern;
 	cb.ret = 0;
 	for_each_rawref(append_ref, &cb);
+	if (detached)
+		head_ref(append_ref, &cb);
 	/*
 	 * The following implementation is currently duplicated in ref-filter. It
 	 * will eventually be removed when we port branch.c to use ref-filter APIs.
@@ -678,15 +674,20 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru
 	if (verbose)
 		maxwidth = calc_maxwidth(&ref_list, strlen(remote_prefix));
 
-	qsort(ref_list.list, ref_list.index, sizeof(struct ref_item), ref_cmp);
+	index = ref_list.index;
+
+	/* Print detached heads before sorting and printing the rest */
+	if (detached && (ref_list.list[index - 1].kind == REF_DETACHED_HEAD) &&
+	    !strcmp(ref_list.list[index - 1].name, head)) {
+		print_ref_item(&ref_list.list[index - 1], maxwidth, verbose, abbrev,
+			       1, remote_prefix);
+		index -= 1;
+	}
 
-	detached = (detached && (kinds & REF_LOCAL_BRANCH));
-	if (detached && match_patterns(pattern, "HEAD"))
-		show_detached(&ref_list, maxwidth);
+	qsort(ref_list.list, index, sizeof(struct ref_item), ref_cmp);
 
-	for (i = 0; i < ref_list.index; i++) {
-		int current = !detached &&
-			(ref_list.list[i].kind == REF_LOCAL_BRANCH) &&
+	for (i = 0; i < index; i++) {
+		int current = !detached && (ref_list.list[i].kind == REF_LOCAL_BRANCH) &&
 			!strcmp(ref_list.list[i].name, head);
 		print_ref_item(&ref_list.list[i], maxwidth, verbose,
 			       abbrev, current, remote_prefix);
@@ -913,7 +914,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 			die(_("branch name required"));
 		return delete_branches(argc, argv, delete > 1, kinds, quiet);
 	} else if (list) {
-		int ret = print_ref_list(kinds, detached, verbose, abbrev,
+		int ret;
+		if (kinds & REF_LOCAL_BRANCH)
+			kinds |= REF_DETACHED_HEAD;
+		ret = print_ref_list(kinds, detached, verbose, abbrev,
 					 with_commit, argv);
 		print_columns(&output, colopts, NULL);
 		string_list_clear(&output, 0);
-- 
2.4.6

[RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer

[Karthik Nayak]

We check if given ref is the current branch in print_ref_list().  Move
this check to print_ref_item() where it is checked right before
printing.

Based-on-patch-by: Jeff King <peff@peff.net>
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 | 29 ++++++++++++-----------------
 1 file changed, 12 insertions(+), 17 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index f3d83d0..ba9cbad 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -534,9 +534,9 @@ static char *get_head_description(void)
 }
 
 static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
-			   int abbrev, int current, const char *remote_prefix)
+			   int abbrev, int detached, const char *remote_prefix)
 {
-	char c;
+	char c = ' ';
 	int color;
 	struct strbuf out = STRBUF_INIT, name = STRBUF_INIT;
 	const char *prefix = "";
@@ -547,7 +547,11 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
 
 	switch (item->kind) {
 	case REF_LOCAL_BRANCH:
-		color = BRANCH_COLOR_LOCAL;
+		if (!detached && !strcmp(item->name, head)) {
+			color = BRANCH_COLOR_CURRENT;
+			c = '*';
+		} else
+			color = BRANCH_COLOR_LOCAL;
 		break;
 	case REF_REMOTE_BRANCH:
 		color = BRANCH_COLOR_REMOTE;
@@ -556,18 +560,13 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
 	case REF_DETACHED_HEAD:
 		color = BRANCH_COLOR_CURRENT;
 		desc = get_head_description();
+		c = '*';
 		break;
 	default:
 		color = BRANCH_COLOR_PLAIN;
 		break;
 	}
 
-	c = ' ';
-	if (current) {
-		c = '*';
-		color = BRANCH_COLOR_CURRENT;
-	}
-
 	strbuf_addf(&name, "%s%s", prefix, desc);
 	if (verbose) {
 		int utf8_compensation = strlen(name.buf) - utf8_strwidth(name.buf);
@@ -677,21 +676,17 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru
 	index = ref_list.index;
 
 	/* Print detached heads before sorting and printing the rest */
-	if (detached && (ref_list.list[index - 1].kind == REF_DETACHED_HEAD) &&
-	    !strcmp(ref_list.list[index - 1].name, head)) {
+	if (detached) {
 		print_ref_item(&ref_list.list[index - 1], maxwidth, verbose, abbrev,
-			       1, remote_prefix);
+			       detached, remote_prefix);
 		index -= 1;
 	}
 
 	qsort(ref_list.list, index, sizeof(struct ref_item), ref_cmp);
 
-	for (i = 0; i < index; i++) {
-		int current = !detached && (ref_list.list[i].kind == REF_LOCAL_BRANCH) &&
-			!strcmp(ref_list.list[i].name, head);
+	for (i = 0; i < index; i++)
 		print_ref_item(&ref_list.list[i], maxwidth, verbose,
-			       abbrev, current, remote_prefix);
-	}
+			       abbrev, detached, remote_prefix);
 
 	free_ref_list(&ref_list);
 
-- 
2.4.6

[RFC/PATCH 08/11] branch: drop non-commit error reporting

[Karthik Nayak]

Based-on-patch-by: Jeff King <peff@peff.net>
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 | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index ba9cbad..8d0521e 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -313,7 +313,6 @@ static char *resolve_symref(const char *src, const char *prefix)
 struct append_ref_cb {
 	struct ref_list *ref_list;
 	const char **pattern;
-	int ret;
 };
 
 static int match_patterns(const char **pattern, const char *refname)
@@ -370,10 +369,8 @@ static int append_ref(const char *refname, const struct object_id *oid, int flag
 	commit = NULL;
 	if (ref_list->verbose || ref_list->with_commit || merge_filter != NO_FILTER) {
 		commit = lookup_commit_reference_gently(oid->hash, 1);
-		if (!commit) {
-			cb->ret = error(_("branch '%s' does not point at a commit"), refname);
+		if (!commit)
 			return 0;
-		}
 
 		/* Filter with with_commit if specified */
 		if (!is_descendant_of(commit, ref_list->with_commit))
@@ -609,7 +606,7 @@ static int calc_maxwidth(struct ref_list *refs, int remote_bonus)
 	return max;
 }
 
-static int print_ref_list(int kinds, int detached, int verbose, int abbrev, struct commit_list *with_commit, const char **pattern)
+static void print_ref_list(int kinds, int detached, int verbose, int abbrev, struct commit_list *with_commit, const char **pattern)
 {
 	int i, index;
 	struct append_ref_cb cb;
@@ -634,7 +631,6 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru
 		init_revisions(&ref_list.revs, NULL);
 	cb.ref_list = &ref_list;
 	cb.pattern = pattern;
-	cb.ret = 0;
 	for_each_rawref(append_ref, &cb);
 	if (detached)
 		head_ref(append_ref, &cb);
@@ -689,11 +685,6 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru
 			       abbrev, detached, remote_prefix);
 
 	free_ref_list(&ref_list);
-
-	if (cb.ret)
-		error(_("some refs could not be read"));
-
-	return cb.ret;
 }
 
 static void rename_branch(const char *oldname, const char *newname, int force)
@@ -909,14 +900,13 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 			die(_("branch name required"));
 		return delete_branches(argc, argv, delete > 1, kinds, quiet);
 	} else if (list) {
-		int ret;
 		if (kinds & REF_LOCAL_BRANCH)
 			kinds |= REF_DETACHED_HEAD;
-		ret = print_ref_list(kinds, detached, verbose, abbrev,
+		print_ref_list(kinds, detached, verbose, abbrev,
 					 with_commit, argv);
 		print_columns(&output, colopts, NULL);
 		string_list_clear(&output, 0);
-		return ret;
+		return 0;
 	}
 	else if (edit_description) {
 		const char *branch_name;
-- 
2.4.6

[RFC/PATCH 09/11] branch.c: use 'ref-filter' data structures

[Karthik Nayak]

Make 'branch.c' use 'ref-filter' data structures and make changes to
support the new data structures. This is a part of the process of
porting 'branch.c' to use 'ref-filter' APIs.

This is a temporary step before porting 'branch.c' to use 'ref-filter'
completely. As this is a temporary step, most of the code introduced
here will be removed when 'branch.c' is ported over to use
'ref-filter' APIs

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

diff --git a/builtin/branch.c b/builtin/branch.c
index 8d0521e..df74527 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -19,6 +19,7 @@
 #include "column.h"
 #include "utf8.h"
 #include "wt-status.h"
+#include "ref-filter.h"
 
 static const char * const builtin_branch_usage[] = {
 	N_("git branch [<options>] [-r | -a] [--merged | --no-merged]"),
@@ -28,10 +29,6 @@ static const char * const builtin_branch_usage[] = {
 	NULL
 };
 
-#define REF_LOCAL_BRANCH    0x01
-#define REF_REMOTE_BRANCH   0x02
-#define REF_DETACHED_HEAD   0x04
-
 static const char *head;
 static unsigned char head_sha1[20];
 
@@ -53,13 +50,6 @@ enum color_branch {
 	BRANCH_COLOR_UPSTREAM = 5
 };
 
-static enum merge_filter {
-	NO_FILTER = 0,
-	SHOW_NOT_MERGED,
-	SHOW_MERGED
-} merge_filter;
-static unsigned char merge_filter_ref[20];
-
 static struct string_list output = STRING_LIST_INIT_DUP;
 static unsigned int colopts;
 
@@ -280,22 +270,6 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 	return(ret);
 }
 
-struct ref_item {
-	char *name;
-	char *dest;
-	unsigned int kind;
-	struct commit *commit;
-	int ignore;
-};
-
-struct ref_list {
-	struct rev_info revs;
-	int index, alloc, verbose, abbrev;
-	struct ref_item *list;
-	struct commit_list *with_commit;
-	int kinds;
-};
-
 static char *resolve_symref(const char *src, const char *prefix)
 {
 	unsigned char sha1[20];
@@ -310,11 +284,6 @@ static char *resolve_symref(const char *src, const char *prefix)
 	return xstrdup(dst);
 }
 
-struct append_ref_cb {
-	struct ref_list *ref_list;
-	const char **pattern;
-};
-
 static int match_patterns(const char **pattern, const char *refname)
 {
 	if (!*pattern)
@@ -327,11 +296,22 @@ static int match_patterns(const char **pattern, const char *refname)
 	return 0;
 }
 
+static void ref_array_append(struct ref_array *array, const char *refname)
+{
+	size_t len = strlen(refname);
+	struct ref_array_item *ref = xcalloc(1, sizeof(struct ref_array_item) + len + 1);
+	memcpy(ref->refname, refname, len);
+	ref->refname[len] = '\0';
+	REALLOC_ARRAY(array->items, array->nr + 1);
+	array->items[array->nr++] = ref;
+}
+
 static int append_ref(const char *refname, const struct object_id *oid, int flags, void *cb_data)
 {
-	struct append_ref_cb *cb = (struct append_ref_cb *)(cb_data);
-	struct ref_list *ref_list = cb->ref_list;
-	struct ref_item *newitem;
+	struct ref_filter_cbdata *cb = (struct ref_filter_cbdata *)(cb_data);
+	struct ref_filter *filter = cb->filter;
+	struct ref_array *array = cb->array;
+	struct ref_array_item *item;
 	struct commit *commit;
 	int kind, i;
 	const char *prefix, *orig_refname = refname;
@@ -360,59 +340,47 @@ static int append_ref(const char *refname, const struct object_id *oid, int flag
 	}
 
 	/* Don't add types the caller doesn't want */
-	if ((kind & ref_list->kinds) == 0)
+	if ((kind & filter->branch_kind) == 0)
 		return 0;
 
-	if (!match_patterns(cb->pattern, refname))
+	if (!match_patterns(filter->name_patterns, refname))
 		return 0;
 
 	commit = NULL;
-	if (ref_list->verbose || ref_list->with_commit || merge_filter != NO_FILTER) {
+	if (filter->verbose || filter->with_commit || filter->merge != REF_FILTER_MERGED_NONE) {
 		commit = lookup_commit_reference_gently(oid->hash, 1);
 		if (!commit)
 			return 0;
 
 		/* Filter with with_commit if specified */
-		if (!is_descendant_of(commit, ref_list->with_commit))
+		if (!is_descendant_of(commit, filter->with_commit))
 			return 0;
 
-		if (merge_filter != NO_FILTER)
-			add_pending_object(&ref_list->revs,
+		if (filter->merge != REF_FILTER_MERGED_NONE)
+			add_pending_object(array->revs,
 					   (struct object *)commit, refname);
 	}
 
-	ALLOC_GROW(ref_list->list, ref_list->index + 1, ref_list->alloc);
+	ref_array_append(array, refname);
+	item = array->items[array->nr - 1];
 
 	/* Record the new item */
-	newitem = &(ref_list->list[ref_list->index++]);
-	newitem->name = xstrdup(refname);
-	newitem->kind = kind;
-	newitem->commit = commit;
-	newitem->dest = resolve_symref(orig_refname, prefix);
-	newitem->ignore = 0;
+	item->kind = kind;
+	item->commit = commit;
+	item->symref = resolve_symref(orig_refname, prefix);
+	item->ignore = 0;
 
 	return 0;
 }
 
-static void free_ref_list(struct ref_list *ref_list)
-{
-	int i;
-
-	for (i = 0; i < ref_list->index; i++) {
-		free(ref_list->list[i].name);
-		free(ref_list->list[i].dest);
-	}
-	free(ref_list->list);
-}
-
 static int ref_cmp(const void *r1, const void *r2)
 {
-	struct ref_item *c1 = (struct ref_item *)(r1);
-	struct ref_item *c2 = (struct ref_item *)(r2);
+	struct ref_array_item *c1 = *((struct ref_array_item **)r1);
+	struct ref_array_item *c2 = *((struct ref_array_item **)r2);
 
 	if (c1->kind != c2->kind)
 		return c1->kind - c2->kind;
-	return strcmp(c1->name, c2->name);
+	return strcmp(c1->refname, c2->refname);
 }
 
 static void fill_tracking_info(struct strbuf *stat, const char *branch_name,
@@ -477,8 +445,8 @@ static void fill_tracking_info(struct strbuf *stat, const char *branch_name,
 	free(ref);
 }
 
-static void add_verbose_info(struct strbuf *out, struct ref_item *item,
-			     int verbose, int abbrev)
+static void add_verbose_info(struct strbuf *out, struct ref_array_item *item,
+			     struct ref_filter *filter)
 {
 	struct strbuf subject = STRBUF_INIT, stat = STRBUF_INIT;
 	const char *sub = _(" **** invalid ref ****");
@@ -490,10 +458,10 @@ static void add_verbose_info(struct strbuf *out, struct ref_item *item,
 	}
 
 	if (item->kind == REF_LOCAL_BRANCH)
-		fill_tracking_info(&stat, item->name, verbose > 1);
+		fill_tracking_info(&stat, item->refname, filter->verbose > 1);
 
 	strbuf_addf(out, " %s %s%s",
-		find_unique_abbrev(item->commit->object.sha1, abbrev),
+		find_unique_abbrev(item->commit->object.sha1, filter->abbrev),
 		stat.buf, sub);
 	strbuf_release(&stat);
 	strbuf_release(&subject);
@@ -530,21 +498,21 @@ static char *get_head_description(void)
 	return strbuf_detach(&desc, NULL);
 }
 
-static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
-			   int abbrev, int detached, const char *remote_prefix)
+static void print_ref_item(struct ref_array_item *item, int maxwidth,
+			   struct ref_filter *filter, const char *remote_prefix)
 {
 	char c = ' ';
 	int color;
 	struct strbuf out = STRBUF_INIT, name = STRBUF_INIT;
 	const char *prefix = "";
-	const char *desc = item->name;
+	const char *desc = item->refname;
 
 	if (item->ignore)
 		return;
 
 	switch (item->kind) {
 	case REF_LOCAL_BRANCH:
-		if (!detached && !strcmp(item->name, head)) {
+		if (!filter->detached && !strcmp(item->refname, head)) {
 			color = BRANCH_COLOR_CURRENT;
 			c = '*';
 		} else
@@ -565,7 +533,7 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
 	}
 
 	strbuf_addf(&name, "%s%s", prefix, desc);
-	if (verbose) {
+	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,
@@ -574,13 +542,13 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
 		strbuf_addf(&out, "%c %s%s%s", c, branch_get_color(color),
 			    name.buf, branch_get_color(BRANCH_COLOR_RESET));
 
-	if (item->dest)
-		strbuf_addf(&out, " -> %s", item->dest);
-	else if (verbose)
+	if (item->symref)
+		strbuf_addf(&out, " -> %s", item->symref);
+	else if (filter->verbose)
 		/* " f7c0c00 [ahead 58, behind 197] vcs-svn: drop obj_pool.h" */
-		add_verbose_info(&out, item, verbose, abbrev);
+		add_verbose_info(&out, item, filter);
 	if (column_active(colopts)) {
-		assert(!verbose && "--column and --verbose are incompatible");
+		assert(!filter->verbose && "--column and --verbose are incompatible");
 		string_list_append(&output, out.buf);
 	} else {
 		printf("%s\n", out.buf);
@@ -589,14 +557,14 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
 	strbuf_release(&out);
 }
 
-static int calc_maxwidth(struct ref_list *refs, int remote_bonus)
+static int calc_maxwidth(struct ref_array *refs, int remote_bonus)
 {
 	int i, max = 0;
-	for (i = 0; i < refs->index; i++) {
-		struct ref_item *it = &refs->list[i];
-		int w = utf8_strwidth(it->name);
+	for (i = 0; i < refs->nr; i++) {
+		struct ref_array_item *it = refs->items[i];
+		int w = utf8_strwidth(it->refname);
 
-		if (refs->list[i].ignore)
+		if (it->ignore)
 			continue;
 		if (it->kind == REF_REMOTE_BRANCH)
 			w += remote_bonus;
@@ -606,85 +574,76 @@ static int calc_maxwidth(struct ref_list *refs, int remote_bonus)
 	return max;
 }
 
-static void print_ref_list(int kinds, int detached, int verbose, int abbrev, struct commit_list *with_commit, const char **pattern)
+static void print_ref_list(struct ref_filter *filter)
 {
 	int i, index;
-	struct append_ref_cb cb;
-	struct ref_list ref_list;
+	struct ref_array array;
+	struct ref_filter_cbdata data;
 	int maxwidth = 0;
 	const char *remote_prefix = "";
+	struct rev_info revs;
 
 	/*
 	 * If we are listing more than just remote branches,
 	 * then remote branches will have a "remotes/" prefix.
 	 * We need to account for this in the width.
 	 */
-	if (kinds != REF_REMOTE_BRANCH)
+	if (filter->branch_kind != REF_REMOTE_BRANCH)
 		remote_prefix = "remotes/";
 
-	memset(&ref_list, 0, sizeof(ref_list));
-	ref_list.kinds = kinds;
-	ref_list.verbose = verbose;
-	ref_list.abbrev = abbrev;
-	ref_list.with_commit = with_commit;
-	if (merge_filter != NO_FILTER)
-		init_revisions(&ref_list.revs, NULL);
-	cb.ref_list = &ref_list;
-	cb.pattern = pattern;
-	for_each_rawref(append_ref, &cb);
-	if (detached)
-		head_ref(append_ref, &cb);
+	memset(&array, 0, sizeof(array));
+	if (filter->merge != REF_FILTER_MERGED_NONE)
+		init_revisions(&revs, NULL);
+
+	data.array = &array;
+	data.filter = filter;
+	array.revs = &revs;
+
+	for_each_rawref(append_ref, &data);
+	if (filter->detached)
+		head_ref(append_ref, &data);
 	/*
 	 * The following implementation is currently duplicated in ref-filter. It
 	 * will eventually be removed when we port branch.c to use ref-filter APIs.
 	 */
-	if (merge_filter != NO_FILTER) {
-		struct commit *filter;
-		filter = lookup_commit_reference_gently(merge_filter_ref, 0);
-		if (!filter)
-			die(_("object '%s' does not point to a commit"),
-			    sha1_to_hex(merge_filter_ref));
-
-		filter->object.flags |= UNINTERESTING;
-		add_pending_object(&ref_list.revs,
-				   (struct object *) filter, "");
-		ref_list.revs.limited = 1;
-
-		if (prepare_revision_walk(&ref_list.revs))
+	if (filter->merge != REF_FILTER_MERGED_NONE) {
+		filter->merge_commit->object.flags |= UNINTERESTING;
+		add_pending_object(&revs, &filter->merge_commit->object, "");
+		revs.limited = 1;
+
+		if (prepare_revision_walk(&revs))
 			die(_("revision walk setup failed"));
 
-		for (i = 0; i < ref_list.index; i++) {
-			struct ref_item *item = &ref_list.list[i];
+		for (i = 0; i < array.nr; i++) {
+			struct ref_array_item *item = array.items[i];
 			struct commit *commit = item->commit;
 			int is_merged = !!(commit->object.flags & UNINTERESTING);
-			item->ignore = is_merged != (merge_filter == SHOW_MERGED);
+			item->ignore = is_merged != (filter->merge == REF_FILTER_MERGED_INCLUDE);
 		}
 
-		for (i = 0; i < ref_list.index; i++) {
-			struct ref_item *item = &ref_list.list[i];
+		for (i = 0; i < array.nr; i++) {
+			struct ref_array_item *item = array.items[i];
 			clear_commit_marks(item->commit, ALL_REV_FLAGS);
 		}
-		clear_commit_marks(filter, ALL_REV_FLAGS);
+		clear_commit_marks(filter->merge_commit, ALL_REV_FLAGS);
 	}
-	if (verbose)
-		maxwidth = calc_maxwidth(&ref_list, strlen(remote_prefix));
+	if (filter->verbose)
+		maxwidth = calc_maxwidth(&array, strlen(remote_prefix));
 
-	index = ref_list.index;
+	index = array.nr;
 
 	/* Print detached heads before sorting and printing the rest */
-	if (detached) {
-		print_ref_item(&ref_list.list[index - 1], maxwidth, verbose, abbrev,
-			       detached, remote_prefix);
+	if (filter->detached) {
+		print_ref_item(array.items[index - 1], maxwidth, filter, remote_prefix);
 		index -= 1;
 	}
 
-	qsort(ref_list.list, index, sizeof(struct ref_item), ref_cmp);
+	qsort(array.items, index, sizeof(struct ref_array_item *), ref_cmp);
 
 	for (i = 0; i < index; i++)
-		print_ref_item(&ref_list.list[i], maxwidth, verbose,
-			       abbrev, detached, remote_prefix);
+		print_ref_item(array.items[i], maxwidth, filter, remote_prefix);
 
-	free_ref_list(&ref_list);
+	ref_array_clear(&array);
 }
 
 static void rename_branch(const char *oldname, const char *newname, int force)
@@ -740,24 +699,6 @@ static void rename_branch(const char *oldname, const char *newname, int force)
 	strbuf_release(&newsection);
 }
 
-/*
- * This function is duplicated in ref-filter. It will eventually be removed
- * when we port branch.c to use ref-filter APIs.
- */
-static int opt_parse_merge_filter(const struct option *opt, const char *arg, int unset)
-{
-	merge_filter = ((opt->long_name[0] == 'n')
-			? SHOW_NOT_MERGED
-			: SHOW_MERGED);
-	if (unset)
-		merge_filter = SHOW_NOT_MERGED; /* b/c for --no-merged */
-	if (!arg)
-		arg = "HEAD";
-	if (get_sha1(arg, merge_filter_ref))
-		die(_("malformed object name %s"), arg);
-	return 0;
-}
-
 static const char edit_description[] = "BRANCH_DESCRIPTION";
 
 static int edit_branch_description(const char *branch_name)
@@ -797,17 +738,15 @@ static int edit_branch_description(const char *branch_name)
 int cmd_branch(int argc, const char **argv, const char *prefix)
 {
 	int delete = 0, rename = 0, force = 0, list = 0;
-	int verbose = 0, abbrev = -1, detached = 0;
 	int reflog = 0, edit_description = 0;
 	int quiet = 0, unset_upstream = 0;
 	const char *new_upstream = NULL;
 	enum branch_track track;
-	int kinds = REF_LOCAL_BRANCH;
-	struct commit_list *with_commit = NULL;
+	struct ref_filter filter;
 
 	struct option options[] = {
 		OPT_GROUP(N_("Generic options")),
-		OPT__VERBOSE(&verbose,
+		OPT__VERBOSE(&filter.verbose,
 			N_("show hash and subject, give twice for upstream branch")),
 		OPT__QUIET(&quiet, N_("suppress informational messages")),
 		OPT_SET_INT('t', "track",  &track, N_("set up tracking mode (see git-pull(1))"),
@@ -817,14 +756,14 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		OPT_STRING('u', "set-upstream-to", &new_upstream, "upstream", "change the upstream info"),
 		OPT_BOOL(0, "unset-upstream", &unset_upstream, "Unset the upstream info"),
 		OPT__COLOR(&branch_use_color, N_("use colored output")),
-		OPT_SET_INT('r', "remotes",     &kinds, N_("act on remote-tracking branches"),
+		OPT_SET_INT('r', "remotes",     &filter.branch_kind, N_("act on remote-tracking branches"),
 			REF_REMOTE_BRANCH),
-		OPT_CONTAINS(&with_commit, N_("print only branches that contain the commit")),
-		OPT_WITH(&with_commit, N_("print only branches that contain the commit")),
-		OPT__ABBREV(&abbrev),
+		OPT_CONTAINS(&filter.with_commit, N_("print only branches that contain the commit")),
+		OPT_WITH(&filter.with_commit, N_("print only branches that contain the commit")),
+		OPT__ABBREV(&filter.abbrev),
 
 		OPT_GROUP(N_("Specific git-branch actions:")),
-		OPT_SET_INT('a', "all", &kinds, N_("list both remote-tracking and local branches"),
+		OPT_SET_INT('a', "all", &filter.branch_kind, N_("list both remote-tracking and local branches"),
 			REF_REMOTE_BRANCH | REF_LOCAL_BRANCH),
 		OPT_BIT('d', "delete", &delete, N_("delete fully merged branch"), 1),
 		OPT_BIT('D', NULL, &delete, N_("delete branch (even if not merged)"), 2),
@@ -835,22 +774,16 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "edit-description", &edit_description,
 			 N_("edit the description for the branch")),
 		OPT__FORCE(&force, N_("force creation, move/rename, deletion")),
-		{
-			OPTION_CALLBACK, 0, "no-merged", &merge_filter_ref,
-			N_("commit"), N_("print only not merged branches"),
-			PARSE_OPT_LASTARG_DEFAULT | PARSE_OPT_NONEG,
-			opt_parse_merge_filter, (intptr_t) "HEAD",
-		},
-		{
-			OPTION_CALLBACK, 0, "merged", &merge_filter_ref,
-			N_("commit"), N_("print only merged branches"),
-			PARSE_OPT_LASTARG_DEFAULT | PARSE_OPT_NONEG,
-			opt_parse_merge_filter, (intptr_t) "HEAD",
-		},
+		OPT_MERGED(&filter, N_("print only branches that are merged")),
+		OPT_NO_MERGED(&filter, N_("print only branches that are not merged")),
 		OPT_COLUMN(0, "column", &colopts, N_("list branches in columns")),
 		OPT_END(),
 	};
 
+	memset(&filter, 0, sizeof(filter));
+	filter.branch_kind = REF_LOCAL_BRANCH;
+	filter.abbrev = -1;
+
 	if (argc == 2 && !strcmp(argv[1], "-h"))
 		usage_with_options(builtin_branch_usage, options);
 
@@ -862,11 +795,9 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	if (!head)
 		die(_("Failed to resolve HEAD as a valid ref."));
 	if (!strcmp(head, "HEAD"))
-		detached = 1;
+		filter.detached = 1;
 	else if (!skip_prefix(head, "refs/heads/", &head))
 		die(_("HEAD not found below refs/heads!"));
-	hashcpy(merge_filter_ref, head_sha1);
-
 
 	argc = parse_options(argc, argv, prefix, options, builtin_branch_usage,
 			     0);
@@ -874,17 +805,17 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	if (!delete && !rename && !edit_description && !new_upstream && !unset_upstream && argc == 0)
 		list = 1;
 
-	if (with_commit || merge_filter != NO_FILTER)
+	if (filter.with_commit || filter.merge != REF_FILTER_MERGED_NONE)
 		list = 1;
 
 	if (!!delete + !!rename + !!new_upstream +
 	    list + unset_upstream > 1)
 		usage_with_options(builtin_branch_usage, options);
 
-	if (abbrev == -1)
-		abbrev = DEFAULT_ABBREV;
+	if (filter.abbrev == -1)
+		filter.abbrev = DEFAULT_ABBREV;
 	finalize_colopts(&colopts, -1);
-	if (verbose) {
+	if (filter.verbose) {
 		if (explicitly_enable_column(colopts))
 			die(_("--column and --verbose are incompatible"));
 		colopts = 0;
@@ -898,12 +829,12 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	if (delete) {
 		if (!argc)
 			die(_("branch name required"));
-		return delete_branches(argc, argv, delete > 1, kinds, quiet);
+		return delete_branches(argc, argv, delete > 1, filter.branch_kind, quiet);
 	} else if (list) {
-		if (kinds & REF_LOCAL_BRANCH)
-			kinds |= REF_DETACHED_HEAD;
-		print_ref_list(kinds, detached, verbose, abbrev,
-					 with_commit, argv);
+		if (filter.branch_kind & REF_LOCAL_BRANCH)
+			filter.branch_kind |= REF_DETACHED_HEAD;
+		filter.name_patterns = argv;
+		print_ref_list(&filter);
 		print_columns(&output, colopts, NULL);
 		string_list_clear(&output, 0);
 		return 0;
@@ -913,7 +844,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		struct strbuf branch_ref = STRBUF_INIT;
 
 		if (!argc) {
-			if (detached)
+			if (filter.detached)
 				die(_("Cannot give description to detached HEAD"));
 			branch_name = head;
 		} else if (argc == 1)
@@ -1001,7 +932,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		if (!branch)
 			die(_("no such branch '%s'"), argv[0]);
 
-		if (kinds != REF_LOCAL_BRANCH)
+		if (filter.branch_kind != REF_LOCAL_BRANCH)
 			die(_("-a and -r options to 'git branch' do not make sense with a branch name"));
 
 		if (track == BRANCH_TRACK_OVERRIDE)
diff --git a/ref-filter.h b/ref-filter.h
index 7d1871d..3458595 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -5,6 +5,7 @@
 #include "refs.h"
 #include "commit.h"
 #include "parse-options.h"
+#include "revision.h"
 
 /* Quoting styles */
 #define QUOTE_NONE 0
@@ -48,6 +49,7 @@ struct ref_sorting {
 struct ref_array_item {
 	unsigned char objectname[20];
 	int flag, kind;
+	int ignore : 1;
 	const char *symref;
 	struct commit *commit;
 	struct atom_value *value;
@@ -57,6 +59,7 @@ struct ref_array_item {
 struct ref_array {
 	int nr, alloc;
 	struct ref_array_item **items;
+	struct rev_info *revs;
 };
 
 struct ref_filter {
@@ -72,8 +75,10 @@ struct ref_filter {
 	struct commit *merge_commit;
 
 	unsigned int with_commit_tag_algo : 1,
-		match_as_path : 1;
+		match_as_path : 1,
+		detached : 1;
 	unsigned int lines, branch_kind;
+	int abbrev, verbose;
 };
 
 struct ref_filter_cbdata {
-- 
2.4.6

[RFC/PATCH 10/11] branch.c: use 'ref-filter' APIs

[Karthik Nayak]

Make 'branch.c' use 'ref-filter' APIs for iterating through refs
sorting. This removes most of the code used in 'branch.c' replacing it
with calls to the 'ref-filter' library.

Make 'tag.c' use the 'filter_refs()' function provided by 'ref-filter'
to filter out tags based on the options set.

We provide a sorting option provided for 'branch.c' by using the sorting
options provided by 'ref-filter'.

Modify documentation and add tests 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 |   9 +-
 builtin/branch.c             | 200 ++++++++-----------------------------------
 ref-filter.c                 |   8 +-
 ref-filter.h                 |   1 -
 t/t1430-bad-ref-name.sh      |   2 +-
 t/t3203-branch-output.sh     |  11 +++
 6 files changed, 60 insertions(+), 171 deletions(-)

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index a67138a..897cd81 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -11,7 +11,7 @@ SYNOPSIS
 'git branch' [--color[=<when>] | --no-color] [-r | -a]
 	[--list] [-v [--abbrev=<length> | --no-abbrev]]
 	[--column[=<options>] | --no-column]
-	[(--merged | --no-merged | --contains) [<commit>]] [<pattern>...]
+	[(--merged | --no-merged | --contains) [<commit>]] [--sort=<key>] [<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>]
@@ -229,6 +229,13 @@ start-point is either a local or remote-tracking branch.
 	The new name for an existing branch. The same restrictions as for
 	<branchname> apply.
 
+--sort=<key>::
+	Sort based on the key given. Prefix `-` to sort in descending
+	order of the value. You may use the --sort=<key> option
+	multiple times, in which case the last key becomes the primary
+	key. The keys supported are the same as those in `git
+	for-each-ref`. Sort order defaults to sorting based on branch
+	type.
 
 Examples
 --------
diff --git a/builtin/branch.c b/builtin/branch.c
index df74527..75d8bfd 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -270,119 +270,6 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 	return(ret);
 }
 
-static char *resolve_symref(const char *src, const char *prefix)
-{
-	unsigned char sha1[20];
-	int flag;
-	const char *dst;
-
-	dst = resolve_ref_unsafe(src, 0, sha1, &flag);
-	if (!(dst && (flag & REF_ISSYMREF)))
-		return NULL;
-	if (prefix)
-		skip_prefix(dst, prefix, &dst);
-	return xstrdup(dst);
-}
-
-static int match_patterns(const char **pattern, const char *refname)
-{
-	if (!*pattern)
-		return 1; /* no pattern always matches */
-	while (*pattern) {
-		if (!wildmatch(*pattern, refname, 0, NULL))
-			return 1;
-		pattern++;
-	}
-	return 0;
-}
-
-static void ref_array_append(struct ref_array *array, const char *refname)
-{
-	size_t len = strlen(refname);
-	struct ref_array_item *ref = xcalloc(1, sizeof(struct ref_array_item) + len + 1);
-	memcpy(ref->refname, refname, len);
-	ref->refname[len] = '\0';
-	REALLOC_ARRAY(array->items, array->nr + 1);
-	array->items[array->nr++] = ref;
-}
-
-static int append_ref(const char *refname, const struct object_id *oid, int flags, void *cb_data)
-{
-	struct ref_filter_cbdata *cb = (struct ref_filter_cbdata *)(cb_data);
-	struct ref_filter *filter = cb->filter;
-	struct ref_array *array = cb->array;
-	struct ref_array_item *item;
-	struct commit *commit;
-	int kind, i;
-	const char *prefix, *orig_refname = refname;
-
-	static struct {
-		int kind;
-		const char *prefix;
-	} ref_kind[] = {
-		{ REF_LOCAL_BRANCH, "refs/heads/" },
-		{ REF_REMOTE_BRANCH, "refs/remotes/" },
-	};
-
-	/* Detect kind */
-	for (i = 0; i < ARRAY_SIZE(ref_kind); i++) {
-		prefix = ref_kind[i].prefix;
-		if (skip_prefix(refname, prefix, &refname)) {
-			kind = ref_kind[i].kind;
-			break;
-		}
-	}
-	if (ARRAY_SIZE(ref_kind) <= i) {
-		if (!strcmp(refname, "HEAD"))
-			kind = REF_DETACHED_HEAD;
-		else
-			return 0;
-	}
-
-	/* Don't add types the caller doesn't want */
-	if ((kind & filter->branch_kind) == 0)
-		return 0;
-
-	if (!match_patterns(filter->name_patterns, refname))
-		return 0;
-
-	commit = NULL;
-	if (filter->verbose || filter->with_commit || filter->merge != REF_FILTER_MERGED_NONE) {
-		commit = lookup_commit_reference_gently(oid->hash, 1);
-		if (!commit)
-			return 0;
-
-		/* Filter with with_commit if specified */
-		if (!is_descendant_of(commit, filter->with_commit))
-			return 0;
-
-		if (filter->merge != REF_FILTER_MERGED_NONE)
-			add_pending_object(array->revs,
-					   (struct object *)commit, refname);
-	}
-
-	ref_array_append(array, refname);
-	item = array->items[array->nr - 1];
-
-	/* Record the new item */
-	item->kind = kind;
-	item->commit = commit;
-	item->symref = resolve_symref(orig_refname, prefix);
-	item->ignore = 0;
-
-	return 0;
-}
-
-static int ref_cmp(const void *r1, const void *r2)
-{
-	struct ref_array_item *c1 = *((struct ref_array_item **)r1);
-	struct ref_array_item *c2 = *((struct ref_array_item **)r2);
-
-	if (c1->kind != c2->kind)
-		return c1->kind - c2->kind;
-	return strcmp(c1->refname, c2->refname);
-}
-
 static void fill_tracking_info(struct strbuf *stat, const char *branch_name,
 		int show_upstream_ref)
 {
@@ -446,7 +333,7 @@ static void fill_tracking_info(struct strbuf *stat, const char *branch_name,
 }
 
 static void add_verbose_info(struct strbuf *out, struct ref_array_item *item,
-			     struct ref_filter *filter)
+			     struct ref_filter *filter, const char *refname)
 {
 	struct strbuf subject = STRBUF_INIT, stat = STRBUF_INIT;
 	const char *sub = _(" **** invalid ref ****");
@@ -458,7 +345,7 @@ static void add_verbose_info(struct strbuf *out, struct ref_array_item *item,
 	}
 
 	if (item->kind == REF_LOCAL_BRANCH)
-		fill_tracking_info(&stat, item->refname, filter->verbose > 1);
+		fill_tracking_info(&stat, refname, filter->verbose > 1);
 
 	strbuf_addf(out, " %s %s%s",
 		find_unique_abbrev(item->commit->object.sha1, filter->abbrev),
@@ -507,18 +394,17 @@ static void print_ref_item(struct ref_array_item *item, int maxwidth,
 	const char *prefix = "";
 	const char *desc = item->refname;
 
-	if (item->ignore)
-		return;
-
 	switch (item->kind) {
 	case REF_LOCAL_BRANCH:
-		if (!filter->detached && !strcmp(item->refname, head)) {
+		skip_prefix(desc, "refs/heads/", &desc);
+		if (!filter->detached && !strcmp(desc, head)) {
 			color = BRANCH_COLOR_CURRENT;
 			c = '*';
 		} else
 			color = BRANCH_COLOR_LOCAL;
 		break;
 	case REF_REMOTE_BRANCH:
+		skip_prefix(desc, "refs/remotes/", &desc);
 		color = BRANCH_COLOR_REMOTE;
 		prefix = remote_prefix;
 		break;
@@ -542,11 +428,13 @@ static void print_ref_item(struct ref_array_item *item, int maxwidth,
 		strbuf_addf(&out, "%c %s%s%s", c, branch_get_color(color),
 			    name.buf, branch_get_color(BRANCH_COLOR_RESET));
 
-	if (item->symref)
-		strbuf_addf(&out, " -> %s", item->symref);
+	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);
+		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);
@@ -562,10 +450,13 @@ static int calc_maxwidth(struct ref_array *refs, int remote_bonus)
 	int i, max = 0;
 	for (i = 0; i < refs->nr; i++) {
 		struct ref_array_item *it = refs->items[i];
-		int w = utf8_strwidth(it->refname);
+		const char *desc = it->refname;
+		int w;
+
+		skip_prefix(it->refname, "refs/heads/", &desc);
+		skip_prefix(it->refname, "refs/remotes/", &desc);
+		w = utf8_strwidth(desc);
 
-		if (it->ignore)
-			continue;
 		if (it->kind == REF_REMOTE_BRANCH)
 			w += remote_bonus;
 		if (w > max)
@@ -574,14 +465,14 @@ static int calc_maxwidth(struct ref_array *refs, int remote_bonus)
 	return max;
 }
 
-static void print_ref_list(struct ref_filter *filter)
+static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sorting)
 {
 	int i, index;
 	struct ref_array array;
-	struct ref_filter_cbdata data;
 	int maxwidth = 0;
 	const char *remote_prefix = "";
-	struct rev_info revs;
+	struct ref_sorting def_sorting;
+	const char *sort_type = "type";
 
 	/*
 	 * If we are listing more than just remote branches,
@@ -592,41 +483,10 @@ static void print_ref_list(struct ref_filter *filter)
 		remote_prefix = "remotes/";
 
 	memset(&array, 0, sizeof(array));
-	if (filter->merge != REF_FILTER_MERGED_NONE)
-		init_revisions(&revs, NULL);
 
-	data.array = &array;
-	data.filter = filter;
-	array.revs = &revs;
+	verify_ref_format("%(refname)%(symref)");
+	filter_refs(&array, filter, FILTER_REFS_BRANCHES);
 
-	for_each_rawref(append_ref, &data);
-	if (filter->detached)
-		head_ref(append_ref, &data);
-	/*
-	 * The following implementation is currently duplicated in ref-filter. It
-	 * will eventually be removed when we port branch.c to use ref-filter APIs.
-	 */
-	if (filter->merge != REF_FILTER_MERGED_NONE) {
-		filter->merge_commit->object.flags |= UNINTERESTING;
-		add_pending_object(&revs, &filter->merge_commit->object, "");
-		revs.limited = 1;
-
-		if (prepare_revision_walk(&revs))
-			die(_("revision walk setup failed"));
-
-		for (i = 0; i < array.nr; i++) {
-			struct ref_array_item *item = array.items[i];
-			struct commit *commit = item->commit;
-			int is_merged = !!(commit->object.flags & UNINTERESTING);
-			item->ignore = is_merged != (filter->merge == REF_FILTER_MERGED_INCLUDE);
-		}
-
-		for (i = 0; i < array.nr; i++) {
-			struct ref_array_item *item = array.items[i];
-			clear_commit_marks(item->commit, ALL_REV_FLAGS);
-		}
-		clear_commit_marks(filter->merge_commit, ALL_REV_FLAGS);
-	}
 	if (filter->verbose)
 		maxwidth = calc_maxwidth(&array, strlen(remote_prefix));
 
@@ -635,14 +495,21 @@ static void print_ref_list(struct ref_filter *filter)
 	/* Print detached heads before sorting and printing the rest */
 	if (filter->detached) {
 		print_ref_item(array.items[index - 1], maxwidth, filter, remote_prefix);
-		index -= 1;
+		array.nr--;
 	}
 
-	qsort(array.items, index, sizeof(struct ref_array_item *), ref_cmp);
+	if (!sorting) {
+		def_sorting.next = NULL;
+		def_sorting.atom = parse_ref_filter_atom(sort_type,
+							 sort_type + strlen(sort_type));
+		sorting = &def_sorting;
+	}
+	ref_array_sort(sorting, &array);
 
-	for (i = 0; i < index; i++)
+	for (i = 0; i < array.nr; i++)
 		print_ref_item(array.items[i], maxwidth, filter, remote_prefix);
 
+	array.nr = index;
 	ref_array_clear(&array);
 }
 
@@ -743,6 +610,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	const char *new_upstream = NULL;
 	enum branch_track track;
 	struct ref_filter filter;
+	static struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;
 
 	struct option options[] = {
 		OPT_GROUP(N_("Generic options")),
@@ -777,6 +645,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		OPT_MERGED(&filter, N_("print only branches that are merged")),
 		OPT_NO_MERGED(&filter, N_("print only branches that are not merged")),
 		OPT_COLUMN(0, "column", &colopts, N_("list branches in columns")),
+		OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"),
+			     N_("field name to sort on"), &parse_opt_ref_sorting),
 		OPT_END(),
 	};
 
@@ -834,7 +704,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		if (filter.branch_kind & REF_LOCAL_BRANCH)
 			filter.branch_kind |= REF_DETACHED_HEAD;
 		filter.name_patterns = argv;
-		print_ref_list(&filter);
+		print_ref_list(&filter, sorting);
 		print_columns(&output, colopts, NULL);
 		string_list_clear(&output, 0);
 		return 0;
diff --git a/ref-filter.c b/ref-filter.c
index ff5a16b..52224f0 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1153,7 +1153,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
 	 * obtain the commit using the 'oid' available and discard all
 	 * non-commits early. The actual filtering is done later.
 	 */
-	if (filter->merge_commit || filter->with_commit) {
+	if (filter->merge_commit || filter->with_commit || filter->verbose) {
 		commit = lookup_commit_reference_gently(oid->hash, 1);
 		if (!commit)
 			return 0;
@@ -1261,9 +1261,11 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int
 		ret = for_each_ref(ref_filter_handler, &ref_cbdata);
 	else if (type & FILTER_REFS_TAGS)
 		ret = for_each_tag_ref_fullpath(ref_filter_handler, &ref_cbdata);
-	else if (type & FILTER_REFS_BRANCHES)
+	else if (type & FILTER_REFS_BRANCHES) {
 		ret = for_each_rawref(ref_filter_handler, &ref_cbdata);
-	else if (type)
+		if (filter->detached)
+			head_ref(ref_filter_handler, &ref_cbdata);
+	} else if (type)
 		die("filter_refs: invalid type");
 
 	/*  Filters that need revision walking */
diff --git a/ref-filter.h b/ref-filter.h
index 3458595..311543a 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -49,7 +49,6 @@ struct ref_sorting {
 struct ref_array_item {
 	unsigned char objectname[20];
 	int flag, kind;
-	int ignore : 1;
 	const char *symref;
 	struct commit *commit;
 	struct atom_value *value;
diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh
index 16d0b8b..b45f5e2 100755
--- a/t/t1430-bad-ref-name.sh
+++ b/t/t1430-bad-ref-name.sh
@@ -38,7 +38,7 @@ test_expect_success 'fast-import: fail on invalid branch name "bad[branch]name"'
 	test_must_fail git fast-import <input
 '
 
-test_expect_success 'git branch shows badly named ref' '
+test_expect_failure 'git branch does not show badly named ref' '
 	cp .git/refs/heads/master .git/refs/heads/broken...ref &&
 	test_when_finished "rm -f .git/refs/heads/broken...ref" &&
 	git branch >output &&
diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
index f51d0f3..38c68bd 100755
--- a/t/t3203-branch-output.sh
+++ b/t/t3203-branch-output.sh
@@ -143,4 +143,15 @@ EOF
 	test_i18ncmp expect actual
 '
 
+test_expect_success 'git branch `--sort` option' '
+	cat >expect <<EOF &&
+* (HEAD detached from fromtag)
+  branch-two
+  branch-one
+  master
+EOF
+	git branch --sort=objectsize >actual &&
+	test_i18ncmp expect actual
+'
+
 test_done
-- 
2.4.6

[RFC/PATCH 11/11] branch: add '--points-at' option

[Karthik Nayak]

Add the '--points-at' option provided by 'ref-filter'. The option lets
the user to list only branches which points at the given object.

Add documentation and tests 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 | 6 +++++-
 builtin/branch.c             | 7 ++++++-
 t/t3203-branch-output.sh     | 9 +++++++++
 3 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index 897cd81..efa23a5 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -11,7 +11,8 @@ SYNOPSIS
 'git branch' [--color[=<when>] | --no-color] [-r | -a]
 	[--list] [-v [--abbrev=<length> | --no-abbrev]]
 	[--column[=<options>] | --no-column]
-	[(--merged | --no-merged | --contains) [<commit>]] [--sort=<key>] [<pattern>...]
+	[(--merged | --no-merged | --contains) [<commit>]] [--sort=<key>]
+	[--points-at <object>] [<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>]
@@ -237,6 +238,9 @@ start-point is either a local or remote-tracking branch.
 	for-each-ref`. Sort order defaults to sorting based on branch
 	type.
 
+--points-at <object>::
+	Only list tags of the given object.
+
 Examples
 --------
 
diff --git a/builtin/branch.c b/builtin/branch.c
index 75d8bfd..d25f43b 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -26,6 +26,7 @@ static const char * const builtin_branch_usage[] = {
 	N_("git branch [<options>] [-l] [-f] <branch-name> [<start-point>]"),
 	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]"),
 	NULL
 };
 
@@ -647,6 +648,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		OPT_COLUMN(0, "column", &colopts, N_("list branches in columns")),
 		OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"),
 			     N_("field name to sort on"), &parse_opt_ref_sorting),
+		{
+			OPTION_CALLBACK, 0, "points-at", &filter.points_at, N_("object"),
+			N_("print only tags of the object"), 0, parse_opt_object_name
+		},
 		OPT_END(),
 	};
 
@@ -675,7 +680,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	if (!delete && !rename && !edit_description && !new_upstream && !unset_upstream && argc == 0)
 		list = 1;
 
-	if (filter.with_commit || filter.merge != REF_FILTER_MERGED_NONE)
+	if (filter.with_commit || filter.merge != REF_FILTER_MERGED_NONE || filter.points_at.nr)
 		list = 1;
 
 	if (!!delete + !!rename + !!new_upstream +
diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
index 38c68bd..1deb7cb 100755
--- a/t/t3203-branch-output.sh
+++ b/t/t3203-branch-output.sh
@@ -154,4 +154,13 @@ EOF
 	test_i18ncmp expect actual
 '
 
+test_expect_success 'git branch --points-at option' '
+	cat >expect <<EOF &&
+  master
+  branch-one
+EOF
+	git branch --points-at=branch-one >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.4.6

Re: [RFC/PATCH 11/11] branch: add '--points-at' option

[Jacob Keller]

On Tue, Jul 28, 2015 at 12:11 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> Add the '--points-at' option provided by 'ref-filter'. The option lets
> the user to list only branches which points at the given object.
>
> Add documentation and tests 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 | 6 +++++-
>  builtin/branch.c             | 7 ++++++-
>  t/t3203-branch-output.sh     | 9 +++++++++
>  3 files changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
> index 897cd81..efa23a5 100644
> --- a/Documentation/git-branch.txt
> +++ b/Documentation/git-branch.txt
> @@ -11,7 +11,8 @@ SYNOPSIS
>  'git branch' [--color[=<when>] | --no-color] [-r | -a]
>         [--list] [-v [--abbrev=<length> | --no-abbrev]]
>         [--column[=<options>] | --no-column]
> -       [(--merged | --no-merged | --contains) [<commit>]] [--sort=<key>] [<pattern>...]
> +       [(--merged | --no-merged | --contains) [<commit>]] [--sort=<key>]
> +       [--points-at <object>] [<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>]
> @@ -237,6 +238,9 @@ start-point is either a local or remote-tracking branch.
>         for-each-ref`. Sort order defaults to sorting based on branch
>         type.
>
> +--points-at <object>::
> +       Only list tags of the given object.
> +

s/tags/branches/ ?? Since this is for the branch version, I think this
is just a copy-paste oversight.

>  Examples
>  --------
>
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 75d8bfd..d25f43b 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -26,6 +26,7 @@ static const char * const builtin_branch_usage[] = {
>         N_("git branch [<options>] [-l] [-f] <branch-name> [<start-point>]"),
>         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]"),
>         NULL
>  };
>
> @@ -647,6 +648,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>                 OPT_COLUMN(0, "column", &colopts, N_("list branches in columns")),
>                 OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"),
>                              N_("field name to sort on"), &parse_opt_ref_sorting),
> +               {
> +                       OPTION_CALLBACK, 0, "points-at", &filter.points_at, N_("object"),
> +                       N_("print only tags of the object"), 0, parse_opt_object_name
> +               },

Same as above. s/tags/branches/

>                 OPT_END(),
>         };
>
> @@ -675,7 +680,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>         if (!delete && !rename && !edit_description && !new_upstream && !unset_upstream && argc == 0)
>                 list = 1;
>
> -       if (filter.with_commit || filter.merge != REF_FILTER_MERGED_NONE)
> +       if (filter.with_commit || filter.merge != REF_FILTER_MERGED_NONE || filter.points_at.nr)
>                 list = 1;
>
>         if (!!delete + !!rename + !!new_upstream +
> diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
> index 38c68bd..1deb7cb 100755
> --- a/t/t3203-branch-output.sh
> +++ b/t/t3203-branch-output.sh
> @@ -154,4 +154,13 @@ EOF
>         test_i18ncmp expect actual
>  '
>
> +test_expect_success 'git branch --points-at option' '
> +       cat >expect <<EOF &&
> +  master
> +  branch-one
> +EOF
> +       git branch --points-at=branch-one >actual &&
> +       test_cmp expect actual
> +'
> +
>  test_done
> --
> 2.4.6
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH 04/11] ref-filter: add 'ifexists' atom

[Jacob Keller]

On Mon, Jul 27, 2015 at 11:56 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> The 'ifexists' atom allows us to print a required format if the
> preceeding atom has a value. If the preceeding atom has no value then

Don't you mean "following atom" here? since you do document it as "the
next atom" below you should fix the commit message as well to match.
In any respect, this is a useful formatting atom :)

%(ifexists:[%s])%(atom) where the contents of atom will be placed into %s?

> the format given is not printed. e.g. to print "[<refname>]" we can
> now use the format "%(ifexists:[%s])%(refname)".
>
> Add documentation and test 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 |  8 ++++++++
>  ref-filter.c                       | 37 ++++++++++++++++++++++++++++++++++---
>  ref-filter.h                       |  5 +++--
>  t/t6302-for-each-ref-filter.sh     | 21 +++++++++++++++++++++
>  4 files changed, 66 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index 9dc02aa..4424020 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -138,6 +138,14 @@ colornext::
>         `<:colorname>`.  Not compatible with `padright` and resets any
>         previous `color`, if set.
>
> +ifexists::
> +       Print required string only if the next atom specified in the
> +       '--format' option exists.
> +       e.g. --format="%(ifexists:[%s])%(symref)" prints the symref
> +       like "[<symref>]" only if the ref has a symref.  This was
> +       incorporated to simulate the output of 'git branch -vv', where
> +       we need to display the upstream branch in square brackets.
> +

I suggest documenting that the atom will be placed into the contents
of ifexists via the %s indicator, as you do show an example but don't
explicitely say %s is the formatting character.

>  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 3f40144..ff5a16b 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -58,6 +58,7 @@ static struct {
>         { "color" },
>         { "padright" },
>         { "colornext" },
> +       { "ifexists" },
>  };
>
>  /*
> @@ -722,6 +723,13 @@ static void populate_value(struct ref_array_item *ref)
>                         v->modifier_atom = 1;
>                         v->color_next = 1;
>                         continue;
> +               } else if (starts_with(name, "ifexists:")) {
> +                       skip_prefix(name, "ifexists:", &v->s);
> +                       if (!*v->s)
> +                               die(_("no string given with 'ifexists:'"));
> +                       v->modifier_atom = 1;
> +                       v->ifexists = 1;
> +                       continue;
>                 } else
>                         continue;
>
> @@ -1315,11 +1323,32 @@ static void apply_formatting_state(struct ref_formatting_state *state,
>  {
>         if (state->color_next && state->pad_to_right)
>                 die(_("cannot use `colornext` and `padright` together"));
> -       if (state->color_next) {
> +       if (state->pad_to_right && state->ifexists)
> +               die(_("cannot use 'align' and 'ifexists' together"));
> +       if (state->color_next && !state->ifexists) {
>                 strbuf_addf(value, "%s%s%s", state->color_next, v->s, GIT_COLOR_RESET);
>                 return;
> -       }
> -       else if (state->pad_to_right) {
> +       } else if (state->ifexists) {
> +               const char *sp = state->ifexists;
> +
> +               while (*sp) {
> +                       if (*sp != '%') {
> +                               strbuf_addch(value, *sp++);
> +                               continue;
> +                       } else if (sp[1] == '%') {
> +                               strbuf_addch(value, *sp++);
> +                               continue;
> +                       } else if (sp[1] == 's') {
> +                               if (state->color_next)
> +                                       strbuf_addf(value, "%s%s%s", state->color_next, v->s, GIT_COLOR_RESET);
> +                               else
> +                                       strbuf_addstr(value, v->s);
> +                               sp += 2;
> +                       }
> +               }
> +
> +               return;
> +       } else if (state->pad_to_right) {
>                 if (!is_utf8(v->s))
>                         strbuf_addf(value, "%-*s", state->pad_to_right, v->s);
>                 else {
> @@ -1413,6 +1442,8 @@ static void store_formatting_state(struct ref_formatting_state *state,
>                 state->color_next = atomv->s;
>         if (atomv->pad_to_right)
>                 state->pad_to_right = atomv->ul;
> +       if (atomv->ifexists)
> +               state->ifexists = atomv->s;
>  }
>
>  static void reset_formatting_state(struct ref_formatting_state *state)
> diff --git a/ref-filter.h b/ref-filter.h
> index a021b04..7d1871d 100644
> --- a/ref-filter.h
> +++ b/ref-filter.h
> @@ -28,13 +28,14 @@ struct atom_value {
>         unsigned long ul; /* used for sorting when not FIELD_STR */
>         unsigned int modifier_atom : 1, /*  atoms which act as modifiers for the next atom */
>                 pad_to_right : 1,
> -               color_next : 1;
> +               color_next : 1,
> +               ifexists : 1;
>  };
>
>  struct ref_formatting_state {
>         int quote_style;
>         unsigned int pad_to_right;
> -       const char *color_next;
> +       const char *color_next, *ifexists;
>  };
>
>  struct ref_sorting {
> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
> index 6aad069..29ed97b 100755
> --- a/t/t6302-for-each-ref-filter.sh
> +++ b/t/t6302-for-each-ref-filter.sh
> @@ -149,4 +149,25 @@ test_expect_success 'check `colornext` format option' '
>         test_cmp expect actual
>  '
>
> +test_expect_success 'check `ifexists` format option' '
> +       cat >expect <<-\EOF &&
> +       [foo1.10]
> +       [foo1.3]
> +       [foo1.6]
> +       EOF
> +       git for-each-ref --format="%(ifexists:[%s])%(refname:short)" | grep "foo" >actual &&
> +       test_cmp expect actual
> +'
> +
> +cat >expect <<EOF &&
> +[$(get_color green)foo1.10$(get_color reset)]||foo1.10
> +[$(get_color green)foo1.3$(get_color reset)]||foo1.3
> +[$(get_color green)foo1.6$(get_color reset)]||foo1.6
> +EOF
> +
> +test_expect_success 'check `ifexists` with `colornext` format option' '
> +       git for-each-ref --format="%(ifexists:[%s])%(colornext:green)%(refname:short)||%(refname:short)" | grep "foo" >actual &&
> +       test_cmp expect actual
> +'
> +
>  test_done
> --
> 2.4.6
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH 10/11] branch.c: use 'ref-filter' APIs

[Jacob Keller]

On Tue, Jul 28, 2015 at 12:11 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> Make 'branch.c' use 'ref-filter' APIs for iterating through refs
> sorting. This removes most of the code used in 'branch.c' replacing it
> with calls to the 'ref-filter' library.
>
> Make 'tag.c' use the 'filter_refs()' function provided by 'ref-filter'
> to filter out tags based on the options set.
>

This patch doesn't do anything to tag.c, so you should update the
commit message to remove this or replace it with s/tag/branch if it
was intended to be about branch.c?

Regards,
Jake

Re: [RFC/PATCH 10/11] branch.c: use 'ref-filter' APIs

[Christian Couder]

On Tue, Jul 28, 2015 at 9:11 AM, Karthik Nayak <karthik.188@gmail.com> wrote:

> diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
> index a67138a..897cd81 100644
> --- a/Documentation/git-branch.txt
> +++ b/Documentation/git-branch.txt
> @@ -11,7 +11,7 @@ SYNOPSIS
>  'git branch' [--color[=<when>] | --no-color] [-r | -a]
>         [--list] [-v [--abbrev=<length> | --no-abbrev]]
>         [--column[=<options>] | --no-column]
> -       [(--merged | --no-merged | --contains) [<commit>]] [<pattern>...]
> +       [(--merged | --no-merged | --contains) [<commit>]] [--sort=<key>] [<pattern>...]

Maybe: [(--[no-]merged | --contains) [<commit>]]

Re: [RFC/PATCH 09/11] branch.c: use 'ref-filter' data structures

[Christian Couder]

On Tue, Jul 28, 2015 at 8:56 AM, Karthik Nayak <karthik.188@gmail.com> wrote:

> +static void ref_array_append(struct ref_array *array, const char *refname)
> +{
> +       size_t len = strlen(refname);
> +       struct ref_array_item *ref = xcalloc(1, sizeof(struct ref_array_item) + len + 1);
> +       memcpy(ref->refname, refname, len);
> +       ref->refname[len] = '\0';
> +       REALLOC_ARRAY(array->items, array->nr + 1);
> +       array->items[array->nr++] = ref;
> +}

This function belongs more to ref-filter.{c,h}...

[...]

> -       ALLOC_GROW(ref_list->list, ref_list->index + 1, ref_list->alloc);
> +       ref_array_append(array, refname);
> +       item = array->items[array->nr - 1];

...and the above is a bit ugly.

Re: [RFC/PATCH 09/11] branch.c: use 'ref-filter' data structures

[Christian Couder]

On Tue, Jul 28, 2015 at 8:56 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> diff --git a/ref-filter.h b/ref-filter.h
> index 7d1871d..3458595 100644
> --- a/ref-filter.h
> +++ b/ref-filter.h
> @@ -5,6 +5,7 @@
>  #include "refs.h"
>  #include "commit.h"
>  #include "parse-options.h"
> +#include "revision.h"
>
>  /* Quoting styles */
>  #define QUOTE_NONE 0
> @@ -48,6 +49,7 @@ struct ref_sorting {
>  struct ref_array_item {
>         unsigned char objectname[20];
>         int flag, kind;
> +       int ignore : 1;

Maybe you could add a comment to say that this will be removed in the
next patch.

>         const char *symref;
>         struct commit *commit;
>         struct atom_value *value;

Re: [RFC/PATCH 01/11] ref-filter: add "%(objectname:size=X)" option

[Matthieu Moy]

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

> +	if (skip_prefix(name, "objectname:size=", &p)) {
> +		unsigned int size = atoi(p);

You have the same problem as for tag.c: don't use atoi, and make
accurate error checking (absence of value, negative value, non-integer
value).

If you have other higher-priorities task, leave a temporary comment 
/* FIXME: ... */ or /* TODO: ... */ and make sure you have no such
comment when you drop the "RFC" in the subject of your emails.

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

Re: [RFC/PATCH 02/11] ref-filter: add 'colornext' atom

[Matthieu Moy]

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

> --- a/t/t6302-for-each-ref-filter.sh
> +++ b/t/t6302-for-each-ref-filter.sh
> @@ -133,4 +133,20 @@ test_expect_success 'reverse version sort' '
>  	test_cmp expect actual
>  '
>  
> +get_color ()
> +{
> +	git config --get-color no.such.slot "$1"
> +}
> +
> +cat >expect <<EOF &&
> +$(get_color green)foo1.10$(get_color reset)||
> +$(get_color green)foo1.3$(get_color reset)||
> +$(get_color green)foo1.6$(get_color reset)||
> +EOF
> +
> +test_expect_success 'check `colornext` format option' '
> +	git for-each-ref --format="%(colornext:green)%(refname:short)||" | grep "foo" >actual &&
> +	test_cmp expect actual
> +'

This is not a very good test: you're not checking that colornext applies
to the next and only this one. Similarly to what I suggested for
padright, I'd suggest

  --format="%(refname:short)%(colornext:green)|%(refname:short)|%(refname:short)|"

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

Re: [RFC/PATCH 04/11] ref-filter: add 'ifexists' atom

[Matthieu Moy]

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

> --- a/t/t6302-for-each-ref-filter.sh
> +++ b/t/t6302-for-each-ref-filter.sh
> @@ -149,4 +149,25 @@ test_expect_success 'check `colornext` format option' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'check `ifexists` format option' '
> +	cat >expect <<-\EOF &&
> +	[foo1.10]
> +	[foo1.3]
> +	[foo1.6]
> +	EOF
> +	git for-each-ref --format="%(ifexists:[%s])%(refname:short)" | grep "foo" >actual &&
> +	test_cmp expect actual
> +'

You're testing only the positive case of ifexists, right? You need a
test for the negative case (i.e. the attribute does not exist) too.
Ideally, check that the ifexist actually applies to the right attribute,
like

--format '%(ifexist:<%s>)%(attribute1) %(ifexist:[%s])%(attribute2)'

and manage to get an output like

<foo>
 [bar]
<foobar> [barfoo]

> +cat >expect <<EOF &&
> +[$(get_color green)foo1.10$(get_color reset)]||foo1.10
> +[$(get_color green)foo1.3$(get_color reset)]||foo1.3
> +[$(get_color green)foo1.6$(get_color reset)]||foo1.6
> +EOF
> +
> +test_expect_success 'check `ifexists` with `colornext` format option' '
> +	git for-each-ref --format="%(ifexists:[%s])%(colornext:green)%(refname:short)||%(refname:short)" | grep "foo" >actual &&
> +	test_cmp expect actual
> +'

You have a double || that is not useful. Not really harmful either, but
it may distract the reader. You may want to s/||/|/.

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

Re: [RFC/PATCH 02/11] ref-filter: add 'colornext' atom

[Christian Couder]

On Tue, Jul 28, 2015 at 8:56 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> @@ -712,6 +713,15 @@ static void populate_value(struct ref_array_item *ref)
>                         v->modifier_atom = 1;
>                         v->pad_to_right = 1;
>                         continue;
> +               } else if (starts_with(name, "colornext:")) {
> +                       char color[COLOR_MAXLEN] = "";
> +
> +                       if (color_parse(name + 10, color) < 0)
> +                               die(_("unable to parse format"));

Maybe use skip_prefix(), and die() with a more helpful message.

> +                       v->s = xstrdup(color);
> +                       v->modifier_atom = 1;
> +                       v->color_next = 1;
> +                       continue;
>                 } else
>                         continue;

Re: [RFC/PATCH 05/11] branch: fix width computation

[Matthieu Moy]

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

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

Why did send-email add this From: header? Strange, it has the same
content as your actual From: field.

> Remove unnecessary variables from ref_list and ref_item
> which were used for width computation. Make other changes
> accordingly. This patch is a precursor for porting branch.c
> to use ref-filter APIs.

You can explain better why this is needed. I guess something like "we're
making struct ref_item similar to ref-filter's ref_array_item", but the
reader shouldn't have to guess.

You should adujst the subject like BTW, I don't think you are "fixing"
anything.

On a side note: on the "tag" series, see how explaining better and
splitting patches led not only to a better history, but also to better
final code, and to finding a actual bugs (the %(color) thing and the
absence of reset on the state variable) even after sereval rounds of
review? I'm being picky and demanding, but don't see that as a complain
from me, just hints on getting even better ;-).

> @@ -386,15 +386,8 @@ static int append_ref(const char *refname, const struct object_id *oid, int flag
>  	newitem->name = xstrdup(refname);
>  	newitem->kind = kind;
>  	newitem->commit = commit;
> -	newitem->width = utf8_strwidth(refname);
>  	newitem->dest = resolve_symref(orig_refname, prefix);
>  	newitem->ignore = 0;
> -	/* adjust for "remotes/" */
> -	if (newitem->kind == REF_REMOTE_BRANCH &&
> -	    ref_list->kinds != REF_REMOTE_BRANCH)
> -		newitem->width += 8;
> -	if (newitem->width > ref_list->maxwidth)
> -		ref_list->maxwidth = newitem->width;
>  
>  	return 0;
>  }

OK, in the old code, the width computation is done when inserting the
ref into the array. In the new code, you build the array and then do the
width computation. You can explain this better in the commit message I
think (instead of "Make other changes accordingly" which doesn't bring
much).

> @@ -505,11 +498,12 @@ static void add_verbose_info(struct strbuf *out, struct ref_item *item,
>  }
>  
>  static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
> -			   int abbrev, int current, char *prefix)
> +			   int abbrev, int current, const char *remote_prefix)
>  {
>  	char c;
>  	int color;
>  	struct strbuf out = STRBUF_INIT, name = STRBUF_INIT;
> +	const char *prefix = "";
>  
>  	if (item->ignore)
>  		return;
> @@ -520,6 +514,7 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
>  		break;
>  	case REF_REMOTE_BRANCH:
>  		color = BRANCH_COLOR_REMOTE;
> +		prefix = remote_prefix;
>  		break;

Why do you need these two hunks? I did not investigate in details, but
it seems you're calling print_ref_item either with prefix="" or with
prefix=remote_prefix so it would seem that keeping "prefix" as argument
would work. I guess I missed something.

> -static int calc_maxwidth(struct ref_list *refs)
> +static int calc_maxwidth(struct ref_list *refs, int remote_bonus)
>  {
> -	int i, w = 0;
> +	int i, max = 0;
>  	for (i = 0; i < refs->index; i++) {
> +		struct ref_item *it = &refs->list[i];
> +		int w = utf8_strwidth(it->name);
> +
>  		if (refs->list[i].ignore)
>  			continue;
> -		if (refs->list[i].width > w)
> -			w = refs->list[i].width;
> +		if (it->kind == REF_REMOTE_BRANCH)
> +			w += remote_bonus;
> +		if (w > max)
> +			max = w;
>  	}
> -	return w;
> +	return max;
>  }

The old code was using 'w' for the max and no variable for the value at
the current iteration. You're using 'max' for the max and 'w' at the
current iteration. Using the same name 'w' for different things in the
pre- and post-image of the patch distracts the reader.

It may make sense to s/w/max/ in a separate preparatory patch. Or maybe
it's overkill.

> @@ -600,21 +600,18 @@ static char *get_head_description(void)
>  	return strbuf_detach(&desc, NULL);
>  }
>  
> -static void show_detached(struct ref_list *ref_list)
> +static void show_detached(struct ref_list *ref_list, int maxwidth)
>  {
>  	struct commit *head_commit = lookup_commit_reference_gently(head_sha1, 1);
>  
>  	if (head_commit && is_descendant_of(head_commit, ref_list->with_commit)) {
>  		struct ref_item item;
>  		item.name = get_head_description();
> -		item.width = utf8_strwidth(item.name);
>  		item.kind = REF_LOCAL_BRANCH;
>  		item.dest = NULL;
>  		item.commit = head_commit;
>  		item.ignore = 0;
> -		if (item.width > ref_list->maxwidth)
> -			ref_list->maxwidth = item.width;
> -		print_ref_item(&item, ref_list->maxwidth, ref_list->verbose, ref_list->abbrev, 1, "");
> +		print_ref_item(&item, maxwidth, ref_list->verbose, ref_list->abbrev, 1, "");
>  		free(item.name);
>  	}
>  }
...
> +	int maxwidth = 0;
...
> +	if (verbose)
> +		maxwidth = calc_maxwidth(&ref_list, strlen(remote_prefix));
>  
>  	qsort(ref_list.list, ref_list.index, sizeof(struct ref_item), ref_cmp);
>  
>  	detached = (detached && (kinds & REF_LOCAL_BRANCH));
>  	if (detached && match_patterns(pattern, "HEAD"))
> -		show_detached(&ref_list);
> +		show_detached(&ref_list, maxwidth);

This hunks could ideally go in a preparatory patch that would just move
the place where maxwidth is computed. This preparatory patch would just
say

	maxwidth = ref_list->maxwidth;

and then you would do the actual change to

	if (verbose)
		maxwidth = calc_maxwidth(...)

without the distracting changes in the function's argument list.

I won't insist on that, again it may be overkill. But reading the patch,
I found it both rather trivial and hard to read, so there's room for
improvement.

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

Re: [RFC/PATCH 06/11] branch: roll show_detached HEAD into regular ref_list

[Matthieu Moy]

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

> Remove show_detached() and make detached HEAD to be rolled into
> regular ref_list by adding REF_DETACHED_HEAD as a kind of branch and
> supporting the same in append_ref().

Again, this lacks the "why?" explanation.

> Bump get_head_description() to the top.

Here also: why? And this could easily go to a separate patch to let the
reviewer focus on actual changes.

> @@ -504,6 +540,7 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
>  	int color;
>  	struct strbuf out = STRBUF_INIT, name = STRBUF_INIT;
>  	const char *prefix = "";
> +	const char *desc = item->name;
>  
>  	if (item->ignore)
>  		return;
> @@ -516,6 +553,10 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
>  		color = BRANCH_COLOR_REMOTE;
>  		prefix = remote_prefix;
>  		break;
> +	case REF_DETACHED_HEAD:
> +		color = BRANCH_COLOR_CURRENT;
> +		desc = get_head_description();

I think you're leaking a string here: get_head_description() builds an
strbuf and returns the dynamically allocated string, which you never
free.

> -static void show_detached(struct ref_list *ref_list, int maxwidth)
> -{
> -	struct commit *head_commit = lookup_commit_reference_gently(head_sha1, 1);
> -
> -	if (head_commit && is_descendant_of(head_commit, ref_list->with_commit)) {

I'm not sure what this if was doing, and why you can get rid of it. My
understanding is that with_commit comes from --contains, and in the
previous code the filtering was done at display time (detached HEAD was
not shown if it was not contained in commits specified with --contains).

Eventually, you'll use ref-filter to do this filtering so you won't need
this check at display time.

But am I correct that for a few commits, you ignore --contains on
detached HEAD?

> @@ -678,15 +674,20 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru
>  	if (verbose)
>  		maxwidth = calc_maxwidth(&ref_list, strlen(remote_prefix));
>  
> -	qsort(ref_list.list, ref_list.index, sizeof(struct ref_item), ref_cmp);
> +	index = ref_list.index;
> +
> +	/* Print detached heads before sorting and printing the rest */

I think you mean head (no s) or HEAD. It's not Mercurial ;-).

> +	if (detached && (ref_list.list[index - 1].kind == REF_DETACHED_HEAD) &&
> +	    !strcmp(ref_list.list[index - 1].name, head)) {
> +		print_ref_item(&ref_list.list[index - 1], maxwidth, verbose, abbrev,
> +			       1, remote_prefix);
> +		index -= 1;
> +	}

This relies on the assumption that HEAD has to be the last in the array.
The assumption is correct since you call head_ref(append_ref, &cb) after
for_each_rawref(append_ref, &cb). I think this deserves a comment to
remind the reader that HEAD is always the last (here or at the call to
head_ref to make sure nobody try to change the order between head_ref
and for_each_rawref).

It may be more natural to do it the other way around: call head_ref
first and get HEAD first, as you are going to display it first (but in
any case, you'll have to call qsort on a subset of the array so it
doesn't change much).

> @@ -913,7 +914,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>  			die(_("branch name required"));
>  		return delete_branches(argc, argv, delete > 1, kinds, quiet);
>  	} else if (list) {
> -		int ret = print_ref_list(kinds, detached, verbose, abbrev,
> +		int ret;
> +		if (kinds & REF_LOCAL_BRANCH)
> +			kinds |= REF_DETACHED_HEAD;

Perhaps add

	/* git branch --local also shows HEAD when it is detached */

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

Re: [RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer

[Matthieu Moy]

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

> We check if given ref is the current branch in print_ref_list().  Move
> this check to print_ref_item() where it is checked right before
> printing.

This means that the '*' and the different color are coded in C, hence
it's not possible to mimick this using "git for-each-ref --format ...".

I do not consider this as blocking, but I think the ultimate goal should
be to allow this, so that all the goodies of "git branch" can be made
available to other ref-listing commands.

> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -534,9 +534,9 @@ static char *get_head_description(void)
>  }
>  
>  static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
> -			   int abbrev, int current, const char *remote_prefix)
> +			   int abbrev, int detached, const char *remote_prefix)
>  {
> -	char c;
> +	char c = ' ';
>  	int color;
>  	struct strbuf out = STRBUF_INIT, name = STRBUF_INIT;
>  	const char *prefix = "";
> @@ -547,7 +547,11 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
>  
>  	switch (item->kind) {
>  	case REF_LOCAL_BRANCH:
> -		color = BRANCH_COLOR_LOCAL;
> +		if (!detached && !strcmp(item->name, head)) {
> +			color = BRANCH_COLOR_CURRENT;
> +			c = '*';
> +		} else
> +			color = BRANCH_COLOR_LOCAL;
>  		break;
>  	case REF_REMOTE_BRANCH:
>  		color = BRANCH_COLOR_REMOTE;
> @@ -556,18 +560,13 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
>  	case REF_DETACHED_HEAD:
>  		color = BRANCH_COLOR_CURRENT;
>  		desc = get_head_description();
> +		c = '*';
>  		break;
>  	default:
>  		color = BRANCH_COLOR_PLAIN;
>  		break;
>  	}
>  
> -	c = ' ';
> -	if (current) {
> -		c = '*';
> -		color = BRANCH_COLOR_CURRENT;
> -	}

I actually prefered the old way: current is a Boolean that says
semantically "this is the current branch", and the Boolean is turned
into presentation directives in a separate piece of code.

I'd write

char c;
int current = 0;

...

if (...)
	current = 1;
...
case REF_DETACHED_HEAD:
	current = 1;
...

and keep the last hunk.

(IOW, turn current from a parameter into a local variable)

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

Re: [RFC/PATCH] Port branch.c to use ref-filter APIs

[Matthieu Moy]

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

> This version also doesn't use the printing options provided by
> branch.c.

Do you mean "provided by ref-filter.{c,h}"?

> I wanted to discuss how exactly to go about that, because in branch.c,
> we might need to change the --format based on attributes such as
> abbrev, verbose and so on. But ref-filter expects us to verify the
> format before filtering.

I took time to understand the problem, but here's my understanding:

ref-filter expects the code to look like

	format = ...;
	verify_ref_format(format);
	filter_refs(...);
	for (...)
		show_ref_array_item(..., format, ...);

and in the case of "git branch -v", you need to align the sha1s based on
the longest branch name, i.e. use %(padright:N+1) where N is the longest
branch name. And you can get N only after calling filter_refs, while you
need it for verify_ref_format().

Is my understanding correct?

If so, what prevents swapping the order of verify_ref_format and
filter_refs? I understand that verify_ref_format() builds used_atom and
other data-structures, hence it has to be called before
show_ref_array_item() and before sorting, but I don't think you need it
before filter_refs (I may have missed something though).

So, the code could look like:

filter_refs(...)
if (--format is given)
	format = the argument of --format
else
	format = STRBUF_INIT;
	strbuf_add(&format, "%(some_directive_to_display '*' if needed)");
	if (verbose)
		strbuf_addf(&format, "%(padright:%d)", max_width);
	...
verify_ref_format(format);
ref_array_sort(...);
for (...)
	show_ref_array_item(...);

(BTW, a trivial helper function to display the whole ref_array could
help. It would avoid having each caller write the 'for' loop)

Ideally, you could also have a modifier atom %(alignleft) that would
not need an argument and that would go through the ref_array to find the
maxwidth, and do the alignment. That would give even more flexibility to
the end users of "for-each-ref --format".

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

Re: [RFC/PATCH 03/11] ref-filter: add option to filter only branches

[Matthieu Moy]

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

> +static int filter_branch_kind(struct ref_filter *filter, const char *refname)
> +{
> +	int kind, i;
> +
> +	static struct {
> +		int kind;
> +		const char *prefix;
> +	} ref_kind[] = {
> +		{ REF_LOCAL_BRANCH, "refs/heads/" },
> +		{ REF_REMOTE_BRANCH, "refs/remotes/" },
> +	};

Nit: I would swap the order of fields, to make it a bit clearer that
this is a kind of dictionary key -> value (I think it's more common to
write it in this order than value <- key).

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

Re: [RFC/PATCH 09/11] branch.c: use 'ref-filter' data structures

[Matthieu Moy]

Christian Couder <christian.couder@gmail.com> writes:

> On Tue, Jul 28, 2015 at 8:56 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>
>> +static void ref_array_append(struct ref_array *array, const char *refname)
>> +{
>> +       size_t len = strlen(refname);
>> +       struct ref_array_item *ref = xcalloc(1, sizeof(struct ref_array_item) + len + 1);
>> +       memcpy(ref->refname, refname, len);
>> +       ref->refname[len] = '\0';

This looks very much like new_ref_array_item, except that the later also
takes an objectname parameter. I find it suspicious that you leave the
objectname field uninitialized.

Why is this code not calling new_ref_array_item?

A detail: you could return a pointer to the newly allocated object to
write

	item = ref_array_append(array, refname);

instead of

	ref_array_append(array, refname);
	item = array->items[array->nr - 1];

>> +       REALLOC_ARRAY(array->items, array->nr + 1);
>> +       array->items[array->nr++] = ref;
>> +}
>
> This function belongs more to ref-filter.{c,h}...

The function disapears in the next commit, but I also think that this
function deserves to exist in ref-filter.{c,h} and remain after the end
of the series.

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

Re: [RFC/PATCH 10/11] branch.c: use 'ref-filter' APIs

[Matthieu Moy]

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

> @@ -458,7 +345,7 @@ static void add_verbose_info(struct strbuf *out, struct ref_array_item *item,
>  	}
>  
>  	if (item->kind == REF_LOCAL_BRANCH)
> -		fill_tracking_info(&stat, item->refname, filter->verbose > 1);
> +		fill_tracking_info(&stat, refname, filter->verbose > 1);

Why can't you continue using item->refname?

(It's a real question)

> @@ -635,14 +495,21 @@ static void print_ref_list(struct ref_filter *filter)
>  	/* Print detached heads before sorting and printing the rest */
>  	if (filter->detached) {
>  		print_ref_item(array.items[index - 1], maxwidth, filter, remote_prefix);
> -		index -= 1;
> +		array.nr--;
>  	}
>  
> -	qsort(array.items, index, sizeof(struct ref_array_item *), ref_cmp);
> +	if (!sorting) {
> +		def_sorting.next = NULL;
> +		def_sorting.atom = parse_ref_filter_atom(sort_type,
> +							 sort_type + strlen(sort_type));
> +		sorting = &def_sorting;
> +	}
> +	ref_array_sort(sorting, &array);

Does this belong to print_ref_list()? Is it not possible to extract it
to get a code closer to the simple:

	filter_refs(...);
	ref_array_sort(...);
	print_ref_list(...);

?

> -	for (i = 0; i < index; i++)
> +	for (i = 0; i < array.nr; i++)
>  		print_ref_item(array.items[i], maxwidth, filter, remote_prefix);

Now that we have show_ref_array_item, it may make sense to rename
print_ref_item to something that make the difference between these
functions more explicit. Well, ideally, you'd get rid of it an actually
use show_ref_array_item, but if you are to keep it, maybe
print_ref_item_default_branch_format (or something shorter)?

> --- a/ref-filter.h
> +++ b/ref-filter.h
> @@ -49,7 +49,6 @@ struct ref_sorting {
>  struct ref_array_item {
>  	unsigned char objectname[20];
>  	int flag, kind;
> -	int ignore : 1;

You should explain why you needed it and why you don't need it anymore
(I guess, because it was used to implement --merge and you now get it
from ref-filter).

> --- a/t/t1430-bad-ref-name.sh
> +++ b/t/t1430-bad-ref-name.sh
> @@ -38,7 +38,7 @@ test_expect_success 'fast-import: fail on invalid branch name "bad[branch]name"'
>  	test_must_fail git fast-import <input
>  '
>  
> -test_expect_success 'git branch shows badly named ref' '
> +test_expect_failure 'git branch does not show badly named ref' '

I'm not sure what's the convention, but I think the test description
should give the expected behavior even with test_expect_failure.

And please help the reviewers by saying what's the status wrt this test
(any plan on how to fix it?).

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

Re: [RFC/PATCH 01/11] ref-filter: add "%(objectname:size=X)" option

[Junio C Hamano]

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

> From: Karthik Nayak <karthik.188@gmail.com>
>
> Add support for %(objectname:size=X) where X is a number.
> This will print the first X characters of an objectname.
> The minimum value for X is 5. Hence any value lesser than
> 5 will default to 5 characters.

Is the reason why you do not call this "abbrev" because you are
allowing it to truncate to a non-unique prefix?

If so, wouldn't it make more sense to treat this kind of string
function in a way very similar to your earlier "padright"?  I.e.
"%(maxwidth:5)%(objectname)" or something?

If not, and if this is really an abbrev, then it makes sense to make
it specific to the objectname, e.g. "%(objectname:abbrev=7)".

Re: [RFC/PATCH] Port branch.c to use ref-filter APIs

[Karthik Nayak]

On Tue, Jul 28, 2015 at 7:05 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> This version also doesn't use the printing options provided by
>> branch.c.
>
> Do you mean "provided by ref-filter.{c,h}"?
>

Yes! my bad.

>> I wanted to discuss how exactly to go about that, because in branch.c,
>> we might need to change the --format based on attributes such as
>> abbrev, verbose and so on. But ref-filter expects us to verify the
>> format before filtering.
>
> I took time to understand the problem, but here's my understanding:
>
> ref-filter expects the code to look like
>
>         format = ...;
>         verify_ref_format(format);
>         filter_refs(...);
>         for (...)
>                 show_ref_array_item(..., format, ...);
>
> and in the case of "git branch -v", you need to align the sha1s based on
> the longest branch name, i.e. use %(padright:N+1) where N is the longest
> branch name. And you can get N only after calling filter_refs, while you
> need it for verify_ref_format().
>
> Is my understanding correct?

Absolutely correct :)

>
> If so, what prevents swapping the order of verify_ref_format and
> filter_refs? I understand that verify_ref_format() builds used_atom and
> other data-structures, hence it has to be called before
> show_ref_array_item() and before sorting, but I don't think you need it
> before filter_refs (I may have missed something though).
>

Nope! This is exactly what I'm trying :D

> So, the code could look like:
>
> filter_refs(...)
> if (--format is given)
>         format = the argument of --format
> else
>         format = STRBUF_INIT;
>         strbuf_add(&format, "%(some_directive_to_display '*' if needed)");
>         if (verbose)
>                 strbuf_addf(&format, "%(padright:%d)", max_width);
>         ...
> verify_ref_format(format);
> ref_array_sort(...);
> for (...)
>         show_ref_array_item(...);
>
> (BTW, a trivial helper function to display the whole ref_array could
> help. It would avoid having each caller write the 'for' loop)
>

This I gotta try! Have been just keeping the flow the same and trying to mess
around with how formatting works instead.

> Ideally, you could also have a modifier atom %(alignleft) that would
> not need an argument and that would go through the ref_array to find the
> maxwidth, and do the alignment. That would give even more flexibility to
> the end users of "for-each-ref --format".

This could work for refname, as each ref_array_item holds the refname,
But other atoms are only filled in after a call to verify_ref_format().
What we could do would be after filling in all atom values, have a loop
through all items with their atom values and maybe implement this.
But I don't see this as priority for now, so will mark it off for later.
Thanks

-- 
Regards,
Karthik Nayak

Re: [RFC/PATCH 01/11] ref-filter: add "%(objectname:size=X)" option

[Karthik Nayak]

On Tue, Jul 28, 2015 at 2:12 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> +     if (skip_prefix(name, "objectname:size=", &p)) {
>> +             unsigned int size = atoi(p);
>
> You have the same problem as for tag.c: don't use atoi, and make
> accurate error checking (absence of value, negative value, non-integer
> value).
>
> If you have other higher-priorities task, leave a temporary comment
> /* FIXME: ... */ or /* TODO: ... */ and make sure you have no such
> comment when you drop the "RFC" in the subject of your emails.
>

It's a small change, will fix it with the drop in RFC :)

-- 
Regards,
Karthik Nayak

Re: [RFC/PATCH 01/11] ref-filter: add "%(objectname:size=X)" option

[Karthik Nayak]

On Tue, Jul 28, 2015 at 9:13 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> From: Karthik Nayak <karthik.188@gmail.com>
>>
>> Add support for %(objectname:size=X) where X is a number.
>> This will print the first X characters of an objectname.
>> The minimum value for X is 5. Hence any value lesser than
>> 5 will default to 5 characters.
>
> Is the reason why you do not call this "abbrev" because you are
> allowing it to truncate to a non-unique prefix?
>
> If so, wouldn't it make more sense to treat this kind of string
> function in a way very similar to your earlier "padright"?  I.e.
> "%(maxwidth:5)%(objectname)" or something?
>
> If not, and if this is really an abbrev, then it makes sense to make
> it specific to the objectname, e.g. "%(objectname:abbrev=7)".

It is finding unique abbrev, I just named size as it was smaller. But
abbrev seems better, will rename, thanks :)


-- 
Regards,
Karthik Nayak

Re: [RFC/PATCH 02/11] ref-filter: add 'colornext' atom

[Karthik Nayak]

On Tue, Jul 28, 2015 at 2:15 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> --- a/t/t6302-for-each-ref-filter.sh
>> +++ b/t/t6302-for-each-ref-filter.sh
>> @@ -133,4 +133,20 @@ test_expect_success 'reverse version sort' '
>>       test_cmp expect actual
>>  '
>>
>> +get_color ()
>> +{
>> +     git config --get-color no.such.slot "$1"
>> +}
>> +
>> +cat >expect <<EOF &&
>> +$(get_color green)foo1.10$(get_color reset)||
>> +$(get_color green)foo1.3$(get_color reset)||
>> +$(get_color green)foo1.6$(get_color reset)||
>> +EOF
>> +
>> +test_expect_success 'check `colornext` format option' '
>> +     git for-each-ref --format="%(colornext:green)%(refname:short)||" | grep "foo" >actual &&
>> +     test_cmp expect actual
>> +'
>
> This is not a very good test: you're not checking that colornext applies
> to the next and only this one. Similarly to what I suggested for
> padright, I'd suggest
>
>   --format="%(refname:short)%(colornext:green)|%(refname:short)|%(refname:short)|"
>

That was the purpose of the "||" but that doesn't check the color of next atom,
Thanks for the example will use that :)

-- 
Regards,
Karthik Nayak

Re: [RFC/PATCH 02/11] ref-filter: add 'colornext' atom

[Karthik Nayak]

On Tue, Jul 28, 2015 at 2:43 PM, Christian Couder
<christian.couder@gmail.com> wrote:
> On Tue, Jul 28, 2015 at 8:56 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> @@ -712,6 +713,15 @@ static void populate_value(struct ref_array_item *ref)
>>                         v->modifier_atom = 1;
>>                         v->pad_to_right = 1;
>>                         continue;
>> +               } else if (starts_with(name, "colornext:")) {
>> +                       char color[COLOR_MAXLEN] = "";
>> +
>> +                       if (color_parse(name + 10, color) < 0)
>> +                               die(_("unable to parse format"));
>
> Maybe use skip_prefix(), and die() with a more helpful message.
>

Okay will do. Thanks!

-- 
Regards,
Karthik Nayak

Re: [RFC/PATCH 01/11] ref-filter: add "%(objectname:size=X)" option

[Junio C Hamano]

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

> From: Karthik Nayak <karthik.188@gmail.com>
>
> Add support for %(objectname:size=X) where X is a number.
> This will print the first X characters of an objectname.
> The minimum value for X is 5. Hence any value lesser than
> 5 will default to 5 characters.

Where does this hardcoded 5 come from?

I'd agree that we would want some minimum for sanity (not safety),
but I do not think we want random callers of find-unique-abbrev
arbitrarily imposing their own minimum, making different codepaths
behave inconsistently without a good reason.

It seems that the minimum we use for sanity at the core level is
MINIMUM_ABBREV.  Is there a reason why that value is inappropriate
for this codepath?

>
> 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 | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index 0a34924..4c5e3f8 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -196,6 +196,8 @@ 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)
>  {
> +	const char *p;
> +
>  	if (!strcmp(name, "objectname")) {
>  		char *s = xmalloc(41);
>  		strcpy(s, sha1_to_hex(sha1));
> @@ -206,6 +208,13 @@ static int grab_objectname(const char *name, const unsigned char *sha1,
>  		v->s = xstrdup(find_unique_abbrev(sha1, DEFAULT_ABBREV));
>  		return 1;
>  	}
> +	if (skip_prefix(name, "objectname:size=", &p)) {
> +		unsigned int size = atoi(p);
> +		if (size < 5)
> +			size = 5;
> +		v->s = xstrdup(find_unique_abbrev(sha1, size));
> +		return 1;
> +	}
>  	return 0;
>  }

Re: [RFC/PATCH 03/11] ref-filter: add option to filter only branches

[Karthik Nayak]

On Tue, Jul 28, 2015 at 7:08 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> +static int filter_branch_kind(struct ref_filter *filter, const char *refname)
>> +{
>> +     int kind, i;
>> +
>> +     static struct {
>> +             int kind;
>> +             const char *prefix;
>> +     } ref_kind[] = {
>> +             { REF_LOCAL_BRANCH, "refs/heads/" },
>> +             { REF_REMOTE_BRANCH, "refs/remotes/" },
>> +     };
>
> Nit: I would swap the order of fields, to make it a bit clearer that
> this is a kind of dictionary key -> value (I think it's more common to
> write it in this order than value <- key).
>

This was borrowed from branch.c, but ok will change it!
Thanks


-- 
Regards,
Karthik Nayak

Re: [RFC/PATCH 04/11] ref-filter: add 'ifexists' atom

[Karthik Nayak]

On Tue, Jul 28, 2015 at 1:24 PM, Jacob Keller <jacob.keller@gmail.com> wrote:
> On Mon, Jul 27, 2015 at 11:56 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> The 'ifexists' atom allows us to print a required format if the
>> preceeding atom has a value. If the preceeding atom has no value then
>
> Don't you mean "following atom" here? since you do document it as "the
> next atom" below you should fix the commit message as well to match.
> In any respect, this is a useful formatting atom :)
>

That should have been `succeeding` atom. My bad! thanks :)

> %(ifexists:[%s])%(atom) where the contents of atom will be placed into %s?
>
> I suggest documenting that the atom will be placed into the contents
> of ifexists via the %s indicator, as you do show an example but don't
> explicitely say %s is the formatting character.
>

Yeah! I should have explicitly mentioned that, thanks!

-- 
Regards,
Karthik Nayak

Re: [RFC/PATCH 04/11] ref-filter: add 'ifexists' atom

[Karthik Nayak]

On Tue, Jul 28, 2015 at 2:20 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> --- a/t/t6302-for-each-ref-filter.sh
>> +++ b/t/t6302-for-each-ref-filter.sh
>> @@ -149,4 +149,25 @@ test_expect_success 'check `colornext` format option' '
>>       test_cmp expect actual
>>  '
>>
>> +test_expect_success 'check `ifexists` format option' '
>> +     cat >expect <<-\EOF &&
>> +     [foo1.10]
>> +     [foo1.3]
>> +     [foo1.6]
>> +     EOF
>> +     git for-each-ref --format="%(ifexists:[%s])%(refname:short)" | grep "foo" >actual &&
>> +     test_cmp expect actual
>> +'
>
> You're testing only the positive case of ifexists, right? You need a
> test for the negative case (i.e. the attribute does not exist) too.
> Ideally, check that the ifexist actually applies to the right attribute,
> like
>
> --format '%(ifexist:<%s>)%(attribute1) %(ifexist:[%s])%(attribute2)'
>
> and manage to get an output like
>
> <foo>
>  [bar]
> <foobar> [barfoo]
>

Yes! this seems like an important test, thanks :)

>
> You have a double || that is not useful. Not really harmful either, but
> it may distract the reader. You may want to s/||/|/.
>

Will change!

-- 
Regards,
Karthik Nayak

Re: [RFC/PATCH] Port branch.c to use ref-filter APIs

[Matthieu Moy]

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

> On Tue, Jul 28, 2015 at 7:05 PM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>
>> Ideally, you could also have a modifier atom %(alignleft) [...]
>
> This could work for refname, as each ref_array_item holds the refname,
> But other atoms are only filled in after a call to
> verify_ref_format().

If you swap the order and call verify_ref_format() after filter_refs(),
then you could detect %(alignleft) and iterate over the list as needed
in verify_ref_format().

(You would actually force swapping the order and you would need to adapt
other callers in for-each-ref and tag)

> But I don't see this as priority for now, so will mark it off for later.

Absolutely, that's what I meant by "Ideally". I'm just thinking aloud.

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

Re: [RFC/PATCH 04/11] ref-filter: add 'ifexists' atom

[Junio C Hamano]

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

> The 'ifexists' atom allows us to print a required format if the
> preceeding atom has a value. If the preceeding atom has no value then
> the format given is not printed. e.g. to print "[<refname>]" we can
> now use the format "%(ifexists:[%s])%(refname)".

A handful of "huh?" on the design.

 - The atom says "if *exists*" and explanation says "has a value".
   How are they related?  Does an atom whose value is an empty
   string has a value?  Or is "ifexists" meant to be used only to
   ignore meaningless atom, e.g. %(*objectname) applied to a ref that
   refers to an object that is not an annotated tag?

 - That %s looks ugly.  Are there cases where a user may want to say
   %(ifexists:[%i]) or something other than 's' after that per-cent?

   . Is it allowed to have more than one %s there?
   . Is it allowed to have no %s there?

 - The syntax makes the reader wonder if [] is part of the
   construct, or just an example of any arbitrary string, i.e. is
   "%(ifexists:the %s can be part of arbitrary string)" valid?

 - If an arbitrary string is allowed, is there any quoting mechanism
   to allow ")" to be part of that arbitrary string?
   
 - What, if anything, is allowed to come between %(ifexists...) and
   the next atom like %(refname)?  For example, are these valid
   constructs?

    . %(ifexists...)%(padright:20)%(refname)
    . %(ifexists...) %(refname) [%(subject)]

 - This syntax does not seem to allow switching on an attribute to
   show or not to show another, e.g. "if %(*objectname) makes sense,
   then show '%(padright:20)%(refname:short) %(*subject)' for it".

Re: [RFC/PATCH 05/11] branch: fix width computation

[Karthik Nayak]

On Tue, Jul 28, 2015 at 3:17 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> From: Karthik Nayak <karthik.188@gmail.com>
>
> Why did send-email add this From: header? Strange, it has the same
> content as your actual From: field.
>

Not sure why, everything else came out fine. Idk what happened here.

>> Remove unnecessary variables from ref_list and ref_item
>> which were used for width computation. Make other changes
>> accordingly. This patch is a precursor for porting branch.c
>> to use ref-filter APIs.
>
> You can explain better why this is needed. I guess something like "we're
> making struct ref_item similar to ref-filter's ref_array_item", but the
> reader shouldn't have to guess.
>

Will explain like you suggested.

> You should adujst the subject like BTW, I don't think you are "fixing"
> anything.
>

I guess refactor would be a better word here.

> On a side note: on the "tag" series, see how explaining better and
> splitting patches led not only to a better history, but also to better
> final code, and to finding a actual bugs (the %(color) thing and the
> absence of reset on the state variable) even after sereval rounds of
> review? I'm being picky and demanding, but don't see that as a complain
> from me, just hints on getting even better ;-).
>

Haha, I look forward to reviews, they show things I usually miss out on,
and help me get better. so keep them coming, I'll keep improving :)
Thanks

>> @@ -386,15 +386,8 @@ static int append_ref(const char *refname, const struct object_id *oid, int flag
>>       newitem->name = xstrdup(refname);
>>       newitem->kind = kind;
>>       newitem->commit = commit;
>> -     newitem->width = utf8_strwidth(refname);
>>       newitem->dest = resolve_symref(orig_refname, prefix);
>>       newitem->ignore = 0;
>> -     /* adjust for "remotes/" */
>> -     if (newitem->kind == REF_REMOTE_BRANCH &&
>> -         ref_list->kinds != REF_REMOTE_BRANCH)
>> -             newitem->width += 8;
>> -     if (newitem->width > ref_list->maxwidth)
>> -             ref_list->maxwidth = newitem->width;
>>
>>       return 0;
>>  }
>
> OK, in the old code, the width computation is done when inserting the
> ref into the array. In the new code, you build the array and then do the
> width computation. You can explain this better in the commit message I
> think (instead of "Make other changes accordingly" which doesn't bring
> much).

Okay I guess will do, just didn't want to explain the whole thing in the commit
message.

>
>> @@ -505,11 +498,12 @@ static void add_verbose_info(struct strbuf *out, struct ref_item *item,
>>  }
>>
>>  static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
>> -                        int abbrev, int current, char *prefix)
>> +                        int abbrev, int current, const char *remote_prefix)
>>  {
>>       char c;
>>       int color;
>>       struct strbuf out = STRBUF_INIT, name = STRBUF_INIT;
>> +     const char *prefix = "";
>>
>>       if (item->ignore)
>>               return;
>> @@ -520,6 +514,7 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
>>               break;
>>       case REF_REMOTE_BRANCH:
>>               color = BRANCH_COLOR_REMOTE;
>> +             prefix = remote_prefix;
>>               break;
>
> Why do you need these two hunks? I did not investigate in details, but
> it seems you're calling print_ref_item either with prefix="" or with
> prefix=remote_prefix so it would seem that keeping "prefix" as argument
> would work. I guess I missed something.

This is needed as whenever we do "git branch", show_detached() calls
print_ref_item()
with remote_prefix="". But print_ref_list() calls print_ref_item()
with remote_prefix="remotes"
(only when `git branch -a` is called remote_prefix=""). But only
remote branches should be
given the remotes prefix.

>
>> -static int calc_maxwidth(struct ref_list *refs)
>> +static int calc_maxwidth(struct ref_list *refs, int remote_bonus)
>>  {
>> -     int i, w = 0;
>> +     int i, max = 0;
>>       for (i = 0; i < refs->index; i++) {
>> +             struct ref_item *it = &refs->list[i];
>> +             int w = utf8_strwidth(it->name);
>> +
>>               if (refs->list[i].ignore)
>>                       continue;
>> -             if (refs->list[i].width > w)
>> -                     w = refs->list[i].width;
>> +             if (it->kind == REF_REMOTE_BRANCH)
>> +                     w += remote_bonus;
>> +             if (w > max)
>> +                     max = w;
>>       }
>> -     return w;
>> +     return max;
>>  }
>
> The old code was using 'w' for the max and no variable for the value at
> the current iteration. You're using 'max' for the max and 'w' at the
> current iteration. Using the same name 'w' for different things in the
> pre- and post-image of the patch distracts the reader.
>
> It may make sense to s/w/max/ in a separate preparatory patch. Or maybe
> it's overkill.
>

Since the change was minimal and easily understandable I think it's ok.
But if you still feel otherwise, I'll be happy to introduce a preparatory patch.

>> @@ -600,21 +600,18 @@ static char *get_head_description(void)
>>       return strbuf_detach(&desc, NULL);
>>  }
>>
>> -static void show_detached(struct ref_list *ref_list)
>> +static void show_detached(struct ref_list *ref_list, int maxwidth)
>>  {
>>       struct commit *head_commit = lookup_commit_reference_gently(head_sha1, 1);
>>
>>       if (head_commit && is_descendant_of(head_commit, ref_list->with_commit)) {
>>               struct ref_item item;
>>               item.name = get_head_description();
>> -             item.width = utf8_strwidth(item.name);
>>               item.kind = REF_LOCAL_BRANCH;
>>               item.dest = NULL;
>>               item.commit = head_commit;
>>               item.ignore = 0;
>> -             if (item.width > ref_list->maxwidth)
>> -                     ref_list->maxwidth = item.width;
>> -             print_ref_item(&item, ref_list->maxwidth, ref_list->verbose, ref_list->abbrev, 1, "");
>> +             print_ref_item(&item, maxwidth, ref_list->verbose, ref_list->abbrev, 1, "");
>>               free(item.name);
>>       }
>>  }
> ...
>> +     int maxwidth = 0;
> ...
>> +     if (verbose)
>> +             maxwidth = calc_maxwidth(&ref_list, strlen(remote_prefix));
>>
>>       qsort(ref_list.list, ref_list.index, sizeof(struct ref_item), ref_cmp);
>>
>>       detached = (detached && (kinds & REF_LOCAL_BRANCH));
>>       if (detached && match_patterns(pattern, "HEAD"))
>> -             show_detached(&ref_list);
>> +             show_detached(&ref_list, maxwidth);
>
> This hunks could ideally go in a preparatory patch that would just move
> the place where maxwidth is computed. This preparatory patch would just
> say
>
>         maxwidth = ref_list->maxwidth;
>
> and then you would do the actual change to
>
>         if (verbose)
>                 maxwidth = calc_maxwidth(...)
>
> without the distracting changes in the function's argument list.
>
> I won't insist on that, again it may be overkill. But reading the patch,
> I found it both rather trivial and hard to read, so there's room for
> improvement.
>

I find it too small to make a preparatory patch again, but if you really feel
so, like I said, I'll change :)

-- 
Regards,
Karthik Nayak

Re: [RFC/PATCH 06/11] branch: roll show_detached HEAD into regular ref_list

[Karthik Nayak]

On Tue, Jul 28, 2015 at 6:31 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Remove show_detached() and make detached HEAD to be rolled into
>> regular ref_list by adding REF_DETACHED_HEAD as a kind of branch and
>> supporting the same in append_ref().
>
> Again, this lacks the "why?" explanation.

Will include a small explanation.

>
>> Bump get_head_description() to the top.
>
> Here also: why? And this could easily go to a separate patch to let the
> reviewer focus on actual changes.

I'll move this to a preparatory patch.

>
> I think you're leaking a string here: get_head_description() builds an
> strbuf and returns the dynamically allocated string, which you never
> free.
>

Ah! good catch, will fix that.

>> -static void show_detached(struct ref_list *ref_list, int maxwidth)
>> -{
>> -     struct commit *head_commit = lookup_commit_reference_gently(head_sha1, 1);
>> -
>> -     if (head_commit && is_descendant_of(head_commit, ref_list->with_commit)) {
>
> I'm not sure what this if was doing, and why you can get rid of it. My
> understanding is that with_commit comes from --contains, and in the
> previous code the filtering was done at display time (detached HEAD was
> not shown if it was not contained in commits specified with --contains).
>
> Eventually, you'll use ref-filter to do this filtering so you won't need
> this check at display time.
>
> But am I correct that for a few commits, you ignore --contains on
> detached HEAD?
>

No we don't ignore --contains on detached HEAD.

Since detached HEAD now gets its data from append_ref(). The function
also checks for the --contains option.

>> @@ -678,15 +674,20 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru
>>       if (verbose)
>>               maxwidth = calc_maxwidth(&ref_list, strlen(remote_prefix));
>>
>> -     qsort(ref_list.list, ref_list.index, sizeof(struct ref_item), ref_cmp);
>> +     index = ref_list.index;
>> +
>> +     /* Print detached heads before sorting and printing the rest */
>
> I think you mean head (no s) or HEAD. It's not Mercurial ;-).
>

I meant HEAD, lol.

>> +     if (detached && (ref_list.list[index - 1].kind == REF_DETACHED_HEAD) &&
>> +         !strcmp(ref_list.list[index - 1].name, head)) {
>> +             print_ref_item(&ref_list.list[index - 1], maxwidth, verbose, abbrev,
>> +                            1, remote_prefix);
>> +             index -= 1;
>> +     }
>
> This relies on the assumption that HEAD has to be the last in the array.
> The assumption is correct since you call head_ref(append_ref, &cb) after
> for_each_rawref(append_ref, &cb). I think this deserves a comment to
> remind the reader that HEAD is always the last (here or at the call to
> head_ref to make sure nobody try to change the order between head_ref
> and for_each_rawref).
>

Yeah! makes sense, will add at the comment at the call to head_ref().

> It may be more natural to do it the other way around: call head_ref
> first and get HEAD first, as you are going to display it first (but in
> any case, you'll have to call qsort on a subset of the array so it
> doesn't change much).

That sorta messes things up with qsort, this keeps it clean I feel.

>
>> @@ -913,7 +914,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>>                       die(_("branch name required"));
>>               return delete_branches(argc, argv, delete > 1, kinds, quiet);
>>       } else if (list) {
>> -             int ret = print_ref_list(kinds, detached, verbose, abbrev,
>> +             int ret;
>> +             if (kinds & REF_LOCAL_BRANCH)
>> +                     kinds |= REF_DETACHED_HEAD;
>
> Perhaps add
>
>         /* git branch --local also shows HEAD when it is detached */
>

Yeah definitely!

-- 
Regards,
Karthik Nayak

Re: [RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer

[Karthik Nayak]

On Tue, Jul 28, 2015 at 6:39 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> We check if given ref is the current branch in print_ref_list().  Move
>> this check to print_ref_item() where it is checked right before
>> printing.
>
> This means that the '*' and the different color are coded in C, hence
> it's not possible to mimick this using "git for-each-ref --format ...".
>
> I do not consider this as blocking, but I think the ultimate goal should
> be to allow this, so that all the goodies of "git branch" can be made
> available to other ref-listing commands.
>

Not sure what you mean here.

>> --- a/builtin/branch.c
>> +++ b/builtin/branch.c
>> @@ -534,9 +534,9 @@ static char *get_head_description(void)
>>  }
>>
>>  static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
>> -                        int abbrev, int current, const char *remote_prefix)
>> +                        int abbrev, int detached, const char *remote_prefix)
>>  {
>> -     char c;
>> +     char c = ' ';
>>       int color;
>>       struct strbuf out = STRBUF_INIT, name = STRBUF_INIT;
>>       const char *prefix = "";
>> @@ -547,7 +547,11 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
>>
>>       switch (item->kind) {
>>       case REF_LOCAL_BRANCH:
>> -             color = BRANCH_COLOR_LOCAL;
>> +             if (!detached && !strcmp(item->name, head)) {
>> +                     color = BRANCH_COLOR_CURRENT;
>> +                     c = '*';
>> +             } else
>> +                     color = BRANCH_COLOR_LOCAL;
>>               break;
>>       case REF_REMOTE_BRANCH:
>>               color = BRANCH_COLOR_REMOTE;
>> @@ -556,18 +560,13 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,
>>       case REF_DETACHED_HEAD:
>>               color = BRANCH_COLOR_CURRENT;
>>               desc = get_head_description();
>> +             c = '*';
>>               break;
>>       default:
>>               color = BRANCH_COLOR_PLAIN;
>>               break;
>>       }
>>
>> -     c = ' ';
>> -     if (current) {
>> -             c = '*';
>> -             color = BRANCH_COLOR_CURRENT;
>> -     }
>
> I actually prefered the old way: current is a Boolean that says
> semantically "this is the current branch", and the Boolean is turned
> into presentation directives in a separate piece of code.
>
> I'd write
>
> char c;
> int current = 0;
>
> ...
>
> if (...)
>         current = 1;
> ...
> case REF_DETACHED_HEAD:
>         current = 1;
> ...
>
> and keep the last hunk.
>
> (IOW, turn current from a parameter into a local variable)
>

Thanks will do this.

-- 
Regards,
Karthik Nayak

Re: [RFC/PATCH 09/11] branch.c: use 'ref-filter' data structures

[Karthik Nayak]

On Tue, Jul 28, 2015 at 1:52 PM, Christian Couder
<christian.couder@gmail.com> wrote:
> On Tue, Jul 28, 2015 at 8:56 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> diff --git a/ref-filter.h b/ref-filter.h
>> index 7d1871d..3458595 100644
>> --- a/ref-filter.h
>> +++ b/ref-filter.h
>> @@ -5,6 +5,7 @@
>>  #include "refs.h"
>>  #include "commit.h"
>>  #include "parse-options.h"
>> +#include "revision.h"
>>
>>  /* Quoting styles */
>>  #define QUOTE_NONE 0
>> @@ -48,6 +49,7 @@ struct ref_sorting {
>>  struct ref_array_item {
>>         unsigned char objectname[20];
>>         int flag, kind;
>> +       int ignore : 1;
>
> Maybe you could add a comment to say that this will be removed in the
> next patch.
>

Yes, Will do. Thanks :)

-- 
Regards,
Karthik Nayak

Re: [RFC/PATCH 09/11] branch.c: use 'ref-filter' data structures

[Karthik Nayak]

On Tue, Jul 28, 2015 at 1:47 PM, Christian Couder
<christian.couder@gmail.com> wrote:
> On Tue, Jul 28, 2015 at 8:56 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>
>> +static void ref_array_append(struct ref_array *array, const char *refname)
>> +{
>> +       size_t len = strlen(refname);
>> +       struct ref_array_item *ref = xcalloc(1, sizeof(struct ref_array_item) + len + 1);
>> +       memcpy(ref->refname, refname, len);
>> +       ref->refname[len] = '\0';
>> +       REALLOC_ARRAY(array->items, array->nr + 1);
>> +       array->items[array->nr++] = ref;
>> +}
>
> This function belongs more to ref-filter.{c,h}...
>

Its a temporary function which is removed in the next patch.

>
>> -       ALLOC_GROW(ref_list->list, ref_list->index + 1, ref_list->alloc);
>> +       ref_array_append(array, refname);
>> +       item = array->items[array->nr - 1];
>
> ...and the above is a bit ugly.

Will fix that :)

-- 
Regards,
Karthik Nayak

Re: [RFC/PATCH 09/11] branch.c: use 'ref-filter' data structures

[Karthik Nayak]

On Tue, Jul 28, 2015 at 7:18 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Christian Couder <christian.couder@gmail.com> writes:
>
>> On Tue, Jul 28, 2015 at 8:56 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>
>>> +static void ref_array_append(struct ref_array *array, const char *refname)
>>> +{
>>> +       size_t len = strlen(refname);
>>> +       struct ref_array_item *ref = xcalloc(1, sizeof(struct ref_array_item) + len + 1);
>>> +       memcpy(ref->refname, refname, len);
>>> +       ref->refname[len] = '\0';
>
> This looks very much like new_ref_array_item, except that the later also
> takes an objectname parameter. I find it suspicious that you leave the
> objectname field uninitialized.
>

Well the objectname is not required here, the idea is to retain the way branch.c
works.

> Why is this code not calling new_ref_array_item?
>

Because no objectname is there.

> A detail: you could return a pointer to the newly allocated object to
> write
>
>         item = ref_array_append(array, refname);
>
> instead of
>
>         ref_array_append(array, refname);
>         item = array->items[array->nr - 1];
>

Yeah doing that :)

>>> +       REALLOC_ARRAY(array->items, array->nr + 1);
>>> +       array->items[array->nr++] = ref;
>>> +}
>>
>> This function belongs more to ref-filter.{c,h}...
>
> The function disapears in the next commit, but I also think that this
> function deserves to exist in ref-filter.{c,h} and remain after the end
> of the series.
>

Why though? I don't see the need when new_ref_array_item() is present.

-- 
Regards,
Karthik Nayak

Re: [RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer

[Jacob Keller]

On Tue, Jul 28, 2015 at 1:12 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> On Tue, Jul 28, 2015 at 6:39 PM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>
>>> We check if given ref is the current branch in print_ref_list().  Move
>>> this check to print_ref_item() where it is checked right before
>>> printing.
>>
>> This means that the '*' and the different color are coded in C, hence
>> it's not possible to mimick this using "git for-each-ref --format ...".
>>
>> I do not consider this as blocking, but I think the ultimate goal should
>> be to allow this, so that all the goodies of "git branch" can be made
>> available to other ref-listing commands.
>>
>
> Not sure what you mean here.
>

He means to make sure that any feature that was in tag, branch,
for-each-ref, etc should be available as part of ref-filter and in all
of them

Maybe he misunderstood code or maybe you misunderstood the comment here?

Regards,
Jake

Re: [RFC/PATCH 10/11] branch.c: use 'ref-filter' APIs

[Karthik Nayak]

On Tue, Jul 28, 2015 at 1:27 PM, Jacob Keller <jacob.keller@gmail.com> wrote:
> On Tue, Jul 28, 2015 at 12:11 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> Make 'branch.c' use 'ref-filter' APIs for iterating through refs
>> sorting. This removes most of the code used in 'branch.c' replacing it
>> with calls to the 'ref-filter' library.
>>
>> Make 'tag.c' use the 'filter_refs()' function provided by 'ref-filter'
>> to filter out tags based on the options set.
>>
>
> This patch doesn't do anything to tag.c, so you should update the
> commit message to remove this or replace it with s/tag/branch if it
> was intended to be about branch.c?
>
> Regards,

Copy-paste error, thanks for pointing out.

-- 
Regards,
Karthik Nayak

Re: [RFC/PATCH 10/11] branch.c: use 'ref-filter' APIs

[Karthik Nayak]

On Tue, Jul 28, 2015 at 1:39 PM, Christian Couder
<christian.couder@gmail.com> wrote:
> On Tue, Jul 28, 2015 at 9:11 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>
>> diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
>> index a67138a..897cd81 100644
>> --- a/Documentation/git-branch.txt
>> +++ b/Documentation/git-branch.txt
>> @@ -11,7 +11,7 @@ SYNOPSIS
>>  'git branch' [--color[=<when>] | --no-color] [-r | -a]
>>         [--list] [-v [--abbrev=<length> | --no-abbrev]]
>>         [--column[=<options>] | --no-column]
>> -       [(--merged | --no-merged | --contains) [<commit>]] [<pattern>...]
>> +       [(--merged | --no-merged | --contains) [<commit>]] [--sort=<key>] [<pattern>...]
>
> Maybe: [(--[no-]merged | --contains) [<commit>]]

Thats an existing string. So not sure if this patch is the right place to
make this change.

-- 
Regards,
Karthik Nayak

Re: [RFC/PATCH 06/11] branch: roll show_detached HEAD into regular ref_list

[Matthieu Moy]

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

> On Tue, Jul 28, 2015 at 6:31 PM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>>
>>> -static void show_detached(struct ref_list *ref_list, int maxwidth)
>>> -{
>>> -     struct commit *head_commit = lookup_commit_reference_gently(head_sha1, 1);
>>> -
>>> -     if (head_commit && is_descendant_of(head_commit, ref_list->with_commit)) {
>>
>> I'm not sure what this if was doing, and why you can get rid of it. My
>> understanding is that with_commit comes from --contains, and in the
>> previous code the filtering was done at display time (detached HEAD was
>> not shown if it was not contained in commits specified with --contains).
>>
>> Eventually, you'll use ref-filter to do this filtering so you won't need
>> this check at display time.
>>
>> But am I correct that for a few commits, you ignore --contains on
>> detached HEAD?
>>
>
> No we don't ignore --contains on detached HEAD.
>
> Since detached HEAD now gets its data from append_ref(). The function
> also checks for the --contains option.

Ah, OK. Previously, detached HEAD and branches were completely
different, each having its own if (is_descendant_of(...)), and you're
now using only one in append_ref() before removing it completely in
favor of ref-filter.

That would deserve an explanation for other reviewers I think.

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

Re: [RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer

[Matthieu Moy]

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

> On Tue, Jul 28, 2015 at 6:39 PM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>
>>> We check if given ref is the current branch in print_ref_list().  Move
>>> this check to print_ref_item() where it is checked right before
>>> printing.
>>
>> This means that the '*' and the different color are coded in C, hence
>> it's not possible to mimick this using "git for-each-ref --format ...".
>>
>> I do not consider this as blocking, but I think the ultimate goal should
>> be to allow this, so that all the goodies of "git branch" can be made
>> available to other ref-listing commands.
>>
>
> Not sure what you mean here.

What you already know, but probably badly explained ;-).

Eventually, the output of "git branch" should correspond to a format
string (so git branch would be almost an alias for "git for-each-ref
refs/heads/ --format '...'"). Internally, this would mean using
show_ref_array_item instead of print_ref_item. This is what you managed
to do for "git tag".

You already identified one difficulty with sha1 alignment in "git branch
-v". I'm pointing out another which is that displaying the "*" in front
of the current branch is currently not possible with a format string.
You would need an atom like %(displayAStarIfTheBranchIsTheCurrentOne)
(for which you'd need to find a short-and-sweet name ;-) ).

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

Re: [RFC/PATCH 09/11] branch.c: use 'ref-filter' data structures

[Matthieu Moy]

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

> On Tue, Jul 28, 2015 at 7:18 PM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>> Christian Couder <christian.couder@gmail.com> writes:
>>
>>> On Tue, Jul 28, 2015 at 8:56 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>>
>>>> +static void ref_array_append(struct ref_array *array, const char *refname)
>>>> +{
>>>> +       size_t len = strlen(refname);
>>>> +       struct ref_array_item *ref = xcalloc(1, sizeof(struct ref_array_item) + len + 1);
>>>> +       memcpy(ref->refname, refname, len);
>>>> +       ref->refname[len] = '\0';
>>
>> This looks very much like new_ref_array_item, except that the later also
>> takes an objectname parameter. I find it suspicious that you leave the
>> objectname field uninitialized.
>>
>
> Well the objectname is not required here, the idea is to retain the way branch.c
> works.
>
>> Why is this code not calling new_ref_array_item?
>>
>
> Because no objectname is there.

You do have the object_id in the scope of the call-site, so why not use
it?

(Well, in any case, do as you think is best, it's temporary throw-away
code, we shouldn't loose too much time polishing it)

>> The function disapears in the next commit, but I also think that this
>> function deserves to exist in ref-filter.{c,h} and remain after the end
>> of the series.
>>
>
> Why though? I don't see the need when new_ref_array_item() is present.

OK, if the function is specificly for "append an item but leave the
objectname uninitialized", it's too specific to be useful somewhere
else. But then, make it more explicit in the name of the function and/or
in a comment.

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

Re: [RFC/PATCH 10/11] branch.c: use 'ref-filter' APIs

[Karthik Nayak]

Re-sending this as it wasn't sent as plain text and failed.

On Tue, Jul 28, 2015 at 7:47 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> @@ -458,7 +345,7 @@ static void add_verbose_info(struct strbuf *out, struct ref_array_item *item,
>>       }
>>
>>       if (item->kind == REF_LOCAL_BRANCH)
>> -             fill_tracking_info(&stat, item->refname, filter->verbose > 1);
>> +             fill_tracking_info(&stat, refname, filter->verbose > 1);
>
> Why can't you continue using item->refname?
>
> (It's a real question)

Because we call add_verbose_info() is called in print_ref_item() which calls
add_verbose_info() with refname=desc, where desc is a shortened refname.
And fill_tracking_info expects a shortened refname. hence we give it refname
which is nothing but desc.

>
>> @@ -635,14 +495,21 @@ static void print_ref_list(struct ref_filter *filter)
>>       /* Print detached heads before sorting and printing the rest */
>>       if (filter->detached) {
>>               print_ref_item(array.items[index - 1], maxwidth, filter, remote_prefix);
>> -             index -= 1;
>> +             array.nr--;
>>       }
>>
>> -     qsort(array.items, index, sizeof(struct ref_array_item *), ref_cmp);
>> +     if (!sorting) {
>> +             def_sorting.next = NULL;
>> +             def_sorting.atom = parse_ref_filter_atom(sort_type,
>> +                                                      sort_type + strlen(sort_type));
>> +             sorting = &def_sorting;
>> +     }
>> +     ref_array_sort(sorting, &array);
>
> Does this belong to print_ref_list()? Is it not possible to extract it
> to get a code closer to the simple:
>
>         filter_refs(...);
>         ref_array_sort(...);
>         print_ref_list(...);
>
> ?
>

We have a ref_defaulting_sorting but that defaults to sorting by 'refname'
but what we want in branch.c is to sort by 'type' rather. Hence we
need to have this small segment of code. About its placement, IDK if
print_ref_list() is the right place. But didn't find a better place.

>> -     for (i = 0; i < index; i++)
>> +     for (i = 0; i < array.nr; i++)
>>               print_ref_item(array.items[i], maxwidth, filter, remote_prefix);
>
> Now that we have show_ref_array_item, it may make sense to rename
> print_ref_item to something that make the difference between these
> functions more explicit. Well, ideally, you'd get rid of it an actually
> use show_ref_array_item, but if you are to keep it, maybe
> print_ref_item_default_branch_format (or something shorter)?
>

I guess it'll be converted to a helper for show_ref_array_item()
eventually so keeping that in mind maybe rename it to
get_format_and_print_ref().

>> --- a/ref-filter.h
>> +++ b/ref-filter.h
>> @@ -49,7 +49,6 @@ struct ref_sorting {
>>  struct ref_array_item {
>>       unsigned char objectname[20];
>>       int flag, kind;
>> -     int ignore : 1;
>
> You should explain why you needed it and why you don't need it anymore
> (I guess, because it was used to implement --merge and you now get it
> from ref-filter).
>

Yeah, I'll add that into the commit message.

>> --- a/t/t1430-bad-ref-name.sh
>> +++ b/t/t1430-bad-ref-name.sh
>> @@ -38,7 +38,7 @@ test_expect_success 'fast-import: fail on invalid branch name "bad[branch]name"'
>>       test_must_fail git fast-import <input
>>  '
>>
>> -test_expect_success 'git branch shows badly named ref' '
>> +test_expect_failure 'git branch does not show badly named ref' '
>
> I'm not sure what's the convention, but I think the test description
> should give the expected behavior even with test_expect_failure.
>
> And please help the reviewers by saying what's the status wrt this test
> (any plan on how to fix it?).
>

Well okay will rename the test description.
Since we use filter_refs() there's no real fix for this, ref_filter_handler()
skips over such refs. I'll mention something on these lines in the
commit message.

-- 
Regards,
Karthik Nayak

Re: [RFC/PATCH 10/11] branch.c: use 'ref-filter' APIs

[Karthik Nayak]

On Tue, Jul 28, 2015 at 7:47 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
>
> I'm not sure what's the convention, but I think the test description
> should give the expected behavior even with test_expect_failure.
>
> And please help the reviewers by saying what's the status wrt this test
> (any plan on how to fix it?).
>

On the other hand I wonder if the test is even needed as, we don't
really need it
Cause we remove that ability of branch.c by using filter_refs().

-- 
Regards,
Karthik Nayak

Re: [RFC/PATCH 11/11] branch: add '--points-at' option

[Karthik Nayak]

On Tue, Jul 28, 2015 at 1:16 PM, Jacob Keller <jacob.keller@gmail.com> wrote:
> On Tue, Jul 28, 2015 at 12:11 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> Add the '--points-at' option provided by 'ref-filter'. The option lets
>> the user to list only branches which points at the given object.
>>
>> Add documentation and tests 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 | 6 +++++-
>>  builtin/branch.c             | 7 ++++++-
>>  t/t3203-branch-output.sh     | 9 +++++++++
>>  3 files changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
>> index 897cd81..efa23a5 100644
>> --- a/Documentation/git-branch.txt
>> +++ b/Documentation/git-branch.txt
>> @@ -11,7 +11,8 @@ SYNOPSIS
>>  'git branch' [--color[=<when>] | --no-color] [-r | -a]
>>         [--list] [-v [--abbrev=<length> | --no-abbrev]]
>>         [--column[=<options>] | --no-column]
>> -       [(--merged | --no-merged | --contains) [<commit>]] [--sort=<key>] [<pattern>...]
>> +       [(--merged | --no-merged | --contains) [<commit>]] [--sort=<key>]
>> +       [--points-at <object>] [<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>]
>> @@ -237,6 +238,9 @@ start-point is either a local or remote-tracking branch.
>>         for-each-ref`. Sort order defaults to sorting based on branch
>>         type.
>>
>> +--points-at <object>::
>> +       Only list tags of the given object.
>> +
>
> s/tags/branches/ ?? Since this is for the branch version, I think this
> is just a copy-paste oversight.
>
>>  Examples
>>  --------
>>
>> diff --git a/builtin/branch.c b/builtin/branch.c
>> index 75d8bfd..d25f43b 100644
>> --- a/builtin/branch.c
>> +++ b/builtin/branch.c
>> @@ -26,6 +26,7 @@ static const char * const builtin_branch_usage[] = {
>>         N_("git branch [<options>] [-l] [-f] <branch-name> [<start-point>]"),
>>         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]"),
>>         NULL
>>  };
>>
>> @@ -647,6 +648,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>>                 OPT_COLUMN(0, "column", &colopts, N_("list branches in columns")),
>>                 OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"),
>>                              N_("field name to sort on"), &parse_opt_ref_sorting),
>> +               {
>> +                       OPTION_CALLBACK, 0, "points-at", &filter.points_at, N_("object"),
>> +                       N_("print only tags of the object"), 0, parse_opt_object_name
>> +               },
>
> Same as above. s/tags/branches/
>
>>                 OPT_END(),
>>         };
>>
>> @@ -675,7 +680,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>>         if (!delete && !rename && !edit_description && !new_upstream && !unset_upstream && argc == 0)
>>                 list = 1;
>>
>> -       if (filter.with_commit || filter.merge != REF_FILTER_MERGED_NONE)
>> +       if (filter.with_commit || filter.merge != REF_FILTER_MERGED_NONE || filter.points_at.nr)
>>                 list = 1;
>>
>>         if (!!delete + !!rename + !!new_upstream +
>> diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
>> index 38c68bd..1deb7cb 100755
>> --- a/t/t3203-branch-output.sh
>> +++ b/t/t3203-branch-output.sh
>> @@ -154,4 +154,13 @@ EOF
>>         test_i18ncmp expect actual
>>  '
>>
>> +test_expect_success 'git branch --points-at option' '
>> +       cat >expect <<EOF &&
>> +  master
>> +  branch-one
>> +EOF
>> +       git branch --points-at=branch-one >actual &&
>> +       test_cmp expect actual
>> +'
>> +
>>  test_done
>> --
>> 2.4.6
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe git" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Copy paste, errors, thanks for pointing out.

-- 
Regards,
Karthik Nayak

Re: [RFC/PATCH 10/11] branch.c: use 'ref-filter' APIs

[Matthieu Moy]

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

> On Tue, Jul 28, 2015 at 7:47 PM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>
>>> -     qsort(array.items, index, sizeof(struct ref_array_item *), ref_cmp);
>>> +     if (!sorting) {
>>> +             def_sorting.next = NULL;
>>> +             def_sorting.atom = parse_ref_filter_atom(sort_type,
>>> +                                                      sort_type + strlen(sort_type));
>>> +             sorting = &def_sorting;
>>> +     }
>>> +     ref_array_sort(sorting, &array);
>>
>> Does this belong to print_ref_list()? Is it not possible to extract it
>> to get a code closer to the simple:
>>
>>         filter_refs(...);
>>         ref_array_sort(...);
>>         print_ref_list(...);
>>
>> ?
>>
>
> We have a ref_defaulting_sorting but that defaults to sorting by 'refname'
> but what we want in branch.c is to sort by 'type' rather. Hence we
> need to have this small segment of code. About its placement, IDK if
> print_ref_list() is the right place. But didn't find a better place.

Hmm, actually I think I misread the code: print_ref_list() does follow
the pattern filter_refs -> sort -> print.

Perhaps the function name is misleading, and you could make it clearer
that it computes the list and displays it. I don't know.

>>> --- a/t/t1430-bad-ref-name.sh
>>> +++ b/t/t1430-bad-ref-name.sh
>>> @@ -38,7 +38,7 @@ test_expect_success 'fast-import: fail on invalid branch name "bad[branch]name"'
>>>       test_must_fail git fast-import <input
>>>  '
>>>
>>> -test_expect_success 'git branch shows badly named ref' '
>>> +test_expect_failure 'git branch does not show badly named ref' '
>>
>> I'm not sure what's the convention, but I think the test description
>> should give the expected behavior even with test_expect_failure.
>>
>> And please help the reviewers by saying what's the status wrt this test
>> (any plan on how to fix it?).
>>
>
> Well okay will rename the test description.
> Since we use filter_refs() there's no real fix for this, ref_filter_handler()
> skips over such refs.

Either there's a good rationale for ignoring these refs, and you should
change the test keeping it "test_expect_success" (i.e. "the test
observed corresponds to the expected behavior"), or you should
eventually modify filter_refs() to allow not dropping them.

This

-test_expect_success '...'
+test_expect_failure '...'

explicitly says "I have a known regression in this patch".

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

Re: [RFC/PATCH] Port branch.c to use ref-filter APIs

[Karthik Nayak]

On Tue, Jul 28, 2015 at 11:23 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> On Tue, Jul 28, 2015 at 7:05 PM, Matthieu Moy
>> <Matthieu.Moy@grenoble-inp.fr> wrote:
>>
>>> Ideally, you could also have a modifier atom %(alignleft) [...]
>>
>> This could work for refname, as each ref_array_item holds the refname,
>> But other atoms are only filled in after a call to
>> verify_ref_format().
>
> If you swap the order and call verify_ref_format() after filter_refs(),
> then you could detect %(alignleft) and iterate over the list as needed
> in verify_ref_format().
>
> (You would actually force swapping the order and you would need to adapt
> other callers in for-each-ref and tag)
>
>> But I don't see this as priority for now, so will mark it off for later.
>
> Absolutely, that's what I meant by "Ideally". I'm just thinking aloud.
>

Maybe after GSoC ;-)

-- 
Regards,
Karthik Nayak

Re: [RFC/PATCH 10/11] branch.c: use 'ref-filter' APIs

[Matthieu Moy]

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

> On Tue, Jul 28, 2015 at 7:47 PM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>>
>> I'm not sure what's the convention, but I think the test description
>> should give the expected behavior even with test_expect_failure.
>>
>> And please help the reviewers by saying what's the status wrt this test
>> (any plan on how to fix it?).
>>
>
> On the other hand I wonder if the test is even needed as, we don't
> really need it
> Cause we remove that ability of branch.c by using filter_refs().

Please read d0f810f (refs.c: allow listing and deleting badly named
refs, 2014-09-03). I think the reasoning makes sense, and we should keep
this ability.

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

Re: [RFC/PATCH 01/11] ref-filter: add "%(objectname:size=X)" option

[Karthik Nayak]

On Tue, Jul 28, 2015 at 9:49 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> From: Karthik Nayak <karthik.188@gmail.com>
>>
>> Add support for %(objectname:size=X) where X is a number.
>> This will print the first X characters of an objectname.
>> The minimum value for X is 5. Hence any value lesser than
>> 5 will default to 5 characters.
>
> Where does this hardcoded 5 come from?
>
> I'd agree that we would want some minimum for sanity (not safety),
> but I do not think we want random callers of find-unique-abbrev
> arbitrarily imposing their own minimum, making different codepaths
> behave inconsistently without a good reason.
>
> It seems that the minimum we use for sanity at the core level is
> MINIMUM_ABBREV.  Is there a reason why that value is inappropriate
> for this codepath?
>

I don't quite remember, This is definitely wrong. Thanks
Will use MINIMUM_ABBREV.

-- 
Regards,
Karthik Nayak

Re: [RFC/PATCH 04/11] ref-filter: add 'ifexists' atom

[Karthik Nayak]

On Tue, Jul 28, 2015 at 11:27 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> The 'ifexists' atom allows us to print a required format if the
>> preceeding atom has a value. If the preceeding atom has no value then
>> the format given is not printed. e.g. to print "[<refname>]" we can
>> now use the format "%(ifexists:[%s])%(refname)".
>
> A handful of "huh?" on the design.
>
>  - The atom says "if *exists*" and explanation says "has a value".
>    How are they related?  Does an atom whose value is an empty
>    string has a value?  Or is "ifexists" meant to be used only to
>    ignore meaningless atom, e.g. %(*objectname) applied to a ref that
>    refers to an object that is not an annotated tag?

It's meant to ignore meaningless atom. atom's whose values are empty
strings are ignored.

>
>  - That %s looks ugly.  Are there cases where a user may want to say
>    %(ifexists:[%i]) or something other than 's' after that per-cent?
>

Couldn't think of a better replacer, any suggestions would be welcome :)

>    . Is it allowed to have more than one %s there?
>    . Is it allowed to have no %s there?
>

1. yes its allowed to have multiple %s
2. yes no %s is also allowed.

>  - The syntax makes the reader wonder if [] is part of the
>    construct, or just an example of any arbitrary string, i.e. is
>    "%(ifexists:the %s can be part of arbitrary string)" valid?
>

Its given as example, is that misleading?

>  - If an arbitrary string is allowed, is there any quoting mechanism
>    to allow ")" to be part of that arbitrary string?

Nope. Haven't done anything for that. I'll look into that :)

>
>  - What, if anything, is allowed to come between %(ifexists...) and
>    the next atom like %(refname)?  For example, are these valid
>    constructs?
>
>     . %(ifexists...)%(padright:20)%(refname)

Doesn't work with padright, maybe we could eventually support that.

>     . %(ifexists...) %(refname) [%(subject)]
>

Not sure what this is.

>  - This syntax does not seem to allow switching on an attribute to
>    show or not to show another, e.g. "if %(*objectname) makes sense,
>    then show '%(padright:20)%(refname:short) %(*subject)' for it".

Yes this doesn't do that, I can say this is a pretty basic version, we could
probably work on and implement more things?
This is currently to support 'git branch -vv' where we have the upstream
in square brackets.

-- 
Regards,
Karthik Nayak

Re: [RFC/PATCH 06/11] branch: roll show_detached HEAD into regular ref_list

[Karthik Nayak]

On Wed, Jul 29, 2015 at 3:26 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> On Tue, Jul 28, 2015 at 6:31 PM, Matthieu Moy
>> <Matthieu.Moy@grenoble-inp.fr> wrote:
>>>
>>>> -static void show_detached(struct ref_list *ref_list, int maxwidth)
>>>> -{
>>>> -     struct commit *head_commit = lookup_commit_reference_gently(head_sha1, 1);
>>>> -
>>>> -     if (head_commit && is_descendant_of(head_commit, ref_list->with_commit)) {
>>>
>>> I'm not sure what this if was doing, and why you can get rid of it. My
>>> understanding is that with_commit comes from --contains, and in the
>>> previous code the filtering was done at display time (detached HEAD was
>>> not shown if it was not contained in commits specified with --contains).
>>>
>>> Eventually, you'll use ref-filter to do this filtering so you won't need
>>> this check at display time.
>>>
>>> But am I correct that for a few commits, you ignore --contains on
>>> detached HEAD?
>>>
>>
>> No we don't ignore --contains on detached HEAD.
>>
>> Since detached HEAD now gets its data from append_ref(). The function
>> also checks for the --contains option.
>
> Ah, OK. Previously, detached HEAD and branches were completely
> different, each having its own if (is_descendant_of(...)), and you're
> now using only one in append_ref() before removing it completely in
> favor of ref-filter.
>
> That would deserve an explanation for other reviewers I think.
>

Will include a small explanation in the commit message :)

-- 
Regards,
Karthik Nayak

Re: [RFC/PATCH 04/11] ref-filter: add 'ifexists' atom

[Junio C Hamano]

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

>> A handful of "huh?" on the design.
>>
>>  - The atom says "if *exists*" and explanation says "has a value".
>>    How are they related?  Does an atom whose value is an empty
>>    string has a value?  Or is "ifexists" meant to be used only to
>>    ignore meaningless atom, e.g. %(*objectname) applied to a ref that
>>    refers to an object that is not an annotated tag?
>
> It's meant to ignore meaningless atom. atom's whose values are empty
> strings are ignored.

That is a self-contradicting answer.

If you ask for "%(*objectname)" on a commit, that request truly is
meaningless, as a commit is not an annotated tag that points at another
object whose objectname is being asked for.

But if a commit has an empty log message (you should be able to
create such an object with commit-tree), then "%(subject)" would be
an empty string.  The fact that the commit happens to have an empty
string as its message is far from meaningless.

Either you ignore an empty string, or you ignore meaningless one.
Which does "ifexists" mean?

>>  - That %s looks ugly.  Are there cases where a user may want to say
>>    %(ifexists:[%i]) or something other than 's' after that per-cent?
>
> Couldn't think of a better replacer, any suggestions would be welcome :)

See below.

> Its given as example, is that misleading?

Othewise I wouldn't be asking.

>>  - What, if anything, is allowed to come between %(ifexists...) and
>>    the next atom like %(refname)?  For example, are these valid
>>    constructs?
>>
>>     . %(ifexists...)%(padright:20)%(refname)
>
> Doesn't work ...
> ...
>>  - This syntax does not seem to allow switching on an attribute to
>>    show or not to show another, e.g. "if %(*objectname) makes sense,
>>    then show '%(padright:20)%(refname:short) %(*subject)' for it".
>
> Yes this doesn't do that,

One way to do all of the above is to make it

    %(ifexists:atom:expansionString)

That is, for example:

 "%(ifexists:*objectname:tag %(color:blue)%(refname:short)%(color:reset))"

would give you a string "tag v1.0" with "v1.0" painted in blue for
refs/tags/v1.0 but nothing for refs/heads/master.

Obviously expansionString part needs some escaping mechanism to
allow it to include an unmatched ")".

Re: [RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer

[Karthik Nayak]

On Wed, Jul 29, 2015 at 6:16 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
> On Tue, Jul 28, 2015 at 1:12 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> On Tue, Jul 28, 2015 at 6:39 PM, Matthieu Moy
>> <Matthieu.Moy@grenoble-inp.fr> wrote:
>>> Karthik Nayak <karthik.188@gmail.com> writes:
>>>
>>>> We check if given ref is the current branch in print_ref_list().  Move
>>>> this check to print_ref_item() where it is checked right before
>>>> printing.
>>>
>>> This means that the '*' and the different color are coded in C, hence
>>> it's not possible to mimick this using "git for-each-ref --format ...".
>>>
>>> I do not consider this as blocking, but I think the ultimate goal should
>>> be to allow this, so that all the goodies of "git branch" can be made
>>> available to other ref-listing commands.
>>>
>>
>> Not sure what you mean here.
>>
>
> He means to make sure that any feature that was in tag, branch,
> for-each-ref, etc should be available as part of ref-filter and in all
> of them
>
> Maybe he misunderstood code or maybe you misunderstood the comment here?
>
> Regards,
> Jake

Ah, I didn't quite understand the first time. Thanks :)

-- 
Regards,
Karthik Nayak

Re: [RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer

[Karthik Nayak]

On Wed, Jul 29, 2015 at 3:31 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> On Tue, Jul 28, 2015 at 6:39 PM, Matthieu Moy
>> <Matthieu.Moy@grenoble-inp.fr> wrote:
>>> Karthik Nayak <karthik.188@gmail.com> writes:
>>>
>>>> We check if given ref is the current branch in print_ref_list().  Move
>>>> this check to print_ref_item() where it is checked right before
>>>> printing.
>>>
>>> This means that the '*' and the different color are coded in C, hence
>>> it's not possible to mimick this using "git for-each-ref --format ...".
>>>
>>> I do not consider this as blocking, but I think the ultimate goal should
>>> be to allow this, so that all the goodies of "git branch" can be made
>>> available to other ref-listing commands.
>>>
>>
>> Not sure what you mean here.
>
> What you already know, but probably badly explained ;-).
>
> Eventually, the output of "git branch" should correspond to a format
> string (so git branch would be almost an alias for "git for-each-ref
> refs/heads/ --format '...'"). Internally, this would mean using
> show_ref_array_item instead of print_ref_item. This is what you managed
> to do for "git tag".
>
> You already identified one difficulty with sha1 alignment in "git branch
> -v". I'm pointing out another which is that displaying the "*" in front
> of the current branch is currently not possible with a format string.
> You would need an atom like %(displayAStarIfTheBranchIsTheCurrentOne)
> (for which you'd need to find a short-and-sweet name ;-) ).
>

What I was thinking of was something like this :

struct strbuf format = STRBUF_INIT;
char c = ' ';
if (current)
    c = '*';
strbuf_addf(&format, "%c....", c, other format options...);
show_ref_array_item(item, format.buf, quote_style, 0);
strbuf_release(&format);

This would remove the need of making the printing of the "*" to be
needed by ref-filter. As this is only needed in branch.c

If going on what you're saying we could have a "%(starifcurrent)" atom or
something, but I don't see a general use for this.


-- 
Regards,
Karthik Nayak

Re: [RFC/PATCH 04/11] ref-filter: add 'ifexists' atom

[Junio C Hamano]

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

>> Couldn't think of a better replacer, any suggestions would be welcome :)
>
> See below.
> ...
> One way to do all of the above is ...

Note that is just "one way", not the only or not necessarily the
best.  It certainly is not the easiest, I think.

    %(if:atom)...%(endif)

might be easier to implement.

Re: [RFC/PATCH 09/11] branch.c: use 'ref-filter' data structures

[Karthik Nayak]

On Wed, Jul 29, 2015 at 3:38 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> On Tue, Jul 28, 2015 at 7:18 PM, Matthieu Moy
>> <Matthieu.Moy@grenoble-inp.fr> wrote:
>>> Christian Couder <christian.couder@gmail.com> writes:
>>>
>>>> On Tue, Jul 28, 2015 at 8:56 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>>>
>>>>> +static void ref_array_append(struct ref_array *array, const char *refname)
>>>>> +{
>>>>> +       size_t len = strlen(refname);
>>>>> +       struct ref_array_item *ref = xcalloc(1, sizeof(struct ref_array_item) + len + 1);
>>>>> +       memcpy(ref->refname, refname, len);
>>>>> +       ref->refname[len] = '\0';
>>>
>>> This looks very much like new_ref_array_item, except that the later also
>>> takes an objectname parameter. I find it suspicious that you leave the
>>> objectname field uninitialized.
>>>
>>
>> Well the objectname is not required here, the idea is to retain the way branch.c
>> works.
>>
>>> Why is this code not calling new_ref_array_item?
>>>
>>
>> Because no objectname is there.
>
> You do have the object_id in the scope of the call-site, so why not use
> it?
>
> (Well, in any case, do as you think is best, it's temporary throw-away
> code, we shouldn't loose too much time polishing it)
>

Also the fact the new_ref_array_item() is static and not shared. Wouldn't make
sense to make it an API only for a single commit, when the code itself
is temporary.

>>> The function disapears in the next commit, but I also think that this
>>> function deserves to exist in ref-filter.{c,h} and remain after the end
>>> of the series.
>>>
>>
>> Why though? I don't see the need when new_ref_array_item() is present.
>
> OK, if the function is specificly for "append an item but leave the
> objectname uninitialized", it's too specific to be useful somewhere
> else. But then, make it more explicit in the name of the function and/or
> in a comment.
>

Will add a comment :)

-- 
Regards,
Karthik Nayak

Re: [RFC/PATCH 02/11] ref-filter: add 'colornext' atom

[Eric Sunshine]

On Tuesday, July 28, 2015, Karthik Nayak <karthik.188@gmail.com> wrote:
> The 'colornext' atom allows us to color only the succeeding atom with
> a particular color. This resets any previous color preferences set. It
> is not compatible with `padright`. This is required for printing
> formats like `git branch -vv` where the upstream is colored in blue.

Can you explain here and in the commit message why %(colornext) is not
compatible with %(padright:)? Is it an implementation limitation, or a
syntax or semantic limitation, or what? Can the implementation be
fixed to make it compatible or does it require a redesign?

Also, please explain here and in the commit message why this highly
specialized colorizer ('colornext'), is needed even though a more
general purpose one ('color') is already available.

Re: [RFC/PATCH 04/11] ref-filter: add 'ifexists' atom

[Matthieu Moy]

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

> Junio C Hamano <gitster@pobox.com> writes:
>
>>> Couldn't think of a better replacer, any suggestions would be welcome :)
>>
>> See below.
>> ...
>> One way to do all of the above is ...
>
> Note that is just "one way", not the only or not necessarily the
> best.  It certainly is not the easiest, I think.
>
>     %(if:atom)...%(endif)
>
> might be easier to implement.

And I find it easier to read or write too. Nested parenthesis in a
format string make them really tricky. That removes the need for
escaping since the content of the if/endif is a format string like the
others, it can use the same escaping rules (IIRC, %% to escape a %).

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

Re: [RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer

[Matthieu Moy]

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

> What I was thinking of was something like this :
>
> struct strbuf format = STRBUF_INIT;
> char c = ' ';
> if (current)
>     c = '*';
> strbuf_addf(&format, "%c....", c, other format options...);
> show_ref_array_item(item, format.buf, quote_style, 0);
> strbuf_release(&format);

I think that would interact badly with verify_ref_format(). Usually, you
have just one format string and call verify_ref_format() on it, not a
different format string for each ref_array_item. That would probably be
solvable.

> This would remove the need of making the printing of the "*" to be
> needed by ref-filter. As this is only needed in branch.c
>
> If going on what you're saying we could have a "%(starifcurrent)" atom or
> something, but I don't see a general use for this.

To have a really customizeable format, you would want to allow e.g.

  git branch --format '%(starifcurrent) %(objectname) %(refname)'

if the user wants to get the sha1 before the refname, and still have the
star in front. It's a bit frustrating to have a hardcoded format that
the user can't reproduce with the --format option, since it means you
can't easily make a small variation on it.

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

Re: [RFC/PATCH 02/11] ref-filter: add 'colornext' atom

[Matthieu Moy]

Eric Sunshine <sunshine@sunshineco.com> writes:

> Also, please explain here and in the commit message why this highly
> specialized colorizer ('colornext'), is needed even though a more
> general purpose one ('color') is already available.

It is needed in the current form to allow
%(colornext:blue)%(ifexists:[%s]) to color only the replacement of %s
and not the [].

But I now think that this would be more elegantly solved by Junio's
%(if) %(endif) idea:

  %(if:atom) [ %(color:blue)%(atom)%(color:reset) ] %(endif)

(I added spaces for clarity)

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

Re: [RFC/PATCH 04/11] ref-filter: add 'ifexists' atom

[Junio C Hamano]

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

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>>> Couldn't think of a better replacer, any suggestions would be welcome :)
>>>
>>> See below.
>>> ...
>>> One way to do all of the above is ...
>>
>> Note that is just "one way", not the only or not necessarily the
>> best.  It certainly is not the easiest, I think.
>>
>>     %(if:atom)...%(endif)
>>
>> might be easier to implement.
>
> And I find it easier to read or write too. Nested parenthesis in a
> format string make them really tricky. That removes the need for
> escaping since the content of the if/endif is a format string like the
> others, it can use the same escaping rules (IIRC, %% to escape a %).

Yeah, that is also a good point.

Re: [RFC/PATCH 02/11] ref-filter: add 'colornext' atom

[Jacob Keller]

On Wed, Jul 29, 2015 at 2:30 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> Also, please explain here and in the commit message why this highly
>> specialized colorizer ('colornext'), is needed even though a more
>> general purpose one ('color') is already available.
>
> It is needed in the current form to allow
> %(colornext:blue)%(ifexists:[%s]) to color only the replacement of %s
> and not the [].
>
> But I now think that this would be more elegantly solved by Junio's
> %(if) %(endif) idea:
>
>   %(if:atom) [ %(color:blue)%(atom)%(color:reset) ] %(endif)
>
> (I added spaces for clarity)
>

I agree, this style seems a lot more elegant and expressive while much
easier to understand. Same for doing this to the alignment atoms and
such as it solves the same problem(s) there.

I can't speak to how easy it is to implement tho.

Regards,
Jake

Re: [RFC/PATCH 10/11] branch.c: use 'ref-filter' APIs

[Karthik Nayak]

On Wed, Jul 29, 2015 at 9:26 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> On Tue, Jul 28, 2015 at 7:47 PM, Matthieu Moy
>> <Matthieu.Moy@grenoble-inp.fr> wrote:
>>>
>>> I'm not sure what's the convention, but I think the test description
>>> should give the expected behavior even with test_expect_failure.
>>>
>>> And please help the reviewers by saying what's the status wrt this test
>>> (any plan on how to fix it?).
>>>
>>
>> On the other hand I wonder if the test is even needed as, we don't
>> really need it
>> Cause we remove that ability of branch.c by using filter_refs().
>
> Please read d0f810f (refs.c: allow listing and deleting badly named
> refs, 2014-09-03). I think the reasoning makes sense, and we should keep
> this ability.
>

This makes sense, I didn't have a thorough look at this but it breaks
a little in
ref-filter.c while getting object attributes. So is it okay to mark
this as TODO?

-- 
Regards,
Karthik Nayak

Re: [RFC/PATCH 10/11] branch.c: use 'ref-filter' APIs

[Matthieu Moy]

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

> On Wed, Jul 29, 2015 at 9:26 PM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>
>>> On Tue, Jul 28, 2015 at 7:47 PM, Matthieu Moy
>>> <Matthieu.Moy@grenoble-inp.fr> wrote:
>>>>
>>>> I'm not sure what's the convention, but I think the test description
>>>> should give the expected behavior even with test_expect_failure.
>>>>
>>>> And please help the reviewers by saying what's the status wrt this test
>>>> (any plan on how to fix it?).
>>>>
>>>
>>> On the other hand I wonder if the test is even needed as, we don't
>>> really need it
>>> Cause we remove that ability of branch.c by using filter_refs().
>>
>> Please read d0f810f (refs.c: allow listing and deleting badly named
>> refs, 2014-09-03). I think the reasoning makes sense, and we should keep
>> this ability.
>>
>
> This makes sense, I didn't have a thorough look at this but it breaks
> a little in
> ref-filter.c while getting object attributes. So is it okay to mark
> this as TODO?

Solving this doesn't seem much harder than

diff --git a/ref-filter.c b/ref-filter.c
index 6c0189f..a4df287 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1117,7 +1117,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
 	struct commit *commit = NULL;
 	unsigned int kind;
 
-	if (flag & REF_BAD_NAME) {
+	if (!filter->show_bad_name_refs && (flag & REF_BAD_NAME)) {
 		  warning("ignoring ref with broken name %s", refname);
 		  return 0;
 	}
diff --git a/ref-filter.h b/ref-filter.h
index 98ebd3b..b9d2bbc 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -79,7 +79,7 @@ struct ref_filter {
 		match_as_path : 1;
 	unsigned int lines, branch_kind;
 	int abbrev, verbose;
-	int detached : 1;
+	int detached : 1, show_bad_name_refs : 1;
 };
 
 struct ref_filter_cbdata {

and setting filter->show_bad_name_refs when needed (untested). Did I
miss something?

IIRC, historicaly Git allowed some weirdly named refs which made some
commands ambiguous (e.g. a branch named after an option like '-d').
We're forbidding their creation so people shouldn't have any, but we
it's important to continue showing them in case some people have old
bad-named branches lying around.

I'd rather have the code strictly better after your contribution than
before.

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

Re: [RFC/PATCH 02/11] ref-filter: add 'colornext' atom

[Junio C Hamano]

Jacob Keller <jacob.keller@gmail.com> writes:

> On Wed, Jul 29, 2015 at 2:30 PM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>> Eric Sunshine <sunshine@sunshineco.com> writes:
>>
>>> Also, please explain here and in the commit message why this highly
>>> specialized colorizer ('colornext'), is needed even though a more
>>> general purpose one ('color') is already available.
>>
>> It is needed in the current form to allow
>> %(colornext:blue)%(ifexists:[%s]) to color only the replacement of %s
>> and not the [].
>>
>> But I now think that this would be more elegantly solved by Junio's
>> %(if) %(endif) idea:
>>
>>   %(if:atom) [ %(color:blue)%(atom)%(color:reset) ] %(endif)
>>
>> (I added spaces for clarity)
>
> I agree, this style seems a lot more elegant and expressive while much
> easier to understand. Same for doing this to the alignment atoms and
> such as it solves the same problem(s) there.

Do you mean something like these?

    %(align:left,20) branch %(refname:short) %(end)
    %(align:middle,20) branch %(refname:short) %(end)
    %(align:right,20) branch %(refname:short) %(end)

to replace and enhance %(padright:20)?

> I can't speak to how easy it is to implement tho.

Perhaps it would go like this:

 * Instead of always emitting to the standard output, emit() and
   print_value() will gain an option to append into a strbuf that is
   passed as argument.  Alternatively, appending to strbuf could be
   made the only output channel for them; show_ref_array_item() can
   prepare an empty strbuf, call them repeatedly to fill it, and
   then print the resulting strbuf itself.

 * Things like %(if) and %(align) would do the following:

   (1) Push the currently active strbuf it got from the calling
       show_ref_array_item() to the formatting state;

   (2) Create a new strbuf and arrange so that further output would
       be diverted to this new one; and

   (3) Push the fact that the diverted output will be processed by
       them (i.e. %(if), %(align), etc.) when the diversion finishes
       to the formatting state.

 * When %(end) is seen, the currently active strbuf (i.e. the new
   one created in (2) above for diversion) holds the output made
   since the previously seen %(if), %(align), etc.  The formatting
   state knows what processing needs to be performed on that from
   (3) above.

   - Pop the strbuf where the output of the entire %(if)...%(end)
     construct needs to go from the formatting state;

   - Have the processing popped from (3) above, e.g. %(if:atom) or
     %(align:left,20), do whatever they need to do on the diverted
     output, and emit their result.

Both %(if) and the hypotherical %(align) can use this same
"diversion" mechanism.  And the above would properly nest,
e.g.

    %(align:middle,40)%(if:taggerdate)tag %(end)%(refname:short)%(end)

Re: [RFC/PATCH 04/11] ref-filter: add 'ifexists' atom

[Karthik Nayak]

On Wed, Jul 29, 2015 at 11:30 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>>> A handful of "huh?" on the design.
>>>
>>>  - The atom says "if *exists*" and explanation says "has a value".
>>>    How are they related?  Does an atom whose value is an empty
>>>    string has a value?  Or is "ifexists" meant to be used only to
>>>    ignore meaningless atom, e.g. %(*objectname) applied to a ref that
>>>    refers to an object that is not an annotated tag?
>>
>> It's meant to ignore meaningless atom. atom's whose values are empty
>> strings are ignored.
>
> That is a self-contradicting answer.
>
> If you ask for "%(*objectname)" on a commit, that request truly is
> meaningless, as a commit is not an annotated tag that points at another
> object whose objectname is being asked for.
>
> But if a commit has an empty log message (you should be able to
> create such an object with commit-tree), then "%(subject)" would be
> an empty string.  The fact that the commit happens to have an empty
> string as its message is far from meaningless.
>
> Either you ignore an empty string, or you ignore meaningless one.
> Which does "ifexists" mean?

I meant ignore atom values which are empty, sorry for the confusion.

>
>>>  - That %s looks ugly.  Are there cases where a user may want to say
>>>    %(ifexists:[%i]) or something other than 's' after that per-cent?
>>
>> Couldn't think of a better replacer, any suggestions would be welcome :)
>
> See below.
>
>> Its given as example, is that misleading?
>
> Othewise I wouldn't be asking.
>
>>>  - What, if anything, is allowed to come between %(ifexists...) and
>>>    the next atom like %(refname)?  For example, are these valid
>>>    constructs?
>>>
>>>     . %(ifexists...)%(padright:20)%(refname)
>>
>> Doesn't work ...
>> ...
>>>  - This syntax does not seem to allow switching on an attribute to
>>>    show or not to show another, e.g. "if %(*objectname) makes sense,
>>>    then show '%(padright:20)%(refname:short) %(*subject)' for it".
>>
>> Yes this doesn't do that,
>
> One way to do all of the above is to make it
>
>     %(ifexists:atom:expansionString)
>
> That is, for example:
>
>  "%(ifexists:*objectname:tag %(color:blue)%(refname:short)%(color:reset))"
>
> would give you a string "tag v1.0" with "v1.0" painted in blue for
> refs/tags/v1.0 but nothing for refs/heads/master.
>
> Obviously expansionString part needs some escaping mechanism to
> allow it to include an unmatched ")".

I liked your other idea of if and endif better :)

-- 
Regards,
Karthik Nayak

Re: [RFC/PATCH 04/11] ref-filter: add 'ifexists' atom

[Karthik Nayak]

On Thu, Jul 30, 2015 at 2:51 AM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>>> Couldn't think of a better replacer, any suggestions would be welcome :)
>>>
>>> See below.
>>> ...
>>> One way to do all of the above is ...
>>
>> Note that is just "one way", not the only or not necessarily the
>> best.  It certainly is not the easiest, I think.
>>
>>     %(if:atom)...%(endif)
>>
>> might be easier to implement.
>
> And I find it easier to read or write too. Nested parenthesis in a
> format string make them really tricky. That removes the need for
> escaping since the content of the if/endif is a format string like the
> others, it can use the same escaping rules (IIRC, %% to escape a %).
>

THat's a really good point. Will work on this :)

-- 
Regards,
Karthik Nayak

Re: [RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer

[Karthik Nayak]

On Thu, Jul 30, 2015 at 2:57 AM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> What I was thinking of was something like this :
>>
>> struct strbuf format = STRBUF_INIT;
>> char c = ' ';
>> if (current)
>>     c = '*';
>> strbuf_addf(&format, "%c....", c, other format options...);
>> show_ref_array_item(item, format.buf, quote_style, 0);
>> strbuf_release(&format);
>
> I think that would interact badly with verify_ref_format(). Usually, you
> have just one format string and call verify_ref_format() on it, not a
> different format string for each ref_array_item. That would probably be
> solvable.
>

Good point! I just was wondering if we need another atom just to print a star.
But your points certainly are valid. I'll implement this. Thanks :)

>> This would remove the need of making the printing of the "*" to be
>> needed by ref-filter. As this is only needed in branch.c
>>
>> If going on what you're saying we could have a "%(starifcurrent)" atom or
>> something, but I don't see a general use for this.
>
> To have a really customizeable format, you would want to allow e.g.
>
>   git branch --format '%(starifcurrent) %(objectname) %(refname)'
>
> if the user wants to get the sha1 before the refname, and still have the
> star in front. It's a bit frustrating to have a hardcoded format that
> the user can't reproduce with the --format option, since it means you
> can't easily make a small variation on it.
>

Agreed. will have a "starifcurrent" atom :)

-- 
Regards,
Karthik Nayak

Re: [RFC/PATCH 04/11] ref-filter: add 'ifexists' atom

[Jacob Keller]

On Fri, Jul 31, 2015 at 11:46 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> On Thu, Jul 30, 2015 at 2:51 AM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> Junio C Hamano <gitster@pobox.com> writes:
>>>
>>>>> Couldn't think of a better replacer, any suggestions would be welcome :)
>>>>
>>>> See below.
>>>> ...
>>>> One way to do all of the above is ...
>>>
>>> Note that is just "one way", not the only or not necessarily the
>>> best.  It certainly is not the easiest, I think.
>>>
>>>     %(if:atom)...%(endif)
>>>
>>> might be easier to implement.
>>
>> And I find it easier to read or write too. Nested parenthesis in a
>> format string make them really tricky. That removes the need for
>> escaping since the content of the if/endif is a format string like the
>> others, it can use the same escaping rules (IIRC, %% to escape a %).
>>
>
> THat's a really good point. Will work on this :)
>
> --
> Regards,
> Karthik Nayak
> --


Not sure how much work it would take to extent other atoms to this
behavior, such as %(padright) and %(align) and such, that way they
could operate on literals and so forth.

Maybe not worth it as part of this GSoC project, as it may be too
complicated, but maybe something to mark as a "TODO" for future or
something?

The only issue being that it might mean we have to keep the old
implementation too for backwards compatibility .. Maybe it's easier to
implement that I think, or maybe it's far more challenging than I
think....

Regards,
Jake

Re: [RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer

[Jacob Keller]

On Fri, Jul 31, 2015 at 11:48 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> On Thu, Jul 30, 2015 at 2:57 AM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>
>>> What I was thinking of was something like this :
>>>
>>> struct strbuf format = STRBUF_INIT;
>>> char c = ' ';
>>> if (current)
>>>     c = '*';
>>> strbuf_addf(&format, "%c....", c, other format options...);
>>> show_ref_array_item(item, format.buf, quote_style, 0);
>>> strbuf_release(&format);
>>
>> I think that would interact badly with verify_ref_format(). Usually, you
>> have just one format string and call verify_ref_format() on it, not a
>> different format string for each ref_array_item. That would probably be
>> solvable.
>>
>
> Good point! I just was wondering if we need another atom just to print a star.
> But your points certainly are valid. I'll implement this. Thanks :)
>
>>> This would remove the need of making the printing of the "*" to be
>>> needed by ref-filter. As this is only needed in branch.c
>>>
>>> If going on what you're saying we could have a "%(starifcurrent)" atom or
>>> something, but I don't see a general use for this.
>>
>> To have a really customizeable format, you would want to allow e.g.
>>
>>   git branch --format '%(starifcurrent) %(objectname) %(refname)'
>>
>> if the user wants to get the sha1 before the refname, and still have the
>> star in front. It's a bit frustrating to have a hardcoded format that
>> the user can't reproduce with the --format option, since it means you
>> can't easily make a small variation on it.
>>
>
> Agreed. will have a "starifcurrent" atom :)
>
> --

Wonder if there is some sort of more "generic" atom that would work? I
can't think of anything obvious at all though... it may be worth
having this even though it definitely seems less useful generically,
as the reason above where --format should be at least as expressive as
the builtin formatting.

Regards,
Jake

Re: [RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer

[Eric Sunshine]

On Sat, Aug 1, 2015 at 2:48 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> On Thu, Jul 30, 2015 at 2:57 AM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
> Good point! I just was wondering if we need another atom just to print a star.
> But your points certainly are valid. I'll implement this. Thanks :)
>
>>> This would remove the need of making the printing of the "*" to be
>>> needed by ref-filter. As this is only needed in branch.c
>>>
>>> If going on what you're saying we could have a "%(starifcurrent)" atom or
>>> something, but I don't see a general use for this.
>>
>> To have a really customizeable format, you would want to allow e.g.
>>
>>   git branch --format '%(starifcurrent) %(objectname) %(refname)'
>>
>> if the user wants to get the sha1 before the refname, and still have the
>> star in front. It's a bit frustrating to have a hardcoded format that
>> the user can't reproduce with the --format option, since it means you
>> can't easily make a small variation on it.
>
> Agreed. will have a "starifcurrent" atom :)

Please don't. It's better to avoid these highly specialized solutions
and instead seek general purpose ones. For instance, utilizing the
%(if:) atom suggested elsewhere, you might add an %(iscurrentbranch)
which would allow a format like this:

    %(if:iscurrentbranch)*%(endif)

Even more generic would be an %(ifeq:x:y) conditional and a
%(currentbranch) atom:

    %(ifeq:refname:currentbranch)*%(endif)

Those are just a couple ideas. Other variations are possible and
likely preferable to the specialized %(starifcurrent).

Re: [RFC/PATCH 02/11] ref-filter: add 'colornext' atom

[Karthik Nayak]

On Thu, Jul 30, 2015 at 9:47 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jacob Keller <jacob.keller@gmail.com> writes:
>
>> On Wed, Jul 29, 2015 at 2:30 PM, Matthieu Moy
>> <Matthieu.Moy@grenoble-inp.fr> wrote:
>>> Eric Sunshine <sunshine@sunshineco.com> writes:
>>>
>>>> Also, please explain here and in the commit message why this highly
>>>> specialized colorizer ('colornext'), is needed even though a more
>>>> general purpose one ('color') is already available.
>>>
>>> It is needed in the current form to allow
>>> %(colornext:blue)%(ifexists:[%s]) to color only the replacement of %s
>>> and not the [].
>>>
>>> But I now think that this would be more elegantly solved by Junio's
>>> %(if) %(endif) idea:
>>>
>>>   %(if:atom) [ %(color:blue)%(atom)%(color:reset) ] %(endif)
>>>
>>> (I added spaces for clarity)
>>
>> I agree, this style seems a lot more elegant and expressive while much
>> easier to understand. Same for doing this to the alignment atoms and
>> such as it solves the same problem(s) there.
>
> Do you mean something like these?
>
>     %(align:left,20) branch %(refname:short) %(end)
>     %(align:middle,20) branch %(refname:short) %(end)
>     %(align:right,20) branch %(refname:short) %(end)
>
> to replace and enhance %(padright:20)?
>
>> I can't speak to how easy it is to implement tho.
>
> Perhaps it would go like this:
>
>  * Instead of always emitting to the standard output, emit() and
>    print_value() will gain an option to append into a strbuf that is
>    passed as argument.  Alternatively, appending to strbuf could be
>    made the only output channel for them; show_ref_array_item() can
>    prepare an empty strbuf, call them repeatedly to fill it, and
>    then print the resulting strbuf itself.
>
>  * Things like %(if) and %(align) would do the following:
>
>    (1) Push the currently active strbuf it got from the calling
>        show_ref_array_item() to the formatting state;
>
>    (2) Create a new strbuf and arrange so that further output would
>        be diverted to this new one; and
>
>    (3) Push the fact that the diverted output will be processed by
>        them (i.e. %(if), %(align), etc.) when the diversion finishes
>        to the formatting state.
>
>  * When %(end) is seen, the currently active strbuf (i.e. the new
>    one created in (2) above for diversion) holds the output made
>    since the previously seen %(if), %(align), etc.  The formatting
>    state knows what processing needs to be performed on that from
>    (3) above.
>
>    - Pop the strbuf where the output of the entire %(if)...%(end)
>      construct needs to go from the formatting state;
>
>    - Have the processing popped from (3) above, e.g. %(if:atom) or
>      %(align:left,20), do whatever they need to do on the diverted
>      output, and emit their result.
>
> Both %(if) and the hypotherical %(align) can use this same
> "diversion" mechanism.  And the above would properly nest,
> e.g.
>
>     %(align:middle,40)%(if:taggerdate)tag %(end)%(refname:short)%(end)
>

This actually looks neat and good, I'll try implementing this :)

-- 
Regards,
Karthik Nayak

Re: [RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer

[Karthik Nayak]

On Sat, Aug 1, 2015 at 2:33 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Sat, Aug 1, 2015 at 2:48 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> On Thu, Jul 30, 2015 at 2:57 AM, Matthieu Moy
>> <Matthieu.Moy@grenoble-inp.fr> wrote:
>> Good point! I just was wondering if we need another atom just to print a star.
>> But your points certainly are valid. I'll implement this. Thanks :)
>>
>>>> This would remove the need of making the printing of the "*" to be
>>>> needed by ref-filter. As this is only needed in branch.c
>>>>
>>>> If going on what you're saying we could have a "%(starifcurrent)" atom or
>>>> something, but I don't see a general use for this.
>>>
>>> To have a really customizeable format, you would want to allow e.g.
>>>
>>>   git branch --format '%(starifcurrent) %(objectname) %(refname)'
>>>
>>> if the user wants to get the sha1 before the refname, and still have the
>>> star in front. It's a bit frustrating to have a hardcoded format that
>>> the user can't reproduce with the --format option, since it means you
>>> can't easily make a small variation on it.
>>
>> Agreed. will have a "starifcurrent" atom :)
>
> Please don't. It's better to avoid these highly specialized solutions
> and instead seek general purpose ones. For instance, utilizing the
> %(if:) atom suggested elsewhere, you might add an %(iscurrentbranch)
> which would allow a format like this:
>
>     %(if:iscurrentbranch)*%(endif)
>
> Even more generic would be an %(ifeq:x:y) conditional and a
> %(currentbranch) atom:
>
>     %(ifeq:refname:currentbranch)*%(endif)
>
> Those are just a couple ideas. Other variations are possible and
> likely preferable to the specialized %(starifcurrent).

This makes sense, thanks. But implementing something like
"%(if:<atom>)" seems to not be as easy as I thought it would be.

First we need to parse that inner atom, but the used_atom_cnt is based
on how many atoms there are initially, which doesn't count this inner atom.

Although we could have a way around that, we'd need to again call populate_value
from itself to get that inner atom's value. This causes more problems.
Either ways
I'm looking at ways around this.
A simple solution would be to do :

%(if)%(atom)%(then).....%(endif)

or just

%(if)%(atom).....%(endif)

-- 
Regards,
Karthik Nayak

Re: [RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer

[Junio C Hamano]

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

>> Even more generic would be an %(ifeq:x:y) conditional and a
>> %(currentbranch) atom:
>>
>>     %(ifeq:refname:currentbranch)*%(endif)
>>
>> Those are just a couple ideas. Other variations are possible and
>> likely preferable to the specialized %(starifcurrent).
>
> This makes sense, thanks. But implementing something like
> "%(if:<atom>)" seems to not be as easy as I thought it would be.
>
> First we need to parse that inner atom, but the used_atom_cnt is based
> on how many atoms there are initially, which doesn't count this inner atom.
>
> Although we could have a way around that, we'd need to again call populate_value
> from itself to get that inner atom's value. This causes more problems.
> Either ways
> I'm looking at ways around this.
> A simple solution would be to do :
>
> %(if)%(atom)%(then).....%(endif)
>
> or just
>
> %(if)%(atom).....%(endif)

My knee-jerk reaction to the former was "Eeww, the users is forced
to keep verbosely typing unnecessarily '%(', ')%(then)' forever,
only because the implementor was too lazy to do the job properly in
parse_atom()".  I do not think the latter a good idea at all.

But I think the former is worth considering, as it will later allow
us to extend it to various forms, e.g.

    %(if:notempty)%(atom)%(then)...%(end)
    %(if)%(atom)%(then)...%(elif)%(atom1)%(atom2)%(then)...%(end)

Two points being

 (1) the default "string is not empty" does not have to be the only
     test criteria, by leaving the door to add %(if:<condition>)
     later;

 (2) what is tested does not have to be a single atom (that is why
     I do not think the one without %(then) good at all) but can be
     a string that can later be interpreted.

And syntactically, something like this even may want to be
introduced later:

    %(if:expr)master == %(refname:short)%(then)...%(end)

Re: [RFC/PATCH 10/11] branch.c: use 'ref-filter' APIs

[Karthik Nayak]

On Thu, Jul 30, 2015 at 12:59 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
> Solving this doesn't seem much harder than
>
> diff --git a/ref-filter.c b/ref-filter.c
> index 6c0189f..a4df287 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1117,7 +1117,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
>         struct commit *commit = NULL;
>         unsigned int kind;
>
> -       if (flag & REF_BAD_NAME) {
> +       if (!filter->show_bad_name_refs && (flag & REF_BAD_NAME)) {
>                   warning("ignoring ref with broken name %s", refname);
>                   return 0;
>         }
> diff --git a/ref-filter.h b/ref-filter.h
> index 98ebd3b..b9d2bbc 100644
> --- a/ref-filter.h
> +++ b/ref-filter.h
> @@ -79,7 +79,7 @@ struct ref_filter {
>                 match_as_path : 1;
>         unsigned int lines, branch_kind;
>         int abbrev, verbose;
> -       int detached : 1;
> +       int detached : 1, show_bad_name_refs : 1;
>  };
>
>  struct ref_filter_cbdata {
>
> and setting filter->show_bad_name_refs when needed (untested). Did I
> miss something?
>

This allows it to be added to ref_array. But it would still fail while
trying to obtain
object details prior to printing.

> IIRC, historicaly Git allowed some weirdly named refs which made some
> commands ambiguous (e.g. a branch named after an option like '-d').
> We're forbidding their creation so people shouldn't have any, but we
> it's important to continue showing them in case some people have old
> bad-named branches lying around.
>
> I'd rather have the code strictly better after your contribution than
> before.
>

Agreed. But then again the warning tells about the broken ref, as in it's name
So I think It's ok?

for e.g. t1430 :
[trash directory.t1430-bad-ref-name] ../../git branch
warning: ignoring ref with broken name refs/heads/broken...ref
* master


-- 
Regards,
Karthik Nayak

Re: [RFC/PATCH 10/11] branch.c: use 'ref-filter' APIs

[Matthieu Moy]

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

> On Thu, Jul 30, 2015 at 12:59 PM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>
>> IIRC, historicaly Git allowed some weirdly named refs which made some
>> commands ambiguous (e.g. a branch named after an option like '-d').
>> We're forbidding their creation so people shouldn't have any, but we
>> it's important to continue showing them in case some people have old
>> bad-named branches lying around.
[...]
> Agreed. But then again the warning tells about the broken ref, as in it's name
> So I think It's ok?
>
> for e.g. t1430 :
> [trash directory.t1430-bad-ref-name] ../../git branch
> warning: ignoring ref with broken name refs/heads/broken...ref
> * master

[ Late answer, I'm still catching up with my holiday's emails ;-) ]

OK, the warning gives a different interface but it seems as good as the
old one.

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

Re: [RFC/PATCH 10/11] branch.c: use 'ref-filter' APIs

[Karthik Nayak]

On Wed, Aug 19, 2015 at 9:19 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> On Thu, Jul 30, 2015 at 12:59 PM, Matthieu Moy
>> <Matthieu.Moy@grenoble-inp.fr> wrote:
>>
>>> IIRC, historicaly Git allowed some weirdly named refs which made some
>>> commands ambiguous (e.g. a branch named after an option like '-d').
>>> We're forbidding their creation so people shouldn't have any, but we
>>> it's important to continue showing them in case some people have old
>>> bad-named branches lying around.
> [...]
>> Agreed. But then again the warning tells about the broken ref, as in it's name
>> So I think It's ok?
>>
>> for e.g. t1430 :
>> [trash directory.t1430-bad-ref-name] ../../git branch
>> warning: ignoring ref with broken name refs/heads/broken...ref
>> * master
>
> [ Late answer, I'm still catching up with my holiday's emails ;-) ]
>
> OK, the warning gives a different interface but it seems as good as the
> old one.
>

Ah! yes :)

-- 
Regards,
Karthik Nayak