fix apt repo urls and priority parsing

bash/patchman-client

@@ -591,6 +591,7 @@ get_repos() {
             mirror_file=$(echo "${mirror_repo}" | sed -e "s/.* mirror+file://g" | cut -d " " -f 1)
             if [ -f "${mirror_file}" ] ; then
                 for url in $(cat ${mirror_file}) ; do
+                    url=${url%/}
                     dist_repo=$(echo "${mirror_repo}" | sed -e "s#mirror+file:${mirror_file}#${url}#g")
                     dist_repos="${dist_repos}"$'\n'"${dist_repo}"
                 done

go/lib/repos/apt.go

@@ -37,16 +37,19 @@ func PrintAptReposTable(repos []AptRepo) {
 }
 
 func ConstructAptRepoURL(url string, components []string) string {
-	repo_url := url
-	if components[0] != "" {
-		repo_url += "/dists/" + components[0]
-	}
-	if components[1] != "Packages" {
-		repo_url += "/binary-" + components[1]
+	repo_url := strings.TrimRight(url, "/")
+	if len(components) < 2 || components[1] == "Packages" {
+		// flat repo, e.g. "./ Packages"
+		if len(components) > 0 {
+			repo_url += "/" + components[0]
+		}
+		return repo_url
 	}
-	return repo_url
+	return repo_url + "/dists/" + components[0] + "/binary-" + components[1]
 }
 
+var aptRepoLineRe = regexp.MustCompile(`^\s*-?[0-9]+\s+(mirror\+file:|https?:)`)
+
 func GetAptRepos(os_info si.OSInfo) ([]AptRepo, error) {
 	cmd, err := util.RunCommand("apt-cache", "policy")
 	if err != nil {
@@ -55,34 +58,43 @@ func GetAptRepos(os_info si.OSInfo) ([]AptRepo, error) {
 	if cmd == nil {
 		return nil, nil // apt-cache not available
 	}
+	return parseAptPolicy(string(cmd), os_info), nil
+}
 
+func parseAptPolicy(output string, os_info si.OSInfo) []AptRepo {
 	apt_repos := []AptRepo{}
-	repoLineRe := regexp.MustCompile(`^ *[0-9]{1,5} mirror\+file|http(s)?:`)
-	for _, line := range strings.Split(string(cmd), "\n") {
+	for _, line := range strings.Split(output, "\n") {
 		if strings.Contains(line, "Translation") {
 			continue
 		}
-		if !repoLineRe.MatchString(line) {
+		if !aptRepoLineRe.MatchString(line) {
 			continue
 		}
-		v := strings.Split(line, " ")
-		priority := v[1]
+		// priority is right-aligned, so the number of leading spaces varies
+		v := strings.Fields(line)
+		if len(v) < 4 {
+			continue
+		}
+		priority := v[0]
 		repo_urls := []string{}
-		if strings.HasPrefix(v[2], "mirror+file:/") {
-			mirror_file := strings.Replace(v[2], "mirror+file:", "", 1)
+		if strings.HasPrefix(v[1], "mirror+file:/") {
+			mirror_file := strings.Replace(v[1], "mirror+file:", "", 1)
 			content, err := os.ReadFile(mirror_file)
 			if err != nil {
 				slog.Warn("failed to read mirror file", "file", mirror_file, "error", err)
 				continue
 			}
 			for _, mirror := range strings.Split(string(content), "\n") {
 				if mirror != "" {
-					repo_url := ConstructAptRepoURL(mirror, v[3:])
+					repo_url := ConstructAptRepoURL(mirror, v[2:])
 					repo_urls = append(repo_urls, repo_url)
 				}
 			}
+			if len(repo_urls) == 0 {
+				continue
+			}
 		} else {
-			repo_url := ConstructAptRepoURL(v[2], v[3:])
+			repo_url := ConstructAptRepoURL(v[1], v[2:])
 			repo_urls = append(repo_urls, repo_url)
 		}
 		name := fmt.Sprintf("%s %s %s repo at %s", os_info.OSVendor, os_info.OSRelease, os_info.OSArch, repo_urls[0])
@@ -92,8 +104,7 @@ func GetAptRepos(os_info si.OSInfo) ([]AptRepo, error) {
 			Urls:     repo_urls,
 		})
 	}
-
-	return apt_repos, nil
+	return apt_repos
 }
 
 func PrintAptRepos(repos []AptRepo) {

go/lib/repos/repos_test.go

@@ -2,8 +2,11 @@ package repos
 
 import (
 	"os"
+	"reflect"
 	"strings"
 	"testing"
+
+	"github.com/furlongm/patchman-client/go/lib/si"
 )
 
 func TestGetYumRepos(t *testing.T) {
@@ -222,6 +225,80 @@ enabled=1
 	}
 }
 
+func TestConstructAptRepoURL(t *testing.T) {
+	tests := []struct {
+		url        string
+		components []string
+		want       string
+	}{
+		{"http://deb.debian.org/debian", []string{"bookworm/main", "amd64", "Packages"},
+			"http://deb.debian.org/debian/dists/bookworm/main/binary-amd64"},
+		{"http://deb.debian.org/debian/", []string{"bookworm/main", "amd64", "Packages"},
+			"http://deb.debian.org/debian/dists/bookworm/main/binary-amd64"},
+		{"https://repo.example.com/flat", []string{"./", "Packages"},
+			"https://repo.example.com/flat/./"},
+	}
+	for _, tt := range tests {
+		if got := ConstructAptRepoURL(tt.url, tt.components); got != tt.want {
+			t.Errorf("ConstructAptRepoURL(%q, %q) = %q, want %q", tt.url, tt.components, got, tt.want)
+		}
+	}
+}
+
+func TestParseAptPolicy(t *testing.T) {
+	output := `Package files:
+ 100 /var/lib/dpkg/status
+     release a=now
+ 500 http://deb.debian.org/debian bookworm/main amd64 Packages
+     release v=12,o=Debian,a=stable,n=bookworm,l=Debian,c=main,b=amd64
+     origin deb.debian.org
+ 500 http://deb.debian.org/debian bookworm/main Translation-en
+   1 http://deb.debian.org/debian experimental/main amd64 Packages
+1001 http://apt.example.com/pinned stable/main amd64 Packages
+ -10 http://apt.example.com/never stable/main amd64 Packages
+ 500 https://repo.example.com/flat ./ Packages
+Pinned packages:
+`
+	want := []AptRepo{
+		{Priority: "500", Urls: []string{"http://deb.debian.org/debian/dists/bookworm/main/binary-amd64"}},
+		{Priority: "1", Urls: []string{"http://deb.debian.org/debian/dists/experimental/main/binary-amd64"}},
+		{Priority: "1001", Urls: []string{"http://apt.example.com/pinned/dists/stable/main/binary-amd64"}},
+		{Priority: "-10", Urls: []string{"http://apt.example.com/never/dists/stable/main/binary-amd64"}},
+		{Priority: "500", Urls: []string{"https://repo.example.com/flat/./"}},
+	}
+	got := parseAptPolicy(output, si.OSInfo{})
+	if len(got) != len(want) {
+		t.Fatalf("got %d repos, want %d: %+v", len(got), len(want), got)
+	}
+	for i := range want {
+		if got[i].Priority != want[i].Priority || !reflect.DeepEqual(got[i].Urls, want[i].Urls) {
+			t.Errorf("repo %d: got %s %q, want %s %q", i, got[i].Priority, got[i].Urls, want[i].Priority, want[i].Urls)
+		}
+	}
+}
+
+func TestParseAptPolicyMirrorFile(t *testing.T) {
+	dir := t.TempDir()
+	mirrors := dir + "/mirrors.txt"
+	if err := os.WriteFile(mirrors, []byte("http://a.example.com/debian/\nhttps://b.example.com/debian\n"), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	empty := dir + "/empty.txt"
+	if err := os.WriteFile(empty, []byte("\n"), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	output := " 500 mirror+file:" + mirrors + " bookworm/main amd64 Packages\n" +
+		" 500 mirror+file:" + empty + " bookworm/main amd64 Packages\n"
+	got := parseAptPolicy(output, si.OSInfo{})
+	want := []string{
+		"http://a.example.com/debian/dists/bookworm/main/binary-amd64",
+		"https://b.example.com/debian/dists/bookworm/main/binary-amd64",
+	}
+	if len(got) != 1 || !reflect.DeepEqual(got[0].Urls, want) {
+		t.Errorf("got %+v, want one repo with %q", got, want)
+	}
+}
+
 func TestGetPacmanRepos(t *testing.T) {
 	// Skip if not on Arch system
 	if _, err := os.Stat("/etc/pacman.conf"); os.IsNotExist(err) {

python/patchman_client/client.py

@@ -669,11 +669,12 @@ def _collect_apt_repos(self) -> None:
                     pass
 
         # Build repo entries from resolved URLs
-        for priority, base_url, dist, component in http_entries:
-            if component.endswith('Packages'):
+        for priority, base_url, dist, apt_arch in http_entries:
+            base_url = base_url.rstrip('/')
+            if apt_arch.endswith('Packages'):
                 url = f'{base_url}/{dist}'
             else:
-                url = f'{base_url}/dists/{dist}/binary-{self.arch}'
+                url = f'{base_url}/dists/{dist}/binary-{apt_arch}'
 
             repo_name = f'{os_name} {version_id} {self.arch} repo at {url}'
 

tests/test_client_output.py

@@ -1376,6 +1376,73 @@ def test_mirror_file_resolution(self, tmp_path=None):
             os.unlink(mirror_path)
 
 
+class AptMirrorFileTrailingSlashTests(unittest.TestCase):
+    """Mirror file URLs with a trailing slash must not produce '//' in repo URLs,
+    and binary-<arch> must use the apt arch, not the machine arch."""
+
+    def setUp(self):
+        import tempfile
+        self.tmpdir = tempfile.mkdtemp()
+        self.addCleanup(__import__('shutil').rmtree, self.tmpdir)
+        mirror_file = os.path.join(self.tmpdir, 'mirrors.txt')
+        with open(mirror_file, 'w') as f:
+            f.write('http://deb.debian.org/debian/\n')
+        self.policy = (
+            'Package files:\n'
+            f' 500 mirror+file:{mirror_file} bookworm/main amd64 Packages\n'
+            '     release v=12,o=Debian,a=stable,n=bookworm,c=main,b=amd64\n'
+        )
+        self.expected = 'http://deb.debian.org/debian/dists/bookworm/main/binary-amd64'
+
+    def test_python_client(self):
+        import sys
+        sys.path.insert(0, os.path.join(
+            os.path.dirname(os.path.dirname(os.path.abspath(__file__))),
+            'python',
+        ))
+        from patchman_client.client import PatchmanClient
+
+        client = PatchmanClient.__new__(PatchmanClient)
+        client.repos = []
+        client.arch = 'x86_64'
+        client.os_string = 'Debian 12'
+        client.log = lambda *a, **kw: None
+        client._command_exists = lambda cmd: True
+        client._run_command = lambda cmd: (0, self.policy, '')
+        client._collect_apt_repos()
+        self.assertEqual(client.repos[0]['urls'], [self.expected])
+
+    def test_bash_client(self):
+        import subprocess
+        bindir = os.path.join(self.tmpdir, 'bin')
+        os.mkdir(bindir)
+        apt_cache = os.path.join(bindir, 'apt-cache')
+        with open(apt_cache, 'w') as f:
+            f.write(f"#!/bin/sh\ncat <<'EOF'\n{self.policy}EOF\n")
+        os.chmod(apt_cache, 0o755)
+        conf = os.path.join(self.tmpdir, 'patchman-client.conf')
+        with open(conf, 'w') as f:
+            f.write('server=https://pm.test\napi_key=test\n')
+        src = os.path.join(
+            os.path.dirname(os.path.dirname(os.path.abspath(__file__))),
+            'bash', 'patchman-client',
+        )
+        # /run/patchman is not writable for unprivileged test runners
+        client = os.path.join(self.tmpdir, 'patchman-client')
+        with open(src) as f:
+            script = f.read()
+        self.assertIn('lock_dir=/run/patchman', script)
+        with open(client, 'w') as f:
+            f.write(script.replace('lock_dir=/run/patchman', f'lock_dir={self.tmpdir}/lock'))
+        env = dict(os.environ, PATH=f'{bindir}:{os.environ["PATH"]}')
+        result = subprocess.run(
+            ['bash', client, '-y', '-d', '-c', conf],
+            capture_output=True, text=True, timeout=60, env=env,
+        )
+        self.assertIn(f"'500' '{self.expected}'", result.stdout)
+        self.assertNotIn('debian//dists', result.stdout)
+
+
 class BashDebFieldBranchingTests(unittest.TestCase):
     """Tests for deb vs rpm field format in bash build_repos_json."""