revparse: Deploy EINVALIDSPEC usage

diff --git a/include/git2/revparse.h b/include/git2/revparse.h
index 4567027e516b26c1950ec121741dc73256f09baa..3df6fef7f8d4cf4a4366f4a4ccdf2e5f3562692f 100644 (file)
--- a/include/git2/revparse.h
+++ b/include/git2/revparse.h
@@ -27,7 +27,8 @@ GIT_BEGIN_DECL
  * @param out pointer to output object
  * @param repo the repository to search in
  * @param spec the textual specification for an object
- * @return  on success, GIT_ERROR otherwise (use git_error_last for information about the error)
+ * @return 0 on success, GIT_ENOTFOUND, GIT_EAMBIGUOUS,
+ * GIT_EINVALIDSPEC or an error code
  */
 GIT_EXTERN(int) git_revparse_single(git_object **out, git_repository *repo, const char *spec);
 

diff --git a/src/common.h b/src/common.h
index a35239e3d1561d49a0f28420ffe63c7269319e94..b3b5515082e8d7ec3d9e2a9615948c78d8584a1c 100644 (file)
--- a/src/common.h
+++ b/src/common.h
@@ -61,9 +61,9 @@ void giterr_set(int error_class, const char *string, ...);
 
 /**
  * Set the error message for a regex failure, using the internal regex
- * error code lookup.
+ * error code lookup and return a libgit error code.
  */
-void giterr_set_regex(const regex_t *regex, int error_code);
+int giterr_set_regex(const regex_t *regex, int error_code);
 
 /* NOTE: other giterr functions are in the public errors.h header file */
 

diff --git a/src/errors.c b/src/errors.c
index ac7fa934d0737a5e61e8e160e254ea6b0aaca4fc..e625072168d1b564eea5346d5bad52d6fc14769d 100644 (file)
--- a/src/errors.c
+++ b/src/errors.c
@@ -93,11 +93,18 @@ void giterr_set_str(int error_class, const char *string)
                set_error(error_class, message);
 }
 
-void giterr_set_regex(const regex_t *regex, int error_code)
+int giterr_set_regex(const regex_t *regex, int error_code)
 {
        char error_buf[1024];
        regerror(error_code, regex, error_buf, sizeof(error_buf));
        giterr_set_str(GITERR_REGEX, error_buf);
+
+       if (error_code == REG_NOMATCH)
+               return GIT_ENOTFOUND;
+       else if (error_code > REG_BADPAT)
+               return GIT_EINVALIDSPEC;
+       else
+               return -1;
 }
 
 void giterr_clear(void)

diff --git a/src/revparse.c b/src/revparse.c
index 308b9292343002eb28f5aad7431d482f56e08a5e..ade03d0e4d32883716dfcb9fbc37773334abdb4c 100644 (file)
--- a/src/revparse.c
+++ b/src/revparse.c
@@ -13,12 +13,6 @@
 
 #include "git2.h"
 
-static int revspec_error(const char *revspec)
-{
-       giterr_set(GITERR_INVALID, "Failed to parse revision specifier - Invalid pattern '%s'", revspec);
-       return -1;
-}
-
 static int disambiguate_refname(git_reference **out, git_repository *repo, const char *refname)
 {
        int error, i;
@@ -51,7 +45,7 @@ static int disambiguate_refname(git_reference **out, git_repository *repo, const
                        goto cleanup;
 
                if (!git_reference_is_valid_name(git_buf_cstr(&refnamebuf))) {
-                       error = GIT_ENOTFOUND;
+                       error = GIT_EINVALIDSPEC;
                        continue;
                }
 
@@ -90,17 +84,18 @@ static int build_regex(regex_t *regex, const char *pattern)
 
        if (*pattern == '\0') {
                giterr_set(GITERR_REGEX, "Empty pattern");
-               return -1;
+               return GIT_EINVALIDSPEC;
        }
 
        error = regcomp(regex, pattern, REG_EXTENDED);
        if (!error)
                return 0;
        
-       giterr_set_regex(regex, error);
+       error = giterr_set_regex(regex, error);
+
        regfree(regex);
 
-       return -1;
+       return error;
 }
 
 static int maybe_describe(git_object**out, git_repository *repo, const char *spec)
@@ -174,7 +169,7 @@ static int try_parse_numeric(int *n, const char *curly_braces_content)
        return 0;
 }
 
