# [PATCH v3 03/23] cat-file: reuse struct ref_format

56 messages from 2018-02-12 to 2018-02-19. Participants: Olga Telezhnaya, Jeff King, Оля Тележная, Junio C Hamano, Adam Dinwoodie, Ramsay Jones.
Thread: https://gitlist.dev/t/47816

## Olga Telezhnaya, 2018-02-12 08:08

Subject: [PATCH v3 03/23] cat-file: reuse struct ref_format
Message-ID: <01020161890f432e-07d6260a-0100-4859-af1c-2cdf8da76829-000000@eu-west-1.amazonses.com>
URL: https://gitlist.dev/e/01020161890f432e-07d6260a-0100-4859-af1c-2cdf8da76829-000000%40eu-west-1.amazonses.com
In-Reply-To: <01020161890f4236-47989eb4-c19f-4282-9084-9d4f90c2ebeb-000000@eu-west-1.amazonses.com>

```
Start using ref_format struct instead of simple char*.
Need that for further reusing of formatting logic from ref-filter.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored by: Jeff King <peff@peff.net>
---
 builtin/cat-file.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index f5fa4fd75af26..98fc5ec069a49 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -13,15 +13,16 @@
 #include "tree-walk.h"
 #include "sha1-array.h"
 #include "packfile.h"
+#include "ref-filter.h"
 
 struct batch_options {
+	struct ref_format format;
 	int enabled;
 	int follow_symlinks;
 	int print_contents;
 	int buffer_output;
 	int all_objects;
 	int cmdmode; /* may be 'w' or 'c' for --filters or --textconv */
-	const char *format;
 };
 
 static const char *force_path;
@@ -348,7 +349,7 @@ static void batch_object_write(const char *obj_name, struct batch_options *opt,
 		return;
 	}
 
-	strbuf_expand(&buf, opt->format, expand_format, data);
+	strbuf_expand(&buf, opt->format.format, expand_format, data);
 	strbuf_addch(&buf, '\n');
 	batch_write(opt, buf.buf, buf.len);
 	strbuf_release(&buf);
@@ -441,8 +442,8 @@ static int batch_objects(struct batch_options *opt)
 	int save_warning;
 	int retval = 0;
 
-	if (!opt->format)
-		opt->format = "%(objectname) %(objecttype) %(objectsize)";
+	if (!opt->format.format)
+		opt->format.format = "%(objectname) %(objecttype) %(objectsize)";
 
 	/*
 	 * Expand once with our special mark_query flag, which will prime the
@@ -451,7 +452,7 @@ static int batch_objects(struct batch_options *opt)
 	 */
 	memset(&data, 0, sizeof(data));
 	data.mark_query = 1;
-	strbuf_expand(&buf, opt->format, expand_format, &data);
+	strbuf_expand(&buf, opt->format.format, expand_format, &data);
 	data.mark_query = 0;
 	if (opt->cmdmode)
 		data.split_on_whitespace = 1;
@@ -543,7 +544,7 @@ static int batch_option_callback(const struct option *opt,
 
 	bo->enabled = 1;
 	bo->print_contents = !strcmp(opt->long_name, "batch");
-	bo->format = arg;
+	bo->format.format = arg;
 
 	return 0;
 }
@@ -552,7 +553,7 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
 {
 	int opt = 0;
 	const char *exp_type = NULL, *obj_name = NULL;
-	struct batch_options batch = {0};
+	struct batch_options batch = { REF_FORMAT_INIT };
 	int unknown_type = 0;
 
 	const struct option options[] = {

--
https://github.com/git/git/pull/452

```

## Olga Telezhnaya, 2018-02-12 08:08

Subject: [PATCH v3 08/23] ref-filter: reuse parse_ref_filter_atom()
Message-ID: <01020161890f434b-3f93f5b7-0fad-4777-ab9b-79c48b2b41d2-000000@eu-west-1.amazonses.com>
URL: https://gitlist.dev/e/01020161890f434b-3f93f5b7-0fad-4777-ab9b-79c48b2b41d2-000000%40eu-west-1.amazonses.com
In-Reply-To: <01020161890f4236-47989eb4-c19f-4282-9084-9d4f90c2ebeb-000000@eu-west-1.amazonses.com>

```
Continue migrating formatting logic from cat-file to ref-filter.
Reuse parse_ref_filter_atom() for unifying all processes in ref-filter
and further removing of mark_atom_in_object_info().

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored by: Jeff King <peff@peff.net>
---
 ref-filter.c | 34 +++++++++++++++++++++++++---------
 1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index ff87e632f463c..5c75259b1ab8c 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -100,6 +100,7 @@ static struct used_atom {
 	} u;
 } *used_atom;
 static int used_atom_cnt, need_tagged, need_symref;
+struct expand_data *cat_file_info;
 
 static void color_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *color_value)
 {
@@ -251,6 +252,16 @@ static void objectname_atom_parser(const struct ref_format *format, struct used_
 		die(_("unrecognized %%(objectname) argument: %s"), arg);
 }
 
+static void objectsize_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
+{
+	if (!arg)
+		; /* default to normal object size */
+	else if (!strcmp(arg, "disk"))
+		cat_file_info->info.disk_sizep = &cat_file_info->disk_size;
+	else
+		die(_("urecognized %%(objectsize) argument: %s"), arg);
+}
+
 static void refname_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
 {
 	refname_atom_parser_internal(&atom->u.refname, arg, atom->name);
@@ -371,6 +382,14 @@ static struct valid_atom {
 	{ "else" },
 };
 
+static struct valid_atom valid_cat_file_atom[] = {
+	{ "objectname" },
+	{ "objecttype" },
+	{ "objectsize", FIELD_ULONG, objectsize_atom_parser },
+	{ "rest" },
+	{ "deltabase" },
+};
+
 #define REF_FORMATTING_STATE_INIT  { 0, NULL }
 
 struct ref_formatting_stack {
@@ -401,20 +420,14 @@ static int is_atom(const char *atom, const char *s, int slen)
 static void mark_atom_in_object_info(const char *atom, int len,
 				    struct expand_data *data)
 {
-	if (is_atom("objectname", atom, len))
-		; /* do nothing */
-	else if (is_atom("objecttype", atom, len))
+	if (is_atom("objecttype", atom, len))
 		data->info.typep = &data->type;
 	else if (is_atom("objectsize", atom, len))
 		data->info.sizep = &data->size;
-	else if (is_atom("objectsize:disk", atom, len))
-		data->info.disk_sizep = &data->disk_size;
 	else if (is_atom("rest", atom, len))
 		data->split_on_whitespace = 1;
 	else if (is_atom("deltabase", atom, len))
 		data->info.delta_base_sha1 = data->delta_base_oid.hash;
-	else
-		die("unknown format element: %.*s", len, atom);
 }
 
 /*
@@ -483,6 +496,8 @@ static int parse_ref_filter_atom(const struct ref_format *format,
 		need_tagged = 1;
 	if (!strcmp(valid_atom[i].name, "symref"))
 		need_symref = 1;
+	if (cat_file_info)
+		mark_atom_in_object_info(atom, atom_len, cat_file_info);
 	return at;
 }
 
@@ -726,6 +741,7 @@ int verify_ref_format(struct ref_format *format)
 {
 	const char *cp, *sp;
 
+	cat_file_info = format->cat_file_data;
 	format->need_color_reset_at_eol = 0;
 	for (cp = format->format; *cp && (sp = find_next(cp)); ) {
 		const char *color, *ep = strchr(sp, ')');
@@ -736,8 +752,8 @@ int verify_ref_format(struct ref_format *format)
 		/* sp points at "%(" and ep points at the closing ")" */
 
 		if (format->cat_file_data)
-			mark_atom_in_object_info(sp + 2, ep - sp - 2,
-						format->cat_file_data);
+			at = parse_ref_filter_atom(format, valid_cat_file_atom,
+						   ARRAY_SIZE(valid_cat_file_atom), sp + 2, ep);
 		else {
 			at = parse_ref_filter_atom(format, valid_atom,
 						   ARRAY_SIZE(valid_atom), sp + 2, ep);

--
https://github.com/git/git/pull/452

```

## Olga Telezhnaya, 2018-02-12 08:08

Subject: [PATCH v3 01/23] ref-filter: get rid of goto
Message-ID: <01020161890f4236-47989eb4-c19f-4282-9084-9d4f90c2ebeb-000000@eu-west-1.amazonses.com>
URL: https://gitlist.dev/e/01020161890f4236-47989eb4-c19f-4282-9084-9d4f90c2ebeb-000000%40eu-west-1.amazonses.com
In-Reply-To: <0102016165b8b3c4-54efe4c4-6d19-435d-b5b9-6c727771353b-000000@eu-west-1.amazonses.com>

```
Get rid of goto command in ref-filter for better readability.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored by: Jeff King <peff@peff.net>
---
 ref-filter.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index f9e25aea7a97e..d04295e33448e 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1477,12 +1477,13 @@ static void populate_value(struct ref_array_item *ref)
 
 	for (i = 0; i < used_atom_cnt; i++) {
 		struct atom_value *v = &ref->value[i];
-		if (v->s == NULL)
-			goto need_obj;
+		if (v->s == NULL) {
+			break;
+		}
 	}
-	return;
+	if (used_atom_cnt <= i)
+		return;
 
- need_obj:
 	buf = get_obj(&ref->objectname, &obj, &size, &eaten);
 	if (!buf)
 		die(_("missing object %s for %s"),

--
https://github.com/git/git/pull/452

```

## Olga Telezhnaya, 2018-02-12 08:08

Subject: [PATCH v3 04/23] ref-filter: make valid_atom as function parameter
Message-ID: <01020161890f4324-be5dda9b-9ad7-402f-9196-30f982fd20c2-000000@eu-west-1.amazonses.com>
URL: https://gitlist.dev/e/01020161890f4324-be5dda9b-9ad7-402f-9196-30f982fd20c2-000000%40eu-west-1.amazonses.com
In-Reply-To: <01020161890f4236-47989eb4-c19f-4282-9084-9d4f90c2ebeb-000000@eu-west-1.amazonses.com>

```
Make valid_atom as a function parameter,
there could be another variable further.
Need that for further reusing of formatting logic in cat-file.c.

We do not need to allow users to pass their own valid_atom variable in
global functions like verify_ref_format() because in the end we want to
have same set of valid atoms for all commands. But, as a first step
of migrating, I create further another version of valid_atom
for cat-file.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored by: Jeff King <peff@peff.net>
---
 ref-filter.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 9ed5e66066a7a..5e7ed0f338490 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -325,7 +325,7 @@ static void head_atom_parser(const struct ref_format *format, struct used_atom *
 	atom->u.head = resolve_refdup("HEAD", RESOLVE_REF_READING, NULL, NULL);
 }
 
-static struct {
+static struct valid_atom {
 	const char *name;
 	cmp_type cmp_type;
 	void (*parser)(const struct ref_format *format, struct used_atom *atom, const char *arg);
@@ -396,6 +396,7 @@ struct atom_value {
  * Used to parse format string and sort specifiers
  */
 static int parse_ref_filter_atom(const struct ref_format *format,
+				 const struct valid_atom *valid_atom, int n_atoms,
 				 const char *atom, const char *ep)
 {
 	const char *sp;
@@ -425,13 +426,13 @@ static int parse_ref_filter_atom(const struct ref_format *format,
 	atom_len = (arg ? arg : ep) - sp;
 
 	/* Is the atom a valid one? */
-	for (i = 0; i < ARRAY_SIZE(valid_atom); i++) {
+	for (i = 0; i < n_atoms; i++) {
 		int len = strlen(valid_atom[i].name);
 		if (len == atom_len && !memcmp(valid_atom[i].name, sp, len))
 			break;
 	}
 
-	if (ARRAY_SIZE(valid_atom) <= i)
+	if (n_atoms <= i)
 		die(_("unknown field name: %.*s"), (int)(ep-atom), atom);
 
 	/* Add it in, including the deref prefix */
@@ -708,7 +709,8 @@ int verify_ref_format(struct ref_format *format)
 		if (!ep)
 			return error(_("malformed format string %s"), sp);
 		/* sp points at "%(" and ep points at the closing ")" */
-		at = parse_ref_filter_atom(format, sp + 2, ep);
+		at = parse_ref_filter_atom(format, valid_atom,
+					   ARRAY_SIZE(valid_atom), sp + 2, ep);
 		cp = ep + 1;
 
 		if (skip_prefix(used_atom[at].name, "color:", &color))
@@ -2145,7 +2147,9 @@ int format_ref_array_item(struct ref_array_item *info,
 		if (cp < sp)
 			append_literal(cp, sp, &state);
 		if (get_ref_atom_value(info,
-				       parse_ref_filter_atom(format, sp + 2, ep),
+				       parse_ref_filter_atom(format, valid_atom,
+							     ARRAY_SIZE(valid_atom),
+							     sp + 2, ep),
 				       &atomv))
 			return -1;
 		atomv->handler(atomv, &state);
@@ -2198,7 +2202,8 @@ static int parse_sorting_atom(const char *atom)
 	 */
 	struct ref_format dummy = REF_FORMAT_INIT;
 	const char *end = atom + strlen(atom);
-	return parse_ref_filter_atom(&dummy, atom, end);
+	return parse_ref_filter_atom(&dummy, valid_atom,
+				     ARRAY_SIZE(valid_atom), atom, end);
 }
 
 /*  If no sorting option is given, use refname to sort as default */

--
https://github.com/git/git/pull/452

```

## Olga Telezhnaya, 2018-02-12 08:08

Subject: [PATCH v3 11/23] ref-filter: rename field in ref_array_item stuct
Message-ID: <01020161890f4354-a54075a4-9dba-45df-9e5c-196362b4e300-000000@eu-west-1.amazonses.com>
URL: https://gitlist.dev/e/01020161890f4354-a54075a4-9dba-45df-9e5c-196362b4e300-000000%40eu-west-1.amazonses.com
In-Reply-To: <01020161890f4236-47989eb4-c19f-4282-9084-9d4f90c2ebeb-000000@eu-west-1.amazonses.com>

```
Rename objectname field to oid in struct ref_array_item.
Next commit will add objectname field that will contain
string representation of object id.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored by: Jeff King <peff@peff.net>
---
 builtin/cat-file.c |  4 ++--
 ref-filter.c       | 10 +++++-----
 ref-filter.h       |  2 +-
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 5b7bc34f1ec6d..0c362828ad81e 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -186,7 +186,7 @@ static void expand_atom(struct strbuf *sb, const char *atom, int len,
 			 struct ref_array_item *item)
 {
 	if (is_atom("objectname", atom, len))
-		strbuf_addstr(sb, oid_to_hex(&item->objectname));
+		strbuf_addstr(sb, oid_to_hex(&item->oid));
 	else if (is_atom("objecttype", atom, len))
 		strbuf_addstr(sb, typename(item->type));
 	else if (is_atom("objectsize", atom, len))
@@ -294,7 +294,7 @@ static void batch_object_write(const char *obj_name, struct batch_options *opt,
 		return;
 	}
 
-	item.objectname = data->oid;
+	item.oid = data->oid;
 	item.type = data->type;
 	item.size = data->size;
 	item.disk_size = data->disk_size;
diff --git a/ref-filter.c b/ref-filter.c
index 4acd391b5dfac..d09ec1bde6d54 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1489,7 +1489,7 @@ int populate_value(struct ref_array_item *ref)
 				v->s = xstrdup(buf + 1);
 			}
 			continue;
-		} else if (!deref && grab_objectname(name, ref->objectname.hash, v, atom)) {
+		} else if (!deref && grab_objectname(name, ref->oid.hash, v, atom)) {
 			continue;
 		} else if (!strcmp(name, "HEAD")) {
 			if (atom->u.head && !strcmp(ref->refname, atom->u.head))
@@ -1534,13 +1534,13 @@ int populate_value(struct ref_array_item *ref)
 	if (used_atom_cnt <= i)
 		return 0;
 
-	buf = get_obj(&ref->objectname, &obj, &size, &eaten);
+	buf = get_obj(&ref->oid, &obj, &size, &eaten);
 	if (!buf)
 		die(_("missing object %s for %s"),
-		    oid_to_hex(&ref->objectname), ref->refname);
+		    oid_to_hex(&ref->oid), ref->refname);
 	if (!obj)
 		die(_("parse_object_buffer failed on %s for %s"),
-		    oid_to_hex(&ref->objectname), ref->refname);
+		    oid_to_hex(&ref->oid), ref->refname);
 
 	grab_values(ref->value, 0, obj, buf, size);
 	if (!eaten)
@@ -1890,7 +1890,7 @@ static struct ref_array_item *new_ref_array_item(const char *refname,
 {
 	struct ref_array_item *ref;
 	FLEX_ALLOC_STR(ref, refname, refname);
-	hashcpy(ref->objectname.hash, objectname);
+	hashcpy(ref->oid.hash, objectname);
 	ref->flag = flag;
 
 	return ref;
diff --git a/ref-filter.h b/ref-filter.h
index e16ea2a990119..87b026b8b76d0 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -34,7 +34,7 @@ struct ref_sorting {
 };
 
 struct ref_array_item {
-	struct object_id objectname;
+	struct object_id oid;
 	int flag;
 	unsigned int kind;
 	const char *symref;

--
https://github.com/git/git/pull/452

```

## Olga Telezhnaya, 2018-02-12 08:08

Subject: [PATCH v3 07/23] cat-file: start migrating formatting to ref-filter
Message-ID: <01020161890f4348-10a54126-e31d-448d-93a3-55fd89c248c4-000000@eu-west-1.amazonses.com>
URL: https://gitlist.dev/e/01020161890f4348-10a54126-e31d-448d-93a3-55fd89c248c4-000000%40eu-west-1.amazonses.com
In-Reply-To: <01020161890f4236-47989eb4-c19f-4282-9084-9d4f90c2ebeb-000000@eu-west-1.amazonses.com>

