solver3: Fix test-resolve-by-keep-new-recommend

Reorder the if branches a bit to simplify the understanding
of the promotions, and allow us to keep track of which
dependency existed in the installed version.

Change the rule for promoting new recommends to only promote
new recommends to depends in *upgrade* and not *dist-upgrade*
per the test-resolve-by-keep-new-recommends test case.

This makes some sense: In a dist-upgrade, the solver could
otherwise decide to remove an installed package if both the
following hold:

(1) something needs the upgraded version | something else
(2) the upgraded version is not installable due to unsat recommends

3 files changed, 40 insertions, 21 deletions

diff --git a/apt-pkg/solver3.cc b/apt-pkg/solver3.cc
index f992d55b4..cebface27 100644
--- a/apt-pkg/solver3.cc
+++ b/apt-pkg/solver3.cc
@@ -831,52 +831,54 @@ APT::Solver::Clause APT::Solver::TranslateOrGroup(pkgCache::DepIterator start, p
    if (clause.optional && start.ParentPkg()->CurrentVer)
    {
       bool important = policy.IsImportantDep(start);
-      bool newOptional = true;
-      bool wasImportant = false;
       auto importantToKeep = [this](pkgCache::DepIterator d)
       {
 	 return policy.IsImportantDep(d) || (KeepRecommends && d->Type == pkgCache::Dep::Recommends) || (KeepSuggests && d->Type == pkgCache::Dep::Suggests);
       };
+      bool satisfied = std::any_of(clause.solutions.begin(), clause.solutions.end(), [this](auto var)
+				   { return var.Pkg(cache) ? var.Pkg(cache)->CurrentVer != nullptr : Var(var.CastPkg(cache).CurrentVer()) == var; });
 
+      // Find the existing dependency
+      pkgCache::DepIterator existing;
       for (auto D = start.ParentPkg().CurrentVer().DependsList(); not D.end(); D++)
 	 if (not D.IsCritical() && importantToKeep(D) && D.TargetPkg() == start.TargetPkg())
 	 {
-	    newOptional = false, wasImportant = policy.IsImportantDep(D);
+	    existing = D;
 	    break;
 	 }
 
-      bool satisfied = std::any_of(clause.solutions.begin(), clause.solutions.end(), [this](auto var)
-				   { return var.Pkg(cache) ? var.Pkg(cache)->CurrentVer != nullptr : Var(var.CastPkg(cache).CurrentVer()) == var; });
-
-      if (important && wasImportant && not newOptional && not satisfied)
+      if (not existing.end() && not important && importantToKeep(start) && satisfied)
       {
 	 if (unlikely(debug >= 3))
-	    std::cerr << "Ignoring unsatisfied Recommends " << clause.toString(cache) << std::endl;
-	 clause.solutions.clear();
+	    std::cerr << "Try to keep satisfied: " << clause.toString(cache, true) << std::endl;
       }
-      else if (not important && not wasImportant && not newOptional && satisfied)
+      else if (not important)
       {
 	 if (unlikely(debug >= 3))
-	    std::cerr << "Promoting satisfied Suggests to Recommends: " << clause.toString(cache) << std::endl;
-	 important = true;
+	    std::cerr << "Ignore unimportant clause: " << clause.toString(cache, true) << std::endl;
+	 return Clause{reason, Group::Satisfy, true};
       }
-      else if (satisfied && important && wasImportant && clause.solutions.size() > 0)
+      else if (not existing.end() && policy.IsImportantDep(existing) && not satisfied)
       {
 	 if (unlikely(debug >= 3))
-	    std::cerr << "Promoting existing Recommends " << clause.toString(cache) << " to depends in upgrade" << std::endl;
-	 clause.optional = false;
+	    std::cerr << "Ignoring unsatisfied clause: " << clause.toString(cache, true) << std::endl;
+	 return Clause{reason, Group::Satisfy, true};
       }
-      else if (newOptional && important && reason.Ver() && clause.solutions.size() > 0 && reason.Ver(cache) != reason.CastPkg(cache).CurrentVer() && IsUpgrade)
+      else if (IsUpgrade && not existing.end() && satisfied)
       {
 	 if (unlikely(debug >= 3))
-	    std::cerr << "Promoting new Recommends " << clause.toString(cache) << " to depends in upgrade" << std::endl;
+	    std::cerr << "Promoting previously satisfied clause to hard dependency: " << clause.toString(cache, true) << std::endl;
 	 clause.optional = false;
       }
-      else if (not important)
+      else if (
+	 IsUpgrade && not(AllowRemove && AllowInstall)				    // promote Recommends to Depends in upgrade, not in dist-upgrade.
+	 && reason.Ver() && reason.Ver(cache) != reason.CastPkg(cache).CurrentVer() // and if this an upgrade to an installed package
+	 && (existing.end() || not policy.IsImportantDep(existing))		    // new Recommends, or upgraded from Suggests
+      )
       {
 	 if (unlikely(debug >= 3))
-	    std::cerr << "Ignoring Suggests " << clause.toString(cache) << std::endl;
-	 return Clause{reason, Group::Satisfy, true};
+	    std::cerr << "Promoting new clause to hard dependency: " << clause.toString(cache) << std::endl;
+	 clause.optional = false;
       }
    }
 
diff --git a/test/integration/solver3.broken b/test/integration/solver3.broken
index 4a4e1532b..0e2005d60 100644
--- a/test/integration/solver3.broken
+++ b/test/integration/solver3.broken
@@ -3,6 +3,5 @@ test-apt-get-upgrade-by-source                              # TBD: Upgrading by
 test-apt-install-order-matters-a-bit                        # Wontfix: Cannot fix, the order is not recorded in the depcache
 test-bug-470115-new-and-tighten-recommends                  # TBD: Calculation of what is already satisfied Recommends is broken
 test-prevent-markinstall-multiarch-same-versionscrew        # TBD: We consider the skewed ones obsolete and remove them...
-test-resolve-by-keep-new-recommends                         # TBD: Fixing this seems to break test-bug-591882-conkeror, why?
 test-resolve-by-keep-obsolete-removals                      # TBD: ResolveByKeep() usage is badly aligned here
 test-ubuntu-bug-1304403-obsolete-priority-standard          # TBD: Solver3 here happily removes 10 deps to upgrade a package
diff --git a/test/integration/test-resolve-by-keep-new-recommends b/test/integration/test-resolve-by-keep-new-recommends
index 3591ed84d..16acbe0ce 100755
--- a/test/integration/test-resolve-by-keep-new-recommends
+++ b/test/integration/test-resolve-by-keep-new-recommends
@@ -19,3 +19,21 @@ The following packages have been kept back:
 0 upgraded, 0 newly installed, 0 to remove and 1 not upgraded."
 testsuccessequal "$UPGRADE_KEEP" aptget upgrade -s
 
+UPGRADE_KEEP="Reading package lists...
+Building dependency tree...
+Calculating upgrade...
+The following packages have been kept back:
+  foo
+0 upgraded, 0 newly installed, 0 to remove and 1 not upgraded."
+testsuccessequal "$UPGRADE_KEEP" apt upgrade -s
+
+UPGRADE_KEEP="Reading package lists...
+Building dependency tree...
+Calculating upgrade...
+The following packages will be upgraded:
+  foo
+1 upgraded, 0 newly installed, 0 to remove and 0 not upgraded.
+Inst foo [1.0] (2.0 unstable [i386])
+Conf foo (2.0 unstable [i386])"
+testsuccessequal "$UPGRADE_KEEP" aptget dist-upgrade -s
+