git: ensure that pin matches checked-out commit #4473
Conversation
|
Test failure is unrelated, but also not good: |
| // git-checkout prefers branches in case of ambiguity | ||
| sha := headSha | ||
| if sha == "" { | ||
| sha = tagSha |
There was a problem hiding this comment.
tagSha can be empty in here as the full ref is also allowed, not only branch/tag name.
Also, if no matches, then error should be better, like in the old code.
There was a problem hiding this comment.
Good catch, my bad.
- I've restored the previous error message for when the ref can't be found.
- I've added tests for full refs (so this doesn't ever accidentally break)
- And then made sure that this patch pass them correctly
- I've also added tests for checking out branches (similar to how we checkout tags), which we somehow hadn't included before.
There was a problem hiding this comment.
I think this is still missing the case where lineRef == ref
There was a problem hiding this comment.
Ok, added this case in, and made sure we test for it (yay more tests 🎉)
There was a problem hiding this comment.
git ls-remote https://github.com/moby/buildkit.git pull/4473/head should be a valid match as well.
There was a problem hiding this comment.
I think this case works in the current state? Or are you suggesting there should be a test for that?
There was a problem hiding this comment.
Not sure why it should work. Note that the input/output do not match for this case:
# git ls-remote https://github.com/moby/buildkit.git pull/4473/head
ad56d1540d61e9b98911cc9f38696fa31a596b56 refs/pull/4473/head
There was a problem hiding this comment.
Should be fixed in the latest update I pushed.
jedevc
force-pushed
the
fix-git-sha-conflict
branch
from
6a4b0b8
to
ca23770
Compare
jedevc
force-pushed
the
fix-git-sha-conflict
branch
from
ca23770
to
ad56d15
Compare
Previously, it was very possible for the CacheKey function to return a
sha key that was *not* the checked out commit.
There are two cases that I've encountered where this can happen:
- An annotated tag will have the pin of the tag, and not the underlying
commit, which will be HEAD after the checkout.
- If multiple tags have the same path component (e.g. "mytag" and
"abc/mytag") then the first alphabetical tag will be selected when (in
this case "abc/mytag").
To avoid this kind of case, we can't just search for a single match in
the results for ls-remote. There's no way to filter for just an exact
match, so we need to scan through the output ourselves. Additionally, we
need to dereference the annotated tags by also selecting refs ending in
"^{}" - which have the commit that the tag points at.
Finally, I've improved the test suite around this to check that:
- The cache-key pin is equivalent to the checked out commit
- We can check out non-master branches
- That full ref syntax like "refs/heads/<branch-name>" and
"refs/tags/<tag-name>" (or even "refs/<anything>") can be used.
Signed-off-by: Justin Chadwell <[email protected]>
jedevc
force-pushed
the
fix-git-sha-conflict
branch
from
ad56d15
to
6a8d2ca
Compare
Originally reported by @vito in relation to https://github.com/dagger/dagger. You can see a simple example of this in https://play.dagger.cloud/playground/ak2KSRH_Gid.
Previously, it was very possible for the CacheKey function to return a sha key that was not the checked out commit.
There are two cases that I've encountered where this can happen:
To avoid this kind of case, we can't just search for a single match in the results for ls-remote. There's no way to filter for just an exact match, so we need to scan through the output ourselves. Additionally, we need to dereference the annotated tags by also selecting refs ending in "^{}" - which have the commit that the tag points at.