[WIP] [PATCH 0/4] Unifying git branch -l, git tag -l, and git for-each-ref

[WIP] [PATCH 0/4] Unifying git branch -l, git tag -l, and git for-each-ref

[karthik nayak]

Hello All,

Just updating on my progress for my GSoC project on "Unifying git 
branch- l, git tag -l and git for-each-ref."

I have been going through the code for the above commands, I started off 
first with 'for-each-ref' because it seemed to be the most basic command 
of the three, I have been building the common library
'ref-filter' based on the requirements for 'for-each-ref', eventually
I plan to merge 'tag -l' and 'branch -l' and extend its functionality.

The '--format' and '--sort' option provided by for-each-ref seemed to be 
the most fundamental and I plan on using this implementation and 
extending the option to 'tag -l' and 'branch -l'. So this left me with 
moving most of the code from 'for-each-ref' to 'ref-filter' and leaving 
'for-each-ref' with nothing but the command call from the main git command.

This is still a WIP and hope to get some suggestions/feedback on my 
progress.

[PATCH 1/4] for-each-ref: rename refinfo members to match similar structures

[Karthik Nayak]

From: Jeff King <peff@peff.net>

Written-by: Jeff King <peff@peff.net>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 builtin/for-each-ref.c | 40 ++++++++++++++++++++--------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 83f9cf9..2721228 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -32,9 +32,9 @@ struct ref_sort {
 };
 
 struct refinfo {
-	char *refname;
-	unsigned char objectname[20];
-	int flag;
+	char *name;
+	unsigned char sha1[20];
+	int flags;
 	const char *symref;
 	struct atom_value *value;
 };
@@ -632,9 +632,9 @@ static void populate_value(struct refinfo *ref)
 
 	ref->value = xcalloc(used_atom_cnt, sizeof(struct atom_value));
 
-	if (need_symref && (ref->flag & REF_ISSYMREF) && !ref->symref) {
+	if (need_symref && (ref->flags & REF_ISSYMREF) && !ref->symref) {
 		unsigned char unused1[20];
-		ref->symref = resolve_refdup(ref->refname, RESOLVE_REF_READING,
+		ref->symref = resolve_refdup(ref->name, RESOLVE_REF_READING,
 					     unused1, NULL);
 		if (!ref->symref)
 			ref->symref = "";
@@ -655,14 +655,14 @@ static void populate_value(struct refinfo *ref)
 		}
 
 		if (starts_with(name, "refname"))
-			refname = ref->refname;
+			refname = ref->name;
 		else if (starts_with(name, "symref"))
 			refname = ref->symref ? ref->symref : "";
 		else if (starts_with(name, "upstream")) {
 			/* only local branches may have an upstream */
-			if (!starts_with(ref->refname, "refs/heads/"))
+			if (!starts_with(ref->name, "refs/heads/"))
 				continue;
-			branch = branch_get(ref->refname + 11);
+			branch = branch_get(ref->name + 11);
 
 			if (!branch || !branch->merge || !branch->merge[0] ||
 			    !branch->merge[0]->dst)
@@ -677,9 +677,9 @@ static void populate_value(struct refinfo *ref)
 			continue;
 		} else if (!strcmp(name, "flag")) {
 			char buf[256], *cp = buf;
-			if (ref->flag & REF_ISSYMREF)
+			if (ref->flags & REF_ISSYMREF)
 				cp = copy_advance(cp, ",symref");
-			if (ref->flag & REF_ISPACKED)
+			if (ref->flags & REF_ISPACKED)
 				cp = copy_advance(cp, ",packed");
 			if (cp == buf)
 				v->s = "";
@@ -688,7 +688,7 @@ static void populate_value(struct refinfo *ref)
 				v->s = xstrdup(buf + 1);
 			}
 			continue;
-		} else if (!deref && grab_objectname(name, ref->objectname, v)) {
+		} else if (!deref && grab_objectname(name, ref->sha1, v)) {
 			continue;
 		} else if (!strcmp(name, "HEAD")) {
 			const char *head;
@@ -696,7 +696,7 @@ static void populate_value(struct refinfo *ref)
 
 			head = resolve_ref_unsafe("HEAD", RESOLVE_REF_READING,
 						  sha1, NULL);
-			if (!strcmp(ref->refname, head))
+			if (!strcmp(ref->name, head))
 				v->s = "*";
 			else
 				v->s = " ";
@@ -774,13 +774,13 @@ static void populate_value(struct refinfo *ref)
 	return;
 
  need_obj:
-	buf = get_obj(ref->objectname, &obj, &size, &eaten);
+	buf = get_obj(ref->sha1, &obj, &size, &eaten);
 	if (!buf)
 		die("missing object %s for %s",
-		    sha1_to_hex(ref->objectname), ref->refname);
+		    sha1_to_hex(ref->sha1), ref->name);
 	if (!obj)
 		die("parse_object_buffer failed on %s for %s",
-		    sha1_to_hex(ref->objectname), ref->refname);
+		    sha1_to_hex(ref->sha1), ref->name);
 
 	grab_values(ref->value, 0, obj, buf, size);
 	if (!eaten)
@@ -808,10 +808,10 @@ static void populate_value(struct refinfo *ref)
 	buf = get_obj(tagged, &obj, &size, &eaten);
 	if (!buf)
 		die("missing object %s for %s",
-		    sha1_to_hex(tagged), ref->refname);
+		    sha1_to_hex(tagged), ref->name);
 	if (!obj)
 		die("parse_object_buffer failed on %s for %s",
-		    sha1_to_hex(tagged), ref->refname);
+		    sha1_to_hex(tagged), ref->name);
 	grab_values(ref->value, 1, obj, buf, size);
 	if (!eaten)
 		free(buf);
@@ -877,9 +877,9 @@ static int grab_single_ref(const char *refname, const unsigned char *sha1, int f
 	 * by maxcount logic.
 	 */
 	ref = xcalloc(1, sizeof(*ref));
-	ref->refname = xstrdup(refname);
-	hashcpy(ref->objectname, sha1);
-	ref->flag = flag;
+	ref->name = xstrdup(refname);
+	hashcpy(ref->sha1, sha1);
+	ref->flags = flag;
 
 	cnt = cb->grab_cnt;
 	REALLOC_ARRAY(cb->grab_array, cnt + 1);
-- 
2.4.1

Re: [PATCH 1/4] for-each-ref: rename refinfo members to match similar structures

[Matthieu Moy]

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

> From: Jeff King <peff@peff.net>

This means that "git am" will consider Peff as the author ...

> Written-by: Jeff King <peff@peff.net>

... hence this is not needed: in the final history, it will appear as if
Peff wrote this Written-by: himself, which would be weird.

If it is the case, you should add in the commit message that there's no
actual changs, and perhaps which renames were done. This makes the
review straightforward.

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

Re: [PATCH 1/4] for-each-ref: rename refinfo members to match similar structures

[karthik nayak]

On 05/20/2015 10:27 PM, Matthieu Moy wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> From: Jeff King <peff@peff.net>
>
> This means that "git am" will consider Peff as the author ...
>
>> Written-by: Jeff King <peff@peff.net>
>
> ... hence this is not needed: in the final history, it will appear as if
> Peff wrote this Written-by: himself, which would be weird.
>
> If it is the case, you should add in the commit message that there's no
> actual changs, and perhaps which renames were done. This makes the
> review straightforward.
>

Noted, will change it.

[PATCH 2/4] ref-filter: add ref-filter API

[Karthik Nayak]

add a ref-filter API to provide functions to filter refs for listing.
This will act as a common library for commands like 'tag -l',
'branch -l' and 'for-each-ref'. ref-filter will enable each of these
commands to benefit from the features of the others.

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>
---
 Makefile     |  1 +
 ref-filter.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 ref-filter.h | 47 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 121 insertions(+)
 create mode 100644 ref-filter.c
 create mode 100644 ref-filter.h

diff --git a/Makefile b/Makefile
index 5f3987f..1e03bda 100644
--- a/Makefile
+++ b/Makefile
@@ -759,6 +759,7 @@ LIB_OBJS += reachable.o
 LIB_OBJS += read-cache.o
 LIB_OBJS += reflog-walk.o
 LIB_OBJS += refs.o
+LIB_OBJS += ref-filter.o
 LIB_OBJS += remote.o
 LIB_OBJS += replace_object.o
 LIB_OBJS += rerere.o
diff --git a/ref-filter.c b/ref-filter.c
new file mode 100644
index 0000000..df571a6
--- /dev/null
+++ b/ref-filter.c
@@ -0,0 +1,73 @@
+#include "builtin.h"
+#include "cache.h"
+#include "refs.h"
+#include "ref-filter.h"
+#include "wildmatch.h"
+#include "commit.h"
+
+static int match_name_as_path(const char **pattern, const char *refname)
+{
+	int namelen = strlen(refname);
+	for (; *pattern; pattern++) {
+		const char *p = *pattern;
+		int plen = strlen(p);
+
+		if ((plen <= namelen) &&
+		    !strncmp(refname, p, plen) &&
+		    (refname[plen] == '\0' ||
+		     refname[plen] == '/' ||
+		     p[plen-1] == '/'))
+			return 1;
+		if (!wildmatch(p, refname, WM_PATHNAME, NULL))
+			return 1;
+	}
+	return 0;
+}
+
+static struct ref_filter_item *new_ref_filter_item(const char *refname,
+						   const unsigned char *sha1,
+						   int flag)
+{
+	struct ref_filter_item *ref =  xcalloc(1, sizeof(struct ref_filter_item));
+	ref->name = xstrdup(refname);
+	hashcpy(ref->sha1, sha1);
+	ref->flags = flag;
+
+	return ref;
+}
+
+/*
+ * A call-back given to for_each_ref().  Filter refs and keep them for
+ * later object processing.
+ */
+int ref_filter_add(const char *refname, const unsigned char *sha1, int flag, void *data)
+{
+	struct ref_filter *ref_list = data;
+	struct ref_filter_item *ref;
+
+	if (flag & REF_BAD_NAME) {
+		warning("ignoring ref with broken name %s", refname);
+		return 0;
+	}
+
+	if (*ref_list->name_patterns && !match_name_as_path(ref_list->name_patterns, refname))
+		return 0;
+
+	ref = new_ref_filter_item(refname, sha1, flag);
+
+	REALLOC_ARRAY(ref_list->items, ref_list->count + 1);
+	ref_list->items[ref_list->count++] = ref;
+
+	return 0;
+}
+
+void ref_filter_clear(struct ref_filter *refs)
+{
+	int i;
+
+	for (i = 0; i < refs->count; i++)
+		free(refs->items[i]);
+	free(refs->items);
+	refs->items = NULL;
+	refs->count = refs->alloc = 0;
+}
diff --git a/ref-filter.h b/ref-filter.h
new file mode 100644
index 0000000..3010d13
--- /dev/null
+++ b/ref-filter.h
@@ -0,0 +1,47 @@
+#ifndef REF_FILTER_H
+#define REF_FILTER_H
+
+#include "sha1-array.h"
+#include "refs.h"
+#include "commit.h"
+
+/*
+ * ref-filter is meant to act as a common provider of API's for
+ * 'tag -l', 'branch -l' and 'for-each-ref'. ref-filter is the attempt
+ * at unification of these three commands so that they ay benefit from
+ * the functionality of each other.
+ */
+
+/* An atom is a valid field atom used for sorting and formatting of refs.*/
+struct atom_value {
+	const char *s;
+	unsigned long ul; /* used for sorting when not FIELD_STR */
+};
+
+struct ref_sort {
+	struct ref_sort *next;
+	int atom; /* index into used_atom array */
+	unsigned reverse : 1;
+};
+
+/* ref_filter_item will hold the data pertaining to a particular ref. */
+struct ref_filter_item {
+	unsigned char sha1[20];
+	int flags;
+	const char *symref;
+	struct atom_value *value;
+	char *name;
+};
+
+/*  ref_filter will hold data pertaining to a list of refs. */
+struct ref_filter {
+	int count, alloc;
+	struct ref_filter_item **items;
+	const char **name_patterns;
+};
+
+/*  ref_filter_add is used to add refs to the ref_filter list. */
+int ref_filter_add(const char *refname, const unsigned char *sha1, int flags, void *data);
+void ref_filter_clear(struct ref_filter *refs);
+
+#endif /*  REF_FILTER_H  */
-- 
2.4.1

Re: [PATCH 2/4] ref-filter: add ref-filter API

[Eric Sunshine]

On Wed, May 20, 2015 at 9:18 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> add a ref-filter API to provide functions to filter refs for listing.
> This will act as a common library for commands like 'tag -l',
> 'branch -l' and 'for-each-ref'. ref-filter will enable each of these
> commands to benefit from the features of the others.
>
> 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>
> ---
>  Makefile     |  1 +
>  ref-filter.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  ref-filter.h | 47 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 121 insertions(+)
>  create mode 100644 ref-filter.c
>  create mode 100644 ref-filter.h

A shortcoming of this approach is that it's not blame-friendly.
Although those of us following this patch series know that much of the
code in this patch was copied from for-each-ref.c, git-blame will not
recognize this unless invoked in the very expensive "git blame -C -C
-C" fashion (if I understand correctly). The most blame-friendly way
to perform this re-organization is to have the code relocation (line
removals and line additions) occur in one patch.

There are multiple ways you could arrange to do so. One would be to
first have a patch which introduces just a skeleton of the intended
API, with do-nothing function implementations. A subsequent patch
would then relocate the code from for-each-ref.c to ref-filter.c, and
update for-each-ref.c to call into the new (now fleshed-out) API.

Re: [PATCH 2/4] ref-filter: add ref-filter API

[karthik nayak]

On 05/21/2015 12:37 AM, Eric Sunshine wrote:
> On Wed, May 20, 2015 at 9:18 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> add a ref-filter API to provide functions to filter refs for listing.
>> This will act as a common library for commands like 'tag -l',
>> 'branch -l' and 'for-each-ref'. ref-filter will enable each of these
>> commands to benefit from the features of the others.
>>
>> 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>
>> ---
>>   Makefile     |  1 +
>>   ref-filter.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   ref-filter.h | 47 ++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 121 insertions(+)
>>   create mode 100644 ref-filter.c
>>   create mode 100644 ref-filter.h
>
> A shortcoming of this approach is that it's not blame-friendly.
> Although those of us following this patch series know that much of the
> code in this patch was copied from for-each-ref.c, git-blame will not
> recognize this unless invoked in the very expensive "git blame -C -C
> -C" fashion (if I understand correctly). The most blame-friendly way
> to perform this re-organization is to have the code relocation (line
> removals and line additions) occur in one patch.
>
> There are multiple ways you could arrange to do so. One would be to
> first have a patch which introduces just a skeleton of the intended
> API, with do-nothing function implementations. A subsequent patch
> would then relocate the code from for-each-ref.c to ref-filter.c, and
> update for-each-ref.c to call into the new (now fleshed-out) API.
>

Did you read Junio's suggestion on how I should re-order this WIP patch 
series ?
That's somewhat on the lines of what you're suggesting. I'll probably be 
going ahead with that, not really sure about how blame works entirely so 
what do you think about that?

Re: [PATCH 2/4] ref-filter: add ref-filter API

[Eric Sunshine]

On Thu, May 21, 2015 at 1:30 PM, karthik nayak <karthik.188@gmail.com> wrote:
> On 05/21/2015 12:37 AM, Eric Sunshine wrote:
>> On Wed, May 20, 2015 at 9:18 AM, Karthik Nayak <karthik.188@gmail.com>
>> wrote:
>>>   Makefile     |  1 +
>>>   ref-filter.c | 73
>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>   ref-filter.h | 47 ++++++++++++++++++++++++++++++++++++++
>>>   3 files changed, 121 insertions(+)
>>>   create mode 100644 ref-filter.c
>>>   create mode 100644 ref-filter.h
>>
>> A shortcoming of this approach is that it's not blame-friendly.
>> Although those of us following this patch series know that much of the
>> code in this patch was copied from for-each-ref.c, git-blame will not
>> recognize this unless invoked in the very expensive "git blame -C -C
>> -C" fashion (if I understand correctly). The most blame-friendly way
>> to perform this re-organization is to have the code relocation (line
>> removals and line additions) occur in one patch.
>>
>> There are multiple ways you could arrange to do so. One would be to
>> first have a patch which introduces just a skeleton of the intended
>> API, with do-nothing function implementations. A subsequent patch
>> would then relocate the code from for-each-ref.c to ref-filter.c, and
>> update for-each-ref.c to call into the new (now fleshed-out) API.
>
> Did you read Junio's suggestion on how I should re-order this WIP patch
> series ?
> That's somewhat on the lines of what you're suggesting. I'll probably be
> going ahead with that, not really sure about how blame works entirely so
> what do you think about that?

Yes, Junio's response did a much better job of saying what I intended.
Also, his response said something I meant to mention but forgot:
namely that, to ease the review task, code movement should be pure
movement, and not involve other changes.

Anyhow, follow Junio's advice. He knows what he's talking about. ;-)

