Stefan Sperling wrote:
> On Thu, Jul 30, 2009 at 07:15:04PM +0100, Julian Foad wrote:
> > Stefan Sperling wrote:
> > > OK. Note that Edmund Wong has been working on this issue, too,
> > > and I told him to do the stat gathering in libsvn_client.
> > > Maybe that is why he hasn't made much progress, given that it
> > > makes the task more difficult -- I gave him bad advice :(
> >
> > Oh... I didn't remember that.
> >
> > The information coming up from libsvn_client is "here are notifications
> > of all the things happening". I think it would be wrong to make
> > libsvn_client say "and also here is some summary information about those
> > notifications, classified in a particular way".
>
> The way I saw it was that notification callbacks exist to print
> notifications. They don't exist to keep track of what happened
> during the entire operation.
Hi Stefan and Edmund. Sorry I didn't join in when you were discussing
this the first time.
The more I think about it, the less I think the libsvn_wc or even
libsvn_client should be involved in the summarizing at all.
I think the notifications exist to notify the client what's happening.
The library's job is to provide information; the client's job is to
decide what to do with that information. The client can choose to print
them if it wants to, and/or it can advance a progress bar, and/or it can
record and categorise them and maybe display a summary later, or
whatever it wants to do with the info. The client is perfectly entitled
to use them to create a record of what's happened during the entire
operation, and I would expect GUI clients to do so. In effect, CLI
clients create such a record too, implicitly, by streaming the list to
stdout.
The notifications are intended to be complete and accurate and are the
primary means of reporting what is happening. We don't expect, for
example, that the library should be able to generate more accurate
statistics internally because certain conflicts are not notified but
should be counted, or because some other conflicts are notified but
should not be counted.
> That's what the batons are for,
> and there is a baton of type svn_wc_traversal_info_t which has
> precisely the scope we need.
It's a technically possible solution.
But summarizing the reported info is a very high-level task. The
low-level libraries already provide the necessary information. It
doesn't seem right to me for a library to provide information and then
provide a particular kind of summary of that information. (In other
situations, it can make sense for a library to provide a summary of
something in advance, if it's then optional for the client to request
the full info and expend significantly more time or resources in doing
so. But that's not what we're talking about.)
> I'm quoting a mail I sent to Edmund about it below.
I know some text in that email might be outdated by now, but there's one
comment I'll make that's still relevant.
> I agree that the approach you suggested is probably simpler, though.
>
> Stefan
>
> ====
>
> So, what we'll do is:
>
> 1) We'll move the conflict counters (see struct notify_baton in
> svn/notify.c) from the svn client's notification baton into the
> svn_wc_notify_t struct.
>
> 2) We introduce a new notification action. Right now, we have
> svn_wc_notify_update_completed, but that is called for each
> update target individually (see svn_client__update_internal
> in libsvn_client/update.c). Rather, we want a notification that
> says "the whole operation has completed and here are the combined
> stats I have collected for you for every target."
The problem there is a case of poor layering. The libsvn_client is
passing (multiple) WC-layer "update completed" notifications straight
through to the client even though the client shouldn't know anything
about how it's driving the WC layer multiple times to accomplish a
single task.
That's a separate issure that we should fix. Libsvn_client could trap
those and send its own (single) "update completed" notification instead.
(We could either keep using the WC-layer notifier data type or create a
client-layer notifier type.)
- Julian
> So we could add a new action like svn_wc_notify_conflict_stats.
> This notification should be issued by svn_client_update3(), after
> processing all the targets (see the "for (i = 0; i < paths->nelts;
> ++i)" loop in svn_client_update3()). When the notification is made,
> the stats the library collected during the operation are added
> to the new counters we added to svn_wc_notify_t in step 1), and
> passed to the client. The client can then read the stats.
>
> I'll leave it to you to figure out where to issue the notification
> for merge. But note that it would suffice for now to just focus on
> update. Once that is solved, solving the problem again for merge
> should be fairly easy.
>
> 3) During update, and merge, whenever the client library notifies
> the client about a conflict, we increment the conflict stats.
>
> The stats need to be stored somewhere with sufficient lifetime
> so that they survive the whole operation. Local variables in
> svn_client_update3() (and the equivalent for merge) might be
> a good choice for storage. However, local variables aren't that
> useful for gathering the stats, because the stats will be gathered
> way down in the call chain (in libsvn_wc/update-editor.c, for update).
>
> There is this interesting thing called svn_wc_traversal_info_t,
> which is used during an update to gather and summarise information
> about externals. It could easily be extended to gather conflict
> stats, too. We'll just need to add conflict counters to it.
>
> The traversal info is allocated in svn_client__update_internal(),
> and passed down the call chain, until it eventually ends up
> in the edit_baton of libsvn_wc/update-editor.c. This is the
> perfect spot to collect conflict stats, because we also issue
> notifications from there.
>
> Because svn_client__update_internal() operates on a single target,
> we can use a traversal info to gather stats for one target only.
> But that's fine, and no worse than the current situation.
> In addition, we could pass pointers to the local variables we
> have in svn_client_update3() down to svn_client__update_internal(),
> which could then add the per-target stats to the overall stats
> maintained in the local variables of svn_client_update3().
>
> So this is the high-level description of the process for update.
>
> For merge, things might end up being a bit different, because most
> of the merge logic is in libsvn_client/merge.c, which also handles
> notifications. We won't need a traversal info, for example.
> Instead, we can tweak any non-public functions inside libsvn_client/merge.c
> to our liking so that we can gather conflict stats.
> But again, it's enough if you focus on update, first.
>
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2377314
Received on 2009-07-31 12:20:21 CEST