Commits

Patrick Mézard committed 4a7793c

gofmt: add gofmt command, fix CL.Upload() format checking

CheckGofmt was being called on working copy files which is correct most of the
time but not always. The new version spawns one gofmt per file and pipe the
content. Maybe that will prove too slow.

Comments (0)

Files changed (4)

 * `clpush`: create or update a changelist from a Mercurial changeset
 * `clclose`: close and optionally delete a changelist bound to a changeset
 * `climport`: turn a changelist into a regular local changeset
+* `gofmt`: run gofmt on changed files or changeset files.
 
 See `hg help codereview` or `hg help COMMAND` for more details.
 

lib/codereview/codereview.py

 #######################################################################
 # Helpers
 
-def RelativePath(path, cwd):
-	n = len(cwd)
-	if path.startswith(cwd) and path[n] == '/':
-		return path[n+1:]
-	return path
-
 def Sub(l1, l2):
 	return [l for l in l1 if l not in l2]
 
 		if not self.files:
 			ui.warn("no files in change list\n")
 		if ui.configbool("codereview", "force_gofmt", True):
-			CheckFormat(ui, repo, self.files)
+			if gofmtfiles(ui, ctx, list(ctx.files())):
+				raise hg_util.Abort('gofmt failed, stopping changelist upload')
 		set_status("uploading CL metadata + diffs")
 		baserev = str(ctx.p1())
 		form_fields = [
 #######################################################################
 # File format checking.
 
-def CheckFormat(ui, repo, files):
-	set_status("running gofmt")
-	CheckGofmt(ui, repo, files, False)
+def gofmtsingle(name, data):
+	p = subprocess.Popen(['gofmt', '-l'], close_fds=os.name=='posix',
+			stdin=subprocess.PIPE, stdout=subprocess.PIPE,
+			stderr=subprocess.STDOUT)
+	out, _ = p.communicate(data)
+	out = out.replace('<standard input>', name).strip()
+	if out:
+		out = out + '\n'
+	if p.returncode and not out:
+		out = 'gofmt failed for unknown reason'
+	return out
 
-# Check that gofmt run on the list of files does not change them
-def CheckGofmt(ui, repo, files, just_warn):
-	files = gofmt_required(files)
-	if not files:
-		return
-	cwd = os.getcwd()
-	files = [RelativePath(repo.root + '/' + f, cwd) for f in files]
-	files = [f for f in files if os.access(f, 0)]
-	if not files:
-		return
-	try:
-		cmd = subprocess.Popen(["gofmt", "-l"] + files, shell=False, stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE, close_fds=sys.platform != "win32")
-		cmd.stdin.close()
-	except:
-		raise hg_util.Abort("gofmt: " + ExceptionDetail())
-	data = cmd.stdout.read()
-	errors = cmd.stderr.read()
-	cmd.wait()
-	set_status("done with gofmt")
-	if len(errors) > 0:
-		ui.warn("gofmt errors:\n" + errors.rstrip() + "\n")
-		return
-	if len(data) > 0:
-		msg = "gofmt needs to format these files (run hg gofmt):\n" + Indent(data, "\t").rstrip()
-		if just_warn:
-			ui.warn("warning: " + msg + "\n")
-		else:
-			raise hg_util.Abort(msg)
-	return
+def gofmtfiles(ui, ctx, files, header=None):
+	if header is None:
+		header = 'error: gofmt failed:\n'
+	res = 0
+	for f in files:
+		if not f.endswith('.go') or f not in ctx:
+			continue
+		errs = gofmtsingle(f, ctx[f].data())
+		if errs:
+			if not res:
+				res = 1
+				if header:
+					ui.warn(header)
+			ui.warn(errs)
+	return res
 
 #######################################################################
 # Mercurial helper functions.
 		return path.replace('\\', '/')
 	return path
 
-def gofmt_required(files):
-	return [f for f in files if (not f.startswith('test/') or f.startswith('test/bench/')) and f.endswith('.go')]
-
 #######################################################################
 # Wrappers around upload.py for interacting with Rietveld
 
 
 	MySend(None)
 
+@hgcommand('gofmt',
+	[('a', 'all', False, 'check all changeset Go files'),
+	 ('r', 'rev', '', 'changeset to check', 'REV'),
+	 ] + commands.walkopts,
+	'[-r REV] [FILE]...')
+def gofmt(ui, repo, *pats, **opts):
+	"""check changeset Go files formatting
+
+	With no argument, runs gofmt on all modified Go files in the working
+	copy. With --rev, runs gofmt on file modified by specified changeset,
+	as they were in the changeset.
+
+	Use --all to run gofmt on all working copy or changeset files.
+
+	Return 1 upon incorrectly formatted file.
+	"""
+	rev = opts.get('rev')
+	ctx = scmutil.revsingle(repo, rev, default=None)
+	m = scmutil.match(ctx, pats, opts)
+	if opts.get('all'):
+		files = list(ctx)
+	else:
+		files = list(ctx.files())
+	files = [f for f in files if m(f)]
+	return gofmtfiles(ui, ctx, files, header='')
+
 def extsetup():
 	try:
 		from mercurial import templatekw

tests/test-clpush.t

   
   CLMETA-URL: https://codereview.appspot.com/\d+ (re)
 
-Updating a changelist
+Updating a changelist, invalid Go file
 
   $ echo a >> a
+  $ cat > test.go <<EOF
+  >   package test
+  > EOF
+  $ hg add test.go
+  $ hg amend
+  $ hg clpush .
+  error: gofmt failed:
+  test.go
+  abort: gofmt failed, stopping changelist upload
+  [255]
+  $ cat > test.go <<EOF
+  > package test
+  > EOF
   $ hg amend
   $ hg clpush .
   Issue updated. URL: https://codereview.appspot.com/\d+ (re)
   @@ -0,0 +1,2 @@
   +a
   +a
+  Index: test.go
+  ===================================================================
+  new file mode 100644
+  --- /dev/null
+  +++ b/test.go
+  @@ -0,0 +1,1 @@
+  +package test
 
 Test clclose arguments
 

tests/test-gofmt.t

+  $ echo "[extensions]" >> $HGRCPATH
+  $ echo "codereview=$(echo $(dirname $TESTDIR))/lib/codereview/codereview.py" >> $HGRCPATH
+  $ echo "rebase=" >> $HGRCPATH
+  $ cp "$TESTDIR/.codereview_upload_cookies_codereview.appspot.com" ~/
+
+  $ hg init repo
+  $ cd repo
+  $ cat > invalid1.go <<EOF
+  > package  test
+  > 
+  > type MyInt  int32
+  > EOF
+  $ cp invalid1.go invalid2.go
+  $ cat >> invalid2.go <<EOF
+  > func MyFunc error {
+  >   return nil
+  > }
+  > EOF
+  $ cp invalid1.go invalid3.go
+  $ cp invalid1.go valid.go
+  $ gofmt -w valid.go
+  $ cp invalid1.go notgo.txt
+  $ hg add invalid1.go invalid2.go invalid3.go valid.go notgo.txt
+  $ hg commit -m init
+
+Parent changeset
+
+  $ hg gofmt -r .
+  invalid1.go
+  invalid2.go:4:13: expected '(', found 'IDENT' error
+  invalid2.go:5:3: expected ')', found 'return'
+  invalid2.go:6:1: expected declaration, found '}'
+  invalid3.go
+  [1]
+
+Working directory
+
+  $ hg cp invalid1.go invalid4.go
+  $ gofmt -w invalid1.go
+  $ hg rm invalid3.go
+  $ hg gofmt
+  invalid4.go
+  [1]
+
+Again as a changeset
+
+  $ hg ci -m change
+  $ hg gofmt -r .
+  invalid4.go
+  [1]
+
+Non-parent changeset
+
+  $ hg gofmt -r .^
+  invalid1.go
+  invalid2.go:4:13: expected '(', found 'IDENT' error
+  invalid2.go:5:3: expected ')', found 'return'
+  invalid2.go:6:1: expected declaration, found '}'
+  invalid3.go
+  [1]
+
+With --include --exclude
+
+  $ hg gofmt -r .^ -X invalid2.go
+  invalid1.go
+  invalid3.go
+  [1]
+
+With --all
+
+  $ hg gofmt -r . --all
+  invalid2.go:4:13: expected '(', found 'IDENT' error
+  invalid2.go:5:3: expected ')', found 'return'
+  invalid2.go:6:1: expected declaration, found '}'
+  invalid4.go
+  [1]
+