llvm_tools: Allow version_range for start/end

At present, the start_version and end_version information are
split as far apart as possible due to alphabetical sorting.
This leads to bugs when developers want to modify those bug
ranges by hand.

This CL is the first step to grouping these version ranges
together under the `version_range` property.

BUG=b:221489531
TEST=python3 patch_manager_unittest.py
TEST=cd patch_sync; cargo test
TEST=edit PATCHES.json w/ version_range; patch_sync show <...>

Change-Id: I9d0fead07c61c0bd0edb745ed623990f0686b8b2
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/toolchain-utils/+/3490757
Reviewed-by: George Burgess <gbiv@chromium.org>
Tested-by: Jordan Abrahams-Whitehead <ajordanr@google.com>
Commit-Queue: Jordan Abrahams-Whitehead <ajordanr@google.com>

7 files changed, 136 insertions, 45 deletions

diff --git a/llvm_tools/get_upstream_patch.py b/llvm_tools/get_upstream_patch.py
index 7a4be3eb..5669b023 100755
--- a/llvm_tools/get_upstream_patch.py
+++ b/llvm_tools/get_upstream_patch.py
@@ -96,15 +96,18 @@ def add_patch(patches_json_path: str, patches_dir: str,
       cwd=llvm_dir,
       encoding='utf-8')
 
+  end_vers = rev.number if isinstance(rev, git_llvm_rev.Rev) else None
   patch_props = {
       'rel_patch_path': rel_patch_path,
-      'start_version': start_version.number,
       'metadata': {
           'title': commit_subject.strip(),
           'info': [],
       },
       'platforms': sorted(platforms),
-      'end_version': rev.number if isinstance(rev, git_llvm_rev.Rev) else None,
+      'version_range': {
+          'from': start_version.number,
+          'until': end_vers,
+      },
   }
   patches_json.append(patch_props)
 
@@ -346,8 +349,8 @@ def _convert_patch(llvm_config: git_llvm_rev.LLVMConfig,
                      is_differential=is_differential)
 
 
-def _get_duplicate_shas(
-    patches: t.List[ParsedPatch]) -> t.List[t.Tuple[ParsedPatch, ParsedPatch]]:
+def _get_duplicate_shas(patches: t.List[ParsedPatch]
+                        ) -> t.List[t.Tuple[ParsedPatch, ParsedPatch]]:
   """Return a list of Patches which have duplicate SHA's"""
   return [(left, right) for i, left in enumerate(patches)
           for right in patches[i + 1:] if left.sha == right.sha]
