Fix failure to fetch git repository location with tagged commit id for git 2.22

Also make some cleanups (always probe URLs prior to git-ls-remote, use peeled
reference ids to identify repository fragments, etc).

17 files changed, 110 insertions, 35 deletions

diff --git a/bpkg/fetch-git.cxx b/bpkg/fetch-git.cxx
index 63ee09d..5ffc9a6 100644
--- a/bpkg/fetch-git.cxx
+++ b/bpkg/fetch-git.cxx
@@ -107,10 +107,9 @@ namespace bpkg
   }
 
   // Start git process. On the first call check that git version is 2.11.0 or
-  // above, and fail if that's not the case. Note that the full functionality
-  // (such as being able to fetch unadvertised commits) requires 2.14.0. And
-  // supporting versions prior to 2.11.0 doesn't seem worth it (plus other
-  // parts of the toolchain also requires 2.11.0).
+  // above, and fail if that's not the case. Note that supporting earlier
+  // versions doesn't seem worth it (plus other parts of the toolchain also
+  // require 2.11.0).
   //
   // Also note that git is executed in the "sanitized" environment, having the
   // environment variables that are local to the repository being unset (all
@@ -790,6 +789,31 @@ namespace bpkg
 
       return r;
     }
+
+    // Return the peeled reference for an annotated tag and the original
+    // reference if it is not an annotated tag or is already peeled.
+    //
+    const ref&
+    peel (const ref& rf) const
+    {
+      if (rf.name.compare (0, 10, "refs/tags/") == 0 && !rf.peeled)
+      {
+        for (const ref& r: *this)
+        {
+          if (r.peeled && r.name == rf.name)
+            return r;
+        }
+
+        // Fall through.
+        //
+        // Presumably is a lightweight tag reference containing the commit id.
+        // Note that git-ls-remote output normally contains peeled references
+        // for all annotated tags.
+        //
+      }
+
+      return rf;
+    }
   };
 
   // Map of repository URLs to their advertized refs/commits.
@@ -971,6 +995,13 @@ namespace bpkg
       }
     case capabilities::smart:
       {
+        // Note that the git server communicating with the client using the
+        // protocol version 2 always supports unadvertised refs fetch (see the
+        // "advertised commit fetch using commit id fails" git bug report for
+        // details). We ignore this fact (because currently this is disabled
+        // by default, even if both support version 2) but may rely on it in
+        // the future.
+        //
         return !rf.commit || commit_advertized (co, url, *rf.commit);
       }
     case capabilities::unadv:
@@ -1020,16 +1051,27 @@ namespace bpkg
       return *cap;
     };
 
-    auto references = [&co, &url] (const string& refname, bool abbr_commit)
+    auto references = [&co, &url, &caps] (const string& refname,
+                                          bool abbr_commit)
       -> refs::search_result
     {
+      // Make sure the URL is probed before running git-ls-remote (see
+      // load_refs() for details).
+      //
+      caps ();
+
       return load_refs (co, url ()).search_names (refname, abbr_commit);
     };
 
     // Return the default reference set (see repository-types(1) for details).
     //
