cli: Make fix tools run from the repo root.

[matts1]

Fixes #4866

Checklist

If applicable:

• I have updated CHANGELOG.md
• I have updated the documentation (README.md, docs/, demos/)
• I have updated the config schema (cli/src/config-schema.json)
• I have added tests to cover my changes

[yuja]

Doesn't it break formatter configuration? Ideally config files should be loaded from the project root.

[matts1]

Would I be correct in saying that the following failure, which occurs only on windows, is a bug in jj itself?

---- test_fix_command::test_executed_but_nothing_changed stdout ----
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ Snapshot Summary ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Snapshot: executed_but_nothing_changed-2
Source: D:\a\jj\jj:816
───────────────────────────────────────────────────────────────────────────────
Expression: redact(&stderr.replace(logfile_path_str, "$logfile"))
───────────────────────────────────────────────────────────────────────────────
-old snapshot
+new results
────────────┬──────────────────────────────────────────────────────────────────
    0     0 │ Warning: The `fix.tool-command` config option is deprecated and will be removed in a future version.
    1     1 │ Hint: Replace it with the following:
    2     2 │             [fix.tools.legacy-tool-command]
    3       │-            command = [<redacted formatter path>, "--tee", "$logfile"]
          3 │+            command = [<redacted formatter path>, "--tee", '$logfile']
    4     4 │             patterns = ["all()"]
    5     5 │             
    6     6 │ Fixed 0 commits of 1 checked.
    7     7 │ Nothing changed.
────────────┴──────────────────────────────────────────────────────────────────

It would seem that on windows, the output of the deprecation command is command = ["formatter", "--tee", '$logfile']. I don't have a a windows computer to test on, so I can't really debug, but that seems incorrect. This seems wierd because it was previously working.

[martinvonz]

Would I be correct in saying that the following failure, which occurs only on windows, is a bug in jj itself?

The message comes from here:

jj/cli/src/commands/fix.rs

Lines 465 to 473 in c5b93a0

	writeln!(
	ui.hint_default(),
	r###"Replace it with the following:
	[fix.tools.legacy-tool-command]
	command = {}
	patterns = ["all()"]
	"###,
	to_toml_value(&config.get::<config::Value>("fix.tool-command").unwrap()).unwrap()
	)?;

The command = ... line in particular comes from here:

jj/cli/src/commands/fix.rs

Line 472 in c5b93a0

to_toml_value(&config.get::<config::Value>("fix.tool-command").unwrap()).unwrap()

to_toml_value() is defined here:

jj/cli/src/config.rs

Lines 52 to 76 in c5b93a0

	pub fn to_toml_value(value: &config::Value) -> Result<toml_edit::Value, config::ConfigError> {
	fn type_error<T: fmt::Display>(message: T) -> config::ConfigError {
	config::ConfigError::Message(message.to_string())
	}
	// It's unlikely that the config object contained unsupported values, but
	// there's no guarantee. For example, values coming from environment
	// variables might be big int.
	match value.kind {
	config::ValueKind::Nil => Err(type_error(format!("Unexpected value: {value}"))),
	config::ValueKind::Boolean(v) => Ok(v.into()),
	config::ValueKind::I64(v) => Ok(v.into()),
	config::ValueKind::I128(v) => Ok(i64::try_from(v).map_err(type_error)?.into()),
	config::ValueKind::U64(v) => Ok(i64::try_from(v).map_err(type_error)?.into()),
	config::ValueKind::U128(v) => Ok(i64::try_from(v).map_err(type_error)?.into()),
	config::ValueKind::Float(v) => Ok(v.into()),
	config::ValueKind::String(ref v) => Ok(v.into()),
	// TODO: Remove sorting when config crate maintains deterministic ordering.
	config::ValueKind::Table(ref table) => table
	.iter()
	.sorted_by_key(|(k, _)| *k)
	.map(|(k, v)| Ok((k, to_toml_value(v)?)))
	.collect(),
	config::ValueKind::Array(ref array) => array.iter().map(to_toml_value).collect(),
	}
	}

That doesn't seem to have any platform-specific code and just returns a toml_edit::Value. That type presumably doesn't contain the quotation marks, so maybe the problem is in how we convert it to a string in the second snippet above. Hmm, I can't find anything platform-dependent there either.

Oh, somehow I had not looked at the redact() function until now. It has this comment:

jj/cli/tests/test_fix_command.rs

Lines 47 to 54 in 068fa0f

	snapshot.replace(
	&if cfg!(windows) {
	format!(r#"'{}'"#, formatter_path.to_str().unwrap())
	} else {
	format!(r#""{}""#, formatter_path.to_str().unwrap())
	},
	"<redacted formatter path>",
	)

That took me quite a while to find. So maybe the redact() closure should be updated to also replace the workspace root by a placeholder?

[martinvonz]

LGTM, but we work for the same company, so I'll leave actual approval to someone else.

[yuja]

nit: insert blank line before

[yuja]

Shouldn't it be workspace_root()? Maybe we can add a fake-formatter option that stat/read a file to test the behavior.

fwiw, this can be inlined as tx.base_workspace_helper()....