Re: [PATCH 2/6] Split unpack_trees 'reset' flag into two for untracked handling
- From
Elijah Newren <newren@gmail.com>
- Date
- Sep 20, 2021, 15:20 UTC
- Message-ID
- <CABPp-BFiKuTi_uSmWXc4FF-zYzODhtVdv8AQbz_--bz6XZjp6A@mail.gmail.com>
- In-Reply-To
- <87wnncbrm4.fsf@evledraar.gmail.com>
On Sun, Sep 19, 2021 at 6:51 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
Show 12 quoted lines
> > On Sat, Sep 18 2021, Elijah Newren via GitGitGadget wrote: > > > - opts.reset = reset; > > + opts.reset_keep_untracked = reset; > > opts.fn = twoway_merge; > > + /* Setup opts.dir so that ignored files in the way get overwritten */ > > + opts.dir = xcalloc(1, sizeof(*opts.dir)); > > + opts.dir->flags |= DIR_SHOW_IGNORED; > > + setup_standard_excludes(opts.dir); > > Is the "opts.dir" free'd later somehow?
No, much like other callsites that set this up (e.g. builtin/checkout.c), there isn't a place that frees it. In copying how they worked, I also copied their bugs. ;-)
I'm tempted to move this code into setup_unpack_trees_porcelain() and then free it in clear_unpack_trees_porcelain()...though not all callers make use of those functions. Hmm...
Show 11 quoted lines
> > opts.head_idx = -1; > > opts.update = worktree; > > opts.skip_unmerged = !worktree; > > - opts.reset = 1; > > + if (o->force) > > + opts.reset_nuke_untracked = 1; > > + else > > + opts.reset_keep_untracked = 1; > > In both cases opts.reset_keep_untracked is set to 1, I assume it's a > mistake
No, it only sets opts.reset_keep_untracked to 1 when o->force is false. If o->force is true, it instead sets opts.reset_nuke_untracked to 1. There's no mistake there.
Show 17 quoted lines
> > opts.merge = 1;
> > opts.fn = oneway_merge;
> > opts.verbose_update = o->show_progress;
> > opts.src_index = &the_index;
> > opts.dst_index = &the_index;
> > + if (o->overwrite_ignore) {
> > + opts.dir = xcalloc(1, sizeof(*opts.dir));
>
> ditto potential leak.
>
> > + opts.dir = xcalloc(1, sizeof(*opts.dir));
> > + opts.dir->flags |= DIR_SHOW_IGNORED;
> > + setup_standard_excludes(opts.dir);
> > + }
>
>
> ditto (also more omitted).Yep.