Re: [PATCH] clean.c, ls-files.c: respect encapsulation of exclude_list_groups

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 16, 2013, 22:20 UTC
Message-ID: <7v1udkwz1f.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1358342758-30503-1-git-send-email-git@adamspiers.org>

Adam Spiers <git@adamspiers.org> writes:

Show 21 quoted lines

> Consumers of the dir.c traversal API should avoid assuming knowledge
> of the internal implementation of exclude_list_groups.  Therefore
> when adding items to an exclude list, it should be accessed via the
> pointer returned from add_exclude_list(), rather than by referencing
> a location within dir.exclude_list_groups[EXC_CMDL].
>
> Signed-off-by: Adam Spiers <git@adamspiers.org>
> ---
>  builtin/clean.c    |  6 +++---
>  builtin/ls-files.c | 15 ++++++++++-----
>  2 files changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/builtin/clean.c b/builtin/clean.c
> index b098288..b9cb7ad 100644
> --- a/builtin/clean.c
> +++ b/builtin/clean.c
> @@ -45,6 +45,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
>  	static const char **pathspec;
>  	struct strbuf buf = STRBUF_INIT;
>  	struct string_list exclude_list = STRING_LIST_INIT_NODUP;
> +	struct exclude_list *el;

When a type "exclude_list" exists and used in the same function, having a local variable of the same name but of a different type becomes a bit awkward.

builtin/ls-files.c shares the same structure. Does the file-scope "exclude_args" variable need to be a file-scope static over there? It seems that it is closely tied to the elements of the string list, so it may make sense to:

    * remove the file-scope static "exclude_args";

    * rename "exclude_list" string list variable to "exclude_args";
      and

    * replace "--exclude_args" in the loop that iterates over
      exclude_list (now exclude_args) with "-(i+1)" or something,
      just like you do in "builtin/clean.c" below.

Show 6 quoted lines

> -	add_exclude_list(&dir, EXC_CMDL, "--exclude option");
> +	el = add_exclude_list(&dir, EXC_CMDL, "--exclude option");
>  	for (i = 0; i < exclude_list.nr; i++)
> -		add_exclude(exclude_list.items[i].string, "", 0,
> -			    &dir.exclude_list_group[EXC_CMDL].el[0], -(i+1));
> +		add_exclude(exclude_list.items[i].string, "", 0, el, -(i+1));

We may want to use for_each_string_list_item() here and in the corresponding loop in builtin/ls-files.c, but because we do need to give the -(i + 1) label to each element, I think the code is OK as-is.