Merge pull request #801 from nulltoken/fix/ref-renaming

refs and revparse love <3

include/git2/reflog.h

@@ -53,6 +53,8 @@ GIT_EXTERN(int) git_reflog_write(git_reference *ref, const git_oid *oid_old, con
 /**
  * Rename the reflog for the given reference
  *
+ * The reflog to be renamed is expected to already exist
+ *
  * @param ref the reference
  * @param new_name the new name of the reference
  * @return 0 or an error code

include/git2/refs.h

@@ -353,6 +353,16 @@ GIT_EXTERN(int) git_reference_foreach_glob(
 		void *payload
 );
 
+/**
+ * Check if a reflog exists for the specified reference.
+ *
+ * @param ref A git reference
+ *
+ * @return 0 when no reflog can be found, 1 when it exists;
+ * otherwise an error code.
+ */
+GIT_EXTERN(int) git_reference_has_log(git_reference *ref);
+
 /** @} */
 GIT_END_DECL
 #endif

src/fileops.c

@@ -94,7 +94,7 @@ int git_futils_open_ro(const char *path)
 {
 	int fd = p_open(path, O_RDONLY);
 	if (fd < 0) {
-		if (errno == ENOENT)
+		if (errno == ENOENT || errno == ENOTDIR)
 			fd = GIT_ENOTFOUND;
 		giterr_set(GITERR_OS, "Failed to open '%s'", path);
 	}

src/reflog.c

@@ -269,18 +269,50 @@ cleanup:
 
 int git_reflog_rename(git_reference *ref, const char *new_name)
 {
-	int error;
+	int error = -1, fd;
 	git_buf old_path = GIT_BUF_INIT;
 	git_buf new_path = GIT_BUF_INIT;
+	git_buf temp_path = GIT_BUF_INIT;
 
-	if (!git_buf_join_n(&old_path, '/', 3, ref->owner->path_repository,
-			GIT_REFLOG_DIR, ref->name) &&
-		!git_buf_join_n(&new_path, '/', 3, ref->owner->path_repository,
-			GIT_REFLOG_DIR, new_name))
-		error = p_rename(git_buf_cstr(&old_path), git_buf_cstr(&new_path));
-	else
-		error = -1;
+	assert(ref && new_name);
+
+	if (git_buf_joinpath(&temp_path, ref->owner->path_repository, GIT_REFLOG_DIR) < 0)
+		return -1;
+
+	if (git_buf_joinpath(&old_path, git_buf_cstr(&temp_path), ref->name) < 0)
+		goto cleanup;
+
+	if (git_buf_joinpath(&new_path, git_buf_cstr(&temp_path), new_name) < 0)
+		goto cleanup;
+
+	/*
+	 * Move the reflog to a temporary place. This two-phase renaming is required
+	 * in order to cope with funny renaming use cases when one tries to move a reference
+	 * to a partially colliding namespace:
+	 *  - a/b -> a/b/c
+	 *  - a/b/c/d -> a/b/c
+	 */
+	if (git_buf_joinpath(&temp_path, git_buf_cstr(&temp_path), "temp_reflog") < 0)
+		goto cleanup;
 
+	if ((fd = git_futils_mktmp(&temp_path, git_buf_cstr(&temp_path))) < 0)
+		goto cleanup;
+	p_close(fd);
+
+	if (p_rename(git_buf_cstr(&old_path), git_buf_cstr(&temp_path)) < 0)
+		goto cleanup;
+
+	if (git_path_isdir(git_buf_cstr(&new_path)) && 
+		(git_futils_rmdir_r(git_buf_cstr(&new_path), GIT_DIRREMOVAL_ONLY_EMPTY_DIRS) < 0))
+		goto cleanup;
+
+	if (git_futils_mkpath2file(git_buf_cstr(&new_path), GIT_REFLOG_DIR_MODE) < 0)
+		goto cleanup;
+
+	error = p_rename(git_buf_cstr(&temp_path), git_buf_cstr(&new_path));
+
+cleanup:
+	git_buf_free(&temp_path);
 	git_buf_free(&old_path);
 	git_buf_free(&new_path);
 

src/refs.c

@@ -1404,18 +1404,11 @@ int git_reference_rename(git_reference *ref, const char *new_name, int force)
 	}
 
 	/*
-	 * Rename the reflog file.
+	 * Rename the reflog file, if it exists.
 	 */
-	if (git_buf_join_n(&aux_path, '/', 3, ref->owner->path_repository, GIT_REFLOG_DIR, ref->name) < 0)
+	if ((git_reference_has_log(ref)) && (git_reflog_rename(ref, new_name) < 0))
 		goto cleanup;
 
-	if (git_path_exists(aux_path.ptr) == true) {
-		if (git_reflog_rename(ref, new_name) < 0)
-			goto cleanup;
-	} else {
-		giterr_clear();
-	}
-
 	/*
 	 * Change the name of the reference given by the user.
 	 */
@@ -1801,3 +1794,20 @@ int git_reference_foreach_glob(
 	return git_reference_foreach(
 			repo, list_flags, fromglob_cb, &data);
 }
+
+int git_reference_has_log(
+	git_reference *ref)
+{
+	git_buf path = GIT_BUF_INIT;
+	int result;
+
+	assert(ref);
+
+	if (git_buf_join_n(&path, '/', 3, ref->owner->path_repository, GIT_REFLOG_DIR, ref->name) < 0)
+		return -1;
+
+	result = git_path_isfile(git_buf_cstr(&path));
+	git_buf_free(&path);
+
+	return result;
+}

src/util.h

@@ -204,6 +204,11 @@ GIT_INLINE(bool) git__isalpha(int c)
     return ((c >= 'A' && c <= 'Z') || (c >= 'a' && c <= 'z'));
 }
 
+GIT_INLINE(bool) git__isdigit(int c)
+{
+    return (c >= '0' && c <= '9');
+}
+
 GIT_INLINE(bool) git__isspace(int c)
 {
     return (c == ' ' || c == '\t' || c == '\n' || c == '\f' || c == '\r' || c == '\v');

tests-clar/refs/read.c

@@ -192,3 +192,13 @@ void test_refs_read__loose_first(void)
 
 	git_reference_free(reference);
 }
+
+void test_refs_read__unfound_return_ENOTFOUND(void)
+{
+	git_reference *reference;
+
+	cl_assert_equal_i(GIT_ENOTFOUND, git_reference_lookup(&reference, g_repo, "test/master"));
+	cl_assert_equal_i(GIT_ENOTFOUND, git_reference_lookup(&reference, g_repo, "refs/test/master"));
+	cl_assert_equal_i(GIT_ENOTFOUND, git_reference_lookup(&reference, g_repo, "refs/tags/test/master"));
+	cl_assert_equal_i(GIT_ENOTFOUND, git_reference_lookup(&reference, g_repo, "refs/tags/test/farther/master"));
+}

tests-clar/refs/reflog.c

@@ -26,7 +26,7 @@ static void assert_signature(git_signature *expected, git_signature *actual)
 // Fixture setup and teardown
 void test_refs_reflog__initialize(void)
 {
-   g_repo = cl_git_sandbox_init("testrepo");
+   g_repo = cl_git_sandbox_init("testrepo.git");
 }
 
 void test_refs_reflog__cleanup(void)
@@ -61,7 +61,7 @@ void test_refs_reflog__write_then_read(void)
 	cl_git_pass(git_reflog_write(ref, &oid, committer, commit_msg));
 
 	/* Reopen a new instance of the repository */
-	cl_git_pass(git_repository_open(&repo2, "testrepo"));
+	cl_git_pass(git_repository_open(&repo2, "testrepo.git"));
 
 	/* Lookup the preivously created branch */
 	cl_git_pass(git_reference_lookup(&lookedup_ref, repo2, new_ref));
@@ -121,3 +121,44 @@ void test_refs_reflog__dont_write_bad(void)
 
 	git_reference_free(ref);
 }
