Blame walk #661

stoivo · 2017-05-30T22:24:21Z

First off, sorry for the big PR. Bit it is a big feature.

I would like to have some review of the code of one other contributor.

Hello, @yyyc514, @benmosher, @stevage, @rchl, @reagle, @dlnsk, @jipumarino, @jipumarino, @jipumarino, @maxim, @zx8
Finally, I got around to implement this. If you are running GitSavvy from git pull down the branch a see if there is something we can improve further?

reagle · 2017-05-31T10:45:30Z

I'm not confident I know how to use it but...

If I hit enter in a row I see options to move blame and, the one's I'm interested in, open commit and show file at commit.

open commit gives:

Traceback (most recent call last):
  File "/Users/reagle/Library/Application Support/Sublime Text 3/Packages/GitSavvy/core/ui_mixins/quick_panel.py", line 59, in on_action_selection
    func(*args, **kwargs)
  File "/Users/reagle/Library/Application Support/Sublime Text 3/Packages/GitSavvy/core/commands/blame.py", line 285, in open_commit
    self.view.window().run_command("gs_show_commit", {"commit_hash": short_hash})
NameError: global name 'short_hash' is not defined

show file at commit results in an untitled and blank tab with the following in the console:

`/usr/bin/git rev-parse --short` failed with following output:

fatal: Needed a single revision

reagle · 2017-05-31T13:22:49Z

ac4215a and 215c08a fix my problem above. But the distinction between (a) "Show file at this commit" and (b) "Show file at this lines commit" is not clear.

(a) shows the file at the most recent commit? And (b) should be "line's".

stoivo · 2017-06-06T06:24:02Z

(a) Show file at commit does open the file in the commit without the blame details. In case you want to read the file with syntax highlighting. Maybe Show file without blame info?

reagle · 2017-06-06T11:36:46Z

I'm not sure what you mean about the blame details. Both options seem to open a version of the file from the past. (a) is HEAD and (b) is the commit corresponding to "this line's commit."

Speaking of syntax highlighting, the resulting tabs that are opened have no syntax highlighting; they are opened as plain text for some reason....?

stoivo · 2017-06-06T11:56:37Z

While you work on it, it is soo clear to you what everything means. It feels so obvious. Now after using it for a week, I understand how confusing it is.

So this pictures will explain it a bit better.

Syntax highlighting, Which language are you working with. It seams to work fine with .py extension.

reagle · 2017-06-06T12:55:41Z

I still don't understand the first distinction, but I am a git novice. Does "show file without blame info" mean show the file at HEAD? What is "blame info"?
I suspect my highlighting problem is related to my use of magicpython. (I have default python disabled). It opens other python files fine, just not here.

stoivo · 2017-06-06T21:26:35Z

The reason why magicpython isn't working because it uses the old version of sublime syntax files. MagicPython is using .tmLanguage while GitSavvy only checks for '.sublime-syntax`.
It is quite easy to fix for MagicPython if they want to. (You can create and issue)
HEAD is just a reference to where you are in the your git commits. When you checkout a branche, HEAD will now point to that branch. One branch is just a pointer to a specific commit, when you commit on this branch you will update the branch and HEAD to point to at the new commit. This new commit then points back to the old commit.
git checkout fix

A - B - D 
      \  C
        ^ master
         ^ fix

HEAD -> C
master -> D
fix -> C

git commit -m"E"

A - B - D 
      \  C - E
        ^ master
             ^ fix

HEAD -> E
master -> D
fix -> E

git checkout master

A - B - D 
      \  C - E
        ^ master
             ^ fix

HEAD -> D
master -> D
fix -> E

git checkout C

A - B - D 
      \  C - E
        ^ master
             ^ fix

HEAD -> C
master -> D
fix -> E

At this stage, you are not on any branch. This is very useful if you want to look at how the code looked 2 weeks ago. If you make a commit when you have checked out a commit, it will be hard to find that commit again.

Hope That explain it a bit better. If you have other questions to git lets take it in the gitter chat. https://gitter.im/divmain/GitSavvy

reagle · 2017-06-06T21:39:40Z

Okay, forgive my mentioning of HEAD, but what do you mean "without blame info." Could you show me what it is supposed to look like with blame info?

1st1 · 2017-06-06T21:39:58Z

@stoivo

It is quite easy to fix for MagicPython if they want to. (You can create and issue)

Unfortunately is's not an "easy fix" for us. .sublime-syntax is a completely different format so it's not going to happen in the foreseeable future.

MagicPython is using .tmLanguage while GitSavvy only checks for '.sublime-syntax`.

