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

list archive contents: lzop #230

Closed
wants to merge 9 commits into from
Closed

list archive contents: lzop #230

wants to merge 9 commits into from

Conversation

stokito
Copy link
Contributor

@stokito stokito commented Nov 2, 2018

In #229 ticket it mentioned that -l option now is supported by lzop.
So in this PR I implemented it.

@lukefromdc lukefromdc requested a review from a team November 2, 2018 22:09
Copy link
Member

@lukefromdc lukefromdc left a comment

Choose a reason for hiding this comment

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

After installing lzop support I was able to make a tar.lzo file, and could use either the "view all files" or "view as a folder" views but could also do this in master. How do I test this? Builds and runs fine.

@lukefromdc lukefromdc requested a review from a team November 2, 2018 22:32
@stokito
Copy link
Contributor Author

stokito commented Nov 2, 2018

How do I test this?

The proposed change affects three columns:

  • size - it shows uncompressed size i.e. real file size instead of size of archive
  • modified - it shows real modification time of the file inside the archive.
  • file name - a real name of the file which may differs. Also it can contain spaces, non ascii symbols

so lzop "some file" then rename mv "some file".lzo archive.lzo and try to open it with engrampa and it should show you "some file" instead of "archive".

Also I added xz support too. But it almost useless because it give us only uncompressed size.

Copy link
Member

@lukefromdc lukefromdc left a comment

Choose a reason for hiding this comment

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

OK, that works and does not in master. This is not something I would ever have seen on my own due to limited use cases

@lukefromdc lukefromdc requested a review from a team November 2, 2018 23:45
@lukefromdc
Copy link
Member

@monsta , @raveit65 , could someone else have a look at this? I'm a bit out of my depth with some of this

@raveit65
Copy link
Member

raveit65 commented Nov 4, 2018

See https://patch-diff.githubusercontent.com/raw/mate-desktop/engrampa/pull/230.diff
Intends can be improved at some places, ihmo.
Are familiar with the git rebase -i <last-commit-id-before-PR> command ?
With that you can edit/squash/reword commits in your local repo.
After all do a git pull -f to update your github repo and this PR.

@stokito
Copy link
Contributor Author

stokito commented Nov 4, 2018

Hi @raveit65 sorry it turned out that the branch was not synchronized with upstream. Also I already fixed indentions in scope of #225 so please merge it.

@raveit65
Copy link
Member

raveit65 commented Nov 4, 2018

@stokito
Copy link
Contributor Author

stokito commented Nov 4, 2018

If I understood your patch correctly, you moved the functions down. Right? But this broke a compilation.
Can you elaborate please what you tried to change?

@raveit65
Copy link
Member

raveit65 commented Nov 4, 2018

Common, this is you resulting patch :)
simply add .diff or .patch to the link from PR.
with .patch it shows all single commits
with .diff it shows one resulting commit.

@raveit65
Copy link
Member

raveit65 commented Nov 4, 2018

I only wanted to show your indents errors. ;)

@raveit65
Copy link
Member

raveit65 commented Nov 4, 2018

I can confirm that renaming a lzo archive don't rename a single file inside the archive any more.
But i don't understand the rest.
Did you add an option to list lzo archive properties?
Like engrampa -lvN archive.lzo ?

@stokito
Copy link
Contributor Author

stokito commented Nov 4, 2018

Yes, you'll always see a real file name regardless the archive name. This achieved by -N option of lzop as described in man lzop:

       -l, --list
           For each compressed file, list the following fields:

             method: compression method
             compressed: size of the compressed file
             uncompr.: size of the uncompressed file
             ratio: compression ratio
             uncompressed_name: name of the uncompressed file

           In combination with the --verbose option, the following fields are also displayed:

             date & time: time stamp for the uncompressed file

           With --name, the uncompressed name, date and time are those stored within the compress file if present.

           With --verbose, the size totals and compression ratio for all files is also displayed. With --quiet, the title and totals lines are not displayed.

           Note that lzop defines compression ratio as compressed_size / uncompressed_size.
``

@raveit65
Copy link
Member

raveit65 commented Nov 4, 2018

This doesn't work.

[rave@mother Test]$ engrampa -l archive.lzo 

** (engrampa:6164): CRITICAL **: 14:44:08.269: Failed to parse arguments: Unknown option -l

I have the feeling this is a big misunderstand between us......

@stokito
Copy link
Contributor Author

stokito commented Nov 4, 2018

I mean that engrampa internally calls lzop -lvN to get the real file name. But I didn't added any additional options to engrampa itself

@raveit65
Copy link
Member

raveit65 commented Nov 4, 2018

Ok, but where can i see this infos from lzop -lvN with engrampa?
Otherwise i don't know if this code works.

@stokito
Copy link
Contributor Author

stokito commented Nov 4, 2018

Let's take a file.txt of size 1KiB and with modification time, for example, 2018-11-04 18:46'
Then compress it with lzop to *.lzo file. This will produce an archive file.txt.lzo with about 400 bytes of size. But let's then rename the file.txt.lzo to another_archive_name.txt.lzo
Now if you try to open the archive with current version of engrampa you'll see:
file name: another_archive_name.txt and size 400 bytes i.e. the size of archive itself instead of original size of the file. Also a modification time will be the time of the archive.
So this patch fixed the problem and you'll see a real file size, it's real modification time and it's real name before it was compressed i.e. file.txt of size 1KiB and modification time of 2018-11-04 18:46.

@raveit65
Copy link
Member

raveit65 commented Nov 5, 2018

Thanks for better explanation, this will speed up review process.
I don't get why the lzo file is bigger than the origin txt file here, all without your PR.

[rave@mother Test]$ LANG=C
[rave@mother Test]$ dd if=/dev/urandom of=/home/rave/Test/file.txt bs=1k count=1
1+0 records in
1+0 records out
1024 bytes (1.0 kB, 1.0 KiB) copied, 9.2044e-05 s, 11.1 MB/s
[rave@mother Test]$ ls -la
total 12
drwxrwxr-x   2 rave rave 4096 Nov  5 10:53 .
drwx------. 36 rave rave 4096 Nov  5 09:33 ..
-rw-rw-r--   1 rave rave 1024 Nov  5 10:53 file.txt
[rave@mother Test]$ lzop file.txt 
[rave@mother Test]$ ls -la
total 16
drwxrwxr-x   2 rave rave 4096 Nov  5 10:54 .
drwx------. 36 rave rave 4096 Nov  5 09:33 ..
-rw-rw-r--   1 rave rave 1024 Nov  5 10:53 file.txt
-rw-rw-r--   1 rave rave 1086 Nov  5 10:53 file.txt.lzo

@stokito
Copy link
Contributor Author

stokito commented Nov 5, 2018

I guess that's because you tried to compress a random data which haven't enough of repeated sequences.
In a nutshell compressors working by analyzing the text and creating a dictionary of most used words and then just use a short code instead of whole word.
BTW for zstd you can use the dictionary as a separate file and create a dict by training on a set of files.
Thus you can create a quicker and more effective compressor for specific type of files

src/fr-command-cfile.c Outdated Show resolved Hide resolved
src/fr-command-cfile.c Outdated Show resolved Hide resolved
src/fr-command-cfile.c Outdated Show resolved Hide resolved
@raveit65
Copy link
Member

raveit65 commented Nov 5, 2018

Please don't use links in commit headers.

@raveit65
Copy link
Member

raveit65 commented Nov 5, 2018

Hmm, another-name.txt.xz shows 1 byte size inside the archive now.
lzop works fine, and i can confirm that another-name.txt.lzo shows the original file name and size inside the archive.
With *.gz files i couldn't reproduce the issue.

@raveit65
Copy link
Member

raveit65 commented Nov 5, 2018

Hmm, another-name.txt.xz shows 1 byte size inside the archive now.

Opps, my fault. Looks like last commit fixes the problem.

@raveit65
Copy link
Member

raveit65 commented Nov 6, 2018

Thank you,
any chance to use the git rebase command for editing commits to avoid commits which fixes only indents?

  1. git rebase -i <last-commit-id-before-PR> (-i = interactive)
    This will open an vi editor in terminal which is self-explanatory.
  2. After all is done do git push -f to update your remote repo at github. Be careful because -f (--force) can't be undo. Pull request will be updated automagically.

I am sure you will love this powerful command....

@raveit65
Copy link
Member

Should we close PR?
Or why you don't rebase PR?

@vkareh vkareh self-requested a review December 22, 2018 12:27
Copy link
Member

@vkareh vkareh left a comment

Choose a reason for hiding this comment

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

This builds and works well for me.

@vkareh
Copy link
Member

vkareh commented Dec 22, 2018

@stokito - I agree with @raveit65, you might want to rebase this.
Easiest way is running this: git rebase -i f06c2164fbc7b0e890fd2efdb05c0516f5ad3142
and in the resulting screen change from pick to squash those commits that should have been part of the previous commit.
The other alternative, not as easy, is to do a git reset f06c2164fbc7b0e890fd2efdb05c0516f5ad3142 and manually re-commit your changes.
In either case, then run git push --force to update your merge request.

@stokito
Copy link
Contributor Author

stokito commented Dec 24, 2018

Hi sorry for a late response. I will resend the PR but I would like to rewrite it.
First of all it turned out that gzip also supports an option -N which allows to see an original file name.
So we can get rid off binary manipulations inside of get_uncompressed_name_from_archive.
The second issue is that during uncompromising the resulting file still has a name of archive instead of original file name.
I guess we even should add an option to UI like "Uncompress with original file name".
But even now the PR is good enough: it adds a new functionality without affecting an old.
I can't exactly say when I send the new PR, but please don't close the PR.
Thank you

@raveit65
Copy link
Member

raveit65 commented Oct 9, 2019

What happens with that PR?
Is someone working on it or can we close it?

@stokito
Copy link
Contributor Author

stokito commented Oct 9, 2019

I'll finish it untill next week. Please do not close

@raveit65
Copy link
Member

Any progress?

@raveit65
Copy link
Member

Can be re-opened when author likes to continue.

@raveit65 raveit65 closed this Nov 16, 2020
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.

4 participants