Paul Burba wrote:
> Julian & Philip,
>
> Attached is a new log and patch that incorporates your respective
> recommendations. When faced with something too awkward to explain in
> words...seek refuge in a table. Which is what I did in the implementation
> comments. It's still a rather lengthy comment, but I think this is
> unavoidable.
OK, I'm happy with that. It would be nice if the comment were shorter but I'd
rather have too much than too little detail, so that anybody changing the
generic part of the function has a chance of understanding how their change
will affect OS400, especially as most of us don't know anything about OS400 or
that version of APR.
(Out of curiosity, are we able to see IBM's version of APR, where this
behaviour might be documented, or are they keeping it private?)
The logic and layout of this looks good. I've only style nits to pick and
they're unimportant.
> [[[
> OS400/EBCDIC Port: Create new files with CCSID 1208.
>
> This is one of several patches to allow Subversion to run on IBM's
> OS400 V5R4. It tags files created by Subversion with the CCSID 1208
> (UTF-8), which is always correct for text files and irrelevant for
> binary files, and allows OS400 applications and utilities to operate
> correctly on these files.
>
> * subversion/libsvn_subr/io.c
> (file_open): new function.
> (svn_io_open_unique_file2, svn_io_file_open): Replace calls to
> apr_file_open with new function.
> ]]]
Nice.
> + if (flag & APR_CREATE)
> + {
> + /* If we are trying to create a file on OS400 ensure it's CCSID is
> + * 1208. */
> + apr_err = apr_file_open(f, fname, flag & ~APR_BINARY, perm, pool);
> +
> + if (!apr_err)
> + apr_file_close(*f);
> + else
> + return apr_err;
No big deal, and no need to change it, but I'd probably write that "if ... else
..." as:
if (apr_err)
return apr_err;
apr_file_close(...
> +
> + /* Unset APR_EXCL before the next call to apr_file_open() doesn't
"before" -> "so" ?
> + * return an error. */
> + flag &= ~APR_EXCL;
> + }
> +#endif /* AS400 */
> + return apr_file_open(f, fname, flag, perm, pool);
> +}
> +
> +
> svn_error_t *
> svn_io_check_resolved_path(const char *path,
> svn_node_kind_t *kind,
> @@ -264,7 +351,7 @@
> SVN_ERR(svn_path_cstring_from_utf8(&unique_name_apr, unique_name,
> pool));
>
> - apr_err = apr_file_open(&file, unique_name_apr, flag | APR_BINARY,
> + apr_err = file_open(&file, unique_name_apr, flag | APR_BINARY,
> APR_OS_DEFAULT, pool);
Re-indent the continuation line, please.
>
> if (APR_STATUS_IS_EEXIST(apr_err))
> @@ -2237,7 +2324,7 @@
> apr_status_t status;
>
> SVN_ERR(svn_path_cstring_from_utf8(&fname_apr, fname, pool));
> - status = apr_file_open(new_file, fname_apr, flag | APR_BINARY, perm, pool);
> + status = file_open(new_file, fname_apr, flag | APR_BINARY, perm, pool);
>
> if (status)
> return svn_error_wrap_apr(status, _("Can't open file '%s'"),
+1 to commit this. Thanks for making it as beautiful as it can be under the
circumstances.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Feb 23 22:42:24 2006