Re: [PATCH v2 3/7] config: convert multi_replace to flags
- From
Emily Shaffer <emilyshaffer@google.com>
- Date
- Nov 23, 2020, 21:43 UTC
- Message-ID
- <20201123214322.GC499823@google.com>
- In-Reply-To
- <0c152faa00881483db0a59f4c5bc7204ebed8966.1606147507.git.gitgitgadget@gmail.com>
On Mon, Nov 23, 2020 at 04:05:03PM +0000, Derrick Stolee via GitGitGadget wrote:
Show 28 quoted lines
>
>
> We will extend the flexibility of the config API. Before doing so, let's
> take an existing 'int multi_replace' parameter and replace it with a new
> 'unsigned flags' parameter that can take multiple options as a bit field.
>
> Update all callers that specified multi_replace to now specify the
> CONFIG_FLAGS_MULTI_REPLACE flag. To add more clarity, extend the
> documentation of git_config_set_multivar_in_file() including a clear
> labeling of its arguments. Other config API methods in config.h require
> only a change of the final parameter from 'int' to 'unsigned'.
>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
> diff --git a/builtin/branch.c b/builtin/branch.c
> index e82301fb1b..5ce3844d22 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -829,10 +829,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
> die(_("Branch '%s' has no upstream information"), branch->name);
>
> strbuf_addf(&buf, "branch.%s.remote", branch->name);
> - git_config_set_multivar(buf.buf, NULL, NULL, 1);
> + git_config_set_multivar(buf.buf, NULL, NULL, CONFIG_FLAGS_MULTI_REPLACE);
> strbuf_reset(&buf);
> strbuf_addf(&buf, "branch.%s.merge", branch->name);
> - git_config_set_multivar(buf.buf, NULL, NULL, 1);
> + git_config_set_multivar(buf.buf, NULL, NULL, CONFIG_FLAGS_MULTI_REPLACE);Converting callers. Straightforward.
[snipping more similar work]
Show 14 quoted lines
> diff --git a/config.c b/config.c > index 2b79fe76ad..096f2eae0d 100644 > --- a/config.c > +++ b/config.c > @@ -2716,9 +2716,9 @@ void git_config_set(const char *key, const char *value) > * if value_regex!=NULL, disregard key/value pairs where value does not match. > * if value_regex==CONFIG_REGEX_NONE, do not match any existing values > * (only add a new one) > - * if multi_replace==0, nothing, or only one matching key/value is replaced, > - * else all matching key/values (regardless how many) are removed, > - * before the new pair is written. > + * if (flags & CONFIG_FLAGS_MULTI_REPLACE) == 0, at most one matching > + * key/value is replaced, else all matching key/values (regardless > + * how many) are removed, before the new pair is written.
This documentation to me sounds like the question you asked on-list the other day: "does replace-all turn many configs into one, or many configs into many with the same value?" Is it reflected in user-facing documentation? Looks like no - you might have a good opportunity here to make that more clear.
Show 9 quoted lines
> * > * Returns 0 on success. > * > @@ -2739,7 +2739,7 @@ void git_config_set(const char *key, const char *value) > int git_config_set_multivar_in_file_gently(const char *config_filename, > const char *key, const char *value, > const char *value_regex, > - int multi_replace) > + unsigned flags)
Well, I wanted to complain about using 'unsigned' instead of 'unsigned int', but 'git grep -P "unsigned(?! int)"' tells me that it's not a thing anybody else seems to mind. So I'll just grumble in my corner instead :)
Show 18 quoted lines
> {
> int fd = -1, in_fd = -1;
> int ret;
> @@ -2756,7 +2756,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
> if (ret)
> goto out_free;
>
> - store.multi_replace = multi_replace;
> + store.multi_replace = (flags & CONFIG_FLAGS_MULTI_REPLACE) != 0;
>
> if (!config_filename)
> config_filename = filename_buf = git_pathdup("config");
> @@ -2845,7 +2845,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
>
> /* if nothing to unset, or too many matches, error out */
> if ((store.seen_nr == 0 && value == NULL) ||
> - (store.seen_nr > 1 && multi_replace == 0)) {
> + (store.seen_nr > 1 && !store.multi_replace)) {Huh, I wonder why 'store.multi_replace' wasn't used here before, since it was bothered to be set at earlier. Ah well.
> void git_config_set_multivar_in_file(const char *config_filename, > const char *key, const char *value, > - const char *value_regex, int multi_replace) > + const char *value_regex, unsigned flags)
And some more signature conversions. [snip]
Show 19 quoted lines
> /** > * takes four parameters: > @@ -276,13 +289,15 @@ int git_config_set_multivar_in_file_gently(const char *, const char *, const cha > * - the value regex, as a string. It will disregard key/value pairs where value > * does not match. > * > - * - a multi_replace value, as an int. If value is equal to zero, nothing or only > - * one matching key/value is replaced, else all matching key/values (regardless > - * how many) are removed, before the new pair is written. > + * - a flags value with bits corresponding to the CONFIG_FLAG_* macros. > * > * It returns 0 on success. > */ > -void git_config_set_multivar_in_file(const char *, const char *, const char *, const char *, int); > +void git_config_set_multivar_in_file(const char *config_filename, > + const char *key, > + const char *value, > + const char *value_regex, > + unsigned flags);
Nice opportunity to make the header a little easier to read here. Thanks.
With just one optional comment,
Reviewed-by: Emily Shaffer <emilyshaffer@google.com>