rust/rhg/src/commands/files.rs
30–33	Note that "no" is not going to be enough since they are many way to express "false" for config boolean. Maybe reverse the logic and fallback for anything that is not the default config ?

Alphare added a subscriber: Alphare.Mar 10 2021, 3:53 AM

Alphare added inline comments.

rust/rhg/src/commands/files.rs
30–33	You could use `get_bool` first?

SimonSapin added inline comments.Mar 11 2021, 4:49 AM

rust/rhg/src/commands/files.rs
30–33	This config is not quite a boolean since it also has a `legacy` value, where `get_bool` would return an error. We could use `.is_some()` and fall back for any value other than the implicit default as I think marmoute suggested, which is more often than necessary. We could also implement the full parsing for this value, but then that leaves us dealing with `legacy` which resolves to either true or false depending on who’s asking.

marmoute added inline comments.Mar 12 2021, 11:51 AM

rust/rhg/src/commands/files.rs
30–33	I vote for doing the dummest correct things for now. And revisite this later.

SimonSapin retitled this revision from rhg: Fall back to Python if ui.relative-paths=no is configured to rhg: Fall back to Python if ui.relative-paths is configured.Mar 12 2021, 5:07 PM

SimonSapin edited the summary of this revision. (Show Details)

SimonSapin updated this revision to Diff 26297.

marmoute added inline comments.Mar 15 2021, 5:42 AM

rust/rhg/src/commands/files.rs
32	Is the error message still valid ?

✅ refresh by Heptapod after a successful CI run (🐙 💚)

Alphare accepted this revision.Mar 16 2021, 4:52 AM

SimonSapin added a commit: rHGc184b490da37: rhg: Fall back to Python if ui.relative-paths is configured.Mar 16 2021, 5:47 PM

This revision was not accepted when it landed; it landed in state Needs Review.

Closed by commit rHGc184b490da37: rhg: Fall back to Python if ui.relative-paths is configured (authored by SimonSapin). · Explain Why

This revision was automatically updated to reflect the committed changes.

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	26176		Mar 9 2021, 4:40 AM	★	★
Diff 2	26196		Mar 9 2021, 8:40 AM	★	★
Diff 3	26297		Mar 12 2021, 5:07 PM	★	★
Diff 4	26343		Mar 15 2021, 9:10 AM	★	★
Diff 5	26420	rHGc184b490da37d76b61852b77513964301df4fc8b	Mar 5 2021, 7:28 AM	★	★

	.long("--revision")			.long("--revision")
	.value_name("REV")			.value_name("REV")
	.takes_value(true),			.takes_value(true),
	)			)
	.about(HELP_TEXT)			.about(HELP_TEXT)
	}			}

	pub fn run(invocation: &crate::CliInvocation) -> Result<(), CommandError> {			pub fn run(invocation: &crate::CliInvocation) -> Result<(), CommandError> {
				let relative = invocation.config.get(b"ui", b"relative-paths");
				if relative.is_some() {
				return Err(CommandError::unsupported(
				marmouteUnsubmitted Not Done Is the error message still valid ? marmoute: Is the error message still valid ?
				"non-default ui.relative-paths",
				marmouteUnsubmitted Not Done Note that "no" is not going to be enough since they are many way to express "false" for config boolean. Maybe reverse the logic and fallback for anything that is not the default config ? marmoute: Note that "no" is not going to be enough since they are many way to express "false" for config…
				AlphareUnsubmitted Not Done You could use `get_bool` first? Alphare: You could use `get_bool` first?
				SimonSapinAuthorUnsubmitted Done This config is not quite a boolean since it also has a `legacy` value, where `get_bool` would return an error. We could use `.is_some()` and fall back for any value other than the implicit default as I think marmoute suggested, which is more often than necessary. We could also implement the full parsing for this value, but then that leaves us dealing with `legacy` which resolves to either true or false depending on who’s asking. SimonSapin: This config is not quite a boolean since it also has a `legacy` value, where `get_bool` would…
				marmouteUnsubmitted Not Done I vote for doing the dummest correct things for now. And revisite this later. marmoute: I vote for doing the dummest correct things for now. And revisite this later.
				));
				}

	let rev = invocation.subcommand_args.value_of("rev");			let rev = invocation.subcommand_args.value_of("rev");

	let repo = invocation.repo?;			let repo = invocation.repo?;
	if let Some(rev) = rev {			if let Some(rev) = rev {
	let files = list_rev_tracked_files(repo, rev).map_err(\|e\| (e, rev))?;			let files = list_rev_tracked_files(repo, rev).map_err(\|e\| (e, rev))?;
	display_files(invocation.ui, repo, files.iter())			display_files(invocation.ui, repo, files.iter())
	} else {			} else {
	let distate = Dirstate::new(repo)?;			let distate = Dirstate::new(repo)?;

This is an archive of the discontinued Mercurial Phabricator instance.

rhg: Fall back to Python if ui.relative-paths is configured
ClosedPublic

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 26420

rust/rhg/src/commands/files.rs