{"thread":{"id":"30822","subject":"[PATCH 0/2] diff --no-index pager cleanups","startedAt":"2012-06-15T20:28:13Z","lastAt":"2012-06-15T21:18:15Z","messageCount":4,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"193735","messageId":"20120615202813.GA12253@sigill.intra.peff.net","threadId":"30822","inReplyTo":null,"subject":"[PATCH 0/2] diff --no-index pager cleanups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-06-15T20:28:13Z","receivedAt":"2012-06-15T20:28:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"While testing Tim's broken case, I noticed a few annoyances with the\npager handling. These patches should clear them up.\n\n  [1/2]: fix pager.diff with diff --no-index\n  [2/2]: do not run pager with diff --no-index --quiet\n\n-Peff\n"},{"id":"193736","messageId":"20120615202948.GA12326@sigill.intra.peff.net","threadId":"30822","inReplyTo":"20120615202813.GA12253@sigill.intra.peff.net","subject":"[PATCH 1/2] fix pager.diff with diff --no-index","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-06-15T20:29:48Z","receivedAt":"2012-06-15T20:29:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"git-diff does not rely on the git wrapper to setup its\npager; instead, it sets it up on its own after seeing\nwhether --quiet or --exit-code has been specified.  After\ndiff_no_index was split off from cmd_diff, commit b3fde6c\n(git diff --no-index: default to page like other diff\nfrontends, 2008-05-26) duplicated the one-liner from\ncmd_diff to turn on the pager.\n\nLater, commit 8f0359f (Allow pager of diff command be\nenabled/disabled, 2008-07-21) taught the the version in\ncmd_diff to respect the pager.diff config, but the version\nin diff_no_index was left behind. This meant that\n\n  git -c pager.diff=0 diff a b\n\nwould not use a pager, but\n\n  git -c pager.diff=0 diff --no-index a b\n\nwould.  Let's fix it by factoring out a common function.\n\nWhile we're there, let's update the antiquated comment,\nwhich claims that the pager interferes with propagating the\nexit code; this has not been the case since ea27a18 (spawn\npager via run_command interface, 2008-07-22).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin.h       |  2 ++\n builtin/diff.c  | 24 +++++++++++++++++-------\n diff-no-index.c |  7 +------\n 3 files changed, 20 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin.h b/builtin.h\nindex 338f540..e426de3 100644\n--- a/builtin.h\n+++ b/builtin.h\n@@ -41,6 +41,8 @@ int copy_note_for_rewrite(struct notes_rewrite_cfg *c,\n void finish_copy_notes_for_rewrite(struct notes_rewrite_cfg *c);\n \n extern int check_pager_config(const char *cmd);\n+struct diff_options;\n+extern void setup_diff_pager(struct diff_options *);\n \n extern int textconv_object(const char *path, unsigned mode, const unsigned char *sha1, char **buf, unsigned long *buf_size);\n \ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 9069dc4..da8f6aa 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -304,13 +304,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \n \tDIFF_OPT_SET(&rev.diffopt, RECURSIVE);\n \n-\t/*\n-\t * If the user asked for our exit code then don't start a\n-\t * pager or we would end up reporting its exit code instead.\n-\t */\n-\tif (!DIFF_OPT_TST(&rev.diffopt, EXIT_WITH_STATUS) &&\n-\t    check_pager_config(\"diff\") != 0)\n-\t\tsetup_pager();\n+\tsetup_diff_pager(&rev.diffopt);\n \n \t/*\n \t * Do we have --cached and not have a pending object, then\n@@ -421,3 +415,19 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\trefresh_index_quietly();\n \treturn result;\n }\n+\n+void setup_diff_pager(struct diff_options *opt)\n+{\n+\t/*\n+\t * If the user asked for our exit code, then either they want --quiet\n+\t * or --exit-code. We should definitely not bother with a pager in the\n+\t * former case, as we will generate no output. Since we still properly\n+\t * report our exit code even when a pager is run, we _could_ run a\n+\t * pager with --exit-code. But since we have not done so historically,\n+\t * and because it is easy to find people oneline advising \"git diff\n+\t * --exit-code\" in hooks and other scripts, we do not do so.\n+\t */\n+\tif (!DIFF_OPT_TST(opt, EXIT_WITH_STATUS) &&\n+\t    check_pager_config(\"diff\") != 0)\n+\t\tsetup_pager();\n+}\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex f0b0010..fc1decd 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -224,12 +224,7 @@ void diff_no_index(struct rev_info *revs,\n \t\t}\n \t}\n \n-\t/*\n-\t * If the user asked for our exit code then don't start a\n-\t * pager or we would end up reporting its exit code instead.\n-\t */\n-\tif (!DIFF_OPT_TST(&revs->diffopt, EXIT_WITH_STATUS))\n-\t\tsetup_pager();\n+\tsetup_diff_pager(&revs->diffopt);\n \n \tif (prefix) {\n \t\tint len = strlen(prefix);\n-- \n1.7.11.rc2.4.g0b9de53\n"},{"id":"193737","messageId":"20120615203255.GB12326@sigill.intra.peff.net","threadId":"30822","inReplyTo":"20120615202813.GA12253@sigill.intra.peff.net","subject":"[PATCH 2/2] do not run pager with diff --no-index --quiet","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-06-15T20:32:55Z","receivedAt":"2012-06-15T20:32:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There is no point in running a pager when --quiet is given,\nsince we are producing no output. The regular diff code path\nhandles this already, because --quiet implies --exit-code,\nand we check for --exit-code when deciding not to run the\npager.\n\nHowever, the \"quiet implies exit-code\" logic is done in\ndiff_setup_done, and the no-index code path sets up its\npager before running diff_setup_done, and misses this case.\n\nWe can fix this by reordering our initialization.\nCurrently we do:\n\n  1. read command line arguments into diff_options\n\n  2. Set pager if EXIT_CODE not requested\n\n  3. always set EXIT_CODE, since we are emulating\n     traditional diff\n\n  4. call diff_setup_done\n\nWe can fix the problem by moving pager initialization (step\n2) after step 4. But step 3 must come after step 2 (since we\nwant to know whether the _user_ requested --exit-code, not\nwhether we turned it on unconditionally). So we must move\nboth.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nWe could also just check for QUICK in setup_diff_pager, which should be\na no-op for the regular diff case. But I think doing things in the same\norder as regular diff could help prevent similar bugs in the future.\n\n diff-no-index.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex fc1decd..77667b8 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -224,8 +224,6 @@ void diff_no_index(struct rev_info *revs,\n \t\t}\n \t}\n \n-\tsetup_diff_pager(&revs->diffopt);\n-\n \tif (prefix) {\n \t\tint len = strlen(prefix);\n \t\tconst char *paths[3];\n@@ -250,13 +248,15 @@ void diff_no_index(struct rev_info *revs,\n \tif (!revs->diffopt.output_format)\n \t\trevs->diffopt.output_format = DIFF_FORMAT_PATCH;\n \n-\tDIFF_OPT_SET(&revs->diffopt, EXIT_WITH_STATUS);\n \tDIFF_OPT_SET(&revs->diffopt, NO_INDEX);\n \n \trevs->max_count = -2;\n \tif (diff_setup_done(&revs->diffopt) < 0)\n \t\tdie(\"diff_setup_done failed\");\n \n+\tsetup_diff_pager(&revs->diffopt);\n+\tDIFF_OPT_SET(&revs->diffopt, EXIT_WITH_STATUS);\n+\n \tif (queue_diff(&revs->diffopt, revs->diffopt.pathspec.raw[0],\n \t\t       revs->diffopt.pathspec.raw[1]))\n \t\texit(1);\n-- \n1.7.11.rc2.4.g0b9de53\n"},{"id":"193740","messageId":"7v1ulg9svc.fsf@alter.siamese.dyndns.org","threadId":"30822","inReplyTo":"20120615202813.GA12253@sigill.intra.peff.net","subject":"Re: [PATCH 0/2] diff --no-index pager cleanups","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-06-15T21:18:15Z","receivedAt":"2012-06-15T21:18:15Z","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> While testing Tim's broken case, I noticed a few annoyances with the\n> pager handling. These patches should clear them up.\n>\n>   [1/2]: fix pager.diff with diff --no-index\n>   [2/2]: do not run pager with diff --no-index --quiet\n\nBoth look sane to me.  Thanks.\n"}]}