Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Optimize EngineFileUtilities.GetFileList #6227

Merged
merged 6 commits into from Mar 11, 2021
Merged

Optimize EngineFileUtilities.GetFileList #6227

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
99f5b6e
Remove unused parameter from GetFileListUnescaped
ladipro Mar 4, 2021
4cdd49e
Don't create empty excludeSpecsEscaped lists
ladipro Mar 4, 2021
3725081
Optimize the order of wildcard checks in BuildItemFragments
ladipro Mar 4, 2021
944c66a
Optimize the order of wildcard checks in FilespecHasWildcards
ladipro Mar 4, 2021
554de4b
Optimize hex number parsing in UnescapeAll
ladipro Mar 4, 2021
5f4f7c2
Optimize ContainsEscapedWildcards to do the work in one pass
ladipro Mar 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
25 changes: 11 additions & 14 deletions src/Build/Evaluation/ItemSpec.cs
View file
Open in desktop
Expand Up @@ -211,31 +211,28 @@ private List<ItemSpecFragment> BuildItemFragments(IElementLocation itemSpecLocat
// The expression is not of the form "@(X)". Treat as string

// Code corresponds to EngineFileUtilities.GetFileList
var containsEscapedWildcards = EscapingUtilities.ContainsEscapedWildcards(splitEscaped);
var containsRealWildcards = FileMatcher.HasWildcards(splitEscaped);
if (!FileMatcher.HasWildcards(splitEscaped))
{
// No real wildcards means we just return the original string. Don't even bother
// escaping ... it should already be escaped appropriately since it came directly
// from the project file

// '*' is an illegal character to have in a filename.
// todo: file-system assumption on legal path characters: https://github.com/Microsoft/msbuild/issues/781
if (containsEscapedWildcards && containsRealWildcards)
fragments.Add(new ValueFragment(splitEscaped, projectDirectory));
}
else if (EscapingUtilities.ContainsEscapedWildcards(splitEscaped))
{
// '*' is an illegal character to have in a filename.
// todo: file-system assumption on legal path characters: https://github.com/Microsoft/msbuild/issues/781
// Just return the original string.
fragments.Add(new ValueFragment(splitEscaped, projectDirectory));
}
else if (!containsEscapedWildcards && containsRealWildcards)
else
{
// Unescape before handing it to the filesystem.
var filespecUnescaped = EscapingUtilities.UnescapeAll(splitEscaped);

fragments.Add(new GlobFragment(filespecUnescaped, projectDirectory));
}
else
{
// No real wildcards means we just return the original string. Don't even bother
// escaping ... it should already be escaped appropriately since it came directly
// from the project file

fragments.Add(new ValueFragment(splitEscaped, projectDirectory));
}
}
}
}
Expand Down
39 changes: 11 additions & 28 deletions src/Build/Utilities/EngineFileUtilities.cs
View file
Open in desktop
Expand Up @@ -53,17 +53,15 @@ public EngineFileUtilities(FileMatcher fileMatcher)
/// </summary>
/// <param name="directoryEscaped">The directory to evaluate, escaped.</param>
/// <param name="filespecEscaped">The filespec to evaluate, escaped.</param>
/// <param name="forceEvaluate">Whether to force file glob expansion when eager expansion is turned off</param>
/// <returns>Array of file paths, unescaped.</returns>
internal string[] GetFileListUnescaped
(
string directoryEscaped,
string filespecEscaped,
bool forceEvaluate = false
string filespecEscaped
)

{
return GetFileList(directoryEscaped, filespecEscaped, false /* returnEscaped */, forceEvaluate);
return GetFileList(directoryEscaped, filespecEscaped, returnEscaped: false, forceEvaluateWildCards: false);
}

/// <summary>
Expand All @@ -89,31 +87,21 @@ internal string[] GetFileListEscaped
bool forceEvaluate = false
)
{
return GetFileList(directoryEscaped, filespecEscaped, true /* returnEscaped */, forceEvaluate, excludeSpecsEscaped);
return GetFileList(directoryEscaped, filespecEscaped, returnEscaped: true, forceEvaluate, excludeSpecsEscaped);
}

internal static bool FilespecHasWildcards(string filespecEscaped)
{
bool containsEscapedWildcards = EscapingUtilities.ContainsEscapedWildcards(filespecEscaped);
bool containsRealWildcards = FileMatcher.HasWildcards(filespecEscaped);

if (containsEscapedWildcards && containsRealWildcards)
{
// Umm, this makes no sense. The item's Include has both escaped wildcards and
// real wildcards. What does he want us to do? Go to the file system and find
// files that literally have '*' in their filename? Well, that's not going to
// happen because '*' is an illegal character to have in a filename.

return false;
}
else if (!containsEscapedWildcards && containsRealWildcards)
{
return true;
}
else
if (!FileMatcher.HasWildcards(filespecEscaped))
{
return false;
}

// If the item's Include has both escaped wildcards and real wildcards, then it's
// not clear what they are asking us to do. Go to the file system and find
// files that literally have '*' in their filename? Well, that's not going to
// happen because '*' is an illegal character to have in a filename.
return !EscapingUtilities.ContainsEscapedWildcards(filespecEscaped);
}

/// <summary>
Expand Down Expand Up @@ -143,11 +131,6 @@ private string[] GetFileList
{
ErrorUtilities.VerifyThrowInternalLength(filespecEscaped, nameof(filespecEscaped));

if (excludeSpecsEscaped == null)
{
excludeSpecsEscaped = Enumerable.Empty<string>();
}

string[] fileList;

if (!FilespecHasWildcards(filespecEscaped) ||
Expand All @@ -166,7 +149,7 @@ private string[] GetFileList
// Unescape before handing it to the filesystem.
var directoryUnescaped = EscapingUtilities.UnescapeAll(directoryEscaped);
var filespecUnescaped = EscapingUtilities.UnescapeAll(filespecEscaped);
var excludeSpecsUnescaped = excludeSpecsEscaped.Where(IsValidExclude).Select(i => EscapingUtilities.UnescapeAll(i)).ToList();
var excludeSpecsUnescaped = excludeSpecsEscaped?.Where(IsValidExclude).Select(i => EscapingUtilities.UnescapeAll(i)).ToList();

// Get the list of actual files which match the filespec. Put
// the list into a string array. If the filespec started out
Expand Down
68 changes: 41 additions & 27 deletions src/Shared/EscapingUtilities.cs
View file
Open in desktop
Expand Up @@ -26,11 +26,25 @@ static internal class EscapingUtilities
/// </summary>
private static Dictionary<string, string> s_unescapedToEscapedStrings = new Dictionary<string, string>(StringComparer.Ordinal);

private static bool IsHexDigit(char character)
private static bool TryDecodeHexDigit(char character, out int value)
{
return ((character >= '0') && (character <= '9'))
|| ((character >= 'A') && (character <= 'F'))
|| ((character >= 'a') && (character <= 'f'));
if (character >= '0' && character <= '9')
{
value = character - '0';
return true;
}
if (character >= 'A' && character <= 'F')
{
value = character - 'A' + 10;
return true;
}
if (character >= 'a' && character <= 'f')
{
value = character - 'a' + 10;
return true;
}
value = default;
return false;
}

/// <summary>
Expand Down Expand Up @@ -85,18 +99,16 @@ internal static string UnescapeAll(string escapedString, bool trim = false)
// for us to even consider doing anything with this.
if (
(indexOfPercent <= (escapedStringLength - 3)) &&
IsHexDigit(escapedString[indexOfPercent + 1]) &&
IsHexDigit(escapedString[indexOfPercent + 2])
TryDecodeHexDigit(escapedString[indexOfPercent + 1], out int digit1) &&
TryDecodeHexDigit(escapedString[indexOfPercent + 2], out int digit2)
)
{
// First copy all the characters up to the current percent sign into
// the destination.
unescapedString.Append(escapedString, currentPosition, indexOfPercent - currentPosition);

// Convert the %XX to an actual real character.
string hexString = escapedString.Substring(indexOfPercent + 1, 2);
char unescapedCharacter = (char)int.Parse(hexString, System.Globalization.NumberStyles.HexNumber,
CultureInfo.InvariantCulture);
char unescapedCharacter = (char)((digit1 << 4) + digit2);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!


// if the unescaped character is not on the exception list, append it
unescapedString.Append(unescapedCharacter);
Expand Down Expand Up @@ -214,28 +226,30 @@ string unescapedString
/// </summary>
/// <param name="escapedString"></param>
/// <returns></returns>
internal static bool ContainsEscapedWildcards
(
string escapedString
)
internal static bool ContainsEscapedWildcards(string escapedString)
{
if (-1 != escapedString.IndexOf('%'))
if (escapedString.Length < 3)
{
// It has a '%' sign. We have promise.
if (
(-1 != escapedString.IndexOf("%2", StringComparison.Ordinal)) ||
(-1 != escapedString.IndexOf("%3", StringComparison.Ordinal))
)
return false;
}
// Look for the first %. We know that it has to be followed by at least two more characters so we subtract 2
// from the length to search.
int index = escapedString.IndexOf('%', 0, escapedString.Length - 2);
KirillOsenkov marked this conversation as resolved.
Show resolved Hide resolved
while (index != -1)
{
if (escapedString[index + 1] == '2' && (escapedString[index + 2] == 'a' || escapedString[index + 2] == 'A'))
{
// %2a or %2A
return true;
}
if (escapedString[index + 1] == '3' && (escapedString[index + 2] == 'f' || escapedString[index + 2] == 'F'))
{
// It has either a '%2' or a '%3'. This is looking very promising.
return

(-1 != escapedString.IndexOf("%2a", StringComparison.Ordinal)) ||
(-1 != escapedString.IndexOf("%2A", StringComparison.Ordinal)) ||
(-1 != escapedString.IndexOf("%3f", StringComparison.Ordinal)) ||
(-1 != escapedString.IndexOf("%3F", StringComparison.Ordinal))
;
// %3f or %3F
return true;
}
// Continue searching for % starting at (index + 1). We know that it has to be followed by at least two
// more characters so we subtract 2 from the length of the substring to search.
index = escapedString.IndexOf('%', index + 1, escapedString.Length - (index + 1) - 2);
}
return false;
}
Expand Down
7 changes: 6 additions & 1 deletion src/Shared/UnitTests/EscapingUtilities_Tests.cs
View file
Open in desktop
Expand Up @@ -75,13 +75,18 @@ public void EscapeUnescape()
public void ContainsEscapedWildcards()
{
Assert.False(EscapingUtilities.ContainsEscapedWildcards("NoStarOrQMark"));
Assert.False(EscapingUtilities.ContainsEscapedWildcards("%"));
Assert.False(EscapingUtilities.ContainsEscapedWildcards("%%"));
Assert.False(EscapingUtilities.ContainsEscapedWildcards("%2"));
Assert.False(EscapingUtilities.ContainsEscapedWildcards("%4"));
Assert.False(EscapingUtilities.ContainsEscapedWildcards("%3B"));
Assert.False(EscapingUtilities.ContainsEscapedWildcards("%3A"));
Assert.False(EscapingUtilities.ContainsEscapedWildcards("%2B"));
Assert.True(EscapingUtilities.ContainsEscapedWildcards("%2a"));
Assert.True(EscapingUtilities.ContainsEscapedWildcards("%2A"));
Assert.True(EscapingUtilities.ContainsEscapedWildcards("%3F"));
Assert.True(EscapingUtilities.ContainsEscapedWildcards("%3f"));
Assert.True(EscapingUtilities.ContainsEscapedWildcards("%%3f"));
Assert.True(EscapingUtilities.ContainsEscapedWildcards("%3%3f"));
}
}
}