From: Eric Sunshine <sunshine@sunshineco.com>
Date: Wed, 15 Aug 2018 16:59:26 GMT
Subject: Re: [PATCH] branch: support configuring --sort via .gitconfig
Message-ID: <CAPig+cSUy7rFwhmJ1SFHsAjPkoWparfY6wAjV=6kJyUL3SLQUw@mail.gmail.com>
In-Reply-To: <20180815111544.30711-1-samuel.maftoul@gmail.com>

On Wed, Aug 15, 2018 at 7:16 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.

Your Signed-off-by: is missing. See Documentation/SubmittingPatches.

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> @@ -1034,6 +1034,11 @@ 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.

I realize that you copied this description from 'tag.sort', thus
inherited its existing weakness, but as a reader of this, the first
question which popped into my head was "what are the possible
<value>s? This description gives no clues and leaves the reader
hanging. Better would be either to list the values or point the reader
(possibly with a linkgit:) at documentation which does list them.

> diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
> @@ -272,6 +272,10 @@ 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.
> +       The keys supported are the same as those in `git for-each-ref`.
> +       Sort order defaults to the value configured for the `tag.sort`

Did you mean s/tag/branch/?

> +       variable if it exists, or lexicographic order otherwise. See
> +       linkgit:git-config[1].

Except for the "See linkgit:git-config[1]", isn't this new text mostly
duplicating what this item already says? When I look at
Documentation/git-branch.txt, I see:

    Sort based on the key given. Prefix `-` to sort in descending
    order of the value. You may use the --sort=<key> option
    multiple times, in which case the last key becomes the primary
    key. **The keys supported are the same as those in `git
    for-each-ref`. Sort order defaults to** sorting based on the
    full refname (including `refs/...` prefix). This lists
    detached HEAD (if present) first, then local branches and
    finally remote-tracking branches.

I added ** to highlight the existing text which this duplicates.

> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> @@ -1305,4 +1305,50 @@ test_expect_success 'tracking with unexpected .fetch refspec' '
> +test_expect_success 'configured committerdate sort' '
> +       git init sort &&
> +       (
> +               cd sort &&
> +               git config branch.sort committerdate &&
> +               [...]
> +       )
> +'
> +
> +test_expect_success 'option override configured sort' '
> +       (
> +               cd sort &&
> +               git branch --sort=refname >actual &&

I would trust this test more if it explicitly configured "branch.sort"
rather than inheriting the value from test(s) above it. That way you
wouldn't have to worry about someone later inserting a test above this
one which changes or removes the value.

> +               cat >expect <<-\EOF &&
> +                 a
> +               * b
> +                 c
> +                 master
> +               EOF
> +               test_cmp expect actual
> +       )
> +'
> +
> +test_expect_success 'invalid sort parameter in configuration' '
> +       (
> +               cd sort &&
> +               git config branch.sort "v:notvalid" &&
> +               test_must_fail git branch
> +
> +       )
> +'

Style: Lose the unnecessary blank line.

Thanks.