threads / discuss / 47693

Please review my code

Subject: Please review my code

## tl;dr

5 messages between Jan 25, 2018 and Jan 26, 2018.

replies: 4people: 2as markdown or json

Оля Тележная· Jan 25, 2018, 17:20 UTC · lore

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

Christian Couder· Jan 25, 2018, 20:22 UTC · re: Оля Тележная · lore

Re: Please review my code

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.

Оля Тележная· Jan 26, 2018, 10:32 UTC · re: Christian Couder · lore

Re: Please review my code

2018-01-25 23:22 GMT+03:00 Christian Couder <christian.couder@gmail.com>:
Show 17 quoted lines
> 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

Show 12 quoted lines
>
>> 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.
Christian Couder· Jan 26, 2018, 16:42 UTC · re: Оля Тележная · lore

Re: Please review my code

On Fri, Jan 26, 2018 at 11:32 AM, Оля Тележная <olyatelezhnaya@gmail.com> wrote:
Show 12 quoted lines
> 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,
Оля Тележная· Jan 26, 2018, 19:34 UTC · re: Christian Couder · lore

Re: Please review my code

2018-01-26 19:42 GMT+03:00 Christian Couder <christian.couder@gmail.com>:
Show 17 quoted lines
> 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,

← back to recent threads