Re: [PATCH] tools: libsubcmd: Drop the less hack that was inherited from Git.

[Josh Poimboeuf]

On Tue, Jan 23, 2018 at 07:38:37PM -0500, Arvind Sankar wrote:
> We inherited this hack with the original code from the Git project. The
> select call is invalid as the two fd_set pointers should not be aliased.
> 
> We could fix it, but the Git project removed this hack in 2012 in commit
> e8320f3 (pager: drop "wait for output to run less" hack). The bug it
> worked around was apparently fixed in less back in June 2007.
> 
> So remove the hack from here as well.
> 
> Signed-off-by: Arvind Sankar <nivedita@xxxxxxxxxxxx>

Looks good to me.

  Acked-by: Josh Poimboeuf <jpoimboe@xxxxxxxxxx>

Libsubcmd is used by perf and objtool, so adding the perf maintainers to
CC.  Arnaldo, do you want to pick this one up?

> ---
>  tools/lib/subcmd/pager.c       | 17 -----------------
>  tools/lib/subcmd/run-command.c |  2 --
>  tools/lib/subcmd/run-command.h |  1 -
>  3 files changed, 20 deletions(-)
> 
> diff --git a/tools/lib/subcmd/pager.c b/tools/lib/subcmd/pager.c
> index 5ba754d17952..94d61d9b511f 100644
> --- a/tools/lib/subcmd/pager.c
> +++ b/tools/lib/subcmd/pager.c
> @@ -1,5 +1,4 @@
>  // SPDX-License-Identifier: GPL-2.0
> -#include <sys/select.h>
>  #include <stdlib.h>
>  #include <stdio.h>
>  #include <string.h>
> @@ -23,21 +22,6 @@ void pager_init(const char *pager_env)
>  	subcmd_config.pager_env = pager_env;
>  }
>  
> -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);
> -
> -	setenv("LESS", "FRSX", 0);
> -}
> -
>  static const char *pager_argv[] = { "sh", "-c", NULL, NULL };
>  static struct child_process pager_process;
>  
> @@ -84,7 +68,6 @@ void setup_pager(void)
>  	pager_argv[2] = pager;
>  	pager_process.argv = pager_argv;
>  	pager_process.in = -1;
> -	pager_process.preexec_cb = pager_preexec;
>  
>  	if (start_command(&pager_process))
>  		return;
> diff --git a/tools/lib/subcmd/run-command.c b/tools/lib/subcmd/run-command.c
> index 5cdac2162532..9e9dca717ed7 100644
> --- a/tools/lib/subcmd/run-command.c
> +++ b/tools/lib/subcmd/run-command.c
> @@ -120,8 +120,6 @@ int start_command(struct child_process *cmd)
>  					unsetenv(*cmd->env);
>  			}
>  		}
> -		if (cmd->preexec_cb)
> -			cmd->preexec_cb();
>  		if (cmd->exec_cmd) {
>  			execv_cmd(cmd->argv);
>  		} else {
> diff --git a/tools/lib/subcmd/run-command.h b/tools/lib/subcmd/run-command.h
> index 17d969c6add3..6256268802b5 100644
> --- a/tools/lib/subcmd/run-command.h
> +++ b/tools/lib/subcmd/run-command.h
> @@ -46,7 +46,6 @@ struct child_process {
>  	unsigned no_stderr:1;
>  	unsigned exec_cmd:1; /* if this is to be external sub-command */
>  	unsigned stdout_to_stderr:1;
> -	void (*preexec_cb)(void);
>  };
>  
>  int start_command(struct child_process *);
> -- 
> 2.13.6
> 

-- 
Josh