Re: [PATCH v2 03/18] ref-filter: make valid_atom as function parameter

[Jeff King]

On Fri, Jan 19, 2018 at 10:47:57AM -0800, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > I also think %(deltabase) does make sense for anything that points to an
> > object. I suspect it's not all that _useful_ for for-each-ref, but that
> > doesn't mean we can't return the sensible thing if somebody asks for it.
> 
> This may not be a new issue (or any issue at all), but is the
> ability to learn deltabase make any sense in the first place?
> 
> What should the code do when an object has three copies in the
> repo, i.e. one as a base object (or a loose one), another as a
> delta against an object, and the third one as a delta against
> a different object?

This was a known issue when I introduced %(deltabase). The documentation
explicitly calls this out and makes no promises about which copy we
describe.

The %(objectsize:disk) atom has the same issue, too. See the CAVEATS
section of git-cat-file(1).

-Peff

Re: [PATCH v2 03/18] ref-filter: make valid_atom as function parameter

[Junio C Hamano]

Jeff King  writes:

> I also think %(deltabase) does make sense for anything that points to an
> object. I suspect it's not all that _useful_ for for-each-ref, but that
> doesn't mean we can't return the sensible thing if somebody asks for it.

This may not be a new issue (or any issue at all), but is the
ability to learn deltabase make any sense in the first place?

What should the code do when an object has three copies in the
repo, i.e. one as a base object (or a loose one), another as a
delta against an object, and the third one as a delta against
a different object?

Re: [PATCH v2 03/18] ref-filter: make valid_atom as function parameter

[Christian Couder]

On Fri, Jan 19, 2018 at 6:22 PM, Оля Тележная  wrote:
> 2018-01-19 20:14 GMT+03:00 Christian Couder :
>> On Thu, Jan 18, 2018 at 7:20 AM, Оля Тележная  
>> wrote:

>>> And another thoughts here - we were thinking about creating format.h
>>> but decided not to move forward with it, and now we are suffering
>>> because of it. Can I create it right now or the history of commits
>>> would be too dirty because of it?
>>
>> It would also make it difficult to refactor your patch series if there
>> is a big move or renaming in the middle.
>>
>>> Also, do you mean just renaming of
>>> ref-filter? I was thinking that I need to put formatting-related logic
>>> to another file and leave all other stuff in ref-filter.
>>
>> Yeah, you can do both a move and a renaming.
>
> Thanks for a response! That thought is not clear enough for me. Do you
> want me to split ref-filter into 2 files (one is for formatting only
> called format and other one is for anything else still called
> ref-filter) - here is a second question by the way, do I need to
> create only format.h (and leave all realizations in ref-filter.c), or
> I also need to create format.c. Or, just to rename ref-filter into
> format and that's all.

Just renaming ref-filter into format (including the filenames) will
probably be enough, but it's also possible that it will make more
sense to keep some code only relevant to ref filtering into
ref-filter.{c,h}. We will be in a better position to decide what we
should do when the migration is finished.

Re: [PATCH v2 03/18] ref-filter: make valid_atom as function parameter

[Christian Couder]

On Fri, Jan 19, 2018 at 6:23 PM, Jeff King  wrote:
> On Fri, Jan 19, 2018 at 06:14:56PM +0100, Christian Couder wrote:
>
>> > Let's discuss, what behavior we are waiting for
>> > when atom seems useless for the command. Die or ignore?
>>
>> We could alternatively just emit a warning, but it looks like there
>> are a lot of die() calls already in ref-filter.c, so I would suggest
>> die().
>
> I actually think it makes sense to just expand nonsense into the empty
> string, for two reasons:
>
>   1. That's what ref-filter already does for the existing cases. For
>  example, try:
>
>git for-each-ref --format='%(objecttype) %(authordate)'
>
>  and you will see that the annotated tags just get a blank author
>  field.
>
>   2. I think we may end up with a case where we feed multiple objects
>  via --batch-check, and the format is only nonsense for _some_ of
>  them. E.g., I envision a world where you can do:
>
>git cat-file --batch-check='%(objecttype) %(refname)' <<-\EOF
>master
>12345abcde12345abcde12345abcde12345abcde
>EOF
>
>  and get output like:
>
>commit refs/heads/master
>commit
>
>  (i.e., if we would remember the refname discovered during the name
>  resolution, we could still report it). It would be annoying if the
>  second line caused us to die().

Yeah, ok, that makes sense.

>> > And, which
>> > atoms are useless (as I understand, "rest" and "deltabase" from
>> > cat-file are useless for all ref-filter users, so the question is
>> > about - am I right in it, and about ref-filter atoms for cat-file).
>>
>> For now and I think until the migration process is finished, you could
>> just die() in case of any atom not already supported by the command.
>
> I'm OK with dying in the interim if it's easier, though I suspect it is
> not much extra work to just expand to the empty string in such cases. If
> that's where we want to end up eventually, it may be worth going
> straight there.
>
> I also think %(deltabase) does make sense for anything that points to an
> object. I suspect it's not all that _useful_ for for-each-ref, but that
> doesn't mean we can't return the sensible thing if somebody asks for it.
>
> I agree that %(rest) probably doesn't make any sense for a caller which
> isn't parsing input.

