Don't build a fake signature #1171
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| @@ -761,48 +757,25 @@ private static ConfigurationEntry<string> BuildConfigEntry(IntPtr entryPtr) | |||
| /// <returns>The signature.</returns> | |||
There was a problem hiding this comment.
The signature or null if no user identity can be found in the configuration stores.
2a57749 to
ac0c91d
Compare
| @@ -758,51 +754,30 @@ private static ConfigurationEntry<string> BuildConfigEntry(IntPtr entryPtr) | |||
| /// </para> | |||
| /// </summary> | |||
| /// <param name="now">The timestamp to use for the <see cref="Signature"/>.</param> | |||
| /// <returns>The signature.</returns> | |||
| /// <returns>The signature or null if no use identity can be found in the configuration.</returns> | |||
There was a problem hiding this comment.
s/no use identity/no user identity/
When there is no user information in the configuration files, BuildSignature() sets "unkown" as the name and uses the environment variables to create the e-mail. This seems peculiar, since it means a consumer of the library cannot use this method to figure out if this configuration does exist but has to do it themselves. As far as I can tell, this started as a way to provide something to the reflog when the user has not set up the information as it's more transient data and progress matters more than accuracy in that case. This is now handled by libgit2 itself, so there is no need for this behaviour inside the library. This leaves us with the curious case of this fake signature only being provided to a consumer of the library when this behaviour was due to some internal users. It also makes it harder than it needs to be to know if the signature the library provided is accurate. Resolve this situation by returning null when there is no signature configured. The internal users can then move to use a method which throws a message mentioning the lack of a necessary signature.
ac0c91d to
53f880c
Compare
nulltoken
added a commit
that referenced
this pull request
Aug 17, 2015
Don't build a fake signature
This was referenced Mar 5, 2016
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
None yet
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When there is no user information in the configuration files, BuildSignature() sets
"unkown" as the name and uses the environment variables to create the
e-mail. This seems peculiar, since it means a consumer of the library
cannot use this method to figure out if this configuration does exist
but has to do it themselves.
As far as I can tell, this started as a way to provide something to the
reflog when the user has not set up the information as it's more
transient data and progress matters more than accuracy in that
case. This is now handled by libgit2 itself, so there is no need for
this behaviour inside the library.
This leaves us with the curious case of this fake signature only being
provided to a consumer of the library when this behaviour was due to
some internal users. It also makes it harder than it needs to be to know
if the signature the library provided is accurate.
Resolve this situation by returning null when there is no signature
configured. The internal users can then move to use a method which
throws a message mentioning the lack of a necessary signature.
I'm not sold on the name of the new method or the exception message, but returning a fake signature to the user is IMO the least useful thing we could do here.