Re: [PATCH] branch: support configuring --sort via .gitconfig

[Eric Sunshine <sunshine@sunshineco.com>]

On Thu, Aug 16, 2018 at 4:06 AM <samuel.maftoul@gmail.com> wrote:
> Add support for configuring default sort ordering for git branches. Command
> line option will override this configured value, using the exact same
> syntax.
>
> Signed-off-by: Samuel Maftoul <samuel.maftoul@gmail.com>
> ---
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> @@ -1034,6 +1034,12 @@ branch.autoSetupRebase::
> +branch.sort::
> +       This variable controls the sort ordering of branches when displayed by
> +       linkgit:git-branch[1]. Without the "--sort=<value>" option provided, the
> +       value of this variable will be used as the default.
> +       See linkgit:git-for-each-ref[1] field names for valid values.

Thanks for adding some information about what values are valid for
this config variable.

> diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
> @@ -272,6 +272,9 @@ start-point is either a local or remote-tracking branch.
>         full refname (including `refs/...` prefix). This lists
>         detached HEAD (if present) first, then local branches and
>         finally remote-tracking branches.
> +       Sort order defaults to the value configured for the `branch.sort`
> +       variable if it exists, or lexicographic order otherwise. See
> +       linkgit:git-config[1].

This change still has problems pointed out by my earlier review[1].
The existing text in git-branch.txt says:

    Sort order defaults to sorting based on the
    full refname...

Which is both redundant with the new text you add and seems to say
something rather different.

> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> @@ -1305,4 +1305,51 @@ test_expect_success 'tracking with unexpected .fetch refspec' '
> +test_expect_success 'invalid sort parameter in configuration' '
> +       (
> +               cd sort &&
> +               git config branch.sort "v:notvalid" &&
> +               test_must_fail git branch
> +
> +       )
> +'

See my earlier review[1] regarding the unnecessary blank line in this new test.

[1]: https://public-inbox.org/git/CAPig+cSUy7rFwhmJ1SFHsAjPkoWparfY6wAjV=6kJyUL3SLQUw@mail.gmail.com/