| Message ID | cover.1709724089.git.ps@pks.im (mailing list archive) |
|---|---|
| Headers | show |
| Series | builtin/config: introduce subcommands | expand |
Patrick Steinhardt <ps@pks.im> writes: > - `git config foo.bar` -> `git config get foo.bar` > > - `git config foo.bar value` -> `git config set foo.bar value` I actually have been perfectly OK with the above two, but I agree that ... > - `git config foo.bar value value-pattern` -> `git config set-all > foo.bar value value-pattern` ... this was less than discoverable, and would be a good update. This one ... > - `git config --get-urlmatch` -> `git config get-urlmatch`. ... is a Meh to me, personally. I'd not actively push it enthusiastically, but I'd passively accept its existence. > Most importantly, this should help discoverability quite a lot by now > also having a proper synopsis in both the manpage, but also in `git > config -h`. > > Of course, backwards compatibility is a big concern. We don't want to > just switch over to the new syntax and break all existing scripts and > muscle memory. This patch series thus abuses the fact that the implicit > modes (`git config foo.bar`, `git config foo.bar value` and `git config > foo.bar value value-pattern`) all require a key as first argument. As > keys _must_ have a dot, this allows us to unambiguously discern those > from actual subcommands. Clever ;-).
Wow, this is great. I’ve always found git-config a bit hard to use. Thanks for this.
On Wed, Mar 06, 2024 at 09:06:08AM -0800, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: > > > - `git config foo.bar` -> `git config get foo.bar` > > > > - `git config foo.bar value` -> `git config set foo.bar value` > > I actually have been perfectly OK with the above two, but I agree > that ... Same here, though I think that we are probably both biased by many years of familiarity with the existing syntax. > > - `git config foo.bar value value-pattern` -> `git config set-all > > foo.bar value value-pattern` > > ... this was less than discoverable, and would be a good update. > This one ... Agreed. > > - `git config --get-urlmatch` -> `git config get-urlmatch`. > > ... is a Meh to me, personally. I'd not actively push it > enthusiastically, but I'd passively accept its existence. I don't have strong feelings about this, but I wonder if `--urlmatch` (or `--url-match`) might be an argument to the "get" mode of this sub-command instead. Something like `git config get --urlmatch` feels much more natural to me than `git config get-urlmatch`. Thanks, Taylor
Taylor Blau <me@ttaylorr.com> writes: > I don't have strong feelings about this, but I wonder if `--urlmatch` > (or `--url-match`) might be an argument to the "get" mode of this > sub-command instead. Something like `git config get --urlmatch` feels > much more natural to me than `git config get-urlmatch`. I like that, too. "--get-regexp" may also be a good candidate to be folded into the base verb "get", with "--regexp" option to tweak what kind of key is used. Could "--get-color" and "--get-colorbool" become verb "get" with "--type=color"? The other parameters they get are somewhat different from "get", so that may not work very well, I guess.
On Wed, Mar 06, 2024 at 03:52:23PM -0800, Junio C Hamano wrote: > Taylor Blau <me@ttaylorr.com> writes: > > > I don't have strong feelings about this, but I wonder if `--urlmatch` > > (or `--url-match`) might be an argument to the "get" mode of this > > sub-command instead. Something like `git config get --urlmatch` feels > > much more natural to me than `git config get-urlmatch`. > > I like that, too. > > "--get-regexp" may also be a good candidate to be folded into the > base verb "get", with "--regexp" option to tweak what kind of key is > used. Yes, definitely. > Could "--get-color" and "--get-colorbool" become verb "get" with > "--type=color"? The other parameters they get are somewhat > different from "get", so that may not work very well, I guess. I'm somewhat embarrassed to not have a better suggestion, since I vaguely remember working on the `--type` option as one of my first contributions to the project. Indeed: fb0dc3bac13 (builtin/config.c: support `--type=<type>` as preferred alias for `--<type>`, 2018-04-18). ;-). Thanks, Taylor
Hello all, On 2024-03-07 00:46, Taylor Blau wrote: > On Wed, Mar 06, 2024 at 09:06:08AM -0800, Junio C Hamano wrote: >> Patrick Steinhardt <ps@pks.im> writes: >> >> > - `git config foo.bar` -> `git config get foo.bar` >> > >> > - `git config foo.bar value` -> `git config set foo.bar value` >> >> I actually have been perfectly OK with the above two, but I agree >> that ... > > Same here, though I think that we are probably both biased by many > years > of familiarity with the existing syntax. Ditto. Though, having "get" and "set" commands will be nice, making it all more self descriptive. >> > - `git config foo.bar value value-pattern` -> `git config set-all >> > foo.bar value value-pattern` >> >> ... this was less than discoverable, and would be a good update. >> This one ... > > Agreed. Also agreed. Having the point below in mind, perhaps we could actually end up with "set --all" instead of "set-all". >> > - `git config --get-urlmatch` -> `git config get-urlmatch`. >> >> ... is a Meh to me, personally. I'd not actively push it >> enthusiastically, but I'd passively accept its existence. > > I don't have strong feelings about this, but I wonder if `--urlmatch` > (or `--url-match`) might be an argument to the "get" mode of this > sub-command instead. Something like `git config get --urlmatch` feels > much more natural to me than `git config get-urlmatch`. Good point. I'd vote for having "get --urlmatch" or "get --url-match", because it feels more natural to me, it doesn't "clog up" the command space, and such an approach, in general, allows git-config(1) to be expanded later easier with more new arguments for the existing commands.
On Wed, Mar 06, 2024 at 06:46:33PM -0500, Taylor Blau wrote: > On Wed, Mar 06, 2024 at 09:06:08AM -0800, Junio C Hamano wrote: > > Patrick Steinhardt <ps@pks.im> writes: > > > - `git config --get-urlmatch` -> `git config get-urlmatch`. > > > > ... is a Meh to me, personally. I'd not actively push it > > enthusiastically, but I'd passively accept its existence. > > I don't have strong feelings about this, but I wonder if `--urlmatch` > (or `--url-match`) might be an argument to the "get" mode of this > sub-command instead. Something like `git config get --urlmatch` feels > much more natural to me than `git config get-urlmatch`. Agreed. I allude to this somewhere in the patch series that I only see this as a first incremental step, and that some of the subcommands really should be converted to become options eventually. Specifically: - `git config get-urlmatch` -> `git config get --urlmatch` - `git config get-regexp` -> `git config get --regexp` - `git config get-all` -> `git config get --all` - `git config set-all` -> `git config set --all` - `git config unset-all` -> `git config unset --all` I didn't yet do it as part of this patch series because I didn't want to make functional changes at the same time (well, except of course for the introduction of subcommands). But if we all agree that this patch series is something we want _and_ that we don't want to have an intermediate step with the above somewhat weird subcommands then I would be happy to pile onto the series. I wouldn't think that keeping this intermediate step would be too bad though. While we would eventually omit the above subcommands, the infra to keep them around needs to stay anyway to support old syntax like `--get-all`. Thus, we can keep the subcommands themselves almost for free even if we eventually decide to hide them and replace them with flags. Patrick
Patrick Steinhardt <ps@pks.im> writes: > On Wed, Mar 06, 2024 at 06:46:33PM -0500, Taylor Blau wrote: >> On Wed, Mar 06, 2024 at 09:06:08AM -0800, Junio C Hamano wrote: >> > Patrick Steinhardt <ps@pks.im> writes: >> > > - `git config --get-urlmatch` -> `git config get-urlmatch`. >> > >> > ... is a Meh to me, personally. I'd not actively push it >> > enthusiastically, but I'd passively accept its existence. >> >> I don't have strong feelings about this, but I wonder if `--urlmatch` >> (or `--url-match`) might be an argument to the "get" mode of this >> sub-command instead. Something like `git config get --urlmatch` feels >> much more natural to me than `git config get-urlmatch`. > > Agreed. I allude to this somewhere in the patch series that I only see > this as a first incremental step, and that some of the subcommands > really should be converted to become options eventually. Specifically: > > - `git config get-urlmatch` -> `git config get --urlmatch` > > - `git config get-regexp` -> `git config get --regexp` > > - `git config get-all` -> `git config get --all` > > - `git config set-all` -> `git config set --all` > > - `git config unset-all` -> `git config unset --all` > > I didn't yet do it as part of this patch series because I didn't want to > make functional changes at the same time (well, except of course for the > introduction of subcommands). But if we all agree that this patch series > is something we want _and_ that we don't want to have an intermediate > step with the above somewhat weird subcommands then I would be happy to > pile onto the series. > > I wouldn't think that keeping this intermediate step would be too bad > though. While we would eventually omit the above subcommands, the infra > to keep them around needs to stay anyway to support old syntax like > `--get-all`. Thus, we can keep the subcommands themselves almost for > free even if we eventually decide to hide them and replace them with > flags. The "more involved" changes along the lines Taylor suggested take some thought in designing the end-user experience, so even if you do not initially plan to support them in the implementation, it would be better to know how the final command line should look like before starting, so that we won't surprise users, if anything. The trickies I anticipate offhand are: * The urlmatch thing wants name and URL. In the new syntax, I think the URL part should become an option parameter to the --url option. E.g. a request with the current UI $ git config --get-urlmatch SECTION.VARIABLE URL would become $ git config get --url=URL SECTION.VARIABLE * The color thing takes the default value to be used when unconfigured. That should also become an option parameter to the --default option. $ git config --get-color SECTION.VARIABLE DEFAULT would become $ git config get --type=color --default=DEFAULT SECTION.VARIABLE Similarly for --tty=true/false used in --get-colorbool. * Now, these extended ones with options introduce possibilities to combine them in ways that weren't possible before. For example, "--get" and "--get-all" are distinct, and there is no way to tell "--get-urlmatch" to give all values that would match, but it is plausible that we may want to handle $ git config get --all --url=URL SECTION.VARIABLE to do so. A more interesting one is the --default=DEFAULT option. What we currently write $ git config --get no.such.variable || echo 'default value' can become $ git config get --default='default value' no.such.variable HTH.
Hi, the UI of git-config(1) is quite arcane and does not really conform to the more modern UIs that we have nowadays in Git: - While it does have modes, those modes come in the form of switches. E.g. you have to say git config --get-all to execute the "get-all" mode. - Its interface depends on the number of args. Given one arg it will print the value of the corresponding config key, given two args it will set that key. Did you know you can even give it three args? I didn't. Now guess what this mode does. This patch series overhauls git-config(1) by introducing subcommands. This results in the following UI that matches more closely what we have in other Git commands which are more modern: - `git config foo.bar` -> `git config get foo.bar` - `git config foo.bar value` -> `git config set foo.bar value` - `git config foo.bar value value-pattern` -> `git config set-all foo.bar value value-pattern` - `git config --get-urlmatch` -> `git config get-urlmatch`. Most importantly, this should help discoverability quite a lot by now also having a proper synopsis in both the manpage, but also in `git config -h`. Of course, backwards compatibility is a big concern. We don't want to just switch over to the new syntax and break all existing scripts and muscle memory. This patch series thus abuses the fact that the implicit modes (`git config foo.bar`, `git config foo.bar value` and `git config foo.bar value value-pattern`) all require a key as first argument. As keys _must_ have a dot, this allows us to unambiguously discern those from actual subcommands. Thus, git-config(1) now supports both old and new style arguments in a completely backwards compatible way, which is also demonstrated by the adapted tests. Eventually, I think we should consider dropping the old style syntax with e.g. Git v3.0. We could of course iterate further from here and keep on improving the UI of the new subcommands, e.g. by merging closely related subcommands. But for the time being I think it's easier to stop at this point and revisit the result at a later point in time. Also, note that I see this as kind of an experiment for how to modernize our UI slowly over time so that things become more consistent and thus hopefully easier to use in the long term. Patrick Patrick Steinhardt (8): builtin/config: move option array around builtin/config: move "fixed-value" option to correct group builtin/config: use `OPT_CMDMODE()` to specify modes builtin/config: move modes into separate functions builtin/config: track subcommands by action builtin/config: introduce subcommands t1300: exercise both old- and new-style modes Documentation/git-config: update to new-style syntax Documentation/git-config.txt | 204 +++++----- builtin/config.c | 671 +++++++++++++++++++------------- t/t0450/txt-help-mismatches | 1 - t/t1300-config.sh | 734 ++++++++++++++++++----------------- 4 files changed, 900 insertions(+), 710 deletions(-)