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

RevDiff: Request GitStatus updates at file manipulations #8894

Merged
1 commit merged into from Mar 1, 2021

Conversation

gerhardol
Copy link
Member

#8886 (comment)

Proposed changes

Whenever files are manipulated in RevDiff, request GitStatusMonitor refresh

  • If the change was just worktree <-> index, git-status is not triggered at all (until next file system change or every fifth minute)
  • If the change is throttled (GitStatusMonitor increase minimum time between updates #8886 increases the minimal time for updates to 30 s), an interactive request is done (delayed 200 ms to possibly bundle changes)

I considered to request the update only at (likely) worktree <-> index changes, but that got more complicated and a faster update is desired after manual changes.
If you stage files one by one, if may cause git-status to run more or less continously (where Git is slow). I do not believe this is a problem, but the request could be done with some delay too.
@mstv?

Note that FormCommit is not affected, it always rescans (not directly with git-status) when a change is done.

Candidate for 3.5 too

Test methodology

Manual


✒️ I contribute this code under The Developer Certificate of Origin.

@codecov
Copy link

codecov bot commented Feb 26, 2021

Codecov Report

Merging #8894 (b351ba3) into master (2bf1e3c) will increase coverage by 0.01%.
The diff coverage is 41.46%.

@@            Coverage Diff             @@
##           master    #8894      +/-   ##
==========================================
+ Coverage   56.05%   56.07%   +0.01%     
==========================================
  Files         922      923       +1     
  Lines       65861    65881      +20     
  Branches    12084    12087       +3     
==========================================
+ Hits        36920    36941      +21     
- Misses      25924    25934      +10     
+ Partials     3017     3006      -11     
Flag Coverage Δ
production 43.30% <41.46%> (+0.02%) ⬆️
tests 94.83% <ø> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Copy link
Member

@mstv mstv left a comment

Choose a reason for hiding this comment

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

Thank you! Feels much more responsive.

Copy link
Member

@RussKie RussKie left a comment

Choose a reason for hiding this comment

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

👍 with minor suggestions

GitUI/CommandsDialogs/RevisionDiffControl.cs Outdated Show resolved Hide resolved
GitUI/CommandsDialogs/FormBrowse.cs Outdated Show resolved Hide resolved
@RussKie RussKie added the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Feb 28, 2021
@ghost ghost removed the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Feb 28, 2021
@gerhardol
Copy link
Member Author

review comments and a similar changes (few) for revfile, browse. (I ignored editing of .gitignore and similar, there are maybe other situations too that will be affected by edit+edit if #8886 is included).

Tested with patched GitStatusMonitor so it runs continuously, works OK. The UI flashes a little but that is separate.

Will squash for merge in about a day (or at feedback).

Copy link
Member

@mstv mstv left a comment

Choose a reason for hiding this comment

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

👍

@gerhardol
Copy link
Member Author

gerhardol commented Feb 28, 2021

A scenario that is not handled is if you update a submodule in the side panel, without any filesystem changes (for example for a merge commit).
Ignored for now.

For (most) operations related to changes of worktree or index status
in Browse, request immediate git-status update, for the commit count
and artificial commit.
This will bypass the delay between updates. Also changes worktree<->index
would not trigger updates.

Note that operations to submodules in the sidepanel still requires file
system changes.
@gerhardol gerhardol force-pushed the feature/rev-diff-refresh-status branch from 7bf5ff5 to 62c5f21 Compare February 28, 2021 21:24
@gerhardol
Copy link
Member Author

@msftbot merge in 24 hours

@ghost ghost added the status: auto merge label Feb 28, 2021
@ghost
Copy link

ghost commented Feb 28, 2021

Hello @gerhardol!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

  • I won't merge this pull request until after the UTC date Mon, 01 Mar 2021 21:27:02 GMT, which is in 1 day

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

@ghost ghost merged commit 273a0f6 into gitextensions:master Mar 1, 2021
@ghost ghost added this to the 3.6 milestone Mar 1, 2021
@gerhardol gerhardol deleted the feature/rev-diff-refresh-status branch March 1, 2021 21:54
This pull request was closed.
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.

None yet

3 participants