apply insteadof on Remote.{fetch,pull,list} functions -- breaking change(?)

[BryanStenson-okta]

Fixes #844

This applies insteadOf rules to Remote.{fetch,pull,list} functions. This may be seen as a breaking change, as the current behavior does not honor insteadOf values. I believe not applying insteadOf 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 to false (to preserve current behavior).

Thoughts?

[boggyhole]

I am not super familiar with the codebase, yet. This PR solves the clone() issue where url.insteadOf rules weren't applied. Are there other use-cases where it would make sense to get them applied (thinking of submodules for example: func (s *Submodule) Repository()? So my actual question is: would it make sense to move r.ApplyURLRules() to a deeper level? I.e. func (r *Repository) CreateRemote(c *config.RemoteConfig) (*Remote, error)?

[BryanStenson-okta]

... would it make sense to move r.ApplyURLRules() to a deeper level? I.e. func (r *Repository) CreateRemote(c *config.RemoteConfig) (*Remote, error)?

yup, good point! i'll update that shortly.

[BryanStenson-okta]

thinking about this more, i think this applying the insteadof substitution logic should happen on the Remote -- so that all fetch, push, etc operations are using substituted urls.

[BryanStenson-okta]

I've updated this PR to call ensureURLRulesApplied() on each Remote.{fetch,pull,list} function, to ensure the URLs will always be rewritten, if necessary.

[pjbgf]

thinking about this more, i think this applying the insteadof substitution logic should happen on the Remote -- so that all fetch, push, etc operations are using substituted urls.

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:

go-git/options.go

Line 178 in 6252084

RemoteURL string

go-git/options.go

Line 40 in 6252084

URL string

We would need tests to that effect.

[pjbgf]

This may be seen as a breaking change, as the current behavior does not honor insteadOf values.

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:

adding a CloneOptions.ApplyInsteadOf bool -- defaulting to false (to preserve current behavior).

[boggyhole]

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:

@pjbgf So would you say that's desired behavior? For my understanding it wouldn't (as I would expect insteadOf rules to be applied to every operation)

The main challenge here is that as a library, not always you would want to be influenced by system wide configuration.

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 Storer? Or is there automatic .gitconfig loading for some scenarios?

gitconfig, err := config.LoadConfig(config.GlobalScope)
if err != nil {
	return nil, err
}

if err := storer.SetConfig(gitconfig); err != nil {
	return nil, err
}