You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Prabhu Gnana Sundar <pr...@collab.net> on 2012/11/01 09:51:48 UTC
Re: [PATCH] Implement svnadmin verify --force
On 10/31/2012 11:31 PM, Julian Foad wrote:
> Daniel Shahaf wrote:
>
>> Julian Foad wrote on Wed, Oct 31, 2012 at 17:36:09 +0000:
>>> Prabhu Gnana Sundar<pr...@collab.net> wrote:
>>> > + revstr = "";
>>> > + svn_handle_error2(notify->err, stderr, FALSE /* non-fatal */,
>>> > + apr_psprintf(scratch_pool, "svnadmin: %s", revstr));
>>> > + return;
>>>
>>> In what cases will the revision number be invalid? This prints a
>>> half-empty message in those cases; what did you intend?
>> The code will print
>> svnadmin: E160004: Corrupt filesystem
>> or
>> svnadmin: Error verifying revision 42: E160004: Corrupt filesystem
>> respectively as the revision number is SVN_INVALID_REVNUM or 42. So
>> there is no "half-empty" message here.
> Oh, I see: that psprintf is the 'prefix' argument of handle_error2. I misunderstood.
>
>> But the code moves the E160004 away from its current location
>> immediately after the "svnadmin:" prefix. I am not sure I like that;
>> is there a good reason not to have the message be of the form
>> svnadmin: E160004: %s
>> in the interest of parseability?
> I agree that would be better: the prefix should remain just "svnadmin: " and the error message should be adjusted instead.
>
> - Julian
Does this look fine ? I personally feel this is easily readable.
* Verified revision 0.
* Verified revision 1.
* Verified revision 2.
Error verifying revision 3. svnadmin: E160004: Missing node-id in node-rev at r3 (offset 787)
Error verifying revision 4. svnadmin: E140001: zlib (uncompress): corrupt data: Decompression of svndiff data failed
* Verified revision 5.
--Prabhu
Re: [PATCH] Implement svnadmin verify --force
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Nov 01, 2012 at 05:40:02PM +0200, Daniel Shahaf wrote:
> Daniel Shahaf wrote on Thu, Nov 01, 2012 at 17:37:53 +0200:
> > That discussion is perfectly within the scope of the current patch:
> > notifying the errors and clearing them _causes_ svn to return non-zero
> > exit code when it encounters a verification error.
> >
> > IOW, the patch causes a regression in the "verification errors cause
> > non-zero exit code" behaviour.
>
> Or, rather, it causes svn to exit(0) despite having printed something to
> stderr. I think it'll be better to exit(1) in that case.
OK, good catch. I agree that exit code behaviour shouldn't be changed.
Re: [PATCH] Implement svnadmin verify --force
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Stefan Sperling wrote on Mon, Nov 05, 2012 at 12:37:19 +0100:
> On Mon, Nov 05, 2012 at 02:42:29PM +0530, Prabhu Gnana Sundar wrote:
> > I guess, this also solves the layer violation problem. Correct me if
> > am wrong.
>
> Daniel suggested to use svn_error_quick_wrap() which you aren't using.
> So I doubt you did what he had in mind. Honestly, I don't really understand
> what he wants you to do either. Daniel, can you take the time to explain
> a bit more clearly what you mean, please?
>
If you wrap notify->err as follows:
svn_error_quick_wrap(notify->err,
_("Error verifying revision %ld:")));
then the output would be:
* Verified revision 2.
svnadmin: E160004: Error verifying revision %ld
svnadmin: E160004: Missing node-id in node-rev at r3 (offset 787)
Now, there are two details I haven't specified:
- Passing the argument of the %ld printf format string. I'm pretty
sure svn_error_quick_wrap doesn't take variadic printf arguments..
- Whether the wrapping happens in the caller or callee of the
notify_func callback. Not hard, just need to ensure each error
is cleared exactly once.
I can argue either for this output or for the "* Error verified revision %ld"
output Prabhu came up with; not sure yet which I prefer.
Makes sense?
Re: [PATCH] Implement svnadmin verify --force
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Daniel Shahaf wrote on Thu, Nov 01, 2012 at 17:37:53 +0200:
> Stefan Sperling wrote on Thu, Nov 01, 2012 at 16:31:24 +0100:
> > On Thu, Nov 01, 2012 at 05:22:14PM +0200, Daniel Shahaf wrote:
> > > +1 to having a non-zero exit code if there was any error throughout.
> >
> > I think we should try to keep this discussion within the scope
> > of the current patch, which is about adding a --keep-going option
> > and providing sensible output when it is used, in order to
> > keep things simple for Prabhu. We can build upon that later
> > and defer things like munging the exit code to a separate patch.
> >
>
> That discussion is perfectly within the scope of the current patch:
> notifying the errors and clearing them _causes_ svn to return non-zero
> exit code when it encounters a verification error.
>
> IOW, the patch causes a regression in the "verification errors cause
> non-zero exit code" behaviour.
Or, rather, it causes svn to exit(0) despite having printed something to
stderr. I think it'll be better to exit(1) in that case.
Re: [PATCH] Implement svnadmin verify --force
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Stefan Sperling wrote on Thu, Nov 01, 2012 at 16:31:24 +0100:
> On Thu, Nov 01, 2012 at 05:22:14PM +0200, Daniel Shahaf wrote:
> > +1 to having a non-zero exit code if there was any error throughout.
>
> I think we should try to keep this discussion within the scope
> of the current patch, which is about adding a --keep-going option
> and providing sensible output when it is used, in order to
> keep things simple for Prabhu. We can build upon that later
> and defer things like munging the exit code to a separate patch.
>
That discussion is perfectly within the scope of the current patch:
notifying the errors and clearing them _causes_ svn to return non-zero
exit code when it encounters a verification error.
IOW, the patch causes a regression in the "verification errors cause
non-zero exit code" behaviour.
Re: [PATCH] Implement svnadmin verify --force
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
> Index: subversion/libsvn_repos/dump.c
> ===================================================================
> --- subversion/libsvn_repos/dump.c (revision 1402414)
> +++ subversion/libsvn_repos/dump.c (working copy)
> @@ -1459,5 +1512,8 @@
> /* Per-backend verification. */
> svn_pool_destroy(iterpool);
>
> + if (found_corruption)
> + return svn_error_createf(SVN_ERR_REPOS_CORRUPTED, NULL,
> + _("Repository has corruptions."));
You don't know that the repository is corrupt (maybe the error was EPERM
on the db/ dir). "Repository '%s' failed to verify" would be better.
Re: [PATCH] Implement svnadmin verify --force
Posted by Mark Phippard <ma...@gmail.com>.
On Thu, Nov 1, 2012 at 1:22 PM, Stefan Sperling <st...@elego.de> wrote:
> On Thu, Nov 01, 2012 at 04:53:41PM +0100, Bert Huijben wrote:
>> Maybe we should default to this new behavior and *add* a quick exit on error
>> for whoever needs it.
>
> That's what I've been arguing all along. I think the new behaviour
> should be the default.
Do we need a new quick exit? I do not think we do. I like idea of
changing the default behavior as long as it exits with an error code.
I would guess most people expected it to report all errors in the
repository as the default.
--
Thanks
Mark Phippard
http://markphip.blogspot.com/
Re: [PATCH] Implement svnadmin verify --force
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Nov 01, 2012 at 04:53:41PM +0100, Bert Huijben wrote:
> Maybe we should default to this new behavior and *add* a quick exit on error
> for whoever needs it.
That's what I've been arguing all along. I think the new behaviour
should be the default.
> I think the total report on which revisions are broken is more informational
> than just the first error. And I don't like the '--force' for argument for
> continuing.
It's called --keep-going in the current iteration of the patch, not --force.
Re: [PATCH] Implement svnadmin verify --force
Posted by Stefan Sperling <st...@elego.de>.
On Mon, Nov 05, 2012 at 02:42:29PM +0530, Prabhu Gnana Sundar wrote:
> On 11/04/2012 06:14 AM, Daniel Shahaf wrote:
> >
> >>Index: subversion/svnadmin/main.c
> >>===================================================================
> >>--- subversion/svnadmin/main.c (revision 1402414)
> >>+++ subversion/svnadmin/main.c (working copy)
> >>@@ -738,6 +743,16 @@
> >> notify->warning_str));
> >> return;
> >>
> >>+ case svn_repos_notify_failure:
> >>+ if (notify->revision != SVN_INVALID_REVNUM)
> >>+ svn_error_clear(svn_stream_printf(feedback_stream, scratch_pool,
> >>+ _("svnadmin: E%d: Error verifying revision %ld\n"),
> >-1 (layering violation). Use svn_error_quick_wrap().
> >
> >While you're at it, wrap to 80 columns.
>
> Thanks Daniel,
>
> Now I have made something like
>
> <snip>
>
> * Verified revision 0.
> * Verified revision 1.
> * Verified revision 2.
> * Error verifying revision 3.
> svnadmin: E160004: Missing node-id in node-rev at r3 (offset 787)
> * Error verifying revision 4.
> svnadmin: E140001: zlib (uncompress): corrupt data: Decompression of svndiff data failed
> * Verified revision 5.
> svnadmin: E165005: Repository 'testrepo' failed to verify.
>
> </snip>
>
>
> I have used "* Error verifying revision x" instead of "svnadmin:
> Error verifying revision x" because when a revision is successfully
> verified we just say it as "* Verified revision x". I feel that this
> is a bit more consistent.
I'm fine with this output.
> So that makes the code to look like,
>
> <snip>
>
> case svn_repos_notify_failure:
> if (notify->revision != SVN_INVALID_REVNUM)
> svn_error_clear(svn_stream_printf(feedback_stream, scratch_pool,
> _("* Error verifying
> revision %ld.\n"),
> notify->revision));
> svn_handle_error2(notify->err, stderr, FALSE /* non-fatal */,
> "svnadmin: ");
Please always align function parameters on consecutive lines along
the column of the opening brace, like this:
svn_handle_error2(notify->err, stderr, FALSE /* non-fatal */,
"svnadmin: ");
^ this column
^ not this one!
Can you please send a complete patch we can try instead of
pasting code snippets?
> </snip>
>
> I guess, this also solves the layer violation problem. Correct me if
> am wrong.
Daniel suggested to use svn_error_quick_wrap() which you aren't using.
So I doubt you did what he had in mind. Honestly, I don't really understand
what he wants you to do either. Daniel, can you take the time to explain
a bit more clearly what you mean, please?
Have you checked whether you patch breaks 'make check'? You may need
to tweak expected output in some tests to prevent them from failing.
Re: [PATCH] Implement svnadmin verify --force
Posted by Prabhu Gnana Sundar <pr...@collab.net>.
On 11/04/2012 06:14 AM, Daniel Shahaf wrote:
>
>> Index: subversion/svnadmin/main.c
>> ===================================================================
>> --- subversion/svnadmin/main.c (revision 1402414)
>> +++ subversion/svnadmin/main.c (working copy)
>> @@ -738,6 +743,16 @@
>> notify->warning_str));
>> return;
>>
>> + case svn_repos_notify_failure:
>> + if (notify->revision != SVN_INVALID_REVNUM)
>> + svn_error_clear(svn_stream_printf(feedback_stream, scratch_pool,
>> + _("svnadmin: E%d: Error verifying revision %ld\n"),
> -1 (layering violation). Use svn_error_quick_wrap().
>
> While you're at it, wrap to 80 columns.
Thanks Daniel,
Now I have made something like
<snip>
* Verified revision 0.
* Verified revision 1.
* Verified revision 2.
* Error verifying revision 3.
svnadmin: E160004: Missing node-id in node-rev at r3 (offset 787)
* Error verifying revision 4.
svnadmin: E140001: zlib (uncompress): corrupt data: Decompression of svndiff data failed
* Verified revision 5.
svnadmin: E165005: Repository 'testrepo' failed to verify.
</snip>
I have used "* Error verifying revision x" instead of "svnadmin: Error
verifying revision x" because when a revision is successfully verified
we just say it as "* Verified revision x". I feel that this is a bit
more consistent.
So that makes the code to look like,
<snip>
case svn_repos_notify_failure:
if (notify->revision != SVN_INVALID_REVNUM)
svn_error_clear(svn_stream_printf(feedback_stream, scratch_pool,
_("* Error verifying revision
%ld.\n"),
notify->revision));
svn_handle_error2(notify->err, stderr, FALSE /* non-fatal */,
"svnadmin: ");
</snip>
I guess, this also solves the layer violation problem. Correct me if am
wrong.
Regards
Prabhu
Re: [PATCH] Implement svnadmin verify --force
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
> Index: subversion/libsvn_repos/dump.c
> ===================================================================
> --- subversion/libsvn_repos/dump.c (revision 1402414)
> +++ subversion/libsvn_repos/dump.c (working copy)
> @@ -1413,19 +1443,31 @@
> void *cancel_edit_baton;
> svn_fs_root_t *to_root;
> apr_hash_t *props;
> + svn_error_t *err;
You were asked to fix this in an earlier review.
> + if (err && keep_going)
> + {
> + notify_verification_error(rev, err, notify_func, notify_baton,
> + iterpool);
> + svn_error_clear(err);
found_corruption = TRUE;
> + return svn_error_createf(SVN_ERR_REPOS_CORRUPTED, NULL,
> + _("Repository has corruptions."));
While I'm here...
- No trailing period, per HACKING
- Why not pass NULL for the last argument?
- Document the fact that an error is returned in this case
> Index: subversion/svnadmin/main.c
> ===================================================================
> --- subversion/svnadmin/main.c (revision 1402414)
> +++ subversion/svnadmin/main.c (working copy)
> @@ -738,6 +743,16 @@
> notify->warning_str));
> return;
>
> + case svn_repos_notify_failure:
> + if (notify->revision != SVN_INVALID_REVNUM)
> + svn_error_clear(svn_stream_printf(feedback_stream, scratch_pool,
> + _("svnadmin: E%d: Error verifying revision %ld\n"),
-1 (layering violation). Use svn_error_quick_wrap().
While you're at it, wrap to 80 columns.
Re: [PATCH] Implement svnadmin verify --force
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Prabhu Gnana Sundar wrote on Thu, Nov 01, 2012 at 23:59:44 +0530:
>
>
> Julian Foad <ju...@btopenworld.com> wrote:
>
> >Prabhu Gnana Sundar <pr...@collab.net> wrote:
> >
> >> Julian Foad <ju...@btopenworld.com> wrote:
> >>> At the very least there must be agreement on how it's going to exit
> >and
> >>> commitment to make it happen. I already asked the question several
> >>> messages back. Prabhu, please share your thoughts.
> >>
> >> Yeah Julian, once an error is encountered, the error message is
> >notified and the
> >> exit code is non zero.
> >
> >What? Are you talking about an existing version of your patch or
> >stating your intentions for the future?
>
> I am talking about the existing version of my patch.
>
>
> Are you talking about an error
> >message to be printed at the end (when the command exits), or the error
> >message per revision?
>
> When a verification failure is encountered, the error message is notified for that particular revision and the verification process continues iff --keep-going is true. Finally the command exits with a exit code which is non-zero.
No, it doesn't.
% $svnadmin verify --keep-going r ; echo $?
subversion/libsvn_fs/fs-loader.c:500: (apr_err=160006)
subversion/libsvn_fs_fs/fs_fs.c:10227: (apr_err=160006)
subversion/libsvn_fs_fs/tree.c:608: (apr_err=160006)
subversion/libsvn_fs_fs/dag.c:611: (apr_err=160006)
subversion/libsvn_fs_fs/fs_fs.c:3019: (apr_err=160006)
subversion/libsvn_fs_fs/fs_fs.c:1877: (apr_err=160006)
svnadmin: E160006: No such revision 0
subversion/libsvn_repos/dump.c:983: (apr_err=160006)
subversion/libsvn_fs/fs-loader.c:858: (apr_err=160006)
subversion/libsvn_fs_fs/tree.c:608: (apr_err=160006)
subversion/libsvn_fs_fs/dag.c:611: (apr_err=160006)
subversion/libsvn_fs_fs/fs_fs.c:3019: (apr_err=160006)
subversion/libsvn_fs_fs/fs_fs.c:1877: (apr_err=160006)
svnadmin: Error verifying revision 0: E160006: No such revision 0
subversion/libsvn_repos/dump.c:983: (apr_err=160006)
subversion/libsvn_fs/fs-loader.c:858: (apr_err=160006)
subversion/libsvn_fs_fs/tree.c:608: (apr_err=160006)
subversion/libsvn_fs_fs/dag.c:611: (apr_err=160006)
subversion/libsvn_fs_fs/fs_fs.c:3019: (apr_err=160006)
subversion/libsvn_fs_fs/fs_fs.c:1877: (apr_err=160006)
svnadmin: Error verifying revision 1: E160006: No such revision 1
0
Re: [PATCH] Implement svnadmin verify --force
Posted by Prabhu Gnana Sundar <pr...@collab.net>.
Julian Foad <ju...@btopenworld.com> wrote:
>Prabhu Gnana Sundar <pr...@collab.net> wrote:
>
>> Julian Foad <ju...@btopenworld.com> wrote:
>>> At the very least there must be agreement on how it's going to exit
>and
>>> commitment to make it happen. I already asked the question several
>>> messages back. Prabhu, please share your thoughts.
>>
>> Yeah Julian, once an error is encountered, the error message is
>notified and the
>> exit code is non zero.
>
>What? Are you talking about an existing version of your patch or
>stating your intentions for the future?
I am talking about the existing version of my patch.
Are you talking about an error
>message to be printed at the end (when the command exits), or the error
>message per revision?
When a verification failure is encountered, the error message is notified for that particular revision and the verification process continues iff --keep-going is true. Finally the command exits with a exit code which is non-zero.
--
Prabhu
Re: [PATCH] Implement svnadmin verify --force
Posted by Julian Foad <ju...@btopenworld.com>.
Prabhu Gnana Sundar <pr...@collab.net> wrote:
> Julian Foad <ju...@btopenworld.com> wrote:
>> At the very least there must be agreement on how it's going to exit and
>> commitment to make it happen. I already asked the question several
>> messages back. Prabhu, please share your thoughts.
>
> Yeah Julian, once an error is encountered, the error message is notified and the
> exit code is non zero.
What? Are you talking about an existing version of your patch or stating your intentions for the future? Are you talking about an error message to be printed at the end (when the command exits), or the error message per revision? Please be more precise.
- Julian
Re: [PATCH] Implement svnadmin verify --force
Posted by Prabhu Gnana Sundar <pr...@collab.net>.
Julian Foad <ju...@btopenworld.com> wrote:
>
>At the very least there must be agreement on how it's going to exit and
>commitment to make it happen. I already asked the question several
>messages back. Prabhu, please share your thoughts.
>
Yeah Julian, once an error is encountered, the error message is notified and the exit code is non zero.
--
Prabhu
Re: [PATCH] Implement svnadmin verify --force
Posted by Julian Foad <ju...@btopenworld.com>.
Stefan Sperling wrote:
> On Thu, Nov 01, 2012 at 05:22:14PM +0200, Daniel Shahaf wrote:
>> +1 to having a non-zero exit code if there was any error throughout.
>
> I think we should try to keep this discussion within the scope
> of the current patch, which is about adding a --keep-going option
> and providing sensible output when it is used, in order to
> keep things simple for Prabhu. We can build upon that later
> and defer things like munging the exit code to a separate patch.
I would be very reluctant to commit any patch that allows "svnadmin verify" to return a zero exit code after it found an error. Keeping the scope of the change constrained is a worthy goal but the possibility that the required follow-up might get forgotten is too much. I would want the second patch to be ready to commit before we commit the first.
At the very least there must be agreement on how it's going to exit and commitment to make it happen. I already asked the question several messages back. Prabhu, please share your thoughts.
- Julian
Re: [PATCH] Implement svnadmin verify --force
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Nov 01, 2012 at 05:22:14PM +0200, Daniel Shahaf wrote:
> +1 to having a non-zero exit code if there was any error throughout.
I think we should try to keep this discussion within the scope
of the current patch, which is about adding a --keep-going option
and providing sensible output when it is used, in order to
keep things simple for Prabhu. We can build upon that later
and defer things like munging the exit code to a separate patch.
Re: [PATCH] Implement svnadmin verify --force
Posted by Prabhu Gnana Sundar <pr...@collab.net>.
>Near the start of this thread we mentioned the main use cases. There
>are basically two:
>
>* Confirm that the repository is in a good state (e.g. before/after a
>backup);
>
>* Diagnostic mode for use when a problem has been encountered.
>
>Keep-going mode is more useful for the latter; stop-quickly mode *may*
>be more useful (and is certainly fine) for the former. So it's not
>obvious that changing the default mode is a good idea.
>
Yes, I don't want to change the default behaviour either. Because if the --keep-going behaviour is made default, we would need a quick-stop mode to just see if the repo is sane, which a normal user would want to be the default behaviour of svnadmin verify.
--
Prabhu
Re: [PATCH] Implement svnadmin verify --force
Posted by Julian Foad <ju...@btopenworld.com>.
Bert Huijben wrote:
>> +1 to having a non-zero exit code if there was any error throughout.
>
> In that case: why do we add --force?
The option is called --keep-going.
> Maybe we should default to this new behavior and *add* a quick exit on error
> for whoever needs it.
>
> I think the total report on which revisions are broken is more informational
> than just the first error. And I don't like the '--force' for
> argument for
> continuing.
Near the start of this thread we mentioned the main use cases. There are basically two:
* Confirm that the repository is in a good state (e.g. before/after a backup);
* Diagnostic mode for use when a problem has been encountered.
Keep-going mode is more useful for the latter; stop-quickly mode *may* be more useful (and is certainly fine) for the former. So it's not obvious that changing the default mode is a good idea.
- Julian
RE: [PATCH] Implement svnadmin verify --force
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Daniel Shahaf [mailto:d.s@daniel.shahaf.name]
> Sent: donderdag 1 november 2012 16:22
> To: C. Michael Pilato
> Cc: Julian Foad; Stefan Sperling; Prabhu Gnana Sundar;
> dev@subversion.apache.org
> Subject: Re: [PATCH] Implement svnadmin verify --force
>
> C. Michael Pilato wrote on Thu, Nov 01, 2012 at 10:56:59 -0400:
> > On 11/01/2012 10:33 AM, Julian Foad wrote:
> > >> Agreed. And for what it's worth, I like the second form, especially
if
> > >> the errorful lines go to stderr.
> > >
> > > Hmm, it's also reasonable to consider a combination of both: print a
> > > notification for every revision ("Verified rX" or "FAILED to verify
rX"
> > > on stdout, AND an error message on stderr for each error.
> >
> > Yes, I think that's ideal, so long as in the error case both the message
to
> > stdout ("FAILED to verify rX") and the message(s) to stderr carry the
> > revision number. In other words, both streams should be independently
> > valuable to the reader.
>
> +1 to having a non-zero exit code if there was any error throughout.
In that case: why do we add --force?
Maybe we should default to this new behavior and *add* a quick exit on error
for whoever needs it.
I think the total report on which revisions are broken is more informational
than just the first error. And I don't like the '--force' for argument for
continuing.
Bert
Re: [PATCH] Implement svnadmin verify --force
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
C. Michael Pilato wrote on Thu, Nov 01, 2012 at 10:56:59 -0400:
> On 11/01/2012 10:33 AM, Julian Foad wrote:
> >> Agreed. And for what it's worth, I like the second form, especially if
> >> the errorful lines go to stderr.
> >
> > Hmm, it's also reasonable to consider a combination of both: print a
> > notification for every revision ("Verified rX" or "FAILED to verify rX"
> > on stdout, AND an error message on stderr for each error.
>
> Yes, I think that's ideal, so long as in the error case both the message to
> stdout ("FAILED to verify rX") and the message(s) to stderr carry the
> revision number. In other words, both streams should be independently
> valuable to the reader.
+1 to having a non-zero exit code if there was any error throughout.
Re: [PATCH] Implement svnadmin verify --force
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 11/01/2012 10:33 AM, Julian Foad wrote:
>> Agreed. And for what it's worth, I like the second form, especially if
>> the errorful lines go to stderr.
>
> Hmm, it's also reasonable to consider a combination of both: print a
> notification for every revision ("Verified rX" or "FAILED to verify rX"
> on stdout, AND an error message on stderr for each error.
Yes, I think that's ideal, so long as in the error case both the message to
stdout ("FAILED to verify rX") and the message(s) to stderr carry the
revision number. In other words, both streams should be independently
valuable to the reader.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Enterprise Cloud Development
Re: [PATCH] Implement svnadmin verify --force
Posted by Julian Foad <ju...@btopenworld.com>.
--
Subversion Live 2012 - 2 Days: training, networking, demos & more! http://bit.ly/NgUaRi
Certified & Supported Apache Subversion Downloads: http://www.wandisco.com/subversion/download
----- Original Message -----
> From: C. Michael Pilato <cm...@collab.net>
> To: Julian Foad <ju...@btopenworld.com>
> Cc: Stefan Sperling <st...@elego.de>; Prabhu Gnana Sundar <pr...@collab.net>; Daniel Shahaf <d....@daniel.shahaf.name>; "dev@subversion.apache.org" <de...@subversion.apache.org>
> Sent: Thursday, 1 November 2012, 10:29
> Subject: Re: [PATCH] Implement svnadmin verify --force
>
> On 11/01/2012 10:17 AM, Julian Foad wrote:
>> That's easily readable, but I don't like it: it's a funny
> mixture of styles. We should choose either "notification" style (that
> is, messages that are not error messages), such as
>>
>> * Verified revision 0.
>> * Verified revision 1.
>> * Verified revision 2.
>> * Error verifying revision 3: Missing node-id in node-rev at r3 (offset
> 787)
>> * Error verifying revision 4: zlib (uncompress): corrupt data:
> Decompression of svndiff data failed
>> * Verified revision 5.
>>
>> or "error messages" style, in which case the messages should be
> formatted like all svn err msgs, for example:
>>
>> * Verified revision 0.
>> * Verified revision 1.
>> * Verified revision 2.
>> svnadmin: E199999: Error verifying revision 3
>> svnadmin: E160004: Missing node-id in node-rev at r3 (offset 787)
>> svnadmin: E199999: Error verifying revision 4
>> svnadmin: E140001: zlib (uncompress): corrupt data: Decompression of
> svndiff data failed
>> * Verified revision 5.
>
> Agreed. And for what it's worth, I like the second form, especially if the
> errorful lines go to stderr.
Hmm, it's also reasonable to consider a combination of both: print a notification for every revision ("Verified rX" or "FAILED to verify rX" on stdout, AND an error message on stderr for each error.
- Julian
Re: [PATCH] Implement svnadmin verify --force
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 11/01/2012 10:17 AM, Julian Foad wrote:
> That's easily readable, but I don't like it: it's a funny mixture of styles. We should choose either "notification" style (that is, messages that are not error messages), such as
>
> * Verified revision 0.
> * Verified revision 1.
> * Verified revision 2.
> * Error verifying revision 3: Missing node-id in node-rev at r3 (offset 787)
> * Error verifying revision 4: zlib (uncompress): corrupt data: Decompression of svndiff data failed
> * Verified revision 5.
>
> or "error messages" style, in which case the messages should be formatted like all svn err msgs, for example:
>
> * Verified revision 0.
> * Verified revision 1.
> * Verified revision 2.
> svnadmin: E199999: Error verifying revision 3
> svnadmin: E160004: Missing node-id in node-rev at r3 (offset 787)
> svnadmin: E199999: Error verifying revision 4
> svnadmin: E140001: zlib (uncompress): corrupt data: Decompression of svndiff data failed
> * Verified revision 5.
Agreed. And for what it's worth, I like the second form, especially if the
errorful lines go to stderr.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Enterprise Cloud Development
Re: [PATCH] Implement svnadmin verify --force
Posted by Prabhu Gnana Sundar <pr...@collab.net>.
Folks,
I seriously think that it is time for me to take a break from sending
iterative patches and just sit back and analyse the whole problem from
first. I value all your precious times spent on this and I wish to come
back with a much cleaner work which would abide by the coding standards
of the subversion community. Now that I have got more and more advices
and ideas from you guys, I guess I need to digest them all and come back
with a cleaner work for this feature.
Special thanks to Daniel, Stefan, Julian and Mike. Will get back soon
with a cleanly analysed work.
Thanks and regards
Prabhu
On 11/05/2012 07:38 PM, Julian Foad wrote:
> Prabhu Gnana Sundar<pr...@collab.net> wrote:
>> Julian Foad wrote:
>>> [...] We should choose either "notification" style
>>> (that is, messages that are not error messages), such as
> [...]
>>> or "error messages" style, in which case the messages should be
>>> formatted like all svn err msgs, for example:
> [...]
>>> But the output-style design here is linked to the question of what
>>> we're going to print at the end of the verification. Prabhu, what's
>>> the plan for when the command exits? Print a summary or an error
>>> message? Exit with non-zero return code (I hope)?
>> I made the output to be exactly like the one you have suggested above.
> Oh... Why did you choose that and not C-Mike Pilato's follow-up suggestion?
>
>
>> But, I am also adding one more error message as follows
>>
>> svnadmin: E165005: Repository has corruptions.
>>
>> and then exit with exit code 1.
> Great, that sounds good, apart from what somebody suggested about changing the wording to something like 'failed to verify'.
>
>> The default behaviour is not at all affected.
>> This new error message is effected only when --keep-going is true. Is that fine
>> ? Now all the test cases pass.
> I think when --keep-going is false we should still emit this new error -- in addition to whatever it already emitted. That would change the existing behaviour a bit, but I think in a good way, because it would be more consistent.
>
>
>> Attaching the patch for you to have a look at it.
>>
>> Not attaching the log message since I am yet to work on sending the
>> "Verified revision x" to the stdout.
> The log message helps people to review the patch, by explaining the reason for the changes. Sorry, but I am not willing to review your patch without a log message.
>
> Also, it would
> really help me (and other reviewers) if you would write responses to the review
> comments. There have been lots of comments, probably many more than you were expecting. I can't tell whether you have read them all and addressed them all in your latest patch, or if you have not yet had enough time to work through them.
>
>
>> I am facing a few issues in it.
>> Like, a few tests fail because the expected output don't match with the
>> actual output. So need suggestions on that too. Is it fine to tweak the already
>> available test case's expected output based on the output of what we send to
>> stdout and stderr ?
> Yes, it is OK to tweak the expectations of existing tests when we change some details of the existing behaviour.
>
> - Julian
Re: [PATCH] Implement svnadmin verify --force
Posted by Julian Foad <ju...@btopenworld.com>.
Prabhu Gnana Sundar <pr...@collab.net> wrote:
> Julian Foad wrote:
>> [...] We should choose either "notification" style
>> (that is, messages that are not error messages), such as
[...]
>> or "error messages" style, in which case the messages should be
>> formatted like all svn err msgs, for example:
[...]
>> But the output-style design here is linked to the question of what
>> we're going to print at the end of the verification. Prabhu, what's
>> the plan for when the command exits? Print a summary or an error
>> message? Exit with non-zero return code (I hope)?
>
> I made the output to be exactly like the one you have suggested above.
Oh... Why did you choose that and not C-Mike Pilato's follow-up suggestion?
> But, I am also adding one more error message as follows
>
> svnadmin: E165005: Repository has corruptions.
>
> and then exit with exit code 1.
Great, that sounds good, apart from what somebody suggested about changing the wording to something like 'failed to verify'.
> The default behaviour is not at all affected.
> This new error message is effected only when --keep-going is true. Is that fine
> ? Now all the test cases pass.
I think when --keep-going is false we should still emit this new error -- in addition to whatever it already emitted. That would change the existing behaviour a bit, but I think in a good way, because it would be more consistent.
> Attaching the patch for you to have a look at it.
>
> Not attaching the log message since I am yet to work on sending the
> "Verified revision x" to the stdout.
The log message helps people to review the patch, by explaining the reason for the changes. Sorry, but I am not willing to review your patch without a log message.
Also, it would
really help me (and other reviewers) if you would write responses to the review
comments. There have been lots of comments, probably many more than you were expecting. I can't tell whether you have read them all and addressed them all in your latest patch, or if you have not yet had enough time to work through them.
> I am facing a few issues in it.
> Like, a few tests fail because the expected output don't match with the
> actual output. So need suggestions on that too. Is it fine to tweak the already
> available test case's expected output based on the output of what we send to
> stdout and stderr ?
Yes, it is OK to tweak the expectations of existing tests when we change some details of the existing behaviour.
- Julian
Re: [PATCH] Implement svnadmin verify --force
Posted by Prabhu Gnana Sundar <pr...@collab.net>.
>>>>> But the code moves the E160004 away from its current location
>>>>> immediately after the "svnadmin:" prefix. I am not sure I like that;
>>>>> is there a good reason not to have the message be of the form
>>>>> svnadmin: E160004: %s
>>>>> in the interest of parseability?
>>>> I agree that would be better: the prefix should remain just
>>>> "svnadmin: " and the error message should be adjusted instead.
>>> Does this look fine ? I personally feel this is easily readable.
>>>
>>> * Verified revision 0.
>>> * Verified revision 1.
>>> * Verified revision 2.
>>> Error verifying revision 3. svnadmin: E160004: Missing node-id in node-rev
>> at r3 (offset 787)
>>> Error verifying revision 4. svnadmin: E140001: zlib (uncompress): corrupt
>> data: Decompression of svndiff data failed
>>> * Verified revision 5.
> That's easily readable, but I don't like it: it's a funny mixture of styles. We should choose either "notification" style (that is, messages that are not error messages), such as
>
> * Verified revision 0.
> * Verified revision 1.
> * Verified revision 2.
> * Error verifying revision 3: Missing node-id in node-rev at r3 (offset 787)
> * Error verifying revision 4: zlib (uncompress): corrupt data: Decompression of svndiff data failed
> * Verified revision 5.
>
> or "error messages" style, in which case the messages should be formatted like all svn err msgs, for example:
>
> * Verified revision 0.
> * Verified revision 1.
> * Verified revision 2.
> svnadmin: E199999: Error verifying revision 3
> svnadmin: E160004: Missing node-id in node-rev at r3 (offset 787)
> svnadmin: E199999: Error verifying revision 4
> svnadmin: E140001: zlib (uncompress): corrupt data: Decompression of svndiff data failed
> * Verified revision 5.
>
> But the output-style design here is linked to the question of what we're going to print at the end of the verification. Prabhu, what's the plan for when the command exits? Print a summary or an error message? Exit with non-zero return code (I hope)?
I made the output to be exactly like the one you have suggested above.
But, I am also adding one more error message as follows
svnadmin: E165005: Repository has corruptions.
and then exit with exit code 1. The default behaviour is not at all
affected. This new error message is effected only when --keep-going is
true. Is that fine ? Now all the test cases pass.
Attaching the patch for you to have a look at it.
Not attaching the log message since I am yet to work on sending the
"Verified revision x" to the stdout. I am facing a few issues in it.
Like, a few tests fail because the expected output don't match with the
actual output. So need suggestions on that too. Is it fine to tweak the
already available test case's expected output based on the output of
what we send to stdout and stderr ?
Thanks and regards
Prabhu
Re: [PATCH] Implement svnadmin verify --force
Posted by Julian Foad <ju...@btopenworld.com>.
Stefan Sperling wrote:
> On Thu, Nov 01, 2012 at 02:21:48PM +0530, Prabhu Gnana Sundar wrote:
>> On 10/31/2012 11:31 PM, Julian Foad wrote:
>>> Daniel Shahaf wrote:
>>>> The code will print
>>>> svnadmin: E160004: Corrupt filesystem
>>>> or
>>>> svnadmin: Error verifying revision 42: E160004: Corrupt filesystem
[...]
>>>> But the code moves the E160004 away from its current location
>>>> immediately after the "svnadmin:" prefix. I am not sure I like that;
>>>> is there a good reason not to have the message be of the form
>>>> svnadmin: E160004: %s
>>>> in the interest of parseability?
>>>
>>> I agree that would be better: the prefix should remain just
>>> "svnadmin: " and the error message should be adjusted instead.
>>
>> Does this look fine ? I personally feel this is easily readable.
>>
>> * Verified revision 0.
>> * Verified revision 1.
>> * Verified revision 2.
>> Error verifying revision 3. svnadmin: E160004: Missing node-id in node-rev
> at r3 (offset 787)
>> Error verifying revision 4. svnadmin: E140001: zlib (uncompress): corrupt
> data: Decompression of svndiff data failed
>> * Verified revision 5.
That's easily readable, but I don't like it: it's a funny mixture of styles. We should choose either "notification" style (that is, messages that are not error messages), such as
* Verified revision 0.
* Verified revision 1.
* Verified revision 2.
* Error verifying revision 3: Missing node-id in node-rev at r3 (offset 787)
* Error verifying revision 4: zlib (uncompress): corrupt data: Decompression of svndiff data failed
* Verified revision 5.
or "error messages" style, in which case the messages should be formatted like all svn err msgs, for example:
* Verified revision 0.
* Verified revision 1.
* Verified revision 2.
svnadmin: E199999: Error verifying revision 3
svnadmin: E160004: Missing node-id in node-rev at r3 (offset 787)
svnadmin: E199999: Error verifying revision 4
svnadmin: E140001: zlib (uncompress): corrupt data: Decompression of svndiff data failed
* Verified revision 5.
But the output-style design here is linked to the question of what we're going to print at the end of the verification. Prabhu, what's the plan for when the command exits? Print a summary or an error message? Exit with non-zero return code (I hope)?
> The lines should start with "svnamdin: ".
The more I look at it, the more I think it makes sense to use notification-style output, and then exit with an error (a standard svn-style error message and non-zero exit code) at the end.
> I believe what Julian was suggesting is to stop doing the apr_psprintf()
> dance I suggested, and make libsvn_repos return a better error message
> instead which includes the revision number. Is that right, Julian?
(Well, that's one way of implementing the behaviour that I was suggesting.)
- Julian
Re: [PATCH] Implement svnadmin verify --force
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Stefan Sperling wrote on Thu, Nov 01, 2012 at 11:59:43 +0100:
> On Thu, Nov 01, 2012 at 02:21:48PM +0530, Prabhu Gnana Sundar wrote:
> > On 10/31/2012 11:31 PM, Julian Foad wrote:
> > >Daniel Shahaf wrote:
> > >
> > >>Julian Foad wrote on Wed, Oct 31, 2012 at 17:36:09 +0000:
> > >>> Prabhu Gnana Sundar<pr...@collab.net> wrote:
> > >>> > + revstr = "";
> > >>> > + svn_handle_error2(notify->err, stderr, FALSE /* non-fatal */,
> > >>> > + apr_psprintf(scratch_pool, "svnadmin: %s", revstr));
> > >>> > + return;
> > >>>
> > >>> In what cases will the revision number be invalid? This prints a
> > >>>half-empty message in those cases; what did you intend?
> > >>The code will print
> > >> svnadmin: E160004: Corrupt filesystem
> > >>or
> > >> svnadmin: Error verifying revision 42: E160004: Corrupt filesystem
> > >>respectively as the revision number is SVN_INVALID_REVNUM or 42. So
> > >>there is no "half-empty" message here.
> > >Oh, I see: that psprintf is the 'prefix' argument of handle_error2. I misunderstood.
> > >
> > >>But the code moves the E160004 away from its current location
> > >>immediately after the "svnadmin:" prefix. I am not sure I like that;
> > >>is there a good reason not to have the message be of the form
> > >> svnadmin: E160004: %s
> > >>in the interest of parseability?
> > >I agree that would be better: the prefix should remain just "svnadmin: " and the error message should be adjusted instead.
> > >
> > >- Julian
> >
> > Does this look fine ? I personally feel this is easily readable.
> >
> >
> > * Verified revision 0.
> > * Verified revision 1.
> > * Verified revision 2.
> > Error verifying revision 3. svnadmin: E160004: Missing node-id in node-rev at r3 (offset 787)
> > Error verifying revision 4. svnadmin: E140001: zlib (uncompress): corrupt data: Decompression of svndiff data failed
> > * Verified revision 5.
>
> The lines should start with "svnamdin: ".
>
> I believe what Julian was suggesting is to stop doing the apr_psprintf()
> dance I suggested, and make libsvn_repos return a better error message
> instead which includes the revision number. Is that right, Julian?
svn_error_quick_wrap maybe?
Re: [PATCH] Implement svnadmin verify --force
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Nov 01, 2012 at 02:21:48PM +0530, Prabhu Gnana Sundar wrote:
> On 10/31/2012 11:31 PM, Julian Foad wrote:
> >Daniel Shahaf wrote:
> >
> >>Julian Foad wrote on Wed, Oct 31, 2012 at 17:36:09 +0000:
> >>> Prabhu Gnana Sundar<pr...@collab.net> wrote:
> >>> > + revstr = "";
> >>> > + svn_handle_error2(notify->err, stderr, FALSE /* non-fatal */,
> >>> > + apr_psprintf(scratch_pool, "svnadmin: %s", revstr));
> >>> > + return;
> >>>
> >>> In what cases will the revision number be invalid? This prints a
> >>>half-empty message in those cases; what did you intend?
> >>The code will print
> >> svnadmin: E160004: Corrupt filesystem
> >>or
> >> svnadmin: Error verifying revision 42: E160004: Corrupt filesystem
> >>respectively as the revision number is SVN_INVALID_REVNUM or 42. So
> >>there is no "half-empty" message here.
> >Oh, I see: that psprintf is the 'prefix' argument of handle_error2. I misunderstood.
> >
> >>But the code moves the E160004 away from its current location
> >>immediately after the "svnadmin:" prefix. I am not sure I like that;
> >>is there a good reason not to have the message be of the form
> >> svnadmin: E160004: %s
> >>in the interest of parseability?
> >I agree that would be better: the prefix should remain just "svnadmin: " and the error message should be adjusted instead.
> >
> >- Julian
>
> Does this look fine ? I personally feel this is easily readable.
>
>
> * Verified revision 0.
> * Verified revision 1.
> * Verified revision 2.
> Error verifying revision 3. svnadmin: E160004: Missing node-id in node-rev at r3 (offset 787)
> Error verifying revision 4. svnadmin: E140001: zlib (uncompress): corrupt data: Decompression of svndiff data failed
> * Verified revision 5.
The lines should start with "svnamdin: ".
I believe what Julian was suggesting is to stop doing the apr_psprintf()
dance I suggested, and make libsvn_repos return a better error message
instead which includes the revision number. Is that right, Julian?