Hyrum K Wright <hyrum_at_hyrumwright.org> writes:
> On Mon, Jan 31, 2011 at 10:52 PM, Noorul Islam K M <noorul_at_collab.net> wrote:
>
>> Hyrum K Wright <hyrum_at_hyrumwright.org> writes:
>>
>>> On Mon, Jan 31, 2011 at 10:53 AM, Noorul Islam K M <noorul_at_collab.net> wrote:
>>>
>>>> Daniel Becroft <djcbecroft_at_gmail.com> writes:
>>>>
>>>>> On 27/01/2011, at 17:04, Noorul Islam K M <noorul_at_collab.net> wrote:
>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> I am planning to work on issue 3690. Before starting with this I have
>>>>>> few questions.
>>>>>>
>>>>>> 1. Hyrum updated the issue with his comment stating that already there
>>>>>> is work going on in the branch ignore-mergeinfo which addresses
>>>>>> subset of issue 3690, i.e ignoring changes to svn:mergeinfo. Is
>>>>>> svn:mergeinfo an svn property set using svn pset command? I think Zvi
>>>>>> Rackover is talking about a new option passing which a user can
>>>>>> ignore revisions with just the following property changes alone.
>>>>>>
>>>>>> author eol-style externals keywords mime-type
>>>>>> date executable ignore log needs-lock
>>>>>
>>>>> Fyi, svn:author, svn:date and svn:log are revision properties - changes to these don't appear in the log.
>>>>>
>>>>> Cheers,
>>>>> Daniel
>>>>> Sent from my phone.
>>>>>
>>>>
>>>> I started working on this and I think I completed the changes for
>>>> svn_ra_local. Attached is the patch. This is a work in progress. I would
>>>> like to get some initial comments/suggestions on the patch and would
>>>> like to know whether I am proceeding on right direction.
>>>>
>>>> This patch adds new option '--ignore-properties' to 'log' sub
>>>> command. If this option is provided then command ignore revisions that
>>>> has only property changes from output.
>>>
>>> I haven't reviewed the patch, but would like to discuss the approach.
>>>
>>> Ignoring all or none of a set of prop mods may not be granular enough.
>>> Since consumers rarely change mergeinfo without changing content, I
>>> don't see that the new behavior would be exercised very often. What
>>> we really want to do is filter out mergeinfo changes on a per-node
>>> basis, not a per-revision basis as the new docs claim. (Perhaps the
>>> docs are inaccurate?)
>>>
>>> I don't see anywhere in the patch where you address RA layers other
>>> than ra_local. Also, what is the expected behavior when operating
>>
>> I mentioned this in the mail. I will be handling this in the future. I
>> just wanted get feedback from list so that I might not end up direction
>> less.
>>
>>> against older servers? If you send the ignore_properties flag to
>>> older servers, they will just ignore the flag and return all the
>>> revisions, and the client will dutifully report them. Do you have a
>>> plan for this scenario?
>>
>> Thank you for pointing this out. So far I have not thought about
>> this. What about throwing an error, if there the server version is less
>> than what we expect?
>
> Or more specifically, if the server doesn't support the required
> capability (as on the ignore-mergeinfo-log branch).
>
I will copy this :)
>>> Finally, is there a reason why you chose to do this against trunk,
>>> rather than the ignore-mergeinfo-log branch, which already addresses
>>> many of the above concerns? I've been hacking on that branch a bit
>>> lately, and it isn't complete but patches and review are definitely
>>> welcome.
>>>
>>
>> I did this against trunk because I could see lots of changes in
>> ignore-mergeinfo-log branch. For me to avoid confusion, I thought I will
>> start preparing patch against trunk. Also if I include this option also
>> to the ignore-mergeinfo-log branch, will that be possible to factor out
>> my changes alone? Or is it a good idea to have a separate branch for
>> this. I am not sure at this point.
>>
>> Do you mean to say that ignore-mergeinfo-log has an option to filter out
>> revisions being send to the receiver if that revision contains only
>> property changes? Isn't that issue 3690 all about?
>
> Well, reading back through issue 3690, it looks like it could use some
> design work. :)
>
> The reporter states the request, and then this motivation: "many users
> are not interested in reviewing changes to property changes and only
> care about content changes." To that I would ask "why?" What
> scenarios does this pop up in.
>
> I know my specific usecase is managing the release branches, and
> having to filter through all the svn:mergeinfo mods when attempting to
> find the last time CHANGES was textually modified. Maybe that would
> be accomplished by a 'ignore all prop mods' flag; maybe not. Maybe
> the approach on the ignore-mergeinfo-log branch is *too* discrete, and
> would end up being a performance killer; maybe not. The more I write,
> the the more I think this deserves some design consideration. (And
> hence my desire to do it on a branch rather than trunk: I don't want
> to hold up 1.7, and don't want to release a half-cooked idea.)
>
> One other option may be to have some way of using the ignored prop mod
> list, as on the branch, to specify ignoring *all* prop mods,
> irrespective of property name. From an API perspective, this could
> accomplish both goals. But I still think we need to think more about
> what problem we're trying to solve.
I will create this patch against ignore-mergeinfo-log branch. I will
make the changes as you did for ignore-mergeinfo one by one. Attached is
the first patch which adds the option to log sub command. It will be
great if you could review them and let me know the comments.
Log
[[[
Add the ignore_properties flag to the command line client, and let 'svn
log' accept it. Don't actually do anything with it yet.
* BRANCH-README: Add ignore-properties task list.
* subversion/svn/cl.h
(svn_cl__opt_state_t): New ignore_properties member.
* subversion/svn/main.c
(svn_cl__longopt_t): Add opt_ignore_properties member.
(svn_cl__options): Document the ignore-properties flag.
(svn_cl__cmd_table): Accept ignore-properties for log operations.
(main): If given --ignore-properties, set the option flag to TRUE.
]]]
>
> Incidentally, I like the issue number: 3690. For bonus points, can
> anybody tell my why that number is significant in Subversion-land? :)
>
> -Hyrum
>
> PS - Sorry if I was a bit harsh in the last mail.
No problems.
Thanks and Regards
Noorul
Index: BRANCH-README
===================================================================
--- BRANCH-README (revision 1066373)
+++ BRANCH-README (working copy)
@@ -6,3 +6,15 @@
TODO:
- Retrieve logs from the server
+
+Also to add --ignore-properties to 'svn log'.
+
+The goal of this flag is to allow users to filter revisions that have
+only property changes from log command output.
+
+TODO:
+ - Add --ignore-properties option to svn log
+ - Modify ra_local APIs
+ - Modify ra_svn APIs
+ - Modify ra_serf APIs
+ - Modify ra_neon APIs
Index: subversion/svn/cl.h
===================================================================
--- subversion/svn/cl.h (revision 1066373)
+++ subversion/svn/cl.h (working copy)
@@ -231,6 +231,9 @@
svn_boolean_t use_git_diff_format; /* Use git's extended diff format */
svn_boolean_t allow_mixed_rev; /* Allow operation on mixed-revision WC */
svn_boolean_t ignore_mergeinfo; /* ignore mergeinfo in reporting commands. */
+ svn_boolean_t ignore_properties; /* Ignore revisions with only property
+ changes from log command output */
+
} svn_cl__opt_state_t;
Index: subversion/svn/main.c
===================================================================
--- subversion/svn/main.c (revision 1066373)
+++ subversion/svn/main.c (working copy)
@@ -123,7 +123,8 @@
opt_internal_diff,
opt_use_git_diff_format,
opt_allow_mixed_revisions,
- opt_ignore_mergeinfo
+ opt_ignore_mergeinfo,
+ opt_ignore_properties,
} svn_cl__longopt_t;
@@ -341,6 +342,8 @@
"Please run 'svn update' instead.")},
{"ignore-mergeinfo", opt_ignore_mergeinfo, 0,
N_("ignore changes to mergeinfo")},
+ {"ignore-properties", opt_ignore_properties, 0,
+ N_("ignore revisions with only property changes")},
/* Long-opt Aliases
*
@@ -671,7 +674,8 @@
" svn log http://www.example.com/repo/project@50 foo.c bar.c\n"),
{'r', 'q', 'v', 'g', 'c', opt_targets, opt_stop_on_copy, opt_incremental,
opt_xml, 'l', opt_with_all_revprops, opt_with_no_revprops, opt_with_revprop,
- opt_diff, opt_diff_cmd, opt_internal_diff, opt_ignore_mergeinfo, 'x'},
+ opt_diff, opt_diff_cmd, opt_internal_diff, opt_ignore_mergeinfo,
+ opt_ignore_properties, 'x'},
{{opt_with_revprop, N_("retrieve revision property ARG")},
{'c', N_("the change made in revision ARG")}} },
@@ -1822,6 +1826,9 @@
case opt_ignore_mergeinfo:
opt_state.ignore_mergeinfo = TRUE;
break;
+ case opt_ignore_properties:
+ opt_state.ignore_properties = TRUE;
+ break;
default:
/* Hmmm. Perhaps this would be a good place to squirrel away
opts that commands like svn diff might need. Hmmm indeed. */
Received on 2011-02-02 10:46:43 CET