The generate-names.pl --workdir parameter is a little dangerous #563

Closed
cmdcolin opened this Issue Feb 18, 2015 · 5 comments

Comments

Projects
None yet
2 participants
@cmdcolin
Contributor

cmdcolin commented Feb 18, 2015

I realized when I saw a mailing list thread where someone misused the --workdir parameter that it is a little dangerous after it was re-added to JBrowse 1.11.6

People might mistake it's meaning and point it to a directory where some data that they want indexed. It will actually delete all that data and they will be very sad.

Instead of removing all data, it could just remove any old hash files.

@enuggetry

This comment has been minimized.

Show comment
Hide comment
@enuggetry

enuggetry Jun 3, 2015

Contributor

I find that if --workdir is not specified, it uses the data/names dir and creates a number of log files, which are eventually cleaned. If --workdir is specified, log files are created in the specified workdir and cleaned before the empty() method is called , which does the dangerous deeds.
Colin and I had a little discussion as to whether we should rename the option to --tempdir, reflecting the temporary nature of the content; or to remove it all together, given remoteness of anyone actually needing the feature.
In the end, here, I think I'll keep it simple and remove the call to empty(), affecting only one line in one file with no other changes to documentation or otherwise, and leaving the option for any future esoteric needs.

Contributor

enuggetry commented Jun 3, 2015

I find that if --workdir is not specified, it uses the data/names dir and creates a number of log files, which are eventually cleaned. If --workdir is specified, log files are created in the specified workdir and cleaned before the empty() method is called , which does the dangerous deeds.
Colin and I had a little discussion as to whether we should rename the option to --tempdir, reflecting the temporary nature of the content; or to remove it all together, given remoteness of anyone actually needing the feature.
In the end, here, I think I'll keep it simple and remove the call to empty(), affecting only one line in one file with no other changes to documentation or otherwise, and leaving the option for any future esoteric needs.

@enuggetry enuggetry closed this in 34896b8 Jun 3, 2015

@cmdcolin

This comment has been minimized.

Show comment
Hide comment
@cmdcolin

cmdcolin Jun 3, 2015

Contributor

I think the "empty" flag has sort of a different purpose and we might still want it.

Basically, the contents are normally re-indexed when generate-names.pl is called, and so the empty flag is turned on by default.

Otherwise, if the --incremental flag is on, then the empty flag is turned off.

Therefore, I think this change kind of makes all the generate-names.pl calls incremental which might not be desirable.

Contributor

cmdcolin commented Jun 3, 2015

I think the "empty" flag has sort of a different purpose and we might still want it.

Basically, the contents are normally re-indexed when generate-names.pl is called, and so the empty flag is turned on by default.

Otherwise, if the --incremental flag is on, then the empty flag is turned off.

Therefore, I think this change kind of makes all the generate-names.pl calls incremental which might not be desirable.

@cmdcolin

This comment has been minimized.

Show comment
Hide comment
@cmdcolin

cmdcolin Jun 3, 2015

Contributor

Here's the line where the empty parameter is set on the hashstore https://github.com/GMOD/jbrowse/blob/master/src/perl5/Bio/JBrowse/Cmd/IndexNames.pm#L228

Contributor

cmdcolin commented Jun 3, 2015

Here's the line where the empty parameter is set on the hashstore https://github.com/GMOD/jbrowse/blob/master/src/perl5/Bio/JBrowse/Cmd/IndexNames.pm#L228

@enuggetry

This comment has been minimized.

Show comment
Hide comment
@enuggetry

enuggetry Jun 3, 2015

Contributor

I see. ok. preserve the rmtree on the names dir but remove it on the
workdir.
presumably there's no safety issue on the names dir, since we expect we are
creating the names dir.
I see why --workdir might have been removed. It seems rather remote that
anybody would use an important dir as a workdir.
If you have no objection, I think I'll just remove the --workdir option.

On Wed, Jun 3, 2015 at 3:51 PM, Colin Diesh notifications@github.com
wrote:

Here's the line where the empty parameter is set on the hashstore
https://github.com/GMOD/jbrowse/blob/master/src/perl5/Bio/JBrowse/Cmd/IndexNames.pm#L228


Reply to this email directly or view it on GitHub
#563 (comment).

Contributor

enuggetry commented Jun 3, 2015

I see. ok. preserve the rmtree on the names dir but remove it on the
workdir.
presumably there's no safety issue on the names dir, since we expect we are
creating the names dir.
I see why --workdir might have been removed. It seems rather remote that
anybody would use an important dir as a workdir.
If you have no objection, I think I'll just remove the --workdir option.

On Wed, Jun 3, 2015 at 3:51 PM, Colin Diesh notifications@github.com
wrote:

Here's the line where the empty parameter is set on the hashstore
https://github.com/GMOD/jbrowse/blob/master/src/perl5/Bio/JBrowse/Cmd/IndexNames.pm#L228


Reply to this email directly or view it on GitHub
#563 (comment).

@cmdcolin

This comment has been minimized.

Show comment
Hide comment
@cmdcolin

cmdcolin Jun 3, 2015

Contributor

I think it might be ok but here is where the --workdir flag was originally suggested #273

The hashing strategy has changed since this issue was made so it may not be relevant anymore

Contributor

cmdcolin commented Jun 3, 2015

I think it might be ok but here is where the --workdir flag was originally suggested #273

The hashing strategy has changed since this issue was made so it may not be relevant anymore

enuggetry added a commit that referenced this issue Jul 2, 2015

Fix #563
This fix causes generate-names.pl to no delete the work directory, if --workdir is specified on the command.
Restore the call the empty().
I find that temporary log files are already removed in another section of code, so no other action is necessary.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment