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
BlameControl: cancellation token for git-blame #9816
BlameControl: cancellation token for git-blame #9816
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 LGTM
Kill() could be used when cancellation is done.
Yes, for later. I would rather give git a chance to clean up by sending a Ctrl+C
signal.
This applies to git log
, too, particularly if the RevisionGrid filters are active.
@@ -340,7 +340,7 @@ private void UpdateSelectedFileViewers(bool force = false) | |||
|
|||
if (tabControl1.SelectedTab == BlameTab) | |||
{ | |||
Blame.LoadBlame(revision, children, fileName, RevisionGrid, BlameTab, Diff.Encoding, force: force); | |||
_ = Blame.LoadBlameAsync(revision, children, fileName, RevisionGrid, BlameTab, Diff.Encoding, force: force, cancellationToken: _viewChangesSequence.Next()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❔ _ =
Why do you explicitly discard the Task
result sometimes and sometimes not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was required in #8914
Will add to FormBlame
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not JTF.RunAsync(() => Blame.LoadBlameAsync(...)).FileAndForget();
or JTF.Run(() => Blame.LoadBlameAsync(...))
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realized that GitUIExtensions.ViewChangesAsync is async Task
changing this too (it should work the same).
The same handling should be used, should ViewChangesAsync be changed too?
@@ -638,7 +638,7 @@ private void blameToolStripMenuItem_Click(object sender, EventArgs e) | |||
} | |||
|
|||
blameToolStripMenuItem.Checked = !blameToolStripMenuItem.Checked; | |||
BlameSelectedFileDiff(); | |||
ShowSelectedFileBlameAsync(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto - why not use JTF?
LGTM |
0173b53
to
fb14952
Compare
squashed and rebased, added one change to clear the file viewer async. Would like to merge this in two days or so. |
18f8c00
to
bef1283
Compare
git-blame may require considerable time, the git executable must be cancelable. Clear the blame viewer when processing a new file. Make the async Clear in FileViewer public. Cancellation token for LoadBlame Missing for RevFileTree (and FileHistory), existing for RevDiff Return the async Task for the Blame.
bef1283
to
4d21470
Compare
Squash merging tomorrow. |
Proposed changes
git-blame may require considerable time, the git executable
must be cancelable.
For instance git-blame of "GitUI/Translation/en_pseudo.xlf" takes about a minute for me.
Clear the blame viewer when processing a new file.
Before the contents of the previous file was not cleared, which was confusing.
(This was not a problem when a separate FileHistory had to be opened for Blame, but is now when Blame can be viewed in FileTree and RevDidd).
Note that cancelling the Git command will not kill the Git process. So clicking around in the file tree can start several long running processes.
Before, they always ran to completion in GitModule, now they are aborted (no exit status and short runtime in command log).
So in some sense cancelling is worse as the command still runs but command cache is not updated if the file is viewed again...
process.Kill() could be used when cancellation is done.
Screenshots
Before
(You can see that the second time English-plugins.xlf is clicked that it is quickly displayed as git-blame ran to completion before the executable was cancelled).
After
Test methodology
Manual
Merge strategy
I agree that the maintainer squash merge this PR (if the commit message is clear).
✒️ I contribute this code under The Developer Certificate of Origin.