Re: [PATCH v2] git-compat-util: undefine fileno if defined

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 9, 2019, 18:34 UTC
Message-ID: <xmqqsgwwizf6.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20190209023621.75255-1-dan.mcgregor@usask.ca>

Dan McGregor <dan.mcgregor@usask.ca> writes:

Show 6 quoted lines

> Commit 8dd2e88a92 ("http: support file handles for HTTP_KEEP_ERROR",
> 2019-01-10) introduced an implicit assumption that rewind, fileno, and
> fflush are functions. At least on FreeBSD fileno is not, and as such
> passing a void * failed.
>
> All systems tested (FreeBSD and NetBSD) that define fineo as a macro

s/fineo/fileno/

> also have a function defined. Undefine the macro on these systems so
> that the function is used.

That smells like the patch is assuming a bit too much. A platform you did not inspect may have it as a macro but the implementation may still be usable without breakage like the one we saw on FreeBSD, for example.

It also robs us the possibility of overriding fileno() with our own macro earlier in this file, like we do for many functions you see in the output from this:

 $ git grep '#define .* git' git-compat-util.h

In general, I'd like to see people thinking twice about an overly simple approach when they justify it with "this should work everywhere I know of".

Show 14 quoted lines

> Signed-off-by: Dan McGregor <dan.mcgregor@usask.ca>
> ---
>  git-compat-util.h | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/git-compat-util.h b/git-compat-util.h
> index 29a19902aa..b5489bbcf2 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -764,6 +764,15 @@ char *gitstrdup(const char *s);
>  extern FILE *git_fopen(const char*, const char*);
>  #endif
>  
> +/* Some systems (the various BSDs for eaxmple) define

Style:

/*
 * Our multi-line comment starts with a lone slash-star
 * and ends with a lone star-slash, like this.
 */

Show 11 quoted lines

> + * fileno as a macro as an optimization. All systems I
> + * know about also define it as a real funcion, so use
> + * the real function to allow passing void *s to fileno.
> + */
> +#ifdef fileno
> +# undef fileno
> +#endif
> +
>  #ifdef SNPRINTF_RETURNS_BOGUS
>  #ifdef snprintf
>  #undef snprintf