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

2013-01-16 Thread Adam Spiers 
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 g...@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;
const char *qname;
char *seen = NULL;
struct option options[] = {
@@ -97,10 +98,9 @@ int cmd_clean(int argc, const char **argv, const char 
*prefix)
if (!ignored)
setup_standard_excludes(dir);
 
-   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));
 
pathspec = get_pathspec(prefix, argv);
 
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index fa9ccb8..b4d8b01 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -421,10 +421,10 @@ static int option_parse_z(const struct option *opt,
 static int option_parse_exclude(const struct option *opt,
const char *arg, int unset)
 {
-   struct exclude_list_group *group = opt-value;
+   struct string_list *exclude_list = opt-value;
 
exc_given = 1;
-   add_exclude(arg, , 0, group-el[0], --exclude_args);
+   string_list_append(exclude_list, arg);
 
return 0;
 }
@@ -453,9 +453,11 @@ static int option_parse_exclude_standard(const struct 
option *opt,
 
 int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 {
-   int require_work_tree = 0, show_tag = 0;
+   int require_work_tree = 0, show_tag = 0, i;
const char *max_prefix;
struct dir_struct dir;
+   struct exclude_list *el;
+   struct string_list exclude_list = STRING_LIST_INIT_NODUP;
struct option builtin_ls_files_options[] = {
{ OPTION_CALLBACK, 'z', NULL, NULL, NULL,
paths are separated with NUL character,
@@ -490,7 +492,7 @@ int cmd_ls_files(int argc, const char **argv, const char 
*cmd_prefix)
OPT_BOOLEAN(0, resolve-undo, show_resolve_undo,
show resolve-undo information),
{ OPTION_CALLBACK, 'x', exclude,
-   dir.exclude_list_group[EXC_CMDL], pattern,
+   exclude_list, pattern,
skip files matching pattern,
0, option_parse_exclude },
{ OPTION_CALLBACK, 'X', exclude-from, dir, file,
@@ -525,9 +527,12 @@ int cmd_ls_files(int argc, const char **argv, const char 
*cmd_prefix)
if (read_cache()  0)
die(index file corrupt);
 
-   add_exclude_list(dir, EXC_CMDL, --exclude option);
argc = parse_options(argc, argv, prefix, builtin_ls_files_options,
ls_files_usage, 0);
+   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, el, 
--exclude_args);
+   }
if (show_tag || show_valid_bit) {
tag_cached = H ;
tag_unmerged = M ;
-- 
1.8.1.291.g0730ed6

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-01-16 Thread Junio C Hamano 
Adam Spiers g...@adamspiers.org writes:

 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].

Sounds sensible.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-01-16 Thread Junio C Hamano 
Adam Spiers g...@adamspiers.org writes:

 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 g...@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.

 - 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.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html