Maybe GitSavvy should check for .tmLanguage files too?

reagle · 2017-06-06T21:41:27Z

@stoivo, in MagicStack/MagicPython/issues/86 they say they aren't likely to migrate away from .tmLanguage. Could you detect that in addition to .sublime-syntax?

stoivo · 2017-06-07T09:04:12Z

Could you show me what it is supposed to look like with blame info?

I open a PR for sublime-syntax on MagicStack/MagicPython#86

reagle · 2017-06-07T13:50:39Z

Okay. I would change:

"Show file without blame info" as "Show file at most recent commit"

I think "Show file at this line's commit" is okay, though it is really "at this row" or "where the cursor is". For a while I thought "show file at this line's commit" meant that when it opened that version, it would automatically go to those line numbers, which would still be a neat feature.

reagle · 2017-06-07T16:13:02Z

New thought for the second: "Show file at this chunk's commit"

stoivo · 2017-06-07T21:38:07Z

@reagle, Thanks for naming commands with me. It is appreciated.

I added one more feature. When to select show file. it jumps to the line you were at. And fixed some other bug when a file is renamed.

reagle · 2017-06-08T10:30:16Z

Looks great and the goto line is indeed handy.

reagle · 2017-06-08T10:35:04Z

BTW: I think the blame tab should be opened read-only, like the GRAPH tab.

randy3k · 2017-06-10T10:57:06Z

core/ui_mixins/quick_panel.py

@@ -157,6 +157,7 @@ class PaginatedPanel:
    limit = 6000
    selected_index = None
    on_highlight = None
+    is_this_selected = None


If I understand correctly, we need this because the index number of the selected item would be unknown if a generator is used. Instead of adding one more argument here, I suggest using the existing variable selected_index.

We could check if it is a callable. If it is, just do

if selected_index(entry): bla bla bla

Yes, I squashed into 514b317, Blame option: Pick a new commit, if you are on a commit, set it as selected.

randy3k · 2017-06-10T10:58:22Z

core/commands/blame.py

+        self.view.run_command("gs_blame_initialize_view")
+
+
+class BlameCommitPanel(PaginatedPanel):


Would it make more sense to extend it from LogPanel?

Not really. The only thing in common is format_item.

But one could argue that LogPanel and PaginatedPanel are only differed by format_item. The on_selection is almost identical.

Also, BlameCommitPanel and LogPanel are both history-related. I don't see why we shouldn't extend it from LogPanel.

randy3k · 2017-06-10T11:02:52Z

core/git_mixins/history.py

