From 86fb898df44b62d4ca7c34878b893bcadec6875b Mon Sep 17 00:00:00 2001 From: Michael Wesemann Date: Sun, 26 Jul 2026 17:35:50 +0200 Subject: [PATCH] Fix `list` breaking when the server's du fails Two mistakes in the size support, reported from a real server. The remote command used "2>/dev/null" to silence du. That is sh syntax, and the git user's login shell need not be sh: in csh it parses as an argument "2" followed by a redirection of stdout, so du was handed a file named "2", complained, and exited non-zero. The redirection is gone -- without it there is no bogus argument to trip over, and the command now uses nothing that differs between sh and csh. Worse, the exit status of the chain is the *last* command's, so that failing du made sshOut return an error and `list` threw away a listing that had arrived perfectly intact. It now reports a failure only when nothing usable came back at all; a listing that parsed is shown whatever the exit status, simply without the size column. Co-Authored-By: Claude Opus 5 --- commands.go | 17 ++++++++----- remotecmd_test.go | 62 +++++++++++++++++++++++++++++++++++++++++++++++ version.txt | 2 +- 3 files changed, 74 insertions(+), 7 deletions(-) diff --git a/commands.go b/commands.go index 29493bb..5a39c38 100644 --- a/commands.go +++ b/commands.go @@ -246,14 +246,12 @@ func runCommandDepth(line string, depth int) bool { if !opt["a"] { // archives are files and carry a real size; repositories are // directories, whose listed size is the inode's, so ask du in the - // same round trip - remote += "; echo " + shq(listMarker) + "; du -sk *.git 2>/dev/null" + // same round trip. Nothing shell-specific here on purpose: the + // login shell may be csh, where "2>/dev/null" is not a redirection + // but an argument followed by one. + remote += "; echo " + shq(listMarker) + "; du -sk *.git" } lines, err := sshOut(remote) - if err != nil { - errorln("could not list " + many + " on the git server") - break - } lsLines, duLines := splitAtMarker(lines, listMarker) sizes := parseDuSizes(duLines) @@ -272,7 +270,14 @@ func runCommandDepth(line string, depth int) bool { total += e.size entries = append(entries, e) } + // The exit status belongs to the last command in the chain, so a `du` + // that fails must not discard a listing that arrived intact. Only + // complain when nothing usable came back at all. if len(entries) == 0 { + if err != nil { + errorln("could not list " + many + " on the git server") + break + } what := "no " + many + " on the git server" if pat != "" { what = "no " + many + " matching '" + word(words, 1) + "'" diff --git a/remotecmd_test.go b/remotecmd_test.go index 64ec74c..e829b2c 100644 --- a/remotecmd_test.go +++ b/remotecmd_test.go @@ -329,3 +329,65 @@ func mustGit(t *testing.T, dir string, args ...string) { t.Fatalf("git %v: %v\n%s", args, err, out) } } + +// TestListSurvivesFailingDu: `list` chains the listing and `du` into one remote +// command, and the exit status is the *last* command's. A server whose du fails +// — a shell that mis-parses the arguments, a du that is not there, a permission +// problem — must still get its repositories listed. +func TestListSurvivesFailingDu(t *testing.T) { + useProject(t, "x") + fakeServer(t, func(cmd string) (string, error) { + return "total 4\n" + + "drwxr-xr-x 7 git git 4096 Jan 3 14:32 notes.git\n" + + "drwxr-xr-x 7 git git 4096 Sep 28 2016 website.git\n" + + listMarker + "\n", + errors.New("exit status 1") // du blew up, ls did not + }) + + out := captureStdout(t, func() { runCommand("list") }) + + if strings.Contains(out, "could not list") { + t.Errorf("a failing du discarded a good listing:\n%s", out) + } + for _, want := range []string{"notes", "website", "2 repositories"} { + if !strings.Contains(out, want) { + t.Errorf("listing missing %q:\n%s", want, out) + } + } + // without sizes there must be no size column, not a column of zeroes + if strings.Contains(out, "0B") { + t.Errorf("zero sizes shown when du produced none:\n%s", out) + } +} + +// TestListReportsATrulyFailedListing: when nothing usable came back, the error +// still has to surface. +func TestListReportsATrulyFailedListing(t *testing.T) { + useProject(t, "x") + fakeServer(t, func(cmd string) (string, error) { + return "", errors.New("ssh: connect failed") + }) + out := captureStdout(t, func() { runCommand("list") }) + if !strings.Contains(out, "could not list") { + t.Errorf("a failed listing was not reported:\n%s", out) + } +} + +// TestListSendsNoShellSpecificSyntax guards the bug this replaced: the remote +// command is run by the git user's login shell, which may be csh, where +// "2>/dev/null" is an argument followed by a redirection rather than a +// redirection of stderr. +func TestListSendsNoShellSpecificSyntax(t *testing.T) { + useProject(t, "x") + sent := fakeServer(t, func(cmd string) (string, error) { return "", nil }) + captureStdout(t, func() { runCommand("list") }) + + if len(*sent) == 0 { + t.Fatal("list sent nothing") + } + for _, c := range *sent { + if strings.Contains(c, "2>") || strings.Contains(c, "&>") { + t.Errorf("remote command uses sh-only redirection: %q", c) + } + } +} diff --git a/version.txt b/version.txt index 6634c5d..a6e8576 100644 --- a/version.txt +++ b/version.txt @@ -1 +1 @@ -4.0.31 +4.0.33