1
0
Fork
You've already forked zar
0

replace magic_string.len with magic_len, fix error message and reading loop bug #29

Closed
alichraghi wants to merge 5 commits from bug into master
pull from: bug
merge into: Moosichu:master
Moosichu:main
Moosichu:actions/cross-compile-redis-windows
Moosichu:feature/mri-parser-integration
Moosichu:actions/zar-redis-ranlib
Moosichu:feature/parse-bsd-symbols
alichraghi commented 2021年10月16日 08:47:16 +02:00 (Migrated from github.com)
Copy link
No description provided.
iddev5 commented 2021年10月16日 08:50:03 +02:00 (Migrated from github.com)
Copy link

replace magic_string.len with magic_len

Why?

> replace magic_string.len with magic_len Why?
iddev5 commented 2021年10月16日 08:59:41 +02:00 (Migrated from github.com)
Copy link

Just tested this.

Remember that the follow is actually supposed to be an error for ar:

!<arch>\n
\n
\n
\n

Before this PR: zar runs into endless loop
After this PR: zar exits quietly
Actual required behavior (LLVM-ar): shows error that archive is malformed

Just tested this. Remember that the follow is actually supposed to be an error for ar: ``` !<arch>\n \n \n \n ``` Before this PR: zar runs into endless loop After this PR: zar exits quietly Actual required behavior (LLVM-ar): shows error that archive is malformed
alichraghi commented 2021年10月16日 09:01:14 +02:00 (Migrated from github.com)
Copy link

thx for that. i will replace it with an error

thx for that. i will replace it with an error
moosichu commented 2021年10月16日 12:21:14 +02:00 (Migrated from github.com)
Copy link

Yeah, can you please change this PR to just the bug fix as requested? Please don't make unnecessary changes like replacing .len with a constant. It's unnecessary clutter.

Yeah, can you please change this PR to just the bug fix as requested? Please don't make unnecessary changes like replacing .len with a constant. It's unnecessary clutter.
alichraghi commented 2021年10月16日 12:30:16 +02:00 (Migrated from github.com)
Copy link

magic_len is unnecessary but it's more regular
anyway i can drop this commit. ayush accepted that

`magic_len` is unnecessary but it's more regular anyway i can drop this commit. ayush accepted that
iddev5 commented 2021年10月16日 12:37:39 +02:00 (Migrated from github.com)
Copy link

Okay, so maybe just revert the .len changes, squash and push.

The bug fix is good, just need to recheck it (later)

Okay, so maybe just revert the .len changes, squash and push. The bug fix is good, just need to recheck it (later)
alichraghi commented 2021年10月16日 12:38:28 +02:00 (Migrated from github.com)
Copy link

ok

ok
iddev5 (Migrated from github.com) requested changes 2021年10月16日 13:04:02 +02:00
@ -636,0 +616,4 @@
// TODO: actually error handle not expected number of bytes being read!
_=tryreader.read(string_table_contents);
// starting_seek_pos = starting_seek_pos + first_line_buffer.len + table_size;
break;
iddev5 (Migrated from github.com) commented 2021年10月16日 13:03:33 +02:00
Copy link
if (chars_read == 0) {
 return;
}

An archive containing only the magic is still valid, we cant error on that. This case need to be specially handled.

```suggestion if (chars_read == 0) { return; } ``` An archive containing only the magic is still valid, we cant error on that. This case need to be specially handled.
iddev5 commented 2021年10月16日 13:04:26 +02:00 (Migrated from github.com)
Copy link

Everything else is fine.

Everything else is fine.
moosichu commented 2021年10月16日 13:13:01 +02:00 (Migrated from github.com)
Copy link

I've integrated your change locall @AliChraghi and cleaned it up a tad, thank you for your contribution! It will be in soon :)

I've integrated your change locall @AliChraghi and cleaned it up a tad, thank you for your contribution! It will be in soon :)
alichraghi commented 2021年10月16日 13:14:05 +02:00 (Migrated from github.com)
Copy link

thank you. I was a little angry

thank you. I was a little angry

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
No reviewers
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
Moosichu/zar!29
Reference in a new issue
Moosichu/zar
No description provided.
Delete branch "bug"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?