ronantakizawa/codereview-bench huggingface dataset:
Repository: neovim/neovim
File: runtime/lua/vim/provider/health.lua (Lua)

Code before:
local health = vim.health
local iswin = vim.fn.has('win32') == 1

local M = {}

local function cmd_ok(cmd)
  local result = vim.system(cmd, { text = true }):wait()
  return result.code == 0, result.stdout
end

local function cli_version(cmd)
  local ok, out = cmd_ok(cmd)
  return ok, vim.version.parse(out, { strict = false })
end

local function system(cmd, args)
  args = args or {}
  local ignore_error = args.ignore_error or false
  --[[@type vim.SystemOpts]]
  local opts = {
    text = true,
    stdin = args.stdin,
    timeout = args.timeout or (30 * 1000),
  }

  local result = vim.system(cmd, opts):wait()
  local output = result.stdout or ''

  if result.code ~= 0 and not ignore_error then
    local emsg = string.format(
      'Command error (exit code %d): %s (in %s)',
      result.code,
      table.concat(cmd, ' '),
      vim.uv.cwd()
    )
    if output:find('%S') then
      emsg = string.format('%s\noutput: %s', emsg, output)
    end
    if result.stderr:find('%S') then
      emsg = string.format('%s\nstderr: %s', emsg, result.stderr)
    end

Review comment (refactor):
why remove this? it was designed to handle `string[]` list commands. it does more than just table.concat()

Code after:
local health = vim.health
local iswin = vim.fn.has('win32') == 1

local M = {}

local function cmd_ok(cmd)
  local result = vim.system(cmd, { text = true }):wait()
  return result.code == 0, result.stdout
end

local function cli_version(cmd)
  local ok, out = cmd_ok(cmd)
  return ok, vim.version.parse(out, { strict = false })
end

-- Attempts to construct a shell command from an args list.
-- Only for display, to help users debug a failed command.
--- @param cmd string|string[]
local function shellify(cmd)
  if type(cmd) ~= 'table' then
    return cmd
  end
  local escaped = {} --- @type string[]
  for i, v in ipairs(cmd) do
    escaped[i] = v:match('[^A-Za-z_/.-]') and vim.fn.shellescape(v) or v
  end
  return table.concat(escaped, ' ')
end

-- Handler for s:system() function.
--- @param self {output: string, stderr: string, add_stderr_to_output: boolean}
local function system_handler(self, _, data, event)
  if event == 'stderr' then
    if self.add_stderr_to_output then
      self.output = self.output .. table.concat(data, '')
    else
      self.stderr = self.stderr .. table.concat(data, '')
    end
  elseif event == 'stdout' then
    self.output = self.output .. table.concat(data, '')
  end
Repository: neovim/neovim
File: runtime/lua/vim/provider/health.lua (Lua)

Code before:
    return { '', '' }
  end

  health.info('pyenv: Root: ' .. pyenv_root)

  return { pyenv_path, pyenv_root }
end

-- Check the Python interpreter's usability.
local function check_bin(bin)
  if not is(bin, 'file') and (not iswin or not is(bin .. '.exe', 'file')) then
    health.error('"' .. bin .. '" was not found.')
    return false
  elseif vim.fn.executable(bin) == 0 then
    health.error('"' .. bin .. '" is not executable.')
    return false
  end
  return true
end

--- Fetch the contents of a URL.
---
--- @param url string
local function download(url)
  local has_curl = vim.fn.executable('curl') == 1
  if has_curl and cmd_ok({ 'curl', '-V' })[2]:find('Protocols:.*https') then
    local out, rc = system({ 'curl', '-sL', url }, { stderr = true, ignore_error = true })
    if rc ~= 0 then
      return 'curl error with ' .. url .. ': ' .. rc
    else
      return out
    end
  elseif vim.fn.executable('python') == 1 then
    local script = ([[
try:
    from urllib.request import urlopen
except ImportError:
    from urllib2 import urlopen

response = urlopen('%s')
print(response.read().decode('utf8'))
]]):format(url)
    local out, rc = system({ 'python', '-c', script })
    if out == '' and rc ~= 0 then
      return 'python urllib.request error: ' .. rc
    else
      return out
    end
  end

  local message = 'missing `curl` '

