On 02/19, Matheus Tavares Bernardino wrote: > On Mon, Feb 18, 2019 at 8:35 PM Thomas Gummerer <t.gummerer@xxxxxxxxx> wrote: > > > Also, I just noticed that dir-iterator follows hidden paths while > > > copy_or_link_directory don't. Maybe another option to add for > > > dir-iterator? > > > > That feels like quite a special case in 'copy_or_link_directory()'. I > > don't think it's worth adding a feature like that for now, at least > > not until we find another use case for it. It's easy enough to skip > > over hidden directories here. > > > > 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 don't know. The best way to look at these things is usually to use 'git blame' and have a look at the commit that introduced whatever you are looking at, in this case why we are skipping anything starting with a '.'. But looking at that commit it's just the translation from shell script to builtin. It's also more than 10 years old by now, so it's unlikely that the original author would still remember why exactly they made this choice. (Note that I didn't take the time to actually dig into the original shell script). 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'm proposing some new options to dir-iterator because as the patch > > > that adds it says "There are obviously a lot of features that could > > > easily be added to this class", and maybe those would be useful for > > > other dir-iterator users. But I don't know if that would be the best > > > way of doing it, so any feedback on this will be much appreciated. > > > Also, I saw on public-inbox[1] a patchset from 2017 proposing new > > > features/options for dir-iterator, but I don't really know if (and > > > why) it was rejected. (I couldn't find the patches on master/pu log) > > > > I don't think we should necessarily add new options because the > > original patch said we should do so. Instead the best way to > > introduce new options is to introduce them when they are actually > > going to be needed in subsequent patches. So it would be nice to > > implement features that are actually needed by your patches, but I > > don't think it's worth trying to introduce new features that could > > potentially be useful in the future, but where we don't know yet > > whether they really will be or not. > > > > Yes, I agree with you. What I meant is that there is some space for > new features at dir-iterator and maybe those that I suggested could be > nice not just for my patch, on clone.c, but for others. But I see your > point. We really should try to keep it simpler and just add the > feature when (and if) there is a more expressive usage for it. Right, if you wanted to pursue this, that could be a separate patch series. However generally patch series that just introduce something that might be used later, but doesn't add much value on its own tend to be much more likely to be rejected. > Thanks again for all the help and feedback. Thanks for working on this!
