# [PATCH RFC 23/24] cat-file: tests for new atoms added

33 messages from 2018-01-26 to 2018-01-31. Participants: Olga Telezhnaya, Junio C Hamano, Оля Тележная.
Thread: https://gitlist.dev/t/47700

## Olga Telezhnaya, 2018-01-26 19:43

Subject: [PATCH RFC 23/24] cat-file: tests for new atoms added
Message-ID: <0102016133ff3b5d-2a898107-6989-411d-b7c5-f349e897873e-000000@eu-west-1.amazonses.com>
URL: https://gitlist.dev/e/0102016133ff3b5d-2a898107-6989-411d-b7c5-f349e897873e-000000%40eu-west-1.amazonses.com
In-Reply-To: <0102016133ff3a86-44d354ec-13c6-4c38-bc75-1ba4422db5a7-000000@eu-west-1.amazonses.com>

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

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

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

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

```

## Olga Telezhnaya, 2018-01-26 19:43

Subject: [PATCH RFC 18/24] ref-filter: make valid_atom general again
Message-ID: <0102016133ff3b59-b5619bdf-930f-4cf7-8dd4-e36f8e3635ff-000000@eu-west-1.amazonses.com>
URL: https://gitlist.dev/e/0102016133ff3b59-b5619bdf-930f-4cf7-8dd4-e36f8e3635ff-000000%40eu-west-1.amazonses.com
In-Reply-To: <0102016133ff3a86-44d354ec-13c6-4c38-bc75-1ba4422db5a7-000000@eu-west-1.amazonses.com>

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

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

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

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

```

## Olga Telezhnaya, 2018-01-26 19:43

Subject: [PATCH RFC 01/24] ref-filter: get rid of goto
Message-ID: <0102016133ff3a86-44d354ec-13c6-4c38-bc75-1ba4422db5a7-000000@eu-west-1.amazonses.com>
URL: https://gitlist.dev/e/0102016133ff3a86-44d354ec-13c6-4c38-bc75-1ba4422db5a7-000000%40eu-west-1.amazonses.com

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

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

diff --git a/ref-filter.c b/ref-filter.c
index f9e25aea7a97e..37337b57aacf4 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1354,16 +1354,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));
 
@@ -1477,53 +1521,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/452

```

## Olga Telezhnaya, 2018-01-26 19:43

Subject: [PATCH RFC 10/24] ref-filter: make populate_value global
Message-ID: <0102016133ff3b05-802b1468-13cf-453f-801f-d5530f2f94ac-000000@eu-west-1.amazonses.com>
URL: https://gitlist.dev/e/0102016133ff3b05-802b1468-13cf-453f-801f-d5530f2f94ac-000000%40eu-west-1.amazonses.com
In-Reply-To: <0102016133ff3a86-44d354ec-13c6-4c38-bc75-1ba4422db5a7-000000@eu-west-1.amazonses.com>

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

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

diff --git a/ref-filter.c b/ref-filter.c
index 95c85009f1f58..e69dd1ff5091f 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 int populate_value(struct ref_array_item *ref)
+int populate_value(struct ref_array_item *ref)
 {
 	int i;
 
diff --git a/ref-filter.h b/ref-filter.h
index 7aaf82799ec2d..6373167aaacd7 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/452

```

## Olga Telezhnaya, 2018-01-26 19:43

Subject: [PATCH RFC 22/24] for-each-ref: tests for new atoms added
Message-ID: <0102016133ff3b7d-d733e1b6-eb98-4033-ba86-83fc6323887b-000000@eu-west-1.amazonses.com>
URL: https://gitlist.dev/e/0102016133ff3b7d-d733e1b6-eb98-4033-ba86-83fc6323887b-000000%40eu-west-1.amazonses.com
In-Reply-To: <0102016133ff3a86-44d354ec-13c6-4c38-bc75-1ba4422db5a7-000000@eu-west-1.amazonses.com>

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

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

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

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

```

## Olga Telezhnaya, 2018-01-26 19:43

Subject: [PATCH RFC 09/24] cat-file: start use ref_array_item struct
Message-ID: <0102016133ff3b28-9969f573-5976-45d9-a51a-4edba26d95c2-000000@eu-west-1.amazonses.com>
URL: https://gitlist.dev/e/0102016133ff3b28-9969f573-5976-45d9-a51a-4edba26d95c2-000000%40eu-west-1.amazonses.com
In-Reply-To: <0102016133ff3a86-44d354ec-13c6-4c38-bc75-1ba4422db5a7-000000@eu-west-1.amazonses.com>

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

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

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

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

```

## Olga Telezhnaya, 2018-01-26 19:43

Subject: [PATCH RFC 19/24] ref-filter: make populate_value internal again
Message-ID: <0102016133ff3b4c-a68ff848-85c3-40e0-acf1-3539e263bcd2-000000@eu-west-1.amazonses.com>
URL: https://gitlist.dev/e/0102016133ff3b4c-a68ff848-85c3-40e0-acf1-3539e263bcd2-000000%40eu-west-1.amazonses.com
In-Reply-To: <0102016133ff3a86-44d354ec-13c6-4c38-bc75-1ba4422db5a7-000000@eu-west-1.amazonses.com>

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

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

diff --git a/ref-filter.c b/ref-filter.c
index 4d0d4f081227d..c3a5c3897295f 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1476,7 +1476,7 @@ static int check_and_fill_for_cat(struct ref_array_item *ref)
 /*
  * Parse the object referred by ref, and grab needed value.
  */
