Fix mutating path of URL without authority (idempotency, empty path segments)

[theskim]

From Issue #984

[codecov]

Codecov Report

Attention: Patch coverage is 93.47826% with 3 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@adb8660). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
url/src/path_segments.rs	88.88%	2 Missing ⚠️
url/src/lib.rs	96.42%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #996   +/-   ##
=======================================
  Coverage        ?   80.38%           
=======================================
  Files           ?       24           
  Lines           ?     4364           
  Branches        ?        0           
=======================================
  Hits            ?     3508           
  Misses          ?      856           
  Partials        ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[valenting]

If I'm not mistaken, these failing tests should also be made to pass as part of this test

rust-url/url/tests/expected_failures.txt

Lines 39 to 40 in da64903

	<non-spec:/.//p> set hostname to <h>
	<non-spec:/.//p> set hostname to <>

That's about mutating the host, not the path, but I think the behaviour is closely related to this one.

[valenting]

Sorry for the delayed review. See inline:

[theskim]

@valenting, should I be worried about this failure?

[valenting]

It seems the MSRV for libc changed
rust-lang/libc@77b3ea4#diff-06572a96a58dc510037d5efa622f9bec8519bc1beab13c9f251e97e657a9d4edR9

Feel free to also increase ours to 1.63

[valenting]

Note: I think we need to add #943 to this PR considering we backed it out in #999

[theskim]

Need to add tests to check scheme_end, username_end, host_start, etc.

[theskim]

@valenting, thoughts on this patch? As you can see, now we actually check the position of host, ref, etc.

[valenting]

Sorry for the delayed review.
I think there are still some improvements and simplifications that can be made.
Have a look at main...valenting:rust-url:pr-996 - I added a few extra tests too.

[valenting]

I don't know if this works for a URL we've already parsed.

For example, this test case fails:

#[test]
fn test_valid_indices_after_set_path2() {
    // Testing everything
    let mut url = Url::parse("moz:/").unwrap();
    assert_eq!(url.as_str(), "moz:/");
    assert!(!url.cannot_be_a_base());

    url.set_path("//p");
    assert_eq!(url.as_str(), "moz:/.//p");
    url.set_path("//d");
    assert_eq!(url.as_str(), "moz:/.//d");
}

I think the problem is that has_host is computed before we remove it again in line 1783

[valenting]

The test I suggested in my prev commit still fails:

#[test]
fn test_set_path_again() {
    // Testing everything
    let mut url = Url::parse("moz:/").unwrap();
    assert_eq!(url.as_str(), "moz:/");
    assert!(!url.cannot_be_a_base());

    url.set_path("//p");
    assert_eq!(url.as_str(), "moz:/.//p");
    url.set_path("//d");
    assert_eq!(url.as_str(), "moz:/.//d");
}

[valenting]

Suggested change

	if !has_host && segment.len() > 1 && path_empty {
	if !has_host && self.path().starts_with("//") {

I think the entire If can be simplified to this?