Re: svn commit: r1908812 - /subversion/branches/pristine-checksum-salt/BRANCH-README

[Daniel Shahaf]

[Replying to the most recent commit to BRANCH-README:]

kot...@apache.org wrote on Thu, 30 Mar 2023 19:04 +00:00:
> Author: kotkov
> Date: Thu Mar 30 19:04:13 2023
> New Revision: 1908812
>
> URL: http://svn.apache.org/viewvc?rev=1908812=rev
> Log:
> On the 'pristine-checksum-salt' branch: Update BRANCH-README.
>
> * BRANCH-README
>   (Dynamically salted SHA-1 checksums): Extend and update this section.
>
> Modified:
> subversion/branches/pristine-checksum-salt/BRANCH-README
>
> Modified: subversion/branches/pristine-checksum-salt/BRANCH-README
> URL: 
> http://svn.apache.org/viewvc/subversion/branches/pristine-checksum-salt/BRANCH-README?rev=1908812=1908811=1908812=diff
> ==
> --- subversion/branches/pristine-checksum-salt/BRANCH-README (original)
> +++ subversion/branches/pristine-checksum-salt/BRANCH-README Thu Mar 30 
> 19:04:13 2023
> @@ -13,5 +13,26 @@ as currently implemented, will use the n
>  Dynamically salted SHA-1 checksums
>  --
> 
> -The implementation on the branch uses a dynamically salted SHA-1 checksum 
> kind.
> -The dynamic salt is generated during the creation of a wc.db.
> +The working copy currently relies on an assumption that files with identical
> +checksum values have identical content.  For SHA-1, there are publicly known
> +checksum collisions [https://shattered.io] and the situation may become worse
> +with the feasibility of chosen-prefix attacks [https://sha-mbles.github.io].
> +
> +To solve the potential problems and to improve the current state around 
> checksum
> +collisions, the implementation on the branch starts using a dynamically 
> salted
> +SHA-1 checksum kind.
> +
> +The 32-byte random salt is generated during the creation of a wc.db.  When 
> the
> +file content is checksummed, the checksum value is calculated as if the salt 
> was
> +prepended to the content.  In other words, checksum = SHA1(content) becomes
> +checksum = SHA1(salt + content).
> +
> +With the dynamic salt:
> +
> +- Publicly known SHA-1 collisions no longer result in collisions when 
> checksummed
> +  by the working copy.  This is because the actually checksummed content now
> +  includes the random prefix salt.
> +
> +- Constructing a chosen-prefix SHA-1 collision no longer results in a 
> collision
> +  when checksummed by the working copy.  This is because the constructed 
> collision
> +  cannot account for the random prefix salt, because it's unknown in advance.

For context, this is similar to 
https://mail-archives.apache.org/mod_mbox/subversion-dev/202301.mbox/%3cadacbb6f-e0cb-4e5b-8603-0eda19f93...@app.fastmail.com%3E
 but with the suffixing changed to prefixing.

(Changing suffixing to prefixing makes sense, since SHAttered collisions
have the property that appending an identical suffix to two colliding
files generates two other colliding files.)

Cheers,

Daniel

Re: svn commit: r1902590 - /subversion/trunk/tools/client-side/store-plaintext-password.py

[Daniel Shahaf]

Nathan Hartman wrote on Thu, Jul 14, 2022 at 10:45:07 -0400:
> On Thu, Jul 14, 2022 at 10:02 AM Daniel Sahlberg
>  wrote:
> >
> > Den tors 14 juli 2022 kl 15:52 skrev Daniel Shahaf 
> > :
> >>
> >> Nathan Hartman wrote on Wed, 13 Jul 2022 15:29 +00:00:
> >> > On Wed, Jul 13, 2022 at 10:55 AM Daniel Shahaf  
> >> > wrote:
> >> >> Should the entry link to the zsh script
> >> >> (https://mail-archives.apache.org/mod_mbox/subversion-dev/202008.mbox/%3C20200816130713.6abca815%40tarpaulin.shahaf.local2%3E)
> >> >> as well, as an alternative?  It might be useful for someone if their
> >> >> environment doesn't have Python installed or if they find the zsh script
> >> >> easier to audit.
> >> >
> >> > I think it would be useful, and...
> >> >
> >> >> (Well, I suppose it might make more sense to copy the script
> >> >> somewhere than to link to an immutable archives message with that
> >> >> subject line.)
> >> >
> >> > ...the place to put it is probably tools/client-side/ just like the
> >> > Python script.
> >>
> >> Being in tools/ would imply dev@ accepts responsibility for bug reports
> >> against the zsh script.  Is dev@ happy to do that?  I'm concerned about
> >> the bus factor.
> >
> >
> > I was just about to say the same thing (and with no intention to
> > discredit the zsh version). If it is desirable to list all available
> > realms and let the user choose interactively, I could add that to
> > the Python script.

Adding such functionality would reduce the amount of legwork for users
(= would move the Python script leftwards on <https://xkcd.com/1343/>).

> > I was also going to add that I think it is better to provide one
> > tool and make sure that tool is working well instead of having two
> > tools that differ only in tiny details, since they might bit-rot in
> > different ways over time and it might be hard for a newcomer to
> > understand the motivation of having different tools.
> 

Agreed: knobs have a cost both to maintainers and to users.  However, we
should balance this downside with potential upsides, such as the ones I
offered above:

> >> >> [...]  It might be useful for someone if their environment
> >> >> doesn't have Python installed or if they find the zsh script
> >> >> easier to audit.

I'm not implying those points outweigh Daniel's; I'm just saying we
have identified pros and cons but haven't tallied them up yet.

For instance, perhaps we should link to both implementations but make it
clear that the Python one is preferred, community supported, "Use it
unless you know you need the other one", etc..

> 
> These are all good points.
> 
> I admit that zsh is a bit of a mystery to me, as is the script, so I
> couldn't provide support for it, at least not with my current
> knowledge. I am impressed that zsh can do so much with so little.
> 

zsh syntax can be terse, but the script is pretty much translatable
line-for-line into Python, except for the 'select' loop:

https://zsh.sourceforge.io/Doc/Release/Shell-Grammar.html#index-select

… which would be this:

def select(choices):
for i_and_element in enumerate(choices):
print("{}: {}".format(*i_and_element))
n = int(input("Choice number: "))
if not (0 <= n < len(choices)):
raise ...
return choices[n]

(plus a few more lines for the argv and loop support)

> It's in the list archives, but as DanielSh points out, is in a thread
> with a not-so-nice subject. That could be addressed by re-mailing it
> to dev@ with a new subject, e.g., "Prototype zsh script to store svn
> password in plaintext" in case anyone ever asks or searches for a
> non-Python way to do it. We could even link to it from the same FAQ,
> e.g., "An example of how to store svn plaintext credentials was
> implemented as a zsh script. It is unsupported by the SVN maintainers
> but can be found at [link] for pedagogical purposes."

If we give the script a new URL, perhaps we could make that URL identify
a _mutable_ resource, so if we ever have to update the script all its
users won't have to update their bookmarks?  Just a nice-to-have.

Cheers,

Daniel

Re: svn commit: r1902722 - in /subversion/trunk/tools/dist: make-keys.sh release.py

[Daniel Shahaf]

dsahlb...@apache.org wrote on Thu, Jul 14, 2022 at 19:40:58 -:
> Author: dsahlberg
> Date: Thu Jul 14 19:40:57 2022
> New Revision: 1902722
> 
> URL: http://svn.apache.org/viewvc?rev=1902722=rev
> Log:
> Follow-up to r1902582: Improvements to the release.py script
> 
> Suggested by: danielsh
> 
> * tools/dist/make-keys.sh:
>   Make the -c argument expect the NAME of the COMMITTERS file, to make it
>   easier in the future to point to a file with another (temporary) name.
> 
> * tools/dist/release.py
>   (get_keys): Better use of NamedTemporaryFile. Doesn't work on Windows
>   (according to Python docs) but there are other release process
>   requirements mandating the use of *nix so it should be ok.

The callsite in roll_tarballs() hasn't been updated.

> +++ subversion/trunk/tools/dist/release.py Thu Jul 14 19:40:57 2022
> @@ -1469,11 +1469,10 @@ def check_sigs(args):
>  
>  def get_keys(args):
>  'Import the LDAP-based KEYS file to gpg'
> -with tempfile.NamedTemporaryFile(delete=False) as tmpfile:
> +with tempfile.NamedTemporaryFile() as tmpfile:
>keyspath = tmpfile.name
> -subprocess.check_call([os.path.dirname(__file__) + '/make-keys.sh', 
> '-c', os.path.dirname(__file__) + '/../..', '-o', keyspath])
> -subprocess.check_call(['gpg', '--import', keyspath])
> -os.remove(keyspath)
> +  subprocess.check_call([os.path.dirname(__file__) + '/make-keys.sh', 
> '-c', os.path.dirname(__file__) + '/../../COMMITTERS', '-o', keyspath])
> +  subprocess.check_call(['gpg', '--import', keyspath])

Thanks.  LGTM.  And considering the code did replaces did already do the
urlopen() thing, here's a stab at it:

[[[
Index: release.py
===
--- release.py  (revision 1902730)
+++ release.py  (working copy)
@@ -1470,11 +1470,18 @@ def check_sigs(args):
 def get_keys(args):
 'Import the LDAP-based KEYS file to gpg'
 with tempfile.NamedTemporaryFile() as keysfile:
-  subprocess.check_call([
-  os.path.dirname(__file__) + '/make-keys.sh',
-  '-c', os.path.dirname(__file__) + '/../../COMMITTERS',
-  '-o', keysfile.name,
-  ])
+  with tempfile.NamedTemporaryFile() as committersfile:
+shutil.copyfileobj(
+urlopen(svn_repos + "/trunk/COMMITTERS"),
+committersfile,
+)
+committersfile.flush()
+#committersfile.seek(0) # Remember to uncomment this line if needed
+subprocess.check_call([
+os.path.dirname(__file__) + '/make-keys.sh',
+'-c', committersfile.name,
+'-o', keysfile.name,
+])
   subprocess.check_call(['gpg', '--import', keysfile.name])
 
 def add_to_changes_dict(changes_dict, audience, section, change, revision):
]]]

WDYT, Daniel?

If this works we can also revert the HACKING change on staging to reduce
the dependencies of rolling.

Cheers,

Daniel

Re: svn commit: r1902723 - /subversion/site/staging/docs/community-guide/releasing.part.html

[Daniel Shahaf]

dsahlb...@apache.org wrote on Thu, 14 Jul 2022 19:51 +00:00:
> +++ subversion/site/staging/docs/community-guide/releasing.part.html Thu Jul 
> 14 19:51:28 2022
> @@ -827,8 +827,7 @@ time pass.
> To run this script, you'll need a Subversion trunk working
> -copy (or a shallow trunk working copy containing the tools/dist and
> -build/generator directories).
> +copy.

How about keeping the parenthesized list and adding COMMITTERS to it?
This would make it easier to minimize dependencies on the trunk tree,
which would be a good thing (e.g., it would be harder to accidentally
use trunk's svn_version.h instead of a branch's).

Re: svn commit: r1902590 - /subversion/trunk/tools/client-side/store-plaintext-password.py

[Daniel Shahaf]

Nathan Hartman wrote on Wed, 13 Jul 2022 15:29 +00:00:
> On Wed, Jul 13, 2022 at 10:55 AM Daniel Shahaf  
> wrote:
>> Should the entry link to the zsh script
>> (https://mail-archives.apache.org/mod_mbox/subversion-dev/202008.mbox/%3C20200816130713.6abca815%40tarpaulin.shahaf.local2%3E)
>> as well, as an alternative?  It might be useful for someone if their
>> environment doesn't have Python installed or if they find the zsh script
>> easier to audit.
>
> I think it would be useful, and...
>
>> (Well, I suppose it might make more sense to copy the script
>> somewhere than to link to an immutable archives message with that
>> subject line.)
>
> ...the place to put it is probably tools/client-side/ just like the
> Python script.

Being in tools/ would imply dev@ accepts responsibility for bug reports
against the zsh script.  Is dev@ happy to do that?  I'm concerned about
the bus factor.

Cheers,

Daniel

Re: svn commit: r1902590 - /subversion/trunk/tools/client-side/store-plaintext-password.py

[Daniel Shahaf]

Nathan Hartman wrote on Wed, 13 Jul 2022 13:43 +00:00:
> On Wed, Jul 13, 2022 at 9:33 AM Daniel Shahaf 
> wrote:
>
>> dsahlb...@apache.org wrote on Fri, Jul 08, 2022 at 23:39:14 -:
>> > A new script to store/update a password in the plain text password store
>> >
>> > * tools/client-side/store-plaintext-password.py
>> >   As above
>> >
>> > Discussed on dev@:
>> https://lists.apache.org/thread/jfd0f5n2qpgnyc30dst6ycnkphcwf6mm
>> >
>> > Added:
>> > subversion/trunk/tools/client-side/store-plaintext-password.py
>>  (with props)
>>
>> Presumably, now that it's been added, we should link it from somewhere
>> to make it discoverable by users?
>
>
>
> Ah yes, it is on my todo list to link to it from the FAQ [1]. :-)
>
> [1] https://subversion.apache.org/faq.html#plaintext-passwords

Added to staging in r1902704.  Hope you don't mind :)  Please take it
from here if you have time.

Should the entry link to the zsh script
(https://mail-archives.apache.org/mod_mbox/subversion-dev/202008.mbox/%3C20200816130713.6abca815%40tarpaulin.shahaf.local2%3E)
as well, as an alternative?  It might be useful for someone if their
environment doesn't have Python installed or if they find the zsh script
easier to audit.

(Well, I suppose it might make more sense to copy the script
somewhere than to link to an immutable archives message with that
subject line.)

Cheers,

Daniel

Re: svn commit: r1902582 - /subversion/trunk/tools/dist/release.py

[Daniel Shahaf]

Daniel Sahlberg wrote on Fri, Jul 08, 2022 at 23:07:08 +0200:
> Den fre 8 juli 2022 kl 22:47 skrev :
> 
> > Author: dsahlberg
> > Date: Fri Jul  8 20:47:42 2022
> > New Revision: 1902582
> >
> > URL: http://svn.apache.org/viewvc?rev=1902582=rev
> > Log:
> > ASF no longer provide a aggregated KEYS file, so we need to construct it
> > ourselves using the make-keys.sh script.
> >
> > * tools/dist/release.py
> >   (roll_tarballs): Call make-keys.sh to create the KEYS file
> >   (get_keys): Call make-keys.sh to create the KEYS file
> >
> > Modified:
> > subversion/trunk/tools/dist/release.py
> >
> > Modified: subversion/trunk/tools/dist/release.py
> > URL:
> > http://svn.apache.org/viewvc/subversion/trunk/tools/dist/release.py?rev=1902582=1902581=1902582=diff
> >
> > ==
> > --- subversion/trunk/tools/dist/release.py (original)
> > +++ subversion/trunk/tools/dist/release.py Fri Jul  8 20:47:42 2022
> > @@ -98,7 +98,6 @@ dist_release_url = dist_repos + '/releas
> >  dist_archive_url = 'https://archive.apache.org/dist/subversion'
> >  buildbot_repos = os.getenv('SVN_RELEASE_BUILDBOT_REPOS',
> > '
> > https://svn.apache.org/repos/infra/infrastructure/buildbot/aegis/buildmaster
> > ')
> > -KEYS = 'https://people.apache.org/keys/group/subversion.asc'
> >  extns = ['zip', 'tar.gz', 'tar.bz2']
> >
> >
> > @@ -980,7 +979,12 @@ def roll_tarballs(args):
> >  # from a committer's LDAP profile down the road)
> >  basename = 'subversion-%s.KEYS' % (str(args.version),)
> >  filepath = os.path.join(get_tempdir(args.base_dir), basename)
> > -download_file(KEYS, filepath, None)
> > +# The following code require release.py to be executed within a
> > +# complete wc, not a shallow wc as indicated in HACKING as one
> > option.
> > +# We /could/ download COMMITTERS from /trunk if it doesn't
> > exist...
> > +subprocess.check_call([os.path.dirname(__file__) +
> > '/make-keys.sh',
> > +   '-c', os.path.dirname(__file__) + '/../..',
> > +   '-o', filepath])
> >  shutil.move(filepath, get_target(args))
> >
> 
> I have tested the above part but NOT within the full roll_tarballs codepath
> since I'm not sure if I might cause changes in the repository. I believe
> the change is correct and I don't think things will be worse than trying to
> download a non-existing URL but I would appreciate the help from someone
> experienced in the release process to review or at least give me the
> confidence to roll a tarball locally.

IIRC, rolling the tarballs in itself just creates the foo.tar.gz files
locally; it doesn't create the tag or do the post-tagging housekeeping
commits.

To be sure it doesn't commit, you can invalidate or delete any caches of
your svn.apache.org password.  Or you could create another local user on
your OS and test from that.  The test user should have its own UID,
homedir, and environment, so it doesn't have access to your regular
user's cached usernames/passwords.

Re: svn commit: r1902582 - /subversion/trunk/tools/dist/release.py

[Daniel Shahaf]

dsahlb...@apache.org wrote on Fri, Jul 08, 2022 at 20:47:42 -:
> +++ subversion/trunk/tools/dist/release.py Fri Jul  8 20:47:42 2022
> @@ -980,7 +979,12 @@ def roll_tarballs(args):
>  # from a committer's LDAP profile down the road)
>  basename = 'subversion-%s.KEYS' % (str(args.version),)
>  filepath = os.path.join(get_tempdir(args.base_dir), basename)
> -download_file(KEYS, filepath, None)
> +# The following code require release.py to be executed within a
> +# complete wc, not a shallow wc as indicated in HACKING as one 
> option.
> +# We /could/ download COMMITTERS from /trunk if it doesn't exist...

Well, could you please either change HACKING or download COMMITTERS?
The code for the latter is basically the tempfile+urlopen mechanics from
the next hunk of this very diff.

> +subprocess.check_call([os.path.dirname(__file__) + '/make-keys.sh',
> +   '-c', os.path.dirname(__file__) + '/../..',
> +   '-o', filepath])
>  shutil.move(filepath, get_target(args))
>  
>  # And we're done!
> @@ -1465,12 +1469,11 @@ def check_sigs(args):
>  
>  def get_keys(args):
>  'Import the LDAP-based KEYS file to gpg'
> -# We use a tempfile because urlopen() objects don't have a .fileno()
> -with tempfile.SpooledTemporaryFile() as fd:
> -fd.write(urlopen(KEYS).read())
> -fd.flush()
> -fd.seek(0)
> -subprocess.check_call(['gpg', '--import'], stdin=fd)
> +with tempfile.NamedTemporaryFile(delete=False) as tmpfile:
> +  keyspath = tmpfile.name
> +subprocess.check_call([os.path.dirname(__file__) + '/make-keys.sh', 
> '-c', os.path.dirname(__file__) + '/../..', '-o', keyspath])
> +subprocess.check_call(['gpg', '--import', keyspath])
> +os.remove(keyspath)

That's not how one uses NamedTemporaryFile().

Generally, all uses of the file should be inside the «with» block, and
unlinking the file should be left to block's implicit handling
(tmpfile.__exit__()).

As written, however, NamedTemporaryFile() is used as though it were
a "generate a safe temporary name" API.  That means the file is not
created atomically and won't be cleaned up if subprocess.check_call()
raises an exception.

Could you rewrite so the file isn't used outside its «with» block?

>  def add_to_changes_dict(changes_dict, audience, section, change, revision):
>  # Normalize arguments
> 
> 

Re: svn commit: r1902590 - /subversion/trunk/tools/client-side/store-plaintext-password.py

[Daniel Shahaf]

dsahlb...@apache.org wrote on Fri, Jul 08, 2022 at 23:39:14 -:
> A new script to store/update a password in the plain text password store
> 
> * tools/client-side/store-plaintext-password.py
>   As above
> 
> Discussed on dev@: 
> https://lists.apache.org/thread/jfd0f5n2qpgnyc30dst6ycnkphcwf6mm
> 
> Added:
> subversion/trunk/tools/client-side/store-plaintext-password.py   (with 
> props)

Presumably, now that it's been added, we should link it from somewhere
to make it discoverable by users?

Cheers,

Daniel
(I have reviewed the changes you mentioned on dev@ and have no comments.)

Re: svn commit: r1900649 - in /subversion/site/publish: ./ docs/community-guide/releasing.part.html roadmap.html

[Daniel Shahaf]

dsahlb...@apache.org wrote on Sat, 07 May 2022 09:52 +00:00:
> Merge from site/staging: 1900404, 1900405, 1900528, 1900532, 1900561, 1900562
>
> Document the revised release policy as discussed on dev@ [1].
>
> * publish/docs/community-guide/releasing.part.html,
>   publish/roadmap.html:
>   Changes in several sections related to release process, see merged
>   revisions for details.
>
> [1] https://lists.apache.org/thread/17v36gol5vltyx3pv9z4wskftq7hn4zb

Both docs/release-notes/index.html and the download page have
noticebox talking about a "6-month regular and 2-year LTS release
schedule".  The constant is wrong (need s/2/4/), as well as the term
"schedule", I believe,

(I found these by grepping for links to roadmap.html#release-planning.)

Cheers,

Daniel

Re: svn commit: r1900404 - in /subversion/site/staging: docs/community-guide/releasing.part.html roadmap.html

[Daniel Shahaf]

Daniel Sahlberg wrote on Mon, 02 May 2022 20:12 +00:00:
> Thanks to everyone for discussing this and moving it forward! I'm sorry I
> wasn't able to be more active last week but life got in the way.
>
> One small point below...
>
> Den lör 30 apr. 2022 kl 00:04 skrev :
> [...]
>
>> +LTS releases are supported for four years from the date of
>> their
>> +initial release.  For instance, 1.15.x will supported until four years
>> after
>> +the announcement of 1.15.0.
>>
>
> Should we really declare 1.15 an LTS release at this stage?

No.  Deciding whether 1.15 should be LTS or Regular deserves a thread of
its own.  As far as this thread is concerned, the documentation should
reflect the status quo: that it has not been decided yet whether 1.15
will be LTS or Regular.

Good catch.

If someone could please update the text staging/ that would be great.

> I would also suggest to remove the "Transition to LTS and Regular 
> Releases"
> section (
> https://subversion-staging.apache.org/roadmap.html#transition-lts-regular-releases)
> since it seems to concern the fixed-time release schedule. I can do 
> this,
> just wanting to check that I don't missread something.

The description of what we backport is "general backports and thereafter
high priority fixes" in this section, and "high priority issues such as
… and sometimes also other issues" in the section above.  We might want
to clarify the "other issues" part of the latter sentence when we delete
this section.

Also, might want to explicitly spell out that 1.10 is now EOL: someone
might think that 1.10 would be supported with security fixes until the
LTS _after 1.14_ is released, as that would have been the case under our
pre-1.11 policy if there hadn't been Regular releases at all.

Also, to answer your question in the OP, we'll want to remove 1.10 from
the download page and from dist/release/.

Cheers,

Daniel

Re: svn commit: r1899276 - /subversion/site/publish/upcoming.part.html

[Daniel Shahaf]

Daniel Sahlberg wrote on Mon, Mar 28, 2022 at 21:55:36 +0200:
> Den mån 28 mars 2022 kl 09:55 skrev Daniel Sahlberg <
> daniel.l.sahlb...@gmail.com>:
> 
> > This commit doesn't look correct.
> >
> > I executed the generate-upcoming-changes-log.sh manually yesterday and it
> > created r1899244, removing a lot of log entries belonging to 1.14.1. This
> > commit (which was executed by cron) restores them.
> >
> > The crontab entry is:
> > [[[
> > # Puppet Name: Update our upcoming changes list
> > SVN=svn
> > 15 4 * * * chronic ~/src/svn/site/tools/generate-upcoming-changes-log.sh
> > ]]]
> >
> > The script begins with a comment
> > [[[
> > # This should be run from the root of a branches/1.{9,10,11}.x working
> > copy.
> > ]]]
> >
> > I suppose the crontab command should be changed to:
> > cd ~/src/svn/1.14.x && chronic
> > ~/src/svn/site/tools/generate-upcoming-changes-log.sh
> >
> 
> So I've investigated this further and my initial analysis seems correct.
> 
> This is what happens:
> * generate-upcoming-changes-log.sh determines last patch release number and
> generates upcoming.part.html based on all commits since that patch release
> was tagged.
> * It has two different ways to determine "the last patch release":
>* If `cwd` is a WC it look in the subversion/include/svn_version.h
>* Else look in https://dist.apache.org/repos/dist/release/subversion/
> * The logic looking at dists.a.o selected the oldest patch release
> available on dists.a.o, in case there are more than one.
> * The crontab entry executed generate-upcoming-changes-log.sh from ~, thus
> forcing a lookup from dists.a.o
> 
> Currently, both 1.14.0 and 1.14.1 are available on dists.a.o. Thus it
> emitted all merged changes since 1.14.0, while expected behaviour (of
> upcoming.part.html) would be to only display merged changes since 1.14.1.
> 

dist/ should only contain the latest release from each supported minor
line, except when a release is being staged.  I.e., today it should
contain 1.14.2 and 1.14.1 since we're in the process of staging 1.14.2;
but by Friday it should contain 1.14.2 and 1.10.8 and only those two.

The script relies on this.  Thus, if we'd deleted 1.14.0's artifacts
when we released 1.14.1 and kept the cron job as it was, the output
would then have been correct (and we wouldn't have had an extra manual
step in our create-a-A.B.x-branch workflow).

> I've pushed a change in puppet to the crontab entry as suggested above.
> This should solve the problem for now. When branching 1.15.x, we need to
> update this crontab entry.

Then please update 

accordingly :)

Cheers,

Daniel

Re: svn commit: r1899373 - /subversion/branches/1.14.x/STATUS

[Daniel Shahaf]

Daniel Sahlberg wrote on Wed, Mar 30, 2022 at 07:45:01 +0200:
> So.. backports failed today as well. After some digging I realised
> backport.pl didn't pick up the branch in this nomination due to a
> whitespace issue in STATUS. I removed one space character on each line and
> the backport worked.
> 
> However one backport remains in STATUS:
> 
> svnsvn@svn-qavm1:~/src/svn/1.14.x$ for i in ~/src/svn/1.*.x; do cd $i &&
> $SVN up -q --non-interactive && YES=1 MAY_COMMIT=1 ../trunk/tools/dist/
> backport.pl; done
> Warning summary
> ===
> 
> 1.14.x-r1881534-no-crlf (Fix issue #4864 "build/ac-macros/macosx.m4:
> workaround AC_RUN_IFELSE"): Revisions 'r1881534' nominated but not included
> in branch
> svnsvn@svn-qavm1:~/src/svn/1.14.x$
> 
> Can someone check it? I'm ENOTIME to dig into it.
> 

The merge of r1881534 to its backport branch did not add that revision
to svn:mergeinfo.  Which is to say, the error message is correct.

(I already pointed this out in
,
though admittedly not too visibly.)

Cheers,

Daniel

> Kind regards,
> Daniel
> 
> 
> Den ons 30 mars 2022 kl 07:41 skrev :
> 
> > Author: dsahlberg
> > Date: Wed Mar 30 05:41:19 2022
> > New Revision: 1899373
> >
> > URL: http://svn.apache.org/viewvc?rev=1899373=rev
> > Log:
> > * STATUS: Adjust whitespace to see if it resolves backport issues
> >
> > Modified:
> > subversion/branches/1.14.x/STATUS
> >
> > Modified: subversion/branches/1.14.x/STATUS
> > URL:
> > http://svn.apache.org/viewvc/subversion/branches/1.14.x/STATUS?rev=1899373=1899372=1899373=diff
> >
> > ==
> > --- subversion/branches/1.14.x/STATUS (original)
> > +++ subversion/branches/1.14.x/STATUS Wed Mar 30 05:41:19 2022
> > @@ -46,13 +46,13 @@ Approved changes:
> >  =
> >
> >   * r1899227
> > -Don't show unreadable copyfrom paths in 'svn log -v'
> > -Justification:
> > -  Makes 'svn log -v' consistent with spec.
> > -Branch:
> > -  1.14.x-r1899227
> > -Votes:
> > -  +1: hartmannathan, dsahlberg, rhuijben
> > +   Don't show unreadable copyfrom paths in 'svn log -v'
> > +   Justification:
> > + Makes 'svn log -v' consistent with spec.
> > +   Branch:
> > + 1.14.x-r1899227
> > +   Votes:
> > + +1: hartmannathan, dsahlberg, rhuijben
> >
> >   * r1898633
> > Fix sporadic testCrash_RequestChannel_nativeRead_AfterException failure
> >
> >
> >

Re: svn commit: r1899275 - /subversion/site/publish/.message-ids.tsv

[Daniel Shahaf]

Daniel Sahlberg wrote on Mon, Mar 28, 2022 at 09:19:19 +0200:
> Anyone got an idea why the URL list was sorted in a different way on the
> new svn-qavm? Not that it is a big difference, but I don't like loose ends.
> 

Probably the locale:

[[[
% <1 LC_ALL=C sort 
http://svn.haxx.se/dev/archive-2010-08/0362.shtml
https://svn.haxx.se/users/archive-2012-09/0236.shtml
https://svn.haxx.se/users/archive-2019-04/0041.shtml
https://svn.haxx.se/users/archive-2020-04/0040.shtml
% <1 LC_ALL=en_US.UTF-8 sort 
https://svn.haxx.se/users/archive-2012-09/0236.shtml
https://svn.haxx.se/users/archive-2019-04/0041.shtml
https://svn.haxx.se/users/archive-2020-04/0040.shtml
http://svn.haxx.se/dev/archive-2010-08/0362.shtml
% 
]]]

Cheers,

Daniel


> In either case, I updated the source to use https on the offending link to
> (r1899280) so we should see another update tomorrow.
> 
> /Daniel
> 
> Den mån 28 mars 2022 kl 06:00 skrev :
> 
> > Author: svn-role
> > Date: Mon Mar 28 04:00:20 2022
> > New Revision: 1899275
> >
> > URL: http://svn.apache.org/viewvc?rev=1899275=rev
> > Log:
> > * publish/.message-ids.tsv: Automatically regenerated.
> >
> > Modified:
> > subversion/site/publish/.message-ids.tsv
> >
> > Modified: subversion/site/publish/.message-ids.tsv
> > URL:
> > http://svn.apache.org/viewvc/subversion/site/publish/.message-ids.tsv?rev=1899275=1899274=1899275=diff
> >
> > ==
> > --- subversion/site/publish/.message-ids.tsv (original)
> > +++ subversion/site/publish/.message-ids.tsv Mon Mar 28 04:00:20 2022
> > @@ -1,5 +1,6 @@
> >  # Message-ids of archived emails that are referenced by a svn.haxx.se
> > URL.
> > -# Generated by tools/haxx-url-to-message-id.sh on 2021-07-04
> > +# Generated by tools/haxx-url-to-message-id.sh on 2022-03-28
> > +http://svn.haxx.se/dev/archive-2010-08/0362.shtml
> > 4c65756c.8070...@collab.net
> >  https://svn.haxx.se/dev/archive-2003-01/1125.shtml
> > 20030116213052.314004c1.tt...@idsoftware.com
> >  https://svn.haxx.se/dev/archive-2003-02/0068.shtml
> > 87wuki4fpy@codematters.co.uk
> >  https://svn.haxx.se/dev/archive-2003-10/0136.shtml
> > 200310031235.h93czgiv064...@bigtex.jrv.org
> > @@ -55,4 +56,3 @@ https://svn.haxx.se/users/archive-2012-0
> >  https://svn.haxx.se/users/archive-2012-09/0236.shtml
> > 20120921085850.gg24...@ted.stsp.name
> >  https://svn.haxx.se/users/archive-2019-04/0041.shtml
> > 9739e241-f88c-8a79-11f5-783a7f119...@neuf.fr
> >  https://svn.haxx.se/users/archive-2020-04/0040.shtml
> > 20200422065424.gl81...@ted.stsp.name
> > -http://svn.haxx.se/dev/archive-2010-08/0362.shtml
> > 4c65756c.8070...@collab.net
> >
> >
> >

Re: svn commit: r1899247 - /subversion/branches/1.13.x/STATUS

[Daniel Shahaf]

Daniel Sahlberg wrote on Mon, Mar 28, 2022 at 10:01:26 +0200:
> Den mån 28 mars 2022 kl 02:33 skrev Daniel Shahaf :
> > Daniel Sahlberg wrote on Sun, 27 Mar 2022 23:30 +00:00:
> > > I also made some additional changes in roadmap.html (r1899268). This could
> > > probably be discussed a bit more but the "out of date" and eted
> > > sections doesn't look nice.
> >
> > All I have to say is that it'd be nice to keep somewhere either
> > a working example of those CSS classes or documentation of them… even if
> > it's just "Refer to roadmap.html@r1899267" :-)
> >
> 
> #todo is used in several other places. I don't know if we have specific CSS
> for .
> 
> This was a little bit of a flame bait to see if anyone else has an opinion
> on the eted table.

I was referring to .divider, .task-level-1, and .in-progress, which were
used to create a table with headers within the table.  They are still
defined in our CSS, but now there's no example of their use.  Sorry for
the unclarity.

Cheers,

Daniel

Re: svn commit: r1899247 - /subversion/branches/1.13.x/STATUS

[Daniel Shahaf]

Daniel Sahlberg wrote on Sun, 27 Mar 2022 23:30 +00:00:
> Den sön 27 mars 2022 kl 19:37 skrev Daniel Shahaf :
>
>> Thanks for these two changes!  The information is duplicated in
>> <https://subversion.apache.org/docs/release-notes/#supported-versions>;
>> anyone wants to fix that, too?  (Also, 1.14.x is absent from there.)
>>
>
> Done (r1899267). This should be quite straight forward and could be merged
> right away. Thanks for pointing out!
>

Thanks for fixing it.

I wonder whether we should add "1.15.x | Not yet supported" to that table.

> I also made some additional changes in roadmap.html (r1899268). This could
> probably be discussed a bit more but the "out of date" and eted
> sections doesn't look nice.

All I have to say is that it'd be nice to keep somewhere either
a working example of those CSS classes or documentation of them… even if
it's just "Refer to roadmap.html@r1899267" :-)

Cheers,

Daniel

Re: svn commit: r1899247 - /subversion/branches/1.13.x/STATUS

[Daniel Shahaf]

Thanks for these two changes!  The information is duplicated in
;
anyone wants to fix that, too?  (Also, 1.14.x is absent from there.)

dsahlb...@apache.org wrote on Sun, 27 Mar 2022 16:14 +00:00:
> Author: dsahlberg
> Date: Sun Mar 27 16:14:27 2022
> New Revision: 1899247
>
> URL: http://svn.apache.org/viewvc?rev=1899247=rev
> Log:
> * STATUS: 1.13.x is end of life (2019-10-30 + 6 months < $TODAY)
>
> Modified:
> subversion/branches/1.13.x/STATUS
>
> Modified: subversion/branches/1.13.x/STATUS
> URL: 
> http://svn.apache.org/viewvc/subversion/branches/1.13.x/STATUS?rev=1899247=1899246=1899247=diff
> ==
> --- subversion/branches/1.13.x/STATUS (original)
> +++ subversion/branches/1.13.x/STATUS Sun Mar 27 16:14:27 2022
> @@ -1,6 +1,6 @@
>* * * * * * * * * * * * * * * * * * * * * * * * * * * *
>* *
> -  *  THIS RELEASE STREAM IS OPEN FOR STABILIZATION. *
> +  *  THIS RELEASE STREAM IS CLOSED: SUPERSEDED BY 1.14. *
>* *
>* * * * * * * * * * * * * * * * * * * * * * * * * * * *

Re: svn commit: r1899014 - /subversion/trunk/subversion/tests/cmdline/upgrade_tests.py

[Daniel Shahaf]

Jun Omae wrote on Fri, Mar 18, 2022 at 11:27:16 +0900:
> Hi,
> 
> On Fri, Mar 18, 2022 at 9:44 AM Daniel Shahaf  wrote:
> >
> > Could someone test this on Windows, please?  I suspect read_wc_formats()
> > (added in r1899012) returns paths with os.sep, but the expectations
> > added in this commit use '/', so something will need to convert.
> >
> > Thanks,
> >
> > Daniel
> 
> I got 3 failures from testing r1899017 on Windows. dav-tests.log is attached.
> See also 
> https://github.com/jun66j5/subversion/runs/5594602036?check_suite_focus=true#step:7:2816
> 

Thank you!

Does r1899019 fix this?

Re: svn commit: r1899014 - /subversion/trunk/subversion/tests/cmdline/upgrade_tests.py

[Daniel Shahaf]

Could someone test this on Windows, please?  I suspect read_wc_formats()
(added in r1899012) returns paths with os.sep, but the expectations
added in this commit use '/', so something will need to convert.

Thanks,

Daniel

danie...@apache.org wrote on Fri, 18 Mar 2022 00:40 +00:00:
> Author: danielsh
> Date: Fri Mar 18 00:40:29 2022
> New Revision: 1899014
>
> URL: http://svn.apache.org/viewvc?rev=1899014=rev
> Log:
> * subversion/tests/cmdline/upgrade_tests.py
>   (upgrade_with_externals): Verify format numbers of upgraded externals.
>   (check_formats): New.
>   (check_format): Verify the argument type to guard against typos.
>
> Modified:
> subversion/trunk/subversion/tests/cmdline/upgrade_tests.py
>
> Modified: subversion/trunk/subversion/tests/cmdline/upgrade_tests.py
> URL: 
> http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/upgrade_tests.py?rev=1899014=1899013=1899014=diff
> ==
> --- subversion/trunk/subversion/tests/cmdline/upgrade_tests.py (original)
> +++ subversion/trunk/subversion/tests/cmdline/upgrade_tests.py Fri Mar 18 
> 00:40:29 2022
> @@ -102,11 +102,21 @@ def replace_sbox_repo_with_tarfile(sbox,
>shutil.move(os.path.join(extract_dir, dir), sbox.repo_dir)
> 
>  def check_format(sbox, expected_format):
> +  assert isinstance(expected_format, int)
>formats = sbox.read_wc_formats()
>if formats[''] != expected_format:
>  raise svntest.Failure("found format '%d'; expected '%d'; in wc '%s'" %
>(formats[''], expected_format, sbox.wc_dir))
> 
> +def check_formats(sbox, expected_formats):
> +  assert isinstance(expected_formats, dict)
> +  formats = sbox.read_wc_formats()
> +  ### If we ever need better error messages here, reuse 
> run_and_verify_info().
> +  if formats != expected_formats:
> +raise svntest.Failure("found format '%s'; expected '%s'; in wc '%s'" %
> +  (formats, expected_formats, sbox.wc_dir))
> +
> +
>  def check_pristine(sbox, files):
>for file in files:
>  file_path = sbox.ospath(file)
> @@ -334,7 +344,18 @@ def upgrade_with_externals(sbox):
>   'upgrade', sbox.wc_dir)
> 
># Actually check the format number of the upgraded working copy
> -  check_format(sbox, get_current_format())
> +  check_formats(sbox,
> +  {relpath: get_current_format()
> +   for relpath in (
> + '',
> + 'A/D/exdir_A',
> + 'A/D/exdir_A/G',
> + 'A/D/exdir_A/H',
> + 'A/D/x',
> + 'A/C/exdir_G',
> + 'A/C/exdir_H',
> +   )})
> +
>check_pristine(sbox, ['iota', 'A/mu',
>  'A/D/x/lambda', 'A/D/x/E/alpha'])

Re: svn commit: r1898525 - in /subversion/trunk/subversion: include/svn_client.h libsvn_client/upgrade.c libsvn_wc/wc.h svn/help-cmd.c svn/info-cmd.c svn/svn.c

[Daniel Shahaf]

julianf...@apache.org wrote on Wed, Mar 02, 2022 at 12:24:40 -:
> +++ subversion/trunk/subversion/include/svn_client.h Wed Mar  2 12:24:40 2022
> @@ -4471,23 +4463,8 @@ typedef struct svn_client_wc_format_t {
>   * @since New in 1.15.
>   */
>  const svn_version_t *
> -svn_client__wc_version_from_format(int wc_format,
> -   apr_pool_t *result_pool,
> -   apr_pool_t *scratch_pool);
> +svn_client_wc_version_from_format(int wc_format,
> +  apr_pool_t *result_pool);

Thanks for renaming this.

Recommend to remove RESULT_POOL too, since it is unused and the return
type is a pointer-to-const.

Cheers,

Daniel

Re: svn commit: r1898389 - in /subversion/trunk/subversion: include/svn_client.h libsvn_client/upgrade.c svn/info-cmd.c svn/svn.c

[Daniel Shahaf]

julianf...@apache.org wrote on Thu, Feb 24, 2022 at 20:06:32 -:
> Multi-WC-format, issue #4884: add svn info --show-item=wc-compatible-version.
> 
> +++ subversion/trunk/subversion/include/svn_client.h Thu Feb 24 20:06:32 2022
> @@ -4451,6 +4451,14 @@ typedef struct svn_client_wc_format_t {
> +/** Return the version of the Subversion library that first supported
> + * the given WC format, @a wc_format.
> + */
> +const svn_version_t *
> +svn_client__wc_version_from_format(int wc_format,
> +   apr_pool_t *result_pool,
> +   apr_pool_t *scratch_pool);
> +
> +++ subversion/trunk/subversion/libsvn_client/upgrade.c Thu Feb 24 20:06:32 
> 2022
> @@ -200,6 +200,34 @@ svn_client_upgrade2(const char *path,
> +const svn_version_t *
> +svn_client__wc_version_from_format(int wc_format,
> +   apr_pool_t *result_pool,
> +   apr_pool_t *scratch_pool)
> +{
> +  static const svn_version_t
> +version_1_0  = { 1, 0, 0, NULL },
> +version_1_4  = { 1, 4, 0, NULL },
> +version_1_5  = { 1, 5, 0, NULL },
> +version_1_6  = { 1, 6, 0, NULL },
> +version_1_7  = { 1, 7, 0, NULL },
> +version_1_8  = { 1, 8, 0, NULL },
> +version_1_15 = { 1, 15, 0, NULL };
> +
> +  switch (wc_format)
> +{
> +  case  4: return _1_0;
> +  case  8: return _1_4;
> +  case  9: return _1_5;
> +  case 10: return _1_6;
> +  case SVN_WC__WC_NG_VERSION: return _1_7;

SVN_WC__WC_NG_VERSION is declared in wc.h, which this file isn't allowed
to use.

Why does this function special-case f12 over all other format numbers
only created by unreleased trunk revisions?  Why is f12 handled
differently to f6, f11, f13, f25, and f30?  It seems to me we should
either return NULL for all of them, or return the appropriate
svn_version_t value for each of them (and for all other integers between
11 and 28), as needed.

> +  case 29: return _1_7;
> +  case 31: return _1_8;
> +  case 32: return _1_15;
> +}
> +  return NULL;
> +}

> +++ subversion/trunk/subversion/svn/info-cmd.c Thu Feb 24 20:06:32 2022
> @@ -397,6 +399,8 @@ static const info_item_map_t info_item_m
>  { SVN__STATIC_STRING("depth"),   info_item_depth },
>  { SVN__STATIC_STRING("changelist"),  info_item_changelist },
>  { SVN__STATIC_STRING("wc-format"),   info_item_wc_format },
> +{ SVN__STATIC_STRING("wc-compatible-version"),
> + 
> info_item_wc_compatible_version },

The trailing comma is not C89-compatible and used to break the Windows
build.

>};

Cheers,

Daniel

Re: svn commit: r1898378 - in /subversion/trunk/subversion: include/ libsvn_client/ libsvn_wc/ svn/ tests/cmdline/getopt_tests_data/

[Daniel Shahaf]

julianf...@apache.org wrote on Thu, Feb 24, 2022 at 15:58:11 -:
> Multi-WC-format: Clarify the supported versions display.
> 
> This patch:
> 
>   - Changes the APIs for querying the default WC format and the supported WC
> formats;
>   - clarifies the display of supported WC formats in 'svn --version'.
> 
> API changes:
> 
>   - Remove svn_client_supported_wc_version().
>   - Add svn_client_default_wc_version().
>   - Add svn_client_supported_wc_formats() and a type it returns.
> 

Looks good.  Just a few comments:

> CLI changes:
> 
>   Old display in 'svn --version':
> 
>   | Supported working copy (WC) versions: from 1.8 to 1.15
> 
>   New display in 'svn --version':
> 
>   | Supported working copy (WC) versions:
>   |
>   | * compatible with Subversion v1.8 to v1.15 (WC format 31)
>   | * compatible with Subversion v1.15 (WC format 32)

Nice!

> The list style, along with inclusion of the WC format number, helps show
> that each line describes a distinct format. Users sometimes also need to
> know about WC format numbers, and the 'version' command is an appropriate
> place to show these. The presentation style matches that used for the lists
> of RA modules and credential caches.
> 
> * subversion/include/svn_client.h,
>   subversion/libsvn_client/upgrade.c
>   (svn_client_checkout4,
>svn_client_upgrade2): Update doc string.
>   (svn_client_supported_wc_version): Remove.
>   (svn_client_default_wc_version,
>svn_client_wc_format_t,
>svn_client_supported_wc_formats): New.
> 
> * subversion/libsvn_client/checkout.c
>   (svn_client_checkout4): Update caller: use svn_client_default_wc_version().
> 
> * subversion/libsvn_wc/wc.h
>   (SVN_WC__SUPPORTED_VERSION): Update doc string.
> 
> * subversion/svn/help-cmd.c
>   (print_supported_wc_formats): New.
>   (svn_cl__help): Use it to display supported WC formats as a list.
> 
> * subversion/svn/svn.c
>   (parse_compatible_version): Update caller: use
> svn_client_supported_wc_formats().
> 
> * subversion/tests/cmdline/getopt_tests_data/svn--version_stdout,
>   subversion/tests/cmdline/getopt_tests_data/svn--version--verbose_stdout
> Update expected output.

> +++ subversion/trunk/subversion/include/svn_client.h Thu Feb 24 15:58:10 2022
> @@ -4428,13 +4428,40 @@ svn_client_upgrade(const char *wcroot_di
> apr_pool_t *scratch_pool);
>  
>  /**
> - * Returns the version related to the earliest supported
> + * Returns the version related to the library's default
>   * working copy metadata format.
>   *
>   * @since New in 1.15.
>   */
>  const svn_version_t *
> -svn_client_supported_wc_version(void);
> +svn_client_default_wc_version(apr_pool_t *result_pool);
> +
> +/**
> + * Information about a WC version.
> + *
> + * Only the @c .major and @c .minor version fields are significant: so a
> + * version_max value of 1.15.0 for example means "up to 1.15.x".

Missing @since.

Missing warning not to manually allocate structs of this type since
fields may be added in the future.

> + */
> +typedef struct svn_client_wc_format_t {
> +/* Oldest version of svn libraries known to support this WC version */
> +const svn_version_t *version_min;
> +/* Newest version of svn libraries known to support this WC version. */
> +const svn_version_t *version_max;
> +/* The WC format number of this format, as defined by libsvn_wc. */
> +int wc_format;
> +} svn_client_wc_format_t;
> +
> +/**
> + * Returns a list of the WC formats supported by the client library.
> + *
> + * The list is sorted from oldest to newest, and terminated by an entry
> + * containing all null/zero fields.
> + *
> + * The returned data are allocated in @a result_pool and/or statically.

Missing @since.

> + */
> +const svn_client_wc_format_t *
> +svn_client_supported_wc_formats(apr_pool_t *result_pool,
> +apr_pool_t *scratch_pool);

> +++ subversion/trunk/subversion/libsvn_client/upgrade.c Thu Feb 24 15:58:10 
> 2022
> @@ -41,6 +41,7 @@
>  
>  #include "svn_private_config.h"
>  #include "private/svn_wc_private.h"
> +#include "../libsvn_wc/wc.h"

I don't think libsvn_client is allowed to use this header.

libsvn_foo/bar.h headers are for internal use by libsvn_foo, not for
interlibrary use; that would be include/private/.  We generally follow
this, too:

% grep -R 'include.*[.][.]/libsvn' subversion/lib* | grep -Eo '[<"].*[">]' | 
sort | uniq -c 
 14 "../../libsvn_fs/fs-loader.h"
  2 "../libsvn_delta/delta.h"
 53 "../libsvn_fs/fs-loader.h"
  1 "../libsvn_fs_base/fs_init.h"
  1 "../libsvn_fs_fs/fs_init.h"
  1 "../libsvn_fs_x/fs_init.h"
 24 "../libsvn_ra/ra_loader.h"
  3 "../libsvn_ra/wrapper_template.h"
% 

The RA/FS ones are presumably related to the pluggable design of those
layers.  (Sorry, I haven't got time to confirm this in detail.)

The delta.h one is in FSFS/FSX, which, as the comment there claims, use
SVN_DELTA_WINDOW_SIZE and nothing else declared or #define'd in that

Re: svn commit: r1897133 - /subversion/trunk/subversion/tests/cmdline/log_tests.py

[Daniel Shahaf]

Daniel Sahlberg wrote on Tue, 25 Jan 2022 13:29 +00:00:
> Oh. That is interesting, I never thought about running the checks on svn
> and dav. Is there a way to flag the test as "only xfails on local", and can
> someone help me do this? I'm a bit short on time right now.

r1897457.

Cheers,

Daniel

Re: svn commit: r1897133 - /subversion/trunk/subversion/tests/cmdline/log_tests.py

[Daniel Shahaf]

Julian Foad wrote on Tue, 25 Jan 2022 13:15 +00:00:
> On a trunk WC (updated today) I get:
>
>   File 
> "/home/julianfoad/src/subversion-d/subversion/tests/cmdline/svntest/main.py", 
> line 619, in 
> assert all(isinstance(arg, (str, unicode, int)) for arg in varargs)
> NameError: name 'unicode' is not defined
>
> This is on Ubuntu 21.10 with Python 3.9. Is the 'unicode' class name too
> old or too new for this Python?

Yes:

$ python2 -c 'print(type(""), type(b""), type(u""))' 
(, , )
$ python3 -c 'print(type(""), type(b""), type(u""))' 
  

So, I think changing the check to «isinstance(arg, (type(b""),
type(u""), int))» would work on both py2 and py3.  We can't just remove
«unicode» from there because that'd break svnrdump_tests under py2.

Incidentally, Python 2 has separate «int» and «long» types (e.g, 2**63
evaluates to a «long») whereas Python 3 has only «int», but that's easy
to handle one way or another.

Thanks for the confirmation elsethread.

Cheers,

Daniel

Re: svn commit: r1897133 - /subversion/trunk/subversion/tests/cmdline/log_tests.py

[Daniel Shahaf]

dsahlb...@apache.org wrote on Sun, 16 Jan 2022 19:19 +00:00:
> Add a test for issue #4856, "invalid xml file produced by: svn log --xml
> --use-merge-history".
⋮
> * subversion/tests/cmdline/log_tests.py
>   (log_xml_with_merge_history): New test.
>   (test_list): Run it.

The new test XFails for me over ra_local but XPasses over
svnserveautocheck and davautocheck.

As we have no buildbots[1], could someone confirm?

Cheers,

Daniel


[1] See the "Buildbot migration" thread and INFRA-22761; current bottom
line: waiting for Infra or someone with an apache.org account to
edit and enable the buildbot configuration.

Re: svn commit: r1891998 - /subversion/branches/1.14.x/STATUS

[Daniel Shahaf]

Daniel Sahlberg wrote on Tue, Aug 03, 2021 at 23:44:53 +0200:
> Den tis 3 aug. 2021 kl 23:40 skrev :
> 
> > Author: dsahlberg
> > Date: Tue Aug  3 21:40:40 2021
> > New Revision: 1891998
> >
> > URL: http://svn.apache.org/viewvc?rev=1891998=rev
> > Log:
> > * STATUS: Upgrading my vote for r1891908 (now being a full committer).
> >
> > Modified:
> > subversion/branches/1.14.x/STATUS
> >
> > Modified: subversion/branches/1.14.x/STATUS
> > URL:
> > http://svn.apache.org/viewvc/subversion/branches/1.14.x/STATUS?rev=1891998=1891997=1891998=diff
> >
> > ==
> > --- subversion/branches/1.14.x/STATUS (original)
> > +++ subversion/branches/1.14.x/STATUS Tue Aug  3 21:40:40 2021
> > @@ -96,8 +96,7 @@ Candidate changes:
> > Justification:
> >   Small fix.  Prevents spurious hard fails of 'make davautocheck'.
> > Votes:
> > - +1: danielsh
> > - +1 (non-binding): dsahlberg
> > + +1: danielsh, dsahlberg
> >
> >  Veto-blocked changes:
> >  =
> >
> 
>  @Daniel Shahaf  Should this be backported also to
> 1.10.x? It exhibits the same problem and the same fix applies there. If so,
> I can copy over the same backporting proposal.

Yes.  As you say, mod_dontdothat is a hard dependency of the test suite
in 1.10.x too, so adding the dependency in Makefile shouldn't break
anything, even in an LTS branch.  I've taken your email as a +1 to
backport and added the nomination with both of our votes.

Thanks,

Daniel

Re: svn commit: r1891570 - /subversion/site/staging/style/site.css

[Daniel Shahaf]

dsahlb...@apache.org wrote on Thu, 15 Jul 2021 11:25 +00:00:
> Found-by: hartmannathan

For future reference, the syntax is "Found by", with a space as opposed
to a hyphen/minus.  contribulyze.py relies on that space (see «field_re»),
and we rely on contribulyze.py to track contributions by people
other than full committers.

Cheers,

Daniel

Re: svn commit: r1891237 - in /subversion/site/staging/docs: ./ api/ api/1.13/ api/1.13/search/ api/1.14/ api/1.14/search/ javahl/ javahl/1.13/ javahl/1.13/org/ javahl/1.13/org/apache/ javahl/1.13/org

[Daniel Shahaf]

Daniel Sahlberg wrote on Thu, 08 Jul 2021 21:12 +00:00:
> I have regenerated the API docs and checked for differences.

Thank you.

Daniel

Re: svn commit: r1891237 - in /subversion/site/staging/docs: ./ api/ api/1.13/ api/1.13/search/ api/1.14/ api/1.14/search/ javahl/ javahl/1.13/ javahl/1.13/org/ javahl/1.13/org/apache/ javahl/1.13/org

[Daniel Shahaf]

Daniel Sahlberg wrote on Sat, Jul 03, 2021 at 23:45:01 +0200:
> It seems that the API and JavaHL docs for 1.13 and 1.14 are missing. I have
> generated them from the branches.

Are there any relevant differences between the branches (@HEAD) and the
latest tags on each branch?

Re: svn commit: r1890239 - in /subversion/site/staging: docs/community-guide/general.part.html faq.html

[Daniel Shahaf]

s...@apache.org wrote on Thu, 27 May 2021 07:12 +00:00:
> Add links to libera.chat's new web interface.
> 
> +++ subversion/site/staging/docs/community-guide/general.part.html Thu May 27 
> 07:12:43 2021
> +or via the https://web.libera.chat/?channel=svn-dev;
> +>libera.chat IRC webchat interface.
>  
> +++ subversion/site/staging/faq.html Thu May 27 07:12:43 2021
> +  the https://web.libera.chat/?channel=#svn;>web interface 
> or

One of the new links has a «#» and one doesn't.  Double-checking that they're 
both correct?

Cheers,

Daniel

Re: svn commit: r1889555 - /subversion/branches/1.14.x/STATUS

[Daniel Shahaf]

Yasuhito FUTATSUKI wrote on Fri, 07 May 2021 06:27 +00:00:
> On 2021/05/07 2:24, Daniel Shahaf wrote:
> > futat...@apache.org wrote on Thu, 06 May 2021 06:52 +00:00:
> >> @@ -79,7 +79,7 @@ Candidate changes:
> >>   * r1889487
> >> swig-py: Fix doubly destroying memory pool with cyclic garbage 
> >> collector.
> >> Votes:
> >> - +1: jun66j5
> >> + +1: jun66j5, futatuki
> > 
> > Bindings changes require only two votes, so you can move this to the 
> > Approved section.
> > 
> > https://subversion.apache.org/docs/community-guide/releasing.html#release-stabilization-how-many-votes
> 
> Thank you for the guidance. I've done it. 

You're welcome.  svn-role should merge it overnight UTC (around
lunchtime in your timezone, judging by your Date: header).

Cheers,

Daniel

Re: svn commit: r1889555 - /subversion/branches/1.14.x/STATUS

[Daniel Shahaf]

futat...@apache.org wrote on Thu, 06 May 2021 06:52 +00:00:
> @@ -79,7 +79,7 @@ Candidate changes:
>   * r1889487
> swig-py: Fix doubly destroying memory pool with cyclic garbage collector.
> Votes:
> - +1: jun66j5
> + +1: jun66j5, futatuki

Bindings changes require only two votes, so you can move this to the Approved 
section.

https://subversion.apache.org/docs/community-guide/releasing.html#release-stabilization-how-many-votes

Cheers,

Daniel

Re: svn commit: r1888589 - in /subversion/trunk: build.conf subversion/tests/libsvn_subr/task-test.c

[Daniel Shahaf]

stef...@apache.org wrote on Sat, Apr 10, 2021 at 15:35:23 -:
> +++ subversion/trunk/subversion/tests/libsvn_subr/task-test.c Sat Apr 10 
> 15:35:23 2021
> @@ -0,0 +1,253 @@
> +static svn_error_t *
> +counter_func(void **result,
⋮
> + apr_pool_t *scratch_pool)
> +{
> +  apr_int64_t value = *(apr_int64_t*)process_baton;
> +
> +  apr_pool_t *sub_task_pool;
> +  apr_int64_t *partial_result;
> +  apr_int64_t *partial_baton;
> +
> +  if (value > 1)
> +{
> +  partial_result = apr_palloc(result_pool, sizeof(partial_result));

s/sizeof(foo)/sizeof(*foo)/, here and later in the function.

I'm surprised that no one caught this in post-commit reviews.  In fact,
I wonder whether people have reviewed this commit and missed the bug, or
didn't review this commit at all — the latter being a silent failure
mode of the commit-then-review paradigm.  (More on this under separate
cover — currently on private@, but may move here later.)

> +  *partial_result = 1;
> +  value -= *partial_result;
> +
> +  sub_task_pool = svn_task__create_process_pool(task);
> +
> +  partial_baton = apr_palloc(sub_task_pool, sizeof(partial_baton));  
> +  *partial_baton = MAX(1, value / 2);
> +  value -= *partial_baton;
> +
> +  SVN_ERR(svn_task__add_similar(task, sub_task_pool, 
> +partial_result, partial_baton));
> +}
> +
> +  if (cancel_func)
> +SVN_ERR(cancel_func(cancel_baton));
> +
> +  if (value > 1)
> +{
> +  partial_result = apr_palloc(result_pool, sizeof(partial_result));
> +  *partial_result = 1;
> +  value -= *partial_result;
> +

I'm not sure I follow what's happening here, and there aren't any
comment breadcrumbs either.

Why is cancel_func called between the two blocks rather than once at the
start (or end), which is the more common pattern?

Why is «partial_result» allocated and initialized in both blocks, rather
than just once?  Why is «value» decremented by 1 in both blocks, rather
than just once?  Which line of code is responsible for doing this
recursive call's work?  test_counting() passes even if I change the
RHS's of both assignments to «*partial_result» to random values.
(test_cancellation() does fail in that case.)

Why is «value» decremented (below) by an integer expression that's equal
to «value - 1», rather than simply being assigned «value = 1»?

Please reduce variables' scope to block scope where possible.

> +  sub_task_pool = svn_task__create_process_pool(task);
> +
> +  partial_baton = apr_palloc(sub_task_pool, sizeof(partial_baton));
> +  *partial_baton = value - 1;
> +  value -= *partial_baton;
> +  SVN_ERR(svn_task__add_similar(task, sub_task_pool,
> +partial_result, partial_baton));
> +}
> +
> +  partial_result = apr_palloc(result_pool, sizeof(partial_result));
> +  *partial_result = value;
> +  *result = partial_result;
> +
> +  return SVN_NO_ERROR;
> +}
> +
> +static svn_error_t *
> +sum_func(svn_task__t *task,
⋮
> +{
> +  apr_int64_t *result_p = result;
> +  apr_int64_t *output_p = output_baton;
> +  
> +  if (result_p)

What's the purpose of this check?  It should never be false.  Should it
be removed?  A segfault is less likely to result in a false negative PASS.

> +*output_p += *result_p;
⋮
> +}
⋮
> +static svn_error_t *
> +test_cancellation(apr_pool_t *pool)
> +{
> +  apr_int64_t start = 100;
⋮
> +  result = 0;
> +  SVN_TEST_ASSERT_ERROR(svn_task__run(8, counter_func, , sum_func, 
> ,
> +  NULL, NULL, cancel_at_10k, ,
> +  pool, pool),
> +SVN_ERR_CANCELLED);
> +  SVN_TEST_ASSERT(result == 1);

Does the API actually guarantee that «result» will be equal to 1?

The docs say:

 * Errors are detected in strictly in post-order, with only the first one
 * being returned from the task runner.  In particular, it may not be the
 * first error that occurs timewise but only the first one encountered when
 * walking the tree and checking the processing results for errors.  Because
 * it takes some time to make the workers cancel all outstanding tasks,
 * additional errors may occur before and after the one that ultimately
 * gets reported.  All those errors are being swallowed.

> +SVN_TEST_MAIN

By the way, the docstring of svn_task__add() says "the current tasks
output function".  That's missing an apostrophe, isn't it?

Cheers,

Daniel

Review of task.c as of svn commit: r1888520 - in /subversion/trunk: build.conf subversion/libsvn_subr/task.c

[Daniel Shahaf]

Good morning Stefan,

stef...@apache.org wrote on Thu, Apr 08, 2021 at 14:35:52 -:
> Initial, single-theaded implementation of the svn_task__t API.

I've committed some minor fixes in r1888533.  Most of them are
straightforward but please double check that all the apostrophes are
where they should be.

Here's also a review of task.c as it stands now.

Index: subversion/include/private/svn_task.h
===
--- subversion/include/private/svn_task.h   (revision 1888527)
+++ subversion/include/private/svn_task.h   (working copy)
@@ -117,6 +117,9 @@ typedef svn_error_t *(*svn_task__process_func_t)(
  * All data that shall be returned by @a svn_task__run() needs to be
  * allocated from @a result_pool.
  *
+ * @note The lifetime of @a result is not guaranteed. If it is to be returned
+ * from svn_task__run(), it needs to be copied to @a result_pool.
+ *
  * @a cancel_func, @a cancel_baton and @a scratch_pool are the usual things.
  */
 typedef svn_error_t *(*svn_task__output_func_t)(
Index: subversion/libsvn_subr/task.c
===
--- subversion/libsvn_subr/task.c   (revision 1888533)
+++ subversion/libsvn_subr/task.c   (working copy)
@@ -139,7 +139,8 @@ struct svn_task__t
 
   /* Parent task.
* NULL for the root node:
-   * (PARENT==NULL) == (*this==ROOT->TASK).
+   * (PARENT==NULL) == (this==ROOT->TASK).
+   * ### assert() that?
*/
   svn_task__t *parent;
 
@@ -177,6 +178,7 @@ struct svn_task__t
   /* Task state. */
 
   /* The callbacks to use. Never NULL. */
+  /* ### assert() that? */
   callbacks_t *callbacks;
 
   /* Process baton to pass into CALLBACKS->PROCESS_FUNC. */
@@ -186,7 +188,7 @@ struct svn_task__t
* Sub-pool of ROOT->PROCESS_POOL.
*
* NULL, iff processing of this task has completed (sub-tasks may still
-   * need processing).  Used to check whether processing has been completed.
+   * need processing).
*/
   apr_pool_t *process_pool;
 
@@ -203,12 +205,13 @@ struct svn_task__t
 
 /* Return the index of the first immediate sub-task of TASK with a ready
  * sub-task in its respective sub-tree.  TASK must have at least one such
- * sub-task.
+ * proper sub-task.
  */
 static apr_size_t first_ready_sub_task_idx(const svn_task__t *task)
 {
   svn_task__t *sub_task = task->first_ready;
   assert(sub_task);
+  assert(sub_task != task);
 
   while (sub_task->parent != task)
 sub_task = sub_task->parent;
@@ -391,6 +394,8 @@ static svn_error_t *remove_task(svn_task__t *task)
  * mainly to prevent error leaks when terminating the task runner early.
  * It is better to have the standard pool cleanups to deal with memory
  * management in that case.
+ *
+ * ### Also resets FIRST_SUB
  */
 static void free_sub_tasks(svn_task__t *task)
 {
@@ -503,6 +508,7 @@ static void process(svn_task__t *task,
 {
   /* Depending on whether there is prior parent output, we may or 
* may not have already an OUTPUT structure allocated for TASK. */
+  /* ### Aside: The names could be clearer.  As it is, output == 
task->output != output->output */
   output_t *output = ensure_output(task);
   output->error = callbacks->process_func(>output, task,
   thread_context,
@@ -555,7 +561,12 @@ static svn_error_t *output_processed(
   /* Post-order, i.e. dive into sub-tasks first.
*
* Note that the "background" processing for CURRENT has completed
-   * and only the output function may add no further sub-tasks. */
+   * and the "foreground" output function may add further sub-tasks. */
+  /* ### The 'if' branch processes 
current->first_sub->output->prior_parent_output —
+   * ### that's output produced by current — in this iteration, and only in
+   * ### the next iteration will handle current->first_sub's subtree's
+   * ### outputs.  That's not post-order, is it?
+   */
   if (current->first_sub)
 {
   current = current->first_sub;

HTH,

Daniel

Re: svn commit: r1886396 - in /subversion/site/publish: ./ doap.rdf docs/release-notes/release-history.html download.html

[Daniel Shahaf]

Daniel Sahlberg wrote on Wed, Feb 17, 2021 at 09:46:54 +0100:
> Den tis 16 feb. 2021 kl 21:37 skrev Daniel Shahaf :
> > Is it worthwhile to automate this step?  doap.rdf changes rarely enough
> > that we needn't bother with "edit part of a file" logic; we can just
> > regenerate the entire file and «svnmucc put» it into place, with a
> > comment indicating it's a generated file.
> 
> The doap.rdf contain references to two separate releases (at least
> right now) and when running release.py you are working on one release
> at a time. So we can't just have a template and add the current
> release number, we also need to know the other release (which could
> have been a year ago or the same day).

Well, yes, and «release.py clean-dist» already has logic to determine
the other release's version number.

> To automate "edit part of file", we would need to search for the same
> major.minor and replace with current relase, but when there is a new
> minor (1.15..) we would have to edit manually anyway.

I don't think so.

We could generate subversion-%(version)s.rdf-excerpt files, drop them in
dist/, and then use clean_dist()-like logic to cat the right subset of
them, adding a fixed header and trailer.  This way, we wouldn't need to
splice lines out of and into the file, and we wouldn't need to special-case
the first release of a minor line or the EOLing of a minor line in the
logic.

> It's a balance between the amount of work done by RM, the downside of
> having different processes for new minor and new patch release and the
> work to code to automate. I'm leaning towards having it as it is, but
> I would listen to Stefan's opinion (since he did the most recent RM).

By and large, agreed, but see above for the details.

Cheers,

Daniel

Re: svn commit: r1886396 - in /subversion/site/publish: ./ doap.rdf docs/release-notes/release-history.html download.html

[Daniel Shahaf]

Daniel Sahlberg wrote on Tue, 16 Feb 2021 13:05 +00:00:
> Den tis 16 feb. 2021 kl 13:15 skrev Stefan Sperling :
> > On Tue, Feb 16, 2021 at 01:05:32PM +0100, Daniel Sahlberg wrote:
> > > What do you think about this?
> > > [[[
> > > List the new release on ^/subversion/site/publish/doap.rdf
> > > There should be a  section for each supported minor release
> > > with the  and  being updated to the current release
> > > date and patch release number.
> > > Do not change anything else in the file (in particular the 
> > > under  is the date when the Subversion project was created).
> > > ]]]
> >
> > That is crystal clear and should avoid mistakes going forward. Thank you!
> 
> r1886589

Is it worthwhile to automate this step?  doap.rdf changes rarely enough
that we needn't bother with "edit part of a file" logic; we can just
regenerate the entire file and «svnmucc put» it into place, with a
comment indicating it's a generated file.

Re: svn commit: r1886494 - /subversion/site/staging/docs/community-guide/releasing.part.html

[Daniel Shahaf]

dsahlb...@apache.org wrote on Sat, Feb 13, 2021 at 21:41:10 -:
> +++ subversion/site/staging/docs/community-guide/releasing.part.html Sat Feb 
> 13 21:41:10 2021
> @@ -1255,80 +1255,24 @@ href="https://reporter.apache.org/addrel
>  
>   
>  
> -

Here, you moved some 70 lines and also made a change between the pre-move and
post-move form.  First, here's the delta for ease of review:

[[[
-NOTE: We announce the release before updating the website since the website
-update links to the release announcement sent to the announce@ mailing 
list.
+NOTE: We update the website before announce the release to make sure any
+links in the release announcement are valid. After announcing the release,
+links to the release announcement e-mail are added to the website.
]]]

On the first added line, "before announce the release" is ungrammatical.

(Also, with some archives it's possible to generate the links in advance; for
example, this message's permalink is
.)

>  
>  Update the website
> -->#releasing-update-website"
>  title="Link to this section">
>  
>  
> +Even though the steps below indicate to update the published website 
> +directly, you may prepare the changes on ^/subversion/site/staging.
> +In that case:
> +
> +  Do a catch-up merge from 
> ^/subversion/site/publish.
> +  Commit any changes to ^/subversion/site/staging and
> +check the results on https://subversion-staging.apache.org;
> +>https://subversion-staging.apache.org.
> +  When ready to publish, merge the changes back to 
> +^/subversion/site/publish.

Suggest to remind here to review the merge results in case there are other
changes on staging at the time «svn merge» is run.

> +

> @@ -1344,9 +1288,9 @@ the oldest supported LTS branch's ST
>  ^/subversion/site/publish/index.html, also removing the
>  oldest News item from that page.  Use release.py write-news to
>  generate a template news item, which should then be customized.
> -At least fill in the URL to the archived announcement email, and check
> -that the date is correct if you generated the template in advance of the
> -release date.
> +For now, comment out the link to the release announcement e-mail.
> +Check that the date is correct if you generated the template in advance 
> of 
> +the release date.

Sounds like we should make write-news generate the HTML comment marker in
advance, and only ask the RM to remove them.  (The RM has a fair amount of work
as it is; every little bit helps.)

Cheers,

Daniel

P.S.  While reviewing this I noticed that
https://subversion-staging.apache.org/favicon.ico is different to
https://svn.apache.org/repos/asf/subversion/site/staging/favicon.ico: an Apache
feather v. a Subversion logo.

Re: svn commit: r1886460 - /subversion/trunk/subversion/tests/cmdline/mod_authz_svn_tests.py

[Daniel Shahaf]

s...@apache.org wrote on Fri, Feb 12, 2021 at 10:40:16 -:
> Author: stsp
> Date: Fri Feb 12 10:40:16 2021
> New Revision: 1886460
> 
> URL: http://svn.apache.org/viewvc?rev=1886460=rev
> Log:
> Add a test for the NULL deref issue also known as CVE-2020-17525.
> 
> * subversion/tests/cmdline/mod_authz_svn_tests.py
>   (nonexistent_repos_relative_access_file): New test.

Propose this for backport?

> +++ subversion/trunk/subversion/tests/cmdline/mod_authz_svn_tests.py Fri Feb 
> 12 10:40:16 2021
> @@ -1072,6 +1072,43 @@ def repos_relative_access_file(sbox):
>  
>verify_gets(test_area_url, in_repos_authz_tests)
>  
> +# test for the bug also known as CVS-2020-17525

s/S/E/

> +@SkipUnless(svntest.main.is_ra_type_dav)
> +def nonexistent_repos_relative_access_file(sbox):
> +  "repos-relative access file with bad repository URL"

Re: svn commit: r1886396 - in /subversion/site/publish: ./ doap.rdf docs/release-notes/release-history.html download.html

[Daniel Shahaf]

s...@apache.org wrote on Wed, Feb 10, 2021 at 20:39:23 -:
> Author: stsp
> Date: Wed Feb 10 20:39:22 2021
> New Revision: 1886396
> 
> URL: http://svn.apache.org/viewvc?rev=1886396=rev
> Log:
> site/publish: Merge from staging area.

For future reference, this commit should have used the "less than 24 hours ago" 
syntax:

% cd site/publish
% grep -R -h -9  | vipe
  
% 

More below.

> +++ subversion/site/publish/doap.rdf Wed Feb 10 20:39:22 2021
> @@ -22,7 +22,7 @@
>  limitations under the License.
>  -->
>http://subversion.apache.org/;>
> -2010-12-28
> +2021-02-10

Huh?

Quoting http://usefulinc.com/ns/doap:

> > http://usefulinc.com/ns/doap#created;>
> > http://usefulinc.com/ns/doap#; />
> > created
> > Date when something was created, in 
> > -MM-DD form. e.g. 2004-04-05
> > ⋮
> > 

The entity referred to by the  tag wasn't created in 2021.  So,
I think the hunk is incorrect… but so was the original value, which referred to
the _file_'s creation date (r1053461), rather than to the date Subversion was
founded (2000), the date it was accepted into the Incubator, or the date it was
promoted to TLP.

Thanks for RMing,

Daniel

Re: svn commit: r1885120 - /subversion/site/publish/contributing.html

[Daniel Shahaf]

Much better, thanks!

(And remember you've got now one more family than you did last year ☺)

Daniel Sahlberg wrote on Thu, 07 Jan 2021 17:54 +00:00:
> Dear Daniel,
> 
> I've updated the commit message, hope it is better now. Thanks for your 
> feedback!
> 
> /Daniel
> 
> (No worries about the wait, my wife keeps reminding me I've got a 
> family to hang out with)
> 
> Den ons 6 jan. 2021 kl 16:45 skrev Daniel Shahaf :
> > Good morning Daniel,
> > 
> > dsahlb...@apache.org wrote on Mon, 04 Jan 2021 18:17 +00:00:
> > > Author: dsahlberg
> > > Date: Mon Jan  4 18:17:01 2021
> > > New Revision: 1885120
> > > 
> > > URL: http://svn.apache.org/viewvc?rev=1885120=rev
> > > Log:
> > > * publish/contributing.html:
> > >   (#code/Become a committer): Remove links to Fitz' articles altogether
> > 
> > Log messages should answer "Why?", not "What?" (the unidiff itself does
> > that), and this one doesn't.  Could you extend it, please?  See r1884997
> > for an example.
> > 
> > A pointer to the dev@ thread would be fine, too.  The pointer should be
> > long-lived; generally that means it should spell out the message-id.  (As
> > to me, usually I either copy the Date/From/To/Cc/Subject/Message-ID
> > headers in this order, or give a mail-archives.a.o link, which
> > identifies the List-ID and Message-ID.  My email client is set up so
> > I can generate either kind of pointer easily.)
> > 
> > Cheers,
> > 
> > Daniel
> > 
> > P.S.  We still owe you an answer on your other thread, I know.  Sorry
> >   for the delay.  I've pinged the private@ thread for you.
> > 
> > > Modified:
> > > subversion/site/publish/contributing.html
> > > 
> > > Modified: subversion/site/publish/contributing.html
> > > URL: 
> > > http://svn.apache.org/viewvc/subversion/site/publish/contributing.html?rev=1885120=1885119=1885120=diff
> > > ==
> > > --- subversion/site/publish/contributing.html (original)
> > > +++ subversion/site/publish/contributing.html Mon Jan  4 18:17:01 2021
> > > @@ -222,11 +222,7 @@
> > > beneficial to the developer community  where quality
> > > developers are concerned, "the more, the merrier"!  But never
> > > underestimate how valuable this experience can be to you
> > > -   personally
> > > -   and  > > href="https://web.archive.org/web/20050803084234/http://onlamp.com/pub/a/onlamp/2005/07/14/osdevelopers.html;
> > > -   >professionally,
> > > -> > href="https://web.archive.org/web/20051214172122/http://onlamp.com/pub/a/onlamp/2005/08/01/opensourcedevelopers.html;
> > > -   >too.
> > > +   personally and professionally.
> > >  
> > >  
> > >  
> > > 
> > > 
> > >

Re: svn commit: r1885120 - /subversion/site/publish/contributing.html

[Daniel Shahaf]

Good morning Daniel,

dsahlb...@apache.org wrote on Mon, 04 Jan 2021 18:17 +00:00:
> Author: dsahlberg
> Date: Mon Jan  4 18:17:01 2021
> New Revision: 1885120
> 
> URL: http://svn.apache.org/viewvc?rev=1885120=rev
> Log:
> * publish/contributing.html:
>   (#code/Become a committer): Remove links to Fitz' articles altogether

Log messages should answer "Why?", not "What?" (the unidiff itself does
that), and this one doesn't.  Could you extend it, please?  See r1884997
for an example.

A pointer to the dev@ thread would be fine, too.  The pointer should be
long-lived; generally that means it should spell out the message-id.  (As
to me, usually I either copy the Date/From/To/Cc/Subject/Message-ID
headers in this order, or give a mail-archives.a.o link, which
identifies the List-ID and Message-ID.  My email client is set up so
I can generate either kind of pointer easily.)

Cheers,

Daniel

P.S.  We still owe you an answer on your other thread, I know.  Sorry
  for the delay.  I've pinged the private@ thread for you.

> Modified:
> subversion/site/publish/contributing.html
> 
> Modified: subversion/site/publish/contributing.html
> URL: 
> http://svn.apache.org/viewvc/subversion/site/publish/contributing.html?rev=1885120=1885119=1885120=diff
> ==
> --- subversion/site/publish/contributing.html (original)
> +++ subversion/site/publish/contributing.html Mon Jan  4 18:17:01 2021
> @@ -222,11 +222,7 @@
> beneficial to the developer community  where quality
> developers are concerned, "the more, the merrier"!  But never
> underestimate how valuable this experience can be to you
> -   personally
> -   and  href="https://web.archive.org/web/20050803084234/http://onlamp.com/pub/a/onlamp/2005/07/14/osdevelopers.html;
> -   >professionally,
> -href="https://web.archive.org/web/20051214172122/http://onlamp.com/pub/a/onlamp/2005/08/01/opensourcedevelopers.html;
> -   >too.
> +   personally and professionally.
>  
>  
>  
> 
> 
>

mailer.py py2/py3 change: non-UTF-8 environments (was: svn commit: r1884427 - in /subversion/trunk/tools/hook-scripts/mailer: mailer.py tests/mailer-t1.output tests/mailer-tweak.py)

[Daniel Shahaf]

s...@apache.org wrote on Mon, 14 Dec 2020 16:57 -:
> URL: http://svn.apache.org/viewvc?rev=1884427=rev
> Log:
> Make mailer.py work properly with Python 3, and drop Python 2 support.
> 
> Most of the changes deal with the handling binary data vs Python strings.
> 
> I've made sure that mailer.py will work in a UTF-8 environment. In general,
> UTF-8 is recommended for hook scripts. See the SVNUseUTF8 mod_dav_svn option.
> Environments using other encodings may not work as expected, but those will
> be problematic for hook scripts in general.

Correct me if I'm wrong, but it sounds like you haven't ruled out the
possibility that this commit will constitute a regression for anyone
who runs mailer.py in a non-UTF-8 environment and will upgrade to this
commit.

I suppose it's fair to classify non-UTF-8 environments as "patches
welcome", following the precedent of libmagic support in the Windows
build, but:

1. Can we detect non-UTF-8 environments and warn or error out hard upon
them?  «locale.getlocale()[1]» seems promising?

2. A change that hasn't been confirmed *not* to constitute a regression
merits a release notes entry.  Would you do the honours?

Cheers,

Daniel

>SVN repositories store internal
> data such as paths in UTF-8. Our Python3 bindings do not deal with encoding
> or decoding of such data, and thus need to work with raw UTF-8 strings, not
> Python strings.
> 
> The encoding of file and property contents is not guaranteed to be UTF-8.
> This was already a problem before this change. This hook script sends email
> with a content type header specifying the UTF-8 encoding. Diffs which contain
> non-UTF-8 text will most likely not render properly when viewed in an email
> reader. At least this problem is now obvious in mailer.py's implementation,
> since all unidiff text is now written out directly as binary data.
> 
> As an additional fix, iterate file groups in sorted order. This results in
> stable output and makes test cases in our tests/ subdirectory reproducible.
> 
> Tested with Python 3.7.5 which is the version I use in my SVN development
> setup at present. Tests with newer versions are welcome.
> 
> * tools/hook-scripts/mailer/mailer.py:
>   Drop Python2-specific includes. Adjust includes as per 2to3.
>   (main): Decode arguments from UTF-8 to string.
>   (OutputBase:write): Encode string to UTF-8 and pass to write_binary().
>OutputBase implementations now need to provide a self.write_binary
>member which implements a write() method for binary data.
>   (MailedOutput): email.Header package is gone, use email.header instead,
>and likewise replace use of email.Utils with email.utils
>   (SMTPOutput): Provide self.write_binary in terms of a BytesIO() object.
>We cannot use StringIO since diffs may contain data in arbitrary encodings.
>   (StandardOutput): Provide self.write_binary in terms of stdout.buffer.
>   (PipeOutput): Provide self.write_binary in terms of pipe.stdin.
>   (Commit): Decode log message and paths from UTF-8 to string, and iterate
> path groups from mailer.conf in sorted order.
>   (Lock): Decode directory entries from UTF-8 to string. Encode paths back
>to UTF-8 when we ask libsvn_fs for a lock on a path.
> Iterate path groups from mailer.conf in sorted order.
>   (DiffGenerator): Decode repository paths from UTF-8 to string.
>   (TextCommitRenderer): Decode author, log message, and path from UTF-8 to
> string. Write diff data via write_binary, bypassing the re-encoding step.
>   (Config): Decode paths from UTF-8 to string before matching them against
>regular expressions. Also decode the repository directory path from UTF-8.
> 
> * tools/hook-scripts/mailer/tests/mailer-t1.output: Adjust expected output.
>File groups are now provided in stable sorted order. This should fix
>spurious test failures in the future.
> 
>  * tools/hook-scripts/mailer/tests/mailer-tweak.py: Drop L suffix from long
> integers and pass binary data instead of strings into libsvn_fs.

Re: svn commit: r1884636 - /subversion/trunk/subversion/tests/cmdline/merge_reintegrate_tests.py

[Daniel Shahaf]

hartmannat...@apache.org wrote on Sun, 20 Dec 2020 03:25 -:
> Patch by: Tim Gates (timgates42) via GitHub PR#23

I don't think contribulyzer would DTRT on the "via GitHub PR#23" part.
Recommend to move it to a next-line parenthetical.

Re: svn commit: r1884279 - in /subversion/trunk: build/ subversion/tests/cmdline/svntest/

[Daniel Shahaf]

futat...@apache.org wrote on Thu, 10 Dec 2020 14:45 +00:00:
> Ensure close file descriptors by using context manager in test suite.

Suggest instead:

test suite: Ensure file descriptors are closed in a timely manner, using 
context managers.

I.e.:

- Use "$area: $description" format
- Fix ungrammatical use of "close"
  (alternative: "Ensure timely closure of file descriptors")

And since I'm reviewing already, a few more nits:

> Generally, it is true that as file descriptors are closed when their
> underlying objects are deleted, we don't need close them explicitly.
> However if a Python language implementation that uses other strategy
> than reference count model, there is no warranty it will happen

"if a Python language implementation uses a strategy other than reference 
counting"

> immediately after when those objects lose the last reference.  So we
> use context manager to ensure close() is called immediately after
> those objects to be unnecessary.  This helps us to run 'make check'
> with PyPy even smaler limit of number of open files, although we

s/even/under an even/

s/smaler/lower/ (the adjective modifies "limit", not "number")

Consider dropping the symbolic name (RLIMIT_NOFILE) into that sentence 
somewhere.

> don't recommend use it because it is much slower than CPython for

s/use it/use of it|to use it/

s/it/${whatever the pronoun refers to}/

> this purpose.

No comments on the diff itself, but I only skimmed it.

Cheers,

Daniel

Re: svn commit: r1882263 - /subversion/site/publish/upcoming.part.html

[Daniel Shahaf]

Since r1882293 effectively reverted this, I'm guessing there was some
transient error.  However, ENOTIME to look into this, particularly as
I didn't get a copy of stderr.

The relevant cron job is:

# Puppet Name: Update our upcoming changes list
SVN=svn
15 4 * * * chronic ~/src/svn/site/tools/generate-upcoming-changes-log.sh

The script is in site/tools/ in the repository.

Cheers,

Daniel


svn-r...@apache.org wrote on Tue, 06 Oct 2020 04:15 +00:00:
> Author: svn-role
> Date: Tue Oct  6 04:15:36 2020
> New Revision: 1882263
> 
> URL: http://svn.apache.org/viewvc?rev=1882263=rev
> Log:
> * upcoming.part.html: Automatically regenerated
> 
> Modified:
> subversion/site/publish/upcoming.part.html
> 
> Modified: subversion/site/publish/upcoming.part.html
> URL: 
> http://svn.apache.org/viewvc/subversion/site/publish/upcoming.part.html?rev=1882263=1882262=1882263=diff
> ==
> --- subversion/site/publish/upcoming.part.html (original)
> +++ subversion/site/publish/upcoming.part.html Tue Oct  6 04:15:36 2020
> @@ -1,179 +1,5 @@
>  
>  
> -Changes in ^/subversion/branches/1.14.x:
> -
> -https://svn.apache.org/r1877978;>r1877978 | svn-role | 
> 2020-05-21 04:00:13 + (Thu, 21 May 2020) | 7 lines
⋮
> -
>  
>  
>  Further changes currently under consideration are listed in each 
> release line's 
> 
> 
>

Re: svn commit: r1882186 - in /subversion/trunk/subversion: libsvn_repos/authz.c tests/cmdline/svnauthz_tests.py tests/libsvn_repos/authz-test.c tests/libsvn_repos/repos-test.c

[Daniel Shahaf]

s...@apache.org wrote on Thu, 01 Oct 2020 19:36 +00:00:
> Fix issue #4762 "authz doesn't combine global and repository rules"
> 
> The new authz implementation of SVN 1.10 introduced an incompatible change
> in the interpretation of authz rules: Global rules access were not
> considered if per-repository access rules were also supplied.
> 
> This change seems entirely unnecessary

I'm wary of this justification.  The semantics being changed here were
covered by tests and, according to your very log message, were likely
implemented deliberately.  Reverting such semantics requires better
arguments than "they seem unnecessary".  That argument is exactly how
the Debian OpenSSL bug was introduced.

I think we should more seriously investigate the possibility that _this_
change, r1882186, will break some use-case or another.

(To be clear, I'm disagreeing only with the _justification_ for the
change, not with the change itself.  I'm not commenting on the
change itself.)

> and is still causing problems today
> for deployments which upgrade from earlier versions, such as from SVN 1.8.
> Existing authz files no longer produce expected results and adjusting
> large existing authz rule files to avoid this problem is a lot of work.

Sounds like this change merits an entry in the 1.14 release notes (if
it's backported) or in the 1.15 release notes if it's not backported.
Would you please add a placeholder (just a section header or a ToC
link) or file a corresponding issue, so we don't forget to document
this change?

> * subversion/libsvn_repos/authz.c
>   (authz_check_access): New helper function, extracted from ...
>   (svn_repos_authz_check_access): ... here. Always consider the global
>rule set in addition to the repository-specific one. The results
>from both rulesets are now merged as was the case before SVN 1.10.
> 
> * subversion/tests/cmdline/svnauthz_tests.py
>   (svnauthz_accessof_file_test,
>svnauthz_accessof_repo_test,
>svnauthz_accessof_groups_file_test,
>svnauthz_accessof_groups_repo_test,
>svnauthz_accessof_is_file_test,
>svnauthz_accessof_is_repo_test): Adjust test expectations accordingly.
> 
> * subversion/tests/libsvn_repos/authz-test.c
>   (reposful_reposless_stanzas_inherit): Remove XFAIL marker.
> 
> * subversion/tests/libsvn_repos/repos-test.c
>   (test_authz_prefixes): Adjust test expectations. This test shows that
>the behaviour change was likely deliberate. This test assumed that
>global rules would only apply if listed after per-repository rules.
>Change test expectations such that global rules are always taken
>into account.

Re: svn commit: r1881985 - /subversion/trunk/subversion/tests/cmdline/merge_tests.py

[Daniel Shahaf]

futat...@apache.org wrote on Thu, 24 Sep 2020 17:06 -:
> Author: futatuki
> Date: Thu Sep 24 17:06:39 2020
> New Revision: 1881985
> 
> URL: http://svn.apache.org/viewvc?rev=1881985=rev
> Log:
> Follow up to r1880192: Fix an EOL issue in test on Windows.
> 
> * subversion/tests/cmdline/merge-tests.py
>   (merge_deleted_folder_with_mergeinfo_2): Use os.linesep instead of '\n'
>   in expected values of svn:mergeinfo.
> 
> Modified:
> subversion/trunk/subversion/tests/cmdline/merge_tests.py
> 
> Modified: subversion/trunk/subversion/tests/cmdline/merge_tests.py
> URL: 
> http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/merge_tests.py?rev=1881985=1881984=1881985=diff
> ==
> --- subversion/trunk/subversion/tests/cmdline/merge_tests.py (original)
> +++ subversion/trunk/subversion/tests/cmdline/merge_tests.py Thu Sep 24 
> 17:06:39 2020
> @@ -18639,11 +18639,19 @@ def merge_deleted_folder_with_mergeinfo_
> )
>  
># verify that mergeinfo is set/changed on A/D, A/D/G, A/D/G2.
> +
> +  # NOTE: When writing out multi-line prop values in svn:* props, the
> +  # client converts to local encoding and local eol style.
> +  # Therefore, the expected output must contain the right kind of eoln
> +  # strings. That's why we use os.linesep in the tests below, not just
> +  # plain '\n'. The _last_ \n is also from the client, but it's not
> +  # part of the prop value and it doesn't get converted in the pipe.

I'm confused.

First, if the client applies any transformation at all to property
values, that'd be a bug.  Property values are opaque octet sequences,
just like file contents, so they must be emitted verbatim.  It so
happens that values of svn:* properties are UTF-8 with LF line endings,
so that's what Python should see, regardless of the local encoding and
EOL style.

For the same reason, I'd have expected the expected output to be given
as bytes objects rather than as str objects.

Second, where is the 'last \n' that the comment refers to?  I don't see
it in the code.  I wouldn't expect a trailing newline to be emitted,
either, since svnadmin.tests.check_prop() passes --strict, which is
documented as a deprecated alias to --no-newline.

Thanks!

Daniel


>expected_mergeinfo = [
>  ('A',   ['/branch_A:3-7']),
> -('A/D', ['/branch_A/D:5-7\n', '/branch_B/C:1*']),
> -('A/D/G',   ['/branch_A/D/G:5-7\n', '/branch_B/C/G:1*']),
> -('A/D/G2',  ['/branch_A/D/G2:5-7\n', '/branch_B/C/G2:1*']),
> +('A/D', ['/branch_A/D:5-7'+os.linesep, '/branch_B/C:1*']),
> +('A/D/G',   ['/branch_A/D/G:5-7'+os.linesep, '/branch_B/C/G:1*']),
> +('A/D/G2',  ['/branch_A/D/G2:5-7'+os.linesep, '/branch_B/C/G2:1*']),
>  ]
>for path, mergeinfo in expected_mergeinfo:
>  svntest.actions.check_prop('svn:mergeinfo', sbox.ospath(path),
> 
> 

Re: svn commit: r1881625 - /subversion/site/publish/faq.ja.html

[Daniel Shahaf]

futat...@apache.org wrote on Thu, 10 Sep 2020 15:52 -:
> +++ subversion/site/publish/faq.ja.html Thu Sep 10 15:52:48 2020
> @@ -514,31 +514,25 @@ Win32システムにã�

Setting svn:mime-type to "text/html; charset=utf-8" should make future diffs 
readable.

We did that in r1806654 for mod_dav_svn, but svnmailer seems to honour
that too, according to getEncodingFromMimeType() in its source.

Cheers,

Daniel

Re: svn commit: r1880914 - /subversion/branches/1.14.x/STATUS

[Daniel Shahaf]

james...@apache.org wrote on Mon, 17 Aug 2020 03:00 -:
> +++ subversion/branches/1.14.x/STATUS Mon Aug 17 03:00:37 2020
> @@ -32,24 +32,25 @@ Candidate changes:
>   * r1880886
> Fix crash in JavaHL JNI wrapper caused by object lifetimes
> Justification:
>   Fixes a crash encountered with GCC 10.
> Votes:
> - +1: hartmannathan
> + +1: hartmannathan, jamessan

Any reason not to move this to approved, James?  Bindings changes
require two votes.

Cheers,

Daniel

Re: svn commit: r1879240 - /subversion/branches/1.13.x/STATUS

[Daniel Shahaf]

Good morning Bert,

rhuij...@apache.org wrote on Fri, 26 Jun 2020 13:39 -:
> Author: rhuijben
> Date: Fri Jun 26 13:39:17 2020
> New Revision: 1879240
> 
> URL: http://svn.apache.org/viewvc?rev=1879240=rev
> Log:
> * STATUS: Cast some votes.

Just making sure: you do remember that 1.13.x is past EOL, right?  Only
1.10.x and 1.14.x are currently supported.

The comment at the top of 1.13.x/STATUS should be updated to reflect
this.

Cheers,

Daniel


> Modified:
> subversion/branches/1.13.x/STATUS
> 
> Modified: subversion/branches/1.13.x/STATUS
> URL: 
> http://svn.apache.org/viewvc/subversion/branches/1.13.x/STATUS?rev=1879240=1879239=1879240=diff
> ==
> --- subversion/branches/1.13.x/STATUS (original)
> +++ subversion/branches/1.13.x/STATUS Fri Jun 26 13:39:17 2020
> @@ -20,7 +20,7 @@ Candidate changes:
>Justification:
>  Server should not be able to trigger assertion failures in clients.
>Votes:
> -+1: stsp
> ++1: stsp, rhuijben
>  
>   * r1872030, r1872031, r1872104, r1872107, r1872108, r1872115, r1872118,
> r1872121, r1872388, r1872433, r1872910, r1872919, r1872921, r1872922,
> @@ -36,7 +36,7 @@ Candidate changes:
> Justification:
>   Fixes wrong output and an assertion failure in debug mode.
> Votes:
> - +1: brane
> + +1: brane, rhuijben
>  
>   * r1879198
> Fix an invalid quoting in working copy upgrade system that only works
> 
> 

Re: svn commit: r1879198 - /subversion/trunk/subversion/libsvn_wc/wc-metadata.sql

[Daniel Shahaf]

Daniel Shahaf wrote on Fri, 26 Jun 2020 13:27 +:
> b...@qqmail.nl wrote on Thu, 25 Jun 2020 19:41 +0200:
> > It looks like you get failures on this when you compile recent SQLite 
> > versions with OMIT_DEPRECATED.
> > (I just bumped several dependencies on the Windows buildbot in an attempt 
> > to fix the python breakage)
> > 
> > This should probably be backported...  
> 
> Nominated with my own vote only.  You may want to add yours.

Oh, you'd beat me to it.  I'll merge the nominations.  Sorry for the noise.

Re: svn commit: r1879198 - /subversion/trunk/subversion/libsvn_wc/wc-metadata.sql

[Daniel Shahaf]

b...@qqmail.nl wrote on Thu, 25 Jun 2020 19:41 +0200:
> It looks like you get failures on this when you compile recent SQLite 
> versions with OMIT_DEPRECATED.
> (I just bumped several dependencies on the Windows buildbot in an attempt to 
> fix the python breakage)
> 
> This should probably be backported...

Nominated with my own vote only.  You may want to add yours.

Cheers,

Daniel

>   Bert
> 
> 
> -Oorspronkelijk bericht-
> Van: rhuij...@apache.org  
> Verzonden: donderdag 25 juni 2020 19:12
> Aan: commits@subversion.apache.org
> Onderwerp: svn commit: r1879198 - 
> /subversion/trunk/subversion/libsvn_wc/wc-metadata.sql
> 
> Author: rhuijben
> Date: Thu Jun 25 17:11:44 2020
> New Revision: 1879198
> 
> URL: http://svn.apache.org/viewvc?rev=1879198=rev
> Log:
> Fix invalid escape sequence in working copy queries which causes
> upgrades from Subversion 1.7 working copies to fail with recent
> SQLite versions (>= 3.29.0).
> 
> * subversion/libsvn_wc/wc-metadata.sql
>   (STMT_UPGRADE_TO_30): Use '' to delimit values and "" to escape column 
> names.
> 
> Modified:
> subversion/trunk/subversion/libsvn_wc/wc-metadata.sql
> 
> Modified: subversion/trunk/subversion/libsvn_wc/wc-metadata.sql
> URL: 
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/wc-metadata.sql?rev=1879198=1879197=1879198=diff
> ==
> --- subversion/trunk/subversion/libsvn_wc/wc-metadata.sql (original)
> +++ subversion/trunk/subversion/libsvn_wc/wc-metadata.sql Thu Jun 25 17:11:44 
> 2020
> @@ -636,7 +636,7 @@ ON NODES (wc_id, moved_to, op_depth);
>  
>  CREATE INDEX IF NOT EXISTS I_PRISTINE_MD5 ON PRISTINE (md5_checksum);
>  
> -UPDATE nodes SET presence = "server-excluded" WHERE presence = "absent";
> +UPDATE nodes SET presence = 'server-excluded' WHERE presence = 'absent';
>  
>  /* Just to be sure clear out file external skels from pre 1.7.0 development
> working copies that were never updated by 1.7.0+ style clients */
> 
> 
> 

Re: svn commit: r1878918 - /subversion/trunk/build.conf

[Daniel Shahaf]

Add this to the backport nomination?

rhuij...@apache.org wrote on Wed, 17 Jun 2020 11:43 -:
> Author: rhuijben
> Date: Wed Jun 17 11:43:11 2020
> New Revision: 1878918
> 
> URL: http://svn.apache.org/viewvc?rev=1878918=rev
> Log:
> * build.conf: Add filesize-test to ALL_TESTS to allow testing on Windows.
> 
> Modified:
> subversion/trunk/build.conf
> 
> Modified: subversion/trunk/build.conf
> URL: 
> http://svn.apache.org/viewvc/subversion/trunk/build.conf?rev=1878918=1878917=1878918=diff
> ==
> --- subversion/trunk/build.conf (original)
> +++ subversion/trunk/build.conf Wed Jun 17 11:43:11 2020
> @@ -1578,7 +1578,7 @@ libs = __ALL__
> checksum-test compat-test config-test hashdump-test mergeinfo-test
> opt-test packed-data-test path-test prefix-string-test
> priority-queue-test root-pools-test stream-test
> -   string-test time-test utf-test bit-array-test
> +   string-test time-test utf-test bit-array-test filesize-test
> error-test error-code-test cache-test spillbuf-test crypto-test
> revision-test
> subst_translate-test io-test
> 
> 

Re: svn commit: r1878211 - in /subversion/branches/1.14.x: ./ STATUS subversion/libsvn_client/mtcc.c subversion/tests/cmdline/svnmucc_tests.py

[Daniel Shahaf]

svn-r...@apache.org wrote on Thu, 28 May 2020 04:00 -:
> +++ subversion/branches/1.14.x/subversion/libsvn_client/mtcc.c Thu May 28 
> 04:00:12 2020
> @@ -453,7 +453,8 @@ mtcc_verify_create(svn_client__mtcc_t *m
>  
>if (op)
>  return svn_error_createf(SVN_ERR_FS_ALREADY_EXISTS, NULL,
> - _("Path '%s' already exists"),
> + _("Path '%s' already exists, or was created 
> "
> +   "by an earlier operation"),
>   new_relpath);

Is "was created" in the correct tense?  This error message is triggered
before the RA session is opened, so it shouldn't imply that the earlier
operation completed successfully.

> +++ subversion/branches/1.14.x/subversion/tests/cmdline/svnmucc_tests.py Thu 
> May 28 04:00:12 2020
> @@ -534,7 +534,8 @@ def svnmucc_type_errors(sbox):
>xtest_svnmucc(sbox.repo_url,
> -["svnmucc: E160020: Path 'Z' already exists"],
> +["svnmucc: E160020: Path 'Z' already exists, or was created "
> + "by an earlier operation"],
>  '-m', '',
>  'mkdir', 'A/Z',
>  'put', sbox.ospath('file'), 'A/Z')
> @@ -576,7 +577,8 @@ def svnmucc_propset_and_put(sbox):
># Put same file twice (non existing)
>xtest_svnmucc(sbox.repo_url,
> -["svnmucc: E160020: Path 't3' already exists"],
> +["svnmucc: E160020: Path 't3' already exists, or was created 
> "
> + "by an earlier operation"],
>  '-m', '',
>  'put', sbox.ospath('file'), 't3',
>  'put', sbox.ospath('file'), 't3')

Re: svn-role didn't run? (was: svn commit: r1878189 - /subversion/branches/1.14.x/STATUS)

[Daniel Shahaf]

Daniel Shahaf wrote on Thu, 28 May 2020 03:01 +:
> jcor...@apache.org wrote on Wed, 27 May 2020 18:39 -:
> > Author: jcorvel
> > Date: Wed May 27 18:39:53 2020
> > New Revision: 1878189
> > 
> > URL: http://svn.apache.org/viewvc?rev=1878189=rev
> > Log:
> > * 1.14.x/STATUS: Vote for r1877072, approving.
> > 
> > Modified:
> > subversion/branches/1.14.x/STATUS  
> 
> Why didn't this get merged overnight?  The svn-role cron job should
> have merged this two hours ago.

Oh.  I forgot to account for daylight savings time.  Sorry, ignore me.

HACKING += release-lines.yaml? (was: svn commit: r1878162 - /subversion/trunk/tools/dist/release-lines.yaml)

[Daniel Shahaf]

s...@apache.org wrote on Wed, 27 May 2020 14:34 -:
> Author: stsp
> Date: Wed May 27 14:34:18 2020
> New Revision: 1878162
> 
> URL: http://svn.apache.org/viewvc?rev=1878162=rev
> Log:
> * tools/dist/release-lines.yaml: Update supported release list for release.py.

Good catch.  Is this already documented in
docs/community-guide/releasing.part.html?  If not, should it be added?

>   1.14 is our new supported release, and 1.10 is also still supported.
>   The 1.13 and 1.9 release lines both drop out of support.
> 
> Modified:
> subversion/trunk/tools/dist/release-lines.yaml
> 
> Modified: subversion/trunk/tools/dist/release-lines.yaml
> URL: 
> http://svn.apache.org/viewvc/subversion/trunk/tools/dist/release-lines.yaml?rev=1878162=1878161=1878162=diff
> ==
> --- subversion/trunk/tools/dist/release-lines.yaml (original)
> +++ subversion/trunk/tools/dist/release-lines.yaml Wed May 27 14:34:18 2020
> @@ -22,9 +22,9 @@ tool_versions:
>'trunk': *TOOL_VERSIONS_1_10
>  
>  # The version that is our current recommended release
> -recommended_release: '1.13'
> +recommended_release: '1.14'
>  # For clean-dist, a whitelist of artifacts to keep, by version.
> -supported_release_lines: ['1.9', '1.10', '1.13']
> +supported_release_lines: ['1.10', '1.14']
>  # Long-Term Support (LTS) versions
> -lts_release_lines: ['1.9', '1.10', '1.14']
> +lts_release_lines: ['1.10', '1.14']
>  
> 
> 

svn-role didn't run? (was: svn commit: r1878189 - /subversion/branches/1.14.x/STATUS)

[Daniel Shahaf]

jcor...@apache.org wrote on Wed, 27 May 2020 18:39 -:
> Author: jcorvel
> Date: Wed May 27 18:39:53 2020
> New Revision: 1878189
> 
> URL: http://svn.apache.org/viewvc?rev=1878189=rev
> Log:
> * 1.14.x/STATUS: Vote for r1877072, approving.
> 
> Modified:
> subversion/branches/1.14.x/STATUS

Why didn't this get merged overnight?  The svn-role cron job should
have merged this two hours ago.

> +++ subversion/branches/1.14.x/STATUS Wed May 27 18:39:53 2020
> @@ -62,3 +54,11 @@ Veto-blocked changes:
>  
>  Approved changes:
>  =
> +
> + * r1877072
> +   svnmucc: Change an error message to state another possible cause of the
> +   error.
> +   Justification:
> + Error messages should be accurate.  User reported (issue #4854).
> +   Votes:
> + +1: danielsh, stsp, jcorvel
> 
> 

Re: svn commit: r1877712 - in /subversion/trunk/subversion/tests/cmdline: entries-dump.c svntest/main.py

[Daniel Shahaf]

futat...@apache.org wrote on Thu, 14 May 2020 01:46 -:
> Log:
> Use safe bytes literals when set string values in working copy entries. 
> 

If someone reads this log message in a year, I don't think it'd be clear
to them what the problem being fixed was.  I think it would be useful to
be more specific about the circumstances; for example:

entries-dump: Escape string-typed attribute values when serializing
them as Python string literals.

Before this commit, a filesystem node named «foo\bar» (a single,
7-character path component) would cause «e.name = 'foo\bar'» to be
emitted.  The unescaped backslash would manifest as a test failure or
a SyntaxError, depending on the following characters.

This was triggered by svnadmin_tests 42 verify_metadata_only under
Python 3 on Windows.

The reference to "svnadmin_tests 42 verify_metadata_only" is only
a placeholder, to be replaced by the correct reference.

I specifically used the term "filesystem node" to clarify that «foo\bar»
was to be taken as an svn_fspath__* path (where backslash has no
special meaning), as opposed to, say, an svn_dirent_* path (where
backslash is a delimiter on Windows).

> * subversion/tests/cmdline/entries-dump.c
>   (print_prefix): New function.
>   (str_value):
>   - Add argument to specify pool.
>   - Print human readable value of "value" as is in comment, then set it
> as str value by using hex escaped bytes literal.
>   (entries_dump): Add pool argument to str_value() calls.
>   (main):
>   - Print "Entry" class definition as prefix before entry_dump() or 
> tree_dump() 
>   - Style fix on if statement (using blocks). 
>   (): Add include files for assert() and svn_xml_escape_attr_cstring()
> * subversion/tests/cmdline/svntest/main.py
>   (run_entiresdump, run_entriesdump_tree): Move definition of "Entry" class
>   into generated code by entries-dump execution. 
> 
> Review By: danielsh

For future reference, the "${Verb}ed by" phrases in log messages are
case-sensitive:

tools/dev/contribulyze.py:521:field_re = re.compile(
tools/dev/contribulyze.py:522:   
'^(Patch|Review(ed)?|Suggested|Found|Inspired|Tested|Reported) by:'
tools/dev/contribulyze.py:523:   '\s*\S.*$')
⋮
tools/dev/contribulyze.py:566:m = field_re.match(line)
⋮
tools/dev/contribulyze.py:604:  m = field_re.match(line)

This doesn't matter in this case, of course, since the person being
credited is already a full committer; however, when crediting people who
aren't already full committers, the correct case form should be used so
the regexp matches and the contribulyzer pages are updated.  (Unless we
change the script)

Cheers,

Daniel

Grace period for 1.13 users to upgrade to 1.14? (was: svn commit: r1877222 - /subversion/site/publish/docs/release-notes/1.14.html)

[Daniel Shahaf]

danie...@apache.org wrote on Thu, 30 Apr 2020 16:21 -:
> +++ subversion/site/publish/docs/release-notes/1.14.html Thu Apr 30 16:21:48 
> 2020
> @@ -1330,6 +1330,20 @@ if they occur.
> +
> +Subversion 1.13.x is end of life
> +   +title="Link to this section">
> +
> +
> +The Subversion 1.13.x line is end of life (EOL).
> +This doesn't mean that your 1.13 installation is doomed; if it works
> +well and is all you need, that's fine.  "End of life" just means we've
> +stopped accepting bug reports against 1.13.x versions, and will not
> +make any more 1.13.x releases.

I just copied the text we use for 1.9, but there's a distinction: users
of 1.9 have had time to upgrade to 1.10 before 1.14.0 becomes GA,
whereas users of 1.13 have not.  So, should we promise some sort of
grace period for users of 1.13.x — i.e., a period following the release
of 1.14.0 during which we'll still fix security bugs in 1.13.0?

Granted, 1.13 was a regular release and was only promised to be
supported for six months (or until 1.14.0) in the first place, so the
grace period needn't be long.

Cheers,

Daniel

Can 1.10.x claim 2020 copyright? (was: svn commit: r1876864 - /subversion/branches/1.10.x/STATUS)

[Daniel Shahaf]

s...@apache.org wrote on Thu, 23 Apr 2020 07:08 +00:00:
> +++ subversion/branches/1.10.x/STATUS Thu Apr 23 07:08:37 2020
> @@ -46,6 +46,13 @@ Candidate changes:
> + * r1874850
> +   Update copyright year to 2020.
> +   Justification:
> + The future is here!
> +   Votes:
> + +1: stsp

Between 2020-01-01 and 2020-04-22 this branch received exactly one
commit that touched files other than STATUS.  That commit, r1876072,
was done in 2020, but it consisted of a conflict-free merge of commits
from 2019.

I'm not sure if under these circumstances we're actually able to claim
2020 in the copyright notice.  Perhaps we are allowed to claim 2020
just because the merge was done in 2020… but then again, perhaps we
aren't.  IANAL.

Presumably we'll merge copyrightable commits authored in 2020 before the
next 1.10.x release, which would resolve this concern.

Re: svn commit: r1876016 - /subversion/trunk/subversion/bindings/swig/INSTALL

[Daniel Shahaf]

Good morning futatuki@,

futat...@apache.org wrote on Wed, 01 Apr 2020 21:07 -:
> Explain SWIG is needed for building Python 2 bindings.
> 
> Since Subversion distibution 1.14.0 and later will ship with SWIG Python
> bindings *.c source code for Python 3, clarify *.c source files for Python 2
> and Python 3 are different and incompatible, and describe how to clear
> incompatible intermediate files.
> 
> * subversion/bindings/swig/INSTALL
>  (STATUS OF THE SWIG BINDINGS):
>   Add explanation about supported Python version and distribution tarball.
>  (BUILDING SWIG BINDINGS FOR SVN ON UNIX Step 1):
>   Add new case that SWIG is needed for Python 2 bindings.
>  (BUILDING SWIG BINDINGS FOR SVN ON UNIX Step 2):
>   Update Python and perl version in commadline example. 
>  (BUILDING SWIG BINDINGS FOR SVN ON UNIX Step 3):
>   Insert optional step to clear intermediate files previously generated.
>  (everywhere) Strip trailing white space in the end of line.

Thanks for this.  May I propose a few tweaks?  Patch attached, as well
as its wdiff for review purposes.  The changes are mainly:

- Use passive voice

- Rearrange information to put the common case first and rarer cases
  nearer the end

Let me know if I missed anything.

Furthermore, since this documentation is for 1.14.0, I assume it should
be backported to branches/1.14.x?  If that is the case, feel free to go
ahead and add it to branches/1.14.x/STATUS.  (You can automate editing
and committing the STATUS file by running «tools/dist/nominate.pl
r1876016 "hello world"» in a working copy of branches/1.14.x.)

Apologies in advance but I'll not be online much in the next few days,
so I may be slow to respond.

Cheers,

Daniel
Index: subversion/bindings/swig/INSTALL
===
--- subversion/bindings/swig/INSTALL	(revision 1876016)
+++ subversion/bindings/swig/INSTALL	(working copy)
@@ -5,12 +5,12 @@ STATUS OF THE SWIG BINDINGS
 * Python
 
   The Python bindings are fairly well developed, although there are some
-  missing parts. We support both of Python 2.7 and Python 3.x, but you
-  can't install SWIG Python bindings for multiple version of Python
-  in the same environment, because they need to install mutually
-  incompatible C shared library in same name.  We are now shipping
-  distribution tarball with C source files for Python 3 bindings
-  generated by SWIG.
+  missing parts. We support both of Python 2.7 and Python 3.x; however,
+  SWIG Python bindings for different versions of Python cannot be
+  simultaneously installed in the same environment, because they need to
+  install mutually incompatible C shared libraries under the same name.  The
+  distribution tarballs are shipped with SWIG-generated C source files for
+  Python 3.x.
 
   (N.B. As discussed below, they will not compile in Debug mode on Windows.)
 
@@ -72,13 +72,13 @@ BUILDING SWIG BINDINGS FOR SVN ON UNIX
 Step 1: [Optional] Install a suitable version of SWIG
 
 * SWIG installation is optional.  You do not need to install SWIG
-  if you are using a Subversion distribution tarball except Python
-  bindings for Python 2 because it already contains the source files
-  generated by SWIG.  The Python bindings source file in the distribution
-  tarball is only for Python 3.  You will need a suitable version of
-  SWIG if you are using a working copy of Subversion's sources checked
-  out from the repository, or if you want to generate the SWIG
-  language bindings C source files by yourself.
+  if you are using a Subversion distribution tarball because it already
+  contains the source files generated by SWIG.  You will need a suitable
+  version of SWIG if you are using a working copy of Subversion's sources
+  checked out from the repository; if you want to generate the SWIG
+  language bindings C source files by yourself; or if you want to build
+  Python 2.x bindings (since the SWIG-generated C source files in the
+  distribution tarballs target Python 3.x).
 
 * We currently support SWIG versions 2.0.0 and later, with the
   following notes:
@@ -160,12 +160,12 @@ Step 3:  Install Specific Language Bindings
 
 *  Python
 
-   1.  (Optional) If you want to build Python bindings for other version of
-   Python than target of previously built bindings C sources (e.g.
-   using Subversion distribution tarball but want to build Python 2
-   bindings), run 'make clean-swig-py' from the top of the Subversion
-   build tree, to ensure not to use incompatible version of bindings
-   source files.
+   1.  (Optional) If you want to build Python bindings for a version of
+   Python than other than that the prebuilt bindings C sources target
+   (e.g., if you use the Subversion distribution tarball but want to build
+   Python 2 bindings), run 'make clean-swig-py' from the top of the
+   Subversion build tree, to ensure not to use 

Re: svn commit: r1875921 - in /subversion/trunk: subversion/include/ subversion/include/private/ subversion/libsvn_fs_fs/ subversion/svnadmin/ subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/

[Daniel Shahaf]

s...@apache.org wrote on Tue, 31 Mar 2020 08:53 -:
> +++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Tue Mar 31 08:53:55 2020
> @@ -2343,3 +2344,168 @@ svn_fs_fs__info_config_files(apr_array_h
>  }  
> +
> +static svn_error_t *
> +ensure_representation_sha1(svn_fs_t *fs,
> +   representation_t *rep,
> +   apr_pool_t *pool)

Please add a docstring.

> +{
> +  if (!rep->has_sha1)
> +{
> +  svn_stream_t *contents;
> +  svn_checksum_t *checksum;
> +
> +  SVN_ERR(svn_fs_fs__get_contents(, fs, rep, FALSE, pool));
> +  SVN_ERR(svn_stream_contents_checksum(, contents,
> +   svn_checksum_sha1, pool, pool));
> +
> +  memcpy(rep->sha1_digest, checksum->digest, APR_SHA1_DIGESTSIZE);
> +  rep->has_sha1 = TRUE;
> +}
> +
> +  return SVN_NO_ERROR;
> +}  
> +
> +static svn_error_t *
> +reindex_node(svn_fs_t *fs,
> + const svn_fs_id_t *id,
> + svn_revnum_t rev,
> + svn_fs_fs__revision_file_t *rev_file,
> + svn_cancel_func_t cancel_func,
> + void *cancel_baton,
> + apr_pool_t *pool)

Please add a docstring.

> +{

Cheers,

Daniel

Re: svn commit: r1875312 - /subversion/site/publish/docs/release-notes/1.14.html

[Daniel Shahaf]

hartmannat...@apache.org wrote on Tue, 17 Mar 2020 18:36 -:
> 1.14 release notes: Document escaping of pathname args to $SVN_EDITOR
> 
> * docs/release-notes/1.14.html
>   (editor-filename-escaping): New section. Documents the pathname escaping
> implemented in r1874057, r1874093, and r1875230.

Thanks!

> +When invoking the user-defined $SVN_EDITOR, such as during

The editor may be configured in other ways: --editor-cmd,
--config-option=…:editor-cmd, $EDITOR.

Suggest to say "When invoking the user-defined editor", with or without
listing some of the ways in which the editor command may be specified.

> +interactive conflict resolution, Subversion now performs escaping of any
> +special characters in the pathname of the file to be edited. (See
> +http://svn.apache.org/r1874057;>r1874057,
> +http://svn.apache.org/r1874093;>r1874093, and
> +http://svn.apache.org/r1875230;>r1875230.)
> +
> +Note that escaping is performed only on the pathname argument.
> +$SVN_EDITOR itself is not escaped: As before, the editor is invoked
> +through the shell and the user must properly escape/quote
> +$SVN_EDITOR. See the related FAQ
> +https://subversion.apache.org/faq.html#svn-editor;>"How do I deal
> +with spaces in the editor path?  Also, how can I define command line options
> +for the editor?"

Add full stop after the closing quotation mark?

Add an example?  (An example is worth a thousand words…)

Cheers,

Daniel

> + 
> +
>  
>  Improvements to the interactive conflict resolver
> 
> 

Re: svn commit: r1875127 - /subversion/site/publish/docs/release-notes/1.14.html

[Daniel Shahaf]

hartmannat...@apache.org wrote on Thu, 12 Mar 2020 15:15 -:
> 1.14 release notes: Document selectable shelving implementation
> 
> * docs/release-notes/1.14.html
>   (shelving): Document that users can select between Shelving-v2 and
> Shelving-v3 via the SVN_EXPERIMENTAL_COMMANDS environment
> variable, as introduced in r1875037.

Shelving is now mentioned in three separate sections:
#shelving-transition, #checkpointing, #shelving.  I am a little
disoriented by this.  Shouldn't #shelving-transition link to #shelving,
for one?

> +The shelving CLI implementation is selected by an environment variable,
> +SVN_EXPERIMENTAL_COMMANDS, as follows:
> +
> +
> +  
> +environment variable
> +shelving CLI implementation
> +  
> +  
> +Environment variable not set
> +Shelving-v3, as introduced in 1.12

This row is no longer correct as of r1875039.

> +  

Cheers,

Daniel

Re: svn commit: r1874634 [3/3] - in /subversion/branches/decouple-shelving-cli/subversion: include/private/svn_client_shelf2.h libsvn_client/shelf2.c svn/shelf2-cmd.c svn/shelf2-cmd.h svn/svn.c tests/

[Daniel Shahaf]

Julian Foad wrote on Sat, 29 Feb 2020 08:12 +00:00:
> 
> Daniel Shahaf wrote:
> 
> 
> > 
> > This doesn't seem forward-compatible: if in 1.15 we add
> > SVN_EXPERIMENTAL_COMMANDS=foo, using that environment variable setting
> > on 1.14 (assuming the above branch is merged by 1.14) would disable
> > shelving. I'm aware that compatibility is a lesser concern for experimental
> > features, but still, this sort of compatibility seems achievable.
> > 
> > 
> 
> 
> I didn't want this commit to change the default, but I think it would 
> be reasonable now to change the default to a simple opt-in (each 
> feature is enabled iff the var contains its feature-name) for 1.14.
> 
> WDYT?

Off the top of my head: that'd be a sensible design, but it's not the
only conceivable design that'd be sensible.  For example, designs that
enable some or all experimental subcommands by default would be sensible
too, since those subcommands' names already have "x-" prefixes to warn
of their experimental status.

Cheers,

Daniel

Re: svn commit: r1874634 [3/3] - in /subversion/branches/decouple-shelving-cli/subversion: include/private/svn_client_shelf2.h libsvn_client/shelf2.c svn/shelf2-cmd.c svn/shelf2-cmd.h svn/svn.c tests/

[Daniel Shahaf]

julianf...@apache.org wrote on Fri, 28 Feb 2020 22:15 -:
> Author: julianfoad
> Date: Fri Feb 28 22:15:15 2020
> New Revision: 1874634
> 
> URL: http://svn.apache.org/viewvc?rev=1874634=rev
> Log:
> Add the shelving v2 implementation from Subversion 1.11, as an alternative
> to the shelving v3 implementation from Subversion 1.12.
> 
> They have substantially different pros and cons, so it is beneficial for the
> user to be able to choose.
> 
> Make the shelving CLI version selectable by an environment variable:
>   env. var. not set => shelving v3 enabled
>   SVN_EXPERIMENTAL_COMMANDS=shelf3  => shelving v3 enabled
>   SVN_EXPERIMENTAL_COMMANDS=shelf2  => shelving v2 enabled
>   SVN_EXPERIMENTAL_COMMANDS==> no shelving CLI
> 
> * subversion/svn/svn.c
>   Enable shelving v3 or v2 or neither, depending on the environment variable
>   SVN_EXPERIMENTAL_COMMANDS.

julianf...@apache.org wrote on Fri, 28 Feb 2020 22:15 -:
> +++ subversion/branches/decouple-shelving-cli/subversion/svn/svn.c Fri Feb 28 
> 22:15:15 2020
> @@ -2066,8 +2068,16 @@ sub_main(int *exit_code, int argc, const
>/* Init the temporary buffer. */
>svn_membuf__create(, 0, pool);
>  
> -  /* Add externally defined commands */
> -  add_commands(svn_cl__cmd_table_shelf3, pool);
> +  /* Add experimental commands, if requested */
> +  exp_cmds = getenv("SVN_EXPERIMENTAL_COMMANDS");
> +  if (!exp_cmds || strstr(exp_cmds, "shelf3"))
> +{
> +  add_commands(svn_cl__cmd_table_shelf3, pool);
> +}
> +  else if (exp_cmds && strstr(exp_cmds, "shelf2"))
> +{
> +  add_commands(svn_cl__cmd_table_shelf2, pool);
> +}

This doesn't seem forward-compatible: if in 1.15 we add
SVN_EXPERIMENTAL_COMMANDS=foo, using that environment variable setting
on 1.14 (assuming the above branch is merged by 1.14) would disable
shelving.  I'm aware that compatibility is a lesser concern for experimental
features, but still, this sort of compatibility seems achievable.

TIMTOWTDI; for example, «os.putenv('SVN_EXPERIMENTAL_COMMANDS', 'shelf=3 
foo=1')»,
or check «strstr(exp_cmds, "shelf")» in addition to whether exp_cmds is not 
NULL.

(I'm not sure whether you intend the envvar-based API to be released or
just to be an interim development phase, but I'm assuming the former
since the log message didn't say otherwise.)

Cheers,

Daniel

Re: svn commit: r1874631 - in /subversion/branches/decouple-shelving-cli/subversion/svn: cl.h shelf-cmd.c shelf-cmd.h svn.c

[Daniel Shahaf]

Good morning Julian,

julianf...@apache.org wrote on Fri, 28 Feb 2020 21:09 -:
> Initialize the 'svn x-shelf-*' commands programmatically at run time,
> instead of hard-coding them in svn's main command table.
> 
> A step towards decoupling the experimental shelving CLI from the main CLI.

Not sure what the grand plan here, but as a request: Could we continue
to have a programmatic way to get all subcommands, --options, etc, that
exist and are enabled?  That exists currently («svn help -v» and «svn
help $subcommand») and it's a _de facto_ API — there are scripts out
there that rely on it — so it'd be nice to keep that functionality
available, under some name or another.

Cheers,

Daniel

Re: svn commit: r1874057 - in /subversion/trunk/subversion: libsvn_subr/cmdline.c tests/cmdline/update_tests.py

[Daniel Shahaf]

james...@apache.org wrote on Sat, 15 Feb 2020 16:24 -:
> Escape filenames when invoking $SVN_EDITOR
> 
> Per https://subversion.apache.org/faq.html#svn-editor, $SVN_EDITOR is invoked
> through the shell instead of directly executed.  The user is expected to
> properly escape/quote $SVN_EDITOR, but svn was putting the filename directly
> into the command without any escaping.  This therefore breaks attempts to,
> e.g., run the editor from the merge conflict dialog when a path has special
> characters.
> 
> Update locations where we invoke the editor to quote the filename as well as
> escape shell special characters using apr_pescape_shell().  The quotes are
> needed in addition to the escaping, since apr_pescape_shell() does not escape
> whitespace.

> +++ subversion/trunk/subversion/libsvn_subr/cmdline.c Sat Feb 15 16:24:53 2020
> @@ -1330,7 +1331,10 @@ svn_cmdline__edit_file_externally(const
>  return svn_error_wrap_apr
>(apr_err, _("Can't change working directory to '%s'"), base_dir);
>  
> -  cmd = apr_psprintf(pool, "%s %s", editor, file_name);
> +  /* editor is explicitly documented as being interpreted by the user's 
> shell,
> + and as such should already be quoted/escaped as needed. */
> +  cmd = apr_psprintf(pool, "%s \"%s\"", editor,
> + apr_pescape_shell(pool, file_name));

If FILE_NAME is «;» (i.e., a single semicolon), apr_pescape_shell()
will return «\;» (two bytes) and then the command string will be «vi "\;"»,
so vi would edit the file «\;» (two bytes).

This can happen in the conflict-callbacks.c caller, which
passes LOCAL_ABSPATH of a versioned file:

[[[
#!/bin/sh
rm -rf r wc
svnadmin create r
svnmucc put -U file://`pwd`/r -m 'r1' /dev/null ';'
svn co -q file://`pwd`/r wc
cd wc
echo foo > ';'
svn ci -q -m 'r2'
svn up -q -r1
echo bar > ';'
../subversion/svn/svn up --accept=e --editor-cmd='false'
]]]

[[[
% ./repro.sh
r1 committed by daniel at 2020-02-15T17:23:47.372055Z
Updating '.':
C;
Updated to revision 2.
system('false "\;"') returned 256
Merge conflicts in ';' marked as resolved.
Summary of conflicts:
  Text conflicts: 0 remaining (and 1 already resolved)
]]]

That could conceivably happen for other callers as well if
svn_io_open_uniquely_named() returns an abspath which contains
a literal semicolon in the directory part.  Its API contract
doesn't forbid that.

Sorry for not noticing this before; I'm sure I saw a previous version
of this patch at some point…

Daniel

>sys_err = system(cmd);
>  
>apr_err = apr_filepath_set(old_cwd, pool);
> @@ -1489,7 +1493,11 @@ svn_cmdline__edit_string_externally(svn_
>err = svn_utf_cstring_from_utf8(_native, tmpfile_name, pool);
>if (err)
>  goto cleanup;
> -  cmd = apr_psprintf(pool, "%s %s", editor, tmpfile_native);
> +
> +  /* editor is explicitly documented as being interpreted by the user's 
> shell,
> + and as such should already be quoted/escaped as needed. */
> +  cmd = apr_psprintf(pool, "%s \"%s\"", editor,
> + apr_pescape_shell(pool, tmpfile_native));

Re: svn commit: r1873943 - /subversion/trunk/subversion/tests/libsvn_fs_fs/fs-fs-pack-test.c

[Daniel Shahaf]

jor...@apache.org wrote on Wed, 12 Feb 2020 13:41 -:
> Author: jorton
> Date: Wed Feb 12 13:41:25 2020
> New Revision: 1873943
> 
> URL: http://svn.apache.org/viewvc?rev=1873943=rev
> Log:
> Fix test failures seen on 32-bit architectures (Fedora Raw Hide, both
> i686 and armv7hl) when building with GCC 10 snapshots.
> 
> * subversion/tests/libsvn_fs_fs/fs-fs-pack-test.c
>   (get_rev_contents): Avoid signed integer overflow on platforms with
>   32-bit long.
> 
> Modified:
> subversion/trunk/subversion/tests/libsvn_fs_fs/fs-fs-pack-test.c
> 
> Modified: subversion/trunk/subversion/tests/libsvn_fs_fs/fs-fs-pack-test.c
> URL: 
> http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_fs_fs/fs-fs-pack-test.c?rev=1873943=1873942=1873943=diff
> ==
> --- subversion/trunk/subversion/tests/libsvn_fs_fs/fs-fs-pack-test.c 
> (original)
> +++ subversion/trunk/subversion/tests/libsvn_fs_fs/fs-fs-pack-test.c Wed Feb 
> 12 13:41:25 2020
> @@ -59,7 +59,8 @@ static const char *
>  get_rev_contents(svn_revnum_t rev, apr_pool_t *pool)
>  {
>/* Toss in a bunch of magic numbers for spice. */
> -  apr_int64_t num = ((rev * 1234353 + 4358) * 4583 + ((rev % 4) << 1)) / 42;
> +  apr_int64_t rev64 = rev;
> +  apr_int64_t num = ((rev64 * 1234353 + 4358) * 4583 + ((rev64 % 4) << 1)) / 
> 42;

Should this be backported?

Cheers,

Daniel

Re: svn commit: r1872919 - /subversion/trunk/subversion/tests/libsvn_subr/mergeinfo-test.c

[Daniel Shahaf]

julianf...@apache.org wrote on Fri, Jan 17, 2020 at 14:54:46 -:
> @@ -2089,18 +2130,22 @@ test_rangelist_merge_random_canonical_in
> +if (failure_mode)
>{
> -printf("testcase FAIL: %s / %s\n",
> +printf("first example of a failure mode: %s / %s\n"
> +   "  %s\n",
> rangelist_to_string(rlx, iterpool),
> +   rangelist_to_string(rly, iterpool),
> +   failure_mode);

Test programs aren't supposed to print arbitrary stuff to stdout.

Specifically, the interface between run_tests.py and the test programs
specifies¹ that test programs shall only print "PASS:" and "FAIL:" lines by
default.  We could of course extend that to support printing of new info, for
example, as "INFO:" lines.  So would it make sense to prepend "INFO:" to that
line?

Cheers,

Daniel

¹ Citation: subversion/tests/README:34

Re: svn commit: r1872398 - in /subversion/trunk/tools/hook-scripts/mailer: mailer.conf.example mailer.py

[Daniel Shahaf]

futat...@apache.org wrote on Mon, Jan 06, 2020 at 23:34:17 -:
> +++ subversion/trunk/tools/hook-scripts/mailer/mailer.conf.example Mon Jan  6 
> 23:34:17 2020
> @@ -23,6 +23,10 @@
>  # This option specifies the hostname for delivery via SMTP.
>  #smtp_hostname = localhost
>  
> +# This option specifies the TCP port number to connect for SMTP.
> +# If it is not specified, 25 is used by default.
> +#smtp_port = 25
> +
> +++ subversion/trunk/tools/hook-scripts/mailer/mailer.py Mon Jan  6 23:34:17 
> 2020
> @@ -299,11 +299,15 @@ class SMTPOutput(MailedOutput):
>  (to minimize the chances of said lockout).
>  """
>  
> +if self.cfg.is_set('general.smtp_port'):
> +   smtp_port = self.cfg.general.smtp_port
> +else:
> +   smtp_port = smtplib.SMTP_PORT
>  try:
>if self.cfg.is_set('general.smtp_ssl') and self.cfg.general.smtp_ssl 
> == 'yes':
> -server = smtplib.SMTP_SSL(self.cfg.general.smtp_hostname)
> +server = smtplib.SMTP_SSL(self.cfg.general.smtp_hostname, smtp_port)

This seems to be a breaking change.  The old code, «smtplib.SMTP_SSL(foo)»,
used port 465; the new code, «smtplib.SMTP_SSL(foo, smtplib.SMTP_PORT)», will
try to connect to port 25 using SMTP-over-SSL until the administrator sets
smtp_port=465 in the config file.

Cheers,

Daniel

>else:
> -server = smtplib.SMTP(self.cfg.general.smtp_hostname)
> +server = smtplib.SMTP(self.cfg.general.smtp_hostname, smtp_port)

Re: svn commit: r1872118 - in /subversion/trunk/subversion: include/private/svn_sorts_private.h libsvn_client/merge.c libsvn_subr/mergeinfo.c libsvn_subr/sorts.c tests/libsvn_subr/mergeinfo-test.c

[Daniel Shahaf]

julianf...@apache.org wrote on Mon, Dec 30, 2019 at 15:42:17 -:
> * subversion/libsvn_subr/mergeinfo.c
>   (svn_rangelist_merge2): Extract the body of this function into a local
> 'rangelist_merge2', leaving the original as an error-checking
> wrapper. If an error occurs, report its inputs.
> 
> +++ subversion/trunk/subversion/libsvn_subr/mergeinfo.c Mon Dec 30 15:42:17 
> 2019
> @@ -1265,40 +1278,98 @@ svn_rangelist_merge2(svn_rangelist_t *ra
>  }
>  
>  #ifdef SVN_DEBUG
> -  SVN_ERR_ASSERT(svn_rangelist__is_canonical(rangelist));
> +  /*SVN_ERR_ASSERT(svn_rangelist__is_canonical(rangelist));*/

Was this change intended to be committed?

>  #endif
>  
>return SVN_NO_ERROR;
>  }

Re: svn commit: r1872030 - /subversion/trunk/subversion/libsvn_subr/mergeinfo.c

[Daniel Shahaf]

Good morning Julian,

julianf...@apache.org wrote on Fri, Dec 27, 2019 at 15:08:31 -:
> Author: julianfoad
> Date: Fri Dec 27 15:08:31 2019
> New Revision: 1872030
> 
> URL: http://svn.apache.org/viewvc?rev=1872030=rev
> Log:
> Avoid converting invalid mergeinfo to bogus valid-looking mergeinfo.
> 
> For issue #4840, "Merge assertion failure in svn_sort__array_insert".

Right.  I did try to follow the 4840 thread when you started it on dev@.

> The internal representation of mergeinfo is not supposed to include
> empty ranges.  If a representation of an empty range did occur, perhaps
> due to a bug in mergeinfo processing or improper data passed to an API,
> the mergeinfo printing functions produced bogus valid-looking output.

I agree that this is a problem…

> For example, for a range with (.start == .end == 10), it wrote "10-11".
> This was very confusing when seen in error messages and debugging.

… and not just because it gets in the way of debugging, but due to the
more general "In the face of ambiguity, refuse the temptation to guess" …

> Instead, print a diagnostic output in the form "[empty-range@10*]".

… but I'm not sure about this part.  This stringification will be used
not only by error messages but also by svn_mergeinfo_to_string(), and
I suspect (but haven't confirmed) it could find its way into versioned
data.  Is this the desired behaviour?  Would it be better for the public
API's error out on invalid data, rather than propagate it (and hope
whoever's getting the data will parse it immediately, lest the error
remain dormant)?  We could still have a way to stringify even invalid
data, for debugging purposes.

> This output is not intended be accepted as input by the parsing
> functions.

HTH,

Daniel

Re: svn commit: r1871916 - /subversion/trunk/subversion/svn/log-cmd.c

[Daniel Shahaf]

danie...@apache.org wrote on Mon, 23 Dec 2019 06:50 +00:00:
> 'svn log': Make the --quiet and --diff options not mutually exclusive.
> 
> +++ subversion/trunk/subversion/svn/log-cmd.c Mon Dec 23 06:50:26 2019
> @@ -733,10 +733,6 @@ svn_cl__log(apr_getopt_t *os,
> -  if (opt_state->quiet && opt_state->show_diff)
> -return svn_error_create(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
> -_("'quiet' and 'diff' options are "
> -  "mutually exclusive"));

If anyone knows of a reason to keep --quiet and --diff mutually
exclusive, please speak up.  (On users@ this change was determined to be
a good idea, but asking here, too, in case we missed something.)

Cheers,

Daniel

Re: svn commit: r1870217 - /subversion/trunk/tools/server-side/svn-backup-dumps.py

[Daniel Shahaf]

futat...@apache.org wrote on Sat, Nov 23, 2019 at 10:08:30 -:
> +++ subversion/trunk/tools/server-side/svn-backup-dumps.py Sat Nov 23 
> 10:08:30 2019
> @@ -402,7 +402,8 @@ class SvnBackup:
> -sys.stdout.write("%s " % buf.decode('utf-8'))
> +sys.stdout.write(buf.decode(sys.stdout.encoding,
> +'backslashrreplace'))

There seems to be a typo: s/backslashrreplace/backslashreplace/

Cheers,

Daniel

Re: svn commit: r1870062 - /subversion/site/publish/docs/release-notes/1.14.html

[Daniel Shahaf]

Nathan Hartman wrote on Fri, 22 Nov 2019 06:08 +00:00:
> Where should these point?
> 
> Perhaps to:
> https://svn.apache.org/viewvc/subversion/trunk/INSTALL?revision=HEAD
> 
> with a TODO reminding us to change 'trunk' to 'branches/1.14.x'?

The link need only be valid when the first 1.14.0-* prerelease is
announced (to announce@, not just to dev@); until then, 1.14.html is
work in progress.  1.A.x branches are created after alphas but before
betas. It's not likely that we'll do a 1.14.0-alpha1, and furthermore,
I'd rather risk a broken link in an alpha release than in a beta or RC
release. Therefore, I'd err on the side of making the link point to
branches/1.14.x _now_ with a TODO reminding us to point the link to
trunk if we do an alpha release…

… but in the great scheme of things, this is a pretty minor issue, and
there's more than one way to peel an orange.  Whoever writes the patch
will get to decide.

Cheers,

Daniel

Re: svn commit: r1870062 - /subversion/site/publish/docs/release-notes/1.14.html

[Daniel Shahaf]

hartmannat...@apache.org wrote on Thu, 21 Nov 2019 04:23 +00:00:
> +The convenience script that downloads Subversion's minimal
> +build-time dependencies, get-deps.sh, has been updated to download
> +py3c. This script is found in the source distribution's root
> +directory. For the full list of Subversion's dependencies, see the
> +INSTALL file in the same directory.

Suggest to link to the INSTALL file.

I'm not sure whether "INSTALL" and "get-deps.sh" should be set in ,
being filenames.  In svnbook they would be, but I'm not sure what our
house style for release notes is.

Cheers,

Daniel

Re: svn commit: r1869982 - in /subversion/trunk/tools/dist: release-lines.yaml release.py

[Daniel Shahaf]

julianf...@apache.org wrote on Mon, 18 Nov 2019 17:00 +00:00:
> +++ subversion/trunk/tools/dist/release.py Mon Nov 18 17:00:16 2019
> @@ -70,43 +71,22 @@ except ImportError:
> +# Read the dist metadata (about release lines)
> +with open(get_dist_metadata_file_path(), 'r') as stream:
> +dist_metadata = yaml.load(stream)

yaml.load() is/was unsafe:

https://github.com/yaml/pyyaml/wiki/PyYAML-yaml.load%28input%29-Deprecation

yaml.safe_load() should be used instead.



Separately, at the risk of bikeshedding, I'd suggest to use json, for
two reasons:

- It's part of the Python stdlib.

- jq(1) exists.

(Yes, I'm happy to make the change myself if needed.)

Cheers,

Daniel

Re: svn commit: r1869981 - /subversion/trunk/tools/dist/release.py

[Daniel Shahaf]

julianf...@apache.org wrote on Mon, 18 Nov 2019 16:31 +00:00:
> Author: julianfoad
> Date: Mon Nov 18 16:31:45 2019
> New Revision: 1869981
> 
> URL: http://svn.apache.org/viewvc?rev=1869981=rev
> Log:
> * tools/dist/release.py (recommended_release): Remove TODO: didn't make sense.

The point of that TODO is that instead of hardcoding
«recommended_release» in the source code, we can derive it from the version 
number being
rolled and the version numbers currently available for download.  This way, we 
won't have to
manually update «recommended_release» as part of the release process.

> +++ subversion/trunk/tools/dist/release.py Mon Nov 18 16:31:45 2019
> @@ -102,7 +102,6 @@ tool_versions['1.13'] = tool_versions['1
>  # The version that is our current recommended release
> -# ### TODO: derive this from svn_version.h; see ../../build/getversion.py
>  recommended_release = '1.13'
>  # For clean-dist, a whitelist of artifacts to keep, by version.
>  supported_release_lines = frozenset({"1.9", "1.10", "1.13"})

Re: svn commit: r1869945 - /subversion/site/staging/docs/release-notes/1.14.html

[Daniel Shahaf]

danie...@apache.org wrote on Sun, 17 Nov 2019 13:53 +00:00:
> +++ subversion/site/staging/docs/release-notes/1.14.html Sun Nov 17 13:53:41 
> 2019
> @@ -269,8 +269,8 @@ Welcome below.
> -
> -Python is Optional.
> +
> +Python is Optional.  href="#python-is-optional">

So, the link works, but when clicking it, the yellow background is
replaced with dark grey, due to the usual logic for links to anchors[1].
Should we consider this a bug or a feature?

Personally, I find box easier to read when it isn't wearing a high-
visibility jacket, so to speak.

Cheers,

Daniel

[1] This:
.
   #site-content :target {
 background-color: #e8e8e8;
   }

Re: svn commit: r1869870 - /subversion/trunk/INSTALL

[Daniel Shahaf]

hartmannat...@apache.org wrote on Fri, Nov 15, 2019 at 16:40:22 -:
> +++ subversion/trunk/INSTALL Fri Nov 15 16:40:22 2019
> @@ -344,17 +344,21 @@ I.INTRODUCTION
> +  7.  Berkeley DB 4.X  (DEPRECATED and OPTIONAL)
⋮
> +  The BDB back end has been deprecated in favor of FSFS, which
> +  stores the repository in a flat filesystem.  You do not need 
> +  Berkeley DB if you will only use the FSFS repository filesystem,
> +  or if you are building a Subversion client that will only speak 
> +  to remote (networked) repositories.

Could "remote (networked) repositories" be misunderstood as referring to
NFS-mounted repositories?  Suggest to append "via the svn:// or http:// URI
schemes and their variants" (svn+ssh, svn+foo, https).

Re: svn commit: r1869868 - /subversion/trunk/INSTALL

[Daniel Shahaf]

hartmannat...@apache.org wrote on Fri, Nov 15, 2019 at 16:21:00 -:
> +++ subversion/trunk/INSTALL Fri Nov 15 16:20:59 2019
> @@ -120,17 +120,17 @@ I.INTRODUCTION
>  assembler modules of OpenSSL.  As of OpenSSL 1.1.0 NASM is the
>  only supported assembler.
>  
> +  * Berkeley DB (DEPRECATED and OPTIONAL for client and server)
>  
> + When you create a repository, you have the option of
> + specifying a storage 'back-end' implementation.  Currently,
> + there are two options.  The newer and recommended one, known

There are three options: bdb, fsfs, fsx.  (This error predates your change.)

Cheers,

Daniel

> + as FSFS, does not require Berkeley DB.  FSFS stores data in a
> + flat filesystem.  The older implementation, known as BDB, has
> + been deprecated and is not recommend, but is still available.
> + BDB stores data in a Berkeley DB database.  This back-end
> + will only be available if the BDB libraries are discovered at
> + compile time.
>  
>* libsasl (OPTIONAL for client and server)
>  
> 
> 

Re: svn commit: r1869856 - /subversion/site/staging/docs/release-notes/1.14.html

[Daniel Shahaf]

hartmannat...@apache.org wrote on Fri, Nov 15, 2019 at 14:52:50 -:
> Author: hartmannathan
> Date: Fri Nov 15 14:52:50 2019
> New Revision: 1869856
> 
> URL: http://svn.apache.org/viewvc?rev=1869856=rev
> Log:
> In 'staging': 1.14 release notes: Explain when/how Python is required
> 
> * docs/release-notes/1.14.html:
>   ("Python is Optional" note box):
> Explain in detail when Subversion requires Python, and also when
> it does not, replacing short and vague paragraph.

Thanks for HTMLifying and committing that.  More below.

> +++ subversion/site/staging/docs/release-notes/1.14.html Fri Nov 15 14:52:50 
> 2019
> @@ -272,10 +272,49 @@ Welcome below.
>  
>  Python is Optional.
>  
> -Note that Subversion does not require Python for its basic
> -operation. If you are not using Subversion's SWIG Python bindings,
> -Subversion's test suite, or other Python-coded tools that ship with
> -Subversion, this change does not affect you.
> +Subversion does not require Python for its basic
> +operation. Python is only required for building Subversion and for
> +using Subversion's SWIG Python bindings or hook scripts coded in
> +Python.

This sentence is inaccurate: hook scripts written in Python that don't require
the bindings will not be an issue.  Suggestion: say "for building Subversion
and for using the SWIG Python bindings", then move the "The Python bindings are
used by:" bit to after this paragraph.

> If you do not do either of these things, then this change
> +does not affect you.
> +
> +In more detail, Python is required for doing any of the
> +following:
> +
> +
⋮
> +
> +
> +The Python bindings are used by:
> +
> +
> +Third-party programs (e.g.,
> +https://github.com/viewvc/viewvc/;>ViewVC)
> +Scripts distributed with Subversion itself in the tools/
> +subdirectory.
> +Any in-house scripts you may have.
> +

Cheers,

Daniel

Re: svn commit: r1869851 - in /subversion/trunk/subversion: include/svn_client.h libsvn_client/revert.c

[Daniel Shahaf]

julianf...@apache.org wrote on Fri, Nov 15, 2019 at 12:34:58 -:
> +++ subversion/trunk/subversion/include/svn_client.h Fri Nov 15 12:34:57 2019
> @@ -4479,6 +4479,9 @@ svn_client_relocate(const char *dir,
>   * removed from the working copy. Otherwise, all items are reverted and
>   * their on-disk state changed to match.
>   *
> + * Consult @a ctx to determine whether or not to revert timestamp to the
> + * time of last commit ('use-commit-times = yes').

Say «#SVN_CONFIG_OPTION_USE_COMMIT_TIMES» somewhere in there?

Re: svn commit: r1869849 - /subversion/site/staging/docs/release-notes/1.14.html

[Daniel Shahaf]

julianf...@apache.org wrote on Fri, Nov 15, 2019 at 11:46:31 -:
> +++ subversion/site/staging/docs/release-notes/1.14.html Fri Nov 15 11:46:31 
> 2019
> @@ -219,7 +219,7 @@ users.  We'll cover those in this sectio
> -Subversion's SWIG Python bindings and automated test suite now
> +Subversion's SWIG Python bindings and Subversion's test suite now
>  support Python 3.x (and newer).

That will be true in 1.13.1 too.  Would it be worthwhile to point this out?
Probably not in 1.14.html but in the "Known bugs" section of 1.13.html?

> @@ -245,9 +246,8 @@ Welcome below.
> +As of 1 January 2020, https://www.python.org/dev/peps/pep-0373/;>
> +Python 2.7 has reached end of life. All users are strongly encouraged

Please change "> Python" to ">Python" with no intervening whitespace; the
whitespace is noticeable in the rendered HTML.  (Or, at least, it _was_ 
noticeable
when I last tested this.)  Thus:

example

Cheers,

Daniel

Re: svn commit: r1869776 - /subversion/site/staging/docs/release-notes/1.14.html

[Daniel Shahaf]

hartmannat...@apache.org wrote on Thu, 14 Nov 2019 05:26 +00:00:
> +
> +Python is Optional.
> +
> +Note that Subversion does not require Python for its basic
> +operation. If you are not using Subversion's SWIG Python bindings,
> +automated test suite, or other Python-coded tools that ship with
> +Subversion, this change does not affect you.

Python is required when building from a tag or branch, but not required
when building from a tarball.  Should this detail be added?

> + 
> +
> +  
> +
>
>  
>  

Re: svn commit: r1869118 - /subversion/trunk/tools/dist/release.py

[Daniel Shahaf]

julianf...@apache.org wrote on Tue, 29 Oct 2019 17:22 +00:00:
> +++ subversion/trunk/tools/dist/release.py Tue Oct 29 17:22:40 2019
> @@ -1188,8 +1188,9 @@ def move_to_dist(args):
>  def write_news(args):
>  'Write text for the Subversion website.'
> -data = { 'date' : datetime.date.today().strftime('%Y%m%d'),
> - 'date_pres' : datetime.date.today().strftime('%Y-%m-%d'),
> +release_date = args.news_release_date or 
> datetime.date.today().strftime('%Y-%m-%d')
> +data = { 'date' : re.sub("-", "", release_date),  # format mmdd

You don't do any input validation on the date in argv anywhere, so
--news-release-date=foo-bar would be accepted here, as would
--news-release-date=2020-1-1 without leading zeroes.

I assume the lack of leading zeroes would cause problems at some point
down the road (for example, if we ever try to use the 'date' or
'date_pres' replaceables in a context that actually parses them as
date strings, such as the ?update= parameter to download.cgi).

I suggest instead:

release_date = time.strptime(args.news_release_date, "-mm-dd") if 
args.news_release_date else datetime.date.today()
… { 'date': release_date.strftime("mmdd"),
'date_pres': release_date.strftime(…) }

Cheers,

Daniel

> + 'date_pres' : release_date,  # format  -mm-dd
>   'major-minor' : args.version.branch,
>   'version' : str(args.version),
>   'version_base' : args.version.base,
> @@ -1796,6 +1797,9 @@ def main():
>  subparser.set_defaults(func=write_news)
>  subparser.add_argument('--announcement-url',
>  help='''The URL to the archived announcement email.''')
> +subparser.add_argument('--news-release-date',
> +help='''The release date for the news, as -MM-DD.
> +Default: today.''')
>  subparser.add_argument('--edit-html-file',
>  help='''Insert the text into this file
>  news.html, index.html).''')
> 
> 
>

Re: svn commit: r1868561 - /subversion/trunk/subversion/tests/cmdline/diff_tests.py

[Daniel Shahaf]

Good morning Nathan,

hartmannat...@apache.org wrote on Thu, Oct 17, 2019 at 16:23:21 -:
> +++ subversion/trunk/subversion/tests/cmdline/diff_tests.py Thu Oct 17 
> 16:23:21 2019
> @@ -5253,6 +5253,83 @@ def diff_git_format_copy(sbox):
> +def diff_nonexistent_in_wc(sbox):
> +  "nonexistent in working copy"
> +
> +  sbox.build(empty=True)
> +  wc_dir = sbox.wc_dir
> +
> +  # We mirror the actions of the reproduction script (with one exception:
> +  # we 'svn up -r 0' instead of checking out a second working copy)
> +
> +  sbox.simple_add_text('test\n', 'file')
> +  sbox.simple_commit()

If instead of using «echo test > file» you would use the existing file 'iota'
(see subversion/tests/README), you'd be able to pass read_only=True to
sbox.build(), which avoids some test suite overhead in setting a dedicated
repository for each test function — i.e., makes tests run half a millisecond 
faster. ☺

Cheers,

Daniel

Re: svn commit: r1867639 - /subversion/site/publish/docs/community-guide/releasing.part.html

[Daniel Shahaf]

julianf...@apache.org wrote on Fri, Sep 27, 2019 at 15:36:07 -:
> +++ subversion/site/publish/docs/community-guide/releasing.part.html Fri Sep 
> 27 15:36:07 2019
> @@ -1494,22 +1494,28 @@ A.B with the version you're preparing, e
> +The following steps are not automated:
> +
> +
>  Ask someone with appropriate access to add the A.B.x branch to the
> svn-role backport mergebot:
> +Someone with admin access to the svn-qavm machine needs to
> +populate the a source directory for the new branch, by running,
> +sudo -u svnsvn  svn up ~svnsvn/src/svn/A.B.x

Suggest to pass -H to sudo(8).  It's not important in this case (nothing will
write to the dotfiles), but it's a good habit.

> +The backport merge bot runs nightly on each such branch directory
> +that exists.
> +To remove a no-longer-supported branch:
> +sudo -u svnsvn  svn up -r0 ~svnsvn/src/svn/Z.Z.x

Why not just 'rm -rf'?  As written, the cron job will just update back to HEAD
on its next run…

> +[Obsolete note?]
>  The exact checkout command is documented in machines/svn-qavm2/notes.txt
>  in the private repository (need to use a trunk client and the 
> svn-master.a.o
>  hostname).

The part about a 'trunk client' is obsolete.  The VM used to have
a /usr/local/svn-current/bin/svn client newer than the /usr/bin/svn.  I don't
remember why we started that (whether it was dogfooding out of general
principles or some particular 'svn merge' bugfix we needed), but we don't do
that any more.

The part about using svn-master.a.o probably does matter, though.  The backports
merger runs «svn ci && svn up» in a loop, and assumes «svn up» will update it 
to the
revision it just committed.  That's not guaranteed when updating from the EU 
mirror
(though few big merges get committed at 4am UTC, when the cron job runs).  The 
buildbot
slaves do this too, for the same reason.

Cheers,

Daniel

Re: svn commit: r1867063 - /subversion/site/staging/docs/community-guide/releasing.part.html

[Daniel Shahaf]

julianf...@apache.org wrote on Tue, 17 Sep 2019 14:49 +00:00:
> +++ subversion/site/staging/docs/community-guide/releasing.part.html Tue Sep 
> 17 14:49:51 2019
> @@ -687,7 +687,7 @@ it.
>  
>  For a non-LTS ("regular") release line
>  
> -A change is approved if it receives one +1 vote and no vetoes.  (Only
> +A change is approved if it receives two +1s and no vetoes.  (Only
>  binding votes count; see above.)
>  
>  For an LTS release line

My reading of consensus is that we changed "three +1s" to "two +1s" for non-LTS
lines, but didn't _increase_ the bar in any way.

However, as written, this change could be taken as saying that the rule
of "one +1 and one +0" does not apply to bindings changes to non-LTS
lines.  (It's not clear whether the "preceded by a bold paragraph" unary
operator has a higher or lower precedence than the binary "paragraph
juxtaposition" operator.)  Clarify, please?

Cheers,

Daniel

Re: svn commit: r1865987 - /subversion/trunk/subversion/libsvn_fs_fs/verify.c

[Daniel Shahaf]

br...@apache.org wrote on Tue, 27 Aug 2019 12:10 +00:00:
> +++ subversion/trunk/subversion/libsvn_fs_fs/verify.c Tue Aug 27 12:10:43 2019
> @@ -694,8 +694,9 @@ compare_p2l_to_rev(svn_fs_t *fs,
>  return svn_error_createf(SVN_ERR_FS_INDEX_CORRUPTION,
>   NULL,
>   _("p2l index entry for changes in"
> -   " revision r%ld is item %ld of type"
> -   " %d at offset %s"),
> +   " revision r%ld is item"
> +   " %"APR_UINT64_T_FMT
> +   " of type %d at offset %s"),

Preexisting problem: entry->type is an apr_uint32_t but formatted as %d.

And with that fixed, backport?

>   entry->item.revision,
>   entry->item.number,
>   entry->type,
> 
> 
>

Re: svn commit: r1865841 - in /subversion/trunk/subversion: libsvn_wc/diff_local.c tests/cmdline/changelist_tests.py

[Daniel Shahaf]

julianf...@apache.org wrote on Sat, 24 Aug 2019 10:50 +00:00:
> Author: julianfoad
> Date: Sat Aug 24 10:50:44 2019
> New Revision: 1865841
> 
> URL: http://svn.apache.org/viewvc?rev=1865841=rev
> Log:
> Fix issue #4822, "svn diff --changelist ARG" broken in subdirectories.

Should this be backported?

Re: svn commit: r1865929 - /subversion/trunk/subversion/tests/cmdline/changelist_tests.py

[Daniel Shahaf]

julianf...@apache.org wrote on Mon, 26 Aug 2019 11:36 +00:00:
> Add a test for issue #4826, diff repos-wc non-infinite depth uses wrong depth.
> 
> +++ subversion/trunk/subversion/tests/cmdline/changelist_tests.py Mon Aug 26 
> 11:35:55 2019
> @@ -553,31 +580,20 @@ def diff_with_changelists(sbox):
> +  ### XFAIL case
> +  if is_repos_wc and (depth == 'empty' or depth == 'files' or 
> depth == 'immediates') and subdir != '.':
> +print("XFAIL case: depth=%s, subdir='%s'\nexpect=%s\nactual=%s" 
> % (depth, subdir,
> +  str(expected_paths), str(paths)))
> +continue

Is this something temporary while you work on this, or meant to remain in the 
code longer-term?

(If the former, ignore the rest of this email.)

IIRC, tests aren't supposed to print() at all, certainly not from within
individual test functions.  Instead, they should log at the following levels:

warning: test failure
error: bug in the test harness

So, if this print() is to remain in the code, I guess it should become a 
log.debug() call?

Cheers,

Daniel

Re: svn commit: r1865522 - /subversion/trunk/subversion/libsvn_subr/sqlite3wrapper.c

[Daniel Shahaf]

kot...@apache.org wrote on Tue, 20 Aug 2019 09:18 +00:00:
> When compiling SQLite, set the SQLITE_DEFAULT_MEMSTATUS=0 compile-time option.
> 
> This is the recommended option that is not enabled by default. Setting it to
> zero avoids using a mutex (and thus suffering a performance penalty) during
> every sqlite3_malloc() call, where this mutex is used to properly update the
> allocations stats.  We don't use sqlite3_status(), so set this option to zero.
> 
> See https://sqlite.org/compile.html#recommended_compile_time_options

Would it break anything if some application that uses libsvn had
its own copy of SQLite that was compiled with another SQLITE_DEFAULT_MEMSTATUS
value?  I don't see anything about that in the linked page.

Cheers,

Daniel

Re: svn commit: r1865523 - /subversion/trunk/subversion/libsvn_subr/sqlite3wrapper.c

[Daniel Shahaf]

kot...@apache.org wrote on Tue, 20 Aug 2019 09:23 +00:00:
> +++ subversion/trunk/subversion/libsvn_subr/sqlite3wrapper.c Tue Aug 20 
> 09:23:55 2019
> @@ -26,6 +26,7 @@
>  #ifdef SVN_SQLITE_INLINE
>  #  define SQLITE_OMIT_DEPRECATED 1
>  #  define SQLITE_DEFAULT_MEMSTATUS 0
> +#  define SQLITE_OMIT_WAL 1

Quoting https://sqlite.org/compile.html#_options_to_omit_features:

 Important Note: The SQLITE_OMIT_* options may not work with
 the amalgamation. SQLITE_OMIT_* compile-time options usually
 work correctly only when SQLite is built from canonical
 source files.

and a bit later:

 Also, not all SQLITE_OMIT_* options are tested. Some
 SQLITE_OMIT_* options might cause SQLite to malfunction and/or
 provide incorrect answers.

I think we should:

- Revert r1865523
  (feel free to add it to INSTALL, notes/knobs, etc; but off by default, please)

- Stop defining SQLITE_OMIT_DEPRECATED

- Backport the latter change to stable branches.

To the latter two bullet points: I'm aware that
https://sqlite.org/compile.html#recommended_compile_time_options
recommends SQLITE_OMIT_DEPRECATED, but
https://sqlite.org/compile.html#_options_to_omit_features says using
that macro may break correctness (!), and I'm voting to err on the side
of caution.

Thanks for linking the upstream documentation in the log message!

Cheers,

Daniel


>  #  define SQLITE_API static
>  #  if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 2)
>  #pragma GCC diagnostic ignored "-Wunreachable-code"
> 
> 
>

Re: svn commit: r1864300 - /subversion/site/publish/upcoming.part.html

[Daniel Shahaf]

svn-r...@apache.org wrote on Sat, 03 Aug 2019 04:15 +00:00:
> * upcoming.part.html: Automatically regenerated
> 
> +++ subversion/site/publish/upcoming.part.html Sat Aug  3 04:15:02 2019
> @@ -2,113 +2,6 @@
>  
>  Changes in ^/subversion/branches/1.12.x:
>  
> 
> -https://svn.apache.org/r1860377;>r1860377 | svn-role | 
> 2019-05-30 04:00:08 + (Thu, 30 May 2019) | 11 lines
...
> -
>  
>  
>  Further changes currently under consideration are listed in each release 
> line's 

This removed all the lines that the previous commit had added, except
for the last diff context line.

This is because the behaviour of upcoming.py depends on whether or not
its cwd is the root of a stable branch source tree.  The previous
commit, r1864265, used ~/src/svn/site/ as the source tree; this commit
used ~/src/svn/1.12.x/ as the source tree.  The point where the two
runs diverged would be tools/upcoming.py:get_reference_version().

Either way, the next time anything is merged to 1.12.x, upcoming.part.html
should be auto-updated.

Cheers,

Daniel

Re: svn commit: r1864265 - /subversion/site/publish/upcoming.part.html

[Daniel Shahaf]

svn-r...@apache.org wrote on Fri, 02 Aug 2019 18:11 +00:00:
> * upcoming.part.html: Automatically regenerated
> 
> +++ subversion/site/publish/upcoming.part.html Fri Aug  2 18:11:34 2019
> @@ -1,3 +1,112 @@

It added lines because it lists the changes since 1.12.0.  (See r1864256
for the rationale for that.)

Re: svn commit: r1855211 - /subversion/site/publish/docs/community-guide/releasing.part.html

[Daniel Shahaf]

julianf...@apache.org wrote on Mon, 11 Mar 2019 11:35 +00:00:
> Author: julianfoad
> Date: Mon Mar 11 11:35:43 2019
> New Revision: 1855211
> 
> URL: http://svn.apache.org/viewvc?rev=1855211=rev
> Log:
> * publish/docs/community-guide/releasing.part.html:
>   Don't use reporter.apache.org for pre-releases.

For what it's worth:

reporter.a.o is AFAIK only used for drafting Board reports; I would
expect the Board to care equally much about any release regardless of
its alpha/beta/RC/GA status; therefore, I would consider release
of an RC to be as worthy of mention in the quarterly report as any
GA release.

Cheers,

Daniel

Re: svn commit: r1850437 - /subversion/site/staging/docs/release-notes/index.html

[Daniel Shahaf]

julianf...@apache.org wrote on Fri, Jan 04, 2019 at 21:14:43 -:
> +++ subversion/site/staging/docs/release-notes/index.html Fri Jan  4 21:14:43 
> 2019
> @@ -77,9 +77,15 @@ official support status for the various
> +These are the changes that have been merged to the 1.11.x branch since 
> the last 1.11.x patch release.
>  

On the day after 1.11.1 is released, no changes will be listed; there
will only be a single line of dashes.  I think that might make the page
slightly confusing, but I'm not sure how to improve it.

Ideas?

Cheers,

Daniel

> +These changes are almost certainly going to be in the next 1.11.x patch 
> release; any of them may be reverted before the release is made, but 
> they probably won't be. More changes besides these may be added.
> +
> +Further changes currently nominated for backport are listed in  href="http://svn.apache.org/repos/asf/subversion/branches/1.11.x/STATUS;>1.11.x/STATUS
>  along with their current voting status. These may get in to the next release 
> if they reach the 'Approved' category in time.
> +
> +See also  href="http://svn.apache.org/repos/asf/subversion/trunk/CHANGES;>trunk/CHANGES,
>  where all significant-enough changes will summarized for each version by the 
> time it is released.
> +
> +Different lists of changes are queued for older supported release lines 
> (1.10.x, 1.9.x), not shown here.
>  
>  

Re: svn commit: r1850737 - /subversion/site/tools/upcoming.py

[Daniel Shahaf]

danie...@apache.org wrote on Tue, 08 Jan 2019 12:03 +:
> Log:
> * tools/upcoming.py
>   (copyfrom_revision_of_previous_tag_of_this_stable_branch):
> Determine the latest release by checking dist/release/, not by checking
> tags, in order to also show changes that have been merged into a patch
> release that has not been announced yet.
> 
> This change will cause tonight's bot run to effectively revert r1850708 (and
> add Monday's merge to the output as well), re-adding all the merges that
> went into 1.11.1 to the Web site — which is correct, since 1.11.1 has not yet
> been published.

There is still a minor race condition here: when 1.11.1 artifacts are
moved to dist/release/, the Website will stop showing merges that
went into 1.11.1 even before the download page and mirrors are
updated to 1.11.1.

I suppose we can live with that, but if anyone has ideas for improving
the situation...

Cheers,

Daniel