Handle repositories whose default branch is not main - #1
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the system's ability to interact with Git repositories by removing the rigid assumption of 'main' as the default branch. It introduces dynamic detection of the actual default branch, improving compatibility with a wider range of Git hosting configurations. The changes ensure that repository cloning and source URL generation accurately reflect the remote's configuration, leading to more robust and flexible operations. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request refactors the Git cloning logic to dynamically detect the default branch of a remote repository instead of hardcoding 'main'. This involves extracting URL normalization and reference resolution into new functions (normalizeRepoURL, resolveCloneRef, detectDefaultBranch) in git.go, and modifying commands.go and source.go to no longer assume 'main' as the default branch. A new test file git_test.go was added to verify the default branch detection. The primary review comment highlights a bug where the resolveCloneRef function swallows errors from detectDefaultBranch, potentially leading to less informative error messages and making subsequent error checks redundant. The reviewer suggests propagating these errors while still allowing git clone to proceed if the default branch is simply not found.
| ref, err := detectDefaultBranch(ctx, repoURL) | ||
| if err == nil && ref != "" { | ||
| return ref, nil | ||
| } | ||
|
|
||
| args := []string{"clone", "--depth", "1", "--branch", ref, repoURL, tmp} | ||
| cmd := exec.CommandContext(ctx, "git", args...) | ||
| return "", nil |
There was a problem hiding this comment.
This logic swallows any error returned by detectDefaultBranch. If git ls-remote fails due to a network issue or an invalid repository URL, the specific error is lost, and git clone will fail later with a potentially less informative message. This also makes the error check for resolveCloneRef in cloneRepo (line 28) effectively dead code.
You should propagate errors from detectDefaultBranch, while still treating the "default branch not found" case as a non-error, since git clone can handle that by using the remote's default. Using string comparison on the error message is brittle; a more robust long-term solution would be for detectDefaultBranch to return a sentinel error.
| ref, err := detectDefaultBranch(ctx, repoURL) | |
| if err == nil && ref != "" { | |
| return ref, nil | |
| } | |
| args := []string{"clone", "--depth", "1", "--branch", ref, repoURL, tmp} | |
| cmd := exec.CommandContext(ctx, "git", args...) | |
| return "", nil | |
| ref, err := detectDefaultBranch(ctx, repoURL) | |
| if err != nil { | |
| // "default branch not found" is not a fatal error; git clone can proceed without a branch. | |
| // However, other errors (e.g., network, git not found) should be propagated. | |
| if err.Error() == "default branch not found" { | |
| return "", nil | |
| } | |
| return "", err | |
| } | |
| return ref, nil |
There was a problem hiding this comment.
Fixed: I introduced a sentinel error for the "default branch not found" case and now propagate other detectDefaultBranch failures instead of swallowing them.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| srcURL := "https://github.com/" + owner + "/" + repo | ||
| if ref != "" { | ||
| srcURL += "/tree/" + ref | ||
| } | ||
|
|
||
| srcURL := "https://github.com/" + owner + "/" + repo + "/tree/" + ref + "/" + strings.TrimPrefix(u.Entry.SkillPath, "/") | ||
| srcURL += "/" + strings.TrimPrefix(u.Entry.SkillPath, "/") |
There was a problem hiding this comment.
🟡 Update constructs invalid GitHub URL (losing skill subpath) when resolveCloneRef returns empty ref
When resolveCloneRef returns an empty ref (which occurs when detectDefaultBranch at git.go:85-106 returns ErrDefaultBranchNotFound—e.g., on git servers that don't support --symref), the URL is built as https://github.com/owner/repo/skills/my-skill instead of including /tree/<branch>/. When this URL is re-parsed by ParseSource at source.go:54-62, the githubTreeRe regex won't match (no /tree/ segment), so the fallback path splits on / and returns only Owner and Repo—the skill subpath (u.Entry.SkillPath) is silently lost. This causes Add to clone the repo and scan from the root instead of the correct subdirectory, potentially installing wrong skills or failing to find the expected skill.
URL construction path when ref is empty vs non-empty
When ref is non-empty (normal case), URL is:
https://github.com/owner/repo/tree/main/skills/my-skill → ParseSource extracts Subpath correctly.
When ref is empty (ErrDefaultBranchNotFound), URL is:
https://github.com/owner/repo/skills/my-skill → ParseSource returns Owner="owner", Repo="repo", Subpath="" (lost).
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Fixed: Update() now resolves the repo default branch when no ref is stored, so it always reconstructs a valid /tree/<branch>/... source URL and preserves the skill subpath.
cloneRepo no longer calls resolveCloneRef/detectDefaultBranch. When no explicit ref is provided, git clone is invoked without --branch, letting git itself pick the remote's default branch. This avoids an unnecessary network round-trip on every install. resolveCloneRef is still used by Update() where the branch name is needed to construct source URLs.
1 participant
Summary
Testing