On Tue, Mar 17, 2026 at 2:09 AM Branko Čibej <[email protected]> wrote:
> On 16. 3. 26 21:23, Timofei Zhakov wrote:
>
> I'm currently revising the utf8-cmdline branch, and I had to make a
> change to the function that was used to check a file for existence.
> It's backed by a function that is called apr_stat in APR.
>
> Subversion has a wrapper around it under svn_io_stat name. It adds
> conversion from UTF8 to native encoding and svn_error error handling.
>
> Here is everything it basically does:
>
> [[[
> svn_error_t *
> svn_io_stat(apr_finfo_t *finfo, const char *fname,
> apr_int32_t wanted, apr_pool_t *pool)
> {
> apr_status_t status;
> const char *fname_apr;
>
> /* APR doesn't like "" directories */
> if (fname[0] == '\0')
> fname = ".";
>
> SVN_ERR(cstring_from_utf8(&fname_apr, fname, pool));
>
> /* Quoting APR: On NT this request is incredibly expensive, but accurate. */
> wanted &= ~SVN__APR_FINFO_MASK_OUT;
>
> status = apr_stat(finfo, fname_apr, wanted, pool);
> if (status)
> return svn_error_wrap_apr(status, _("Can't stat '%s'"),
> svn_dirent_local_style(fname, pool));
>
> return SVN_NO_ERROR;
> }
> ]]]
>
> And here is the change made in utf8-cmdline (taken from a diff after
> running merge into trunk):
>
> [[[
> @@ -3179,23 +3151,33 @@ sub_main(int *exit_code,
> if (opt_state.message)
> {
> apr_finfo_t finfo;
> - if (apr_stat(&finfo, opt_state.message /* not converted to UTF-8
> */,
> - APR_FINFO_MIN, pool) == APR_SUCCESS)
> +
> + /* We don't want to warn for '' */
> + if (opt_state.message[0] != '\0')
> {
> - if (subcommand->cmd_func != svn_cl__lock)
> + err = svn_io_stat(&finfo, opt_state.message,
> + APR_FINFO_MIN, pool);
> +
> + if (!err)
> {
> - return svn_error_create
> - (SVN_ERR_CL_LOG_MESSAGE_IS_PATHNAME, NULL,
> - _("The log message is a pathname "
> - "(was -F intended?); use '--force-log' to override"));
> + if (subcommand->cmd_func != svn_cl__lock)
> + {
> + return svn_error_create(
> + SVN_ERR_CL_LOG_MESSAGE_IS_PATHNAME, NULL,
> + _("The log message is a pathname "
> + "(was -F intended?); use '--force-log' to "
> + "override"));
> + }
> + else
> + {
> + return svn_error_create(
> + SVN_ERR_CL_LOG_MESSAGE_IS_PATHNAME, NULL,
> + _("The lock comment is a pathname "
> + "(was -F intended?); use '--force-log' to "
> + "override"));
> + }
> }
> - else
> - {
> - return svn_error_create
> - (SVN_ERR_CL_LOG_MESSAGE_IS_PATHNAME, NULL,
> - _("The lock comment is a pathname "
> - "(was -F intended?); use '--force-log' to override"));
> - }
> + svn_error_clear(err);
> }
> }
> }
> ]]]
>
> Assuming there are no silly logical mistakes, it does not change any
> existing behaviour.
>
>
> I don't quite understand. Are you saying that opt_state.message is now
> encoded in UTF-8? Because if it isn't, that svn_io_stat() can't possibly
> work.
>
> Also I don't see how this:
>
> + /* We don't want to warn for '' */
> + if (opt_state.message[0] != '\0')
>
>
> could be considered not changing behaviour. It could be fixing wrong
> behaviour, but that's certainly changing it.
>
>
> I'm considering committing it into trunk
> separately before the rest of changes made in that branch. The only
> difference is that as at the current state of trunk, all arguments are
> handled in a native encoding, hence should be converted using the
> svn_path_cstring_from_utf8 function. This is not shown in the patch
> above.
>
> If anyone has any concerns about that, please speak up. I remember
> there was a discussion that has not been entirely resolved.
>
>
>
What I suggest is to effectively convert opt_state.message into UTF8 and
back to the native encoding as the svn_io_stat is called. Doing that would
require a local variable because we don't want to modify opt_state.
In trunk it's in native encoding. In the utf8-cmdline branch it's in UTF8.
> Also I don't see how this:
>
> + /* We don't want to warn for '' */
> + if (opt_state.message[0] != '\0')
This one is weird. It handles the case of let's say [svn commit -m ""]
which is a completely normal scenario. It's just a commit with an empty log
message. The code before relied on the fact that APR's apr_stat treats an
empty string as an illegal path. Which caused a non-APR_SUCCESS return code.
svn_io_stat on the other hand, checks for that and converts empty paths to
'.':
[[[
/* APR doesn't like "" directories */
if (fname[0] == '\0')
fname = ".";
]]]
--
Timofei Zhakov
