Fix eng/common cashing

[dkurepa]

#4751

[Copilot]

Pull Request Overview

This PR addresses caching improvements by updating methods to include an additional repository flag (repoIsVmr), ensuring that caching keys are correctly differentiated for multiple repositories.

• Updated method signatures and calls to include the repoIsVmr parameter.
• Adjusted caching key composition in GitHubClient.
• Introduced an extraneous variable which may need removal.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/Microsoft.DotNet.Darc/DarcLib/RemoteRepoBase.cs	Unused variable introduced in CommitFilesAsync
src/Microsoft.DotNet.Darc/DarcLib/Remote.cs	Updated GetFilesAtCommitAsync signature to pass repoIsVmr
src/Microsoft.DotNet.Darc/DarcLib/IRemoteGitRepo.cs	Updated interface signature for GetFilesAtCommitAsync
src/Microsoft.DotNet.Darc/DarcLib/GitHubClient.cs	Updated method signatures, parameter propagation, and caching key formation
src/Microsoft.DotNet.Darc/DarcLib/AzureDevOpsClient.cs	Updated GetFilesAtCommitAsync signature to pass repoIsVmr

[mmitche]

Why would we need to specify isVMR or not? This implies that treeItem.Path is the same between an VMR-sourced arcade update and an arcade-sourced arcade update. Could we instead of treeItem.Path specify a full relative repo path? That way there is never any overlap.

[dkurepa]

Why would we need to specify isVMR or not? This implies that treeItem.Path is the same between an VMR-sourced arcade update and an arcade-sourced arcade update. Could we instead of treeItem.Path specify a full relative repo path? That way there is never any overlap.

we currently don't have it, but I think we could add it to some wrapper while we're traversing the tree, if you think that'd be a better solution

[mmitche]

we're traversing the tree, if you think that'd be a better solution

I think a more general solution would be passing a more complete path down, if it's feasible.

[dkurepa]

we're traversing the tree, if you think that'd be a better solution

I think a more general solution would be passing a more complete path down, if it's feasible.

I debugged the code, I got it a bit wrong on my first try. The blob type trees have their full path as the Path property from the root of the tree. So for example the blob tree containing src\arcade\eng\common\core-templates\steps\cleanup-microbuild.yml will have core-templates\steps\cleanup-microbuild.yml as the Path, and the path variable we already have in the method is the true path to eng/common, so it's src/arcade/eng/common for the VMR and eng/common for non VMR, so just adding that to the key makes things work too

[dkurepa]

Suggested change

	return await Cache.GetOrCreateAsync((path, treeItem.Path, treeItem.Sha), async (entry) =>
	return await Cache.GetOrCreateAsync(($"{path}/{treeItem.Path}", treeItem.Sha), async (entry) =>

we could also do this, but I don't think we need the extra string interpolation, think the above is more efficient.