[jedevc]

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:

• 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.

[jedevc]

Test failure is unrelated, but also not good:

 === Failed
=== FAIL: client TestIntegration/TestLocalSourceDiffer/worker=oci-rootless/differ=metadata (0.05s)
    client_test.go:1892: 
        	Error Trace:	/src/client/client_test.go:1892
        	            				/src/client/client_test.go:1825
        	Error:      	Not equal: 
        	            	expected: []byte{0x66, 0x6f, 0x6f}
        	            	actual  : []byte{0x62, 0x61, 0x72}
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1,3 +1,3 @@
        	            	 ([]uint8) (len=3) {
        	            	- 00000000  66 6f 6f                                          |foo|
        	            	+ 00000000  62 61 72                                          |bar|
        	            	 }
        	Test:       	TestIntegration/TestLocalSourceDiffer/worker=oci-rootless/differ=metadata
        --- FAIL: TestIntegration/TestLocalSourceDiffer/worker=oci-rootless/differ=metadata (0.05s)

[tonistiigi]

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.

[jedevc]

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.

[tonistiigi]

I think this is still missing the case where lineRef == ref

[jedevc]

Ok, added this case in, and made sure we test for it (yay more tests 🎉)

[tonistiigi]

git ls-remote https://github.com/moby/buildkit.git pull/4473/head should be a valid match as well.

[jedevc]

I think this case works in the current state? Or are you suggesting there should be a test for that?

[tonistiigi]

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

[jedevc]

Should be fixed in the latest update I pushed.