[PATCH v15 00/13] port builtin/tag.c to use ref-filter APIs.

[PATCH v15 00/13] port builtin/tag.c to use ref-filter APIs.

[Karthik Nayak]

The previous iteration of this series is found here:
http://article.gmane.org/gmane.comp.version-control.git/276779

Changes in this version:
* Make %(contents:lines=X) use existing ref-filter code rather
than reply completely on the code borrowed from tag.c.
* Make struct align and struct contents static variables inside
atom_value and put them inside an union.
* Since there was some redundant code movement, I moved most of
the code to the top. (Caused a noisy interdiff but should be more
benifital in the long run).
* Move the error checking of `align` into the block for processing
the align atom.
* Remove introduction of format_ref_array_item(), probably re-introduce
it if/when required.
* More tests added for %(contents:lines=X) and made them cleaner to read.
* Only the second element of the stack performs quoting at the end, all
elements after it perform no quoting themselves and rely on the quoting
performed by element two.

Karthik Nayak (13):
  ref-filter: move `struct atom_value` to ref-filter.c
  ref-filter: introduce ref_formatting_state and ref_formatting_stack
  utf8: add function to align a string into given strbuf
  ref-filter: introduce handler function for each atom
  ref-filter: implement an `align` atom
  ref-filter: add option to filter out tags, branches and remotes
  ref-filter: add support for %(contents:lines=X)
  ref-filter: add support to sort by version
  ref-filter: add option to match literal pattern
  tag.c: use 'ref-filter' data structures
  tag.c: use 'ref-filter' APIs
  tag.c: implement '--format' option
  tag.c: implement '--merged' and '--no-merged' options

 Documentation/git-for-each-ref.txt |  15 +-
 Documentation/git-tag.txt          |  27 ++-
 builtin/for-each-ref.c             |   1 +
 builtin/tag.c                      | 368 +++++++----------------------------
 ref-filter.c                       | 382 ++++++++++++++++++++++++++++++++-----
 ref-filter.h                       |  25 ++-
 refs.c                             |   9 +
 refs.h                             |   1 +
 t/t6302-for-each-ref-filter.sh     | 173 +++++++++++++++++
 t/t7004-tag.sh                     |  47 ++++-
 utf8.c                             |  21 ++
 utf8.h                             |  15 ++
 12 files changed, 710 insertions(+), 374 deletions(-)

Interdiff between v14 and v15:

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 1b48b95..d039f40 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -133,8 +133,8 @@ align::
 	`<position>` is either left, right or middle and `<width>` is
 	the total length of the content with alignment. If the
 	contents length is more than the width then no alignment is
-	performed. If used with '--quote' everything in between %(align:..)
-	and %(end) is quoted.
+	performed. If used with '--quote' everything in between
+	%(align:..)  and %(end) is quoted.
 
 In addition to the above, for commit and tag objects, the header
 field names (`tree`, `parent`, `object`, `type`, and `tag`) can
@@ -148,8 +148,8 @@ The complete message in a commit and tag object is `contents`.
 Its first line is `contents:subject`, where subject is the concatenation
 of all lines of the commit message up to the first blank line.  The next
 line is 'contents:body', where body is all of the lines after the first
-blank line.  Finally, the optional GPG signature is `contents:signature`.
-The first `N` lines of the object is obtained using `contents:lines=N`.
+blank line.  The optional GPG signature is `contents:signature`.  The
+first `N` lines of the message is obtained using `contents:lines=N`.
 
 For sorting purposes, fields with numeric values sort in numeric
 order (`objectsize`, `authordate`, `committerdate`, `taggerdate`).
diff --git a/ref-filter.c b/ref-filter.c
index f268cd7..b37d57a 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -61,6 +61,8 @@ static struct {
 	{ "contents:lines" },
 };
 
+#define REF_FORMATTING_STATE_INIT  { 0, NULL }
+
 struct align {
 	align_type position;
 	unsigned int width;
@@ -71,8 +73,6 @@ struct contents {
 	struct object_id oid;
 };
 
-#define REF_FORMATTING_STATE_INIT  { 0, NULL }
-
 struct ref_formatting_stack {
 	struct ref_formatting_stack *prev;
 	struct strbuf output;
@@ -87,8 +87,10 @@ struct ref_formatting_state {
 
 struct atom_value {
 	const char *s;
-	struct align *align;
-	struct contents *contents;
+	union {
+		struct align align;
+		struct contents contents;
+	} u;
 	void (*handler)(struct atom_value *atomv, struct ref_formatting_state *state);
 	unsigned long ul; /* used for sorting when not FIELD_STR */
 };
@@ -183,6 +185,30 @@ static void quote_formatting(struct strbuf *s, const char *str, int quote_style)
 	}
 }
 
+static void align_handler(struct ref_formatting_stack *stack)
+{
+	struct align *align = (struct align *)stack->cb_data;
+	struct strbuf s = STRBUF_INIT;
+
+	strbuf_utf8_align(&s, align->position, align->width, stack->output.buf);
+	strbuf_swap(&stack->output, &s);
+	strbuf_release(&s);
+}
+
+static void append_atom(struct atom_value *v, struct ref_formatting_state *state)
+{
+	/*
+	 * Quote formatting is only done when the stack has a single
+	 * element. Otherwise quote formatting is done on the
+	 * element's entire output strbuf when the %(end) atom is
+	 * encountered.
+	 */
+	if (!state->stack->prev)
+		quote_formatting(&state->stack->output, v->s, state->quote_style);
+	else
+		strbuf_addstr(&state->stack->output, v->s);
+}
+
 static void push_stack_element(struct ref_formatting_stack **stack)
 {
 	struct ref_formatting_stack *s = xcalloc(1, sizeof(struct ref_formatting_stack));
@@ -204,6 +230,37 @@ static void pop_stack_element(struct ref_formatting_stack **stack)
 	*stack = prev;
 }
 
+static void align_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
+{
+	struct ref_formatting_stack *new;
+
+	push_stack_element(&state->stack);
+	new = state->stack;
+	new->at_end = align_handler;
+	new->cb_data = &atomv->u.align;
+}
+
+static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
+{
+	struct ref_formatting_stack *current = state->stack;
+	struct strbuf s = STRBUF_INIT;
+
+	if (!current->at_end)
+		die(_("format: `end` atom used without a supporting atom"));
+	current->at_end(current);
+
+	/*
+	 * Perform quote formatting when the stack element is that of
+	 * a modifier atom and right above the first stack element.
+	 */
+	if (!state->stack->prev->prev) {
+		quote_formatting(&s, current->output.buf, state->quote_style);
+		strbuf_swap(&current->output, &s);
+	}
+	strbuf_release(&s);
+	pop_stack_element(&state->stack);
+}
+
 /*
  * In a format string, find the next occurrence of %(atom).
  */
@@ -580,34 +637,20 @@ static void find_subpos(const char *buf, unsigned long sz,
 
 /*
  * If 'lines' is greater than 0, append that many lines from the given
- * object_id 'oid' to the given strbuf.
+ * 'buf' of length 'size' to the given strbuf.
  */
-static void append_tag_lines(struct strbuf *out, const struct object_id *oid, int lines)
+static void append_lines(struct strbuf *out, const char *buf, unsigned long size, int lines)
 {
 	int i;
-	unsigned long size;
-	enum object_type type;
-	char *buf, *sp, *eol;
+	const char *sp, *eol;
 	size_t len;
 
-	buf = read_sha1_file(oid->hash, &type, &size);
-	if (!buf)
-		die_errno("unable to read object %s", oid_to_hex(oid));
-	if (type != OBJ_COMMIT && type != OBJ_TAG)
-		goto free_return;
-	if (!size)
-		die("an empty %s object %s?",
-		    typename(type), oid_to_hex(oid));
-
-	/* skip header */
-	sp = strstr(buf, "\n\n");
-	if (!sp)
-		goto free_return;
-
-	/* only take up to "lines" lines, and strip the signature from a tag */
-	if (type == OBJ_TAG)
-		size = parse_signature(buf, size);
-	for (i = 0, sp += 2; i < lines && sp < buf + size; i++) {
+	if ((sp = strstr(buf, "\n\n")) && (sp <= buf + size))
+		size += 2;
+
+	sp = buf;
+
+	for (i = 0; i < lines && sp < buf + size; i++) {
 		if (i)
 			strbuf_addstr(out, "\n    ");
 		eol = memchr(sp, '\n', size - (sp - buf));
@@ -617,20 +660,6 @@ static void append_tag_lines(struct strbuf *out, const struct object_id *oid, in
 			break;
 		sp = eol + 1;
 	}
-free_return:
-	free(buf);
-}
-
-static void contents_lines_handler(struct atom_value *atomv, struct ref_formatting_state *state)
-{
-	struct contents *contents = (struct contents *)atomv->contents;
-	struct strbuf s = STRBUF_INIT;
-
-	append_tag_lines(&s, &contents->oid, contents->lines);
-	quote_formatting(&state->stack->output, s.buf, state->quote_style);
-	strbuf_release(&s);
-
-	free(contents);
 }
 
 /* See grab_values */
@@ -675,13 +704,11 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj
 		else if (!strcmp(name, "contents"))
 			v->s = xstrdup(subpos);
 		else if (skip_prefix(name, "contents:lines=", &valp)) {
-			struct contents *contents = xmalloc(sizeof(struct contents));
-
-			if (strtoul_ui(valp, 10, &contents->lines))
-				die(_("positive width expected align:%s"), valp);
-			hashcpy(contents->oid.hash, obj->sha1);
-			v->handler = contents_lines_handler;
-			v->contents = contents;
+			struct strbuf s = STRBUF_INIT;
+			if (strtoul_ui(valp, 10, &v->u.contents.lines))
+				die(_("positive width expected contents:lines=%s"), valp);
+			append_lines(&s, subpos, sublen + bodylen - siglen, v->u.contents.lines);
+			v->s = strbuf_detach(&s, NULL);
 		}
 	}
 }
@@ -740,63 +767,6 @@ static inline char *copy_advance(char *dst, const char *src)
 	return dst;
 }
 
-static void align_handler(struct ref_formatting_stack *stack)
-{
-	struct align *align = (struct align *)stack->cb_data;
-	struct strbuf s = STRBUF_INIT;
-
-	strbuf_utf8_align(&s, align->position, align->width, stack->output.buf);
-	strbuf_swap(&stack->output, &s);
-	strbuf_release(&s);
-	free(align);
-}
-
-static void align_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
-{
-	struct ref_formatting_stack *new;
-
-	push_stack_element(&state->stack);
-	new = state->stack;
-	new->at_end = align_handler;
-	new->cb_data = atomv->align;
-}
-
-static void append_atom(struct atom_value *v, struct ref_formatting_state *state)
-{
-	/*
-	 * Quote formatting is only done when the stack has a single
-	 * element. Otherwise quote formatting is done on the
-	 * element's entire output strbuf when the %(end) atom is
-	 * encountered.
-	 */
-	if (!state->stack->prev)
-		quote_formatting(&state->stack->output, v->s, state->quote_style);
-	else
-		strbuf_addstr(&state->stack->output, v->s);
-}
-
-static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
-{
-	struct ref_formatting_stack *current = state->stack;
-	struct strbuf s = STRBUF_INIT;
-
-	if (!current->at_end)
-		die(_("format: `end` atom used without a supporting atom"));
-	current->at_end(current);
-	/*
-	 * Whenever we have more than one stack element that means we
-	 * are using a certain modifier atom. In that case we need to
-	 * perform quote formatting.
-	 */
-	if (state->stack->prev) {
-		quote_formatting(&s, current->output.buf, state->quote_style);
-		strbuf_reset(&current->output);
-		strbuf_addbuf(&current->output, &s);
-	}
-	strbuf_release(&s);
-	pop_stack_element(&state->stack);
-}
-
 /*
  * Parse the object referred by ref, and grab needed value.
  */
@@ -893,21 +863,27 @@ static void populate_value(struct ref_array_item *ref)
 			else
 				v->s = " ";
 			continue;
-		} else if (!strcmp(name, "align"))
-			die(_("format: incomplete use of the `align` atom"));
-		else if (skip_prefix(name, "align:", &valp)) {
-			struct align *align = xmalloc(sizeof(struct align));
-			struct strbuf **s = strbuf_split_str(valp, ',', 0);
+		} else if (skip_prefix(name, "align", &valp)) {
+			struct align *align = &v->u.align;
+			struct strbuf **s;
+
+			if (valp[0] != ':')
+				die(_("format: usage %%(align:<width>,<position>)"));
+			else
+				valp++;
+
+			s = strbuf_split_str(valp, ',', 0);
 
 			/* If the position is given trim the ',' from the first strbuf */
 			if (s[1])
-				strbuf_remove(s[0], s[0]->len - 1, 1);
+				strbuf_setlen(s[0], s[0]->len - 1);
 
 			if (strtoul_ui(s[0]->buf, 10, &align->width))
 				die(_("positive width expected align:%s"), s[0]->buf);
 
-			/* If no position is given, default to ALIGN_LEFT */
-			if (!s[1] || !strcmp(s[1]->buf, "left"))
+			if (!s[1])
+				align->position = ALIGN_LEFT;
+			else if (!strcmp(s[1]->buf, "left"))
 				align->position = ALIGN_LEFT;
 			else if (!strcmp(s[1]->buf, "right"))
 				align->position = ALIGN_RIGHT;
@@ -918,7 +894,6 @@ static void populate_value(struct ref_array_item *ref)
 
 			strbuf_list_free(s);
 
-			v->align = align;
 			v->handler = align_atom_handler;
 			continue;
 		} else if (!strcmp(name, "end")) {
@@ -1554,10 +1529,10 @@ static void append_literal(const char *cp, const char *ep, struct ref_formatting
 	}
 }
 
-void format_ref_array_item(struct strbuf *out, struct ref_array_item *info,
-			   const char *format, int quote_style)
+void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style)
 {
 	const char *cp, *sp, *ep;
+	struct strbuf *final_buf;
 	struct ref_formatting_state state = REF_FORMATTING_STATE_INIT;
 
 	state.quote_style = quote_style;
@@ -1587,17 +1562,10 @@ void format_ref_array_item(struct strbuf *out, struct ref_array_item *info,
 	}
 	if (state.stack->prev)
 		die(_("format: `end` atom missing"));
-	strbuf_addbuf(out, &state.stack->output);
+	final_buf = &state.stack->output;
+	fwrite(final_buf->buf, 1, final_buf->len, stdout);
 	pop_stack_element(&state.stack);
-}
-
-void show_ref_array_item(struct ref_array_item *item, const char *format, unsigned int quote_style)
-{
-	struct strbuf out = STRBUF_INIT;
-	format_ref_array_item(&out, item, format, quote_style);
-	fwrite(out.buf, out.len, 1, stdout);
-	printf("\n");
-	strbuf_release(&out);
+	putchar('\n');
 }
 
 /*  If no sorting option is given, use refname to sort as default */
diff --git a/ref-filter.h b/ref-filter.h
index 179944c..a5cfa5e 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -94,11 +94,8 @@ int parse_ref_filter_atom(const char *atom, const char *ep);
 int verify_ref_format(const char *format);
 /*  Sort the given ref_array as per the ref_sorting provided */
 void ref_array_sort(struct ref_sorting *sort, struct ref_array *array);
-/*  Format the ref as per given format and quote_style and store it into the strbuf */
-void format_ref_array_item(struct strbuf *out, struct ref_array_item *info,
-			   const char *format, int quote_style);
-/*  Wrapper around format_ref_array_item() which prints the given ref_array_item */
-void show_ref_array_item(struct ref_array_item *item, const char *format, unsigned int quote_style);
+/*  Print the ref using the given format and quote_style */
+void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style);
 /*  Callback function for parsing the sort option */
 int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset);
 /*  Default sort option based on refname */
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index 8f18f86..d95b781 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -166,22 +166,58 @@ test_expect_success 'nested alignment' '
 	test_cmp expect actual
 '
 
-test_expect_success 'check `%(contents:lines=X)`' '
+test_expect_success 'check `%(contents:lines=1)`' '
 	cat >expect <<-\EOF &&
-	master three
-	side four
-	odd/spot three
-	double-tag Annonated doubly
-	four four
-	one one
-	signed-tag A signed tag message
-	three three
-	two two
-	EOF
-	git for-each-ref --format="%(refname:short) %(contents:lines=1)" >actual &&
+	master |three
+	side |four
+	odd/spot |three
+	double-tag |Annonated doubly
+	four |four
+	one |one
+	signed-tag |A signed tag message
+	three |three
+	two |two
+	EOF
+	git for-each-ref --format="%(refname:short) |%(contents:lines=1)" >actual &&
 	test_cmp expect actual
 '
 
+test_expect_success 'check `%(contents:lines=0)`' '
+	cat >expect <<-\EOF &&
+	master |
+	side |
+	odd/spot |
+	double-tag |
+	four |
+	one |
+	signed-tag |
+	three |
+	two |
+	EOF
+	git for-each-ref --format="%(refname:short) |%(contents:lines=0)" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check `%(contents:lines=99999)`' '
+	cat >expect <<-\EOF &&
+	master |three
+	side |four
+	odd/spot |three
+	double-tag |Annonated doubly
+	four |four
+	one |one
+	signed-tag |A signed tag message
+	three |three
+	two |two
+	EOF
+	git for-each-ref --format="%(refname:short) |%(contents:lines=99999)" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '`%(contents:lines=-1)` should fail' '
+	test_must_fail git for-each-ref --format="%(refname:short) |%(contents:lines=-1)"
+'
+
 test_expect_success 'setup for version sort' '
 	test_commit foo1.3 &&
 	test_commit foo1.6 &&

-- 
2.5.0

[PATCH v15 01/13] ref-filter: move `struct atom_value` to ref-filter.c

[Karthik Nayak]

Since atom_value is only required for the internal working of
ref-filter it doesn't belong in the public header.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
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 | 5 +++++
 ref-filter.h | 5 +----
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 46963a5..e53c77e 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -55,6 +55,11 @@ static struct {
 	{ "color" },
 };
 
+struct atom_value {
+	const char *s;
+	unsigned long ul; /* used for sorting when not FIELD_STR */
+};
+
 /*
  * An atom is a valid field atom listed above, possibly prefixed with
  * a "*" to denote deref_tag().
diff --git a/ref-filter.h b/ref-filter.h
index 6bf27d8..45026d0 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -16,10 +16,7 @@
 #define FILTER_REFS_INCLUDE_BROKEN 0x1
 #define FILTER_REFS_ALL 0x2
 
-struct atom_value {
-	const char *s;
-	unsigned long ul; /* used for sorting when not FIELD_STR */
-};
+struct atom_value;
 
 struct ref_sorting {
 	struct ref_sorting *next;
-- 
2.5.0

[PATCH v15 02/13] ref-filter: introduce ref_formatting_state and ref_formatting_stack

[Karthik Nayak]

Introduce ref_formatting_state which will hold the formatted output
strbuf instead of directly printing to stdout. This will help us in
creating modifier atoms which modify the format specified before
printing to stdout.

Implement a stack machinery for ref_formatting_state, this allows us
to push and pop elements onto the stack. Whenever we pop an element
from the stack, the strbuf from that element is appended to the strbuf
of the next element on the stack, this will allow us to support
nesting of modifier atoms.

Rename some functions to reflect the changes made:
print_value() -> append_atom()
emit()        -> append_literal()

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 | 78 +++++++++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 59 insertions(+), 19 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index e53c77e..432cea0 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -55,6 +55,18 @@ static struct {
 	{ "color" },
 };
 
+#define REF_FORMATTING_STATE_INIT  { 0, NULL }
+
+struct ref_formatting_stack {
+	struct ref_formatting_stack *prev;
+	struct strbuf output;
+};
+
+struct ref_formatting_state {
+	int quote_style;
+	struct ref_formatting_stack *stack;
+};
+
 struct atom_value {
 	const char *s;
 	unsigned long ul; /* used for sorting when not FIELD_STR */
@@ -129,6 +141,27 @@ int parse_ref_filter_atom(const char *atom, const char *ep)
 	return at;
 }
 
+static void push_stack_element(struct ref_formatting_stack **stack)
+{
+	struct ref_formatting_stack *s = xcalloc(1, sizeof(struct ref_formatting_stack));
+
+	strbuf_init(&s->output, 0);
+	s->prev = *stack;
+	*stack = s;
+}
+
+static void pop_stack_element(struct ref_formatting_stack **stack)
+{
+	struct ref_formatting_stack *current = *stack;
+	struct ref_formatting_stack *prev = current->prev;
+
+	if (prev)
+		strbuf_addbuf(&prev->output, &current->output);
+	strbuf_release(&current->output);
+	free(current);
+	*stack = prev;
+}
+
 /*
  * In a format string, find the next occurrence of %(atom).
  */
@@ -1195,30 +1228,27 @@ void ref_array_sort(struct ref_sorting *sorting, struct ref_array *array)
 	qsort(array->items, array->nr, sizeof(struct ref_array_item *), compare_refs);
 }
 
-static void print_value(struct atom_value *v, int quote_style)
+static void append_atom(struct atom_value *v, struct ref_formatting_state *state)
 {
-	struct strbuf sb = STRBUF_INIT;
-	switch (quote_style) {
+	struct strbuf *s = &state->stack->output;
+
+	switch (state->quote_style) {
 	case QUOTE_NONE:
-		fputs(v->s, stdout);
+		strbuf_addstr(s, v->s);
 		break;
 	case QUOTE_SHELL:
-		sq_quote_buf(&sb, v->s);
+		sq_quote_buf(s, v->s);
 		break;
 	case QUOTE_PERL:
-		perl_quote_buf(&sb, v->s);
+		perl_quote_buf(s, v->s);
 		break;
 	case QUOTE_PYTHON:
-		python_quote_buf(&sb, v->s);
+		python_quote_buf(s, v->s);
 		break;
 	case QUOTE_TCL:
-		tcl_quote_buf(&sb, v->s);
+		tcl_quote_buf(s, v->s);
 		break;
 	}
-	if (quote_style != QUOTE_NONE) {
-		fputs(sb.buf, stdout);
-		strbuf_release(&sb);
-	}
 }
 
 static int hex1(char ch)
@@ -1239,8 +1269,10 @@ static int hex2(const char *cp)
 		return -1;
 }
 
