Re: [PATCH] pager: drop "wait for output to run less" hack
Jeff King <peff@peff.net> writes:
Show 19 quoted lines
> On Tue, Jun 05, 2012 at 02:56:28AM -0400, Jeff King wrote:
>
>> > +static int fd_to_close;
>> > +void close_fd_prexec_cb(void)
>> > +{
>> > + if(debug)
>> > + fprintf(stderr, "close_fd_prexec_cb closing %d\n", fd_to_close);
>> > + close(fd_to_close);
>> > +}
>>
>> Note that preexec_cb does not work at all on Windows, as it assumes a
>> forking model (rather than a spawn, which leaves no room to execute
>> arbitrary code in the child). If all you want to do is open an extra
>> pipe, then probably run-command should be extended to handle this
>> (though I have no idea how complex that would be for the Windows side of
>> things, it is at least _possible_, as opposed to preexec_cb, which will
>> never be possible).
>
> I'm really tempted to do this:Why (I am slower than my usual slow self today, so pardon me)?
Aren't these already protected with "ifndef WIN32"?
Show 15 quoted lines
>
> -- >8 --
> Commit 35ce862 (pager: Work around window resizing bug in
> 'less', 2007-01-24) causes git's pager sub-process to wait
> to receive input after forking but before exec-ing the
> pager. To handle this, run-command had to grow a "pre-exec
> callback" feature. Unfortunately, this feature does not work
> at all on Windows (where we do not fork), and interacts
> poorly with run-command's parent notification system. Its
> use should be discouraged.
>
> The bug in less was fixed in version 406, which was released
> in June 2007. It is probably safe at this point to remove
> our workaround. That lets us rip out the preexec_cb feature
> entirely.
Show 7 quoted lines
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I checked, and even RHEL5 is on less 436. So besides people on antique
> "I installed less from source more than 5 years ago" systems, my only
> concern would be that some other pager depends on this hack in a weird
> way. But I have never heard of such a thing, so...
Show 74 quoted lines
> pager.c | 18 ------------------
> run-command.c | 10 ----------
> run-command.h | 1 -
> 3 files changed, 29 deletions(-)
>
> diff --git a/pager.c b/pager.c
> index 4dcb08d..c0b4387 100644
> --- a/pager.c
> +++ b/pager.c
> @@ -11,21 +11,6 @@
> * something different on Windows.
> */
>
> -#ifndef WIN32
> -static void pager_preexec(void)
> -{
> - /*
> - * Work around bug in "less" by not starting it until we
> - * have real input
> - */
> - fd_set in;
> -
> - FD_ZERO(&in);
> - FD_SET(0, &in);
> - select(1, &in, NULL, &in, NULL);
> -}
> -#endif
> -
> static const char *pager_argv[] = { NULL, NULL };
> static struct child_process pager_process;
>
> @@ -93,9 +78,6 @@ void setup_pager(void)
> static const char *env[] = { "LESS=FRSX", NULL };
> pager_process.env = env;
> }
> -#ifndef WIN32
> - pager_process.preexec_cb = pager_preexec;
> -#endif
> if (start_command(&pager_process))
> return;
>
> diff --git a/run-command.c b/run-command.c
> index 606791d..dff28a7 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -394,16 +394,6 @@ fail_pipe:
> unsetenv(*cmd->env);
> }
> }
> - if (cmd->preexec_cb) {
> - /*
> - * We cannot predict what the pre-exec callback does.
> - * Forgo parent notification.
> - */
> - close(child_notifier);
> - child_notifier = -1;
> -
> - cmd->preexec_cb();
> - }
> if (cmd->git_cmd) {
> execv_git_cmd(cmd->argv);
> } else if (cmd->use_shell) {
> diff --git a/run-command.h b/run-command.h
> index 44f7d2b..850c638 100644
> --- a/run-command.h
> +++ b/run-command.h
> @@ -39,7 +39,6 @@ struct child_process {
> unsigned stdout_to_stderr:1;
> unsigned use_shell:1;
> unsigned clean_on_exit:1;
> - void (*preexec_cb)(void);
> };
>
> int start_command(struct child_process *);Junio C Hamano <gitster@pobox.com>