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
 

Reply via email to