pagure

#933 #879 follow-up: strip GPG signatures from tag messages at releases page
Closed by pingou. Opened by jpokorny.
jpokorny/pagure releases-strip-gpg-sigs  into  master

Download 933.patch

untested, mainly to demonstrate behaviour I'd like to see in pagure.

Note there is more to fix/enhance, just for posterity:

  • repeated tagid id, previously it used to be class
  • there should be anchors on per-release basis, if not
    single-release pages (preferred)

Hm, help me to understand the changes, if I understand correctly, all this does is putting the gpg signature from the tag into a <div style="white-space:pre-wrap">, is that correct?

Have you thought on using a <pre> instead?

Also, looking at the page as it is now: https://pagure.io/pagure/releases I am not seeing much difference, do you have an example where your changes have a bigger impact?

Nope, the intent is to drop the signature completely for the time being.
Currently the GPG information is useless (may be used later as a matter of dealing with issue #885).
At least my perception is that those lines are annoying in the current state.
See also https://pagure.io/clufter/releases.

The PR also fixes unnecessary inefficiency and superfluous empty lines.

Ah, in this case I propose using:

diff --git a/ pagure/templates/releases.html b/ pagure/templates/releases.html
index 2c68876..4f82bbe 100644
--- a/ pagure/templates/releases.html   
+++ b/ pagure/templates/releases.html   
@@ -42,16 +42,13 @@
         {% if tag['objecttype'] == "tag" %}
           {% if tag['object'].message %}
             <strong>{{tag['tagname']}}</strong>
-            {% set msg_head_tail = tag['object'].message.split('\n', 1) %}
-            {{ msg_head_tail[0] }}
-            {% set detailtext = msg_head_tail.pop().strip() %}
-            {% if msg_head_tail and detailtext.endswith('\n-----END PGP SIGNATURE-----') %}
-              {% set detailtext = detailtext.rsplit('\n-----BEGIN PGP SIGNATURE-----\n', 1)[0] %}
-              {% if detailtext %}
+            {{ tag['object'].message.split('\n')[0] }}
+            {% set detailtext = tag['object'].message.partition('\n')[2] %}
+            {% set detailtext = detailtext.partition('-----BEGIN PGP SIGNATURE-----')[0] %}
+            {% if detailtext %}
               <div style="white-space:pre-wrap">
                 {{ detailtext }}
               </div>
-              {% endif %}
             {% endif %}
           {% endif %}
         {% else %}

(I can send you a formal patch if you like)

That doesn't deal with case of legitimate use of -----BEGIN PGP SIGNATURE----- in the commit message. And please, avoid using unbounded split when you really just want to cut once (that's one part of that inefficiency I am talking about).

avoid using unbounded split when you really just want to cut once

That's an easy one to fix

That doesn't deal with case of legitimate use of -----BEGIN PGP SIGNATURE----- in the commit message.

Ok I think I see what you mean, but I had to add a .strip() as otherwise the endswith() approach you took wasn't working for me.

The changes look like this then:

diff --git a/ pagure/templates/releases.html b/ pagure/templates/releases.html
index 2c68876..c52352a 100644
--- a/ pagure/templates/releases.html   
+++ b/ pagure/templates/releases.html   
@@ -42,16 +42,15 @@
         {% if tag['objecttype'] == "tag" %}
           {% if tag['object'].message %}
             <strong>{{tag['tagname']}}</strong>
-            {% set msg_head_tail = tag['object'].message.split('\n', 1) %}
-            {{ msg_head_tail[0] }}
-            {% set detailtext = msg_head_tail.pop().strip() %}
-            {% if msg_head_tail and detailtext.endswith('\n-----END PGP SIGNATURE-----') %}
-              {% set detailtext = detailtext.rsplit('\n-----BEGIN PGP SIGNATURE-----\n', 1)[0] %}
-              {% if detailtext %}
+            {% set headtext, _, detailtext = tag['object'].message.partition('\n') %}
+            {{ headtext }}
+            {% if detailtext.strip().endswith('-----END PGP SIGNATURE-----') %}
+            {% set detailtext = detailtext.partition('-----BEGIN PGP SIGNATURE-----')[0] %}
+            {% endif %}
+            {% if detailtext.strip() %}
               <div style="white-space:pre-wrap">
                 {{ detailtext }}
               </div>
-              {% endif %}
             {% endif %}
           {% endif %}
         {% else %}

@jpokorny shall I close this PR and open a new one, or do you want a patch to add to your branch to update this PR?

Or do you have more remarks about the change I proposed? :)

To be honest, I haven't heard what's wrong with my original approach so far :)

It didn't work in pagure's release page, at least one .strip() is missing :)

Oh, I expected that set detailtext = msg_head_tail.pop().strip() covers that.
I work on blind-patching basis now.

While I tested it on a local instance :)

(which is why I asked for more info about the PR, since locally it wasn't really changing much)
Edited by pingou

Is this better then?

diff --git a/pagure/templates/releases.html b/pagure/templates/releases.html
index 2c68876..e269ac4 100644
--- a/pagure/templates/releases.html
+++ b/pagure/templates/releases.html
@@ -46,8 +46,8 @@
             {{ msg_head_tail[0] }}
             {% set detailtext = msg_head_tail.pop().strip() %}
             {% if msg_head_tail and detailtext.endswith('\n-----END PGP SIGNATURE-----') %}
-              {% set detailtext = detailtext.rsplit('\n-----BEGIN PGP SIGNATURE-----\n', 1)[0] %}
-              {% if detailtext %}
+              {% set detailtext = detailtext.rsplit('-----BEGIN PGP SIGNATURE-----\n', 1)[0] %}
+              {% if detailtext.strip() %}
               <div style="white-space:pre-wrap">
                 {{ detailtext }}
               </div>

It won't trigger on detailtext.endswith(...).

Also, don't you like the .partition() approach where all the splitting it done in one go w/o further need to use [0] or .pop()?

Oh, I see, it would trigger on detailtext.endswith(...) you're right (you removed the \n).

I still find the use of partition() here clearer though :)

Could you also rebase your branch after updating the patch? I try to keep the history linear :)

ping?

@jpokorny so what should we do about this PR?

I'd like to cut a release by tomorrow when freeze is over and would be nice to have this in then :)

@jpokorny did you see my previous messages?

Ok, I restarted looking at the PR and it's still doesn't work but it's odd, to work it requires:

-            {% set detailtext = msg_head_tail.pop().strip() %}
+            {% set detailtext = msg_head_tail.pop() %}

Otherwise, if we keep .strip() it looks like it needs this change:

-            {% if msg_head_tail and detailtext.endswith('\n-----END PGP SIGNATURE-----') %}
+            {% if msg_head_tail and detailtext.endswith('\n-----END PGP SIGNATURE-----\n') %}

I'll go with the first change and open a new PR :)

https://pagure.io/pagure/pull-request/967 fixed this :)

Pull-Request has been closed by pingou

Thanks, I really appreciate the form we've ended up with :)
And sorry regarding my responsiveness, was burning cycles at what's deemed more hot (yeah, and there's a bit of a barrier that I didn't get to set a custom instance up for immediate feedback on the code change).

Can't wait for the clean release page.
Metadata
None
No Tags
Static archive. pagure.io was sunset in 2026; this is a read-only snapshot.
Red Hat Logo

Fedora is sponsored by Red Hat. Learn more about the relationship between Red Hat and Fedora »

© Red Hat, Inc. and others.