+
+void test_refs_reflog__renaming_the_reference_moves_the_reflog(void)
+{
+	git_reference *master;
+	git_buf master_log_path = GIT_BUF_INIT, moved_log_path = GIT_BUF_INIT;
+
+	git_buf_joinpath(&master_log_path, git_repository_path(g_repo), GIT_REFLOG_DIR);
+	git_buf_puts(&moved_log_path, git_buf_cstr(&master_log_path));
+	git_buf_joinpath(&master_log_path, git_buf_cstr(&master_log_path), "refs/heads/master");
+	git_buf_joinpath(&moved_log_path, git_buf_cstr(&moved_log_path), "refs/moved");
+
+	cl_assert_equal_i(true, git_path_isfile(git_buf_cstr(&master_log_path)));
+	cl_assert_equal_i(false, git_path_isfile(git_buf_cstr(&moved_log_path)));
+
+	cl_git_pass(git_reference_lookup(&master, g_repo, "refs/heads/master"));
+	cl_git_pass(git_reference_rename(master, "refs/moved", 0));
+
+	cl_assert_equal_i(false, git_path_isfile(git_buf_cstr(&master_log_path)));
+	cl_assert_equal_i(true, git_path_isfile(git_buf_cstr(&moved_log_path)));
+
+	git_reference_free(master);
+	git_buf_free(&moved_log_path);
+	git_buf_free(&master_log_path);
+}
+static void assert_has_reflog(bool expected_result, const char *name)
+{
+	git_reference *ref;
+
+	cl_git_pass(git_reference_lookup(&ref, g_repo, name));
+
+	cl_assert_equal_i(expected_result, git_reference_has_log(ref));
+
+	git_reference_free(ref);
+}
+
+void test_refs_reflog__reference_has_reflog(void)
+{
+	assert_has_reflog(true, "HEAD");
+	assert_has_reflog(true, "refs/heads/master");
+	assert_has_reflog(false, "refs/heads/subtrees");
+}

