[PATCH 0/3] fix "v"iew subcommand in "git am -i"

The 'v'iew subcommand of the interactive mode of "git am -i" was
broken by the rewrite to C we did at around 2.6.0 timeframe at
7ff26832 (builtin-am: implement -i/--interactive, 2015-08-04); we
used to spawn the pager via the shell, accepting things like

	PAGER='less -S'

in the environment, but the rewrite forgot and tried to directly
spawn a command whose name is the entire string.

The bug is understandable, because there are things we need to do
other than just run_command() to run the pager, such as running it
with default LESS/LV settings and running it via the shell, but
these pieces of necessary knowledge about what is the right thing to
do are hoarded by the setup_pager() entry point, which is only good
if we are feeding our own standard output to the pager.  A codepath
that wants to run the pager but not on our output needs to do the
right thing on its own.

So the first patch in this series factors out a helper function to
let the caller run the pager the right way.  They make the third
patch to fix the breakage in "am" trivial.

I debated myself where the call of git_pager() should go (it could
be argued that it conceptually belongs to the new prepare_pager_args()
helper), but I opted for a simpler change.

Junio C Hamano (3):
  pager: lose a separate argv[]
  pager: factor out a helper to prepare a child process to run the pager
  am -i: fix "v"iew

 builtin/am.c |  5 +++--
 cache.h      |  4 ++++
 pager.c      | 26 ++++++++++++++++++--------
 3 files changed, 25 insertions(+), 10 deletions(-)

-- 
2.7.1-460-gd45d0a4

These days, using the embedded args array in the child_process
structure is the norm.  Follow that practice.

Signed-off-by: Junio C Hamano <redacted>
---
 pager.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/pager.c b/pager.c
index 070dc11..5dbcc5a 100644
--- a/pager.c
+++ b/pager.c

@@ -11,7 +11,6 @@
  * something different on Windows.
  */
 
-static const char *pager_argv[] = { NULL, NULL };
 static struct child_process pager_process = CHILD_PROCESS_INIT;
 
 static void wait_for_pager(void)

@@ -70,9 +69,8 @@ void setup_pager(void)
 	setenv("GIT_PAGER_IN_USE", "true", 1);
 
 	/* spawn the pager */
-	pager_argv[0] = pager;
+	argv_array_push(&pager_process.args, pager);
 	pager_process.use_shell = 1;
-	pager_process.argv = pager_argv;
 	pager_process.in = -1;
 	if (!getenv("LESS"))
 		argv_array_push(&pager_process.env_array, "LESS=FRX");

-- 
2.7.1-460-gd45d0a4

When running a pager, we need to run the program git_pager() gave
us, but we need to make sure we spawn it via the shell (i.e. it is
valid to say PAGER='less -S', for example) and give default values
to $LESS and $LV environment variables.  Factor out these details
to a separate helper function.

Signed-off-by: Junio C Hamano <redacted>
---
 cache.h |  4 ++++
 pager.c | 24 ++++++++++++++++++------
 2 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/cache.h b/cache.h
index 6bb7119..6827acb 100644
--- a/cache.h
+++ b/cache.h

@@ -210,7 +210,9 @@ struct cache_entry {
 #error "CE_EXTENDED_FLAGS out of range"
 #endif
 
+/* Forward structure decls */
 struct pathspec;
+struct child_process;
 
 /*
  * Copy the sha1 and stat state of a cache entry from one to

@@ -1550,6 +1552,8 @@ extern int pager_use_color;
 extern int term_columns(void);
 extern int decimal_width(uintmax_t);
 extern int check_pager_config(const char *cmd);
+LAST_ARG_MUST_BE_NULL
+extern void prepare_pager_args(struct child_process *, ...);
 
 extern const char *editor_program;
 extern const char *askpass_program;

diff --git a/pager.c b/pager.c
index 5dbcc5a..1406370 100644
--- a/pager.c
+++ b/pager.c

@@ -53,6 +53,23 @@ const char *git_pager(int stdout_is_tty)
 	return pager;
 }
 
+void prepare_pager_args(struct child_process *pager_process, ...)
+{
+	va_list ap;
+	const char *arg;
+
+	va_start(ap, pager_process);
+	while ((arg = va_arg(ap, const char *)))
+		argv_array_push(&pager_process->args, arg);
+	va_end(ap);
+
+	pager_process->use_shell = 1;
+	if (!getenv("LESS"))
+		argv_array_push(&pager_process->env_array, "LESS=FRX");
+	if (!getenv("LV"))
+		argv_array_push(&pager_process->env_array, "LV=-c");
+}
+
 void setup_pager(void)
 {
 	const char *pager = git_pager(isatty(1));

@@ -69,13 +86,8 @@ void setup_pager(void)
 	setenv("GIT_PAGER_IN_USE", "true", 1);
 
 	/* spawn the pager */
-	argv_array_push(&pager_process.args, pager);
-	pager_process.use_shell = 1;
+	prepare_pager_args(&pager_process, pager, NULL);
 	pager_process.in = -1;
-	if (!getenv("LESS"))
-		argv_array_push(&pager_process.env_array, "LESS=FRX");
-	if (!getenv("LV"))
-		argv_array_push(&pager_process.env_array, "LV=-c");
 	argv_array_push(&pager_process.env_array, "GIT_PAGER_IN_USE");
 	if (start_command(&pager_process))
 		return;

-- 
2.7.1-460-gd45d0a4

The 'v'iew subcommand of the interactive mode of "git am -i" was
broken by the rewrite to C we did at around 2.6.0 timeframe at
7ff26832 (builtin-am: implement -i/--interactive, 2015-08-04); we
used to spawn the pager via the shell, accepting things like

	PAGER='less -S'

in the environment, but the rewrite forgot and tried to directly
spawn a command whose name is the entire string.

The previous refactoring of the new helper function makes it easier
for us to do the right thing.

Signed-off-by: Junio C Hamano <redacted>
---
 builtin/am.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 1399c8d..aecf917 100644
--- a/builtin/am.c
+++ b/builtin/am.c

@@ -1740,8 +1740,9 @@ static int do_interactive(struct am_state *state)
 
 			if (!pager)
 				pager = "cat";
-			argv_array_push(&cp.args, pager);
-			argv_array_push(&cp.args, am_path(state, "patch"));
+
+			prepare_pager_args(&cp, pager,
+					   am_path(state, "patch"), NULL);
 			run_command(&cp);
 		}
 	}

