A couple of issues in ISourceControlProvider and FSourceControlModule

Development Programming & Scripting C++
, , , ,
1

Thanks for the feedback!

The issue you see where status is conflated with bool is a result of a minor refactor that went on before release. It looks like not all the locations that were being used were caught.

As for the lack of NULL checking, this is likely because none of our providers currently return NULL states in any circumstances.

I’ll look to get these issues fixed up!

2

The first issue is very minor, ISourceControlProvider::GetState() is implemented like so:

virtual TSharedPtr<ISourceControlState, ESPMode::ThreadSafe> GetState( const FString& InFile, EStateCacheUsage::Type InStateCacheUsage )
{
	TArray<FString> Files;
	TArray< TSharedRef<ISourceControlState, ESPMode::ThreadSafe> > States;
	Files.Add(InFile);
	if(GetState(Files, States, InStateCacheUsage))
	{
		TSharedRef<ISourceControlState, ESPMode::ThreadSafe> State = States[0];
		return State;
	}
	return NULL;
}

What’s wrong with this line?

if(GetState(Files, States, InStateCacheUsage))

That call to GetState() returns an ECommandResult::Type not a bool, regardless everything works correctly for now because ECommandResult::Failed is zero.

The second issue is in FSourceControlModule::QueueStatusUpdate():

void FSourceControlModule::QueueStatusUpdate(const FString& InFilename)
{
	if(IsEnabled())
	{
		TSharedPtr<ISourceControlState, ESPMode::ThreadSafe> SourceControlState = GetProvider().GetState(InFilename, EStateCacheUsage::Use);
		FTimespan TimeSinceLastUpdate = FDateTime::Now() - SourceControlState->GetTimeStamp();
		if(TimeSinceLastUpdate > SourceControlConstants::StateRefreshInterval)
		{
			PendingStatusUpdateFiles.AddUnique(InFilename);
		}
	}
}

If you look back at the first code snippet I posted GetProvider().GetState(InFilename, EStateCacheUsage::Use) may return NULL, if it does the SourceControlState pointer will not be valid, and when the following line calls SourceControlState->GetTimeStamp() bad things happen.

I’m working on a source control provider for Mercurial and ran into this because I haven’t implemented ISourceControlState yet, it may well be that it makes no sense to have no state for a any given file but if so that should be made obvious by some well placed check() asserts :slight_smile: