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

[Christian Couder]

On Thu, Jan 18, 2018 at 7:22 AM, Оля Тележная  wrote:
> 2018-01-18 2:04 GMT+03:00 Christian Couder :
>> On Wed, Jan 17, 2018 at 10:49 PM, Jeff King  wrote:
>>> On Tue, Jan 16, 2018 at 10:22:23AM +0300, Оля Тележная wrote:
>>>
 >> In other words, I think the endgame is that expand_atom() isn't there at
 >> all, and we're calling the equivalent of format_ref_item() for each
 >> object (except that in a unified formatting world, it probably doesn't
 >> have the word "ref" in it, since that's just one of the items a caller
 >> might pass in).

 Agree! I want to merge current edits, then create format.h file and
 make some renames, then finish migrating process to new format.h and
 support all new meaningful tags.
>>>
>>> I think we have a little bit of chicken and egg there, though. I'm
>>> having trouble reviewing the current work, because it's hard to evaluate
>>> whether it's doing the right thing without seeing the end state.
>>
>> Yeah, to me it feels like you are at a middle point and there are many
>> ways to go forward.
>
> OK. Maybe I misunderstood you and Jeff in our call, I thought that was
> your idea to make a merge now, sorry. I will continue my work here.

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

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

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

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

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

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

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

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

[Christian Couder]

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

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

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

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

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

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

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

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

[Jeff King]

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

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

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

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

-Peff

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

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

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

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

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

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

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

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

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

[Jeff King]

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

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

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

I think what would help for reviewing this is:

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

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

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

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

-Peff

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

[Jeff King]

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

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

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

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

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

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

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

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

-Peff

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

[Olga Telezhnaya]

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

Signed-off-by: Olga Telezhnaia 
Mentored-by: Christian Couder 
Mentored by: Jeff King 
---
 builtin/cat-file.c | 73 +-
 1 file changed, 39 insertions(+), 34 deletions(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index f5fa4fd75af26..f783b39b9bd5c 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -217,47 +217,49 @@ static int is_atom(const char *atom, const char *s, int 
slen)
return alen == slen && !memcmp(atom, s, alen);
 }
 
-static void expand_atom(struct strbuf *sb, const char *atom, int len,
-   void *vdata)
+static void expand_atom_into_fields(struct strbuf *sb, const char *atom, int 
len,
+   struct expand_data *data)
 {
-   struct expand_data *data = vdata;
+   if (is_atom("objectname", atom, len))
+   ; /* do nothing */
+   else if (is_atom("objecttype", atom, len))
+   data->info.typep = >type;
+   else if (is_atom("objectsize", atom, len))
+   data->info.sizep = >size;
+   else if (is_atom("objectsize:disk", atom, len))
+   data->info.disk_sizep = >disk_size;
+   else if (is_atom("rest", atom, len))
+   data->split_on_whitespace = 1;
+   else if (is_atom("deltabase", atom, len))
+   data->info.delta_base_sha1 = data->delta_base_oid.hash;
+   else
+   die("unknown format element: %.*s", len, atom);
+}
 
-   if (is_atom("objectname", atom, len)) {
-   if (!data->mark_query)
-   strbuf_addstr(sb, oid_to_hex(>oid));
-   } else if (is_atom("objecttype", atom, len)) {
-   if (data->mark_query)
-   data->info.typep = >type;
-   else
-   strbuf_addstr(sb, typename(data->type));
-   } else if (is_atom("objectsize", atom, len)) {
-   if (data->mark_query)
-   data->info.sizep = >size;
-   else
-   strbuf_addf(sb, "%lu", data->size);
-   } else if (is_atom("objectsize:disk", atom, len)) {
-   if (data->mark_query)
-   data->info.disk_sizep = >disk_size;
-   else
-   strbuf_addf(sb, "%"PRIuMAX, (uintmax_t)data->disk_size);
-   } else if (is_atom("rest", atom, len)) {
-   if (data->mark_query)
-   data->split_on_whitespace = 1;
-   else if (data->rest)
+static void expand_atom(struct strbuf *sb, const char *atom, int len,
+struct expand_data *data)
+{
+   if (is_atom("objectname", atom, len))
+   strbuf_addstr(sb, oid_to_hex(>oid));
+   else if (is_atom("objecttype", atom, len))
+   strbuf_addstr(sb, typename(data->type));
+   else if (is_atom("objectsize", atom, len))
+   strbuf_addf(sb, "%lu", data->size);
+   else if (is_atom("objectsize:disk", atom, len))
+   strbuf_addf(sb, "%"PRIuMAX, (uintmax_t)data->disk_size);
+   else if (is_atom("rest", atom, len)) {
+   if (data->rest)
strbuf_addstr(sb, data->rest);
-   } else if (is_atom("deltabase", atom, len)) {
-   if (data->mark_query)
-   data->info.delta_base_sha1 = data->delta_base_oid.hash;
-   else
-   strbuf_addstr(sb,
- oid_to_hex(>delta_base_oid));
-   } else
+   } else if (is_atom("deltabase", atom, len))
+   strbuf_addstr(sb, oid_to_hex(>delta_base_oid));
+   else
die("unknown format element: %.*s", len, atom);
 }
 
-static size_t expand_format(struct strbuf *sb, const char *start, void *data)
+static size_t expand_format(struct strbuf *sb, const char *start, void *vdata)
 {
const char *end;
+   struct expand_data *data = vdata;
 
if (*start != '(')
return 0;
@@ -265,7 +267,10 @@ static size_t expand_format(struct strbuf *sb, const char 
*start, void *data)
if (!end)
die("format element '%s' does not end in ')'", start);
 
-   expand_atom(sb, start + 1, end - start - 1, data);
+   if (data->mark_query)
+   expand_atom_into_fields(sb, start + 1, end - start - 1, data);
+   else
+   expand_atom(sb, start + 1, end - start - 1, data);
 
return end - start + 1;
 }

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