Fixed git status retrieval splitting entries
ClosedPublic

Authored by macdems on Thu, Nov 7, 8:58 AM.

Details

Summary

BUG: 413870

Sometimes, for large repositories the status is messed up (which in unlucky case cases Commit.. command to disappear).

The reason tho this is that FileViewGitPlugin::beginRetrieval method cannot correctly parse the git status output. The reason for this is that FileViewGitPlugin::readUntilZeroChar does not return complete entry, because it is not provided fast enough.

I have made a patch that fixes this issue. The idea is to allow the FileViewGitPlugin::readUntilZeroChar to wait for the remaining data.

Test Plan

The bug depends on the race condition between the plugin code and executed git code. Hence it is hard to test. To fake the issue make fake git process that pauses mid entry. In such case the loop in FileViewGitPlugin::beginRetrieval should parse eg:

!! ABCDEFGH\0x00

Instead it gets:

!! AB\0x00
CDEFGH\0x00

which results in two entries for non-existent files.

The path solves this issue.

Diff Detail

Repository
R449 Plugins for Dolphin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
macdems requested review of this revision.Thu, Nov 7, 8:58 AM
macdems created this revision.
macdems added a reviewer: elvisangelaccio.
elvisangelaccio added inline comments.Sat, Nov 9, 6:12 PM
git/fileviewgitplugin.cpp
143

Aren't 30 seconds a bit too much? I'm worried about UI freezes in the worst case. Do we really need to wait this long?

macdems added inline comments.Sat, Nov 9, 8:36 PM
git/fileviewgitplugin.cpp
143

Probably it is too long. However, this number is default value for QProcess::waitForReadyRead, which is used e.g. in FileViewGitPlugin::beginRetrieval. This is why I put this number here. However, to me it can be easily reduced to e.g. 3 seconds or even half a second. If so, the change should be introduced both here and in FileViewGitPlugin::beginRetrieval.

git/fileviewgitplugin.cpp
143

I see. I'd put this information in a comment, e.g. "// 30 seconds because we need to match the QProcess::waitForReadyRead default"

macdems updated this revision to Diff 69550.Sun, Nov 10, 8:49 PM

Added a comment explaining the chosen timeout.

macdems marked an inline comment as done.Sun, Nov 10, 8:49 PM
elvisangelaccio accepted this revision.Sun, Nov 10, 8:58 PM

Please provide an email address so that we can commit this patch on your behalf. Thanks!

This revision is now accepted and ready to land.Sun, Nov 10, 8:58 PM

Please provide an email address so that we can commit this patch on your behalf. Thanks!

macdems@gmail.com

Thanks :)

This revision was automatically updated to reflect the committed changes.