Re: git-diff should not fire up $PAGER if there is no diff

Re: git-diff should not fire up $PAGER if there is no diff

[jidanni]

>>>>> "JS" == Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

JS> Maybe you are not aware just how annoying these "this should do that, that
JS> should do this" without a patch are?

Yes but I must first learn the thing I am about to make a patch for.

In the mean time I could like most beginners, just feel dumb.

Or I could let you have my valuable insights on how things look to a
beginner. You would else have no way of knowing. Sorry I always come
across as Don Rickles the pushy customer... it's all a charade... now
let's get back to business,

>>>>> "JK" == Jeff King <peff@peff.net> writes:

JK> The usual workaround is to ask the pager to exit immediately if the
JK> output is small. I.e., putting "F" in your LESS variable (which git does
JK> automatically if you don't already have LESS set).

Ah, but who doesn't have LESS set these days? OK, the man page should
mention that so people will put the F on their LESS variable, like I
recall another man page already does.

Re: git-diff should not fire up $PAGER if there is no diff

[Jeff King]

On Tue, Dec 16, 2008 at 09:16:29AM +0800, jidanni@jidanni.org wrote:

> JK> The usual workaround is to ask the pager to exit immediately if the
> JK> output is small. I.e., putting "F" in your LESS variable (which git does
> JK> automatically if you don't already have LESS set).
> 
> Ah, but who doesn't have LESS set these days? OK, the man page should
> mention that so people will put the F on their LESS variable, like I
> recall another man page already does.

Not unreasonable. Care to make a patch to the documentation?

-Peff

Re: git-diff should not fire up $PAGER if there is no diff

[jidanni]

Oh barf, having LESS=F in one's environment messes up less +G:
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=508919

git-diff should not fire up $PAGER if there is no diff

[jidanni]

git-diff should not fire up $PAGER if there is no diff output.
Just exit. The man page doesn't even mention $PAGER too.

Re: git-diff should not fire up $PAGER if there is no diff

[Jeff King]

On Tue, Dec 16, 2008 at 08:21:33AM +0800, jidanni@jidanni.org wrote:

> git-diff should not fire up $PAGER if there is no diff output.
> Just exit. The man page doesn't even mention $PAGER too.

I agree that would be nice, but it is a little difficult to implement.
The current behavior forks early and then pipes the output to the pager.
So we would have to:

  1. change that behavior to instead delay starting the pager until the
     first output. Which means intercepting every
     write/fwrite/printf/fputs/etc call.

  2. detect EOF before starting the pager. We in fact already delay
     running the pager in the forked process until we have some activity
     on the pipe, but I don't know if there is a portable way of
     detecting that that activity is EOF without performing an actual
     read() call (which is undesirable, since it eats the first byte of
     output that should go to the pager).

  3. a hacky solution to (2) above would be to make _2_ pipes, one of
     which signals to the pager sub-process either "exit now" or "proceed
     with running the pager".

The usual workaround is to ask the pager to exit immediately if the
output is small. I.e., putting "F" in your LESS variable (which git does
automatically if you don't already have LESS set).

-Peff

Re: git-diff should not fire up $PAGER if there is no diff

[Stefan Karpinski]

> On Mon, Dec 15, 2008 at 7:56 PM, Jeff King <peff@peff.net> wrote:
>  2. detect EOF before starting the pager. We in fact already delay
>     running the pager in the forked process until we have some activity
>     on the pipe, but I don't know if there is a portable way of
>     detecting that that activity is EOF without performing an actual
>     read() call (which is undesirable, since it eats the first byte of
>     output that should go to the pager).

Wouldn't ungetc work? Or is that not portable enough? (It would only
work here because the EOF has to be the first character.)

Re: git-diff should not fire up $PAGER if there is no diff

[Jeff King]

On Tue, Dec 16, 2008 at 01:35:53AM -0500, Stefan Karpinski wrote:

> > On Mon, Dec 15, 2008 at 7:56 PM, Jeff King <peff@peff.net> wrote:
> >  2. detect EOF before starting the pager. We in fact already delay
> >     running the pager in the forked process until we have some activity
> >     on the pipe, but I don't know if there is a portable way of
> >     detecting that that activity is EOF without performing an actual
> >     read() call (which is undesirable, since it eats the first byte of
> >     output that should go to the pager).
> 
> Wouldn't ungetc work? Or is that not portable enough? (It would only
> work here because the EOF has to be the first character.)

No, it won't work. ungetc works on the buffered stdio object, so it is
useful for pushing back characters onto the buffer to be read later in
the program from the same buffer. But in this case, we are going to
execv() (or on Windows, spawn) the pager, meaning it will throw away
anything that has been read() from the pipe and put in the buffer.

So we would need a system call to push a character back to the OS, so
that it was available for read() by the pager process.

-Peff

Re: git-diff should not fire up $PAGER if there is no diff

[Stefan Karpinski]

On Tue, Dec 16, 2008 at 2:44 AM, Jeff King <peff@peff.net> wrote:
> On Tue, Dec 16, 2008 at 01:35:53AM -0500, Stefan Karpinski wrote:
>
>> > On Mon, Dec 15, 2008 at 7:56 PM, Jeff King <peff@peff.net> wrote:
>> >  2. detect EOF before starting the pager. We in fact already delay
>> >     running the pager in the forked process until we have some activity
>> >     on the pipe, but I don't know if there is a portable way of
>> >     detecting that that activity is EOF without performing an actual
>> >     read() call (which is undesirable, since it eats the first byte of
>> >     output that should go to the pager).
>>
>> Wouldn't ungetc work? Or is that not portable enough? (It would only
>> work here because the EOF has to be the first character.)
>
> No, it won't work. ungetc works on the buffered stdio object, so it is
> useful for pushing back characters onto the buffer to be read later in
> the program from the same buffer. But in this case, we are going to
> execv() (or on Windows, spawn) the pager, meaning it will throw away
> anything that has been read() from the pipe and put in the buffer.
>
> So we would need a system call to push a character back to the OS, so
> that it was available for read() by the pager process.

Yeah, I realized that after I sent the message. Late night sending bad!