Re: [PATCH v3 02/19] Improve documentation and comments regarding directory traversal API
On Wed, Jan 02, 2013 at 12:54:19PM +, Adam Spiers wrote:
> On Tue, Jan 1, 2013 at 8:52 PM, Junio C Hamano wrote:
> > Adam Spiers writes:
> >> 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).
Sorry, I meant prior to v3 11/19.
> > 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.
Again, I got confused and forgot that I already included the switch to
exclude_list_groups as v3 11/19. But since the patch being discussed
here is v3 02/19 which precedes it, everything I said still applies.
--
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 v3 02/19] Improve documentation and comments regarding directory traversal API
On Tue, Jan 1, 2013 at 8:52 PM, Junio C Hamano wrote:
> Adam Spiers 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 02/19] Improve documentation and comments regarding directory traversal API
Adam Spiers 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.
After all, the word "traversal" pretty much implies that the library
goes in and out of the directories recursively.
> @@ -39,7 +42,7 @@ options are:
> If set, recurse into a directory that looks like a git
> directory. Otherwise it is shown as a directory.
>
> -The result of the enumeration is left in these fields::
> +The result of the enumeration is left in these fields:
Good eyes.
> 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".
Pop the ones that are not for parent directories of the path
being checked
perhaps?
--
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
