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

Limit the number of commits in completions #3230

Merged
merged 1 commit into from Aug 27, 2016

Conversation

Projects
None yet
6 participants
@gladhorn
Contributor

gladhorn commented Jul 14, 2016

Offering auto completion for existing commits is great, but on big
repositories, it suddenly becomes really slow, even with fast hard
disks, since each commit is read and then a line processed for it.

Instead limit to the last 500 commits (arbitrary number) which still
feels fast. Going back further in history can easily and more reasonably
done with git log etc.

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Jul 14, 2016

Contributor

👍 Seems reasonable to me. Let's let this bake for a couple of days to see if anyone objects.

Contributor

krader1961 commented Jul 14, 2016

👍 Seems reasonable to me. Let's let this bake for a couple of days to see if anyone objects.

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Jul 14, 2016

Member

How do the official bash completions do it (or zsh's best ones)? Have they found a more clever way to narrow the scope of the results returned by git (perhaps in line with excluding revnos that aren't particularly actionable given the context?) I think our git log command is rather broad and perhaps we could do better than chopping it off at 500 for that particular query.

Member

floam commented Jul 14, 2016

How do the official bash completions do it (or zsh's best ones)? Have they found a more clever way to narrow the scope of the results returned by git (perhaps in line with excluding revnos that aren't particularly actionable given the context?) I think our git log command is rather broad and perhaps we could do better than chopping it off at 500 for that particular query.

@gladhorn

This comment has been minimized.

Show comment
Hide comment
@gladhorn

gladhorn Jul 14, 2016

Contributor

I don't think that the official bash completion completes commits - offering the sha1s with the short message is a big luxury that fish offers ;)

Contributor

gladhorn commented Jul 14, 2016

I don't think that the official bash completion completes commits - offering the sha1s with the short message is a big luxury that fish offers ;)

@gladhorn

This comment has been minimized.

Show comment
Hide comment
@gladhorn

gladhorn Jul 14, 2016

Contributor

Of course the 500 is a crazy random number - but it makes is fast enough for my purposes, I imagine it's still slow for people without SSD. And going back that far is of course crazy anyway, then you're better off doing it by other means (git log with search etc).

Contributor

gladhorn commented Jul 14, 2016

Of course the 500 is a crazy random number - but it makes is fast enough for my purposes, I imagine it's still slow for people without SSD. And going back that far is of course crazy anyway, then you're better off doing it by other means (git log with search etc).

@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Jul 14, 2016

Member

Here's the problem I have with this approach: If you enter a part of a commit hash and it's not among those randomly (is it really or does git pick the last 500?) selected, you won't get a completion.

For cherry-pick, I added the __fish_git_possible_commithash function that returns true when you entered at least 3 characters that could be a hash (i.e. are valid hex-chars), and that strikes me to be a better solution - when you entered a sha1, you'll get your sha1 completed.

Though I haven't tested this on huge repos - which one are you testing on, @gladhorn?

Member

faho commented Jul 14, 2016

Here's the problem I have with this approach: If you enter a part of a commit hash and it's not among those randomly (is it really or does git pick the last 500?) selected, you won't get a completion.

For cherry-pick, I added the __fish_git_possible_commithash function that returns true when you entered at least 3 characters that could be a hash (i.e. are valid hex-chars), and that strikes me to be a better solution - when you entered a sha1, you'll get your sha1 completed.

Though I haven't tested this on huge repos - which one are you testing on, @gladhorn?

@gladhorn

This comment has been minimized.

Show comment
Hide comment
@gladhorn

gladhorn Jul 15, 2016

Contributor

My day to day repos are git://code.qt.io/qt/qtbase.git (29396 commits) and other Qt repositories.
What I really like is the current version in context of fixup. Assuming you work on a series of patches and want to change a detail in the first patch. Make sure to have autosquash in your git config. Now create a new commit for the change of the original patch with "git commit --fixup=" and use autocompletion to find the first commit that you want to change (you have the commit short title to help you), press return, run rebase and done.

So in short, listing the most recent commits for me is invaluable.
I see how this conflicts/is alternative to the completion of sha1s that have been typed manually.

Contributor

gladhorn commented Jul 15, 2016

