fix: readObject when reading from packed objects with format wrapped (isomorphic-git#1935)

* fix: return packed object result early in readObject.js

* chore: simplify conditional logic in readObject.js

* chore: add tests for reading different format objects from packfile

* docs: add @limond as a contributor

Author: limond

.all-contributorsrc

@@ -710,6 +710,16 @@
         "doc",
         "bug"
       ]
+    },
+    {
+      "login": "limond",
+      "name": "Leon Kaucher",
+      "avatar_url": "https://avatars.githubusercontent.com/u/1025682?v=4",
+      "profile": "https://github.com/limond",
+      "contributions": [
+        "code",
+        "test"
+      ]
     }
   ],
   "commitConvention": "angular"

README.md

@@ -374,6 +374,7 @@ Thanks goes to these wonderful people ([emoji key](https://github.com/kentcdodds
   </tr>
   <tr>
     <td align="center"><a href="https://github.com/lsegurado"><img src="https://avatars.githubusercontent.com/u/27731047?v=4?s=60" width="60px;" alt=""/><br /><sub><b>Lucas Martin Segurado</b></sub></a><br /><a href="https://github.com/isomorphic-git/isomorphic-git/commits?author=lsegurado" title="Documentation">📖</a> <a href="https://github.com/isomorphic-git/isomorphic-git/issues?q=author%3Alsegurado" title="Bug reports">🐛</a></td>
+    <td align="center"><a href="https://github.com/limond"><img src="https://avatars.githubusercontent.com/u/1025682?v=4?s=60" width="60px;" alt=""/><br /><sub><b>Leon Kaucher</b></sub></a><br /><a href="https://github.com/isomorphic-git/isomorphic-git/commits?author=limond" title="Code">💻</a> <a href="https://github.com/isomorphic-git/isomorphic-git/commits?author=limond" title="Tests">⚠️</a></td>
   </tr>
 </table>
 

__tests__/test-readObject.js

@@ -132,7 +132,7 @@ describe('readObject', () => {
       '"78019d93c9cea3481084e7cc53d4ddea76010586d1cc68001b0cde30fc06ec1b4b4161966237e6e9dbd373ed53e729158a487d522a625a55f9005896e5ff183a8c01869194f2c206411162cc210c798cc294976431158524e1a0148a308e9926ec703d8008a51207c544c6bc2023568c60ca238e97e2084708c274938492248712138e03a11df0f3b204fbbc1c680dfe7a55e4e7f66f568579f93da6d53f8015208724c44108bec1cf05e6a37e1007fc3bd9acc9fa3c03dffe1b75679867601b36704de3ac7cdd9cdd4f9d011eeefa9cd67f02a31e6d034c2c0318905fcd58551455c91443bd5a8f2789a8a2506b67ddadf1e0bbb9180a9e70b3d71f4837c595c5f2b6a306fdc0615590b39e013cb1674ede3a0795e8c354ac467725091cbf26b7b47b7314fb7e22de9d22ae8b79566e835aa78b5798b2923966cc9ebf4e0c2042301c4fd731d294c34bb2fcc99b68b0fb5a5e9e72d956493569c877afda715cd1866271ed6f9ca9e8beb6b0898ad71eed18700a280905b937fdc75a0fe34720aaef7b4bf477915a4729d3f4c9719767deaa66d4db9ba0e54e043d0be5702f8565f6f89101ad567022a9c971b52a5e69508edc3d3106555e954fbe29d833f65b87dfc88bb31064b3509f038b955a778e97a850f4cb9d012215c8265c9fda923db4be2aef74756cb4e6f94eaa46196c2a96ecad47215fe6aa70b4268dc873e670fbc1250e8ae4cd8501b5d90436aab3375ae4dbbb0b82796ef2ebb55e175ebd1e0fd930198d545ff49c5291b5b55c7ef6dcb5bace713faa177c5931609be8ad5070f6e9fc38bef26540b6b43352cfa2767424c9dd46d4cf4fda78f7d65c7a26902ee5ba6537e2dee89732ed0afcf926cf3d60155abc22cca6f384d16672ce7b4f529452de124cf963df3c319d6c2e7fc7da5eb7219fb98d76488e8eea68e88b01010b5555df4a35d54e813547428beb2e5de183934809ca36d610bf97d6cb0af52a4a8669480879bea3d2f2b2cc487d0b0c8895f0b576efe6c3e01dda2931cbe68f4d3f85f0a99b2e7e56bbc5c4d1a8117749fc0b77b9f88e379d12f27ebcb6c75ba6440cb8e633e1a2cace3a9ec8f5dc72dbaa66c0df68b53d33ffd76477defeaa248c59351d9d30e8704fcb093b3805030524ac9312838a761814799df480a61f4bda7f074a928001f743cffc00fa8263c9"'
     )
   })
-  it('from packfile', async () => {
+  it('from packfile deflated', async () => {
     // Setup
     const { fs, gitdir } = await makeFixture('test-readObject')
     // Test
@@ -142,6 +142,48 @@ describe('readObject', () => {
       oid: '0b8faa11b353db846b40eb064dfb299816542a46',
       format: 'deflated',
     })
+    // packed objects will always be returned as 'content' format
+    expect(ref.format).toEqual('content')
+    expect(ref.type).toEqual('commit')
+    expect(ref.source).toBe(
+      'objects/pack/pack-1a1e70d2f116e8cb0cb42d26019e5c7d0eb01888.pack'
+    )
+    if (ref.format !== 'content') throw new Error('wrong type')
+    expect(Buffer.from(ref.object).toString('hex')).toMatchInlineSnapshot(
+      `"7472656520346431363665323666636639666537623231383633343336313337633434613339613231613930660a706172656e7420666264353662343964343030613139656531383561653733353431376264623334633038343632310a617574686f722057696c6c69616d2048696c746f6e203c776d68696c746f6e40676d61696c2e636f6d3e2031353038323034303133202d303430300a636f6d6d69747465722057696c6c69616d2048696c746f6e203c776d68696c746f6e40676d61696c2e636f6d3e2031353038323034303133202d303430300a0a696e646578206f6e206d61737465723a2066626435366234204164642027756e706b6727206b657920746f207061636b6167652e6a736f6e0a"`
+    )
+  })
+  it('from packfile wrapped', async () => {
+    // Setup
+    const { fs, gitdir } = await makeFixture('test-readObject')
+    // Test
+    const ref = await readObject({
+      fs,
+      gitdir,
+      oid: '0b8faa11b353db846b40eb064dfb299816542a46',
+      format: 'wrapped',
+    })
+    // packed objects will always be returned as 'content' format
+    expect(ref.format).toEqual('content')
+    expect(ref.type).toEqual('commit')
+    expect(ref.source).toBe(
+      'objects/pack/pack-1a1e70d2f116e8cb0cb42d26019e5c7d0eb01888.pack'
+    )
+    if (ref.format !== 'content') throw new Error('wrong type')
+    expect(Buffer.from(ref.object).toString('hex')).toMatchInlineSnapshot(
+      `"7472656520346431363665323666636639666537623231383633343336313337633434613339613231613930660a706172656e7420666264353662343964343030613139656531383561653733353431376264623334633038343632310a617574686f722057696c6c69616d2048696c746f6e203c776d68696c746f6e40676d61696c2e636f6d3e2031353038323034303133202d303430300a636f6d6d69747465722057696c6c69616d2048696c746f6e203c776d68696c746f6e40676d61696c2e636f6d3e2031353038323034303133202d303430300a0a696e646578206f6e206d61737465723a2066626435366234204164642027756e706b6727206b657920746f207061636b6167652e6a736f6e0a"`
+    )
+  })
+  it('from packfile content', async () => {
+    // Setup
+    const { fs, gitdir } = await makeFixture('test-readObject')
+    // Test
+    const ref = await readObject({
+      fs,
+      gitdir,
+      oid: '0b8faa11b353db846b40eb064dfb299816542a46',
+      format: 'content',
+    })
     expect(ref.format).toEqual('content')
     expect(ref.type).toEqual('commit')
     expect(ref.source).toBe(

src/storage/readObject.js

@@ -45,41 +45,44 @@ export async function _readObject({
       oid,
       getExternalRefDelta,
     })
-  }
-  // Finally
-  if (!result) {
-    throw new NotFoundError(oid)
+
+    if (!result) {
+      throw new NotFoundError(oid)
+    }
+
+    // Directly return packed result, as specified: packed objects always return the 'content' format.
+    return result
   }
 
+  // Loose objects are always deflated, return early
   if (format === 'deflated') {
     return result
   }
 
+  // All loose objects are deflated but the hard-coded empty tree is `wrapped` so we have to check if we need to inflate the object.
   if (result.format === 'deflated') {
     result.object = Buffer.from(await inflate(result.object))
     result.format = 'wrapped'
   }
 
-  if (result.format === 'wrapped') {
-    if (format === 'wrapped' && result.format === 'wrapped') {
-      return result
-    }
-    const sha = await shasum(result.object)
-    if (sha !== oid) {
-      throw new InternalError(
-        `SHA check failed! Expected ${oid}, computed ${sha}`
-      )
-    }
-    const { object, type } = GitObject.unwrap(result.object)
-    result.type = type
-    result.object = object
-    result.format = 'content'
+  if (format === 'wrapped') {
+    return result
   }
 
-  if (result.format === 'content') {
-    if (format === 'content') return result
-    return
+  const sha = await shasum(result.object)
+  if (sha !== oid) {
+    throw new InternalError(
+      `SHA check failed! Expected ${oid}, computed ${sha}`
+    )
+  }
+  const { object, type } = GitObject.unwrap(result.object)
+  result.type = type
+  result.object = object
+  result.format = 'content'
+
+  if (format === 'content') {
+    return result
   }
 
-  throw new InternalError(`invalid format "${result.format}"`)
+  throw new InternalError(`invalid requested format "${format}"`)
 }