-int populate_value(struct ref_array_item *ref)
+static int populate_value(struct ref_array_item *ref)
 {
 	int i;
 
diff --git a/ref-filter.h b/ref-filter.h
index d8064086f36a9..285a2311f6cc9 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -172,9 +172,6 @@ void setup_ref_filter_porcelain_msg(void);
 void pretty_print_ref(const char *name, const unsigned char *sha1,
 		      const struct ref_format *format);
 
-/* Fill the values of request and prepare all data for final string creation */
-int populate_value(struct ref_array_item *ref);
-
 /* Search for atom "rest" in given format. */
 int is_rest_atom_used(const struct ref_format *format);
 

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

```

## Olga Telezhnaya, 2018-01-26 19:43

Subject: [PATCH RFC 03/24] cat-file: split expand_atom into 2 functions
Message-ID: <0102016133ff3afb-6bab2c11-e0f3-4bef-9626-fbcc826bcb15-000000@eu-west-1.amazonses.com>
URL: https://gitlist.dev/e/0102016133ff3afb-6bab2c11-e0f3-4bef-9626-fbcc826bcb15-000000%40eu-west-1.amazonses.com
In-Reply-To: <0102016133ff3a86-44d354ec-13c6-4c38-bc75-1ba4422db5a7-000000@eu-west-1.amazonses.com>

```
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/452

```

## Olga Telezhnaya, 2018-01-26 19:43

Subject: [PATCH RFC 04/24] cat-file: reuse struct ref_format
Message-ID: <0102016133ff3af0-0ebed14d-deb5-4744-9267-4c5b0c6a30c8-000000@eu-west-1.amazonses.com>
URL: https://gitlist.dev/e/0102016133ff3af0-0ebed14d-deb5-4744-9267-4c5b0c6a30c8-000000%40eu-west-1.amazonses.com
In-Reply-To: <0102016133ff3a86-44d354ec-13c6-4c38-bc75-1ba4422db5a7-000000@eu-west-1.amazonses.com>

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

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

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index f783b39b9bd5c..65c300184cab8 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,7 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
 {
 	int opt = 0;
 	const char *exp_type = NULL, *obj_name = NULL;
-	struct batch_options batch = {0};
+	struct batch_options batch = { REF_FORMAT_INIT };
 	int unknown_type = 0;
 
 	const struct option options[] = {

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

```

## Olga Telezhnaya, 2018-01-26 19:43

Subject: [PATCH RFC 24/24] cat-file: update of docs
Message-ID: <0102016133ff3b58-dd205bee-24b9-4827-907c-eaf4a15c6f00-000000@eu-west-1.amazonses.com>
URL: https://gitlist.dev/e/0102016133ff3b58-dd205bee-24b9-4827-907c-eaf4a15c6f00-000000%40eu-west-1.amazonses.com
In-Reply-To: <0102016133ff3a86-44d354ec-13c6-4c38-bc75-1ba4422db5a7-000000@eu-west-1.amazonses.com>

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

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

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

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

```

## Olga Telezhnaya, 2018-01-26 19:43

Subject: [PATCH RFC 20/24] ref-filter: unifying formatting of cat-file opts
Message-ID: <0102016133ff3b49-fe78f01d-12a2-4f53-afae-fe88d9d73754-000000@eu-west-1.amazonses.com>
URL: https://gitlist.dev/e/0102016133ff3b49-fe78f01d-12a2-4f53-afae-fe88d9d73754-000000%40eu-west-1.amazonses.com
In-Reply-To: <0102016133ff3a86-44d354ec-13c6-4c38-bc75-1ba4422db5a7-000000@eu-west-1.amazonses.com>

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

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

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

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

```

## Olga Telezhnaya, 2018-01-26 19:43

Subject: [PATCH RFC 13/24] ref-filter: add is_cat flag
Message-ID: <0102016133ff3b3b-a0ab669e-bc4b-4a25-ac66-5365c56856b1-000000@eu-west-1.amazonses.com>
URL: https://gitlist.dev/e/0102016133ff3b3b-a0ab669e-bc4b-4a25-ac66-5365c56856b1-000000%40eu-west-1.amazonses.com
In-Reply-To: <0102016133ff3a86-44d354ec-13c6-4c38-bc75-1ba4422db5a7-000000@eu-west-1.amazonses.com>

```
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 c2ca645662ae7..8f34d085962ed 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;
 	verify_ref_format(&opt->format);
 	if (opt->cmdmode)
 		data.split_on_whitespace = 1;
diff --git a/ref-filter.c b/ref-filter.c
index 8d10d356609e7..3b61e790e90d1 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_atom[i].name, "rest"))
+	if (is_cat && !strcmp(valid_atom[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 {
@@ -1481,7 +1483,7 @@ int populate_value(struct ref_array_item *ref)
 			ref->symref = "";
 	}
 
-	if (cat_file_info && check_and_fill_for_cat(ref))
+	if (is_cat && check_and_fill_for_cat(ref))
 		return -1;
 
 	/* Fill in specials first */
diff --git a/ref-filter.h b/ref-filter.h
index e41d2913c0fff..c848370bed84a 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/452

```

## Olga Telezhnaya, 2018-01-26 19:43

Subject: [PATCH RFC 17/24] cat-file: reuse printing logic from ref-filter
Message-ID: <0102016133ff3b3f-26fceb2a-c1ef-447b-9013-caccccc052f9-000000@eu-west-1.amazonses.com>
URL: https://gitlist.dev/e/0102016133ff3b3f-26fceb2a-c1ef-447b-9013-caccccc052f9-000000%40eu-west-1.amazonses.com
In-Reply-To: <0102016133ff3a86-44d354ec-13c6-4c38-bc75-1ba4422db5a7-000000@eu-west-1.amazonses.com>

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

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

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index bd803856537b8..26b35bef4ba02 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -176,45 +176,6 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
 	return 0;
 }
 
