Re: svn commit: r10091 - in trunk/subversion: include libsvn_repos svnadmin

[kf...@collab.net]

Ben Collins-Sussman <su...@collab.net> writes:
> > Make svnadmin load less verbose by default and add a --verbose flag.
> 
> Did we forget to run 'make check'?  This breaks all the
> svnadmin_tests.py tests.

In fact it breaks *all* the .py tests, because of this code in
subversion/tests/clients/cmdline/svntest/main.py:copy_repos():

  load_re = re.compile(r'^------- Committed revision (\d+) >>>$')
  expect_revision = 1
  for load_line in load_lines:
    match = load_re.match(load_line)
    if match:
      if match.group(1) != str(expect_revision):
        print 'ERROR:  load failed:', load_line,
        raise SVNRepositoryCopyFailure
      expect_revision += 1
  if expect_revision != head_revision + 1:
    print 'ERROR:  load failed; did not see revision', head_revision
    raise SVNRepositoryCopyFailure

For now, I'll fix it by adding '--verbose' to the invocation in the
test framework, so we don't lose any more developer time on this.

However, in the long term, I feel it's wrong that we changed the
default output format of 'svnadmin load' in a 1.x release.  The output
formats are part of the API.  If anyone is tempted to argue with that,
I will simply point to the current situation as proof :-).

I don't think we need to revert r10091.  We just need to tweak it so
that the default is verbosity; -q produces quiet operation; we file a
2.0 issue to change the default; and of course we can revert the
band-aid change I'm about to commit to the test suite.

Objections, alternatives?

-Karl

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: svn commit: r10091 - in trunk/subversion: include libsvn_repos svnadmin

[kf...@collab.net]

kfogel@collab.net writes:
> For now, I'll fix it by adding '--verbose' to the invocation in the
> test framework, so we don't lose any more developer time on this.

Nope, that doesn't seem to work either.

I'm reverting r10091 now.  It can be restored, but only if it doesn't
break the test suite and meets the criteria implied below, IMHO:

> However, in the long term, I feel it's wrong that we changed the
> default output format of 'svnadmin load' in a 1.x release.  The output
> formats are part of the API.  If anyone is tempted to argue with that,
> I will simply point to the current situation as proof :-).
> 
> I don't think we need to revert r10091.  We just need to tweak it so
> that the default is verbosity; -q produces quiet operation; we file a
> 2.0 issue to change the default; and of course we can revert the
> band-aid change I'm about to commit to the test suite.

-Karl

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: svn commit: r10091 - in trunk/subversion: include libsvn_repos svnadmin

[kf...@collab.net]

Tobias Ringström <to...@ringstrom.mine.nu> writes:
> >For now, I'll fix it by adding '--verbose' to the invocation in the
> >test framework, so we don't lose any more developer time on this.
>
> You say in your next email that this didn't work which I find very
> surprising because it works perfectly here. I just changed
>   load_args = ' load "' + dst_path + '"'
> to
>   load_args = ' load -v "' + dst_path + '"'
> 
> in main.py. All tests pass now with my patch included.

I see now what I did wrong: I added the "--verbose" as a separate
string term, but I didn't notice the double quotes that would make it
be part of dst_path in practice.

> >However, in the long term, I feel it's wrong that we changed the
> >default output format of 'svnadmin load' in a 1.x release.  The output
> >formats are part of the API.  If anyone is tempted to argue with that,
> >I will simply point to the current situation as proof :-).
>
> That was why I posted the patch several days before committing it.

Oh, I wasn't accusing you of dereliction of duty :-).  I too failed to
think of the consequences at the time.  When I wrote "we changed the
default output", it wasn't just a rhetorical device.  I really meant
that it is/was all of our responsibility, not just yours.

> >I don't think we need to revert r10091.  We just need to tweak it so
> >that the default is verbosity; -q produces quiet operation; we file a
> >2.0 issue to change the default; and of course we can revert the
> >band-aid change I'm about to commit to the test suite.
>
> There is already a -q option to make it quiet, but it makes it print
> nothing.
> 
> I wanted a less verbose load because I've been running cvs2svn and
> svnadmin load a lot lately and the load scrolls the output so fast
> that I can't even see what revision it is at. It's not more important
> than that, so I'll drop the patch.

Well, let's at least schedule it for 2.0.

(I really like the patch.  My only objection is about compatibility;
in terms of inherent desirability, the patch is a real improvement
IMHO.)

-Karl

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: svn commit: r10091 - in trunk/subversion: include libsvn_repos svnadmin

[Tobias Ringström <to...@ringstrom.mine.nu>]

kfogel@collab.net wrote:

>Ben Collins-Sussman <su...@collab.net> writes:
>  
>
>>>Make svnadmin load less verbose by default and add a --verbose flag.
>>>      
>>>
>>Did we forget to run 'make check'?  This breaks all the
>>svnadmin_tests.py tests.
>>    
>>
Oh crap, I must have. I thought I always did, but obviously I failed 
this time. My apologies!

>For now, I'll fix it by adding '--verbose' to the invocation in the
>test framework, so we don't lose any more developer time on this.
>  
>
You say in your next email that this didn't work which I find very 
surprising because it works perfectly here. I just changed
  load_args = ' load "' + dst_path + '"'
to
  load_args = ' load -v "' + dst_path + '"'

in main.py. All tests pass now with my patch included.

>However, in the long term, I feel it's wrong that we changed the
>default output format of 'svnadmin load' in a 1.x release.  The output
>formats are part of the API.  If anyone is tempted to argue with that,
>I will simply point to the current situation as proof :-).
>  
>
That was why I posted the patch several days before committing it.

>I don't think we need to revert r10091.  We just need to tweak it so
>that the default is verbosity; -q produces quiet operation; we file a
>2.0 issue to change the default; and of course we can revert the
>band-aid change I'm about to commit to the test suite.
>  
>
There is already a -q option to make it quiet, but it makes it print 
nothing.

I wanted a less verbose load because I've been running cvs2svn and 
svnadmin load a lot lately and the load scrolls the output so fast that 
I can't even see what revision it is at. It's not more important than 
that, so I'll drop the patch.

/Tobias


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org