-static void emit(const char *cp, const char *ep)
+static void append_literal(const char *cp, const char *ep, struct ref_formatting_state *state)
 {
+	struct strbuf *s = &state->stack->output;
+
 	while (*cp && (!ep || cp < ep)) {
 		if (*cp == '%') {
 			if (cp[1] == '%')
@@ -1248,13 +1280,13 @@ static void emit(const char *cp, const char *ep)
 			else {
 				int ch = hex2(cp + 1);
 				if (0 <= ch) {
-					putchar(ch);
+					strbuf_addch(s, ch);
 					cp += 3;
 					continue;
 				}
 			}
 		}
-		putchar(*cp);
+		strbuf_addch(s, *cp);
 		cp++;
 	}
 }
@@ -1262,19 +1294,24 @@ static void emit(const char *cp, const char *ep)
 void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style)
 {
 	const char *cp, *sp, *ep;
+	struct strbuf *final_buf;
+	struct ref_formatting_state state = REF_FORMATTING_STATE_INIT;
+
+	state.quote_style = quote_style;
+	push_stack_element(&state.stack);
 
 	for (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {
 		struct atom_value *atomv;
 
 		ep = strchr(sp, ')');
 		if (cp < sp)
-			emit(cp, sp);
+			append_literal(cp, sp, &state);
 		get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);
-		print_value(atomv, quote_style);
+		append_atom(atomv, &state);
 	}
 	if (*cp) {
 		sp = cp + strlen(cp);
-		emit(cp, sp);
+		append_literal(cp, sp, &state);
 	}
 	if (need_color_reset_at_eol) {
 		struct atom_value resetv;
@@ -1283,8 +1320,11 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu
 		if (color_parse("reset", color) < 0)
 			die("BUG: couldn't parse 'reset' as a color");
 		resetv.s = color;
-		print_value(&resetv, quote_style);
+		append_atom(&resetv, &state);
 	}
+	final_buf = &state.stack->output;
+	fwrite(final_buf->buf, 1, final_buf->len, stdout);
+	pop_stack_element(&state.stack);
 	putchar('\n');
 }
 
-- 
2.5.0

[PATCH v15 03/13] utf8: add function to align a string into given strbuf

[Karthik Nayak]

Add strbuf_utf8_align() which will align a given string into a strbuf
as per given align_type and width. If the width is greater than the
string length then no alignment is performed.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
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>
---
 utf8.c | 21 +++++++++++++++++++++
 utf8.h | 15 +++++++++++++++
 2 files changed, 36 insertions(+)

diff --git a/utf8.c b/utf8.c
index 28e6d76..00e10c8 100644
--- a/utf8.c
+++ b/utf8.c
@@ -644,3 +644,24 @@ int skip_utf8_bom(char **text, size_t len)
 	*text += strlen(utf8_bom);
 	return 1;
 }
+
+void strbuf_utf8_align(struct strbuf *buf, align_type position, unsigned int width,
+		       const char *s)
+{
+	int slen = strlen(s);
+	int display_len = utf8_strnwidth(s, slen, 0);
+	int utf8_compensation = slen - display_len;
+
+	if (display_len >= width) {
+		strbuf_addstr(buf, s);
+		return;
+	}
+
+	if (position == ALIGN_LEFT)
+		strbuf_addf(buf, "%-*s", width + utf8_compensation, s);
+	else if (position == ALIGN_MIDDLE) {
+		int left = (width - display_len) / 2;
+		strbuf_addf(buf, "%*s%-*s", left, "", width - left + utf8_compensation, s);
+	} else if (position == ALIGN_RIGHT)
+		strbuf_addf(buf, "%*s", width + utf8_compensation, s);
+}
diff --git a/utf8.h b/utf8.h
index 5a9e94b..7930b44 100644
--- a/utf8.h
+++ b/utf8.h
@@ -55,4 +55,19 @@ int mbs_chrlen(const char **text, size_t *remainder_p, const char *encoding);
  */
 int is_hfs_dotgit(const char *path);
 
+typedef enum {
+	ALIGN_LEFT,
+	ALIGN_MIDDLE,
+	ALIGN_RIGHT
+} align_type;
+
+/*
+ * Align the string given and store it into a strbuf as per the
+ * 'position' and 'width'. If the given string length is larger than
+ * 'width' than then the input string is not truncated and no
+ * alignment is done.
+ */
+void strbuf_utf8_align(struct strbuf *buf, align_type position, unsigned int width,
+		       const char *s);
+
 #endif
-- 
2.5.0

[PATCH v15 04/13] ref-filter: introduce handler function for each atom

[Karthik Nayak]

Introduce a handler function for each atom, which is called when the
atom is processed in show_ref_array_item().

In this context make append_atom() as the default handler function and
extract quote_formatting() out of append_atom(). Bump this to the top.

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 | 54 ++++++++++++++++++++++++++++++------------------------
 1 file changed, 30 insertions(+), 24 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 432cea0..a993216 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -69,6 +69,7 @@ struct ref_formatting_state {
 
 struct atom_value {
 	const char *s;
+	void (*handler)(struct atom_value *atomv, struct ref_formatting_state *state);
 	unsigned long ul; /* used for sorting when not FIELD_STR */
 };
 
@@ -141,6 +142,32 @@ int parse_ref_filter_atom(const char *atom, const char *ep)
 	return at;
 }
 
+static void quote_formatting(struct strbuf *s, const char *str, int quote_style)
+{
+	switch (quote_style) {
+	case QUOTE_NONE:
+		strbuf_addstr(s, str);
+		break;
+	case QUOTE_SHELL:
+		sq_quote_buf(s, str);
+		break;
+	case QUOTE_PERL:
+		perl_quote_buf(s, str);
+		break;
+	case QUOTE_PYTHON:
+		python_quote_buf(s, str);
+		break;
+	case QUOTE_TCL:
+		tcl_quote_buf(s, str);
+		break;
+	}
+}
+
+static void append_atom(struct atom_value *v, struct ref_formatting_state *state)
+{
+	quote_formatting(&state->stack->output, v->s, state->quote_style);
+}
+
 static void push_stack_element(struct ref_formatting_stack **stack)
 {
 	struct ref_formatting_stack *s = xcalloc(1, sizeof(struct ref_formatting_stack));
@@ -662,6 +689,8 @@ static void populate_value(struct ref_array_item *ref)
 		const char *formatp;
 		struct branch *branch = NULL;
 
+		v->handler = append_atom;
+
 		if (*name == '*') {
 			deref = 1;
 			name++;
@@ -1228,29 +1257,6 @@ void ref_array_sort(struct ref_sorting *sorting, struct ref_array *array)
 	qsort(array->items, array->nr, sizeof(struct ref_array_item *), compare_refs);
 }
 
-static void append_atom(struct atom_value *v, struct ref_formatting_state *state)
-{
-	struct strbuf *s = &state->stack->output;
-
-	switch (state->quote_style) {
-	case QUOTE_NONE:
-		strbuf_addstr(s, v->s);
-		break;
-	case QUOTE_SHELL:
-		sq_quote_buf(s, v->s);
-		break;
-	case QUOTE_PERL:
-		perl_quote_buf(s, v->s);
-		break;
-	case QUOTE_PYTHON:
-		python_quote_buf(s, v->s);
-		break;
-	case QUOTE_TCL:
-		tcl_quote_buf(s, v->s);
-		break;
-	}
-}
-
 static int hex1(char ch)
 {
 	if ('0' <= ch && ch <= '9')
@@ -1307,7 +1313,7 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu
 		if (cp < sp)
 			append_literal(cp, sp, &state);
 		get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);
-		append_atom(atomv, &state);
+		atomv->handler(atomv, &state);
 	}
 	if (*cp) {
 		sp = cp + strlen(cp);
-- 
2.5.0

[PATCH v15 05/13] ref-filter: implement an `align` atom

[Karthik Nayak]

Implement an `align` atom which left-, middle-, or right-aligns the
content between %(align:..) and %(end).

It is followed by `:<width>,<position>`, where the `<position>` is
either left, right or middle and `<width>` is the size of the area
into which the content will be placed. If the content between
%(align:) and %(end) is more than the width then no alignment is
performed. e.g. to align a refname atom to the middle with a total
width of 40 we can do: --format="%(align:middle,40)%(refname)%(end)".

We have an `at_end` function for each element of the stack which is to
be called when the `end` atom is encountered. Using this we implement
the aling_handler() for the `align` atom, this aligns the final strbuf
by calling `strbuf_utf8_align()` from utf8.c.

Ensure that quote formatting is performed on the whole of
%(align)...%(end) rather than individual atoms inside.  We skip quote
formatting for individual atoms when the current stack element is
handling an %(align) atom and perform quote formatting at the end when
we encounter the %(end) atom.

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-for-each-ref.txt |   9 ++++
 ref-filter.c                       | 102 ++++++++++++++++++++++++++++++++++++-
 t/t6302-for-each-ref-filter.sh     |  85 +++++++++++++++++++++++++++++++
 3 files changed, 195 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index e49d578..cac3128 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -127,6 +127,15 @@ color::
 	Change output color.  Followed by `:<colorname>`, where names
 	are described in `color.branch.*`.
 
+align::
+	Left-, middle-, or right-align the content between %(align:..)
+	and %(end). Followed by `:<width>,<position>`, where the
+	`<position>` is either left, right or middle and `<width>` is
+	the total length of the content with alignment. If the
+	contents length is more than the width then no alignment is
+	performed. If used with '--quote' everything in between
+	%(align:..)  and %(end) is quoted.
+
 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 a993216..3a5f0a7 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -10,6 +10,7 @@
 #include "quote.h"
 #include "ref-filter.h"
 #include "revision.h"
+#include "utf8.h"
 
 typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
 
@@ -53,13 +54,22 @@ static struct {
 	{ "flag" },
 	{ "HEAD" },
 	{ "color" },
+	{ "align" },
+	{ "end" },
 };
 
 #define REF_FORMATTING_STATE_INIT  { 0, NULL }
 
+struct align {
+	align_type position;
+	unsigned int width;
+};
+
 struct ref_formatting_stack {
 	struct ref_formatting_stack *prev;
 	struct strbuf output;
+	void (*at_end)(struct ref_formatting_stack *stack);
+	void *cb_data;
 };
 
 struct ref_formatting_state {
@@ -69,6 +79,7 @@ struct ref_formatting_state {
 
 struct atom_value {
 	const char *s;
+	struct align align;
 	void (*handler)(struct atom_value *atomv, struct ref_formatting_state *state);
 	unsigned long ul; /* used for sorting when not FIELD_STR */
 };
@@ -163,9 +174,28 @@ static void quote_formatting(struct strbuf *s, const char *str, int quote_style)
 	}
 }
 
+static void align_handler(struct ref_formatting_stack *stack)
+{
+	struct align *align = (struct align *)stack->cb_data;
+	struct strbuf s = STRBUF_INIT;
+
+	strbuf_utf8_align(&s, align->position, align->width, stack->output.buf);
+	strbuf_swap(&stack->output, &s);
+	strbuf_release(&s);
+}
+
 static void append_atom(struct atom_value *v, struct ref_formatting_state *state)
 {
-	quote_formatting(&state->stack->output, v->s, state->quote_style);
+	/*
+	 * Quote formatting is only done when the stack has a single
+	 * element. Otherwise quote formatting is done on the
+	 * element's entire output strbuf when the %(end) atom is
+	 * encountered.
+	 */
+	if (!state->stack->prev)
+		quote_formatting(&state->stack->output, v->s, state->quote_style);
+	else
+		strbuf_addstr(&state->stack->output, v->s);
 }
 
 static void push_stack_element(struct ref_formatting_stack **stack)
@@ -189,6 +219,37 @@ static void pop_stack_element(struct ref_formatting_stack **stack)
 	*stack = prev;
 }
 
+static void align_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
+{
+	struct ref_formatting_stack *new;
+
+	push_stack_element(&state->stack);
+	new = state->stack;
+	new->at_end = align_handler;
+	new->cb_data = &atomv->align;
+}
+
+static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
+{
+	struct ref_formatting_stack *current = state->stack;
+	struct strbuf s = STRBUF_INIT;
+
+	if (!current->at_end)
+		die(_("format: `end` atom used without a supporting atom"));
+	current->at_end(current);
+
+	/*
+	 * Perform quote formatting when the stack element is that of
+	 * a modifier atom and right above the first stack element.
+	 */
+	if (!state->stack->prev->prev) {
+		quote_formatting(&s, current->output.buf, state->quote_style);
+		strbuf_swap(&current->output, &s);
+	}
+	strbuf_release(&s);
+	pop_stack_element(&state->stack);
+}
+
 /*
  * In a format string, find the next occurrence of %(atom).
  */
@@ -687,6 +748,7 @@ static void populate_value(struct ref_array_item *ref)
 		int deref = 0;
 		const char *refname;
 		const char *formatp;
+		const char *valp;
 		struct branch *branch = NULL;
 
 		v->handler = append_atom;
@@ -754,6 +816,42 @@ static void populate_value(struct ref_array_item *ref)
 			else
 				v->s = " ";
 			continue;
+		} else if (skip_prefix(name, "align", &valp)) {
+			struct align *align = &v->align;
+			struct strbuf **s;
+
+			if (valp[0] != ':')
+				die(_("format: usage %%(align:<width>,<position>)"));
+			else
+				valp++;
+
+			s = strbuf_split_str(valp, ',', 0);
+
+			/* If the position is given trim the ',' from the first strbuf */
+			if (s[1])
+				strbuf_setlen(s[0], s[0]->len - 1);
+
+			if (strtoul_ui(s[0]->buf, 10, &align->width))
+				die(_("positive width expected align:%s"), s[0]->buf);
+
+			if (!s[1])
+				align->position = ALIGN_LEFT;
+			else if (!strcmp(s[1]->buf, "left"))
+				align->position = ALIGN_LEFT;
+			else if (!strcmp(s[1]->buf, "right"))
+				align->position = ALIGN_RIGHT;
+			else if (!strcmp(s[1]->buf, "middle"))
+				align->position = ALIGN_MIDDLE;
+			else
+				die(_("improper format entered align:%s"), s[1]->buf);
+
+			strbuf_list_free(s);
+
+			v->handler = align_atom_handler;
+			continue;
+		} else if (!strcmp(name, "end")) {
+			v->handler = end_atom_handler;
+			continue;
 		} else
 			continue;
 
@@ -1328,6 +1426,8 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu
 		resetv.s = color;
 		append_atom(&resetv, &state);
 	}
+	if (state.stack->prev)
+		die(_("format: `end` atom missing"));
 	final_buf = &state.stack->output;
 	fwrite(final_buf->buf, 1, final_buf->len, stdout);
 	pop_stack_element(&state.stack);
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index 505a360..cef7a41 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -81,4 +81,89 @@ test_expect_success 'filtering with --contains' '
 	test_cmp expect actual
 '
 
+test_expect_success 'left alignment' '
+	cat >expect <<-\EOF &&
+	refname is refs/heads/master  |refs/heads/master
+	refname is refs/heads/side    |refs/heads/side
+	refname is refs/odd/spot      |refs/odd/spot
+	refname is refs/tags/double-tag|refs/tags/double-tag
+	refname is refs/tags/four     |refs/tags/four
+	refname is refs/tags/one      |refs/tags/one
+	refname is refs/tags/signed-tag|refs/tags/signed-tag
+	refname is refs/tags/three    |refs/tags/three
+	refname is refs/tags/two      |refs/tags/two
+	EOF
+	git for-each-ref --format="%(align:30,left)refname is %(refname)%(end)|%(refname)" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'middle alignment' '
+	cat >expect <<-\EOF &&
+	| refname is refs/heads/master |refs/heads/master
+	|  refname is refs/heads/side  |refs/heads/side
+	|   refname is refs/odd/spot   |refs/odd/spot
+	|refname is refs/tags/double-tag|refs/tags/double-tag
+	|  refname is refs/tags/four   |refs/tags/four
+	|   refname is refs/tags/one   |refs/tags/one
+	|refname is refs/tags/signed-tag|refs/tags/signed-tag
+	|  refname is refs/tags/three  |refs/tags/three
+	|   refname is refs/tags/two   |refs/tags/two
+	EOF
+	git for-each-ref --format="|%(align:30,middle)refname is %(refname)%(end)|%(refname)" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'right alignment' '
+	cat >expect <<-\EOF &&
+	|  refname is refs/heads/master|refs/heads/master
+	|    refname is refs/heads/side|refs/heads/side
+	|      refname is refs/odd/spot|refs/odd/spot
+	|refname is refs/tags/double-tag|refs/tags/double-tag
+	|     refname is refs/tags/four|refs/tags/four
+	|      refname is refs/tags/one|refs/tags/one
+	|refname is refs/tags/signed-tag|refs/tags/signed-tag
+	|    refname is refs/tags/three|refs/tags/three
+	|      refname is refs/tags/two|refs/tags/two
+	EOF
+	git for-each-ref --format="|%(align:30,right)refname is %(refname)%(end)|%(refname)" >actual &&
+	test_cmp expect actual
+'
+
+# Everything in between the %(align)...%(end) atom must be quoted, hence we test this by
+# introducing single quote's in %(align)...%(end), which must not be escaped.
+
+sq="'"
+
+test_expect_success 'alignment with format quote' '
+	cat >expect <<-EOF &&
+	refname is ${sq}           ${sq}\\${sq}${sq}master${sq}\\${sq}${sq}           ${sq}|
+	refname is ${sq}            ${sq}\\${sq}${sq}side${sq}\\${sq}${sq}            ${sq}|
+	refname is ${sq}          ${sq}\\${sq}${sq}odd/spot${sq}\\${sq}${sq}          ${sq}|
+	refname is ${sq}         ${sq}\\${sq}${sq}double-tag${sq}\\${sq}${sq}         ${sq}|
+	refname is ${sq}            ${sq}\\${sq}${sq}four${sq}\\${sq}${sq}            ${sq}|
+	refname is ${sq}            ${sq}\\${sq}${sq}one${sq}\\${sq}${sq}             ${sq}|
+	refname is ${sq}         ${sq}\\${sq}${sq}signed-tag${sq}\\${sq}${sq}         ${sq}|
+	refname is ${sq}           ${sq}\\${sq}${sq}three${sq}\\${sq}${sq}            ${sq}|
+	refname is ${sq}            ${sq}\\${sq}${sq}two${sq}\\${sq}${sq}             ${sq}|
+	EOF
+	git for-each-ref --shell --format="refname is %(align:30,middle)${sq}%(refname:short)${sq}%(end)|" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'nested alignment' '
+	cat >expect <<-\EOF &&
+	|         master               |
+	|           side               |
+	|       odd/spot               |
+	|     double-tag               |
+	|           four               |
+	|            one               |
+	|     signed-tag               |
+	|          three               |
+	|            two               |
+	EOF
+	git for-each-ref --format="|%(align:30,left)%(align:15,right)%(refname:short)%(end)%(end)|" >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.5.0

[PATCH v15 06/13] ref-filter: add option to filter out tags, branches and remotes

[Karthik Nayak]

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

Add a function called 'for_each_fullref_in()' to refs.{c,h} which
iterates through each ref for the given path without trimming the path
and also accounting for broken refs, if mentioned.

Add 'filter_ref_kind()' in ref-filter.c to check the kind of ref being
handled and return the kind to 'ref_filter_handler()', where we
discard refs which we do not need and assign the kind to needed refs.

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 | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 ref-filter.h | 13 +++++++++++--
 refs.c       |  9 +++++++++
 refs.h       |  1 +
 4 files changed, 79 insertions(+), 7 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 3a5f0a7..430c80b 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1163,6 +1163,34 @@ static struct ref_array_item *new_ref_array_item(const char *refname,
 	return ref;
 }
 
+static int filter_ref_kind(struct ref_filter *filter, const char *refname)
+{
+	unsigned int i;
+
+	static struct {
+		const char *prefix;
+		unsigned int kind;
+	} ref_kind[] = {
+		{ "refs/heads/" , FILTER_REFS_BRANCHES },
+		{ "refs/remotes/" , FILTER_REFS_REMOTES },
+		{ "refs/tags/", FILTER_REFS_TAGS}
+	};
+
+	if (filter->kind == FILTER_REFS_BRANCHES ||
+	    filter->kind == FILTER_REFS_REMOTES ||
+	    filter->kind == FILTER_REFS_TAGS)
+		return filter->kind;
+	else if (!strcmp(refname, "HEAD"))
+		return FILTER_REFS_DETACHED_HEAD;
+
+	for (i = 0; i < ARRAY_SIZE(ref_kind); i++) {
+		if (starts_with(refname, ref_kind[i].prefix))
+			return ref_kind[i].kind;
+	}
+
+	return FILTER_REFS_OTHERS;
+}
+
 /*
  * A call-back given to for_each_ref().  Filter refs and keep them for
  * later object processing.
@@ -1173,6 +1201,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);
@@ -1184,6 +1213,15 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
 		return 0;
 	}
 
+	/*
+	 * Get the current ref kind. If we're filtering tags, remotes or local branches
+	 * only then the current ref-kind is nothing but filter->kind and filter_ref_kind()
+	 * will only return that value.
+	 */
+	kind = filter_ref_kind(filter, refname);
+	if (!(kind & filter->kind))
+		return 0;
+
 	if (*filter->name_patterns && !match_name_as_path(filter->name_patterns, refname))
 		return 0;
 
@@ -1215,6 +1253,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
 
 	REALLOC_ARRAY(ref_cbdata->array->items, ref_cbdata->array->nr + 1);
 	ref_cbdata->array->items[ref_cbdata->array->nr++] = ref;
+	ref->kind = kind;
 	return 0;
 }
 
