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

karthik nayak [off-list ref] writes:

On Tue, Jun 16, 2015 at 9:48 PM, Matthieu Moy
[off-list ref] wrote:

quoted

Karthik Nayak [off-list ref] writes:

quoted

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/