-static int is_atom(const char *atom, const char *s, int slen)
-{
-	int alen = strlen(atom);
-	return alen == slen && !memcmp(atom, s, alen);
-}
-
-static void expand_atom(struct strbuf *sb, const char *atom, int len,
-			 struct ref_array_item *item)
-{
-	if (is_atom("objectname", atom, len))
-		strbuf_addstr(sb, oid_to_hex(&item->objectname));
-	else if (is_atom("objecttype", atom, len))
-		strbuf_addstr(sb, typename(item->type));
-	else if (is_atom("objectsize", atom, len))
-		strbuf_addf(sb, "%lu", item->size);
-	else if (is_atom("objectsize:disk", atom, len))
-		strbuf_addf(sb, "%"PRIuMAX, (uintmax_t)item->disk_size);
-	else if (is_atom("rest", atom, len)) {
-		if (item->rest)
-			strbuf_addstr(sb, item->rest);
-	} else if (is_atom("deltabase", atom, len))
-		strbuf_addstr(sb, oid_to_hex(item->delta_base_oid));
-}
-
-static size_t expand_format(struct strbuf *sb, const char *start, void *data)
-{
-	const char *end;
-	struct ref_array_item *item = data;
-
-	if (*start != '(')
-		return 0;
-	end = strchr(start + 1, ')');
-	if (!end)
-		die("format element '%s' does not end in ')'", start);
-
-	expand_atom(sb, start + 1, end - start - 1, item);
-	return end - start + 1;
-}
-
 static void batch_write(struct batch_options *opt, const void *data, int len)
 {
 	if (opt->buffer_output) {
@@ -282,22 +243,19 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d
 static void batch_object_write(const char *obj_name, struct batch_options *opt,
 			       struct expand_data *data)
 {
-	struct strbuf buf = STRBUF_INIT;
 	struct ref_array_item item = {0};
 
 	item.objectname = data->oid;
 	item.rest = data->rest;
 	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');
-	batch_write(opt, buf.buf, buf.len);
-	strbuf_release(&buf);
+	if (show_ref_array_item(&item, &opt->format))
+		return;
+	if (!opt->buffer_output)
+		fflush(stdout);
 
 	if (opt->print_contents) {
+		data->type = item.type;
 		print_object_or_die(opt, data);
 		batch_write(opt, "\n", 1);
 	}
diff --git a/ref-filter.c b/ref-filter.c
index 906a5344949f7..bfbc7c83fdd47 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1522,7 +1522,21 @@ int populate_value(struct ref_array_item *ref)
 			name++;
 		}
 
-		if (starts_with(name, "refname"))
+		if (is_cat) {
+			if (starts_with(name, "objectname"))
+				v->s = oid_to_hex(&ref->objectname);
+			else if (starts_with(name, "objecttype"))
+				v->s = typename(ref->type);
+			else if (starts_with(name, "objectsize")) {
+				v->s = xstrfmt("%lu", ref->size);
+			} else if (starts_with(name, "objectsize:disk")) {
+				v->s = xstrfmt("%"PRIuMAX, (uintmax_t)ref->disk_size);
+			} else if (starts_with(name, "rest"))
+				v->s = ref->rest;
+			else if (starts_with(name, "deltabase"))
+				v->s = xstrdup(oid_to_hex(ref->delta_base_oid));
+			continue;
+		} else if (starts_with(name, "refname"))
 			refname = get_refname(atom, ref);
 		else if (starts_with(name, "symref"))
 			refname = get_symref(atom, ref);
@@ -2219,6 +2233,7 @@ int format_ref_array_item(struct ref_array_item *info,
 {
 	const char *cp, *sp, *ep;
 	struct ref_formatting_state state = REF_FORMATTING_STATE_INIT;
+	int retval = 0;
 
 	state.quote_style = format->quote_style;
 	push_stack_element(&state.stack);
@@ -2229,13 +2244,22 @@ int format_ref_array_item(struct ref_array_item *info,
 		ep = strchr(sp, ')');
 		if (cp < sp)
 			append_literal(cp, sp, &state);
-		get_ref_atom_value(info,
+		if (is_cat) {
+			if(get_ref_atom_value(info,
+				parse_ref_filter_atom(format, valid_cat_file_atom,
+					ARRAY_SIZE(valid_cat_file_atom), sp + 2, ep),
+					&atomv))
+				return -1;
+		} else
+			get_ref_atom_value(info,
 				   parse_ref_filter_atom(format, valid_atom,
 							 ARRAY_SIZE(valid_atom),
 							 sp + 2, ep),
 				   &atomv);
 		atomv->handler(atomv, &state);
 	}
+	if (is_cat && strlen(format->format) == 0)
+		retval = check_and_fill_for_cat(info);
 	if (*cp) {
 		sp = cp + strlen(cp);
 		append_literal(cp, sp, &state);
@@ -2249,7 +2273,7 @@ int format_ref_array_item(struct ref_array_item *info,
 		die(_("format: %%(end) atom missing"));
 	strbuf_addbuf(final_buf, &state.stack->output);
 	pop_stack_element(&state.stack);
-	return 0;
+	return retval;
 }
 
 int show_ref_array_item(struct ref_array_item *info,

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

```

## Olga Telezhnaya, 2018-01-26 19:43

Subject: [PATCH RFC 16/24] ref-filter: make cat_file_info independent
Message-ID: <0102016133ff3b3d-001897ca-5d18-45e6-bd4c-118aa9a3d659-000000@eu-west-1.amazonses.com>
URL: https://gitlist.dev/e/0102016133ff3b3d-001897ca-5d18-45e6-bd4c-118aa9a3d659-000000%40eu-west-1.amazonses.com
In-Reply-To: <0102016133ff3a86-44d354ec-13c6-4c38-bc75-1ba4422db5a7-000000@eu-west-1.amazonses.com>

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

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

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 3a52c551f366a..bd803856537b8 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -290,6 +290,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.all_objects = opt->all_objects;
 	verify_ref_format(&opt->format);
diff --git a/ref-filter.c b/ref-filter.c
index 29a1b75c93181..906a5344949f7 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);
 }
@@ -752,7 +752,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)); ) {
@@ -779,8 +778,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;
 }
@@ -1470,18 +1469,20 @@ static void need_object(struct ref_array_item *ref) {
 
 static int check_and_fill_for_cat(struct ref_array_item *ref)
 {
-	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;
 	return 0;
 }
 
diff --git a/ref-filter.h b/ref-filter.h
index 4af89c4c86bee..d8064086f36a9 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -118,7 +118,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 all_objects;
 };

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

```

## Olga Telezhnaya, 2018-01-26 19:43

Subject: [PATCH RFC 14/24] ref_filter: add is_rest_atom_used function
Message-ID: <0102016133ff3b3d-58cb3635-e54a-4473-a12c-78351f613c2c-000000@eu-west-1.amazonses.com>
URL: https://gitlist.dev/e/0102016133ff3b3d-58cb3635-e54a-4473-a12c-78351f613c2c-000000%40eu-west-1.amazonses.com
In-Reply-To: <0102016133ff3a86-44d354ec-13c6-4c38-bc75-1ba4422db5a7-000000@eu-west-1.amazonses.com>

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

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

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

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

```

## Olga Telezhnaya, 2018-01-26 19:43

Subject: [PATCH RFC 12/24] ref-filter: get rid of expand_atom_into_fields
Message-ID: <0102016133ff3b27-2758b620-815d-4289-9714-9c6aea69e3ab-000000@eu-west-1.amazonses.com>
URL: https://gitlist.dev/e/0102016133ff3b27-2758b620-815d-4289-9714-9c6aea69e3ab-000000%40eu-west-1.amazonses.com
In-Reply-To: <0102016133ff3a86-44d354ec-13c6-4c38-bc75-1ba4422db5a7-000000@eu-west-1.amazonses.com>

```
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 12ca236815411..8d10d356609e7 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_atom[i].name, "rest"))
+		cat_file_info->split_on_whitespace = 1;
 	return at;
 }
 

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

```

## Olga Telezhnaya, 2018-01-26 19:43

Subject: [PATCH RFC 11/24] cat-file: start reusing populate_value
Message-ID: <0102016133ff3b1b-dcaaf82b-b21b-49b9-b21c-85f55873bd09-000000@eu-west-1.amazonses.com>
URL: https://gitlist.dev/e/0102016133ff3b1b-dcaaf82b-b21b-49b9-b21c-85f55873bd09-000000%40eu-west-1.amazonses.com
In-Reply-To: <0102016133ff3a86-44d354ec-13c6-4c38-bc75-1ba4422db5a7-000000@eu-west-1.amazonses.com>

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

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

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 61b7acc79155d..c2ca645662ae7 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -285,21 +285,11 @@ static void batch_object_write(const char *obj_name, struct batch_options *opt,
 	struct strbuf buf = STRBUF_INIT;
 	struct ref_array_item item = {0};
 
-	if (!data->skip_object_info &&
-	    sha1_object_info_extended(data->oid.hash, &data->info,
-				      OBJECT_INFO_LOOKUP_REPLACE) < 0) {
-		printf("%s missing\n",
-		       obj_name ? obj_name : oid_to_hex(&data->oid));
-		fflush(stdout);
-		return;
-	}
-
 	item.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 e69dd1ff5091f..12ca236815411 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1451,6 +1451,23 @@ static void need_object(struct ref_array_item *ref) {
 		free(buf);
 }
 
+static int check_and_fill_for_cat(struct ref_array_item *ref)
+{
+	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;
+	return 0;
+}
+
 /*
  * Parse the object referred by ref, and grab needed value.
  */
@@ -1467,6 +1484,9 @@ int populate_value(struct ref_array_item *ref)
 			ref->symref = "";
 	}
 
+	if (cat_file_info && check_and_fill_for_cat(ref))
+		return -1;
+
 	/* Fill in specials first */
 	for (i = 0; i < used_atom_cnt; i++) {
 		struct used_atom *atom = &used_atom[i];
diff --git a/ref-filter.h b/ref-filter.h
index 6373167aaacd7..e41d2913c0fff 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/452

```

## Olga Telezhnaya, 2018-01-26 19:43

Subject: [PATCH RFC 02/24] ref-filter: add return value to some functions
Message-ID: <0102016133ff3ad8-abf99952-4df0-42a4-9ae9-06f602e84f3b-000000@eu-west-1.amazonses.com>
URL: https://gitlist.dev/e/0102016133ff3ad8-abf99952-4df0-42a4-9ae9-06f602e84f3b-000000%40eu-west-1.amazonses.com
In-Reply-To: <0102016133ff3a86-44d354ec-13c6-4c38-bc75-1ba4422db5a7-000000@eu-west-1.amazonses.com>

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

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

diff --git a/ref-filter.c b/ref-filter.c
index 37337b57aacf4..7030d35c84a81 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1405,7 +1405,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)
+static int populate_value(struct ref_array_item *ref)
 {
 	int i;
 
@@ -1526,20 +1526,22 @@ static void populate_value(struct ref_array_item *ref)
 			break;
 		}
 	}
-	return;
+	return 0;
 }
 
 /*
  * Given a ref, return the value for the atom.  This lazily gets value
  * out of the object by calling populate value.
  */
-static void get_ref_atom_value(struct ref_array_item *ref, int atom, struct atom_value **v)
+static int get_ref_atom_value(struct ref_array_item *ref, int atom, struct atom_value **v)
 {
+	int retval = 0;
 	if (!ref->value) {
-		populate_value(ref);
+		retval = populate_value(ref);
 		fill_missing_values(ref->value);
 	}
 	*v = &ref->value[atom];
+	return retval;
 }
 
 /*
@@ -2124,7 +2126,7 @@ static void append_literal(const char *cp, const char *ep, struct ref_formatting
 	}
 }
 
-void format_ref_array_item(struct ref_array_item *info,
+int format_ref_array_item(struct ref_array_item *info,
 			   const struct ref_format *format,
 			   struct strbuf *final_buf)
 {
@@ -2158,17 +2160,20 @@ void format_ref_array_item(struct ref_array_item *info,
 		die(_("format: %%(end) atom missing"));
 	strbuf_addbuf(final_buf, &state.stack->output);
 	pop_stack_element(&state.stack);
+	return 0;
 }
 
-void show_ref_array_item(struct ref_array_item *info,
+int show_ref_array_item(struct ref_array_item *info,
 			 const struct ref_format *format)
 {
 	struct strbuf final_buf = STRBUF_INIT;
+	int retval = format_ref_array_item(info, format, &final_buf);
 
-	format_ref_array_item(info, format, &final_buf);
 	fwrite(final_buf.buf, 1, final_buf.len, stdout);
 	strbuf_release(&final_buf);
-	putchar('\n');
+	if (!retval)
+		putchar('\n');
+	return retval;
 }
 
 void pretty_print_ref(const char *name, const unsigned char *sha1,
diff --git a/ref-filter.h b/ref-filter.h
index 0d98342b34319..ff416b733b4b1 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -110,11 +110,11 @@ int verify_ref_format(struct ref_format *format);
 /*  Sort the given ref_array as per the ref_sorting provided */
 void ref_array_sort(struct ref_sorting *sort, struct ref_array *array);
 /*  Based on the given format and quote_style, fill the strbuf */
-void format_ref_array_item(struct ref_array_item *info,
+int format_ref_array_item(struct ref_array_item *info,
 			   const struct ref_format *format,
 			   struct strbuf *final_buf);
 /*  Print the ref using the given format and quote_style */
-void show_ref_array_item(struct ref_array_item *info, const struct ref_format *format);
+int show_ref_array_item(struct ref_array_item *info, const struct ref_format *format);
 /*  Parse a single sort specifier and add it to the list */
 void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *atom);
 /*  Callback function for parsing the sort option */

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

```

## Olga Telezhnaya, 2018-01-26 19:43

Subject: [PATCH RFC 08/24] ref-filter: reuse parse_ref_filter_atom
Message-ID: <0102016133ff3b0d-d1f39e25-3569-4703-92ef-49355ea03aa2-000000@eu-west-1.amazonses.com>
URL: https://gitlist.dev/e/0102016133ff3b0d-d1f39e25-3569-4703-92ef-49355ea03aa2-000000%40eu-west-1.amazonses.com
In-Reply-To: <0102016133ff3a86-44d354ec-13c6-4c38-bc75-1ba4422db5a7-000000@eu-west-1.amazonses.com>

```
Continue migrating formatting logic from cat-file to ref-filter.
Reuse parse_ref_filter_atom for unifying all processes in ref-filter
and further 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 8e384b2818b22..95c85009f1f58 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/452

```

## Olga Telezhnaya, 2018-01-26 19:43

Subject: [PATCH RFC 06/24] cat-file: move struct expand_data into ref-filter
Message-ID: <0102016133ff3b0c-042864ef-1f19-4991-a240-7358c56daec9-000000@eu-west-1.amazonses.com>
URL: https://gitlist.dev/e/0102016133ff3b0c-042864ef-1f19-4991-a240-7358c56daec9-000000%40eu-west-1.amazonses.com
In-Reply-To: <0102016133ff3a86-44d354ec-13c6-4c38-bc75-1ba4422db5a7-000000@eu-west-1.amazonses.com>

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

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

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 65c300184cab8..490d9f8a8a922 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 ff416b733b4b1..56093a85d52b8 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -72,6 +72,42 @@ struct ref_filter {
 		verbose;
 };
 
+struct expand_data {
+	struct object_id oid;
+	enum object_type type;
+	unsigned long size;
+	off_t disk_size;
+	const char *rest;
+	struct object_id delta_base_oid;
+
+	/*
+	 * If mark_query is true, we do not expand anything, but rather
+	 * just mark the object_info with items we wish to query.
+	 */
+	int mark_query;
+
+	/*
+	 * Whether to split the input on whitespace before feeding it to
+	 * get_sha1; this is decided during the mark_query phase based on
+	 * whether we have a %(rest) token in our format.
+	 */
+	int split_on_whitespace;
+
+	/*
+	 * After a mark_query run, this object_info is set up to be
+	 * passed to sha1_object_info_extended. It will point to the data
+	 * elements above, so you can retrieve the response from there.
+	 */
+	struct object_info info;
+
+	/*
+	 * This flag will be true if the requested batch format and options
+	 * don't require us to call sha1_object_info, which can then be
+	 * optimized out.
+	 */
+	unsigned skip_object_info : 1;
+};
+
 struct ref_format {
 	/*
 	 * Set these to define the format; make sure you call

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

```

## Olga Telezhnaya, 2018-01-26 19:43

Subject: [PATCH RFC 21/24] ref-filter: work with objectsize:disk
Message-ID: <0102016133ff3b53-0d7be81e-3350-4d32-bbb1-b108a1b27c41-000000@eu-west-1.amazonses.com>
URL: https://gitlist.dev/e/0102016133ff3b53-0d7be81e-3350-4d32-bbb1-b108a1b27c41-000000%40eu-west-1.amazonses.com
In-Reply-To: <0102016133ff3a86-44d354ec-13c6-4c38-bc75-1ba4422db5a7-000000@eu-west-1.amazonses.com>

```
Make a temporary solution for commands that could use
objectsize:disk atom.
It's better to fill it with value or give an error if there is no value
for this atom, but as a first solution we do dothing.
It means that if objectsize:disk is used, we put an empty string there.

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

diff --git a/ref-filter.c b/ref-filter.c
index 2cac394e93f52..bf49ed21eaac5 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -826,8 +826,10 @@ static void grab_common_values(struct atom_value *val, int deref, struct object
 			v->value = sz;
 			v->s = xstrfmt("%lu", sz);
 		} else if (!strcmp(name, "objectsize:disk")) {
-			v->value = cat_file_info.disk_size;
-			v->s = xstrfmt("%"PRIuMAX, (uintmax_t)v->value);
+			if (is_cat) {
+				v->value = cat_file_info.disk_size;
+				v->s = xstrfmt("%"PRIuMAX, (uintmax_t)v->value);
+			}
 		} else if (deref)
 			grab_objectname(name, obj->oid.hash, v, &used_atom[i]);
 	}

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

```

## Olga Telezhnaya, 2018-01-26 19:43

Subject: [PATCH RFC 05/24] ref-filter: make valid_atom as function parameter
Message-ID: <0102016133ff3afa-721294ba-c691-4b45-b3a8-622525285902-000000@eu-west-1.amazonses.com>
URL: https://gitlist.dev/e/0102016133ff3afa-721294ba-c691-4b45-b3a8-622525285902-000000%40eu-west-1.amazonses.com
In-Reply-To: <0102016133ff3a86-44d354ec-13c6-4c38-bc75-1ba4422db5a7-000000@eu-west-1.amazonses.com>

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

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

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

diff --git a/ref-filter.c b/ref-filter.c
index 7030d35c84a81..ee2aa7d6d072b 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))
@@ -2143,7 +2145,9 @@ int 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);
 	}