@@ -63,12 +65,13 @@ def log(self, author=None, branch=None, file_path=None, start_end=None, cherry=N

        return entries

-    def commit_generator(self, limit = 6000):
+    def commit_generator(self, limit = 6000, follow=False):


This is related to an earlier refactoring, should we actually pass the arguments branch and file_path?

Yes. Generally, I prefer to pass several arguments then to set instance variables. On log it looks a little ugly, but it is very nice and explicit what's all the arguments.

One less important point, this function returns a generator of log entries rather that commits. Maybe log_generator is a better name for this?

log_generator seem good since it if calls to log.

randy3k · 2017-06-10T11:32:12Z

core/commands/blame.py

+            "commit_hash": commit_hash,
+            "filepath": self.file_path,
+            "lineno": self.find_lineno(),
+            "lang" : self.view.settings().get('git_savvy.original_syntax', None)


Nice, any idea to patch the diff view? I don't think the same fix could be applied to diff view.

I think is is already handled https://github.com/divmain/GitSavvy/blob/53b878a5891f15b90f8d796c34c16c44639d0f5e/core/commands/inline_diff.py#L40-L49

Or which diff did you think of?

I am talking about https://github.com/divmain/GitSavvy/blob/53b878a5891f15b90f8d796c34c16c44639d0f5e/core/interfaces/status.py#L261
and
https://github.com/divmain/GitSavvy/blob/53b878a5891f15b90f8d796c34c16c44639d0f5e/core/commands/diff.py#L277

In these functions, the file syntax is detected using the util function but not the default syntax.

I can't think of any way to get that without opening the file and read the syntax, then close it. 👎

We could open the file in the output panel to make it invisible to users.

PS: it turns out we cannot open file directly in the output panel.
https://stackoverflow.com/questions/37905081/open-a-file-in-output-panel

We could also capture the scopes of the file in the output panel to highlight the blame view.

https://gist.github.com/mattst/71488757f2a2ca573c2d5e73af636d4c#file-getfileextensionsyntax-py

Yes but this can be a separate PR.

randy3k · 2017-06-10T17:04:47Z

core/git_mixins/history.py

@@ -63,12 +65,13 @@ def log(self, author=None, branch=None, file_path=None, start_end=None, cherry=N

        return entries

-    def commit_generator(self, limit = 6000):
+    def commit_generator(self, file_path, branch=None, limit=6000, follow=False):


Is there any reason to switch the order of file_path and branch?

Perhaps something like this?

def log_generator(self, limit=6000, **kwargs): # Generator for show_log_panel skip = 0 while True: logs = self.log(limit=limit, skip=skip, **kwargs) if not logs: break for l in logs: yield l skip = skip + limit

randy3k · 2017-06-10T17:44:50Z

core/commands/log.py

-        self._file_path = file_path
-        self._branch = branch
-        sublime.set_timeout_async(self.run_async)
+        sublime.set_timeout_async(lambda: self.run_async(file_path, branch), 0)



These changes are not necessary per se. But they do not hurt.

I have to disagree, the change does hurt, multiple commands get broken. Specifically, any command inheriting from LogMixin that overrides run_async and does not support the 2 new arguments.

I'm going to patch the occurrences I found so far, to support this cleaner call type: GsLogByAuthorCommand, GsLogByBranchCommand, GsResetCommand and GsBranchesLogCommand . See commit 0776af2

Unfortunately this patch did not fix the issue I was seeing in "reset to branch" command, and I have run out of time. Hope you guys can help me again 😅

Just answering my own question, as in commit message: implicitly passing more arguments into kwargs raises an issue in python3, as seen here: http://www.asmeurer.com/python3-presentation/slides.html#15

I have also noticed that reset commands are not working as they should. Thanks for testing

@asfaltboy you are absolutely right. 👍

reagle · 2017-06-10T19:08:12Z

BTW: "hunk" is already a term in git, so maybe we should use that instead of chunk.

stoivo · 2017-06-10T19:16:56Z

but I think of hunk as a park of the diff, and nothing else. I would stick with chunk

randy3k · 2017-06-10T20:28:00Z

We also need a help pop up for blame view: https://github.com/divmain/GitSavvy/blob/master/popups/diff_view.html

Some more keybinds to navigate between commits should also be considered.

stoivo · 2017-06-10T22:52:31Z

Should , and . go one commit forth and back based on the cursor or just jump one commit forward and backward compared to the commit?

randy3k · 2017-06-11T00:01:15Z

I would say neither. Perhaps , and . to go up and down between chunks? > and < nagivate between commits of the current file. Ctrl + > and < move btw commits based on current chunk.

…file When always opening the file with the original syntax if the file extension changes it will be go wrong. I don't think thats a big deal. We can't always find the correct syntax files since some packages uses tmLanguage like https://github.com/MagicStack/MagicPython

… variables

I wanted to use the commit description but the syntax does not give it a scope and it's simpler to to use the commit hash. I think it as good to use commit sha as commit message. Maybe even better.

Blame popup option 2 sqash me Fix typoes

asfaltboy · 2017-06-17T08:12:05Z

@stoivo @randy3k amazing work guys! I'm trying to add vintagious friendly mode to the blame command, at least for navigating hunks, and for some reason it's not working for me (see last commit 029e687). Any idea what I'm doing wrong?

stoivo · 2017-06-17T09:49:22Z

core/commands/blame.py

@@ -71,8 +72,7 @@ def picked_commit(self, commit_hash):
        super().run()


-
-class GsBlameInitializeViewCommand(TextCommand, GitCommand):
+class GsBlameInitializeViewCommand(GsHandleVintageousCommand, TextCommand, GitCommand):


I am sorry, thought of vintage support but apparently forgot. Sorry.
I think the issue is that either you need to call
view.run_command("gs_handle_vintageous") on line 65
Then you won't need to inherit it here.

This is who it's done in inline diff core/commands/inline_diff.py#L84-L85

stoivo · 2017-06-17T18:07:32Z

@asfaltboy, this should do the trick.

NOTE: I'm not sure whether this was added in python 3, but passing 3 positional arguments when method expects 1 pos and 2 kwargs, seems to cause an issue...

stoivo · 2017-06-18T20:43:34Z

Hi, I just want to tell you that I will leave for vacation on Wednesday next week, and will probably not reply or make any updated for the coming 5 weeks.

divmain · 2017-06-20T06:53:56Z

@stoivo, what needs to be done for this to be merged? If documentation is all that is left, we can document the remaining work in a separate issue, so that this can be merged prior to your vacation. Also, have fun!

asfaltboy

@divmain currently there's a bit of a regression going on on this branch. I've details these in a comment that's currently shown below as outdated. However, my last [WIP] commit 0776af2 doesn't fix regression entirely.

asfaltboy · 2017-06-18T17:32:14Z

core/commands/log.py

-        self._file_path = file_path
-        self._branch = branch
-        sublime.set_timeout_async(self.run_async)
+        sublime.set_timeout_async(lambda: self.run_async(file_path, branch), 0)



I have to disagree, the change does hurt, multiple commands get broken. Specifically, any command inheriting from LogMixin that overrides run_async and does not support the 2 new arguments.

I'm going to patch the occurrences I found so far, to support this cleaner call type: GsLogByAuthorCommand, GsLogByBranchCommand, GsResetCommand and GsBranchesLogCommand . See commit 0776af2

Unfortunately this patch did not fix the issue I was seeing in "reset to branch" command, and I have run out of time. Hope you guys can help me again 😅

asfaltboy · 2017-06-19T16:40:28Z

core/commands/log.py

-        self._file_path = file_path
-        self._branch = branch
-        sublime.set_timeout_async(self.run_async)
+        sublime.set_timeout_async(lambda: self.run_async(file_path, branch), 0)



Just answering my own question, as in commit message: implicitly passing more arguments into kwargs raises an issue in python3, as seen here: http://www.asmeurer.com/python3-presentation/slides.html#15

stoivo · 2017-06-23T13:12:53Z

@asfaltboy, I fixed the reset.

Can you pull it and see if you can find any other bug?

asfaltboy · 2017-06-24T10:11:19Z

Reset to branch works now, I'll keep using the branch for a while to see if anything else creeped in

divmain · 2017-06-29T17:09:05Z

I pulled this down and it looks great to me. How do you feel about merging into master and resolving any issues that pop up there? This is a big branch, and my preference is to merge early in order to avoid merge conflicts and subtle errors from master/branch divergence. @stoivo @asfaltboy?

asfaltboy · 2017-06-29T17:10:02Z

@divmain agreed, good point!

stoivo · 2017-06-30T21:31:48Z

I would like to merge

divmain · 2017-06-30T21:35:54Z

Thanks a ton @stoivo! This is merged :)

asfaltboy · 2017-07-01T06:40:45Z

This also needs an update of the relevant docs.

reagle mentioned this pull request Jun 6, 2017

MagicPython not compatible with GitSavvy because of old syntax file? MagicStack/MagicPython#86

Closed

stoivo force-pushed the blame_walk branch from b3c6c3d to f627e41 Compare June 7, 2017 21:34

randy3k reviewed Jun 10, 2017

View reviewed changes

stoivo force-pushed the blame_walk branch 2 times, most recently from 1a2c0b6 to 0f1c2e7 Compare June 10, 2017 16:41

randy3k reviewed Jun 10, 2017

View reviewed changes

stoivo force-pushed the blame_walk branch from 0f1c2e7 to 47d4228 Compare June 10, 2017 19:13

stoivo added 7 commits June 17, 2017 09:20

Refactor: commit_generator -> log_generator does not use any instance…

77d93be

… variables

Refactor: Switch BlameCommitPanel to extend LogPanel

dad98ee

Blame view: , and . to navigate the view

6b5570b

I wanted to use the commit description but the syntax does not give it a scope and it's simpler to to use the commit hash. I think it as good to use commit sha as commit message. Maybe even better.

Blame option: Navigate between checked out commits

1e7145d

Blame option: ? to open help details

28a1b15

Blame popup option 2 sqash me Fix typoes

Blame: Documentation

49ce7bd

stoivo force-pushed the blame_walk branch from 6a752e9 to 49ce7bd Compare June 17, 2017 07:20

stoivo commented Jun 17, 2017

View reviewed changes

stoivo force-pushed the blame_walk branch from 029e687 to 21fe142 Compare June 17, 2017 18:06

Feature: make blame view vintageous friendly

77cf14c

stoivo force-pushed the blame_walk branch from 21fe142 to 77cf14c Compare June 17, 2017 18:07

[WIP] Blame regression: run_async now expects 3 arguments or kwargs

0776af2

NOTE: I'm not sure whether this was added in python 3, but passing 3 positional arguments when method expects 1 pos and 2 kwargs, seems to cause an issue...

asfaltboy requested changes Jun 20, 2017

View reviewed changes

[WIP] Blame regression: run_async now expects 3 arguments or kwargs

e7a92e9

asfaltboy approved these changes Jun 29, 2017

View reviewed changes

divmain merged commit 03b112b into master Jun 30, 2017

divmain deleted the blame_walk branch June 30, 2017 21:35

randy3k mentioned this pull request Jul 18, 2017

Git blame line walker #152

Closed

		self.view.run_command("gs_blame_initialize_view")


		class BlameCommitPanel(PaginatedPanel):

Blame walk #661

Blame walk #661

Conversation

stoivo commented May 30, 2017

reagle commented May 31, 2017

reagle commented May 31, 2017

stoivo commented Jun 6, 2017

reagle commented Jun 6, 2017

stoivo commented Jun 6, 2017

reagle commented Jun 6, 2017

stoivo commented Jun 6, 2017

reagle commented Jun 6, 2017

1st1 commented Jun 6, 2017

reagle commented Jun 6, 2017 • edited Loading

stoivo commented Jun 7, 2017

reagle commented Jun 7, 2017

reagle commented Jun 7, 2017

stoivo commented Jun 7, 2017

reagle commented Jun 8, 2017

reagle commented Jun 8, 2017

randy3k Jun 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

randy3k Jun 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

randy3k Jun 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

randy3k Jun 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

randy3k Jun 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reagle commented Jun 10, 2017

stoivo commented Jun 10, 2017 • edited Loading

randy3k commented Jun 10, 2017

stoivo commented Jun 10, 2017

randy3k commented Jun 11, 2017 • edited Loading

asfaltboy commented Jun 17, 2017

Choose a reason for hiding this comment

stoivo commented Jun 17, 2017

stoivo commented Jun 18, 2017

divmain commented Jun 20, 2017

asfaltboy left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stoivo commented Jun 23, 2017

asfaltboy commented Jun 24, 2017

divmain commented Jun 29, 2017

asfaltboy commented Jun 29, 2017

stoivo commented Jun 30, 2017

divmain commented Jun 30, 2017

asfaltboy commented Jul 1, 2017

reagle commented Jun 6, 2017 •

edited

Loading

randy3k Jun 10, 2017 •

edited

Loading

randy3k Jun 10, 2017 •

edited

Loading

randy3k Jun 11, 2017 •

edited

Loading

randy3k Jun 10, 2017 •

edited

Loading

randy3k Jun 10, 2017 •

edited

Loading

stoivo commented Jun 10, 2017 •

edited

Loading

randy3k commented Jun 11, 2017 •

edited

Loading

asfaltboy left a comment •

edited

Loading