[PATCH 01/20] cat-file: split expand_atom into 2 functions

[PATCH 01/20] cat-file: split expand_atom into 2 functions

[Olga Telezhnaya]

Split expand_atom function into 2 different functions,
expand_atom_into_fields prepares variable for further filling,
(new) expand_atom creates resulting string.
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 | 73 +++++++++++++++++++++++++++++-------------------------
 1 file changed, 39 insertions(+), 34 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index f5fa4fd75af26..f783b39b9bd5c 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -217,47 +217,49 @@ 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 expand_atom_into_fields(struct strbuf *sb, 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
+	} else if (is_atom("deltabase", atom, len))
+		strbuf_addstr(sb, oid_to_hex(&data->delta_base_oid));
+	else
 		die("unknown format element: %.*s", len, atom);
 }
 
-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;
@@ -265,7 +267,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)
+		expand_atom_into_fields(sb, 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/450

[PATCH 19/20] cat-file: move skip_object_info into ref-filter

[Olga Telezhnaya]

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 5aac10b9808ff..19cee0d22fabe 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -395,16 +395,11 @@ static int batch_objects(struct batch_options *opt)
 	opt->format->cat_file_data = &data;
 	opt->format->is_cat = 1;
 	opt->format->split_on_whitespace = &data.split_on_whitespace;
+	opt->format->all_objects = opt->all_objects;
 	verify_ref_format(opt->format);
 	if (opt->cmdmode)
 		data.split_on_whitespace = 1;
 
-	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 ee27edb2dad56..dbca6856432c0 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -766,6 +766,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 (is_cat && 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 3228661414623..7f7b17e659241 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -128,6 +128,7 @@ struct ref_format {
 	struct expand_data *cat_file_data;
 	int is_cat;
 	int *split_on_whitespace;
+	int all_objects;
 };
 
 #define REF_FORMAT_INIT { NULL, 0, -1 }

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

[PATCH 13/20] cat-file: start use ref_array_item struct

[Olga Telezhnaya]

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 | 30 +++++++++++++++++++-----------
 ref-filter.h       |  5 +++++
 2 files changed, 24 insertions(+), 11 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 1f331559e55c7..1c92194faaede 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -183,26 +183,26 @@ 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))
-		strbuf_addstr(sb, data->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;
@@ -210,7 +210,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;
 }
 
@@ -282,6 +282,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;
 
 	if (!data->skip_object_info &&
 	    sha1_object_info_extended(data->oid.hash, &data->info,
@@ -292,7 +293,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 de3fd3263ac64..28774e8e0f771 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 *values;
+	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/450

[PATCH 07/20] cat-file: reuse parse_ref_filter_atom

[Olga Telezhnaya]

Continue migrating formatting logic from cat-file to ref-filter.
Reuse parse_ref_filter_atom for unifying all processes in ref-filter
and further reducing of expand_atom_into_fields function.

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 | 73 +++++++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 45 insertions(+), 28 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 98bb10185ae96..2bd9a8fc4e39c 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -100,6 +100,7 @@ static struct used_atom {
 	} u;
 } *used_atoms;
 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_atoms[] = {
+	{ "objectname" },
+	{ "objecttype" },
+	{ "objectsize", FIELD_ULONG, objectsize_atom_parser },
+	{ "rest" },
+	{ "deltabase" },
+};
+
 #define REF_FORMATTING_STATE_INIT  { 0, NULL }
 
 struct ref_formatting_stack {
@@ -392,6 +411,25 @@ 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 expand_atom_into_fields(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
  */
@@ -458,6 +496,8 @@ static int parse_ref_filter_atom(const struct ref_format *format,
 		need_tagged = 1;
 	if (!strcmp(valid_atoms[i].name, "symref"))
 		need_symref = 1;
+	if (cat_file_info)
+		expand_atom_into_fields(atom, atom_len, cat_file_info);
 	return at;
 }
 
@@ -693,31 +733,6 @@ static const char *find_next(const char *cp)
 	return NULL;
 }
 
-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_into_fields(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);
-}
-
 /*
  * Make sure the format string is well formed, and parse out
  * the used atoms.
@@ -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,9 +752,10 @@ int verify_ref_format(struct ref_format *format)
 		/* sp points at "%(" and ep points at the closing ")" */
 
 		if (format->cat_file_data)
-			expand_atom_into_fields(sp + 2, ep - sp - 2,
-						format->cat_file_data);
-		else
+		{
+			at = parse_ref_filter_atom(format, valid_cat_file_atoms,
+						   ARRAY_SIZE(valid_cat_file_atoms), sp + 2, ep);
+		} else
 		{
 			at = parse_ref_filter_atom(format, valid_atoms,
 						   ARRAY_SIZE(valid_atoms), sp + 2, ep);

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

Re: [PATCH 07/20] cat-file: reuse parse_ref_filter_atom

[Junio C Hamano]

Olga Telezhnaya <olyatelezhnaya@gmail.com> writes:

> +static int is_atom(const char *atom, const char *s, int slen)
> +{
> +...
> +}
> +
> +static void expand_atom_into_fields(const char *atom, int len,
> +				    struct expand_data *data)
> +{
> +...
> +}
> ...
> -static int is_atom(const char *atom, const char *s, int slen)
> -{
> -...
> -}
> -
> -static void expand_atom_into_fields(const char *atom, int len,
> -				    struct expand_data *data)
> -{
> -...
> -}
> -
>  /*
>   * Make sure the format string is well formed, and parse out
>   * the used atoms.

There is no need to move these in this step if the previous step
planned ahead well, and that is something you can correct by
pretending to be perfect with "rebase -i" ;-) That also will help
reduce reviewers' load.

> @@ -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,9 +752,10 @@ int verify_ref_format(struct ref_format *format)
>  		/* sp points at "%(" and ep points at the closing ")" */
>  
>  		if (format->cat_file_data)
> -			expand_atom_into_fields(sp + 2, ep - sp - 2,
> -						format->cat_file_data);
> -		else
> +		{
> +			at = parse_ref_filter_atom(format, valid_cat_file_atoms,
> +						   ARRAY_SIZE(valid_cat_file_atoms), sp + 2, ep);
> +		} else
>  		{
>  			at = parse_ref_filter_atom(format, valid_atoms,
>  						   ARRAY_SIZE(valid_atoms), sp + 2, ep);

if/else with bodies that are compound statements are formatted like
so:

	if (condition) {
		do this;
	} else {
		do that;
		do that, too;
	}

[PATCH 04/20] cat-file: make valid_atoms as a function parameter

[Olga Telezhnaya]

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

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 | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 1426f0c28bce7..2d6e81fe1ab00 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -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_atoms, 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_atoms); i++) {
+	for (i = 0; i < n_atoms; i++) {
 		int len = strlen(valid_atoms[i].name);
 		if (len == atom_len && !memcmp(valid_atoms[i].name, sp, len))
 			break;
 	}
 
-	if (ARRAY_SIZE(valid_atoms) <= 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_atoms,
+					   ARRAY_SIZE(valid_atoms), sp + 2, ep);
 		cp = ep + 1;
 
 		if (skip_prefix(used_atoms[at].name, "color:", &color))
@@ -2139,7 +2141,9 @@ void format_ref_array_item(struct ref_array_item *info,
 		if (cp < sp)
 			append_literal(cp, sp, &state);
 		get_ref_atom_value(info,
-				   parse_ref_filter_atom(format, sp + 2, ep),
+				   parse_ref_filter_atom(format, valid_atoms,
+							 ARRAY_SIZE(valid_atoms),
+							 sp + 2, ep),
 				   &atomv);
 		atomv->handler(atomv, &state);
 	}
@@ -2187,7 +2191,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_atoms,
+				     ARRAY_SIZE(valid_atoms), atom, end);
 }
 
 /*  If no sorting option is given, use refname to sort as default */

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

Re: [PATCH 04/20] cat-file: make valid_atoms as a function parameter

[Junio C Hamano]

Olga Telezhnaya <olyatelezhnaya@gmail.com> writes:

> Make valid_atoms as a function parameter,
> there could be another variable further.
> Need that for further reusing of formatting logic in cat-file.c.
>
> 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 | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)

Please be a bit more careful about your title.  This change does not
seem to have anything to do with "cat-file".

I was not sure what you meant by "make X as a function parameter"
after reading the proposed log message twice, but I am guessing that
you are allowing these functions to operate on not just the global
singleton but a different instance of array.  

I also suspect that this step may make the references to the
valid_atom[] array somewhat inconsistent in that the functions that
are touched by this patch would refer to the passed-in array while
the remainder of the existing code still works on the global
singleton.  For example, parse_ref_filter_atom() is called by
verify_ref_format(), but this patch does not make _it_ take the
array as a parameter, and instead uses the global singleton, so
anybody who wants to use verify_ref_format() with different
valid_atom[] are SOL.  I am not saying that this inconsistency will
be a problem, but that the patch (including the proposed log
message) does not explain why it is not---and it should.

Thanks.

> diff --git a/ref-filter.c b/ref-filter.c
> index 1426f0c28bce7..2d6e81fe1ab00 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -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_atoms, 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_atoms); i++) {
> +	for (i = 0; i < n_atoms; i++) {
>  		int len = strlen(valid_atoms[i].name);
>  		if (len == atom_len && !memcmp(valid_atoms[i].name, sp, len))
>  			break;
>  	}
>  
> -	if (ARRAY_SIZE(valid_atoms) <= 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_atoms,
> +					   ARRAY_SIZE(valid_atoms), sp + 2, ep);
>  		cp = ep + 1;
>  
>  		if (skip_prefix(used_atoms[at].name, "color:", &color))
> @@ -2139,7 +2141,9 @@ void format_ref_array_item(struct ref_array_item *info,
>  		if (cp < sp)
>  			append_literal(cp, sp, &state);
>  		get_ref_atom_value(info,
> -				   parse_ref_filter_atom(format, sp + 2, ep),
> +				   parse_ref_filter_atom(format, valid_atoms,
> +							 ARRAY_SIZE(valid_atoms),
> +							 sp + 2, ep),
>  				   &atomv);
>  		atomv->handler(atomv, &state);
>  	}
> @@ -2187,7 +2191,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_atoms,
> +				     ARRAY_SIZE(valid_atoms), atom, end);
>  }
>  
>  /*  If no sorting option is given, use refname to sort as default */
>
> --
> https://github.com/git/git/pull/450

[PATCH 11/20] cat-file: get rid of duplicate checking

[Olga Telezhnaya]

We could remove this because we have already checked that
at verify_ref_format function in 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 | 2 --
 1 file changed, 2 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index dd43457c0ad7e..1f331559e55c7 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -197,8 +197,6 @@ static void expand_atom(struct strbuf *sb, const char *atom, int len,
 		strbuf_addstr(sb, data->rest);
 	else if (is_atom("deltabase", atom, len))
 		strbuf_addstr(sb, oid_to_hex(&data->delta_base_oid));
-	else
-		die("unknown format element: %.*s", len, atom);
 }
 
 static size_t expand_format(struct strbuf *sb, const char *start, void *vdata)

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

[PATCH 17/20] cat-file: add is_cat flag in ref-filter

[Olga Telezhnaya]

Add is_cat flag, further it helps to get rid of cat_file_data field
in ref_format.

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 | 1 +
 ref-filter.c       | 8 +++++---
 ref-filter.h       | 1 +
 3 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index e11dbf88e386c..289912ab1f858 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -393,6 +393,7 @@ static int batch_objects(struct batch_options *opt)
 	 */
 	memset(&data, 0, sizeof(data));
 	opt->format->cat_file_data = &data;
+	opt->format->is_cat = 1;
 	verify_ref_format(opt->format);
 	if (opt->cmdmode)
 		data.split_on_whitespace = 1;
diff --git a/ref-filter.c b/ref-filter.c
index 112336edbe871..4259c9b9cd767 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -101,6 +101,7 @@ static struct used_atom {
 } *used_atoms;
 static int used_atom_cnt, need_tagged, need_symref;
 struct expand_data *cat_file_info;
+static int is_cat = 0;
 
 static void color_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *color_value)
 {
@@ -493,7 +494,7 @@ static int parse_ref_filter_atom(const struct ref_format *format,
 		need_tagged = 1;
 	if (!strcmp(valid_atoms[i].name, "symref"))
 		need_symref = 1;
-	if (cat_file_info && !strcmp(valid_atoms[i].name, "rest"))
+	if (is_cat && !strcmp(valid_atoms[i].name, "rest"))
 		cat_file_info->split_on_whitespace = 1;
 	return at;
 }
@@ -739,6 +740,7 @@ int verify_ref_format(struct ref_format *format)
 	const char *cp, *sp;
 
 	cat_file_info = format->cat_file_data;
+	is_cat = format->is_cat;
 	format->need_color_reset_at_eol = 0;
 	for (cp = format->format; *cp && (sp = find_next(cp)); ) {
 		const char *color, *ep = strchr(sp, ')');
@@ -748,7 +750,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 (is_cat)
 		{
 			at = parse_ref_filter_atom(format, valid_cat_file_atoms,
 						   ARRAY_SIZE(valid_cat_file_atoms), sp + 2, ep);
@@ -1466,7 +1468,7 @@ int populate_value(struct ref_array_item *ref)
 			ref->symref = "";
 	}
 
-	if (cat_file_info) {
+	if (is_cat) {
 		if (!cat_file_info->skip_object_info &&
 		    sha1_object_info_extended(ref->objectname.hash, &cat_file_info->info,
 					      OBJECT_INFO_LOOKUP_REPLACE) < 0) {
diff --git a/ref-filter.h b/ref-filter.h
index 9e4444cf3ef9a..5ba4724a472f6 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -126,6 +126,7 @@ struct ref_format {
 	 * hopefully would be reduced later.
 	 */
 	struct expand_data *cat_file_data;
+	int is_cat;
 };
 
 #define REF_FORMAT_INIT { NULL, 0, -1 }

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

[PATCH 16/20] cat-file: get rid of expand_atom_into_fields

[Olga Telezhnaya]

Remove expand_atom_into_fields function 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 7df6d7e3d6511..112336edbe871 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_atoms[] = {
 	{ "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 expand_atom_into_fields(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_atoms[i].name, "symref"))
 		need_symref = 1;
-	if (cat_file_info)
-		expand_atom_into_fields(atom, atom_len, cat_file_info);
+	if (cat_file_info && !strcmp(valid_atoms[i].name, "rest"))
+		cat_file_info->split_on_whitespace = 1;
 	return at;
 }
 

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

[PATCH 18/20] cat-file: add split_on_whitespace flag

[Olga Telezhnaya]

Add flag to ref_format struct so that we could pass needed info
to 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 | 1 +
 ref-filter.c       | 4 ++--
 ref-filter.h       | 1 +
 3 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 289912ab1f858..5aac10b9808ff 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -394,6 +394,7 @@ static int batch_objects(struct batch_options *opt)
 	memset(&data, 0, sizeof(data));
 	opt->format->cat_file_data = &data;
 	opt->format->is_cat = 1;
+	opt->format->split_on_whitespace = &data.split_on_whitespace;
 	verify_ref_format(opt->format);
 	if (opt->cmdmode)
 		data.split_on_whitespace = 1;
diff --git a/ref-filter.c b/ref-filter.c
index 4259c9b9cd767..ee27edb2dad56 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -494,8 +494,8 @@ static int parse_ref_filter_atom(const struct ref_format *format,
 		need_tagged = 1;
 	if (!strcmp(valid_atoms[i].name, "symref"))
 		need_symref = 1;
-	if (is_cat && !strcmp(valid_atoms[i].name, "rest"))
-		cat_file_info->split_on_whitespace = 1;
+	if (!strcmp(valid_atoms[i].name, "rest"))
+		*format->split_on_whitespace = 1;
 	return at;
 }
 
diff --git a/ref-filter.h b/ref-filter.h
index 5ba4724a472f6..3228661414623 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -127,6 +127,7 @@ struct ref_format {
 	 */
 	struct expand_data *cat_file_data;
 	int is_cat;
+	int *split_on_whitespace;
 };
 
 #define REF_FORMAT_INIT { NULL, 0, -1 }

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

[PATCH 20/20] cat-file: make cat_file_info variable independent

[Olga Telezhnaya]

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 |  2 +-
 ref-filter.c       | 28 ++++++++++++++--------------
 ref-filter.h       |  1 -
 3 files changed, 15 insertions(+), 16 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 19cee0d22fabe..ffa8e61213e36 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -289,6 +289,7 @@ static void batch_object_write(const char *obj_name, struct batch_options *opt,
 	item.start_of_request = obj_name;
 
 	if (populate_value(&item)) return;
+	data->type = item.type;
 
 	strbuf_expand(&buf, opt->format->format, expand_format, &item);
 	strbuf_addch(&buf, '\n');
@@ -392,7 +393,6 @@ static int batch_objects(struct batch_options *opt)
 	 * object.
 	 */
 	memset(&data, 0, sizeof(data));
-	opt->format->cat_file_data = &data;
 	opt->format->is_cat = 1;
 	opt->format->split_on_whitespace = &data.split_on_whitespace;
 	opt->format->all_objects = opt->all_objects;
diff --git a/ref-filter.c b/ref-filter.c
index dbca6856432c0..a8a93b488db37 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -100,7 +100,7 @@ static struct used_atom {
 	} u;
 } *used_atoms;
 static int used_atom_cnt, need_tagged, need_symref;
-struct expand_data *cat_file_info;
+struct expand_data cat_file_info;
 static int is_cat = 0;
 
 static void color_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *color_value)
@@ -256,9 +256,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);
 }
@@ -266,7 +266,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);
 }
@@ -274,7 +274,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);
 }