@@ -2194,7 +2198,8 @@ static int parse_sorting_atom(const char *atom)
 	 */
 	struct ref_format dummy = REF_FORMAT_INIT;
 	const char *end = atom + strlen(atom);
-	return parse_ref_filter_atom(&dummy, atom, end);
+	return parse_ref_filter_atom(&dummy, valid_atom,
+				     ARRAY_SIZE(valid_atom), atom, end);
 }
 
 /*  If no sorting option is given, use refname to sort as default */

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

```

## Olga Telezhnaya, 2018-01-26 19:43

Subject: [PATCH RFC 07/24] cat-file: start migrating to ref-filter
Message-ID: <0102016133ff3b0a-d7e5bf0c-4d99-41e4-882b-47a875c528fb-000000@eu-west-1.amazonses.com>
URL: https://gitlist.dev/e/0102016133ff3b0a-d7e5bf0c-4d99-41e4-882b-47a875c528fb-000000%40eu-west-1.amazonses.com
In-Reply-To: <0102016133ff3a86-44d354ec-13c6-4c38-bc75-1ba4422db5a7-000000@eu-west-1.amazonses.com>

```
Start moving formatting stuff related to data preparation
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 | 32 +++-----------------------------
 ref-filter.c       | 41 ++++++++++++++++++++++++++++++++++++-----
 ref-filter.h       | 12 ++++++------
 3 files changed, 45 insertions(+), 40 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 490d9f8a8a922..909412747cbd2 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)
 {
@@ -217,8 +198,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)
@@ -232,11 +211,7 @@ static size_t expand_format(struct strbuf *sb, const char *start, void *vdata)
 	if (!end)
 		die("format element '%s' does not end in ')'", start);
 