Yeah, ok.

Re: [PATCH v2 03/18] ref-filter: make valid_atom as function parameter

[Jeff King]

On Fri, Jan 19, 2018 at 06:14:56PM +0100, Christian Couder wrote:

> > Let's discuss, what behavior we are waiting for
> > when atom seems useless for the command. Die or ignore?
> 
> We could alternatively just emit a warning, but it looks like there
> are a lot of die() calls already in ref-filter.c, so I would suggest
> die().

I actually think it makes sense to just expand nonsense into the empty
string, for two reasons:

  1. That's what ref-filter already does for the existing cases. For
 example, try:

   git for-each-ref --format='%(objecttype) %(authordate)'

 and you will see that the annotated tags just get a blank author
 field.

  2. I think we may end up with a case where we feed multiple objects
 via --batch-check, and the format is only nonsense for _some_ of
 them. E.g., I envision a world where you can do:

   git cat-file --batch-check='%(objecttype) %(refname)' <<-\EOF
   master
   12345abcde12345abcde12345abcde12345abcde
   EOF

 and get output like:

   commit refs/heads/master
   commit

 (i.e., if we would remember the refname discovered during the name
 resolution, we could still report it). It would be annoying if the
 second line caused us to die().

> > And, which
> > atoms are useless (as I understand, "rest" and "deltabase" from
> > cat-file are useless for all ref-filter users, so the question is
> > about - am I right in it, and about ref-filter atoms for cat-file).
> 
> For now and I think until the migration process is finished, you could
> just die() in case of any atom not already supported by the command.

I'm OK with dying in the interim if it's easier, though I suspect it is
not much extra work to just expand to the empty string in such cases. If
that's where we want to end up eventually, it may be worth going
straight there.

I also think %(deltabase) does make sense for anything that points to an
object. I suspect it's not all that _useful_ for for-each-ref, but that
doesn't mean we can't return the sensible thing if somebody asks for it.

I agree that %(rest) probably doesn't make any sense for a caller which
isn't parsing input.

-Peff

Re: [PATCH v2 03/18] ref-filter: make valid_atom as function parameter

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

2018-01-19 20:14 GMT+03:00 Christian Couder :
> On Thu, Jan 18, 2018 at 7:20 AM, Оля Тележная  
> wrote:
>> 2018-01-18 1:39 GMT+03:00 Christian Couder :
>>> On Wed, Jan 17, 2018 at 10:43 PM, Jeff King  wrote:
 On Tue, Jan 16, 2018 at 09:55:22AM +0300, Оля Тележная wrote:

> > IOW, the progression I'd expect in a series like this is:
> >
> >   1. Teach ref-filter.c to support everything that cat-file can do.
> >
> >   2. Convert cat-file to use ref-filter.c.
>
> I agree, I even made this and it's working fine:
> https://github.com/git/git/pull/450/commits/1b74f1047f07434dccb207534d1ad45a143e3f2b
>>>
>>> (Nit: it looks like the above link does not work any more, but it
>>> seems that you are talking about the last patch on the catfile
>>> branch.)
>>>
> But I decided not to add that to patch because I expand the
> functionality of several commands (not only cat-file and
> for-each-ref), and I need to support all new functionality in a proper
> way, make these error messages, test everything and - the hardest one
> - support many new commands for cat-file. As I understand, it is not
> possible unless we finally move to ref-filter and print results also
> there. Oh, and I also need to rewrite docs in that case. And I decided
> to apply this in another patch. But, please, say your opinion, maybe
> we could do that here in some way.

 Yeah, I agree that it will cause changes to other users of ref-filter,
 and you'd need to deal with documentation and tests there. But I think
 we're going to have to do those things eventually (since supporting
 those extra fields everywhere is part of the point of the project). And
 by doing them now, I think it can make the transition for cat-file a lot
 simpler, because we never have to puzzle over this intermediate state
 where only some of the atoms are valid for cat-file.
