On 11-06-28 06:32 PM, Jens Lehmann wrote: > Am 29.06.2011 00:06, schrieb Marc Branchaud: >> First, I expect performance will be terrible in repositories with large >> and/or many submodules. I'd go so far as to say that it's pretty much a >> show-stopper for our repository. > > Large submodules won't be the problem here, but many submodules and/or > many commits might cause some performance degradations here. But please > notice that there is no communication with the upstream of the submodules, > we only check the refs present locally. Do you still think doing some > "git rev-list" invocations in your submodules will be a problem? Then > please say so. Yes, that's exactly what I'm saying. I'm concerned about rev-list's performance, particularly when my system's disk cache is empty (or already full with something else). When I saw the patch I tried a quick git rev-list X --not --remotes -n 1 in a Linux kernel submodule where X was a non-existent ref, and watched my disk whirl for a few minutes. Admittedly that's nothing like what the patch does -- mea culpa. I see now that the check looks for refs in the submodule's recent history, so the scan is likely to be pretty short. Also, as you say below and Fredrik said in his reply, the patch only checks submodules affected by a super-repo ref that is being pushed. So I agree that the performance hit is likely to be minimal even with large submodules. (Fredrik, FYI my super-repo itself is large enough to make "git status" take up a good part of the disk cache. This repo has several submodules, including a few (different) Linux kernel repos. I already get bogged down a bit when I have to status the main repo and one of the Linux submodules. So anything that adds extra submodule inspection makes me nervous.) >> Second, there are many times where I'm working in a submodule on branch >> "TopicA" but not in branch "TopicB". If I've made submodule changes in >> TopicA then try to push up TopicB, won't I have have to tell push to "-f"? >> But that turns off other checks that I'd rather keep. > > Nope, this patch only checks the refs to be pushed, not any others. So it > will only check that all submodule commits on branch "TopicB" are pushed. Thanks for pointing that out! >> I'd feel a lot better about this patch if the check was *off* by default and >> there was a config setting / command-line option to turn it on. > > I have no objections against making that configurable, although I tend > towards making this check default "on". But please feel free to test this > feature and tell us if it really hinders you in your work or does cause > performance degradation, we'll really appreciate the feedback! Well I'm less concerned about it being configurable now. I did a few more tests and the rev-list performance looked fine. M. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html