On 02/21, Matheus Tavares Bernardino wrote: > 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@xxxxxxxxx> 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. > 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
