Skip to content
New issue

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

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

Already on GitHub? # to your account

[experimental] feat(commands): git-log picker #4984

Closed
wants to merge 1 commit into from

Conversation

matoous
Copy link
Contributor

@matoous matoous commented Dec 3, 2022

[experimental, wip]

Add git-log command that opens picker with git commits and their preview.

@Proful
Copy link

Proful commented Dec 3, 2022

Can I help in testing it as you progress?

@matoous
Copy link
Contributor Author

matoous commented Dec 3, 2022

@Proful yes please, feel free to. This should already be functioning as it is to some extend but the implementation is in very rough early state. I will mostly want/need input from @pascalkuthe (if you find some time for me please, mostly architectural questions) and also importantly, I would like to know how far we want to go with vcs/git integration and what should be left to plugins eventually (cc @archseer).

I think this will need split into 3 PRs: one for picker refactor/new kind of picker, one for VCS registry refactor, and one for this log picker itself. I would also like to keep this PR as small as possible so I think the scope of this PR would could be kept at commit preview - without picker action, etc. which could be implemented in follow-up PRs.

@Proful
Copy link

Proful commented Dec 3, 2022

Looking great!!

image

Minor observation:-

  • Commit picker started as soon as :git-log typed. It should appear after 'enter' pressed
  • Bcoz of above tab key is not going to change-current-directory
  • Loading of all the git log message is very fast

The below suggestion is too early. But it would be great to see something like this in future

image

@pascalkuthe pascalkuthe added the A-helix-term Area: Helix term improvements label Dec 3, 2022
@pascalkuthe pascalkuthe self-assigned this Dec 3, 2022
@pascalkuthe pascalkuthe added S-waiting-on-review Status: Awaiting review from a maintainer. A-vcs Area: Version control system interaction labels Dec 3, 2022
@pascalkuthe
Copy link
Member

pascalkuthe commented Dec 4, 2022

A commit picker will require a more sophisticated design than the approach in this PR.

This PR walks all ancestor which can quickly become quite slow for larger repos.
In fact walking all ancestors is the main bottleneck during object counting.
Servers use special file caches to keep clones reasonably fast (which will be the subject of my future work on gitoxide) however these caches are usually not generated for normal git usage.
That means that this implementation is too slow for larger repos.
For example trying this PR with the linux kernel freezes/crashes the editor.

The reason normal git log is fast is that it paginates the history and only transverses the commit graph as far as you scroll.
However this is not possible with the picker that currently requires all elements as a Vec.
This is not just an API limitation but requirde to perform a fuzzy search on the elements.

Herein lies another problem. This PR fuzzy matches on all commits in a repo on every keypress (and blocks the UI on that). Just for reference a git revparse which does a regex search of all commits (which is faster then fuzzy matching) on the linux kernel takes 10s. That is too slow to perform on every keypress in the picker.

A workaround for a git log like UI could be to paginate the transversal and keep only a fixed number like 1k commits in the picker and keep the transversal around and refresh the picker once you scroll past the end. This might build on work from #3110 (although it's different as it relates to scrolling).

For fuzzy matching another shortcoming of this PR is that is sorts the matching by their fuzzy match score.
That is good for a picker but I think for a history view this actually loses a lot of information. Instead I would retain the original sorting and only filter commits. When a search exhausts the existing pagiated commits we could asynchrounsly transvers further and populate the picker with more results (alltough that last part is likely far of in the future and not required for an initial implementation).

