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

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

[Junio C Hamano]

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

[PATCH 1/3] pager: lose a separate argv[]

[Junio C Hamano]

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

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 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

[PATCH 2/3] pager: factor out a helper to prepare a child process to run the pager

[Junio C Hamano]

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 <gitster@pobox.com>
---
 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

Re: [PATCH 2/3] pager: factor out a helper to prepare a child process to run the pager

[Jeff King]

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

> 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");
> +}

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

Re: [PATCH 2/3] pager: factor out a helper to prepare a child process to run the pager

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Feb 16, 2016 at 03:06:56PM -0800, Junio C Hamano wrote:
>
>> 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");
>> +}
>
> 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.

;-)

I had that originally, and then incorrectly fed pager to va_start()
which broke the whole thing.

> 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.

After all that may be a nicer way to structure.

> 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).

Conceptually I am on the same page, but I am not sure how well that
interacts with what "git am -i" codepath wants to do, though.

One big difference between the "we'll feed our output to pager"
codepath and "we'll spawn a pager to let a file on the filesystem be
read" codepath is that the former needs to call git_pager() and
check the NULL-ness of the return value to decide that it does not
want to spawn a pager and let the standard output just go straight
to the outside world.  The latter, on the other hand, does want to
spawn something to cause the file to be presented to the end user
even git_pager() returns NULL.

And that is why I didn't make this helper call git_pager() itself.

Re: [PATCH 2/3] pager: factor out a helper to prepare a child process to run the pager

[Jeff King]

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

> > 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).
> 
> Conceptually I am on the same page, but I am not sure how well that
> interacts with what "git am -i" codepath wants to do, though.
> 
> One big difference between the "we'll feed our output to pager"
> codepath and "we'll spawn a pager to let a file on the filesystem be
> read" codepath is that the former needs to call git_pager() and
> check the NULL-ness of the return value to decide that it does not
> want to spawn a pager and let the standard output just go straight
> to the outside world.  The latter, on the other hand, does want to
> spawn something to cause the file to be presented to the end user
> even git_pager() returns NULL.
> 
> And that is why I didn't make this helper call git_pager() itself.

That makes sense. I didn't dig into it carefully. I saw the "pager=cat"
thing in the context of your diff to git-am, and assumed it was weird
fallback that should be done by the regular pager infrastructure. But
it's the exact thing you're talking about here.

So of all of the things I suggested, I think the non-varargs one that
takes "pager" as a string makes the most sense.

-Peff

[PATCH 3/3] am -i: fix "v"iew

[Junio C Hamano]

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 <gitster@pobox.com>
---
 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

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

[Junio C Hamano]

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

[PATCH v2 1/3] pager: lose a separate argv[]

[Junio C Hamano]

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

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * 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

[PATCH v2 2/3] pager: factor out a helper to prepare a child process to run the pager

[Junio C Hamano]

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 <gitster@pobox.com>
---
 * 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

[PATCH v2 3/3] am -i: fix "v"iew

[Junio C Hamano]

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 <gitster@pobox.com>
---
 * 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

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

[Jeff King]

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