>>>
>>> I agree that you will have to deal with documentation and tests at one
>>> point and that it could be a good idea to do that now.
>>>
>>> I wonder if it is possible to add atoms one by one into ref-filter and
>>> to add tests and documentation at the same time, instead of merging
>>> cat-file atoms with ref-filter atoms in one big step.
>>>
>>> When all the cat-file atoms have been added to ref-filter's
>>> valid_atom, maybe you could add ref-filter's atoms to cat-file's
>>> valid_cat_file_atom one by one and add tests and documentation at the
>>> same time.
>>>
>>> And then when ref-filter's valid_atom and cat-file's
>>> valid_cat_file_atom are identical you can remove cat-file's
>>> valid_cat_file_atom and maybe after that rename "ref-filter" to
>>> "format".
>>
>> I think it's important to finish migrating process at first. I mean,
>> now we are preparing and collecting everything in ref-filter, but we
>> make resulting string and print still in cat-file. And I am not sure,
>> but maybe it will not be possible to start using new atoms in cat-file
>> while some part of logic still differs.
>
> Ok, you can finish the migration process then.
>
>> And another thoughts here - we were thinking about creating format.h
>> but decided not to move forward with it, and now we are suffering
>> because of it. Can I create it right now or the history of commits
>> would be too dirty because of it?
>
> It would also make it difficult to refactor your patch series if there
> is a big move or renaming in the middle.
>
>> Also, do you mean just renaming of
>> ref-filter? I was thinking that I need to put formatting-related logic
>> to another file and leave all other stuff in ref-filter.
>
> Yeah, you can do both a move and a renaming.

Thanks for a response! That thought is not clear enough for me. Do you
want me to split ref-filter into 2 files (one is for formatting only
called format and other one is for anything else still called
ref-filter) - here is a second question by the way, do I need to
create only format.h (and leave all realizations in ref-filter.c), or
I also need to create format.c. Or, just to rename ref-filter into
format and that's all.

>
>> Anyway, your suggested steps looks good, and I am planning to
>> implement them later.
>
> Ok.
>
>> Let's discuss, what behavior we are waiting for
>> when atom seems useless for the command. Die or ignore?
>
> We could alternatively just emit a warning, but it looks like there
> are a lot of die() calls already in ref-filter.c, so I would suggest
> die().
>
>> And, which
>> atoms are useless (as I understand, "rest" and "deltabase" from
>> cat-file are useless for all ref-filter users, so the question is
>> about - am I right in it, and about ref-filter atoms for cat-file).
>
> For now and I think until the migration process is finished, you could
> just die() in case of any atom not already supported by the command.
>
>> I have never written any tests 

Re: [PATCH v2 03/18] ref-filter: make valid_atom as function parameter

[Christian Couder]

On Thu, Jan 18, 2018 at 7:20 AM, Оля Тележная  wrote:
> 2018-01-18 1:39 GMT+03:00 Christian Couder :
>> On Wed, Jan 17, 2018 at 10:43 PM, Jeff King  wrote:
>>> On Tue, Jan 16, 2018 at 09:55:22AM +0300, Оля Тележная wrote:
>>>
 > IOW, the progression I'd expect in a series like this is:
 >
 >   1. Teach ref-filter.c to support everything that cat-file can do.
 >
 >   2. Convert cat-file to use ref-filter.c.

 I agree, I even made this and it's working fine:
 https://github.com/git/git/pull/450/commits/1b74f1047f07434dccb207534d1ad45a143e3f2b
>>
>> (Nit: it looks like the above link does not work any more, but it
>> seems that you are talking about the last patch on the catfile
>> branch.)
>>
 But I decided not to add that to patch because I expand the
 functionality of several commands (not only cat-file and
 for-each-ref), and I need to support all new functionality in a proper
 way, make these error messages, test everything and - the hardest one
 - support many new commands for cat-file. As I understand, it is not
 possible unless we finally move to ref-filter and print results also
 there. Oh, and I also need to rewrite docs in that case. And I decided
 to apply this in another patch. But, please, say your opinion, maybe
 we could do that here in some way.
>>>
>>> Yeah, I agree that it will cause changes to other users of ref-filter,
>>> and you'd need to deal with documentation and tests there. But I think
>>> we're going to have to do those things eventually (since supporting
>>> those extra fields everywhere is part of the point of the project). And
>>> by doing them now, I think it can make the transition for cat-file a lot
>>> simpler, because we never have to puzzle over this intermediate state
>>> where only some of the atoms are valid for cat-file.
>>
>> I agree that you will have to deal with documentation and tests at one
>> point and that it could be a good idea to do that now.
>>
>> I wonder if it is possible to add atoms one by one into ref-filter and
>> to add tests and documentation at the same time, instead of merging
>> cat-file atoms with ref-filter atoms in one big step.
>>
>> When all the cat-file atoms have been added to ref-filter's
>> valid_atom, maybe you could add ref-filter's atoms to cat-file's
>> valid_cat_file_atom one by one and add tests and documentation at the
>> same time.
>>
>> And then when ref-filter's valid_atom and cat-file's
>> valid_cat_file_atom are identical you can remove cat-file's
>> valid_cat_file_atom and maybe after that rename "ref-filter" to
>> "format".
>
> I think it's important to finish migrating process at first. I mean,
> now we are preparing and collecting everything in ref-filter, but we
> make resulting string and print still in cat-file. And I am not sure,
> but maybe it will not be possible to start using new atoms in cat-file
> while some part of logic still differs.

Ok, you can finish the migration process then.

