Re: [PATCH v2 07/13] builtin/config: introduce "get" subcommand

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 13, 2024, 03:11 UTC
Message-ID: <CAPig+cRwh4HCi=Q01tGJ0WOb59iE18HWSCNxGogcreOz+2w1WA@mail.gmail.com>
In-Reply-To: <8ceced0fc58e9b4cf1c5e5b678e864f82c1b8c01.1710198711.git.ps@pks.im>

On Mon, Mar 11, 2024 at 7:20 PM Patrick Steinhardt <ps@pks.im> wrote:

Show 12 quoted lines

> Introduce a new "get" subcommand to git-config(1). Please refer to
> preceding commits regarding the motivation behind this change.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> @@ -80,6 +76,12 @@ COMMANDS
> +get::
> +       Get value for one or more config options. Values can be filtered by
> +       regexes and URLs.Returns error code 1 if the key was not found and the
> +       last value if multiple key values were found. If `--all` is set, then
> +       all values will be shown.

s/URLs.Returns/URLs. Returns/

It's not a new problem with this description (since you're mostly just relocating existing text), but I find the discussion of what is returned quite confusing and difficult to interpret. Breaking it down into simpler sentences might help:

    Emits the value of the specified key. If key is present
    multiple times in the configuration, emits the last
    value. If `--all` is specified, emits all values
    associated with key. Returns error code 1 if key
    is not present.

But, doing so may be outside the scope of this patch series and can be tackled at a later date (or not at all).

> @@ -93,22 +95,16 @@ OPTIONS
> +--all::
> +       With "get", Return all values for a multi-valued key.

s/Return/return/ s/"get"/`get`/

Show 5 quoted lines

> +---regexp::
> +       With "get", interpret the name as a regular expression. Regular
> +       expression matching is currently case-sensitive and done against a
> +       canonicalized version of the key in which section and variable names
> +       are lowercased, but subsection names are not.

s/"get"/`get`/

Show 5 quoted lines

> @@ -286,7 +271,7 @@ Valid `<type>`'s include:
>  --default <value>::
> -  When using `--get`, and the requested variable is not found, behave as if
> +  When using `get`, and the requested variable is not found, behave as if
>    <value> were the value assigned to the that variable.

Not a fault of this patch (and need not be fixed by this series): "to the that" should be either "to the" or "to that".

Show 14 quoted lines

> @@ -506,25 +509,25 @@ you have to provide a regex matching the value of exactly one line.
>  To query the value for a given key, do
>
>  ------------
> -% git config --get core.filemode
> +% git config get core.filemode
>  ------------
>
>  or
>
>  ------------
> -% git config core.filemode
> +% git config get core.filemode
>  ------------

Meh. We only need to retain one of these examples now, not both, right?

Show 17 quoted lines

> diff --git a/t/t1300-config.sh b/t/t1300-config.sh
> @@ -17,9 +17,15 @@ do
>  case "$mode" in
>  legacy)
>         mode_prefix="--"
> +       mode_get=""
> +       mode_get_all="--get-all"
> +       mode_get_regexp="--get-regexp"
>         ;;
>  subcommands)
>         mode_prefix=""
> +       mode_get="get"
> +       mode_get_all="get --all"
> +       mode_get_regexp="get --regexp --all --show-names"
>         ;;
>  *)
>         echo "unknown mode $mode" >&2

The variables added by this patch to the `case` arms invalidate the suggested simplification in my review of [6/13].