-static int retrieve_previously_checked_out_branch_or_revision(git_object **out, git_reference **base_ref, git_repository *repo, const char *spec, const char *identifier, size_t position)
+static int retrieve_previously_checked_out_branch_or_revision(git_object **out, git_reference **base_ref, git_repository *repo, const char *identifier, size_t position)
 {
        git_reference *ref = NULL;
        git_reflog *reflog = NULL;
@@ -189,7 +184,7 @@ static int retrieve_previously_checked_out_branch_or_revision(git_object **out,
        cur = position;
 
        if (*identifier != '\0' || *base_ref != NULL)
-               return revspec_error(spec);
+               return GIT_EINVALIDSPEC;
 
        if (build_regex(&preg, "checkout: moving from (.*) to .*") < 0)
                return -1;
@@ -332,6 +327,11 @@ static int retrieve_remote_tracking_reference(git_reference **base_ref, const ch
                *base_ref = NULL;
        }
 
+       if (!git_reference_is_branch(ref)) {
+               error = GIT_EINVALIDSPEC;
+               goto cleanup;
+       }
+
        if ((error = git_branch_tracking(&tracking, ref)) < 0)
                goto cleanup;
        
@@ -357,13 +357,13 @@ static int handle_at_syntax(git_object **out, git_reference **ref, const char *s
        is_numeric = !try_parse_numeric(&parsed, curly_braces_content);
 
        if (*curly_braces_content == '-' && (!is_numeric || parsed == 0)) {
-               error = revspec_error(spec);
+               error = GIT_EINVALIDSPEC;
                goto cleanup;
        }
 
        if (is_numeric) {
                if (parsed < 0)
-                       error = retrieve_previously_checked_out_branch_or_revision(out, ref, repo, spec, git_buf_cstr(&identifier), -parsed);
+                       error = retrieve_previously_checked_out_branch_or_revision(out, ref, repo, git_buf_cstr(&identifier), -parsed);
                else
                        error = retrieve_revobject_from_reflog(out, ref, repo, git_buf_cstr(&identifier), parsed);
 
@@ -416,8 +416,9 @@ static int handle_caret_parent_syntax(git_object **out, git_object *obj, int n)
        git_object *temp_commit = NULL;
        int error;
 
-       if (git_object_peel(&temp_commit, obj, GIT_OBJ_COMMIT) < 0)
-               return -1;
+       if ((error = git_object_peel(&temp_commit, obj, GIT_OBJ_COMMIT)) < 0)
+               return (error == GIT_EAMBIGUOUS || error == GIT_ENOTFOUND) ?
+                       GIT_EINVALIDSPEC : error;
 
        if (n == 0) {
                *out = temp_commit;
@@ -435,8 +436,9 @@ static int handle_linear_syntax(git_object **out, git_object *obj, int n)
        git_object *temp_commit = NULL;
        int error;
 
-       if (git_object_peel(&temp_commit, obj, GIT_OBJ_COMMIT) < 0)
-               return -1;
+       if ((error = git_object_peel(&temp_commit, obj, GIT_OBJ_COMMIT)) < 0)
+               return (error == GIT_EAMBIGUOUS || error == GIT_ENOTFOUND) ?
+                       GIT_EINVALIDSPEC : error;
 
        error = git_commit_nth_gen_ancestor((git_commit **)out, (git_commit*)temp_commit, n);
 
@@ -453,8 +455,8 @@ static int handle_colon_syntax(
        int error = -1;
        git_tree_entry *entry = NULL;
 
-       if (git_object_peel(&tree, obj, GIT_OBJ_TREE) < 0)
-               return -1;
+       if ((error = git_object_peel(&tree, obj, GIT_OBJ_TREE)) < 0)
+               return error == GIT_ENOTFOUND ? GIT_EINVALIDSPEC : error;
 
        if (*path == '\0') {
                *out = tree;
@@ -507,21 +509,21 @@ static int handle_grep_syntax(git_object **out, git_repository *repo, const git_
 {
        regex_t preg;
        git_revwalk *walk = NULL;
-       int error = -1;
+       int error;
 
-       if (build_regex(&preg, pattern) < 0)
-               return -1;
+       if ((error = build_regex(&preg, pattern)) < 0)
+               return error;
                
-       if (git_revwalk_new(&walk, repo) < 0)
+       if ((error = git_revwalk_new(&walk, repo)) < 0)
                goto cleanup;
 
        git_revwalk_sorting(walk, GIT_SORT_TIME);
 
        if (spec_oid == NULL) {
                // TODO: @carlosmn: The glob should be refs/* but this makes git_revwalk_next() fails
-               if (git_revwalk_push_glob(walk, GIT_REFS_HEADS_DIR "*") < 0)
+               if ((error = git_revwalk_push_glob(walk, GIT_REFS_HEADS_DIR "*")) < 0)
                        goto cleanup;
-       } else if (git_revwalk_push(walk, spec_oid) < 0)
+       } else if ((error = git_revwalk_push(walk, spec_oid)) < 0)
                        goto cleanup;
 
        error = walk_and_search(out, walk, &preg);
@@ -546,7 +548,7 @@ static int handle_caret_curly_syntax(git_object **out, git_object *obj, const ch
        expected_type = parse_obj_type(curly_braces_content);
 
        if (expected_type == GIT_OBJ_BAD)
-               return -1;
+               return GIT_EINVALIDSPEC;
 
        return git_object_peel(out, obj, expected_type);
 }
@@ -560,13 +562,13 @@ static int extract_curly_braces_content(git_buf *buf, const char *spec, size_t *
        (*pos)++;
 
        if (spec[*pos] == '\0' || spec[*pos] != '{')
-               return revspec_error(spec);
+               return GIT_EINVALIDSPEC;
 
        (*pos)++;
 
        while (spec[*pos] != '}') {
                if (spec[*pos] == '\0')
-                       return revspec_error(spec);
+                       return GIT_EINVALIDSPEC;
 
                git_buf_putc(buf, spec[(*pos)++]);
        }
@@ -610,7 +612,7 @@ static int extract_how_many(int *n, const char *spec, size_t *pos)
 
                if (git__isdigit(spec[*pos])) {
                        if ((git__strtol32(&parsed, spec + *pos, &end_ptr, 10) < 0) < 0)
-                               return revspec_error(spec);
+                               return GIT_EINVALIDSPEC;
 
                        accumulated += (parsed - 1);
                        *pos = end_ptr - spec;
@@ -655,7 +657,7 @@ static int ensure_base_rev_loaded(git_object **object, git_reference **reference
        }
 
        if (!allow_empty_identifier && identifier_len == 0)
-               return revspec_error(spec);
+               return GIT_EINVALIDSPEC;
 
        if (git_buf_put(&identifier, spec, identifier_len) < 0)
                return -1;
@@ -666,12 +668,12 @@ static int ensure_base_rev_loaded(git_object **object, git_reference **reference
        return error;
 }
 
-static int ensure_base_rev_is_not_known_yet(git_object *object, const char *spec)
+static int ensure_base_rev_is_not_known_yet(git_object *object)
 {
        if (object == NULL)
                return 0;
 
-       return revspec_error(spec);
+       return GIT_EINVALIDSPEC;
 }
 
 static bool any_left_hand_identifier(git_object *object, git_reference *reference, size_t identifier_len)
@@ -688,12 +690,12 @@ static bool any_left_hand_identifier(git_object *object, git_reference *referenc
        return false;
 }
 
-static int ensure_left_hand_identifier_is_not_known_yet(git_object *object, git_reference *reference, const char *spec)
+static int ensure_left_hand_identifier_is_not_known_yet(git_object *object, git_reference *reference)
 {
-       if (!ensure_base_rev_is_not_known_yet(object, spec) && reference == NULL)
+       if (!ensure_base_rev_is_not_known_yet(object) && reference == NULL)
                return 0;
 
-       return revspec_error(spec);
+       return GIT_EINVALIDSPEC;
 }
 
 int git_revparse_single(git_object **out, git_repository *repo, const char *spec)
@@ -800,7 +802,7 @@ int git_revparse_single(git_object **out, git_repository *repo, const char *spec
                                if ((error = extract_curly_braces_content(&buf, spec, &pos)) < 0)
                                        goto cleanup;
 
-                               if ((error = ensure_base_rev_is_not_known_yet(base_rev, spec)) < 0)
+                               if ((error = ensure_base_rev_is_not_known_yet(base_rev)) < 0)
                                        goto cleanup;
 
                                if ((error = handle_at_syntax(&temp_object, &reference, spec, identifier_len, repo, git_buf_cstr(&buf))) < 0)
@@ -815,7 +817,7 @@ int git_revparse_single(git_object **out, git_repository *repo, const char *spec
                }
 
                default:
-                       if ((error = ensure_left_hand_identifier_is_not_known_yet(base_rev, reference, spec)) < 0)
+                       if ((error = ensure_left_hand_identifier_is_not_known_yet(base_rev, reference)) < 0)
                                goto cleanup;
 
                        pos++;
@@ -830,8 +832,13 @@ int git_revparse_single(git_object **out, git_repository *repo, const char *spec
        error = 0;
 
 cleanup:
-       if (error)
+       if (error) {
+               if (error == GIT_EINVALIDSPEC)
+                       giterr_set(GITERR_INVALID,
+                               "Failed to parse revision specifier - Invalid pattern '%s'", spec);
+
                git_object_free(base_rev);
+       }
        git_reference_free(reference);
        git_buf_free(&buf);
        return error;

diff --git a/tests-clar/refs/revparse.c b/tests-clar/refs/revparse.c
index 3fb5c3f72dcbdee32a1c5d744aa8935c44d671df..81a6bc469fed1064346809e3aabc7cca4538f582 100644 (file)
--- a/tests-clar/refs/revparse.c
+++ b/tests-clar/refs/revparse.c
@@ -49,13 +49,17 @@ void test_refs_revparse__nonexistant_object(void)
        test_object("this-does-not-exist~2", NULL);
 }
 
-void test_refs_revparse__invalid_reference_name(void)
+static void assert_invalid_spec(const char *invalid_spec)
 {
-       cl_git_fail(git_revparse_single(&g_obj, g_repo, "this doesn't make sense"));
-       cl_git_fail(git_revparse_single(&g_obj, g_repo, "this doesn't make sense^1"));
-       cl_git_fail(git_revparse_single(&g_obj, g_repo, "this doesn't make sense~2"));
-       cl_git_fail(git_revparse_single(&g_obj, g_repo, ""));
+       cl_assert_equal_i(
+               GIT_EINVALIDSPEC, git_revparse_single(&g_obj, g_repo, invalid_spec));
+}
 
+void test_refs_revparse__invalid_reference_name(void)
+{
+       assert_invalid_spec("this doesn't make sense");
+       assert_invalid_spec("Inv@{id");
+       assert_invalid_spec("");
 }
 
 void test_refs_revparse__shas(void)
@@ -94,9 +98,11 @@ void test_refs_revparse__describe_output(void)
 
 void test_refs_revparse__nth_parent(void)
 {
-       cl_git_fail(git_revparse_single(&g_obj, g_repo, "be3563a^-1"));
-       cl_git_fail(git_revparse_single(&g_obj, g_repo, "^"));
-       cl_git_fail(git_revparse_single(&g_obj, g_repo, "be3563a^{tree}^"));
+       assert_invalid_spec("be3563a^-1");
+       assert_invalid_spec("^");
+       assert_invalid_spec("be3563a^{tree}^");
+       assert_invalid_spec("point_to_blob^{blob}^");
+       assert_invalid_spec("this doesn't make sense^1");
 
        test_object("be3563a^1", "9fd738e8f7967c078dceed8190330fc8648ee56a");
        test_object("be3563a^", "9fd738e8f7967c078dceed8190330fc8648ee56a");
@@ -123,8 +129,10 @@ void test_refs_revparse__not_tag(void)
 
 void test_refs_revparse__to_type(void)
 {
-       cl_git_fail(git_revparse_single(&g_obj, g_repo, "wrapped_tag^{blob}"));
-       cl_git_fail(git_revparse_single(&g_obj, g_repo, "wrapped_tag^{trip}"));
+       assert_invalid_spec("wrapped_tag^{trip}");
+       test_object("point_to_blob^{commit}", NULL);
+       cl_assert_equal_i(
+               GIT_EAMBIGUOUS, git_revparse_single(&g_obj, g_repo, "wrapped_tag^{blob}"));
 
        test_object("wrapped_tag^{commit}", "a65fedf39aefe402d3bb6e24df4d4f5fe4547750");
        test_object("wrapped_tag^{tree}", "944c0f6e4dfa41595e6eb3ceecdb14f50fe18162");
@@ -134,11 +142,15 @@ void test_refs_revparse__to_type(void)
 
 void test_refs_revparse__linear_history(void)
 {
-       cl_git_fail(git_revparse_single(&g_obj, g_repo, "~"));
-       cl_git_fail(git_revparse_single(&g_obj, g_repo, "foo~bar"));
-       cl_git_fail(git_revparse_single(&g_obj, g_repo, "master~bar"));
-       cl_git_fail(git_revparse_single(&g_obj, g_repo, "master~-1"));
-       cl_git_fail(git_revparse_single(&g_obj, g_repo, "master~0bar"));
+       assert_invalid_spec("~");
+       test_object("foo~bar", NULL);
+
+       assert_invalid_spec("master~bar");
+       assert_invalid_spec("master~-1");
+       assert_invalid_spec("master~0bar");
+       assert_invalid_spec("this doesn't make sense~2");
+       assert_invalid_spec("be3563a^{tree}~");
+       assert_invalid_spec("point_to_blob^{blob}~");
 
        test_object("master~0", "a65fedf39aefe402d3bb6e24df4d4f5fe4547750");
        test_object("master~1", "be3563ae3f795b2b4353bcce3a527ad0a4f7f644");
@@ -149,10 +161,10 @@ void test_refs_revparse__linear_history(void)
 
 void test_refs_revparse__chaining(void)
 {
-       cl_git_fail(git_revparse_single(&g_obj, g_repo, "master@{0}@{0}"));
-       cl_git_fail(git_revparse_single(&g_obj, g_repo, "@{u}@{-1}"));
-       cl_git_fail(git_revparse_single(&g_obj, g_repo, "@{-1}@{-1}"));
-       cl_git_fail(git_revparse_single(&g_obj, g_repo, "@{-3}@{0}"));
+       assert_invalid_spec("master@{0}@{0}");
+       assert_invalid_spec("@{u}@{-1}");
+       assert_invalid_spec("@{-1}@{-1}");
+       assert_invalid_spec("@{-3}@{0}");
 
        test_object("master@{0}~1^1", "9fd738e8f7967c078dceed8190330fc8648ee56a");
        test_object("@{u}@{0}", "be3563ae3f795b2b4353bcce3a527ad0a4f7f644");
@@ -168,8 +180,9 @@ void test_refs_revparse__chaining(void)
 
 void test_refs_revparse__upstream(void)
 {
-       cl_git_fail(git_revparse_single(&g_obj, g_repo, "e90810b@{u}"));
-       cl_git_fail(git_revparse_single(&g_obj, g_repo, "refs/tags/e90810b@{u}"));
+       assert_invalid_spec("e90810b@{u}");
+       assert_invalid_spec("refs/tags/e90810b@{u}");
+       test_object("refs/heads/e90810b@{u}", NULL);
 
        test_object("master@{upstream}", "be3563ae3f795b2b4353bcce3a527ad0a4f7f644");
        test_object("@{u}", "be3563ae3f795b2b4353bcce3a527ad0a4f7f644");
@@ -180,7 +193,7 @@ void test_refs_revparse__upstream(void)
 
 void test_refs_revparse__ordinal(void)
 {
-       cl_git_fail(git_revparse_single(&g_obj, g_repo, "master@{-2}"));
+       assert_invalid_spec("master@{-2}");
        
        /* TODO: make the test below actually fail
         * cl_git_fail(git_revparse_single(&g_obj, g_repo, "master@{1a}"));
@@ -202,9 +215,9 @@ void test_refs_revparse__ordinal(void)
 
 void test_refs_revparse__previous_head(void)
 {
-       cl_git_fail(git_revparse_single(&g_obj, g_repo, "@{-xyz}"));
-       cl_git_fail(git_revparse_single(&g_obj, g_repo, "@{-0}"));
-       cl_git_fail(git_revparse_single(&g_obj, g_repo, "@{-1b}"));
+       assert_invalid_spec("@{-xyz}");
+       assert_invalid_spec("@{-0}");
+       assert_invalid_spec("@{-1b}");
 
        test_object("@{-42}", NULL);
 
@@ -261,9 +274,9 @@ void test_refs_revparse__reflog_of_a_ref_under_refs(void)
 
 void test_refs_revparse__revwalk(void)
 {
-       cl_git_fail(git_revparse_single(&g_obj, g_repo, "master^{/not found in any commit}"));
-       cl_git_fail(git_revparse_single(&g_obj, g_repo, "master^{/merge}"));
-       cl_git_fail(git_revparse_single(&g_obj, g_repo, "master^{/((}"));
+       test_object("master^{/not found in any commit}", NULL);
+       test_object("master^{/merge}", NULL);
+       assert_invalid_spec("master^{/((}");
 
        test_object("master^{/anoth}", "5b5b025afb0b4c913b4c338a42934a3863bf3644");
        test_object("master^{/Merge}", "be3563ae3f795b2b4353bcce3a527ad0a4f7f644");
@@ -344,8 +357,9 @@ void test_refs_revparse__date(void)
 
 void test_refs_revparse__colon(void)
 {
-       cl_git_fail(git_revparse_single(&g_obj, g_repo, ":/"));
-       cl_git_fail(git_revparse_single(&g_obj, g_repo, ":2:README"));
+       assert_invalid_spec(":/");
+       assert_invalid_spec("point_to_blob:readme.txt");
+       cl_git_fail(git_revparse_single(&g_obj, g_repo, ":2:README")); /* Not implemented  */
 
        test_object(":/not found in any commit", NULL);
        test_object("subtrees:ab/42.txt", NULL);
@@ -435,11 +449,8 @@ void test_refs_revparse__disambiguation(void)
 
 void test_refs_revparse__a_too_short_objectid_returns_EAMBIGUOUS(void)
 {
-       int result;
-       
-       result = git_revparse_single(&g_obj, g_repo, "e90");
-       
-       cl_assert_equal_i(GIT_EAMBIGUOUS, result);
+       cl_assert_equal_i(
+               GIT_EAMBIGUOUS, git_revparse_single(&g_obj, g_repo, "e90"));
 }
 
 void test_refs_revparse__issue_994(void)