Add backup option to convert-ly (Issue 3572)

New patchset with documentation and picking up Julien's review comment.

[PhilEHolmes, 2013-09-27 13:11:59]

Initial patch to check the principle is OK.  Will require documentation updates
before it's pushed.  Please review Eluze's Python.

[Julien Rioux, 2013-09-27 19:10:19]

This suggestion from David sounded like a good default:
 Make numbered backups for files that have numbered backups already.

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

https://codereview.appspot.com/14040043/diff/1/scripts/convert-ly.py#newcode243
scripts/convert-ly.py:243: back_up = file + '.' + str(n) + '~'
Should we go with what David suggested: file.ly.~1~ etc.

[PhilEHolmes, 2013-09-28 11:49:45]

Adopts DAK's numbering syste; adds documentation

[PhilEHolmes, 2013-09-28 11:51:04]

Please review.

[Ian Hulin (gmail), 2013-09-29 01:00:49]

I'd prefer a single tilde to indicate the a backup file after the version
string.

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#newco...
scripts/convert-ly.py:243: back_up = file + '.~' + str(n) + '~'
            back_up = file + '.' + str(n) + "~"
# I think mysource.ly.0~ or myinclude.ily.45~ is just as distinctive as a
# convert-ly backup file as mysource.ly.~0~ or myinclude.ily.~45~ . 
# The first ~ makes it harder for humans to read, and doesn't hinder programs
# identifying it as a convert-ly backup file.

[dak, 2013-09-29 05:46:39]

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#newco...
scripts/convert-ly.py:243: back_up = file + '.~' + str(n) + '~'
On 2013/09/29 01:00:49, Ian Hulin (gmail) wrote:
>             back_up = file + '.' + str(n) + "~"
> # I think mysource.ly.0~ or myinclude.ily.45~ is just as distinctive as a
> # convert-ly backup file as mysource.ly.~0~ or myinclude.ily.~45~ . 
> # The first ~ makes it harder for humans to read, and doesn't hinder programs
> # identifying it as a convert-ly backup file.

The name here was chosen to correspond with the numbered backups of Emacs. 
Emacs will recognize a numbered backup file (joining the backup scheme) only
when there is a good match.  In addition, this makes it recognize that the
"real" file extension is .ly rather than .45.

[dak, 2013-09-29 06:05:56]

On 2013/09/29 05:46:39, dak wrote:
> https://codereview.appspot.com/14040043/diff/6001/scripts/convert-ly.py

> The name here was chosen to correspond with the numbered backups of Emacs. 
> Emacs will recognize a numbered backup file (joining the backup scheme) only
> when there is a good match.  In addition, this makes it recognize that the
> "real" file extension is .ly rather than .45.

More importantly: lilypond.1~ already is the name of the normal backup file for
the nroff source of the lilypond manual page.  There really is no point in
trying to invent our own scheme here.

[Julien Rioux, 2013-09-30 10:30:35]

Again, can we add a check if there is already a numbered backup around?

Make numbered backups for files that have numbered backups already.
     Otherwise, make single backups.  This is the default.

That would mean:

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#newco...
scripts/convert-ly.py:239: if numbered:
if numbered or (os.path.exists(file + '.~1~') and os.path.isfile(file +
'.~1~')):

[PhilEHolmes, 2013-09-30 15:10:00]

Julien - I'm not convinced that's a good idea.  It would mean that, once you'd
"turned numbering on", then you couldn't turn it off except by deleting all the
numbered files.  I think it's better to let the user select, based on the
command line switches.

[dak, 2013-09-30 15:29:38]

On 2013/09/30 15:10:00, PhilEHolmes wrote:
> Julien - I'm not convinced that's a good idea.  It would mean that, once you'd
> "turned numbering on", then you couldn't turn it off except by deleting all
the
> numbered files.  I think it's better to let the user select, based on the
> command line switches.

By deleting or renaming the _first_ numbered file.  I think that's ok as a
default, but it might make sense to have an option -n0 or whatever that
explicitly turns it off even when there are already numbered backups.

[mail_philholmes.net, 2013-09-30 15:53:56]