@@ -739,7 +739,6 @@ int verify_ref_format(struct ref_format *format)
 {
 	const char *cp, *sp;
 
-	cat_file_info = format->cat_file_data;
 	is_cat = format->is_cat;
 	format->need_color_reset_at_eol = 0;
 	for (cp = format->format; *cp && (sp = find_next(cp)); ) {
@@ -768,8 +767,8 @@ int verify_ref_format(struct ref_format *format)
 		format->need_color_reset_at_eol = 0;
 	if (is_cat && 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;
 }
@@ -1474,18 +1473,19 @@ int populate_value(struct ref_array_item *ref)
 	}
 
 	if (is_cat) {
-		if (!cat_file_info->skip_object_info &&
-		    sha1_object_info_extended(ref->objectname.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->objectname.hash, &cat_file_info.info,
 					      OBJECT_INFO_LOOKUP_REPLACE) < 0) {
 			const char *e = ref->start_of_request;
 			printf("%s missing\n", e ? e : oid_to_hex(&ref->objectname));
 			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;
 	}
 
 	/* Fill in specials first */
diff --git a/ref-filter.h b/ref-filter.h
index 7f7b17e659241..ebaed0412fac2 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -125,7 +125,6 @@ struct ref_format {
 	 * Helps to move all formatting logic from cat-file to ref-filter,
 	 * hopefully would be reduced later.
 	 */
-	struct expand_data *cat_file_data;
 	int is_cat;
 	int *split_on_whitespace;
 	int all_objects;

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

[PATCH 06/20] cat-file: start migrating to ref-filter

[Olga Telezhnaya]

Start moving all formatting stuff from cat-file to ref-filter.
Start from simple moving, it would be integrated into
all ref-filter processes 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 |  5 ++---
 ref-filter.c       | 42 +++++++++++++++++++++++++++++++++++++-----
 ref-filter.h       |  6 ++++++
 3 files changed, 45 insertions(+), 8 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 7fd5b960ad698..0a3f4a47bf1ae 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -420,9 +420,8 @@ static int batch_objects(struct batch_options *opt)
 	 * 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 2d6e81fe1ab00..98bb10185ae96 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -693,6 +693,31 @@ static const char *find_next(const char *cp)
 	return NULL;
 }
 
+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_into_fields(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);
+}
+
 /*
  * Make sure the format string is well formed, and parse out
  * the used atoms.
@@ -709,12 +734,19 @@ 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_atoms,
-					   ARRAY_SIZE(valid_atoms), sp + 2, ep);
-		cp = ep + 1;
 
-		if (skip_prefix(used_atoms[at].name, "color:", &color))
-			format->need_color_reset_at_eol = !!strcmp(color, "reset");
+		if (format->cat_file_data)
+			expand_atom_into_fields(sp + 2, ep - sp - 2,
+						format->cat_file_data);
+		else
+		{
+			at = parse_ref_filter_atom(format, valid_atoms,
+						   ARRAY_SIZE(valid_atoms), sp + 2, ep);
+			if (skip_prefix(used_atoms[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 16d00e4b1bded..97169548939cd 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -119,6 +119,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/450

[PATCH 05/20] cat-file: move struct expand_data into ref-filter

[Olga Telezhnaya]

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 7655a9a726773..7fd5b960ad698 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 0d98342b34319..16d00e4b1bded 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/450

[PATCH 12/20] cat-file: rename variable in ref-filter

[Olga Telezhnaya]

Rename variable for easier reading.
It points not to values, but to arrays.
Added "s" ending so that it could be obvious.

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

diff --git a/ref-filter.c b/ref-filter.c
index 1b8c8787190a9..56d2687e07df9 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1418,7 +1418,7 @@ static void need_object(struct ref_array_item *ref) {
 		die(_("parse_object_buffer failed on %s for %s"),
 		    oid_to_hex(&ref->objectname), ref->refname);
 
-	grab_values(ref->value, 0, obj, buf, size);
+	grab_values(ref->values, 0, obj, buf, size);
 	if (!eaten)
 		free(buf);
 
@@ -1448,7 +1448,7 @@ static void need_object(struct ref_array_item *ref) {
 	if (!obj)
 		die(_("parse_object_buffer failed on %s for %s"),
 		    oid_to_hex(tagged), ref->refname);
-	grab_values(ref->value, 1, obj, buf, size);
+	grab_values(ref->values, 1, obj, buf, size);
 	if (!eaten)
 		free(buf);
 }
@@ -1460,7 +1460,7 @@ static void populate_value(struct ref_array_item *ref)
 {
 	int i;
 
-	ref->value = xcalloc(used_atom_cnt, sizeof(struct atom_value));
+	ref->values = xcalloc(used_atom_cnt, sizeof(struct atom_value));
 
 	if (need_symref && (ref->flag & REF_ISSYMREF) && !ref->symref) {
 		ref->symref = resolve_refdup(ref->refname, RESOLVE_REF_READING,
@@ -1473,7 +1473,7 @@ static void populate_value(struct ref_array_item *ref)
 	for (i = 0; i < used_atom_cnt; i++) {
 		struct used_atom *atom = &used_atoms[i];
 		const char *name = used_atoms[i].name;
-		struct atom_value *v = &ref->value[i];
+		struct atom_value *v = &ref->values[i];
 		int deref = 0;
 		const char *refname;
 		struct branch *branch = NULL;
@@ -1571,7 +1571,7 @@ static void populate_value(struct ref_array_item *ref)
 	}
 
 	for (i = 0; i < used_atom_cnt; i++) {
-		struct atom_value *v = &ref->value[i];
+		struct atom_value *v = &ref->values[i];
 		if (v->s == NULL) {
 			need_object(ref);
 			break;
@@ -1586,11 +1586,11 @@ static void populate_value(struct ref_array_item *ref)
  */
 static void get_ref_atom_value(struct ref_array_item *ref, int atom, struct atom_value **v)
 {
-	if (!ref->value) {
+	if (!ref->values) {
 		populate_value(ref);
-		fill_missing_values(ref->value);
+		fill_missing_values(ref->values);
 	}
-	*v = &ref->value[atom];
+	*v = &ref->values[atom];
 }
 
 /*
diff --git a/ref-filter.h b/ref-filter.h
index 590a60ffe034d..de3fd3263ac64 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -39,7 +39,7 @@ struct ref_array_item {
 	unsigned int kind;
 	const char *symref;
 	struct commit *commit;
-	struct atom_value *value;
+	struct atom_value *values;
 	char refname[FLEX_ARRAY];
 };
 

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

[PATCH 02/20] cat-file: reuse struct ref_format

[Olga Telezhnaya]

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

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index f783b39b9bd5c..7655a9a726773 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;
@@ -353,7 +354,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);
@@ -446,8 +447,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
@@ -456,7 +457,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;
@@ -548,7 +549,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;
 }
@@ -557,7 +558,8 @@ 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 ref_format format = REF_FORMAT_INIT;
+	struct batch_options batch = {&format};
 	int unknown_type = 0;
 
 	const struct option options[] = {

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

[PATCH 03/20] cat-file: rename variables in ref-filter

[Olga Telezhnaya]

Rename some variables for easier reading.
They point not to values, but to arrays.
Added "s" ending so that it could be obvious.

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

diff --git a/ref-filter.c b/ref-filter.c
index 3f9161707e66b..1426f0c28bce7 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -98,7 +98,7 @@ static struct used_atom {
 		struct refname_atom refname;
 		char *head;
 	} u;
-} *used_atom;
+} *used_atoms;
 static int used_atom_cnt, need_tagged, need_symref;
 
 static void color_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *color_value)
@@ -325,11 +325,11 @@ 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);
-} valid_atom[] = {
+} valid_atoms[] = {
 	{ "refname" , FIELD_STR, refname_atom_parser },
 	{ "objecttype" },
 	{ "objectsize", FIELD_ULONG },
@@ -410,38 +410,38 @@ static int parse_ref_filter_atom(const struct ref_format *format,
 
 	/* Do we have the atom already used elsewhere? */
 	for (i = 0; i < used_atom_cnt; i++) {
-		int len = strlen(used_atom[i].name);
-		if (len == ep - atom && !memcmp(used_atom[i].name, atom, len))
+		int len = strlen(used_atoms[i].name);
+		if (len == ep - atom && !memcmp(used_atoms[i].name, atom, len))
 			return i;
 	}
 
 	/*
 	 * If the atom name has a colon, strip it and everything after
 	 * it off - it specifies the format for this entry, and
-	 * shouldn't be used for checking against the valid_atom
+	 * shouldn't be used for checking against the valid_atoms
 	 * table.
 	 */
 	arg = memchr(sp, ':', ep - sp);
 	atom_len = (arg ? arg : ep) - sp;
 
 	/* Is the atom a valid one? */
-	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))
+	for (i = 0; i < ARRAY_SIZE(valid_atoms); i++) {
+		int len = strlen(valid_atoms[i].name);
+		if (len == atom_len && !memcmp(valid_atoms[i].name, sp, len))
 			break;
 	}
 
-	if (ARRAY_SIZE(valid_atom) <= i)
+	if (ARRAY_SIZE(valid_atoms) <= i)
 		die(_("unknown field name: %.*s"), (int)(ep-atom), atom);
 
 	/* Add it in, including the deref prefix */
 	at = used_atom_cnt;
 	used_atom_cnt++;
