Re: [PATCH] log: if --decorate is not given, default to --decorate=auto

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Mar 23, 2017 at 10:52:34AM -0600, Alex Henrie wrote:

> Unfortunately, I think I found a bug. Even when using `git -p`, the
> function pager_in_use() always returns false if the output is not a
> TTY. So, `isatty(1) || pager_in_use()` and `color_stdout_is_tty ||
> (pager_in_use() && pager_use_color)` are redundant.
> 
> If we want to use `git -p log` in a test, we'll have to change the
> behavior of pager_in_use(). Alternatively, we could use
> `GIT_PAGER_IN_USE=1 git log` instead.

I see you ended up with a test that uses test_terminal, which is much
better (and your patch looks good to me).

But I was concerned that there might be a bug in pager_in_use(), so I
dug into it a little. I think the code there is correct; it's just
relaying the environment variable's value. Likewise, on the setting
side, I think the code is correct. We set the environment variable
whenever we start the pager in setup_pager().

I think what is confusing is that "git -p" does _not_ mean
"unconditionally use a pager". It means "start a pager if stdout is a
terminal, even if this command is not usually paged". So something like
"git -p log >actual" is correct not to trigger pager_in_use().

I also double-checked the documentation, which covers this case
accurately. So I think all is well, and there's nothing to fix. You
might want an option for "even though stdout is not a tty pretend like a
pager is in use", but that is exactly what GIT_PAGER_IN_USE=1 is for.

-Peff



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]