Update go-ipld-git to a go-ipld-prime codec

[willscott]

The existing go-ipld-git codec allows for translation between git file data, and go-ipld-format node representations of the IPLD dag. This work attempts to instead provide a go-ipld-prime representation of that dag.

• [x] Decoder codec (git -> ipld-prime)
• [x] Encoder code (ipld-prime -> git)
• [x] Round-trip tests

fix #45

[rvagg]

@willscott @mvdan this codec changes all of the struct fields to camel-case, object -> Object, etc. Would you mind if we change it back for consistency with the old codec and the JS one? It's also closer to the native format's idea of naming as well. I can make the change here if you're both OK with that.

[rvagg]

Some additional thoughts here where the differences in forms can be seen from a pathing perspective: https://github.com/ipfs/go-ipfs/pull/8287

If you're OK with it, I'd like to try and get the forms here back to the original.

[mvdan]

That sounds fine to me - I'm not sure if ipld-prime codegen expects capitalized names, for the sake of exported names in the generated Go code.

[willscott]

I also have no problem with changing name case back for consistency

[rvagg]

0436bc7c4 now has the field names matching the original (lower-case, and some other minor adjustments). tagType is unfortunate since we can't use type in the codegen. Tree is now a map too so you can look up by directory name as per the original codec, which lets us path the same way back in the sharness test.

I think we need to do something about this strict need to match Go conventions on these names since it's not always going to match what you want your data model form to look like.

[mvdan]

Out of curiosity, would it be enough to add "repr" mapping of field names so that the repr layer uses lowercase names?

Aside from that, I wonder if it's a bit weird for the schema to have the field names be lowercase, but the type names be uppercase.

[rvagg]

Out of curiosity, would it be enough to add "repr" mapping of field names so that the repr layer uses lowercase names?

Sadly, no, because it's pathing at the data model layer that we want to care about, not encoding. Having renames is only going to help if we cared about these objects going into a standard codec (dag-cbor, dag-json), otherwise they're kind of useless here.

[rvagg]

I wonder if it's a bit weird for the schema to have the field names be lowercase, but the type names be uppercase.

This seems fine to me, it actually looks pretty reasonable to my eyes (eyes that have been trained on C-family-style and aren't as accustomed to the upper-case first character rule, granted). Have a look at the schemas that Hannah wrote up in https://github.com/ipfs/go-unixfsnode/blob/main/data/gen/main.go (although the schema field names don't match what's being generated, but you can see the same thing going on there).

[mvdan]

SGTM, thanks for confirming on both accounts. I've replied to https://github.com/ipld/go-ipld-prime/issues/206 :)

[rvagg]

@mvdan @willscott I've brought this up to what I think is a mergable state:

• rebased on master - which brought in a bunch of stuff to get it working with the PL actions workflows
• updated the README
• done some minor jiggering of the types to match the README, and therefore closer to the original, (one rename of "text" to "message" and a shuffle of some of the struct fields, which mostly shouldn't have any impact on anything).
• brought back the JSON tests using dag-json
• updated to latest go-ipld-prime which fixes the typed link bug

and a few other minor things.

The main difference that I'm in two minds about is date handling - we're pulling out date and timezone and keeping them faithfully in their original format so round-trips work properly. The original codec presented them as a nice stringified date but with the original values kept internally. An approach I've toyed with in the Bitcoin codec (in JS at least and I was thinking of doing the same in Go) was to do both - keep originals and present the nice usable forms. It just raises questions about what happens when/if they get out of sync. Two possible approaches would be: (1) error if they're out of sync when serializing, or (2) ignore the usable forms entirely when serializing.

For now I think it'd be fine to just leave this as is and leave it to the user to reform the stringified version if they want it, we could add back in a utility function to do that if needed, or evolve this code further in the future.

IMO this can be merged now.

[willscott]

Making sure we've got all the loose ends covered here:

• i looked at the at date handling code again. I think i prefer the current splitting out of timezone in the data model, and i believe it actually is backwards compatible - previous codec encodings will have the full date (with timezone) in their date field, and the prime codec will take that encoding and produce the correct git representation of that data.
• We should see if we can clean up the the tagType field nomenclature per https://github.com/ipld/go-ipld-prime/issues/206#issuecomment-889903156

[aschmahmann]

Yes, and a review for other breaking changes. mergeTag seems like a breaking change that seems like something we can just restore. I'm not sure if there are others at the moment.

Posting what the release notes for this release would look like would also be great since as soon as this is merged we're going to cut a release anyway and that release is going to need release notes 😄.

[rvagg]

Have fixed up tagType to type thanks to Eric's hint, and mergeTag is now mergetag, plus a few more readability and clarity tweaks.

Original code has a MergeTag as well as a Tag with the main difference that one has a text field and the other a message field but otherwise they are the same. This one just has Tag now with message which I think is more in line with the git model. That'll be a breaking change from a DAG traversal perspective so we should note it in the release notes.

Will put together some basic release notes next week.

[rvagg]

Release notes:

---

BREAKING CHANGES

• go-ipld-git is now a go-ipld-prime IPLD codec. Use Decode(na ipld.NodeAssembler, r io.Reader) error and Encode(n ipld.Node, w io.Writer) error for direct use if required.
• There is now only one Tag type, MergeTag has been removed which had a text property. Use Tag's message property instead to retrieve the tag message from a commit's mergetag. i.e. <commit>/mergetag/message instead of <commit>/mergetag/text.
• PersonInfo no longer exposes the human-readable RFC3339 format date field as a DAG node. The date and timezone fields are kept as their original string forms (to enable precise round-trips) as they exist in encoded Git data. e.g. <commit>/author/date now returns seconds in string form rather than an RFC3339 date string. Use this value and <commit>/author/timezone to reconstruct the original if needed.

[warpfork]

Might I suggest we drop that change info into a CHANGELOG.md file right in the diff? PRs disappear from memory quickly and aren't easily indexed over.

(I'm pretty happy with the loose format in go-ipld-prime/CHANGELOG.md#released-changes, if having an outline to riff from greases the wheels. :))

[warpfork]

(Informational note, no action required: I found myself nerdsniped by mergetag. Yes, that's a thing. Yes, keeping it alllowercase is correct. Docs here: http://github.com/git/git/blob/master/Documentation/technical/signature-format.txt .)

[rvagg]

Have added a changelog and fixed the readme bork.

Normally I don't like changelogs because it's too easy for them to get out of sync, but recently I've adopted the release-on-every-merge practice in JS and it includes both tag + github release + changelog updates and all I need to do is make sure my commit messages are informative and standardised, e.g. https://github.com/rvagg/cborg/blob/master/CHANGELOG.md & https://github.com/rvagg/cborg/releases & https://www.npmjs.com/package/cborg all from pressing the merge button. Can recommend, although Go folks have other things to worry about in the release process I suppose.

[warpfork]

Sweet. I think this is officially ready for the mergeparty (later this week? early next week?) where we land a bunch of these similar PRs in other repos at once.

[rvagg]

I have created a draft release & tag for this, v0.1.0: https://github.com/ipfs/go-ipld-git/releases

I'm working on some assumptions here about Go (and PL) release practices that may not be quite correct. So someone correct me in the next day or two before I press merge and create the release & tag. I would prefer to use semver properly and do a v1.0.0 but I don't think we're in the habit of doing that with Go so I'm assuming v0.1.0 is appropriate, a tag is sufficient and a GitHub release is a bonus.