Fix previous OTP-23 compat patches

- Normalize path behaviour to always be "/" across versions
- Normalize port visibility behaviour to match input string when
  appending
- restore rebar_utils function to avoid breaking random plugins that
  peek into our libs for their stuff
- Support URIOpts equivalent and protocol-based overrides; the behaviour
  there is not necessarily consistent across versions in terms of what
  ports are returned (http_uri always returned a port but uri_string
  only returns it if explicitly specified -- we choose the latter
  behaviour on newer releases), but the calls work to ensure URI parsing
  consistently works across versions

The latter judgement call is a bit of an odd one; for consistency we
could always mandate ports, but this would come at a performance penalty
when appending URL paths (i.e. when fetching packages).

The reason for this is that we standardize on the new uri_string
behaviour for path appending (if the port wasn't specified, we don't add
it), and with the http_uri rules, we need to do a kind of parsing round
that checks if the port was included or not to make it equivalent. This
is costly, and _not_ returning the port when it isn't specified lets us
do this transparently.

This allows to maintain backwards compat in older append functions, and
nobody aside from us currently uses the new rebar_uri module so we can
decide to introduce this potential inconsistency if we wish to.

src/rebar_git_resource.erl

@@ -100,13 +100,6 @@ compare_url(Dir, Url) ->
     ?DEBUG("Comparing git url ~p with ~p", [ParsedUrl, ParsedCurrentUrl]),
     ParsedCurrentUrl =:= ParsedUrl.
 
--ifdef (OTP_RELEASE).
-  -if(?OTP_RELEASE >= 23).
-    -compile({nowarn_deprecated_function, [{http_uri, parse, 2},
-                                           {http_uri, scheme_defaults, 0}]}).
-  -endif.
--endif.
-
 parse_git_url(Url) ->
     %% Checks for standard scp style git remote
     case re:run(Url, ?SCP_PATTERN, [{capture, [host, path], list}, unicode]) of
@@ -116,7 +109,7 @@ parse_git_url(Url) ->
             parse_git_url(not_scp, Url)
     end.
 parse_git_url(not_scp, Url) ->
-    UriOpts = [{scheme_defaults, [{git, 9418} | http_uri:scheme_defaults()]}],
+    UriOpts = [{scheme_defaults, [{git, 9418} | rebar_uri:scheme_defaults()]}],
     case rebar_uri:parse(Url, UriOpts) of
         #{path := Path, host := Host} ->
             {ok, {Host, filename:rootname(Path, ".git")}};

src/rebar_uri.erl

@@ -2,26 +2,33 @@
 -module(rebar_uri).
 
 -export([
-         parse/1,
+         parse/1, parse/2, scheme_defaults/0,
          append_path/2
 ]).
 
--import(rebar_utils, [to_list/1]).
-
--ifdef (OTP_RELEASE).
+-ifdef(OTP_RELEASE).
 -spec parse(URIString) -> URIMap when
     URIString :: uri_string:uri_string(),
     URIMap :: uri_string:uri_map() | uri_string:error().
 
 parse(URIString) ->
-    uri_string:parse(URIString).
+    parse(URIString, []).
+
+parse(URIString, URIOpts) ->
+    case uri_string:parse(URIString) of
+        #{path := ""} = Map -> apply_opts(Map#{path => "/"}, URIOpts);
+        Map -> apply_opts(Map, URIOpts)
+    end.
 -else.
 -spec parse(URIString) -> URIMap when
     URIString :: iodata(),
     URIMap :: map() | {error, atom(), term()}.
 
 parse(URIString) ->
-    case http_uri:parse(URIString) of
+    parse(URIString, []).
+
+parse(URIString, URIOpts) ->
+    case http_uri:parse(URIString, URIOpts) of
         {error, Reason} ->
             %% no additional parser/term info available to us,
             %% e.g. see what uri_string returns in
@@ -46,11 +53,11 @@ parse(URIString) ->
 -endif.
 
 %% OTP 21+
--ifdef (OTP_RELEASE).
+-ifdef(OTP_RELEASE).
 append_path(Url, ExtraPath) ->
      case parse(Url) of
          #{path := Path} = Map ->
-             FullPath = filename:join(Path, ExtraPath),
+             FullPath = join(Path, ExtraPath),
              {ok, uri_string:recompose(maps:update(path, FullPath, Map))};
          _ ->
              error
@@ -58,14 +65,66 @@ append_path(Url, ExtraPath) ->
 -else.
 append_path(Url, ExtraPath) ->
      case parse(Url) of
-         #{scheme := Scheme, userinfo := UserInfo, host := Host, port := Port, path := Path, query := Query} ->
+         #{scheme := Scheme, userinfo := UserInfo, host := Host,
+           port := Port, path := Path, query := Query} ->
+             ListScheme = rebar_utils:to_list(Scheme),
              PrefixedQuery = case Query of
                                []    -> [];
                                Other -> lists:append(["?", Other])
                              end,
-             {ok, lists:append([to_list(Scheme), "://", UserInfo, Host, ":", to_list(Port),
-                                filename:join(Path, ExtraPath), PrefixedQuery])};
+             NormPath = case Path of
+                            "" -> "/";
+                            _ -> Path
+                        end,
+             {ok, maybe_port(
+                Url, lists:append([ListScheme, "://", UserInfo, Host]),
+                [$: | rebar_utils:to_list(Port)],
+                lists:append([join(NormPath, ExtraPath), PrefixedQuery])
+             )};
          _ ->
              error
      end.
 -endif.