My day to day repos are git://code.qt.io/qt/qtbase.git (29396 commits) and other Qt repositories.
What I really like is the current version in context of fixup. Assuming you work on a series of patches and want to change a detail in the first patch. Make sure to have autosquash in your git config. Now create a new commit for the change of the original patch with "git commit --fixup=" and use autocompletion to find the first commit that you want to change (you have the commit short title to help you), press return, run rebase and done.

So in short, listing the most recent commits for me is invaluable.
I see how this conflicts/is alternative to the completion of sha1s that have been typed manually.

@gladhorn

This comment has been minimized.

Show comment
Hide comment
@gladhorn

gladhorn Jul 15, 2016

Contributor

About the random part: git picks the most recent 500 commits. I'm not sure how it decides among branches. For most people it should return the right commits though :)

Contributor

gladhorn commented Jul 15, 2016

About the random part: git picks the most recent 500 commits. I'm not sure how it decides among branches. For most people it should return the right commits though :)

@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Jul 15, 2016

Member

My day to day repos are git://code.qt.io/qt/qtbase.git (29396 commits) and other Qt repositories.

Neat, that's something I use daily.

I cloned qtbase (which has about 5 times as many commits as fish) and completing git show takes about 3 seconds for me. With "--max-count=5000" (i.e. ten times more than in this PR), it takes about half a second. "500" is imperceptible. My machine is probably on the low end.

@gladhorn: One possibility I've thought about is limiting only for select commands, like the fixup case you mentioned. Because you're not gonna fixup a commit that's a year old, but you might want to show it. Would that make sense to you?

Member

faho commented Jul 15, 2016

My day to day repos are git://code.qt.io/qt/qtbase.git (29396 commits) and other Qt repositories.

Neat, that's something I use daily.

I cloned qtbase (which has about 5 times as many commits as fish) and completing git show takes about 3 seconds for me. With "--max-count=5000" (i.e. ten times more than in this PR), it takes about half a second. "500" is imperceptible. My machine is probably on the low end.

@gladhorn: One possibility I've thought about is limiting only for select commands, like the fixup case you mentioned. Because you're not gonna fixup a commit that's a year old, but you might want to show it. Would that make sense to you?

@gladhorn

This comment has been minimized.

Show comment
Hide comment
@gladhorn

gladhorn Jul 16, 2016

Contributor

Looking at various git commands, it seems not very straight forward to construct something that looks for sha1 and short message at the same time. Therefore I think the current approach is probably quite good for the use case of searching the entire history. If it was only to find messages, git log --grep would be good, for only finding sha1s maybe git rev-parse --disambiguate=sha1 (needs four hex-chars), even though I had trouble getting sensible results with that.

So I completely agree, splitting it into two functions with different goal, or having a parameter to limit the number of commits makes sense. No need to over do it either :)
I can have a look at how the function is used soonish and update this patch.

Contributor

gladhorn commented Jul 16, 2016

Looking at various git commands, it seems not very straight forward to construct something that looks for sha1 and short message at the same time. Therefore I think the current approach is probably quite good for the use case of searching the entire history. If it was only to find messages, git log --grep would be good, for only finding sha1s maybe git rev-parse --disambiguate=sha1 (needs four hex-chars), even though I had trouble getting sensible results with that.

So I completely agree, splitting it into two functions with different goal, or having a parameter to limit the number of commits makes sense. No need to over do it either :)
I can have a look at how the function is used soonish and update this patch.

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Aug 3, 2016

Contributor

@gladhorn, This is starting to smell of neglect. Personally I'm happy with the behavior this change introduces. But then I'm a neophyte git user who might not notice problems this introduces. In your opinion should this be merged? If not, even if you intend to further refine the concept, we should close this particular PR.

Contributor

krader1961 commented Aug 3, 2016

@gladhorn, This is starting to smell of neglect. Personally I'm happy with the behavior this change introduces. But then I'm a neophyte git user who might not notice problems this introduces. In your opinion should this be merged? If not, even if you intend to further refine the concept, we should close this particular PR.

git completion: Limit the number of commits in completions
Offering auto completion for existing commits is great, but on big
repositories, it suddenly becomes really slow, even with fast hard
disks, since each commit is read and then a line processed for it.

