Extend `/pgp` command to make key exchange procedure easier #1850

H3rnand3zzz · 2023-05-13T18:47:45Z

See commit messages

jubalh · 2023-05-15T12:36:31Z

Hi! Didn't have time to review yet, only skimmed it. But I would suggest (for the future if you don't want to do it for this PR) to split the commits.
I think this should actually be two or three commits :)

The reason is that it makes it easier to revert commits. Let's say for some (imagined) reason we would like to remove the pgp exchange procedure. Then we could just do git revert xxx. But if all three changes are in one commit we would also loose the typo fixes and the improvements not directly related to the exchange procedure. So that means we would have to revert by hand.

So it's good practise to have such things in individual commits. Even when it's unlikely that a revert will happen :)

src/command/cmd_defs.c

src/command/cmd_ac.c

jubalh · 2023-05-29T11:51:42Z

@sjaeckel ping

src/command/cmd_funcs.c

src/pgp/gpg.c

jubalh · 2023-06-29T11:32:23Z

@sjaeckel I'm uncertain if you plan to review again :)
Otherwise I would merge.

sjaeckel

I like the refactor of _cmd_set_boolean_preference() 👍

Would it maybe make sense to have a /pgp import command which lets you select one of the last messages in a private chat and then tries to import that? ... in case a user doesn't want to auto-import all public keys. But that'd be for another PR I guess.

src/command/cmd_funcs.c

src/pgp/gpg.c

H3rnand3zzz · 2023-07-01T08:03:39Z

@jubalh review suggestions are applied. After almost 2 month in waiting, maybe it's time to merge?

jubalh

Thanks for your patience!

So far I only looked at the code but now realize there could be a change in the commit messages as well.

In the first commit (Add /pgp sendpub command ) I'm missing a description what the command will do/why it is useful. And also with which other clients it is compatible (if it applies and you are aware of this info. I think you said it works with PSI).

Same for Add optional pgp public key autoimport.

I think it deserves a short description of what it will do/why its wanted.

After this I think good to go. Sorry again for the long waiting time!

sjaeckel

Sorry for the next bunch of change requests, but this PR is so huge, it's not easy to review. Also there are two sets of independent changes in there, which makes it even harder. That's really where the PR focused review approach falls short. If this were a commit based review approach it'd be cleaner, but it is as it is and therefore 🤷

Still I suggest to rebase on top of master and re-order the commits. Put "Refactor _cmd_set_boolean_preference function" as the first commit and create a separate branch & PR for that single commit, which can then be quickly merged, so the "real changes" of this feature become visible.

sjaeckel · 2023-07-02T09:39:40Z

src/pgp/gpg.c

@@ -381,6 +375,7 @@ p_gpg_list_keys(void)

    gpgme_release(ctx);

+    // TODO: move autocomplete in other place


Why? Where?

It might be considered unintended functionality, the function is a bit too bloated. It's more a design question and I left it for anyone who want to clean up the code even more in the future.

src/pgp/gpg.c

src/command/cmd_funcs.c

sjaeckel · 2023-07-02T10:30:32Z

Still I suggest to rebase on top of master and re-order the commits. Put "Refactor _cmd_set_boolean_preference function" as the first commit and create a separate branch & PR for that single commit, which can then be quickly merged, so the "real changes" of this feature become visible.

If you open a separate PR for those unrelated changes, maybe also include Cleanup char* to auto_gchar gchar* for prefs_get_string and Fix memory leak and cleanup of #1857

Command allows to share your PGP pub key with ease, it's not described in XEP-0027, but used in some clients, such as PSI, Pidgin. Fix typos Minor improvements

Improve documentation Update code formatting and indentation for better readability Show state if no argument provided Show if argument wasn't changed Reduce amount of arguments

Refactor `p_gpg_list_keys` Add `/pgp autoimport` command, it's not described in XEP-0027, but used in some clients, such as PSI, Pidgin. It will autoimport keys received with `/pgp sendpub`, in plain text as a message, or using features, provided in other clients. It doesn't autoassign them, but shows command to assign, letting user to decide. Improve documentation for some preexisting functions Add contact argument to `/pgp sendpub`

Use `_cmd_ac_complete_params` when fits, thus reducing plurality

jubalh

That's great descriptive commit messages now! Thanks :)

jubalh · 2023-07-03T12:18:54Z

Thanks for your contribution!

H3rnand3zzz · 2023-07-03T14:10:16Z

Thank you for reviews

H3rnand3zzz changed the title ~~Improve PGP~~ Extend /pgp command to make key exchange procedure easier May 13, 2023

H3rnand3zzz force-pushed the feature/pgp-improved branch from 6eca994 to ae77bb5 Compare May 15, 2023 09:54

H3rnand3zzz marked this pull request as ready for review May 16, 2023 00:46

jubalh assigned H3rnand3zzz May 16, 2023

jubalh added feature pgp labels May 16, 2023

jubalh added this to the next milestone May 16, 2023

jubalh requested review from jubalh and sjaeckel May 16, 2023 05:57

