patch, 3 partsclone: convert explicit dir traversal to dir-iterator

> Add the pedantic option to dir-iterator's initialization function,
> dir_iterator_begin. When this option is set to true,
> dir_iterator_advance will immediately return ITER_ERROR when failing to
> fetch the next entry. When set to false, dir_iterator_advance will emit
> a warning and keep looking for the next entry.
> 
> Also adjust refs/files-backend.c to the new dir_iterator_begin
> signature.

> Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
> ---
> Changes in v2:
>  - Added in v2
> 
>  dir-iterator.c       | 23 +++++++++++++++++++++--
>  dir-iterator.h       | 16 +++++++++++++---
>  refs/files-backend.c |  2 +-
>  3 files changed, 35 insertions(+), 6 deletions(-)
> 
> diff --git a/dir-iterator.c b/dir-iterator.c
> index f2dcd82fde..070a656555 100644
> --- a/dir-iterator.c
> +++ b/dir-iterator.c
> @@ -48,6 +48,13 @@ struct dir_iterator_int {
>  	 * that will be included in this iteration.
>  	 */
>  	struct dir_iterator_level *levels;
> +
> +	/*
> +	 * Boolean value to define dir-iterator's behaviour when failing to
> +	 * fetch next entry. See comments on dir_iterator_begin at
> +	 * dir-iterator.h
> +	 */
> +	int pedantic;
>  };
>  
>  int dir_iterator_advance(struct dir_iterator *dir_iterator)
> @@ -71,6 +78,8 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)
>  
>  			level->dir = opendir(iter->base.path.buf);
>  			if (!level->dir && errno != ENOENT) {
> +				if (iter->pedantic)
> +					goto error_out;

>  				warning("error opening directory %s: %s",
>  					iter->base.path.buf, strerror(errno));
>  				/* Popping the level is handled below */
> @@ -122,6 +131,8 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)
>  			if (!de) {
>  				/* This level is exhausted; pop up a level. */
>  				if (errno) {
> +					if (iter->pedantic)
> +						goto error_out;
>  					warning("error reading directory %s: %s",
>  						iter->base.path.buf, strerror(errno));
>  				} else if (closedir(level->dir))
> @@ -139,10 +150,13 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)
>  
>  			strbuf_addstr(&iter->base.path, de->d_name);
>  			if (lstat(iter->base.path.buf, &iter->base.st) < 0) {
> -				if (errno != ENOENT)
> +				if (errno != ENOENT) {
> +					if (iter->pedantic)
> +						goto error_out;
>  					warning("error reading path '%s': %s",
>  						iter->base.path.buf,
>  						strerror(errno));
> +				}
>  				continue;
>  			}
>  
> @@ -159,6 +173,10 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)
>  			return ITER_OK;
>  		}
>  	}
> +
> +error_out:
> +	dir_iterator_abort(dir_iterator);
> +	return ITER_ERROR;
>  }
>  
>  int dir_iterator_abort(struct dir_iterator *dir_iterator)
> @@ -182,7 +200,7 @@ int dir_iterator_abort(struct dir_iterator *dir_iterator)
>  	return ITER_DONE;
>  }
>  
> -struct dir_iterator *dir_iterator_begin(const char *path)
> +struct dir_iterator *dir_iterator_begin(const char *path, int pedantic)