Re: [PATCH 2/4] ref-filter: add ref-filter API

[karthik nayak]

On 05/22/2015 12:10 AM, Eric Sunshine wrote:
> On Thu, May 21, 2015 at 1:30 PM, karthik nayak <karthik.188@gmail.com> wrote:
>> On 05/21/2015 12:37 AM, Eric Sunshine wrote:
>>> On Wed, May 20, 2015 at 9:18 AM, Karthik Nayak <karthik.188@gmail.com>
>>> wrote:
>>>>    Makefile     |  1 +
>>>>    ref-filter.c | 73
>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>    ref-filter.h | 47 ++++++++++++++++++++++++++++++++++++++
>>>>    3 files changed, 121 insertions(+)
>>>>    create mode 100644 ref-filter.c
>>>>    create mode 100644 ref-filter.h
>>>
>>> A shortcoming of this approach is that it's not blame-friendly.
>>> Although those of us following this patch series know that much of the
>>> code in this patch was copied from for-each-ref.c, git-blame will not
>>> recognize this unless invoked in the very expensive "git blame -C -C
>>> -C" fashion (if I understand correctly). The most blame-friendly way
>>> to perform this re-organization is to have the code relocation (line
>>> removals and line additions) occur in one patch.
>>>
>>> There are multiple ways you could arrange to do so. One would be to
>>> first have a patch which introduces just a skeleton of the intended
>>> API, with do-nothing function implementations. A subsequent patch
>>> would then relocate the code from for-each-ref.c to ref-filter.c, and
>>> update for-each-ref.c to call into the new (now fleshed-out) API.
>>
>> Did you read Junio's suggestion on how I should re-order this WIP patch
>> series ?
>> That's somewhat on the lines of what you're suggesting. I'll probably be
>> going ahead with that, not really sure about how blame works entirely so
>> what do you think about that?
>
> Yes, Junio's response did a much better job of saying what I intended.
> Also, his response said something I meant to mention but forgot:
> namely that, to ease the review task, code movement should be pure
> movement, and not involve other changes.
>
> Anyhow, follow Junio's advice. He knows what he's talking about. ;-)
>

Alright, Thanks for clearing that out.

Regards,
Karthik

Re: [PATCH 2/4] ref-filter: add ref-filter API

[Matthieu Moy]

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

> +static int match_name_as_path(const char **pattern, const char *refname)

I would have appreciated a short docstring. The full doc would probably
be as long as the code, but a few examples of what matches and what
doesn't can help the reader.

