Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix directory type #6905

Merged
merged 13 commits into from
Jul 30, 2020
Merged

fix directory type #6905

merged 13 commits into from
Jul 30, 2020

Conversation

fuxingZhang
Copy link
Contributor

I use tar.append to add a folder and I get incorrect entry.type.

image

@CLAassistant
Copy link

CLAassistant commented Jul 28, 2020

CLA assistant check
All committers have signed the CLA.

@fuxingZhang fuxingZhang marked this pull request as draft July 28, 2020 11:11
@fuxingZhang fuxingZhang marked this pull request as ready for review July 29, 2020 09:42
@@ -383,7 +387,7 @@ export class Tar {
fileSize: pad(fileSize, 11),
mtime: pad(mtime, 11),
checksum: " ",
type: "0", // just a file
type: info?.isDirectory ?? opts.type === "directory" ? "5" : "0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn’t there an enum that can be used here? The value “5” and “0” are not very descriptive. (I see that it was using “0” before but that seems like bad code. Now would be a good opportunity to clean this up.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have updated the code, but I am not sure if it is appropriate.

I referenced some websites, and I add "fifo" and "contiguous-file" into Types

https://github.com/mafintosh/tar-stream
http://www.gnu.org/software/tar/manual/html_node/Standard.html

@fuxingZhang fuxingZhang requested a review from ry July 29, 2020 17:54
"directory" = 5,
"fifo" = 6,
"contiguous-file" = 7,
}
Copy link
Member

@ry ry Jul 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the value is only used as a string why not use a string here?

enum Types {
  "file" = "0",
  "link" = "1",
  // ...
}

I suggest "FileTypes" instead of "Types" - just a bit less ambiguous.

Copy link
Contributor Author

@fuxingZhang fuxingZhang Jul 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in method #getMetadata of Class Untar, convert a numeric string to string, such as "0" => "file"

// before
meta.type = types[meta.type as string] || meta.type;
// now
meta.type = FileTypes[parseInt(meta.type!)] ?? meta.type;

if the value is string, i get an error

enum FileTypes {
  "file" = "0",
  "link" = 1,
}
console.log(FileTypes.file);
// ok
console.log(FileTypes[1]);
// throw an error: Element implicitly has an 'any' type because index expression is not of type 'number'.ts(7015)
console.log(FileTypes["0"]);

meta.type is a numeric string
```ts
meta.type = FileTypes[parseInt(meta.type!)] ?? meta.type;
// meta.type = FileTypes[parseInt("0")] ?? "0";
```
Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - thanks @fuxingZhang

@ry ry merged commit 95597fc into denoland:master Jul 30, 2020
@fuxingZhang fuxingZhang deleted the patch-1 branch July 30, 2020 16:49
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 21, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 24, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 24, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 24, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Feb 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants