Stop panicking when CString conversion fails

[DanielEScherzer]

I started this at #1295 and #1296 and then realized that there are a lot more places that should be adjusted, so filing to discuss first.

CheckoutBuilder::path(), ancestor_label(), our_label(), their_label()
TreeUpdateBuilder::remove(), upsert()
DescribeFormatOptions::dirty_suffix()
DescribeOptions::pattern()
DiffOptions::old_prefix(), new_prefix(), pathspec()
EmailCreateOptions::subject_prefix()
ObjectType::from_str()
MergeFileOptions::ancestor_label(), our_label(), their_label()
MergeFileInput::path()
ProxyOptions::url()
RebaseOptions::rewrite_notes_ref()
Refspec::dst_matches(), src_matches(), transform(), rtransform()
FetchOptions::custom_headers()
PushOptions::custom_headers(), remote_push_options()
RepositoryInitOptions::workdir_path(), description(), template_path(), initial_head(), origin_url()
StatusOptions::pathspec()
Transaction::lock_ref(), set_target(), set_symbolic_target(), set_reflog(), remove()
Tree::get_name_bytes()

[DanielEScherzer]

Should I be adding regression tests in one commit and switching to stop panicking in a separate commit, or would those tests not really be useful? I added tests in 1295 and 1296 but if there are this many places to update I realized that the tests might not really be useful

[DanielEScherzer]

Also crosslinking #1294, #1290, #1229

[weihanglo]

Should I be adding regression tests in one commit and switching to stop panicking in a separate commit, or would those tests not really be useful? I added tests in 1295 and 1296 but if there are this many places to update I realized that the tests might not really be useful

Slightly prefer having tests, though fine with either way.

[weihanglo]

See #1295 (comment).

[weihanglo]

Arguments

From #1295 (comment)

So I just started working on writing an extension for PHP in Rust that integrates with the git2 rust library for the logic - and that is what I was thinking of when I wanted to reduce the use of panics

Basically, returning a result (from Rust) means that we can throw an exception (in PHP) without needing to do the validation manually

From #1295 (comment)

PHP exceptions can be caught by the userland code

https://github.com/libgit2/php-git is very much outdated (and I'm doing this to be able to present a workshop on writing extensions in Rust, not just for fun)

[weihanglo]

For those panic changes. It is still more an ergonomic issue to me, and I don't see a real world scenario that application gets the bad input and need to filter it out.
Panics is allowed also in other well-known crates and std, though we would need to categorize why they requires a panic rather than Result.

For example,

• development errors -> likely panic
• catastrophic failures, like OOM -> panic (though in std we have also try_*)
• runtime errors -> likely error

From my point of view, these may be development errors, though we need to add a ## Panic in the doc comment to make it so. I personally don't want to trade ergonomic with panic-free because of this rare case (OSes inherently don't allow /0 in paths). The returning Error is also more confusing to Rust users, as there are too many different error kinds.

[DanielEScherzer]

How about for the functions that already return Result enums with an error - then switching to returning an error instead of panicking shouldn't change the ergonomics as much, since the return type doesn't change

[weihanglo]

For those, sure!

[weihanglo]

Though that introduces consistency (which should be fine if we document those panics?)

[DanielEScherzer]

Okay, I started with #1308 for things that already return Result, and also included things returning Option or bool