{"thread":{"id":"62520","subject":"[PATCH] git: fix paginate handling for commands with DELAY_PAGER_CONFIG","startedAt":"2024-11-20T10:17:50Z","lastAt":"2024-11-28T22:27:20Z","messageCount":8,"participants":["Matthew Bystrin","Martin Ågren","Junio C Hamano","Kristoffer Haugsbakk"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"507704","messageId":"20241120101741.8225-1-dev.mbstr@gmail.com","threadId":"62520","inReplyTo":null,"subject":"[PATCH] git: fix paginate handling for commands with DELAY_PAGER_CONFIG","fromName":"Matthew Bystrin","fromEmail":"dev.mbstr@gmail.com","sentAt":"2024-11-20T10:17:40Z","receivedAt":"2024-11-20T10:17:50Z","isPatch":true,"sender":{"key":"dev.mbstr@gmail.com","avatar":null},"body":"Calling commands using editor in terminal with `--paginate` option will\nbreak things. For example `git --paginate config --edit`. Add extra\ncheck to ignore paginate flag in case command have DELAY_PAGER_CONFIG\nset.\n\nRelates: cd878a206e8c (t7006: add tests for how git config paginates)\nSigned-off-by: Matthew Bystrin \n---\nHi!\n\nSome time ago I've sent RFC patch [1], which was not quite ready, and I didn't\nreceive any feedback. Now I'm sending a more complete version of it. Fixing\nmentioned behaviour of `note` command can be done in separate patch series.\n\nLink: https://lore.kernel.org/git/20241104140536.4970-1-dev.mbstr@gmail.com/\n\n git.c | 5 +++--\n t/t7006-pager.sh | 4 ++--\n 2 files changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex c2c1b8e22c..2b3b049f4a 100644\n--- a/git.c\n+++ b/git.c\n@@ -464,11 +464,12 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct\n \t}\n \tassert(!prefix || *prefix);\n \tprecompose_argv_prefix(argc, argv, NULL);\n-\tif (use_pager == -1 && run_setup &&\n-\t\t!(p->option & DELAY_PAGER_CONFIG))\n+\tif (use_pager == -1 && run_setup && !(p->option & DELAY_PAGER_CONFIG))\n \t\tuse_pager = check_pager_config(p->cmd);\n \tif (use_pager == -1 && p->option & USE_PAGER)\n \t\tuse_pager = 1;\n+\tif (use_pager == 1 && (p->option & DELAY_PAGER_CONFIG))\n+\t\tuse_pager = 0;\n \tif (run_setup && startup_info->have_repository)\n \t\t/* get_git_dir() may set up repo, avoid that */\n \t\ttrace_repo_setup();\ndiff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\nindex a0296d6ca4..218c5093d3 100755\n--- a/t/t7006-pager.sh\n+++ b/t/t7006-pager.sh\n@@ -188,11 +188,11 @@ test_expect_success TTY 'git tag -a ignores pager.tag' '\n \ttest_path_is_missing paginated.out\n '\n \n-test_expect_success TTY 'git tag -a respects --paginate' '\n+test_expect_success TTY 'git tag -a do not respects --paginate' '\n \ttest_when_finished \"git tag -d newtag\" &&\n \trm -f paginated.out &&\n \ttest_terminal git --paginate tag -am message newtag &&\n-\ttest_path_is_file paginated.out\n+\ttest_path_is_missing paginated.out\n '\n \n test_expect_success TTY 'git tag as alias ignores pager.tag with -a' '\n-- \n2.43.0\n\n"},{"id":"507848","messageId":"CAN0heSoryo-DO6WXKqqASJ5nDLqb3PNNiZv1TP=Pq11wsiSM6A@mail.gmail.com","threadId":"62520","inReplyTo":"20241120101741.8225-1-dev.mbstr@gmail.com","subject":"Re: [PATCH] git: fix paginate handling for commands with DELAY_PAGER_CONFIG","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2024-11-21T21:50:59Z","receivedAt":"2024-11-21T21:51:12Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Hi Matthew,\n\nOn Wed, 20 Nov 2024 at 11:41, Matthew Bystrin wrote:\n>\n> Calling commands using editor in terminal with `--paginate` option will\n> break things. For example `git --paginate config --edit`. Add extra\n> check to ignore paginate flag in case command have DELAY_PAGER_CONFIG\n> set.\n>\n> Relates: cd878a206e8c (t7006: add tests for how git config paginates)\n> Signed-off-by: Matthew Bystrin \n> ---\n\n> Some time ago I've sent RFC patch [1], which was not quite ready, and I didn't\n> receive any feedback. Now I'm sending a more complete version of it. Fixing\n> mentioned behaviour of `note` command can be done in separate patch series.\n\nThanks for reposting, and welcome to the list.\n\n> + if (use_pager == 1 && (p->option & DELAY_PAGER_CONFIG))\n> + use_pager = 0;\n> if (run_setup && startup_info->have_repository)\n\nThis flag goes back to c409824cc2a (\"git.c: let builtins opt for\nhandling `pager.foo` themselves\", 2017-08-02). The original observation\nwas that setting `pager.tag` to true was more or less foolish -- it\nwould help `git tag --list`, sure, but would also break `git tag -a`\nquite badly along the way since the latter wants to open an editor.\n\nThat's why c409824cc2a allowed builtins to basically say \"delay pager\nconfig handling -- I'll respect it for some options/modes, but not for\nothers\". Now this proposed patch wants to extend that handling beyond\n\"delay pager *config*\" to even go \"for some options/modes, I'm going to\ncompletely ignore `--paginate`.\" (\"Delay pager *option*\"?)\n\nActually, no, it's not so much ignoring as *forcing*. Since you force it\nto 0, doesn't that mean that `--paginate` ends up basically being\n`--no-pager`? So `git --paginate branch` is now `git --no-pager branch`?\nThat doesn't seem right. An optionless `git branch` would have\npaginated, so adding `--paginate` shouldn't change anything.\n\nBut even if we force it to -1 instead (for \"maybe\"), I'm not sure I\nunderstand why such an undoing of user intention is wanted. If I run\n`git --paginate tag -a ...`, maybe that's just self-inflicted harm, but\nare we certain that for all the `git foo --bar` which won't respect\n`pager.foo`, that it's also completely crazy to provide `--paginate`?\n\nMartin\n"},{"id":"507946","messageId":"xmqqmshrrxpq.fsf@gitster.g","threadId":"62520","inReplyTo":"CAN0heSoryo-DO6WXKqqASJ5nDLqb3PNNiZv1TP=Pq11wsiSM6A@mail.gmail.com","subject":"Re: [PATCH] git: fix paginate handling for commands with DELAY_PAGER_CONFIG","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-22T17:21:21Z","receivedAt":"2024-11-22T17:21:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren writes:\n\n> Hi Matthew,\n> ...\n> Actually, no, it's not so much ignoring as *forcing*. Since you force it\n> to 0, doesn't that mean that `--paginate` ends up basically being\n> `--no-pager`? So `git --paginate branch` is now `git --no-pager branch`?\n> That doesn't seem right. An optionless `git branch` would have\n> paginated, so adding `--paginate` shouldn't change anything.\n>\n> But even if we force it to -1 instead (for \"maybe\"), I'm not sure I\n> understand why such an undoing of user intention is wanted. If I run\n> `git --paginate tag -a ...`, maybe that's just self-inflicted harm, but\n> are we certain that for all the `git foo --bar` which won't respect\n> `pager.foo`, that it's also completely crazy to provide `--paginate`?\n\nThe whole thing started with\n\n Calling commands using editor in terminal with `--paginate`\n option will break things. For example `git --paginate config\n --edit`.\n\nwhich many of us may respond with \"it hurts? do not do it then\", so\nI agree with you that a fallout would be worse than the problem the\nchange is trying to \"fix\".\n\n\n"},{"id":"507952","messageId":"D5SWT5JKU6S9.2LH8H7ECRZXXM@gmail.com","threadId":"62520","inReplyTo":"xmqqmshrrxpq.fsf@gitster.g","subject":"Re: [PATCH] git: fix paginate handling for commands with DELAY_PAGER_CONFIG","fromName":"Matthew Bystrin","fromEmail":"dev.mbstr@gmail.com","sentAt":"2024-11-22T18:13:01Z","receivedAt":"2024-11-22T18:13:05Z","isPatch":true,"sender":{"key":"dev.mbstr@gmail.com","avatar":null},"body":"Junio and Martin thanks for your replies!\n\n> Martin Ågren writes:\n>\n> > Hi Matthew,\n> > ...\n> > Actually, no, it's not so much ignoring as *forcing*. Since you force it\n> > to 0, doesn't that mean that `--paginate` ends up basically being\n> > `--no-pager`? So `git --paginate branch` is now `git --no-pager branch`?\n> > That doesn't seem right. An optionless `git branch` would have\n> > paginated, so adding `--paginate` shouldn't change anything.\n> >\n> > But even if we force it to -1 instead (for \"maybe\"), I'm not sure I\n> > understand why such an undoing of user intention is wanted. If I run\n> > `git --paginate tag -a ...`, maybe that's just self-inflicted harm, but\n> > are we certain that for all the `git foo --bar` which won't respect\n> > `pager.foo`, that it's also completely crazy to provide `--paginate`?\n>\n> The whole thing started with\n>\n> Calling commands using editor in terminal with `--paginate`\n> option will break things. For example `git --paginate config\n> --edit`.\n>\n> which many of us may respond with \"it hurts? do not do it then\", so\n> I agree with you that a fallout would be worse than the problem the\n> change is trying to \"fix\".\n\nI see the point and totally agree with it.\n\nThe root of the 'problem' is related with editor, not with commands. So maybe it\nis a good way to deal with it in editor code? I've quickly come up with\nsomething like this:\n\ndiff --git a/editor.c b/editor.c\nindex 6b9ce81d5f..04a1f71694 100644\n--- a/editor.c\n+++ b/editor.c\n@@ -13,6 +13,7 @@\n #include \"strvec.h\"\n #include \"run-command.h\"\n #include \"sigchain.h\"\n+#include \"pager.h\"\n \n #ifndef DEFAULT_EDITOR\n #define DEFAULT_EDITOR \"vi\"\n@@ -60,6 +61,9 @@ const char *git_sequence_editor(void)\n static int launch_specified_editor(const char *editor, const char *path,\n \t\t\t\t struct strbuf *buffer, const char *const *env)\n {\n+\tif (pager_in_use())\n+\t\twait_for_pager();\n+\n \tif (!editor)\n \t\treturn error(\"Terminal is dumb, but EDITOR unset\");\n\nBrief testing shows what it works, but more complex approach may be needed. What\ndo you think about that? Should I continue work on that despite the fact it does\nnot really hurts? If yes, is it better to create new patch or send as an update\nof the current?\n"},{"id":"507953","messageId":"1d3aa75d-e816-4544-b10a-ab1a215dcd83@app.fastmail.com","threadId":"62520","inReplyTo":"20241120101741.8225-1-dev.mbstr@gmail.com","subject":"Re: [PATCH] git: fix paginate handling for commands with DELAY_PAGER_CONFIG","fromName":"Kristoffer Haugsbakk","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2024-11-22T18:50:45Z","receivedAt":"2024-11-22T18:51:08Z","isPatch":true,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"I was originally not going to nitpick this in case the patch would go through\nreview (not to disrupt).\n\nOn Wed, Nov 20, 2024, at 11:17, Matthew Bystrin wrote:\n> Calling commands using editor in terminal with `--paginate` option will\n> break things. For example `git --paginate config --edit`. Add extra\n> check to ignore paginate flag in case command have DELAY_PAGER_CONFIG\n> set.\n>\n> Relates: cd878a206e8c (t7006: add tests for how git config paginates)\n\nTrailers in this project usually credit/mention people. Related commits tend to\nbe mentioned in the body (the rest of the body) of the commit message.\n\n> Signed-off-by: Matthew Bystrin \n\n-- \nKristoffer Haugsbakk\n\n"},{"id":"508003","messageId":"xmqqa5do834m.fsf@gitster.g","threadId":"62520","inReplyTo":"D5SWT5JKU6S9.2LH8H7ECRZXXM@gmail.com","subject":"Re: [PATCH] git: fix paginate handling for commands with DELAY_PAGER_CONFIG","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-25T02:21:29Z","receivedAt":"2024-11-25T02:21:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Matthew Bystrin\" writes:\n\n> The root of the 'problem' is related with editor, not with commands. So maybe it\n> is a good way to deal with it in editor code? I've quickly come up with\n> something like this:\n>\n> diff --git a/editor.c b/editor.c\n> index 6b9ce81d5f..04a1f71694 100644\n> --- a/editor.c\n> +++ b/editor.c\n> @@ -13,6 +13,7 @@\n> #include \"strvec.h\"\n> #include \"run-command.h\"\n> #include \"sigchain.h\"\n> +#include \"pager.h\"\n> \n> #ifndef DEFAULT_EDITOR\n> #define DEFAULT_EDITOR \"vi\"\n> @@ -60,6 +61,9 @@ const char *git_sequence_editor(void)\n> static int launch_specified_editor(const char *editor, const char *path,\n> \t\t\t\t struct strbuf *buffer, const char *const *env)\n> {\n> +\tif (pager_in_use())\n> +\t\twait_for_pager();\n> +\n> \tif (!editor)\n> \t\treturn error(\"Terminal is dumb, but EDITOR unset\");\n>\n> Brief testing shows what it works, but more complex approach may be needed.\n\nI am not sure what you mean by \"it works\", and I do not really\nunderstand what you meant by \"break things\" in the original claim in\nthe proposed commit log message, either.\n\n Calling commands using editor in terminal with `--paginate` option will\n break things. For example `git --paginate config --edit`.\n\nThe last sentence is incomplete and it is unclear which part of what\n`git -p config -e' does (or does not do) is considered \"breaks\nthings\".\n\nI cannot tell if the simplified change above (which is far nicer\nthan sprinkling code to tweak use_pager variable everywhere) is\ndoing the right thing without knowing what definition of \"working\"\n(and hence \"breaking\") you are using when you report that the above\nchange \"works\".\n\nStepping back a bit, whatever code change your next version\ncontains, please make sure that the proposed log message that\nexplains your change tells the readers what you see, and what you\nwould want to see in the ideal world (with a fix), and what the\ndifferences between these two are, instead of just saying \"currently\nit breaks, so let's fix it\".\n\nThanks.\n"},{"id":"508092","messageId":"CAN0heSqb+dzGAVnN6=_AkKa-qh4GYo7ku90No=uXBg2+wobL1Q@mail.gmail.com","threadId":"62520","inReplyTo":"D5SWT5JKU6S9.2LH8H7ECRZXXM@gmail.com","subject":"Re: [PATCH] git: fix paginate handling for commands with DELAY_PAGER_CONFIG","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2024-11-25T10:55:42Z","receivedAt":"2024-11-25T10:55:55Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Fri, 22 Nov 2024 at 19:13, Matthew Bystrin wrote:\n>\n> > The whole thing started with\n> >\n> > Calling commands using editor in terminal with `--paginate`\n> > option will break things. For example `git --paginate config\n> > --edit`.\n> >\n> > which many of us may respond with \"it hurts? do not do it then\", so\n> > I agree with you that a fallout would be worse than the problem the\n> > change is trying to \"fix\".\n>\n> I see the point and totally agree with it.\n>\n> The root of the 'problem' is related with editor, not with commands. So maybe it\n> is a good way to deal with it in editor code? I've quickly come up with\n> something like this:\n>\n> diff --git a/editor.c b/editor.c\n> index 6b9ce81d5f..04a1f71694 100644\n> --- a/editor.c\n> +++ b/editor.c\n> @@ -13,6 +13,7 @@\n> #include \"strvec.h\"\n> #include \"run-command.h\"\n> #include \"sigchain.h\"\n> +#include \"pager.h\"\n>\n> #ifndef DEFAULT_EDITOR\n> #define DEFAULT_EDITOR \"vi\"\n> @@ -60,6 +61,9 @@ const char *git_sequence_editor(void)\n> static int launch_specified_editor(const char *editor, const char *path,\n> struct strbuf *buffer, const char *const *env)\n> {\n> + if (pager_in_use())\n> + wait_for_pager();\n> +\n> if (!editor)\n> return error(\"Terminal is dumb, but EDITOR unset\");\n>\n> Brief testing shows what it works, but more complex approach may be needed. What\n> do you think about that? Should I continue work on that despite the fact it does\n> not really hurts? If yes, is it better to create new patch or send as an update\n> of the current?\n\nSo the high-level idea here is \"if we're going to open an editor, let's\nfirst shut down any pager\".\n\nThis helper is relatively recent, from e8bd8883fe (pager: introduce\nwait_for_pager, 2024-07-25) and fc87b2f7c1 (add-patch: render hunks\nthrough the pager, 2024-07-25). The context there seems to be that a\nlong-running git process wants to open the pager, maybe even more than\nonce. So we need a way to shut down the pager we just opened -- it's\ndone its thing.\n\nWhat gives me a bit of pause here is that we're now approaching this\nfrom a quite different direction: a git process wants to shut down the\npager not because it just opened it and the pager has done its thing,\nbut because we want to open an editor and think that any pager might\ninterfere.\n\nIf `git add -p` is a long-running process that wants to repeatedly\nlaunch a pager, what could the opposite look like? A long-running\nprocess that wants to repeatedly launch an editor? While paging?\n\nMaybe `git rebase -i` could be one such example. TBH, running its output\nthrough a pager might not look super-pretty today due to it producing\ntemporary output that it then erases, but at least it works. The editor\nmight be graphical (or a script?) that won't actually suffer from the\npager running. Killing the pager might be unwanted. So I don't know.\n\nWhat is the original problem here? Is some kind of tooling issuing a\n`git -p tag -a` where it's not possible to teach it to drop the `-p`?\n\nMartin\n"},{"id":"508310","messageId":"D5Y5Z3NCOYPB.1VT7HMAHZTOVE@gmail.com","threadId":"62520","inReplyTo":"CAN0heSqb+dzGAVnN6=_AkKa-qh4GYo7ku90No=uXBg2+wobL1Q@mail.gmail.com","subject":"Re: [PATCH] git: fix paginate handling for commands with DELAY_PAGER_CONFIG","fromName":"Matthew Bystrin","fromEmail":"dev.mbstr@gmail.com","sentAt":"2024-11-28T22:27:16Z","receivedAt":"2024-11-28T22:27:20Z","isPatch":true,"sender":{"key":"dev.mbstr@gmail.com","avatar":null},"body":"Martin Ågren wrote:\n> If `git add -p` is a long-running process that wants to repeatedly\n> launch a pager, what could the opposite look like? A long-running\n> process that wants to repeatedly launch an editor? While paging?\n>\n> Maybe `git rebase -i` could be one such example. TBH, running its output\n> through a pager might not look super-pretty today due to it producing\n> temporary output that it then erases, but at least it works. The editor\n> might be graphical (or a script?) that won't actually suffer from the\n> pager running. Killing the pager might be unwanted. So I don't know.\n\nAgree, killing might be unwanted. Another approach proposed below.\n\n> What is the original problem here? Is some kind of tooling issuing a\n> `git -p tag -a` where it's not possible to teach it to drop the `-p`?\n\nI faced such 'wrong' behaviour by typing commands manually.\n\nIt feels like one flag should not break execution of interactive commands like\n`git -p add -p`, `git -p rebase -i`, `git -p config -e` etc. I took into account\nJunio's comments, so please let me describe situation in more correct way. The\nfollowing is related with workflow in terminal.\n\nCurrently when some interactive command is called with `--paginate` I see\ninterfered output and cannot proceed to desirable actions. A good analogy is to\ncall `vim | less`.\n\nWhat I want to see is if we are calling interactive command with\n`--paginate` (1) it would rather fail explicitly with an error message, or (2)\nit would close+reopen/bypass pager when some interaction is needed. Bypassing\npager is a complicated thing, I'm not sure it can be done (because of how they\nclaim stdin)\n\nOf course we can just simply do not use `-p` flag with interactive commands and\nprovide some info in documentation.\n\nThanks!\n"}]}