Daniel Sahlberg wrote on Fri, Feb 26, 2021 at 08:31:55 +0100:
> Den tors 25 feb. 2021 kl 08:00 skrev Daniel Shahaf <d...@daniel.shahaf.name>:
>
> > Daniel Sahlberg wrote on Wed, Feb 24, 2021 at 23:27:18 +0100:
> > > Learning Python has been on my todo list for ages, so I've cobbled
> > > together something that seems to do the job.
> >
> > ☺
> >
>
> Thank you very much for your detailed feedback! Based on it I more or less
> did a ground-up rewrite, sorry for forcing you to restart the review from
> scratch. I will address your commets below.
No worries.
> > > +# K [length]
> > > +# [keyname]
> > > +# V [length]
> > > +# [value]
> > > +# ...
> > > +# END
> > > +#
> > > +# The length is not validated
> >
> > It's not just "not validated"; it's not used at all. The file format is
> > binary — the lengths are in octets — but the Python code below treats
> > the file as a line-based format. That may be fine for now, but is not
> > forward-compatible with ~/.subversion/auth/ files that may be created by
> > future versions.
> >
>
> Length is now validated.
>
> I think it is acceptable that we make some assumptions about the format. If
> some future version would make a more complex format, the script can be
> updated. (This part of the code is removed, but the comment is relevant for
> the section updating an existing file).
*nod*
> > I'm not sure what level of the API (public or private) third-party
> > products would need to operate at in order to add their own custom keys
> > to the cache files. I don't know of anyone doing that, but it's not
> > impossible.
> >
>
> Switched to us a dictionary (but the old code should also have been
> agnostic of key ordering).
I was concerned not of key ordering but about cache files that have more
than just the standard set of keys. In particular, such custom keys
aren't necessarily restricted to texty, single-line values.
> > > +# Write a key/value pair to a hashfile
> > > +def writeHash(file, key, val):
> >
> > The docstring should be a string literal as the first statement in the
> > function, as in subversion/tests/cmdline/. That then becomes the
> > function's __doc__ attribute.
> >
>
> Fixed!
Thanks. That's a thing for the top-of-file comment too, by the way. We
aren't consistent about this, but ezt.py and run_tests.py both do this.
> >
> > > + file.write('K ' + str(len(key)) + '\n')
> > > + file.write(key + '\n')
> >
> > str() gives the length in characters, but the file format demands
> > a length in octets, so this would break as soon as somebody puts
> > non-ASCII in their password — which, given the nature of passwords,
> > is plausible.
> >
> > You'll want to convert from str to bytes. See str.encode(). You'll
> > need to use b'' literals in concatenations.
> >
>
> Better now?
Better, yes. However, I think as it stands in v3, it'll write CRLF on
Windows? I.e., don't you need 'b' in open()'s mode=* argument?
> > +if args.listpassword or args.list:
> > > + data = []
> > > + if args.listpassword:
> > > + header = ['Realm', 'Username', 'Password']
> > > + else:
> > > + header = ['Realm', 'Username']
> > > + for name in listdir(authpath):
> > > + fullname = join(authpath, name)
> > > + if isfile(fullname):
> > > + file = open(fullname, 'r')
> > > + realm = ''
> > > + user = ''
> > > + password = ''
> > > + while(True):
> >
> > The parentheses aren't idiomatic.
> >
>
> C-sick. ;-)
Then write a C-shell script to upload C-metric eggs to C-pan? I'm sure
I can come up with a cpan(1) invocation that will break a egg… :P
(https://www.cpan.org/; https://packaging.python.org/glossary/#term-Egg)
> > + parser.print_help()
> > > + quit()
> >
> > This will NameError on 'quit'. At least the exit code will be non-zero
> > this way.
That's still applicable to v3.
> > > +# In an existing file, we add/replace password/username/passtype
> >
> > This bit of code conflates several distinct pieces of logic:
> >
> > - Parsing a serialized hash file
> >
> > - Serializing a hash
> >
> > - The actual patching of the hash
> >
> > - Implementing editing a file by an atomic rename
> >
> > I can't say I like this at all. All these concerns should be cleanly
> > separated from one another.
> >
>
> The rewrite first read the complete hash to a dictionary. Then edit the
> dictionary. Finally writing the dictionary to a new file.
Much better.
> > +if isfile(fullname):
> > > + pwdAdded = False
> > > + passtypeAdded = False
> > > + usernameAdded = False
> > > +
> > > + # Read existing file, write to .tmp which we rename in the end
> > > + inFile = open(fullname, 'r')
> > > + outFile = open(fullname + '.tmp', 'w')
> >
> > Also 'x' please for O_EXCL. If nothing else, this'll plug a race
> > condition when two instances of this script run simultaneously.
> >
>
> Fixed.
Thanks. You changed it to 'x'; shouldn't the 'w' be retained as well,
i.e., 'wx' (and 'wxb' in combination with a previous point)? O_EXCL
doesn't imply either O_RDWR or O_WRONLY.
> Thanks again for your review!
You're welcome. I see you've posted v3 so I'll skip reviewing v2.
Cheers,
Daniel
