Remove empty (sub-)directories when deleting refs #4833

csware · 2018-10-05T11:30:23Z

Fixes issue #4806.

pks-t

Thanks for tackling this! I've accidentally also implemented a fix yesterday, but didn't yet upload it. I think essentially, both fixes boiled down to the same thing: using the rmdir_r function that's already available. I already got enough PRs open right now that I have to handle, so I'm going to favour yours :)

pks-t · 2018-10-05T12:42:21Z

src/refdb_fs.c

+	git_buf base_path = GIT_BUF_INIT;
+	git_buf relative_path = GIT_BUF_INIT;
+	int just_delete = 1;
+	int error = 0;


No need to initialize error = 0, as you immediately set it in the first statement

pks-t · 2018-10-05T12:42:32Z

src/refdb_fs.c

+	const char *ref_name,
+	const char *ref_path,
+	bool reflog
+)


The opening bracket belongs on the last line

pks-t · 2018-10-05T12:46:02Z

src/refdb_fs.c

+
+		while (*start_ptr == '/') /* drop multiple path separators */
+			++start_ptr;
+	}


I think this is a bit too lenient. E.g. if I'm creating a thing "foo/bar/x", it will just skip "foo/bar". I think we should instead explicitly special-case precious directories:

git_buf_sets(&buf, ref_name); git_path_squash_slashes(&buf); if ((commonlen = git_path_common_dirlen("refs/heads/", buf.ptr)) || (commonlen = git_path_common_dirlen("refs/tags/", buf.ptr)) || ...) { git_buf base = GIT_BUF_INIT; git_buf_printf(&base, "%s/%.*s", backend->commonpath, commonlen, buf.ptr); git_futils_rmdir_r(ref_path + commonlen, base.ptr, GIT_RMDIR_EMPTY_PARENTS|GIT_RMDIR_REMOVE_FILES|GIT_RMDIR_SKIP_ROOT); } else { p_unlink(ref_path); }

Something like the above. It's completely untested and out of my head, but I guess it's sufficient to demonstrate what I imagine.

pks-t · 2018-10-05T12:56:15Z

src/refdb_fs.c

 		goto cleanup;
 	}

-	error = packed_write(backend);
+	/* unlock here, so that we can delete the all empty parents */
+	git_filebuf_cleanup(file);


We're cleaning up file twice now, aren't we?

We need to close the lock file before we can remove the possibly empty parents, otherwise it won't work. And we still need it in the cleanup path. As cleanup disposes the internal buffer, calling it twice doesn't cause problems.

pks-t · 2018-10-05T12:57:32Z

src/refdb_fs.c

+	/* unlock here, so that we can delete the all empty parents */
+	git_filebuf_cleanup(file);
+
+	error = refdb_fs_backend__delete_empty_parents(backend, ref_name, git_buf_cstr(&loose_path), false);


I think we should rename this function. It doesn't only remove parents, but also the reference itself.

csware · 2018-10-05T13:25:44Z

@pks-t Updated...

tiennou · 2018-10-08T18:17:33Z

/rebuild

libgit2-azure-pipelines · 2018-10-08T18:17:38Z

Okay, @tiennou, I started to rebuild this pull request.

csware · 2018-10-09T10:38:17Z

/rebuild

libgit2-azure-pipelines · 2018-10-09T10:38:19Z

Sorry @csware, you're not allowed to do that.

pks-t · 2018-10-11T11:05:58Z

@ethomson: shouldn't PR owners be allowed to rebuild their own PRs?

ethomson · 2018-10-11T11:38:39Z

Sure, I'll track that at ethomson/probot-azure-pipelines#5

…

On Thu, Oct 11, 2018 at 12:06 PM Patrick Steinhardt < ***@***.***> wrote: @ethomson <https://github.com/ethomson>: shouldn't PR owners be allowed to rebuild their own PRs? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4833 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABE-HsQKbGIQigUpQvWkZl0fctQq38JSks5ujyYfgaJpZM4XKDLJ> .

csware · 2018-10-11T20:46:42Z

@pks-t Any new comments?

pks-t · 2018-10-12T10:31:20Z

Huh, CI is still failing. Let's try it again

pks-t · 2018-10-12T10:31:28Z

/rebuild

libgit2-azure-pipelines · 2018-10-12T10:31:32Z

Okay, @pks-t, I started to rebuild this pull request.

pks-t · 2018-10-12T10:35:50Z

