New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
apply insteadof on Remote.{fetch,pull,list} functions -- breaking change(?) #882
base: master
Are you sure you want to change the base?
apply insteadof on Remote.{fetch,pull,list} functions -- breaking change(?) #882
Conversation
I am not super familiar with the codebase, yet. This PR solves the |
yup, good point! i'll update that shortly. |
thinking about this more, i think this applying the |
1451ee4
to
542c21c
Compare
call ensureURLRulesApplied() from all Remote.{fetch,push,list}() functions supporting tests
542c21c
to
9ca133d
Compare
I've updated this PR to call |
Good shout! One interesting workflow is when URLs are being set directly at the operation options, which I believe would always overwrite the ones from the Remote. I don't think we need to change the code for this, but I would expand on their comments to highlight that InsteadOf rules does not apply to such fields: Line 178 in 6252084
Line 40 in 6252084
We would need tests to that effect. |
The main challenge here is that as a library, not always you would want to be influenced by system wide configuration. However, I agree with your suggested approach below which would support the new behaviour while keeping backwards compatibility:
|
@pjbgf So would you say that's desired behavior? For my understanding it wouldn't (as I would expect
To my understanding it's not the case? From what I've seen one always needs to manually load system wide configuration and apply it to the
|
Fixes #844
This applies
insteadOf
rules toRemote.{fetch,pull,list}
functions. This may be seen as a breaking change, as the current behavior does not honorinsteadOf
values. I believe not applyinginsteadOf
values is actually a bug (#844)...so I'm not sure if this is a "breaking change", or a "bugfix".I'm open to a discussion on whether this should be gated via feature flag. For example, adding a
CloneOptions.ApplyInsteadOf bool
-- defaulting tofalse
(to preserve current behavior).Thoughts?