On 6 December 2013 02:09, Ben Reser <breser_at_apache.org> wrote:
> On 12/5/13 12:30 PM, Ivan Zhakov wrote:
>> On 5 December 2013 06:20, <breser_at_apache.org> wrote:
>>> Author: breser
>>> Date: Thu Dec 5 02:20:41 2013
>>> New Revision: 1547995
>>>
>>> URL: http://svn.apache.org/r1547995
>>> Log:
>>> Make mergeinfo and log commands share the same log receiver implementation.
>>>
>> The only problem with this change that 'svn log' has special behavior
>> for printing merged revisions and this behavior probably doesn't make
>> sense for svn mergeinfo --log. That's why I didn't share
>> implementation half a year ago.
>
Hi Ben,
Sorry for delayed reply.
> How is that a problem? It only activates if a log entry has the has_children
> field set to true, which can never happen with mergeinfo --log since
> svn_client_mergeinfo_log2() doesn't even have a include_merged_revisions
> option. The only complication it really presents is we have to allocate a
> apr_array_header_t for merge_stack since the code in the log receiver doesn't
> check it. Which actually would be really trivial to fix, I just didn't do it.
>
I was worried a but that we have to care that mergeinfo --log will
never have include_merged_revisions option.
I didn't worry about allocation of empty apr_array_header_t of course.
After further looking to r1548334 I would suggest use merge_stack as a
flag for printing merged revisions or not, instead of deferred
allocation. I mean if merge_stack is non-zero then handle children
revisions, otherwise do not. But I'm also with always allocating
merge_stack member, since code is more clear when we don't have to
care to perform deferred allocation of apr_header_t.
> My intention for doing this was adding the various options log supports to
> mergeinfo --log. I thought about trying to move the support to log and
> specifying --show-revs on log. The problem with that is:
>
> 1) log takes multiple targets. But mergeinfo needs the source and destination
> paths to work. It felt clunky trying to add an argument for the target branch
> to log.
>
> 2) --show-revs itself as a parameter is poorly named to allow reuse on other
> commands.
>
> Pulling in the options that log supports into mergeinfo also feels clunky, but
> from a UI perspective it works. The duplicated code to support the options is
> really trivial when both commands share the same log receivers.
>
> Adding support for all the features that both will support onto the log
> receiver for mergeinfo becomes more and more problematic. As I'm sure you can
> guess I'm planning to add --xml as well.
I fully agree with your. I didn't have objections on idea itself.
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Received on 2013-12-09 22:57:05 CET