>  {
>  	struct dir_iterator_int *iter = xcalloc(1, sizeof(*iter));
>  	struct dir_iterator *dir_iterator = &iter->base;
> @@ -195,6 +213,7 @@ struct dir_iterator *dir_iterator_begin(const char *path)
>  
>  	ALLOC_GROW(iter->levels, 10, iter->levels_alloc);
>  
> +	iter->pedantic = pedantic;
>  	iter->levels_nr = 1;
>  	iter->levels[0].initialized = 0;
>  
> diff --git a/dir-iterator.h b/dir-iterator.h
> index 970793d07a..50ca8e1a27 100644
> --- a/dir-iterator.h
> +++ b/dir-iterator.h
> @@ -19,7 +19,7 @@
>   * A typical iteration looks like this:
>   *
>   *     int ok;
> - *     struct iterator *iter = dir_iterator_begin(path);
> + *     struct iterator *iter = dir_iterator_begin(path, 0);
>   *
>   *     while ((ok = dir_iterator_advance(iter)) == ITER_OK) {
>   *             if (want_to_stop_iteration()) {
> @@ -65,9 +65,15 @@ struct dir_iterator {
>   * The iteration includes all paths under path, not including path
>   * itself and not including "." or ".." entries.
>   *
> - * path is the starting directory. An internal copy will be made.
> + * Parameters are:
> + * - path is the starting directory. An internal copy will be made.
> + * - pedantic is a boolean value. If true, dir-iterator will free
> + *   resources and return ITER_ERROR immediately, in case of an error
> + *   while trying to fetch the next entry in dir_iterator_advance. If
> + *   false, it will just emit a warning and keep looking for the next
> + *   entry.
>   */
> -struct dir_iterator *dir_iterator_begin(const char *path);
> +struct dir_iterator *dir_iterator_begin(const char *path, int pedantic);
>  
>  /*
>   * Advance the iterator to the first or next item and return ITER_OK.
> @@ -76,6 +82,10 @@ struct dir_iterator *dir_iterator_begin(const char *path);
>   * dir_iterator and associated resources and return ITER_ERROR. It is
>   * a bug to use iterator or call this function again after it has
>   * returned ITER_DONE or ITER_ERROR.
> + *
> + * Note that whether dir_iterator_advance will return ITER_ERROR when
> + * failing to fetch the next entry or keep going is defined by the
> + * 'pedantic' option at dir-iterator's initialization.
>   */
>  int dir_iterator_advance(struct dir_iterator *iterator);
>  
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index dd8abe9185..c3d3b6c454 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2143,7 +2143,7 @@ static struct ref_iterator *reflog_iterator_begin(struct ref_store *ref_store,
>  
>  	base_ref_iterator_init(ref_iterator, &files_reflog_iterator_vtable, 0);
>  	strbuf_addf(&sb, "%s/logs", gitdir);
> -	iter->dir_iterator = dir_iterator_begin(sb.buf);
> +	iter->dir_iterator = dir_iterator_begin(sb.buf, 0);
>  	iter->ref_store = ref_store;
>  	strbuf_release(&sb);
>  
> -- 
> 2.20.1
> 

>
> On 02/23, Matheus Tavares wrote:
> > Add the pedantic option to dir-iterator's initialization function,
> > dir_iterator_begin. When this option is set to true,
> > dir_iterator_advance will immediately return ITER_ERROR when failing to
> > fetch the next entry. When set to false, dir_iterator_advance will emit
> > a warning and keep looking for the next entry.
> >
> > Also adjust refs/files-backend.c to the new dir_iterator_begin
> > signature.
>
> Thanks, this makes sense to me.  This commit message describes what we
> are doing in this patch, but not why we are doing it.  From previously
> reviewing this series, I know this is going to be used in a subsequent
> patch, but it is nice to give reviewers and future readers of this
> patch that context as well.  Just something like "This behaviour is
> going to be used in a subsequent patch." should be enough here.

> >  int dir_iterator_advance(struct dir_iterator *dir_iterator)
> > @@ -71,6 +78,8 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)
> >
> >                       level->dir = opendir(iter->base.path.buf);
> >                       if (!level->dir && errno != ENOENT) {
> > +                             if (iter->pedantic)
> > +                                     goto error_out;
>
> I think we should also print an error here.  The caller doesn't have
> any context on what went wrong, and will probably just 'die()' if an
> error is encountered.  I think it would make sense to call
> 'error(...)' here before 'goto error_out;' to give a useful error
> message here.

> > -struct dir_iterator *dir_iterator_begin(const char *path)
> > +struct dir_iterator *dir_iterator_begin(const char *path, int pedantic)
>
> Thinking about the future evolution of this interface, it might make
> more sense to have that second parameter be a "struct
> dir_iterator_opts".  For now it would just have one member "pedantic",
> but in the future we could add additional options there instead of
> adding additional parameters.

>
> On Sat, Feb 23, 2019 at 10:37 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:
> >
> > On 02/23, Matheus Tavares wrote:
> > > Add the pedantic option to dir-iterator's initialization function,
> > > dir_iterator_begin. When this option is set to true,
> > > dir_iterator_advance will immediately return ITER_ERROR when failing to
> > > fetch the next entry. When set to false, dir_iterator_advance will emit
> > > a warning and keep looking for the next entry.
> > >
> > > Also adjust refs/files-backend.c to the new dir_iterator_begin
> > > signature.
> >
> > Thanks, this makes sense to me.  This commit message describes what we
> > are doing in this patch, but not why we are doing it.  From previously
> > reviewing this series, I know this is going to be used in a subsequent
> > patch, but it is nice to give reviewers and future readers of this
> > patch that context as well.  Just something like "This behaviour is
> > going to be used in a subsequent patch." should be enough here.
>
> I agree that it's a good idea to add just that.
>

> > >  int dir_iterator_advance(struct dir_iterator *dir_iterator)
> > > @@ -71,6 +78,8 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)
> > >
> > >                       level->dir = opendir(iter->base.path.buf);
> > >                       if (!level->dir && errno != ENOENT) {
> > > +                             if (iter->pedantic)
> > > +                                     goto error_out;
> >
> > I think we should also print an error here.  The caller doesn't have
> > any context on what went wrong, and will probably just 'die()' if an
> > error is encountered.

> > I think it would make sense to call
> > 'error(...)' here before 'goto error_out;' to give a useful error
> > message here.
>
> If we start to give error messages, then we might as well give error
> messages all the times when we error out. This will avoid the callers
> wondering if they need to give an error message or not.
>
> I am not sure it's necessary here though. And I think if it's useful,
> it can be added in another patch or another patch series.
>

> > >                               warning("error opening directory %s: %s",
> > >                                       iter->base.path.buf, strerror(errno));
> > >                               /* Popping the level is handled below */
>
> > > -struct dir_iterator *dir_iterator_begin(const char *path)
> > > +struct dir_iterator *dir_iterator_begin(const char *path, int pedantic)
> >
> > Thinking about the future evolution of this interface, it might make
> > more sense to have that second parameter be a "struct
> > dir_iterator_opts".  For now it would just have one member "pedantic",
> > but in the future we could add additional options there instead of
> > adding additional parameters.
>
> I think it's ok with `int pedantic` for now as improvements can be
> done when they are really needed. And we will perhaps find out that
> it's better to just change `int pedantic` to `unsigned flags` instead
> of `struct dir_iterator_opts`.
>

> On Sun, Feb 24, 2019 at 5:35 AM Christian Couder
> <christian.couder@gmail.com> wrote:
> > On Sat, Feb 23, 2019 at 10:37 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:
> > > >  int dir_iterator_advance(struct dir_iterator *dir_iterator)
> > > > @@ -71,6 +78,8 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)
> > > >
> > > >                       level->dir = opendir(iter->base.path.buf);
> > > >                       if (!level->dir && errno != ENOENT) {
> > > > +                             if (iter->pedantic)
> > > > +                                     goto error_out;
> > >
> > > I think we should also print an error here.  The caller doesn't have
> > > any context on what went wrong, and will probably just 'die()' if an
> > > error is encountered.
> 
> To correctly handle the error, I assumed that the caller wouldn't need
> to know which exact function returned an error, as long as it knew it
> was a "failure to fetch the next entry" kind of error, which is the
> "category" of errors caught with the 'pedantic' option. (currently, it
> includes errors on lstat, opendir and readdir). Is this assumption
> valid?
>
> > > I think it would make sense to call
> > > 'error(...)' here before 'goto error_out;' to give a useful error
> > > message here.
> >
> > If we start to give error messages, then we might as well give error
> > messages all the times when we error out. This will avoid the callers
> > wondering if they need to give an error message or not.
> >
> > I am not sure it's necessary here though. And I think if it's useful,
> > it can be added in another patch or another patch series.
> >
>
> I could just copy the warning messages bellow each 'goto error_out'
> and use then at an 'error(...)' call before the goto. But as Christian
> pointed out, I think this would confuse callers wether they should
> print error messages or not. On the other side, it may just be
> different 'layers' of errors... I don't have any strong opinion about
> this.

> > > >                               warning("error opening directory %s: %s",
> > > >                                       iter->base.path.buf, strerror(errno));
> > > >                               /* Popping the level is handled below */
> >
> > > > -struct dir_iterator *dir_iterator_begin(const char *path)
> > > > +struct dir_iterator *dir_iterator_begin(const char *path, int pedantic)
> > >
> > > Thinking about the future evolution of this interface, it might make
> > > more sense to have that second parameter be a "struct
> > > dir_iterator_opts".  For now it would just have one member "pedantic",
> > > but in the future we could add additional options there instead of
> > > adding additional parameters.
> >
> > I think it's ok with `int pedantic` for now as improvements can be
> > done when they are really needed. And we will perhaps find out that
> > it's better to just change `int pedantic` to `unsigned flags` instead
> > of `struct dir_iterator_opts`.
> >
> 
> I did thought about using `unsigned flags` instead of `int pedantic`
> for the same reasons Thomas pointed out, but as there would be just
> one flag for now, it seemed to me that `int pedantic` would make more
> sense (following the 'YAGNI' principle). But if it is already known
> that more flags (or options) are coming in a very near future, I may
> change this to `unsigned flags` or `struct dir_iterator_opts` in v3 if
> you think it is needed. Just let me know, please.

>
> Extract dir creation code snippet from copy_or_link_directory to its own
> function named mkdir_if_missing. This change will help removing
> copy_or_link_directory's explicit recursion, which will be done in a
> following patch. Also makes code more readable.
>
> Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
> ---
> Changes in v2:
>  - Replaced explicit reference to a following patch at patch message to
>    a more generic reference.
>  - Changed struct stat variable name to st, which is the most common
>    name for such variables in git's code.

> Replace usage of opendir/readdir/closedir API to traverse directories
> recursively, at copy_or_link_directory function, by the dir-iterator
> API. This simplifies the code and avoid recursive calls to
> copy_or_link_directory.
> 
> This process also brings some safe behaviour changes to
> copy_or_link_directory:
>  - It will no longer follows symbolic links. This is not a problem,
>    since the function is only used to copy .git/objects directory, and
>    symbolic links are not expected there.
>  - Hidden directories won't be skipped anymore. In fact, it is odd that
>    the function currently skip hidden directories but not hidden files.
>    The reason for that could be unintentional: probably the intention
>    was to skip '.' and '..' only, but it ended up accidentally skipping
>    all directories starting with '.'. Again, it must not be a problem
>    not to skip hidden dirs since hidden dirs/files are not expected at
>    .git/objects.
>  - Now, copy_or_link_directory will call die() in case of an error on
>    openddir, readdir or lstat, inside dir_iterator_advance. That means
>    it will abort in case of an error trying to fetch any iteration
>    entry.
> 
> Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
> ---
> Changes in v2:
>  - Improved patch message
>  - Removed a now unused variable

>  builtin/clone.c | 47 ++++++++++++++++++++++++++++-------------------
>  1 file changed, 28 insertions(+), 19 deletions(-)
> 
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 862d2ea69c..515dc91d63 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"
> @@ -411,42 +413,45 @@ 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)
> +				   const char *src_repo)
>  {
> -	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;
> +	struct stat st;
>  
>  	mkdir_if_missing(dest->buf, 0777);
>  
> +	iter = dir_iterator_begin(src->buf, 1);
> +
>  	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)) {
> +		strbuf_addstr(dest, iter->relative_path);
> +
> +		/*
> +		 * dir_iterator_advance already calls lstat to populate iter->st
> +		 * but, unlike stat, lstat does not checks for permissions on
> +		 * the given path.
> +		 */

> +		if (stat(src->buf, &st)) {
>  			warning (_("failed to stat %s\n"), src->buf);
>  			continue;
>  		}
> -		if (S_ISDIR(buf.st_mode)) {
> -			if (de->d_name[0] != '.')
> -				copy_or_link_directory(src, dest,
> -						       src_repo, src_baselen);
> +
> +		if (S_ISDIR(iter->st.st_mode)) {
> +			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 +468,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);
> +	}
>  }
>  
>  static void clone_local(const char *src_repo, const char *dest_repo)
> @@ -481,7 +490,7 @@ static void clone_local(const char *src_repo, const char *dest_repo)
>  		get_common_dir(&dest, dest_repo);
>  		strbuf_addstr(&src, "/objects");
>  		strbuf_addstr(&dest, "/objects");
> -		copy_or_link_directory(&src, &dest, src_repo, src.len);
> +		copy_or_link_directory(&src, &dest, src_repo);
>  		strbuf_release(&src);
>  		strbuf_release(&dest);
>  	}
> -- 
> 2.20.1
> 

>
> On 02/23, Matheus Tavares wrote:
> > ---
> > Changes in v2:
> >  - Improved patch message
> >  - Removed a now unused variable
>
> s/variable/parameter/ I believe?

> > +     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)) {
> > +             strbuf_addstr(dest, iter->relative_path);
> > +
> > +             /*
> > +              * dir_iterator_advance already calls lstat to populate iter->st
> > +              * but, unlike stat, lstat does not checks for permissions on
> > +              * the given path.
> > +              */
>
> Hmm, lstat does check the permissions on the path, it just doesn't
> follow symlinks.  I think I actually mislead you in my previous review
> here, and was reading the code in dir-iterator.c all wrong.
>
> I thought it said "if (errno == ENOENT) warning(...)", however the
> condition is "errno != ENOENT", which is why I thought we were loosing
> warnings when errno == EACCES for example.
>
> As we decided that we would no longer follow symlinks now, I think we
> can actually get rid of the stat call here.  Sorry about the confusion.
>

>  - Hidden directories won't be skipped anymore. In fact, it is odd that
>    the function currently skip hidden directories but not hidden files.
>    The reason for that could be unintentional: probably the intention
>    was to skip '.' and '..' only, but it ended up accidentally skipping
>    all directories starting with '.'. Again, it must not be a problem
>    not to skip hidden dirs since hidden dirs/files are not expected at
>    .git/objects.

>
>
> On Sat, Feb 23 2019, 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. This simplifies the code and avoid recursive calls to
> > copy_or_link_directory.
>
> Sounds good in principle.
>
> > This process also brings some safe behaviour changes to
> > copy_or_link_directory:
>
> I ad-hoc tested some of these, and could spot behavior changes. We
> should have tests for these.

> >  - It will no longer follows symbolic links. This is not a problem,
> >    since the function is only used to copy .git/objects directory, and
> >    symbolic links are not expected there.
>
> I don't think we should make that assumption, and I don't know of
> anything else in git that does.

> When trying to test this I made e.g. objects/c4 a symlink to /tmp/c4,
> and a specific object in objects/4d/ a symlink to /tmp too.
>
> Without this patch the individual object is still a symlink, but the
> object under the directory gets resolved, and "un-symlinked", also with
> --dissociate, which seems like an existing bug.
>
> With your patch that symlink structure is copied as-is. That's more
> faithful under --local, but a regression for --dissociate (which didn't
> work fully to begin with...).

> But then I try with --no-hardlinks and stock git dereferences my symlink
> structure, but with your patches fails completely:
>
>     Cloning into bare repository 'repo2'...
>     error: copy-fd: read returned: Is a directory
>     fatal: failed to copy file to 'repo2/objects/c4': Is a directory
>     fatal: the remote end hung up unexpectedly
>     fatal: cannot change to 'repo2': No such file or directory

> So there's at least one case in a few minutes of prodding this where we
> can't clone a working repo now, however obscure the setup.
>
> >  - Hidden directories won't be skipped anymore. In fact, it is odd that
> >    the function currently skip hidden directories but not hidden files.
> >    The reason for that could be unintentional: probably the intention
> >    was to skip '.' and '..' only, but it ended up accidentally skipping
> >    all directories starting with '.'. Again, it must not be a problem
> >    not to skip hidden dirs since hidden dirs/files are not expected at
> >    .git/objects.
>
> I reproduce this with --local. A ".foo" isn't copied before, now it
> is. Good, I guess. We'd have already copied a "foo".
>
> >  - Now, copy_or_link_directory will call die() in case of an error on
> >    openddir, readdir or lstat, inside dir_iterator_advance. That means
> >    it will abort in case of an error trying to fetch any iteration
> >    entry.

> Unfortunately, in this as in so many edge case we have no existing
> tests.
>
> This would be much easier to review and would give reviewers more
> confidence if the parts of this that changed behavior started with a
> patch or patches that just manually objects/ dirs with various

> combinations of symlinks, hardlinks etc., and asserted that the various
> options did exactly what they're doing now, and made sure the
> source/target repos were the same after/both passed "fsck".
>
> Then followed by some version of this patch which changes the behavior,
> and would be forced to tweak those tests. To make it clear e.g. that
> some cases where we have a working "clone" are now a hard error.

> On Sat, Feb 23, 2019 at 11:48 PM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>>
>>
>> On Sat, Feb 23 2019, 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. This simplifies the code and avoid recursive calls to
>> > copy_or_link_directory.
>>
>> Sounds good in principle.
>>
>> > This process also brings some safe behaviour changes to
>> > copy_or_link_directory:
>>
>> I ad-hoc tested some of these, and could spot behavior changes. We
>> should have tests for these.
>
> I agree that ideally we should have a few tests for these, but this is
> a grey area (see below) and there are areas that are not grey for
> which we don't have any test...
>
> And then adding tests would make this series become larger than a
> typical GSoC micro-project...
>
>> >  - It will no longer follows symbolic links. This is not a problem,
>> >    since the function is only used to copy .git/objects directory, and
>> >    symbolic links are not expected there.
>>
>> I don't think we should make that assumption, and I don't know of
>> anything else in git that does.
>
> I think Git itself doesn't create symlinks in ".git/objects/" and we
> don't recommend people manually tweaking what's inside ".git/". That's
> why I think it's a grey area.
>
>> I've certainly symlinked individual objects or packs into a repo for
>> debugging / recovery, and it would be unexpected to clone that and miss
>> something.
>
> If people tweak what's inside ".git" by hand, they are expected to
> know what they doing and be able to debug it.
>
>> So in the general case we should be strict in what we generate, but
>> permissive in what we accept. We don't want a "clone" of an existing
>> repo to fail, or "fsck" to fail after clone...
>
> Yeah, but realistically I don't think we are going to foolproof Git
> from everything that someone could do by tweaking random things
> manually in ".git/".
>
> I am not saying that it should be ok to make things much worse than
> they are now in case some things have been tweaked in ".git/", but if
> things in general don't look worse in this grey area, and a patch
> otherwise improves things, then I think the patch should be ok.
>
>> When trying to test this I made e.g. objects/c4 a symlink to /tmp/c4,
>> and a specific object in objects/4d/ a symlink to /tmp too.
>>
>> Without this patch the individual object is still a symlink, but the
>> object under the directory gets resolved, and "un-symlinked", also with
>> --dissociate, which seems like an existing bug.
>>
>> With your patch that symlink structure is copied as-is. That's more
>> faithful under --local, but a regression for --dissociate (which didn't
>> work fully to begin with...).
>
> I think that I use --local (which is the default if the repository is
> specified as a local path) much more often than --dissociate, so for
> me the patch would be very positive, especially since --dissociate is
> already buggy anyway in this case.
>
>> I was paranoid that "no longer follows symbolic links" could also mean
>> "will ignore those objects", but it seems to more faithfully copy things
>> as-is for *that* case.
>
> Nice!
>
>> But then I try with --no-hardlinks and stock git dereferences my symlink
>> structure, but with your patches fails completely:
>>
>>     Cloning into bare repository 'repo2'...
>>     error: copy-fd: read returned: Is a directory
>>     fatal: failed to copy file to 'repo2/objects/c4': Is a directory
>>     fatal: the remote end hung up unexpectedly
>>     fatal: cannot change to 'repo2': No such file or directory
>
> Maybe this could be fixed. Anyway I don't use --no-hardlinks very
> often, so I still think the patch is a positive even with this
> failure.
>
>> So there's at least one case in a few minutes of prodding this where we
>> can't clone a working repo now, however obscure the setup.
>>
>> >  - Hidden directories won't be skipped anymore. In fact, it is odd that
>> >    the function currently skip hidden directories but not hidden files.
>> >    The reason for that could be unintentional: probably the intention
>> >    was to skip '.' and '..' only, but it ended up accidentally skipping
>> >    all directories starting with '.'. Again, it must not be a problem
>> >    not to skip hidden dirs since hidden dirs/files are not expected at
>> >    .git/objects.
>>
>> I reproduce this with --local. A ".foo" isn't copied before, now it
>> is. Good, I guess. We'd have already copied a "foo".
>>
>> >  - Now, copy_or_link_directory will call die() in case of an error on
>> >    openddir, readdir or lstat, inside dir_iterator_advance. That means
>> >    it will abort in case of an error trying to fetch any iteration
>> >    entry.
>
> It would be nice if the above paragraph in the commit message would
> say what was the previous behavior and why it's better to die() .
>
>> Good, but really IMNSHO this series is tweaking some critical core code
>> and desperately needs tests.
>
> It's critical that this code works well in the usual case, yes. (And
> there are already a lot of tests that test that.) But when people have
> manually tweaked things in their ".git/objects/", it's not critical
> what happens. Many systems have "undefined behaviors" at some point
> and that's ok.
>
> And no, I am not saying that we should consider it completely
> "undefined behavior" as soon as something is manually tweaked in
> ".git/", and yes, tests would be nice, and your comments and manual
> tests on this are very much appreciated. It's just that I don't think
> we should require too much when a patch, especially from a first time
> contributor, is already improving things, though it also changes a few
> things in a grey area.
>
>> Unfortunately, in this as in so many edge case we have no existing
>> tests.
>>
>> This would be much easier to review and would give reviewers more
>> confidence if the parts of this that changed behavior started with a
>> patch or patches that just manually objects/ dirs with various
>
> I think "created" is missing between "manually" and  "objects/" in the
> above sentence.
>
>> combinations of symlinks, hardlinks etc., and asserted that the various
>> options did exactly what they're doing now, and made sure the
>> source/target repos were the same after/both passed "fsck".
>>
>> Then followed by some version of this patch which changes the behavior,
>> and would be forced to tweak those tests. To make it clear e.g. that
>> some cases where we have a working "clone" are now a hard error.
>
> Unfortunately this would be a lot of work and not appropriate for a
> GSoC micro-project.
>
> Thanks,
> Christian.

