Noorul Islam K M <noo...@collab.net> writes:
> Hyrum K Wright <hy...@hyrumwright.org> writes:
>
>> On Mon, Jan 31, 2011 at 10:52 PM, Noorul Islam K M <noo...@collab.net> wrote:
>>
>>> Hyrum K Wright <hy...@hyrumwright.org> writes:
>>>
>>>> On Mon, Jan 31, 2011 at 10:53 AM, Noorul Islam K M <noo...@collab.net>
>>>> wrote:
>>>>
>>>>> Daniel Becroft <djcbecr...@gmail.com> writes:
>>>>>
>>>>>> On 27/01/2011, at 17:04, Noorul Islam K M <noo...@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
[[[
On the ignore-mergeinfo-log branch:
Add a simple test of ignoring revisions with only property changes.
* subversion/tests/cmdline/log_tests.py
(log_ignore_properties): New.
(test_list): Run the new test as XFail.
]]]
Thanks and Regards
Noorul
Index: subversion/tests/cmdline/log_tests.py
===================================================================
--- subversion/tests/cmdline/log_tests.py (revision 1066373)
+++ subversion/tests/cmdline/log_tests.py (working copy)
@@ -1795,7 +1795,27 @@
iota_path)
check_log_chain(parse_log_output(output), [5, 3])
+def log_ignore_properties(sbox):
+ "svn log --ignore-properties"
+ sbox.build()
+ wc_dir = sbox.wc_dir
+ iota_file = os.path.join(wc_dir, 'iota')
+ svntest.main.run_svn(None, 'propset', 'foo', 'bar', iota_file)
+ svntest.main.run_svn(None, 'ci', '-m',
+ 'Set property "foo" to "bar" on A/iota', wc_dir)
+
+ svntest.main.run_svn(None, 'propset', 'bar', 'foo', iota_file)
+ svntest.main.run_svn(None, 'ci', '-m',
+ 'Set property "bar" to "foo" on A/iota', wc_dir)
+ exit_code, output, error = svntest.main.run_svn(0, 'log',
+ '--ignore-properties',
+ wc_dir)
+
+ expected_output_re = re.compile(".*Set property.*")
+ if expected_output_re.match("".join(output)):
+ raise svntest.Failure('Log failed with --ignore-properties')
+
########################################################################
# Run the tests
@@ -1839,6 +1859,7 @@
server_has_mergeinfo),
log_of_local_copy,
SkipUnless(XFail(ignore_mergeinfo_log), server_has_mergeinfo),
+ XFail(log_ignore_properties),
]
if __name__ == '__main__':
