Re: [PATCH 3/7] config: convert multi_replace to flags

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 19, 2020, 22:32 UTC
Message-ID: <xmqqk0uhvuy9.fsf@gitster.c.googlers.com>
In-Reply-To: <c9caed3854cfe694f105b19657cf57f73412ad4a.1605801143.git.gitgitgadget@gmail.com>

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 5 quoted lines

> From: Derrick Stolee <dstolee@microsoft.com>
>
> 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
> 'int flags' parameter that can take multiple options as a bit field.

Nice. I was afraid we may have to _add_ a new parameter, but here, we can turn an existing bool into a flags word, which is great. But please use "unsigned" for such flags, just out of inertia/convention.

Show 11 quoted lines

> @@ -70,6 +70,13 @@ enum config_event_t {
>  	CONFIG_EVENT_ERROR
>  };
>  
> +/*
> + * When CONFIG_FLAGS_MULTI_REPLACE is specified, all matching key/values
> + * are removed before a new pair is written. If the flag is not present,
> + * then set operations replace only the first match.
> + */
> +#define CONFIG_FLAGS_MULTI_REPLACE (1 << 0)
> +

The description is clear, but how far is this comment from the actual parameter definition where "flag" is used? What I am trying to get at is if it is clear enough to the readers, when we say "when ... is specified", where they can specify it. If it is not clear, perhaps we want to start the above comment like so:

	/*
	 * These are bits in the flags word to be given to functions
	 * like git_config_set_multivar_in_file(), etc.
	 *
	 * When CONFIG_FLAGS_MULTI_REPLACE is specified, ...

Show 8 quoted lines

> @@ -276,13 +283,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.

This side is good---it tells readers what the bits in the flags word are called (i.e. they can look for CONFIG_FLAGS_ in the file).

Show 12 quoted lines

>   *
>   * 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,
> +				     int flags);
>  
>  /**
>   * rename or remove sections in the config file