On Fri, 2009-11-27 at 18:45 +0000, Julian Foad wrote: > 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.
(BTW: When I wrote "... at least for debugging ..." in my original message, what I meant was we might not want to put that assertion in the repository yet, until we fix the bugs that it finds, because if we do then all the tests break.) > > 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". (BTW: THe variable was originally called just "source", and was changed to "source_abspath" when the get_absolute() call was added.) > - Julian > > > > If the above fix seems fine, may I send it as a formal patch? > > >
