On Tue, 2010-11-16, Greg Stein wrote:
> 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".
Quting myself: "My goals: to be able to understand the callers, and then
adapt them to op-depth semantics and also simplify them."
To expand: I may not be the brightest kid on the block but I find that
with hours of effort I still can't see precisely what many of these
functions are *trying* to do. (Obviously I can see precisely what they
*are* doing in terms of shuffling bits around.) I am extremely
frustrated by my slow progress in extending WC code. Something must be
wrong: the desired behaviour is complex but not *that* complex.
So I am seeking ways to make it more understandable and easier to work
with. Basically ways to tie the embedded concepts more obviously and
directly to the function names, inputs and outputs. To help myself, and
because I am certain it will help others too.
If that's too hand-wavey, any of the uses enumerated as 2, 4, 5, 7, 8
below are good concrete examples of the phenomenon.
- Julian
> 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-17 11:38:56 CET