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.f...@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 > > > > >
