[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: scan_deletion - why?

From: Greg Stein <gstein_at_gmail.com>
Date: Tue, 16 Nov 2010 17:59:18 -0500

I'm not sure that I understand the goal here. Is there a problem that
is trying to be solved? If not, then I'd recommend just marking this
down as an item to "review in 1.8".

Cheers,
-g

On Tue, Nov 16, 2010 at 12:51, Julian Foad <julian.foad_at_wandisco.com> wrote:
> I'm auditing the uses of scan_deletion() and its semi-public wrapper
> svn_wc__db_scan_deletion(): what do its callers really want?  My goals:
> to be able to understand the callers, and then adapt them to op-depth
> semantics and also simplify them.  It seems to me there's far too much
> obfuscation going on in these callers at present, with lots of
> near-but-not-quite-duplication.
>
> Here are all the uses and what I can understand about them:
>
> 1. adm_crawler.c:find_base_rev()
>  -> work_del_abspath
>  This code path is unused in a test suite run.
>
> 2. entries.c:get_base_info_for_deleted() (first occurrence)
>  -> work_del_abspath
>  Use of result:
>    read_info(dirname(work_del_abspath))+scan_add -> repo+relpath
>  Ultimate need:
>    Copyfrom repo+relpath of the deleted node.
>    (The repo+relpath that this path belonged to before it was
>    deleted; if this deletion is inside a move/copy, then the repo
>    location within the copy source.)
>
> 3. entries.c:get_base_info_for_deleted() (second occurrence)
>  -> base_replaced, work_del_abspath
>  Use of result:
>    base_replaced,
>    read_info(dirname(work_del_abspath))+scan_add -> status
>  Ultimate need:
>    Status indications for constructing an entry_t.
>
> 4. node.c:svn_wc__node_get_working_rev_info()
>  -> base_del_abspath, work_del_abspath
>  This function is only used for back-compat in status reporting.
>
> 5. node.c:svn_wc__node_get_commit_base_rev()
>  -> work_del_abspath
>  Use of result:
>    read_info(dirname(work_del_abspath))+scan_add -> original_rev
>    assert(status is some kind of 'added')
>  Ultimate need:
>    The "commit base rev" of the deleted node, which probably
>    really means the repository revision of the parent dir of the
>    deletion root.  See doc string - "should go away".
>
> 6. node.c:svn_wc__internal_node_get_schedule()
>  -> work_del_abspath
>  Use of result:
>    work_del_abspath == NULL?
>  Ultimate need:
>    To know whether this deletion is inside a copy/move.
>
> 7. wc_db.c:get_info_for_copy()
>  -> work_del_relpath
>  Use of result:
>    scan_add(dirname(work_del_relpath)) -> original_repos_*, op_root
>  Ultimate aim:
>    Copyfrom repos+relpath+rev of the deleted node.
>    ### Relpath of this node, or of root of the copy?
>
> 8. wc_db.c:svn_wc__db_global_relocate()
>  -> work_del_relpath
>  Use of result:
>    dirname(work_del_relpath)
>  Ultimate aim:
>    Repos+relpath and local path of the parent of the root of the
>    deletion.
>
> 9. db-test.c:test_scan_deletion()
>  Several calls, each checking all the output parameters.
>
>
> So what to do with this?  Work out what semantics the callers really
> want, and provide them through one or more APIs.  In each case, return
> the wanted data in a single call instead of providing a path.  Then
> we'll be in a position where we can optimize the queries inside each
> such function if we need to.
>
> 1. Seems unused, so ignore for now.  I'm not confident that it can't be
> triggered.  I hope this whole private function can be removed and some
> (semi-public) API used instead.
>
> 2, 5, 7, 8. These are all similar and want to know the repos location of
> "where the deleted node would have been".  Need to check carefully what
> relpath these want: the relpath of the given path, or of the deletion
> root, or of the parent of the deletion root.  Also the revnum - should
> it be the revnum that the node was at before it was deleted, or the
> revnum that its parent is at?  They can differ in a mixed-rev base or in
> a copy of a mixed-rev base.
>
> 3. Wants status of parent of deletion root.
>
> 4. The function for back-compatibility in "status" reporting.  The old,
> deprecated version of the public status API should retrieve the
> back-compat fields natively, rather than having the client make a
> separate call.  The newer, revved status API can be non-compatible and
> avoid that overhead.
>
> 6. "Is this deletion inside a copy/move?"  This usage is not a problem
> and can continue to use scan_deletion or one of the functions that may
> replace it.
>
> 9. Tests: amend as appropriate.
>
> The "moved_to_abspath" output is never used.  Ditch it?
>
> The "base_replaced" output is only used in case 3.  Ditch it from the
> main API and use something else in this case?
>
> The "base_del_abspath" output is only used in case 4.  Ditch it from the
> main API and use something else in this case?
>
> - Julian
>
>
>
>
>
Received on 2010-11-16 23:59:54 CET

This is an archived mail posted to the Subversion Dev mailing list.