Glide: Force-pushed branches break ~/.glide/cache

[jameshfisher]

Steps to reproduce:

1. Add a dependency github.com/X/Y to glide.yaml
2. glide update
3. Force-push to the branch in glide.yaml
4. glide update

Expected behavior: Glide updates to the latest commit of the branch.

Actual behavior: a repository under .glide/cache/src/X/Y goes into merge mode, resulting in confusing errors like:

[ERROR] Update failed for github.com/X/Y: github.com/X/Y contains uncommitted changes. Skipping update
[ERROR] Failed to do initial checkout of config: github.com/X/Y contains uncommitted changes. Skipping update

This has to be considered a bug - a cache is always supposed to be invisible to the end-user.

[mattfarina]

This is a hard problem.

If you've never read Linus thoughts on this it's worth reading.

Once a git history is published it's now a shared history. So, changing things is generally a bad idea because it messes with others who are consuming something.

There is a command to clear the cache. If an upstream dev does force push than the downstream devs have to deal with that. In Glides case users can clear the cache. If you have ideas on how to automate I'm game to listen.

But, as a rule... once a history is published it's tough on downstream people when there is a force push. Read what Linus said for a better explanation.

[mdpye]

I agree that upstream forcing-pushing was rude, especially in the golang ecosystem where SHAs _are_ the public version numbers.

But surely this case could be detected and the error message improved. If the cache has local changes then glide could recommend clearing the cache or even test to see if those changes are part of an open merge and hint at what might have happened.

It would have saved a bunch of time diagnosing the issue. For example, we weren't aware glide had a separate cache, so having checked all the possible $GOPATH locations and eliminated copies of the repo in question, it was pretty confusing to be told there were still local changes. Even adding the path to the local repo which has changes in the logging would be a significant improvement the first time you run in to this issue.

[jameshfisher]

I second @mdpye - force-push is terrible etiquette, but it should not result in such an unclear problem for people downstream.

How should we interpret the following entry in a glide.yaml?

- package: github.com/x/y
  version: foo

IMO, this should mean "I depend on the commit pointed at by the branch foo on the remote github.com/x/y." (I'm assuming foo is a branch, as it was in my case.)

In other words, this is about _remote_ branches, not local ones. The Glide cache should not even need local branches.

However, the Glide cache currently seems to use local branches, and when it updates its cache, it runs a git merge to update its local branch. This is how Glide got into the confused situation on my machine.

Skimming the Glide source, it appears to run a git fetch followed by a git pull.

git pull is git fetch followed by git merge, so Glide is currently running: git fetch && git fetch && git merge.

The second git fetch is redundant, and the git merge is undesirable, so I think it should just be running git fetch. That is, the git pull lines should just be deleted.

Then Glide will need to use the remote-tracking branch instead of the local branch when fetching from its cache.

Optionally, Glide could detect forced updates when fetching, and report these to the user as a warning about force-pushes upstream. git fetch reports these in this fashion:

 + 1abb5ee...9412d0d master     -> origin/master  (forced update)

Similar principles should apply to tags instead of branches, and the warnings should be more severe (on the principle that tags really really should never change).

[mdpye]

If the dependency definition specifies a branch name then surely the correct way to update the cache is git fetch followed by git reset --hard origin/<branchname>.

As @jameshfisher says, the dependency is declared in terms of the remote branch, so update should make it match what is there now, regardless of how the history moved to get there.

[sdboyer]

yep, you're identifying some definite problems with the glide cache implementation as it exists today. and i agree - the error message should be improved.

still, handling these states is tricky; i wouldn't expect an improved error message until after we move glide to gps (and even then, i've not implemented a specific error for that particular failure mode just yet). we should at least, however, be able to avoid strictly incorrect behavior that owes to treating the local state as though it were the remote state. (related: #592)

[hibooboo2]

:+1: for fixing this please.