[mike@mwxm4]

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
-	// run in the login directory
-	if !strings.HasPrefix(tar, "cd archive && tar ") || !strings.Contains(tar, shq(name+".git.tar.gz")) {
+	// `tar -C <dir>`, not `cd <dir> && tar`: on the tcsh server `cd` is aliased
+	// to `cd !*;echo $cwd`, which turns the guard into `cd X; echo && tar` —
+	// 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
+			}
+		}
 	}
 }