> On Sun, Feb 24, 2019 at 9:45 PM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> ..
>> Can't the utility function we're moving to just be made to be
>> bug-compatible with what we're doing now with symlinks?
>
> I haven't really followed closely this thread. But I think the first
> step should be bug-compatible (it really depends if you count the
> current behavior "buggy"). Then add improvements on top if there's
> still time or strong consensus. It's easier to bisect that way at
> least.

>
> This is surely a horrible idea, but all tests pass with this (not that
> I trust them much). Doing this for later WIP use in clone.c.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  dir-iterator.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/dir-iterator.c b/dir-iterator.c
> index 070a656555..6a9c0c4d08 100644
> --- a/dir-iterator.c
> +++ b/dir-iterator.c
> @@ -149,7 +149,7 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)
>                                 continue;
>
>                         strbuf_addstr(&iter->base.path, de->d_name);
> -                       if (lstat(iter->base.path.buf, &iter->base.st) < 0) {
> +                       if (stat(iter->base.path.buf, &iter->base.st) < 0) {

>                                 if (errno != ENOENT) {
>                                         if (iter->pedantic)
>                                                 goto error_out;
> --
> 2.21.0.rc2.1.g2d5e20a900.dirty
>

>
> From: Matheus Tavares <matheus.bernardino@usp.br>
>
> Replace usage of opendir/readdir/closedir API to traverse directories
> recursively, at copy_or_link_directory function, by the dir-iterator
> API. This simplifies the code and avoid recursive calls to
> copy_or_link_directory.
>
> [Ævar: This should be bug-compatible with the existing "clone"
> behavior. The whole bit here with "iter->relative_path[0] == '.'" is a
> dirty hack. We don't copy dot-dirs, and then later on just blindly
> ignore ENOENT errors as we descend into them. That case really wants
> to be a is_dotdir_or_file_within() test instead]
>
> Now, copy_or_link_directory will call die() in case of an error on
> openddir, readdir or lstat, inside dir_iterator_advance. That means it
> will abort in case of an error trying to fetch any iteration entry.
>
> Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  builtin/clone.c | 55 +++++++++++++++++++++++++++++--------------------
>  1 file changed, 33 insertions(+), 22 deletions(-)
>
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 862d2ea69c..c32e9022b3 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"
> @@ -411,42 +413,47 @@ 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)
> +                                  const char *src_repo)
>  {
> -       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;
> +       struct stat st;
>
>         mkdir_if_missing(dest->buf, 0777);
>
> +       iter = dir_iterator_begin(src->buf, 1);
> +
>         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)) {
> +               strbuf_addstr(dest, iter->relative_path);
> +
> +               /*
> +                * dir_iterator_advance already calls lstat to populate iter->st
> +                * but, unlike stat, lstat does not checks for permissions on
> +                * the given path.
> +                */
> +               if (stat(src->buf, &st)) {
>                         warning (_("failed to stat %s\n"), src->buf);
>                         continue;
>                 }
> -               if (S_ISDIR(buf.st_mode)) {
> -                       if (de->d_name[0] != '.')
> -                               copy_or_link_directory(src, dest,
> -                                                      src_repo, src_baselen);
> +
> +               if (S_ISDIR(iter->st.st_mode)) {
> +                       if (iter->relative_path[0] == '.')

> +                               continue;
> +                       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;
>                 }
> @@ -456,14 +463,18 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,
>                 if (!option_no_hardlinks) {
>                         if (!link(src->buf, dest->buf))
>                                 continue;
> -                       if (option_local > 0)
> -                               die_errno(_("failed to create link '%s'"), dest->buf);
> +                       if (option_local > 0 && errno != ENOENT)
> +                               warning_errno(_("failed to create link '%s'"), dest->buf);
>                         option_no_hardlinks = 1;
>                 }
> -               if (copy_file_with_time(dest->buf, src->buf, 0666))
> +               if (copy_file_with_time(dest->buf, src->buf, 0666) && errno != ENOENT)
>                         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);
> +       }
>  }
>
>  static void clone_local(const char *src_repo, const char *dest_repo)
> @@ -481,7 +492,7 @@ static void clone_local(const char *src_repo, const char *dest_repo)
>                 get_common_dir(&dest, dest_repo);
>                 strbuf_addstr(&src, "/objects");
>                 strbuf_addstr(&dest, "/objects");
> -               copy_or_link_directory(&src, &dest, src_repo, src.len);
> +               copy_or_link_directory(&src, &dest, src_repo);
>                 strbuf_release(&src);
>                 strbuf_release(&dest);
>         }
> --
> 2.21.0.rc2.1.g2d5e20a900.dirty
>

