Den fre 24 dec. 2021 kl 20:44 skrev Daniel Sahlberg < daniel.l.sahlb...@gmail.com>:
> 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. > I would like to commit this fix and unless anyone objects I will do this during next week. In addition to testing on my own tree and the test case in #4711, I have also run the following command on 1.13.0 (r1867053), as supplied in Ubuntu 20.04.3, on 1.14.2-dev (built from a recent trunk) as well as current trunk with the attached patch: [[[ svn log --search=revision -g --xml https://svn.apache.org/repos/asf/subversion/branches/1.10.x ]]] In 1.13.0 and 1.14.2-dev this produce a lot of mismatched </logentry> end tags. With this patch the XML file is balanced. Kind regards, Daniel
