| Message ID | 80f15223-246e-4cfb-a139-e47af829c938@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | use the pager in 'add -p' | expand |
Rubén Justo <rjusto@gmail.com> writes: > We send errors to the pager since 61b80509e3 (sending errors to stdout > under $PAGER, 2008-02-16). > > In a8335024c2 (pager: do not dup2 stderr if it is already redirected, > 2008-12-15) an exception was introduced to avoid redirecting stderr if > it is not connected to a terminal. > > In such exceptional cases, the close(STDERR_FILENO) we're doing in > close_pager_fds, is unnecessary. > Furthermore, in a subsequent commit we're going to introduce changes > that might call close_pager_fds multiple times. With this in mind, > unconditionally closing stderr will become undesirable. In a new world with such a change, what does it mean to call close_pager_fds()? It used to mean "we are really done with the pager and we no longer need them, ever". And we still call the helper for that purpose after this change, from wait_for_pager_atexit() and wait_for_pager_signal(). So no matter what "a subsequent commit" does, it feels conceptually wrong to call it more than once in the first place. In other words, what is wrong is that this function closes stderr, but "a subsequent commit" calls this function multiple times, no? > static struct child_process pager_process; > static const char *pager_program; > +static int old_fd2 = -1; What does the magic number "-1" mean? We often use it to signal "uninitialized", but then what are concrete "initialized" values mean? "We dup2()'ed something else to stderr/fd #2 but before doing so we saved the original fd #2 away to this variable, so that we can restore fd #2 by another dup2() of the value of this variable when we declare that we are done with the standard error stream"? But that does not look like what is happening here. > /* Is the value coming back from term_columns() just a guess? */ > static int term_columns_guessed; > @@ -23,7 +24,8 @@ static void close_pager_fds(void) > { > /* signal EOF to pager */ > close(1); > - close(2); > + if (old_fd2 != -1) > + close(2); > } > > static void wait_for_pager_atexit(void) > @@ -141,8 +143,10 @@ void setup_pager(void) > > /* original process continues, but writes to the pipe */ > dup2(pager_process.in, 1); > - if (isatty(2)) > + if (isatty(2)) { > + old_fd2 = 1; Equally unclear magic number "1" is used here. This value is different from pager_process.in, and my earlier "we are saving away" does not apply, either. > dup2(pager_process.in, 2); > + } > close(pager_process.in); Puzzled...
On Mon, May 20, 2024 at 12:14:04PM -0700, Junio C Hamano wrote: > Rubén Justo <rjusto@gmail.com> writes: > > > We send errors to the pager since 61b80509e3 (sending errors to stdout > > under $PAGER, 2008-02-16). > > > > In a8335024c2 (pager: do not dup2 stderr if it is already redirected, > > 2008-12-15) an exception was introduced to avoid redirecting stderr if > > it is not connected to a terminal. > > > > In such exceptional cases, the close(STDERR_FILENO) we're doing in > > close_pager_fds, is unnecessary. > > > > Furthermore, in a subsequent commit we're going to introduce changes > > that might call close_pager_fds multiple times. With this in mind, > > unconditionally closing stderr will become undesirable. > > In a new world with such a change, what does it mean to call > close_pager_fds()? There is no change in what calling close_pager_fds() means. > It used to mean "we are really done with the > pager and we no longer need them, ever". > > And we still call the helper for that purpose after this change, > from wait_for_pager_atexit() and wait_for_pager_signal(). Yes, no change here either. In the next commit, a new client of the helper is introduced, the new API: wait_for_pager(). > > So no matter what "a subsequent commit" does, it feels conceptually > wrong to call it more than once in the first place. > In other words, > what is wrong is that this function closes stderr, but "a subsequent > commit" calls this function multiple times, no? This series is trying to allow triggering the pager multiple times. Reaching to close_pager_fds() multiple times is a consequence of it. > > > static struct child_process pager_process; > > static const char *pager_program; > > +static int old_fd2 = -1; > > What does the magic number "-1" mean? Invalid fd. > We often use it to signal > "uninitialized", but then what are concrete "initialized" values > mean? "We dup2()'ed something else to stderr/fd #2 but before doing > so we saved the original fd #2 away to this variable, so that we can > restore fd #2 by another dup2() of the value of this variable when > we declare that we are done with the standard error stream"? > > But that does not look like what is happening here. > > > /* Is the value coming back from term_columns() just a guess? */ > > static int term_columns_guessed; > > @@ -23,7 +24,8 @@ static void close_pager_fds(void) > > { > > /* signal EOF to pager */ > > close(1); > > - close(2); > > + if (old_fd2 != -1) > > + close(2); > > } > > > > static void wait_for_pager_atexit(void) > > @@ -141,8 +143,10 @@ void setup_pager(void) > > > > /* original process continues, but writes to the pipe */ > > dup2(pager_process.in, 1); > > - if (isatty(2)) > > + if (isatty(2)) { > > + old_fd2 = 1; > > Equally unclear magic number "1" is used here. > > This value is different from pager_process.in, and my earlier "we > are saving away" does not apply, either. It applies, in 3/5. > > > dup2(pager_process.in, 2); > > + } > > close(pager_process.in); > > Puzzled... Thanks for reading the series.
Rubén Justo <rjusto@gmail.com> writes: >> > static struct child_process pager_process; >> > static const char *pager_program; >> > +static int old_fd2 = -1; >> >> What does the magic number "-1" mean? > > Invalid fd. > >> We often use it to signal >> "uninitialized", but then what are concrete "initialized" values >> mean? "We dup2()'ed something else to stderr/fd #2 but before doing >> so we saved the original fd #2 away to this variable, so that we can >> restore fd #2 by another dup2() of the value of this variable when >> we declare that we are done with the standard error stream"? >> >> But that does not look like what is happening here. >> .... >> Equally unclear magic number "1" is used here. >> >> This value is different from pager_process.in, and my earlier "we >> are saving away" does not apply, either. > > It applies, in 3/5. We need to be prepared to see a series chomped at an early stage and it should still make sense. If the series does not make sense when you stop before applying patch 3, it is a strong sign that this step and the next step can be separated and structured better. Or perhaps if they are made into a single patch it makes more sense and becomes easier to explain?
On Tue, May 21, 2024 at 01:57:19PM -0700, Junio C Hamano wrote: > Rubén Justo <rjusto@gmail.com> writes: > > >> > static struct child_process pager_process; > >> > static const char *pager_program; > >> > +static int old_fd2 = -1; > >> > >> What does the magic number "-1" mean? > > > > Invalid fd. > > > >> We often use it to signal > >> "uninitialized", but then what are concrete "initialized" values > >> mean? "We dup2()'ed something else to stderr/fd #2 but before doing > >> so we saved the original fd #2 away to this variable, so that we can > >> restore fd #2 by another dup2() of the value of this variable when > >> we declare that we are done with the standard error stream"? > >> > >> But that does not look like what is happening here. > >> .... > >> Equally unclear magic number "1" is used here. > >> > >> This value is different from pager_process.in, and my earlier "we > >> are saving away" does not apply, either. > > > > It applies, in 3/5. > > We need to be prepared to see a series chomped at an early stage and > it should still make sense. If the series does not make sense when > you stop before applying patch 3, it is a strong sign that this step > and the next step can be separated and structured better. > > Or perhaps if they are made into a single patch it makes more sense > and becomes easier to explain? > Adding logic to adjust when we close(stderr) in close_pager_fds() makes sense on its own, I think. And, the values for the flag "do-we-want-to-close-stderr-at-exit", too, to me. I am happy with the series; the 'P' command introduced in v2 is a good improvement. Combining 2/5 and 3/5, I think it is not a good idea. Therefore, I'm not sure how to alleviate the puzzling.
Rubén Justo <rjusto@gmail.com> writes: > Adding logic to adjust when we close(stderr) in close_pager_fds() makes > sense on its own, I think. The feature may be. > And, the values for the flag "do-we-want-to-close-stderr-at-exit", too, > to me. But the thing is, the flag is *NOT* named as such, and an undocumented "value -1 means X, while value 1 means Y", do not make any sense, either.
On Tue, May 21, 2024 at 03:00:10PM -0700, Junio C Hamano wrote: > Rubén Justo <rjusto@gmail.com> writes: > > > Adding logic to adjust when we close(stderr) in close_pager_fds() makes > > sense on its own, I think. > > The feature may be. > > > And, the values for the flag "do-we-want-to-close-stderr-at-exit", too, > > to me. > > But the thing is, the flag is *NOT* named as such, and an > undocumented "value -1 means X, while value 1 means Y", do not make > any sense, either Perhaps this makes more sense?: 1: 8fe915a820 ! 1: 70cc34efc4 pager: do not close fd 2 unnecessarily @@ pager.c: int pager_use_color = 1; static struct child_process pager_process; static const char *pager_program; -+static int old_fd2 = -1; ++static int old_fd2; /* Is the value coming back from term_columns() just a guess? */ static int term_columns_guessed; @@ pager.c: static void close_pager_fds(void) /* signal EOF to pager */ close(1); - close(2); -+ if (old_fd2 != -1) ++ if (old_fd2) + close(2); }
Rubén Justo <rjusto@gmail.com> writes: > Perhaps this makes more sense?: > > 1: 8fe915a820 ! 1: 70cc34efc4 pager: do not close fd 2 unnecessarily > @@ pager.c: int pager_use_color = 1; > > static struct child_process pager_process; > static const char *pager_program; > -+static int old_fd2 = -1; > ++static int old_fd2; > > /* Is the value coming back from term_columns() just a guess? */ > static int term_columns_guessed; > @@ pager.c: static void close_pager_fds(void) > /* signal EOF to pager */ > close(1); > - close(2); > -+ if (old_fd2 != -1) > ++ if (old_fd2) > + close(2); > } Not really. The name "old_fd2" strongly implies "where did fd#2 come from?" and it did not come from fd#0, did it? Until [3/5] this variable used to mean something different from "this was the saved fd#2 we can use to restore it later", which is the name "old_fd2" clearly wants to stand for. If you really want to have them as two separate patches, I would expect the proposed log message for the [3/5] step to say something like ... we added variable X to signal if we should close fd#2 in function F in the previous step. As store away the original fd#2 with dup(2) to be restored later after we close() it, the question the previous step asked, "should we be the one closing fd#2?" becomes equivalent to "have we stored away the original fd#2 (in which case we close() fd#2 when we are done with the pager and restore the original one)?" Rename X to old_fd2 and have it serve both purposes. or somesuch.
On Wed, May 22, 2024 at 10:40:18AM -0700, Junio C Hamano wrote: > Not really. The name "old_fd2" strongly implies "where did fd#2 > come from?" and it did not come from fd#0, did it? Perhaps "close_fd2" is a better name?: @@ pager.c: int pager_use_color = 1; static struct child_process pager_process; static const char *pager_program; -+static int old_fd2 = -1; ++static int close_fd2; /* Is the value coming back from term_columns() just a guess? */ static int term_columns_guessed; @@ pager.c: static void close_pager_fds(void) /* signal EOF to pager */ close(1); - close(2); -+ if (old_fd2 != -1) ++ if (close_fd2) + close(2); }
Rubén Justo <rjusto@gmail.com> writes: > On Wed, May 22, 2024 at 10:40:18AM -0700, Junio C Hamano wrote: > >> Not really. The name "old_fd2" strongly implies "where did fd#2 >> come from?" and it did not come from fd#0, did it? > > Perhaps "close_fd2" is a better name?: > @@ pager.c: static void close_pager_fds(void) > /* signal EOF to pager */ > close(1); > - close(2); > -+ if (old_fd2 != -1) > ++ if (close_fd2) > + close(2); That's a very straight-forward name that says what effect anybody who assigns to the variable wants to see.
diff --git a/pager.c b/pager.c index b8822a9381..3ef6798f7e 100644 --- a/pager.c +++ b/pager.c @@ -14,6 +14,7 @@ int pager_use_color = 1; static struct child_process pager_process; static const char *pager_program; +static int old_fd2 = -1; /* Is the value coming back from term_columns() just a guess? */ static int term_columns_guessed; @@ -23,7 +24,8 @@ static void close_pager_fds(void) { /* signal EOF to pager */ close(1); - close(2); + if (old_fd2 != -1) + close(2); } static void wait_for_pager_atexit(void) @@ -141,8 +143,10 @@ void setup_pager(void) /* original process continues, but writes to the pipe */ dup2(pager_process.in, 1); - if (isatty(2)) + if (isatty(2)) { + old_fd2 = 1; dup2(pager_process.in, 2); + } close(pager_process.in); /* this makes sure that the parent terminates after the pager */
We send errors to the pager since 61b80509e3 (sending errors to stdout under $PAGER, 2008-02-16). In a8335024c2 (pager: do not dup2 stderr if it is already redirected, 2008-12-15) an exception was introduced to avoid redirecting stderr if it is not connected to a terminal. In such exceptional cases, the close(STDERR_FILENO) we're doing in close_pager_fds, is unnecessary. Furthermore, in a subsequent commit we're going to introduce changes that might call close_pager_fds multiple times. With this in mind, unconditionally closing stderr will become undesirable. Let's close(STDERR_FILENO) only when necessary, and pave the way for the coming changes. Signed-off-by: Rubén Justo <rjusto@gmail.com> --- pager.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)