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

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

[samuel.maftoul]

From: Samuel Maftoul <samuel.maftoul@gmail.com>

Add support for configuring default sort ordering for git branches. Command
line option will override this configured value, using the exact same
syntax.
---
 Documentation/config.txt     |  5 +++++
 Documentation/git-branch.txt |  4 ++++
 builtin/branch.c             | 10 +++++++++-
 t/t3200-branch.sh            | 46 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 64 insertions(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 63365dcf3..82e306d20 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1034,6 +1034,11 @@ branch.autoSetupRebase::
 	branch to track another branch.
 	This option defaults to never.
 
+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.
+
 branch.<name>.remote::
 	When on branch <name>, it tells 'git fetch' and 'git push'
 	which remote to fetch from/push to.  The remote to push to
diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index 1072ca0eb..9212a8d5d 100644
--- 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`
+	variable if it exists, or lexicographic order otherwise. See
+	linkgit:git-config[1].
 
 
 --points-at <object>::
diff --git a/builtin/branch.c b/builtin/branch.c
index 4fc55c350..bbd006aab 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -74,6 +74,14 @@ define_list_config_array(color_branch_slots);
 static int git_branch_config(const char *var, const char *value, void *cb)
 {
 	const char *slot_name;
+	struct ref_sorting **sorting_tail = (struct ref_sorting **)cb;
+
+	if (!strcmp(var, "branch.sort")) {
+		if (!value)
+			return config_error_nonbool(var);
+		parse_ref_sorting(sorting_tail, value);
+		return 0;
+	}
 
 	if (starts_with(var, "column."))
 		return git_column_config(var, value, "branch", &colopts);
@@ -653,7 +661,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	if (argc == 2 && !strcmp(argv[1], "-h"))
 		usage_with_options(builtin_branch_usage, options);
 
-	git_config(git_branch_config, NULL);
+	git_config(git_branch_config, sorting_tail);
 
 	track = git_branch_track;
 
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index dbca665da..8bd42e9c6 100755
--- 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_commit initial &&
+		git checkout -b a &&
+		test_commit a &&
+		git checkout -b c &&
+		test_commit c &&
+		git checkout -b b &&
+		test_commit b &&
+		git branch >actual &&
+		cat >expect <<-\EOF &&
+		  master
+		  a
+		  c
+		* b
+		EOF
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'option override configured sort' '
+	(
+		cd sort &&
+		git branch --sort=refname >actual &&
+		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
+
+	)
+'
+
 test_done
-- 
2.14.3 (Apple Git-98)

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

[Eric Sunshine]

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.

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

[samuel.maftoul]

From: Samuel Maftoul <samuel.maftoul@gmail.com>

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>
---
 Documentation/config.txt     |  6 +++++
 Documentation/git-branch.txt |  3 +++
 builtin/branch.c             | 10 +++++++-
 t/t3200-branch.sh            | 47 ++++++++++++++++++++++++++++++++++++
 4 files changed, 65 insertions(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 63365dcf3..1236d1ec9 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1034,6 +1034,12 @@ branch.autoSetupRebase::
 	branch to track another branch.
 	This option defaults to never.
 
+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.
+
 branch.<name>.remote::
 	When on branch <name>, it tells 'git fetch' and 'git push'
 	which remote to fetch from/push to.  The remote to push to
diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index 1072ca0eb..1be009a35 100644
--- 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].
 
 
 --points-at <object>::
diff --git a/builtin/branch.c b/builtin/branch.c
index 4fc55c350..bbd006aab 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -74,6 +74,14 @@ define_list_config_array(color_branch_slots);
 static int git_branch_config(const char *var, const char *value, void *cb)
 {
 	const char *slot_name;
+	struct ref_sorting **sorting_tail = (struct ref_sorting **)cb;
+
+	if (!strcmp(var, "branch.sort")) {
+		if (!value)
+			return config_error_nonbool(var);
+		parse_ref_sorting(sorting_tail, value);
+		return 0;
+	}
 
 	if (starts_with(var, "column."))
 		return git_column_config(var, value, "branch", &colopts);
@@ -653,7 +661,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	if (argc == 2 && !strcmp(argv[1], "-h"))
 		usage_with_options(builtin_branch_usage, options);
 
-	git_config(git_branch_config, NULL);
+	git_config(git_branch_config, sorting_tail);
 
 	track = git_branch_track;
 
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index dbca665da..448c93527 100755
--- 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 'configured committerdate sort' '
+	git init sort &&
+	(
+		cd sort &&
+		git config branch.sort committerdate &&
+		test_commit initial &&
+		git checkout -b a &&
+		test_commit a &&
+		git checkout -b c &&
+		test_commit c &&
+		git checkout -b b &&
+		test_commit b &&
+		git branch >actual &&
+		cat >expect <<-\EOF &&
+		  master
+		  a
+		  c
+		* b
+		EOF
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'option override configured sort' '
+	(
+		cd sort &&
+		git config branch.sort committerdate &&
+		git branch --sort=refname >actual &&
+		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
+
+	)
+'
+
 test_done
-- 
2.18.0

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

[Eric Sunshine]

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/

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

[samuel.maftoul]

From: Samuel Maftoul <samuel.maftoul@gmail.com>

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>
---
 Documentation/config.txt     |  6 +++++
 Documentation/git-branch.txt |  5 ++--
 builtin/branch.c             | 10 +++++++-
 t/t3200-branch.sh            | 46 ++++++++++++++++++++++++++++++++++++
 4 files changed, 64 insertions(+), 3 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index fd8d27e76..7f7a50123 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1039,6 +1039,12 @@ branch.autoSetupRebase::
 	branch to track another branch.
 	This option defaults to never.
 
+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.
+
 branch.<name>.remote::
 	When on branch <name>, it tells 'git fetch' and 'git push'
 	which remote to fetch from/push to.  The remote to push to
diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index 1072ca0eb..9767b2b48 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -268,10 +268,11 @@ start-point is either a local or remote-tracking branch.
 	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
+	for-each-ref`. Sort order defaults to the value configured for the
+	`branch.sort` variable if exists, or 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.
+	finally remote-tracking branches. See linkgit:git-config[1].
 
 
 --points-at <object>::
diff --git a/builtin/branch.c b/builtin/branch.c
index 4fc55c350..bbd006aab 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -74,6 +74,14 @@ define_list_config_array(color_branch_slots);
 static int git_branch_config(const char *var, const char *value, void *cb)
 {
 	const char *slot_name;
+	struct ref_sorting **sorting_tail = (struct ref_sorting **)cb;
+
+	if (!strcmp(var, "branch.sort")) {
+		if (!value)
+			return config_error_nonbool(var);
+		parse_ref_sorting(sorting_tail, value);
+		return 0;
+	}
 
 	if (starts_with(var, "column."))
 		return git_column_config(var, value, "branch", &colopts);
@@ -653,7 +661,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	if (argc == 2 && !strcmp(argv[1], "-h"))
 		usage_with_options(builtin_branch_usage, options);
 
-	git_config(git_branch_config, NULL);
+	git_config(git_branch_config, sorting_tail);
 
 	track = git_branch_track;
 
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index dbca665da..93f21ab07 100755
--- 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_commit initial &&
+		git checkout -b a &&
+		test_commit a &&
+		git checkout -b c &&
+		test_commit c &&
+		git checkout -b b &&
+		test_commit b &&
+		git branch >actual &&
+		cat >expect <<-\EOF &&
+		  master
+		  a
+		  c
+		* b
+		EOF
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'option override configured sort' '
+	(
+		cd sort &&
+		git config branch.sort committerdate &&
+		git branch --sort=refname >actual &&
+		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
+	)
+'
+
 test_done
-- 
2.18.0

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

[Junio C Hamano]

samuel.maftoul@gmail.com writes:

> From: Samuel Maftoul <samuel.maftoul@gmail.com>
>
> Add support for configuring default sort ordering for git branches. Command
> line option will override this configured value, using the exact same
> syntax.

Using the exact same syntax as ...?

> Signed-off-by: Samuel Maftoul <samuel.maftoul@gmail.com>
> ---
>  Documentation/config.txt     |  6 +++++
>  Documentation/git-branch.txt |  5 ++--
>  builtin/branch.c             | 10 +++++++-
>  t/t3200-branch.sh            | 46 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 64 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index fd8d27e76..7f7a50123 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1039,6 +1039,12 @@ branch.autoSetupRebase::
>  	branch to track another branch.
>  	This option defaults to never.
>  
> +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.

OK, the answer to the above question is "same syntax as used for the
value of the `branch.sort` configuration variable".

> diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
> index 1072ca0eb..9767b2b48 100644
> --- a/Documentation/git-branch.txt
> +++ b/Documentation/git-branch.txt
> @@ -268,10 +268,11 @@ start-point is either a local or remote-tracking branch.
>  	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
> +	for-each-ref`. Sort order defaults to the value configured for the
> +	`branch.sort` variable if exists, or 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.
> +	finally remote-tracking branches. See linkgit:git-config[1].

OK.

> diff --git a/builtin/branch.c b/builtin/branch.c
> index 4fc55c350..bbd006aab 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -74,6 +74,14 @@ define_list_config_array(color_branch_slots);
>  static int git_branch_config(const char *var, const char *value, void *cb)
>  {
>  	const char *slot_name;
> +	struct ref_sorting **sorting_tail = (struct ref_sorting **)cb;
> +
> +	if (!strcmp(var, "branch.sort")) {
> +		if (!value)
> +			return config_error_nonbool(var);
> +		parse_ref_sorting(sorting_tail, value);
> +		return 0;
> +	}

Hmph.  It is unfortunate that "don't feed me NULL" check is not in
parse_ref_sorting() but is in parse_opt_ref_sorting().  But that is
a separate low-hanging fruit.  The code we see here is correct.

Will queue.  Thanks.