> And another thoughts here - we were thinking about creating format.h
> but decided not to move forward with it, and now we are suffering
> because of it. Can I create it right now or the history of commits
> would be too dirty because of it?

It would also make it difficult to refactor your patch series if there
is a big move or renaming in the middle.

> Also, do you mean just renaming of
> ref-filter? I was thinking that I need to put formatting-related logic
> to another file and leave all other stuff in ref-filter.

Yeah, you can do both a move and a renaming.

> Anyway, your suggested steps looks good, and I am planning to
> implement them later.

Ok.

> Let's discuss, what behavior we are waiting for
> when atom seems useless for the command. Die or ignore?

We could alternatively just emit a warning, but it looks like there
are a lot of die() calls already in ref-filter.c, so I would suggest
die().

> And, which
> atoms are useless (as I understand, "rest" and "deltabase" from
> cat-file are useless for all ref-filter users, so the question is
> about - am I right in it, and about ref-filter atoms for cat-file).

For now and I think until the migration process is finished, you could
just die() in case of any atom not already supported by the command.

> I have never written any tests and docs for Git, I will try to explore
> by myself how to do that, but if you have any special links/materials
> about it - please send them to me :)

I think that looking at the existing documentation and tests is
probably the best way to learn about it.

Re: [PATCH v2 03/18] ref-filter: make valid_atom as function parameter

[Christian Couder]

On Fri, Jan 19, 2018 at 1:24 PM, Оля Тележная  wrote:
> 2018-01-18 17:23 GMT+03:00 Christian Couder :
>> On Thu, Jan 18, 2018 at 12:49 PM, Оля Тележная  
>> wrote:
>>> 2018-01-18 9:20 GMT+03:00 Оля Тележная :

 I think it's important to finish migrating process at first. I mean,
 now we are preparing and collecting everything in ref-filter, but we
 make resulting string and print still in cat-file. And I am not sure,
 but maybe it will not be possible to start using new atoms in cat-file
 while some part of logic still differs.
>>>
>>> I tried to make that part here:
>>> https://github.com/telezhnaya/git/commit/19a148614f1d4db1f8e628eb4e6d7c819d2da875
>>> I know that the code is disgusting and there is a memory leak :) I
>>> just try to reuse ref-filter logic, I will cleanup everything later.
>>> At first, I try to make it work.
>>> The problem is that I have segfault, and if I use gdb, I get:
>>>
>>> Program received signal SIGSEGV, Segmentation fault.
>>> 0x in ?? ()
>>
>> Make sure that you compile with debug options like -g3. For example I use:
>>
>> $ make -j 4 DEVELOPER=1 CFLAGS="-g3"
>
> Is it OK that I get different test results with simple make and with
> make with all that flags?
> Have a code: https://github.com/telezhnaya/git/commits/catfile
> I do:
>
> olya@ubuntu17-vm:~/git$ make install
> olya@ubuntu17-vm:~/git$ cd t
> olya@ubuntu17-vm:~/git/t$ ./t1006-cat-file.sh
>
> And I have 17 tests broken.
> Then, without any changes in code, I do:
>
> olya@ubuntu17-vm:~/git$ make -j 4 DEVELOPER=1 CFLAGS="-g3" install
> olya@ubuntu17-vm:~/git$ cd t
> olya@ubuntu17-vm:~/git/t$ ./t1006-cat-file.sh
>
> And there is 42 tests broken.
> And it's really hard to search for errors in such situation.

Some segfaults and other memory issues might happen or not happen
depending on how the code has been compiled. I think that if you fix
all the memory related issues, the behavior should be the same.

Re: [PATCH v2 03/18] ref-filter: make valid_atom as function parameter

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

2018-01-18 17:23 GMT+03:00 Christian Couder :
> On Thu, Jan 18, 2018 at 12:49 PM, Оля Тележная  
> wrote:
>> 2018-01-18 9:20 GMT+03:00 Оля Тележная :
>>>
>>> I think it's important to finish migrating process at first. I mean,
>>> now we are preparing and collecting everything in ref-filter, but we
>>> make resulting string and print still in cat-file. And I am not sure,
>>> but maybe it will not be possible to start using new atoms in cat-file
>>> while some part of logic still differs.
>>
>> I tried to make that part here:
>> https://github.com/telezhnaya/git/commit/19a148614f1d4db1f8e628eb4e6d7c819d2da875
>> I know that the code is disgusting and there is a memory leak :) I
>> just try to reuse ref-filter logic, I will cleanup everything later.
>> At first, I try to make it work.
>> The problem is that I have segfault, and if I use gdb, I get:
>>
>> Program received signal SIGSEGV, Segmentation fault.
>> 0x in ?? ()
>
> Make sure that you compile with debug options like -g3. For example I use:
>
> $ make -j 4 DEVELOPER=1 CFLAGS="-g3"

Is it OK that I get different test results with simple make and with
make with all that flags?
Have a code: https://github.com/telezhnaya/git/commits/catfile
I do:

