On Wed, Oct 29, 2008 at 8:03 AM, Stephen Butler <sbutler_at_elego.de> wrote:
>...
> I'm hacking away at the skipping problem, too. For the allow-existing-add
> feature, I simply gave check_tree_conflicts() a ptr-to-bool arg. Here's
> the signature for that function in my incomplete patch:
How big and ugly is that function? update_editor.c has a ton of these
huge nasty functions. The code complexity is really bad.
I'm assuming this is similar, and so in that vein:
> /* First, check whether a significant conflict has already been raised
> * on FULL_PATH. For any incoming change ACTION, check for a tree
> * conflict. When adding an item, text and prop conflicts may also be
> * checked. If an existing conflict is found, return NULL in
> * *PCONFLICT and set *SKIP_P to TRUE.
Can this first check be broken out into a function, say,
covered_by_existing_conflict(...) ?
> *
> * Second, check whether the incoming change ACTION on FULL_PATH would
> * conflict with FULL_PATH's scheduled change. If so, then FULL_PATH
> * is a new tree conflict victim. Raise a persistent tree conflict by
> * appending log actions to LOG_ACCUM. Return a conflict description
> * in *PCONFLICT and set *SKIP_P to TRUE.
Rather than taking LOG_ACCUM, can this function instead rely on the
caller to perform log actions. IOW, only *check* for a conflict. Don't
take any actions on it.
> *
> * If no conflicts are found, return NULL in *PCONFLICT and set
> * *SKIP_P to FALSE.
> *
> * When adding a file or directory, the KIND is the kind of the item
> * to be added, and *ADD_EXISTED_P will be set to TRUE if the item is
> * already scheduled for add without history (which is not a
> * conflict). For other actions, KIND and ADD_EXISTED_P are ignored.
Can this be left to the caller? This doesn't seem to be related to
tree conflicts.
> *
> * The edit baton EB gives information including whether the operation is
> * an update or a switch.
How many pieces of EB are accessed? Having "the whole EB" can give
this function ugly powers. If you only need one or two parameters,
then I'd suggest passing those directly.
> *
> * PARENT_ADM_ACCESS is the admin access baton of FULL_PATH's parent
> * directory.
Parent? Why not pass the corresponding access baton, and if/when this
function needs it, then it can retrieve it itself. Maybe this is
nomenclature. Consider:
If kind==file, then is this baton, the directory that the file resides
in? Or the parent of *that* directory?
If kind==dir, then is this baton related to self, or is this the
parent of the directory?
Please clarify.
Cheers,
-g
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-10-29 17:12:42 CET