Fix annotations on ostree_mutable_tree_lookup()

[jameswestman]

(nullable) and (optional) were missing on lookup()'s out parameters, which caused the rust bindings for the function to not work. Due to the missing (nullable), it would return a Result<(GString, MutableTree), _>, not a Result<(Option<GString>, Option<MutableTree>), _>, which led to panics.

[openshift-ci]

Hi @jameswestman. Thanks for your PR.

I'm waiting for a ostreedev member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[lucab]

/approve
/ok-to-test

[lucab]

Thanks for the PR! Could you maybe share a reproducer or a backtrace from the panics you are seeing?

While you reasoning seems right to me, looking into the logic of ostree_mutable_tree_lookup() I think that in the non-error cases it can only return a properly populated (GString, MutableTree) pair 🤔.
Perhaps you are observing a panic coming from some other root cause?

[jameswestman]

Based on the if statement on line 423, it seems that the function returns one or the other, but never both.

Here is the backtrace:

thread '<unnamed>' panicked at 'assertion failed: !ptr.is_null()', /home/jwestman/.cargo/registry/src/github.com-1ecc6299db9ec823/glib-0.14.8/src/gstring.rs:42:9
stack backtrace:
   0: rust_begin_unwind
             at /rustc/acbe4443cc4c9695c0b74a7b64b60333c990a400/library/std/src/panicking.rs:498:5
   1: core::panicking::panic_fmt
             at /rustc/acbe4443cc4c9695c0b74a7b64b60333c990a400/library/core/src/panicking.rs:107:14
   2: core::panicking::panic
             at /rustc/acbe4443cc4c9695c0b74a7b64b60333c990a400/library/core/src/panicking.rs:48:5
   3: glib::gstring::GString::new
             at /home/jwestman/.cargo/registry/src/github.com-1ecc6299db9ec823/glib-0.14.8/src/gstring.rs:42:9
   4: <glib::gstring::GString as glib::translate::FromGlibPtrFull<*mut i8>>::from_glib_full
             at /home/jwestman/.cargo/registry/src/github.com-1ecc6299db9ec823/glib-0.14.8/src/gstring.rs:322:9
   5: glib::translate::from_glib_full
             at /home/jwestman/.cargo/registry/src/github.com-1ecc6299db9ec823/glib-0.14.8/src/translate.rs:1376:5
   6: ostree::auto::mutable_tree::MutableTree::lookup
             at /home/jwestman/.cargo/registry/src/github.com-1ecc6299db9ec823/ostree-0.13.7/src/auto/mutable_tree.rs:126:38
   7: flatmanager::jobs::CommitJobInstance::mtree_lookup
             at ./src/jobs.rs:518:17
   8: flatmanager::jobs::CommitJobInstance::use_storefront_info
             at ./src/jobs.rs:568:38
   9: flatmanager::jobs::CommitJobInstance::do_commit_build_refs
             at ./src/jobs.rs:430:35
  10: <flatmanager::jobs::CommitJobInstance as flatmanager::jobs::JobInstance>::handle_job
             at ./src/jobs.rs:644:19
  11: flatmanager::jobs::process_one_job
             at ./src/jobs.rs:1237:51
  12: <flatmanager::jobs::JobExecutor as actix::handler::Handler<flatmanager::jobs::ProcessOneJob>>::handle
             at ./src/jobs.rs:1299:12
  13: <actix::sync::SyncContextEnvelope<A,M> as actix::address::envelope::EnvelopeProxy>::handle
             at /home/jwestman/.cargo/registry/src/github.com-1ecc6299db9ec823/actix-0.8.3/src/sync.rs:353:13
  14: <actix::address::envelope::Envelope<A> as actix::address::envelope::EnvelopeProxy>::handle
             at /home/jwestman/.cargo/registry/src/github.com-1ecc6299db9ec823/actix-0.8.3/src/address/envelope.rs:71:9
  15: actix::sync::SyncContext<A>::run
             at /home/jwestman/.cargo/registry/src/github.com-1ecc6299db9ec823/actix-0.8.3/src/sync.rs:251:21
  16: actix::sync::SyncArbiter<A>::start::{{closure}}
             at /home/jwestman/.cargo/registry/src/github.com-1ecc6299db9ec823/actix-0.8.3/src/sync.rs:122:17
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

[lucab]

Based on the if statement on line 423, it seems that the function returns one or the other, but never both.

You are right, I now see what you mean, I misread the logic.
For reference, this is the code we are talking about:

ostree/src/libostree/ostree-mutable-tree.c

Lines 402 to 435 in 00e39eb

	/**
	* ostree_mutable_tree_lookup:
	* @self: Tree
	* @name: name
	* @out_file_checksum: (out) (transfer full): checksum
	* @out_subdir: (out) (transfer full): subdirectory
	* @error: a #GError
	*/
	gboolean
	ostree_mutable_tree_lookup (OstreeMutableTree *self,
	const char *name,
	char **out_file_checksum,
	OstreeMutableTree **out_subdir,
	GError **error)
	{
	if (!_ostree_mutable_tree_make_whole (self, NULL, error))
	return FALSE;
	g_autofree char *ret_file_checksum = NULL;
	g_autoptr(OstreeMutableTree) ret_subdir =
	ot_gobject_refz (g_hash_table_lookup (self->subdirs, name));
	if (!ret_subdir)
	{
	ret_file_checksum = g_strdup (g_hash_table_lookup (self->files, name));
	if (!ret_file_checksum)
	return set_error_noent (error, name);
	}
	if (out_file_checksum)
	*out_file_checksum = g_steal_pointer (&ret_file_checksum);
	if (out_subdir)
	*out_subdir = g_steal_pointer (&ret_subdir);
	return TRUE;
	}

Thanks for the patch, LGTM!

[cgwalters]

So one thing we can suddenly start doing is actually updating the Rust bindings in the same PR that changes the annotations...but it's fine not to do it, I'll look at a followup PR.