Conversation
There was a problem hiding this comment.
Pull request overview
Updates DARC’s remote file-fetching logic to avoid a null-reference exception when the optional baseDirectory is not provided, specifically in GetCommonScriptFilesAsync.
Changes:
- Coalesce
baseDirectoryto an empty path and build a singlecommonScriptsPath. - Pass the computed
commonScriptsPathinto_remoteGitClient.GetFilesAtCommitAsync. - Update logging to report the computed common scripts path.
| baseDirectory = baseDirectory ?? UnixPath.Empty; | ||
|
|
||
| List<GitFile> files = await _remoteGitClient.GetFilesAtCommitAsync(repoUri, commit, path); | ||
| string commonScriptsPath = baseDirectory / Constants.CommonScriptFilesPath; | ||
|
|
||
| List<GitFile> files = await _remoteGitClient.GetFilesAtCommitAsync(repoUri, commit, commonScriptsPath); |
There was a problem hiding this comment.
When baseDirectory is null this now becomes UnixPath.Empty, but commonScriptsPath = baseDirectory / ... uses the LocalPath / operator (compile-time type), which will combine empty left + right into a path with a leading / (e.g. /eng/common). Previously the null case used Constants.CommonScriptFilesPath (eng/common) without the leading slash. Consider preserving the old behavior (keep the conditional), or cast/convert to UnixPath before combining so UnixPath's empty-path handling is used.
There was a problem hiding this comment.
public static UnixPath operator /(UnixPath left, string right)
=> IsEmptyPath(left)
? new(right)
: new(left.Combine(left.Path, left.NormalizePath(right)), false);UnixPath.Empty combined with another path should not produce any extra slashes according to this implementation
| if (stripBaseDirectory) | ||
| { |
There was a problem hiding this comment.
The stripBaseDirectory branch below uses f.FilePath.TrimStart(baseDirectory).ToString() which does not remove a base-directory prefix; it trims any leading characters contained in baseDirectory (via MemoryExtensions.TrimStart on spans). For base directories like src/arcade, this can corrupt paths (e.g., stripping the leading e from eng/...). Consider replacing that logic with explicit prefix/path-segment removal (e.g., StartsWith baseDirectory + separator and slice/replace only that prefix).
#5830