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

2014-07-10 Thread Junio C Hamano 
"Keller, Jacob E"  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  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).



--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-07-10 Thread Keller, Jacob E 
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  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
N�r��yb�X��ǧv�^�)޺{.n�+ا���ܨ}���Ơz�&j:+v���zZ+��+zf���h���~i���z��w���?�&�)ߢf

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

2014-07-10 Thread Keller, Jacob E 
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 
> ---
>  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;

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

2014-07-10 Thread Jeff King 
On Thu, Jul 10, 2014 at 10:59:36AM -0700, Junio C Hamano wrote:

> Jeff King  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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-07-10 Thread Junio C Hamano 
Jeff King  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 


 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;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-07-09 Thread Jeff King 
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 
---
 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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-07-09 Thread Jeff King 
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] tag: support configuring --sort via .gitconfig

2014-07-09 Thread Jacob Keller 
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 
Signed-off-by: Jacob Keller 
---
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..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=". 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 ] [-f] [-m |-F ]  
[]"),
N_("git tag -d ..."),
@@ -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