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(git): git_commits shows the current branch graph #2288

Merged
merged 2 commits into from
Dec 28, 2022
Merged

feat(git): git_commits shows the current branch graph #2288

merged 2 commits into from
Dec 28, 2022

Conversation

CyanJoeng
Copy link
Contributor

Description

I'd like to see a graph view in git_commits picker, and notice the closed issue (#1041) when I try to find some solutions.
I'm not familiar with the Lua language, so, a little modifications are written to achieve the basic demand.

Feat # (#1041)
show the current branch graph when viewing git_commits

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Invoking the git picker function builtin.git_commits by a vim key mapping, the the commits log with graph will displayed in the popup window.

NB. commit logs showing with graph will insert extra entries with no SHA value, so a error msg will shown in the vim msg bar if these extra entries selected.

@kkharji
Copy link
Member

kkharji commented Dec 27, 2022

I gave it quick test. It showed a bit of graph on telescope. However, tags/HEAD no longer showing on the commits.

Could you please make it optional change. Thanks

btw: #1041 example is amazing. Wish if this can be produced or configured here. The date thing is pretty useful

@kkharji kkharji self-requested a review December 27, 2022 15:48
@CyanJoeng
Copy link
Contributor Author

CyanJoeng commented Dec 27, 2022

Ops. Sorry for that mistake, and the new commit added the tags/HEAD.

the #1041 example is pretty cool, and I am trying to add colours to get a nicer display.

Cheers 🍻

Copy link
Member

@kkharji kkharji left a comment

Choose a reason for hiding this comment

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

LGTM. Indeed I can see the tags/HEAD now.

Please fix the CI failing actions so we can merge.

Any additional enhancement can be done in another pr (tag me)

Comment on lines +439 to 444
msg = ""
end

if not msg then
sha = entry
msg = "<empty commit message>"
end
Copy link
Member

Choose a reason for hiding this comment

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

won't not msg be always false?

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 the check makes sense. its nil (and we enter this branch) if we match a line that has no commit message (its nil because of string.match) but its and empty string if we dont even have a sha, so we do not enter this branch

Copy link
Member

@Conni2461 Conni2461 left a comment

Choose a reason for hiding this comment

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

Thanks :)

LGTM

@kkharji feel free to merge, if your last comment is resolved and sorry about the broken CI, i merged something styled wrong into master a week ago and was to lazy to resolve it ^^

Edit: I just rebased the branch, you are still author of the commits :)

@kkharji kkharji changed the title feat: show the current branch graph when viewing git_commits feat(git): git_commits shows the current branch graph Dec 28, 2022
@kkharji kkharji merged commit 01d9228 into nvim-telescope:master Dec 28, 2022
@SebTalbot
Copy link

This should be optional. It breaks custom git_commands unless we use --graph which forces us to include it.

Also, the help documentation isn't updated and it was pretty confusing.

@petobens
Copy link
Contributor

@kkharji @CyanJoeng for some reason when using git_bcommits I'm seeings lots of empty lines with ... (see image below where I compare telescope output with git-terminal output). Can those lines be suppressed?

Also @SebTalbot is right, documentations need a revamp.
Thank in advance!

image

This was referenced Jan 31, 2023
@Conni2461
Copy link
Member

There is a proposed solution in #2357 which makes things even worse (see #2357 (comment)), so i decided to revert. sorry for the inconvenience, i should have caught and this should have never been merged

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.

5 participants