[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: Problem with svn log for unrelated revisions

From: Paul Burba <ptburba_at_gmail.com>
Date: Fri, 17 Jun 2011 13:04:57 -0400

On Fri, Jun 17, 2011 at 12:34 PM, Dirk Thomas <websvn_at_dirk-thomas.net> wrote:
>> That does appear to be a bug.  I don't see a similar issue in the
>> tracker; would you mind filing a new issue?  No worries if you cannot,
>> I can do it at some point today.
> i have created issue #3931.

Hi Dirk,

Thanks for adding the issue.

>> I'll commit your patch once we have an issue.
> Prepend:
> @Issue(3931)
> before "def log_unrelated(sbox):"
>> Note that this is not a regression from 1.6.17 so it
>> probably may not be fixed for 1.7.0 (I'm not sure if this is hindering
>> your work on issue #3830).
> But may be it is possible that someone takes a closer look before in order
> to decide if it easily fixable or not.

Agreed, I just wanted to mention it was not a regression. We are
pretty focused on 1.7.0 right now, so most of the focus is on blockers
for that.

>>> +  target = os.path.join(sbox.repo_url, 'A', 'D', 'G', 'rho') + "@2"
>> Don't construct URLs with os.path.join, it uses os.sep to join the
>> paths, which on windows is '\', so you won't end up with a valid URL.
> This is done using os.path.join in the whole file - then all other test
> functions should be updated accordingly?

You are likely looking at the creation of *working copy* targets, your
patch is creating a *URL* target. We use os.path.join when creating
WC paths in the test to maintain cross-platform compatibility (Windows
with its crazy '\' path separators :-). If you replaced
"sbox.repo_url" with "sbox.wc_dir" your patch would be fine. Or my
suggestion would work too. Doesn't matter much either way.

>> A few comments explaining where/why the test fails is always helpful.
>> Sure, we'll have an associated issue and/or dev threads to check, but
>> a quick bit like this never hurts:
>>> +    # Currently this test fails because instead of returning the
>>> expected
>>> +    # 'Unable to find repository location for ^/A/D/G/rho in revision 9'
>>> +    # error, the log for ^/A/D/G/rho_at_8 is returned, but that is an
>>> unrelated
>>   >  +   # line of history.
>> This is especially helpful in more complex tests which start failing
>> for other reasons.  When someone comes along months (or even years)
>> later to work on an issue it's really nice to know if you have new
>> problems to tackle.
> Of course, are you going to add the comments before submitting the patch or
> do you want be to provide an updated diff?

I'll add them, no need for a new patch.

>>> +  file_not_found = ".*File not found.*"
>>> +
>>> +  exit_code, output, err = svntest.actions.run_and_verify_svn(
>>> +    None, None, file_not_found,
>>> +    'log', '-r', '6:7', target)
>>> +  exit_code, output, err = svntest.actions.run_and_verify_svn(
>>> +    None, None, file_not_found,
>>> +    'log', '-r', '7:6', target)
>> This is personal preference, but since this last block works maybe
>> move it ahead of the failing block?
> Ok, actually this last block testing for "File not found" is not really
> needed.
> It mainly exists to confirm that patch for 3830 has no regressions.
> May be it would be better to move this test to a separate test-function (as
> it is unrelated to the name of this test: "log_unrelated")

No need for a separate test I think. We try to walk a fine line
between "too many small tests" and "too few big tests that do too
much". I think we are fine here.

Received on 2011-06-17 19:05:28 CEST

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.