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

From: Jeff King <peff@peff.net>
Date: Mar 24, 2017, 18:55 UTC
Message-ID: <20170324185543.uq4clqyvbdkahbzo@sigill.intra.peff.net>
In-Reply-To: <CAMMLpeRtzYM3a639DcNbhqAmz3dNkRebvQps-gusEKx+eQqPgQ@mail.gmail.com>

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

Show 8 quoted lines

> 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