On Wed, Oct 29, 2008 at 9:46 AM, Stephen Butler <sbutler_at_elego.de> wrote:
> Quoting Greg Stein <gstein_at_gmail.com>:
>
>> 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.
>
> Maybe I've been working in the update-editor mine too long and lost
> perspective...
Um... is it? or not? If it is a straightforward function, then ignore
me. But given the lengthy parameter list, I'm guessing "long", so that
was my question to you.
>> 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(...) ?
>
> Yes. Actually, it already is. Doh!
Okay. So it *is*. Does that mean you're going to keep it that way, or
are you proposing to mush the two together? Not sure what your answer
meant.
>>> * 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.
>
> We could pull out the code that writes the tree conflict data to the
> parent dir's entry. But if we find a new tree conflict, we always
> write it.
Hunh? I mean the code that puts stuff into LOG_ACCUM. Can we move that
code out of this function, thereby not need to take a LOG_ACCUM
parameter? And thus also making this file a *check* rather than a
*modify*.
> On a related note, we expect the caller to flush and run the log
> (for update and switch).
Assumed so, yeah.
>>> * 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.
>
> Actually it is. It's a special case where Subversion automatically
> resolves a potential tree conflict without raising it.
Alrighty. And I presume the caller needs to know this for some reason.
Would the argument better be named something like ADD_CONFLICT_CLEARED
? Or whatever. It seems to suggest that a conflict should be
*cleared*. Sure, there was an add, but what is the *actual* semantic
of what happened or should happen?
>>> * 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.
>
> Let's see, from the edit baton we need these members for the tree
> conflict check:
>
> allow_unver_obstructions
> cancel_func
> cancel_baton
>
> And one more for the tree conflict persistence:
>
> switch_url (just a boolean if-null check)
>
> Not so many after all.
Then I might suggest a couple booleans, and a cancel func/baton pair.
Part of the problem in update_editor is the prevelent use of the
batons as a pseudo-global variable space. As a result, data flow and
coupling is near-impossible to track down. I just spent a great number
of days unwinding some of that around fb->new_text_base_path. After
unwinding all of that, plus some previous work, I turned *four* passes
over a file into *one*. We're pretty inefficient :-(
>>> * 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.
>
> It's always the parent dir of the potential tree conflict victim.
Okay. In pseudo-code: get_access_baton(get_dirname(full_path)).
> Should each function retrieve this dir's access baton on its own?
> Or is it better to pass around the same baton, which the caller
> can use later (if there's no conflict and the update proceeds)?
Doesn't really matter. The access batons can quickly translate between
all affected directories. So if you have one, then you have all of
them.
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 20:10:06 CET