threads / patch / 41427

patch, 3 partsfix "v"iew subcommand in "git am -i"

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

## tl;dr

12 messages between Feb 16, 2016 and Feb 17, 2016. Diffs are folded; open one to read it.

replies: 11people: 2as markdown or json

Junio C Hamano· Feb 16, 2016, 23:06 UTC · lore

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
Junio C Hamano· Feb 16, 2016, 23:06 UTC · re: Junio C Hamano · lore

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

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(-)
Show changes to pager.c +1 −3
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
Junio C Hamano· Feb 16, 2016, 23:06 UTC · re: Junio C Hamano · lore

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

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(-)
Show changes to 2 files +22 −6

cache.h, pager.c

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
Jeff King· Feb 16, 2016, 23:26 UTC · re: Junio C Hamano · lore

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

On Tue, Feb 16, 2016 at 03:06:56PM -0800, Junio C Hamano wrote:
Show 24 quoted lines
> 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
Junio C Hamano· Feb 16, 2016, 23:49 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 36 quoted lines
> 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.

Show 7 quoted lines
> 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.
Show 9 quoted lines
> 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.
Jeff King· Feb 17, 2016, 00:32 UTC · re: Junio C Hamano · lore

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

On Tue, Feb 16, 2016 at 03:49:55PM -0800, Junio C Hamano wrote:
Show 23 quoted lines
> > 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
Junio C Hamano· Feb 16, 2016, 23:06 UTC · re: Junio C Hamano · lore

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

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(-)
Show changes to builtin/am.c +3 −2
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
Junio C Hamano· Feb 17, 2016, 19:15 UTC · re: Junio C Hamano · lore

[PATCH v2 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. 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
Junio C Hamano· Feb 17, 2016, 19:15 UTC · re: Junio C Hamano · lore

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

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(-)
Show changes to pager.c +1 −3
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
Junio C Hamano· Feb 17, 2016, 19:15 UTC · re: Junio C Hamano · lore

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

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(-)
Show changes to 2 files +14 −6

cache.h, pager.c

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
Junio C Hamano· Feb 17, 2016, 19:15 UTC · re: Junio C Hamano · lore

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

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(-)
Show changes to builtin/am.c +1 −1
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
Jeff King· Feb 17, 2016, 19:19 UTC · re: Junio C Hamano · lore

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

On Wed, Feb 17, 2016 at 11:15:13AM -0800, Junio C Hamano wrote:
Show 17 quoted lines
> 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

← back to recent threads