From: Patrick Steinhardt <ps@pks.im>
Date: Thu, 18 Sep 2025 06:03:02 GMT
Subject: Re: [PATCH v2 5/5] builtin/config: do not spawn pager when printing color codes
Message-ID: <aMuglu_TtQPht1xP@pks.im>
In-Reply-To: <xmqqikhjhbgo.fsf@gitster.g>

On Mon, Sep 15, 2025 at 10:28:23AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > With `git config get --type=color` the user asks us to parse a specific
> > configuration key and turn the value into an ANSI color escape sequence.
> > The printed string can then for example be used as part of shell scripts
> > to reuse the same colors as Git.
> >
> > Right now though we set up the auto-pager, which means that the string
> > may instead be written to the pager command. This is of course quite
> > nonsensical; there shouldn't be any use case where the color code should
> > end up in the pager instead of in the TTY.
> >
> > Fix this by disabling the pager in case the user is asking us to print
> > color sequences.
> 
> I am of two minds.  Part of me obviously agrees that it is more
> straight forward with this change.  But it may 
> 
> An interactive user experimenting while writing their own script
> might say something like
> 
>  $ git config --type=color --default="reverse red" n.n
> 
> If the command emitted directly to the terminal, then everything
> they type from then on will be bloody red, but the pager protects
> them from such an accident.  Instead, they are forced to say
> 
>  $ C=$(git config get --type=color --default="reverse red" n.n)
>  $ R=$(git config get --type=color --default="reset" n.n)
>  $ echo "So$C Bloody ${R}Red"
> 
> but these are likely what they would be writing in their script
> anyway, so...

True. That being said, I'm mostly trying to emulate the old behaviour
that we had in `git config --get-color`. We have the following condition
there:

	/*
	 * The following actions may produce more than one line of output and
	 * should therefore be paged.
	 */
	if (actions & (ACTION_LIST | ACTION_GET_ALL | ACTION_GET_REGEXP | ACTION_GET_URLMATCH))
		setup_auto_pager("config", 1);

`ACTION_GET_COLOR` is not part of this condition, so we wouldn't set up
the auto pager there, either. So I think it's sensible to match that
behaviour so that the new command really is a drop-in replacement for
the old one.

I should probably clarify the commit message.

Patrick