ref-filter: make "--contains <id>" less chatty if <id> is invalid

Some git commands which use --contains <id> print the whole
help text if <id> is invalid. It should only show the error
message instead.

This patch applies to "git tag", "git branch", "git for-each-ref".

This bug was a side effect of looking up the commit in option
parser callback. When a error occurs in the option parser, the
full usage is shown. To fix this bug, the part related to
looking up the commit was moved outside of the option parser
to the ref-filter module.

Basically, the option parser only parses strings that represent
commits and the ref-filter performs the commit look-up. If an
error occurs during the option parsing, then it must be an invalid
argument and the user should be informed of usage, but if a error
occurs during ref-filtering, then it is a problem with the
argument.

Signed-off-by: Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/branch.c b/builtin/branch.c
index 8dcc2ed058be6e653f885e48c9aa5f465a5f9749..43442c12e0499e266e68612e43d93172699a355c 100644 (file)
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -596,10 +596,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
                OPT__COLOR(&branch_use_color, N_("use colored output")),
                OPT_SET_INT('r', "remotes",     &filter.kind, N_("act on remote-tracking branches"),
                        FILTER_REFS_REMOTES),
-               OPT_CONTAINS(&filter.with_commit, N_("print only branches that contain the commit")),
-               OPT_NO_CONTAINS(&filter.no_commit, N_("print only branches that don't contain the commit")),
-               OPT_WITH(&filter.with_commit, N_("print only branches that contain the commit")),
-               OPT_WITHOUT(&filter.no_commit, N_("print only branches that don't contain the commit")),
+               OPT_CONTAINS(&filter.with_commit_strs, N_("print only branches that contain the commit")),
+               OPT_NO_CONTAINS(&filter.no_commit_strs, N_("print only branches that don't contain the commit")),
+               OPT_WITH(&filter.with_commit_strs, N_("print only branches that contain the commit")),
+               OPT_WITHOUT(&filter.no_commit_strs, N_("print only branches that don't contain the commit")),
                OPT__ABBREV(&filter.abbrev),
 
                OPT_GROUP(N_("Specific git-branch actions:")),
@@ -657,8 +657,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
        if (!delete && !rename && !copy && !edit_description && !new_upstream && !unset_upstream && argc == 0)
                list = 1;
 
-       if (filter.with_commit || filter.merge != REF_FILTER_MERGED_NONE || filter.points_at.nr ||
-           filter.no_commit)
+       if (filter.with_commit_strs.nr || filter.merge != REF_FILTER_MERGED_NONE || filter.points_at.nr ||
+           filter.no_commit_strs.nr)
                list = 1;
 
        if (!!delete + !!rename + !!copy + !!new_upstream +

diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index e931be9ce4d9f180de23d09d227e5a2f571f4f1a..deb9a779a80c6b1647c244ba567dbd3129cda4d9 100644 (file)
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -44,8 +44,8 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
                             parse_opt_object_name),
                OPT_MERGED(&filter, N_("print only refs that are merged")),
                OPT_NO_MERGED(&filter, N_("print only refs that are not merged")),
-               OPT_CONTAINS(&filter.with_commit, N_("print only refs which contain the commit")),
-               OPT_NO_CONTAINS(&filter.no_commit, N_("print only refs which don't contain the commit")),
+               OPT_CONTAINS(&filter.with_commit_strs, N_("print only refs which contain the commit")),
+               OPT_NO_CONTAINS(&filter.no_commit_strs, N_("print only refs which don't contain the commit")),
                OPT_BOOL(0, "ignore-case", &icase, N_("sorting and filtering are case insensitive")),
                OPT_END(),
        };

diff --git a/builtin/tag.c b/builtin/tag.c
index 8885e21ddc81e4d2686aaeb6fe4c1bbffc916ec1..6be7f53ae8d75424d990e005c98fa793a5025bad 100644 (file)
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -396,10 +396,10 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 
                OPT_GROUP(N_("Tag listing options")),
                OPT_COLUMN(0, "column", &colopts, N_("show tag list in columns")),