This brings me to the next thing. While I like the basic idea of reusing the picker UI what you implemented here has no actual usecase besides looking pretty. The same thing could be achieved by just opening gitui or git log in a terminal.
The value you pick is never used anywhere. I can definitely think of a few usecases for this. The primary one being that I want to allow users to change the commit the diff-gutter diff is performed against (for example set it to master for a codereview). However for that I don't want just pick from history as I might want to select a different branch.
Furthermore here we would want to use revparse (to allow things like HEAD, HEAD^ or master). I can imagine a global search like variant where we revparse first and if the revparse returns multiple results (like from a regex`) we then open a picker to fuzzy select one commit from the results. This picker would function more like other pickers (so sorted by match score instead of history).

I can still think scrolling the history instead could be useful tough when you do not know what you are looking for exactly.
Perhaphs we would have two commands one that picks from the history and one that acts as described above.
So I would imagine a general commit picker that can be configured either way.

That brings me to UI. When I tried this PR the picker was first populated with a printout of the commit however when I started typing it got populated with the hashes instead. This is disorienting the picker contents should always remain the same. Furthermore I think the UI could be improved. I think we could take inspiration from gitui here:
image

For helix I think the picker should be more condensed. Perhaps <HASH> <AUTHOR> <MESSAGE> with more aggressive truncation for the author to avoid this much empty space.
Ideally we extend the picker so a fuzzy match can not jump between the three different fields here (so sort of a picker with separate fields).

You choose something that is very challenging to implement (and doesn't quite have a use-case in helix yet). I would love to see this implemented well

@pascalkuthe pascalkuthe added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from a maintainer. labels Dec 4, 2022
@David-Else
Copy link
Contributor

I would like to know how far we want to go with vcs/git integration and what should be left to plugins eventually (cc @archseer).

I think LazyGit is the ultimate UI for git in the terminal and there is no point in trying to compete with it. You can use a terminal with window splitting and open it next to Helix, job done.

Screenshot from 2022-12-04 16-26-05

There are some git integrations that would really work in Helix and can't be done in another window, for example the git gutters. I don't think there is any need to add a lot of git integration other than what can't be achieved with other tools.

There is already a discussion on this subject: #4193

I think these are the two most popular TUI git tools:

https://github.com/jesseduffield/lazygit
https://github.com/extrawurst/gitui

@pascalkuthe
Copy link
Member

I think LazyGit is the ultimate UI for git in the terminal and there is no point in trying to compete with it. You can use a terminal with window splitting and open it next to Helix, job done.

As I mentioned above integrating a history just for the sake of showing the history is not really useful. However for dif/diff-gutter I want to change the commit we are comparing against with a command for which I do think a picker would be very useful. Doing this outside of helix and then copying the commit hash into a command is theoretically possible (using fzf in the terminal for fuzzy search or some gitui for looking at the history) but that argument can be made for many pickers (for the file picker you could use fzf to select a file on the terminal instead and copy the path into the :open command). So I do think a commit picker has merit inside helix (and I think a history based one could be a useful alternative to a direct search).

@univerz
Copy link

univerz commented Dec 4, 2022

I think LazyGit is the ultimate UI for git

diff without proper syntax highlighting on that screenshot looks suboptimal to me.

@David-Else
Copy link
Contributor

I think LazyGit is the ultimate UI for git

diff without proper syntax highlighting on that screenshot looks suboptimal to me.

LazyGit optionally uses Delta which lets you choose your own theme for syntax highlighting based on bat. I am using the VS Code one, it is not very close, but others should be better.

@univerz
Copy link

univerz commented Dec 4, 2022

based on bat

i could probably configure it somewhere close to my custom helix theme, but the point is - we are diffing (and staging!) code, we should see it as is most natural == same as we see it in our editor of choice, ideally in the editor - this is what i call ultimate UI.

@jvoisin
Copy link

jvoisin commented Dec 5, 2022

I'm unsure about re-implementing magit into helix, it's better suited as a plugin, or we'll end up with mercurial/svn/darcs/… there in it as well.

@pascalkuthe
Copy link
Member

pascalkuthe commented Dec 5, 2022

I'm unsure about re-implementing magit into helix, it's better suited as a plugin, or we'll end up with mercurial/svn/darcs/… there in it as well.

This PR is not about reimplementing magit. I get that the discussion is sort of drifting in that direction but I don't think there is anything to be gained from that kind of general discussion on a PR. It just ends in endless back and fourth.

I believe we should evaluate every feature on it's own and determine if it's something we want in helix. The main merit is here:

  • Can it only be done in the editor (like git gutters/side by side diffs)/does it interact closely with the main/edit feedback loop
  • Or does it significantly improve the ergonomics of features that fit the previous cirterion

In my opinion a commit picker will be useful for side-by-side diffs to make selecting the commit we diff against ergonomic. Offering an (optional) linear history mode for that picker would be a good convenience feature so I do think this PR would fit criterion 2 (although not in it's current form see my comment above)

@matoous matoous mentioned this pull request Dec 26, 2022
@matoous
Copy link
Contributor Author

matoous commented Dec 26, 2022

I am turning this into and issue: #5292, let's continue this conversation there. This PR is as far from ready as it gets (as mentioned by @pascalkuthe) and re-opining a new PR at later point once we are closer to being able to support the commit picker seems ineviatble.

@matoous matoous closed this Dec 26, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A-helix-term Area: Helix term improvements A-vcs Area: Version control system interaction S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants