Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NIP-34: git stuff #997

Merged
merged 16 commits into from
Mar 5, 2024
Merged

NIP-34: git stuff #997

merged 16 commits into from
Mar 5, 2024

Conversation

fiatjaf
Copy link
Member

@fiatjaf fiatjaf commented Jan 21, 2024

The simplest possible git collaboration flow that has a chance of working.

The patch stuff is based on http://git.jb55.com/git-nostr-tools, with some changes.

https://github.com/nostr-protocol/nips/blob/git/34.md

Here's a standalone CLI tool that allows for announcing a repository, sending and downloading patches: https://github.com/fiatjaf/gitstr -- probably very rough still, but I just merged a patch from myself using it so I am happy.

@jb55
Copy link
Contributor

jb55 commented Jan 22, 2024

awesome, this is very close to what I was converging on.

@mikedilger
Copy link
Contributor

When I looked into this topic, I discovered https://git-scm.com/book/en/v2/Git-on-the-Server-Smart-HTTP
where you can avoid SSH key management and use HTTP digest passwords for pushing (on linux git comes with a CGI server in /usr/bin/git-core/git-http-backend). I thought that could work with NIP-98.

Copy link
Member

@Giszmo Giszmo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With a lot of stuff being optional in this nip already I took a shot at more stuff we'd definitely need long term but not urgently on the first shot.

