Merge lp:~stefan.goetz-deactivatedaccount/hipl/commitguard-usability into lp:hipl

[Stefan Götz]

Improves the usability of the commitguard plugin by making its output more readable and improving the interaction with the user.

[Diego Biurrun (diego-biurrun)]

 review abstain

On Sat, Jan 21, 2012 at 11:38:39AM +0000, Stefan Götz wrote:
> Stefan Götz has proposed merging lp:~stefan.goetz/hipl/commitguard-usability into lp:hipl with lp:~stefan.goetz/hipl/commitguard-whitespace as a prerequisite.

This is Python, so I cannot really review. I'll trust you on it, though.

> --- .commitguards/copyright-year.guard 2012-01-21 11:37:29 +0000
> +++ .commitguards/copyright-year.guard 2012-01-21 11:37:29 +0000
> @@ -24,4 +24,4 @@
> [guard]
> files=\.(c|h|am|ac|in|py|sh|guard)$
> actions=amr
> -command=CR_TOKEN=" Copyright \(c\) "; CR_HOLDER=" Aalto "; Y=$(date +%Y); if ! egrep "$CR_TOKEN.*$CR_HOLDER" "{}" | egrep -q "\b$Y\b"; then echo "{}: The copyright statement is not up-to-date"; L=$(( $Y - 1 )); sed -r "/ Copyright \(c\) / { /\b$Y\b/ ! { /-$L / s/-$L /-$Y /; s/ $L / $L-$Y /; /\b$Y\b/ ! s/ ([0-9, -]+) ([a-zA-Z]+)/ \1, $Y \2/ } }" "{}" | diff -u "{}" - >&2; false; fi
> +command=CR_TOKEN=" Copyright \(c\) "; CR_HOLDER=" Aalto "; Y=$(date +%Y); if ! egrep "$CR_TOKEN.*$CR_HOLDER" "{}" | egrep -q "\b$Y\b"; then echo "The copyright statement is not up-to-date"; L=$(( $Y - 1 )); sed -r "/ Copyright \(c\) / { /\b$Y\b/ ! { /-$L / s/-$L /-$Y /; s/ $L / $L-$Y /; /\b$Y\b/ ! s/ ([0-9, -]+) ([a-zA-Z]+)/ \1, $Y \2/ } }" "{}" | diff -u "{}" - >&2; false; fi

Maybe you could break this sequence of commands:

> +command=CR_TOKEN=" Copyright \(c\) ";
                      CR_HOLDER=" Aalto ";
                      Y=$(date +%Y);
                      if ! egrep "$CR_TOKEN.*$CR_HOLDER" "{}" | egrep -q "\b$Y\b"; then
                          echo "The copyright statement is not up-to-date";
                          L=$(( $Y - 1 ));
                          sed -r "/ Copyright \(c\) / { /\b$Y\b/ ! { /-$L / s/-$L /-$Y /; s/ $L / $L-$Y /; /\b$Y\b/ ! s/ ([0-9, -]+) ([a-zA-Z]+)/ \1, $Y \2/ } }" "{}" | diff -u "{}" - >&2;
                          false;
                      fi

This might even work if the ';' are replaced by '\'. :)

Diego

review: Abstain

[Stefan Götz (stefan.goetz-deactivatedaccount)]

Hi Diego!

Thanks for the review!

> Maybe you could break this sequence of commands:

Thanks for making me look at that - it turns out that the python parser for
these guard files has support for line continuations built-in and the standard
line breaks and indentations of shell scripts can be used in those files which
makes them much more readable. I updated all guards on all commitguard branches
accordingly.

Cheers,
 Stefan

[René Hummen (rene-hummen)]

review: Approve

[Stefan Götz (stefan.goetz-deactivatedaccount)]

Hi Rene!

> I just got a suggested workflow that would allow merging of the stylechecker with this script:
> 1) Make a local copy of the files touched in the current commit.
> 2) Apply the commit guards (including uncrustify) one after the other to each of the local copies.
> 2.1) Tell the user which guard failed on which file.
> 2.2) Apply an eventual change of a commit guard to the local copy.
> 3) After all commit guards have been executed (and the corresponding changes have been applied to the local copies incrementally), show the user the diff between the local files and the files in the commit.
> 3.1) Apply diff, if accepted by the user.

That makes a lot of sense and is certainly more usable than the current
approach. I'll work on it on the usability branch.

> Additionally, it might be worth having an install_bzr_hooks.sh script that copies the scripts to one of the two valid bazaar plugin locations in order to increase usage and acceptance.

Yeah, well, I don't think the difference between a single cp command and a
script will change something about usage and acceptance - so I'll let someone
else write that script :)

Stefan

[lp:~stefan.goetz-deactivatedaccount/hipl/commitguard-usability]

6278. By Stefan Götz on 2012-01-22

Improve readability of output when a commit guard fails

6279. By Stefan Götz on 2012-01-22

Simplify output of commit guards. They do not need to print the name of
the affected file because the commit guard plugin takes care of that.

6280. By Stefan Götz on 2012-01-22

Make output more readable by asking for user intervention before
displaying a potentially long diff.

Unmerged revisions

6289. By Stefan Götz on 2012-02-11

When a file is added to a shadow repository, create it on the file system as a copy of the original

6288. By Stefan Götz on 2012-02-11

Ensure that both directory name strings in ShadowRepo do not end with a slash and make sure that the repository base directory is determined correctly

6287. By Stefan Götz on 2012-02-11

Create and delete shadow repository object, but do not use it

6286. By Stefan Götz on 2012-02-11

Add function for retrieving a patch file for the differences between the repository and the shadow repository

6285. By Stefan Götz on 2012-02-11

Add function for deleting a shadow repository

6284. By Stefan Götz on 2012-02-03

Add ShadowRepo class as infrastructure for in-place editing by commitguards

6283. By Stefan Götz on 2012-02-03

Add class definition that is part of new infrastructure for in-place editing through commitguards

6282. By Stefan Götz on 2012-02-01

Apply all commit guards to all committed files instead of aborting on the first violation

6281. By Stefan Götz on 2012-02-01

Remove the ability to apply patches printed on stderr by commit guards

6280. By Stefan Götz on 2012-01-22

Make output more readable by asking for user intervention before
displaying a potentially long diff.

=== modified file '.commitguards/author-attribution.guard'
--- .commitguards/author-attribution.guard	2012-01-22 13:08:25 +0000
+++ .commitguards/author-attribution.guard	2012-01-22 13:08:25 +0000
@@ -26,6 +26,6 @@
 actions=amr
 command=
     if AUTHORS="$(egrep "@author\b" "{}")"; then
-        echo "{}: files should not contain author attributions such as '$(echo "$AUTHORS" | head -n 1)'"
+        echo "Authorship attributions (such as '$(echo "$AUTHORS" | head -n 1)') should be removed"
         false
     fi

=== modified file '.commitguards/copyright-guard-space.py'
--- .commitguards/copyright-guard-space.py	2012-01-22 13:08:25 +0000
+++ .commitguards/copyright-guard-space.py	2012-01-22 13:08:25 +0000
@@ -61,5 +61,5 @@
     fh.close()
 
     if not match:
-        print 'The copyright header in file %s should be followed by an empty line' % (sys.argv[1])
+        print 'The copyright header should be followed by an empty line'
         sys.exit(1)

=== modified file '.commitguards/copyright-year.guard'
--- .commitguards/copyright-year.guard	2012-01-22 13:08:25 +0000
+++ .commitguards/copyright-year.guard	2012-01-22 13:08:25 +0000
@@ -29,7 +29,7 @@
     CR_HOLDER=" Aalto "
     Y=$(date +%Y)
     if ! egrep "$CR_TOKEN.*$CR_HOLDER" "{}" | egrep -q "\b$Y\b"; then
-        echo "{}: The copyright statement is not up-to-date"
+        echo "The copyright statement is not up-to-date"
         L=$(( $Y - 1 ))
         sed -r "/ Copyright \(c\) / { /\b$Y\b/ ! { /-$L / s/-$L /-$Y /; s/ $L / $L-$Y /; /\b$Y\b/ ! s/ ([0-9, -]+) ([a-zA-Z]+)/ \1, $Y \2/ } }" "{}" | diff -u "{}" - >&2
         false

=== modified file 'tools/bazaar/plugins/commitguard.py'
--- tools/bazaar/plugins/commitguard.py	2012-01-22 13:08:25 +0000
+++ tools/bazaar/plugins/commitguard.py	2012-01-22 13:08:25 +0000
@@ -310,12 +310,14 @@
     guards = create_guards(local or master)
 
     veto = False
+    info_file = tempfile.NamedTemporaryFile(prefix = plugin_name + "-msg-")
     diff_file = tempfile.NamedTemporaryFile(prefix = plugin_name + "-diff-")
 
+    tree_file_name = None
     for tree_file_name, action in get_commit_files(tree_delta):
         local_file_name = get_local_file_name(local or master, tree_file_name)
         for guard in guards:
-            if not guard.run(local_file_name, action, sys.stdout, diff_file):
+            if not guard.run(local_file_name, action, info_file, diff_file):
                 veto = True
                 break
 
@@ -323,16 +325,23 @@
             break
 
     if veto:
+        info_file.flush()
+        info_file.seek(0)
+        print "\n\n%s:\n   " % (tree_file_name), info_file.read()
+        info_file.close()
+
         diff_file.flush()
         if diff_file.tell():
-            print "\nThe following patch is suggested so as to comply with the commit guard requirements:\n"
-            diff_file.seek(0)
-            print diff_file.read()
-            print "\nWould you like to apply these changes to your local branch now? [y/N] ",
+            print "Would you like to review a patch that solves this issue? [y|N] ",
             reply = sys.stdin.readline()
             if reply.strip() == 'y':
-                apply_patch(diff_file)
-                print "Changes successfully applied.\n"
+                diff_file.seek(0)
+                print "\n", diff_file.read()
+                print "\nWould you like to apply these changes to your local branch now? [y/N] ",
+                reply = sys.stdin.readline()
+                if reply.strip() == 'y':
+                    apply_patch(diff_file)
+                    print "Changes successfully applied.\n"
         diff_file.close()
 
         msg_file = tempfile.NamedTemporaryFile(prefix = 'commit-msg-%d-' % future_revno, delete = False)
@@ -340,6 +349,7 @@
         msg_file.close()
 
         raise bzrlib.errors.BzrError("This commit has been aborted. The original commit message, stored in %s, was:\n--\n%s\n--" % (msg_file.name, get_commit_message(local, master, future_revid)))
+    info_file.close()
     diff_file.close()