Re: [PATCH 01/10] ref-filter: add option to filter only branches

From: Karthik Nayak <karthik.188@gmail.com>
Date: Aug 14, 2015, 18:45 UTC
Message-ID: <CAOLa=ZTiVftC51e+dpq1=Ui38L5bUseq0M0+vMJ3O-Db9311XQ@mail.gmail.com>
In-Reply-To: <xmqq614hkgnd.fsf@gitster.dls.corp.google.com>

On Fri, Aug 14, 2015 at 9:26 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 37 quoted lines

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> On Thu, Aug 13, 2015 at 5:05 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>> On Thu, Aug 13, 2015 at 4:21 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>>>
>>>> This was taken from branch.c, I thought of using an enum instead but that
>>>> would again require most of branch.c, hence it's been carried over
>>>> without changing
>>>> I'm thinking of changing it, any suggestions?
>>>>
>>>
>>> What I was thinking was of having:
>>>
>>> #define FILTER_REFS_INCLUDE_BROKEN 0x1
>>> #define FILTER_REFS_TAGS 0x2
>>> #define FILTER_REFS_BRANCHES 0x4
>>> #define FILTER_REFS_REMOTES 0x8
>>> #define FILTER_REFS_DETACHED_HEAD 0x16
>>>
>>> and using these for showing ref kind also instead of separately
>>> having 'REF_DETACHED_HEAD' and so on.
>>>
>>
>> Something like this:
>> https://github.com/KarthikNayak/git/commit/0ec5381420dcdfe7c62000b56168e2842d5d0063
>
> I notice a few things in ref-filter.c in that commit (a web
> interface including GitHub one is horrible in showing the things in
> wider context across files, so I'll base my discussion by guessing
> what the caller of this function and helpers this function calls
> do):
>
>  - Your "ALL" silently overrides others.  Is that sensible?  Perhaps
>    you would instead want to define FILTER_REFS_OTHER (not needed to
>    be exposed to UI) and then define FILTER_REFS_ALL as the ORed
>    value of FILTER_REFS_{BRANCHES,...,OTHER}?
>

Well okay I could something on those lines.

>  - When the caller asks for "--branches --tags", you run
>    ref-filter-handler twice on ref_cbdata.  Does that make sense?
>    Shouldn't you iterate over all the available refs just once,
>    rejecting ones that aren't in either refs/{heads,tags}/ instead?

I was under the idea that since we're dealing with do_for_each_entry() eventually and in that we set the loose_dir value based on the given 'base' hence when the caller asks for something like "--branches --tags", it would be better to just iterate through the refs in the directory of "--branches --tags" rather than go through the whole list of refs and drop ones which don't belong to "--branches --tags". but this was an over the top look at how do_for_each_entry() works, I could be totally off the mark.

-- 
Regards,
Karthik Nayak