"kind": 30617,
"content": "",
"tags": [
["d", "<repo-id>"],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A word on this, please. I can't find a repo-id definition

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's just an arbitrary string, as in all other d tags.

Copy link
Member

@Giszmo Giszmo Jan 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it make sense to prescribe this? If we used the initial commit hash for example, finding/detecting forks would be easier. fiatjaf:30617:helloWorld might be a very different repo than giszmo:30617:helloWorld but giszmo:30617:<sha256-of-bitcoin-root-commit> ... would hopefully match between all forks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this idea a lot - something github doesn't do and we could - that allows people to find all the forks. But this particular event contains data that would differ between forks.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it too, but how can you be sure? Maybe we can have another single-letter tag for this that only interested implementations would include, to prevent others from having to check invalid data.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some OSS products which fork has become the official developer because the original author retired the development.

https://github.com/ctrlpvim/ctrlp.vim

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not make it the root commit id? For forks it could be the id of the earliest unique commit. The repository COULD contain a config file which specifies this id so that clients MAY access the config to automatically identify the correct id.

34.md Outdated
Comment on lines 48 to 51
## Branch merge

To be defined.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would remove these lines and add some ## Outlook at the bottom to mention what else this nip could cover. Furthermore are you thinking of a "merge request" event here? As the different clone urls in the original repo might use different branch names, the target needs one specific url to reference the branch to merge into. As branches can be renamed, these events have no absolute cryptographic reference unless we add one via the branches heads.

MRs call for code reviews ...

Suggested change
## Branch merge
To be defined.
## Merge Request
Notify repository owner of an improvement under development.
```jsonc
{
"kind": 31617,
"content": "<Description using markdown>",
"tags": [
["d", "<something unique / UUID>"],
["a", "30617:<base-repo-owner-pubkey>:<base-repo-id>"],
["title", "<merge request title>"], //
["state", "[wip|ready|final]"], // wip: share an idea, knowing it's not ready to merge, ready: author thinks it can be merged, final: author doesn't intend to update the source repo after feedback
["source", "<url for git-cloning>", "<branch name>", "<current branch head sha1-hex>"], //
["target", "<url for git-cloning>", "<branch name>", "<current branch head sha1-hex>"], //
]
}
```
## Merge Request Review
```jsonc
{
"kind": 2617,
"content": "<summary/comment allowing markdown>",
"tags": [
["a", "30617:<base-repo-owner-pubkey>:<base-repo-id>"],
["a", "31617:<merge-request-owner-pubkey>:<merge-request-d-tag>"],
["type", "[ACK|NACK|...]"],
["anchor", "<source-commit-id>", "<target-commit-id>"],
["source-comment", "<file-with-path>:<line>", "<comment>"],
["target-comment", "<file-with-path>:<line>", "<comment>"],
]
}
```
Both `a` tags are required. The other tags are optional. `type` and `anchor` may only be used once.

Copy link
Member Author

@fiatjaf fiatjaf Jan 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the branch merge thing would probably have to specify a branch name and a URL from where to fetch it. The person calling for a branch merge must have a git server somewhere. I don't know the details.

I like the review kind -- but I would leave that for much later (it's good to have the proposal here though).

I don't understand the "notify owner of improvement", to me that fits under the "issue" kind, but what do I know?

I was also thinking of another kind for standalone inline comments of files, but all of that can also be done much later I think.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The MR state was just me being creative. An issue would usually not include the fix and when you want to send a fix, you can do so with a patch. The MR with a final state would be better done with a patch but on the other hand, maybe you want to propose a 5GB change, so a patch wouldn't work but if you host a fork, you can propose the 5GB change that way.

Copy link

@cypherhoodlum cypherhoodlum Jan 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The state field is probably not the best way to approach this. For example, closing an issue or a PR doesn't have to publish a whole new Issue Event, having just one field changed. I think it's better to have a separate kind for closing issues and PRs.

34.md Outdated Show resolved Hide resolved
@buttercat1791
Copy link

This proposal has a lot in common with the NIP-62 PR I opened recently. Perhaps we can merge them.

I think NIP-62 proposes a general solution that can be applied to the Git use case, whereas NIP-32 is very Git-specific. Git is a large use case, so it could make sense to have a NIP solely for that purpose; but in general, don't we want NIPs to be reusable and broadly applicable?

@fiatjaf
Copy link
Member Author

fiatjaf commented Jan 22, 2024

@buttercat1791 no, I think having NIPs not be general is better. Of course it depends and we must think of it case-by-case, but as a rule of thumb less general ends up being better.

@fiatjaf
Copy link
Member Author

fiatjaf commented Jan 22, 2024

@jb55 what do you think is missing or could be changed here?

34.md Show resolved Hide resolved
@alexgleason
Copy link
Member

Aside fom "head", the data format is almost like package.json

@RandyMcMillan
Copy link
Contributor

git commit proof of work miner

#gnostr

https://github.com/gnostr-org/gnostr-legit

@alexgleason
Copy link
Member

It would be worth giving ForgeFed a look over: https://forgefed.org/spec/

GitLab is currently implementating this: https://gitlab.com/groups/gitlab-org/-/epics/11247

@SilberWitch
Copy link
Contributor

@fiatjaf I would be interested to see your analysis on why there needs to be a specific NIP for each type of version control system, when we could easily define the same solution for multiple types.

Are we really to have a seperate NIP for each brand name covered?

@Giszmo
Copy link
Member

Giszmo commented Jan 23, 2024

@alexgleason this nip is explicitly an interoperability layer between gitlab, gitea, github and your various local git instances.

From the document you linked:

The gist of it is: what people really want is to have one global "Gitlab network" to be able to interact between various projects without having to register on each of their hosts.

The gist of this nip here: what people really want is to have one global "Git network" to be able to interact between various projects without having to register on each of their hosts.

@fiatjaf
Copy link
Member Author

fiatjaf commented Jan 23, 2024

@SilberWitch not for each version control system, just for git. No one uses the others.

git is very specific and an open protocol of its own and requires special handling. It's also used by a ton of people, so it makes no sense to try to shove it into some generic thing that wouldn't know about its intricacies. We would still need to write software specific for git with hardcoded git stuff in it, so why not define that in a specific NIP?

Now for some kind of plaintext diffing that doesn't come with all the crazy features that git has (including its network effect) we can come up with our own scheme, like the NIP-62 proposal linked above. Indeed it would be very useful given that there is basically no decent version-control of anything except code in this world. Many people have tried to apply git or git-like approaches to other things and as far as I know never had any success.

@fiatjaf
Copy link
Member Author

fiatjaf commented Jan 23, 2024

https://github.com/fiatjaf/nak, https://github.com/nbd-wtf/go-nostr and https://github.com/fiatjaf/gitstr are now accepting patches over Nostr. Check their READMEs.

@vitorpamplona
Copy link
Collaborator

This looks really good. Can we find a common way to mark the patch/issue as merged/closed?

@buttercat1791
Copy link

@fiatjaf As far as I can tell, the core concept behind both NIP-62 and NIP-34 is pretty much the same: define an event to announce something happening off of Nostr (such as an update to a Git repository) and offer clients hints as to where to find out more (such as a git server URL).

Having some common standard for announcing off-Nostr events would be valuable, I think. Git-specific things, like patches, issues, etc. can and should be defined separately, but I think the underlying concept of announcing off-Nostr things is inherently reusable and should be defined as such.

34.md Outdated Show resolved Hide resolved

The tags `web`, `clone`, `patches` and `issues` can be specified multiple times. Except `d`, all tags are optional.

## Patches
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how do you send a patch series?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it simpler to send a pull request?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know? How do people normally do it in git send-email? I know you can send multiple patches in the same email and you could do the same in the Nostr event. Then when applying with git am -i <patch> it works fine.

Or you could send multiple events? I'm not sure what is the best flow.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

git send-email sends multiple emails when you give it multiple patches. it creates a proper email thread so that you can reply to each patch individually. the nostr equivalent would be building a nip-10 thread of patches.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it simpler to send a pull request?

this implies you have write access to the repo in question, or that you have a hosted fork somewhere. it is much easier to just send patches as it requires no hosting.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about a pull request event which patches tag? later updates based on feedback or rebases could also be associated with the same pull request.

34.md Show resolved Hide resolved
@gsovereignty
Copy link
Contributor

I've been trying to solve the same problem in the context of Nostrocket, I've added my thoughts here: https://nostrocket.org/Snub/problems/c1cc39a3386b96d79dc5b5de335a88acd8476cce191efc76da79fc286955ddfb

@fiatjaf
Copy link
Member Author

fiatjaf commented Jan 23, 2024

@gazhayes I left a comment on your post expressing my disagreements. But aside from that I agree with the rest. So your suggestion is just to do everything as I say here, but add a new kind for "pull request" that includes a URL to another repository from which the code is to be pulled? I like that. We can definitely add that to this NIP.

@jb55
Copy link
Contributor

jb55 commented Jan 24, 2024

@gazhayes I left a comment on your post expressing my disagreements. But aside from that I agree with the rest. So your suggestion is just to do everything as I say here, but add a new kind for "pull request" that includes a URL to another repository from which the code is to be pulled? I like that. We can definitely add that to this NIP.

the equivalent in git+email is git-request-pull

should definitely support that, but we should encourage the patch workflow as that is self contained

patches are best for code review, pull requests are better if you're someone like linus torvalds with submaintainers who review things for you and just need to merge stuff.

"kind": 1617,
"content": "<patch>", // contents of <git format-patch>
"tags": [
["a", "30617:<base-repo-owner-pubkey>:<base-repo-id>"],
Copy link
Contributor

@jb55 jb55 Jan 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we probably also want a "version" tag. patches tend to have more than one version git format-patch -v2, -v3, etc

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean lots of this stuff already shows up in the patch itself, so not sure if worth it. but just something to consider.

34.md Outdated Show resolved Hide resolved
Copy link
Contributor

@DanConwayDev DanConwayDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a final note in my review: Should the key header items of the patch be optionally extract and put into nostr tags to make it easy for non-specialised clients to read or is this putting to much of a burden on clients that apply patches which would need to render it?

Instead of repeating this data it could be extracted and removed. Here is the format:
From [commit-id] Mon Sep 17 00:00:00 2001\nFrom: [author-name] <[author email]>\nDate: [authored-date]\nSubject: \[PATCH\] [commit-msg]\n---\n

note date format here is: ddd, DD MMM YYY HH:mm:ss ZZ

Example:

From 1263051aa4426937c5ef4f7616e06e9a8ea021e0 Mon Sep 17 00:00:00 2001\nFrom: William Casarin <jb55@jb55.com>\nDate: Mon, 22 Jan 2024 14:41:54 -0800\nSubject: [PATCH] Revert "mention: fix broken mentions when there is text is\n directly after"\n\nThis reverts commit af75eed83a2a1dd0eb33a0a27ded71c9f44dacbd.\n---\n

"kind": 30617,
"content": "",
"tags": [
["d", "<repo-id>"],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
["d", "<repo-id>"],
["d", "<repo-id>"], // the earliest unique commit id. usually the root commit id unless it's a fork.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this enables clients with access to the local repo to find repo events for it. It is also resilient to name changes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alternatively, the following could be added.

    ["r", "<repository-unique-reference>"], // the earliest unique commit id. usually the root commit id unless it's a fork.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if I have two projects with the same root commit id?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then there is a fork and the earliest unique commit id should be used instead. the repo event should contain a ["r","[commit-id]"] outlining this so the contributor doesn't need to identify it.

Clients could make more sense of this if, in the case of a fork, 2 tags are used as follows:

Suggested change
["d", "<repo-id>"],
["d", "<repo-id>"], // the earliest unique commit id. usually the root commit id unless it's a fork.
["r", "<repo-id>"],
["r", "<first-commit-id>"], // if the first commit id isn't the earliest unique commit.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me, but I think we should leave d as an arbitrary string and use r for the root/fork git commit identifiers.

34.md Outdated Show resolved Hide resolved
34.md Outdated Show resolved Hide resolved
```jsonc
{
"kind": 1621,
"content": "<markdown text>",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about issue title? It could be a tag or the first line of the content with any markdown heading notation removed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really want Markdown here?

But yes, let's add a title tag.

## Issues

Issues are Markdown text that is just human-readable conversational threads related to the repository: bug reports, feature requests, questions or comments of any kind. Like patches, these SHOULD be sent to the relays specified in that repository's announcement event's `"relays"` tag.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is common place for titles to be updated by the author or maintainer to better reflect the issue at hand. What about an update mechanism similar to the status kind?

34.md Show resolved Hide resolved
["parent-commit", "<parent-commit-id>"],
["commit-pgp-sig", "-----BEGIN PGP SIGNATURE-----..."],
["committer", "<name>", "<email>", "<timezone offset in minutes>"],
]
Copy link
Contributor

@DanConwayDev DanConwayDev Jan 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
]
// if first patch in patch set with no cover_letter
["t", "root"], // if first version
["t", "root-version"], // if first additional version
["e", "<id-of-root-of-previous-version", "mention"], // if first additional version
// else
["e", "<id-of-root-patch-or-cover-letter", "root"], // this may be a root-version
["e", "<id-of-[previous-patch-in-set]-or-for-first-[cover-letter]", "reply"],
]

reflecting proposal in #997 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused by these quotes, "["root"]?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think instead of writing all these e tags in the example we should just say these events should follow NIP-10 threading rules. It will be less confusing for people reading.

Now about the t tags I don't think they help. If a human is reading a list of patches or a thread with multiple patches and comments they will be able to see which patches are roots and which ones are v2, v3 and so, either from the context or from something written in the patch title. And if a machine is reading it or if someone wants to refer to a patch with absolute precision they should use the Nostr event id.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused by these quotes, "["root"]?

typo fixed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think instead of writing all these e tags in the example we should just say these events should follow NIP-10 threading rules. It will be less confusing for people reading.

I agree that what I wrote is confusing regarding threading however, its not clear how NIP-10 threading rules should be applied. Should the third patch in the set 'reply' to the second? a naive reading of the rules may suggest not unless it directly uses the change, but clients shouldn't be expected to automatically detect this. I think we this nip should clarify, for both consistency and as the decision effects comparability with the PR model.

The problem trying to be solved by stating that patches should reply to the previous patch in the PR model is as follows:
Assuming the commit id's cannot be replied upon, how can a client know if the patch is a intended to be used in combination with, or replace, another patch?

typo-PR-model

without the reply mechanism the typo fix 'amended' commit could be seen as a descendant, instead of a replacement by a client. Of course a human would see it, but we want clients to detect this and provide a smooth user experience.

Now about the t tags I don't think they help. If a human is reading a list of patches or a thread with multiple patches and comments they will be able to see which patches are roots and which ones are v2, v3 and so, either from the context or from something written in the patch title. And if a machine is reading it or if someone wants to refer to a patch with absolute precision they should use the Nostr event id.

Here is the problem this is trying to solve: as a client I want to create a single subscription to return the only the root event of proposed changes so that I can display their titles as a list.

I suppose it is not too much of a problem to get all versions of proposed changes and filter them client side by filtering those that 'mention' other proposed changes. This would remove the root-version tag.

How else could this be achieved without the ["t","root"] tag or requesting every patch?

"kind": 1617,
"content": "<patch>", // contents of <git format-patch>
"tags": [
["a", "30617:<base-repo-owner-pubkey>:<base-repo-id>"],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
["a", "30617:<base-repo-owner-pubkey>:<base-repo-id>"],
["a", "30617:<base-repo-owner-pubkey>:<base-repo-id>", "<relay-url>"],
["r", "<repository-unique-reference>"], // the earliest unique commit id. usually the root commit id unless it's a fork.

This associates the patch with the git repository. If the maintainer changes (or if there are multiple), clients can easily query relays for all patch.
also adds relay hint to a

"kind": 1622,
"content": "<markdown text>",
"tags": [
["a", "30617:<base-repo-owner-pubkey>:<base-repo-id>", "<relay-url>"],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
["a", "30617:<base-repo-owner-pubkey>:<base-repo-id>", "<relay-url>"],
["a", "30617:<base-repo-owner-pubkey>:<base-repo-id>", "<relay-url>"],
["r", "<repository-unique-reference>"], // the earliest unique commit id. usually the root commit id unless it's a fork.

This associates the issue with the git repository. If the maintainer changes (or if there are multiple), clients can easily query relays for all issues.


- "status" kind (for letting people know a patch was merged or an issue was fixed or won't be fixed)
- "branch merge" kind (specifying a URL from where to fetch the branch to be merged)
- "cover letter" kind (to which multiple patches can refer and serve as a unifying layer to them)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as mentioned in #997 (comment) we should have a cover_letter as a separate kind than a patch because it is different in nature.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the difference between a "cover letter" kind and an "issue" kind?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A cover letter is always paired with a proposed change. An issue may result in zero or more changes in the future.

@DanConwayDev
Copy link
Contributor

Here is an idea for reducing duplicate patches in the patch model which wouldn't be feasible in the email client model:
patch-data-saving

@jb55
Copy link
Contributor

jb55 commented Jan 31, 2024 via email

@fiatjaf
Copy link
Member Author

fiatjaf commented Feb 7, 2024

As @DanConwayDev pointed out it is important to have a way to identify the "root" of a series of patches and comments that refer to that in order to have UIs that display these patches in a coherent way.

I'm not thinking about just "patch sets", but also reviews of the same patch by the author after comments, or the cases in which someone might reply to someone else's patch proposing a different way to implement the same thing, or even amending someone else's patch with a patch to the patch -- I don't know.

We have some options:

  1. enforce a tag t=root or something like that for the first patch in a series of patches so they can be queried on relays
  2. download everything and assemble threads using NIP-10 rules on the client side
  3. use a different kind to establish a root (either the "cover letter" kind with patches attached to it or the first patch in a series uses a different kind from the others)

@jb55 @gazhayes @vitorpamplona what do you think would be the best?

@fiatjaf
Copy link
Member Author

fiatjaf commented Feb 7, 2024

Actually now that I wrote that I think this might be a nice solution:

  1. the first patch has an a tag pointing to the repository announcement, but the subsequent patches don't, they only refer to the initial patch, so they don't show in lists of patches for any given repo.

@jb55
Copy link
Contributor

jb55 commented Feb 7, 2024 via email

@jb55
Copy link
Contributor

jb55 commented Feb 7, 2024 via email

@vitorpamplona
Copy link
Collaborator

vitorpamplona commented Feb 7, 2024

I have very little knowledge of how git actually works (I never cared enough to learn the clusterfuck of commands that is git), so take this suggestion with a grain of salt.

I would keep the Repo Announcement kind (30617) and create two chain heads as new event kinds:

  • Issue: (1621)
  • Change Request (1618): -- new kind

These don't necessarily contain any description or patch but they SHOULD have a subject line. They just start a discussion thread in the repo. They are designed to help build the "GitHub" client and help repo owners navigate their issues and patches with sanity. Both of these will receive labels by the repo owner on their status (New, Accepted, Closed, etc).

From those two kinds, users can attach Comments/Replies (kind:1622) and Patches (1617). These events MUST point to an Issue OR a Change request which controls their lifecycle. People can submit a Patch during a discussion of an Issue (say to show how things could be done) and a Comment during the discussion of a Change Request. They are interchangeable but their order in the chain matters.

Both Comments and Patches should reference their parent event in the same way kind 1 replies work. So, not only patches can be applied to the main repo, but also to other patches (or after parent patches were applied to the repo).

The "GitHub" client can help take one of these tree branches of replies and patches, apply all patches in sequence to the main repo before merging them. There will be ways to preview each step and rebase them if needed.

The order of patches will be dictated by the e-tags and not by the date of each post.

Once an Issue or a Change Request is marked as closed, all comments/replies and patches are also closed.

@DanConwayDev
Copy link
Contributor

today I released the first version of https://gitworkshop.dev and ngit

i'd really appreciate your feedback

announcement event on nostr

you can read more about them on the about gitworkshop.dev and ngit pages

they implement this nip34 draft with some optional and backwards comparable features (which we could call nip34+ for now), most of which I have mentioned in the thread. the most notable features are to enable a PR-like workflow for those who want it.

if anyone's interested I can produce a revision of this nip so you can compare them more easily

@vitorpamplona
Copy link
Collaborator

I don't know if NIP-34 is the place for this or not, but I need releases and release notes. :)

@fiatjaf
Copy link
Member Author

fiatjaf commented Mar 2, 2024

I think it is. I was thinking of having an event kind for tracking commits and tags. Like you could publish it on every commit, or on every tag so people following a repo could automatically update their local version, or CI services could have their actions triggered.

A tag event with a description is a "release".

@vitorpamplona
Copy link
Collaborator

@DanConwayDev I see that you are using different event types for each status (Open, Closed, Resolved). Was that necessary?

@DanConwayDev
Copy link
Contributor

@DanConwayDev I see that you are using different event types for each status (Open, Closed, Resolved). Was that necessary?

Originally I had a single status kind and the status name would be included as a hashtag. The approach of using separate kinds for each status, suggested by @fiatjaf, reduces the likelihood of status bloat.

I intend to add some optional tags to the applied status kind (1631):

  1. ["applied-as-commit","<commit-id-1>", "<commit-id-2>",...] if the patch, or patches within the patch set, were applied
  2. ["merge-commit","<merge-commit-id>"] if the patches were recreated as commits, retaining their commit id, and merged.

I am currently reusing the same status kinds for root patches and issues. Perhaps that's less kinds for clients to worry about but the 'applied' status feels awkward as it really means 'resolved' in the context of an issue. Perhaps an additional kind could be added just for that.

It is also my intention to enables labels (with nip32) so that proposals and issues can be tagged with 'bug', 'feature' 'good-first-issue', 'vnext', 'repo-specific-label', etc. repo maintainers could optionally include a field in their repo event that specifies labels they would like to see used.

I think it is. I was thinking of having an event kind for tracking commits and tags. Like you could publish it on every commit, or on every tag so people following a repo could automatically update their local version, or CI services could have their actions triggered.

A tag event with a description is a "release".

Are you suggesting an event per commit? whats the rationale?
My original nip proposal 561 included kinds for commits, branches and tags. I moved away from that idea (and storing all commits diffs in events, removing the need for a git server) in favor of a more minimalist approach of including branch and tag states in the repo event as mentioned in a comment. Repo events could be the authority, reducing trust in git_servers, allowing multiple mirrored git servers, which could be seamlessly swapped out by maintainers. This has the benefit of storing only a single replaceable event rather than 1000's of commit events, simplifying life for clients and leaving the heavy lifting of storing and serving git data efficiently to git server's which do this very well.

storing releases in a replaceable tag event is an interesting idea. links to files such as binaries, checksums could be added later, if some of these require the assistance of CI tools (eg platform specific binaries).

@fiatjaf
Copy link
Member Author

fiatjaf commented Mar 5, 2024

If no one opposes that I'm going to merge this and then @DanConwayDev can you open a PR adding the status stuff?

@vitorpamplona
Copy link
Collaborator

Let's go! We got 3 clients implementing it.

@fiatjaf fiatjaf marked this pull request as ready for review March 5, 2024 11:49
@fiatjaf fiatjaf merged commit 9a28379 into master Mar 5, 2024
@fiatjaf fiatjaf deleted the git branch March 5, 2024 11:58
@gsovereignty
Copy link
Contributor

Let's go! We got 3 clients implementing it.

make that 4. Nostrocket too, and also this service: https://nostrocket.org/products, so 5 depending on how you count.

@DanConwayDev
Copy link
Contributor

Looks like I was 20 minutes late but DanConwayDev@6fba968 includes some extra tags and clarifications in case you would like to cherry pick any of it.

@fiatjaf
Copy link
Member Author

fiatjaf commented Mar 5, 2024

Why late? No, the idea was that you would open the PR to master.

@fiatjaf
Copy link
Member Author

fiatjaf commented Mar 5, 2024

It's ridiculous that we're using GitHub PRs to define how we will get rid of GitHub PRs.

Copy link

@ariard ariard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good overall.

I personally have given up on pgp. It definitely shines in key rotation,
I give it that, but key rotation in pgp is a huge pain. Right now I
can't really rotate my pgp keys on my yubikey because I have to go
through 100 arcane gpg smartcard steps that I can't be bothered to
figure out again. nostr has already replaced pgp as my web of trust
model for most things.

pgp has a fundamental advantage over NIP-01 as an authenticated format message.
you have multiple signature scheme algorithm supported.
nostr has only one so far, schnorr bip340.


The tags `web`, `clone`, `relays` can have multiple values.

Except `d`, all tags are optional.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think you can recommend d to be a 80-bit / 128-bit random string.
that way minimize collision at the relay level
there is name for human-readable label.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using human readable d identifiers follows a precedent established outside of nip34.
replaceable events are referenced with <kind>:<pubkey>:<identifier> so the collision problem is limited to repo events issued by the same pubkey.
The r tag with a euc marker (earliest unique commit) can also be used to identify the repository.

"tags": [
["a", "30617:<base-repo-owner-pubkey>:<base-repo-id>"],
["p", "<repository-owner>"],
["p", "<other-user>"], // optionally send the patch to another user to bring it to their attention
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can say a default limit (e.g 20’s other-user).
relays can have local policy saying they allow more.
or price them with zaps or other micro-payments.
otherwise amplification attack vector for relay.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

events that tag large numbers of pubkeys or events can be identified as spam either at the relay level or by clients.
I'm not sure there needs to be a default limit as other spam identification criteria can be used such as web of trust, reports, etc.

// if the maintainer doesn't care about these things
["commit", "<current-commit-id>"],
["parent-commit", "<parent-commit-id>"],
["commit-pgp-sig", "-----BEGIN PGP SIGNATURE-----..."], // empty string for unsigned commit
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

often on projects like linux, you can have commit signed by multiple authors / contributors.
so i think “commit-pgp-sig” + “committer” could be option duplicated.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's an interesting idea. git only supports a single pgp signature which in the git-over-email model is the committer rather than the author(s).
The main purpose of retaining the pgp signature is to retain the original commit id.
Including more than one pgp signature and committer would make it unclear which should be used to create the commit id.
What would be the value of including additional pgp signatures when git can't interpret them and all patches / responses are in signed nostr events?

["d", "<repo-id>"],
["name", "<human-readable project name>"],
["description", "brief human-readable project description>"],
["web", "<url for browsing>", ...], // a webpage url, if the git server being used provides such a thing
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think this could document all you could use directly for mailto URI email address.
emails still widely used for project like linux.
also a fall-back if the list of original relay is perm down.

"content": "<markdown text>",
"tags": [
["a", "30617:<base-repo-owner-pubkey>:<base-repo-id>"],
["p", "<repository-owner>"]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think you could have a list of “p” - “other-user” in kind 1621 as generally you have many people working on large projects.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. The p tag can be used on any event to bring it to a users attention. The explicit inclusion of this only in the Patch section of the nip is potentially confusing.

["a", "30617:<base-repo-owner-pubkey>:<base-repo-id>", "<relay-url>"],
["e", "<issue-or-patch-id-hex>", "", "root"],

// other "e" and "p" tags should be applied here when necessary, following the threading rules of NIP-10
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think you could have “relay-local” threading policy on max default “p” elements.

["description", "brief human-readable project description>"],
["web", "<url for browsing>", ...], // a webpage url, if the git server being used provides such a thing
["clone", "<url for git-cloning>", ...], // a url to be given to `git clone` so anyone can clone it
["relays", "<relay-url>", ...] // relays that this repository will monitor for patches and issues
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is missing some timestamp / expiry notion.
you can have collaborative codes contributions spawning years on some project.
that ways can be used for policy migration if you wish to update repo-id / relays / root.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all nostr events must include a created_at timestamp.
any nostr event with a kind in the 30000-40000 range is a 'parameterized replaceable event'. This means all metadata apart from the pubkey and d tag can be updated by issuing a new event with a larger timestamp.
a deletion event (nip09) would indicate the repository event has expired

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.