Added checks for volume files by aonez · Pull Request #38 · abbeycode/UnrarKit

aonez · 2017-01-09T11:25:06Z

From pull #36

This one contains various functions to check wether the file is a parted RAR volume, the get the first part path and to get the list of all the parts paths of the volume.

aonez · 2017-01-09T11:30:58Z

Here a sample code using some of the functions:

NSError *archiveError = nil;
URKArchive *archive = [[URKArchive alloc] initWithPath:rar error:&archiveError];
NSError *error = nil;

if (archive.isVolume) {
	NSLog(@"isVolume");
	NSLog(@"First volume: %@", [archive firstVolumePath]);
            
	URKArchive *volarchive = [[URKArchive alloc] initWithPath:[archive firstVolumePath] error:&archiveError];
	NSArray<NSString*> *paths = [volarchive listVolumePaths:&error];
	NSLog(@"List of volumes:");

	for (NSString *path in paths) {
		NSLog(@"%@", path);
	}
            
}
else
	NSLog(@"NOT isVolume");

Even if isVolume returns false, the listVolumePaths can return the list of volumes, if any.

aonez · 2017-01-09T20:35:05Z

Just noticed the code does not return true when asked if it is a volume if it's called with the first part. (.rar or part1.rar). The flag (MHD_VOLUME) used to detect if it is a volume returns false with that first volume.

Those flags might be needed, but are always returning false for the first volume part:
MHD_FIRSTVOLUME
LHD_SPLIT_AFTER

abbeycode · 2017-01-11T13:19:40Z

Thanks for keeping up on this! It looks like it'll make a good addition to the library. I'll review as soon as I'm able to.

abbeycode · 2017-01-13T18:20:52Z

I'm starting to take a look at this now. To give me some context, can you explain how your code is using multi-volume archives, and how each of the methods you defined fits into your use case?

Also, have you looked at the RAR spec? It is helpful in figuring out the right way to approach problems like these.

http://www.rarlab.com/technote.htm

aonez · 2017-01-15T10:59:36Z

I needed a way to list all the parts of a volume, so I can delete them all after an extraction. Also it is useful when a user tries to extract a volume using all the parts, I can select the first one and dismiss the others.

First, this one tells me if the file is a volume part, so I can search for the first one and list all the parts. I'll try 0x0010 flag to detect the first volume (thanks for the technote tip):

- (BOOL)isVolume:(NSURL *)fileURL

If it is a volume, I want to be sure it is the first part. With this function I'll get the first volume path:

- (NSString *)firstVolumePath:(NSString *)filePath

Then, I can just extract the file using the first path. Also, I can get all the volume paths from the first one, so I can delete them or dismiss them from the extraction process:

- (NSArray<NSString*> *)listVolumePaths:(NSError **)error

This last function maybe can be done in another way. Right now I list the contents of the file so I can get the paths of all (found) volumes. Maybe it can be done using 0x0008 and 0x0010 flags and wishing the previous/next volume path and checking for it.

aonez · 2017-01-15T11:00:24Z

I'll be using it here: aonez/Keka#8

aonez · 2017-01-15T11:13:01Z

Checked again, ~~maybe I can't get the volume info in the header because I'm using RARHeaderDataEx instead of RARHeaderData~~. Just checked RARHeaderData gets the flags from RARHeaderDataEx.

abbeycode · 2017-02-04T13:31:59Z

Sorry it took me a while to get back to you, but I was getting the code all cleaned up to work with the latest Xcode version, and it took longer than expected. That's great you found a better way to list the volume paths! Can you push an updated commit?

Also, I'm curious about why you need the - (BOOL)isVolume:(NSURL *)fileURL and - (NSString *)firstVolumePath:(NSString *)filePath overloads. When are the simpler - (BOOL)isVolume and - (NSString *)firstVolumePath not sufficient?

I'll try to keep up on this a little more to get it out the door. I'm going to be pushing a branch I created that'll help us get started on unit tests for these new methods. I'm using the rar CLI tool to create some multi-volume archives and run tests on them. I'll post more when I'm ready for you to look at them and integrate them into your PR.

aonez · 2017-02-05T13:02:32Z

@abbeycode not hurry at all. Sadly I did not found any other way to get the first part of the volume detected as a volume, nor get a consistent way to detect the next/previous volume existence. So right now the code is as here.

The overloads are not necessary for me. I'm using with the opened archive (self.filename), but I think maybe someone will need to pass another path, and why not coding it already this capability. If you like I can change it to use always self.filename and get rid of the path overload, no problem at all.

Let me know how the tests go! Maybe you get motivated and find the right flag to get the first volume path detected ;)

aonez · 2017-02-05T13:42:13Z