-               OPT_CONTAINS(&filter.with_commit, N_("print only tags that contain the commit")),
-               OPT_NO_CONTAINS(&filter.no_commit, N_("print only tags that don't contain the commit")),
-               OPT_WITH(&filter.with_commit, N_("print only tags that contain the commit")),
-               OPT_WITHOUT(&filter.no_commit, N_("print only tags that don't contain the commit")),
+               OPT_CONTAINS(&filter.with_commit_strs, N_("print only tags that contain the commit")),
+               OPT_NO_CONTAINS(&filter.no_commit_strs, N_("print only tags that don't contain the commit")),
+               OPT_WITH(&filter.with_commit_strs, N_("print only tags that contain the commit")),
+               OPT_WITHOUT(&filter.no_commit_strs, 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"),
@@ -435,8 +435,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
        if (!cmdmode) {
                if (argc == 0)
                        cmdmode = 'l';
-               else if (filter.with_commit || filter.no_commit ||
-                        filter.points_at.nr || filter.merge_commit ||
+               else if (filter.points_at.nr || filter.merge_commit ||
+                        filter.with_commit_strs.nr || filter.no_commit_strs.nr ||
                         filter.lines != -1)
                        cmdmode = 'l';
        }
@@ -473,9 +473,9 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
        }
        if (filter.lines != -1)
                die(_("-n option is only allowed in list mode"));
-       if (filter.with_commit)
+       if (filter.with_commit_strs.nr)
                die(_("--contains option is only allowed in list mode"));
-       if (filter.no_commit)
+       if (filter.no_commit_strs.nr)
                die(_("--no-contains option is only allowed in list mode"));
        if (filter.points_at.nr)
                die(_("--points-at option is only allowed in list mode"));

diff --git a/parse-options.h b/parse-options.h
index af711227ae3aac7af07de0322a364ccac03b819d..4b4734f2e7e4e67a4ef1f1cf59655b7e7b2d7705 100644 (file)
--- a/parse-options.h
+++ b/parse-options.h
@@ -256,8 +256,9 @@ extern int parse_opt_passthru_argv(const struct option *, const char *, int);
 #define _OPT_CONTAINS_OR_WITH(name, variable, help, flag) \
        { OPTION_CALLBACK, 0, name, (variable), N_("commit"), (help), \
          PARSE_OPT_LASTARG_DEFAULT | flag, \
-         parse_opt_commits, (intptr_t) "HEAD" \
+         parse_opt_string_list, (intptr_t) "HEAD" \
        }
+
 #define OPT_CONTAINS(v, h) _OPT_CONTAINS_OR_WITH("contains", v, h, PARSE_OPT_NONEG)
 #define OPT_NO_CONTAINS(v, h) _OPT_CONTAINS_OR_WITH("no-contains", v, h, PARSE_OPT_NONEG)
 #define OPT_WITH(v, h) _OPT_CONTAINS_OR_WITH("with", v, h, PARSE_OPT_HIDDEN | PARSE_OPT_NONEG)

diff --git a/ref-filter.c b/ref-filter.c
index f9e25aea7a97e18b5723c8fa379da859d6dc9fcf..aa282a27f4d996ea2c6b9e7d7974314258e773a2 100644 (file)
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2000,6 +2000,25 @@ static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata)
        free(to_clear);
 }
 
+int add_str_to_commit_list(struct string_list_item *item, void *commit_list)
+{
+       struct object_id oid;
+       struct commit *commit;
+
+       if (get_oid(item->string, &oid)) {
+               error(_("malformed object name %s"), item->string);
+               exit(1);
+       }
+       commit = lookup_commit_reference(&oid);
+       if (!commit) {
+               error(_("no such commit %s"), item->string);
+               exit(1);
+       }
+       commit_list_insert(commit, commit_list);
+
+       return 0;
+}
+
 /*
  * 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
@@ -2012,6 +2031,10 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int
        int ret = 0;
        unsigned int broken = 0;
 
+       /* Convert string representation and add to commit list. */
+       for_each_string_list(&filter->with_commit_strs, add_str_to_commit_list, &filter->with_commit);
+       for_each_string_list(&filter->no_commit_strs, add_str_to_commit_list, &filter->no_commit);
+
        ref_cbdata.array = array;
        ref_cbdata.filter = filter;
 

diff --git a/ref-filter.h b/ref-filter.h
index 0d98342b343196387c0f4e2dcd5978a9361d8edb..62f37760fc33e30befb76deb262323e8e474ec8d 100644 (file)
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -55,6 +55,9 @@ struct ref_filter {
        struct commit_list *with_commit;
        struct commit_list *no_commit;
 
+       struct string_list with_commit_strs;
+       struct string_list no_commit_strs;
+
        enum {
                REF_FILTER_MERGED_NONE = 0,
                REF_FILTER_MERGED_INCLUDE,

diff --git a/t/tcontains.sh b/t/tcontains.sh
new file mode 100755 (executable)
index 0000000..4856111
--- /dev/null
+++ b/t/tcontains.sh
@@ -0,0 +1,92 @@
+#!/bin/sh
+
+test_description='Test "contains" argument behavior'
+
+. ./test-lib.sh
+
+test_expect_success 'setup ' '
+       git init . &&
+       echo "this is a test" >file &&
+       git add -A &&
+       git commit -am "tag test" &&
+       git tag "v1.0" &&
+       git tag "v1.1"
+'
+
+test_expect_success 'tag --contains <existent_tag>' '
+       git tag --contains "v1.0" >actual &&
+       grep "v1.0" actual &&
+       grep "v1.1" actual
+'
+
+test_expect_success 'tag --contains <inexistent_tag>' '
+       test_must_fail git tag --contains "notag" 2>actual &&
+       test_i18ngrep "error" actual
+'
+
+test_expect_success 'tag --no-contains <existent_tag>' '
+       git tag --no-contains "v1.1" >actual &&
+       test_line_count = 0 actual
+'
+
+test_expect_success 'tag --no-contains <inexistent_tag>' '
+       test_must_fail git tag --no-contains "notag" 2>actual &&
+       test_i18ngrep "error" actual
+'
+
+test_expect_success 'tag usage error' '
+       test_must_fail git tag --noopt 2>actual &&
+       test_i18ngrep "usage" actual
+'
+
+test_expect_success 'branch --contains <existent_commit>' '
+       git branch --contains "master" >actual &&
+       test_i18ngrep "master" actual
+'
+
+test_expect_success 'branch --contains <inexistent_commit>' '
+       test_must_fail git branch --no-contains "nocommit" 2>actual &&
+       test_i18ngrep "error" actual
+'
+
+test_expect_success 'branch --no-contains <existent_commit>' '
+       git branch --no-contains "master" >actual &&
+       test_line_count = 0 actual
+'
+
+test_expect_success 'branch --no-contains <inexistent_commit>' '
+       test_must_fail git branch --no-contains "nocommit" 2>actual &&
+       test_i18ngrep "error" actual
+'
+
+test_expect_success 'branch usage error' '
+       test_must_fail git branch --noopt 2>actual &&
+       test_i18ngrep "usage" actual
+'
+
+test_expect_success 'for-each-ref --contains <existent_object>' '
+       git for-each-ref --contains "master" >actual &&
+       test_line_count = 3 actual
+'
+
+test_expect_success 'for-each-ref --contains <inexistent_object>' '
+       test_must_fail git for-each-ref --no-contains "noobject" 2>actual &&
+       test_i18ngrep "error" actual
+'
+
+test_expect_success 'for-each-ref --no-contains <existent_object>' '
+       git for-each-ref --no-contains "master" >actual &&
+       test_line_count = 0 actual
+'
+
+test_expect_success 'for-each-ref --no-contains <inexistent_object>' '
+       test_must_fail git for-each-ref --no-contains "noobject" 2>actual &&
+       test_i18ngrep "error" actual
+'
+
+test_expect_success 'for-each-ref usage error' '
+       test_must_fail git for-each-ref --noopt 2>actual &&
+       test_i18ngrep "usage" actual
+'
+
+test_done