Merge pull request #3499 from ethomson/ref_dir_errmsgs

Improve error messages when dirs prevent ref/reflog creation

src/filebuf.c

@@ -357,6 +357,12 @@ int git_filebuf_open(git_filebuf *file, const char *path, int flags, mode_t mode
 		memcpy(file->path_lock, file->path_original, path_len);
 		memcpy(file->path_lock + path_len, GIT_FILELOCK_EXTENSION, GIT_FILELOCK_EXTLENGTH);
 
+		if (git_path_isdir(file->path_original)) {
+			giterr_set(GITERR_FILESYSTEM, "path '%s' is a directory", file->path_original);
+			error = GIT_EDIRECTORY;
+			goto cleanup;
+		}
+
 		/* open the file for locking */
 		if ((error = lock_file(file, flags, mode)) < 0)
 			goto cleanup;

src/refdb_fs.c

@@ -733,8 +733,11 @@ static int loose_lock(git_filebuf *file, refdb_fs_backend *backend, const char *
 
 	error = git_filebuf_open(file, ref_path.ptr, GIT_FILEBUF_FORCE, GIT_REFS_FILE_MODE);
 
+	if (error == GIT_EDIRECTORY)
+		giterr_set(GITERR_REFERENCE, "cannot lock ref '%s', there are refs beneath that folder", name);
+
 	git_buf_free(&ref_path);
-        return error;
+	return error;
 }
 
 static int loose_commit(git_filebuf *file, const git_reference *ref)
@@ -1785,10 +1788,17 @@ static int reflog_append(refdb_fs_backend *backend, const git_reference *ref, co
 	/* If the new branch matches part of the namespace of a previously deleted branch,
 	 * there maybe an obsolete/unused directory (or directory hierarchy) in the way.
 	 */
-	if (git_path_isdir(git_buf_cstr(&path)) &&
-		(git_futils_rmdir_r(git_buf_cstr(&path), NULL, GIT_RMDIR_SKIP_NONEMPTY) < 0)) {
-		error = -1;
-		goto cleanup;
+	if (git_path_isdir(git_buf_cstr(&path))) {
+		if ((git_futils_rmdir_r(git_buf_cstr(&path), NULL, GIT_RMDIR_SKIP_NONEMPTY) < 0))
+			error = -1;
+		else if (git_path_isdir(git_buf_cstr(&path))) {
+			giterr_set(GITERR_REFERENCE, "cannot create reflog at '%s', there are reflogs beneath that folder",
+				ref->name);
+			error = GIT_EDIRECTORY;
+		}
+
+		if (error != 0)
+			goto cleanup;
 	}
 
 	error = git_futils_writebuffer(&buf, git_buf_cstr(&path), O_WRONLY|O_CREAT|O_APPEND, GIT_REFLOG_FILE_MODE);

tests/core/filebuf.c

@@ -230,3 +230,12 @@ void test_core_filebuf__hidden_file(void)
 	git_filebuf_cleanup(&file);
 #endif
 }
+
+void test_core_filebuf__detects_directory(void)
+{
+	git_filebuf file = GIT_FILEBUF_INIT, fail = GIT_FILEBUF_INIT;
+
+	cl_must_pass(p_mkdir("foo", 0777));
+	cl_git_fail_with(GIT_EDIRECTORY, git_filebuf_open(&file, "foo", 0, 0666));
+	cl_must_pass(p_rmdir("foo"));
+}

tests/refs/create.c

@@ -151,6 +151,23 @@ void test_refs_create__propagate_eexists(void)
 	cl_assert(error == GIT_EEXISTS);
 }
 
+void test_refs_create__existing_dir_propagates_edirectory(void)
+{
+	git_reference *new_reference, *fail_reference;
+	git_oid id;
+	const char *dir_head = "refs/heads/new-dir/new-head",
+		*fail_head = "refs/heads/new-dir";
+
+	git_oid_fromstr(&id, current_master_tip);
+
+	/* Create and write the new object id reference */
+	cl_git_pass(git_reference_create(&new_reference, g_repo, dir_head, &id, 1, NULL));
+	cl_git_fail_with(GIT_EDIRECTORY,
+		git_reference_create(&fail_reference, g_repo, fail_head, &id, false, NULL));
+
+	git_reference_free(new_reference);
+}
+
 static void test_invalid_name(const char *name)
 {
 	git_reference *new_reference;

tests/refs/reflog/reflog.c

@@ -125,6 +125,77 @@ void test_refs_reflog_reflog__renaming_the_reference_moves_the_reflog(void)
 	git_buf_free(&master_log_path);
 }
 
+void test_refs_reflog_reflog__deleting_the_reference_deletes_the_reflog(void)
+{
+	git_reference *master;
+	git_buf master_log_path = GIT_BUF_INIT;
+
+	git_buf_joinpath(&master_log_path, git_repository_path(g_repo), GIT_REFLOG_DIR);
+	git_buf_joinpath(&master_log_path, git_buf_cstr(&master_log_path), "refs/heads/master");
+
+	cl_assert_equal_i(true, git_path_isfile(git_buf_cstr(&master_log_path)));
+
+	cl_git_pass(git_reference_lookup(&master, g_repo, "refs/heads/master"));
+	cl_git_pass(git_reference_delete(master));
+	git_reference_free(master);
+
+	cl_assert_equal_i(false, git_path_isfile(git_buf_cstr(&master_log_path)));
+	git_buf_free(&master_log_path);
+}
+
+void test_refs_reflog_reflog__removes_empty_reflog_dir(void)
+{
+	git_reference *ref;
+	git_buf log_path = GIT_BUF_INIT;
+	git_oid id;
+
+	/* Create a new branch pointing at the HEAD */
+	git_oid_fromstr(&id, current_master_tip);
+	cl_git_pass(git_reference_create(&ref, g_repo, "refs/heads/new-dir/new-head", &id, 0, NULL));
+
+	git_buf_joinpath(&log_path, git_repository_path(g_repo), GIT_REFLOG_DIR);
+	git_buf_joinpath(&log_path, git_buf_cstr(&log_path), "refs/heads/new-dir/new-head");
+
+	cl_assert_equal_i(true, git_path_isfile(git_buf_cstr(&log_path)));
+
+	cl_git_pass(git_reference_delete(ref));
+	git_reference_free(ref);
+
+	/* new ref creation should succeed since new-dir is empty */
+	git_oid_fromstr(&id, current_master_tip);
+	cl_git_pass(git_reference_create(&ref, g_repo, "refs/heads/new-dir", &id, 0, NULL));
+	git_reference_free(ref);
+
+	git_buf_free(&log_path);
+}
+
+void test_refs_reflog_reflog__fails_gracefully_on_nonempty_reflog_dir(void)
+{
+	git_reference *ref;
+	git_buf log_path = GIT_BUF_INIT;
+	git_oid id;
+
+	/* Create a new branch pointing at the HEAD */
+	git_oid_fromstr(&id, current_master_tip);
+	cl_git_pass(git_reference_create(&ref, g_repo, "refs/heads/new-dir/new-head", &id, 0, NULL));
+	git_reference_free(ref);
+
+	git_buf_joinpath(&log_path, git_repository_path(g_repo), GIT_REFLOG_DIR);
+	git_buf_joinpath(&log_path, git_buf_cstr(&log_path), "refs/heads/new-dir/new-head");
+
+	cl_assert_equal_i(true, git_path_isfile(git_buf_cstr(&log_path)));
+
+	/* delete the ref manually, leave the reflog */
+	cl_must_pass(p_unlink("testrepo.git/refs/heads/new-dir/new-head"));
+
+	/* new ref creation should fail since new-dir contains reflogs still */
+	git_oid_fromstr(&id, current_master_tip);
+	cl_git_fail_with(GIT_EDIRECTORY, git_reference_create(&ref, g_repo, "refs/heads/new-dir", &id, 0, NULL));
+	git_reference_free(ref);
+
+	git_buf_free(&log_path);
+}
+
 static void assert_has_reflog(bool expected_result, const char *name)
 {
 	cl_assert_equal_i(expected_result, git_reference_has_log(g_repo, name));