* [RFC PATCH] diff --no-index: test for pager after option parsing
@ 2009-01-06 23:56 Thomas Rast
2009-01-07 0:09 ` Thomas Rast
2009-01-07 0:09 ` Junio C Hamano
0 siblings, 2 replies; 8+ messages in thread
From: Thomas Rast @ 2009-01-06 23:56 UTC (permalink / raw
To: git; +Cc: Junio C Hamano
We need to parse options before we can see if --exit-code was
provided.
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
I noticed this while working on the earlier patch for diff --no-index.
It seems like the right thing to do (and passes tests), but I don't
have a clue about git's normal setup sequences, so I'm flagging it
RFC.
diff-no-index.c | 14 +++++++-------
1 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/diff-no-index.c b/diff-no-index.c
index b60d345..f655f64 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -198,13 +198,6 @@ void diff_no_index(struct rev_info *revs,
die("git diff %s takes two paths",
no_index ? "--no-index" : "[--no-index]");
- /*
- * 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();
-
diff_setup(&revs->diffopt);
if (!revs->diffopt.output_format)
revs->diffopt.output_format = DIFF_FORMAT_PATCH;
@@ -222,6 +215,13 @@ 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();
+
if (prefix) {
int len = strlen(prefix);
--
tg: (e9b8523..) t/diff-no-index-status (depends on: origin/master)
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [RFC PATCH] diff --no-index: test for pager after option parsing
2009-01-06 23:56 [RFC PATCH] diff --no-index: test for pager after option parsing Thomas Rast
@ 2009-01-07 0:09 ` Thomas Rast
2009-01-07 0:09 ` Junio C Hamano
1 sibling, 0 replies; 8+ messages in thread
From: Thomas Rast @ 2009-01-07 0:09 UTC (permalink / raw
To: git; +Cc: Junio C Hamano
[-- Attachment #1: Type: text/plain, Size: 859 bytes --]
Speaking of diff --no-index code, there's also this bit:
/* diff-no-index.c:206 */
for (i = 1; i < argc - 2; ) {
int j;
if (!strcmp(argv[i], "--no-index"))
i++;
else if (!strcmp(argv[1], "-q"))
options |= DIFF_SILENT_ON_REMOVED;
Note the argv[i] vs. argv[1]. The entire block is from 0569e9b ("git
diff": do not ignore index without --no-index, 2008-05-23).
While it seems obvious that this should be argv[i], I'm rather
confused by the option itself. It is not documented in my version of
git-diff(1). Furthermore, I can't see what being silent about removed
paths (which relates to the index?) has to do with a diff --no-index
(which takes two paths that must exist).
Or perhaps I should take the "no mails/patches after midnight" rule a
tad bit more serious...
--
Thomas Rast
trast@{inf,student}.ethz.ch
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH] diff --no-index: test for pager after option parsing
2009-01-06 23:56 [RFC PATCH] diff --no-index: test for pager after option parsing Thomas Rast
2009-01-07 0:09 ` Thomas Rast
@ 2009-01-07 0:09 ` Junio C Hamano
2009-01-07 3:20 ` Miklos Vajna
1 sibling, 1 reply; 8+ messages in thread
From: Junio C Hamano @ 2009-01-07 0:09 UTC (permalink / raw
To: Thomas Rast; +Cc: git
Thomas Rast <trast@student.ethz.ch> writes:
> I noticed this while working on the earlier patch for diff --no-index.
> It seems like the right thing to do (and passes tests), but I don't
> have a clue about git's normal setup sequences, so I'm flagging it
> RFC.
I think the patch itself makes sense from the logic flow point of view.
But I wonder if it still makes a difference in real life.idn't we stop
reporting the exit status from the pager some time ago?
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH] diff --no-index: test for pager after option parsing
2009-01-07 0:09 ` Junio C Hamano
@ 2009-01-07 3:20 ` Miklos Vajna
2009-01-07 6:42 ` Jeff King
2009-01-07 7:02 ` Junio C Hamano
0 siblings, 2 replies; 8+ messages in thread
From: Miklos Vajna @ 2009-01-07 3:20 UTC (permalink / raw
To: Junio C Hamano; +Cc: Thomas Rast, git
[-- Attachment #1: Type: text/plain, Size: 352 bytes --]
On Tue, Jan 06, 2009 at 04:09:18PM -0800, Junio C Hamano <gitster@pobox.com> wrote:
> But I wonder if it still makes a difference in real life.idn't we stop
> reporting the exit status from the pager some time ago?
I just wanted to write this, I think that code could be just removed
since ea27a18 (spawn pager via run_command interface, 2008-07-22).
[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH] diff --no-index: test for pager after option parsing
2009-01-07 3:20 ` Miklos Vajna
@ 2009-01-07 6:42 ` Jeff King
2009-01-07 7:02 ` Junio C Hamano
1 sibling, 0 replies; 8+ messages in thread
From: Jeff King @ 2009-01-07 6:42 UTC (permalink / raw
To: Miklos Vajna; +Cc: Junio C Hamano, Thomas Rast, git
On Wed, Jan 07, 2009 at 04:20:13AM +0100, Miklos Vajna wrote:
> On Tue, Jan 06, 2009 at 04:09:18PM -0800, Junio C Hamano <gitster@pobox.com> wrote:
> > But I wonder if it still makes a difference in real life.idn't we stop
> > reporting the exit status from the pager some time ago?
>
> I just wanted to write this, I think that code could be just removed
> since ea27a18 (spawn pager via run_command interface, 2008-07-22).
I don't think just removing it is right. You would also need to put
SETUP_PAGER into the flags for calling cmd_diff.
We do pass along the error code properly these days, but I think it is
nice that --exit-code always just suppresses the pager. Otherwise a
script like this:
if git diff --exit-code $x $y; then
do something
fi
will invoke the pager (and not everybody's setup immediately exits if
there is no output, either because they have different LESS options or
because they use a different pager). Of course one might argue that the
script should not be using "git diff" porcelain at all, but I don't
think there is another way to get a --no-index diff.
-Peff
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH] diff --no-index: test for pager after option parsing
2009-01-07 3:20 ` Miklos Vajna
2009-01-07 6:42 ` Jeff King
@ 2009-01-07 7:02 ` Junio C Hamano
2009-01-07 11:15 ` [PATCH] diff --no-index -q: fix endless loop Thomas Rast
1 sibling, 1 reply; 8+ messages in thread
From: Junio C Hamano @ 2009-01-07 7:02 UTC (permalink / raw
To: Miklos Vajna; +Cc: Thomas Rast, git
Miklos Vajna <vmiklos@frugalware.org> writes:
> On Tue, Jan 06, 2009 at 04:09:18PM -0800, Junio C Hamano <gitster@pobox.com> wrote:
>> But I wonder if it still makes a difference in real life.idn't we stop
>> reporting the exit status from the pager some time ago?
>
> I just wanted to write this, I think that code could be just removed
> since ea27a18 (spawn pager via run_command interface, 2008-07-22).
I think we shouldn't.
People may already have got used to "git diff --exit-code" to disable the
pager, and doing the same for "git diff --exit-code --no-index" should be
with less surprises.
I'll queue the "--" fix, "-q" fix and this pager fix. Thanks.
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] diff --no-index -q: fix endless loop
2009-01-07 7:02 ` Junio C Hamano
@ 2009-01-07 11:15 ` Thomas Rast
2009-01-07 21:30 ` Junio C Hamano
0 siblings, 1 reply; 8+ messages in thread
From: Thomas Rast @ 2009-01-07 11:15 UTC (permalink / raw
To: Junio C Hamano; +Cc: git
We forgot to move to the next argument when parsing -q, getting stuck
in an endless loop.
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
Junio C Hamano wrote:
> I'll queue the "--" fix, "-q" fix and this pager fix. Thanks.
Seems the after-midnight rule indeed has some value. I pointed out
the argv[i] bug, which I see you have squashed into the -- fix, but
failed to notice that the -q parsing is also missing an i++.
I'm still not convinced of the option's value, but this patch at least
fixes the bug.
diff-no-index.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/diff-no-index.c b/diff-no-index.c
index 12ff1f1..60ed174 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -207,8 +207,10 @@ void diff_no_index(struct rev_info *revs,
int j;
if (!strcmp(argv[i], "--no-index"))
i++;
- else if (!strcmp(argv[i], "-q"))
+ else if (!strcmp(argv[i], "-q")) {
options |= DIFF_SILENT_ON_REMOVED;
+ i++;
+ }
else if (!strcmp(argv[i], "--"))
i++;
else {
--
tg: (3bbe36c..) t/diff-q-endless (depends on: next)
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] diff --no-index -q: fix endless loop
2009-01-07 11:15 ` [PATCH] diff --no-index -q: fix endless loop Thomas Rast
@ 2009-01-07 21:30 ` Junio C Hamano
0 siblings, 0 replies; 8+ messages in thread
From: Junio C Hamano @ 2009-01-07 21:30 UTC (permalink / raw
To: Thomas Rast; +Cc: git
Thanks.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2009-01-07 21:32 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-01-06 23:56 [RFC PATCH] diff --no-index: test for pager after option parsing Thomas Rast
2009-01-07 0:09 ` Thomas Rast
2009-01-07 0:09 ` Junio C Hamano
2009-01-07 3:20 ` Miklos Vajna
2009-01-07 6:42 ` Jeff King
2009-01-07 7:02 ` Junio C Hamano
2009-01-07 11:15 ` [PATCH] diff --no-index -q: fix endless loop Thomas Rast
2009-01-07 21:30 ` Junio C Hamano
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).