----- Original Message ----- 
From: <dak@gnu.org>
To: <PhilEHolmes@googlemail.com>; <julien.rioux@gmail.com>; 
<ianhulin44@gmail.com>
Cc: <reply@codereview-hr.appspotmail.com>; <lilypond-devel@gnu.org>
Sent: Monday, September 30, 2013 4:29 PM
Subject: Re: Add backup option to convert-ly (Issue 3572) (issue 14040043)


> On 2013/09/30 15:10:00, PhilEHolmes wrote:
>> Julien - I'm not convinced that's a good idea.  It would mean that,
> once you'd
>> "turned numbering on", then you couldn't turn it off except by
> deleting all the
>> numbered files.  I think it's better to let the user select, based on
> the
>> command line switches.
>
> By deleting or renaming the _first_ numbered file.  I think that's ok as
> a default, but it might make sense to have an option -n0 or whatever
> that explicitly turns it off even when there are already numbered
> backups.
>
> https://codereview.appspot.com/14040043/


It was Eluze's enhancement and his coding.  Let's let him decide.

--
Phil Holmes 

[dak, 2013-10-04 14:24:53]

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#newco...
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.

The expectation is that a file created with -n option will not be deleted
automatically.  Naming the first "numbered" backup file file~ will violate that
expectation.

[email_philholmes.net, 2013-10-04 14:41:28]

----- Original Message ----- 
From: <dak@gnu.org>
To: <PhilEHolmes@googlemail.com>; <julien.rioux@gmail.com>; 
<ianhulin44@gmail.com>; <mail@philholmes.net>
Cc: <lilypond-devel@gnu.org>; <reply@codereview-hr.appspotmail.com>
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#newco...
> 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.

I don't understand your point here, but would re-iterate - this patch fixes 
the reported issue.  When it's pushed, further tinkering is possible, so 
let's just push a patch that simply adds a new option without changing 
existing options one iota.

> The expectation is that a file created with -n option will not be
> deleted automatically.  Naming the first "numbered" backup file file~
> will violate that expectation.


The expectation of the current code is that backups are overwritten.

--
Phil Holmes 

[dak, 2013-10-04 14:53:47]

On 2013/10/04 14:41:28, email_philholmes.net wrote:
> ----- Original Message ----- 
> From: <mailto:dak@gnu.org>
> To: <PhilEHolmes@googlemail.com>; <julien.rioux@gmail.com>; 
> <ianhulin44@gmail.com>; <mailto:mail@philholmes.net>
> Cc: <lilypond-devel@gnu.org>; <mailto:reply@codereview-hr.appspotmail.com>
> 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#newco...
> > 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.

[email_philholmes.net, 2013-10-04 15:09:48]

----- Original Message ----- 
From: <dak@gnu.org>
To: <PhilEHolmes@googlemail.com>; <julien.rioux@gmail.com>; 
<ianhulin44@gmail.com>; <mail@philholmes.net>; <email@philholmes.net>
Cc: <lilypond-devel@gnu.org>; <reply@codereview-hr.appspotmail.com>
Sent: Friday, October 04, 2013 3:53 PM
Subject: Re: Add backup option to convert-ly (Issue 3572) (issue 14040043)



>> > 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

I'm really confused here.  -n is the option for no-version.  How is this 
related to backup?

> 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/

I presume Eluze put it in for a specific reason which I don't know.  Eluze?

--
Phil Holmes 

[dak, 2013-10-04 15:47:17]

On 2013/10/04 15:09:48, email_philholmes.net wrote:

> > convert-ly -edn file.ly
> 
> I'm really confused here.  -n is the option for no-version.  How is this 
> related to backup?

Sorry, probably confused this with -b.  The rest of the comment stands.

[dak, 2013-10-04 16:12:45]

On 2013/10/04 15:47:17, dak wrote:
> On 2013/10/04 15:09:48, http://email_philholmes.net wrote:
> 
> > > convert-ly -edn file.ly
> > 
> > I'm really confused here.  -n is the option for no-version.  How is this 
> > related to backup?
> 
> Sorry, probably confused this with -b.  The rest of the comment stands.

Well, but it's wobbling.  Turns out that the while loop checks for an existing
file.ly in its first pass rather than an existing file.ly~ as I read it first. 
That should indeed be pretty harmless but is a rather unclean way to make sure
the loop runs at least once.  I'll put forward what I consider better code.