On Thu, 14 Nov 2019 at 07:29, Jeff King <peff@xxxxxxxx> wrote: > This seems like a good idea, but I wonder why we'd limit it to bools? Basically because that's what the existing left-over-bits mentioned and it felt like a good place to start. But you're right in asking about the bigger picture up front. > It seems like any type would probably be better off matching a > normalized version. > > We already have two functions in builtin/config.c which handle types: > > - format_config() is for actually interpreting an existing value and > writing it to stdout > > - normalize_value() is used when writing a new value into a config > file, and normalizing what the user provided on the command-line > > I don't think you'd want to use format_config() here. I just speculated a little around format_config() in a reply to Junio. Already with my patch series and with type=bool-or-int, there's a visible funny-funky corner case where one hand doesn't know what the other is doing. > For example, if I > say: > > git config --type=color --get-regexp ^foo red > > I want to match the original "red" color, but _output_ the ANSI code. > But normalize_value(), by contrast, leaves colors intact. So maybe it's > a good match? > > OTOH, I'd probably expect to match expanded pathnames or expiration > dates there, too, and it doesn't expand those. Those ones are less clear > to me. The whole premise of this series is making an assumption that > "--type" is about how you want to match, Right. > and not just about what you > want to output. In your example above it's clear that you don't care > about the output type (because you're using --name-only), but for: > > git config --type=path --get-regexp ^foo /home/peff > > it's unclear if you want to match a value of "~peff/foo", or if you > simply want to output the expansion. Hmm, I feel like we're opening a can of worms here. > I wonder if we'd want to allow specifying the output type and the > matching type separately? Maybe that just makes it more awkward to use > for little benefit (I admit that it's hard to imagine somebody wanting > to normalize bools on output but _not_ for matching). > > Anyway. It would be nice if we could build on the existing logic in some > way, rather than making a third place that has to handle every type we > know about. Understood. Thanks a lot for sharing your thoughts. > > The last patch is not meant for immediate inclusion, but I post it > > anyway. I can re-submit it at an appropriate time, or maybe it could > > slumber on pu until the time is ripe for completing the switch. > > I think bailing on values that can't be converted is normal for other > code paths. E.g., just trying to print: > > $ git -c foo.bar=abc config --type=bool foo.abr > fatal: bad numeric config value 'abc' for 'foo.bar': invalid unit I'm not sure if you mean "... so we could be a lot more aggressive here"? I'm running now and I feel like I'll need to read your mail again tonight and get back to you in more detail. Thanks Martin
