From 340edc365beb37ecff4672b4fb88afa22d340aba Mon Sep 17 00:00:00 2001
From: Josip Sokcevic <sokcevic@google.com>
Date: Thu, 8 Jul 2021 17:01:46 +0000
Subject: [PATCH] Extract bug/fix from branch only on create

depot_tools supports extracting information from branch name. E.g. if
branch contains fix-\d or bug-\d, commit description will contains
appropriate git footers.

However, such behavior should happen only on initial CL upload. Should
user decide to delete such footer, we shouldn't set bug / fix on any
following PS.

R=ehmaldonado@chromium.org

Bug: 1225663
Change-Id: I66adfdca070083ab66727d132919d47feb7ddd43
Fixed: 1225663
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/3010709
Auto-Submit: Josip Sokcevic <sokcevic@google.com>
Reviewed-by: Andy Perelson <ajp@chromium.org>
Commit-Queue: Josip Sokcevic <sokcevic@google.com>
---
 git_cl.py            | 27 ++++++++++++++++++---------
 tests/git_cl_test.py | 33 +++++++++++++++++++++------------
 2 files changed, 39 insertions(+), 21 deletions(-)

diff --git a/git_cl.py b/git_cl.py
index 6f5ea49e3..ff66b6895 100755
--- a/git_cl.py
+++ b/git_cl.py
@@ -1192,7 +1192,7 @@ class Changelist(object):
     return '%s/%s' % (server, issue)
 
   def GetUsePython3(self):
-      return settings.GetUsePython3()
+    return settings.GetUsePython3()
 
   def FetchDescription(self, pretty=False):
     assert self.GetIssue(), 'issue is required to query Gerrit'
@@ -1414,16 +1414,25 @@ class Changelist(object):
       if options.title and options.squash:
         description = options.title + '\n\n' + description
 
-    # Extract bug number from branch name.
     bug = options.bug
     fixed = options.fixed
-    match = re.match(r'(?P<type>bug|fix(?:e[sd])?)[_-]?(?P<bugnum>\d+)',
-                     self.GetBranch())
-    if not bug and not fixed and match:
-      if match.group('type') == 'bug':
-        bug = match.group('bugnum')
-      else:
-        fixed = match.group('bugnum')
+    if not self.GetIssue():
+      # Extract bug number from branch name, but only if issue is being created.
+      # It must start with bug or fix, followed by _ or - and number.
+      # Optionally, it may contain _ or - after number with arbitrary text.
+      # Examples:
+      #   bug-123
+      #   bug_123
+      #   fix-123
+      #   fix-123-some-description
+      match = re.match(
+          r'^(?P<type>bug|fix(?:e[sd])?)[_-]?(?P<bugnum>\d+)([-_]|$)',
+          self.GetBranch())
+      if not bug and not fixed and match:
+        if match.group('type') == 'bug':
+          bug = match.group('bugnum')
+        else:
+          fixed = match.group('bugnum')
 
     change_description = ChangeDescription(description, bug, fixed)
 
diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py
index 7741591bd..b7a6b0401 100755
--- a/tests/git_cl_test.py
+++ b/tests/git_cl_test.py
@@ -1585,15 +1585,15 @@ class TestGitCl(unittest.TestCase):
     }
 
     cl = git_cl.Changelist(issue=1234)
-    actual = cl._GetDescriptionForUpload(
-        options=mock.Mock(
-            bug=bug,
-            fixed=fixed,
-            reviewers=reviewers,
-            tbrs=tbrs,
-            add_owners_to=add_owners_to),
-        git_diff_args=None,
-        files=list(owners_by_path))
+    actual = cl._GetDescriptionForUpload(options=mock.Mock(
+        bug=bug,
+        fixed=fixed,
+        reviewers=reviewers,
+        tbrs=tbrs,
+        add_owners_to=add_owners_to,
+        message=initial_description),
+                                         git_diff_args=None,
+                                         files=list(owners_by_path))
     self.assertEqual(expected_description, actual.description)
 
   def testGetDescriptionForUpload(self):
@@ -1617,8 +1617,9 @@ class TestGitCl(unittest.TestCase):
           'Fixed: prefix:1234',
         ]))
 
-
-  def testGetDescriptionForUpload_BugFromBranch(self):
+  @mock.patch('git_cl.Changelist.GetIssue')
+  def testGetDescriptionForUpload_BugFromBranch(self, mockGetIssue):
+    mockGetIssue.return_value = None
     self.getDescriptionForUploadTest(
         branch='bug-1234',
         expected_description='\n'.join([
@@ -1627,7 +1628,9 @@ class TestGitCl(unittest.TestCase):
           'Bug: prefix:1234',
         ]))
 
-  def testGetDescriptionForUpload_FixedFromBranch(self):
+  @mock.patch('git_cl.Changelist.GetIssue')
+  def testGetDescriptionForUpload_FixedFromBranch(self, mockGetIssue):
+    mockGetIssue.return_value = None
     self.getDescriptionForUploadTest(
         branch='fix-1234',
         expected_description='\n'.join([
@@ -1636,6 +1639,12 @@ class TestGitCl(unittest.TestCase):
           'Fixed: prefix:1234',
         ]))
 
+  def testGetDescriptionForUpload_SkipBugFromBranchIfAlreadyUploaded(self):
+    self.getDescriptionForUploadTest(
+        branch='bug-1234',
+        expected_description='desc',
+    )
+
   def testGetDescriptionForUpload_AddOwnersToR(self):
     self.getDescriptionForUploadTest(
         reviewers=['a@example.com'],