Re: [PATCH v2 00/14] new git check-ignore sub-command
- From
Adam Spiers <git@adamspiers.org>
- Date
- Dec 26, 2012, 15:44 UTC
- Message-ID
- <CAOkDyE_hs9kVtk4DbN+0gTaud35oLv4S4wiuAgYmtxCnPuUzhA@mail.gmail.com>
- In-Reply-To
- <7vsjac8j52.fsf@alter.siamese.dyndns.org>
On Thu, Sep 20, 2012 at 10:43 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 18 quoted lines
> Junio C Hamano <gitster@pobox.com> writes: >> Adam Spiers <git@adamspiers.org> writes: >>> 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]
Show 7 quoted lines
> 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.
Show 5 quoted lines
> 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.