I personally would prefer to put this check up a layer in determineTreeRoot. If somebody explicitly specifies --tree-root "", I'd also like them to get this error, rather than having it fall back to the current directory.

If somebody explicitly specifies --tree-root " "

I was about to comment that "" would be treated as tree-root being undefined, however if a whitespace-only value is defined then you're right, it would run into this edge-case.

Oops, good point. I didn't remember we're treating "unset" and "set to empty string" as the same thing here.

I still do feel like it would make more sense to do this check as part of the "is this potential root a valid path? let's normalize it" codepath, and not in the implementation of a specific root discovery mechanism.

I'm not quite following what is being advocated for here.

I believe @jfly is suggesting the new assertion for non-empty (trimmed) stdout could also apply to other methods of setting the tree root.

Currently it is possible to explicitly define tree-root = " ", which would bypass the tree-root != "" checks.

IMO this begs the question of whether trimming stdout is the correct approach anyway. If not, this argument would be kinda irrelevant.

However trimming is good enough for an initial implementation, since valid filepaths with leading/trailing whitespace seem very unlikely, and the command printing additional blank lines is rather likely.

IMO this begs the question of whether trimming stdout is the correct approach anyway.

In hindsight, I believe the most correct approach here would be to split on \n, then remove the empty lines.

You can then assert that the remaining line count is 1, and also use the lines slice to log the stdout line-by-line.

lines := strings.Split(stdout, "\n")
nonEmptyLines := slices.DeleteFunc(lines, func(line string) bool {
	return line == ""
}

switch (len(nonEmptyLines)) {
case 1:
	// no-op

case 0:
	return "", fmt.Errorf("empty output received after executing tree-root-cmd: %s", cfg.TreeRootCmd)

default:
	// Log all stdout line-by-line, including the blank lines
	logger := log.WithPrefix("tree-root-cmd | stdout")
	for _, line := range lines {
		logger.Error(line)
	}
	return "", fmt.Errorf("tree-root-cmd cannot output multiple lines: %s", cfg.TreeRootCmd)
}

As said before, trimming is fine for the initial impl, I can open a follow up PR for this if preferred.

All mechanisms must pass through the same stage of realising their output as absolute paths, which should throw an error for any malformed paths. Trimming the output of the command execution and enforcing a single line seems prudent, specifically for the tree-root-cmd pathway.

which should throw an error for any malformed paths

This is the part that bothers me: IIRC, we're using go's built-in Path.abs to do this, which accepts empty strings rather than erroring.

This is the part that bothers me: IIRC, we're using go's built-in Path.abs to do this, which accepts empty strings rather than erroring.

I believe it is impossible to define a truly empty tree-root path; attempting to set tree-root = "" will be treated as undefined. Returning an empty string from tree-root-cmd will be caught here.

As for blank/whitespace paths, a whitepace string is technically a valid relative path. On some filesystems you can name a file/dir " " (if you really wanted to).

This is one reason why the current impl splits stdout into lines, instead of just trimming it.

IMO this begs the question of whether trimming stdout is the correct approach anyway.

In hindsight, I believe the most correct approach here would be to split on \n, then remove the empty lines.

You can then assert that the remaining line count is 1, and also use the lines slice to log the stdout line-by-line.

lines := strings.Split(stdout, "\n")
nonEmptyLines := slices.DeleteFunc(lines, func(line string) bool {
	return line == ""
}

switch (len(nonEmptyLines)) {
case 1:
	// no-op

case 0:
	return "", fmt.Errorf("empty output received after executing tree-root-cmd: %s", cfg.TreeRootCmd)

default:
	// Log all stdout line-by-line, including the blank lines
	logger := log.WithPrefix("tree-root-cmd | stdout")
	for _, line := range lines {
		logger.Error(line)
	}
	return "", fmt.Errorf("tree-root-cmd cannot output multiple lines: %s", cfg.TreeRootCmd)
}

As said before, trimming is fine for the initial impl, I can open a follow up PR for this if preferred.