You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Branko Čibej <br...@wandisco.com> on 2014/03/08 14:14:48 UTC
Re: svn commit: r1574868 - in /subversion/trunk/subversion: libsvn_repos/dump.c
tests/libsvn_repos/repos-test.c
On 06.03.2014 14:09, julianfoad@apache.org wrote:
> Author: julianfoad
> Date: Thu Mar 6 13:09:26 2014
> New Revision: 1574868
>
> URL: http://svn.apache.org/r1574868
> Log:
> Don't let invalid mergeinfo stop 'svnadmin dump' from producing a dump.
> Just continue.
[...]
> + /* An error in verifying the mergeinfo must not prevent dumping
> + the data. Ignore any such error. */
> + svn_error_clear(verify_mergeinfo_revisions(
> + eb->found_old_mergeinfo,
> + mergeinfo_str->data, eb->oldest_dumped_rev,
> + eb->notify_func, eb->notify_baton,
> + pool));
> }
> }
Shouldn't we at least emit a warning here?
-- Brane
--
Branko Čibej | Director of Subversion
WANdisco // Non-Stop Data
e. brane@wandisco.com
Re: svn commit: r1574868 - dump with r0 mergeinfo
Posted by Julian Foad <ju...@btopenworld.com>.
[switching back to plain text]
Branko Čibej wrote:
> Julian Foad wrote:
>> Branko Čibej wrote:
>>> + /* An error in verifying the mergeinfo must not prevent dumping
>>> + the data. Ignore any such error. */
>>> + svn_error_clear(verify_mergeinfo_revisions(...));
>>>
>>> Shouldn't we at least emit a warning here?
>>
>> Yes and no. I still have the patch to do so, but I wanted to commit this
>> simple version first in any case, as it is considerably easier to back-port.
>>
>> The interesting thing is that this code is only executed on a partial dump,
>> where start revision is greater than 1, and only in order to be able to emit
>> other warnings. It's not even executed at all during 'verify'. There is
>> still merit in just adding a warning here, but it could almost be seen as
>> misleading to warn in some cases yet fail to warn during full dumps and
>> during verify. It seems to me it would be much better to parse the
>> mergeinfo and warn if it's bad in all dumps and especially also during
>> 'verify'. What do you think?
>
> AIUI, the warning really does apply only to partial dumps; since
> it's not only checking for r0 buit for any mergeinfo that refers to
> revisions outside the dump range.
>
> So we're really talking about two different cases, right?
We're talking about two different warnings.
There is a warning about mergeinfo referring to pre-dump revisions. This is only meaningful, and only checked for, when dumping a range of revisions starting after r1.
There is now the potential to add a new warning that says, "While trying to examine the mergeinfo in order to warn you about references to pre-dump revisions, we failed to parse the mergeinfo, so we can't warn you about pre-dump revisions, but you might want to know that we failed to parse the mergeinfo."
I don't want us to do just that, because I think it would lead some users to believe that we warn about invalid mergeinfo when in fact we would only do so *sometimes*. I think that would lead to confusion and dissatisfaction when they eventually discover that a full dump doesn't warn and a verify doesn't warn.
I think it would be better if we always try to parse the mergeinfo during every 'dump' and during every 'verify', and always warn (on dump) or error (on verify) if it's bad. (We would leave the first warning about references to pre-dump revisions just as it is: only for 'dump' and only when first rev > 1.)
- Julian
Re: svn commit: r1574868 - dump with r0 mergeinfo
Posted by Branko Čibej <br...@wandisco.com>.
On 10.03.2014 12:11, Julian Foad wrote:
> [switching back to plain text]
>
> Branko Čibej wrote:
>>> + /* An error in verifying the mergeinfo must not prevent dumping
>>> + the data. Ignore any such error. */
>>> + svn_error_clear(verify_mergeinfo_revisions(
>>> + eb->found_old_mergeinfo,
>>> + mergeinfo_str->data, eb->oldest_dumped_rev,
>>> + eb->notify_func, eb->notify_baton,
>>> + pool));
>> Shouldn't we at least emit a warning here?
> Yes and no. I still have the patch to do so, but I wanted to commit this simple version first in any case, as it is considerably easier to back-port.
>
> The interesting thing is that this code is only executed on a partial dump, where start revision is greater than 1, and only in order to be able to emit other warnings. It's not even executed at all during 'verify'. There is still merit in just adding a warning here, but it could almost be seen as misleading to warn in some cases yet fail to warn during full dumps and during verify. It seems to me it would be much better to parse the mergeinfo and warn if it's bad in all dumps and especially also during 'verify'.
>
> What do you think?
AIUI, the warning really does apply only to partial dumps; since it's
not only checking for r0 buit for any mergeinfo that refers to revisions
outside the dump range.
So we're really talking about two different cases, right?
-- Brane
--
Branko Čibej | Director of Subversion
WANdisco // Non-Stop Data
e. brane@wandisco.com
Re: svn commit: r1574868 - dump with r0 mergeinfo
Posted by Julian Foad <ju...@btopenworld.com>.
[switching back to plain text]
Branko Čibej wrote:
>> + /* An error in verifying the mergeinfo must not prevent dumping
>> + the data. Ignore any such error. */
>> + svn_error_clear(verify_mergeinfo_revisions(
>> + eb->found_old_mergeinfo,
>> + mergeinfo_str->data, eb->oldest_dumped_rev,
>> + eb->notify_func, eb->notify_baton,
>> + pool));
>
> Shouldn't we at least emit a warning here?
Yes and no. I still have the patch to do so, but I wanted to commit this simple version first in any case, as it is considerably easier to back-port.
The interesting thing is that this code is only executed on a partial dump, where start revision is greater than 1, and only in order to be able to emit other warnings. It's not even executed at all during 'verify'. There is still merit in just adding a warning here, but it could almost be seen as misleading to warn in some cases yet fail to warn during full dumps and during verify. It seems to me it would be much better to parse the mergeinfo and warn if it's bad in all dumps and especially also during 'verify'.
What do you think?
- Julian