```
Move mark_atom_in_object_info() from cat-file to ref-filter and
start using it in verify_ref_format().
It also means that we start reusing verify_ref_format() in cat-file.

Start from simple moving of mark_atom_in_object_info(),
it would be removed later by integrating all needed processes into
ref-filter logic.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored by: Jeff King <peff@peff.net>
---
 builtin/cat-file.c | 35 +++++------------------------------
 ref-filter.c       | 41 ++++++++++++++++++++++++++++++++++++-----
 ref-filter.h       | 12 ++++++------
 3 files changed, 47 insertions(+), 41 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index edb04a96d9bd3..67e7790d2f319 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -182,25 +182,6 @@ static int is_atom(const char *atom, const char *s, int slen)
 	return alen == slen && !memcmp(atom, s, alen);
 }
 
-static void mark_atom_in_object_info(const char *atom, int len,
-				     struct expand_data *data)
-{
-	if (is_atom("objectname", atom, len))
-		; /* do nothing */
-	else if (is_atom("objecttype", atom, len))
-		data->info.typep = &data->type;
-	else if (is_atom("objectsize", atom, len))
-		data->info.sizep = &data->size;
-	else if (is_atom("objectsize:disk", atom, len))
-		data->info.disk_sizep = &data->disk_size;
-	else if (is_atom("rest", atom, len))
-		data->split_on_whitespace = 1;
-	else if (is_atom("deltabase", atom, len))
-		data->info.delta_base_sha1 = data->delta_base_oid.hash;
-	else
-		die("unknown format element: %.*s", len, atom);
-}
-
 static void expand_atom(struct strbuf *sb, const char *atom, int len,
 			 struct expand_data *data)
 {
@@ -230,11 +211,7 @@ static size_t expand_format(struct strbuf *sb, const char *start, void *vdata)
 	if (!end)
 		die("format element '%s' does not end in ')'", start);
 
-	if (data->mark_query)
-		mark_atom_in_object_info(start + 1, end - start - 1, data);
-	else
-		expand_atom(sb, start + 1, end - start - 1, data);
-
+	expand_atom(sb, start + 1, end - start - 1, data);
 	return end - start + 1;
 }
 
@@ -413,14 +390,12 @@ static int batch_objects(struct batch_options *opt)
 		opt->format.format = "%(objectname) %(objecttype) %(objectsize)";
 
 	/*
-	 * Expand once with our special mark_query flag, which will prime the
-	 * object_info to be handed to sha1_object_info_extended for each
-	 * object.
+	 * Call verify_ref_format to prepare object_info to be handed to
+	 * sha1_object_info_extended for each object.
 	 */
 	memset(&data, 0, sizeof(data));
-	data.mark_query = 1;
-	strbuf_expand(&buf, opt->format.format, expand_format, &data);
-	data.mark_query = 0;
+	opt->format.cat_file_data = &data;
+	verify_ref_format(&opt->format);
 	if (opt->cmdmode)
 		data.split_on_whitespace = 1;
 
diff --git a/ref-filter.c b/ref-filter.c
index 5e7ed0f338490..ff87e632f463c 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -392,6 +392,31 @@ struct atom_value {
 	struct used_atom *atom;
 };
 
+static int is_atom(const char *atom, const char *s, int slen)
+{
+	int alen = strlen(atom);
+	return alen == slen && !memcmp(atom, s, alen);
+}
+
+static void mark_atom_in_object_info(const char *atom, int len,
+				    struct expand_data *data)
+{
+	if (is_atom("objectname", atom, len))
+		; /* do nothing */
+	else if (is_atom("objecttype", atom, len))
+		data->info.typep = &data->type;
+	else if (is_atom("objectsize", atom, len))
+		data->info.sizep = &data->size;
+	else if (is_atom("objectsize:disk", atom, len))
+		data->info.disk_sizep = &data->disk_size;
+	else if (is_atom("rest", atom, len))
+		data->split_on_whitespace = 1;
+	else if (is_atom("deltabase", atom, len))
+		data->info.delta_base_sha1 = data->delta_base_oid.hash;
+	else
+		die("unknown format element: %.*s", len, atom);
+}
+
 /*
  * Used to parse format string and sort specifiers
  */
@@ -709,12 +734,18 @@ int verify_ref_format(struct ref_format *format)
 		if (!ep)
 			return error(_("malformed format string %s"), sp);
 		/* sp points at "%(" and ep points at the closing ")" */
-		at = parse_ref_filter_atom(format, valid_atom,
-					   ARRAY_SIZE(valid_atom), sp + 2, ep);
-		cp = ep + 1;
 
-		if (skip_prefix(used_atom[at].name, "color:", &color))
-			format->need_color_reset_at_eol = !!strcmp(color, "reset");
+		if (format->cat_file_data)
+			mark_atom_in_object_info(sp + 2, ep - sp - 2,
+						format->cat_file_data);
+		else {
+			at = parse_ref_filter_atom(format, valid_atom,
+						   ARRAY_SIZE(valid_atom), sp + 2, ep);
+			if (skip_prefix(used_atom[at].name, "color:", &color))
+				format->need_color_reset_at_eol = !!strcmp(color, "reset");
+		}
+
+		cp = ep + 1;
 	}
 	if (format->need_color_reset_at_eol && !want_color(format->use_color))
 		format->need_color_reset_at_eol = 0;
diff --git a/ref-filter.h b/ref-filter.h
index 17f2ac24d2739..52e07dbe6864a 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -80,12 +80,6 @@ struct expand_data {
 	const char *rest;
 	struct object_id delta_base_oid;
 
-	/*
-	 * If mark_query is true, we do not expand anything, but rather
-	 * just mark the object_info with items we wish to query.
-	 */
-	int mark_query;
-
 	/*
 	 * Whether to split the input on whitespace before feeding it to
 	 * get_sha1; this is decided during the mark_query phase based on
@@ -119,6 +113,12 @@ struct ref_format {
 
 	/* Internal state to ref-filter */
 	int need_color_reset_at_eol;
+
+	/*
+	 * Helps to move all formatting logic from cat-file to ref-filter,
+	 * hopefully would be reduced later.
+	 */
+	struct expand_data *cat_file_data;
 };
 
 #define REF_FORMAT_INIT { NULL, 0, -1 }

--
https://github.com/git/git/pull/452

```

## Olga Telezhnaya, 2018-02-12 08:08

Subject: [PATCH v3 19/23] ref-filter: make populate_value() internal again
Message-ID: <01020161890f4370-4ae9a8d6-4f01-4f56-9fde-c40fd76d69b5-000000@eu-west-1.amazonses.com>
URL: https://gitlist.dev/e/01020161890f4370-4ae9a8d6-4f01-4f56-9fde-c40fd76d69b5-000000%40eu-west-1.amazonses.com
In-Reply-To: <01020161890f4236-47989eb4-c19f-4282-9084-9d4f90c2ebeb-000000@eu-west-1.amazonses.com>

```
Remove populate_value() from header file. We needed that
for interim step, now it could be returned back.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored by: Jeff King <peff@peff.net>
---
 ref-filter.c | 2 +-
 ref-filter.h | 3 ---
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index eb53b0babdb83..8d104b567eb7c 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1428,7 +1428,7 @@ static int check_and_fill_for_cat(struct ref_array_item *ref)
  * Parse the object referred by ref, and grab needed value.
  * Return 0 if everything was successful, -1 otherwise.
  */
-int populate_value(struct ref_array_item *ref)
+static int populate_value(struct ref_array_item *ref)
 {
 	void *buf;
 	struct object *obj;
diff --git a/ref-filter.h b/ref-filter.h
index 4eaf6d0514502..115d00288fdee 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -174,9 +174,6 @@ void setup_ref_filter_porcelain_msg(void);
 void pretty_print_ref(const char *name, const unsigned char *sha1,
 		      const struct ref_format *format);
 
-/* Fill the values of request and prepare all data for final string creation */
-int populate_value(struct ref_array_item *ref);
-
 /* Search for atom in given format. */
 int is_atom_used(const struct ref_format *format, const char *atom);
 

--
https://github.com/git/git/pull/452

```

## Olga Telezhnaya, 2018-02-12 08:08

Subject: [PATCH v3 16/23] ref-filter: make cat_file_info independent
Message-ID: <01020161890f4376-0d1c5615-ae9c-4320-bc2d-fd3bda19bce0-000000@eu-west-1.amazonses.com>
URL: https://gitlist.dev/e/01020161890f4376-0d1c5615-ae9c-4320-bc2d-fd3bda19bce0-000000%40eu-west-1.amazonses.com
In-Reply-To: <01020161890f4236-47989eb4-c19f-4282-9084-9d4f90c2ebeb-000000@eu-west-1.amazonses.com>

```
Remove connection between expand_data variable
in cat-file and in ref-filter.
It will help further to get rid of using expand_data in cat-file.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored by: Jeff King <peff@peff.net>
---
 builtin/cat-file.c |  3 ++-
 ref-filter.c       | 36 +++++++++++++++++++-----------------
 ref-filter.h       |  7 ++-----
 3 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 582679f3dca2c..273b4038e3893 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -292,6 +292,7 @@ static void batch_object_write(const char *obj_name, struct batch_options *opt,
 	if (populate_value(&item))
 		return;
 
+	data->type = item.type;
 	strbuf_expand(&buf, opt->format.format, expand_format, &item);
 	strbuf_addch(&buf, '\n');
 	batch_write(opt, buf.buf, buf.len);
@@ -392,7 +393,7 @@ static int batch_objects(struct batch_options *opt)
 	 * sha1_object_info_extended for each object.
 	 */
 	memset(&data, 0, sizeof(data));
-	opt->format.cat_file_data = &data;
+	opt->format.is_cat_file = 1;
 	opt->format.all_objects = opt->all_objects;
 	verify_ref_format(&opt->format);
 
diff --git a/ref-filter.c b/ref-filter.c
index 104cd6aef0102..cc70bcf2bb8b1 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -100,7 +100,7 @@ static struct used_atom {
 	} u;
 } *used_atom;
 static int used_atom_cnt, need_tagged, need_symref;
-struct expand_data *cat_file_info;
+struct expand_data cat_file_info;
 
 static void color_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *color_value)
 {
@@ -255,9 +255,9 @@ static void objectname_atom_parser(const struct ref_format *format, struct used_
 static void objectsize_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
 {
 	if (!arg)
-		cat_file_info->info.sizep = &cat_file_info->size;
+		cat_file_info.info.sizep = &cat_file_info.size;
 	else if (!strcmp(arg, "disk"))
-		cat_file_info->info.disk_sizep = &cat_file_info->disk_size;
+		cat_file_info.info.disk_sizep = &cat_file_info.disk_size;
 	else
 		die(_("urecognized %%(objectsize) argument: %s"), arg);
 }
@@ -265,7 +265,7 @@ static void objectsize_atom_parser(const struct ref_format *format, struct used_
 static void objecttype_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
 {
 	if (!arg)
-		cat_file_info->info.typep = &cat_file_info->type;
+		cat_file_info.info.typep = &cat_file_info.type;
 	else
 		die(_("urecognized %%(objecttype) argument: %s"), arg);
 }
@@ -273,7 +273,7 @@ static void objecttype_atom_parser(const struct ref_format *format, struct used_
 static void deltabase_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
 {
 	if (!arg)
-		cat_file_info->info.delta_base_sha1 = cat_file_info->delta_base_oid.hash;
+		cat_file_info.info.delta_base_sha1 = cat_file_info.delta_base_oid.hash;
 	else
 		die(_("urecognized %%(deltabase) argument: %s"), arg);
 }
@@ -751,7 +751,7 @@ int verify_ref_format(struct ref_format *format)
 {
 	const char *cp, *sp;
 
-	cat_file_info = format->cat_file_data;
+	cat_file_info.is_cat_file = format->is_cat_file;
 	format->need_color_reset_at_eol = 0;
 	for (cp = format->format; *cp && (sp = find_next(cp)); ) {
 		const char *color, *ep = strchr(sp, ')');
@@ -761,7 +761,7 @@ int verify_ref_format(struct ref_format *format)
 			return error(_("malformed format string %s"), sp);
 		/* sp points at "%(" and ep points at the closing ")" */
 
-		if (format->cat_file_data)
+		if (cat_file_info.is_cat_file)
 			at = parse_ref_filter_atom(format, valid_cat_file_atom,
 						   ARRAY_SIZE(valid_cat_file_atom), sp + 2, ep);
 		else {
@@ -775,10 +775,10 @@ int verify_ref_format(struct ref_format *format)
 	}
 	if (format->need_color_reset_at_eol && !want_color(format->use_color))
 		format->need_color_reset_at_eol = 0;
-	if (cat_file_info && format->all_objects) {
+	if (cat_file_info.is_cat_file && format->all_objects) {
 		struct object_info empty = OBJECT_INFO_INIT;
-		if (!memcmp(&cat_file_info->info, &empty, sizeof(empty)))
-			cat_file_info->skip_object_info = 1;
+		if (!memcmp(&cat_file_info.info, &empty, sizeof(empty)))
+			cat_file_info.skip_object_info = 1;
 	}
 	return 0;
 }
@@ -1420,18 +1420,20 @@ static const char *get_refname(struct used_atom *atom, struct ref_array_item *re
 
 static int check_and_fill_for_cat(struct ref_array_item *ref)
 {
-	if (!cat_file_info->skip_object_info &&
-	    sha1_object_info_extended(ref->oid.hash, &cat_file_info->info,
+	if (!cat_file_info.info.typep)
+		cat_file_info.info.typep = &cat_file_info.type;
+	if (!cat_file_info.skip_object_info &&
+	    sha1_object_info_extended(ref->oid.hash, &cat_file_info.info,
 				      OBJECT_INFO_LOOKUP_REPLACE) < 0) {
 		const char *e = ref->objectname;
 		printf("%s missing\n", e ? e : oid_to_hex(&ref->oid));
 		fflush(stdout);
 		return -1;
 	}
-	ref->type = cat_file_info->type;
-	ref->size = cat_file_info->size;
-	ref->disk_size = cat_file_info->disk_size;
-	ref->delta_base_oid = &cat_file_info->delta_base_oid;
+	ref->type = cat_file_info.type;
+	ref->size = cat_file_info.size;
+	ref->disk_size = cat_file_info.disk_size;
+	ref->delta_base_oid = &cat_file_info.delta_base_oid;
 	return 0;
 }
 
@@ -1456,7 +1458,7 @@ int populate_value(struct ref_array_item *ref)
 			ref->symref = "";
 	}
 
-	if (cat_file_info && check_and_fill_for_cat(ref))
+	if (cat_file_info.is_cat_file && check_and_fill_for_cat(ref))
 		return -1;
 
 	/* Fill in specials first */
diff --git a/ref-filter.h b/ref-filter.h
index b1c668c12428b..4eaf6d0514502 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -99,6 +99,7 @@ struct expand_data {
 	 * optimized out.
 	 */
 	unsigned skip_object_info : 1;
+	unsigned is_cat_file : 1;
 };
 
 struct ref_format {
@@ -113,12 +114,8 @@ struct ref_format {
 	/* Internal state to ref-filter */
 	int need_color_reset_at_eol;
 
-	/*
-	 * Helps to move all formatting logic from cat-file to ref-filter,
-	 * hopefully would be reduced later.
-	 */
-	struct expand_data *cat_file_data;
 	unsigned all_objects : 1;
+	unsigned is_cat_file : 1;
 };
 
 #define REF_FORMAT_INIT { NULL, 0, -1 }

--
https://github.com/git/git/pull/452

```

## Olga Telezhnaya, 2018-02-12 08:08

Subject: [PATCH v3 20/23] ref-filter: unifying formatting of cat-file opts
Message-ID: <01020161890f437a-302b1577-f034-4a24-a3a2-851d1cda0044-000000@eu-west-1.amazonses.com>
URL: https://gitlist.dev/e/01020161890f437a-302b1577-f034-4a24-a3a2-851d1cda0044-000000%40eu-west-1.amazonses.com
In-Reply-To: <01020161890f4236-47989eb4-c19f-4282-9084-9d4f90c2ebeb-000000@eu-west-1.amazonses.com>

```
cat-file options are now filled by general logic.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored by: Jeff King <peff@peff.net>
---
 ref-filter.c | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 8d104b567eb7c..5781416cf9126 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -824,8 +824,12 @@ static void grab_common_values(struct atom_value *val, int deref, struct object
 		else if (!strcmp(name, "objectsize")) {
 			v->value = sz;
 			v->s = xstrfmt("%lu", sz);
-		}
-		else if (deref)
+		} else if (!strcmp(name, "objectsize:disk")) {
+			if (cat_file_info.is_cat_file) {
+				v->value = cat_file_info.disk_size;
+				v->s = xstrfmt("%"PRIuMAX, (uintmax_t)v->value);
+			}
+		} else if (deref)
 			grab_objectname(name, obj->oid.hash, v, &used_atom[i]);
 	}
 }
@@ -1465,21 +1469,7 @@ static int populate_value(struct ref_array_item *ref)
 			name++;
 		}
 
-		if (cat_file_info.is_cat_file) {
-			if (starts_with(name, "objectname"))
-				v->s = oid_to_hex(&ref->oid);
-			else if (starts_with(name, "objecttype"))
-				v->s = typename(ref->type);
-			else if (starts_with(name, "objectsize")) {
-				v->s = xstrfmt("%lu", ref->size);
-			} else if (starts_with(name, "objectsize:disk")) {
-				v->s = xstrfmt("%"PRIuMAX, (uintmax_t)ref->disk_size);
-			} else if (starts_with(name, "rest"))
-				v->s = ref->rest;
-			else if (starts_with(name, "deltabase"))
-				v->s = xstrdup(oid_to_hex(ref->delta_base_oid));
-			continue;
-		} else if (starts_with(name, "refname"))
+		if (starts_with(name, "refname"))
 			refname = get_refname(atom, ref);
 		else if (starts_with(name, "symref"))
 			refname = get_symref(atom, ref);
@@ -1535,6 +1525,15 @@ static int populate_value(struct ref_array_item *ref)
 			else
 				v->s = " ";
 			continue;
+		} else if (starts_with(name, "rest")) {
+			v->s = ref->rest ? ref->rest : "";
+			continue;
+		} else if (starts_with(name, "deltabase")) {
+			if (ref->delta_base_oid)
+				v->s = xstrdup(oid_to_hex(ref->delta_base_oid));
+			else
+				v->s = "";
+			continue;
 		} else if (starts_with(name, "align")) {
 			v->handler = align_atom_handler;
 			continue;

--
https://github.com/git/git/pull/452

```

## Olga Telezhnaya, 2018-02-12 08:08

Subject: [PATCH v3 10/23] ref-filter: make populate_value() global
Message-ID: <01020161890f4356-2cfaddc9-cd7d-4bd7-9da4-df08783ed06b-000000@eu-west-1.amazonses.com>
URL: https://gitlist.dev/e/01020161890f4356-2cfaddc9-cd7d-4bd7-9da4-df08783ed06b-000000%40eu-west-1.amazonses.com
In-Reply-To: <01020161890f4236-47989eb4-c19f-4282-9084-9d4f90c2ebeb-000000@eu-west-1.amazonses.com>

