threads / patch / 45580

patch[GSOC] prune_worktrees(): reimplement with dir_iterator

Subject: [PATCH] [GSOC] prune_worktrees(): reimplement with dir_iterator

## tl;dr

4 messages between Apr 1, 2017 and Apr 1, 2017. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Robert Stanca· Apr 1, 2017, 01:21 UTC · lore
Replaces recursive traversing of opendir with dir_iterator
Signed-off-by: Robert Stanca <robert.stanca7@gmail.com>
---
 builtin/worktree.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)
Show changes to builtin/worktree.c +7 −10
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 9993ded..7cfd78c 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -10,6 +10,8 @@
 #include "refs.h"
 #include "utf8.h"
 #include "worktree.h"
+#include "iterator.h"
+#include "dir-iterator.h"
 
 static const char * const worktree_usage[] = {
 	N_("git worktree add [<options>] <path> [<branch>]"),
@@ -91,30 +93,25 @@ static void prune_worktrees(void)
 {
 	struct strbuf reason = STRBUF_INIT;
 	struct strbuf path = STRBUF_INIT;
-	DIR *dir = opendir(git_path("worktrees"));
-	struct dirent *d;
+	struct dir_iterator *diter = dir_iterator_begin(git_path("worktrees"));
 	int ret;
-	if (!dir)
-		return;
-	while ((d = readdir(dir)) != NULL) {
-		if (is_dot_or_dotdot(d->d_name))
-			continue;
+
+	while (dir_iterator_advance(diter) == ITER_OK) {
 		strbuf_reset(&reason);
-		if (!prune_worktree(d->d_name, &reason))
+		if (!prune_worktree(diter->relative_path, &reason))
 			continue;
 		if (show_only || verbose)
 			printf("%s\n", reason.buf);
 		if (show_only)
 			continue;
 		strbuf_reset(&path);
-		strbuf_addstr(&path, git_path("worktrees/%s", d->d_name));
+		strbuf_addstr(&path, git_path("worktrees/%s", diter->relative_path));
 		ret = remove_dir_recursively(&path, 0);
 		if (ret < 0 && errno == ENOTDIR)
 			ret = unlink(path.buf);
 		if (ret)
 			error_errno(_("failed to remove '%s'"), path.buf);
 	}
-	closedir(dir);
 	if (!show_only)
 		rmdir(git_path("worktrees"));
 	strbuf_release(&reason);

-- 
2.7.4
Junio C Hamano· Apr 1, 2017, 02:29 UTC · re: Robert Stanca · lore

Re: [PATCH] [GSOC] prune_worktrees(): reimplement with dir_iterator

Robert Stanca <robert.stanca7@gmail.com> writes:
> Replaces recursive traversing of opendir with dir_iterator

The original code for this one, and also the other one, is not recursive traversing. This one enumerates all the _direct_ subdirectories of ".git/worktrees", and feeds them to prune_worktree() without recursing. The other one scans all the files _directly_ underneath ".git/objects/pack" to find the ones that begin with the packtmp prefix, and unlinks them without recursing. Neither of them touches anything in subdirectory of ".git/worktrees/" nor ".git/objects/pack/".

Using dir_iterator() to make it recursive is not just overkill but is a positively wrong change, isn't it?

Robert Stanca· Apr 1, 2017, 13:29 UTC · re: Junio C Hamano · lore

Re: [PATCH] [GSOC] prune_worktrees(): reimplement with dir_iterator

On Sat, Apr 1, 2017 at 5:29 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 16 quoted lines
>
> Robert Stanca <robert.stanca7@gmail.com> writes:
>
> > Replaces recursive traversing of opendir with dir_iterator
>
> The original code for this one, and also the other one, is not
> recursive traversing.  This one enumerates all the _direct_
> subdirectories of ".git/worktrees", and feeds them to
> prune_worktree() without recursing.  The other one scans all the
> files _directly_ underneath ".git/objects/pack" to find the ones
> that begin with the packtmp prefix, and unlinks them without
> recursing.  Neither of them touches anything in subdirectory of
> ".git/worktrees/" nor ".git/objects/pack/".
>
> Using dir_iterator() to make it recursive is not just overkill but
> is a positively wrong change, isn't it?

Thanks for the review, and yes the commit message is misleading (my bad there). I understood that remove_temp_files has no recursion component to it and it removes all ".tmp-id-pack-" files from /objects/pack , but shouldn't dir-iterator work even if there's no recursion level? My understanding of dir-iterator is that it is used for directory iteration (recursive or not) and where are no subdirectories it's basically recursive with level = 0 .

Junio C Hamano· Apr 1, 2017, 18:33 UTC · re: Robert Stanca · lore

Re: [PATCH] [GSOC] prune_worktrees(): reimplement with dir_iterator

Robert Stanca <robert.stanca7@gmail.com> writes:
Show 10 quoted lines
> On Sat, Apr 1, 2017 at 5:29 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Using dir_iterator() to make it recursive is not just overkill but
>> is a positively wrong change, isn't it?
>
> Thanks for the review, and yes the commit message is misleading (my
> bad there). I understood that remove_temp_files has no recursion
> component to it and it removes all ".tmp-id-pack-" files from
> /objects/pack , but shouldn't dir-iterator work even if there's no
> recursion level?

The point is what should happen when somebody (perhaps a newer version of Git, or a third-party add-on that works with Git) adds a subdirectory in .git/objects/pack/ or .git/worktrees/. The answer is that files and directories in such a subdirectory must not be touched, because prune_worktrees() or remove_temporary_files() do not know what these files and directories are.

The dir-iterator API does not allow you to say "only scan the directory without recursing into the subdirectories" so your code ends up recursing into them, without knowing what the files and directories inside are (or are not). As Peff mentioned in his review on the other one, if we add a knob to dir-iterator API to allow you to tell it not to recurse, then it would make it possible to do a faithful conversion from the original, but without such a change, you are changing the behaviour.

← back to recent threads