fix: empty git-upload-pack given on remote.fetch()

[greenpau]

More info: This change prevents an attempt to upload pack
when UploadPackRequest is empty.

Partial Resolution: #328

Signed-off-by: Paul Greenberg greenpau@outlook.com

[mcuadros]

can you cover it with some test?

[greenpau]

can you cover it with some test?

@mcuadros , will do! 👍 will also need to do some adjustments to the code.

[greenpau]

@mcuadros , moved the check to the UploadPack function. The was introduced to ./plumbing/transport/internal/common/common.go in #311.

Here, I add the same to the UploadPack functions in the following files.

• ./plumbing/transport/http/upload_pack.go
• ./plumbing/transport/file/server.go

Created TestFetchShallows test, where provided depth 0.

Receiving the following error in the unrelated repository_test.go.

FAIL: repository_test.go:640: RepositorySuite.TestPlainOpenNotExistsDetectDotGit

repository_test.go:646:
    c.Assert(err, Equals, ErrRepositoryNotExists)
... obtained = nil
... expected *errors.errorString = &errors.errorString{s:"repository does not exist"} ("repository does not exist")

I am not 💯 sure about the TestFetchShallows. Please take a look and let me know what to adjust in the test. Many thanks!

[magnusbaeck]

This patch doesn't help in my situation, which perhaps isn't surprising since I'm not making a shallow fetch. What's puzzling to me is that the wanted SHA-1 in wants really exists in the haves list so it's clear why isEmpty returns true. But shouldn't a commit in haves be available in the git, i.e. a checkout or git show operation would succeed? If I comment out the conditional in upload_pack.go that returns ErrEmptyUploadPackRequest I get an empty packfile and a ref that points to a non-existent object.

I apologize if you think I'm derailing your PR, maybe my problem is something entirely different (mysterious problems are often a case of PEBKAC), but it might also hint at a different root cause whereas the current state of this PR only addresses a particular corner case. Either way, I'd be happy to help debug the problem.

[greenpau]

I apologize if you think I'm derailing your PR, maybe my problem is something entirely different (mysterious problems are often a case of PEBKAC), but it might also hint at a different root cause whereas the current state of this PR only addresses a particular corner case. Either way, I'd be happy to help debug the problem.

@magnusbaeck , this is valuable discussion! 👍 Please challenge, propose something new, etc.

[magnusbaeck]

The reason my wanted SHA-1 ended up in haves was that HEAD contained that SHA-1. My code looks like this:

	err = worktree.Checkout(&git.CheckoutOptions{
		Hash:  plumbing.NewHash(commitID),
		Force: true,
	})
	if errors.Is(err, plumbing.ErrObjectNotFound) {
		refSpec := fmt.Sprintf("+%s:tmp", ref)
		err = gr.git.FetchContext(ctx, &git.FetchOptions{
			RefSpecs: []gitconfig.RefSpec{gitconfig.RefSpec(refSpec)},
			Tags:     git.NoTags,
		})
		...

It seems the failing checkout operation still ends up storing the wanted SHA-1 in HEAD, so when the subsequent fetch operation computes the haves set it picks up the SHA-1 from HEAD, resulting in an empty (wants − haves) set and hence ErrEmptyUploadPackRequest.

I'll double check to make sure the SHA-1 didn't end up in HEAD by other means, but if my analysis is correct I'll file an issue for the checkout behavior unless there already is one.

[magnusbaeck]

I'll double check to make sure the SHA-1 didn't end up in HEAD by other means, but if my analysis is correct I'll file an issue for the checkout behavior unless there already is one.

For posterity my unrelated problem was filed as #457.

[sarabala1979]

Can you test it with non-zero value?

[l0nax]

@greenpau @mcuadros Is there any reason this PR cannot be merged?

[greenpau]

Is there any reason this PR cannot be merged?

@l0nax , I cannot merge that. Not a maintainer of this package.

[greenpau]

Is there any reason this PR cannot be merged?

@l0nax , I cannot merge that. Not a maintainer of this package.

[pjbgf]

@greenpau do you mind rebasing your PR and also adding a test case to validate non-zero values for Depth?

[greenpau]

@pjbgf , I rebased. Unfortunately, it has been a while since I looked into this code. Could you add a test yourself?

[AriehSchneier]

This can be closed as it was fixed in #792