On 02/15, Matheus Tavares wrote:
> Replace usage of opendir/readdir/closedir API to traverse directories
> recursively, at copy_or_link_directory function, by the dir-iterator
> API.
>
> Signed-off-by: Matheus Tavares <matheus.bernardino@xxxxxx>
> ---
> builtin/clone.c | 39 +++++++++++++++++++--------------------
> 1 file changed, 19 insertions(+), 20 deletions(-)
>
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 2a1cc4dab9..66ae347f79 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -23,6 +23,8 @@
> #include "transport.h"
> #include "strbuf.h"
> #include "dir.h"
> +#include "dir-iterator.h"
> +#include "iterator.h"
> #include "sigchain.h"
> #include "branch.h"
> #include "remote.h"
> @@ -413,40 +415,33 @@ static void mkdir_if_missing(const char *pathname, mode_t mode)
> static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,
> const char *src_repo, int src_baselen)
> {
The src_baselen parameter is now unused, and should be removed. Our
codebase currently doesn't compile with -Wunused-parameter, so this is
not something the compiler can catch at the moment unfortunately.
However there is some work going on towards removing unused parameter
from the codebase, so it would be nice to not make things worse here.
*1*: https://public-inbox.org/git/20190214054736.GA20091@xxxxxxxxxxxxxxxxxxxxx
> - struct dirent *de;
> - struct stat buf;
> int src_len, dest_len;
> - DIR *dir;
> -
> - dir = opendir(src->buf);
> - if (!dir)
> - die_errno(_("failed to open '%s'"), src->buf);
> + struct dir_iterator *iter;
> + int iter_status;
>
> mkdir_if_missing(dest->buf, 0777);
>
> + iter = dir_iterator_begin(src->buf);
> +
> strbuf_addch(src, '/');
> src_len = src->len;
> strbuf_addch(dest, '/');
> dest_len = dest->len;
>
> - while ((de = readdir(dir)) != NULL) {
> + while ((iter_status = dir_iterator_advance(iter)) == ITER_OK) {
> strbuf_setlen(src, src_len);
> - strbuf_addstr(src, de->d_name);
> + strbuf_addstr(src, iter->relative_path);
> strbuf_setlen(dest, dest_len);
> - strbuf_addstr(dest, de->d_name);
> - if (stat(src->buf, &buf)) {
> - warning (_("failed to stat %s\n"), src->buf);
> - continue;
> - }
Why was this warning removed? I don't see a corresponding warning in
the iterator API. The one thing the iterator API is doing is that it
does an lstat on the path to check if it exists. However that does
not follow symlinks, and ignores possible other errors such as
permission errors.
If there is a good reason to remove the warning, that would be useful
to describe in the commit message.
> - if (S_ISDIR(buf.st_mode)) {
> - if (de->d_name[0] != '.')
> - copy_or_link_directory(src, dest,
> - src_repo, src_baselen);
> + strbuf_addstr(dest, iter->relative_path);
> +
> + if (S_ISDIR(iter->st.st_mode)) {
> + if (iter->basename[0] != '.')
> + mkdir_if_missing(dest->buf, 0777);
> continue;
> }
>
> /* Files that cannot be copied bit-for-bit... */
> - if (!strcmp(src->buf + src_baselen, "/info/alternates")) {
> + if (!strcmp(iter->relative_path, "info/alternates")) {
> copy_alternates(src, dest, src_repo);
> continue;
> }
> @@ -463,7 +458,11 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,
> if (copy_file_with_time(dest->buf, src->buf, 0666))
> die_errno(_("failed to copy file to '%s'"), dest->buf);
> }
> - closedir(dir);
> +
> + if (iter_status != ITER_DONE) {
> + strbuf_setlen(src, src_len);
> + die(_("failed to iterate over '%s'"), src->buf);
> + }
Interestingly enough, this is not something that can currently
happen if I read the dir-iterator code correctly. Even though the
dir_iterator_advance function says it can return ITER_ERROR, it never
actually does. The only way the iter_status can be not ITER_DONE at
this point is if we would 'break' out of the loop.
I don't think it hurts to be defensive here in case someone decides to
break out of the loop in the future, just something odd I noticed
while reviewing the code.
> }
>
> static void clone_local(const char *src_repo, const char *dest_repo)
> --
> 2.20.1
>