Review comment (suggestion):
this isn't going to work. multi-return can't be indexed. need to either surround with `({ ... })` or save to a local var.

Code after:
    return { '', '' }
  end

  health.info('pyenv: Root: ' .. pyenv_root)

  return { pyenv_path, pyenv_root }
end

-- Check the Python interpreter's usability.
local function check_bin(bin)
  if not is(bin, 'file') and (not iswin or not is(bin .. '.exe', 'file')) then
    health.error('"' .. bin .. '" was not found.')
    return false
  elseif vim.fn.executable(bin) == 0 then
    health.error('"' .. bin .. '" is not executable.')
    return false
  end
  return true
end

--- Fetch the contents of a URL.
---
--- @param url string
local function download(url)
  local has_curl = vim.fn.executable('curl') == 1
  if has_curl then
    local ok, out = cmd_ok({ 'curl', '-V' })
    if ok and out:find('Protocols:.*https') then
      local content, rc = system({ 'curl', '-sL', url }, { stderr = true, ignore_error = true })
      if rc ~= 0 then
        return 'curl error with ' .. url .. ': ' .. rc
      else
        return content
      end
    end
  elseif vim.fn.executable('python') == 1 then
    local script = ([[
try:
    from urllib.request import urlopen
except ImportError:
    from urllib2 import urlopen

response = urlopen('%s')
print(response.read().decode('utf8'))
]]):format(url)
    local out, rc = system({ 'python', '-c', script })
    if out == '' and rc ~= 0 then
      return 'python urllib.request error: ' .. rc
    else
      return out
    end
Repository: neovim/neovim
File: runtime/lua/vim/version.lua (Lua)

Code before:
    elseif mods == '~' then
      if #parts >= 2 then
        to[2] = to[2] + 1
        to[3] = 0
      else
        to[1] = to[1] + 1
        to[2] = 0
        to[3] = 0
      end
    elseif mods == '^' then
      for i = 1, 3 do
        if to[i] ~= 0 then
          to[i] = to[i] + 1
          for j = i + 1, 3 do
            to[j] = 0
          end
          break
        end
      end
    end
    ---@diagnostic enable: need-check-nil
    return setmetatable({ from = from, to = to }, range_mt)
  end
end

--- Compute common range
---
--- @param r1 vim.VersionRange First range to intersect.
--- @param r2 vim.VersionRange Second range to intersect.
--- @return vim.VersionRange? Biggest range that is present inside both `r1` and `r2`.
---   `nil` if such range does not exist.
function M.intersect(r1, r2)
  assert(getmetatable(r1) == range_mt)
  assert(getmetatable(r2) == range_mt)

  local from = r1.from <= r2.from and r2.from or r1.from
  local to = (r1.to == nil or (r2.to ~= nil and r2.to <= r1.to)) and r2.to or r1.to
  if to == nil or from < to or (from == to and r1:has(from) and r2:has(from)) then
    return setmetatable({ from = from, to = to }, VersionRange)
  end
end

---@param v string|vim.Version
---@return string
local function create_err_msg(v)
  if type(v) == 'string' then
    return string.format('invalid version: "%s"', tostring(v))
  elseif type(v) == 'table' and v.major then
    return string.format('invalid version: %s', vim.inspect(v))
  end
  return string.format('invalid version: %s (%s)', tostring(v), type(v))

Review comment (suggestion):
missing `@since`

```suggestion
--- Computes the common range shared by the given ranges.
```

Code after:
      end
      to = nil
    elseif mods == '>=' then
      to = nil
    elseif mods == '~' then
      if #parts >= 2 then
        to[2] = to[2] + 1
        to[3] = 0
      else
        to[1] = to[1] + 1
        to[2] = 0
        to[3] = 0
      end
    elseif mods == '^' then
      for i = 1, 3 do
        if to[i] ~= 0 then
          to[i] = to[i] + 1
          for j = i + 1, 3 do
            to[j] = 0
          end
          break
        end
      end
    end
    ---@diagnostic enable: need-check-nil
    return setmetatable({ from = from, to = to }, range_mt)
  end
end