-	REALLOC_ARRAY(used_atom, used_atom_cnt);
-	used_atom[at].name = xmemdupz(atom, ep - atom);
-	used_atom[at].type = valid_atom[i].cmp_type;
+	REALLOC_ARRAY(used_atoms, used_atom_cnt);
+	used_atoms[at].name = xmemdupz(atom, ep - atom);
+	used_atoms[at].type = valid_atoms[i].cmp_type;
 	if (arg) {
-		arg = used_atom[at].name + (arg - atom) + 1;
+		arg = used_atoms[at].name + (arg - atom) + 1;
 		if (!*arg) {
 			/*
 			 * Treat empty sub-arguments list as NULL (i.e.,
@@ -450,12 +450,12 @@ static int parse_ref_filter_atom(const struct ref_format *format,
 			arg = NULL;
 		}
 	}
-	memset(&used_atom[at].u, 0, sizeof(used_atom[at].u));
-	if (valid_atom[i].parser)
-		valid_atom[i].parser(format, &used_atom[at], arg);
+	memset(&used_atoms[at].u, 0, sizeof(used_atoms[at].u));
+	if (valid_atoms[i].parser)
+		valid_atoms[i].parser(format, &used_atoms[at], arg);
 	if (*atom == '*')
 		need_tagged = 1;
-	if (!strcmp(valid_atom[i].name, "symref"))
+	if (!strcmp(valid_atoms[i].name, "symref"))
 		need_symref = 1;
 	return at;
 }
@@ -711,7 +711,7 @@ int verify_ref_format(struct ref_format *format)
 		at = parse_ref_filter_atom(format, sp + 2, ep);
 		cp = ep + 1;
 
-		if (skip_prefix(used_atom[at].name, "color:", &color))
+		if (skip_prefix(used_atoms[at].name, "color:", &color))
 			format->need_color_reset_at_eol = !!strcmp(color, "reset");
 	}
 	if (format->need_color_reset_at_eol && !want_color(format->use_color))
@@ -762,7 +762,7 @@ static void grab_common_values(struct atom_value *val, int deref, struct object
 	int i;
 
 	for (i = 0; i < used_atom_cnt; i++) {
-		const char *name = used_atom[i].name;
+		const char *name = used_atoms[i].name;
 		struct atom_value *v = &val[i];
 		if (!!deref != (*name == '*'))
 			continue;
@@ -775,7 +775,7 @@ static void grab_common_values(struct atom_value *val, int deref, struct object
 			v->s = xstrfmt("%lu", sz);
 		}
 		else if (deref)
-			grab_objectname(name, obj->oid.hash, v, &used_atom[i]);
+			grab_objectname(name, obj->oid.hash, v, &used_atoms[i]);
 	}
 }
 
@@ -786,7 +786,7 @@ static void grab_tag_values(struct atom_value *val, int deref, struct object *ob
 	struct tag *tag = (struct tag *) obj;
 
 	for (i = 0; i < used_atom_cnt; i++) {
-		const char *name = used_atom[i].name;
+		const char *name = used_atoms[i].name;
 		struct atom_value *v = &val[i];
 		if (!!deref != (*name == '*'))
 			continue;
@@ -808,7 +808,7 @@ static void grab_commit_values(struct atom_value *val, int deref, struct object
 	struct commit *commit = (struct commit *) obj;
 
 	for (i = 0; i < used_atom_cnt; i++) {
-		const char *name = used_atom[i].name;
+		const char *name = used_atoms[i].name;
 		struct atom_value *v = &val[i];
 		if (!!deref != (*name == '*'))
 			continue;
@@ -938,7 +938,7 @@ static void grab_person(const char *who, struct atom_value *val, int deref, stru
 	const char *wholine = NULL;
 
 	for (i = 0; i < used_atom_cnt; i++) {
-		const char *name = used_atom[i].name;
+		const char *name = used_atoms[i].name;
 		struct atom_value *v = &val[i];
 		if (!!deref != (*name == '*'))
 			continue;
@@ -976,7 +976,7 @@ static void grab_person(const char *who, struct atom_value *val, int deref, stru
 	if (!wholine)
 		return;
 	for (i = 0; i < used_atom_cnt; i++) {
-		const char *name = used_atom[i].name;
+		const char *name = used_atoms[i].name;
 		struct atom_value *v = &val[i];
 		if (!!deref != (*name == '*'))
 			continue;
@@ -1066,7 +1066,7 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj
 	unsigned long sublen = 0, bodylen = 0, nonsiglen = 0, siglen = 0;
 
 	for (i = 0; i < used_atom_cnt; i++) {
-		struct used_atom *atom = &used_atom[i];
+		struct used_atom *atom = &used_atoms[i];
 		const char *name = atom->name;
 		struct atom_value *v = &val[i];
 		if (!!deref != (*name == '*'))
@@ -1127,7 +1127,7 @@ static void fill_missing_values(struct atom_value *val)
 
 /*
  * val is a list of atom_value to hold returned values.  Extract
- * the values for atoms in used_atom array out of (obj, buf, sz).
+ * the values for atoms in used_atoms array out of (obj, buf, sz).
  * when deref is false, (obj, buf, sz) is the object that is
  * pointed at by the ref itself; otherwise it is the object the
  * ref (which is a tag) refers to.
@@ -1376,8 +1376,8 @@ static void populate_value(struct ref_array_item *ref)
 
 	/* Fill in specials first */
 	for (i = 0; i < used_atom_cnt; i++) {
-		struct used_atom *atom = &used_atom[i];
-		const char *name = used_atom[i].name;
+		struct used_atom *atom = &used_atoms[i];
+		const char *name = used_atoms[i].name;
 		struct atom_value *v = &ref->value[i];
 		int deref = 0;
 		const char *refname;
@@ -2059,7 +2059,7 @@ static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, stru
 {
 	struct atom_value *va, *vb;
 	int cmp;
-	cmp_type cmp_type = used_atom[s->atom].type;
+	cmp_type cmp_type = used_atoms[s->atom].type;
 	int (*cmp_fn)(const char *, const char *);
 
 	get_ref_atom_value(a, s->atom, &va);

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

Re: [PATCH 03/20] cat-file: rename variables in ref-filter

[Junio C Hamano]

Olga Telezhnaya <olyatelezhnaya@gmail.com> writes:

> Rename some variables for easier reading.
> They point not to values, but to arrays.

Once the code is written and people start to build on top, a change
like this is not worth the code churn, especially because there are
two equally valid schools of naming convention.

 - When you have an array, each of whose 20 slots holds a single
   dosh, I would prefer to call the array dosh[20], not doshes[20],
   so that I can refer to the seventh dosh as "dosh[7]".

 - If you more often refer to the array as a whole (than you refer
   to individual elements) and want to stress the fact that the
   array holds multiple elements in it, I can understand that you
   may be tempted to call the whole array "doshes[]".

So please drop this and other "rename variables" patches from the
series.

Re: [PATCH 03/20] cat-file: rename variables in ref-filter

[Оля Тележная]

2018-01-10 1:04 GMT+03:00 Junio C Hamano <gitster@pobox.com>:
> Olga Telezhnaya <olyatelezhnaya@gmail.com> writes:
>
>> Rename some variables for easier reading.
>> They point not to values, but to arrays.
>
> Once the code is written and people start to build on top, a change
> like this is not worth the code churn, especially because there are
> two equally valid schools of naming convention.
>
>  - When you have an array, each of whose 20 slots holds a single
>    dosh, I would prefer to call the array dosh[20], not doshes[20],
>    so that I can refer to the seventh dosh as "dosh[7]".
>
>  - If you more often refer to the array as a whole (than you refer
>    to individual elements) and want to stress the fact that the
>    array holds multiple elements in it, I can understand that you
>    may be tempted to call the whole array "doshes[]".
>
> So please drop this and other "rename variables" patches from the
> series.
>
>

OK, I will revert that. I have done this because it's hard for me to
keep in mind that it's not just a simple pointer to a single value,
and I tried to make the code more intuitive.

[PATCH 08/20] cat-file: remove unused code

[Olga Telezhnaya]

No further need in mark_query parameter.
All logic related to expand_atom_into_fields is not needed here also,
we are doing that in 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 | 25 +------------------------
 ref-filter.h       |  6 ------
 2 files changed, 1 insertion(+), 30 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 0a3f4a47bf1ae..9055fa3a8b0ae 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 expand_atom_into_fields(struct strbuf *sb, 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)
 {
@@ -232,11 +213,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)
-		expand_atom_into_fields(sb, 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;
 }
 
diff --git a/ref-filter.h b/ref-filter.h
index 97169548939cd..590a60ffe034d 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

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

[PATCH 15/20] cat-file: start reusing populate_value

[Olga Telezhnaya]

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 | 16 +++-------------
 ref-filter.c       | 15 +++++++++++++++
 ref-filter.h       |  2 ++
 3 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 1c92194faaede..e11dbf88e386c 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -284,21 +284,11 @@ static void batch_object_write(const char *obj_name, struct batch_options *opt,
 	struct strbuf buf = STRBUF_INIT;
 	struct ref_array_item item;
 
-	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.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;
+	item.start_of_request = 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 ed1b78943b8b4..7df6d7e3d6511 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1469,6 +1469,21 @@ int populate_value(struct ref_array_item *ref)
 			ref->symref = "";
 	}
 
+	if (cat_file_info) {
+		if (!cat_file_info->skip_object_info &&
+		    sha1_object_info_extended(ref->objectname.hash, &cat_file_info->info,
+					      OBJECT_INFO_LOOKUP_REPLACE) < 0) {
+			const char *e = ref->start_of_request;
+			printf("%s missing\n", e ? e : oid_to_hex(&ref->objectname));
+			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;
+	}
+
 	/* Fill in specials first */
 	for (i = 0; i < used_atom_cnt; i++) {
 		struct used_atom *atom = &used_atoms[i];
diff --git a/ref-filter.h b/ref-filter.h
index d541749f9b1dc..9e4444cf3ef9a 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -45,6 +45,8 @@ struct ref_array_item {
 	off_t disk_size;
 	const char *rest;
 	struct object_id delta_base_oid;
+	/* Need it for better explanation in error log. */
+	const char *start_of_request;
 	char refname[FLEX_ARRAY];
 };
 

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

[PATCH 14/20] cat-file: make populate_value global

[Olga Telezhnaya]

Make function global for further using in cat-file.
Also added return value for handling errors.

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 | 4 ++--
 ref-filter.h | 3 +++
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 56d2687e07df9..ed1b78943b8b4 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1456,7 +1456,7 @@ static void need_object(struct ref_array_item *ref) {
 /*
  * Parse the object referred by ref, and grab needed value.
  */
-static void populate_value(struct ref_array_item *ref)
+int populate_value(struct ref_array_item *ref)
 {
 	int i;
 
@@ -1577,7 +1577,7 @@ static void populate_value(struct ref_array_item *ref)
 			break;
 		}
 	}
-	return;
+	return 0;
 }
 
 /*
diff --git a/ref-filter.h b/ref-filter.h
index 28774e8e0f771..d541749f9b1dc 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -176,4 +176,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/450

[PATCH 10/20] cat-file: simplify expand_atom function

[Olga Telezhnaya]

Not sure, but looks like there is no need in that checking.
There is a checking before whether it is null and we die in such case.

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

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 9055fa3a8b0ae..dd43457c0ad7e 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -193,10 +193,9 @@ static void expand_atom(struct strbuf *sb, const char *atom, int 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))
+	else if (is_atom("rest", atom, len))
+		strbuf_addstr(sb, data->rest);
+	else if (is_atom("deltabase", atom, len))
 		strbuf_addstr(sb, oid_to_hex(&data->delta_base_oid));
 	else
 		die("unknown format element: %.*s", len, atom);

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

[PATCH 09/20] cat-file: get rid of goto in ref-filter

[Olga Telezhnaya]

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 | 103 ++++++++++++++++++++++++++++++-----------------------------
 1 file changed, 53 insertions(+), 50 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 2bd9a8fc4e39c..1b8c8787190a9 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1405,16 +1405,60 @@ static const char *get_refname(struct used_atom *atom, struct ref_array_item *re
 	return show_ref(&atom->u.refname, ref->refname);
 }
 
+static void need_object(struct ref_array_item *ref) {
+	struct object *obj;
+	const struct object_id *tagged;
+	unsigned long size;
+	int eaten;
+	void *buf = get_obj(&ref->objectname, &obj, &size, &eaten);
+	if (!buf)
+		die(_("missing object %s for %s"),
+		    oid_to_hex(&ref->objectname), ref->refname);
+	if (!obj)
+		die(_("parse_object_buffer failed on %s for %s"),
+		    oid_to_hex(&ref->objectname), ref->refname);
+
+	grab_values(ref->value, 0, obj, buf, size);
+	if (!eaten)
+		free(buf);
+
+	/*
+	 * If there is no atom that wants to know about tagged
+	 * object, we are done.
+	 */
+	if (!need_tagged || (obj->type != OBJ_TAG))
+		return;
+
+	/*
+	 * If it is a tag object, see if we use a value that derefs
+	 * the object, and if we do grab the object it refers to.
+	 */
+	tagged = &((struct tag *)obj)->tagged->oid;
+
+	/*
+	 * NEEDSWORK: This derefs tag only once, which
+	 * is good to deal with chains of trust, but
+	 * is not consistent with what deref_tag() does
+	 * which peels the onion to the core.
+	 */
+	buf = get_obj(tagged, &obj, &size, &eaten);
+	if (!buf)
+		die(_("missing object %s for %s"),
+		    oid_to_hex(tagged), ref->refname);
+	if (!obj)
+		die(_("parse_object_buffer failed on %s for %s"),
+		    oid_to_hex(tagged), ref->refname);
+	grab_values(ref->value, 1, obj, buf, size);
+	if (!eaten)
+		free(buf);
+}
+
 /*
  * Parse the object referred by ref, and grab needed value.
  */
 static void populate_value(struct ref_array_item *ref)
 {
-	void *buf;
-	struct object *obj;
-	int eaten, i;
-	unsigned long size;
-	const struct object_id *tagged;
+	int i;
 
 	ref->value = xcalloc(used_atom_cnt, sizeof(struct atom_value));
 
@@ -1528,53 +1572,12 @@ 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) {
+			need_object(ref);
+			break;
+		}
 	}
 	return;
-
- need_obj:
-	buf = get_obj(&ref->objectname, &obj, &size, &eaten);
-	if (!buf)
-		die(_("missing object %s for %s"),
-		    oid_to_hex(&ref->objectname), ref->refname);
-	if (!obj)
-		die(_("parse_object_buffer failed on %s for %s"),
-		    oid_to_hex(&ref->objectname), ref->refname);
-
-	grab_values(ref->value, 0, obj, buf, size);
-	if (!eaten)
-		free(buf);
-
-	/*
-	 * If there is no atom that wants to know about tagged
-	 * object, we are done.
-	 */
-	if (!need_tagged || (obj->type != OBJ_TAG))
-		return;
-
-	/*
-	 * If it is a tag object, see if we use a value that derefs
-	 * the object, and if we do grab the object it refers to.
-	 */
-	tagged = &((struct tag *)obj)->tagged->oid;
-
-	/*
-	 * NEEDSWORK: This derefs tag only once, which
-	 * is good to deal with chains of trust, but
-	 * is not consistent with what deref_tag() does
-	 * which peels the onion to the core.
-	 */
-	buf = get_obj(tagged, &obj, &size, &eaten);
-	if (!buf)
-		die(_("missing object %s for %s"),
-		    oid_to_hex(tagged), ref->refname);
-	if (!obj)
-		die(_("parse_object_buffer failed on %s for %s"),
-		    oid_to_hex(tagged), ref->refname);
-	grab_values(ref->value, 1, obj, buf, size);
-	if (!eaten)
-		free(buf);
 }
 
 /*

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

[PATCH v2 01/18] cat-file: split expand_atom into 2 functions

[Olga Telezhnaya]

Split expand_atom function into 2 different functions,
expand_atom_into_fields prepares variable for further filling,
(new) expand_atom creates resulting string.
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 | 73 +++++++++++++++++++++++++++++-------------------------
 1 file changed, 39 insertions(+), 34 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index f5fa4fd75af26..f783b39b9bd5c 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -217,47 +217,49 @@ 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 expand_atom_into_fields(struct strbuf *sb, 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
+	} else if (is_atom("deltabase", atom, len))
+		strbuf_addstr(sb, oid_to_hex(&data->delta_base_oid));
+	else
 		die("unknown format element: %.*s", len, atom);
 }
 
-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;
@@ -265,7 +267,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)
+		expand_atom_into_fields(sb, 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/450

[PATCH v2 13/18] cat-file: start reusing populate_value

[Olga Telezhnaya]

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 | 16 +++-------------
 ref-filter.c       | 15 +++++++++++++++
 ref-filter.h       |  2 ++
 3 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 1c92194faaede..e11dbf88e386c 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -284,21 +284,11 @@ static void batch_object_write(const char *obj_name, struct batch_options *opt,
 	struct strbuf buf = STRBUF_INIT;
 	struct ref_array_item item;
 
-	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.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;
+	item.start_of_request = 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 c15906cb091c7..35e16cec6d862 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1467,6 +1467,21 @@ int populate_value(struct ref_array_item *ref)
 			ref->symref = "";
 	}
 
+	if (cat_file_info) {
+		if (!cat_file_info->skip_object_info &&
+		    sha1_object_info_extended(ref->objectname.hash, &cat_file_info->info,
+					      OBJECT_INFO_LOOKUP_REPLACE) < 0) {
+			const char *e = ref->start_of_request;
+			printf("%s missing\n", e ? e : oid_to_hex(&ref->objectname));
+			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;
+	}
+
 	/* 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 6df45c5bd9dcb..0c7253913735b 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -45,6 +45,8 @@ struct ref_array_item {
 	off_t disk_size;
 	const char *rest;
 	struct object_id delta_base_oid;
+	/* Need it for better explanation in error log. */
+	const char *start_of_request;
 	char refname[FLEX_ARRAY];
 };
 

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

[PATCH v2 08/18] ref-filter: get rid of goto

[Olga Telezhnaya]

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 | 103 ++++++++++++++++++++++++++++++-----------------------------
 1 file changed, 53 insertions(+), 50 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index bb09875e2dbf4..575c5351d0f79 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1403,16 +1403,60 @@ static const char *get_refname(struct used_atom *atom, struct ref_array_item *re
 	return show_ref(&atom->u.refname, ref->refname);
 }
 
+static void need_object(struct ref_array_item *ref) {
+	struct object *obj;
+	const struct object_id *tagged;
+	unsigned long size;
+	int eaten;
+	void *buf = get_obj(&ref->objectname, &obj, &size, &eaten);
+	if (!buf)
+		die(_("missing object %s for %s"),
+		    oid_to_hex(&ref->objectname), ref->refname);
+	if (!obj)
+		die(_("parse_object_buffer failed on %s for %s"),
+		    oid_to_hex(&ref->objectname), ref->refname);
+
+	grab_values(ref->value, 0, obj, buf, size);
+	if (!eaten)
+		free(buf);
+
+	/*
+	 * If there is no atom that wants to know about tagged
+	 * object, we are done.
+	 */
+	if (!need_tagged || (obj->type != OBJ_TAG))
+		return;
+
+	/*
+	 * If it is a tag object, see if we use a value that derefs
+	 * the object, and if we do grab the object it refers to.
+	 */
+	tagged = &((struct tag *)obj)->tagged->oid;
+
+	/*
+	 * NEEDSWORK: This derefs tag only once, which
+	 * is good to deal with chains of trust, but
+	 * is not consistent with what deref_tag() does
+	 * which peels the onion to the core.
+	 */
+	buf = get_obj(tagged, &obj, &size, &eaten);
+	if (!buf)
+		die(_("missing object %s for %s"),
+		    oid_to_hex(tagged), ref->refname);
+	if (!obj)
+		die(_("parse_object_buffer failed on %s for %s"),
+		    oid_to_hex(tagged), ref->refname);
+	grab_values(ref->value, 1, obj, buf, size);
+	if (!eaten)
+		free(buf);
+}
+
 /*
  * Parse the object referred by ref, and grab needed value.
  */
 static void populate_value(struct ref_array_item *ref)
 {
-	void *buf;
-	struct object *obj;
-	int eaten, i;
-	unsigned long size;
-	const struct object_id *tagged;
+	int i;
 
 	ref->value = xcalloc(used_atom_cnt, sizeof(struct atom_value));
 
@@ -1526,53 +1570,12 @@ 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) {
+			need_object(ref);
+			break;
+		}
 	}
 	return;
