Catch os... errors #320

Bwko · 2016-11-30T23:27:52Z

No description provided.

tboerger · 2016-12-01T06:55:11Z

cmd/dump.go

@@ -79,7 +79,10 @@ func runDump(ctx *cli.Context) error {
 	log.Printf("Packing dump files...")
 	z, err := zip.Create(fileName)
 	if err != nil {
-		os.Remove(fileName)
+		osErr := os.Remove(fileName)


Maybe we should keep ignoring this error?

_ := os.Remove(fileName) ?

Here's a dump question... Why is it deleting a file it can't create in the first place?

Good catch :D

Maybe zip.Create might fail some some after file creation?

tboerger · 2016-12-01T06:55:34Z

cmd/dump.go

@@ -102,7 +105,10 @@ func runDump(ctx *cli.Context) error {
 	}
 	// FIXME: SSH key file.
 	if err = z.Close(); err != nil {
-		os.Remove(fileName)
+		osErr := os.Remove(fileName)


tboerger · 2016-12-01T06:57:33Z

cmd/serve.go

@@ -13,6 +13,8 @@ import (
 	"strings"
 	"time"

+	goLog "log"


Do we already have a logger here?

modules/log, not sure which one to use in this case...

tboerger · 2016-12-01T06:57:57Z

cmd/web.go

-
+		osErr := os.Remove(listenAddr)
+		if osErr != nil {
+			osLog.Fatalf("Fail to remove unix socket directory %s: %v", listenAddr, osErr)


Just use the already available logger?

tboerger · 2016-12-01T06:59:01Z

main.go

@@ -37,5 +38,7 @@ func main() {
 		cmd.CmdAdmin,
 	}
 	app.Flags = append(app.Flags, []cli.Flag{}...)
-	app.Run(os.Args)
+	err := app.Run(os.Args)


Generally I'm wrapping this directly into println

@tboerger Don't do that. Last time, it results in a bug I spent many time to fix it.

But now it's not a big difference

It's not using any logger, it's only printing to stdout. So we will have the same bug again

yes, don't output anything to stdout when git via ssh.

@lunny how do you want me to fix this?

@lunny cmd/serve.go always returns nil, so this isn't really an issue here :)

tboerger · 2016-12-01T06:59:37Z

models/pull.go

+	file := path.Join(pr.BaseRepo.RepoPath(), headFile)
+	err = os.Remove(file)
+	if err != nil {
+		return fmt.Errorf("Fail to removee dir %s: %v", path.Join(pr.BaseRepo.RepoPath(), headFile), err)


bkcsoft

Don't do this 😆

bkcsoft · 2016-12-01T08:33:44Z

cmd/serve.go

-		os.Chdir(workDir)
+		osErr := os.Chdir(workDir)
+		if osErr != nil {
+			goLog.Fatalf("Fail to change directory %s: %v", workDir, osErr)


Printing server-internal dirs to any random client?! 😱

bkcsoft · 2016-12-01T08:37:20Z

Overall, change the format to this instead whenever possible. Keeps errors(variables) bound to their context(block).

if err := os.Remove(...); err != nil {
  [...]
}

lunny · 2016-12-01T11:23:02Z

cmd/web.go

@@ -8,6 +8,7 @@ import (
 	"crypto/tls"
 	"fmt"
 	"io/ioutil"
+	osLog "log"


lunny · 2016-12-01T11:24:12Z

main.go

@@ -37,5 +38,7 @@ func main() {
 		cmd.CmdAdmin,
 	}
 	app.Flags = append(app.Flags, []cli.Flag{}...)
-	app.Run(os.Args)
+	err := app.Run(os.Args)
+	fmt.Printf("Fail to run app with %s: %v", os.Args, err)


if err != nil

lunny · 2016-12-01T11:25:11Z

modules/log/log.go

@@ -6,6 +6,7 @@ package log

 import (
 	"fmt"
+	"log"


lunny · 2016-12-01T11:25:22Z

modules/ssh/ssh.go

@@ -8,6 +8,7 @@ import (
 	"fmt"
 	"io"
 	"io/ioutil"
+	osLog "log"


Bwko · 2016-12-01T22:57:24Z

I've updated my PR.

lunny · 2016-12-02T01:12:33Z

LGTM

bkcsoft · 2016-12-02T06:41:02Z

LGTM

tboerger · 2016-12-02T12:51:39Z

This partially solves #257

Suppress the error when we're removing a file that may not exist

Fix for #320

Bwko force-pushed the Errcheck branch from 858d2a7 to 1c51417 Compare November 30, 2016 23:31

tboerger reviewed Dec 1, 2016

View reviewed changes

bkcsoft suggested changes Dec 1, 2016

View reviewed changes

tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 1, 2016

lunny reviewed Dec 1, 2016

View reviewed changes

cmd/web.go

@@ -8,6 +8,7 @@ import (

"crypto/tls"

"fmt"

"io/ioutil"

osLog "log"

Copy link

Member

lunny Dec 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

lunny requested changes Dec 1, 2016

View reviewed changes

Bwko force-pushed the Errcheck branch 6 times, most recently from 8fa7687 to 0ebf310 Compare December 1, 2016 20:33

Catch os... errors

0ebf310

lunny added this to the 1.0.0 milestone Dec 2, 2016

tboerger added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Dec 2, 2016

lunny approved these changes Dec 2, 2016

View reviewed changes

bkcsoft approved these changes Dec 2, 2016

View reviewed changes

tboerger added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Dec 2, 2016

bkcsoft merged commit 4ff0db0 into go-gitea:master Dec 2, 2016

Bwko deleted the Errcheck branch December 2, 2016 11:31

tboerger added the type/enhancement An improvement of existing functionality label Dec 2, 2016

Bwko added a commit to Bwko/gitea that referenced this pull request Dec 3, 2016

Fix for go-gitea#320

0118b27

Suppress the error when we're removing a file that may not exist

andreynering added a commit that referenced this pull request Dec 3, 2016

Merge pull request #345 from Bwko/fix_for_320

dc14d0c

Fix for #320

go-gitea locked and limited conversation to collaborators Nov 23, 2020

Uh oh!

Catch os... errors #320

Catch os... errors #320

Uh oh!

Conversation

Bwko commented Nov 30, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bkcsoft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bkcsoft commented Dec 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bwko commented Dec 1, 2016

Uh oh!

lunny commented Dec 2, 2016

Uh oh!

bkcsoft commented Dec 2, 2016

Uh oh!

tboerger commented Dec 2, 2016

Uh oh!

Uh oh!