Merge lp:~nacc/ubuntu-dev-tools/update-vcs into lp:~ubuntu-dev/ubuntu-dev-tools/trunk

[Nish Aravamudan]

I am not necessarily requesting this for merging yet, but it's a PoC implementation that resolves a process issue we hit somewhat often on the Server Team.

I am open to:

  - Making this a separate command
  - Adding a new command (update-metadata?) that update-maintainer aliases to for BC which supports the new flag (perhaps without the flag and by default therefore?)
  - Adding a parameter to --vcs to specify a new value should be used instead (similar for vcs-browser I guess?). Could use the protocol of the repository to determine the protocol to use for the tag, if needed?
  - ... any other changes, I just think code speaks louder than words :)

[Benjamin Drung (bdrung)]

A quick review.

1) Instead of duplicating code (including classes), please re-use ubuntutools.update_maintainer and add your new methods to the Control class.

2) The functions update_maintainer and restore_maintainer could gain an vcs parameter to update the vcs option or you can refactor the code to retrieve the control class and then run update_maintainer, update_vcs on it.

3) Having an option to set the vcs values would be nice

I don't thinks that splitting this feature into a separate command makes sense.

review: Needs Fixing

[Nish Aravamudan (nacc)]

I think I have addressed 1) and 2). I need to do some more testing and want to actually write some tests before it gets merged.

[Mattia Rizzolo (mapreri)]

I think the current code is fine now.
@nacc do you plan to also write some tests? It would be really nice to have some.

I do not think things like the point 3 @bdrung should stop the merge (but you are welcome to implement it of course!).

Another thing: you are closing bug #1595744 in usd-importer with this, is that really fine?

review: Needs Information

[Nish Aravamudan (nacc)]

Hi Mattia,

Yes, I plan on writing tests still. Feel free to hold off on merging
until I get to that!

The importer bug is correct to close (it was a user request to have
this feature in `update-maintainer`, which we use in the server team's
documented git-based merge workflow). But I did update it to have a
task for ubuntu-dev-tools specifically, so I can update the snap of
the importer to fix that task, once this fix is avaiable.

On Sun, Apr 30, 2017 at 10:05 AM, Mattia Rizzolo <email address hidden> wrote:
> Review: Needs Information
>
> I think the current code is fine now.
> @nacc do you plan to also write some tests? It would be really nice to have some.
>
> I do not think things like the point 3 @bdrung should stop the merge (but you are welcome to implement it of course!).
>
> Another thing: you are closing bug #1595744 in usd-importer with this, is that really fine?
> --
> https://code.launchpad.net/~nacc/ubuntu-dev-tools/update-vcs/+merge/308871
> You are the owner of lp:~nacc/ubuntu-dev-tools/update-vcs.

[Mattia Rizzolo (mapreri)]

@nacc any update here?

[Mattia Rizzolo (mapreri)]

@nacc Could you please finish this up? :)

Unmerged revisions

1456. By Nish Aravamudan on 2017-03-31

Fix obvious syntax errors. Sorry!

1455. By Nish Aravamudan on 2017-03-31

Pass verbose down to the internal functions.

1454. By Nish Aravamudan on 2017-03-31

Drop unnecessary whitespace change.

1453. By Nish Aravamudan on 2017-03-31

Merge with lp:ubuntu-dev-tools

1452. By Nish Aravamudan on 2017-03-31

Refactor code based upon MR feedback. Remove duplication of code between
update_maintainer.py and update_vcs.py. Delete update_vcs.py. Add a vcs
parameter to {update,restore}_maintainer to indicate if VCS fields
should be modified. Refactor both functions to not continue in the loop
until both maintainer and VCS fields have been examined if necessary.

1451. By Nish Aravamudan on 2016-10-19

update-maintainer: add --vcs mode

As we perform merges on the server team, we tend to try and replay the
maintainer changes to debian/control via update-maintainer. However, for
those packages that also update the Vcs fields, we have no automated
tooling to do the same. Add a fairly simple version of the same code
as in update_maintainer, but handling Vcs-Git-* fields (including
Browser).

LP: #567629, LP: #1595744

Signed-off-by: Nishanth Aravamudan <email address hidden>

=== modified file 'ubuntutools/update_maintainer.py'
--- ubuntutools/update_maintainer.py	2014-12-18 20:57:17 +0000
+++ ubuntutools/update_maintainer.py	2017-03-31 22:08:28 +0000
@@ -53,6 +53,17 @@
             maintainer = maintainer.group(1)
         return maintainer
 
