Support Range header end in lfs

[burbon]

Current implementation of Range header support in LFS only uses range start and ignores range end.
We have a use case where for media files we want to download a fixed range data including range end. This change is an attempt to allow it.

[zeripath]

@burbon Thanks for the PR.

More Range support

Unfortunately, your proposed PR breaks the unspecfied end range option: Range: bytes=<range-start>- which should continue to support. It may also be helpful to support the Range: bytes=-<suffix-length> format too.

Take a look at: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Range

There are a few others that could be useful to support too.

Tests

I wonder if you would be able to add a test into either:

1. integrations/git_test.go preferably around here:

gitea/integrations/git_test.go

Lines 107 to 111 in 505e456

	littleLFS, bigLFS := lfsCommitAndPushTest(t, dstPath)
	rawTest(t, &sshContext, little, big, littleLFS, bigLFS)
	mediaTest(t, &sshContext, little, big, littleLFS, bigLFS)
	t.Run("BranchProtectMerge", doBranchProtectPRMerge(&sshContext, dstPath))

bigLFS is the git SHA of the large object. Here shows how to get the pointer file

gitea/integrations/git_test.go

Line 227 in 505e456

req = NewRequest(t, "GET", path.Join("/", username, reponame, "/raw/branch/master/", bigLFS))

or

2. In integrations/lfs_getobject_test.go, probably within this function:

gitea/integrations/lfs_getobject_test.go

Line 59 in 505e456

