Re: [PATCH] clean.c, ls-files.c: respect encapsulation of exclude_list_groups
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:55:48
Adam Spiers [off-list ref] writes:
quoted hunk
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 <redacted> --- 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.
- 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.