Re: [PATCH v3 04/11] unpack-trees: introduce preserve_ignored to unpack_trees_options
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Sep 30, 2021, 14:04 UTC
- Message-ID
- <87lf3etaih.fsf@evledraar.gmail.com>
- In-Reply-To
- <CABPp-BEEWPF6oBN69PH_GtmqqvAj1HMtDX5ipQxa2bLzyCCu0g@mail.gmail.com>
On Wed, Sep 29 2021, Elijah Newren wrote:
Show 59 quoted lines
> On Wed, Sep 29, 2021 at 11:32 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>>
>> On Wed, Sep 29 2021, Elijah Newren wrote:
>>
>> > On Wed, Sep 29, 2021 at 2:27 AM Ævar Arnfjörð Bjarmason
>> > <avarab@gmail.com> wrote:
>> >>
> ...
>> >>
>> >> I think getting rid of the boilerplate makes sense, but it doesn't sound
>> >> from the commit message like you've considered just making that "struct
>> >> dir*" member a "struct dir" instead.
>> >>
>> >> That simplifies things a lot, i.e. we can just DIR_INIT it, and don't
>> >> need every caller to malloc/free it.
>> >
>> > See the next patch in the series. :-)
>>
>> Ah!
>>
>> >> Sometimes a pointer makes sense, but in this case the "struct
>> >> unpack_trees_options" can just own it.
>> >
>> > I did make it internal to unpack_trees_options in the next patch, but
>> > kept it as a pointer just because that let me know whether it was used
>> > or not. I guess I could have added a boolean as well. But I don't
>> > actually allocate anything, because it's either a NULL pointer, or a
>> > pointer to something on the stack. So, I do get to just use DIR_INIT.
>>
>> I think I'm probably missing something. I just made it allocated on the
>> stack by the caller using "struct unpack_trees_options", but then you
>> end up having a dir* in the struct, but that's only filled in as a
>> pointer to the stack variable? Maybe there's some subtlety I'm missing
>> here...
>
> As per the next patch:
>
> int unpack_trees(..., struct unpack_trees_options *o)
> {
> struct dir_struct dir = DIR_INIT;
> ...
> if (!o->preserve_ignored) {
> /* Setup 'dir', make o->dir point to it */
> ....
> o->dir = &dir;
> }
> ...
> if (o->dir)
> /* cleanup */
> ....
> }
>
> The caller doesn't touch o->dir (other than initializing it to zeros);
> unpack_trees() is wholly responsible for it. I'd kind of like to
> entirely remove dir from unpack_trees_options(), but I need a way of
> passing it down through all the other functions in unpack-trees.c, and
> leaving it in unpack_trees_options seems the easiest way to do so. So
> I just marked it as "for internal use only".I think I understand *how* it works, I'm puzzled by why you went for this whole level of indirection when you're using a struct on the stack in the end anyway, just ... put that in "struct unpack_trees_options"?
Anyway, I see I have only myself to blame here, as you added these leak fixes in the v2 in response to some of my offhand comments.
FWIW I then went on to do some deeper fixes not just on these leaks but the surrounding leaks, which will be blocked by 2/11 & 05/11 of this topic for a while. I suppose I only have myself to blame :)
Below is a patch-on-top that I think makes this whole thing much simpler by doing away with the pointer entirely.
I suppose this is also a partial reply to https://lore.kernel.org/git/CABPp-BG_qigBoirMGR-Yk9Niyxt0UmYCEqojsYxbSEarLAmraA@mail.gmail.com/; but I quite dislike this pattern of including a pointer like this where it's not needed just for the practicalities of memory management.
I.e. here you use DIR_INIT. In my local patches to fix up the wider memory leaks in this area I've got DIR_INIT also using a STRBUF_INIT, and DIR_INIT will in turn be referenced by a UNPACK_TREES_OPTIONS_INIT. It's quite nice if you're having to initialize with "UNPACK_TREES_OPTIONS_INIT" have that initialization work all the way down the chain, and not need e.g. a manual strbuf_init(), dir_init() etc.
I removed the dir_init() in ce93a4c6127 (dir.[ch]: replace dir_init() with DIR_INIT, 2021-07-01), but would probably need to bring it back, of course you need some "release()" method for the UNPACK_TREES_OPTIONS_INIT, which in turn needs to call the dir_release() (well, "dir_clear()" in that case), and it needs to call "strbuf_release()". It's just nicer if that boilerplate is all on destruction, but not also on struct/object setup.
We do need that setup in some cases (although a lot could just be replaced by lazy initialization), but if we don't....
diff --git a/unpack-trees.c b/unpack-trees.c index a7e1712d236..de5cc6cd025 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -1694,15 +1694,12 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options static struct cache_entry *dfc; struct pattern_list pl; int free_pattern_list = 0; - struct dir_struct dir = DIR_INIT; if (o->reset == UNPACK_RESET_INVALID) BUG("o->reset had a value of 1; should be UNPACK_TREES_*_UNTRACKED"); if (len > MAX_UNPACK_TREES) die("unpack_trees takes at most %d trees", MAX_UNPACK_TREES); - if (o->dir) - BUG("o->dir is for internal use only"); trace_performance_enter(); trace2_region_enter("unpack_trees", "unpack_trees", the_repository); @@ -1718,9 +1715,8 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options BUG("UNPACK_RESET_OVERWRITE_UNTRACKED incompatible with preserved ignored files"); if (!o->preserve_ignored) { - o->dir = &dir; - o->dir->flags |= DIR_SHOW_IGNORED; - setup_standard_excludes(o->dir); + o->dir.flags |= DIR_SHOW_IGNORED; + setup_standard_excludes(&o->dir); } if (!core_apply_sparse_checkout || !o->update) @@ -1884,10 +1880,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options done: if (free_pattern_list) clear_pattern_list(&pl); - if (o->dir) { - dir_clear(o->dir); - o->dir = NULL; - } + dir_clear(&o->dir); trace2_region_leave("unpack_trees", "unpack_trees", the_repository); trace_performance_leave("unpack_trees"); return ret; @@ -2153,8 +2146,7 @@ static int verify_clean_subdirectory(const struct cache_entry *ce, pathbuf = xstrfmt("%.*s/", namelen, ce->name); memset(&d, 0, sizeof(d)); - if (o->dir) - d.exclude_per_dir = o->dir->exclude_per_dir; + d.exclude_per_dir = o->dir.exclude_per_dir; i = read_directory(&d, o->src_index, pathbuf, namelen+1, NULL); if (i) return add_rejected_path(o, ERROR_NOT_UPTODATE_DIR, ce->name); @@ -2201,8 +2193,7 @@ static int check_ok_to_remove(const char *name, int len, int dtype, if (ignore_case && icase_exists(o, name, len, st)) return 0; - if (o->dir && - is_excluded(o->dir, o->src_index, name, &dtype)) + if (is_excluded(&o->dir, o->src_index, name, &dtype)) /* * ce->name is explicitly excluded, so it is Ok to * overwrite it. diff --git a/unpack-trees.h b/unpack-trees.h index 71ffb7eeb0c..a8afbb20170 100644 --- a/unpack-trees.h +++ b/unpack-trees.h @@ -5,6 +5,7 @@ #include "strvec.h" #include "string-list.h" #include "tree-walk.h" +#include "dir.h" #define MAX_UNPACK_TREES MAX_TRAVERSE_TREES @@ -95,7 +96,7 @@ struct unpack_trees_options { struct index_state result; struct pattern_list *pl; /* for internal use */ - struct dir_struct *dir; /* for internal use only */ + struct dir_struct dir; /* for internal use only */ struct checkout_metadata meta; };