NeogitOrg / neogit

An interactive and powerful Git interface for Neovim, inspired by Magit
MIT License
4.04k stars 239 forks source link

Neogit doesn't use the user's configured .gitmessages file #66

Closed akinsho closed 3 years ago

akinsho commented 3 years ago

image

Just to flag this although it might be a known issue already but whilst using neogit it doesn't read my .gitmessages from my .gitconfig. I've set the commit message text to use a different placeholder to match the commit style I use. The screen shot show the commit buffer with vim fugitive and neogit. Fugitive somehow is reading this information whereas neogit doesn't.

It's not a blocking issue at all just worth pointing out.

TimUntersberger commented 3 years ago

We are currently manually rebuilding the commit content and we are not respecting any configiurations on the git side.

Are you using any other configurations on the git side?

akinsho commented 3 years ago

I have git config files for work and personal use that make some changes to the defaults for example

[push]
  followTags = true
[pull]
  rebase = true
[rebase]
  autoStash = true

These are some things I've tweaked that aren't recognised by neogit .

EDIT: I forgot to include the one that's actually relevant to this issue

[commit]
  template = ~/.gitmessage
TimUntersberger commented 3 years ago

@RianFuro do you have any opinions regarding respecting specific git configurations?

Some things like gitmessage template are a must have imo, but should we also support things like push followTags and whatever (Just like he has configured)? I never changed git defaults so I don't even know what is possible and what each settings does.

RianFuro commented 3 years ago

Yeah same here. I have gpg signing configured so I made sure that still worked when committing, but I must admit I use less of git than I probably should.

On a conceptual level I feel we should support everything at some point; otherwise we'd betray user expectations when they decide to give neogit a try. That being said, since we just call the cli like a normal user would in most cases, things should "just work". For example, I'm interested in why pulling via neogit would ignore the rebase = true setting; maybe the --no-commit arg we add is forcing a merge or something?

Ideally we'd really only have to take care where we deviate from calling git "normally", e.g. our commit workflow, and minimize those places as much as possible.

TimUntersberger commented 3 years ago

Ideally we'd really only have to take care where we deviate from calling git "normally", e.g. our commit workflow, and minimize those places as much as possible.

đź‘Ť

Odie commented 3 years ago

Might want to try setting the HOME env var when launching git. It should help it locate the user’s .gitconfig files.

RianFuro commented 3 years ago

@Odie we already do that: https://github.com/TimUntersberger/neogit/blob/537cc6e1757c41bd75717ebd4421c27b7ebe9205/lua/neogit/process.lua#L25-L38 (and afaik not setting env at all makes spawn use the parent's environment)

Odie commented 3 years ago

Okay. I played around with the code a bit. Here's what I found:

  1. param.env is not usually set As neovim starts up, without even going to the status buffer, neogit fires off about 10 calls to vim.loop.spawn() via neogot/process.lua. Going into the status buffer via ":Neogit" will fire off 1 more. Of these 11 calls, 8 of them had options.env = {}. The remaining 3 of them did not include the options.env field at all.

    This means that the above code @RianFuro referenced, which adds HOME and GNUPGPHOME, never fires, at least for these 11 calls that I saw.

  2. It's unclear if env gets inherited http://docs.libuv.org/en/v1.x/guide/processes.html says this:

    uv_process_options_t.env is a null-terminated array of strings, each of the form VAR=VALUE used to set up the environment variables for the process. Set this to NULL to inherit the environment from the parent (this) process.

    I'm not sure if setting params.env explicitly to nil will cause this to happen. But in any case, params.env is never set to nil, so env inheritance probably won't automatically happen.

  3. Bringing up the commit buffer does not go through spawn or git No calls were made to git using vim.loop.spawn() in neogit/process.lua. So even if $HOME is successfully set, it would not affect how the contents of the commit buffer is prepped.


I'm not familiar with the codebase enough try putting in fixes here. This particular issue can probably be fixed along with #54, by getting git to prep COMMIT_EDITMSG directly.

RianFuro commented 3 years ago

I'm not sure if setting params.env explicitly to nil will cause this to happen. But in any case, params.env is never set to nil, so env inheritance probably won't automatically happen.

It's not that involved really. You don't really have to possibility in lua to distinguish between a field set to nil and a field not set at all; the 2 can be thought of as equivalent. Actually, not even the c-api can distinguish the 2: calling lua_getfield with a key that does not exist on the table still just pushes nil onto the stack. So yes, not setting env should have the child automatically inherit the parent process' environment.

You can "easily" check that by constructing a toy example (template from here, just adapted the called script):

local uv = vim.loop
local stdin = uv.new_pipe(false)
local stdout = uv.new_pipe(false)
local stderr = uv.new_pipe(false)

print("stdin", stdin)
print("stdout", stdout)
print("stderr", stderr)

local handle, pid = uv.spawn("sh", {
  args = {'test.sh'},
  stdio = {stdin, stdout, stderr}
}, function(code, signal) -- on exit
  print("exit code", code)
  print("exit signal", signal)
end)

print("process opened", handle, pid)

uv.read_start(stdout, function(err, data)
  assert(not err, err)
  if data then
    print("stdout chunk", stdout, data)
  else
    print("stdout end", stdout)
  end
end)

uv.read_start(stderr, function(err, data)
  assert(not err, err)
  if data then
    print("stderr chunk", stderr, data)
  else
    print("stderr end", stderr)
  end
end)

uv.write(stdin, "Hello World")

uv.shutdown(stdin, function()
  print("stdin shutdown", stdin)
  uv.close(handle, function()
    print("process closed", handle, pid)
  end)
end)

Is sadly the simplest way to call spawn. test.sh is then simply:

#!/bin/sh
echo $MY_VAR

Executing the above script from neovim (for example with luafile %), passes the environment var down to the script as expected (if set when spawning nvim of course)

Odie commented 3 years ago

I see! Thanks for that code snippet. I tried dumping $HOME both via a shell script and yet another lua script. Both printed out $HOME successfully. x)

So does that mean if neogit just asked git to prep COMMIT_EDITMSG as usual, everything should just work?

Iron-E commented 3 years ago

I'll add this comment here (since I believe this may be related to the parent issue), but sometimes the message showing which files have been staged for commit doesn't update to reflect which files are actually going to be committed. I don't have a repro for this, but I have noticed in using Neogit over the past few days that if you mix committing using the CLI and with Neogit it will not update the status message.

For example, it will say that I am committing file Foo.rs (which was committed previously) when I am actually committing Bar.rs. It will also the old list of unstaged files from the previous commit, as well. Doesn't seem to consistently happen, but it does once in a while.

I think fixing whatever issue this is might also fix that one, since it's all touching similar code (if I have read this issue correctly).

akinsho commented 3 years ago

So I had a look through the mountain of vimscript in vim-fugitive for some hints about how it has managed to solve this. I'm no expert in vimscript but it seems that the config file is being read all over the place and used to get the value of some options.

I'm not sure this is the best solution/why it was needed since a git config also allows a user to do things like [include] paths to other files to be read and a user can also do this conditionally using [includeIf] which means you'd have to resolve these as well.

TimUntersberger commented 3 years ago

I'm not sure this is the best solution/why it was needed since a git config also allows a user to do things like [include] paths to other files to be read and a user can also do this conditionally using [includeIf] which means you'd have to resolve these as well.

Maybe there is an easy way to do this using git config.

Maybe git config --global --get commit.template ?

Iron-E commented 3 years ago

I made a demo for this. Currently only the 'c' action is supported. Not sure if I will make a full PR or not, depends on how time-consuming the rest of the commit actions end up being :P

I notice the reword and amend git texts look a bit different (they mention author name, commit date, etc). Would probably have to parse git --no-pager log output for that