* [PATCH 0/2] diff --no-index pager cleanups
@ 2012-06-15 20:28 Jeff King
2012-06-15 20:29 ` [PATCH 1/2] fix pager.diff with diff --no-index Jeff King
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Jeff King @ 2012-06-15 20:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Tim Henigan, git
While testing Tim's broken case, I noticed a few annoyances with the
pager handling. These patches should clear them up.
[1/2]: fix pager.diff with diff --no-index
[2/2]: do not run pager with diff --no-index --quiet
-Peff
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH 1/2] fix pager.diff with diff --no-index
2012-06-15 20:28 [PATCH 0/2] diff --no-index pager cleanups Jeff King
@ 2012-06-15 20:29 ` Jeff King
2012-06-15 20:32 ` [PATCH 2/2] do not run pager with diff --no-index --quiet Jeff King
2012-06-15 21:18 ` [PATCH 0/2] diff --no-index pager cleanups Junio C Hamano
2 siblings, 0 replies; 4+ messages in thread
From: Jeff King @ 2012-06-15 20:29 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Tim Henigan, git
git-diff does not rely on the git wrapper to setup its
pager; instead, it sets it up on its own after seeing
whether --quiet or --exit-code has been specified. After
diff_no_index was split off from cmd_diff, commit b3fde6c
(git diff --no-index: default to page like other diff
frontends, 2008-05-26) duplicated the one-liner from
cmd_diff to turn on the pager.
Later, commit 8f0359f (Allow pager of diff command be
enabled/disabled, 2008-07-21) taught the the version in
cmd_diff to respect the pager.diff config, but the version
in diff_no_index was left behind. This meant that
git -c pager.diff=0 diff a b
would not use a pager, but
git -c pager.diff=0 diff --no-index a b
would. Let's fix it by factoring out a common function.
While we're there, let's update the antiquated comment,
which claims that the pager interferes with propagating the
exit code; this has not been the case since ea27a18 (spawn
pager via run_command interface, 2008-07-22).
Signed-off-by: Jeff King <peff@peff.net>
---
builtin.h | 2 ++
builtin/diff.c | 24 +++++++++++++++++-------
diff-no-index.c | 7 +------
3 files changed, 20 insertions(+), 13 deletions(-)
diff --git a/builtin.h b/builtin.h
index 338f540..e426de3 100644
--- a/builtin.h
+++ b/builtin.h
@@ -41,6 +41,8 @@ int copy_note_for_rewrite(struct notes_rewrite_cfg *c,
void finish_copy_notes_for_rewrite(struct notes_rewrite_cfg *c);
extern int check_pager_config(const char *cmd);
+struct diff_options;
+extern void setup_diff_pager(struct diff_options *);
extern int textconv_object(const char *path, unsigned mode, const unsigned char *sha1, char **buf, unsigned long *buf_size);
diff --git a/builtin/diff.c b/builtin/diff.c
index 9069dc4..da8f6aa 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -304,13 +304,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
DIFF_OPT_SET(&rev.diffopt, RECURSIVE);
- /*
- * If the user asked for our exit code then don't start a
- * pager or we would end up reporting its exit code instead.
- */
- if (!DIFF_OPT_TST(&rev.diffopt, EXIT_WITH_STATUS) &&
- check_pager_config("diff") != 0)
- setup_pager();
+ setup_diff_pager(&rev.diffopt);
/*
* Do we have --cached and not have a pending object, then
@@ -421,3 +415,19 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
refresh_index_quietly();
return result;
}
+
+void setup_diff_pager(struct diff_options *opt)
+{
+ /*
+ * If the user asked for our exit code, then either they want --quiet
+ * or --exit-code. We should definitely not bother with a pager in the
+ * former case, as we will generate no output. Since we still properly
+ * report our exit code even when a pager is run, we _could_ run a
+ * pager with --exit-code. But since we have not done so historically,
+ * and because it is easy to find people oneline advising "git diff
+ * --exit-code" in hooks and other scripts, we do not do so.
+ */
+ if (!DIFF_OPT_TST(opt, EXIT_WITH_STATUS) &&
+ check_pager_config("diff") != 0)
+ setup_pager();
+}
diff --git a/diff-no-index.c b/diff-no-index.c
index f0b0010..fc1decd 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -224,12 +224,7 @@ void diff_no_index(struct rev_info *revs,
}
}
- /*
- * If the user asked for our exit code then don't start a
- * pager or we would end up reporting its exit code instead.
- */
- if (!DIFF_OPT_TST(&revs->diffopt, EXIT_WITH_STATUS))
- setup_pager();
+ setup_diff_pager(&revs->diffopt);
if (prefix) {
int len = strlen(prefix);
--
1.7.11.rc2.4.g0b9de53
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH 2/2] do not run pager with diff --no-index --quiet
2012-06-15 20:28 [PATCH 0/2] diff --no-index pager cleanups Jeff King
2012-06-15 20:29 ` [PATCH 1/2] fix pager.diff with diff --no-index Jeff King
@ 2012-06-15 20:32 ` Jeff King
2012-06-15 21:18 ` [PATCH 0/2] diff --no-index pager cleanups Junio C Hamano
2 siblings, 0 replies; 4+ messages in thread
From: Jeff King @ 2012-06-15 20:32 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Tim Henigan, git
There is no point in running a pager when --quiet is given,
since we are producing no output. The regular diff code path
handles this already, because --quiet implies --exit-code,
and we check for --exit-code when deciding not to run the
pager.
However, the "quiet implies exit-code" logic is done in
diff_setup_done, and the no-index code path sets up its
pager before running diff_setup_done, and misses this case.
We can fix this by reordering our initialization.
Currently we do:
1. read command line arguments into diff_options
2. Set pager if EXIT_CODE not requested
3. always set EXIT_CODE, since we are emulating
traditional diff
4. call diff_setup_done
We can fix the problem by moving pager initialization (step
2) after step 4. But step 3 must come after step 2 (since we
want to know whether the _user_ requested --exit-code, not
whether we turned it on unconditionally). So we must move
both.
Signed-off-by: Jeff King <peff@peff.net>
---
We could also just check for QUICK in setup_diff_pager, which should be
a no-op for the regular diff case. But I think doing things in the same
order as regular diff could help prevent similar bugs in the future.
diff-no-index.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/diff-no-index.c b/diff-no-index.c
index fc1decd..77667b8 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -224,8 +224,6 @@ void diff_no_index(struct rev_info *revs,
}
}
- setup_diff_pager(&revs->diffopt);
-
if (prefix) {
int len = strlen(prefix);
const char *paths[3];
@@ -250,13 +248,15 @@ void diff_no_index(struct rev_info *revs,
if (!revs->diffopt.output_format)
revs->diffopt.output_format = DIFF_FORMAT_PATCH;
- DIFF_OPT_SET(&revs->diffopt, EXIT_WITH_STATUS);
DIFF_OPT_SET(&revs->diffopt, NO_INDEX);
revs->max_count = -2;
if (diff_setup_done(&revs->diffopt) < 0)
die("diff_setup_done failed");
+ setup_diff_pager(&revs->diffopt);
+ DIFF_OPT_SET(&revs->diffopt, EXIT_WITH_STATUS);
+
if (queue_diff(&revs->diffopt, revs->diffopt.pathspec.raw[0],
revs->diffopt.pathspec.raw[1]))
exit(1);
--
1.7.11.rc2.4.g0b9de53
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 0/2] diff --no-index pager cleanups
2012-06-15 20:28 [PATCH 0/2] diff --no-index pager cleanups Jeff King
2012-06-15 20:29 ` [PATCH 1/2] fix pager.diff with diff --no-index Jeff King
2012-06-15 20:32 ` [PATCH 2/2] do not run pager with diff --no-index --quiet Jeff King
@ 2012-06-15 21:18 ` Junio C Hamano
2 siblings, 0 replies; 4+ messages in thread
From: Junio C Hamano @ 2012-06-15 21:18 UTC (permalink / raw)
To: Jeff King; +Cc: Tim Henigan, git
Jeff King <peff@peff.net> writes:
> While testing Tim's broken case, I noticed a few annoyances with the
> pager handling. These patches should clear them up.
>
> [1/2]: fix pager.diff with diff --no-index
> [2/2]: do not run pager with diff --no-index --quiet
Both look sane to me. Thanks.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2012-06-15 21:18 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-06-15 20:28 [PATCH 0/2] diff --no-index pager cleanups Jeff King
2012-06-15 20:29 ` [PATCH 1/2] fix pager.diff with diff --no-index Jeff King
2012-06-15 20:32 ` [PATCH 2/2] do not run pager with diff --no-index --quiet Jeff King
2012-06-15 21:18 ` [PATCH 0/2] diff --no-index pager cleanups Junio C Hamano
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.