> On Mon, Feb 25, 2019 at 9:26 PM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>>
>> From: Matheus Tavares <matheus.bernardino@usp.br>
>>
>> Replace usage of opendir/readdir/closedir API to traverse directories
>> recursively, at copy_or_link_directory function, by the dir-iterator
>> API. This simplifies the code and avoid recursive calls to
>> copy_or_link_directory.
>>
>> [Ævar: This should be bug-compatible with the existing "clone"
>> behavior. The whole bit here with "iter->relative_path[0] == '.'" is a
>> dirty hack. We don't copy dot-dirs, and then later on just blindly
>> ignore ENOENT errors as we descend into them. That case really wants
>> to be a is_dotdir_or_file_within() test instead]
>>
>> Now, copy_or_link_directory will call die() in case of an error on
>> openddir, readdir or lstat, inside dir_iterator_advance. That means it
>> will abort in case of an error trying to fetch any iteration entry.
>>
>> Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>  builtin/clone.c | 55 +++++++++++++++++++++++++++++--------------------
>>  1 file changed, 33 insertions(+), 22 deletions(-)
>>
>> diff --git a/builtin/clone.c b/builtin/clone.c
>> index 862d2ea69c..c32e9022b3 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"
>> @@ -411,42 +413,47 @@ 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)
>> +                                  const char *src_repo)
>>  {
>> -       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;
>> +       struct stat st;
>>
>>         mkdir_if_missing(dest->buf, 0777);
>>
>> +       iter = dir_iterator_begin(src->buf, 1);
>> +
>>         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)) {
>> +               strbuf_addstr(dest, iter->relative_path);
>> +
>> +               /*
>> +                * dir_iterator_advance already calls lstat to populate iter->st
>> +                * but, unlike stat, lstat does not checks for permissions on
>> +                * the given path.
>> +                */
>> +               if (stat(src->buf, &st)) {
>>                         warning (_("failed to stat %s\n"), src->buf);
>>                         continue;
>>                 }
>> -               if (S_ISDIR(buf.st_mode)) {
>> -                       if (de->d_name[0] != '.')
>> -                               copy_or_link_directory(src, dest,
>> -                                                      src_repo, src_baselen);
>> +
>> +               if (S_ISDIR(iter->st.st_mode)) {
>> +                       if (iter->relative_path[0] == '.')
>
> I think it should be iter->basename[0] here, instead, right?

>> +                               continue;
>> +                       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;
>>                 }
>> @@ -456,14 +463,18 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,
>>                 if (!option_no_hardlinks) {
>>                         if (!link(src->buf, dest->buf))
>>                                 continue;
>> -                       if (option_local > 0)
>> -                               die_errno(_("failed to create link '%s'"), dest->buf);
>> +                       if (option_local > 0 && errno != ENOENT)
>> +                               warning_errno(_("failed to create link '%s'"), dest->buf);
>>                         option_no_hardlinks = 1;
>>                 }
>> -               if (copy_file_with_time(dest->buf, src->buf, 0666))
>> +               if (copy_file_with_time(dest->buf, src->buf, 0666) && errno != ENOENT)
>>                         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);
>> +       }
>>  }
>>
>>  static void clone_local(const char *src_repo, const char *dest_repo)
>> @@ -481,7 +492,7 @@ static void clone_local(const char *src_repo, const char *dest_repo)
>>                 get_common_dir(&dest, dest_repo);
>>                 strbuf_addstr(&src, "/objects");
>>                 strbuf_addstr(&dest, "/objects");
>> -               copy_or_link_directory(&src, &dest, src_repo, src.len);
>> +               copy_or_link_directory(&src, &dest, src_repo);
>>                 strbuf_release(&src);
>>                 strbuf_release(&dest);
>>         }
>> --
>> 2.21.0.rc2.1.g2d5e20a900.dirty
>>

