You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Paul Burba <pt...@gmail.com> on 2009/04/29 14:50:28 UTC
[PATCH]: Notify on mergeinfo changes made to describe the merge
Yesterday in IRC:
Apr 28 18:06:11 <Bert> pburba: Do you know what it would take to fix
notifications for merge info only changes on files while merging? (I'm
not sure if we would want it as output of svn, but I would really like
to have the knowledge of which file changed as a notification in
AnkhSVN (and I heard Mark would like to have the info too)
Apr 28 18:07:54 <pburba> Bert: I dont understand the question, you
want to suppress the notifications of property changes during a merge
if the property is svn:mergeinfo?
Apr 28 18:08:11 <pburba> files only?
Apr 28 18:13:34 <Bert> No.. I want to know exactly which files are
changed.. And I get reports of some files that receive svn:mergeinfo
changes but aren't notified as changed
Apr 28 18:14:30 <Bert> We use the notifications as hint to a status
cache.. and only call svn status on files that have changes
Apr 28 18:15:29 <Bert> (actually: We call it on the parent directory
with --depth files, because that is virtually for free performance
wise with WC-1.0.. but that is an implementation detail)
Apr 28 18:16:47 <pburba> Bert: Ok, I understand you now. You want
merge to notify that it is recording mergeinfo on a path describing
the merge when that path was not directly affected by the diff (and
thus not mentioned in the notifications). This type of thing can
happen due to elision, shallow merges, shallow, targets, switched
subtrees etc.
Apr 28 18:17:39 <pburba> Bert: The thing is, the merge code doesn't
record the mergeinfo describing the merge until after the merge (and
the notifications) are complete. So it would have to be something
tacked on to the merge output at the end.
Apr 28 18:18:19 <Bert> Or just a notification with another action-id,
so it wouldn't be notified by svn.. but is available to the bindings
Apr 28 18:18:58 <Bert> But I wouldn't mind when or how it happens.. I
just would like to have the paths of the files that changed :)
Apr 28 18:25:05 <pburba> Bert: Ah, ok. Well the place that mergeinfo
is recorded describing the merge is in
libsvn_client/merge.c:do_directory_merge(), see the comment ''/*
Record mergeinfo where appropriate.*/". (and yes that whole chunk of
code needs to be factored out)
Apr 28 19:33:55 <lisppaste4> Bert pasted "Initial version of mergeinfo
change notifications" at http://paste.lisp.org/display/79375
Apr 28 19:35:34 <Bert> pburba: If you can find some time, could you
take a quick look at that patch to see if that would give me the
info?.. (It's 1:30 AM here.. time to get some sleep)
Apr 28 23:05:13 <pburba> Bert: I will check that out tomorrow
Bert,
Your patch looks good, making the notification callback from
svn_client__record_wc_mergeinfo looks fine for the most part.
My only *very* minor concerns are:
1) process_children_with_new_mergeinfo() will make a
svn_wc_notify_merge_record_info notification but this isn't a
situation where we are recording mergeinfo *describing the merge*.
Rather it is the case where the diff itself added mergeinfo to a path
that didn't previously have any mergeinfo and we need to make sure the
path's mergeinfo gets any inherited mergeinfo made explicit. Of
course these paths will later get mergeinfo describing the merge and
the svn_wc_notify_merge_record_info notification will take place
then...so I think this is largely a moot concern.
2) It will be possible to generate svn_wc_notify_merge_record_info
notifications for paths that have no mergeinfo to start, get mergeinfo
during the merge, then have their mergeinfo elided away, the end
result being no change to the path. A false positive of sorts, but
since you are only using this as a "hint" to the status cache there is
no harm done.
I made two minor comment tweaks to the attached patch:
1) Made clear in the doc string for
svn_wc_notify_action_t:svn_wc_notify_merge_record_info that this
action is for merges only and then only when recording mergeinfo
*describing* the merge (to differentiate from property changes that
are made to svn:mergeinfo as part of the diff).
2) Noted in doc string for svn_client__record_wc_mergeinfo that it now
does a notification.
Paul
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1980943
Re: [PATCH]: Notify on mergeinfo changes made to describe the merge
Posted by Paul Burba <pt...@gmail.com>.
On Wed, Apr 29, 2009 at 11:08 AM, Paul Burba <pt...@gmail.com> wrote:
> On Wed, Apr 29, 2009 at 11:04 AM, Mark Phippard <ma...@gmail.com> wrote:
>> I cannot comment on the patch. But in general my feeling was that we
>> do not have to change the command line output if we do not want to,
>> but the notifications should be sent for API users so that we can know
>> these items were changed. So I trust Bert to handle that since he has
>> the same need as me (JavaHL patch not withstanding)
>
> Mark,
>
> Agreed, Bert's patch does not change the CL output, I didn't mean to
> imply it did.
>
> Paul
Tying up this thread: Bert committed this change in r37541.
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2302212
Re: [PATCH]: Notify on mergeinfo changes made to describe the merge
Posted by Paul Burba <pt...@gmail.com>.
On Wed, Apr 29, 2009 at 11:04 AM, Mark Phippard <ma...@gmail.com> wrote:
> I cannot comment on the patch. But in general my feeling was that we
> do not have to change the command line output if we do not want to,
> but the notifications should be sent for API users so that we can know
> these items were changed. So I trust Bert to handle that since he has
> the same need as me (JavaHL patch not withstanding)
Mark,
Agreed, Bert's patch does not change the CL output, I didn't mean to
imply it did.
Paul
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1981071
Re: [PATCH]: Notify on mergeinfo changes made to describe the merge
Posted by Mark Phippard <ma...@gmail.com>.
I cannot comment on the patch. But in general my feeling was that we
do not have to change the command line output if we do not want to,
but the notifications should be sent for API users so that we can know
these items were changed. So I trust Bert to handle that since he has
the same need as me (JavaHL patch not withstanding)
Mark
On Wed, Apr 29, 2009 at 10:50 AM, Paul Burba <pt...@gmail.com> wrote:
> Yesterday in IRC:
>
> Apr 28 18:06:11 <Bert> pburba: Do you know what it would take to fix
> notifications for merge info only changes on files while merging? (I'm
> not sure if we would want it as output of svn, but I would really like
> to have the knowledge of which file changed as a notification in
> AnkhSVN (and I heard Mark would like to have the info too)
>
> Apr 28 18:07:54 <pburba> Bert: I dont understand the question, you
> want to suppress the notifications of property changes during a merge
> if the property is svn:mergeinfo?
> Apr 28 18:08:11 <pburba> files only?
>
> Apr 28 18:13:34 <Bert> No.. I want to know exactly which files are
> changed.. And I get reports of some files that receive svn:mergeinfo
> changes but aren't notified as changed
> Apr 28 18:14:30 <Bert> We use the notifications as hint to a status
> cache.. and only call svn status on files that have changes
> Apr 28 18:15:29 <Bert> (actually: We call it on the parent directory
> with --depth files, because that is virtually for free performance
> wise with WC-1.0.. but that is an implementation detail)
>
> Apr 28 18:16:47 <pburba> Bert: Ok, I understand you now. You want
> merge to notify that it is recording mergeinfo on a path describing
> the merge when that path was not directly affected by the diff (and
> thus not mentioned in the notifications). This type of thing can
> happen due to elision, shallow merges, shallow, targets, switched
> subtrees etc.
> Apr 28 18:17:39 <pburba> Bert: The thing is, the merge code doesn't
> record the mergeinfo describing the merge until after the merge (and
> the notifications) are complete. So it would have to be something
> tacked on to the merge output at the end.
>
> Apr 28 18:18:19 <Bert> Or just a notification with another action-id,
> so it wouldn't be notified by svn.. but is available to the bindings
> Apr 28 18:18:58 <Bert> But I wouldn't mind when or how it happens.. I
> just would like to have the paths of the files that changed :)
>
> Apr 28 18:25:05 <pburba> Bert: Ah, ok. Well the place that mergeinfo
> is recorded describing the merge is in
> libsvn_client/merge.c:do_directory_merge(), see the comment ''/*
> Record mergeinfo where appropriate.*/". (and yes that whole chunk of
> code needs to be factored out)
>
> Apr 28 19:33:55 <lisppaste4> Bert pasted "Initial version of mergeinfo
> change notifications" at http://paste.lisp.org/display/79375
> Apr 28 19:35:34 <Bert> pburba: If you can find some time, could you
> take a quick look at that patch to see if that would give me the
> info?.. (It's 1:30 AM here.. time to get some sleep)
>
> Apr 28 23:05:13 <pburba> Bert: I will check that out tomorrow
>
> Bert,
>
> Your patch looks good, making the notification callback from
> svn_client__record_wc_mergeinfo looks fine for the most part.
>
> My only *very* minor concerns are:
>
> 1) process_children_with_new_mergeinfo() will make a
> svn_wc_notify_merge_record_info notification but this isn't a
> situation where we are recording mergeinfo *describing the merge*.
> Rather it is the case where the diff itself added mergeinfo to a path
> that didn't previously have any mergeinfo and we need to make sure the
> path's mergeinfo gets any inherited mergeinfo made explicit. Of
> course these paths will later get mergeinfo describing the merge and
> the svn_wc_notify_merge_record_info notification will take place
> then...so I think this is largely a moot concern.
>
> 2) It will be possible to generate svn_wc_notify_merge_record_info
> notifications for paths that have no mergeinfo to start, get mergeinfo
> during the merge, then have their mergeinfo elided away, the end
> result being no change to the path. A false positive of sorts, but
> since you are only using this as a "hint" to the status cache there is
> no harm done.
>
> I made two minor comment tweaks to the attached patch:
>
> 1) Made clear in the doc string for
> svn_wc_notify_action_t:svn_wc_notify_merge_record_info that this
> action is for merges only and then only when recording mergeinfo
> *describing* the merge (to differentiate from property changes that
> are made to svn:mergeinfo as part of the diff).
>
> 2) Noted in doc string for svn_client__record_wc_mergeinfo that it now
> does a notification.
>
> Paul
>
> ------------------------------------------------------
> http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1980943
--
Thanks
Mark Phippard
http://markphip.blogspot.com/
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1981052