olya@ubuntu17-vm:~/git$ make install
olya@ubuntu17-vm:~/git$ cd t
olya@ubuntu17-vm:~/git/t$ ./t1006-cat-file.sh

And I have 17 tests broken.
Then, without any changes in code, I do:

olya@ubuntu17-vm:~/git$ make -j 4 DEVELOPER=1 CFLAGS="-g3" install
olya@ubuntu17-vm:~/git$ cd t
olya@ubuntu17-vm:~/git/t$ ./t1006-cat-file.sh

And there is 42 tests broken.
And it's really hard to search for errors in such situation.

>
>> I tried to google it, it's my first time when I get that strange
>> message, and unfortunately find nothing. So please explain me the
>> reason, why I can't find a place of segfault that way.
>
> I get the following:
>
> (gdb) run cat-file --batch < myarg.txt
> Starting program: /home/ubuntu/bin/git cat-file --batch < myarg.txt
> [Thread debugging using libthread_db enabled]
> Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
>
> Program received signal SIGSEGV, Segmentation fault.
> 0x556ea7cf in format_ref_array_item (info=0x7fffd460,
> format=0x7fffd6e0,
> final_buf=0x7fffd410) at ref-filter.c:2234
> 2234atomv->handler(atomv, );
> (gdb) bt
> #0  0x556ea7cf in format_ref_array_item (info=0x7fffd460,
> format=0x7fffd6e0, final_buf=0x7fffd410) at ref-filter.c:2234
> #1  0x556ea91c in show_ref_array_item (info=0x7fffd460,
> format=0x7fffd6e0)
> at ref-filter.c:2256
> #2  0x55577ef7 in batch_object_write (
> obj_name=0x55a66770 "5e1c309dae7f45e0f39b1bf3ac3cd9db12e7d689",
> opt=0x7fffd6e0, data=0x7fffd5e0) at builtin/cat-file.c:298

Re: [PATCH v2 03/18] ref-filter: make valid_atom as function parameter

[Christian Couder]

On Thu, Jan 18, 2018 at 12:49 PM, Оля Тележная  wrote:
> 2018-01-18 9:20 GMT+03:00 Оля Тележная :
>>
>> I think it's important to finish migrating process at first. I mean,
>> now we are preparing and collecting everything in ref-filter, but we
>> make resulting string and print still in cat-file. And I am not sure,
>> but maybe it will not be possible to start using new atoms in cat-file
>> while some part of logic still differs.
>
> I tried to make that part here:
> https://github.com/telezhnaya/git/commit/19a148614f1d4db1f8e628eb4e6d7c819d2da875
> I know that the code is disgusting and there is a memory leak :) I
> just try to reuse ref-filter logic, I will cleanup everything later.
> At first, I try to make it work.
> The problem is that I have segfault, and if I use gdb, I get:
>
> Program received signal SIGSEGV, Segmentation fault.
> 0x in ?? ()

Make sure that you compile with debug options like -g3. For example I use:

$ make -j 4 DEVELOPER=1 CFLAGS="-g3"

> I tried to google it, it's my first time when I get that strange
> message, and unfortunately find nothing. So please explain me the
> reason, why I can't find a place of segfault that way.

I get the following:

(gdb) run cat-file --batch < myarg.txt
Starting program: /home/ubuntu/bin/git cat-file --batch < myarg.txt
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".

Program received signal SIGSEGV, Segmentation fault.
0x556ea7cf in format_ref_array_item (info=0x7fffd460,
format=0x7fffd6e0,
final_buf=0x7fffd410) at ref-filter.c:2234
2234atomv->handler(atomv, );
(gdb) bt
#0  0x556ea7cf in format_ref_array_item (info=0x7fffd460,
format=0x7fffd6e0, final_buf=0x7fffd410) at ref-filter.c:2234
#1  0x556ea91c in show_ref_array_item (info=0x7fffd460,
format=0x7fffd6e0)
at ref-filter.c:2256
#2  0x55577ef7 in batch_object_write (
obj_name=0x55a66770 "5e1c309dae7f45e0f39b1bf3ac3cd9db12e7d689",
opt=0x7fffd6e0, data=0x7fffd5e0) at builtin/cat-file.c:298

Re: [PATCH v2 03/18] ref-filter: make valid_atom as function parameter

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

2018-01-18 9:20 GMT+03:00 Оля Тележная :
> 2018-01-18 1:39 GMT+03:00 Christian Couder :
>> On Wed, Jan 17, 2018 at 10:43 PM, Jeff King  wrote:
>>> On Tue, Jan 16, 2018 at 09:55:22AM +0300, Оля Тележная wrote:
>>>
 > IOW, the progression I'd expect in a series like this is:
 >
 >   1. Teach ref-filter.c to support everything that cat-file can do.
 >
 >   2. Convert cat-file to use ref-filter.c.

 I agree, I even made this and it's working fine:
 https://github.com/git/git/pull/450/commits/1b74f1047f07434dccb207534d1ad45a143e3f2b