--- Computes the common range shared by the given ranges.
---
--- @since 14
--- @param r1 vim.VersionRange First range to intersect.
--- @param r2 vim.VersionRange Second range to intersect.
--- @return vim.VersionRange? Maximal range that is present inside both `r1` and `r2`.
---   `nil` if such range does not exist.
function M.intersect(r1, r2)
  assert(getmetatable(r1) == range_mt)
  assert(getmetatable(r2) == range_mt)

  local from = r1.from <= r2.from and r2.from or r1.from
  local to = (r1.to == nil or (r2.to ~= nil and r2.to <= r1.to)) and r2.to or r1.to
  if to == nil or from < to or (from == to and r1:has(from) and r2:has(from)) then
    return setmetatable({ from = from, to = to }, VersionRange)
  end
end

---@param v string|vim.Version
---@return string
local function create_err_msg(v)
  if type(v) == 'string' then
Repository: neovim/neovim
File: runtime/plugin/shada.lua (Lua)

Code before:
if vim.g.loaded_shada_plugin then
  return
end
vim.g.loaded_shada_plugin = 1

local augroup = vim.api.nvim_create_augroup('nvim.shada', {})
local pattern = { '*.shada', '*.shada.tmp.[a-z]' }

---Ensures that pattern and augroup are set correctly.
---@param event string|string[]
---@param opts vim.api.keyset.create_autocmd
local function shada_autocmd(event, opts)
  opts.group = augroup
  opts.pattern = pattern
  vim.api.nvim_create_autocmd(event, opts)
end

---@type fun(binstrings: string[]): string[]
local shada_get_strings = vim.fn['shada#get_strings']
---@type fun(strings: string[]): string[]
local shada_get_binstrings = vim.fn['shada#get_binstrings']

---Read shada strings from file.
---@param file string Filename
---@return string[] # lines from shada file
local function read_strings(file)
  local f = assert(io.open(file, 'rb'))
  local strings = f:read('*a')
  f:close()
  return shada_get_strings(strings)
end

