patchoidmap.h: strongly discourage using OIDMAP_INIT directly

> In Git's source code, we have this convention that quite a few data
> structures can be initialized using a macro *_INIT while defining an
> instance (instead of having to call a function to initialize the data
> structure). You will see that idiom quite a bit, e.g. `struct strbuf buf
> = STRBUF_INIT;`
> 
> This works for oidsets, too: `struct oidset oids = OIDSET_INIT;` is
> perfectly legal and you can use that data structure right away, without
> having to call `oidset_init()` first.
> 
> That pattern is violated by OIDMAP_INIT, though. The first call to
> oidmap_put() or oidmap_get() will succeed, but by mistake rather than by
> design: The underlying hashmap is not initialized correctly, as the
> cmpfn function pointer still points to NULL, but since there are no
> entries to be compared, cmpfn will not be called. Things break down,
> though, as soon as there is even one entry.
> 
> Rather than causing confusion, frustration and needless loss of time due
> to pointless debugging, let's *rename* OIDMAP_INIT so that developers
> who have gotten used to the pattern `struct xyz a = XYZ_INIT;` won't do
> that with oidmaps.
> 
> An alternative would be to introduce a HASHMAP_INIT(cmp_fn) and use that
> in oidmap.h. However, there are *no* call sites in Git's source code
> that would benefit from that macro, i.e. this would be premature
> optimization (and cost a lot more time than this here trivial patch).
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> Published-As: https://github.com/dscho/git/releases/tag/discourage-OIDMAP_INIT-v1
> Fetch-It-Via: git fetch https://github.com/dscho/git discourage-OIDMAP_INIT-v1
>  oidmap.h | 6 +++++-
>  oidset.h | 7 ++++++-
>  2 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/oidmap.h b/oidmap.h
> index 18f54cde143..1a73c392b79 100644
> --- a/oidmap.h
> +++ b/oidmap.h
> @@ -21,7 +21,11 @@ struct oidmap {
>  	struct hashmap map;
>  };
>  
> -#define OIDMAP_INIT { { NULL } }
> +/*
> + * This macro initializes the data structure only incompletely, just enough
> + * to call oidmap_get() on an empty map. Use oidmap_init() instead.
> + */
> +#define OIDMAP_INIT_INCOMPLETELY { { NULL } }

>  
>  /*
>   * Initializes an oidmap structure.
> diff --git a/oidset.h b/oidset.h
> index f4c9e0f9c04..a11d88edc1d 100644
> --- a/oidset.h
> +++ b/oidset.h
> @@ -22,7 +22,12 @@ struct oidset {
>  	struct oidmap map;
>  };
>  
> -#define OIDSET_INIT { OIDMAP_INIT }
> +/*
> + * It is okay to initialize the map incompletely here because oidset_insert()
> + * will call oidset_init() (which will call oidmap_init()), and
> + * oidset_contains() works as intended even before oidset_init() was called.
> + */
> +#define OIDSET_INIT { OIDMAP_INIT_INCOMPLETELY }
>  
>  /**
>   * Returns true iff `set` contains `oid`.
> 
> base-commit: 936d1b989416a95f593bf81ccae8ac62cd83f279
> -- 
> 2.15.1.windows.2

>> -#define OIDMAP_INIT { { NULL } }
>> +/*
>> + * This macro initializes the data structure only incompletely, just enough
>> + * to call oidmap_get() on an empty map. Use oidmap_init() instead.
>> + */
>> +#define OIDMAP_INIT_INCOMPLETELY { { NULL } }
>
> This is one way of approaching the problem.  Couldn't we also take the
> approach like we have with oidset and ensure that when oidmap_get() or
> _put() are called that if the oidmap isn't initialized, we simply do
> that then?

