Re: [PATCH v4 05/19] ref-filter: add parse_opt_merge_filter()

From: Karthik Nayak <karthik.188@gmail.com>
Date: Jun 22, 2015, 19:00 UTC
Message-ID: <CAOLa=ZR9=2RUhtN=mC2u78vin0hGcmD8fL450ygAX6N9A4Yq8A@mail.gmail.com>
In-Reply-To: <CAPc5daUkHb5sQ_d-g0EeFwXTkFUf4=PVV=QcV-nekObKSJL27g@mail.gmail.com>

On Mon, Jun 22, 2015 at 6:25 AM, Junio C Hamano <gitster@pobox.com> wrote:

>
> Why SHOUT here?
>

Just used to typing "macros" in caps. Will change!

Show 11 quoted lines

>> This is copied from 'builtin/branch.c' which will eventually be removed
>> when we port 'branch.c' to use ref-filter APIs.
>
> Hmph. I somehow thought Matthieu's instruction was to finish tag.c
> side first and then
> do branch (i.e. with 3 and 4 you brought things from tag to
> for-each-ref, now it is a time
> to rewrite tag by using what you wrote for for-each-ref with 3 and 4,
> before moving to
> this patch)? Was that plan scrapped or found inappropriate or something?
>

> I would call in "advice" rather than "instruction". I still think we
> should prioritize the tag.c side, but if this patch is ready, it makes
> sense to keep it in the series.

Yes, Matthieu did advice that. But I had already started working on this. But if you guys think thats a better option I can do that also, as I already have tag.c ported over to use ref-filter on my local branch. But that'll include a lot of changes.

Also I found this more systematic as we will have a complete ref-filter library ready and only porting of tag.c and branch.c would be left.

Show 5 quoted lines

>
> When does this trigger? You have lastarg-default with "HEAD", and I am
> having trouble guessing when "arg" upon entry to this function can ever
> be NULL.
>

This is redundant. will remove.

Show 6 quoted lines

>
> Can --merged (or --no-merged) be given more than once? Is the rule
> "the last one wins"?
> Does the old value of rf->merge_commit leak (no, it does not, but I am
> just asking for
> completeness)?

Yes! currently for e.g. `git for-each-ref --merged master --merged ref_filter` would mean that ref_filter would be considered for the --merge option.

"Does the old value of rf->merge_commit leak" From my understanding of object.c/commit.c, It just points to an object in the obj_hash array, so when multiple options are given the pointer just points to another part of obj_hash, I'm sure there's more to it. This is what I gathered from an over the top view.

-- 
Regards,
Karthik Nayak