Re: [PATCH]: Was [PROPOSAL] Takeover Take 2

rooneg@gmail.com wrote on 06/22/2006 10:49:26 AM:

> On 6/22/06, Paul Burba <paulb@softlanding.com> wrote:
> > Hi All,
> >
> > I'm reposting this patch in the hope someone might be able to take a
look.
> > It is a fairly complex change compared to what I've done before (at
least
> > outside of the OS400/EBCDIC port) so I'd like to have another set of
eyes
> > look at it.

Hi Garrett,

Thanks for taking a look.

> I just went through the diff, and it looks fine to me... I also built
> it and tried it out, and some of the new checkout tests are failing
> (number 2 and 7). I believe it's because I'm building in maintainer
> mode and the line numbers for errors are screwing up the regexes
> looking for the right error message.

Fixed that.

> Also, those tests print random stuff out to stdout when we have
> problems, we don't usually do that.

By "have problems" do you mean printing a message and raising
svntest.Failure?

e.g.:

  # Checkout the standard greek repos into a directory that has a dir
named
  # "iota" obstructing the file "iota" in the repos. This should fail.
  sout, serr = svntest.actions.run_and_verify_svn("Expected error during
co",
                                                  None, SVNAnyOutput,
"co",
                                                  "--force",
sbox.repo_url,
                                                  other_wc)

  expected_error = "svn: Failed to add file.*a non-file object of the " +
\
                   "same name already exists"

  if not re.compile(expected_error).search(serr[0]):
    print "forced co failed but not in the expected way"
    raise svntest.Failure

Ok, assuming I'm understanding you correctly(?) I looked at all the
instances of "raise svntest.Failure" and found we actually do do one of
the following some frequency:

  A) print 'some explanation of the error'
     raise svntest.Failure

  B) raise svntest.Failure('some explanation of the error')

Regardless of what's been done, where is the harm in providing a bit more
information to someone reading the log? Perhaps this doesn't matter in
most circumstances since the tests usually pass and I doubt people spend
much time pouring over the logs. But in my own experience porting
Subversion to OS/400 I've read more logs than I care to remember; and
tests that raised failures with no explanation made finding the underlying
problem more difficult than necessary.

Anyhow, I'm not married to the additional message, but I'd prefer it if
there are no strong objections.

FWIW I changed my patch to use approach B rather than A. I didn't realize
svntest.Failure supported the message.

Paul B.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Jun 23 19:24:32 2006