```
Make function global for further using in cat-file.
In the end of patch series this function becomes internal again,
so this is a part of middle step. cat-file would use more general
functions further.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored by: Jeff King <peff@peff.net>
---
 ref-filter.c | 2 +-
 ref-filter.h | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/ref-filter.c b/ref-filter.c
index 5c75259b1ab8c..4acd391b5dfac 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1407,7 +1407,7 @@ static const char *get_refname(struct used_atom *atom, struct ref_array_item *re
  * Parse the object referred by ref, and grab needed value.
  * Return 0 if everything was successful, -1 otherwise.
  */
-static int populate_value(struct ref_array_item *ref)
+int populate_value(struct ref_array_item *ref)
 {
 	void *buf;
 	struct object *obj;
diff --git a/ref-filter.h b/ref-filter.h
index 781921d4e0978..e16ea2a990119 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -182,4 +182,7 @@ void setup_ref_filter_porcelain_msg(void);
 void pretty_print_ref(const char *name, const unsigned char *sha1,
 		      const struct ref_format *format);
 
+/* Fill the values of request and prepare all data for final string creation */
+int populate_value(struct ref_array_item *ref);
+
 #endif /*  REF_FILTER_H  */

--
https://github.com/git/git/pull/452

```

## Olga Telezhnaya, 2018-02-12 08:08

Subject: [PATCH v3 15/23] cat-file: move skip_object_info into ref-filter
Message-ID: <01020161890f435c-80fa0ef3-afaf-45b7-84a7-28e2bb66f320-000000@eu-west-1.amazonses.com>
URL: https://gitlist.dev/e/01020161890f435c-80fa0ef3-afaf-45b7-84a7-28e2bb66f320-000000%40eu-west-1.amazonses.com
In-Reply-To: <01020161890f4236-47989eb4-c19f-4282-9084-9d4f90c2ebeb-000000@eu-west-1.amazonses.com>

```
Move logic related to skip_object_info into ref-filter,
so that cat-file does not use that field at all.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored by: Jeff King <peff@peff.net>
---
 builtin/cat-file.c | 7 +------
 ref-filter.c       | 5 +++++
 ref-filter.h       | 1 +
 3 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 3a49b55a1cc2e..582679f3dca2c 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -393,14 +393,9 @@ static int batch_objects(struct batch_options *opt)
 	 */
 	memset(&data, 0, sizeof(data));
 	opt->format.cat_file_data = &data;
+	opt->format.all_objects = opt->all_objects;
 	verify_ref_format(&opt->format);
 
-	if (opt->all_objects) {
-		struct object_info empty = OBJECT_INFO_INIT;
-		if (!memcmp(&data.info, &empty, sizeof(empty)))
-			data.skip_object_info = 1;
-	}
-
 	/*
 	 * If we are printing out the object, then always fill in the type,
 	 * since we will want to decide whether or not to stream.
diff --git a/ref-filter.c b/ref-filter.c
index 4adeea6aad0da..104cd6aef0102 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -775,6 +775,11 @@ int verify_ref_format(struct ref_format *format)
 	}
 	if (format->need_color_reset_at_eol && !want_color(format->use_color))
 		format->need_color_reset_at_eol = 0;
+	if (cat_file_info && format->all_objects) {
+		struct object_info empty = OBJECT_INFO_INIT;
+		if (!memcmp(&cat_file_info->info, &empty, sizeof(empty)))
+			cat_file_info->skip_object_info = 1;
+	}
 	return 0;
 }
 
diff --git a/ref-filter.h b/ref-filter.h
index fffc443726e28..b1c668c12428b 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -118,6 +118,7 @@ struct ref_format {
 	 * hopefully would be reduced later.
 	 */
 	struct expand_data *cat_file_data;
+	unsigned all_objects : 1;
 };
 
 #define REF_FORMAT_INIT { NULL, 0, -1 }

--
https://github.com/git/git/pull/452

```

## Olga Telezhnaya, 2018-02-12 08:08

Subject: [PATCH v3 21/23] for-each-ref: tests for new atoms added
Message-ID: <01020161890f438c-9d30a51c-129c-4721-a85a-a3d66a2270b6-000000@eu-west-1.amazonses.com>
URL: https://gitlist.dev/e/01020161890f438c-9d30a51c-129c-4721-a85a-a3d66a2270b6-000000%40eu-west-1.amazonses.com
In-Reply-To: <01020161890f4236-47989eb4-c19f-4282-9084-9d4f90c2ebeb-000000@eu-west-1.amazonses.com>

```
Add tests for new formatting atoms: rest, deltabase, objectsize:disk.
rest means nothing and we expand it into empty string.
We need this atom for cat-file command.
Have plans to support deltabase and objectsize:disk further
(as it is done in cat-file), now also expand it to empty string.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored by: Jeff King <peff@peff.net>
---
 t/t6300-for-each-ref.sh | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index c128dfc579079..eee656a6abba9 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -316,6 +316,24 @@ test_expect_success 'exercise strftime with odd fields' '
 	test_cmp expected actual
 '
 
+test_expect_success 'Check format %(objectsize:disk) gives empty output ' '
+	echo >expected &&
+	git for-each-ref --format="%(objectsize:disk)" refs/heads >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'Check format %(rest) gives empty output ' '
+	echo >expected &&
+	git for-each-ref --format="%(rest)" refs/heads >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'Check format %(deltabase) gives empty output ' '
+	echo >expected &&
+	git for-each-ref --format="%(deltabase)" refs/heads >actual &&
+	test_cmp expected actual
+'
+
 cat >expected <<\EOF
 refs/heads/master
 refs/remotes/origin/master

--
https://github.com/git/git/pull/452

```

## Olga Telezhnaya, 2018-02-12 08:08

Subject: [PATCH v3 02/23] ref-filter: add return value to some functions
Message-ID: <01020161890f4311-ab635cac-1d48-47ee-90e1-f178f134db1c-000000@eu-west-1.amazonses.com>
URL: https://gitlist.dev/e/01020161890f4311-ab635cac-1d48-47ee-90e1-f178f134db1c-000000%40eu-west-1.amazonses.com
In-Reply-To: <01020161890f4236-47989eb4-c19f-4282-9084-9d4f90c2ebeb-000000@eu-west-1.amazonses.com>

```
Add return flag to format_ref_array_item(), show_ref_array_item(),
get_ref_array_info() and populate_value() for further using.
Need it to handle situations when item is broken but we can not invoke
die() because we are in batch mode and all items need to be processed.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored by: Jeff King <peff@peff.net>
---
 ref-filter.c | 37 ++++++++++++++++++++++++-------------
 ref-filter.h | 14 ++++++++++----
 2 files changed, 34 insertions(+), 17 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index d04295e33448e..9ed5e66066a7a 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1356,8 +1356,9 @@ static const char *get_refname(struct used_atom *atom, struct ref_array_item *re
 
 /*
  * Parse the object referred by ref, and grab needed value.
+ * Return 0 if everything was successful, -1 otherwise.
  */
-static void populate_value(struct ref_array_item *ref)
+static int populate_value(struct ref_array_item *ref)
 {
 	void *buf;
 	struct object *obj;
@@ -1482,7 +1483,7 @@ static void populate_value(struct ref_array_item *ref)
 		}
 	}
 	if (used_atom_cnt <= i)
-		return;
+		return 0;
 
 	buf = get_obj(&ref->objectname, &obj, &size, &eaten);
 	if (!buf)
@@ -1501,7 +1502,7 @@ static void populate_value(struct ref_array_item *ref)
 	 * object, we are done.
 	 */
 	if (!need_tagged || (obj->type != OBJ_TAG))
-		return;
+		return 0;
 
 	/*
 	 * If it is a tag object, see if we use a value that derefs
@@ -1525,19 +1526,24 @@ static void populate_value(struct ref_array_item *ref)
 	grab_values(ref->value, 1, obj, buf, size);
 	if (!eaten)
 		free(buf);
+
+	return 0;
 }
 
 /*
  * Given a ref, return the value for the atom.  This lazily gets value
  * out of the object by calling populate value.
+ * Return 0 if everything was successful, -1 otherwise.
  */
-static void get_ref_atom_value(struct ref_array_item *ref, int atom, struct atom_value **v)
+static int get_ref_atom_value(struct ref_array_item *ref, int atom, struct atom_value **v)
 {
+	int retval = 0;
 	if (!ref->value) {
-		populate_value(ref);
+		retval = populate_value(ref);
 		fill_missing_values(ref->value);
 	}
 	*v = &ref->value[atom];
+	return retval;
 }
 
 /*
@@ -2122,7 +2128,7 @@ static void append_literal(const char *cp, const char *ep, struct ref_formatting
 	}
 }
 
-void format_ref_array_item(struct ref_array_item *info,
+int format_ref_array_item(struct ref_array_item *info,
 			   const struct ref_format *format,
 			   struct strbuf *final_buf)
 {
@@ -2138,9 +2144,10 @@ void format_ref_array_item(struct ref_array_item *info,
 		ep = strchr(sp, ')');
 		if (cp < sp)
 			append_literal(cp, sp, &state);
-		get_ref_atom_value(info,
-				   parse_ref_filter_atom(format, sp + 2, ep),
-				   &atomv);
+		if (get_ref_atom_value(info,
+				       parse_ref_filter_atom(format, sp + 2, ep),
+				       &atomv))
+			return -1;
 		atomv->handler(atomv, &state);
 	}
 	if (*cp) {
@@ -2156,17 +2163,21 @@ void format_ref_array_item(struct ref_array_item *info,
 		die(_("format: %%(end) atom missing"));
 	strbuf_addbuf(final_buf, &state.stack->output);
 	pop_stack_element(&state.stack);
+	return 0;
 }
 
-void show_ref_array_item(struct ref_array_item *info,
+int show_ref_array_item(struct ref_array_item *info,
 			 const struct ref_format *format)
 {
 	struct strbuf final_buf = STRBUF_INIT;
+	int retval = format_ref_array_item(info, format, &final_buf);
 
-	format_ref_array_item(info, format, &final_buf);
-	fwrite(final_buf.buf, 1, final_buf.len, stdout);
+	if (!retval) {
+		fwrite(final_buf.buf, 1, final_buf.len, stdout);
+		putchar('\n');
+	}
 	strbuf_release(&final_buf);
-	putchar('\n');
+	return retval;
 }
 
 void pretty_print_ref(const char *name, const unsigned char *sha1,
diff --git a/ref-filter.h b/ref-filter.h
index 0d98342b34319..b75c8ac45248e 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -109,12 +109,18 @@ void ref_array_clear(struct ref_array *array);
 int verify_ref_format(struct ref_format *format);
 /*  Sort the given ref_array as per the ref_sorting provided */
 void ref_array_sort(struct ref_sorting *sort, struct ref_array *array);
-/*  Based on the given format and quote_style, fill the strbuf */
-void format_ref_array_item(struct ref_array_item *info,
+/*
+ * Based on the given format and quote_style, fill the strbuf.
+ * Return 0 if everything was successful, -1 otherwise (and strbuf remains empty)
+ */
+int format_ref_array_item(struct ref_array_item *info,
 			   const struct ref_format *format,
 			   struct strbuf *final_buf);
-/*  Print the ref using the given format and quote_style */
-void show_ref_array_item(struct ref_array_item *info, const struct ref_format *format);
+/*
+ * Print the ref using the given format and quote_style.
+ * Return 0 if everything was successful, -1 otherwise.
+ */
+int show_ref_array_item(struct ref_array_item *info, const struct ref_format *format);
 /*  Parse a single sort specifier and add it to the list */
 void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *atom);
 /*  Callback function for parsing the sort option */

--
https://github.com/git/git/pull/452

```

## Olga Telezhnaya, 2018-02-12 08:08

Subject: [PATCH v3 09/23] cat-file: start use ref_array_item struct
Message-ID: <01020161890f435d-05435bc4-30da-41c4-8bb2-37dcccb4a413-000000@eu-west-1.amazonses.com>
URL: https://gitlist.dev/e/01020161890f435d-05435bc4-30da-41c4-8bb2-37dcccb4a413-000000%40eu-west-1.amazonses.com
In-Reply-To: <01020161890f4236-47989eb4-c19f-4282-9084-9d4f90c2ebeb-000000@eu-west-1.amazonses.com>

```
Moving from using expand_data to ref_array_item structure.
That helps us to reuse functions from ref-filter easier.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored by: Jeff King <peff@peff.net>
---
 builtin/cat-file.c | 32 ++++++++++++++++++++------------
 ref-filter.h       |  5 +++++
 2 files changed, 25 insertions(+), 12 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 67e7790d2f319..5b7bc34f1ec6d 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -183,27 +183,27 @@ static int is_atom(const char *atom, const char *s, int slen)
 }
 
 static void expand_atom(struct strbuf *sb, const char *atom, int len,
-			 struct expand_data *data)
+			 struct ref_array_item *item)
 {
 	if (is_atom("objectname", atom, len))
-		strbuf_addstr(sb, oid_to_hex(&data->oid));
+		strbuf_addstr(sb, oid_to_hex(&item->objectname));
 	else if (is_atom("objecttype", atom, len))
-		strbuf_addstr(sb, typename(data->type));
+		strbuf_addstr(sb, typename(item->type));
 	else if (is_atom("objectsize", atom, len))
-		strbuf_addf(sb, "%lu", data->size);
+		strbuf_addf(sb, "%lu", item->size);
 	else if (is_atom("objectsize:disk", atom, len))
-		strbuf_addf(sb, "%"PRIuMAX, (uintmax_t)data->disk_size);
+		strbuf_addf(sb, "%"PRIuMAX, (uintmax_t)item->disk_size);
 	else if (is_atom("rest", atom, len)) {
-		if (data->rest)
-			strbuf_addstr(sb, data->rest);
+		if (item->rest)
+			strbuf_addstr(sb, item->rest);
 	} else if (is_atom("deltabase", atom, len))
-		strbuf_addstr(sb, oid_to_hex(&data->delta_base_oid));
+		strbuf_addstr(sb, oid_to_hex(item->delta_base_oid));
 }
 
-static size_t expand_format(struct strbuf *sb, const char *start, void *vdata)
+static size_t expand_format(struct strbuf *sb, const char *start, void *data)
 {
 	const char *end;
-	struct expand_data *data = vdata;
+	struct ref_array_item *item = data;
 
 	if (*start != '(')
 		return 0;
@@ -211,7 +211,7 @@ static size_t expand_format(struct strbuf *sb, const char *start, void *vdata)
 	if (!end)
 		die("format element '%s' does not end in ')'", start);
 
-	expand_atom(sb, start + 1, end - start - 1, data);
+	expand_atom(sb, start + 1, end - start - 1, item);
 	return end - start + 1;
 }
 
@@ -283,6 +283,7 @@ static void batch_object_write(const char *obj_name, struct batch_options *opt,
 			       struct expand_data *data)
 {
 	struct strbuf buf = STRBUF_INIT;
+	struct ref_array_item item = {0};
 
 	if (!data->skip_object_info &&
 	    sha1_object_info_extended(data->oid.hash, &data->info,
@@ -293,7 +294,14 @@ static void batch_object_write(const char *obj_name, struct batch_options *opt,
 		return;
 	}
 
-	strbuf_expand(&buf, opt->format.format, expand_format, data);
+	item.objectname = data->oid;
+	item.type = data->type;
+	item.size = data->size;
+	item.disk_size = data->disk_size;
+	item.rest = data->rest;
+	item.delta_base_oid = &data->delta_base_oid;
+
+	strbuf_expand(&buf, opt->format.format, expand_format, &item);
 	strbuf_addch(&buf, '\n');
 	batch_write(opt, buf.buf, buf.len);
 	strbuf_release(&buf);
diff --git a/ref-filter.h b/ref-filter.h
index 52e07dbe6864a..781921d4e0978 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -40,6 +40,11 @@ struct ref_array_item {
 	const char *symref;
 	struct commit *commit;
 	struct atom_value *value;
+	enum object_type type;
+	unsigned long size;
+	off_t disk_size;
+	const char *rest;
+	struct object_id *delta_base_oid;
 	char refname[FLEX_ARRAY];
 };
 

--
https://github.com/git/git/pull/452

```

## Olga Telezhnaya, 2018-02-12 08:08

Subject: [PATCH v3 18/23] cat-file: reuse printing logic from ref-filter
Message-ID: <01020161890f4373-5a54682e-c1b8-4408-9476-bea5551b992c-000000@eu-west-1.amazonses.com>
URL: https://gitlist.dev/e/01020161890f4373-5a54682e-c1b8-4408-9476-bea5551b992c-000000%40eu-west-1.amazonses.com
In-Reply-To: <01020161890f4236-47989eb4-c19f-4282-9084-9d4f90c2ebeb-000000@eu-west-1.amazonses.com>