-    auto default_references = [&co, &url] () -> refs::search_result
+    auto default_references = [&co, &url, &caps] () -> refs::search_result
     {
+      // Make sure the URL is probed before running git-ls-remote (see
+      // load_refs() for details).
+      //
+      caps ();
+
       refs::search_result r;
       for (const ref& rf: load_refs (co, url ()))
       {
@@ -1146,7 +1188,9 @@ namespace bpkg
                             ? default_references ()
                             : references (*rf.name, true /* abbr_commit */))
         {
-          const string& c (r.get ().commit);
+          // Reduce the reference to the commit id.
+          //
+          const string& c (load_refs (co, url ()).peel (r).commit);
 
           if (!rf.exclusion)
           {
@@ -1180,7 +1224,7 @@ namespace bpkg
       {
         assert (!rf.exclusion); // Already handled.
 
-        add_spec (*rf.commit, strings ({*rf.commit}), true);
+        add_spec (*rf.commit, strings ({*rf.commit}), true /* shallow */);
       }
       // If the shallow fetch is not possible for the commit but the refname
       // containing the commit is specified, then we fetch the whole history
@@ -1313,41 +1357,35 @@ namespace bpkg
     // Fetch the refspecs. If no refspecs are specified, then fetch the
     // whole repository history.
     //
-    auto fetch = [&co, &url, &dir] (const strings& refspecs, bool shallow)
+    auto fetch = [&co, &url, &dir, &caps] (const strings& refspecs,
+                                           bool shallow)
     {
       // We don't shallow fetch the whole repository.
       //
       assert (!refspecs.empty () || !shallow);
 
-      // Prior to 2.14.0 the git-fetch command didn't accept commit id as a
-      // refspec:
-      //
-      // $ git fetch --no-recurse-submodules --depth 1 origin 5e8245ee3526530a3467f59b0601bbffb614f45b
-      //   error: Server does not allow request for unadvertised object 5e8245ee3526530a3467f59b0601bbffb614f45b
-      //
-      // We will try to remap commits back to git refs (tags, branches, etc)
-      // based on git-ls-remote output and fail if unable to do so (which
-      // should only happen for unadvertised commits).
-      //
-      // Note that in this case we will fail only for servers supporting
-      // unadvertised refs fetch. For other protocols we have already fallen
-      // back to fetching some history, passing to fetch() either advertised
-      // commit ids (of branches, tags, etc) or an empty refspecs list (the
-      // whole repository history). So we could just reduce the server
-      // capabilities from 'unadv' to 'smart' for such old clients.
+      // Note that peeled references present in git-ls-remote output are not
+      // advertised, so we need to "unpeel" them back to the annotated tag
+      // references. Also note that prior to 2.14.0 the git-fetch command
+      // didn't accept commit id as a refspec for advertised commits except
+      // for servers with dumb or unadv capabilities. Seems that remapping
+      // reference ids back to git refs (tags, branches, etc) is harmless and
+      // works consistently across different git versions. That's why we will
+      // always remap except for the server with the unadv capabilities, where
+      // we may not have a name to remap to. That seems OK as no issues with
+      // using commit ids for fetching from such servers were encountered so
+      // far.
       //
       optional<strings> remapped_refspecs;
-      if (!refspecs.empty () && git_ver < semantic_version {2, 14, 0})
+
+      if (!refspecs.empty () && caps () != capabilities::unadv)
       {
         remapped_refspecs = strings ();
 
         for (const string& c: refspecs)
         {
           const ref* r (load_refs (co, url ()).find_commit (c));
-
-          if (r == nullptr)
-            fail << "git version is too old for specified location" <<
-              info << "consider upgrading git to 2.14.0 or above";
+          assert (r != nullptr); // Otherwise we would fail earlier.
 
           remapped_refspecs->push_back (r->name);
         }
diff --git a/tests/common/git/init b/tests/common/git/init
index 9519779..8cac2da 100755
--- a/tests/common/git/init
+++ b/tests/common/git/init
@@ -114,8 +114,10 @@ git -C style-basic.git checkout master
 #
 git -C libbar.git init
 git -C libbar.git add '*'
-git -C libbar.git submodule add -b stable ../style-basic.git extras
 git -C libbar.git commit -am 'Create'
+git -C libbar.git tag -a 'v1.0.0' -m 'Tag version 1.0.0'
+git -C libbar.git submodule add -b stable ../style-basic.git extras
+git -C libbar.git commit -am 'Add extras'
 
 # Create master branch for libfoo.git, adding style.git and libbar.git as
 # submodules.
diff --git a/tests/common/git/state0/libbar.tar b/tests/common/git/state0/libbar.tar
index b63a7b5..cf00747 100644
--- a/tests/common/git/state0/libbar.tar
+++ b/tests/common/git/state0/libbar.tar
diff --git a/tests/common/git/state0/libfoo.tar b/tests/common/git/state0/libfoo.tar
index 200ebf5..05281ee 100644
--- a/tests/common/git/state0/libfoo.tar
+++ b/tests/common/git/state0/libfoo.tar
diff --git a/tests/common/git/state0/libfox.tar b/tests/common/git/state0/libfox.tar
index df77539..b024532 100644
--- a/tests/common/git/state0/libfox.tar
+++ b/tests/common/git/state0/libfox.tar
diff --git a/tests/common/git/state0/links.tar b/tests/common/git/state0/links.tar
index 25c4089..3769fd1 100644
--- a/tests/common/git/state0/links.tar
+++ b/tests/common/git/state0/links.tar
diff --git a/tests/common/git/state0/style-basic.tar b/tests/common/git/state0/style-basic.tar
index 7223c52..01eadbc 100644
--- a/tests/common/git/state0/style-basic.tar
+++ b/tests/common/git/state0/style-basic.tar
diff --git a/tests/common/git/state0/style.tar b/tests/common/git/state0/style.tar
index 93007d5..042684e 100644
--- a/tests/common/git/state0/style.tar
+++ b/tests/common/git/state0/style.tar
diff --git a/tests/common/git/state1/libbaz.tar b/tests/common/git/state1/libbaz.tar
index d46b25c..5238b4a 100644
--- a/tests/common/git/state1/libbaz.tar
+++ b/tests/common/git/state1/libbaz.tar
diff --git a/tests/common/git/state1/libfoo.tar b/tests/common/git/state1/libfoo.tar
index 683f5ab..c0e7523 100644
--- a/tests/common/git/state1/libfoo.tar
+++ b/tests/common/git/state1/libfoo.tar
diff --git a/tests/common/git/state1/libfox.tar b/tests/common/git/state1/libfox.tar
index 49c4752..e502974 100644
--- a/tests/common/git/state1/libfox.tar
+++ b/tests/common/git/state1/libfox.tar
diff --git a/tests/common/git/state1/style-basic.tar b/tests/common/git/state1/style-basic.tar
index 5ab346d..1ef7399 100644
--- a/tests/common/git/state1/style-basic.tar
+++ b/tests/common/git/state1/style-basic.tar
diff --git a/tests/common/git/state1/style.tar b/tests/common/git/state1/style.tar
index 71be292..2a0120b 100644
--- a/tests/common/git/state1/style.tar
+++ b/tests/common/git/state1/style.tar
diff --git a/tests/publish b/tests/publish
index df355eb..bd1f327 100755
--- a/tests/publish
+++ b/tests/publish
@@ -60,7 +60,7 @@ for r in $(find test -type d -regex '.*/git/.*/[^/]+\.git'); do
 
     # Push local branches and tags to the remote repository, if it exists.
     #
-    if git ls-remote "$url" 2>/dev/null >&2; then
+    if git ls-remote --refs "$url" 2>/dev/null >&2; then
       # Point the bare repository origin to the remote repository.
       #
       git -C $br config remote.origin.url "$url"
diff --git a/tests/remote-git.testscript b/tests/remote-git.testscript
index 40f7ae5..56c33b7 100644
--- a/tests/remote-git.testscript
+++ b/tests/remote-git.testscript
@@ -63,8 +63,8 @@ else
   rep_git_https_dumb        = "https://build2.org/bpkg/git/$cmd"
   rep_git_https_smart       = "https://git.build2.org/testing/bpkg/advonly/$cmd"
   rep_git_https_smart_unadv = "https://git.build2.org/testing/bpkg/unadv/$cmd"
-  rep_git_git               = "git://git.build2.org/testing/bpkg/unadv/$cmd"
-  rep_git_ssh               = "ssh://git.build2.org/var/scm/testing/bpkg/unadv/$cmd"
+  rep_git_git               = "git://git.build2.org/testing/bpkg/advonly/$cmd"
+  rep_git_ssh               = "ssh://git.build2.org/var/scm/testing/bpkg/advonly/$cmd"
 
   rep_git = $rep_git_https_dumb # Default remote repository URL.
 end
diff --git a/tests/rep-fetch-git-commit.testscript b/tests/rep-fetch-git-commit.testscript
index 3f0fbe7..8af9ea1 100644
--- a/tests/rep-fetch-git-commit.testscript
+++ b/tests/rep-fetch-git-commit.testscript
@@ -3,6 +3,7 @@
 # license   : MIT; see accompanying LICENSE file
 
 +git clone "$rep_git/state0/style-basic.git" 2>! &style-basic/***
++git clone "$rep_git/state0/libbar.git"      2>! &libbar/***
 
 : unadvertised
 :
@@ -94,3 +95,37 @@ if ($git_fully_supported || $git_protocol != 'https-smart-unadv')
       EOE
   }
 }
+
+: peeled
+:
+{
+  +git -C ../libbar log '--pretty=format:%H' --all --grep='Create' | \
+   set commit
+
+  : remap
+  :
+  : Test that the commit id is properly remapped back to the advertised tag
+  : reference.
+  :
+  {
+    $clone_root_cfg && $rep_add "$rep/state0/libbar.git#@$commit";
+
+    $* 2>>~"%EOE%"
+      %fetching git:.+libbar#@$commit%
+      %.+
+      EOE
+  }
+
+  : peel
+  :
+  : Test that the tag reference is properly peeled into the commit id.
+  :
+  {
+    $clone_root_cfg && $rep_add "$rep/state0/libbar.git#v1.0.0,-$commit";
+
+    $* 2>>~%EOE%
+      %.+
+      0 package(s) in 1 repository(s)
+      EOE
+  }
+}
diff --git a/tests/rep-fetch.testscript b/tests/rep-fetch.testscript
index 8e7011a..c2fd4cc 100644
--- a/tests/rep-fetch.testscript
+++ b/tests/rep-fetch.testscript
@@ -809,7 +809,7 @@ else
   {
     $clone_root_cfg && $rep_add "$rep_git_git/state0/libfoo.git#master";
     $* 2>! &cfg/.bpkg/repos/*/***;
-    $rep_add "$rep_git_https_smart_unadv/state0/libfoo.git#master";
+    $rep_add "$rep_git_https_smart/state0/libfoo.git#master";
 
     $* 2>>~%EOE%
       %fetching git:.+libfoo#master%