feat!: give Init the same flags as Create, and add an ssh remote option - #5
feat!: give Init the same flags as Create, and add an ssh remote option#5paulo-granthon wants to merge 22 commits into
Conversation
…emote
Two changes to `:AgitateRepoInit`, both breaking.
Argument parsing now goes through `parse_args`, matching `Create`:
:AgitateRepoInit -r my-repo -u my-org
:AgitateRepoInit my-repo my-org
`Init` previously counted its arguments, treating one argument as the
repository name and two as username followed by repository name. It was
the only command left doing its own parsing, it had no flags, and it
silently ignored a third argument.
The positional order is now repository then username, the same as
`Create`. That is the reverse of what two positional arguments used to
mean, so `:AgitateRepoInit my-user my-repo` now reads `my-user` as the
repository name. Consistency between the two commands is worth more
than preserving the old order, and the failure mode is an `origin` URL
with the two halves swapped, which is visible immediately and fixed
with a single `git remote set-url`. Passing `-r` and `-u` explicitly
avoids the ambiguity entirely.
Adds `repo.init.remote_protocol`, defaulting to `https`. Setting it to
`ssh` produces `git@github.com:owner/repo.git`. Users with keys
configured previously had to rewrite the remote by hand after every
`Init`, which is exactly the friction this plugin exists to remove.
The value is validated, so a typo is reported rather than silently
falling back to https.
Unrecognised arguments are now reported, using the leftovers
`parse_args` returns.
There was a problem hiding this comment.
🟡 Changes recommended
Init currently rejects a nil remote_protocol despite it being documented/typed as optional, and its leftover-argument error path misreports missing flag values as “unrecognised.”
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates :AgitateRepoInit to use the same -r/-u argument parsing style as :AgitateRepoCreate, and adds a configuration option to control whether the origin remote is created as HTTPS or SSH.
Changes:
- Added
util.build_github_remote_urland tests for HTTPS/SSH remote URL construction. - Updated
core.repo.Initto parse-r/-u(including positional fill order) and to reject leftover/unmatched args. - Added
repo.init.remote_protocolto defaults, types, and README, and used it to choose theoriginremote format.
File summaries
| File | Description |
|---|---|
| tests/util_spec.lua | Adds coverage for build_github_remote_url (https default/explicit, ssh form, .git suffix). |
| README.md | Documents Init flag/positional order and the new repo.init.remote_protocol option. |
| lua/agitate/util.lua | Introduces build_github_remote_url helper for https/ssh remotes. |
| lua/agitate/types/config.lua | Extends init config type docs with remote_protocol. |
| lua/agitate/core/repo.lua | Switches Init to parse_args, validates protocol, uses remote URL builder, and errors on leftovers. |
| lua/agitate/config.lua | Adds default repo.init.remote_protocol = 'https'. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| local protocol = options.repo.init.remote_protocol | ||
|
|
||
| if protocol ~= 'https' and protocol ~= 'ssh' then | ||
| return agitate_error.throw('core.repo.Init -- Error: `repo.init.remote_protocol` expects `https` or `ssh`, got `' .. tostring(protocol) .. '`') | ||
| end |
| if #leftover > 0 then | ||
| return agitate_error.throw('core.repo.Init -- Error: unrecognised arguments: ' .. table.concat(leftover, ' ')) | ||
| end |
…ng value Two review findings. `repo.init.remote_protocol` is documented and typed as optional, but the validation rejected nil, so anything that produced an options table without it turned a legitimately absent setting into an error. An absent optional value means the default, not a mistake. Only a value that is present and wrong is refused now. `Init` reported a declared flag given without a value as an unrecognised argument, so `:AgitateRepoInit -r` said `-r` was unrecognised when the real problem was the missing value. It now uses the `incomplete` list that `parse_args` returns and names the flag that needs one, matching what `Create` already does.
There was a problem hiding this comment.
🟡 Changes recommended
Init still shells out with an unescaped repo name (command-injection/quoting risk) and accepts empty-string repo/user values, which can produce invalid commands/URLs.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Lite
| return agitate_error.throw('core.repo.Init -- Error: `repo.init.remote_protocol` expects `https` or `ssh`, got `' .. tostring(protocol) .. '`') | ||
| end | ||
|
|
||
| util.execute_command('echo "# ' .. github_repository_name .. '" >> README.md') |
| if not github_username or not github_repository_name then | ||
| return agitate_error.throw('core.repo.Init -- Error: undefined GitHub username or repository name') | ||
| end |
Two review findings. `Init` created the initial README by shelling out to `echo "# <name>" >> README.md` through `vim.fn.systemlist`, with the repository name interpolated unescaped. A name containing a quote or a shell metacharacter could break the command or run something else. Writes the file with `vim.fn.writefile` in append mode instead, which involves no shell at all. The username and repository checks tested only for nil, so `:AgitateRepoInit -r ""` passed validation and went on to build an invalid remote URL and an invalid git command. An empty string is a missing value here and is now refused with the same message.
There was a problem hiding this comment.
🟡 Changes recommended
Init still interpolates the configurable commit message into an Ex command without escaping, which can break the :G commit invocation and should be fixed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
| vim.cmd('G init') | ||
| vim.cmd('G add README.md') | ||
| vim.cmd('G commit -m "' .. options.repo.init.first_commit_message .. '"') | ||
| vim.cmd('G branch -M main') | ||
| vim.cmd('G remote add origin ' .. util.build_github_html_url(github_username, github_repository_name) .. '.git') | ||
| vim.cmd('G remote add origin ' .. util.build_github_remote_url(github_username, github_repository_name, protocol)) |
The review raised the commit message: it is user configured and was
interpolated inside double quotes in `vim.cmd('G commit -m "..."')`, so
a message containing a quote broke the command. The same file had
already been hardened against shell injection, which made this the
remaining soft spot.
It is not only the quote. `|` is an Ex command separator, so any of
these values containing one would have run the remainder as a second Ex
command, the same defect fixed in `core.branch`.
All six steps now run as argument vectors, which are parsed by neither
Ex nor a shell, and each is checked: `Init` previously ran the whole
sequence blind and reported success even when `git init` or the push
had failed.
There was a problem hiding this comment.
🟢 Approval recommended
The changes are cohesive and well-covered where behavior was added (remote URL construction), with only a minor type-annotation issue noted.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
lua/agitate/core/repo.lua:116
- The LuaLS annotation for
optional_parametersis too generic (table<string>). This argument is passed fromopts.fargsand consumed byparse_argsas an array of strings, so the type should bestring[]|nilto match actual usage and avoid misleading tooling.
---@param optional_parameters? table<string> Parameters can be passed in order or explicitly
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
There are a couple of small but concrete robustness/API issues in the new code paths (stored as PR comments) that should be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Lite
| function M.build_github_remote_url(username, repository_name, protocol) | ||
| if protocol == 'ssh' then | ||
| return 'git@github.com:' .. username .. '/' .. repository_name .. '.git' | ||
| end | ||
|
|
||
| return M.build_github_html_url(username, repository_name) .. '.git' | ||
| end |
| -- Written directly rather than shelled out to `echo ... >> README.md`. That | ||
| -- ran through a shell with the repository name interpolated unescaped, so a | ||
| -- name containing a quote or a metacharacter could run something else. | ||
| vim.fn.writefile({ '# ' .. github_repository_name }, 'README.md', 'a') | ||
| -- Run through git directly rather than `:G`. Every one of these |
Two review findings. `build_github_remote_url` returned the https form for any protocol that was not exactly `ssh`, so a typo produced a silently wrong remote. Only `core.repo.Init` validates before calling, so every other caller had no protection at all. Nil still means the default; anything unrecognised now raises rather than being quietly reinterpreted. `Init` ignored the result of `vim.fn.writefile`. A readonly directory or a permissions problem left README.md unwritten and the sequence carried on, so the failure surfaced two steps later as a confusing `git add` error. Checked and reported where it happens.
There was a problem hiding this comment.
🟢 Approval recommended
The changes are cohesive, documented, and include targeted tests for the new URL-building behavior while keeping the Init workflow’s error handling and validation straightforward.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The changes are internally consistent (flags/config/docs/tests aligned), add coverage for the new URL-building behavior, and include input/config validation for the new option.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
The step loop used `vim.fn.systemlist`, which captures stdout only. git puts almost all failure detail on stderr, so a failed `git init` or a rejected push named the command and then explained nothing, which is worse than the fugitive version it replaced: that at least showed git's output in a window. Uses `vim.system`, preferring stderr and falling back to stdout, so the message carries git's own reason. The curl path already redirects stderr for the same reason; this brings the git path in line.
There was a problem hiding this comment.
🟢 Approval recommended
The breaking argument-order change is clearly documented, the new protocol option is validated end-to-end, and the new URL behavior is covered by focused unit tests.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
core.repo.Init currently passes an optional config value directly into the git commit -m argv and also defaults remote_protocol in a way that can silently accept invalid config (false) instead of validating it.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Lite
| local steps = { | ||
| { 'init' }, | ||
| { 'add', 'README.md' }, | ||
| { 'commit', '-m', options.repo.init.first_commit_message }, |
| local protocol = options.repo.init.remote_protocol or 'https' | ||
|
|
||
| if protocol ~= 'https' and protocol ~= 'ssh' then | ||
| return agitate_error.throw('core.repo.Init -- Error: `repo.init.remote_protocol` expects `https` or `ssh`, got `' .. tostring(protocol) .. '`') | ||
| end |
Two review findings.
`first_commit_message` is optional in the config type and was passed
straight into the argv. A nil produced `{'commit', '-m'}`, so git
received a `-m` with no argument and answered with a usage error that
says nothing about Agitate's configuration. Defaults to the documented
value, and treats an empty string the same way.
`remote_protocol` used `or 'https'`, which also swallows `false`. Only
an absent value should mean the default; `false` is a misconfiguration
and the validation immediately below exists to report it.
f708a14 to
0363e8e
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
The new vim.system dependency in Init needs an explicit compatibility guard (or documented minimum Neovim version), and the README’s RepoCreate overview is now misleading relative to the actual Create flags.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
lua/agitate/core/repo.lua:216
:AgitateRepoInitnow relies onvim.system, which is only available in newer Neovim versions; without an explicit guard this will hard-error on older installs. Add a version/API check and fail with a clear Agitate error message (or document/bump the minimum supported Neovim version).
-- `vim.system` rather than `systemlist`, which captures stdout only. git
-- reports almost every failure on stderr, so a failed step would otherwise
-- name the command and explain nothing.
local completed = vim.system(command, { text = true }):wait()
README.md:36
- The README “quick overview” still describes
:AgitateRepoCreateas accepting only an optional repository name, butcore.repo.Createactually supports-r,-u, and-v(and positional fill order). Since this section is being updated for Init/Create parity, updating the Create entry too would avoid misleading users.
- `:AgitateRepoInit` - Initializes the current directory as a GitHub
repository.
Takes `-r` for the repository name and `-u` for the username or organization,
in that order if you pass them positionally. Defaults to the current directory
name and the configured username.
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
Conflicted on the `Init` docstring: this branch rewrote it for the flag based arguments, while fixes-general corrected the `table<string>` annotation to `string[]`. Keeps this branch's wording with the corrected annotation, rather than either side wholesale.
The step loop built its own git invocation, which inherited the process working directory. Fugitive resolved the repository from the buffer, so with `:cd` pointing elsewhere `Init` initialised and pushed the wrong directory. The branch commands had the same defect and were reported separately; there is now one runner instead of copies drifting apart. `util.git` merges stderr, which the local copy already did, so the failure detail is unchanged.
70c2916 to
c944023
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Init runs git commands in the current buffer’s directory via util.git, which can diverge from the “current directory” behavior and break initialization when the buffer is in a subdirectory.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
| -- Through `util.git`, which runs in the buffer's repository rather than the | ||
| -- process working directory and merges stderr, where git puts its reasons. | ||
| for _, step in ipairs(steps) do | ||
| local output, step_ok = util.git(step) | ||
|
|
||
| if not step_ok then | ||
| return agitate_error.throw('core.repo.Init -- Error: `git ' .. table.concat(step, ' ') .. '` failed.\n' .. table.concat(output, '\n')) | ||
| end | ||
| end |
`Init` writes `README.md` relative to `getcwd()` and, after moving onto `util.git`, ran its git steps in the *buffer's* directory. With the current buffer anywhere other than the working directory, the README was created in one place and `git init`, `git add README.md` and the push ran in another, so the add failed on a file that was not there. A defect I introduced by adopting the shared runner without noticing the two commands disagree about which directory they mean. `Init` now names the working directory explicitly, which matches both its documentation and where it writes.
There was a problem hiding this comment.
🟢 Approval recommended
The changes are cohesive, validated in code, and include targeted tests for the new remote URL behavior without introducing correctness issues in the updated init flow.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
Init now depends on vim.system via util.git and can hard-error on Neovim versions without it unless guarded with a clear error or fallback.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
| for _, step in ipairs(steps) do | ||
| local output, step_ok = util.git(step, directory) | ||
|
|
||
| if not step_ok then | ||
| return agitate_error.throw('core.repo.Init -- Error: `git ' .. table.concat(step, ' ') .. '` failed.\n' .. table.concat(output, '\n')) | ||
| end | ||
| end |
Two changes to
:AgitateRepoInit, both breaking.Flag parity with Create
Initwas the last command doing its own argument handling: it counted arguments, treating one as the repository name and two as username-then-repository. It had no flags and silently ignored a third argument.Positional order is now repository, then username, matching
Create. That is the reverse of what two positional arguments used to mean.I took consistency over preserving the old order because the two commands sitting side by side in the README with opposite argument orders is a trap that never stops costing. The failure mode here is mild and immediate: an
originURL with the halves swapped, visible in the push that follows, fixed with onegit remote set-url. Nothing is created remotely by this command.Passing
-rand-uexplicitly sidesteps it entirely, and the README now documents the order.If you would rather keep the old order, say so and I will flip it; the change is one line and the flags stay either way.
repo.init.remote_protocolsshproducesgit@github.com:owner/repo.git. Users with keys configured previously had to rewrite the remote by hand after everyInit, which is the exact friction the plugin exists to remove.The value is validated, so a typo is reported rather than silently falling back to https.
Also
Unrecognised arguments are reported, using the leftovers
parse_argsnow returns.Tests
Four new cases on
build_github_remote_url: https default, explicit https, the scp-style ssh form, and that both forms end in.git.The
Initbody itself, seven fugitive commands in sequence, is not tested. Doing so means driving a realgit initand push in a scratch repository, which is a harness change rather than a feature change. What is tested is everything that decides the arguments those commands receive.