| Message ID | cover.1715595550.git.ps@pks.im (mailing list archive) |
|---|---|
| Headers | show |
| Series | builtin/config: remove global state | expand |
On Mon, May 13, 2024 at 3:22 AM Patrick Steinhardt <ps@pks.im> wrote: > > Hi, > > this is the second version of my patch series that removes global state > from "builtin/config.c". Changes compared to v1: > > - Reinstated a comment in patch 5. > > - Fixed a memory leak in patch 9. > > - A couple of commit message fixes. > > The series continues to build on top of ps/config-subcommands. > > Thanks! > > Patrick > > Patrick Steinhardt (21): > builtin/config: stop printing full usage on misuse > builtin/config: move legacy mode into its own function > builtin/config: move subcommand options into `cmd_config()` > builtin/config: move legacy options into `cmd_config()` > builtin/config: move actions into `cmd_config_actions()` > builtin/config: check for writeability after source is set up > config: make the config source const > builtin/config: refactor functions to have common exit paths > builtin/config: move location options into local variables > builtin/config: move display options into local variables > builtin/config: move type options into display options > builtin/config: move default value into display options > builtin/config: move `respect_includes_opt` into location options > builtin/config: convert `do_not_match` to a local variable > builtin/config: convert `value_pattern` to a local variable > builtin/config: convert `regexp` to a local variable > builtin/config: convert `key_regexp` to a local variable > builtin/config: convert `key` to a local variable > builtin/config: track "fixed value" option via flags only > builtin/config: convert flags to a local variable > builtin/config: pass data between callbacks via local variables > > builtin/config.c | 968 ++++++++++++++++++++++++++-------------------- > config.c | 4 +- > config.h | 2 +- > t/t1300-config.sh | 9 +- > 4 files changed, 550 insertions(+), 433 deletions(-) > > Range-diff against v1: Looks good to me > 1: 0ba7628126 = 1: 0ba7628126 builtin/config: stop printing full usage on misuse > 2: 663e1f74f8 = 2: 663e1f74f8 builtin/config: move legacy mode into its own function > 3: 1239c151d0 = 3: 1239c151d0 builtin/config: move subcommand options into `cmd_config()` > 4: 82964510c5 = 4: 82964510c5 builtin/config: move legacy options into `cmd_config()` > 5: 2e308393ed ! 5: 0a6ecae2cc builtin/config: move actions into `cmd_config_actions()` > @@ builtin/config.c: static int cmd_config_actions(int argc, const char **argv, con > comment = git_config_prepare_comment_string(comment_arg); > > - if (actions & PAGING_ACTIONS) > ++ /* > ++ * The following actions may produce more than one line of output and > ++ * should therefore be paged. > ++ */ > + if (actions & (ACTION_LIST | ACTION_GET_ALL | ACTION_GET_REGEXP | ACTION_GET_URLMATCH)) > setup_auto_pager("config", 1); > > 6: edfd7caa39 = 6: 7ab99a27c1 builtin/config: check for writeability after source is set up > 7: bfba68aa1e = 7: 1460d3a36c config: make the config source const > 8: 6bff0410e9 = 8: 018ed0226b builtin/config: refactor functions to have common exit paths > 9: a96c122280 ! 9: b5d43b6f85 builtin/config: move location options into local variables > @@ builtin/config.c: static char *default_user_config(void) > - given_config_source.file = git_global_config(); > - if (!given_config_source.file) > + if (opts->use_global_config) { > -+ opts->source.file = xstrdup_or_null(git_global_config()); > ++ opts->source.file = git_global_config(); > + if (!opts->source.file) > /* > * It is unknown if HOME/.gitconfig exists, so > 10: 06c1e08fc4 = 10: d66e14af30 builtin/config: move display options into local variables > 11: 9610897662 = 11: 63436c3416 builtin/config: move type options into display options > 12: eb79462861 = 12: 106b8ac8a2 builtin/config: move default value into display options > 13: b9ebfbd667 = 13: 8a6b555b58 builtin/config: move `respect_includes_opt` into location options > 14: 2b40b784fe ! 14: 0dd22bf51a builtin/config: convert `do_not_match` to a local variable > @@ Commit message > builtin/config: convert `do_not_match` to a local variable > > The `do_not_match` variable is used by the `format_config()` callback as > - an indicator whteher or not the passed regular expression is negated. It > + an indicator whether or not the passed regular expression is negated. It > is only ever set up by its only caller, `collect_config()` and can thus > easily be moved into the `collect_config_data` structure. > > 15: 71d1b7a51b = 15: b656951f0c builtin/config: convert `value_pattern` to a local variable > 16: c3b340f119 = 16: b56a07bda0 builtin/config: convert `regexp` to a local variable > 17: 835cc0acfb = 17: 323cb05120 builtin/config: convert `key_regexp` to a local variable > 18: 2aee7ec5d8 ! 18: e972e63be8 builtin/config: convert `key` to a local variable > @@ Commit message > The `key` variable is used by the `get_value()` function for two > purposes: > > - - It is used to store the result of `git_config_parse_key()`, which si > + - It is used to store the result of `git_config_parse_key()`, which is > then passed on to `collect_config()`. > > - It is used as a store to convert the provided key to an > all-lowercase key when `use_key_regexp` is set. > > - Both of these cases don't warrant a global variable at all. In the > - former case we can pass the key via `struct collect_config_data`. And in > - the latter case we really only want to have it as a temporary local > - variable such that we can free associated memory. > + Neither of these cases warrant a global variable at all. In the former > + case we can pass the key via `struct collect_config_data`. And in the > + latter case we really only want to have it as a temporary local variable > + such that we can free associated memory. > > Refactor the code accordingly to reduce our reliance on global state. > > 19: 625216a774 ! 19: d83c3d085e builtin/config: track "fixed value" option via flags only > @@ Commit message > is not aware that this is tracked via two separate mechanisms. > > Refactor the code to use the flag exclusively. We already pass it to all > - the require callsites anyway, except for `collect_config()`. > + the required callsites anyway, except for `collect_config()`. > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > > 20: 05254d512b = 20: 294bcd96a4 builtin/config: convert flags to a local variable > 21: 3a5f059789 = 21: 0496b958e2 builtin/config: pass data between callbacks via local variables > -- > 2.45.GIT >
Patrick Steinhardt <ps@pks.im> writes: > this is the second version of my patch series that removes global state > from "builtin/config.c". Changes compared to v1: > > - Reinstated a comment in patch 5. > > - Fixed a memory leak in patch 9. > > - A couple of commit message fixes. > > The series continues to build on top of ps/config-subcommands. I do not offhand know if this iteration has already been seen by me, but a few recent CI runs of 'seen' did break with *-leaks jobs in t13XX series around "config". Hopefully with the fix in "patch 9" listed above the problem has gone away? We'll know soon enough when I push out the integration result. Thanks.
On Tue, May 14, 2024 at 07:48:47AM -0700, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: > > > this is the second version of my patch series that removes global state > > from "builtin/config.c". Changes compared to v1: > > > > - Reinstated a comment in patch 5. > > > > - Fixed a memory leak in patch 9. > > > > - A couple of commit message fixes. > > > > The series continues to build on top of ps/config-subcommands. > > I do not offhand know if this iteration has already been seen by me, > but a few recent CI runs of 'seen' did break with *-leaks jobs in > t13XX series around "config". Hopefully with the fix in "patch 9" > listed above the problem has gone away? We'll know soon enough when > I push out the integration result. I think so, yes. v1 also broke pipelines at GitLab -- I didn't notice though because pipelines had already been broken due to the Python 2 deprecation in Ubuntu 24.04, so I missed that there were in fact multiple issues. The GitLab pipeline now passes with v2, so I assume that it would also pass in GitHub now. Patrick
On Tue, May 14, 2024 at 04:52:35PM +0200, Patrick Steinhardt wrote: > On Tue, May 14, 2024 at 07:48:47AM -0700, Junio C Hamano wrote: > > Patrick Steinhardt <ps@pks.im> writes: > > > > > this is the second version of my patch series that removes global state > > > from "builtin/config.c". Changes compared to v1: > > > > > > - Reinstated a comment in patch 5. > > > > > > - Fixed a memory leak in patch 9. > > > > > > - A couple of commit message fixes. > > > > > > The series continues to build on top of ps/config-subcommands. > > > > I do not offhand know if this iteration has already been seen by me, > > but a few recent CI runs of 'seen' did break with *-leaks jobs in > > t13XX series around "config". Hopefully with the fix in "patch 9" > > listed above the problem has gone away? We'll know soon enough when > > I push out the integration result. > > I think so, yes. v1 also broke pipelines at GitLab -- I didn't notice > though because pipelines had already been broken due to the Python 2 > deprecation in Ubuntu 24.04, so I missed that there were in fact > multiple issues. The GitLab pipeline now passes with v2, so I assume > that it would also pass in GitHub now. > > Patrick Oh, there is a second memory leak that I missed. *sigh* I'll do a better job at that next time and send a v3 that fixes it. Patrick
Hi, this is the second version of my patch series that removes global state from "builtin/config.c". Changes compared to v1: - Reinstated a comment in patch 5. - Fixed a memory leak in patch 9. - A couple of commit message fixes. The series continues to build on top of ps/config-subcommands. Thanks! Patrick Patrick Steinhardt (21): builtin/config: stop printing full usage on misuse builtin/config: move legacy mode into its own function builtin/config: move subcommand options into `cmd_config()` builtin/config: move legacy options into `cmd_config()` builtin/config: move actions into `cmd_config_actions()` builtin/config: check for writeability after source is set up config: make the config source const builtin/config: refactor functions to have common exit paths builtin/config: move location options into local variables builtin/config: move display options into local variables builtin/config: move type options into display options builtin/config: move default value into display options builtin/config: move `respect_includes_opt` into location options builtin/config: convert `do_not_match` to a local variable builtin/config: convert `value_pattern` to a local variable builtin/config: convert `regexp` to a local variable builtin/config: convert `key_regexp` to a local variable builtin/config: convert `key` to a local variable builtin/config: track "fixed value" option via flags only builtin/config: convert flags to a local variable builtin/config: pass data between callbacks via local variables builtin/config.c | 968 ++++++++++++++++++++++++++-------------------- config.c | 4 +- config.h | 2 +- t/t1300-config.sh | 9 +- 4 files changed, 550 insertions(+), 433 deletions(-) Range-diff against v1: 1: 0ba7628126 = 1: 0ba7628126 builtin/config: stop printing full usage on misuse 2: 663e1f74f8 = 2: 663e1f74f8 builtin/config: move legacy mode into its own function 3: 1239c151d0 = 3: 1239c151d0 builtin/config: move subcommand options into `cmd_config()` 4: 82964510c5 = 4: 82964510c5 builtin/config: move legacy options into `cmd_config()` 5: 2e308393ed ! 5: 0a6ecae2cc builtin/config: move actions into `cmd_config_actions()` @@ builtin/config.c: static int cmd_config_actions(int argc, const char **argv, con comment = git_config_prepare_comment_string(comment_arg); - if (actions & PAGING_ACTIONS) ++ /* ++ * The following actions may produce more than one line of output and ++ * should therefore be paged. ++ */ + if (actions & (ACTION_LIST | ACTION_GET_ALL | ACTION_GET_REGEXP | ACTION_GET_URLMATCH)) setup_auto_pager("config", 1); 6: edfd7caa39 = 6: 7ab99a27c1 builtin/config: check for writeability after source is set up 7: bfba68aa1e = 7: 1460d3a36c config: make the config source const 8: 6bff0410e9 = 8: 018ed0226b builtin/config: refactor functions to have common exit paths 9: a96c122280 ! 9: b5d43b6f85 builtin/config: move location options into local variables @@ builtin/config.c: static char *default_user_config(void) - given_config_source.file = git_global_config(); - if (!given_config_source.file) + if (opts->use_global_config) { -+ opts->source.file = xstrdup_or_null(git_global_config()); ++ opts->source.file = git_global_config(); + if (!opts->source.file) /* * It is unknown if HOME/.gitconfig exists, so 10: 06c1e08fc4 = 10: d66e14af30 builtin/config: move display options into local variables 11: 9610897662 = 11: 63436c3416 builtin/config: move type options into display options 12: eb79462861 = 12: 106b8ac8a2 builtin/config: move default value into display options 13: b9ebfbd667 = 13: 8a6b555b58 builtin/config: move `respect_includes_opt` into location options 14: 2b40b784fe ! 14: 0dd22bf51a builtin/config: convert `do_not_match` to a local variable @@ Commit message builtin/config: convert `do_not_match` to a local variable The `do_not_match` variable is used by the `format_config()` callback as - an indicator whteher or not the passed regular expression is negated. It + an indicator whether or not the passed regular expression is negated. It is only ever set up by its only caller, `collect_config()` and can thus easily be moved into the `collect_config_data` structure. 15: 71d1b7a51b = 15: b656951f0c builtin/config: convert `value_pattern` to a local variable 16: c3b340f119 = 16: b56a07bda0 builtin/config: convert `regexp` to a local variable 17: 835cc0acfb = 17: 323cb05120 builtin/config: convert `key_regexp` to a local variable 18: 2aee7ec5d8 ! 18: e972e63be8 builtin/config: convert `key` to a local variable @@ Commit message The `key` variable is used by the `get_value()` function for two purposes: - - It is used to store the result of `git_config_parse_key()`, which si + - It is used to store the result of `git_config_parse_key()`, which is then passed on to `collect_config()`. - It is used as a store to convert the provided key to an all-lowercase key when `use_key_regexp` is set. - Both of these cases don't warrant a global variable at all. In the - former case we can pass the key via `struct collect_config_data`. And in - the latter case we really only want to have it as a temporary local - variable such that we can free associated memory. + Neither of these cases warrant a global variable at all. In the former + case we can pass the key via `struct collect_config_data`. And in the + latter case we really only want to have it as a temporary local variable + such that we can free associated memory. Refactor the code accordingly to reduce our reliance on global state. 19: 625216a774 ! 19: d83c3d085e builtin/config: track "fixed value" option via flags only @@ Commit message is not aware that this is tracked via two separate mechanisms. Refactor the code to use the flag exclusively. We already pass it to all - the require callsites anyway, except for `collect_config()`. + the required callsites anyway, except for `collect_config()`. Signed-off-by: Patrick Steinhardt <ps@pks.im> 20: 05254d512b = 20: 294bcd96a4 builtin/config: convert flags to a local variable 21: 3a5f059789 = 21: 0496b958e2 builtin/config: pass data between callbacks via local variables