>>
>> (Nit: it looks like the above link does not work any more, but it
>> seems that you are talking about the last patch on the catfile
>> branch.)
>>
 But I decided not to add that to patch because I expand the
 functionality of several commands (not only cat-file and
 for-each-ref), and I need to support all new functionality in a proper
 way, make these error messages, test everything and - the hardest one
 - support many new commands for cat-file. As I understand, it is not
 possible unless we finally move to ref-filter and print results also
 there. Oh, and I also need to rewrite docs in that case. And I decided
 to apply this in another patch. But, please, say your opinion, maybe
 we could do that here in some way.
>>>
>>> Yeah, I agree that it will cause changes to other users of ref-filter,
>>> and you'd need to deal with documentation and tests there. But I think
>>> we're going to have to do those things eventually (since supporting
>>> those extra fields everywhere is part of the point of the project). And
>>> by doing them now, I think it can make the transition for cat-file a lot
>>> simpler, because we never have to puzzle over this intermediate state
>>> where only some of the atoms are valid for cat-file.
>>
>> I agree that you will have to deal with documentation and tests at one
>> point and that it could be a good idea to do that now.
>>
>> I wonder if it is possible to add atoms one by one into ref-filter and
>> to add tests and documentation at the same time, instead of merging
>> cat-file atoms with ref-filter atoms in one big step.
>>
>> When all the cat-file atoms have been added to ref-filter's
>> valid_atom, maybe you could add ref-filter's atoms to cat-file's
>> valid_cat_file_atom one by one and add tests and documentation at the
>> same time.
>>
>> And then when ref-filter's valid_atom and cat-file's
>> valid_cat_file_atom are identical you can remove cat-file's
>> valid_cat_file_atom and maybe after that rename "ref-filter" to
>> "format".
>
> I think it's important to finish migrating process at first. I mean,
> now we are preparing and collecting everything in ref-filter, but we
> make resulting string and print still in cat-file. And I am not sure,
> but maybe it will not be possible to start using new atoms in cat-file
> while some part of logic still differs.

I tried to make that part here:
https://github.com/telezhnaya/git/commit/19a148614f1d4db1f8e628eb4e6d7c819d2da875
I know that the code is disgusting and there is a memory leak :) I
just try to reuse ref-filter logic, I will cleanup everything later.
At first, I try to make it work.
The problem is that I have segfault, and if I use gdb, I get:

Program received signal SIGSEGV, Segmentation fault.
0x in ?? ()

I tried to google it, it's my first time when I get that strange
message, and unfortunately find nothing. So please explain me the
reason, why I can't find a place of segfault that way.
Thanks!

> And another thoughts here - we were thinking about creating format.h
> but decided not to move forward with it, and now we are suffering
> because of it. Can I create it right now or the history of commits
> would be too dirty because of it? Also, do you mean just renaming of
> ref-filter? I was thinking that I need to put formatting-related logic
> to another file and leave all other stuff in ref-filter.

By the way, I can create format.h in absolutely another branch, we
could merge it, and I will deal with all merge conflicts with my
current work. It sounds tediously, but actually it's not such a big
problem, I can do that.
But I still need to fully understand, what do you find more proper -
just rename ref-filter, or create new file and move formatting-related
logic.

>
> Anyway, your suggested steps looks good, and I am planning to
> implement them later. Let's discuss, what behavior we are waiting for
> when atom seems useless for the command. Die or ignore? And, which
> atoms are useless (as I understand, "rest" and "deltabase" from
> cat-file are useless for all ref-filter users, so the question is
> about - am I right in it, and about ref-filter atoms for cat-file).
>
> I have never written any tests and docs for Git, I will try to explore
> by myself how to do that, but if you have any special links/materials
> about it - please send 

Re: [PATCH v2 03/18] ref-filter: make valid_atom as function parameter

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