@@ -1291,17 +1330,31 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int
 {
 	struct ref_filter_cbdata ref_cbdata;
 	int ret = 0;
+	unsigned int broken = 0;
 
 	ref_cbdata.array = array;
 	ref_cbdata.filter = filter;
 
+	if (type & FILTER_REFS_INCLUDE_BROKEN)
+		broken = 1;
+	filter->kind = type & FILTER_REFS_KIND_MASK;
+
 	/*  Simple per-ref filtering */
-	if (type & (FILTER_REFS_ALL | FILTER_REFS_INCLUDE_BROKEN))
-		ret = for_each_rawref(ref_filter_handler, &ref_cbdata);
-	else if (type & FILTER_REFS_ALL)
-		ret = for_each_ref(ref_filter_handler, &ref_cbdata);
-	else if (type)
+	if (!filter->kind)
 		die("filter_refs: invalid type");
+	else {
+		if (filter->kind == FILTER_REFS_BRANCHES)
+			ret = for_each_fullref_in("refs/heads/", ref_filter_handler, &ref_cbdata, broken);
+		else if (filter->kind == FILTER_REFS_REMOTES)
+			ret = for_each_fullref_in("refs/remotes/", ref_filter_handler, &ref_cbdata, broken);
+		else if (filter->kind == FILTER_REFS_TAGS)
+			ret = for_each_fullref_in("refs/tags/", ref_filter_handler, &ref_cbdata, broken);
+		else if (filter->kind & FILTER_REFS_ALL)
+			ret = for_each_fullref_in("", ref_filter_handler, &ref_cbdata, broken);
+		if (!ret && (filter->kind & FILTER_REFS_DETACHED_HEAD))
+			head_ref(ref_filter_handler, &ref_cbdata);
+	}
+
 
 	/*  Filters that need revision walking */
 	if (filter->merge_commit)
diff --git a/ref-filter.h b/ref-filter.h
index 45026d0..0913ba9 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -13,8 +13,15 @@
 #define QUOTE_PYTHON 4
 #define QUOTE_TCL 8
 
-#define FILTER_REFS_INCLUDE_BROKEN 0x1
-#define FILTER_REFS_ALL 0x2
+#define FILTER_REFS_INCLUDE_BROKEN 0x0001
+#define FILTER_REFS_TAGS           0x0002
+#define FILTER_REFS_BRANCHES       0x0004
+#define FILTER_REFS_REMOTES        0x0008
+#define FILTER_REFS_OTHERS         0x0010
+#define FILTER_REFS_ALL            (FILTER_REFS_TAGS | FILTER_REFS_BRANCHES | \
+				    FILTER_REFS_REMOTES | FILTER_REFS_OTHERS)
+#define FILTER_REFS_DETACHED_HEAD  0x0020
+#define FILTER_REFS_KIND_MASK      (FILTER_REFS_ALL | FILTER_REFS_DETACHED_HEAD)
 
 struct atom_value;
 
