| Message ID | patch-v8-7.7-060483fb5ce-20211228T150728Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | progress: test fixes / cleanup | expand |
Am 28.12.21 um 16:19 schrieb Ævar Arnfjörð Bjarmason: > 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 ) ) > > 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)
On Tue, Dec 28 2021, René Scharfe wrote: > Am 28.12.21 um 16:19 schrieb Ævar Arnfjörð Bjarmason: >> 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 > ) > ) We don't bother with EXIT_SUCCESS and EXIT_FAILURE, and for those (VMS) there is a reason to not use the constants, as EXIT_FAILURE may differ. But for these I personally think these symbolic names are rather useless. They never differ, and when working on POSIX systems you're going to need to know that 1 is stdout, 2 is stderr. You're also going to have to maintain shellscripts that use ">&2" or whatever. Those aren't using a hypothetical ">&$STDERR_FILENO". But in any case, this change isn't even trying to make the argument that we *should* use one over the other, just that the constants are used much more than *_FILENO, so changing them to make a subsequent (now ejected out of this series) change easier to explain is worth it. So I'd think we can just take this small change, and argue separately whether it's worth it to apply that coccinelle rule.
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)
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]. 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(-)