2018-01-18 1:39 GMT+03:00 Christian Couder :
> On Wed, Jan 17, 2018 at 10:43 PM, Jeff King  wrote:
>> On Tue, Jan 16, 2018 at 09:55:22AM +0300, Оля Тележная wrote:
>>
>>> > IOW, the progression I'd expect in a series like this is:
>>> >
>>> >   1. Teach ref-filter.c to support everything that cat-file can do.
>>> >
>>> >   2. Convert cat-file to use ref-filter.c.
>>>
>>> I agree, I even made this and it's working fine:
>>> https://github.com/git/git/pull/450/commits/1b74f1047f07434dccb207534d1ad45a143e3f2b
>
> (Nit: it looks like the above link does not work any more, but it
> seems that you are talking about the last patch on the catfile
> branch.)
>
>>> But I decided not to add that to patch because I expand the
>>> functionality of several commands (not only cat-file and
>>> for-each-ref), and I need to support all new functionality in a proper
>>> way, make these error messages, test everything and - the hardest one
>>> - support many new commands for cat-file. As I understand, it is not
>>> possible unless we finally move to ref-filter and print results also
>>> there. Oh, and I also need to rewrite docs in that case. And I decided
>>> to apply this in another patch. But, please, say your opinion, maybe
>>> we could do that here in some way.
>>
>> Yeah, I agree that it will cause changes to other users of ref-filter,
>> and you'd need to deal with documentation and tests there. But I think
>> we're going to have to do those things eventually (since supporting
>> those extra fields everywhere is part of the point of the project). And
>> by doing them now, I think it can make the transition for cat-file a lot
>> simpler, because we never have to puzzle over this intermediate state
>> where only some of the atoms are valid for cat-file.
>
> I agree that you will have to deal with documentation and tests at one
> point and that it could be a good idea to do that now.
>
> I wonder if it is possible to add atoms one by one into ref-filter and
> to add tests and documentation at the same time, instead of merging
> cat-file atoms with ref-filter atoms in one big step.
>
> When all the cat-file atoms have been added to ref-filter's
> valid_atom, maybe you could add ref-filter's atoms to cat-file's
> valid_cat_file_atom one by one and add tests and documentation at the
> same time.
>
> And then when ref-filter's valid_atom and cat-file's
> valid_cat_file_atom are identical you can remove cat-file's
> valid_cat_file_atom and maybe after that rename "ref-filter" to
> "format".

I think it's important to finish migrating process at first. I mean,
now we are preparing and collecting everything in ref-filter, but we
make resulting string and print still in cat-file. And I am not sure,
but maybe it will not be possible to start using new atoms in cat-file
while some part of logic still differs.
And another thoughts here - we were thinking about creating format.h
but decided not to move forward with it, and now we are suffering
because of it. Can I create it right now or the history of commits
would be too dirty because of it? Also, do you mean just renaming of
ref-filter? I was thinking that I need to put formatting-related logic
to another file and leave all other stuff in ref-filter.

Anyway, your suggested steps looks good, and I am planning to
implement them later. Let's discuss, what behavior we are waiting for
when atom seems useless for the command. Die or ignore? And, which
atoms are useless (as I understand, "rest" and "deltabase" from
cat-file are useless for all ref-filter users, so the question is
about - am I right in it, and about ref-filter atoms for cat-file).

I have never written any tests and docs for Git, I will try to explore
by myself how to do that, but if you have any special links/materials
about it - please send them to me :)

Olga

Re: [PATCH v2 03/18] ref-filter: make valid_atom as function parameter

[Christian Couder]

On Wed, Jan 17, 2018 at 10:43 PM, Jeff King  wrote:
> On Tue, Jan 16, 2018 at 09:55:22AM +0300, Оля Тележная wrote:
>
>> > IOW, the progression I'd expect in a series like this is:
>> >
>> >   1. Teach ref-filter.c to support everything that cat-file can do.
>> >
>> >   2. Convert cat-file to use ref-filter.c.
>>
>> I agree, I even made this and it's working fine:
>> https://github.com/git/git/pull/450/commits/1b74f1047f07434dccb207534d1ad45a143e3f2b

(Nit: it looks like the above link does not work any more, but it
seems that you are talking about the last patch on the catfile
branch.)

>> But I decided not to add that to patch because I expand the
>> functionality of several commands (not only cat-file and
>> for-each-ref), and I need to support all new functionality in a proper
>> way, make these error messages, test everything and - the hardest one
>> - support many new commands for cat-file. As I understand, it is not
>> possible unless we finally move to ref-filter and print results also
>> there. Oh, and I also need to rewrite docs in that case. And I decided
>> to apply this in another patch. But, please, say your opinion, maybe
>> we could do that here in some way.
>
> Yeah, I agree that it will cause changes to other users of ref-filter,
> and you'd need to deal with documentation and tests there. But I think
> we're going to have to do those things eventually (since supporting
> those extra fields everywhere is part of the point of the project). And
> by doing them now, I think it can make the transition for cat-file a lot
> simpler, because we never have to puzzle over this intermediate state
> where only some of the atoms are valid for cat-file.

I agree that you will have to deal with documentation and tests at one
point and that it could be a good idea to do that now.

I wonder if it is possible to add atoms one by one into ref-filter and
to add tests and documentation at the same time, instead of merging
cat-file atoms with ref-filter atoms in one big step.

When all the cat-file atoms have been added to ref-filter's
valid_atom, maybe you could add ref-filter's atoms to cat-file's
valid_cat_file_atom one by one and add tests and documentation at the
same time.

And then when ref-filter's valid_atom and cat-file's
valid_cat_file_atom are identical you can remove cat-file's
valid_cat_file_atom and maybe after that rename "ref-filter" to
"format".

Re: [PATCH v2 03/18] ref-filter: make valid_atom as function parameter

[Jeff King]

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

