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.

Sign up for GitHub

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

Jump to bottom

plumbing: wire up contexts for Transport.AdvertisedReferences #246

Merged
merged 2 commits into from Mar 26, 2021
Merged

plumbing: wire up contexts for Transport.AdvertisedReferences #246

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
44faab6
plumbing: wire up contexts for Transport.AdvertisedReferences
asuffield Jan 18, 2021
bbd4c4f
add more tests for context wiring
asuffield Mar 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions plumbing/transport/common.go
View file
Open in desktop
Expand Up @@ -58,6 +58,11 @@ type Session interface {
// If the repository does not exist, returns ErrRepositoryNotFound.
// If the repository exists, but is empty, returns ErrEmptyRemoteRepository.
AdvertisedReferences() (*packp.AdvRefs, error)
// AdvertisedReferencesContext retrieves the advertised references for a
// repository.
// If the repository does not exist, returns ErrRepositoryNotFound.
// If the repository exists, but is empty, returns ErrEmptyRemoteRepository.
AdvertisedReferencesContext(context.Context) (*packp.AdvRefs, error)
io.Closer
}

Expand Down
5 changes: 3 additions & 2 deletions plumbing/transport/http/common.go
View file
Open in desktop
Expand Up @@ -3,6 +3,7 @@ package http

