plumbing: wire up contexts for Transport.AdvertisedReferences

[asuffield]

This is primarily for the sake of the http transport, which otherwise makes its first call without a context, causing context timeouts to not work if the remote is unresponsive.

Fixed up several tests which were getting this wrong. I am particularly fond of the ones which canceled the context and then proceeded to test that the function worked after being canceled.

The AdvertisedReferences function can be removed after this change, but this is technically breaking the API, since library users could call it, so it should really be deferred to v6.

[asuffield]

Failed checks are a flaky test (network access from the test to the internet) and fractional coverage decrease from the shifting error paths, which I'm inclined to ignore.

[sevki]

Perhaps someone should disable them all together

#51

[xiujuan95]

Hi, I was using go-git package to do git ls-remote check. But I found this command doesn't have a timeout configuration. I saw you were doing this action. That would be nice!

But why do you pass context.TODO() to it? As far as I know, context.TODO() ia an empty context. This means, git ls-remote also doesn't have timeout. Could you explain it for me pls? Thanks in advance!

For me, I expect list func also can be configured ctx parameter, like this:

func (r *Remote) list(ctx context.Context, o *ListOptions) (rfs []*plumbing.Reference, err error) {
    ... ...
    ... ...
    ar, err := s.AdvertisedReferencesContext(ctx)
    if err != nil {
          return nil, err
    }
   ... ....
   ... ....
}

So that we can control git ls-remote command timeout within several seconds. How do you think?

[asuffield]

FetchContext and PushContext exist, but there's no ListContext method. That's a reasonable thing to add, it's just not part of this PR, which is doing the plumbing.

[xiujuan95]

Thanks for your reply! Could you please also help add ListContext func? Or may I submit a new PR based on your changes? But this requires your PR is merged, not sure how long it will take?

[asuffield]

I'd put the API change in a separate MR. No idea how long it will take somebody to get around to merging this one.

[xiujuan95]

Hope this PR can be merged soon!!!! By the way, about API change PR, do you want to own it? If not, I can do it.

[xiujuan95]

Hi, could you please add a test as @mcuadros said so that this PR can be merged? Because in our environment, this feature is needed and lack of it iss causing some problems for us. Thanks in advance!

[asuffield]

You can do the API change, I don't need it myself.

[xiujuan95]

ok, thanks!

[mcuadros]

Sorry for the late review, I will happy to merge this if you add a test to ensure the context propagation.

[asuffield]

Sorry for the late review, I will happy to merge this if you add a test to ensure the context propagation.

The existing tests already covered this reasonably well - I had just fixed them to ensure that context propagates, rather than ensuring that it does not. I made a second pass and added a couple more cases, did you have any more specific in mind?

[xiujuan95]

Any updates about this PR?

[mcuadros]

The existing tests already covered this reasonably well - I had just fixed them to ensure that context propagates, rather than ensuring that it does not. I made a second pass and added a couple more cases, did you have any more specific in mind?

We need a test that fails with the old behavior, to validate that we pass properly the context.

[asuffield]

We need a test that fails with the old behavior, to validate that we pass properly the context.

The existing tests do this. For example, TestPlainCloneContextNonExistingOverExistingGitDirectory. Fixing those tests to defer cancel() instead of calling it immediately is functionally inverting the test so that it verifies the context is passed, instead of verifying that the context is not passed.