> > IOW, the progression I'd expect in a series like this is:
> >
> >   1. Teach ref-filter.c to support everything that cat-file can do.
> >
> >   2. Convert cat-file to use ref-filter.c.
> 
> I agree, I even made this and it's working fine:
> https://github.com/git/git/pull/450/commits/1b74f1047f07434dccb207534d1ad45a143e3f2b
> But I decided not to add that to patch because I expand the
> functionality of several commands (not only cat-file and
> for-each-ref), and I need to support all new functionality in a proper
> way, make these error messages, test everything and - the hardest one
> - support many new commands for cat-file. As I understand, it is not
> possible unless we finally move to ref-filter and print results also
> there. Oh, and I also need to rewrite docs in that case. And I decided
> to apply this in another patch. But, please, say your opinion, maybe
> we could do that here in some way.

Yeah, I agree that it will cause changes to other users of ref-filter,
and you'd need to deal with documentation and tests there. But I think
we're going to have to do those things eventually (since supporting
those extra fields everywhere is part of the point of the project). And
by doing them now, I think it can make the transition for cat-file a lot
simpler, because we never have to puzzle over this intermediate state
where only some of the atoms are valid for cat-file.

I also agree that moving cat-file's printing over to ref-filter.c is
going to introduce a lot of corner cases (like the behavior when
somebody does "cat-file --batch-check=%(refname)" with a bare sha1). But
I think a progression of patches handling those cases would be pretty
easy to follow. E.g., I'd expect to see a patch that teaches
ref-filter[1] how to handle callers that don't have a ref, but just a
bare object name. And it would be easier to evaluate that on its own,
because we know that's an end state we're going to have to handle, and
not some funny state created as part of the transition.

-Peff

[1] And here's where we might start to think about calling it something
besides ref-filter, if we don't necessarily have a ref. :)

Re: [PATCH v2 03/18] ref-filter: make valid_atom as function parameter

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

2018-01-16 0:42 GMT+03:00 Jeff King :
> On Wed, Jan 10, 2018 at 09:36:41AM +, Olga Telezhnaya wrote:
>
>> Make valid_atom as a function parameter,
>> there could be another variable further.
>> Need that for further reusing of formatting logic in cat-file.c.
>>
>> We do not need to allow users to pass their own valid_atom variable in
>> global functions like verify_ref_format because in the end we want to
>> have same set of valid atoms for all commands. But, as a first step
>> of migrating, I create further another version of valid_atom
>> for cat-file.
>
> I agree in the end we'd want a single valid_atom list. It doesn't look
> like we hit that end state in this series, though.
>
> I guess I'm not quite clear on why we're not adding these new atoms to
> ref-filter (and for-each-ref) right away, though. We already have the
> first three (name, type, and size), and we'd just need to support
> %(rest) and %(deltabase).
>
> I think %(rest) doesn't really make sense for for-each-ref (we're not
> reading any input), but it could expand to the empty string by default
> (or even throw an error if the caller asks us not to support it).
>
> IOW, the progression I'd expect in a series like this is:
>
>   1. Teach ref-filter.c to support everything that cat-file can do.
>
>   2. Convert cat-file to use ref-filter.c.
>
> -Peff

I agree, I even made this and it's working fine:
https://github.com/git/git/pull/450/commits/1b74f1047f07434dccb207534d1ad45a143e3f2b
But I decided not to add that to patch because I expand the
functionality of several commands (not only cat-file and
for-each-ref), and I need to support all new functionality in a proper
way, make these error messages, test everything and - the hardest one
- support many new commands for cat-file. As I understand, it is not
possible unless we finally move to ref-filter and print results also
there. Oh, and I also need to rewrite docs in that case. And I decided
to apply this in another patch. But, please, say your opinion, maybe
we could do that here in some way.

Olga

Re: [PATCH v2 03/18] ref-filter: make valid_atom as function parameter

[Jeff King]

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

> Make valid_atom as a function parameter,
> there could be another variable further.
> Need that for further reusing of formatting logic in cat-file.c.
> 
> We do not need to allow users to pass their own valid_atom variable in
> global functions like verify_ref_format because in the end we want to
> have same set of valid atoms for all commands. But, as a first step
> of migrating, I create further another version of valid_atom
> for cat-file.

I agree in the end we'd want a single valid_atom list. It doesn't look
like we hit that end state in this series, though.

I guess I'm not quite clear on why we're not adding these new atoms to
ref-filter (and for-each-ref) right away, though. We already have the
first three (name, type, and size), and we'd just need to support
%(rest) and %(deltabase).

I think %(rest) doesn't really make sense for for-each-ref (we're not
reading any input), but it could expand to the empty string by default
(or even throw an error if the caller asks us not to support it).

IOW, the progression I'd expect in a series like this is:

  1. Teach ref-filter.c to support everything that cat-file can do.

  2. Convert cat-file to use ref-filter.c.

-Peff