Re: [PATCH v2 12/14] Extract some useful pathspec handling code from builtin/add.c into a library

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Sep 21, 2012, 07:54 UTC
Message-ID: <505C1D27.7080100@alum.mit.edu>
In-Reply-To: <1348170383-15751-13-git-send-email-git@adamspiers.org>

On 09/20/2012 09:46 PM, Adam Spiers wrote:

Show 25 quoted lines

> This is in preparation for reuse by a new git check-ignore command.
> 
> Signed-off-by: Adam Spiers <git@adamspiers.org>
> ---
>  Makefile      |  2 ++
>  builtin/add.c | 95 ++-------------------------------------------------------
>  pathspec.c    | 97 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  pathspec.h    |  6 ++++
>  4 files changed, 108 insertions(+), 92 deletions(-)
>  create mode 100644 pathspec.c
>  create mode 100644 pathspec.h
> 
> [...]
> diff --git a/pathspec.h b/pathspec.h
> new file mode 100644
> index 0000000..4ed40a5
> --- /dev/null
> +++ b/pathspec.h
> @@ -0,0 +1,6 @@
> +extern void validate_path(const char *prefix, const char *path);
> +extern const char **validate_pathspec(const char *prefix, const char **files);
> +extern char *find_used_pathspec(const char **pathspec);
> +extern void fill_pathspec_matches(const char **pathspec, char *seen, int specs);
> +extern const char *treat_gitlink(const char **path);
> +extern void treat_gitlinks(const char **pathspec);

It was unfortunate that these functions were not already documented. But now that you are elevating them from static to global functions, it would be great if you would add some comments for them.

(By the way, thanks for the docstrings that you added in an earlier patch.)

This patch is not only moving code around, but also:

* extracting a new function, validate_path()

* changing the signature of validate_pathspec()

* maybe other things?  How is a reviewer to know without examining every
line of the patch?

Each self-contained change should be done in a separate patch. For example, one patch should move the code while making only the minimal changes logically connected to the move (e.g., removing "static" qualifiers). The other changes should be made separate commits.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/