This is an archive of the discontinued Mercurial Phabricator instance.

Differential D10189

rhg: Remove `rhg.fallback-executable=hg` default configuration
ClosedPublic

Authored by SimonSapin on Mar 12 2021, 5:08 PM.

Download Raw Diff

Details

Reviewers

marmoute

Group Reviewers

hg-reviewers

Commits

rHGbde90e9b4507: rhg: Remove `rhg.fallback-executable=hg` default configuration

Summary

When rhg.on-unsupported is configured to fallback and an unsupported
feature is encountered, the previous default was to look for an hg
executable in $PATH.

This default was fragile since it was easy to end up accidentally using
an older version of Mercurial installed system-wide,
when a local (perhaps patched) installation was intended.

Instead, it is now an error to have rhg.on-unsupported=fallback
without also configuring an explicit path or the fallback executable.

Diff Detail

Repository

rHG Mercurial

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

SimonSapin created this revision.Mar 12 2021, 5:08 PM

Herald added a reviewer: hg-reviewers. · View Herald TranscriptMar 12 2021, 5:08 PM

Herald added a subscriber: mercurial-patches. · View Herald Transcript

SimonSapin added a child revision: D10190: tests: Disable for rhg remaining tests that fail in that mode.Mar 12 2021, 5:08 PM

SimonSapin mentioned this in D10094: rhg: Make fallback to Python the default behavior.Mar 12 2021, 5:15 PM

Looks overall good, with maybe a folow up for the message.

rust/rhg/src/main.rs
411–412	That message in a bit long, what about: abort: 'rhg.on-unsupported=fallback' without 'rhg.fallback-executable' set. to get < 80 char ?

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

SimonSapin added a commit: rHGbde90e9b4507: rhg: Remove `rhg.fallback-executable=hg` default configuration.Mar 16 2021, 5:48 PM

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

Closed by commit rHGbde90e9b4507: rhg: Remove `rhg.fallback-executable=hg` default configuration (authored by SimonSapin). · Explain Why

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

			Path	Packages
M			rust/rhg/src/main.rs (34 lines)
M			tests/test-rhg.t (8 lines)

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	26303		Mar 12 2021, 5:08 PM	★	★
Diff 2	26352		Mar 15 2021, 9:10 AM	★	★
Diff 3	26429	rHGbde90e9b4507a96916d50772d3bf1aec3121c1fd	Mar 12 2021, 3:44 PM	★	★

Status	Author	Revision
Closed	SimonSapin	D10132 rhg: Ignore trailing newlines in .hg/sharedpath
Closed	SimonSapin	D10196 ci: Add a job testing with rhg installed as `hg`
Closed	SimonSapin	D10195 run-test: install rhg if --rhg is passed
Closed	SimonSapin	D10194 makefile: add a install option
Closed	SimonSapin	D10193 makefile: add a build-rhg option
Closed	SimonSapin	D10192 makefile: add a install-chg option
Closed	SimonSapin	D10191 makefile: add a build-chg option
Closed	SimonSapin	D10190 tests: Disable for rhg remaining tests that fail in that mode
Closed	SimonSapin	D10189 rhg: Remove `rhg.fallback-executable=hg` default configuration
Closed	SimonSapin	D10145 rhg: Return an error code for `rhg config Section.idontexist`
Closed	SimonSapin	D10144 rhg: Fall back to Python for --version
Closed	SimonSapin	D10143 rhg: Exit with an error code if `files` finds nothing
Closed	SimonSapin	D10142 rhg: `cat` command: print error messages for missing files
Closed	SimonSapin	D10141 rhg: Silently ignore missing files in config %include
Closed	SimonSapin	D10140 rhg: Add support for environment variables in config include paths
Closed	SimonSapin	D10139 rhg: Add support for the HGRCSKIPREPO environment variable
Closed	SimonSapin	D10138 rhg: Don’t make repository path absolute too early
Closed	SimonSapin	D10137 rhg: Fall back to Python if ui.relative-paths is configured
Closed	SimonSapin	D10136 tests: clarify some missing output in test-merge-subrepos
Closed	SimonSapin	D10135 tests: Adapt expected output for minor differences with rhg
Closed	SimonSapin	D10134 rhg: Add support for --cwd
Closed	SimonSapin	D10133 rhg: Align with Python on some more error messages
Closed	SimonSapin	D10188 rhg: Add an allow-list of ignored extensions
Closed	SimonSapin	D10112 rhg: Fall back to Python if unsupported extensions are enabled
Closed	SimonSapin	D10111 rhg: Sort config files when adding a directory
Closed	SimonSapin	D10110 rhg: Align config file parse error formatting with Python
Closed	SimonSapin	D10102 rhg: Fall back to Python for bundle repositories
Closed	SimonSapin	D10101 rhg: Fall back to Python on --repository with an URL
Closed	SimonSapin	D10100 rhg: Print non-absolutized path in "repository {} not found" errors
Closed	SimonSapin	D10099 rhg: Align "malformed --config" error message with Python
Closed	SimonSapin	D10098 rhg: Fall back to Python on unsupported `rhg config <section>`
Closed	SimonSapin	D10097 rhg: Fall back to Python for unsupported revset syntax
Closed	SimonSapin	D10096 tests: Add `rhg` and `no-rhg` for #require and #if in .t files
Closed	SimonSapin	D10187 tests: Enable rhg fallback to Python by default in tests
Closed	SimonSapin	D10186 rhg: Add environment variables for fallback configuration