@@ -27,6 +34,7 @@ struct ref_sorting {
 struct ref_array_item {
 	unsigned char objectname[20];
 	int flag;
+	unsigned int kind;
 	const char *symref;
 	struct commit *commit;
 	struct atom_value *value;
@@ -51,6 +59,7 @@ struct ref_filter {
 	struct commit *merge_commit;
 
 	unsigned int with_commit_tag_algo : 1;
+	unsigned int kind;
 };
 
 struct ref_filter_cbdata {
diff --git a/refs.c b/refs.c
index 4e15f60..a9469c2 100644
--- a/refs.c
+++ b/refs.c
@@ -2108,6 +2108,15 @@ int for_each_ref_in(const char *prefix, each_ref_fn fn, void *cb_data)
 	return do_for_each_ref(&ref_cache, prefix, fn, strlen(prefix), 0, cb_data);
 }
 
+int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data, unsigned int broken)
+{
+	unsigned int flag = 0;
+
+	if (broken)
+		flag = DO_FOR_EACH_INCLUDE_BROKEN;
+	return do_for_each_ref(&ref_cache, prefix, fn, 0, flag, cb_data);
+}
+
 int for_each_ref_in_submodule(const char *submodule, const char *prefix,
 		each_ref_fn fn, void *cb_data)
 {
diff --git a/refs.h b/refs.h
index e9a5f32..6d30c98 100644
--- a/refs.h
+++ b/refs.h
@@ -173,6 +173,7 @@ typedef int each_ref_fn(const char *refname,
 extern int head_ref(each_ref_fn fn, void *cb_data);
 extern int for_each_ref(each_ref_fn fn, void *cb_data);
 extern int for_each_ref_in(const char *prefix, each_ref_fn fn, void *cb_data);
+extern int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data, unsigned int broken);
 extern int for_each_tag_ref(each_ref_fn fn, void *cb_data);
 extern int for_each_branch_ref(each_ref_fn fn, void *cb_data);
 extern int for_each_remote_ref(each_ref_fn fn, void *cb_data);
-- 
2.5.0

[PATCH v15 07/13] ref-filter: add support for %(contents:lines=X)

[Karthik Nayak]

In 'tag.c' we can print N lines from the annotation of the tag using
the '-n<num>' option. Copy code from 'tag.c' to 'ref-filter' and
modify it to support appending of N lines from the annotation of tags
to the given strbuf.

Implement %(contents:lines=X) where X lines of the given object are
obtained.

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 |  3 ++-
 builtin/tag.c                      |  4 +++
 ref-filter.c                       | 53 +++++++++++++++++++++++++++++++++++---
 ref-filter.h                       |  3 ++-
 t/t6302-for-each-ref-filter.sh     | 52 +++++++++++++++++++++++++++++++++++++
 5 files changed, 109 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index cac3128..98eb027 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -148,7 +148,8 @@ The complete message in a commit and tag object is `contents`.
 Its first line is `contents:subject`, where subject is the concatenation
 of all lines of the commit message up to the first blank line.  The next
 line is 'contents:body', where body is all of the lines after the first
-blank line.  Finally, the optional GPG signature is `contents:signature`.
+blank line.  The optional GPG signature is `contents:signature`.  The
+first `N` lines of the message is obtained using `contents:lines=N`.
 
 For sorting purposes, fields with numeric values sort in numeric
 order (`objectsize`, `authordate`, `committerdate`, `taggerdate`).
diff --git a/builtin/tag.c b/builtin/tag.c
index 471d6b1..b0bc1c5 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -185,6 +185,10 @@ static enum contains_result contains(struct commit *candidate,
 	return contains_test(candidate, want);
 }
 
+/*
+ * Currently modified and used in ref-filter as append_lines(), will
+ * eventually be removed as we port tag.c to use ref-filter APIs.
+ */
 static void show_tag_lines(const struct object_id *oid, int lines)
 {
 	int i;
diff --git a/ref-filter.c b/ref-filter.c
index 430c80b..0e58fee 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -56,6 +56,7 @@ static struct {
 	{ "color" },
 	{ "align" },
 	{ "end" },
+	{ "contents:lines" },
 };
 
 #define REF_FORMATTING_STATE_INIT  { 0, NULL }
@@ -65,6 +66,11 @@ struct align {
 	unsigned int width;
 };
 
+struct contents {
+	unsigned int lines;
+	struct object_id oid;
+};
+
 struct ref_formatting_stack {
 	struct ref_formatting_stack *prev;
 	struct strbuf output;
@@ -79,7 +85,10 @@ struct ref_formatting_state {
 
 struct atom_value {
 	const char *s;
-	struct align align;
+	union {
+		struct align align;
+		struct contents contents;
+	} u;
 	void (*handler)(struct atom_value *atomv, struct ref_formatting_state *state);
 	unsigned long ul; /* used for sorting when not FIELD_STR */
 };
@@ -226,7 +235,7 @@ static void align_atom_handler(struct atom_value *atomv, struct ref_formatting_s
 	push_stack_element(&state->stack);
 	new = state->stack;
 	new->at_end = align_handler;
-	new->cb_data = &atomv->align;
+	new->cb_data = &atomv->u.align;
 }
 
 static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
@@ -624,6 +633,33 @@ static void find_subpos(const char *buf, unsigned long sz,
 	*nonsiglen = *sig - buf;
 }
 
+/*
+ * If 'lines' is greater than 0, append that many lines from the given
+ * 'buf' of length 'size' to the given strbuf.
+ */
+static void append_lines(struct strbuf *out, const char *buf, unsigned long size, int lines)
+{
+	int i;
+	const char *sp, *eol;
+	size_t len;
+
+	if ((sp = strstr(buf, "\n\n")) && (sp <= buf + size))
+		size += 2;
+
+	sp = buf;
+
+	for (i = 0; i < lines && sp < buf + size; i++) {
+		if (i)
+			strbuf_addstr(out, "\n    ");
+		eol = memchr(sp, '\n', size - (sp - buf));
+		len = eol ? eol - sp : size - (sp - buf);
+		strbuf_add(out, sp, len);
+		if (!eol)
+			break;
+		sp = eol + 1;
+	}
+}
+
 /* See grab_values */
 static void grab_sub_body_contents(struct atom_value *val, int deref, struct object *obj, void *buf, unsigned long sz)
 {
@@ -634,6 +670,7 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj
 	for (i = 0; i < used_atom_cnt; i++) {
 		const char *name = used_atom[i];
 		struct atom_value *v = &val[i];
+		const char *valp = NULL;
 		if (!!deref != (*name == '*'))
 			continue;
 		if (deref)
@@ -643,7 +680,8 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj
 		    strcmp(name, "contents") &&
 		    strcmp(name, "contents:subject") &&
 		    strcmp(name, "contents:body") &&
-		    strcmp(name, "contents:signature"))
+		    strcmp(name, "contents:signature") &&
+		    !starts_with(name, "contents:lines="))
 			continue;
 		if (!subpos)
 			find_subpos(buf, sz,
@@ -663,6 +701,13 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj
 			v->s = xmemdupz(sigpos, siglen);
 		else if (!strcmp(name, "contents"))
 			v->s = xstrdup(subpos);
+		else if (skip_prefix(name, "contents:lines=", &valp)) {
+			struct strbuf s = STRBUF_INIT;
+			if (strtoul_ui(valp, 10, &v->u.contents.lines))
+				die(_("positive width expected contents:lines=%s"), valp);
+			append_lines(&s, subpos, sublen + bodylen - siglen, v->u.contents.lines);
+			v->s = strbuf_detach(&s, NULL);
+		}
 	}
 }
 
@@ -817,7 +862,7 @@ static void populate_value(struct ref_array_item *ref)
 				v->s = " ";
 			continue;
 		} else if (skip_prefix(name, "align", &valp)) {
-			struct align *align = &v->align;
+			struct align *align = &v->u.align;
 			struct strbuf **s;
 
 			if (valp[0] != ':')
diff --git a/ref-filter.h b/ref-filter.h
index 0913ba9..ab76b22 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -59,7 +59,8 @@ struct ref_filter {
 	struct commit *merge_commit;
 
 	unsigned int with_commit_tag_algo : 1;
-	unsigned int kind;
+	unsigned int kind,
+		lines;
 };
 
 struct ref_filter_cbdata {
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index cef7a41..75da32f 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -166,4 +166,56 @@ test_expect_success 'nested alignment' '
 	test_cmp expect actual
 '
 
+test_expect_success 'check `%(contents:lines=1)`' '
+	cat >expect <<-\EOF &&
+	master |three
+	side |four
+	odd/spot |three
+	double-tag |Annonated doubly
+	four |four
+	one |one
+	signed-tag |A signed tag message
+	three |three
+	two |two
+	EOF
+	git for-each-ref --format="%(refname:short) |%(contents:lines=1)" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check `%(contents:lines=0)`' '
+	cat >expect <<-\EOF &&
+	master |
+	side |
+	odd/spot |
+	double-tag |
+	four |
+	one |
+	signed-tag |
+	three |
+	two |
+	EOF
+	git for-each-ref --format="%(refname:short) |%(contents:lines=0)" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'check `%(contents:lines=99999)`' '
+	cat >expect <<-\EOF &&
+	master |three
+	side |four
+	odd/spot |three
+	double-tag |Annonated doubly
+	four |four
+	one |one
+	signed-tag |A signed tag message
+	three |three
+	two |two
+	EOF
+	git for-each-ref --format="%(refname:short) |%(contents:lines=99999)" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '`%(contents:lines=-1)` should fail' '
+	test_must_fail git for-each-ref --format="%(refname:short) |%(contents:lines=-1)"
+'
+
 test_done
-- 
2.5.0

[PATCH v15 08/13] ref-filter: add support to sort by version

[Karthik Nayak]

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

Add support to sort by version using the "v:refname" and
"version:refname" option. This is achieved by using the 'versioncmp()'
function as the comparing function for qsort.

This option is included to support sorting by versions in `git tag -l`
which will eventually be ported to use ref-filter APIs.

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-for-each-ref.txt |  3 +++
 ref-filter.c                       | 15 ++++++++++-----
 ref-filter.h                       |  3 ++-
 t/t6302-for-each-ref-filter.sh     | 36 ++++++++++++++++++++++++++++++++++++
 4 files changed, 51 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 98eb027..d039f40 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -155,6 +155,9 @@ For sorting purposes, fields with numeric values sort in numeric
 order (`objectsize`, `authordate`, `committerdate`, `taggerdate`).
 All other fields are used to sort in their byte-value order.
 
+There is also an option to sort by versions, this can be done by using
+the fieldname `version:refname` or its alias `v:refname`.
+
 In any case, a field name that refers to a field inapplicable to
 the object referred by the ref does not cause an error.  It
 returns an empty string instead.
diff --git a/ref-filter.c b/ref-filter.c
index 0e58fee..a545fd4 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -11,6 +11,8 @@
 #include "ref-filter.h"
 #include "revision.h"
 #include "utf8.h"
+#include "git-compat-util.h"
+#include "version.h"
 
 typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
 
@@ -1416,19 +1418,19 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru
 
 	get_ref_atom_value(a, s->atom, &va);
 	get_ref_atom_value(b, s->atom, &vb);
-	switch (cmp_type) {
-	case FIELD_STR:
+	if (s->version)
+		cmp = versioncmp(va->s, vb->s);
+	else if (cmp_type == FIELD_STR)
 		cmp = strcmp(va->s, vb->s);
-		break;
-	default:
+	else {
 		if (va->ul < vb->ul)
 			cmp = -1;
 		else if (va->ul == vb->ul)
 			cmp = 0;
 		else
 			cmp = 1;
-		break;
 	}
+
 	return (s->reverse) ? -cmp : cmp;
 }
 
@@ -1561,6 +1563,9 @@ int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset)
 		s->reverse = 1;
 		arg++;
 	}
+	if (skip_prefix(arg, "version:", &arg) ||
+	    skip_prefix(arg, "v:", &arg))
+		s->version = 1;
 	len = strlen(arg);
 	s->atom = parse_ref_filter_atom(arg, arg+len);
 	return 0;
diff --git a/ref-filter.h b/ref-filter.h
index ab76b22..ef25b6e 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -28,7 +28,8 @@ struct atom_value;
 struct ref_sorting {
 	struct ref_sorting *next;
 	int atom; /* index into used_atom array (internal) */
-	unsigned reverse : 1;
+	unsigned reverse : 1,
+		version : 1;
 };
 
 struct ref_array_item {
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index 75da32f..d95b781 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -218,4 +218,40 @@ test_expect_success '`%(contents:lines=-1)` should fail' '
 	test_must_fail git for-each-ref --format="%(refname:short) |%(contents:lines=-1)"
 '
 
+test_expect_success 'setup for version sort' '
+	test_commit foo1.3 &&
+	test_commit foo1.6 &&
+	test_commit foo1.10
+'
+
+test_expect_success 'version sort' '
+	git for-each-ref --sort=version:refname --format="%(refname:short)" refs/tags/ | grep "foo" >actual &&
+	cat >expect <<-\EOF &&
+	foo1.3
+	foo1.6
+	foo1.10
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'version sort (shortened)' '
+	git for-each-ref --sort=v:refname --format="%(refname:short)" refs/tags/ | grep "foo" >actual &&
+	cat >expect <<-\EOF &&
+	foo1.3
+	foo1.6
+	foo1.10
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'reverse version sort' '
+	git for-each-ref --sort=-version:refname --format="%(refname:short)" refs/tags/ | grep "foo" >actual &&
+	cat >expect <<-\EOF &&
+	foo1.10
+	foo1.6
+	foo1.3
+	EOF
+	test_cmp expect actual
+'
+
 test_done
-- 
2.5.0

[PATCH v15 09/13] ref-filter: add option to match literal pattern

[Karthik Nayak]

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

Since 'ref-filter' only has an option to match path names add an
option for plain fnmatch pattern-matching.

This is to support the pattern matching options which are used in `git
tag -l` and `git branch -l` where we can match patterns like `git tag
-l foo*` which would match all tags which has a "foo*" pattern.

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/for-each-ref.c |  1 +
 ref-filter.c           | 40 +++++++++++++++++++++++++++++++++++++---
 ref-filter.h           |  3 ++-
 3 files changed, 40 insertions(+), 4 deletions(-)

diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 40f343b..4e9f6c2 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -68,6 +68,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 	git_config(git_default_config, NULL);
 
 	filter.name_patterns = argv;
+	filter.match_as_path = 1;
 	filter_refs(&array, &filter, FILTER_REFS_ALL | FILTER_REFS_INCLUDE_BROKEN);
 	ref_array_sort(sorting, &array);
 
diff --git a/ref-filter.c b/ref-filter.c
index a545fd4..b37d57a 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1140,9 +1140,33 @@ static int commit_contains(struct ref_filter *filter, struct commit *commit)
 
 /*
  * Return 1 if the refname matches one of the patterns, otherwise 0.
+ * A pattern can be a literal prefix (e.g. a refname "refs/heads/master"
+ * matches a pattern "refs/heads/mas") or a wildcard (e.g. the same ref
+ * matches "refs/heads/mas*", too).
+ */
+static int match_pattern(const char **patterns, const char *refname)
+{
+	/*
+	 * When no '--format' option is given we need to skip the prefix
+	 * for matching refs of tags and branches.
+	 */
+	(void)(skip_prefix(refname, "refs/tags/", &refname) ||
+	       skip_prefix(refname, "refs/heads/", &refname) ||
+	       skip_prefix(refname, "refs/remotes/", &refname) ||
+	       skip_prefix(refname, "refs/", &refname));
+
+	for (; *patterns; patterns++) {
+		if (!wildmatch(*patterns, refname, 0, NULL))
+			return 1;
+	}
+	return 0;
+}
+
+/*
+ * Return 1 if the refname matches one of the patterns, otherwise 0.
  * A pattern can be path prefix (e.g. a refname "refs/heads/master"
- * matches a pattern "refs/heads/") or a wildcard (e.g. the same ref
- * matches "refs/heads/m*",too).
+ * matches a pattern "refs/heads/" but not "refs/heads/m") or a
+ * wildcard (e.g. the same ref matches "refs/heads/m*", too).
  */
 static int match_name_as_path(const char **pattern, const char *refname)
 {
@@ -1163,6 +1187,16 @@ static int match_name_as_path(const char **pattern, const char *refname)
 	return 0;
 }
 
+/* Return 1 if the refname matches one of the patterns, otherwise 0. */
+static int filter_pattern_match(struct ref_filter *filter, const char *refname)
+{
+	if (!*filter->name_patterns)
+		return 1; /* No pattern always matches */
+	if (filter->match_as_path)
+		return match_name_as_path(filter->name_patterns, refname);
+	return match_pattern(filter->name_patterns, refname);
+}
+
 /*
  * Given a ref (sha1, refname), check if the ref belongs to the array
  * of sha1s. If the given ref is a tag, check if the given tag points
@@ -1269,7 +1303,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
 	if (!(kind & filter->kind))
 		return 0;
 
-	if (*filter->name_patterns && !match_name_as_path(filter->name_patterns, refname))
+	if (!filter_pattern_match(filter, refname))
 		return 0;
 
 	if (filter->points_at.nr && !match_points_at(&filter->points_at, oid->hash, refname))
diff --git a/ref-filter.h b/ref-filter.h
index ef25b6e..a5cfa5e 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -59,7 +59,8 @@ struct ref_filter {
 	} merge;
 	struct commit *merge_commit;
 
-	unsigned int with_commit_tag_algo : 1;
+	unsigned int with_commit_tag_algo : 1,
+		match_as_path : 1;
 	unsigned int kind,
 		lines;
 };
-- 
2.5.0

[PATCH v15 10/13] tag.c: use 'ref-filter' data structures

[Karthik Nayak]

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

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

This is a temporary step before porting 'tag.c' to use 'ref-filter'
completely. As this is a temporary step, most of the code
introduced here will be removed when 'tag.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/tag.c | 106 +++++++++++++++++++++++++++++++---------------------------
 1 file changed, 57 insertions(+), 49 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index b0bc1c5..fe66f7b 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -17,6 +17,7 @@
 #include "gpg-interface.h"
 #include "sha1-array.h"
 #include "column.h"
+#include "ref-filter.h"
 
 static const char * const git_tag_usage[] = {
 	N_("git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] <tagname> [<head>]"),
@@ -34,15 +35,6 @@ static const char * const git_tag_usage[] = {
 
 static int tag_sort;
 
-struct tag_filter {
-	const char **patterns;
-	int lines;
-	int sort;
-	struct string_list tags;
-	struct commit_list *with_commit;
-};
-
-static struct sha1_array points_at;
 static unsigned int colopts;
 
 static int match_pattern(const char **patterns, const char *ref)
@@ -61,19 +53,20 @@ static int match_pattern(const char **patterns, const char *ref)
  * removed as we port tag.c to use the ref-filter APIs.
  */
 static const unsigned char *match_points_at(const char *refname,
-					    const unsigned char *sha1)
+					    const unsigned char *sha1,
+					    struct sha1_array *points_at)
 {
 	const unsigned char *tagged_sha1 = NULL;
 	struct object *obj;
 
-	if (sha1_array_lookup(&points_at, sha1) >= 0)
+	if (sha1_array_lookup(points_at, sha1) >= 0)
 		return sha1;
 	obj = parse_object(sha1);
 	if (!obj)
 		die(_("malformed object at '%s'"), refname);
 	if (obj->type == OBJ_TAG)
 		tagged_sha1 = ((struct tag *)obj)->tagged->sha1;
-	if (tagged_sha1 && sha1_array_lookup(&points_at, tagged_sha1) >= 0)
+	if (tagged_sha1 && sha1_array_lookup(points_at, tagged_sha1) >= 0)
 		return tagged_sha1;
 	return NULL;
 }
@@ -228,12 +221,24 @@ free_return:
 	free(buf);
 }
 
+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 show_reference(const char *refname, const struct object_id *oid,
 			  int flag, void *cb_data)
 {
-	struct tag_filter *filter = cb_data;
+	struct ref_filter_cbdata *data = cb_data;
+	struct ref_array *array = data->array;
+	struct ref_filter *filter = data->filter;
 
-	if (match_pattern(filter->patterns, refname)) {
+	if (match_pattern(filter->name_patterns, refname)) {
 		if (filter->with_commit) {
 			struct commit *commit;
 
@@ -244,12 +249,12 @@ static int show_reference(const char *refname, const struct object_id *oid,
 				return 0;
 		}
 
-		if (points_at.nr && !match_points_at(refname, oid->hash))
+		if (filter->points_at.nr && !match_points_at(refname, oid->hash, &filter->points_at))
 			return 0;
 
 		if (!filter->lines) {
-			if (filter->sort)
-				string_list_append(&filter->tags, refname);
+			if (tag_sort)
+				ref_array_append(array, refname);
 			else
 				printf("%s\n", refname);
 			return 0;
@@ -264,36 +269,36 @@ static int show_reference(const char *refname, const struct object_id *oid,
 
 static int sort_by_version(const void *a_, const void *b_)
 {
-	const struct string_list_item *a = a_;
-	const struct string_list_item *b = b_;
-	return versioncmp(a->string, b->string);
+	const struct ref_array_item *a = *((struct ref_array_item **)a_);
+	const struct ref_array_item *b = *((struct ref_array_item **)b_);
+	return versioncmp(a->refname, b->refname);
 }
 
-static int list_tags(const char **patterns, int lines,
-		     struct commit_list *with_commit, int sort)
+static int list_tags(struct ref_filter *filter, int sort)
 {
-	struct tag_filter filter;
+	struct ref_array array;
+	struct ref_filter_cbdata data;
+
+	memset(&array, 0, sizeof(array));
+	data.array = &array;
+	data.filter = filter;
 
-	filter.patterns = patterns;
-	filter.lines = lines;
-	filter.sort = sort;
-	filter.with_commit = with_commit;
-	memset(&filter.tags, 0, sizeof(filter.tags));
-	filter.tags.strdup_strings = 1;
+	if (filter->lines == -1)
+		filter->lines = 0;
 
-	for_each_tag_ref(show_reference, (void *)&filter);
+	for_each_tag_ref(show_reference, &data);
 	if (sort) {
 		int i;
 		if ((sort & SORT_MASK) == VERCMP_SORT)
-			qsort(filter.tags.items, filter.tags.nr,
-			      sizeof(struct string_list_item), sort_by_version);
+			qsort(array.items, array.nr,
+			      sizeof(struct ref_array_item *), sort_by_version);
 		if (sort & REVERSE_SORT)
-			for (i = filter.tags.nr - 1; i >= 0; i--)
-				printf("%s\n", filter.tags.items[i].string);
+			for (i = array.nr - 1; i >= 0; i--)
+				printf("%s\n", array.items[i]->refname);
 		else
-			for (i = 0; i < filter.tags.nr; i++)
-				printf("%s\n", filter.tags.items[i].string);
-		string_list_clear(&filter.tags, 0);
+			for (i = 0; i < array.nr; i++)
+				printf("%s\n", array.items[i]->refname);
+		ref_array_clear(&array);
 	}
 	return 0;
 }
@@ -574,17 +579,17 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	const char *object_ref, *tag;
 	struct create_tag_options opt;
 	char *cleanup_arg = NULL;
-	int annotate = 0, force = 0, lines = -1;
 	int create_reflog = 0;
+	int annotate = 0, force = 0;
 	int cmdmode = 0;
 	const char *msgfile = NULL, *keyid = NULL;
 	struct msg_arg msg = { 0, STRBUF_INIT };
-	struct commit_list *with_commit = NULL;
 	struct ref_transaction *transaction;
 	struct strbuf err = STRBUF_INIT;
+	struct ref_filter filter;
 	struct option options[] = {
 		OPT_CMDMODE('l', "list", &cmdmode, N_("list tag names"), 'l'),
-		{ OPTION_INTEGER, 'n', NULL, &lines, N_("n"),
+		{ OPTION_INTEGER, 'n', NULL, &filter.lines, N_("n"),
 				N_("print <n> lines of each tag message"),
 				PARSE_OPT_OPTARG, NULL, 1 },
 		OPT_CMDMODE('d', "delete", &cmdmode, N_("delete tags"), 'd'),
@@ -606,14 +611,14 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 
 		OPT_GROUP(N_("Tag listing options")),
 		OPT_COLUMN(0, "column", &colopts, N_("show tag list in columns")),
-		OPT_CONTAINS(&with_commit, N_("print only tags that contain the commit")),
-		OPT_WITH(&with_commit, N_("print only tags that contain the commit")),
+		OPT_CONTAINS(&filter.with_commit, N_("print only tags that contain the commit")),
+		OPT_WITH(&filter.with_commit, N_("print only tags that contain the commit")),
 		{
 			OPTION_CALLBACK, 0, "sort", &tag_sort, N_("type"), N_("sort tags"),
 			PARSE_OPT_NONEG, parse_opt_sort
 		},
 		{
-			OPTION_CALLBACK, 0, "points-at", &points_at, N_("object"),
+			OPTION_CALLBACK, 0, "points-at", &filter.points_at, N_("object"),
 			N_("print only tags of the object"), 0, parse_opt_object_name
 		},
 		OPT_END()
@@ -622,6 +627,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	git_config(git_tag_config, NULL);
 
 	memset(&opt, 0, sizeof(opt));
+	memset(&filter, 0, sizeof(filter));
+	filter.lines = -1;
 
 	argc = parse_options(argc, argv, prefix, options, git_tag_usage, 0);
 
@@ -638,7 +645,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 		usage_with_options(git_tag_usage, options);
 
 	finalize_colopts(&colopts, -1);
-	if (cmdmode == 'l' && lines != -1) {
+	if (cmdmode == 'l' && filter.lines != -1) {
 		if (explicitly_enable_column(colopts))
 			die(_("--column and -n are incompatible"));
 		colopts = 0;
@@ -651,18 +658,19 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 			copts.padding = 2;
 			run_column_filter(colopts, &copts);
 		}
-		if (lines != -1 && tag_sort)
+		if (filter.lines != -1 && tag_sort)
 			die(_("--sort and -n are incompatible"));
-		ret = list_tags(argv, lines == -1 ? 0 : lines, with_commit, tag_sort);
+		filter.name_patterns = argv;
+		ret = list_tags(&filter, tag_sort);
 		if (column_active(colopts))
 			stop_column_filter();
 		return ret;
 	}
-	if (lines != -1)
+	if (filter.lines != -1)
 		die(_("-n option is only allowed with -l."));
-	if (with_commit)
+	if (filter.with_commit)
 		die(_("--contains option is only allowed with -l."));
-	if (points_at.nr)
+	if (filter.points_at.nr)
 		die(_("--points-at option is only allowed with -l."));
 	if (cmdmode == 'd')
 		return for_each_tag_name(argv, delete_tag);
-- 
2.5.0

[PATCH v15 11/13] tag.c: use 'ref-filter' APIs

[Karthik Nayak]

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

Make 'tag.c' use 'ref-filter' APIs for iterating through refs, sorting
and printing of refs. This removes most of the code used in 'tag.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.

For printing tags we use 'show_ref_array_item()' function provided by
'ref-filter'.

We improve the sorting option provided by 'tag.c' by using the sorting
options provided by 'ref-filter'. This causes the test 'invalid sort
parameter on command line' in t7004 to fail, as 'ref-filter' throws an
error for all sorting fields which are incorrect. The test is changed
to reflect the same.

Modify documentation for the same.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 Documentation/git-tag.txt |  16 ++-
 builtin/tag.c             | 344 ++++++----------------------------------------
 t/t7004-tag.sh            |   8 +-
 3 files changed, 52 insertions(+), 316 deletions(-)

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 84f6496..3ac4a96 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -13,7 +13,7 @@ SYNOPSIS
 	<tagname> [<commit> | <object>]
 'git tag' -d <tagname>...
 'git tag' [-n[<num>]] -l [--contains <commit>] [--points-at <object>]
-	[--column[=<options>] | --no-column] [--create-reflog] [<pattern>...]
+	[--column[=<options>] | --no-column] [--create-reflog] [--sort=<key>] [<pattern>...]
 'git tag' -v <tagname>...
 
 DESCRIPTION
@@ -94,14 +94,16 @@ OPTIONS
 	using fnmatch(3)).  Multiple patterns may be given; if any of
 	them matches, the tag is shown.
 
---sort=<type>::
-	Sort in a specific order. Supported type is "refname"
-	(lexicographic order), "version:refname" or "v:refname" (tag
+--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. Also supports "version:refname" or "v:refname" (tag
 	names are treated as versions). The "version:refname" sort
 	order can also be affected by the
-	"versionsort.prereleaseSuffix" configuration variable. Prepend
-	"-" to reverse sort order. When this option is not given, the
-	sort order defaults to the value configured for the 'tag.sort'
+	"versionsort.prereleaseSuffix" configuration variable.
+	The keys supported are the same as those in `git for-each-ref`.
+	Sort order defaults to the value configured for the 'tag.sort'
 	variable if it exists, or lexicographic order otherwise. See
 	linkgit:git-config[1].
 
diff --git a/builtin/tag.c b/builtin/tag.c
index fe66f7b..2d348f4 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -28,278 +28,34 @@ static const char * const git_tag_usage[] = {
 	NULL
 };
 
-#define STRCMP_SORT     0	/* must be zero */
-#define VERCMP_SORT     1
-#define SORT_MASK       0x7fff
-#define REVERSE_SORT    0x8000
-
-static int tag_sort;
-
 static unsigned int colopts;
 
-static int match_pattern(const char **patterns, const char *ref)
-{
-	/* no pattern means match everything */
-	if (!*patterns)
-		return 1;
-	for (; *patterns; patterns++)
-		if (!wildmatch(*patterns, ref, 0, NULL))
-			return 1;
-	return 0;
-}
-
-/*
- * This is currently duplicated in ref-filter.c, and will eventually be
- * removed as we port tag.c to use the ref-filter APIs.
- */
-static const unsigned char *match_points_at(const char *refname,
-					    const unsigned char *sha1,
-					    struct sha1_array *points_at)
-{
-	const unsigned char *tagged_sha1 = NULL;
-	struct object *obj;
-
-	if (sha1_array_lookup(points_at, sha1) >= 0)
-		return sha1;
-	obj = parse_object(sha1);
-	if (!obj)
-		die(_("malformed object at '%s'"), refname);
-	if (obj->type == OBJ_TAG)
-		tagged_sha1 = ((struct tag *)obj)->tagged->sha1;
-	if (tagged_sha1 && sha1_array_lookup(points_at, tagged_sha1) >= 0)
-		return tagged_sha1;
-	return NULL;
-}
-
-static int in_commit_list(const struct commit_list *want, struct commit *c)
-{
-	for (; want; want = want->next)
-		if (!hashcmp(want->item->object.sha1, c->object.sha1))
-			return 1;
-	return 0;
-}
-
-/*
- * The entire code segment for supporting the --contains option has been
- * copied over to ref-filter.{c,h}. This will be deleted evetually when
- * we port tag.c to use ref-filter APIs.
- */
-enum contains_result {
-	CONTAINS_UNKNOWN = -1,
-	CONTAINS_NO = 0,
-	CONTAINS_YES = 1
-};
-
-/*
- * Test whether the candidate or one of its parents is contained in the list.
- * Do not recurse to find out, though, but return -1 if inconclusive.
- */
-static enum contains_result contains_test(struct commit *candidate,
-			    const struct commit_list *want)
-{
-	/* was it previously marked as containing a want commit? */
-	if (candidate->object.flags & TMP_MARK)
-		return 1;
-	/* or marked as not possibly containing a want commit? */
-	if (candidate->object.flags & UNINTERESTING)
-		return 0;
-	/* or are we it? */
-	if (in_commit_list(want, candidate)) {
-		candidate->object.flags |= TMP_MARK;
-		return 1;
-	}
-
-	if (parse_commit(candidate) < 0)
-		return 0;
-
-	return -1;
-}
-
-/*
- * Mimicking the real stack, this stack lives on the heap, avoiding stack
- * overflows.
- *
- * At each recursion step, the stack items points to the commits whose
- * ancestors are to be inspected.
- */
-struct stack {
-	int nr, alloc;
-	struct stack_entry {
-		struct commit *commit;
-		struct commit_list *parents;
-	} *stack;
-};
-
-static void push_to_stack(struct commit *candidate, struct stack *stack)
-{
-	int index = stack->nr++;
-	ALLOC_GROW(stack->stack, stack->nr, stack->alloc);
-	stack->stack[index].commit = candidate;
-	stack->stack[index].parents = candidate->parents;
-}
-
-static enum contains_result contains(struct commit *candidate,
-		const struct commit_list *want)
-{
-	struct stack stack = { 0, 0, NULL };
-	int result = contains_test(candidate, want);
-
-	if (result != CONTAINS_UNKNOWN)
-		return result;
-
-	push_to_stack(candidate, &stack);
-	while (stack.nr) {
-		struct stack_entry *entry = &stack.stack[stack.nr - 1];
-		struct commit *commit = entry->commit;
-		struct commit_list *parents = entry->parents;
-
-		if (!parents) {
-			commit->object.flags |= UNINTERESTING;
-			stack.nr--;
-		}
-		/*
-		 * If we just popped the stack, parents->item has been marked,
-		 * therefore contains_test will return a meaningful 0 or 1.
-		 */
-		else switch (contains_test(parents->item, want)) {
-		case CONTAINS_YES:
-			commit->object.flags |= TMP_MARK;
-			stack.nr--;
-			break;
-		case CONTAINS_NO:
-			entry->parents = parents->next;
-			break;
-		case CONTAINS_UNKNOWN:
-			push_to_stack(parents->item, &stack);
-			break;
-		}
-	}
-	free(stack.stack);
-	return contains_test(candidate, want);
-}
-
-/*
- * Currently modified and used in ref-filter as append_lines(), will
- * eventually be removed as we port tag.c to use ref-filter APIs.
- */
-static void show_tag_lines(const struct object_id *oid, int lines)
-{
-	int i;
-	unsigned long size;
-	enum object_type type;
-	char *buf, *sp, *eol;
-	size_t len;
-
-	buf = read_sha1_file(oid->hash, &type, &size);
-	if (!buf)
-		die_errno("unable to read object %s", oid_to_hex(oid));
-	if (type != OBJ_COMMIT && type != OBJ_TAG)
-		goto free_return;
-	if (!size)
-		die("an empty %s object %s?",
-		    typename(type), oid_to_hex(oid));
-
-	/* skip header */
-	sp = strstr(buf, "\n\n");
-	if (!sp)
-		goto free_return;
-
-	/* only take up to "lines" lines, and strip the signature from a tag */
-	if (type == OBJ_TAG)
-		size = parse_signature(buf, size);
-	for (i = 0, sp += 2; i < lines && sp < buf + size; i++) {
-		if (i)
-			printf("\n    ");
-		eol = memchr(sp, '\n', size - (sp - buf));
-		len = eol ? eol - sp : size - (sp - buf);
-		fwrite(sp, len, 1, stdout);
-		if (!eol)
-			break;
-		sp = eol + 1;
-	}
-free_return:
-	free(buf);
-}
-
-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 show_reference(const char *refname, const struct object_id *oid,
-			  int flag, void *cb_data)
-{
-	struct ref_filter_cbdata *data = cb_data;
-	struct ref_array *array = data->array;
-	struct ref_filter *filter = data->filter;
-
-	if (match_pattern(filter->name_patterns, refname)) {
-		if (filter->with_commit) {
-			struct commit *commit;
-
-			commit = lookup_commit_reference_gently(oid->hash, 1);
-			if (!commit)
-				return 0;
-			if (!contains(commit, filter->with_commit))
-				return 0;
-		}
-
-		if (filter->points_at.nr && !match_points_at(refname, oid->hash, &filter->points_at))
-			return 0;
-
-		if (!filter->lines) {
-			if (tag_sort)
-				ref_array_append(array, refname);
-			else
-				printf("%s\n", refname);
-			return 0;
-		}
-		printf("%-15s ", refname);
-		show_tag_lines(oid, filter->lines);
-		putchar('\n');
-	}
-
-	return 0;
-}
-
-static int sort_by_version(const void *a_, const void *b_)
-{
-	const struct ref_array_item *a = *((struct ref_array_item **)a_);
-	const struct ref_array_item *b = *((struct ref_array_item **)b_);
-	return versioncmp(a->refname, b->refname);
-}
-
-static int list_tags(struct ref_filter *filter, int sort)
+static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting)
 {
 	struct ref_array array;
-	struct ref_filter_cbdata data;
+	char *format, *to_free = NULL;
+	int i;
 
 	memset(&array, 0, sizeof(array));
-	data.array = &array;
-	data.filter = filter;
 
 	if (filter->lines == -1)
 		filter->lines = 0;
 
-	for_each_tag_ref(show_reference, &data);
-	if (sort) {
-		int i;
-		if ((sort & SORT_MASK) == VERCMP_SORT)
-			qsort(array.items, array.nr,
-			      sizeof(struct ref_array_item *), sort_by_version);
-		if (sort & REVERSE_SORT)
-			for (i = array.nr - 1; i >= 0; i--)
-				printf("%s\n", array.items[i]->refname);
-		else
-			for (i = 0; i < array.nr; i++)
-				printf("%s\n", array.items[i]->refname);
-		ref_array_clear(&array);
-	}
+	if (filter->lines)
+		format = to_free = xstrfmt("%%(align:15,left)%%(refname:short)%%(end) %%(contents:lines=%d)",
+					   filter->lines);
+	else
+		format = "%(refname:short)";
+
+	verify_ref_format(format);
+	filter_refs(&array, filter, FILTER_REFS_TAGS);
+	ref_array_sort(sorting, &array);
+
+	for (i = 0; i < array.nr; i++)
+		show_ref_array_item(array.items[i], format, 0);
+	ref_array_clear(&array);
+	free(to_free);
+
 	return 0;
 }
 
@@ -366,35 +122,26 @@ static const char tag_template_nocleanup[] =
 	"Lines starting with '%c' will be kept; you may remove them"
 	" yourself if you want to.\n");
 
-/*
- * Parse a sort string, and return 0 if parsed successfully. Will return
- * non-zero when the sort string does not parse into a known type. If var is
- * given, the error message becomes a warning and includes information about
- * the configuration value.
- */
-static int parse_sort_string(const char *var, const char *arg, int *sort)
+/* Parse arg given and add it the ref_sorting array */
+static int parse_sorting_string(const char *arg, struct ref_sorting **sorting_tail)
 {
-	int type = 0, flags = 0;
-
-	if (skip_prefix(arg, "-", &arg))
-		flags |= REVERSE_SORT;
+	struct ref_sorting *s;
+	int len;
 
-	if (skip_prefix(arg, "version:", &arg) || skip_prefix(arg, "v:", &arg))
-		type = VERCMP_SORT;
-	else
-		type = STRCMP_SORT;
+	s = xcalloc(1, sizeof(*s));
+	s->next = *sorting_tail;
+	*sorting_tail = s;
 
-	if (strcmp(arg, "refname")) {
-		if (!var)
-			return error(_("unsupported sort specification '%s'"), arg);
-		else {
-			warning(_("unsupported sort specification '%s' in variable '%s'"),
-				var, arg);
-			return -1;
-		}
+	if (*arg == '-') {
+		s->reverse = 1;
+		arg++;
 	}
+	if (skip_prefix(arg, "version:", &arg) ||
+	    skip_prefix(arg, "v:", &arg))
+		s->version = 1;
 
-	*sort = (type | flags);
+	len = strlen(arg);
+	s->atom = parse_ref_filter_atom(arg, arg+len);
 
 	return 0;
 }
@@ -402,11 +149,12 @@ static int parse_sort_string(const char *var, const char *arg, int *sort)
 static int git_tag_config(const char *var, const char *value, void *cb)
 {
 	int status;
+	struct ref_sorting **sorting_tail = (struct ref_sorting **)cb;
 
 	if (!strcmp(var, "tag.sort")) {
 		if (!value)
 			return config_error_nonbool(var);
-		parse_sort_string(var, value, &tag_sort);
+		parse_sorting_string(value, sorting_tail);
 		return 0;
 	}
 
@@ -564,13 +312,6 @@ static int strbuf_check_tag_ref(struct strbuf *sb, const char *name)
 	return check_refname_format(sb->buf, 0);
 }
 
-static int parse_opt_sort(const struct option *opt, const char *arg, int unset)
-{
-	int *sort = opt->value;
-
-	return parse_sort_string(NULL, arg, sort);
-}
-
 int cmd_tag(int argc, const char **argv, const char *prefix)
 {
 	struct strbuf buf = STRBUF_INIT;
@@ -587,6 +328,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	struct ref_transaction *transaction;
 	struct strbuf err = STRBUF_INIT;
 	struct ref_filter filter;
+	static struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;
 	struct option options[] = {
 		OPT_CMDMODE('l', "list", &cmdmode, N_("list tag names"), 'l'),
 		{ OPTION_INTEGER, 'n', NULL, &filter.lines, N_("n"),
@@ -613,10 +355,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 		OPT_COLUMN(0, "column", &colopts, N_("show tag list in columns")),
 		OPT_CONTAINS(&filter.with_commit, N_("print only tags that contain the commit")),
 		OPT_WITH(&filter.with_commit, N_("print only tags that contain the commit")),
-		{
-			OPTION_CALLBACK, 0, "sort", &tag_sort, N_("type"), N_("sort tags"),
-			PARSE_OPT_NONEG, parse_opt_sort
-		},
+		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
@@ -624,7 +364,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
-	git_config(git_tag_config, NULL);
+	git_config(git_tag_config, sorting_tail);
 
 	memset(&opt, 0, sizeof(opt));
 	memset(&filter, 0, sizeof(filter));
@@ -650,6 +390,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 			die(_("--column and -n are incompatible"));
 		colopts = 0;
 	}
+	if (!sorting)
+		sorting = ref_default_sorting();
 	if (cmdmode == 'l') {
 		int ret;
 		if (column_active(colopts)) {
@@ -658,10 +400,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 			copts.padding = 2;
 			run_column_filter(colopts, &copts);
 		}
-		if (filter.lines != -1 && tag_sort)
-			die(_("--sort and -n are incompatible"));
 		filter.name_patterns = argv;
-		ret = list_tags(&filter, tag_sort);
+		ret = list_tags(&filter, sorting);
 		if (column_active(colopts))
 			stop_column_filter();
 		return ret;
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index d31788c..84153ef 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1462,13 +1462,7 @@ test_expect_success 'invalid sort parameter on command line' '
 
 test_expect_success 'invalid sort parameter in configuratoin' '
 	git config tag.sort "v:notvalid" &&
-	git tag -l "foo*" >actual &&
-	cat >expect <<-\EOF &&
-	foo1.10
-	foo1.3
-	foo1.6
-	EOF
-	test_cmp expect actual
+	test_must_fail git tag -l "foo*"
 '
 
 test_expect_success 'version sort with prerelease reordering' '
-- 
2.5.0

[PATCH v15 12/13] tag.c: implement '--format' option

[Karthik Nayak]

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

Add tests and documentation for the same.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 Documentation/git-tag.txt |  8 +++++++-
 builtin/tag.c             | 22 +++++++++++++---------
 t/t7004-tag.sh            | 12 ++++++++++++
 3 files changed, 32 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 3ac4a96..0c7f4e6 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -13,7 +13,8 @@ SYNOPSIS
 	<tagname> [<commit> | <object>]
 'git tag' -d <tagname>...
 'git tag' [-n[<num>]] -l [--contains <commit>] [--points-at <object>]
-	[--column[=<options>] | --no-column] [--create-reflog] [--sort=<key>] [<pattern>...]
+	[--column[=<options>] | --no-column] [--create-reflog] [--sort=<key>]
+	[--format=<format>] [<pattern>...]
 'git tag' -v <tagname>...
 
 DESCRIPTION
@@ -158,6 +159,11 @@ This option is only applicable when listing tags without annotation lines.
 	The object that the new tag will refer to, usually a commit.
 	Defaults to HEAD.
 
+<format>::
+	A string that interpolates `%(fieldname)` from the object
+	pointed at by a ref being shown.  The format is the same as
+	that of linkgit:git-for-each-ref[1].  When unspecified,
+	defaults to `%(refname:short)`.
 
 CONFIGURATION
 -------------
diff --git a/builtin/tag.c b/builtin/tag.c
index 2d348f4..dc4494d 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -23,17 +23,17 @@ static const char * const git_tag_usage[] = {
 	N_("git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] <tagname> [<head>]"),
 	N_("git tag -d <tagname>..."),
 	N_("git tag -l [-n[<num>]] [--contains <commit>] [--points-at <object>]"
-		"\n\t\t[<pattern>...]"),
+		"\n\t\t[--format=<format>] [<pattern>...]"),
 	N_("git tag -v <tagname>..."),
 	NULL
 };
 
 static unsigned int colopts;
 
-static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting)
+static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, const char *format)
 {
 	struct ref_array array;
-	char *format, *to_free = NULL;
+	char *to_free = NULL;
 	int i;
 
 	memset(&array, 0, sizeof(array));
@@ -41,11 +41,13 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting)
 	if (filter->lines == -1)
 		filter->lines = 0;
 
-	if (filter->lines)
-		format = to_free = xstrfmt("%%(align:15,left)%%(refname:short)%%(end) %%(contents:lines=%d)",
-					   filter->lines);
-	else
-		format = "%(refname:short)";
+	if (!format) {
+		if (filter->lines)
+			format = to_free = xstrfmt("%%(align:15,left)%%(refname:short)%%(end) %%(contents:lines=%d)",
+						   filter->lines);
+		else
+			format = "%(refname:short)";
+	}
 
 	verify_ref_format(format);
 	filter_refs(&array, filter, FILTER_REFS_TAGS);
@@ -329,6 +331,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	struct strbuf err = STRBUF_INIT;
 	struct ref_filter filter;
 	static struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;
+	const char *format = NULL;
 	struct option options[] = {
 		OPT_CMDMODE('l', "list", &cmdmode, N_("list tag names"), 'l'),
 		{ OPTION_INTEGER, 'n', NULL, &filter.lines, N_("n"),
@@ -361,6 +364,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 			OPTION_CALLBACK, 0, "points-at", &filter.points_at, N_("object"),
 			N_("print only tags of the object"), 0, parse_opt_object_name
 		},
+		OPT_STRING(  0 , "format", &format, N_("format"), N_("format to use for the output")),
 		OPT_END()
 	};
 
@@ -401,7 +405,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 			run_column_filter(colopts, &copts);
 		}
 		filter.name_patterns = argv;
-		ret = list_tags(&filter, sorting);
+		ret = list_tags(&filter, sorting, format);
 		if (column_active(colopts))
 			stop_column_filter();
 		return ret;
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 84153ef..8987fb1 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1519,4 +1519,16 @@ EOF"
 	test_cmp expect actual
 '
 
+test_expect_success '--format should list tags as per format given' '
+	cat >expect <<-\EOF &&
+	refname : refs/tags/foo1.10
+	refname : refs/tags/foo1.3
+	refname : refs/tags/foo1.6
+	refname : refs/tags/foo1.6-rc1
+	refname : refs/tags/foo1.6-rc2
+	EOF
+	git tag -l --format="refname : %(refname)" "foo*" >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.5.0

[PATCH v15 13/13] tag.c: implement '--merged' and '--no-merged' options

[Karthik Nayak]

Use 'ref-filter' APIs to implement the '--merged' and '--no-merged'
options into 'tag.c'. The '--merged' option lets the user to only list
tags merged into the named commit. The '--no-merged' option lets the
user to only list tags not merged into the named commit.  If no object
is provided it assumes HEAD as the 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-tag.txt |  7 ++++++-
 builtin/tag.c             |  6 +++++-
 t/t7004-tag.sh            | 27 +++++++++++++++++++++++++++
 3 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 0c7f4e6..3803bf7 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -14,7 +14,7 @@ SYNOPSIS
 'git tag' -d <tagname>...
 'git tag' [-n[<num>]] -l [--contains <commit>] [--points-at <object>]
 	[--column[=<options>] | --no-column] [--create-reflog] [--sort=<key>]
-	[--format=<format>] [<pattern>...]
+	[--format=<format>] [--[no-]merged [<commit>]] [<pattern>...]
 'git tag' -v <tagname>...
 
 DESCRIPTION
@@ -165,6 +165,11 @@ This option is only applicable when listing tags without annotation lines.
 	that of linkgit:git-for-each-ref[1].  When unspecified,
 	defaults to `%(refname:short)`.
 
+--[no-]merged [<commit>]::
+	Only list tags whose tips are reachable, or not reachable
+	if '--no-merged' is used, from the specified commit ('HEAD'
+	if not specified).
+
 CONFIGURATION
 -------------
 By default, 'git tag' in sign-with-default mode (-s) will use your
diff --git a/builtin/tag.c b/builtin/tag.c
index dc4494d..9fa1400 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -23,7 +23,7 @@ static const char * const git_tag_usage[] = {
 	N_("git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>] <tagname> [<head>]"),
 	N_("git tag -d <tagname>..."),
 	N_("git tag -l [-n[<num>]] [--contains <commit>] [--points-at <object>]"
-		"\n\t\t[--format=<format>] [<pattern>...]"),
+		"\n\t\t[--format=<format>] [--[no-]merged [<commit>]] [<pattern>...]"),
 	N_("git tag -v <tagname>..."),
 	NULL
 };
@@ -358,6 +358,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 		OPT_COLUMN(0, "column", &colopts, N_("show tag list in columns")),
 		OPT_CONTAINS(&filter.with_commit, N_("print only tags that contain the commit")),
 		OPT_WITH(&filter.with_commit, N_("print only tags that contain the commit")),
+		OPT_MERGED(&filter, N_("print only tags that are merged")),
+		OPT_NO_MERGED(&filter, N_("print only tags that are not merged")),
 		OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"),
 			     N_("field name to sort on"), &parse_opt_ref_sorting),
 		{
@@ -416,6 +418,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 		die(_("--contains option is only allowed with -l."));
 	if (filter.points_at.nr)
 		die(_("--points-at option is only allowed with -l."));
+	if (filter.merge_commit)
+		die(_("--merged and --no-merged option are only allowed with -l"));
 	if (cmdmode == 'd')
 		return for_each_tag_name(argv, delete_tag);
 	if (cmdmode == 'v')
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 8987fb1..3dd2f51 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1531,4 +1531,31 @@ test_expect_success '--format should list tags as per format given' '
 	test_cmp expect actual
 '
 
+test_expect_success 'setup --merged test tags' '
+	git tag mergetest-1 HEAD~2 &&
+	git tag mergetest-2 HEAD~1 &&
+	git tag mergetest-3 HEAD
+'
+
+test_expect_success '--merged cannot be used in non-list mode' '
+	test_must_fail git tag --merged=mergetest-2 foo
+'
+
+test_expect_success '--merged shows merged tags' '
+	cat >expect <<-\EOF &&
+	mergetest-1
+	mergetest-2
+	EOF
+	git tag -l --merged=mergetest-2 mergetest-* >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--no-merged show unmerged tags' '
+	cat >expect <<-\EOF &&
+	mergetest-3
+	EOF
+	git tag -l --no-merged=mergetest-2 mergetest-* >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.5.0

Re: [PATCH v15 05/13] ref-filter: implement an `align` atom

[Junio C Hamano]

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

> We have an `at_end` function for each element of the stack which is to
> be called when the `end` atom is encountered. Using this we implement
> the aling_handler() for the `align` atom, this aligns the final strbuf

align_handler().

>  struct ref_formatting_stack {
>  	struct ref_formatting_stack *prev;
>  	struct strbuf output;
> +	void (*at_end)(struct ref_formatting_stack *stack);
> +	void *cb_data;
>  };

s/cb_data/at_end_data/ or something, as this is not really about a
function callback.  Imagine a fictional future where you add a new
functions at_middle---the readers cannot tell what cb_data is about
at that point.

> +static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
> +{
> +	struct ref_formatting_stack *current = state->stack;
> +	struct strbuf s = STRBUF_INIT;
> +
> +	if (!current->at_end)
> +		die(_("format: `end` atom used without a supporting atom"));

Not a show-stopper, but we may need some wordsmithing for "a
supporting atom" here; an end-user would not know what it is.

> +	current->at_end(current);
> +
> +	/*
> +	 * Perform quote formatting when the stack element is that of
> +	 * a modifier atom and right above the first stack element.
> +	 */
> +	if (!state->stack->prev->prev) {
> +		quote_formatting(&s, current->output.buf, state->quote_style);
> +		strbuf_swap(&current->output, &s);
> +	}
> +	strbuf_release(&s);
> +	pop_stack_element(&state->stack);
> +}

Nice.

> @@ -687,6 +748,7 @@ static void populate_value(struct ref_array_item *ref)
>  		int deref = 0;
>  		const char *refname;
>  		const char *formatp;
> +		const char *valp;
>  		struct branch *branch = NULL;
>  
>  		v->handler = append_atom;
> @@ -754,6 +816,42 @@ static void populate_value(struct ref_array_item *ref)
>  			else
>  				v->s = " ";
>  			continue;
> +		} else if (skip_prefix(name, "align", &valp)) {

This looked as if you are willing to take %(align) in addition to
%(align:...), but...

> +			struct align *align = &v->align;
> +			struct strbuf **s;
> +
> +			if (valp[0] != ':')
> +				die(_("format: usage %%(align:<width>,<position>)"));

... apparently that is not what is happening.  Why not skip "align:"
with colon as the prefix, then?

> +			else
> +				valp++;
> +			s = strbuf_split_str(valp, ',', 0);
> +
> +			/* If the position is given trim the ',' from the first strbuf */
> +			if (s[1])
> +				strbuf_setlen(s[0], s[0]->len - 1);
> +
> +			if (strtoul_ui(s[0]->buf, 10, &align->width))
> +				die(_("positive width expected align:%s"), s[0]->buf);
> +
> +			if (!s[1])
> +				align->position = ALIGN_LEFT;
> +			else if (!strcmp(s[1]->buf, "left"))
> +				align->position = ALIGN_LEFT;
> +			else if (!strcmp(s[1]->buf, "right"))
> +				align->position = ALIGN_RIGHT;
> +			else if (!strcmp(s[1]->buf, "middle"))
> +				align->position = ALIGN_MIDDLE;
> +			else
> +				die(_("improper format entered align:%s"), s[1]->buf);

This does not reject %(align:40,left,junk), no?  Before "s[1] does
not exist so default to left align", you would want

	if (s[2])
		die("align:width,position followed by garbage: ,%s", s[2]->buf);

I have a few observations; these are not necessarily we would want
to change in the scope of this series, though.

 - The design of strbuf_split_buf API feels screwy.  A variant of
   this function that strips the terminator at the end would be what
   most callers would want.  Granted, leaving the terminator in the
   resulting buffer does let the caller tell if the input ended with
   an incomplete line that lacked the final terminator, but for all
   s[i] for 0 <= i < N-1 where s[N] is the first element that is
   NULL, they must end with the terminator---otherwise the elements
   would not have split into the array in the first place.  "By
   keeping the terminator, you can tell which one of the possible
   terminators was used" could be a valid rationale for the API if
   the function allowed more than one terminators, but that does not
   apply here, either.

 - I would have expected the above code to look more like this:

	width = -1; position = ALIGN_LEFT;
	s = strbuf_split_str(valp, ',', 0);
	while (*s) {
		if (s[1])
                	strbuf_setlen(*s, *s->len - 1);
		if (!strtoul_ui(*s->buf, 10, &width))
                	; /* parsed width successfully */
                else if (!strcmp(*s->buf, "left"))
                	position = ALIGN_LEFT;
		else if ...
		else
                	die("unknown parameter: %s", *s->buf);
		s++;			
	}
	if (width < 0)
		... perhaps set to the default width, or
                ... call die() complaining that you did not see
                ... an explicit width specified

   Doing the code that way, it would be more obvious that a way to
   extend the parser to accept forms like

	%(align:width=40,position=left)

   is by adding "keyword=value" parser before the fallbacks for
   short-hand, i.e. "if looks like number" and everything else.
        

Re: [PATCH v15 06/13] ref-filter: add option to filter out tags, branches and remotes

[Junio C Hamano]

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

> +	if (!filter->kind)
>  		die("filter_refs: invalid type");
> +	else {
> +		if (filter->kind == FILTER_REFS_BRANCHES)
> +			ret = for_each_fullref_in("refs/heads/", ref_filter_handler, &ref_cbdata, broken);
> +		else if (filter->kind == FILTER_REFS_REMOTES)
> +			ret = for_each_fullref_in("refs/remotes/", ref_filter_handler, &ref_cbdata, broken);
> +		else if (filter->kind == FILTER_REFS_TAGS)
> +			ret = for_each_fullref_in("refs/tags/", ref_filter_handler, &ref_cbdata, broken);
> +		else if (filter->kind & FILTER_REFS_ALL)
> +			ret = for_each_fullref_in("", ref_filter_handler, &ref_cbdata, broken);

This if/else if/else if/ cascade and ...

> +		if (!ret && (filter->kind & FILTER_REFS_DETACHED_HEAD))
> +			head_ref(ref_filter_handler, &ref_cbdata);
> +	}
> +
>  
>  	/*  Filters that need revision walking */
>  	if (filter->merge_commit)
> diff --git a/ref-filter.h b/ref-filter.h
> index 45026d0..0913ba9 100644
> --- a/ref-filter.h
> +++ b/ref-filter.h
> @@ -13,8 +13,15 @@
>  #define QUOTE_PYTHON 4
>  #define QUOTE_TCL 8
>  
> -#define FILTER_REFS_INCLUDE_BROKEN 0x1
> -#define FILTER_REFS_ALL 0x2
> +#define FILTER_REFS_INCLUDE_BROKEN 0x0001
> +#define FILTER_REFS_TAGS           0x0002
> +#define FILTER_REFS_BRANCHES       0x0004
> +#define FILTER_REFS_REMOTES        0x0008
> +#define FILTER_REFS_OTHERS         0x0010

... the bit assignment here conceptually do not mesh well with each
other.  These bits look as if I can ask for both tags and branches
by passing 0x0006, and if the code above were

	empty the result set;
	if (filter->kind & FILTER_REFS_BRANCHES)
        	add in things from refs/heads/ to the result set;
	if (filter->kind & FILTER_REFS_TAGS)                
        	add in things from refs/tags/ to the result set;
	...

without "else if", that would conceptually make sense.

Alternatively, if the code does not (and will not ever) support such
an arbitrary mixing of bits and intends to only allow "one of
BRANCHES, REMOTES, TAGS or ALL, and no other choice", then it is
wrong to pretend as if they can be mixed by defining the constant
with values with non-overlapping bit patterns.  If you defined these
constants as

#define FILTER_REFS_TAGS           100
#define FILTER_REFS_BRANCHES       101
#define FILTER_REFS_REMOTES        102
#define FILTER_REFS_OTHERS         103

then nobody would be think that the function can collect both tags
and branches by passing FILTER_REFS_TAGS|FILTER_REFS_BRANCHES.

The former, i.e. keep the bits distinct and allow them to be OR'ed
together by updating the code to allow such callers, would be more
preferrable, of course.

Re: [PATCH v15 06/13] ref-filter: add option to filter out tags, branches and remotes

[Karthik Nayak]

On Wed, Sep 2, 2015 at 3:00 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> +     if (!filter->kind)
>>               die("filter_refs: invalid type");
>> +     else {
>> +             if (filter->kind == FILTER_REFS_BRANCHES)
>> +                     ret = for_each_fullref_in("refs/heads/", ref_filter_handler, &ref_cbdata, broken);
>> +             else if (filter->kind == FILTER_REFS_REMOTES)
>> +                     ret = for_each_fullref_in("refs/remotes/", ref_filter_handler, &ref_cbdata, broken);
>> +             else if (filter->kind == FILTER_REFS_TAGS)
>> +                     ret = for_each_fullref_in("refs/tags/", ref_filter_handler, &ref_cbdata, broken);
>> +             else if (filter->kind & FILTER_REFS_ALL)
>> +                     ret = for_each_fullref_in("", ref_filter_handler, &ref_cbdata, broken);
>
> This if/else if/else if/ cascade and ...

Did you notice the "==" for others and "&" for the ALL?

>
>> +             if (!ret && (filter->kind & FILTER_REFS_DETACHED_HEAD))
>> +                     head_ref(ref_filter_handler, &ref_cbdata);
>> +     }
>> +
>>
>>       /*  Filters that need revision walking */
>>       if (filter->merge_commit)
>> diff --git a/ref-filter.h b/ref-filter.h
>> index 45026d0..0913ba9 100644
>> --- a/ref-filter.h
>> +++ b/ref-filter.h
>> @@ -13,8 +13,15 @@
>>  #define QUOTE_PYTHON 4
>>  #define QUOTE_TCL 8
>>
>> -#define FILTER_REFS_INCLUDE_BROKEN 0x1
>> -#define FILTER_REFS_ALL 0x2
>> +#define FILTER_REFS_INCLUDE_BROKEN 0x0001
>> +#define FILTER_REFS_TAGS           0x0002
>> +#define FILTER_REFS_BRANCHES       0x0004
>> +#define FILTER_REFS_REMOTES        0x0008
>> +#define FILTER_REFS_OTHERS         0x0010
>
> ... the bit assignment here conceptually do not mesh well with each
> other.  These bits look as if I can ask for both tags and branches
> by passing 0x0006, and if the code above were
>
>         empty the result set;
>         if (filter->kind & FILTER_REFS_BRANCHES)
>                 add in things from refs/heads/ to the result set;
>         if (filter->kind & FILTER_REFS_TAGS)
>                 add in things from refs/tags/ to the result set;
>         ...
>
> without "else if", that would conceptually make sense.
>
> Alternatively, if the code does not (and will not ever) support such
> an arbitrary mixing of bits and intends to only allow "one of
> BRANCHES, REMOTES, TAGS or ALL, and no other choice", then it is
> wrong to pretend as if they can be mixed by defining the constant
> with values with non-overlapping bit patterns.  If you defined these
> constants as

Hmm, The code does support mixing if you see, whenever we mix and match
these bits (consider 0x0006) we satisfy the ` else if (filter->kind &
FILTER_REFS_ALL)`
condition. Which would then go through the entire set of refs and pick
only refs we
need using filter_ref_kind(). This may seem a little confusing but this avoids
ref type filtering when we do not mix bits up.

>
> #define FILTER_REFS_TAGS           100
> #define FILTER_REFS_BRANCHES       101
> #define FILTER_REFS_REMOTES        102
> #define FILTER_REFS_OTHERS         103
>
> then nobody would be think that the function can collect both tags
> and branches by passing FILTER_REFS_TAGS|FILTER_REFS_BRANCHES.
>
> The former, i.e. keep the bits distinct and allow them to be OR'ed
> together by updating the code to allow such callers, would be more
> preferrable, of course.
>

Just to confirm, I even changed the function call of for-each-ref to
    filter_refs(&array, &filter, FILTER_REFS_BRANCHES |
FILTER_REFS_TAGS | FILTER_REFS_INCLUDE_BROKEN);
and it printed only "refs/heads/" and "refs/tags/".
Thanks :)

-- 
Regards,
Karthik Nayak

Re: [PATCH v15 06/13] ref-filter: add option to filter out tags, branches and remotes

[Junio C Hamano]

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

>>> +             if (filter->kind == FILTER_REFS_BRANCHES)
>>> + ret = for_each_fullref_in("refs/heads/", ref_filter_handler,
>>> &ref_cbdata, broken);
>>> +             else if (filter->kind == FILTER_REFS_REMOTES)
>>> + ret = for_each_fullref_in("refs/remotes/", ref_filter_handler,
>>> &ref_cbdata, broken);
>>> +             else if (filter->kind == FILTER_REFS_TAGS)
>>> + ret = for_each_fullref_in("refs/tags/", ref_filter_handler,
>>> &ref_cbdata, broken);
>>> +             else if (filter->kind & FILTER_REFS_ALL)
>>> + ret = for_each_fullref_in("", ref_filter_handler, &ref_cbdata,
>>> broken);
>>
>> This if/else if/else if/ cascade and ...
>
> Did you notice the "==" for others and "&" for the ALL?

I didn't.  Thanks for pointing it out.

So the point of the earlier part of the cascade is to optimize for
common cases?  If that is the case, it probably deserves some
commenting.  I also suspect that a table-based control might be
easier to maintain, but that kind of change might fall into the
category of premature optimization.

Re: [PATCH v15 05/13] ref-filter: implement an `align` atom

[Matthieu Moy]

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

> +test_expect_success 'alignment with format quote' '
> +	cat >expect <<-EOF &&
> +	refname is ${sq}           ${sq}\\${sq}${sq}master${sq}\\${sq}${sq}           ${sq}|
> +	refname is ${sq}            ${sq}\\${sq}${sq}side${sq}\\${sq}${sq}            ${sq}|
> +	refname is ${sq}          ${sq}\\${sq}${sq}odd/spot${sq}\\${sq}${sq}          ${sq}|
> +	refname is ${sq}         ${sq}\\${sq}${sq}double-tag${sq}\\${sq}${sq}         ${sq}|
> +	refname is ${sq}            ${sq}\\${sq}${sq}four${sq}\\${sq}${sq}            ${sq}|
> +	refname is ${sq}            ${sq}\\${sq}${sq}one${sq}\\${sq}${sq}             ${sq}|
> +	refname is ${sq}         ${sq}\\${sq}${sq}signed-tag${sq}\\${sq}${sq}         ${sq}|
> +	refname is ${sq}           ${sq}\\${sq}${sq}three${sq}\\${sq}${sq}            ${sq}|
> +	refname is ${sq}            ${sq}\\${sq}${sq}two${sq}\\${sq}${sq}             ${sq}|
> +	EOF
> +	git for-each-ref --shell --format="refname is %(align:30,middle)${sq}%(refname:short)${sq}%(end)|" >actual &&
> +	test_cmp expect actual
> +'

Someone (Eric IIRC) suggested using double-quotes around the last
argument of test_expect_success. Even though I'm the one who suggested
this ${sq}, I have to agree with this suggestion. The result looks like

test_expect_success 'alignment with quote' "
	cat >expect <<-\EOF &&
	refname is '               '\\'''master'\\''
	...
"

Because you used "" at the toplevel, ' is not a special character
anymore. You do have to be careful with \\ though, but adding before EOF
as I did should do the trick. Untested.

You don't have test for nested alignment with quotes. I think it
deserves to be tested, if only to cast in stone that the current
behavior is your intention.

Perhaps just adding --shell to the test below would be OK to avoid the
proliferation of tests :

> +test_expect_success 'nested alignment' '
> +	cat >expect <<-\EOF &&
> +	|         master               |
> +	|           side               |
> +	|       odd/spot               |
> +	|     double-tag               |
> +	|           four               |
> +	|            one               |
> +	|     signed-tag               |
> +	|          three               |
> +	|            two               |
> +	EOF
> +	git for-each-ref --format="|%(align:30,left)%(align:15,right)%(refname:short)%(end)%(end)|" >actual &&
> +	test_cmp expect actual
> +'
> +
>  test_done

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

Re: [PATCH v15 05/13] ref-filter: implement an `align` atom

[Matthieu Moy]

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

> --- a/ref-filter.c
> +++ b/ref-filter.c

> @@ -163,9 +174,28 @@ static void quote_formatting(struct strbuf *s, const char *str, int quote_style)
>  	}
>  }
>  
> +static void align_handler(struct ref_formatting_stack *stack)

Perhaps name it end_align_handler, to make the difference with
align_atom_handler clear.

Also, I think moving this function to be right next to
align_atom_handler in the code would make this more readable.

> @@ -754,6 +816,42 @@ static void populate_value(struct ref_array_item *ref)
>  			else
>  				v->s = " ";
>  			continue;
> +		} else if (skip_prefix(name, "align", &valp)) {
> +			struct align *align = &v->align;
> +			struct strbuf **s;
> +
> +			if (valp[0] != ':')
> +				die(_("format: usage %%(align:<width>,<position>)"));
> +			else
> +				valp++;

I agree with Junio that skip_prefix(name, "align:" ...) is simpler for
the same thing.

> +	if (state.stack->prev)
> +		die(_("format: `end` atom missing"));

Perhaps spell it %(end) instead of just end.

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

Re: [PATCH v15 07/13] ref-filter: add support for %(contents:lines=X)

[Matthieu Moy]

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

> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -185,6 +185,10 @@ static enum contains_result contains(struct commit *candidate,
>  	return contains_test(candidate, want);
>  }
>  
> +/*
> + * Currently modified and used in ref-filter as append_lines(), will
> + * eventually be removed as we port tag.c to use ref-filter APIs.
> + */
>  static void show_tag_lines(const struct object_id *oid, int lines)

I would rather have one "cut and paste" patch followed by a "modify and
use" patch for review.

As-is, reading the patch doesn't tell me what change you did.

That said, I did get this information in the interdiff, so I won't
insist on that.

> +static void append_lines(struct strbuf *out, const char *buf, unsigned long size, int lines)
> +{
> +	int i;
> +	const char *sp, *eol;
> +	size_t len;
> +
> +	if ((sp = strstr(buf, "\n\n")) && (sp <= buf + size))
> +		size += 2;

Why is this "size += 2" needed?

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

Re: [PATCH v15 11/13] tag.c: use 'ref-filter' APIs

[Matthieu Moy]

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

> +	if (filter->lines)
> +		format = to_free = xstrfmt("%%(align:15,left)%%(refname:short)%%(end) %%(contents:lines=%d)",
> +					   filter->lines);
> +	else
> +		format = "%(refname:short)";
> +
> +	verify_ref_format(format);
> +	filter_refs(&array, filter, FILTER_REFS_TAGS);
> +	ref_array_sort(sorting, &array);
> +
> +	for (i = 0; i < array.nr; i++)
> +		show_ref_array_item(array.items[i], format, 0);

Nice :-).

(I'd cut the string argument to xstrfmt after "%(end)" to avoid long
line)

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

Re: [PATCH v15 05/13] ref-filter: implement an `align` atom

[Karthik Nayak]

On Wed, Sep 2, 2015 at 2:49 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> We have an `at_end` function for each element of the stack which is to
>> be called when the `end` atom is encountered. Using this we implement
>> the aling_handler() for the `align` atom, this aligns the final strbuf
>
> align_handler().

Will change.

>
>>  struct ref_formatting_stack {
>>       struct ref_formatting_stack *prev;
>>       struct strbuf output;
>> +     void (*at_end)(struct ref_formatting_stack *stack);
>> +     void *cb_data;
>>  };
>
> s/cb_data/at_end_data/ or something, as this is not really about a
> function callback.  Imagine a fictional future where you add a new
> functions at_middle---the readers cannot tell what cb_data is about
> at that point.
>

Makes sense will change.

>> +static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
>> +{
>> +     struct ref_formatting_stack *current = state->stack;
>> +     struct strbuf s = STRBUF_INIT;
>> +
>> +     if (!current->at_end)
>> +             die(_("format: `end` atom used without a supporting atom"));
>
> Not a show-stopper, but we may need some wordsmithing for "a
> supporting atom" here; an end-user would not know what it is.
>

Probably something like "format: `end` atom should only be
used with modifier atoms".

>> +     current->at_end(current);
>> +
>> +     /*
>> +      * Perform quote formatting when the stack element is that of
>> +      * a modifier atom and right above the first stack element.
>> +      */
>> +     if (!state->stack->prev->prev) {
>> +             quote_formatting(&s, current->output.buf, state->quote_style);
>> +             strbuf_swap(&current->output, &s);
>> +     }
>> +     strbuf_release(&s);
>> +     pop_stack_element(&state->stack);
>> +}
>
> Nice.
>
>> @@ -687,6 +748,7 @@ static void populate_value(struct ref_array_item *ref)
>>               int deref = 0;
>>               const char *refname;
>>               const char *formatp;
>> +             const char *valp;
>>               struct branch *branch = NULL;
>>
>>               v->handler = append_atom;
>> @@ -754,6 +816,42 @@ static void populate_value(struct ref_array_item *ref)
>>                       else
>>                               v->s = " ";
>>                       continue;
>> +             } else if (skip_prefix(name, "align", &valp)) {
>
> This looked as if you are willing to take %(align) in addition to
> %(align:...), but...
>
>> +                     struct align *align = &v->align;
>> +                     struct strbuf **s;
>> +
>> +                     if (valp[0] != ':')
>> +                             die(_("format: usage %%(align:<width>,<position>)"));
>
> ... apparently that is not what is happening.  Why not skip "align:"
> with colon as the prefix, then?
>

Cause we wanted to provide an error for usage of "%(ailgn)" without any
subvalues as such.

>> +                     else
>> +                             valp++;
>> +                     s = strbuf_split_str(valp, ',', 0);
>> +
>> +                     /* If the position is given trim the ',' from the first strbuf */
>> +                     if (s[1])
>> +                             strbuf_setlen(s[0], s[0]->len - 1);
>> +
>> +                     if (strtoul_ui(s[0]->buf, 10, &align->width))
>> +                             die(_("positive width expected align:%s"), s[0]->buf);
>> +
>> +                     if (!s[1])
>> +                             align->position = ALIGN_LEFT;
>> +                     else if (!strcmp(s[1]->buf, "left"))
>> +                             align->position = ALIGN_LEFT;
>> +                     else if (!strcmp(s[1]->buf, "right"))
>> +                             align->position = ALIGN_RIGHT;
>> +                     else if (!strcmp(s[1]->buf, "middle"))
>> +                             align->position = ALIGN_MIDDLE;
>> +                     else
>> +                             die(_("improper format entered align:%s"), s[1]->buf);
>
> This does not reject %(align:40,left,junk), no?  Before "s[1] does
> not exist so default to left align", you would want
>
>         if (s[2])
>                 die("align:width,position followed by garbage: ,%s", s[2]->buf);
>

Yea we should probably do that.

> I have a few observations; these are not necessarily we would want
> to change in the scope of this series, though.
>
>  - The design of strbuf_split_buf API feels screwy.  A variant of
>    this function that strips the terminator at the end would be what
>    most callers would want.  Granted, leaving the terminator in the
>    resulting buffer does let the caller tell if the input ended with
>    an incomplete line that lacked the final terminator, but for all
>    s[i] for 0 <= i < N-1 where s[N] is the first element that is
>    NULL, they must end with the terminator---otherwise the elements
>    would not have split into the array in the first place.  "By
>    keeping the terminator, you can tell which one of the possible
>    terminators was used" could be a valid rationale for the API if
>    the function allowed more than one terminators, but that does not
>    apply here, either.
>
>  - I would have expected the above code to look more like this:
>
>         width = -1; position = ALIGN_LEFT;
>         s = strbuf_split_str(valp, ',', 0);
>         while (*s) {
>                 if (s[1])
>                         strbuf_setlen(*s, *s->len - 1);
>                 if (!strtoul_ui(*s->buf, 10, &width))
>                         ; /* parsed width successfully */
>                 else if (!strcmp(*s->buf, "left"))
>                         position = ALIGN_LEFT;
>                 else if ...
>                 else
>                         die("unknown parameter: %s", *s->buf);
>                 s++;
>         }
>         if (width < 0)
>                 ... perhaps set to the default width, or
>                 ... call die() complaining that you did not see
>                 ... an explicit width specified
>
>    Doing the code that way, it would be more obvious that a way to
>    extend the parser to accept forms like
>
>         %(align:width=40,position=left)
>
>    is by adding "keyword=value" parser before the fallbacks for
>    short-hand, i.e. "if looks like number" and everything else.
>

I'll keep this in mind, probably work on this later after this series is done.
Thanks

-- 
Regards,
Karthik Nayak

Re: [PATCH v15 06/13] ref-filter: add option to filter out tags, branches and remotes

[Karthik Nayak]

On Wed, Sep 2, 2015 at 9:45 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>>>> +             if (filter->kind == FILTER_REFS_BRANCHES)
>>>> + ret = for_each_fullref_in("refs/heads/", ref_filter_handler,
>>>> &ref_cbdata, broken);
>>>> +             else if (filter->kind == FILTER_REFS_REMOTES)
>>>> + ret = for_each_fullref_in("refs/remotes/", ref_filter_handler,
>>>> &ref_cbdata, broken);
>>>> +             else if (filter->kind == FILTER_REFS_TAGS)
>>>> + ret = for_each_fullref_in("refs/tags/", ref_filter_handler,
>>>> &ref_cbdata, broken);
>>>> +             else if (filter->kind & FILTER_REFS_ALL)
>>>> + ret = for_each_fullref_in("", ref_filter_handler, &ref_cbdata,
>>>> broken);
>>>
>>> This if/else if/else if/ cascade and ...
>>
>> Did you notice the "==" for others and "&" for the ALL?
>
> I didn't.  Thanks for pointing it out.
>
> So the point of the earlier part of the cascade is to optimize for
> common cases?  If that is the case, it probably deserves some
> commenting.  I also suspect that a table-based control might be
> easier to maintain, but that kind of change might fall into the
> category of premature optimization.

Yes, thats the point.
Will add a comment, thanks :)

-- 
Regards,
Karthik Nayak

Re: [PATCH v15 05/13] ref-filter: implement an `align` atom

[Karthik Nayak]

On Wed, Sep 2, 2015 at 2:11 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> +test_expect_success 'alignment with format quote' '
>> +     cat >expect <<-EOF &&
>> +     refname is ${sq}           ${sq}\\${sq}${sq}master${sq}\\${sq}${sq}           ${sq}|
>> +     refname is ${sq}            ${sq}\\${sq}${sq}side${sq}\\${sq}${sq}            ${sq}|
>> +     refname is ${sq}          ${sq}\\${sq}${sq}odd/spot${sq}\\${sq}${sq}          ${sq}|
>> +     refname is ${sq}         ${sq}\\${sq}${sq}double-tag${sq}\\${sq}${sq}         ${sq}|
>> +     refname is ${sq}            ${sq}\\${sq}${sq}four${sq}\\${sq}${sq}            ${sq}|
>> +     refname is ${sq}            ${sq}\\${sq}${sq}one${sq}\\${sq}${sq}             ${sq}|
>> +     refname is ${sq}         ${sq}\\${sq}${sq}signed-tag${sq}\\${sq}${sq}         ${sq}|
>> +     refname is ${sq}           ${sq}\\${sq}${sq}three${sq}\\${sq}${sq}            ${sq}|
>> +     refname is ${sq}            ${sq}\\${sq}${sq}two${sq}\\${sq}${sq}             ${sq}|
>> +     EOF
>> +     git for-each-ref --shell --format="refname is %(align:30,middle)${sq}%(refname:short)${sq}%(end)|" >actual &&
>> +     test_cmp expect actual
>> +'
>
> Someone (Eric IIRC) suggested using double-quotes around the last
> argument of test_expect_success. Even though I'm the one who suggested
> this ${sq}, I have to agree with this suggestion. The result looks like
>
> test_expect_success 'alignment with quote' "
>         cat >expect <<-\EOF &&
>         refname is '               '\\'''master'\\''
>         ...
> "
>
> Because you used "" at the toplevel, ' is not a special character
> anymore. You do have to be careful with \\ though, but adding before EOF
> as I did should do the trick. Untested.
>

Oops! I missed that out, thanks for bringing that up again, will do it.

> You don't have test for nested alignment with quotes. I think it
> deserves to be tested, if only to cast in stone that the current
> behavior is your intention.
>
> Perhaps just adding --shell to the test below would be OK to avoid the
> proliferation of tests :
>

Makes sense will add it to the following test.

>> +test_expect_success 'nested alignment' '
>> +     cat >expect <<-\EOF &&
>> +     |         master               |
>> +     |           side               |
>> +     |       odd/spot               |
>> +     |     double-tag               |
>> +     |           four               |
>> +     |            one               |
>> +     |     signed-tag               |
>> +     |          three               |
>> +     |            two               |
>> +     EOF
>> +     git for-each-ref --format="|%(align:30,left)%(align:15,right)%(refname:short)%(end)%(end)|" >actual &&
>> +     test_cmp expect actual
>> +'
>> +
>>  test_done
>
> --
> Matthieu Moy
> http://www-verimag.imag.fr/~moy/



-- 
Regards,
Karthik Nayak

Re: [PATCH v15 05/13] ref-filter: implement an `align` atom

[Karthik Nayak]

On Wed, Sep 2, 2015 at 2:15 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>
>> @@ -163,9 +174,28 @@ static void quote_formatting(struct strbuf *s, const char *str, int quote_style)
>>       }
>>  }
>>
>> +static void align_handler(struct ref_formatting_stack *stack)
>
> Perhaps name it end_align_handler, to make the difference with
> align_atom_handler clear.
>
> Also, I think moving this function to be right next to
> align_atom_handler in the code would make this more readable.
>

will do.

>> @@ -754,6 +816,42 @@ static void populate_value(struct ref_array_item *ref)
>>                       else
>>                               v->s = " ";
>>                       continue;
>> +             } else if (skip_prefix(name, "align", &valp)) {
>> +                     struct align *align = &v->align;
>> +                     struct strbuf **s;
>> +
>> +                     if (valp[0] != ':')
>> +                             die(_("format: usage %%(align:<width>,<position>)"));
>> +                     else
>> +                             valp++;
>
> I agree with Junio that skip_prefix(name, "align:" ...) is simpler for
> the same thing.

Correct me if mistaken, but Junio wanted the first skip_prefix to
check for "align:" rather than "align", but that would mean we don't have
the error printing.

Are you sure we want to skip that?

>
>> +     if (state.stack->prev)
>> +             die(_("format: `end` atom missing"));
>
> Perhaps spell it %(end) instead of just end.
>

Yes, will do

-- 
Regards,
Karthik Nayak

Re: [PATCH v15 07/13] ref-filter: add support for %(contents:lines=X)

[Karthik Nayak]

On Wed, Sep 2, 2015 at 2:37 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> --- a/builtin/tag.c
>> +++ b/builtin/tag.c
>> @@ -185,6 +185,10 @@ static enum contains_result contains(struct commit *candidate,
>>       return contains_test(candidate, want);
>>  }
>>
>> +/*
>> + * Currently modified and used in ref-filter as append_lines(), will
>> + * eventually be removed as we port tag.c to use ref-filter APIs.
>> + */
>>  static void show_tag_lines(const struct object_id *oid, int lines)
>
> I would rather have one "cut and paste" patch followed by a "modify and
> use" patch for review.
>
> As-is, reading the patch doesn't tell me what change you did.
>
> That said, I did get this information in the interdiff, so I won't
> insist on that.

Its only borrowed slightly, so I don't really see the need for this.
But if you insist, we could do that .

>
>> +static void append_lines(struct strbuf *out, const char *buf, unsigned long size, int lines)
>> +{
>> +     int i;
>> +     const char *sp, *eol;
>> +     size_t len;
>> +
>> +     if ((sp = strstr(buf, "\n\n")) && (sp <= buf + size))
>> +             size += 2;
>
> Why is this "size += 2" needed?
>

We pass size as "sublen + bodylen - siglen" if there exists a "\n\n"
between sublen and bodylen this is not accounted for. hence we
add 2 here.

-- 
Regards,
Karthik Nayak

Re: [PATCH v15 05/13] ref-filter: implement an `align` atom

[Junio C Hamano]

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

>>> +             die(_("format: `end` atom used without a supporting atom"));
>>
>> Not a show-stopper, but we may need some wordsmithing for "a
>> supporting atom" here; an end-user would not know what it is.
>
> Probably something like "format: `end` atom should only be
> used with modifier atoms".

Between "supporting" and "modifier" I do not see much difference,
though.

>>> +             } else if (skip_prefix(name, "align", &valp)) {
>>
>> This looked as if you are willing to take %(align) in addition to
>> %(align:...), but...
>>
>>> +                     struct align *align = &v->align;
>>> +                     struct strbuf **s;
>>> +
>>> +                     if (valp[0] != ':')
>>> +                             die(_("format: usage %%(align:<width>,<position>)"));
>>
>> ... apparently that is not what is happening.  Why not skip "align:"
>> with colon as the prefix, then?
>
> Cause we wanted to provide an error for usage of "%(ailgn)" without any
> subvalues as such.

Wouldn't it be something that would be caught in the same codepath
as what catches %(unrecognized) in the format string?

Re: [PATCH v15 05/13] ref-filter: implement an `align` atom

[Karthik Nayak]

On Wed, Sep 2, 2015 at 8:31 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>>>> +             die(_("format: `end` atom used without a supporting atom"));
>>>
>>> Not a show-stopper, but we may need some wordsmithing for "a
>>> supporting atom" here; an end-user would not know what it is.
>>
>> Probably something like "format: `end` atom should only be
>> used with modifier atoms".
>
> Between "supporting" and "modifier" I do not see much difference,
> though.
>

I don't see how we could provide a better message, as %(end) atom
would be common to various atoms eventually.

>>>> +             } else if (skip_prefix(name, "align", &valp)) {
>>>
>>> This looked as if you are willing to take %(align) in addition to
>>> %(align:...), but...
>>>
>>>> +                     struct align *align = &v->align;
>>>> +                     struct strbuf **s;
>>>> +
>>>> +                     if (valp[0] != ':')
>>>> +                             die(_("format: usage %%(align:<width>,<position>)"));
>>>
>>> ... apparently that is not what is happening.  Why not skip "align:"
>>> with colon as the prefix, then?
>>
>> Cause we wanted to provide an error for usage of "%(ailgn)" without any
>> subvalues as such.
>
> Wouldn't it be something that would be caught in the same codepath
> as what catches %(unrecognized) in the format string?

No, since "align" is defined as an atom, in the valid_atom struct.
Changing it to "align:" would work, but that seems a little inconsistent
with the other atoms.

Hence --format="%(align)foo%(end)" would just result in the
"format: `end` atom used without a supporting atom" error being
displayed, hence confusing the user even more.

-- 
Regards,
Karthik Nayak

Re: [PATCH v15 11/13] tag.c: use 'ref-filter' APIs

[Junio C Hamano]

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

> +	if (filter->lines)
> +		format = to_free = xstrfmt("%%(align:15,left)%%(refname:short)%%(end) %%(contents:lines=%d)",
> +					   filter->lines);

I recall hearing that you were more in favor of

	"%(align:16)%(refname:short) %(end)%(contents:lines=4)"

somewhere in the earlier discussions?

Re: [PATCH v15 11/13] tag.c: use 'ref-filter' APIs

[Karthik Nayak]

On Wed, Sep 2, 2015 at 8:40 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> +     if (filter->lines)
>> +             format = to_free = xstrfmt("%%(align:15,left)%%(refname:short)%%(end) %%(contents:lines=%d)",
>> +                                        filter->lines);
>
> I recall hearing that you were more in favor of
>
>         "%(align:16)%(refname:short) %(end)%(contents:lines=4)"
>
> somewhere in the earlier discussions?

I did, but Matthieu suggested that this would be better!
Can't find a link to that.

-- 
Regards,
Karthik Nayak

Re: [PATCH v15 05/13] ref-filter: implement an `align` atom

[Junio C Hamano]

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

> On Wed, Sep 2, 2015 at 8:31 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>
>>>>> +             die(_("format: `end` atom used without a supporting atom"));
>>>>
>>>> Not a show-stopper, but we may need some wordsmithing for "a
>>>> supporting atom" here; an end-user would not know what it is.
>>>
>>> Probably something like "format: `end` atom should only be
>>> used with modifier atoms".
>>
>> Between "supporting" and "modifier" I do not see much difference,
>> though.
>
> I don't see how we could provide a better message, as %(end) atom
> would be common to various atoms eventually.

I said "not a show-stopper" without giving a suggestion exactly
because I didn't (and I still don't) think either you or I can come
up with a good wording ;-).  That is why the message was Cc'ed to
the list for others to comment.

>>> Cause we wanted to provide an error for usage of "%(ailgn)" without any
>>> subvalues as such.
>>
>> Wouldn't it be something that would be caught in the same codepath
>> as what catches %(unrecognized) in the format string?

One potential issue you have with prefix matching with "align" is
that you can never have a different atom whose name happens to begin
with that substring.  I think the best behaviour is for the higher
level parser to recognize %(ATOM) and %(ATOM:anything) and nothing
else for all ATOM in the valid atoms registry.  That would prevent
%(ATOMfoo) from being handled as part of handling ATOM.

And make the code consider %(ATOM) form as a short-hand for %(ATOM:)
that does not have customizations.  Conceptually %(refname) is a
%(refname:default).

For an atom like 'align' that does not have a reasonable default,
the parser for 'align' can notice that a required customization is
missing and give an error that is specific to 'align'.

So all calls in your code of the from

	... else if (skip_prefix(name, "align", &val)) {
        	...

should become a call to helper that does more than skip_prefix().

int match_atom_name(const char *name, const char *atom_name, char **val)
{
	char *body

       	if (!skip_prefix(name, atom_name, &body))
        	return 0; /* doesn't even begin with "align" */
	if (!body[0]) {
        	*val = NULL; /* %(align) and no customization */
                return 1;
	}
	if (body[0] != ':')
        	return 0; /* "alignfoo" is not "align" or "align:..." */
	*val = body + 1; /* "align:val" */
        return 1;
}

Re: [PATCH v15 05/13] ref-filter: implement an `align` atom

[Matthieu Moy]

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

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>>>> +             } else if (skip_prefix(name, "align", &valp)) {
>>>
>>> This looked as if you are willing to take %(align) in addition to
>>> %(align:...), but...
>>>
>>>> +                     struct align *align = &v->align;
>>>> +                     struct strbuf **s;
>>>> +
>>>> +                     if (valp[0] != ':')
>>>> +                             die(_("format: usage %%(align:<width>,<position>)"));
>>>
>>> ... apparently that is not what is happening.  Why not skip "align:"
>>> with colon as the prefix, then?
>>
>> Cause we wanted to provide an error for usage of "%(ailgn)" without any
>> subvalues as such.
>
> Wouldn't it be something that would be caught in the same codepath
> as what catches %(unrecognized) in the format string?

After thinking about it, I agree with Karthik: if we get the same
codepath to complain about %(nosuchatom) and %(align), then we won't be
able to provide an accurate error message. Or we would need to re-parse
the atom, notice that it's one we know about, i.e. redo what we're
already doing here.

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

Re: [PATCH v15 05/13] ref-filter: implement an `align` atom

[Matthieu Moy]

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

> On Wed, Sep 2, 2015 at 2:15 PM, Matthieu Moy
>>> @@ -754,6 +816,42 @@ static void populate_value(struct ref_array_item *ref)
>>>                       else
>>>                               v->s = " ";
>>>                       continue;
>>> +             } else if (skip_prefix(name, "align", &valp)) {
>>> +                     struct align *align = &v->align;
>>> +                     struct strbuf **s;
>>> +
>>> +                     if (valp[0] != ':')
>>> +                             die(_("format: usage %%(align:<width>,<position>)"));
>>> +                     else
>>> +                             valp++;
>>
>> I agree with Junio that skip_prefix(name, "align:" ...) is simpler for
>> the same thing.
>
> Correct me if mistaken, but Junio wanted the first skip_prefix to
> check for "align:" rather than "align", but that would mean we don't have
> the error printing.
>
> Are you sure we want to skip that?

See my other message: you convinced me ;-).

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

Re: [PATCH v15 05/13] ref-filter: implement an `align` atom

[Karthik Nayak]

On Wed, Sep 2, 2015 at 9:15 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> On Wed, Sep 2, 2015 at 8:31 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Karthik Nayak <karthik.188@gmail.com> writes:
>>>
>>>>>> +             die(_("format: `end` atom used without a supporting atom"));
>>>>>
>>>>> Not a show-stopper, but we may need some wordsmithing for "a
>>>>> supporting atom" here; an end-user would not know what it is.
>>>>
>>>> Probably something like "format: `end` atom should only be
>>>> used with modifier atoms".
>>>
>>> Between "supporting" and "modifier" I do not see much difference,
>>> though.
>>
>> I don't see how we could provide a better message, as %(end) atom
>> would be common to various atoms eventually.
>
> I said "not a show-stopper" without giving a suggestion exactly
> because I didn't (and I still don't) think either you or I can come
> up with a good wording ;-).  That is why the message was Cc'ed to
> the list for others to comment.

Haha okay.

>
>>>> Cause we wanted to provide an error for usage of "%(ailgn)" without any
>>>> subvalues as such.
>>>
>>> Wouldn't it be something that would be caught in the same codepath
>>> as what catches %(unrecognized) in the format string?
>
> One potential issue you have with prefix matching with "align" is
> that you can never have a different atom whose name happens to begin
> with that substring.  I think the best behaviour is for the higher
> level parser to recognize %(ATOM) and %(ATOM:anything) and nothing
> else for all ATOM in the valid atoms registry.  That would prevent
> %(ATOMfoo) from being handled as part of handling ATOM.
>
> And make the code consider %(ATOM) form as a short-hand for %(ATOM:)
> that does not have customizations.  Conceptually %(refname) is a
> %(refname:default).
>
> For an atom like 'align' that does not have a reasonable default,
> the parser for 'align' can notice that a required customization is
> missing and give an error that is specific to 'align'.
>
> So all calls in your code of the from
>
>         ... else if (skip_prefix(name, "align", &val)) {
>                 ...
>
> should become a call to helper that does more than skip_prefix().
>

This is what Eric suggested and we agreed on working on a general
solution to this problem eventually, seems like you beat me to it :)

Will incorporate this :)

