Re: svn commit: r1903814 - /subversion/branches/issue-4908/subversion/libsvn_wc/deprecated.c

2022-09-21 Thread Nathan Hartman
On Wed, Sep 21, 2022 at 5:51 AM Bert Huijben  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

2022-09-21 Thread Bert Huijben
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 
wrote:

> On Fri, Sep 9, 2022 at 6:31 AM Bert Huijben  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
>