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

From: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
To: karthik nayak <karthik.188@gmail.com>
Cc: Git <git@vger.kernel.org>, Christian Couder <christian.couder@gmail.com>
Subject: Re: [PATCH v3 05/11] ref-filter: add parse_opt_merge_filter()
Date: Wed, 17 Jun 2015 10:27:41 +0200	[thread overview]
Message-ID: <vpqzj3yzqma.fsf@anie.imag.fr> (raw)
In-Reply-To: <CAOLa=ZS_gY-MuGJ86TFpHhMXNYJ1gHWQWyfv71gDCVm1sMXZNw@mail.gmail.com> (karthik nayak's message of "Tue, 16 Jun 2015 22:00:20 +0530")

karthik nayak <karthik.188@gmail.com> writes:

> On Tue, Jun 16, 2015 at 9:48 PM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>
>>> This is copied from 'builtin/branch.c' which will eventually be removed
>>> when we port 'branch.c' to use ref-filter APIs.
>>
>> Earlier in the series you took code from tag.c.
>>
>> I think you should focus on either merge or tag, get a ref-filter-based
>> replacement that passes the tests for it, and then consider the other.
>> The fact that the test pass for a rewritten command is important to
>> check the correctness of the these patches.
>>
>> I'm not asking you to remove commits from this series though. Just
>> impatient to see one command fully replaced (actually, I see that you
>> have more commits than you sent in your branch, so I guess it will come
>> soon on the list) :-).
>>
>
> The idea is to currently get ref-filter to support all options and port it over
> to for-each-ref which would be the first command to completely use ref-filter.

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

> And like you said, the challenge is to then ensure tag.c and branch.c to use
> ref-filter and make them pass all tests.

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.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/