-	if (data->mark_query)
-		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;
 }
 
@@ -420,9 +395,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 ee2aa7d6d072b..8e384b2818b22 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 56093a85d52b8..b50c8e6aaf2c4 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -80,12 +80,6 @@ struct expand_data {
 	const char *rest;
 	struct object_id delta_base_oid;
 
-	/*
-	 * If mark_query is true, we do not expand anything, but rather
-	 * just mark the object_info with items we wish to query.
-	 */
-	int mark_query;
-
 	/*
 	 * Whether to split the input on whitespace before feeding it to
 	 * get_sha1; this is decided during the mark_query phase based on
@@ -119,6 +113,12 @@ struct ref_format {
 
 	/* Internal state to ref-filter */
 	int need_color_reset_at_eol;
+
+	/*
+	 * Helps to move all formatting logic from cat-file to ref-filter,
+	 * hopefully would be reduced later.
+	 */
+	struct expand_data *cat_file_data;
 };
 
 #define REF_FORMAT_INIT { NULL, 0, -1 }

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

```

## Olga Telezhnaya, 2018-01-26 19:43

Subject: [PATCH RFC 15/24] cat-file: move skip_object_info into ref-filter
Message-ID: <0102016133ff3b41-c77d5053-8546-4b12-831e-135f3025a2ea-000000@eu-west-1.amazonses.com>
URL: https://gitlist.dev/e/0102016133ff3b41-c77d5053-8546-4b12-831e-135f3025a2ea-000000%40eu-west-1.amazonses.com
In-Reply-To: <0102016133ff3a86-44d354ec-13c6-4c38-bc75-1ba4422db5a7-000000@eu-west-1.amazonses.com>

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

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

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 601a87d9b5f7c..3a52c551f366a 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -394,14 +394,9 @@ 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.all_objects = opt->all_objects;
 	verify_ref_format(&opt->format);
 
-	if (opt->all_objects) {
-		struct object_info empty = OBJECT_INFO_INIT;
-		if (!memcmp(&data.info, &empty, sizeof(empty)))
-			data.skip_object_info = 1;
-	}
-
 	/*
 	 * If we are printing out the object, then always fill in the type,
 	 * since we will want to decide whether or not to stream.
diff --git a/ref-filter.c b/ref-filter.c
index 51da76dc21136..29a1b75c93181 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -777,6 +777,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 276de387f3bd0..4af89c4c86bee 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -120,6 +120,7 @@ struct ref_format {
 	 */
 	struct expand_data *cat_file_data;
 	int is_cat;
+	int all_objects;
 };
 
 #define REF_FORMAT_INIT { NULL, 0, -1 }

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

```

