From dfdd159c9c11c08d84c8c050d2a1a4db29147916 Mon Sep 17 00:00:00 2001 From: Omar Jarjur Date: Tue, 12 Jan 2016 17:40:47 -0800 Subject: [PATCH 1/2] 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 | 32 ++--------------- commands/input/input.go | 79 +++++++++++++++++++++++++++++++++++++++++ commands/reject.go | 32 ++--------------- 3 files changed, 85 insertions(+), 58 deletions(-) create mode 100644 commands/input/input.go diff --git a/commands/comment.go b/commands/comment.go index a79c28d8..e4cd1739 100644 --- a/commands/comment.go +++ b/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() diff --git a/commands/input/input.go b/commands/input/input.go new file mode 100644 index 00000000..9999c593 --- /dev/null +++ b/commands/input/input.go @@ -0,0 +1,79 @@ +/* +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. +// +// 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 failse 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 +} diff --git a/commands/reject.go b/commands/reject.go index cac6548c..b3681b0f 100644 --- a/commands/reject.go +++ b/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() From aee6f75ddd9ef88304e58356aa8d331e6923f972 Mon Sep 17 00:00:00 2001 From: Omar Jarjur Date: Thu, 14 Jan 2016 10:29:59 -0800 Subject: [PATCH 2/2] Improved the godoc and inline comments for the LaunchEditor method --- commands/input/input.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/commands/input/input.go b/commands/input/input.go index 9999c593..c860c28a 100644 --- a/commands/input/input.go +++ b/commands/input/input.go @@ -24,7 +24,8 @@ import ( "os/exec" ) -// LaunchEditor launches the default editor configured for the given repo. +// 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 @@ -45,7 +46,7 @@ func LaunchEditor(repo repository.Repo, fileName string) (string, error) { // 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 failse try with sh + // 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 {