{"thread":{"id":"28097","subject":"[PATCH] Utilize config variable pager.stash in stash list command","startedAt":"2011-08-14T14:31:49Z","lastAt":"2011-08-16T22:56:40Z","messageCount":5,"participants":["Ingo Brückl","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"173499","messageId":"4e47dcf9.55313988.bm000@wupperonline.de","threadId":"28097","inReplyTo":null,"subject":"[PATCH] Utilize config variable pager.stash in stash list command","fromName":"Ingo Brückl","fromEmail":"ib@wupperonline.de","sentAt":"2011-08-14T14:31:49Z","receivedAt":"2011-08-14T14:31:49Z","isPatch":true,"sender":{"key":"ib@wupperonline.de","avatar":"https://avatars.githubusercontent.com/u/123327?v=4"},"body":"Signed-off-by: Ingo Brückl <ib@wupperonline.de>\n---\n By now stash list ignores it.\n\n git-stash.sh |    3 ++-\n 1 files changed, 2 insertions(+), 1 deletions(-)\n\ndiff --git a/git-stash.sh b/git-stash.sh\nindex f4e6f05..7bb0856 100755\n--- a/git-stash.sh\n+++ b/git-stash.sh\n@@ -264,7 +264,8 @@ have_stash () {\n\n list_stash () {\n \thave_stash || return 0\n-\tgit log --format=\"%gd: %gs\" -g \"$@\" $ref_stash --\n+\ttest \"$(git config --get pager.stash)\" = \"false\" && no_pager=--no-pager\n+\tgit $no_pager log --format=\"%gd: %gs\" -g \"$@\" $ref_stash --\n }\n\n show_stash () {\n--\n1.7.6\n"},{"id":"173563","messageId":"20110815234714.GB4699@sigill.intra.peff.net","threadId":"28097","inReplyTo":"4e47dcf9.55313988.bm000@wupperonline.de","subject":"Re: [PATCH] Utilize config variable pager.stash in stash list command","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-08-15T23:47:14Z","receivedAt":"2011-08-15T23:47:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Aug 14, 2011 at 04:31:49PM +0200, Ingo Brückl wrote:\n\n> Signed-off-by: Ingo Brückl <ib@wupperonline.de>\n> ---\n>  By now stash list ignores it.\n> \n>  git-stash.sh |    3 ++-\n>  1 files changed, 2 insertions(+), 1 deletions(-)\n> \n> diff --git a/git-stash.sh b/git-stash.sh\n> index f4e6f05..7bb0856 100755\n> --- a/git-stash.sh\n> +++ b/git-stash.sh\n> @@ -264,7 +264,8 @@ have_stash () {\n> \n>  list_stash () {\n>  \thave_stash || return 0\n> -\tgit log --format=\"%gd: %gs\" -g \"$@\" $ref_stash --\n> +\ttest \"$(git config --get pager.stash)\" = \"false\" && no_pager=--no-pager\n> +\tgit $no_pager log --format=\"%gd: %gs\" -g \"$@\" $ref_stash --\n>  }\n\nIt's not quite as simple as this these days. The pager.* variables can\nalso point to a program to run as a pager for this specific command.\n\nThis stuff is supposed to be handled by the \"git\" wrapper itself, which\nwill either run the pager (if the config is boolean true, or a specific\ncommand), or will set an environment variable to avoid running one for\nany subcommand (if it's boolean false).\n\nHowever, we don't respect pager.* config for external commands there at\nall. I think this was due to some initialization-order bugs that made it\nhard for us to look at config before exec'ing external commands. But\nperhaps they are gone, as the patch below[1] seems to work OK for me.\n\n---\n git.c |    2 ++\n 1 files changed, 2 insertions(+), 0 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex 8828c18..47a6d3d 100644\n--- a/git.c\n+++ b/git.c\n@@ -459,6 +459,8 @@ static void execv_dashed_external(const char **argv)\n \tconst char *tmp;\n \tint status;\n \n+\tif (use_pager == -1)\n+\t\tuse_pager = check_pager_config(argv[0]);\n \tcommit_pager_choice();\n \n \tstrbuf_addf(&cmd, \"git-%s\", argv[0]);\n\n-Peff\n\n[1] I posted this in a similar discussion several months ago:\n\n    http://thread.gmane.org/gmane.comp.version-control.git/161756/focus=161771\n\nI think what it really needs is more testing to see if looking at the\nconfig then has any unintended side effects.\n"},{"id":"173593","messageId":"4e4a4743.4e230d8a.bm000@wupperonline.de","threadId":"28097","inReplyTo":"20110815234714.GB4699@sigill.intra.peff.net","subject":"Re: [PATCH] Utilize config variable pager.stash in stash list command","fromName":"Ingo Brückl","fromEmail":"ib@wupperonline.de","sentAt":"2011-08-16T10:10:45Z","receivedAt":"2011-08-16T10:10:45Z","isPatch":true,"sender":{"key":"ib@wupperonline.de","avatar":"https://avatars.githubusercontent.com/u/123327?v=4"},"body":"Jeff King wrote on Mon, 15 Aug 2011 16:47:14 -0700:\n\n> On Sun, Aug 14, 2011 at 04:31:49PM +0200, Ingo Brückl wrote:\n\n>> Signed-off-by: Ingo Brückl <ib@wupperonline.de>\n>> ---\n>>  By now stash list ignores it.\n>>\n>>  git-stash.sh |    3 ++-\n>>  1 files changed, 2 insertions(+), 1 deletions(-)\n>>\n>> diff --git a/git-stash.sh b/git-stash.sh\n>> index f4e6f05..7bb0856 100755\n>> --- a/git-stash.sh\n>> +++ b/git-stash.sh\n>> @@ -264,7 +264,8 @@ have_stash () {\n>>\n>>  list_stash () {\n>>       have_stash || return 0\n>> -     git log --format=\"%gd: %gs\" -g \"$@\" $ref_stash --\n>> +     test \"$(git config --get pager.stash)\" = \"false\" && no_pager=--no-pager\n>> +     git $no_pager log --format=\"%gd: %gs\" -g \"$@\" $ref_stash --\n>>  }\n\n> It's not quite as simple as this these days. The pager.* variables can\n> also point to a program to run as a pager for this specific command.\n\n> This stuff is supposed to be handled by the \"git\" wrapper itself, which\n> will either run the pager (if the config is boolean true, or a specific\n> command), or will set an environment variable to avoid running one for\n> any subcommand (if it's boolean false).\n\n> However, we don't respect pager.* config for external commands there at\n> all. I think this was due to some initialization-order bugs that made it\n> hard for us to look at config before exec'ing external commands. But\n> perhaps they are gone, as the patch below[1] seems to work OK for me.\n\n>  git.c |    2 ++\n>  1 files changed, 2 insertions(+), 0 deletions(-)\n\n> diff --git a/git.c b/git.c\n> index 8828c18..47a6d3d 100644\n> +++ b/git.c\n> @@ -459,6 +459,8 @@ static void execv_dashed_external(const char **argv)\n>         const char *tmp;\n>         int status;\n>\n> +       if (use_pager == -1)\n> +               use_pager = check_pager_config(argv[0]);\n>         commit_pager_choice();\n>\n>         strbuf_addf(&cmd, \"git-%s\", argv[0]);\n\n> -Peff\n\n> [1] I posted this in a similar discussion several months ago:\n\n\n> http://thread.gmane.org/gmane.comp.version-control.git/161756/focus=161771\n\nActually, I only wanted to change the stash list behavior (but better should\nhave used $(git config --get pager.stash.list) for that). Unfortunately, it\nis impossible then to force the pager with --paginate again.\n\n> I think what it really needs is more testing to see if looking at the\n> config then has any unintended side effects.\n\nYours surely is a far better approach, although it only can handle the main\ncommand (stash), not the sub-command (list), but this is totally in\naccordance with everything else in git.\n\nWith \"pager.stash false\" (which would then require --paginate for a lot of\nstash commands), I found that a paginated output of 'git -p stash show -p'\nloses the diff colors, but that seems unrelated to your patch. It still is\nstrange though.\n\nIngo\n"},{"id":"173599","messageId":"4e4a58c2.33fdbc51.bm000@wupperonline.de","threadId":"28097","inReplyTo":"4e4a4743.4e230d8a.bm000@wupperonline.de","subject":"Re: [PATCH] Utilize config variable pager.stash in stash list command","fromName":"Ingo Brückl","fromEmail":"ib@wupperonline.de","sentAt":"2011-08-16T11:24:58Z","receivedAt":"2011-08-16T11:24:58Z","isPatch":true,"sender":{"key":"ib@wupperonline.de","avatar":"https://avatars.githubusercontent.com/u/123327?v=4"},"body":"I wrote on Tue, 16 Aug 2011 12:10:45 +0200:\n\n> Actually, I only wanted to change the stash list behavior (but better\n> should have used $(git config --get pager.stash.list) for that).\n> Unfortunately, it is impossible then to force the pager with --paginate\n> again.\n\nActually, it *is* possible to force the pager with --paginate again, so this\nis exactly what I was trying to achieve (only using pager.stash.list now).\n\nIngo\n"},{"id":"173644","messageId":"20110816225639.GA20050@sigill.intra.peff.net","threadId":"28097","inReplyTo":"4e4a4743.4e230d8a.bm000@wupperonline.de","subject":"Re: [PATCH] Utilize config variable pager.stash in stash list command","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-08-16T22:56:40Z","receivedAt":"2011-08-16T22:56:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 16, 2011 at 12:10:45PM +0200, Ingo Brückl wrote:\n\n> > http://thread.gmane.org/gmane.comp.version-control.git/161756/focus=161771\n> \n> Actually, I only wanted to change the stash list behavior (but better should\n> have used $(git config --get pager.stash.list) for that). Unfortunately, it\n> is impossible then to force the pager with --paginate again.\n> \n> > I think what it really needs is more testing to see if looking at the\n> > config then has any unintended side effects.\n> \n> Yours surely is a far better approach, although it only can handle the main\n> command (stash), not the sub-command (list), but this is totally in\n> accordance with everything else in git.\n\nYeah, that is a general problem with git's pager handling. We only have\none context: a single git command. But some commands may have multiple\nsubcommands, and a pager only makes sense for some of them.\n\nYou've run into it for \"stash show\", but it is no different than\nsomething like \"git branch\". You might want the list of branches to go\nthrough a pager, but almost certainly not branch creation or deletion\noperations.\n\nI think something like pager.stash.list is the right way forward. But\nyour patch by itself isn't enough. It only handles the negative case.\nSetting \"pager.stash.list\" to \"true\" would do nothing.\n\n> With \"pager.stash false\" (which would then require --paginate for a lot of\n> stash commands), I found that a paginated output of 'git -p stash show -p'\n> loses the diff colors, but that seems unrelated to your patch. It still is\n> strange though.\n\nWe auto-detect whether to use colors based on whether we are outputting\nto a terminal or not. If we start the pager ourselves, we will also\noutput colors (unless color.pager is false). I suspect the \"pager in\nuse\" flag is not making it to the external command.\n\n-Peff\n"}]}