Skip to content

Add some more tests for git_futils_rmdir_r and some cleanup - #4828

Merged
ethomson merged 2 commits into
libgit2:masterfrom
csware:git_futils_rmdir_r_failing
Oct 7, 2018
Merged

Add some more tests for git_futils_rmdir_r and some cleanup#4828
ethomson merged 2 commits into
libgit2:masterfrom
csware:git_futils_rmdir_r_failing

Conversation

@csware

@csware csware commented Oct 2, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

Update: I cannot reproduce the issue any more which was the initial reason for this MR, however, this is a good extension for the test cases now...

old: When working on another issue I found that git_futils_rmdir_r does not delete an empty path hierarchy, just the last folder

@csware csware changed the title git_futils_rmdir_r does not delete empty path hierarchy WIP: git_futils_rmdir_r does not delete empty path hierarchy Oct 2, 2018
@pks-t

pks-t commented Oct 4, 2018

Copy link
Copy Markdown
Member

I've fixed your tests and also added a cleanup function that always removes left-over bits of the test directory hierarchy. With these changes, all tests pass just fine, and it seems to me like the empty directory hierarchy is being deleted just fine. Also, the following simple tests passes (__initialize creates an empty directory hierarchy by default):

void test_core_rmdir__can_remove_empty_dirs(void)
{
	cl_git_pass(git_futils_rmdir_r(empty_tmp_dir, NULL, GIT_RMDIR_SKIP_NONEMPTY));
	cl_assert(!git_path_exists(empty_tmp_dir));
}

csware and others added 2 commits October 5, 2018 13:58
@csware
tests: Add some more tests for git_futils_rmdir_r
e886ab4 
Signed-off-by: Sven Strickroth <email@cs-ware.de>
@pks-t @csware
tests: sanitize file hierarchy after running rmdir tests
ad27371 
Currently, we do not clean up after ourselves after tests in core::rmdir
have created new files in the directory hierarchy. This may leave stale
files and/or directories after having run tests, confusing subsequent
tests that expect a pristine test environment. Most importantly, it may
cause the test initialization to fail which expects being able to
re-create the testing hierarchy before each test in case where another
test hasn't cleaned up after itself.

Fix the issue by adding a cleanup function that removes the temporary
testing hierarchy after each test if it still exists.
@csware csware changed the title WIP: git_futils_rmdir_r does not delete empty path hierarchy Add some more tests for git_futils_rmdir_r and some cleanup Oct 5, 2018
@csware

csware commented Oct 5, 2018

Copy link
Copy Markdown
Contributor Author

I cannot reproduce the issue any more which was the initial reason for this MR, however, this is a good extension for the test cases now...

ethomson
ethomson reviewed Oct 5, 2018
View reviewed changes
Comment thread tests/core/rmdir.c
@pks-t

pks-t commented Oct 5, 2018

Copy link
Copy Markdown
Member

Agreed, @csware, we should just merge this even though it doesn't catch any errors right now.

@ethomson

ethomson commented Oct 7, 2018

Copy link
Copy Markdown
Member

Agreed here, too. Thanks for the PR!

@ethomson
ethomson merged commit 838a2f2 into libgit2:master Oct 7, 2018
@csware
csware deleted the git_futils_rmdir_r_failing branch October 10, 2018 09:23
@snyk-bot snyk-bot mentioned this pull request Feb 23, 2020
Open
@snyk-bot snyk-bot mentioned this pull request Apr 22, 2020
Open
@snyk-bot snyk-bot mentioned this pull request May 5, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ethomson ethomson ethomson left review comments
@pks-t pks-t pks-t left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@csware @pks-t @ethomson