## Junio C Hamano, 2018-01-26 20:19

Subject: Re: [PATCH RFC 01/24] ref-filter: get rid of goto
Message-ID: <xmqqfu6s74pf.fsf@gitster.mtv.corp.google.com>
URL: https://gitlist.dev/e/xmqqfu6s74pf.fsf%40gitster.mtv.corp.google.com
In-Reply-To: <0102016133ff3a86-44d354ec-13c6-4c38-bc75-1ba4422db5a7-000000@eu-west-1.amazonses.com>

```
Olga Telezhnaya <olyatelezhnaya@gmail.com> writes:

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

How was this patch "mentored by" these two folks?  Have they already
reviewed and gave you OK, or are you asking them to also help reviewing
with this message?  Mostly just being curious.

It is not convincning that this splitting the last part of a single
function into a separate helper function that is called from only
one place improves readability.  If better readability is the
purpose, I would even say

         for (i = 0; i < used_atom_cnt; i++) {
		if (...)
-			goto need_obj;
+			break;
	}
-	return;
+	if (used_atom_cnt <= i)
		return;

-need_obj:

would make the result easier to follow with a much less impact.

If we later in the series will use this new helper function from
other places, it certainly makes sense to create a new helper like
this patch does, but then "get rid of goto for readability" is not
the justification for such a change.

>  ref-filter.c | 103 ++++++++++++++++++++++++++++++-----------------------------
>  1 file changed, 53 insertions(+), 50 deletions(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index f9e25aea7a97e..37337b57aacf4 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1354,16 +1354,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) {

Style.  The opening brace at the beginning of the function sits on
its own line alone.

```

## Junio C Hamano, 2018-01-26 21:05

Subject: Re: [PATCH RFC 02/24] ref-filter: add return value to some functions
Message-ID: <xmqq1sic72ku.fsf@gitster.mtv.corp.google.com>
URL: https://gitlist.dev/e/xmqq1sic72ku.fsf%40gitster.mtv.corp.google.com
In-Reply-To: <0102016133ff3ad8-abf99952-4df0-42a4-9ae9-06f602e84f3b-000000@eu-west-1.amazonses.com>

```
Olga Telezhnaya <olyatelezhnaya@gmail.com> writes:

> Add return flag to format_ref_array_item, show_ref_array_item,
> get_ref_array_info and populate_value for further using.
> Need it to handle situations when item is broken but we can not invoke
> die() because we are in batch mode and all items need to be processed.
>
> Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored by: Jeff King <peff@peff.net>
> ---
>  ref-filter.c | 21 +++++++++++++--------
>  ref-filter.h |  4 ++--
>  2 files changed, 15 insertions(+), 10 deletions(-)

Makes sense as a preparatory step to pass the return status
throughout the call chain.  The functions that actually will detect
issues should probably gain

	/*
	 * a comment before the function
	 * that documents what it does and what values it returns
	 * to signal the failure.
	 */

before them; those that only pass the errorcode through to the
caller do not necessarily have to, though.

> -void show_ref_array_item(struct ref_array_item *info,
> +int show_ref_array_item(struct ref_array_item *info,
>  			 const struct ref_format *format)
>  {
>  	struct strbuf final_buf = STRBUF_INIT;
> +	int retval = format_ref_array_item(info, format, &final_buf);
>  
> -	format_ref_array_item(info, format, &final_buf);
>  	fwrite(final_buf.buf, 1, final_buf.len, stdout);
>  	strbuf_release(&final_buf);
> -	putchar('\n');
> +	if (!retval)
> +		putchar('\n');
> +	return retval;

This is questionable.  Why does it write final_buf out regardless of
the return value, even though it refrains from writing terminating LF
upon non-zero return from the same function that prepared final_buf?

"Because final_buf is left unmodified when formatter returns an
error, and calling fwrite on an empty buffer ends up being a no-op"
is a wrong answer---it relies on having too intimate knowledge on
how the callee happens to work currently.

```

## Junio C Hamano, 2018-01-26 21:46

Subject: Re: [PATCH RFC 03/24] cat-file: split expand_atom into 2 functions
Message-ID: <xmqqvafo5m5h.fsf@gitster.mtv.corp.google.com>
URL: https://gitlist.dev/e/xmqqvafo5m5h.fsf%40gitster.mtv.corp.google.com
In-Reply-To: <0102016133ff3afb-6bab2c11-e0f3-4bef-9626-fbcc826bcb15-000000@eu-west-1.amazonses.com>

