{"thread":{"id":"41427","subject":"[PATCH 0/3] fix \"v\"iew subcommand in \"git am -i\"","startedAt":"2016-02-16T23:06:54Z","lastAt":"2016-02-17T19:19:24Z","messageCount":12,"participants":["Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"278404","messageId":"1455664017-27588-1-git-send-email-gitster@pobox.com","threadId":"41427","inReplyTo":null,"subject":"[PATCH 0/3] fix \"v\"iew subcommand in \"git am -i\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-16T23:06:54Z","receivedAt":"2016-02-16T23:06:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The 'v'iew subcommand of the interactive mode of \"git am -i\" was\nbroken by the rewrite to C we did at around 2.6.0 timeframe at\n7ff26832 (builtin-am: implement -i/--interactive, 2015-08-04); we\nused to spawn the pager via the shell, accepting things like\n\n\tPAGER='less -S'\n\nin the environment, but the rewrite forgot and tried to directly\nspawn a command whose name is the entire string.\n\nThe bug is understandable, because there are things we need to do\nother than just run_command() to run the pager, such as running it\nwith default LESS/LV settings and running it via the shell, but\nthese pieces of necessary knowledge about what is the right thing to\ndo are hoarded by the setup_pager() entry point, which is only good\nif we are feeding our own standard output to the pager. A codepath\nthat wants to run the pager but not on our output needs to do the\nright thing on its own.\n\nSo the first patch in this series factors out a helper function to\nlet the caller run the pager the right way. They make the third\npatch to fix the breakage in \"am\" trivial.\n\nI debated myself where the call of git_pager() should go (it could\nbe argued that it conceptually belongs to the new prepare_pager_args()\nhelper), but I opted for a simpler change.\n\nJunio C Hamano (3):\n pager: lose a separate argv[]\n pager: factor out a helper to prepare a child process to run the pager\n am -i: fix \"v\"iew\n\n builtin/am.c | 5 +++--\n cache.h | 4 ++++\n pager.c | 26 ++++++++++++++++++--------\n 3 files changed, 25 insertions(+), 10 deletions(-)\n\n-- \n2.7.1-460-gd45d0a4\n"},{"id":"278405","messageId":"1455664017-27588-2-git-send-email-gitster@pobox.com","threadId":"41427","inReplyTo":"1455664017-27588-1-git-send-email-gitster@pobox.com","subject":"[PATCH 1/3] pager: lose a separate argv[]","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-16T23:06:55Z","receivedAt":"2016-02-16T23:06:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"These days, using the embedded args array in the child_process\nstructure is the norm. Follow that practice.\n\nSigned-off-by: Junio C Hamano \n---\n pager.c | 4 +---\n 1 file changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/pager.c b/pager.c\nindex 070dc11..5dbcc5a 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -11,7 +11,6 @@\n * something different on Windows.\n */\n \n-static const char *pager_argv[] = { NULL, NULL };\n static struct child_process pager_process = CHILD_PROCESS_INIT;\n \n static void wait_for_pager(void)\n@@ -70,9 +69,8 @@ void setup_pager(void)\n \tsetenv(\"GIT_PAGER_IN_USE\", \"true\", 1);\n \n \t/* spawn the pager */\n-\tpager_argv[0] = pager;\n+\targv_array_push(&pager_process.args, pager);\n \tpager_process.use_shell = 1;\n-\tpager_process.argv = pager_argv;\n \tpager_process.in = -1;\n \tif (!getenv(\"LESS\"))\n \t\targv_array_push(&pager_process.env_array, \"LESS=FRX\");\n-- \n2.7.1-460-gd45d0a4\n"},{"id":"278406","messageId":"1455664017-27588-3-git-send-email-gitster@pobox.com","threadId":"41427","inReplyTo":"1455664017-27588-1-git-send-email-gitster@pobox.com","subject":"[PATCH 2/3] pager: factor out a helper to prepare a child process to run the pager","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-16T23:06:56Z","receivedAt":"2016-02-16T23:06:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When running a pager, we need to run the program git_pager() gave\nus, but we need to make sure we spawn it via the shell (i.e. it is\nvalid to say PAGER='less -S', for example) and give default values\nto $LESS and $LV environment variables. Factor out these details\nto a separate helper function.\n\nSigned-off-by: Junio C Hamano \n---\n cache.h | 4 ++++\n pager.c | 24 ++++++++++++++++++------\n 2 files changed, 22 insertions(+), 6 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 6bb7119..6827acb 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -210,7 +210,9 @@ struct cache_entry {\n #error \"CE_EXTENDED_FLAGS out of range\"\n #endif\n \n+/* Forward structure decls */\n struct pathspec;\n+struct child_process;\n \n /*\n * Copy the sha1 and stat state of a cache entry from one to\n@@ -1550,6 +1552,8 @@ extern int pager_use_color;\n extern int term_columns(void);\n extern int decimal_width(uintmax_t);\n extern int check_pager_config(const char *cmd);\n+LAST_ARG_MUST_BE_NULL\n+extern void prepare_pager_args(struct child_process *, ...);\n \n extern const char *editor_program;\n extern const char *askpass_program;\ndiff --git a/pager.c b/pager.c\nindex 5dbcc5a..1406370 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -53,6 +53,23 @@ const char *git_pager(int stdout_is_tty)\n \treturn pager;\n }\n \n+void prepare_pager_args(struct child_process *pager_process, ...)\n+{\n+\tva_list ap;\n+\tconst char *arg;\n+\n+\tva_start(ap, pager_process);\n+\twhile ((arg = va_arg(ap, const char *)))\n+\t\targv_array_push(&pager_process->args, arg);\n+\tva_end(ap);\n+\n+\tpager_process->use_shell = 1;\n+\tif (!getenv(\"LESS\"))\n+\t\targv_array_push(&pager_process->env_array, \"LESS=FRX\");\n+\tif (!getenv(\"LV\"))\n+\t\targv_array_push(&pager_process->env_array, \"LV=-c\");\n+}\n+\n void setup_pager(void)\n {\n \tconst char *pager = git_pager(isatty(1));\n@@ -69,13 +86,8 @@ void setup_pager(void)\n \tsetenv(\"GIT_PAGER_IN_USE\", \"true\", 1);\n \n \t/* spawn the pager */\n-\targv_array_push(&pager_process.args, pager);\n-\tpager_process.use_shell = 1;\n+\tprepare_pager_args(&pager_process, pager, NULL);\n \tpager_process.in = -1;\n-\tif (!getenv(\"LESS\"))\n-\t\targv_array_push(&pager_process.env_array, \"LESS=FRX\");\n-\tif (!getenv(\"LV\"))\n-\t\targv_array_push(&pager_process.env_array, \"LV=-c\");\n \targv_array_push(&pager_process.env_array, \"GIT_PAGER_IN_USE\");\n \tif (start_command(&pager_process))\n \t\treturn;\n-- \n2.7.1-460-gd45d0a4\n"},{"id":"278407","messageId":"1455664017-27588-4-git-send-email-gitster@pobox.com","threadId":"41427","inReplyTo":"1455664017-27588-1-git-send-email-gitster@pobox.com","subject":"[PATCH 3/3] am -i: fix \"v\"iew","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-16T23:06:57Z","receivedAt":"2016-02-16T23:06:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The 'v'iew subcommand of the interactive mode of \"git am -i\" was\nbroken by the rewrite to C we did at around 2.6.0 timeframe at\n7ff26832 (builtin-am: implement -i/--interactive, 2015-08-04); we\nused to spawn the pager via the shell, accepting things like\n\n\tPAGER='less -S'\n\nin the environment, but the rewrite forgot and tried to directly\nspawn a command whose name is the entire string.\n\nThe previous refactoring of the new helper function makes it easier\nfor us to do the right thing.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/am.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 1399c8d..aecf917 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1740,8 +1740,9 @@ static int do_interactive(struct am_state *state)\n \n \t\t\tif (!pager)\n \t\t\t\tpager = \"cat\";\n-\t\t\targv_array_push(&cp.args, pager);\n-\t\t\targv_array_push(&cp.args, am_path(state, \"patch\"));\n+\n+\t\t\tprepare_pager_args(&cp, pager,\n+\t\t\t\t\t am_path(state, \"patch\"), NULL);\n \t\t\trun_command(&cp);\n \t\t}\n \t}\n-- \n2.7.1-460-gd45d0a4\n"},{"id":"278411","messageId":"20160216232657.GB18634@sigill.intra.peff.net","threadId":"41427","inReplyTo":"1455664017-27588-3-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 2/3] pager: factor out a helper to prepare a child process to run the pager","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-16T23:26:58Z","receivedAt":"2016-02-16T23:26:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 16, 2016 at 03:06:56PM -0800, Junio C Hamano wrote:\n\n> diff --git a/pager.c b/pager.c\n> index 5dbcc5a..1406370 100644\n> --- a/pager.c\n> +++ b/pager.c\n> @@ -53,6 +53,23 @@ const char *git_pager(int stdout_is_tty)\n> \treturn pager;\n> }\n> \n> +void prepare_pager_args(struct child_process *pager_process, ...)\n> +{\n> +\tva_list ap;\n> +\tconst char *arg;\n> +\n> +\tva_start(ap, pager_process);\n> +\twhile ((arg = va_arg(ap, const char *)))\n> +\t\targv_array_push(&pager_process->args, arg);\n> +\tva_end(ap);\n> +\n> +\tpager_process->use_shell = 1;\n> +\tif (!getenv(\"LESS\"))\n> +\t\targv_array_push(&pager_process->env_array, \"LESS=FRX\");\n> +\tif (!getenv(\"LV\"))\n> +\t\targv_array_push(&pager_process->env_array, \"LV=-c\");\n> +}\n\nWhen reading this, I had to wonder what the \"...\" args were supposed to\nbe. I figured it out when I read the caller, but I wonder if a comment\nwould help. Also, we are expecting the pager here as the first argument,\nso maybe:\n\n void prepare_pager_args(struct child_process *pager_process,\n const char *pager, ...);\n\nwould be a better signature. That also made me wonder if we could simply\nget away with:\n\n void prepare_pager_args(struct child_process *pager_process,\n const char *pager);\n\nand have callers argv_array_push() themselves afterwards.\n\nAnd if you put the git_pager() call inside prepare_pager_args (which I\nagree would be cleaner), we just have:\n\n void prepare_pager_args(struct child_process *pager_process);\n\nwhich is pretty self-explanatory (though it might need a new name; I'd\nbe tempted to call it init_pager_process() or something, and actually\nhave it do the child_process_init() to make sure it is working with a\nsane clean slate).\n\n-Peff\n"},{"id":"278417","messageId":"xmqqk2m4kyfw.fsf@gitster.mtv.corp.google.com","threadId":"41427","inReplyTo":"20160216232657.GB18634@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] pager: factor out a helper to prepare a child process to run the pager","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-16T23:49:55Z","receivedAt":"2016-02-16T23:49:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Feb 16, 2016 at 03:06:56PM -0800, Junio C Hamano wrote:\n>\n>> diff --git a/pager.c b/pager.c\n>> index 5dbcc5a..1406370 100644\n>> --- a/pager.c\n>> +++ b/pager.c\n>> @@ -53,6 +53,23 @@ const char *git_pager(int stdout_is_tty)\n>> \treturn pager;\n>> }\n>> \n>> +void prepare_pager_args(struct child_process *pager_process, ...)\n>> +{\n>> +\tva_list ap;\n>> +\tconst char *arg;\n>> +\n>> +\tva_start(ap, pager_process);\n>> +\twhile ((arg = va_arg(ap, const char *)))\n>> +\t\targv_array_push(&pager_process->args, arg);\n>> +\tva_end(ap);\n>> +\n>> +\tpager_process->use_shell = 1;\n>> +\tif (!getenv(\"LESS\"))\n>> +\t\targv_array_push(&pager_process->env_array, \"LESS=FRX\");\n>> +\tif (!getenv(\"LV\"))\n>> +\t\targv_array_push(&pager_process->env_array, \"LV=-c\");\n>> +}\n>\n> When reading this, I had to wonder what the \"...\" args were supposed to\n> be. I figured it out when I read the caller, but I wonder if a comment\n> would help. Also, we are expecting the pager here as the first argument,\n> so maybe:\n>\n> void prepare_pager_args(struct child_process *pager_process,\n> const char *pager, ...);\n>\n> would be a better signature.\n\n;-)\n\nI had that originally, and then incorrectly fed pager to va_start()\nwhich broke the whole thing.\n\n> That also made me wonder if we could simply\n> get away with:\n>\n> void prepare_pager_args(struct child_process *pager_process,\n> const char *pager);\n>\n> and have callers argv_array_push() themselves afterwards.\n\nAfter all that may be a nicer way to structure.\n\n> And if you put the git_pager() call inside prepare_pager_args (which I\n> agree would be cleaner), we just have:\n>\n> void prepare_pager_args(struct child_process *pager_process);\n>\n> which is pretty self-explanatory (though it might need a new name; I'd\n> be tempted to call it init_pager_process() or something, and actually\n> have it do the child_process_init() to make sure it is working with a\n> sane clean slate).\n\nConceptually I am on the same page, but I am not sure how well that\ninteracts with what \"git am -i\" codepath wants to do, though.\n\nOne big difference between the \"we'll feed our output to pager\"\ncodepath and \"we'll spawn a pager to let a file on the filesystem be\nread\" codepath is that the former needs to call git_pager() and\ncheck the NULL-ness of the return value to decide that it does not\nwant to spawn a pager and let the standard output just go straight\nto the outside world. The latter, on the other hand, does want to\nspawn something to cause the file to be presented to the end user\neven git_pager() returns NULL.\n\nAnd that is why I didn't make this helper call git_pager() itself.\n"},{"id":"278423","messageId":"20160217003215.GC1187@sigill.intra.peff.net","threadId":"41427","inReplyTo":"xmqqk2m4kyfw.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/3] pager: factor out a helper to prepare a child process to run the pager","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-17T00:32:15Z","receivedAt":"2016-02-17T00:32:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 16, 2016 at 03:49:55PM -0800, Junio C Hamano wrote:\n\n> > And if you put the git_pager() call inside prepare_pager_args (which I\n> > agree would be cleaner), we just have:\n> >\n> > void prepare_pager_args(struct child_process *pager_process);\n> >\n> > which is pretty self-explanatory (though it might need a new name; I'd\n> > be tempted to call it init_pager_process() or something, and actually\n> > have it do the child_process_init() to make sure it is working with a\n> > sane clean slate).\n> \n> Conceptually I am on the same page, but I am not sure how well that\n> interacts with what \"git am -i\" codepath wants to do, though.\n> \n> One big difference between the \"we'll feed our output to pager\"\n> codepath and \"we'll spawn a pager to let a file on the filesystem be\n> read\" codepath is that the former needs to call git_pager() and\n> check the NULL-ness of the return value to decide that it does not\n> want to spawn a pager and let the standard output just go straight\n> to the outside world. The latter, on the other hand, does want to\n> spawn something to cause the file to be presented to the end user\n> even git_pager() returns NULL.\n> \n> And that is why I didn't make this helper call git_pager() itself.\n\nThat makes sense. I didn't dig into it carefully. I saw the \"pager=cat\"\nthing in the context of your diff to git-am, and assumed it was weird\nfallback that should be done by the regular pager infrastructure. But\nit's the exact thing you're talking about here.\n\nSo of all of the things I suggested, I think the non-varargs one that\ntakes \"pager\" as a string makes the most sense.\n\n-Peff\n"},{"id":"278492","messageId":"1455736516-13466-1-git-send-email-gitster@pobox.com","threadId":"41427","inReplyTo":"1455664017-27588-1-git-send-email-gitster@pobox.com","subject":"[PATCH v2 0/3] fix \"v\"iew subcommand in \"git am -i\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-17T19:15:13Z","receivedAt":"2016-02-17T19:15:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The 'v'iew subcommand of the interactive mode of \"git am -i\" was\nbroken by the rewrite to C we did at around 2.6.0 timeframe at\n7ff26832 (builtin-am: implement -i/--interactive, 2015-08-04); we\nused to spawn the pager via the shell, accepting things like\n\n\tPAGER='less -S'\n\nin the environment, but the rewrite forgot and tried to directly\nspawn a command whose name is the entire string.\n\nThe bug is understandable, because there are things we need to do\nother than just run_command() to run the pager, such as running it\nwith default LESS/LV settings and running it via the shell, but\nthese pieces of necessary knowledge about what is the right thing to\ndo are hoarded by the setup_pager() entry point, which is only good\nif we are feeding our own standard output to the pager. A codepath\nthat wants to run the pager but not on our output needs to do the\nright thing on its own.\n\nSo the first patch in this series factors out a helper function to\nlet the caller run the pager the right way. They make the third\npatch to fix the breakage in \"am\" trivial. Compared to v1, the\nhelper was much simplified with help by Peff: it always and only\ntakes child-process and the pager command string. The caller can\nappend extra command line arguments after the helper returns if it\nwants to.\n\nJunio C Hamano (3):\n pager: lose a separate argv[]\n pager: factor out a helper to prepare a child process to run the pager\n am -i: fix \"v\"iew\n\n builtin/am.c | 2 +-\n cache.h | 3 +++\n pager.c | 19 +++++++++++--------\n 3 files changed, 15 insertions(+), 9 deletions(-)\n\n-- \n2.7.1-489-g20b2cbe\n"},{"id":"278491","messageId":"1455736516-13466-2-git-send-email-gitster@pobox.com","threadId":"41427","inReplyTo":"1455736516-13466-1-git-send-email-gitster@pobox.com","subject":"[PATCH v2 1/3] pager: lose a separate argv[]","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-17T19:15:14Z","receivedAt":"2016-02-17T19:15:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"These days, using the embedded args array in the child_process\nstructure is the norm. Follow that practice.\n\nSigned-off-by: Junio C Hamano \n---\n * Same as v1\n\n pager.c | 4 +---\n 1 file changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/pager.c b/pager.c\nindex 070dc11..5dbcc5a 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -11,7 +11,6 @@\n * something different on Windows.\n */\n \n-static const char *pager_argv[] = { NULL, NULL };\n static struct child_process pager_process = CHILD_PROCESS_INIT;\n \n static void wait_for_pager(void)\n@@ -70,9 +69,8 @@ void setup_pager(void)\n \tsetenv(\"GIT_PAGER_IN_USE\", \"true\", 1);\n \n \t/* spawn the pager */\n-\tpager_argv[0] = pager;\n+\targv_array_push(&pager_process.args, pager);\n \tpager_process.use_shell = 1;\n-\tpager_process.argv = pager_argv;\n \tpager_process.in = -1;\n \tif (!getenv(\"LESS\"))\n \t\targv_array_push(&pager_process.env_array, \"LESS=FRX\");\n-- \n2.7.1-489-g20b2cbe\n"},{"id":"278493","messageId":"1455736516-13466-3-git-send-email-gitster@pobox.com","threadId":"41427","inReplyTo":"1455736516-13466-1-git-send-email-gitster@pobox.com","subject":"[PATCH v2 2/3] pager: factor out a helper to prepare a child process to run the pager","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-17T19:15:15Z","receivedAt":"2016-02-17T19:15:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When running a pager, we need to run the program git_pager() gave\nus, but we need to make sure we spawn it via the shell (i.e. it is\nvalid to say PAGER='less -S', for example) and give default values\nto $LESS and $LV environment variables. Factor out these details\nto a separate helper function.\n\nSigned-off-by: Junio C Hamano \n---\n * Simplified per Peff's suggestion.\n\n cache.h | 3 +++\n pager.c | 17 +++++++++++------\n 2 files changed, 14 insertions(+), 6 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 6bb7119..a839acc 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -210,7 +210,9 @@ struct cache_entry {\n #error \"CE_EXTENDED_FLAGS out of range\"\n #endif\n \n+/* Forward structure decls */\n struct pathspec;\n+struct child_process;\n \n /*\n * Copy the sha1 and stat state of a cache entry from one to\n@@ -1550,6 +1552,7 @@ extern int pager_use_color;\n extern int term_columns(void);\n extern int decimal_width(uintmax_t);\n extern int check_pager_config(const char *cmd);\n+extern void prepare_pager_args(struct child_process *, const char *pager);\n \n extern const char *editor_program;\n extern const char *askpass_program;\ndiff --git a/pager.c b/pager.c\nindex 5dbcc5a..cb28207 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -53,6 +53,16 @@ const char *git_pager(int stdout_is_tty)\n \treturn pager;\n }\n \n+void prepare_pager_args(struct child_process *pager_process, const char *pager)\n+{\n+\targv_array_push(&pager_process->args, pager);\n+\tpager_process->use_shell = 1;\n+\tif (!getenv(\"LESS\"))\n+\t\targv_array_push(&pager_process->env_array, \"LESS=FRX\");\n+\tif (!getenv(\"LV\"))\n+\t\targv_array_push(&pager_process->env_array, \"LV=-c\");\n+}\n+\n void setup_pager(void)\n {\n \tconst char *pager = git_pager(isatty(1));\n@@ -69,13 +79,8 @@ void setup_pager(void)\n \tsetenv(\"GIT_PAGER_IN_USE\", \"true\", 1);\n \n \t/* spawn the pager */\n-\targv_array_push(&pager_process.args, pager);\n-\tpager_process.use_shell = 1;\n+\tprepare_pager_args(&pager_process, pager);\n \tpager_process.in = -1;\n-\tif (!getenv(\"LESS\"))\n-\t\targv_array_push(&pager_process.env_array, \"LESS=FRX\");\n-\tif (!getenv(\"LV\"))\n-\t\targv_array_push(&pager_process.env_array, \"LV=-c\");\n \targv_array_push(&pager_process.env_array, \"GIT_PAGER_IN_USE\");\n \tif (start_command(&pager_process))\n \t\treturn;\n-- \n2.7.1-489-g20b2cbe\n"},{"id":"278495","messageId":"1455736516-13466-4-git-send-email-gitster@pobox.com","threadId":"41427","inReplyTo":"1455736516-13466-1-git-send-email-gitster@pobox.com","subject":"[PATCH v2 3/3] am -i: fix \"v\"iew","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-17T19:15:16Z","receivedAt":"2016-02-17T19:15:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The 'v'iew subcommand of the interactive mode of \"git am -i\" was\nbroken by the rewrite to C we did at around 2.6.0 timeframe at\n7ff26832 (builtin-am: implement -i/--interactive, 2015-08-04); we\nused to spawn the pager via the shell, accepting things like\n\n\tPAGER='less -S'\n\nin the environment, but the rewrite forgot and tried to directly\nspawn a command whose name is the entire string.\n\nThe previous refactoring of the new helper function makes it easier\nfor us to do the right thing.\n\nSigned-off-by: Junio C Hamano \n---\n * Essentially the same as v1, modulo adjustment for the change in 2/3\n\n builtin/am.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 1399c8d..56cf26e 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1740,7 +1740,7 @@ static int do_interactive(struct am_state *state)\n \n \t\t\tif (!pager)\n \t\t\t\tpager = \"cat\";\n-\t\t\targv_array_push(&cp.args, pager);\n+\t\t\tprepare_pager_args(&cp, pager);\n \t\t\targv_array_push(&cp.args, am_path(state, \"patch\"));\n \t\t\trun_command(&cp);\n \t\t}\n-- \n2.7.1-489-g20b2cbe\n"},{"id":"278494","messageId":"20160217191923.GA16265@sigill.intra.peff.net","threadId":"41427","inReplyTo":"1455736516-13466-1-git-send-email-gitster@pobox.com","subject":"Re: [PATCH v2 0/3] fix \"v\"iew subcommand in \"git am -i\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-17T19:19:24Z","receivedAt":"2016-02-17T19:19:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 17, 2016 at 11:15:13AM -0800, Junio C Hamano wrote:\n\n> So the first patch in this series factors out a helper function to\n> let the caller run the pager the right way. They make the third\n> patch to fix the breakage in \"am\" trivial. Compared to v1, the\n> helper was much simplified with help by Peff: it always and only\n> takes child-process and the pager command string. The caller can\n> append extra command line arguments after the helper returns if it\n> wants to.\n> \n> Junio C Hamano (3):\n> pager: lose a separate argv[]\n> pager: factor out a helper to prepare a child process to run the pager\n> am -i: fix \"v\"iew\n> \n> builtin/am.c | 2 +-\n> cache.h | 3 +++\n> pager.c | 19 +++++++++++--------\n> 3 files changed, 15 insertions(+), 9 deletions(-)\n\nThe whole thing looks good to me.\n\n-Peff\n"}]}