dir-iterator: refactor state machine model
dir_iterator_advance() is a large function with two nested loops. Let's improve its readability factoring out three functions and simplifying its mechanics. The refactored model will no longer depend on level.initialized and level.dir_state to keep track of the iteration state and will perform on a single loop. Also, dir_iterator_begin() currently does not check if the given string represents a valid directory path. Since the refactored model will have to stat() the given path at initialization, let's also check for this kind of error and make dir_iterator_begin() return NULL, on failures, with errno appropriately set. And add tests for this new behavior. Improve documentation at dir-iteration.h and code comments at dir-iterator.c to reflect the changes and eliminate possible ambiguities. Finally, adjust refs/files-backend.c to check for now possible dir_iterator_begin() failures. Original-patch-by: Daniel Ferreira <bnmvco@gmail.com> Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Matheus Tavares committed
Jul 10, 2019 at 20:58 UTC
3012397e0327f5e4dfd1d1183a792268429744ae
5 files changed
+164
-122
dir-iterator.c
+122
-112
@@ -4,8 +4,6 @@
4
#include "dir-iterator.h"
5
6
struct dir_iterator_level {
7
- int initialized;
8
-
7
DIR *dir;
8
9
/*
@@ -13,16 +11,6 @@ struct dir_iterator_level {
11
* (including a trailing '/'):
12
*/
13
size_t prefix_len;
16
-
17
- /*
18
- * The last action that has been taken with the current entry
19
- * (needed for directories, which have to be included in the
20
- * iteration and also iterated into):
21
- */
22
- enum {
23
- DIR_STATE_ITER,
24
- DIR_STATE_RECURSE
25
- } dir_state;
14
};
15
16
/*
@@ -34,9 +22,11 @@ struct dir_iterator_int {
22
struct dir_iterator base;
23
24
/*
37
- * The number of levels currently on the stack. This is always
38
- * at least 1, because when it becomes zero the iteration is
39
- * ended and this struct is freed.
25
+ * The number of levels currently on the stack. After the first
26
+ * call to dir_iterator_begin(), if it succeeds to open the
27
+ * first level's dir, this will always be at least 1. Then,
28
+ * when it comes to zero the iteration is ended and this
29
+ * struct is freed.
30
*/
31
size_t levels_nr;
32
@@ -50,113 +40,118 @@ struct dir_iterator_int {
40
struct dir_iterator_level *levels;
41
};
42
43
+/*
44
+ * Push a level in the iter stack and initialize it with information from
45
+ * the directory pointed by iter->base->path. It is assumed that this
46
+ * strbuf points to a valid directory path. Return 0 on success and -1
47
+ * otherwise, leaving the stack unchanged.
48
+ */
49
+static int push_level(struct dir_iterator_int *iter)
50
+{
51
+ struct dir_iterator_level *level;
52
+
53
+ ALLOC_GROW(iter->levels, iter->levels_nr + 1, iter->levels_alloc);
54
+ level = &iter->levels[iter->levels_nr++];
55
+
56
+ if (!is_dir_sep(iter->base.path.buf[iter->base.path.len - 1]))
57
+ strbuf_addch(&iter->base.path, '/');
58
+ level->prefix_len = iter->base.path.len;
59
+
60
+ level->dir = opendir(iter->base.path.buf);
61
+ if (!level->dir) {
62
+ if (errno != ENOENT) {
63
+ warning_errno("error opening directory '%s'",
64
+ iter->base.path.buf);
65
+ }
66
+ iter->levels_nr--;
67
+ return -1;
68
+ }
69
+
70
+ return 0;
71
+}
72
+
73
+/*
74
+ * Pop the top level on the iter stack, releasing any resources associated
75
+ * with it. Return the new value of iter->levels_nr.
76
+ */
77
+static int pop_level(struct dir_iterator_int *iter)
78
+{
79
+ struct dir_iterator_level *level =
80
+ &iter->levels[iter->levels_nr - 1];
81
+
82
+ if (level->dir && closedir(level->dir))
83
+ warning_errno("error closing directory '%s'",
84
+ iter->base.path.buf);
85
+ level->dir = NULL;
86
+
87
+ return --iter->levels_nr;
88
+}
89
+
90
+/*
91
+ * Populate iter->base with the necessary information on the next iteration
92
+ * entry, represented by the given dirent de. Return 0 on success and -1
93
+ * otherwise.
94
+ */
95
+static int prepare_next_entry_data(struct dir_iterator_int *iter,
96
+ struct dirent *de)
97
+{
98
+ strbuf_addstr(&iter->base.path, de->d_name);
99
+ /*
100
+ * We have to reset these because the path strbuf might have
101
+ * been realloc()ed at the previous strbuf_addstr().
102
+ */
103
+ iter->base.relative_path = iter->base.path.buf +
104
+ iter->levels[0].prefix_len;
105
+ iter->base.basename = iter->base.path.buf +
106
+ iter->levels[iter->levels_nr - 1].prefix_len;
107
+
108
+ if (lstat(iter->base.path.buf, &iter->base.st)) {
109
+ if (errno != ENOENT)
110
+ warning_errno("failed to stat '%s'", iter->base.path.buf);
111
+ return -1;
112
+ }
113
+
114
+ return 0;
115
+}
116
+
117
int dir_iterator_advance(struct dir_iterator *dir_iterator)
118
{
119
struct dir_iterator_int *iter =
120
(struct dir_iterator_int *)dir_iterator;
121
122
+ if (S_ISDIR(iter->base.st.st_mode)) {
123
+ if (push_level(iter) && iter->levels_nr == 0) {
124
+ /* Pushing the first level failed */
125
+ return dir_iterator_abort(dir_iterator);
126
+ }
127
+ }
128
+
129
+ /* Loop until we find an entry that we can give back to the caller. */
130
while (1) {
131
+ struct dirent *de;
132
struct dir_iterator_level *level =
133
&iter->levels[iter->levels_nr - 1];
61
- struct dirent *de;
134
63
- if (!level->initialized) {
64
- /*
65
- * Note: dir_iterator_begin() ensures that
66
- * path is not the empty string.
67
- */
68
- if (!is_dir_sep(iter->base.path.buf[iter->base.path.len - 1]))
69
- strbuf_addch(&iter->base.path, '/');
70
- level->prefix_len = iter->base.path.len;
71
-
72
- level->dir = opendir(iter->base.path.buf);
73
- if (!level->dir && errno != ENOENT) {
74
- warning_errno("error opening directory '%s'",
135
+ strbuf_setlen(&iter->base.path, level->prefix_len);
136
+ errno = 0;
137
+ de = readdir(level->dir);
138
+
139
+ if (!de) {
140
+ if (errno)
141
+ warning_errno("error reading directory '%s'",
142
iter->base.path.buf);
76
- /* Popping the level is handled below */
77
- }
78
-
79
- level->initialized = 1;
80
- } else if (S_ISDIR(iter->base.st.st_mode)) {
81
- if (level->dir_state == DIR_STATE_ITER) {
82
- /*
83
- * The directory was just iterated
84
- * over; now prepare to iterate into
85
- * it.
86
- */
87
- level->dir_state = DIR_STATE_RECURSE;
88
- ALLOC_GROW(iter->levels, iter->levels_nr + 1,
89
- iter->levels_alloc);
90
- level = &iter->levels[iter->levels_nr++];
91
- level->initialized = 0;
92
- continue;
93
- } else {
94
- /*
95
- * The directory has already been
96
- * iterated over and iterated into;
97
- * we're done with it.
98
- */
99
- }
143
+ else if (pop_level(iter) == 0)
144
+ return dir_iterator_abort(dir_iterator);
145
+ continue;
146
}
147
102
- if (!level->dir) {
103
- /*
104
- * This level is exhausted (or wasn't opened
105
- * successfully); pop up a level.
106
- */
107
- if (--iter->levels_nr == 0)
108
- return dir_iterator_abort(dir_iterator);
148
+ if (is_dot_or_dotdot(de->d_name))
149
+ continue;
150
151
+ if (prepare_next_entry_data(iter, de))
152
continue;
111
- }
153
113
- /*
114
- * Loop until we find an entry that we can give back
115
- * to the caller:
116
- */
117
- while (1) {
118
- strbuf_setlen(&iter->base.path, level->prefix_len);
119
- errno = 0;
120
- de = readdir(level->dir);
121
-
122
- if (!de) {
123
- /* This level is exhausted; pop up a level. */
124
- if (errno) {
125
- warning_errno("error reading directory '%s'",
126
- iter->base.path.buf);
127
- } else if (closedir(level->dir))
128
- warning_errno("error closing directory '%s'",
129
- iter->base.path.buf);
130
-
131
- level->dir = NULL;
132
- if (--iter->levels_nr == 0)
133
- return dir_iterator_abort(dir_iterator);
134
- break;
135
- }
136
-
137
- if (is_dot_or_dotdot(de->d_name))
138
- continue;
139
-
140
- strbuf_addstr(&iter->base.path, de->d_name);
141
- if (lstat(iter->base.path.buf, &iter->base.st) < 0) {
142
- if (errno != ENOENT)
143
- warning_errno("failed to stat '%s'",
144
- iter->base.path.buf);
145
- continue;
146
- }
147
-
148
- /*
149
- * We have to set these each time because
150
- * the path strbuf might have been realloc()ed.
151
- */
152
- iter->base.relative_path =
153
- iter->base.path.buf + iter->levels[0].prefix_len;
154
- iter->base.basename =
155
- iter->base.path.buf + level->prefix_len;
156
- level->dir_state = DIR_STATE_ITER;
157
-
158
- return ITER_OK;
159
- }
154
+ return ITER_OK;
155
}
156
}
157
@@ -187,17 +182,32 @@ struct dir_iterator *dir_iterator_begin(const char *path)
182
{
183
struct dir_iterator_int *iter = xcalloc(1, sizeof(*iter));
184
struct dir_iterator *dir_iterator = &iter->base;
190
-
191
- if (!path || !*path)
192
- BUG("empty path passed to dir_iterator_begin()");
185
+ int saved_errno;
186
187
strbuf_init(&iter->base.path, PATH_MAX);
188
strbuf_addstr(&iter->base.path, path);
189
190
ALLOC_GROW(iter->levels, 10, iter->levels_alloc);
191
+ iter->levels_nr = 0;
192
199
- iter->levels_nr = 1;
200
- iter->levels[0].initialized = 0;
193
+ /*
194
+ * Note: stat already checks for NULL or empty strings and
195
+ * inexistent paths.
196
+ */
197
+ if (stat(iter->base.path.buf, &iter->base.st) < 0) {
198
+ saved_errno = errno;
199
+ goto error_out;
200
+ }
201
+
202
+ if (!S_ISDIR(iter->base.st.st_mode)) {
203
+ saved_errno = ENOTDIR;
204
+ goto error_out;
205
+ }
206
207
return dir_iterator;
208
+
209
+error_out:
210
+ dir_iterator_abort(dir_iterator);
211
+ errno = saved_errno;
212
+ return NULL;
213
}
dir-iterator.h
+11
-6
@@ -8,18 +8,22 @@
8
*
9
* Iterate over a directory tree, recursively, including paths of all
10
* types and hidden paths. Skip "." and ".." entries and don't follow
11
- * symlinks except for the original path.
11
+ * symlinks except for the original path. Note that the original path
12
+ * is not included in the iteration.
13
*
14
* Every time dir_iterator_advance() is called, update the members of
15
* the dir_iterator structure to reflect the next path in the
16
* iteration. The order that paths are iterated over within a
16
- * directory is undefined, but directory paths are always iterated
17
- * over before the subdirectory contents.
17
+ * directory is undefined, directory paths are always given before
18
+ * their contents.
19
*
20
* A typical iteration looks like this:
21
*
22
* int ok;
22
- * struct iterator *iter = dir_iterator_begin(path);
23
+ * struct dir_iterator *iter = dir_iterator_begin(path);
24
+ *
25
+ * if (!iter)
26
+ * goto error_handler;
27
*
28
* while ((ok = dir_iterator_advance(iter)) == ITER_OK) {
29
* if (want_to_stop_iteration()) {
@@ -59,8 +63,9 @@ struct dir_iterator {
63
};
64
65
/*
62
- * Start a directory iteration over path. Return a dir_iterator that
63
- * holds the internal state of the iteration.
66
+ * Start a directory iteration over path. On success, return a
67
+ * dir_iterator that holds the internal state of the iteration.
68
+ * In case of failure, return NULL and set errno accordingly.
69
*
70
* The iteration includes all paths under path, not including path
71
* itself and not including "." or ".." entries.
refs/files-backend.c
+13
-4
@@ -2143,13 +2143,22 @@ static struct ref_iterator_vtable files_reflog_iterator_vtable = {
2143
static struct ref_iterator *reflog_iterator_begin(struct ref_store *ref_store,
2144
const char *gitdir)
2145
{
2146
- struct files_reflog_iterator *iter = xcalloc(1, sizeof(*iter));
2147
- struct ref_iterator *ref_iterator = &iter->base;
2146
+ struct dir_iterator *diter;
2147
+ struct files_reflog_iterator *iter;
2148
+ struct ref_iterator *ref_iterator;
2149
struct strbuf sb = STRBUF_INIT;
2150
2150
- base_ref_iterator_init(ref_iterator, &files_reflog_iterator_vtable, 0);
2151
strbuf_addf(&sb, "%s/logs", gitdir);
2152
- iter->dir_iterator = dir_iterator_begin(sb.buf);
2152
+
2153
+ diter = dir_iterator_begin(sb.buf);
2154
+ if(!diter)
2155
+ return empty_ref_iterator_begin();
2156
+
2157
+ iter = xcalloc(1, sizeof(*iter));
2158
+ ref_iterator = &iter->base;
2159
+
2160
+ base_ref_iterator_init(ref_iterator, &files_reflog_iterator_vtable, 0);
2161
+ iter->dir_iterator = diter;
2162
iter->ref_store = ref_store;
2163
strbuf_release(&sb);
2164
t/helper/test-dir-iterator.c
+5
@@ -17,6 +17,11 @@ int cmd__dir_iterator(int argc, const char **argv)
17
18
diter = dir_iterator_begin(path.buf);
19
20
+ if (!diter) {
21
+ printf("dir_iterator_begin failure: %d\n", errno);
22
+ exit(EXIT_FAILURE);
23
+ }
24
+
25
while (dir_iterator_advance(diter) == ITER_OK) {
26
if (S_ISDIR(diter->st.st_mode))
27
printf("[d] ");
t/t0066-dir-iterator.sh
+13
@@ -52,4 +52,17 @@ test_expect_success 'dir-iterator should list files in the correct order' '
52
test_cmp expected-pre-order-output actual-pre-order-output
53
'
54
55
+test_expect_success 'begin should fail upon inexistent paths' '
56
+ test_must_fail test-tool dir-iterator ./inexistent-path \
57
+ >actual-inexistent-path-output &&
58
+ echo "dir_iterator_begin failure: 2" >expected-inexistent-path-output &&
59
+ test_cmp expected-inexistent-path-output actual-inexistent-path-output
60
+'
61
+
62
+test_expect_success 'begin should fail upon non directory paths' '
63
+ test_must_fail test-tool dir-iterator ./dir/b >actual-non-dir-output &&
64
+ echo "dir_iterator_begin failure: 20" >expected-non-dir-output &&
65
+ test_cmp expected-non-dir-output actual-non-dir-output
66
+'
67
+
68
test_done