>
> On Sat, Feb 23, 2019 at 11:48 PM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
> >
> >
> > On Sat, Feb 23 2019, 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. This simplifies the code and avoid recursive calls to
> > > copy_or_link_directory.
> >
> > Sounds good in principle.
> >
> > > This process also brings some safe behaviour changes to
> > > copy_or_link_directory:
> >
> > I ad-hoc tested some of these, and could spot behavior changes. We
> > should have tests for these.
>
> I agree that ideally we should have a few tests for these, but this is
> a grey area (see below) and there are areas that are not grey for
> which we don't have any test...
>
> And then adding tests would make this series become larger than a
> typical GSoC micro-project...
>
> > >  - It will no longer follows symbolic links. This is not a problem,
> > >    since the function is only used to copy .git/objects directory, and
> > >    symbolic links are not expected there.
> >
> > I don't think we should make that assumption, and I don't know of
> > anything else in git that does.
>
> I think Git itself doesn't create symlinks in ".git/objects/" and we
> don't recommend people manually tweaking what's inside ".git/". That's
> why I think it's a grey area.
>
> > I've certainly symlinked individual objects or packs into a repo for
> > debugging / recovery, and it would be unexpected to clone that and miss
> > something.
>
> If people tweak what's inside ".git" by hand, they are expected to
> know what they doing and be able to debug it.
>
> > So in the general case we should be strict in what we generate, but
> > permissive in what we accept. We don't want a "clone" of an existing
> > repo to fail, or "fsck" to fail after clone...
>
> Yeah, but realistically I don't think we are going to foolproof Git
> from everything that someone could do by tweaking random things
> manually in ".git/".
>
> I am not saying that it should be ok to make things much worse than
> they are now in case some things have been tweaked in ".git/", but if
> things in general don't look worse in this grey area, and a patch
> otherwise improves things, then I think the patch should be ok.
>
> > When trying to test this I made e.g. objects/c4 a symlink to /tmp/c4,
> > and a specific object in objects/4d/ a symlink to /tmp too.
> >
> > Without this patch the individual object is still a symlink, but the
> > object under the directory gets resolved, and "un-symlinked", also with
> > --dissociate, which seems like an existing bug.
> >
> > With your patch that symlink structure is copied as-is. That's more
> > faithful under --local, but a regression for --dissociate (which didn't
> > work fully to begin with...).
>
> I think that I use --local (which is the default if the repository is
> specified as a local path) much more often than --dissociate, so for
> me the patch would be very positive, especially since --dissociate is
> already buggy anyway in this case.
>
> > I was paranoid that "no longer follows symbolic links" could also mean
> > "will ignore those objects", but it seems to more faithfully copy things
> > as-is for *that* case.
>
> Nice!
>
> > But then I try with --no-hardlinks and stock git dereferences my symlink
> > structure, but with your patches fails completely:
> >
> >     Cloning into bare repository 'repo2'...
> >     error: copy-fd: read returned: Is a directory
> >     fatal: failed to copy file to 'repo2/objects/c4': Is a directory
> >     fatal: the remote end hung up unexpectedly
> >     fatal: cannot change to 'repo2': No such file or directory
>
> Maybe this could be fixed. Anyway I don't use --no-hardlinks very
> often, so I still think the patch is a positive even with this
> failure.
>
> > So there's at least one case in a few minutes of prodding this where we
> > can't clone a working repo now, however obscure the setup.
> >
> > >  - Hidden directories won't be skipped anymore. In fact, it is odd that
> > >    the function currently skip hidden directories but not hidden files.
> > >    The reason for that could be unintentional: probably the intention
> > >    was to skip '.' and '..' only, but it ended up accidentally skipping
> > >    all directories starting with '.'. Again, it must not be a problem
> > >    not to skip hidden dirs since hidden dirs/files are not expected at
> > >    .git/objects.
> >
> > I reproduce this with --local. A ".foo" isn't copied before, now it
> > is. Good, I guess. We'd have already copied a "foo".
> >
> > >  - Now, copy_or_link_directory will call die() in case of an error on
> > >    openddir, readdir or lstat, inside dir_iterator_advance. That means
> > >    it will abort in case of an error trying to fetch any iteration
> > >    entry.
>
> It would be nice if the above paragraph in the commit message would
> say what was the previous behavior and why it's better to die() .
>
> > Good, but really IMNSHO this series is tweaking some critical core code
> > and desperately needs tests.
>
> It's critical that this code works well in the usual case, yes. (And
> there are already a lot of tests that test that.) But when people have
> manually tweaked things in their ".git/objects/", it's not critical
> what happens. Many systems have "undefined behaviors" at some point
> and that's ok.
>
> And no, I am not saying that we should consider it completely
> "undefined behavior" as soon as something is manually tweaked in
> ".git/", and yes, tests would be nice, and your comments and manual
> tests on this are very much appreciated. It's just that I don't think
> we should require too much when a patch, especially from a first time
> contributor, is already improving things, though it also changes a few
> things in a grey area.
>
> > Unfortunately, in this as in so many edge case we have no existing
> > tests.
> >
> > This would be much easier to review and would give reviewers more
> > confidence if the parts of this that changed behavior started with a
> > patch or patches that just manually objects/ dirs with various
>
> I think "created" is missing between "manually" and  "objects/" in the
> above sentence.
>
> > combinations of symlinks, hardlinks etc., and asserted that the various
> > options did exactly what they're doing now, and made sure the
> > source/target repos were the same after/both passed "fsck".
> >
> > Then followed by some version of this patch which changes the behavior,
> > and would be forced to tweak those tests. To make it clear e.g. that
> > some cases where we have a working "clone" are now a hard error.
>
> Unfortunately this would be a lot of work and not appropriate for a
> GSoC micro-project.
>
> Thanks,
> Christian.

