netops: handle intact query parameters in service_suffix removal #5339
Conversation
There was a problem hiding this comment.
I honestly find it very hard to reason about this, but this is no fault of yours. We do have tests in "tests/network/redirect.c" that are supposed to exercise this function. Would you feel up to the task of adding some tests in there that show what exact scenario you're fixing? Feel free to say "no" here and I'll try to come up with some tests myself based on your description.
Some servers leave the query parameters intact in the Location header when responding with a redirect. The service_suffix removal check as written assumed that the server removed them. Handle both cases. Along with PR libgit2#5325, this fixes libgit2#5321. There are two new tests. The first already passed; the second previously failed.
That's kind, but I'll take the fault. I've added more documentation in the code.
Thanks for the pointer. I hadn't found those, only the tests against live servers. I've added two tests. The first already passed; the second previously failed. |
|
|
||
| /* Check for a redirect without query parameters, like "/newloc/info/refs" */ |
There was a problem hiding this comment.
Thanks a lot, these comments make things much clearer to me!
Some servers leave the query parameters intact in the
Location header when responding with a redirect.
The service_suffix removal check as written assumed
that the server removed them.
Handle both cases.
Along with PR #5325, this fixes #5321.
Please review skeptically; my C is extremely rusty.