-
- need_obj:
-	buf = get_obj(&ref->objectname, &obj, &size, &eaten);
-	if (!buf)
-		die(_("missing object %s for %s"),
-		    oid_to_hex(&ref->objectname), ref->refname);
-	if (!obj)
-		die(_("parse_object_buffer failed on %s for %s"),
-		    oid_to_hex(&ref->objectname), ref->refname);
-
-	grab_values(ref->value, 0, obj, buf, size);
-	if (!eaten)
-		free(buf);
-
-	/*
-	 * If there is no atom that wants to know about tagged
-	 * object, we are done.
-	 */
-	if (!need_tagged || (obj->type != OBJ_TAG))
-		return;
-
-	/*
-	 * If it is a tag object, see if we use a value that derefs
-	 * the object, and if we do grab the object it refers to.
-	 */
-	tagged = &((struct tag *)obj)->tagged->oid;
-
-	/*
-	 * NEEDSWORK: This derefs tag only once, which
-	 * is good to deal with chains of trust, but
-	 * is not consistent with what deref_tag() does
-	 * which peels the onion to the core.
-	 */
-	buf = get_obj(tagged, &obj, &size, &eaten);
-	if (!buf)
-		die(_("missing object %s for %s"),
-		    oid_to_hex(tagged), ref->refname);
-	if (!obj)
-		die(_("parse_object_buffer failed on %s for %s"),
-		    oid_to_hex(tagged), ref->refname);
-	grab_values(ref->value, 1, obj, buf, size);
-	if (!eaten)
-		free(buf);
 }
 
 /*

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

[PATCH v2 03/18] ref-filter: make valid_atom as function parameter

[Olga Telezhnaya]

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 3f9161707e66b..32b7e918bca75 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))
@@ -2139,7 +2141,9 @@ void format_ref_array_item(struct ref_array_item *info,
 		if (cp < sp)
 			append_literal(cp, sp, &state);
 		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);
 		atomv->handler(atomv, &state);
 	}
@@ -2187,7 +2191,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/450

Re: [PATCH v2 03/18] ref-filter: make valid_atom as function parameter

[Jeff King]

On Wed, Jan 10, 2018 at 09:36:41AM +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.

I agree in the end we'd want a single valid_atom list. It doesn't look
like we hit that end state in this series, though.

I guess I'm not quite clear on why we're not adding these new atoms to
ref-filter (and for-each-ref) right away, though. We already have the
first three (name, type, and size), and we'd just need to support
%(rest) and %(deltabase).

I think %(rest) doesn't really make sense for for-each-ref (we're not
reading any input), but it could expand to the empty string by default
(or even throw an error if the caller asks us not to support it).

IOW, the progression I'd expect in a series like this is:

  1. Teach ref-filter.c to support everything that cat-file can do.

  2. Convert cat-file to use ref-filter.c.

-Peff

Re: [PATCH v2 03/18] ref-filter: make valid_atom as function parameter

[Оля Тележная]

2018-01-16 0:42 GMT+03:00 Jeff King <peff@peff.net>:
> On Wed, Jan 10, 2018 at 09:36:41AM +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.
>
> I agree in the end we'd want a single valid_atom list. It doesn't look
> like we hit that end state in this series, though.
>
> I guess I'm not quite clear on why we're not adding these new atoms to
> ref-filter (and for-each-ref) right away, though. We already have the
> first three (name, type, and size), and we'd just need to support
> %(rest) and %(deltabase).
>
> I think %(rest) doesn't really make sense for for-each-ref (we're not
> reading any input), but it could expand to the empty string by default
> (or even throw an error if the caller asks us not to support it).
>
> IOW, the progression I'd expect in a series like this is:
>
>   1. Teach ref-filter.c to support everything that cat-file can do.
>
>   2. Convert cat-file to use ref-filter.c.
>
> -Peff

I agree, I even made this and it's working fine:
https://github.com/git/git/pull/450/commits/1b74f1047f07434dccb207534d1ad45a143e3f2b
But I decided not to add that to patch because I expand the
functionality of several commands (not only cat-file and
for-each-ref), and I need to support all new functionality in a proper
way, make these error messages, test everything and - the hardest one
- support many new commands for cat-file. As I understand, it is not
possible unless we finally move to ref-filter and print results also
there. Oh, and I also need to rewrite docs in that case. And I decided
to apply this in another patch. But, please, say your opinion, maybe
we could do that here in some way.

Olga

Re: [PATCH v2 03/18] ref-filter: make valid_atom as function parameter

[Jeff King]

On Tue, Jan 16, 2018 at 09:55:22AM +0300, Оля Тележная wrote:

> > IOW, the progression I'd expect in a series like this is:
> >
> >   1. Teach ref-filter.c to support everything that cat-file can do.
> >
> >   2. Convert cat-file to use ref-filter.c.
> 
> I agree, I even made this and it's working fine:
> https://github.com/git/git/pull/450/commits/1b74f1047f07434dccb207534d1ad45a143e3f2b
> But I decided not to add that to patch because I expand the
> functionality of several commands (not only cat-file and
> for-each-ref), and I need to support all new functionality in a proper
> way, make these error messages, test everything and - the hardest one
> - support many new commands for cat-file. As I understand, it is not
> possible unless we finally move to ref-filter and print results also
> there. Oh, and I also need to rewrite docs in that case. And I decided
> to apply this in another patch. But, please, say your opinion, maybe
> we could do that here in some way.

Yeah, I agree that it will cause changes to other users of ref-filter,
and you'd need to deal with documentation and tests there. But I think
we're going to have to do those things eventually (since supporting
those extra fields everywhere is part of the point of the project). And
by doing them now, I think it can make the transition for cat-file a lot
simpler, because we never have to puzzle over this intermediate state
where only some of the atoms are valid for cat-file.

I also agree that moving cat-file's printing over to ref-filter.c is
going to introduce a lot of corner cases (like the behavior when
somebody does "cat-file --batch-check=%(refname)" with a bare sha1). But
I think a progression of patches handling those cases would be pretty
easy to follow. E.g., I'd expect to see a patch that teaches
ref-filter[1] how to handle callers that don't have a ref, but just a
bare object name. And it would be easier to evaluate that on its own,
because we know that's an end state we're going to have to handle, and
not some funny state created as part of the transition.

-Peff

[1] And here's where we might start to think about calling it something
    besides ref-filter, if we don't necessarily have a ref. :)

Re: [PATCH v2 03/18] ref-filter: make valid_atom as function parameter

[Christian Couder]

On Wed, Jan 17, 2018 at 10:43 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Jan 16, 2018 at 09:55:22AM +0300, Оля Тележная wrote:
>
>> > IOW, the progression I'd expect in a series like this is:
>> >
>> >   1. Teach ref-filter.c to support everything that cat-file can do.
>> >
>> >   2. Convert cat-file to use ref-filter.c.
>>
>> I agree, I even made this and it's working fine:
>> https://github.com/git/git/pull/450/commits/1b74f1047f07434dccb207534d1ad45a143e3f2b

(Nit: it looks like the above link does not work any more, but it
seems that you are talking about the last patch on the catfile
branch.)

>> But I decided not to add that to patch because I expand the
>> functionality of several commands (not only cat-file and
>> for-each-ref), and I need to support all new functionality in a proper
>> way, make these error messages, test everything and - the hardest one
>> - support many new commands for cat-file. As I understand, it is not
>> possible unless we finally move to ref-filter and print results also
>> there. Oh, and I also need to rewrite docs in that case. And I decided
>> to apply this in another patch. But, please, say your opinion, maybe
>> we could do that here in some way.
>
> Yeah, I agree that it will cause changes to other users of ref-filter,
> and you'd need to deal with documentation and tests there. But I think
> we're going to have to do those things eventually (since supporting
> those extra fields everywhere is part of the point of the project). And
> by doing them now, I think it can make the transition for cat-file a lot
> simpler, because we never have to puzzle over this intermediate state
> where only some of the atoms are valid for cat-file.

I agree that you will have to deal with documentation and tests at one
point and that it could be a good idea to do that now.

I wonder if it is possible to add atoms one by one into ref-filter and
to add tests and documentation at the same time, instead of merging
cat-file atoms with ref-filter atoms in one big step.

When all the cat-file atoms have been added to ref-filter's
valid_atom, maybe you could add ref-filter's atoms to cat-file's
valid_cat_file_atom one by one and add tests and documentation at the
same time.

And then when ref-filter's valid_atom and cat-file's
valid_cat_file_atom are identical you can remove cat-file's
valid_cat_file_atom and maybe after that rename "ref-filter" to
"format".

Re: [PATCH v2 03/18] ref-filter: make valid_atom as function parameter

[Оля Тележная]

2018-01-18 1:39 GMT+03:00 Christian Couder <christian.couder@gmail.com>:
> On Wed, Jan 17, 2018 at 10:43 PM, Jeff King <peff@peff.net> wrote:
>> On Tue, Jan 16, 2018 at 09:55:22AM +0300, Оля Тележная wrote:
>>
>>> > IOW, the progression I'd expect in a series like this is:
>>> >
>>> >   1. Teach ref-filter.c to support everything that cat-file can do.
>>> >
>>> >   2. Convert cat-file to use ref-filter.c.
>>>
>>> I agree, I even made this and it's working fine:
>>> https://github.com/git/git/pull/450/commits/1b74f1047f07434dccb207534d1ad45a143e3f2b
>
> (Nit: it looks like the above link does not work any more, but it
> seems that you are talking about the last patch on the catfile
> branch.)
>
>>> But I decided not to add that to patch because I expand the
>>> functionality of several commands (not only cat-file and
>>> for-each-ref), and I need to support all new functionality in a proper
>>> way, make these error messages, test everything and - the hardest one
>>> - support many new commands for cat-file. As I understand, it is not
>>> possible unless we finally move to ref-filter and print results also
>>> there. Oh, and I also need to rewrite docs in that case. And I decided
>>> to apply this in another patch. But, please, say your opinion, maybe
>>> we could do that here in some way.
>>
>> Yeah, I agree that it will cause changes to other users of ref-filter,
>> and you'd need to deal with documentation and tests there. But I think
>> we're going to have to do those things eventually (since supporting
>> those extra fields everywhere is part of the point of the project). And
>> by doing them now, I think it can make the transition for cat-file a lot
>> simpler, because we never have to puzzle over this intermediate state
>> where only some of the atoms are valid for cat-file.
>
> I agree that you will have to deal with documentation and tests at one
> point and that it could be a good idea to do that now.
>
> I wonder if it is possible to add atoms one by one into ref-filter and
> to add tests and documentation at the same time, instead of merging
> cat-file atoms with ref-filter atoms in one big step.
>
> When all the cat-file atoms have been added to ref-filter's
> valid_atom, maybe you could add ref-filter's atoms to cat-file's
> valid_cat_file_atom one by one and add tests and documentation at the
> same time.
>
> And then when ref-filter's valid_atom and cat-file's
> valid_cat_file_atom are identical you can remove cat-file's
> valid_cat_file_atom and maybe after that rename "ref-filter" to
> "format".

I think it's important to finish migrating process at first. I mean,
now we are preparing and collecting everything in ref-filter, but we
make resulting string and print still in cat-file. And I am not sure,
but maybe it will not be possible to start using new atoms in cat-file
while some part of logic still differs.
And another thoughts here - we were thinking about creating format.h
but decided not to move forward with it, and now we are suffering
because of it. Can I create it right now or the history of commits
would be too dirty because of it? Also, do you mean just renaming of
ref-filter? I was thinking that I need to put formatting-related logic
to another file and leave all other stuff in ref-filter.

Anyway, your suggested steps looks good, and I am planning to
implement them later. Let's discuss, what behavior we are waiting for
when atom seems useless for the command. Die or ignore? And, which
atoms are useless (as I understand, "rest" and "deltabase" from
cat-file are useless for all ref-filter users, so the question is
about - am I right in it, and about ref-filter atoms for cat-file).

I have never written any tests and docs for Git, I will try to explore
by myself how to do that, but if you have any special links/materials
about it - please send them to me :)

Olga

Re: [PATCH v2 03/18] ref-filter: make valid_atom as function parameter

[Оля Тележная]

2018-01-18 9:20 GMT+03:00 Оля Тележная <olyatelezhnaya@gmail.com>:
> 2018-01-18 1:39 GMT+03:00 Christian Couder <christian.couder@gmail.com>:
>> On Wed, Jan 17, 2018 at 10:43 PM, Jeff King <peff@peff.net> wrote:
>>> On Tue, Jan 16, 2018 at 09:55:22AM +0300, Оля Тележная wrote:
>>>
>>>> > IOW, the progression I'd expect in a series like this is:
>>>> >
>>>> >   1. Teach ref-filter.c to support everything that cat-file can do.
>>>> >
>>>> >   2. Convert cat-file to use ref-filter.c.
>>>>
>>>> I agree, I even made this and it's working fine:
>>>> https://github.com/git/git/pull/450/commits/1b74f1047f07434dccb207534d1ad45a143e3f2b
>>
>> (Nit: it looks like the above link does not work any more, but it
>> seems that you are talking about the last patch on the catfile
>> branch.)
>>
>>>> But I decided not to add that to patch because I expand the
>>>> functionality of several commands (not only cat-file and
>>>> for-each-ref), and I need to support all new functionality in a proper
>>>> way, make these error messages, test everything and - the hardest one
>>>> - support many new commands for cat-file. As I understand, it is not
>>>> possible unless we finally move to ref-filter and print results also
>>>> there. Oh, and I also need to rewrite docs in that case. And I decided
>>>> to apply this in another patch. But, please, say your opinion, maybe
>>>> we could do that here in some way.
>>>
>>> Yeah, I agree that it will cause changes to other users of ref-filter,
>>> and you'd need to deal with documentation and tests there. But I think
>>> we're going to have to do those things eventually (since supporting
>>> those extra fields everywhere is part of the point of the project). And
>>> by doing them now, I think it can make the transition for cat-file a lot
>>> simpler, because we never have to puzzle over this intermediate state
>>> where only some of the atoms are valid for cat-file.
>>
>> I agree that you will have to deal with documentation and tests at one
>> point and that it could be a good idea to do that now.
>>
>> I wonder if it is possible to add atoms one by one into ref-filter and
>> to add tests and documentation at the same time, instead of merging
>> cat-file atoms with ref-filter atoms in one big step.
>>
>> When all the cat-file atoms have been added to ref-filter's
>> valid_atom, maybe you could add ref-filter's atoms to cat-file's
>> valid_cat_file_atom one by one and add tests and documentation at the
>> same time.
>>
>> And then when ref-filter's valid_atom and cat-file's
>> valid_cat_file_atom are identical you can remove cat-file's
>> valid_cat_file_atom and maybe after that rename "ref-filter" to
>> "format".
>
> I think it's important to finish migrating process at first. I mean,
> now we are preparing and collecting everything in ref-filter, but we
> make resulting string and print still in cat-file. And I am not sure,
> but maybe it will not be possible to start using new atoms in cat-file
> while some part of logic still differs.

I tried to make that part here:
https://github.com/telezhnaya/git/commit/19a148614f1d4db1f8e628eb4e6d7c819d2da875
I know that the code is disgusting and there is a memory leak :) I
just try to reuse ref-filter logic, I will cleanup everything later.
At first, I try to make it work.
The problem is that I have segfault, and if I use gdb, I get:

Program received signal SIGSEGV, Segmentation fault.
0x0000000000000000 in ?? ()

I tried to google it, it's my first time when I get that strange
message, and unfortunately find nothing. So please explain me the
reason, why I can't find a place of segfault that way.
Thanks!

> And another thoughts here - we were thinking about creating format.h
> but decided not to move forward with it, and now we are suffering
> because of it. Can I create it right now or the history of commits
> would be too dirty because of it? Also, do you mean just renaming of
> ref-filter? I was thinking that I need to put formatting-related logic
> to another file and leave all other stuff in ref-filter.

By the way, I can create format.h in absolutely another branch, we
could merge it, and I will deal with all merge conflicts with my
current work. It sounds tediously, but actually it's not such a big
problem, I can do that.
But I still need to fully understand, what do you find more proper -
just rename ref-filter, or create new file and move formatting-related
logic.

>
> Anyway, your suggested steps looks good, and I am planning to
> implement them later. Let's discuss, what behavior we are waiting for
> when atom seems useless for the command. Die or ignore? And, which
> atoms are useless (as I understand, "rest" and "deltabase" from
> cat-file are useless for all ref-filter users, so the question is
> about - am I right in it, and about ref-filter atoms for cat-file).
>
> I have never written any tests and docs for Git, I will try to explore
> by myself how to do that, but if you have any special links/materials
> about it - please send them to me :)
>
> Olga

Re: [PATCH v2 03/18] ref-filter: make valid_atom as function parameter

[Christian Couder]

On Thu, Jan 18, 2018 at 12:49 PM, Оля Тележная <olyatelezhnaya@gmail.com> wrote:
> 2018-01-18 9:20 GMT+03:00 Оля Тележная <olyatelezhnaya@gmail.com>:
>>
>> I think it's important to finish migrating process at first. I mean,
>> now we are preparing and collecting everything in ref-filter, but we
>> make resulting string and print still in cat-file. And I am not sure,
>> but maybe it will not be possible to start using new atoms in cat-file
>> while some part of logic still differs.
>
> I tried to make that part here:
> https://github.com/telezhnaya/git/commit/19a148614f1d4db1f8e628eb4e6d7c819d2da875
> I know that the code is disgusting and there is a memory leak :) I
> just try to reuse ref-filter logic, I will cleanup everything later.
> At first, I try to make it work.
> The problem is that I have segfault, and if I use gdb, I get:
>
> Program received signal SIGSEGV, Segmentation fault.
> 0x0000000000000000 in ?? ()

Make sure that you compile with debug options like -g3. For example I use:

$ make -j 4 DEVELOPER=1 CFLAGS="-g3"

> I tried to google it, it's my first time when I get that strange
> message, and unfortunately find nothing. So please explain me the
> reason, why I can't find a place of segfault that way.

I get the following:

(gdb) run cat-file --batch < myarg.txt
Starting program: /home/ubuntu/bin/git cat-file --batch < myarg.txt
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".

Program received signal SIGSEGV, Segmentation fault.
0x00005555556ea7cf in format_ref_array_item (info=0x7fffffffd460,
format=0x7fffffffd6e0,
    final_buf=0x7fffffffd410) at ref-filter.c:2234
2234                    atomv->handler(atomv, &state);
(gdb) bt
#0  0x00005555556ea7cf in format_ref_array_item (info=0x7fffffffd460,
    format=0x7fffffffd6e0, final_buf=0x7fffffffd410) at ref-filter.c:2234
#1  0x00005555556ea91c in show_ref_array_item (info=0x7fffffffd460,
format=0x7fffffffd6e0)
    at ref-filter.c:2256
#2  0x0000555555577ef7 in batch_object_write (
    obj_name=0x555555a66770 "5e1c309dae7f45e0f39b1bf3ac3cd9db12e7d689",
    opt=0x7fffffffd6e0, data=0x7fffffffd5e0) at builtin/cat-file.c:298

Re: [PATCH v2 03/18] ref-filter: make valid_atom as function parameter

[Оля Тележная]

2018-01-18 17:23 GMT+03:00 Christian Couder <christian.couder@gmail.com>:
> On Thu, Jan 18, 2018 at 12:49 PM, Оля Тележная <olyatelezhnaya@gmail.com> wrote:
>> 2018-01-18 9:20 GMT+03:00 Оля Тележная <olyatelezhnaya@gmail.com>:
>>>
>>> I think it's important to finish migrating process at first. I mean,
>>> now we are preparing and collecting everything in ref-filter, but we
>>> make resulting string and print still in cat-file. And I am not sure,
>>> but maybe it will not be possible to start using new atoms in cat-file
>>> while some part of logic still differs.
>>
>> I tried to make that part here:
>> https://github.com/telezhnaya/git/commit/19a148614f1d4db1f8e628eb4e6d7c819d2da875
>> I know that the code is disgusting and there is a memory leak :) I
>> just try to reuse ref-filter logic, I will cleanup everything later.
>> At first, I try to make it work.
>> The problem is that I have segfault, and if I use gdb, I get:
>>
>> Program received signal SIGSEGV, Segmentation fault.
>> 0x0000000000000000 in ?? ()
>
> Make sure that you compile with debug options like -g3. For example I use:
>
> $ make -j 4 DEVELOPER=1 CFLAGS="-g3"

Is it OK that I get different test results with simple make and with
make with all that flags?
Have a code: https://github.com/telezhnaya/git/commits/catfile
I do:

olya@ubuntu17-vm:~/git$ make install
olya@ubuntu17-vm:~/git$ cd t
olya@ubuntu17-vm:~/git/t$ ./t1006-cat-file.sh

And I have 17 tests broken.
Then, without any changes in code, I do:

olya@ubuntu17-vm:~/git$ make -j 4 DEVELOPER=1 CFLAGS="-g3" install
olya@ubuntu17-vm:~/git$ cd t
olya@ubuntu17-vm:~/git/t$ ./t1006-cat-file.sh

And there is 42 tests broken.
And it's really hard to search for errors in such situation.

>
>> I tried to google it, it's my first time when I get that strange
>> message, and unfortunately find nothing. So please explain me the
>> reason, why I can't find a place of segfault that way.
>
> I get the following:
>
> (gdb) run cat-file --batch < myarg.txt
> Starting program: /home/ubuntu/bin/git cat-file --batch < myarg.txt
> [Thread debugging using libthread_db enabled]
> Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
>
> Program received signal SIGSEGV, Segmentation fault.
> 0x00005555556ea7cf in format_ref_array_item (info=0x7fffffffd460,
> format=0x7fffffffd6e0,
>     final_buf=0x7fffffffd410) at ref-filter.c:2234
> 2234                    atomv->handler(atomv, &state);
> (gdb) bt
> #0  0x00005555556ea7cf in format_ref_array_item (info=0x7fffffffd460,
>     format=0x7fffffffd6e0, final_buf=0x7fffffffd410) at ref-filter.c:2234
> #1  0x00005555556ea91c in show_ref_array_item (info=0x7fffffffd460,
> format=0x7fffffffd6e0)
>     at ref-filter.c:2256
> #2  0x0000555555577ef7 in batch_object_write (
>     obj_name=0x555555a66770 "5e1c309dae7f45e0f39b1bf3ac3cd9db12e7d689",
>     opt=0x7fffffffd6e0, data=0x7fffffffd5e0) at builtin/cat-file.c:298

Re: [PATCH v2 03/18] ref-filter: make valid_atom as function parameter

[Christian Couder]

On Fri, Jan 19, 2018 at 1:24 PM, Оля Тележная <olyatelezhnaya@gmail.com> wrote:
> 2018-01-18 17:23 GMT+03:00 Christian Couder <christian.couder@gmail.com>:
>> On Thu, Jan 18, 2018 at 12:49 PM, Оля Тележная <olyatelezhnaya@gmail.com> wrote:
>>> 2018-01-18 9:20 GMT+03:00 Оля Тележная <olyatelezhnaya@gmail.com>:
>>>>
>>>> I think it's important to finish migrating process at first. I mean,
>>>> now we are preparing and collecting everything in ref-filter, but we
>>>> make resulting string and print still in cat-file. And I am not sure,
>>>> but maybe it will not be possible to start using new atoms in cat-file
>>>> while some part of logic still differs.
>>>
>>> I tried to make that part here:
>>> https://github.com/telezhnaya/git/commit/19a148614f1d4db1f8e628eb4e6d7c819d2da875
>>> I know that the code is disgusting and there is a memory leak :) I
>>> just try to reuse ref-filter logic, I will cleanup everything later.
>>> At first, I try to make it work.
>>> The problem is that I have segfault, and if I use gdb, I get:
>>>
>>> Program received signal SIGSEGV, Segmentation fault.
>>> 0x0000000000000000 in ?? ()
>>
>> Make sure that you compile with debug options like -g3. For example I use:
>>
>> $ make -j 4 DEVELOPER=1 CFLAGS="-g3"
>
> Is it OK that I get different test results with simple make and with
> make with all that flags?
> Have a code: https://github.com/telezhnaya/git/commits/catfile
> I do:
>
> olya@ubuntu17-vm:~/git$ make install
> olya@ubuntu17-vm:~/git$ cd t
> olya@ubuntu17-vm:~/git/t$ ./t1006-cat-file.sh
>
> And I have 17 tests broken.
> Then, without any changes in code, I do:
>
> olya@ubuntu17-vm:~/git$ make -j 4 DEVELOPER=1 CFLAGS="-g3" install
> olya@ubuntu17-vm:~/git$ cd t
> olya@ubuntu17-vm:~/git/t$ ./t1006-cat-file.sh
>
> And there is 42 tests broken.
> And it's really hard to search for errors in such situation.

Some segfaults and other memory issues might happen or not happen
depending on how the code has been compiled. I think that if you fix
all the memory related issues, the behavior should be the same.

Re: [PATCH v2 03/18] ref-filter: make valid_atom as function parameter

[Christian Couder]

On Thu, Jan 18, 2018 at 7:20 AM, Оля Тележная <olyatelezhnaya@gmail.com> wrote:
> 2018-01-18 1:39 GMT+03:00 Christian Couder <christian.couder@gmail.com>:
>> On Wed, Jan 17, 2018 at 10:43 PM, Jeff King <peff@peff.net> wrote:
>>> On Tue, Jan 16, 2018 at 09:55:22AM +0300, Оля Тележная wrote:
>>>
>>>> > IOW, the progression I'd expect in a series like this is:
>>>> >
>>>> >   1. Teach ref-filter.c to support everything that cat-file can do.
>>>> >
>>>> >   2. Convert cat-file to use ref-filter.c.
>>>>
>>>> I agree, I even made this and it's working fine:
>>>> https://github.com/git/git/pull/450/commits/1b74f1047f07434dccb207534d1ad45a143e3f2b
>>
>> (Nit: it looks like the above link does not work any more, but it
>> seems that you are talking about the last patch on the catfile
>> branch.)
>>
>>>> But I decided not to add that to patch because I expand the
>>>> functionality of several commands (not only cat-file and
>>>> for-each-ref), and I need to support all new functionality in a proper
>>>> way, make these error messages, test everything and - the hardest one
>>>> - support many new commands for cat-file. As I understand, it is not
>>>> possible unless we finally move to ref-filter and print results also
>>>> there. Oh, and I also need to rewrite docs in that case. And I decided
>>>> to apply this in another patch. But, please, say your opinion, maybe
>>>> we could do that here in some way.
>>>
>>> Yeah, I agree that it will cause changes to other users of ref-filter,
>>> and you'd need to deal with documentation and tests there. But I think
>>> we're going to have to do those things eventually (since supporting
>>> those extra fields everywhere is part of the point of the project). And
>>> by doing them now, I think it can make the transition for cat-file a lot
>>> simpler, because we never have to puzzle over this intermediate state
>>> where only some of the atoms are valid for cat-file.
>>
>> I agree that you will have to deal with documentation and tests at one
>> point and that it could be a good idea to do that now.
>>
>> I wonder if it is possible to add atoms one by one into ref-filter and
>> to add tests and documentation at the same time, instead of merging
>> cat-file atoms with ref-filter atoms in one big step.
>>
>> When all the cat-file atoms have been added to ref-filter's
>> valid_atom, maybe you could add ref-filter's atoms to cat-file's
>> valid_cat_file_atom one by one and add tests and documentation at the
>> same time.
>>
>> And then when ref-filter's valid_atom and cat-file's
>> valid_cat_file_atom are identical you can remove cat-file's
>> valid_cat_file_atom and maybe after that rename "ref-filter" to
>> "format".
>
> I think it's important to finish migrating process at first. I mean,
> now we are preparing and collecting everything in ref-filter, but we
> make resulting string and print still in cat-file. And I am not sure,
> but maybe it will not be possible to start using new atoms in cat-file
> while some part of logic still differs.

Ok, you can finish the migration process then.

> And another thoughts here - we were thinking about creating format.h
> but decided not to move forward with it, and now we are suffering
> because of it. Can I create it right now or the history of commits
> would be too dirty because of it?

It would also make it difficult to refactor your patch series if there
is a big move or renaming in the middle.

> Also, do you mean just renaming of
> ref-filter? I was thinking that I need to put formatting-related logic
> to another file and leave all other stuff in ref-filter.

Yeah, you can do both a move and a renaming.

> Anyway, your suggested steps looks good, and I am planning to
> implement them later.

Ok.

> Let's discuss, what behavior we are waiting for
> when atom seems useless for the command. Die or ignore?

We could alternatively just emit a warning, but it looks like there
are a lot of die() calls already in ref-filter.c, so I would suggest
die().

> And, which
> atoms are useless (as I understand, "rest" and "deltabase" from
> cat-file are useless for all ref-filter users, so the question is
> about - am I right in it, and about ref-filter atoms for cat-file).

For now and I think until the migration process is finished, you could
just die() in case of any atom not already supported by the command.

> I have never written any tests and docs for Git, I will try to explore
> by myself how to do that, but if you have any special links/materials
> about it - please send them to me :)

I think that looking at the existing documentation and tests is
probably the best way to learn about it.

Re: [PATCH v2 03/18] ref-filter: make valid_atom as function parameter

[Оля Тележная]

2018-01-19 20:14 GMT+03:00 Christian Couder <christian.couder@gmail.com>:
> On Thu, Jan 18, 2018 at 7:20 AM, Оля Тележная <olyatelezhnaya@gmail.com> wrote:
>> 2018-01-18 1:39 GMT+03:00 Christian Couder <christian.couder@gmail.com>:
>>> On Wed, Jan 17, 2018 at 10:43 PM, Jeff King <peff@peff.net> wrote:
>>>> On Tue, Jan 16, 2018 at 09:55:22AM +0300, Оля Тележная wrote:
>>>>
>>>>> > IOW, the progression I'd expect in a series like this is:
>>>>> >
>>>>> >   1. Teach ref-filter.c to support everything that cat-file can do.
>>>>> >
>>>>> >   2. Convert cat-file to use ref-filter.c.
>>>>>
>>>>> I agree, I even made this and it's working fine:
>>>>> https://github.com/git/git/pull/450/commits/1b74f1047f07434dccb207534d1ad45a143e3f2b
>>>
>>> (Nit: it looks like the above link does not work any more, but it
>>> seems that you are talking about the last patch on the catfile
>>> branch.)
>>>
>>>>> But I decided not to add that to patch because I expand the
>>>>> functionality of several commands (not only cat-file and
>>>>> for-each-ref), and I need to support all new functionality in a proper
>>>>> way, make these error messages, test everything and - the hardest one
>>>>> - support many new commands for cat-file. As I understand, it is not
>>>>> possible unless we finally move to ref-filter and print results also
>>>>> there. Oh, and I also need to rewrite docs in that case. And I decided
>>>>> to apply this in another patch. But, please, say your opinion, maybe
>>>>> we could do that here in some way.
>>>>
>>>> Yeah, I agree that it will cause changes to other users of ref-filter,
>>>> and you'd need to deal with documentation and tests there. But I think
>>>> we're going to have to do those things eventually (since supporting
>>>> those extra fields everywhere is part of the point of the project). And
>>>> by doing them now, I think it can make the transition for cat-file a lot
>>>> simpler, because we never have to puzzle over this intermediate state
>>>> where only some of the atoms are valid for cat-file.
>>>
>>> I agree that you will have to deal with documentation and tests at one
>>> point and that it could be a good idea to do that now.
>>>
>>> I wonder if it is possible to add atoms one by one into ref-filter and
>>> to add tests and documentation at the same time, instead of merging
>>> cat-file atoms with ref-filter atoms in one big step.
>>>
>>> When all the cat-file atoms have been added to ref-filter's
>>> valid_atom, maybe you could add ref-filter's atoms to cat-file's
>>> valid_cat_file_atom one by one and add tests and documentation at the
>>> same time.
>>>
>>> And then when ref-filter's valid_atom and cat-file's
>>> valid_cat_file_atom are identical you can remove cat-file's
>>> valid_cat_file_atom and maybe after that rename "ref-filter" to
>>> "format".
>>
>> I think it's important to finish migrating process at first. I mean,
>> now we are preparing and collecting everything in ref-filter, but we
>> make resulting string and print still in cat-file. And I am not sure,
>> but maybe it will not be possible to start using new atoms in cat-file
>> while some part of logic still differs.
>
> Ok, you can finish the migration process then.
>
>> And another thoughts here - we were thinking about creating format.h
>> but decided not to move forward with it, and now we are suffering
>> because of it. Can I create it right now or the history of commits
>> would be too dirty because of it?
>
> It would also make it difficult to refactor your patch series if there
> is a big move or renaming in the middle.
>
>> Also, do you mean just renaming of
>> ref-filter? I was thinking that I need to put formatting-related logic
>> to another file and leave all other stuff in ref-filter.
>
> Yeah, you can do both a move and a renaming.

Thanks for a response! That thought is not clear enough for me. Do you
want me to split ref-filter into 2 files (one is for formatting only
called format and other one is for anything else still called
ref-filter) - here is a second question by the way, do I need to
create only format.h (and leave all realizations in ref-filter.c), or
I also need to create format.c. Or, just to rename ref-filter into
format and that's all.

>
>> Anyway, your suggested steps looks good, and I am planning to
>> implement them later.
>
> Ok.
>
>> Let's discuss, what behavior we are waiting for
>> when atom seems useless for the command. Die or ignore?
>
> We could alternatively just emit a warning, but it looks like there
> are a lot of die() calls already in ref-filter.c, so I would suggest
> die().
>
>> And, which
>> atoms are useless (as I understand, "rest" and "deltabase" from
>> cat-file are useless for all ref-filter users, so the question is
>> about - am I right in it, and about ref-filter atoms for cat-file).
>
> For now and I think until the migration process is finished, you could
> just die() in case of any atom not already supported by the command.
>
>> I have never written any tests and docs for Git, I will try to explore
>> by myself how to do that, but if you have any special links/materials
>> about it - please send them to me :)
>
> I think that looking at the existing documentation and tests is
> probably the best way to learn about it.

Re: [PATCH v2 03/18] ref-filter: make valid_atom as function parameter

[Christian Couder]

On Fri, Jan 19, 2018 at 6:22 PM, Оля Тележная <olyatelezhnaya@gmail.com> wrote:
> 2018-01-19 20:14 GMT+03:00 Christian Couder <christian.couder@gmail.com>:
>> On Thu, Jan 18, 2018 at 7:20 AM, Оля Тележная <olyatelezhnaya@gmail.com> wrote:

>>> And another thoughts here - we were thinking about creating format.h
>>> but decided not to move forward with it, and now we are suffering
>>> because of it. Can I create it right now or the history of commits
>>> would be too dirty because of it?
>>
>> It would also make it difficult to refactor your patch series if there
>> is a big move or renaming in the middle.
>>
>>> Also, do you mean just renaming of
>>> ref-filter? I was thinking that I need to put formatting-related logic
>>> to another file and leave all other stuff in ref-filter.
>>
>> Yeah, you can do both a move and a renaming.
>
> Thanks for a response! That thought is not clear enough for me. Do you
> want me to split ref-filter into 2 files (one is for formatting only
> called format and other one is for anything else still called
> ref-filter) - here is a second question by the way, do I need to
> create only format.h (and leave all realizations in ref-filter.c), or
> I also need to create format.c. Or, just to rename ref-filter into
> format and that's all.

Just renaming ref-filter into format (including the filenames) will
probably be enough, but it's also possible that it will make more
sense to keep some code only relevant to ref filtering into
ref-filter.{c,h}. We will be in a better position to decide what we
should do when the migration is finished.

Re: [PATCH v2 03/18] ref-filter: make valid_atom as function parameter

[Jeff King]

On Fri, Jan 19, 2018 at 06:14:56PM +0100, Christian Couder wrote:

> > Let's discuss, what behavior we are waiting for
> > when atom seems useless for the command. Die or ignore?
> 
> We could alternatively just emit a warning, but it looks like there
> are a lot of die() calls already in ref-filter.c, so I would suggest
> die().

I actually think it makes sense to just expand nonsense into the empty
string, for two reasons:

  1. That's what ref-filter already does for the existing cases. For
     example, try:

       git for-each-ref --format='%(objecttype) %(authordate)'

     and you will see that the annotated tags just get a blank author
     field.

  2. I think we may end up with a case where we feed multiple objects
     via --batch-check, and the format is only nonsense for _some_ of
     them. E.g., I envision a world where you can do:

       git cat-file --batch-check='%(objecttype) %(refname)' <<-\EOF
       master
       12345abcde12345abcde12345abcde12345abcde
       EOF

     and get output like:

       commit refs/heads/master
       commit

     (i.e., if we would remember the refname discovered during the name
     resolution, we could still report it). It would be annoying if the
     second line caused us to die().

> > And, which
> > atoms are useless (as I understand, "rest" and "deltabase" from
> > cat-file are useless for all ref-filter users, so the question is
> > about - am I right in it, and about ref-filter atoms for cat-file).
> 
> For now and I think until the migration process is finished, you could
> just die() in case of any atom not already supported by the command.

I'm OK with dying in the interim if it's easier, though I suspect it is
not much extra work to just expand to the empty string in such cases. If
that's where we want to end up eventually, it may be worth going
straight there.

I also think %(deltabase) does make sense for anything that points to an
object. I suspect it's not all that _useful_ for for-each-ref, but that
doesn't mean we can't return the sensible thing if somebody asks for it.

I agree that %(rest) probably doesn't make any sense for a caller which
isn't parsing input.

-Peff

Re: [PATCH v2 03/18] ref-filter: make valid_atom as function parameter

[Christian Couder]

On Fri, Jan 19, 2018 at 6:23 PM, Jeff King <peff@peff.net> wrote:
> On Fri, Jan 19, 2018 at 06:14:56PM +0100, Christian Couder wrote:
>
>> > Let's discuss, what behavior we are waiting for
>> > when atom seems useless for the command. Die or ignore?
>>
>> We could alternatively just emit a warning, but it looks like there
>> are a lot of die() calls already in ref-filter.c, so I would suggest
>> die().
>
> I actually think it makes sense to just expand nonsense into the empty
> string, for two reasons:
>
>   1. That's what ref-filter already does for the existing cases. For
>      example, try:
>
>        git for-each-ref --format='%(objecttype) %(authordate)'
>
>      and you will see that the annotated tags just get a blank author
>      field.
>
>   2. I think we may end up with a case where we feed multiple objects
>      via --batch-check, and the format is only nonsense for _some_ of
>      them. E.g., I envision a world where you can do:
>
>        git cat-file --batch-check='%(objecttype) %(refname)' <<-\EOF
>        master
>        12345abcde12345abcde12345abcde12345abcde
>        EOF
>
>      and get output like:
>
>        commit refs/heads/master
>        commit
>
>      (i.e., if we would remember the refname discovered during the name
>      resolution, we could still report it). It would be annoying if the
>      second line caused us to die().

Yeah, ok, that makes sense.

>> > And, which
>> > atoms are useless (as I understand, "rest" and "deltabase" from
>> > cat-file are useless for all ref-filter users, so the question is
>> > about - am I right in it, and about ref-filter atoms for cat-file).
>>
>> For now and I think until the migration process is finished, you could
>> just die() in case of any atom not already supported by the command.
>
> I'm OK with dying in the interim if it's easier, though I suspect it is
> not much extra work to just expand to the empty string in such cases. If
> that's where we want to end up eventually, it may be worth going
> straight there.
>
> I also think %(deltabase) does make sense for anything that points to an
> object. I suspect it's not all that _useful_ for for-each-ref, but that
> doesn't mean we can't return the sensible thing if somebody asks for it.
>
> I agree that %(rest) probably doesn't make any sense for a caller which
> isn't parsing input.

Yeah, ok.

Re: [PATCH v2 03/18] ref-filter: make valid_atom as function parameter

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I also think %(deltabase) does make sense for anything that points to an
> object. I suspect it's not all that _useful_ for for-each-ref, but that
> doesn't mean we can't return the sensible thing if somebody asks for it.

This may not be a new issue (or any issue at all), but is the
ability to learn deltabase make any sense in the first place?

What should the code do when an object has three copies in the
repo, i.e. one as a base object (or a loose one), another as a
delta against an object, and the third one as a delta against
a different object?

Re: [PATCH v2 03/18] ref-filter: make valid_atom as function parameter

[Jeff King]

On Fri, Jan 19, 2018 at 10:47:57AM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > I also think %(deltabase) does make sense for anything that points to an
> > object. I suspect it's not all that _useful_ for for-each-ref, but that
> > doesn't mean we can't return the sensible thing if somebody asks for it.
> 
> This may not be a new issue (or any issue at all), but is the
> ability to learn deltabase make any sense in the first place?
> 
> What should the code do when an object has three copies in the
> repo, i.e. one as a base object (or a loose one), another as a
> delta against an object, and the third one as a delta against
> a different object?

This was a known issue when I introduced %(deltabase). The documentation
explicitly calls this out and makes no promises about which copy we
describe.

The %(objectsize:disk) atom has the same issue, too. See the CAVEATS
section of git-cat-file(1).

-Peff

[PATCH v2 14/18] ref-filter: get rid of expand_atom_into_fields

[Olga Telezhnaya]

Remove expand_atom_into_fields function 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 35e16cec6d862..93248ce18152f 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 expand_atom_into_fields(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)
-		expand_atom_into_fields(atom, atom_len, cat_file_info);
+	if (cat_file_info && !strcmp(valid_atoms[i].name, "rest"))
+		cat_file_info->split_on_whitespace = 1;
 	return at;
 }
 

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

[PATCH v2 09/18] cat-file: simplify expand_atom function

[Olga Telezhnaya]

Not sure, but looks like there is no need in that checking.
There is a checking before whether it is null and we die in such case.

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

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 9055fa3a8b0ae..dd43457c0ad7e 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -193,10 +193,9 @@ static void expand_atom(struct strbuf *sb, const char *atom, int 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))
+	else if (is_atom("rest", atom, len))
+		strbuf_addstr(sb, data->rest);
+	else if (is_atom("deltabase", atom, len))
 		strbuf_addstr(sb, oid_to_hex(&data->delta_base_oid));
 	else
 		die("unknown format element: %.*s", len, atom);

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

[PATCH v2 06/18] ref-filter: reuse parse_ref_filter_atom

[Olga Telezhnaya]

Continue migrating formatting logic from cat-file to ref-filter.
Reuse parse_ref_filter_atom for unifying all processes in ref-filter
and further reducing of expand_atom_into_fields function.

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 | 38 +++++++++++++++++++++++++++-----------
 1 file changed, 27 insertions(+), 11 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 6ca4a671086ee..bb09875e2dbf4 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 expand_atom_into_fields(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)
+		expand_atom_into_fields(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, ')');
@@ -735,10 +751,10 @@ 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)
-			expand_atom_into_fields(sp + 2, ep - sp - 2,
-						format->cat_file_data);
-		else {
+		if (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);
 			if (skip_prefix(used_atom[at].name, "color:", &color))

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

[PATCH v2 18/18] ref-filter: make cat_file_info independent

[Olga Telezhnaya]

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 |  2 +-
 ref-filter.c       | 28 ++++++++++++++--------------
 ref-filter.h       |  1 -
 3 files changed, 15 insertions(+), 16 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 19cee0d22fabe..ffa8e61213e36 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -289,6 +289,7 @@ static void batch_object_write(const char *obj_name, struct batch_options *opt,
 	item.start_of_request = obj_name;
 
 	if (populate_value(&item)) return;
+	data->type = item.type;
 
 	strbuf_expand(&buf, opt->format->format, expand_format, &item);
 	strbuf_addch(&buf, '\n');
@@ -392,7 +393,6 @@ static int batch_objects(struct batch_options *opt)
 	 * object.
 	 */
 	memset(&data, 0, sizeof(data));
