Re: [PATCH] builtin/receive-pack: avoid generic function name hmac

[Junio C Hamano <gitster@pobox.com>]

Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

> fabec2c5c3 (builtin/receive-pack: switch to use the_hash_algo, 2019-08-18)
> renames hmac_sha1 to hmac, as it was updated to (optionally) use the hash
> function used by git (which won't be sha1 in the future).
>
> hmac() is provided by NetBSD > 8 libc and conflicts as shown by :
>
> builtin/receive-pack.c:421:13: error: conflicting types for 'hmac'
>  static void hmac(unsigned char *out,
>              ^~~~
> In file included from ./git-compat-util.h:172:0,
>                  from ./builtin.h:4,
>                  from builtin/receive-pack.c:1:
> /usr/include/stdlib.h:305:10: note: previous declaration of 'hmac' was here
>  ssize_t  hmac(const char *, const void *, size_t, const void *, size_t, void *,
>           ^~~~

Including <stdlib.h> is allowed to contaminate the namespace this way?

At least
https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/stdlib.h.html
does not seem to list hmac() there.

> While the conflict, posses the question of why are we even implementing our
> own RFC 2104 complaint HMAC while alternatives are readily available, the
> simplest solution is to make sure the name is not as generic so it would
> conflict with an equivalent one from the system (or linked libraries); so
> rename it again to hmac_hash to reflect it will use the git's defined hash
> function.

I do not mind renaming ours, as it is harder to get the <stdlib.h>
fixed on the NetBSD, but I would phrase s/equivalent/unrelated/ (or
even "irrelevant"), as it is clearly not an "alternative" that is
readily available outside NetBSD.  Otherwise we would have found
this a lot sooner (it used to be called hmac_sha1() and renamed to
hmac() in August last year).

> ---
>  builtin/receive-pack.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Missing sign-off.  The patch text is trivially correct.

> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 2cc18bbffd..b1d939e7ed 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -418,7 +418,7 @@ static int copy_to_sideband(int in, int out, void *arg)
>  	return 0;
>  }
>  
> -static void hmac(unsigned char *out,
> +static void hmac_hash(unsigned char *out,
>  		      const char *key_in, size_t key_len,
>  		      const char *text, size_t text_len)
>  {
> @@ -463,7 +463,7 @@ static char *prepare_push_cert_nonce(const char *path, timestamp_t stamp)
>  	unsigned char hash[GIT_MAX_RAWSZ];
>  
>  	strbuf_addf(&buf, "%s:%"PRItime, path, stamp);
> -	hmac(hash, buf.buf, buf.len, cert_nonce_seed, strlen(cert_nonce_seed));
> +	hmac_hash(hash, buf.buf, buf.len, cert_nonce_seed, strlen(cert_nonce_seed));
>  	strbuf_release(&buf);
>  
>  	/* RFC 2104 5. HMAC-SHA1-80 */