-- 
2.7.1-460-gd45d0a4

On Tue, Feb 16, 2016 at 03:06:56PM -0800, Junio C Hamano wrote:

When reading this, I had to wonder what the "..." args were supposed to
be. I figured it out when I read the caller, but I wonder if a comment
would help. Also, we are expecting the pager here as the first argument,
so maybe:

  void prepare_pager_args(struct child_process *pager_process,
                          const char *pager, ...);

would be a better signature. That also made me wonder if we could simply
get away with:

  void prepare_pager_args(struct child_process *pager_process,
                          const char *pager);

and have callers argv_array_push() themselves afterwards.

And if you put the git_pager() call inside prepare_pager_args (which I
agree would be cleaner), we just have:

  void prepare_pager_args(struct child_process *pager_process);

which is pretty self-explanatory (though it might need a new name; I'd
be tempted to call it init_pager_process() or something, and actually
have it do the child_process_init() to make sure it is working with a
sane clean slate).

-Peff

These days, using the embedded args array in the child_process
structure is the norm.  Follow that practice.

Signed-off-by: Junio C Hamano <redacted>
---
 * Same as v1

 pager.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/pager.c b/pager.c
index 070dc11..5dbcc5a 100644
--- a/pager.c
+++ b/pager.c

@@ -11,7 +11,6 @@
  * something different on Windows.
  */
 
-static const char *pager_argv[] = { NULL, NULL };
 static struct child_process pager_process = CHILD_PROCESS_INIT;
 
 static void wait_for_pager(void)

@@ -70,9 +69,8 @@ void setup_pager(void)
 	setenv("GIT_PAGER_IN_USE", "true", 1);
 
 	/* spawn the pager */
-	pager_argv[0] = pager;
+	argv_array_push(&pager_process.args, pager);
 	pager_process.use_shell = 1;
-	pager_process.argv = pager_argv;
 	pager_process.in = -1;
 	if (!getenv("LESS"))
 		argv_array_push(&pager_process.env_array, "LESS=FRX");

-- 
2.7.1-489-g20b2cbe

The 'v'iew subcommand of the interactive mode of "git am -i" was
broken by the rewrite to C we did at around 2.6.0 timeframe at
7ff26832 (builtin-am: implement -i/--interactive, 2015-08-04); we
used to spawn the pager via the shell, accepting things like

	PAGER='less -S'

in the environment, but the rewrite forgot and tried to directly
spawn a command whose name is the entire string.

The bug is understandable, because there are things we need to do
other than just run_command() to run the pager, such as running it
with default LESS/LV settings and running it via the shell, but
these pieces of necessary knowledge about what is the right thing to
do are hoarded by the setup_pager() entry point, which is only good
if we are feeding our own standard output to the pager.  A codepath
that wants to run the pager but not on our output needs to do the
right thing on its own.

So the first patch in this series factors out a helper function to
let the caller run the pager the right way.  They make the third
patch to fix the breakage in "am" trivial.  Compared to v1, the
helper was much simplified with help by Peff: it always and only
takes child-process and the pager command string.  The caller can
append extra command line arguments after the helper returns if it
wants to.

