Skip to content
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

plumbing: wire up contexts for Transport.AdvertisedReferences #246

Merged
merged 2 commits into from Mar 26, 2021

Conversation

@asuffield
Copy link
Contributor

@asuffield asuffield commented Jan 18, 2021

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
Copy link
Contributor Author

@asuffield asuffield commented Jan 19, 2021

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
Copy link

@sevki sevki commented Jan 19, 2021

Perhaps someone should disable them all together

#51

@@ -1032,7 +1032,7 @@ func (r *Remote) List(o *ListOptions) (rfs []*plumbing.Reference, err error) {

defer ioutil.CheckClose(s, &err)

ar, err := s.AdvertisedReferences()
ar, err := s.AdvertisedReferencesContext(context.TODO())

This comment has been minimized.

@xiujuan95

xiujuan95 Mar 11, 2021
Contributor

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?

This comment has been minimized.

@asuffield

asuffield Mar 11, 2021
Author Contributor

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.

This comment has been minimized.

@xiujuan95

xiujuan95 Mar 11, 2021
Contributor

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?

This comment has been minimized.

@asuffield

asuffield Mar 11, 2021
Author Contributor

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.

This comment has been minimized.

@xiujuan95

xiujuan95 Mar 12, 2021
Contributor

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.

This comment has been minimized.

@xiujuan95

xiujuan95 Mar 15, 2021
Contributor

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!

This comment has been minimized.

@asuffield

asuffield Mar 15, 2021
Author Contributor

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

This comment has been minimized.

@xiujuan95

xiujuan95 Mar 16, 2021
Contributor

ok, thanks!

@mcuadros
Copy link
Member

@mcuadros mcuadros commented Mar 11, 2021

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

@asuffield asuffield force-pushed the asuffield:http-context branch from 4c1cb24 to bbd4c4f Mar 15, 2021
@asuffield
Copy link
Contributor Author

@asuffield asuffield commented Mar 15, 2021

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
Copy link
Contributor

@xiujuan95 xiujuan95 commented Mar 22, 2021

Any updates about this PR?

@mcuadros
Copy link
Member

@mcuadros mcuadros commented Mar 25, 2021

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
Copy link
Contributor Author

@asuffield asuffield commented Mar 25, 2021

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.

@mcuadros mcuadros merged commit e5bbc4d into go-git:master Mar 26, 2021
8 of 9 checks passed
8 of 9 checks passed
@github-actions
test (master, ubuntu-latest)
Details
@github-actions
version-matrix (1.13.x, ubuntu-latest)
Details
@github-actions
test (v2.11.0, ubuntu-latest) test (v2.11.0, ubuntu-latest)
Details
@github-actions
version-matrix (1.13.x, macos-latest)
Details
@github-actions
version-matrix (1.13.x, windows-latest)
Details
@github-actions
version-matrix (1.14.x, ubuntu-latest)
Details
@github-actions
version-matrix (1.14.x, macos-latest)
Details
@github-actions
version-matrix (1.14.x, windows-latest)
Details
coverage/coveralls Coverage decreased (-0.06%) to 83.713%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants