Skip to content

Issue #15: add the function to show all blame output. #16

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

Merged
merged 6 commits into from
Nov 7, 2017

Conversation

gh640
Copy link
Contributor

@gh640 gh640 commented Oct 29, 2017

Closes #15

Thank you for the great package. This package helps me a lot.

I'd like to propose an approach to realize the multi-line blame display function.

Here is a sample screenshot.

15-1

@psykzz
Copy link
Collaborator

psykzz commented Oct 29, 2017

I like the approach, currently is the only way to remove the phantom from the right click context menu?
Is it possible to have an on screen icon that can be clicked to make things a little slicker?

Either way im happy to merge regardless of your answer, just let me know.

@gh640
Copy link
Contributor Author

gh640 commented Oct 29, 2017

@psykzz thank you very much for your kind reply :)

currently is the only way to remove the phantom from the right click context menu?

Currently there are 2 ways to remove the phantoms: 1) edit the file, 2) select Git Blame Erase All command. I think 1) is important to prevent a mismatch between the file and the blame result and broken layout.

Is it possible to have an on screen icon that can be clicked to make things a little slicker?

I tried adding x icons with the new commit. Please see the screenshot. (Actually we don't need to put the x icon in all the lines and we can impove it later.)

16-1

@psykzz
Copy link
Collaborator

psykzz commented Oct 29, 2017

Looks good to me, will merge and release later today.

@psykzz
Copy link
Collaborator

psykzz commented Oct 29, 2017

@Tardog wanna check our these changes and screens and see if that helps with your issue #15?

@psykzz
Copy link
Collaborator

psykzz commented Oct 29, 2017

One other thing @gh640 could you update the readme too with the new commands?
If not i can do it after merging.

@gh640
Copy link
Contributor Author

gh640 commented Oct 29, 2017

@psykzz thank you for addressing this proactively.

One other thing @gh640 could you update the readme too with the new commands?

Why not? I tried adding the description for the new function in README.md. I'm happy if you check and review it. Thanks.

@Tardog
Copy link

Tardog commented Oct 29, 2017

@gh640 I checked out your branch and tested the new commands with a couple of files in several projects. For the most part it works well, and I think the layout shows a good amount of information without taking up too much space.

One caveat, though: On very large files (for example, a CSS file with > 1200 lines), the command doesn't appear to do anything. Not sure if this is due to API limits or an issue with this implementation (I didn't have time to look at your code yet).

@gh640
Copy link
Contributor Author

gh640 commented Oct 29, 2017

@Tardog thank you for testing the branch!

One caveat, though: On very large files (for example, a CSS file with > 1200 lines), the command doesn't appear to do anything. Not sure if this is due to API limits or an issue with this implementation (I didn't have time to look at your code yet).

I believe it's caused since I didn't properly consider the leading caret of commit hash (SHA). I fixed it in a new commit.

6c3867a

Also, I found a problem with my PR which breaks the existing function with my mistake of renaming a variable name :( I fixed it in another new commit.

662cf5e

Could you please try the new HEAD? Thank you.

@gh640
Copy link
Contributor Author

gh640 commented Oct 29, 2017

FYI, I noticed that Sublime Text almost freezes if I use this new command for a file with over 10,000 lines in my environment... (This happens with the existing git blame command with over 10,000 selections, too.)

In the new command, the git blame is called only once and is not problematic. The cause is the phantom instances, I think. So it may be better to provide a setting to limit the maximum file size fro the new command to support in a later release.

@psykzz
Copy link
Collaborator

psykzz commented Oct 30, 2017

Ill do another review once @Tardog has had a look into if your changes fixes their issue.

@psykzz psykzz self-requested a review October 30, 2017 01:04
@Tardog
Copy link

Tardog commented Oct 30, 2017

@gh640 Wow, that was fast! I’ve looked at the changes in your latest commits and pulled them for another round of testing. There are still no phantoms showing up in the large CSS file. Ditto for a much smaller (~500 lines) Python file. In order to help debugging this issue a little better, I disabled all other packages (just to make sure I can test your changes in isolation), and looked at the git blame output of the affected files.

There is no leading caret in any of their commit hashes – although it was quick thinking on your part to anticipate this scenario and change the regex accordingly. :) If I find a file with boundary commits, I'm going to make sure to test it as well.

