On Mon, 2009-12-21, Paul Burba wrote: > This is replicable from the command line when the svn mergeinfo target > is a working copy path pegged at HEAD or DATE, e.g.: > > C:\SVN\src-trunk-2\Release\subversion\tests\cmdline\svn-test-work\working_copies\mergeinfo_tests-8>svn > mergeinfo --show-revs eligible A a_c...@head > svn: Bogus revision information given > > I fixed this in r892902 and also expanded mergeinfo_tests.py 8 so it > also covers this bug.
> ------------------------------------------------------------------------ > r892902 | pburba | 2009-12-21 17:45:34 +0000 (Mon, 21 Dec 2009) | 17 lines > Changed paths: > M /subversion/trunk/subversion/libsvn_client/url.c > M /subversion/trunk/subversion/tests/cmdline/mergeinfo_tests.py > > Fix 'svn mergeinfo' bug when target is a WC path pegged at HEAD or DATE. > > This fixes the failing JavaHL test BasicTests.java:testBasicMerge. > See http://svn.haxx.se/dev/archive-2009-12/0346.shtml. > > * subversion/libsvn_client/url.c > (svn_client__derive_location): Follow-up to r886880; contact the server if > asking about a working copy path pegged at DATE or HEAD. Previously we > tried to resolve the peg rev in this case with a call to > svn_client__entry_location(), but since r886880 that function has > returned an error if asked about revisions only the repos knows about. > > * subversion/tests/cmdline/mergeinfo_tests.py > (mergeinfo_on_pegged_wc_path): Fix typo, this test should have tested > WC paths pegged at HEAD, but was instead twice testing WC paths pegged > at BASE. Also add --show-revs=eligible variants of the tests. > > ------------------------------------------------------------------------ Thank you for identifying and fixing that bug. I looked at the function svn_client__entry_location() which is involved in this logic, and it is totally crazy. I can't fathom its purpose. What is this function for? I think it's time to forget about what it was originally for, and to examine what we want it for now. I looked briefly at the six callers, and I couldn't see easily what all of them expected from this function, so some more investigation is needed. Your log message says "since r886880 that function has returned an error if asked about revisions only the repos knows about." However, the function doesn't mention that in its doc string, and it still claims to handle requests for revision "previous" which only the repos knows about. Keep in mind that a node's path (URL) exists in a particular revision and might not exist in another revision. [[[ > /* Get the repository URL and revision number for LOCAL_ABSPATH and put them > in *URL and *REVNUM. REVNUM may be null, in which case it is ignored. Get the URL and revnum of what version of the node? LOCAL_ABSPATH identifies a "working" node, and thus a line of history; we can identify locations along this line of history at various revisions, some of which could be specified by the PEG_REV_KIND parameter. I think the word "peg" is getting in the way again. A "peg revision" is a revision that goes along with a URL in order to identify a line of history along which other revisions can be found. In the context of this function, the line of history can be identified solely by LOCAL_ABSPATH, and so the revision we want to look up is not a peg but an operative revision. (We might not want to use the word "operative" either, reserving it just for high-level operations. If, in the context of the caller, the revision determined here is in turn going to act as a peg for a subsequent look-up, that is none of our business here.) So delete the word "peg". > If PEG_REV_KIND is svn_opt_revision_working, then use the LOCAL_ABSPATH's > copyfrom info to populate *URL and *REVNUM. What if it isn't a copy/move? > If PEG_REV_KIND is svn_opt_revision_date or svn_opt_revision_head then > return SVN_ERR_CLIENT_BAD_REVISION. > > If PEG_REV_KIND is svn_opt_revision_committed or svn_opt_revision_previous > then set *REVNUM to the last committed or previous revision respectively. And what about *URL? We can work out the (or a) previous revision number from local WC info, but we can't work out the previous URL, so I don't think this is a sensible API if we want to avoid contacting the repository. > If PEG_REV_NUM is svn_opt_revision_unspecified, svn_opt_revision_number, > svn_opt_revision_base, or svn_opt_revision_working then set *REVNUM > to the base revision. */ The param is called PEG_REV_KIND. There's no point in defining such a meaning (or any meaning) to the kind "_unspecified". It's silly to map "_number" to mean "_base". It's sensible that "_base" means "_base". It's a typo/error that "_working" is mentioned again here. > svn_error_t * > svn_client__entry_location(const char **url, > svn_revnum_t *revnum, > svn_wc_context_t *wc_ctx, > const char *local_abspath, > enum svn_opt_revision_kind peg_rev_kind, > apr_pool_t *result_pool, > apr_pool_t *scratch_pool); ]]] Note that it may be good to split up this API into multiple functions, if we don't need the ability for the revision kind to be passed in as a parameter. - Julian
