Re: svn commit: r25156 - in branches/merge-sensitive-log/subversion: svn tests/cmdline

Nice!! Time to merge to trunk.

On Fri, 25 May 2007, hwright@tigris.org wrote:
...
> On the merge-sensitive-log branch:
> Add command line client output of merge sensitive log information. Update test,
> and mark it as passing.
>
> * subversion/svn/log-cmd.c
> (log_receiver_baton): Add the merge_stack member.
> (merge_frame): New structure.
> (log_message_receiver): Check for nesting, and output extra "Result of merge"
> line if the message of interested in a child of another message. Also,
> check for children, and push state onto the merge stack if they exist.
> (svn_cl__log): Initialize data structures.
>
> * subversion/test/cmdline/log_tests.py
> (check_merge_results): Check the results of the parse of the "Result of merge"
> line with the expected value.
> (simple_merge_sensitive_log): Update the exepected_merges value to match
> the expected input to check_merge_results().
> (test_list): Mark test simple_merge_sensitive_log() test as passing.
...
> --- branches/merge-sensitive-log/subversion/svn/log-cmd.c (original)
> +++ branches/merge-sensitive-log/subversion/svn/log-cmd.c Fri May 25 11:48:55 2007
> @@ -50,6 +50,22 @@
>
> /* Don't print log message body nor its line count. */
> svn_boolean_t omit_log_message;
> +
> + /* Stack with keeps track of merge revision nesting. */
              ^^^^
Doc string should use "that" or "which". You might also want to
mention the data type that this list holds (struct merge_frame *'s).

> + apr_array_header_t *merge_stack;
> +};
> +
> +/* Structure to hold merging revisions, and the number of children they have
> + remaining. These structures get pushed and popped from
> + log_receiver_baton.merge_stack, and help implement the pre-order traversal
> + of the log message tree. */
> +struct merge_frame
> +{
> + /* The revision the merge occured in. */
> + svn_revnum_t merge_rev;
> +
> + /* The number of outstanding children. */
> + apr_uint64_t child_count;

We ended up calling this "nbr_children" elsewhere.

> };
...

I was wondering if we want to use an iterpool in the following loop;
but, it doesn't look like it's necessary.

> + if (lb->merge_stack->nelts > 0)
> + {
> + int i;
> + struct merge_frame *frame = APR_ARRAY_IDX(lb->merge_stack,
> + lb->merge_stack->nelts - 1,
> + struct merge_frame *);
> +
> + /* Print the result of merge line */
> + SVN_ERR(svn_cmdline_printf(pool, _("Result of a merge from:")));
> + for (i = 0; i < lb->merge_stack->nelts; i++)
> + {
> + struct merge_frame *output_frame = APR_ARRAY_IDX(lb->merge_stack, i,
> + struct merge_frame *);
> +
> + SVN_ERR(svn_cmdline_printf(pool, " r%ld%c", output_frame->merge_rev,
> + i == lb->merge_stack->nelts - 1 ?
> + '\n' : ','));
> + }
> +
> + /* Decrement the child_counter, and check to see if we have any more
> + children. If not, pop the stack. */
> + frame->child_count -= 1;

Or:

         frame->child_count--;

> + if (frame->child_count == 0)
> + apr_array_pop(lb->merge_stack);
> + }
...
> + if (log_entry->nbr_children > 0)
> + {
> + struct merge_frame *frame = apr_palloc(pool, sizeof(*frame));
> +
> + frame->merge_rev = log_entry->revision;
> + frame->child_count = log_entry->nbr_children;
> +
> + APR_ARRAY_PUSH(lb->merge_stack, struct merge_frame *) = frame;
> + }
> +
> return SVN_NO_ERROR;
> }
>
> @@ -490,6 +542,7 @@
> lb.cancel_func = ctx->cancel_func;
> lb.cancel_baton = ctx->cancel_baton;
> lb.omit_log_message = opt_state->quiet;
> + lb.merge_stack = apr_array_make(pool, 1, sizeof(svn_revnum_t));

The element size for this list is incorrect. You want sizeof(struct
merge_frame *).

...
> --- branches/merge-sensitive-log/subversion/tests/cmdline/log_tests.py (original)
> +++ branches/merge-sensitive-log/subversion/tests/cmdline/log_tests.py Fri May 25 11:48:55 2007
> @@ -927,6 +927,13 @@
> for rev in expected_merges.keys():
> try:
> log = [x for x in log_chain if x['revision'] == rev][0]
> + actual = log['merges']
> + expected = expected_merges[rev]
> +
> + if actual != expected:
> + raise SVNUnexpectedLogs((("Merging revisions in rev %d not correct; " +
> + "expecting ") % rev) + str(expected) +
> + ", found " + str(actual), log_chain)

This string concat might look a little cleaner with format specifiers.
Also, the indentation looks a tad suspect.

> except IndexError:
> raise SVNUnexpectedLogs("Merged revision '%d' missing" % rev, log_chain)
>
> @@ -957,7 +964,7 @@
>
> log_chain = parse_log_output(output)
> expected_merges = {
> - 1 : 7, 3 : 7, 4 : 7, 5 : 7, 6 : 7,
> + 1 : [2], 3 : [7], 4 : [7], 6 : [7],
> }
> check_merge_results(log_chain, expected_merges)
...