solver3: Refactor (rewrite) FromDepCache()

Refactor the various Install requests into a single one and move
some stuff around, this should be a bit easier to maintain and
we don't repeat ourselves all the time.

This accidentally uncovered some bugs in the code, that get fixed
by this:

- The UpgradeManual and InstallManual groups did not work correctly,
  Essential packages were Install and others were Upgrade.

- Automatically installed packages falsely got promoted to the
  manual groups as they got promoted into the manual code path,
  whenever we ran without allowing their removal.

- The combination of --mark-auto --auto-remove now works correctly

  We previously skipped the installation of automatically installed
  packages when we did autoremoval, but we did not consider if the
  action was protected, that is specified on the command-line.

  Now we do not autoremove protected actions, and have added a test
  case for it.

- The AllowRemoveManual option no longer is applied if removals are
  generally denied.

Closes: #1071519

5 files changed, 58 insertions, 41 deletions

diff --git a/apt-pkg/solver3.cc b/apt-pkg/solver3.cc
index f7f72b1a4..7580d6e18 100644
--- a/apt-pkg/solver3.cc
+++ b/apt-pkg/solver3.cc
@@ -906,10 +906,7 @@ bool APT::Solver::Solve()
 // \brief Apply the selections from the dep cache to the solver
 bool APT::Solver::FromDepCache(pkgDepCache &depcache)
 {
-   bool KeepAuto = not _config->FindB("APT::Get::AutomaticRemove");
-   bool AllowRemove = _config->FindB("APT::Solver::Remove", true);
-   bool AllowInstall = _config->FindB("APT::Solver::Install", true);
-   bool AllowRemoveManual = _config->FindB("APT::Solver::RemoveManual", false);
+   bool AllowRemoveManual = AllowRemove && _config->FindB("APT::Solver::RemoveManual", false);
    DefaultRootSetFunc2 rootSet(&cache);
 
    for (auto P = cache.PkgBegin(); not P.end(); P++)
@@ -918,10 +915,6 @@ bool APT::Solver::FromDepCache(pkgDepCache &depcache)
 	 continue;
 
       auto state = depcache[P];
-      auto maybeInstall = state.Install() || (state.Keep() && P->CurrentVer);
-      auto reject = state.Delete() || (depcache[P].Keep() && not P->CurrentVer && depcache[P].Protect());
-      auto isAuto = (depcache[P].Flags & pkgCache::Flag::Auto);
-      auto isOptional = isAuto || (AllowRemoveManual && not depcache[P].Protect());
       if (P->SelectedState == pkgCache::State::Hold && not state.Protect())
       {
 	 if (unlikely(debug >= 1))
@@ -929,45 +922,46 @@ bool APT::Solver::FromDepCache(pkgDepCache &depcache)
 	 if (P->CurrentVer ? not Install(P.CurrentVer(), {}, Group::HoldOrDelete) : not Reject(P, {}, Group::HoldOrDelete))
 	    return false;
       }
-      else if (reject)
+      else if (state.Delete()						  // Normal delete request.
+	       || (not P->CurrentVer && state.Keep() && state.Protect())  // Delete request of not installed package.
+	       || (not P->CurrentVer && state.Keep() && not AllowInstall) // New package installs not allowed.
+      )
       {
 	 if (unlikely(debug >= 1))
 	    std::cerr << "Delete " << P.FullName() << "\n";
 	 if (!Reject(P, {}, Group::HoldOrDelete))
 	    return false;
       }
-      else if (maybeInstall && P->Flags & (pkgCache::Flag::Essential | pkgCache::Flag::Important))
-      {
-	 if (unlikely(debug >= 1))
-	    std::cerr << "ESSENTIAL " << P.FullName() << "\n";
-	 if (depcache[P].Keep() ? not Install(P, {}, Group::InstallManual) : not Install(depcache.GetCandidateVersion(P), {}, Group::InstallManual))
-	    return false;
-      }
-      else if (maybeInstall && not isOptional)
-      {
-	 auto Upgrade = depcache.GetCandidateVersion(P) != P.CurrentVer();
-	 auto Group = (Upgrade ? Group::UpgradeManual : Group::InstallManual);
-	 if (unlikely(debug >= 1))
-	    std::cerr << "MANUAL " << P.FullName() << "\n";
-	 if (depcache[P].Keep() ? not Install(P, {}, Group) : not Install(depcache.GetCandidateVersion(P), {}, Group))
-	    return false;
-      }
-      else if (maybeInstall && isOptional && (KeepAuto || rootSet.InRootSet(P) || not isAuto))
+      else if (state.Install() || (state.Keep() && P->CurrentVer))
       {
+	 auto isEssential = P->Flags & (pkgCache::Flag::Essential | pkgCache::Flag::Important);
+	 auto isAuto = (depcache[P].Flags & pkgCache::Flag::Auto);
+	 auto isOptional = ((isAuto && AllowRemove) || AllowRemoveManual) && not isEssential && not depcache[P].Protect();
+	 auto Root = rootSet.InRootSet(P);
 	 auto Upgrade = depcache.GetCandidateVersion(P) != P.CurrentVer();
 	 auto Group = isAuto ? (Upgrade ? Group::UpgradeAuto : Group::KeepAuto)
 			     : (Upgrade ? Group::UpgradeManual : Group::InstallManual);
+
+	 if (isAuto && not depcache[P].Protect() && not isEssential && not KeepAuto && not rootSet.InRootSet(P))
+	 {
+	    if (unlikely(debug >= 1))
+	       std::cerr << "Ignore automatic install " << P.FullName() << " (" << (isEssential ? "E" : "") << (isAuto ? "M" : "") << (Root ? "R" : "") << ")"
+			 << "\n";
+	    continue;
+	 }
 	 if (unlikely(debug >= 1))
-	    std::cerr << "AUTOMATIC " << P.FullName() << (Upgrade ? " - upgrade" : "") << "\n";
+	    std::cerr << "Install " << P.FullName() << " (" << (isEssential ? "E" : "") << (isAuto ? "M" : "") << (Root ? "R" : "") << ")"
+		      << "\n";
 
-	 if (not AllowRemove)
+	 if (not isOptional)
 	 {
+	    // Pre-empt the non-optional requests, as we don't want to queue them, we can just "unit propagate" here.
 	    if (depcache[P].Keep() ? not Install(P, {}, Group) : not Install(depcache.GetCandidateVersion(P), {}, Group))
 	       return false;
 	 }
 	 else
 	 {
-	    Work w{Reason(), depth(), Group, true, Upgrade};
+	    Work w{Reason(), depth(), Group, isOptional, Upgrade};
 	    for (auto V = P.VersionList(); not V.end(); ++V)
 	       if (IsAllowedVersion(V))
 		  w.solutions.push_back(V);
@@ -975,13 +969,6 @@ bool APT::Solver::FromDepCache(pkgDepCache &depcache)
 	    AddWork(std::move(w));
 	 }
       }
-      else if (P->CurrentVer == 0 && not AllowInstall)
-      {
-	 if (unlikely(debug >= 1))
-	    std::cerr << "NOT ALLOWING INSTALL OF " << P.FullName() << "\n";
-	 if (not Reject(P, {}, Group::HoldOrDelete))
-	    return false;
-      }
    }
 
    return true;
diff --git a/test/integration/test-apt-get-mark-auto b/test/integration/test-apt-get-mark-auto
new file mode 100755
index 000000000..844412e67
--- /dev/null
+++ b/test/integration/test-apt-get-mark-auto
@@ -0,0 +1,30 @@
+#!/bin/sh
+set -e
+
+TESTDIR="$(readlink -f "$(dirname "$0")")"
+. "$TESTDIR/framework"
+setupenvironment
+configarchitecture 'amd64'
+
+insertpackage 'unstable' 'package' 'all' '3'
+
+setupaptarchive
+
+testsuccessequal "Reading package lists...
+Building dependency tree...
+The following NEW packages will be installed:
+  package
+0 upgraded, 1 newly installed, 0 to remove and 0 not upgraded.
+Inst package (3 unstable [all])
+Conf package (3 unstable [all])" aptget install package --mark-auto -s
+
+# Specifically if we mark the package as automatically installed but also have auto-remove on,
+# the package should still be installed...
+
+testsuccessequal "Reading package lists...
+Building dependency tree...
+The following NEW packages will be installed:
+  package
+0 upgraded, 1 newly installed, 0 to remove and 0 not upgraded.
+Inst package (3 unstable [all])
+Conf package (3 unstable [all])" aptget install package --mark-auto --auto-remove -s
diff --git a/test/integration/test-bug-549968-install-depends-of-not-installed b/test/integration/test-bug-549968-install-depends-of-not-installed
index 39c86cc8d..f575f9940 100755
--- a/test/integration/test-bug-549968-install-depends-of-not-installed
+++ b/test/integration/test-bug-549968-install-depends-of-not-installed
@@ -32,7 +32,7 @@ Conf coolstuff (1.0 unstable [all])" aptget install coolstuff extracoolstuff- -o
 testsuccessequal "Reading package lists...
 Building dependency tree...
 Package 'extracoolstuff' is not installed, so not removed
-Solving dependencies...MANUAL coolstuff:i386
+Solving dependencies...Install coolstuff:i386 ()
 [0] Install:coolstuff:i386=1.0 ()
 Delete extracoolstuff:i386
 [0] Reject:extracoolstuff:i386 ()
diff --git a/test/integration/test-bug-960705-propagate-protected-to-satisfied-depends b/test/integration/test-bug-960705-propagate-protected-to-satisfied-depends
index da11fc881..6ffb69bf5 100755
--- a/test/integration/test-bug-960705-propagate-protected-to-satisfied-depends
+++ b/test/integration/test-bug-960705-propagate-protected-to-satisfied-depends
@@ -20,7 +20,7 @@ setupaptarchive
 
 testsuccessequal "Reading package lists...
 Building dependency tree...
-Solving dependencies...MANUAL foobar:amd64
+Solving dependencies...Install foobar:amd64 ()
 [0] Install:foobar:amd64=1 ()
 [0] Install:requires-foo:amd64=1 (foobar:amd64=1)
 [0] Install:foo:amd64=1 (foobar:amd64=1 -> requires-foo:amd64=1)
diff --git a/test/integration/test-dpkg-i-apt-install-fix-broken b/test/integration/test-dpkg-i-apt-install-fix-broken
index 5c517aaef..a4c3c66d9 100755
--- a/test/integration/test-dpkg-i-apt-install-fix-broken
+++ b/test/integration/test-dpkg-i-apt-install-fix-broken
@@ -14,13 +14,13 @@ setupaptarchive
 testfailure dpkg -i incoming/autopkgtest-*.deb
 testsuccessequal 'Reading package lists...
 Building dependency tree...
-Correcting dependencies...MANUAL autopkgtest-satdep:amd64
+Correcting dependencies...Install autopkgtest-satdep:amd64 ()
 [0] Install:autopkgtest-satdep:amd64 ()
 [0] Install:autopkgtest-satdep:amd64=1 (autopkgtest-satdep:amd64)
 [0] Install:debhelper:amd64=1 (autopkgtest-satdep:amd64 -> autopkgtest-satdep:amd64=1)
  Done
-Solving dependencies...AUTOMATIC debhelper:amd64 - upgrade
-MANUAL autopkgtest-satdep:amd64
+Solving dependencies...Install debhelper:amd64 (M)
+Install autopkgtest-satdep:amd64 ()
 [0] Install:autopkgtest-satdep:amd64 ()
 [0] Install:autopkgtest-satdep:amd64=1 (autopkgtest-satdep:amd64)
 [0] Install:debhelper:amd64=1 (autopkgtest-satdep:amd64 -> autopkgtest-satdep:amd64=1)