On Thu, Jul 29, 2021 at 9:29 AM Jeff King <peff@xxxxxxxx> wrote:
>
> On Thu, Jul 29, 2021 at 03:58:38AM +0000, Elijah Newren via GitGitGadget wrote:
>
> > diff --git a/merge-ort.c b/merge-ort.c
> > index 2bca4b71f2a..5fd2a4ccd35 100644
> > --- a/merge-ort.c
> > +++ b/merge-ort.c
> > @@ -539,15 +539,19 @@ static void clear_or_reinit_internal_opts(struct merge_options_internal *opti,
> > void (*strset_func)(struct strset *) =
> > reinitialize ? strset_partial_clear : strset_clear;
> >
> > - /*
> > - * We marked opti->paths with strdup_strings = 0, so that we
> > - * wouldn't have to make another copy of the fullpath created by
> > - * make_traverse_path from setup_path_info(). But, now that we've
> > - * used it and have no other references to these strings, it is time
> > - * to deallocate them.
> > - */
> > - free_strmap_strings(&opti->paths);
> > - strmap_func(&opti->paths, 1);
> > + if (opti->pool)
> > + strmap_func(&opti->paths, 0);
>
> This isn't new in your patch here, but I did scratch my head a bit over
> what "strmap_func" is. It's a bit less confusing if you read the whole
> function (as opposed to a diff), since then you're more likely to see
> the definition. But something like "strmap_clear_func()" would have been
> a lot less confusing.
Makes sense.
> Arguably, the existence of these function indirections is perhaps a sign
> that the strmap API should provide a version of the clear functions that
> takes "partial / not-partial" as a parameter.
Are you suggesting a modification of str{map,intmap,set}_clear() to
take an extra parameter, or removing the
str{map,intmap,set}_partial_clear() functions and introducing new
functions that take a partial/not-partial parameter? I think you're
suggesting the latter, and that makes more sense to me...but I'm
drawing blanks trying to come up with a reasonable function name.
(If it helps for context -- the only current callers of the
*_partial_clear() functions are found in diffcore-rename.c and
merge-ort.c, so it'd be a pretty easy change to make to those. There
are additionally some callers of strmap_clear() and strset_clear() in
builtin/shortlog.c and rerere.c, and it'd be nice to avoid exposing
those to the complexity of the partial clearing.)
> (Again, not really part of this patch series, but I hadn't looked at
> some of the earlier optimization steps).
Yeah, but this is the kind of reason I wanted you to review this
series, because I figured you might have more good comments on the
str{map,intmap,set} API calls. :-)
