Update to libgit2 6aa06b6 #1237
Conversation
| { 8000 | ||
| /// <summary> | ||
| /// Defaults | ||
| /// </summary> | ||
| GIT_MERGE_FILE_DEFAULT = 0, | ||
|
|
||
| /// <summary> | ||
| /// <summ ary> |
|
generally looks good - left minor comments. |
| @@ -35,7 +35,7 @@ internal struct GitMergeOpts | |||
| /// <summary> | |||
| /// File merging flags. | |||
| /// </summary> | |||
| public GitMergeFileFlags FileFlags; | |||
| public GitMergeFileFlag FileFlags; | |||
There was a problem hiding this comment.
This struct needs a public uint RenameThreshold between the SimilarityMetric and the MergeFileFavorFlags. (Sorry for the breaking change.)
There was a problem hiding this comment.
Oops, I missed that in the diff.
|
Shouldn't |
It should, but hotel wifi is its own special circle of hell, so after nuget failed to upload the package a few times, I decided I'd do it after I get back home. |
2b1fd22 to
913eed2
Compare
|
I've now also added a |
| @@ -23,6 +23,16 @@ protected MergeOptionsBase() | |||
| public bool FindRenames { get; set; } | |||
|
|
|||
| /// <summary> | |||
| /// If set, fail on the first conflict. No merged index will be created. | |||
| /// </summary> | |||
| public bool FailOnConflict { get; set; } | |||
There was a problem hiding this comment.
If this switch actually triggers a throw, maybe should we describe the thrown exception in the xml doc? and maybe rename this member as ThrowsOnFirstConflict ?
There was a problem hiding this comment.
Done (explanation), but I'd rather keep the libgit2 name for the option. It gets harder to know what you're doing if they don't match.
There was a problem hiding this comment.
In order to help to user, shouldn't we describe the type of exception that's going to be thrown?
It gets harder to know what you're doing if they don't match.
I agree, from a troubleshooting perspective, and we've done this for internal types.
However, Throws means something very clear in .NET land. Fail doesn't, and I'd rather stick with the idioms of the language when we're exposing a public type.
There was a problem hiding this comment.
The new text does show which exception we throw. But there's of course the issue of whether we should throw at all, or if this should just be part of the merge result we return.
913eed2 to
575df91
Compare
Instead of throwing an exception, return the merge error in the normal way, as this early failure was requested by the user and thus not an exceptional circunstance.
|
I figured throwing when the user explicitly asked us to error out is Bad Form, so now I've made it so we return the normal error condition, just without any actual conflict listing. |
I'm not sold on this. Thats looks like a disguised scenario for "Can I merge without a conflict?". And returning an empty list of conflicts seems like very weird. The |
This stops at the first error so we can return negatives quicker.
|
This is indeed a feature intended for that scenario, and yes we should make the already-existing Also note that Regardless of whether we want to expose this option to users or hide it behind a very specific method, throwing when we've asked for the short-circuit on error seems wrong. In There is yet another reason why I don't want to make everyone go through |
Exactly. |
Update to 6aa06b6 with the speed fixes for merge and tree parsing.