Submitting review dfdd159

Consolidate the editor logic into a single place, and make it more reliable.

With this change we will try to run the editor in a shell if launching it directly does not work.

commands/comment.go

@@ -20,12 +20,10 @@ import (
 	"errors"
 	"flag"
 	"fmt"
+	"github.com/google/git-appraise/commands/input"
 	"github.com/google/git-appraise/repository"
 	"github.com/google/git-appraise/review"
 	"github.com/google/git-appraise/review/comment"
-	"io/ioutil"
-	"os"
-	"os/exec"
 )
 
 var commentFlagSet = flag.NewFlagSet("comment", flag.ExitOnError)
@@ -87,34 +85,10 @@ func commentOnReview(repo repository.Repo, args []string) error {
 	}
 
 	if *commentMessage == "" {
-		editor, err := repo.GetCoreEditor()
+		*commentMessage, err = input.LaunchEditor(repo, commentFilename)
 		if err != nil {
-			return fmt.Errorf("Unable to detect default git editor: %v\n", err)
+			return err
 		}
-
-		path := fmt.Sprintf("%s/.git/%s", repo.GetPath(), commentFilename)
-
-		cmd := exec.Command(editor, path)
-		cmd.Stdin = os.Stdin
-		cmd.Stdout = os.Stdout
-		cmd.Stderr = os.Stderr
-		err = cmd.Start()
-		if err != nil {
-			return fmt.Errorf("Unable to start editor: %v\n", err)
-		}
-
-		err = cmd.Wait()
-		if err != nil {
-			return fmt.Errorf("Editing finished with error: %v\n", err)
-		}
-
-		comment, err := ioutil.ReadFile(path)
-		if err != nil {
-			os.Remove(path)
-			return fmt.Errorf("Error reading comment file: %v\n", err)
-		}
-		*commentMessage = string(comment)
-		os.Remove(path)
 	}
 
 	commentedUponCommit, err := r.GetHeadCommit()

commands/input/input.go

@@ -0,0 +1,80 @@
+/*
+Copyright 2015 Google Inc. All rights reserved.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package input
+
+import (
+	"fmt"
+	"github.com/google/git-appraise/repository"
+	"io/ioutil"
+	"os"
+	"os/exec"
+)
+
+// LaunchEditor launches the default editor configured for the given repo. This
+// method blocks until the editor command has returned.
+//
+// The specified filename should be a temporary file and provided as a relative path
+// from the repo (e.g. "FILENAME" will be converted to ".git/FILENAME"). This file
+// will be deleted after the editor is closed and its contents have been read.
+//
+// This method returns the text that was read from the temporary file, or
+// an error if any step in the process failed.
+func LaunchEditor(repo repository.Repo, fileName string) (string, error) {
+	editor, err := repo.GetCoreEditor()
+	if err != nil {
+		return "", fmt.Errorf("Unable to detect default git editor: %v\n", err)
+	}
+
+	path := fmt.Sprintf("%s/.git/%s", repo.GetPath(), fileName)
+
+	cmd, err := startInlineCommand(editor, path)
+	if err != nil {
+		// Running the editor directly did not work. This might mean that
+		// the editor string is not a path to an executable, but rather
+		// a shell command (e.g. "emacsclient --tty"). As such, we'll try
+		// to run the command through bash, and if that fails, try with sh
+		args := []string{"-c", fmt.Sprintf("%s %q", editor, path)}
+		cmd, err = startInlineCommand("bash", args...)
+		if err != nil {
+			cmd, err = startInlineCommand("sh", args...)
+		}
+	}
+	if err != nil {
+		return "", fmt.Errorf("Unable to start editor: %v\n", err)
+	}
+
+	if err := cmd.Wait(); err != nil {
+		return "", fmt.Errorf("Editing finished with error: %v\n", err)
+	}
+
+	output, err := ioutil.ReadFile(path)
+	if err != nil {
+		os.Remove(path)
+		return "", fmt.Errorf("Error reading edited file: %v\n", err)
+	}
+	os.Remove(path)
+	return string(output), err
+}
+
+func startInlineCommand(command string, args ...string) (*exec.Cmd, error) {
+	cmd := exec.Command(command, args...)
+	cmd.Stdin = os.Stdin
+	cmd.Stdout = os.Stdout
+	cmd.Stderr = os.Stderr
+	err := cmd.Start()
+	return cmd, err
+}

commands/reject.go

@@ -20,12 +20,10 @@ import (
 	"errors"
 	"flag"
 	"fmt"
+	"github.com/google/git-appraise/commands/input"
 	"github.com/google/git-appraise/repository"
 	"github.com/google/git-appraise/review"
 	"github.com/google/git-appraise/review/comment"
-	"io/ioutil"
-	"os"
-	"os/exec"
 )
 
 var rejectFlagSet = flag.NewFlagSet("reject", flag.ExitOnError)
@@ -60,34 +58,10 @@ func rejectReview(repo repository.Repo, args []string) error {
 	}
 
 	if *rejectMessage == "" {
-		editor, err := repo.GetCoreEditor()
+		*rejectMessage, err = input.LaunchEditor(repo, rejectFilename)
 		if err != nil {
-			return fmt.Errorf("Unable to detect default git editor: %v\n", err)
+			return err
 		}
-
-		path := fmt.Sprintf("%s/.git/%s", repo.GetPath(), rejectFilename)
-
-		cmd := exec.Command(editor, path)
-		cmd.Stdin = os.Stdin
-		cmd.Stdout = os.Stdout
-		cmd.Stderr = os.Stderr
-		err = cmd.Start()
-		if err != nil {
-			return fmt.Errorf("Unable to start editor: %v\n", err)
-		}
-
-		err = cmd.Wait()
-		if err != nil {
-			return fmt.Errorf("Editing finished with error: %v\n", err)
-		}
-
-		comment, err := ioutil.ReadFile(path)
-		if err != nil {
-			os.Remove(path)
-			return fmt.Errorf("Error reading comment file: %v\n", err)
-		}
-		*rejectMessage = string(comment)
-		os.Remove(path)
 	}
 
 	rejectedCommit, err := r.GetHeadCommit()