@ethomson: seems like CI is currently having problems?

ethomson · 2018-10-12T10:48:18Z

The CI is not having problems, the code simply doesn't compile.

csware · 2018-10-12T10:49:37Z

The CI is not having problems, the code simply doesn't compile.

I see Docker issues:

Unable to find image 'libgit2/trusty-mbedtls:latest' locally
latest: Pulling from libgit2/trusty-mbedtls
28bfaceaff9b: Pulling fs layer
Pulling fs layer
2965585ef8b8: Pulling fs layer
2416bb9f3ad2: Pulling fs layer
93b55a6a6807: Pulling fs layer
b1f4f03bf07c: Pulling fs layer
dfd74b70ab8f: Pulling fs layer
e7a3b540949e: Pulling fs layer
Pulling fs layer

ethomson · 2018-10-12T10:49:51Z

2018-10-12T10:33:54.5086580Z /Users/vsts/agent/2.140.2/work/1/s/tests/refs/branches/delete.c:154:12: error: implicit declaration of function 'git_path_exists' [-Werror,-Wimplicit-function-declaration]
2018-10-12T10:33:54.5086830Z         cl_assert(git_path_exists(git_buf_cstr(&ref_folder)) == true);
2018-10-12T10:33:54.5086960Z                   ^
2018-10-12T10:33:54.5107770Z /Users/vsts/agent/2.140.2/work/1/s/tests/refs/branches/delete.c:154:12: note: did you mean 'git_odb_exists'?
2018-10-12T10:33:54.5109090Z /Users/vsts/agent/2.140.2/work/1/s/include/git2/odb.h:160:17: note: 'git_odb_exists' declared here
2018-10-12T10:33:54.5109230Z GIT_EXTERN(int) git_odb_exists(git_odb *db, const git_oid *id);
2018-10-12T10:33:54.5109280Z                 ^
2018-10-12T10:33:54.5110210Z /Users/vsts/agent/2.140.2/work/1/s/tests/refs/branches/delete.c:154:12: error: this function declaration is not a prototype [-Werror,-Wstrict-prototypes]
2018-10-12T10:33:54.5110330Z         cl_assert(git_path_exists(git_buf_cstr(&ref_folder)) == true);
2018-10-12T10:33:54.5110690Z                   ^
2018-10-12T10:33:54.5122930Z 2 errors generated.

ethomson · 2018-10-12T10:50:22Z

I see Docker issues:

It's reporting the first things that hit stderr. Expand the section.

pks-t · 2018-10-12T10:56:40Z

Ooh, that's why. I've also only spotted the Docker issues, didn't even think it might hide the relevant information.

ethomson · 2018-10-12T11:02:23Z

Yeah, unfortunately since we're running inside docker we can't just push all its noisy stderr to stdout, since we'd also be doing that for the things being executed in the container. :/ But I'm going to investigate this a big more since having the relevant problems up-front (and not "behind the fold") would be a big improvement.

neithernut · 2018-10-12T13:59:09Z

tests/refs/branches/delete.c

+	cl_git_pass(git_oid_fromstr(&commit_id, "a65fedf39aefe402d3bb6e24df4d4f5fe4547750"));
+	cl_git_pass(git_commit_lookup(&commit, repo, &commit_id));
+	cl_git_pass(git_branch_create(&branch, repo, "some/deep/ref", commit, 0));
+	git_commit_free(&commit);


Ci says:

/src/tests/refs/branches/delete.c: In function 'test_refs_branches_delete__removes_empty_folders': /src/tests/refs/branches/delete.c:163:2: warning: passing argument 1 of 'git_commit_free' from incompatible pointer type [enabled by default] git_commit_free(&commit); ^ In file included from /src/include/git2.h:20:0, from /src/tests/clar_libgit2.h:5, from /src/tests/refs/branches/delete.c:1: /src/include/git2/commit.h:70:18: note: expected 'struct git_commit *' but argument is of type 'struct git_commit **' GIT_EXTERN(void) git_commit_free(git_commit *commit); ^

So you probably want to pass commit rather than &commit.

I wonder why this doesn't result in a compilation failure...

Signed-off-by: Sven Strickroth <email@cs-ware.de>

csware · 2018-10-16T11:34:07Z

@pks-t
All issues fixed.

pks-t · 2018-12-19T10:01:51Z

Indeed they are. Thanks for being patient and for your work!

csware changed the title ~~WIP: Remove empty directories when deleting refs~~ Remove empty directories when deleting refs Oct 5, 2018

csware changed the title ~~Remove empty directories when deleting refs~~ Remove empty (sub-)directories when deleting refs Oct 5, 2018

pks-t requested changes Oct 5, 2018

View reviewed changes

csware changed the title ~~Remove empty (sub-)directories when deleting refs~~ WIP: Remove empty (sub-)directories when deleting refs Oct 12, 2018

csware changed the title ~~WIP: Remove empty (sub-)directories when deleting refs~~ Remove empty (sub-)directories when deleting refs Oct 12, 2018

neithernut reviewed Oct 12, 2018

View reviewed changes

Remove empty directories when deleting refs

9f48dc3

Signed-off-by: Sven Strickroth <email@cs-ware.de>

pks-t approved these changes Dec 19, 2018

View reviewed changes

pks-t merged commit bc21965 into libgit2:master Dec 19, 2018

csware deleted the drop-empty-dirs branch December 20, 2018 12:55

snyk-bot mentioned this pull request Feb 23, 2020

[Snyk] Upgrade nodegit from 0.4.1 to 0.26.4 saurabharch/Breezeblocks#1

Open

snyk-bot mentioned this pull request Apr 22, 2020

[Snyk] Upgrade nodegit from 0.24.3 to 0.26.5 aminatakonate000/Graviton-App#4

Open

snyk-bot mentioned this pull request May 5, 2020

[Snyk] Upgrade nodegit from 0.24.3 to 0.26.5 Barnstorm-Online/ngp-openapi-generator#1

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove empty (sub-)directories when deleting refs #4833

Remove empty (sub-)directories when deleting refs #4833

csware commented Oct 5, 2018

pks-t left a comment

pks-t Oct 5, 2018

pks-t Oct 5, 2018

pks-t Oct 5, 2018

pks-t Oct 5, 2018

csware Oct 5, 2018

pks-t Oct 5, 2018

csware commented Oct 5, 2018

tiennou commented Oct 8, 2018

libgit2-azure-pipelines bot commented Oct 8, 2018

csware commented Oct 9, 2018

libgit2-azure-pipelines bot commented Oct 9, 2018

pks-t commented Oct 11, 2018

ethomson commented Oct 11, 2018 via email

csware commented Oct 11, 2018

pks-t commented Oct 12, 2018

pks-t commented Oct 12, 2018

libgit2-azure-pipelines bot commented Oct 12, 2018

pks-t commented Oct 12, 2018

ethomson commented Oct 12, 2018

csware commented Oct 12, 2018

ethomson commented Oct 12, 2018

ethomson commented Oct 12, 2018

pks-t commented Oct 12, 2018

ethomson commented Oct 12, 2018

neithernut Oct 12, 2018 •

edited

csware commented Oct 16, 2018

pks-t commented Dec 19, 2018

Remove empty (sub-)directories when deleting refs #4833

Remove empty (sub-)directories when deleting refs #4833

Conversation

csware commented Oct 5, 2018

pks-t left a comment

Choose a reason for hiding this comment

pks-t Oct 5, 2018

Choose a reason for hiding this comment

pks-t Oct 5, 2018

Choose a reason for hiding this comment

pks-t Oct 5, 2018

Choose a reason for hiding this comment

pks-t Oct 5, 2018

Choose a reason for hiding this comment

csware Oct 5, 2018

Choose a reason for hiding this comment

pks-t Oct 5, 2018

Choose a reason for hiding this comment

csware commented Oct 5, 2018

tiennou commented Oct 8, 2018

libgit2-azure-pipelines bot commented Oct 8, 2018

csware commented Oct 9, 2018

libgit2-azure-pipelines bot commented Oct 9, 2018

pks-t commented Oct 11, 2018

ethomson commented Oct 11, 2018 via email

csware commented Oct 11, 2018

pks-t commented Oct 12, 2018

pks-t commented Oct 12, 2018

libgit2-azure-pipelines bot commented Oct 12, 2018

pks-t commented Oct 12, 2018

ethomson commented Oct 12, 2018

csware commented Oct 12, 2018

ethomson commented Oct 12, 2018

ethomson commented Oct 12, 2018

pks-t commented Oct 12, 2018

ethomson commented Oct 12, 2018

neithernut Oct 12, 2018 • edited

Choose a reason for hiding this comment

csware commented Oct 16, 2018

pks-t commented Dec 19, 2018

neithernut Oct 12, 2018 •

edited