Fix unexpected 'unable to read HTTP response status line' fetch error

3 files changed, 43 insertions, 8 deletions

diff --git a/bpkg/fetch-git.cxx b/bpkg/fetch-git.cxx
index 4b1a242..f3c6355 100644
--- a/bpkg/fetch-git.cxx
+++ b/bpkg/fetch-git.cxx
@@ -654,6 +654,31 @@ namespace bpkg
         return capabilities::smart;
       }
 
+      // Fail on any other HTTP error (e.g., 404). In the case of a success
+      // code other than 200 (e.g. 204 (No Content)) just let the capabilities
+      // detection to take its course.
+      //
+      if (ps.second != 0 && (ps.second < 200 || ps.second >= 300))
+      {
+        // Note that we don't care about the process exit code here (see above
+        // for the reasoning).
+        //
+        is.close ();
+
+        // Dump the potentially redirected process stderr stream content since
+        // it may be helpful to the user.
+        //
+        // Note, however, that we don't know if it really contains the error
+        // description since the fetch program may even exit successfully (see
+        // start_fetch_http() for details). Thus, we additionally print the
+        // HTTP status code in the diagnostics.
+        //
+        dump_stderr ();
+
+        fail << "unable to fetch " << url <<
+          info << "HTTP status code " << ps.second << endg;
+      }
+
       string l;
       getline (is, l); // Is empty if no refs returned by the dumb server.
 
diff --git a/bpkg/fetch.cxx b/bpkg/fetch.cxx
index 573dfde..a9e4df2 100644
--- a/bpkg/fetch.cxx
+++ b/bpkg/fetch.cxx
@@ -317,7 +317,6 @@ namespace bpkg
 
     cstrings args {
       prog.string ().c_str (),
-      "-f", // Fail on HTTP errors (e.g., 404).
       "-L", // Follow redirects.
       "-A", ua.c_str ()
     };
@@ -397,12 +396,24 @@ namespace bpkg
 
     // Status code.
     //
+    // Add the --include|-i option if HTTP status code needs to be retrieved
+    // in order to include the HTTP response headers to the output. Otherwise,
+    // add the --fail|-f option not to print the response body and exit with
+    // non-zero status code on HTTP error (e.g., 404), so that the caller can
+    // recognize the request failure.
+    //
+    // Note that older versions of curl (e.g., 7.55.1) ignore the --include|-i
+    // option in the presence of the --fail|-f option on HTTP errors and don't
+    // print the response status line and headers.
+    //
     if (out_is != nullptr)
     {
       assert (!fo); // Currently unsupported (see start_fetch() for details).
 
       args.push_back ("-i");
     }
+    else
+      args.push_back ("-f");
 
     args.push_back (url.c_str ());
     args.push_back (nullptr);
@@ -482,8 +493,8 @@ namespace bpkg
         assert (e != nullptr);
 
         return *e == '\0' && c >= 100 && c < 600
-        ? static_cast<uint16_t> (c)
-        : 0;
+               ? static_cast<uint16_t> (c)
+               : 0;
       };
 
       // Read the CRLF-terminated line from the stream stripping the trailing
@@ -535,10 +546,7 @@ namespace bpkg
         close_streams ();
 
         fail << "invalid HTTP response status line '" << l
-             << "' while fetching " << url;
-
-        assert (false); // Can't be here.
-        return 0;
+             << "' while fetching " << url << endf;
       };
 
       sc = read_status ();
diff --git a/bpkg/fetch.hxx b/bpkg/fetch.hxx
index 78e77a7..6578f0d 100644
--- a/bpkg/fetch.hxx
+++ b/bpkg/fetch.hxx
@@ -156,7 +156,9 @@ namespace bpkg
   // whether stderr is redirected by checking process::in_efd.
   //
   // In contrast to start_fetch() always fetch to stdout, which can be read by
-  // the caller from the specified stream.
+  // the caller from the specified stream. On HTTP errors (e.g., 404) this
+  // stream may contain the error description returned by the server and the
+  // process may exit with 0 code.
   //
   // If the quiet argument is true, then, if stderr is redirected, minimize
   // the amount of diagnostics printed by the fetch program by only printing