Dep ensure silently updating lock file when encountering an orphaned commit

[kragniz]

Small repoducer here: https://github.com/kragniz/deptest

When running dep ensure on this project, my Gopkg.lock file gets modified to a different reviesion:

diff --git a/Gopkg.lock b/Gopkg.lock
index b57fc12..6025f24 100644
--- a/Gopkg.lock
+++ b/Gopkg.lock
@@ -7,10 +7,10 @@ memo = "992a6dae47ba88517cb895db7d8863c0b548be912a7fc57c623e77426d789d01"
   revision = "4c0e84591b9aa9e6dcfdf3e020114cd81f89d5f9"
 
 [[projects]]
-  branch = "release-3.1"
   name = "github.com/coreos/etcd"
   packages = ["auth/authpb","clientv3","etcdserver/api/v3rpc/rpctypes","etcdserver/etcdserverpb","mvcc/mvccpb","pkg/tlsutil"]
-  revision = "4dfc6a8a7e15229398c0a018b6d7a078cccae9c8"
+  revision = "e5b7ee2d03627ca33201da428b8110ef7c3e95f1"
+  version = "v3.1.6"

After looking at the verbose output, it appears that dep knows that the lock file references a particular revision, but bails on it:

(1)	✗   Unable to update checked out version: fatal: reference is not a tree: 4dfc6a8a7e15229398c0a018b6d7a078cccae9c8

This commit turned out to be orphaned in etcd/coreos (looks like a force pushed rebase or something).

My assumption here is that dep ensure should not modify existing revisions in the lock file, so should output an error when encountering situations like this rather than deviating from the revision in the lock file.

[carolynvs]

I agree that if -update wasn't specified, if dep can't resolve something in the lock anymore, prompting the user or at the very least returning an error would be less unexpected/surprising.

[sdboyer]

Ah, cool, glad we figured out the root cause. This is a tricky case.

First, I'm a bit surprised you still got that fatal message - I thought some of the last round of changes I made to gps would have caught that. Need more test cases!

Unfortunately, this particular issue - that you happened to be on an orphaned commit - is something that's very difficult to detect, as the methods we might use to discover that the git commit became orphaned will all eventually fail, I think, due to the way that git garbage collects orphaned commits. And of course, the rules are totally different (I don't even know them) for bzr or hg.

My assumption here is that dep ensure should not modify existing revisions in the lock file, so should output an error when encountering situations like this rather than deviating from the revision in the lock file.

The goal of dep ensure is, basically, to ensure that you have complete and correct end-to-end synchronization between your code, manifest, lock, and vendor dirs. Without e.g. -update, it's a further goal that any versions in the existing lock file be preserved.

But that's not a requirement, precisely because of situations like this - conditions can arise, even without any local changes having been made, that make it impossible to fulfill the synchronization requirement while maintaining the lock entirely. It's quite rare, but they can happen.

When they do, the tool opts to take the new solution, and doesn't tell the user about this choice at all. That's something we can control - e.g., an ensure -err-on-change flag, which causes dep to refuse a solution that changes anything in the lock file (but, I think probably, still accepts additions). This would be useful in a number of cases.

But I'd say the default behavior should still be accept the change, possibly with a warning to the user. (We need to work on ensure's output in general, anyway).

[kragniz]

First, I'm a bit surprised you still got that fatal message - I thought some of the last round of changes I made to gps would have caught that. Need more test cases!

To clarify, the fatal: reference is not a tree: comes from the output of dep ensure -v.

But I'd say the default behavior should still be accept the change, possibly with a warning to the user. (We need to work on ensure's output in general, anyway).

It still seems odd that the lock file is changed without some explicit action from the user. My current mental model of the lock file is that it is the source of truth of the exact versions required by the project. From what you're saying, it seems more like a suggestion that will be ignored if something external to the project changes (or otherwise becomes resolvable)?

[sdboyer]

To clarify, the fatal: reference is not a tree: comes from the output of dep ensure -v.

Yep, that's what I meant. That error is coming straight back from git when a higher-order command (e.g. git checkout) failed; I'm miffed because I thought the changes I made would have made the solver explicitly check the revision's existence prior to getting to the point where a higher-order command fails.

It still seems odd that the lock file is changed without some explicit action from the user.

dep ensure is that explicit action. We no doubt need to take more steps to communicate that to users, but I do think this is inevitably going to be a sticking point, because folks don't have accurate expectations about how hard a problem version selection is.

The default interaction model is based on the idea that, when you run into a tricky case, you may end up doing git reset --hard a few times before you get the outcome you want. That lets us make the default case much smoother, but supporting the behavior you want is still possible via the flag I described in my last comment.

My current mental model of the lock file is that it is the source of truth of the exact versions required by the project.

Yes, that's accurate - the lock file represents a set of precise, typically immutable versions for the entire transitive closure of dependencies for a project. But "the project" can be, and is, decomposed into just a bunch of arguments to an algorithm. When those inputs change, the lock may need to change as well.

Under most circumstances, if those arguments don't change, then the lock remains fine and correct. You've hit one one of the few cases where that guarantee doesn't apply. The fact that you ran dep ensure and it DID a solve is a product of some arguments changing; that solving failed because this particular commit had become stale is a separate problem.

From what you're saying, it seems more like a suggestion that will be ignored if something external to the project changes (or otherwise becomes resolvable)?

No - the only time a suggestion from a lock changes is if the solver cannot find a solution which uses the suggestion, or if the user explicitly indicated they wanted that version to move.

The new spec in #277 does describe a dep ensure -vendor-only flag, which skips any possibility of generating a new lock, and just populates vendor from the current one. That, combined with the other suggested flag, might be helpful for your desired approach.

Consider this - from dep's perspective, there's no difference between your case and one where someone went in and changed an a to a c in that commit hash. We might, depending on how long its been (per git's gc rules), be able to figure out that that commit did exist once. But in general, as far as we can tell, it's just garbage - so we ignore it.

[sdboyer]

closing this - while the more general case of invalid revs might be something we can do something about, this is focused on telling the user a commit is orphaned, which isn't feasible.