On Tue, Mar 17, 2026 at 8:52 AM Timofei Zhakov <[email protected]> wrote:
> 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 = "."; > ]]] > I went ahead and committed this change in r1932394. Thanks for constructive review! -- Timofei Zhakov
