[GSoC,3/3] clone: use dir-iterator to avoid explicit dir traversal
diff mbox series

Message ID 20190223190309.6728-4-matheus.bernardino@usp.br
State New
Headers show
Series
  • clone: convert explicit dir traversal to dir-iterator
Related show

Commit Message

Matheus Tavares Bernardino Feb. 23, 2019, 7:03 p.m. UTC
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
 - Put warning on stat error back
 - Added pedantic option to dir-iterator initialization
 - Modified copy_or_link_directory not to skip hidden paths

 builtin/clone.c | 47 ++++++++++++++++++++++++++++-------------------
 1 file changed, 28 insertions(+), 19 deletions(-)

Comments

Thomas Gummerer Feb. 23, 2019, 9:48 p.m. UTC | #1
On 02/23, 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.
> 
> 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

s/variable/parameter/ I believe?

>  - Put warning on stat error back
>  - Added pedantic option to dir-iterator initialization
>  - Modified copy_or_link_directory not to skip hidden paths

Thanks, these descriptions are very useful for reviewers that had a
look at previous rounds.

>  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.
> +		 */

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.

> +		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
>
Ævar Arnfjörð Bjarmason Feb. 23, 2019, 10:40 p.m. UTC | #2
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.

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.

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...

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 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.

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.

Good, but really IMNSHO this series is tweaking some critical core code
and desperately needs tests.

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.

Thanks for working on this!
Christian Couder Feb. 24, 2019, 9:41 a.m. UTC | #3
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.
Ævar Arnfjörð Bjarmason Feb. 24, 2019, 2:45 p.m. UTC | #4
On Sun, Feb 24 2019, Christian Couder 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.

Here's a test that works before 3/3 and fails after, can be used with my SOB:

    diff --git a/t/t5604-clone-reference.sh b/t/t5604-clone-reference.sh
    index 4320082b1b..80c0a4a19b 100755
    --- a/t/t5604-clone-reference.sh
    +++ b/t/t5604-clone-reference.sh
    @@ -221,4 +221,37 @@ test_expect_success 'clone, dissociate from alternates' '
     	( cd C && git fsck )
     '

    +test_expect_success SHA1,SYMLINKS 'setup repo with manually symlinked objects/*' '
    +	git init S &&
    +	(
    +		cd S &&
    +		test_commit A &&
    +		git gc &&
    +		test_commit B &&
    +		(
    +			cd .git/objects &&
    +			mv 22/3b7836fb19fdf64ba2d3cd6173c6a283141f78 . &&
    +			ln -s ../3b7836fb19fdf64ba2d3cd6173c6a283141f78 22/ &&
    +			mv 40 forty &&
    +			ln -s forty 40 &&
    +			mv pack packs &&
    +			ln -s packs pack
    +		)
    +	)
    +'
    +
    +test_expect_success SHA1,SYMLINKS 'clone repo with manually symlinked objects/*' '
    +	for option in --local --no-hardlinks --shared --dissociate
    +	do
    +		git clone $option S S$option || return 1 &&
    +		git -C S$option fsck || return 1
    +	done &&
    +	find S-* -type l | sort >actual &&
    +	cat >expected <<-EOF &&
    +	S--dissociate/.git/objects/22/3b7836fb19fdf64ba2d3cd6173c6a283141f78
    +	S--local/.git/objects/22/3b7836fb19fdf64ba2d3cd6173c6a283141f78
    +	EOF
    +	test_cmp expected actual
    +'
    +
     test_done

Obviously not ideal, there's a way to do this without relying on the
SHA1 prereq, but in this case I don't see much point. Also similar to
the test_cmp there it would make senses to create
.git/objects/{foo,.foo} and see if they show up in the clone.

The GSOC project is basically "replace existing thing with identical
utility function", but software can always be more/less complex when it
gets to writing the code, so now it's become "we no longer uniformly
accept the same repository formats across fsck/clone as a side-effect".

To me that's not a grey area. Yes we don't produce that ourselves, but
it's the on-disk format we've always supported, and can expect e.g. that
someone's symlinking objects/?? to different partitions etc. in some
advanced setups.

Can't the utility function we're moving to just be made to be
bug-compatible with what we're doing now with symlinks?
Matheus Tavares Bernardino Feb. 24, 2019, 6:19 p.m. UTC | #5
On Sat, Feb 23, 2019 at 6:48 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:
>
> On 02/23, Matheus Tavares wrote:
> > ---
> > Changes in v2:
> >  - Improved patch message
> >  - Removed a now unused variable
>
> s/variable/parameter/ I believe?

Yes, you are right!

> > +     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.
>

Ok, I also read the lstat man page wrongly and though it didn't check
for permissions. Thanks for noticing that. I will remove the lstat
call in v3.
Matheus Tavares Bernardino Feb. 25, 2019, 2:31 a.m. UTC | #6
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.

So, what would be the best here?

Best,
Matheus Tavares

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.
Duy Nguyen Feb. 25, 2019, 9:45 a.m. UTC | #7
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.
Ævar Arnfjörð Bjarmason Feb. 25, 2019, 10:25 a.m. UTC | #8
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.

Rather, my comment is on the process. Clone behavior is too important to
leave to prose in a commit message. I already found a case where we hard
error not explicitly called out, are there other edge cases I didn't
think of?

So having this e.g. be a 4-part series where 3/4 is introducing tests in
the direction I posted upthread (but needs more work), with 4/4 going
through/justifying each one.


> 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.
Christian Couder Feb. 25, 2019, 8:40 p.m. UTC | #9
On Mon, Feb 25, 2019 at 11:25 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
>
> 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.

And maybe one day we will find a very good way to take advantage of
symlinks in .git/objects/ when Git is used normally, but that will go
against what we have decided now, though we have no real need at all
to decide now.

That's why I think it can actually be a good thing not to decide
anything now, and to let us free to decide later.

It's kind of the same as with short options versus long options. It's
a good idea not to use up all your short options too soon (in the name
of current ease of use), and instead wait until people really want a
short option for one option they use really often before attributing
it to this option.

> Rather, my comment is on the process. Clone behavior is too important to
> leave to prose in a commit message.

The same argument could be made to say that what happens in
.git/objects/ when cloning and there are symlinks is too important a
still free design option we have left to give it up right now for no
good reason.
Christian Couder Feb. 26, 2019, 10:33 a.m. UTC | #10
Hi Matheus,

On Mon, Feb 25, 2019 at 3:31 AM Matheus Tavares Bernardino
<matheus.bernardino@usp.br> wrote:
>
> I am a little confused about what I should do next. How should I
> proceed with this series?

When there are different opinions about what you should do, I would
suggest doing only things everyone is ok with, and then resending
saying in the cover letter that such and such issues still need to be
decided. Then hopefully either Junio will decide or a consensus will
eventually appear about what you should do.

In any case please don't wait until something is decided before
resending with improvements (or just pinging us if you have already
made all the improvements that were suggested) as that may not happen.
If people on the list stop hearing about your patch series, they might
just forget about it.

Thanks,
Christian.

Patch
diff mbox series

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);
 	}