On Fri, 2009-11-27 at 23:25 +0530, Kannan wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA1 > > Julian Foad wrote: > >> I spotted an error in the conversion to use abs-paths: > >> > >> [[[ > >> static svn_error_t * > >> normalize_merge_sources(apr_array_header_t **merge_sources_p, > > [...] > >> SVN_ERR(svn_dirent_get_absolute(&source_abspath, source, pool)); > >> ]]] > >> > >> In that last line shown, 'source' can be (and often is) a URL so this is > >> wrong. > > I've already reported this case here(though proposed as a patch): > http://subversion.tigris.org/ds/viewMessage.do?dsMessageId=2414438&dsForumId=462
Oh yes, so you did. However, that patch you initially proposed there passed a part of the URL to svn_dirent_get_absolute(), which is not right either. > > I suggest adding > > > > SVN_ERR_ASSERT(!svn_path_is_url(relative)); > > > > inside svn_dirent_get_absolute(), at least for debugging, as we should > > never be calling it on any path that looks like a URL. When I tried > > doing that, almost all tests failed, presumably because some universal > > operation is calling it on a URL. > > In cases of handling both URLs and wc paths, there's a check made on > the type of the path based on which the desired dirent/URI method is > invoked right(except for the rare case above)? Yes: the case above is a bug, and in all non-buggy cases we only call svn_dirent_get_absolute() on a dirent. > Indeed the 'assert' > suggested above is useful for debugging, but if we're to check the > type of the path before making a call to this method, which confirms > it's not a URL then, won't the 'assert' here become obsolete? "Assertions are forever." (A quote from the book "Writing Solid Code".) The purpose of assertions is to catch programming errors (bugs). They don't become obsolete when we have eliminated all known bugs, they stay there to catch any future bugs or any bugs that haven't so far shown up. > Also for the above case, may I suggest the following change? > [[[ > if(!svn_path_is_url(source) > SVN_ERR(svn_dirent_get_absolute(&source_abspath, source, pool)); > else > source_abspath = source; > ]]] It is very misleading to put a URL into a variable called "source_abspath". If the variable needs to serve a dual purpose, please rename it to something like "source_abspath_or_url". - Julian > If the above fix seems fine, may I send it as a formal patch?
