[PATCH 1/4] ref-filter: add info_source to valid_atom

[PATCH 1/4] ref-filter: add info_source to valid_atom

[Olga Telezhnaya]

Add the source of object data to prevent parsing of unneeded data.
The goal is to improve performance by avoiding calling expensive
functions when we don't need the information they provide
or when we could get it by using a cheaper function.

It is stored in valid_atoms because it depends on the atoms we are
interested in.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
 ref-filter.c | 82 +++++++++++++++++++++++++++++++-----------------------------
 1 file changed, 43 insertions(+), 39 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index fa3685d91f046..8611c24fd57d1 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -41,6 +41,7 @@ void setup_ref_filter_porcelain_msg(void)
 
 typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
 typedef enum { COMPARE_EQUAL, COMPARE_UNEQUAL, COMPARE_NONE } cmp_status;
+typedef enum { SOURCE_NONE = 0, SOURCE_OBJ, SOURCE_OTHER } info_source;
 
 struct align {
 	align_type position;
@@ -73,6 +74,7 @@ struct refname_atom {
 static struct used_atom {
 	const char *name;
 	cmp_type type;
+	info_source source;
 	union {
 		char color[COLOR_MAXLEN];
 		struct align align;
@@ -380,49 +382,50 @@ static int head_atom_parser(const struct ref_format *format, struct used_atom *a
 
 static struct {
 	const char *name;
+	info_source source;
 	cmp_type cmp_type;
 	int (*parser)(const struct ref_format *format, struct used_atom *atom,
 		      const char *arg, struct strbuf *err);
 } valid_atom[] = {
-	{ "refname" , FIELD_STR, refname_atom_parser },
-	{ "objecttype" },
-	{ "objectsize", FIELD_ULONG },
-	{ "objectname", FIELD_STR, objectname_atom_parser },
-	{ "tree" },
-	{ "parent" },
-	{ "numparent", FIELD_ULONG },
-	{ "object" },
-	{ "type" },
-	{ "tag" },
-	{ "author" },
-	{ "authorname" },
-	{ "authoremail" },
-	{ "authordate", FIELD_TIME },
-	{ "committer" },
-	{ "committername" },
-	{ "committeremail" },
-	{ "committerdate", FIELD_TIME },
-	{ "tagger" },
-	{ "taggername" },
-	{ "taggeremail" },
-	{ "taggerdate", FIELD_TIME },
-	{ "creator" },
-	{ "creatordate", FIELD_TIME },
-	{ "subject", FIELD_STR, subject_atom_parser },
-	{ "body", FIELD_STR, body_atom_parser },
-	{ "trailers", FIELD_STR, trailers_atom_parser },
-	{ "contents", FIELD_STR, contents_atom_parser },
-	{ "upstream", FIELD_STR, remote_ref_atom_parser },
-	{ "push", FIELD_STR, remote_ref_atom_parser },
-	{ "symref", FIELD_STR, refname_atom_parser },
-	{ "flag" },
-	{ "HEAD", FIELD_STR, head_atom_parser },
-	{ "color", FIELD_STR, color_atom_parser },
-	{ "align", FIELD_STR, align_atom_parser },
-	{ "end" },
-	{ "if", FIELD_STR, if_atom_parser },
-	{ "then" },
-	{ "else" },
+	{ "refname", SOURCE_NONE, FIELD_STR, refname_atom_parser },
+	{ "objecttype", SOURCE_OTHER },
+	{ "objectsize", SOURCE_OTHER, FIELD_ULONG },
+	{ "objectname", SOURCE_OTHER, FIELD_STR, objectname_atom_parser },
+	{ "tree", SOURCE_OBJ },
+	{ "parent", SOURCE_OBJ },
+	{ "numparent", SOURCE_OBJ, FIELD_ULONG },
+	{ "object", SOURCE_OBJ },
+	{ "type", SOURCE_OBJ },
+	{ "tag", SOURCE_OBJ },
+	{ "author", SOURCE_OBJ },
+	{ "authorname", SOURCE_OBJ },
+	{ "authoremail", SOURCE_OBJ },
+	{ "authordate", SOURCE_OBJ, FIELD_TIME },
+	{ "committer", SOURCE_OBJ },
+	{ "committername", SOURCE_OBJ },
+	{ "committeremail", SOURCE_OBJ },
+	{ "committerdate", SOURCE_OBJ, FIELD_TIME },
+	{ "tagger", SOURCE_OBJ },
+	{ "taggername", SOURCE_OBJ },
+	{ "taggeremail", SOURCE_OBJ },
+	{ "taggerdate", SOURCE_OBJ, FIELD_TIME },
+	{ "creator", SOURCE_OBJ },
+	{ "creatordate", SOURCE_OBJ, FIELD_TIME },
+	{ "subject", SOURCE_OBJ, FIELD_STR, subject_atom_parser },
+	{ "body", SOURCE_OBJ, FIELD_STR, body_atom_parser },
+	{ "trailers", SOURCE_OBJ, FIELD_STR, trailers_atom_parser },
+	{ "contents", SOURCE_OBJ, FIELD_STR, contents_atom_parser },
+	{ "upstream", SOURCE_NONE, FIELD_STR, remote_ref_atom_parser },
+	{ "push", SOURCE_NONE, FIELD_STR, remote_ref_atom_parser },
+	{ "symref", SOURCE_NONE, FIELD_STR, refname_atom_parser },
+	{ "flag", SOURCE_NONE },
+	{ "HEAD", SOURCE_NONE, FIELD_STR, head_atom_parser },
+	{ "color", SOURCE_NONE, FIELD_STR, color_atom_parser },
+	{ "align", SOURCE_NONE, FIELD_STR, align_atom_parser },
+	{ "end", SOURCE_NONE },
+	{ "if", SOURCE_NONE, FIELD_STR, if_atom_parser },
+	{ "then", SOURCE_NONE },
+	{ "else", SOURCE_NONE },
 };
 
 #define REF_FORMATTING_STATE_INIT  { 0, NULL }
@@ -498,6 +501,7 @@ static int parse_ref_filter_atom(const struct ref_format *format,
 	REALLOC_ARRAY(used_atom, used_atom_cnt);
 	used_atom[at].name = xmemdupz(atom, ep - atom);
 	used_atom[at].type = valid_atom[i].cmp_type;
+	used_atom[at].source = valid_atom[i].source;
 	if (arg) {
 		arg = used_atom[at].name + (arg - atom) + 1;
 		if (!*arg) {

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

[PATCH 3/4] ref-filter: merge get_obj and get_object

[Olga Telezhnaya]

Inline get_obj(): it would be easier to edit the code
without this split.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
 ref-filter.c | 36 +++++++++++-------------------------
 1 file changed, 11 insertions(+), 25 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 27733ef013bed..f04169f0ea0e3 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -797,24 +797,6 @@ int verify_ref_format(struct ref_format *format)
 	return 0;
 }
 
-/*
- * Given an object name, read the object data and size, and return a
- * "struct object".  If the object data we are returning is also borrowed
- * by the "struct object" representation, set *eaten as well---it is a
- * signal from parse_object_buffer to us not to free the buffer.
- */
-static void *get_obj(const struct object_id *oid, struct object **obj, unsigned long *sz, int *eaten)
-{
-	enum object_type type;
-	void *buf = read_object_file(oid, &type, sz);
-
-	if (buf)
-		*obj = parse_object_buffer(oid, type, *sz, buf, eaten);
-	else
-		*obj = NULL;
-	return buf;
-}
-
 static int grab_objectname(const char *name, const struct object_id *oid,
 			   struct atom_value *v, struct used_atom *atom)
 {
@@ -1437,20 +1419,24 @@ static const char *get_refname(struct used_atom *atom, struct ref_array_item *re
 }
 
 static int get_object(struct ref_array_item *ref, const struct object_id *oid,
-		       int deref, struct object **obj, struct strbuf *err)
+		      int deref, struct object **obj, struct strbuf *err)
 {
 	int eaten;
 	int ret = 0;
 	unsigned long size;
-	void *buf = get_obj(oid, obj, &size, &eaten);
+	enum object_type type;
+	void *buf = read_object_file(oid, &type, &size);
 	if (!buf)
 		ret = strbuf_addf_ret(err, -1, _("missing object %s for %s"),
 				      oid_to_hex(oid), ref->refname);
-	else if (!*obj)
-		ret = strbuf_addf_ret(err, -1, _("parse_object_buffer failed on %s for %s"),
-				      oid_to_hex(oid), ref->refname);
-	else
-		grab_values(ref->value, deref, *obj, buf, size);
+	else {
+		*obj = parse_object_buffer(oid, type, size, buf, &eaten);
+		if (!*obj)
+			ret = strbuf_addf_ret(err, -1, _("parse_object_buffer failed on %s for %s"),
+					      oid_to_hex(oid), ref->refname);
+		else
+			grab_values(ref->value, deref, *obj, buf, size);
+	}
 	if (!eaten)
 		free(buf);
 	return ret;

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

[PATCH 2/4] ref-filter: add empty values to technical fields

[Olga Telezhnaya]

Atoms like "align" or "end" do not have string representation.
Earlier we had to go and parse whole object with a hope that we
could fill their string representations. It's easier to fill them
with an empty string before we start to work with whole object.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
 ref-filter.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/ref-filter.c b/ref-filter.c
index 8611c24fd57d1..27733ef013bed 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1497,6 +1497,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 			refname = get_symref(atom, ref);
 		else if (starts_with(name, "upstream")) {
 			const char *branch_name;
+			v->s = "";
 			/* only local branches may have an upstream */
 			if (!skip_prefix(ref->refname, "refs/heads/",
 					 &branch_name))
@@ -1509,6 +1510,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 			continue;
 		} else if (atom->u.remote_ref.push) {
 			const char *branch_name;
+			v->s = "";
 			if (!skip_prefix(ref->refname, "refs/heads/",
 					 &branch_name))
 				continue;
@@ -1549,22 +1551,26 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 			continue;
 		} else if (starts_with(name, "align")) {
 			v->handler = align_atom_handler;
+			v->s = "";
 			continue;
 		} else if (!strcmp(name, "end")) {
 			v->handler = end_atom_handler;
+			v->s = "";
 			continue;
 		} else if (starts_with(name, "if")) {
 			const char *s;
-
+			v->s = "";
 			if (skip_prefix(name, "if:", &s))
 				v->s = xstrdup(s);
 			v->handler = if_atom_handler;
 			continue;
 		} else if (!strcmp(name, "then")) {
 			v->handler = then_atom_handler;
+			v->s = "";
 			continue;
 		} else if (!strcmp(name, "else")) {
 			v->handler = else_atom_handler;
+			v->s = "";
 			continue;
 		} else
 			continue;

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

[PATCH 4/4] ref-filter: use oid_object_info() to get object

[Olga Telezhnaya]

Use oid_object_info_extended() to get object info instead of
read_object_file().
It will help to handle some requests faster (e.g., we do not need to
parse whole object if we need to know %(objectsize)).
It could also help us to add new atoms such as %(objectsize:disk)
and %(deltabase).

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
 ref-filter.c | 120 +++++++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 87 insertions(+), 33 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index f04169f0ea0e3..9bf7de4c561cb 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -61,6 +61,17 @@ struct refname_atom {
 	int lstrip, rstrip;
 };
 
+struct expand_data {
+	struct object_id oid;
+	enum object_type type;
+	unsigned long size;
+	off_t disk_size;
+	struct object_id delta_base_oid;
+	void *content;
+
+	struct object_info info;
+} oi, oi_deref;
+
 /*
  * An atom is a valid field atom listed below, possibly prefixed with
  * a "*" to denote deref_tag().
@@ -202,6 +213,30 @@ static int remote_ref_atom_parser(const struct ref_format *format, struct used_a
 	return 0;
 }
 
+static int objecttype_atom_parser(const struct ref_format *format, struct used_atom *atom,
+				  const char *arg, struct strbuf *err)
+{
+	if (arg)
+		return strbuf_addf_ret(err, -1, _("%%(objecttype) does not take arguments"));
+	if (*atom->name == '*')
+		oi_deref.info.typep = &oi_deref.type;
+	else
+		oi.info.typep = &oi.type;
+	return 0;
+}
+
+static int objectsize_atom_parser(const struct ref_format *format, struct used_atom *atom,
+				  const char *arg, struct strbuf *err)
+{
+	if (arg)
+		return strbuf_addf_ret(err, -1, _("%%(objectsize) does not take arguments"));
+	if (*atom->name == '*')
+		oi_deref.info.sizep = &oi_deref.size;
+	else
+		oi.info.sizep = &oi.size;
+	return 0;
+}
+
 static int body_atom_parser(const struct ref_format *format, struct used_atom *atom,
 			    const char *arg, struct strbuf *err)
 {
@@ -388,8 +423,8 @@ static struct {
 		      const char *arg, struct strbuf *err);
 } valid_atom[] = {
 	{ "refname", SOURCE_NONE, FIELD_STR, refname_atom_parser },
-	{ "objecttype", SOURCE_OTHER },
-	{ "objectsize", SOURCE_OTHER, FIELD_ULONG },
+	{ "objecttype", SOURCE_OTHER, FIELD_STR, objecttype_atom_parser },
+	{ "objectsize", SOURCE_OTHER, FIELD_ULONG, objectsize_atom_parser },
 	{ "objectname", SOURCE_OTHER, FIELD_STR, objectname_atom_parser },
 	{ "tree", SOURCE_OBJ },
 	{ "parent", SOURCE_OBJ },
@@ -502,6 +537,12 @@ static int parse_ref_filter_atom(const struct ref_format *format,
 	used_atom[at].name = xmemdupz(atom, ep - atom);
 	used_atom[at].type = valid_atom[i].cmp_type;
 	used_atom[at].source = valid_atom[i].source;
+	if (used_atom[at].source == SOURCE_OBJ) {
+		if (*atom == '*')
+			oi_deref.info.contentp = &oi_deref.content;
+		else
+			oi.info.contentp = &oi.content;
+	}
 	if (arg) {
 		arg = used_atom[at].name + (arg - atom) + 1;
 		if (!*arg) {
@@ -817,7 +858,7 @@ static int grab_objectname(const char *name, const struct object_id *oid,
 }
 
 /* See grab_values */
-static void grab_common_values(struct atom_value *val, int deref, struct object *obj, void *buf, unsigned long sz)
+static void grab_common_values(struct atom_value *val, int deref, struct expand_data *oi)
 {
 	int i;
 
@@ -829,13 +870,13 @@ static void grab_common_values(struct atom_value *val, int deref, struct object
 		if (deref)
 			name++;
 		if (!strcmp(name, "objecttype"))
-			v->s = type_name(obj->type);
+			v->s = type_name(oi->type);
 		else if (!strcmp(name, "objectsize")) {
-			v->value = sz;
-			v->s = xstrfmt("%lu", sz);
+			v->value = oi->size;
+			v->s = xstrfmt("%lu", oi->size);
 		}
 		else if (deref)
-			grab_objectname(name, &obj->oid, v, &used_atom[i]);
+			grab_objectname(name, &oi->oid, v, &used_atom[i]);
 	}
 }
 
@@ -1194,7 +1235,6 @@ static void fill_missing_values(struct atom_value *val)
  */
 static void grab_values(struct atom_value *val, int deref, struct object *obj, void *buf, unsigned long sz)
 {
-	grab_common_values(val, deref, obj, buf, sz);
 	switch (obj->type) {
 	case OBJ_TAG:
 		grab_tag_values(val, deref, obj, buf, sz);
@@ -1418,28 +1458,35 @@ static const char *get_refname(struct used_atom *atom, struct ref_array_item *re
 	return show_ref(&atom->u.refname, ref->refname);
 }
 
-static int get_object(struct ref_array_item *ref, const struct object_id *oid,
-		      int deref, struct object **obj, struct strbuf *err)
+static int get_object(struct ref_array_item *ref, int deref, struct object **obj,
+		      struct expand_data *oi, struct strbuf *err)
 {
 	int eaten;
-	int ret = 0;
-	unsigned long size;
-	enum object_type type;
-	void *buf = read_object_file(oid, &type, &size);
-	if (!buf)
-		ret = strbuf_addf_ret(err, -1, _("missing object %s for %s"),
-				      oid_to_hex(oid), ref->refname);
-	else {
-		*obj = parse_object_buffer(oid, type, size, buf, &eaten);
-		if (!*obj)
-			ret = strbuf_addf_ret(err, -1, _("parse_object_buffer failed on %s for %s"),
-					      oid_to_hex(oid), ref->refname);
-		else
-			grab_values(ref->value, deref, *obj, buf, size);
+	if (oi->info.contentp) {
+		/* We need to know that to use parse_object_buffer properly */
+		oi->info.sizep = &oi->size;
+		oi->info.typep = &oi->type;
 	}
+	if (oid_object_info_extended(the_repository, &oi->oid, &oi->info,
+				     OBJECT_INFO_LOOKUP_REPLACE))
+		return strbuf_addf_ret(err, -1, _("missing object %s for %s"),
+				       oid_to_hex(&oi->oid), ref->refname);
+
+	if (oi->info.contentp) {
+		*obj = parse_object_buffer(&oi->oid, oi->type, oi->size, oi->content, &eaten);
+		if (!obj) {
+			if (!eaten)
+				free(oi->content);
+			return strbuf_addf_ret(err, -1, _("parse_object_buffer failed on %s for %s"),
+					      oid_to_hex(&oi->oid), ref->refname);
+		}
+		grab_values(ref->value, deref, *obj, oi->content, oi->size);
+	}
+
+	grab_common_values(ref->value, deref, oi);
 	if (!eaten)
-		free(buf);
-	return ret;
+		free(oi->content);
+	return 0;
 }
 
 /*
@@ -1449,7 +1496,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 {
 	struct object *obj;
 	int i;
-	const struct object_id *tagged;
+	struct object_info empty = OBJECT_INFO_INIT;
 
 	ref->value = xcalloc(used_atom_cnt, sizeof(struct atom_value));
 
@@ -1569,13 +1616,20 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 
 	for (i = 0; i < used_atom_cnt; i++) {
 		struct atom_value *v = &ref->value[i];
-		if (v->s == NULL)
-			break;
+		if (v->s == NULL && used_atom[i].source == SOURCE_NONE)
+			return strbuf_addf_ret(err, -1, _("missing object %s for %s"),
+					       oid_to_hex(&ref->objectname), ref->refname);
 	}
-	if (used_atom_cnt <= i)
+
+	if (need_tagged)
+		oi.info.contentp = &oi.content;
+	if (!memcmp(&oi.info, &empty, sizeof(empty)) &&
+	    !memcmp(&oi_deref.info, &empty, sizeof(empty)))
 		return 0;
 
-	if (get_object(ref, &ref->objectname, 0, &obj, err))
+
+	oi.oid = ref->objectname;
+	if (get_object(ref, 0, &obj, &oi, err))
 		return -1;
 
 	/*
@@ -1589,7 +1643,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 	 * 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;
+	oi_deref.oid = ((struct tag *)obj)->tagged->oid;
 
 	/*
 	 * NEEDSWORK: This derefs tag only once, which
@@ -1597,7 +1651,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 	 * is not consistent with what deref_tag() does
 	 * which peels the onion to the core.
 	 */
-	return get_object(ref, tagged, 1, &obj, err);
+	return get_object(ref, 1, &obj, &oi_deref, err);
 }
 
 /*

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

Re: [PATCH 2/4] ref-filter: add empty values to technical fields

[Junio C Hamano]

Olga Telezhnaya <olyatelezhnaya@gmail.com> writes:

> Atoms like "align" or "end" do not have string representation.
> Earlier we had to go and parse whole object with a hope that we
> could fill their string representations. It's easier to fill them
> with an empty string before we start to work with whole object.
>
> Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
> ---

Just being curious, but is there any meaningful relationship between
what was labelled as SOURCE_NONE in the previous step and what this
step calls "technical fields"?  Things like "upstream" (which is not
affected by the contents of the object, but is affected by the ref
in question) and "if" (which merely exists to construct the language
syntax) would fall into quite different category, so one might be
subset/superset of the other, but I am wondering if we can take
advantage of more table-driven approach taken by the previous step. 


>  ref-filter.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index 8611c24fd57d1..27733ef013bed 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1497,6 +1497,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
>  			refname = get_symref(atom, ref);
>  		else if (starts_with(name, "upstream")) {
>  			const char *branch_name;
> +			v->s = "";
>  			/* only local branches may have an upstream */
>  			if (!skip_prefix(ref->refname, "refs/heads/",
>  					 &branch_name))
> @@ -1509,6 +1510,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
>  			continue;
>  		} else if (atom->u.remote_ref.push) {
>  			const char *branch_name;
> +			v->s = "";
>  			if (!skip_prefix(ref->refname, "refs/heads/",
>  					 &branch_name))
>  				continue;
> @@ -1549,22 +1551,26 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
>  			continue;
>  		} else if (starts_with(name, "align")) {
>  			v->handler = align_atom_handler;
> +			v->s = "";
>  			continue;
>  		} else if (!strcmp(name, "end")) {
>  			v->handler = end_atom_handler;
> +			v->s = "";
>  			continue;
>  		} else if (starts_with(name, "if")) {
>  			const char *s;
> -
> +			v->s = "";
>  			if (skip_prefix(name, "if:", &s))
>  				v->s = xstrdup(s);
>  			v->handler = if_atom_handler;
>  			continue;
>  		} else if (!strcmp(name, "then")) {
>  			v->handler = then_atom_handler;
> +			v->s = "";
>  			continue;
>  		} else if (!strcmp(name, "else")) {
>  			v->handler = else_atom_handler;
> +			v->s = "";
>  			continue;
>  		} else
>  			continue;
>
> --
> https://github.com/git/git/pull/520

Re: [PATCH 2/4] ref-filter: add empty values to technical fields

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

2018-07-10 1:39 GMT+03:00 Junio C Hamano <gitster@pobox.com>:
> Olga Telezhnaya <olyatelezhnaya@gmail.com> writes:
>
>> Atoms like "align" or "end" do not have string representation.
>> Earlier we had to go and parse whole object with a hope that we
>> could fill their string representations. It's easier to fill them
>> with an empty string before we start to work with whole object.
>>
>> Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
>> ---
>
> Just being curious, but is there any meaningful relationship between
> what was labelled as SOURCE_NONE in the previous step and what this
> step calls "technical fields"?  Things like "upstream" (which is not
> affected by the contents of the object, but is affected by the ref
> in question) and "if" (which merely exists to construct the language
> syntax) would fall into quite different category, so one might be
> subset/superset of the other, but I am wondering if we can take
> advantage of more table-driven approach taken by the previous step.

Sorry that it was not clear enough.
SOURCE_NONE fields are the fields that do not require object info.
By "technical fields" in this particular commit I mean fields that
will never be filled. So, it's a good idea to fill such fields with an
empty string and do not take them into account later (when we are
thinking whether we need to get and parse an object or not). I guess
we do not need to mark these "technical fields" in a special way: we
don't need this information. Moreover, some of the fields that I
filled with an empty string here, sometimes have non-empty
representation, and I changed nothing for such cases.

>
>
>>  ref-filter.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/ref-filter.c b/ref-filter.c
>> index 8611c24fd57d1..27733ef013bed 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -1497,6 +1497,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
>>                       refname = get_symref(atom, ref);
>>               else if (starts_with(name, "upstream")) {
>>                       const char *branch_name;
>> +                     v->s = "";
>>                       /* only local branches may have an upstream */
>>                       if (!skip_prefix(ref->refname, "refs/heads/",
>>                                        &branch_name))
>> @@ -1509,6 +1510,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
>>                       continue;
>>               } else if (atom->u.remote_ref.push) {
>>                       const char *branch_name;
>> +                     v->s = "";
>>                       if (!skip_prefix(ref->refname, "refs/heads/",
>>                                        &branch_name))
>>                               continue;
>> @@ -1549,22 +1551,26 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
>>                       continue;
>>               } else if (starts_with(name, "align")) {
>>                       v->handler = align_atom_handler;
>> +                     v->s = "";
>>                       continue;
>>               } else if (!strcmp(name, "end")) {
>>                       v->handler = end_atom_handler;
>> +                     v->s = "";
>>                       continue;
>>               } else if (starts_with(name, "if")) {
>>                       const char *s;
>> -
>> +                     v->s = "";
>>                       if (skip_prefix(name, "if:", &s))
>>                               v->s = xstrdup(s);
>>                       v->handler = if_atom_handler;
>>                       continue;
>>               } else if (!strcmp(name, "then")) {
>>                       v->handler = then_atom_handler;
>> +                     v->s = "";
>>                       continue;
>>               } else if (!strcmp(name, "else")) {
>>                       v->handler = else_atom_handler;
>> +                     v->s = "";
>>                       continue;
>>               } else
>>                       continue;
>>
>> --
>> https://github.com/git/git/pull/520

Re: [PATCH 3/4] ref-filter: merge get_obj and get_object

[SZEDER Gábor]

>  static int get_object(struct ref_array_item *ref, const struct object_id *oid,
> -		       int deref, struct object **obj, struct strbuf *err)
> +		      int deref, struct object **obj, struct strbuf *err)
>  {
>  	int eaten;

Here the variable 'eaten' is declared, but left uninitialized.  This
was fine until now, because ...

>  	int ret = 0;
>  	unsigned long size;
> -	void *buf = get_obj(oid, obj, &size, &eaten);

... this line used to set it anyway.

> +	enum object_type type;
> +	void *buf = read_object_file(oid, &type, &size);
>  	if (!buf)
>  		ret = strbuf_addf_ret(err, -1, _("missing object %s for %s"),
>  				      oid_to_hex(oid), ref->refname);
> -	else if (!*obj)
> -		ret = strbuf_addf_ret(err, -1, _("parse_object_buffer failed on %s for %s"),
> -				      oid_to_hex(oid), ref->refname);
> -	else
> -		grab_values(ref->value, deref, *obj, buf, size);
> +	else {
> +		*obj = parse_object_buffer(oid, type, size, buf, &eaten);

However, with this change 'eaten' is only set here conditionally: if
read_object_file() doesn't return a valid object buffer, then 'eaten'
remains uninitialized.

> +		if (!*obj)
> +			ret = strbuf_addf_ret(err, -1, _("parse_object_buffer failed on %s for %s"),
> +					      oid_to_hex(oid), ref->refname);
> +		else
> +			grab_values(ref->value, deref, *obj, buf, size);
> +	}
>  	if (!eaten)

And ultimately this condition could depend on an uninitialized value.

>  		free(buf);
>  	return ret;
> 
> --
> https://github.com/git/git/pull/520
> 

Re: [PATCH 3/4] ref-filter: merge get_obj and get_object

[Johannes Schindelin]

Hi Olga,

On Mon, 9 Jul 2018, Olga Telezhnaya wrote:

> diff --git a/ref-filter.c b/ref-filter.c
> index 27733ef013bed..f04169f0ea0e3 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1437,20 +1419,24 @@ static const char *get_refname(struct used_atom *atom, struct ref_array_item *re
>  }
>  
>  static int get_object(struct ref_array_item *ref, const struct object_id *oid,
> -		       int deref, struct object **obj, struct strbuf *err)
> +		      int deref, struct object **obj, struct strbuf *err)
>  {
>  	int eaten;
>  	int ret = 0;
>  	unsigned long size;
> -	void *buf = get_obj(oid, obj, &size, &eaten);

So previously, `eaten` has been assigned always... but...

> +	enum object_type type;
> +	void *buf = read_object_file(oid, &type, &size);
>  	if (!buf)
>  		ret = strbuf_addf_ret(err, -1, _("missing object %s for %s"),
>  				      oid_to_hex(oid), ref->refname);
> -	else if (!*obj)
> -		ret = strbuf_addf_ret(err, -1, _("parse_object_buffer failed on %s for %s"),
> -				      oid_to_hex(oid), ref->refname);
> -	else
> -		grab_values(ref->value, deref, *obj, buf, size);
> +	else {
> +		*obj = parse_object_buffer(oid, type, size, buf, &eaten);

... now it only gets assigned in case `buf` was non-`NULL`... yet...

> +		if (!*obj)
> +			ret = strbuf_addf_ret(err, -1, _("parse_object_buffer failed on %s for %s"),
> +					      oid_to_hex(oid), ref->refname);
> +		else
> +			grab_values(ref->value, deref, *obj, buf, size);
> +	}
>  	if (!eaten)
>		free(buf);

... here, we still act on `eaten`. This causes GCC to complain thusly:


```
2018-07-10T04:59:38.6368270Z ref-filter.c:1477:6: error: variable 'eaten' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
2018-07-10T04:59:38.6468620Z         if (oi->info.contentp) {
2018-07-10T04:59:38.6568710Z             ^~~~~~~~~~~~~~~~~
2018-07-10T04:59:38.6669970Z ref-filter.c:1489:7: note: uninitialized use occurs here
2018-07-10T04:59:38.6774240Z         if (!eaten)
2018-07-10T04:59:38.6874860Z              ^~~~~
2018-07-10T04:59:38.6976740Z ref-filter.c:1477:2: note: remove the 'if' if its condition is always true
2018-07-10T04:59:38.7072330Z         if (oi->info.contentp) {
2018-07-10T04:59:38.7172760Z         ^~~~~~~~~~~~~~~~~~~~~~~
2018-07-10T04:59:38.7274040Z ref-filter.c:1466:11: note: initialize the variable 'eaten' to silence this warning
2018-07-10T04:59:38.7374670Z         int eaten;
2018-07-10T04:59:38.7474870Z                  ^
2018-07-10T04:59:38.7575690Z                   = 0
```

(See
https://mseng.visualstudio.com/VSOnline/_build/results?buildId=6640204&view=logs
for details)

I think that GCC is correct, and at the same time, it isn't. Because it
does not matter whether `eaten` is uninitialized here: if it is, then
`buf` is NULL, and the `free(buf);` call does nothing in particular.

However, it *is* sloppy to have a conditional block of code that acts on
an uninitialized value, whether it has adverse consequences or not. So I
would suggest to initialize `eaten` to `1` (not `0` as suggested by GCC
because we can avoid the no-op `free(buf)` while we're already touching
that code path).

Ciao,
Johannes

Re: [PATCH 3/4] ref-filter: merge get_obj and get_object

[SZEDER Gábor]

> This causes GCC to complain thusly:
> 
> 
> ```
> 2018-07-10T04:59:38.6368270Z ref-filter.c:1477:6: error: variable 'eaten' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
> 2018-07-10T04:59:38.6468620Z         if (oi->info.contentp) {
> 2018-07-10T04:59:38.6568710Z             ^~~~~~~~~~~~~~~~~
> 2018-07-10T04:59:38.6669970Z ref-filter.c:1489:7: note: uninitialized use occurs here
> 2018-07-10T04:59:38.6774240Z         if (!eaten)
> 2018-07-10T04:59:38.6874860Z              ^~~~~
> 2018-07-10T04:59:38.6976740Z ref-filter.c:1477:2: note: remove the 'if' if its condition is always true
> 2018-07-10T04:59:38.7072330Z         if (oi->info.contentp) {
> 2018-07-10T04:59:38.7172760Z         ^~~~~~~~~~~~~~~~~~~~~~~
> 2018-07-10T04:59:38.7274040Z ref-filter.c:1466:11: note: initialize the variable 'eaten' to silence this warning
> 2018-07-10T04:59:38.7374670Z         int eaten;
> 2018-07-10T04:59:38.7474870Z                  ^
> 2018-07-10T04:59:38.7575690Z                   = 0
> ```
> 
> (See
> https://mseng.visualstudio.com/VSOnline/_build/results?buildId=6640204&view=logs
> for details)
> 
> I think that GCC is correct, and at the same time, it isn't. Because it
> does not matter whether `eaten` is uninitialized here: 

It's undefined behaviour; 'eaten' is int, and an int may have padding
bits and trap representations.

> if it is, then
> `buf` is NULL, and the `free(buf);` call does nothing in particular.

Re: [PATCH 3/4] ref-filter: merge get_obj and get_object

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

Fully agree, thank you so much.
I have fixed it. Waiting for other issues that need to be fixed, then
I will re-send the patch.

Thank you!

[PATCH v2 2/4] ref-filter: fill empty fields with empty values

[Olga Telezhnaya]

Atoms like "align" or "end" do not have string representation.
Earlier we had to go and parse whole object with a hope that we
could fill their string representations. It's easier to fill them
with an empty string before we start to work with whole object.

It is important to mention that we fill only these atoms that must
contain nothing. So, if we could not fill the atom because, for example,
the object is missing, we leave it with NULL.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
 ref-filter.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/ref-filter.c b/ref-filter.c
index 8611c24fd57d1..27733ef013bed 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1497,6 +1497,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 			refname = get_symref(atom, ref);
 		else if (starts_with(name, "upstream")) {
 			const char *branch_name;
+			v->s = "";
 			/* only local branches may have an upstream */
 			if (!skip_prefix(ref->refname, "refs/heads/",
 					 &branch_name))
@@ -1509,6 +1510,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 			continue;
 		} else if (atom->u.remote_ref.push) {
 			const char *branch_name;
+			v->s = "";
 			if (!skip_prefix(ref->refname, "refs/heads/",
 					 &branch_name))
 				continue;
@@ -1549,22 +1551,26 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 			continue;
 		} else if (starts_with(name, "align")) {
 			v->handler = align_atom_handler;
+			v->s = "";
 			continue;
 		} else if (!strcmp(name, "end")) {
 			v->handler = end_atom_handler;
+			v->s = "";
 			continue;
 		} else if (starts_with(name, "if")) {
 			const char *s;
-
+			v->s = "";
 			if (skip_prefix(name, "if:", &s))
 				v->s = xstrdup(s);
 			v->handler = if_atom_handler;
 			continue;
 		} else if (!strcmp(name, "then")) {
 			v->handler = then_atom_handler;
+			v->s = "";
 			continue;
 		} else if (!strcmp(name, "else")) {
 			v->handler = else_atom_handler;
+			v->s = "";
 			continue;
 		} else
 			continue;

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

[PATCH v2 4/4] ref-filter: use oid_object_info() to get object

[Olga Telezhnaya]

Use oid_object_info_extended() to get object info instead of
read_object_file().
It will help to handle some requests faster (e.g., we do not need to
parse whole object if we need to know %(objectsize)).
It could also help us to add new atoms such as %(objectsize:disk)
and %(deltabase).

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
 ref-filter.c | 123 ++++++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 89 insertions(+), 34 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index f04169f0ea0e3..112955f006648 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -61,6 +61,17 @@ struct refname_atom {
 	int lstrip, rstrip;
 };
 
+static struct expand_data {
+	struct object_id oid;
+	enum object_type type;
+	unsigned long size;
+	off_t disk_size;
+	struct object_id delta_base_oid;
+	void *content;
+
+	struct object_info info;
+} oi, oi_deref;
+
 /*
  * An atom is a valid field atom listed below, possibly prefixed with
  * a "*" to denote deref_tag().
@@ -202,6 +213,30 @@ static int remote_ref_atom_parser(const struct ref_format *format, struct used_a
 	return 0;
 }
 
+static int objecttype_atom_parser(const struct ref_format *format, struct used_atom *atom,
+				  const char *arg, struct strbuf *err)
+{
+	if (arg)
+		return strbuf_addf_ret(err, -1, _("%%(objecttype) does not take arguments"));
+	if (*atom->name == '*')
+		oi_deref.info.typep = &oi_deref.type;
+	else
+		oi.info.typep = &oi.type;
+	return 0;
+}
+
+static int objectsize_atom_parser(const struct ref_format *format, struct used_atom *atom,
+				  const char *arg, struct strbuf *err)
+{
+	if (arg)
+		return strbuf_addf_ret(err, -1, _("%%(objectsize) does not take arguments"));
+	if (*atom->name == '*')
+		oi_deref.info.sizep = &oi_deref.size;
+	else
+		oi.info.sizep = &oi.size;
+	return 0;
+}
+
 static int body_atom_parser(const struct ref_format *format, struct used_atom *atom,
 			    const char *arg, struct strbuf *err)
 {
@@ -388,8 +423,8 @@ static struct {
 		      const char *arg, struct strbuf *err);
 } valid_atom[] = {
 	{ "refname", SOURCE_NONE, FIELD_STR, refname_atom_parser },
-	{ "objecttype", SOURCE_OTHER },
-	{ "objectsize", SOURCE_OTHER, FIELD_ULONG },
+	{ "objecttype", SOURCE_OTHER, FIELD_STR, objecttype_atom_parser },
+	{ "objectsize", SOURCE_OTHER, FIELD_ULONG, objectsize_atom_parser },
 	{ "objectname", SOURCE_OTHER, FIELD_STR, objectname_atom_parser },
 	{ "tree", SOURCE_OBJ },
 	{ "parent", SOURCE_OBJ },
@@ -502,6 +537,12 @@ static int parse_ref_filter_atom(const struct ref_format *format,
 	used_atom[at].name = xmemdupz(atom, ep - atom);
 	used_atom[at].type = valid_atom[i].cmp_type;
 	used_atom[at].source = valid_atom[i].source;
+	if (used_atom[at].source == SOURCE_OBJ) {
+		if (*atom == '*')
+			oi_deref.info.contentp = &oi_deref.content;
+		else
+			oi.info.contentp = &oi.content;
+	}
 	if (arg) {
 		arg = used_atom[at].name + (arg - atom) + 1;
 		if (!*arg) {
@@ -817,7 +858,7 @@ static int grab_objectname(const char *name, const struct object_id *oid,
 }
 
 /* See grab_values */
-static void grab_common_values(struct atom_value *val, int deref, struct object *obj, void *buf, unsigned long sz)
+static void grab_common_values(struct atom_value *val, int deref, struct expand_data *oi)
 {
 	int i;
 
@@ -829,13 +870,13 @@ static void grab_common_values(struct atom_value *val, int deref, struct object
 		if (deref)
 			name++;
 		if (!strcmp(name, "objecttype"))
-			v->s = type_name(obj->type);
+			v->s = type_name(oi->type);
 		else if (!strcmp(name, "objectsize")) {
-			v->value = sz;
-			v->s = xstrfmt("%lu", sz);
+			v->value = oi->size;
+			v->s = xstrfmt("%lu", oi->size);
 		}
 		else if (deref)
-			grab_objectname(name, &obj->oid, v, &used_atom[i]);
+			grab_objectname(name, &oi->oid, v, &used_atom[i]);
 	}
 }
 
@@ -1194,7 +1235,6 @@ static void fill_missing_values(struct atom_value *val)
  */
 static void grab_values(struct atom_value *val, int deref, struct object *obj, void *buf, unsigned long sz)
 {
-	grab_common_values(val, deref, obj, buf, sz);
 	switch (obj->type) {
 	case OBJ_TAG:
 		grab_tag_values(val, deref, obj, buf, sz);
@@ -1418,28 +1458,36 @@ static const char *get_refname(struct used_atom *atom, struct ref_array_item *re
 	return show_ref(&atom->u.refname, ref->refname);
 }
 
-static int get_object(struct ref_array_item *ref, const struct object_id *oid,
-		      int deref, struct object **obj, struct strbuf *err)
+static int get_object(struct ref_array_item *ref, int deref, struct object **obj,
+		      struct expand_data *oi, struct strbuf *err)
 {
-	int eaten;
-	int ret = 0;
-	unsigned long size;
-	enum object_type type;
-	void *buf = read_object_file(oid, &type, &size);
-	if (!buf)
-		ret = strbuf_addf_ret(err, -1, _("missing object %s for %s"),
-				      oid_to_hex(oid), ref->refname);
-	else {
-		*obj = parse_object_buffer(oid, type, size, buf, &eaten);
-		if (!*obj)
-			ret = strbuf_addf_ret(err, -1, _("parse_object_buffer failed on %s for %s"),
-					      oid_to_hex(oid), ref->refname);
-		else
-			grab_values(ref->value, deref, *obj, buf, size);
+	/* parse_object_buffer() will set eaten to 0 if free() will be needed */
+	int eaten = 1;
+	if (oi->info.contentp) {
+		/* We need to know that to use parse_object_buffer properly */
+		oi->info.sizep = &oi->size;
+		oi->info.typep = &oi->type;
 	}
+	if (oid_object_info_extended(the_repository, &oi->oid, &oi->info,
+				     OBJECT_INFO_LOOKUP_REPLACE))
+		return strbuf_addf_ret(err, -1, _("missing object %s for %s"),
+				       oid_to_hex(&oi->oid), ref->refname);
+
+	if (oi->info.contentp) {
+		*obj = parse_object_buffer(&oi->oid, oi->type, oi->size, oi->content, &eaten);
+		if (!obj) {
+			if (!eaten)
+				free(oi->content);
+			return strbuf_addf_ret(err, -1, _("parse_object_buffer failed on %s for %s"),
+					       oid_to_hex(&oi->oid), ref->refname);
+		}
+		grab_values(ref->value, deref, *obj, oi->content, oi->size);
+	}
+
+	grab_common_values(ref->value, deref, oi);
 	if (!eaten)
-		free(buf);
-	return ret;
+		free(oi->content);
+	return 0;
 }
 
 /*
@@ -1449,7 +1497,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 {
 	struct object *obj;
 	int i;
-	const struct object_id *tagged;
+	struct object_info empty = OBJECT_INFO_INIT;
 
 	ref->value = xcalloc(used_atom_cnt, sizeof(struct atom_value));
 
@@ -1569,13 +1617,20 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 
 	for (i = 0; i < used_atom_cnt; i++) {
 		struct atom_value *v = &ref->value[i];
-		if (v->s == NULL)
-			break;
+		if (v->s == NULL && used_atom[i].source == SOURCE_NONE)
+			return strbuf_addf_ret(err, -1, _("missing object %s for %s"),
+					       oid_to_hex(&ref->objectname), ref->refname);
 	}
-	if (used_atom_cnt <= i)
+
+	if (need_tagged)
+		oi.info.contentp = &oi.content;
+	if (!memcmp(&oi.info, &empty, sizeof(empty)) &&
+	    !memcmp(&oi_deref.info, &empty, sizeof(empty)))
 		return 0;
 
-	if (get_object(ref, &ref->objectname, 0, &obj, err))
+
+	oi.oid = ref->objectname;
+	if (get_object(ref, 0, &obj, &oi, err))
 		return -1;
 
 	/*
@@ -1589,7 +1644,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 	 * 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;
+	oi_deref.oid = ((struct tag *)obj)->tagged->oid;
 
 	/*
 	 * NEEDSWORK: This derefs tag only once, which
@@ -1597,7 +1652,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 	 * is not consistent with what deref_tag() does
 	 * which peels the onion to the core.
 	 */
-	return get_object(ref, tagged, 1, &obj, err);
+	return get_object(ref, 1, &obj, &oi_deref, err);
 }
 
 /*

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

[PATCH v2 3/4] ref-filter: merge get_obj and get_object

[Olga Telezhnaya]

Inline get_obj(): it would be easier to edit the code
without this split.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
 ref-filter.c | 36 +++++++++++-------------------------
 1 file changed, 11 insertions(+), 25 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 27733ef013bed..f04169f0ea0e3 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -797,24 +797,6 @@ int verify_ref_format(struct ref_format *format)
 	return 0;
 }
 
-/*
- * Given an object name, read the object data and size, and return a
- * "struct object".  If the object data we are returning is also borrowed
- * by the "struct object" representation, set *eaten as well---it is a
- * signal from parse_object_buffer to us not to free the buffer.
- */
-static void *get_obj(const struct object_id *oid, struct object **obj, unsigned long *sz, int *eaten)
-{
-	enum object_type type;
-	void *buf = read_object_file(oid, &type, sz);
-
-	if (buf)
-		*obj = parse_object_buffer(oid, type, *sz, buf, eaten);
-	else
-		*obj = NULL;
-	return buf;
-}
-
 static int grab_objectname(const char *name, const struct object_id *oid,
 			   struct atom_value *v, struct used_atom *atom)
 {
@@ -1437,20 +1419,24 @@ static const char *get_refname(struct used_atom *atom, struct ref_array_item *re
 }
 
 static int get_object(struct ref_array_item *ref, const struct object_id *oid,
-		       int deref, struct object **obj, struct strbuf *err)
+		      int deref, struct object **obj, struct strbuf *err)
 {
 	int eaten;
 	int ret = 0;
 	unsigned long size;
-	void *buf = get_obj(oid, obj, &size, &eaten);
+	enum object_type type;
+	void *buf = read_object_file(oid, &type, &size);
 	if (!buf)
 		ret = strbuf_addf_ret(err, -1, _("missing object %s for %s"),
 				      oid_to_hex(oid), ref->refname);
-	else if (!*obj)
-		ret = strbuf_addf_ret(err, -1, _("parse_object_buffer failed on %s for %s"),
-				      oid_to_hex(oid), ref->refname);
-	else
-		grab_values(ref->value, deref, *obj, buf, size);
+	else {
+		*obj = parse_object_buffer(oid, type, size, buf, &eaten);
+		if (!*obj)
+			ret = strbuf_addf_ret(err, -1, _("parse_object_buffer failed on %s for %s"),
+					      oid_to_hex(oid), ref->refname);
+		else
+			grab_values(ref->value, deref, *obj, buf, size);
+	}
 	if (!eaten)
 		free(buf);
 	return ret;

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

[PATCH v2 1/4] ref-filter: add info_source to valid_atom

[Olga Telezhnaya]

Add the source of object data to prevent parsing of unneeded data.
The goal is to improve performance by avoiding calling expensive
functions when we don't need the information they provide
or when we could get it by using a cheaper function.

It is stored in valid_atoms because it depends on the atoms we are
interested in.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
 ref-filter.c | 82 +++++++++++++++++++++++++++++++-----------------------------
 1 file changed, 43 insertions(+), 39 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index fa3685d91f046..8611c24fd57d1 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -41,6 +41,7 @@ void setup_ref_filter_porcelain_msg(void)
 
 typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
 typedef enum { COMPARE_EQUAL, COMPARE_UNEQUAL, COMPARE_NONE } cmp_status;
+typedef enum { SOURCE_NONE = 0, SOURCE_OBJ, SOURCE_OTHER } info_source;
 
 struct align {
 	align_type position;
@@ -73,6 +74,7 @@ struct refname_atom {
 static struct used_atom {
 	const char *name;
 	cmp_type type;
+	info_source source;
 	union {
 		char color[COLOR_MAXLEN];
 		struct align align;
@@ -380,49 +382,50 @@ static int head_atom_parser(const struct ref_format *format, struct used_atom *a
 
 static struct {
 	const char *name;
+	info_source source;
 	cmp_type cmp_type;
 	int (*parser)(const struct ref_format *format, struct used_atom *atom,
 		      const char *arg, struct strbuf *err);
 } valid_atom[] = {
-	{ "refname" , FIELD_STR, refname_atom_parser },
-	{ "objecttype" },
-	{ "objectsize", FIELD_ULONG },
-	{ "objectname", FIELD_STR, objectname_atom_parser },
-	{ "tree" },
-	{ "parent" },
-	{ "numparent", FIELD_ULONG },
-	{ "object" },
-	{ "type" },
-	{ "tag" },
-	{ "author" },
-	{ "authorname" },
-	{ "authoremail" },
-	{ "authordate", FIELD_TIME },
-	{ "committer" },
-	{ "committername" },
-	{ "committeremail" },
-	{ "committerdate", FIELD_TIME },
-	{ "tagger" },
-	{ "taggername" },
-	{ "taggeremail" },
-	{ "taggerdate", FIELD_TIME },
-	{ "creator" },
-	{ "creatordate", FIELD_TIME },
-	{ "subject", FIELD_STR, subject_atom_parser },
-	{ "body", FIELD_STR, body_atom_parser },
-	{ "trailers", FIELD_STR, trailers_atom_parser },
-	{ "contents", FIELD_STR, contents_atom_parser },
-	{ "upstream", FIELD_STR, remote_ref_atom_parser },
-	{ "push", FIELD_STR, remote_ref_atom_parser },
-	{ "symref", FIELD_STR, refname_atom_parser },
-	{ "flag" },
-	{ "HEAD", FIELD_STR, head_atom_parser },
-	{ "color", FIELD_STR, color_atom_parser },
-	{ "align", FIELD_STR, align_atom_parser },
-	{ "end" },
-	{ "if", FIELD_STR, if_atom_parser },
-	{ "then" },
-	{ "else" },
+	{ "refname", SOURCE_NONE, FIELD_STR, refname_atom_parser },
+	{ "objecttype", SOURCE_OTHER },
+	{ "objectsize", SOURCE_OTHER, FIELD_ULONG },
+	{ "objectname", SOURCE_OTHER, FIELD_STR, objectname_atom_parser },
+	{ "tree", SOURCE_OBJ },
+	{ "parent", SOURCE_OBJ },
+	{ "numparent", SOURCE_OBJ, FIELD_ULONG },
+	{ "object", SOURCE_OBJ },
+	{ "type", SOURCE_OBJ },
+	{ "tag", SOURCE_OBJ },
+	{ "author", SOURCE_OBJ },
+	{ "authorname", SOURCE_OBJ },
+	{ "authoremail", SOURCE_OBJ },
+	{ "authordate", SOURCE_OBJ, FIELD_TIME },
+	{ "committer", SOURCE_OBJ },
+	{ "committername", SOURCE_OBJ },
+	{ "committeremail", SOURCE_OBJ },
+	{ "committerdate", SOURCE_OBJ, FIELD_TIME },
+	{ "tagger", SOURCE_OBJ },
+	{ "taggername", SOURCE_OBJ },
+	{ "taggeremail", SOURCE_OBJ },
+	{ "taggerdate", SOURCE_OBJ, FIELD_TIME },
+	{ "creator", SOURCE_OBJ },
+	{ "creatordate", SOURCE_OBJ, FIELD_TIME },
+	{ "subject", SOURCE_OBJ, FIELD_STR, subject_atom_parser },
+	{ "body", SOURCE_OBJ, FIELD_STR, body_atom_parser },
+	{ "trailers", SOURCE_OBJ, FIELD_STR, trailers_atom_parser },
+	{ "contents", SOURCE_OBJ, FIELD_STR, contents_atom_parser },
+	{ "upstream", SOURCE_NONE, FIELD_STR, remote_ref_atom_parser },
+	{ "push", SOURCE_NONE, FIELD_STR, remote_ref_atom_parser },
+	{ "symref", SOURCE_NONE, FIELD_STR, refname_atom_parser },
+	{ "flag", SOURCE_NONE },
+	{ "HEAD", SOURCE_NONE, FIELD_STR, head_atom_parser },
+	{ "color", SOURCE_NONE, FIELD_STR, color_atom_parser },
+	{ "align", SOURCE_NONE, FIELD_STR, align_atom_parser },
+	{ "end", SOURCE_NONE },
+	{ "if", SOURCE_NONE, FIELD_STR, if_atom_parser },
+	{ "then", SOURCE_NONE },
+	{ "else", SOURCE_NONE },
 };
 
 #define REF_FORMATTING_STATE_INIT  { 0, NULL }
@@ -498,6 +501,7 @@ static int parse_ref_filter_atom(const struct ref_format *format,
 	REALLOC_ARRAY(used_atom, used_atom_cnt);
 	used_atom[at].name = xmemdupz(atom, ep - atom);
 	used_atom[at].type = valid_atom[i].cmp_type;
+	used_atom[at].source = valid_atom[i].source;
 	if (arg) {
 		arg = used_atom[at].name + (arg - atom) + 1;
 		if (!*arg) {

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

Re: [PATCH v2 4/4] ref-filter: use oid_object_info() to get object

[Junio C Hamano]

Olga Telezhnaya <olyatelezhnaya@gmail.com> writes:

> -static int get_object(struct ref_array_item *ref, const struct object_id *oid,
> -		      int deref, struct object **obj, struct strbuf *err)
> +static int get_object(struct ref_array_item *ref, int deref, struct object **obj,
> +		      struct expand_data *oi, struct strbuf *err)
>  {
> -	int eaten;
> -	int ret = 0;
> -	unsigned long size;
> -	enum object_type type;
> -	void *buf = read_object_file(oid, &type, &size);
> -	if (!buf)
> -		ret = strbuf_addf_ret(err, -1, _("missing object %s for %s"),
> -				      oid_to_hex(oid), ref->refname);
> -	else {
> -		*obj = parse_object_buffer(oid, type, size, buf, &eaten);
> -		if (!*obj)
> -			ret = strbuf_addf_ret(err, -1, _("parse_object_buffer failed on %s for %s"),
> -					      oid_to_hex(oid), ref->refname);
> -		else
> -			grab_values(ref->value, deref, *obj, buf, size);
> +	/* parse_object_buffer() will set eaten to 0 if free() will be needed */
> +	int eaten = 1;

Hmph, doesn't this belong to the previous step?  In other words,
isn't the result of applying 1-3/4 has a bug that can leave eaten
uninitialized (and base decision to free(buf) later on it), and
isn't this change a fix for it?

> +	if (oi->info.contentp) {
> +		/* We need to know that to use parse_object_buffer properly */
> +		oi->info.sizep = &oi->size;
> +		oi->info.typep = &oi->type;
>  	}
> +	if (oid_object_info_extended(the_repository, &oi->oid, &oi->info,
> +				     OBJECT_INFO_LOOKUP_REPLACE))
> +		return strbuf_addf_ret(err, -1, _("missing object %s for %s"),
> +				       oid_to_hex(&oi->oid), ref->refname);
> +
> +	if (oi->info.contentp) {
> +		*obj = parse_object_buffer(&oi->oid, oi->type, oi->size, oi->content, &eaten);
> +		if (!obj) {
> +			if (!eaten)
> +				free(oi->content);
> +			return strbuf_addf_ret(err, -1, _("parse_object_buffer failed on %s for %s"),
> +					       oid_to_hex(&oi->oid), ref->refname);
> +		}
> +		grab_values(ref->value, deref, *obj, oi->content, oi->size);
> +	}
> +
> +	grab_common_values(ref->value, deref, oi);
>  	if (!eaten)
> -		free(buf);
> -	return ret;
> +		free(oi->content);
> +	return 0;
>  }

Re: [PATCH v2 4/4] ref-filter: use oid_object_info() to get object

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

2018-07-16 23:53 GMT+03:00 Junio C Hamano <gitster@pobox.com>:
> Olga Telezhnaya <olyatelezhnaya@gmail.com> writes:
>
>> -static int get_object(struct ref_array_item *ref, const struct object_id *oid,
>> -                   int deref, struct object **obj, struct strbuf *err)
>> +static int get_object(struct ref_array_item *ref, int deref, struct object **obj,
>> +                   struct expand_data *oi, struct strbuf *err)
>>  {
>> -     int eaten;
>> -     int ret = 0;
>> -     unsigned long size;
>> -     enum object_type type;
>> -     void *buf = read_object_file(oid, &type, &size);
>> -     if (!buf)
>> -             ret = strbuf_addf_ret(err, -1, _("missing object %s for %s"),
>> -                                   oid_to_hex(oid), ref->refname);
>> -     else {
>> -             *obj = parse_object_buffer(oid, type, size, buf, &eaten);
>> -             if (!*obj)
>> -                     ret = strbuf_addf_ret(err, -1, _("parse_object_buffer failed on %s for %s"),
>> -                                           oid_to_hex(oid), ref->refname);
>> -             else
>> -                     grab_values(ref->value, deref, *obj, buf, size);
>> +     /* parse_object_buffer() will set eaten to 0 if free() will be needed */
>> +     int eaten = 1;
>
> Hmph, doesn't this belong to the previous step?  In other words,
> isn't the result of applying 1-3/4 has a bug that can leave eaten
> uninitialized (and base decision to free(buf) later on it), and
> isn't this change a fix for it?

Oh. I was thinking that it was new bug created by me. Now I see that
previously we had the same problem. I guess it's better to make
separate commit to fix it. Hope I can do it in this patch (I don't
want to create separate patch because of so minor update).
Thank you! I will fix it soon.

>
>> +     if (oi->info.contentp) {
>> +             /* We need to know that to use parse_object_buffer properly */
>> +             oi->info.sizep = &oi->size;
>> +             oi->info.typep = &oi->type;
>>       }
>> +     if (oid_object_info_extended(the_repository, &oi->oid, &oi->info,
>> +                                  OBJECT_INFO_LOOKUP_REPLACE))
>> +             return strbuf_addf_ret(err, -1, _("missing object %s for %s"),
>> +                                    oid_to_hex(&oi->oid), ref->refname);
>> +
>> +     if (oi->info.contentp) {
>> +             *obj = parse_object_buffer(&oi->oid, oi->type, oi->size, oi->content, &eaten);
>> +             if (!obj) {
>> +                     if (!eaten)
>> +                             free(oi->content);
>> +                     return strbuf_addf_ret(err, -1, _("parse_object_buffer failed on %s for %s"),
>> +                                            oid_to_hex(&oi->oid), ref->refname);
>> +             }
>> +             grab_values(ref->value, deref, *obj, oi->content, oi->size);
>> +     }
>> +
>> +     grab_common_values(ref->value, deref, oi);
>>       if (!eaten)
>> -             free(buf);
>> -     return ret;
>> +             free(oi->content);
>> +     return 0;
>>  }
>

[PATCH v3 3/5] ref-filter: initialize eaten variable

[Olga Telezhnaya]

Initialize variable `eaten` before its using. We may initialize it in
parse_object_buffer(), but there are cases when we do not reach this
invocation.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
 ref-filter.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/ref-filter.c b/ref-filter.c
index 27733ef013bed..8db7ca95b12c0 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1439,7 +1439,8 @@ static const char *get_refname(struct used_atom *atom, struct ref_array_item *re
 static int get_object(struct ref_array_item *ref, const struct object_id *oid,
 		       int deref, struct object **obj, struct strbuf *err)
 {
-	int eaten;
+	/* parse_object_buffer() will set eaten to 0 if free() will be needed */
+	int eaten = 1;
 	int ret = 0;
 	unsigned long size;
 	void *buf = get_obj(oid, obj, &size, &eaten);

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

[PATCH v3 2/5] ref-filter: fill empty fields with empty values

[Olga Telezhnaya]

Atoms like "align" or "end" do not have string representation.
Earlier we had to go and parse whole object with a hope that we
could fill their string representations. It's easier to fill them
with an empty string before we start to work with whole object.

It is important to mention that we fill only these atoms that must
contain nothing. So, if we could not fill the atom because, for example,
the object is missing, we leave it with NULL.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
 ref-filter.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/ref-filter.c b/ref-filter.c
index 8611c24fd57d1..27733ef013bed 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1497,6 +1497,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 			refname = get_symref(atom, ref);
 		else if (starts_with(name, "upstream")) {
 			const char *branch_name;
+			v->s = "";
 			/* only local branches may have an upstream */
 			if (!skip_prefix(ref->refname, "refs/heads/",
 					 &branch_name))
@@ -1509,6 +1510,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 			continue;
 		} else if (atom->u.remote_ref.push) {
 			const char *branch_name;
+			v->s = "";
 			if (!skip_prefix(ref->refname, "refs/heads/",
 					 &branch_name))
 				continue;
@@ -1549,22 +1551,26 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 			continue;
 		} else if (starts_with(name, "align")) {
 			v->handler = align_atom_handler;
+			v->s = "";
 			continue;
 		} else if (!strcmp(name, "end")) {
 			v->handler = end_atom_handler;
+			v->s = "";
 			continue;
 		} else if (starts_with(name, "if")) {
 			const char *s;
-
+			v->s = "";
 			if (skip_prefix(name, "if:", &s))
 				v->s = xstrdup(s);
 			v->handler = if_atom_handler;
 			continue;
 		} else if (!strcmp(name, "then")) {
 			v->handler = then_atom_handler;
+			v->s = "";
 			continue;
 		} else if (!strcmp(name, "else")) {
 			v->handler = else_atom_handler;
+			v->s = "";
 			continue;
 		} else
 			continue;

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

[PATCH v3 5/5] ref-filter: use oid_object_info() to get object

[Olga Telezhnaya]

Use oid_object_info_extended() to get object info instead of
read_object_file().
It will help to handle some requests faster (e.g., we do not need to
parse whole object if we need to know %(objectsize)).
It could also help us to add new atoms such as %(objectsize:disk)
and %(deltabase).

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
 ref-filter.c | 120 +++++++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 87 insertions(+), 33 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 2b401a17c4689..112955f006648 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -61,6 +61,17 @@ struct refname_atom {
 	int lstrip, rstrip;
 };
 
+static struct expand_data {
+	struct object_id oid;
+	enum object_type type;
+	unsigned long size;
+	off_t disk_size;
+	struct object_id delta_base_oid;
+	void *content;
+
+	struct object_info info;
+} oi, oi_deref;
+
 /*
  * An atom is a valid field atom listed below, possibly prefixed with
  * a "*" to denote deref_tag().
@@ -202,6 +213,30 @@ static int remote_ref_atom_parser(const struct ref_format *format, struct used_a
 	return 0;
 }
 
+static int objecttype_atom_parser(const struct ref_format *format, struct used_atom *atom,
+				  const char *arg, struct strbuf *err)
+{
+	if (arg)
+		return strbuf_addf_ret(err, -1, _("%%(objecttype) does not take arguments"));
+	if (*atom->name == '*')
+		oi_deref.info.typep = &oi_deref.type;
+	else
+		oi.info.typep = &oi.type;
+	return 0;
+}
+
+static int objectsize_atom_parser(const struct ref_format *format, struct used_atom *atom,
+				  const char *arg, struct strbuf *err)
+{
+	if (arg)
+		return strbuf_addf_ret(err, -1, _("%%(objectsize) does not take arguments"));
+	if (*atom->name == '*')
+		oi_deref.info.sizep = &oi_deref.size;
+	else
+		oi.info.sizep = &oi.size;
+	return 0;
+}
+
 static int body_atom_parser(const struct ref_format *format, struct used_atom *atom,
 			    const char *arg, struct strbuf *err)
 {
@@ -388,8 +423,8 @@ static struct {
 		      const char *arg, struct strbuf *err);
 } valid_atom[] = {
 	{ "refname", SOURCE_NONE, FIELD_STR, refname_atom_parser },
-	{ "objecttype", SOURCE_OTHER },
-	{ "objectsize", SOURCE_OTHER, FIELD_ULONG },
+	{ "objecttype", SOURCE_OTHER, FIELD_STR, objecttype_atom_parser },
+	{ "objectsize", SOURCE_OTHER, FIELD_ULONG, objectsize_atom_parser },
 	{ "objectname", SOURCE_OTHER, FIELD_STR, objectname_atom_parser },
 	{ "tree", SOURCE_OBJ },
 	{ "parent", SOURCE_OBJ },
@@ -502,6 +537,12 @@ static int parse_ref_filter_atom(const struct ref_format *format,
 	used_atom[at].name = xmemdupz(atom, ep - atom);
 	used_atom[at].type = valid_atom[i].cmp_type;
 	used_atom[at].source = valid_atom[i].source;
+	if (used_atom[at].source == SOURCE_OBJ) {
+		if (*atom == '*')
+			oi_deref.info.contentp = &oi_deref.content;
+		else
+			oi.info.contentp = &oi.content;
+	}
 	if (arg) {
 		arg = used_atom[at].name + (arg - atom) + 1;
 		if (!*arg) {
@@ -817,7 +858,7 @@ static int grab_objectname(const char *name, const struct object_id *oid,
 }
 
 /* See grab_values */
-static void grab_common_values(struct atom_value *val, int deref, struct object *obj, void *buf, unsigned long sz)
+static void grab_common_values(struct atom_value *val, int deref, struct expand_data *oi)
 {
 	int i;
 
@@ -829,13 +870,13 @@ static void grab_common_values(struct atom_value *val, int deref, struct object
 		if (deref)
 			name++;
 		if (!strcmp(name, "objecttype"))
-			v->s = type_name(obj->type);
+			v->s = type_name(oi->type);
 		else if (!strcmp(name, "objectsize")) {
-			v->value = sz;
-			v->s = xstrfmt("%lu", sz);
+			v->value = oi->size;
+			v->s = xstrfmt("%lu", oi->size);
 		}
 		else if (deref)
-			grab_objectname(name, &obj->oid, v, &used_atom[i]);
+			grab_objectname(name, &oi->oid, v, &used_atom[i]);
 	}
 }
 
@@ -1194,7 +1235,6 @@ static void fill_missing_values(struct atom_value *val)
  */
 static void grab_values(struct atom_value *val, int deref, struct object *obj, void *buf, unsigned long sz)
 {
-	grab_common_values(val, deref, obj, buf, sz);
 	switch (obj->type) {
 	case OBJ_TAG:
 		grab_tag_values(val, deref, obj, buf, sz);
@@ -1418,29 +1458,36 @@ static const char *get_refname(struct used_atom *atom, struct ref_array_item *re
 	return show_ref(&atom->u.refname, ref->refname);
 }
 
-static int get_object(struct ref_array_item *ref, const struct object_id *oid,
-		      int deref, struct object **obj, struct strbuf *err)
+static int get_object(struct ref_array_item *ref, int deref, struct object **obj,
+		      struct expand_data *oi, struct strbuf *err)
 {
 	/* parse_object_buffer() will set eaten to 0 if free() will be needed */
 	int eaten = 1;
-	int ret = 0;
-	unsigned long size;
-	enum object_type type;
-	void *buf = read_object_file(oid, &type, &size);
-	if (!buf)
-		ret = strbuf_addf_ret(err, -1, _("missing object %s for %s"),
-				      oid_to_hex(oid), ref->refname);
-	else {
-		*obj = parse_object_buffer(oid, type, size, buf, &eaten);
-		if (!*obj)
-			ret = strbuf_addf_ret(err, -1, _("parse_object_buffer failed on %s for %s"),
-					      oid_to_hex(oid), ref->refname);
-		else
-			grab_values(ref->value, deref, *obj, buf, size);
+	if (oi->info.contentp) {
+		/* We need to know that to use parse_object_buffer properly */
+		oi->info.sizep = &oi->size;
+		oi->info.typep = &oi->type;
 	}
+	if (oid_object_info_extended(the_repository, &oi->oid, &oi->info,
+				     OBJECT_INFO_LOOKUP_REPLACE))
+		return strbuf_addf_ret(err, -1, _("missing object %s for %s"),
+				       oid_to_hex(&oi->oid), ref->refname);
+
+	if (oi->info.contentp) {
+		*obj = parse_object_buffer(&oi->oid, oi->type, oi->size, oi->content, &eaten);
+		if (!obj) {
+			if (!eaten)
+				free(oi->content);
+			return strbuf_addf_ret(err, -1, _("parse_object_buffer failed on %s for %s"),
+					       oid_to_hex(&oi->oid), ref->refname);
+		}
+		grab_values(ref->value, deref, *obj, oi->content, oi->size);
+	}
+
+	grab_common_values(ref->value, deref, oi);
 	if (!eaten)
-		free(buf);
-	return ret;
+		free(oi->content);
+	return 0;
 }
 
 /*
@@ -1450,7 +1497,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 {
 	struct object *obj;
 	int i;
-	const struct object_id *tagged;
+	struct object_info empty = OBJECT_INFO_INIT;
 
 	ref->value = xcalloc(used_atom_cnt, sizeof(struct atom_value));
 
@@ -1570,13 +1617,20 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 
 	for (i = 0; i < used_atom_cnt; i++) {
 		struct atom_value *v = &ref->value[i];
-		if (v->s == NULL)
-			break;
+		if (v->s == NULL && used_atom[i].source == SOURCE_NONE)
+			return strbuf_addf_ret(err, -1, _("missing object %s for %s"),
+					       oid_to_hex(&ref->objectname), ref->refname);
 	}
-	if (used_atom_cnt <= i)
+
+	if (need_tagged)
+		oi.info.contentp = &oi.content;
+	if (!memcmp(&oi.info, &empty, sizeof(empty)) &&
+	    !memcmp(&oi_deref.info, &empty, sizeof(empty)))
 		return 0;
 
-	if (get_object(ref, &ref->objectname, 0, &obj, err))
+
+	oi.oid = ref->objectname;
+	if (get_object(ref, 0, &obj, &oi, err))
 		return -1;
 
 	/*
@@ -1590,7 +1644,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 	 * 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;
+	oi_deref.oid = ((struct tag *)obj)->tagged->oid;
 
 	/*
 	 * NEEDSWORK: This derefs tag only once, which
@@ -1598,7 +1652,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
 	 * is not consistent with what deref_tag() does
 	 * which peels the onion to the core.
 	 */
-	return get_object(ref, tagged, 1, &obj, err);
+	return get_object(ref, 1, &obj, &oi_deref, err);
 }
 
 /*

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

[PATCH v3 4/5] ref-filter: merge get_obj and get_object

[Olga Telezhnaya]

Inline get_obj(): it would be easier to edit the code
without this split.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
 ref-filter.c | 36 +++++++++++-------------------------
 1 file changed, 11 insertions(+), 25 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 8db7ca95b12c0..2b401a17c4689 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -797,24 +797,6 @@ int verify_ref_format(struct ref_format *format)
 	return 0;
 }
 
-/*
- * Given an object name, read the object data and size, and return a
- * "struct object".  If the object data we are returning is also borrowed
- * by the "struct object" representation, set *eaten as well---it is a
- * signal from parse_object_buffer to us not to free the buffer.
- */
-static void *get_obj(const struct object_id *oid, struct object **obj, unsigned long *sz, int *eaten)
-{
-	enum object_type type;
-	void *buf = read_object_file(oid, &type, sz);
-
-	if (buf)
-		*obj = parse_object_buffer(oid, type, *sz, buf, eaten);
-	else
-		*obj = NULL;
-	return buf;
-}
-
 static int grab_objectname(const char *name, const struct object_id *oid,
 			   struct atom_value *v, struct used_atom *atom)
 {
@@ -1437,21 +1419,25 @@ static const char *get_refname(struct used_atom *atom, struct ref_array_item *re
 }
 
 static int get_object(struct ref_array_item *ref, const struct object_id *oid,
-		       int deref, struct object **obj, struct strbuf *err)
+		      int deref, struct object **obj, struct strbuf *err)
 {
 	/* parse_object_buffer() will set eaten to 0 if free() will be needed */
 	int eaten = 1;
 	int ret = 0;
 	unsigned long size;
-	void *buf = get_obj(oid, obj, &size, &eaten);
+	enum object_type type;
+	void *buf = read_object_file(oid, &type, &size);
 	if (!buf)
 		ret = strbuf_addf_ret(err, -1, _("missing object %s for %s"),
 				      oid_to_hex(oid), ref->refname);
-	else if (!*obj)
-		ret = strbuf_addf_ret(err, -1, _("parse_object_buffer failed on %s for %s"),
-				      oid_to_hex(oid), ref->refname);
-	else
-		grab_values(ref->value, deref, *obj, buf, size);
+	else {
+		*obj = parse_object_buffer(oid, type, size, buf, &eaten);
+		if (!*obj)
+			ret = strbuf_addf_ret(err, -1, _("parse_object_buffer failed on %s for %s"),
+					      oid_to_hex(oid), ref->refname);
+		else
+			grab_values(ref->value, deref, *obj, buf, size);
+	}
 	if (!eaten)
 		free(buf);
 	return ret;

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

[PATCH v3 1/5] ref-filter: add info_source to valid_atom

[Olga Telezhnaya]

Add the source of object data to prevent parsing of unneeded data.
The goal is to improve performance by avoiding calling expensive
functions when we don't need the information they provide
or when we could get it by using a cheaper function.

It is stored in valid_atoms because it depends on the atoms we are
interested in.

Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
---
 ref-filter.c | 82 +++++++++++++++++++++++++++++++-----------------------------
 1 file changed, 43 insertions(+), 39 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index fa3685d91f046..8611c24fd57d1 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -41,6 +41,7 @@ void setup_ref_filter_porcelain_msg(void)
 
 typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
 typedef enum { COMPARE_EQUAL, COMPARE_UNEQUAL, COMPARE_NONE } cmp_status;
+typedef enum { SOURCE_NONE = 0, SOURCE_OBJ, SOURCE_OTHER } info_source;
 
 struct align {
 	align_type position;
@@ -73,6 +74,7 @@ struct refname_atom {
 static struct used_atom {
 	const char *name;
 	cmp_type type;
+	info_source source;
 	union {
 		char color[COLOR_MAXLEN];
 		struct align align;
@@ -380,49 +382,50 @@ static int head_atom_parser(const struct ref_format *format, struct used_atom *a
 
 static struct {
 	const char *name;
+	info_source source;
 	cmp_type cmp_type;
 	int (*parser)(const struct ref_format *format, struct used_atom *atom,
 		      const char *arg, struct strbuf *err);
 } valid_atom[] = {
-	{ "refname" , FIELD_STR, refname_atom_parser },
-	{ "objecttype" },
-	{ "objectsize", FIELD_ULONG },
-	{ "objectname", FIELD_STR, objectname_atom_parser },
-	{ "tree" },
-	{ "parent" },
-	{ "numparent", FIELD_ULONG },
-	{ "object" },
-	{ "type" },
-	{ "tag" },
-	{ "author" },
-	{ "authorname" },
-	{ "authoremail" },
-	{ "authordate", FIELD_TIME },
-	{ "committer" },
-	{ "committername" },
-	{ "committeremail" },
-	{ "committerdate", FIELD_TIME },
-	{ "tagger" },
-	{ "taggername" },
-	{ "taggeremail" },
-	{ "taggerdate", FIELD_TIME },
-	{ "creator" },
-	{ "creatordate", FIELD_TIME },
-	{ "subject", FIELD_STR, subject_atom_parser },
-	{ "body", FIELD_STR, body_atom_parser },
-	{ "trailers", FIELD_STR, trailers_atom_parser },
-	{ "contents", FIELD_STR, contents_atom_parser },
-	{ "upstream", FIELD_STR, remote_ref_atom_parser },
-	{ "push", FIELD_STR, remote_ref_atom_parser },
-	{ "symref", FIELD_STR, refname_atom_parser },
-	{ "flag" },
-	{ "HEAD", FIELD_STR, head_atom_parser },
-	{ "color", FIELD_STR, color_atom_parser },
-	{ "align", FIELD_STR, align_atom_parser },
-	{ "end" },
-	{ "if", FIELD_STR, if_atom_parser },
-	{ "then" },
-	{ "else" },
+	{ "refname", SOURCE_NONE, FIELD_STR, refname_atom_parser },
+	{ "objecttype", SOURCE_OTHER },
+	{ "objectsize", SOURCE_OTHER, FIELD_ULONG },
+	{ "objectname", SOURCE_OTHER, FIELD_STR, objectname_atom_parser },
+	{ "tree", SOURCE_OBJ },
+	{ "parent", SOURCE_OBJ },
+	{ "numparent", SOURCE_OBJ, FIELD_ULONG },
+	{ "object", SOURCE_OBJ },
+	{ "type", SOURCE_OBJ },
+	{ "tag", SOURCE_OBJ },
+	{ "author", SOURCE_OBJ },
+	{ "authorname", SOURCE_OBJ },
+	{ "authoremail", SOURCE_OBJ },
+	{ "authordate", SOURCE_OBJ, FIELD_TIME },
+	{ "committer", SOURCE_OBJ },
+	{ "committername", SOURCE_OBJ },
+	{ "committeremail", SOURCE_OBJ },
+	{ "committerdate", SOURCE_OBJ, FIELD_TIME },
+	{ "tagger", SOURCE_OBJ },
+	{ "taggername", SOURCE_OBJ },
+	{ "taggeremail", SOURCE_OBJ },
+	{ "taggerdate", SOURCE_OBJ, FIELD_TIME },
+	{ "creator", SOURCE_OBJ },
+	{ "creatordate", SOURCE_OBJ, FIELD_TIME },
+	{ "subject", SOURCE_OBJ, FIELD_STR, subject_atom_parser },
+	{ "body", SOURCE_OBJ, FIELD_STR, body_atom_parser },
+	{ "trailers", SOURCE_OBJ, FIELD_STR, trailers_atom_parser },
+	{ "contents", SOURCE_OBJ, FIELD_STR, contents_atom_parser },
+	{ "upstream", SOURCE_NONE, FIELD_STR, remote_ref_atom_parser },
+	{ "push", SOURCE_NONE, FIELD_STR, remote_ref_atom_parser },
+	{ "symref", SOURCE_NONE, FIELD_STR, refname_atom_parser },
+	{ "flag", SOURCE_NONE },
+	{ "HEAD", SOURCE_NONE, FIELD_STR, head_atom_parser },
+	{ "color", SOURCE_NONE, FIELD_STR, color_atom_parser },
+	{ "align", SOURCE_NONE, FIELD_STR, align_atom_parser },
+	{ "end", SOURCE_NONE },
+	{ "if", SOURCE_NONE, FIELD_STR, if_atom_parser },
+	{ "then", SOURCE_NONE },
+	{ "else", SOURCE_NONE },
 };
 
 #define REF_FORMATTING_STATE_INIT  { 0, NULL }
@@ -498,6 +501,7 @@ static int parse_ref_filter_atom(const struct ref_format *format,
 	REALLOC_ARRAY(used_atom, used_atom_cnt);
 	used_atom[at].name = xmemdupz(atom, ep - atom);
 	used_atom[at].type = valid_atom[i].cmp_type;
+	used_atom[at].source = valid_atom[i].source;
 	if (arg) {
 		arg = used_atom[at].name + (arg - atom) + 1;
 		if (!*arg) {

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

Re: [PATCH v2 4/4] ref-filter: use oid_object_info() to get object

[Junio C Hamano]

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

>> Hmph, doesn't this belong to the previous step?  In other words,
>> isn't the result of applying 1-3/4 has a bug that can leave eaten
>> uninitialized (and base decision to free(buf) later on it), and
>> isn't this change a fix for it?
>
> Oh. I was thinking that it was new bug created by me. Now I see that
> previously we had the same problem.

The original said something like:

	int eaten;
	void *buf = get_obj(..., &eaten);
	...
	if (!eaten)
		free(buf);

and get_obj() left eaten untouched when it returned NULL.  As a
random uninitialized cruft in eaten that happened to be "true" would
just cause free(NULL) on many archs, there was no practical problem
in such a code, but it is undefined behaviour nevertheless.

And the previous step made it a bit more alarming ;-)