[PATCH v6] gnu: python-sphinx: Update to 1.4.8.

[PATCH v6] gnu: python-sphinx: Update to 1.4.8.

[Danny Milosavljevic]

* gnu/packages/python.scm (python-sphinx)[version]: Update to 1.4.8.
  [source]: Use pypi-uri.
  [propagated-inputs]: Add python-imagesize, python-sphinx-alabaster-theme,
  python-babel, python-snowballstemmer, python-six.
  [properties]: Add python2-variant.
(python2-sphinx)[native-inputs]: Add python2-mock.
  [propagated-inputs]: Add python2-pytz.
---
 gnu/packages/python.scm | 31 ++++++++++++++++++++++---------
 1 file changed, 22 insertions(+), 9 deletions(-)
---
 gnu/packages/python.scm | 38 ++++++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 14 deletions(-)

diff --git a/gnu/packages/python.scm b/gnu/packages/python.scm
index 4e4a15feb..721b7a993 100644
--- a/gnu/packages/python.scm
+++ b/gnu/packages/python.scm
@@ -2907,30 +2907,40 @@ reStructuredText.")
 (define-public python-sphinx
   (package
     (name "python-sphinx")
-    (version "1.2.3")
-    (source
-     (origin
-       (method url-fetch)
-       (uri (string-append
-             "https://pypi.python.org/packages/source/S/Sphinx/Sphinx-"
-             version ".tar.gz"))
-       (sha256
-        (base32
-         "011xizm3jnmf4cvs5i6kgf6c5nn046h79i8j0vd0f27yw9j3p4wl"))))
+    (version "1.4.8")
+    (source (origin
+              (method url-fetch)
+              (uri (pypi-uri "Sphinx" version))
+              (sha256
+                (base32
+                  "0zvh8wwhm6gy21rr0cg42znsy4zzv2mnsxxk9gmn5y1ycn7rgbs1"))))
     (build-system python-build-system)
     (propagated-inputs
-     `(("python-jinja2" ,python-jinja2)
+     `(("python-imagesize" ,python-imagesize)
+       ("python-sphinx-alabaster-theme"
+        ,python-sphinx-alabaster-theme)
+       ("python-babel" ,python-babel)
+       ("python-snowballstemmer" ,python-snowballstemmer)
        ("python-docutils" ,python-docutils)
-       ("python-pygments" ,python-pygments)))
+       ("python-jinja2" ,python-jinja2)
+       ("python-pygments" ,python-pygments)
+       ("python-six" ,python-six)))
     (home-page "http://sphinx-doc.org/")
     (synopsis "Python documentation generator")
     (description "Sphinx is a tool that makes it easy to create documentation
 for Python projects or other documents consisting of multiple reStructuredText
 sources.")
-    (license license:bsd-3)))
+    (license license:bsd-3)
+    (properties `((python2-variant . ,(delay python2-sphinx))))))
 
 (define-public python2-sphinx
-  (package-with-python2 python-sphinx))
+  (let ((base (package-with-python2 (strip-python2-variant python-sphinx))))
+    (package
+      (inherit base)
+      (native-inputs `(("python2-mock" ,python2-mock)
+                       ,@(package-native-inputs base)))
+      (propagated-inputs `(("python2-pytz" ,python2-pytz)
+                       ,@(package-propagated-inputs base))))))
 
 (define-public python-sphinx-rtd-theme
   (package

Re: [PATCH v6] gnu: python-sphinx: Update to 1.4.8.

[Marius Bakke]

[-- Attachment #1: Type: text/plain, Size: 704 bytes --]

Danny Milosavljevic <dannym@scratchpost.org> writes:

> * gnu/packages/python.scm (python-sphinx)[version]: Update to 1.4.8.
>   [source]: Use pypi-uri.
>   [propagated-inputs]: Add python-imagesize, python-sphinx-alabaster-theme,
>   python-babel, python-snowballstemmer, python-six.
>   [properties]: Add python2-variant.
> (python2-sphinx)[native-inputs]: Add python2-mock.
>   [propagated-inputs]: Add python2-pytz.

LGTM, thanks! As per the prior discussion, it should be applied in the
'python-tests' branch. Since it requires some packages only present in
'master', it will have to wait until the remaining failures are fixed.

Then we can merge master, add this patch and start a new evaluation.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

bug#25177: [PATCH v6] gnu: python-sphinx: Update to 1.4.8.

[Marius Bakke]

[-- Attachment #1: Type: text/plain, Size: 1754 bytes --]

Marius Bakke <mbakke@fastmail.com> writes:

> Danny Milosavljevic <dannym@scratchpost.org> writes:
>
>> * gnu/packages/python.scm (python-sphinx)[version]: Update to 1.4.8.
>>   [source]: Use pypi-uri.
>>   [propagated-inputs]: Add python-imagesize, python-sphinx-alabaster-theme,
>>   python-babel, python-snowballstemmer, python-six.
>>   [properties]: Add python2-variant.
>> (python2-sphinx)[native-inputs]: Add python2-mock.
>>   [propagated-inputs]: Add python2-pytz.
>
> LGTM, thanks! As per the prior discussion, it should be applied in the
> 'python-tests' branch. Since it requires some packages only present in
> 'master', it will have to wait until the remaining failures are fixed.
>
> Then we can merge master, add this patch and start a new evaluation.

Leo: I'm unable to reproduce the Hydra failures, so I decided to merge
master so we can restart this branch. However, Savannah refuses the push
for no good reason!

$ git reset --hard savannah/python-tests
$ git merge master
$ <fix two conflicts>
$ git push -v savannah python-tests
Pushing to mbakke@git.sv.gnu.org:/srv/git/guix.git
error: failed to push some refs to 'mbakke@git.sv.gnu.org:/srv/git/guix.git'

Any ideas? The merged branch is 687 commits ahead.

You can try the merge yourself, the local.mk conflict is easy and in
bioinformatics.scm you want to keep [arguments] and not [native-inputs].

I suppose we could rebase it and force-push, but it seems heavy-handed.

Danny: I cannot apply the sphinx update patch for some reason. Did you
use git-send-email? In any case, since you will have commit access soon,
can you please push it to the 'python-tests' branch once this merge is
resolved. Then we can start a new evaluation and hopefully do the last
round of fixes :-)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

bug#25177: [PATCH v6] gnu: python-sphinx: Update to 1.4.8.

[Danny Milosavljevic]

Hi,

> Danny: I cannot apply the sphinx update patch for some reason. Did you
> use git-send-email? 

Yes. I also pulled from guix master and did "git am" with my patch-E-Mail from back then a moment ago - it applied fine.

>In any case, since you will have commit access soon,
> can you please push it to the 'python-tests' branch once this merge is
> resolved. Then we can start a new evaluation and hopefully do the last
> round of fixes :-)

Sure.

bug#25177: [PATCH v6] gnu: python-sphinx: Update to 1.4.8.

[Leo Famulari]

[-- Attachment #1: Type: text/plain, Size: 1127 bytes --]

On Fri, Jan 13, 2017 at 01:34:21PM +0100, Marius Bakke wrote:
> Leo: I'm unable to reproduce the Hydra failures, so I decided to merge
> master so we can restart this branch. However, Savannah refuses the push
> for no good reason!
> 
> $ git reset --hard savannah/python-tests
> $ git merge master
> $ <fix two conflicts>
> $ git push -v savannah python-tests
> Pushing to mbakke@git.sv.gnu.org:/srv/git/guix.git
> error: failed to push some refs to 'mbakke@git.sv.gnu.org:/srv/git/guix.git'

I bet that you are using the new pre-push hook that verifies commit
signatures, and you're trying to push some commits that fail the
signature verification check.

Someone should add some error reporting to the hook.

> Any ideas? The merged branch is 687 commits ahead.
> 
> You can try the merge yourself, the local.mk conflict is easy and in
> bioinformatics.scm you want to keep [arguments] and not [native-inputs].

Done with the hook disabled.

> I suppose we could rebase it and force-push, but it seems heavy-handed.

IMO we should avoid it when possible; rebasing loses the original
signatures.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

bug#25177: [PATCH v6] gnu: python-sphinx: Update to 1.4.8.

[Danny Milosavljevic]

Done on the python-tests branch as commit 9a8acd00da3ad44ac6ef423f3d9cba87645cc022.

Matplotlib will fail now.

bug#25177: [PATCH v6] gnu: python-sphinx: Update to 1.4.8.

[Danny Milosavljevic]

Hi,

I've fixed python-matplotlib locally. It builds fine now.

The fix itself would be:

+                 (substitute* "users/intro.rst"
+                   ;; Fix reST markup error (see <https://github.com/sphinx-doc/sphinx/issues/3044>)
+                   (("[[][*][]]") "[#]"))

However, I've also changed alist-cons* phase construction to modify-phases. That would make the patch look real scary.

So here is a weird patch created via "git diff -b" where you can actually see what's happening without all the noise caused by indentation changes:

diff --git a/gnu/packages/python.scm b/gnu/packages/python.scm
index c540d7b5a..a5220e05e 100644
--- a/gnu/packages/python.scm
+++ b/gnu/packages/python.scm
@@ -3723,8 +3723,8 @@ transcendental functions).")
        ("texinfo" ,texinfo)))
     (arguments
      `(#:phases
-       (alist-cons-before
-        'build 'configure-environment
+       (modify-phases %standard-phases
+         (add-before 'build 'configure-environment
            (lambda* (#:key outputs inputs #:allow-other-keys)
              (let ((cairo (assoc-ref inputs "cairo"))
                    (gtk+ (assoc-ref inputs "gtk+")))
@@ -3740,9 +3740,8 @@ basedirlist = ~a,~a~%
  [rc_options]~%
 backend = TkAgg~%"
                            (assoc-ref inputs "tcl")
-                        (assoc-ref inputs "tk"))))))
-        (alist-cons-after
-         'install 'install-doc
+                           (assoc-ref inputs "tk")))))))
+         (add-after 'install 'install-doc
            (lambda* (#:key inputs outputs #:allow-other-keys)
              (let* ((data (string-append (assoc-ref outputs "doc") "/share"))
                     (doc (string-append data "/doc/" ,name "-" ,version))
@@ -3756,6 +3755,9 @@ backend = TkAgg~%"
                  (substitute* (find-files "." "conf\\.py")
                    (("latex_paper_size = 'letter'")
                     "latex_paper_size = 'a4'"))
+                 (substitute* "users/intro.rst"
+                   ;; Fix reST markup error (see <https://github.com/sphinx-doc/sphinx/issues/3044>)
+                   (("[[][*][]]") "[#]"))
                  (mkdir-p html)
                  (mkdir-p info)
                  ;; The doc recommends to run the 'html' target twice.
@@ -3777,8 +3779,7 @@ backend = TkAgg~%"
                  (copy-file "build/texinfo/matplotlib.info"
                             (string-append info "/matplotlib.info"))
                  (copy-file "build/latex/Matplotlib.pdf"
-                          (string-append doc "/Matplotlib.pdf")))))
-        %standard-phases))))
+                            (string-append doc "/Matplotlib.pdf")))))))))
     (home-page "http://matplotlib.org")
     (synopsis "2D plotting library for Python")
     (description

Should I split this into two commits - one for modify-phases and one for the actual change? Should I post them to the list? If so, should I mark that it's for python-tests somehow?

bug#25177: [PATCH v6] gnu: python-sphinx: Update to 1.4.8.

[Marius Bakke]

[-- Attachment #1: Type: text/plain, Size: 3619 bytes --]

Danny Milosavljevic <dannym@scratchpost.org> writes:

> Hi,
>
> I've fixed python-matplotlib locally. It builds fine now.
>
> The fix itself would be:
>
> +                 (substitute* "users/intro.rst"
> +                   ;; Fix reST markup error (see <https://github.com/sphinx-doc/sphinx/issues/3044>)
> +                   (("[[][*][]]") "[#]"))
>
> However, I've also changed alist-cons* phase construction to modify-phases. That would make the patch look real scary.
>
> So here is a weird patch created via "git diff -b" where you can actually see what's happening without all the noise caused by indentation changes:
>
> diff --git a/gnu/packages/python.scm b/gnu/packages/python.scm
> index c540d7b5a..a5220e05e 100644
> --- a/gnu/packages/python.scm
> +++ b/gnu/packages/python.scm
> @@ -3723,8 +3723,8 @@ transcendental functions).")
>         ("texinfo" ,texinfo)))
>      (arguments
>       `(#:phases
> -       (alist-cons-before
> -        'build 'configure-environment
> +       (modify-phases %standard-phases
> +         (add-before 'build 'configure-environment
>             (lambda* (#:key outputs inputs #:allow-other-keys)
>               (let ((cairo (assoc-ref inputs "cairo"))
>                     (gtk+ (assoc-ref inputs "gtk+")))
> @@ -3740,9 +3740,8 @@ basedirlist = ~a,~a~%
>   [rc_options]~%
>  backend = TkAgg~%"
>                             (assoc-ref inputs "tcl")
> -                        (assoc-ref inputs "tk"))))))
> -        (alist-cons-after
> -         'install 'install-doc
> +                           (assoc-ref inputs "tk")))))))
> +         (add-after 'install 'install-doc
>             (lambda* (#:key inputs outputs #:allow-other-keys)
>               (let* ((data (string-append (assoc-ref outputs "doc") "/share"))
>                      (doc (string-append data "/doc/" ,name "-" ,version))
> @@ -3756,6 +3755,9 @@ backend = TkAgg~%"
>                   (substitute* (find-files "." "conf\\.py")
>                     (("latex_paper_size = 'letter'")
>                      "latex_paper_size = 'a4'"))
> +                 (substitute* "users/intro.rst"
> +                   ;; Fix reST markup error (see <https://github.com/sphinx-doc/sphinx/issues/3044>)
> +                   (("[[][*][]]") "[#]"))
>                   (mkdir-p html)
>                   (mkdir-p info)
>                   ;; The doc recommends to run the 'html' target twice.
> @@ -3777,8 +3779,7 @@ backend = TkAgg~%"
>                   (copy-file "build/texinfo/matplotlib.info"
>                              (string-append info "/matplotlib.info"))
>                   (copy-file "build/latex/Matplotlib.pdf"
> -                          (string-append doc "/Matplotlib.pdf")))))
> -        %standard-phases))))
> +                            (string-append doc "/Matplotlib.pdf")))))))))
>      (home-page "http://matplotlib.org")
>      (synopsis "2D plotting library for Python")
>      (description
>
> Should I split this into two commits - one for modify-phases and one
> for the actual change?

Yes, please do! That's what we've done so far, try to 'git grep' for
modify-phases.

> Should I post them to the list? If so, should I mark that it's for
> python-tests somehow?

That's up to you. Typically cosmetic commits like changing to use
modify-phases are OK to just push, assuming it is followed up with a
more "meaty" commit with actual changes. Otherwise it will just cause
pointless rebuilds.

When you do change it to use modify-phases, please also check that
phases end on a #t. It should be added explicitly if the ending
statement has an unspecified return value :-)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

bug#25177: [PATCH v6] gnu: python-sphinx: Update to 1.4.8.

[Marius Bakke]

[-- Attachment #1: Type: text/plain, Size: 1293 bytes --]

Leo Famulari <leo@famulari.name> writes:

> On Fri, Jan 13, 2017 at 01:34:21PM +0100, Marius Bakke wrote:
>> Leo: I'm unable to reproduce the Hydra failures, so I decided to merge
>> master so we can restart this branch. However, Savannah refuses the push
>> for no good reason!
>> 
>> $ git reset --hard savannah/python-tests
>> $ git merge master
>> $ <fix two conflicts>
>> $ git push -v savannah python-tests
>> Pushing to mbakke@git.sv.gnu.org:/srv/git/guix.git
>> error: failed to push some refs to 'mbakke@git.sv.gnu.org:/srv/git/guix.git'
>
> I bet that you are using the new pre-push hook that verifies commit
> signatures, and you're trying to push some commits that fail the
> signature verification check.
>
> Someone should add some error reporting to the hook.
>
>> Any ideas? The merged branch is 687 commits ahead.
>> 
>> You can try the merge yourself, the local.mk conflict is easy and in
>> bioinformatics.scm you want to keep [arguments] and not [native-inputs].
>
> Done with the hook disabled.

Thank you for this! You are of course completely right, I had forgotten
about the hook already. I'll see if I can make it fail in a less
misleading way :-)

Can you start a new evaluation on Hydra or Cuirass, once the matplotlib
commit is in?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

bug#25177: [PATCH v6] gnu: python-sphinx: Update to 1.4.8.

[Danny Milosavljevic]

Pushed it as 2 commits to python-tests.

bug#25177: [PATCH v6] gnu: python-sphinx: Update to 1.4.8.

[Leo Famulari]

[-- Attachment #1: Type: text/plain, Size: 339 bytes --]

On Sat, Jan 14, 2017 at 04:35:24PM +0100, Marius Bakke wrote:
> Leo Famulari <leo@famulari.name> writes:
> Can you start a new evaluation on Hydra or Cuirass, once the matplotlib
> commit is in?

Sure, but let's wait for more progress on the gnome-updates evaluation.

I'll try to remember to check tomorrow, but feel free to remind me :)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

bug#25177: [PATCH v6] gnu: python-sphinx: Update to 1.4.8.

[Leo Famulari]

[-- Attachment #1: Type: text/plain, Size: 201 bytes --]

On Sat, Jan 14, 2017 at 04:35:24PM +0100, Marius Bakke wrote:
> Can you start a new evaluation on Hydra or Cuirass, once the matplotlib
> commit is in?

It's running:
https://hydra.gnu.org/eval/109444

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

pre-push signature hook error reporting [was Re: [PATCH v6] gnu: python-sphinx: Update to 1.4.8.]

[Leo Famulari]

[-- Attachment #1: Type: text/plain, Size: 1061 bytes --]

On Fri, Jan 13, 2017 at 10:24:00AM -0500, Leo Famulari wrote:
> I bet that you are using the new pre-push hook that verifies commit
> signatures, and you're trying to push some commits that fail the
> signature verification check.
> 
> Someone should add some error reporting to the hook.

In Git 2.11.0, it seems that `git verify-commit` can't tell the user
which commits failed verification:

https://git.kernel.org/cgit/git/git.git/tree/builtin/verify-commit.c?h=v2.11.0

With a warm cache and all the public keys on my machine, checking the
signature of all 17813 commits on the master branch takes ~40 seconds
with `git verify-commit $(git rev-list HEAD)`. This is what the pre-push
hook does now.

Checking the commits one at a time takes ~105 seconds, using something
like this:

for commit in $(git rev-list HEAD); do
	if ! git verify-commit $commit; then
		echo $commit
	fi
done

We could make the hook do something like that. Thoughts? I think the
performance regression is worth the convenience of knowing why it
failed.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: pre-push signature hook error reporting [was Re: [PATCH v6] gnu: python-sphinx: Update to 1.4.8.]

[Danny Milosavljevic]

Hi Leo,

On Mon, 16 Jan 2017 22:14:14 -0500
Leo Famulari <leo@famulari.name> wrote:

> In Git 2.11.0, it seems that `git verify-commit` can't tell the user
> which commits failed verification:
> 
> https://git.kernel.org/cgit/git/git.git/tree/builtin/verify-commit.c?h=v2.11.0

We should report that upstream and add the one line that does tell the user which commits failed verification upstream (for example print argv[i-1] in line 92). 

> With a warm cache and all the public keys on my machine, checking the
> signature of all 17813 commits on the master branch takes ~40 seconds
...
> Checking the commits one at a time takes ~105 seconds, using something
> like this:
> 
> for commit in $(git rev-list HEAD); do

For minimal improvement (I don't even think it's measureable), try `git rev-list HEAD` (backquotes) - it prevents having to spawn a subshell.

> We could make the hook do something like that. Thoughts? I think the
> performance regression is worth the convenience of knowing why it
> failed.

Uhhh it's already very slow... so even slower doesn't matter anymore (HIG guideline maximum duration is 2 seconds, so we are way off anyhow).

So I'd say do it your way for now and report it upstream for the future.

Depending on whether we think it will fail more often than not we could also combine it: 
- first check the fast (40 s) path
- if it fails,
  - print "Signature could not be verified to be correct. We are checking which failed..." info message
  - check the slow (105 s) path

Do we think that failures are likely?

Also, git seems to invoke the gpg executable for each and every commit. It would be interesting whether gpg-interface.c verify_signed_buffer could be adapted to either invoke gpg once or to just use a library instead (gpgme ?). 

Long term we could also cache the checking result - I think that's something more difficult in the face of keys that expire. It would have to store at least the expiration date, the public key and the list of commit hashes that were checked and validated successfully.

Re: pre-push signature hook error reporting [was Re: [PATCH v6] gnu: python-sphinx: Update to 1.4.8.]

[Hartmut Goebel]

[-- Attachment #1: Type: text/plain, Size: 508 bytes --]

Am 17.01.2017 um 12:34 schrieb Danny Milosavljevic:
> For minimal improvement (I don't even think it's measureable), try `git rev-list HEAD` (backquotes) - it prevents having to spawn a subshell.

Huh? I doubt this. The bash manual, section "Command Substitution" does
not distinguish between these both, as far as I understand it.

-- 
Regards
Hartmut Goebel

| Hartmut Goebel          | h.goebel@crazy-compilers.com               |
| www.crazy-compilers.com | compilers which you thought are impossible |


[-- Attachment #2: 0xBF773B65.asc --]
[-- Type: application/pgp-keys, Size: 14855 bytes --]

Re: pre-push signature hook error reporting [was Re: [PATCH v6] gnu: python-sphinx: Update to 1.4.8.]

[Hartmut Goebel]

Am 17.01.2017 um 04:14 schrieb Leo Famulari:
> with `git verify-commit $(git rev-list HEAD)`. This is what the pre-push

As far as I understand the example hook, it would be possible to do
something like

git verify-commit $(git rev-list $remote_sha..$local_sha)

this would only verify the commits which are going to be pushed.


-- 
Regards
Hartmut Goebel

| Hartmut Goebel          | h.goebel@crazy-compilers.com               |
| www.crazy-compilers.com | compilers which you thought are impossible |

Re: pre-push signature hook error reporting [was Re: [PATCH v6] gnu: python-sphinx: Update to 1.4.8.]

[Leo Famulari]

On Tue, Jan 17, 2017 at 12:34:28PM +0100, Danny Milosavljevic wrote:
> Leo Famulari <leo@famulari.name> wrote:
> > In Git 2.11.0, it seems that `git verify-commit` can't tell the user
> > which commits failed verification:
> 
> We should report that upstream and add the one line that does tell the
> user which commits failed verification upstream (for example print
> argv[i-1] in line 92). 

Well, it does print the output of `gpg`, but parsing that is
error-prone. I'm sure Git would take a patch that did the right thing,
however.

> Uhhh it's already very slow... so even slower doesn't matter anymore
> (HIG guideline maximum duration is 2 seconds, so we are way off
> anyhow).

Do you notice it in practice? Or do you mean that 40 seconds is already
very slow?

Remember that typical usage does not involve checking every commit, but
only the handful of new commits; this is *much* faster. Checking all
commits is just the benchmark I chose before starting because I wanted
any performance difference to be starkly illustrated.

> Do we think that failures are likely?

Yes, we've seen *bad* signatures pushed to Savannah recently, so I think
it's important for everyone to check their commits before pushing.

> Also, git seems to invoke the gpg executable for each and every
> commit. It would be interesting whether gpg-interface.c
> verify_signed_buffer could be adapted to either invoke gpg once or to
> just use a library instead (gpgme ?). 

Indeed, that would be better.

> Long term we could also cache the checking result - I think that's
> something more difficult in the face of keys that expire. It would
> have to store at least the expiration date, the public key and the
> list of commit hashes that were checked and validated successfully.

Agreed. But this hook is only a convenience tool to prevent mistakes.
We need to revamp `guix pull` to handle this properly.

Re: pre-push signature hook error reporting [was Re: [PATCH v6] gnu: python-sphinx: Update to 1.4.8.]

[Leo Famulari]

On Tue, Jan 17, 2017 at 03:55:42PM +0100, Hartmut Goebel wrote:
> Am 17.01.2017 um 04:14 schrieb Leo Famulari:
> > with `git verify-commit $(git rev-list HEAD)`. This is what the pre-push
> 
> As far as I understand the example hook, it would be possible to do
> something like
> 
> git verify-commit $(git rev-list $remote_sha..$local_sha)
> 
> this would only verify the commits which are going to be pushed.

I'm sorry that I was unclear.

If you read the hook, you'll see that it does what you suggest. I chose
to check the entire history as a benchmark.

Re: pre-push signature hook error reporting [was Re: [PATCH v6] gnu: python-sphinx: Update to 1.4.8.]

[Leo Famulari]

On Tue, Jan 17, 2017 at 01:56:29PM +0100, Hartmut Goebel wrote:
> Am 17.01.2017 um 12:34 schrieb Danny Milosavljevic:
> > For minimal improvement (I don't even think it's measureable), try
> > `git rev-list HEAD` (backquotes) - it prevents having to spawn a
> > subshell.
> 
> Huh? I doubt this. The bash manual, section "Command Substitution" does
> not distinguish between these both, as far as I understand it.

The POSIX shell command language specification says that:

"The shell shall expand the command substitution by executing command in
a subshell environment (see Shell Execution Environment) and replacing
the command substitution (the text of command plus the enclosing "$()"
or backquotes) with the standard output of the command, removing
sequences of one or more <newline>s at the end of the substitution."

http://pubs.opengroup.org/onlinepubs/007904875/utilities/xcu_chap02.html#tag_02_06_03

Maybe it's faster, maybe not, but I think my benchmark was
misinterpreted...

Re: pre-push signature hook error reporting [was Re: [PATCH v6] gnu: python-sphinx: Update to 1.4.8.]

[Ludovic Courtès]

Leo Famulari <leo@famulari.name> skribis:

> On Fri, Jan 13, 2017 at 10:24:00AM -0500, Leo Famulari wrote:
>> I bet that you are using the new pre-push hook that verifies commit
>> signatures, and you're trying to push some commits that fail the
>> signature verification check.
>> 
>> Someone should add some error reporting to the hook.
>
> In Git 2.11.0, it seems that `git verify-commit` can't tell the user
> which commits failed verification:
>
> https://git.kernel.org/cgit/git/git.git/tree/builtin/verify-commit.c?h=v2.11.0
>
> With a warm cache and all the public keys on my machine, checking the
> signature of all 17813 commits on the master branch takes ~40 seconds
> with `git verify-commit $(git rev-list HEAD)`. This is what the pre-push
> hook does now.
>
> Checking the commits one at a time takes ~105 seconds, using something
> like this:
>
> for commit in $(git rev-list HEAD); do
> 	if ! git verify-commit $commit; then
> 		echo $commit
> 	fi
> done
>
> We could make the hook do something like that. Thoughts? I think the
> performance regression is worth the convenience of knowing why it
> failed.

For the pre-push hook, the overhead seems reasonable (perhaps we could
limit the range to commits after the first signed commit to avoid
looping for no reason?) and an improvement.

Eventually we could rewrite in Scheme using guile-git, which should be
faster (no need to fork that much).

Thanks!

Ludo’.

Re: pre-push signature hook error reporting [was Re: [PATCH v6] gnu: python-sphinx: Update to 1.4.8.]

[Leo Famulari]

[-- Attachment #1: Type: text/plain, Size: 1187 bytes --]

On Fri, Jan 20, 2017 at 03:05:42PM +0100, Ludovic Courtès wrote:
> For the pre-push hook, the overhead seems reasonable (perhaps we could
> limit the range to commits after the first signed commit to avoid
> looping for no reason?) and an improvement.

I agree that it's reasonable and an improvement for the common case of
pushing to existing branches; only the new commits' signatures are
verified in this case.

It's a good idea to limit the range when pushing new branches. It will
still fail invariably, but it will fail more quickly.

I believe the first signed commit is e3d0fcbf7e55 (gnu: Default to
GCC 5.).

Due to merges in the history (I think), using `git rev-list` to
enumerate the commits from e3d0fcbf7e55^..HEAD gives a list of commits
begins with aae03c484f21832 (gnu: Add singular.), which is an earlier
commit.

That's a little confusing, but maybe it doesn't matter if we are just
trying to save the user some time before it fails. They'll have to
disable the hook to push a branch anyways.

WDYT?

> Eventually we could rewrite in Scheme using guile-git, which should be
> faster (no need to fork that much).

Yes, that would be good!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: pre-push signature hook error reporting

[Leo Famulari]

[-- Attachment #1.1: Type: text/plain, Size: 478 bytes --]

On Fri, Jan 20, 2017 at 03:05:42PM +0100, Ludovic Courtès wrote:
> For the pre-push hook, the overhead seems reasonable (perhaps we could
> limit the range to commits after the first signed commit to avoid
> looping for no reason?) and an improvement.

Here is a patch for the hook that I've been using for the past couple weeks.

For the common use case of pushing new commits to an existing branch, I
don't notice the hook at all, except when it catches my mistakes.

[-- Attachment #1.2: 0001-etc-The-pre-push-hook-says-which-commits-failed-the-.patch --]
[-- Type: text/plain, Size: 1610 bytes --]

From 7d8206949f98a121bb2d50e0eecfcba1d9cce27a Mon Sep 17 00:00:00 2001
From: Leo Famulari <leo@famulari.name>
Date: Mon, 23 Jan 2017 00:57:46 -0500
Subject: [PATCH] etc: The pre-push hook says which commits failed the
 signature check.

* etc/git/pre-push: Check each commit's signature individually so that
we can report which commits fail the check.
---
 etc/git/pre-push | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/etc/git/pre-push b/etc/git/pre-push
index c894c5a9e..9206a2dfe 100755
--- a/etc/git/pre-push
+++ b/etc/git/pre-push
@@ -40,17 +40,29 @@ do
 	else
 		if [ "$remote_sha" = $z40 ]
 		then
-			# New branch, examine all commits
-			range="$local_sha"
+			# We are pushing a new branch. To prevent wasting too
+			# much time for this relatively rare case, we examine
+			# all commits since the first signed commit, rather than
+			# the full history. This check *will* fail, and the user
+			# will need to temporarily disable the hook to push the
+			# new branch.
+			range="e3d0fcbf7e55e8cbe8d0a1c5a24d73f341d7243b..$local_sha"
 		else
 			# Update to existing branch, examine new commits
 			range="$remote_sha..$local_sha"
 		fi
 
 		# Verify the signatures of all commits being pushed.
-		git verify-commit $(git rev-list $range) >/dev/null 2>&1
-
-		exit $?
+		ret=0
+		for commit in $(git rev-list $range)
+		do
+			if ! git verify-commit $commit >/dev/null 2>&1
+			then
+				printf "%s failed signature check\n" $commit
+				ret=1
+			fi
+		done
+		exit $ret
 	fi
 done
 
-- 
2.11.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: pre-push signature hook error reporting

[Marius Bakke]

[-- Attachment #1: Type: text/plain, Size: 602 bytes --]

Leo Famulari <leo@famulari.name> writes:

> On Fri, Jan 20, 2017 at 03:05:42PM +0100, Ludovic Courtès wrote:
>> For the pre-push hook, the overhead seems reasonable (perhaps we could
>> limit the range to commits after the first signed commit to avoid
>> looping for no reason?) and an improvement.
>
> Here is a patch for the hook that I've been using for the past couple weeks.
>
> For the common use case of pushing new commits to an existing branch, I
> don't notice the hook at all, except when it catches my mistakes.

Thanks a lot for this! I haven't tested it, but the code LGTM.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

Re: pre-push signature hook error reporting

[Ludovic Courtès]

Marius Bakke <mbakke@fastmail.com> skribis:

> Leo Famulari <leo@famulari.name> writes:
>
>> On Fri, Jan 20, 2017 at 03:05:42PM +0100, Ludovic Courtès wrote:
>>> For the pre-push hook, the overhead seems reasonable (perhaps we could
>>> limit the range to commits after the first signed commit to avoid
>>> looping for no reason?) and an improvement.
>>
>> Here is a patch for the hook that I've been using for the past couple weeks.
>>
>> For the common use case of pushing new commits to an existing branch, I
>> don't notice the hook at all, except when it catches my mistakes.
>
> Thanks a lot for this! I haven't tested it, but the code LGTM.

Ditto, thank you!

Ludo’.