Re: [PATCH v8 7/7] *.c: use isatty(0|2), not isatty(STDIN_FILENO|STDERR_FILENO)

From: René Scharfe <l.s.r@web.de>
Date: Dec 28, 2021, 16:47 UTC
Message-ID: <d0bdc7e5-9e34-49d8-33cf-dd96a807617e@web.de>
In-Reply-To: <patch-v8-7.7-060483fb5ce-20211228T150728Z-avarab@gmail.com>

Am 28.12.21 um 16:19 schrieb Ævar Arnfjörð Bjarmason:

Show 8 quoted lines

> We have over 50 uses of "isatty(1)" and "isatty(2)" in the codebase,
> and around 10 "isatty(0)", but three callers used the
> {STDIN_FILENO,STD{OUT,ERR}_FILENO} macros in "stdlib.h" to refer to
> them.
>
> Let's change these for consistency.  This makes it easier to change
> all calls to isatty() at a whim, which is useful to test some
> scenarios[1].

Hmm. Matching e.g. "(0|STDIN_FILENO)" instead of "0" is harder, of course, but not much.

Shouldn't we use these macros more to reduce the number of magic values? The code is slightly easier to read before this patch because it doesn't require the reader to know the meaning of these numbers.

Reducing the constants to their numerical values is easy to automate in general; the opposite direction is harder. Coccinelle can help us take such a step with a semantic patch like this:

	@@
	@@
	  isatty(
	(
	- 0
	+ STDIN_FILENO
	|
	- 1
	+ STDOUT_FILENO
	|
	- 2
	+ STDERR_FILENO
	)
	  )

Show 49 quoted lines

>
> 1. https://lore.kernel.org/git/patch-v6-8.8-bff919994b5-20211102T122507Z-avarab@gmail.com/
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  builtin/bisect--helper.c | 2 +-
>  builtin/bundle.c         | 2 +-
>  compat/mingw.c           | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 28a2e6a5750..21360a4e70b 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -830,7 +830,7 @@ static int bisect_autostart(struct bisect_terms *terms)
>  	fprintf_ln(stderr, _("You need to start by \"git bisect "
>  			  "start\"\n"));
>
> -	if (!isatty(STDIN_FILENO))
> +	if (!isatty(0))
>  		return -1;
>
>  	/*
> diff --git a/builtin/bundle.c b/builtin/bundle.c
> index 5a85d7cd0fe..df69c651753 100644
> --- a/builtin/bundle.c
> +++ b/builtin/bundle.c
> @@ -56,7 +56,7 @@ static int parse_options_cmd_bundle(int argc,
>
>  static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {
>  	int all_progress_implied = 0;
> -	int progress = isatty(STDERR_FILENO);
> +	int progress = isatty(2);
>  	struct strvec pack_opts;
>  	int version = -1;
>  	int ret;
> diff --git a/compat/mingw.c b/compat/mingw.c
> index e14f2d5f77c..7c55d0f0414 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -2376,7 +2376,7 @@ int mingw_raise(int sig)
>  	switch (sig) {
>  	case SIGALRM:
>  		if (timer_fn == SIG_DFL) {
> -			if (isatty(STDERR_FILENO))
> +			if (isatty(2))
>  				fputs("Alarm clock\n", stderr);
>  			exit(128 + SIGALRM);
>  		} else if (timer_fn != SIG_IGN)