Re: [PATCH 2/2 v4] tag: support configuring --sort via .gitconfig
Jacob Keller jacob.e.kel...@intel.com 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 p...@peff.net
Signed-off-by: Jacob Keller jacob.e.kel...@intel.com
---
- 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.
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?
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?
@@ -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.
+ 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?
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
Re: [PATCH 2/2 v4] tag: support configuring --sort via .gitconfig
On Fri, 2014-07-11 at 08:04 -0700, Junio C Hamano wrote:
Jacob Keller jacob.e.kel...@intel.com 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 p...@peff.net
Signed-off-by: Jacob Keller jacob.e.kel...@intel.com
---
- 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,
Re: [PATCH 2/2 v4] tag: support configuring --sort via .gitconfig
Keller, Jacob E jacob.e.kel...@intel.com writes: On Fri, 2014-07-11 at 08:04 -0700, Junio C Hamano wrote: ... +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. Either would be fine, but just to clarify. Because these macro definitions are for the .sort field of that structure, and the new tag_sort variable is the second user of that macro, my suggestion to put it _after_ was to be in line with add new things at the end, when there is no compelling reason not to below. 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 ;-). Thanks. -- 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 2/2 v4] tag: support configuring --sort via .gitconfig
On Fri, 2014-07-11 at 08:04 -0700, Junio C Hamano wrote:
Jacob Keller jacob.e.kel...@intel.com 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 p...@peff.net
Signed-off-by: Jacob Keller jacob.e.kel...@intel.com
---
- 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.
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?
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?
@@ -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.
+ 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?
When there is no reason to have things in
Re: [PATCH 2/2 v4] tag: support configuring --sort via .gitconfig
Keller, Jacob E jacob.e.kel...@intel.com writes: This is not how the rest of the current tests work. I will submit a patch which fixes up the current --sort tests (but not every test, for now) as well. I do not want to pile more work that is unrelated to the task at hand on your plate, i.e. clean-up work, so I would assign a very low priority to fix up the current tests. At the same time, existing mistakes are not excuses to introduce new similar ones, hence my suggestions to the added code in the patch I saw. -- 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 2/2 v4] tag: support configuring --sort via .gitconfig
On Fri, 2014-07-11 at 11:29 -0700, Junio C Hamano wrote: Keller, Jacob E jacob.e.kel...@intel.com writes: This is not how the rest of the current tests work. I will submit a patch which fixes up the current --sort tests (but not every test, for now) as well. I do not want to pile more work that is unrelated to the task at hand on your plate, i.e. clean-up work, so I would assign a very low priority to fix up the current tests. At the same time, existing mistakes are not excuses to introduce new similar ones, hence my suggestions to the added code in the patch I saw. It was trivial to fix at least the --sort tests, so I submitted a patch that goes before this one to fix those as well. Thanks, Jake