-	opt->format->cat_file_data = &data;
 	opt->format->is_cat = 1;
 	opt->format->split_on_whitespace = &data.split_on_whitespace;
 	opt->format->all_objects = opt->all_objects;
diff --git a/ref-filter.c b/ref-filter.c
index 83bacdd8bdd78..aee6be32f53ef 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 int is_cat = 0;
 
 static void color_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *color_value)
@@ -256,9 +256,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);
 }
@@ -266,7 +266,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);
 }
@@ -274,7 +274,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);
 }
@@ -739,7 +739,6 @@ int verify_ref_format(struct ref_format *format)
 {
 	const char *cp, *sp;
 
-	cat_file_info = format->cat_file_data;
 	is_cat = format->is_cat;
 	format->need_color_reset_at_eol = 0;
 	for (cp = format->format; *cp && (sp = find_next(cp)); ) {
@@ -766,8 +765,8 @@ int verify_ref_format(struct ref_format *format)
 		format->need_color_reset_at_eol = 0;
 	if (is_cat && 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;
 }
@@ -1472,18 +1471,19 @@ int populate_value(struct ref_array_item *ref)
 	}
 
 	if (is_cat) {
-		if (!cat_file_info->skip_object_info &&
-		    sha1_object_info_extended(ref->objectname.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->objectname.hash, &cat_file_info.info,
 					      OBJECT_INFO_LOOKUP_REPLACE) < 0) {
 			const char *e = ref->start_of_request;
 			printf("%s missing\n", e ? e : oid_to_hex(&ref->objectname));
 			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;
 	}
 
 	/* Fill in specials first */
diff --git a/ref-filter.h b/ref-filter.h
index e588d1f50eef6..d3e0cbdd4ce6d 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -125,7 +125,6 @@ struct ref_format {
 	 * Helps to move all formatting logic from cat-file to ref-filter,
 	 * hopefully would be reduced later.
 	 */
-	struct expand_data *cat_file_data;
 	int is_cat;
 	int *split_on_whitespace;
 	int all_objects;

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

[PATCH v2 04/18] cat-file: move struct expand_data into ref-filter

[Olga Telezhnaya]

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 7655a9a726773..7fd5b960ad698 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 0d98342b34319..16d00e4b1bded 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/450

Re: [PATCH v2 04/18] cat-file: move struct expand_data into ref-filter

[Jeff King]

On Wed, Jan 10, 2018 at 09:36:41AM +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.

I think some of these will want to remain in cat-file.c. For instance,
split_on_whitespace is not something that ref-filter.c itself would care
about. I'd expect in the endgame for cat-file to keep its own
split_on_whitespace flag, and to set it based on the presence of the
%(rest) flag, which it can check by seeing if the "rest" atom is in the
"used_atom" list after parsing the format.

-Peff

Re: [PATCH v2 04/18] cat-file: move struct expand_data into ref-filter

[Оля Тележная]

2018-01-16 0:44 GMT+03:00 Jeff King <peff@peff.net>:
> On Wed, Jan 10, 2018 at 09:36:41AM +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.
>
> I think some of these will want to remain in cat-file.c. For instance,
> split_on_whitespace is not something that ref-filter.c itself would care
> about. I'd expect in the endgame for cat-file to keep its own
> split_on_whitespace flag, and to set it based on the presence of the
> %(rest) flag, which it can check by seeing if the "rest" atom is in the
> "used_atom" list after parsing the format.
>
> -Peff

Yes, maybe we will need to leave some part of expand_data variables.
But, if it would be only "split_on_whitespace", it's better to make
just one flag without any other stuff. Actually, I thought about
moving related logic to ref-filter also. Anyway, it's hard to say
exact things before we code everything. Do I need to fix commit
message and make it more soft?

Olga

Re: [PATCH v2 04/18] cat-file: move struct expand_data into ref-filter

[Jeff King]

On Tue, Jan 16, 2018 at 10:00:42AM +0300, Оля Тележная wrote:

> > I think some of these will want to remain in cat-file.c. For instance,
> > split_on_whitespace is not something that ref-filter.c itself would care
> > about. I'd expect in the endgame for cat-file to keep its own
> > split_on_whitespace flag, and to set it based on the presence of the
> > %(rest) flag, which it can check by seeing if the "rest" atom is in the
> > "used_atom" list after parsing the format.
> 
> Yes, maybe we will need to leave some part of expand_data variables.
> But, if it would be only "split_on_whitespace", it's better to make
> just one flag without any other stuff. Actually, I thought about
> moving related logic to ref-filter also. Anyway, it's hard to say
> exact things before we code everything. Do I need to fix commit
> message and make it more soft?

Yeah, I think it might just be split_on_whitespace, and that could live
on as a single variable in cat-file.c.

I don't think it makes sense to move that part to ref-filter, since it's
not about formatting at all. It's about how cat-file parses its input,
which is going to be unlike other ref-filter users (which don't
generally parse input at all).

-Peff

Re: [PATCH v2 04/18] cat-file: move struct expand_data into ref-filter

[Оля Тележная]

2018-01-18 0:45 GMT+03:00 Jeff King <peff@peff.net>:
> On Tue, Jan 16, 2018 at 10:00:42AM +0300, Оля Тележная wrote:
>
>> > I think some of these will want to remain in cat-file.c. For instance,
>> > split_on_whitespace is not something that ref-filter.c itself would care
>> > about. I'd expect in the endgame for cat-file to keep its own
>> > split_on_whitespace flag, and to set it based on the presence of the
>> > %(rest) flag, which it can check by seeing if the "rest" atom is in the
>> > "used_atom" list after parsing the format.
>>
>> Yes, maybe we will need to leave some part of expand_data variables.
>> But, if it would be only "split_on_whitespace", it's better to make
>> just one flag without any other stuff. Actually, I thought about
>> moving related logic to ref-filter also. Anyway, it's hard to say
>> exact things before we code everything. Do I need to fix commit
>> message and make it more soft?
>
> Yeah, I think it might just be split_on_whitespace, and that could live
> on as a single variable in cat-file.c.
>
> I don't think it makes sense to move that part to ref-filter, since it's
> not about formatting at all. It's about how cat-file parses its input,
> which is going to be unlike other ref-filter users (which don't
> generally parse input at all).
>
> -Peff

OK, I will leave split_on_whitespace in cat-file for now. :) There are
so many places that are more important now, in my opinion, so I am
planning just to leave this variable and do other stuff, and I will
return to this question later.

[PATCH v2 16/18] ref_format: add split_on_whitespace flag

[Olga Telezhnaya]

Add flag to ref_format struct so that we could pass needed info
to 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 | 1 +
 ref-filter.c       | 4 ++--
 ref-filter.h       | 1 +
 3 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 289912ab1f858..5aac10b9808ff 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -394,6 +394,7 @@ static int batch_objects(struct batch_options *opt)
 	memset(&data, 0, sizeof(data));
 	opt->format->cat_file_data = &data;
 	opt->format->is_cat = 1;
+	opt->format->split_on_whitespace = &data.split_on_whitespace;
 	verify_ref_format(opt->format);
 	if (opt->cmdmode)
 		data.split_on_whitespace = 1;
diff --git a/ref-filter.c b/ref-filter.c
index 2c955e90bb4c9..0fb33198fb450 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -494,8 +494,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 (is_cat && !strcmp(valid_atoms[i].name, "rest"))
-		cat_file_info->split_on_whitespace = 1;
+	if (!strcmp(valid_atom[i].name, "rest"))
+		*format->split_on_whitespace = 1;
 	return at;
 }
 
diff --git a/ref-filter.h b/ref-filter.h
index 12a938b0aa6a3..aaf1790e43e62 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -127,6 +127,7 @@ struct ref_format {
 	 */
 	struct expand_data *cat_file_data;
 	int is_cat;
+	int *split_on_whitespace;
 };
 
 #define REF_FORMAT_INIT { NULL, 0, -1 }

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

[PATCH v2 02/18] cat-file: reuse struct ref_format

[Olga Telezhnaya]

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

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index f783b39b9bd5c..7655a9a726773 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;
@@ -353,7 +354,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);
@@ -446,8 +447,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
@@ -456,7 +457,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;
@@ -548,7 +549,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;
 }
