Could configure_command just be a builder function?

[EliahKagan]

The private configure_command function in gix-testtools operates on an already-existing instance of std::process::Command:

gitoxide/tests/tools/src/lib.rs

Lines 591 to 595 in d69c617

	fn configure_command<'a>(
	cmd: &'a mut std::process::Command,
	args: &[String],
	script_result_directory: &Path,
	) -> &'a mut std::process::Command {

But other than in one of its own tests, it is always called immediately after a call to Command::new, on the instance that was returned by that call.

The non-test calls appear in scripted_fixture_read_only_with_args_inner, where the variable is reused across the two calls, but its value is not:

gitoxide/tests/tools/src/lib.rs

Lines 552 to 562 in d69c617

	let mut cmd = std::process::Command::new(&script_absolute_path);
	let output = match configure_command(&mut cmd, &args, &script_result_directory).output() {
	Ok(out) => out,
	Err(err)
	if err.kind() == std::io::ErrorKind::PermissionDenied || err.raw_os_error() == Some(193) /* windows */ =>
	{
	cmd = std::process::Command::new("bash");
	configure_command(cmd.arg(script_absolute_path), &args, &script_result_directory).output()?
	}
	Err(err) => return Err(err.into()),
	};

There, the calls and their immediately preceding Command::new calls are:

gitoxide/tests/tools/src/lib.rs

