Please review my code

Please review my code

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

Hi everyone,
I haven't sent the code by mailing lists because 25 commits (every
commit in separate message) look like a spam.

Please look at my code:
https://github.com/telezhnaya/git/commits/catfile
You could send me any ideas here or in Github.

The main idea of the patch is to get rid of using custom formatting in
cat-file and start using general one from ref-filter.
Additional bonus is that cat-file becomes to support many new
formatting commands like %(if), %(color), %(committername) etc.

I remember I need to rewrite docs, I will do that in the near future.

I would be happy to get any ideas from you.
Thanks!
Olga

Re: Please review my code

[Christian Couder]

Hi Olga,

On Thu, Jan 25, 2018 at 6:20 PM, Оля Тележная <olyatelezhnaya@gmail.com> wrote:
> Hi everyone,
> I haven't sent the code by mailing lists because 25 commits (every
> commit in separate message) look like a spam.

Yeah, so now that you added tests, it might be interesting to see if
the patch series can be refactored to be shorter or to be clearer.

> Please look at my code:
> https://github.com/telezhnaya/git/commits/catfile
> You could send me any ideas here or in Github.

I left some comments on GitHub. My main suggestion is to try to get
rid of the is_cat global and if possible to remove the "struct
expand_data *cat_file_info" global.

> The main idea of the patch is to get rid of using custom formatting in
> cat-file and start using general one from ref-filter.
> Additional bonus is that cat-file becomes to support many new
> formatting commands like %(if), %(color), %(committername) etc.

Yeah, that is a really nice result.

> I remember I need to rewrite docs, I will do that in the near future.

Great, thanks,
Christian.

Re: Please review my code

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

2018-01-25 23:22 GMT+03:00 Christian Couder <christian.couder@gmail.com>:
> Hi Olga,
>
> On Thu, Jan 25, 2018 at 6:20 PM, Оля Тележная <olyatelezhnaya@gmail.com> wrote:
>> Hi everyone,
>> I haven't sent the code by mailing lists because 25 commits (every
>> commit in separate message) look like a spam.
>
> Yeah, so now that you added tests, it might be interesting to see if
> the patch series can be refactored to be shorter or to be clearer.
>
>> Please look at my code:
>> https://github.com/telezhnaya/git/commits/catfile
>> You could send me any ideas here or in Github.
>
> I left some comments on GitHub. My main suggestion is to try to get
> rid of the is_cat global and if possible to remove the "struct
> expand_data *cat_file_info" global.

Thanks for your comments, I find them very useful. Most of issues are
fixed except the main one, that you mentioned here :)
I left the comment on GitHub.
https://github.com/telezhnaya/git/commit/403ab584fbf543acef1ecdf279ce31f4fc2ced3f

Thanks again!
Olga

>
>> The main idea of the patch is to get rid of using custom formatting in
>> cat-file and start using general one from ref-filter.
>> Additional bonus is that cat-file becomes to support many new
>> formatting commands like %(if), %(color), %(committername) etc.
>
> Yeah, that is a really nice result.
>
>> I remember I need to rewrite docs, I will do that in the near future.
>
> Great, thanks,
> Christian.

Re: Please review my code

[Christian Couder]

On Fri, Jan 26, 2018 at 11:32 AM, Оля Тележная <olyatelezhnaya@gmail.com> wrote:
> 2018-01-25 23:22 GMT+03:00 Christian Couder <christian.couder@gmail.com>:
>> On Thu, Jan 25, 2018 at 6:20 PM, Оля Тележная <olyatelezhnaya@gmail.com> wrote:
>>> Please look at my code:
>>> https://github.com/telezhnaya/git/commits/catfile
>>> You could send me any ideas here or in Github.
>>
>> I left some comments on GitHub. My main suggestion is to try to get
>> rid of the is_cat global and if possible to remove the "struct
>> expand_data *cat_file_info" global.
>
> Thanks for your comments, I find them very useful. Most of issues are
> fixed except the main one, that you mentioned here :)

Ok, no problem, we will see what happens on the mailing list.

It looks like the for-each-ref documentation has not been changed though.

Otherwise it looks good to me and perhaps you could send your series
to the mailing list even if it's long. For the first version, you may
want to add "RFC" in the subject of the patch emails you send.

Thanks,

Re: Please review my code

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

2018-01-26 19:42 GMT+03:00 Christian Couder <christian.couder@gmail.com>:
> On Fri, Jan 26, 2018 at 11:32 AM, Оля Тележная <olyatelezhnaya@gmail.com> wrote:
>> 2018-01-25 23:22 GMT+03:00 Christian Couder <christian.couder@gmail.com>:
>>> On Thu, Jan 25, 2018 at 6:20 PM, Оля Тележная <olyatelezhnaya@gmail.com> wrote:
>>>> Please look at my code:
>>>> https://github.com/telezhnaya/git/commits/catfile
>>>> You could send me any ideas here or in Github.
>>>
>>> I left some comments on GitHub. My main suggestion is to try to get
>>> rid of the is_cat global and if possible to remove the "struct
>>> expand_data *cat_file_info" global.
>>
>> Thanks for your comments, I find them very useful. Most of issues are
>> fixed except the main one, that you mentioned here :)
>
> Ok, no problem, we will see what happens on the mailing list.
>
> It looks like the for-each-ref documentation has not been changed though.

Have you seen last commit? I updated cat-file documentation and I
mentioned why I decided not to touch for-each-ref docs. Please share
with me any ideas how can I make that place better.

>
> Otherwise it looks good to me and perhaps you could send your series
> to the mailing list even if it's long. For the first version, you may
> want to add "RFC" in the subject of the patch emails you send.

Great, thanks, I will send it now.
Olga

>
> Thanks,