```
Olga Telezhnaya <olyatelezhnaya@gmail.com> writes:

> 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(-)

As expand_atom() is file-scope static and its callers are well
isolated, it is OK to change its meaning while restructuring the
code like this patch does (as opposed to a public function to which
new callers may be added on other topics in flight).

The split itself looks sensible, but expand_atom_into_fields() is a
questionable name.  expand_atom() does fill the data in sb, but
calling expand_atom_into_fields() does not fill any data into
separated fields---it merely prepares somebody else to do so.

Helped by this comment:

	/*
	 * 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;

we can guess that a better name would mention or hint "object_info",
"query" and probably "prepare" (because we would do so before
actually querying).

I am not sure if separating the logic into these two functions is a
good way to organize things.  When a new %(atom) is introduced, it
is more likely that a programmer adds it to one but forgets to make
a matching change to the other, no?  (here, "I am not sure" is just
that.  It is very different from "I am sure this is wrong").

Thanks.

```

## Junio C Hamano, 2018-01-26 22:18

Subject: Re: [PATCH RFC 04/24] cat-file: reuse struct ref_format
Message-ID: <xmqq4ln8qn5g.fsf@gitster-ct.c.googlers.com>
URL: https://gitlist.dev/e/xmqq4ln8qn5g.fsf%40gitster-ct.c.googlers.com
In-Reply-To: <0102016133ff3af0-0ebed14d-deb5-4744-9267-4c5b0c6a30c8-000000@eu-west-1.amazonses.com>

```
Olga Telezhnaya <olyatelezhnaya@gmail.com> writes:

> Start using ref_format struct instead of simple char*.
> Need that for further reusing of formatting logic from ref-filter.
>
> Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored by: Jeff King <peff@peff.net>
> ---
>  builtin/cat-file.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)

OK, so at this stage we only use its .format field (which happens to
be the same simple "char *"), but we can later extend the functions
on the callchain this thing is passed through.

Makes sense.

Thanks.


>
> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index f783b39b9bd5c..65c300184cab8 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,7 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
>  {
>  	int opt = 0;
>  	const char *exp_type = NULL, *obj_name = NULL;
> -	struct batch_options batch = {0};
> +	struct batch_options batch = { REF_FORMAT_INIT };
>  	int unknown_type = 0;
>  
>  	const struct option options[] = {
>
> --
> https://github.com/git/git/pull/452

```

## Оля Тележная, 2018-01-29 07:13

Subject: Re: [PATCH RFC 01/24] ref-filter: get rid of goto
Message-ID: <CAL21BmkOGE6UZQJTFQjq3qnmywwBJQOUXE7jSwP1_Vn6PZ8ezA@mail.gmail.com>
URL: https://gitlist.dev/e/CAL21BmkOGE6UZQJTFQjq3qnmywwBJQOUXE7jSwP1_Vn6PZ8ezA%40mail.gmail.com
In-Reply-To: <xmqqfu6s74pf.fsf@gitster.mtv.corp.google.com>

```
2018-01-26 23:19 GMT+03:00 Junio C Hamano <gitster@pobox.com>:
> Olga Telezhnaya <olyatelezhnaya@gmail.com> writes:
>
>> 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>
>> ---
>
> How was this patch "mentored by" these two folks?  Have they already
> reviewed and gave you OK, or are you asking them to also help reviewing
> with this message?  Mostly just being curious.

Christian and Jeff help me when I have different sort of difficulties.
Not sure that they were helping me with that commit separately.
Both of them reviewed my code and said that it's ready for a final
review (actually, Christian said, but it's usual situation when I ask
for help/review and one of them helps me. The other one could add
something, but, as I understand, if he totally agree, he will keep
silence, and I find that behavior logical).
Do I need to delete these lines from some of commits where I do not
remember help from them?

>
> It is not convincning that this splitting the last part of a single
> function into a separate helper function that is called from only
> one place improves readability.  If better readability is the
> purpose, I would even say
>
>          for (i = 0; i < used_atom_cnt; i++) {
>                 if (...)
> -                       goto need_obj;
> +                       break;
>         }
> -       return;
> +       if (used_atom_cnt <= i)
>                 return;
>
> -need_obj:
>
> would make the result easier to follow with a much less impact.

It's hard for me to read the code with goto, and as I know, it's not
only my problem, it's usual situation to try to get rid of gotos. I
always need to re-check whether we use that piece of code somewhere
else or not, and how we do that. I also think that it's good that most
of variables in the beginning of the function populate_value go to new
function.

>
> If we later in the series will use this new helper function from
> other places, it certainly makes sense to create a new helper like
> this patch does, but then "get rid of goto for readability" is not
> the justification for such a change.

We don't use that new function anywhere else further. So, I can delete
this commit or I can change commit message (if so, please give me some
ideas what I need to mention there).

>
>>  ref-filter.c | 103 ++++++++++++++++++++++++++++++-----------------------------
>>  1 file changed, 53 insertions(+), 50 deletions(-)
>>
>> diff --git a/ref-filter.c b/ref-filter.c
>> index f9e25aea7a97e..37337b57aacf4 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -1354,16 +1354,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) {
>
> Style.  The opening brace at the beginning of the function sits on
> its own line alone.

Thanks, I will fix that when we decide how to finally improve that commit.

Olga

```

## Оля Тележная, 2018-01-29 07:26

Subject: Re: [PATCH RFC 03/24] cat-file: split expand_atom into 2 functions
Message-ID: <CAL21BmmAq+FRx2F8eiQj+jHhR-Y=Vdr26hC5T1F_9Ga8TyWJPQ@mail.gmail.com>
URL: https://gitlist.dev/e/CAL21BmmAq%2BFRx2F8eiQj%2BjHhR-Y%3DVdr26hC5T1F_9Ga8TyWJPQ%40mail.gmail.com
In-Reply-To: <xmqqvafo5m5h.fsf@gitster.mtv.corp.google.com>

```
2018-01-27 0:46 GMT+03:00 Junio C Hamano <gitster@pobox.com>:
> Olga Telezhnaya <olyatelezhnaya@gmail.com> writes:
>
>> 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(-)
>
> As expand_atom() is file-scope static and its callers are well
> isolated, it is OK to change its meaning while restructuring the
> code like this patch does (as opposed to a public function to which
> new callers may be added on other topics in flight).
>
> The split itself looks sensible, but expand_atom_into_fields() is a
> questionable name.  expand_atom() does fill the data in sb, but
> calling expand_atom_into_fields() does not fill any data into
> separated fields---it merely prepares somebody else to do so.
>
> Helped by this comment:
>
>         /*
>          * 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;
>
> we can guess that a better name would mention or hint "object_info",
> "query" and probably "prepare" (because we would do so before
> actually querying).

OK, I will rename that function. Actually, not sure that we really
need to have ideal name here because both functions will be deleted by
the end of this patch.
"mark_atom_in_object_info"?

>
> I am not sure if separating the logic into these two functions is a
> good way to organize things.  When a new %(atom) is introduced, it
> is more likely that a programmer adds it to one but forgets to make
> a matching change to the other, no?  (here, "I am not sure" is just
> that.  It is very different from "I am sure this is wrong").

In the end of the patch we don't have both of those functions, so
there would not be such problem.
But, I need that split, it helps me to go further and apply new
changes step-by-step.

>
> Thanks.

```

