You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Bert Huijben <be...@qqmail.nl> on 2022/09/09 10:30:52 UTC
Re: svn commit: r1903814 - /subversion/branches/issue-4908/subversion/libsvn_wc/deprecated.c
Why the branch?
Looks like a quite obvious fix to me, so you could have just committed this
to trunk.
+1 on backporting this fix, although I would recommend users of the old
(pre 1.7) api to upgrade to the newer status apis, as that makes an insane
amount of performance difference on most platforms.
Bert
On Thu, Sep 1, 2022 at 4:10 PM <ha...@apache.org> wrote:
> Author: hartmannathan
> Date: Thu Sep 1 14:10:23 2022
> New Revision: 1903814
>
> URL: http://svn.apache.org/viewvc?rev=1903814&view=rev
> Log:
> On the 'issue-4908' branch, add the proposed fix
>
> * subversion/libsvn_wc/deprecated.c
> (svn_wc__status2_from_3): To allow users of deprecated APIs, such as
> PySVN,
> to detect working copy locked status, copy 'locked' from old_status to
> status.
>
> Modified:
> subversion/branches/issue-4908/subversion/libsvn_wc/deprecated.c
>
> Modified: subversion/branches/issue-4908/subversion/libsvn_wc/deprecated.c
> URL:
> http://svn.apache.org/viewvc/subversion/branches/issue-4908/subversion/libsvn_wc/deprecated.c?rev=1903814&r1=1903813&r2=1903814&view=diff
>
> ==============================================================================
> --- subversion/branches/issue-4908/subversion/libsvn_wc/deprecated.c
> (original)
> +++ subversion/branches/issue-4908/subversion/libsvn_wc/deprecated.c Thu
> Sep 1 14:10:23 2022
> @@ -2815,6 +2815,7 @@ svn_wc__status2_from_3(svn_wc_status2_t
> }
>
> (*status)->entry = entry;
> + (*status)->locked = old_status->locked;
> (*status)->copied = old_status->copied;
> (*status)->repos_lock = svn_lock_dup(old_status->repos_lock,
> result_pool);
>
>
>
>
Re: svn commit: r1903814 - /subversion/branches/issue-4908/subversion/libsvn_wc/deprecated.c
Posted by Nathan Hartman <ha...@gmail.com>.
On Wed, Sep 21, 2022 at 5:51 AM Bert Huijben <be...@qqmail.nl> wrote:
>
> File externals didn't exist in the entry world, so you can ignore that edge case. Older code will just detect them as a switched path which is fine, as that is essentially what they are anyway.
>
> Normal (directory) externals should be mapped, but I assume they already are or we would have found these earlier on in the WC-NG migration.
>
> The lock flag is seldom tested, nor seen from testcode as it would imply the working copy is broken by some operation.... If we have such a testcase, we usually fix the real issue instead of increasing test coverage.
> (The issue was far more common pre WC-NG, where we didn't have true recursive operations... so we had to lock on many levels separately)
Thank you for this explanation. Knowing this, I am OK to commit the
'locked' fix without trying to add a specific test case for it.
Committed in r1904193.
Nominated for backport, with your vote, in 1904194.
I'll leave the file_external flag alone for now.
Cheers,
Nathan
Re: svn commit: r1903814 - /subversion/branches/issue-4908/subversion/libsvn_wc/deprecated.c
Posted by Bert Huijben <be...@qqmail.nl>.
File externals didn't exist in the entry world, so you can ignore that edge
case. Older code will just detect them as a switched path which is fine, as
that is essentially what they are anyway.
Normal (directory) externals should be mapped, but I assume they already
are or we would have found these earlier on in the WC-NG migration.
The lock flag is seldom tested, nor seen from testcode as it would imply
the working copy is broken by some operation.... If we have such a
testcase, we usually fix the real issue instead of increasing test coverage.
(The issue was far more common pre WC-NG, where we didn't have true
recursive operations... so we had to lock on many levels separately)
Bert
On Fri, Sep 9, 2022 at 10:46 PM Nathan Hartman <ha...@gmail.com>
wrote:
> On Fri, Sep 9, 2022 at 6:31 AM Bert Huijben <be...@qqmail.nl> wrote:
> >
> > Why the branch?
> > Looks like a quite obvious fix to me, so you could have just committed
> this to trunk.
>
> That does seem a little excessive, doesn't it? :-)
>
> I'll merge and nominate for backport when I get to a computer.
>
> I wanted to write a regression test too, but got stuck there -- was
> looking for the right place to add it and something that already
> exercises that version API to use as a starting point.
>
> The test suite (without a new regression test) didn't throw up any
> complaints.
>
> Btw, while looking at this, I found we also aren't copying the
> 'external' flag (or file_external, I don't have the name handy at the
> moment), so I'll fix that too.
>
> AFAICT we're copying/translating everything else that has a
> representation in the older struct. So, just the lock and external
> flags are missing.
>
> > +1 on backporting this fix, although I would recommend users of the old
> (pre 1.7) api to upgrade to the newer status apis, as that makes an insane
> amount of performance difference on most platforms.
>
> Cool. I'll add your vote when I nominate it...
>
> Cheers,
> Nathan
>
Re: svn commit: r1903814 - /subversion/branches/issue-4908/subversion/libsvn_wc/deprecated.c
Posted by Nathan Hartman <ha...@gmail.com>.
On Fri, Sep 9, 2022 at 6:31 AM Bert Huijben <be...@qqmail.nl> wrote:
>
> Why the branch?
> Looks like a quite obvious fix to me, so you could have just committed this to trunk.
That does seem a little excessive, doesn't it? :-)
I'll merge and nominate for backport when I get to a computer.
I wanted to write a regression test too, but got stuck there -- was
looking for the right place to add it and something that already
exercises that version API to use as a starting point.
The test suite (without a new regression test) didn't throw up any complaints.
Btw, while looking at this, I found we also aren't copying the
'external' flag (or file_external, I don't have the name handy at the
moment), so I'll fix that too.
AFAICT we're copying/translating everything else that has a
representation in the older struct. So, just the lock and external
flags are missing.
> +1 on backporting this fix, although I would recommend users of the old (pre 1.7) api to upgrade to the newer status apis, as that makes an insane amount of performance difference on most platforms.
Cool. I'll add your vote when I nominate it...
Cheers,
Nathan