diff --git a/llvm_tools/patch_manager.py b/llvm_tools/patch_manager.py
index 3c83fa96..f2d6b322 100755
--- a/llvm_tools/patch_manager.py
+++ b/llvm_tools/patch_manager.py
@@ -212,8 +212,13 @@ def GetPatchMetadata(patch_dict):
   """
 
   # Get the metadata values of a patch if possible.
-  start_version = patch_dict.get('start_version', 0)
-  end_version = patch_dict.get('end_version', None)
+  # FIXME(b/221489531): Remove  start_version & end_version
+  if 'version_range' in patch_dict:
+    start_version = patch_dict['version_range'].get('from', 0)
+    end_version = patch_dict['version_range'].get('until', None)
+  else:
+    start_version = patch_dict.get('start_version', 0)
+    end_version = patch_dict.get('end_version', None)
   is_critical = patch_dict.get('is_critical', False)
 
   return start_version, end_version, is_critical
diff --git a/llvm_tools/patch_manager_unittest.py b/llvm_tools/patch_manager_unittest.py
index 62947ed1..69bb683e 100755
--- a/llvm_tools/patch_manager_unittest.py
+++ b/llvm_tools/patch_manager_unittest.py
@@ -182,7 +182,9 @@ class PatchManagerTest(unittest.TestCase):
     test_patch = {
         'comment': 'Redirects output to stdout',
         'rel_patch_path': 'cherry/fixes_stdout.patch',
-        'end_version': 1000
+        'version_range': {
+            'until': 1000,
+        }
     }
 
     self.assertEqual(
@@ -275,7 +277,9 @@ class PatchManagerTest(unittest.TestCase):
     patch = [{
         'comment': 'Redirects output to stdout',
         'rel_patch_path': 'cherry/fixes_output.patch',
-        'start_version': 10
+        'version_range': {
+            'from': 10,
+        },
     }]
 
     abs_patch_path = '/abs/path/to/filesdir/PATCHES'
@@ -293,13 +297,17 @@ class PatchManagerTest(unittest.TestCase):
     test_updated_patch_metadata = [{
         'comment': 'Redirects output to stdout',
         'rel_patch_path': 'cherry/fixes_output.patch',
-        'start_version': 10
+        'version_range': {
+            'from': 10,
+        }
     }]
 
     expected_patch_metadata = {
         'comment': 'Redirects output to stdout',
         'rel_patch_path': 'cherry/fixes_output.patch',
-        'start_version': 10
+        'version_range': {
+            'from': 10,
+        }
     }
 
     with CreateTemporaryJsonFile() as json_test_file:
@@ -335,7 +343,9 @@ class PatchManagerTest(unittest.TestCase):
     test_patch_metadata = [{
         'comment': 'Redirects output to stdout',
         'rel_patch_path': rel_patch_path,
-        'start_version': 10
+        'version_range': {
+            'from': 10,
+        }
     }]
 
     with CreateTemporaryJsonFile() as json_test_file:
@@ -379,7 +389,9 @@ class PatchManagerTest(unittest.TestCase):
     test_patch_metadata = [{
         'comment': 'Redirects output to stdout',
         'rel_patch_path': rel_patch_path,
-        'start_version': 1000
+        'version_range': {
+            'from': 1000,
+        },
     }]
 
     with CreateTemporaryJsonFile() as json_test_file:
@@ -415,28 +427,36 @@ class PatchManagerTest(unittest.TestCase):
     test_patch_1 = {
         'comment': 'Redirects output to stdout',
         'rel_patch_path': 'cherry/fixes_output.patch',
-        'start_version': 1000,
-        'end_version': 1250
+        'version_range': {
+            'from': 1000,
+            'until': 1250
+        }
     }
 
     test_patch_2 = {
         'comment': 'Fixes input',
         'rel_patch_path': 'cherry/fixes_input.patch',
-        'start_version': 1000
+        'version_range': {
+            'from': 1000
+        }
     }
 
     test_patch_3 = {
         'comment': 'Adds a warning',
         'rel_patch_path': 'add_warning.patch',
-        'start_version': 750,
-        'end_version': 1500
+        'version_range': {
+            'from': 750,
+            'until': 1500
+        }
     }
 
     test_patch_4 = {
         'comment': 'Adds a helper function',
         'rel_patch_path': 'add_helper.patch',
-        'start_version': 20,
-        'end_version': 900
+        'version_range': {
+            'from': 20,
+            'until': 900
+        }
     }
 
     test_patch_metadata = [
@@ -520,28 +540,36 @@ class PatchManagerTest(unittest.TestCase):
     test_patch_1 = {
         'comment': 'Redirects output to stdout',
         'rel_patch_path': 'cherry/fixes_output.patch',
-        'start_version': 1000,
-        'end_version': 1190
+        'version_range': {
+            'from': 1000,
+            'until': 1190
+        }
     }
 
     test_patch_2 = {
         'comment': 'Fixes input',
         'rel_patch_path': 'cherry/fixes_input.patch',
-        'start_version': 1000
+        'version_range': {
+            'from': 1000
+        }
     }
 
     test_patch_3 = {
         'comment': 'Adds a warning',
         'rel_patch_path': 'add_warning.patch',
-        'start_version': 750,
-        'end_version': 1500
+        'version_range': {
+            'from': 750,
+            'until': 1500
+        }
     }
 
     test_patch_4 = {
         'comment': 'Adds a helper function',
         'rel_patch_path': 'add_helper.patch',
-        'start_version': 20,
-        'end_version': 2000
+        'version_range': {
+            'from': 20,
+            'until': 2000
+        }
     }
 
     test_patch_metadata = [
@@ -654,8 +682,10 @@ class PatchManagerTest(unittest.TestCase):
     test_patch_1 = {
         'comment': 'Redirects output to stdout',
         'rel_patch_path': 'cherry/fixes_output.patch',
-        'start_version': 1000,
-        'end_version': 1190
+        'version_range': {
+            'from': 1000,
+            'until': 1190
+        }
     }
 
     # For the 'remove_patches' mode, this patch is expected to be in the
@@ -665,7 +695,9 @@ class PatchManagerTest(unittest.TestCase):
     test_patch_2 = {
         'comment': 'Fixes input',
         'rel_patch_path': 'cherry/fixes_input.patch',
-        'start_version': 1000
+        'version_range': {
+            'from': 1000
+        }
     }
 
     # For the 'remove_patches' mode, this patch is expected to be in the
@@ -674,8 +706,10 @@ class PatchManagerTest(unittest.TestCase):
     test_patch_3 = {
         'comment': 'Adds a warning',
         'rel_patch_path': 'add_warning.patch',
-        'start_version': 750,
-        'end_version': 1500
+        'version_range': {
+            'from': 750,
+            'until': 1500
+        }
     }
 
     # For the 'remove_patches' mode, this patch is expected to be in the
@@ -684,8 +718,10 @@ class PatchManagerTest(unittest.TestCase):
     test_patch_4 = {
         'comment': 'Adds a helper function',
         'rel_patch_path': 'add_helper.patch',
-        'start_version': 20,
-        'end_version': 1400
+        'version_range': {
+            'from': 20,
+            'until': 1400
+        }
     }
 
     test_patch_metadata = [
@@ -786,8 +822,10 @@ class PatchManagerTest(unittest.TestCase):
     test_patch_1 = {
         'comment': 'Redirects output to stdout',
         'rel_patch_path': 'cherry/fixes_output.patch',
-        'start_version': 1000,
-        'end_version': 1190
+        'version_range': {
+            'from': 1000,
+            'until': 1190
+        }
     }
 
     # For the 'remove_patches' mode, this patch is expected to be in the
@@ -797,7 +835,9 @@ class PatchManagerTest(unittest.TestCase):
     test_patch_2 = {
         'comment': 'Fixes input',
         'rel_patch_path': 'cherry/fixes_input.patch',
-        'start_version': 1000
+        'version_range': {
+            'from': 1000,
+        }
     }
 
     # For the 'remove_patches' mode, this patch is expected to be in the
@@ -806,8 +846,10 @@ class PatchManagerTest(unittest.TestCase):
     test_patch_3 = {
         'comment': 'Adds a warning',
         'rel_patch_path': 'add_warning.patch',
-        'start_version': 750,
-        'end_version': 1500
+        'version_range': {
+            'from': 750,
+            'until': 1500
+        }
     }
 
     # For the 'remove_patches' mode, this patch is expected to be in the
@@ -816,8 +858,10 @@ class PatchManagerTest(unittest.TestCase):
     test_patch_4 = {
         'comment': 'Adds a helper function',
         'rel_patch_path': 'add_helper.patch',
-        'start_version': 1600,
-        'end_version': 2000
+        'version_range': {
+            'from': 1600,
+            'until': 2000
+        }
     }
 
     test_patch_metadata = [
diff --git a/llvm_tools/patch_sync/Cargo.lock b/llvm_tools/patch_sync/Cargo.lock
index 62db2220..1ad74a77 100644
--- a/llvm_tools/patch_sync/Cargo.lock
+++ b/llvm_tools/patch_sync/Cargo.lock
@@ -162,7 +162,7 @@ checksum = "624a8340c38c1b80fd549087862da4ba43e08858af025b236e509b6649fc13d5"
 
 [[package]]
 name = "patch_sync"
-version = "0.1.0"
+version = "1.1.0"
 dependencies = [
  "anyhow",
  "rand",
diff --git a/llvm_tools/patch_sync/Cargo.toml b/llvm_tools/patch_sync/Cargo.toml
index ba8817d5..ed33d5ca 100644
--- a/llvm_tools/patch_sync/Cargo.toml
+++ b/llvm_tools/patch_sync/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "patch_sync"
-version = "0.1.0"
+version = "1.1.0"
 authors = ["Jordan R Abrahams-Whitehead <ajordanr@google.com>"]
 edition = "2018"
 
diff --git a/llvm_tools/patch_sync/src/main.rs b/llvm_tools/patch_sync/src/main.rs
index b7967c31..c244f1c0 100644
--- a/llvm_tools/patch_sync/src/main.rs
+++ b/llvm_tools/patch_sync/src/main.rs
@@ -163,13 +163,14 @@ fn transpose_subcmd(args: TransposeOpt) -> Result<()> {
             )
         })?
     };
-    let new_android_patches =
-        new_android_patches.filter_patches(|p| match (p.start_version, p.end_version) {
+    let new_android_patches = new_android_patches.filter_patches(|p| {
+        match (p.get_start_version(), p.get_end_version()) {
             (Some(start), Some(end)) => start <= android_llvm_version && android_llvm_version < end,
             (Some(start), None) => start <= android_llvm_version,
             (None, Some(end)) => android_llvm_version < end,
             (None, None) => true,
-        });
+        }
+    });
 
     if args.verbose {
         display_patches("New patches from Chromium OS", &new_cros_patches);
diff --git a/llvm_tools/patch_sync/src/patch_parsing.rs b/llvm_tools/patch_sync/src/patch_parsing.rs
index 581b1899..124f0d6f 100644
--- a/llvm_tools/patch_sync/src/patch_parsing.rs
+++ b/llvm_tools/patch_sync/src/patch_parsing.rs
@@ -8,14 +8,43 @@ use serde::{Deserialize, Serialize};
 use sha2::{Digest, Sha256};
 
 /// JSON serde struct.
+// FIXME(b/221489531): Remove when we clear out start_version and
+// end_version.
 #[derive(Debug, Clone, Serialize, Deserialize)]
 pub struct PatchDictSchema {
+    /// [deprecated(since = "1.1", note = "Use version_range")]
+    #[serde(skip_serializing_if = "Option::is_none")]
     pub end_version: Option<u64>,
     pub metadata: Option<BTreeMap<String, serde_json::Value>>,
     #[serde(default, skip_serializing_if = "BTreeSet::is_empty")]
     pub platforms: BTreeSet<String>,
     pub rel_patch_path: String,
+    /// [deprecated(since = "1.1", note = "Use version_range")]
+    #[serde(skip_serializing_if = "Option::is_none")]
     pub start_version: Option<u64>,
+    pub version_range: Option<VersionRange>,
+}
+
+#[derive(Debug, Clone, Copy, Serialize, Deserialize)]
+pub struct VersionRange {
+    pub from: Option<u64>,
+    pub until: Option<u64>,
+}
+
+// FIXME(b/221489531): Remove when we clear out start_version and
+// end_version.
+impl PatchDictSchema {
+    pub fn get_start_version(&self) -> Option<u64> {
+        self.version_range
+            .map(|x| x.from)
+            .unwrap_or(self.start_version)
+    }
+
+    pub fn get_end_version(&self) -> Option<u64> {
+        self.version_range
+            .map(|x| x.until)
+            .unwrap_or(self.end_version)
+    }
 }
 
 /// Struct to keep track of patches and their relative paths.
@@ -137,6 +166,7 @@ impl PatchCollection {
                             end_version: p.end_version,
                             platforms: new_platforms,
                             metadata: p.metadata.clone(),
+                            version_range: p.version_range,
                         });
                         // iii.
                         *merged = true;
@@ -358,6 +388,10 @@ mod test {
             rel_patch_path: "a".into(),
             metadata: None,
             platforms: BTreeSet::from(["x".into()]),
+            version_range: Some(VersionRange {
+                from: Some(0),
+                until: Some(1),
+            }),
         };
         let patch2 = PatchDictSchema {
             rel_patch_path: "b".into(),
@@ -402,6 +436,10 @@ mod test {
             rel_patch_path: "a".into(),
             metadata: None,
             platforms: Default::default(),
+            version_range: Some(VersionRange {
+                from: Some(0),
+                until: Some(1),
+            }),
         };
         let collection1 = PatchCollection {
             workdir: PathBuf::new(),