> int match_atom_name(const char *name, const char *atom_name, char **val)
> {
>         char *body
>
>         if (!skip_prefix(name, atom_name, &body))
>                 return 0; /* doesn't even begin with "align" */
>         if (!body[0]) {
>                 *val = NULL; /* %(align) and no customization */
>                 return 1;
>         }
>         if (body[0] != ':')
>                 return 0; /* "alignfoo" is not "align" or "align:..." */
>         *val = body + 1; /* "align:val" */
>         return 1;
> }

Thanks.

-- 
Regards,
Karthik Nayak

Re: [PATCH v15 07/13] ref-filter: add support for %(contents:lines=X)

[Matthieu Moy]

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

> On Wed, Sep 2, 2015 at 2:37 PM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>
>>> --- a/builtin/tag.c
>>> +++ b/builtin/tag.c
>>> @@ -185,6 +185,10 @@ static enum contains_result contains(struct commit *candidate,
>>>       return contains_test(candidate, want);
>>>  }
>>>
>>> +/*
>>> + * Currently modified and used in ref-filter as append_lines(), will
>>> + * eventually be removed as we port tag.c to use ref-filter APIs.
>>> + */
>>>  static void show_tag_lines(const struct object_id *oid, int lines)
>>
>> I would rather have one "cut and paste" patch followed by a "modify and
>> use" patch for review.
>>
>> As-is, reading the patch doesn't tell me what change you did.
>>
>> That said, I did get this information in the interdiff, so I won't
>> insist on that.
>
> Its only borrowed slightly, so I don't really see the need for this.
> But if you insist, we could do that .

