On Tue, May 29, 2012 at 6:07 AM, Jens Lehmann <Jens.Lehmann@xxxxxx> wrote: > Am 27.05.2012 17:34, schrieb Jon Seymour: >> This series improves the robustness of path handling by 'git submodule' by: >> >> * detecting submodule URLs that will result in non-sensical submodule origin URLs >> >> * improving handling of various kinds of relative superproject origin URLs >> >> * improving handling of various kinds of denormalized superproject origin URLs > > Hmm, this has become a quite invasive patch series. While I bought the > use case of having a superproject with a relative url and was inclined > to accept that it might even not start "./" or "../" (even though that > is a pretty unusual use and can be easily fixed by prepending a "./"), > I'm not sure the in depth check of URLs is worth the code churn. And > especially the high probability of breaking other peoples use cases in > rather subtle ways worry me (this did happen quite often when the > submodule script was changed in the past; as an example take the > windows path issues Johannes already pointed out in his email). And I > can't remember bug reports that people complained about URL problems > due to the issues you intend to fix here, which makes me think they > might be well intended but possibly unnecessary (but my memory might > server me wrong here). > > So I'd vote for just fixing the relative submodule path issues and to > not care about the possible issues with URLs. Opinions? I'll write a minimal patch to solve my relative path problem without fixing the invalid/"greedy" submodule url or url normalization issues. The reason I went with a more extensive series is that the change in 6/9 considerably simplified the change I wanted to make in 7/9 while at the same time making the path handling of resolve_relative_url more precise, in the sense documented by the tests in 2/9. The refactoring in 5/9 and the changes in 8/9 and 9/9 are related to renormalization which I realised was a weakness (if not a problem people were complaining about) in the original code as documented by the tests in 4/9. Do you have any comments about whether the failures documented in 2/9 and 4/9 are worth noting, at least, as weaknesses? > > (And patches 6-8 contain changes to test cases other than just changing > test_expect_failure to test_expect_success which makes reviewing this > series unnecessarily hard) Agree absolutely about patch 8 - I will re-roll with separate tests to document the test setup issue I fixed in 8. The only other changes to tests in 6 and 7 were the removal of comments about the actual bad behaviour. Would your preference be that I removed these #actual comments completely or that I moved documentation of the actual behaviour to the header of the test? jon. -- 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