Skip to content

Get-Content -Head and -Tail disallow negative values and cleanup #19715

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

Merged
daxian-dbw merged 15 commits into from
Jun 21, 2023
Merged

Get-Content -Head and -Tail disallow negative values and cleanup #19715

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
ad5957e
TotalCount {get; set;}
CarloToso May 23, 2023
fdf3335
Tail { get; set; }
CarloToso May 23, 2023
055da9f
Add comment Tail
CarloToso May 24, 2023
fe59996
add {} while, for
CarloToso May 24, 2023
19ba389
remove remainder
CarloToso May 24, 2023
4ba3554
Add {} if
CarloToso May 24, 2023
f74f1e8
remove space
CarloToso May 24, 2023
4d2459b
var, Logs
CarloToso May 25, 2023
777d526
fast return
CarloToso May 25, 2023
c60a839
remove unnecessary if
CarloToso May 25, 2023
c4d1b51
fix comment
CarloToso May 25, 2023
c5729b1
capitalize comment
CarloToso May 25, 2023
174ad16
edit comment
CarloToso May 25, 2023
434dfe8
Add Tests
CarloToso Jun 9, 2023
727d53a
Add {}
CarloToso Jun 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
169 changes: 63 additions & 106 deletions src/Microsoft.PowerShell.Commands.Management/commands/management/GetContentCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,48 +31,20 @@ public class GetContentCommand : ContentCommandBase
public long ReadCount { get; set; } = 1;

/// <summary>
/// The number of content items to retrieve. By default this
/// value is -1 which means read all the content.
/// The number of content items to retrieve.
/// </summary>
[Parameter(ValueFromPipelineByPropertyName = true)]
[ValidateRange(0, long.MaxValue)]
[Alias("First", "Head")]
public long TotalCount
{
get
{
return _totalCount;
}

set
{
_totalCount = value;
_totalCountSpecified = true;
}
}

private bool _totalCountSpecified = false;
public long TotalCount { get; set; } = -1;
Copy link
Collaborator

@JamesWTruher JamesWTruher Jun 21, 2023

Choose a reason for hiding this comment

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

hmmm, I wonder if this should be just be long? TotalCount


/// <summary>
/// The number of content items to retrieve from the back of the file.
/// </summary>
[Parameter(ValueFromPipelineByPropertyName = true)]
[ValidateRange(0, int.MaxValue)]
[Alias("Last")]
public int Tail
{
get
{
return _backCount;
}

set
{
_backCount = value;
_tailSpecified = true;
}
}

private int _backCount = -1;
private bool _tailSpecified = false;
public int Tail { get; set; } = -1;

/// <summary>
/// A virtual method for retrieving the dynamic parameters for a cmdlet. Derived cmdlets
Expand All @@ -98,15 +70,6 @@ internal override object GetDynamicParameters(CmdletProviderContext context)

#endregion Parameters

#region parameter data

/// <summary>
/// The number of content items to retrieve.
/// </summary>
private long _totalCount = -1;

#endregion parameter data

#region Command code

/// <summary>
Expand All @@ -116,15 +79,15 @@ protected override void ProcessRecord()
{
// TotalCount and Tail should not be specified at the same time.
// Throw out terminating error if this is the case.
Copy link
Collaborator

@JamesWTruher JamesWTruher Jun 21, 2023

Choose a reason for hiding this comment

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

this comment seems wrong - it doesn't look like a terminating error

if (_totalCountSpecified && _tailSpecified)
if (TotalCount != -1 && Tail != -1)
{
string errMsg = StringUtil.Format(SessionStateStrings.GetContent_TailAndHeadCannotCoexist, "TotalCount", "Tail");
ErrorRecord error = new(new InvalidOperationException(errMsg), "TailAndHeadCannotCoexist", ErrorCategory.InvalidOperation, null);
WriteError(error);
return;
}

if (TotalCount == 0)
if (TotalCount == 0 || Tail == 0)
{
// Don't read anything
return;
Expand All @@ -141,23 +104,21 @@ protected override void ProcessRecord()
{
long countRead = 0;

Dbg.Diagnostics.Assert(
holder.Reader != null,
"All holders should have a reader assigned");
Dbg.Diagnostics.Assert(holder.Reader != null, "All holders should have a reader assigned");

if (_tailSpecified && holder.Reader is not FileSystemContentReaderWriter)
if (Tail != -1 && holder.Reader is not FileSystemContentReaderWriter)
{
string errMsg = SessionStateStrings.GetContent_TailNotSupported;
ErrorRecord error = new(new InvalidOperationException(errMsg), "TailNotSupported", ErrorCategory.InvalidOperation, Tail);
WriteError(error);
continue;
}

// If Tail is negative, we are supposed to read all content out. This is same
// If Tail is -1, we are supposed to read all content out. This is same
// as reading forwards. So we read forwards in this case.
// If Tail is positive, we seek the right position. Or, if the seek failed
// because of an unsupported encoding, we scan forward to get the tail content.
if (Tail >= 0)
if (Tail > 0)
{
bool seekSuccess = false;

Expand Down Expand Up @@ -197,72 +158,61 @@ protected override void ProcessRecord()
}
}

if (TotalCount != 0)
IList results = null;

do
{
IList results = null;
long countToRead = ReadCount;

do
// Make sure we only ask for the amount the user wanted
// I am using TotalCount - countToRead so that I don't
// have to worry about overflow
if (TotalCount > 0 && (countToRead == 0 || TotalCount - countToRead < countRead))
{
long countToRead = ReadCount;
countToRead = TotalCount - countRead;
}

// Make sure we only ask for the amount the user wanted
// I am using TotalCount - countToRead so that I don't
// have to worry about overflow
try
{
results = holder.Reader.Read(countToRead);
}
catch (Exception e) // Catch-all OK. 3rd party callout
{
ProviderInvocationException providerException =
new(
"ProviderContentReadError",
SessionStateStrings.ProviderContentReadError,
holder.PathInfo.Provider,
holder.PathInfo.Path,
e);

if ((TotalCount > 0) && (countToRead == 0 || (TotalCount - countToRead < countRead)))
{
countToRead = TotalCount - countRead;
}
// Log a provider health event
MshLog.LogProviderHealthEvent(this.Context, holder.PathInfo.Provider.Name, providerException, Severity.Warning);
WriteError(new ErrorRecord(providerException.ErrorRecord, providerException));

try
{
results = holder.Reader.Read(countToRead);
}
catch (Exception e) // Catch-all OK. 3rd party callout
break;
}

if (results != null && results.Count > 0)
{
countRead += results.Count;
if (ReadCount == 1)
{
ProviderInvocationException providerException =
new(
"ProviderContentReadError",
SessionStateStrings.ProviderContentReadError,
holder.PathInfo.Provider,
holder.PathInfo.Path,
e);

// Log a provider health event
MshLog.LogProviderHealthEvent(
this.Context,
holder.PathInfo.Provider.Name,
providerException,
Severity.Warning);

WriteError(new ErrorRecord(
providerException.ErrorRecord,
providerException));

break;
// Write out the content as a single object
WriteContentObject(results[0], countRead, holder.PathInfo, currentContext);
}

if (results != null && results.Count > 0)
else
{
countRead += results.Count;
if (ReadCount == 1)
{
// Write out the content as a single object
WriteContentObject(results[0], countRead, holder.PathInfo, currentContext);
}
else
{
// Write out the content as an array of objects
WriteContentObject(results, countRead, holder.PathInfo, currentContext);
}
// Write out the content as an array of objects
WriteContentObject(results, countRead, holder.PathInfo, currentContext);
}
} while (results != null && results.Count > 0 && ((TotalCount < 0) || countRead < TotalCount));
}
}
} while (results != null && results.Count > 0 && (TotalCount == -1 || countRead < TotalCount));
}
}
finally
{
// close all the content readers
// Close all the content readers

CloseContent(contentStreams, false);

Expand All @@ -284,7 +234,7 @@ private bool ScanForwardsForTail(in ContentHolder holder, CmdletProviderContext
{
var fsReader = holder.Reader as FileSystemContentReaderWriter;
Dbg.Diagnostics.Assert(fsReader != null, "Tail is only supported for FileSystemContentReaderWriter");
var tailResultQueue = new Queue<object>();
Queue<object> tailResultQueue = new();
IList results = null;
ErrorRecord error = null;

Expand Down Expand Up @@ -327,7 +277,10 @@ private bool ScanForwardsForTail(in ContentHolder holder, CmdletProviderContext
foreach (object entry in results)
{
if (tailResultQueue.Count == Tail)
{
tailResultQueue.Dequeue();
}

tailResultQueue.Enqueue(entry);
}
}
Expand All @@ -349,21 +302,25 @@ private bool ScanForwardsForTail(in ContentHolder holder, CmdletProviderContext
{
// Write out the content as single object
while (tailResultQueue.Count > 0)
{
WriteContentObject(tailResultQueue.Dequeue(), count++, holder.PathInfo, currentContext);
}
}
else // ReadCount < Queue.Count
{
while (tailResultQueue.Count >= ReadCount)
{
var outputList = new List<object>((int)ReadCount);
List<object> outputList = new((int)ReadCount);
for (int idx = 0; idx < ReadCount; idx++, count++)
{
outputList.Add(tailResultQueue.Dequeue());
}

// Write out the content as an array of objects
WriteContentObject(outputList.ToArray(), count, holder.PathInfo, currentContext);
}

int remainder = tailResultQueue.Count;
if (remainder > 0)
if (tailResultQueue.Count > 0)
{
// Write out the content as an array of objects
WriteContentObject(tailResultQueue.ToArray(), count, holder.PathInfo, currentContext);
Expand Down
12 changes: 12 additions & 0 deletions test/powershell/Modules/Microsoft.PowerShell.Management/Get-Content.Tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,14 @@ Describe "Get-Content" -Tags "CI" {
Get-Content -Path $testPath2 -Last 1 | Should -BeExactly $fifthline
}

It 'Verifies -TotalCount reports a ParameterArgumentValidationError error for negative values' {
{Get-Content -Path $testPath2 -TotalCount -2} | Should -Throw -ErrorId 'ParameterArgumentValidationError,Microsoft.PowerShell.Commands.GetContentCommand'
}

It 'Verifies -Tail reports a ParameterArgumentValidationError error for negative values' {
{Get-Content -Path $testPath2 -Tail -2} | Should -Throw -ErrorId 'ParameterArgumentValidationError,Microsoft.PowerShell.Commands.GetContentCommand'
}

It "Should be able to get content within a different drive" {
Push-Location env:
$expectedoutput = [Environment]::GetEnvironmentVariable("PATH");
Expand Down Expand Up @@ -271,6 +279,10 @@ Describe "Get-Content" -Tags "CI" {
Get-Content -Path $testPath -TotalCount 0 | Should -BeNullOrEmpty
}

It "Should return no content when -Tail value is 0" {
Get-Content -Path $testPath -Tail 0 | Should -BeNullOrEmpty
}

It "Should throw TailAndHeadCannotCoexist when both -Tail and -TotalCount are used" {
{
Get-Content -Path $testPath -Tail 1 -TotalCount 1 -ErrorAction Stop
Expand Down