import (
"bytes"
"context"
"fmt"
"net"
"net/http"
Expand Down Expand Up @@ -32,7 +33,7 @@ func applyHeadersToRequest(req *http.Request, content *bytes.Buffer, host string

const infoRefsPath = "/info/refs"

func advertisedReferences(s *session, serviceName string) (ref *packp.AdvRefs, err error) {
func advertisedReferences(ctx context.Context, s *session, serviceName string) (ref *packp.AdvRefs, err error) {
url := fmt.Sprintf(
"%s%s?service=%s",
s.endpoint.String(), infoRefsPath, serviceName,
Expand All @@ -45,7 +46,7 @@ func advertisedReferences(s *session, serviceName string) (ref *packp.AdvRefs, e

s.ApplyAuthToRequest(req)
applyHeadersToRequest(req, nil, s.endpoint.Host, serviceName)
res, err := s.client.Do(req)
res, err := s.client.Do(req.WithContext(ctx))
if err != nil {
return nil, err
}
Expand Down
6 changes: 5 additions & 1 deletion plumbing/transport/http/receive_pack.go
View file
Open in desktop
Expand Up @@ -25,7 +25,11 @@ func newReceivePackSession(c *http.Client, ep *transport.Endpoint, auth transpor
}

func (s *rpSession) AdvertisedReferences() (*packp.AdvRefs, error) {
return advertisedReferences(s.session, transport.ReceivePackServiceName)
return advertisedReferences(context.TODO(), s.session, transport.ReceivePackServiceName)
}

func (s *rpSession) AdvertisedReferencesContext(ctx context.Context) (*packp.AdvRefs, error) {
return advertisedReferences(ctx, s.session, transport.ReceivePackServiceName)
}

func (s *rpSession) ReceivePack(ctx context.Context, req *packp.ReferenceUpdateRequest) (
Expand Down
6 changes: 5 additions & 1 deletion plumbing/transport/http/upload_pack.go
View file
Open in desktop
Expand Up @@ -25,7 +25,11 @@ func newUploadPackSession(c *http.Client, ep *transport.Endpoint, auth transport
}

func (s *upSession) AdvertisedReferences() (*packp.AdvRefs, error) {
return advertisedReferences(s.session, transport.UploadPackServiceName)
return advertisedReferences(context.TODO(), s.session, transport.UploadPackServiceName)
}

func (s *upSession) AdvertisedReferencesContext(ctx context.Context) (*packp.AdvRefs, error) {
return advertisedReferences(ctx, s.session, transport.UploadPackServiceName)
}

func (s *upSession) UploadPack(
Expand Down
31 changes: 31 additions & 0 deletions plumbing/transport/http/upload_pack_test.go
View file
Open in desktop
@@ -1,8 +1,10 @@
package http

import (
"context"
"fmt"
"io/ioutil"
"net/url"
"os"
"path/filepath"

Expand Down Expand Up @@ -103,3 +105,32 @@ func (s *UploadPackSuite) TestAdvertisedReferencesRedirectSchema(c *C) {
url := session.(*upSession).endpoint.String()
c.Assert(url, Equals, "https://github.com/git-fixtures/basic")
}

func (s *UploadPackSuite) TestAdvertisedReferencesContext(c *C) {
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
endpoint, _ := transport.NewEndpoint("http://github.com/git-fixtures/basic")

session, err := s.Client.NewUploadPackSession(endpoint, s.EmptyAuth)
c.Assert(err, IsNil)

info, err := session.AdvertisedReferencesContext(ctx)
c.Assert(err, IsNil)
c.Assert(info, NotNil)

url := session.(*upSession).endpoint.String()
c.Assert(url, Equals, "https://github.com/git-fixtures/basic")
}

func (s *UploadPackSuite) TestAdvertisedReferencesContextCanceled(c *C) {
ctx, cancel := context.WithCancel(context.Background())
cancel()
endpoint, _ := transport.NewEndpoint("http://github.com/git-fixtures/basic")

session, err := s.Client.NewUploadPackSession(endpoint, s.EmptyAuth)
c.Assert(err, IsNil)

info, err := session.AdvertisedReferencesContext(ctx)
c.Assert(err, DeepEquals, &url.Error{Op: "Get", URL: "http://github.com/git-fixtures/basic/info/refs?service=git-upload-pack", Err: context.Canceled})
c.Assert(info, IsNil)
}
10 changes: 7 additions & 3 deletions plumbing/transport/internal/common/common.go
View file
Open in desktop
Expand Up @@ -162,14 +162,18 @@ func (c *client) listenFirstError(r io.Reader) chan string {
return errLine
}

// AdvertisedReferences retrieves the advertised references from the server.
func (s *session) AdvertisedReferences() (*packp.AdvRefs, error) {
return s.AdvertisedReferencesContext(context.TODO())
}

// AdvertisedReferences retrieves the advertised references from the server.
func (s *session) AdvertisedReferencesContext(ctx context.Context) (*packp.AdvRefs, error) {
if s.advRefs != nil {
return s.advRefs, nil
}

ar := packp.NewAdvRefs()
if err := ar.Decode(s.Stdout); err != nil {
if err := ar.Decode(s.StdoutContext(ctx)); err != nil {
if err := s.handleAdvRefDecodeError(err); err != nil {
return nil, err
}
Expand Down Expand Up @@ -237,7 +241,7 @@ func (s *session) UploadPack(ctx context.Context, req *packp.UploadPackRequest)
return nil, err
}

if _, err := s.AdvertisedReferences(); err != nil {
if _, err := s.AdvertisedReferencesContext(ctx); err != nil {
return nil, err
}

Expand Down
8 changes: 8 additions & 0 deletions plumbing/transport/server/server.go
View file
Open in desktop
Expand Up @@ -108,6 +108,10 @@ type upSession struct {
}

func (s *upSession) AdvertisedReferences() (*packp.AdvRefs, error) {
return s.AdvertisedReferencesContext(context.TODO())
}

func (s *upSession) AdvertisedReferencesContext(ctx context.Context) (*packp.AdvRefs, error) {
ar := packp.NewAdvRefs()

if err := s.setSupportedCapabilities(ar.Capabilities); err != nil {
Expand Down Expand Up @@ -204,6 +208,10 @@ type rpSession struct {
}

func (s *rpSession) AdvertisedReferences() (*packp.AdvRefs, error) {
return s.AdvertisedReferencesContext(context.TODO())
}

func (s *rpSession) AdvertisedReferencesContext(ctx context.Context) (*packp.AdvRefs, error) {
ar := packp.NewAdvRefs()

if err := s.setSupportedCapabilities(ar.Capabilities); err != nil {
Expand Down
6 changes: 3 additions & 3 deletions remote.go
View file
Open in desktop
Expand Up @@ -109,7 +109,7 @@ func (r *Remote) PushContext(ctx context.Context, o *PushOptions) (err error) {

defer ioutil.CheckClose(s, &err)

ar, err := s.AdvertisedReferences()
ar, err := s.AdvertisedReferencesContext(ctx)
if err != nil {
return err
}
Expand Down Expand Up @@ -316,7 +316,7 @@ func (r *Remote) fetch(ctx context.Context, o *FetchOptions) (sto storer.Referen

defer ioutil.CheckClose(s, &err)

ar, err := s.AdvertisedReferences()
ar, err := s.AdvertisedReferencesContext(ctx)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -1034,7 +1034,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())
Copy link
Contributor

@xiujuan95 xiujuan95 Mar 11, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@xiujuan95 xiujuan95 Mar 12, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, thanks!

if err != nil {
return nil, err
}
Expand Down
49 changes: 47 additions & 2 deletions remote_test.go
View file
Open in desktop
Expand Up @@ -154,6 +154,22 @@ func (s *RemoteSuite) TestFetchContext(c *C) {
URLs: []string{s.GetLocalRepositoryURL(fixtures.ByTag("tags").One())},
})

ctx, cancel := context.WithCancel(context.Background())
defer cancel()

err := r.FetchContext(ctx, &FetchOptions{
RefSpecs: []config.RefSpec{
config.RefSpec("+refs/heads/master:refs/remotes/origin/master"),
},
})
c.Assert(err, IsNil)
}

func (s *RemoteSuite) TestFetchContextCanceled(c *C) {
r := NewRemote(memory.NewStorage(), &config.RemoteConfig{
URLs: []string{s.GetLocalRepositoryURL(fixtures.ByTag("tags").One())},
})

ctx, cancel := context.WithCancel(context.Background())
cancel()

Expand All @@ -162,7 +178,7 @@ func (s *RemoteSuite) TestFetchContext(c *C) {
config.RefSpec("+refs/heads/master:refs/remotes/origin/master"),
},
})
c.Assert(err, NotNil)
c.Assert(err, Equals, context.Canceled)
}

func (s *RemoteSuite) TestFetchWithAllTags(c *C) {
Expand Down Expand Up @@ -478,6 +494,35 @@ func (s *RemoteSuite) TestPushContext(c *C) {
URLs: []string{url},
})

ctx, cancel := context.WithCancel(context.Background())
defer cancel()

numGoroutines := runtime.NumGoroutine()

err = r.PushContext(ctx, &PushOptions{
RefSpecs: []config.RefSpec{"refs/tags/*:refs/tags/*"},
})
c.Assert(err, IsNil)

// let the goroutine from pushHashes finish and check that the number of
// goroutines is the same as before
time.Sleep(100 * time.Millisecond)
c.Assert(runtime.NumGoroutine(), Equals, numGoroutines)
}

func (s *RemoteSuite) TestPushContextCanceled(c *C) {
url := c.MkDir()
_, err := PlainInit(url, true)
c.Assert(err, IsNil)

fs := fixtures.ByURL("https://github.com/git-fixtures/tags.git").One().DotGit()
sto := filesystem.NewStorage(fs, cache.NewObjectLRUDefault())

r := NewRemote(sto, &config.RemoteConfig{
Name: DefaultRemoteName,
URLs: []string{url},
})

ctx, cancel := context.WithCancel(context.Background())
cancel()

Expand All @@ -486,7 +531,7 @@ func (s *RemoteSuite) TestPushContext(c *C) {
err = r.PushContext(ctx, &PushOptions{
RefSpecs: []config.RefSpec{"refs/tags/*:refs/tags/*"},
})
c.Assert(err, NotNil)
c.Assert(err, Equals, context.Canceled)

// let the goroutine from pushHashes finish and check that the number of
// goroutines is the same as before
Expand Down
12 changes: 6 additions & 6 deletions repository_test.go
View file
Open in desktop
Expand Up @@ -187,7 +187,7 @@ func (s *RepositorySuite) TestCloneContext(c *C) {
})

c.Assert(r, NotNil)
c.Assert(err, ErrorMatches, ".* context canceled")
c.Assert(err, Equals, context.Canceled)
}

func (s *RepositorySuite) TestCloneWithTags(c *C) {
Expand Down Expand Up @@ -655,12 +655,12 @@ func (s *RepositorySuite) TestPlainCloneContextCancel(c *C) {
})

c.Assert(r, NotNil)
c.Assert(err, ErrorMatches, ".* context canceled")
c.Assert(err, Equals, context.Canceled)
}

func (s *RepositorySuite) TestPlainCloneContextNonExistentWithExistentDir(c *C) {
ctx, cancel := context.WithCancel(context.Background())
cancel()
defer cancel()

tmpDir := c.MkDir()
repoDir := tmpDir
Expand All @@ -681,7 +681,7 @@ func (s *RepositorySuite) TestPlainCloneContextNonExistentWithExistentDir(c *C)

func (s *RepositorySuite) TestPlainCloneContextNonExistentWithNonExistentDir(c *C) {
ctx, cancel := context.WithCancel(context.Background())
cancel()
defer cancel()

tmpDir := c.MkDir()
repoDir := filepath.Join(tmpDir, "repoDir")
Expand Down Expand Up @@ -719,7 +719,7 @@ func (s *RepositorySuite) TestPlainCloneContextNonExistentWithNotDir(c *C) {

func (s *RepositorySuite) TestPlainCloneContextNonExistentWithNotEmptyDir(c *C) {
ctx, cancel := context.WithCancel(context.Background())
cancel()
defer cancel()

tmpDir := c.MkDir()
repoDirPath := filepath.Join(tmpDir, "repoDir")
Expand All @@ -743,7 +743,7 @@ func (s *RepositorySuite) TestPlainCloneContextNonExistentWithNotEmptyDir(c *C)

func (s *RepositorySuite) TestPlainCloneContextNonExistingOverExistingGitDirectory(c *C) {
ctx, cancel := context.WithCancel(context.Background())
cancel()
defer cancel()

tmpDir := c.MkDir()
r, err := PlainInit(tmpDir, false)
Expand Down