parse_opt_ref_sorting: always use with NONEG flag

The "--sort" parameter of for-each-ref, etc, does not handle negation,
and instead returns an error to the parse-options code. But neither
piece of code prints anything for the user, which may leave them
confused:

$ git for-each-ref --no-sort
$ echo $?
129

As the comment in the callback function notes, this probably should
clear the list, which would make it consistent with other list-like
options (i.e., anything that uses OPT_STRING_LIST currently).
Unfortunately that's a bit tricky due to the way the ref-filter code
works. But in the meantime, let's at least make the error a little less
confusing:

- switch to using PARSE_OPT_NONEG in the option definition, which will
cause the options code to produce a useful message

- since this was cut-and-pasted to four different spots, let's define
a single OPT_REF_SORT() macro that we can use everywhere

- the callback can use BUG_ON_OPT_NEG() to make sure the correct flags
are used (incidentally, this also satisfies -Wunused-parameters,
since we're now looking at "unset")

- expand the comment into a NEEDSWORK to make it clear that the
direction is right, but the details need to be worked out

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/branch.c b/builtin/branch.c
index 4c830557306398360e06f0e53166e78a4c590da4..d4359b33ac0fb27b6b8be7109a2a862bc9cd2d77 100644 (file)
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -644,8 +644,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
                OPT_MERGED(&filter, N_("print only branches that are merged")),
                OPT_NO_MERGED(&filter, N_("print only branches that are not merged")),
                OPT_COLUMN(0, "column", &colopts, N_("list branches in columns")),
                OPT_MERGED(&filter, N_("print only branches that are merged")),
                OPT_NO_MERGED(&filter, N_("print only branches that are not merged")),
                OPT_COLUMN(0, "column", &colopts, N_("list branches in columns")),

-               OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"),
-                            N_("field name to sort on"), &parse_opt_ref_sorting),
+               OPT_REF_SORT(sorting_tail),

                {
                        OPTION_CALLBACK, 0, "points-at", &filter.points_at, N_("object"),
                        N_("print only branches of the object"), 0, parse_opt_object_name
                {
                        OPTION_CALLBACK, 0, "points-at", &filter.points_at, N_("object"),
                        N_("print only branches of the object"), 0, parse_opt_object_name

diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index e931be9ce4d9f180de23d09d227e5a2f571f4f1a..465153e85337fdf6b857a94b8c245fe33832b0dc 100644 (file)
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -37,8 +37,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
                OPT_INTEGER( 0 , "count", &maxcount, N_("show only <n> matched refs")),
                OPT_STRING(  0 , "format", &format.format, N_("format"), N_("format to use for the output")),
                OPT__COLOR(&format.use_color, N_("respect format colors")),
                OPT_INTEGER( 0 , "count", &maxcount, N_("show only <n> matched refs")),
                OPT_STRING(  0 , "format", &format.format, N_("format"), N_("format to use for the output")),
                OPT__COLOR(&format.use_color, N_("respect format colors")),

-               OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"),
-                           N_("field name to sort on"), &parse_opt_ref_sorting),
+               OPT_REF_SORT(sorting_tail),

                OPT_CALLBACK(0, "points-at", &filter.points_at,
                             N_("object"), N_("print only refs which points at the given object"),
                             parse_opt_object_name),
                OPT_CALLBACK(0, "points-at", &filter.points_at,
                             N_("object"), N_("print only refs which points at the given object"),
                             parse_opt_object_name),

diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index 1d7f1f5ce27834cafcf934db2085b36d4301e3c0..6ef519514bd1a4dfc39b149967ee60dcf14dfbaa 100644 (file)
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -67,8 +67,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
                OPT_BIT(0, "refs", &flags, N_("do not show peeled tags"), REF_NORMAL),
                OPT_BOOL(0, "get-url", &get_url,
                         N_("take url.<base>.insteadOf into account")),
                OPT_BIT(0, "refs", &flags, N_("do not show peeled tags"), REF_NORMAL),
                OPT_BOOL(0, "get-url", &get_url,
                         N_("take url.<base>.insteadOf into account")),

-               OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"),
-                            N_("field name to sort on"), &parse_opt_ref_sorting),
+               OPT_REF_SORT(sorting_tail),

                OPT_SET_INT_F(0, "exit-code", &status,
                              N_("exit with exit code 2 if no matching refs are found"),
                              2, PARSE_OPT_NOCOMPLETE),
                OPT_SET_INT_F(0, "exit-code", &status,
                              N_("exit with exit code 2 if no matching refs are found"),
                              2, PARSE_OPT_NOCOMPLETE),

