On Fri, 2014-07-11 at 08:04 -0700, Junio C Hamano wrote:
> Jacob Keller <jacob.e.keller@xxxxxxxxx> writes:
>
> > 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@xxxxxxxx>
> > Signed-off-by: Jacob Keller <jacob.e.keller@xxxxxxxxx>
> > ---
> > - v4
> > * base on top of suggested change by Jeff King to use skip_prefix instead
> >
> > Documentation/config.txt | 6 ++++++
> > Documentation/git-tag.txt | 1 +
> > builtin/tag.c | 46 ++++++++++++++++++++++++++++++++--------------
> > t/t7004-tag.sh | 21 +++++++++++++++++++++
> > 4 files changed, 60 insertions(+), 14 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.
> > +
>
> This is not technically incorrect per-se, but the third sentence
> talks about "--sort" on "the command line" while this applies only
> to "the command line of the 'git tag' command" and nobody else's
> "--sort" option.
>
> Perhaps rephrasing it like this may be easier to understand?
>
> When "git tag" command is used to list existing tags,
> without "--sort=<value>" option on the command line,
> the value of this variable is used as the default.
>
> This way, it would be clear, without explicitly saying anything,
> that the value will be spelled exactly the same way as you would
> spell the value for the --sort option on the command line.
>
Makes sense.
> > 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].
>
> It is not particularly friendly to readers to refer to
> "git-config[1]" from any other page, if done without spelling out
> which variable the reader is expected to look up. Some addition
> to the description of the "--sort" option is needed if this SEE ALSO
> were to be any useful, I guess?
>
> --sort=<type>::
> ... (existing description) ...
> When this option is not given, the sort order
> defaults to the value configured for the `tag.sort`
> variable, if exists, or lexicographic otherwise.
>
> or something like that, perhaps?
>
Alright, this sounds good too.
> > diff --git a/builtin/tag.c b/builtin/tag.c
> > index 7ccb6f3c581b..a53e27d4e7e4 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;
>
> Please do not initialize variables in bss segment to 0 by hand.
>
> If this variable is meant to take one of these *CMP_SORT values
> defined as macro later in this file, it is better to define this
> variable somewhere after and close to the definitions of the macros.
> Perhaps immediately after the "struct tag_filter" is declared?
>
I put it just above the struct tag_filter now, as this puts it right
below the #defines regarding it's value.
> > @@ -346,9 +348,33 @@ 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 (skip_prefix(arg, "-", &arg))
> > + flags |= REVERSE_SORT;
> > +
> > + if (skip_prefix(arg, "version:", &arg) || skip_prefix(arg, "v:", &arg))
> > + sort = VERCMP_SORT;
> > +
> > + if (strcmp(arg, "refname"))
> > + die(_("unsupported sort specification %s"), arg);
>
> Hmm. I _thought_ we try to catch unsupported option value coming
> from the command line and die but at the same time we try *not* to
> die but warn and whatever is sensible when the value comes from the
> configuration, so that .git/config or $HOME/.gitconfig can be shared
> by those who use different versions of Git.
>
> Do we already have many precedences where we see configuration value
> that our version of Git do not yet understand?
>
> Not a very strong objection; just something that worries me.
>
I like this change, and I think I have a way to handle it. I will
re-work this so that the die is handled by the normal block.
> > + 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);
> > + }
> > +
>
> Why doesn't this return success after noticing that the variable is
> to be interpreted by this block and nobody else?
>
I was copying the git_gpg_config. But yes I agree, returning here would
make sense, and actually maybe fix git_gpg_config as well in a future
patch.
> When there is no reason to have things in a particular order, it is
> customary to add new things at the end, not in the front, unless the
> new thing is so much more important than everything else---but then
> we are no longer talking about the case where there is no reason to
> have things in a particular order ;-).
>
> Remainder of the changes to builtin/tag.c looks good.
Thanks.
>
> > 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
> > +'
>
> Please write the above like so:
>
> ...
> cat >expect <<-\EOF &&
> foo1.3
> ...
> EOF
> test_cmp expect actual
>
> The dash immediately after the here-doc redirection lets us indent
> the data with HT to allow the test boundaries easier to spot, and by
> quoting the token to end here-doc, we relieve the readers from
> having to wonder if there are variable substitutions going on that
> they need to be careful about.
>
> Overall, I think this is done well. Thanks for working on it.
> --
Yep. I'll try to have a new version soon.
Regards,
Jake
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
��.n��������+%������w��{.n��������n�r������&��z�ޗ�zf���h���~����������_��+v���)ߣ