> +static struct ref_filter_item *new_ref_filter_item(const char *refname,
> +						   const unsigned char *sha1,
> +						   int flag)
> +{
> +	struct ref_filter_item *ref =  xcalloc(1, sizeof(struct ref_filter_item));

double-space after =.

> +++ b/ref-filter.h
> @@ -0,0 +1,47 @@
> +#ifndef REF_FILTER_H
> +#define REF_FILTER_H
> +
> +#include "sha1-array.h"
> +#include "refs.h"
> +#include "commit.h"
> +
> +/*
> + * ref-filter is meant to act as a common provider of API's for
> + * 'tag -l', 'branch -l' and 'for-each-ref'. ref-filter is the attempt

Don't be shy: attempt at unification -> unification. This message may be
an attempt, but we'll polish it until it is more than that.

> + * at unification of these three commands so that they ay benefit from

they *may*?

> + * the functionality of each other.
> + */

I miss a high-level description of what the code is doing. Essentially,
there's the complete repository list of refs, and you want to filter
only some of them, right?

>From the name, I would guess that ref_filter is the structure describing
how you are filtering, but from the code it seems to be the list you're
filtering, not the filter.

> +/* An atom is a valid field atom used for sorting and formatting of refs.*/

"used for" is very vague. Be more precise, say how it will be involved
in sorting & formatting.

> +/*  ref_filter will hold data pertaining to a list of refs. */

This is the answer to the "what?" question, which is not very hard to
infer from the code. That's not anwsering "what for?" or "why?", which
are much harder to infer for the reader.

(plus you have a double-space after /*)

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

Re: [PATCH 2/4] ref-filter: add ref-filter API

[karthik nayak]

On 05/21/2015 02:17 PM, Matthieu Moy wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> +static int match_name_as_path(const char **pattern, const char *refname)
>
> I would have appreciated a short docstring. The full doc would probably
> be as long as the code, but a few examples of what matches and what
> doesn't can help the reader.
>

Will patch with an explanation and some examples.

>> +static struct ref_filter_item *new_ref_filter_item(const char *refname,
>> +						   const unsigned char *sha1,
>> +						   int flag)
>> +{
>> +	struct ref_filter_item *ref =  xcalloc(1, sizeof(struct ref_filter_item));
>
> double-space after =.

Noted.

>
>> +++ b/ref-filter.h
>> @@ -0,0 +1,47 @@
>> +#ifndef REF_FILTER_H
>> +#define REF_FILTER_H
>> +
>> +#include "sha1-array.h"
>> +#include "refs.h"
>> +#include "commit.h"
>> +
>> +/*
>> + * ref-filter is meant to act as a common provider of API's for
>> + * 'tag -l', 'branch -l' and 'for-each-ref'. ref-filter is the attempt
>
> Don't be shy: attempt at unification -> unification. This message may be
> an attempt, but we'll polish it until it is more than that.

Haha, OK, will change that.

>
>> + * at unification of these three commands so that they ay benefit from
>
> they *may*?

Yes. Will change

>
>> + * the functionality of each other.
>> + */
>
> I miss a high-level description of what the code is doing. Essentially,
> there's the complete repository list of refs, and you want to filter
> only some of them, right?
>
>  From the name, I would guess that ref_filter is the structure describing
> how you are filtering, but from the code it seems to be the list you're
> filtering, not the filter.

Will write a better explanation and description.
>
>> +/* An atom is a valid field atom used for sorting and formatting of refs.*/
>
> "used for" is very vague. Be more precise, say how it will be involved
> in sorting & formatting.

Noted.

>
>> +/*  ref_filter will hold data pertaining to a list of refs. */
>
> This is the answer to the "what?" question, which is not very hard to
> infer from the code. That's not anwsering "what for?" or "why?", which
> are much harder to infer for the reader.
>
> (plus you have a double-space after /*)
>

Noted! Thanks for the suggestions :)

Re: [PATCH 2/4] ref-filter: add ref-filter API

[karthik nayak]

> I miss a high-level description of what the code is doing. Essentially,
> there's the complete repository list of refs, and you want to filter
> only some of them, right?
>
>  From the name, I would guess that ref_filter is the structure describing
> how you are filtering, but from the code it seems to be the list you're
> filtering, not the filter.

Reading this again, A bit confused by what you're trying to imply. Could 
you rephrase please?

Re: [PATCH 2/4] ref-filter: add ref-filter API

[Matthieu Moy]

karthik nayak <karthik.188@gmail.com> writes:

>> I miss a high-level description of what the code is doing. Essentially,
>> there's the complete repository list of refs, and you want to filter
>> only some of them, right?
>>
>>  From the name, I would guess that ref_filter is the structure describing
>> how you are filtering, but from the code it seems to be the list you're
>> filtering, not the filter.
>
> Reading this again, A bit confused by what you're trying to imply.
> Could you rephrase please?

At some point, I'd expect something like

  filtered_list_of_refs = filer(full_list_of_refs, description_of_filter);

That would remove some refs from full_list_of_refs according to
description_of_filter.

(totally invented code, only to show the idea)

If there's a piece of code looking like this, then you need a data
structure to store list of refs (full_list_of_refs and
filtered_list_of_refs) and another to describe what you're doing with it
(description_of_filter).

The name ref_filter implies to me that it contains the description of
the filter, but looking at the code it doesn't seem to be the case.

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

Re: [PATCH 2/4] ref-filter: add ref-filter API

[karthik nayak]

On 05/22/2015 12:14 PM, Matthieu Moy wrote:
> karthik nayak <karthik.188@gmail.com> writes:
>
>>> I miss a high-level description of what the code is doing. Essentially,
>>> there's the complete repository list of refs, and you want to filter
>>> only some of them, right?
>>>
>>>   From the name, I would guess that ref_filter is the structure describing
>>> how you are filtering, but from the code it seems to be the list you're
>>> filtering, not the filter.
>>
>> Reading this again, A bit confused by what you're trying to imply.
>> Could you rephrase please?
>
> At some point, I'd expect something like
>
>    filtered_list_of_refs = filer(full_list_of_refs, description_of_filter);
>
> That would remove some refs from full_list_of_refs according to
> description_of_filter.
>
> (totally invented code, only to show the idea)
>
> If there's a piece of code looking like this, then you need a data
> structure to store list of refs (full_list_of_refs and
> filtered_list_of_refs) and another to describe what you're doing with it
> (description_of_filter).
>
> The name ref_filter implies to me that it contains the description of
> the filter, but looking at the code it doesn't seem to be the case.
>

But it does just that, doesn't it?

strict ref_filter {
	int count, alloc;
	struct ref_filter_item **items;
	const char **name_patterns;
};

If you see it does contain 'name_patterns' according to which it will 
filter the given refs, but thats just the start, as 'for-each-ref' only 
supports filtering based on the given pattern, eventually as I merge the 
functionality of 'git tag -l' and 'git branch -l' it will contain more 
filters like, 'contains_commit', 'merged' and so on. Eventually becoming 
more of a filter description as you put it. I hope that clears out things :)

Regards,
Karthik

Re: [PATCH 2/4] ref-filter: add ref-filter API

[Matthieu Moy]

karthik nayak <karthik.188@gmail.com> writes:

>> At some point, I'd expect something like
>>
>>    filtered_list_of_refs = filer(full_list_of_refs, description_of_filter);
>>
>> That would remove some refs from full_list_of_refs according to
>> description_of_filter.
>>
>> (totally invented code, only to show the idea)
>>
>> If there's a piece of code looking like this, then you need a data
>> structure to store list of refs (full_list_of_refs and
>> filtered_list_of_refs) and another to describe what you're doing with it
>> (description_of_filter).
>>
>> The name ref_filter implies to me that it contains the description of
>> the filter, but looking at the code it doesn't seem to be the case.
>>
>
> But it does just that, doesn't it?
>
> struct ref_filter {
> 	int count, alloc;
> 	struct ref_filter_item **items;
> 	const char **name_patterns;
> };
>
> If you see it does contain 'name_patterns' according to which it will
> filter the given refs,

But it also contains struct ref_filter_item **items, which as I
understand it contains a list of refs (with name, sha1 & such).

That's the part I do not find natural: the same structure contains both
the list of refs and the way it should be filtered.

Re-reading the patch, I seem to understand that you're putting both on
the same struct because of the API of for_each_ref() which takes one
'data' pointer to be casted, so you want both the input (filter
description) and the output (list of refs after filtering) to be
contained in the same struct.

But I think this could be clearer in the code (and/or comment + commit
message). Perhaps stg like:

struct ref_filter_data /* Probably not the best name */ {
        struct ref_list list;
        struct ref_filter filter;
};

struct ref_list {
 	int count, alloc;
 	struct ref_filter_item **items;
 	const char **name_patterns;
};

struct ref_filter {
	const char **name_patterns;
	/* There will be more here later */
};

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

Re: [PATCH 2/4] ref-filter: add ref-filter API

[Christian Couder]

On Sat, May 23, 2015 at 4:42 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> karthik nayak <karthik.188@gmail.com> writes:
>
>>> At some point, I'd expect something like
>>>
>>>    filtered_list_of_refs = filer(full_list_of_refs, description_of_filter);
>>>
>>> That would remove some refs from full_list_of_refs according to
>>> description_of_filter.
>>>
>>> (totally invented code, only to show the idea)
>>>
>>> If there's a piece of code looking like this, then you need a data
>>> structure to store list of refs (full_list_of_refs and
>>> filtered_list_of_refs) and another to describe what you're doing with it
>>> (description_of_filter).
>>>
>>> The name ref_filter implies to me that it contains the description of
>>> the filter, but looking at the code it doesn't seem to be the case.
>>>
>>
>> But it does just that, doesn't it?
>>
>> struct ref_filter {
>>       int count, alloc;
>>       struct ref_filter_item **items;
>>       const char **name_patterns;
>> };
>>
>> If you see it does contain 'name_patterns' according to which it will
>> filter the given refs,
>
> But it also contains struct ref_filter_item **items, which as I
> understand it contains a list of refs (with name, sha1 & such).
>
> That's the part I do not find natural: the same structure contains both
> the list of refs and the way it should be filtered.
>
> Re-reading the patch, I seem to understand that you're putting both on
> the same struct because of the API of for_each_ref() which takes one
> 'data' pointer to be casted, so you want both the input (filter
> description) and the output (list of refs after filtering) to be
> contained in the same struct.
>
> But I think this could be clearer in the code (and/or comment + commit
> message). Perhaps stg like:
>
> struct ref_filter_data /* Probably not the best name */ {
>         struct ref_list list;
>         struct ref_filter filter;
> };
>
> struct ref_list {
>         int count, alloc;
>         struct ref_filter_item **items;
>         const char **name_patterns;
> };

Matthieu, I think you forgot to remove "const char **name_patterns;"
in the above struct, as you put it in the "ref_filter" struct below:

> struct ref_filter {
>         const char **name_patterns;
>         /* There will be more here later */
> };

I agree that it might be clearer to separate both. In this case
instead of "ref_list" the struct might be called "ref_filter_array" as
we already have "argv_array" in argv-array.h and "sha1_array" in
"sha1-array.h".

Re: [PATCH 2/4] ref-filter: add ref-filter API

[Matthieu Moy]

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

>> struct ref_list {
>>         int count, alloc;
>>         struct ref_filter_item **items;
>>         const char **name_patterns;
>> };
>
> Matthieu, I think you forgot to remove "const char **name_patterns;"
> in the above struct, as you put it in the "ref_filter" struct below:

Yes, indeed. Too quick cut-and-paste.

> I agree that it might be clearer to separate both. In this case
> instead of "ref_list" the struct might be called "ref_filter_array" as
> we already have "argv_array" in argv-array.h and "sha1_array" in
> "sha1-array.h".

I'd drop the "filter" part and make it ref_array then. There's no reason
we could not use it it places other than filter.

But we also have string_list which is an array underneath, so I think
both names (_array and _list) are fine.

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

Re: [PATCH 2/4] ref-filter: add ref-filter API

[Junio C Hamano]

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

> But I think this could be clearer in the code (and/or comment + commit
> message). Perhaps stg like:
>
> struct ref_filter_data /* Probably not the best name */ {
>         struct ref_list list;
>         struct ref_filter filter;
> };
>
> struct ref_list {
>  	int count, alloc;
>  	struct ref_filter_item **items;
> };

If you plan to use ALLOC_GROW() API, name the bookkeeping variables
"alloc" & "nr", unless you have a compelling reason to deviate from
the prevailing practice.

> struct ref_filter {
> 	const char **name_patterns;
> 	/* There will be more here later */
> };

Very good suggestion.

Whatever the final name would be, it is a good idea to separate the
"list of things that are operated on" and the "set of operations to
be applied".  That makes things conceptually cleaner; you can have
multiple of the former operated on with a singleton of the latter
and then their results merged, etc. etc.

And I do not think an array of things that are operated on should
not be named "ref_filter_item".

Surely, the latter "set of operations to be applied" may currently
be only filtering, but who says it has to stay that way?  "I have a
set of refs that represent my local branches I am interested
in. Please map them to their corresponding @{upstream}" is a
reasonable request once you have an infrastructure to represent "set
of refs to be worked on" and "set of operations to apply", and at
that point, the items are no longer filter-items (map-items?).

Re: [PATCH 2/4] ref-filter: add ref-filter API

[Matthieu Moy]

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

> And I do not think an array of things that are operated on should
> not be named "ref_filter_item".

Is the double-negation intended? It seems contradictory with:

> Surely, the latter "set of operations to be applied" may currently
> be only filtering, but who says it has to stay that way?

(With which I do agree)

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

Re: [PATCH 2/4] ref-filter: add ref-filter API

[Karthik Nayak]

>
> But it also contains struct ref_filter_item **items, which as I
> understand it contains a list of refs (with name, sha1 & such).
>
> That's the part I do not find natural: the same structure contains both
> the list of refs and the way it should be filtered.
>
> Re-reading the patch, I seem to understand that you're putting both on
> the same struct because of the API of for_each_ref() which takes one
> 'data' pointer to be casted, so you want both the input (filter
> description) and the output (list of refs after filtering) to be
> contained in the same struct.

Was kinda confused, This clears out things, Thanks.

>
> But I think this could be clearer in the code (and/or comment + commit
> message). Perhaps stg like:
>
> struct ref_filter_data /* Probably not the best name */ {
>          struct ref_list list;
>          struct ref_filter filter;
> };
>
> struct ref_list {
>   	int count, alloc;
>   	struct ref_filter_item **items;
>   	const char **name_patterns;
> };
>
> struct ref_filter {
> 	const char **name_patterns;
> 	/* There will be more here later */
> };
>

This seems cleaner, agreed.

 >
 > I agree that it might be clearer to separate both. In this case
 > instead of "ref_list" the struct might be called "ref_filter_array" as
 > we already have "argv_array" in argv-array.h and "sha1_array" in
 > "sha1-array.h".
 >

Somehow ref_list seems more real to me, list of refs.

 >
 > And I do not think an array of things that are operated on should
 > not be named "ref_filter_item".
 >
 > Surely, the latter "set of operations to be applied" may currently
 > be only filtering, but who says it has to stay that way?  "I have a
 > set of refs that represent my local branches I am interested
 > in. Please map them to their corresponding @{upstream}" is a
 > reasonable request once you have an infrastructure to represent "set
 > of refs to be worked on" and "set of operations to apply", and at
 > that point, the items are no longer filter-items (map-items?).
 >

That's also a good point to consider, I shall rename and restructure the 
code as discussed here, thanks.

-- 
Regards,
Karthik

[PATCH 3/4] for-each-ref: convert to ref-filter

[Karthik Nayak]

convert 'for-each-ref' to use the common API provided by 'ref-filter'.

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 | 118 +++++++++----------------------------------------
 1 file changed, 20 insertions(+), 98 deletions(-)

diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 2721228..c9875d1 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -10,6 +10,7 @@
 #include "parse-options.h"
 #include "remote.h"
 #include "color.h"
+#include "ref-filter.h"
 
 /* Quoting styles */
 #define QUOTE_NONE 0
@@ -20,25 +21,6 @@
 
 typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
 
-struct atom_value {
-	const char *s;
-	unsigned long ul; /* used for sorting when not FIELD_STR */
-};
-
-struct ref_sort {
-	struct ref_sort *next;
-	int atom; /* index into used_atom array */
-	unsigned reverse : 1;
-};
-
-struct refinfo {
-	char *name;
-	unsigned char sha1[20];
-	int flags;
-	const char *symref;
-	struct atom_value *value;
-};
-
 static struct {
 	const char *name;
 	cmp_type cmp_type;
@@ -85,7 +67,7 @@ static struct {
  * a "*" to denote deref_tag().
  *
  * We parse given format string and sort specifiers, and make a list
- * of properties that we need to extract out of objects.  refinfo
+ * of properties that we need to extract out of objects.  ref_filter_item
  * structure will hold an array of values extracted that can be
  * indexed with the "atom number", which is an index into this
  * array.
@@ -622,7 +604,7 @@ static inline char *copy_advance(char *dst, const char *src)
 /*
  * Parse the object referred by ref, and grab needed value.
  */
-static void populate_value(struct refinfo *ref)
+static void populate_value(struct ref_filter_item *ref)
 {
 	void *buf;
 	struct object *obj;
@@ -821,7 +803,7 @@ static void populate_value(struct refinfo *ref)
  * Given a ref, return the value for the atom.  This lazily gets value
  * out of the object by calling populate value.
  */
-static void get_value(struct refinfo *ref, int atom, struct atom_value **v)
+static void get_value(struct ref_filter_item *ref, int atom, struct atom_value **v)
 {
 	if (!ref->value) {
 		populate_value(ref);
@@ -830,65 +812,7 @@ static void get_value(struct refinfo *ref, int atom, struct atom_value **v)
 	*v = &ref->value[atom];
 }
 
-struct grab_ref_cbdata {
-	struct refinfo **grab_array;
-	const char **grab_pattern;
-	int grab_cnt;
-};
-
-/*
- * A call-back given to for_each_ref().  Filter refs and keep them for
- * later object processing.
- */
-static int grab_single_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)
-{
-	struct grab_ref_cbdata *cb = cb_data;
-	struct refinfo *ref;
-	int cnt;
-
-	if (flag & REF_BAD_NAME) {
-		  warning("ignoring ref with broken name %s", refname);
-		  return 0;
-	}
-
-	if (*cb->grab_pattern) {
-		const char **pattern;
-		int namelen = strlen(refname);
-		for (pattern = cb->grab_pattern; *pattern; pattern++) {
-			const char *p = *pattern;
-			int plen = strlen(p);
-
-			if ((plen <= namelen) &&
-			    !strncmp(refname, p, plen) &&
-			    (refname[plen] == '\0' ||
-			     refname[plen] == '/' ||
-			     p[plen-1] == '/'))
-				break;
-			if (!wildmatch(p, refname, WM_PATHNAME, NULL))
-				break;
-		}
-		if (!*pattern)
-			return 0;
-	}
-
-	/*
-	 * We do not open the object yet; sort may only need refname
-	 * to do its job and the resulting list may yet to be pruned
-	 * by maxcount logic.
-	 */
-	ref = xcalloc(1, sizeof(*ref));
-	ref->name = xstrdup(refname);
-	hashcpy(ref->sha1, sha1);
-	ref->flags = flag;
-
-	cnt = cb->grab_cnt;
-	REALLOC_ARRAY(cb->grab_array, cnt + 1);
-	cb->grab_array[cnt++] = ref;
-	cb->grab_cnt = cnt;
-	return 0;
-}
-
-static int cmp_ref_sort(struct ref_sort *s, struct refinfo *a, struct refinfo *b)
+static int cmp_ref_sort(struct ref_sort *s, struct ref_filter_item *a, struct ref_filter_item *b)
 {
 	struct atom_value *va, *vb;
 	int cmp;
@@ -915,8 +839,8 @@ static int cmp_ref_sort(struct ref_sort *s, struct refinfo *a, struct refinfo *b
 static struct ref_sort *ref_sort;
 static int compare_refs(const void *a_, const void *b_)
 {
-	struct refinfo *a = *((struct refinfo **)a_);
-	struct refinfo *b = *((struct refinfo **)b_);
+	struct ref_filter_item *a = *((struct ref_filter_item **)a_);
+	struct ref_filter_item *b = *((struct ref_filter_item **)b_);
 	struct ref_sort *s;
 
 	for (s = ref_sort; s; s = s->next) {
@@ -927,10 +851,10 @@ static int compare_refs(const void *a_, const void *b_)
 	return 0;
 }
 
-static void sort_refs(struct ref_sort *sort, struct refinfo **refs, int num_refs)
+static void sort_refs(struct ref_sort *sort, struct ref_filter *refs)
 {
 	ref_sort = sort;
-	qsort(refs, num_refs, sizeof(struct refinfo *), compare_refs);
+	qsort(refs->items, refs->count, sizeof(struct ref_filter_item *), compare_refs);
 }
 
 static void print_value(struct atom_value *v, int quote_style)
@@ -997,7 +921,7 @@ static void emit(const char *cp, const char *ep)
 	}
 }
 
-static void show_ref(struct refinfo *info, const char *format, int quote_style)
+static void show_ref(struct ref_filter_item *info, const char *format, int quote_style)
 {
 	const char *cp, *sp, *ep;
 
@@ -1066,12 +990,12 @@ static char const * const for_each_ref_usage[] = {
 
 int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 {
-	int i, num_refs;
+	int i;
 	const char *format = "%(objectname) %(objecttype)\t%(refname)";
 	struct ref_sort *sort = NULL, **sort_tail = &sort;
 	int maxcount = 0, quote_style = 0;
-	struct refinfo **refs;
-	struct grab_ref_cbdata cbdata;
+	struct ref_filter refs;
+	memset(&refs, 0, sizeof(refs));
 
 	struct option opts[] = {
 		OPT_BIT('s', "shell", &quote_style,
@@ -1109,17 +1033,15 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 	/* for warn_ambiguous_refs */
 	git_config(git_default_config, NULL);
 
-	memset(&cbdata, 0, sizeof(cbdata));
-	cbdata.grab_pattern = argv;
-	for_each_rawref(grab_single_ref, &cbdata);
-	refs = cbdata.grab_array;
-	num_refs = cbdata.grab_cnt;
+	refs.name_patterns = argv;
+	for_each_rawref(ref_filter_add, &refs);
 
-	sort_refs(sort, refs, num_refs);
+	sort_refs(sort, &refs);
 
-	if (!maxcount || num_refs < maxcount)
-		maxcount = num_refs;
+	if (!maxcount || refs.count < maxcount)
+		maxcount = refs.count;
 	for (i = 0; i < maxcount; i++)
-		show_ref(refs[i], format, quote_style);
+		show_ref(refs.items[i], format, quote_style);
+	ref_filter_clear(&refs);
 	return 0;
 }
-- 
2.4.1

Re: [PATCH 3/4] for-each-ref: convert to ref-filter

[Junio C Hamano]

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

> convert 'for-each-ref' to use the common API provided by 'ref-filter'.

Start a sentence with capital?

More importantly, the above is misleading, as if you invented a new
ref-filter API and made for-each-ref build on that new
infrastructure.

This series is in a form that is very unfriendly to reviewers.  The
previous step did not introduce any callers to ref-filter, so for
the purpose of review, it needs to be read together with this step
anyway.

And when reading these patches that way, what this half is really
doing is to move the code from for-each-ref to ref-filter, but it
does unnecessary or unrelated renaming of a handful of symbols.  It
makes it even harder to compare and contrast the original code that
was in the original for-each-ref and moved code that ends up in the
new ref-filter.  Don't do that.

You would probably want to organize them in these two steps instead:

 * Rename symbols as necessary while all the code is still in
   for-each-ref. Do not create ref-filter in this step. Justify it
   along the lines of "some symbol names were fine while they were
   file scope static implementation detail of for-each-ref, but we
   will make the machinery available from other commands by moving
   it to a library-ish place, so rename X to foo_X to clarify that
   this is about foo (which is now necessary as it is not specific
   to for-each-ref"...

 * If you want to do other tweaks like wrapping refs & num_refs into
   a single structure, do so while the code is still in
   for-each-ref.  You can do that in the same patch as the above
   (i.e. it's just part of preparatory step for a move).

 * Create ref-filter by _moving_ code from for-each-ref. Do not
   touch these moved lines in this step. You would need to add
   include at the top of for-each-ref and ref-filter, of course.


> -	for_each_rawref(grab_single_ref, &cbdata);
> -	refs = cbdata.grab_array;
> -	num_refs = cbdata.grab_cnt;
> +	refs.name_patterns = argv;
> +	for_each_rawref(ref_filter_add, &refs);

I think ref_filter_add() may be misnamed as a public API function.
grab_single_ref() was OK only because it was an implementation
dtail, but if you are making it public, the name should make it
clear that it is meant to be used as a for_each_*ref() callback
function.  Otherwise people may be tempted to add random parameter
to it in the future, but the signature of that function is dictated
by for_each_*ref() API.

Re: [PATCH 3/4] for-each-ref: convert to ref-filter

[karthik nayak]

On 05/21/2015 05:20 AM, Junio C Hamano wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> convert 'for-each-ref' to use the common API provided by 'ref-filter'.
>
> Start a sentence with capital?
>
> More importantly, the above is misleading, as if you invented a new
> ref-filter API and made for-each-ref build on that new
> infrastructure.
>
> This series is in a form that is very unfriendly to reviewers.  The
> previous step did not introduce any callers to ref-filter, so for
> the purpose of review, it needs to be read together with this step
> anyway.
>
> And when reading these patches that way, what this half is really
> doing is to move the code from for-each-ref to ref-filter, but it
> does unnecessary or unrelated renaming of a handful of symbols.  It
> makes it even harder to compare and contrast the original code that
> was in the original for-each-ref and moved code that ends up in the
> new ref-filter.  Don't do that.
>
> You would probably want to organize them in these two steps instead:
>
>   * Rename symbols as necessary while all the code is still in
>     for-each-ref. Do not create ref-filter in this step. Justify it
>     along the lines of "some symbol names were fine while they were
>     file scope static implementation detail of for-each-ref, but we
>     will make the machinery available from other commands by moving
>     it to a library-ish place, so rename X to foo_X to clarify that
>     this is about foo (which is now necessary as it is not specific
>     to for-each-ref"...
>
>   * If you want to do other tweaks like wrapping refs & num_refs into
>     a single structure, do so while the code is still in
>     for-each-ref.  You can do that in the same patch as the above
>     (i.e. it's just part of preparatory step for a move).
>
>   * Create ref-filter by _moving_ code from for-each-ref. Do not
>     touch these moved lines in this step. You would need to add
>     include at the top of for-each-ref and ref-filter, of course.
>
Thanks for the suggestion's Junio, will follow with the path you've 
mentioned.
>
>> -	for_each_rawref(grab_single_ref, &cbdata);
>> -	refs = cbdata.grab_array;
>> -	num_refs = cbdata.grab_cnt;
>> +	refs.name_patterns = argv;
>> +	for_each_rawref(ref_filter_add, &refs);
>
> I think ref_filter_add() may be misnamed as a public API function.
> grab_single_ref() was OK only because it was an implementation
> dtail, but if you are making it public, the name should make it
> clear that it is meant to be used as a for_each_*ref() callback
> function.  Otherwise people may be tempted to add random parameter
> to it in the future, but the signature of that function is dictated
> by for_each_*ref() API.
>

Looking through the current each_ref_f() names, I thought I could use
'ref_filter_handler()' instead of 'ref_filter_add()', as per your 
suggestion. What do you think?

[PATCH 4/4] ref-filter: move formatting/sorting options from 'for-each-ref'

[Karthik Nayak]

'for-each-ref' uses structures like 'atom_value' and 'ref_sort' to
format or sort the given list of refs, move these functions to
'ref-filter' and make them available as an API so that they can be
used by others.

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 | 985 +------------------------------------------------
 ref-filter.c           | 970 ++++++++++++++++++++++++++++++++++++++++++++++++
 ref-filter.h           |  16 +
 3 files changed, 989 insertions(+), 982 deletions(-)

diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index c9875d1..6a928cd 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -1,987 +1,8 @@
 #include "builtin.h"
 #include "cache.h"
 #include "refs.h"
-#include "object.h"
-#include "tag.h"
-#include "commit.h"
-#include "tree.h"
-#include "blob.h"
-#include "quote.h"
-#include "parse-options.h"
-#include "remote.h"
-#include "color.h"
 #include "ref-filter.h"
-
-/* Quoting styles */
-#define QUOTE_NONE 0
-#define QUOTE_SHELL 1
-#define QUOTE_PERL 2
-#define QUOTE_PYTHON 4
-#define QUOTE_TCL 8
-
-typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
-
-static struct {
-	const char *name;
-	cmp_type cmp_type;
-} valid_atom[] = {
-	{ "refname" },
-	{ "objecttype" },
-	{ "objectsize", FIELD_ULONG },
-	{ "objectname" },
-	{ "tree" },
-	{ "parent" },
-	{ "numparent", FIELD_ULONG },
-	{ "object" },
-	{ "type" },
-	{ "tag" },
-	{ "author" },
-	{ "authorname" },
-	{ "authoremail" },
-	{ "authordate", FIELD_TIME },
-	{ "committer" },
-	{ "committername" },
-	{ "committeremail" },
-	{ "committerdate", FIELD_TIME },
-	{ "tagger" },
-	{ "taggername" },
-	{ "taggeremail" },
-	{ "taggerdate", FIELD_TIME },
-	{ "creator" },
-	{ "creatordate", FIELD_TIME },
-	{ "subject" },
-	{ "body" },
-	{ "contents" },
-	{ "contents:subject" },
-	{ "contents:body" },
-	{ "contents:signature" },
-	{ "upstream" },
-	{ "symref" },
-	{ "flag" },
-	{ "HEAD" },
-	{ "color" },
-};
-
-/*
- * An atom is a valid field atom listed above, possibly prefixed with
- * a "*" to denote deref_tag().
- *
- * We parse given format string and sort specifiers, and make a list
- * of properties that we need to extract out of objects.  ref_filter_item
- * structure will hold an array of values extracted that can be
- * indexed with the "atom number", which is an index into this
- * array.
- */
-static const char **used_atom;
-static cmp_type *used_atom_type;
-static int used_atom_cnt, need_tagged, need_symref;
-static int need_color_reset_at_eol;
-
-/*
- * Used to parse format string and sort specifiers
- */
-static int parse_atom(const char *atom, const char *ep)
-{
-	const char *sp;
-	int i, at;
-
-	sp = atom;
-	if (*sp == '*' && sp < ep)
-		sp++; /* deref */
-	if (ep <= sp)
-		die("malformed field name: %.*s", (int)(ep-atom), atom);
-
-	/* Do we have the atom already used elsewhere? */
-	for (i = 0; i < used_atom_cnt; i++) {
-		int len = strlen(used_atom[i]);
-		if (len == ep - atom && !memcmp(used_atom[i], atom, len))
-			return i;
-	}
-
-	/* Is the atom a valid one? */
-	for (i = 0; i < ARRAY_SIZE(valid_atom); i++) {
-		int len = strlen(valid_atom[i].name);
-		/*
-		 * If the atom name has a colon, strip it and everything after
-		 * it off - it specifies the format for this entry, and
-		 * shouldn't be used for checking against the valid_atom
-		 * table.
-		 */
-		const char *formatp = strchr(sp, ':');
-		if (!formatp || ep < formatp)
-			formatp = ep;
-		if (len == formatp - sp && !memcmp(valid_atom[i].name, sp, len))
-			break;
-	}
-
-	if (ARRAY_SIZE(valid_atom) <= i)
-		die("unknown field name: %.*s", (int)(ep-atom), atom);
-
-	/* Add it in, including the deref prefix */
-	at = used_atom_cnt;
-	used_atom_cnt++;
-	REALLOC_ARRAY(used_atom, used_atom_cnt);
-	REALLOC_ARRAY(used_atom_type, used_atom_cnt);
-	used_atom[at] = xmemdupz(atom, ep - atom);
-	used_atom_type[at] = valid_atom[i].cmp_type;
-	if (*atom == '*')
-		need_tagged = 1;
-	if (!strcmp(used_atom[at], "symref"))
-		need_symref = 1;
-	return at;
-}
-
-/*
- * In a format string, find the next occurrence of %(atom).
- */
-static const char *find_next(const char *cp)
-{
-	while (*cp) {
-		if (*cp == '%') {
-			/*
-			 * %( is the start of an atom;
-			 * %% is a quoted per-cent.
-			 */
-			if (cp[1] == '(')
-				return cp;
-			else if (cp[1] == '%')
-				cp++; /* skip over two % */
-			/* otherwise this is a singleton, literal % */
-		}
-		cp++;
-	}
-	return NULL;
-}
-
-/*
- * Make sure the format string is well formed, and parse out
- * the used atoms.
- */
-static int verify_format(const char *format)
-{
-	const char *cp, *sp;
-
-	need_color_reset_at_eol = 0;
-	for (cp = format; *cp && (sp = find_next(cp)); ) {
-		const char *color, *ep = strchr(sp, ')');
-		int at;
-
-		if (!ep)
-			return error("malformed format string %s", sp);
-		/* sp points at "%(" and ep points at the closing ")" */
-		at = parse_atom(sp + 2, ep);
-		cp = ep + 1;
-
-		if (skip_prefix(used_atom[at], "color:", &color))
-			need_color_reset_at_eol = !!strcmp(color, "reset");
-	}
-	return 0;
-}
-
-/*
- * Given an object name, read the object data and size, and return a
- * "struct object".  If the object data we are returning is also borrowed
- * by the "struct object" representation, set *eaten as well---it is a
- * signal from parse_object_buffer to us not to free the buffer.
- */
-static void *get_obj(const unsigned char *sha1, struct object **obj, unsigned long *sz, int *eaten)
-{
-	enum object_type type;
-	void *buf = read_sha1_file(sha1, &type, sz);
-
-	if (buf)
-		*obj = parse_object_buffer(sha1, type, *sz, buf, eaten);
-	else
-		*obj = NULL;
-	return buf;
-}
-
-static int grab_objectname(const char *name, const unsigned char *sha1,
-			    struct atom_value *v)
-{
-	if (!strcmp(name, "objectname")) {
-		char *s = xmalloc(41);
-		strcpy(s, sha1_to_hex(sha1));
-		v->s = s;
-		return 1;
-	}
-	if (!strcmp(name, "objectname:short")) {
-		v->s = xstrdup(find_unique_abbrev(sha1, DEFAULT_ABBREV));
-		return 1;
-	}
-	return 0;
-}
-
-/* See grab_values */
-static void grab_common_values(struct atom_value *val, int deref, struct object *obj, void *buf, unsigned long sz)
-{
-	int i;
-
-	for (i = 0; i < used_atom_cnt; i++) {
-		const char *name = used_atom[i];
-		struct atom_value *v = &val[i];
-		if (!!deref != (*name == '*'))
-			continue;
-		if (deref)
-			name++;
-		if (!strcmp(name, "objecttype"))
-			v->s = typename(obj->type);
-		else if (!strcmp(name, "objectsize")) {
-			char *s = xmalloc(40);
-			sprintf(s, "%lu", sz);
-			v->ul = sz;
-			v->s = s;
-		}
-		else if (deref)
-			grab_objectname(name, obj->sha1, v);
-	}
-}
-
-/* See grab_values */
-static void grab_tag_values(struct atom_value *val, int deref, struct object *obj, void *buf, unsigned long sz)
-{
-	int i;
-	struct tag *tag = (struct tag *) obj;
-
-	for (i = 0; i < used_atom_cnt; i++) {
-		const char *name = used_atom[i];
-		struct atom_value *v = &val[i];
-		if (!!deref != (*name == '*'))
-			continue;
-		if (deref)
-			name++;
-		if (!strcmp(name, "tag"))
-			v->s = tag->tag;
-		else if (!strcmp(name, "type") && tag->tagged)
-			v->s = typename(tag->tagged->type);
-		else if (!strcmp(name, "object") && tag->tagged) {
-			char *s = xmalloc(41);
-			strcpy(s, sha1_to_hex(tag->tagged->sha1));
-			v->s = s;
-		}
-	}
-}
-
-/* See grab_values */
-static void grab_commit_values(struct atom_value *val, int deref, struct object *obj, void *buf, unsigned long sz)
-{
-	int i;
-	struct commit *commit = (struct commit *) obj;
-
-	for (i = 0; i < used_atom_cnt; i++) {
-		const char *name = used_atom[i];
-		struct atom_value *v = &val[i];
-		if (!!deref != (*name == '*'))
-			continue;
-		if (deref)
-			name++;
-		if (!strcmp(name, "tree")) {
-			char *s = xmalloc(41);
-			strcpy(s, sha1_to_hex(commit->tree->object.sha1));
-			v->s = s;
-		}
-		if (!strcmp(name, "numparent")) {
-			char *s = xmalloc(40);
-			v->ul = commit_list_count(commit->parents);
-			sprintf(s, "%lu", v->ul);
-			v->s = s;
-		}
-		else if (!strcmp(name, "parent")) {
-			int num = commit_list_count(commit->parents);
-			int i;
-			struct commit_list *parents;
-			char *s = xmalloc(41 * num + 1);
-			v->s = s;
-			for (i = 0, parents = commit->parents;
-			     parents;
-			     parents = parents->next, i = i + 41) {
-				struct commit *parent = parents->item;
-				strcpy(s+i, sha1_to_hex(parent->object.sha1));
-				if (parents->next)
-					s[i+40] = ' ';
-			}
-			if (!i)
-				*s = '\0';
-		}
-	}
-}
-
-static const char *find_wholine(const char *who, int wholen, const char *buf, unsigned long sz)
-{
-	const char *eol;
-	while (*buf) {
-		if (!strncmp(buf, who, wholen) &&
-		    buf[wholen] == ' ')
-			return buf + wholen + 1;
-		eol = strchr(buf, '\n');
-		if (!eol)
-			return "";
-		eol++;
-		if (*eol == '\n')
-			return ""; /* end of header */
-		buf = eol;
-	}
-	return "";
-}
-
-static const char *copy_line(const char *buf)
-{
-	const char *eol = strchrnul(buf, '\n');
-	return xmemdupz(buf, eol - buf);
-}
-
-static const char *copy_name(const char *buf)
-{
-	const char *cp;
-	for (cp = buf; *cp && *cp != '\n'; cp++) {
-		if (!strncmp(cp, " <", 2))
-			return xmemdupz(buf, cp - buf);
-	}
-	return "";
-}
-
-static const char *copy_email(const char *buf)
-{
-	const char *email = strchr(buf, '<');
-	const char *eoemail;
-	if (!email)
-		return "";
-	eoemail = strchr(email, '>');
-	if (!eoemail)
-		return "";
-	return xmemdupz(email, eoemail + 1 - email);
-}
-
-static char *copy_subject(const char *buf, unsigned long len)
-{
-	char *r = xmemdupz(buf, len);
-	int i;
-
-	for (i = 0; i < len; i++)
-		if (r[i] == '\n')
-			r[i] = ' ';
-
-	return r;
-}
-
-static void grab_date(const char *buf, struct atom_value *v, const char *atomname)
-{
-	const char *eoemail = strstr(buf, "> ");
-	char *zone;
-	unsigned long timestamp;
-	long tz;
-	enum date_mode date_mode = DATE_NORMAL;
-	const char *formatp;
-
-	/*
-	 * We got here because atomname ends in "date" or "date<something>";
-	 * it's not possible that <something> is not ":<format>" because
-	 * parse_atom() wouldn't have allowed it, so we can assume that no
-	 * ":" means no format is specified, and use the default.
-	 */
-	formatp = strchr(atomname, ':');
-	if (formatp != NULL) {
-		formatp++;
-		date_mode = parse_date_format(formatp);
-	}
-
-	if (!eoemail)
-		goto bad;
-	timestamp = strtoul(eoemail + 2, &zone, 10);
-	if (timestamp == ULONG_MAX)
-		goto bad;
-	tz = strtol(zone, NULL, 10);
-	if ((tz == LONG_MIN || tz == LONG_MAX) && errno == ERANGE)
-		goto bad;
-	v->s = xstrdup(show_date(timestamp, tz, date_mode));
-	v->ul = timestamp;
-	return;
- bad:
-	v->s = "";
-	v->ul = 0;
-}
-
-/* See grab_values */
-static void grab_person(const char *who, struct atom_value *val, int deref, struct object *obj, void *buf, unsigned long sz)
-{
-	int i;
-	int wholen = strlen(who);
-	const char *wholine = NULL;
-
-	for (i = 0; i < used_atom_cnt; i++) {
-		const char *name = used_atom[i];
-		struct atom_value *v = &val[i];
-		if (!!deref != (*name == '*'))
-			continue;
-		if (deref)
-			name++;
-		if (strncmp(who, name, wholen))
-			continue;
-		if (name[wholen] != 0 &&
-		    strcmp(name + wholen, "name") &&
-		    strcmp(name + wholen, "email") &&
-		    !starts_with(name + wholen, "date"))
-			continue;
-		if (!wholine)
-			wholine = find_wholine(who, wholen, buf, sz);
-		if (!wholine)
-			return; /* no point looking for it */
-		if (name[wholen] == 0)
-			v->s = copy_line(wholine);
-		else if (!strcmp(name + wholen, "name"))
-			v->s = copy_name(wholine);
-		else if (!strcmp(name + wholen, "email"))
-			v->s = copy_email(wholine);
-		else if (starts_with(name + wholen, "date"))
-			grab_date(wholine, v, name);
-	}
-
-	/*
-	 * For a tag or a commit object, if "creator" or "creatordate" is
-	 * requested, do something special.
-	 */
-	if (strcmp(who, "tagger") && strcmp(who, "committer"))
-		return; /* "author" for commit object is not wanted */
-	if (!wholine)
-		wholine = find_wholine(who, wholen, buf, sz);
-	if (!wholine)
-		return;
-	for (i = 0; i < used_atom_cnt; i++) {
-		const char *name = used_atom[i];
-		struct atom_value *v = &val[i];
-		if (!!deref != (*name == '*'))
-			continue;
-		if (deref)
-			name++;
-
-		if (starts_with(name, "creatordate"))
-			grab_date(wholine, v, name);
-		else if (!strcmp(name, "creator"))
-			v->s = copy_line(wholine);
-	}
-}
-
-static void find_subpos(const char *buf, unsigned long sz,
-			const char **sub, unsigned long *sublen,
-			const char **body, unsigned long *bodylen,
-			unsigned long *nonsiglen,
-			const char **sig, unsigned long *siglen)
-{
-	const char *eol;
-	/* skip past header until we hit empty line */
-	while (*buf && *buf != '\n') {
-		eol = strchrnul(buf, '\n');
-		if (*eol)
-			eol++;
-		buf = eol;
-	}
-	/* skip any empty lines */
-	while (*buf == '\n')
-		buf++;
-
-	/* parse signature first; we might not even have a subject line */
-	*sig = buf + parse_signature(buf, strlen(buf));
-	*siglen = strlen(*sig);
-
-	/* subject is first non-empty line */
-	*sub = buf;
-	/* subject goes to first empty line */
-	while (buf < *sig && *buf && *buf != '\n') {
-		eol = strchrnul(buf, '\n');
-		if (*eol)
-			eol++;
-		buf = eol;
-	}
-	*sublen = buf - *sub;
-	/* drop trailing newline, if present */
-	if (*sublen && (*sub)[*sublen - 1] == '\n')
-		*sublen -= 1;
-
-	/* skip any empty lines */
-	while (*buf == '\n')
-		buf++;
-	*body = buf;
-	*bodylen = strlen(buf);
-	*nonsiglen = *sig - buf;
-}
-
-/* See grab_values */
-static void grab_sub_body_contents(struct atom_value *val, int deref, struct object *obj, void *buf, unsigned long sz)
-{
-	int i;
-	const char *subpos = NULL, *bodypos = NULL, *sigpos = NULL;
-	unsigned long sublen = 0, bodylen = 0, nonsiglen = 0, siglen = 0;
-
-	for (i = 0; i < used_atom_cnt; i++) {
-		const char *name = used_atom[i];
-		struct atom_value *v = &val[i];
-		if (!!deref != (*name == '*'))
-			continue;
-		if (deref)
-			name++;
-		if (strcmp(name, "subject") &&
-		    strcmp(name, "body") &&
-		    strcmp(name, "contents") &&
-		    strcmp(name, "contents:subject") &&
-		    strcmp(name, "contents:body") &&
-		    strcmp(name, "contents:signature"))
-			continue;
-		if (!subpos)
-			find_subpos(buf, sz,
-				    &subpos, &sublen,
-				    &bodypos, &bodylen, &nonsiglen,
-				    &sigpos, &siglen);
-
-		if (!strcmp(name, "subject"))
-			v->s = copy_subject(subpos, sublen);
-		else if (!strcmp(name, "contents:subject"))
-			v->s = copy_subject(subpos, sublen);
-		else if (!strcmp(name, "body"))
-			v->s = xmemdupz(bodypos, bodylen);
-		else if (!strcmp(name, "contents:body"))
-			v->s = xmemdupz(bodypos, nonsiglen);
-		else if (!strcmp(name, "contents:signature"))
-			v->s = xmemdupz(sigpos, siglen);
-		else if (!strcmp(name, "contents"))
-			v->s = xstrdup(subpos);
-	}
-}
-
-/*
- * We want to have empty print-string for field requests
- * that do not apply (e.g. "authordate" for a tag object)
- */
-static void fill_missing_values(struct atom_value *val)
-{
-	int i;
-	for (i = 0; i < used_atom_cnt; i++) {
-		struct atom_value *v = &val[i];
-		if (v->s == NULL)
-			v->s = "";
-	}
-}
-
-/*
- * val is a list of atom_value to hold returned values.  Extract
- * the values for atoms in used_atom array out of (obj, buf, sz).
- * when deref is false, (obj, buf, sz) is the object that is
- * pointed at by the ref itself; otherwise it is the object the
- * ref (which is a tag) refers to.
- */
-static void grab_values(struct atom_value *val, int deref, struct object *obj, void *buf, unsigned long sz)
-{
-	grab_common_values(val, deref, obj, buf, sz);
-	switch (obj->type) {
-	case OBJ_TAG:
-		grab_tag_values(val, deref, obj, buf, sz);
-		grab_sub_body_contents(val, deref, obj, buf, sz);
-		grab_person("tagger", val, deref, obj, buf, sz);
-		break;
-	case OBJ_COMMIT:
-		grab_commit_values(val, deref, obj, buf, sz);
-		grab_sub_body_contents(val, deref, obj, buf, sz);
-		grab_person("author", val, deref, obj, buf, sz);
-		grab_person("committer", val, deref, obj, buf, sz);
-		break;
-	case OBJ_TREE:
-		/* grab_tree_values(val, deref, obj, buf, sz); */
-		break;
-	case OBJ_BLOB:
-		/* grab_blob_values(val, deref, obj, buf, sz); */
-		break;
-	default:
-		die("Eh?  Object of type %d?", obj->type);
-	}
-}
-
-static inline char *copy_advance(char *dst, const char *src)
-{
-	while (*src)
-		*dst++ = *src++;
-	return dst;
-}
-
-/*
- * Parse the object referred by ref, and grab needed value.
- */
-static void populate_value(struct ref_filter_item *ref)
-{
-	void *buf;
-	struct object *obj;
-	int eaten, i;
-	unsigned long size;
-	const unsigned char *tagged;
-
-	ref->value = xcalloc(used_atom_cnt, sizeof(struct atom_value));
-
-	if (need_symref && (ref->flags & REF_ISSYMREF) && !ref->symref) {
-		unsigned char unused1[20];
-		ref->symref = resolve_refdup(ref->name, RESOLVE_REF_READING,
-					     unused1, NULL);
-		if (!ref->symref)
-			ref->symref = "";
-	}
-
-	/* Fill in specials first */
-	for (i = 0; i < used_atom_cnt; i++) {
-		const char *name = used_atom[i];
-		struct atom_value *v = &ref->value[i];
-		int deref = 0;
-		const char *refname;
-		const char *formatp;
-		struct branch *branch = NULL;
-
-		if (*name == '*') {
-			deref = 1;
-			name++;
-		}
-
-		if (starts_with(name, "refname"))
-			refname = ref->name;
-		else if (starts_with(name, "symref"))
-			refname = ref->symref ? ref->symref : "";
-		else if (starts_with(name, "upstream")) {
-			/* only local branches may have an upstream */
-			if (!starts_with(ref->name, "refs/heads/"))
-				continue;
-			branch = branch_get(ref->name + 11);
-
-			if (!branch || !branch->merge || !branch->merge[0] ||
-			    !branch->merge[0]->dst)
-				continue;
-			refname = branch->merge[0]->dst;
-		} else if (starts_with(name, "color:")) {
-			char color[COLOR_MAXLEN] = "";
-
-			if (color_parse(name + 6, color) < 0)
-				die(_("unable to parse format"));
-			v->s = xstrdup(color);
-			continue;
-		} else if (!strcmp(name, "flag")) {
-			char buf[256], *cp = buf;
-			if (ref->flags & REF_ISSYMREF)
-				cp = copy_advance(cp, ",symref");
-			if (ref->flags & REF_ISPACKED)
-				cp = copy_advance(cp, ",packed");
-			if (cp == buf)
-				v->s = "";
-			else {
-				*cp = '\0';
-				v->s = xstrdup(buf + 1);
-			}
-			continue;
-		} else if (!deref && grab_objectname(name, ref->sha1, v)) {
-			continue;
-		} else if (!strcmp(name, "HEAD")) {
-			const char *head;
-			unsigned char sha1[20];
-
-			head = resolve_ref_unsafe("HEAD", RESOLVE_REF_READING,
-						  sha1, NULL);
-			if (!strcmp(ref->name, head))
-				v->s = "*";
-			else
-				v->s = " ";
-			continue;
-		} else
-			continue;
-
-		formatp = strchr(name, ':');
-		if (formatp) {
-			int num_ours, num_theirs;
-
-			formatp++;
-			if (!strcmp(formatp, "short"))
-				refname = shorten_unambiguous_ref(refname,
-						      warn_ambiguous_refs);
-			else if (!strcmp(formatp, "track") &&
-				 starts_with(name, "upstream")) {
-				char buf[40];
-
-				if (stat_tracking_info(branch, &num_ours,
-						       &num_theirs) != 1)
-					continue;
-
-				if (!num_ours && !num_theirs)
-					v->s = "";
-				else if (!num_ours) {
-					sprintf(buf, "[behind %d]", num_theirs);
-					v->s = xstrdup(buf);
-				} else if (!num_theirs) {
-					sprintf(buf, "[ahead %d]", num_ours);
-					v->s = xstrdup(buf);
-				} else {
-					sprintf(buf, "[ahead %d, behind %d]",
-						num_ours, num_theirs);
-					v->s = xstrdup(buf);
-				}
-				continue;
-			} else if (!strcmp(formatp, "trackshort") &&
-				   starts_with(name, "upstream")) {
-				assert(branch);
-
-				if (stat_tracking_info(branch, &num_ours,
-							&num_theirs) != 1)
-					continue;
-
-				if (!num_ours && !num_theirs)
-					v->s = "=";
-				else if (!num_ours)
-					v->s = "<";
-				else if (!num_theirs)
-					v->s = ">";
-				else
-					v->s = "<>";
-				continue;
-			} else
-				die("unknown %.*s format %s",
-				    (int)(formatp - name), name, formatp);
-		}
-
-		if (!deref)
-			v->s = refname;
-		else {
-			int len = strlen(refname);
-			char *s = xmalloc(len + 4);
-			sprintf(s, "%s^{}", refname);
-			v->s = s;
-		}
-	}
-
-	for (i = 0; i < used_atom_cnt; i++) {
-		struct atom_value *v = &ref->value[i];
-		if (v->s == NULL)
-			goto need_obj;
-	}
-	return;
-
- need_obj:
-	buf = get_obj(ref->sha1, &obj, &size, &eaten);
-	if (!buf)
-		die("missing object %s for %s",
-		    sha1_to_hex(ref->sha1), ref->name);
-	if (!obj)
-		die("parse_object_buffer failed on %s for %s",
-		    sha1_to_hex(ref->sha1), ref->name);
-
-	grab_values(ref->value, 0, obj, buf, size);
-	if (!eaten)
-		free(buf);
-
-	/*
-	 * If there is no atom that wants to know about tagged
-	 * object, we are done.
-	 */
-	if (!need_tagged || (obj->type != OBJ_TAG))
-		return;
-
-	/*
-	 * If it is a tag object, see if we use a value that derefs
-	 * the object, and if we do grab the object it refers to.
-	 */
-	tagged = ((struct tag *)obj)->tagged->sha1;
-
-	/*
-	 * NEEDSWORK: This derefs tag only once, which
-	 * is good to deal with chains of trust, but
-	 * is not consistent with what deref_tag() does
-	 * which peels the onion to the core.
-	 */
-	buf = get_obj(tagged, &obj, &size, &eaten);
-	if (!buf)
-		die("missing object %s for %s",
-		    sha1_to_hex(tagged), ref->name);
-	if (!obj)
-		die("parse_object_buffer failed on %s for %s",
-		    sha1_to_hex(tagged), ref->name);
-	grab_values(ref->value, 1, obj, buf, size);
-	if (!eaten)
-		free(buf);
-}
-
-/*
- * Given a ref, return the value for the atom.  This lazily gets value
- * out of the object by calling populate value.
- */
-static void get_value(struct ref_filter_item *ref, int atom, struct atom_value **v)
-{
-	if (!ref->value) {
-		populate_value(ref);
-		fill_missing_values(ref->value);
-	}
-	*v = &ref->value[atom];
-}
-
-static int cmp_ref_sort(struct ref_sort *s, struct ref_filter_item *a, struct ref_filter_item *b)
-{
-	struct atom_value *va, *vb;
-	int cmp;
-	cmp_type cmp_type = used_atom_type[s->atom];
-
-	get_value(a, s->atom, &va);
-	get_value(b, s->atom, &vb);
-	switch (cmp_type) {
-	case FIELD_STR:
-		cmp = strcmp(va->s, vb->s);
-		break;
-	default:
-		if (va->ul < vb->ul)
-			cmp = -1;
-		else if (va->ul == vb->ul)
-			cmp = 0;
-		else
-			cmp = 1;
-		break;
-	}
-	return (s->reverse) ? -cmp : cmp;
-}
-
-static struct ref_sort *ref_sort;
-static int compare_refs(const void *a_, const void *b_)
-{
-	struct ref_filter_item *a = *((struct ref_filter_item **)a_);
-	struct ref_filter_item *b = *((struct ref_filter_item **)b_);
-	struct ref_sort *s;
-
-	for (s = ref_sort; s; s = s->next) {
-		int cmp = cmp_ref_sort(s, a, b);
-		if (cmp)
-			return cmp;
-	}
-	return 0;
-}
-
-static void sort_refs(struct ref_sort *sort, struct ref_filter *refs)
-{
-	ref_sort = sort;
-	qsort(refs->items, refs->count, sizeof(struct ref_filter_item *), compare_refs);
-}
-
-static void print_value(struct atom_value *v, int quote_style)
-{
-	struct strbuf sb = STRBUF_INIT;
-	switch (quote_style) {
-	case QUOTE_NONE:
-		fputs(v->s, stdout);
-		break;
-	case QUOTE_SHELL:
-		sq_quote_buf(&sb, v->s);
-		break;
-	case QUOTE_PERL:
-		perl_quote_buf(&sb, v->s);
-		break;
-	case QUOTE_PYTHON:
-		python_quote_buf(&sb, v->s);
-		break;
-	case QUOTE_TCL:
-		tcl_quote_buf(&sb, v->s);
-		break;
-	}
-	if (quote_style != QUOTE_NONE) {
-		fputs(sb.buf, stdout);
-		strbuf_release(&sb);
-	}
-}
-
-static int hex1(char ch)
-{
-	if ('0' <= ch && ch <= '9')
-		return ch - '0';
-	else if ('a' <= ch && ch <= 'f')
-		return ch - 'a' + 10;
-	else if ('A' <= ch && ch <= 'F')
-		return ch - 'A' + 10;
-	return -1;
-}
-static int hex2(const char *cp)
-{
-	if (cp[0] && cp[1])
-		return (hex1(cp[0]) << 4) | hex1(cp[1]);
-	else
-		return -1;
-}
-
-static void emit(const char *cp, const char *ep)
-{
-	while (*cp && (!ep || cp < ep)) {
-		if (*cp == '%') {
-			if (cp[1] == '%')
-				cp++;
-			else {
-				int ch = hex2(cp + 1);
-				if (0 <= ch) {
-					putchar(ch);
-					cp += 3;
-					continue;
-				}
-			}
-		}
-		putchar(*cp);
-		cp++;
-	}
-}
-
-static void show_ref(struct ref_filter_item *info, const char *format, int quote_style)
-{
-	const char *cp, *sp, *ep;
-
-	for (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {
-		struct atom_value *atomv;
-
-		ep = strchr(sp, ')');
-		if (cp < sp)
-			emit(cp, sp);
-		get_value(info, parse_atom(sp + 2, ep), &atomv);
-		print_value(atomv, quote_style);
-	}
-	if (*cp) {
-		sp = cp + strlen(cp);
-		emit(cp, sp);
-	}
-	if (need_color_reset_at_eol) {
-		struct atom_value resetv;
-		char color[COLOR_MAXLEN] = "";
-
-		if (color_parse("reset", color) < 0)
-			die("BUG: couldn't parse 'reset' as a color");
-		resetv.s = color;
-		print_value(&resetv, quote_style);
-	}
-	putchar('\n');
-}
-
-static struct ref_sort *default_sort(void)
-{
-	static const char cstr_name[] = "refname";
-
-	struct ref_sort *sort = xcalloc(1, sizeof(*sort));
-
-	sort->next = NULL;
-	sort->atom = parse_atom(cstr_name, cstr_name + strlen(cstr_name));
-	return sort;
-}
-
-static int opt_parse_sort(const struct option *opt, const char *arg, int unset)
-{
-	struct ref_sort **sort_tail = opt->value;
-	struct ref_sort *s;
-	int len;
-
-	if (!arg) /* should --no-sort void the list ? */
-		return -1;
-
-	s = xcalloc(1, sizeof(*s));
-	s->next = *sort_tail;
-	*sort_tail = s;
-
-	if (*arg == '-') {
-		s->reverse = 1;
-		arg++;
-	}
-	len = strlen(arg);
-	s->atom = parse_atom(arg, arg+len);
-	return 0;
-}
+#include "parse-options.h"
 
 static char const * const for_each_ref_usage[] = {
 	N_("git for-each-ref [<options>] [<pattern>]"),
@@ -1011,7 +32,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 		OPT_INTEGER( 0 , "count", &maxcount, N_("show only <n> matched refs")),
 		OPT_STRING(  0 , "format", &format, N_("format"), N_("format to use for the output")),
 		OPT_CALLBACK(0 , "sort", sort_tail, N_("key"),
-			    N_("field name to sort on"), &opt_parse_sort),
+			     N_("field name to sort on"), &ref_opt_parse_sort),
 		OPT_END(),
 	};
 
@@ -1028,7 +49,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
 		usage_with_options(for_each_ref_usage, opts);
 
 	if (!sort)
-		sort = default_sort();
+		sort = ref_default_sort();
 
 	/* for warn_ambiguous_refs */
 	git_config(git_default_config, NULL);
diff --git a/ref-filter.c b/ref-filter.c
index df571a6..42f0f35 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1,9 +1,979 @@
 #include "builtin.h"
 #include "cache.h"
+#include "parse-options.h"
 #include "refs.h"
 #include "ref-filter.h"
 #include "wildmatch.h"
 #include "commit.h"
+#include "remote.h"
+#include "color.h"
+#include "tag.h"
+#include "quote.h"
+
+typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
+
+static struct {
+	const char *name;
+	cmp_type cmp_type;
+} valid_atom[] = {
+	{ "refname" },
+	{ "objecttype" },
+	{ "objectsize", FIELD_ULONG },
+	{ "objectname" },
+	{ "tree" },
+	{ "parent" },
+	{ "numparent", FIELD_ULONG },
+	{ "object" },
+	{ "type" },
+	{ "tag" },
+	{ "author" },
+	{ "authorname" },
+	{ "authoremail" },
+	{ "authordate", FIELD_TIME },
+	{ "committer" },
+	{ "committername" },
+	{ "committeremail" },
+	{ "committerdate", FIELD_TIME },
+	{ "tagger" },
+	{ "taggername" },
+	{ "taggeremail" },
+	{ "taggerdate", FIELD_TIME },
+	{ "creator" },
+	{ "creatordate", FIELD_TIME },
+	{ "subject" },
+	{ "body" },
+	{ "contents" },
+	{ "contents:subject" },
+	{ "contents:body" },
+	{ "contents:signature" },
+	{ "upstream" },
+	{ "symref" },
+	{ "flag" },
+	{ "HEAD" },
+	{ "color" },
+};
+
+/*
+ * An atom is a valid field atom listed above, possibly prefixed with
+ * a "*" to denote deref_tag().
+ *
+ * We parse given format string and sort specifiers, and make a list
+ * of properties that we need to extract out of objects.  ref_filter_item
+ * structure will hold an array of values extracted that can be
+ * indexed with the "atom number", which is an index into this
+ * array.
+ */
+static const char **used_atom;
+static cmp_type *used_atom_type;
+static int used_atom_cnt, need_tagged, need_symref;
+int need_color_reset_at_eol;
+
+/*
+ * Used to parse format string and sort specifiers
+ */
+int parse_atom(const char *atom, const char *ep)
+{
+	const char *sp;
+	int i, at;
+
+	sp = atom;
+	if (*sp == '*' && sp < ep)
+		sp++; /* deref */
+	if (ep <= sp)
+		die("malformed field name: %.*s", (int)(ep-atom), atom);
+
+	/* Do we have the atom already used elsewhere? */
+	for (i = 0; i < used_atom_cnt; i++) {
+		int len = strlen(used_atom[i]);
+		if (len == ep - atom && !memcmp(used_atom[i], atom, len))
+			return i;
+	}
+
+	/* Is the atom a valid one? */
+	for (i = 0; i < ARRAY_SIZE(valid_atom); i++) {
+		int len = strlen(valid_atom[i].name);
+		/*
+		 * If the atom name has a colon, strip it and everything after
+		 * it off - it specifies the format for this entry, and
+		 * shouldn't be used for checking against the valid_atom
+		 * table.
+		 */
+		const char *formatp = strchr(sp, ':');
+		if (!formatp || ep < formatp)
+			formatp = ep;
+		if (len == formatp - sp && !memcmp(valid_atom[i].name, sp, len))
+			break;
+	}
+
+	if (ARRAY_SIZE(valid_atom) <= i)
+		die("unknown field name: %.*s", (int)(ep-atom), atom);
+
+	/* Add it in, including the deref prefix */
+	at = used_atom_cnt;
+	used_atom_cnt++;
+	REALLOC_ARRAY(used_atom, used_atom_cnt);
+	REALLOC_ARRAY(used_atom_type, used_atom_cnt);
+	used_atom[at] = xmemdupz(atom, ep - atom);
+	used_atom_type[at] = valid_atom[i].cmp_type;
+	if (*atom == '*')
+		need_tagged = 1;
+	if (!strcmp(used_atom[at], "symref"))
+		need_symref = 1;
+	return at;
+}
+
+/*
+ * In a format string, find the next occurrence of %(atom).
+ */
+static const char *find_next(const char *cp)
+{
+	while (*cp) {
+		if (*cp == '%') {
+			/*
+			 * %( is the start of an atom;
+			 * %% is a quoted per-cent.
+			 */
+			if (cp[1] == '(')
+				return cp;
+			else if (cp[1] == '%')
+				cp++; /* skip over two % */
+			/* otherwise this is a singleton, literal % */
+		}
+		cp++;
+	}
+	return NULL;
+}
+
+/*
+ * Make sure the format string is well formed, and parse out
+ * the used atoms.
+ */
+int verify_format(const char *format)
+{
+	const char *cp, *sp;
+
+	need_color_reset_at_eol = 0;
+	for (cp = format; *cp && (sp = find_next(cp)); ) {
+		const char *color, *ep = strchr(sp, ')');
+		int at;
+
+		if (!ep)
+			return error("malformed format string %s", sp);
+		/* sp points at "%(" and ep points at the closing ")" */
+		at = parse_atom(sp + 2, ep);
+		cp = ep + 1;
+
+		if (skip_prefix(used_atom[at], "color:", &color))
+			need_color_reset_at_eol = !!strcmp(color, "reset");
+	}
+	return 0;
+}
+
+/*
+ * Given an object name, read the object data and size, and return a
+ * "struct object".  If the object data we are returning is also borrowed
+ * by the "struct object" representation, set *eaten as well---it is a
+ * signal from parse_object_buffer to us not to free the buffer.
+ */
+static void *get_obj(const unsigned char *sha1, struct object **obj, unsigned long *sz, int *eaten)
+{
+	enum object_type type;
+	void *buf = read_sha1_file(sha1, &type, sz);
+
+	if (buf)
+		*obj = parse_object_buffer(sha1, type, *sz, buf, eaten);
+	else
+		*obj = NULL;
+	return buf;
+}
+
+static int grab_objectname(const char *name, const unsigned char *sha1,
+			    struct atom_value *v)
+{
+	if (!strcmp(name, "objectname")) {
+		char *s = xmalloc(41);
+		strcpy(s, sha1_to_hex(sha1));
+		v->s = s;
+		return 1;
+	}
+	if (!strcmp(name, "objectname:short")) {
+		v->s = xstrdup(find_unique_abbrev(sha1, DEFAULT_ABBREV));
+		return 1;
+	}
+	return 0;
+}
+
+/* See grab_values */
+static void grab_common_values(struct atom_value *val, int deref, struct object *obj, void *buf, unsigned long sz)
+{
+	int i;
+
+	for (i = 0; i < used_atom_cnt; i++) {
+		const char *name = used_atom[i];
+		struct atom_value *v = &val[i];
+		if (!!deref != (*name == '*'))
+			continue;
+		if (deref)
+			name++;
+		if (!strcmp(name, "objecttype"))
+			v->s = typename(obj->type);
+		else if (!strcmp(name, "objectsize")) {
+			char *s = xmalloc(40);
+			sprintf(s, "%lu", sz);
+			v->ul = sz;
+			v->s = s;
+		}
+		else if (deref)
+			grab_objectname(name, obj->sha1, v);
+	}
+}
+
+/* See grab_values */
+static void grab_tag_values(struct atom_value *val, int deref, struct object *obj, void *buf, unsigned long sz)
+{
+	int i;
+	struct tag *tag = (struct tag *) obj;
+
+	for (i = 0; i < used_atom_cnt; i++) {
+		const char *name = used_atom[i];
+		struct atom_value *v = &val[i];
+		if (!!deref != (*name == '*'))
+			continue;
+		if (deref)
+			name++;
+		if (!strcmp(name, "tag"))
+			v->s = tag->tag;
+		else if (!strcmp(name, "type") && tag->tagged)
+			v->s = typename(tag->tagged->type);
+		else if (!strcmp(name, "object") && tag->tagged) {
+			char *s = xmalloc(41);
+			strcpy(s, sha1_to_hex(tag->tagged->sha1));
+			v->s = s;
+		}
+	}
+}
+
+/* See grab_values */
+static void grab_commit_values(struct atom_value *val, int deref, struct object *obj, void *buf, unsigned long sz)
+{
+	int i;
+	struct commit *commit = (struct commit *) obj;
+
+	for (i = 0; i < used_atom_cnt; i++) {
+		const char *name = used_atom[i];
+		struct atom_value *v = &val[i];
+		if (!!deref != (*name == '*'))
+			continue;
+		if (deref)
+			name++;
+		if (!strcmp(name, "tree")) {
+			char *s = xmalloc(41);
+			strcpy(s, sha1_to_hex(commit->tree->object.sha1));
+			v->s = s;
+		}
+		if (!strcmp(name, "numparent")) {
+			char *s = xmalloc(40);
+			v->ul = commit_list_count(commit->parents);
+			sprintf(s, "%lu", v->ul);
+			v->s = s;
+		}
+		else if (!strcmp(name, "parent")) {
+			int num = commit_list_count(commit->parents);
+			int i;
+			struct commit_list *parents;
+			char *s = xmalloc(41 * num + 1);
+			v->s = s;
+			for (i = 0, parents = commit->parents;
+			     parents;
+			     parents = parents->next, i = i + 41) {
+				struct commit *parent = parents->item;
+				strcpy(s+i, sha1_to_hex(parent->object.sha1));
+				if (parents->next)
+					s[i+40] = ' ';
+			}
+			if (!i)
+				*s = '\0';
+		}
+	}
+}
+
+static const char *find_wholine(const char *who, int wholen, const char *buf, unsigned long sz)
+{
+	const char *eol;
+	while (*buf) {
+		if (!strncmp(buf, who, wholen) &&
+		    buf[wholen] == ' ')
+			return buf + wholen + 1;
+		eol = strchr(buf, '\n');
+		if (!eol)
+			return "";
+		eol++;
+		if (*eol == '\n')
+			return ""; /* end of header */
+		buf = eol;
+	}
+	return "";
+}
+
+static const char *copy_line(const char *buf)
+{
+	const char *eol = strchrnul(buf, '\n');
+	return xmemdupz(buf, eol - buf);
+}
+
+static const char *copy_name(const char *buf)
+{
+	const char *cp;
+	for (cp = buf; *cp && *cp != '\n'; cp++) {
+		if (!strncmp(cp, " <", 2))
+			return xmemdupz(buf, cp - buf);
+	}
+	return "";
+}
+
+static const char *copy_email(const char *buf)
+{
+	const char *email = strchr(buf, '<');
+	const char *eoemail;
+	if (!email)
+		return "";
+	eoemail = strchr(email, '>');
+	if (!eoemail)
+		return "";
+	return xmemdupz(email, eoemail + 1 - email);
+}
+
+static char *copy_subject(const char *buf, unsigned long len)
+{
+	char *r = xmemdupz(buf, len);
+	int i;
+
+	for (i = 0; i < len; i++)
+		if (r[i] == '\n')
+			r[i] = ' ';
+
+	return r;
+}
+
+static void grab_date(const char *buf, struct atom_value *v, const char *atomname)
+{
+	const char *eoemail = strstr(buf, "> ");
+	char *zone;
+	unsigned long timestamp;
+	long tz;
+	enum date_mode date_mode = DATE_NORMAL;
+	const char *formatp;
+
+	/*
+	 * We got here because atomname ends in "date" or "date<something>";
+	 * it's not possible that <something> is not ":<format>" because
+	 * parse_atom() wouldn't have allowed it, so we can assume that no
+	 * ":" means no format is specified, and use the default.
+	 */
+	formatp = strchr(atomname, ':');
+	if (formatp != NULL) {
+		formatp++;
+		date_mode = parse_date_format(formatp);
+	}
+
+	if (!eoemail)
+		goto bad;
+	timestamp = strtoul(eoemail + 2, &zone, 10);
+	if (timestamp == ULONG_MAX)
+		goto bad;
+	tz = strtol(zone, NULL, 10);
+	if ((tz == LONG_MIN || tz == LONG_MAX) && errno == ERANGE)
+		goto bad;
+	v->s = xstrdup(show_date(timestamp, tz, date_mode));
+	v->ul = timestamp;
+	return;
+ bad:
+	v->s = "";
+	v->ul = 0;
+}
+
+/* See grab_values */
+static void grab_person(const char *who, struct atom_value *val, int deref, struct object *obj, void *buf, unsigned long sz)
+{
+	int i;
+	int wholen = strlen(who);
+	const char *wholine = NULL;
+
+	for (i = 0; i < used_atom_cnt; i++) {
+		const char *name = used_atom[i];
+		struct atom_value *v = &val[i];
+		if (!!deref != (*name == '*'))
+			continue;
+		if (deref)
+			name++;
+		if (strncmp(who, name, wholen))
+			continue;
+		if (name[wholen] != 0 &&
+		    strcmp(name + wholen, "name") &&
+		    strcmp(name + wholen, "email") &&
+		    !starts_with(name + wholen, "date"))
+			continue;
+		if (!wholine)
+			wholine = find_wholine(who, wholen, buf, sz);
+		if (!wholine)
+			return; /* no point looking for it */
+		if (name[wholen] == 0)
+			v->s = copy_line(wholine);
+		else if (!strcmp(name + wholen, "name"))
+			v->s = copy_name(wholine);
+		else if (!strcmp(name + wholen, "email"))
+			v->s = copy_email(wholine);
+		else if (starts_with(name + wholen, "date"))
+			grab_date(wholine, v, name);
+	}
+
+	/*
+	 * For a tag or a commit object, if "creator" or "creatordate" is
+	 * requested, do something special.
+	 */
+	if (strcmp(who, "tagger") && strcmp(who, "committer"))
+		return; /* "author" for commit object is not wanted */
+	if (!wholine)
+		wholine = find_wholine(who, wholen, buf, sz);
+	if (!wholine)
+		return;
+	for (i = 0; i < used_atom_cnt; i++) {
+		const char *name = used_atom[i];
+		struct atom_value *v = &val[i];
+		if (!!deref != (*name == '*'))
+			continue;
+		if (deref)
+			name++;
+
+		if (starts_with(name, "creatordate"))
+			grab_date(wholine, v, name);
+		else if (!strcmp(name, "creator"))
+			v->s = copy_line(wholine);
+	}
+}
+
+static void find_subpos(const char *buf, unsigned long sz,
+			const char **sub, unsigned long *sublen,
+			const char **body, unsigned long *bodylen,
+			unsigned long *nonsiglen,
+			const char **sig, unsigned long *siglen)
+{
+	const char *eol;
+	/* skip past header until we hit empty line */
+	while (*buf && *buf != '\n') {
+		eol = strchrnul(buf, '\n');
+		if (*eol)
+			eol++;
+		buf = eol;
+	}
+	/* skip any empty lines */
+	while (*buf == '\n')
+		buf++;
+
+	/* parse signature first; we might not even have a subject line */
+	*sig = buf + parse_signature(buf, strlen(buf));
+	*siglen = strlen(*sig);
+
+	/* subject is first non-empty line */
+	*sub = buf;
+	/* subject goes to first empty line */
+	while (buf < *sig && *buf && *buf != '\n') {
+		eol = strchrnul(buf, '\n');
+		if (*eol)
+			eol++;
+		buf = eol;
+	}
+	*sublen = buf - *sub;
+	/* drop trailing newline, if present */
+	if (*sublen && (*sub)[*sublen - 1] == '\n')
+		*sublen -= 1;
+
+	/* skip any empty lines */
+	while (*buf == '\n')
+		buf++;
+	*body = buf;
+	*bodylen = strlen(buf);
+	*nonsiglen = *sig - buf;
+}
+
+/* See grab_values */
+static void grab_sub_body_contents(struct atom_value *val, int deref, struct object *obj, void *buf, unsigned long sz)
+{
+	int i;
+	const char *subpos = NULL, *bodypos = NULL, *sigpos = NULL;
+	unsigned long sublen = 0, bodylen = 0, nonsiglen = 0, siglen = 0;
+
+	for (i = 0; i < used_atom_cnt; i++) {
+		const char *name = used_atom[i];
+		struct atom_value *v = &val[i];
+		if (!!deref != (*name == '*'))
+			continue;
+		if (deref)
+			name++;
+		if (strcmp(name, "subject") &&
+		    strcmp(name, "body") &&
+		    strcmp(name, "contents") &&
+		    strcmp(name, "contents:subject") &&
+		    strcmp(name, "contents:body") &&
+		    strcmp(name, "contents:signature"))
+			continue;
+		if (!subpos)
+			find_subpos(buf, sz,
+				    &subpos, &sublen,
+				    &bodypos, &bodylen, &nonsiglen,
+				    &sigpos, &siglen);
+
+		if (!strcmp(name, "subject"))
+			v->s = copy_subject(subpos, sublen);
+		else if (!strcmp(name, "contents:subject"))
+			v->s = copy_subject(subpos, sublen);
+		else if (!strcmp(name, "body"))
+			v->s = xmemdupz(bodypos, bodylen);
+		else if (!strcmp(name, "contents:body"))
+			v->s = xmemdupz(bodypos, nonsiglen);
+		else if (!strcmp(name, "contents:signature"))
+			v->s = xmemdupz(sigpos, siglen);
+		else if (!strcmp(name, "contents"))
+			v->s = xstrdup(subpos);
+	}
+}
+
+/*
+ * We want to have empty print-string for field requests
+ * that do not apply (e.g. "authordate" for a tag object)
+ */
+static void fill_missing_values(struct atom_value *val)
+{
+	int i;
+	for (i = 0; i < used_atom_cnt; i++) {
+		struct atom_value *v = &val[i];
+		if (v->s == NULL)
+			v->s = "";
+	}
+}
+
+/*
+ * val is a list of atom_value to hold returned values.  Extract
+ * the values for atoms in used_atom array out of (obj, buf, sz).
+ * when deref is false, (obj, buf, sz) is the object that is
+ * pointed at by the ref itself; otherwise it is the object the
+ * ref (which is a tag) refers to.
+ */
+static void grab_values(struct atom_value *val, int deref, struct object *obj, void *buf, unsigned long sz)
+{
+	grab_common_values(val, deref, obj, buf, sz);
+	switch (obj->type) {
+	case OBJ_TAG:
+		grab_tag_values(val, deref, obj, buf, sz);
+		grab_sub_body_contents(val, deref, obj, buf, sz);
+		grab_person("tagger", val, deref, obj, buf, sz);
+		break;
+	case OBJ_COMMIT:
+		grab_commit_values(val, deref, obj, buf, sz);
+		grab_sub_body_contents(val, deref, obj, buf, sz);
+		grab_person("author", val, deref, obj, buf, sz);
+		grab_person("committer", val, deref, obj, buf, sz);
+		break;
+	case OBJ_TREE:
+		/* grab_tree_values(val, deref, obj, buf, sz); */
+		break;
+	case OBJ_BLOB:
+		/* grab_blob_values(val, deref, obj, buf, sz); */
+		break;
+	default:
+		die("Eh?  Object of type %d?", obj->type);
+	}
+}
+
+static inline char *copy_advance(char *dst, const char *src)
+{
+	while (*src)
+		*dst++ = *src++;
+	return dst;
+}
+
+/*
+ * Parse the object referred by ref, and grab needed value.
+ */
+static void populate_value(struct ref_filter_item *ref)
+{
+	void *buf;
+	struct object *obj;
+	int eaten, i;
+	unsigned long size;
+	const unsigned char *tagged;
+
+	ref->value = xcalloc(used_atom_cnt, sizeof(struct atom_value));
+
+	if (need_symref && (ref->flags & REF_ISSYMREF) && !ref->symref) {
+		unsigned char unused1[20];
+		ref->symref = resolve_refdup(ref->name, RESOLVE_REF_READING,
+					     unused1, NULL);
+		if (!ref->symref)
+			ref->symref = "";
+	}
+
+	/* Fill in specials first */
+	for (i = 0; i < used_atom_cnt; i++) {
+		const char *name = used_atom[i];
+		struct atom_value *v = &ref->value[i];
+		int deref = 0;
+		const char *refname;
+		const char *formatp;
+		struct branch *branch = NULL;
+
+		if (*name == '*') {
+			deref = 1;
+			name++;
+		}
+
+		if (starts_with(name, "refname"))
+			refname = ref->name;
+		else if (starts_with(name, "symref"))
+			refname = ref->symref ? ref->symref : "";
+		else if (starts_with(name, "upstream")) {
+			/* only local branches may have an upstream */
+			if (!starts_with(ref->name, "refs/heads/"))
+				continue;
+			branch = branch_get(ref->name + 11);
+
+			if (!branch || !branch->merge || !branch->merge[0] ||
+			    !branch->merge[0]->dst)
+				continue;
+			refname = branch->merge[0]->dst;
+		} else if (starts_with(name, "color:")) {
+			char color[COLOR_MAXLEN] = "";
+
+			if (color_parse(name + 6, color) < 0)
+				die(_("unable to parse format"));
+			v->s = xstrdup(color);
+			continue;
+		} else if (!strcmp(name, "flag")) {
+			char buf[256], *cp = buf;
+			if (ref->flags & REF_ISSYMREF)
+				cp = copy_advance(cp, ",symref");
+			if (ref->flags & REF_ISPACKED)
+				cp = copy_advance(cp, ",packed");
+			if (cp == buf)
+				v->s = "";
+			else {
+				*cp = '\0';
+				v->s = xstrdup(buf + 1);
+			}
+			continue;
+		} else if (!deref && grab_objectname(name, ref->sha1, v)) {
+			continue;
+		} else if (!strcmp(name, "HEAD")) {
+			const char *head;
+			unsigned char sha1[20];
+
+			head = resolve_ref_unsafe("HEAD", RESOLVE_REF_READING,
+						  sha1, NULL);
+			if (!strcmp(ref->name, head))
+				v->s = "*";
+			else
+				v->s = " ";
+			continue;
+		} else
+			continue;
+
+		formatp = strchr(name, ':');
+		if (formatp) {
+			int num_ours, num_theirs;
+
+			formatp++;
+			if (!strcmp(formatp, "short"))
+				refname = shorten_unambiguous_ref(refname,
+						      warn_ambiguous_refs);
+			else if (!strcmp(formatp, "track") &&
+				 starts_with(name, "upstream")) {
+				char buf[40];
+
+				if (stat_tracking_info(branch, &num_ours,
+						       &num_theirs) != 1)
+					continue;
+
+				if (!num_ours && !num_theirs)
+					v->s = "";
+				else if (!num_ours) {
+					sprintf(buf, "[behind %d]", num_theirs);
+					v->s = xstrdup(buf);
+				} else if (!num_theirs) {
+					sprintf(buf, "[ahead %d]", num_ours);
+					v->s = xstrdup(buf);
+				} else {
+					sprintf(buf, "[ahead %d, behind %d]",
+						num_ours, num_theirs);
+					v->s = xstrdup(buf);
+				}
+				continue;
+			} else if (!strcmp(formatp, "trackshort") &&
+				   starts_with(name, "upstream")) {
+				assert(branch);
+
+				if (stat_tracking_info(branch, &num_ours,
+							&num_theirs) != 1)
+					continue;
+
+				if (!num_ours && !num_theirs)
+					v->s = "=";
+				else if (!num_ours)
+					v->s = "<";
+				else if (!num_theirs)
+					v->s = ">";
+				else
+					v->s = "<>";
+				continue;
+			} else
+				die("unknown %.*s format %s",
+				    (int)(formatp - name), name, formatp);
+		}
+
+		if (!deref)
+			v->s = refname;
+		else {
+			int len = strlen(refname);
+			char *s = xmalloc(len + 4);
+			sprintf(s, "%s^{}", refname);
+			v->s = s;
+		}
+	}
+
+	for (i = 0; i < used_atom_cnt; i++) {
+		struct atom_value *v = &ref->value[i];
+		if (v->s == NULL)
+			goto need_obj;
+	}
+	return;
+
+ need_obj:
+	buf = get_obj(ref->sha1, &obj, &size, &eaten);
+	if (!buf)
+		die("missing object %s for %s",
+		    sha1_to_hex(ref->sha1), ref->name);
+	if (!obj)
+		die("parse_object_buffer failed on %s for %s",
+		    sha1_to_hex(ref->sha1), ref->name);
+
+	grab_values(ref->value, 0, obj, buf, size);
+	if (!eaten)
+		free(buf);
+
+	/*
+	 * If there is no atom that wants to know about tagged
+	 * object, we are done.
+	 */
+	if (!need_tagged || (obj->type != OBJ_TAG))
+		return;
+
+	/*
+	 * If it is a tag object, see if we use a value that derefs
+	 * the object, and if we do grab the object it refers to.
+	 */
+	tagged = ((struct tag *)obj)->tagged->sha1;
+
+	/*
+	 * NEEDSWORK: This derefs tag only once, which
+	 * is good to deal with chains of trust, but
+	 * is not consistent with what deref_tag() does
+	 * which peels the onion to the core.
+	 */
+	buf = get_obj(tagged, &obj, &size, &eaten);
+	if (!buf)
+		die("missing object %s for %s",
+		    sha1_to_hex(tagged), ref->name);
+	if (!obj)
+		die("parse_object_buffer failed on %s for %s",
+		    sha1_to_hex(tagged), ref->name);
+	grab_values(ref->value, 1, obj, buf, size);
+	if (!eaten)
+		free(buf);
+}
+
+/*
+ * Given a ref, return the value for the atom.  This lazily gets value
+ * out of the object by calling populate value.
+ */
+static void get_value(struct ref_filter_item *ref, int atom, struct atom_value **v)
+{
+	if (!ref->value) {
+		populate_value(ref);
+		fill_missing_values(ref->value);
+	}
+	*v = &ref->value[atom];
+}
+
+static int cmp_ref_sort(struct ref_sort *s, struct ref_filter_item *a, struct ref_filter_item *b)
+{
+	struct atom_value *va, *vb;
+	int cmp;
+	cmp_type cmp_type = used_atom_type[s->atom];
+
+	get_value(a, s->atom, &va);
+	get_value(b, s->atom, &vb);
+	switch (cmp_type) {
+	case FIELD_STR:
+		cmp = strcmp(va->s, vb->s);
+		break;
+	default:
+		if (va->ul < vb->ul)
+			cmp = -1;
+		else if (va->ul == vb->ul)
+			cmp = 0;
+		else
+			cmp = 1;
+		break;
+	}
+	return (s->reverse) ? -cmp : cmp;
+}
+
+static struct ref_sort *ref_sort;
+static int compare_refs(const void *a_, const void *b_)
+{
+	struct ref_filter_item *a = *((struct ref_filter_item **)a_);
+	struct ref_filter_item *b = *((struct ref_filter_item **)b_);
+	struct ref_sort *s;
+
+	for (s = ref_sort; s; s = s->next) {
+		int cmp = cmp_ref_sort(s, a, b);
+		if (cmp)
+			return cmp;
+	}
+	return 0;
+}
+
+void sort_refs(struct ref_sort *sort, struct ref_filter *refs)
+{
+	ref_sort = sort;
+	qsort(refs->items, refs->count, sizeof(struct ref_filter_item *), compare_refs);
+}
+
+static void print_value(struct atom_value *v, int quote_style)
+{
+	struct strbuf sb = STRBUF_INIT;
+	switch (quote_style) {
+	case QUOTE_NONE:
+		fputs(v->s, stdout);
+		break;
+	case QUOTE_SHELL:
+		sq_quote_buf(&sb, v->s);
+		break;
+	case QUOTE_PERL:
+		perl_quote_buf(&sb, v->s);
+		break;
+	case QUOTE_PYTHON:
+		python_quote_buf(&sb, v->s);
+		break;
+	case QUOTE_TCL:
+		tcl_quote_buf(&sb, v->s);
+		break;
+	}
+	if (quote_style != QUOTE_NONE) {
+		fputs(sb.buf, stdout);
+		strbuf_release(&sb);
+	}
+}
+
+static int hex1(char ch)
+{
+	if ('0' <= ch && ch <= '9')
+		return ch - '0';
+	else if ('a' <= ch && ch <= 'f')
+		return ch - 'a' + 10;
+	else if ('A' <= ch && ch <= 'F')
+		return ch - 'A' + 10;
+	return -1;
+}
+static int hex2(const char *cp)
+{
+	if (cp[0] && cp[1])
+		return (hex1(cp[0]) << 4) | hex1(cp[1]);
+	else
+		return -1;
+}
+
+static void emit(const char *cp, const char *ep)
+{
+	while (*cp && (!ep || cp < ep)) {
+		if (*cp == '%') {
+			if (cp[1] == '%')
+				cp++;
+			else {
+				int ch = hex2(cp + 1);
+				if (0 <= ch) {
+					putchar(ch);
+					cp += 3;
+					continue;
+				}
+			}
+		}
+		putchar(*cp);
+		cp++;
+	}
+}
+
+void show_ref(struct ref_filter_item *info, const char *format, int quote_style)
+{
+	const char *cp, *sp, *ep;
+	extern int need_color_reset_at_eol;
+
+	for (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {
+		struct atom_value *atomv;
+
+		ep = strchr(sp, ')');
+		if (cp < sp)
+			emit(cp, sp);
+		get_value(info, parse_atom(sp + 2, ep), &atomv);
+		print_value(atomv, quote_style);
+	}
+	if (*cp) {
+		sp = cp + strlen(cp);
+		emit(cp, sp);
+	}
+	if (need_color_reset_at_eol) {
+		struct atom_value resetv;
+		char color[COLOR_MAXLEN] = "";
+
+		if (color_parse("reset", color) < 0)
+			die("BUG: couldn't parse 'reset' as a color");
+		resetv.s = color;
+		print_value(&resetv, quote_style);
+	}
+	putchar('\n');
+}
+
+struct ref_sort *ref_default_sort(void)
+{
+	static const char cstr_name[] = "refname";
+
+	struct ref_sort *sort = xcalloc(1, sizeof(*sort));
+
+	sort->next = NULL;
+	sort->atom = parse_atom(cstr_name, cstr_name + strlen(cstr_name));
+	return sort;
+}
+
+int ref_opt_parse_sort(const struct option *opt, const char *arg, int unset)
+{
+	struct ref_sort **sort_tail = opt->value;
+	struct ref_sort *s;
+	int len;
+
+	if (!arg) /* should --no-sort void the list ? */
+		return -1;
+
+	s = xcalloc(1, sizeof(*s));
+	s->next = *sort_tail;
+	*sort_tail = s;
+
+	if (*arg == '-') {
+		s->reverse = 1;
+		arg++;
+	}
+	len = strlen(arg);
+	s->atom = parse_atom(arg, arg+len);
+	return 0;
+}
 
 static int match_name_as_path(const char **pattern, const char *refname)
 {
diff --git a/ref-filter.h b/ref-filter.h
index 3010d13..f16978a 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -4,6 +4,7 @@
 #include "sha1-array.h"
 #include "refs.h"
 #include "commit.h"
+#include "parse-options.h"
 
 /*
  * ref-filter is meant to act as a common provider of API's for
@@ -12,6 +13,13 @@
  * the functionality of each other.
  */
 
+/* Quoting styles */
+#define QUOTE_NONE 0
+#define QUOTE_SHELL 1
+#define QUOTE_PERL 2
+#define QUOTE_PYTHON 4
+#define QUOTE_TCL 8
+
 /* An atom is a valid field atom used for sorting and formatting of refs.*/
 struct atom_value {
 	const char *s;
@@ -44,4 +52,12 @@ struct ref_filter {
 int ref_filter_add(const char *refname, const unsigned char *sha1, int flags, void *data);
 void ref_filter_clear(struct ref_filter *refs);
 
+/*  formatting and sorting functions */
+int parse_atom(const char *atom, const char *ep);
+int verify_format(const char *format);
+void sort_refs(struct ref_sort *sort, struct ref_filter *refs);
+void show_ref(struct ref_filter_item *info, const char *format, int quote_style);
+int ref_opt_parse_sort(const struct option *opt, const char *arg, int unset);
+struct ref_sort *ref_default_sort(void);
+
 #endif /*  REF_FILTER_H  */
-- 
2.4.1