Diff 26429

rust/rhg/src/main.rs

	static ref SCHEME_RE: regex::bytes::Regex =			static ref SCHEME_RE: regex::bytes::Regex =
	// Same as `_matchscheme` in `mercurial/util.py`			// Same as `_matchscheme` in `mercurial/util.py`
	regex::bytes::Regex::new("^[a-zA-Z0-9+.\\-]+:").unwrap();			regex::bytes::Regex::new("^[a-zA-Z0-9+.\\-]+:").unwrap();
	}			}
	if SCHEME_RE.is_match(&repo_path_bytes) {			if SCHEME_RE.is_match(&repo_path_bytes) {
	exit(			exit(
	&initial_current_dir,			&initial_current_dir,
	&ui,			&ui,
	OnUnsupported::from_config(&non_repo_config),			OnUnsupported::from_config(&ui, &non_repo_config),
	Err(CommandError::UnsupportedFeature {			Err(CommandError::UnsupportedFeature {
	message: format_bytes!(			message: format_bytes!(
	b"URL-like --repository {}",			b"URL-like --repository {}",
	repo_path_bytes			repo_path_bytes
	),			),
	}),			}),
	)			)
	}			}
	}			}
	let repo_path = early_args.repo.as_deref().map(get_path_from_bytes);			let repo_path = early_args.repo.as_deref().map(get_path_from_bytes);
	let repo_result = match Repo::find(&non_repo_config, repo_path) {			let repo_result = match Repo::find(&non_repo_config, repo_path) {
	Ok(repo) => Ok(repo),			Ok(repo) => Ok(repo),
	Err(RepoError::NotFound { at }) if repo_path.is_none() => {			Err(RepoError::NotFound { at }) if repo_path.is_none() => {
	// Not finding a repo is not fatal yet, if `-R` was not given			// Not finding a repo is not fatal yet, if `-R` was not given
	Err(NoRepoInCwdError { cwd: at })			Err(NoRepoInCwdError { cwd: at })
	}			}
	Err(error) => exit(			Err(error) => exit(
	&initial_current_dir,			&initial_current_dir,
	&ui,			&ui,
	OnUnsupported::from_config(&non_repo_config),			OnUnsupported::from_config(&ui, &non_repo_config),
	Err(error.into()),			Err(error.into()),
	),			),
	};			};

	let config = if let Ok(repo) = &repo_result {			let config = if let Ok(repo) = &repo_result {
	repo.config()			repo.config()
	} else {			} else {
	&non_repo_config			&non_repo_config
	};			};
				let on_unsupported = OnUnsupported::from_config(&ui, config);

	let result = main_with_result(			let result = main_with_result(
	&process_start_time,			&process_start_time,
	&ui,			&ui,
	repo_result.as_ref(),			repo_result.as_ref(),
	config,			config,
	);			);
	exit(			exit(&initial_current_dir, &ui, on_unsupported, result)
	&initial_current_dir,
	&ui,
	OnUnsupported::from_config(config),
	result,
	)
	}			}

	fn exit_code(result: &Result<(), CommandError>) -> i32 {			fn exit_code(result: &Result<(), CommandError>) -> i32 {
	match result {			match result {
	Ok(()) => exitcode::OK,			Ok(()) => exitcode::OK,
	Err(CommandError::Abort { .. }) => exitcode::ABORT,			Err(CommandError::Abort { .. }) => exitcode::ABORT,
	Err(CommandError::Unsuccessful) => exitcode::UNSUCCESSFUL,			Err(CommandError::Unsuccessful) => exitcode::UNSUCCESSFUL,

	b"tried to fall back to a '{}' sub-process but got error {}\n",			b"tried to fall back to a '{}' sub-process but got error {}\n",
	executable, format_bytes::Utf8(error)			executable, format_bytes::Utf8(error)
	));			));
	on_unsupported = OnUnsupported::Abort			on_unsupported = OnUnsupported::Abort
	}			}
	}			}
	}			}
	}			}
				exit_no_fallback(ui, on_unsupported, result)
				}

				fn exit_no_fallback(
				ui: &Ui,
				on_unsupported: OnUnsupported,
				result: Result<(), CommandError>,
				) -> ! {
	match &result {			match &result {
	Ok(_) => {}			Ok(_) => {}
	Err(CommandError::Unsuccessful) => {}			Err(CommandError::Unsuccessful) => {}
	Err(CommandError::Abort { message }) => {			Err(CommandError::Abort { message }) => {
	if !message.is_empty() {			if !message.is_empty() {
	// Ignore errors when writing to stderr, we’re already exiting			// Ignore errors when writing to stderr, we’re already exiting
	// with failure code so there’s not much more we can do.			// with failure code so there’s not much more we can do.
	let _ = ui.write_stderr(&format_bytes!(b"{}\n", message));			let _ = ui.write_stderr(&format_bytes!(b"{}\n", message));
	/// Silently exit with code 252.			/// Silently exit with code 252.
	AbortSilent,			AbortSilent,
	/// Try running a Python implementation			/// Try running a Python implementation
	Fallback { executable: Vec<u8> },			Fallback { executable: Vec<u8> },
	}			}

	impl OnUnsupported {			impl OnUnsupported {
	const DEFAULT: Self = OnUnsupported::Abort;			const DEFAULT: Self = OnUnsupported::Abort;
	const DEFAULT_FALLBACK_EXECUTABLE: &'static [u8] = b"hg";

	fn from_config(config: &Config) -> Self {			fn from_config(ui: &Ui, config: &Config) -> Self {
	match config			match config
	.get(b"rhg", b"on-unsupported")			.get(b"rhg", b"on-unsupported")
	.map(\|value\| value.to_ascii_lowercase())			.map(\|value\| value.to_ascii_lowercase())
	.as_deref()			.as_deref()
	{			{
	Some(b"abort") => OnUnsupported::Abort,			Some(b"abort") => OnUnsupported::Abort,
	Some(b"abort-silent") => OnUnsupported::AbortSilent,			Some(b"abort-silent") => OnUnsupported::AbortSilent,
	Some(b"fallback") => OnUnsupported::Fallback {			Some(b"fallback") => OnUnsupported::Fallback {
	executable: config			executable: config
	.get(b"rhg", b"fallback-executable")			.get(b"rhg", b"fallback-executable")
	.unwrap_or(Self::DEFAULT_FALLBACK_EXECUTABLE)			.unwrap_or_else(\|\| {
				exit_no_fallback(
				ui,
				Self::Abort,
				Err(CommandError::abort(
				"abort: 'rhg.on-unsupported=fallback' without \
				'rhg.fallback-executable' set."
				marmouteUnsubmitted Not Done That message in a bit long, what about: abort: 'rhg.on-unsupported=fallback' without 'rhg.fallback-executable' set. to get < 80 char ? marmoute: That message in a bit long, what about: ``` abort: 'rhg.on-unsupported=fallback' without 'rhg.
				)),
				)
				})
	.to_owned(),			.to_owned(),
	},			},
	None => Self::DEFAULT,			None => Self::DEFAULT,
	Some(_) => {			Some(_) => {
	// TODO: warn about unknown config value			// TODO: warn about unknown config value
	Self::DEFAULT			Self::DEFAULT
	}			}
	}			}

tests/test-rhg.t


	Fallback to Python			Fallback to Python
	$ $NO_FALLBACK rhg cat original			$ $NO_FALLBACK rhg cat original
	unsupported feature: `rhg cat` without `--rev` / `-r`			unsupported feature: `rhg cat` without `--rev` / `-r`
	[252]			[252]
	$ rhg cat original			$ rhg cat original
	original content			original content

				$ FALLBACK_EXE="$RHG_FALLBACK_EXECUTABLE"
				$ unset RHG_FALLBACK_EXECUTABLE
				$ rhg cat original
				abort: 'rhg.on-unsupported=fallback' without 'rhg.fallback-executable' set.
				[255]
				$ RHG_FALLBACK_EXECUTABLE="$FALLBACK_EXE"
				$ export RHG_FALLBACK_EXECUTABLE

	$ rhg cat original --config rhg.fallback-executable=false			$ rhg cat original --config rhg.fallback-executable=false
	[1]			[1]

	$ rhg cat original --config rhg.fallback-executable=hg-non-existent			$ rhg cat original --config rhg.fallback-executable=hg-non-existent
	tried to fall back to a 'hg-non-existent' sub-process but got error $ENOENT$			tried to fall back to a 'hg-non-existent' sub-process but got error $ENOENT$
	unsupported feature: `rhg cat` without `--rev` / `-r`			unsupported feature: `rhg cat` without `--rev` / `-r`
	[252]			[252]