feat: find tree root via a user-specified command

[brianmcgee]

Closes #530

[zimbatm]

it's good

[brianmcgee]

@MattSturgeon, do you mind giving this a review as well?

[MattSturgeon]

LGTM

The concept looks good to me, thanks! I'll defer to @jfly for the more technical review.

shlex

I do wonder about the merit of using shlex vs either requiring that multi-arg commands be configured as a list, or evaluated using a real shell (i.e. bash).

treefmt-nix

Slightly off topic, but I wonder what implications this has for treefmt-nix?

Now that git rev-parse --show-toplevel is used by default, does that mean that treefmt-nix no longer needs to provide a default for tree-root-file, projectRootFile (default ".git/config"), projectRoot (default self¹), etc?

Also, how should the change be communicate to users who have gotten in the habit of setting tree-root-file or projectRootFile themselves, as that is now likely to be redundant?

Footnotes

1. Tangent: does projectRoot's current default self make sense? Since that'll be an in-store path, and won't ever be used anyway because projectRootFile is always defined ↩

[brianmcgee]

Took another pass and fixed some bad assumptions.

Fresh reviews have been requested 🙏

[jfly]

I find the new determineTreeRoot function to be way more readable than before, thank you!

[brianmcgee]

treefmt-nix

Slightly off topic, but I wonder what implications this has for treefmt-nix?

Now that git rev-parse --show-toplevel is used by default, does that mean that treefmt-nix no longer needs to provide a default for tree-root-file, projectRootFile (default ".git/config"), projectRoot (default self1), etc?

Correct.

The default root file of .git/config can be removed. I'll apply that change when I next update treefmt-nix to incorporate this change (after the release).

Also, how should the change be communicate to users who have gotten in the habit of setting tree-root-file or projectRootFile themselves, as that is now likely to be redundant?

This is a good point. It will be in the release notes, and I can point it out on Fosstodon as well.

A Nixos discourse post might also be appropriate?

[jfly]

Nice work!

[jfly]

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.

[MattSturgeon]

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.

[jfly]

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.

[brianmcgee]

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

[MattSturgeon]

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.

[MattSturgeon]

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.

[brianmcgee]

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.

[jfly]

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.

[MattSturgeon]

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.

[MattSturgeon]

LGTM overall. Thanks again for working on this!

A few minor things outstanding most of which you probably don't need to block on if you don't want to:

• Docs for how tree-root-cmd is parsed (feat: find tree root via a user-specified command #571 (comment))
• Maybe docs for where tree-root-cmd is run (feat: find tree root via a user-specified command #571 (comment))
• Update docs for tree-root's default (feat: find tree root via a user-specified command #571 (comment))
• Maybe also checking the trimmed treeRoot is non-empty for explicit tree-root defs (feat: find tree root via a user-specified command #571 (comment))
• Packaging nits
  (feat: find tree root via a user-specified command #571 (comment))
  (feat: find tree root via a user-specified command #571 (comment))
• A couple more nits below:

[MattSturgeon]

Sorry to review again with such minor comments... Despite the noise, the PR is looking really good!

[MattSturgeon]

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.

[brianmcgee]

Sorry to review again with such minor comments... Despite the noise, the PR is looking really good!

I don't mind, I'd rather get it right.

[MattSturgeon]

Only things outstanding for me are

• Removing empty lines instead of arbitrarily trimming stdout
  (feat: find tree root via a user-specified command #571 (comment))
• Having a test case for git-repo & no defined tree-root option
  (feat: find tree root via a user-specified command #571 (comment))

Neither of these are blocking IMO, so I approve 🎉

(I haven't tested locally, but the diff & tests all look good)