Reply to community feedback

From: Matheus Afonso Martins Moreira <matheus.a.m.moreira@gmail.com>
Date: Apr 29, 2024, 22:04 UTC
Message-ID: <e7f49f373b2a3b51785d369e1f504825@gmail.com>
In-Reply-To: <20240429205351.GA27257@tb-raspi4>

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