Re: [PATCH v3 0/8] builtin: implement, document and test url-parse
From: Matheus Afonso Martins Moreira <hidden>
Date: 2026-05-03 19:36:55
Reviewers comment: Nicely done.
Thank you!
More a question to myself, may be, about t9904 (and may be other parts) I have in mind that the parser learned to handle file://server/share/repo correctly under Windows. I don't know if this needs to be addressed here or in a follow-up commit ?
I'd be happy to revisit this in a follow-up. It's been a while
since I used MSYS but I do remember the fact it rewrites paths
internally. I wasn't sure how to handle it properly in the tests.
The problematic test case is:
test_must_fail git url-parse "/abs/path" 2>err &&
test_grep "is not a URL" err &&
test_grep "file:///abs/path" err
MSYS bash rewrites /abs/path to C:/Program Files/Git/abs/path
before git even runs. This edge case caused the error message:
fatal: 'C:/Program Files/Git/abs/path' is not a URL;
if you meant a local repository, use a 'file://' URL
with an absolute path
The test_grep "is not a URL" passed but test_grep "file:///abs/path"
failed because the suggestion did not contain the literal string
"file:///abs/path". The drive letter broke the tool's absolute
path recognition: it was printing the generic error message.
The fix was to use has_dos_drive_prefix() to recognize the edge case.
However, that led to the generation of error messages containing paths
that I wasn't sure if I could depend on in the test suite, such as:
file:///C:/Program Files/Git/abs/path
So I decided to relax the test case just a little:
test_must_fail git url-parse "/abs/path" 2>err &&
test_grep "is not a URL" err &&
test_grep "file:///" err
The "file:///" checks that the path was properly recognized
and that the friendlier error message was printed, all while
avoiding the hard coding of a "C:/Program Files/Git" prefix
that may or may not vary depending on testing environment.
In any case, the parser already handles it correctly.
It decomposes:
file://server/share/repo
As:
- scheme: file
- host: server
- path: /share/repo
Which is the correct interpretation.
On Windows, connect.c then takes that data and reconstructs
the UNC path \\server\share\repo for the filesystem.
So the UNC reconstruction happens downstream in connect.c,
not directly in the url-parse builtin or url_parse logic.
Matheus