Issue 3832: react sanely to ill-formed version strings

Mostly, this cleans up Lilypond_version

We no longer convert to int because it is weird, but we still want
version comparisons.  Conversion to bool for checking validity seems
nice in addition.

And bombing out because of invalid data to String_convert::dec2int
seemed definitely non-nice.  This version requires specifying at
least major and minor version number explicitly separated by dots.

It accepts a trailing dot, too.  A trailing extra_patch_string after
major.minor.patchlevel. is also accepted but not interpreted.

diff --git a/lily/include/lilypond-version.hh b/lily/include/lilypond-version.hh
index a38b04bc537625e81a10a194256eb02a58bcceec..c59afb561ee687d76a4c4fa7cfa23a04468de17b 100644 (file)
--- a/lily/include/lilypond-version.hh
+++ b/lily/include/lilypond-version.hh
@@ -21,6 +21,7 @@
 #define LILYPOND_VERSION_HH
 
 #include "std-string.hh"
+#include "compare.hh"
 
 struct Lilypond_version
 {
@@ -28,7 +29,8 @@ struct Lilypond_version
   Lilypond_version (const string &str);
 
   string to_string () const;
-  operator int () const;
+  operator bool () const;
+  static int compare (const Lilypond_version &, const Lilypond_version &);
 
   int major_;
   int minor_;
@@ -36,6 +38,8 @@ struct Lilypond_version
   string extra_patch_string_;
 };
 
+INSTANTIATE_COMPARE (const Lilypond_version &, Lilypond_version::compare);
+
 extern Lilypond_version oldest_version;
 
 #endif // LILYPOND_VERSION_HH

diff --git a/lily/lexer.ll b/lily/lexer.ll
index fbe878c56c9c227955a53b591f7562a330633f8e..f242158df78a0ad137efa96ea1da64d97d65908e 100644 (file)
--- a/lily/lexer.ll
+++ b/lily/lexer.ll
@@ -1274,7 +1274,12 @@ is_valid_version (string s)
 {
   Lilypond_version current ( MAJOR_VERSION "." MINOR_VERSION "." PATCH_LEVEL );
   Lilypond_version ver (s);
-  if (int (ver) < oldest_version)
+  if (!ver)
+  {
+         non_fatal_error (_f ("Invalid version string \"%s\"", s));
+         return false;
+  }
+  if (ver < oldest_version)
        {       
                non_fatal_error (_f ("file too old: %s (oldest supported: %s)", ver.to_string (), oldest_version.to_string ()));
                non_fatal_error (_ ("consider updating the input with the convert-ly script"));

diff --git a/lily/lilypond-version.cc b/lily/lilypond-version.cc
index fd76deec67e1ea33654c1a44db35381fed1d9d6a..fcee302dd0c8d647ec61f26c2cb98dbe6c168da0 100644 (file)
--- a/lily/lilypond-version.cc
+++ b/lily/lilypond-version.cc
@@ -32,38 +32,60 @@ Lilypond_version::Lilypond_version (int major, int minor, int patch)
 
 Lilypond_version::Lilypond_version (const string &str)
 {
-  major_ = 0;
+  major_ = -1;
   minor_ = 0;
   patch_ = 0;
 
   vector<string> version;
+  const char *digits = "0123456789";
   version = string_split (str, '.');
 
-  if (version.size () > 0 && isdigit (version[0][0]))
-    major_ = String_convert::dec2int (version[0]);
-  if (version.size () > 1 && isdigit (version[1][0]))
-    minor_ = String_convert::dec2int (version[1]);
-
-  patch_ = 0;
-  if (version.size () >= 3
-      && isdigit (version[2][0]))
-    patch_ = String_convert::dec2int (version[2]);
-
-  if (version.size () >= 4)
+  switch (version.size ()) {
+  case 4:
     extra_patch_string_ = version[3];
+    if (version[2].empty ())
+      return;
+  case 3:
+    if (version[2].find_first_not_of (digits) != string::npos
+        || version[1].empty ())
+      return;
+    patch_ = String_convert::dec2int (version[2]);
+  case 2:
+    if (version[1].find_first_not_of (digits) != string::npos
+        || version[1].empty () || version[0].empty ())
+      return;
+    minor_ = String_convert::dec2int (version[1]);
+    if (version[0].find_first_not_of (digits) != string::npos)
+      return;
+    major_ = String_convert::dec2int (version[0]);
+  }
 }
 
 string
 Lilypond_version::to_string () const
 {
+  if (major_ < 0)
+    return "invalid";
   return ::to_string (major_)
          + "." + ::to_string (minor_)
          + "." + ::to_string (patch_);
 }
 
-Lilypond_version::operator int () const
+Lilypond_version::operator bool () const
 {
-  // ugh
-  return 100000 * major_ + 1000 * minor_ + patch_;
+  return !(major_ < 0);
 }
 
+int
+Lilypond_version::compare (const Lilypond_version &a, const Lilypond_version &b)
+{
+  if (!a || !b)
+    return 0;
+  if (a.major_ != b.major_)
+    return a.major_ > b.major_ ? 1 : -1;
+  if (a.minor_ != b.minor_)
+    return a.minor_ > b.minor_ ? 1 : -1;
+  if (a.patch_ != b.patch_)
+    return a.patch_ > b.patch_ ? 1 : -1;
+  return 0;
+}