Just rebased with your master, seems there're too many changes now in the pull request...

abbeycode · 2017-02-05T13:46:23Z

That's strange that it's showing all of those as changes. Did you do a git rebase, or did you git merge?

aonez · 2017-02-05T13:48:05Z

Did a rebase but using the Fork GUI. First rebase for me by the way, so not surprised it got broken...

abbeycode · 2017-02-05T13:49:27Z

Interesting... I think it messed it up. Also, regardless of what it did, I think GitHub expects you to just merge, not rebase. I've run into it before. Can you try doing a hard reset of your branch to a previous revision and do it over?

aonez · 2017-02-05T13:50:35Z

Sure, I’ll try to fix it right away :)

aonez · 2017-02-05T15:05:43Z

Done! Hard reset and a merge did it.

… a pull request's tests (PR #38)

abbeycode · 2017-02-09T23:54:56Z

I added the tests and stubbed out methods. I renamed the one method to be a little clearer, and added NSURL-based overloads and documentation.

Also, while I was running the tests with your current implementation, it looks like you're going to want to use this in your isVolume check:

bool isVolume = (flags->Flags & MHD_VOLUME) != 0;

RARReadHeaderEx() reads file headers, not the archive header.

Let me know if you have any other questions! Also, you should probably update the PR to merge into that new multivolume branch I created instead of master. I'll merge it in from there.

aonez · 2017-02-10T17:00:24Z

You got it, using flags the first volume is detected. I'll spend some time next week to merge it into the branch and take a look at all the test and modifications you made. Thanks @abbeycode!

aonez · 2017-02-26T13:51:32Z

Little busy right now, but I'll get to it!

abbeycode · 2017-08-16T16:15:21Z

Hello. It's been a while, and I was wondering if you were still planning on pushing this PR over the finish line. If so, let me know and I'll bring the multivolume branch up to date with the v2.9 branch, and keep it up to date.

aonez · 2017-08-17T08:50:39Z

Just was thinking about this some days ago. Sure I will!

aonez · 2017-08-17T09:43:10Z

Ok, so my PR is updated to the current status of the multivolume branch. Let me know when you update it with v2.9changes so I'll update it again 😅

abbeycode · 2017-08-17T15:00:42Z

I updated the multivolume branch. Let's make sure the Travis build completes successfully before you merge. I'll send another note when it looks good.

abbeycode · 2017-08-17T15:39:37Z

The build failed, but that's because of the unit tests that this PR will fix, so you may proceed!

aonez · 2017-08-17T22:51:24Z

Should work now, let's see what travis says 😛

abbeycode · 2017-08-18T00:01:13Z

Hmmmm still looks like the new unit tests are failing...

aonez · 2017-08-18T04:22:22Z

I'll take a look in a few hours...

aonez · 2017-08-18T11:45:59Z

Cleaned all my code and failed again. Then I saw your branch failed too. Look at your latest commit travis CI status.

I'm mostly sure this is why it failed yesterday. I'm not familiar with travis CI, maybe you'll see whats the problem there.

aonez

Fillet = Filled 😑

abbeycode

Thanks for picking this back up! I'm excited to include this new functionality in the library. I have one incredibly minor nitpick, and one more substantial issue. Please take a look when you can, and we'll get this over the finish line.

abbeycode · 2017-09-29T11:48:59Z

Classes/URKArchive.mm

+        return false;
+    }
+
+    return (volumeURLs.count > 1);


Very very minor suggestion: No need for the parentheses here.

abbeycode · 2017-09-29T11:50:21Z

Classes/URKArchive.mm

