Fix submodule status loading error

[drewnoakes]

Fixes @gerhardol's comments on #8875

Proposed changes

Refactoring during annotation identified a potential issue here, but the change was not correct. This change allows the task itself to be null (an uncommon pattern) which indicates that SetSubmoduleStatus has not been called.

Test methodology

• Manual testing

To trigger the broken behaviour:

1. Open the GE repo
2. Select the "File Tree" tab
3. Select a submodule in the tree
4. Press H to open the history

---

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

[codecov]

Codecov Report

Merging #8881 (30c0456) into master (b6905f1) will increase coverage by 0.02%.
The diff coverage is 31.98%.

@@            Coverage Diff             @@
##           master    #8881      +/-   ##
==========================================
+ Coverage   56.03%   56.06%   +0.02%     
==========================================
  Files         922      922              
  Lines       65944    65847      -97     
  Branches    12070    12084      +14     
==========================================
- Hits        36951    36915      -36     
+ Misses      25985    25921      -64     
- Partials     3008     3011       +3     

Flag	Coverage Δ
production	43.30% <22.76%> (+0.04%)	⬆️
tests	94.82% <87.27%> (-0.01%)	⬇️

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

[gerhardol]

quick check only

[gerhardol]

I pushed a proposal to a local branch, using pattern matching declarations where possible and find the code much more easy to read

[RussKie]

I'm struggling with this - statusTask can be null, and in case when it is will it throw on line:1956 or line:1957?

[gerhardol]

I rewrote most of the checks using pattern matching declarations, I find this much easier to read

[gerhardol]

need to remove this check to set status om L942

[gerhardol]

removed the check in a separate push

[drewnoakes]

@gerhardol the type check is essentially a null check too.

Suggested change

	&& item.GetSubmoduleStatusAsync() is Task<GitSubmoduleStatus> task
	&& task is not null)
	&& item.GetSubmoduleStatusAsync() is Task<GitSubmoduleStatus> task)

I'm not a fan of this code though. It makes it look like a runtime type test is needed, which is not the case. This is just a null check.

[drewnoakes]

Another way of writing this is:

Suggested change

	&& item.GetSubmoduleStatusAsync() is Task<GitSubmoduleStatus> task
	&& task is not null)
	&& item.GetSubmoduleStatusAsync() is { } task)

But is that actually better than assigning the task to a local and doing an explicit is not null test? The only downside is a bit of nesting, which is not the end of the world.

[mstv]

We had this discussion before: This is a hard to read. One does not need to use every new C# syntax invention.

[gerhardol]

The task is not null checks in FileStatusList.cs are not needed.
I changed to that style as it is required in FormCommit

I prefer to have a minimal code why my preference is to just removing is not null where possible.