On Fri, Jul 11, 2014 at 6:55 PM, Jacob Keller <jacob.e.keller@xxxxxxxxx> wrote:
> 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>
> ---
> Made parse_sort_string take a "var" parameter, and if given will only warn
> about invalid parameter, instead of error.
This seems unnecessarily ugly since it's hard-coding specialized
knowledge of the callers' error-reporting requirements into what
should be a generalized parsing function. If you instead make
parse_sort_string() responsible only for attempting to parse the
value, but leave error-reporting to the callers, then this ugliness
goes away. See below.
> diff --git a/builtin/tag.c b/builtin/tag.c
> index 9d7643f127e7..97c5317c28e5 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -32,6 +32,8 @@ static const char * const git_tag_usage[] = {
> #define SORT_MASK 0x7fff
> #define REVERSE_SORT 0x8000
>
> +static int tag_sort;
> +
> struct tag_filter {
> const char **patterns;
> int lines;
> @@ -346,9 +348,49 @@ static const char tag_template_nocleanup[] =
> "Lines starting with '%c' will be kept; you may remove them"
> " yourself if you want to.\n");
>
> +/*
> + * Parse a sort string, and return 0 if parsed successfully. Will return
> + * non-zero when the sort string does not parse into a known type.
> + */
> +static int parse_sort_string(const char *var, const char *value, int *sort)
> +{
> + int type = 0, flags = 0;
> +
> + if (skip_prefix(value, "-", &value))
> + flags |= REVERSE_SORT;
> +
> + if (skip_prefix(value, "version:", &value) || skip_prefix(value, "v:", &value))
> + type = VERCMP_SORT;
> + else
> + type = STRCMP_SORT;
> +
> + if (strcmp(value, "refname")) {
> + if (!var)
> + return error(_("unsupported sort specification '%s'"), value);
> + else {
> + warning(_("unsupported sort specification '%s' in variable '%s'"),
> + var, value);
> + return -1;
Just return -1 here, but don't print any diagnostics. Let the callers
do that. (See below.)
> + }
> + }
> +
> + *sort = (type | flags);
> +
> + return 0;
> +}
> +
> 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")) {
> + if (!value)
> + return config_error_nonbool(var);
> + parse_sort_string(var, value, &tag_sort);
if (parse_sort_string(value, &tag_sort))
warning(_("unsupported sort specification '%s' in variable '%s'"),
var, value);
> + return 0;
> + }
> +
> + status = git_gpg_config(var, value, cb);
> if (status)
> return status;
> if (starts_with(var, "column."))
> @@ -522,20 +564,8 @@ 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 (skip_prefix(arg, "-", &arg))
> - flags |= REVERSE_SORT;
> -
> - if (skip_prefix(arg, "version:", &arg) || skip_prefix(arg, "v:", &arg))
> - *sort = VERCMP_SORT;
> - else
> - *sort = STRCMP_SORT;
> -
> - if (strcmp(arg, "refname"))
> - die(_("unsupported sort specification %s"), arg);
> - *sort |= flags;
> - return 0;
> + return parse_sort_string(NULL, arg, sort);
if (parse_sort_string(arg, sort))
return error(_("unsupported sort specification '%s'"), arg);
return 0;
> }
>
> int cmd_tag(int argc, const char **argv, const char *prefix)
> @@ -548,7 +578,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;
> @@ -574,7 +604,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
> },
>
> @@ -630,9 +660,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;
--
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