Skip to content

Support dest and gitflags in forge repo clone#101

Merged
andrew merged 2 commits into
git-pkgs:mainfrom
untitaker:repo-clone-args
May 25, 2026
Merged

Support dest and gitflags in forge repo clone#101
andrew merged 2 commits into
git-pkgs:mainfrom
untitaker:repo-clone-args

Conversation

@untitaker
Copy link
Copy Markdown
Contributor

Fix #98

Copy link
Copy Markdown
Contributor

@andrew andrew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this is the right shape and the -- guard on the server URL is preserved.

One thing before merging: the arg parsing is too permissive. forge repo clone o/r a b c silently uses a as the destination and drops b c, and forge repo clone o/r a b -- --bare drops b the same way. gh errors on extra positionals here and I think we should too. Relatedly, forge repo clone -- --depth 1 has dashIdx == 0 so args[0] (--depth) ends up as both repoArg and the first git flag, and the resulting resolve.Repo error is confusing.

Something like this after computing dashIdx would cover both:

positional := len(args)
if dashIdx != -1 {
    positional = dashIdx
}
if positional < 1 {
    return errors.New("repository argument required")
}
if positional > 2 {
    return fmt.Errorf("accepts at most 2 arg(s) before --, received %d", positional)
}

Then the two branches collapse to repoArg = args[0], if positional > 1 { dest = args[1] }, if dashIdx != -1 { gitFlags = args[dashIdx:] }.

Everything else looks good, the new gitCloneArgs test cases are exactly what I'd want.

@andrew
Copy link
Copy Markdown
Contributor

andrew commented May 25, 2026

Thanks

@andrew andrew merged commit 8eed915 into git-pkgs:main May 25, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

support forge repo clone URL PATH

2 participants