WIP - Notes

[yorah]

Hi, I started working on git notes. Here is the first draft, more to follow soon.

One of the issue I'm foreseeing, is about how to get all notes, from all namespaces (similar to the git log --show-notes=*). I don't know of a good way to do this (iterating through the refs first to find all the refs/notes/xxx ones?), so if you have an idea, feel free to share it!

Also, in terms of API, I was thinking about making Notes its own class (inheriting from IEnumerable), so that I could have the Delete and Modify methods on it.

[dahlbyk]

Also, in terms of API, I was thinking about making Notes its own class (inheriting from IEnumerable), so that I could have the Delete and Modify methods on it.

NoteCollection would be consistent with BranchCollection, CommitCollection, etc. and is a natural place to support indexing by namespace. A few things that come to mind...

• Since a commit can have a single note per namespace, does enumerating a NoteCollection return notes for all namespaces?
• If so, should Note expose the namespace where it lives?
• How does one access the note for the default namespace - commit.Notes.Default?

[yorah]

NoteCollection would be consistent with BranchCollection, CommitCollection, etc. and is a natural place to support indexing by namespace.

I created NotesCollection, and moved all the retrieval of the notes into it. It feels better not to clutter Commit with that stuff.

Since a commit can have a single note per namespace, does enumerating a NoteCollection return notes for all namespaces?

I think so. However, I'm not sure about the way I'm doing it at the moment (see the enumerator of NoteCollection).

If so, should Note expose the namespace where it lives?

Yep, done. I actually felt it was needed before reading your comment 😄

How does one access the note for the default namespace - commit.Notes.Default?

commit.Notes.Default can be a possibility. However, it made me think about what is the default namespace for LibGit2Sharp.

In git commandline, you can change the default namespace by modifying the GIT_NOTES_REF env variable.
In libgit2, the default namespace is hardcoded with "refs/notes/commits" (GIT_NOTES_DEFAULT_REF).

Thus, the current implementation I tried relies on a hardcoded default to "refs/notes/commits".

[dahlbyk]

NoteCollection looks great! Though I agree the GetEnumerator() implementation feels a bit odd. I wonder if separating out IEnumerable<string> NoteCollection.Namespaces { get; } would be useful, splitting GetEnumerator() as a side-effect.

In git commandline, you can change the default namespace by modifying the GIT_NOTES_REF env variable.
In libgit2, the default namespace is hardcoded with "refs/notes/commits" (GIT_NOTES_DEFAULT_REF).

How does libgit2 handle other places where git commandline respects env variables? Related: libgit2/libgit2#649

Thus, the current implementation I tried relies on a hardcoded default to "refs/notes/commits".

It looks like git_note_read() should accept null for notes_ref, though that would cascade through to Note.Namespace. I wonder if libgit2 should expose git_note_default_ref()?

[yorah]

NoteCollection looks great! Though I agree the GetEnumerator() implementation feels a bit odd. I wonder if separating out IEnumerable NoteCollection.Namespaces { get; } would be useful, splitting GetEnumerator() as a side-effect.

I tried to play with it, but it was the same. Maybe it's just that I don't see what you mean, so if you (or someone else) has time to play with the code, I would be grateful.

I wonder if libgit2 should expose git_note_default_ref()?

I added a comment on libgit2/libgit2#649 about that. In the meantime, I think passing the namespace to be in control of what the default is, is the best option.

On a separate topic, I implemented add/delete/modify notes behavior on the NoteCollection object. @nulltoken, @dahlbyk, would you be so kind as to review it please (at the API level only; I will add the doc when the API is stable)?

I was also wondering if we should have a Notes namespace at the Repository level.
I am not sure it would make sense, because from what I understand a note is strongly related to a given commit. I don't see in which cases one might want to access notes without going through a Commit first.

However, I understand you may have better insight on the use cases than I have, so if you think it would be better, please tell me, and I can try to implement Notes on top of the Repository.

[dahlbyk]

NoteCollection.Namespaces: 5219688f2e04efc2bd11b6f4aa967503f2ef1f53

add/delete/modify look good. Should Edit() provide an overload that doesn't require namespace?

IMO Repository.Notes can wait until someone asks for it. I can't think of a use...

[nulltoken]

I would favor a different approach.

The way I see it is that, from a high-level perspective, a Note is a bit like an annotated Tag. It's a decorator, along with a message, an author and committer. Similarly to a Tag, it can be applied on any GitObject, not only commits. However, contrarily to the Tag, it's ok to mess with it (update/remove, ...) :)

So I'd rather see a Repository.Notes namespace which would expose Create(), Delete(), Namespaces...

In order to facilitate the implementation of the git log scenario, we could indeed add a Notes property to the Commit object. However, this Notes would not be the same full blown NotesCollection as described above. It should only allow the user to enumerate notes, not modify them, nor add some more.

I'd see something similar to this:

Commit commit = repo.Head.Tip;

/* Enumerate notes for this object in the default namespace
 *
 * Would return 0 or 1 note.
 */
foreach (Note note in commit.Notes)
{
    //...
}

/* Enumerate notes for this object in the "builds" namespace
 *
 * Would return 0 or 1 note.
 */
foreach (Note note in commit.Notes["builds"])
{
    //...
}

/* Enumerate notes for this object from all namespaces
 *
 * Would return 0 or more notes.
 */
foreach (Note note in commit.Notes["*"])
{
    //...
}

As for the retrieval of the notes in the main Notes collection, we could try the following

Note note;

/* Retrieve note for specified object in the default namespace */
note = repo.Notes[objectId];