+    def get_vcs(self):
+        """Returns the value(s) of the Vcs-{Git,Bzr,Browser,...} field(s)."""
+        return re.findall("^(Vcs-(?:Arch|Bzr|Cvs|Darcs|Git|Hg|Mtn|Svn|Browser)): ?(.*)$",
+                          self._content, re.MULTILINE)
+
+    def get_original_vcs(self):
+        """Returns the value(s) of the XS-Debian-Vcs-{Git,Bzr,Browser...} field(s)."""
+        orig_vcs = re.findall("^(?:[XS]*-)?Debian-(Vcs-(?:Arch|Bzr|Cvs|Darcs|Git|Hg|Mtn|Svn|Browser)): ?(.*)$",
+                              self._content, re.MULTILINE)
+        return orig_vcs 
+ 
     def get_original_maintainer(self):
         """Returns the value of the XSBC-Original-Maintainer field."""
         orig_maintainer = re.search("^(?:[XSBC]*-)?Original-Maintainer: ?(.*)$",
@@ -86,6 +97,29 @@
             self._content = pattern.sub(r"\1\n" + original_maintainer,
                                         self._content)
 
+    def reset_vcs(self, vcs):
+        """Sets the value of the Vcs- field(s)."""
+        for v in vcs:
+            pattern = re.compile("^XS-Debian-%s: ?.*$" % v[0], re.MULTILINE)
+            self._content = pattern.sub("%s: %s" % (v[0], v[1]), self._content)
+
+
+    def set_original_vcs(self, original_vcs):
+        replace = False
+        if self.get_original_vcs():
+            replace = True
+        for vcs in original_vcs:
+            ovcs = "XS-Debian-" + vcs[0] + ": " + vcs[1]
+            if replace:
+                pattern = re.compile("^(?:[XS]*-)?Debian-" + vcs[0] + ":.*$",
+                                     re.MULTILINE)
+                self._content = pattern.sub(ovcs, self._content)
+            else:
+                pattern = re.compile("^(" + vcs[0] + ":.*)$",
+                                     re.MULTILINE)
+                self._content = pattern.sub(ovcs,
+                                            self._content)
+
     def remove_original_maintainer(self):
         """Strip out out the XSBC-Original-Maintainer line"""
         pattern = re.compile("^(?:[XSBC]*-)?Original-Maintainer:.*?$.*?^",
@@ -133,16 +167,71 @@
     return (changelog_file, control_files)
 
 
-def update_maintainer(debian_directory, verbose=False):
-    """updates the Maintainer field of an Ubuntu package
+def _update_maintainer(control, verbose):
+    original_maintainer = control.get_maintainer()
+
+    if original_maintainer is None:
+        Logger.error("No Maintainer field found in %s.", control_file)
+        raise MaintainerUpdateException("No Maintainer field found")
+
+    if original_maintainer.strip().lower() in _PREVIOUS_UBUNTU_MAINTAINER:
+        if verbose:
+            print("The old maintainer was: %s" % original_maintainer)
+            print("Resetting as: %s" % _UBUNTU_MAINTAINER)
+        control.set_maintainer(_UBUNTU_MAINTAINER)
+        control.save()
+        return
+
+    if original_maintainer.strip().endswith("ubuntu.com>"):
+        if verbose:
+            print ("The Maintainer email is set to an ubuntu.com address. "
+                   "Doing nothing.")
+        return
+
+    if control.get_original_maintainer() is not None:
+        Logger.warn("Overwriting original maintainer: %s",
+                    control.get_original_maintainer())
+
+    if verbose:
+        print("The original maintainer is: %s" % original_maintainer)
+        print("Resetting as: %s" % _UBUNTU_MAINTAINER)
+    control.set_original_maintainer(original_maintainer)
+    control.set_maintainer(_UBUNTU_MAINTAINER)
+    control.save()
+
+
+def _update_vcs(control, verbose):
+    original_vcs = control.get_vcs()
+    if len(original_vcs) == 0:
+        Logger.error("No Vcs-* fields found in %s.", control_file)
+        raise VcsUpdateException("No Vcs-* fields found")
+     
+    if len(control.get_original_vcs()) != 0:
+        Logger.warn("Overwriting original Vcs: %s",
+                    control.get_original_vcs())
+
+    if verbose:
+        print("The original Vcs values are:")
+        for vcs in original_vcs:
+            print("  %s: %s" % (vcs[0], vcs[1]))
+    control.set_original_vcs(original_vcs)
+    control.save()
+
+
+def update_maintainer(debian_directory, verbose=False, vcs=False):
+    """updates the Maintainer and VCS field(s) of an Ubuntu package
 
     * No modifications are made if the Maintainer field contains an ubuntu.com
       email address. Otherwise, the Maintainer field will be set to
       Ubuntu Developers <[e-mail]>
     * The old value will be saved in a field named XSBC-Original-Maintainer
       if the Maintainer field is modified.
+    * If @vcs is True, no modifications are made if no Vcs-<SCM> or
+      Vcs-Browser fields are found. Otherwise, they are replaced with
+      Xs-Debian-Vcs-<SCM> and Xs-Debian-Vcs-Browser, respectively.
 
     Policy: https://wiki.ubuntu.com/DebianMaintainerField
+    VCS discussion: https://lists.ubuntu.com/archives/ubuntu-devel/2007-March/023332.html
     """
     try:
         changelog_file, control_files = _find_files(debian_directory, verbose)
@@ -152,47 +241,42 @@
 
     distribution = _get_distribution(changelog_file)
     for control_file in control_files:
-        control = Control(control_file)
-        original_maintainer = control.get_maintainer()
-
-        if original_maintainer is None:
-            Logger.error("No Maintainer field found in %s.", control_file)
-            raise MaintainerUpdateException("No Maintainer field found")
-
-        if original_maintainer.strip().lower() in _PREVIOUS_UBUNTU_MAINTAINER:
-            if verbose:
-                print("The old maintainer was: %s" % original_maintainer)
-                print("Resetting as: %s" % _UBUNTU_MAINTAINER)
-            control.set_maintainer(_UBUNTU_MAINTAINER)
-            control.save()
-            continue
-
-        if original_maintainer.strip().endswith("ubuntu.com>"):
-            if verbose:
-                print ("The Maintainer email is set to an ubuntu.com address. "
-                       "Doing nothing.")
-            continue
-
         if distribution in ("stable", "testing", "unstable", "experimental"):
             if verbose:
                 print("The package targets Debian. Doing nothing.")
             return
 
-        if control.get_original_maintainer() is not None:
-            Logger.warn("Overwriting original maintainer: %s",
-                        control.get_original_maintainer())
-
-        if verbose:
-            print("The original maintainer is: %s" % original_maintainer)
-            print("Resetting as: %s" % _UBUNTU_MAINTAINER)
-        control.set_original_maintainer(original_maintainer)
-        control.set_maintainer(_UBUNTU_MAINTAINER)
-        control.save()
-
+        control = Control(control_file)
+        _update_maintainer(control, verbose)
+        if vcs:
+            _update_vcs(control, verbose)
     return
 
 
-def restore_maintainer(debian_directory, verbose=False):
+def _restore_maintainer(control, verbose):
+    orig_maintainer = control.get_original_maintainer()
+    if not orig_maintainer:
+        return
+    if verbose:
+        print("Restoring original maintainer: %s" % orig_maintainer)
+    control.set_maintainer(orig_maintainer)
+    control.remove_original_maintainer()
+    control.save()
+
+
+def _restore_vcs(control, verbose):
+    orig_vcs = control.get_original_vcs()
+    if not orig_vcs:
+        return
+    if verbose:
+        print("Restoring original Vcs values:")
+        for vcs in orig_vcs:
+            print("  %s: %s" % (vcs[0], vcs[1]))
+    control.reset_vcs(orig_vcs)
+    control.save()
+
+
+def restore_maintainer(debian_directory, verbose=False, vcs=False):
     """Restore the original maintainer"""
     try:
         changelog_file, control_files = _find_files(debian_directory, verbose)
@@ -202,11 +286,6 @@
 
     for control_file in control_files:
         control = Control(control_file)
-        orig_maintainer = control.get_original_maintainer()
-        if not orig_maintainer:
-            continue
-        if verbose:
-            print("Restoring original maintainer: %s" % orig_maintainer)
-        control.set_maintainer(orig_maintainer)
-        control.remove_original_maintainer()
-        control.save()
+        _restore_maintainer(control, verbose)
+        if vcs:
+            _restore_vcs(control, verbose)
 
=== modified file 'update-maintainer'
--- update-maintainer	2012-05-06 17:42:39 +0000
+++ update-maintainer	2017-03-31 22:08:28 +0000
@@ -36,6 +36,10 @@
                       action='store_true', default=False)
     parser.add_option("-q", "--quiet", help="print no informational messages",
                       dest="quiet", action="store_true", default=False)
+    parser.add_option("-v", "--vcs",
+                      help="Apply the same operation (change/restore) "
+                            "to VCS fields",
+                      action='store_true', default=False)
     (options, args) = parser.parse_args()
 
     if len(args) != 0:
@@ -49,7 +53,9 @@
         operation = restore_maintainer
 
     try:
-        operation(options.debian_directory, not options.quiet)
+        operation(options.debian_directory,
+                  not options.quiet,
+                  options.vcs)
     except MaintainerUpdateException:
         sys.exit(1)