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

>
> On Thu, Jul 29, 2021 at 12:37:52PM -0600, Elijah Newren wrote:
>
> > > 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.
>
> It does seem a shame to add the "partial" parameter to strmap_clear(),
> just because most callers don't need it (so they end up with this
> inscrutable "0" parameter).
>
> What if there was a flags field? Then it could be combined with the
> free_values parameter. The result is kind of verbose in two ways:
>
>  - now strset_clear(), etc, need a "flags" parameter, which they didn't
>    before (and is just "0" most of the time!)
>
>  - now "strmap_clear(foo, 1)" becomes "strmap_clear(foo, STRMAP_FREE_VALUES)".
>    That's a lot longer, though arguably it's easier to understand since
>    the boolean is explained.
>
> Having gone through the exercise, I am not sure it is actually making
> anything more readable (messy patch is below for reference).

>
> diff --git a/builtin/shortlog.c b/builtin/shortlog.c
> index 3e7ab1ca82..dfbdba53da 100644
> --- a/builtin/shortlog.c
> +++ b/builtin/shortlog.c
> @@ -242,7 +242,7 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)
>                 insert_records_from_trailers(log, &dups, commit, &ctx, oneline_str);
>         }
>
> -       strset_clear(&dups);
> +       strset_clear(&dups, 0);
>         strbuf_release(&ident);
>         strbuf_release(&oneline);
>  }
> diff --git a/diffcore-rename.c b/diffcore-rename.c
> index 7e6b3e1b14..0c960111d1 100644
> --- a/diffcore-rename.c
> +++ b/diffcore-rename.c
> @@ -665,9 +665,10 @@ void partial_clear_dir_rename_count(struct strmap *dir_rename_count)
>
>         strmap_for_each_entry(dir_rename_count, &iter, entry) {
>                 struct strintmap *counts = entry->value;
> -               strintmap_clear(counts);
> +               strintmap_clear(counts, 0);
>         }
> -       strmap_partial_clear(dir_rename_count, 1);
> +       strmap_clear(dir_rename_count,
> +                    STRMAP_FREE_VALUES | STRMAP_PARTIAL_CLEAR);
>  }
>
>  static void cleanup_dir_rename_info(struct dir_rename_info *info,
> @@ -683,15 +684,15 @@ static void cleanup_dir_rename_info(struct dir_rename_info *info,
>                 return;
>
>         /* idx_map */
> -       strintmap_clear(&info->idx_map);
> +       strintmap_clear(&info->idx_map, 0);
>
>         /* dir_rename_guess */
> -       strmap_clear(&info->dir_rename_guess, 1);
> +       strmap_clear(&info->dir_rename_guess, STRMAP_FREE_VALUES);
>
>         /* relevant_source_dirs */
>         if (info->relevant_source_dirs &&
>             info->relevant_source_dirs != dirs_removed) {
> -               strintmap_clear(info->relevant_source_dirs);
> +               strintmap_clear(info->relevant_source_dirs, 0);
>                 FREE_AND_NULL(info->relevant_source_dirs);
>         }
>
> @@ -716,7 +717,7 @@ static void cleanup_dir_rename_info(struct dir_rename_info *info,
>
>                 if (!strintmap_get(dirs_removed, source_dir)) {
>                         string_list_append(&to_remove, source_dir);
> -                       strintmap_clear(counts);
> +                       strintmap_clear(counts, 0);
>                         continue;
>                 }
>
> @@ -1045,8 +1046,8 @@ static int find_basename_matches(struct diff_options *options,
>                 }
>         }
>
> -       strintmap_clear(&sources);
> -       strintmap_clear(&dests);
> +       strintmap_clear(&sources, 0);
> +       strintmap_clear(&dests, 0);
>
>         return renames;
>  }
> @@ -1700,7 +1701,7 @@ void diffcore_rename_extended(struct diff_options *options,
>         FREE_AND_NULL(rename_src);
>         rename_src_nr = rename_src_alloc = 0;
>         if (break_idx) {
> -               strintmap_clear(break_idx);
> +               strintmap_clear(break_idx, 0);
>                 FREE_AND_NULL(break_idx);
>         }
>         trace2_region_leave("diff", "write back to queue", options->repo);
> diff --git a/merge-ort.c b/merge-ort.c
> index 0fb942692a..0765e23577 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -532,15 +532,10 @@ static void clear_or_reinit_internal_opts(struct merge_options_internal *opti,
>  {
>         struct rename_info *renames = &opti->renames;
>         int i;
> -       void (*strmap_func)(struct strmap *, int) =
> -               reinitialize ? strmap_partial_clear : strmap_clear;
> -       void (*strintmap_func)(struct strintmap *) =
> -               reinitialize ? strintmap_partial_clear : strintmap_clear;
> -       void (*strset_func)(struct strset *) =
> -               reinitialize ? strset_partial_clear : strset_clear;
> +       unsigned flags = reinitialize ? STRMAP_PARTIAL_CLEAR : 0;
>
>         if (opti->pool)
> -               strmap_func(&opti->paths, 0);
> +               strmap_clear(&opti->paths, flags);
>         else {
>                 /*
>                  * We marked opti->paths with strdup_strings = 0, so that
> @@ -550,15 +545,15 @@ static void clear_or_reinit_internal_opts(struct merge_options_internal *opti,
>                  * to these strings, it is time to deallocate them.
>                  */
>                 free_strmap_strings(&opti->paths);
> -               strmap_func(&opti->paths, 1);
> +               strmap_clear(&opti->paths, flags | STRMAP_FREE_VALUES);
>         }
>
>         /*
>          * All keys and values in opti->conflicted are a subset of those in
>          * opti->paths.  We don't want to deallocate anything twice, so we
>          * don't free the keys and we pass 0 for free_values.
>          */
> -       strmap_func(&opti->conflicted, 0);
> +       strmap_clear(&opti->conflicted, flags);
>
>         if (!opti->pool) {
>                 /*
> @@ -579,24 +574,24 @@ static void clear_or_reinit_internal_opts(struct merge_options_internal *opti,
>
>         /* Free memory used by various renames maps */
>         for (i = MERGE_SIDE1; i <= MERGE_SIDE2; ++i) {
> -               strintmap_func(&renames->dirs_removed[i]);
> -               strmap_func(&renames->dir_renames[i], 0);
> -               strintmap_func(&renames->relevant_sources[i]);
> +               strintmap_clear(&renames->dirs_removed[i], flags);
> +               strmap_clear(&renames->dir_renames[i], flags);
> +               strintmap_clear(&renames->relevant_sources[i], flags);
>                 if (!reinitialize)
>                         assert(renames->cached_pairs_valid_side == 0);
>                 if (i != renames->cached_pairs_valid_side &&
>                     -1 != renames->cached_pairs_valid_side) {
> -                       strset_func(&renames->cached_target_names[i]);
> -                       strmap_func(&renames->cached_pairs[i], 1);
> -                       strset_func(&renames->cached_irrelevant[i]);
> +                       strset_clear(&renames->cached_target_names[i], flags);
> +                       strmap_clear(&renames->cached_pairs[i], flags | STRMAP_FREE_VALUES);
> +                       strset_clear(&renames->cached_irrelevant[i], flags);
>                         partial_clear_dir_rename_count(&renames->dir_rename_count[i]);
>                         if (!reinitialize)
>                                 strmap_clear(&renames->dir_rename_count[i], 1);
>                 }
>         }
>         for (i = MERGE_SIDE1; i <= MERGE_SIDE2; ++i) {
> -               strintmap_func(&renames->deferred[i].possible_trivial_merges);
> -               strset_func(&renames->deferred[i].target_dirs);
> +               strintmap_clear(&renames->deferred[i].possible_trivial_merges, flags);
> +               strset_clear(&renames->deferred[i].target_dirs, flags);
>                 renames->deferred[i].trivial_merges_okay = 1; /* 1 == maybe */
>         }
>         renames->cached_pairs_valid_side = 0;
> @@ -1482,7 +1477,7 @@ static int handle_deferred_entries(struct merge_options *opt,
>                         if (ret < 0)
>                                 return ret;
>                 }
> -               strintmap_clear(&copy);
> +               strintmap_clear(&copy, 0);
>                 strintmap_for_each_entry(&renames->deferred[side].possible_trivial_merges,
>                                          &iter, entry) {
>                         const char *path = entry->key;
> diff --git a/strmap.c b/strmap.c
> index 4fb9f6100e..7343800df5 100644
> --- a/strmap.c
> +++ b/strmap.c
> @@ -37,10 +37,11 @@ void strmap_init_with_options(struct strmap *map,
>         map->strdup_strings = strdup_strings;
>  }
>
> -static void strmap_free_entries_(struct strmap *map, int free_values)
> +static void strmap_free_entries_(struct strmap *map, unsigned flags)
>  {
>         struct hashmap_iter iter;
>         struct strmap_entry *e;
> +       int free_values = flags & STRMAP_FREE_VALUES;
>
>         if (!map)
>                 return;
> @@ -64,16 +65,13 @@ static void strmap_free_entries_(struct strmap *map, int free_values)
>         }
>  }
>
> -void strmap_clear(struct strmap *map, int free_values)
> +void strmap_clear(struct strmap *map, unsigned flags)
>  {
> -       strmap_free_entries_(map, free_values);
> -       hashmap_clear(&map->map);
> -}
> -
> -void strmap_partial_clear(struct strmap *map, int free_values)
> -{
> -       strmap_free_entries_(map, free_values);
> -       hashmap_partial_clear(&map->map);
> +       strmap_free_entries_(map, flags);
> +       if (flags & STRMAP_PARTIAL_CLEAR)
> +               hashmap_partial_clear(&map->map);
> +       else
> +               hashmap_clear(&map->map);
>  }
>
>  static struct strmap_entry *create_entry(struct strmap *map,
> diff --git a/strmap.h b/strmap.h
> index 1e152d832d..d03d451654 100644
> --- a/strmap.h
> +++ b/strmap.h
> @@ -46,16 +46,14 @@ void strmap_init_with_options(struct strmap *map,
>                               struct mem_pool *pool,
>                               int strdup_strings);
>
> -/*
> - * Remove all entries from the map, releasing any allocated resources.
> - */
> -void strmap_clear(struct strmap *map, int free_values);
> +#define STRMAP_FREE_VALUES 1 /* 1 for historical compat, but we should probably
> +                               update callers to use the correct name) */
> +#define STRMAP_PARTIAL_CLEAR 2
>
>  /*
> - * Similar to strmap_clear() but leaves map->map->table allocated and
> - * pre-sized so that subsequent uses won't need as many rehashings.
> + * Remove all entries from the map, releasing any allocated resources.
>   */
> -void strmap_partial_clear(struct strmap *map, int free_values);
> +void strmap_clear(struct strmap *map, unsigned flags);
>
>  /*
>   * Insert "str" into the map, pointing to "data".
> @@ -148,14 +146,10 @@ static inline void strintmap_init_with_options(struct strintmap *map,
>         map->default_value = default_value;
>  }
>
> -static inline void strintmap_clear(struct strintmap *map)
> -{
> -       strmap_clear(&map->map, 0);
> -}
> -
> -static inline void strintmap_partial_clear(struct strintmap *map)
> +static inline void strintmap_clear(struct strintmap *map, unsigned flags)
>  {
> -       strmap_partial_clear(&map->map, 0);
> +       /* maybe clear STRMAP_FREE_VALUES bit for extra protection */
> +       strmap_clear(&map->map, flags);
>  }
>
>  static inline int strintmap_contains(struct strintmap *map, const char *str)
> @@ -232,14 +226,9 @@ static inline void strset_init_with_options(struct strset *set,
>         strmap_init_with_options(&set->map, pool, strdup_strings);
>  }
>
> -static inline void strset_clear(struct strset *set)
> -{
> -       strmap_clear(&set->map, 0);
> -}
> -
> -static inline void strset_partial_clear(struct strset *set)
> +static inline void strset_clear(struct strset *set, unsigned flags)
>  {
> -       strmap_partial_clear(&set->map, 0);
> +       strmap_clear(&set->map, flags);
>  }
>
>  static inline int strset_contains(struct strset *set, const char *str)