1

I am learning vimscript, and below the part of my .vimrc used to display the current git branch in the statusline. I am aware that there are ready-made options out there, but this is more of an educational exercise. The purpose of this question is to solicit advice on improving the code.

The only arbitrary requirement of this exercise is that the git branch has to be obtained using a system call (instead of directly through the .git directory).

function! StatuslineGitBranch()
    if exists("g:git_branch")
        return g:git_branch
    else
        return ''
    endif
endfunction

function! GetGitBranch()
    let l:is_git_dir = system('echo -n $(git rev-parse --is-inside-work-tree)')
    let g:git_branch = l:is_git_dir == 'true' ?
        \ system('bash -c "echo -n $(git rev-parse --abbrev-ref HEAD)"') : ''
endfunction

autocmd BufEnter * call GetGitBranch()

set laststatus=2
set statusline=
set statusline+=%{StatuslineGitBranch()}

Reason for the existence of g:git_branch, and the slightly convoluted execution: so that statusline does not involve a system call (reference).

How can the above be improved? I'm new to this, so comments will be very helpful.

  • 2
    relevant meta: vi.meta.stackexchange.com/q/1482/10604 – D. Ben Knoble Jul 7 at 22:13
  • @D.BenKnoble You mentioned that the title somewhat misleading (I agree). Can you suggest a better title? – Flux Jul 8 at 2:39
  • 1
    Comments will be helpful? How about a comment about comments? Or lack thereof. This is a short and straightforward bit of code so it's not a DEFCON 1 situation but it did pop a little red flag for me (experienced developer). If you share your code commenting it is critical but even if you never share at the very least you could be doing yourself a favor when you look back at your stuff in the future. (The older you get the truer that last part gets.) – B Layer Jul 8 at 5:34
  • 2
    Should you consider using a per-buffer variable (b:git_branch) instead of a global one? Technically, you could.have buffers opening files from different repos, with different branches checked out... – filbranden Jul 8 at 7:45
5

The most general improvements I can give are to avoid the long ?: and to make use of the get function on dictionaries.

For example, I would write

return get(g:, 'git_branch', '')

For the if, use a plain if

let l:is_git_dir = trim(system('git rev-parse --in-inside-work-tree'))
if l:is_git_dir is# 'true'
  let g:git_branch = trim(system('git rev-parse --abbrev-ref HEAD'))
else
  let g:git_branch = ''
endif

(Note that I've upgraded your test to use ==# for casing and the system call to be more portable: system() already uses 'shell', so no need to wrap that. Invoke git more or less directly. Also, echo -n is unreliable, and there is never a reason to echo $(command) when you could just command.)

Or, encode your data as data:

let l:is_git_dir = trim(system('git rev-parse --in-inside-work-tree'))
let g:git_branch = get(
        \ { 'true': trim(system('git rev-parse --abbrev-ref HEAD')) },
        \ l:is_git_dir,
        \ '')

Another note is that you're performing two system calls on every BufEnter; this may or may not be acceptable in terms of performance (measure!!!).

Finally, I'll add that your best bet is to dig into the ever-popular fugitive plugin and try to understand how its statusline function is constructed.

  • 1
    Thank you. I believe fugitive looks at the files in .git to find the branch name instead of using system. Other than fugitive, there is also vim-gitbranch. – Flux Jul 8 at 2:43
4

0- +1 to D. Ben Knoble suggestions.


Regarding pure vim-scripting

1- Never define autocommands globally. Prefer to define them in their own group, and when filling the group, start by clearing it. This way, the related snippet of code can be re-executed, which you'll will want when you'll be working on your script

aug MyGitBranch
  au!
  au BufEnter * call s:GetGitBranch()
aug END

2- Avoid globally defined functions. Global functions may be defined/changed/removed by any other piece of code. Either define them as script local functions (with the s: prefix as I've done above), or as autoloaded functions if you need to export them (i.e. if you're defining a library plugin), or if they can be lazily loaded.

Regarding the specific case of statusline, I don't remember if we can use script-local functions.

3- Also, it's better to annotate functions with the abort keyword. In case an error is triggered (even if this is impossible on the current code, problems may be introduced on different flavours of Vim (the suggested trim() is quite recent for instance), or in later versions of your plugin.

abort ensure the function doesn't continue (this should be the default, but this is not how Vim used to work, and backward compatibility has been chosen here), and it'll also help decoding possible error messages.

4- I'd use finddir() to recursively find the upstream .git/ directory instead of using system(). On user saturated machines, it can make the difference -- I've experienced such shared environments, and I've come to the conclusion it's always better to reduce the dependence of the system. Also, on Windows machines, calls to system() are really annoying and inefficient.


Otherwise, I not sure we can really avoid checking the current branch every time we switch the current buffer. If a git checkout is operated, we'll want to know the branch has changed. I don't know if there is a smart way to do it.

  • 1
    The reason for git rev-parse is that .git may not be in the project hierarchy. Worktrees and one of the git env variables (GIT_DIR maybe?) screw with this assumption. – D. Ben Knoble Jul 10 at 3:40
  • Regarding the specific case of statusline, I don't remember if we can use script-local functions. A late comment: that's not directly possible, as <SNR> must be resolved first. But the following will work: let &statusline .= '%{' . get(function("s:some_local_func"), "name") . '()}' Although I don't see any reason to prefer this over an autoloaded function. – Matt 2 days ago

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.