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

Implement command 'list' to show archive contents #129

Merged
merged 6 commits into from
Nov 2, 2021

Conversation

AntonHermann
Copy link
Contributor

This implements a new command 'list' (alias 'l') to show archive contents.

I was unsure how to handle displaying directories:
In tar archives, each directory is listed as a separate file, while zip archives only list the files.

for example, having a file in nested dirs like so: a/b/file.txt will display as

$ ouch list archive.tar
a
a/b
a/b/file.txt

for tar archives and as

$ ouch list archive.zip
a/b/file.txt

for zip archives.

This is matching with the output of unzip -l archive.zip and tar -tf archive.tar but if we compine those in a single tool, maybe it would be preferable to choose only one of the above.


Currently only simple listing, 1 file per line, no tree and no further information is implemented.
A --tree command line option is included, that's what I will be working on next. Further, I added a FileInArchive struct as a common structure for both tar and zip archive files, this currently only contains the file's path, but metadata can be added in the future.

I wasn't sure how much functionality should be added to the list command: from "just print out some files" to "exa-style formatting" anything would be possible, but where to draw the line?

Maybe a tracking issue would be a good idea here (if me working on that command is of interest to you guys at all ^^ I don't want to push you to changes you're not comfortable with in this project :) )

@AntonHermann
Copy link
Contributor Author

Dammit, I forgot to reference the issue #6 and while looking it up, I notices that it was assigned to @vrmiguel, which I didn't see when I looked the last time :/

I'm sorry, I hope that is not a problem..

@vrmiguel vrmiguel requested review from marcospb19 and vrmiguel and removed request for marcospb19 October 29, 2021 14:10
@marcospb19
Copy link
Member

I'm sorry, I hope that is not a problem..

Not a problem! Don't worry.

The listing for tar is different from zip's, but I guess that's ok for this initial implementation, we can add a string trie to filter out redundant directories later (maybe just show directories if they are empty), but I'll leave this decision to @vrmiguel.

(Adding hacktoberfest-accepted label just in case, today is the last day.)

@marcospb19 marcospb19 added the hacktoberfest-accepted Tag PR as accepted for the hacktoberfest event label Oct 31, 2021
@AntonHermann
Copy link
Contributor Author

Oh damn, didn't know that pushing to the branch in my fork would apply to this PR as well, I wanted to do a separate PR for the tree implementation. And it appears that by force-pushing, I destroyed the commit you @vrmiguel approved already :(

Sorry, I'm new to contributing to a public project.

@marcospb19
Copy link
Member

No problem, in that case you should've probably created another branch starting from command_list, it would probably make for less conflicts because we already were about to merge your branch.

Just to confirm, the new branch commit you just did is the same as the last one? Or you want a new review because of new additions?

@AntonHermann
Copy link
Contributor Author

In the last commits, I implemented a tree option, so I think it should be reviewed again or at least the last changes :/

@marcospb19
Copy link
Member

Looks awesome, there's some specifics into the error treatment part that I can improve later.

Before I merge, this code looks familiar in some extent to the implementation at https://github.com/jez/as-tree, have you used that code for reference or inspiration?

If so, let me know, but don't worry, we would just need to add a link to the previous license in our LICENSE file, @jez used a compatible license there.

@AntonHermann
Copy link
Contributor Author

No, I looked for crates that I could use to build the tree, but couldn't find any after a quick search so I decided on doing it myself.

For the formatting I looked how exa does it, but it and lsd both have real tree structures to begin with while we only have a än unstructured list of paths

@marcospb19
Copy link
Member

I did some mistake, reverted last commit, trying again.

@AntonHermann
Copy link
Contributor Author

Something I was thinking about: currently, the root of the tree is the absolute path of the archive, but I think the path the user entered would look nicer.

However this information is lost when we normalize the paths. Do you think it would be okay to keep this information to improve the output later? If so, I could add that later on, either in a new PR or as a commit to this one :)

@marcospb19
Copy link
Member

I'd prefer to see it in another PR, if I understood correctly you are talking about not running this fs::canonicalize, right?

ouch/src/cli.rs

Lines 24 to 40 in cd77b98

*files = canonicalize_files(files)?;
let skip_questions_positively = if opts.yes {
QuestionPolicy::AlwaysYes
} else if opts.no {
QuestionPolicy::AlwaysNo
} else {
QuestionPolicy::Ask
};
Ok((opts, skip_questions_positively))
}
}
fn canonicalize_files(files: &[impl AsRef<Path>]) -> io::Result<Vec<PathBuf>> {
files.iter().map(fs::canonicalize).collect()
}

It's something I already had in mind, but I'd need to replace with other checks and fix some of the error treatment so that we don't lose some of the error messages it provides to us.

@AntonHermann
Copy link
Contributor Author

Yeah, however I didn't think about removing it, as I thought it would surely be there for good reason and instead keeping the original path and storing them together. But I haven't really put thought into where this canonicalization is necessary at all

@marcospb19
Copy link
Member

marcospb19 commented Nov 2, 2021

The canonicalization is being used just to check if the files are really there, we should use the Metadata function instead, it what Path::exists docs recommends for checking if files exist, but with proper error treatment capabilities.

@marcospb19 marcospb19 merged commit 9a9488f into ouch-org:master Nov 2, 2021
@marcospb19
Copy link
Member

I think I finally got it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted Tag PR as accepted for the hacktoberfest event
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants