It seems like this is actually interspersing two pieces of logic:

• Determining the encoding we want to use
• Creating the file stream writer

If that's the case, it might be nicer to factor the first into its own method, similar to the pre-existing code:

private Encoding GetPathEncoding()
{
    using (var reader = new StreamReader(Path, Utils.utf8NoBom, detectEncodingFromByteOrderMarks: true))
    {
        _ = reader.Read();
       return reader.CurrentEncoding;
    }
}

internal void FlushContentToDisk()
{
    lock (OutputBeingLogged)
    {
        try
        {
            // Does this dispose check need to be within the lock at all?
            if (disposed)
            {
                return;
            }

            if (_contentWriter != null)
            {
                return;
            }

            try
            {
                _contentWriter = new StreamWriter(
                    new FileStream(Path, FileMode.OpenOrCreate, FileAccess.ReadWrite),
                    GetPathEncoding());

                // Worth adding a comment here to explain what's happening
                _contentWriter.BaseStream.Seek(0, SeekOrigin.End);
            }
            catch (IOException)
            {
                _contentWriter = new StreamWriter(
                    new FileStream(Path, FileMode.Append, FileAccess.Write, FileShare.Read),
                    Utils.utf8NoBom);
            }
        }
        finally
        {
            OutputBeingLogger.Clear();
        }
    }
}

That's what I had initially, with the GetPathEncoding defined in the Utils class, but I think @iSazonov didn't like the method there.

In the end, it will be used only in this FlushContentToDisk, maybe it should be a local function? This way it is factored, but still available only in this method?

Otherwise I can put it back into Utils, in the end it's still valid, even more so if we just let the IOException bubble if there's an issue reading the file.

If a short method is used once I don't see a point to have it (especially since it should be in
try-catch block). We could add a comment before the code block.

@rjmholt what do you think?

Well I don't like making you go around in circles. So I'm happy to leave the code as it is.

@iSazonov the current PowerShell code is full of long tracts of statements, and breaking things into methods is an easy way to get readability and also improves reusability later on. Honestly I just find it a useful tool to simplify the process for writing and reviewing code. That's where my preference comes from. And given the complexity of some parts of our codebase, I don't see much argument against it.

especially since it should be in try-catch block

This is the kind of clarity that putting something into a method provides. As is, it's not clear what the full intent and purpose of the try/catch is (why do we fall back to a totally different setting in the catch block?). Once you separate the logic into two pieces and must give them a human-readable name, it's easier to assess what each operation's error semantics should be.

I don't mind changing things, in the end the goal was to make the code more readable / simple, and I tend to agree that it would probably be clearer in a method, which makes it clear why we're creating this StreamReader here.

Since what seems to bother @iSazonov is the fact that the method would be used only in this method, I think it's a good candidate for a local function, it groups the logic while keeping it "inside" the method and usable only there. Do you think this would be an acceptable proposal?

@rjmholt Thanks for clarify!
I see your point and agree that a method with an informative name adds readability. But in this case it seemed to me unnecessary. We have a request to create a cmdlet like Get-FileEncoding but I don't think this piece of code is worth worrying about reuse.
As for this try-catch block, I was afraid that we could change something in the behavior and get regression now or in the future.

So, as discussed I did a test with a local function. I think it's a good middle ground between your two opinions, we have the code factored and with an understandable name and at the same time it's available only in the FlushContentToDisk method.
Waiting on your remarks.

I think a local function sounds ideal 🙂

Please moved the local function to start of the method.
Also please use Pascal name case - local function is de-facto a method and could be move in another scope.

Done 😄. Interesting that you prefer the local function at the start. I haven't seen any recommendation for local functions yet. Do you happen to have coding conventions available for them?

In my team we prefer to put it at the bottom, this way you have "pseudo-code" at the beginning with only the calls and you can just scroll down to see the implementation details.