On 6 December 2013 02:09, Ben Reser <bre...@apache.org> wrote: > On 12/5/13 12:30 PM, Ivan Zhakov wrote: >> On 5 December 2013 06:20, <bre...@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
