fix: correctness pass over error handling, argument parsing and repo commands - #2
Open
paulo-granthon wants to merge 32 commits into
Open
fix: correctness pass over error handling, argument parsing and repo commands#2paulo-granthon wants to merge 32 commits into
paulo-granthon wants to merge 32 commits into
Conversation
… `CreateGitHubCurl` function of `core.repo`
BREAKING CHANGE: The `CreateGitHubCurl` function was renamed to `Create` in the `core.repo` and consequently in the `api.repo` module, this breaks current defined keybindings. You should now use `:AgitateRepoCreate` instead of `:AgitateRepoCreateGitHubCurl` for keybindings.
BREAKING CHANGE: The `InitGitHub` function was renamed to `Init` in the `core.repo` and consequently in the `api.repo` module, this breaks current defined keybindings. You should now use `:AgitateRepoInit` instead of `:AgitateRepoInitGitHub` for keybindings.
… function in `util`
`throw` had three defects that combined into an infinite loop. The `type(error) == 'table'` branch iterated the value but never returned, so execution fell through to the checks below it. Those checks then saw a non string value and called `M.throw(M.unhandled(...))`, but `unhandled` returns a table, so the same path ran again with another table, forever. Any error that was not already a plain string took the plugin down with a stack overflow rather than reporting anything. The third defect was that `AgitateError.message` was never read, so even a well formed error object produced no useful output. Splits message resolution into `describe`, which turns a string, an `AgitateError`, or a list of either into a printable message and returns `nil` when there is genuinely nothing to report. `throw` then substitutes the unhandled message itself instead of calling back into `throw`, which is what removes the loop. Reports through `vim.notify` at ERROR level rather than the deprecated `nvim_err_writeln`. Adds `tests/error_spec.lua`, including a regression test that a messageless table reports once and returns.
Every module opened with `local ok, error = pcall(require,
'agitate.error')`, which bound the error module to the name `error` and
shadowed Lua's builtin of the same name for the rest of the file.
`core.repo` then called `error('...')` in two places, intending the
builtin. Because the name resolved to the module table, both calls
raised `attempt to call a table value`. The two paths affected are the
ones that check for a missing GitHub username or access token, so the
plugin hard crashed on exactly the misconfiguration those checks exist
to report.
Renames the module local to `agitate_error` across all seven modules
that import it, which un-shadows the builtin, and routes both call
sites through `agitate_error.throw` so the user gets the intended
message.
The failure branch reported the error but did not return, so execution fell straight through into the checks that index `github_post_response.errors` and `github_post_response.html_url`. On a transport failure that response is an `AgitateError`, not a GitHub payload, so the user saw the real error followed by a second, bogus `html_url not found` report describing a request that never completed. Returns from the failure branch so only the real error is reported.
`post_new_repo` built its request body by concatenation, and the
closing fragment still carried the quote that used to terminate the
`name` string:
-d '{"name":"myrepo","private":true"}'
The stray quote after the boolean makes the body invalid JSON, so
GitHub rejects the request and no repository is created.
The bug arrived in f212921, which added `"private":<bool>` between the
repository name and the closing fragment but left `[["}']]` in place.
Before that commit the body ended right after the name and the quote
was correct, so `:AgitateRepoCreate` has been sending a malformed body
for every call since.
Drops the stray quote from the closing fragment.
…ond value `core.repo.Create` already destructured two values from `parse_args`, but the function only ever returned one, so the second was always nil. Arguments that matched no declared flag were dropped without a trace: a user who mistyped a flag, or passed more values than there are flags, got silence rather than a hint that part of their input was ignored. Returns a `leftover` list as the second value. Callers can now report unrecognised input instead of guessing. Two related behaviours are also corrected: - An undeclared flag such as `-z value` was written straight into the result table, so an unknown flag was silently accepted as if it were real. Only declared flags are honoured now, and an unknown one goes to the leftovers. - A positional argument that ran out of free flags was dropped inside an inner loop that fell off its end. It now lands in the leftovers. Also drops the trailing no-op loop that assigned `nil` over keys that were already `nil`, and rewrites the positional pass so the flag cursor is not re-tested against flags it has already filled. Expands the spec from one case to eight, covering explicit pairs winning over positional order, leftovers, undeclared flags, a declared flag with no value, and a nil argument list.
Two problems in `util`. `json_lr_trim` is annotated as returning a boolean and an optional string, and callers in `service.github` destructure both, but the failure path returned a single value. Returns an explicit `nil` second value so the contract holds on every path. `flatten_table` named its first parameter `table`, shadowing the Lua standard library table for the whole function body, which is why it accumulated its result with repeated string concatenation rather than using `table.concat`. That is quadratic in the number of lines, and it also prefixed the result with a stray leading space, which then showed up in every error message built from a flattened response. Renames the parameter to `lines`, builds the result with `table.concat`, and drops the leading space. The skip option now selects the starting index directly instead of counting down inside the loop. Adds `tests/util_spec.lua` covering both functions and `build_github_html_url`.
`Init` built its fugitive command as:
G remote add originhttps://github.com/user/repo.git
so the remote was never added and the `G push -u origin main` that
follows had no origin to push to. The whole point of the command,
wiring the local repository to its GitHub remote, did not happen.
The space was lost in b1bb2f2, which replaced the literal
`'G remote add origin https://github.com/'` with a call to
`build_github_html_url`. The space had been part of that literal, and
nothing reintroduced it alongside the helper.
Adds the separator back.
`nvim_err_writeln` is deprecated as of Neovim 0.11. Replaces all eleven call sites with `vim.notify` at an explicit level, which also means Agitate messages now flow through whatever notification plugin the user has installed rather than always going to the message area. Informational output moves from bare `print` to `vim.notify` at INFO, so a user can filter or route it like any other message. Two error paths in `core.repo.Create` were building their own message and calling the writer directly. They now go through `agitate_error.throw`, so every Agitate error has one shape and one level. Fixes the reporting in those paths while moving them: - The `html_url` missing branch passed the decoded response to `util.flatten_table`, which counts list elements. A decoded JSON object has none, so the length is zero and the branch always reported `Full response:` followed by nothing. Uses `vim.inspect`, which renders the object. - The GitHub errors branch indexed `errors[1].message` unguarded and would itself error if the array came back empty. Falls back to inspecting the array. `core.branch.CreateCheckoutAndPush` reported a missing branch name with `print`, which is not an error channel. It now notifies at ERROR. Breaking for anyone capturing Agitate output by redirecting messages, since notifications are no longer written to the message area directly.
The `Project functions` entry read `todo!()` with no indication that it is not simply unimplemented but currently unreachable by the same means as everything else on the list. GitHub sunset the classic Projects REST API, and Projects V2 is exposed only through GraphQL. Supporting it means carrying a second API paradigm next to the REST client and asking users for a token with the `project` scope, which is a larger decision than the remaining REST backed items on the list. Notes the constraint so the reason survives, rather than deleting the entry or leaving it looking like an ordinary unstarted task.
`vim` is declared as a read global so that plugin code cannot assign to it by accident. Specs legitimately need to, though: capturing what a function reports means swapping `vim.notify` for a recorder and restoring it afterwards. luacheck flagged that as `setting read-only field notify of global vim`. Declares `vim` as a writable global for `tests/` only. Plugin code keeps the stricter treatment.
This was referenced Aug 25, 2026
There was a problem hiding this comment.
🟡 Changes recommended
Several modules currently “return after notify/throw” on dependency-load failures, which can make require(...) succeed with a nil module value and cause follow-on nil-index crashes rather than being caught by pcall(require, ...).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR focuses on fixing correctness issues across Agitate’s error handling, argument parsing, and GitHub repository workflows, while also completing a command rename/migration and expanding the test suite to cover the fixed behaviors.
Changes:
- Reworked error handling (
agitate.error) to avoid recursion and to standardize user-facing reporting viavim.notify. - Hardened
parse_argsto support explicit + positional flags and to return leftovers instead of silently dropping unmatched args. - Fixed GitHub repo creation/init behavior (JSON body correctness, remote add spacing, URL formatting) and updated command names/docs accordingly.
File summaries
| File | Description |
|---|---|
| tests/util_spec.lua | Adds coverage for flatten_table, json_lr_trim, and build_github_html_url. |
| tests/parse_args_spec.lua | Expands parse_args coverage for positional fill, explicit precedence, and leftovers. |
| tests/error_spec.lua | Adds regression coverage for error recursion + notification behavior. |
| README.md | Updates command names and documents Projects feature blockage rationale. |
| lua/agitate/util.lua | Fixes flatten_table, json_lr_trim contract, and GitHub URL construction. |
| lua/agitate/types/config.lua | Updates config type docs to renamed repo-init command. |
| lua/agitate/service/github.lua | Fixes repo-create JSON payload and migrates error output to vim.notify. |
| lua/agitate/parse_args.lua | Reworks parsing to return (parsed, leftover) and ignore undeclared flags. |
| lua/agitate/init.lua | Migrates import failure reporting to vim.notify and uses renamed error var. |
| lua/agitate/error.lua | Replaces recursive throw logic with describe + single notify path. |
| lua/agitate/core/repo.lua | Renames commands, fixes init remote spacing, improves error handling. |
| lua/agitate/core/branch.lua | Migrates missing-arg reporting to vim.notify at ERROR level. |
| lua/agitate/config.lua | Migrates import failure reporting to vim.notify and uses renamed error var. |
| lua/agitate/api/repo.lua | Renames user commands and migrates error handling to vim.notify. |
| lua/agitate/api/init.lua | Migrates error handling to vim.notify and uses renamed error var. |
| lua/agitate/api/branch.lua | Migrates error handling to vim.notify and uses renamed error var. |
| .luacheckrc | Allows writable vim in tests to support notify stubbing. |
Review details
- Files reviewed: 17/17 changed files
- Comments generated: 9
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
3
to
6
| local types_ok, _ = pcall(require, 'agitate.types.error') | ||
| if not types_ok then | ||
| return vim.api.nvim_err_writeln(require('agitate.const.error').err_types) | ||
| end | ||
|
|
||
| ---Throws the given error, closing the current execution of agitate.nvim | ||
| ---@param error AgitateError|table|string The error to throw | ||
| function M.throw(error) | ||
| if type(error) == 'table' then | ||
| for _, value in ipairs(error) do | ||
| M.throw(value) | ||
| end | ||
| end | ||
| vim.api.nvim_err_writeln('There was an error during execution of agitate.nvim:') | ||
| if type(error) ~= 'string' then | ||
| return M.throw(M.unhandled('agitate.error.throw')) | ||
| end | ||
| vim.api.nvim_err_writeln(error) | ||
| return vim.notify(require('agitate.const.error').err_types, vim.log.levels.ERROR) | ||
| end |
Comment on lines
+3
to
6
| local ok, agitate_error = pcall(require, 'agitate.error') | ||
| if not ok then | ||
| return vim.api.nvim_err_writeln(require('agitate.const.error').import) | ||
| return vim.notify(require('agitate.const.error').import, vim.log.levels.ERROR) | ||
| end |
| end | ||
|
|
||
| return false, error.throw('service.github.get_organization -- Error:' .. json_decoded.message) | ||
| return false, agitate_error.throw('service.github.get_organization -- Error:' .. json_decoded.message) |
Comment on lines
+3
to
6
| local ok, agitate_error = pcall(require, 'agitate.error') | ||
| if not ok then | ||
| return vim.api.nvim_err_writeln(require('agitate.const.error').import) | ||
| return vim.notify(require('agitate.const.error').import, vim.log.levels.ERROR) | ||
| end |
Comment on lines
+3
to
6
| local ok, agitate_error = pcall(require, 'agitate.error') | ||
| if not ok then | ||
| return vim.api.nvim_err_writeln(require('agitate.const.error').import) | ||
| return vim.notify(require('agitate.const.error').import, vim.log.levels.ERROR) | ||
| end |
Comment on lines
+3
to
11
| local ok, agitate_error = pcall(require, 'agitate.error') | ||
| if not ok then | ||
| return vim.api.nvim_err_writeln(require('agitate.const.error').import) | ||
| return vim.notify(require('agitate.const.error').import, vim.log.levels.ERROR) | ||
| end | ||
|
|
||
| local types_ok, types_or_err = pcall(require, 'agitate.types.config') | ||
| if not types_ok then | ||
| return error.throw(types_or_err) | ||
| return agitate_error.throw(types_or_err) | ||
| end |
Comment on lines
+3
to
6
| local ok, agitate_error = pcall(require, 'agitate.error') | ||
| if not ok then | ||
| return vim.api.nvim_err_writeln(require('agitate.const.error').import) | ||
| return vim.notify(require('agitate.const.error').import, vim.log.levels.ERROR) | ||
| end |
Comment on lines
+3
to
6
| local ok, agitate_error = pcall(require, 'agitate.error') | ||
| if not ok then | ||
| return vim.api.nvim_err_writeln(require('agitate.const.error').import) | ||
| return vim.notify(require('agitate.const.error').import, vim.log.levels.ERROR) | ||
| end |
Comment on lines
+3
to
6
| local ok, agitate_error = pcall(require, 'agitate.error') | ||
| if not ok then | ||
| return vim.api.nvim_err_writeln(require('agitate.const.error').import) | ||
| return vim.notify(require('agitate.const.error').import, vim.log.levels.ERROR) | ||
| end |
Every module opened with a guard of the shape:
local ok, agitate_error = pcall(require, 'agitate.error')
if not ok then
return vim.notify(...)
end
`vim.notify` returns nil, so the module returns nil, and Lua turns a nil
module return into `true` in `package.loaded`. The caller's own
`pcall(require, 'agitate.core.repo')` therefore succeeds and hands back
a boolean, and the next line crashes with `attempt to index a boolean
value`.
Confirmed against LuaJIT: a module returning nil yields `pcall ok: true,
value: true`. Every one of these guards was reporting the problem and
then producing a worse one, and the layered `pcall(require, ...)` checks
throughout could never fire.
Notifies and then raises, so `pcall(require, ...)` returns false and the
caller's existing guard works as written. Applies to all eight
bootstrap guards and the six dependency guards that returned
`agitate_error.throw(...)`, which had the same shape for the same
reason.
`get_organization` returned `false, agitate_error.throw(...)`. `throw`
returns nothing, so the error payload was dropped and the caller got
`false, nil`. Returns the error instead of throwing from inside a
value-returning path; the failure still surfaces, because the repository
creation that follows reports its own.
`parse_args` gains a third return value and stops dropping input:
- An undeclared flag now takes its value with it. Previously `-z value`
left `value` unconsumed, so it filled the first declared flag
positionally and a mistyped flag name quietly assigned its value to
something else.
- A declared flag with no value is reported separately from
unrecognised arguments. Both were leftovers, so `:AgitateRepoCreate
-r` complained that `-r` was unrecognised when the real problem was
the missing value.
Tracking those two states needed separating "not available to fill a
flag positionally" from "must still be reported": marking a rejected
value only as consumed stopped it being reinterpreted but also dropped
it silently, which was the original defect.
`Create` now reports both, having previously discarded the second
return value entirely.
There was a problem hiding this comment.
🟡 Changes recommended
The GitHub service layer still constructs curl commands via shell strings (injection/quoting risk) and has a misleading 404 handling path in get_organization that should be corrected before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 17/17 changed files
- Comments generated: 2
- Review effort level: Lite
Comment on lines
119
to
123
| if json_decoded.message then | ||
| if json_decoded.message == 'Not Found' then | ||
| return false, error.unhandled('service.github.get_organization') | ||
| return false, agitate_error.unhandled('service.github.get_organization') | ||
| end | ||
|
|
Comment on lines
48
to
52
| .. repository | ||
| .. [[","private":]] | ||
| .. tostring(is_private) | ||
| .. [["}']] | ||
| .. [[}']] | ||
| ) |
`util.execute_command` passes its argument to `vim.fn.systemlist`, which runs a string through a shell. Both requests interpolated the access token, the repository name and the API path into that string unescaped, so a repository name containing a quote or a shell metacharacter could break the command or run something else entirely. Both now pass an argument vector, which never reaches a shell, and the request body is built with `vim.json.encode` rather than by hand. That also removes the concatenated JSON that produced a malformed body until recently. The token is still visible in argv through `/proc/<pid>/cmdline`, which the HTTP branch fixes properly by moving it to curl's stdin. This change is about the injection, not the exposure. `get_organization` returned `unhandled(...)` for a 404. A 404 there is how GitHub says the name belongs to a user rather than an organization, which is the common case and not a defect, so labelling it as an unhandled error made the error channel useless for real failures. It returns the decoded payload instead.
There was a problem hiding this comment.
🔵 Needs a closer look
A couple of newly introduced/modified code paths need small but important input validation hardening (e.g., flatten_table skip normalization and -v visibility validation) to prevent silent misbehavior or runtime errors.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
lua/agitate/util.lua:26
flatten_tableassumesopts.skipis a non-negative integer; if a caller passes a negative or non-integer value, the numericforstart index can become <= 0 or fractional, causing nil entries and atable.concatfailure. Clamp/normalizeskipbefore looping to keep the helper robust.
local skip = opts and opts.skip or 0
local parts = {}
for index = skip + 1, #lines do
parts[#parts + 1] = lines[index]
end
lua/agitate/core/repo.lua:56
- The
-vflag is documented as accepting onlypublicorprivate, but the current logic treats any other value aspublic(since it only checks for'private'). This can silently do the wrong thing on typos; validate the value and report an error when it’s not one of the supported options.
local repository_name = parameters['-r'] or util.get_directory_name()
local github_username = parameters['-u'] or options.github_username
local is_private = parameters['-v'] == 'private'
- Files reviewed: 17/17 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
There are a few confirmed user-facing correctness/polish issues (duplicate error notifications in service/github.lua, missing validation for -v visibility, and a misleading throw docstring) that should be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
lua/agitate/service/github.lua:90
- Same as the earlier trim failure: this code path calls
vim.notifyand then returns an error which is typically reported again by the caller. Returning a structured error here avoids duplicate notifications and keeps presentation in one place.
if json_decoded == nil or json_decoded == '' then
vim.notify('post_new_repo -- Error: Empty json response after decode: `' .. flattened_github_response .. '`', vim.log.levels.ERROR)
return false, agitate_error.unhandled('service.github.post_new_repo')
else
- Files reviewed: 17/17 changed files
- Comments generated: 3
- Review effort level: Lite
Comment on lines
64
to
68
| if not json_lr_trim_ok then | ||
| vim.api.nvim_err_writeln('post_new_repo -- Error: Empty json response after trim: `' .. flattened_github_response .. '`') | ||
| vim.notify('post_new_repo -- Error: Empty json response after trim: `' .. flattened_github_response .. '`', vim.log.levels.ERROR) | ||
|
|
||
| return json_lr_trim_ok, error.unhandled('service.github.post_new_repo') | ||
| return json_lr_trim_ok, agitate_error.unhandled('service.github.post_new_repo') | ||
| end |
Comment on lines
+54
to
57
| local repository_name = parameters['-r'] or util.get_directory_name() | ||
| local github_username = parameters['-u'] or options.github_username | ||
| local is_private = parameters['-v'] == 'private' | ||
|
|
Comment on lines
+53
to
+54
| ---Reports the given error to the user, ending the current execution of agitate.nvim | ||
| ---@param err AgitateError|table|string The error to throw |
…tring Three review findings. `post_new_repo` notified the user and then returned an error that `Create` reports through `agitate_error.throw`, so a malformed response produced two notifications for one failure. The service now only returns, carrying the response text the notify used to print, and the caller decides how to show it. `Create` derived `is_private` from `parameters['-v'] == 'private'`, so anything else meant public and `-v privte` silently created a public repository. Of the two directions that is the one that cannot be taken back, so the value is validated. The HTTP branch already did this; it belongs here too, since this branch can merge alone. `throw`'s docstring claimed it ends execution. It reports and returns, which is why callers write `return agitate_error.throw(...)` and why the load guards pair it with `error(...)`. Says so.
There was a problem hiding this comment.
🟢 Approval recommended
The changes address concrete correctness bugs, update call sites consistently, and are backed by significantly expanded automated tests for the touched behavior.
Review details
- Files reviewed: 17/17 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
One remaining user-facing failure path in repo creation can mask GitHub’s actual error message (e.g. “Bad credentials”) behind a generic “no html_url” report.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 17/17 changed files
- Comments generated: 1
- Review effort level: Lite
Comment on lines
114
to
+118
| if not github_post_response.html_url then | ||
| return vim.api.nvim_err_writeln( | ||
| 'Agitate | CreateGitHubCurl | Error:' | ||
| .. '\nError during repository creation at ' | ||
| .. util.build_github_html_url(github_username, new_github_repository_name) | ||
| .. '\nReason: `html_url` not found in response. Full response: `' | ||
| .. util.flatten_table(github_post_response) | ||
| .. '`' | ||
| return agitate_error.throw( | ||
| 'core.repo.Create -- Error: repository creation at ' | ||
| .. util.build_github_html_url(github_username, repository_name) | ||
| .. ' returned no `html_url`.' |
A failure body like `{"message":"Bad credentials"}` has no `errors`
array, so it skipped the errors branch, fell through to the `html_url`
check and was reported as "returned no html_url". The user got a
structural complaint about the response instead of "Bad credentials".
Checks for a top level `message` before that branch, so the reason
GitHub gave is the reason shown.
Also corrects the `optional_parameters` annotations from
`table<string>`, which is not valid LuaLS and describes the wrong
shape, to `string[]`. Done across the file rather than at the one line
that was flagged, since this has now been reported on several branches
one file at a time.
Every direct git call in the plugin had the same two defects, and they were being reported and fixed one call site per review round. It ran in the process working directory. Fugitive resolves the repository from the buffer, so replacing `:G` with a direct call meant `:cd` silently changed which repository Agitate acted on. Deleting a branch in the wrong repository is not recoverable. It captured stdout only, and git reports almost every failure on stderr, so a failed command was reported with no reason. `util.git` does both correctly in one place: `-C` the buffer's directory, falling back to the working directory for a nameless buffer, and both streams merged. Call sites move onto it in the branches that own them.
There was a problem hiding this comment.
🟡 Changes recommended
Newly added util.buffer_directory/util.git functionality is not covered by tests despite other agitate.util helpers being tested, increasing regression risk for repository targeting logic.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 17/17 changed files
- Comments generated: 1
- Review effort level: Lite
Comment on lines
+85
to
+89
| function M.git(argv) | ||
| local command = { 'git', '-C', M.buffer_directory() } | ||
| vim.list_extend(command, argv) | ||
|
|
||
| local completed = vim.system(command, { text = true }):wait() |
`M.git` always ran in the buffer's directory, which is right for the branch commands but wrong for `Init`. `Init` is documented as initialising the *current* directory and writes its README relative to `getcwd()`, so running git somewhere else split one command across two directories: the README landed in one and `git init` ran in the other. The parameter defaults to the buffer's directory, so every existing caller is unchanged, and `Init` can ask for the working directory explicitly.
`buffer_directory` and `git` decide which repository every git command in the plugin acts on, and neither had a test. `buffer_directory` in particular is the whole fix for the `:cd` bug, so it was the one function most worth pinning. Four cases: the working directory fallback for a nameless buffer, the buffer winning over the working directory, success and failure reporting with git's stderr reaching the caller, and the explicit directory argument being honoured.
There was a problem hiding this comment.
🟢 Approval recommended
The changes are cohesive, address concrete correctness failures, and are backed by expanded automated test coverage for the touched logic.
Review details
- Files reviewed: 17/17 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The changes address concrete correctness issues, align return contracts with callers, and add targeted tests covering the new/changed behavior.
Review details
- Files reviewed: 17/17 changed files
- Comments generated: 0 new
- Review effort level: Lite
`util.git` calls `vim.system`, which arrived in 0.10. On anything older the failure was `attempt to call field 'system' (a nil value)` raised from inside a git operation, which names neither the requirement nor the command that hit it. Returns the plugin's usual failure pair with a message naming the version, so the caller reports it the same way it reports any other git failure. The CI floor is already 0.10; this makes the runtime say so.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
All the correctness bugs found during the repo review, plus the four wanted commits rescued from the abandoned
lua-httpbranch.Three of these are user-facing breakage on
maintoday: repository creation sends invalid JSON,:AgitateRepoInitnever adds a remote, and a missing username or token hard crashes instead of reporting.Bugs fixed
error.throwrecursed forevererrorlocal shadowed Lua's builtinerror(...)called a table:attempt to call a table valueon missing credentialsCreatedid not return after throwing{"name":"x","private":true"}, GitHub rejects it, no repo createdf212921G remote add originhttps://..., remote never added, the following push has no originb1bb2f2parse_argsreturned one valueflatten_tableon a decoded object'', soFull response:was always emptyflatten_tableshadowedtablejson_lr_trimreturned one value on failureerrors[1].messageunguardedThe two regressions worth reading
f212921added"private":<bool>to the request body but left the closing fragment[["}']]in place. That quote had been terminating thenamestring. Since that commit the body has been:b1bb2f2replaced the literal'G remote add origin https://github.com/'withbuild_github_html_url. The space was part of that literal and nothing reintroduced it.Also in here
lua-http: theInitGitHubtoInitandCreateGitHubCurltoCreaterenames, therepository_namerename, and the trailing-slash fix. The two luasocket commits are dropped;socket.httpcannot do HTTPS and the dependency was rejected.vim.notifymigration: all elevennvim_err_writelncalls, deprecated since 0.11, now notify at an explicit level. Informationalprintcalls become INFO. Markedrefactor!because output no longer goes straight to the message area.parse_argshardening: undeclared flags like-z valuewere written into the result as if real. Only declared flags are honoured now, and anything unmatched is returned as leftovers.projectscope) instead of leaving it as an undifferentiatedtodo!().Tests
Suite goes from 2 to 25. New
error_specandutil_spec,parse_args_specexpanded from 1 case to 8.The error spec includes a direct regression test for the recursion: a messageless table must report exactly once and return.
Not verified
The JSON payload fix is verified by reproducing the string, not by a live API call, since that would create a real repository. The HTTP PR that follows replaces this code path entirely with
vim.systemand tests the request construction directly.