tests-clar/refs/revparse.c

 #include "clar_libgit2.h"
 
 #include "git2/revparse.h"
+#include "buffer.h"
+#include "refs.h"
+#include "path.h"
 
 static git_repository *g_repo;
 static git_object *g_obj;
@@ -9,18 +12,28 @@ static char g_orig_tz[16] = {0};
 
 
 /* Helpers */
-static void test_object(const char *spec, const char *expected_oid)
+static void test_object_inrepo(const char *spec, const char *expected_oid, git_repository *repo)
 {
 	char objstr[64] = {0};
+	git_object *obj = NULL;
+	int error;
 
-	cl_git_pass(git_revparse_single(&g_obj, g_repo, spec));
-	git_oid_fmt(objstr, git_object_id(g_obj));
-	cl_assert_equal_s(objstr, expected_oid);
+	error = git_revparse_single(&obj, repo, spec);
 
-	git_object_free(g_obj);
-	g_obj = NULL;
+	if (expected_oid != NULL) {
+		cl_assert_equal_i(0, error);
+		git_oid_fmt(objstr, git_object_id(obj));
+		cl_assert_equal_s(objstr, expected_oid);
+	} else
+		cl_assert_equal_i(GIT_ENOTFOUND, error);
+
+	git_object_free(obj);
 }
 
+static void test_object(const char *spec, const char *expected_oid)
+{
+	test_object_inrepo(spec, expected_oid, g_repo);
+}
 
 void test_refs_revparse__initialize(void)
 {
@@ -28,21 +41,29 @@ void test_refs_revparse__initialize(void)
 	if (tz)
 		strcpy(g_orig_tz, tz);
 	cl_setenv("TZ", "UTC");
-	g_repo = cl_git_sandbox_init("testrepo.git");
+
+	cl_git_pass(git_repository_open(&g_repo, cl_fixture("testrepo.git")));
 }
 
 void test_refs_revparse__cleanup(void)
 {
-	cl_git_sandbox_cleanup();
+	git_repository_free(g_repo);
 	g_obj = NULL;
 	cl_setenv("TZ", g_orig_tz);
 }
 
 void test_refs_revparse__nonexistant_object(void)
 {
-	cl_assert_equal_i(GIT_ENOTFOUND, git_revparse_single(&g_obj, g_repo, "this doesn't exist"));
-	cl_git_fail(git_revparse_single(&g_obj, g_repo, "this doesn't exist^1"));
-	cl_git_fail(git_revparse_single(&g_obj, g_repo, "this doesn't exist~2"));
+	test_object("this-does-not-exist", NULL);
+	test_object("this-does-not-exist^1", NULL);
+	test_object("this-does-not-exist~2", NULL);
+}
+
+void test_refs_revparse__invalid_reference_name(void)
+{
+	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"));
 }
 
 void test_refs_revparse__shas(void)