shada_autocmd('BufReadCmd', {
  callback = function(ev)
    if vim.v.cmdarg ~= '' then
      error('++opt not supported')
    end

Review comment (refactor):
if you're going to abstract this then might as well use it to save more code?
```suggestion
local function def_autocmd(event, opts, fn)
  ...
  opts.callback = function(ev)
    if vim.v.cmdarg ~= '' then
      error('++opt not supported')
    end
    fn(ev)
  end
  vim.api.nvim_create_autocmd(event, opts)
```

Code after:
if vim.g.loaded_shada_plugin then
  return
end
vim.g.loaded_shada_plugin = 1

local augroup = vim.api.nvim_create_augroup('nvim.shada', {})

---@type fun(binstrings: string[]): string[]
local shada_get_strings = vim.fn['shada#get_strings']

---@type fun(strings: string[]): string[]
local shada_get_binstrings = vim.fn['shada#get_binstrings']

---Ensures that pattern and augroup are set correctly.
---@param event string|string[]
---@param opts vim.api.keyset.create_autocmd
---@param fn fun(args: vim.api.keyset.create_autocmd.callback_args): boolean?
local function def_autocmd(event, opts, fn)
  opts = opts or {}
  opts.group = augroup
  opts.pattern = { '*.shada', '*.shada.tmp.[a-z]' }
  opts.callback = function(ev)
    if vim.v.cmdarg ~= '' then
      error('++opt not supported')
    end
    fn(ev)
  end
  vim.api.nvim_create_autocmd(event, opts)
end

---Read shada strings from file.
---@param file string Filename
---@return string[] # lines from shada file
local function read_strings(file)
  local f = assert(io.open(file, 'rb'))
  local strings = f:read('*a')
  f:close()
Repository: neovim/neovim
File: runtime/lua/vim/_system.lua (Lua)

Code before:

  local towrite --- @type string|string[]?
  if type(input) == 'string' or type(input) == 'table' then
    towrite = input
  end

  return assert(uv.new_pipe(false)), towrite
end

--- @return table<string,string>
local function base_env()
  local env = vim.fn.environ() --- @type table<string,string>
  env['NVIM'] = vim.v.servername
  env['NVIM_LISTEN_ADDRESS'] = nil
  return env
end

--- uv.spawn will completely overwrite the environment
--- when we just want to modify the existing one, so
--- make sure to prepopulate it with the current env.
--- @param env? table<string,string|number>
--- @param clear_env? boolean
--- @return string[]?
local function setup_env(env, clear_env)
  if not clear_env then
    --- @type table<string,string|number>
    env = vim.tbl_extend('force', base_env(), env or {})
  end

  local renv = {} --- @type string[]
  for k, v in pairs(env or {}) do
    renv[#renv + 1] = string.format('%s=%s', k, tostring(v))
  end

  return renv
end

local is_win = vim.fn.has('win32') == 1

local M = {}

--- @param cmd string
--- @param opts uv.spawn.options
--- @param on_exit fun(code: integer, signal: integer)
--- @param on_error fun()
--- @return uv.uv_process_t, integer
local function spawn(cmd, opts, on_exit, on_error)
  if is_win then
    local cmd1 = vim.fn.exepath(cmd)
    if cmd1 ~= '' then
      cmd = cmd1

Review comment (performance):
is this for performance? would it resolve your use-case if we stored a module-level env table and returned it here ?
```suggestion
    return _base_env
```

Code after:

  local towrite --- @type string|string[]?
  if type(input) == 'string' or type(input) == 'table' then
    towrite = input
  end

  return assert(uv.new_pipe(false)), towrite
end

--- @return table<string,string>
local function base_env()
  local env = vim.fn.environ() --- @type table<string,string>
  env['NVIM'] = vim.v.servername
  env['NVIM_LISTEN_ADDRESS'] = nil
  return env
end

--- uv.spawn will completely overwrite the environment
--- when we just want to modify the existing one, so
--- make sure to prepopulate it with the current env.
--- @param env? table<string,string|number>
--- @param clear_env? boolean
--- @return string[]?
local function setup_env(env, clear_env)
  if not env and clear_env then
    return
  end

  env = env or {}
  if not clear_env then
    --- @type table<string,string|number>
    env = vim.tbl_extend('force', base_env(), env)
  end

  local renv = {} --- @type string[]
  for k, v in pairs(env) do
    renv[#renv + 1] = string.format('%s=%s', k, tostring(v))
  end

  return renv
end

local is_win = vim.fn.has('win32') == 1

local M = {}

--- @param cmd string
--- @param opts uv.spawn.options
--- @param on_exit fun(code: integer, signal: integer)
--- @param on_error fun()
--- @return uv.uv_process_t, integer
Repository: neovim/neovim
File: src/nvim/ex_docmd.c (C)

Code before:

  if (fname == NULL) {
    return;
  }

  eap->arg = fname;
  do_exedit(eap, NULL);
  xfree(fname);
}

/// ":edit", ":badd", ":balt", ":visual".
static void ex_edit(exarg_T *eap)
{
  char *ffname = eap->cmdidx == CMD_enew ? NULL : eap->arg;

  // Exclude commands which keep the window's current buffer
  if (eap->cmdidx != CMD_badd
      && eap->cmdidx != CMD_balt
      // All other commands must obey 'winfixbuf' / ! rules
      && (is_other_file(0, ffname) && !check_can_set_curbuf_forceit(eap->forceit))) {
    return;
  }

  // prevent use of :edit on prompt-buffers
  if (bt_prompt(curbuf) && eap->cmdidx == CMD_edit && *eap->arg == NUL) {
    emsg(":edit on prompt buffer itself is not allowed");
    return;
  }

  do_exedit(eap, NULL);
}

/// ":edit <file>" command and alike.
///
/// @param old_curwin  curwin before doing a split or NULL
void do_exedit(exarg_T *eap, win_T *old_curwin)
{
  // ":vi" command ends Ex mode.
  if (exmode_active && (eap->cmdidx == CMD_visual
                        || eap->cmdidx == CMD_view)) {
    exmode_active = false;
    ex_pressedreturn = false;
    if (*eap->arg == NUL) {
      // Special case:  ":global/pat/visual\NLvi-commands"
      if (global_busy) {
        if (eap->nextcmd != NULL) {
          stuffReadbuff(eap->nextcmd);
          eap->nextcmd = NULL;
        }

        const int save_rd = RedrawingDisabled;

Review comment (suggestion):
Is there a way we can allow it but just don't empty the buffer? 
(I'm wondering if there are useful events that `:edit` triggers, which plugins might want.)

OTOH, we could try this and relax it later if needed.

```suggestion
    emsg("cannot :edit a prompt buffer");
```

Code after:

  if (fname == NULL) {
    return;
  }

  eap->arg = fname;
  do_exedit(eap, NULL);
  xfree(fname);
}

/// ":edit", ":badd", ":balt", ":visual".
static void ex_edit(exarg_T *eap)
{
  char *ffname = eap->cmdidx == CMD_enew ? NULL : eap->arg;

  // Exclude commands which keep the window's current buffer
  if (eap->cmdidx != CMD_badd
      && eap->cmdidx != CMD_balt
      // All other commands must obey 'winfixbuf' / ! rules
      && (is_other_file(0, ffname) && !check_can_set_curbuf_forceit(eap->forceit))) {
    return;
  }

  // prevent use of :edit on prompt-buffers
  if (bt_prompt(curbuf) && eap->cmdidx == CMD_edit && *eap->arg == NUL) {
    emsg("cannot :edit a prompt buffer");
    return;
  }

  do_exedit(eap, NULL);
}

/// ":edit <file>" command and alike.
///
/// @param old_curwin  curwin before doing a split or NULL
void do_exedit(exarg_T *eap, win_T *old_curwin)
{
  // ":vi" command ends Ex mode.
  if (exmode_active && (eap->cmdidx == CMD_visual
                        || eap->cmdidx == CMD_view)) {
    exmode_active = false;
    ex_pressedreturn = false;
    if (*eap->arg == NUL) {
      // Special case:  ":global/pat/visual\NLvi-commands"
      if (global_busy) {
        if (eap->nextcmd != NULL) {
          stuffReadbuff(eap->nextcmd);
          eap->nextcmd = NULL;
        }

        const int save_rd = RedrawingDisabled;
Repository: neovim/neovim
File: src/nvim/ex_docmd.c (C)

Code before:
    if (globaldir == NULL && pdir != NULL) {
      globaldir = xstrdup(pdir);
    }
  }

  char cwd[MAXPATHL];
  if (os_dirname(cwd, MAXPATHL) != OK) {
    return;
  }
  switch (scope) {
  case kCdScopeGlobal:
    // We are now in the global directory, no need to remember its name.
    XFREE_CLEAR(globaldir);
    break;
  case kCdScopeTabpage:
    curtab->tp_localdir = xstrdup(cwd);
    break;
  case kCdScopeWindow:
    curwin->w_localdir = xstrdup(cwd);
    break;
  case kCdScopeInvalid:
    abort();
  }

  last_chdir_reason = NULL;
  shorten_fnames(strchr(p_cpo, CPO_NOSYMLINKS) == NULL);

  if (trigger_dirchanged) {
    do_autocmd_dirchanged(cwd, scope, kCdCauseManual, false);
  }
}

/// Change directory function used by :cd/:tcd/:lcd Ex commands and the chdir() function.
/// @param new_dir  The directory to change to.
/// @param scope    Scope of the function call (global, tab or window).
/// @return true if the directory is successfully changed.
bool changedir_func(char *new_dir, CdScope scope)
{
  if (new_dir == NULL || allbuf_locked()) {
    return false;
  }

  char *pdir = NULL;
  // ":cd -": Change to previous directory
  if (strcmp(new_dir, "-") == 0) {
    pdir = get_prevdir(scope);
    if (pdir == NULL) {
      emsg(_("E186: No previous directory"));
      return false;
    }
    new_dir = pdir;

Review comment (suggestion):
```suggestion
  shorten_fnames(vim_strchr(p_cpo, CPO_NOSYMLINKS) == NULL);
```

Code after:
    if (globaldir == NULL && pdir != NULL) {
      globaldir = xstrdup(pdir);
    }
  }

  char cwd[MAXPATHL];
  if (os_dirname(cwd, MAXPATHL) != OK) {
    return;
  }
  switch (scope) {
  case kCdScopeGlobal:
    // We are now in the global directory, no need to remember its name.
    XFREE_CLEAR(globaldir);
    break;
  case kCdScopeTabpage:
    curtab->tp_localdir = xstrdup(cwd);
    break;
  case kCdScopeWindow:
    curwin->w_localdir = xstrdup(cwd);
    break;
  case kCdScopeInvalid:
    abort();
  }

  last_chdir_reason = NULL;
  shorten_fnames(vim_strchr(p_cpo, CPO_NOSYMLINKS) == NULL);

  if (trigger_dirchanged) {
    do_autocmd_dirchanged(cwd, scope, kCdCauseManual, false);
  }
}

/// Change directory function used by :cd/:tcd/:lcd Ex commands and the chdir() function.
/// @param new_dir  The directory to change to.
/// @param scope    Scope of the function call (global, tab or window).
/// @return true if the directory is successfully changed.
bool changedir_func(char *new_dir, CdScope scope)
{
  if (new_dir == NULL || allbuf_locked()) {
    return false;
  }

  char *pdir = NULL;
  // ":cd -": Change to previous directory
  if (strcmp(new_dir, "-") == 0) {
    pdir = get_prevdir(scope);
    if (pdir == NULL) {
      emsg(_("E186: No previous directory"));
      return false;
    }
    new_dir = pdir;
Repository: neovim/neovim
File: src/nvim/ex_docmd.c (C)

Code before:
}

/// ":qall": try to quit all windows
static void ex_quitall(exarg_T *eap)
{
  if (before_quit_all(eap) == FAIL) {
    return;
  }
  exiting = true;
  if (!eap->forceit && check_changed_any(false, false)) {
    not_exiting();
    return;
  }
  getout(0);
}

/// ":restart": restart the Nvim server (using ":qall").
/// ":restart +cmd": restart the Nvim server using ":cmd".
static void ex_restart(exarg_T *eap)
{
  if (eap->forceit) {
    emsg("bang (!) not supported");
    return;
  }
  Error err = ERROR_INIT;
  if (check_changed_any(false, false) || !remote_ui_restart(current_ui, &err)) {
    if (ERROR_SET(&err)) {
      emsg(err.msg);  // UI disappeared already?
      api_clear_error(&err);
    }
    return;
  }
  char *quit_cmd = (eap->do_ecmd_cmd) ? eap->do_ecmd_cmd : "qall";
  nvim_command(cstr_as_string(quit_cmd), &err);
  if (ERROR_SET(&err)) {
    emsg(err.msg);  // Could not exit
    api_clear_error(&err);
    return;
  }
  if (!exiting) {
    ELOG("':%s' did not quit the server, quitting manually", quit_cmd);
    getout(0);
  }
}

/// ":close": close current window, unless it is the last one
static void ex_close(exarg_T *eap)
{
  win_T *win = NULL;
  int winnr = 0;
  if (cmdwin_type != 0) {

Review comment (refactor):
If you have this check here, doesn't this essentially make it pointless to support `:restart +qall!`?

On the other hand, if you remove this check, even `:restart +echo` will quit the server without confirmation, as you still call `getout(0)` anyway.

Code after:
  }

  if (before_quit_autocmds(curwin, true, eap->forceit)) {
    return FAIL;
  }

  return OK;
}

/// ":qall": try to quit all windows
static void ex_quitall(exarg_T *eap)
{
  if (before_quit_all(eap) == FAIL) {
    return;
  }
  exiting = true;
  if (!eap->forceit && check_changed_any(false, false)) {
    not_exiting();
    return;
  }
  getout(0);
}

/// ":restart": restart the Nvim server (using ":qall!").
/// ":restart +cmd": restart the Nvim server using ":cmd".
static void ex_restart(exarg_T *eap)
{
  char *quit_cmd = (eap->do_ecmd_cmd) ? eap->do_ecmd_cmd : "qall!";
  Error err = ERROR_INIT;
  if ((cmdmod.cmod_flags & CMOD_CONFIRM) && check_changed_any(false, false)) {
    return;
  }
  restarting = true;
  nvim_command(cstr_as_string(quit_cmd), &err);
  if (ERROR_SET(&err)) {
    emsg(err.msg);  // Could not exit
    api_clear_error(&err);
    not_restarting();
    return;
  }
  if (!exiting) {
    emsg("restart failed: +cmd did not quit the server");
    not_restarting();
  }
}

/// ":close": close current window, unless it is the last one
static void ex_close(exarg_T *eap)
{
  win_T *win = NULL;
  int winnr = 0;