Daniel Sahlberg wrote on Sun, Mar 07, 2021 at 19:34:15 +0100:
> Den fre 26 feb. 2021 kl 16:11 skrev Daniel Shahaf <d...@daniel.shahaf.name>:
> > Daniel Sahlberg wrote on Fri, Feb 26, 2021 at 13:49:25 +0100:
> > > + with open(filename + '.tmp', 'x', encoding='utf-8') as file:
> >
> > See my previous mail about adding 'w' and ensuring CR isn't emitted on
> > Windows.
> >
>
> Python doesn't like wx combined:
>
> Traceback (most recent call last):
> File "./store-plaintext-password.py", line 112, in writeHashFile
> with open(tmpFilename, 'wxb') as fd:
> ValueError: must have exactly one of create/read/write/append mode
I stand corrected.
> > > +parser.add_argument('realm', help='Server authentication real')
> > > +parser.add_argument('-u', '--user', help='Set username', nargs='?')
> > > +args = parser.parse_args()
> > > +
> > > +# The file name is the md5encoding of the realm
> > > +m = hashlib.md5()
> > > +m.update(args.realm.encode('utf-8'))
> > > +authfileName =
> > > os.path.join(os.path.expanduser('~/.subversion/auth/svn.simple/'),
> > > .hexdigest())
> > > +
> > > +# If new file, check that username was given as an argument before
> > > prompting for password
> > > +if not os.path.isfile(authfileName) and args.user is None:
> >
> > «not isfile()» could mean it's a directory or a broken symlink. How
> > about:
> > .
> > if isfile():
> > elif not exists():
> > else:
> > .
> > ?
> >
>
> I switched to exists() only. I'm not sure it makes sense to care for every
> possible variation - if it is a directory then open() will raise an
> exception and I think that is acceptable in this case.
+1
> To reduce the number of stat() I'm storing the result form exists() in a
> variable and reusing late. As for race condition... well it's not really
> that kind of script where I could bother.
Let's just say that "should X be done" and "could Daniel be bothered to
do X" are not synonymous.
> > (The point below about stat() applies to this pseudocode too.)
> >
> > > + print('New file, username required (see -u)\n', file=sys.stderr)
> >
> > Error messages should start with the argv[0] of whoever generated them.
> >
>
> Point taken, but in this particular case it is displayed by print_help().
Yes, but that's printed later. If this script is invoked by another,
that's then invoked by another that's invoked by some CI, etc.,
_prefixing_ the script's name to the message tends to be helpful.
> > > +# In an existing file, we add/replace password/username/passtype
> > > +if os.path.isfile(authfileName):
> >
> > Calling stat() twice has a performance impact and looks like a race
> > condition. Maybe in this case it doesn't matter, though…
> >
>
> Using the result from the previous check. Race condition, agree..
>
> > > +# For a new file, set realmstring, username, password and passtype
> > > +else:
> > > + hash = {}
> > > + hash['svn:realmstring'] = args.realm
> > > + hash['username'] = args.user
> > > + hash['passtype'] = 'simple'
> > > + hash['password'] = password
> >
> > Style: Use a single, multiline initializer rather than multiple
> > assignments?
> >
>
> Like this?
> hash = {
> b'svn:realmstring': args.realm.encode('utf-8'),
> b'username': args.user.encode('utf-8'),
> b'passtype': 'simple'.encode('utf-8'),
> b'password': password.encode('utf-8')
> }
Yeah, except add a comma on the fourth line to minimize future diffs.
For future reference, there's also a «dict(foo=42)» syntax which is
sometimes convenient.
> > Forward cribbing compatibility: «del password» once it's no longer needed?
> >
>
> Sorry, didn't quite understand this. Do you ask for an option to remove a
> stored password? Isn't svn auth --remove enough?
No, I meant literally add the line of code «del password» once the
variable «password» is no longer needed. That won't actually overwrite
the value's bytes in memory, but it will prevent the variable from being
accidentally used afterwards.
> +++ contrib/client-side/store-plaintext-password.py (working copy)
> @@ -0,0 +1,184 @@
> +def outputHash(fd, hash):
> + """\
> + Write a dictionary to an FD in the same format as used by
> + svn_hash_write2(), ie 'K <length>\\n<key>\\nV <length>\\n<value>\\n'
> + terminated by "END\n".
Escape the newline.
Mention the data type of HASH's keys and values?
> + """
> + assert 'b' in fd.mode
> +
> + for key, val in hash.items():
> + fd.write(b'K ' + bytes(str(len(key)), 'utf-8') + b'\n')
> + fd.write(key + b'\n')
> + fd.write(b'V ' + bytes(str(len(val)), 'utf-8') + b'\n')
> + fd.write(val + b'\n')
> + fd.write(TERMINATOR)
> +
> +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>.
> + """
> + tmpFilename = filename + '.tmp'
> + try:
> + with open(tmpFilename, 'xb') as fd:
> + outputHash(fd, hash)
> + os.rename(tmpFilename, filename)
> + except FileExistsError:
> + print('{} File {} already exist. Is the script already
> running?\n'.format(argv[0], tmpFilename), file=sys.stderr)
s/{}/{}:/ (first time)
s/{}/{!r}/ (second time) - in this case this will probably just add
quotes, but still. (tl;dr: This prints the repr() of the formatee
rather than its str().)
Is there any point in naming the exception and printing something for it
(probably one of its attributes)? I don't see a failure mode in which
that would make a difference, but absence of evidence isn't evidence of
absence.
> + except:
> + os.remove(tmpFilename)
> + raise
> +
> +def main():
> + """Main function"""
I'm not a fan of docstrings that simply repeat what may be inferred from
the identifier. I don't think there's much this function's docstring
_could_ say, though. I'd consider removing the docstring entirely or
having it use the term "entry point" so as to make it less tautological.
> + # 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.
Cheers,
Daniel