@@ -85,7 +106,7 @@ void test_refs_revparse__nth_parent(void)
 	test_object("be3563a^2^1", "5b5b025afb0b4c913b4c338a42934a3863bf3644");
 	test_object("be3563a^0", "be3563ae3f795b2b4353bcce3a527ad0a4f7f644");
 
-	cl_assert_equal_i(GIT_ENOTFOUND, git_revparse_single(&g_obj, g_repo, "be3563a^42"));
+	test_object("be3563a^42", NULL);
 }
 
 void test_refs_revparse__not_tag(void)
@@ -122,23 +143,92 @@ void test_refs_revparse__chaining(void)
 	test_object("master^1^1^1^1^1", "8496071c1b46c854b31185ea97743be6a8774479");
 }
 
-void test_refs_revparse__reflog(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}"));
+
+	test_object("master@{upstream}", "be3563ae3f795b2b4353bcce3a527ad0a4f7f644");
+	test_object("@{u}", "be3563ae3f795b2b4353bcce3a527ad0a4f7f644");
+	test_object("master@{u}", "be3563ae3f795b2b4353bcce3a527ad0a4f7f644");
+	test_object("heads/master@{u}", "be3563ae3f795b2b4353bcce3a527ad0a4f7f644");
+	test_object("refs/heads/master@{u}", "be3563ae3f795b2b4353bcce3a527ad0a4f7f644");
+}
+
+void test_refs_revparse__ordinal(void)
+{
+	cl_git_fail(git_revparse_single(&g_obj, g_repo, "master@{-2}"));
+
+	test_object("nope@{0}", NULL);
+	test_object("master@{31415}", NULL);
+	test_object("@{1000}", NULL);
+	test_object("@{2}", NULL);
+
+	test_object("@{0}", "a65fedf39aefe402d3bb6e24df4d4f5fe4547750");
+	test_object("@{1}", "be3563ae3f795b2b4353bcce3a527ad0a4f7f644");
+
+	test_object("master@{0}", "a65fedf39aefe402d3bb6e24df4d4f5fe4547750");
+	test_object("master@{1}", "be3563ae3f795b2b4353bcce3a527ad0a4f7f644");
+	test_object("heads/master@{1}", "be3563ae3f795b2b4353bcce3a527ad0a4f7f644");
+	test_object("refs/heads/master@{1}", "be3563ae3f795b2b4353bcce3a527ad0a4f7f644");
+}
+
+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, "@{1000}"));
 
-	cl_assert_equal_i(GIT_ENOTFOUND, git_revparse_single(&g_obj, g_repo, "nope@{0}"));
-	cl_assert_equal_i(GIT_ENOTFOUND, git_revparse_single(&g_obj, g_repo, "master@{31415}"));
+	test_object("@{-42}", NULL);
 
 	test_object("@{-2}", "a65fedf39aefe402d3bb6e24df4d4f5fe4547750");
 	test_object("@{-1}", "a4a7dce85cf63874e984719f4fdd239f5145052f");
-	test_object("master@{0}", "a65fedf39aefe402d3bb6e24df4d4f5fe4547750");
-	test_object("master@{1}", "be3563ae3f795b2b4353bcce3a527ad0a4f7f644");
-	test_object("@{0}", "a65fedf39aefe402d3bb6e24df4d4f5fe4547750");
-	test_object("@{1}", "be3563ae3f795b2b4353bcce3a527ad0a4f7f644");
-	test_object("master@{upstream}", "be3563ae3f795b2b4353bcce3a527ad0a4f7f644");
-	test_object("master@{u}", "be3563ae3f795b2b4353bcce3a527ad0a4f7f644");
+}
+
+static void create_fake_stash_reference_and_reflog(git_repository *repo)
+{
+	git_reference *master;
+	git_buf log_path = GIT_BUF_INIT;
+
+	git_buf_joinpath(&log_path, git_repository_path(repo), "logs/refs/fakestash");
+
+	cl_assert_equal_i(false, git_path_isfile(git_buf_cstr(&log_path)));
+
+	cl_git_pass(git_reference_lookup(&master, repo, "refs/heads/master"));
+	cl_git_pass(git_reference_rename(master, "refs/fakestash", 0));
+
+	cl_assert_equal_i(true, git_path_isfile(git_buf_cstr(&log_path)));
+
+	git_buf_free(&log_path);
+	git_reference_free(master);
+}
+
+void test_refs_revparse__reflog_of_a_ref_under_refs(void)
+{
+	git_repository *repo = cl_git_sandbox_init("testrepo.git");
+
+	test_object_inrepo("refs/fakestash", NULL, repo);
+
+	create_fake_stash_reference_and_reflog(repo);
+
+	/*
+	 * $ git reflog -1 refs/fakestash
+	 * a65fedf refs/fakestash@{0}: commit: checking in
+	 *
+	 * $ git reflog -1 refs/fakestash@{0}
+	 * a65fedf refs/fakestash@{0}: commit: checking in
+	 *
+	 * $ git reflog -1 fakestash
+	 * a65fedf fakestash@{0}: commit: checking in
+	 *
+	 * $ git reflog -1 fakestash@{0}
+	 * a65fedf fakestash@{0}: commit: checking in
+	 */
+	test_object_inrepo("refs/fakestash", "a65fedf39aefe402d3bb6e24df4d4f5fe4547750", repo);
+	test_object_inrepo("refs/fakestash@{0}", "a65fedf39aefe402d3bb6e24df4d4f5fe4547750", repo);
+	test_object_inrepo("fakestash", "a65fedf39aefe402d3bb6e24df4d4f5fe4547750", repo);
+	test_object_inrepo("fakestash@{0}", "a65fedf39aefe402d3bb6e24df4d4f5fe4547750", repo);
+
+	cl_git_sandbox_cleanup();
 }
 
 void test_refs_revparse__revwalk(void)
