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

[Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>]

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.

> +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 =.

> +++ 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/