Coming back to a thread from long time ago, with the intention to commit the script in tools/client-side so we can link this from the FAQ based on a recent question on IRC [1], [2], [3]
[1] https://colabti.org/irclogger/irclogger_log/svn-dev?date=2022-07-01 [2] https://colabti.org/irclogger/irclogger_log/svn-dev?date=2022-07-04 [3] https://colabti.org/irclogger/irclogger_log/svn-dev?date=2022-07-07 More below... Den tis 9 mars 2021 kl 21:09 skrev Daniel Shahaf <d...@daniel.shahaf.name>: > Daniel Sahlberg wrote on Mon, Mar 08, 2021 at 09:15:21 +0100: > > Attaching version 5, with changes as below. > > > > Den sön 7 mars 2021 kl 20:42 skrev Daniel Shahaf <d...@daniel.shahaf.name > >: > > > > > Daniel Sahlberg wrote on Sun, Mar 07, 2021 at 19:34:15 +0100: > > > > + # Parse arguments > > > > + import argparse > > > > + parser = argparse.ArgumentParser( > > > ⋮ > > > > + # The file name is the md5encoding of the realm > > > > + import hashlib > > > > + m = hashlib.md5() > > > > > > Hmm. Should all function-local imports be grouped at the top of the > > > function? I don't remember the convention here. > [... long discussion about coding style with the conclusion that even though PEP-8 says all import should be in the top of the file, CPython's own source code contains function level imports. And it would be more PEP-8-ish to have function level imports at the top of the function ...] I've changed it this way and sorted them alphabetically. > (Offtopic: When reviewing /trunk/doc, I had a look at > > /trunk/doc/programmer/WritingChangeLogs.txt. Is it still relevant? It > seems > > very CVS-ish and mostly covered in HACKING). > > Everything up to the last two section headers is good advice for writing > log messages. For those parts, they should be added to HACKING or > referenced therefrom. > > The remaining two sections don't seem applicable to us at all, but as > the file is "an essay by jimb", I don't think we can change it. > I was more thinking about removing it if it doesn't apply to the Subversion project (anymore). > +++ contrib/client-side/store-plaintext-password.py (working copy) > > @@ -0,0 +1,189 @@ > > +def _read_one_datum(fd, letter): > ⋮ > > + # Read the letter and the space > > + if fd.read(1) != letter or fd.read(1) != b' ': > > + raise ... > > (Raising ellipses is discussed elsethread.) > Now raising a ValueError(). I'm not experienced in Python but it looked like the most suitable (standard) exception class. > > > +def outputHash(fd, hash): > > + """\ > > + Write a dictionary to an FD in the same format as used by > > Say the formal parameter's name somewhere in here? > > Calling it "hash" could be confusing, because svn_hash_write2() and > hashlib use that term in different senses, but I don't have a suggestion > about this. > Renamed the parameter to dict and added the reference in the docstring. > > +def writeHashFile(filename, hash): > > + """\ > > + Open a temporary file and write the dictionary to the temp > > + file in svn_hash_write2() format. > > + > > + If the file is successfully written it is renamed to <filename>. > > Please have all docstrings use the same markup for formal parameter > names. outputHash() above uses the "capitalize the name" convention the > C code uses for non-public-API function. I'll also use that for the > example I write in the next paragraph. > > Describe the contract, not the implementation. For instance, "Write the > dictionary HASH to a file named FILENAME in svn_hash_write2() format." > would be a reasonable first sentence. The docstring could then promise > a postcondition that if the file exists upon return, then it's not > truncated, mention what happens if at entry FILENAME is a regular file > or an unbroken symlink, etc.. > Docstring rewritten. > > > + """ > > + tmpFilename = filename + '.tmp' > > + try: > > + with open(tmpFilename, 'xb') as fd: > > + outputHash(fd, hash) > > + os.rename(tmpFilename, filename) > > + except FileExistsError: > > + print('{}: File {!r} already exist. Is the script already > running?\n'.format(argv[0], tmpFilename), file=sys.stderr) > > s/the/this/ > > Wrap to 80 columns. > done > > > + except: > > + os.remove(tmpFilename) > > + raise > > + > > +def main(): > > + # Parse arguments > > + import argparse > > + parser = argparse.ArgumentParser( > > + description=PARSERDESCR, > > + formatter_class=argparse.RawDescriptionHelpFormatter) > > + parser.add_argument('realm', help='Server authentication real') > > + parser.add_argument('-u', '--user', help='Set username', nargs='?') > > Just delete nargs entirely, for reasons given upthread? Sorry for > missing that in my reviews between then and now. > done > > > + args = parser.parse_args() > > + > > + # The file name is the md5encoding of the realm > > + import hashlib > > + m = hashlib.md5() > > + m.update(args.realm.encode('utf-8')) > > + authfileName = > os.path.join(os.path.expanduser('~/.subversion/auth/svn.simple/'), > m.hexdigest()) > > + > > + # If new file, check that username was given as an argument before > prompting for password > > + existingFile = os.path.exists(authfileName) > > + if not existingFile and args.user is None: > > + print('{}: New file, username required (see > -u)\n'.print(argv[0]), file=sys.stderr) > > The phrase "new file" in the comment isn't clear, probably because it > isn't a full sentence. > > The phrase "New file" in the error message has the same problem, as well > as being phrased in implementation terms rather than in user-facing > terms. > Rephrased, and also added an interactive prompt for the username if missing. > > > + parser.print_help() > > + return > > The exit code should be non-zero whenever an error was printed to > stderr. > > Aren't you reimplementing one of > https://docs.python.org/3/library/argparse.html#exiting-methods > ? > Changed to parser.exit() I took the courage to commit as r1902590 and we can work any changes from there. Kind regards, Daniel