```
Reuse code from ref-filter to print resulting message.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored by: Jeff King <peff@peff.net>
---
 builtin/cat-file.c | 51 ++++-----------------------------------------------
 ref-filter.c       | 21 +++++++++++++++++++--
 2 files changed, 23 insertions(+), 49 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 273b4038e3893..21007995c5ac6 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -176,45 +176,6 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
 	return 0;
 }
 
-static int is_atom(const char *atom, const char *s, int slen)
-{
-	int alen = strlen(atom);
-	return alen == slen && !memcmp(atom, s, alen);
-}
-
-static void expand_atom(struct strbuf *sb, const char *atom, int len,
-			 struct ref_array_item *item)
-{
-	if (is_atom("objectname", atom, len))
-		strbuf_addstr(sb, oid_to_hex(&item->oid));
-	else if (is_atom("objecttype", atom, len))
-		strbuf_addstr(sb, typename(item->type));
-	else if (is_atom("objectsize", atom, len))
-		strbuf_addf(sb, "%lu", item->size);
-	else if (is_atom("objectsize:disk", atom, len))
-		strbuf_addf(sb, "%"PRIuMAX, (uintmax_t)item->disk_size);
-	else if (is_atom("rest", atom, len)) {
-		if (item->rest)
-			strbuf_addstr(sb, item->rest);
-	} else if (is_atom("deltabase", atom, len))
-		strbuf_addstr(sb, oid_to_hex(item->delta_base_oid));
-}
-
-static size_t expand_format(struct strbuf *sb, const char *start, void *data)
-{
-	const char *end;
-	struct ref_array_item *item = data;
-
-	if (*start != '(')
-		return 0;
-	end = strchr(start + 1, ')');
-	if (!end)
-		die("format element '%s' does not end in ')'", start);
-
-	expand_atom(sb, start + 1, end - start - 1, item);
-	return end - start + 1;
-}
-
 static void batch_write(struct batch_options *opt, const void *data, int len)
 {
 	if (opt->buffer_output) {
@@ -282,23 +243,19 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d
 static void batch_object_write(const char *obj_name, struct batch_options *opt,
 			       struct expand_data *data)
 {
-	struct strbuf buf = STRBUF_INIT;
 	struct ref_array_item item = {0};
 
 	item.oid = data->oid;
 	item.rest = data->rest;
 	item.objectname = obj_name;
 
-	if (populate_value(&item))
+	if (show_ref_array_item(&item, &opt->format))
 		return;
-
-	data->type = item.type;
-	strbuf_expand(&buf, opt->format.format, expand_format, &item);
-	strbuf_addch(&buf, '\n');
-	batch_write(opt, buf.buf, buf.len);
-	strbuf_release(&buf);
+	if (!opt->buffer_output)
+		fflush(stdout);
 
 	if (opt->print_contents) {
+		data->type = item.type;
 		print_object_or_die(opt, data);
 		batch_write(opt, "\n", 1);
 	}
diff --git a/ref-filter.c b/ref-filter.c
index ee311d51ff81c..eb53b0babdb83 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1465,7 +1465,21 @@ int populate_value(struct ref_array_item *ref)
 			name++;
 		}
 
-		if (starts_with(name, "refname"))
+		if (cat_file_info.is_cat_file) {
+			if (starts_with(name, "objectname"))
+				v->s = oid_to_hex(&ref->oid);
+			else if (starts_with(name, "objecttype"))
+				v->s = typename(ref->type);
+			else if (starts_with(name, "objectsize")) {
+				v->s = xstrfmt("%lu", ref->size);
+			} else if (starts_with(name, "objectsize:disk")) {
+				v->s = xstrfmt("%"PRIuMAX, (uintmax_t)ref->disk_size);
+			} else if (starts_with(name, "rest"))
+				v->s = ref->rest;
+			else if (starts_with(name, "deltabase"))
+				v->s = xstrdup(oid_to_hex(ref->delta_base_oid));
+			continue;
+		} else if (starts_with(name, "refname"))
 			refname = get_refname(atom, ref);
 		else if (starts_with(name, "symref"))
 			refname = get_symref(atom, ref);
@@ -2207,6 +2221,7 @@ int format_ref_array_item(struct ref_array_item *info,
 {
 	const char *cp, *sp, *ep;
 	struct ref_formatting_state state = REF_FORMATTING_STATE_INIT;
+	int retval = 0;
 
 	state.quote_style = format->quote_style;
 	push_stack_element(&state.stack);
@@ -2223,6 +2238,8 @@ int format_ref_array_item(struct ref_array_item *info,
 			return -1;
 		atomv->handler(atomv, &state);
 	}
+	if (cat_file_info.is_cat_file && strlen(format->format) == 0)
+		retval = check_and_fill_for_cat(info);
 	if (*cp) {
 		sp = cp + strlen(cp);
 		append_literal(cp, sp, &state);
@@ -2236,7 +2253,7 @@ int format_ref_array_item(struct ref_array_item *info,
 		die(_("format: %%(end) atom missing"));
 	strbuf_addbuf(final_buf, &state.stack->output);
 	pop_stack_element(&state.stack);
-	return 0;
+	return retval;
 }
 
 int show_ref_array_item(struct ref_array_item *info,

--
https://github.com/git/git/pull/452

```

## Olga Telezhnaya, 2018-02-12 08:08

Subject: [PATCH v3 05/23] cat-file: move struct expand_data into ref-filter
Message-ID: <01020161890f4325-7ab9a01a-ab67-4beb-9721-3dcc5095720a-000000@eu-west-1.amazonses.com>
URL: https://gitlist.dev/e/01020161890f4325-7ab9a01a-ab67-4beb-9721-3dcc5095720a-000000%40eu-west-1.amazonses.com
In-Reply-To: <01020161890f4236-47989eb4-c19f-4282-9084-9d4f90c2ebeb-000000@eu-west-1.amazonses.com>

```
Need that for further reusing of formatting logic in cat-file.
Have plans to get rid of using expand_data in cat-file at all,
and use it only in ref-filter for collecting, formatting and printing
needed data.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored by: Jeff King <peff@peff.net>
---
 builtin/cat-file.c | 36 ------------------------------------
 ref-filter.h       | 36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+), 36 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 98fc5ec069a49..37d6096d201b5 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -176,42 +176,6 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
 	return 0;
 }
 
-struct expand_data {
-	struct object_id oid;
-	enum object_type type;
-	unsigned long size;
-	off_t disk_size;
-	const char *rest;
-	struct object_id delta_base_oid;
-
-	/*
-	 * If mark_query is true, we do not expand anything, but rather
-	 * just mark the object_info with items we wish to query.
-	 */
-	int mark_query;
-
-	/*
-	 * Whether to split the input on whitespace before feeding it to
-	 * get_sha1; this is decided during the mark_query phase based on
-	 * whether we have a %(rest) token in our format.
-	 */
-	int split_on_whitespace;
-
-	/*
-	 * After a mark_query run, this object_info is set up to be
-	 * passed to sha1_object_info_extended. It will point to the data
-	 * elements above, so you can retrieve the response from there.
-	 */
-	struct object_info info;
-
-	/*
-	 * This flag will be true if the requested batch format and options
-	 * don't require us to call sha1_object_info, which can then be
-	 * optimized out.
-	 */
-	unsigned skip_object_info : 1;
-};
-
 static int is_atom(const char *atom, const char *s, int slen)
 {
 	int alen = strlen(atom);
diff --git a/ref-filter.h b/ref-filter.h
index b75c8ac45248e..17f2ac24d2739 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -72,6 +72,42 @@ struct ref_filter {
 		verbose;
 };
 
+struct expand_data {
+	struct object_id oid;
+	enum object_type type;
+	unsigned long size;
+	off_t disk_size;
+	const char *rest;
+	struct object_id delta_base_oid;
+
+	/*
+	 * If mark_query is true, we do not expand anything, but rather
+	 * just mark the object_info with items we wish to query.
+	 */
+	int mark_query;
+
+	/*
+	 * Whether to split the input on whitespace before feeding it to
+	 * get_sha1; this is decided during the mark_query phase based on
+	 * whether we have a %(rest) token in our format.
+	 */
+	int split_on_whitespace;
+
+	/*
+	 * After a mark_query run, this object_info is set up to be
+	 * passed to sha1_object_info_extended. It will point to the data
+	 * elements above, so you can retrieve the response from there.
+	 */
+	struct object_info info;
+
+	/*
+	 * This flag will be true if the requested batch format and options
+	 * don't require us to call sha1_object_info, which can then be
+	 * optimized out.
+	 */
+	unsigned skip_object_info : 1;
+};
+
 struct ref_format {
 	/*
 	 * Set these to define the format; make sure you call

--
https://github.com/git/git/pull/452

```

## Olga Telezhnaya, 2018-02-12 08:08

Subject: [PATCH v3 23/23] cat-file: update of docs
Message-ID: <01020161890f4391-9f76231a-4768-461b-80b9-02cada27a943-000000@eu-west-1.amazonses.com>
URL: https://gitlist.dev/e/01020161890f4391-9f76231a-4768-461b-80b9-02cada27a943-000000%40eu-west-1.amazonses.com
In-Reply-To: <01020161890f4236-47989eb4-c19f-4282-9084-9d4f90c2ebeb-000000@eu-west-1.amazonses.com>

```
Update the docs for cat-file command. Some new formatting atoms added
because of reusing ref-filter code.
We do not support cat-file atoms in general formatting logic
(there is just the support for cat-file), that is why some of the atoms
are still explained in cat-file docs.
We need to move these explanations when atoms will be supported
by other commands.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored by: Jeff King <peff@peff.net>
---
 Documentation/git-cat-file.txt | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt
index f90f09b03fae5..90639ac21d0e8 100644
--- a/Documentation/git-cat-file.txt
+++ b/Documentation/git-cat-file.txt
@@ -187,17 +187,8 @@ linkgit:git-rev-parse[1].
 You can specify the information shown for each object by using a custom
 `<format>`. The `<format>` is copied literally to stdout for each
 object, with placeholders of the form `%(atom)` expanded, followed by a
-newline. The available atoms are:
-
-`objectname`::
-	The 40-hex object name of the object.
-
-`objecttype`::
-	The type of the object (the same as `cat-file -t` reports).
-
-`objectsize`::
-	The size, in bytes, of the object (the same as `cat-file -s`
-	reports).
+newline. The available atoms are the same as that of
+linkgit:git-for-each-ref[1], but there are some additional ones:
 
 `objectsize:disk`::
 	The size, in bytes, that the object takes up on disk. See the

--
https://github.com/git/git/pull/452

```

## Olga Telezhnaya, 2018-02-12 08:08

Subject: [PATCH v3 12/23] cat-file: start reusing populate_value()
Message-ID: <01020161890f435c-c3044984-d9aa-4aaf-90e3-1affa109b3b3-000000@eu-west-1.amazonses.com>
URL: https://gitlist.dev/e/01020161890f435c-c3044984-d9aa-4aaf-90e3-1affa109b3b3-000000%40eu-west-1.amazonses.com
In-Reply-To: <01020161890f4236-47989eb4-c19f-4282-9084-9d4f90c2ebeb-000000@eu-west-1.amazonses.com>

```
Move logic related to getting object info from cat-file to ref-filter.
It will help to reuse whole formatting logic from ref-filter further.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored by: Jeff King <peff@peff.net>
---
 builtin/cat-file.c | 17 ++++-------------
 ref-filter.c       | 20 ++++++++++++++++++++
 ref-filter.h       |  1 +
 3 files changed, 25 insertions(+), 13 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 0c362828ad81e..6db57e3533806 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -285,21 +285,12 @@ static void batch_object_write(const char *obj_name, struct batch_options *opt,
 	struct strbuf buf = STRBUF_INIT;
 	struct ref_array_item item = {0};
 
-	if (!data->skip_object_info &&
-	    sha1_object_info_extended(data->oid.hash, &data->info,
-				      OBJECT_INFO_LOOKUP_REPLACE) < 0) {
-		printf("%s missing\n",
-		       obj_name ? obj_name : oid_to_hex(&data->oid));
-		fflush(stdout);
-		return;
-	}
-
 	item.oid = data->oid;
-	item.type = data->type;
-	item.size = data->size;
-	item.disk_size = data->disk_size;
 	item.rest = data->rest;
-	item.delta_base_oid = &data->delta_base_oid;
+	item.objectname = obj_name;
+
+	if (populate_value(&item))
+		return;
 
 	strbuf_expand(&buf, opt->format.format, expand_format, &item);
 	strbuf_addch(&buf, '\n');
diff --git a/ref-filter.c b/ref-filter.c
index d09ec1bde6d54..3f92a27d98b6c 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1403,6 +1403,23 @@ static const char *get_refname(struct used_atom *atom, struct ref_array_item *re
 	return show_ref(&atom->u.refname, ref->refname);
 }
 
+static int check_and_fill_for_cat(struct ref_array_item *ref)
+{
+	if (!cat_file_info->skip_object_info &&
+	    sha1_object_info_extended(ref->oid.hash, &cat_file_info->info,
+				      OBJECT_INFO_LOOKUP_REPLACE) < 0) {
+		const char *e = ref->objectname;
+		printf("%s missing\n", e ? e : oid_to_hex(&ref->oid));
+		fflush(stdout);
+		return -1;
+	}
+	ref->type = cat_file_info->type;
+	ref->size = cat_file_info->size;
+	ref->disk_size = cat_file_info->disk_size;
+	ref->delta_base_oid = &cat_file_info->delta_base_oid;
+	return 0;
+}
+
 /*
  * Parse the object referred by ref, and grab needed value.
  * Return 0 if everything was successful, -1 otherwise.
@@ -1424,6 +1441,9 @@ int populate_value(struct ref_array_item *ref)
 			ref->symref = "";
 	}
 
+	if (cat_file_info && check_and_fill_for_cat(ref))
+		return -1;
+
 	/* Fill in specials first */
 	for (i = 0; i < used_atom_cnt; i++) {
 		struct used_atom *atom = &used_atom[i];
diff --git a/ref-filter.h b/ref-filter.h
index 87b026b8b76d0..5c6e019998716 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -45,6 +45,7 @@ struct ref_array_item {
 	off_t disk_size;
 	const char *rest;
 	struct object_id *delta_base_oid;
+	const char *objectname;
 	char refname[FLEX_ARRAY];
 };
 

--
https://github.com/git/git/pull/452

```

## Olga Telezhnaya, 2018-02-12 08:08

Subject: [PATCH v3 14/23] ref_filter: add is_atom_used function
Message-ID: <01020161890f4357-05e636bc-8b44-425b-a252-ff2341f91cdd-000000@eu-west-1.amazonses.com>
URL: https://gitlist.dev/e/01020161890f4357-05e636bc-8b44-425b-a252-ff2341f91cdd-000000%40eu-west-1.amazonses.com
In-Reply-To: <01020161890f4236-47989eb4-c19f-4282-9084-9d4f90c2ebeb-000000@eu-west-1.amazonses.com>

```
Delete all items related to split_on_whitespace from ref-filter
and add new function for handling the logic.
Now cat-file could invoke that function to implementing its logic.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored by: Jeff King <peff@peff.net>
---
 builtin/cat-file.c |  8 +++-----
 ref-filter.c       | 17 +++++++++++++++--
 ref-filter.h       | 10 +++-------
 3 files changed, 21 insertions(+), 14 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 6db57e3533806..3a49b55a1cc2e 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -382,8 +382,7 @@ static int batch_objects(struct batch_options *opt)
 {
 	struct strbuf buf = STRBUF_INIT;
 	struct expand_data data;
-	int save_warning;
-	int retval = 0;
+	int save_warning, is_rest, retval = 0;
 
 	if (!opt->format.format)
 		opt->format.format = "%(objectname) %(objecttype) %(objectsize)";
@@ -395,8 +394,6 @@ static int batch_objects(struct batch_options *opt)
 	memset(&data, 0, sizeof(data));
 	opt->format.cat_file_data = &data;
 	verify_ref_format(&opt->format);
-	if (opt->cmdmode)
-		data.split_on_whitespace = 1;
 
 	if (opt->all_objects) {
 		struct object_info empty = OBJECT_INFO_INIT;
@@ -435,9 +432,10 @@ static int batch_objects(struct batch_options *opt)
 	 */
 	save_warning = warn_on_object_refname_ambiguity;
 	warn_on_object_refname_ambiguity = 0;
+	is_rest = opt->cmdmode || is_atom_used(&opt->format, "rest");
 
 	while (strbuf_getline(&buf, stdin) != EOF) {
-		if (data.split_on_whitespace) {
+		if (is_rest) {
 			/*
 			 * Split at first whitespace, tying off the beginning
 			 * of the string and saving the remainder (or NULL) in
diff --git a/ref-filter.c b/ref-filter.c
index 34a54db168265..4adeea6aad0da 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -493,8 +493,6 @@ static int parse_ref_filter_atom(const struct ref_format *format,
 		need_tagged = 1;
 	if (!strcmp(valid_atom[i].name, "symref"))
 		need_symref = 1;
-	if (cat_file_info && !strcmp(valid_atom[i].name, "rest"))
-		cat_file_info->split_on_whitespace = 1;
 	return at;
 }
 
@@ -730,6 +728,21 @@ static const char *find_next(const char *cp)
 	return NULL;
 }
 
+/* Search for atom in given format. */
+int is_atom_used(const struct ref_format *format, const char *atom)
+{
+	const char *cp, *sp;
+	for (cp = format->format; *cp && (sp = find_next(cp)); ) {
+		const char *ep = strchr(sp, ')');
+		int atom_len = ep - sp - 2;
+		sp += 2;
+		if (atom_len == strlen(atom) && !memcmp(sp, atom, atom_len))
+			return 1;
+		cp = ep + 1;
+	}
+	return 0;
+}
+
 /*
  * Make sure the format string is well formed, and parse out
  * the used atoms.
diff --git a/ref-filter.h b/ref-filter.h
index 5c6e019998716..fffc443726e28 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -86,13 +86,6 @@ struct expand_data {
 	const char *rest;
 	struct object_id delta_base_oid;
 
-	/*
-	 * Whether to split the input on whitespace before feeding it to
-	 * get_sha1; this is decided during the mark_query phase based on
-	 * whether we have a %(rest) token in our format.
-	 */
-	int split_on_whitespace;
-
 	/*
 	 * After a mark_query run, this object_info is set up to be
 	 * passed to sha1_object_info_extended. It will point to the data
@@ -186,4 +179,7 @@ void pretty_print_ref(const char *name, const unsigned char *sha1,
 /* Fill the values of request and prepare all data for final string creation */
 int populate_value(struct ref_array_item *ref);
 
+/* Search for atom in given format. */
+int is_atom_used(const struct ref_format *format, const char *atom);
+
 #endif /*  REF_FILTER_H  */

--
https://github.com/git/git/pull/452

```

## Olga Telezhnaya, 2018-02-12 08:08

Subject: [PATCH v3 17/23] ref-filter: make valid_atom general again
Message-ID: <01020161890f4374-60ceebf6-c8aa-4011-bac6-1350549235b1-000000@eu-west-1.amazonses.com>
URL: https://gitlist.dev/e/01020161890f4374-60ceebf6-c8aa-4011-bac6-1350549235b1-000000%40eu-west-1.amazonses.com
In-Reply-To: <01020161890f4236-47989eb4-c19f-4282-9084-9d4f90c2ebeb-000000@eu-west-1.amazonses.com>

```
Stop using valid_cat_file_atom, making the code more general.
Further commits will contain some tests, docs and
support of new features.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored by: Jeff King <peff@peff.net>
---
 ref-filter.c | 34 +++++++++-------------------------
 1 file changed, 9 insertions(+), 25 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index cc70bcf2bb8b1..ee311d51ff81c 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -358,8 +358,8 @@ static struct valid_atom {
 	void (*parser)(const struct ref_format *format, struct used_atom *atom, const char *arg);
 } valid_atom[] = {
 	{ "refname" , FIELD_STR, refname_atom_parser },
-	{ "objecttype" },
-	{ "objectsize", FIELD_ULONG },
+	{ "objecttype", FIELD_STR, objecttype_atom_parser },
+	{ "objectsize", FIELD_ULONG, objectsize_atom_parser },
 	{ "objectname", FIELD_STR, objectname_atom_parser },
 	{ "tree" },
 	{ "parent" },
@@ -396,12 +396,6 @@ static struct valid_atom {
 	{ "if", FIELD_STR, if_atom_parser },
 	{ "then" },
 	{ "else" },
-};
-
-static struct valid_atom valid_cat_file_atom[] = {
-	{ "objectname" },
-	{ "objecttype", FIELD_STR, objecttype_atom_parser },
-	{ "objectsize", FIELD_ULONG, objectsize_atom_parser },
 	{ "rest" },
 	{ "deltabase", FIELD_STR, deltabase_atom_parser },
 };
@@ -431,7 +425,6 @@ struct atom_value {
  * Used to parse format string and sort specifiers
  */
 static int parse_ref_filter_atom(const struct ref_format *format,
-				 const struct valid_atom *valid_atom, int n_atoms,
 				 const char *atom, const char *ep)
 {
 	const char *sp;
@@ -461,13 +454,13 @@ static int parse_ref_filter_atom(const struct ref_format *format,
 	atom_len = (arg ? arg : ep) - sp;
 
 	/* Is the atom a valid one? */
-	for (i = 0; i < n_atoms; i++) {
+	for (i = 0; i < ARRAY_SIZE(valid_atom); i++) {
 		int len = strlen(valid_atom[i].name);
 		if (len == atom_len && !memcmp(valid_atom[i].name, sp, len))
 			break;
 	}
 
-	if (n_atoms <= i)
+	if (ARRAY_SIZE(valid_atom) <= i)
 		die(_("unknown field name: %.*s"), (int)(ep-atom), atom);
 
 	/* Add it in, including the deref prefix */
@@ -761,15 +754,9 @@ int verify_ref_format(struct ref_format *format)
 			return error(_("malformed format string %s"), sp);
 		/* sp points at "%(" and ep points at the closing ")" */
 
-		if (cat_file_info.is_cat_file)
-			at = parse_ref_filter_atom(format, valid_cat_file_atom,
-						   ARRAY_SIZE(valid_cat_file_atom), sp + 2, ep);
-		else {
-			at = parse_ref_filter_atom(format, valid_atom,
-						   ARRAY_SIZE(valid_atom), sp + 2, ep);
-			if (skip_prefix(used_atom[at].name, "color:", &color))
-				format->need_color_reset_at_eol = !!strcmp(color, "reset");
-		}
+		at = parse_ref_filter_atom(format, sp + 2, ep);
+		if (skip_prefix(used_atom[at].name, "color:", &color))
+			format->need_color_reset_at_eol = !!strcmp(color, "reset");
 
 		cp = ep + 1;
 	}
@@ -2231,9 +2218,7 @@ int format_ref_array_item(struct ref_array_item *info,
 		if (cp < sp)
 			append_literal(cp, sp, &state);
 		if (get_ref_atom_value(info,
-				       parse_ref_filter_atom(format, valid_atom,
-							     ARRAY_SIZE(valid_atom),
-							     sp + 2, ep),
+				       parse_ref_filter_atom(format, sp + 2, ep),
 				       &atomv))
 			return -1;
 		atomv->handler(atomv, &state);
@@ -2286,8 +2271,7 @@ static int parse_sorting_atom(const char *atom)
 	 */
 	struct ref_format dummy = REF_FORMAT_INIT;
 	const char *end = atom + strlen(atom);
-	return parse_ref_filter_atom(&dummy, valid_atom,
-				     ARRAY_SIZE(valid_atom), atom, end);
+	return parse_ref_filter_atom(&dummy, atom, end);
 }
 
 /*  If no sorting option is given, use refname to sort as default */

--
https://github.com/git/git/pull/452

```

## Olga Telezhnaya, 2018-02-12 08:08

Subject: [PATCH v3 13/23] ref-filter: get rid of mark_atom_in_object_info()
Message-ID: <01020161890f4353-44cc5f60-0e42-492a-9d7f-0f6f1c7b3501-000000@eu-west-1.amazonses.com>
URL: https://gitlist.dev/e/01020161890f4353-44cc5f60-0e42-492a-9d7f-0f6f1c7b3501-000000%40eu-west-1.amazonses.com
In-Reply-To: <01020161890f4236-47989eb4-c19f-4282-9084-9d4f90c2ebeb-000000@eu-west-1.amazonses.com>

```
Remove mark_atom_in_object_info() and create same logic
in terms of ref-filter style.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored by: Jeff King <peff@peff.net>
---
 ref-filter.c | 45 +++++++++++++++++++++------------------------
 1 file changed, 21 insertions(+), 24 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 3f92a27d98b6c..34a54db168265 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -255,13 +255,29 @@ static void objectname_atom_parser(const struct ref_format *format, struct used_
 static void objectsize_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
 {
 	if (!arg)
-		; /* default to normal object size */
+		cat_file_info->info.sizep = &cat_file_info->size;
 	else if (!strcmp(arg, "disk"))
 		cat_file_info->info.disk_sizep = &cat_file_info->disk_size;
 	else
 		die(_("urecognized %%(objectsize) argument: %s"), arg);
 }
 
+static void objecttype_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
+{
+	if (!arg)
+		cat_file_info->info.typep = &cat_file_info->type;
+	else
+		die(_("urecognized %%(objecttype) argument: %s"), arg);
+}
+
+static void deltabase_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
+{
+	if (!arg)
+		cat_file_info->info.delta_base_sha1 = cat_file_info->delta_base_oid.hash;
+	else
+		die(_("urecognized %%(deltabase) argument: %s"), arg);
+}
+
 static void refname_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)
 {
 	refname_atom_parser_internal(&atom->u.refname, arg, atom->name);
@@ -384,10 +400,10 @@ static struct valid_atom {
 
 static struct valid_atom valid_cat_file_atom[] = {
 	{ "objectname" },
-	{ "objecttype" },
+	{ "objecttype", FIELD_STR, objecttype_atom_parser },
 	{ "objectsize", FIELD_ULONG, objectsize_atom_parser },
 	{ "rest" },
-	{ "deltabase" },
+	{ "deltabase", FIELD_STR, deltabase_atom_parser },
 };
 
 #define REF_FORMATTING_STATE_INIT  { 0, NULL }
@@ -411,25 +427,6 @@ struct atom_value {
 	struct used_atom *atom;
 };
 
-static int is_atom(const char *atom, const char *s, int slen)
-{
-	int alen = strlen(atom);
-	return alen == slen && !memcmp(atom, s, alen);
-}
-
-static void mark_atom_in_object_info(const char *atom, int len,
-				    struct expand_data *data)
-{
-	if (is_atom("objecttype", atom, len))
-		data->info.typep = &data->type;
-	else if (is_atom("objectsize", atom, len))
-		data->info.sizep = &data->size;
-	else if (is_atom("rest", atom, len))
-		data->split_on_whitespace = 1;
-	else if (is_atom("deltabase", atom, len))
-		data->info.delta_base_sha1 = data->delta_base_oid.hash;
-}
-
 /*
  * Used to parse format string and sort specifiers
  */
@@ -496,8 +493,8 @@ static int parse_ref_filter_atom(const struct ref_format *format,
 		need_tagged = 1;
 	if (!strcmp(valid_atom[i].name, "symref"))
 		need_symref = 1;
-	if (cat_file_info)
-		mark_atom_in_object_info(atom, atom_len, cat_file_info);
+	if (cat_file_info && !strcmp(valid_atom[i].name, "rest"))
+		cat_file_info->split_on_whitespace = 1;
 	return at;
 }
 

--
https://github.com/git/git/pull/452

```

## Olga Telezhnaya, 2018-02-12 08:08

Subject: [PATCH v3 22/23] cat-file: tests for new atoms added
Message-ID: <01020161890f4388-f6427c9d-2be6-4628-afc9-bfaeb58b1ebe-000000@eu-west-1.amazonses.com>
URL: https://gitlist.dev/e/01020161890f4388-f6427c9d-2be6-4628-afc9-bfaeb58b1ebe-000000%40eu-west-1.amazonses.com
In-Reply-To: <01020161890f4236-47989eb4-c19f-4282-9084-9d4f90c2ebeb-000000@eu-west-1.amazonses.com>

```
Add some tests for new formatting atoms from ref-filter.
Some of new atoms are supported automatically,
some of them are expanded into empty string
(because they are useless for some types of objects),
some of them could be supported later in other patches.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored by: Jeff King <peff@peff.net>
---
 t/t1006-cat-file.sh | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
index b19f332694620..e72fcaf0e02c5 100755
--- a/t/t1006-cat-file.sh
+++ b/t/t1006-cat-file.sh
@@ -20,6 +20,19 @@ maybe_remove_timestamp () {
     fi
 }
 
+test_atom () {
+    name=$1
+    sha1=$2
+    atoms=$3
+    expected=$4
+
+    test_expect_success "$name" '
+	echo "$expected" >expect &&
+	echo $sha1 | git cat-file --batch-check="$atoms" >actual &&
+	test_cmp expect actual
+    '
+}
+
 run_tests () {
     type=$1
     sha1=$2
@@ -119,6 +132,13 @@ $content"
 	maybe_remove_timestamp "$(cat actual.full)" $no_ts >actual &&
 	test_cmp expect actual
     '
+
+    for atom in refname parent body trailers upstream push symref flag
+    do
+	test_atom "Check %($atom) gives empty output" "$sha1" "%($atom)" ""
+    done
+
+    test_atom "Check %(HEAD) gives only one space as output" "$sha1" '%(HEAD)' ' '
 }
 
 hello_content="Hello World"
@@ -140,6 +160,12 @@ test_expect_success '--batch-check without %(rest) considers whole line' '
 	test_cmp expect actual
 '
 
+shortname=`echo $hello_sha1 | sed 's/^.\{0\}\(.\{7\}\).*/\1/'`
+test_atom 'Check format option %(objectname:short) works' "$hello_sha1" '%(objectname:short)' "$shortname"
+
+test_atom 'Check format option %(align) is not broken' \
+    "$hello_sha1" "%(align:8)%(objecttype)%(end)%(objectname)" "blob    $hello_sha1"
+
 tree_sha1=$(git write-tree)
 tree_size=33
 tree_pretty_content="100644 blob $hello_sha1	hello"
@@ -157,6 +183,17 @@ $commit_message"
 
 run_tests 'commit' $commit_sha1 $commit_size "$commit_content" "$commit_content" 1
 
+test_atom "Check format option %(if) is not broken" "$commit_sha1" \
+    "%(if)%(author)%(then)%(objectname)%(end)" "$commit_sha1"
+test_atom "Check %(tree) works for commit" "$commit_sha1" "%(tree)" "$tree_sha1"
+test_atom "Check %(numparent) works for commit" "$commit_sha1" "%(numparent)" "0"
+test_atom "Check %(authorname) works for commit" "$commit_sha1" "%(authorname)" "$GIT_AUTHOR_NAME"
+test_atom "Check %(authoremail) works for commit" "$commit_sha1" "%(authoremail)" "<$GIT_AUTHOR_EMAIL>"
+test_atom "Check %(committername) works for commit" "$commit_sha1" "%(committername)" "$GIT_COMMITTER_NAME"
+test_atom "Check %(committeremail) works for commit" "$commit_sha1" "%(committeremail)" "<$GIT_COMMITTER_EMAIL>"
+test_atom "Check %(subject) works for commit" "$commit_sha1" "%(subject)" "$commit_message"
+test_atom "Check %(contents) works for commit" "$commit_sha1" "%(contents)" "$commit_message"
+
 tag_header_without_timestamp="object $hello_sha1
 type blob
 tag hellotag
@@ -171,6 +208,17 @@ tag_size=$(strlen "$tag_content")
 
 run_tests 'tag' $tag_sha1 $tag_size "$tag_content" "$tag_content" 1
 
+test_atom "Check %(object) works for tag" "$tag_sha1" "%(object)" "$hello_sha1"
+test_atom "Check %(type) works for tag" "$tag_sha1" "%(type)" "blob"
+test_atom "Check %(tag) works for tag" "$tag_sha1" "%(tag)" "hellotag"
+test_atom "Check %(taggername) works for tag" "$tag_sha1" "%(taggername)" "$GIT_COMMITTER_NAME"
+test_atom "Check %(taggeremail) works for tag" "$tag_sha1" "%(taggeremail)" "<$GIT_COMMITTER_EMAIL>"
+test_atom "Check %(subject) works for tag" "$tag_sha1" "%(subject)" "$tag_description"
+test_atom "Check %(contents) works for tag" "$tag_sha1" "%(contents)" "$tag_description"
+
+test_atom "Check %(color) gives no additional output" "$sha1" \
+    "%(objectname) %(color:green) %(objecttype)" "$sha1  $type"
+
 test_expect_success \
     "Reach a blob from a tag pointing to it" \
     "test '$hello_content' = \"\$(git cat-file blob $tag_sha1)\""

--
https://github.com/git/git/pull/452

```

## Olga Telezhnaya, 2018-02-12 08:08

Subject: [PATCH v3 06/23] cat-file: split expand_atom() into 2 functions
Message-ID: <01020161890f433d-ba3260aa-b9e5-47d0-8e80-46ccf9f8c60f-000000@eu-west-1.amazonses.com>
URL: https://gitlist.dev/e/01020161890f433d-ba3260aa-b9e5-47d0-8e80-46ccf9f8c60f-000000%40eu-west-1.amazonses.com
In-Reply-To: <01020161890f4236-47989eb4-c19f-4282-9084-9d4f90c2ebeb-000000@eu-west-1.amazonses.com>

```
Split expand_atom() into 2 different functions,
mark_atom_in_object_info() prepares variable for further filling,
(new) expand_atom() creates resulting string.
Need that for further reusing of formatting logic from ref-filter.
Both functions will be step-by-step removed by the end of this patch.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored by: Jeff King <peff@peff.net>
---
 builtin/cat-file.c | 73 ++++++++++++++++++++++++++++--------------------------
 1 file changed, 38 insertions(+), 35 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 37d6096d201b5..edb04a96d9bd3 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -182,47 +182,47 @@ static int is_atom(const char *atom, const char *s, int slen)
 	return alen == slen && !memcmp(atom, s, alen);
 }
 
-static void expand_atom(struct strbuf *sb, const char *atom, int len,
-			void *vdata)
+static void mark_atom_in_object_info(const char *atom, int len,
+				     struct expand_data *data)
 {
-	struct expand_data *data = vdata;
+	if (is_atom("objectname", atom, len))
+		; /* do nothing */
+	else if (is_atom("objecttype", atom, len))
+		data->info.typep = &data->type;
+	else if (is_atom("objectsize", atom, len))
+		data->info.sizep = &data->size;
+	else if (is_atom("objectsize:disk", atom, len))
+		data->info.disk_sizep = &data->disk_size;
+	else if (is_atom("rest", atom, len))
+		data->split_on_whitespace = 1;
+	else if (is_atom("deltabase", atom, len))
+		data->info.delta_base_sha1 = data->delta_base_oid.hash;
+	else
+		die("unknown format element: %.*s", len, atom);
+}
 
-	if (is_atom("objectname", atom, len)) {
-		if (!data->mark_query)
-			strbuf_addstr(sb, oid_to_hex(&data->oid));
-	} else if (is_atom("objecttype", atom, len)) {
-		if (data->mark_query)
-			data->info.typep = &data->type;
-		else
-			strbuf_addstr(sb, typename(data->type));
-	} else if (is_atom("objectsize", atom, len)) {
-		if (data->mark_query)
-			data->info.sizep = &data->size;
-		else
-			strbuf_addf(sb, "%lu", data->size);
-	} else if (is_atom("objectsize:disk", atom, len)) {
-		if (data->mark_query)
-			data->info.disk_sizep = &data->disk_size;
-		else
-			strbuf_addf(sb, "%"PRIuMAX, (uintmax_t)data->disk_size);
-	} else if (is_atom("rest", atom, len)) {
-		if (data->mark_query)
-			data->split_on_whitespace = 1;
-		else if (data->rest)
+static void expand_atom(struct strbuf *sb, const char *atom, int len,
+			 struct expand_data *data)
+{
+	if (is_atom("objectname", atom, len))
+		strbuf_addstr(sb, oid_to_hex(&data->oid));
+	else if (is_atom("objecttype", atom, len))
+		strbuf_addstr(sb, typename(data->type));
+	else if (is_atom("objectsize", atom, len))
+		strbuf_addf(sb, "%lu", data->size);
+	else if (is_atom("objectsize:disk", atom, len))
+		strbuf_addf(sb, "%"PRIuMAX, (uintmax_t)data->disk_size);
+	else if (is_atom("rest", atom, len)) {
+		if (data->rest)
 			strbuf_addstr(sb, data->rest);
-	} else if (is_atom("deltabase", atom, len)) {
-		if (data->mark_query)
-			data->info.delta_base_sha1 = data->delta_base_oid.hash;
-		else
-			strbuf_addstr(sb,
-				      oid_to_hex(&data->delta_base_oid));
-	} else
-		die("unknown format element: %.*s", len, atom);
+	} else if (is_atom("deltabase", atom, len))
+		strbuf_addstr(sb, oid_to_hex(&data->delta_base_oid));
 }
 
-static size_t expand_format(struct strbuf *sb, const char *start, void *data)
+static size_t expand_format(struct strbuf *sb, const char *start, void *vdata)
 {
 	const char *end;
+	struct expand_data *data = vdata;
 
 	if (*start != '(')
 		return 0;
@@ -230,7 +230,10 @@ static size_t expand_format(struct strbuf *sb, const char *start, void *data)
 	if (!end)
 		die("format element '%s' does not end in ')'", start);
 
-	expand_atom(sb, start + 1, end - start - 1, data);
+	if (data->mark_query)
+		mark_atom_in_object_info(start + 1, end - start - 1, data);
+	else
+		expand_atom(sb, start + 1, end - start - 1, data);
 
 	return end - start + 1;
 }

--
https://github.com/git/git/pull/452

```

## Jeff King, 2018-02-15 05:08

Subject: Re: [PATCH v3 01/23] ref-filter: get rid of goto
Message-ID: <20180215050850.GA18780@sigill.intra.peff.net>
URL: https://gitlist.dev/e/20180215050850.GA18780%40sigill.intra.peff.net
In-Reply-To: <01020161890f4236-47989eb4-c19f-4282-9084-9d4f90c2ebeb-000000@eu-west-1.amazonses.com>

