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