@@ -557,7 +558,8 @@ 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 ref_format format = REF_FORMAT_INIT;
+	struct batch_options batch = {&format};
 	int unknown_type = 0;
 
 	const struct option options[] = {

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

Re: [PATCH v2 02/18] cat-file: reuse struct ref_format

[Jeff King]

On Wed, Jan 10, 2018 at 09:36:41AM +0000, Olga Telezhnaya wrote:

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

OK, this makes sense (though again, at some point we want this to stop
being a "ref_format" and just be a "format").

>  struct batch_options {
> +	struct ref_format *format;

Does this need to be a pointer? We can just store the ref_format inside
the struct, right? And then...

> @@ -557,7 +558,8 @@ 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 ref_format format = REF_FORMAT_INIT;
> +	struct batch_options batch = {&format};
>  	int unknown_type = 0;

...here you would not need the extra local variable. You can initialize
it like:

  struct batch_options batch = { REF_FORMAT_INIT };

-Peff

Re: [PATCH v2 02/18] cat-file: reuse struct ref_format

[Оля Тележная]

2018-01-16 0:37 GMT+03:00 Jeff King <peff@peff.net>:
> On Wed, Jan 10, 2018 at 09:36:41AM +0000, Olga Telezhnaya wrote:
>
>> Start using ref_format struct instead of simple char*.
>> Need that for further reusing of formatting logic from ref-filter.
>
> OK, this makes sense (though again, at some point we want this to stop
> being a "ref_format" and just be a "format").
>
>>  struct batch_options {
>> +     struct ref_format *format;
>
> Does this need to be a pointer? We can just store the ref_format inside
> the struct, right? And then...
>
>> @@ -557,7 +558,8 @@ 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 ref_format format = REF_FORMAT_INIT;
>> +     struct batch_options batch = {&format};
>>       int unknown_type = 0;
>
> ...here you would not need the extra local variable. You can initialize
> it like:
>
>   struct batch_options batch = { REF_FORMAT_INIT };
>
> -Peff

Thanks a lot!
Fixed, please check new version here: https://github.com/git/git/pull/450
If everything else is OK, I will send it to the mailing list.
As I said in other email threads, not sure that we need to include
last commit ("make valid_atom general again") into a new patch.

Thanks again,
Olga

[PATCH v2 17/18] cat-file: move skip_object_info into ref-filter

[Olga Telezhnaya]

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 5aac10b9808ff..19cee0d22fabe 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -395,16 +395,11 @@ static int batch_objects(struct batch_options *opt)
 	opt->format->cat_file_data = &data;
 	opt->format->is_cat = 1;
 	opt->format->split_on_whitespace = &data.split_on_whitespace;
+	opt->format->all_objects = opt->all_objects;
 	verify_ref_format(opt->format);
 	if (opt->cmdmode)
 		data.split_on_whitespace = 1;
 
-	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 0fb33198fb450..83bacdd8bdd78 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -764,6 +764,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 (is_cat && 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 aaf1790e43e62..e588d1f50eef6 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -128,6 +128,7 @@ struct ref_format {
 	struct expand_data *cat_file_data;
 	int is_cat;
 	int *split_on_whitespace;
+	int all_objects;
 };
 
 #define REF_FORMAT_INIT { NULL, 0, -1 }

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

[PATCH v2 07/18] cat-file: remove unused code

[Olga Telezhnaya]

No further need in mark_query parameter.
All logic related to expand_atom_into_fields is not needed here also,
we are doing that in 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 | 25 +------------------------
 ref-filter.h       |  6 ------
 2 files changed, 1 insertion(+), 30 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 0a3f4a47bf1ae..9055fa3a8b0ae 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 expand_atom_into_fields(struct strbuf *sb, 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)
 {
@@ -232,11 +213,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)
-		expand_atom_into_fields(sb, 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;
 }
 
diff --git a/ref-filter.h b/ref-filter.h
index 97169548939cd..590a60ffe034d 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

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

[PATCH v2 10/18] cat-file: get rid of duplicate checking

[Olga Telezhnaya]

We could remove this because we have already checked that
at verify_ref_format function in 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 | 2 --
 1 file changed, 2 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index dd43457c0ad7e..1f331559e55c7 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -197,8 +197,6 @@ static void expand_atom(struct strbuf *sb, const char *atom, int len,
 		strbuf_addstr(sb, data->rest);
 	else if (is_atom("deltabase", atom, len))
 		strbuf_addstr(sb, oid_to_hex(&data->delta_base_oid));
-	else
-		die("unknown format element: %.*s", len, atom);
 }
 
 static size_t expand_format(struct strbuf *sb, const char *start, void *vdata)

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

[PATCH v2 11/18] cat-file: start use ref_array_item struct

[Olga Telezhnaya]

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 | 30 +++++++++++++++++++-----------
 ref-filter.h       |  5 +++++
 2 files changed, 24 insertions(+), 11 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 1f331559e55c7..1c92194faaede 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -183,26 +183,26 @@ 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))
-		strbuf_addstr(sb, data->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;
@@ -210,7 +210,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;
 }
 
@@ -282,6 +282,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;
 
 	if (!data->skip_object_info &&
 	    sha1_object_info_extended(data->oid.hash, &data->info,
@@ -292,7 +293,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 590a60ffe034d..9bd36243481b4 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/450

[PATCH v2 12/18] ref-filter: make populate_value global

[Olga Telezhnaya]

Make function global for further using in cat-file.
Also added return value for handling errors.

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 | 4 ++--
 ref-filter.h | 3 +++
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 575c5351d0f79..c15906cb091c7 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1454,7 +1454,7 @@ static void need_object(struct ref_array_item *ref) {
 /*
  * Parse the object referred by ref, and grab needed value.
  */
-static void populate_value(struct ref_array_item *ref)
+int populate_value(struct ref_array_item *ref)
 {
 	int i;
 
@@ -1575,7 +1575,7 @@ static void populate_value(struct ref_array_item *ref)
 			break;
 		}
 	}
-	return;
+	return 0;
 }
 
 /*
diff --git a/ref-filter.h b/ref-filter.h
index 9bd36243481b4..6df45c5bd9dcb 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -176,4 +176,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/450

[PATCH v2 15/18] ref-filter: add is_cat flag

[Olga Telezhnaya]

Add is_cat flag, further it helps to get rid of cat_file_data field
in ref_format.

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 | 1 +
 ref-filter.c       | 8 +++++---
 ref-filter.h       | 1 +
 3 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index e11dbf88e386c..289912ab1f858 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -393,6 +393,7 @@ static int batch_objects(struct batch_options *opt)
 	 */
 	memset(&data, 0, sizeof(data));
 	opt->format->cat_file_data = &data;
+	opt->format->is_cat = 1;
 	verify_ref_format(opt->format);
 	if (opt->cmdmode)
 		data.split_on_whitespace = 1;
diff --git a/ref-filter.c b/ref-filter.c
index 93248ce18152f..2c955e90bb4c9 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -101,6 +101,7 @@ static struct used_atom {
 } *used_atom;
 static int used_atom_cnt, need_tagged, need_symref;
 struct expand_data *cat_file_info;
+static int is_cat = 0;
 
 static void color_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *color_value)
 {
@@ -493,7 +494,7 @@ 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_atoms[i].name, "rest"))
+	if (is_cat && !strcmp(valid_atoms[i].name, "rest"))
 		cat_file_info->split_on_whitespace = 1;
 	return at;
 }
