Page MenuHomePhabricator

rhg: only complain about poorly configured fallback when falling back
ClosedPublic

Authored by aalekseyev on Nov 11 2021, 9:27 AM.

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

aalekseyev created this revision.Nov 11 2021, 9:27 AM
aalekseyev added a comment.EditedNov 11 2021, 9:39 AM

The motivation here is that I kept running into this error when testing things with rhg, and I thought it didn't need to be an error.
I think there is no (significant) downside, since checking that fallback-executable is set by no means guarantees that the fallback will work, so not much is lost by delaying that check.

I admit that the reason we end up with this is that our set up is a bit weird (on-unsupported=fallback is set by one component while fallback-executable is set by another), so I won't defend this change if there's any push back.

I admit that the reason we end up with this is that our set up is a bit weird (on-unsupported=fallback is set by one component while fallback-executable is set by another), so I won't defend this change if there's any push back.

I'm not sure I understand what your use case is. The config is already fully read by the point we read it, so how does that help you?

I'm not sure I understand what your use case is. The config is already fully read by the point we read it, so how does that help you?

The static config we have only has on-unsupported=fallback, while the fallback executable is set by a command-line invocation, because that's where it's decided what the fallback binary should be.
This makes it so that when I'm making my own command line rhg invocation for debugging I have to either pass on-unsupported=abort, or an actual fallback executable, which is a bit of a chore.

Anyway, the point here is not to support this exact workflow, but simply to be a bit more forgiving to the format (so that omitting the executable is equivalent to specifying an invalid executable).

Sure, seems fine!

However the test works without this change, so I'm not sure what it's supposed to test.

aalekseyev updated this revision to Diff 31167.Nov 26 2021, 1:53 PM

Sorry, that test was weird indeed. I pushed a better one.

Alphare accepted this revision.Nov 29 2021, 6:06 AM
This revision is now accepted and ready to land.Nov 29 2021, 6:06 AM