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 <d...@daniel.shahaf.name>: > > 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