Re: svnwcsub.py does not work across different SVN trees
sebb wrote on Thu, 13 May 2021 14:46 +00:00: > On Thu, 13 May 2021 at 15:03, Daniel Shahaf wrote: > > > > sebb wrote on Wed, May 12, 2021 at 10:47:15 +0100: > > > It looks like svnwcsub.py is not always able to update the workspace > > > > Don't use made-up terminology. The terms "repository" and "working > > copy" have specific meanings. Use those terms to refer to what they > > mean. Don't use those terms to mean other things and don't use other > > terms to mean those things. > > > > > if the source URL is updated in the configuration file. > > > > > > I am seeing an error of the form: > > > > > > svn: E195012: Path '/www/ace.apache.org' does not share common version > > > control ancestry with the requested switch location. Use > > > --ignore-ancestry to disable this check. > > > svn: E195012: 'https://svn-master.apache.org/repos/infra/sites/ace' > > > shares no common ancestry with '/www/ace.apache.org > > > > > > Is there any reason why the svn switch command does not include the > > > suggested option? > > > > You mean, why svnwcsub.py doesn't pass that option? For starters, > > because svnwcsub.py was written before that option existed. > > Any other reasons? > If you want to propose that that flag be added, the onus is on you to make the case for adding it, not on everyone else for making the case against adding it. You've been around long enough to know this. > > > Seems to me it ought to be possible to replace the checkout with a > > > different SVN repo without needing to manually remove the existing > > > checkout. > > > > -1. That would be data loss in case of a new svnwcsub deployment where > > the target dir already exists and is non-empty. This project is > > generally rather averse to the possibility of data loss. > > This is not about a new deployment, but about changing the URL for an > existing WC. How would the code tell the two cases apart? What are the proposed new semantics?
Re: svnwcsub.py does not work across different SVN trees
On Thu, 13 May 2021 at 15:03, Daniel Shahaf wrote: > > sebb wrote on Wed, May 12, 2021 at 10:47:15 +0100: > > It looks like svnwcsub.py is not always able to update the workspace > > Don't use made-up terminology. The terms "repository" and "working > copy" have specific meanings. Use those terms to refer to what they > mean. Don't use those terms to mean other things and don't use other > terms to mean those things. > > > if the source URL is updated in the configuration file. > > > > I am seeing an error of the form: > > > > svn: E195012: Path '/www/ace.apache.org' does not share common version > > control ancestry with the requested switch location. Use > > --ignore-ancestry to disable this check. > > svn: E195012: 'https://svn-master.apache.org/repos/infra/sites/ace' > > shares no common ancestry with '/www/ace.apache.org > > > > Is there any reason why the svn switch command does not include the > > suggested option? > > You mean, why svnwcsub.py doesn't pass that option? For starters, > because svnwcsub.py was written before that option existed. Any other reasons? > > Seems to me it ought to be possible to replace the checkout with a > > different SVN repo without needing to manually remove the existing > > checkout. > > -1. That would be data loss in case of a new svnwcsub deployment where > the target dir already exists and is non-empty. This project is > generally rather averse to the possibility of data loss. This is not about a new deployment, but about changing the URL for an existing WC.
Re: svnpubsub/commit-hook.py:65 - variable url assigned but not used
Furthermore, the method does not actually return a URL. The method behaves like the following: https://python.readthedocs.io/en/v2.7.2/library/urllib2.html#urllib2.urlopen On Thu, 13 May 2021 at 15:16, sebb wrote: > > The problem is that it is not clear whether the variable is supposed > to be used or not. > Is it misspelt? > > AIUI the convention for intentionally unused variables is to prefix > the name with an underscore, as this no longer triggers the warning in > PyLint > > On Thu, 13 May 2021 at 15:11, Daniel Shahaf wrote: > > > > sebb wrote on Wed, May 12, 2021 at 11:49:41 +0100: > > > As the subject says > > > > Assuming opener.open() actually returns a URL, I don't see the problem > > here. The variable documents the return type for anyone who may want > > to extend the function.
Re: svnwcsub.py - various pylint errors
sebb wrote on Wed, May 12, 2021 at 12:25:26 +0100: > pylint reports quite a few warnings and errors: > > svnwcsub.py:55:0: W0311: Bad indentation. Found 2 spaces, expected 4 > (bad-indentation) Don't hard-wrap program output in your emails. > svnwcsub.py:57:0: W0311: Bad indentation. Found 2 spaces, expected 4 > (bad-indentation) > svnwcsub.py:61:0: W0311: Bad indentation. Found 2 spaces, expected 4 > (bad-indentation) > svnwcsub.py:63:0: W0311: Bad indentation. Found 2 spaces, expected 4 > (bad-indentation) > svnwcsub.py:67:0: W0311: Bad indentation. Found 2 spaces, expected 4 > (bad-indentation) > svnwcsub.py:69:0: W0311: Bad indentation. Found 2 spaces, expected 4 > (bad-indentation) I know it's PEP8, but the churn isn't worthwhile, IMO, especially as the entire core tests directory uses non-PEP8 indentation. Thus, I'm voting WONTFIX. editorconfig(5) is already set correctly for these. > svnwcsub.py:87:4: W0612: Unused variable 'output' (unused-variable) I suppose you could change it to «_, errput = …», but it's rather a borderline case insofar as readability v. churn is concerned, IMO. > svnwcsub.py:115:12: W0612: Unused variable 'x' (unused-variable) How'd you rewrite it? > svnwcsub.py:439:38: W0613: Unused argument 'event_arg' (unused-argument) Looks like a false positive. Did you triage these warnings before you posted them to the list? > svnwcsub.py:46:0: W0611: Unused import errno (unused-import) > svnwcsub.py:58:0: W0611: Unused import time (unused-import) > svnwcsub.py:65:0: W0611: Unused import functools (unused-import) > svnwcsub.py:67:2: W0611: Unused import urlparse (unused-import) > svnwcsub.py:69:2: W0611: Unused urllib.parse imported as urlparse > (unused-import) Patch please. > svnwcsub.py:378:18: E0602: Undefined variable 'fname' (undefined-variable) > (should be self.fname) Did you reproduce the bug and confirm the proposed change fixes it? If so, post a complete patch, please.
Re: svnpubsub/commit-hook.py:65 - variable url assigned but not used
The problem is that it is not clear whether the variable is supposed to be used or not. Is it misspelt? AIUI the convention for intentionally unused variables is to prefix the name with an underscore, as this no longer triggers the warning in PyLint On Thu, 13 May 2021 at 15:11, Daniel Shahaf wrote: > > sebb wrote on Wed, May 12, 2021 at 11:49:41 +0100: > > As the subject says > > Assuming opener.open() actually returns a URL, I don't see the problem > here. The variable documents the return type for anyone who may want > to extend the function.
Re: svnpubsub/commit-hook.py:65 - variable url assigned but not used
sebb wrote on Wed, May 12, 2021 at 11:49:41 +0100: > As the subject says Assuming opener.open() actually returns a URL, I don't see the problem here. The variable documents the return type for anyone who may want to extend the function.
Re: watcher.py no longer uses urlparse
sebb wrote on Wed, May 12, 2021 at 11:48:17 +0100: > As the subject says. +1 to commit, but the log message should follow the house style, for example: [[[ * tools/server-side/svnpubsub/watcher.py (urlparse): Remove unused import. Approved by: danielsh ]]]
Re: svnwcsub.py does not work across different SVN trees
sebb wrote on Wed, May 12, 2021 at 10:47:15 +0100: > It looks like svnwcsub.py is not always able to update the workspace Don't use made-up terminology. The terms "repository" and "working copy" have specific meanings. Use those terms to refer to what they mean. Don't use those terms to mean other things and don't use other terms to mean those things. > if the source URL is updated in the configuration file. > > I am seeing an error of the form: > > svn: E195012: Path '/www/ace.apache.org' does not share common version > control ancestry with the requested switch location. Use > --ignore-ancestry to disable this check. > svn: E195012: 'https://svn-master.apache.org/repos/infra/sites/ace' > shares no common ancestry with '/www/ace.apache.org > > Is there any reason why the svn switch command does not include the > suggested option? You mean, why svnwcsub.py doesn't pass that option? For starters, because svnwcsub.py was written before that option existed. > Seems to me it ought to be possible to replace the checkout with a > different SVN repo without needing to manually remove the existing > checkout. -1. That would be data loss in case of a new svnwcsub deployment where the target dir already exists and is non-empty. This project is generally rather averse to the possibility of data loss.
Re: svnpubsub/svnwcsub.py crashes if pidfile option is not supplied
sebb wrote on Tue, May 11, 2021 at 22:41:38 +0100:
> On Tue, 11 May 2021 at 19:33, Daniel Shahaf wrote:
> >
> > sebb wrote on Tue, 11 May 2021 16:25 +00:00:
> > > As the subject says, the code crashes as below unless --pidfile is
> > > provided
> > >
> > > File "./svnwcsub.py", line 559, in
> > > main(sys.argv[1:])
> > > File "./svnwcsub.py", line 548, in main
> > > d = Daemon('/dev/null', os.path.abspath(options.pidfile),
> > > File
> > > "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/posixpath.py",
> > > line 367, in abspath
> > > if not isabs(path):
> > > File
> > > "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/posixpath.py",
> > > line 54, in isabs
> > > return s.startswith('/')
> > > AttributeError: 'NoneType' object has no attribute 'startswith'
> >
> > Any chance of a patch? Feel free to just make --pidfile required, if
> > making it properly optional is a larger diff than you have tuits for.
Update the usage message, use «svn diff -x-p», root the diff at the
branch root, address it to dev@, include a proposed log message (that
follows our log message syntax and linguistic conventions and is free of
spelling errors); see https://subversion.apache.org/patches for details.
