Re: [PATCH v2 4/7] merge-ort: switch our strmaps over to using memory pools

On Thu, Jul 29, 2021 at 9:29 AM Jeff King [off-list ref] wrote:

On Thu, Jul 29, 2021 at 03:58:38AM +0000, Elijah Newren via GitGitGadget wrote:

quoted

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.  :-)