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

feat: commit component #1027

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

Conversation

gzbd
Copy link

@gzbd gzbd commented Apr 27, 2023

Hi,

This PR adds a new component called commit which displays the difference in commits count between the current branch and the upstream or master branch. It's useful to get a visual indication in the editor that some commits are available in origin.

2023-04-27_17-17

This is what the component looks like, next to the branch component. First, with the red color, you can see that there are 2 commits in the master branch that are conflicting, then with the gray color, you see that no commits to pull are available, and lastly, you can see that one commit is not pushed to the current branch remote (no conflict with remote).

To get updates on new commits the component watches some files in the .git/ dir. Also, the git fetch command is run from time to time to get new commits on the remote. Only the current buffer repo is actively watched. Watch on previously discovered repositories (when many buffers from different repositories are loaded) is suspended, and resumed on BufEnter. This approach makes git commands run in the repository (outside of the neovim) to be reflected in the editor.

I took some code and a general approach from the branch component. I think that the find_git_dir function could be shared between those two.

I've been using this component for about a week and looks good. This component, probably will not work on Windows due to the use of sh -c and pipes. I'm not sure if I did everything in the right way in Lua, if not please comment.

@gzbd gzbd marked this pull request as ready for review April 27, 2023 15:20
@shadmansaleh
Copy link
Member

This looks quite interesting. I'm in favor of adding this component in general. There are a decent amount of changes in this pr it'll take me a while to review the whole thing.

@gzbd
Copy link
Author

gzbd commented Jun 21, 2023

Hey, any chance of getting this merged?

@chrisgrieser
Copy link

Just found this via search and bumping this, so it does not go lost amidst all the other open PRs.

This is indeed would indeed be a great feature to have.

@chrisgrieser
Copy link

chrisgrieser commented Oct 17, 2023

@gzbd just gave your fork a try and so far, it looks good to me. Since your fork does not allow issues, and in the hope of taking some burden from shadmansaleh, some feedback on some minor things (only from a user perspective, haven't looked at the code)

  • There is no option to remove the leading git-icon. No other lualine component has a leading icon, so for consistency this should probably be removed or made an opt-in. I tried removing it via fmt, but apparently it's even hard-coded? (Also, the git-icon is a nerdfont, which should not be added by default, since not everyone uses nerdfonts.)
  • The three arrow icons do not align well in multiple fonts I tried. The component could use these arrows, which also work without nerdfont: →↓↑
  • On startup, I briefly get three numbers with the same color before the component loads the correct stats. I think it'd be better if the component should nothing before the correct numbers are calculated.
  • show_only_diverged should by default be true, not false. That would make it consistent with the behavior of the diff component.

@gzbd
Copy link
Author

gzbd commented Oct 19, 2023

@chrisgrieser thanks for the feedback. I'll work on this over the weekend

}

local default_options = {
icon = '',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if user sets icon option to empty string icon would go away

@chrisgrieser

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, you are right. Thanks!

In that case, this is just a case of the icon not being documented in the component options.

lualine_a = {
{
'commit',
master_name = 'master', -- Default master branch name, some repositories use `main`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should mention you're setting a default value for icon option

README.md Outdated
unpushed_icon = '⇡ ', -- Icon shown for unpushed changes on current branch.
use_check_icon = true, -- Use checkmark icon, instead of `0`.
check_icon = '󰸞', -- Icon to display check mark instead of `0`.
show_only_diverged = false, -- Don't show `0` or check mark if up to date.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should be default true

end

function M.get_master_name(cwd, callback)
M._run_job("git remote show origin | grep 'HEAD branch' | cut -d' ' -f5", cwd, function(exit_code, output, _)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not take the output of the command than process it in lua instead of running grep and cut. that way we can drop the pipes and sh -c

end)
end

function M.check_for_conflict(cwd, source, target, callback)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it feels like this might get expensive

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might get expensive when comparing large diffs involving hundreds of commits. The diff operation is done by git itself and it's done in the memory (no disk writes), so I cannot speed this up. One thing that comes to my mind is to add a setting disabling this feature by the users if unwanted.

end)
end

function M.commit_diff(cwd, source, target, callback)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this do same as git rev-list --count --left-right HEAD...@{upstream} ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'll make a change to use this command as it produces less output.


function M.check_for_conflict(cwd, source, target, callback)
local cmd =
string.format([[git merge-tree `git merge-base %s %s` %s %s | grep '<<<<<<<']], source, target, target, source)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here pipes can be removed I think

@chrisgrieser
Copy link

chrisgrieser commented Nov 4, 2023

One issue I noted with the commit component: there seems to be some timeout for git fetch missing.

Yesterday, github was intermittently down, resulting in various git operation failing when called in the terminal. In my nvim, the result was quite peculiar, since I got a lot of errors from various plugins like nvim-lint (which have nothing to do with github) about their jobs failing. The errors stopped at the same time the github issues were fixed?

My guess what happened is that the commit component's job never got resolved, keeping it in a never-ending loop, and with each fetch-interval, there was another job added, it started blocking other plugin's jobs?

edit: false alarm, it was actually the other way round, a bug in nvim-lint created non-terminating jobs that were blocking lualine.

@gzbd
Copy link
Author

gzbd commented Dec 7, 2023

Hey, I applied some changes as requested. Can you take a look?

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

3 participants