As you prefer.

Perhaps just adapt the comment to say "Currently redundant with
ref-filter'.c's append_line ...", but that's not important.

>>> +static void append_lines(struct strbuf *out, const char *buf, unsigned long size, int lines)
>>> +{
>>> +     int i;
>>> +     const char *sp, *eol;
>>> +     size_t len;
>>> +
>>> +     if ((sp = strstr(buf, "\n\n")) && (sp <= buf + size))
>>> +             size += 2;
>>
>> Why is this "size += 2" needed?
>>
>
> We pass size as "sublen + bodylen - siglen" if there exists a "\n\n"
> between sublen and bodylen this is not accounted for. hence we
> add 2 here.

That's too much magic for uncommented code. If this is really needed,
then thes explanations should go in a comment, and I think this logic
should be moved out of append_lines (if you read the comment above, the
function, it is actually lying about what the function does).

I think you can simplify this: you know where the buffer ends (bodypos +
bodylen) and where it starts (subpos), so you know the size: bodypos +
bodylen - subpos.

IOW, I think you can apply this:

--- a/ref-filter.c
+++ b/ref-filter.c
@@ -645,9 +645,6 @@ static void append_lines(struct strbuf *out, const char *buf, unsigned long size
 	const char *sp, *eol;
 	size_t len;
 
-	if ((sp = strstr(buf, "\n\n")) && (sp <= buf + size))
-		size += 2;
-
 	sp = buf;
 
 	for (i = 0; i < lines && sp < buf + size; i++) {
@@ -707,7 +704,7 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj
 			struct strbuf s = STRBUF_INIT;
 			if (strtoul_ui(valp, 10, &v->u.contents.lines))
 				die(_("positive width expected contents:lines=%s"), valp);
-			append_lines(&s, subpos, sublen + bodylen - siglen, v->u.contents.lines);
+			append_lines(&s, subpos, bodypos + bodylen - subpos, v->u.contents.lines);
 			v->s = strbuf_detach(&s, NULL);
 		}
 	}

(half-tested only)

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

Re: [PATCH v15 11/13] tag.c: use 'ref-filter' APIs

[Matthieu Moy]

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

> On Wed, Sep 2, 2015 at 8:40 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>
>>> +     if (filter->lines)
>>> +             format = to_free = xstrfmt("%%(align:15,left)%%(refname:short)%%(end) %%(contents:lines=%d)",
>>> +                                        filter->lines);
>>
>> I recall hearing that you were more in favor of
>>
>>         "%(align:16)%(refname:short) %(end)%(contents:lines=4)"
>>
>> somewhere in the earlier discussions?
>
> I did, but Matthieu suggested that this would be better!
> Can't find a link to that.

It was off-list. I don't have strong preference. My argument was:

> -            format = "%(align:16,left)%(refname:short)%(end)";
> +            format = "%(align:16,left)%(refname:short) %(end)";

I actually found the other more readable, as it reads "display the
refname aligned on 15 columns, and then a space", while this one reads
as "append a space to the refname, then align the result", which is
equivalent but requires more effort to understand.

But it's not important, keep this version if you prefer it like this.

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

Re: [PATCH v15 11/13] tag.c: use 'ref-filter' APIs

[Junio C Hamano]

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

> I actually found the other more readable, as it reads "display the
> refname aligned on 15 columns, and then a space",

FWIW, I too find the "15-col followed by a space" easier to
understand.  I was merely being curious when/why Karthik changed
preference, not objecting to the actual choice.

Re: [PATCH v15 05/13] ref-filter: implement an `align` atom

[Matthieu Moy]

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

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> On Wed, Sep 2, 2015 at 8:31 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Karthik Nayak <karthik.188@gmail.com> writes:
>>>
>>>>>> +             die(_("format: `end` atom used without a supporting atom"));
>>>>>
>>>>> Not a show-stopper, but we may need some wordsmithing for "a
>>>>> supporting atom" here; an end-user would not know what it is.
>>>>
>>>> Probably something like "format: `end` atom should only be
>>>> used with modifier atoms".
>>>
>>> Between "supporting" and "modifier" I do not see much difference,
>>> though.
>>
>> I don't see how we could provide a better message, as %(end) atom
>> would be common to various atoms eventually.
>
> I said "not a show-stopper" without giving a suggestion exactly
> because I didn't (and I still don't) think either you or I can come
> up with a good wording ;-).  That is why the message was Cc'ed to
> the list for others to comment.