+
+%% OTP 21+
+-ifdef(OTP_RELEASE).
+scheme_defaults() ->
+    %% no scheme defaults here; just custom ones
+    [].
+-else.
+scheme_defaults() ->
+    http_uri:scheme_defaults().
+-endif.
+
+join(URI, "") -> URI;
+join(URI, "/") -> URI;
+join("/", [$/|_] = Path) -> Path;
+join("/", Path) -> [$/ | Path];
+join("", [$/|_] = Path) -> Path;
+join("", Path) -> [$/ | Path];
+join([H|T], Path) -> [H | join(T, Path)].
+
+
+-ifdef(OTP_RELEASE).
+apply_opts(Map = #{port := _}, _) ->
+    Map;
+apply_opts(Map = #{scheme := Scheme}, URIOpts) ->
+    SchemeDefaults = proplists:get_value(scheme_defaults, URIOpts, []),
+    %% Here is the funky bit: don't add the port number if it's in a default
+    %% to maintain proper default behaviour.
+    try lists:keyfind(list_to_existing_atom(Scheme), 1, SchemeDefaults) of
+        {_, Port} ->
+            Map#{port => Port};
+        false ->
+            Map
+    catch
+        error:badarg -> % not an existing atom, not in the list
+            Map
+    end.
+-else.
+maybe_port(Url, Host, Port, PathQ) ->
+    case lists:prefix(Host ++ Port, Url) of
+        true -> Host ++ Port ++ PathQ; % port was explicit
+        false -> Host ++ PathQ % port was implicit
+    end.
+-endif.

src/rebar_utils.erl

@@ -64,6 +64,7 @@
          tup_find/2,
          line_count/1,
          set_httpc_options/0,
+         url_append_path/2,
          escape_chars/1,
          escape_double_quotes/1,
          escape_double_quotes_weak/1,
@@ -938,6 +939,9 @@ normalise_proxy(Scheme, URI) ->
         _ -> URI
     end.
 
+url_append_path(Url, ExtraPath) ->
+    rebar_uri:append_path(Url, ExtraPath).
+
 %% escape\ as\ a\ shell\?
 escape_chars(Str) when is_atom(Str) ->
     escape_chars(atom_to_list(Str));

test/rebar_uri_SUITE.erl

@@ -9,20 +9,20 @@
 -include_lib("kernel/include/file.hrl").
 
 all() ->
-    [parse].
+    [parse, append_path].
 
 parse(_Config) ->
     #{scheme := Scheme, host := Host, path := Path} = rebar_uri:parse("https://repo.hex.pm"),
     ?assertEqual("https", Scheme),
     ?assertEqual("repo.hex.pm", Host),
-    ?assert(lists:member(Path, ["", "/"])),
+    ?assertEqual(Path, "/"), % Normalize on OTP-23 behaviour.
 
     #{scheme := Scheme2, host := Host2, port := Port2, path := Path2, query := Query2} =
         rebar_uri:parse("https://repo.hex.pm:443?foo=bar"),
     ?assertEqual("https", Scheme2),
     ?assertEqual("repo.hex.pm", Host2),
     ?assertEqual(443, Port2),
-    ?assert(lists:member(Path2, ["", "/"])),
+    ?assertEqual(Path2, "/"), % Normalize on old http_uri behaviour
     ?assertEqual("foo=bar", Query2),
 
     #{scheme := Scheme3, host := Host3, path := Path3, query := Query3} =
@@ -30,19 +30,21 @@ parse(_Config) ->
     ?assertEqual("https", Scheme3),
     ?assertEqual("repo.hex.pm", Host3),
     ?assertEqual("/over/here", Path3),
-    ?assertEqual("foo=bar", Query3).
+    ?assertEqual("foo=bar", Query3),
+
+    %% override default port and get it parsed as such
+    ?assertMatch(#{port := 1337},
+                 rebar_uri:parse("https://repo.hex.pm/",
+                                 [{scheme_defaults, [{https,1337}]}])),
+    ok.
 
 append_path(_Config) ->
-    %% OTP version differences
-    {ok, Val1} = rebar_utils:append_path("https://repo.hex.pm", "/repos/org"),
-    ?assert(lists:member(Val1, [
-        "https://repo.hex.pm/repos/org",
-        "https://repo.hex.pm:443/repos/org"
-    ])),
-    {ok, Val2} = rebar_utils:append_path("https://repo.hex.pm?foo=bar", "/repos/org"),
-    ?assert(lists:member(Val2, [
-        "https://repo.hex.pm/repos/org?foo=bar",
-        "https://repo.hex.pm:443/repos/org?foo=bar"
-    ])),
+    %% Default port for the proto is omitted if not mentioned originally
+    {ok, Val1} = rebar_uri:append_path("https://repo.hex.pm/", "/repos/org"),
+    ?assertEqual("https://repo.hex.pm/repos/org", Val1),
+    %% QS elements come after the path
+    {ok, Val2} = rebar_uri:append_path("https://repo.hex.pm?foo=bar", "/repos/org"),
+    ?assertEqual("https://repo.hex.pm/repos/org?foo=bar", Val2),
+    %% If the port is explicitly mentioned, keep it.
     ?assertEqual({ok, "https://repo.hex.pm:443/repos/org?foo=bar"},
-                 rebar_utils:append_path("https://repo.hex.pm:443?foo=bar", "/repos/org")).
+                 rebar_uri:append_path("https://repo.hex.pm:443?foo=bar", "/repos/org")).