You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Sahlberg <da...@gmail.com> on 2021/12/24 19:44:33 UTC
Re: [PATCH] Issue #4711 invalid xml file produced by svn log --xml -g --search
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
Re: [PATCH] Issue #4711 invalid xml file produced by svn log --xml -g --search
Posted by 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