From: Junio C Hamano <gitster@pobox•com>
To: "Rubén Justo" <rjusto@gmail•com>
Cc: Git List <git@vger•kernel.org>,
Dragan Simic <dsimic@manjaro•org>, Jeff King <peff@peff•net>
Subject: Re: [PATCH v4 3/6] pager: introduce wait_for_pager
Date: Tue, 04 Jun 2024 09:25:34 -0700 [thread overview]
Message-ID: <xmqqsexszncx.fsf@gitster.g> (raw)
In-Reply-To: <76c725b4-1bc4-4916-81d8-98cad8fc4ca0@gmail.com> ("Rubén Justo"'s message of "Mon, 3 Jun 2024 22:38:19 +0200")
Rubén Justo <rjusto@gmail•com> writes:
> static struct child_process pager_process;
> static const char *pager_program;
> -static int close_fd2;
> +static int old_fd1 = -1, old_fd2 = -1;
;-)
Presumably when old_fd2 does not have a valid value (i.e. -1) it
means we did not do dup2() to save it away, and we refrain from
closing #2 in that case?
It is curious that #1 did not have similar problem 2/6 addressed,
as we never redirected it with dup() to save it away. But now we
do for some reason that the proposed log message did not explain.
We should say something like
We need to take back the standard output and the standard error
stream after we are done with an invocation of the pager. For
that, save away the original file descriptors 1 and 2 when
spawning the pager, and restore them once the pager exits. The
presence of saved fd#2 can be used to replace the "close_fd2"
flag introduced in the previous patch.
perhaps.
> /* Is the value coming back from term_columns() just a guess? */
> static int term_columns_guessed;
> @@ -24,20 +24,41 @@ static void close_pager_fds(void)
> {
> /* signal EOF to pager */
> close(1);
> - if (close_fd2)
> + if (old_fd2 != -1)
> close(2);
> }
OK.
> static void wait_for_pager_atexit(void)
> {
> + if (old_fd1 == -1)
> + return;
> +
> fflush(stdout);
> fflush(stderr);
> close_pager_fds();
> finish_command(&pager_process);
> }
>
> +void wait_for_pager(void)
> +{
> + if (old_fd1 == -1)
> + return;
> +
> + wait_for_pager_atexit();
> + unsetenv("GIT_PAGER_IN_USE");
> + dup2(old_fd1, 1);
> + old_fd1 = -1;
> + if (old_fd2 != -1) {
> + dup2(old_fd2, 2);
> + old_fd2 = -1;
> + }
> +}
Presumably these use old_fd1's validity as a signal to see if have
pager running that need to be cleaned up? It feels a bit unnatural
why we do not ask about such a process the structure that is set up
to control it, namely, the pager_process structure, but this is OK
for now.
It is just a naming issue, but it smells strange that the normal
code path (wait_for_pager()) calls a function that is an atexit
handler, which is more specific and only to be called atexit.
I would have made
static void finish_pager(void)
{
fflush(stdout);
fflush(stderr);
close_pager_fds();
finish_command(&pager_process);
}
and then called it from the atexit handler and wait_for_pager().
> @@ -113,6 +134,7 @@ void prepare_pager_args(struct child_process *pager_process, const char *pager)
>
> void setup_pager(void)
> {
> + static int once = 0;
> const char *pager = git_pager(isatty(1));
>
> if (!pager)
> @@ -142,16 +164,18 @@ void setup_pager(void)
> return;
>
> /* original process continues, but writes to the pipe */
> + old_fd1 = dup(1);
> dup2(pager_process.in, 1);
> if (isatty(2)) {
> - close_fd2 = 1;
> + old_fd2 = dup(2);
> dup2(pager_process.in, 2);
> }
> close(pager_process.in);
Can dup(2) fail and return -1?
>
> - /* this makes sure that the parent terminates after the pager */
> - sigchain_push_common(wait_for_pager_signal);
> - atexit(wait_for_pager_atexit);
> + if (!once++) {
> + sigchain_push_common(wait_for_pager_signal);
> + atexit(wait_for_pager_atexit);
> + }
> }
Can we give a name better than "once" to this thing?
> int pager_in_use(void)
> diff --git a/pager.h b/pager.h
> index b77433026d..103ecac476 100644
> --- a/pager.h
> +++ b/pager.h
> @@ -5,6 +5,7 @@ struct child_process;
>
> const char *git_pager(int stdout_is_tty);
> void setup_pager(void);
> +void wait_for_pager(void);
> int pager_in_use(void);
> int term_columns(void);
> void term_clear_line(void);
Other than that, overall it looks good.
Thanks, will queue.
next prev parent reply other threads:[~2024-06-04 16:25 UTC|newest]
Thread overview: 113+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-19 7:06 [PATCH 0/5] use the pager in 'add -p' Rubén Justo
2024-05-19 7:10 ` [PATCH 1/5] add-patch: test for 'p' command Rubén Justo
2024-05-19 7:12 ` [PATCH 2/5] pager: do not close fd 2 unnecessarily Rubén Justo
2024-05-20 19:14 ` Junio C Hamano
2024-05-20 22:33 ` Rubén Justo
2024-05-21 20:57 ` Junio C Hamano
2024-05-21 21:35 ` Rubén Justo
2024-05-21 22:00 ` Junio C Hamano
2024-05-22 17:19 ` Rubén Justo
2024-05-22 17:40 ` Junio C Hamano
2024-05-26 6:48 ` Rubén Justo
2024-05-26 21:26 ` Junio C Hamano
2024-05-19 7:13 ` [PATCH 3/5] pager: introduce wait_for_pager Rubén Justo
2024-05-19 7:14 ` [PATCH 4/5] test-terminal: introduce --no-stdin-pty Rubén Justo
2024-05-19 7:14 ` [PATCH 5/5] add-patch: render hunks through the pager Rubén Justo
2024-05-20 19:30 ` Junio C Hamano
2024-05-20 19:45 ` Dragan Simic
2024-05-20 22:35 ` Rubén Justo
2024-05-20 23:54 ` Dragan Simic
2024-05-21 19:56 ` Rubén Justo
2024-05-21 7:07 ` Jeff King
2024-05-21 19:59 ` Rubén Justo
2024-05-23 9:06 ` Jeff King
2024-05-23 14:00 ` Junio C Hamano
2024-05-23 14:18 ` Dragan Simic
2024-05-23 23:04 ` Junio C Hamano
2024-05-23 23:28 ` Dragan Simic
2024-05-23 23:43 ` Dragan Simic
2024-05-23 23:54 ` Junio C Hamano
2024-05-23 23:57 ` Dragan Simic
2024-05-25 4:54 ` Jeff King
2024-05-23 22:25 ` Rubén Justo
2024-05-23 23:03 ` Dragan Simic
2024-05-20 22:47 ` Rubén Justo
2024-05-20 23:18 ` Junio C Hamano
2024-05-20 23:27 ` Rubén Justo
2024-05-21 20:49 ` [PATCH v2 0/5] use the pager in 'add -p' Rubén Justo
2024-05-21 20:51 ` [PATCH v2 1/5] add-patch: test for 'p' command Rubén Justo
2024-05-21 20:52 ` [PATCH v2 2/5] pager: do not close fd 2 unnecessarily Rubén Justo
2024-05-21 20:52 ` [PATCH v2 3/5] pager: introduce wait_for_pager Rubén Justo
2024-05-21 20:52 ` [PATCH v2 4/5] test-terminal: introduce --no-stdin-pty Rubén Justo
2024-05-21 20:52 ` [PATCH v2 5/5] add-patch: render hunks through the pager Rubén Justo
2024-05-22 8:09 ` Dragan Simic
2024-05-22 18:47 ` Junio C Hamano
2024-05-22 21:23 ` Rubén Justo
2024-05-22 21:27 ` Dragan Simic
2024-06-02 15:38 ` [PATCH v3 0/6] use the pager in 'add -p' Rubén Justo
2024-06-02 15:42 ` [PATCH v3 1/6] add-patch: test for 'p' command Rubén Justo
2024-06-02 15:42 ` [PATCH v3 2/6] pager: do not close fd 2 unnecessarily Rubén Justo
2024-06-02 15:43 ` [PATCH v3 3/6] pager: introduce wait_for_pager Rubén Justo
2024-06-02 15:43 ` [PATCH v3 4/6] pager: introduce setup_custom_pager Rubén Justo
2024-06-02 15:43 ` [PATCH v3 5/6] test-terminal: introduce --no-stdin-pty Rubén Justo
2024-06-02 15:44 ` [PATCH v3 6/6] add-patch: introduce the command '|' Rubén Justo
2024-06-02 16:36 ` [PATCH v3 0/6] use the pager in 'add -p' Junio C Hamano
2024-06-02 17:11 ` Junio C Hamano
2024-06-02 17:33 ` Rubén Justo
2024-06-02 17:13 ` Rubén Justo
2024-06-02 17:46 ` Dragan Simic
2024-06-03 9:03 ` Junio C Hamano
2024-06-03 10:21 ` Dragan Simic
2024-06-03 15:28 ` Junio C Hamano
2024-06-04 17:34 ` Dragan Simic
2024-06-02 17:36 ` Dragan Simic
2024-06-03 16:01 ` Junio C Hamano
2024-06-04 17:41 ` Dragan Simic
2024-06-04 17:42 ` Dragan Simic
2024-06-03 20:19 ` Rubén Justo
2024-06-04 18:13 ` Dragan Simic
2024-06-03 20:35 ` [PATCH v4 " Rubén Justo
2024-06-03 20:38 ` [PATCH v4 1/6] add-patch: test for 'p' command Rubén Justo
2024-06-03 20:38 ` [PATCH v4 2/6] pager: do not close fd 2 unnecessarily Rubén Justo
2024-06-04 15:50 ` Junio C Hamano
2024-06-03 20:38 ` [PATCH v4 3/6] pager: introduce wait_for_pager Rubén Justo
2024-06-04 10:00 ` Phillip Wood
2024-06-04 16:29 ` Junio C Hamano
2024-06-05 22:03 ` Rubén Justo
2024-06-04 16:25 ` Junio C Hamano [this message]
2024-06-03 20:38 ` [PATCH v4 4/6] pager: introduce setup_custom_pager Rubén Justo
2024-06-04 16:43 ` Junio C Hamano
2024-06-03 20:38 ` [PATCH v4 5/6] test-terminal: introduce --no-stdin-pty Rubén Justo
2024-06-04 10:05 ` Phillip Wood
2024-06-04 10:33 ` Jeff King
2024-06-05 15:39 ` Junio C Hamano
2024-06-06 8:24 ` Jeff King
2024-06-05 22:50 ` Rubén Justo
2024-06-06 8:27 ` Jeff King
2024-06-09 7:26 ` Rubén Justo
2024-06-03 20:38 ` [PATCH v4 6/6] add-patch: introduce the command '|' Rubén Justo
2024-06-04 17:12 ` Junio C Hamano
2024-06-04 20:05 ` Dragan Simic
2024-06-05 5:16 ` Junio C Hamano
2024-06-04 10:17 ` [PATCH v3 0/6] use the pager in 'add -p' Jeff King
2024-06-04 15:32 ` Junio C Hamano
2024-06-05 9:09 ` Jeff King
2024-06-05 13:21 ` Phillip Wood
2024-06-08 5:54 ` Dragan Simic
2024-06-09 7:44 ` Rubén Justo
2024-06-09 7:57 ` Dragan Simic
2024-06-10 19:09 ` Rubén Justo
2024-06-10 21:02 ` Dragan Simic
2024-06-10 14:09 ` Phillip Wood
2024-06-10 16:13 ` Junio C Hamano
2024-06-10 19:14 ` Rubén Justo
2024-06-10 19:56 ` Junio C Hamano
2024-06-10 21:08 ` Dragan Simic
2024-06-10 19:28 ` Dragan Simic
2024-06-10 20:08 ` Junio C Hamano
2024-06-10 21:16 ` Dragan Simic
2024-06-09 14:29 ` phillip.wood123
2024-06-09 17:20 ` Dragan Simic
2024-06-10 8:27 ` Phillip Wood
2024-06-10 9:09 ` Dragan Simic
2024-06-05 17:24 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqsexszncx.fsf@gitster.g \
--to=gitster@pobox$(echo .)com \
--cc=dsimic@manjaro$(echo .)org \
--cc=git@vger$(echo .)kernel.org \
--cc=peff@peff$(echo .)net \
--cc=rjusto@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox