You are viewing a plain text version of this content. The canonical link for it is here.
Posted to users@subversion.apache.org by Alfred Perlstein <br...@mu.org> on 2013/01/31 16:37:14 UTC
FreeBSD project and subversion.
FreeBSD has moved to Subversion a few years ago and it's worked very,
very well for us.
The one area where we are having issues is merging code from project
branches back into trunk.
The typical workflow is:
1) create project branch.
2) code code code.
3) sync from HEAD (this works great).
4) repeat steps 2+3 until feature is complete.
5) svn diff and apply to head then commit.
Is there a way to do 5 automatically?
I think the worry is mergeinfo from the project branch coming back into
HEAD.
Any tips would be appreciated.
thank you,
-Alfred
Re: FreeBSD project and subversion.
Posted by Stefan Sperling <st...@elego.de>.
On Fri, Feb 01, 2013 at 11:11:05AM -0500, Alfred Perlstein wrote:
> So I have two answers here:
>
> 1) about mergeinfo
> It seems as if doing it all at the top can make merges over long
> haul internet very painful.
I acknowledge that a merge that runs across the entire FreeBSD src
tree can be slow.
For this and other reasons, it can make sense to create mergeinfo at
particular levels of the tree.
For example, it can make sense to run kernel code merges within src/sys,
and userland changes elsewhere. Subversion won't care. Keeping a consistent
pattern by documenting the expected roots used for merges and then using
those consistently will help create a clean log history.
I see that FreeBSD has already documented this, which is good!
The major hassle created by lots of subtree mergeinfo is property
changes cluttering out of commands such as 'svn log -v' and 'svn diff'.
This was particularly bad with Subversion 1.6, which updated any subtree
mergeinfo with a merge target, regardless of whether the merge actually
touched content of the files and directories which had their svn:mergeinfo
property updated. This has been addressed in Subversion 1.7, see:
http://subversion.apache.org/docs/release-notes/1.7.html#subtree-mergeinfo-recording
Also, there is no way to filter excessive property changes from the
output of 'svn diff'. This will be partly addressed in Subversion 1.8
by a new --ignore-properties option, see
http://subversion.apache.org/docs/release-notes/1.8.html#svn-diff
While this suppresses all property diffs and doesn't target
svn:mergeinfo in particular, it can help with producing diff output
that more usable than the default output in some cases.
> 2) about reintegrate
> I've attached the two messages that show what makes FreeBSD gun shy
> about merges to HEAD.
> Maybe these issues are resolved, or maybe the developer did
> something incorrect, or maybe something else entirely different
> happened.
See my response below.
It turns out that this wasn't a problem with --reintegrate at all,
but a user error made during a sync merge.
> Date: Fri, 1 Feb 2013 09:49:23 -0500
> From: John Baldwin <jh...@freebsd.org>
> To: Alfred Perlstein <al...@freebsd.org>
> Subject: Fwd: Re: SVN merge question.
> X-Spam-Level:
> User-Agent: KMail/1.13.5 (FreeBSD/8.2-CBSD-20110714-p22; KDE/4.5.5; amd64;
> ; )
> Content-Type: Multipart/Mixed; boundary="Boundary-00=_zX9CRbNRbX0geE6"
> Message-Id: <20...@freebsd.org>
>
>
> --
> John Baldwin
> Date: Fri, 1 Jun 2012 13:56:03 -0400
> From: John Baldwin <jh...@freebsd.org>
> To: obrien@freebsd.org
> Cc: Grzegorz Bernacki <gb...@freebsd.org>, Eitan Adler <ea...@freebsd.org>,
> developers@freebsd.org
> Subject: Re: SVN merge question.
> User-Agent: KMail/1.13.5 (FreeBSD/8.2-CBSD-20110714-p13; KDE/4.5.5; amd64;
> ; )
> Content-Type: Text/Plain; charset="iso-8859-1"
> Message-Id: <20...@freebsd.org>
>
> On Friday, June 01, 2012 1:40:29 pm David O'Brien wrote:
> > On Wed, May 16, 2012 at 11:00:48AM -0400, John Baldwin wrote:
> > > I more or less don't trust svn merge to DTRT here. This was done with
> > > the cpuset branch merge up to HEAD and it broke the log history of many
> > > files in HEAD.
> >
> > Specifically how did it break log history?
>
> http://svnweb.freebsd.org/base/head/share/man/man4/geom_map.4?view=log
>
> You have to walk up to the previous directory in svnweb and go back to
> change 222812 and then click on geom_map.4 to find its original log.
>
> sys/dev/iicbus/ad7417.c was also busted this way.
Adrian Chadd originally added the man page to the head branch in this
revision:
[[[
------------------------------------------------------------------------
r221501 | adrian | 2011-05-05 16:43:35 +0200 (Thu, 05 May 2011) | 4 lines
Changed paths:
M /head/share/man/man4/Makefile
A /head/share/man/man4/geom_map.4
Add a manpage for geom_map(4).
Submitted by: ray@dlink.ua
]]]
In a later revision, Attilio Rao re-added the same file to the largeSMP
branch, apparently while being immersed in a cloud of rage against svn :)
[[[
------------------------------------------------------------------------
r221716 | attilio | 2011-05-10 00:13:07 +0200 (Tue, 10 May 2011) | 4 lines
Changed paths:
A /projects/largeSMP/share/man/man4/geom_map.4
A /projects/largeSMP/sys/nfs/nfs_kdtrace.h
A /projects/largeSMP/sys/sys/_stdint.h
A /projects/largeSMP/tools/build/options/WITHOUT_GPIO
A /projects/largeSMP/tools/regression/bin/sh/parser/dollar-quote1.0
A /projects/largeSMP/tools/regression/bin/sh/parser/dollar-quote2.0
A /projects/largeSMP/tools/regression/bin/sh/parser/dollar-quote3.0
A /projects/largeSMP/tools/regression/bin/sh/parser/dollar-quote4.0
A /projects/largeSMP/tools/regression/bin/sh/parser/dollar-quote5.0
A /projects/largeSMP/tools/regression/bin/sh/parser/dollar-quote6.0
A /projects/largeSMP/tools/regression/bin/sh/parser/dollar-quote7.0
A /projects/largeSMP/tools/regression/bin/sh/parser/dollar-quote8.0
A /projects/largeSMP/tools/regression/bin/sh/parser/dollar-quote9.0
Fix by hand files that aren't added automatically by svn.
BIG SUCKAGE!
]]]
It's not clear what happened here and why the files were missing.
The largeSMP branch was first created in r221273 as an unmodified
copy of ^/head@221272. That's good.
Just before the "big suckage" commit, Attilio merged r221699-221709
from ^/head in r221716. This merged range does not include r221501,
so the files had been missing from his branch much earlier.
In r221562, an earlier sync merge from ^/head to ^/projects/largeSMP
was committed, which includes r221501 in its svn:mergeinfo changes
(the range merged was r221494-221557).
Let's try this sync merge:
$ svn co svn://svn.freebsd.org/base/projects/largeSMP@221561
$ cd largeSMP
$ svn merge -r221494:221558 ^/head
The result is:
--- Merging r221495 through r221558 into '.':
A tools/build/options/WITHOUT_GPIO
U tools/build/options/WITHOUT_FLOPPY
U tools/build/options/WITHOUT_FDT
U tools/build/options/WITHOUT_ACCT
D tools/build/options/WITH_GPIO
A tools/regression/bin/sh/parser/dollar-quote1.0
A tools/regression/bin/sh/parser/dollar-quote2.0
A tools/regression/bin/sh/parser/dollar-quote3.0
A tools/regression/bin/sh/parser/dollar-quote4.0
A tools/regression/bin/sh/parser/dollar-quote5.0
A tools/regression/bin/sh/parser/dollar-quote6.0
A tools/regression/bin/sh/parser/dollar-quote7.0
A tools/regression/bin/sh/parser/dollar-quote8.0
A tools/regression/bin/sh/parser/dollar-quote9.0
[...]
--- Merging r221495 through r221558 into '.':
A share/man/man4/geom_map.4
[...]
$ svn status tools/regression/bin/sh/parser/
A + tools/regression/bin/sh/parser/dollar-quote1.0
A + tools/regression/bin/sh/parser/dollar-quote2.0
A + tools/regression/bin/sh/parser/dollar-quote3.0
A + tools/regression/bin/sh/parser/dollar-quote4.0
A + tools/regression/bin/sh/parser/dollar-quote5.0
A + tools/regression/bin/sh/parser/dollar-quote6.0
A + tools/regression/bin/sh/parser/dollar-quote7.0
A + tools/regression/bin/sh/parser/dollar-quote8.0
A + tools/regression/bin/sh/parser/dollar-quote9.0
$ svn status share/man/man4
M share/man/man4/Makefile
A + share/man/man4/geom_map.4
$ svn info share/man/man4/geom_map.4 | grep ^Copied\ From
Copied From URL: svn://svn.freebsd.org/base/head/share/man/man4/geom_map.4
Copied From Rev: 221558
So r221562 commit should have included these files, and we should be
seeing lines in 'svn log -v -r221562' such as:
A /projects/largeSMP/share/man/man4/geom_map.4 (from /head/share/man/man4/geom_map:221558)
For some reason these changes weren't committed by Attilio.
I guess that Attilio had some local state in his working copy that
prevented some files from being added (unversioned files of the same
name, e.g. due to build artifacts?), and he realised the problem only
after committing his botched merge.
When he realised that files were missing from his branch, Attilio ran
'svn add' on unversioned files in his working copy, creating a separate
line of history for these files.
Subversion makes additions and deletions of objects explicit (unlike many
other version control systems), and trusts users to tell it the truth about
whether or not an objected added to a path is genuinely new (and not a copy
of something else).
While I cannot explain why this mistake was made in the first place,
I can show you how to fix it.
First, I'll show you what Attilio should have done when he realised
his mistake.
Merging these additions to largeSMP would have required first reverse-merging
revisions which added the missing files on ^/head, and then merging
these revisions from ^/head again.
For instance, to re-merge the missing addition of the geom_map man page,
Attilio could have done this:
$ svn merge -c-r221501 ^/head
### resolve tree conflict
$ svn merge -cr221501 ^/head
$ svn commit
We can try this on a working copy of ^/projects/largeSMP@221716 (the
revision where the new files had just been added to the branch):
$ svn merge -c-r221501 ^/head
--- Reverse-merging r221501 into '.':
U share/man/man4/Makefile
C share/man/man4/geom_map.4
--- Recording mergeinfo for reverse merge of r221501 into '.':
U .
Summary of conflicts:
Tree conflicts: 1
$ svn status
M .
M share/man/man4/Makefile
C share/man/man4/geom_map.4
> local edit, incoming delete upon merge
Summary of conflicts:
Tree conflicts: 1
Resolving tree conflicts can be hard for novice users because the svn ui
is rather unforgiving when it comes to tree conflicts. Work is underway
to fix this but it will still take a while.
With the current UI, I would resolve this conflict as follows:
Subversion is warning us that the merge is trying to delete geom_map.4,
which we know is a file that was added to locally on this branch. Adding
this file to the branch was a mistake! We want it to be deleted. So we
can resolve the conflict by deleting the file:
$ svn rm share/man/man4/geom_map.4
D share/man/man4/geom_map.4
$ svn resolved share/man/man4/geom_map.4
$ svn status
M .
M share/man/man4/Makefile
D share/man/man4/geom_map.4
Now we can merge r221501 again, this time in the forward direction of
history, to bring in the file we actually want:
$ svn merge -cr221501 ^/head
--- Merging r221501 into '.':
G share/man/man4/Makefile
A share/man/man4/geom_map.4
--- Recording mergeinfo for merge of r221501 into '.':
G .
$ svn st
R + share/man/man4/geom_map.4
$
So the changeset scheduled for commit now now is a replacement of the
geom_map.4 file with its cousin which has the proper line of history:
$ svn info share/man/man4/geom_map.4 | grep ^Copied\ From
Copied From URL: svn://svn.freebsd.org/base/head/share/man/man4/geom_map.4
Copied From Rev: 221501
Since the largeSMP branch has already been merged back to ^/head, the
mistake now needs to be fixed up on ^/head. This could again be done
via appropriate reverse-merges and re-merges, but there's an easier way.
We can reconnect the file to its proper line of history by deleting the
file with the bad line of history and replacing it with the file with
the good line of history.
First, let's check for differences between the latest version of
the file in head and the version from before the largeSMP branch
was reintegrated:
$ svn diff ^/head/share/man/man4/geom_map.4 ^/head/share/man/man4/geom_map.4@r222012
Index: geom_map.4
===================================================================
--- geom_map.4 (revision 246254)
+++ geom_map.4 (revision 222012)
Property changes on: geom_map.4
___________________________________________________________________
Deleted: svn:eol-style
## -1 +0,0 ##
-native
\ No newline at end of property
Deleted: svn:mime-type
## -1 +0,0 ##
-text/plain
\ No newline at end of property
The only information we need to manually preserve are these two properties.
That's easy:
$ svn rm share/man/man4/geom_map.4
D share/man/man4/geom_map.4
$ svn copy ^/head/share/man/man4/com_map.4@r222012 share/man/man4/geom_map.4
A share/man/man4/geom_map.4
1.7.x-power $ svn status
RM + share/man/man4/geom_map.4
$ svn propset svn:eol-style native share/man/man4/geom_map.4
property 'svn:eol-style' set on 'share/man/man4/geom_map.4'
$ svn propset svn:mime-type text/plain share/man/man4/geom_map.4
property 'svn:mime-type' set on 'share/man/man4/geom_map.4'
$ svn status
RM + share/man/man4/geom_map.4
$ svn diff
Index: share/man/man4/geom_map.4
===================================================================
--- share/man/man4/geom_map.4 (revision 246254)
+++ share/man/man4/geom_map.4 (working copy)
Property changes on: share/man/man4/geom_map.4
___________________________________________________________________
Added: svn:mergeinfo
Merged /projects/quota64/share/man/man4/geom_map.4:r184125-207707
Merged /vendor/resolver/dist/share/man/man4/geom_map.4:r1540-186085
Now the only property changes are svn:mergeinfo changes. We don't really
care about these, but always commit them!!!
Notice how log history has been repaired:
$ svn log share/man/man4/geom_map.4
------------------------------------------------------------------------
r222012 | uqs | 2011-05-17 11:51:02 +0200 (Tue, 17 May 2011) | 4 lines
More thorough mdoc and language fixes.
Submitted by: ru
------------------------------------------------------------------------
r222008 | uqs | 2011-05-17 10:12:59 +0200 (Tue, 17 May 2011) | 2 lines
Typos, wording and mdoc fixes.
------------------------------------------------------------------------
r221501 | adrian | 2011-05-05 16:43:35 +0200 (Thu, 05 May 2011) | 4 lines
Add a manpage for geom_map(4).
Submitted by: ray@dlink.ua
------------------------------------------------------------------------
> > > I would just generate a diff and manually apply that to
> > > a HEAD checkout and commit that. You could perhaps svn cp over new files
> > > from the nand branch to HEAD to preserve their history, but I worry that
> > > anything other than diff + patch for existing files risks hosing history.
> >
> > WOAH!! Please lets gain some new experience with 'svn merge' using
> > version 1.7. We do 100's of merges a year at $WORK (with svn 1.6)
> > on a code base 10x that of FreeBSD -- it works.
>
> I've never had problems with merging downstream into work branches. I've
> only seen upstream merges blow up.
>
> --
> John Baldwin
> --
> This mail is for the internal use of the FreeBSD project committers,
> and as such is private. This mail may not be published or forwarded
> outside the FreeBSD committers' group or disclosed to other unauthorised
> parties without the explicit permission of the author(s).
>
> Date: Fri, 1 Feb 2013 09:50:37 -0500
> From: John Baldwin <jh...@freebsd.org>
> To: Alfred Perlstein <br...@mu.org>
> Cc: Peter Wemm <pe...@wemm.org>
> Subject: Re: Fwd: Re: FreeBSD project and subversion.
> X-Spam-Level:
> User-Agent: KMail/1.13.5 (FreeBSD/8.2-CBSD-20110714-p22; KDE/4.5.5; amd64;
> ; )
> Content-Type: Text/Plain; charset="iso-8859-15"
> Message-Id: <20...@freebsd.org>
>
> On Friday, February 01, 2013 7:53:57 am Alfred Perlstein wrote:
> > John and Peter, both of you are +inf more knowledgeable about svn than I am.
> >
> > I see we still try to minimize svn mergeinfo from the FAQ ("Selecting
> > the Source and Target")
> > http://www.freebsd.org/doc/en_US.ISO8859-1/articles/committers-
> guide/subversion-primer.html#AEN771
> >
> > I know I've seen some emails explaining the reasoning behind this but I
> > can't find them. What would the effect of bringing mergeinfo to the top be?
> >
> > Possible problems:
> > 1) it would get very large
> > 2) if we ever were to split up the repo it would be a problem.
> > 3) ... ?
>
> It makes merges from across the continental US take a long time.
>
> > Additionally, what are our concerns about the --reintegrate option
> > (which was added (or at least improved) after we switched)
>
> It trashes history if SVN mucks it up (and it can). This has already happened
> at least once, and I've noted that in a thread on developers.
I would guess that these were similar user errors. Use of the --reintegrate
option is very important. Failure to use it can trigger spurious conflicts
during the merge back to ^/head, and can even exacerbate problems during
future merges from/to any branch.
Subversion 1.8 will finally deprecate this option, which is a _horrible_ UI
quirk because it requires users to reason about the directions their merges
are taking. Subversion 1.8 will automatically figure whether the option is
needed: http://subversion.apache.org/docs/release-notes/1.8.html#auto-merge
But while you're still using Subversion 1.7, please use --reintegrate
when merging branches back into their parent branch.
If this doesn't work in the expected way, mail our users@ list,
describe the problem, and we'll help you. In the long run you will
have a much better Subversion experience this way.
Re: FreeBSD project and subversion.
Posted by Alfred Perlstein <br...@mu.org>.
On 1/31/13 12:08 PM, Stefan Sperling wrote:
> On Thu, Jan 31, 2013 at 10:37:14AM -0500, Alfred Perlstein wrote:
>> FreeBSD has moved to Subversion a few years ago and it's worked
>> very, very well for us.
> Thanks! That's encouraging to hear.
>
>> The one area where we are having issues is merging code from project
>> branches back into trunk.
>>
>> The typical workflow is:
>> 1) create project branch.
>> 2) code code code.
>> 3) sync from HEAD (this works great).
>> 4) repeat steps 2+3 until feature is complete.
>> 5) svn diff and apply to head then commit.
>>
>> Is there a way to do 5 automatically?
>>
>> I think the worry is mergeinfo from the project branch coming back
>> into HEAD.
>>
>> Any tips would be appreciated.
> I've read the FreeBSD svn merging docs some time ago. I'm not sure if
> changes have been made since, but it was probably an older version
> of what currently lives at this URL:
> http://www.freebsd.org/doc/en_US.ISO8859-1/articles/committers-guide/subversion-primer.html
>
> Back then I was somewhat worried that apparently the person who wrote them
> didn't really understand much about how merges in Subversion work.
>
> There was irrational fear of mergeinfo propagation, to the point where
> the recommended practice is to remove mergeinfo before commit, which
> any Subversion user with a clue will know is very wrong (expect in very
> exceptional circumstances and only if you are equipped with sufficient
> expertise to deal with the consequences).
>
> What surprised me also was a complete lack of mention of the --reintegrate
> option, which I suspect must be causing quite a lot of grief among FreeBSD
> developers due to bogus conflicts during merges back into FreeBSD's head
> branch (i.e. the trunk).
>
> We'll need more details to help you in a constructive way, though.
> Can you provide more details about your steps 1 to 5, i.e. show the
> exact commands you're running in each step? The repository is public,
> after all, which will help greatly with identifying and reproducing
> specific problems.
>
> I'm happy to provide input for improving FreeBSD's docs to the point
> where FreeBSD makes the best possible use of Subversion 1.7's merge
> implementation, and can also provide some hints as to how future versions
> of Subversion will improve to make life easier in certain cases.
>
> BTW, I went over one of FreeBSD's svn wiki pages a while back, and added
> answers to open questions on this page:
> https://wiki.freebsd.org/SubversionMissing
Thank you Stefan,
So I have two answers here:
1) about mergeinfo
It seems as if doing it all at the top can make merges over long haul
internet very painful.
2) about reintegrate
I've attached the two messages that show what makes FreeBSD gun shy
about merges to HEAD.
Maybe these issues are resolved, or maybe the developer did something
incorrect, or maybe something else entirely different happened.
See attached.
Thank you,
-Alfred
Re: FreeBSD project and subversion.
Posted by Alexey Neyman <st...@att.net>.
On Tuesday, February 05, 2013 12:59:43 PM Johan Corveleyn wrote:
> On Tue, Feb 5, 2013 at 1:39 AM, Alexey Neyman <st...@att.net> wrote:
> > On Monday, February 04, 2013 10:17:29 PM Stefan Sperling wrote:
> >> On Mon, Feb 04, 2013 at 11:54:21AM -0800, Alexey Neyman wrote:
> >> > Is there a reason why it is necessary to use "the svn diff --old
> >> >
> >> > ^/somebranch --new ." instead of the intuitive "svn ^/somebranch ."?
> >>
> >> Would "svn diff ^/somebranch ." be synopsis 1?
> >>
> >> svn diff -rN:M ^/somebranch@HEAD .@HEAD
> >
> > It cannot be "synopsis 1", as its description states that "TARGETs may be
> > all working copy paths or all URLs". In this case, we have one of each.
> > Moreover, synopsis 1 says that for URLs, -r or -c must be present - and my
> > example has no -r/-c option.
>
> Sorry, that's not true. From 'svn help diff' of 1.7:
>
> diff (di): Display the differences between two revisions or paths.
> usage: 1. diff [-c M | -r N[:M]] [TARGET[@REV]...]
> 2. diff [-r N[:M]] --old=OLD-TGT[@OLDREV] [--new=NEW-TGT[@NEWREV]] \
> [PATH...]
> 3. diff OLD-URL[@OLDREV] NEW-URL[@NEWREV]
>
> So for usage 1 the -c and -r are optional.
You didn't quote the second part of 'svn help diff' description of synopsis 1.
Here it is:
1. Display the changes made to TARGETs as they are seen in REV between
two revisions. TARGETs may be all working copy paths or all URLs.
If TARGETs are working copy paths, N defaults to BASE and M to the
working copy; if URLs, N must be specified and M defaults to HEAD.
The '-c M' option is equivalent to '-r N:M' where N = M-1.
Using '-c -M' does the reverse: '-r M:N' where N = M-1.
If you read it, you'll see that
(a) Mix of URLs and paths are not allowed for this synopsis
(b) For URLs, either -r or -c must be provided
Regards,
Alexey.
y.
Re: [PATCH] Weird 'svn diff' operation with --old/--new (was: Re: FreeBSD project and subversion.)
Posted by Julian Foad <ju...@btopenworld.com>.
Alexey Neyman wrote:
> Julian Foad wrote:
>> Alexey Neyman wrote:
>> > I noticed a weird behavior of 'svn diff --old=... --new=...' where
>> > reversing the old/new targets does not produce the reverse diff, as
>> > one would expect.
> [[[
> Make 'svn diff --old=FOO --new=BAR default to WORKING revision for old target
> if new target is explicitly specified, so that it is the reverse diff from 'svn diff --old=BAR --new=FOO'.
>
> * subversion/svn/diff-cmd.c
> (svn_cl__diff) Change defaults as described and simplify the logic.
> ]]]
Thanks -- I think this all looks good.
Committed in 1443821, with just some minor editing of the comment.
(Incidentally, applying your patch using 'svn patch' led me to discover a bug in 'svn patch' which I have just emailed about.)
- Julian
Re: [PATCH] Weird 'svn diff' operation with --old/--new (was: Re: FreeBSD project and subversion.)
Posted by Alexey Neyman <st...@att.net>.
Hi Julian,
On Thursday, February 07, 2013 04:00:49 PM Julian Foad wrote:
> Alexey Neyman wrote:
> > [Cross-posting to dev@ for patch review]
> > I noticed a weird behavior of 'svn diff --old=... --new=...' where
> > reversing the old/new targets does not produce the reverse diff, as
> > one would expect.
>
> Hi Alexey. Thanks for the report. As you pointed out, I changed something
> in this area recently, so I'll take a closer look and fix it. I haven't
> done so yet but should get to it in the next day or two.
>
> [...]
Thanks!
[... snip ...]
> > [[[
> > Make svn diff --old=.. --new=.. default to WORKING revision for old target
> > if new target is explicitly specified.
>
> A log message should always indicate *why* the change was made.
Hope this is better:
[[[
Make 'svn diff --old=FOO --new=BAR default to WORKING revision for old target
if new target is explicitly specified, so that it is the reverse diff from
'svn diff --old=BAR --new=FOO'.
* subversion/svn/diff-cmd.c
(svn_cl__diff) Change defaults as described and simplify the logic.
]]]
Regards,
Alexey.
Re: [PATCH] Weird 'svn diff' operation with --old/--new (was: Re: FreeBSD project and subversion.)
Posted by Julian Foad <ju...@btopenworld.com>.
Alexey Neyman wrote:
> [Cross-posting to dev@ for patch review]
> I noticed a weird behavior of 'svn diff --old=... --new=...' where
> reversing the old/new targets does not produce the reverse diff, as
> one would expect.
Hi Alexey. Thanks for the report. As you pointed out, I changed something in this area recently, so I'll take a closer look and fix it. I haven't done so yet but should get to it in the next day or two.
[...]
> The reason for this behavior is that the code in diff-cmd.c makes the
> following defaults for the old/new revisions:
>
> if (opt_state->start_revision.kind == svn_opt_revision_unspecified)
> opt_state->start_revision.kind = svn_path_is_url(old_target)
> ? svn_opt_revision_head : svn_opt_revision_base;
>
> if (opt_state->end_revision.kind == svn_opt_revision_unspecified)
> opt_state->end_revision.kind = svn_path_is_url(new_target)
> ? svn_opt_revision_head : svn_opt_revision_working;
>
> These defaults make some sense, if the new target is not specified (it
> defaults to old target in that case). If the new target is specified,
> and is different from from old target, I am not so sure if it makes
> sense. Note that there is a special case if both old/new targets are
> WC paths - in that case, both old and new targets default to
> svn_opt_revision_working. So,
>
> $ svn diff --old=foo --new=bar
>
> will compare MODIFIED version of 'foo' against modified version of
> 'bar' [*], but
>
> $ svn diff --old=foo --new=^/bar
>
> would compare BASE version of 'foo' against head version of 'bar.
> Does it make sense? I would say, if both old and new are specified,
> old target's revision should default to 'working' as well.
>
> Problem is further aggravated since there is no commandline alias
> to request svn_opt_revision_working setting from the command line:
> only 'head', 'prev', 'base' and 'committed' are currently available.
> Hence, it is not possible to request the exact opposite of the patch
> using -rN:M syntax if one of the --old/--new targets is a working
> copy path. Is there a reason why 'working' is not parsed by
> revision_from_word()?
>
> Also note that 'svn help diff' does not describe revision defaults
> for synopsis 2 (and the default is different from synopsis 1, which
> requires old revision to be specified for URLs).
>
> PS. Stephan apparently made 'working' revision as default in his
> check-in (r1442640) at least for the short-hand form, but Julian
> Foad, "optimizing the logic" in r1442659 and r1442676, switched it
> back to be the same as --old/--new logic.
Oops, I didn't notice I was changing the logic. As my log message said, I thought I was just simplifying it.
> PPS. If agreed to suggested change of default, the patch is attached.
>
> [[[
> Make svn diff --old=.. --new=.. default to WORKING revision for old target
> if new target is explicitly specified.
A log message should always indicate *why* the change was made.
>
> * subversion/svn/diff-cmd.c
> (svn_cl__diff) Change defaults as described and simplify the logic.
> ]]]
- Julian
[PATCH] Weird 'svn diff' operation with --old/--new (was: Re: FreeBSD project and subversion.)
Posted by Alexey Neyman <st...@att.net>.
[Cross-posting to dev@ for patch review]
Hi,
I noticed a weird behavior of 'svn diff --old=... --new=...' where reversing
the old/new targets does not produce the reverse diff, as one would expect.
Here is the script for reproduction:
[[[
#!/bin/bash
REPO=/tmp/foo
WC=/tmp/foo.wc
rm -rf $REPO $WC
svnadmin create $REPO
svn co -q file://$REPO $WC
cd $WC
echo r1 > a
svn add -q a
svn ci -q -m R1
echo r2 > a
svn ci -q -m R2
svn up -q -r 1
echo new > a
echo "Issue: --old=A --new=B is not opposite of --old=B --new=A"
echo " Running: svn di --old=^/ --new=."
svn di --old=^/ --new=.
echo " Running: svn di --old=. --new=^/"
svn di --old=. --new=^/
]]]
And here is the output (svn 1.7.6):
[[[
Issue: --old=A --new=B is not opposite of --old=B --new=A
Running: svn di --old=^/ --new=.
Index: a
===================================================================
--- a (.../file:///tmp/foo) (revision 2)
+++ a (working copy)
@@ -1 +1 @@
-r2
+new
Running: svn di --old=. --new=^/
Index: a
===================================================================
--- a (working copy)
+++ a (.../file:///tmp/foo) (revision 2)
@@ -1 +1 @@
-r1
+r2
]]]
The reason for this behavior is that the code in diff-cmd.c makes the
following defaults for the old/new revisions:
if (opt_state->start_revision.kind == svn_opt_revision_unspecified)
opt_state->start_revision.kind = svn_path_is_url(old_target)
? svn_opt_revision_head : svn_opt_revision_base;
if (opt_state->end_revision.kind == svn_opt_revision_unspecified)
opt_state->end_revision.kind = svn_path_is_url(new_target)
? svn_opt_revision_head : svn_opt_revision_working;
These defaults make some sense, if the new target is not specified (it
defaults to old target in that case). If the new target is specified, and is
different from from old target, I am not so sure if it makes sense. Note that
there is a special case if both old/new targets are WC paths - in that case,
both old and new targets default to svn_opt_revision_working. So,
$ svn diff --old=foo --new=bar
will compare MODIFIED version of 'foo' against modified version of 'bar' [*],
but
$ svn diff --old=foo --new=^/bar
would compare BASE version of 'foo' against head version of 'bar. Does it make
sense? I would say, if both old and new are specified, old target's revision
should default to 'working' as well.
Problem is further aggravated since there is no commandline alias to request
svn_opt_revision_working setting from the command line: only 'head', 'prev',
'base' and 'committed' are currently available. Hence, it is not possible to
request the exact opposite of the patch using -rN:M syntax if one of the --
old/--new targets is a working copy path. Is there a reason why 'working' is
not parsed by revision_from_word()?
Also note that 'svn help diff' does not describe revision defaults for
synopsis 2 (and the default is different from synopsis 1, which requires old
revision to be specified for URLs).
PS. Stephan apparently made 'working' revision as default in his check-in
(r1442640) at least for the short-hand form, but Julian Foad, "optimizing the
logic" in r1442659 and r1442676, switched it back to be the same as --old/--
new logic.
PPS. If agreed to suggested change of default, the patch is attached.
[[[
Make svn diff --old=.. --new=.. default to WORKING revision for old target
if new target is explicitly specified.
* subversion/svn/diff-cmd.c
(svn_cl__diff) Change defaults as described and simplify the logic.
]]]
Regards,
Alexey.
Re: FreeBSD project and subversion.
Posted by Alexey Neyman <st...@att.net>.
On Tuesday, February 05, 2013 01:55:20 PM Lathan Bidwell wrote:
> On 02/05/2013 01:14 PM, Stefan Sperling wrote:
> > On Tue, Feb 05, 2013 at 09:19:19AM -0800, Alexey Neyman wrote:
> >> There is one more weird issue with svn diff, see the script below. The
> >> issue is that "--old=A --new=B" is not opposite of "--old=B --new=A". I
> >> don't know if it is a bug or another ambuguity I am not aware of :)
> >>
> >> Here is the script:
> >> [[[
> >> #!/bin/bash
> >>
> >> REPO=/tmp/foo
> >> WC=/tmp/foo.wc
> >>
> >> rm -rf $REPO $WC
> >> svnadmin create $REPO
> >> svn co -q file://$REPO $WC
> >> cd $WC
> >>
> >> echo r1 > a
> >> svn add -q a
> >> svn ci -q -m R1
> >> echo r2 > a
> >> svn ci -q -m R2
> >> svn up -q -r 1
> >> echo new > a
> >> echo "Issue: --old=A --new=B is not opposite of --old=B --new=A"
> >> echo " Running: svn di --old=^/ --new=."
> >> svn di --old=^/ --new=.
> >> echo " Running: svn di --old=. --new=^/"
> >> svn di --old=. --new=^/
> >> ]]]
> >>
> >> And here is the output (svn 1.7.6):
> >> [[[
> >> Issue: --old=A --new=B is not opposite of --old=B --new=A
> >>
> >> Running: svn di --old=^/ --new=.
> >>
> >> Index: a
> >> ===================================================================
> >> --- a (.../file:///tmp/foo) (revision 2)
> >> +++ a (working copy)
> >> @@ -1 +1 @@
> >> -r2
> >> +new
> >>
> >> Running: svn di --old=. --new=^/
> >>
> >> Index: a
> >> ===================================================================
> >> --- a (working copy)
> >> +++ a (.../file:///tmp/foo) (revision 2)
> >> @@ -1 +1 @@
> >> -r1
> >> +r2
> >> ]]]
> >>
> >> Regards,
> >> Alexey.
> >
> > I can reproduce this with a trunk build. Can you please file an issue
> > for this? I'm not going to get to this right away. Thanks!
>
> Here is the issue that I see:
>
> The --old=. get's the workspace version of ., but --new get's the
> non-changed version of /.
>
> So, I believe it is comparing "r1" with the r2 contents of "r2" and not
> comparing both workspace versions.
>
> Rev Contents
> 1 r1
> 2 r2
> 2* new
>
> 2* only get's referenced when it is in the --old position,
> 2 get's used when its in the --new position.
>
> Please correct me if I'm wrong about this, but that is what seems to be
> a logical reasoning behind that behavior.
First, it is just the opposite: 2* gets referenced when it is in the --new,
not --old position. Second, it is not the reasoning (why it behaves so) but
rather a description of current behavior, which I think was rather obvious
from the example script.
The reason for this behavior is that the code in diff-cmd.c makes the
following defaults for the old/new revisions:
if (opt_state->start_revision.kind == svn_opt_revision_unspecified)
opt_state->start_revision.kind = svn_path_is_url(old_target)
? svn_opt_revision_head : svn_opt_revision_base;
if (opt_state->end_revision.kind == svn_opt_revision_unspecified)
opt_state->end_revision.kind = svn_path_is_url(new_target)
? svn_opt_revision_head : svn_opt_revision_working;
These defaults make some sense, given that new target is optional and defaults
to old target if not specified. If new target is specified, and is different
from from old target, I am not so sure if it makes sense. Note that there is a
special case if both old/new targets are WC paths - in that case, both old and
new targets default to svn_opt_revision_working. So,
$ svn diff --old=foo --new=bar
will compare modified version of 'foo' against modified version of 'bar' [*],
but
$ svn diff --old=foo --new=^/bar
would compare base version of 'foo' against HEAD version of 'bar. Does it make
sense? I would say, if both old and new are specified, old target's revision
should default to 'working' as well.
Problem is further aggravated since there is no commandline alias to request
svn_opt_revision_working setting from the command line: only 'head', 'prev',
'base' and 'committed' are currently available. Hence, it is not possible to
request the exact opposite of the patch using -rN:M syntax if one of the --
old/--new targets is a working copy path. Is there a reason why 'working' is
not parsed by revision_from_word()?
Also note that 'svn help diff' does not describe revision defaults for
synopsis 2 (and the default is different from synopsis 1, which requires old
revision to be specified for URLs).
PS. Stephan apparently made 'working' revision as default in his check-in
(r1442640) at least for the short-hand form, but Julian Foad, "optimizing the
logic" in r1442659 and r1442676, switched it back to be the same as --old/--
new logic.
PPS. If agreed to suggested change of default, the patch is attached.
[[[
Make svn diff --old=.. --new=.. default to WORKING revision for old target
if new target is explicitly specified.
* subversion/svn/diff-cmd.c
(svn_cl__diff) Change defaults as described and simplify the logic.
]]]
Regards,
Alexey.
Re: FreeBSD project and subversion.
Posted by Lathan Bidwell <la...@andrews.edu>.
On 02/05/2013 01:14 PM, Stefan Sperling wrote:
> On Tue, Feb 05, 2013 at 09:19:19AM -0800, Alexey Neyman wrote:
>> There is one more weird issue with svn diff, see the script below. The issue
>> is that "--old=A --new=B" is not opposite of "--old=B --new=A". I don't know
>> if it is a bug or another ambuguity I am not aware of :)
>>
>> Here is the script:
>> [[[
>> #!/bin/bash
>>
>> REPO=/tmp/foo
>> WC=/tmp/foo.wc
>>
>> rm -rf $REPO $WC
>> svnadmin create $REPO
>> svn co -q file://$REPO $WC
>> cd $WC
>>
>> echo r1 > a
>> svn add -q a
>> svn ci -q -m R1
>> echo r2 > a
>> svn ci -q -m R2
>> svn up -q -r 1
>> echo new > a
>> echo "Issue: --old=A --new=B is not opposite of --old=B --new=A"
>> echo " Running: svn di --old=^/ --new=."
>> svn di --old=^/ --new=.
>> echo " Running: svn di --old=. --new=^/"
>> svn di --old=. --new=^/
>> ]]]
>>
>> And here is the output (svn 1.7.6):
>> [[[
>> Issue: --old=A --new=B is not opposite of --old=B --new=A
>> Running: svn di --old=^/ --new=.
>> Index: a
>> ===================================================================
>> --- a (.../file:///tmp/foo) (revision 2)
>> +++ a (working copy)
>> @@ -1 +1 @@
>> -r2
>> +new
>> Running: svn di --old=. --new=^/
>> Index: a
>> ===================================================================
>> --- a (working copy)
>> +++ a (.../file:///tmp/foo) (revision 2)
>> @@ -1 +1 @@
>> -r1
>> +r2
>> ]]]
>>
>> Regards,
>> Alexey.
> I can reproduce this with a trunk build. Can you please file an issue
> for this? I'm not going to get to this right away. Thanks!
>
>
Here is the issue that I see:
The --old=. get's the workspace version of ., but --new get's the
non-changed version of /.
So, I believe it is comparing "r1" with the r2 contents of "r2" and not
comparing both workspace versions.
Rev Contents
1 r1
2 r2
2* new
2* only get's referenced when it is in the --old position,
2 get's used when its in the --new position.
Please correct me if I'm wrong about this, but that is what seems to be
a logical reasoning behind that behavior.
Lathan
Re: FreeBSD project and subversion.
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Feb 05, 2013 at 09:19:19AM -0800, Alexey Neyman wrote:
> There is one more weird issue with svn diff, see the script below. The issue
> is that "--old=A --new=B" is not opposite of "--old=B --new=A". I don't know
> if it is a bug or another ambuguity I am not aware of :)
>
> Here is the script:
> [[[
> #!/bin/bash
>
> REPO=/tmp/foo
> WC=/tmp/foo.wc
>
> rm -rf $REPO $WC
> svnadmin create $REPO
> svn co -q file://$REPO $WC
> cd $WC
>
> echo r1 > a
> svn add -q a
> svn ci -q -m R1
> echo r2 > a
> svn ci -q -m R2
> svn up -q -r 1
> echo new > a
> echo "Issue: --old=A --new=B is not opposite of --old=B --new=A"
> echo " Running: svn di --old=^/ --new=."
> svn di --old=^/ --new=.
> echo " Running: svn di --old=. --new=^/"
> svn di --old=. --new=^/
> ]]]
>
> And here is the output (svn 1.7.6):
> [[[
> Issue: --old=A --new=B is not opposite of --old=B --new=A
> Running: svn di --old=^/ --new=.
> Index: a
> ===================================================================
> --- a (.../file:///tmp/foo) (revision 2)
> +++ a (working copy)
> @@ -1 +1 @@
> -r2
> +new
> Running: svn di --old=. --new=^/
> Index: a
> ===================================================================
> --- a (working copy)
> +++ a (.../file:///tmp/foo) (revision 2)
> @@ -1 +1 @@
> -r1
> +r2
> ]]]
>
> Regards,
> Alexey.
I can reproduce this with a trunk build. Can you please file an issue
for this? I'm not going to get to this right away. Thanks!
Re: FreeBSD project and subversion.
Posted by Alexey Neyman <st...@att.net>.
On Tuesday, February 05, 2013 05:08:40 PM Stefan Sperling wrote:
> On Tue, Feb 05, 2013 at 12:59:43PM +0100, Johan Corveleyn wrote:
> > Given what I said above, this can't be done in general (not if you
> > give two URLs or two wc paths, because then both synopsis 1 and 2 are
> > possible -- it would obviously break backwards compatibility if we
> > would now start erroring out on these, while these used to work fine
> > with automatically picking usage 1 before). Only if one of them is a
> > URL and the other a WC path, because we give an error message then
> > anyway.
>
> I think Alexey meant only the cases where we currently error out, i.e. where
> exactly two targets are given and one is a path and the other is a URL.
>
> I didn't realise this but there is indeed no ambiguity in allowing this
> syntax as a shorthand for a corresponding '--old X --new Y' diff invocation.
>
> Thanks Alexey, see http://svn.apache.org/r1442640
> Making these invocations produce a diff instead of an error makes things
> less surprising for users.
Thanks!
There is one more weird issue with svn diff, see the script below. The issue
is that "--old=A --new=B" is not opposite of "--old=B --new=A". I don't know
if it is a bug or another ambuguity I am not aware of :)
Here is the script:
[[[
#!/bin/bash
REPO=/tmp/foo
WC=/tmp/foo.wc
rm -rf $REPO $WC
svnadmin create $REPO
svn co -q file://$REPO $WC
cd $WC
echo r1 > a
svn add -q a
svn ci -q -m R1
echo r2 > a
svn ci -q -m R2
svn up -q -r 1
echo new > a
echo "Issue: --old=A --new=B is not opposite of --old=B --new=A"
echo " Running: svn di --old=^/ --new=."
svn di --old=^/ --new=.
echo " Running: svn di --old=. --new=^/"
svn di --old=. --new=^/
]]]
And here is the output (svn 1.7.6):
[[[
Issue: --old=A --new=B is not opposite of --old=B --new=A
Running: svn di --old=^/ --new=.
Index: a
===================================================================
--- a (.../file:///tmp/foo) (revision 2)
+++ a (working copy)
@@ -1 +1 @@
-r2
+new
Running: svn di --old=. --new=^/
Index: a
===================================================================
--- a (working copy)
+++ a (.../file:///tmp/foo) (revision 2)
@@ -1 +1 @@
-r1
+r2
]]]
Regards,
Alexey.
Re: FreeBSD project and subversion.
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Feb 06, 2013 at 07:21:04AM +0100, Branko Čibej wrote:
> Given all the above, I think we should find a way to warn users -- with
> a one-line note in the header of the diff output, for example? -- about
> the cases where the two-parameter diff could be ambiguous (which, I
> believe, is when both parametrs are WC paths or URLs); and further, when
> any of the paths are unversioned.
We've always had the following 2-params cases (where no -r or -c
options are present):
diff WC-PATH WC-PATH -> show diff against BASE for each target
diff URL1 URL2 -> show diff between URLs
We've now added "URL WC-PATH" and "WC-PATH URL" cases to the mix,
in response to a user who expected these to behave like "URL1 URL2",
and was surprised to learn that they were erroring out instead:
diff URL WC-PATH -> show diff between URL and WC-PATH
diff WC-PATH URL -> show diff between WC-PATH and URL
I think there is a case to be made for keeping these.
Because the odd one out here is the "WC-PATH WC-PATH" case.
As you stated this case had to be compatible to CVS. It has always behaved
differently to the "URL1 URL2" case. So users have already been "confused"
with 2-param diff behaviour depending on the target type.
Hence I don't think adding the new cases makes things any worse.
I still think it's a usability enhancement because the user gets a
reasonable diff instead of an error.
But I can see that adding unversioned paths to the mix would increase
the likelihood of user confusion. That would open the door for 'svn diff
PATH1 PATH2' to produce either a diff against BASE for each file if
both files are versioned, or a diff between PATH1 and PATH2 if either
or both of PATH1 are unversioned. That's asking for trouble...
Re: 2-params diff (was: Re: FreeBSD project and subversion.)
Posted by Les Mikesell <le...@gmail.com>.
On Wed, Feb 6, 2013 at 6:23 AM, Stefan Sperling <st...@elego.de> wrote:
> On Wed, Feb 06, 2013 at 12:41:05PM +0100, Branko Čibej wrote:
>> One other option would be, as was noted elsewhere in this thread, to
>> invent a new subcommand for tree comparisons, keeping only the
>> historical differences feature in "svn diff".
>>
>> That would solve the ambiguity, but it would (a) break backward
>> compatibility, and (b) invent yet another subcommand, which we don't like.
>
> I don't think we need to change how 'svn diff' works. A new subcommand
> could simply provide nicer syntax than 'svn diff' for some use cases.
> We can keep 'svn diff' working as it is.
>
> I don't think we should ever change how 'svn diff URL1 URL2' and
> 'svn diff WC-PATH1 WC-PATH2' work.
>
> But a new subcommand could behave differently in the 'WC-PATH1 WC-PATH2'
> case, and compare WC-PATH1 to WC-PATH2. It would essentially be an
> alias for 'diff --old --new'. It may need to overlap a lot with 'svn diff',
> for options such as --summarize perhaps, and all the little options that
> control little aspects of the diff (--ignore-properties,
> --show-copies-as-adds, etc). Would it be worth adding? Not sure.
>
>> Yet another option would be to introduce a new option, so instead of using
>>
>> svn diff --old A --new B
>>
>> you'd get
>>
>> svn diff --compare A B
>>
>> to generate a diff between the two targets, and plain
>>
>> svn diff A B
>>
>> to get the historical diff of each target.
>
> I don't think adding a new option to 'svn diff' would help at all.
> The point is to unclutter the user interface by providing a more
> straightforward way of comparing two paths/URLs with each other.
> Another option (especially one that overlaps with functionality provided by
> existing --old and --new options) would make the UI even more complicated
> than it already is.
>From a naive user's point of view, if you call it 'diff' you should
expect it to take two arguments and give you the difference like the
well known thing called diff. If it doesn't do that, why call it diff
and if it doesn't do that in the case of one of those arguments
being a working copy target and the other a URL it should tell you the
right syntax for that operation instead of pretending it can't do it.
And if working copy references are sometimes magically/silently
converted to HEAD when used as the target of --old or --new (and
that's desirable for some reason I don't understand...) maybe you need
something like @WC to show that when you reference the working copy
path you really mean the working copy instance.
--
Les Mikesell
lesmikesell@gmail.com
2-params diff (was: Re: FreeBSD project and subversion.)
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Feb 06, 2013 at 12:41:05PM +0100, Branko Čibej wrote:
> One other option would be, as was noted elsewhere in this thread, to
> invent a new subcommand for tree comparisons, keeping only the
> historical differences feature in "svn diff".
>
> That would solve the ambiguity, but it would (a) break backward
> compatibility, and (b) invent yet another subcommand, which we don't like.
I don't think we need to change how 'svn diff' works. A new subcommand
could simply provide nicer syntax than 'svn diff' for some use cases.
We can keep 'svn diff' working as it is.
I don't think we should ever change how 'svn diff URL1 URL2' and
'svn diff WC-PATH1 WC-PATH2' work.
But a new subcommand could behave differently in the 'WC-PATH1 WC-PATH2'
case, and compare WC-PATH1 to WC-PATH2. It would essentially be an
alias for 'diff --old --new'. It may need to overlap a lot with 'svn diff',
for options such as --summarize perhaps, and all the little options that
control little aspects of the diff (--ignore-properties,
--show-copies-as-adds, etc). Would it be worth adding? Not sure.
> Yet another option would be to introduce a new option, so instead of using
>
> svn diff --old A --new B
>
> you'd get
>
> svn diff --compare A B
>
> to generate a diff between the two targets, and plain
>
> svn diff A B
>
> to get the historical diff of each target.
I don't think adding a new option to 'svn diff' would help at all.
The point is to unclutter the user interface by providing a more
straightforward way of comparing two paths/URLs with each other.
Another option (especially one that overlaps with functionality provided by
existing --old and --new options) would make the UI even more complicated
than it already is.
Re: FreeBSD project and subversion.
Posted by Branko Čibej <br...@wandisco.com>.
On 06.02.2013 07:21, Branko Čibej wrote:
> Given all the above, I think we should find a way to warn users --
> with a one-line note in the header of the diff output, for example? --
> about the cases where the two-parameter diff could be ambiguous
> (which, I believe, is when both parametrs are WC paths or URLs); and
> further, when any of the paths are unversioned.
One other option would be, as was noted elsewhere in this thread, to
invent a new subcommand for tree comparisons, keeping only the
historical differences feature in "svn diff".
That would solve the ambiguity, but it would (a) break backward
compatibility, and (b) invent yet another subcommand, which we don't like.
Yet another option would be to introduce a new option, so instead of using
svn diff --old A --new B
you'd get
svn diff --compare A B
to generate a diff between the two targets, and plain
svn diff A B
to get the historical diff of each target.
Unfortunately this option has all the drawbacks of the new subcommand,
and none of the benefits (the benefit being that with a subcommand, the
help text could be shorter, since that's tied to the subcommand, not
individual options).
-- Brane
--
Branko Čibej
Director of Subversion | WANdisco | www.wandisco.com
Re: FreeBSD project and subversion.
Posted by Branko Čibej <br...@wandisco.com>.
On 06.02.2013 00:58, Stefan Sperling wrote:
> Joke aside, I'm not sure if there is a really good way to resolve
> these ambiguities. I don't really like that fact that we've got these
> --old and --new options in the first place. It seems like a crutch for
> a lack of a better syntax that can express all use cases equally well
> without being rather weird and uncommon. But since the syntax is
> present in releases we have to keep supporting it...
I agree about not liking --old and --new. But we went through exactly
the same arguments back in the day when they were invented, and the
argument that won was against confusing users with the fact that:
$ svn diff param1 param2
could have two completely different meanings based on some magic
involved in interpreted the parameters, whereas:
$ svn diff param1
or
$ svn diff param1 param2 param3
can each have only one meaning.
I think the only way out of adding --new and --old, given the premise
that user confusion was absolutely evil (which I agreed with and still
do), would have been to have only two forms of "svn diff" -- the
one-parameter form that produces a historical diff, and the
two-parameter form which would produce a diff between the two targets.
However, that was seen as insufficient, given the (then) goal of CVS
feature parity, and simple usability required that we had a way to
produce historical diffs for N targets (where N >= 1).
Given all the above, I think we should find a way to warn users -- with
a one-line note in the header of the diff output, for example? -- about
the cases where the two-parameter diff could be ambiguous (which, I
believe, is when both parametrs are WC paths or URLs); and further, when
any of the paths are unversioned.
-- Brane
--
Branko Čibej
Director of Subversion | WANdisco | www.wandisco.com
Re: FreeBSD project and subversion.
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Feb 06, 2013 at 12:30:05AM +0100, Johan Corveleyn wrote:
> Nah, it's fine, I think I'm starting to get convinced ;-). But it's
> still a pity that there is no relief (at least I don't see any) for
> confusion around the fact that 'svn diff WC-PATH1 WC-PATH2' doesn't
> diff WC-PATH1 and WC-PATH2 against each other.
I agree very much that it is rather confusing that
svn diff WC-PATH1 WC-PATH1
does something else than:
svn diff --old WC-PATH1 --new WC-PATH2
However, I think that the current help text makes the difference clear,
if only the user cares to read it.
As long as the documentation is clear enough, such subtleties are fine
with me. I mean that I'll probably be able to remember them and explain them
to people -- which is what I'm supposed to be doing at $DAYJOB -- hence
that's why I'm fine with it :)
Joke aside, I'm not sure if there is a really good way to resolve these
ambiguities. I don't really like that fact that we've got these --old and
--new options in the first place. It seems like a crutch for a lack of a
better syntax that can express all use cases equally well without being
rather weird and uncommon. But since the syntax is present in releases we
have to keep supporting it...
> The shorthands with the unversioned paths are quite nice I think, but
> there is an additional risk of confusion: 'svn diff UNVERSIONED-PATH1
> UNVERSIONED-PATH2' will mean something totally different than 'svn
> diff WC-PATH1 WC-PATH2' (even though they could both refer to the same
> local files, but the commands are separated by an invocation of 'svn
> add').
Yeah, it is going to make things even more confusing, of course!
But only for those who type the diff command and expect a different
result than they're getting :)
> > The error message is triggered by an invocation like:
> >
> > svn diff -c42 foo ^/trunk bar
> >
> > Which is clearly invalid usage according to 'svn help diff'.
>
> Yes, but in this case the user gives 3 targets, so he clearly couldn't
> have meant one of the shorthand syntaxes, right?
Only if the user did not understand the (somewhat obscure by nature) help
text... The warning is really meant to get people who provide nonsense
arguments to read 'svn help diff'.
Re: FreeBSD project and subversion.
Posted by Johan Corveleyn <jc...@gmail.com>.
On Wed, Feb 6, 2013 at 12:07 AM, Stefan Sperling <st...@apache.org> wrote:
> On Tue, Feb 05, 2013 at 09:18:33PM +0100, Johan Corveleyn wrote:
>> Hmm, okay, but I would have preferred some more discussion before you
>> implemented this. It's not clear to me that this decreases the
>> surprises. I can perfectly imagine just as many questions being asked
>> on users@ with the question: "Why does 'svn diff left.txt right.txt'
>> give me an empty diff, while 'svn diff ^/left.txt right.txt' does the
>> right thing?"
>>
>> I think it would be better to consistently point users to the
>> --old/--new syntax, so as to educate users more (so I'm okay with the
>> change in the error message), not hide it even more from them.
>
> Well, the point that convinced me was that previously 'svn diff' raised
> an error in the case of 'svn diff ^/left.txt right.txt'.
> That's clearly intended to be a comparison between a URL and a path,
> which the diff code already supports. So I think the diff code should
> just show an appropriate diff. I don't see how people could be conflating
> a URL->WC or WC->URL diff with a WC->WC diff.
>
> BTW, I'm thinking about adding more shortcuts:
>
> svn diff UNVERSIONED-PATH WCPATH
> svn diff WCPATH UNVERSIONED-PATH
> svn diff UNVERSIONED-PATH1 UNVERSIONED-PATH2
>
> All of which are supported by 'svn diff --old=X --new=Y' (in 1.8-to-be,
> not in 1.7), but not by plain 'svn diff'.
>
> The --old --new syntax is specific to Subversion. Many users who are
> accustomed to "standard" diff commands will not try using --old and --new
> (unless they've taken the time to read 'svn help diff', of course, which
> apparently is too much time to ask from some users ;)
>
> That said, if you'd rather just print the error message instead, that's
> fine with me, too. I don't care strongly enough about this to the point
> where I'd say that we _must_ make this change to 'svn diff'. It's been
> committed, but that doesn't mean that this has to be in 1.8.0 at all costs.
Nah, it's fine, I think I'm starting to get convinced ;-). But it's
still a pity that there is no relief (at least I don't see any) for
confusion around the fact that 'svn diff WC-PATH1 WC-PATH2' doesn't
diff WC-PATH1 and WC-PATH2 against each other.
The shorthands with the unversioned paths are quite nice I think, but
there is an additional risk of confusion: 'svn diff UNVERSIONED-PATH1
UNVERSIONED-PATH2' will mean something totally different than 'svn
diff WC-PATH1 WC-PATH2' (even though they could both refer to the same
local files, but the commands are separated by an invocation of 'svn
add').
>> Speaking of the error message:
>
>> How is this useful after r1442640? Seems to me we should either
>> improve the error message (which would be my preferred option) *or*
>> make 'svn diff' handle these automatically as a shorthand syntax
>> (hence no error message). But not both.
>>
>> Or am I missing something?
>
> The error message is triggered by an invocation like:
>
> svn diff -c42 foo ^/trunk bar
>
> Which is clearly invalid usage according to 'svn help diff'.
Yes, but in this case the user gives 3 targets, so he clearly couldn't
have meant one of the shorthand syntaxes, right?
--
Johan
RE: FreeBSD project and subversion.
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Stefan Sperling [mailto:stsp@apache.org]
> Sent: woensdag 6 februari 2013 00:08
> To: Johan Corveleyn
> Cc: Alexey Neyman; users@subversion.apache.org; Alfred Perlstein
> Subject: Re: FreeBSD project and subversion.
>
> On Tue, Feb 05, 2013 at 09:18:33PM +0100, Johan Corveleyn wrote:
> > Hmm, okay, but I would have preferred some more discussion before you
> > implemented this. It's not clear to me that this decreases the
> > surprises. I can perfectly imagine just as many questions being asked
> > on users@ with the question: "Why does 'svn diff left.txt right.txt'
> > give me an empty diff, while 'svn diff ^/left.txt right.txt' does the
> > right thing?"
> >
> > I think it would be better to consistently point users to the
> > --old/--new syntax, so as to educate users more (so I'm okay with the
> > change in the error message), not hide it even more from them.
>
> Well, the point that convinced me was that previously 'svn diff' raised
> an error in the case of 'svn diff ^/left.txt right.txt'.
> That's clearly intended to be a comparison between a URL and a path,
> which the diff code already supports. So I think the diff code should
> just show an appropriate diff. I don't see how people could be conflating
> a URL->WC or WC->URL diff with a WC->WC diff.
>
> BTW, I'm thinking about adding more shortcuts:
>
> svn diff UNVERSIONED-PATH WCPATH
> svn diff WCPATH UNVERSIONED-PATH
> svn diff UNVERSIONED-PATH1 UNVERSIONED-PATH2
>
> All of which are supported by 'svn diff --old=X --new=Y' (in 1.8-to-be,
> not in 1.7), but not by plain 'svn diff'.
I'm not sure if I like these variants with unversioned paths.
This might make users think that these paths are versioned and without
warning who wouldn't expect that?
The new unversioned/arbitrary diff is nice in specific situations, but it
shouldn't' break the normal cases where 'svn diff *' would do one thing
completely differently if it happens to match exactly two files, where one
is versioned and one is not, or both are unversioned.
If we want something like that we should support that with a flag like
--unversioned (better name suggestions welcome)
I really like the url vs path diff changes though.
Bert
Re: FreeBSD project and subversion.
Posted by Stefan Sperling <st...@apache.org>.
On Tue, Feb 05, 2013 at 09:18:33PM +0100, Johan Corveleyn wrote:
> Hmm, okay, but I would have preferred some more discussion before you
> implemented this. It's not clear to me that this decreases the
> surprises. I can perfectly imagine just as many questions being asked
> on users@ with the question: "Why does 'svn diff left.txt right.txt'
> give me an empty diff, while 'svn diff ^/left.txt right.txt' does the
> right thing?"
>
> I think it would be better to consistently point users to the
> --old/--new syntax, so as to educate users more (so I'm okay with the
> change in the error message), not hide it even more from them.
Well, the point that convinced me was that previously 'svn diff' raised
an error in the case of 'svn diff ^/left.txt right.txt'.
That's clearly intended to be a comparison between a URL and a path,
which the diff code already supports. So I think the diff code should
just show an appropriate diff. I don't see how people could be conflating
a URL->WC or WC->URL diff with a WC->WC diff.
BTW, I'm thinking about adding more shortcuts:
svn diff UNVERSIONED-PATH WCPATH
svn diff WCPATH UNVERSIONED-PATH
svn diff UNVERSIONED-PATH1 UNVERSIONED-PATH2
All of which are supported by 'svn diff --old=X --new=Y' (in 1.8-to-be,
not in 1.7), but not by plain 'svn diff'.
The --old --new syntax is specific to Subversion. Many users who are
accustomed to "standard" diff commands will not try using --old and --new
(unless they've taken the time to read 'svn help diff', of course, which
apparently is too much time to ask from some users ;)
That said, if you'd rather just print the error message instead, that's
fine with me, too. I don't care strongly enough about this to the point
where I'd say that we _must_ make this change to 'svn diff'. It's been
committed, but that doesn't mean that this has to be in 1.8.0 at all costs.
> Speaking of the error message:
> How is this useful after r1442640? Seems to me we should either
> improve the error message (which would be my preferred option) *or*
> make 'svn diff' handle these automatically as a shorthand syntax
> (hence no error message). But not both.
>
> Or am I missing something?
The error message is triggered by an invocation like:
svn diff -c42 foo ^/trunk bar
Which is clearly invalid usage according to 'svn help diff'.
Re: FreeBSD project and subversion.
Posted by Johan Corveleyn <jc...@gmail.com>.
On Tue, Feb 5, 2013 at 5:08 PM, Stefan Sperling <st...@elego.de> wrote:
> On Tue, Feb 05, 2013 at 12:59:43PM +0100, Johan Corveleyn wrote:
>> Given what I said above, this can't be done in general (not if you
>> give two URLs or two wc paths, because then both synopsis 1 and 2 are
>> possible -- it would obviously break backwards compatibility if we
>> would now start erroring out on these, while these used to work fine
>> with automatically picking usage 1 before). Only if one of them is a
>> URL and the other a WC path, because we give an error message then
>> anyway.
>
> I think Alexey meant only the cases where we currently error out, i.e. where
> exactly two targets are given and one is a path and the other is a URL.
>
> I didn't realise this but there is indeed no ambiguity in allowing this
> syntax as a shorthand for a corresponding '--old X --new Y' diff invocation.
>
> Thanks Alexey, see http://svn.apache.org/r1442640
> Making these invocations produce a diff instead of an error makes things
> less surprising for users.
Hmm, okay, but I would have preferred some more discussion before you
implemented this. It's not clear to me that this decreases the
surprises. I can perfectly imagine just as many questions being asked
on users@ with the question: "Why does 'svn diff left.txt right.txt'
give me an empty diff, while 'svn diff ^/left.txt right.txt' does the
right thing?"
I think it would be better to consistently point users to the
--old/--new syntax, so as to educate users more (so I'm okay with the
change in the error message), not hide it even more from them.
Speaking of the error message:
On Tue, Feb 5, 2013 at 5:24 PM, Stefan Sperling <st...@elego.de> wrote:
> On Mon, Feb 04, 2013 at 04:39:32PM -0800, Alexey Neyman wrote:
>> What adds oil to the fire is that the error message, which claims that "Target
>> lists to diff may not contain both working copy paths and URL". If instead it
>> said that the supplied syntax is ambiguous and suggested to use --new/--old
>> for disambiguation, this would have saved a lot of frustration and would
>> eliminate the need of "svn compare" subcommand :)
>>
>> For example:
>> svn: Ambiguous target list (contains both work copy paths and URLs). Use
>> --old/--new syntax to disambiguate.
>
> Agreed. The error message wasn't very helpful.
> I've improved it in http://svn.apache.org/r1442645 based on your suggestion.
> Thanks!
How is this useful after r1442640? Seems to me we should either
improve the error message (which would be my preferred option) *or*
make 'svn diff' handle these automatically as a shorthand syntax
(hence no error message). But not both.
Or am I missing something?
--
Johan
Re: FreeBSD project and subversion.
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Feb 05, 2013 at 12:59:43PM +0100, Johan Corveleyn wrote:
> Given what I said above, this can't be done in general (not if you
> give two URLs or two wc paths, because then both synopsis 1 and 2 are
> possible -- it would obviously break backwards compatibility if we
> would now start erroring out on these, while these used to work fine
> with automatically picking usage 1 before). Only if one of them is a
> URL and the other a WC path, because we give an error message then
> anyway.
I think Alexey meant only the cases where we currently error out, i.e. where
exactly two targets are given and one is a path and the other is a URL.
I didn't realise this but there is indeed no ambiguity in allowing this
syntax as a shorthand for a corresponding '--old X --new Y' diff invocation.
Thanks Alexey, see http://svn.apache.org/r1442640
Making these invocations produce a diff instead of an error makes things
less surprising for users.
Re: FreeBSD project and subversion.
Posted by Johan Corveleyn <jc...@gmail.com>.
On Tue, Feb 5, 2013 at 1:39 AM, Alexey Neyman <st...@att.net> wrote:
> On Monday, February 04, 2013 10:17:29 PM Stefan Sperling wrote:
>
>> On Mon, Feb 04, 2013 at 11:54:21AM -0800, Alexey Neyman wrote:
>
>> > On Thursday, January 31, 2013 09:08:20 am Stefan Sperling wrote:
>
>> > > BTW, I went over one of FreeBSD's svn wiki pages a while back, and
>> > > added
>
>> > > answers to open questions on this page:
>
>> > > https://wiki.freebsd.org/SubversionMissing
>
>> >
>
>> > Speaking of which:
>
>> >
>
>> > Is there a reason why it is necessary to use "the svn diff --old
>
>> > ^/somebranch --new ." instead of the intuitive "svn ^/somebranch ."?
>
>>
>
>> Would "svn diff ^/somebranch ." be synopsis 1?
>
>>
>
>> svn diff -rN:M ^/somebranch@HEAD .@HEAD
>
>
>
> It cannot be "synopsis 1", as its description states that "TARGETs may be
> all working copy paths or all URLs". In this case, we have one of each.
> Moreover, synopsis 1 says that for URLs, -r or -c must be present - and my
> example has no -r/-c option.
Sorry, that's not true. From 'svn help diff' of 1.7:
diff (di): Display the differences between two revisions or paths.
usage: 1. diff [-c M | -r N[:M]] [TARGET[@REV]...]
2. diff [-r N[:M]] --old=OLD-TGT[@OLDREV] [--new=NEW-TGT[@NEWREV]] \
[PATH...]
3. diff OLD-URL[@OLDREV] NEW-URL[@NEWREV]
So for usage 1 the -c and -r are optional.
Also, the condition "may be all working copy paths or all URLs" is
pretty weak for deciding to use usage 1, because for usage 2
(--old/--new) you can just as well use two working copy paths or two
URLs. I think it would be even more confusing if 'svn diff A B' would
decide to use usage 1 if they are both URLs or both wc paths, and use
usage 2 if only one of them is a URL and the other a wc path (even
though usage 2 can just as well accomodate two URLs and two wc paths).
>
> So, the only possible use of this syntax would be synopsis 2:
>
>
>
>> svn diff --old ^/somebranch@HEAD --new .@HEAD
>
>> (i.e. show the difference between the two (path, revision) pairs
>
>> somebranch@HEAD and .@HEAD) ?
>
>>
>
>> > It has bugged me for a while until I discovered this --old/--new trick.
>
>>
>
>> I agree that --old --new syntax isn't intuitive at all.
>
>
>
> What adds oil to the fire is that the error message, which claims that
> "Target lists to diff may not contain both working copy paths and URL". If
> instead it said that the supplied syntax is ambiguous and suggested to use
> --new/--old for disambiguation, this would have saved a lot of frustration
> and would eliminate the need of "svn compare" subcommand :)
>
>
>
> For example:
>
> svn: Ambiguous target list (contains both work copy paths and URLs). Use
>
> --old/--new syntax to disambiguate.
>
Given what I said above, this can't be done in general (not if you
give two URLs or two wc paths, because then both synopsis 1 and 2 are
possible -- it would obviously break backwards compatibility if we
would now start erroring out on these, while these used to work fine
with automatically picking usage 1 before). Only if one of them is a
URL and the other a WC path, because we give an error message then
anyway.
--
Johan
Re: FreeBSD project and subversion.
Posted by Stefan Sperling <st...@elego.de>.
On Mon, Feb 04, 2013 at 04:39:32PM -0800, Alexey Neyman wrote:
> What adds oil to the fire is that the error message, which claims that "Target
> lists to diff may not contain both working copy paths and URL". If instead it
> said that the supplied syntax is ambiguous and suggested to use --new/--old
> for disambiguation, this would have saved a lot of frustration and would
> eliminate the need of "svn compare" subcommand :)
>
> For example:
> svn: Ambiguous target list (contains both work copy paths and URLs). Use
> --old/--new syntax to disambiguate.
Agreed. The error message wasn't very helpful.
I've improved it in http://svn.apache.org/r1442645 based on your suggestion.
Thanks!
Re: FreeBSD project and subversion.
Posted by Alexey Neyman <st...@att.net>.
On Monday, February 04, 2013 10:17:29 PM Stefan Sperling wrote:
> On Mon, Feb 04, 2013 at 11:54:21AM -0800, Alexey Neyman wrote:
> > On Thursday, January 31, 2013 09:08:20 am Stefan Sperling wrote:
> > > BTW, I went over one of FreeBSD's svn wiki pages a while back, and added
> > > answers to open questions on this page:
> > > https://wiki.freebsd.org/SubversionMissing
> >
> > Speaking of which:
> >
> > Is there a reason why it is necessary to use "the svn diff --old
> > ^/somebranch --new ." instead of the intuitive "svn ^/somebranch ."?
>
> Would "svn diff ^/somebranch ." be synopsis 1?
>
> svn diff -rN:M ^/somebranch@HEAD .@HEAD
It cannot be "synopsis 1", as its description states that "TARGETs may be all
working copy paths or all URLs". In this case, we have one of each. Moreover,
synopsis 1 says that for URLs, -r or -c must be present - and my example has
no -r/-c option.
So, the only possible use of this syntax would be synopsis 2:
> svn diff --old ^/somebranch@HEAD --new .@HEAD
> (i.e. show the difference between the two (path, revision) pairs
> somebranch@HEAD and .@HEAD) ?
>
> > It has bugged me for a while until I discovered this --old/--new trick.
>
> I agree that --old --new syntax isn't intuitive at all.
What adds oil to the fire is that the error message, which claims that "Target
lists to diff may not contain both working copy paths and URL". If instead it
said that the supplied syntax is ambiguous and suggested to use --new/--old
for disambiguation, this would have saved a lot of frustration and would
eliminate the need of "svn compare" subcommand :)
For example:
svn: Ambiguous target list (contains both work copy paths and URLs). Use
--old/--new syntax to disambiguate.
Regards,
Alexey.
Re: FreeBSD project and subversion.
Posted by Stefan Sperling <st...@elego.de>.
On Mon, Feb 04, 2013 at 11:54:21AM -0800, Alexey Neyman wrote:
> On Thursday, January 31, 2013 09:08:20 am Stefan Sperling wrote:
> > BTW, I went over one of FreeBSD's svn wiki pages a while back, and added
> > answers to open questions on this page:
> > https://wiki.freebsd.org/SubversionMissing
>
> Speaking of which:
>
> Is there a reason why it is necessary to use "the svn diff --old ^/somebranch
> --new ." instead of the intuitive "svn ^/somebranch ."?
Would "svn diff ^/somebranch ." be synopsis 1?
svn diff -rN:M ^/somebranch@HEAD .@HEAD
(i.e. svn diff -rN:M ^/somebranch@HEAD; svn diff -rN:M .@HEAD;
show the differences between rN to rM for ^/somebranch@HEAD and then show
the differences between rN and rM for .@HEAD -- where N defaults to
BASE if the target is a working copy path, and M defaults to HEAD)
Note that you can also specify 3 or more targets:
svn diff -rN:M ^/somebranch@HEAD .@HEAD ^/someotherbranch@42 ...
Or would it be synopsis 2:
svn diff --old ^/somebranch@HEAD --new .@HEAD
(i.e. show the difference between the two (path, revision) pairs
somebranch@HEAD and .@HEAD) ?
> It has bugged me for a while until I discovered this --old/--new trick.
I agree that --old --new syntax isn't intuitive at all.
But I believe it exists to resolve the above ambiguity.
The only remedy that comes to my mind is to introduce a new subcommand:
svn compare TARGET1 TARGET2
which would equal
svn diff --old TARGET1 TARGET2
I'm not sure if the community would like to see yet another new
subcommand though :)
Re: FreeBSD project and subversion.
Posted by Alexey Neyman <st...@att.net>.
On Thursday, January 31, 2013 09:08:20 am Stefan Sperling wrote:
> BTW, I went over one of FreeBSD's svn wiki pages a while back, and added
> answers to open questions on this page:
> https://wiki.freebsd.org/SubversionMissing
Speaking of which:
Is there a reason why it is necessary to use "the svn diff --old ^/somebranch
--new ." instead of the intuitive "svn ^/somebranch ."?
It has bugged me for a while until I discovered this --old/--new trick.
Regards,
Alexey.
Re: PATCH: keyword expansion 1.8
Posted by Alfred Perlstein <br...@mu.org>.
On 4/10/13 11:05 AM, Stefan Sperling wrote:
> On Tue, Feb 12, 2013 at 08:42:29PM +0100, Stefan Sperling wrote:
>> Thanks! Are you willing to spend time on providing an updated patch?
> I've taken the time to wrap this up myself today, see:
> http://svn.apache.org/r1466570
> http://svn.apache.org/r1466598
>
> So hopefully keyword expansion for svn.freebsd.org will work fine
> with a stock Subversion 1.8 client.
>
> It seems to work fine in my testing. I'd be very happy about test
> reports from the FreeBSD side with an svn client compiled from
> Subversion's trunk as of r1466598.
>
> Thanks for contributing this patch!
>
Thank you, this is very exciting.
-Alfred
Re: PATCH: keyword expansion 1.8
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Feb 12, 2013 at 08:42:29PM +0100, Stefan Sperling wrote:
> Thanks! Are you willing to spend time on providing an updated patch?
I've taken the time to wrap this up myself today, see:
http://svn.apache.org/r1466570
http://svn.apache.org/r1466598
So hopefully keyword expansion for svn.freebsd.org will work fine
with a stock Subversion 1.8 client.
It seems to work fine in my testing. I'd be very happy about test
reports from the FreeBSD side with an svn client compiled from
Subversion's trunk as of r1466598.
Thanks for contributing this patch!
Re: PATCH: keyword expansion 1.8
Posted by Stefan Sperling <st...@elego.de>.
On Sat, Feb 09, 2013 at 09:21:56PM +0100, Stefan Sperling wrote:
> I'll try to review this during the coming week. Thanks!
Hi again,
Please see below for my review.
> > Index: subversion/include/private/svn_subst_private.h
> > ===================================================================
> > --- subversion/include/private/svn_subst_private.h (revision 0)
> > +++ subversion/include/private/svn_subst_private.h (working copy)
> > @@ -0,0 +1,68 @@
> > +/**
> > + * @copyright
> > + * ====================================================================
> > + * Licensed to the Apache Software Foundation (ASF) under one
> > + * or more contributor license agreements. See the NOTICE file
> > + * distributed with this work for additional information
> > + * regarding copyright ownership. The ASF licenses this file
> > + * to you under the Apache License, Version 2.0 (the
> > + * "License"); you may not use this file except in compliance
> > + * with the License. You may obtain a copy of the License at
> > + *
> > + * http://www.apache.org/licenses/LICENSE-2.0
> > + *
> > + * Unless required by applicable law or agreed to in writing,
> > + * software distributed under the License is distributed on an
> > + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
> > + * KIND, either express or implied. See the License for the
> > + * specific language governing permissions and limitations
> > + * under the License.
> > + * ====================================================================
> > + * @endcopyright
> > + *
> > + * @file svn_subst_private.h
> > + * @brief Non-public subst utility functions.
> > + */
> > +
> > +
> > +#ifndef SVN_SUBST_PRIVATE_H
> > +#define SVN_SUBST_PRIVATE_H
> > +
> > +#include "svn_subst.h" /* for svn_boolean_t, svn_error_t */
> > +
> > +#ifdef __cplusplus
> > +extern "C" {
> > +#endif /* __cplusplus */
> > +
> > +/**
> > + * Set @a *kw to a new keywords hash filled with the appropriate contents
> > + * given a @a keywords_string (the contents of the svn:keywords
> > + * property for the file in question), the revision @a rev, the @a url,
> > + * the url of the root of the @a repos, the @a date the file was committed
> > + * on, and the @a author of the last commit. Any of these can be @c NULL
> > + * to indicate that the information is not present, or @c 0 for @a date.
> > + *
> > + * Hash keys are of type <tt>const char *</tt>.
> > + * Hash values are of type <tt>svn_string_t *</tt>.
> > + *
> > + * All memory is allocated out of @a pool.
> > + *
> > + * @since New in 1.6
> > + */
> > +svn_error_t *
> > +svn_subst_build_keywords3(apr_hash_t **kw,
> > + const char *keywords_string,
> > + const char *rev,
> > + const char *url,
> > + const char *repos,
> > + apr_time_t date,
> > + const char *author,
> > + apr_pool_t *pool);
> > +
Pleaes don't add a new header for this function.
It should be declared in svn_subst.h as it it a public API.
It should replace svn_subst_build_keywords2(), so the
svn_subst_build_keywords2() function should be marked deprecated.
The above docstring seems to be based on an outdated version of the
svn_subst_build_keywords2() docstring.
Here's what I'd suggest using instead (as a diff against svn_subst.h):
[[[
Index: subversion/include/svn_subst.h
===================================================================
--- subversion/include/svn_subst.h (revision 1445280)
+++ subversion/include/svn_subst.h (working copy)
@@ -127,13 +127,13 @@ typedef struct svn_subst_keywords_t
* Set @a *kw to a new keywords hash filled with the appropriate contents
* given a @a keywords_string (the contents of the svn:keywords
* property for the file in question), the revision @a rev, the @a url,
- * the @a date the file was committed on, and the @a author of the last
- * commit.
+ * the @a date the file was committed on, the @a author of the last
+ * commit, and the URL of the repository root @a repos_root_url.
*
- * Any of the inputs @a rev, @a url, @a date and @a author can be @c NULL,
- * or @c 0 for @a date, to indicate that the information is not present.
- * Each piece of information that is not present expands to the empty
- * string wherever it appears in an expanded keyword value. (This can
+ * Any of the inputs @a rev, @a url, @a date, @a author, and @a repos_root_url
+ * can be @c NULL, or @c 0 for @a date, to indicate that the information is
+ * not present. Each piece of information that is not present expands to the
+ * empty string wherever it appears in an expanded keyword value. (This can
* result in multiple adjacent spaces in the expansion of a multi-valued
* keyword such as "Id".)
*
@@ -142,8 +142,25 @@ typedef struct svn_subst_keywords_t
*
* All memory is allocated out of @a pool.
*
+ * @since New in 1.8.
+ */
+svn_error_t *
+svn_subst_build_keywords3(apr_hash_t **kw,
+ const char *keywords_string,
+ const char *rev,
+ const char *url,
+ const char *repos_root_url,
+ apr_time_t date,
+ const char *author,
+ apr_pool_t *pool);
+
+/** Similar to svn_subst_build_keywords3() except that it does not accept
+ * the @a repos_root_url parameter and hence supports less substitutions.
+ *
* @since New in 1.3.
+ * @deprecated Provided for backward compatibility with the 1.7 API.
*/
+SVN_DEPRECATED
svn_error_t *
svn_subst_build_keywords2(apr_hash_t **kw,
const char *keywords_string,
]]]
> > +
> > +
> > +#ifdef __cplusplus
> > +}
> > +#endif /* __cplusplus */
> > +
> > +#endif /* SVN_SUBST_PRIVATE_H */
> > Index: subversion/libsvn_client/cat.c
> > ===================================================================
> > --- subversion/libsvn_client/cat.c (revision 1441445)
> > +++ subversion/libsvn_client/cat.c (working copy)
> > @@ -40,6 +40,7 @@
> >
> > #include "svn_private_config.h"
> > #include "private/svn_wc_private.h"
> > +#include "private/svn_subst_private.h"
> >
> >
> > /*** Code. ***/
> > @@ -124,12 +125,15 @@
> > const char *author;
> > const char *url;
> > apr_time_t tm;
> > + const char *repos;
Bad variable name. It isn't clear what this is referring to.
It is a repository URL or a path in the repository?
I suppose either repos_relpath or repos_root_url would be suitable
here. Based on what svn_subst_build_keywords3() expects I suppose
repos_root_url is intended here?
> >
> > SVN_ERR(svn_wc__node_get_changed_info(&changed_rev, &tm, &author, wc_ctx,
> > local_abspath, scratch_pool,
> > scratch_pool));
> > SVN_ERR(svn_wc__node_get_url(&url, wc_ctx, local_abspath, scratch_pool,
> > scratch_pool));
> > + SVN_ERR(svn_wc__node_get_repos_info(&repos, NULL, wc_ctx, local_abspath,
This function takes additional arguments on trunk, so the patch
doesn't compile as it is.
> > + scratch_pool, scratch_pool));
> >
> > if (local_mod)
> > {
> > @@ -152,8 +156,8 @@
> > rev_str = apr_psprintf(scratch_pool, "%ld", changed_rev);
> > }
> >
> > - SVN_ERR(svn_subst_build_keywords2(&kw, keywords->data, rev_str, url, tm,
> > - author, scratch_pool));
> > + SVN_ERR(svn_subst_build_keywords3(&kw, keywords->data, rev_str, url,
> > + repos, tm, author, scratch_pool));
> > }
> >
> > /* Wrap the output stream if translation is needed. */
> > @@ -181,6 +185,7 @@
> > svn_string_t *eol_style;
> > svn_string_t *keywords;
> > apr_hash_t *props;
> > + const char *repos_root_url;
Much better name! :)
> > svn_stream_t *output = out;
> > svn_error_t *err;
> >
> > @@ -225,6 +230,9 @@
> > peg_revision,
> > revision, ctx, pool));
> >
> > + /* Find the repos root URL */
> > + SVN_ERR(svn_ra_get_repos_root2(ra_session, &repos_root_url, pool));
> > +
> > /* Grab some properties we need to know in order to figure out if anything
> > special needs to be done with this file. */
> > err = svn_ra_get_file(ra_session, "", loc->rev, NULL, NULL, &props, pool);
> > @@ -275,10 +283,11 @@
> > if (cmt_date)
> > SVN_ERR(svn_time_from_cstring(&when, cmt_date->data, pool));
> >
> > - SVN_ERR(svn_subst_build_keywords2
> > + SVN_ERR(svn_subst_build_keywords3
> > (&kw, keywords->data,
> > cmt_rev->data,
> > loc->url,
> > + repos_root_url,
Please don't use tabs for indentation, only spaces.
The coding conventions have been in place for a decade and we
try to stick to them for consistency, not necessarily beauty (I don't
really like the indentation style personally but I still stick to it).
> > when,
> > cmt_author ? cmt_author->data : NULL,
> > pool));
> > Index: subversion/libsvn_client/commit.c
> > ===================================================================
> > --- subversion/libsvn_client/commit.c (revision 1441445)
> > +++ subversion/libsvn_client/commit.c (working copy)
> > @@ -44,6 +44,7 @@
> > #include "client.h"
> > #include "private/svn_wc_private.h"
> > #include "private/svn_ra_private.h"
> > +#include "private/svn_subst_private.h"
> >
> > #include "svn_private_config.h"
> >
Why is this file changed? The only change seems to be an additional
include. And there's no need to include this header in the first place if
you declare svn_subst_build_keywords3() in svn_subst.h.
> > Index: subversion/libsvn_client/export.c
> > ===================================================================
> > --- subversion/libsvn_client/export.c (revision 1441445)
> > +++ subversion/libsvn_client/export.c (working copy)
> > @@ -45,6 +45,7 @@
> > #include "private/svn_subr_private.h"
> > #include "private/svn_delta_private.h"
> > #include "private/svn_wc_private.h"
> > +#include "private/svn_subst_private.h"
> >
> > #ifndef ENABLE_EV2_IMPL
> > #define ENABLE_EV2_IMPL 0
> > @@ -377,6 +378,7 @@
> > {
> > svn_revnum_t changed_rev = status->changed_rev;
> > const char *suffix;
> > + const char *repos;
Again, repos_relpath or repos_root_url please.
> > const char *url = svn_path_url_add_component2(status->repos_root_url,
> > status->repos_relpath,
> > scratch_pool);
> > @@ -395,10 +397,13 @@
> > suffix = "";
> > }
> >
> > - SVN_ERR(svn_subst_build_keywords2
> > + SVN_ERR(svn_wc__node_get_repos_info(&repos, NULL, wc_ctx,
> > + eib->origin_abspath /* XXX: ?? from_abspath */,
Seems whoever wrote the above comment wasn't sure what they were doing.
This should be checked.
> > + scratch_pool, scratch_pool));
> > + SVN_ERR(svn_subst_build_keywords3
> > (&kw, keywords->data,
> > apr_psprintf(scratch_pool, "%ld%s", changed_rev, suffix),
> > - url, tm, author, scratch_pool));
> > + url, repos, tm, author, scratch_pool));
> > }
> >
> > /* For atomicity, we translate to a tmp file and then rename the tmp file
> > @@ -544,6 +549,7 @@
> > /* Any keyword vals to be substituted */
> > const char *revision;
> > const char *url;
> > + const char *repos;
Again, bad variable name.
> > const char *author;
> > apr_time_t date;
> >
> > @@ -665,6 +671,7 @@
> > fb->edit_baton = eb;
> > fb->path = full_path;
> > fb->url = full_url;
> > + fb->repos = eb->root_url;
Same.
> > fb->pool = pool;
> >
> > *baton = fb;
> > @@ -830,8 +837,8 @@
> > }
> >
> > if (fb->keywords_val)
> > - SVN_ERR(svn_subst_build_keywords2(&final_kw, fb->keywords_val->data,
> > - fb->revision, fb->url, fb->date,
> > + SVN_ERR(svn_subst_build_keywords3(&final_kw, fb->keywords_val->data,
> > + fb->revision, fb->url, fb->repos, fb->date,
> > fb->author, pool));
> >
> > SVN_ERR(svn_subst_copy_and_translate4(fb->tmppath, fb->path,
> > Index: subversion/libsvn_client/import.c
> > ===================================================================
> > --- subversion/libsvn_client/import.c (revision 1441445)
> > +++ subversion/libsvn_client/import.c (working copy)
> > @@ -49,6 +49,7 @@
> > #include "private/svn_subr_private.h"
> > #include "private/svn_ra_private.h"
> > #include "private/svn_magic.h"
> > +#include "private/svn_subst_private.h"
> >
> > #include "svn_private_config.h"
> >
> > @@ -133,9 +134,9 @@
> > }
> >
> > if (keywords_val)
> > - SVN_ERR(svn_subst_build_keywords2(&keywords, keywords_val->data,
> > + SVN_ERR(svn_subst_build_keywords3(&keywords, keywords_val->data,
> > APR_STRINGIFY(SVN_INVALID_REVNUM),
> > - "", 0, "", pool));
> > + "", "", 0, "", pool));
> > else
> > keywords = NULL;
> >
> > Index: subversion/libsvn_subr/subst.c
> > ===================================================================
> > --- subversion/libsvn_subr/subst.c (revision 1441445)
> > +++ subversion/libsvn_subr/subst.c (working copy)
> > @@ -49,6 +49,7 @@
> > #include "svn_private_config.h"
> >
> > #include "private/svn_string_private.h"
> > +#include "private/svn_subst_private.h"
> >
> > /**
> > * The textual elements of a detranslated special file. One of these
> > @@ -135,8 +136,11 @@
> > * %b basename of the URL of this file
> > * %d short format of date of this revision
> > * %D long format of date of this revision
> > + * %P path relative to root of repos
> > * %r number of this revision
> > + * %R root url of repository
> > * %u URL of this file
> > + * %_ a space
> > * %% a literal %
Are supported expansions documented anywhere else than this code comment?
Seems strange to document them here.
Perhaps the only other place is in the svnbook, in which case we don't
need any more documentation in this patch. However, if there's a public
API docstring somewhere that documents them, we should update it, too.
I don't know if such a docstring exists, though. If it doesn't, we should
consider moving this coment or parts of it into svn_subst.h. But that can
be done in a separate patch as it's a separate documentation problem.
> > *
> > * All memory is allocated out of @a pool.
> > @@ -145,12 +149,14 @@
> > keyword_printf(const char *fmt,
> > const char *rev,
> > const char *url,
> > + const char *repos,
Bad name again.
> > apr_time_t date,
> > const char *author,
> > apr_pool_t *pool)
> > {
> > svn_stringbuf_t *value = svn_stringbuf_ncreate("", 0, pool);
> > const char *cur;
> > + const char *relative;
Same. repos_relpath please.
> > size_t n;
> >
> > for (;;)
> > @@ -203,6 +209,23 @@
> > svn_stringbuf_appendcstr(value,
> > svn_time_to_human_cstring(date, pool));
> > break;
> > + case 'P': /* relative path of this file */
> > + relative = url;
> > + if (relative && repos)
> > + {
> > + int len = strlen(repos);
> > +
> > + if (strncmp(repos, relative, len) == 0
> > + && relative[len] == '/')
> > + relative += len + 1;
There's tabs all over the place in this section.
I'm not sure I like the manual URL munging here. Consider checking
svn_dirent_uri.h for a suitable helper functions to split the in-repos
path portion off the URL (e.g. svn_uri_skip_ancestor() might work).
> > + }
> > + if (relative)
> > + svn_stringbuf_appendcstr(value, relative);
> > + break;
> > + case 'R': /* root of repos */
> > + if (repos)
> > + svn_stringbuf_appendcstr(value, repos);
> > + break;
> > case 'r': /* number of this revision */
> > if (rev)
> > svn_stringbuf_appendcstr(value, rev);
> > @@ -211,6 +234,9 @@
> > if (url)
> > svn_stringbuf_appendcstr(value, url);
> > break;
> > + case '_': /* '%_' => a space */
> > + svn_stringbuf_appendbytes(value, " ", 1);
Consider using svn_stringbuf_appendbyte(), which is new in trunk.
> > + break;
> > case '%': /* '%%' => a literal % */
> > svn_stringbuf_appendbyte(value, *cur);
> > break;
> > @@ -246,8 +272,8 @@
> > apr_hash_t *kwhash;
> > const svn_string_t *val;
> >
> > - SVN_ERR(svn_subst_build_keywords2(&kwhash, keywords_val, rev,
> > - url, date, author, pool));
> > + SVN_ERR(svn_subst_build_keywords3(&kwhash, keywords_val, rev,
> > + url, "", date, author, pool));
> >
> > /* The behaviour of pre-1.3 svn_subst_build_keywords, which we are
> > * replicating here, is to write to a slot in the svn_subst_keywords_t
> > @@ -286,6 +312,21 @@
> > const char *author,
> > apr_pool_t *pool)
> > {
> > + SVN_ERR(svn_subst_build_keywords3(kw, keywords_val, rev,
> > + url, "", date, author, pool));
> > + return SVN_NO_ERROR;
> > +}
> > +
> > +svn_error_t *
> > +svn_subst_build_keywords3(apr_hash_t **kw,
> > + const char *keywords_val,
> > + const char *rev,
> > + const char *url,
> > + const char *repos,
> > + apr_time_t date,
> > + const char *author,
> > + apr_pool_t *pool)
> > +{
> > apr_array_header_t *keyword_tokens;
> > int i;
> > *kw = apr_hash_make(pool);
> > @@ -296,14 +337,32 @@
> > for (i = 0; i < keyword_tokens->nelts; ++i)
> > {
> > const char *keyword = APR_ARRAY_IDX(keyword_tokens, i, const char *);
> > + apr_array_header_t *keyword_tokens2;
> >
> > + keyword_tokens2 = svn_cstring_split(keyword, "=", TRUE /* chop */, pool);
> > + if (keyword_tokens2->nelts == 2)
> > + {
> > + svn_string_t *custom_val;
> > + const char *custom_expand;
> > +
> > + keyword = APR_ARRAY_IDX(keyword_tokens2, 0, const char*);
> > + custom_expand = APR_ARRAY_IDX(keyword_tokens2, 1, const char*);
> > + if (! strcmp(custom_expand, "%H"))
> > + custom_expand = "%P %r %d %a";
> > + else if (! strcmp(custom_expand, "%I"))
> > + custom_expand = "%b %r %d %a";
Tabs above.
Are the new %H and %I expansions documented anywhere?
What is the reason to add them? Did they exist in CVS?
The rest looks good (apart from further use of the variable name 'repos').
Thanks! Are you willing to spend time on providing an updated patch?
Please also include a log message as described here:
http://subversion.apache.org/docs/community-guide/general.html#patches
Would you also be willing to tweak the regression tests in
subversion/test/cmdline/trans_tests.py to account for the
new keyword substitutions?
> > + custom_val = keyword_printf(custom_expand, rev, url, repos, date, author, pool);
> > + apr_hash_set(*kw, keyword, APR_HASH_KEY_STRING, custom_val);
> > + return SVN_NO_ERROR;
> > + }
> > +
> > if ((! strcmp(keyword, SVN_KEYWORD_REVISION_LONG))
> > || (! strcmp(keyword, SVN_KEYWORD_REVISION_MEDIUM))
> > || (! svn_cstring_casecmp(keyword, SVN_KEYWORD_REVISION_SHORT)))
> > {
> > svn_string_t *revision_val;
> >
> > - revision_val = keyword_printf("%r", rev, url, date, author, pool);
> > + revision_val = keyword_printf("%r", rev, url, repos, date, author, pool);
> > apr_hash_set(*kw, SVN_KEYWORD_REVISION_LONG,
> > APR_HASH_KEY_STRING, revision_val);
> > apr_hash_set(*kw, SVN_KEYWORD_REVISION_MEDIUM,
> > @@ -316,7 +375,7 @@
> > {
> > svn_string_t *date_val;
> >
> > - date_val = keyword_printf("%D", rev, url, date, author, pool);
> > + date_val = keyword_printf("%D", rev, url, repos, date, author, pool);
> > apr_hash_set(*kw, SVN_KEYWORD_DATE_LONG,
> > APR_HASH_KEY_STRING, date_val);
> > apr_hash_set(*kw, SVN_KEYWORD_DATE_SHORT,
> > @@ -327,7 +386,7 @@
> > {
> > svn_string_t *author_val;
> >
> > - author_val = keyword_printf("%a", rev, url, date, author, pool);
> > + author_val = keyword_printf("%a", rev, url, repos, date, author, pool);
> > apr_hash_set(*kw, SVN_KEYWORD_AUTHOR_LONG,
> > APR_HASH_KEY_STRING, author_val);
> > apr_hash_set(*kw, SVN_KEYWORD_AUTHOR_SHORT,
> > @@ -338,7 +397,7 @@
> > {
> > svn_string_t *url_val;
> >
> > - url_val = keyword_printf("%u", rev, url, date, author, pool);
> > + url_val = keyword_printf("%u", rev, url, repos, date, author, pool);
> > apr_hash_set(*kw, SVN_KEYWORD_URL_LONG,
> > APR_HASH_KEY_STRING, url_val);
> > apr_hash_set(*kw, SVN_KEYWORD_URL_SHORT,
> > @@ -348,7 +407,7 @@
> > {
> > svn_string_t *id_val;
> >
> > - id_val = keyword_printf("%b %r %d %a", rev, url, date, author,
> > + id_val = keyword_printf("%b %r %d %a", rev, url, repos, date, author,
> > pool);
> > apr_hash_set(*kw, SVN_KEYWORD_ID,
> > APR_HASH_KEY_STRING, id_val);
> > @@ -357,7 +416,7 @@
> > {
> > svn_string_t *header_val;
> >
> > - header_val = keyword_printf("%u %r %d %a", rev, url, date, author,
> > + header_val = keyword_printf("%u %r %d %a", rev, url, repos, date, author,
> > pool);
> > apr_hash_set(*kw, SVN_KEYWORD_HEADER,
> > APR_HASH_KEY_STRING, header_val);
> > Index: subversion/libsvn_wc/translate.c
> > ===================================================================
> > --- subversion/libsvn_wc/translate.c (revision 1441445)
> > +++ subversion/libsvn_wc/translate.c (working copy)
> > @@ -46,6 +46,7 @@
> >
> > #include "svn_private_config.h"
> > #include "private/svn_wc_private.h"
> > +#include "private/svn_subst_private.h"
> >
> >
> >
> > @@ -313,10 +314,10 @@
> > apr_time_t changed_date;
> > const char *changed_author;
> > const char *url;
> > + const char *repos_root_url;
> >
> > if (! for_normalization)
> > {
> > - const char *repos_root_url;
> > const char *repos_relpath;
> >
> > SVN_ERR(svn_wc__db_read_info(NULL, NULL, NULL, &repos_relpath,
> > @@ -341,13 +342,23 @@
> > changed_rev = SVN_INVALID_REVNUM;
> > changed_date = 0;
> > changed_author = "";
> > +
> > + SVN_ERR(svn_wc__db_read_info(NULL, NULL, NULL, NULL,
> > + &repos_root_url, NULL, NULL,
> > + NULL, NULL, NULL,
> > + NULL, NULL, NULL, NULL, NULL, NULL,
> > + NULL, NULL, NULL, NULL, NULL, NULL, NULL,
> > + NULL, NULL, NULL, NULL,
> > + db, local_abspath,
> > + scratch_pool, scratch_pool));
> > }
> >
> > - SVN_ERR(svn_subst_build_keywords2(keywords,
> > + SVN_ERR(svn_subst_build_keywords3(keywords,
> > keyword_list,
> > apr_psprintf(scratch_pool, "%ld",
> > changed_rev),
> > url,
> > + repos_root_url,
> > changed_date,
> > changed_author,
> > result_pool));
Re: PATCH: keyword expansion 1.8
Posted by Daniel Shahaf <da...@elego.de>.
Stefan Sperling wrote on Mon, Feb 11, 2013 at 12:36:29 +0100:
> On Mon, Feb 11, 2013 at 09:38:05AM +0000, Philip Martin wrote:
> > Stefan Sperling <st...@elego.de> writes:
> > >> +svn_error_t *
> > >> +svn_subst_build_keywords3(apr_hash_t **kw,
> > >> + const char *keywords_string,
> > >> + const char *rev,
> > >> + const char *url,
> > >> + const char *repos,
> > >> + apr_time_t date,
> > >> + const char *author,
> > >> + apr_pool_t *pool);
> > >> +
> >
> > Our private API uses a double underscore svn_subst__build_keywords3
> > however this is replacing a public API svn_subst_build_keywords2
> > declared in include/svn_subst.h.
> >
> > Is the intention to deprecate the entire svn_subst_build_keywords public
> > API? Or is svn_subst_build_keywords3 intended to be the new public API?
>
> I believe I can explain this, given the history of this patch.
>
> This was a patch to the FreeBSD port, and I suppose they made the new
> APIs they added in that patch private because it wasn't part of the
> official public API. So, yes, this API should be public now that the
> intention is to add it to the upstream code base.
Yes, that's correct. They used to declare the API in
subversion/include/, but the declaration has been moved to
subversion/include/private/ since it wasn't part of our (upstream's)
public API:
http://www.freebsd.org/cgi/query-pr.cgi?pr=165311
Re: PATCH: keyword expansion 1.8
Posted by Stefan Sperling <st...@elego.de>.
On Mon, Feb 11, 2013 at 09:38:05AM +0000, Philip Martin wrote:
> Stefan Sperling <st...@elego.de> writes:
>
> >> Index: subversion/include/private/svn_subst_private.h
> >> ===================================================================
> >> --- subversion/include/private/svn_subst_private.h (revision 0)
> >> +++ subversion/include/private/svn_subst_private.h (working copy)
> >> @@ -0,0 +1,68 @@
>
> >> + * @file svn_subst_private.h
> >> + * @brief Non-public subst utility functions.
> >> + */
> >> +
> >> +
> >> +#ifndef SVN_SUBST_PRIVATE_H
> >> +#define SVN_SUBST_PRIVATE_H
> >> +
> >> +#include "svn_subst.h" /* for svn_boolean_t, svn_error_t */
> >> +
> >> +#ifdef __cplusplus
> >> +extern "C" {
> >> +#endif /* __cplusplus */
> >> +
> >> +/**
> >> + * Set @a *kw to a new keywords hash filled with the appropriate contents
> >> + * given a @a keywords_string (the contents of the svn:keywords
> >> + * property for the file in question), the revision @a rev, the @a url,
> >> + * the url of the root of the @a repos, the @a date the file was committed
> >> + * on, and the @a author of the last commit. Any of these can be @c NULL
> >> + * to indicate that the information is not present, or @c 0 for @a date.
> >> + *
> >> + * Hash keys are of type <tt>const char *</tt>.
> >> + * Hash values are of type <tt>svn_string_t *</tt>.
> >> + *
> >> + * All memory is allocated out of @a pool.
> >> + *
> >> + * @since New in 1.6
>
> 1.6 is wrong.
Indeed. It should say "1.8" instead.
> >> + */
> >> +svn_error_t *
> >> +svn_subst_build_keywords3(apr_hash_t **kw,
> >> + const char *keywords_string,
> >> + const char *rev,
> >> + const char *url,
> >> + const char *repos,
> >> + apr_time_t date,
> >> + const char *author,
> >> + apr_pool_t *pool);
> >> +
>
> Our private API uses a double underscore svn_subst__build_keywords3
> however this is replacing a public API svn_subst_build_keywords2
> declared in include/svn_subst.h.
>
> Is the intention to deprecate the entire svn_subst_build_keywords public
> API? Or is svn_subst_build_keywords3 intended to be the new public API?
I believe I can explain this, given the history of this patch.
This was a patch to the FreeBSD port, and I suppose they made the new
APIs they added in that patch private because it wasn't part of the
official public API. So, yes, this API should be public now that the
intention is to add it to the upstream code base.
I don't think Alfred knows about the subtleties of our API versioning
guidelines yet. Nor should he have to -- after all, that's why we're
reviewing the patch and are pointing this out.
Re: PATCH: keyword expansion 1.8
Posted by Philip Martin <ph...@wandisco.com>.
Stefan Sperling <st...@elego.de> writes:
>> Index: subversion/include/private/svn_subst_private.h
>> ===================================================================
>> --- subversion/include/private/svn_subst_private.h (revision 0)
>> +++ subversion/include/private/svn_subst_private.h (working copy)
>> @@ -0,0 +1,68 @@
>> + * @file svn_subst_private.h
>> + * @brief Non-public subst utility functions.
>> + */
>> +
>> +
>> +#ifndef SVN_SUBST_PRIVATE_H
>> +#define SVN_SUBST_PRIVATE_H
>> +
>> +#include "svn_subst.h" /* for svn_boolean_t, svn_error_t */
>> +
>> +#ifdef __cplusplus
>> +extern "C" {
>> +#endif /* __cplusplus */
>> +
>> +/**
>> + * Set @a *kw to a new keywords hash filled with the appropriate contents
>> + * given a @a keywords_string (the contents of the svn:keywords
>> + * property for the file in question), the revision @a rev, the @a url,
>> + * the url of the root of the @a repos, the @a date the file was committed
>> + * on, and the @a author of the last commit. Any of these can be @c NULL
>> + * to indicate that the information is not present, or @c 0 for @a date.
>> + *
>> + * Hash keys are of type <tt>const char *</tt>.
>> + * Hash values are of type <tt>svn_string_t *</tt>.
>> + *
>> + * All memory is allocated out of @a pool.
>> + *
>> + * @since New in 1.6
1.6 is wrong.
>> + */
>> +svn_error_t *
>> +svn_subst_build_keywords3(apr_hash_t **kw,
>> + const char *keywords_string,
>> + const char *rev,
>> + const char *url,
>> + const char *repos,
>> + apr_time_t date,
>> + const char *author,
>> + apr_pool_t *pool);
>> +
Our private API uses a double underscore svn_subst__build_keywords3
however this is replacing a public API svn_subst_build_keywords2
declared in include/svn_subst.h.
Is the intention to deprecate the entire svn_subst_build_keywords public
API? Or is svn_subst_build_keywords3 intended to be the new public API?
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
Re: PATCH: keyword expansion 1.8
Posted by Stefan Sperling <st...@elego.de>.
On Sat, Feb 09, 2013 at 07:12:38AM -0800, Alfred Perlstein wrote:
> [[ Adding users@subversion.apache.org ]]
>
> He folks. I am in no way a Subversion expert, however per request
> I've taken the FreeBSD patch for adding custom keyword expansion and
> forward ported it to what is soon to be Subversion 1.8.
>
> I am wondering if anyone can help me test this or give me some
> things to try to ensure that it works properly.
>
> -Alfred
Hi Alfred,
Patches should go to dev@, not users@.
I've changed the Cc to the right list in this reply.
I'll try to review this during the coming week. Thanks!
> On 2/1/13 7:40 AM, Alfred Perlstein wrote:
> >[[ bringing in Lev Serebryakov, FreeBSD port maintainer for Subversion ]]
> >
> >Hello Lev,
> >
> >Stefan of the Subversion project asked me about bringing in our
> >keywords patch for subversion 1.8.
> >
> >I've taken a bit of time and gotten the patch to apply to
> >subversion trunk (untested).
> >
> >Can you have a look and let us know what we need to do with
> >regards to testing or anything else?
> >
> >
> >Stefan, any suggestions on going forward?
> >
> >-Alfred
> >
> >On 2/1/13 8:34 AM, Stefan Sperling wrote:
> >>On Fri, Feb 01, 2013 at 08:01:02AM -0500, Alfred Perlstein wrote:
> >>>[[ reply private ]]
> >>>
> >>>First of all, thank you very much for taking the time to explain this.
> >>>
> >>>I've forwarded your response to a few people who are more
> >>>knowledgeable about Subversion in our community (John Baldwin /
> >>>Peter Wemm) to hopefully get back us on the issue of mergeinfo and
> >>>reintegrate.
> >>Thanks!
> >>
> >>>Regarding the patch:
> >>>http://subversion.tigris.org/issues/show_bug.cgi?id=890
> >>>I just read this. What can I do to help on this issue?
> >>See the mailing list thread linked from
> >>http://subversion.tigris.org/issues/show_bug.cgi?id=890#desc40
> >>That's the last time the patch got reviewed. It needs to be updated
> >>to Subversion's trunk (to-be-1.8) before it can be committed.
> >>
> >
>
> Index: subversion/include/private/svn_subst_private.h
> ===================================================================
> --- subversion/include/private/svn_subst_private.h (revision 0)
> +++ subversion/include/private/svn_subst_private.h (working copy)
> @@ -0,0 +1,68 @@
> +/**
> + * @copyright
> + * ====================================================================
> + * Licensed to the Apache Software Foundation (ASF) under one
> + * or more contributor license agreements. See the NOTICE file
> + * distributed with this work for additional information
> + * regarding copyright ownership. The ASF licenses this file
> + * to you under the Apache License, Version 2.0 (the
> + * "License"); you may not use this file except in compliance
> + * with the License. You may obtain a copy of the License at
> + *
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing,
> + * software distributed under the License is distributed on an
> + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
> + * KIND, either express or implied. See the License for the
> + * specific language governing permissions and limitations
> + * under the License.
> + * ====================================================================
> + * @endcopyright
> + *
> + * @file svn_subst_private.h
> + * @brief Non-public subst utility functions.
> + */
> +
> +
> +#ifndef SVN_SUBST_PRIVATE_H
> +#define SVN_SUBST_PRIVATE_H
> +
> +#include "svn_subst.h" /* for svn_boolean_t, svn_error_t */
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif /* __cplusplus */
> +
> +/**
> + * Set @a *kw to a new keywords hash filled with the appropriate contents
> + * given a @a keywords_string (the contents of the svn:keywords
> + * property for the file in question), the revision @a rev, the @a url,
> + * the url of the root of the @a repos, the @a date the file was committed
> + * on, and the @a author of the last commit. Any of these can be @c NULL
> + * to indicate that the information is not present, or @c 0 for @a date.
> + *
> + * Hash keys are of type <tt>const char *</tt>.
> + * Hash values are of type <tt>svn_string_t *</tt>.
> + *
> + * All memory is allocated out of @a pool.
> + *
> + * @since New in 1.6
> + */
> +svn_error_t *
> +svn_subst_build_keywords3(apr_hash_t **kw,
> + const char *keywords_string,
> + const char *rev,
> + const char *url,
> + const char *repos,
> + apr_time_t date,
> + const char *author,
> + apr_pool_t *pool);
> +
> +
> +
> +#ifdef __cplusplus
> +}
> +#endif /* __cplusplus */
> +
> +#endif /* SVN_SUBST_PRIVATE_H */
> Index: subversion/libsvn_client/cat.c
> ===================================================================
> --- subversion/libsvn_client/cat.c (revision 1441445)
> +++ subversion/libsvn_client/cat.c (working copy)
> @@ -40,6 +40,7 @@
>
> #include "svn_private_config.h"
> #include "private/svn_wc_private.h"
> +#include "private/svn_subst_private.h"
>
>
> /*** Code. ***/
> @@ -124,12 +125,15 @@
> const char *author;
> const char *url;
> apr_time_t tm;
> + const char *repos;
>
> SVN_ERR(svn_wc__node_get_changed_info(&changed_rev, &tm, &author, wc_ctx,
> local_abspath, scratch_pool,
> scratch_pool));
> SVN_ERR(svn_wc__node_get_url(&url, wc_ctx, local_abspath, scratch_pool,
> scratch_pool));
> + SVN_ERR(svn_wc__node_get_repos_info(&repos, NULL, wc_ctx, local_abspath,
> + scratch_pool, scratch_pool));
>
> if (local_mod)
> {
> @@ -152,8 +156,8 @@
> rev_str = apr_psprintf(scratch_pool, "%ld", changed_rev);
> }
>
> - SVN_ERR(svn_subst_build_keywords2(&kw, keywords->data, rev_str, url, tm,
> - author, scratch_pool));
> + SVN_ERR(svn_subst_build_keywords3(&kw, keywords->data, rev_str, url,
> + repos, tm, author, scratch_pool));
> }
>
> /* Wrap the output stream if translation is needed. */
> @@ -181,6 +185,7 @@
> svn_string_t *eol_style;
> svn_string_t *keywords;
> apr_hash_t *props;
> + const char *repos_root_url;
> svn_stream_t *output = out;
> svn_error_t *err;
>
> @@ -225,6 +230,9 @@
> peg_revision,
> revision, ctx, pool));
>
> + /* Find the repos root URL */
> + SVN_ERR(svn_ra_get_repos_root2(ra_session, &repos_root_url, pool));
> +
> /* Grab some properties we need to know in order to figure out if anything
> special needs to be done with this file. */
> err = svn_ra_get_file(ra_session, "", loc->rev, NULL, NULL, &props, pool);
> @@ -275,10 +283,11 @@
> if (cmt_date)
> SVN_ERR(svn_time_from_cstring(&when, cmt_date->data, pool));
>
> - SVN_ERR(svn_subst_build_keywords2
> + SVN_ERR(svn_subst_build_keywords3
> (&kw, keywords->data,
> cmt_rev->data,
> loc->url,
> + repos_root_url,
> when,
> cmt_author ? cmt_author->data : NULL,
> pool));
> Index: subversion/libsvn_client/commit.c
> ===================================================================
> --- subversion/libsvn_client/commit.c (revision 1441445)
> +++ subversion/libsvn_client/commit.c (working copy)
> @@ -44,6 +44,7 @@
> #include "client.h"
> #include "private/svn_wc_private.h"
> #include "private/svn_ra_private.h"
> +#include "private/svn_subst_private.h"
>
> #include "svn_private_config.h"
>
> Index: subversion/libsvn_client/export.c
> ===================================================================
> --- subversion/libsvn_client/export.c (revision 1441445)
> +++ subversion/libsvn_client/export.c (working copy)
> @@ -45,6 +45,7 @@
> #include "private/svn_subr_private.h"
> #include "private/svn_delta_private.h"
> #include "private/svn_wc_private.h"
> +#include "private/svn_subst_private.h"
>
> #ifndef ENABLE_EV2_IMPL
> #define ENABLE_EV2_IMPL 0
> @@ -377,6 +378,7 @@
> {
> svn_revnum_t changed_rev = status->changed_rev;
> const char *suffix;
> + const char *repos;
> const char *url = svn_path_url_add_component2(status->repos_root_url,
> status->repos_relpath,
> scratch_pool);
> @@ -395,10 +397,13 @@
> suffix = "";
> }
>
> - SVN_ERR(svn_subst_build_keywords2
> + SVN_ERR(svn_wc__node_get_repos_info(&repos, NULL, wc_ctx,
> + eib->origin_abspath /* XXX: ?? from_abspath */,
> + scratch_pool, scratch_pool));
> + SVN_ERR(svn_subst_build_keywords3
> (&kw, keywords->data,
> apr_psprintf(scratch_pool, "%ld%s", changed_rev, suffix),
> - url, tm, author, scratch_pool));
> + url, repos, tm, author, scratch_pool));
> }
>
> /* For atomicity, we translate to a tmp file and then rename the tmp file
> @@ -544,6 +549,7 @@
> /* Any keyword vals to be substituted */
> const char *revision;
> const char *url;
> + const char *repos;
> const char *author;
> apr_time_t date;
>
> @@ -665,6 +671,7 @@
> fb->edit_baton = eb;
> fb->path = full_path;
> fb->url = full_url;
> + fb->repos = eb->root_url;
> fb->pool = pool;
>
> *baton = fb;
> @@ -830,8 +837,8 @@
> }
>
> if (fb->keywords_val)
> - SVN_ERR(svn_subst_build_keywords2(&final_kw, fb->keywords_val->data,
> - fb->revision, fb->url, fb->date,
> + SVN_ERR(svn_subst_build_keywords3(&final_kw, fb->keywords_val->data,
> + fb->revision, fb->url, fb->repos, fb->date,
> fb->author, pool));
>
> SVN_ERR(svn_subst_copy_and_translate4(fb->tmppath, fb->path,
> Index: subversion/libsvn_client/import.c
> ===================================================================
> --- subversion/libsvn_client/import.c (revision 1441445)
> +++ subversion/libsvn_client/import.c (working copy)
> @@ -49,6 +49,7 @@
> #include "private/svn_subr_private.h"
> #include "private/svn_ra_private.h"
> #include "private/svn_magic.h"
> +#include "private/svn_subst_private.h"
>
> #include "svn_private_config.h"
>
> @@ -133,9 +134,9 @@
> }
>
> if (keywords_val)
> - SVN_ERR(svn_subst_build_keywords2(&keywords, keywords_val->data,
> + SVN_ERR(svn_subst_build_keywords3(&keywords, keywords_val->data,
> APR_STRINGIFY(SVN_INVALID_REVNUM),
> - "", 0, "", pool));
> + "", "", 0, "", pool));
> else
> keywords = NULL;
>
> Index: subversion/libsvn_subr/subst.c
> ===================================================================
> --- subversion/libsvn_subr/subst.c (revision 1441445)
> +++ subversion/libsvn_subr/subst.c (working copy)
> @@ -49,6 +49,7 @@
> #include "svn_private_config.h"
>
> #include "private/svn_string_private.h"
> +#include "private/svn_subst_private.h"
>
> /**
> * The textual elements of a detranslated special file. One of these
> @@ -135,8 +136,11 @@
> * %b basename of the URL of this file
> * %d short format of date of this revision
> * %D long format of date of this revision
> + * %P path relative to root of repos
> * %r number of this revision
> + * %R root url of repository
> * %u URL of this file
> + * %_ a space
> * %% a literal %
> *
> * All memory is allocated out of @a pool.
> @@ -145,12 +149,14 @@
> keyword_printf(const char *fmt,
> const char *rev,
> const char *url,
> + const char *repos,
> apr_time_t date,
> const char *author,
> apr_pool_t *pool)
> {
> svn_stringbuf_t *value = svn_stringbuf_ncreate("", 0, pool);
> const char *cur;
> + const char *relative;
> size_t n;
>
> for (;;)
> @@ -203,6 +209,23 @@
> svn_stringbuf_appendcstr(value,
> svn_time_to_human_cstring(date, pool));
> break;
> + case 'P': /* relative path of this file */
> + relative = url;
> + if (relative && repos)
> + {
> + int len = strlen(repos);
> +
> + if (strncmp(repos, relative, len) == 0
> + && relative[len] == '/')
> + relative += len + 1;
> + }
> + if (relative)
> + svn_stringbuf_appendcstr(value, relative);
> + break;
> + case 'R': /* root of repos */
> + if (repos)
> + svn_stringbuf_appendcstr(value, repos);
> + break;
> case 'r': /* number of this revision */
> if (rev)
> svn_stringbuf_appendcstr(value, rev);
> @@ -211,6 +234,9 @@
> if (url)
> svn_stringbuf_appendcstr(value, url);
> break;
> + case '_': /* '%_' => a space */
> + svn_stringbuf_appendbytes(value, " ", 1);
> + break;
> case '%': /* '%%' => a literal % */
> svn_stringbuf_appendbyte(value, *cur);
> break;
> @@ -246,8 +272,8 @@
> apr_hash_t *kwhash;
> const svn_string_t *val;
>
> - SVN_ERR(svn_subst_build_keywords2(&kwhash, keywords_val, rev,
> - url, date, author, pool));
> + SVN_ERR(svn_subst_build_keywords3(&kwhash, keywords_val, rev,
> + url, "", date, author, pool));
>
> /* The behaviour of pre-1.3 svn_subst_build_keywords, which we are
> * replicating here, is to write to a slot in the svn_subst_keywords_t
> @@ -286,6 +312,21 @@
> const char *author,
> apr_pool_t *pool)
> {
> + SVN_ERR(svn_subst_build_keywords3(kw, keywords_val, rev,
> + url, "", date, author, pool));
> + return SVN_NO_ERROR;
> +}
> +
> +svn_error_t *
> +svn_subst_build_keywords3(apr_hash_t **kw,
> + const char *keywords_val,
> + const char *rev,
> + const char *url,
> + const char *repos,
> + apr_time_t date,
> + const char *author,
> + apr_pool_t *pool)
> +{
> apr_array_header_t *keyword_tokens;
> int i;
> *kw = apr_hash_make(pool);
> @@ -296,14 +337,32 @@
> for (i = 0; i < keyword_tokens->nelts; ++i)
> {
> const char *keyword = APR_ARRAY_IDX(keyword_tokens, i, const char *);
> + apr_array_header_t *keyword_tokens2;
>
> + keyword_tokens2 = svn_cstring_split(keyword, "=", TRUE /* chop */, pool);
> + if (keyword_tokens2->nelts == 2)
> + {
> + svn_string_t *custom_val;
> + const char *custom_expand;
> +
> + keyword = APR_ARRAY_IDX(keyword_tokens2, 0, const char*);
> + custom_expand = APR_ARRAY_IDX(keyword_tokens2, 1, const char*);
> + if (! strcmp(custom_expand, "%H"))
> + custom_expand = "%P %r %d %a";
> + else if (! strcmp(custom_expand, "%I"))
> + custom_expand = "%b %r %d %a";
> + custom_val = keyword_printf(custom_expand, rev, url, repos, date, author, pool);
> + apr_hash_set(*kw, keyword, APR_HASH_KEY_STRING, custom_val);
> + return SVN_NO_ERROR;
> + }
> +
> if ((! strcmp(keyword, SVN_KEYWORD_REVISION_LONG))
> || (! strcmp(keyword, SVN_KEYWORD_REVISION_MEDIUM))
> || (! svn_cstring_casecmp(keyword, SVN_KEYWORD_REVISION_SHORT)))
> {
> svn_string_t *revision_val;
>
> - revision_val = keyword_printf("%r", rev, url, date, author, pool);
> + revision_val = keyword_printf("%r", rev, url, repos, date, author, pool);
> apr_hash_set(*kw, SVN_KEYWORD_REVISION_LONG,
> APR_HASH_KEY_STRING, revision_val);
> apr_hash_set(*kw, SVN_KEYWORD_REVISION_MEDIUM,
> @@ -316,7 +375,7 @@
> {
> svn_string_t *date_val;
>
> - date_val = keyword_printf("%D", rev, url, date, author, pool);
> + date_val = keyword_printf("%D", rev, url, repos, date, author, pool);
> apr_hash_set(*kw, SVN_KEYWORD_DATE_LONG,
> APR_HASH_KEY_STRING, date_val);
> apr_hash_set(*kw, SVN_KEYWORD_DATE_SHORT,
> @@ -327,7 +386,7 @@
> {
> svn_string_t *author_val;
>
> - author_val = keyword_printf("%a", rev, url, date, author, pool);
> + author_val = keyword_printf("%a", rev, url, repos, date, author, pool);
> apr_hash_set(*kw, SVN_KEYWORD_AUTHOR_LONG,
> APR_HASH_KEY_STRING, author_val);
> apr_hash_set(*kw, SVN_KEYWORD_AUTHOR_SHORT,
> @@ -338,7 +397,7 @@
> {
> svn_string_t *url_val;
>
> - url_val = keyword_printf("%u", rev, url, date, author, pool);
> + url_val = keyword_printf("%u", rev, url, repos, date, author, pool);
> apr_hash_set(*kw, SVN_KEYWORD_URL_LONG,
> APR_HASH_KEY_STRING, url_val);
> apr_hash_set(*kw, SVN_KEYWORD_URL_SHORT,
> @@ -348,7 +407,7 @@
> {
> svn_string_t *id_val;
>
> - id_val = keyword_printf("%b %r %d %a", rev, url, date, author,
> + id_val = keyword_printf("%b %r %d %a", rev, url, repos, date, author,
> pool);
> apr_hash_set(*kw, SVN_KEYWORD_ID,
> APR_HASH_KEY_STRING, id_val);
> @@ -357,7 +416,7 @@
> {
> svn_string_t *header_val;
>
> - header_val = keyword_printf("%u %r %d %a", rev, url, date, author,
> + header_val = keyword_printf("%u %r %d %a", rev, url, repos, date, author,
> pool);
> apr_hash_set(*kw, SVN_KEYWORD_HEADER,
> APR_HASH_KEY_STRING, header_val);
> Index: subversion/libsvn_wc/translate.c
> ===================================================================
> --- subversion/libsvn_wc/translate.c (revision 1441445)
> +++ subversion/libsvn_wc/translate.c (working copy)
> @@ -46,6 +46,7 @@
>
> #include "svn_private_config.h"
> #include "private/svn_wc_private.h"
> +#include "private/svn_subst_private.h"
>
>
>
> @@ -313,10 +314,10 @@
> apr_time_t changed_date;
> const char *changed_author;
> const char *url;
> + const char *repos_root_url;
>
> if (! for_normalization)
> {
> - const char *repos_root_url;
> const char *repos_relpath;
>
> SVN_ERR(svn_wc__db_read_info(NULL, NULL, NULL, &repos_relpath,
> @@ -341,13 +342,23 @@
> changed_rev = SVN_INVALID_REVNUM;
> changed_date = 0;
> changed_author = "";
> +
> + SVN_ERR(svn_wc__db_read_info(NULL, NULL, NULL, NULL,
> + &repos_root_url, NULL, NULL,
> + NULL, NULL, NULL,
> + NULL, NULL, NULL, NULL, NULL, NULL,
> + NULL, NULL, NULL, NULL, NULL, NULL, NULL,
> + NULL, NULL, NULL, NULL,
> + db, local_abspath,
> + scratch_pool, scratch_pool));
> }
>
> - SVN_ERR(svn_subst_build_keywords2(keywords,
> + SVN_ERR(svn_subst_build_keywords3(keywords,
> keyword_list,
> apr_psprintf(scratch_pool, "%ld",
> changed_rev),
> url,
> + repos_root_url,
> changed_date,
> changed_author,
> result_pool));
PATCH: keyword expansion 1.8
Posted by Alfred Perlstein <br...@mu.org>.
[[ Adding users@subversion.apache.org ]]
He folks. I am in no way a Subversion expert, however per request I've
taken the FreeBSD patch for adding custom keyword expansion and forward
ported it to what is soon to be Subversion 1.8.
I am wondering if anyone can help me test this or give me some things to
try to ensure that it works properly.
-Alfred
On 2/1/13 7:40 AM, Alfred Perlstein wrote:
> [[ bringing in Lev Serebryakov, FreeBSD port maintainer for Subversion ]]
>
> Hello Lev,
>
> Stefan of the Subversion project asked me about bringing in our
> keywords patch for subversion 1.8.
>
> I've taken a bit of time and gotten the patch to apply to subversion
> trunk (untested).
>
> Can you have a look and let us know what we need to do with regards to
> testing or anything else?
>
>
> Stefan, any suggestions on going forward?
>
> -Alfred
>
> On 2/1/13 8:34 AM, Stefan Sperling wrote:
>> On Fri, Feb 01, 2013 at 08:01:02AM -0500, Alfred Perlstein wrote:
>>> [[ reply private ]]
>>>
>>> First of all, thank you very much for taking the time to explain this.
>>>
>>> I've forwarded your response to a few people who are more
>>> knowledgeable about Subversion in our community (John Baldwin /
>>> Peter Wemm) to hopefully get back us on the issue of mergeinfo and
>>> reintegrate.
>> Thanks!
>>
>>> Regarding the patch:
>>> http://subversion.tigris.org/issues/show_bug.cgi?id=890
>>> I just read this. What can I do to help on this issue?
>> See the mailing list thread linked from
>> http://subversion.tigris.org/issues/show_bug.cgi?id=890#desc40
>> That's the last time the patch got reviewed. It needs to be updated
>> to Subversion's trunk (to-be-1.8) before it can be committed.
>>
>
Re: FreeBSD project and subversion.
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Jan 31, 2013 at 10:37:14AM -0500, Alfred Perlstein wrote:
> FreeBSD has moved to Subversion a few years ago and it's worked
> very, very well for us.
Thanks! That's encouraging to hear.
> The one area where we are having issues is merging code from project
> branches back into trunk.
>
> The typical workflow is:
> 1) create project branch.
> 2) code code code.
> 3) sync from HEAD (this works great).
> 4) repeat steps 2+3 until feature is complete.
> 5) svn diff and apply to head then commit.
>
> Is there a way to do 5 automatically?
>
> I think the worry is mergeinfo from the project branch coming back
> into HEAD.
>
> Any tips would be appreciated.
I've read the FreeBSD svn merging docs some time ago. I'm not sure if
changes have been made since, but it was probably an older version
of what currently lives at this URL:
http://www.freebsd.org/doc/en_US.ISO8859-1/articles/committers-guide/subversion-primer.html
Back then I was somewhat worried that apparently the person who wrote them
didn't really understand much about how merges in Subversion work.
There was irrational fear of mergeinfo propagation, to the point where
the recommended practice is to remove mergeinfo before commit, which
any Subversion user with a clue will know is very wrong (expect in very
exceptional circumstances and only if you are equipped with sufficient
expertise to deal with the consequences).
What surprised me also was a complete lack of mention of the --reintegrate
option, which I suspect must be causing quite a lot of grief among FreeBSD
developers due to bogus conflicts during merges back into FreeBSD's head
branch (i.e. the trunk).
We'll need more details to help you in a constructive way, though.
Can you provide more details about your steps 1 to 5, i.e. show the
exact commands you're running in each step? The repository is public,
after all, which will help greatly with identifying and reproducing
specific problems.
I'm happy to provide input for improving FreeBSD's docs to the point
where FreeBSD makes the best possible use of Subversion 1.7's merge
implementation, and can also provide some hints as to how future versions
of Subversion will improve to make life easier in certain cases.
BTW, I went over one of FreeBSD's svn wiki pages a while back, and added
answers to open questions on this page:
https://wiki.freebsd.org/SubversionMissing
One more point, which I like to bring up whenever a FreeBSD person shows
up on our lists: There is an outstanding patch in the FreeBSD ports tree
which should be shepherded into Subversion's upstream sources. I'm happy
to be the upstream point of contact for this but we also need someone
from the FreeBSD project to drive this and digest our feedback. Because
we want this feature to be generally usable and also avoid breaking
functionality that FreeBSD is relying on in their custom patch, of
course. So both sides need to be involved. See here for details:
http://subversion.tigris.org/issues/show_bug.cgi?id=890
(and also the mailing list threads linked from the issue)