Handle trailing slashes when parsing a repo url in init by Muhtasim-Munif-Fahim · Pull Request #715 · DagsHub/client

Muhtasim-Munif-Fahim · 2026-08-13T06:54:18Z

Fixes #701.

The bug

init(url=...) takes the last two segments of the url without normalizing it first:

if url.endswith(".git"):
    url = url[:-4]
parts = url.split("/")
repo_owner, repo_name = parts[-2], parts[-1]

A url copied from the browser address bar carries a trailing slash, which shifts every segment by one:

url	`repo_owner`	`repo_name`
`https://dagshub.com/owner/repo`	`owner`	`repo`
`https://dagshub.com/owner/repo/`	`repo`	`""`
`https://dagshub.com/owner/repo.git`	`owner`	`repo`
`https://dagshub.com/owner/repo.git/`	`repo.git`	`""`

The .git case is worse because the suffix is only stripped when it sits at the very end of the string, so a trailing slash defeats that too.

Nothing catches the empty value afterwards — RepoAPI("repo/") is constructed and, on the not-found branch, create_repo("", ...) is called.

The fix

Strip trailing slashes before the .git suffix is removed, and fail loudly when the url carries no owner/name pair rather than continuing with empty segments:

url = url.rstrip("/")
if url.endswith(".git"):
    url = url[:-4]
parts = url.rstrip("/").split("/")
repo_owner, repo_name = parts[-2] if len(parts) > 1 else "", parts[-1]
if not repo_owner or not repo_name:
    raise ValueError(...)

All four rows above now yield ("owner", "repo"), as do repeated slashes.

I used ValueError for the malformed-url case. init already raises AttributeError for the mismatched-args case just above, so say the word if you would rather these were consistent.

Tests

Extends tests/common/test_init.py using the fixtures already there:

test_init_from_url_tolerates_trailing_slash, parametrized over the trailing slash, the .git + slash combination, and repeated slashes, asserting RepoAPI and create_repo receive my-org / my-repo;
test_init_from_url_without_owner_and_name_raises, parametrized over urls with no owner/name pair.

All six fail without the change and pass with it. tests/common plus tests/test_misc.py is 54 passed.

tests/common/test_determine_repo.py fails to collect on my machine for want of pytest_git; that is unrelated to this change and reproduces on a clean checkout. I also left the surrounding formatting alone — black --line-length 120 wants to reformat parts of test_init.py that predate this PR, so I kept the diff to the new cases.

🤖 Generated with Claude Code

`init(url=...)` split the url on "/" and took the last two segments without normalizing it first. A url copied from the browser address bar carries a trailing slash, which shifted every segment by one: "https://dagshub.com/owner/repo/" -> repo_owner="repo", repo_name="" `RepoAPI("repo/")` and `create_repo("", ...)` were then called with those empty values instead of failing. A ".git" suffix combined with a trailing slash was worse still, yielding repo_owner="repo.git", because the suffix was only stripped when it sat at the very end of the string. Strips trailing slashes before the ".git" suffix is removed, and raises ValueError when the url carries no owner/name pair rather than continuing with empty segments. Adds cases to tests/common/test_init.py for the trailing slash, the ".git" + slash combination, repeated slashes, and the urls that should now raise. All six fail without this change. Fixes DagsHub#701 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

coderabbitai · 2026-08-13T06:54:35Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 42bd9944-42cf-4a82-941f-cd099ee7eea2

📥 Commits

Reviewing files that changed from the base of the PR and between e8af285 and 6cbd0cf.

📒 Files selected for processing (2)

dagshub/common/init.py
tests/common/test_init.py

Included review availability: Your plan includes up to 2 reviews per rolling hour; 1 remains after this review.

📜 Recent review details

🔇 Additional comments (2)

dagshub/common/init.py (1)

135-138: Remove the .git suffix from the parsed path.

For a URL such as https://dagshub.com/my-org/my-repo.git?tab=files, Line 136 does not remove .git. _parse_repo_url then uses my-repo.git as the repository name after it drops the query.

Normalize the parsed path before extracting the repository name, or reject query and fragment components.

tests/common/test_init.py (1)

1-1: LGTM!

Also applies to: 133-183

📝 Walkthrough

Walkthrough

dagshub.init now sanitizes repository URLs, extracts owner and repository segments, and raises ValueError for invalid inputs. Tests cover trailing slashes, .git suffixes, repeated slashes, missing segments, and credential removal.

Changes

Repository URL validation

Layer / File(s)	Summary
Normalize and validate repository URLs `dagshub/common/init.py`, `tests/common/test_init.py`	`dagshub.init` normalizes URL paths, removes credentials and query data, validates owner and repository segments, and uses the sanitized URL for tracking. Tests cover valid forms, invalid inputs, tracking URIs, and redacted errors. The organization repository log call was reformatted without behavior changes.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: 🔵 Low · up to 6cbd0

The change correctly handles trailing slashes but still leaves a bounded URL-parsing gap for .git URLs with query parameters, which can target the wrong repository name. The PR is otherwise localized and mergeable with explicit owner follow-up to normalize or reject query and fragment components.

Possibly related PRs

DagsHub/client#709: Both changes update repository URL parsing and related tests.

Poem

A rabbit trims each trailing slash,
And keeps the URL clean.
Owner and name now parse as planned,
With no credentials seen.
Invalid paths raise safely—
Tests hop across the scene.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the primary change: handling trailing slashes during repository URL parsing in init.
Description check	✅ Passed	The description directly explains the URL parsing bug, the fix, validation behavior, tests, and related implementation details.
Linked Issues check	✅ Passed	The changes satisfy issue `#701` by normalizing slash variants, extracting valid owner and repository segments, and rejecting malformed URLs.
Out of Scope Changes check	✅ Passed	The changes remain focused on repository URL parsing, canonical URL handling, validation, credential removal, and related tests.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai

