| Message ID | cover.1714982328.git.ps@pks.im (mailing list archive) |
|---|---|
| Headers | show |
| Series | builtin/config: introduce subcommands | expand |
Patrick Steinhardt <ps@pks.im> writes: > Hi, > > this is the fifth and hopefully last version of my patch sthat > introduces subcommands into git-config(1). > > The only changes compared to v4 are some fixes to commit messages. > Otherwise I'm not aware of any other feedback that would need to be > addressed. > > Patrick > > Patrick Steinhardt (14): > config: clarify memory ownership when preparing comment strings > 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: pull out function to handle config location > builtin/config: pull out function to handle `--null` > builtin/config: introduce "list" subcommand > builtin/config: introduce "get" subcommand > builtin/config: introduce "set" subcommand > builtin/config: introduce "unset" subcommand > builtin/config: introduce "rename-section" subcommand > builtin/config: introduce "remove-section" subcommand > builtin/config: introduce "edit" subcommand > builtin/config: display subcommand help > > Documentation/git-config.txt | 219 ++++++++------- > builtin/config.c | 512 ++++++++++++++++++++++++++++------- > config.c | 16 +- > config.h | 2 +- > t/t0450/txt-help-mismatches | 1 - > t/t1300-config.sh | 432 +++++++++++++++++------------ > 6 files changed, 812 insertions(+), 370 deletions(-) > > Range-diff against v4: > 1: 3aa26d5bff ! 1: 881d2b5426 config: clarify memory ownership when preparing comment strings > @@ Commit message > not like this micro-optimization really matters. Thus, callers are now > always responsible for freeing the value. > > + Signed-off-by: Patrick Steinhardt <ps@pks.im> > + > ## builtin/config.c ## > @@ builtin/config.c: static struct config_options config_options; > static int show_origin; > 2: 8f0804ab48 = 2: 66dffaa8f2 builtin/config: move option array around > 3: ddcd8031d7 ! 3: 36abda0e02 builtin/config: move "fixed-value" option to correct group > @@ Commit message > builtin/config: move "fixed-value" option to correct group > > The `--fixed-value` option can be used to alter how the value-pattern > - parameter is interpreted for the various submodes of git-config(1). But > - while it is an option, it is currently listed as part of the submodes > - group the command, which is wrong. > + parameter is interpreted for the various actions of git-config(1). But > + while it is an option, it is currently listed as part of the actions > + group, which is wrong. > > Move the option to the "Other" group, which hosts the various options > known to git-config(1). > 4: 1bc3918840 = 4: 34b66f9c87 builtin/config: use `OPT_CMDMODE()` to specify modes > 5: 3754812309 = 5: 4f90f206e7 builtin/config: pull out function to handle config location > 6: cb1714c493 = 6: df1a6f14e6 builtin/config: pull out function to handle `--null` > 7: b3f3c3ba6a ! 7: 1df76a9970 builtin/config: introduce "list" subcommand > @@ Commit message > builtin/config: introduce "list" subcommand > > While git-config(1) has several modes, those modes are not exposed with > - subcommands but instead by specifying e.g. `--unset` or `--list`. This > - user interface is not really in line with how our more modern commands > - work, where it is a lot more customary to say e.g. `git remote list`. > - Furthermore, to add to the confusion, git-config(1) also allows the user > - to request modes implicitly by just specifying the correct number of > - arguments. Thus, `git config foo.bar` will retrieve the value of > - "foo.bar" while `git config foo.bar baz` will set it to "baz". > + subcommands but instead by specifying action flags like `--unset` or > + `--list`. This user interface is not really in line with how our more > + modern commands work, where it is a lot more customary to say e.g. `git > + remote list`. Furthermore, to add to the confusion, git-config(1) also > + allows the user to request modes implicitly by just specifying the > + correct number of arguments. Thus, `git config foo.bar` will retrieve > + the value of "foo.bar" while `git config foo.bar baz` will set it to > + "baz". > > Overall, this makes for a confusing interface that could really use a > makeover. It hurts discoverability of what you can do with git-config(1) > 8: 0e6da909ac = 8: 29676b81e0 builtin/config: introduce "get" subcommand > 9: 8a623a31b9 = 9: 94afb5a5b7 builtin/config: introduce "set" subcommand > 10: e25e5b69cd = 10: e525c2326a builtin/config: introduce "unset" subcommand > 11: f24008d356 = 11: a797889890 builtin/config: introduce "rename-section" subcommand > 12: fc2ddd3201 = 12: 8ec214755e builtin/config: introduce "remove-section" subcommand > 13: 4c2d817eff = 13: 1893c23afc builtin/config: introduce "edit" subcommand > 14: 4c351b12b8 = 14: 97a48ab81d builtin/config: display subcommand help > > base-commit: d4cc1ec35f3bcce816b69986ca41943f6ce21377 > -- > 2.45.0 The range diff looks good. Overall I have nothing more to add to this series. Thanks Karthik
On Mon, May 06, 2024 at 10:55:51AM +0200, Patrick Steinhardt wrote: > Hi, > > this is the fifth and hopefully last version of my patch sthat > introduces subcommands into git-config(1). > > The only changes compared to v4 are some fixes to commit messages. > Otherwise I'm not aware of any other feedback that would need to be > addressed. Thanks for the new round. I took a close look through the series, and couldn't see any remaining issues. I appreciate the translation guide you included in the documentation to indicate "git config --add" is replaced by things like "git config set --append". Thanks, Taylor
On Monday, May 6, 2024 4:22 PM, Taylor Blau wrote: >On Mon, May 06, 2024 at 10:55:51AM +0200, Patrick Steinhardt wrote: >> Hi, >> >> this is the fifth and hopefully last version of my patch sthat >> introduces subcommands into git-config(1). >> >> The only changes compared to v4 are some fixes to commit messages. >> Otherwise I'm not aware of any other feedback that would need to be >> addressed. > >Thanks for the new round. I took a close look through the series, and couldn't see >any remaining issues. I appreciate the translation guide you included in the >documentation to indicate "git config --add" is replaced by things like "git config set >--append". Please make sure that there is a compatibility mode available for some large period. There are huge numbers of scripts in the customer base I deal with that use config extensively. Changing the CLI for this will be dire consequences.
On Mon, May 06, 2024 at 04:38:59PM -0400, rsbecker@nexbridge.com wrote: > On Monday, May 6, 2024 4:22 PM, Taylor Blau wrote: > >On Mon, May 06, 2024 at 10:55:51AM +0200, Patrick Steinhardt wrote: > >> Hi, > >> > >> this is the fifth and hopefully last version of my patch sthat > >> introduces subcommands into git-config(1). > >> > >> The only changes compared to v4 are some fixes to commit messages. > >> Otherwise I'm not aware of any other feedback that would need to be > >> addressed. > > > >Thanks for the new round. I took a close look through the series, and couldn't see > >any remaining issues. I appreciate the translation guide you included in the > >documentation to indicate "git config --add" is replaced by things like "git config set > >--append". > > Please make sure that there is a compatibility mode available for some > large period. There are huge numbers of scripts in the customer base I > deal with that use config extensively. Changing the CLI for this will > be dire consequences. Yes, that's a very sensible thing to ask for. I think the bare minimum we should aim for is a year (4 releases), where two years (8 releases) feels a bit more sensible. Do you feel like that is reasonable, or do you think it would be too short? Patrick
Hi, this is the fifth and hopefully last version of my patch sthat introduces subcommands into git-config(1). The only changes compared to v4 are some fixes to commit messages. Otherwise I'm not aware of any other feedback that would need to be addressed. Patrick Patrick Steinhardt (14): config: clarify memory ownership when preparing comment strings 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: pull out function to handle config location builtin/config: pull out function to handle `--null` builtin/config: introduce "list" subcommand builtin/config: introduce "get" subcommand builtin/config: introduce "set" subcommand builtin/config: introduce "unset" subcommand builtin/config: introduce "rename-section" subcommand builtin/config: introduce "remove-section" subcommand builtin/config: introduce "edit" subcommand builtin/config: display subcommand help Documentation/git-config.txt | 219 ++++++++------- builtin/config.c | 512 ++++++++++++++++++++++++++++------- config.c | 16 +- config.h | 2 +- t/t0450/txt-help-mismatches | 1 - t/t1300-config.sh | 432 +++++++++++++++++------------ 6 files changed, 812 insertions(+), 370 deletions(-) Range-diff against v4: 1: 3aa26d5bff ! 1: 881d2b5426 config: clarify memory ownership when preparing comment strings @@ Commit message not like this micro-optimization really matters. Thus, callers are now always responsible for freeing the value. + Signed-off-by: Patrick Steinhardt <ps@pks.im> + ## builtin/config.c ## @@ builtin/config.c: static struct config_options config_options; static int show_origin; 2: 8f0804ab48 = 2: 66dffaa8f2 builtin/config: move option array around 3: ddcd8031d7 ! 3: 36abda0e02 builtin/config: move "fixed-value" option to correct group @@ Commit message builtin/config: move "fixed-value" option to correct group The `--fixed-value` option can be used to alter how the value-pattern - parameter is interpreted for the various submodes of git-config(1). But - while it is an option, it is currently listed as part of the submodes - group the command, which is wrong. + parameter is interpreted for the various actions of git-config(1). But + while it is an option, it is currently listed as part of the actions + group, which is wrong. Move the option to the "Other" group, which hosts the various options known to git-config(1). 4: 1bc3918840 = 4: 34b66f9c87 builtin/config: use `OPT_CMDMODE()` to specify modes 5: 3754812309 = 5: 4f90f206e7 builtin/config: pull out function to handle config location 6: cb1714c493 = 6: df1a6f14e6 builtin/config: pull out function to handle `--null` 7: b3f3c3ba6a ! 7: 1df76a9970 builtin/config: introduce "list" subcommand @@ Commit message builtin/config: introduce "list" subcommand While git-config(1) has several modes, those modes are not exposed with - subcommands but instead by specifying e.g. `--unset` or `--list`. This - user interface is not really in line with how our more modern commands - work, where it is a lot more customary to say e.g. `git remote list`. - Furthermore, to add to the confusion, git-config(1) also allows the user - to request modes implicitly by just specifying the correct number of - arguments. Thus, `git config foo.bar` will retrieve the value of - "foo.bar" while `git config foo.bar baz` will set it to "baz". + subcommands but instead by specifying action flags like `--unset` or + `--list`. This user interface is not really in line with how our more + modern commands work, where it is a lot more customary to say e.g. `git + remote list`. Furthermore, to add to the confusion, git-config(1) also + allows the user to request modes implicitly by just specifying the + correct number of arguments. Thus, `git config foo.bar` will retrieve + the value of "foo.bar" while `git config foo.bar baz` will set it to + "baz". Overall, this makes for a confusing interface that could really use a makeover. It hurts discoverability of what you can do with git-config(1) 8: 0e6da909ac = 8: 29676b81e0 builtin/config: introduce "get" subcommand 9: 8a623a31b9 = 9: 94afb5a5b7 builtin/config: introduce "set" subcommand 10: e25e5b69cd = 10: e525c2326a builtin/config: introduce "unset" subcommand 11: f24008d356 = 11: a797889890 builtin/config: introduce "rename-section" subcommand 12: fc2ddd3201 = 12: 8ec214755e builtin/config: introduce "remove-section" subcommand 13: 4c2d817eff = 13: 1893c23afc builtin/config: introduce "edit" subcommand 14: 4c351b12b8 = 14: 97a48ab81d builtin/config: display subcommand help base-commit: d4cc1ec35f3bcce816b69986ca41943f6ce21377