So, what else could be preventing the phantoms from showing up? No errors in the ST console, no slowdown when executing the "Show all" command, and the single-line blame works flawlessly.

I started editing the problematic Python file, removing parts of the code and executing "Git Blame: Show all" after each modification. It quickly turned out that even with only a few lines left in the file, the phantoms were still AWOL. Not a matter of size, then?

After comparing git blame output of the troublesome Python and CSS files, I noticed both have several commits from authors with special characters in their names. I think these are causing the command to fail.

To make this easier for you to reproduce, here is the Python file I used for testing: https://github.com/DragonComputer/Dragonfire/blob/master/dragonfire/sr/experimental.py

Run the command, see it fail.
Remove everything, except for this block of code:

try:
    raw_input          # Python 2
except NameError:
    raw_input = input  # Python 3
try:
    xrange             # Python 2
except NameError:
    xrange = range     # Python 3

Run the command again. Phantoms should pop up now.

Thank you for putting in the effort to get this feature up and running! I'll gladly help and continue testing with as many different files as possible.

@gh640
Copy link
Contributor Author

gh640 commented Oct 30, 2017

@Tardog thank you very much for addressing the problem. Your thorough tests gave me lots of hints!

Firstly I could reproduce the phenomenon with the file you shared:

https://github.com/DragonComputer/Dragonfire/blob/master/dragonfire/sr/experimental.py

I found a cause which made the command fail silently. It's the file name in the blame output. I didn't properly recognize the pattern that blame result has file names.

(As psykzz knows) the file names are displayed in blame result only when the file name was changed in the commit history. I didn't understand that behavior of git blame.

15-3

I adjusted the code a little in the new commit to make it work. The Git Blame Show All command worked well for the above file experimental.py in my environment after the commit.

I'd like you to check and review it out again :)

@gh640
Copy link
Contributor Author

gh640 commented Oct 30, 2017

(I'd like to propose a debug option to make debugging easier in this package later)

@Tardog
Copy link

Tardog commented Oct 30, 2017

(I'd like to propose a debug option to make debugging easier in this package later)

I second this. A debug option would be superb.

With the latest changes I get the expected output for all the files on which the command failed before. Looks good to me!

So far I haven't been able to find a project with boundary commits to test. Can someone help out with this?

@gh640 I could have sworn it was the special characters. :D Turns out I didn't know about the optional file name display in git blame either, so I was able to learn something new today, which is always a good thing.

@gh640
Copy link
Contributor Author

gh640 commented Oct 30, 2017

@Tardog thank you for your swift response! I'm happy to hear that.

So far I haven't been able to find a project with boundary commits to test. Can someone help out with this?

I myself had a file with boundary commits and tested it. The command seems to work well for the file.

15-4

so I was able to learn something new today, which is always a good thing.

Yeah. It was a good learning for me, too :D

As for the debug option, I'll give it a try after we've completed this issue.

Thanks!

@gh640
Copy link
Contributor Author

gh640 commented Nov 5, 2017

@psykzz I'd like you to review the PR when you have a time as Tardog finished to review it. Thank you in advance.

@psykzz psykzz merged commit 03519f9 into frou:master Nov 7, 2017
@psykzz
Copy link
Collaborator

psykzz commented Nov 7, 2017

Merged and released as version 1.1.0. Thanks for all the help guys!

@gh640
Copy link
Contributor Author

gh640 commented Nov 7, 2017

@psykzz @Tardog I upgraded the GitBlame package in my Sublime Text to 1.1.0. It works fine! Thanks!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants