Re: [PATCH] builtin/config.c: don't print a newline with --color

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 5, 2019, 05:57 UTC
Message-ID: <xmqqva0xvp6b.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20190305042050.GE19800@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 26 quoted lines

> I do wonder, though, if we're digging ourselves a hole with the
> inconsistency between different --types that will bite us later. Given
> that it's not that hard to chomp the output (and as you noted, the shell
> does it fairly transparently), and given that the caller has to switch
> between "--get-color" and "--type=color", it's not that hard to handle
> the output differently if you know to do so.
>
> Mostly I was just surprised by the new behavior. Perhaps the right
> solution is not a patch to the code, but to the documentation. Something
> like:
>
> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> index 495bb57416..61f3a9cdd7 100644
> --- a/Documentation/git-config.txt
> +++ b/Documentation/git-config.txt
> @@ -252,7 +252,9 @@ Valid `<type>`'s include:
>  	output.  The optional `default` parameter is used instead, if
>  	there is no color configured for `name`.
>  +
> -`--type=color [--default=<default>]` is preferred over `--get-color`.
> +`--type=color [--default=<default>]` is preferred over `--get-color`
> +(but note that `--get-color` will omit the trailing newline printed by
> +--type=color).
>  
>  -e::
>  --edit::

Yup, that would be a very sensible first step, regardless of what the next step is.

After that, choices are

 (1) we'd introduce new inconsistency among --type=<type> by
     matching what --type=color does to what --get-color does, to
     allow us to revert that documentation update, or

 (2) we'd drop LF from all --type=<type>, that makes everything
     consistent and risk breaking a few existing scripts while doing
     so, and get yelled at by end users, or

 (3) we stop at this documentation update and do nothing else.