Skip to content

Feat: add simpler interface to install from Github repo with rv add owner/repo#414

Open
JosephBARBIERDARNAL wants to merge 6 commits intoA2-ai:mainfrom
JosephBARBIERDARNAL:install-pkg-github
Open

Feat: add simpler interface to install from Github repo with rv add owner/repo#414
JosephBARBIERDARNAL wants to merge 6 commits intoA2-ai:mainfrom
JosephBARBIERDARNAL:install-pkg-github

Conversation

@JosephBARBIERDARNAL
Copy link
Copy Markdown

@JosephBARBIERDARNAL JosephBARBIERDARNAL commented Feb 13, 2026

Hi! This PR tries to propose a solution for #404, to allow rv add user/project for packages on Github (or other sources).

I'm still trying to understand the codebase and this is a work a in progress.


Example usage

mkdir temp-pkg && cd temp-pkg

rv init
rv configure repository add CRAN --url https://cran.r-project.org/

rv add a2-ai/reportifyr
rv add r-lib/cli@v3.6.2

The config can also specify where to look for (instead of Github by default) thanks to git_shorthand_base_url:

[project]
name = "temp-pkg"
r_version = "4.5"
git_shorthand_base_url = "https://codeberg.org"

repositories = [
    { alias = "CRAN", url = "https://cran.r-project.org/" },
]

dependencies = [

]
  • Install a package from codeberg:
rv add R-packages/behaviorchange
[project]
name = "temp-pkg"
r_version = "4.5"
git_shorthand_base_url = "https://codeberg.org"

repositories = [
    { alias = "CRAN", url = "https://cran.r-project.org/" },
]

dependencies = [
    { name = "behaviorchange", git = "https://codeberg.org/R-packages/behaviorchange", reference = "HEAD" },
]

Known issues to fix

  • trying to install a repo while the url is wrong will still write the package dependency in rproject.toml, which will make rv sync to fail too
  • git_shorthand_base_url is kind of verbose, maybe git_base_url is better?
  • running rv add r-lib/cli@v3.6.2 after rv add r-lib/cli@v3.6.1 will not do anything. Intuitively I would expect this to bump it and install it, but I'm not sure if rv has an already existing way of bumping dependencies.

@JosephBARBIERDARNAL JosephBARBIERDARNAL marked this pull request as draft February 13, 2026 16:27
Comment thread src/git/local.rs
run_git(&["checkout", "-b", branch_name], &work_path);
// Some git installations default to `main`, others to `master`.
// Force checkout/reset to `main` so the test is stable across environments.
run_git(&["checkout", "-B", branch_name], &work_path);
Copy link
Copy Markdown
Author

@JosephBARBIERDARNAL JosephBARBIERDARNAL Feb 13, 2026

Choose a reason for hiding this comment

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

This change is just to avoid this error when testing:

thread 'git::local::tests::test_branch_update_after_checkout' panicked at src/git/local.rs:454:9:
git checkout -b main failed: fatal: a branch named 'main' already exists

Comment thread src/dependency_edit.rs
use crate::{Config, config::ConfigLoadError, git::url::GitUrl};

pub const DEFAULT_GIT_SHORTHAND_BASE_URL: &str = "https://github.com";
const DEFAULT_GIT_HEAD_REFERENCE: &str = "HEAD";
Copy link
Copy Markdown
Author

@JosephBARBIERDARNAL JosephBARBIERDARNAL Feb 13, 2026

Choose a reason for hiding this comment

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

