Re: [PATCH v3 16/23] ref-filter: make cat_file_info independent

[Jeff King]

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

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

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

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

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

   the_object_info->sizep = 

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

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

But that has two problems:

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

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

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

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

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

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

-Peff

Re: [PATCH v3 16/23] ref-filter: make cat_file_info independent

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

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

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

>
> -Peff

Re: [PATCH v3 16/23] ref-filter: make cat_file_info independent

[Jeff King]

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

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

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

-Peff

[PATCH v3 16/23] ref-filter: make cat_file_info independent

[Olga Telezhnaya]

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

Signed-off-by: Olga Telezhnaia 
Mentored-by: Christian Couder 
Mentored by: Jeff King 
---
 builtin/cat-file.c |  3 ++-
 ref-filter.c   | 36 +++-
 ref-filter.h   |  7 ++-
 3 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 582679f3dca2c..273b4038e3893 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -292,6 +292,7 @@ static void batch_object_write(const char *obj_name, struct 
batch_options *opt,
if (populate_value())
return;
 
+   data->type = item.type;
strbuf_expand(, opt->format.format, expand_format, );
strbuf_addch(, '\n');
batch_write(opt, buf.buf, buf.len);
@@ -392,7 +393,7 @@ static int batch_objects(struct batch_options *opt)
 * sha1_object_info_extended for each object.
 */
memset(, 0, sizeof(data));
-   opt->format.cat_file_data = 
+   opt->format.is_cat_file = 1;
opt->format.all_objects = opt->all_objects;
verify_ref_format(>format);
 
diff --git a/ref-filter.c b/ref-filter.c
index 104cd6aef0102..cc70bcf2bb8b1 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -100,7 +100,7 @@ static struct used_atom {
} u;
 } *used_atom;
 static int used_atom_cnt, need_tagged, need_symref;
-struct expand_data *cat_file_info;
+struct expand_data cat_file_info;
 
 static void color_atom_parser(const struct ref_format *format, struct 
used_atom *atom, const char *color_value)
 {
@@ -255,9 +255,9 @@ static void objectname_atom_parser(const struct ref_format 
*format, struct used_
 static void objectsize_atom_parser(const struct ref_format *format, struct 
used_atom *atom, const char *arg)
 {
if (!arg)
-   cat_file_info->info.sizep = _file_info->size;
+   cat_file_info.info.sizep = _file_info.size;
else if (!strcmp(arg, "disk"))
-   cat_file_info->info.disk_sizep = _file_info->disk_size;
+   cat_file_info.info.disk_sizep = _file_info.disk_size;
else
die(_("urecognized %%(objectsize) argument: %s"), arg);
 }
@@ -265,7 +265,7 @@ static void objectsize_atom_parser(const struct ref_format 
*format, struct used_
 static void objecttype_atom_parser(const struct ref_format *format, struct 
used_atom *atom, const char *arg)
 {
if (!arg)
-   cat_file_info->info.typep = _file_info->type;
+   cat_file_info.info.typep = _file_info.type;
else
die(_("urecognized %%(objecttype) argument: %s"), arg);
 }
@@ -273,7 +273,7 @@ static void objecttype_atom_parser(const struct ref_format 
*format, struct used_
 static void deltabase_atom_parser(const struct ref_format *format, struct 
used_atom *atom, const char *arg)
 {
if (!arg)
-   cat_file_info->info.delta_base_sha1 = 
cat_file_info->delta_base_oid.hash;
+   cat_file_info.info.delta_base_sha1 = 
cat_file_info.delta_base_oid.hash;
else
die(_("urecognized %%(deltabase) argument: %s"), arg);
 }
@@ -751,7 +751,7 @@ int verify_ref_format(struct ref_format *format)
 {
const char *cp, *sp;
 
-   cat_file_info = format->cat_file_data;
+   cat_file_info.is_cat_file = format->is_cat_file;
format->need_color_reset_at_eol = 0;
for (cp = format->format; *cp && (sp = find_next(cp)); ) {
const char *color, *ep = strchr(sp, ')');
@@ -761,7 +761,7 @@ int verify_ref_format(struct ref_format *format)
return error(_("malformed format string %s"), sp);
/* sp points at "%(" and ep points at the closing ")" */
 
-   if (format->cat_file_data)
+   if (cat_file_info.is_cat_file)
at = parse_ref_filter_atom(format, valid_cat_file_atom,
   
ARRAY_SIZE(valid_cat_file_atom), sp + 2, ep);
else {
@@ -775,10 +775,10 @@ int verify_ref_format(struct ref_format *format)
}
if (format->need_color_reset_at_eol && !want_color(format->use_color))
format->need_color_reset_at_eol = 0;
-   if (cat_file_info && format->all_objects) {
+   if (cat_file_info.is_cat_file && format->all_objects) {
struct object_info empty = OBJECT_INFO_INIT;
-   if (!memcmp(_file_info->info, , sizeof(empty)))
-   cat_file_info->skip_object_info = 1;
+   if (!memcmp(_file_info.info, , sizeof(empty)))
+   cat_file_info.skip_object_info = 1;
}
return 0;
 }
@@ -1420,18 +1420,20 @@ static const char *get_refname(struct used_atom *atom, 
struct ref_array_item *re
 
 static int