Re: [PATCH 2/4] ref-filter: add ref-filter API
On 05/21/2015 02:17 PM, Matthieu Moy wrote:
Show 8 quoted lines
> 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.
Show 7 quoted lines
>> +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 =.Show 16 quoted lines
>
>> +++ 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*?
Show 11 quoted lines
>
>> + * 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.
Show 5 quoted lines
>
>> +/* 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.
Show 9 quoted lines
>
>> +/* 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 :)
karthik nayak <karthik.188@gmail.com>