I know from original issue (#404) that you want it to work with other options, but I think it makes sense to use Github as default since that's where most public packages live.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i think a github default makes a lot of sense, and as you're doing here just wanted to make sure we worked through providing that capability if someone (thank you!) started to give a shot at this :-)

@dpastoor
Copy link
Copy Markdown
Member

running rv add r-lib/cli@v3.6.2 after rv add r-lib/cli@v3.6.1 will not do anything. Intuitively I would expect this to bump it and install it, but I'm not sure if rv has an already existing way of bumping dependencies.

I think this is a gap for git packages. Eg when we first designed this it was having packages, and the concept of upgrading a package was really to upgrade the repository

❯ rv upgrade --help
Upgrade packages to the latest versions available

Usage: rv upgrade [OPTIONS]

Options:
      --dry-run
  -v, --verbose...                 Increase logging verbosity
  -q, --quiet...                   Decrease logging verbosity
      --json                       Output in JSON format. This will also ignore the --verbose flag and not log anything
  -c, --config-file <CONFIG_FILE>  Path to a config file other than rproject.toml in the current directory [default: rproject.toml]
  -h, --help                       Print help

so we'd either need to make add to add better logic that if the package isn't just a simple type, that it would first scan if the package is already present in any way and then replace.

Eg you could also imagine a development scenario of starting with a package from cran, then wanting to upgrade to a dev version. Should that all happen within add, or should add only add, and force users to go through explicit upgrade. I'm on the fence - we have preferred being explict, where eg rv add should maybe error and need an --overwrite flag.

Copy link
Copy Markdown
Member

@dpastoor dpastoor left a comment

Choose a reason for hiding this comment

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

i think this PR has a couple things going on with it - many of which I like. One is the addition of a general reference concept. I am ok with including this, however, my preference would be that even for the shorthand rv should attempt to resolve ref type at add-time rather than persisting a generic reference field. This avoids the broad fetch penalty and makes configs clearer. If we cannot fetch that could potentially be a fallback (though admittedly i don't like fallbacks as more fallbacks = more testing/risk of fallbacks not actually working if we don't test).

So the ideal situation to me would be that shorthand would fetch in some way sufficient info about the repository to identify what the default branch is. I honestly do not know how easy that is or feasible without significant workarounds. There seems to be some prior info about this https://superuser.com/questions/1718677/find-the-default-branch-name-for-a-git-repository

one thing we can do to help provide a supported place to test that, is next week @weswc can we throw up another git repo where it has a default branch of dev - we can use that as a test case where the shorthand for like pkg.x@a2-ai/git.pkg.x should properly resolve a ref of dev, compared to other test packages that resolve to main.

I don't think this is too far off accepting scope wise - though I'll also look to @Keats to take an additional look through the code too.

Very much appreciate you taking some time to put this together. Thanks!

Comment thread src/dependency_edit.rs Outdated

fn is_git_url(package_spec: &str) -> bool {
package_spec.starts_with("https://")
|| package_spec.starts_with("http://")
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

URL dependencies are not necessarily git urls

Comment thread src/lockfile.rs Outdated
|| directory != directory2
|| branch != branch2
|| tag != tag2
|| reference != reference2
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

can't a reference be a branch/tag?

Comment thread src/cli/commands/init.rs
Comment thread src/resolver/mod.rs Outdated
} else if let Some(t) = tag {
GitReference::Tag(t)
} else if let Some(r) = reference {
GitReference::Unknown(r)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

AFAIK this means that we never update the repo even if there are new commits? Since we skip fetching for GitReference::Unknown

@JosephBARBIERDARNAL JosephBARBIERDARNAL marked this pull request as ready for review February 16, 2026 19:57
@JosephBARBIERDARNAL JosephBARBIERDARNAL changed the title (draft) add simpler interface to install from Github repo with rv add user/project Feat: add simpler interface to install from Github repo with rv add owner/repo Feb 16, 2026
@JosephBARBIERDARNAL
Copy link
Copy Markdown
Author

Made a bunch of updates based on your feedback:

  • Added sanitized git-url detection so https://…/repo.git:subdir still counts as git
  • Stopped writing the generic reference field; instead resolve_add_options_reference_with_executor() now resolves shorthand refs (HEAD, branch, tag) via git ls-remote before add_packages runs. CLI wiring and the public API both call this, so configs always store an explicit branch/tag/commit.
git ls-remote --symref https://github.com/a2-ai/reportifyr HEAD
ref: refs/heads/main    HEAD
333bf2a5464295b619dc8852c4226c7f928761a1        HEAD

and then it parses to find main, in this case.

  • Exported the helper so the CLI and any API consumers share the same logic, and trimmed the redundant git-ref code out of main.rs

Copy link
Copy Markdown
Collaborator

@Keats Keats left a comment

Choose a reason for hiding this comment

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

Maybe let's scale it down a bit and just work on adding plain shorthands and not full URLs to rv add? You will need to do a sparse checkout like we do in the resolution to get the DESCRIPTION file to get the actual package name.

Comment thread src/main.rs
},
/// Add packages to the project and sync
Add {
/// Package names or git repo specs like owner/repo[@ref][:subdir] or https://...[@ref][:subdir]
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think we need to decide what we want here @weswc

Currently it's just the packages list but now we can mix and match. Should we allow remote http archive, local folders, all kind of git URLs? Eg if we want full git URLs, then we also need git@ etc.

We could keep it simple to start with and only have package names and git shorthands, that's it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we should leave it simple for now, only allowing package names and git shorthand. That seems to be where the most want is coming from and we have the flags if a user still wants to add the other kinds.

I also don't think we want all kind of git URLs. We should have an env var that users could set a non-github.qkg1.top default if they want, but I fall back to, if you need that level of tune-ability, use the more complex flag.

Comment thread src/dependency_edit.rs

Err(format!(
"Could not determine default branch for `{git_url}` from `git ls-remote` output."
))
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We already have that code in checkout_head, we should extract it instead of copying it

Comment thread src/dependency_edit.rs
Ok(full_url)
}

fn extract_package_name(source: &str) -> Result<String, String> {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

you can't assume package name == repo name, you need to at least do a sparse checkout the DESCRIPTION file to get it

Comment thread src/config.rs
format!("{base_url}owner/repo")
} else {
format!("{}/owner/repo", base_url.trim_end_matches('/'))
};
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

is that the same for gitlab/gitea?

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR implements the feature requested in issue #404, adding a simpler shorthand syntax rv add owner/repo[@ref][:subdir] for installing packages from GitHub (or other git hosting services configured via git_shorthand_base_url). The feature resolves git references by querying the remote via git ls-remote, then writes the resolved branch or tag into the config.

Changes:

  • New parse_add_package_spec function in dependency_edit.rs parses shorthand git repo specs, full HTTPS git URLs (.git suffix required), and SSH git URLs
  • New resolve_add_options_reference_with_executor resolves ambiguous refs (branch/tag/HEAD) by querying the remote via git ls-remote, called from both main.rs and inside add_packages
  • Config write in main.rs is now deferred until after a successful sync, fixing the known issue of writing config even when installation fails

Reviewed changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/dependency_edit.rs Core parsing and git reference resolution logic for shorthand specs
src/main.rs CLI integration: shorthand detection, option propagation, deferred config write
src/config.rs New git_shorthand_base_url field with validation; error message typo fix
src/lib.rs New public exports for parsing utilities and git executor
src/git/local.rs Test stability fix: use git checkout -B instead of -b
src/cli/commands/init.rs Adds commented-out git_shorthand_base_url field to init template
.gitignore Adds temp-pkg (developer test artifact, should be removed)
Comments suppressed due to low confidence (1)

src/config.rs:438

  • The original error message had a typo "ons and only one" which has been corrected to "one and only one" in this PR.
                    git,
                    tag,

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Comment thread src/dependency_edit.rs
Comment on lines +68 to +74
pub fn has_source_options(&self) -> bool {
self.repository.is_some()
|| self.force_source
|| self.git.is_some()
|| self.path.is_some()
|| self.url.is_some()
}
Copy link

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

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

The has_source_options() method includes force_source, which causes rv add owner/repo --force-source to skip shorthand git spec parsing and treat owner/repo as a literal package name instead.

The force_source flag means "build from source instead of binary" — it does not specify a package source (git/path/url/repository). A user who runs rv add r-lib/cli --force-source intends to install the GitHub package from source, but the current code bypasses the shorthand resolution and passes r-lib/cli as a literal package name to add_packages.

force_source should be removed from has_source_options(), so that shorthand resolution still runs even when --force-source is specified. After parse_add_package_spec returns, the force_source flag should be copied onto the parsed options (similar to how install_suggestions and dependencies_only are copied at lines 536–537).

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

seems legit

Comment thread src/main.rs

let mut options = parsed.options;
options.install_suggestions = add_options.install_suggestions;
options.dependencies_only = add_options.dependencies_only;
Copy link

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

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

When --force-source is passed alongside a shorthand spec like r-lib/cli, the options.force_source flag is not propagated to the parsed options like install_suggestions and dependencies_only are at lines 536–537. This means the --force-source flag would be silently ignored when using shorthand syntax.

Even if the has_source_options issue in has_source_options() is fixed, the force_source field also needs to be copied from add_options to options in this block.

Suggested change
options.dependencies_only = add_options.dependencies_only;
options.dependencies_only = add_options.dependencies_only;
options.force_source = add_options.force_source;

Copilot uses AI. Check for mistakes.
Comment thread src/dependency_edit.rs
Comment on lines +531 to +554
pub fn resolve_add_options_reference_with_executor(
options: &mut AddOptions,
git_exec: &impl CommandExecutor,
) -> Result<(), String> {
let Some(git_url) = options.git.clone() else {
return Ok(());
};

if options.commit.is_some() || options.tag.is_some() || options.branch.is_some() {
return Ok(());
}

let raw_ref = options
.reference
.take()
.unwrap_or_else(|| DEFAULT_GIT_HEAD_REFERENCE.to_string());

match resolve_git_reference(git_exec, git_url.as_str(), raw_ref.as_str())? {
ResolvedGitRef::Branch(branch) => options.branch = Some(branch),
ResolvedGitRef::Tag(tag) => options.tag = Some(tag),
}

Ok(())
}
Copy link

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

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

The resolve_add_options_reference_with_executor, resolve_git_reference, resolve_default_branch, and git_ls_remote_has_ref functions have no unit tests. The codebase already uses a mock CommandExecutor (FakeGit in src/resolver/mod.rs:765) pattern, so similar mock executor tests could be written here to verify the branch/tag resolution logic for different raw refs (HEAD, branch name, tag name, ambiguous, unresolvable).

Copilot uses AI. Check for mistakes.
Comment thread src/dependency_edit.rs
Comment on lines +145 to +150
match reference {
Some(ParsedReference::Commit(reference)) => options.commit = Some(reference),
Some(ParsedReference::Tag(reference)) => options.tag = Some(reference),
Some(ParsedReference::Branch(reference)) => options.branch = Some(reference),
Some(ParsedReference::Unknown(reference)) => options.reference = Some(reference),
None => options.reference = Some(DEFAULT_GIT_HEAD_REFERENCE.to_string()),
Copy link

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

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

The PR description shows the expected TOML output as { name = "behaviorchange", git = "...", reference = "HEAD" }, but the actual code resolves reference = "HEAD" to a concrete branch name via git ls-remote, and then writes branch = "main" (or the actual default branch). The TOML format written to rproject.toml will have branch = "<default-branch>", not reference = "HEAD". The PR description example output is inaccurate and may confuse users.

Copilot uses AI. Check for mistakes.
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.

5 participants