Reply to community feedback

[Matheus Afonso Martins Moreira <matheus.a.m.moreira@gmail.com>]

Thank you for your feedback.

> are there any plans to integrate the parser into connect.c and fetch ?

Yes.

That was my intention but I was not confident enough to touch connect.c
before getting feedback from the community, since it's critical code
and it is my first contribution.

I do want to merge all URL parsing in git into this one function though,
thereby creating a "single point of truth". This is so that if the algorithm
is modified the changes are visible to the URL parser builtin as well.

> Speaking as a person, who manage to break the parsing of URLs once,
> with the good intention to improve things, I need to learn that
> test cases are important.

Absolutely agree.

When adding test cases, I looked at the possibilities enumerated in urls.txt
and generated test cases based on those. I also looked at the urlmatch.h
test cases. However...

> Some work can be seen in t5601-clone.sh

... I did not think to check those.

> Especially, when dealing with literal IPv6 addresses,
> the ones with [] and the simplified ssh syntax 'myhost:src'
> are interesting to test.

You're right about that. I shall prepare an updated v2 patchset
with more test cases, and also any other changes/improvements
requested by maintainers.

> And some features using the [] syntax to embedd a port number
> inside the simplified ssh syntax had not been documented,
> but used in practise, and are now part of the test suite.
> See "[myhost:123]:src" in t5601

Indeed, I did not read anything of the sort when I checked it.
Would you like me to commit a note to this effect to urls.txt ?

> Or is this new tool just a helper, to verify "good" URL's,
> and not accepting our legacy parser quirks ?

It is my intention that this builtin be able to accept, parse
and decompose all types of URLs that git itself can accept.

> Then we still should see some IPv6 tests ?

I will add them!

> Or may be not, as we prefer hostnames these days ?

I would have to defer that choice to someone more experienced
with the codebase. Please advise on how to proceed.

> The RFC 1738 uses the term "scheme" here, and using the very generic
> term "protocol" may lead to name clashes later.
> Would something like "git_scheme" or so be better ?

Scheme does seem like a better word if it's the terminology used by RFCs.
I can change that in a new version if necessary.
That code is based on the existing connect.c parsing code though.

> I think that the "///" version is superflous, it should already
> be covered by the "//" version

I thought it was a good idea because of existing precedent:
my first approach to creating the test cases was to copy the
ones from t0110-urlmatch-normalization.sh which did have many
cases such as those. Then as I developed the code I came to
believe that it was not necessary: I call url_normalize
in the url_parse function and url_normalize is already being
tested. I think I just forgot to delete those lines.

Reading that file over once again, it does have IPv6 address
test cases. So I should probably go over it again.

Thanks again for the feedback,

  Matheus