[mike@mwxm4]

commands.go

@@ -20,7 +20,10 @@ var (
 	// literally named "git".
 	lsEntryRe = regexp.MustCompile(`^\S+\s+\d+\s+\S+\s+\S+\s+(\d+)\s+(\S+)\s+(\S+)\s+(\S+)\s+(.*)$`)
 	gitDirRe  = regexp.MustCompile(`^(.*)\.git$`)
-	sanRe     = regexp.MustCompile(`[,;:\\/='"|?><-]+`)
+	// '!' is in here for the server's sake: csh expands history even inside
+	// single quotes, so a comment like "fix!now" would reach it as something
+	// else entirely (see remoteRejected).
+	sanRe = regexp.MustCompile(`[!,;:\\/='"|?><-]+`)
 	wsRe  = regexp.MustCompile(`\s+`)
 )
 
@@ -58,7 +61,13 @@ func parseDuSizes(lines []string) map[string]int64 {
 		if err != nil {
 			continue
 		}
-		out[strings.TrimPrefix(strings.TrimSpace(m[2]), "./")] = kb * 1024
+		// du echoes the path it was given, and find gives it an absolute one:
+		// key on the last element, which is what the listing calls the entry
+		name := strings.TrimSpace(m[2])
+		if i := strings.LastIndexByte(name, '/'); i >= 0 {
+			name = name[i+1:]
+		}
+		out[name] = kb * 1024
 	}
 	return out
 }
@@ -251,14 +260,22 @@ func runCommandDepth(line string, depth int) bool {
 			one, many = "archive", "archives"
 		}
 		pat := strings.ToLower(word(words, 1))
-		remote := "/bin/ls -ltr " + shq(path)
+		remote := "/bin/ls -ltr " + shq(serverPath(path))
 		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. 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"
+			//
+			// The pattern goes to find, quoted, and never to the shell. A glob
+			// that matches nothing is a fatal error in a non-interactive zsh —
+			// "no matches found: *.git" — which aborts the command and turned an
+			// empty server into "could not list". sh would have handed the
+			// literal "*.git" to du instead, which is not much better.
+			remote += "; echo " + shq(listMarker) +
+				"; find " + shq(serverPath(path)) + " -maxdepth 1 -name " +
+				shq("*"+suffix) + " -exec du -sk {} +"
 		}
 		lines, err := sshOut(remote)
 		lsLines, duLines := splitAtMarker(lines, listMarker)
