fix: chalk not parsing some git ref objects #241
Conversation
root was was there was unnecessary .strip() and sometimes git objects end with `\n` which is part of a compressed data-stream hence by removing it zips checksum wasnt able to validate which is why it was making some tests flaky which were asserting git plugin keys
There was a problem hiding this comment.
Looks good, although I still see one warning locally when just running nimble build. Do you? Any idea what's going on there?
Before this PR:
$ nimble build
[...]
warn: unable to retrieve Git ref data: 045c6e2d6f357040991d2dd165e92dabafc0b6b0
warn: unable to retrieve Git ref data: fe750922dda1d33c407b41a7aeae7b17acdae78cWith this PR:
$ nimble build
[...]
warn: unable to retrieve Git ref data: 045c6e2d6f357040991d2dd165e92dabafc0b6b0 due to: invalid commit object$ git show 045c6e2d6f357040991d2dd165e92dabafc0b6b0 | head
tag v0.2.1
Tagger: John Viega <john@zork.org>
Date: Wed Oct 25 13:37:20 2023 -0400
Patch release.
commit bb1d5b0e10ffc692eb1e6945316a02a5e5f826eb
Author: John Viega <viega@users.noreply.github.com>
Date: Wed Oct 25 13:32:55 2023 -0400
|
hmm interesting. I dont get any warnings on this branch. let me try that commit directly |
|
|
|
@ee7 can you try with should work now |
| data = stream.readStr(initialReadSize) | ||
| byte = uint8(data[0]) | ||
| objType = ((byte shr 4) and 7) | ||
| if not (objType == gitObjCommit or objType == gitObjTag): |
There was a problem hiding this comment.
| if not (objType == gitObjCommit or objType == gitObjTag): | |
| if objType notin [gitObjCommit, gitObjTag]: |
Non-blocking suggestion: I personally find this more readable.
I also find that more readable than:
if objType != gitObjCommit and objType != gitObjTag:There was a problem hiding this comment.
notin didnt seem to work with uint8 and int comparison although either way this is faster as it can do direct boolean logic vs scanning a list
cc @drraid we actually spoke about it with him while pairing on a fix as he wrote all the original logic for the packed files
There was a problem hiding this comment.
notindidnt seem to work withuint8andintcomparison
Oh, yes. We should probably have writen:
gitObjCommit = 1'u8
gitObjTag = 4'u8or have a gitObjKind enum with values.
either way this is faster as it can do direct boolean logic vs scanning a list
Just FYI: The compiler can optimize it. But even in debug mode, the performance overhead there is orders of magnitude less than that of the unnecessary heap allocations in this file.
Consider:
const
a = 1
b = 4
proc foo(n: int): bool =
n notin [a, b]
proc bar(n: int): bool =
n != a and n != bCompiling with -d:release, I see foo producing the instructions:
endbr64
cmp rdi,0x1
je 12 <foo+0x12>
cmp rdi,0x4
setne al
ret
xor eax,eax
ret
data16 cs nop WORD PTR [rax+rax*1+0x0]and bar producing:
endbr64
xor eax,eax
cmp rdi,0x1
je 33 <bar+0x13>
cmp rdi,0x4
setne al
ret
data16 cs nop WORD PTR [rax+rax*1+0x0]
nop
Issue
flaky tests such as https://github.com/crashappsec/chalk/actions/runs/8282914433/job/22664882760
Description
root was was there was unnecessary .strip() and sometimes git objects end with
\nwhich is part of a compressed data-stream hence by removing it zips checksum wasnt able to validate which is why it was making some tests flaky which were asserting git plugin keysTesting
looks like chalk v0.3.3 tag ends with newline so interact with chalk repo with that tag being present: