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>

Jeff King committed Mar 20, 2019 at 16:22 UTC 95be717cd5a5d4956a5152210176e598cf49ec75
6 files changed +16 -10
builtin/branch.c
+1 -2
@@ -644,8 +644,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
644 OPT_MERGED(&filter, N_("print only branches that are merged")),
645 OPT_NO_MERGED(&filter, N_("print only branches that are not merged")),
646 OPT_COLUMN(0, "column", &colopts, N_("list branches in columns")),
647 - OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"),
648 - N_("field name to sort on"), &parse_opt_ref_sorting),
647 + OPT_REF_SORT(sorting_tail),
648 {
649 OPTION_CALLBACK, 0, "points-at", &filter.points_at, N_("object"),
650 N_("print only branches of the object"), 0, parse_opt_object_name
builtin/for-each-ref.c
+1 -2
@@ -37,8 +37,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
37 OPT_INTEGER( 0 , "count", &maxcount, N_("show only <n> matched refs")),
38 OPT_STRING( 0 , "format", &format.format, N_("format"), N_("format to use for the output")),
39 OPT__COLOR(&format.use_color, N_("respect format colors")),
40 - OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"),
41 - N_("field name to sort on"), &parse_opt_ref_sorting),
40 + OPT_REF_SORT(sorting_tail),
41 OPT_CALLBACK(0, "points-at", &filter.points_at,
42 N_("object"), N_("print only refs which points at the given object"),
43 parse_opt_object_name),
builtin/ls-remote.c
+1 -2
@@ -67,8 +67,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
67 OPT_BIT(0, "refs", &flags, N_("do not show peeled tags"), REF_NORMAL),
68 OPT_BOOL(0, "get-url", &get_url,
69 N_("take url.<base>.insteadOf into account")),
70 - OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"),
71 - N_("field name to sort on"), &parse_opt_ref_sorting),
70 + OPT_REF_SORT(sorting_tail),
71 OPT_SET_INT_F(0, "exit-code", &status,
72 N_("exit with exit code 2 if no matching refs are found"),
73 2, PARSE_OPT_NOCOMPLETE),
builtin/tag.c
+1 -2
@@ -412,8 +412,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
412 OPT_WITHOUT(&filter.no_commit, N_("print only tags that don't contain the commit")),
413 OPT_MERGED(&filter, N_("print only tags that are merged")),
414 OPT_NO_MERGED(&filter, N_("print only tags that are not merged")),
415 - OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"),
416 - N_("field name to sort on"), &parse_opt_ref_sorting),
415 + OPT_REF_SORT(sorting_tail),
416 {
417 OPTION_CALLBACK, 0, "points-at", &filter.points_at, N_("object"),
418 N_("print only tags of the object"), PARSE_OPT_LASTARG_DEFAULT,
ref-filter.c
+7 -2
@@ -2337,8 +2337,13 @@ void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *arg)
2337
2338 int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset)
2339 {
2340 - if (!arg) /* should --no-sort void the list ? */
2341 - return -1;
2340 + /*
2341 + * NEEDSWORK: We should probably clear the list in this case, but we've
2342 + * already munged the global used_atoms list, which would need to be
2343 + * undone.
2344 + */
2345 + BUG_ON_OPT_NEG(unset);
2346 +
2347 parse_ref_sorting(opt->value, arg);
2348 return 0;
2349 }
ref-filter.h
+5
@@ -96,6 +96,11 @@ struct ref_format {
96 #define OPT_MERGED(f, h) _OPT_MERGED_NO_MERGED("merged", f, h)
97 #define OPT_NO_MERGED(f, h) _OPT_MERGED_NO_MERGED("no-merged", f, h)
98
99 +#define OPT_REF_SORT(var) \
100 + OPT_CALLBACK_F(0, "sort", (var), \
101 + N_("key"), N_("field name to sort on"), \
102 + PARSE_OPT_NONEG, parse_opt_ref_sorting)
103 +
104 /*
105 * API for filtering a set of refs. Based on the type of refs the user
106 * has requested, we iterate through those refs and apply filters