{"thread":{"id":"62369","subject":"[PATCH] diff: setup pager only before diff contents truly ready","startedAt":"2024-10-19T20:39:54Z","lastAt":"2024-12-04T00:18:36Z","messageCount":12,"participants":["Y5 via GitGitGadget","Kristoffer Haugsbakk","Jeff King","Philip Yung","Taylor Blau","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"505511","messageId":"pull.1817.git.git.1729370390416.gitgitgadget@gmail.com","threadId":"62369","inReplyTo":null,"subject":"[PATCH] diff: setup pager only before diff contents truly ready","fromName":"Y5 via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-10-19T20:39:50Z","receivedAt":"2024-10-19T20:39:54Z","isPatch":true,"sender":{"key":"name:Y5","avatar":null},"body":"From: y5c4l3 <y5c4l3@proton.me>\n\ngit-diff setups pager at an early stage in cmd_diff; running diff with\ninvalid options like git diff --invalid will unexpectedly starts a\npager, which causes behavior inconsistency.\n\nThe pager setup routine should be moved right before the real diff\ncontents, in case there is any argv error.\n\nSigned-off-by: y5c4l3 <y5c4l3@proton.me>\n---\n    diff: setup pager only before diff contents truly ready\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1817%2Fy5c4l3%2Fdiff-invalid-argv-remove-pager-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1817/y5c4l3/diff-invalid-argv-remove-pager-v1\nPull-Request: https://github.com/git/git/pull/1817\n\n builtin/diff.c | 8 ++++++--\n 1 file changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex dca52d4221e..03340173700 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -105,6 +105,7 @@ static void builtin_diff_b_f(struct rev_info *revs,\n \t\t     1, 0,\n \t\t     blob[0]->path ? blob[0]->path : path,\n \t\t     path);\n+\tsetup_diff_pager(&revs->diffopt);\n \tdiffcore_std(&revs->diffopt);\n \tdiff_flush(&revs->diffopt);\n }\n@@ -129,6 +130,7 @@ static void builtin_diff_blobs(struct rev_info *revs,\n \t\t     &blob[0]->item->oid, &blob[1]->item->oid,\n \t\t     1, 1,\n \t\t     blob_path(blob[0]), blob_path(blob[1]));\n+\tsetup_diff_pager(&revs->diffopt);\n \tdiffcore_std(&revs->diffopt);\n \tdiff_flush(&revs->diffopt);\n }\n@@ -164,6 +166,7 @@ static void builtin_diff_index(struct rev_info *revs,\n \t} else if (repo_read_index(the_repository) < 0) {\n \t\tdie_errno(\"repo_read_cache\");\n \t}\n+\tsetup_diff_pager(&revs->diffopt);\n \trun_diff_index(revs, option);\n }\n \n@@ -201,6 +204,7 @@ static void builtin_diff_tree(struct rev_info *revs,\n \t\toid[swap] = &ent0->item->oid;\n \t\toid[1 - swap] = &ent1->item->oid;\n \t}\n+\tsetup_diff_pager(&revs->diffopt);\n \tdiff_tree_oid(oid[0], oid[1], \"\", &revs->diffopt);\n \tlog_tree_diff_flush(revs);\n }\n@@ -227,6 +231,7 @@ static void builtin_diff_combined(struct rev_info *revs,\n \t\tif (i != first_non_parent)\n \t\t\toid_array_append(&parents, &ent[i].item->oid);\n \t}\n+\tsetup_diff_pager(&revs->diffopt);\n \tdiff_tree_combined(&ent[first_non_parent].item->oid, &parents, revs);\n \toid_array_clear(&parents);\n }\n@@ -283,6 +288,7 @@ static void builtin_diff_files(struct rev_info *revs, int argc, const char **arg\n \t\t\t\t    0) < 0) {\n \t\tdie_errno(\"repo_read_index_preload\");\n \t}\n+\tsetup_diff_pager(&revs->diffopt);\n \trun_diff_files(revs, options);\n }\n \n@@ -523,8 +529,6 @@ int cmd_diff(int argc,\n \trev.diffopt.flags.recursive = 1;\n \trev.diffopt.rotate_to_strict = 1;\n \n-\tsetup_diff_pager(&rev.diffopt);\n-\n \t/*\n \t * Do we have --cached and not have a pending object, then\n \t * default to HEAD by hand.  Eek.\n\nbase-commit: 15030f9556f545b167b1879b877a5d780252dc16\n-- \ngitgitgadget\n"},{"id":"505512","messageId":"348f4415-77cb-4893-8388-a2495a391a39@app.fastmail.com","threadId":"62369","inReplyTo":"pull.1817.git.git.1729370390416.gitgitgadget@gmail.com","subject":"Re: [PATCH] diff: setup pager only before diff contents truly ready","fromName":"Kristoffer Haugsbakk","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2024-10-19T20:57:09Z","receivedAt":"2024-10-19T20:57:45Z","isPatch":true,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"On Sat, Oct 19, 2024, at 22:39, Y5 via GitGitGadget wrote:\n> From: y5c4l3 <y5c4l3@proton.me>\n>\n> git-diff setups pager at an early stage in cmd_diff; running diff with\n> invalid options like git diff --invalid will unexpectedly starts a\n\ns/starts a/start a/\n\n> pager, which causes behavior inconsistency.\n>\n> The pager setup routine should be moved right before the real diff\n> contents, in case there is any argv error.\n\n*Any* argv error?  Maybe “an argv error”?\n\n“any argv error” looks like there isn’t an agreement on plural/singular.\n\n-- \nKristoffer Haugsbakk\n\n"},{"id":"505514","messageId":"20241019211938.GA589728@coredump.intra.peff.net","threadId":"62369","inReplyTo":"pull.1817.git.git.1729370390416.gitgitgadget@gmail.com","subject":"Re: [PATCH] diff: setup pager only before diff contents truly ready","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-10-19T21:19:38Z","receivedAt":"2024-10-19T21:26:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Oct 19, 2024 at 08:39:50PM +0000, Y5 via GitGitGadget wrote:\n\n> git-diff setups pager at an early stage in cmd_diff; running diff with\n> invalid options like git diff --invalid will unexpectedly starts a\n> pager, which causes behavior inconsistency.\n\nI do think the outcome is a little nicer for the user, but I'd hesitate\nto call it more inconsistent. Most of the rest of Git is setting up the\npager in git.c, before we even call into the builtin. So any early\nerrors will likewise go to the pager. E.g., try \"git log --foo\".\n\nSo I dunno. I'm not strictly opposed to making things nicer when we can\ndo so easily.  But the endgame of this is probably getting rid of\nUSE_PAGER entirely and asking each builtin to decide when to commit to\nusing the pager (presumably after option parsing).\n\nAnd even then, it wouldn't apply to commands implemented as an external\nprocess. And of course we can still die(), etc, after starting the\npager. So it would never be totally consistent.\n\n> Signed-off-by: y5c4l3 <y5c4l3@proton.me>\n\nWe usually ask for something approaching a legal name, as this sign-off\nis supposed to be certifying the DCO (see the \"dco\" section in\nDocumentation/SubmittingPatches).\n\n>  builtin/diff.c | 8 ++++++--\n>  1 file changed, 6 insertions(+), 2 deletions(-)\n\nThe patch itself looks plausibly correct. The biggest regression risk\nwould be missing a spot that needed a new setup_diff_pager() call, and I\nsuspect we don't have good test coverage here. But going top-down from\nbuiltin_diff(), I don't see any paths you missed.\n\n-Peff\n"},{"id":"505582","messageId":"UZMh2lyzbLOgsf0PXfMnq6HnWVnCK3y36jY3IMKUykPi74ztNucf8bgywoeO0DdeApq31JDDGMZiEya99zAcI3l8y_zcVqiN8FpEnT1DRZU=@proton.me","threadId":"62369","inReplyTo":"20241019211938.GA589728@coredump.intra.peff.net","subject":"Re: [PATCH] diff: setup pager only before diff contents truly ready","fromName":"Philip Yung","fromEmail":"y5c4l3@proton.me","sentAt":"2024-10-21T00:11:33Z","receivedAt":"2024-10-21T00:11:51Z","isPatch":true,"sender":{"key":"y5c4l3@proton.me","avatar":null},"body":"> errors will likewise go to the pager. E.g., try \"git log --foo\".\n\nHope that I didn't take it the wrong way, but I don't think `git log --foo`\nstarts a pager, where the routine `setup_pager()` is put after argv parsing.\n(checked by `strace`)\n\n> So I dunno. I'm not strictly opposed to making things nicer when we can\n> do so easily. But the endgame of this is probably getting rid of\n> USE_PAGER entirely and asking each builtin to decide when to commit to\n> using the pager (presumably after option parsing).\n> \n> And even then, it wouldn't apply to commands implemented as an external\n> process. And of course we can still die(), etc, after starting the\n> pager. So it would never be totally consistent.\n\nDespite the example, it is overall convincing since currently there is no design\nto ensure the pager consistency. However, it's something we can do right now to\nmake, at least our own builtins, more consistent.\n\n> We usually ask for something approaching a legal name, as this sign-off\n> is supposed to be certifying the DCO (see the \"dco\" section in\n> Documentation/SubmittingPatches).\n\nSorry if my first GitGitGadget experience bothers the mailing list, thanks for\nthe reminder. :)\n\n> would be missing a spot that needed a new setup_diff_pager() call, and I\n> suspect we don't have good test coverage here.\n\nThis is actually my concern as well when I was naively testing the coverage\nusing GDB, which turned out to be quite tedious. Would you consider it's fine to\nadd a pager consistency test for builtins, probably in another patch with regard\nto `t7006-pager.sh` OR a new test `t7007`?\n\nI'll reword and re-signoff this patch as soon as it looks really fine to you.\n\nPhilip Yung\nBest Regards\n"},{"id":"505583","messageId":"R7cWZCIlTr03dRb1ETcVUc62FHFAZT5_U8lWX4Q3QtJsqmtwFr7hmOtPq4yNfdzHgBXnv52HXFqiG6VqFQ5MFk60TpnvnR1DPdIrT2YTMgo=@proton.me","threadId":"62369","inReplyTo":"348f4415-77cb-4893-8388-a2495a391a39@app.fastmail.com","subject":"Re: [PATCH] diff: setup pager only before diff contents truly ready","fromName":"Philip Yung","fromEmail":"y5c4l3@proton.me","sentAt":"2024-10-21T00:17:28Z","receivedAt":"2024-10-21T00:17:40Z","isPatch":true,"sender":{"key":"y5c4l3@proton.me","avatar":null},"body":"> s/starts a/start a/\n>\n> Any argv error? Maybe “an argv error”? \n> “any argv error” looks like there isn’t an agreement on plural/singular.\n> \n> --\n> Kristoffer Haugsbakk\n\nAppreciate your revision, thanks!\n\nPhilip Yung\nBest Regards\n"},{"id":"505722","messageId":"20241021190045.GB1219228@coredump.intra.peff.net","threadId":"62369","inReplyTo":"UZMh2lyzbLOgsf0PXfMnq6HnWVnCK3y36jY3IMKUykPi74ztNucf8bgywoeO0DdeApq31JDDGMZiEya99zAcI3l8y_zcVqiN8FpEnT1DRZU=@proton.me","subject":"Re: [PATCH] diff: setup pager only before diff contents truly ready","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-10-21T19:00:45Z","receivedAt":"2024-10-21T19:00:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 21, 2024 at 12:11:33AM +0000, Philip Yung wrote:\n\n> > errors will likewise go to the pager. E.g., try \"git log --foo\".\n> \n> Hope that I didn't take it the wrong way, but I don't think `git log --foo`\n> starts a pager, where the routine `setup_pager()` is put after argv parsing.\n> (checked by `strace`)\n\nHmm, this actually depends on config. If you have pager.log defined,\nwe'll start it early in git.c, but otherwise not until the setup_pager()\ncall.\n\nI was mildly surprised that pager.diff would not have the same effect,\neven with your patch. But that's because we only handle pager config if\nRUN_SETUP is true, which it is not for diff (because we might be doing\nan out-of-repo --index diff). And the reason for that is mostly\nhistorical, as reading config early interferes with repo setup (though\nI'm even sure that's still the case, as check_pager_config() these days\nuses the \"early\" config mechanism which is supposed to address that).\n\nWhat a horrid mess of inconsistency and hacks. ;)\n\nLikewise, any builtin that sets USE_PAGER in git.c will turn on the\npager early. So \"git shortlog --foo\" will go through the pager, as will\nrange-diff. I was somewhat surprised those are the only two these days.\nLooks like 1fda91b511 (Fix 'git log' early pager startup error case,\n2010-08-24) dropped many. And I think your patch is the spiritual\nsuccessor to that.\n\nSo I think in an ideal world we'd:\n\n  - convert those two commands to do the pager setup themselves and\n    retire the USE_PAGER flag entirely\n\n  - move configured pager handling down into more commands. So git-log\n    should set DELAY_PAGER_CONFIG and then call setup_auto_pager()\n    rather than setup_pager(). Ideally DELAY_PAGER_CONFIG would be the\n    default, but we can't do that until every builtin makes its own call\n    to setup_auto_pager() at the right moment.\n\n  - push calls to setup_pager() (or setup_auto_pager()) as far down\n    within commands as possible (right before we start generating\n    output). Your patch does that for git-diff, but there may be other\n    cases.\n\n  - consistently handle pager config whether USE_SETUP is set or not.\n    That means git-diff should set DELAY_PAGER_CONFIG, since it handles\n    the pager itself.\n\nAnd that would make things more consistent overall, and avoid pushing\nearly errors into the pager (though of course it would still be possible\nto get some errors in the pager if they happen after we start it).\n\nI don't blame you if you don't want to start down that rabbit hole. :) I\nthink it would probably be OK to peck away at it incrementally, and your\npatch does that.\n\n> > would be missing a spot that needed a new setup_diff_pager() call, and I\n> > suspect we don't have good test coverage here.\n> \n> This is actually my concern as well when I was naively testing the coverage\n> using GDB, which turned out to be quite tedious. Would you consider it's fine to\n> add a pager consistency test for builtins, probably in another patch with regard\n> to `t7006-pager.sh` OR a new test `t7007`?\n\nTBH, I am not all that worried about adding tests just for your patch.\nYou'd need to identify all of the possible diff code paths in order to\nadd tests for them, which is the same thing you had to do to fix the\ncode paths. I was mostly just commenting that we're not likely to be\nable to rely on existing tests to help us here.\n\nIt might be worth adding a test that shows off your improved diff\nbehavior, though I would be OK if it was a representative command and\nnot exhaustive. I think adding to t7006 should be fine.\n\nIf we fixed some of the bits I mentioned above, some of that should\nlikewise be covered by tests.\n\n-Peff\n"},{"id":"505730","messageId":"ZxatqdBiB+NoMP+j@nand.local","threadId":"62369","inReplyTo":"20241021190045.GB1219228@coredump.intra.peff.net","subject":"Re: [PATCH] diff: setup pager only before diff contents truly ready","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-21T19:38:17Z","receivedAt":"2024-10-21T19:38:19Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 21, 2024 at 03:00:45PM -0400, Jeff King wrote:\n> So I think in an ideal world we'd:\n>\n>   - convert those two commands to do the pager setup themselves and\n>     retire the USE_PAGER flag entirely\n>\n>   - move configured pager handling down into more commands. So git-log\n>     should set DELAY_PAGER_CONFIG and then call setup_auto_pager()\n>     rather than setup_pager(). Ideally DELAY_PAGER_CONFIG would be the\n>     default, but we can't do that until every builtin makes its own call\n>     to setup_auto_pager() at the right moment.\n>\n>   - push calls to setup_pager() (or setup_auto_pager()) as far down\n>     within commands as possible (right before we start generating\n>     output). Your patch does that for git-diff, but there may be other\n>     cases.\n>\n>   - consistently handle pager config whether USE_SETUP is set or not.\n>     That means git-diff should set DELAY_PAGER_CONFIG, since it handles\n>     the pager itself.\n>\n> And that would make things more consistent overall, and avoid pushing\n> early errors into the pager (though of course it would still be possible\n> to get some errors in the pager if they happen after we start it).\n>\n> I don't blame you if you don't want to start down that rabbit hole. :) I\n> think it would probably be OK to peck away at it incrementally, and your\n> patch does that.\n\nNicely put. I think what's nice about the patch here is that it starts\nus down that direction you outlined above, so we'd want it regardless of\nhow much of the rest of the work Y5 is willing to do.\n\n> > > would be missing a spot that needed a new setup_diff_pager() call, and I\n> > > suspect we don't have good test coverage here.\n> >\n> > This is actually my concern as well when I was naively testing the coverage\n> > using GDB, which turned out to be quite tedious. Would you consider it's fine to\n> > add a pager consistency test for builtins, probably in another patch with regard\n> > to `t7006-pager.sh` OR a new test `t7007`?\n>\n> TBH, I am not all that worried about adding tests just for your patch.\n> You'd need to identify all of the possible diff code paths in order to\n> add tests for them, which is the same thing you had to do to fix the\n> code paths. I was mostly just commenting that we're not likely to be\n> able to rely on existing tests to help us here.\n>\n> It might be worth adding a test that shows off your improved diff\n> behavior, though I would be OK if it was a representative command and\n> not exhaustive. I think adding to t7006 should be fine.\n\nAgreed.\n\nThanks,\nTaylor\n"},{"id":"507454","messageId":"xmqq34jpl5sl.fsf@gitster.g","threadId":"62369","inReplyTo":"ZxatqdBiB+NoMP+j@nand.local","subject":"Re: [PATCH] diff: setup pager only before diff contents truly ready","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-18T00:55:06Z","receivedAt":"2024-11-18T00:55:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n>> You'd need to identify all of the possible diff code paths in order to\n>> add tests for them, which is the same thing you had to do to fix the\n>> code paths. I was mostly just commenting that we're not likely to be\n>> able to rely on existing tests to help us here.\n>>\n>> It might be worth adding a test that shows off your improved diff\n>> behavior, though I would be OK if it was a representative command and\n>> not exhaustive. I think adding to t7006 should be fine.\n>\n> Agreed.\n\nIt seems that this topic is waiting for a reroll?\n"},{"id":"508094","messageId":"20241125113105.GA1070162@coredump.intra.peff.net","threadId":"62369","inReplyTo":"xmqq34jpl5sl.fsf@gitster.g","subject":"Re: [PATCH] diff: setup pager only before diff contents truly ready","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-11-25T11:31:05Z","receivedAt":"2024-11-25T11:31:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 18, 2024 at 09:55:06AM +0900, Junio C Hamano wrote:\n\n> Taylor Blau <me@ttaylorr.com> writes:\n> \n> >> You'd need to identify all of the possible diff code paths in order to\n> >> add tests for them, which is the same thing you had to do to fix the\n> >> code paths. I was mostly just commenting that we're not likely to be\n> >> able to rely on existing tests to help us here.\n> >>\n> >> It might be worth adding a test that shows off your improved diff\n> >> behavior, though I would be OK if it was a representative command and\n> >> not exhaustive. I think adding to t7006 should be fine.\n> >\n> > Agreed.\n> \n> It seems that this topic is waiting for a reroll?\n\nI think we could go either way. I outlined a few further possible steps,\nbut there is no need to hold up this first step. The only question is\nwhether or not to add a single test to show off and protect the\nimprovement.\n\n-Peff\n"},{"id":"508515","messageId":"xmqqo71tcovl.fsf@gitster.g","threadId":"62369","inReplyTo":"20241125113105.GA1070162@coredump.intra.peff.net","subject":"Re: [PATCH] diff: setup pager only before diff contents truly ready","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-03T05:33:02Z","receivedAt":"2024-12-03T05:33:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Mon, Nov 18, 2024 at 09:55:06AM +0900, Junio C Hamano wrote:\n>\n>> Taylor Blau <me@ttaylorr.com> writes:\n>> \n>> >> You'd need to identify all of the possible diff code paths in order to\n>> >> add tests for them, which is the same thing you had to do to fix the\n>> >> code paths. I was mostly just commenting that we're not likely to be\n>> >> able to rely on existing tests to help us here.\n>> >>\n>> >> It might be worth adding a test that shows off your improved diff\n>> >> behavior, though I would be OK if it was a representative command and\n>> >> not exhaustive. I think adding to t7006 should be fine.\n>> >\n>> > Agreed.\n>> \n>> It seems that this topic is waiting for a reroll?\n>\n> I think we could go either way. I outlined a few further possible steps,\n> but there is no need to hold up this first step. The only question is\n> whether or not to add a single test to show off and protect the\n> improvement.\n\nHmph, a few messages upthread\n<UZMh2lyzbLOgsf0PXfMnq6HnWVnCK3y36jY3IMKUykPi74ztNucf8bgywoeO0DdeApq31JDDGMZiEya99zAcI3l8y_zcVqiN8FpEnT1DRZU=@proton.me>\n(Ugh, why do some MUAs or mail providers use such an overly long\nmessage ID, Yuck) was where I got the impression that we were\nwaiting for a reroll.\n\nI am not all that convinced that sprinkling setup_diff_pager() call\nall over the place is a good idea from longer-term maintainability's\nsake to begin with, by the way.  What problem are we really solving?\nFolks who run \"git diff --no-such-option\" see \"behaviour inconsistency\"?\nAll I see is \"error: invalid option: --invalid\" followed by a help message,\nwhich is quite expected.\n\nSo, I donno.\n\nThanks.\n"},{"id":"508560","messageId":"20241203212441.GA1424493@coredump.intra.peff.net","threadId":"62369","inReplyTo":"xmqqo71tcovl.fsf@gitster.g","subject":"Re: [PATCH] diff: setup pager only before diff contents truly ready","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-12-03T21:24:41Z","receivedAt":"2024-12-03T21:24:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 03, 2024 at 02:33:02PM +0900, Junio C Hamano wrote:\n\n> >> It seems that this topic is waiting for a reroll?\n> >\n> > I think we could go either way. I outlined a few further possible steps,\n> > but there is no need to hold up this first step. The only question is\n> > whether or not to add a single test to show off and protect the\n> > improvement.\n> \n> Hmph, a few messages upthread\n> <UZMh2lyzbLOgsf0PXfMnq6HnWVnCK3y36jY3IMKUykPi74ztNucf8bgywoeO0DdeApq31JDDGMZiEya99zAcI3l8y_zcVqiN8FpEnT1DRZU=@proton.me>\n> (Ugh, why do some MUAs or mail providers use such an overly long\n> message ID, Yuck) was where I got the impression that we were\n> waiting for a reroll.\n\nYeah, I think Philip was offering to add some tests. Since he has been\nquiet since, I do not have a strong opinion on whether we should just\ntake it as-is or let it go unless he comes back.\n\n> I am not all that convinced that sprinkling setup_diff_pager() call\n> all over the place is a good idea from longer-term maintainability's\n> sake to begin with, by the way.  What problem are we really solving?\n> Folks who run \"git diff --no-such-option\" see \"behaviour inconsistency\"?\n> All I see is \"error: invalid option: --invalid\" followed by a help message,\n> which is quite expected.\n\nI think it is just about not starting the pager when there is no useful\noutput produced. Depending on your pager config, it is a little nicer if\nwe avoid it for a one-liner error. E.g., I do not use \"-F\", so \"git diff\nfoo bar\" drops me into the pager with a single error line.\n\n-Peff\n"},{"id":"508589","messageId":"xmqqa5dc712d.fsf@gitster.g","threadId":"62369","inReplyTo":"20241203212441.GA1424493@coredump.intra.peff.net","subject":"Re: [PATCH] diff: setup pager only before diff contents truly ready","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-04T00:18:34Z","receivedAt":"2024-12-04T00:18:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> I am not all that convinced that sprinkling setup_diff_pager() call\n>> all over the place is a good idea from longer-term maintainability's\n>> sake to begin with, by the way.  What problem are we really solving?\n>> Folks who run \"git diff --no-such-option\" see \"behaviour inconsistency\"?\n>> All I see is \"error: invalid option: --invalid\" followed by a help message,\n>> which is quite expected.\n>\n> I think it is just about not starting the pager when there is no useful\n> output produced. Depending on your pager config, it is a little nicer if\n> we avoid it for a one-liner error. E.g., I do not use \"-F\", so \"git diff\n> foo bar\" drops me into the pager with a single error line.\n\nOOok...\n"}]}