Re: [PATCH] Issue #4711 invalid xml file produced by svn log --xml -g --search

[Daniel Sahlberg <da...@gmail.com>]

Den fre 24 dec. 2021 kl 20:44 skrev Daniel Sahlberg <
daniel.l.sahlberg@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.s@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