Re: [PATCH 2/4] ref-filter: add ref-filter API

[karthik nayak <karthik.188@gmail.com>]

On 05/21/2015 02:17 PM, Matthieu Moy wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> +static int match_name_as_path(const char **pattern, const char *refname)
>
> I would have appreciated a short docstring. The full doc would probably
> be as long as the code, but a few examples of what matches and what
> doesn't can help the reader.
>

Will patch with an explanation and some examples.

>> +static struct ref_filter_item *new_ref_filter_item(const char *refname,
>> +						   const unsigned char *sha1,
>> +						   int flag)
>> +{
>> +	struct ref_filter_item *ref =  xcalloc(1, sizeof(struct ref_filter_item));
>
> double-space after =.

Noted.

>
>> +++ b/ref-filter.h
>> @@ -0,0 +1,47 @@
>> +#ifndef REF_FILTER_H
>> +#define REF_FILTER_H
>> +
>> +#include "sha1-array.h"
>> +#include "refs.h"
>> +#include "commit.h"
>> +
>> +/*
>> + * ref-filter is meant to act as a common provider of API's for
>> + * 'tag -l', 'branch -l' and 'for-each-ref'. ref-filter is the attempt
>
> Don't be shy: attempt at unification -> unification. This message may be
> an attempt, but we'll polish it until it is more than that.

Haha, OK, will change that.

>
>> + * at unification of these three commands so that they ay benefit from
>
> they *may*?

Yes. Will change

>
>> + * the functionality of each other.
>> + */
>
> I miss a high-level description of what the code is doing. Essentially,
> there's the complete repository list of refs, and you want to filter
> only some of them, right?
>
>  From the name, I would guess that ref_filter is the structure describing
> how you are filtering, but from the code it seems to be the list you're
> filtering, not the filter.

Will write a better explanation and description.
>
>> +/* An atom is a valid field atom used for sorting and formatting of refs.*/
>
> "used for" is very vague. Be more precise, say how it will be involved
> in sorting & formatting.

Noted.

>
>> +/*  ref_filter will hold data pertaining to a list of refs. */
>
> This is the answer to the "what?" question, which is not very hard to
> infer from the code. That's not anwsering "what for?" or "why?", which
> are much harder to infer for the reader.
>
> (plus you have a double-space after /*)
>

Noted! Thanks for the suggestions :)