Junio C Hamano [off-list ref] writes:
Adam Spiers [off-list ref] writes:
quoted
Adam Spiers (14):
Update directory listing API doc to match code
Improve documentation and comments regarding directory traversal API
Rename cryptic 'which' variable to more consistent name
Rename path_excluded() to is_path_excluded()
Rename excluded_from_list() to is_excluded_from_list()
Rename excluded() to is_excluded()
Refactor is_excluded_from_list()
Refactor is_excluded()
Refactor is_path_excluded()
For each exclude pattern, store information about where it came from
Refactor treat_gitlinks()
Extract some useful pathspec handling code from builtin/add.c into a
library
Provide free_directory() for reclaiming dir_struct memory
Add git-check-ignore sub-command
Please retitle these to have a short "prefix: " that names a
specific area the series intends to touch. I retitled your other
series to share "test :" as their common prefix.
Just to clarify, I think most of them can say "dir.c: ".
I saw quite a few decl-after-statement in new code. Please fix
them.
As to the "who owns x->src and when is it freed" question, it may
make sense to give el a "filename" field (command line and other
special cases would get whatever value you deem appropriate, like
NULL or "<command line>"), have x->src point at that field when you
queue many x's to the el in add_exc_from_file_to_list(). Then when
you pop an element in the exclude_stack, you can free el->filename
to plug a potential leak.
Also I do not see why you need to have the strdup() in the caller of
add_excludes_from_file_to_list(). If you need to keep it stable
because you are copying it away in exclude or excludde_list,
wouldn't it make more sense to do that at the beginning of the
callee, i.e. add_excludes_from_file_to_list() function?
On Thu, Sep 20, 2012 at 10:43 PM, Junio C Hamano [off-list ref] wrote:
Junio C Hamano [off-list ref] writes:
quoted
Adam Spiers [off-list ref] writes:
quoted
Adam Spiers (14):
Update directory listing API doc to match code
Improve documentation and comments regarding directory traversal API
Rename cryptic 'which' variable to more consistent name
Rename path_excluded() to is_path_excluded()
Rename excluded_from_list() to is_excluded_from_list()
Rename excluded() to is_excluded()
Refactor is_excluded_from_list()
Refactor is_excluded()
Refactor is_path_excluded()
For each exclude pattern, store information about where it came from
Refactor treat_gitlinks()
Extract some useful pathspec handling code from builtin/add.c into a
library
Provide free_directory() for reclaiming dir_struct memory
Add git-check-ignore sub-command
Please retitle these to have a short "prefix: " that names a
specific area the series intends to touch. I retitled your other
series to share "test :" as their common prefix.
Just to clarify, I think most of them can say "dir.c: ".
Sure, I can do that, but shouldn't this convention be documented in
SubmittingPatches?
I saw quite a few decl-after-statement in new code. Please fix
them.
Again, I can do that no problem, but again this convention is
undocumented and I am not psychic ;-) I see that a patch was provided
5 years ago to document this, but was apparently never pulled in:
http://thread.gmane.org/gmane.comp.version-control.git/47843/focus=48015
It would save everybody's time if these things are documented, since
then they wouldn't create noise during the review process.
I also see in the same thread that a patch to add
-Wdeclaration-after-statement to CFLAGS was also offered but never
pulled in, presumably on the stated grounds that that option was
relatively recent 5 years ago. But wouldn't it be trivial to do
gcc -v --help 2>&1 | grep declaration-after-statement
at build-time?
I'm also curious to know why this convention exists. Are people
really still compiling git with compilers which don't support C99?
As to the "who owns x->src and when is it freed" question, it may
make sense to give el a "filename" field (command line and other
special cases would get whatever value you deem appropriate, like
NULL or "<command line>"), have x->src point at that field when you
queue many x's to the el in add_exc_from_file_to_list(). Then when
you pop an element in the exclude_stack, you can free el->filename
to plug a potential leak.
Also I do not see why you need to have the strdup() in the caller of
add_excludes_from_file_to_list(). If you need to keep it stable
because you are copying it away in exclude or excludde_list,
wouldn't it make more sense to do that at the beginning of the
callee, i.e. add_excludes_from_file_to_list() function?
Thanks, I'll take a look at these suggestions tomorrow.
On Thu, Sep 20, 2012 at 10:43 PM, Junio C Hamano [off-list ref] wrote:
Junio C Hamano [off-list ref] writes:
quoted
Adam Spiers [off-list ref] writes:
quoted
Adam Spiers (14):
Update directory listing API doc to match code
Improve documentation and comments regarding directory traversal API
Rename cryptic 'which' variable to more consistent name
Rename path_excluded() to is_path_excluded()
Rename excluded_from_list() to is_excluded_from_list()
Rename excluded() to is_excluded()
Refactor is_excluded_from_list()
Refactor is_excluded()
Refactor is_path_excluded()
For each exclude pattern, store information about where it came from
Refactor treat_gitlinks()
Extract some useful pathspec handling code from builtin/add.c into a
library
Provide free_directory() for reclaiming dir_struct memory
Add git-check-ignore sub-command
[snipped]
As to the "who owns x->src and when is it freed" question, it may
make sense to give el a "filename" field (command line and other
special cases would get whatever value you deem appropriate, like
NULL or "<command line>"), have x->src point at that field when you
queue many x's to the el in add_exc_from_file_to_list(). Then when
you pop an element in the exclude_stack, you can free el->filename
to plug a potential leak.
I have done this, but it required a change to struct dir:
Currently, when dir->exclude_stack is more than one entry deep, the
exclude_list pointed to by dir->exclude_list[EXC_DIRS] is a
concatenation of exclude elements from multiple files, so there will
be different values for src. The same is true of EXC_FILE, which
typically mixes patterns from .git/info/exclude and core.excludesfile.
Therefore I have split each exclude_list into potentially multiple
exclude_lists, one per pattern source, whilst preserving the EXC_*
grouping and ordering.
My latest re-roll of as/check-ignore is nearly ready, and when I send
it, you will see a new patch in there covering the above change.
Also I do not see why you need to have the strdup() in the caller of
add_excludes_from_file_to_list(). If you need to keep it stable
because you are copying it away in exclude or excludde_list,
wouldn't it make more sense to do that at the beginning of the
callee, i.e. add_excludes_from_file_to_list() function?
No, because in all other callers of add_excludes_from_file_to_list(),
the exclude source is already stable. The re-roll will make this
clearer.