jubalh reviewed May 16, 2023

View reviewed changes

src/command/cmd_defs.c Show resolved Hide resolved

jubalh reviewed May 16, 2023

View reviewed changes

src/command/cmd_ac.c Outdated Show resolved Hide resolved

sjaeckel requested changes Jun 14, 2023

View reviewed changes

src/command/cmd_funcs.c Outdated Show resolved Hide resolved

src/pgp/gpg.c Outdated Show resolved Hide resolved

H3rnand3zzz force-pushed the feature/pgp-improved branch from 81f64f5 to 6ee9142 Compare June 15, 2023 08:49

H3rnand3zzz requested a review from sjaeckel June 15, 2023 08:52

H3rnand3zzz force-pushed the feature/pgp-improved branch from 6ee9142 to 8ae2963 Compare June 15, 2023 09:13

sjaeckel reviewed Jun 30, 2023

View reviewed changes

src/command/cmd_funcs.c Outdated Show resolved Hide resolved

src/command/cmd_funcs.c Outdated Show resolved Hide resolved

src/pgp/gpg.c Outdated Show resolved Hide resolved

src/pgp/gpg.c Outdated Show resolved Hide resolved

H3rnand3zzz force-pushed the feature/pgp-improved branch 2 times, most recently from 19b0e37 to 507936a Compare June 30, 2023 13:09

jubalh self-requested a review July 2, 2023 07:28

jubalh requested changes Jul 2, 2023

View reviewed changes

H3rnand3zzz force-pushed the feature/pgp-improved branch from 507936a to eeb0e57 Compare July 2, 2023 08:41

sjaeckel requested changes Jul 2, 2023

View reviewed changes

H3rnand3zzz force-pushed the feature/pgp-improved branch 2 times, most recently from 150bb8d to a0e0bde Compare July 2, 2023 12:21

Add /pgp sendpub command

a59623a

Command allows to share your PGP pub key with ease, it's not described in XEP-0027, but used in some clients, such as PSI, Pidgin. Fix typos Minor improvements

H3rnand3zzz added 3 commits July 2, 2023 14:25

Refactor _cmd_set_boolean_preference function

349e4cb

Improve documentation Update code formatting and indentation for better readability Show state if no argument provided Show if argument wasn't changed Reduce amount of arguments

Refactor autocomplete

7d6cbfd

Use `_cmd_ac_complete_params` when fits, thus reducing plurality

H3rnand3zzz force-pushed the feature/pgp-improved branch from a0e0bde to 7d6cbfd Compare July 2, 2023 12:26

jubalh self-requested a review July 3, 2023 08:43

jubalh approved these changes Jul 3, 2023

View reviewed changes

sjaeckel approved these changes Jul 3, 2023

View reviewed changes

jubalh merged commit f67d548 into profanity-im:master Jul 3, 2023
6 checks passed

H3rnand3zzz deleted the feature/pgp-improved branch August 22, 2023 11:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend `/pgp` command to make key exchange procedure easier #1850

Extend `/pgp` command to make key exchange procedure easier #1850

H3rnand3zzz commented May 13, 2023

jubalh commented May 15, 2023

jubalh commented May 29, 2023

jubalh commented Jun 29, 2023

sjaeckel left a comment

H3rnand3zzz commented Jul 1, 2023

jubalh left a comment

sjaeckel left a comment •

edited

Loading

sjaeckel Jul 2, 2023

H3rnand3zzz Jul 2, 2023 •

edited

Loading

sjaeckel commented Jul 2, 2023 •

edited

Loading

jubalh left a comment

jubalh commented Jul 3, 2023

H3rnand3zzz commented Jul 3, 2023

		@@ -381,6 +375,7 @@ p_gpg_list_keys(void)

		gpgme_release(ctx);

		// TODO: move autocomplete in other place

Extend /pgp command to make key exchange procedure easier #1850

Extend /pgp command to make key exchange procedure easier #1850

Conversation

H3rnand3zzz commented May 13, 2023

jubalh commented May 15, 2023

jubalh commented May 29, 2023

jubalh commented Jun 29, 2023

sjaeckel left a comment

Choose a reason for hiding this comment

H3rnand3zzz commented Jul 1, 2023

jubalh left a comment

Choose a reason for hiding this comment

sjaeckel left a comment • edited Loading

Choose a reason for hiding this comment

sjaeckel Jul 2, 2023

Choose a reason for hiding this comment

H3rnand3zzz Jul 2, 2023 • edited Loading

Choose a reason for hiding this comment

sjaeckel commented Jul 2, 2023 • edited Loading

jubalh left a comment

Choose a reason for hiding this comment

jubalh commented Jul 3, 2023

H3rnand3zzz commented Jul 3, 2023

Extend `/pgp` command to make key exchange procedure easier #1850

Extend `/pgp` command to make key exchange procedure easier #1850

sjaeckel left a comment •

edited

Loading

H3rnand3zzz Jul 2, 2023 •

edited

Loading

sjaeckel commented Jul 2, 2023 •

edited

Loading