From 3e199f428513fbf04b126d536409deae1c6d045f Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Wed, 1 May 2013 04:18:46 -0700 Subject: [PATCH 01/22] Set error message for branch functions There were a couple of places where an error was being returned from branch related code but no error message was being set. --- src/branch.c | 14 ++++++++++++-- tests-clar/refs/branches/remote.c | 6 ++++++ 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/src/branch.c b/src/branch.c index 88f052529..ab661f422 100644 --- a/src/branch.c +++ b/src/branch.c @@ -185,7 +185,7 @@ int git_branch_move( git_buf_cstr(&old_config_section), git_buf_cstr(&new_config_section))) < 0) goto done; - + if ((error = git_reference_rename(out, branch, git_buf_cstr(&new_reference_name), force)) < 0) goto done; @@ -276,6 +276,8 @@ int git_branch_upstream__name( goto cleanup; if (!*remote_name || !*merge_name) { + giterr_set(GITERR_REFERENCE, + "branch '%s' does not have an upstream", canonical_branch_name); error = GIT_ENOTFOUND; goto cleanup; } @@ -342,6 +344,9 @@ static int remote_name(git_buf *buf, git_repository *repo, const char *canonical remote_name = remote_list.strings[i]; } else { git_remote_free(remote); + + giterr_set(GITERR_REFERENCE, + "Reference '%s' is ambiguous", canonical_branch_name); error = GIT_EAMBIGUOUS; goto cleanup; } @@ -354,6 +359,8 @@ static int remote_name(git_buf *buf, git_repository *repo, const char *canonical git_buf_clear(buf); error = git_buf_puts(buf, remote_name); } else { + giterr_set(GITERR_REFERENCE, + "Could not determine remote for '%s'", canonical_branch_name); error = GIT_ENOTFOUND; } @@ -490,8 +497,11 @@ int git_branch_set_upstream(git_reference *branch, const char *upstream_name) local = 1; else if (git_branch_lookup(&upstream, repo, upstream_name, GIT_BRANCH_REMOTE) == 0) local = 0; - else + else { + giterr_set(GITERR_REFERENCE, + "Cannot set upstream for branch '%s'", shortname); return GIT_ENOTFOUND; + } /* * If it's local, the remote is "." and the branch name is diff --git a/tests-clar/refs/branches/remote.c b/tests-clar/refs/branches/remote.c index 6043828b3..c110adb33 100644 --- a/tests-clar/refs/branches/remote.c +++ b/tests-clar/refs/branches/remote.c @@ -49,16 +49,20 @@ void test_refs_branches_remote__no_matching_remote_returns_error(void) { const char *unknown = "refs/remotes/nonexistent/master"; + giterr_clear(); cl_git_fail_with(git_branch_remote_name( NULL, 0, g_repo, unknown), GIT_ENOTFOUND); + cl_assert(giterr_last() != NULL); } void test_refs_branches_remote__local_remote_returns_error(void) { const char *local = "refs/heads/master"; + giterr_clear(); cl_git_fail_with(git_branch_remote_name( NULL, 0, g_repo, local), GIT_ERROR); + cl_assert(giterr_last() != NULL); } void test_refs_branches_remote__ambiguous_remote_returns_error(void) @@ -75,6 +79,8 @@ void test_refs_branches_remote__ambiguous_remote_returns_error(void) git_remote_free(remote); + giterr_clear(); cl_git_fail_with(git_branch_remote_name(NULL, 0, g_repo, remote_tracking_branch_name), GIT_EAMBIGUOUS); + cl_assert(giterr_last() != NULL); } From f6f48f9008a9aaba5cbcfc611b616d8ae332b5e3 Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Wed, 1 May 2013 04:57:05 -0700 Subject: [PATCH 02/22] Simplify error reporting --- src/clone.c | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/src/clone.c b/src/clone.c index aeb7bbf5c..a19788699 100644 --- a/src/clone.c +++ b/src/clone.c @@ -387,18 +387,6 @@ static int setup_remotes_and_fetch( } -static bool path_is_okay(const char *path) -{ - /* The path must either not exist, or be an empty directory */ - if (!git_path_exists(path)) return true; - if (!git_path_is_empty_dir(path)) { - giterr_set(GITERR_INVALID, - "'%s' exists and is not an empty directory", path); - return false; - } - return true; -} - static bool should_checkout( git_repository *repo, bool is_bare, @@ -444,7 +432,10 @@ int git_clone( normalize_options(&normOptions, options); GITERR_CHECK_VERSION(&normOptions, GIT_CLONE_OPTIONS_VERSION, "git_clone_options"); - if (!path_is_okay(local_path)) { + /* Only clone to a new directory or an empty directory */ + if (git_path_exists(local_path) && !git_path_is_empty_dir(local_path)) { + giterr_set(GITERR_INVALID, + "'%s' exists and is not an empty directory", path); return GIT_ERROR; } From ae99f5e2ab090c1c666dfbf36753b8c18ab88478 Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Wed, 1 May 2013 04:57:24 -0700 Subject: [PATCH 03/22] Make sure error messages get set --- src/index.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/index.c b/src/index.c index 2e2d373b5..3c5173888 100644 --- a/src/index.c +++ b/src/index.c @@ -496,8 +496,10 @@ const git_index_entry *git_index_get_bypath( git_vector_sort(&index->entries); - if (index_find(&pos, index, path, stage) < 0) + if (index_find(&pos, index, path, stage) < 0) { + giterr_set(GITERR_INDEX, "Index does not contain %s", path); return NULL; + } return git_index_get_byindex(index, pos); } @@ -778,8 +780,11 @@ int git_index_remove(git_index *index, const char *path, int stage) git_vector_sort(&index->entries); - if (index_find(&position, index, path, stage) < 0) + if (index_find(&position, index, path, stage) < 0) { + giterr_set(GITERR_INDEX, "Index does not contain %s at stage %d", + path, stage); return GIT_ENOTFOUND; + } entry = git_vector_get(&index->entries, position); if (entry != NULL) From 46779411f93589fe567817fbdd61304847aa83c3 Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Wed, 1 May 2013 05:32:10 -0700 Subject: [PATCH 04/22] fix typo --- src/clone.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/clone.c b/src/clone.c index a19788699..36da9c276 100644 --- a/src/clone.c +++ b/src/clone.c @@ -435,7 +435,7 @@ int git_clone( /* Only clone to a new directory or an empty directory */ if (git_path_exists(local_path) && !git_path_is_empty_dir(local_path)) { giterr_set(GITERR_INVALID, - "'%s' exists and is not an empty directory", path); + "'%s' exists and is not an empty directory", local_path); return GIT_ERROR; } From 155ee751143a98356ec72d9930d2a82ec750e466 Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Wed, 1 May 2013 05:34:01 -0700 Subject: [PATCH 05/22] Add error messages for failed submodule lookup --- src/submodule.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/submodule.c b/src/submodule.c index 0a22e3b13..f4fbcd35a 100644 --- a/src/submodule.c +++ b/src/submodule.c @@ -128,6 +128,10 @@ int git_submodule_lookup( git_buf_free(&path); } + giterr_set(GITERR_SUBMODULE, (error == GIT_ENOTFOUND) ? + "No submodule named '%s'" : + "Submodule '%s' has not been added yet", name); + return error; } From 3f663178ed856a333fd11d8dd57231ebe331baf2 Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Wed, 1 May 2013 13:38:56 -0700 Subject: [PATCH 06/22] More care catching and setting config errors --- src/config.c | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/src/config.c b/src/config.c index 2e1268ef3..3f475ea63 100644 --- a/src/config.c +++ b/src/config.c @@ -344,6 +344,13 @@ int git_config_delete_entry(git_config *cfg, const char *name) * Setters **************/ +static int config_error_nofiles(const char *name) +{ + giterr_set(GITERR_CONFIG, + "Cannot set value for '%s' when no config files exist", name); + return GIT_ENOTFOUND; +} + int git_config_set_int64(git_config *cfg, const char *name, int64_t value) { char str_value[32]; /* All numbers should fit in here */ @@ -373,12 +380,9 @@ int git_config_set_string(git_config *cfg, const char *name, const char *value) } internal = git_vector_get(&cfg->files, 0); - if (!internal) { + if (!internal) /* Should we auto-vivify .git/config? Tricky from this location */ - giterr_set(GITERR_CONFIG, "Cannot set value when no config files exist"); - return GIT_ENOTFOUND; - } - + return config_error_nofiles(name); file = internal->file; error = file->set(file, name, value); @@ -442,6 +446,12 @@ static int get_string_at_file(const char **out, const git_config_backend *file, return res; } +static int config_error_notfound(const char *name) +{ + giterr_set(GITERR_CONFIG, "Config value '%s' was not found", name); + return GIT_ENOTFOUND; +} + static int get_string(const char **out, const git_config *cfg, const char *name) { file_internal *internal; @@ -454,7 +464,7 @@ static int get_string(const char **out, const git_config *cfg, const char *name) return res; } - return GIT_ENOTFOUND; + return config_error_notfound(name); } int git_config_get_bool(int *out, const git_config *cfg, const char *name) @@ -494,7 +504,7 @@ int git_config_get_entry(const git_config_entry **out, const git_config *cfg, co return ret; } - return GIT_ENOTFOUND; + return config_error_notfound(name); } int git_config_get_multivar(const git_config *cfg, const char *name, const char *regexp, @@ -517,7 +527,7 @@ int git_config_get_multivar(const git_config *cfg, const char *name, const char return ret; } - return 0; + return (ret == GIT_ENOTFOUND) ? config_error_notfound(name) : 0; } int git_config_set_multivar(git_config *cfg, const char *name, const char *regexp, const char *value) @@ -526,6 +536,8 @@ int git_config_set_multivar(git_config *cfg, const char *name, const char *regex file_internal *internal; internal = git_vector_get(&cfg->files, 0); + if (!internal) + return config_error_nofiles(name); file = internal->file; return file->set_multivar(file, name, regexp, value); From 52c102b7f679674b14d046a62091f76431d3eb1b Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Wed, 1 May 2013 13:43:48 -0700 Subject: [PATCH 07/22] More care reporting diff patch iteration errors --- src/diff_output.c | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/src/diff_output.c b/src/diff_output.c index 64ff6b5be..bac8622c8 100644 --- a/src/diff_output.c +++ b/src/diff_output.c @@ -1611,6 +1611,12 @@ int git_diff_patch_line_stats( return 0; } +static int diff_error_outofrange(const char *thing) +{ + giterr_set(GITERR_INVALID, "Diff patch %s index out of range", thing); + return GIT_ENOTFOUND; +} + int git_diff_patch_get_hunk( const git_diff_range **range, const char **header, @@ -1628,7 +1634,8 @@ int git_diff_patch_get_hunk( if (header) *header = NULL; if (header_len) *header_len = 0; if (lines_in_hunk) *lines_in_hunk = 0; - return GIT_ENOTFOUND; + + return diff_error_outofrange("hunk"); } hunk = &patch->hunks[hunk_idx]; @@ -1648,7 +1655,7 @@ int git_diff_patch_num_lines_in_hunk( assert(patch); if (hunk_idx >= patch->hunks_size) - return GIT_ENOTFOUND; + return diff_error_outofrange("hunk"); else return (int)patch->hunks[hunk_idx].line_count; } @@ -1665,15 +1672,20 @@ int git_diff_patch_get_line_in_hunk( { diff_patch_hunk *hunk; diff_patch_line *line; + const char *thing; assert(patch); - if (hunk_idx >= patch->hunks_size) + if (hunk_idx >= patch->hunks_size) { + thing = "hunk"; goto notfound; + } hunk = &patch->hunks[hunk_idx]; - if (line_of_hunk >= hunk->line_count) + if (line_of_hunk >= hunk->line_count) { + thing = "link"; goto notfound; + } line = &patch->lines[hunk->line_start + line_of_hunk]; @@ -1692,7 +1704,7 @@ notfound: if (old_lineno) *old_lineno = -1; if (new_lineno) *new_lineno = -1; - return GIT_ENOTFOUND; + return diff_error_outofrange(thing); } static int print_to_buffer_cb( From e830c020f3915e272cf0810899f40dd5c84fbbf6 Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Wed, 1 May 2013 13:50:39 -0700 Subject: [PATCH 08/22] Report stat error when checking if file modified --- src/fileops.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/fileops.c b/src/fileops.c index d6244711f..cc55207e0 100644 --- a/src/fileops.c +++ b/src/fileops.c @@ -988,8 +988,10 @@ int git_futils_filestamp_check( if (stamp == NULL) return 1; - if (p_stat(path, &st) < 0) + if (p_stat(path, &st) < 0) { + giterr_set(GITERR_OS, "Could not stat '%s'", path); return GIT_ENOTFOUND; + } if (stamp->mtime == (git_time_t)st.st_mtime && stamp->size == (git_off_t)st.st_size && From de19c4a9582b74eec1d510d4ded5889d3b3e7976 Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Wed, 1 May 2013 14:00:20 -0700 Subject: [PATCH 09/22] Set error when no merge base is found --- src/merge.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/merge.c b/src/merge.c index e0010d6a4..48ff80cd4 100644 --- a/src/merge.c +++ b/src/merge.c @@ -86,6 +86,7 @@ int git_merge_base_many(git_oid *out, git_repository *repo, const git_oid input_ goto cleanup; if (!result) { + giterr_set(GITERR_MERGE, "No merge base found"); error = GIT_ENOTFOUND; goto cleanup; } @@ -131,7 +132,7 @@ int git_merge_base(git_oid *out, git_repository *repo, const git_oid *one, const if (!result) { git_revwalk_free(walk); - giterr_clear(); + giterr_set(GITERR_MERGE, "No merge base found"); return GIT_ENOTFOUND; } From 734c6fc13430f770f7e98cf5d23b7f6e470c29ef Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Wed, 1 May 2013 14:15:55 -0700 Subject: [PATCH 10/22] Report errors finding notes --- src/notes.c | 41 ++++++++++++++++++++++------------------- 1 file changed, 22 insertions(+), 19 deletions(-) diff --git a/src/notes.c b/src/notes.c index ef48ac88e..4ca2aaa63 100644 --- a/src/notes.c +++ b/src/notes.c @@ -13,6 +13,12 @@ #include "iterator.h" #include "signature.h" +static int note_error_notfound(void) +{ + giterr_set(GITERR_INVALID, "Note could not be found"); + return GIT_ENOTFOUND; +} + static int find_subtree_in_current_level( git_tree **out, git_repository *repo, @@ -26,7 +32,7 @@ static int find_subtree_in_current_level( *out = NULL; if (parent == NULL) - return GIT_ENOTFOUND; + return note_error_notfound(); for (i = 0; i < git_tree_entrycount(parent); i++) { entry = git_tree_entry_byindex(parent, i); @@ -44,7 +50,7 @@ static int find_subtree_in_current_level( return GIT_EEXISTS; } - return GIT_ENOTFOUND; + return note_error_notfound(); } static int find_subtree_r(git_tree **out, git_tree *root, @@ -56,9 +62,8 @@ static int find_subtree_r(git_tree **out, git_tree *root, *out = NULL; error = find_subtree_in_current_level(&subtree, repo, root, target, *fanout); - if (error == GIT_EEXISTS) { + if (error == GIT_EEXISTS) return git_tree_lookup(out, repo, git_tree_id(root)); - } if (error < 0) return error; @@ -85,7 +90,8 @@ static int find_blob(git_oid *blob, git_tree *tree, const char *target) return 0; } } - return GIT_ENOTFOUND; + + return note_error_notfound(); } static int tree_write( @@ -316,8 +322,8 @@ static int note_new(git_note **out, git_oid *note_oid, git_blob *blob) return 0; } -static int note_lookup(git_note **out, git_repository *repo, - git_tree *tree, const char *target) +static int note_lookup( + git_note **out, git_repository *repo, git_tree *tree, const char *target) { int error, fanout = 0; git_oid oid; @@ -382,6 +388,7 @@ static int note_get_default_ref(const char **out, git_repository *repo) ret = git_config_get_string(out, cfg, "core.notesRef"); if (ret == GIT_ENOTFOUND) { + giterr_clear(); *out = GIT_NOTES_DEFAULT_REF; return 0; } @@ -432,12 +439,10 @@ int git_note_read(git_note **out, git_repository *repo, target = git_oid_allocfmt(oid); GITERR_CHECK_ALLOC(target); - if ((error = retrieve_note_tree_and_commit(&tree, &commit, repo, ¬es_ref)) < 0) - goto cleanup; + if (!(error = retrieve_note_tree_and_commit( + &tree, &commit, repo, ¬es_ref))) + error = note_lookup(out, repo, tree, target); - error = note_lookup(out, repo, tree, target); - -cleanup: git__free(target); git_tree_free(tree); git_commit_free(commit); @@ -489,13 +494,11 @@ int git_note_remove(git_repository *repo, const char *notes_ref, target = git_oid_allocfmt(oid); GITERR_CHECK_ALLOC(target); - if ((error = retrieve_note_tree_and_commit(&tree, &commit, repo, ¬es_ref)) < 0) - goto cleanup; + if (!(error = retrieve_note_tree_and_commit( + &tree, &commit, repo, ¬es_ref))) + error = note_remove( + repo, author, committer, notes_ref, tree, target, &commit); - error = note_remove(repo, author, committer, notes_ref, - tree, target, &commit); - -cleanup: git__free(target); git_commit_free(commit); git_tree_free(tree); @@ -533,7 +536,7 @@ static int process_entry_path( const char* entry_path, git_oid *annotated_object_id) { - int error = -1; + int error = 0; size_t i = 0, j = 0, len; git_buf buf = GIT_BUF_INIT; From 8915a140cb996f84eeafadb99b195c33301c7d30 Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Wed, 1 May 2013 14:23:01 -0700 Subject: [PATCH 11/22] Report a couple object error conditions --- src/object.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/src/object.c b/src/object.c index b87a07404..43aecf434 100644 --- a/src/object.c +++ b/src/object.c @@ -122,8 +122,10 @@ int git_object_lookup_prefix( assert(repo && object_out && id); - if (len < GIT_OID_MINPREFIXLEN) + if (len < GIT_OID_MINPREFIXLEN) { + giterr_set(GITERR_OBJECT, "Ambiguous lookup - OID prefix is too short"); return GIT_EAMBIGUOUS; + } error = git_repository_odb__weakptr(&odb, repo); if (error < 0) @@ -311,18 +313,22 @@ int git_object_peel( git_object *source, *deref = NULL; int error; - if (target_type != GIT_OBJ_TAG && - target_type != GIT_OBJ_COMMIT && - target_type != GIT_OBJ_TREE && - target_type != GIT_OBJ_BLOB && - target_type != GIT_OBJ_ANY) - return GIT_EINVALIDSPEC; - assert(object && peeled); if (git_object_type(object) == target_type) return git_object_dup(peeled, (git_object *)object); + if (target_type != GIT_OBJ_TAG && + target_type != GIT_OBJ_COMMIT && + target_type != GIT_OBJ_TREE && + target_type != GIT_OBJ_BLOB && + target_type != GIT_OBJ_ANY) { + + giterr_set(GITERR_OBJECT, "Cannot peel to object type %d", + (int)target_type); + return GIT_EINVALIDSPEC; + } + source = (git_object *)object; while (!(error = dereference_object(&deref, source))) { From f063f578981bfb7f88880e9bdfbfd0a370dfefea Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Wed, 1 May 2013 14:48:35 -0700 Subject: [PATCH 12/22] Catch some odd odb backend corner case errors There are some cases, particularly where no loaded ODB backends support a particular operation, where we would return an error code without having set an error. This catches those cases and reports that no ODB backends support the operation in question. --- src/odb.c | 65 ++++++++++++++++++++++++++++++++++++------------------- 1 file changed, 43 insertions(+), 22 deletions(-) diff --git a/src/odb.c b/src/odb.c index 07e1ea6eb..4ab86b14e 100644 --- a/src/odb.c +++ b/src/odb.c @@ -424,6 +424,14 @@ size_t git_odb_num_backends(git_odb *odb) return odb->backends.length; } +static int git_odb__error_unsupported_in_backend(const char *action) +{ + giterr_set(GITERR_ODB, + "Cannot %s - unsupported in the loaded odb backends", action); + return -1; +} + + int git_odb_get_backend(git_odb_backend **out, git_odb *odb, size_t pos) { backend_internal *internal; @@ -436,6 +444,7 @@ int git_odb_get_backend(git_odb_backend **out, git_odb *odb, size_t pos) return 0; } + giterr_set(GITERR_ODB, "No ODB backend loaded at index " PRIuZ, pos); return GIT_ENOTFOUND; } @@ -469,7 +478,7 @@ static int add_default_backends( return 0; } #endif - + /* add the loose object backend */ if (git_odb_backend_loose(&loose, objects_dir, -1, 0) < 0 || add_backend_internal(db, loose, GIT_LOOSE_PRIORITY, as_alternates, inode) < 0) @@ -492,9 +501,8 @@ static int load_alternates(git_odb *odb, const char *objects_dir, int alternate_ int result = 0; /* Git reports an error, we just ignore anything deeper */ - if (alternate_depth > GIT_ALTERNATES_MAX_DEPTH) { + if (alternate_depth > GIT_ALTERNATES_MAX_DEPTH) return 0; - } if (git_buf_joinpath(&alternates_path, objects_dir, GIT_ALTERNATES_FILE) < 0) return -1; @@ -684,7 +692,7 @@ int git_odb__read_header_or_object( int git_odb_read(git_odb_object **out, git_odb *db, const git_oid *id) { - size_t i; + size_t i, reads = 0; int error; bool refreshed = false; git_rawobj raw; @@ -692,11 +700,6 @@ int git_odb_read(git_odb_object **out, git_odb *db, const git_oid *id) assert(out && db && id); - if (db->backends.length == 0) { - giterr_set(GITERR_ODB, "Failed to lookup object: no backends loaded"); - return GIT_ENOTFOUND; - } - *out = git_cache_get_raw(odb_cache(db), id); if (*out != NULL) return 0; @@ -708,8 +711,10 @@ attempt_lookup: backend_internal *internal = git_vector_get(&db->backends, i); git_odb_backend *b = internal->backend; - if (b->read != NULL) + if (b->read != NULL) { + ++reads; error = b->read(&raw.data, &raw.len, &raw.type, b, id); + } } if (error == GIT_ENOTFOUND && !refreshed) { @@ -720,8 +725,11 @@ attempt_lookup: goto attempt_lookup; } - if (error && error != GIT_PASSTHROUGH) + if (error && error != GIT_PASSTHROUGH) { + if (!reads) + return git_odb__error_notfound("no match for id", id); return error; + } if ((object = odb_object__alloc(id, &raw)) == NULL) return -1; @@ -841,10 +849,10 @@ int git_odb_write( if (!error || error == GIT_PASSTHROUGH) return 0; - /* if no backends were able to write the object directly, we try a streaming - * write to the backends; just write the whole object into the stream in one - * push */ - + /* if no backends were able to write the object directly, we try a + * streaming write to the backends; just write the whole object into the + * stream in one push + */ if ((error = git_odb_open_wstream(&stream, db, len, type)) != 0) return error; @@ -858,7 +866,7 @@ int git_odb_write( int git_odb_open_wstream( git_odb_stream **stream, git_odb *db, size_t size, git_otype type) { - size_t i; + size_t i, writes = 0; int error = GIT_ERROR; assert(stream && db); @@ -871,21 +879,26 @@ int git_odb_open_wstream( if (internal->is_alternate) continue; - if (b->writestream != NULL) + if (b->writestream != NULL) { + ++writes; error = b->writestream(stream, b, size, type); - else if (b->write != NULL) + } else if (b->write != NULL) { + ++writes; error = init_fake_wstream(stream, b, size, type); + } } if (error == GIT_PASSTHROUGH) error = 0; + if (error < 0 && !writes) + error = git_odb__error_unsupported_in_backend("write object"); return error; } int git_odb_open_rstream(git_odb_stream **stream, git_odb *db, const git_oid *oid) { - size_t i; + size_t i, reads = 0; int error = GIT_ERROR; assert(stream && db); @@ -894,19 +907,23 @@ int git_odb_open_rstream(git_odb_stream **stream, git_odb *db, const git_oid *oi backend_internal *internal = git_vector_get(&db->backends, i); git_odb_backend *b = internal->backend; - if (b->readstream != NULL) + if (b->readstream != NULL) { + ++reads; error = b->readstream(stream, b, oid); + } } if (error == GIT_PASSTHROUGH) error = 0; + if (error < 0 && !reads) + error = git_odb__error_unsupported_in_backend("read object streamed"); return error; } int git_odb_write_pack(struct git_odb_writepack **out, git_odb *db, git_transfer_progress_callback progress_cb, void *progress_payload) { - size_t i; + size_t i, writes = 0; int error = GIT_ERROR; assert(out && db); @@ -919,12 +936,16 @@ int git_odb_write_pack(struct git_odb_writepack **out, git_odb *db, git_transfer if (internal->is_alternate) continue; - if (b->writepack != NULL) + if (b->writepack != NULL) { + ++writes; error = b->writepack(out, b, progress_cb, progress_payload); + } } if (error == GIT_PASSTHROUGH) error = 0; + if (error < 0 && !writes) + error = git_odb__error_unsupported_in_backend("write pack"); return error; } From 62caf3f38fa25537b5f824329d8f88875ee837f2 Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Wed, 1 May 2013 15:01:47 -0700 Subject: [PATCH 13/22] Report some errors returnable by push --- src/push.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/push.c b/src/push.c index 9b1e78c8e..0499d8648 100644 --- a/src/push.c +++ b/src/push.c @@ -303,7 +303,7 @@ static int revwalk(git_vector *commits, git_push *push) continue; if (!git_odb_exists(push->repo->_odb, &spec->roid)) { - giterr_clear(); + giterr_set(GITERR_REFERENCE, "Cannot push missing reference"); error = GIT_ENONFASTFORWARD; goto on_error; } @@ -313,7 +313,8 @@ static int revwalk(git_vector *commits, git_push *push) if (error == GIT_ENOTFOUND || (!error && !git_oid_equal(&base, &spec->roid))) { - giterr_clear(); + giterr_set(GITERR_REFERENCE, + "Cannot push non-fastforwardable reference"); error = GIT_ENONFASTFORWARD; goto on_error; } @@ -333,12 +334,13 @@ static int revwalk(git_vector *commits, git_push *push) while ((error = git_revwalk_next(&oid, rw)) == 0) { git_oid *o = git__malloc(GIT_OID_RAWSZ); - GITERR_CHECK_ALLOC(o); - git_oid_cpy(o, &oid); - if (git_vector_insert(commits, o) < 0) { + if (!o) { error = -1; goto on_error; } + git_oid_cpy(o, &oid); + if ((error = git_vector_insert(commits, o)) < 0) + goto on_error; } on_error: @@ -519,7 +521,7 @@ static int calculate_work(git_push *push) /* This is a create or update. Local ref must exist. */ if (git_reference_name_to_id( &spec->loid, push->repo, spec->lref) < 0) { - giterr_set(GIT_ENOTFOUND, "No such reference '%s'", spec->lref); + giterr_set(GITERR_REFERENCE, "No such reference '%s'", spec->lref); return -1; } } From 41e93563e7462bd37cf3a09b8aaac35736046482 Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Wed, 1 May 2013 15:08:12 -0700 Subject: [PATCH 14/22] Error messages for a couple other boundary conditions --- src/reflog.c | 4 +++- src/refs.c | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/reflog.c b/src/reflog.c index 8c133fe53..4cc20d2c7 100644 --- a/src/reflog.c +++ b/src/reflog.c @@ -483,8 +483,10 @@ int git_reflog_drop( entry = (git_reflog_entry *)git_reflog_entry_byindex(reflog, idx); - if (entry == NULL) + if (entry == NULL) { + giterr_set(GITERR_REFERENCE, "No reflog entry at index "PRIuZ, idx); return GIT_ENOTFOUND; + } reflog_entry_free(entry); diff --git a/src/refs.c b/src/refs.c index b85a2e828..8bba3941e 100644 --- a/src/refs.c +++ b/src/refs.c @@ -844,8 +844,10 @@ static int reference__update_terminal( git_reference *ref; int error = 0; - if (nesting > MAX_NESTING_LEVEL) + if (nesting > MAX_NESTING_LEVEL) { + giterr_set(GITERR_REFERENCE, "Reference chain too deep (%d)", nesting); return GIT_ENOTFOUND; + } error = git_reference_lookup(&ref, repo, ref_name); From bf6bebe22eff29c6b7ff744b809057acede4e615 Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Wed, 1 May 2013 15:23:40 -0700 Subject: [PATCH 15/22] Factor out some code that needed to clear errors A number of places were looking up option config values and then not clearing the error codes if the values were not found. This moves the repeated pattern into a shared routine and adds the extra call to giterr_clear() when needed. --- src/remote.c | 85 ++++++++++++++++++++++++++-------------------------- 1 file changed, 42 insertions(+), 43 deletions(-) diff --git a/src/remote.c b/src/remote.c index 6eaaf8b49..cba6b2548 100644 --- a/src/remote.c +++ b/src/remote.c @@ -63,8 +63,10 @@ static int download_tags_value(git_remote *remote, git_config *cfg) else if (!error && !strcmp(val, "--tags")) remote->download_tags = GIT_REMOTE_DOWNLOAD_TAGS_ALL; - if (error == GIT_ENOTFOUND) + if (error == GIT_ENOTFOUND) { + giterr_clear(); error = 0; + } return error; } @@ -210,6 +212,31 @@ static int refspec_cb(const git_config_entry *entry, void *payload) return add_refspec(data->remote, entry->value, data->fetch); } +static int get_optional_config( + git_config *config, git_buf *buf, git_config_foreach_cb cb, void *payload) +{ + int error = 0; + const char *key = git_buf_cstr(buf); + + if (git_buf_oom(buf)) + return -1; + + if (cb != NULL) + error = git_config_get_multivar(config, key, NULL, cb, payload); + else + error = git_config_get_string(payload, config, key); + + if (error == GIT_ENOTFOUND) { + giterr_clear(); + error = 0; + } + + if (error < 0) + error = -1; + + return error; +} + int git_remote_load(git_remote **out, git_repository *repo, const char *name) { git_remote *remote; @@ -250,7 +277,7 @@ int git_remote_load(git_remote **out, git_repository *repo, const char *name) if ((error = git_config_get_string(&val, config, git_buf_cstr(&buf))) < 0) goto cleanup; - + if (strlen(val) == 0) { giterr_set(GITERR_INVALID, "Malformed remote '%s' - missing URL", name); error = -1; @@ -261,60 +288,32 @@ int git_remote_load(git_remote **out, git_repository *repo, const char *name) remote->url = git__strdup(val); GITERR_CHECK_ALLOC(remote->url); + val = NULL; git_buf_clear(&buf); - if (git_buf_printf(&buf, "remote.%s.pushurl", name) < 0) { - error = -1; - goto cleanup; - } + git_buf_printf(&buf, "remote.%s.pushurl", name); - error = git_config_get_string(&val, config, git_buf_cstr(&buf)); - if (error == GIT_ENOTFOUND) { - val = NULL; - error = 0; - } - - if (error < 0) { - error = -1; + if ((error = get_optional_config(config, &buf, NULL, &val)) < 0) goto cleanup; - } if (val) { remote->pushurl = git__strdup(val); GITERR_CHECK_ALLOC(remote->pushurl); } - git_buf_clear(&buf); - if (git_buf_printf(&buf, "remote.%s.fetch", name) < 0) { - error = -1; - goto cleanup; - } - data.remote = remote; data.fetch = true; - error = git_config_get_multivar(config, git_buf_cstr(&buf), NULL, refspec_cb, &data); - if (error == GIT_ENOTFOUND) - error = 0; - - if (error < 0) { - error = -1; - goto cleanup; - } - git_buf_clear(&buf); - if (git_buf_printf(&buf, "remote.%s.push", name) < 0) { - error = -1; + git_buf_printf(&buf, "remote.%s.fetch", name); + + if ((error = get_optional_config(config, &buf, refspec_cb, &data)) < 0) goto cleanup; - } data.fetch = false; - error = git_config_get_multivar(config, git_buf_cstr(&buf), NULL, refspec_cb, &data); - if (error == GIT_ENOTFOUND) - error = 0; + git_buf_clear(&buf); + git_buf_printf(&buf, "remote.%s.push", name); - if (error < 0) { - error = -1; + if ((error = get_optional_config(config, &buf, refspec_cb, &data)) < 0) goto cleanup; - } if (download_tags_value(remote, config) < 0) goto cleanup; @@ -336,7 +335,7 @@ static int update_config_refspec(const git_remote *remote, git_config *config, i int push; const char *dir; size_t i; - int error = -1; + int error = 0; push = direction == GIT_DIRECTION_PUSH; dir = push ? "push" : "fetch"; @@ -345,9 +344,8 @@ static int update_config_refspec(const git_remote *remote, git_config *config, i return -1; /* Clear out the existing config */ - do { + while (!error) error = git_config_delete_entry(config, git_buf_cstr(&name)); - } while (!error); if (error != GIT_ENOTFOUND) return error; @@ -358,7 +356,8 @@ static int update_config_refspec(const git_remote *remote, git_config *config, i if (spec->push != push) continue; - if ((error = git_config_set_multivar(config, git_buf_cstr(&name), "", spec->string)) < 0) { + if ((error = git_config_set_multivar( + config, git_buf_cstr(&name), "", spec->string)) < 0) { goto cleanup; } } From 1a9e406c218e3d5a174bc4f8dce0333d73d7bbff Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Wed, 1 May 2013 15:47:37 -0700 Subject: [PATCH 16/22] minor missing error message --- src/repository.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/repository.c b/src/repository.c index e6eaf753c..44e7ca3c4 100644 --- a/src/repository.c +++ b/src/repository.c @@ -1598,6 +1598,7 @@ int git_repository_message(char *buffer, size_t len, git_repository *repo) if ((error = p_stat(git_buf_cstr(&path), &st)) < 0) { if (errno == ENOENT) error = GIT_ENOTFOUND; + giterr_set(GITERR_OS, "Could not access message file"); } else if (buffer != NULL) { error = git_futils_readbuffer(&buf, git_buf_cstr(&path)); From f470b00b037cfcd40e19e88913a9a8b64d98288f Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Wed, 1 May 2013 15:48:40 -0700 Subject: [PATCH 17/22] Fix one error not reported in revparse There are many paths through revparse that may return an error code without reporting an error, I believe. This fixes one of them. Because of the backtracking in revparse, it is pretty complicated to fix the others. --- src/revparse.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/revparse.c b/src/revparse.c index 8a22a04f3..e8cc32aff 100644 --- a/src/revparse.c +++ b/src/revparse.c @@ -17,7 +17,7 @@ static int disambiguate_refname(git_reference **out, git_repository *repo, const char *refname) { int error = 0, i; - bool fallbackmode = true; + bool fallbackmode = true, foundvalid = false; git_reference *ref; git_buf refnamebuf = GIT_BUF_INIT, name = GIT_BUF_INIT; @@ -49,6 +49,7 @@ static int disambiguate_refname(git_reference **out, git_repository *repo, const error = GIT_EINVALIDSPEC; continue; } + foundvalid = true; error = git_reference_lookup_resolved(&ref, repo, git_buf_cstr(&refnamebuf), -1); @@ -63,6 +64,12 @@ static int disambiguate_refname(git_reference **out, git_repository *repo, const } cleanup: + if (error && !foundvalid) { + /* never found a valid reference name */ + giterr_set(GITERR_REFERENCE, + "Could not use '%s' as valid reference name", git_buf_cstr(&name)); + } + git_buf_free(&name); git_buf_free(&refnamebuf); return error; From 52c52737353a7ee7b653ab314d7b89ca6ddafe63 Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Wed, 1 May 2013 15:51:30 -0700 Subject: [PATCH 18/22] Clear error msg when we eat error silently --- src/stash.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/stash.c b/src/stash.c index 355c5dc9c..19b29be77 100644 --- a/src/stash.c +++ b/src/stash.c @@ -587,8 +587,10 @@ int git_stash_foreach( const git_reflog_entry *entry; error = git_reference_lookup(&stash, repo, GIT_REFS_STASH_FILE); - if (error == GIT_ENOTFOUND) + if (error == GIT_ENOTFOUND) { + giterr_clear(); return 0; + } if (error < 0) goto cleanup; @@ -651,7 +653,7 @@ int git_stash_drop( const git_reflog_entry *entry; entry = git_reflog_entry_byindex(reflog, 0); - + git_reference_free(stash); error = git_reference_create(&stash, repo, GIT_REFS_STASH_FILE, &entry->oid_cur, 1); } From 2f28219ce3da1387548b8c614d73ee01ef80f9d6 Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Wed, 1 May 2013 15:53:12 -0700 Subject: [PATCH 19/22] clarify where error message is set --- src/status.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/status.c b/src/status.c index ac6b4379b..73472ab14 100644 --- a/src/status.c +++ b/src/status.c @@ -141,7 +141,7 @@ int git_status_foreach_ext( /* if there is no HEAD, that's okay - we'll make an empty iterator */ if (((err = git_repository_head_tree(&head, repo)) < 0) && !(err == GIT_ENOTFOUND || err == GIT_EORPHANEDHEAD)) - return err; + return err; memcpy(&diffopt.pathspec, &opts->pathspec, sizeof(diffopt.pathspec)); @@ -238,7 +238,7 @@ static int get_one_status(const char *path, unsigned int status, void *data) p_fnmatch(sfi->expected, path, sfi->fnm_flags) != 0)) { sfi->ambiguous = true; - return GIT_EAMBIGUOUS; + return GIT_EAMBIGUOUS; /* giterr_set will be done by caller */ } return 0; From b60d95c714f5e68f07c5251aebbe72ba3ad5806d Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Wed, 1 May 2013 15:55:54 -0700 Subject: [PATCH 20/22] clarify error propogation --- src/tree.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tree.c b/src/tree.c index 79cbcffcb..a48b322d4 100644 --- a/src/tree.c +++ b/src/tree.c @@ -149,7 +149,7 @@ static int tree_key_search( /* Initial homing search; find an entry on the tree with * the same prefix as the filename we're looking for */ if (git_vector_bsearch2(&homing, entries, &homing_search_cmp, &ksearch) < 0) - return GIT_ENOTFOUND; + return GIT_ENOTFOUND; /* just a signal error; not passed back to user */ /* We found a common prefix. Look forward as long as * there are entries that share the common prefix */ From 0cce210a54b931462c402c1cb79091474d0b8577 Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Thu, 2 May 2013 10:36:58 -0700 Subject: [PATCH 21/22] Use assert for peel target type check --- src/object.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/src/object.c b/src/object.c index 43aecf434..a6807f26b 100644 --- a/src/object.c +++ b/src/object.c @@ -318,16 +318,11 @@ int git_object_peel( if (git_object_type(object) == target_type) return git_object_dup(peeled, (git_object *)object); - if (target_type != GIT_OBJ_TAG && - target_type != GIT_OBJ_COMMIT && - target_type != GIT_OBJ_TREE && - target_type != GIT_OBJ_BLOB && - target_type != GIT_OBJ_ANY) { - - giterr_set(GITERR_OBJECT, "Cannot peel to object type %d", - (int)target_type); - return GIT_EINVALIDSPEC; - } + assert(target_type == GIT_OBJ_TAG || + target_type == GIT_OBJ_COMMIT || + target_type == GIT_OBJ_TREE || + target_type == GIT_OBJ_BLOB || + target_type == GIT_OBJ_ANY); source = (git_object *)object; From 6e286e8dc59874db30b6fbb0ca5d32d4a2b5642c Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Sat, 4 May 2013 01:04:23 -0700 Subject: [PATCH 22/22] Remove obsolete test for peel type Peeling to an invalid type is now checked via an assert so this test is no longer relevant. --- tests-clar/object/peel.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/tests-clar/object/peel.c b/tests-clar/object/peel.c index bb0bbd096..b6c9c7a3b 100644 --- a/tests-clar/object/peel.c +++ b/tests-clar/object/peel.c @@ -103,8 +103,3 @@ void test_object_peel__target_any_object_for_type_change(void) /* fail to peel blob */ assert_peel_error(GIT_ENOTFOUND, "0266163a49e280c4f5ed1e08facd36a2bd716bcf", GIT_OBJ_ANY); } - -void test_object_peel__should_use_a_well_known_type(void) -{ - assert_peel_error(GIT_EINVALIDSPEC, "7b4384978d2493e851f9cca7858815fac9b10980", GIT_OBJ__EXT2); -}