Lines 552 to 553 in d69c617

	let mut cmd = std::process::Command::new(&script_absolute_path);
	let output = match configure_command(&mut cmd, &args, &script_result_directory).output() {

gitoxide/tests/tools/src/lib.rs

Lines 558 to 559 in d69c617

	cmd = std::process::Command::new("bash");
	configure_command(cmd.arg(script_absolute_path), &args, &script_result_directory).output()?

This was the case prior to the introduction of the configure_command_clears_external_config test added in 85c5e2f (#1570), where I do perform operations on the cmd object first:

gitoxide/tests/tools/src/lib.rs

Lines 917 to 921 in d69c617

	let mut cmd = std::process::Command::new("git");
	let args = ["config", "-l", "--show-origin"].map(String::from);
	cmd.env("GIT_CONFIG_SYSTEM", SCOPE_ENV_VALUE);
	cmd.env("GIT_CONFIG_GLOBAL", SCOPE_ENV_VALUE);
	configure_command(&mut cmd, &args, temp.path());

But I don't think this test justifies keeping the current design of configure_command, because:

• If we can use #[serial] in tests of gix-testtools, even though the consequences of it being underprotective may be harder to reason about if fixture scripts are running at the same time, then the tests can be simple and robust both without the above technique and without dependency injection to allow std::env facilities to be mocked.

  (Thus, I have opened #1577 to inquire about how freely #[serial] may be used. I think I can use dependency injection here to mock most relevant behavior of the inherited environment, but unlike 15235bf (#1456) I don't think this is a case where the abstraction is necessarily justified.)

• The configure_command_clears_external_config test, while I think it is correct, is slightly misleading, and the technique it uses cannot be readily extended. This is because it only covers the case where calling env on a Command instance produces the same effect, as observed in the test, of the value actually being set in the environment. But this does not hold in general.

  Furthermore, there is already an environment variable for which it does not hold in configure_command: setting MSYS by calling env on a Command object has no effect on the state after a call to configure_command, which always replaces it with a value based on what is actually in the environment.
  

  gitoxide/tests/tools/src/lib.rs

  Lines 596 to 597 in d69c617

  	let mut msys_for_git_bash_on_windows = std::env::var("MSYS").unwrap_or_default();
  	msys_for_git_bash_on_windows.push_str(" winsymlinks:nativestrict");

  gitoxide/tests/tools/src/lib.rs

  Line 605 in d69c617

  .env("MSYS", msys_for_git_bash_on_windows)

  I encountered this when working on #1580, which was originally going to add more tests, which I ultimately did not include because they would not really work.

That's what motivated me to look into this: configure_command could be extended to also check if MSYS has been overridden for the Command instance it operates on, but it seems unlikely that this would be worthwhile.

Part of why it is probably not worthwhile is that it may require implementing Windows-compatible case-folding to be sure that it is always correct, which if done in a way whose soundness and correctness could both easily be verified would involve adding the windows crate as a dependency of gix-testttools.

But part of the reason is that configure_command is only ever being used where a builder function—by which I mean a function that takes no &mut Command argument and returns a Command—would suffice.

Is there any reason to refrain from changing it to that design? Are uses that benefit from having an existing Command instance first anticipated?

[Byron]

I think if in doubt, a new function could be introduced that does not take &mut Command as first argument, but the item to execute, and then calls configure_command() internally, keeping it available for those few cases that currently (actually) need it.

Otherwise, I also see no problem in adjusting configure_command() to create the Command instance itself, and adjust the few cases that used it with &mut Command to not do that anymore.

Option 1 seems like it's faster to implement.

Also, for future reference, please do feel free to freely refactor such things, or make any change you see a need for, there isn't any need to ask. In PRs that make such alteration, I would be finding solutions together with you if there is anything speaking against them.

[EliahKagan]

Also, for future reference, please do feel free to freely refactor such things, or make any change you see a need for, there isn't any need to ask. In PRs that make such alteration, I would be finding solutions together with you if there is anything speaking against them.

Thanks--I will keep this in mind, and also I was probably already willing to do this. Part of why I asked first in this case was that I thought a change like this would create a conflict with the other PRs I was opening around then.

[EliahKagan]

I figured out what is going on here and why configure_command is designed the way it is. Contrary to what I had claimed above, the configure_command here does not immediately follow the Command::new call, because the Command::arg call, i.e. cmd.arg(script_absolute_path), comes in between:

gitoxide/tests/tools/src/lib.rs

Lines 558 to 559 in d69c617

	cmd = std::process::Command::new("bash");
	configure_command(cmd.arg(script_absolute_path), &args, &script_result_directory).output()?

I'm not sure how I missed that earlier! Anyway, I think it means that we either need to be able to chain the arguments together without lots of distracting syntax, or configure_command can itself take on the responsibility for running the command with a shell if requested.

configure_command can be generic in the same way Command::args itself is, and that actually makes testing more convenient. But I don't know of an elegant way to chain a PathBuf with a Strings. What they have in common is that they satisfy AsRef<OsStr>. It may be that I just don't know how to do it nicely, but it seems like the code in the caller will become more complicated if this is done. I suspect also that this may be the reason Command itself has both arg and args methods and allows calls to them to be arbitrarily interleaved: chaining those calls is easer to read (and, less importantly, to write) than chaining iterators and making one call.

A third option could be to make configure_command a method on Command itself, by introducing a trait. If giving configure_command a shell parameter turns out not to look like a good solution then I may explore that.

[Byron]

Please feel free to explore options, as in the worst case, you will probably gain better intuition about Rust generics, or questions related to APIs in Rust and some tradeoffs.

And just to share it: I (personally) would leave it alone merely because it seems like a minute detail, with bigger fish to fry. But, it's also a learning opportunity

[EliahKagan]

Please feel free to explore options, as in the worst case, you will probably gain better intuition about Rust generics, or questions related to APIs in Rust and some tradeoffs.

Yes, I wanted to post to correct the inaccurate claim I had made earlier, which is why I first expressed these ideas in a comment here rather than a PR later. 😄

And just to share it: I (personally) would leave it alone merely because it seems like a minute detail, with bigger fish to fry.

If there is a clear improvement to be made to configure_command, then even if the stakes are low, I may want to make it before proceeding with changes to fix #1359, which is part of my thinking. Probably they can be separate, though, since that is mostly about figuring out what to run instead of bash on some (most) Windows systems.

But, it's also a learning opportunity

That, too.

[EliahKagan]

I experimented with a few things and all the major changes seemed like they weren't clearly better than the current design. I made some small changes, but none that affect how the main caller uses it, in #1592.

It may be that the value of further changes will become apparent during or after future work on #1359 .