```
On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:

> Get rid of goto command in ref-filter for better readability.
> 
> Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored by: Jeff King <peff@peff.net>

OK, makes sense, and the patch looks correct.

-Peff

```

## Jeff King, 2018-02-15 05:16

Subject: Re: [PATCH v3 02/23] ref-filter: add return value to some functions
Message-ID: <20180215051658.GB18780@sigill.intra.peff.net>
URL: https://gitlist.dev/e/20180215051658.GB18780%40sigill.intra.peff.net
In-Reply-To: <01020161890f4311-ab635cac-1d48-47ee-90e1-f178f134db1c-000000@eu-west-1.amazonses.com>

```
On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:

> Add return flag to format_ref_array_item(), show_ref_array_item(),
> get_ref_array_info() and populate_value() for further using.
> Need it to handle situations when item is broken but we can not invoke
> die() because we are in batch mode and all items need to be processed.

OK. The source of these errors would eventually be calls in
populate_value(), but we don't flag any errors there yet (well, we do,
but they all end up in die() for now). So I'd expect to see later in the
series those die() calls converted to errors (I haven't looked further
yet; just making a note to myself).

> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1356,8 +1356,9 @@ static const char *get_refname(struct used_atom *atom, struct ref_array_item *re
>  
>  /*
>   * Parse the object referred by ref, and grab needed value.
> + * Return 0 if everything was successful, -1 otherwise.
>   */

We discussed off-list the concept that the caller may want to know one
of three outcomes:

  - we completed the request, having accessed the object
  - we completed the request, but it didn't require accessing any
    objects
  - an error occurred accessing the object

Since callers like "cat-file" would need to check has_sha1_file()
manually in the second case. Should this return value actually be an
enum, which would make it easier to convert later to a tri-state?

> -static void populate_value(struct ref_array_item *ref)
> +static int populate_value(struct ref_array_item *ref)
>  {
>  	void *buf;
>  	struct object *obj;
> @@ -1482,7 +1483,7 @@ static void populate_value(struct ref_array_item *ref)
>  		}
>  	}
>  	if (used_atom_cnt <= i)
> -		return;
> +		return 0;

Most of these conversions are obviously correct, because they just turn
a void return into one with a value. But this one is trickier:

> @@ -2138,9 +2144,10 @@ void format_ref_array_item(struct ref_array_item *info,
>  		ep = strchr(sp, ')');
>  		if (cp < sp)
>  			append_literal(cp, sp, &state);
> -		get_ref_atom_value(info,
> -				   parse_ref_filter_atom(format, sp + 2, ep),
> -				   &atomv);
> +		if (get_ref_atom_value(info,
> +				       parse_ref_filter_atom(format, sp + 2, ep),
> +				       &atomv))
> +			return -1;
>  		atomv->handler(atomv, &state);
>  	}

since it affects the control flow. Might we be skipping any necessary
cleanup in the function if we see an error?

It looks like we may have called push_stack_element(), but we'd never
get to the end of the function where we call pop_stack_element(),
causing us to leak.

-Peff

```

## Jeff King, 2018-02-15 05:18

Subject: Re: [PATCH v3 03/23] cat-file: reuse struct ref_format
Message-ID: <20180215051831.GC18780@sigill.intra.peff.net>
URL: https://gitlist.dev/e/20180215051831.GC18780%40sigill.intra.peff.net
In-Reply-To: <01020161890f432e-07d6260a-0100-4859-af1c-2cdf8da76829-000000@eu-west-1.amazonses.com>

```
On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:

> Start using ref_format struct instead of simple char*.
> Need that for further reusing of formatting logic from ref-filter.

Makes sense, and the patch itself looks correct.

-Peff

```

## Jeff King, 2018-02-15 05:23

Subject: Re: [PATCH v3 04/23] ref-filter: make valid_atom as function parameter
Message-ID: <20180215052312.GD18780@sigill.intra.peff.net>
URL: https://gitlist.dev/e/20180215052312.GD18780%40sigill.intra.peff.net
In-Reply-To: <01020161890f4324-be5dda9b-9ad7-402f-9196-30f982fd20c2-000000@eu-west-1.amazonses.com>

```
On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:

> Make valid_atom as a function parameter,
> there could be another variable further.
> Need that for further reusing of formatting logic in cat-file.c.
> 
> We do not need to allow users to pass their own valid_atom variable in
> global functions like verify_ref_format() because in the end we want to
> have same set of valid atoms for all commands. But, as a first step
> of migrating, I create further another version of valid_atom
> for cat-file.

Hmm. So I see where you're going here, but I think in the end we'd want
to have a single valid_atom list again, and we wouldn't need this.

And indeed, it looks like this goes away in patch 17. Can we
reorganize/rebase the series so that we avoid dead-end directions like
this?

-Peff

```

## Jeff King, 2018-02-15 05:26

Subject: Re: [PATCH v3 05/23] cat-file: move struct expand_data into ref-filter
Message-ID: <20180215052604.GE18780@sigill.intra.peff.net>
URL: https://gitlist.dev/e/20180215052604.GE18780%40sigill.intra.peff.net
In-Reply-To: <01020161890f4325-7ab9a01a-ab67-4beb-9721-3dcc5095720a-000000@eu-west-1.amazonses.com>

```
On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:

> Need that for further reusing of formatting logic in cat-file.
> Have plans to get rid of using expand_data in cat-file at all,
> and use it only in ref-filter for collecting, formatting and printing
> needed data.

This seems like another step that we're going to end up reversing later,
because ref-filter has a different representation for these kinds of
expansions. I'll keep reading...

-Peff

```

## Jeff King, 2018-02-15 05:28

Subject: Re: [PATCH v3 06/23] cat-file: split expand_atom() into 2 functions
Message-ID: <20180215052840.GF18780@sigill.intra.peff.net>
URL: https://gitlist.dev/e/20180215052840.GF18780%40sigill.intra.peff.net
In-Reply-To: <01020161890f433d-ba3260aa-b9e5-47d0-8e80-46ccf9f8c60f-000000@eu-west-1.amazonses.com>

```
On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:

> Split expand_atom() into 2 different functions,
> mark_atom_in_object_info() prepares variable for further filling,
> (new) expand_atom() creates resulting string.
> Need that for further reusing of formatting logic from ref-filter.
> Both functions will be step-by-step removed by the end of this patch.

Hmm, this is duplicating the list of atoms. One of the reasons for the
mark_query flag in the original is to have a single list, so that it
can't go out of sync. So I'd hope to see this split go away by the end
of the series...

-Peff

```

## Jeff King, 2018-02-15 05:32

Subject: Re: [PATCH v3 07/23] cat-file: start migrating formatting to ref-filter
Message-ID: <20180215053244.GG18780@sigill.intra.peff.net>
URL: https://gitlist.dev/e/20180215053244.GG18780%40sigill.intra.peff.net
In-Reply-To: <01020161890f4348-10a54126-e31d-448d-93a3-55fd89c248c4-000000@eu-west-1.amazonses.com>

```
On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:

> Move mark_atom_in_object_info() from cat-file to ref-filter and
> start using it in verify_ref_format().
> It also means that we start reusing verify_ref_format() in cat-file.
> 
> Start from simple moving of mark_atom_in_object_info(),
> it would be removed later by integrating all needed processes into
> ref-filter logic.

I see where you're going here, but it feels like there's a lot of
boilerplate to move this thing whole, when we plan to get rid of it in
the long run. After all, we _already_ support most of these placeholders
in ref-filter. I'll keep reading...

-Peff

```

## Jeff King, 2018-02-15 05:37

Subject: Re: [PATCH v3 08/23] ref-filter: reuse parse_ref_filter_atom()
Message-ID: <20180215053714.GH18780@sigill.intra.peff.net>
URL: https://gitlist.dev/e/20180215053714.GH18780%40sigill.intra.peff.net
In-Reply-To: <01020161890f434b-3f93f5b7-0fad-4777-ab9b-79c48b2b41d2-000000@eu-west-1.amazonses.com>

```
On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:

> Continue migrating formatting logic from cat-file to ref-filter.
> Reuse parse_ref_filter_atom() for unifying all processes in ref-filter
> and further removing of mark_atom_in_object_info().

OK, now it looks we're moving in a good direction.

One thing that puzzles me:

> @@ -401,20 +420,14 @@ static int is_atom(const char *atom, const char *s, int slen)
>  static void mark_atom_in_object_info(const char *atom, int len,
>  				    struct expand_data *data)
>  {
> -	if (is_atom("objectname", atom, len))
> -		; /* do nothing */
> -	else if (is_atom("objecttype", atom, len))
> +	if (is_atom("objecttype", atom, len))
>  		data->info.typep = &data->type;
>  	else if (is_atom("objectsize", atom, len))
>  		data->info.sizep = &data->size;
> -	else if (is_atom("objectsize:disk", atom, len))
> -		data->info.disk_sizep = &data->disk_size;
>  	else if (is_atom("rest", atom, len))
>  		data->split_on_whitespace = 1;
>  	else if (is_atom("deltabase", atom, len))
>  		data->info.delta_base_sha1 = data->delta_base_oid.hash;
> -	else
> -		die("unknown format element: %.*s", len, atom);
>  }

Why do some of these atoms go away and not others? It seems like we're
now relying on ref-filter to parse some of the common ones using its
existing atom-parser. But wouldn't it have objecttype and objectsize
already, then?

-Peff

```

## Jeff King, 2018-02-15 05:40

Subject: Re: [PATCH v3 09/23] cat-file: start use ref_array_item struct
Message-ID: <20180215054018.GI18780@sigill.intra.peff.net>
URL: https://gitlist.dev/e/20180215054018.GI18780%40sigill.intra.peff.net
In-Reply-To: <01020161890f435d-05435bc4-30da-41c4-8bb2-37dcccb4a413-000000@eu-west-1.amazonses.com>

```
On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:

> Moving from using expand_data to ref_array_item structure.
> That helps us to reuse functions from ref-filter easier.

This one feels weird. The point of a ref_array_item is for the caller to
feed data into the ref-filter formatting code (usually that data comes
from an earlier call to filter_refs(), but in the new cat-file we'd
presumably feed single items).

But here we're adding a bunch of fields for items that we'd expect the
format code to compute itself.

-Peff

```

## Jeff King, 2018-02-15 05:42

Subject: Re: [PATCH v3 12/23] cat-file: start reusing populate_value()
Message-ID: <20180215054203.GJ18780@sigill.intra.peff.net>
URL: https://gitlist.dev/e/20180215054203.GJ18780%40sigill.intra.peff.net
In-Reply-To: <01020161890f435c-c3044984-d9aa-4aaf-90e3-1affa109b3b3-000000@eu-west-1.amazonses.com>

```
On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:

> Move logic related to getting object info from cat-file to ref-filter.
> It will help to reuse whole formatting logic from ref-filter further.

This feels like another wrong-direction step, because we'd eventually
expect populate_value() to be called under the hood. And I think in the
end we end up back there. Which is good, but it's hard to keep track of
the dead-ends here...

-Peff

```

## Jeff King, 2018-02-15 05:45

Subject: Re: [PATCH v3 13/23] ref-filter: get rid of mark_atom_in_object_info()
Message-ID: <20180215054542.GK18780@sigill.intra.peff.net>
URL: https://gitlist.dev/e/20180215054542.GK18780%40sigill.intra.peff.net
In-Reply-To: <01020161890f4353-44cc5f60-0e42-492a-9d7f-0f6f1c7b3501-000000@eu-west-1.amazonses.com>

```
On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:

> Remove mark_atom_in_object_info() and create same logic
> in terms of ref-filter style.

This one is definitely a step in the right direction. In fact, this is
what I would have expected to see in the beginning. It seems like this
first half of the series really would benefit from being squashed into a
few commits. I.e., I'd have expected to see the whole series looking
something like:

  - preparatory cleanup of ref-filter

  - teach ref-filter any new atoms (or atom options) that cat-file knows
    about but it doesn't

  - convert cat-file to use ref-filter

Most of what I've seen so far is basically that second step, but strung
out along a bunch of commits. Can we compact it into a few commits that
all make clear forward progress (by using "rebase -i" with "squash")?

-Peff

```

## Jeff King, 2018-02-15 05:49

Subject: Re: [PATCH v3 14/23] ref_filter: add is_atom_used function
Message-ID: <20180215054945.GL18780@sigill.intra.peff.net>
URL: https://gitlist.dev/e/20180215054945.GL18780%40sigill.intra.peff.net
In-Reply-To: <01020161890f4357-05e636bc-8b44-425b-a252-ff2341f91cdd-000000@eu-west-1.amazonses.com>

```
On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:

> Delete all items related to split_on_whitespace from ref-filter
> and add new function for handling the logic.
> Now cat-file could invoke that function to implementing its logic.

OK, this is a good direction. I think in a more compact series we'd
avoid moving the split-on-whitespace bits over to ref-filter in the
first place, and have two commits:

 - one early in the series adding is_atom_used()

 - one late in the series switching cat-file over to is_atom_used() as
   part of the conversion to ref-filter

> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index 6db57e3533806..3a49b55a1cc2e 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -382,8 +382,7 @@ static int batch_objects(struct batch_options *opt)
>  {
>  	struct strbuf buf = STRBUF_INIT;
>  	struct expand_data data;
> -	int save_warning;
> -	int retval = 0;
> +	int save_warning, is_rest, retval = 0;

Try to avoid reformatting existing code that you're not otherwise
touching, as it makes the diff noisier. Just adding "int is_rest" would
make this easier to review.

I also think the variable name should probably still be
"split_on_whitespace". It's set based on whether we saw a "%(rest)"
atom, but ultimately we'll use it to decide whether to split.

-Peff

```

## Jeff King, 2018-02-15 05:51

Subject: Re: [PATCH v3 15/23] cat-file: move skip_object_info into ref-filter
Message-ID: <20180215055158.GM18780@sigill.intra.peff.net>
URL: https://gitlist.dev/e/20180215055158.GM18780%40sigill.intra.peff.net
In-Reply-To: <01020161890f435c-80fa0ef3-afaf-45b7-84a7-28e2bb66f320-000000@eu-west-1.amazonses.com>

```
On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:

> Move logic related to skip_object_info into ref-filter,
> so that cat-file does not use that field at all.

I think this is going the wrong way. ref-filter should always do as
little work as possible to fulfill the request. So it should skip the
object_info call whenever it can. And then any callers who want to make
sure that the object exists can do so (as long as the formatting code
tells them whether it looked up the object or not).

And then ref-filter doesn't have to know about this skip_object_info
flag at all.

-Peff

```

## Jeff King, 2018-02-15 05:53

Subject: Re: [PATCH v3 16/23] ref-filter: make cat_file_info independent
Message-ID: <20180215055334.GN18780@sigill.intra.peff.net>
URL: https://gitlist.dev/e/20180215055334.GN18780%40sigill.intra.peff.net
In-Reply-To: <01020161890f4376-0d1c5615-ae9c-4320-bc2d-fd3bda19bce0-000000@eu-west-1.amazonses.com>

```
On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:

> Remove connection between expand_data variable
> in cat-file and in ref-filter.
> It will help further to get rid of using expand_data in cat-file.

I have to admit I'm confused at this point about what is_cat_file is
for, or even why we need cat_file_data. Shouldn't these items be handled
by their matching ref-filter atoms at this point?

-Peff

```

## Jeff King, 2018-02-15 05:53

Subject: Re: [PATCH v3 17/23] ref-filter: make valid_atom general again
Message-ID: <20180215055351.GO18780@sigill.intra.peff.net>
URL: https://gitlist.dev/e/20180215055351.GO18780%40sigill.intra.peff.net
In-Reply-To: <01020161890f4374-60ceebf6-c8aa-4011-bac6-1350549235b1-000000@eu-west-1.amazonses.com>

```
On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:

> Stop using valid_cat_file_atom, making the code more general.
> Further commits will contain some tests, docs and
> support of new features.

Yay.

-Peff

```

## Jeff King, 2018-02-15 05:56

Subject: Re: [PATCH v3 20/23] ref-filter: unifying formatting of cat-file opts
Message-ID: <20180215055618.GP18780@sigill.intra.peff.net>
URL: https://gitlist.dev/e/20180215055618.GP18780%40sigill.intra.peff.net
In-Reply-To: <01020161890f437a-302b1577-f034-4a24-a3a2-851d1cda0044-000000@eu-west-1.amazonses.com>

```
On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:

> cat-file options are now filled by general logic.

Yay.

One puzzling thing:

> diff --git a/ref-filter.c b/ref-filter.c
> index 8d104b567eb7c..5781416cf9126 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -824,8 +824,12 @@ static void grab_common_values(struct atom_value *val, int deref, struct object
>  		else if (!strcmp(name, "objectsize")) {
>  			v->value = sz;
>  			v->s = xstrfmt("%lu", sz);
> -		}
> -		else if (deref)
> +		} else if (!strcmp(name, "objectsize:disk")) {
> +			if (cat_file_info.is_cat_file) {
> +				v->value = cat_file_info.disk_size;
> +				v->s = xstrfmt("%"PRIuMAX, (uintmax_t)v->value);
> +			}
> +		} else if (deref)

Why do we care about is_cat_file here. Shouldn't:

  git for-each-ref --format='%(objectsize:disk)'

work? I.e., shouldn't the cat_file_info.disk_size variable be held
somewhere in a used_atom struct?

-Peff

```

## Jeff King, 2018-02-15 05:57

Subject: Re: [PATCH v3 21/23] for-each-ref: tests for new atoms added
Message-ID: <20180215055711.GQ18780@sigill.intra.peff.net>
URL: https://gitlist.dev/e/20180215055711.GQ18780%40sigill.intra.peff.net
In-Reply-To: <01020161890f438c-9d30a51c-129c-4721-a85a-a3d66a2270b6-000000@eu-west-1.amazonses.com>

```
On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:

> Add tests for new formatting atoms: rest, deltabase, objectsize:disk.
> rest means nothing and we expand it into empty string.
> We need this atom for cat-file command.
> Have plans to support deltabase and objectsize:disk further
> (as it is done in cat-file), now also expand it to empty string.

I'm glad that you're adding tests, but I'm not sure it's a good idea to
add tests checking for the thing we know to be wrong. If anything, you
could be adding test_expect_failure looking for the _right_ thing, and
accept that it does not yet work.

-Peff

```

## Jeff King, 2018-02-15 06:00

