git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Jonathan Nieder <jrnieder@gmail.com>,
	Emily Shaffer <emilyshaffer@google.com>,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>,
	Jeff King <peff@peff.net>,
	Derrick Stolee <derrickstolee@github.com>,
	Derrick Stolee <dstolee@microsoft.com>
Subject: Re: [PATCH 3/7] config: convert multi_replace to flags
Date: Thu, 19 Nov 2020 14:32:46 -0800
Message-ID: <xmqqk0uhvuy9.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <c9caed3854cfe694f105b19657cf57f73412ad4a.1605801143.git.gitgitgadget@gmail.com> (Derrick Stolee via GitGitGadget's message of "Thu, 19 Nov 2020 15:52:19 +0000")

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

> 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.

> @@ -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, ...


> @@ -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).

>   *
>   * 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

  reply	other threads:[~2020-11-19 22:34 UTC|newest]

Thread overview: 72+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-19 15:52 [PATCH 0/7] config: add --literal-value option Derrick Stolee via GitGitGadget
2020-11-19 15:52 ` [PATCH 1/7] t1300: test "set all" mode with value_regex Derrick Stolee via GitGitGadget
2020-11-19 22:24   ` Junio C Hamano
2020-11-20  2:09     ` brian m. carlson
2020-11-20  2:33       ` Junio C Hamano
2020-11-20 18:39     ` Jeff King
2020-11-20 22:35       ` Junio C Hamano
2020-11-21 22:27       ` brian m. carlson
2020-11-22  3:31         ` Junio C Hamano
2020-11-24  2:38           ` Jeff King
2020-11-24 19:43             ` Junio C Hamano
2020-11-19 15:52 ` [PATCH 2/7] t1300: add test for --replace-all " Derrick Stolee via GitGitGadget
2020-11-19 15:52 ` [PATCH 3/7] config: convert multi_replace to flags Derrick Stolee via GitGitGadget
2020-11-19 22:32   ` Junio C Hamano [this message]
2020-11-19 15:52 ` [PATCH 4/7] config: add --literal-value option, un-implemented Derrick Stolee via GitGitGadget
2020-11-19 22:42   ` Junio C Hamano
2020-11-20  6:35   ` Martin Ågren
2020-11-19 15:52 ` [PATCH 5/7] config: plumb --literal-value into config API Derrick Stolee via GitGitGadget
2020-11-19 22:45   ` Junio C Hamano
2020-11-19 15:52 ` [PATCH 6/7] config: implement --literal-value with --get* Derrick Stolee via GitGitGadget
2020-11-19 15:52 ` [PATCH 7/7] maintenance: use 'git config --literal-value' Derrick Stolee via GitGitGadget
2020-11-19 23:17   ` Junio C Hamano
2020-11-20 13:19 ` [PATCH 0/7] config: add --literal-value option Ævar Arnfjörð Bjarmason
2020-11-20 13:23   ` Derrick Stolee
2020-11-20 18:30     ` Junio C Hamano
2020-11-20 18:51       ` Derrick Stolee
2020-11-20 21:52         ` Junio C Hamano
2020-11-24 12:35     ` Ævar Arnfjörð Bjarmason
2020-11-23 16:05 ` [PATCH v2 0/7] config: add --fixed-value option Derrick Stolee via GitGitGadget
2020-11-23 16:05   ` [PATCH v2 1/7] t1300: test "set all" mode with value_regex Derrick Stolee via GitGitGadget
2020-11-23 19:37     ` Emily Shaffer
2020-11-23 16:05   ` [PATCH v2 2/7] t1300: add test for --replace-all " Derrick Stolee via GitGitGadget
2020-11-23 19:40     ` Emily Shaffer
2020-11-23 16:05   ` [PATCH v2 3/7] config: convert multi_replace to flags Derrick Stolee via GitGitGadget
2020-11-23 21:43     ` Emily Shaffer
2020-11-23 16:05   ` [PATCH v2 4/7] config: add --fixed-value option, un-implemented Derrick Stolee via GitGitGadget
2020-11-23 19:37     ` Junio C Hamano
2020-11-23 21:51     ` Emily Shaffer
2020-11-23 22:41       ` Junio C Hamano
2020-11-25 14:08         ` Derrick Stolee
2020-11-25 17:22           ` Derrick Stolee
2020-11-25 17:28           ` Eric Sunshine
2020-11-25 19:30             ` Junio C Hamano
2020-11-25 19:29           ` Junio C Hamano
2020-11-23 16:05   ` [PATCH v2 5/7] config: plumb --fixed-value into config API Derrick Stolee via GitGitGadget
2020-11-23 22:21     ` Emily Shaffer
2020-11-24  0:52       ` Eric Sunshine
2020-11-25 15:41       ` Derrick Stolee
2020-11-25 17:55         ` Eric Sunshine
2020-11-23 16:05   ` [PATCH v2 6/7] config: implement --fixed-value with --get* Derrick Stolee via GitGitGadget
2020-11-23 19:53     ` Junio C Hamano
2020-11-23 22:43     ` Emily Shaffer
2020-11-23 16:05   ` [PATCH v2 7/7] maintenance: use 'git config --fixed-value' Derrick Stolee via GitGitGadget
2020-11-23 21:39     ` Junio C Hamano
2020-11-23 22:48     ` Emily Shaffer
2020-11-23 23:27       ` Junio C Hamano
2020-11-23 19:33   ` [PATCH v2 0/7] config: add --fixed-value option Junio C Hamano
2020-11-25 22:12   ` [PATCH v3 0/8] " Derrick Stolee via GitGitGadget
2020-11-25 22:12     ` [PATCH v3 1/8] config: convert multi_replace to flags Derrick Stolee via GitGitGadget
2020-11-25 22:12     ` [PATCH v3 2/8] config: replace 'value_regex' with 'value_pattern' Derrick Stolee via GitGitGadget
2020-11-25 22:50       ` Eric Sunshine
2020-11-25 22:12     ` [PATCH v3 3/8] t1300: test "set all" mode with value-pattern Derrick Stolee via GitGitGadget
2020-11-25 22:12     ` [PATCH v3 4/8] t1300: add test for --replace-all " Derrick Stolee via GitGitGadget
2020-11-25 22:12     ` [PATCH v3 5/8] config: add --fixed-value option, un-implemented Derrick Stolee via GitGitGadget
2020-11-25 23:04       ` Eric Sunshine
2020-11-25 22:12     ` [PATCH v3 6/8] config: plumb --fixed-value into config API Derrick Stolee via GitGitGadget
2020-11-25 22:12     ` [PATCH v3 7/8] config: implement --fixed-value with --get* Derrick Stolee via GitGitGadget
2020-11-25 22:12     ` [PATCH v3 8/8] maintenance: use 'git config --fixed-value' Derrick Stolee via GitGitGadget
2020-11-25 23:09       ` Junio C Hamano
2020-11-25 23:00     ` [PATCH v3 0/8] config: add --fixed-value option Junio C Hamano
2020-11-26 11:17       ` Derrick Stolee
2020-12-01  4:45         ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqk0uhvuy9.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=derrickstolee@github.com \
    --cc=dstolee@microsoft.com \
    --cc=emilyshaffer@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=jrnieder@gmail.com \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for the project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git