From 9b148098e6802f9dd797471fc4f20cfc58a846b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Wed, 18 Dec 2013 19:58:16 +0100 Subject: refs: conditional ref updates Allow updating references if the old value matches the given one. --- include/git2/refs.h | 49 ++++++++++++++++++++++++++++++++++++---- include/git2/sys/refdb_backend.h | 3 ++- src/refdb.c | 4 ++-- src/refdb.h | 2 +- src/refdb_fs.c | 41 +++++++++++++++++++++++++++------ src/refs.c | 47 ++++++++++++++++++++++++++++++-------- 6 files changed, 120 insertions(+), 26 deletions(-) diff --git a/include/git2/refs.h b/include/git2/refs.h index f9aaea827..296fcb67d 100644 --- a/include/git2/refs.h +++ b/include/git2/refs.h @@ -143,6 +143,49 @@ GIT_EXTERN(int) git_reference_symbolic_create(git_reference **out, git_repositor */ GIT_EXTERN(int) git_reference_create(git_reference **out, git_repository *repo, const char *name, const git_oid *id, int force, const git_signature *signature, const char *log_message); +/** + * Conditionally create new direct reference + * + * A direct reference (also called an object id reference) refers directly + * to a specific object id (a.k.a. OID or SHA) in the repository. The id + * permanently refers to the object (although the reference itself can be + * moved). For example, in libgit2 the direct ref "refs/tags/v0.17.0" + * refers to OID 5b9fac39d8a76b9139667c26a63e6b3f204b3977. + * + * The direct reference will be created in the repository and written to + * the disk. The generated reference object must be freed by the user. + * + * Valid reference names must follow one of two patterns: + * + * 1. Top-level names must contain only capital letters and underscores, + * and must begin and end with a letter. (e.g. "HEAD", "ORIG_HEAD"). + * 2. Names prefixed with "refs/" can be almost anything. You must avoid + * the characters '~', '^', ':', '\\', '?', '[', and '*', and the + * sequences ".." and "@{" which have special meaning to revparse. + * + * This function will return an error if a reference already exists with the + * given name unless `force` is true, in which case it will be overwritten. + * + * The signature and message for the reflog will be ignored if the + * reference does not belong in the standard set (HEAD, branches and + * remote-tracking branches) and and it does not have a reflog. + * + * It will also return an error if the reference's value at the time + * of updating does not match the one passed. + * + * @param out Pointer to the newly created reference + * @param repo Repository where that reference will live + * @param name The name of the reference + * @param id The object id pointed to by the reference. + * @param force Overwrite existing references + * @param force Overwrite existing references + * @param signature The identity that will used to populate the reflog entry + * @param log_message The one line long message to be appended to the reflog + * @param old_id The old value which the reference should have + * @return 0 on success, GIT_EEXISTS, GIT_EINVALIDSPEC or an error code + */ +GIT_EXTERN(int) git_reference_create_if(git_reference **out, git_repository *repo, const char *name, const git_oid *id, int force, const git_signature *signature, const char *log_message, const git_oid *old_id); + /** * Get the OID pointed to by a direct reference. * @@ -254,16 +297,12 @@ GIT_EXTERN(int) git_reference_symbolic_set_target( const char *log_message); /** - * Create a new reference with the same name as the given reference but a + * Conditionally create a new reference with the same name as the given reference but a * different OID target. The reference must be a direct reference, otherwise * this will fail. * * The new reference will be written to disk, overwriting the given reference. * - * The signature and message for the reflog will be ignored if the - * reference does not belong in the standard set (HEAD, branches and - * remote-tracking branches) and and it does not have a reflog. - * * @param out Pointer to the newly created reference * @param ref The reference * @param id The new target OID for the reference diff --git a/include/git2/sys/refdb_backend.h b/include/git2/sys/refdb_backend.h index 5bbd4ba4c..e43f9960b 100644 --- a/include/git2/sys/refdb_backend.h +++ b/include/git2/sys/refdb_backend.h @@ -94,7 +94,8 @@ struct git_refdb_backend { */ int (*write)(git_refdb_backend *backend, const git_reference *ref, int force, - const git_signature *who, const char *message); + const git_signature *who, const char *message, + const git_oid *old); int (*rename)( git_reference **out, git_refdb_backend *backend, diff --git a/src/refdb.c b/src/refdb.c index 411423d57..9ff812433 100644 --- a/src/refdb.c +++ b/src/refdb.c @@ -167,14 +167,14 @@ void git_refdb_iterator_free(git_reference_iterator *iter) iter->free(iter); } -int git_refdb_write(git_refdb *db, git_reference *ref, int force, const git_signature *who, const char *message) +int git_refdb_write(git_refdb *db, git_reference *ref, int force, const git_signature *who, const char *message, const git_oid *old) { assert(db && db->backend); GIT_REFCOUNT_INC(db); ref->db = db; - return db->backend->write(db->backend, ref, force, who, message); + return db->backend->write(db->backend, ref, force, who, message, old); } int git_refdb_rename( diff --git a/src/refdb.h b/src/refdb.h index 91eecb782..86a1f8971 100644 --- a/src/refdb.h +++ b/src/refdb.h @@ -42,7 +42,7 @@ int git_refdb_iterator_next(git_reference **out, git_reference_iterator *iter); int git_refdb_iterator_next_name(const char **out, git_reference_iterator *iter); void git_refdb_iterator_free(git_reference_iterator *iter); -int git_refdb_write(git_refdb *refdb, git_reference *ref, int force, const git_signature *who, const char *message); +int git_refdb_write(git_refdb *refdb, git_reference *ref, int force, const git_signature *who, const char *message, const git_oid *old); int git_refdb_delete(git_refdb *refdb, const char *ref_name); int git_refdb_reflog_read(git_reflog **out, git_refdb *db, const char *name); diff --git a/src/refdb_fs.c b/src/refdb_fs.c index 89c77c14c..fd6d61e1d 100644 --- a/src/refdb_fs.c +++ b/src/refdb_fs.c @@ -690,8 +690,11 @@ static int reference_path_available( static int loose_lock(git_filebuf *file, refdb_fs_backend *backend, const git_reference *ref) { + int error; git_buf ref_path = GIT_BUF_INIT; + assert(file && backend && ref); + /* Remove a possibly existing empty directory hierarchy * which name would collide with the reference name */ @@ -701,17 +704,16 @@ static int loose_lock(git_filebuf *file, refdb_fs_backend *backend, const git_re if (git_buf_joinpath(&ref_path, backend->path, ref->name) < 0) return -1; - if (git_filebuf_open(file, ref_path.ptr, GIT_FILEBUF_FORCE, GIT_REFS_FILE_MODE) < 0) { - git_buf_free(&ref_path); - return -1; - } + error = git_filebuf_open(file, ref_path.ptr, GIT_FILEBUF_FORCE, GIT_REFS_FILE_MODE); git_buf_free(&ref_path); - return 0; + return error; } static int loose_commit(git_filebuf *file, const git_reference *ref) { + assert(file && ref); + if (ref->type == GIT_REF_OID) { char oid[GIT_OID_HEXSZ + 1]; git_oid_nfmt(oid, sizeof(oid), &ref->target.oid); @@ -933,10 +935,12 @@ static int refdb_fs_backend__write( const git_reference *ref, int force, const git_signature *who, - const char *message) + const char *message, + const git_oid *old_id) { refdb_fs_backend *backend = (refdb_fs_backend *)_backend; git_filebuf file = GIT_FILEBUF_INIT; + git_reference *old_ref; int error; assert(backend); @@ -945,10 +949,28 @@ static int refdb_fs_backend__write( if (error < 0) return error; - /* We need to perform the reflog append under the ref's lock */ + /* We need to perform the reflog append and old value check under the ref's lock */ if ((error = loose_lock(&file, backend, ref)) < 0) return error; + if (old_id) { + if ((error = refdb_fs_backend__lookup(&old_ref, _backend, ref->name)) < 0) { + git_filebuf_cleanup(&file); + return error; + } + + if (old_ref->type == GIT_REF_SYMBOLIC) { + giterr_set(GITERR_REFERENCE, "cannot compare id to symbolic reference target"); + goto on_error; + } + + /* Finally we can compare the ids */ + if (git_oid_cmp(old_id, &old_ref->target.oid)) { + giterr_set(GITERR_REFERENCE, "old reference value does not match"); + goto on_error; + } + } + if (should_write_reflog(backend->repo, ref->name) && (error = reflog_append(backend, ref, who, message)) < 0) { git_filebuf_cleanup(&file); @@ -956,6 +978,11 @@ static int refdb_fs_backend__write( } return loose_commit(&file, ref); + +on_error: + git_filebuf_cleanup(&file); + git_reference_free(old_ref); + return -1; } static int refdb_fs_backend__delete( diff --git a/src/refs.c b/src/refs.c index ca5f24ea2..a5492cf91 100644 --- a/src/refs.c +++ b/src/refs.c @@ -331,7 +331,8 @@ static int reference__create( const char *symbolic, int force, const git_signature *signature, - const char *log_message) + const char *log_message, + const git_oid *old_id) { char normalized[GIT_REFNAME_MAX]; git_refdb *refdb; @@ -380,7 +381,7 @@ static int reference__create( GITERR_CHECK_ALLOC(ref); - if ((error = git_refdb_write(refdb, ref, force, signature, log_message)) < 0) { + if ((error = git_refdb_write(refdb, ref, force, signature, log_message, old_id)) < 0) { git_reference_free(ref); return error; } @@ -410,15 +411,28 @@ int git_reference_create( git_reference **ref_out, git_repository *repo, const char *name, - const git_oid *oid, + const git_oid *id, int force, const git_signature *signature, const char *log_message) +{ + return git_reference_create_if(ref_out, repo, name, id, force, signature, log_message, NULL); +} + +int git_reference_create_if( + git_reference **ref_out, + git_repository *repo, + const char *name, + const git_oid *id, + int force, + const git_signature *signature, + const char *log_message, + const git_oid *old_id) { int error; git_signature *who = NULL; - assert(oid); + assert(id); if (!signature) { if ((error = log_signature(&who, repo)) < 0) @@ -428,7 +442,7 @@ int git_reference_create( } error = reference__create( - ref_out, repo, name, oid, NULL, force, signature, log_message); + ref_out, repo, name, id, NULL, force, signature, log_message, old_id); git_signature_free(who); return error; @@ -456,7 +470,7 @@ int git_reference_symbolic_create( } error = reference__create( - ref_out, repo, name, NULL, target, force, signature, log_message); + ref_out, repo, name, NULL, target, force, signature, log_message, NULL); git_signature_free(who); return error; @@ -471,22 +485,25 @@ static int ensure_is_an_updatable_direct_reference(git_reference *ref) return -1; } -int git_reference_set_target( +int git_reference_set_target_if( git_reference **out, git_reference *ref, const git_oid *id, const git_signature *signature, - const char *log_message) + const char *log_message, + const git_oid *old_id) { int error; + git_repository *repo; assert(out && ref && id); + repo = ref->db->repo; + if ((error = ensure_is_an_updatable_direct_reference(ref)) < 0) return error; - return git_reference_create( - out, ref->db->repo, ref->name, id, 1, signature, log_message); + return git_reference_create_if(out, repo, ref->name, id, 1, signature, log_message, old_id); } static int ensure_is_an_updatable_symbolic_reference(git_reference *ref) @@ -498,6 +515,16 @@ static int ensure_is_an_updatable_symbolic_reference(git_reference *ref) return -1; } +int git_reference_set_target( + git_reference **out, + git_reference *ref, + const git_oid *id, + const git_signature *signature, + const char *log_message) +{ + return git_reference_set_target_if(out, ref, id, signature, log_message, NULL); +} + int git_reference_symbolic_set_target( git_reference **out, git_reference *ref, -- cgit v1.2.3 From 5d96fe8828505db852671175d5895dfd275f3d06 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Tue, 14 Jan 2014 15:33:29 +0100 Subject: refs: changes from feedback Change the name to _matching() intead of _if(), and force _set_target() to be a conditional update. If the user doesn't care about the old value, they should use git_reference_create(). --- include/git2/refs.h | 2 +- src/refs.c | 44 +++++++++++++++++--------------------------- 2 files changed, 18 insertions(+), 28 deletions(-) diff --git a/include/git2/refs.h b/include/git2/refs.h index 296fcb67d..72622948b 100644 --- a/include/git2/refs.h +++ b/include/git2/refs.h @@ -184,7 +184,7 @@ GIT_EXTERN(int) git_reference_create(git_reference **out, git_repository *repo, * @param old_id The old value which the reference should have * @return 0 on success, GIT_EEXISTS, GIT_EINVALIDSPEC or an error code */ -GIT_EXTERN(int) git_reference_create_if(git_reference **out, git_repository *repo, const char *name, const git_oid *id, int force, const git_signature *signature, const char *log_message, const git_oid *old_id); +GIT_EXTERN(int) git_reference_create_matching(git_reference **out, git_repository *repo, const char *name, const git_oid *id, int force, const git_signature *signature, const char *log_message, const git_oid *old_id); /** * Get the OID pointed to by a direct reference. diff --git a/src/refs.c b/src/refs.c index a5492cf91..007fdf353 100644 --- a/src/refs.c +++ b/src/refs.c @@ -407,19 +407,7 @@ static int log_signature(git_signature **out, git_repository *repo) return 0; } -int git_reference_create( - git_reference **ref_out, - git_repository *repo, - const char *name, - const git_oid *id, - int force, - const git_signature *signature, - const char *log_message) -{ - return git_reference_create_if(ref_out, repo, name, id, force, signature, log_message, NULL); -} - -int git_reference_create_if( +int git_reference_create_matching( git_reference **ref_out, git_repository *repo, const char *name, @@ -428,6 +416,7 @@ int git_reference_create_if( const git_signature *signature, const char *log_message, const git_oid *old_id) + { int error; git_signature *who = NULL; @@ -448,6 +437,18 @@ int git_reference_create_if( return error; } +int git_reference_create( + git_reference **ref_out, + git_repository *repo, + const char *name, + const git_oid *id, + int force, + const git_signature *signature, + const char *log_message) +{ + return git_reference_create_matching(ref_out, repo, name, id, force, signature, log_message, NULL); +} + int git_reference_symbolic_create( git_reference **ref_out, git_repository *repo, @@ -485,13 +486,12 @@ static int ensure_is_an_updatable_direct_reference(git_reference *ref) return -1; } -int git_reference_set_target_if( +int git_reference_set_target( git_reference **out, git_reference *ref, const git_oid *id, const git_signature *signature, - const char *log_message, - const git_oid *old_id) + const char *log_message) { int error; git_repository *repo; @@ -503,7 +503,7 @@ int git_reference_set_target_if( if ((error = ensure_is_an_updatable_direct_reference(ref)) < 0) return error; - return git_reference_create_if(out, repo, ref->name, id, 1, signature, log_message, old_id); + return git_reference_create_matching(out, repo, ref->name, id, 1, signature, log_message, &ref->target.oid); } static int ensure_is_an_updatable_symbolic_reference(git_reference *ref) @@ -515,16 +515,6 @@ static int ensure_is_an_updatable_symbolic_reference(git_reference *ref) return -1; } -int git_reference_set_target( - git_reference **out, - git_reference *ref, - const git_oid *id, - const git_signature *signature, - const char *log_message) -{ - return git_reference_set_target_if(out, ref, id, signature, log_message, NULL); -} - int git_reference_symbolic_set_target( git_reference **out, git_reference *ref, -- cgit v1.2.3 From fc4728e3e2a4094b202a63319232f25adbd55fed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Wed, 29 Jan 2014 14:07:18 +0100 Subject: refs: return GIT_EMODIFIED if the ref target moved In case we loose the race to update the reference, return GIT_EMODIFIED to let the user distinguish it from other types of errors. --- include/git2/errors.h | 1 + include/git2/refs.h | 6 ++++-- src/refdb_fs.c | 6 ++++-- 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/include/git2/errors.h b/include/git2/errors.h index 973d56003..bcf2f80ab 100644 --- a/include/git2/errors.h +++ b/include/git2/errors.h @@ -40,6 +40,7 @@ typedef enum { GIT_EINVALIDSPEC = -12, /*< Name/ref spec was not in a valid format */ GIT_EMERGECONFLICT = -13, /*< Merge conflicts prevented operation */ GIT_ELOCKED = -14, /*< Lock file prevented operation */ + GIT_EMODIFIED = -15, /*< Reference value does not match expected */ GIT_PASSTHROUGH = -30, /*< Internal only */ GIT_ITEROVER = -31, /*< Signals end of iteration with iterator */ diff --git a/include/git2/refs.h b/include/git2/refs.h index 72622948b..aab8715fb 100644 --- a/include/git2/refs.h +++ b/include/git2/refs.h @@ -182,7 +182,8 @@ GIT_EXTERN(int) git_reference_create(git_reference **out, git_repository *repo, * @param signature The identity that will used to populate the reflog entry * @param log_message The one line long message to be appended to the reflog * @param old_id The old value which the reference should have - * @return 0 on success, GIT_EEXISTS, GIT_EINVALIDSPEC or an error code + * @return 0 on success, GIT_EMODIFIED if the value of the reference + * has changed, GIT_EEXISTS, GIT_EINVALIDSPEC or an error code */ GIT_EXTERN(int) git_reference_create_matching(git_reference **out, git_repository *repo, const char *name, const git_oid *id, int force, const git_signature *signature, const char *log_message, const git_oid *old_id); @@ -308,7 +309,8 @@ GIT_EXTERN(int) git_reference_symbolic_set_target( * @param id The new target OID for the reference * @param signature The identity that will used to populate the reflog entry * @param log_message The one line long message to be appended to the reflog - * @return 0 or an error code + * @return 0 on success, GIT_EMODIFIED if the value of the reference + * has changed, or an error code */ GIT_EXTERN(int) git_reference_set_target( git_reference **out, diff --git a/src/refdb_fs.c b/src/refdb_fs.c index fd6d61e1d..0a2a33a51 100644 --- a/src/refdb_fs.c +++ b/src/refdb_fs.c @@ -941,7 +941,7 @@ static int refdb_fs_backend__write( refdb_fs_backend *backend = (refdb_fs_backend *)_backend; git_filebuf file = GIT_FILEBUF_INIT; git_reference *old_ref; - int error; + int error = 0; assert(backend); @@ -961,12 +961,14 @@ static int refdb_fs_backend__write( if (old_ref->type == GIT_REF_SYMBOLIC) { giterr_set(GITERR_REFERENCE, "cannot compare id to symbolic reference target"); + error = -1; goto on_error; } /* Finally we can compare the ids */ if (git_oid_cmp(old_id, &old_ref->target.oid)) { giterr_set(GITERR_REFERENCE, "old reference value does not match"); + error = GIT_EMODIFIED; goto on_error; } } @@ -982,7 +984,7 @@ static int refdb_fs_backend__write( on_error: git_filebuf_cleanup(&file); git_reference_free(old_ref); - return -1; + return error; } static int refdb_fs_backend__delete( -- cgit v1.2.3 From 1202c7eaa6f0fd6407bc386881edd686771fc0f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Tue, 4 Feb 2014 21:35:44 +0100 Subject: refs: fix leak on successful update Free the old ref even on success. --- src/refdb_fs.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/refdb_fs.c b/src/refdb_fs.c index 0a2a33a51..554fe42c9 100644 --- a/src/refdb_fs.c +++ b/src/refdb_fs.c @@ -941,7 +941,7 @@ static int refdb_fs_backend__write( refdb_fs_backend *backend = (refdb_fs_backend *)_backend; git_filebuf file = GIT_FILEBUF_INIT; git_reference *old_ref; - int error = 0; + int error = 0, cmp; assert(backend); @@ -954,19 +954,20 @@ static int refdb_fs_backend__write( return error; if (old_id) { - if ((error = refdb_fs_backend__lookup(&old_ref, _backend, ref->name)) < 0) { - git_filebuf_cleanup(&file); - return error; - } + if ((error = refdb_fs_backend__lookup(&old_ref, _backend, ref->name)) < 0) + goto on_error; if (old_ref->type == GIT_REF_SYMBOLIC) { + git_reference_free(old_ref); giterr_set(GITERR_REFERENCE, "cannot compare id to symbolic reference target"); error = -1; goto on_error; } /* Finally we can compare the ids */ - if (git_oid_cmp(old_id, &old_ref->target.oid)) { + cmp = git_oid_cmp(old_id, &old_ref->target.oid); + git_reference_free(old_ref); + if (cmp) { giterr_set(GITERR_REFERENCE, "old reference value does not match"); error = GIT_EMODIFIED; goto on_error; @@ -983,7 +984,6 @@ static int refdb_fs_backend__write( on_error: git_filebuf_cleanup(&file); - git_reference_free(old_ref); return error; } -- cgit v1.2.3 From d6236cf662ebd4ba8ef4902c81a19bbfd92847f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Tue, 4 Feb 2014 21:40:22 +0100 Subject: refs: add tests for conditional updates --- tests/refs/races.c | 42 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) create mode 100644 tests/refs/races.c diff --git a/tests/refs/races.c b/tests/refs/races.c new file mode 100644 index 000000000..4d24896f1 --- /dev/null +++ b/tests/refs/races.c @@ -0,0 +1,42 @@ +#include "clar_libgit2.h" + +#include "repository.h" +#include "git2/reflog.h" +#include "reflog.h" +#include "ref_helpers.h" + +static const char *commit_id = "099fabac3a9ea935598528c27f866e34089c2eff"; +static const char *refname = "refs/heads/master"; +static const char *other_commit_id = "a65fedf39aefe402d3bb6e24df4d4f5fe4547750"; + +static git_repository *g_repo; + +void test_refs_races__initialize(void) +{ + g_repo = cl_git_sandbox_init("testrepo"); +} + +void test_refs_races__cleanup(void) +{ + cl_git_sandbox_cleanup(); +} + +void test_refs_races__create_matching(void) +{ + int error; + git_reference *ref, *ref2, *ref3; + git_oid id, other_id; + + git_oid_fromstr(&id, commit_id); + git_oid_fromstr(&other_id, other_commit_id); + + cl_git_fail_with(GIT_EMODIFIED, git_reference_create_matching(&ref, g_repo, refname, &other_id, 1, NULL, NULL, &other_id)); + + cl_git_pass(git_reference_lookup(&ref, g_repo, refname)); + cl_git_pass(git_reference_create_matching(&ref2, g_repo, refname, &other_id, 1, NULL, NULL, &id)); + cl_git_fail_with(GIT_EMODIFIED, git_reference_set_target(&ref3, ref, &other_id, NULL, NULL)); + + git_reference_free(ref); + git_reference_free(ref2); + git_reference_free(ref3); +} -- cgit v1.2.3 From 911236619b5d774e33dd9f3de92a7c86c2befb26 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Tue, 4 Feb 2014 22:04:00 +0100 Subject: refdb: add conditional symbolic updates Add a parameter to the backend to allow checking for the old symbolic target. --- include/git2/sys/refdb_backend.h | 2 +- src/refdb.c | 4 ++-- src/refdb.h | 2 +- src/refdb_fs.c | 35 ++++++++++++++++++----------------- src/refs.c | 9 +++++---- 5 files changed, 27 insertions(+), 25 deletions(-) diff --git a/include/git2/sys/refdb_backend.h b/include/git2/sys/refdb_backend.h index e43f9960b..13ce9a026 100644 --- a/include/git2/sys/refdb_backend.h +++ b/include/git2/sys/refdb_backend.h @@ -95,7 +95,7 @@ struct git_refdb_backend { int (*write)(git_refdb_backend *backend, const git_reference *ref, int force, const git_signature *who, const char *message, - const git_oid *old); + const git_oid *old, const char *old_target); int (*rename)( git_reference **out, git_refdb_backend *backend, diff --git a/src/refdb.c b/src/refdb.c index 9ff812433..66d943e86 100644 --- a/src/refdb.c +++ b/src/refdb.c @@ -167,14 +167,14 @@ void git_refdb_iterator_free(git_reference_iterator *iter) iter->free(iter); } -int git_refdb_write(git_refdb *db, git_reference *ref, int force, const git_signature *who, const char *message, const git_oid *old) +int git_refdb_write(git_refdb *db, git_reference *ref, int force, const git_signature *who, const char *message, const git_oid *old_id, const char *old_target) { assert(db && db->backend); GIT_REFCOUNT_INC(db); ref->db = db; - return db->backend->write(db->backend, ref, force, who, message, old); + return db->backend->write(db->backend, ref, force, who, message, old_id, old_target); } int git_refdb_rename( diff --git a/src/refdb.h b/src/refdb.h index 86a1f8971..eabb5969b 100644 --- a/src/refdb.h +++ b/src/refdb.h @@ -42,7 +42,7 @@ int git_refdb_iterator_next(git_reference **out, git_reference_iterator *iter); int git_refdb_iterator_next_name(const char **out, git_reference_iterator *iter); void git_refdb_iterator_free(git_reference_iterator *iter); -int git_refdb_write(git_refdb *refdb, git_reference *ref, int force, const git_signature *who, const char *message, const git_oid *old); +int git_refdb_write(git_refdb *refdb, git_reference *ref, int force, const git_signature *who, const char *message, const git_oid *old_id, const char *old_target); int git_refdb_delete(git_refdb *refdb, const char *ref_name); int git_refdb_reflog_read(git_reflog **out, git_refdb *db, const char *name); diff --git a/src/refdb_fs.c b/src/refdb_fs.c index 554fe42c9..879e48514 100644 --- a/src/refdb_fs.c +++ b/src/refdb_fs.c @@ -936,12 +936,13 @@ static int refdb_fs_backend__write( int force, const git_signature *who, const char *message, - const git_oid *old_id) + const git_oid *old_id, + const char *old_target) { refdb_fs_backend *backend = (refdb_fs_backend *)_backend; git_filebuf file = GIT_FILEBUF_INIT; - git_reference *old_ref; - int error = 0, cmp; + git_reference *old_ref = NULL; + int error = 0, cmp = 0; assert(backend); @@ -953,25 +954,25 @@ static int refdb_fs_backend__write( if ((error = loose_lock(&file, backend, ref)) < 0) return error; - if (old_id) { + if (old_id || old_target) { if ((error = refdb_fs_backend__lookup(&old_ref, _backend, ref->name)) < 0) goto on_error; + } - if (old_ref->type == GIT_REF_SYMBOLIC) { - git_reference_free(old_ref); - giterr_set(GITERR_REFERENCE, "cannot compare id to symbolic reference target"); - error = -1; - goto on_error; - } - - /* Finally we can compare the ids */ + if (old_id && old_ref->type == GIT_REF_OID) { cmp = git_oid_cmp(old_id, &old_ref->target.oid); git_reference_free(old_ref); - if (cmp) { - giterr_set(GITERR_REFERENCE, "old reference value does not match"); - error = GIT_EMODIFIED; - goto on_error; - } + } + + if (old_target && old_ref->type == GIT_REF_SYMBOLIC) { + cmp = git__strcmp(old_target, old_ref->target.symbolic); + git_reference_free(old_ref); + } + + if (cmp) { + giterr_set(GITERR_REFERENCE, "old reference value does not match"); + error = GIT_EMODIFIED; + goto on_error; } if (should_write_reflog(backend->repo, ref->name) && diff --git a/src/refs.c b/src/refs.c index 007fdf353..888b5cb3c 100644 --- a/src/refs.c +++ b/src/refs.c @@ -332,7 +332,8 @@ static int reference__create( int force, const git_signature *signature, const char *log_message, - const git_oid *old_id) + const git_oid *old_id, + const char *old_target) { char normalized[GIT_REFNAME_MAX]; git_refdb *refdb; @@ -381,7 +382,7 @@ static int reference__create( GITERR_CHECK_ALLOC(ref); - if ((error = git_refdb_write(refdb, ref, force, signature, log_message, old_id)) < 0) { + if ((error = git_refdb_write(refdb, ref, force, signature, log_message, old_id, old_target)) < 0) { git_reference_free(ref); return error; } @@ -431,7 +432,7 @@ int git_reference_create_matching( } error = reference__create( - ref_out, repo, name, id, NULL, force, signature, log_message, old_id); + ref_out, repo, name, id, NULL, force, signature, log_message, old_id, NULL); git_signature_free(who); return error; @@ -471,7 +472,7 @@ int git_reference_symbolic_create( } error = reference__create( - ref_out, repo, name, NULL, target, force, signature, log_message, NULL); + ref_out, repo, name, NULL, target, force, signature, log_message, NULL, NULL); git_signature_free(who); return error; -- cgit v1.2.3 From 878fb66f5765115eff34213cfc8dd04b8a56b2a5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Wed, 5 Feb 2014 10:19:17 +0100 Subject: refs: bring conditional symbolic updates to the frontend Bring the race detection goodness to symbolic references as well. --- include/git2/refs.h | 40 ++++++++++++++++++++++++++++++++++++++++ src/refs.c | 23 ++++++++++++++++++----- tests/refs/races.c | 21 ++++++++++++++++++++- 3 files changed, 78 insertions(+), 6 deletions(-) diff --git a/include/git2/refs.h b/include/git2/refs.h index aab8715fb..284671d2c 100644 --- a/include/git2/refs.h +++ b/include/git2/refs.h @@ -67,6 +67,46 @@ GIT_EXTERN(int) git_reference_name_to_id( */ GIT_EXTERN(int) git_reference_dwim(git_reference **out, git_repository *repo, const char *shorthand); +/** + * Conditionally create a new symbolic reference. + * + * A symbolic reference is a reference name that refers to another + * reference name. If the other name moves, the symbolic name will move, + * too. As a simple example, the "HEAD" reference might refer to + * "refs/heads/master" while on the "master" branch of a repository. + * + * The symbolic reference will be created in the repository and written to + * the disk. The generated reference object must be freed by the user. + * + * Valid reference names must follow one of two patterns: + * + * 1. Top-level names must contain only capital letters and underscores, + * and must begin and end with a letter. (e.g. "HEAD", "ORIG_HEAD"). + * 2. Names prefixed with "refs/" can be almost anything. You must avoid + * the characters '~', '^', ':', '\\', '?', '[', and '*', and the + * sequences ".." and "@{" which have special meaning to revparse. + * + * This function will return an error if a reference already exists with the + * given name unless `force` is true, in which case it will be overwritten. + * + * The signature and message for the reflog will be ignored if the + * reference does not belong in the standard set (HEAD, branches and + * remote-tracking branches) and it does not have a reflog. + * + * It will also return an error if the reference's value at the time + * of updating does not match the one passed. + * + * @param out Pointer to the newly created reference + * @param repo Repository where that reference will live + * @param name The name of the reference + * @param target The target of the reference + * @param force Overwrite existing references + * @param signature The identity that will used to populate the reflog entry + * @param log_message The one line long message to be appended to the reflog + * @return 0 on success, GIT_EEXISTS, GIT_EINVALIDSPEC, GIT_EMODIFIED or an error code + */ +GIT_EXTERN(int) git_reference_symbolic_create_matching(git_reference **out, git_repository *repo, const char *name, const char *target, int force, const git_signature *signature, const char *log_message, const char *old_value); + /** * Create a new symbolic reference. * diff --git a/src/refs.c b/src/refs.c index 888b5cb3c..864bfce59 100644 --- a/src/refs.c +++ b/src/refs.c @@ -450,14 +450,15 @@ int git_reference_create( return git_reference_create_matching(ref_out, repo, name, id, force, signature, log_message, NULL); } -int git_reference_symbolic_create( +int git_reference_symbolic_create_matching( git_reference **ref_out, git_repository *repo, const char *name, const char *target, int force, const git_signature *signature, - const char *log_message) + const char *log_message, + const char *old_target) { int error; git_signature *who = NULL; @@ -472,12 +473,24 @@ int git_reference_symbolic_create( } error = reference__create( - ref_out, repo, name, NULL, target, force, signature, log_message, NULL, NULL); + ref_out, repo, name, NULL, target, force, signature, log_message, NULL, old_target); git_signature_free(who); return error; } +int git_reference_symbolic_create( + git_reference **ref_out, + git_repository *repo, + const char *name, + const char *target, + int force, + const git_signature *signature, + const char *log_message) +{ + return git_reference_symbolic_create_matching(ref_out, repo, name, target, force, signature, log_message, NULL); +} + static int ensure_is_an_updatable_direct_reference(git_reference *ref) { if (ref->type == GIT_REF_OID) @@ -530,8 +543,8 @@ int git_reference_symbolic_set_target( if ((error = ensure_is_an_updatable_symbolic_reference(ref)) < 0) return error; - return git_reference_symbolic_create( - out, ref->db->repo, ref->name, target, 1, signature, log_message); + return git_reference_symbolic_create_matching( + out, ref->db->repo, ref->name, target, 1, signature, log_message, ref->target.symbolic); } static int reference__rename(git_reference **out, git_reference *ref, const char *new_name, int force, diff --git a/tests/refs/races.c b/tests/refs/races.c index 4d24896f1..6613fc20f 100644 --- a/tests/refs/races.c +++ b/tests/refs/races.c @@ -7,6 +7,7 @@ static const char *commit_id = "099fabac3a9ea935598528c27f866e34089c2eff"; static const char *refname = "refs/heads/master"; +static const char *other_refname = "refs/heads/foo"; static const char *other_commit_id = "a65fedf39aefe402d3bb6e24df4d4f5fe4547750"; static git_repository *g_repo; @@ -23,7 +24,6 @@ void test_refs_races__cleanup(void) void test_refs_races__create_matching(void) { - int error; git_reference *ref, *ref2, *ref3; git_oid id, other_id; @@ -40,3 +40,22 @@ void test_refs_races__create_matching(void) git_reference_free(ref2); git_reference_free(ref3); } + +void test_refs_races__symbolic_create_matching(void) +{ + git_reference *ref, *ref2, *ref3; + git_oid id, other_id; + + git_oid_fromstr(&id, commit_id); + git_oid_fromstr(&other_id, other_commit_id); + + cl_git_fail_with(GIT_EMODIFIED, git_reference_symbolic_create_matching(&ref, g_repo, "HEAD", other_refname, 1, NULL, NULL, other_refname)); + + cl_git_pass(git_reference_lookup(&ref, g_repo, "HEAD")); + cl_git_pass(git_reference_symbolic_create_matching(&ref2, g_repo, "HEAD", other_refname, 1, NULL, NULL, refname)); + cl_git_fail_with(GIT_EMODIFIED, git_reference_symbolic_set_target(&ref3, ref, other_refname, NULL, NULL)); + + git_reference_free(ref); + git_reference_free(ref2); + git_reference_free(ref3); +} -- cgit v1.2.3 From f8621dde4057aba2df8d7585f6fbf2d6ee4fc023 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Wed, 5 Feb 2014 10:42:42 +0100 Subject: refs: factor out old value comparison We will reuse this later for deletion. --- src/refdb_fs.c | 40 ++++++++++++++++++++++++++-------------- 1 file changed, 26 insertions(+), 14 deletions(-) diff --git a/src/refdb_fs.c b/src/refdb_fs.c index 879e48514..d1b20f8e3 100644 --- a/src/refdb_fs.c +++ b/src/refdb_fs.c @@ -930,6 +930,30 @@ static bool should_write_reflog(git_repository *repo, const char *name) return 0; } +static int cmp_old_ref(int *cmp, git_refdb_backend *backend, const git_reference *ref, + const git_oid *old_id, const char *old_target) +{ + int error = 0; + git_reference *old_ref = NULL; + + *cmp = 0; + if (old_id || old_target) { + if ((error = refdb_fs_backend__lookup(&old_ref, backend, ref->name)) < 0) + goto out; + } + + if (old_id && old_ref->type == GIT_REF_OID) + *cmp = git_oid_cmp(old_id, &old_ref->target.oid); + + if (old_target && old_ref->type == GIT_REF_SYMBOLIC) + *cmp = git__strcmp(old_target, old_ref->target.symbolic); + +out: + git_reference_free(old_ref); + + return error; +} + static int refdb_fs_backend__write( git_refdb_backend *_backend, const git_reference *ref, @@ -954,20 +978,8 @@ static int refdb_fs_backend__write( if ((error = loose_lock(&file, backend, ref)) < 0) return error; - if (old_id || old_target) { - if ((error = refdb_fs_backend__lookup(&old_ref, _backend, ref->name)) < 0) - goto on_error; - } - - if (old_id && old_ref->type == GIT_REF_OID) { - cmp = git_oid_cmp(old_id, &old_ref->target.oid); - git_reference_free(old_ref); - } - - if (old_target && old_ref->type == GIT_REF_SYMBOLIC) { - cmp = git__strcmp(old_target, old_ref->target.symbolic); - git_reference_free(old_ref); - } + if ((error = cmp_old_ref(&cmp, _backend, ref, old_id, old_target)) < 0) + goto on_error; if (cmp) { giterr_set(GITERR_REFERENCE, "old reference value does not match"); -- cgit v1.2.3 From 7ee8c7e6776a3c5b3a45cfd10ccf205eebb3f3fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Wed, 5 Feb 2014 11:07:34 +0100 Subject: refs: placeholder conditional delete We don't actually pass the old value yet. --- include/git2/sys/refdb_backend.h | 2 +- src/refdb.c | 4 +-- src/refdb.h | 2 +- src/refdb_fs.c | 66 ++++++++++++++++++++++++++-------------- src/refs.c | 2 +- 5 files changed, 49 insertions(+), 27 deletions(-) diff --git a/include/git2/sys/refdb_backend.h b/include/git2/sys/refdb_backend.h index 13ce9a026..aa5ef9ecc 100644 --- a/include/git2/sys/refdb_backend.h +++ b/include/git2/sys/refdb_backend.h @@ -106,7 +106,7 @@ struct git_refdb_backend { * Deletes the given reference from the refdb. A refdb implementation * must provide this function. */ - int (*del)(git_refdb_backend *backend, const char *ref_name); + int (*del)(git_refdb_backend *backend, const char *ref_name, const git_oid *old_id, const char *old_target); /** * Suggests that the given refdb compress or optimize its references. diff --git a/src/refdb.c b/src/refdb.c index 66d943e86..984c3c7f6 100644 --- a/src/refdb.c +++ b/src/refdb.c @@ -201,10 +201,10 @@ int git_refdb_rename( return 0; } -int git_refdb_delete(struct git_refdb *db, const char *ref_name) +int git_refdb_delete(struct git_refdb *db, const char *ref_name, const git_oid *old_id, const char *old_target) { assert(db && db->backend); - return db->backend->del(db->backend, ref_name); + return db->backend->del(db->backend, ref_name, old_id, old_target); } int git_refdb_reflog_read(git_reflog **out, git_refdb *db, const char *name) diff --git a/src/refdb.h b/src/refdb.h index eabb5969b..cbad86faf 100644 --- a/src/refdb.h +++ b/src/refdb.h @@ -43,7 +43,7 @@ int git_refdb_iterator_next_name(const char **out, git_reference_iterator *iter) void git_refdb_iterator_free(git_reference_iterator *iter); int git_refdb_write(git_refdb *refdb, git_reference *ref, int force, const git_signature *who, const char *message, const git_oid *old_id, const char *old_target); -int git_refdb_delete(git_refdb *refdb, const char *ref_name); +int git_refdb_delete(git_refdb *refdb, const char *ref_name, const git_oid *old_id, const char *old_target); int git_refdb_reflog_read(git_reflog **out, git_refdb *db, const char *name); int git_refdb_reflog_write(git_reflog *reflog); diff --git a/src/refdb_fs.c b/src/refdb_fs.c index d1b20f8e3..c9f9c0f1c 100644 --- a/src/refdb_fs.c +++ b/src/refdb_fs.c @@ -688,20 +688,20 @@ static int reference_path_available( return 0; } -static int loose_lock(git_filebuf *file, refdb_fs_backend *backend, const git_reference *ref) +static int loose_lock(git_filebuf *file, refdb_fs_backend *backend, const char *name) { int error; git_buf ref_path = GIT_BUF_INIT; - assert(file && backend && ref); + assert(file && backend && name); /* Remove a possibly existing empty directory hierarchy * which name would collide with the reference name */ - if (git_futils_rmdir_r(ref->name, backend->path, GIT_RMDIR_SKIP_NONEMPTY) < 0) + if (git_futils_rmdir_r(name, backend->path, GIT_RMDIR_SKIP_NONEMPTY) < 0) return -1; - if (git_buf_joinpath(&ref_path, backend->path, ref->name) < 0) + if (git_buf_joinpath(&ref_path, backend->path, name) < 0) return -1; error = git_filebuf_open(file, ref_path.ptr, GIT_FILEBUF_FORCE, GIT_REFS_FILE_MODE); @@ -930,7 +930,7 @@ static bool should_write_reflog(git_repository *repo, const char *name) return 0; } -static int cmp_old_ref(int *cmp, git_refdb_backend *backend, const git_reference *ref, +static int cmp_old_ref(int *cmp, git_refdb_backend *backend, const char *name, const git_oid *old_id, const char *old_target) { int error = 0; @@ -938,7 +938,7 @@ static int cmp_old_ref(int *cmp, git_refdb_backend *backend, const git_reference *cmp = 0; if (old_id || old_target) { - if ((error = refdb_fs_backend__lookup(&old_ref, backend, ref->name)) < 0) + if ((error = refdb_fs_backend__lookup(&old_ref, backend, name)) < 0) goto out; } @@ -965,7 +965,6 @@ static int refdb_fs_backend__write( { refdb_fs_backend *backend = (refdb_fs_backend *)_backend; git_filebuf file = GIT_FILEBUF_INIT; - git_reference *old_ref = NULL; int error = 0, cmp = 0; assert(backend); @@ -975,10 +974,10 @@ static int refdb_fs_backend__write( return error; /* We need to perform the reflog append and old value check under the ref's lock */ - if ((error = loose_lock(&file, backend, ref)) < 0) + if ((error = loose_lock(&file, backend, ref->name)) < 0) return error; - if ((error = cmp_old_ref(&cmp, _backend, ref, old_id, old_target)) < 0) + if ((error = cmp_old_ref(&cmp, _backend, ref->name, old_id, old_target)) < 0) goto on_error; if (cmp) { @@ -1002,16 +1001,32 @@ on_error: static int refdb_fs_backend__delete( git_refdb_backend *_backend, - const char *ref_name) + const char *ref_name, + const git_oid *old_id, const char *old_target) { refdb_fs_backend *backend = (refdb_fs_backend *)_backend; git_buf loose_path = GIT_BUF_INIT; - size_t pack_pos; - int error = 0; + size_t pack_pos +; git_filebuf file = GIT_FILEBUF_INIT; + int error = 0, cmp = 0; bool loose_deleted = 0; assert(backend && ref_name); + if ((error = loose_lock(&file, backend, ref_name)) < 0) + return error; + + error = cmp_old_ref(&cmp, _backend, ref_name, old_id, old_target); + //git_filebuf_cleanup(&file); + if (error < 0) + goto cleanup; + + if (cmp) { + giterr_set(GITERR_REFERENCE, "old reference value does not match"); + error = GIT_EMODIFIED; + goto cleanup; + } + /* If a loose reference exists, remove it from the filesystem */ if (git_buf_joinpath(&loose_path, backend->path, ref_name) < 0) return -1; @@ -1024,14 +1039,14 @@ static int refdb_fs_backend__delete( git_buf_free(&loose_path); if (error != 0) - return error; + goto cleanup; - if (packed_reload(backend) < 0) - return -1; + if ((error = packed_reload(backend)) < 0) + goto cleanup; /* If a packed reference exists, remove it from the packfile and repack */ - if (git_sortedcache_wlock(backend->refcache) < 0) - return -1; + if ((error = git_sortedcache_wlock(backend->refcache)) < 0) + goto cleanup; if (!(error = git_sortedcache_lookup_index( &pack_pos, backend->refcache, ref_name))) @@ -1039,10 +1054,17 @@ static int refdb_fs_backend__delete( git_sortedcache_wunlock(backend->refcache); - if (error == GIT_ENOTFOUND) - return loose_deleted ? 0 : ref_error_notfound(ref_name); + if (error == GIT_ENOTFOUND) { + error = loose_deleted ? 0 : ref_error_notfound(ref_name); + goto cleanup; + } + + error = packed_write(backend); - return packed_write(backend); +cleanup: + git_filebuf_cleanup(&file); + + return error; } static int refdb_reflog_fs__rename(git_refdb_backend *_backend, const char *old_name, const char *new_name); @@ -1068,7 +1090,7 @@ static int refdb_fs_backend__rename( (error = refdb_fs_backend__lookup(&old, _backend, old_name)) < 0) return error; - if ((error = refdb_fs_backend__delete(_backend, old_name)) < 0) { + if ((error = refdb_fs_backend__delete(_backend, old_name, NULL, NULL)) < 0) { git_reference_free(old); return error; } @@ -1079,7 +1101,7 @@ static int refdb_fs_backend__rename( return -1; } - if ((error = loose_lock(&file, backend, new)) < 0) { + if ((error = loose_lock(&file, backend, new->name)) < 0) { git_reference_free(new); return error; } diff --git a/src/refs.c b/src/refs.c index 864bfce59..f45c64246 100644 --- a/src/refs.c +++ b/src/refs.c @@ -116,7 +116,7 @@ void git_reference_free(git_reference *reference) int git_reference_delete(git_reference *ref) { - return git_refdb_delete(ref->db, ref->name); + return git_refdb_delete(ref->db, ref->name, NULL, NULL); } int git_reference_lookup(git_reference **ref_out, -- cgit v1.2.3 From f44fd59ed7b3533bf9cbaa07969a8a57475a77aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Wed, 5 Feb 2014 11:21:14 +0100 Subject: refs: check the ref's old value when deleting Recognize when the reference has changed since we loaded it. --- include/git2/refs.h | 5 ++++- src/refs.c | 10 +++++++++- tests/refs/races.c | 33 +++++++++++++++++++++++++++++++++ 3 files changed, 46 insertions(+), 2 deletions(-) diff --git a/include/git2/refs.h b/include/git2/refs.h index 284671d2c..0e5f779cf 100644 --- a/include/git2/refs.h +++ b/include/git2/refs.h @@ -398,8 +398,11 @@ GIT_EXTERN(int) git_reference_rename( * will be immediately removed on disk but the memory will not be freed. * Callers must call `git_reference_free`. * + * This function will return an error if the reference has changed + * from the time it was looked up. + * * @param ref The reference to remove - * @return 0 or an error code + * @return 0, GIT_EMODIFIED or an error code */ GIT_EXTERN(int) git_reference_delete(git_reference *ref); diff --git a/src/refs.c b/src/refs.c index f45c64246..90340d09c 100644 --- a/src/refs.c +++ b/src/refs.c @@ -116,7 +116,15 @@ void git_reference_free(git_reference *reference) int git_reference_delete(git_reference *ref) { - return git_refdb_delete(ref->db, ref->name, NULL, NULL); + const git_oid *old_id = NULL; + const char *old_target = NULL; + + if (ref->type == GIT_REF_OID) + old_id = &ref->target.oid; + else + old_target = ref->target.symbolic; + + return git_refdb_delete(ref->db, ref->name, old_id, old_target); } int git_reference_lookup(git_reference **ref_out, diff --git a/tests/refs/races.c b/tests/refs/races.c index 6613fc20f..396f13d2d 100644 --- a/tests/refs/races.c +++ b/tests/refs/races.c @@ -59,3 +59,36 @@ void test_refs_races__symbolic_create_matching(void) git_reference_free(ref2); git_reference_free(ref3); } + +void test_refs_races__delete(void) +{ + git_reference *ref, *ref2; + git_oid id, other_id; + + git_oid_fromstr(&id, commit_id); + git_oid_fromstr(&other_id, other_commit_id); + + /* We can delete a value that matches */ + cl_git_pass(git_reference_lookup(&ref, g_repo, refname)); + cl_git_pass(git_reference_delete(ref)); + git_reference_free(ref); + + /* We cannot delete a symbolic value that doesn't match */ + cl_git_pass(git_reference_lookup(&ref, g_repo, "HEAD")); + cl_git_pass(git_reference_symbolic_create_matching(&ref2, g_repo, "HEAD", other_refname, 1, NULL, NULL, refname)); + cl_git_fail_with(GIT_EMODIFIED, git_reference_delete(ref)); + + git_reference_free(ref); + git_reference_free(ref2); + + cl_git_pass(git_reference_create(&ref, g_repo, refname, &id, 1, NULL, NULL)); + git_reference_free(ref); + + /* We cannot delete an oid value that doesn't match */ + cl_git_pass(git_reference_lookup(&ref, g_repo, refname)); + cl_git_pass(git_reference_create_matching(&ref2, g_repo, refname, &other_id, 1, NULL, NULL, &id)); + cl_git_fail_with(GIT_EMODIFIED, git_reference_delete(ref)); + + git_reference_free(ref); + git_reference_free(ref2); +} -- cgit v1.2.3 From b7ae71ecf263047c427be099a3e1536cca17dc5d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Wed, 5 Feb 2014 11:47:33 +0100 Subject: refs: catch cases where the ref type has changed If the type of the on-disk reference has changed, the old value comparison should fail. --- src/refdb_fs.c | 18 ++++++++++++++--- tests/refs/races.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 73 insertions(+), 3 deletions(-) diff --git a/src/refdb_fs.c b/src/refdb_fs.c index c9f9c0f1c..ea758deea 100644 --- a/src/refdb_fs.c +++ b/src/refdb_fs.c @@ -937,9 +937,21 @@ static int cmp_old_ref(int *cmp, git_refdb_backend *backend, const char *name, git_reference *old_ref = NULL; *cmp = 0; - if (old_id || old_target) { - if ((error = refdb_fs_backend__lookup(&old_ref, backend, name)) < 0) - goto out; + /* It "matches" if there is no old value to compare against */ + if (!old_id && !old_target) + return 0; + + if ((error = refdb_fs_backend__lookup(&old_ref, backend, name)) < 0) + goto out; + + /* If the types don't match, there's no way the values do */ + if (old_id && old_ref->type != GIT_REF_OID) { + *cmp = -1; + goto out; + } + if (old_target && old_ref->type != GIT_REF_SYMBOLIC) { + *cmp = 1; + goto out; } if (old_id && old_ref->type == GIT_REF_OID) diff --git a/tests/refs/races.c b/tests/refs/races.c index 396f13d2d..02d57eff1 100644 --- a/tests/refs/races.c +++ b/tests/refs/races.c @@ -92,3 +92,61 @@ void test_refs_races__delete(void) git_reference_free(ref); git_reference_free(ref2); } + +void test_refs_races__switch_oid_to_symbolic(void) +{ + git_reference *ref, *ref2, *ref3; + git_oid id, other_id; + + git_oid_fromstr(&id, commit_id); + git_oid_fromstr(&other_id, other_commit_id); + + /* Removing a direct ref when it's currently symbolic should fail */ + cl_git_pass(git_reference_lookup(&ref, g_repo, refname)); + cl_git_pass(git_reference_symbolic_create(&ref2, g_repo, refname, other_refname, 1, NULL, NULL)); + cl_git_fail_with(GIT_EMODIFIED, git_reference_delete(ref)); + + git_reference_free(ref); + git_reference_free(ref2); + + cl_git_pass(git_reference_create(&ref, g_repo, refname, &id, 1, NULL, NULL)); + git_reference_free(ref); + + /* Updating a direct ref when it's currently symbolic should fail */ + cl_git_pass(git_reference_lookup(&ref, g_repo, refname)); + cl_git_pass(git_reference_symbolic_create(&ref2, g_repo, refname, other_refname, 1, NULL, NULL)); + cl_git_fail_with(GIT_EMODIFIED, git_reference_set_target(&ref3, ref, &other_id, NULL, NULL)); + + git_reference_free(ref); + git_reference_free(ref2); + git_reference_free(ref3); +} + +void test_refs_races__switch_symbolic_to_oid(void) +{ + git_reference *ref, *ref2, *ref3; + git_oid id, other_id; + + git_oid_fromstr(&id, commit_id); + git_oid_fromstr(&other_id, other_commit_id); + + /* Removing a symbolic ref when it's currently direct should fail */ + cl_git_pass(git_reference_lookup(&ref, g_repo, "HEAD")); + cl_git_pass(git_reference_create(&ref2, g_repo, "HEAD", &id, 1, NULL, NULL)); + cl_git_fail_with(GIT_EMODIFIED, git_reference_delete(ref)); + + git_reference_free(ref); + git_reference_free(ref2); + + cl_git_pass(git_reference_symbolic_create(&ref, g_repo, "HEAD", refname, 1, NULL, NULL)); + git_reference_free(ref); + + /* Updating a symbolic ref when it's currently direct should fail */ + cl_git_pass(git_reference_lookup(&ref, g_repo, "HEAD")); + cl_git_pass(git_reference_create(&ref2, g_repo, "HEAD", &id, 1, NULL, NULL)); + cl_git_fail_with(GIT_EMODIFIED, git_reference_symbolic_set_target(&ref3, ref, other_refname, NULL, NULL)); + + git_reference_free(ref); + git_reference_free(ref2); + git_reference_free(ref3); +} -- cgit v1.2.3 From 5367ec4b84dc3b4ff3ff441347ce07d6065dd759 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Wed, 5 Feb 2014 12:02:52 +0100 Subject: refs: add an unconditional delete Add it under the git_reference_remove() name, letting the user pass the repo and name, analogous to unconditional setting/creation. --- include/git2/refs.h | 11 +++++++++++ src/refs.c | 11 +++++++++++ tests/refs/delete.c | 14 ++++++++++++++ 3 files changed, 36 insertions(+) diff --git a/include/git2/refs.h b/include/git2/refs.h index 0e5f779cf..970faf744 100644 --- a/include/git2/refs.h +++ b/include/git2/refs.h @@ -406,6 +406,17 @@ GIT_EXTERN(int) git_reference_rename( */ GIT_EXTERN(int) git_reference_delete(git_reference *ref); +/** + * Delete an existing reference by name + * + * This method removes the named reference from the repository without + * looking at its old value. + * + * @param ref The reference to remove + * @return 0, GIT_EMODIFIED or an error code + */ +GIT_EXTERN(int) git_reference_remove(git_repository *repo, const char *name); + /** * Fill a list with all the references that can be found in a repository. * diff --git a/src/refs.c b/src/refs.c index 90340d09c..bdf2da37e 100644 --- a/src/refs.c +++ b/src/refs.c @@ -127,6 +127,17 @@ int git_reference_delete(git_reference *ref) return git_refdb_delete(ref->db, ref->name, old_id, old_target); } +int git_reference_remove(git_repository *repo, const char *name) +{ + git_refdb *db; + int error; + + if ((error = git_repository_refdb__weakptr(&db, repo)) < 0) + return error; + + return git_refdb_delete(db, name, NULL, NULL); +} + int git_reference_lookup(git_reference **ref_out, git_repository *repo, const char *name) { diff --git a/tests/refs/delete.c b/tests/refs/delete.c index 5e4afb138..9d1c3fd79 100644 --- a/tests/refs/delete.c +++ b/tests/refs/delete.c @@ -91,3 +91,17 @@ void test_refs_delete__packed_only(void) git_reference_free(ref); git_refdb_free(refdb); } + +void test_refs_delete__remove(void) +{ + git_reference *ref; + + /* Check that passing no old values lets us delete */ + + cl_git_pass(git_reference_lookup(&ref, g_repo, packed_test_head_name)); + git_reference_free(ref); + + cl_git_pass(git_reference_remove(g_repo, packed_test_head_name)); + + cl_git_fail(git_reference_lookup(&ref, g_repo, packed_test_head_name)); +} -- cgit v1.2.3