I don't really have a better proposal either. What we really mean is
"%(end) requires an atom that requires to be paired with %(end)", but
that wouldn't really help. I prefer "supporting" to "modifier":
To me, %(color:red) can be called a "modifier" by I wouldn't call %(if)
a modifier. "Supporting" is vague, but less misleading to me.

Perhaps "corresponding"? (not convinced myself ...)

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

Re: [PATCH v15 05/13] ref-filter: implement an `align` atom

[Junio C Hamano]

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

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>
>>> On Wed, Sep 2, 2015 at 8:31 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>>> Karthik Nayak <karthik.188@gmail.com> writes:
>>>>
>>>>>>> +             die(_("format: `end` atom used without a supporting atom"));
>>>>>>
>>>>>> Not a show-stopper, but we may need some wordsmithing for "a
>>>>>> supporting atom" here; an end-user would not know what it is.
>>>>>
>>>>> Probably something like "format: `end` atom should only be
>>>>> used with modifier atoms".
>>>>
>>>> Between "supporting" and "modifier" I do not see much difference,
>>>> though.
>>>
>>> I don't see how we could provide a better message, as %(end) atom
>>> would be common to various atoms eventually.
>>
>> I said "not a show-stopper" without giving a suggestion exactly
>> because I didn't (and I still don't) think either you or I can come
>> up with a good wording ;-).  That is why the message was Cc'ed to
>> the list for others to comment.
>
> I don't really have a better proposal either. What we really mean is
> "%(end) requires an atom that requires to be paired with %(end)", but
> that wouldn't really help. I prefer "supporting" to "modifier":
> To me, %(color:red) can be called a "modifier" by I wouldn't call %(if)
> a modifier. "Supporting" is vague, but less misleading to me.
>
> Perhaps "corresponding"? (not convinced myself ...)

Yeah, it is like an open and a close parentheses that form a
matching pair.  "%(end) without a corresponding atom" (implying
"that opened the environment the %(end) attempts to close")?

We'd need to define what an atom is (or "supporting atom" for that
matter) and explain how nesting works in the documentation anyway,
and I'd expect we would gain definitions of a few terms we can use
in this error message.

Re: [PATCH v15 05/13] ref-filter: implement an `align` atom

[Karthik Nayak]

On Wed, Sep 2, 2015 at 10:58 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> Karthik Nayak <karthik.188@gmail.com> writes:
>>>
>>>> On Wed, Sep 2, 2015 at 8:31 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>>>> Karthik Nayak <karthik.188@gmail.com> writes:
>>>>>
>>>>>>>> +             die(_("format: `end` atom used without a supporting atom"));
>>>>>>>
>>>>>>> Not a show-stopper, but we may need some wordsmithing for "a
>>>>>>> supporting atom" here; an end-user would not know what it is.
>>>>>>
>>>>>> Probably something like "format: `end` atom should only be
>>>>>> used with modifier atoms".
>>>>>
>>>>> Between "supporting" and "modifier" I do not see much difference,
>>>>> though.
>>>>
>>>> I don't see how we could provide a better message, as %(end) atom
>>>> would be common to various atoms eventually.
>>>
>>> I said "not a show-stopper" without giving a suggestion exactly
>>> because I didn't (and I still don't) think either you or I can come
>>> up with a good wording ;-).  That is why the message was Cc'ed to
>>> the list for others to comment.
>>
>> I don't really have a better proposal either. What we really mean is
>> "%(end) requires an atom that requires to be paired with %(end)", but
>> that wouldn't really help. I prefer "supporting" to "modifier":
>> To me, %(color:red) can be called a "modifier" by I wouldn't call %(if)
>> a modifier. "Supporting" is vague, but less misleading to me.
>>
>> Perhaps "corresponding"? (not convinced myself ...)
>
> Yeah, it is like an open and a close parentheses that form a
> matching pair.  "%(end) without a corresponding atom" (implying
> "that opened the environment the %(end) attempts to close")?
>
> We'd need to define what an atom is (or "supporting atom" for that
> matter) and explain how nesting works in the documentation anyway,
> and I'd expect we would gain definitions of a few terms we can use
> in this error message.
>

Then I'll just change it to corresponding for now, and probably go back to it
after the series? Maybe work on some documentation at the end of the series.

-- 
Regards,
Karthik Nayak

Re: [PATCH v15 11/13] tag.c: use 'ref-filter' APIs

[Karthik Nayak]

On Wed, Sep 2, 2015 at 10:13 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>> I actually found the other more readable, as it reads "display the
>> refname aligned on 15 columns, and then a space",
>
> FWIW, I too find the "15-col followed by a space" easier to
> understand.  I was merely being curious when/why Karthik changed
> preference, not objecting to the actual choice.
>

I'm surprised you recall that from memory :D

-- 
Regards,
Karthik Nayak

Re: [PATCH v15 07/13] ref-filter: add support for %(contents:lines=X)

[Karthik Nayak]

On Wed, Sep 2, 2015 at 9:41 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> On Wed, Sep 2, 2015 at 2:37 PM, Matthieu Moy
>> <Matthieu.Moy@grenoble-inp.fr> wrote:
>>> Karthik Nayak <karthik.188@gmail.com> writes:
>>>
>>>> --- a/builtin/tag.c
>>>> +++ b/builtin/tag.c
>>>> @@ -185,6 +185,10 @@ static enum contains_result contains(struct commit *candidate,
>>>>       return contains_test(candidate, want);
>>>>  }
>>>>
>>>> +/*
>>>> + * Currently modified and used in ref-filter as append_lines(), will
>>>> + * eventually be removed as we port tag.c to use ref-filter APIs.
>>>> + */
>>>>  static void show_tag_lines(const struct object_id *oid, int lines)
>>>
>>> I would rather have one "cut and paste" patch followed by a "modify and
>>> use" patch for review.
>>>
>>> As-is, reading the patch doesn't tell me what change you did.
>>>
>>> That said, I did get this information in the interdiff, so I won't
>>> insist on that.
>>
>> Its only borrowed slightly, so I don't really see the need for this.
>> But if you insist, we could do that .
>
> As you prefer.
>
> Perhaps just adapt the comment to say "Currently redundant with
> ref-filter'.c's append_line ...", but that's not important.

"Currently modified and used in ref-filter as append_lines()" seems better
as only a part of this is used.

>
>>>> +static void append_lines(struct strbuf *out, const char *buf, unsigned long size, int lines)
>>>> +{
>>>> +     int i;
>>>> +     const char *sp, *eol;
>>>> +     size_t len;
>>>> +
>>>> +     if ((sp = strstr(buf, "\n\n")) && (sp <= buf + size))
>>>> +             size += 2;
>>>
>>> Why is this "size += 2" needed?
>>>
>>
>> We pass size as "sublen + bodylen - siglen" if there exists a "\n\n"
>> between sublen and bodylen this is not accounted for. hence we
>> add 2 here.
>
> That's too much magic for uncommented code. If this is really needed,
> then thes explanations should go in a comment, and I think this logic
> should be moved out of append_lines (if you read the comment above, the
> function, it is actually lying about what the function does).
>
> I think you can simplify this: you know where the buffer ends (bodypos +
> bodylen) and where it starts (subpos), so you know the size: bodypos +
> bodylen - subpos.
>
> IOW, I think you can apply this:
>
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -645,9 +645,6 @@ static void append_lines(struct strbuf *out, const char *buf, unsigned long size
>         const char *sp, *eol;
>         size_t len;
>
> -       if ((sp = strstr(buf, "\n\n")) && (sp <= buf + size))
> -               size += 2;
> -
>         sp = buf;
>
>         for (i = 0; i < lines && sp < buf + size; i++) {
> @@ -707,7 +704,7 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj
>                         struct strbuf s = STRBUF_INIT;
>                         if (strtoul_ui(valp, 10, &v->u.contents.lines))
>                                 die(_("positive width expected contents:lines=%s"), valp);
> -                       append_lines(&s, subpos, sublen + bodylen - siglen, v->u.contents.lines);
> +                       append_lines(&s, subpos, bodypos + bodylen - subpos, v->u.contents.lines);
>                         v->s = strbuf_detach(&s, NULL);
>                 }
>         }
>
> (half-tested only)

It is important, without it we'll be missing characters at the end.

I'll try what you suggested. Looks good, will test :)

-- 
Regards,
Karthik Nayak

Re: [PATCH v15 07/13] ref-filter: add support for %(contents:lines=X)

[Karthik Nayak]

On Thu, Sep 3, 2015 at 7:04 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>                         struct strbuf s = STRBUF_INIT;
>>                         if (strtoul_ui(valp, 10, &v->u.contents.lines))
>>                                 die(_("positive width expected contents:lines=%s"), valp);
>> -                       append_lines(&s, subpos, sublen + bodylen - siglen, v->u.contents.lines);
>> +                       append_lines(&s, subpos, bodypos + bodylen - subpos, v->u.contents.lines);
>>                         v->s = strbuf_detach(&s, NULL);
>>                 }
>>         }