Instead limit to the last 500 commits (arbitrary number) which still
feels fast. Going back further in history can easily and more reasonably
done with git log etc.
@gladhorn

This comment has been minimized.

Show comment
Hide comment
@gladhorn

gladhorn Aug 3, 2016

Contributor

Is there any way to sort completions? For fixup, I'd like to return commits in the current branch, and then it's most sensible to return the most recent ones first.

Contributor

gladhorn commented Aug 3, 2016

Is there any way to sort completions? For fixup, I'd like to return commits in the current branch, and then it's most sensible to return the most recent ones first.

@gladhorn

This comment has been minimized.

Show comment
Hide comment
@gladhorn

gladhorn Aug 3, 2016

Contributor

Or rather the opposite - to return a list of completions that is then not sorted later on. Because git returns them in the right order.

Contributor

gladhorn commented Aug 3, 2016

Or rather the opposite - to return a list of completions that is then not sorted later on. Because git returns them in the right order.

@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Aug 3, 2016

Member

Is there any way to sort completions?

@gladhorn: No, see #361.

Member

faho commented Aug 3, 2016

Is there any way to sort completions?

@gladhorn: No, see #361.

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Aug 25, 2016

Contributor

@gladhorn, What's the status of this PR? It's been three weeks since the last update. Should it be merged? If you're still working on this change and have no idea when it will be ready for another review this PR should be closed. You can open a new PR when you're ready for us to take another look.

Contributor

krader1961 commented Aug 25, 2016

@gladhorn, What's the status of this PR? It's been three weeks since the last update. Should it be merged? If you're still working on this change and have no idea when it will be ready for another review this PR should be closed. You can open a new PR when you're ready for us to take another look.

@gladhorn

This comment has been minimized.

Show comment
Hide comment
@gladhorn

gladhorn Aug 26, 2016

Contributor

@krader1961 to me this is still a clear improvement. As long as there is no sorting of completions beyond the default sorting it's probably as good as it gets.

Contributor

gladhorn commented Aug 26, 2016

@krader1961 to me this is still a clear improvement. As long as there is no sorting of completions beyond the default sorting it's probably as good as it gets.

@faho faho merged commit f37995c into fish-shell:master Aug 27, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Aug 27, 2016

Member

@gladhorn: Merged. It'd be nice to figure out how to handle this properly in the general case, but that's gonna take some more work.

Member

faho commented Aug 27, 2016

@gladhorn: Merged. It'd be nice to figure out how to handle this properly in the general case, but that's gonna take some more work.

@lgarron

This comment has been minimized.

Show comment
Hide comment
@lgarron

lgarron Aug 30, 2016

I commented on f37995c#commitcomment-18822542 but it seems the commit is 50, while this issue and the commit description say 500. Not sure which was intended.

(Doesn't make a difference for my problems in #3342 either way.)

lgarron commented Aug 30, 2016

I commented on f37995c#commitcomment-18822542 but it seems the commit is 50, while this issue and the commit description say 500. Not sure which was intended.

(Doesn't make a difference for my problems in #3342 either way.)

@zanchey zanchey modified the milestones: next-2.x, fish-future Aug 30, 2016

faho added a commit that referenced this pull request Aug 30, 2016

git completions: Only show last 1000 commits
This can be prohibitively slow on large repositories (minutes!).

While regrettable, no user is going to like waiting that long.

Work towards #3342, rerun of #3230.

Many thanks to @gladhorn for the idea!

@krader1961 krader1961 modified the milestones: fish 2.4.0, next-2.x Sep 3, 2016

nomaed added a commit to nomaed/fish-shell that referenced this pull request Jul 17, 2017

git completion: Limit the number of commits for --fixup (#3230)
Offering auto completion for existing commits is great, but on big
repositories, it suddenly becomes really slow, even with fast hard
disks, since each commit is read and then a line processed for it.

Instead limit to the last 500 commits (arbitrary number) which still
feels fast. Going back further in history can easily and more reasonably
done with git log etc.

nomaed added a commit to nomaed/fish-shell that referenced this pull request Jul 17, 2017

git completions: Only show last 1000 commits
This can be prohibitively slow on large repositories (minutes!).

While regrettable, no user is going to like waiting that long.

Work towards #3342, rerun of #3230.

Many thanks to @gladhorn for the idea!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment