move APT::Never-MarkAuto-Sections handling to MarkDelete

Having the handling in MarkInstall means that it just effects
installation of the metapackage, but if the dependencies change the new
dependencies aren't protected (and the old dependencies are still
protected for no 'reason'). Having it in MarkDelete means that if a
metapackage is sheduled for removal all its currently installed
dependencies are marked as manual, which helps against both as in this
case there is no new/old and additionally if a user decides the
installation of a metapackage was wrong he can just remove it
explicitely avoid the manual marking entirely.

3 files changed, 66 insertions, 49 deletions

diff --git a/apt-pkg/depcache.cc b/apt-pkg/depcache.cc
index e466cba28..99e694a06 100644
--- a/apt-pkg/depcache.cc
+++ b/apt-pkg/depcache.cc
@@ -836,6 +836,41 @@ bool pkgDepCache::MarkDelete(PkgIterator const &Pkg, bool rPurge,
 
    ActionGroup group(*this);
 
+   if (FromUser == false)
+   {
+      VerIterator const PV = P.InstVerIter(*this);
+      if (PV.end() == false)
+      {
+	 // removed metapackages mark their dependencies as manual to prevent in "desktop depends browser, texteditor"
+	 // the removal of browser to suggest the removal of desktop and texteditor.
+	 // We ignore the auto-bit here as we can't deal with metapackage cascardes otherwise.
+	 // We do not check for or-groups here as we don't know which package takes care of
+	 // providing the feature the user likes e.g.:  browser1 | browser2 | browser3
+	 // Temporary removals are effected by this as well, which is bad, but unlikely in practice
+	 bool const PinNeverMarkAutoSection = (PV->Section != 0 && ConfigValueInSubTree("APT::Never-MarkAuto-Sections", PV.Section()));
+	 if (PinNeverMarkAutoSection)
+	 {
+	    for (DepIterator D = PV.DependsList(); D.end() != true; ++D)
+	    {
+	       if (D.IsMultiArchImplicit() == true || D.IsNegative() == true || IsImportantDep(D) == false)
+		  continue;
+
+	       pkgCacheFile CacheFile(this);
+	       APT::VersionList verlist = APT::VersionList::FromDependency(CacheFile, D, APT::CacheSetHelper::INSTALLED);
+	       for (auto const &V : verlist)
+	       {
+		  PkgIterator const DP = V.ParentPkg();
+		  if(DebugAutoInstall == true)
+		     std::clog << OutputInDepth(Depth) << "Setting " << DP.FullName(false) << " NOT as auto-installed (direct "
+			<< D.DepType() << " of " << Pkg.FullName(false) << " which is in APT::Never-MarkAuto-Sections)" << std::endl;
+
+		  MarkAuto(DP, false);
+	       }
+	    }
+	 }
+      }
+   }
+
    if (DebugMarker == true)
       std::clog << OutputInDepth(Depth) << (rPurge ? "MarkPurge " : "MarkDelete ") << Pkg << " FU=" << FromUser << std::endl;
 
@@ -1096,7 +1131,6 @@ bool pkgDepCache::MarkInstall(PkgIterator const &Pkg,bool AutoInst,
    VerIterator const PV = P.InstVerIter(*this);
    if (unlikely(PV.end() == true))
       return false;
-   bool const PinNeverMarkAutoSection = (PV->Section != 0 && ConfigValueInSubTree("APT::Never-MarkAuto-Sections", PV.Section()));
 
    DepIterator Dep = PV.DependsList();
    for (; Dep.end() != true;)
@@ -1210,14 +1244,6 @@ bool pkgDepCache::MarkInstall(PkgIterator const &Pkg,bool AutoInst,
 	       verlist.erase(InstVer);
 	       continue;
 	    }
-	    // now check if we should consider it a automatic dependency or not
-	    if(InstPkg->CurrentVer == 0 && PinNeverMarkAutoSection)
-	    {
-	       if(DebugAutoInstall == true)
-		  std::clog << OutputInDepth(Depth) << "Setting NOT as auto-installed (direct "
-                            << Start.DepType() << " of pkg in APT::Never-MarkAuto-Sections)" << std::endl;
-	       MarkAuto(InstPkg, false);
-	    }
 	    break;
 	 } while(true);
 	 continue;
diff --git a/test/integration/framework b/test/integration/framework
index 2f08c5fdc..2efe7439e 100644
--- a/test/integration/framework
+++ b/test/integration/framework
@@ -1324,6 +1324,17 @@ testmarkedauto() {
 	fi
 	aptmark showauto 2>&1 | checkdiff $COMPAREFILE - && msgpass || msgfail
 }
+testmarkedmanual() {
+	local COMPAREFILE="${TMPWORKINGDIRECTORY}/rootdir/tmp/testmarkedmanual.comparefile"
+	if [ -n "$1" ]; then
+		msgtest 'Test for correctly marked as manually installed' "$*"
+		while [ -n "$1" ]; do echo "$1"; shift; done | sort > $COMPAREFILE
+	else
+		msgtest 'Test for correctly marked as manually installed' 'no package'
+		echo -n > $COMPAREFILE
+	fi
+	aptmark showmanual 2>&1 | checkdiff $COMPAREFILE - && msgpass || msgfail
+}
 
 msgfailoutput() {
 	local MSG="$1"
diff --git a/test/integration/test-apt-never-markauto-sections b/test/integration/test-apt-never-markauto-sections
index 6c88c69fa..a469b4c15 100755
--- a/test/integration/test-apt-never-markauto-sections
+++ b/test/integration/test-apt-never-markauto-sections
@@ -9,22 +9,6 @@ configarchitecture 'amd64' 'i386'
 aptconfig dump --no-empty --format '%v%n' APT::Never-MarkAuto-Sections > nevermarkauto.sections
 testsuccess grep '^metapackages$' nevermarkauto.sections
 
-# this is a very crude regression test, not a "this is how it should be" test:
-# In theory mydesktop-core and texteditor should be marked as manual, but
-# texteditor is installed as a dependency of bad-texteditor, not of
-# mydesktop-core and mydesktop-core is removed while bad-texteditor is
-# installed losing the manual bit as the problem resolver will later decide to
-# drop bad-texteditor and re-instate mydesktop-core which is considered an
-# auto-install at that point (in theory the never-auto handling should be
-# copied to this place – as to the many other places dependencies are resolved
-# 'by hand' instead of via MarkInstall AutoInst…
-#
-# Both could be fixed if apt would figure out early that installing
-# bad-texteditor is a bad idea and eventually it should (as mydesktop-core is
-# a direct descendant of mydesktop which was a user-request mydesktop-core should
-# be as protected from removal as mydesktop is), but this is hard in the general case
-# as with more or-groups and provides you can produce 'legal' examples for this.
-
 buildsimplenativepackage 'mydesktop' 'all' '1' 'unstable' 'Depends: mydesktop-core, foreignpkg
 Recommends: notavailable' '' 'metapackages'
 buildsimplenativepackage 'mydesktop-core' 'amd64' '1' 'unstable' 'Depends: bad-texteditor | texteditor, browser (>= 42), nosection, foreignpkg
@@ -45,21 +29,21 @@ testequal 'dpkg' aptmark showmanual
 
 testsuccess aptget install mydesktop -y -o Debug::pkgProblemResolver=1 -o Debug::pkgDepCache::Marker=1
 
-testequal 'browser
-dpkg
-foreignpkg:i386
-mydesktop
-nosection' aptmark showmanual
-testmarkedauto 'mydesktop-core' 'texteditor'
+testmarkedmanual 'dpkg' 'mydesktop'
+testmarkedauto 'mydesktop-core' 'foreignpkg:i386' 'texteditor' 'browser' 'nosection'
 
+# if the remove is from a user, don't do manual-bit passing
 testequal 'Reading package lists...
 Building dependency tree...
 Reading state information...
 The following packages will be REMOVED:
-  mydesktop mydesktop-core texteditor
-0 upgraded, 0 newly installed, 3 to remove and 0 not upgraded.
+  browser foreignpkg:i386 mydesktop mydesktop-core nosection texteditor
+0 upgraded, 0 newly installed, 6 to remove and 0 not upgraded.
 Remv mydesktop [1]
 Remv mydesktop-core [1]
+Remv browser [42]
+Remv foreignpkg:i386 [1]
+Remv nosection [1]
 Remv texteditor [1]' aptget autoremove mydesktop -s
 
 testequal 'Reading package lists...
@@ -70,37 +54,33 @@ The following packages will be REMOVED:
 0 upgraded, 0 newly installed, 3 to remove and 0 not upgraded.
 Remv mydesktop [1]
 Remv mydesktop-core [1]
-Remv texteditor [1]' aptget autoremove texteditor -s
+Remv texteditor [1]' aptget autoremove texteditor -s #-o Debug::pkgDepCache::AutoInstall=1 -o Debug::pkgProblemResolver=1 -o Debug::pkgDepCache::Marker=1
 testsuccess aptget autoremove texteditor -y
 
 testdpkgnotinstalled mydesktop mydesktop-core texteditor
 testdpkginstalled browser
 
-testequal 'browser
-dpkg
-foreignpkg:i386
-nosection' aptmark showmanual
+testmarkedmanual 'browser' 'dpkg' 'foreignpkg:i386' 'nosection'
 testmarkedauto
 
 # test that installed/upgraded auto-pkgs are not set to manual
 
 testsuccess aptget install browser=41 -y --force-yes
 
-testequal 'browser
-dpkg
-foreignpkg:i386
-nosection' aptmark showmanual
+testmarkedmanual 'browser' 'dpkg' 'foreignpkg:i386' 'nosection'
 testmarkedauto
 testsuccess aptmark auto browser
 testmarkedauto 'browser'
 testsuccess aptmark auto nosection
 testmarkedauto 'browser' 'nosection'
-testequal 'dpkg
-foreignpkg:i386' aptmark showmanual
+testmarkedmanual 'dpkg' 'foreignpkg:i386'
+
+# nosection should be auto, not manual, but is marked as such by the resolver
+# removing mydesktop-core temporally… the resolver should be figuring out here
+# that there is no point of removing mydesktop-core as its an unavoidable
+# dependency of the user-requested mydesktop
 
-testsuccess aptget install mydesktop -y
+testsuccess aptget install mydesktop -y -o Debug::pkgProblemResolver=1 -o Debug::pkgDepCache::Marker=1 -o Debug::pkgDepCache::AutoInstall=1
 
-testequal 'dpkg
-foreignpkg:i386
-mydesktop' aptmark showmanual
-testmarkedauto 'browser' 'nosection' 'mydesktop-core' 'texteditor'
+testmarkedmanual 'dpkg' 'foreignpkg:i386' 'mydesktop' 'nosection'
+testmarkedauto 'browser' 'mydesktop-core' 'texteditor'