We look for and analyze errors in the GitExtensions code.

As it is known, the Git Kernel is a set of command line utilities with parameters. For comfortable work, as a rule, utilities are used that give us a familiar graphical interface. So I happened to work with such a utility for Git as GitExtensions in due time. Not to say that this is the most convenient tool I've used (for example, I like TortoiseGit more), but it clearly and not unreasonably occupies its niche in the list of favorite and proven utilities for working with Git.

Recently, I remembered about it, and decided to check with the static analyzer for errors and misprints of its source code. About what errors were found after verification, I will discuss in this article.

GitExtensions

GitExtensions is a cross-platform visual client for working with open source Git version control system.
')
The GitExtensions project is small. In total, there are 10 main projects, 20 plug-ins and 2 additional projects compiled into auxiliary libraries. A total of 56,091 lines of code in the 441 file.

Let's see if something interesting can be found in this project with the PVS-Studio static analyzer.

Test results

Following the audit, 121 warnings were received. If we consider in more detail, 16 warnings were received at the first (high) level. 11 of them clearly pointed to problem areas or errors. At the second level (average), 80 warnings were received. In my subjective opinion, 69 warnings were correct and pointed to the place with errors or typographical errors. We will not consider the third (low) level of warnings, since it basically indicates the places where the occurrence of errors is unlikely. And so, we proceed to the consideration of errors found.

Expression is always true

Begins our hit parade rather strange piece of code.

string rev1 = ""; string rev2 = ""; var revisions = RevisionGrid.GetSelectedRevisions(); if (revisions.Count > 0) { rev1 = ....; rev2 = ....; .... } else if (string.IsNullOrEmpty(rev1) || string.IsNullOrEmpty(rev2)) // <= { MessageBox.Show(....); return; } 

V3022 Expression 'string.IsNullOrEmpty (rev1) || string.IsNullOrEmpty (rev2) 'is always true. GitUI FormFormatPatch.cs 155

The analyzer issued a warning that the expression with checking the variables rev1 and rev2 will always be true. At first, I thought that this is a common typo, an error in the logic of the algorithm, which does not affect the correctness of the program. But having considered the code in more detail I noticed, apparently the superfluous else operator. It is before the second check, which will be performed only in case of inequality of the first.

One of the comparisons is redundant

The second number in our charts is a common typo. It does not affect the logic of the program, but this example shows well how useful static analysis is.

 public void EditNotes(string revision) { string editor = GetEffectivePathSetting("core.editor").ToLower(); if (editor.Contains("gitextensions") || editor.Contains("notepad") || // <= editor.Contains("notepad++")) // <= { RunGitCmd("notes edit " + revision); } .... } 

V3053 An excessive expression. Examine the substrings 'notepad' and 'notepad ++'. GitCommands GitModule.cs 691

In the expression, a longer ( notepad ++ ) and a shorter ( notepad ) substring is searched. At the same time, checking with a longer string will never work, since checking with a search for a shorter string will prevent it. As I have already mentioned, this is not a mistake, but just a typo, but in another situation an innocent redundant check could turn into a potentially insidious bug.

The variable is used before checking for null.

The third place is taken by a fairly common mistake, but I cannot say that in this case it will cause the program to work incorrectly 100%, because I didn’t go too deep into the logic of this code. Only the fact that the variable being checked for null can speak of an assumed zero value.

 void DataReceived(string data) { if (data.StartsWith(CommitBegin)) // <= { .... } if (!string.IsNullOrEmpty(data)) { .... } } 

V3095 The 'data' object was used against it. Check lines: 319, 376. GitCommands RevisionGraph.cs 319

The data variable is used before checking for null , which can potentially lead to a NullReferenceException exception. If the data variable is never null, then the check below is useless and should be removed so that it is not misleading.

A NullReferenceException exception may occur in the overridden Equals method.

This error is a lot like the previous one. If you compare two objects using the overridden Equals method, there is always a chance that null will come as the second comparison object.

 public override bool Equals(object obj) { return GetHashCode() == obj.GetHashCode(); // <= } 

V3115 Passing 'null' to 'Equals (object obj)' method should not result in 'NullReferenceException'. Git.hub User.cs 16

Upon a further call to the overridden Equals method, a NullReferenceException exception is possible if the obj parameter is null . To prevent this situation, you must use null checking. For example:

 public override bool Equals(object obj) { return GetHashCode() == obj?.GetHashCode(); // <= } 

Identical expressions in the if condition

Under the fifth number proudly located 2 typos. They do not affect the result of the program, but we can classify them as redundant checks.

 private void ConfigureRemotes() { .... if (!remoteHead.IsRemote || localHead.IsRemote || !string.IsNullOrEmpty(localHead.GetTrackingRemote(localConfig)) || !string.IsNullOrEmpty(localHead.GetTrackingRemote(localConfig)) || remoteHead.IsTag || localHead.IsTag || !remoteHead.Name.ToLower().Contains(localHead.Name.ToLower()) || !remoteHead.Name.ToLower().Contains(_remote.ToLower())) continue; .... } 

V3001 There are identical to the left and the right of the | || operator. GitUI FormRemotes.cs 192

If you look closely, you can see 2 identical conditions in the check. Most likely this is a consequence of copy-paste. There is also the possibility of an error if we consider that the second expression implied the use of the variable remoteHead instead of localHead , but without a deep analysis of the algorithm of the program’s work, it is hard to say.

Also found another similar error.

 if (!curItem.IsSourceEqual(item.NeutralValue) && // <= (!String.IsNullOrEmpty(curItem.TranslatedValue) && !curItem.IsSourceEqual(item.NeutralValue))) // <= { curItem.TranslatedValue = ""; } 

V3001 operator and the operator. TranslationApp TranslationHelpers.cs 112

The number of parameters does not match when formatting a string

The sixth place goes to a fairly common error that occurs as a result of the inattention of programmers when refactoring the text of formatted strings.

 Debug.WriteLine("Loading plugin...", pluginFile.Name); // <= 

V3025 Incorrect format. A different number of items is expected while calling the 'WriteLine' function. Arguments not used: pluginFile.Name. GitUI LoadPlugins.cs 35

In this situation, I can assume that the programmer thought that the value of the variable pluginFile.Name would be added to the end of the formatted string when outputting to debug, but this is not so. The correct code will look like this:

 Debug.WriteLine("Loading '{0}' plugin...", pluginFile.Name); // <= 

Part of the expression is always true.

And at the end of another typo, which could have been avoided.

 private void toolStripButton(...) { var button = (ToolStripButton)sender; if (!button.Enabled) { StashMessage.ReadOnly = true; } else if (button.Enabled && button.Checked) // <= { StashMessage.ReadOnly = false; } } 

If it is evaluated: button.Enabled. GitUI FormStash.cs 301

Since we check that button.Enabled is false , then in the else of this check we can safely say that button.Enabled will be true and thus exclude this check again.

Conclusion

In this project, there were other errors, typos, shortcomings. But they did not seem interesting to me to describe them in the article. GitExtensions developers will easily be able to find all the shortcomings using the PVS-Studio tool. You can also look for errors in your projects using the static analyzer proposed above.

I want to remind you that the main advantage of static analysis is its regular use. Download and one-time check code, this is not serious. For example, programmers watch the compiler warnings regularly, rather than turning them on every 3 years before one of the releases. With regular use, a static analyzer will save a lot of time searching for typos and errors.


If you want to share this article with an English-speaking audience, then please use the link to the translation: Ivan Kishchenko. GitExtensions bugs found and analyzed .