> Hi, Christian and Ævar
>
> First of all, thanks for the fast and attentive reviews.
>
> I am a little confused about what I should do next. How should I
> proceed with this series?
>
> By what was said, I understood that the series:
> 1) Is indeed an improvement under --local, because it won't deference
> symlinks in this case.
> 2) Don't make --dissociate any better but as it is already buggy that
> would be some work for another patchset.
> 3) Makes git-clone copy hidden paths which is a good behaviour.
> 4) Breaks --no-hardlinks when there are symlinks at the repo's objects
> directory.
>
> I understood that even though git itself does not create symlinks in
> .git/objects, we should take care of the case where the user manually
> creates them, right? But what would be the appropriate behaviour: to
> follow (i.e. deference) symlinks (the way it is done now) or just copy
> the link file itself (the way my series currently do)? And shouldn't
> we document this decision somewhere?
>
> About the failure with --no-hardlinks having symlinks at .dir/objects,
> it's probably because copy_or_link_directory() is trying to copy a
> file which is a symlink to a dir and the copy function used is trying
> to copy the dir not the link itself. A possible fix is to change
> copy.c to copy the link file, but I haven't studied yet how that could
> be accomplished.
>
> Another possible fix is to make copy_or_link_directory() deference
> symlink structures when --no-hardlinks is given. But because the
> function falls back to no-hardlinks when failing to hardlink, I don't
> think it would be easy to accomplish this without making the function
> *always* deference symlinks. And that would make the series lose the
> item 1), which I understand you liked.

> On Sun, Feb 24, 2019 at 6:41 AM Christian Couder
> <christian.couder@gmail.com> wrote:
>>
>> On Sat, Feb 23, 2019 at 11:48 PM Ævar Arnfjörð Bjarmason
>> <avarab@gmail.com> wrote:
>> >
>> >
>> > On Sat, Feb 23 2019, 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. This simplifies the code and avoid recursive calls to
>> > > copy_or_link_directory.
>> >
>> > Sounds good in principle.
>> >
>> > > This process also brings some safe behaviour changes to
>> > > copy_or_link_directory:
>> >
>> > I ad-hoc tested some of these, and could spot behavior changes. We
>> > should have tests for these.
>>
>> I agree that ideally we should have a few tests for these, but this is
>> a grey area (see below) and there are areas that are not grey for
>> which we don't have any test...
>>
>> And then adding tests would make this series become larger than a
>> typical GSoC micro-project...
>>
>> > >  - It will no longer follows symbolic links. This is not a problem,
>> > >    since the function is only used to copy .git/objects directory, and
>> > >    symbolic links are not expected there.
>> >
>> > I don't think we should make that assumption, and I don't know of
>> > anything else in git that does.
>>
>> I think Git itself doesn't create symlinks in ".git/objects/" and we
>> don't recommend people manually tweaking what's inside ".git/". That's
>> why I think it's a grey area.
>>
>> > I've certainly symlinked individual objects or packs into a repo for
>> > debugging / recovery, and it would be unexpected to clone that and miss
>> > something.
>>
>> If people tweak what's inside ".git" by hand, they are expected to
>> know what they doing and be able to debug it.
>>
>> > So in the general case we should be strict in what we generate, but
>> > permissive in what we accept. We don't want a "clone" of an existing
>> > repo to fail, or "fsck" to fail after clone...
>>
>> Yeah, but realistically I don't think we are going to foolproof Git
>> from everything that someone could do by tweaking random things
>> manually in ".git/".
>>
>> I am not saying that it should be ok to make things much worse than
>> they are now in case some things have been tweaked in ".git/", but if
>> things in general don't look worse in this grey area, and a patch
>> otherwise improves things, then I think the patch should be ok.
>>
>> > When trying to test this I made e.g. objects/c4 a symlink to /tmp/c4,
>> > and a specific object in objects/4d/ a symlink to /tmp too.
>> >
>> > Without this patch the individual object is still a symlink, but the
>> > object under the directory gets resolved, and "un-symlinked", also with
>> > --dissociate, which seems like an existing bug.
>> >
>> > With your patch that symlink structure is copied as-is. That's more
>> > faithful under --local, but a regression for --dissociate (which didn't
>> > work fully to begin with...).
>>
>> I think that I use --local (which is the default if the repository is
>> specified as a local path) much more often than --dissociate, so for
>> me the patch would be very positive, especially since --dissociate is
>> already buggy anyway in this case.
>>
>> > I was paranoid that "no longer follows symbolic links" could also mean
>> > "will ignore those objects", but it seems to more faithfully copy things
>> > as-is for *that* case.
>>
>> Nice!
>>
>> > But then I try with --no-hardlinks and stock git dereferences my symlink
>> > structure, but with your patches fails completely:
>> >
>> >     Cloning into bare repository 'repo2'...
>> >     error: copy-fd: read returned: Is a directory
>> >     fatal: failed to copy file to 'repo2/objects/c4': Is a directory
>> >     fatal: the remote end hung up unexpectedly
>> >     fatal: cannot change to 'repo2': No such file or directory
>>
>> Maybe this could be fixed. Anyway I don't use --no-hardlinks very
>> often, so I still think the patch is a positive even with this
>> failure.
>>
>> > So there's at least one case in a few minutes of prodding this where we
>> > can't clone a working repo now, however obscure the setup.
>> >
>> > >  - Hidden directories won't be skipped anymore. In fact, it is odd that
>> > >    the function currently skip hidden directories but not hidden files.
>> > >    The reason for that could be unintentional: probably the intention
>> > >    was to skip '.' and '..' only, but it ended up accidentally skipping
>> > >    all directories starting with '.'. Again, it must not be a problem
>> > >    not to skip hidden dirs since hidden dirs/files are not expected at
>> > >    .git/objects.
>> >
>> > I reproduce this with --local. A ".foo" isn't copied before, now it
>> > is. Good, I guess. We'd have already copied a "foo".
>> >
>> > >  - Now, copy_or_link_directory will call die() in case of an error on
>> > >    openddir, readdir or lstat, inside dir_iterator_advance. That means
>> > >    it will abort in case of an error trying to fetch any iteration
>> > >    entry.
>>
>> It would be nice if the above paragraph in the commit message would
>> say what was the previous behavior and why it's better to die() .
>>
>> > Good, but really IMNSHO this series is tweaking some critical core code
>> > and desperately needs tests.
>>
>> It's critical that this code works well in the usual case, yes. (And
>> there are already a lot of tests that test that.) But when people have
>> manually tweaked things in their ".git/objects/", it's not critical
>> what happens. Many systems have "undefined behaviors" at some point
>> and that's ok.
>>
>> And no, I am not saying that we should consider it completely
>> "undefined behavior" as soon as something is manually tweaked in
>> ".git/", and yes, tests would be nice, and your comments and manual
>> tests on this are very much appreciated. It's just that I don't think
>> we should require too much when a patch, especially from a first time
>> contributor, is already improving things, though it also changes a few
>> things in a grey area.
>>
>> > Unfortunately, in this as in so many edge case we have no existing
>> > tests.
>> >
>> > This would be much easier to review and would give reviewers more
>> > confidence if the parts of this that changed behavior started with a
>> > patch or patches that just manually objects/ dirs with various
>>
>> I think "created" is missing between "manually" and  "objects/" in the
>> above sentence.
>>
>> > combinations of symlinks, hardlinks etc., and asserted that the various
>> > options did exactly what they're doing now, and made sure the
>> > source/target repos were the same after/both passed "fsck".
>> >
>> > Then followed by some version of this patch which changes the behavior,
>> > and would be forced to tweak those tests. To make it clear e.g. that
>> > some cases where we have a working "clone" are now a hard error.
>>
>> Unfortunately this would be a lot of work and not appropriate for a
>> GSoC micro-project.
>>
>> Thanks,
>> Christian.

