Missing fetch options: --force, --no-tags and --prune

[lupine]

Encouraged by the success of go-git in our elasticsearch indexer (https://gitlab.com/gitlab-org/gitlab-elasticsearch-indexer), I was hoping to use it in our repository mirroring code, as I'm adding support for SSH public key auth (https://gitlab.com/gitlab-org/gitlab-shell/issues/95 https://gitlab.com/gitlab-org/gitlab-shell/merge_requests/147). This is a simple wrapper around git fetch, but I don't seem able to replicate several aspects of existing behaviour with go-git:

• git fetch --force seems to be the default behaviour? I can't see any mention of the check --force disables in updateReferenceStorerIfNeeded or its callers, at any rate.

• git fetch --tags is FetchOptions.Tags = git.AllTags, but what about git fetch --no-tags? There doesn't seem to be a NoTags constant

• git fetch --prune seems unsupported. In fact, all aspects of git gc seem unsupported - although this could be worked around.

I'd be happy to submit PRs to implement the first two options if they'd be considered for merge.

[mcuadros]

Great news!

About the missing features:

git fetch --force seems to be the default behaviour? I can't see any mention of the check --force disables in updateReferenceStorerIfNeeded or its callers, at any rate.

Yep is the default behavior will be great having it as git does.

git fetch --tags is FetchOptions.Tags = git.AllTags, but what about git fetch --no-tags? There doesn't seem to be a NoTags constant

Great to have it in a PR

git fetch --prune seems unsupported. In fact, all aspects of git gc seem unsupported - although this could be worked around.

Yep we don't have gc, but at least can be implemented for the references.

[lupine]

@mcuadros for force, are you willing to change the default behaviour in v4 so that the default becomes not to force? Or would you see that as a breaking change?

It affects whether FetchOptions (and PullOptions) get a Force or NoForce member.

[lupine]

git fetch without --force will update all the refs it can, rather than failing early. Worth preserving?

[strib]

@lupine: I'm very interested in having a non-force mode as well. I'm curious what your plan is regarding locking, since presumably you'll need to lock the reference between when you check it and when you set it. Will this be a new ability of the Storer interface? Ideally it would be something that can be abstracted out, as different storage layers might need to do different things there.

Let me know if you need any help on the design or implementation.

(I have no opinions re: default behavior. Ideally it would match the command line in terms of not failing early, but I don't feel strongly about it.)

[strib]

Hi @lupine and @mcuadros -- has there been any progress on this issue? I'm working on a project that will need it relatively soon, so I can potentially look into it if no one else has started yet (though I'm not 100% sure where to start). If anyone has thoughts or a status update, that would be great. Thanks!

[mcuadros]

Which part you need? @lupine was working on it on this #497, but I don't know the status.

[strib]

I need the ability to turn off forced fetches in a consistent way. That is, without the "+" prefix to a ref, I want go-git to be able to check for fast-forward and update a ref all under the same lock, so that no other operation can come in between the check and the update.

[mcuadros]

Ok, sorry about the delay answering properly, but I have all the answers now:

implementation of git fetch --no-tags

This was implemented recently at 38b9d57

implementation of git fetch --force

Based on the git fetch man, --force is only used on exact refspecs, and looks UX thing, the general format of doing a forced fetch is adding the + to the refspec.

go-git checks if a fetch is ff when the references are being calculated, and if the fetch fails if the refspec doesn't force a non-ff update.

So I rather no implement it and close the PR #497.
In resume Remote.Fetch supports force and non-force updates.

implementation of git pull --force

This doesn't exists on the command, what exists is --ff, --ff-only or --no-ff.

Worktree.Pull is implemented as a --ff-only pull, since we don't implement any merge logic, and the other two options are based on merge commits.

lock of the storage during Remote.Fetch

go-git is not designed to access a repository with concurrent processes, this means that you will end with race-conditions or worst things.

Having a lock for ensure that the references remain the same during the fetch process goes against this design decision.

[strib]

implementation of git fetch --force

Based on the git fetch man, --force is only used on exact refspecs, and looks UX thing, the general > format of doing a forced fetch is adding the + to the refspec.

go-git checks if a fetch is ff when the references are being calculated, and if the fetch fails if the refspec doesn't force a non-ff update.

So I rather no implement it and close the PR #497.
In resume Remote.Fetch supports force and non-force updates.

Non-force fetch is required in order to implement a git remote helper using go-git, because in that case a user's push actually turns into a fetch from the local filesystem repo, and it's needed to distinguish a fast-forward push from a force push. (At least, that's the best way I've come up with so far to implement it.) We will definitely need the option to have a fetch that respects the + in the refspec. We're currently planning to implement it ourselves (probably @taruti will do it), and we hope you'll consider merging it.

[mcuadros]

As I said, you have non-force and force fetch based on refspecs, this doesn't work for you?

[strib]

Oh, I was confused by what you said I guess, because you said "I rather no implement it", so I thought you were not going to implement it.

If you mean today's code already supports force and non-force, I don't believe that's true. There's certainly nothing that takes a lock between checking the current HEAD and updating it, so non-force cannot work accurately. That's what we're planning to implement.

[mcuadros]

The force is based on "+" flag in the refspec, is how it works.

go-git/remote.go

Lines 442 to 446 in f9a1c7a

	if !rs.IsForceUpdate() {
	if err := checkFastForwardUpdate(r.s, remoteRefs, cmd); err != nil {
	return err
	}
	}

go-git/remote.go

Lines 554 to 566 in f9a1c7a

	func checkFastForwardUpdate(s storer.EncodedObjectStorer, remoteRefs storer.ReferenceStorer, cmd *packp.Command) error {
	if cmd.Old == plumbing.ZeroHash {
	_, err := remoteRefs.Reference(cmd.Name)
	if err == plumbing.ErrReferenceNotFound {
	return nil
	}
	if err != nil {
	return err
	}
	return fmt.Errorf("non-fast-forward update: %s", cmd.Name.String())
	}

Also what I tried to explain before is that a lock is not required from our point of view because go-git is not designed to having concurrent call to the same Repository object.

You can how its working at this test:

go-git/remote_test.go

Lines 452 to 478 in f9a1c7a

	func (s *RemoteSuite) TestPushRejectNonFastForward(c *C) {
	fs := fixtures.Basic().One().DotGit()
	server, err := filesystem.NewStorage(fs)
	c.Assert(err, IsNil)
	r, err := PlainClone(c.MkDir(), true, &CloneOptions{
	URL: fs.Root(),
	})
	c.Assert(err, IsNil)
	remote, err := r.Remote(DefaultRemoteName)
	c.Assert(err, IsNil)
	branch := plumbing.ReferenceName("refs/heads/branch")
	oldRef, err := server.Reference(branch)
	c.Assert(err, IsNil)
	c.Assert(oldRef, NotNil)
	err = remote.Push(&PushOptions{RefSpecs: []config.RefSpec{
	"refs/heads/master:refs/heads/branch",
	}})
	c.Assert(err, ErrorMatches, "non-fast-forward update: refs/heads/branch")
	newRef, err := server.Reference(branch)
	c.Assert(err, IsNil)
	c.Assert(newRef, DeepEquals, oldRef)
	}

[strib]

Oh I see. In this case, it's not the same Repository object. It would be a Repository object backed by a distributed storage layer, which could be accessed concurrently from multiple devices (like a real git server). I hope you will consider accepting a patch that supports that case, especially if it doesn't come with a local performance penalty for non-distributed repos.

[mcuadros]

Then you need a lock to accessing the repositories. As we do at:
https://github.com/src-d/borges

If you are planning to create a server, as part of this server you will need to provide a lock for the write operations.

Depending of the design of this feature, if for example is a the Storage layer can be an interesting feature.

Maybe can be as easy of add a Mutex to the storage.Storer interface and use this lock in all the write operations.

[mcuadros]

Closing this issue, feel free to open another issue with the feature request of the lock.

[strib]

Thanks, yes, we already have distributed lock in our storage layer. We'll open a PR for locking in the near future. We already have one for Billy that we hope to get merged: src-d/go-billy#44