Raise Error When the compression_level is invalid

[YafahEdelman]

Wouldn't this be better than forcing to a specific compression level?
Note: I just saw this while glancing through the code, so I may very well be wrong.

[lelutin]

Hi there, thanks for submitting a change. It's really appreciated. I have some comments on the code, and then some pointers of how to submit further contributions.

I think you're making a valid point here: if git.py's _encode_packobj() gets (via the PackWriter constructor) a value that's not an int in the right scope, then it's a programming error.

In bup we usually don't use BaseException for such cases though. that would probably be better tested with an assert clause: if someone gets an error there, then the programmers screwed up somewhere and the code should be fixed.

Now this does indeed bring up some issues in the current code using PackWriter. cmd/save-cmd.py and cmd/split-cmd.py both pass on an unverified value to PackWriter's compression_level argument. This means that you can do stuff like this (and get an undefined compression level):

$ bup save -n blah --compression=foo /
$ tar c some/path | bup split --compression=bar

So I would suggest you change your patch to be:

• use assert instead of raising BaseException
• fix bup-save and bup-split by making them ensure that opt.compress is an integer between 0 and 9 (and call o.fatal() otherwise)

Last but not least: bup usually expects contributions to be sent to the mailing list instead of as pull requests. And also, to signify that you accept to license your code to GPLv2 (bup's license), you should use a line that starts with "Signed-Off-By:".

The latter can be obtained by using "-s" when committing code with git.

The former can be achieved with "git send-email", and you can also use "git format-patch" to dump patches to files before sending them by email in order to review them. You can find some information about this in https://github.com/bup/bup/blob/master/HACKING . Also note that you can send messages to the list without being subscribed -- it is this list's etiquette to always use "reply-all" so that non-members can receive responses to their submissions.

[ghost]

Hi @lelutin, I saw you recently posted #13 (comment). Considering this change is less than 15 lines wouldn't it also fit the exception?

[lelutin]

@r04r if it's that shor, I guess so yes. but keep in mind this kind of exception is usually "additive" so it's 15 lines overall for a person.

Also, since the patch needed to be modified I took the liberty of explaining the process we use in bup.

[rlbdv]

Thanks for the help. In accordance with ./HACKING I've redirected this pull request to the list for discussion: https://groups.google.com/d/msg/bup-list/8Cydu84L2LE/blc-wyphAAAJ

[rlbdv]

@lelutin pushed to master as 5c97cd7, and we'll handle additional changes afterward.