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

[Eric Sunshine <sunshine@sunshineco.com>]

On Mon, Mar 11, 2024 at 7:20 PM Patrick Steinhardt <ps@pks.im> wrote:
> 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`/

> +---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`/

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

> @@ -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?

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