impl Clone for Command

[japaric]

dejavu

Both (unix/windows) underlying Command implementations are already Clone.

r? @alexcrichton

[alexcrichton]

I actually explicitly requested that we not implement Clone for Command this time around:

We may actually want to consider removing this Clone implementation. I've thought in the past that if stdin, stdout, and stderr happen to take some form of I/O object it will likely not implement Clone.

Although we could also just say that the Clone implementation clears the input/output handles.

That being said, I'd be willing to revisit it.

[japaric]

I actually explicitly requested that we not implement Clone for Command this time around:

That'd be bad for me, because I rely on clonning Command in my benchmark library to bench an external program under different inputs: e.g.

let inputs = 0..10;
criterion.bench_prog_with_inputs("py",Command::new("python").arg("under_test.py"), inputs);

Without it, doing the same operation would be unergonomic as you'll have to repeat the Command chain several times:

// one call for each value of `$N`
criterion.bench_prog("py/$N",Command::new("python").arg("under_test.py").arg("$N"));
...// and the user must not forget to summarize the results at the endcriterion.summarize("py");

Although we could also just say that the Clone implementation clears the input/output handles.

That sounds reasonable to me.

I'll stick to the old Command for the time being. I guess this PR needs an approved RFC? (If yes, feel free to close)

[alexcrichton]

Without it, doing the same operation would be unergonomic as you'll have to repeat the Command chain several times:

Another option, however, would be taking a FnMut() -> Command instead of a Command itself (e.g. a factory vs explicitly cloneable object). I do agree though that it is less ergonomic.

I guess this PR needs an approved RFC?

That feels a bit heavy-handed for this particular aspect in my opinion. I believe @aturon is on vacation right now but I would be curious to hear his thoughts on this as well.

[bors]

☔ The latest upstream changes (presumably #22882) made this pull request unmergeable. Please resolve the merge conflicts.

[aturon]

@alexcrichton

I believe @aturon is on vacation right now but I would be curious to hear his thoughts on this as well.

So, I'm uncomfortable with a Clone implementation that doesn't actually "fully" clone the object, e.g. by clearing out the handles. We could provide some differently-named method to make more clear what was happening, perhaps.

[japaric]

We could provide some differently-named method to make more clear what was happening, perhaps.

I'd be happy with this alternative.

[aturon]

@japaric Ok, I think we could probably take an #[unstable] API like that; any thoughts on a good name? The ones coming to mind for me are pretty long, e.g. clone_without_handles.

[alexcrichton]

Closing due to inactivity, but feel free to reopen with @aturon's suggestion!