From 1bb3e9af41db2a9df025c3d07d3862370a72d6fa Mon Sep 17 00:00:00 2001 From: Zoupers <1171443643@qq.com> Date: Fri, 31 May 2024 13:40:58 +0000 Subject: [PATCH 1/6] chore: add debug and trace log for lfs handle --- modules/lfs/http_client.go | 2 +- modules/lfs/transferadapter.go | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/modules/lfs/http_client.go b/modules/lfs/http_client.go index e06879baea4fe..98000a5e41903 100644 --- a/modules/lfs/http_client.go +++ b/modules/lfs/http_client.go @@ -251,6 +251,6 @@ func handleErrorResponse(resp *http.Response) error { return err } - log.Trace("ErrorResponse: %v", er) + log.Trace("ErrorResponse(%v): %v", resp.Status, er) return errors.New(er.Message) } diff --git a/modules/lfs/transferadapter.go b/modules/lfs/transferadapter.go index d425b91946d17..fbc3a3ad8c716 100644 --- a/modules/lfs/transferadapter.go +++ b/modules/lfs/transferadapter.go @@ -37,6 +37,7 @@ func (a *BasicTransferAdapter) Download(ctx context.Context, l *Link) (io.ReadCl if err != nil { return nil, err } + log.Debug("Download Request: %+v", req) resp, err := performRequest(ctx, a.client, req) if err != nil { return nil, err From 9f95db6cd6a1abb6fc2b428aa38af1d82ecae502 Mon Sep 17 00:00:00 2001 From: Zoupers <1171443643@qq.com> Date: Fri, 31 May 2024 13:44:45 +0000 Subject: [PATCH 2/6] Fix #31185 try fix lfs download from bitbucket failed --- modules/lfs/transferadapter.go | 1 + modules/lfs/transferadapter_test.go | 9 ++++++++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/modules/lfs/transferadapter.go b/modules/lfs/transferadapter.go index fbc3a3ad8c716..b461ae7ce8b24 100644 --- a/modules/lfs/transferadapter.go +++ b/modules/lfs/transferadapter.go @@ -37,6 +37,7 @@ func (a *BasicTransferAdapter) Download(ctx context.Context, l *Link) (io.ReadCl if err != nil { return nil, err } + req.Header.Del("Accept") log.Debug("Download Request: %+v", req) resp, err := performRequest(ctx, a.client, req) if err != nil { diff --git a/modules/lfs/transferadapter_test.go b/modules/lfs/transferadapter_test.go index 6023cd07d32fd..3168b1a4b8ee9 100644 --- a/modules/lfs/transferadapter_test.go +++ b/modules/lfs/transferadapter_test.go @@ -24,9 +24,12 @@ func TestBasicTransferAdapterName(t *testing.T) { func TestBasicTransferAdapter(t *testing.T) { p := Pointer{Oid: "b5a2c96250612366ea272ffac6d9744aaf4b45aacd96aa7cfcb931ee3b558259", Size: 5} + downloadTestFlag := false roundTripHandler := func(req *http.Request) *http.Response { - assert.Equal(t, MediaType, req.Header.Get("Accept")) + if !downloadTestFlag { + assert.Equal(t, MediaType, req.Header.Get("Accept")) + } assert.Equal(t, "test-value", req.Header.Get("test-header")) url := req.URL.String() @@ -71,6 +74,10 @@ func TestBasicTransferAdapter(t *testing.T) { a := &BasicTransferAdapter{hc} t.Run("Download", func(t *testing.T) { + downloadTestFlag = true + t.Cleanup(func() { + downloadTestFlag = false + }) cases := []struct { link *Link expectederror string From 6f2f14c8c8b506d67c9e5b3796db9fb1520f4aa6 Mon Sep 17 00:00:00 2001 From: Zoupers <1171443643@qq.com> Date: Sat, 1 Jun 2024 03:15:43 +0000 Subject: [PATCH 3/6] refactor: seperate accept header from MediaType --- modules/lfs/http_client.go | 2 +- modules/lfs/http_client_test.go | 4 ++-- modules/lfs/shared.go | 3 ++- modules/lfs/transferadapter.go | 1 - modules/lfs/transferadapter_test.go | 2 +- services/lfs/server.go | 2 +- tests/integration/api_repo_lfs_locks_test.go | 10 +++++----- tests/integration/api_repo_lfs_test.go | 4 ++-- 8 files changed, 14 insertions(+), 14 deletions(-) diff --git a/modules/lfs/http_client.go b/modules/lfs/http_client.go index 98000a5e41903..f5ddd38b0911d 100644 --- a/modules/lfs/http_client.go +++ b/modules/lfs/http_client.go @@ -211,7 +211,7 @@ func createRequest(ctx context.Context, method, url string, headers map[string]s for key, value := range headers { req.Header.Set(key, value) } - req.Header.Set("Accept", MediaType) + req.Header.Set("Accept", AcceptHeader) return req, nil } diff --git a/modules/lfs/http_client_test.go b/modules/lfs/http_client_test.go index 7459d9c0c97e8..7431132f760f4 100644 --- a/modules/lfs/http_client_test.go +++ b/modules/lfs/http_client_test.go @@ -155,7 +155,7 @@ func TestHTTPClientDownload(t *testing.T) { hc := &http.Client{Transport: RoundTripFunc(func(req *http.Request) *http.Response { assert.Equal(t, "POST", req.Method) assert.Equal(t, MediaType, req.Header.Get("Content-type")) - assert.Equal(t, MediaType, req.Header.Get("Accept")) + assert.Equal(t, AcceptHeader, req.Header.Get("Accept")) var batchRequest BatchRequest err := json.NewDecoder(req.Body).Decode(&batchRequest) @@ -263,7 +263,7 @@ func TestHTTPClientUpload(t *testing.T) { hc := &http.Client{Transport: RoundTripFunc(func(req *http.Request) *http.Response { assert.Equal(t, "POST", req.Method) assert.Equal(t, MediaType, req.Header.Get("Content-type")) - assert.Equal(t, MediaType, req.Header.Get("Accept")) + assert.Equal(t, AcceptHeader, req.Header.Get("Accept")) var batchRequest BatchRequest err := json.NewDecoder(req.Body).Decode(&batchRequest) diff --git a/modules/lfs/shared.go b/modules/lfs/shared.go index 6b2e55f2fbfd3..ef7d951b50406 100644 --- a/modules/lfs/shared.go +++ b/modules/lfs/shared.go @@ -9,7 +9,8 @@ import ( const ( // MediaType contains the media type for LFS server requests - MediaType = "application/vnd.git-lfs+json" + MediaType = "application/vnd.git-lfs+json" + AcceptHeader = "application/vnd.git-lfs+json;q=0.9, */*;q=0.8" ) // BatchRequest contains multiple requests processed in one batch operation. diff --git a/modules/lfs/transferadapter.go b/modules/lfs/transferadapter.go index b461ae7ce8b24..fbc3a3ad8c716 100644 --- a/modules/lfs/transferadapter.go +++ b/modules/lfs/transferadapter.go @@ -37,7 +37,6 @@ func (a *BasicTransferAdapter) Download(ctx context.Context, l *Link) (io.ReadCl if err != nil { return nil, err } - req.Header.Del("Accept") log.Debug("Download Request: %+v", req) resp, err := performRequest(ctx, a.client, req) if err != nil { diff --git a/modules/lfs/transferadapter_test.go b/modules/lfs/transferadapter_test.go index 3168b1a4b8ee9..f51ec18456f9a 100644 --- a/modules/lfs/transferadapter_test.go +++ b/modules/lfs/transferadapter_test.go @@ -28,7 +28,7 @@ func TestBasicTransferAdapter(t *testing.T) { roundTripHandler := func(req *http.Request) *http.Response { if !downloadTestFlag { - assert.Equal(t, MediaType, req.Header.Get("Accept")) + assert.Equal(t, AcceptHeader, req.Header.Get("Accept")) } assert.Equal(t, "test-value", req.Header.Get("test-header")) diff --git a/services/lfs/server.go b/services/lfs/server.go index 2e330aa1a4865..ae3dffe0c2912 100644 --- a/services/lfs/server.go +++ b/services/lfs/server.go @@ -477,7 +477,7 @@ func buildObjectResponse(rc *requestContext, pointer lfs_module.Pointer, downloa } // This is only needed to workaround https://github.com/git-lfs/git-lfs/issues/3662 - verifyHeader["Accept"] = lfs_module.MediaType + verifyHeader["Accept"] = lfs_module.AcceptHeader rep.Actions["verify"] = &lfs_module.Link{Href: rc.VerifyLink(pointer), Header: verifyHeader} } diff --git a/tests/integration/api_repo_lfs_locks_test.go b/tests/integration/api_repo_lfs_locks_test.go index 5aa1396941ddd..427e0b9fb11db 100644 --- a/tests/integration/api_repo_lfs_locks_test.go +++ b/tests/integration/api_repo_lfs_locks_test.go @@ -105,7 +105,7 @@ func TestAPILFSLocksLogged(t *testing.T) { for _, test := range tests { session := loginUser(t, test.user.Name) req := NewRequestWithJSON(t, "POST", fmt.Sprintf("/%s.git/info/lfs/locks", test.repo.FullName()), map[string]string{"path": test.path}) - req.Header.Set("Accept", lfs.MediaType) + req.Header.Set("Accept", lfs.AcceptHeader) req.Header.Set("Content-Type", lfs.MediaType) resp := session.MakeRequest(t, req, test.httpResult) if len(test.addTime) > 0 { @@ -123,7 +123,7 @@ func TestAPILFSLocksLogged(t *testing.T) { for _, test := range resultsTests { session := loginUser(t, test.user.Name) req := NewRequestf(t, "GET", "/%s.git/info/lfs/locks", test.repo.FullName()) - req.Header.Set("Accept", lfs.MediaType) + req.Header.Set("Accept", lfs.AcceptHeader) resp := session.MakeRequest(t, req, http.StatusOK) var lfsLocks api.LFSLockList DecodeJSON(t, resp, &lfsLocks) @@ -135,7 +135,7 @@ func TestAPILFSLocksLogged(t *testing.T) { } req = NewRequestWithJSON(t, "POST", fmt.Sprintf("/%s.git/info/lfs/locks/verify", test.repo.FullName()), map[string]string{}) - req.Header.Set("Accept", lfs.MediaType) + req.Header.Set("Accept", lfs.AcceptHeader) req.Header.Set("Content-Type", lfs.MediaType) resp = session.MakeRequest(t, req, http.StatusOK) var lfsLocksVerify api.LFSLockListVerify @@ -159,7 +159,7 @@ func TestAPILFSLocksLogged(t *testing.T) { for _, test := range deleteTests { session := loginUser(t, test.user.Name) req := NewRequestWithJSON(t, "POST", fmt.Sprintf("/%s.git/info/lfs/locks/%s/unlock", test.repo.FullName(), test.lockID), map[string]string{}) - req.Header.Set("Accept", lfs.MediaType) + req.Header.Set("Accept", lfs.AcceptHeader) req.Header.Set("Content-Type", lfs.MediaType) resp := session.MakeRequest(t, req, http.StatusOK) var lfsLockRep api.LFSLockResponse @@ -172,7 +172,7 @@ func TestAPILFSLocksLogged(t *testing.T) { for _, test := range resultsTests { session := loginUser(t, test.user.Name) req := NewRequestf(t, "GET", "/%s.git/info/lfs/locks", test.repo.FullName()) - req.Header.Set("Accept", lfs.MediaType) + req.Header.Set("Accept", lfs.AcceptHeader) resp := session.MakeRequest(t, req, http.StatusOK) var lfsLocks api.LFSLockList DecodeJSON(t, resp, &lfsLocks) diff --git a/tests/integration/api_repo_lfs_test.go b/tests/integration/api_repo_lfs_test.go index 211dcf76c1128..6b42b83bc5a1e 100644 --- a/tests/integration/api_repo_lfs_test.go +++ b/tests/integration/api_repo_lfs_test.go @@ -84,7 +84,7 @@ func TestAPILFSBatch(t *testing.T) { newRequest := func(t testing.TB, br *lfs.BatchRequest) *RequestWrapper { return NewRequestWithJSON(t, "POST", "/user2/lfs-batch-repo.git/info/lfs/objects/batch", br). - SetHeader("Accept", lfs.MediaType). + SetHeader("Accept", lfs.AcceptHeader). SetHeader("Content-Type", lfs.MediaType) } decodeResponse := func(t *testing.T, b *bytes.Buffer) *lfs.BatchResponse { @@ -447,7 +447,7 @@ func TestAPILFSVerify(t *testing.T) { newRequest := func(t testing.TB, p *lfs.Pointer) *RequestWrapper { return NewRequestWithJSON(t, "POST", "/user2/lfs-verify-repo.git/info/lfs/verify", p). - SetHeader("Accept", lfs.MediaType). + SetHeader("Accept", lfs.AcceptHeader). SetHeader("Content-Type", lfs.MediaType) } From d5460198da250dce176060bb8f720ef61f91de51 Mon Sep 17 00:00:00 2001 From: Zoupers <1171443643@qq.com> Date: Sat, 1 Jun 2024 03:29:51 +0000 Subject: [PATCH 4/6] doc: add comment to AcceptHeader --- modules/lfs/shared.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/modules/lfs/shared.go b/modules/lfs/shared.go index ef7d951b50406..6454fb5f769c4 100644 --- a/modules/lfs/shared.go +++ b/modules/lfs/shared.go @@ -9,7 +9,8 @@ import ( const ( // MediaType contains the media type for LFS server requests - MediaType = "application/vnd.git-lfs+json" + MediaType = "application/vnd.git-lfs+json" + // Some LFS server might offer content with other types, so open with */* AcceptHeader = "application/vnd.git-lfs+json;q=0.9, */*;q=0.8" ) From 6cb79e3affdd0a2f3d73a44d1966d86aa466d445 Mon Sep 17 00:00:00 2001 From: Zoupers <1171443643@qq.com> Date: Tue, 4 Jun 2024 10:15:26 +0800 Subject: [PATCH 5/6] fix: remove special test handle --- modules/lfs/transferadapter_test.go | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/modules/lfs/transferadapter_test.go b/modules/lfs/transferadapter_test.go index f51ec18456f9a..7fec137efe569 100644 --- a/modules/lfs/transferadapter_test.go +++ b/modules/lfs/transferadapter_test.go @@ -24,12 +24,9 @@ func TestBasicTransferAdapterName(t *testing.T) { func TestBasicTransferAdapter(t *testing.T) { p := Pointer{Oid: "b5a2c96250612366ea272ffac6d9744aaf4b45aacd96aa7cfcb931ee3b558259", Size: 5} - downloadTestFlag := false roundTripHandler := func(req *http.Request) *http.Response { - if !downloadTestFlag { - assert.Equal(t, AcceptHeader, req.Header.Get("Accept")) - } + assert.Equal(t, AcceptHeader, req.Header.Get("Accept")) assert.Equal(t, "test-value", req.Header.Get("test-header")) url := req.URL.String() @@ -74,10 +71,6 @@ func TestBasicTransferAdapter(t *testing.T) { a := &BasicTransferAdapter{hc} t.Run("Download", func(t *testing.T) { - downloadTestFlag = true - t.Cleanup(func() { - downloadTestFlag = false - }) cases := []struct { link *Link expectederror string From 1b28411de3e26ade0f6bbf444cce049c9e64ec4b Mon Sep 17 00:00:00 2001 From: Zoupers <1171443643@qq.com> Date: Tue, 4 Jun 2024 10:25:54 +0800 Subject: [PATCH 6/6] apply suggestion from code review --- modules/lfs/shared.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/lfs/shared.go b/modules/lfs/shared.go index 6454fb5f769c4..80f4fed00d406 100644 --- a/modules/lfs/shared.go +++ b/modules/lfs/shared.go @@ -10,7 +10,7 @@ import ( const ( // MediaType contains the media type for LFS server requests MediaType = "application/vnd.git-lfs+json" - // Some LFS server might offer content with other types, so open with */* + // Some LFS servers offer content with other types, so fallback to '*/*' if application/vnd.git-lfs+json cannot be served AcceptHeader = "application/vnd.git-lfs+json;q=0.9, */*;q=0.8" )