Add git note support #166

2015-01-06T12:28:36-06:00

benburkert commented

2015-01-06 12:28:36 -06:00

(Migrated from github.com)

Adds support for the git_note_* functions.

Adds support for the `git_note_*` functions.

calavera commented

2015-01-06 16:06:14 -06:00

(Migrated from github.com)

Hi @benburkert 👋

Nice PR! 🎊

Hi @benburkert :wave: Nice PR! :confetti_ball:

benburkert commented

2015-01-07 14:51:11 -06:00

(Migrated from github.com)

@carlosmn I fixed the typo in the comment.

carlosmn commented

2015-01-08 04:45:11 -06:00

(Migrated from github.com)

The receivers for the methods are values rather than objects (e.g. func (n Note) Id() *Oid {). AFAICT this will create a copy of the original Note object.

This could then create a race condition between this copy for use in these methods and the original object being garbage-collected, which would call the finalizer and free the underlying C object, making the C function call use a dangling pointer.

All this to say that as I understand it, we need to have pointer receivers instead of value receivers.

The receivers for the methods are values rather than objects (e.g. `func (n Note) Id() *Oid {`). AFAICT this will create a copy of the original `Note` object. This could then create a race condition between this copy for use in these methods and the original object being garbage-collected, which would call the finalizer and free the underlying C object, making the C function call use a dangling pointer. All this to say that as I understand it, we need to have pointer receivers instead of value receivers.

benburkert commented

2015-01-08 13:36:28 -06:00

(Migrated from github.com)

I switched the methods definitions to pointer receivers. I tend to define methods on values for non mutating functions of small structs (as a way of marking it as "const"), but it looks like the convention in this package is to define methods on pointers.

Although I haven't found anything to back this up definitively, I don't think the finalizer is copied when a struct is placed on the stack. Assuming that's the case, then dangling pointers should not be an issue from inside these methods because the previous stack frame would always hold a reference to the value on the heap and prevent GC from calling the finalizer.

I switched the methods definitions to pointer receivers. I tend to define methods on values for non mutating functions of small structs (as a way of marking it as "const"), but it looks like the convention in this package is to define methods on pointers. Although I haven't found anything to back this up definitively, I don't think the finalizer is copied when a struct is placed on the stack. Assuming that's the case, then dangling pointers should not be an issue from inside these methods because the previous stack frame would always hold a reference to the value on the heap and prevent GC from calling the finalizer.

carlosmn commented

2015-01-09 03:16:11 -06:00

(Migrated from github.com)

Using the structs as values works well if we do everything with the garbage collector, but we can't increase a refcount on copy.

The finalizer applies to a particular object rather than the class/type, so it would not be copied. While it's unlikely that the garbage run would decide to free the original object, this is not something we should assume. I've seen situations where a variable is freed mid-function after the last usage, which could be an issue here.

Using the structs as values works well if we do everything with the garbage collector, but we can't increase a refcount on copy. The finalizer applies to a particular object rather than the class/type, so it would not be copied. While it's unlikely that the garbage run would decide to free the original object, this is not something we should assume. I've seen situations where a variable is freed mid-function after the last usage, which could be an issue here.

Sign in to join this conversation.

No reviewers