@@ -170,7 +260,7 @@ void test_refs_revparse__date(void)
 	 * a65fedf HEAD@{1335806603 -0900}: commit:
 	 * be3563a HEAD@{1335806563 -0700}: clone: from /Users/ben/src/libgit2/tests/resour
 	 */
-	cl_assert_equal_i(GIT_ENOTFOUND, git_revparse_single(&g_obj, g_repo, "HEAD@{10 years ago}"));
+	test_object("HEAD@{10 years ago}", NULL);
 
 	test_object("HEAD@{1 second}", "a65fedf39aefe402d3bb6e24df4d4f5fe4547750");
 	test_object("HEAD@{1 second ago}", "a65fedf39aefe402d3bb6e24df4d4f5fe4547750");
@@ -191,8 +281,8 @@ void test_refs_revparse__date(void)
 	 * $ git reflog -1 "master@{2012-04-30 17:22:42 +0000}"
 	 * warning: Log for 'master' only goes back to Mon, 30 Apr 2012 09:22:43 -0800.
 	 */
-	cl_assert_equal_i(GIT_ENOTFOUND, git_revparse_single(&g_obj, g_repo, "master@{2012-04-30 17:22:42 +0000}"));
-	cl_assert_equal_i(GIT_ENOTFOUND, git_revparse_single(&g_obj, g_repo, "master@{2012-04-30 09:22:42 -0800}"));
+	test_object("master@{2012-04-30 17:22:42 +0000}", NULL);
+	test_object("master@{2012-04-30 09:22:42 -0800}", NULL);
 
 	/*
 	 * $ git reflog -1 "master@{2012-04-30 17:22:43 +0000}"
@@ -229,11 +319,11 @@ 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"));
 
-	cl_assert_equal_i(GIT_ENOTFOUND, git_revparse_single(&g_obj, g_repo, ":/not found in any commit"));
-	cl_assert_equal_i(GIT_ENOTFOUND, git_revparse_single(&g_obj, g_repo, "subtrees:ab/42.txt"));
-	cl_assert_equal_i(GIT_ENOTFOUND, git_revparse_single(&g_obj, g_repo, "subtrees:ab/4.txt/nope"));
-	cl_assert_equal_i(GIT_ENOTFOUND, git_revparse_single(&g_obj, g_repo, "subtrees:nope"));
-	cl_assert_equal_i(GIT_ENOTFOUND, git_revparse_single(&g_obj, g_repo, "test/master^1:branch_file.txt"));
+	test_object(":/not found in any commit", NULL);
+	test_object("subtrees:ab/42.txt", NULL);
+	test_object("subtrees:ab/4.txt/nope", NULL);
+	test_object("subtrees:nope", NULL);
+	test_object("test/master^1:branch_file.txt", NULL);
 
 	/* Trees */
 	test_object("master:", "944c0f6e4dfa41595e6eb3ceecdb14f50fe18162");