Re: Add backup option to convert-ly (Issue 3572) (issue 14040043)

[dak]

On 2013/10/04 14:41:28, email_philholmes.net wrote:
> ----- Original Message -----
> From: <mailto:address@hidden>
> To: <address@hidden>; <address@hidden>;
> <address@hidden>; <mailto:address@hidden>
> Cc: <address@hidden>;
<mailto:address@hidden>
> Sent: Friday, October 04, 2013 3:24 PM
> Subject: Re: Add backup option to convert-ly (Issue 3572) (issue
14040043)


> >
> >
https://codereview.appspot.com/14040043/diff/6001/scripts/convert-ly.py
> > File scripts/convert-ly.py (right):
> >
> >

https://codereview.appspot.com/14040043/diff/6001/scripts/convert-ly.py#newcode241
> > scripts/convert-ly.py:241: while os.path.exists(back_up) and
> > os.path.isfile(back_up):
> > I'd do a repeat-until loop here in order to keep non-numbered and
> > numbered backup files strictly separate.
> >
> > With the current code, if there is no file~, the backup file will
always
> > (even in the presence of -n) be called file~, but file~ is much more
> > prone to overwriting accidentally than file.~1~ not least of all by
> > convert-ly (when called without -n option at a later point of time)
> > itself.

> Well, that's the whole point of this patch.  Without this new code,
the
> backup is always over-written.  With it, using the -b option prevents
> over-writing.

No, it doesn't.

If I have one particularly important revision and no previous backup
file, and I very much want to preserve the particularly important
revision, let's assume that I do

convert-ly -edn file.ly

once, and do future calls to convert-ly only via

convert-ly -ed file.ly

Does that mean that the important revision will be preserved?  No, it
will get overwritten by the future calls of convert-ly -ed because when
_no_ previous backup file is present, the first "numbered" backup will
be named "file.ly~" rather than "file.ly.~1~".

And I think that's a mistake.

And while we are at it: the loop has the condition
 while os.path.exists(back_up) and os.path.isfile(back_up):
for skipping over existing files.  The second part of the condition is
nonsensical since it means that a name will be used for backing up even
if it is already taken by a directory or fifo or socket.

https://codereview.appspot.com/14040043/