empty git-upload-pack given errors since v5.4.0

[hiddeco]

We are extensive consumers of this library and besides the issue reported in #323, we have seen an increase of empty git-upload-pack given errors reported by our users since we upgraded from v5.3.0 to v5.4.x.

The error seems unrelated to the Git v2 protocol as reported in #64 or #157, given users that previously were able to perform operations on their e.g. GitHub repository using v5.3.0 are now no longer able to do so using v5.4.x.

Diving into the changelog nothing directly related seems to have changed, except for some plumbing changes in #246.

[zeripath]

I think this should be fixed following the merging of #329

[mcuadros]

Can you test this against the new v5.4.2?

[hiddeco]

Based on fluxcd/image-automation-controller#176 (comment), this does not seem to have been the cause.

[Robbilie]

looking at the MR, we are facing this issue with a basically fresh repo, single branch, ~10 super simple yaml files, nothing really big in any way

[zeripath]

I think this can be closed now?

[laszlocph]

@zeripath I still get this error with v5.4.2

With the following scenario:

opts := &git.CloneOptions{
		URL: fmt.Sprintf("%s/%s", "https://github.com", repoName),
		Auth: &http.BasicAuth{
			Username: user,
			Password: token,
		},
		Depth: 100,
	}
repo, err := git.PlainClone(repoPath, false, opts)

err = repo.Fetch(&git.FetchOptions{
		RefSpecs: []config.RefSpec{"refs/*:refs/*", "HEAD:refs/heads/HEAD"},
		Auth: &http.BasicAuth{
			Username: user,
			Password: token,
		},
		Depth: 100,
	})

repo, err := git.PlainOpen(path)
worktree.Pull(&git.PullOptions{
		RemoteName: "origin",
		Auth: &http.BasicAuth{
			Username: user,
			Password: token,
		},
	})

Currently running v5.3.0 as a workaround, where this works.

[zeripath]

Ok then I don't think this can be related to my changes. If you have a specific and repeatable test case you could try to bisect?

[qhua948]

Same problem here, first clone went ok, same problem when doing subsequent pull/fetch

[linneawang]

Same problem here, first clone went ok, same problem when doing subsequent pull/fetch

same here

[takirala]

Same problem here, first clone went ok, same problem when doing subsequent pull/fetch

same here

[laszlocph]

@zeripath The following test fails with empty git-upload-pack given, just replace xxx with a valid git personal token.

package worker

import (
	"github.com/go-git/go-git/v5"
	"github.com/go-git/go-git/v5/plumbing/transport/http"
	"github.com/stretchr/testify/assert"
	"os"
	"testing"
)

func Test_emptyUploadPack(t *testing.T) {
	repoPath := "/tmp/deleteme"
	os.RemoveAll(repoPath)
	os.MkdirAll(repoPath, 0777)

	token := "xxx"
	user := "abc123"

	opts := &git.CloneOptions{
		URL: "https://github.com/go-git/go-git",
		Auth: &http.BasicAuth{
			Username: user,
			Password: token,
		},
		Depth: 1,
		Tags:  git.NoTags,
	}

	repo, err := git.PlainClone(repoPath, false, opts)
	assert.Nil(t, err)

	err = repo.Fetch(&git.FetchOptions{
		Auth: &http.BasicAuth{
			Username: user,
			Password: token,
		},
		Depth: 1,
		Tags:  git.NoTags,
	})
	if err != nil && err != git.NoErrAlreadyUpToDate {
		t.Errorf(err.Error())
	}
}

[laszlocph]

The issue started with 320db9a

Works before

$ go get github.com/go-git/go-git/v5@db2bc57350561c4368a8d32c42476699b48d2a09
go get: downgraded github.com/go-git/go-git/v5 v5.3.1-0.20210512203949-320db9af8ba8 => v5.3.1-0.20210503000431-db2bc5735056
$ laszlo@laszlo-XPS-13-9370:~/projects/empty-uploadpack$ go test
PASS
ok  	github.com/laszlocph/empty-upload-pack	1.847s

Fails afterwards:

$laszlo@laszlo-XPS-13-9370:~/projects/empty-uploadpack$ go get github.com/go-git/go-git/v5@320db9af8ba8b0046e833013504eb07c61a3573c
go get: upgraded github.com/go-git/go-git/v5 v5.3.1-0.20210503000431-db2bc5735056 => v5.3.1-0.20210512203949-320db9af8ba8
$ laszlo@laszlo-XPS-13-9370:~/projects/empty-uploadpack$ go test
--- FAIL: Test_emptyUploadPack (1.50s)
    uploadpack_test.go:41: empty git-upload-pack given
FAIL
exit status 1
FAIL	github.com/laszlocph/empty-upload-pack	1.509s

[greenpau]

Have the same issue greenpau/caddy-git#2 when using PlainOpen() followed by worktree Pull().

[greenpau]

