Skip to content

Deprecates git replies in favor of NIP-22#1744

Merged
fiatjaf merged 3 commits into
nostr-protocol:masterfrom
vitorpamplona:kills-git-reply
Mar 26, 2025
Merged

Deprecates git replies in favor of NIP-22#1744
fiatjaf merged 3 commits into
nostr-protocol:masterfrom
vitorpamplona:kills-git-reply

Conversation

@vitorpamplona

Copy link
Copy Markdown
Collaborator

No description provided.

@fiatjaf

fiatjaf commented Feb 5, 2025

Copy link
Copy Markdown
Member

@DanConwayDev

@dluvian

dluvian commented Feb 20, 2025

Copy link
Copy Markdown
Contributor

I agree.
I use nip22 in Gitplaza and Gitworkshop uses kind 1 but it's a key priority to switch to nip22: https://gitworkshop.dev/r/naddr1qvzqqqrhnypzpgqgmmc409hm4xsdd74sf68a2uyf9pwel4g9mfdg8l5244t6x4jdqy2hwumn8ghj7un9d3shjtnyv9kh2uewd9hj7qqtva5hgam0wf4hx6r0wq2r9ayq/issues/note1smqtj4vfkqt0ldcrhlqgp4y72sgxua8z66pjj5gecdznuj2dhehscse0lt

Does anyone even use this GitReply event?

@cypherhoodlum

cypherhoodlum commented Feb 20, 2025

Copy link
Copy Markdown
Contributor

Does anyone even use this GitReply event?

I'm literally testing the NIP-34 replies right now on the Nestr client but our working implementation is handled very differently (using Martti Malmi's irisdb-hooks). I thought I'd test if NIP-34 could work for us too but if this is going to get deprecated then I won't bother testing it. The main reason we'd switch is to be more interoperable. We also have pull requests which is not specified in NIP-34 at all. PRs have almost identical structure to issues but should have a different kind number imo.

@cypherhoodlum

Copy link
Copy Markdown
Contributor

The only problem I see with NIP-22 in this use case is that markdown is the standard format of git replies and that is not allowed in kind 1111 event .content.

@vitorpamplona

Copy link
Copy Markdown
Collaborator Author

On that point, I think content restrictions on NIP-22 will have to be per scope. The comment kind must always follow the formatting requirements of the scope they are being applied to.

@vitorpamplona

Copy link
Copy Markdown
Collaborator Author

Check this out: #1797

@AsaiToshiya

Copy link
Copy Markdown
Collaborator

Check this out: #1797

LGTM.

@DanConwayDev

DanConwayDev commented Feb 20, 2025 via email

Copy link
Copy Markdown
Contributor

@dluvian

dluvian commented Feb 20, 2025

Copy link
Copy Markdown
Contributor

We should deprecate nip10 all throughout. Use e for parent and E for root instead of these marker e tags which make lazy loading impossible

@DanConwayDev

DanConwayDev commented Feb 20, 2025 via email

Copy link
Copy Markdown
Contributor

@mikedilger

Copy link
Copy Markdown
Contributor

I say we keep git replies since they are markdown and nip22 comments are not. BUT maybe the tags could be more like nip22, I dunno.

@dluvian

dluvian commented Feb 21, 2025

Copy link
Copy Markdown
Contributor

We're not keeping git replies. No one uses them. I say we use nip22 and don't stress about markdown.

@cypherhoodlum

Copy link
Copy Markdown
Contributor

You can see how the ngit git remote helper is intended to be used with this [PR] workflow

Wow that is not accessible at all and doesn't leverage Nostr. Working on fixing this 👍

@cypherhoodlum

Copy link
Copy Markdown
Contributor

I say we use nip22 and don't stress about markdown.

I agree. Kind 1 clients can choose to display or hide events based on the NIP-22 tags. If a client doesn't support displaying the root event (context for the comment) then it's trivial to hide the comment entirely to avoid contextless "spam" which probably contains formats like markdown in the .content that the client is not expecting.

@DanConwayDev

DanConwayDev commented Feb 21, 2025

Copy link
Copy Markdown
Contributor

Wow that is not accessible at all and doesn't leverage Nostr. Working on fixing this 👍

Not sure what you mean? the PR is entirely on nostr in patch event(s).

@cypherhoodlum

Copy link
Copy Markdown
Contributor

I'm sure I'm missing something then, ignore me. We'll be doing PRs very differently on our project anyway to keep the github-like UX. No patches.

@DanConwayDev

Copy link
Copy Markdown
Contributor

ACK

nip10 threading applies throughout nip34 and not just to the reply kind. I'll review this PR tomorrow including the impact on the rest of nip34.

I've reviewed the impact of nip22 on the rest of nip34 and concluded that its best to use nip22 comments here but not to apply the threading style to patch and status events as its a breaking change. I came to this conclusion by drafting the changes that would be needed - #1799 includes this if you are interested.

@vitorpamplona

Copy link
Copy Markdown
Collaborator Author

Replaced by #1799

@vitorpamplona

Copy link
Copy Markdown
Collaborator Author

@DanConwayDev I am confused about this statement. Are you saying we should merge this or #1799?

@DanConwayDev

Copy link
Copy Markdown
Contributor

I'm saying we should merge this and not #1799 so we minimised breaking changes.

I've implemented nip22 for gitworkshop.dev to reflect this PR (see git issue on gitworkshop). Are there any clients left using kind 1 or the legacy kind for git replies? Has amethyst updated?

Let's go ahead and merge this and discuss if we need furture changed in the #1799 thread.

@vitorpamplona

Copy link
Copy Markdown
Collaborator Author

Nice, I will switch on Amethyst too

@staab

staab commented Feb 21, 2025

Copy link
Copy Markdown
Member

Either PR seems fine to me, but regarding plaintext I've drafted #1800. Hopefully this can thread the needle between rich text and plain text.

@DanConwayDev

Copy link
Copy Markdown
Contributor

can we merge this?

@fiatjaf fiatjaf merged commit 86fc82c into nostr-protocol:master Mar 26, 2025
RandyMcMillan pushed a commit to gnostr-org/gnostr-nips that referenced this pull request Apr 5, 2025
pats2sats pushed a commit to sudonym-btc/marketplace-listing-nip that referenced this pull request May 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants