On Tue, Jan 1, 2013 at 8:52 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Adam Spiers <git@xxxxxxxxxxxxxx> writes:
>
>> diff --git a/Documentation/technical/api-directory-listing.txt b/Documentation/technical/api-directory-listing.txt
>> index 0356d25..944fc39 100644
>> --- a/Documentation/technical/api-directory-listing.txt
>> +++ b/Documentation/technical/api-directory-listing.txt
>> @@ -9,8 +9,11 @@ Data structure
>> --------------
>>
>> `struct dir_struct` structure is used to pass directory traversal
>> -options to the library and to record the paths discovered. The notable
>> -options are:
>> +options to the library and to record the paths discovered. A single
>> +`struct dir_struct` is used regardless of whether or not the traversal
>> +recursively descends into subdirectories.
>
> I am somewhat lukewarm on this part of the change.
>
> The added "regardless of..." does not seem to add as much value as
> the two extra lines the patch spends. If we say something like:
>
> A `struct dir_struct` structure is used to pass options to
> traverse directories recursively, and to record all the
> paths discovered by the traversal.
>
> it might be much more direct and informative, I suspect, though.
I somewhat disagree ;) When I first encountered this code, I naturally
assumed that one struct would be created per sub-directory traversed.
This is after all a natural and very common design pattern. The point
of this hunk was to make it explicitly clear that this is *not* how it
works in dir.c. IMHO your rewording still contains a certain amount of
ambiguity in this regard. For example, it could mean that each
dir_struct records all the paths discovered underneath the sub-directory
it represents, and that these recursively bubble up to a top-level
dir_struct.
>> diff --git a/dir.c b/dir.c
>> index ee8e711..89e27a6 100644
>> --- a/dir.c
>> +++ b/dir.c
>> @@ -2,6 +2,8 @@
>> * This handles recursive filename detection with exclude
>> * files, index knowledge etc..
>> *
>> + * See Documentation/technical/api-directory-listing.txt
>> + *
>> * Copyright (C) Linus Torvalds, 2005-2006
>> * Junio Hamano, 2005-2006
>> */
>> @@ -476,6 +478,10 @@ void add_excludes_from_file(struct dir_struct *dir, const char *fname)
>> die("cannot use %s as an exclude file", fname);
>> }
>>
>> +/*
>> + * Loads the per-directory exclude list for the substring of base
>> + * which has a char length of baselen.
>> + */
>> static void prep_exclude(struct dir_struct *dir, const char *base, int baselen)
>> {
>> struct exclude_list *el;
>> @@ -486,7 +492,7 @@ static void prep_exclude(struct dir_struct *dir, const char *base, int baselen)
>> (baselen + strlen(dir->exclude_per_dir) >= PATH_MAX))
>> return; /* too long a path -- ignore */
>>
>> - /* Pop the ones that are not the prefix of the path being checked. */
>> + /* Pop the directories that are not the prefix of the path being checked. */
>
> The "one" does not refer to a "directory", but to an "exclude-list".
No, if that was the case, it would mean that multiple exclude lists
would be popped, but that is not the case here (prior to v4).
> Pop the ones that are not for parent directories of the path
> being checked
Better would be:
Pop the entries within the EXCL_DIRS exclude list which originate
from directories not in the prefix of the path being checked.
although as previously stated, the v4 series I have been holding off
from submitting (in order not to distract you from a maint release)
actually changes this behaviour so EXCL_DIRS becomes an exclude_group of
multiple exclude_lists, one per directory. So in v4, multiple
exclude_lists *will* be popped. I'll tweak the comment in v4 to make
this clear.
--
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