func doLfs(t *testing.T, content *[]byte, expectGzip bool) {

[codecov-io]

Codecov Report

Merging #11314 into master will increase coverage by 0.01%.
The diff coverage is 25.00%.

@@            Coverage Diff             @@
##           master   #11314      +/-   ##
==========================================
+ Coverage   43.82%   43.83%   +0.01%     
==========================================
  Files         607      607              
  Lines       86919    86926       +7     
==========================================
+ Hits        38094    38106      +12     
+ Misses      44117    44113       -4     
+ Partials     4708     4707       -1     

Impacted Files	Coverage Δ
modules/lfs/server.go	40.17% <25.00%> (-0.19%)	⬇️
modules/queue/unique_queue_disk_channel.go	53.84% <0.00%> (-1.93%)	⬇️
modules/git/repo.go	50.62% <0.00%> (-1.26%)	⬇️
models/repo_list.go	76.51% <0.00%> (-0.81%)	⬇️
services/pull/pull.go	32.63% <0.00%> (+0.19%)	⬆️
models/gpg_key.go	53.95% <0.00%> (+0.52%)	⬆️
modules/log/event.go	65.64% <0.00%> (+1.02%)	⬆️
services/pull/check.go	55.48% <0.00%> (+1.82%)	⬆️
modules/queue/workerpool.go	60.14% <0.00%> (+2.13%)	⬆️
modules/git/command.go	89.56% <0.00%> (+2.60%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 505e456...b683741. Read the comment docs.

[burbon]

Hi @zeripath
Thanks for looking at it. Indeed previous change broke unspecified end range. This is fixed now and covered by integration tests - thanks for pointing to them.
If this is OK i'm happy to add suffix length as well in the same fashion, but not that keen on multiple range sets.
Found Range header implemented here:
https://github.com/golang/go/blob/master/src/net/http/fs.go#L219
But not sure if it is worth to try to use it - there are additional checks i.e. for If-Modified headers
which probably are not needed in this case?

[burbon]

Pressed the close button accidentally.

[zeripath]

Unfortunately the way you've changed the tests is not going to work. I'll have a think about how best to fix this.

[zeripath]

You would need to move the preamble:

defer prepareTestEnv(t)()
setting.CheckLFSVersion()
if !setting.LFS.StartServer {
	t.Skip()
	return
}

to the start of each of those Test... functions.

I would also remove the use of the do... prefix from doLFS function as we should save the do is for declarative tests - that is a test that is self contained and could be run from any other test. Here doLFS is a function that returns a value rather than test.

Let's take an example of (at least what I call) a declarative test:

func doAPICreateRepository(ctx APITestContext, empty bool, callback ...func(*testing.T, api.Repository)) func(*testing.T) {
	return func(t *testing.T) {
		createRepoOption := &api.CreateRepoOption{
			AutoInit:    !empty,
			Description: "Temporary repo",
			Name:        ctx.Reponame,
			Private:     true,
			Gitignores:  "",
			License:     "WTFPL",
			Readme:      "Default",
		}
		req := NewRequestWithJSON(t, "POST", "/api/v1/user/repos?token="+ctx.Token, createRepoOption)
		if ctx.ExpectedCode != 0 {
			ctx.Session.MakeRequest(t, req, ctx.ExpectedCode)
			return
		}
		resp := ctx.Session.MakeRequest(t, req, http.StatusCreated)

		var repository api.Repository
		DecodeJSON(t, resp, &repository)
		if len(callback) > 0 {
			callback[0](t, repository)
		}
	}
}

I can use doAPICreateTestRepository as a subtest in multiple other tests without having to rewrite it and if I care about the results I can pass in a callback that will still run in that current subtest so I can mark that subtest failing. In fact we use doAPICreateTestRepository in multiple places , the most declarative example is in ssh_key_test.go:

gitea/integrations/ssh_key_test.go

Lines 87 to 214 in dbb7497

	func testKeyOnlyOneType(t *testing.T, u *url.URL) {
	// Once a key is a user key we cannot use it as a deploy key
	// If we delete it from the user we should be able to use it as a deploy key
	reponame := "ssh-key-test-repo"
	username := "user2"
	u.Path = fmt.Sprintf("%s/%s.git", username, reponame)
	keyname := fmt.Sprintf("%s-push", reponame)
	// OK login
	ctx := NewAPITestContext(t, username, reponame)
	otherCtx := ctx
	otherCtx.Reponame = "ssh-key-test-repo-2"
	failCtx := ctx
	failCtx.ExpectedCode = http.StatusUnprocessableEntity
	t.Run("CreateRepository", doAPICreateRepository(ctx, false))
	t.Run("CreateOtherRepository", doAPICreateRepository(otherCtx, false))
	withKeyFile(t, keyname, func(keyFile string) {
	var userKeyPublicKeyID int64
	t.Run("KeyCanOnlyBeUser", func(t *testing.T) {
	dstPath, err := ioutil.TempDir("", ctx.Reponame)
	assert.NoError(t, err)
	defer os.RemoveAll(dstPath)
	sshURL := createSSHUrl(ctx.GitPath(), u)
	t.Run("FailToClone", doGitCloneFail(sshURL))
	t.Run("CreateUserKey", doAPICreateUserKey(ctx, keyname, keyFile, func(t *testing.T, publicKey api.PublicKey) {
	userKeyPublicKeyID = publicKey.ID
	}))
	t.Run("FailToAddReadOnlyDeployKey", doAPICreateDeployKey(failCtx, keyname, keyFile, true))
	t.Run("FailToAddDeployKey", doAPICreateDeployKey(failCtx, keyname, keyFile, false))
	t.Run("Clone", doGitClone(dstPath, sshURL))
	t.Run("AddChanges", doAddChangesToCheckout(dstPath, "CHANGES1.md"))
	t.Run("Push", doGitPushTestRepository(dstPath, "origin", "master"))
	t.Run("DeleteUserKey", doAPIDeleteUserKey(ctx, userKeyPublicKeyID))
	})
	t.Run("KeyCanBeAnyDeployButNotUserAswell", func(t *testing.T) {
	dstPath, err := ioutil.TempDir("", ctx.Reponame)
	assert.NoError(t, err)
	defer os.RemoveAll(dstPath)
	sshURL := createSSHUrl(ctx.GitPath(), u)
	t.Run("FailToClone", doGitCloneFail(sshURL))
	// Should now be able to add...
	t.Run("AddReadOnlyDeployKey", doAPICreateDeployKey(ctx, keyname, keyFile, true))
	t.Run("Clone", doGitClone(dstPath, sshURL))
	t.Run("AddChanges", doAddChangesToCheckout(dstPath, "CHANGES2.md"))
	t.Run("FailToPush", doGitPushTestRepositoryFail(dstPath, "origin", "master"))
	otherSSHURL := createSSHUrl(otherCtx.GitPath(), u)
	dstOtherPath, err := ioutil.TempDir("", otherCtx.Reponame)
	assert.NoError(t, err)
	defer os.RemoveAll(dstOtherPath)
	t.Run("AddWriterDeployKeyToOther", doAPICreateDeployKey(otherCtx, keyname, keyFile, false))
	t.Run("CloneOther", doGitClone(dstOtherPath, otherSSHURL))
	t.Run("AddChangesToOther", doAddChangesToCheckout(dstOtherPath, "CHANGES3.md"))
	t.Run("PushToOther", doGitPushTestRepository(dstOtherPath, "origin", "master"))
	t.Run("FailToCreateUserKey", doAPICreateUserKey(failCtx, keyname, keyFile))
	})
	t.Run("DeleteRepositoryShouldReleaseKey", func(t *testing.T) {
	otherSSHURL := createSSHUrl(otherCtx.GitPath(), u)
	dstOtherPath, err := ioutil.TempDir("", otherCtx.Reponame)
	assert.NoError(t, err)
	defer os.RemoveAll(dstOtherPath)
	t.Run("DeleteRepository", doAPIDeleteRepository(ctx))
	t.Run("FailToCreateUserKeyAsStillDeploy", doAPICreateUserKey(failCtx, keyname, keyFile))
	t.Run("MakeSureCloneOtherStillWorks", doGitClone(dstOtherPath, otherSSHURL))
	t.Run("AddChangesToOther", doAddChangesToCheckout(dstOtherPath, "CHANGES3.md"))
	t.Run("PushToOther", doGitPushTestRepository(dstOtherPath, "origin", "master"))
	t.Run("DeleteOtherRepository", doAPIDeleteRepository(otherCtx))
	t.Run("RecreateRepository", doAPICreateRepository(ctx, false))
	t.Run("CreateUserKey", doAPICreateUserKey(ctx, keyname, keyFile, func(t *testing.T, publicKey api.PublicKey) {
	userKeyPublicKeyID = publicKey.ID
	}))
	dstPath, err := ioutil.TempDir("", ctx.Reponame)
	assert.NoError(t, err)
	defer os.RemoveAll(dstPath)
	sshURL := createSSHUrl(ctx.GitPath(), u)
	t.Run("Clone", doGitClone(dstPath, sshURL))
	t.Run("AddChanges", doAddChangesToCheckout(dstPath, "CHANGES1.md"))
	t.Run("Push", doGitPushTestRepository(dstPath, "origin", "master"))
	})
	t.Run("DeleteUserKeyShouldRemoveAbilityToClone", func(t *testing.T) {
	sshURL := createSSHUrl(ctx.GitPath(), u)
	t.Run("DeleteUserKey", doAPIDeleteUserKey(ctx, userKeyPublicKeyID))
	t.Run("FailToClone", doGitCloneFail(sshURL))
	})
	})
	}

In particular take a look at:

gitea/integrations/ssh_key_test.go

Lines 169 to 187 in 57b6f83

	t.Run("DeleteRepositoryShouldReleaseKey", func(t *testing.T) {
	otherSSHURL := createSSHUrl(otherCtx.GitPath(), u)
	dstOtherPath, err := ioutil.TempDir("", otherCtx.Reponame)
	assert.NoError(t, err)
	defer os.RemoveAll(dstOtherPath)
	t.Run("DeleteRepository", doAPIDeleteRepository(ctx))
	t.Run("FailToCreateUserKeyAsStillDeploy", doAPICreateUserKey(failCtx, keyname, keyFile))
	t.Run("MakeSureCloneOtherStillWorks", doGitClone(dstOtherPath, otherSSHURL))
	t.Run("AddChangesToOther", doAddChangesToCheckout(dstOtherPath, "CHANGES3.md"))
	t.Run("PushToOther", doGitPushTestRepository(dstOtherPath, "origin", "master"))
	t.Run("DeleteOtherRepository", doAPIDeleteRepository(otherCtx))
	t.Run("RecreateRepository", doAPICreateRepository(ctx, false))

The style almost self-documents - it says exactly what it is testing and you can see how using these components you can build a progressively more complex test. A test that follows a simple story of what it is doing.

(That's not to say these tests are bad - they're just not declarative and componentisable. Making these componentisable may not be sensible - I cannot at present imagine we would want to request specific ranges in other tests, but we should try to keep the componentised test namespace as clean as possible.)

[burbon]

Thanks for fixing tests. There is no need to return after t.Skip() isn't it?

[zeripath]

I think it's good practice to just return tbh

[burbon]

I think it's good practice to just return tbh

Can't find it being used in golang src.

Myself I found it a bit confusing, suggesting that code after Skip() is still being executed, while it is not. Skip() just calls runtime.Goexit().