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

Missing documentation: archive_entry_update_pathname_utf8() returns 1 on success #2085

Open
haferburg opened this issue Mar 7, 2024 · 0 comments

Comments

@haferburg
Copy link

haferburg commented Mar 7, 2024

As of 3.7.2, archive_entry_update_pathname_utf8() is undocumented. Please add documentation.

I'm not actually sure how it's supposed to work.

Here's the code

int
archive_entry_update_pathname_utf8(struct archive_entry *entry, const char *name)
{
	if (archive_mstring_update_utf8(entry->archive,
	    &entry->ae_pathname, name) == 0)
		return (1);
	if (errno == ENOMEM)
		__archive_errx(1, "No memory");
	return (0);
}

and

int
archive_mstring_update_utf8(struct archive *a, struct archive_mstring *aes,
    const char *utf8)
{
	struct archive_string_conv *sc;
	int r;

	if (utf8 == NULL) {
		aes->aes_set = 0;
		return (0); /* Succeeded in clearing everything. */
	}
//  ------8<-*snip*------8<-*snip*------8<-*snip*------8<-*snip*------8<-*snip*-----
	/* All conversions succeeded. */
	return (0);
}

So if archive_mstring_update_utf8() is successful, it returns 0, and this makes archive_entry_update_pathname_utf8() return 1. Which is quite unexpected. I thought 1 means EOF and 0 means OK. The intention would be clearer if the code would use a define like ARCHIVE_OK instead of a literal int.

And then there's this code in the tests:

	/* Skip test if archive_entry_update_pathname_utf8() is broken. */
	/* In particular, this is currently broken on Win32 because
	 * setlocale() does not set the default encoding for CP_ACP. */
	entry = archive_entry_new();
	if (archive_entry_update_pathname_utf8(entry, badname_utf8)) {
		archive_entry_free(entry);
		skipping("Cannot test conversion failures.");
		return;
	}
	archive_entry_free(entry);

Here a non-zero return value is apparently interpreted as a conversion failure. This seems wrong.

Also, the comment in that latter snippet indicates a limitation that should be mentioned in the documentation as well, right? Is this what causes #1264 or #358?

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

No branches or pull requests

1 participant