Extend RepositoryOptions to accept path to alternate System and Global config files #160
Conversation
| public static extern int git_config_add_file_ondisk( | ||
| ConfigurationSafeHandle cfg, | ||
| [MarshalAs(UnmanagedType.CustomMarshaler, MarshalTypeRef = typeof(FilePathMarshaler))] FilePath path, | ||
| IntPtr priority); |
There was a problem hiding this comment.
Why is this a pointer? The function expects an int.
There was a problem hiding this comment.
int is either 4 bytes on x86 or 8 bytes on amd64.
IntPtr has its size aligned with the architecture and thus matches the int size.
I know, it's far from being explicit, but I haven't found a better alternative yet :)
There was a problem hiding this comment.
I feel like we've been through this before... An int is 32-bits in C, be it a 32-bit or 64-bit machine. And MSDN tells me this is also the case with C#'s int type. An IntPtr will be the size of a pointer in the machine, which is not the same as an int in 64-bit processors, which means you'd be passing a variable with the wrong size. Here it's not that bad because it'd be at the top of the stack, so the other parameters would still be in the right place, but I'm not sure we'd end up reading the lower bytes of the argument (in amd64 more args get passed through registers, so this might end up working because of that).
There's also ints being used all over NativeMethods.cs where we want an int, and it seems to work just fine.
There was a problem hiding this comment.
ZOMG! You're right! Thank you for pointing at this mistake. The issue lies with longs, not ints.
And we might have already discussed this while you were implementing the retrieval of 64bits wide entries from the config.
There was a problem hiding this comment.
Yeah, I thought you might have been thinking about longs after I posted. It's all a mess, but ints are probably the most reliable datatype you can have.
There was a problem hiding this comment.
Thanks again for watching my back!
On a less positive note, this also means I have to review all the potentially wrongly chosen IntPtr in the NativeMethods type :)
…l configuration files Fix #157
|
Merged in vNext |
No description provided.