Subject: Re: [PATCH v3 23/23] cat-file: update of docs
Message-ID: <20180215060033.GR18780@sigill.intra.peff.net>
URL: https://gitlist.dev/e/20180215060033.GR18780%40sigill.intra.peff.net
In-Reply-To: <01020161890f4391-9f76231a-4768-461b-80b9-02cada27a943-000000@eu-west-1.amazonses.com>

```
On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:

> Update the docs for cat-file command. Some new formatting atoms added
> because of reusing ref-filter code.
> We do not support cat-file atoms in general formatting logic
> (there is just the support for cat-file), that is why some of the atoms
> are still explained in cat-file docs.
> We need to move these explanations when atoms will be supported
> by other commands.

OK, so I've read through the whole series now. I still think it really
needs some squashing in the middle to turn into a more comprehensible
series.

And I think by the end we need to address the atoms here that don't work
in ref-filter. We certainly want them to work in the long term, and the
fact that they don't is I think pointing to having the wrong
architecture here in the intermediate step. We should just have a single
set of placeholders, with no special "cat_file_data". If I understand
correctly, that's what's causing those atoms not to work in
for-each-ref.

-Peff

```

## Оля Тележная, 2018-02-15 09:59

Subject: Re: [PATCH v3 02/23] ref-filter: add return value to some functions
Message-ID: <CAL21BmkQJTGYwR2BywNtsDVuMSGQVO+eWDTqTJL-WoiuAjmg0w@mail.gmail.com>
URL: https://gitlist.dev/e/CAL21BmkQJTGYwR2BywNtsDVuMSGQVO%2BeWDTqTJL-WoiuAjmg0w%40mail.gmail.com
In-Reply-To: <20180215051658.GB18780@sigill.intra.peff.net>

```
2018-02-15 8:16 GMT+03:00 Jeff King <peff@peff.net>:
> On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:
>
>> Add return flag to format_ref_array_item(), show_ref_array_item(),
>> get_ref_array_info() and populate_value() for further using.
>> Need it to handle situations when item is broken but we can not invoke
>> die() because we are in batch mode and all items need to be processed.
>
> OK. The source of these errors would eventually be calls in
> populate_value(), but we don't flag any errors there yet (well, we do,
> but they all end up in die() for now). So I'd expect to see later in the
> series those die() calls converted to errors (I haven't looked further
> yet; just making a note to myself).
>
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -1356,8 +1356,9 @@ static const char *get_refname(struct used_atom *atom, struct ref_array_item *re
>>
>>  /*
>>   * Parse the object referred by ref, and grab needed value.
>> + * Return 0 if everything was successful, -1 otherwise.
>>   */
>
> We discussed off-list the concept that the caller may want to know one
> of three outcomes:
>
>   - we completed the request, having accessed the object
>   - we completed the request, but it didn't require accessing any
>     objects
>   - an error occurred accessing the object
>
> Since callers like "cat-file" would need to check has_sha1_file()
> manually in the second case. Should this return value actually be an
> enum, which would make it easier to convert later to a tri-state?

I decided not to implement this particular scenario because all other
callers are waiting that everything will be printed inside ref-filter.
We just add support for cat-file there. I don't think that I need to
re-think all printing process and move printing logic to all other
callers so that cat-file will behave fine. In my opinion, in the final
version cat-file must accept all ref-filter logic parts and adapt to
them.

>
>> -static void populate_value(struct ref_array_item *ref)
>> +static int populate_value(struct ref_array_item *ref)
>>  {
>>       void *buf;
>>       struct object *obj;
>> @@ -1482,7 +1483,7 @@ static void populate_value(struct ref_array_item *ref)
>>               }
>>       }
>>       if (used_atom_cnt <= i)
>> -             return;
>> +             return 0;
>
> Most of these conversions are obviously correct, because they just turn
> a void return into one with a value. But this one is trickier:
>
>> @@ -2138,9 +2144,10 @@ void format_ref_array_item(struct ref_array_item *info,
>>               ep = strchr(sp, ')');
>>               if (cp < sp)
>>                       append_literal(cp, sp, &state);
>> -             get_ref_atom_value(info,
>> -                                parse_ref_filter_atom(format, sp + 2, ep),
>> -                                &atomv);
>> +             if (get_ref_atom_value(info,
>> +                                    parse_ref_filter_atom(format, sp + 2, ep),
>> +                                    &atomv))
>> +                     return -1;
>>               atomv->handler(atomv, &state);
>>       }
>
> since it affects the control flow. Might we be skipping any necessary
> cleanup in the function if we see an error?
>
> It looks like we may have called push_stack_element(), but we'd never
> get to the end of the function where we call pop_stack_element(),
> causing us to leak.

Agree,  I will fix this.

>
> -Peff

```

## Оля Тележная, 2018-02-15 10:03

Subject: Re: [PATCH v3 04/23] ref-filter: make valid_atom as function parameter
Message-ID: <CAL21BmmyjgzYEHm_VHJkU4KEH+uhCo4G3Au=KM54YAU3n2n_4w@mail.gmail.com>
URL: https://gitlist.dev/e/CAL21BmmyjgzYEHm_VHJkU4KEH%2BuhCo4G3Au%3DKM54YAU3n2n_4w%40mail.gmail.com
In-Reply-To: <20180215052312.GD18780@sigill.intra.peff.net>

```
2018-02-15 8:23 GMT+03:00 Jeff King <peff@peff.net>:
> On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:
>
>> Make valid_atom as a function parameter,
>> there could be another variable further.
>> Need that for further reusing of formatting logic in cat-file.c.
>>
>> We do not need to allow users to pass their own valid_atom variable in
>> global functions like verify_ref_format() because in the end we want to
>> have same set of valid atoms for all commands. But, as a first step
>> of migrating, I create further another version of valid_atom
>> for cat-file.
>
> Hmm. So I see where you're going here, but I think in the end we'd want
> to have a single valid_atom list again, and we wouldn't need this.
>
> And indeed, it looks like this goes away in patch 17. Can we
> reorganize/rebase the series so that we avoid dead-end directions like
> this?

I tried to do that, but in my opinion it's easier to understand
everything in current version. I need to squash too many items so that
commits do not look atomic, and it's really hard to understand what is
going on. Now we have that helper commit that will be cancelled later,
and other logic is more clear for readers.

>
> -Peff

```

## Оля Тележная, 2018-02-15 10:11

Subject: Re: [PATCH v3 08/23] ref-filter: reuse parse_ref_filter_atom()
Message-ID: <CAL21Bm=fJxWN9266PXni_SZn=T0t6_jT1kJMtPYo4UA41pj_ZQ@mail.gmail.com>
URL: https://gitlist.dev/e/CAL21Bm%3DfJxWN9266PXni_SZn%3DT0t6_jT1kJMtPYo4UA41pj_ZQ%40mail.gmail.com
In-Reply-To: <20180215053714.GH18780@sigill.intra.peff.net>

```
2018-02-15 8:37 GMT+03:00 Jeff King <peff@peff.net>:
> On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:
>
>> Continue migrating formatting logic from cat-file to ref-filter.
>> Reuse parse_ref_filter_atom() for unifying all processes in ref-filter
>> and further removing of mark_atom_in_object_info().
>
> OK, now it looks we're moving in a good direction.
>
> One thing that puzzles me:
>
>> @@ -401,20 +420,14 @@ static int is_atom(const char *atom, const char *s, int slen)
>>  static void mark_atom_in_object_info(const char *atom, int len,
>>                                   struct expand_data *data)
>>  {
>> -     if (is_atom("objectname", atom, len))
>> -             ; /* do nothing */
>> -     else if (is_atom("objecttype", atom, len))
>> +     if (is_atom("objecttype", atom, len))
>>               data->info.typep = &data->type;
>>       else if (is_atom("objectsize", atom, len))
>>               data->info.sizep = &data->size;
>> -     else if (is_atom("objectsize:disk", atom, len))
>> -             data->info.disk_sizep = &data->disk_size;
>>       else if (is_atom("rest", atom, len))
>>               data->split_on_whitespace = 1;
>>       else if (is_atom("deltabase", atom, len))
>>               data->info.delta_base_sha1 = data->delta_base_oid.hash;
>> -     else
>> -             die("unknown format element: %.*s", len, atom);
>>  }
>
> Why do some of these atoms go away and not others?

I deleted "objectname" because we were doing nothing there;
"objectsize:disk" because we have its own parser function;
"die" because ref-filter has its own checker whether the atom is valid or not.
I left all others because I haven't supported them at that point. This
whole function will be removed later.

> It seems like we're
> now relying on ref-filter to parse some of the common ones using its
> existing atom-parser. But wouldn't it have objecttype and objectsize
> already, then?

We haven't migrated enough to ref-filter at this point and we can't
reuse general ref-filter logic about filling the fields. So, we still
need to have our own function for doing that. Anyway, as I said
earlier, we will reach that status in the end of the patch: this
function would be deleted and we will use general ref-filter logic.

>
> -Peff

```

## Оля Тележная, 2018-02-15 10:13

Subject: Re: [PATCH v3 09/23] cat-file: start use ref_array_item struct
Message-ID: <CAL21Bm=+MAEfPRVKjp=3ktTHJLp=0Qf4rfLZNPGP4YfZRd2T6Q@mail.gmail.com>
URL: https://gitlist.dev/e/CAL21Bm%3D%2BMAEfPRVKjp%3D3ktTHJLp%3D0Qf4rfLZNPGP4YfZRd2T6Q%40mail.gmail.com
In-Reply-To: <20180215054018.GI18780@sigill.intra.peff.net>

```
2018-02-15 8:40 GMT+03:00 Jeff King <peff@peff.net>:
> On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:
>
>> Moving from using expand_data to ref_array_item structure.
>> That helps us to reuse functions from ref-filter easier.
>
> This one feels weird. The point of a ref_array_item is for the caller to
> feed data into the ref-filter formatting code (usually that data comes
> from an earlier call to filter_refs(), but in the new cat-file we'd
> presumably feed single items).
>
> But here we're adding a bunch of fields for items that we'd expect the
> format code to compute itself.

It would be changed later, it's just the addition of new structure
that we have never used in cat-file before.

>
> -Peff

```

## Оля Тележная, 2018-02-15 10:19

Subject: Re: [PATCH v3 13/23] ref-filter: get rid of mark_atom_in_object_info()
Message-ID: <CAL21BmmeFW08vqx7EA7MsGzAbZjMt-uraz1kqUWWt5aXpziqJQ@mail.gmail.com>
URL: https://gitlist.dev/e/CAL21BmmeFW08vqx7EA7MsGzAbZjMt-uraz1kqUWWt5aXpziqJQ%40mail.gmail.com
In-Reply-To: <20180215054542.GK18780@sigill.intra.peff.net>

```
2018-02-15 8:45 GMT+03:00 Jeff King <peff@peff.net>:
> On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:
>
>> Remove mark_atom_in_object_info() and create same logic
>> in terms of ref-filter style.
>
> This one is definitely a step in the right direction. In fact, this is
> what I would have expected to see in the beginning. It seems like this
> first half of the series really would benefit from being squashed into a
> few commits. I.e., I'd have expected to see the whole series looking
> something like:
>
>   - preparatory cleanup of ref-filter
>
>   - teach ref-filter any new atoms (or atom options) that cat-file knows
>     about but it doesn't
>
>   - convert cat-file to use ref-filter
>
> Most of what I've seen so far is basically that second step, but strung
> out along a bunch of commits. Can we compact it into a few commits that
> all make clear forward progress (by using "rebase -i" with "squash")?

I am afraid that I don't really see any 100% improvements by squashing
some of the commits. I tried to do that several times and I squashed
some of them, but now for me it looks like nothing else could be
squashed so that commits still look close to making atomic changes (I
am sure that it's super important).

>
> -Peff

```

## Оля Тележная, 2018-02-15 10:20

Subject: Re: [PATCH v3 14/23] ref_filter: add is_atom_used function
Message-ID: <CAL21Bm=7R2URkaRVCbM-T3Yxeq-S+E6Pq_ECSiBk_yJf9D1rag@mail.gmail.com>
URL: https://gitlist.dev/e/CAL21Bm%3D7R2URkaRVCbM-T3Yxeq-S%2BE6Pq_ECSiBk_yJf9D1rag%40mail.gmail.com
In-Reply-To: <20180215054945.GL18780@sigill.intra.peff.net>

```
2018-02-15 8:49 GMT+03:00 Jeff King <peff@peff.net>:
> On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:
>
>> Delete all items related to split_on_whitespace from ref-filter
>> and add new function for handling the logic.
>> Now cat-file could invoke that function to implementing its logic.
>
> OK, this is a good direction. I think in a more compact series we'd
> avoid moving the split-on-whitespace bits over to ref-filter in the
> first place, and have two commits:
>
>  - one early in the series adding is_atom_used()
>
>  - one late in the series switching cat-file over to is_atom_used() as
>    part of the conversion to ref-filter
>
>> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
>> index 6db57e3533806..3a49b55a1cc2e 100644
>> --- a/builtin/cat-file.c
>> +++ b/builtin/cat-file.c
>> @@ -382,8 +382,7 @@ static int batch_objects(struct batch_options *opt)
>>  {
>>       struct strbuf buf = STRBUF_INIT;
>>       struct expand_data data;
>> -     int save_warning;
>> -     int retval = 0;
>> +     int save_warning, is_rest, retval = 0;
>
> Try to avoid reformatting existing code that you're not otherwise
> touching, as it makes the diff noisier. Just adding "int is_rest" would
> make this easier to review.
>
> I also think the variable name should probably still be
> "split_on_whitespace". It's set based on whether we saw a "%(rest)"
> atom, but ultimately we'll use it to decide whether to split.

OK, I will fix that.

>
> -Peff

```

## Оля Тележная, 2018-02-15 10:27

Subject: Re: [PATCH v3 15/23] cat-file: move skip_object_info into ref-filter
Message-ID: <CAL21BmmvOHFve7FRfnRKuzsEm+Hkwo5AZUmqWrM-tmcbcLTSFQ@mail.gmail.com>
URL: https://gitlist.dev/e/CAL21BmmvOHFve7FRfnRKuzsEm%2BHkwo5AZUmqWrM-tmcbcLTSFQ%40mail.gmail.com
In-Reply-To: <20180215055158.GM18780@sigill.intra.peff.net>

```
2018-02-15 8:51 GMT+03:00 Jeff King <peff@peff.net>:
> On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:
>
>> Move logic related to skip_object_info into ref-filter,
>> so that cat-file does not use that field at all.
>
> I think this is going the wrong way. ref-filter should always do as
> little work as possible to fulfill the request. So it should skip the
> object_info call whenever it can. And then any callers who want to make
> sure that the object exists can do so (as long as the formatting code
> tells them whether it looked up the object or not).
>
> And then ref-filter doesn't have to know about this skip_object_info
> flag at all.

Your message looks contradictory to me.
I agree that ref-filter should do as least as it's possible, and that
is the main reason why I put this code there. Moreover, I think that
it's a good idea to implement that variable not only for cat-file, but
for all ref-filter callers. And I think that it's a task of ref-filter
to check whether the object exists or not (or - whether the ref is
valid or not). But I am not sure that I need to solve that moment in
current patch. It sounds like another separate task.

>
> -Peff

```

## Оля Тележная, 2018-02-15 10:33

Subject: Re: [PATCH v3 16/23] ref-filter: make cat_file_info independent
Message-ID: <CAL21BmmFaDstAT3Jm6AWh95mfWLJqN1MQF+pBTa3erX5w9MWRQ@mail.gmail.com>
URL: https://gitlist.dev/e/CAL21BmmFaDstAT3Jm6AWh95mfWLJqN1MQF%2BpBTa3erX5w9MWRQ%40mail.gmail.com
In-Reply-To: <20180215055334.GN18780@sigill.intra.peff.net>

```
2018-02-15 8:53 GMT+03:00 Jeff King <peff@peff.net>:
> On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:
>
>> Remove connection between expand_data variable
>> in cat-file and in ref-filter.
>> It will help further to get rid of using expand_data in cat-file.
>
> I have to admit I'm confused at this point about what is_cat_file is
> for, or even why we need cat_file_data. Shouldn't these items be handled
> by their matching ref-filter atoms at this point?

We discussed that earlier outside of mailing list, and I even tried to
implement that idea and spent a couple of days to prove that it's not
possible.
The problem is that the list of atoms is made dynamically, and we
can't store pointers to any values in each atom. That's why we need
separate cat_file_info variable that is outside of main atom list.
We also need is_cat_file because we still have some part of logic that
is different for cat-file and for all other commands, and sometimes we
need to know that information.

>
> -Peff

```

## Оля Тележная, 2018-02-15 10:34

Subject: Re: [PATCH v3 20/23] ref-filter: unifying formatting of cat-file opts
Message-ID: <CAL21BmkVgCzihUOxM=Mih=EzYKZwYzpgkSrAZFVr12nkfoCtXw@mail.gmail.com>
URL: https://gitlist.dev/e/CAL21BmkVgCzihUOxM%3DMih%3DEzYKZwYzpgkSrAZFVr12nkfoCtXw%40mail.gmail.com
In-Reply-To: <20180215055618.GP18780@sigill.intra.peff.net>

```
2018-02-15 8:56 GMT+03:00 Jeff King <peff@peff.net>:
> On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:
>
>> cat-file options are now filled by general logic.
>
> Yay.
>
> One puzzling thing:
>
>> diff --git a/ref-filter.c b/ref-filter.c
>> index 8d104b567eb7c..5781416cf9126 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -824,8 +824,12 @@ static void grab_common_values(struct atom_value *val, int deref, struct object
>>               else if (!strcmp(name, "objectsize")) {
>>                       v->value = sz;
>>                       v->s = xstrfmt("%lu", sz);
>> -             }
>> -             else if (deref)
>> +             } else if (!strcmp(name, "objectsize:disk")) {
>> +                     if (cat_file_info.is_cat_file) {
>> +                             v->value = cat_file_info.disk_size;
>> +                             v->s = xstrfmt("%"PRIuMAX, (uintmax_t)v->value);
>> +                     }
>> +             } else if (deref)
>
> Why do we care about is_cat_file here. Shouldn't:
>
>   git for-each-ref --format='%(objectsize:disk)'
>
> work? I.e., shouldn't the cat_file_info.disk_size variable be held
> somewhere in a used_atom struct?

At that point - no.
I think it sounds like other separate task to add this functionality
and to test it properly.

>
> -Peff

```

## Оля Тележная, 2018-02-15 10:38