append_lines(&s, subpos, bodylen + bodypos - subpos - siglen,
v->u.contents.lines);

We need to eliminate the signature if existing also.

-- 
Regards,
Karthik Nayak

Re: [PATCH v15 05/13] ref-filter: implement an `align` atom

[Eric Sunshine]

On Tue, Sep 1, 2015 at 2:26 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> Implement an `align` atom which left-, middle-, or right-aligns the
> content between %(align:..) and %(end).

Spell this either %(align:) or %(align:...) with three dots, not two.
I, personally, think %(align:) is sufficient.

> It is followed by `:<width>,<position>`, where the `<position>` is
> either left, right or middle and `<width>` is the size of the area
> into which the content will be placed. If the content between
> %(align:) and %(end) is more than the width then no alignment is
> performed. e.g. to align a refname atom to the middle with a total
> width of 40 we can do: --format="%(align:middle,40)%(refname)%(end)".
>
> We have an `at_end` function for each element of the stack which is to
> be called when the `end` atom is encountered. Using this we implement
> the aling_handler() for the `align` atom, this aligns the final strbuf
> by calling `strbuf_utf8_align()` from utf8.c.
>
> Ensure that quote formatting is performed on the whole of
> %(align)...%(end) rather than individual atoms inside.  We skip quote

Add colon: %(align:)

> formatting for individual atoms when the current stack element is
> handling an %(align) atom and perform quote formatting at the end when

%(align:)

> we encounter the %(end) atom.
>
> Add documentation and tests for the same.
>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index e49d578..cac3128 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -127,6 +127,15 @@ color::
>         Change output color.  Followed by `:<colorname>`, where names
>         are described in `color.branch.*`.
>
> +align::
> +       Left-, middle-, or right-align the content between %(align:..)

%(align:)

> +       and %(end). Followed by `:<width>,<position>`, where the
> +       `<position>` is either left, right or middle and `<width>` is
> +       the total length of the content with alignment. If the
> +       contents length is more than the width then no alignment is
> +       performed. If used with '--quote' everything in between
> +       %(align:..)  and %(end) is quoted.

%(align:)
Also drop the extra space before "and": s/\s+and/ and/

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

Re: [PATCH v15 07/13] ref-filter: add support for %(contents:lines=X)

[Eric Sunshine]

On Tue, Sep 1, 2015 at 2:26 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> In 'tag.c' we can print N lines from the annotation of the tag using
> the '-n<num>' option. Copy code from 'tag.c' to 'ref-filter' and
> modify it to support appending of N lines from the annotation of tags
> to the given strbuf.
>
> Implement %(contents:lines=X) where X lines of the given object are
> obtained.
>
> Add documentation and test for the same.
>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  struct atom_value {
>         const char *s;
> -       struct align align;
> +       union {
> +               struct align align;
> +               struct contents contents;
> +       } u;
>         void (*handler)(struct atom_value *atomv, struct ref_formatting_state *state);
>         unsigned long ul; /* used for sorting when not FIELD_STR */
>  };
> @@ -226,7 +235,7 @@ static void align_atom_handler(struct atom_value *atomv, struct ref_formatting_s
>         push_stack_element(&state->stack);
>         new = state->stack;
>         new->at_end = align_handler;
> -       new->cb_data = &atomv->align;
> +       new->cb_data = &atomv->u.align;

You could declare atom_value with the union from the start, even if it
has only a single member ('align', at first). Doing so would make this
patch less noisy and wouldn't distract the reader with these seemingly
unrelated changes.

>  }
>
>  static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
> @@ -624,6 +633,33 @@ static void find_subpos(const char *buf, unsigned long sz,
>         *nonsiglen = *sig - buf;
>  }
>
> +/*
> + * If 'lines' is greater than 0, append that many lines from the given
> + * 'buf' of length 'size' to the given strbuf.
> + */
> +static void append_lines(struct strbuf *out, const char *buf, unsigned long size, int lines)
> +{
> +       int i;
> +       const char *sp, *eol;
> +       size_t len;
> +
> +       if ((sp = strstr(buf, "\n\n")) && (sp <= buf + size))
> +               size += 2;

Aside from the +2 which Matthieu already questioned, this code has a
much more serious problem. strstr() assumes that 'buf' is
NUL-terminated, however, the fact that buf's size is also being passed
to the function, implies that it may not be NUL-terminated.
Consequently, strstr() could zip well past the end of 'buf', trying to
consult memory not part of 'buf', which is a violation of the C
standard. Even with the band-aid (sp <= buf + size) which tries to
detect this situation, it's still a violation, and could crash if
strstr() attempts to consult memory which hasn't even been allocated
to the process or is otherwise protected in some fashion.

It's possible that 'buf' may actually always be NUL-terminated, but
this code does not convey that fact. If it is known to be
NUL-terminated, then it is safe to use strstr(), however, that should
be shown more clearly either by revising the code or adding an
appropriate comment.

> +       sp = buf;
> +
> +       for (i = 0; i < lines && sp < buf + size; i++) {
> +               if (i)
> +                       strbuf_addstr(out, "\n    ");
> +               eol = memchr(sp, '\n', size - (sp - buf));
> +               len = eol ? eol - sp : size - (sp - buf);
> +               strbuf_add(out, sp, len);
> +               if (!eol)
> +                       break;
> +               sp = eol + 1;
> +       }
> +}
> @@ -663,6 +701,13 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj
>                         v->s = xmemdupz(sigpos, siglen);
>                 else if (!strcmp(name, "contents"))
>                         v->s = xstrdup(subpos);
> +               else if (skip_prefix(name, "contents:lines=", &valp)) {
> +                       struct strbuf s = STRBUF_INIT;
> +                       if (strtoul_ui(valp, 10, &v->u.contents.lines))
> +                               die(_("positive width expected contents:lines=%s"), valp);

This error message is still too tightly coupled to %(align:), from
which it was copied. "width"?

> +                       append_lines(&s, subpos, sublen + bodylen - siglen, v->u.contents.lines);
> +                       v->s = strbuf_detach(&s, NULL);
> +               }
>         }
>  }

Re: [PATCH v15 07/13] ref-filter: add support for %(contents:lines=X)

[Eric Sunshine]

On Thu, Sep 3, 2015 at 10:39 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Tue, Sep 1, 2015 at 2:26 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> +static void append_lines(struct strbuf *out, const char *buf, unsigned long size, int lines)
>> +{
>> +       if ((sp = strstr(buf, "\n\n")) && (sp <= buf + size))
>> +               size += 2;
>
> Aside from the +2 which Matthieu already questioned, this code has a
> much more serious problem. strstr() assumes that 'buf' is
> NUL-terminated, however, the fact that buf's size is also being passed
> to the function, implies that it may not be NUL-terminated.
> Consequently, strstr() could zip well past the end of 'buf', trying to
> consult memory not part of 'buf', which is a violation of the C
> standard. Even with the band-aid (sp <= buf + size) which tries to
> detect this situation, it's still a violation, and could crash if
> strstr() attempts to consult memory which hasn't even been allocated
> to the process or is otherwise protected in some fashion.
>
> It's possible that 'buf' may actually always be NUL-terminated, but
> this code does not convey that fact. If it is known to be
> NUL-terminated, then it is safe to use strstr(), however, that should
> be shown more clearly either by revising the code or adding an
> appropriate comment.

Also, if 'buf' is indeed unconditionally NUL-terminated, then the (sp
<= buf + size) check is wasted code since the result of strstr() will
always be either NULL or pointing somewhere within the NUL-terminated
string.

Re: [PATCH v15 07/13] ref-filter: add support for %(contents:lines=X)

[Matthieu Moy]

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

> On Thu, Sep 3, 2015 at 7:04 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>>                         struct strbuf s = STRBUF_INIT;
>>>                         if (strtoul_ui(valp, 10, &v->u.contents.lines))
>>>                                 die(_("positive width expected contents:lines=%s"), valp);
>>> -                       append_lines(&s, subpos, sublen + bodylen - siglen, v->u.contents.lines);
>>> +                       append_lines(&s, subpos, bodypos + bodylen - subpos, v->u.contents.lines);
>>>                         v->s = strbuf_detach(&s, NULL);
>>>                 }
>>>         }
>
> append_lines(&s, subpos, bodylen + bodypos - subpos - siglen,
> v->u.contents.lines);
>
> We need to eliminate the signature if existing also.

Indeed. I thought body did not include the signature.

I'd write it as

  bodylen + bodypos - siglen - subpos

or even

  char *contents_end = bodylen + bodypos - siglen;
  ...
  append_lines(&s, subpos, contents_end - subpos, ...);

to make it self-explanatory.

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

Re: [PATCH v15 07/13] ref-filter: add support for %(contents:lines=X)

[Matthieu Moy]

Eric Sunshine <sunshine@sunshineco.com> writes:

>> @@ -624,6 +633,33 @@ static void find_subpos(const char *buf, unsigned long sz,
>>         *nonsiglen = *sig - buf;
>>  }
>>
>> +/*
>> + * If 'lines' is greater than 0, append that many lines from the given
>> + * 'buf' of length 'size' to the given strbuf.
>> + */
>> +static void append_lines(struct strbuf *out, const char *buf, unsigned long size, int lines)
>> +{
>> +       int i;
>> +       const char *sp, *eol;
>> +       size_t len;
>> +
>> +       if ((sp = strstr(buf, "\n\n")) && (sp <= buf + size))
>> +               size += 2;
>
> Aside from the +2 which Matthieu already questioned, this code has a
> much more serious problem. strstr() assumes that 'buf' is
> NUL-terminated, however, the fact that buf's size is also being passed
> to the function, implies that it may not be NUL-terminated.

If Karthik applies my suggestion, then the strstr would go away. I think
the code would be correct even on non-null-terminated strings.

Actually, we're already making the assumption that the buffer for the
whole tag object is null-terminated (and contains no '\0') for
%(contents):

		else if (!strcmp(name, "contents"))
			v->s = xstrdup(subpos);

(But I agree that even if the assumption is correct, it should be made
explicit if it remains a precondition of append_lines).

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

Re: [PATCH v15 07/13] ref-filter: add support for %(contents:lines=X)

[Matthieu Moy]

Eric Sunshine <sunshine@sunshineco.com> writes:

> Also, if 'buf' is indeed unconditionally NUL-terminated, then the (sp
> <= buf + size) check is wasted code since the result of strstr() will
> always be either NULL or pointing somewhere within the NUL-terminated
> string.

The null character is there, but after the signature. buf + size points
before the signature.

Anyway, all this should go away if Karthik applies my suggestion, which
I like even more after this discussion ;-).

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

Re: [PATCH v15 05/13] ref-filter: implement an `align` atom

[Karthik Nayak]

On Thu, Sep 3, 2015 at 7:42 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Tue, Sep 1, 2015 at 2:26 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> Implement an `align` atom which left-, middle-, or right-aligns the
>> content between %(align:..) and %(end).
>
> Spell this either %(align:) or %(align:...) with three dots, not two.
> I, personally, think %(align:) is sufficient.
>

Will change.

>> It is followed by `:<width>,<position>`, where the `<position>` is
>> either left, right or middle and `<width>` is the size of the area
>> into which the content will be placed. If the content between
>> %(align:) and %(end) is more than the width then no alignment is
>> performed. e.g. to align a refname atom to the middle with a total
>> width of 40 we can do: --format="%(align:middle,40)%(refname)%(end)".
>>
>> We have an `at_end` function for each element of the stack which is to
>> be called when the `end` atom is encountered. Using this we implement
>> the aling_handler() for the `align` atom, this aligns the final strbuf
>> by calling `strbuf_utf8_align()` from utf8.c.
>>
>> Ensure that quote formatting is performed on the whole of
>> %(align)...%(end) rather than individual atoms inside.  We skip quote
>
> Add colon: %(align:)
>

Okay.

>> formatting for individual atoms when the current stack element is
>> handling an %(align) atom and perform quote formatting at the end when
>
> %(align:)
>

will do.

>> we encounter the %(end) atom.
>>
>> Add documentation and tests for the same.
>>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
>> index e49d578..cac3128 100644
>> --- a/Documentation/git-for-each-ref.txt
>> +++ b/Documentation/git-for-each-ref.txt
>> @@ -127,6 +127,15 @@ color::
>>         Change output color.  Followed by `:<colorname>`, where names
>>         are described in `color.branch.*`.
>>
>> +align::
>> +       Left-, middle-, or right-align the content between %(align:..)
>
> %(align:)
>

noted.

>> +       and %(end). Followed by `:<width>,<position>`, where the
>> +       `<position>` is either left, right or middle and `<width>` is
>> +       the total length of the content with alignment. If the
>> +       contents length is more than the width then no alignment is
>> +       performed. If used with '--quote' everything in between
>> +       %(align:..)  and %(end) is quoted.
>
> %(align:)
> Also drop the extra space before "and": s/\s+and/ and/
>

Noted, thanks for the review.

-- 
Regards,
Karthik Nayak

Re: [PATCH v15 07/13] ref-filter: add support for %(contents:lines=X)

[Karthik Nayak]

On Thu, Sep 3, 2015 at 8:09 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Tue, Sep 1, 2015 at 2:26 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> In 'tag.c' we can print N lines from the annotation of the tag using
>> the '-n<num>' option. Copy code from 'tag.c' to 'ref-filter' and
>> modify it to support appending of N lines from the annotation of tags
>> to the given strbuf.
>>
>> Implement %(contents:lines=X) where X lines of the given object are
>> obtained.
>>
>> Add documentation and test for the same.
>>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>>  struct atom_value {
>>         const char *s;
>> -       struct align align;
>> +       union {
>> +               struct align align;
>> +               struct contents contents;
>> +       } u;
>>         void (*handler)(struct atom_value *atomv, struct ref_formatting_state *state);
>>         unsigned long ul; /* used for sorting when not FIELD_STR */
>>  };
>> @@ -226,7 +235,7 @@ static void align_atom_handler(struct atom_value *atomv, struct ref_formatting_s
>>         push_stack_element(&state->stack);
>>         new = state->stack;
>>         new->at_end = align_handler;
>> -       new->cb_data = &atomv->align;
>> +       new->cb_data = &atomv->u.align;
>
> You could declare atom_value with the union from the start, even if it
> has only a single member ('align', at first). Doing so would make this
> patch less noisy and wouldn't distract the reader with these seemingly
> unrelated changes.
>

Will do.

>>  }
>>
>>  static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
>> @@ -624,6 +633,33 @@ static void find_subpos(const char *buf, unsigned long sz,
>>         *nonsiglen = *sig - buf;
>>  }
>>
>> +/*
>> + * If 'lines' is greater than 0, append that many lines from the given
>> + * 'buf' of length 'size' to the given strbuf.
>> + */
>> +static void append_lines(struct strbuf *out, const char *buf, unsigned long size, int lines)
>> +{
>> +       int i;
>> +       const char *sp, *eol;
>> +       size_t len;
>> +
>> +       if ((sp = strstr(buf, "\n\n")) && (sp <= buf + size))
>> +               size += 2;
>
> Aside from the +2 which Matthieu already questioned, this code has a
> much more serious problem. strstr() assumes that 'buf' is
> NUL-terminated, however, the fact that buf's size is also being passed
> to the function, implies that it may not be NUL-terminated.
> Consequently, strstr() could zip well past the end of 'buf', trying to
> consult memory not part of 'buf', which is a violation of the C
> standard. Even with the band-aid (sp <= buf + size) which tries to
> detect this situation, it's still a violation, and could crash if
> strstr() attempts to consult memory which hasn't even been allocated
> to the process or is otherwise protected in some fashion.
>
> It's possible that 'buf' may actually always be NUL-terminated, but
> this code does not convey that fact. If it is known to be
> NUL-terminated, then it is safe to use strstr(), however, that should
> be shown more clearly either by revising the code or adding an
> appropriate comment.
>

Although it is NUL terminated, you do have a point, I hope you checked out
Matthieu's suggestion of removing that snippet of code and calculating size
in the function call itself.

>> +       sp = buf;
>> +
>> +       for (i = 0; i < lines && sp < buf + size; i++) {
>> +               if (i)
>> +                       strbuf_addstr(out, "\n    ");
>> +               eol = memchr(sp, '\n', size - (sp - buf));
>> +               len = eol ? eol - sp : size - (sp - buf);
>> +               strbuf_add(out, sp, len);
>> +               if (!eol)
>> +                       break;
>> +               sp = eol + 1;
>> +       }
>> +}
>> @@ -663,6 +701,13 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj
>>                         v->s = xmemdupz(sigpos, siglen);
>>                 else if (!strcmp(name, "contents"))
>>                         v->s = xstrdup(subpos);
>> +               else if (skip_prefix(name, "contents:lines=", &valp)) {
>> +                       struct strbuf s = STRBUF_INIT;
>> +                       if (strtoul_ui(valp, 10, &v->u.contents.lines))
>> +                               die(_("positive width expected contents:lines=%s"), valp);
>
> This error message is still too tightly coupled to %(align:), from
> which it was copied. "width"?
>

Will change it, thanks.

-- 
Regards,
Karthik Nayak

Re: [PATCH v15 07/13] ref-filter: add support for %(contents:lines=X)

[Karthik Nayak]

On Thu, Sep 3, 2015 at 8:35 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> Also, if 'buf' is indeed unconditionally NUL-terminated, then the (sp
>> <= buf + size) check is wasted code since the result of strstr() will
>> always be either NULL or pointing somewhere within the NUL-terminated
>> string.
>
> The null character is there, but after the signature. buf + size points
> before the signature.
>
> Anyway, all this should go away if Karthik applies my suggestion, which
> I like even more after this discussion ;-).

Yes, your suggestion seems like a good way to go.

-- 
Regards,
Karthik Nayak

Re: [PATCH v15 07/13] ref-filter: add support for %(contents:lines=X)

[Karthik Nayak]

On Thu, Sep 3, 2015 at 8:17 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> On Thu, Sep 3, 2015 at 7:04 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>>>                         struct strbuf s = STRBUF_INIT;
>>>>                         if (strtoul_ui(valp, 10, &v->u.contents.lines))
>>>>                                 die(_("positive width expected contents:lines=%s"), valp);
>>>> -                       append_lines(&s, subpos, sublen + bodylen - siglen, v->u.contents.lines);
>>>> +                       append_lines(&s, subpos, bodypos + bodylen - subpos, v->u.contents.lines);
>>>>                         v->s = strbuf_detach(&s, NULL);
>>>>                 }
>>>>         }
>>
>> append_lines(&s, subpos, bodylen + bodypos - subpos - siglen,
>> v->u.contents.lines);
>>
>> We need to eliminate the signature if existing also.
>
> Indeed. I thought body did not include the signature.
>
> I'd write it as
>
>   bodylen + bodypos - siglen - subpos
>
> or even
>
>   char *contents_end = bodylen + bodypos - siglen;
>   ...
>   append_lines(&s, subpos, contents_end - subpos, ...);
>
> to make it self-explanatory.
>

I was thinking of adding a comment but a self explanatory comment
seems like a good idea. Thanks.

-- 
Regards,
Karthik Nayak

Re: [PATCH v15 05/13] ref-filter: implement an `align` atom

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Tue, Sep 1, 2015 at 2:26 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> Implement an `align` atom which left-, middle-, or right-aligns the
>> content between %(align:..) and %(end).
>
> Spell this either %(align:) or %(align:...) with three dots, not two.
> I, personally, think %(align:) is sufficient.

I agree with you that double-dot does not signal "some things are
ellided here" to a usual reader.

I actually think consistent use of %(align:...) is needed, simply
because my knee-jerk reaction to "%(align:)" was "huh?  where does
the need for the trailing colon come from?", not "ah, you try to
imply that there must be something more by having just a colon
there".

Re: [PATCH v15 07/13] ref-filter: add support for %(contents:lines=X)

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> Also, if 'buf' is indeed unconditionally NUL-terminated, then the (sp
> <= buf + size) check is wasted code since the result of strstr() will
> always be either NULL or pointing somewhere within the NUL-terminated
> string.

A caller can give a buf that is NUL terminated but specify that the
only early part of the buffer to be used by giving you a shorter
size, no?  In such a case, strstr() is safe in the sense that it is
guaranteed not to go on forever, but you need to verify the location
of the string it found is within the bounds.

Re: [PATCH v15 07/13] ref-filter: add support for %(contents:lines=X)

[Karthik Nayak]

On Thu, Sep 3, 2015 at 9:57 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> Also, if 'buf' is indeed unconditionally NUL-terminated, then the (sp
>> <= buf + size) check is wasted code since the result of strstr() will
>> always be either NULL or pointing somewhere within the NUL-terminated
>> string.
>
> A caller can give a buf that is NUL terminated but specify that the
> only early part of the buffer to be used by giving you a shorter
> size, no?  In such a case, strstr() is safe in the sense that it is
> guaranteed not to go on forever, but you need to verify the location
> of the string it found is within the bounds.

That was the idea behind this, but will stick to Matthieu's suggestion.

-- 
Regards,
Karthik Nayak

Re: [PATCH v15 05/13] ref-filter: implement an `align` atom

[Karthik Nayak]

On Thu, Sep 3, 2015 at 9:53 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> On Tue, Sep 1, 2015 at 2:26 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>> Implement an `align` atom which left-, middle-, or right-aligns the
>>> content between %(align:..) and %(end).
>>
>> Spell this either %(align:) or %(align:...) with three dots, not two.
>> I, personally, think %(align:) is sufficient.
>
> I agree with you that double-dot does not signal "some things are
> ellided here" to a usual reader.
>
> I actually think consistent use of %(align:...) is needed, simply
> because my knee-jerk reaction to "%(align:)" was "huh?  where does
> the need for the trailing colon come from?", not "ah, you try to
> imply that there must be something more by having just a colon
> there".
>

Yeah, it makes more sense to keep the triple dot.

-- 
Regards,
Karthik Nayak