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

From: Thomas Gummerer <t.gummerer@gmail.com>
Date: Feb 21, 2019, 21:25 UTC
Message-ID: <20190221212522.GO6085@hank.intra.tgummerer.com>
In-Reply-To: <CAHd-oW6VPrHMo1QjAXn6G8P_gmJOQ91LVO+UDK1a8T7uhuM9uQ@mail.gmail.com>

On 02/21, Matheus Tavares Bernardino wrote:

Show 39 quoted lines

> Ok, I think I'm almost there and I should be able to send a v2 on the
> weekend. But again, a few questions arose while I'm coding v2. Please,
> see inline.
> 
> On Tue, Feb 19, 2019 at 8:45 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:
> >
> > On 02/19, Matheus Tavares Bernardino wrote:
> > > Ok, I agree. I noticed copy_or_link_directory only skips hidden
> > > directories, not hidden files. And I couldn't think why we would want
> > > to skip one but not the other... Could that be unintentional? I mean,
> > > maybe the idea was to skip just "." and ".."? If that is the case, I
> > > don't even need to make an if check at copy_or_link_directory, since
> > > dir-iterator already skips "." and "..". What do you think about that?
> 
> [...]
> 
> > I feel like you are probably right in that it could be unintentional,
> > and I don't think anyone should be messing with the objects
> > directory.  However that is no guarantee that changing this wouldn't
> > break *something*.
> >
> > For the purpose of this change I would try to keep the same behaviour
> > as we currently have where possible, to not increase the scope too
> > much.
> >
> 
> I understand your point, but to make copy_or_link_directory() skip
> just hidden directories using dir-iterator seems, now, a little
> tricker than I though before. The "if (iter->basename[0] != '.')"
> statement in the patch I sent for v1 only skips the hidden directories
> creation, but it does not avoid the attempt to copy the hidden
> directory contents (which would obviously fail). If we were to do
> that, we would need to check every directory in the relative path
> string of each iteration entry looking for a hidden directory. That
> seems a little too much, IMHO. Because of that and the intuition that
> skipping over hidden dirs was an unintentional operation, I think we
> could modify copy_or_link_directory() to skip '.' and '..', only.
> Also, hidden dirs/files are not expected to be at .git/objects anyway,
> are they?

No, as far as I know hidden dirs/files should not be in .git/objects. The only reason they would be there is if somebody would create them manually. I didn't think of hidden child directories, which as you noticed should be excluded as well to keep current behaviour.

With that in mind, I think I agree with the change of only excluding '.' and '..', for which we don't need to do anything in your dir-iterator patches. Also thinking of what the worst case that could happen is, it's that while cloning a repository locally a few more directories could be copied, which is not all that bad.

So I think I was overly paranoid here and am on board with skipping '.' and '..' only. I even think that this can be just described in detail in the commit message, rather than being done in a separate patch, though others may disagree with that.

Show 6 quoted lines

> And to have copy_or_link_directory()'s behaviour changed as little as
> possible, I could add the option to not follow hidden paths (dirs and
> regular files) at dir-iterator.c and use it at
> copy_or_link_directory() (now that I'm adding the 'pedantic' option,
> this won't be so difficult, anyway). Would these suggestions be a good
> plan?

I think this would be a valid plan as well, however thinking this through again I don't feel like it is necessary. I'm happy with whichever way you prefer here.

> Thanks,
> Matheus Tavares