> Ensure that an oidmap is initialized before attempting to add, remove,
> or retrieve an entry by simply performing the initialization step
> before accessing the underlying hashmap.
> 
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
>  oidmap.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/oidmap.c b/oidmap.c
> index 6db4fffcd..d9fb19ba6 100644
> --- a/oidmap.c
> +++ b/oidmap.c
> @@ -33,12 +33,19 @@ void oidmap_free(struct oidmap *map, int free_entries)
>  
>  void *oidmap_get(const struct oidmap *map, const struct object_id *key)
>  {
> +	if (!map->map.cmpfn)
> +		return NULL;
> +

>  	return hashmap_get_from_hash(&map->map, hash(key), key);
>  }
>  
>  void *oidmap_remove(struct oidmap *map, const struct object_id *key)
>  {
>  	struct hashmap_entry entry;
> +
> +	if (!map->map.cmpfn)
> +		oidmap_init(map, 0);
> +
>  	hashmap_entry_init(&entry, hash(key));
>  	return hashmap_remove(&map->map, &entry, key);
>  }
> @@ -46,6 +53,10 @@ void *oidmap_remove(struct oidmap *map, const struct object_id *key)
>  void *oidmap_put(struct oidmap *map, void *entry)
>  {
>  	struct oidmap_entry *to_put = entry;
> +
> +	if (!map->map.cmpfn)
> +		oidmap_init(map, 0);
> +
>  	hashmap_entry_init(&to_put->internal_entry, hash(&to_put->oid));
>  	return hashmap_put(&map->map, to_put);
>  }

> Ensure that an oidmap is initialized before attempting to add, remove,
> or retrieve an entry by simply performing the initialization step
> before accessing the underlying hashmap.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---

>  oidmap.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/oidmap.c b/oidmap.c
> index 6db4fffcd..d9fb19ba6 100644
> --- a/oidmap.c
> +++ b/oidmap.c
> @@ -33,12 +33,19 @@ void oidmap_free(struct oidmap *map, int free_entries)
>  
>  void *oidmap_get(const struct oidmap *map, const struct object_id *key)
>  {
> +	if (!map->map.cmpfn)
> +		return NULL;
> +
>  	return hashmap_get_from_hash(&map->map, hash(key), key);
>  }
>  
>  void *oidmap_remove(struct oidmap *map, const struct object_id *key)
>  {
>  	struct hashmap_entry entry;
> +
> +	if (!map->map.cmpfn)
> +		oidmap_init(map, 0);
> +
>  	hashmap_entry_init(&entry, hash(key));
>  	return hashmap_remove(&map->map, &entry, key);
>  }
> @@ -46,6 +53,10 @@ void *oidmap_remove(struct oidmap *map, const struct object_id *key)
>  void *oidmap_put(struct oidmap *map, void *entry)
>  {
>  	struct oidmap_entry *to_put = entry;
> +
> +	if (!map->map.cmpfn)
> +		oidmap_init(map, 0);
> +
>  	hashmap_entry_init(&to_put->internal_entry, hash(&to_put->oid));
>  	return hashmap_put(&map->map, to_put);
>  }

> Brandon Williams <bmwill@google.com> writes:
> 
> >> -#define OIDMAP_INIT { { NULL } }
> >> +/*
> >> + * This macro initializes the data structure only incompletely, just enough
> >> + * to call oidmap_get() on an empty map. Use oidmap_init() instead.
> >> + */
> >> +#define OIDMAP_INIT_INCOMPLETELY { { NULL } }
> >
> > This is one way of approaching the problem.  Couldn't we also take the
> > approach like we have with oidset and ensure that when oidmap_get() or
> > _put() are called that if the oidmap isn't initialized, we simply do
> > that then?
> 
> Hmph.  Can you show how the alternative code would look like?

>> That pattern is violated by OIDMAP_INIT, though. The first call to
>> oidmap_put() or oidmap_get() will succeed, but by mistake rather than by
>> design: The underlying hashmap is not initialized correctly, as the
>> cmpfn function pointer still points to NULL, but since there are no
>> entries to be compared, cmpfn will not be called. Things break down,
>> though, as soon as there is even one entry.
>>
>> Rather than causing confusion, frustration and needless loss of time due
>> to pointless debugging, let's *rename* OIDMAP_INIT so that developers
>> who have gotten used to the pattern `struct xyz a = XYZ_INIT;` won't do
>> that with oidmaps.
>>   
>> -#define OIDMAP_INIT { { NULL } }
>> +/*
>> + * This macro initializes the data structure only incompletely, just enough
>> + * to call oidmap_get() on an empty map. Use oidmap_init() instead.
>> + */
>> +#define OIDMAP_INIT_INCOMPLETELY { { NULL } }