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

Added support for git stash list #1280

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

Conversation

AlphabetsAlphabets
Copy link

@AlphabetsAlphabets AlphabetsAlphabets commented May 6, 2024

Added basic support for git stash list. Current functionality in the :Neogit screen.

  1. Z then l will give a list of stashes.
  2. Pressing ENTER on any of the stashes will bring up a diff for that stash.

self:close()
end,
["<enter>"] = function()
-- Still looking for how to view a stash
Copy link
Member

@CKolkey CKolkey May 7, 2024

Choose a reason for hiding this comment

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

You can use

function M.oid(rev)
(git rev-parse 'stash@{0}') to get the OID from a stash, which can be passed into the buffer constructor.

@@ -219,7 +219,7 @@ M.CommitEntry = Component.new(function(commit, args)
}, graph, { text(" ") }, ref, { text(commit.subject) }),
{
virtual_text = {
{ " ", "Constant" },
{ " ", "Constant" },
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{ " ", "Constant" },
{ " ", "Constant" },

rel_date = cli.log
.max_count(1)
.format("%cr")
.args("stash@{" .. idx .. "}")
Copy link
Member

@CKolkey CKolkey May 7, 2024

Choose a reason for hiding this comment

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

Suggested change
.args("stash@{" .. idx .. "}")
.args(("stash@{%s}"):format(idx))

Not a major thing, but in lua strings are interned, so concatenation operations will re-hash the new string, for every .. operator.

In this case that would be something like:

hash("stash@{")
hash("stash@{1")
hash("stash@{1}")

Many successive re-hashings will have a noticeable performance hit. While it's probable that this list will never be long enough to encounter that, I just prefer to do string formatting or table.concat() to join strings :)

@CKolkey
Copy link
Member

CKolkey commented May 7, 2024

Nice work! Left some feedback

@AlphabetsAlphabets
Copy link
Author

Hey, @CKolkey I fixed all the issues you brought up in the review. I also merged the latest from master and tested the code and all is well. Can you double check to make sure everything is alright?

@AlphabetsAlphabets
Copy link
Author

@CKolkey hey there. Would you mind taking a look at the changes?

@CKolkey
Copy link
Member

CKolkey commented May 25, 2024

It's pretty good - there are still a handful of comments I left that are unresolved though. Take care of those and we'll merge this :)

@ej-shafran
Copy link
Contributor

Hey - is there any movement on this? @AlphabetsAlphabets if you're no longer working on this feature, you mind if I fork off of you and fix the mentioned comments?

@AlphabetsAlphabets
Copy link
Author

Hi, feel free to do so. I'm very busy right now.

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