+        @try {
+            NSError *firstVolumeError = nil;
+
+            if ([self _unrarOpenFile:firstVolumeURL.path


Please take a look at this suggestion. It's the only real thing holding this back. Thanks for hopping back on this request!

aonez · 2017-09-29T15:23:52Z

Classes/URKArchive.h

 *  @return Returns the URL, or nil if there was an error
 */
- (nullable NSURL *)firstVolumeURL;
+- (NSURL *)firstVolumeURL:(NSURL *)fileURL;


Those will be not nullable, since they will be called after a (!fileURL) check. If no first part, they will return the passed fileURL.

aonez · 2017-09-29T15:24:21Z

Classes/URKArchive.mm

+        return false;
+    }
+
+    return volumeURLs.count > 1;


No extra parenthesis 👍

aonez · 2017-09-29T15:25:08Z

Classes/URKArchive.mm


- (nullable NSURL *)firstVolumeURL {
-    NSString *path = [self firstVolumePath];
+- (NSURL *)firstVolumeURL:(NSURL *)fileURL {


Changed the contents to the NSURL method because init uses NSURL

I have a few thoughts on this.

Since we're working with regular expressions in the firstVolume... method, it makes sense for it to use a path. Let it be true to itself

I think it would be clearer if firstVolumePath were a class method that takes a path, since it isn't using any internal state of the class

It only needs to be private. We can get rid of the public firstVolumeURL and firstPathURL instance methods, since all instances are going to represent the first volume. Basically, it will always be true that firstVolumeURL == fileURL

aonez · 2017-09-29T15:25:26Z

Classes/URKArchive.mm

+            return [NSURL fileURLWithPath:volumePath];
+        }
+        else
+            URKLogInfo("First volume part %@ not found. Skipping first volume selection", volumePath);


Just skip, do not return nil. URKLogInfo, this will get to the final user?

No, all logging is for the dev to see, not the user.

Also, look at the README for logging suggestions. This should probably be Debug level

aonez · 2017-09-29T15:25:48Z

Classes/URKArchive.mm

+            URKLogInfo("First volume part %@ not found. Skipping first volume selection", volumePath);
+    }
+
+    return fileURL;


If no match, always return the passed fileURL

aonez · 2017-09-29T15:27:18Z

Classes/URKArchive.mm

-    return @[];
+    NSMutableOrderedSet<NSString*> *volumePaths = [NSMutableOrderedSet new];
+
+    NSArray<URKFileInfo*> *listFileInfo = [self listFileInfo:error];


Way cleaner 😃

The NSMutableOrderedSet class doesn't sort members as they're added. You're better off using a regular NSMutableSet and sorting it when you convert to an array.

aonez · 2017-09-29T15:28:02Z

Classes/URKArchive.mm

        }
-
+
+        fileURL = [self firstVolumeURL:fileURL];


Just added it before (!fileURL) check and [self init], let me know if you think there's a better place

aonez · 2017-09-29T15:29:48Z

Classes/URKArchive.mm


-    if (![path length]) {
-        return nil;
+    URKLogDebug("Checking if the archive is part of a volume...");


Added URKLogDebug trough all the process

aonez · 2017-09-29T15:33:28Z

If the changes are ok for you, the case tests calls to [archive firstVolumePath] and [archive firstVolumeURL] should be replaced to [archive filename] and [archive fileURL].

Alse we need to determine what to do with this check:

- (void)testMultipleVolume_FirstVolumeMissing;

Maybe add an overload to firstVolumeURL to return nil if not found so the developer can forcibly ask for it?

abbeycode

I appreciate all of your work on this. I still feel like there's some work to be done, but I'll leave it up to you whether you want to continue going through the PR feedback loop or not. I'm fine will pulling your changes into the multivolume branch at this point and making the changes I'm suggesting myself. If you feel like you're getting something out of the review process, though, then we can keep going and bring it over the finish line together. It's your call.

abbeycode · 2017-09-29T20:43:08Z

Classes/URKArchive.mm


- (nullable NSURL *)firstVolumeURL {
-    NSString *path = [self firstVolumePath];
+- (NSURL *)firstVolumeURL:(NSURL *)fileURL {


I have a few thoughts on this.

Since we're working with regular expressions in the firstVolume... method, it makes sense for it to use a path. Let it be true to itself

I think it would be clearer if firstVolumePath were a class method that takes a path, since it isn't using any internal state of the class

It only needs to be private. We can get rid of the public firstVolumeURL and firstPathURL instance methods, since all instances are going to represent the first volume. Basically, it will always be true that firstVolumeURL == fileURL

abbeycode · 2017-09-29T20:44:32Z

Classes/URKArchive.mm

-        return nil;
+    URKLogDebug("Checking if the archive is part of a volume...");
+
+    __block NSString *volumePath = fileURL.path;


This doesn't need to be a __block variable, since it's not modified from inside a block. Also, once we refactor to a single class method (+[URKArchive firstVolumePath:]), it'll be the input argument.

abbeycode · 2017-09-29T20:51:44Z

Classes/URKArchive.mm

+    __block NSString *volumePath = fileURL.path;
+    NSTextCheckingResult * match;
+
+    if (volumePath.length)


Invert this if and exit early. If volumePath is empty, log an error and return nil. That way we un-nest everything below, and you don't need to check for match twice. You can simplify the rest of the method's conditions.

abbeycode · 2017-09-29T20:56:06Z

Classes/URKArchive.mm

-    return @[];
+    NSMutableOrderedSet<NSString*> *volumePaths = [NSMutableOrderedSet new];
+
+    NSArray<URKFileInfo*> *listFileInfo = [self listFileInfo:error];


The NSMutableOrderedSet class doesn't sort members as they're added. You're better off using a regular NSMutableSet and sorting it when you convert to an array.

aonez · 2017-09-30T09:04:31Z

Go ahead with the pull. I think now all the elements are working, it's just a matter of "coding" preferences and optimisations that maybe you'll do faster. I'll take a look at the final code once is there 🤓 Obviously if you need anything from my part just ask.

abbeycode · 2017-10-02T12:01:12Z

@aonez Thanks again for all your work on this! I'll mention you in the PR I create to merge the multivolume branch into my release branch (v2.9).

abbeycode · 2017-10-03T12:16:19Z

@aonez Take a look at the PR I created (linked above) and let me know what you think. If it looks good, I'll merge to v2.9 and release a second beta soon.

abbeycode added a commit that referenced this pull request Feb 9, 2017

Excluded this branch from Travis, since it's only meant as a demo for…

b83ad8f

… a pull request's tests (PR #38)

abbeycode added a commit that referenced this pull request Feb 9, 2017

Added tests, headers, and stubs for multi-volume archives (PR #38)

2230b22

aonez changed the base branch from master to multivolume August 17, 2017 09:37

aonez and others added 8 commits September 29, 2017 09:32

Was at master, merging branch…

61ac50b

Implementing isVolume methods

b3b05ad

isVolume method removed

53311f2

Removing extra space

614f35f

Rebase to @abbeycode multivolume branch

0f89b94

Fillet multivolume methods

e83d7af

Using new error method

7043e0d

Using NSMutableOrderedSet for sorted array

8364d76

aonez commented Sep 29, 2017

View reviewed changes

aone added 2 commits September 29, 2017 11:58

Fixed bad travis config

8f4992e

Added newline break

e5d4743

abbeycode requested changes Sep 29, 2017

View reviewed changes

aone added 2 commits September 29, 2017 16:15

Removed unnecessary parenthesis

41a6107

Checking first part on archive init

8231542

aonez commented Sep 29, 2017

View reviewed changes

Classes/URKArchive.mm

return false;

}

return volumeURLs.count > 1;

Copy link

Contributor Author

aonez Sep 29, 2017

Choose a reason for hiding this comment

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

No extra parenthesis 👍

aonez commented Sep 29, 2017

View reviewed changes

abbeycode reviewed Sep 29, 2017

View reviewed changes

abbeycode merged commit d129c73 into abbeycode:multivolume Oct 2, 2017

abbeycode mentioned this pull request Oct 3, 2017

Add enhanced support for multi-volume archives #59

Merged

abbeycode mentioned this pull request Oct 7, 2017

Add methods to check data integrity #61

Merged

Conversation

aonez commented Jan 9, 2017

Uh oh!

aonez commented Jan 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aonez commented Jan 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abbeycode commented Jan 11, 2017

Uh oh!

abbeycode commented Jan 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aonez commented Jan 15, 2017

Uh oh!

aonez commented Jan 15, 2017

Uh oh!

aonez commented Jan 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abbeycode commented Feb 4, 2017

Uh oh!

aonez commented Feb 5, 2017

Uh oh!

aonez commented Feb 5, 2017

Uh oh!

abbeycode commented Feb 5, 2017

Uh oh!

aonez commented Feb 5, 2017

Uh oh!

abbeycode commented Feb 5, 2017

Uh oh!

aonez commented Feb 5, 2017 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aonez commented Feb 5, 2017

Uh oh!

abbeycode commented Feb 9, 2017

Uh oh!

aonez commented Feb 10, 2017

Uh oh!

aonez commented Feb 26, 2017

Uh oh!

abbeycode commented Aug 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aonez commented Aug 17, 2017

Uh oh!

aonez commented Aug 17, 2017

Uh oh!

abbeycode commented Aug 17, 2017

Uh oh!

abbeycode commented Aug 17, 2017

Uh oh!

aonez commented Aug 17, 2017

Uh oh!

abbeycode commented Aug 18, 2017

Uh oh!

aonez commented Aug 18, 2017 via email

Uh oh!

aonez commented Aug 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aonez left a comment

Choose a reason for hiding this comment

Uh oh!

abbeycode left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aonez Sep 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aonez commented Jan 9, 2017 •

edited

Loading

aonez commented Jan 9, 2017 •

edited

Loading

abbeycode commented Jan 13, 2017 •

edited

Loading

aonez commented Jan 15, 2017 •

edited

Loading

aonez commented Feb 5, 2017 via email •

edited

Loading

abbeycode commented Aug 16, 2017 •

edited

Loading

aonez commented Aug 18, 2017 •

edited

Loading

aonez Sep 29, 2017 •

edited

Loading

aonez Sep 29, 2017 •

edited

Loading

aonez Sep 29, 2017 •

edited

Loading

abbeycode Sep 29, 2017 •

edited

Loading

aonez commented Sep 29, 2017 •

edited

Loading