/* Retrieve note for specified object in the "builds" namespace */
note = repo.Notes[objectId, "builds"];

One thing I'm not completely comfortable with is the following remark from the doc

In principle, a note is a regular Git blob, and any kind of (non-)format is accepted.
You can binary-safely create notes from arbitrary files using git hash-object:

My proposal would be to wait for a specific request/use case related to binary notes.

[nulltoken]

http://stackoverflow.com/q/5813311 :)

[yorah]

I think I can almost understand why you want a repo.Notes namespace. At least, I can feel there is an itch to scratch here 😄

If you don't mind, let me ask a few questions to make sure I understand:

• Would it be possible to enumerate Repo.Notes? If yes, what would we enumerate here (I see that for tags, you enumerate all tags, but for notes, there is no git_notes_list method)? If no, I think the name NoteCollection would not be good anymore. In that case, I was thinking about simply Notes, but I would like your thoughts on that.
• Are you sure you want to have repo.Notes return an IEnumerable with 0 or 1 note for the default namespace, and to have repo.Notes["*"] return a real IEnumerable with all the notes? I can see the upside of that (similar to git commandline), but the downside would be that it doesn't feel very C#'ish.

I'm quite sure other questions will pop later. Once again, sorry for being a bit slow on the understanding part, I'm doing my best here ;)

[nulltoken]

Would it be possible to enumerate Repo.Notes?

git notes list allows this. Similarly to commit.Notes behavior we could make the type an IEnumerable and add a string based indexer to control the namespace.

BTW, the doc states that "list [...] all note objects and the objects they annotate" which may tend to make the oid of the annotated object a part of the Equality contributor....

Are you sure you want to have repo.Notes return an IEnumerable with 0 or 1

Huh... commit.Notes would return this 0 or 1 Enumerable, not repo.Notes.
Straight answer: No, I'm not sure. But beside being deferred, it "feels" more composable, more easy to handle in a git-log context with Linq extension methods. I might be completely wrong there, but the if null then ... pattern didn't "feel" right.

Once again, sorry for being a bit slow on the understanding part

Slow? Youmust be kidding me? I think the git notes are one of the most obscur part of git.

I'm doing my best here ;)

No you don't. You're just warming up. You're better than this! 😄

[yorah]

I pushed a spike based on your feedback. Could you please take a look and tell me what you think about that?

Points of interest:

• I kept the Default property: it doesn't feel right not to have it, writing tests without it is... painful? Anyway, it's easy to remove should you not want it.
• Same thing for the IEnumerable returning only 0 or 1 result: it felt weird when writing client code.
• Instead of the syntax note = repo.Notes[objectId, "builds"];, I currently have note = repo.Notes[objectId]["builds"];. This basically allows me to reuse the readonly collection on the Commit (renamed to GitObjectNoteCollection).

Anyway, as everything now goes through repo.Notes, it should be easy to change.

[dahlbyk]

repo.Notes[ref] as a shortcut for repo.Lookup<Commit>(ref).Notes makes sense, but notes don't seem all that interesting to me without their corresponding commit - having the primary access point not be Commit.Notes feels weird.

[nulltoken]

@yorah I pushed a rebound @ https://github.com/nulltoken/libgit2sharp/tree/topic/notes2/rebound

I've moved some code into some extensions methods. And I'd vote to delete it. Let's expose the Notes as is, ordered with the default Namespace in first position.
The git log scenario will be satisfied satisfied with the Commit.Notes

Note: I've worked fast. It's not polished. Sorry for that.

having the primary access point not be Commit.Notes feels weird.

@dahlbyk You will still be able to access the Notes related to the Commit, as a IEnumerable<Note> through a commit.Notes property. The code is not finished yet. Or am I misunderstanding something?

[nulltoken]

@dahlbyk But I'd really prefer not allow addition/updation of Notes through the Commit object.

[dahlbyk]

But I'd really prefer not allow addition/updation of Notes through the Commit object.

Because Commit is otherwise immutable?

[nulltoken]

Because Commit is otherwise immutable?

Yes, mostly.

[yorah]

Hi.
@nulltoken this is what I ended up with, following your "rebound".

I'm still struggling a bit to code the "git notes list" behavior (not sure it's possible, but I'll let you know if I'm really stuck). In the meantime, what do you think about the last commits?

[yorah]

Updated the branch with:

• comments
• notes list implementation

Missing/TODO is to rebase on next vNext to benefit from:

• native libgit2 notes list implementation (Add git_note_foreach() libgit2#696)
• native libgit2 default namespace retrieval (Honor core.notesRef config option libgit2#663)

[yorah]

Done.

@nulltoken, @dahlbyk, can you please perform a final review?

[dahlbyk]

Only thing you might add is a test that enumerates NoteCollection, expecting notes from the default namespace. Otherwise, 👍 from me!

[yorah]

Thanks for the time you spent on this.

Regarding the use case "Enumerating NoteCollection returns notes from the default namespace", you'll see that it is already tested in the test method CanRetrieveTheListOfNotesForAGivenNamespace :)

Assert.Equal("commits", repo.Notes.DefaultNamespace);
Assert.Equal(expectedNotes, repo.Notes.Select(n => new Tuple<string, string>(n.BlobId.Sha, n.TargetObjectId.Sha)).ToArray());

[dahlbyk]

Regarding the use case "Enumerating NoteCollection returns notes from the default namespace", you'll see that it is already tested in the test method CanRetrieveTheListOfNotesForAGivenNamespace :)

So it is - good thinking. 👏

[nulltoken]

I agree with @dahlbyk: Very nice job!

Manually merged in vNext

[nulltoken]

Released as part of v0.9.5