Subject: Re: [PATCH v3 21/23] for-each-ref: tests for new atoms added
Message-ID: <CAL21Bmk6vcyKLDMdd9+Q-mt3huUEguO2fEaHFbce0hWNTE8h6g@mail.gmail.com>
URL: https://gitlist.dev/e/CAL21Bmk6vcyKLDMdd9%2BQ-mt3huUEguO2fEaHFbce0hWNTE8h6g%40mail.gmail.com
In-Reply-To: <20180215055711.GQ18780@sigill.intra.peff.net>

```
2018-02-15 8:57 GMT+03:00 Jeff King <peff@peff.net>:
> On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:
>
>> Add tests for new formatting atoms: rest, deltabase, objectsize:disk.
>> rest means nothing and we expand it into empty string.
>> We need this atom for cat-file command.
>> Have plans to support deltabase and objectsize:disk further
>> (as it is done in cat-file), now also expand it to empty string.
>
> I'm glad that you're adding tests, but I'm not sure it's a good idea to
> add tests checking for the thing we know to be wrong. If anything, you
> could be adding test_expect_failure looking for the _right_ thing, and
> accept that it does not yet work.

OK, I will try to fix that when other parts of the patch will look
good enough. I am not sure at this point that we will add my code to
the main version of the project, so these tests were written actually
for checking current functionality better before sending the code for
the review.

>
> -Peff

```

## Junio C Hamano, 2018-02-15 18:31

Subject: Re: [PATCH v3 20/23] ref-filter: unifying formatting of cat-file opts
Message-ID: <xmqq8tbu9ka2.fsf@gitster-ct.c.googlers.com>
URL: https://gitlist.dev/e/xmqq8tbu9ka2.fsf%40gitster-ct.c.googlers.com
In-Reply-To: <CAL21BmkVgCzihUOxM=Mih=EzYKZwYzpgkSrAZFVr12nkfoCtXw@mail.gmail.com>

```
Оля Тележная  <olyatelezhnaya@gmail.com> writes:

>>> -             else if (deref)
>>> +             } else if (!strcmp(name, "objectsize:disk")) {
>>> +                     if (cat_file_info.is_cat_file) {
>>> +                             v->value = cat_file_info.disk_size;
>>> +                             v->s = xstrfmt("%"PRIuMAX, (uintmax_t)v->value);
>>> +                     }
>>> +             } else if (deref)
>>
>> Why do we care about is_cat_file here. Shouldn't:
>>
>>   git for-each-ref --format='%(objectsize:disk)'
>>
>> work? I.e., shouldn't the cat_file_info.disk_size variable be held
>> somewhere in a used_atom struct?
>
> At that point - no.
> I think it sounds like other separate task to add this functionality
> and to test it properly.

What does "that point" refer to?  This point at 20th patch in the
23-patch series it is not premature, but it will become feasible in
later steps?

As Peff already said in his review on earlier steps like 4/23 and
7/23, I too found the series quite confusing and felt as if I was
watching somebody stumbling in all directions in the dark in the
earlier steps in the series before deciding to go in one direction.

```

## Adam Dinwoodie, 2018-02-16 14:55

Subject: Re: [PATCH v3 22/23] cat-file: tests for new atoms added
Message-ID: <CA+kUOamJowkxp0xAPf5FA+wBkiOjQeYzW1sKMwcatQBBA1qWpw@mail.gmail.com>
URL: https://gitlist.dev/e/CA%2BkUOamJowkxp0xAPf5FA%2BwBkiOjQeYzW1sKMwcatQBBA1qWpw%40mail.gmail.com
In-Reply-To: <01020161890f4388-f6427c9d-2be6-4628-afc9-bfaeb58b1ebe-000000@eu-west-1.amazonses.com>

```
On 12 February 2018 at 08:08, Olga Telezhnaya wrote:
> Add some tests for new formatting atoms from ref-filter.
> Some of new atoms are supported automatically,
> some of them are expanded into empty string
> (because they are useless for some types of objects),
> some of them could be supported later in other patches.

This commit appears to be introducing failures in t1006 on Cygwin.
Specifically, tests 15, 36, 58 and 89, all titled "Check %(refname)
gives empty output", are failing on the pu branch at 21937aad4, and
git bisect identifies this commit, 3c1571744 ("cat-file: tests for new
atoms added", 2018-02-12), as the culprit.

I'm afraid I'm not going to have the time to investigate the failure
any further in the immediate future, but I wanted to report it
promptly in case you / someone else can see what's going wrong.

Adam

```

## Jeff King, 2018-02-16 21:54

Subject: Re: [PATCH v3 16/23] ref-filter: make cat_file_info independent
Message-ID: <20180216215442.GA22270@sigill.intra.peff.net>
URL: https://gitlist.dev/e/20180216215442.GA22270%40sigill.intra.peff.net
In-Reply-To: <CAL21BmmFaDstAT3Jm6AWh95mfWLJqN1MQF+pBTa3erX5w9MWRQ@mail.gmail.com>

```
On Thu, Feb 15, 2018 at 01:33:25PM +0300, Оля Тележная wrote:

> 2018-02-15 8:53 GMT+03:00 Jeff King <peff@peff.net>:
> > On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:
> >
> >> Remove connection between expand_data variable
> >> in cat-file and in ref-filter.
> >> It will help further to get rid of using expand_data in cat-file.
> >
> > I have to admit I'm confused at this point about what is_cat_file is
> > for, or even why we need cat_file_data. Shouldn't these items be handled
> > by their matching ref-filter atoms at this point?
> 
> We discussed that earlier outside of mailing list, and I even tried to
> implement that idea and spent a couple of days to prove that it's not
> possible.
> The problem is that the list of atoms is made dynamically, and we
> can't store pointers to any values in each atom. That's why we need
> separate cat_file_info variable that is outside of main atom list.
> We also need is_cat_file because we still have some part of logic that
> is different for cat-file and for all other commands, and sometimes we
> need to know that information.

OK, right, I forgot about the pointers thing. This is definitely the
sort of design decision that should go into the commit message.

I went over our off-list discussion again to refresh my memory. Let me
try to summarize a bit (for myself, or for other readerss). Naively, one
would hope that you could do something like:

  1. While parsing the %(objectsize) atom, add a pointer like:

       the_object_info->sizep = &atom.size;

  2. Later when fulfilling the request for a specific object, if
     the_object_info is not empty, then call sha1_object_info() with it
     to hold the results.

  3. Read the value out of atom.size when assembling the formatted
     string.

But that has two problems:

  a. During parsing, we're resizing the atom array, so the &atom.size
     pointer is subject to change.  This is the issue you mentioned
     above. I think we could solve it by taking a pass over the atoms
     after parsing and filling in the pointers then. But...

  b. Another problem is that the same atom may appear multiple times. So
     parsing "%(objectsize) %(objectsize)", we can't point
     the_object_info at _both_ of them. We need our call to
     sha1_object_info() to store the result in a single location, and
     then as we format each atom they can both use that (which works
     even if they might format it differently, say if you had
     "%(deltabase) %(deltabase:abbrev)".

So we do need some kind of global storage to hold these results. This is
sort of the same problem we have with parsing the objects at all. There
we get by without a global because all of the logic is crammed into
populate_value(). In theory we could do the same thing here, but there
are just a lot of different items we might be asking for. So instead of
"need_tagged", you'd have a multitude of need_objectsize_disk,
need_deltabase, etc, which gets unwieldy. We may as well fill in a
"struct object_info".

So OK, I agree that something like "struct expand_data" is needed.

But what seems non-ideal to me is the way that ref-filter depends on it
in this cat-file-specific way. If it needs those values it should be
calling sha1_object_info(). And if it doesn't, then it can skip the
call. It shouldn't need to know about cat-file at all.

And likewise cat-file should not know about expand_data. And I think
that's true by the end of the series, though the steps in the middle
left me a little confused.

-Peff

```

## Ramsay Jones, 2018-02-18 22:55

Subject: Re: [PATCH v3 22/23] cat-file: tests for new atoms added
Message-ID: <58b2bdcd-d621-fd21-ab4d-6a9478319b19@ramsayjones.plus.com>
URL: https://gitlist.dev/e/58b2bdcd-d621-fd21-ab4d-6a9478319b19%40ramsayjones.plus.com
In-Reply-To: <CA+kUOamJowkxp0xAPf5FA+wBkiOjQeYzW1sKMwcatQBBA1qWpw@mail.gmail.com>

```


On 16/02/18 14:55, Adam Dinwoodie wrote:
> On 12 February 2018 at 08:08, Olga Telezhnaya wrote:
>> Add some tests for new formatting atoms from ref-filter.
>> Some of new atoms are supported automatically,
>> some of them are expanded into empty string
>> (because they are useless for some types of objects),
>> some of them could be supported later in other patches.
> 
> This commit appears to be introducing failures in t1006 on Cygwin.
> Specifically, tests 15, 36, 58 and 89, all titled "Check %(refname)
> gives empty output", are failing on the pu branch at 21937aad4, and
> git bisect identifies this commit, 3c1571744 ("cat-file: tests for new
> atoms added", 2018-02-12), as the culprit.

Hi Adam, Olga,

Sparse has been complaining about this 'ot/cat-batch-format' branch for
a while, so I had already noted (apart from a symbol which should be
marked as static) something that looked somewhat off - namely, the on
stack initialisation of a 'struct ref_array_item' (builtin/cat-file.c,
line 246, in function batch_object_write()).

In particular, the 'struct ref_array_item' ends with a [FLEX_ARRAY] field
for the refname, so it clearly isn't meant to be declared on the stack.
The 'ref-filter' code uses a 'constructor' function called 'new_ref_array_\
item() which, among others, takes a 'refname' parameter with which to
initialise the refname FLEX_ARRAY field.

So, on Linux, if you build with SANITIZE=address and then run that test:

  $ ./t1006-cat-file.sh -i -v
  Initialized empty Git repository in /home/ramsay/git/t/trash directory.t1006-cat-file/.git/
  expecting success: 
  	echo_without_newline "$hello_content" > hello &&
  	git update-index --add hello
  
  ok 1 - setup
  
  ...
  
  =================================================================
  ==12556==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7fff132f04a8 at pc 0x7f1c1b3ca20b bp 0x7fff132f00f0 sp 0x7fff132ef898
  READ of size 4 at 0x7fff132f04a8 thread T0
      #0 0x7f1c1b3ca20a in __interceptor_strlen (/usr/lib/x86_64-linux-gnu/libasan.so.2+0x7020a)
      #1 0x6798cc in strbuf_addstr /home/ramsay/git/strbuf.h:273
      #2 0x6798cc in quote_formatting /home/ramsay/git/ref-filter.c:496
      #3 0x68325d in format_ref_array_item /home/ramsay/git/ref-filter.c:2238
      #4 0x68358a in show_ref_array_item /home/ramsay/git/ref-filter.c:2262
      #5 0x429866 in batch_object_write builtin/cat-file.c:252
      #6 0x42b095 in batch_one_object builtin/cat-file.c:306
      #7 0x42b095 in batch_objects builtin/cat-file.c:407
      #8 0x42b095 in cmd_cat_file builtin/cat-file.c:528
      #9 0x40d3cb in run_builtin /home/ramsay/git/git.c:346
      #10 0x40d3cb in handle_builtin /home/ramsay/git/git.c:556
      #11 0x40f8ae in run_argv /home/ramsay/git/git.c:608
      #12 0x40f8ae in cmd_main /home/ramsay/git/git.c:685
      #13 0x40b667 in main /home/ramsay/git/common-main.c:43
      #14 0x7f1c1ab7982f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
      #15 0x40ce38 in _start (/home/ramsay/git/git+0x40ce38)
  
  Address 0x7fff132f04a8 is located in stack of thread T0 at offset 328 in frame
      #0 0x4296ff in batch_object_write builtin/cat-file.c:245
  
    This frame has 4 object(s):
      [32, 36) 'type'
      [96, 104) 'contents'
      [160, 168) 'size'
      [224, 328) 'item' <== Memory access at offset 328 overflows this variable
  HINT: this may be a false positive if your program uses some custom stack unwind mechanism or swapcontext
        (longjmp and C++ exceptions *are* supported)
  SUMMARY: AddressSanitizer: stack-buffer-overflow ??:0 __interceptor_strlen
  Shadow bytes around the buggy address:
    0x100062656040: 00 00 f3 f3 f3 f3 f3 f3 f3 f3 00 00 00 00 00 00
    0x100062656050: 00 00 00 00 f1 f1 f1 f1 00 00 00 f4 f3 f3 f3 f3
    0x100062656060: 00 00 00 00 00 00 00 00 00 00 00 00 f1 f1 f1 f1
    0x100062656070: 04 f4 f4 f4 f2 f2 f2 f2 00 f4 f4 f4 f2 f2 f2 f2
    0x100062656080: 00 f4 f4 f4 f2 f2 f2 f2 00 00 00 00 00 00 00 00
  =>0x100062656090: 00 00 00 00 00[f4]f4 f4 f3 f3 f3 f3 00 00 00 00
    0x1000626560a0: 00 00 00 00 00 00 00 00 00 00 00 00 f1 f1 f1 f1
    0x1000626560b0: 04 f4 f4 f4 f2 f2 f2 f2 04 f4 f4 f4 f2 f2 f2 f2
    0x1000626560c0: 04 f4 f4 f4 f2 f2 f2 f2 00 f4 f4 f4 f2 f2 f2 f2
    0x1000626560d0: 00 f4 f4 f4 f2 f2 f2 f2 00 00 f4 f4 f2 f2 f2 f2
    0x1000626560e0: 00 00 00 f4 f2 f2 f2 f2 00 00 00 f4 f2 f2 f2 f2
  Shadow byte legend (one shadow byte represents 8 application bytes):
    Addressable:           00
    Partially addressable: 01 02 03 04 05 06 07 
    Heap left redzone:       fa
    Heap right redzone:      fb
    Freed heap region:       fd
    Stack left redzone:      f1
    Stack mid redzone:       f2
    Stack right redzone:     f3
    Stack partial redzone:   f4
    Stack after return:      f5
    Stack use after scope:   f8
    Global redzone:          f9
    Global init order:       f6
    Poisoned by user:        f7
    Container overflow:      fc
    Array cookie:            ac
    Intra object redzone:    bb
    ASan internal:           fe
  ==12556==ABORTING
  Aborted (core dumped)
  not ok 15 - Check %(refname) gives empty output
  #	
  #		echo "$expected" >expect &&
  #		echo $sha1 | git cat-file --batch-check="$atoms" >actual &&
  #		test_cmp expect actual
  #	    
  $ 
 
... you get a stack-overflow at that very stackframe. Incidentally, the
output from the failed test #15 on cygwin always looked the same: 
  
  $ xxd actual
  00000000: a0c4 ffff 0a                             .....
  $ 
 
So, just as an experiment, I tried changing that variable to a heap
variable and initialising it with an empty ("") refname:
 
  diff --git a/builtin/cat-file.c b/builtin/cat-file.c
  index c9d83ceff..12a743034 100644
  --- a/builtin/cat-file.c
  +++ b/builtin/cat-file.c
  @@ -243,19 +243,20 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d
   static void batch_object_write(const char *obj_name, struct batch_options *opt,
   			       struct expand_data *data)
   {
  -	struct ref_array_item item = {0};
  +	struct ref_array_item *item;
   
  -	item.oid = data->oid;
  -	item.rest = data->rest;
  -	item.objectname = obj_name;
  +	FLEX_ALLOC_STR(item, refname, "");
  +	item->oid = data->oid;
  +	item->rest = data->rest;
  +	item->objectname = obj_name;
   
  -	if (show_ref_array_item(&item, &opt->format))
  +	if (show_ref_array_item(item, &opt->format))
   		return;
   	if (!opt->buffer_output)
   		fflush(stdout);
   
   	if (opt->print_contents) {
  -		data->type = item.type;
  +		data->type = item->type;
   		print_object_or_die(opt, data);
   		batch_write(opt, "\n", 1);
   	}
  -- 

... and now this test passes on cygwin (and the SANITIZE build on Linux).

Of course, this is not a real fix, since this has probably only changed
the stack-overflow into an un-diagnosed heap-overflow bug! ;-)

However, the above should provide enough info for someone more familiar
with the code to implement a correct fix.

[BTW, the symbol that should be marked static is: cat_file_info, in file
ref-filter.c, line 103.]

ATB,
Ramsay Jones



```

## Ramsay Jones, 2018-02-19 01:04

Subject: Re: [PATCH v3 22/23] cat-file: tests for new atoms added
Message-ID: <17c88c6d-c286-1857-965b-2d41f8f67bf4@ramsayjones.plus.com>
URL: https://gitlist.dev/e/17c88c6d-c286-1857-965b-2d41f8f67bf4%40ramsayjones.plus.com
In-Reply-To: <58b2bdcd-d621-fd21-ab4d-6a9478319b19@ramsayjones.plus.com>

```


On 18/02/18 22:55, Ramsay Jones wrote:
> 
> 
> On 16/02/18 14:55, Adam Dinwoodie wrote:
>> On 12 February 2018 at 08:08, Olga Telezhnaya wrote:
>>> Add some tests for new formatting atoms from ref-filter.
>>> Some of new atoms are supported automatically,
>>> some of them are expanded into empty string
>>> (because they are useless for some types of objects),
>>> some of them could be supported later in other patches.
>>
>> This commit appears to be introducing failures in t1006 on Cygwin.
>> Specifically, tests 15, 36, 58 and 89, all titled "Check %(refname)
>> gives empty output", are failing on the pu branch at 21937aad4, and
>> git bisect identifies this commit, 3c1571744 ("cat-file: tests for new
>> atoms added", 2018-02-12), as the culprit.
> 
> Hi Adam, Olga,
> 
> Sparse has been complaining about this 'ot/cat-batch-format' branch for
> a while, so I had already noted (apart from a symbol which should be
> marked as static) something that looked somewhat off - namely, the on
> stack initialisation of a 'struct ref_array_item' (builtin/cat-file.c,
> line 246, in function batch_object_write()).
> 
> In particular, the 'struct ref_array_item' ends with a [FLEX_ARRAY] field
> for the refname, so it clearly isn't meant to be declared on the stack.
> The 'ref-filter' code uses a 'constructor' function called 'new_ref_array_\
> item() which, among others, takes a 'refname' parameter with which to
> initialise the refname FLEX_ARRAY field.
> 
> So, on Linux, if you build with SANITIZE=address and then run that test:

BTW, I forgot to mention that, if you run that test without -i, then
more than tests #15,36,58 and 89 will fail. In addition, tests #19-20,
#40-41, #62-63 and #93-94 failed - I did not investigate these
failures at all.

ATB,
Ramsay Jones


```