{"thread":{"id":"47816","subject":"[PATCH v3 03/23] cat-file: reuse struct ref_format","startedAt":"2018-02-12T08:09:04Z","lastAt":"2018-02-19T01:05:03Z","messageCount":56,"participants":["Olga Telezhnaya","Jeff King","Оля Тележная","Junio C Hamano","Adam Dinwoodie","Ramsay Jones"],"isPatch":true,"patchVersion":3,"patchTotal":23},"messages":[{"id":"339039","messageId":"01020161890f432e-07d6260a-0100-4859-af1c-2cdf8da76829-000000@eu-west-1.amazonses.com","threadId":"47816","inReplyTo":"01020161890f4236-47989eb4-c19f-4282-9084-9d4f90c2ebeb-000000@eu-west-1.amazonses.com","subject":"[PATCH v3 03/23] cat-file: reuse struct ref_format","fromName":"Olga Telezhnaya","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-02-12T08:08:54Z","receivedAt":"2018-02-12T08:09:04Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"Start using ref_format struct instead of simple char*.\nNeed that for further reusing of formatting logic from ref-filter.\n\nSigned-off-by: Olga Telezhnaia \nMentored-by: Christian Couder \nMentored by: Jeff King \n---\n builtin/cat-file.c | 15 ++++++++-------\n 1 file changed, 8 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex f5fa4fd75af26..98fc5ec069a49 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -13,15 +13,16 @@\n #include \"tree-walk.h\"\n #include \"sha1-array.h\"\n #include \"packfile.h\"\n+#include \"ref-filter.h\"\n \n struct batch_options {\n+\tstruct ref_format format;\n \tint enabled;\n \tint follow_symlinks;\n \tint print_contents;\n \tint buffer_output;\n \tint all_objects;\n \tint cmdmode; /* may be 'w' or 'c' for --filters or --textconv */\n-\tconst char *format;\n };\n \n static const char *force_path;\n@@ -348,7 +349,7 @@ static void batch_object_write(const char *obj_name, struct batch_options *opt,\n \t\treturn;\n \t}\n \n-\tstrbuf_expand(&buf, opt->format, expand_format, data);\n+\tstrbuf_expand(&buf, opt->format.format, expand_format, data);\n \tstrbuf_addch(&buf, '\\n');\n \tbatch_write(opt, buf.buf, buf.len);\n \tstrbuf_release(&buf);\n@@ -441,8 +442,8 @@ static int batch_objects(struct batch_options *opt)\n \tint save_warning;\n \tint retval = 0;\n \n-\tif (!opt->format)\n-\t\topt->format = \"%(objectname) %(objecttype) %(objectsize)\";\n+\tif (!opt->format.format)\n+\t\topt->format.format = \"%(objectname) %(objecttype) %(objectsize)\";\n \n \t/*\n \t * Expand once with our special mark_query flag, which will prime the\n@@ -451,7 +452,7 @@ static int batch_objects(struct batch_options *opt)\n \t */\n \tmemset(&data, 0, sizeof(data));\n \tdata.mark_query = 1;\n-\tstrbuf_expand(&buf, opt->format, expand_format, &data);\n+\tstrbuf_expand(&buf, opt->format.format, expand_format, &data);\n \tdata.mark_query = 0;\n \tif (opt->cmdmode)\n \t\tdata.split_on_whitespace = 1;\n@@ -543,7 +544,7 @@ static int batch_option_callback(const struct option *opt,\n \n \tbo->enabled = 1;\n \tbo->print_contents = !strcmp(opt->long_name, \"batch\");\n-\tbo->format = arg;\n+\tbo->format.format = arg;\n \n \treturn 0;\n }\n@@ -552,7 +553,7 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)\n {\n \tint opt = 0;\n \tconst char *exp_type = NULL, *obj_name = NULL;\n-\tstruct batch_options batch = {0};\n+\tstruct batch_options batch = { REF_FORMAT_INIT };\n \tint unknown_type = 0;\n \n \tconst struct option options[] = {\n\n--\nhttps://github.com/git/git/pull/452\n"},{"id":"339040","messageId":"01020161890f434b-3f93f5b7-0fad-4777-ab9b-79c48b2b41d2-000000@eu-west-1.amazonses.com","threadId":"47816","inReplyTo":"01020161890f4236-47989eb4-c19f-4282-9084-9d4f90c2ebeb-000000@eu-west-1.amazonses.com","subject":"[PATCH v3 08/23] ref-filter: reuse parse_ref_filter_atom()","fromName":"Olga Telezhnaya","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-02-12T08:08:54Z","receivedAt":"2018-02-12T08:09:09Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"Continue migrating formatting logic from cat-file to ref-filter.\nReuse parse_ref_filter_atom() for unifying all processes in ref-filter\nand further removing of mark_atom_in_object_info().\n\nSigned-off-by: Olga Telezhnaia \nMentored-by: Christian Couder \nMentored by: Jeff King \n---\n ref-filter.c | 34 +++++++++++++++++++++++++---------\n 1 file changed, 25 insertions(+), 9 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex ff87e632f463c..5c75259b1ab8c 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -100,6 +100,7 @@ static struct used_atom {\n \t} u;\n } *used_atom;\n static int used_atom_cnt, need_tagged, need_symref;\n+struct expand_data *cat_file_info;\n \n static void color_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *color_value)\n {\n@@ -251,6 +252,16 @@ static void objectname_atom_parser(const struct ref_format *format, struct used_\n \t\tdie(_(\"unrecognized %%(objectname) argument: %s\"), arg);\n }\n \n+static void objectsize_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)\n+{\n+\tif (!arg)\n+\t\t; /* default to normal object size */\n+\telse if (!strcmp(arg, \"disk\"))\n+\t\tcat_file_info->info.disk_sizep = &cat_file_info->disk_size;\n+\telse\n+\t\tdie(_(\"urecognized %%(objectsize) argument: %s\"), arg);\n+}\n+\n static void refname_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)\n {\n \trefname_atom_parser_internal(&atom->u.refname, arg, atom->name);\n@@ -371,6 +382,14 @@ static struct valid_atom {\n \t{ \"else\" },\n };\n \n+static struct valid_atom valid_cat_file_atom[] = {\n+\t{ \"objectname\" },\n+\t{ \"objecttype\" },\n+\t{ \"objectsize\", FIELD_ULONG, objectsize_atom_parser },\n+\t{ \"rest\" },\n+\t{ \"deltabase\" },\n+};\n+\n #define REF_FORMATTING_STATE_INIT { 0, NULL }\n \n struct ref_formatting_stack {\n@@ -401,20 +420,14 @@ static int is_atom(const char *atom, const char *s, int slen)\n static void mark_atom_in_object_info(const char *atom, int len,\n \t\t\t\t struct expand_data *data)\n {\n-\tif (is_atom(\"objectname\", atom, len))\n-\t\t; /* do nothing */\n-\telse if (is_atom(\"objecttype\", atom, len))\n+\tif (is_atom(\"objecttype\", atom, len))\n \t\tdata->info.typep = &data->type;\n \telse if (is_atom(\"objectsize\", atom, len))\n \t\tdata->info.sizep = &data->size;\n-\telse if (is_atom(\"objectsize:disk\", atom, len))\n-\t\tdata->info.disk_sizep = &data->disk_size;\n \telse if (is_atom(\"rest\", atom, len))\n \t\tdata->split_on_whitespace = 1;\n \telse if (is_atom(\"deltabase\", atom, len))\n \t\tdata->info.delta_base_sha1 = data->delta_base_oid.hash;\n-\telse\n-\t\tdie(\"unknown format element: %.*s\", len, atom);\n }\n \n /*\n@@ -483,6 +496,8 @@ static int parse_ref_filter_atom(const struct ref_format *format,\n \t\tneed_tagged = 1;\n \tif (!strcmp(valid_atom[i].name, \"symref\"))\n \t\tneed_symref = 1;\n+\tif (cat_file_info)\n+\t\tmark_atom_in_object_info(atom, atom_len, cat_file_info);\n \treturn at;\n }\n \n@@ -726,6 +741,7 @@ int verify_ref_format(struct ref_format *format)\n {\n \tconst char *cp, *sp;\n \n+\tcat_file_info = format->cat_file_data;\n \tformat->need_color_reset_at_eol = 0;\n \tfor (cp = format->format; *cp && (sp = find_next(cp)); ) {\n \t\tconst char *color, *ep = strchr(sp, ')');\n@@ -736,8 +752,8 @@ int verify_ref_format(struct ref_format *format)\n \t\t/* sp points at \"%(\" and ep points at the closing \")\" */\n \n \t\tif (format->cat_file_data)\n-\t\t\tmark_atom_in_object_info(sp + 2, ep - sp - 2,\n-\t\t\t\t\t\tformat->cat_file_data);\n+\t\t\tat = parse_ref_filter_atom(format, valid_cat_file_atom,\n+\t\t\t\t\t\t ARRAY_SIZE(valid_cat_file_atom), sp + 2, ep);\n \t\telse {\n \t\t\tat = parse_ref_filter_atom(format, valid_atom,\n \t\t\t\t\t\t ARRAY_SIZE(valid_atom), sp + 2, ep);\n\n--\nhttps://github.com/git/git/pull/452\n"},{"id":"339041","messageId":"01020161890f4236-47989eb4-c19f-4282-9084-9d4f90c2ebeb-000000@eu-west-1.amazonses.com","threadId":"47816","inReplyTo":"0102016165b8b3c4-54efe4c4-6d19-435d-b5b9-6c727771353b-000000@eu-west-1.amazonses.com","subject":"[PATCH v3 01/23] ref-filter: get rid of goto","fromName":"Olga Telezhnaya","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-02-12T08:08:54Z","receivedAt":"2018-02-12T08:09:11Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"Get rid of goto command in ref-filter for better readability.\n\nSigned-off-by: Olga Telezhnaia \nMentored-by: Christian Couder \nMentored by: Jeff King \n---\n ref-filter.c | 9 +++++----\n 1 file changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex f9e25aea7a97e..d04295e33448e 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1477,12 +1477,13 @@ static void populate_value(struct ref_array_item *ref)\n \n \tfor (i = 0; i < used_atom_cnt; i++) {\n \t\tstruct atom_value *v = &ref->value[i];\n-\t\tif (v->s == NULL)\n-\t\t\tgoto need_obj;\n+\t\tif (v->s == NULL) {\n+\t\t\tbreak;\n+\t\t}\n \t}\n-\treturn;\n+\tif (used_atom_cnt <= i)\n+\t\treturn;\n \n- need_obj:\n \tbuf = get_obj(&ref->objectname, &obj, &size, &eaten);\n \tif (!buf)\n \t\tdie(_(\"missing object %s for %s\"),\n\n--\nhttps://github.com/git/git/pull/452\n"},{"id":"339042","messageId":"01020161890f4324-be5dda9b-9ad7-402f-9196-30f982fd20c2-000000@eu-west-1.amazonses.com","threadId":"47816","inReplyTo":"01020161890f4236-47989eb4-c19f-4282-9084-9d4f90c2ebeb-000000@eu-west-1.amazonses.com","subject":"[PATCH v3 04/23] ref-filter: make valid_atom as function parameter","fromName":"Olga Telezhnaya","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-02-12T08:08:54Z","receivedAt":"2018-02-12T08:09:12Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"Make valid_atom as a function parameter,\nthere could be another variable further.\nNeed that for further reusing of formatting logic in cat-file.c.\n\nWe do not need to allow users to pass their own valid_atom variable in\nglobal functions like verify_ref_format() because in the end we want to\nhave same set of valid atoms for all commands. But, as a first step\nof migrating, I create further another version of valid_atom\nfor cat-file.\n\nSigned-off-by: Olga Telezhnaia \nMentored-by: Christian Couder \nMentored by: Jeff King \n---\n ref-filter.c | 17 +++++++++++------\n 1 file changed, 11 insertions(+), 6 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 9ed5e66066a7a..5e7ed0f338490 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -325,7 +325,7 @@ static void head_atom_parser(const struct ref_format *format, struct used_atom *\n \tatom->u.head = resolve_refdup(\"HEAD\", RESOLVE_REF_READING, NULL, NULL);\n }\n \n-static struct {\n+static struct valid_atom {\n \tconst char *name;\n \tcmp_type cmp_type;\n \tvoid (*parser)(const struct ref_format *format, struct used_atom *atom, const char *arg);\n@@ -396,6 +396,7 @@ struct atom_value {\n * Used to parse format string and sort specifiers\n */\n static int parse_ref_filter_atom(const struct ref_format *format,\n+\t\t\t\t const struct valid_atom *valid_atom, int n_atoms,\n \t\t\t\t const char *atom, const char *ep)\n {\n \tconst char *sp;\n@@ -425,13 +426,13 @@ static int parse_ref_filter_atom(const struct ref_format *format,\n \tatom_len = (arg ? arg : ep) - sp;\n \n \t/* Is the atom a valid one? */\n-\tfor (i = 0; i < ARRAY_SIZE(valid_atom); i++) {\n+\tfor (i = 0; i < n_atoms; i++) {\n \t\tint len = strlen(valid_atom[i].name);\n \t\tif (len == atom_len && !memcmp(valid_atom[i].name, sp, len))\n \t\t\tbreak;\n \t}\n \n-\tif (ARRAY_SIZE(valid_atom) <= i)\n+\tif (n_atoms <= i)\n \t\tdie(_(\"unknown field name: %.*s\"), (int)(ep-atom), atom);\n \n \t/* Add it in, including the deref prefix */\n@@ -708,7 +709,8 @@ int verify_ref_format(struct ref_format *format)\n \t\tif (!ep)\n \t\t\treturn error(_(\"malformed format string %s\"), sp);\n \t\t/* sp points at \"%(\" and ep points at the closing \")\" */\n-\t\tat = parse_ref_filter_atom(format, sp + 2, ep);\n+\t\tat = parse_ref_filter_atom(format, valid_atom,\n+\t\t\t\t\t ARRAY_SIZE(valid_atom), sp + 2, ep);\n \t\tcp = ep + 1;\n \n \t\tif (skip_prefix(used_atom[at].name, \"color:\", &color))\n@@ -2145,7 +2147,9 @@ int format_ref_array_item(struct ref_array_item *info,\n \t\tif (cp < sp)\n \t\t\tappend_literal(cp, sp, &state);\n \t\tif (get_ref_atom_value(info,\n-\t\t\t\t parse_ref_filter_atom(format, sp + 2, ep),\n+\t\t\t\t parse_ref_filter_atom(format, valid_atom,\n+\t\t\t\t\t\t\t ARRAY_SIZE(valid_atom),\n+\t\t\t\t\t\t\t sp + 2, ep),\n \t\t\t\t &atomv))\n \t\t\treturn -1;\n \t\tatomv->handler(atomv, &state);\n@@ -2198,7 +2202,8 @@ static int parse_sorting_atom(const char *atom)\n \t */\n \tstruct ref_format dummy = REF_FORMAT_INIT;\n \tconst char *end = atom + strlen(atom);\n-\treturn parse_ref_filter_atom(&dummy, atom, end);\n+\treturn parse_ref_filter_atom(&dummy, valid_atom,\n+\t\t\t\t ARRAY_SIZE(valid_atom), atom, end);\n }\n \n /* If no sorting option is given, use refname to sort as default */\n\n--\nhttps://github.com/git/git/pull/452\n"},{"id":"339043","messageId":"01020161890f4354-a54075a4-9dba-45df-9e5c-196362b4e300-000000@eu-west-1.amazonses.com","threadId":"47816","inReplyTo":"01020161890f4236-47989eb4-c19f-4282-9084-9d4f90c2ebeb-000000@eu-west-1.amazonses.com","subject":"[PATCH v3 11/23] ref-filter: rename field in ref_array_item stuct","fromName":"Olga Telezhnaya","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-02-12T08:08:54Z","receivedAt":"2018-02-12T08:09:18Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"Rename objectname field to oid in struct ref_array_item.\nNext commit will add objectname field that will contain\nstring representation of object id.\n\nSigned-off-by: Olga Telezhnaia \nMentored-by: Christian Couder \nMentored by: Jeff King \n---\n builtin/cat-file.c | 4 ++--\n ref-filter.c | 10 +++++-----\n ref-filter.h | 2 +-\n 3 files changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 5b7bc34f1ec6d..0c362828ad81e 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -186,7 +186,7 @@ static void expand_atom(struct strbuf *sb, const char *atom, int len,\n \t\t\t struct ref_array_item *item)\n {\n \tif (is_atom(\"objectname\", atom, len))\n-\t\tstrbuf_addstr(sb, oid_to_hex(&item->objectname));\n+\t\tstrbuf_addstr(sb, oid_to_hex(&item->oid));\n \telse if (is_atom(\"objecttype\", atom, len))\n \t\tstrbuf_addstr(sb, typename(item->type));\n \telse if (is_atom(\"objectsize\", atom, len))\n@@ -294,7 +294,7 @@ static void batch_object_write(const char *obj_name, struct batch_options *opt,\n \t\treturn;\n \t}\n \n-\titem.objectname = data->oid;\n+\titem.oid = data->oid;\n \titem.type = data->type;\n \titem.size = data->size;\n \titem.disk_size = data->disk_size;\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 4acd391b5dfac..d09ec1bde6d54 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1489,7 +1489,7 @@ int populate_value(struct ref_array_item *ref)\n \t\t\t\tv->s = xstrdup(buf + 1);\n \t\t\t}\n \t\t\tcontinue;\n-\t\t} else if (!deref && grab_objectname(name, ref->objectname.hash, v, atom)) {\n+\t\t} else if (!deref && grab_objectname(name, ref->oid.hash, v, atom)) {\n \t\t\tcontinue;\n \t\t} else if (!strcmp(name, \"HEAD\")) {\n \t\t\tif (atom->u.head && !strcmp(ref->refname, atom->u.head))\n@@ -1534,13 +1534,13 @@ int populate_value(struct ref_array_item *ref)\n \tif (used_atom_cnt <= i)\n \t\treturn 0;\n \n-\tbuf = get_obj(&ref->objectname, &obj, &size, &eaten);\n+\tbuf = get_obj(&ref->oid, &obj, &size, &eaten);\n \tif (!buf)\n \t\tdie(_(\"missing object %s for %s\"),\n-\t\t oid_to_hex(&ref->objectname), ref->refname);\n+\t\t oid_to_hex(&ref->oid), ref->refname);\n \tif (!obj)\n \t\tdie(_(\"parse_object_buffer failed on %s for %s\"),\n-\t\t oid_to_hex(&ref->objectname), ref->refname);\n+\t\t oid_to_hex(&ref->oid), ref->refname);\n \n \tgrab_values(ref->value, 0, obj, buf, size);\n \tif (!eaten)\n@@ -1890,7 +1890,7 @@ static struct ref_array_item *new_ref_array_item(const char *refname,\n {\n \tstruct ref_array_item *ref;\n \tFLEX_ALLOC_STR(ref, refname, refname);\n-\thashcpy(ref->objectname.hash, objectname);\n+\thashcpy(ref->oid.hash, objectname);\n \tref->flag = flag;\n \n \treturn ref;\ndiff --git a/ref-filter.h b/ref-filter.h\nindex e16ea2a990119..87b026b8b76d0 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -34,7 +34,7 @@ struct ref_sorting {\n };\n \n struct ref_array_item {\n-\tstruct object_id objectname;\n+\tstruct object_id oid;\n \tint flag;\n \tunsigned int kind;\n \tconst char *symref;\n\n--\nhttps://github.com/git/git/pull/452\n"},{"id":"339044","messageId":"01020161890f4348-10a54126-e31d-448d-93a3-55fd89c248c4-000000@eu-west-1.amazonses.com","threadId":"47816","inReplyTo":"01020161890f4236-47989eb4-c19f-4282-9084-9d4f90c2ebeb-000000@eu-west-1.amazonses.com","subject":"[PATCH v3 07/23] cat-file: start migrating formatting to ref-filter","fromName":"Olga Telezhnaya","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-02-12T08:08:54Z","receivedAt":"2018-02-12T08:09:20Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"Move mark_atom_in_object_info() from cat-file to ref-filter and\nstart using it in verify_ref_format().\nIt also means that we start reusing verify_ref_format() in cat-file.\n\nStart from simple moving of mark_atom_in_object_info(),\nit would be removed later by integrating all needed processes into\nref-filter logic.\n\nSigned-off-by: Olga Telezhnaia \nMentored-by: Christian Couder \nMentored by: Jeff King \n---\n builtin/cat-file.c | 35 +++++------------------------------\n ref-filter.c | 41 ++++++++++++++++++++++++++++++++++++-----\n ref-filter.h | 12 ++++++------\n 3 files changed, 47 insertions(+), 41 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex edb04a96d9bd3..67e7790d2f319 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -182,25 +182,6 @@ static int is_atom(const char *atom, const char *s, int slen)\n \treturn alen == slen && !memcmp(atom, s, alen);\n }\n \n-static void mark_atom_in_object_info(const char *atom, int len,\n-\t\t\t\t struct expand_data *data)\n-{\n-\tif (is_atom(\"objectname\", atom, len))\n-\t\t; /* do nothing */\n-\telse if (is_atom(\"objecttype\", atom, len))\n-\t\tdata->info.typep = &data->type;\n-\telse if (is_atom(\"objectsize\", atom, len))\n-\t\tdata->info.sizep = &data->size;\n-\telse if (is_atom(\"objectsize:disk\", atom, len))\n-\t\tdata->info.disk_sizep = &data->disk_size;\n-\telse if (is_atom(\"rest\", atom, len))\n-\t\tdata->split_on_whitespace = 1;\n-\telse if (is_atom(\"deltabase\", atom, len))\n-\t\tdata->info.delta_base_sha1 = data->delta_base_oid.hash;\n-\telse\n-\t\tdie(\"unknown format element: %.*s\", len, atom);\n-}\n-\n static void expand_atom(struct strbuf *sb, const char *atom, int len,\n \t\t\t struct expand_data *data)\n {\n@@ -230,11 +211,7 @@ static size_t expand_format(struct strbuf *sb, const char *start, void *vdata)\n \tif (!end)\n \t\tdie(\"format element '%s' does not end in ')'\", start);\n \n-\tif (data->mark_query)\n-\t\tmark_atom_in_object_info(start + 1, end - start - 1, data);\n-\telse\n-\t\texpand_atom(sb, start + 1, end - start - 1, data);\n-\n+\texpand_atom(sb, start + 1, end - start - 1, data);\n \treturn end - start + 1;\n }\n \n@@ -413,14 +390,12 @@ static int batch_objects(struct batch_options *opt)\n \t\topt->format.format = \"%(objectname) %(objecttype) %(objectsize)\";\n \n \t/*\n-\t * Expand once with our special mark_query flag, which will prime the\n-\t * object_info to be handed to sha1_object_info_extended for each\n-\t * object.\n+\t * Call verify_ref_format to prepare object_info to be handed to\n+\t * sha1_object_info_extended for each object.\n \t */\n \tmemset(&data, 0, sizeof(data));\n-\tdata.mark_query = 1;\n-\tstrbuf_expand(&buf, opt->format.format, expand_format, &data);\n-\tdata.mark_query = 0;\n+\topt->format.cat_file_data = &data;\n+\tverify_ref_format(&opt->format);\n \tif (opt->cmdmode)\n \t\tdata.split_on_whitespace = 1;\n \ndiff --git a/ref-filter.c b/ref-filter.c\nindex 5e7ed0f338490..ff87e632f463c 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -392,6 +392,31 @@ struct atom_value {\n \tstruct used_atom *atom;\n };\n \n+static int is_atom(const char *atom, const char *s, int slen)\n+{\n+\tint alen = strlen(atom);\n+\treturn alen == slen && !memcmp(atom, s, alen);\n+}\n+\n+static void mark_atom_in_object_info(const char *atom, int len,\n+\t\t\t\t struct expand_data *data)\n+{\n+\tif (is_atom(\"objectname\", atom, len))\n+\t\t; /* do nothing */\n+\telse if (is_atom(\"objecttype\", atom, len))\n+\t\tdata->info.typep = &data->type;\n+\telse if (is_atom(\"objectsize\", atom, len))\n+\t\tdata->info.sizep = &data->size;\n+\telse if (is_atom(\"objectsize:disk\", atom, len))\n+\t\tdata->info.disk_sizep = &data->disk_size;\n+\telse if (is_atom(\"rest\", atom, len))\n+\t\tdata->split_on_whitespace = 1;\n+\telse if (is_atom(\"deltabase\", atom, len))\n+\t\tdata->info.delta_base_sha1 = data->delta_base_oid.hash;\n+\telse\n+\t\tdie(\"unknown format element: %.*s\", len, atom);\n+}\n+\n /*\n * Used to parse format string and sort specifiers\n */\n@@ -709,12 +734,18 @@ int verify_ref_format(struct ref_format *format)\n \t\tif (!ep)\n \t\t\treturn error(_(\"malformed format string %s\"), sp);\n \t\t/* sp points at \"%(\" and ep points at the closing \")\" */\n-\t\tat = parse_ref_filter_atom(format, valid_atom,\n-\t\t\t\t\t ARRAY_SIZE(valid_atom), sp + 2, ep);\n-\t\tcp = ep + 1;\n \n-\t\tif (skip_prefix(used_atom[at].name, \"color:\", &color))\n-\t\t\tformat->need_color_reset_at_eol = !!strcmp(color, \"reset\");\n+\t\tif (format->cat_file_data)\n+\t\t\tmark_atom_in_object_info(sp + 2, ep - sp - 2,\n+\t\t\t\t\t\tformat->cat_file_data);\n+\t\telse {\n+\t\t\tat = parse_ref_filter_atom(format, valid_atom,\n+\t\t\t\t\t\t ARRAY_SIZE(valid_atom), sp + 2, ep);\n+\t\t\tif (skip_prefix(used_atom[at].name, \"color:\", &color))\n+\t\t\t\tformat->need_color_reset_at_eol = !!strcmp(color, \"reset\");\n+\t\t}\n+\n+\t\tcp = ep + 1;\n \t}\n \tif (format->need_color_reset_at_eol && !want_color(format->use_color))\n \t\tformat->need_color_reset_at_eol = 0;\ndiff --git a/ref-filter.h b/ref-filter.h\nindex 17f2ac24d2739..52e07dbe6864a 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -80,12 +80,6 @@ struct expand_data {\n \tconst char *rest;\n \tstruct object_id delta_base_oid;\n \n-\t/*\n-\t * If mark_query is true, we do not expand anything, but rather\n-\t * just mark the object_info with items we wish to query.\n-\t */\n-\tint mark_query;\n-\n \t/*\n \t * Whether to split the input on whitespace before feeding it to\n \t * get_sha1; this is decided during the mark_query phase based on\n@@ -119,6 +113,12 @@ struct ref_format {\n \n \t/* Internal state to ref-filter */\n \tint need_color_reset_at_eol;\n+\n+\t/*\n+\t * Helps to move all formatting logic from cat-file to ref-filter,\n+\t * hopefully would be reduced later.\n+\t */\n+\tstruct expand_data *cat_file_data;\n };\n \n #define REF_FORMAT_INIT { NULL, 0, -1 }\n\n--\nhttps://github.com/git/git/pull/452\n"},{"id":"339045","messageId":"01020161890f4370-4ae9a8d6-4f01-4f56-9fde-c40fd76d69b5-000000@eu-west-1.amazonses.com","threadId":"47816","inReplyTo":"01020161890f4236-47989eb4-c19f-4282-9084-9d4f90c2ebeb-000000@eu-west-1.amazonses.com","subject":"[PATCH v3 19/23] ref-filter: make populate_value() internal again","fromName":"Olga Telezhnaya","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-02-12T08:08:54Z","receivedAt":"2018-02-12T08:09:24Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"Remove populate_value() from header file. We needed that\nfor interim step, now it could be returned back.\n\nSigned-off-by: Olga Telezhnaia \nMentored-by: Christian Couder \nMentored by: Jeff King \n---\n ref-filter.c | 2 +-\n ref-filter.h | 3 ---\n 2 files changed, 1 insertion(+), 4 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex eb53b0babdb83..8d104b567eb7c 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1428,7 +1428,7 @@ static int check_and_fill_for_cat(struct ref_array_item *ref)\n * Parse the object referred by ref, and grab needed value.\n * Return 0 if everything was successful, -1 otherwise.\n */\n-int populate_value(struct ref_array_item *ref)\n+static int populate_value(struct ref_array_item *ref)\n {\n \tvoid *buf;\n \tstruct object *obj;\ndiff --git a/ref-filter.h b/ref-filter.h\nindex 4eaf6d0514502..115d00288fdee 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -174,9 +174,6 @@ void setup_ref_filter_porcelain_msg(void);\n void pretty_print_ref(const char *name, const unsigned char *sha1,\n \t\t const struct ref_format *format);\n \n-/* Fill the values of request and prepare all data for final string creation */\n-int populate_value(struct ref_array_item *ref);\n-\n /* Search for atom in given format. */\n int is_atom_used(const struct ref_format *format, const char *atom);\n \n\n--\nhttps://github.com/git/git/pull/452\n"},{"id":"339046","messageId":"01020161890f4376-0d1c5615-ae9c-4320-bc2d-fd3bda19bce0-000000@eu-west-1.amazonses.com","threadId":"47816","inReplyTo":"01020161890f4236-47989eb4-c19f-4282-9084-9d4f90c2ebeb-000000@eu-west-1.amazonses.com","subject":"[PATCH v3 16/23] ref-filter: make cat_file_info independent","fromName":"Olga Telezhnaya","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-02-12T08:08:54Z","receivedAt":"2018-02-12T08:09:39Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"Remove connection between expand_data variable\nin cat-file and in ref-filter.\nIt will help further to get rid of using expand_data in cat-file.\n\nSigned-off-by: Olga Telezhnaia \nMentored-by: Christian Couder \nMentored by: Jeff King \n---\n builtin/cat-file.c | 3 ++-\n ref-filter.c | 36 +++++++++++++++++++-----------------\n ref-filter.h | 7 ++-----\n 3 files changed, 23 insertions(+), 23 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 582679f3dca2c..273b4038e3893 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -292,6 +292,7 @@ static void batch_object_write(const char *obj_name, struct batch_options *opt,\n \tif (populate_value(&item))\n \t\treturn;\n \n+\tdata->type = item.type;\n \tstrbuf_expand(&buf, opt->format.format, expand_format, &item);\n \tstrbuf_addch(&buf, '\\n');\n \tbatch_write(opt, buf.buf, buf.len);\n@@ -392,7 +393,7 @@ static int batch_objects(struct batch_options *opt)\n \t * sha1_object_info_extended for each object.\n \t */\n \tmemset(&data, 0, sizeof(data));\n-\topt->format.cat_file_data = &data;\n+\topt->format.is_cat_file = 1;\n \topt->format.all_objects = opt->all_objects;\n \tverify_ref_format(&opt->format);\n \ndiff --git a/ref-filter.c b/ref-filter.c\nindex 104cd6aef0102..cc70bcf2bb8b1 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -100,7 +100,7 @@ static struct used_atom {\n \t} u;\n } *used_atom;\n static int used_atom_cnt, need_tagged, need_symref;\n-struct expand_data *cat_file_info;\n+struct expand_data cat_file_info;\n \n static void color_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *color_value)\n {\n@@ -255,9 +255,9 @@ static void objectname_atom_parser(const struct ref_format *format, struct used_\n static void objectsize_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)\n {\n \tif (!arg)\n-\t\tcat_file_info->info.sizep = &cat_file_info->size;\n+\t\tcat_file_info.info.sizep = &cat_file_info.size;\n \telse if (!strcmp(arg, \"disk\"))\n-\t\tcat_file_info->info.disk_sizep = &cat_file_info->disk_size;\n+\t\tcat_file_info.info.disk_sizep = &cat_file_info.disk_size;\n \telse\n \t\tdie(_(\"urecognized %%(objectsize) argument: %s\"), arg);\n }\n@@ -265,7 +265,7 @@ static void objectsize_atom_parser(const struct ref_format *format, struct used_\n static void objecttype_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)\n {\n \tif (!arg)\n-\t\tcat_file_info->info.typep = &cat_file_info->type;\n+\t\tcat_file_info.info.typep = &cat_file_info.type;\n \telse\n \t\tdie(_(\"urecognized %%(objecttype) argument: %s\"), arg);\n }\n@@ -273,7 +273,7 @@ static void objecttype_atom_parser(const struct ref_format *format, struct used_\n static void deltabase_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)\n {\n \tif (!arg)\n-\t\tcat_file_info->info.delta_base_sha1 = cat_file_info->delta_base_oid.hash;\n+\t\tcat_file_info.info.delta_base_sha1 = cat_file_info.delta_base_oid.hash;\n \telse\n \t\tdie(_(\"urecognized %%(deltabase) argument: %s\"), arg);\n }\n@@ -751,7 +751,7 @@ int verify_ref_format(struct ref_format *format)\n {\n \tconst char *cp, *sp;\n \n-\tcat_file_info = format->cat_file_data;\n+\tcat_file_info.is_cat_file = format->is_cat_file;\n \tformat->need_color_reset_at_eol = 0;\n \tfor (cp = format->format; *cp && (sp = find_next(cp)); ) {\n \t\tconst char *color, *ep = strchr(sp, ')');\n@@ -761,7 +761,7 @@ int verify_ref_format(struct ref_format *format)\n \t\t\treturn error(_(\"malformed format string %s\"), sp);\n \t\t/* sp points at \"%(\" and ep points at the closing \")\" */\n \n-\t\tif (format->cat_file_data)\n+\t\tif (cat_file_info.is_cat_file)\n \t\t\tat = parse_ref_filter_atom(format, valid_cat_file_atom,\n \t\t\t\t\t\t ARRAY_SIZE(valid_cat_file_atom), sp + 2, ep);\n \t\telse {\n@@ -775,10 +775,10 @@ int verify_ref_format(struct ref_format *format)\n \t}\n \tif (format->need_color_reset_at_eol && !want_color(format->use_color))\n \t\tformat->need_color_reset_at_eol = 0;\n-\tif (cat_file_info && format->all_objects) {\n+\tif (cat_file_info.is_cat_file && format->all_objects) {\n \t\tstruct object_info empty = OBJECT_INFO_INIT;\n-\t\tif (!memcmp(&cat_file_info->info, &empty, sizeof(empty)))\n-\t\t\tcat_file_info->skip_object_info = 1;\n+\t\tif (!memcmp(&cat_file_info.info, &empty, sizeof(empty)))\n+\t\t\tcat_file_info.skip_object_info = 1;\n \t}\n \treturn 0;\n }\n@@ -1420,18 +1420,20 @@ static const char *get_refname(struct used_atom *atom, struct ref_array_item *re\n \n static int check_and_fill_for_cat(struct ref_array_item *ref)\n {\n-\tif (!cat_file_info->skip_object_info &&\n-\t sha1_object_info_extended(ref->oid.hash, &cat_file_info->info,\n+\tif (!cat_file_info.info.typep)\n+\t\tcat_file_info.info.typep = &cat_file_info.type;\n+\tif (!cat_file_info.skip_object_info &&\n+\t sha1_object_info_extended(ref->oid.hash, &cat_file_info.info,\n \t\t\t\t OBJECT_INFO_LOOKUP_REPLACE) < 0) {\n \t\tconst char *e = ref->objectname;\n \t\tprintf(\"%s missing\\n\", e ? e : oid_to_hex(&ref->oid));\n \t\tfflush(stdout);\n \t\treturn -1;\n \t}\n-\tref->type = cat_file_info->type;\n-\tref->size = cat_file_info->size;\n-\tref->disk_size = cat_file_info->disk_size;\n-\tref->delta_base_oid = &cat_file_info->delta_base_oid;\n+\tref->type = cat_file_info.type;\n+\tref->size = cat_file_info.size;\n+\tref->disk_size = cat_file_info.disk_size;\n+\tref->delta_base_oid = &cat_file_info.delta_base_oid;\n \treturn 0;\n }\n \n@@ -1456,7 +1458,7 @@ int populate_value(struct ref_array_item *ref)\n \t\t\tref->symref = \"\";\n \t}\n \n-\tif (cat_file_info && check_and_fill_for_cat(ref))\n+\tif (cat_file_info.is_cat_file && check_and_fill_for_cat(ref))\n \t\treturn -1;\n \n \t/* Fill in specials first */\ndiff --git a/ref-filter.h b/ref-filter.h\nindex b1c668c12428b..4eaf6d0514502 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -99,6 +99,7 @@ struct expand_data {\n \t * optimized out.\n \t */\n \tunsigned skip_object_info : 1;\n+\tunsigned is_cat_file : 1;\n };\n \n struct ref_format {\n@@ -113,12 +114,8 @@ struct ref_format {\n \t/* Internal state to ref-filter */\n \tint need_color_reset_at_eol;\n \n-\t/*\n-\t * Helps to move all formatting logic from cat-file to ref-filter,\n-\t * hopefully would be reduced later.\n-\t */\n-\tstruct expand_data *cat_file_data;\n \tunsigned all_objects : 1;\n+\tunsigned is_cat_file : 1;\n };\n \n #define REF_FORMAT_INIT { NULL, 0, -1 }\n\n--\nhttps://github.com/git/git/pull/452\n"},{"id":"339047","messageId":"01020161890f437a-302b1577-f034-4a24-a3a2-851d1cda0044-000000@eu-west-1.amazonses.com","threadId":"47816","inReplyTo":"01020161890f4236-47989eb4-c19f-4282-9084-9d4f90c2ebeb-000000@eu-west-1.amazonses.com","subject":"[PATCH v3 20/23] ref-filter: unifying formatting of cat-file opts","fromName":"Olga Telezhnaya","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-02-12T08:08:54Z","receivedAt":"2018-02-12T08:09:45Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"cat-file options are now filled by general logic.\n\nSigned-off-by: Olga Telezhnaia \nMentored-by: Christian Couder \nMentored by: Jeff King \n---\n ref-filter.c | 33 ++++++++++++++++-----------------\n 1 file changed, 16 insertions(+), 17 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 8d104b567eb7c..5781416cf9126 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -824,8 +824,12 @@ static void grab_common_values(struct atom_value *val, int deref, struct object\n \t\telse if (!strcmp(name, \"objectsize\")) {\n \t\t\tv->value = sz;\n \t\t\tv->s = xstrfmt(\"%lu\", sz);\n-\t\t}\n-\t\telse if (deref)\n+\t\t} else if (!strcmp(name, \"objectsize:disk\")) {\n+\t\t\tif (cat_file_info.is_cat_file) {\n+\t\t\t\tv->value = cat_file_info.disk_size;\n+\t\t\t\tv->s = xstrfmt(\"%\"PRIuMAX, (uintmax_t)v->value);\n+\t\t\t}\n+\t\t} else if (deref)\n \t\t\tgrab_objectname(name, obj->oid.hash, v, &used_atom[i]);\n \t}\n }\n@@ -1465,21 +1469,7 @@ static int populate_value(struct ref_array_item *ref)\n \t\t\tname++;\n \t\t}\n \n-\t\tif (cat_file_info.is_cat_file) {\n-\t\t\tif (starts_with(name, \"objectname\"))\n-\t\t\t\tv->s = oid_to_hex(&ref->oid);\n-\t\t\telse if (starts_with(name, \"objecttype\"))\n-\t\t\t\tv->s = typename(ref->type);\n-\t\t\telse if (starts_with(name, \"objectsize\")) {\n-\t\t\t\tv->s = xstrfmt(\"%lu\", ref->size);\n-\t\t\t} else if (starts_with(name, \"objectsize:disk\")) {\n-\t\t\t\tv->s = xstrfmt(\"%\"PRIuMAX, (uintmax_t)ref->disk_size);\n-\t\t\t} else if (starts_with(name, \"rest\"))\n-\t\t\t\tv->s = ref->rest;\n-\t\t\telse if (starts_with(name, \"deltabase\"))\n-\t\t\t\tv->s = xstrdup(oid_to_hex(ref->delta_base_oid));\n-\t\t\tcontinue;\n-\t\t} else if (starts_with(name, \"refname\"))\n+\t\tif (starts_with(name, \"refname\"))\n \t\t\trefname = get_refname(atom, ref);\n \t\telse if (starts_with(name, \"symref\"))\n \t\t\trefname = get_symref(atom, ref);\n@@ -1535,6 +1525,15 @@ static int populate_value(struct ref_array_item *ref)\n \t\t\telse\n \t\t\t\tv->s = \" \";\n \t\t\tcontinue;\n+\t\t} else if (starts_with(name, \"rest\")) {\n+\t\t\tv->s = ref->rest ? ref->rest : \"\";\n+\t\t\tcontinue;\n+\t\t} else if (starts_with(name, \"deltabase\")) {\n+\t\t\tif (ref->delta_base_oid)\n+\t\t\t\tv->s = xstrdup(oid_to_hex(ref->delta_base_oid));\n+\t\t\telse\n+\t\t\t\tv->s = \"\";\n+\t\t\tcontinue;\n \t\t} else if (starts_with(name, \"align\")) {\n \t\t\tv->handler = align_atom_handler;\n \t\t\tcontinue;\n\n--\nhttps://github.com/git/git/pull/452\n"},{"id":"339048","messageId":"01020161890f4356-2cfaddc9-cd7d-4bd7-9da4-df08783ed06b-000000@eu-west-1.amazonses.com","threadId":"47816","inReplyTo":"01020161890f4236-47989eb4-c19f-4282-9084-9d4f90c2ebeb-000000@eu-west-1.amazonses.com","subject":"[PATCH v3 10/23] ref-filter: make populate_value() global","fromName":"Olga Telezhnaya","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-02-12T08:08:54Z","receivedAt":"2018-02-12T08:09:47Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"Make function global for further using in cat-file.\nIn the end of patch series this function becomes internal again,\nso this is a part of middle step. cat-file would use more general\nfunctions further.\n\nSigned-off-by: Olga Telezhnaia \nMentored-by: Christian Couder \nMentored by: Jeff King \n---\n ref-filter.c | 2 +-\n ref-filter.h | 3 +++\n 2 files changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 5c75259b1ab8c..4acd391b5dfac 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1407,7 +1407,7 @@ static const char *get_refname(struct used_atom *atom, struct ref_array_item *re\n * Parse the object referred by ref, and grab needed value.\n * Return 0 if everything was successful, -1 otherwise.\n */\n-static int populate_value(struct ref_array_item *ref)\n+int populate_value(struct ref_array_item *ref)\n {\n \tvoid *buf;\n \tstruct object *obj;\ndiff --git a/ref-filter.h b/ref-filter.h\nindex 781921d4e0978..e16ea2a990119 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -182,4 +182,7 @@ void setup_ref_filter_porcelain_msg(void);\n void pretty_print_ref(const char *name, const unsigned char *sha1,\n \t\t const struct ref_format *format);\n \n+/* Fill the values of request and prepare all data for final string creation */\n+int populate_value(struct ref_array_item *ref);\n+\n #endif /* REF_FILTER_H */\n\n--\nhttps://github.com/git/git/pull/452\n"},{"id":"339049","messageId":"01020161890f435c-80fa0ef3-afaf-45b7-84a7-28e2bb66f320-000000@eu-west-1.amazonses.com","threadId":"47816","inReplyTo":"01020161890f4236-47989eb4-c19f-4282-9084-9d4f90c2ebeb-000000@eu-west-1.amazonses.com","subject":"[PATCH v3 15/23] cat-file: move skip_object_info into ref-filter","fromName":"Olga Telezhnaya","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-02-12T08:08:54Z","receivedAt":"2018-02-12T08:09:49Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"Move logic related to skip_object_info into ref-filter,\nso that cat-file does not use that field at all.\n\nSigned-off-by: Olga Telezhnaia \nMentored-by: Christian Couder \nMentored by: Jeff King \n---\n builtin/cat-file.c | 7 +------\n ref-filter.c | 5 +++++\n ref-filter.h | 1 +\n 3 files changed, 7 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 3a49b55a1cc2e..582679f3dca2c 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -393,14 +393,9 @@ static int batch_objects(struct batch_options *opt)\n \t */\n \tmemset(&data, 0, sizeof(data));\n \topt->format.cat_file_data = &data;\n+\topt->format.all_objects = opt->all_objects;\n \tverify_ref_format(&opt->format);\n \n-\tif (opt->all_objects) {\n-\t\tstruct object_info empty = OBJECT_INFO_INIT;\n-\t\tif (!memcmp(&data.info, &empty, sizeof(empty)))\n-\t\t\tdata.skip_object_info = 1;\n-\t}\n-\n \t/*\n \t * If we are printing out the object, then always fill in the type,\n \t * since we will want to decide whether or not to stream.\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 4adeea6aad0da..104cd6aef0102 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -775,6 +775,11 @@ int verify_ref_format(struct ref_format *format)\n \t}\n \tif (format->need_color_reset_at_eol && !want_color(format->use_color))\n \t\tformat->need_color_reset_at_eol = 0;\n+\tif (cat_file_info && format->all_objects) {\n+\t\tstruct object_info empty = OBJECT_INFO_INIT;\n+\t\tif (!memcmp(&cat_file_info->info, &empty, sizeof(empty)))\n+\t\t\tcat_file_info->skip_object_info = 1;\n+\t}\n \treturn 0;\n }\n \ndiff --git a/ref-filter.h b/ref-filter.h\nindex fffc443726e28..b1c668c12428b 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -118,6 +118,7 @@ struct ref_format {\n \t * hopefully would be reduced later.\n \t */\n \tstruct expand_data *cat_file_data;\n+\tunsigned all_objects : 1;\n };\n \n #define REF_FORMAT_INIT { NULL, 0, -1 }\n\n--\nhttps://github.com/git/git/pull/452\n"},{"id":"339050","messageId":"01020161890f438c-9d30a51c-129c-4721-a85a-a3d66a2270b6-000000@eu-west-1.amazonses.com","threadId":"47816","inReplyTo":"01020161890f4236-47989eb4-c19f-4282-9084-9d4f90c2ebeb-000000@eu-west-1.amazonses.com","subject":"[PATCH v3 21/23] for-each-ref: tests for new atoms added","fromName":"Olga Telezhnaya","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-02-12T08:08:54Z","receivedAt":"2018-02-12T08:09:53Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"Add tests for new formatting atoms: rest, deltabase, objectsize:disk.\nrest means nothing and we expand it into empty string.\nWe need this atom for cat-file command.\nHave plans to support deltabase and objectsize:disk further\n(as it is done in cat-file), now also expand it to empty string.\n\nSigned-off-by: Olga Telezhnaia \nMentored-by: Christian Couder \nMentored by: Jeff King \n---\n t/t6300-for-each-ref.sh | 18 ++++++++++++++++++\n 1 file changed, 18 insertions(+)\n\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex c128dfc579079..eee656a6abba9 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -316,6 +316,24 @@ test_expect_success 'exercise strftime with odd fields' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'Check format %(objectsize:disk) gives empty output ' '\n+\techo >expected &&\n+\tgit for-each-ref --format=\"%(objectsize:disk)\" refs/heads >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'Check format %(rest) gives empty output ' '\n+\techo >expected &&\n+\tgit for-each-ref --format=\"%(rest)\" refs/heads >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'Check format %(deltabase) gives empty output ' '\n+\techo >expected &&\n+\tgit for-each-ref --format=\"%(deltabase)\" refs/heads >actual &&\n+\ttest_cmp expected actual\n+'\n+\n cat >expected <<\\EOF\n refs/heads/master\n refs/remotes/origin/master\n\n--\nhttps://github.com/git/git/pull/452\n"},{"id":"339051","messageId":"01020161890f4311-ab635cac-1d48-47ee-90e1-f178f134db1c-000000@eu-west-1.amazonses.com","threadId":"47816","inReplyTo":"01020161890f4236-47989eb4-c19f-4282-9084-9d4f90c2ebeb-000000@eu-west-1.amazonses.com","subject":"[PATCH v3 02/23] ref-filter: add return value to some functions","fromName":"Olga Telezhnaya","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-02-12T08:08:54Z","receivedAt":"2018-02-12T08:09:56Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"Add return flag to format_ref_array_item(), show_ref_array_item(),\nget_ref_array_info() and populate_value() for further using.\nNeed it to handle situations when item is broken but we can not invoke\ndie() because we are in batch mode and all items need to be processed.\n\nSigned-off-by: Olga Telezhnaia \nMentored-by: Christian Couder \nMentored by: Jeff King \n---\n ref-filter.c | 37 ++++++++++++++++++++++++-------------\n ref-filter.h | 14 ++++++++++----\n 2 files changed, 34 insertions(+), 17 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex d04295e33448e..9ed5e66066a7a 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1356,8 +1356,9 @@ static const char *get_refname(struct used_atom *atom, struct ref_array_item *re\n \n /*\n * Parse the object referred by ref, and grab needed value.\n+ * Return 0 if everything was successful, -1 otherwise.\n */\n-static void populate_value(struct ref_array_item *ref)\n+static int populate_value(struct ref_array_item *ref)\n {\n \tvoid *buf;\n \tstruct object *obj;\n@@ -1482,7 +1483,7 @@ static void populate_value(struct ref_array_item *ref)\n \t\t}\n \t}\n \tif (used_atom_cnt <= i)\n-\t\treturn;\n+\t\treturn 0;\n \n \tbuf = get_obj(&ref->objectname, &obj, &size, &eaten);\n \tif (!buf)\n@@ -1501,7 +1502,7 @@ static void populate_value(struct ref_array_item *ref)\n \t * object, we are done.\n \t */\n \tif (!need_tagged || (obj->type != OBJ_TAG))\n-\t\treturn;\n+\t\treturn 0;\n \n \t/*\n \t * If it is a tag object, see if we use a value that derefs\n@@ -1525,19 +1526,24 @@ static void populate_value(struct ref_array_item *ref)\n \tgrab_values(ref->value, 1, obj, buf, size);\n \tif (!eaten)\n \t\tfree(buf);\n+\n+\treturn 0;\n }\n \n /*\n * Given a ref, return the value for the atom. This lazily gets value\n * out of the object by calling populate value.\n+ * Return 0 if everything was successful, -1 otherwise.\n */\n-static void get_ref_atom_value(struct ref_array_item *ref, int atom, struct atom_value **v)\n+static int get_ref_atom_value(struct ref_array_item *ref, int atom, struct atom_value **v)\n {\n+\tint retval = 0;\n \tif (!ref->value) {\n-\t\tpopulate_value(ref);\n+\t\tretval = populate_value(ref);\n \t\tfill_missing_values(ref->value);\n \t}\n \t*v = &ref->value[atom];\n+\treturn retval;\n }\n \n /*\n@@ -2122,7 +2128,7 @@ static void append_literal(const char *cp, const char *ep, struct ref_formatting\n \t}\n }\n \n-void format_ref_array_item(struct ref_array_item *info,\n+int format_ref_array_item(struct ref_array_item *info,\n \t\t\t const struct ref_format *format,\n \t\t\t struct strbuf *final_buf)\n {\n@@ -2138,9 +2144,10 @@ void format_ref_array_item(struct ref_array_item *info,\n \t\tep = strchr(sp, ')');\n \t\tif (cp < sp)\n \t\t\tappend_literal(cp, sp, &state);\n-\t\tget_ref_atom_value(info,\n-\t\t\t\t parse_ref_filter_atom(format, sp + 2, ep),\n-\t\t\t\t &atomv);\n+\t\tif (get_ref_atom_value(info,\n+\t\t\t\t parse_ref_filter_atom(format, sp + 2, ep),\n+\t\t\t\t &atomv))\n+\t\t\treturn -1;\n \t\tatomv->handler(atomv, &state);\n \t}\n \tif (*cp) {\n@@ -2156,17 +2163,21 @@ void format_ref_array_item(struct ref_array_item *info,\n \t\tdie(_(\"format: %%(end) atom missing\"));\n \tstrbuf_addbuf(final_buf, &state.stack->output);\n \tpop_stack_element(&state.stack);\n+\treturn 0;\n }\n \n-void show_ref_array_item(struct ref_array_item *info,\n+int show_ref_array_item(struct ref_array_item *info,\n \t\t\t const struct ref_format *format)\n {\n \tstruct strbuf final_buf = STRBUF_INIT;\n+\tint retval = format_ref_array_item(info, format, &final_buf);\n \n-\tformat_ref_array_item(info, format, &final_buf);\n-\tfwrite(final_buf.buf, 1, final_buf.len, stdout);\n+\tif (!retval) {\n+\t\tfwrite(final_buf.buf, 1, final_buf.len, stdout);\n+\t\tputchar('\\n');\n+\t}\n \tstrbuf_release(&final_buf);\n-\tputchar('\\n');\n+\treturn retval;\n }\n \n void pretty_print_ref(const char *name, const unsigned char *sha1,\ndiff --git a/ref-filter.h b/ref-filter.h\nindex 0d98342b34319..b75c8ac45248e 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -109,12 +109,18 @@ void ref_array_clear(struct ref_array *array);\n int verify_ref_format(struct ref_format *format);\n /* Sort the given ref_array as per the ref_sorting provided */\n void ref_array_sort(struct ref_sorting *sort, struct ref_array *array);\n-/* Based on the given format and quote_style, fill the strbuf */\n-void format_ref_array_item(struct ref_array_item *info,\n+/*\n+ * Based on the given format and quote_style, fill the strbuf.\n+ * Return 0 if everything was successful, -1 otherwise (and strbuf remains empty)\n+ */\n+int format_ref_array_item(struct ref_array_item *info,\n \t\t\t const struct ref_format *format,\n \t\t\t struct strbuf *final_buf);\n-/* Print the ref using the given format and quote_style */\n-void show_ref_array_item(struct ref_array_item *info, const struct ref_format *format);\n+/*\n+ * Print the ref using the given format and quote_style.\n+ * Return 0 if everything was successful, -1 otherwise.\n+ */\n+int show_ref_array_item(struct ref_array_item *info, const struct ref_format *format);\n /* Parse a single sort specifier and add it to the list */\n void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *atom);\n /* Callback function for parsing the sort option */\n\n--\nhttps://github.com/git/git/pull/452\n"},{"id":"339052","messageId":"01020161890f435d-05435bc4-30da-41c4-8bb2-37dcccb4a413-000000@eu-west-1.amazonses.com","threadId":"47816","inReplyTo":"01020161890f4236-47989eb4-c19f-4282-9084-9d4f90c2ebeb-000000@eu-west-1.amazonses.com","subject":"[PATCH v3 09/23] cat-file: start use ref_array_item struct","fromName":"Olga Telezhnaya","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-02-12T08:08:54Z","receivedAt":"2018-02-12T08:09:59Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"Moving from using expand_data to ref_array_item structure.\nThat helps us to reuse functions from ref-filter easier.\n\nSigned-off-by: Olga Telezhnaia \nMentored-by: Christian Couder \nMentored by: Jeff King \n---\n builtin/cat-file.c | 32 ++++++++++++++++++++------------\n ref-filter.h | 5 +++++\n 2 files changed, 25 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 67e7790d2f319..5b7bc34f1ec6d 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -183,27 +183,27 @@ static int is_atom(const char *atom, const char *s, int slen)\n }\n \n static void expand_atom(struct strbuf *sb, const char *atom, int len,\n-\t\t\t struct expand_data *data)\n+\t\t\t struct ref_array_item *item)\n {\n \tif (is_atom(\"objectname\", atom, len))\n-\t\tstrbuf_addstr(sb, oid_to_hex(&data->oid));\n+\t\tstrbuf_addstr(sb, oid_to_hex(&item->objectname));\n \telse if (is_atom(\"objecttype\", atom, len))\n-\t\tstrbuf_addstr(sb, typename(data->type));\n+\t\tstrbuf_addstr(sb, typename(item->type));\n \telse if (is_atom(\"objectsize\", atom, len))\n-\t\tstrbuf_addf(sb, \"%lu\", data->size);\n+\t\tstrbuf_addf(sb, \"%lu\", item->size);\n \telse if (is_atom(\"objectsize:disk\", atom, len))\n-\t\tstrbuf_addf(sb, \"%\"PRIuMAX, (uintmax_t)data->disk_size);\n+\t\tstrbuf_addf(sb, \"%\"PRIuMAX, (uintmax_t)item->disk_size);\n \telse if (is_atom(\"rest\", atom, len)) {\n-\t\tif (data->rest)\n-\t\t\tstrbuf_addstr(sb, data->rest);\n+\t\tif (item->rest)\n+\t\t\tstrbuf_addstr(sb, item->rest);\n \t} else if (is_atom(\"deltabase\", atom, len))\n-\t\tstrbuf_addstr(sb, oid_to_hex(&data->delta_base_oid));\n+\t\tstrbuf_addstr(sb, oid_to_hex(item->delta_base_oid));\n }\n \n-static size_t expand_format(struct strbuf *sb, const char *start, void *vdata)\n+static size_t expand_format(struct strbuf *sb, const char *start, void *data)\n {\n \tconst char *end;\n-\tstruct expand_data *data = vdata;\n+\tstruct ref_array_item *item = data;\n \n \tif (*start != '(')\n \t\treturn 0;\n@@ -211,7 +211,7 @@ static size_t expand_format(struct strbuf *sb, const char *start, void *vdata)\n \tif (!end)\n \t\tdie(\"format element '%s' does not end in ')'\", start);\n \n-\texpand_atom(sb, start + 1, end - start - 1, data);\n+\texpand_atom(sb, start + 1, end - start - 1, item);\n \treturn end - start + 1;\n }\n \n@@ -283,6 +283,7 @@ static void batch_object_write(const char *obj_name, struct batch_options *opt,\n \t\t\t struct expand_data *data)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n+\tstruct ref_array_item item = {0};\n \n \tif (!data->skip_object_info &&\n \t sha1_object_info_extended(data->oid.hash, &data->info,\n@@ -293,7 +294,14 @@ static void batch_object_write(const char *obj_name, struct batch_options *opt,\n \t\treturn;\n \t}\n \n-\tstrbuf_expand(&buf, opt->format.format, expand_format, data);\n+\titem.objectname = data->oid;\n+\titem.type = data->type;\n+\titem.size = data->size;\n+\titem.disk_size = data->disk_size;\n+\titem.rest = data->rest;\n+\titem.delta_base_oid = &data->delta_base_oid;\n+\n+\tstrbuf_expand(&buf, opt->format.format, expand_format, &item);\n \tstrbuf_addch(&buf, '\\n');\n \tbatch_write(opt, buf.buf, buf.len);\n \tstrbuf_release(&buf);\ndiff --git a/ref-filter.h b/ref-filter.h\nindex 52e07dbe6864a..781921d4e0978 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -40,6 +40,11 @@ struct ref_array_item {\n \tconst char *symref;\n \tstruct commit *commit;\n \tstruct atom_value *value;\n+\tenum object_type type;\n+\tunsigned long size;\n+\toff_t disk_size;\n+\tconst char *rest;\n+\tstruct object_id *delta_base_oid;\n \tchar refname[FLEX_ARRAY];\n };\n \n\n--\nhttps://github.com/git/git/pull/452\n"},{"id":"339053","messageId":"01020161890f4373-5a54682e-c1b8-4408-9476-bea5551b992c-000000@eu-west-1.amazonses.com","threadId":"47816","inReplyTo":"01020161890f4236-47989eb4-c19f-4282-9084-9d4f90c2ebeb-000000@eu-west-1.amazonses.com","subject":"[PATCH v3 18/23] cat-file: reuse printing logic from ref-filter","fromName":"Olga Telezhnaya","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-02-12T08:08:54Z","receivedAt":"2018-02-12T08:10:02Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"Reuse code from ref-filter to print resulting message.\n\nSigned-off-by: Olga Telezhnaia \nMentored-by: Christian Couder \nMentored by: Jeff King \n---\n builtin/cat-file.c | 51 ++++-----------------------------------------------\n ref-filter.c | 21 +++++++++++++++++++--\n 2 files changed, 23 insertions(+), 49 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 273b4038e3893..21007995c5ac6 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -176,45 +176,6 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,\n \treturn 0;\n }\n \n-static int is_atom(const char *atom, const char *s, int slen)\n-{\n-\tint alen = strlen(atom);\n-\treturn alen == slen && !memcmp(atom, s, alen);\n-}\n-\n-static void expand_atom(struct strbuf *sb, const char *atom, int len,\n-\t\t\t struct ref_array_item *item)\n-{\n-\tif (is_atom(\"objectname\", atom, len))\n-\t\tstrbuf_addstr(sb, oid_to_hex(&item->oid));\n-\telse if (is_atom(\"objecttype\", atom, len))\n-\t\tstrbuf_addstr(sb, typename(item->type));\n-\telse if (is_atom(\"objectsize\", atom, len))\n-\t\tstrbuf_addf(sb, \"%lu\", item->size);\n-\telse if (is_atom(\"objectsize:disk\", atom, len))\n-\t\tstrbuf_addf(sb, \"%\"PRIuMAX, (uintmax_t)item->disk_size);\n-\telse if (is_atom(\"rest\", atom, len)) {\n-\t\tif (item->rest)\n-\t\t\tstrbuf_addstr(sb, item->rest);\n-\t} else if (is_atom(\"deltabase\", atom, len))\n-\t\tstrbuf_addstr(sb, oid_to_hex(item->delta_base_oid));\n-}\n-\n-static size_t expand_format(struct strbuf *sb, const char *start, void *data)\n-{\n-\tconst char *end;\n-\tstruct ref_array_item *item = data;\n-\n-\tif (*start != '(')\n-\t\treturn 0;\n-\tend = strchr(start + 1, ')');\n-\tif (!end)\n-\t\tdie(\"format element '%s' does not end in ')'\", start);\n-\n-\texpand_atom(sb, start + 1, end - start - 1, item);\n-\treturn end - start + 1;\n-}\n-\n static void batch_write(struct batch_options *opt, const void *data, int len)\n {\n \tif (opt->buffer_output) {\n@@ -282,23 +243,19 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d\n static void batch_object_write(const char *obj_name, struct batch_options *opt,\n \t\t\t struct expand_data *data)\n {\n-\tstruct strbuf buf = STRBUF_INIT;\n \tstruct ref_array_item item = {0};\n \n \titem.oid = data->oid;\n \titem.rest = data->rest;\n \titem.objectname = obj_name;\n \n-\tif (populate_value(&item))\n+\tif (show_ref_array_item(&item, &opt->format))\n \t\treturn;\n-\n-\tdata->type = item.type;\n-\tstrbuf_expand(&buf, opt->format.format, expand_format, &item);\n-\tstrbuf_addch(&buf, '\\n');\n-\tbatch_write(opt, buf.buf, buf.len);\n-\tstrbuf_release(&buf);\n+\tif (!opt->buffer_output)\n+\t\tfflush(stdout);\n \n \tif (opt->print_contents) {\n+\t\tdata->type = item.type;\n \t\tprint_object_or_die(opt, data);\n \t\tbatch_write(opt, \"\\n\", 1);\n \t}\ndiff --git a/ref-filter.c b/ref-filter.c\nindex ee311d51ff81c..eb53b0babdb83 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1465,7 +1465,21 @@ int populate_value(struct ref_array_item *ref)\n \t\t\tname++;\n \t\t}\n \n-\t\tif (starts_with(name, \"refname\"))\n+\t\tif (cat_file_info.is_cat_file) {\n+\t\t\tif (starts_with(name, \"objectname\"))\n+\t\t\t\tv->s = oid_to_hex(&ref->oid);\n+\t\t\telse if (starts_with(name, \"objecttype\"))\n+\t\t\t\tv->s = typename(ref->type);\n+\t\t\telse if (starts_with(name, \"objectsize\")) {\n+\t\t\t\tv->s = xstrfmt(\"%lu\", ref->size);\n+\t\t\t} else if (starts_with(name, \"objectsize:disk\")) {\n+\t\t\t\tv->s = xstrfmt(\"%\"PRIuMAX, (uintmax_t)ref->disk_size);\n+\t\t\t} else if (starts_with(name, \"rest\"))\n+\t\t\t\tv->s = ref->rest;\n+\t\t\telse if (starts_with(name, \"deltabase\"))\n+\t\t\t\tv->s = xstrdup(oid_to_hex(ref->delta_base_oid));\n+\t\t\tcontinue;\n+\t\t} else if (starts_with(name, \"refname\"))\n \t\t\trefname = get_refname(atom, ref);\n \t\telse if (starts_with(name, \"symref\"))\n \t\t\trefname = get_symref(atom, ref);\n@@ -2207,6 +2221,7 @@ int format_ref_array_item(struct ref_array_item *info,\n {\n \tconst char *cp, *sp, *ep;\n \tstruct ref_formatting_state state = REF_FORMATTING_STATE_INIT;\n+\tint retval = 0;\n \n \tstate.quote_style = format->quote_style;\n \tpush_stack_element(&state.stack);\n@@ -2223,6 +2238,8 @@ int format_ref_array_item(struct ref_array_item *info,\n \t\t\treturn -1;\n \t\tatomv->handler(atomv, &state);\n \t}\n+\tif (cat_file_info.is_cat_file && strlen(format->format) == 0)\n+\t\tretval = check_and_fill_for_cat(info);\n \tif (*cp) {\n \t\tsp = cp + strlen(cp);\n \t\tappend_literal(cp, sp, &state);\n@@ -2236,7 +2253,7 @@ int format_ref_array_item(struct ref_array_item *info,\n \t\tdie(_(\"format: %%(end) atom missing\"));\n \tstrbuf_addbuf(final_buf, &state.stack->output);\n \tpop_stack_element(&state.stack);\n-\treturn 0;\n+\treturn retval;\n }\n \n int show_ref_array_item(struct ref_array_item *info,\n\n--\nhttps://github.com/git/git/pull/452\n"},{"id":"339054","messageId":"01020161890f4325-7ab9a01a-ab67-4beb-9721-3dcc5095720a-000000@eu-west-1.amazonses.com","threadId":"47816","inReplyTo":"01020161890f4236-47989eb4-c19f-4282-9084-9d4f90c2ebeb-000000@eu-west-1.amazonses.com","subject":"[PATCH v3 05/23] cat-file: move struct expand_data into ref-filter","fromName":"Olga Telezhnaya","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-02-12T08:08:54Z","receivedAt":"2018-02-12T08:10:05Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"Need that for further reusing of formatting logic in cat-file.\nHave plans to get rid of using expand_data in cat-file at all,\nand use it only in ref-filter for collecting, formatting and printing\nneeded data.\n\nSigned-off-by: Olga Telezhnaia \nMentored-by: Christian Couder \nMentored by: Jeff King \n---\n builtin/cat-file.c | 36 ------------------------------------\n ref-filter.h | 36 ++++++++++++++++++++++++++++++++++++\n 2 files changed, 36 insertions(+), 36 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 98fc5ec069a49..37d6096d201b5 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -176,42 +176,6 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,\n \treturn 0;\n }\n \n-struct expand_data {\n-\tstruct object_id oid;\n-\tenum object_type type;\n-\tunsigned long size;\n-\toff_t disk_size;\n-\tconst char *rest;\n-\tstruct object_id delta_base_oid;\n-\n-\t/*\n-\t * If mark_query is true, we do not expand anything, but rather\n-\t * just mark the object_info with items we wish to query.\n-\t */\n-\tint mark_query;\n-\n-\t/*\n-\t * Whether to split the input on whitespace before feeding it to\n-\t * get_sha1; this is decided during the mark_query phase based on\n-\t * whether we have a %(rest) token in our format.\n-\t */\n-\tint split_on_whitespace;\n-\n-\t/*\n-\t * After a mark_query run, this object_info is set up to be\n-\t * passed to sha1_object_info_extended. It will point to the data\n-\t * elements above, so you can retrieve the response from there.\n-\t */\n-\tstruct object_info info;\n-\n-\t/*\n-\t * This flag will be true if the requested batch format and options\n-\t * don't require us to call sha1_object_info, which can then be\n-\t * optimized out.\n-\t */\n-\tunsigned skip_object_info : 1;\n-};\n-\n static int is_atom(const char *atom, const char *s, int slen)\n {\n \tint alen = strlen(atom);\ndiff --git a/ref-filter.h b/ref-filter.h\nindex b75c8ac45248e..17f2ac24d2739 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -72,6 +72,42 @@ struct ref_filter {\n \t\tverbose;\n };\n \n+struct expand_data {\n+\tstruct object_id oid;\n+\tenum object_type type;\n+\tunsigned long size;\n+\toff_t disk_size;\n+\tconst char *rest;\n+\tstruct object_id delta_base_oid;\n+\n+\t/*\n+\t * If mark_query is true, we do not expand anything, but rather\n+\t * just mark the object_info with items we wish to query.\n+\t */\n+\tint mark_query;\n+\n+\t/*\n+\t * Whether to split the input on whitespace before feeding it to\n+\t * get_sha1; this is decided during the mark_query phase based on\n+\t * whether we have a %(rest) token in our format.\n+\t */\n+\tint split_on_whitespace;\n+\n+\t/*\n+\t * After a mark_query run, this object_info is set up to be\n+\t * passed to sha1_object_info_extended. It will point to the data\n+\t * elements above, so you can retrieve the response from there.\n+\t */\n+\tstruct object_info info;\n+\n+\t/*\n+\t * This flag will be true if the requested batch format and options\n+\t * don't require us to call sha1_object_info, which can then be\n+\t * optimized out.\n+\t */\n+\tunsigned skip_object_info : 1;\n+};\n+\n struct ref_format {\n \t/*\n \t * Set these to define the format; make sure you call\n\n--\nhttps://github.com/git/git/pull/452\n"},{"id":"339055","messageId":"01020161890f4391-9f76231a-4768-461b-80b9-02cada27a943-000000@eu-west-1.amazonses.com","threadId":"47816","inReplyTo":"01020161890f4236-47989eb4-c19f-4282-9084-9d4f90c2ebeb-000000@eu-west-1.amazonses.com","subject":"[PATCH v3 23/23] cat-file: update of docs","fromName":"Olga Telezhnaya","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-02-12T08:08:54Z","receivedAt":"2018-02-12T08:10:07Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"Update the docs for cat-file command. Some new formatting atoms added\nbecause of reusing ref-filter code.\nWe do not support cat-file atoms in general formatting logic\n(there is just the support for cat-file), that is why some of the atoms\nare still explained in cat-file docs.\nWe need to move these explanations when atoms will be supported\nby other commands.\n\nSigned-off-by: Olga Telezhnaia \nMentored-by: Christian Couder \nMentored by: Jeff King \n---\n Documentation/git-cat-file.txt | 13 ++-----------\n 1 file changed, 2 insertions(+), 11 deletions(-)\n\ndiff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt\nindex f90f09b03fae5..90639ac21d0e8 100644\n--- a/Documentation/git-cat-file.txt\n+++ b/Documentation/git-cat-file.txt\n@@ -187,17 +187,8 @@ linkgit:git-rev-parse[1].\n You can specify the information shown for each object by using a custom\n ``. The `` is copied literally to stdout for each\n object, with placeholders of the form `%(atom)` expanded, followed by a\n-newline. The available atoms are:\n-\n-`objectname`::\n-\tThe 40-hex object name of the object.\n-\n-`objecttype`::\n-\tThe type of the object (the same as `cat-file -t` reports).\n-\n-`objectsize`::\n-\tThe size, in bytes, of the object (the same as `cat-file -s`\n-\treports).\n+newline. The available atoms are the same as that of\n+linkgit:git-for-each-ref[1], but there are some additional ones:\n \n `objectsize:disk`::\n \tThe size, in bytes, that the object takes up on disk. See the\n\n--\nhttps://github.com/git/git/pull/452\n"},{"id":"339056","messageId":"01020161890f435c-c3044984-d9aa-4aaf-90e3-1affa109b3b3-000000@eu-west-1.amazonses.com","threadId":"47816","inReplyTo":"01020161890f4236-47989eb4-c19f-4282-9084-9d4f90c2ebeb-000000@eu-west-1.amazonses.com","subject":"[PATCH v3 12/23] cat-file: start reusing populate_value()","fromName":"Olga Telezhnaya","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-02-12T08:08:54Z","receivedAt":"2018-02-12T08:10:10Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"Move logic related to getting object info from cat-file to ref-filter.\nIt will help to reuse whole formatting logic from ref-filter further.\n\nSigned-off-by: Olga Telezhnaia \nMentored-by: Christian Couder \nMentored by: Jeff King \n---\n builtin/cat-file.c | 17 ++++-------------\n ref-filter.c | 20 ++++++++++++++++++++\n ref-filter.h | 1 +\n 3 files changed, 25 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 0c362828ad81e..6db57e3533806 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -285,21 +285,12 @@ static void batch_object_write(const char *obj_name, struct batch_options *opt,\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct ref_array_item item = {0};\n \n-\tif (!data->skip_object_info &&\n-\t sha1_object_info_extended(data->oid.hash, &data->info,\n-\t\t\t\t OBJECT_INFO_LOOKUP_REPLACE) < 0) {\n-\t\tprintf(\"%s missing\\n\",\n-\t\t obj_name ? obj_name : oid_to_hex(&data->oid));\n-\t\tfflush(stdout);\n-\t\treturn;\n-\t}\n-\n \titem.oid = data->oid;\n-\titem.type = data->type;\n-\titem.size = data->size;\n-\titem.disk_size = data->disk_size;\n \titem.rest = data->rest;\n-\titem.delta_base_oid = &data->delta_base_oid;\n+\titem.objectname = obj_name;\n+\n+\tif (populate_value(&item))\n+\t\treturn;\n \n \tstrbuf_expand(&buf, opt->format.format, expand_format, &item);\n \tstrbuf_addch(&buf, '\\n');\ndiff --git a/ref-filter.c b/ref-filter.c\nindex d09ec1bde6d54..3f92a27d98b6c 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1403,6 +1403,23 @@ static const char *get_refname(struct used_atom *atom, struct ref_array_item *re\n \treturn show_ref(&atom->u.refname, ref->refname);\n }\n \n+static int check_and_fill_for_cat(struct ref_array_item *ref)\n+{\n+\tif (!cat_file_info->skip_object_info &&\n+\t sha1_object_info_extended(ref->oid.hash, &cat_file_info->info,\n+\t\t\t\t OBJECT_INFO_LOOKUP_REPLACE) < 0) {\n+\t\tconst char *e = ref->objectname;\n+\t\tprintf(\"%s missing\\n\", e ? e : oid_to_hex(&ref->oid));\n+\t\tfflush(stdout);\n+\t\treturn -1;\n+\t}\n+\tref->type = cat_file_info->type;\n+\tref->size = cat_file_info->size;\n+\tref->disk_size = cat_file_info->disk_size;\n+\tref->delta_base_oid = &cat_file_info->delta_base_oid;\n+\treturn 0;\n+}\n+\n /*\n * Parse the object referred by ref, and grab needed value.\n * Return 0 if everything was successful, -1 otherwise.\n@@ -1424,6 +1441,9 @@ int populate_value(struct ref_array_item *ref)\n \t\t\tref->symref = \"\";\n \t}\n \n+\tif (cat_file_info && check_and_fill_for_cat(ref))\n+\t\treturn -1;\n+\n \t/* Fill in specials first */\n \tfor (i = 0; i < used_atom_cnt; i++) {\n \t\tstruct used_atom *atom = &used_atom[i];\ndiff --git a/ref-filter.h b/ref-filter.h\nindex 87b026b8b76d0..5c6e019998716 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -45,6 +45,7 @@ struct ref_array_item {\n \toff_t disk_size;\n \tconst char *rest;\n \tstruct object_id *delta_base_oid;\n+\tconst char *objectname;\n \tchar refname[FLEX_ARRAY];\n };\n \n\n--\nhttps://github.com/git/git/pull/452\n"},{"id":"339057","messageId":"01020161890f4357-05e636bc-8b44-425b-a252-ff2341f91cdd-000000@eu-west-1.amazonses.com","threadId":"47816","inReplyTo":"01020161890f4236-47989eb4-c19f-4282-9084-9d4f90c2ebeb-000000@eu-west-1.amazonses.com","subject":"[PATCH v3 14/23] ref_filter: add is_atom_used function","fromName":"Olga Telezhnaya","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-02-12T08:08:54Z","receivedAt":"2018-02-12T08:10:12Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"Delete all items related to split_on_whitespace from ref-filter\nand add new function for handling the logic.\nNow cat-file could invoke that function to implementing its logic.\n\nSigned-off-by: Olga Telezhnaia \nMentored-by: Christian Couder \nMentored by: Jeff King \n---\n builtin/cat-file.c | 8 +++-----\n ref-filter.c | 17 +++++++++++++++--\n ref-filter.h | 10 +++-------\n 3 files changed, 21 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 6db57e3533806..3a49b55a1cc2e 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -382,8 +382,7 @@ static int batch_objects(struct batch_options *opt)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct expand_data data;\n-\tint save_warning;\n-\tint retval = 0;\n+\tint save_warning, is_rest, retval = 0;\n \n \tif (!opt->format.format)\n \t\topt->format.format = \"%(objectname) %(objecttype) %(objectsize)\";\n@@ -395,8 +394,6 @@ static int batch_objects(struct batch_options *opt)\n \tmemset(&data, 0, sizeof(data));\n \topt->format.cat_file_data = &data;\n \tverify_ref_format(&opt->format);\n-\tif (opt->cmdmode)\n-\t\tdata.split_on_whitespace = 1;\n \n \tif (opt->all_objects) {\n \t\tstruct object_info empty = OBJECT_INFO_INIT;\n@@ -435,9 +432,10 @@ static int batch_objects(struct batch_options *opt)\n \t */\n \tsave_warning = warn_on_object_refname_ambiguity;\n \twarn_on_object_refname_ambiguity = 0;\n+\tis_rest = opt->cmdmode || is_atom_used(&opt->format, \"rest\");\n \n \twhile (strbuf_getline(&buf, stdin) != EOF) {\n-\t\tif (data.split_on_whitespace) {\n+\t\tif (is_rest) {\n \t\t\t/*\n \t\t\t * Split at first whitespace, tying off the beginning\n \t\t\t * of the string and saving the remainder (or NULL) in\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 34a54db168265..4adeea6aad0da 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -493,8 +493,6 @@ static int parse_ref_filter_atom(const struct ref_format *format,\n \t\tneed_tagged = 1;\n \tif (!strcmp(valid_atom[i].name, \"symref\"))\n \t\tneed_symref = 1;\n-\tif (cat_file_info && !strcmp(valid_atom[i].name, \"rest\"))\n-\t\tcat_file_info->split_on_whitespace = 1;\n \treturn at;\n }\n \n@@ -730,6 +728,21 @@ static const char *find_next(const char *cp)\n \treturn NULL;\n }\n \n+/* Search for atom in given format. */\n+int is_atom_used(const struct ref_format *format, const char *atom)\n+{\n+\tconst char *cp, *sp;\n+\tfor (cp = format->format; *cp && (sp = find_next(cp)); ) {\n+\t\tconst char *ep = strchr(sp, ')');\n+\t\tint atom_len = ep - sp - 2;\n+\t\tsp += 2;\n+\t\tif (atom_len == strlen(atom) && !memcmp(sp, atom, atom_len))\n+\t\t\treturn 1;\n+\t\tcp = ep + 1;\n+\t}\n+\treturn 0;\n+}\n+\n /*\n * Make sure the format string is well formed, and parse out\n * the used atoms.\ndiff --git a/ref-filter.h b/ref-filter.h\nindex 5c6e019998716..fffc443726e28 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -86,13 +86,6 @@ struct expand_data {\n \tconst char *rest;\n \tstruct object_id delta_base_oid;\n \n-\t/*\n-\t * Whether to split the input on whitespace before feeding it to\n-\t * get_sha1; this is decided during the mark_query phase based on\n-\t * whether we have a %(rest) token in our format.\n-\t */\n-\tint split_on_whitespace;\n-\n \t/*\n \t * After a mark_query run, this object_info is set up to be\n \t * passed to sha1_object_info_extended. It will point to the data\n@@ -186,4 +179,7 @@ void pretty_print_ref(const char *name, const unsigned char *sha1,\n /* Fill the values of request and prepare all data for final string creation */\n int populate_value(struct ref_array_item *ref);\n \n+/* Search for atom in given format. */\n+int is_atom_used(const struct ref_format *format, const char *atom);\n+\n #endif /* REF_FILTER_H */\n\n--\nhttps://github.com/git/git/pull/452\n"},{"id":"339058","messageId":"01020161890f4374-60ceebf6-c8aa-4011-bac6-1350549235b1-000000@eu-west-1.amazonses.com","threadId":"47816","inReplyTo":"01020161890f4236-47989eb4-c19f-4282-9084-9d4f90c2ebeb-000000@eu-west-1.amazonses.com","subject":"[PATCH v3 17/23] ref-filter: make valid_atom general again","fromName":"Olga Telezhnaya","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-02-12T08:08:54Z","receivedAt":"2018-02-12T08:10:14Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"Stop using valid_cat_file_atom, making the code more general.\nFurther commits will contain some tests, docs and\nsupport of new features.\n\nSigned-off-by: Olga Telezhnaia \nMentored-by: Christian Couder \nMentored by: Jeff King \n---\n ref-filter.c | 34 +++++++++-------------------------\n 1 file changed, 9 insertions(+), 25 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex cc70bcf2bb8b1..ee311d51ff81c 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -358,8 +358,8 @@ static struct valid_atom {\n \tvoid (*parser)(const struct ref_format *format, struct used_atom *atom, const char *arg);\n } valid_atom[] = {\n \t{ \"refname\" , FIELD_STR, refname_atom_parser },\n-\t{ \"objecttype\" },\n-\t{ \"objectsize\", FIELD_ULONG },\n+\t{ \"objecttype\", FIELD_STR, objecttype_atom_parser },\n+\t{ \"objectsize\", FIELD_ULONG, objectsize_atom_parser },\n \t{ \"objectname\", FIELD_STR, objectname_atom_parser },\n \t{ \"tree\" },\n \t{ \"parent\" },\n@@ -396,12 +396,6 @@ static struct valid_atom {\n \t{ \"if\", FIELD_STR, if_atom_parser },\n \t{ \"then\" },\n \t{ \"else\" },\n-};\n-\n-static struct valid_atom valid_cat_file_atom[] = {\n-\t{ \"objectname\" },\n-\t{ \"objecttype\", FIELD_STR, objecttype_atom_parser },\n-\t{ \"objectsize\", FIELD_ULONG, objectsize_atom_parser },\n \t{ \"rest\" },\n \t{ \"deltabase\", FIELD_STR, deltabase_atom_parser },\n };\n@@ -431,7 +425,6 @@ struct atom_value {\n * Used to parse format string and sort specifiers\n */\n static int parse_ref_filter_atom(const struct ref_format *format,\n-\t\t\t\t const struct valid_atom *valid_atom, int n_atoms,\n \t\t\t\t const char *atom, const char *ep)\n {\n \tconst char *sp;\n@@ -461,13 +454,13 @@ static int parse_ref_filter_atom(const struct ref_format *format,\n \tatom_len = (arg ? arg : ep) - sp;\n \n \t/* Is the atom a valid one? */\n-\tfor (i = 0; i < n_atoms; i++) {\n+\tfor (i = 0; i < ARRAY_SIZE(valid_atom); i++) {\n \t\tint len = strlen(valid_atom[i].name);\n \t\tif (len == atom_len && !memcmp(valid_atom[i].name, sp, len))\n \t\t\tbreak;\n \t}\n \n-\tif (n_atoms <= i)\n+\tif (ARRAY_SIZE(valid_atom) <= i)\n \t\tdie(_(\"unknown field name: %.*s\"), (int)(ep-atom), atom);\n \n \t/* Add it in, including the deref prefix */\n@@ -761,15 +754,9 @@ int verify_ref_format(struct ref_format *format)\n \t\t\treturn error(_(\"malformed format string %s\"), sp);\n \t\t/* sp points at \"%(\" and ep points at the closing \")\" */\n \n-\t\tif (cat_file_info.is_cat_file)\n-\t\t\tat = parse_ref_filter_atom(format, valid_cat_file_atom,\n-\t\t\t\t\t\t ARRAY_SIZE(valid_cat_file_atom), sp + 2, ep);\n-\t\telse {\n-\t\t\tat = parse_ref_filter_atom(format, valid_atom,\n-\t\t\t\t\t\t ARRAY_SIZE(valid_atom), sp + 2, ep);\n-\t\t\tif (skip_prefix(used_atom[at].name, \"color:\", &color))\n-\t\t\t\tformat->need_color_reset_at_eol = !!strcmp(color, \"reset\");\n-\t\t}\n+\t\tat = parse_ref_filter_atom(format, sp + 2, ep);\n+\t\tif (skip_prefix(used_atom[at].name, \"color:\", &color))\n+\t\t\tformat->need_color_reset_at_eol = !!strcmp(color, \"reset\");\n \n \t\tcp = ep + 1;\n \t}\n@@ -2231,9 +2218,7 @@ int format_ref_array_item(struct ref_array_item *info,\n \t\tif (cp < sp)\n \t\t\tappend_literal(cp, sp, &state);\n \t\tif (get_ref_atom_value(info,\n-\t\t\t\t parse_ref_filter_atom(format, valid_atom,\n-\t\t\t\t\t\t\t ARRAY_SIZE(valid_atom),\n-\t\t\t\t\t\t\t sp + 2, ep),\n+\t\t\t\t parse_ref_filter_atom(format, sp + 2, ep),\n \t\t\t\t &atomv))\n \t\t\treturn -1;\n \t\tatomv->handler(atomv, &state);\n@@ -2286,8 +2271,7 @@ static int parse_sorting_atom(const char *atom)\n \t */\n \tstruct ref_format dummy = REF_FORMAT_INIT;\n \tconst char *end = atom + strlen(atom);\n-\treturn parse_ref_filter_atom(&dummy, valid_atom,\n-\t\t\t\t ARRAY_SIZE(valid_atom), atom, end);\n+\treturn parse_ref_filter_atom(&dummy, atom, end);\n }\n \n /* If no sorting option is given, use refname to sort as default */\n\n--\nhttps://github.com/git/git/pull/452\n"},{"id":"339059","messageId":"01020161890f4353-44cc5f60-0e42-492a-9d7f-0f6f1c7b3501-000000@eu-west-1.amazonses.com","threadId":"47816","inReplyTo":"01020161890f4236-47989eb4-c19f-4282-9084-9d4f90c2ebeb-000000@eu-west-1.amazonses.com","subject":"[PATCH v3 13/23] ref-filter: get rid of mark_atom_in_object_info()","fromName":"Olga Telezhnaya","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-02-12T08:08:54Z","receivedAt":"2018-02-12T08:10:20Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"Remove mark_atom_in_object_info() and create same logic\nin terms of ref-filter style.\n\nSigned-off-by: Olga Telezhnaia \nMentored-by: Christian Couder \nMentored by: Jeff King \n---\n ref-filter.c | 45 +++++++++++++++++++++------------------------\n 1 file changed, 21 insertions(+), 24 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 3f92a27d98b6c..34a54db168265 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -255,13 +255,29 @@ static void objectname_atom_parser(const struct ref_format *format, struct used_\n static void objectsize_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)\n {\n \tif (!arg)\n-\t\t; /* default to normal object size */\n+\t\tcat_file_info->info.sizep = &cat_file_info->size;\n \telse if (!strcmp(arg, \"disk\"))\n \t\tcat_file_info->info.disk_sizep = &cat_file_info->disk_size;\n \telse\n \t\tdie(_(\"urecognized %%(objectsize) argument: %s\"), arg);\n }\n \n+static void objecttype_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)\n+{\n+\tif (!arg)\n+\t\tcat_file_info->info.typep = &cat_file_info->type;\n+\telse\n+\t\tdie(_(\"urecognized %%(objecttype) argument: %s\"), arg);\n+}\n+\n+static void deltabase_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)\n+{\n+\tif (!arg)\n+\t\tcat_file_info->info.delta_base_sha1 = cat_file_info->delta_base_oid.hash;\n+\telse\n+\t\tdie(_(\"urecognized %%(deltabase) argument: %s\"), arg);\n+}\n+\n static void refname_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg)\n {\n \trefname_atom_parser_internal(&atom->u.refname, arg, atom->name);\n@@ -384,10 +400,10 @@ static struct valid_atom {\n \n static struct valid_atom valid_cat_file_atom[] = {\n \t{ \"objectname\" },\n-\t{ \"objecttype\" },\n+\t{ \"objecttype\", FIELD_STR, objecttype_atom_parser },\n \t{ \"objectsize\", FIELD_ULONG, objectsize_atom_parser },\n \t{ \"rest\" },\n-\t{ \"deltabase\" },\n+\t{ \"deltabase\", FIELD_STR, deltabase_atom_parser },\n };\n \n #define REF_FORMATTING_STATE_INIT { 0, NULL }\n@@ -411,25 +427,6 @@ struct atom_value {\n \tstruct used_atom *atom;\n };\n \n-static int is_atom(const char *atom, const char *s, int slen)\n-{\n-\tint alen = strlen(atom);\n-\treturn alen == slen && !memcmp(atom, s, alen);\n-}\n-\n-static void mark_atom_in_object_info(const char *atom, int len,\n-\t\t\t\t struct expand_data *data)\n-{\n-\tif (is_atom(\"objecttype\", atom, len))\n-\t\tdata->info.typep = &data->type;\n-\telse if (is_atom(\"objectsize\", atom, len))\n-\t\tdata->info.sizep = &data->size;\n-\telse if (is_atom(\"rest\", atom, len))\n-\t\tdata->split_on_whitespace = 1;\n-\telse if (is_atom(\"deltabase\", atom, len))\n-\t\tdata->info.delta_base_sha1 = data->delta_base_oid.hash;\n-}\n-\n /*\n * Used to parse format string and sort specifiers\n */\n@@ -496,8 +493,8 @@ static int parse_ref_filter_atom(const struct ref_format *format,\n \t\tneed_tagged = 1;\n \tif (!strcmp(valid_atom[i].name, \"symref\"))\n \t\tneed_symref = 1;\n-\tif (cat_file_info)\n-\t\tmark_atom_in_object_info(atom, atom_len, cat_file_info);\n+\tif (cat_file_info && !strcmp(valid_atom[i].name, \"rest\"))\n+\t\tcat_file_info->split_on_whitespace = 1;\n \treturn at;\n }\n \n\n--\nhttps://github.com/git/git/pull/452\n"},{"id":"339060","messageId":"01020161890f4388-f6427c9d-2be6-4628-afc9-bfaeb58b1ebe-000000@eu-west-1.amazonses.com","threadId":"47816","inReplyTo":"01020161890f4236-47989eb4-c19f-4282-9084-9d4f90c2ebeb-000000@eu-west-1.amazonses.com","subject":"[PATCH v3 22/23] cat-file: tests for new atoms added","fromName":"Olga Telezhnaya","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-02-12T08:08:54Z","receivedAt":"2018-02-12T08:10:22Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"Add some tests for new formatting atoms from ref-filter.\nSome of new atoms are supported automatically,\nsome of them are expanded into empty string\n(because they are useless for some types of objects),\nsome of them could be supported later in other patches.\n\nSigned-off-by: Olga Telezhnaia \nMentored-by: Christian Couder \nMentored by: Jeff King \n---\n t/t1006-cat-file.sh | 48 ++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 48 insertions(+)\n\ndiff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\nindex b19f332694620..e72fcaf0e02c5 100755\n--- a/t/t1006-cat-file.sh\n+++ b/t/t1006-cat-file.sh\n@@ -20,6 +20,19 @@ maybe_remove_timestamp () {\n fi\n }\n \n+test_atom () {\n+ name=$1\n+ sha1=$2\n+ atoms=$3\n+ expected=$4\n+\n+ test_expect_success \"$name\" '\n+\techo \"$expected\" >expect &&\n+\techo $sha1 | git cat-file --batch-check=\"$atoms\" >actual &&\n+\ttest_cmp expect actual\n+ '\n+}\n+\n run_tests () {\n type=$1\n sha1=$2\n@@ -119,6 +132,13 @@ $content\"\n \tmaybe_remove_timestamp \"$(cat actual.full)\" $no_ts >actual &&\n \ttest_cmp expect actual\n '\n+\n+ for atom in refname parent body trailers upstream push symref flag\n+ do\n+\ttest_atom \"Check %($atom) gives empty output\" \"$sha1\" \"%($atom)\" \"\"\n+ done\n+\n+ test_atom \"Check %(HEAD) gives only one space as output\" \"$sha1\" '%(HEAD)' ' '\n }\n \n hello_content=\"Hello World\"\n@@ -140,6 +160,12 @@ test_expect_success '--batch-check without %(rest) considers whole line' '\n \ttest_cmp expect actual\n '\n \n+shortname=`echo $hello_sha1 | sed 's/^.\\{0\\}\\(.\\{7\\}\\).*/\\1/'`\n+test_atom 'Check format option %(objectname:short) works' \"$hello_sha1\" '%(objectname:short)' \"$shortname\"\n+\n+test_atom 'Check format option %(align) is not broken' \\\n+ \"$hello_sha1\" \"%(align:8)%(objecttype)%(end)%(objectname)\" \"blob $hello_sha1\"\n+\n tree_sha1=$(git write-tree)\n tree_size=33\n tree_pretty_content=\"100644 blob $hello_sha1\thello\"\n@@ -157,6 +183,17 @@ $commit_message\"\n \n run_tests 'commit' $commit_sha1 $commit_size \"$commit_content\" \"$commit_content\" 1\n \n+test_atom \"Check format option %(if) is not broken\" \"$commit_sha1\" \\\n+ \"%(if)%(author)%(then)%(objectname)%(end)\" \"$commit_sha1\"\n+test_atom \"Check %(tree) works for commit\" \"$commit_sha1\" \"%(tree)\" \"$tree_sha1\"\n+test_atom \"Check %(numparent) works for commit\" \"$commit_sha1\" \"%(numparent)\" \"0\"\n+test_atom \"Check %(authorname) works for commit\" \"$commit_sha1\" \"%(authorname)\" \"$GIT_AUTHOR_NAME\"\n+test_atom \"Check %(authoremail) works for commit\" \"$commit_sha1\" \"%(authoremail)\" \"<$GIT_AUTHOR_EMAIL>\"\n+test_atom \"Check %(committername) works for commit\" \"$commit_sha1\" \"%(committername)\" \"$GIT_COMMITTER_NAME\"\n+test_atom \"Check %(committeremail) works for commit\" \"$commit_sha1\" \"%(committeremail)\" \"<$GIT_COMMITTER_EMAIL>\"\n+test_atom \"Check %(subject) works for commit\" \"$commit_sha1\" \"%(subject)\" \"$commit_message\"\n+test_atom \"Check %(contents) works for commit\" \"$commit_sha1\" \"%(contents)\" \"$commit_message\"\n+\n tag_header_without_timestamp=\"object $hello_sha1\n type blob\n tag hellotag\n@@ -171,6 +208,17 @@ tag_size=$(strlen \"$tag_content\")\n \n run_tests 'tag' $tag_sha1 $tag_size \"$tag_content\" \"$tag_content\" 1\n \n+test_atom \"Check %(object) works for tag\" \"$tag_sha1\" \"%(object)\" \"$hello_sha1\"\n+test_atom \"Check %(type) works for tag\" \"$tag_sha1\" \"%(type)\" \"blob\"\n+test_atom \"Check %(tag) works for tag\" \"$tag_sha1\" \"%(tag)\" \"hellotag\"\n+test_atom \"Check %(taggername) works for tag\" \"$tag_sha1\" \"%(taggername)\" \"$GIT_COMMITTER_NAME\"\n+test_atom \"Check %(taggeremail) works for tag\" \"$tag_sha1\" \"%(taggeremail)\" \"<$GIT_COMMITTER_EMAIL>\"\n+test_atom \"Check %(subject) works for tag\" \"$tag_sha1\" \"%(subject)\" \"$tag_description\"\n+test_atom \"Check %(contents) works for tag\" \"$tag_sha1\" \"%(contents)\" \"$tag_description\"\n+\n+test_atom \"Check %(color) gives no additional output\" \"$sha1\" \\\n+ \"%(objectname) %(color:green) %(objecttype)\" \"$sha1 $type\"\n+\n test_expect_success \\\n \"Reach a blob from a tag pointing to it\" \\\n \"test '$hello_content' = \\\"\\$(git cat-file blob $tag_sha1)\\\"\"\n\n--\nhttps://github.com/git/git/pull/452\n"},{"id":"339061","messageId":"01020161890f433d-ba3260aa-b9e5-47d0-8e80-46ccf9f8c60f-000000@eu-west-1.amazonses.com","threadId":"47816","inReplyTo":"01020161890f4236-47989eb4-c19f-4282-9084-9d4f90c2ebeb-000000@eu-west-1.amazonses.com","subject":"[PATCH v3 06/23] cat-file: split expand_atom() into 2 functions","fromName":"Olga Telezhnaya","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-02-12T08:08:54Z","receivedAt":"2018-02-12T08:10:26Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"Split expand_atom() into 2 different functions,\nmark_atom_in_object_info() prepares variable for further filling,\n(new) expand_atom() creates resulting string.\nNeed that for further reusing of formatting logic from ref-filter.\nBoth functions will be step-by-step removed by the end of this patch.\n\nSigned-off-by: Olga Telezhnaia \nMentored-by: Christian Couder \nMentored by: Jeff King \n---\n builtin/cat-file.c | 73 ++++++++++++++++++++++++++++--------------------------\n 1 file changed, 38 insertions(+), 35 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 37d6096d201b5..edb04a96d9bd3 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -182,47 +182,47 @@ static int is_atom(const char *atom, const char *s, int slen)\n \treturn alen == slen && !memcmp(atom, s, alen);\n }\n \n-static void expand_atom(struct strbuf *sb, const char *atom, int len,\n-\t\t\tvoid *vdata)\n+static void mark_atom_in_object_info(const char *atom, int len,\n+\t\t\t\t struct expand_data *data)\n {\n-\tstruct expand_data *data = vdata;\n+\tif (is_atom(\"objectname\", atom, len))\n+\t\t; /* do nothing */\n+\telse if (is_atom(\"objecttype\", atom, len))\n+\t\tdata->info.typep = &data->type;\n+\telse if (is_atom(\"objectsize\", atom, len))\n+\t\tdata->info.sizep = &data->size;\n+\telse if (is_atom(\"objectsize:disk\", atom, len))\n+\t\tdata->info.disk_sizep = &data->disk_size;\n+\telse if (is_atom(\"rest\", atom, len))\n+\t\tdata->split_on_whitespace = 1;\n+\telse if (is_atom(\"deltabase\", atom, len))\n+\t\tdata->info.delta_base_sha1 = data->delta_base_oid.hash;\n+\telse\n+\t\tdie(\"unknown format element: %.*s\", len, atom);\n+}\n \n-\tif (is_atom(\"objectname\", atom, len)) {\n-\t\tif (!data->mark_query)\n-\t\t\tstrbuf_addstr(sb, oid_to_hex(&data->oid));\n-\t} else if (is_atom(\"objecttype\", atom, len)) {\n-\t\tif (data->mark_query)\n-\t\t\tdata->info.typep = &data->type;\n-\t\telse\n-\t\t\tstrbuf_addstr(sb, typename(data->type));\n-\t} else if (is_atom(\"objectsize\", atom, len)) {\n-\t\tif (data->mark_query)\n-\t\t\tdata->info.sizep = &data->size;\n-\t\telse\n-\t\t\tstrbuf_addf(sb, \"%lu\", data->size);\n-\t} else if (is_atom(\"objectsize:disk\", atom, len)) {\n-\t\tif (data->mark_query)\n-\t\t\tdata->info.disk_sizep = &data->disk_size;\n-\t\telse\n-\t\t\tstrbuf_addf(sb, \"%\"PRIuMAX, (uintmax_t)data->disk_size);\n-\t} else if (is_atom(\"rest\", atom, len)) {\n-\t\tif (data->mark_query)\n-\t\t\tdata->split_on_whitespace = 1;\n-\t\telse if (data->rest)\n+static void expand_atom(struct strbuf *sb, const char *atom, int len,\n+\t\t\t struct expand_data *data)\n+{\n+\tif (is_atom(\"objectname\", atom, len))\n+\t\tstrbuf_addstr(sb, oid_to_hex(&data->oid));\n+\telse if (is_atom(\"objecttype\", atom, len))\n+\t\tstrbuf_addstr(sb, typename(data->type));\n+\telse if (is_atom(\"objectsize\", atom, len))\n+\t\tstrbuf_addf(sb, \"%lu\", data->size);\n+\telse if (is_atom(\"objectsize:disk\", atom, len))\n+\t\tstrbuf_addf(sb, \"%\"PRIuMAX, (uintmax_t)data->disk_size);\n+\telse if (is_atom(\"rest\", atom, len)) {\n+\t\tif (data->rest)\n \t\t\tstrbuf_addstr(sb, data->rest);\n-\t} else if (is_atom(\"deltabase\", atom, len)) {\n-\t\tif (data->mark_query)\n-\t\t\tdata->info.delta_base_sha1 = data->delta_base_oid.hash;\n-\t\telse\n-\t\t\tstrbuf_addstr(sb,\n-\t\t\t\t oid_to_hex(&data->delta_base_oid));\n-\t} else\n-\t\tdie(\"unknown format element: %.*s\", len, atom);\n+\t} else if (is_atom(\"deltabase\", atom, len))\n+\t\tstrbuf_addstr(sb, oid_to_hex(&data->delta_base_oid));\n }\n \n-static size_t expand_format(struct strbuf *sb, const char *start, void *data)\n+static size_t expand_format(struct strbuf *sb, const char *start, void *vdata)\n {\n \tconst char *end;\n+\tstruct expand_data *data = vdata;\n \n \tif (*start != '(')\n \t\treturn 0;\n@@ -230,7 +230,10 @@ static size_t expand_format(struct strbuf *sb, const char *start, void *data)\n \tif (!end)\n \t\tdie(\"format element '%s' does not end in ')'\", start);\n \n-\texpand_atom(sb, start + 1, end - start - 1, data);\n+\tif (data->mark_query)\n+\t\tmark_atom_in_object_info(start + 1, end - start - 1, data);\n+\telse\n+\t\texpand_atom(sb, start + 1, end - start - 1, data);\n \n \treturn end - start + 1;\n }\n\n--\nhttps://github.com/git/git/pull/452\n"},{"id":"339434","messageId":"20180215050850.GA18780@sigill.intra.peff.net","threadId":"47816","inReplyTo":"01020161890f4236-47989eb4-c19f-4282-9084-9d4f90c2ebeb-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH v3 01/23] ref-filter: get rid of goto","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-15T05:08:50Z","receivedAt":"2018-02-15T05:08:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:\n\n> Get rid of goto command in ref-filter for better readability.\n> \n> Signed-off-by: Olga Telezhnaia \n> Mentored-by: Christian Couder \n> Mentored by: Jeff King \n\nOK, makes sense, and the patch looks correct.\n\n-Peff\n"},{"id":"339436","messageId":"20180215051658.GB18780@sigill.intra.peff.net","threadId":"47816","inReplyTo":"01020161890f4311-ab635cac-1d48-47ee-90e1-f178f134db1c-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH v3 02/23] ref-filter: add return value to some functions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-15T05:16:58Z","receivedAt":"2018-02-15T05:17:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:\n\n> Add return flag to format_ref_array_item(), show_ref_array_item(),\n> get_ref_array_info() and populate_value() for further using.\n> Need it to handle situations when item is broken but we can not invoke\n> die() because we are in batch mode and all items need to be processed.\n\nOK. The source of these errors would eventually be calls in\npopulate_value(), but we don't flag any errors there yet (well, we do,\nbut they all end up in die() for now). So I'd expect to see later in the\nseries those die() calls converted to errors (I haven't looked further\nyet; just making a note to myself).\n\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -1356,8 +1356,9 @@ static const char *get_refname(struct used_atom *atom, struct ref_array_item *re\n> \n> /*\n> * Parse the object referred by ref, and grab needed value.\n> + * Return 0 if everything was successful, -1 otherwise.\n> */\n\nWe discussed off-list the concept that the caller may want to know one\nof three outcomes:\n\n - we completed the request, having accessed the object\n - we completed the request, but it didn't require accessing any\n objects\n - an error occurred accessing the object\n\nSince callers like \"cat-file\" would need to check has_sha1_file()\nmanually in the second case. Should this return value actually be an\nenum, which would make it easier to convert later to a tri-state?\n\n> -static void populate_value(struct ref_array_item *ref)\n> +static int populate_value(struct ref_array_item *ref)\n> {\n> \tvoid *buf;\n> \tstruct object *obj;\n> @@ -1482,7 +1483,7 @@ static void populate_value(struct ref_array_item *ref)\n> \t\t}\n> \t}\n> \tif (used_atom_cnt <= i)\n> -\t\treturn;\n> +\t\treturn 0;\n\nMost of these conversions are obviously correct, because they just turn\na void return into one with a value. But this one is trickier:\n\n> @@ -2138,9 +2144,10 @@ void format_ref_array_item(struct ref_array_item *info,\n> \t\tep = strchr(sp, ')');\n> \t\tif (cp < sp)\n> \t\t\tappend_literal(cp, sp, &state);\n> -\t\tget_ref_atom_value(info,\n> -\t\t\t\t parse_ref_filter_atom(format, sp + 2, ep),\n> -\t\t\t\t &atomv);\n> +\t\tif (get_ref_atom_value(info,\n> +\t\t\t\t parse_ref_filter_atom(format, sp + 2, ep),\n> +\t\t\t\t &atomv))\n> +\t\t\treturn -1;\n> \t\tatomv->handler(atomv, &state);\n> \t}\n\nsince it affects the control flow. Might we be skipping any necessary\ncleanup in the function if we see an error?\n\nIt looks like we may have called push_stack_element(), but we'd never\nget to the end of the function where we call pop_stack_element(),\ncausing us to leak.\n\n-Peff\n"},{"id":"339437","messageId":"20180215051831.GC18780@sigill.intra.peff.net","threadId":"47816","inReplyTo":"01020161890f432e-07d6260a-0100-4859-af1c-2cdf8da76829-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH v3 03/23] cat-file: reuse struct ref_format","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-15T05:18:31Z","receivedAt":"2018-02-15T05:18:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:\n\n> Start using ref_format struct instead of simple char*.\n> Need that for further reusing of formatting logic from ref-filter.\n\nMakes sense, and the patch itself looks correct.\n\n-Peff\n"},{"id":"339438","messageId":"20180215052312.GD18780@sigill.intra.peff.net","threadId":"47816","inReplyTo":"01020161890f4324-be5dda9b-9ad7-402f-9196-30f982fd20c2-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH v3 04/23] ref-filter: make valid_atom as function parameter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-15T05:23:12Z","receivedAt":"2018-02-15T05:23:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:\n\n> Make valid_atom as a function parameter,\n> there could be another variable further.\n> Need that for further reusing of formatting logic in cat-file.c.\n> \n> We do not need to allow users to pass their own valid_atom variable in\n> global functions like verify_ref_format() because in the end we want to\n> have same set of valid atoms for all commands. But, as a first step\n> of migrating, I create further another version of valid_atom\n> for cat-file.\n\nHmm. So I see where you're going here, but I think in the end we'd want\nto have a single valid_atom list again, and we wouldn't need this.\n\nAnd indeed, it looks like this goes away in patch 17. Can we\nreorganize/rebase the series so that we avoid dead-end directions like\nthis?\n\n-Peff\n"},{"id":"339439","messageId":"20180215052604.GE18780@sigill.intra.peff.net","threadId":"47816","inReplyTo":"01020161890f4325-7ab9a01a-ab67-4beb-9721-3dcc5095720a-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH v3 05/23] cat-file: move struct expand_data into ref-filter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-15T05:26:04Z","receivedAt":"2018-02-15T05:26:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:\n\n> Need that for further reusing of formatting logic in cat-file.\n> Have plans to get rid of using expand_data in cat-file at all,\n> and use it only in ref-filter for collecting, formatting and printing\n> needed data.\n\nThis seems like another step that we're going to end up reversing later,\nbecause ref-filter has a different representation for these kinds of\nexpansions. I'll keep reading...\n\n-Peff\n"},{"id":"339440","messageId":"20180215052840.GF18780@sigill.intra.peff.net","threadId":"47816","inReplyTo":"01020161890f433d-ba3260aa-b9e5-47d0-8e80-46ccf9f8c60f-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH v3 06/23] cat-file: split expand_atom() into 2 functions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-15T05:28:40Z","receivedAt":"2018-02-15T05:28:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:\n\n> Split expand_atom() into 2 different functions,\n> mark_atom_in_object_info() prepares variable for further filling,\n> (new) expand_atom() creates resulting string.\n> Need that for further reusing of formatting logic from ref-filter.\n> Both functions will be step-by-step removed by the end of this patch.\n\nHmm, this is duplicating the list of atoms. One of the reasons for the\nmark_query flag in the original is to have a single list, so that it\ncan't go out of sync. So I'd hope to see this split go away by the end\nof the series...\n\n-Peff\n"},{"id":"339441","messageId":"20180215053244.GG18780@sigill.intra.peff.net","threadId":"47816","inReplyTo":"01020161890f4348-10a54126-e31d-448d-93a3-55fd89c248c4-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH v3 07/23] cat-file: start migrating formatting to ref-filter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-15T05:32:44Z","receivedAt":"2018-02-15T05:32:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:\n\n> Move mark_atom_in_object_info() from cat-file to ref-filter and\n> start using it in verify_ref_format().\n> It also means that we start reusing verify_ref_format() in cat-file.\n> \n> Start from simple moving of mark_atom_in_object_info(),\n> it would be removed later by integrating all needed processes into\n> ref-filter logic.\n\nI see where you're going here, but it feels like there's a lot of\nboilerplate to move this thing whole, when we plan to get rid of it in\nthe long run. After all, we _already_ support most of these placeholders\nin ref-filter. I'll keep reading...\n\n-Peff\n"},{"id":"339442","messageId":"20180215053714.GH18780@sigill.intra.peff.net","threadId":"47816","inReplyTo":"01020161890f434b-3f93f5b7-0fad-4777-ab9b-79c48b2b41d2-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH v3 08/23] ref-filter: reuse parse_ref_filter_atom()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-15T05:37:15Z","receivedAt":"2018-02-15T05:37:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:\n\n> Continue migrating formatting logic from cat-file to ref-filter.\n> Reuse parse_ref_filter_atom() for unifying all processes in ref-filter\n> and further removing of mark_atom_in_object_info().\n\nOK, now it looks we're moving in a good direction.\n\nOne thing that puzzles me:\n\n> @@ -401,20 +420,14 @@ static int is_atom(const char *atom, const char *s, int slen)\n> static void mark_atom_in_object_info(const char *atom, int len,\n> \t\t\t\t struct expand_data *data)\n> {\n> -\tif (is_atom(\"objectname\", atom, len))\n> -\t\t; /* do nothing */\n> -\telse if (is_atom(\"objecttype\", atom, len))\n> +\tif (is_atom(\"objecttype\", atom, len))\n> \t\tdata->info.typep = &data->type;\n> \telse if (is_atom(\"objectsize\", atom, len))\n> \t\tdata->info.sizep = &data->size;\n> -\telse if (is_atom(\"objectsize:disk\", atom, len))\n> -\t\tdata->info.disk_sizep = &data->disk_size;\n> \telse if (is_atom(\"rest\", atom, len))\n> \t\tdata->split_on_whitespace = 1;\n> \telse if (is_atom(\"deltabase\", atom, len))\n> \t\tdata->info.delta_base_sha1 = data->delta_base_oid.hash;\n> -\telse\n> -\t\tdie(\"unknown format element: %.*s\", len, atom);\n> }\n\nWhy do some of these atoms go away and not others? It seems like we're\nnow relying on ref-filter to parse some of the common ones using its\nexisting atom-parser. But wouldn't it have objecttype and objectsize\nalready, then?\n\n-Peff\n"},{"id":"339443","messageId":"20180215054018.GI18780@sigill.intra.peff.net","threadId":"47816","inReplyTo":"01020161890f435d-05435bc4-30da-41c4-8bb2-37dcccb4a413-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH v3 09/23] cat-file: start use ref_array_item struct","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-15T05:40:18Z","receivedAt":"2018-02-15T05:40:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:\n\n> Moving from using expand_data to ref_array_item structure.\n> That helps us to reuse functions from ref-filter easier.\n\nThis one feels weird. The point of a ref_array_item is for the caller to\nfeed data into the ref-filter formatting code (usually that data comes\nfrom an earlier call to filter_refs(), but in the new cat-file we'd\npresumably feed single items).\n\nBut here we're adding a bunch of fields for items that we'd expect the\nformat code to compute itself.\n\n-Peff\n"},{"id":"339444","messageId":"20180215054203.GJ18780@sigill.intra.peff.net","threadId":"47816","inReplyTo":"01020161890f435c-c3044984-d9aa-4aaf-90e3-1affa109b3b3-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH v3 12/23] cat-file: start reusing populate_value()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-15T05:42:03Z","receivedAt":"2018-02-15T05:42:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:\n\n> Move logic related to getting object info from cat-file to ref-filter.\n> It will help to reuse whole formatting logic from ref-filter further.\n\nThis feels like another wrong-direction step, because we'd eventually\nexpect populate_value() to be called under the hood. And I think in the\nend we end up back there. Which is good, but it's hard to keep track of\nthe dead-ends here...\n\n-Peff\n"},{"id":"339445","messageId":"20180215054542.GK18780@sigill.intra.peff.net","threadId":"47816","inReplyTo":"01020161890f4353-44cc5f60-0e42-492a-9d7f-0f6f1c7b3501-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH v3 13/23] ref-filter: get rid of mark_atom_in_object_info()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-15T05:45:42Z","receivedAt":"2018-02-15T05:45:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:\n\n> Remove mark_atom_in_object_info() and create same logic\n> in terms of ref-filter style.\n\nThis one is definitely a step in the right direction. In fact, this is\nwhat I would have expected to see in the beginning. It seems like this\nfirst half of the series really would benefit from being squashed into a\nfew commits. I.e., I'd have expected to see the whole series looking\nsomething like:\n\n - preparatory cleanup of ref-filter\n\n - teach ref-filter any new atoms (or atom options) that cat-file knows\n about but it doesn't\n\n - convert cat-file to use ref-filter\n\nMost of what I've seen so far is basically that second step, but strung\nout along a bunch of commits. Can we compact it into a few commits that\nall make clear forward progress (by using \"rebase -i\" with \"squash\")?\n\n-Peff\n"},{"id":"339446","messageId":"20180215054945.GL18780@sigill.intra.peff.net","threadId":"47816","inReplyTo":"01020161890f4357-05e636bc-8b44-425b-a252-ff2341f91cdd-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH v3 14/23] ref_filter: add is_atom_used function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-15T05:49:45Z","receivedAt":"2018-02-15T05:49:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:\n\n> Delete all items related to split_on_whitespace from ref-filter\n> and add new function for handling the logic.\n> Now cat-file could invoke that function to implementing its logic.\n\nOK, this is a good direction. I think in a more compact series we'd\navoid moving the split-on-whitespace bits over to ref-filter in the\nfirst place, and have two commits:\n\n - one early in the series adding is_atom_used()\n\n - one late in the series switching cat-file over to is_atom_used() as\n part of the conversion to ref-filter\n\n> diff --git a/builtin/cat-file.c b/builtin/cat-file.c\n> index 6db57e3533806..3a49b55a1cc2e 100644\n> --- a/builtin/cat-file.c\n> +++ b/builtin/cat-file.c\n> @@ -382,8 +382,7 @@ static int batch_objects(struct batch_options *opt)\n> {\n> \tstruct strbuf buf = STRBUF_INIT;\n> \tstruct expand_data data;\n> -\tint save_warning;\n> -\tint retval = 0;\n> +\tint save_warning, is_rest, retval = 0;\n\nTry to avoid reformatting existing code that you're not otherwise\ntouching, as it makes the diff noisier. Just adding \"int is_rest\" would\nmake this easier to review.\n\nI also think the variable name should probably still be\n\"split_on_whitespace\". It's set based on whether we saw a \"%(rest)\"\natom, but ultimately we'll use it to decide whether to split.\n\n-Peff\n"},{"id":"339447","messageId":"20180215055158.GM18780@sigill.intra.peff.net","threadId":"47816","inReplyTo":"01020161890f435c-80fa0ef3-afaf-45b7-84a7-28e2bb66f320-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH v3 15/23] cat-file: move skip_object_info into ref-filter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-15T05:51:58Z","receivedAt":"2018-02-15T05:52:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:\n\n> Move logic related to skip_object_info into ref-filter,\n> so that cat-file does not use that field at all.\n\nI think this is going the wrong way. ref-filter should always do as\nlittle work as possible to fulfill the request. So it should skip the\nobject_info call whenever it can. And then any callers who want to make\nsure that the object exists can do so (as long as the formatting code\ntells them whether it looked up the object or not).\n\nAnd then ref-filter doesn't have to know about this skip_object_info\nflag at all.\n\n-Peff\n"},{"id":"339448","messageId":"20180215055334.GN18780@sigill.intra.peff.net","threadId":"47816","inReplyTo":"01020161890f4376-0d1c5615-ae9c-4320-bc2d-fd3bda19bce0-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH v3 16/23] ref-filter: make cat_file_info independent","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-15T05:53:34Z","receivedAt":"2018-02-15T05:53:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:\n\n> Remove connection between expand_data variable\n> in cat-file and in ref-filter.\n> It will help further to get rid of using expand_data in cat-file.\n\nI have to admit I'm confused at this point about what is_cat_file is\nfor, or even why we need cat_file_data. Shouldn't these items be handled\nby their matching ref-filter atoms at this point?\n\n-Peff\n"},{"id":"339449","messageId":"20180215055351.GO18780@sigill.intra.peff.net","threadId":"47816","inReplyTo":"01020161890f4374-60ceebf6-c8aa-4011-bac6-1350549235b1-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH v3 17/23] ref-filter: make valid_atom general again","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-15T05:53:51Z","receivedAt":"2018-02-15T05:53:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:\n\n> Stop using valid_cat_file_atom, making the code more general.\n> Further commits will contain some tests, docs and\n> support of new features.\n\nYay.\n\n-Peff\n"},{"id":"339450","messageId":"20180215055618.GP18780@sigill.intra.peff.net","threadId":"47816","inReplyTo":"01020161890f437a-302b1577-f034-4a24-a3a2-851d1cda0044-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH v3 20/23] ref-filter: unifying formatting of cat-file opts","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-15T05:56:19Z","receivedAt":"2018-02-15T05:56:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:\n\n> cat-file options are now filled by general logic.\n\nYay.\n\nOne puzzling thing:\n\n> diff --git a/ref-filter.c b/ref-filter.c\n> index 8d104b567eb7c..5781416cf9126 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -824,8 +824,12 @@ static void grab_common_values(struct atom_value *val, int deref, struct object\n> \t\telse if (!strcmp(name, \"objectsize\")) {\n> \t\t\tv->value = sz;\n> \t\t\tv->s = xstrfmt(\"%lu\", sz);\n> -\t\t}\n> -\t\telse if (deref)\n> +\t\t} else if (!strcmp(name, \"objectsize:disk\")) {\n> +\t\t\tif (cat_file_info.is_cat_file) {\n> +\t\t\t\tv->value = cat_file_info.disk_size;\n> +\t\t\t\tv->s = xstrfmt(\"%\"PRIuMAX, (uintmax_t)v->value);\n> +\t\t\t}\n> +\t\t} else if (deref)\n\nWhy do we care about is_cat_file here. Shouldn't:\n\n git for-each-ref --format='%(objectsize:disk)'\n\nwork? I.e., shouldn't the cat_file_info.disk_size variable be held\nsomewhere in a used_atom struct?\n\n-Peff\n"},{"id":"339451","messageId":"20180215055711.GQ18780@sigill.intra.peff.net","threadId":"47816","inReplyTo":"01020161890f438c-9d30a51c-129c-4721-a85a-a3d66a2270b6-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH v3 21/23] for-each-ref: tests for new atoms added","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-15T05:57:12Z","receivedAt":"2018-02-15T05:57:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:\n\n> Add tests for new formatting atoms: rest, deltabase, objectsize:disk.\n> rest means nothing and we expand it into empty string.\n> We need this atom for cat-file command.\n> Have plans to support deltabase and objectsize:disk further\n> (as it is done in cat-file), now also expand it to empty string.\n\nI'm glad that you're adding tests, but I'm not sure it's a good idea to\nadd tests checking for the thing we know to be wrong. If anything, you\ncould be adding test_expect_failure looking for the _right_ thing, and\naccept that it does not yet work.\n\n-Peff\n"},{"id":"339452","messageId":"20180215060033.GR18780@sigill.intra.peff.net","threadId":"47816","inReplyTo":"01020161890f4391-9f76231a-4768-461b-80b9-02cada27a943-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH v3 23/23] cat-file: update of docs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-15T06:00:33Z","receivedAt":"2018-02-15T06:00:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:\n\n> Update the docs for cat-file command. Some new formatting atoms added\n> because of reusing ref-filter code.\n> We do not support cat-file atoms in general formatting logic\n> (there is just the support for cat-file), that is why some of the atoms\n> are still explained in cat-file docs.\n> We need to move these explanations when atoms will be supported\n> by other commands.\n\nOK, so I've read through the whole series now. I still think it really\nneeds some squashing in the middle to turn into a more comprehensible\nseries.\n\nAnd I think by the end we need to address the atoms here that don't work\nin ref-filter. We certainly want them to work in the long term, and the\nfact that they don't is I think pointing to having the wrong\narchitecture here in the intermediate step. We should just have a single\nset of placeholders, with no special \"cat_file_data\". If I understand\ncorrectly, that's what's causing those atoms not to work in\nfor-each-ref.\n\n-Peff\n"},{"id":"339455","messageId":"CAL21BmkQJTGYwR2BywNtsDVuMSGQVO+eWDTqTJL-WoiuAjmg0w@mail.gmail.com","threadId":"47816","inReplyTo":"20180215051658.GB18780@sigill.intra.peff.net","subject":"Re: [PATCH v3 02/23] ref-filter: add return value to some functions","fromName":"Оля Тележная","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-02-15T09:59:00Z","receivedAt":"2018-02-15T09:59:10Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"2018-02-15 8:16 GMT+03:00 Jeff King :\n> On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:\n>\n>> Add return flag to format_ref_array_item(), show_ref_array_item(),\n>> get_ref_array_info() and populate_value() for further using.\n>> Need it to handle situations when item is broken but we can not invoke\n>> die() because we are in batch mode and all items need to be processed.\n>\n> OK. The source of these errors would eventually be calls in\n> populate_value(), but we don't flag any errors there yet (well, we do,\n> but they all end up in die() for now). So I'd expect to see later in the\n> series those die() calls converted to errors (I haven't looked further\n> yet; just making a note to myself).\n>\n>> --- a/ref-filter.c\n>> +++ b/ref-filter.c\n>> @@ -1356,8 +1356,9 @@ static const char *get_refname(struct used_atom *atom, struct ref_array_item *re\n>>\n>> /*\n>> * Parse the object referred by ref, and grab needed value.\n>> + * Return 0 if everything was successful, -1 otherwise.\n>> */\n>\n> We discussed off-list the concept that the caller may want to know one\n> of three outcomes:\n>\n> - we completed the request, having accessed the object\n> - we completed the request, but it didn't require accessing any\n> objects\n> - an error occurred accessing the object\n>\n> Since callers like \"cat-file\" would need to check has_sha1_file()\n> manually in the second case. Should this return value actually be an\n> enum, which would make it easier to convert later to a tri-state?\n\nI decided not to implement this particular scenario because all other\ncallers are waiting that everything will be printed inside ref-filter.\nWe just add support for cat-file there. I don't think that I need to\nre-think all printing process and move printing logic to all other\ncallers so that cat-file will behave fine. In my opinion, in the final\nversion cat-file must accept all ref-filter logic parts and adapt to\nthem.\n\n>\n>> -static void populate_value(struct ref_array_item *ref)\n>> +static int populate_value(struct ref_array_item *ref)\n>> {\n>> void *buf;\n>> struct object *obj;\n>> @@ -1482,7 +1483,7 @@ static void populate_value(struct ref_array_item *ref)\n>> }\n>> }\n>> if (used_atom_cnt <= i)\n>> - return;\n>> + return 0;\n>\n> Most of these conversions are obviously correct, because they just turn\n> a void return into one with a value. But this one is trickier:\n>\n>> @@ -2138,9 +2144,10 @@ void format_ref_array_item(struct ref_array_item *info,\n>> ep = strchr(sp, ')');\n>> if (cp < sp)\n>> append_literal(cp, sp, &state);\n>> - get_ref_atom_value(info,\n>> - parse_ref_filter_atom(format, sp + 2, ep),\n>> - &atomv);\n>> + if (get_ref_atom_value(info,\n>> + parse_ref_filter_atom(format, sp + 2, ep),\n>> + &atomv))\n>> + return -1;\n>> atomv->handler(atomv, &state);\n>> }\n>\n> since it affects the control flow. Might we be skipping any necessary\n> cleanup in the function if we see an error?\n>\n> It looks like we may have called push_stack_element(), but we'd never\n> get to the end of the function where we call pop_stack_element(),\n> causing us to leak.\n\nAgree, I will fix this.\n\n>\n> -Peff\n"},{"id":"339456","messageId":"CAL21BmmyjgzYEHm_VHJkU4KEH+uhCo4G3Au=KM54YAU3n2n_4w@mail.gmail.com","threadId":"47816","inReplyTo":"20180215052312.GD18780@sigill.intra.peff.net","subject":"Re: [PATCH v3 04/23] ref-filter: make valid_atom as function parameter","fromName":"Оля Тележная","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-02-15T10:03:05Z","receivedAt":"2018-02-15T10:03:12Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"2018-02-15 8:23 GMT+03:00 Jeff King :\n> On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:\n>\n>> Make valid_atom as a function parameter,\n>> there could be another variable further.\n>> Need that for further reusing of formatting logic in cat-file.c.\n>>\n>> We do not need to allow users to pass their own valid_atom variable in\n>> global functions like verify_ref_format() because in the end we want to\n>> have same set of valid atoms for all commands. But, as a first step\n>> of migrating, I create further another version of valid_atom\n>> for cat-file.\n>\n> Hmm. So I see where you're going here, but I think in the end we'd want\n> to have a single valid_atom list again, and we wouldn't need this.\n>\n> And indeed, it looks like this goes away in patch 17. Can we\n> reorganize/rebase the series so that we avoid dead-end directions like\n> this?\n\nI tried to do that, but in my opinion it's easier to understand\neverything in current version. I need to squash too many items so that\ncommits do not look atomic, and it's really hard to understand what is\ngoing on. Now we have that helper commit that will be cancelled later,\nand other logic is more clear for readers.\n\n>\n> -Peff\n"},{"id":"339457","messageId":"CAL21Bm=fJxWN9266PXni_SZn=T0t6_jT1kJMtPYo4UA41pj_ZQ@mail.gmail.com","threadId":"47816","inReplyTo":"20180215053714.GH18780@sigill.intra.peff.net","subject":"Re: [PATCH v3 08/23] ref-filter: reuse parse_ref_filter_atom()","fromName":"Оля Тележная","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-02-15T10:11:24Z","receivedAt":"2018-02-15T10:11:31Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"2018-02-15 8:37 GMT+03:00 Jeff King :\n> On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:\n>\n>> Continue migrating formatting logic from cat-file to ref-filter.\n>> Reuse parse_ref_filter_atom() for unifying all processes in ref-filter\n>> and further removing of mark_atom_in_object_info().\n>\n> OK, now it looks we're moving in a good direction.\n>\n> One thing that puzzles me:\n>\n>> @@ -401,20 +420,14 @@ static int is_atom(const char *atom, const char *s, int slen)\n>> static void mark_atom_in_object_info(const char *atom, int len,\n>> struct expand_data *data)\n>> {\n>> - if (is_atom(\"objectname\", atom, len))\n>> - ; /* do nothing */\n>> - else if (is_atom(\"objecttype\", atom, len))\n>> + if (is_atom(\"objecttype\", atom, len))\n>> data->info.typep = &data->type;\n>> else if (is_atom(\"objectsize\", atom, len))\n>> data->info.sizep = &data->size;\n>> - else if (is_atom(\"objectsize:disk\", atom, len))\n>> - data->info.disk_sizep = &data->disk_size;\n>> else if (is_atom(\"rest\", atom, len))\n>> data->split_on_whitespace = 1;\n>> else if (is_atom(\"deltabase\", atom, len))\n>> data->info.delta_base_sha1 = data->delta_base_oid.hash;\n>> - else\n>> - die(\"unknown format element: %.*s\", len, atom);\n>> }\n>\n> Why do some of these atoms go away and not others?\n\nI deleted \"objectname\" because we were doing nothing there;\n\"objectsize:disk\" because we have its own parser function;\n\"die\" because ref-filter has its own checker whether the atom is valid or not.\nI left all others because I haven't supported them at that point. This\nwhole function will be removed later.\n\n> It seems like we're\n> now relying on ref-filter to parse some of the common ones using its\n> existing atom-parser. But wouldn't it have objecttype and objectsize\n> already, then?\n\nWe haven't migrated enough to ref-filter at this point and we can't\nreuse general ref-filter logic about filling the fields. So, we still\nneed to have our own function for doing that. Anyway, as I said\nearlier, we will reach that status in the end of the patch: this\nfunction would be deleted and we will use general ref-filter logic.\n\n>\n> -Peff\n"},{"id":"339459","messageId":"CAL21Bm=+MAEfPRVKjp=3ktTHJLp=0Qf4rfLZNPGP4YfZRd2T6Q@mail.gmail.com","threadId":"47816","inReplyTo":"20180215054018.GI18780@sigill.intra.peff.net","subject":"Re: [PATCH v3 09/23] cat-file: start use ref_array_item struct","fromName":"Оля Тележная","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-02-15T10:13:49Z","receivedAt":"2018-02-15T10:13:58Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"2018-02-15 8:40 GMT+03:00 Jeff King :\n> On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:\n>\n>> Moving from using expand_data to ref_array_item structure.\n>> That helps us to reuse functions from ref-filter easier.\n>\n> This one feels weird. The point of a ref_array_item is for the caller to\n> feed data into the ref-filter formatting code (usually that data comes\n> from an earlier call to filter_refs(), but in the new cat-file we'd\n> presumably feed single items).\n>\n> But here we're adding a bunch of fields for items that we'd expect the\n> format code to compute itself.\n\nIt would be changed later, it's just the addition of new structure\nthat we have never used in cat-file before.\n\n>\n> -Peff\n"},{"id":"339460","messageId":"CAL21BmmeFW08vqx7EA7MsGzAbZjMt-uraz1kqUWWt5aXpziqJQ@mail.gmail.com","threadId":"47816","inReplyTo":"20180215054542.GK18780@sigill.intra.peff.net","subject":"Re: [PATCH v3 13/23] ref-filter: get rid of mark_atom_in_object_info()","fromName":"Оля Тележная","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-02-15T10:19:39Z","receivedAt":"2018-02-15T10:19:46Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"2018-02-15 8:45 GMT+03:00 Jeff King :\n> On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:\n>\n>> Remove mark_atom_in_object_info() and create same logic\n>> in terms of ref-filter style.\n>\n> This one is definitely a step in the right direction. In fact, this is\n> what I would have expected to see in the beginning. It seems like this\n> first half of the series really would benefit from being squashed into a\n> few commits. I.e., I'd have expected to see the whole series looking\n> something like:\n>\n> - preparatory cleanup of ref-filter\n>\n> - teach ref-filter any new atoms (or atom options) that cat-file knows\n> about but it doesn't\n>\n> - convert cat-file to use ref-filter\n>\n> Most of what I've seen so far is basically that second step, but strung\n> out along a bunch of commits. Can we compact it into a few commits that\n> all make clear forward progress (by using \"rebase -i\" with \"squash\")?\n\nI am afraid that I don't really see any 100% improvements by squashing\nsome of the commits. I tried to do that several times and I squashed\nsome of them, but now for me it looks like nothing else could be\nsquashed so that commits still look close to making atomic changes (I\nam sure that it's super important).\n\n>\n> -Peff\n"},{"id":"339461","messageId":"CAL21Bm=7R2URkaRVCbM-T3Yxeq-S+E6Pq_ECSiBk_yJf9D1rag@mail.gmail.com","threadId":"47816","inReplyTo":"20180215054945.GL18780@sigill.intra.peff.net","subject":"Re: [PATCH v3 14/23] ref_filter: add is_atom_used function","fromName":"Оля Тележная","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-02-15T10:20:50Z","receivedAt":"2018-02-15T10:20:57Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"2018-02-15 8:49 GMT+03:00 Jeff King :\n> On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:\n>\n>> Delete all items related to split_on_whitespace from ref-filter\n>> and add new function for handling the logic.\n>> Now cat-file could invoke that function to implementing its logic.\n>\n> OK, this is a good direction. I think in a more compact series we'd\n> avoid moving the split-on-whitespace bits over to ref-filter in the\n> first place, and have two commits:\n>\n> - one early in the series adding is_atom_used()\n>\n> - one late in the series switching cat-file over to is_atom_used() as\n> part of the conversion to ref-filter\n>\n>> diff --git a/builtin/cat-file.c b/builtin/cat-file.c\n>> index 6db57e3533806..3a49b55a1cc2e 100644\n>> --- a/builtin/cat-file.c\n>> +++ b/builtin/cat-file.c\n>> @@ -382,8 +382,7 @@ static int batch_objects(struct batch_options *opt)\n>> {\n>> struct strbuf buf = STRBUF_INIT;\n>> struct expand_data data;\n>> - int save_warning;\n>> - int retval = 0;\n>> + int save_warning, is_rest, retval = 0;\n>\n> Try to avoid reformatting existing code that you're not otherwise\n> touching, as it makes the diff noisier. Just adding \"int is_rest\" would\n> make this easier to review.\n>\n> I also think the variable name should probably still be\n> \"split_on_whitespace\". It's set based on whether we saw a \"%(rest)\"\n> atom, but ultimately we'll use it to decide whether to split.\n\nOK, I will fix that.\n\n>\n> -Peff\n"},{"id":"339462","messageId":"CAL21BmmvOHFve7FRfnRKuzsEm+Hkwo5AZUmqWrM-tmcbcLTSFQ@mail.gmail.com","threadId":"47816","inReplyTo":"20180215055158.GM18780@sigill.intra.peff.net","subject":"Re: [PATCH v3 15/23] cat-file: move skip_object_info into ref-filter","fromName":"Оля Тележная","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-02-15T10:27:40Z","receivedAt":"2018-02-15T10:27:46Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"2018-02-15 8:51 GMT+03:00 Jeff King :\n> On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:\n>\n>> Move logic related to skip_object_info into ref-filter,\n>> so that cat-file does not use that field at all.\n>\n> I think this is going the wrong way. ref-filter should always do as\n> little work as possible to fulfill the request. So it should skip the\n> object_info call whenever it can. And then any callers who want to make\n> sure that the object exists can do so (as long as the formatting code\n> tells them whether it looked up the object or not).\n>\n> And then ref-filter doesn't have to know about this skip_object_info\n> flag at all.\n\nYour message looks contradictory to me.\nI agree that ref-filter should do as least as it's possible, and that\nis the main reason why I put this code there. Moreover, I think that\nit's a good idea to implement that variable not only for cat-file, but\nfor all ref-filter callers. And I think that it's a task of ref-filter\nto check whether the object exists or not (or - whether the ref is\nvalid or not). But I am not sure that I need to solve that moment in\ncurrent patch. It sounds like another separate task.\n\n>\n> -Peff\n"},{"id":"339463","messageId":"CAL21BmmFaDstAT3Jm6AWh95mfWLJqN1MQF+pBTa3erX5w9MWRQ@mail.gmail.com","threadId":"47816","inReplyTo":"20180215055334.GN18780@sigill.intra.peff.net","subject":"Re: [PATCH v3 16/23] ref-filter: make cat_file_info independent","fromName":"Оля Тележная","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-02-15T10:33:25Z","receivedAt":"2018-02-15T10:33:32Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"2018-02-15 8:53 GMT+03:00 Jeff King :\n> On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:\n>\n>> Remove connection between expand_data variable\n>> in cat-file and in ref-filter.\n>> It will help further to get rid of using expand_data in cat-file.\n>\n> I have to admit I'm confused at this point about what is_cat_file is\n> for, or even why we need cat_file_data. Shouldn't these items be handled\n> by their matching ref-filter atoms at this point?\n\nWe discussed that earlier outside of mailing list, and I even tried to\nimplement that idea and spent a couple of days to prove that it's not\npossible.\nThe problem is that the list of atoms is made dynamically, and we\ncan't store pointers to any values in each atom. That's why we need\nseparate cat_file_info variable that is outside of main atom list.\nWe also need is_cat_file because we still have some part of logic that\nis different for cat-file and for all other commands, and sometimes we\nneed to know that information.\n\n>\n> -Peff\n"},{"id":"339464","messageId":"CAL21BmkVgCzihUOxM=Mih=EzYKZwYzpgkSrAZFVr12nkfoCtXw@mail.gmail.com","threadId":"47816","inReplyTo":"20180215055618.GP18780@sigill.intra.peff.net","subject":"Re: [PATCH v3 20/23] ref-filter: unifying formatting of cat-file opts","fromName":"Оля Тележная","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-02-15T10:34:48Z","receivedAt":"2018-02-15T10:34:54Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"2018-02-15 8:56 GMT+03:00 Jeff King :\n> On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:\n>\n>> cat-file options are now filled by general logic.\n>\n> Yay.\n>\n> One puzzling thing:\n>\n>> diff --git a/ref-filter.c b/ref-filter.c\n>> index 8d104b567eb7c..5781416cf9126 100644\n>> --- a/ref-filter.c\n>> +++ b/ref-filter.c\n>> @@ -824,8 +824,12 @@ static void grab_common_values(struct atom_value *val, int deref, struct object\n>> else if (!strcmp(name, \"objectsize\")) {\n>> v->value = sz;\n>> v->s = xstrfmt(\"%lu\", sz);\n>> - }\n>> - else if (deref)\n>> + } else if (!strcmp(name, \"objectsize:disk\")) {\n>> + if (cat_file_info.is_cat_file) {\n>> + v->value = cat_file_info.disk_size;\n>> + v->s = xstrfmt(\"%\"PRIuMAX, (uintmax_t)v->value);\n>> + }\n>> + } else if (deref)\n>\n> Why do we care about is_cat_file here. Shouldn't:\n>\n> git for-each-ref --format='%(objectsize:disk)'\n>\n> work? I.e., shouldn't the cat_file_info.disk_size variable be held\n> somewhere in a used_atom struct?\n\nAt that point - no.\nI think it sounds like other separate task to add this functionality\nand to test it properly.\n\n>\n> -Peff\n"},{"id":"339465","messageId":"CAL21Bmk6vcyKLDMdd9+Q-mt3huUEguO2fEaHFbce0hWNTE8h6g@mail.gmail.com","threadId":"47816","inReplyTo":"20180215055711.GQ18780@sigill.intra.peff.net","subject":"Re: [PATCH v3 21/23] for-each-ref: tests for new atoms added","fromName":"Оля Тележная","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2018-02-15T10:38:35Z","receivedAt":"2018-02-15T10:38:41Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"2018-02-15 8:57 GMT+03:00 Jeff King :\n> On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:\n>\n>> Add tests for new formatting atoms: rest, deltabase, objectsize:disk.\n>> rest means nothing and we expand it into empty string.\n>> We need this atom for cat-file command.\n>> Have plans to support deltabase and objectsize:disk further\n>> (as it is done in cat-file), now also expand it to empty string.\n>\n> I'm glad that you're adding tests, but I'm not sure it's a good idea to\n> add tests checking for the thing we know to be wrong. If anything, you\n> could be adding test_expect_failure looking for the _right_ thing, and\n> accept that it does not yet work.\n\nOK, I will try to fix that when other parts of the patch will look\ngood enough. I am not sure at this point that we will add my code to\nthe main version of the project, so these tests were written actually\nfor checking current functionality better before sending the code for\nthe review.\n\n>\n> -Peff\n"},{"id":"339491","messageId":"xmqq8tbu9ka2.fsf@gitster-ct.c.googlers.com","threadId":"47816","inReplyTo":"CAL21BmkVgCzihUOxM=Mih=EzYKZwYzpgkSrAZFVr12nkfoCtXw@mail.gmail.com","subject":"Re: [PATCH v3 20/23] ref-filter: unifying formatting of cat-file opts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-15T18:31:49Z","receivedAt":"2018-02-15T18:31:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Оля Тележная writes:\n\n>>> - else if (deref)\n>>> + } else if (!strcmp(name, \"objectsize:disk\")) {\n>>> + if (cat_file_info.is_cat_file) {\n>>> + v->value = cat_file_info.disk_size;\n>>> + v->s = xstrfmt(\"%\"PRIuMAX, (uintmax_t)v->value);\n>>> + }\n>>> + } else if (deref)\n>>\n>> Why do we care about is_cat_file here. Shouldn't:\n>>\n>> git for-each-ref --format='%(objectsize:disk)'\n>>\n>> work? I.e., shouldn't the cat_file_info.disk_size variable be held\n>> somewhere in a used_atom struct?\n>\n> At that point - no.\n> I think it sounds like other separate task to add this functionality\n> and to test it properly.\n\nWhat does \"that point\" refer to? This point at 20th patch in the\n23-patch series it is not premature, but it will become feasible in\nlater steps?\n\nAs Peff already said in his review on earlier steps like 4/23 and\n7/23, I too found the series quite confusing and felt as if I was\nwatching somebody stumbling in all directions in the dark in the\nearlier steps in the series before deciding to go in one direction.\n"},{"id":"339538","messageId":"CA+kUOamJowkxp0xAPf5FA+wBkiOjQeYzW1sKMwcatQBBA1qWpw@mail.gmail.com","threadId":"47816","inReplyTo":"01020161890f4388-f6427c9d-2be6-4628-afc9-bfaeb58b1ebe-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH v3 22/23] cat-file: tests for new atoms added","fromName":"Adam Dinwoodie","fromEmail":"git@dinwoodie.org","sentAt":"2018-02-16T14:55:13Z","receivedAt":"2018-02-16T14:56:00Z","isPatch":true,"sender":{"key":"git@dinwoodie.org","avatar":null},"body":"On 12 February 2018 at 08:08, Olga Telezhnaya wrote:\n> Add some tests for new formatting atoms from ref-filter.\n> Some of new atoms are supported automatically,\n> some of them are expanded into empty string\n> (because they are useless for some types of objects),\n> some of them could be supported later in other patches.\n\nThis commit appears to be introducing failures in t1006 on Cygwin.\nSpecifically, tests 15, 36, 58 and 89, all titled \"Check %(refname)\ngives empty output\", are failing on the pu branch at 21937aad4, and\ngit bisect identifies this commit, 3c1571744 (\"cat-file: tests for new\natoms added\", 2018-02-12), as the culprit.\n\nI'm afraid I'm not going to have the time to investigate the failure\nany further in the immediate future, but I wanted to report it\npromptly in case you / someone else can see what's going wrong.\n\nAdam\n"},{"id":"339582","messageId":"20180216215442.GA22270@sigill.intra.peff.net","threadId":"47816","inReplyTo":"CAL21BmmFaDstAT3Jm6AWh95mfWLJqN1MQF+pBTa3erX5w9MWRQ@mail.gmail.com","subject":"Re: [PATCH v3 16/23] ref-filter: make cat_file_info independent","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-16T21:54:43Z","receivedAt":"2018-02-16T21:54:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 15, 2018 at 01:33:25PM +0300, Оля Тележная wrote:\n\n> 2018-02-15 8:53 GMT+03:00 Jeff King :\n> > On Mon, Feb 12, 2018 at 08:08:54AM +0000, Olga Telezhnaya wrote:\n> >\n> >> Remove connection between expand_data variable\n> >> in cat-file and in ref-filter.\n> >> It will help further to get rid of using expand_data in cat-file.\n> >\n> > I have to admit I'm confused at this point about what is_cat_file is\n> > for, or even why we need cat_file_data. Shouldn't these items be handled\n> > by their matching ref-filter atoms at this point?\n> \n> We discussed that earlier outside of mailing list, and I even tried to\n> implement that idea and spent a couple of days to prove that it's not\n> possible.\n> The problem is that the list of atoms is made dynamically, and we\n> can't store pointers to any values in each atom. That's why we need\n> separate cat_file_info variable that is outside of main atom list.\n> We also need is_cat_file because we still have some part of logic that\n> is different for cat-file and for all other commands, and sometimes we\n> need to know that information.\n\nOK, right, I forgot about the pointers thing. This is definitely the\nsort of design decision that should go into the commit message.\n\nI went over our off-list discussion again to refresh my memory. Let me\ntry to summarize a bit (for myself, or for other readerss). Naively, one\nwould hope that you could do something like:\n\n 1. While parsing the %(objectsize) atom, add a pointer like:\n\n the_object_info->sizep = &atom.size;\n\n 2. Later when fulfilling the request for a specific object, if\n the_object_info is not empty, then call sha1_object_info() with it\n to hold the results.\n\n 3. Read the value out of atom.size when assembling the formatted\n string.\n\nBut that has two problems:\n\n a. During parsing, we're resizing the atom array, so the &atom.size\n pointer is subject to change. This is the issue you mentioned\n above. I think we could solve it by taking a pass over the atoms\n after parsing and filling in the pointers then. But...\n\n b. Another problem is that the same atom may appear multiple times. So\n parsing \"%(objectsize) %(objectsize)\", we can't point\n the_object_info at _both_ of them. We need our call to\n sha1_object_info() to store the result in a single location, and\n then as we format each atom they can both use that (which works\n even if they might format it differently, say if you had\n \"%(deltabase) %(deltabase:abbrev)\".\n\nSo we do need some kind of global storage to hold these results. This is\nsort of the same problem we have with parsing the objects at all. There\nwe get by without a global because all of the logic is crammed into\npopulate_value(). In theory we could do the same thing here, but there\nare just a lot of different items we might be asking for. So instead of\n\"need_tagged\", you'd have a multitude of need_objectsize_disk,\nneed_deltabase, etc, which gets unwieldy. We may as well fill in a\n\"struct object_info\".\n\nSo OK, I agree that something like \"struct expand_data\" is needed.\n\nBut what seems non-ideal to me is the way that ref-filter depends on it\nin this cat-file-specific way. If it needs those values it should be\ncalling sha1_object_info(). And if it doesn't, then it can skip the\ncall. It shouldn't need to know about cat-file at all.\n\nAnd likewise cat-file should not know about expand_data. And I think\nthat's true by the end of the series, though the steps in the middle\nleft me a little confused.\n\n-Peff\n"},{"id":"339595","messageId":"58b2bdcd-d621-fd21-ab4d-6a9478319b19@ramsayjones.plus.com","threadId":"47816","inReplyTo":"CA+kUOamJowkxp0xAPf5FA+wBkiOjQeYzW1sKMwcatQBBA1qWpw@mail.gmail.com","subject":"Re: [PATCH v3 22/23] cat-file: tests for new atoms added","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2018-02-18T22:55:29Z","receivedAt":"2018-02-18T22:55:38Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 16/02/18 14:55, Adam Dinwoodie wrote:\n> On 12 February 2018 at 08:08, Olga Telezhnaya wrote:\n>> Add some tests for new formatting atoms from ref-filter.\n>> Some of new atoms are supported automatically,\n>> some of them are expanded into empty string\n>> (because they are useless for some types of objects),\n>> some of them could be supported later in other patches.\n> \n> This commit appears to be introducing failures in t1006 on Cygwin.\n> Specifically, tests 15, 36, 58 and 89, all titled \"Check %(refname)\n> gives empty output\", are failing on the pu branch at 21937aad4, and\n> git bisect identifies this commit, 3c1571744 (\"cat-file: tests for new\n> atoms added\", 2018-02-12), as the culprit.\n\nHi Adam, Olga,\n\nSparse has been complaining about this 'ot/cat-batch-format' branch for\na while, so I had already noted (apart from a symbol which should be\nmarked as static) something that looked somewhat off - namely, the on\nstack initialisation of a 'struct ref_array_item' (builtin/cat-file.c,\nline 246, in function batch_object_write()).\n\nIn particular, the 'struct ref_array_item' ends with a [FLEX_ARRAY] field\nfor the refname, so it clearly isn't meant to be declared on the stack.\nThe 'ref-filter' code uses a 'constructor' function called 'new_ref_array_\\\nitem() which, among others, takes a 'refname' parameter with which to\ninitialise the refname FLEX_ARRAY field.\n\nSo, on Linux, if you build with SANITIZE=address and then run that test:\n\n $ ./t1006-cat-file.sh -i -v\n Initialized empty Git repository in /home/ramsay/git/t/trash directory.t1006-cat-file/.git/\n expecting success: \n \techo_without_newline \"$hello_content\" > hello &&\n \tgit update-index --add hello\n \n ok 1 - setup\n \n ...\n \n =================================================================\n ==12556==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7fff132f04a8 at pc 0x7f1c1b3ca20b bp 0x7fff132f00f0 sp 0x7fff132ef898\n READ of size 4 at 0x7fff132f04a8 thread T0\n #0 0x7f1c1b3ca20a in __interceptor_strlen (/usr/lib/x86_64-linux-gnu/libasan.so.2+0x7020a)\n #1 0x6798cc in strbuf_addstr /home/ramsay/git/strbuf.h:273\n #2 0x6798cc in quote_formatting /home/ramsay/git/ref-filter.c:496\n #3 0x68325d in format_ref_array_item /home/ramsay/git/ref-filter.c:2238\n #4 0x68358a in show_ref_array_item /home/ramsay/git/ref-filter.c:2262\n #5 0x429866 in batch_object_write builtin/cat-file.c:252\n #6 0x42b095 in batch_one_object builtin/cat-file.c:306\n #7 0x42b095 in batch_objects builtin/cat-file.c:407\n #8 0x42b095 in cmd_cat_file builtin/cat-file.c:528\n #9 0x40d3cb in run_builtin /home/ramsay/git/git.c:346\n #10 0x40d3cb in handle_builtin /home/ramsay/git/git.c:556\n #11 0x40f8ae in run_argv /home/ramsay/git/git.c:608\n #12 0x40f8ae in cmd_main /home/ramsay/git/git.c:685\n #13 0x40b667 in main /home/ramsay/git/common-main.c:43\n #14 0x7f1c1ab7982f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)\n #15 0x40ce38 in _start (/home/ramsay/git/git+0x40ce38)\n \n Address 0x7fff132f04a8 is located in stack of thread T0 at offset 328 in frame\n #0 0x4296ff in batch_object_write builtin/cat-file.c:245\n \n This frame has 4 object(s):\n [32, 36) 'type'\n [96, 104) 'contents'\n [160, 168) 'size'\n [224, 328) 'item' <== Memory access at offset 328 overflows this variable\n HINT: this may be a false positive if your program uses some custom stack unwind mechanism or swapcontext\n (longjmp and C++ exceptions *are* supported)\n SUMMARY: AddressSanitizer: stack-buffer-overflow ??:0 __interceptor_strlen\n Shadow bytes around the buggy address:\n 0x100062656040: 00 00 f3 f3 f3 f3 f3 f3 f3 f3 00 00 00 00 00 00\n 0x100062656050: 00 00 00 00 f1 f1 f1 f1 00 00 00 f4 f3 f3 f3 f3\n 0x100062656060: 00 00 00 00 00 00 00 00 00 00 00 00 f1 f1 f1 f1\n 0x100062656070: 04 f4 f4 f4 f2 f2 f2 f2 00 f4 f4 f4 f2 f2 f2 f2\n 0x100062656080: 00 f4 f4 f4 f2 f2 f2 f2 00 00 00 00 00 00 00 00\n =>0x100062656090: 00 00 00 00 00[f4]f4 f4 f3 f3 f3 f3 00 00 00 00\n 0x1000626560a0: 00 00 00 00 00 00 00 00 00 00 00 00 f1 f1 f1 f1\n 0x1000626560b0: 04 f4 f4 f4 f2 f2 f2 f2 04 f4 f4 f4 f2 f2 f2 f2\n 0x1000626560c0: 04 f4 f4 f4 f2 f2 f2 f2 00 f4 f4 f4 f2 f2 f2 f2\n 0x1000626560d0: 00 f4 f4 f4 f2 f2 f2 f2 00 00 f4 f4 f2 f2 f2 f2\n 0x1000626560e0: 00 00 00 f4 f2 f2 f2 f2 00 00 00 f4 f2 f2 f2 f2\n Shadow byte legend (one shadow byte represents 8 application bytes):\n Addressable: 00\n Partially addressable: 01 02 03 04 05 06 07 \n Heap left redzone: fa\n Heap right redzone: fb\n Freed heap region: fd\n Stack left redzone: f1\n Stack mid redzone: f2\n Stack right redzone: f3\n Stack partial redzone: f4\n Stack after return: f5\n Stack use after scope: f8\n Global redzone: f9\n Global init order: f6\n Poisoned by user: f7\n Container overflow: fc\n Array cookie: ac\n Intra object redzone: bb\n ASan internal: fe\n ==12556==ABORTING\n Aborted (core dumped)\n not ok 15 - Check %(refname) gives empty output\n #\t\n #\t\techo \"$expected\" >expect &&\n #\t\techo $sha1 | git cat-file --batch-check=\"$atoms\" >actual &&\n #\t\ttest_cmp expect actual\n #\t \n $ \n \n... you get a stack-overflow at that very stackframe. Incidentally, the\noutput from the failed test #15 on cygwin always looked the same: \n \n $ xxd actual\n 00000000: a0c4 ffff 0a .....\n $ \n \nSo, just as an experiment, I tried changing that variable to a heap\nvariable and initialising it with an empty (\"\") refname:\n \n diff --git a/builtin/cat-file.c b/builtin/cat-file.c\n index c9d83ceff..12a743034 100644\n --- a/builtin/cat-file.c\n +++ b/builtin/cat-file.c\n @@ -243,19 +243,20 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d\n static void batch_object_write(const char *obj_name, struct batch_options *opt,\n \t\t\t struct expand_data *data)\n {\n -\tstruct ref_array_item item = {0};\n +\tstruct ref_array_item *item;\n \n -\titem.oid = data->oid;\n -\titem.rest = data->rest;\n -\titem.objectname = obj_name;\n +\tFLEX_ALLOC_STR(item, refname, \"\");\n +\titem->oid = data->oid;\n +\titem->rest = data->rest;\n +\titem->objectname = obj_name;\n \n -\tif (show_ref_array_item(&item, &opt->format))\n +\tif (show_ref_array_item(item, &opt->format))\n \t\treturn;\n \tif (!opt->buffer_output)\n \t\tfflush(stdout);\n \n \tif (opt->print_contents) {\n -\t\tdata->type = item.type;\n +\t\tdata->type = item->type;\n \t\tprint_object_or_die(opt, data);\n \t\tbatch_write(opt, \"\\n\", 1);\n \t}\n -- \n\n... and now this test passes on cygwin (and the SANITIZE build on Linux).\n\nOf course, this is not a real fix, since this has probably only changed\nthe stack-overflow into an un-diagnosed heap-overflow bug! ;-)\n\nHowever, the above should provide enough info for someone more familiar\nwith the code to implement a correct fix.\n\n[BTW, the symbol that should be marked static is: cat_file_info, in file\nref-filter.c, line 103.]\n\nATB,\nRamsay Jones\n\n\n"},{"id":"339596","messageId":"17c88c6d-c286-1857-965b-2d41f8f67bf4@ramsayjones.plus.com","threadId":"47816","inReplyTo":"58b2bdcd-d621-fd21-ab4d-6a9478319b19@ramsayjones.plus.com","subject":"Re: [PATCH v3 22/23] cat-file: tests for new atoms added","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2018-02-19T01:04:55Z","receivedAt":"2018-02-19T01:05:03Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 18/02/18 22:55, Ramsay Jones wrote:\n> \n> \n> On 16/02/18 14:55, Adam Dinwoodie wrote:\n>> On 12 February 2018 at 08:08, Olga Telezhnaya wrote:\n>>> Add some tests for new formatting atoms from ref-filter.\n>>> Some of new atoms are supported automatically,\n>>> some of them are expanded into empty string\n>>> (because they are useless for some types of objects),\n>>> some of them could be supported later in other patches.\n>>\n>> This commit appears to be introducing failures in t1006 on Cygwin.\n>> Specifically, tests 15, 36, 58 and 89, all titled \"Check %(refname)\n>> gives empty output\", are failing on the pu branch at 21937aad4, and\n>> git bisect identifies this commit, 3c1571744 (\"cat-file: tests for new\n>> atoms added\", 2018-02-12), as the culprit.\n> \n> Hi Adam, Olga,\n> \n> Sparse has been complaining about this 'ot/cat-batch-format' branch for\n> a while, so I had already noted (apart from a symbol which should be\n> marked as static) something that looked somewhat off - namely, the on\n> stack initialisation of a 'struct ref_array_item' (builtin/cat-file.c,\n> line 246, in function batch_object_write()).\n> \n> In particular, the 'struct ref_array_item' ends with a [FLEX_ARRAY] field\n> for the refname, so it clearly isn't meant to be declared on the stack.\n> The 'ref-filter' code uses a 'constructor' function called 'new_ref_array_\\\n> item() which, among others, takes a 'refname' parameter with which to\n> initialise the refname FLEX_ARRAY field.\n> \n> So, on Linux, if you build with SANITIZE=address and then run that test:\n\nBTW, I forgot to mention that, if you run that test without -i, then\nmore than tests #15,36,58 and 89 will fail. In addition, tests #19-20,\n#40-41, #62-63 and #93-94 failed - I did not investigate these\nfailures at all.\n\nATB,\nRamsay Jones\n\n"}]}