Re: [PATCH] builtin/config.c: don't print a newline with --color
- From
Jeff King <peff@peff.net>
- Date
- Mar 5, 2019, 04:20 UTC
- Message-ID
- <20190305042050.GE19800@sigill.intra.peff.net>
- In-Reply-To
- <xmqqh8cjw7ob.fsf@gitster-ct.c.googlers.com>
On Mon, Mar 04, 2019 at 02:05:40PM +0900, Junio C Hamano wrote:
Show 23 quoted lines
> >> With respect to backwards compatibility, my thinking on the matter was > >> basically: > >> > >> 1. Since --type=color was supposed to be a drop-in replacement for > >> --get-color, it's a bug that they don't behave the same. > >> > >> 2. It's a fairly recent feature, so nobody really noticed the bug > >> until recently (and it was in fact me who noticed and got annoyed > >> by it). If it were an ancient behavior, we might think about > >> retaining even bug compatibility, but that's not the case here. > > > > Now I think "we weren't consistent to begin with with --get-color, > > and treating --type=color as a special case is justifiable"; and I > > agree with the above two points. > > Just to avoid an awkward situation where the ball gets dropped and > left on the floor forgotten, the above does not mean I am 100% happy > with the patch as posted. There is no mention of --get-color > anywhere, let alone it shows the ANSI sequence without traililng LF, > which I would consider to be the most important part of the > justification. It is much stronger than "I expected there won't be > any trailing LF from 'git config'", which was the only thing I > managed to read in the original and led to my response.
Yeah, I agree it needs to be the main justification in the commit message.
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:: -Peff