Bug#650132: hint: binary to source mapping has unhelpful side-effects

["Adam D. Barratt" <adam@adam-barratt.org.uk>]

Control: tags -1 + patch

On Fri, 2015-12-04 at 18:09 +0000, Adam D. Barratt wrote:
> On Sat, 2011-11-26 at 20:38 +0000, Adam D. Barratt wrote:
> > hint's automagic binary to source mapping has unintended and undesired
> > side-effects in some circumstances.  The situation where issues have
> > been noticed most is with "hint clean" and renamed source packages, but
> > it will likely manifest in other cases.
> [...]
> > I haven't found an easy way of resolving this yet, so am filing this in
> > the BTS so that the issue is documented and in case anyone else has any
> > thoughts.
> 
> After another of my periodic pokes at this, the main problem is that I
> can't see an easy way of fixing it without bending the encapsulation.
> 
> "clean" uses the HintFileParagraph class to represent the lines it has
> read from the hint file, which in turn instantiates a number of Hint
> objects. The Hint class's constructor then parses the package list,
> including performing the binary to source mappings. By this point, the
> knowledge of which subcommand of "hint" was used is well and truly
> invisible.
> 
> We could extend the Hint class to take keyword arguments so that the
> package mapping could be disabled when required, but this would then
> somehow need to be communicated via HintFileParagraph, which feels a
> little dirty.
> 
> Admittedly, the HintFileParagraph is only used by the clean action, so
> we could simply say that the class will not modify its input, and thus
> have it pass a "don't do binary to source mappings" flag to the Hint
> class.
> 
> Thoughts welcome, before I either decide that the above is a silly idea,
> or just do it. :-)

fwiw, here's a patch that does that.

The second "don't update" hunk also changes the behaviour in another way
- currently, if you have a hint for foo and foo is then removed from
unstable, a subsequent "hint clean" will drop the hint; after my update,
that will no longer happen. However, I think this is actually better,
for two reasons:

- currently "easy foo" would simply be replaced by "# No packages left
in hint", which isn't particularly helpful
- "easy foo bar" will be replaced by "easy bar", with no indication that
the hint has been changed. I don't think "clean" should make changes
like that.

Regards,

Adam

diff --git a/scripts/hint b/scripts/hint
index 9d38274..3b3b1ee 100755
--- a/scripts/hint
+++ b/scripts/hint
@@ -513,9 +513,11 @@ class Hint(object):
 
     ##
 
-    def __init__(self, hintname, *args):
+    def __init__(self, hintname, *args, **kwargs):
         starts_with_letter = re.compile(r'(?i)^[a-z]')
 
+        self.kwargs = kwargs
+
         self.name = hintname
         self.pkgs = []
 
@@ -562,7 +564,8 @@ class Hint(object):
                 suite = self.SOURCE_DISTRIBUTION
             except AttributeError:
                 suite = 'unstable'
-            if not projectb.is_source_pkg(suite, src):
+            if not projectb.is_source_pkg(suite, src) and \
+              ('noupdate' not in kwargs or kwargs['noupdate'] != 1):
                 tmpsrc = src
                 src = projectb.get_source_pkg(suite, src)
                 if src is None:
@@ -634,7 +637,7 @@ class Hint(object):
             words = [ ]
         return ' '.join(words)
 
-    def __new__(cls, hintname, *args):
+    def __new__(cls, hintname, *args, **kwargs):
         if hintname in \
                 [ 'easy', 'hint', 'force', 'force-hint', 'unblock', 'urgent', 'unblock-udeb' ]:
             subclass = UnstableToTestingHint
@@ -685,8 +688,8 @@ class UnversionedHint(Hint):
 class MigrationHint(Hint):
     """A hint whose packages are meant to move from one distribution to another."""
 
-    def __init__(self, *args):
-        Hint.__init__(self, *args)
+    def __init__(self, *args, **kwargs):
+        Hint.__init__(self, *args, **kwargs)
         self._versions = None
 
     @property
@@ -699,12 +702,13 @@ class MigrationHint(Hint):
             'target': get_version_numbers(self.TARGET_DISTRIBUTION, self.pkgs),
         }
 
-        for pkg, ver in self._versions['source'].items():
-            if ver is None and pkg not in self.remove_pkgs:
-                print >>sys.stderr, (
-                    'W: package %s not in %s, skipping.'
-                    % (pkg, self.SOURCE_DISTRIBUTION))
-                self.pkgs.remove(pkg)
+        if 'noupdate' not in self.kwargs or self.kwargs['noupdate'] != 1:
+            for pkg, ver in self._versions['source'].items():
+                if ver is None and pkg not in self.remove_pkgs:
+                    print >>sys.stderr, (
+                        'W: package %s not in %s, skipping.'
+                        % (pkg, self.SOURCE_DISTRIBUTION))
+                    self.pkgs.remove(pkg)
 
         return self._versions
 
@@ -783,8 +787,8 @@ class TpuToTestingHint(MigrationHint):
 
 
 class AgeDaysHint(UnstableToTestingHint):
-    def __init__(self, hintname, days, *args):
-        UnstableToTestingHint.__init__(self, '%s %s' % (hintname, days), *args)
+    def __init__(self, hintname, days, *args, **kwargs):
+        UnstableToTestingHint.__init__(self, '%s %s' % (hintname, days), *args, **kwargs)
 
 
 class RemovalHint(Hint):
@@ -865,7 +869,7 @@ class HintfileParagraph(object):
             else:
                 if seen_hint and split_out:
                     self._groups.append([])
-                hint = Hint(*line.strip().split())
+                hint = Hint(*line.strip().split(), noupdate=1)
                 self._groups[-1].append(hint)
                 seen_hint = True