>
>
> On Mon, Feb 25 2019, Matheus Tavares Bernardino wrote:
>
> > Hi, Christian and Ævar
> >
> > First of all, thanks for the fast and attentive reviews.
> >
> > I am a little confused about what I should do next. How should I
> > proceed with this series?
> >
> > By what was said, I understood that the series:
> > 1) Is indeed an improvement under --local, because it won't deference
> > symlinks in this case.
> > 2) Don't make --dissociate any better but as it is already buggy that
> > would be some work for another patchset.
> > 3) Makes git-clone copy hidden paths which is a good behaviour.
> > 4) Breaks --no-hardlinks when there are symlinks at the repo's objects
> > directory.
> >
> > I understood that even though git itself does not create symlinks in
> > .git/objects, we should take care of the case where the user manually
> > creates them, right? But what would be the appropriate behaviour: to
> > follow (i.e. deference) symlinks (the way it is done now) or just copy
> > the link file itself (the way my series currently do)? And shouldn't
> > we document this decision somewhere?
> >
> > About the failure with --no-hardlinks having symlinks at .dir/objects,
> > it's probably because copy_or_link_directory() is trying to copy a
> > file which is a symlink to a dir and the copy function used is trying
> > to copy the dir not the link itself. A possible fix is to change
> > copy.c to copy the link file, but I haven't studied yet how that could
> > be accomplished.
> >
> > Another possible fix is to make copy_or_link_directory() deference
> > symlink structures when --no-hardlinks is given. But because the
> > function falls back to no-hardlinks when failing to hardlink, I don't
> > think it would be easy to accomplish this without making the function
> > *always* deference symlinks. And that would make the series lose the
> > item 1), which I understand you liked.
>
> I don't really have formed opinions one way or the other about what
> these specific flags should do in combination with such a repository,
> e.g. should --dissociate copy data rather than point to the same
> symlinks?
>
> I'm inclined to think so, but I've only thought about it for a couple of
> minutes. Maybe if someone starts digging they'll rightly come to a
> different conclusion.

>
> Add pedantic option to dir_iterator_begin at dir-iterator.c and convert
> explicit recursive directory traversal at copy_or_link_directory
> (builtin/clone.c) to the dir-iterator API.
>
> This is my microproject for GSoC 2019. Idea taken from
> https://git.github.io/SoC-2019-Microprojects/#use-dir-iterator-to-avoid-explicit-recursive-directory-traversal
>
> Build: https://travis-ci.org/MatheusBernardino/git/builds/497512561
>
> Matheus Tavares (3):
>   dir-iterator: add pedantic option to dir_iterator_begin
>   clone: extract function from copy_or_link_directory
>   clone: use dir-iterator to avoid explicit dir traversal
>
>  builtin/clone.c      | 72 ++++++++++++++++++++++++++++----------------
>  dir-iterator.c       | 23 ++++++++++++--
>  dir-iterator.h       | 16 ++++++++--
>  refs/files-backend.c |  2 +-
>  4 files changed, 81 insertions(+), 32 deletions(-)
>

> On Sat, Feb 23, 2019 at 4:03 PM Matheus Tavares
> <matheus.bernardino@usp.br> wrote:
>>
>> Add pedantic option to dir_iterator_begin at dir-iterator.c and convert
>> explicit recursive directory traversal at copy_or_link_directory
>> (builtin/clone.c) to the dir-iterator API.
>>
>> This is my microproject for GSoC 2019. Idea taken from
>> https://git.github.io/SoC-2019-Microprojects/#use-dir-iterator-to-avoid-explicit-recursive-directory-traversal
>>
>> Build: https://travis-ci.org/MatheusBernardino/git/builds/497512561
>>
>> Matheus Tavares (3):
>>   dir-iterator: add pedantic option to dir_iterator_begin
>>   clone: extract function from copy_or_link_directory
>>   clone: use dir-iterator to avoid explicit dir traversal
>>
>>  builtin/clone.c      | 72 ++++++++++++++++++++++++++++----------------
>>  dir-iterator.c       | 23 ++++++++++++--
>>  dir-iterator.h       | 16 ++++++++--
>>  refs/files-backend.c |  2 +-
>>  4 files changed, 81 insertions(+), 32 deletions(-)
>>
>
> Sadly, I forgot the v2 tag, but this is a v2. Should I resent the
> series with [PATCH v2 ...] or is it ok?

> This is my microproject for GSoC 2019. Idea taken from
> https://git.github.io/SoC-2019-Microprojects/#use-dir-iterator-to-avoid-explicit-recursive-directory-traversal
> 
> Build: https://travis-ci.org/MatheusBernardino/git/builds/497512561
> 
> Matheus Tavares (3):
>   dir-iterator: add pedantic option to dir_iterator_begin
>   clone: extract function from copy_or_link_directory
>   clone: use dir-iterator to avoid explicit dir traversal
> 
>  builtin/clone.c      | 72 ++++++++++++++++++++++++++++----------------
>  dir-iterator.c       | 23 ++++++++++++--
>  dir-iterator.h       | 16 ++++++++--
>  refs/files-backend.c |  2 +-
>  4 files changed, 81 insertions(+), 32 deletions(-)
> 
> -- 
> 2.20.1
> 

>
> On 02/23, Matheus Tavares wrote:
> > Add pedantic option to dir_iterator_begin at dir-iterator.c and convert
> > explicit recursive directory traversal at copy_or_link_directory
> > (builtin/clone.c) to the dir-iterator API.
>
> Thanks for another iteration of this.
>
> To make life easier for reviewers, please include a link (or the
> message-ID) to all previous iteration of the series.  We often use
> links to the public-inbox mirror for this,
> e.g. https://public-inbox.org/git/20190215154913.18800-1-matheus.bernardino@usp.br/.
>
> This helps reviewers go back quickly to previous iterations of the
> series, and refresh their memory on the comments that were left
> there.
>
> You can also use the --in-reply-to option in 'git send-email' to chain
> the threads, which also makes life easier for reviewers.
>
> An additional way to help reviewers is to include a 'range-diff'
> between the previous iteration of the series, and the current
> iteration.  See the 'git range-diff' command or the '--range-diff'
> option to 'git format-patch' for that.  That helps reviewers to
> quickly see what changed between iterations, so that they don't have
> to re-review everything, if they can still remember the last round
> well enough.
>
> I also added Christian Couder back to the Cc list, as he commented on
> the RFC.  It's good to keep people that commented on the series in Cc,
> as they have shown some interest in the series, so keeping them in the
> Cc list helps highlight those emails for them, and makes it more
> likely that the patches get reviewed quickly.
>

> > This is my microproject for GSoC 2019. Idea taken from
> > https://git.github.io/SoC-2019-Microprojects/#use-dir-iterator-to-avoid-explicit-recursive-directory-traversal
> >
> > Build: https://travis-ci.org/MatheusBernardino/git/builds/497512561
> >
> > Matheus Tavares (3):
> >   dir-iterator: add pedantic option to dir_iterator_begin
> >   clone: extract function from copy_or_link_directory
> >   clone: use dir-iterator to avoid explicit dir traversal
> >
> >  builtin/clone.c      | 72 ++++++++++++++++++++++++++++----------------
> >  dir-iterator.c       | 23 ++++++++++++--
> >  dir-iterator.h       | 16 ++++++++--
> >  refs/files-backend.c |  2 +-
> >  4 files changed, 81 insertions(+), 32 deletions(-)
> >
> > --
> > 2.20.1
> >

> I am part of a FLOSS group here at USP called FLUSP
> (https://flusp.ime.usp.br/), and I plan to write some posts on our
> website about what I am learning in the git community so that other
> people in the group can have as a base if they decide to start
> contributing to git too. So all this tips and explanations are of
> great value, not only for me but for others here! Thanks a lot.