Actionable comments posted: 1

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: a39d5eef-886e-47d4-8cb2-308d103f70a4

📥 Commits

Reviewing files that changed from the base of the PR and between 733227f and 0678d9d.

📒 Files selected for processing (2)

dagshub/common/init.py
tests/common/test_init.py

📜 Review details

🔇 Additional comments (2)

dagshub/common/init.py (1)

104-104: LGTM!

tests/common/test_init.py (1)

87-109: LGTM!

Review caught a real hole in the previous commit. Taking the last two slash-separated pieces of the whole url lets the hostname stand in for the owner when the path has only one segment: https://dagshub.com/my-repo -> ("dagshub.com", "my-repo") That is malformed input the code was supposed to reject, and it sailed through to RepoAPI as a plausible-looking owner/name pair. Owner and name are now taken from the url *path* via urlparse, and a path with fewer than two non-empty segments raises. With a scheme present the hostname lands in `netloc` and is never a candidate; without one, urlparse puts everything in `path`, so a bare "owner/repo" still works. Extends the raising cases with the reported url and its trailing-slash form. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Muhtasim-Munif-Fahim · 2026-08-15T05:13:52Z

Good catch, and it was a real hole — thanks.

https://dagshub.com/my-repo has one path segment, but the last two slash-separated pieces of the whole string are dagshub.com and my-repo, so my check passed it through as a valid owner/name pair rather than raising.

Fixed in e8af285 by parsing the url path with urlparse and requiring two non-empty segments. With a scheme present the hostname lands in netloc and can never be a candidate; without one, urlparse puts everything in path, so a bare owner/repo still works.

Added the reported url and its trailing-slash form to the raising cases. Worth noting the old behaviour made the test suite hang rather than fail — the malformed url reached the API instead of raising, which is exactly the failure mode this was meant to prevent.

coderabbitai

Actionable comments posted: 2

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 3298e8c5-e165-4bef-8e9b-f0f5f667c031

📥 Commits

Reviewing files that changed from the base of the PR and between 0678d9d and e8af285.

📒 Files selected for processing (2)

dagshub/common/init.py
tests/common/test_init.py

🚧 Files skipped from review as they are similar to previous changes (1)

tests/common/test_init.py

📜 Review details

🔇 Additional comments (1)

dagshub/common/init.py (1)

123-123: LGTM!

Two follow-ups from review, both on the url that `init` derives. The parse skips empty path segments when reading owner and name, but the url itself was passed through untouched, so the two could disagree: https://dagshub.com/my-org//my-repo -> RepoAPI("my-org/my-repo") -> MLFLOW_TRACKING_URI = "https://dagshub.com/my-org//my-repo.mlflow" The url is now rebuilt from the same segments the owner and name come from, so whatever reaches MLflow and DVC agrees with what reached the API. Rebuilding also drops any userinfo. That matters beyond tidiness: `url` is written into .dvc/config, which is a committed file, so a token pasted into the url as "https://user:token@dagshub.com/owner/repo" would have been committed to the repository. MLflow and DVC are both handed credentials separately from the token, so nothing depended on the userinfo being carried. The same redaction applies to the "could not determine the repo owner and name" error, which quotes the url back and is likely to end up in a log. Four of the five new cases fail without this change; the `.git` one passes already and is there as a guard. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Muhtasim-Munif-Fahim · 2026-08-17T06:16:32Z

Pushed 6cbd0cf for both findings.

The url is now rebuilt from the same segments the owner and name are read from, so the two cannot disagree. Previously https://dagshub.com/my-org//my-repo reached RepoAPI as my-org/my-repo while MLflow was pointed at .../my-org//my-repo.mlflow.

One thing worth flagging beyond the finding as written: rebuilding also drops any userinfo, and that closes a real leak rather than just tidying the string. url is written into .dvc/config, which is a committed file — so a token pasted into the url as https://user:token@dagshub.com/owner/repo was being committed to the repository. MLflow and DVC are each handed credentials separately from the token, so nothing depended on the userinfo being carried through. The same redaction now applies to the "could not determine the repo owner and name" error, which quotes the url back and is likely to end up in a log.

Five new cases cover this; four of them fail without the change, and the .git one passes already and is there as a guard. 104 tests pass in tests/common, and flake8 is clean under the arguments the lint workflow uses.

coderabbitai Bot requested changes Aug 13, 2026

View reviewed changes

Comment thread dagshub/common/init.py Outdated

coderabbitai Bot requested changes Aug 15, 2026

View reviewed changes

Comment thread dagshub/common/init.py Outdated

Comment thread dagshub/common/init.py

coderabbitai Bot approved these changes Aug 17, 2026

View reviewed changes

Navigation Menu

Uh oh!

Handle trailing slashes when parsing a repo url in init - #715

Handle trailing slashes when parsing a repo url in init#715
Muhtasim-Munif-Fahim wants to merge 3 commits into
DagsHub:mainfrom
Muhtasim-Munif-Fahim:fix/init-url-trailing-slash

Muhtasim-Munif-Fahim commented Aug 13, 2026

Uh oh!

coderabbitai Bot commented Aug 13, 2026 •

edited

Loading

Walkthrough

Changes

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Muhtasim-Munif-Fahim commented Aug 15, 2026

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Muhtasim-Munif-Fahim commented Aug 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

Muhtasim-Munif-Fahim commented Aug 13, 2026

The bug

The fix

Tests

Uh oh!

coderabbitai Bot commented Aug 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Muhtasim-Munif-Fahim commented Aug 15, 2026

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Muhtasim-Munif-Fahim commented Aug 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

coderabbitai Bot commented Aug 13, 2026 •

edited

Loading