diff --git a/builtin/tag.c b/builtin/tag.c
index 02f6bd1279d9c360f97d04ebba0b1f88ff2f27e1..ad97595fbfaa25c8ec44efec8ce5f946cf7de184 100644 (file)
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -412,8 +412,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
                OPT_WITHOUT(&filter.no_commit, N_("print only tags that don't contain the commit")),
                OPT_MERGED(&filter, N_("print only tags that are merged")),
                OPT_NO_MERGED(&filter, N_("print only tags that are not merged")),
                OPT_WITHOUT(&filter.no_commit, N_("print only tags that don't contain the commit")),
                OPT_MERGED(&filter, N_("print only tags that are merged")),
                OPT_NO_MERGED(&filter, N_("print only tags that are not merged")),

-               OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"),
-                            N_("field name to sort on"), &parse_opt_ref_sorting),
+               OPT_REF_SORT(sorting_tail),

                {
                        OPTION_CALLBACK, 0, "points-at", &filter.points_at, N_("object"),
                        N_("print only tags of the object"), PARSE_OPT_LASTARG_DEFAULT,
                {
                        OPTION_CALLBACK, 0, "points-at", &filter.points_at, N_("object"),
                        N_("print only tags of the object"), PARSE_OPT_LASTARG_DEFAULT,

diff --git a/ref-filter.c b/ref-filter.c
index 3aca105307863f5342699babfbea2ac3035fdacb..8d11a94cbd40d89302ec8b03245744f00bc02abf 100644 (file)
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2337,8 +2337,13 @@ void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *arg)
 
 int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset)
 {
 
 int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset)
 {

-       if (!arg) /* should --no-sort void the list ? */
-               return -1;
+       /*
+        * NEEDSWORK: We should probably clear the list in this case, but we've
+        * already munged the global used_atoms list, which would need to be
+        * undone.
+        */
+       BUG_ON_OPT_NEG(unset);
+

        parse_ref_sorting(opt->value, arg);
        return 0;
 }
        parse_ref_sorting(opt->value, arg);
        return 0;
 }

diff --git a/ref-filter.h b/ref-filter.h
index 85c8ebc3b904e9b44bed8b164b9cdf62839d6dae..f1dcff4c6e23e831eff9915b659da062a835e8c8 100644 (file)
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -96,6 +96,11 @@ struct ref_format {
 #define OPT_MERGED(f, h) _OPT_MERGED_NO_MERGED("merged", f, h)
 #define OPT_NO_MERGED(f, h) _OPT_MERGED_NO_MERGED("no-merged", f, h)
 
 #define OPT_MERGED(f, h) _OPT_MERGED_NO_MERGED("merged", f, h)
 #define OPT_NO_MERGED(f, h) _OPT_MERGED_NO_MERGED("no-merged", f, h)
 

+#define OPT_REF_SORT(var) \
+       OPT_CALLBACK_F(0, "sort", (var), \
+                      N_("key"), N_("field name to sort on"), \
+                      PARSE_OPT_NONEG, parse_opt_ref_sorting)
+

 /*
  * API for filtering a set of refs. Based on the type of refs the user
  * has requested, we iterate through those refs and apply filters
 /*
  * API for filtering a set of refs. Based on the type of refs the user
  * has requested, we iterate through those refs and apply filters