Re: [PATCH 2/4] ref-filter: add ref-filter API
- From
Matthieu Moy <matthieu.moy@grenoble-inp.fr>
- Date
- May 21, 2015, 08:47 UTC
- Message-ID
- <vpqr3qagvv6.fsf@anie.imag.fr>
- In-Reply-To
- <1432127904-21070-2-git-send-email-karthik.188@gmail.com>
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.
Show 5 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 12 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.
> + * at unification of these three commands so that they ay benefit from
they *may*?
> + * 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.
> +/* 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.
> +/* 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 /*)
-- Matthieu Moy http://www-verimag.imag.fr/~moy/