## Оля Тележная, 2018-01-29 07:36

Subject: Re: [PATCH RFC 02/24] ref-filter: add return value to some functions
Message-ID: <CAL21BmmWvrUjH3WESeMq45=ByhQ1cZhmZetUM6C2H7mGuFp84Q@mail.gmail.com>
URL: https://gitlist.dev/e/CAL21BmmWvrUjH3WESeMq45%3DByhQ1cZhmZetUM6C2H7mGuFp84Q%40mail.gmail.com
In-Reply-To: <xmqq1sic72ku.fsf@gitster.mtv.corp.google.com>

```
2018-01-27 0:05 GMT+03:00 Junio C Hamano <gitster@pobox.com>:
> Olga Telezhnaya <olyatelezhnaya@gmail.com> writes:
>
>> Add return flag to format_ref_array_item, show_ref_array_item,
>> get_ref_array_info and populate_value for further using.
>> Need it to handle situations when item is broken but we can not invoke
>> die() because we are in batch mode and all items need to be processed.
>>
>> Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
>> Mentored-by: Christian Couder <christian.couder@gmail.com>
>> Mentored by: Jeff King <peff@peff.net>
>> ---
>>  ref-filter.c | 21 +++++++++++++--------
>>  ref-filter.h |  4 ++--
>>  2 files changed, 15 insertions(+), 10 deletions(-)
>
> Makes sense as a preparatory step to pass the return status
> throughout the call chain.  The functions that actually will detect
> issues should probably gain
>
>         /*
>          * a comment before the function
>          * that documents what it does and what values it returns
>          * to signal the failure.
>          */
>
> before them; those that only pass the errorcode through to the
> caller do not necessarily have to, though.

I will add comments, agree, thank you.

>
>> -void show_ref_array_item(struct ref_array_item *info,
>> +int show_ref_array_item(struct ref_array_item *info,
>>                        const struct ref_format *format)
>>  {
>>       struct strbuf final_buf = STRBUF_INIT;
>> +     int retval = format_ref_array_item(info, format, &final_buf);
>>
>> -     format_ref_array_item(info, format, &final_buf);
>>       fwrite(final_buf.buf, 1, final_buf.len, stdout);
>>       strbuf_release(&final_buf);
>> -     putchar('\n');
>> +     if (!retval)
>> +             putchar('\n');
>> +     return retval;
>
> This is questionable.  Why does it write final_buf out regardless of
> the return value, even though it refrains from writing terminating LF
> upon non-zero return from the same function that prepared final_buf?
>
> "Because final_buf is left unmodified when formatter returns an
> error, and calling fwrite on an empty buffer ends up being a no-op"
> is a wrong answer---it relies on having too intimate knowledge on
> how the callee happens to work currently.

I will change that piece of code by putting fwrite into if statement
also, thank you. We really do not put anything to buffer if retval is
not equal to zero (checked that).

Thank you for all your comments, waiting for further review.
Olga

```

## Junio C Hamano, 2018-01-30 20:49

Subject: Re: [PATCH RFC 01/24] ref-filter: get rid of goto
Message-ID: <xmqqd11rt6ks.fsf@gitster-ct.c.googlers.com>
URL: https://gitlist.dev/e/xmqqd11rt6ks.fsf%40gitster-ct.c.googlers.com
In-Reply-To: <CAL21BmkOGE6UZQJTFQjq3qnmywwBJQOUXE7jSwP1_Vn6PZ8ezA@mail.gmail.com>

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

>> one place improves readability.  If better readability is the
>> purpose, I would even say
>>
>>          for (i = 0; i < used_atom_cnt; i++) {
>>                 if (...)
>> -                       goto need_obj;
>> +                       break;
>>         }
>> -       return;
>> +       if (used_atom_cnt <= i)
>>                 return;
>>
>> -need_obj:
>>
>> would make the result easier to follow with a much less impact.
>
> It's hard for me to read the code with goto, and as I know, it's not
> only my problem,...

That sounds as if you are complaining "I wanted to get rid of goto
and you tell me not to do so???", but read what I showed above again
and notice that it is also getting rid of "goto".

The main difference from your version is that the original function
is still kept as a single unit of work, instead of two.

```

## Оля Тележная, 2018-01-31 06:39

Subject: Re: [PATCH RFC 01/24] ref-filter: get rid of goto
Message-ID: <CAL21Bm=cMQ-dx1v9rRhEuZQW7BrHGsCMTmvy=ONrv0weLM+JJg@mail.gmail.com>
URL: https://gitlist.dev/e/CAL21Bm%3DcMQ-dx1v9rRhEuZQW7BrHGsCMTmvy%3DONrv0weLM%2BJJg%40mail.gmail.com
In-Reply-To: <xmqqd11rt6ks.fsf@gitster-ct.c.googlers.com>

```
2018-01-30 23:49 GMT+03:00 Junio C Hamano <gitster@pobox.com>:
> Оля Тележная  <olyatelezhnaya@gmail.com> writes:
>
>>> one place improves readability.  If better readability is the
>>> purpose, I would even say
>>>
>>>          for (i = 0; i < used_atom_cnt; i++) {
>>>                 if (...)
>>> -                       goto need_obj;
>>> +                       break;
>>>         }
>>> -       return;
>>> +       if (used_atom_cnt <= i)
>>>                 return;
>>>
>>> -need_obj:
>>>
>>> would make the result easier to follow with a much less impact.
>>
>> It's hard for me to read the code with goto, and as I know, it's not
>> only my problem,...
>
> That sounds as if you are complaining "I wanted to get rid of goto
> and you tell me not to do so???", but read what I showed above again
> and notice that it is also getting rid of "goto".

No, I am not complaining. I tried to explain why I did everything that
way. Sorry if it was not clear enough.

>
> The main difference from your version is that the original function
> is still kept as a single unit of work, instead of two.

And I am not sure that it is good, the function is too big and it
actually does so many different separate pieces. If it is possible to
shorten long function by getting some separate logic (that's our case,
we do not request object until that final goto statement), I think
it's good idea and we need to do so and simplify future reading. But,
if you do not agree with this fact, please explain your position in
detail, and I will change that place as you want.

Thanks.

```