The empty git-upload-pack given is being returned by remote.fetch call in PullContext().

go-git/worktree.go

Lines 73 to 82 in a5bbcd2

	fetchHead, err := remote.fetch(ctx, &FetchOptions{
	RemoteName: o.RemoteName,
	RemoteURL: o.RemoteURL,
	Depth: o.Depth,
	Auth: o.Auth,
	Progress: o.Progress,
	Force: o.Force,
	InsecureSkipTLS: o.InsecureSkipTLS,
	CABundle: o.CABundle,
	})

The fetch() returns error from fetchPack here:

go-git/remote.go

Lines 463 to 465 in a5bbcd2

	if err = r.fetchPack(ctx, o, s, req); err != nil {
	return nil, err
	}

The fetchPack returns error from transport.UploadPackSession.UploadPack:

go-git/remote.go

Lines 547 to 550 in a5bbcd2

	reader, err := s.UploadPack(ctx, req)
	if err != nil {
	return err
	}

The UploadPackSession is an interface and it has UploadPack method:

go-git/plumbing/transport/common.go

Lines 74 to 84 in a5bbcd2

	// UploadPackSession represents a git-upload-pack session.
	// A git-upload-pack session has two steps: reference discovery
	// (AdvertisedReferences) and uploading pack (UploadPack).
	type UploadPackSession interface {
	Session
	// UploadPack takes a git-upload-pack request and returns a response,
	// including a packfile. Don't be confused by terminology, the client
	// side of a git-upload-pack is called git-fetch-pack, although here
	// the same interface is used to make it RPC-like.
	UploadPack(context.Context, *packp.UploadPackRequest) (*packp.UploadPackResponse, error)
	}

The UploadPack returns transport.ErrEmptyUploadPackRequest, because the req in remote.go passes an empty request.

go-git/plumbing/transport/http/upload_pack.go

Lines 35 to 41 in a5bbcd2

	func (s *upSession) UploadPack(
	ctx context.Context, req *packp.UploadPackRequest,
	) (*packp.UploadPackResponse, error) {
	if req.IsEmpty() {
	return nil, transport.ErrEmptyUploadPackRequest
	}

The UploadPackRequest is empty when Haves are contained in the Wants, or if Wants length is zero:

go-git/plumbing/protocol/packp/uppackreq.go

Lines 41 to 45 in a5bbcd2

	// IsEmpty a request if empty if Haves are contained in the Wants, or if Wants
	// length is zero
	func (r *UploadPackRequest) IsEmpty() bool {
	return isSubset(r.Wants, r.Haves)
	}

🤔 should we modify remote.go such that when the req is empty, then we don't do UploadPack()?

[ydcool]

Got same issue. I have to downgrade to v5.3.0, what's the progress now?

[talwat]

I also have the same issue, v5.3.0 seems to work but I'm worried about using an older version. Is there any kind of workaround?

[greenpau]

@ydcool , @talwat , I am solving the issue by not specifying depth.

[talwat]

Yes, but limiting depth is super useful and I need it. If I don’t specify depth my program is way slower.

[loxia-byte]

Got same issue. I have to downgrade to v5.3.0, what's the progress now?

Same here.

[mmessmore]

I wasn't specifying Depth on the Pull(), but was on the initial Clone() and get this error on any subsequent Pull().

If I remove specifying Depth on the Clone(), the Pull()s work fine.

This is not ideal for me since my service is useless until the initial clone is complete and making it shallower reduces that time. The repo I'm working with has a very long (and mostly irrelevant) history, so I was trying to limit that initial transfer. I just need the last couple weeks of changes at a given point in time.

[Charles546]

Hey team, what is going on here? Are we not going to fix this?

[taigrr]

These changes did end up fixing the issue I was having in one of my repos.

Any chance we could get a new version tagged release so we can go get -u ?

Thanks in advance, go-git team.

[pjbgf]

@taigrr The version v5.8.0 was released last week and should contain the fix.

[williambanfield]

Using Version 5.8.0

Hey, I think there may still be an issue here. It appears that this fails if I don't specify a Depth in the Pull function. If I run the command line git pull within a shallow cloned directory, the pull succeeds.

The reason this is valuable is that specifying Depth will cause the Pull to fail if the the number of commits between the local HEAD and the remote HEAD is greater than the depth. This happens because git cannot determine that the fetched commits are descendants of the local branch. Command line git appears to pull enough commits to determine the ancestry, which was the behavior I was hoping would exist here. However, when I run without the Depth specified, I run into the same empty git-upload-pack given error.

Let me know if I'm misunderstanding something.

The following script reproduces the issue by panic'ing on the error from Pull:

package main

import (
	"os"
	"strings"

	gogit "github.com/go-git/go-git/v5"
	"github.com/go-git/go-git/v5/plumbing/transport/http"
)

func main() {
	t := os.Getenv("GITHUB_API_TOKEN")

	auth := &http.BasicAuth{
		Username: "username",
		Password: strings.TrimSuffix(t, "\n"),
	}

	cloneOptions := gogit.CloneOptions{
		Auth:         auth,
		URL:          "xxx",
		SingleBranch: true,
		Depth:        1,
	}

	d, err := os.MkdirTemp("", "")
	if err != nil {
		panic(err)
	}
	defer os.RemoveAll(d)
	repo, err := gogit.PlainClone(d, false, &cloneOptions)
	if err != nil {
		panic(err)
	}
	w, err := repo.Worktree()
	if err != nil {
		panic(err)
	}
	err = w.Pull(&gogit.PullOptions{
		Auth: auth,
	})
	if err != nil && err != gogit.NoErrAlreadyUpToDate {
		panic(err)
	}
}

[AriehSchneier]

@pjbgf I took a look and this case is different to what I fixed. This is trying to unshallow a repo after it was cloned as shallow. If you add this code to the end of TestFetchAfterShallowClone it reproduces the issue:

	// Try fetch with no depth specified to unshallow
	s.testFetch(c, r, &FetchOptions{
		Tags: NoTags,

		RefSpecs: []config.RefSpec{
			"+refs/heads/master:refs/heads/master",
			"+refs/heads/master:refs/remotes/origin/master",
		},
	}, []*plumbing.Reference{
		plumbing.NewReferenceFromStrings("refs/heads/master", sha5.String()),
		plumbing.NewReferenceFromStrings("refs/remotes/origin/master", sha5.String()),
		plumbing.NewSymbolicReference("HEAD", "refs/heads/master"),
	})

It seems we are only handling shallow commits if we are requesting a depth greater than 0, so it fails if we try to unshallow our clone.

In Remote.fetch I tried changing this section:

	if !req.Depth.IsZero() {
		req.Shallows, err = r.s.Shallow()
		if err != nil {
			return nil, fmt.Errorf("existing checkout is not shallow")
		}
	}

to

	req.Shallows, err = r.s.Shallow()
	if err != nil && !req.Depth.IsZero() {
		return nil, fmt.Errorf("existing checkout is not shallow")
	} else if len(req.Shallows) > 0 && req.Depth.IsZero() {
		// trying to unshallow, need to set the shallow capability
		if err := req.Capabilities.Set(capability.Shallow); err != nil {
			return nil, err
		}
	}

However it seems we don't update the existing list of shallows correctly (ie remove commits that are no longer shallow) during fetchPack and I don't know the git protocol well enough to know what is missing from the code.

Any chance you can take a look?

[pjbgf]

@AriehSchneier I will try to take a look at it later on this week. In the meantime, will re-open this issue.

[pjbgf]

@AriehSchneier I had an initial look at this, by shallow cloning the repo then unshallowing it:

GIT_TRACE_PACKET=true git -c protocol.version=1 clone --no-progress --no-tags --depth 1 --single-branch https://github.com/go-git/go-git

GIT_TRACE_PACKET=true git -c protocol.version=1 fetch --no-progress --unshallow

On the fetch (trying to unshallow), go-git does not seem to be sending that shallow and deepen across:

09:32:38.194674 pkt-line.c:86           packet:   fetch-pack> want cd6170c6f808453f58dcfd15c6c59345e3df402b multi_ack_detailed no-done side-band-64k thin-pack no-progress ofs-delta deepen-since deepen-not agent=git/2.41.0
09:32:38.194680 pkt-line.c:86           packet:   fetch-pack> shallow cd6170c6f808453f58dcfd15c6c59345e3df402b
09:32:38.194683 pkt-line.c:86           packet:   fetch-pack> deepen 2147483647
09:32:38.194685 pkt-line.c:86           packet:   fetch-pack> 0000

Which means it would not receive the unshallow back:

09:32:38.194741 pkt-line.c:86           packet:          git< 00a4want cd6170c6f808453f58dcfd15c6c59345e3df402b multi_ack_detailed no-done side-band-64k thin-pack no-progress ofs-delta deepen-since deepen-not agent=git/2.41.00034shallow cd6170c6f808453f58dcfd15c6c59345e3df402b0015deepen 21474836470000
09:32:38.194751 pkt-line.c:86           packet:          git< 0000
09:32:38.343729 pkt-line.c:86           packet:   fetch-pack< unshallow cd6170c6f808453f58dcfd15c6c59345e3df402b
09:32:38.343766 pkt-line.c:86           packet:   fetch-pack< 0000

[pjbgf]

@williambanfield Just wanted to call out on your example that you probably want to add Tags: git.NoTags on your CloneOptions, as that will impact the amount of commits you will be shallowing as it will bring depth + the number of unique referenced commits from all the tags within the repository. You can check that with cat .git/shallow.

[Tang8330]

Still getting this error when going a clone with a depth specified, then running git pull without depth on v5.9

[Xe]

I just ran into this today