Re: [PATCH v7 17/31] merge-recursive: add a new hashmap for storing directory renames

[Stefan Beller <sbeller@google.com>]

On Tue, Jan 30, 2018 at 3:25 PM, Elijah Newren <newren@gmail.com> wrote:
> This just adds dir_rename_entry and the associated functions; code using
> these will be added in subsequent commits.
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  merge-recursive.c | 35 +++++++++++++++++++++++++++++++++++
>  merge-recursive.h |  8 ++++++++
>  2 files changed, 43 insertions(+)
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index 8ac69e1cbb..3b6d0e3f70 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -49,6 +49,41 @@ static unsigned int path_hash(const char *path)
>         return ignore_case ? strihash(path) : strhash(path);
>  }
>
> +static struct dir_rename_entry *dir_rename_find_entry(struct hashmap *hashmap,
> +                                                     char *dir)
> +{
> +       struct dir_rename_entry key;
> +
> +       if (dir == NULL)
> +               return NULL;
> +       hashmap_entry_init(&key, strhash(dir));
> +       key.dir = dir;
> +       return hashmap_get(hashmap, &key, NULL);
> +}
> +
> +static int dir_rename_cmp(void *unused_cmp_data,
> +                         const struct dir_rename_entry *e1,
> +                         const struct dir_rename_entry *e2,
> +                         const void *unused_keydata)
> +{
> +       return strcmp(e1->dir, e2->dir);
> +}
> +
> +static void dir_rename_init(struct hashmap *map)
> +{
> +       hashmap_init(map, (hashmap_cmp_fn) dir_rename_cmp, NULL, 0);

See 55c965f3a2 (Merge branch 'sb/hashmap-cleanup', 2017-08-11) or rather
201c14e375 (attr.c: drop hashmap_cmp_fn cast, 2017-06-30) for an attempt to
keep out the casting in the init, but cast the void->type inside the function.

> +static void dir_rename_entry_init(struct dir_rename_entry *entry,
> +                                 char *directory)
> +{
> +       hashmap_entry_init(entry, strhash(directory));
> +       entry->dir = directory;
> +       entry->non_unique_new_dir = 0;
> +       strbuf_init(&entry->new_dir, 0);
> +       string_list_init(&entry->possible_new_dirs, 0);
> +}
> +
>  static void flush_output(struct merge_options *o)
>  {
>         if (o->buffer_output < 2 && o->obuf.len) {
> diff --git a/merge-recursive.h b/merge-recursive.h
> index 80d69d1401..d7f4cc80c1 100644
> --- a/merge-recursive.h
> +++ b/merge-recursive.h
> @@ -29,6 +29,14 @@ struct merge_options {
>         struct string_list df_conflict_file_set;
>  };
>
> +struct dir_rename_entry {
> +       struct hashmap_entry ent; /* must be the first member! */
> +       char *dir;
> +       unsigned non_unique_new_dir:1;
> +       struct strbuf new_dir;
> +       struct string_list possible_new_dirs;
> +};

Could you add comments what these are and if they have any constraints?
e.g. is 'dir' the full path from the root of the repo and does it end
with a '/' ?

Having a stringlist of potentially new dirs sounds like the algorithm is
at least n^2, but how do I know? I'll read on.

This patch only adds static functions, so some compilers may even refuse
to compile after this patch (-Werror -Wunused). I am not sure how strict we
are there, but as git-bisect still hasn't learned about going only
into the first
parent to have bisect working on a "per series" level rather than a "per commit"
level, it is possible that someone will end up on this commit in the future and
it won't compile well. :/

Not sure what to recommend, maybe squash this with the patch that makes
use of these functions?

Thanks,
Stefan