Fix Git status issues in Source Control view

[westbury]

What it does

Fixes #7789

Fixes issues with the the Source Control view.

• The list of staged and unstaged files are incorrect
• The status indicators are incorrect

I have attached here four different screen-shots of the same working tree and index. The following actions were taken:

• FileA was created, staged, then modified
• FileB was created, staged, then deleted
• git-diff-contribution.ts was renamed to git-diff-contributionA.ts, staged, then modified

This is what we see with this PR:

This is what we see in current Theia master. FileB is missing altogether which is not correct because if the user were to commit then FileB would be committed. FileA shows as Untracked in the working tree which is incorrect because the file has been staged and the user would expect to be able to see the changes in the file which have not been staged. git-diff-contributionA.ts shows as a Rename in both groups but it should show as Modified in the unstaged section because it was not renamed again, just modified.

This is what we see in VS-Code. This is not correct.

This is what we see in the git-gui tool (staged and unstaged sections are reversed). This matches what we see with this PR except that the rename is shown as an add and a delete. Selecting either of the two modified files in the unstaged changes will show the changes between the working tree and the index.

You will notice that this code calls dugite, not dugite-extra, for the status. This change was made because the status function in dugite-extra has been written to support a simplified UI in which there is no Staged or Unstaged section shown but just a single list of files that can be checked (to include in the commit) or unchecked (to exclude from the commit). This UI is used, for example, by Github Desktop. Clients that show separate staged and unstaged sections should use the status as returned by git status.

The initial code to check the git version was copied from dugite-extra. We could change dugite-extra to expose that code so it is not duplicated. I don't know about the other clients of dugite-extra so I have not done that.

You will notice some of the diffs do not open. For example if you rename a file and make a change, then stage it. Selecting the file in the staged section, one would expect to see a diff with the change. The problem is caused because GitScmProvider.getUriToOpen is not using the old uri for the HEAD uri. This is changed by #8084 so will be fixed when that is merged.

How to test

The three cases above are not exhaustive. There are numerous combinations of Rename, Copy, Add, Modify, and Delete that can be in the staged and unstaged sections. Ideally all these combinations should be tested.

Review checklist

• [x ] as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

• as a reviewer, I agree to behave in accordance with the review guidelines

[akosyakov]

@AlexTugarev, @kittaakos Could you have a look at dugite changes? I

[kittaakos]

We should either fix it in dugite-extra or move all code from dugite-extra to theia.

[westbury]

We should either fix it in dugite-extra or move all code from dugite-extra to theia.

@kittaakos I'm in favor of moving all code from Dugite-extra to Theia, but I don't know the requirements of other users of Dugite-extra. There may be bits that could be upstreamed to Dugite, for example the code for handling optional locks. But I think the parsing of the machine readable Git output is best done in Theia. I'm happy to modify this PR to move all of Dugite-extra into Theia's git package. It's your call.

[kittaakos]

but I don't know the requirements of other users of Dugite-extra

We do not have to know the requirements of other dugite-extra users, we simply won't increase its version number, ever. We won't break any downstreams, we just stop maintaining the lib.

There may be bits that could be upstreamed to Dugite, for example the code for handling optional locks

By default, dugite comes with git; optional locking, or (porcelain) parsing is not an issue for them as they know what git version they're using. You can try to propose changes there, though. Also, please keep in mind, dugite-extra depends on dugite-no-gpl and not dugite.

But I think the parsing of the machine readable Git output is best done in Theia

Sure. We already have a couple of output parsers in Theia.

It's your call.

I'd leave it open for the community, I am not even a user of the Git extension.

I'm happy to modify

Wow, sounds great! If we all agree on making this move, let's do it.

[westbury]

@kittaakos I've now modified dugite-git.ts so that it calls git directly instead of calling dugite-extra. I've also added two tests that demonstrate the fixes in the PR. I.e. these two tests fail with the old code.

Note that git.ts has been copied from dugite-extra. It comes with MIT license, though you may want to change to the EPL.

[kittaakos]

Note that git.ts has been copied from dugite-extra.

@westbury, why is it required to copy code from dugite-extra to Theia?

[westbury]

why is it required to copy code from dugite-extra to Theia?

@kittaakos Just to minimize the changes and to ensure consistent behavior. I've now removed the copy of the git.ts file, and changed dugite-git.ts and git-exec-provider.ts so they don't need that file.

[westbury]

I have added a second commit to this PR. This commit fixes a problem introduced by #8911. In that commit, Git status decorators were linked to the file URI. However the same file can have a different status in the staged changes than in the unstaged changes. So this commit fixes that by adding a fragment (staged or unstaged) to the file URI.

This commit could be a separate PR, as it fixes a different problem with the status decorator. However it is hard to test without both fixes.

[thegecko]

This change is no longer a priority for Arm, so we aren't actively trying to get it merged. It looks like it fixes a reported issue which may also be resolved by using the VS Code git extension. This PR can be re-opened if required.