You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@btopenworld.com> on 2014/07/03 17:13:54 UTC
Mergeinfo property diff regression
'svn diff' describes svn:mergeinfo property diffs as "Merged ..." or "Reverse merged..." rather than using the unified diff output style that it uses for other property values.
With Subversion 1.8.9:
[[[
$ svn propget -rBASE svn:mergeinfo A; svn propget svn:mergeinfo A
/trunk:10-15,20-25
/trunk:10-18,20-25
$ svn diff
Index: A
===================================================================
--- A (revision 1)
+++ A (working copy)
Property changes on: A
___________________________________________________________________
Modified: svn:mergeinfo
Merged /trunk:r16-18
]]]
With Subversion trunk the last part of the output changes to:
[[[
Modified: svn:mergeinfo
## -0,0 +0,1 ##
Merged /trunk:r16-18
]]]
The line beginning with "##" has crept in, where it was previously omitted.
I assume this is unintentional, as the information in it is not useful.
- Julian
Re: Mergeinfo property diff regression
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Jul 03, 2014 at 05:27:00PM +0100, Julian Foad wrote:
> Stefan Sperling wrote:
> > On Thu, Jul 03, 2014 at 04:13:54PM +0100, Julian Foad wrote:
> >> With Subversion 1.8.9:
> >> [[[
> >> Modified: svn:mergeinfo
> >> Merged /trunk:r16-18
> >> ]]]
> >>
> >> With Subversion trunk the last part of the output changes to:
> >>
> >> [[[
> >> Modified: svn:mergeinfo
> >> ## -0,0 +0,1 ##
> >> Merged /trunk:r16-18
> >> ]]]
> >
> > There is a rational outlined in the commit which introduced this change.
> >
> > ------------------------------------------------------------------------
> > r1598092 | stsp | 2014-05-28 20:08:52 +0200 (Wed, 28 May 2014) | 14 lines
> >
> > For issue #3747, 'svn patch' support of svn:mergeinfo, make it
> > possible to parse svn:mergeinfo diffs regardless of i18n.
> >
> > * subversion/libsvn_diff/util.c
> > (display_mergeinfo_diff): Print a hunk header line for mergeinfo diffs.
> > The header indicates the number of reverse-merges and forward-merges in
> > the diff. Before this change, this information was only printed as part
> > of output which is translated for i18n. This made it impossible to reliably
> > parse svn:mergeinfo diffs from patch files.
> > Futher changes to the diff parser and svn patch are coming soon.
> > ------------------------------------------------------------------------
>
> Thanks for explaining the rationale.
>
> I like having parseable output.
>
> How exactly does the '##' line indicate the number of merges?
The docstring of libsvn_diff/parse-diff.c:parse_mergeinfo() mentions
how the format works. The leading zeroes (before comma) don't mean
anything. I kept them so I could reuse existing parser code, and for
visual consistency with other property diff hunk headers where these
numbers carry the same meaning as in regular unidiff file content
changes.
/* A helper function to parse svn:mergeinfo diffs.
*
* These diffs use a special pretty-print format, for instance:
*
* Added: svn:mergeinfo
* ## -0,0 +0,1 ##
* Merged /trunk:r2-3
*
* The hunk header has the following format:
* ## -0,NUMBER_OF_REVERSE_MERGES +0,NUMBER_OF_FORWARD_MERGES ##
*
* At this point, the number of reverse merges has already been
* parsed into HUNK->ORIGINAL_LENGTH, and the number of forward
* merges has been parsed into HUNK->MODIFIED_LENGTH.
*
* The header is followed by a list of mergeinfo, one path per line.
* This function parses such lines. Lines describing reverse merges
* appear first, and then all lines describing forward merges appear.
*
* Parts of the line are affected by i18n. The words 'Merged'
* and 'Reverse-merged' can appear in any language and at any
* position within the line. We can only assume that a leading
* '/' starts the merge source path, the path is followed by
* ":r", which in turn is followed by a mergeinfo revision range,
* which is terminated by whitespace or end-of-string.
*
* If the current line meets the above criteria and we're able
* to parse valid mergeinfo from it, the resulting mergeinfo
* is added to patch->mergeinfo or patch->reverse_mergeinfo,
* and we proceed to the next line.
*/
static svn_error_t *
parse_mergeinfo(svn_boolean_t *found_mergeinfo,
svn_stringbuf_t *line,
svn_diff_hunk_t *hunk,
svn_patch_t *patch,
apr_pool_t *result_pool,
apr_pool_t *scratch_pool)
Re: Mergeinfo property diff regression
Posted by Julian Foad <ju...@btopenworld.com>.
Stefan Sperling wrote:
> On Thu, Jul 03, 2014 at 04:13:54PM +0100, Julian Foad wrote:
>> With Subversion 1.8.9:
>> [[[
>> Modified: svn:mergeinfo
>> Merged /trunk:r16-18
>> ]]]
>>
>> With Subversion trunk the last part of the output changes to:
>>
>> [[[
>> Modified: svn:mergeinfo
>> ## -0,0 +0,1 ##
>> Merged /trunk:r16-18
>> ]]]
>
> There is a rational outlined in the commit which introduced this change.
>
> ------------------------------------------------------------------------
> r1598092 | stsp | 2014-05-28 20:08:52 +0200 (Wed, 28 May 2014) | 14 lines
>
> For issue #3747, 'svn patch' support of svn:mergeinfo, make it
> possible to parse svn:mergeinfo diffs regardless of i18n.
>
> * subversion/libsvn_diff/util.c
> (display_mergeinfo_diff): Print a hunk header line for mergeinfo diffs.
> The header indicates the number of reverse-merges and forward-merges in
> the diff. Before this change, this information was only printed as part
> of output which is translated for i18n. This made it impossible to reliably
> parse svn:mergeinfo diffs from patch files.
> Futher changes to the diff parser and svn patch are coming soon.
> ------------------------------------------------------------------------
Thanks for explaining the rationale.
I like having parseable output.
How exactly does the '##' line indicate the number of merges?
- Julian
> The idea to make 'svn patch' support mergeinfo was later vetoed.
>
> I think the information is still valuable to diff parsers, and our diff
> parser was also changed to look for it (though svn patch doesn't use it).
> I don't see a problem with adding this feature but if someone can show
> that it arguably introduces a huge problem I would revert it.
Re: Mergeinfo property diff regression
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Jul 03, 2014 at 04:13:54PM +0100, Julian Foad wrote:
> 'svn diff' describes svn:mergeinfo property diffs as "Merged ..." or "Reverse merged..." rather than using the unified diff output style that it uses for other property values.
>
> With Subversion 1.8.9:
>
> [[[
> $ svn propget -rBASE svn:mergeinfo A; svn propget svn:mergeinfo A
> /trunk:10-15,20-25
> /trunk:10-18,20-25
>
> $ svn diff
> Index: A
> ===================================================================
> --- A (revision 1)
> +++ A (working copy)
>
> Property changes on: A
> ___________________________________________________________________
> Modified: svn:mergeinfo
> Merged /trunk:r16-18
> ]]]
>
> With Subversion trunk the last part of the output changes to:
>
> [[[
> Modified: svn:mergeinfo
> ## -0,0 +0,1 ##
> Merged /trunk:r16-18
> ]]]
>
> The line beginning with "##" has crept in, where it was previously omitted.
>
> I assume this is unintentional, as the information in it is not useful.
>
> - Julian
There is a rational outlined in the commit which introduced this change.
------------------------------------------------------------------------
r1598092 | stsp | 2014-05-28 20:08:52 +0200 (Wed, 28 May 2014) | 14 lines
For issue #3747, 'svn patch' support of svn:mergeinfo, make it
possible to parse svn:mergeinfo diffs regardless of i18n.
* subversion/libsvn_diff/util.c
(display_mergeinfo_diff): Print a hunk header line for mergeinfo diffs.
The header indicates the number of reverse-merges and forward-merges in
the diff. Before this change, this information was only printed as part
of output which is translated for i18n. This made it impossible to reliably
parse svn:mergeinfo diffs from patch files.
Futher changes to the diff parser and svn patch are coming soon.
* subversion/tests/cmdline/merge_tests.py
(merge_in_new_file_and_diff): Adjust expected output.
------------------------------------------------------------------------
The idea to make 'svn patch' support mergeinfo was later vetoed.
I think the information is still valuable to diff parsers, and our diff
parser was also changed to look for it (though svn patch doesn't use it).
I don't see a problem with adding this feature but if someone can show
that it arguably introduces a huge problem I would revert it.