Re: [PATCH v3 05/11] ref-filter: add parse_opt_merge_filter()

From: karthik nayak <karthik.188@gmail.com>
Date: Jun 17, 2015, 19:12 UTC
Message-ID: <CAOLa=ZQ-4VGunAAjrN6KPgA5hEg8JFhCrb-pobGitydeB9pNLg@mail.gmail.com>
In-Reply-To: <vpqzj3yzqma.fsf@anie.imag.fr>

On Wed, Jun 17, 2015 at 1:57 PM, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:

Show 18 quoted lines

> Err, for-each-ref already uses it before this series, no?
>
> So, you don't need any extra option to get for-each-ref, because it is
> already there. Having these extra options is a good side effect, though.
>
> To make sure I'm clear enough, what you're doing is
>
> - add all options to for-each-ref
> - port tag.c
> - port branch.c
>
> What I'm suggesting is to prioritize this way
>
> - add all options required for tag.c
> - port tag.c
> - add all options required for branch.c
> - port branch.c
>

I meant somewhat on those lines only.  Let me clear that out.
The steps I plan to take are:
1. Move code from for-each-ref to ref-filter.
2. Add options to ref-filter which is available in tag.c and branch.c
(--points-at, --contains, --merged)
3. Add there options to for-each-ref.
4. Add options required for functioning of tag.c alone to ref-filter
5. Port tag.c
6. Add options required for functioning of branch.c alone to ref-filter
7. Port branch.c

Now why i want to complete step 2 right after 1 is so that while porting tag.c and branch.c I do not want to focus on making those common options available. Because I rather work on getting their specific options (verbose in branch.c, -n in tag.c and so on) working before porting over tag.c/branch.c.

Show 13 quoted lines

>
> Not only the challenge, but also the way to validate your work. Think of
> it as a rather comprehensive set of tests that you get for free once you
> ported a command.
>
> BTW, talking about tests, did you do some coverage analysis on git
> branch and git tag? If not, I'd suggest that you do this to make sure
> that the pieces of code you're rewritting using ref-filter are well
> tested before being rewritten (a bit like Paul's work on shell -> C).
> You don't have to actually do this before porting, but it should come
> befor the port in the patch series to make sure that tests pass both the
> old and new implementation.
>

Yes good point.

I did not do a deep coverage analysis on git tag and its tests. You are right this would be a crucial step for porting. I had a glance over the tests. Will look into it. As for git branch I'll do that after porting tag.c over to using ref-filter.

-- 
Regards,
Karthik Nayak