@@ -284,7 +301,13 @@ func runCommandDepth(line string, depth int) bool {
 		// complain when nothing usable came back at all.
 		if len(entries) == 0 {
 			if err != nil {
-				errorln("could not list " + many + " on the git server")
+				what := "could not list " + many + " on the git server"
+				if opt["a"] {
+					// by far the likeliest reason, and one the wording used to
+					// hide behind something that sounded like a dead connection
+					what += " — is there an 'archive' directory in " + cfg.GitPath + "?"
+				}
+				errorln(what)
 				break
 			}
 			what := "no " + many + " on the git server"
@@ -325,8 +348,8 @@ func runCommandDepth(line string, depth int) bool {
 			errorln("repository not found")
 			break
 		}
-		logLines, _ := sshOut("cd " + shq(cfg.GitPath+"/"+prj+".git") +
+		logLines, _ := sshOut("git --git-dir=" + shq(serverPath(prj+".git")) +
-			" && git log --reverse --format='%h  %ct  %s'")
+			" log --reverse --format='%h  %ct  %s'")
 		repolog(logLines)
 
 	case "log":
@@ -392,7 +415,7 @@ func runCommandDepth(line string, depth int) bool {
 		if !gitOK(DIR, "push") {
 			break
 		}
-		sshOK("touch " + shq(cfg.GitPath+"/"+PRJ+".git"))
+		sshOK("touch " + shq(serverPath(PRJ+".git")))
 		if targets, _ := cfg.mirrorTargets(); truthy(cfg.Mirror) && len(activeRemotes(targets)) > 0 {
 			handlePushRemote("") // auto-mirror to every active server
 		}
@@ -442,13 +465,17 @@ func runCommandDepth(line string, depth int) bool {
 		if comment != "" {
 			name = PRJ + "_" + z + "_" + comment
 		}
-		if !sshOK("cp -r " + shq(PRJ+".git") + " " + shq("archive/"+name+".git")) {
+		// the archive directory is mgsh's own convention, so make it rather than
+		// fail on a server where nobody has created it yet
+		if !sshOK("mkdir -p " + shq(serverPath("archive")) +
+			" && cp -r " + shq(serverPath(PRJ+".git")) + " " + shq(serverPath("archive/"+name+".git"))) {
 			break
 		}
-		if !sshOK("cd archive && tar cvzf " + shq(name+".git.tar.gz") + " " + shq(name+".git")) {
+		if !sshOK("tar cvzf " + shq(serverPath("archive/"+name+".git.tar.gz")) +
+			" -C " + shq(serverPath("archive")) + " " + shq(name+".git")) {
 			break
 		}
-		sshOK("rm -rf " + shq("archive/"+name+".git"))
+		sshOK("rm -rf " + shq(serverPath("archive/"+name+".git")))
 
 	case "init": // create a new repository from the current directory
 		if !requireProject() {
@@ -468,11 +495,13 @@ func runCommandDepth(line string, depth int) bool {
 		if exists && !yesno("overwrite existing repository "+PRJ+" on the server?", false) {
 			break
 		}
-		remote := shq(cfg.GitPath + "/" + PRJ + ".git")
+		remote := shq(serverPath(PRJ + ".git"))
 		if !sshOK("rm -rf " + remote) {
 			break
 		}
-		if !sshOK("mkdir " + remote + " && cd " + remote + " && git --bare init") {
+		// `git init --bare <path>` makes the directory itself: one command, and
+		// none of it depends on a `cd` having worked
+		if !sshOK("git init --bare " + remote) {
 			break
 		}
 		gi := DIR + "/.gitignore"
@@ -555,13 +584,14 @@ func runCommandDepth(line string, depth int) bool {
 				break
 			}
 		} else {
-			if !sshOK("cd archive && tar xvzf " + shq(prj+".git.tar.gz")) {
+			if !sshOK("tar xvzf " + shq(serverPath("archive/"+prj+".git.tar.gz")) +
+				" -C " + shq(serverPath("archive"))) {
 				break
 			}
 			if !gitOK(BASE, "clone", URL+"/archive/"+prj+".git") {
 				break
 			}
-			sshOK("rm -rf " + shq("archive/"+prj+".git"))
+			sshOK("rm -rf " + shq(serverPath("archive/"+prj+".git")))
 		}
 		if isDir(BASE + "/" + prj) {
 			PRJ = prj
@@ -572,7 +602,7 @@ func runCommandDepth(line string, depth int) bool {
 		if opt["a"] {
 			path = "./archive"
 		}
-		lines, err := sshOut("/bin/ls " + shq(path))
+		lines, err := sshOut("/bin/ls " + shq(serverPath(path)))
 		if err != nil {
 			errorln("could not list repositories on the git server")
 			break

completion.go

@@ -127,7 +127,7 @@ func fetchServerRepos() {
 	}
 	// a missing ./archive is a permanent, unremarkable state: still cache
 	var archives []string
-	if lines, err := sshOut("/bin/ls archive"); err == nil {
+	if lines, err := sshOut("/bin/ls " + shq(serverPath("archive"))); err == nil {
 		for _, ln := range lines {
 			t := strings.TrimSpace(ln)
 			if strings.HasSuffix(t, ".git.tar.gz") {

git.go

@@ -1,6 +1,7 @@
 package main
 
 import (
+	"errors"
 	"os"
 	"os/exec"
 	"path/filepath"
@@ -127,8 +128,30 @@ var sshExec = func(remote string, capture bool) ([]byte, error) {
 	return out, err
 }
 
+// errRemoteBang is what a command carrying a '!' comes back with, so a caller
+// sees a failure rather than a command that quietly did something else.
+var errRemoteBang = errors.New("remote command contains '!'")
+
+// remoteRejected reports whether a command must not be sent at all, and says
+// why. '!' is the one character shq cannot protect: csh expands history *before*
+// it looks at quotes, and it does so non-interactively too — `echo 'fix!now'`
+// answers "Event not found" on the tcsh server. No spelling survives both csh
+// and sh, so the only safe move is not to send one. This is the place every
+// remote command passes.
+func remoteRejected(remote string) bool {
+	if !strings.ContainsRune(remote, '!') {
+		return false
+	}
+	errorln("not sending a command with '!' in it — the server's login shell " +
+		"would expand it instead of passing it on: " + remote)
+	return true
+}
+
 // ssh runs a single remote command over ssh with inherited stdio.
 func ssh(remote string) error {
+	if remoteRejected(remote) {
+		return errRemoteBang
+	}
 	_, err := sshExec(remote, false)
 	return err
 }
@@ -144,6 +167,9 @@ func sshOK(remote string) bool {
 
 // sshOut runs a remote command and returns its stdout split into lines.
 func sshOut(remote string) ([]string, error) {
+	if remoteRejected(remote) {
+		return nil, errRemoteBang
+	}
 	out, err := sshExec(remote, true)
 	lines := strings.Split(string(out), "\n")
 	for len(lines) > 0 && lines[len(lines)-1] == "" {
@@ -152,10 +178,27 @@ func sshOut(remote string) ([]string, error) {
 	return lines, err
 }
 
+// serverPath anchors a path on the git server at the configured gitpath.
+//
+// Every remote command has to name its target outright, because the login
+// directory of the git user is not necessarily the directory holding the bare
+// repositories. With `gituser = git` and `gitpath = /home/git` the two are the
+// same place and a bare "." worked by luck; with `gituser = root` and
+// `gitpath = /root/mgsh` it lists the home directory, where there is nothing to
+// find.
+func serverPath(rel string) string {
+	base := strings.TrimRight(cfg.GitPath, "/")
+	rel = strings.TrimPrefix(strings.TrimSpace(rel), "./")
+	if rel == "" || rel == "." {
+		return base
+	}
+	return base + "/" + rel
+}
+
 // serverRepoNames lists the bare repositories on the git server, without the
 // ".git" suffix.
 func serverRepoNames() ([]string, error) {
-	lines, err := sshOut("/bin/ls .")
+	lines, err := sshOut("/bin/ls " + shq(serverPath(".")))
 	if err != nil {
 		return nil, err
 	}
@@ -169,10 +212,11 @@ func serverRepoNames() ([]string, error) {
 }
 
 // serverEntryExists reports whether entry is present in the remote directory
-// path (relative to the git user's home). The error is returned rather than
+// path (relative to gitpath, which serverPath resolves). The error is returned
-// folded into the bool so a failed lookup is never mistaken for "not there".
+// rather than folded into the bool so a failed lookup is never mistaken for
+// "not there".
 func serverEntryExists(path, entry string) (bool, error) {
-	lines, err := sshOut("/bin/ls " + shq(path))
+	lines, err := sshOut("/bin/ls " + shq(serverPath(path)))
 	if err != nil {
 		return false, err
 	}

remotecmd_test.go

@@ -101,7 +101,7 @@ func TestInitKeepsServerRepoWhenDeclined(t *testing.T) {
 	if c := findCmd(*sent, "rm -rf"); c != "" {
 		t.Fatalf("init destroyed the server repository after the user declined: %q", c)
 	}
-	if c := findCmd(*sent, "git --bare init"); c != "" {
+	if c := findCmd(*sent, "git init --bare"); c != "" {
 		t.Fatalf("init re-created the repository after the user declined: %q", c)
 	}
 }
@@ -147,7 +147,7 @@ func TestInitOnFreshProjectCreatesRepo(t *testing.T) {
 	if findCmd(*sent, "rm -rf "+shq("/home/git/notes.git")) == "" {
 		t.Errorf("init did not clear the target path, sent: %q", *sent)
 	}
-	if c := findCmd(*sent, "git --bare init"); !strings.Contains(c, shq("/home/git/notes.git")) {
+	if c := findCmd(*sent, "git init --bare"); !strings.Contains(c, shq("/home/git/notes.git")) {
 		t.Errorf("init did not create the bare repository at the configured path: %q", c)
 	}
 }
@@ -224,25 +224,110 @@ func TestArchiveNamesAndQuotesSnapshot(t *testing.T) {
 	}
 	cp, tar, rm := (*sent)[1], (*sent)[2], (*sent)[3]
 
-	// name is <project>_<stamp>_<sanitised comment>
+	// name is <project>_<stamp>_<sanitised comment>, and every path is spelled
+	// out from gitpath — the login directory is not necessarily the same place
 	stamp := archiveStamp()
 	name := "notes_" + stamp + "_before_rewrite"
-	if !strings.HasPrefix(cp, "cp -r "+shq("notes.git")+" ") {
+	// the archive directory is made first: on a server where nobody created it,
+	// the copy used to fail with a raw cp error
+	if !strings.HasPrefix(cp, "mkdir -p "+shq("/home/git/archive")+" && cp -r "+shq("/home/git/notes.git")+" ") {
 		t.Errorf("cp command = %q", cp)
 	}
-	if !strings.Contains(cp, shq("archive/"+name+".git")) {
+	if !strings.Contains(cp, shq("/home/git/archive/"+name+".git")) {
 		t.Errorf("cp target = %q, want it to contain %q", cp, name)
 	}
-	// `cd archive && tar`, not `cd archive;tar`: a failed cd must not let tar
+	// `tar -C <dir>`, not `cd <dir> && tar`: on the tcsh server `cd` is aliased
-	// run in the login directory
+	// to `cd !*;echo $cwd`, which turns the guard into `cd X; echo && tar` —
-	if !strings.HasPrefix(tar, "cd archive && tar ") || !strings.Contains(tar, shq(name+".git.tar.gz")) {
+	// tar then runs in the login directory even when the cd failed, and the
+	// whole command still reports success
+	if !strings.HasPrefix(tar, "tar cvzf "+shq("/home/git/archive/"+name+".git.tar.gz")+" -C "+shq("/home/git/archive")+" ") ||
+		!strings.Contains(tar, shq(name+".git")) {
 		t.Errorf("tar command = %q", tar)
 	}
-	if rm != "rm -rf "+shq("archive/"+name+".git") {
+	if rm != "rm -rf "+shq("/home/git/archive/"+name+".git") {
 		t.Errorf("cleanup command = %q", rm)
 	}
 }
 
+// TestServerCommandsAreAnchoredAtGitPath: mgsh used to address the server
+// through the login directory, which only worked because `gituser = git` and
+// `gitpath = /home/git` happen to be the same place. With `gituser = root` and
+// `gitpath = /root/mgsh` every command went to /root instead — `list` came back
+// empty and `archive` had nothing to copy.
+func TestServerCommandsAreAnchoredAtGitPath(t *testing.T) {
+	useProject(t, "notes")
+	cfg.GitPath = "/root/mgsh" // the repositories are NOT in the login directory
+	sent := fakeServer(t, func(cmd string) (string, error) {
+		if strings.HasPrefix(cmd, "/bin/ls") {
+			return "notes.git\n", nil
+		}
+		return "", nil
+	})
+
+	captureStdout(t, func() {
+		runCommand("list")
+		runCommand("list -a")
+		runCommand("show notes")
+		runCommand("archive")
+	})
+
+	if len(*sent) == 0 {
+		t.Fatal("no remote commands recorded")
+	}
+	for _, c := range *sent {
+		if !strings.Contains(c, "/root/mgsh") {
+			t.Errorf("remote command not anchored at gitpath: %q", c)
+		}
+		// No `cd` either. On the tcsh server it is aliased to `cd !*;echo $cwd`,
+		// which splits `cd X && Y` into `cd X; echo $cwd && Y`: Y runs even when
+		// the cd failed, and the command still exits 0. Every tool mgsh uses can
+		// be told its directory instead — tar -C, git --git-dir, find <path>.
+		if strings.HasPrefix(c, "cd ") || strings.Contains(c, " cd ") {
+			t.Errorf("remote command relies on cd: %q", c)
+		}
+	}
+}
+
+// TestRemoteCommandsRefuseExclamationMark: '!' is the one character shq cannot
+// protect. csh expands history before it looks at quotes, and does so
+// non-interactively too, so `cp -r '/home/git/wei!rd.git' …` arrives as
+// something else. Such a command must not be sent at all.
+func TestRemoteCommandsRefuseExclamationMark(t *testing.T) {
+	useProject(t, "wei!rd")
+	sent := fakeServer(t, func(cmd string) (string, error) {
+		if strings.HasPrefix(cmd, "/bin/ls") {
+			return "wei!rd.git\n", nil // the repository is there, so both proceed
+		}
+		return "", nil
+	})
+
+	out := captureStdout(t, func() {
+		runCommand("show wei!rd")
+		runCommand("archive")
+	})
+
+	for _, c := range *sent {
+		if strings.Contains(c, "!") {
+			t.Errorf("sent a command containing '!': %q", c)
+		}
+	}
+	if c := findCmd(*sent, "--git-dir"); c != "" {
+		t.Errorf("show sent %q despite the '!' in the name", c)
+	}
+	if c := findCmd(*sent, "cp -r"); c != "" {
+		t.Errorf("archive sent %q despite the '!' in the name", c)
+	}
+	if !strings.Contains(out, "not sending") {
+		t.Errorf("output = %q, want the refusal to say what it did not do", out)
+	}
+
+	// the other way in is an archive comment, which is why sanitizeComment
+	// drops the character before it ever becomes part of a name
+	if got := sanitizeComment("fix!now"); strings.ContainsRune(got, '!') {
+		t.Errorf("sanitizeComment(%q) = %q, want the '!' gone", "fix!now", got)
+	}
+}
+
 // TestCloneRefusesUnknownRepository: `clone` must not start a git clone for a
 // repository the server does not list.
 func TestCloneRefusesUnknownRepository(t *testing.T) {
@@ -389,5 +474,15 @@ func TestListSendsNoShellSpecificSyntax(t *testing.T) {
 		if strings.Contains(c, "2>") || strings.Contains(c, "&>") {
 			t.Errorf("remote command uses sh-only redirection: %q", c)
 		}
+		// A glob is expanded by that same login shell, and a non-interactive
+		// zsh that finds nothing to match does not pass the pattern on like sh
+		// does — it fails the command outright ("no matches found: *.git"),
+		// which is how an empty server came to be reported as unreachable.
+		for _, idx := range indexesOf(c, "*") {
+			if idx == 0 || c[idx-1] != '\'' {
+				t.Errorf("unquoted glob in remote command: %q", c)
+				break
+			}
+		}
 	}
 }

version.txt

@@ -1 +1 @@
-4.0.66
+4.0.69