fix(services/hf): correct the refs/convert revision split

[PDGGK]

Which issue does this PR close?

None — filing directly, per CONTRIBUTING. I searched first: no open issue mentions the HF revision parsing, and the only two open PRs touching core/services/hf are the v0.58.2 release prep and #7801's build_abs_path rename, neither of which overlaps.

Rationale of this change

parse_revision recovers the revision from a URI by slicing:

if let Some(rest) = rev_and_path.strip_prefix("refs/convert/") {
    return if let Some(slash) = rest.find('/') {
        (
            rev_and_path[..14 + slash].to_string(),
            rest[slash + 1..].to_string(),
        )

"refs/convert/" is 13 bytes, not 14, so the / that terminates the revision segment is pulled into the revision:

datasets/squad@refs/convert/parquet/default/train/0000.parquet
  revision -> "refs/convert/parquet/"     (should be "refs/convert/parquet")
  path     -> "default/train/0000.parquet"

The trailing slash is copied into the operator config by HfConfig::from_uri and then percent-encoded by percent_encode_revision — which uses NON_ALPHANUMERIC and does not trim it — into paths_info_url, git_commit_url, resolve_url and file_tree_url, so requests go out with refs%2Fconvert%2Fparquet%2F. The HF API resolves revisions exactly, and that is not a valid ref.

The repo already agrees on the correct shape: test_hf_path_info_url_custom_endpoint pins the URL as .../paths-info/refs%2Fconvert%2Fparquet, with no trailing %2F.

Why it was not caught

resolve_refs_convert_revision only covers datasets/squad@refs/convert/parquet, which has no path after the revision. That takes the else arm and never reaches the slice, so the off-by-one was invisible.

What changed

The refs/pr/ arm ten lines below already avoids the whole class of bug by rebuilding with format!:

let revision = format!("refs/pr/{}", &rest[..slash]);

This adopts the same idiom for refs/convert/ rather than changing 14 to 13 — a corrected magic offset is still a magic offset.

Tests

New resolve_refs_convert_revision_with_path next to the existing case. On main:

test core::uri::tests::resolve_refs_convert_revision ............. ok
test core::uri::tests::resolve_refs_convert_revision_with_path ... FAILED

assertion `left == right` failed
  left: Some("refs/convert/parquet/")
 right: Some("refs/convert/parquet")

The existing test passing alongside it is the point — it shows the gap rather than the change moving any goalposts.

With this change: cargo test -p opendal-service-hf is 57 passed / 0 failed, and cargo fmt --check and cargo clippy --all-targets are clean.

Scope note

HfConfig::from_uri also discards the parsed path_in_repo — root comes only from opts.get("root") — so in this exact scenario the path is dropped regardless. That looked like a separate change and I have left it alone; happy to follow up if you would like it fixed.

This only affects URI-based construction (Operator::from_uri("hf://...")). Anyone calling HuggingfaceBuilder::revision("refs/convert/parquet") directly never reaches parse_revision.

[erickguan]

Good investigation!

minor comment so approve already.

[erickguan]

Suggested change

	if let Some(rest) = rev_and_path.strip_prefix("refs/convert/") {
	return if let Some(slash) = rest.find('/') {
	(
	rev_and_path[..14 + slash].to_string(),
	rest[slash + 1..].to_string(),
	)
	let revision = format!("refs/convert/{}", &rest[..slash]);
	(revision, rest[slash + 1..].to_string())
	} else {
	(rev_and_path.to_string(), String::new())
	};
	if let Some(rest) = rev_and_path.strip_prefix("refs/convert/") {
	return match rest.split_once('/') {
	Some((segment, path)) => (
	format!("refs/convert/{segment}"),
	path.to_string(),
	),
	None => (rev_and_path.to_string(), String::new()),
	};
	}

[PDGGK]

Thanks — applied in 35f23d3. split_once is clearly better here: the original bug was exactly the index arithmetic (14 + slash against the un-stripped string), and your version removes the arithmetic instead of just correcting it, so the same mistake can't come back.

One thing beyond your suggestion, easy to drop if you'd rather not have it: I applied the same shape to the adjacent refs/pr/ arm. That arm was already correct — it slices rest, not rev_and_path — but leaving the two neighbouring arms in two different idioms looked worse than the one-line diff of unifying them. It's a pure refactor:

• rest.find('/') → Some(slash) gives rest[..slash] / rest[slash + 1..]
• rest.split_once('/') → Some((segment, path)) gives the same two halves

Say the word and I'll drop that hunk.

57 unit tests pass, cargo fmt --check and cargo clippy --all-targets are clean.