You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Stephen Butler <sb...@elego.de> on 2008/10/29 17:17:19 UTC
Re: svn commit: r33944 - in branches/tc-merge-notify/subversion:
include libsvn_client libsvn_wc
Quoting "Neels J. Hofmeyr" <ne...@elego.de>:
> Hey tree-conflicts folks,
>
> please review and/or jump right in and fix stuff.
>
> Thanks!
> ~Neels
>
> neels@tigris.org wrote:
>> Author: neels
>> Date: Tue Oct 28 23:02:46 2008
>> New Revision: 33944
>>
>> Log:
>> On lightweight branch tc-merge-notify:
>>
>> Start off implementing per-victim notification of tree-conflicts
>> during merge.
Hi Neels,
The changes to merge.c and diff.c look good to me, on a quick
overview.
But I think there's some extra changes needed to support
skipping the victims. We need to separate tree conflict
notifications from all of the others.
I propose that we remove the (new) tree_conflicted field from
svn_wc_notify_t, and add one or more notify-actions instead.
At the same time, change the notify() function in the client to
accept the new tree conflict notifications.
Specifically, in notify(), we should add a couple of new cases,
and remove the 'C' in the fourth column from all other cases
that have it. E.g.:
switch (n->action)
{
case svn_wc_notify_tree_conflict:
nb->in_external ? nb->ext_tree_conflicts++
: nb->tree_conflicts++;
if ((err = svn_cmdline_printf(pool, " C %s\n", path_local)))
goto print_error;
break;
case svn_wc_notify_conflict_skip:
nb->in_external ? nb->ext_skipped_paths++
: nb->skipped_paths++;
if ((err = svn_cmdline_printf
(pool, _("Skipped conflicted path '%s'\n"), path_local)))
goto print_error;
break;
The current code (for update, too) mixes tree conflict output
with other output.
This will affect repos_diff.c. Callbacks like the following
should do tree conflict notification earlier, before skipping
the rest of the callback (including the notify code below).
The notify code at the end of the repos_diff callbacks
shouldn't set the tree_conflicted field, because it won't
be in svn_wc_notify_t anymore.
>> + notify->tree_conflicted = b->tree_conflicted;
Comments?
Steve
==============================================================================
>> ---
>> branches/tc-merge-notify/subversion/libsvn_client/repos_diff.c Tue
>> Oct 28 22:17:24 2008 (r33943)
>> +++
>> branches/tc-merge-notify/subversion/libsvn_client/repos_diff.c Tue
>> Oct 28 23:02:46 2008 (r33944)
[...]
>> @@ -566,10 +585,11 @@ add_directory(const char *path,
>> }
>> else
>> new_action = kas->action;
>> - notify = svn_wc_create_notify(b->wcpath, new_action, pool);
>> + notify = svn_wc_create_notify(b->wcpath, new_action, pool);
>> notify->kind = kas->kind;
>> notify->content_state = notify->prop_state = kas->state;
>> notify->lock_state = svn_wc_notify_lock_state_inapplicable;
>> + notify->tree_conflicted = kas->tree_conflicted;
>> (*eb->notify_func)(eb->notify_baton, notify, pool);
>> apr_hash_set(eb->deleted_paths, b->wcpath,
>> APR_HASH_KEY_STRING, NULL);
>> @@ -579,6 +599,7 @@ add_directory(const char *path,
>> {
>> notify = svn_wc_create_notify(b->wcpath, action, pool);
>> notify->kind = svn_node_dir;
>> + notify->tree_conflicted = b->tree_conflicted;
>> (*eb->notify_func)(eb->notify_baton, notify, pool);
>> }
>> }
--
Stephen Butler | Software Developer
elego Software Solutions GmbH
Gustav-Meyer-Allee 25 | 13355 Berlin | Germany
fon: +49 30 2345 8696 | mobile: +49 163 25 45 015
fax: +49 30 2345 8695 | http://www.elegosoft.com
Geschäftsführer: Olaf Wagner | Sitz der Gesellschaft: Berlin
Amtsgericht Charlottenburg HRB 77719 | USt-IdNr: DE163214194
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r33944 - in branches/tc-merge-notify/subversion:
include libsvn_client libsvn_wc
Posted by Neels J Hofmeyr <ne...@elego.de>.
> Quoting Julian Foad <ju...@btopenworld.com>:
>
>> On Thu, 2008-10-30 at 03:10 +0100, Neels J. Hofmeyr wrote:
>>>
>>> Stephen Butler wrote:
>>> > I propose that we remove the (new) tree_conflicted field from
>>> > svn_wc_notify_t, and add one or more notify-actions instead.
>>> > At the same time, change the notify() function in the client to
>>> > accept the new tree conflict notifications.
>>>
>>> Hey, that's actually a very good idea. I did this thinking that it
>>> would be
>>> good to have the usual notification alongside the conflict. But that is
>>> better solved with two new columns, as we discussed elsewhere.
As discussed, I have a patch simmering that, in svn_wc_notify_t, changes the
separate boolean TREE_CONFLICTED to a new svn_wc_notify_action_t called
svn_wc_notify_tree_conflicted.
I was hoping get it through quickly but it's bedtime now and it so far only
breaks everything. Haven't investigated thoroughly why that is so, yet.
>> There is already a "skip" notification defined:
>>
>> /** The type of action occurring. */
>> typedef enum svn_wc_notify_action_t
>> {
>> [...]
>> /** Skipping a path. */
>> svn_wc_notify_skip,
>> [...]
>>
>> Shouldn't we be using svn_wc_notify_skip as the "action occurring", and
>> set the "content_state" to "conflicted" or the "tree_conflicted" flag to
>> true to indicate that the reason is a conflict?
Hmm... I overlooked this. We don't have consensus on using the action to
notify a tree-conflict? Well, it doesn't make much difference really. But it
looks so far as if using the action fits pretty well and avoids some
conditions going true even though nothing happened as they expected
(comparing action to *_add to record something in a list about added items).
We could adapt the conditions and stuff, but it looks like most stuff
becomes simpler with a separate svn_wc_notify_tree_conflict action.
~Neels
Re: svn commit: r33944 - in branches/tc-merge-notify/subversion:
include libsvn_client libsvn_wc
Posted by Stephen Butler <sb...@elego.de>.
Quoting Julian Foad <ju...@btopenworld.com>:
> On Thu, 2008-10-30 at 03:10 +0100, Neels J. Hofmeyr wrote:
>>
>> Stephen Butler wrote:
>> > Quoting "Neels J. Hofmeyr" <ne...@elego.de>:
>> >
>> >> Hey tree-conflicts folks,
>> >>
>> >> please review and/or jump right in and fix stuff.
>> >>
>> >> Thanks!
>> >> ~Neels
>> [...]
>> > But I think there's some extra changes needed to support
>> > skipping the victims. We need to separate tree conflict
>> > notifications from all of the others.
>> >
>> > I propose that we remove the (new) tree_conflicted field from
>> > svn_wc_notify_t, and add one or more notify-actions instead.
>> > At the same time, change the notify() function in the client to
>> > accept the new tree conflict notifications.
>>
>> Hey, that's actually a very good idea. I did this thinking that it would be
>> good to have the usual notification alongside the conflict. But that is
>> better solved with two new columns, as we discussed elsewhere.
>> > Comments?
>>
>> About this
>> > case svn_wc_notify_conflict_skip:
>> that prints
>> > (pool, _("Skipped conflicted path '%s'\n"), path_local)))
>>
>> Where is this going to be used, exactly? For persisting tree-conflicts? Not
>> for nodes inside a newly tree-conflicted directory, I presume.
That's correct. We'll be silent inside a tree conflicted dir that
we've already notified the user about.
>
> There is already a "skip" notification defined:
>
> /** The type of action occurring. */
> typedef enum svn_wc_notify_action_t
> {
> [...]
> /** Skipping a path. */
> svn_wc_notify_skip,
> [...]
>
> Shouldn't we be using svn_wc_notify_skip as the "action occurring", and
> set the "content_state" to "conflicted" or the "tree_conflicted" flag to
> true to indicate that the reason is a conflict?
Yes, it would be clearer to have just one skip action. BTW we
should get rid of the "Skipped missing target" output, because
that will always be a tree conflict.
Steve
--
Stephen Butler | Software Developer
elego Software Solutions GmbH
Gustav-Meyer-Allee 25 | 13355 Berlin | Germany
fon: +49 30 2345 8696 | mobile: +49 163 25 45 015
fax: +49 30 2345 8695 | http://www.elegosoft.com
Geschäftsführer: Olaf Wagner | Sitz der Gesellschaft: Berlin
Amtsgericht Charlottenburg HRB 77719 | USt-IdNr: DE163214194
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r33944 - in branches/tc-merge-notify/subversion:
include libsvn_client libsvn_wc
Posted by Julian Foad <ju...@btopenworld.com>.
On Thu, 2008-10-30 at 03:10 +0100, Neels J. Hofmeyr wrote:
>
> Stephen Butler wrote:
> > Quoting "Neels J. Hofmeyr" <ne...@elego.de>:
> >
> >> Hey tree-conflicts folks,
> >>
> >> please review and/or jump right in and fix stuff.
> >>
> >> Thanks!
> >> ~Neels
> [...]
> > But I think there's some extra changes needed to support
> > skipping the victims. We need to separate tree conflict
> > notifications from all of the others.
> >
> > I propose that we remove the (new) tree_conflicted field from
> > svn_wc_notify_t, and add one or more notify-actions instead.
> > At the same time, change the notify() function in the client to
> > accept the new tree conflict notifications.
>
> Hey, that's actually a very good idea. I did this thinking that it would be
> good to have the usual notification alongside the conflict. But that is
> better solved with two new columns, as we discussed elsewhere.
>
> >
> > Specifically, in notify(), we should add a couple of new cases,
> > and remove the 'C' in the fourth column from all other cases
> > that have it. E.g.:
> >
> > switch (n->action)
> > {
> > case svn_wc_notify_tree_conflict:
> > nb->in_external ? nb->ext_tree_conflicts++
> > : nb->tree_conflicts++;
> > if ((err = svn_cmdline_printf(pool, " C %s\n", path_local)))
> > goto print_error;
> > break;
> >
> > case svn_wc_notify_conflict_skip:
> > nb->in_external ? nb->ext_skipped_paths++
> > : nb->skipped_paths++;
> > if ((err = svn_cmdline_printf
> > (pool, _("Skipped conflicted path '%s'\n"), path_local)))
> > goto print_error;
> > break;
> >
> >
> > The current code (for update, too) mixes tree conflict output
> > with other output.
> >
> > This will affect repos_diff.c. Callbacks like the following
> > should do tree conflict notification earlier, before skipping
> > the rest of the callback (including the notify code below).
> >
> > The notify code at the end of the repos_diff callbacks
> > shouldn't set the tree_conflicted field, because it won't
> > be in svn_wc_notify_t anymore.
> >
> > >> + notify->tree_conflicted = b->tree_conflicted;
>
> Huh? Where is that?
> Oh, you mean when removing that field. Naturally.
>
> >
> > Comments?
>
> About this
> > case svn_wc_notify_conflict_skip:
> that prints
> > (pool, _("Skipped conflicted path '%s'\n"), path_local)))
>
> Where is this going to be used, exactly? For persisting tree-conflicts? Not
> for nodes inside a newly tree-conflicted directory, I presume.
There is already a "skip" notification defined:
/** The type of action occurring. */
typedef enum svn_wc_notify_action_t
{
[...]
/** Skipping a path. */
svn_wc_notify_skip,
[...]
Shouldn't we be using svn_wc_notify_skip as the "action occurring", and
set the "content_state" to "conflicted" or the "tree_conflicted" flag to
true to indicate that the reason is a conflict?
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r33944 - in branches/tc-merge-notify/subversion:
include libsvn_client libsvn_wc
Posted by "Neels J. Hofmeyr" <ne...@elego.de>.
Stephen Butler wrote:
> Quoting "Neels J. Hofmeyr" <ne...@elego.de>:
>
>> Hey tree-conflicts folks,
>>
>> please review and/or jump right in and fix stuff.
>>
>> Thanks!
>> ~Neels
[...]
> But I think there's some extra changes needed to support
> skipping the victims. We need to separate tree conflict
> notifications from all of the others.
>
> I propose that we remove the (new) tree_conflicted field from
> svn_wc_notify_t, and add one or more notify-actions instead.
> At the same time, change the notify() function in the client to
> accept the new tree conflict notifications.
Hey, that's actually a very good idea. I did this thinking that it would be
good to have the usual notification alongside the conflict. But that is
better solved with two new columns, as we discussed elsewhere.
>
> Specifically, in notify(), we should add a couple of new cases,
> and remove the 'C' in the fourth column from all other cases
> that have it. E.g.:
>
> switch (n->action)
> {
> case svn_wc_notify_tree_conflict:
> nb->in_external ? nb->ext_tree_conflicts++
> : nb->tree_conflicts++;
> if ((err = svn_cmdline_printf(pool, " C %s\n", path_local)))
> goto print_error;
> break;
>
> case svn_wc_notify_conflict_skip:
> nb->in_external ? nb->ext_skipped_paths++
> : nb->skipped_paths++;
> if ((err = svn_cmdline_printf
> (pool, _("Skipped conflicted path '%s'\n"), path_local)))
> goto print_error;
> break;
>
>
> The current code (for update, too) mixes tree conflict output
> with other output.
>
> This will affect repos_diff.c. Callbacks like the following
> should do tree conflict notification earlier, before skipping
> the rest of the callback (including the notify code below).
>
> The notify code at the end of the repos_diff callbacks
> shouldn't set the tree_conflicted field, because it won't
> be in svn_wc_notify_t anymore.
>
> >> + notify->tree_conflicted = b->tree_conflicted;
Huh? Where is that?
Oh, you mean when removing that field. Naturally.
>
> Comments?
About this
> case svn_wc_notify_conflict_skip:
that prints
> (pool, _("Skipped conflicted path '%s'\n"), path_local)))
Where is this going to be used, exactly? For persisting tree-conflicts? Not
for nodes inside a newly tree-conflicted directory, I presume.
~Neels