Junio C Hamano <gitster@xxxxxxxxx> writes:
> Adam Spiers <git@xxxxxxxxxxxxxx> writes:
>
>> diff --git a/builtin/clean.c b/builtin/clean.c
>> index 0c7b3d0..bd18b88 100644
>> --- a/builtin/clean.c
>> +++ b/builtin/clean.c
>> @@ -97,9 +97,10 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
>> if (!ignored)
>> setup_standard_excludes(&dir);
>>
>> + add_exclude_list(&dir, EXC_CMDL);
>> for (i = 0; i < exclude_list.nr; i++)
>> add_exclude(exclude_list.items[i].string, "", 0,
>> - &dir.exclude_list[EXC_CMDL]);
>> + &dir.exclude_list_groups[EXC_CMDL].ary[0]);
>
> This looks somewhat ugly for two reasons.
>
> * The abstraction add_exclude() offers to its callers is just to
> let them add one pattern to the list of patterns for the kind
> (here, EXC_CMDL); why should they care about .ary[0] part? Are
> there cases any sane caller (not the implementation of the
> exclude_list_group machinery e.g. add_excludes_from_... function)
> may want to call it with .ary[1]? I do not think of any.
> Shouldn't the public API function add_exclude() take a pointer to
> the list group itself?
>
> * When naming an array of things, we tend to prefer naming it
>
> type thing[count]
>
> so that the second element can be called "thing[2]" and not
> "things[2]". dir.exclude_list_group[EXC_CMDL] reads better.
Also, "ary[]" is a bad name, even as an implementation detail, for
two reasons: it is naming it after its type (being an "array") not
after what it is (if it holds the patterns from the same information
source, e.g. file, togeter, "src" might be a better name), and it
uses rather unusual abbreviation (I haven't seen "array" shortened
to "ary" anywhere else).
>
>> diff --git a/builtin/ls-files.c b/builtin/ls-files.c
>> index ef7f99a..c448e06 100644
>> --- a/builtin/ls-files.c
>> +++ b/builtin/ls-files.c
>> @@ -420,10 +420,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 *list = opt->value;
>> + struct exclude_list_group *group = opt->value;
>>
>> exc_given = 1;
>> - add_exclude(arg, "", 0, list);
>> + add_exclude(arg, "", 0, &group->ary[0]);
>
> This is another example where the caller would wish to be able to say
>
> add_exclude(arg, "", 0, group);
>
> instead.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html