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_COPY_at_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
Received on 2009-12-22 13:17:40 CET