Re: [GSoC][PATCH 3/3] clone: use dir-iterator to avoid explicit dir traversal

From: Christian Couder <christian.couder@gmail.com>
Date: Feb 24, 2019, 09:41 UTC
Message-ID: <CAP8UFD2LLL+V54k3XmHzmg5t1zanpAvY_=7GqXL43vLPTTmuww@mail.gmail.com>
In-Reply-To: <87va1a3z8e.fsf@evledraar.gmail.com>

On Sat, Feb 23, 2019 at 11:48 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 16 quoted lines

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

Show 6 quoted lines

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

Show 10 quoted lines

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

Show 8 quoted lines

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

Show 18 quoted lines

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

Show 6 quoted lines

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

Show 7 quoted lines

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