Commit count: Listing ignored files could hang GE

[gerhardol]

Should be reviewed together with #4327, this PR is based on #4327 even if the changes can be separated.

Issue #4256
Listing ignored files with a separate "ls-files -o -i --exclude-standard" seem to never exit in some situations, see #4256
This reuses the git-status call with --ignored as an option to get the ignored files. (The option is only added when ls-files were called previously.) This call did not seem to hang with many ignored files and the command runtime does not increase much.

The solution is not optional though. It still lists all ignored files in memory. The time to find ignored files is faster, but a lot of memory is needed. #4256 has some discussions.
Also, processing all output to GitStatusItem first before creating the HashSet will require some more memory (temporary) than required.

There are likely better places to optimize (and this PR reduces CPU load by skipping a separate heavy ls-files), but memory and CPU usage (normally a few ms) can be improved by skipping ignored by default in GetAllChangedFilesFromString() and process the ignored separately. I felt this would make the solution more complex and duplicate code.

It would be more efficient from memory usage (some CPU hits) to use the Git 2.15.1 option --ignored=matching, but a fallback solution (at least) is needed.

Changes proposed in this pull request:

• Use git-status to retrieve ignored files instead of ls-files

What did I do to test the code and ensure quality:

• Test that ignored files are ignored in a similar way as before

Has been tested on (remove any that don't apply):

• GIT 2.15
• Windows 10

[RussKie]

Please update/rebase when you have time.
Thank you

[gerhardol]

Will rebase now when #4327 is merged, the two last commits are applicable

[gerhardol]

Rebased without changes.

There are likely better places to optimize (and this PR reduces CPU load by skipping a separate heavy ls-files), but memory and CPU usage (normally a few ms) can be improved by skipping ignored by default in GetAllChangedFilesFromString() and process the ignored separately. I felt this would make the solution more complex and duplicate code.

Repeating from PR description. This is the simplest way and a solution in the same way as normal GE functionality, but this is not the same as this the best way to add more info to GitItemStatus (maybe better to add ~100 lines of duplicated code).

[RussKie]

Thanks Reading the commit message, you suggest 2.15.1 provides more optimal behavior. Will we need to revisit this after adding 2.15.1+ to our setup? Do we need to add conditional pivots depending on user's git version?

…

On 02/02/2018 8:08 AM, "Gerhard Olsson" ***@***.***> wrote: Rebased without changes. There are likely better places to optimize (and this PR reduces CPU load by skipping a separate heavy ls-files), but memory and CPU usage (normally a few ms) can be improved by skipping ignored by default in GetAllChangedFilesFromString() and process the ignored separately. I felt this would make the solution more complex and duplicate code. Repeating from PR description. This is the simplest way and a solution in the same way as normal GE functionality, but this is not the same as this the best way to add more info to GitItemStatus (maybe better to add ~100 lines of duplicated code). — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#4328 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEMyXlZBhv0ysE10g6daRtkSViRPmOW1ks5tQifZgaJpZM4RVf9_> .

[RussKie]

Not saying it is an action item, but typically if to imement something we need to duplicate code - then we need to refactor and extract the shared implementation. :)

…

On 02/02/2018 8:18 AM, "Igor Velikorossov" ***@***.***> wrote: Thanks Reading the commit message, you suggest 2.15.1 provides more optimal behavior. Will we need to revisit this after adding 2.15.1+ to our setup? Do we need to add conditional pivots depending on user's git version? On 02/02/2018 8:08 AM, "Gerhard Olsson" ***@***.***> wrote: > Rebased without changes. > > There are likely better places to optimize (and this PR reduces CPU load > by skipping a separate heavy ls-files), but memory and CPU usage (normally > a few ms) can be improved by skipping ignored by default in > GetAllChangedFilesFromString() and process the ignored separately. I felt > this would make the solution more complex and duplicate code. > > Repeating from PR description. This is the simplest way and a solution in > the same way as normal GE functionality, but this is not the same as this > the best way to add more info to GitItemStatus (maybe better to add ~100 > lines of duplicated code). > > — > You are receiving this because you commented. > Reply to this email directly, view it on GitHub > <#4328 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AEMyXlZBhv0ysE10g6daRtkSViRPmOW1ks5tQifZgaJpZM4RVf9_> > . >

[gerhardol]

Will we need to revisit this after adding 2.15.1+ to our setup?

2.15.1 can optimize the solution, primarily minimize memory usage. But that will not require many lines of code (most is to detect Git version and to set --ignored=matching, then only ignored directories will be listed). Less memory but higher CPU usage and some corner cases will be ignored (a file that is not ignored in an ignored directory will not trigger an update, but we can live with that corner case).
I do not like to have different behavior for different Git revisions.

Not saying it is an action item, but typically if to imement something we need to duplicate code

This solution requires no duplication. Not adding IsIgnored will likely require duplication of GetAllChangedFilesFromString().