@@ -739,6 +740,7 @@ int verify_ref_format(struct ref_format *format)
 	const char *cp, *sp;
 
 	cat_file_info = format->cat_file_data;
+	is_cat = format->is_cat;
 	format->need_color_reset_at_eol = 0;
 	for (cp = format->format; *cp && (sp = find_next(cp)); ) {
 		const char *color, *ep = strchr(sp, ')');
@@ -748,7 +750,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 (is_cat) {
 			at = parse_ref_filter_atom(format, valid_cat_file_atom,
 						   ARRAY_SIZE(valid_cat_file_atom), sp + 2, ep);
 		} else {
@@ -1464,7 +1466,7 @@ int populate_value(struct ref_array_item *ref)
 			ref->symref = "";
 	}
 
-	if (cat_file_info) {
+	if (is_cat) {
 		if (!cat_file_info->skip_object_info &&
 		    sha1_object_info_extended(ref->objectname.hash, &cat_file_info->info,
 					      OBJECT_INFO_LOOKUP_REPLACE) < 0) {
diff --git a/ref-filter.h b/ref-filter.h
index 0c7253913735b..12a938b0aa6a3 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -126,6 +126,7 @@ struct ref_format {
 	 * hopefully would be reduced later.
 	 */
 	struct expand_data *cat_file_data;
+	int is_cat;
 };
 
 #define REF_FORMAT_INIT { NULL, 0, -1 }

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

[PATCH v2 05/18] cat-file: start migrating to ref-filter

[Olga Telezhnaya]

Start moving all formatting stuff from cat-file to ref-filter.
Start from simple moving, it would be integrated into
all ref-filter processes 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 |  5 ++---
 ref-filter.c       | 41 ++++++++++++++++++++++++++++++++++++-----
 ref-filter.h       |  6 ++++++
 3 files changed, 44 insertions(+), 8 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 7fd5b960ad698..0a3f4a47bf1ae 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -420,9 +420,8 @@ static int batch_objects(struct batch_options *opt)
 	 * 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 32b7e918bca75..6ca4a671086ee 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 expand_atom_into_fields(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)
+			expand_atom_into_fields(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 16d00e4b1bded..97169548939cd 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -119,6 +119,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/450

Re: [PATCH v2 01/18] cat-file: split expand_atom into 2 functions

[Jeff King]

On Wed, Jan 10, 2018 at 09:36:41AM +0000, Olga Telezhnaya wrote:

> Split expand_atom function into 2 different functions,
> expand_atom_into_fields prepares variable for further filling,
> (new) expand_atom creates resulting string.
> Need that for further reusing of formatting logic from ref-filter.

This commit puzzled me, and I had to look ahead to try to figure out why
we want this split (because on its face, the split is bad, since it
duplicates the list).

Later, the preparation step goes away, but we still are left with
expand_atom(). That's because the preparation was all moved into
ref-filter.c, where we rely on populate_value() to fill in the values,
and then we pick them out with our own formats.

That works, but I don't think it's where we want to end up in the long
run. Because:

  1. We still have the set of formats duplicated between expand_atom()
     and the "preparation" step. It's just that the preparation is now
     in ref-filter.c. What happens if ref-filter.c learns new formatting
     placeholders (or options for those placeholders) that cat-file.c
     doesn't, or vice versa? The two have to be kept in sync.

  2. We're missing out on all of the other placeholders that ref-filter
     knows about. Not all of them are meaningful (e.g., %(refname)
     wouldn't make sense here), but part of our goal is to support the
     same set of placeholders as much as possible. Some obvious ones
     that ought to work for cat-file: %(objectname:short), %(if), and
     things like %(subject) when the appropriate object type is used.

In other words, I think the endgame is that expand_atom() isn't there at
all, and we're calling the equivalent of format_ref_item() for each
object (except that in a unified formatting world, it probably doesn't
have the word "ref" in it, since that's just one of the items a caller
might pass in).

-Peff

Re: [PATCH v2 01/18] cat-file: split expand_atom into 2 functions

[Jeff King]

On Mon, Jan 15, 2018 at 04:33:35PM -0500, Jeff King wrote:

> That works, but I don't think it's where we want to end up in the long
> run. Because:
> 
>   1. We still have the set of formats duplicated between expand_atom()
>      and the "preparation" step. It's just that the preparation is now
>      in ref-filter.c. What happens if ref-filter.c learns new formatting
>      placeholders (or options for those placeholders) that cat-file.c
>      doesn't, or vice versa? The two have to be kept in sync.
> 
>   2. We're missing out on all of the other placeholders that ref-filter
>      knows about. Not all of them are meaningful (e.g., %(refname)
>      wouldn't make sense here), but part of our goal is to support the
>      same set of placeholders as much as possible. Some obvious ones
>      that ought to work for cat-file: %(objectname:short), %(if), and
>      things like %(subject) when the appropriate object type is used.
> 
> In other words, I think the endgame is that expand_atom() isn't there at
> all, and we're calling the equivalent of format_ref_item() for each
> object (except that in a unified formatting world, it probably doesn't
> have the word "ref" in it, since that's just one of the items a caller
> might pass in).

I read carefully up through about patch 6, and then skimmed through the
rest. I think we really want to push this in the direction of more
unification, as above. I know that the patches here may be a step on the
way, but I had a hard time evaluating each patch to see if it was
leading us in the right direction.

I think what would help for reviewing this is:

  1. Start with a cover letter that makes it clear what the end state of
     the series is, and why that's the right place to end up.

  2. Include a rough roadmap of the patches in the cover letter.
     Hopefully they should group into a few obvious steps (like "in
     patches 1-5, we're teaching ref-filter to support everything that
     cat-file can do, then in 6-10 we're preparing cat-file for the
     conversion, and then in 11 we do the conversion"). If it doesn't
     form a coherent narrative, then it may be worth stepping back and
     thinking about combining or reordering the patches in a different
     way, so that the progression becomes more plain.

     I think one of the things that makes the progression here hard to
     understand (for me, anyway) is that it's "inside out" of what I'd
     expect. There's a lot of code movement happening first, and then
     refactoring and simplifying after that. So between those two steps,
     there's a lot of interim ugliness (e.g., having to reach across
     module boundaries to look at expand_data). It's hard to tell when
     looking at each individual patch how necessary the ugliness is, and
     whether and when it's going to go away later in the series.

There's a lot of good work here, and you've figured out a lot about how
the two systems function. I think we just need to rearrange things a bit
to make sure each step is moving in the right direction.

-Peff

Re: [PATCH v2 01/18] cat-file: split expand_atom into 2 functions

[Оля Тележная]

2018-01-16 1:09 GMT+03:00 Jeff King <peff@peff.net>:
> On Mon, Jan 15, 2018 at 04:33:35PM -0500, Jeff King wrote:
>
>> That works, but I don't think it's where we want to end up in the long
>> run.

I absolutely agree, I want to merge current edits and then continue
migrating process. And hopefully second part of the function will also
be removed.

>> Because:
>>
>>   1. We still have the set of formats duplicated between expand_atom()
>>      and the "preparation" step. It's just that the preparation is now
>>      in ref-filter.c. What happens if ref-filter.c learns new formatting
>>      placeholders (or options for those placeholders) that cat-file.c
>>      doesn't, or vice versa? The two have to be kept in sync.
>>
>>   2. We're missing out on all of the other placeholders that ref-filter
>>      knows about. Not all of them are meaningful (e.g., %(refname)
>>      wouldn't make sense here), but part of our goal is to support the
>>      same set of placeholders as much as possible. Some obvious ones
>>      that ought to work for cat-file: %(objectname:short), %(if), and
>>      things like %(subject) when the appropriate object type is used.
>>
>> In other words, I think the endgame is that expand_atom() isn't there at
>> all, and we're calling the equivalent of format_ref_item() for each
>> object (except that in a unified formatting world, it probably doesn't
>> have the word "ref" in it, since that's just one of the items a caller
>> might pass in).

Agree! I want to merge current edits, then create format.h file and
make some renames, then finish migrating process to new format.h and
support all new meaningful tags.

>
> I read carefully up through about patch 6, and then skimmed through the
> rest. I think we really want to push this in the direction of more
> unification, as above. I know that the patches here may be a step on the
> way, but I had a hard time evaluating each patch to see if it was
> leading us in the right direction.
>
> I think what would help for reviewing this is:
>
>   1. Start with a cover letter that makes it clear what the end state of
>      the series is, and why that's the right place to end up.
>
>   2. Include a rough roadmap of the patches in the cover letter.
>      Hopefully they should group into a few obvious steps (like "in
>      patches 1-5, we're teaching ref-filter to support everything that
>      cat-file can do, then in 6-10 we're preparing cat-file for the
>      conversion, and then in 11 we do the conversion"). If it doesn't
>      form a coherent narrative, then it may be worth stepping back and
>      thinking about combining or reordering the patches in a different
>      way, so that the progression becomes more plain.
>
>      I think one of the things that makes the progression here hard to
>      understand (for me, anyway) is that it's "inside out" of what I'd
>      expect. There's a lot of code movement happening first, and then
>      refactoring and simplifying after that. So between those two steps,
>      there's a lot of interim ugliness (e.g., having to reach across
>      module boundaries to look at expand_data). It's hard to tell when
>      looking at each individual patch how necessary the ugliness is, and
>      whether and when it's going to go away later in the series.
>
> There's a lot of good work here, and you've figured out a lot about how
> the two systems function. I think we just need to rearrange things a bit
> to make sure each step is moving in the right direction.
>
> -Peff

As I said, I am sure that I will continue working on that, so this is
not the end state. So I am not sure that I am able to write finalizing
messages for now. But, if we merge current edits, it will be much
easier for me to continue working on that (next patch would be about
creating format.h and I am afraid of some merge conflicts if I will
develop absolutely all process from start to finish in my branch, it
takes time. It's not a big problem, but, if we find final goal
worthwhile, so maybe we could go to it step-by-step).

Re: [PATCH v2 01/18] cat-file: split expand_atom into 2 functions

[Jeff King]

On Tue, Jan 16, 2018 at 10:22:23AM +0300, Оля Тележная wrote:

> >> In other words, I think the endgame is that expand_atom() isn't there at
> >> all, and we're calling the equivalent of format_ref_item() for each
> >> object (except that in a unified formatting world, it probably doesn't
> >> have the word "ref" in it, since that's just one of the items a caller
> >> might pass in).
> 
> Agree! I want to merge current edits, then create format.h file and
> make some renames, then finish migrating process to new format.h and
> support all new meaningful tags.

I think we have a little bit of chicken and egg there, though. I'm
having trouble reviewing the current work, because it's hard to evaluate
whether it's doing the right thing without seeing the end state. So what
I was suggesting in my earlier mails was that we actually _not_ try to
merge this series, but use its components and ideas to build a new
series that does things in a bit different order.

Some of the patches here would end up thrown out, and some would get
cherry-picked into the new series. And some would have some conflicts
and need to get parts rewritten to fit into the new order.

-Peff

Re: [PATCH v2 01/18] cat-file: split expand_atom into 2 functions

[Christian Couder]

On Wed, Jan 17, 2018 at 10:49 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Jan 16, 2018 at 10:22:23AM +0300, Оля Тележная wrote:
>
>> >> In other words, I think the endgame is that expand_atom() isn't there at
>> >> all, and we're calling the equivalent of format_ref_item() for each
>> >> object (except that in a unified formatting world, it probably doesn't
>> >> have the word "ref" in it, since that's just one of the items a caller
>> >> might pass in).
>>
>> Agree! I want to merge current edits, then create format.h file and
>> make some renames, then finish migrating process to new format.h and
>> support all new meaningful tags.
>
> I think we have a little bit of chicken and egg there, though. I'm
> having trouble reviewing the current work, because it's hard to evaluate
> whether it's doing the right thing without seeing the end state.

Yeah, to me it feels like you are at a middle point and there are many
ways to go forward.

As I wrote in another email though, I think it might be a good time to
consolidate new functionality by adding tests (and perhaps
documentation at the same time) for each new atom that is added to
ref-filter or cat-file. It will help you refactor the code and your
patch series later without breaking the new functionality.

> So what
> I was suggesting in my earlier mails was that we actually _not_ try to
> merge this series, but use its components and ideas to build a new
> series that does things in a bit different order.

Yeah, I think you will have to do that, but the tests that you can add
now for the new features will help you when you will build the new
series.

And hopefully it will not be too much work to create this new series
as you will perhaps be able to just use the interactive rebase to
build it.

I also don't think it's a big problem if the current patch series gets
quite long before you start creating a new series.

Re: [PATCH v2 01/18] cat-file: split expand_atom into 2 functions

[Оля Тележная]

2018-01-18 2:04 GMT+03:00 Christian Couder <christian.couder@gmail.com>:
> On Wed, Jan 17, 2018 at 10:49 PM, Jeff King <peff@peff.net> wrote:
>> On Tue, Jan 16, 2018 at 10:22:23AM +0300, Оля Тележная wrote:
>>
>>> >> In other words, I think the endgame is that expand_atom() isn't there at
>>> >> all, and we're calling the equivalent of format_ref_item() for each
>>> >> object (except that in a unified formatting world, it probably doesn't
>>> >> have the word "ref" in it, since that's just one of the items a caller
>>> >> might pass in).
>>>
>>> Agree! I want to merge current edits, then create format.h file and
>>> make some renames, then finish migrating process to new format.h and
>>> support all new meaningful tags.
>>
>> I think we have a little bit of chicken and egg there, though. I'm
>> having trouble reviewing the current work, because it's hard to evaluate
>> whether it's doing the right thing without seeing the end state.
>
> Yeah, to me it feels like you are at a middle point and there are many
> ways to go forward.

OK. Maybe I misunderstood you and Jeff in our call, I thought that was
your idea to make a merge now, sorry. I will continue my work here.

>
> As I wrote in another email though, I think it might be a good time to
> consolidate new functionality by adding tests (and perhaps
> documentation at the same time) for each new atom that is added to
> ref-filter or cat-file. It will help you refactor the code and your
> patch series later without breaking the new functionality.
>
>> So what
>> I was suggesting in my earlier mails was that we actually _not_ try to
>> merge this series, but use its components and ideas to build a new
>> series that does things in a bit different order.
>
> Yeah, I think you will have to do that, but the tests that you can add
> now for the new features will help you when you will build the new
> series.
>
> And hopefully it will not be too much work to create this new series
> as you will perhaps be able to just use the interactive rebase to
> build it.
>
> I also don't think it's a big problem if the current patch series gets
> quite long before you start creating a new series.

Re: [PATCH v2 01/18] cat-file: split expand_atom into 2 functions

[Christian Couder]

On Thu, Jan 18, 2018 at 7:22 AM, Оля Тележная <olyatelezhnaya@gmail.com> wrote:
> 2018-01-18 2:04 GMT+03:00 Christian Couder <christian.couder@gmail.com>:
>> On Wed, Jan 17, 2018 at 10:49 PM, Jeff King <peff@peff.net> wrote:
>>> On Tue, Jan 16, 2018 at 10:22:23AM +0300, Оля Тележная wrote:
>>>
>>>> >> In other words, I think the endgame is that expand_atom() isn't there at
>>>> >> all, and we're calling the equivalent of format_ref_item() for each
>>>> >> object (except that in a unified formatting world, it probably doesn't
>>>> >> have the word "ref" in it, since that's just one of the items a caller
>>>> >> might pass in).
>>>>
>>>> Agree! I want to merge current edits, then create format.h file and
>>>> make some renames, then finish migrating process to new format.h and
>>>> support all new meaningful tags.
>>>
>>> I think we have a little bit of chicken and egg there, though. I'm
>>> having trouble reviewing the current work, because it's hard to evaluate
>>> whether it's doing the right thing without seeing the end state.
>>
>> Yeah, to me it feels like you are at a middle point and there are many
>> ways to go forward.
>
> OK. Maybe I misunderstood you and Jeff in our call, I thought that was
> your idea to make a merge now, sorry. I will continue my work here.

If you think you can now prepare a patch series that looks like it is
going in the right direction, then yes you can do that. But after
looking at the current patch series I agree with Peff that, as it
doesn't look finished, it's difficult to tell if all the steps are
good.

In general it is a good idea to try to merge things as soon possible,
but for that to be possible the state of the code that we want to be
merged should look quite stable, and it doesn't look very stable to
me.