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/

On Wed, Jun 17, 2015 at 1:57 PM, Matthieu Moy
[off-list ref] wrote:

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.

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