Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(actions): detached worktrees not detected for git actions #2705

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Subjective
Copy link

Description

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Tested the following pickers with detached/attached working trees and git worktrees.

  • git_files
  • git_status
  • git_stash
  • git_commits
  • git_bcommits
  • git_branches

Configuration:

  • Neovim version (nvim --version): NVIM v0.10.0-dev-d70667a
  • Operating system and version: MacOS 13.4

Checklist:

  • My code follows the style guidelines of this project (stylua)
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (lua annotations)

@Subjective
Copy link
Author

@jamestrew Since you originally created the PR for the detached git_worktrees feature, do you mind reviewing these changes?

@Subjective
Copy link
Author

Now that I think about it, none of the git actions are inheriting the use_file_path, and use_git_root options passed into the builtin git pickers.

Parameters: ~
{opts} (table) options to pass to the picker
Options: ~
{cwd} (string) specify the path of the repo
{use_file_path} (boolean) if we should use the current buffer git
root (default: false)
{use_git_root} (boolean) if we should use git root as cwd or the cwd
(important for submodule) (default: true)
{git_command} (table) command that will be executed.

From what I can tell, it doesn't seem like the cwd used by any of the actions is even guaranteed to match the cwd that was passed into the picker — currently it's being retrieved from Telescope's internal state:

local cwd = action_state.get_current_picker(prompt_bufnr).cwd

@Subjective
Copy link
Author

While I'm not yet entirely sure how telescope manages its internal state for pickers, it seems like it might be possible to store the options passed into any of the builtin get pickers via:

--- Set the status for a particular prompt bufnr
function state.set_status(prompt_bufnr, status)
TelescopeGlobalState[prompt_bufnr] = status
end

If this is true, then they can retrieved with something like:

local opts = action_state.get_current_picker(prompt_bufnr).opts

local get_git_command_output = function(args, opts)
  return utils.get_os_command_output(utils.__git_command(args, opts), opts.cwd)
end

get_git_command_output({ "checkout", "-b", new_branch }, opts))

Instead of how it's done currently:

local cwd = action_state.get_current_picker(prompt_bufnr).cwd
utils.get_os_command_output(git_command { "checkout", "-b", new_branch }, cwd)

@jamestrew
Copy link
Contributor

Hmm... yeah I see the issue and honestly it was a bit of an oversight on my part not handling the actions before merging that previous PR.

As far as getting access to the gitdir and toplevel fields inside actions, I have a couple of alternative solutions:

  1. inherit and extend the base picker and just add gitdir/toplevel (and any other necessary fields) to it and use that for all git related pickers -- then I think we can just do like
local gitdir = action_state.get_current_picker(prompt_bufnr).gitdir
  1. bind the necessary fields to the finder but this might not work since some actions cause a refresh of the picker so the binding would get lost
  2. wrap all git actions, passing in necessary fields so the actions have access to them as part of the closure
    eg.
actions.git_create_branch = function(opts)
  return function(prompt_bufnr)
    -- has access to opts.gitdir, etc
    
  end
end

-- then map like this
map({ "i", "n" }, "<c-a>", actions.git_create_branch(opts))

This way we can avoid resolving gitdir & toplevel each time want to call a git command I think.

Option 1 might be easiest but I don't think it's something we do.

@Conni2461 Is there an idiomatic way of passing arbitrary option fields for pickers to actions?

@Conni2461
Copy link
Member

I dont like this solution at all. I think there is a good reason to just store the original passed opts inside the picker and then use these later in actions. That would simplify a lot.

But i also dont like the original solution for detached worktrees (with a default config option) and i think ill have to redo them as well

@psnelgrove-r7
Copy link

psnelgrove-r7 commented Oct 14, 2023

I've been working through setting up a bare git repository for managing my dotfiles. Been using the work around mentioned in the last comment of this issue.

#435

Works reasonably well but it got me digging into why git_files wasn't working out of the box and I found #2597 which links to this issue.

What's a little confusing is where along the line git_worktrees was dropped and why.

EDIT: Ah I see that's it's in master and not the 1.x branch.

@jamestrew
Copy link
Contributor

jamestrew commented Oct 14, 2023

I've been working through setting up a bare git repository for managing my dotfiles. Been using the work around mentioned in the last comment of this issue.

Some of the git pickers support a git_command option where you can pass these git env variables. But not all do. The aim of #2597 was to enable passing these git env vars to all git pickers (for multiple git worktrees if desired) at the top level telescope settings.

Problem with both these methods is that the actions related to the pickers still won't know about the env vars.

I think there is a good reason to just store the original passed opts inside the picker and then use these later in actions.

Yeah that's what I'd like to do if I'm interpreting this correctly. Like do something like this inside each of the git actions:

local current_picker = action_state.get_current_picker(prompt_bufnr)
local gitdir = current_picker.gitdir
local toplevel = current_picker.toplevel

Is this currently possible? I don't think I've seen example of something like this in the code base.

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.

None yet

4 participants