Sorry for the long delay but I finally found some time to circle back to
this.
Den tors 26 aug. 2021 kl 17:21 skrev Daniel Shahaf <[email protected]>:
> > I'm leaning towards setting this as a milestone=2.0 issue and for the
> time
> > leaving it alone not to risk destabilising anything.
>
> That's an option. Or we could include this change in the next alpha or
> beta release to get feedback on it. (We might even be able to use
> a preprocessor timebomb so the change is automatically reverted between
> the last beta and rc1 unless we take positive action to keep it;
> cf. r1889012.)
>
[removed a long thread regarding showing "reverse merges" in the text
version output of svn log]
I agree with Daniel Shahaf in principle, but I'm ignoring this question
right now to focus on fixing the svn log --xml issue. Anyone wanting to
work on the text output can easily pick up from here. I might do it myself
in a future fix.
> Have you reviewed the patch from a coding style perspective? I would
> > appreciate a +1 from someone before committing.
>
> I have now. A few things:
>
> - It's useful to describe the issue (e.g., copy its title) rather than
> refer to it just by number. (subject line)
>
Mail thread. Noted, subject line is now updated.
- Indentation was mangled by emailing, so I didn't review it.
>
Attaching the fix in a file. I have reviewed (and fixed a bunch of issues)
it so hope it should be alright.
> - Some lines appeared to exceed the 80 columns limit.
>
Fixed.
> - I'd have declared «int i;» twice in the two relevant blocks, rather
> than moved it to function scope, so it would have the least possible
> scope. That would convert some bugs (using «i» in a wrong place) into
> compile errors (using an undeclared variable).
>
Ok! (My primary language doesn't allow declarations in blocks so I forgot
that it's the C way).
> + Order of elements of the new struct is good: wider types before
> narrower ones. (Matters for padding bytes.)
>
> That's a review for style. I didn't review correctness.
I hope someone can have a look at this. It seems to work for the different
cases I have thrown at it including the original
log_with_merge_history_and_search test.
In the patch I've extended log_with_merge_history_and_search with a more
complex merge scenario (same as I've used up-thread). This works as well,
but I didn't find a way to verify that the xml structure is nested the way
it is supposed to do.
Kind regards,
Daniel Sahlberg
Index: subversion/svn/log-cmd.c
===================================================================
--- subversion/svn/log-cmd.c (revision 1896351)
+++ subversion/svn/log-cmd.c (working copy)
@@ -45,6 +45,12 @@
#include "svn_private_config.h"
+typedef struct merge_stack {
+ svn_revnum_t rev;
+ svn_boolean_t emitted;
+ svn_boolean_t subtractive_merge;
+} merge_stack_t;
+
/*** Code. ***/
@@ -355,10 +361,16 @@
{
if (log_entry->has_children)
{
+ merge_stack_t ms;
+
if (! lb->merge_stack)
- lb->merge_stack = apr_array_make(lb->pool, 1,
sizeof(svn_revnum_t));
+ lb->merge_stack = apr_array_make(lb->pool, 1,
+ sizeof(merge_stack_t));
- APR_ARRAY_PUSH(lb->merge_stack, svn_revnum_t) = log_entry->revision;
+ ms.rev = log_entry->revision;
+ ms.emitted = FALSE;
+ ms.subtractive_merge = log_entry->subtractive_merge;
+ APR_ARRAY_PUSH(lb->merge_stack, merge_stack_t) = ms;
}
return SVN_NO_ERROR;
@@ -435,10 +447,10 @@
iterpool = svn_pool_create(pool);
for (i = 0; i < lb->merge_stack->nelts; i++)
{
- svn_revnum_t rev = APR_ARRAY_IDX(lb->merge_stack, i, svn_revnum_t);
+ merge_stack_t ms = APR_ARRAY_IDX(lb->merge_stack, i, merge_stack_t);
svn_pool_clear(iterpool);
- SVN_ERR(svn_cmdline_printf(iterpool, " r%ld%c", rev,
+ SVN_ERR(svn_cmdline_printf(iterpool, " r%ld%c", ms.rev,
i == lb->merge_stack->nelts - 1 ?
'\n' : ','));
}
@@ -475,10 +487,15 @@
if (log_entry->has_children)
{
+ merge_stack_t ms;
+
if (! lb->merge_stack)
- lb->merge_stack = apr_array_make(lb->pool, 1, sizeof(svn_revnum_t));
+ lb->merge_stack = apr_array_make(lb->pool, 1, sizeof(merge_stack_t));
- APR_ARRAY_PUSH(lb->merge_stack, svn_revnum_t) = log_entry->revision;
+ ms.rev = log_entry->revision;
+ ms.emitted = FALSE;
+ ms.subtractive_merge = log_entry->subtractive_merge;
+ APR_ARRAY_PUSH(lb->merge_stack, merge_stack_t) = ms;
}
return SVN_NO_ERROR;
@@ -544,11 +561,17 @@
if (! SVN_IS_VALID_REVNUM(log_entry->revision))
{
- svn_xml_make_close_tag(&sb, pool, "logentry");
- SVN_ERR(svn_cl__error_checked_fputs(sb->data, stdout));
if (lb->merge_stack)
- apr_array_pop(lb->merge_stack);
-
+ {
+ if (lb->merge_stack->nelts > 0 &&
+ APR_ARRAY_IDX(lb->merge_stack, lb->merge_stack->nelts-1,
+ merge_stack_t).emitted)
+ {
+ svn_xml_make_close_tag(&sb, pool, "logentry");
+ SVN_ERR(svn_cl__error_checked_fputs(sb->data, stdout));
+ }
+ apr_array_pop(lb->merge_stack);
+ }
return SVN_NO_ERROR;
}
@@ -559,15 +582,48 @@
{
if (log_entry->has_children)
{
+ merge_stack_t ms;
+
if (! lb->merge_stack)
- lb->merge_stack = apr_array_make(lb->pool, 1,
sizeof(svn_revnum_t));
+ lb->merge_stack = apr_array_make(lb->pool, 1,
+ sizeof(merge_stack_t));
- APR_ARRAY_PUSH(lb->merge_stack, svn_revnum_t) = log_entry->revision;
+ ms.rev = log_entry->revision;
+ ms.emitted = FALSE;
+ ms.subtractive_merge = log_entry->subtractive_merge;
+ APR_ARRAY_PUSH(lb->merge_stack, merge_stack_t) = ms;
}
return SVN_NO_ERROR;
}
+ else if (lb->search_patterns && lb->merge_stack)
+ {
+ /* match_search_patterns returned true */
+ int i;
+ for (i = 0; i < lb->merge_stack->nelts; i++)
+ {
+ merge_stack_t ms = APR_ARRAY_IDX(lb->merge_stack, i, merge_stack_t);
+ if (!ms.emitted)
+ {
+ revstr = apr_psprintf(pool, "%ld", ms.rev);
+ if (i == 0)
+ {
+ svn_xml_make_open_tag(&sb, pool, svn_xml_normal, "logentry",
+ "revision", revstr, SVN_VA_NULL);
+ }
+ else
+ {
+ svn_xml_make_open_tag(&sb, pool, svn_xml_normal, "logentry",
+ "revision", revstr, "reverse-merge",
+ ms.subtractive_merge ? "true" :
"false",
+ SVN_VA_NULL);
+ }
+ APR_ARRAY_IDX(lb->merge_stack, i, merge_stack_t).emitted = TRUE;
+ }
+ }
+ }
+
if (author)
author = svn_xml_fuzzy_escape(author, pool);
if (date)
@@ -684,10 +740,15 @@
if (log_entry->has_children)
{
+ merge_stack_t ms;
+
if (! lb->merge_stack)
- lb->merge_stack = apr_array_make(lb->pool, 1, sizeof(svn_revnum_t));
+ lb->merge_stack = apr_array_make(lb->pool, 1, sizeof(merge_stack_t));
- APR_ARRAY_PUSH(lb->merge_stack, svn_revnum_t) = log_entry->revision;
+ ms.rev = log_entry->revision;
+ ms.emitted = TRUE;
+ ms.subtractive_merge = log_entry->subtractive_merge;
+ APR_ARRAY_PUSH(lb->merge_stack, merge_stack_t) = ms;
}
else
svn_xml_make_close_tag(&sb, pool, "logentry");
Index: subversion/tests/cmdline/log_tests.py
===================================================================
--- subversion/tests/cmdline/log_tests.py (revision 1896351)
+++ subversion/tests/cmdline/log_tests.py (working copy)
@@ -2779,7 +2779,6 @@
'',
'-q', '-c', '1-2')
-@XFail()
@Issue(4711)
def log_with_merge_history_and_search(sbox):
"log --use-merge-history --search"
@@ -2789,33 +2788,45 @@
# r2: create branch
sbox.simple_repo_copy('A', 'A2') # r2
- # r3: mod in trunk
- sbox.simple_append('A/mu', 'line 2')
- sbox.simple_commit(message='r3: mod')
+ # r3: create branch
+ sbox.simple_repo_copy('A', 'A3') # r3
+
+ # r4: mod in trunk
+ sbox.simple_append('A/mu', 'line 2\n')
+ sbox.simple_commit(message='r4: mod')
sbox.simple_update()
- # r4: merge
+ # r5: merge A to A2
svntest.main.run_svn(None, 'merge', sbox.repo_url + '/A', sbox.ospath('A2'))
- sbox.simple_commit(message='r4: merge')
+ sbox.simple_commit(message='r5: merge A to A2')
sbox.simple_update()
- # Helper function
- def count(haystack, needle):
- """Return the number of times the string NEEDLE occurs in the string
- HAYSTACK."""
- return len(haystack.split(needle)) - 1
+ # r6: merge A2 to A3
+ svntest.main.run_svn(None, 'merge', sbox.repo_url + '/A2', sbox.ospath('A3'))
+ sbox.simple_commit(message='r6: merge A2 to A3')
+ sbox.simple_update()
- # Check the output is valid
- # ### Since the test is currently XFail, we only smoke test the output.
- # ### When fixing this test to PASS, extend this validation.
- _, output, _ = svntest.main.run_svn(None, 'log', '--xml', '-g',
- '--search', "this will have no matches",
- sbox.ospath('A2'))
+ # r7: reverse merge everything
+ svntest.main.run_svn(None, 'merge', '-r', 'HEAD:3', sbox.repo_url,
sbox.ospath(''))
+ sbox.simple_commit(message='r7: reverse merge')
+ sbox.simple_update()
+
+ # This should return an empty <log> object
+ stdout=['<?xml version="1.0" encoding="UTF-8"?>\n', '<log>\n', '</log>\n']
+ svntest.actions.run_and_verify_log_xml(expected_stdout=stdout,
+ args=['-g', '--search',
+ 'this_will_have_no_matches!',
+ sbox.wc_dir])
+ # This should be a fairly complex log from A3 consisting of
+ # both normal and reverse merges
+ # ### This test should also check if <logentry> are nested properly
+ # ### but I don't find a way to do this without hardcoding the complete
+ # ### output.
+ log_attrs = [{'revision': '7'}, {'reverse-merge': 'true', 'revision': '5'},
{'reverse-merge': 'true', 'revision': '2'}, {'revision': '6'}, {'revision':
'3'}, {}]
+ svntest.actions.run_and_verify_log_xml(expected_log_attrs=log_attrs,
+ args=['-g', '--search',
+ 'merge', sbox.ospath('A3')])
- output = '\n'.join(output)
- if count(output, "<logentry") != count(output, "</logentry"):
- raise svntest.Failure("Apparently invalid XML in " + repr(output))
-
########################################################################
# Run the tests
