git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v7 0/9] submodule: improve robustness of path handling

From
Jon Seymour <jon.seymour@gmail.com>
Date
May 28, 2012, 22:01 UTC
Message-ID
<CAH3AnroT1vs-s==ykNyogq6gbVncY0pt5U1=fMp+b6B0jwG19Q@mail.gmail.com>
In-Reply-To
<4FC3DAEF.1070508@web.de>
On Tue, May 29, 2012 at 6:07 AM, Jens Lehmann <Jens.Lehmann@web.de> wrote:
Show 25 quoted lines
> 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.
Previous: Jens LehmannNext: Jens Lehmann
Message 16 of 17 in “submodule: improve robustness of path handling”
  1. 0/9 submodule: improve robustness of path handlingJon Seymour, May 27, 2012
  2. 1/9 submodule: additional regression tests for relative URLsJon Seymour, May 27, 2012
  3. 2/9 submodule: document failure to detect invalid submodule URLsJon Seymour, May 27, 2012
  4. 3/9 submodule: document failure to handle relative superproject origin URLsJon Seymour, May 27, 2012
  5. 4/9 submodule: document failure to handle improperly normalized remote origin URLsJon Seymour, May 27, 2012
  6. 5/9 submodule: extract normalize_path into standalone functionJon Seymour, May 27, 2012
  7. 6/9 submodule: fix detection of invalid submodule URLJon Seymour, May 27, 2012
  8. Johannes SixtMay 28, 2012
  9. Jon SeymourMay 28, 2012
  10. Jon SeymourJun 3, 2012
  11. 7/9 submodule: fix sync handling of relative superproject origin URLsJon Seymour, May 27, 2012
  12. 8/9 submodule: fix handling of denormalized superproject origin URLsJon Seymour, May 27, 2012
  13. Jon SeymourMay 27, 2012
  14. 9/9 submodule: fix normalization to handle repeated ./Jon Seymour, May 27, 2012
  15. Jens LehmannMay 28, 2012
  16. Jon SeymourMay 28, 2012
  17. Jens LehmannMay 29, 2012

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.