Junio C Hamano (3):
  pager: lose a separate argv[]
  pager: factor out a helper to prepare a child process to run the pager
  am -i: fix "v"iew

 builtin/am.c |  2 +-
 cache.h      |  3 +++
 pager.c      | 19 +++++++++++--------
 3 files changed, 15 insertions(+), 9 deletions(-)

-- 
2.7.1-489-g20b2cbe

When running a pager, we need to run the program git_pager() gave
us, but we need to make sure we spawn it via the shell (i.e. it is
valid to say PAGER='less -S', for example) and give default values
to $LESS and $LV environment variables.  Factor out these details
to a separate helper function.

Signed-off-by: Junio C Hamano <redacted>
---
 * Simplified per Peff's suggestion.

 cache.h |  3 +++
 pager.c | 17 +++++++++++------
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/cache.h b/cache.h
index 6bb7119..a839acc 100644
--- a/cache.h
+++ b/cache.h

@@ -210,7 +210,9 @@ struct cache_entry {
 #error "CE_EXTENDED_FLAGS out of range"
 #endif
 
+/* Forward structure decls */
 struct pathspec;
+struct child_process;
 
 /*
  * Copy the sha1 and stat state of a cache entry from one to

@@ -1550,6 +1552,7 @@ extern int pager_use_color;
 extern int term_columns(void);
 extern int decimal_width(uintmax_t);
 extern int check_pager_config(const char *cmd);
+extern void prepare_pager_args(struct child_process *, const char *pager);
 
 extern const char *editor_program;
 extern const char *askpass_program;

diff --git a/pager.c b/pager.c
index 5dbcc5a..cb28207 100644
--- a/pager.c
+++ b/pager.c

@@ -53,6 +53,16 @@ const char *git_pager(int stdout_is_tty)
 	return pager;
 }
 
+void prepare_pager_args(struct child_process *pager_process, const char *pager)
+{
+	argv_array_push(&pager_process->args, pager);
+	pager_process->use_shell = 1;
+	if (!getenv("LESS"))
+		argv_array_push(&pager_process->env_array, "LESS=FRX");
+	if (!getenv("LV"))
+		argv_array_push(&pager_process->env_array, "LV=-c");
+}
+
 void setup_pager(void)
 {
 	const char *pager = git_pager(isatty(1));

@@ -69,13 +79,8 @@ void setup_pager(void)
 	setenv("GIT_PAGER_IN_USE", "true", 1);
 
 	/* spawn the pager */
-	argv_array_push(&pager_process.args, pager);
-	pager_process.use_shell = 1;
+	prepare_pager_args(&pager_process, pager);
 	pager_process.in = -1;
-	if (!getenv("LESS"))
-		argv_array_push(&pager_process.env_array, "LESS=FRX");
-	if (!getenv("LV"))
-		argv_array_push(&pager_process.env_array, "LV=-c");
 	argv_array_push(&pager_process.env_array, "GIT_PAGER_IN_USE");
 	if (start_command(&pager_process))
 		return;

-- 
2.7.1-489-g20b2cbe

On Wed, Feb 17, 2016 at 11:15:13AM -0800, Junio C Hamano wrote:

So the first patch in this series factors out a helper function to
let the caller run the pager the right way.  They make the third
patch to fix the breakage in "am" trivial.  Compared to v1, the
helper was much simplified with help by Peff: it always and only
takes child-process and the pager command string.  The caller can
append extra command line arguments after the helper returns if it
wants to.

Junio C Hamano (3):
  pager: lose a separate argv[]
  pager: factor out a helper to prepare a child process to run the pager
  am -i: fix "v"iew

 builtin/am.c |  2 +-
 cache.h      |  3 +++
 pager.c      | 19 +++++++++++--------
 3 files changed, 15 insertions(+), 9 deletions(-)

The whole thing looks good to me.

-Peff

The 'v'iew subcommand of the interactive mode of "git am -i" was
broken by the rewrite to C we did at around 2.6.0 timeframe at
7ff26832 (builtin-am: implement -i/--interactive, 2015-08-04); we
used to spawn the pager via the shell, accepting things like

	PAGER='less -S'

in the environment, but the rewrite forgot and tried to directly
spawn a command whose name is the entire string.

The previous refactoring of the new helper function makes it easier
for us to do the right thing.

Signed-off-by: Junio C Hamano <redacted>
---
 * Essentially the same as v1, modulo adjustment for the change in 2/3

 builtin/am.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/am.c b/builtin/am.c
index 1399c8d..56cf26e 100644
--- a/builtin/am.c
+++ b/builtin/am.c

@@ -1740,7 +1740,7 @@ static int do_interactive(struct am_state *state)
 
 			if (!pager)
 				pager = "cat";
-			argv_array_push(&cp.args, pager);
+			prepare_pager_args(&cp, pager);
 			argv_array_push(&cp.args, am_path(state, "patch"));
 			run_command(&cp);
 		}

-- 
2.7.1-489-g20b2cbe