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