* [PATCH v2] tag: support configuring --sort via .gitconfig
@ 2014-07-09 22:36 Jacob Keller
2014-07-10 4:00 ` Jeff King
2014-07-10 4:07 ` Jeff King
0 siblings, 2 replies; 8+ messages in thread
From: Jacob Keller @ 2014-07-09 22:36 UTC (permalink / raw)
To: git; +Cc: Jacob Keller, Jeff King
Add support for configuring default sort ordering for git tags. Command
line option will override this configured value, using the exact same
syntax.
Cc: Jeff King <peff@peff.net>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
Repost with changes suggested by Peff. These include fixing documentation to
remove duplicatation, and having git-config reference git-tag. Directly assign
a tag_sort global that we use for the config and option variable.
Documentation/config.txt | 6 +++++
Documentation/git-tag.txt | 1 +
builtin/tag.c | 60 ++++++++++++++++++++++++++++++-----------------
t/t7004-tag.sh | 21 +++++++++++++++++
4 files changed, 67 insertions(+), 21 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1d718bdb9662..ad8e75fed988 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2354,6 +2354,12 @@ submodule.<name>.ignore::
"--ignore-submodules" option. The 'git submodule' commands are not
affected by this setting.
+tag.sort::
+ This variable is used to control the sort ordering of tags. It is
+ interpreted precisely the same way as "--sort=<value>". If --sort is
+ given on the command line it will override the selection chosen in the
+ configuration. See linkgit:git-tag[1] for more details.
+
tar.umask::
This variable can be used to restrict the permission bits of
tar archive entries. The default is 0002, which turns off the
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index b424a1bc48bb..2d246725aeb5 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -317,6 +317,7 @@ include::date-formats.txt[]
SEE ALSO
--------
linkgit:git-check-ref-format[1].
+linkgit:git-config[1].
GIT
---
diff --git a/builtin/tag.c b/builtin/tag.c
index ef765563388c..a679e32db866 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -18,6 +18,8 @@
#include "sha1-array.h"
#include "column.h"
+static int tag_sort = 0;
+
static const char * const git_tag_usage[] = {
N_("git tag [-a|-s|-u <key-id>] [-f] [-m <msg>|-F <file>] <tagname> [<head>]"),
N_("git tag -d <tagname>..."),
@@ -346,9 +348,39 @@ static const char tag_template_nocleanup[] =
"Lines starting with '%c' will be kept; you may remove them"
" yourself if you want to.\n");
+static int parse_sort_string(const char *arg)
+{
+ int sort = 0;
+ int flags = 0;
+
+ if (*arg == '-') {
+ flags |= REVERSE_SORT;
+ arg++;
+ }
+ if (starts_with(arg, "version:")) {
+ sort = VERCMP_SORT;
+ arg += 8;
+ } else if (starts_with(arg, "v:")) {
+ sort = VERCMP_SORT;
+ arg += 2;
+ } else
+ sort = STRCMP_SORT;
+ if (strcmp(arg, "refname"))
+ die(_("unsupported sort specification %s"), arg);
+ sort |= flags;
+
+ return sort;
+}
+
static int git_tag_config(const char *var, const char *value, void *cb)
{
- int status = git_gpg_config(var, value, cb);
+ int status;
+
+ if (!strcmp(var, "tag.sort")) {
+ tag_sort = parse_sort_string(value);
+ }
+
+ status = git_gpg_config(var, value, cb);
if (status)
return status;
if (starts_with(var, "column."))
@@ -522,23 +554,9 @@ static int parse_opt_points_at(const struct option *opt __attribute__((unused)),
static int parse_opt_sort(const struct option *opt, const char *arg, int unset)
{
int *sort = opt->value;
- int flags = 0;
- if (*arg == '-') {
- flags |= REVERSE_SORT;
- arg++;
- }
- if (starts_with(arg, "version:")) {
- *sort = VERCMP_SORT;
- arg += 8;
- } else if (starts_with(arg, "v:")) {
- *sort = VERCMP_SORT;
- arg += 2;
- } else
- *sort = STRCMP_SORT;
- if (strcmp(arg, "refname"))
- die(_("unsupported sort specification %s"), arg);
- *sort |= flags;
+ *sort = parse_sort_string(arg);
+
return 0;
}
@@ -552,7 +570,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
struct create_tag_options opt;
char *cleanup_arg = NULL;
int annotate = 0, force = 0, lines = -1;
- int cmdmode = 0, sort = 0;
+ int cmdmode = 0;
const char *msgfile = NULL, *keyid = NULL;
struct msg_arg msg = { 0, STRBUF_INIT };
struct commit_list *with_commit = NULL;
@@ -578,7 +596,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
OPT__FORCE(&force, N_("replace the tag if exists")),
OPT_COLUMN(0, "column", &colopts, N_("show tag list in columns")),
{
- OPTION_CALLBACK, 0, "sort", &sort, N_("type"), N_("sort tags"),
+ OPTION_CALLBACK, 0, "sort", &tag_sort, N_("type"), N_("sort tags"),
PARSE_OPT_NONEG, parse_opt_sort
},
@@ -634,9 +652,9 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
copts.padding = 2;
run_column_filter(colopts, &copts);
}
- if (lines != -1 && sort)
+ if (lines != -1 && tag_sort)
die(_("--sort and -n are incompatible"));
- ret = list_tags(argv, lines == -1 ? 0 : lines, with_commit, sort);
+ ret = list_tags(argv, lines == -1 ? 0 : lines, with_commit, tag_sort);
if (column_active(colopts))
stop_column_filter();
return ret;
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index e4ab0f5b6419..1e8300f6ed7c 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1423,6 +1423,27 @@ EOF
test_cmp expect actual
'
+test_expect_success 'configured lexical sort' '
+ git config tag.sort "v:refname" &&
+ git tag -l "foo*" >actual &&
+ cat >expect <<EOF &&
+foo1.3
+foo1.6
+foo1.10
+EOF
+ test_cmp expect actual
+'
+
+test_expect_success 'option override configured sort' '
+ git tag -l --sort=-refname "foo*" >actual &&
+ cat >expect <<EOF &&
+foo1.6
+foo1.3
+foo1.10
+EOF
+ test_cmp expect actual
+'
+
run_with_limited_stack () {
(ulimit -s 64 && "$@")
}
--
2.0.1.475.g9b8d714
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] tag: support configuring --sort via .gitconfig
2014-07-09 22:36 [PATCH v2] tag: support configuring --sort via .gitconfig Jacob Keller
@ 2014-07-10 4:00 ` Jeff King
2014-07-10 4:07 ` Jeff King
1 sibling, 0 replies; 8+ messages in thread
From: Jeff King @ 2014-07-10 4:00 UTC (permalink / raw)
To: Jacob Keller; +Cc: git
On Wed, Jul 09, 2014 at 03:36:51PM -0700, Jacob Keller wrote:
> Add support for configuring default sort ordering for git tags. Command
> line option will override this configured value, using the exact same
> syntax.
Thanks, this version looks pretty good to me. A few minor comments:
> + if (!strcmp(var, "tag.sort")) {
> + tag_sort = parse_sort_string(value);
> + }
Our style is to usually avoid braces for a one-liner. However, I think
would actually make sense to "return 0" from this conditional.
> +test_expect_success 'configured lexical sort' '
> + git config tag.sort "v:refname" &&
> + git tag -l "foo*" >actual &&
> [...]
Please use:
test_config tag.sort "v:refname"
here, which will clean up the config value after the test ends (and thus
not pollute any later tests).
Though you will need to add an extra "test_config" to the following
test:
> +test_expect_success 'option override configured sort' '
> + git tag -l --sort=-refname "foo*" >actual &&
> [...]
I think that's a good thing, though (it makes it more clear in the
second test what is being tested, rather than relying on the state left
by the previous test).
-Peff
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] tag: support configuring --sort via .gitconfig
2014-07-09 22:36 [PATCH v2] tag: support configuring --sort via .gitconfig Jacob Keller
2014-07-10 4:00 ` Jeff King
@ 2014-07-10 4:07 ` Jeff King
2014-07-10 17:59 ` Junio C Hamano
2014-07-10 20:38 ` Keller, Jacob E
1 sibling, 2 replies; 8+ messages in thread
From: Jeff King @ 2014-07-10 4:07 UTC (permalink / raw)
To: Jacob Keller; +Cc: git
On Wed, Jul 09, 2014 at 03:36:51PM -0700, Jacob Keller wrote:
> +static int parse_sort_string(const char *arg)
> +{
> + int sort = 0;
> + int flags = 0;
> +
> + if (*arg == '-') {
> + flags |= REVERSE_SORT;
> + arg++;
> + }
> + if (starts_with(arg, "version:")) {
> + sort = VERCMP_SORT;
> + arg += 8;
> + } else if (starts_with(arg, "v:")) {
> + sort = VERCMP_SORT;
> + arg += 2;
> + } else
> + sort = STRCMP_SORT;
> + if (strcmp(arg, "refname"))
> + die(_("unsupported sort specification %s"), arg);
> + sort |= flags;
> +
> + return sort;
> +}
I know this is existing code you are moving, but I noticed it looks ripe
for using skip_prefix. Perhaps while we are in the area we should do the
following on top of your patch (I'd also be happy for it be squashed
in, but that may be too much in one patch).
-- >8 --
Subject: [PATCH] tag: use skip_prefix instead of magic numbers
We can make the parsing of the --sort parameter a bit more
readable by having skip_prefix keep our pointer up to date.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/tag.c | 14 +++++---------
1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/builtin/tag.c b/builtin/tag.c
index a679e32..016df98 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -353,18 +353,14 @@ static int parse_sort_string(const char *arg)
int sort = 0;
int flags = 0;
- if (*arg == '-') {
+ if (skip_prefix(arg, "-", &arg))
flags |= REVERSE_SORT;
- arg++;
- }
- if (starts_with(arg, "version:")) {
- sort = VERCMP_SORT;
- arg += 8;
- } else if (starts_with(arg, "v:")) {
+
+ if (skip_prefix(arg, "version:", &arg) || skip_prefix(arg, "v:", &arg))
sort = VERCMP_SORT;
- arg += 2;
- } else
+ else
sort = STRCMP_SORT;
+
if (strcmp(arg, "refname"))
die(_("unsupported sort specification %s"), arg);
sort |= flags;
--
2.0.0.566.gfe3e6b2
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] tag: support configuring --sort via .gitconfig
2014-07-10 4:07 ` Jeff King
@ 2014-07-10 17:59 ` Junio C Hamano
2014-07-10 19:34 ` Jeff King
2014-07-10 20:38 ` Keller, Jacob E
1 sibling, 1 reply; 8+ messages in thread
From: Junio C Hamano @ 2014-07-10 17:59 UTC (permalink / raw)
To: Jeff King; +Cc: Jacob Keller, git
Jeff King <peff@peff.net> writes:
> I know this is existing code you are moving, but I noticed it looks ripe
> for using skip_prefix. Perhaps while we are in the area we should do the
> following on top of your patch (I'd also be happy for it be squashed
> in, but that may be too much in one patch).
I am tempted to suggest going the other way around, i.e. queue (an
equivalent of) this on jk/skip-prefix and merge it to 'next' and
then 'master' quickly.
I can go either way, but I tend to prefer building new things on top
of obviously correct clean-up, not the other way around.
> -- >8 --
> Subject: [PATCH] tag: use skip_prefix instead of magic numbers
>
> We can make the parsing of the --sort parameter a bit more
> readable by having skip_prefix keep our pointer up to date.
>
> Signed-off-by: Jeff King <peff@peff.net>
builtin/tag.c | 14 +++++---------
1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/builtin/tag.c b/builtin/tag.c
index c6e8a71..1101c19 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -524,18 +524,14 @@ static int parse_opt_sort(const struct option *opt, const char *arg, int unset)
int *sort = opt->value;
int flags = 0;
- if (*arg == '-') {
+ if (skip_prefix(arg, "-", &arg))
flags |= REVERSE_SORT;
- arg++;
- }
- if (starts_with(arg, "version:")) {
- *sort = VERCMP_SORT;
- arg += 8;
- } else if (starts_with(arg, "v:")) {
+
+ if (skip_prefix(arg, "version:", &arg) || skip_prefix(arg, "v:", &arg))
*sort = VERCMP_SORT;
- arg += 2;
- } else
+ else
*sort = STRCMP_SORT;
+
if (strcmp(arg, "refname"))
die(_("unsupported sort specification %s"), arg);
*sort |= flags;
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] tag: support configuring --sort via .gitconfig
2014-07-10 17:59 ` Junio C Hamano
@ 2014-07-10 19:34 ` Jeff King
2014-07-10 21:50 ` Keller, Jacob E
0 siblings, 1 reply; 8+ messages in thread
From: Jeff King @ 2014-07-10 19:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jacob Keller, git
On Thu, Jul 10, 2014 at 10:59:36AM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > I know this is existing code you are moving, but I noticed it looks ripe
> > for using skip_prefix. Perhaps while we are in the area we should do the
> > following on top of your patch (I'd also be happy for it be squashed
> > in, but that may be too much in one patch).
>
> I am tempted to suggest going the other way around, i.e. queue (an
> equivalent of) this on jk/skip-prefix and merge it to 'next' and
> then 'master' quickly.
>
> I can go either way, but I tend to prefer building new things on top
> of obviously correct clean-up, not the other way around.
Me too. I just didn't want to make more work for Jacob (in having to
rebase on top of mine) or for you (in having to do a non-obvious merge a
few days from now).
-Peff
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] tag: support configuring --sort via .gitconfig
2014-07-10 4:07 ` Jeff King
2014-07-10 17:59 ` Junio C Hamano
@ 2014-07-10 20:38 ` Keller, Jacob E
1 sibling, 0 replies; 8+ messages in thread
From: Keller, Jacob E @ 2014-07-10 20:38 UTC (permalink / raw)
To: peff@peff.net; +Cc: git@vger.kernel.org
On Thu, 2014-07-10 at 00:07 -0400, Jeff King wrote:
> On Wed, Jul 09, 2014 at 03:36:51PM -0700, Jacob Keller wrote:
>
> > +static int parse_sort_string(const char *arg)
> > +{
> > + int sort = 0;
> > + int flags = 0;
> > +
> > + if (*arg == '-') {
> > + flags |= REVERSE_SORT;
> > + arg++;
> > + }
> > + if (starts_with(arg, "version:")) {
> > + sort = VERCMP_SORT;
> > + arg += 8;
> > + } else if (starts_with(arg, "v:")) {
> > + sort = VERCMP_SORT;
> > + arg += 2;
> > + } else
> > + sort = STRCMP_SORT;
> > + if (strcmp(arg, "refname"))
> > + die(_("unsupported sort specification %s"), arg);
> > + sort |= flags;
> > +
> > + return sort;
> > +}
>
> I know this is existing code you are moving, but I noticed it looks ripe
> for using skip_prefix. Perhaps while we are in the area we should do the
> following on top of your patch (I'd also be happy for it be squashed
> in, but that may be too much in one patch).
>
I am fine with this being another patch or squashed in. I didn't even
know that was available :) I also like putting the two equivalent
conditionals into the same if block.
Thanks,
Jake
> -- >8 --
> Subject: [PATCH] tag: use skip_prefix instead of magic numbers
>
> We can make the parsing of the --sort parameter a bit more
> readable by having skip_prefix keep our pointer up to date.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> builtin/tag.c | 14 +++++---------
> 1 file changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/builtin/tag.c b/builtin/tag.c
> index a679e32..016df98 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -353,18 +353,14 @@ static int parse_sort_string(const char *arg)
> int sort = 0;
> int flags = 0;
>
> - if (*arg == '-') {
> + if (skip_prefix(arg, "-", &arg))
> flags |= REVERSE_SORT;
> - arg++;
> - }
> - if (starts_with(arg, "version:")) {
> - sort = VERCMP_SORT;
> - arg += 8;
> - } else if (starts_with(arg, "v:")) {
> +
> + if (skip_prefix(arg, "version:", &arg) || skip_prefix(arg, "v:", &arg))
> sort = VERCMP_SORT;
> - arg += 2;
> - } else
> + else
> sort = STRCMP_SORT;
> +
> if (strcmp(arg, "refname"))
> die(_("unsupported sort specification %s"), arg);
> sort |= flags;
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] tag: support configuring --sort via .gitconfig
2014-07-10 19:34 ` Jeff King
@ 2014-07-10 21:50 ` Keller, Jacob E
2014-07-10 22:46 ` Junio C Hamano
0 siblings, 1 reply; 8+ messages in thread
From: Keller, Jacob E @ 2014-07-10 21:50 UTC (permalink / raw)
To: peff@peff.net; +Cc: git@vger.kernel.org, gitster@pobox.com
On Thu, 2014-07-10 at 15:34 -0400, Jeff King wrote:
> On Thu, Jul 10, 2014 at 10:59:36AM -0700, Junio C Hamano wrote:
>
> > Jeff King <peff@peff.net> writes:
> >
> > > I know this is existing code you are moving, but I noticed it looks ripe
> > > for using skip_prefix. Perhaps while we are in the area we should do the
> > > following on top of your patch (I'd also be happy for it be squashed
> > > in, but that may be too much in one patch).
> >
> > I am tempted to suggest going the other way around, i.e. queue (an
> > equivalent of) this on jk/skip-prefix and merge it to 'next' and
> > then 'master' quickly.
> >
> > I can go either way, but I tend to prefer building new things on top
> > of obviously correct clean-up, not the other way around.
>
> Me too. I just didn't want to make more work for Jacob (in having to
> rebase on top of mine) or for you (in having to do a non-obvious merge a
> few days from now).
>
> -Peff
I'm perfectly fine rebasing. :)
Thanks,
Jake
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] tag: support configuring --sort via .gitconfig
2014-07-10 21:50 ` Keller, Jacob E
@ 2014-07-10 22:46 ` Junio C Hamano
0 siblings, 0 replies; 8+ messages in thread
From: Junio C Hamano @ 2014-07-10 22:46 UTC (permalink / raw)
To: Keller, Jacob E; +Cc: peff@peff.net, git@vger.kernel.org
"Keller, Jacob E" <jacob.e.keller@intel.com> writes:
> On Thu, 2014-07-10 at 15:34 -0400, Jeff King wrote:
>> On Thu, Jul 10, 2014 at 10:59:36AM -0700, Junio C Hamano wrote:
>>
>> > Jeff King <peff@peff.net> writes:
>> >
>> > > I know this is existing code you are moving, but I noticed it looks ripe
>> > > for using skip_prefix. Perhaps while we are in the area we should do the
>> > > following on top of your patch (I'd also be happy for it be squashed
>> > > in, but that may be too much in one patch).
>> >
>> > I am tempted to suggest going the other way around, i.e. queue (an
>> > equivalent of) this on jk/skip-prefix and merge it to 'next' and
>> > then 'master' quickly.
>> >
>> > I can go either way, but I tend to prefer building new things on top
>> > of obviously correct clean-up, not the other way around.
>>
>> Me too. I just didn't want to make more work for Jacob (in having to
>> rebase on top of mine) or for you (in having to do a non-obvious merge a
>> few days from now).
>>
>> -Peff
>
> I'm perfectly fine rebasing. :)
Alright, thanks.
I am still not ready to push out today's integration result, but
when it happens, Peff's "tag: use skip_prefix" should appear at
ce85604, as a direct follow-up to the tip of already merged
jk/skip-prefix topic which was 67a31f61 (http-push: refactor parsing
of remote object names, 2014-06-19).
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2014-07-10 22:46 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-07-09 22:36 [PATCH v2] tag: support configuring --sort via .gitconfig Jacob Keller
2014-07-10 4:00 ` Jeff King
2014-07-10 4:07 ` Jeff King
2014-07-10 17:59 ` Junio C Hamano
2014-07-10 19:34 ` Jeff King
2014-07-10 21:50 ` Keller, Jacob E
2014-07-10 22:46 ` Junio C Hamano
2014-07-10 20:38 ` Keller, Jacob E
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).