{"action":"created","issue":{"url":"https://api.github.com/repos/libgit2/libgit2/issues/2101","labels_url":"https://api.github.com/repos/libgit2/libgit2/issues/2101/labels{/name}","comments_url":"https://api.github.com/repos/libgit2/libgit2/issues/2101/comments","events_url":"https://api.github.com/repos/libgit2/libgit2/issues/2101/events","html_url":"https://github.com/libgit2/libgit2/pull/2101","id":26917068,"number":2101,"title":"Add a warning API and use it for bad signatures","user":{"login":"arrbee","id":103350,"avatar_url":"https://avatars.githubusercontent.com/u/103350?v=3","gravatar_id":"","url":"https://api.github.com/users/arrbee","html_url":"https://github.com/arrbee","followers_url":"https://api.github.com/users/arrbee/followers","following_url":"https://api.github.com/users/arrbee/following{/other_user}","gists_url":"https://api.github.com/users/arrbee/gists{/gist_id}","starred_url":"https://api.github.com/users/arrbee/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/arrbee/subscriptions","organizations_url":"https://api.github.com/users/arrbee/orgs","repos_url":"https://api.github.com/users/arrbee/repos","events_url":"https://api.github.com/users/arrbee/events{/privacy}","received_events_url":"https://api.github.com/users/arrbee/received_events","type":"User","site_admin":true},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":19,"created_at":"2014-02-04T21:28:32Z","updated_at":"2015-04-27T15:45:16Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/libgit2/libgit2/pulls/2101","html_url":"https://github.com/libgit2/libgit2/pull/2101","diff_url":"https://github.com/libgit2/libgit2/pull/2101.diff","patch_url":"https://github.com/libgit2/libgit2/pull/2101.patch"},"body":"The idea here is to add an API for reporting warnings where data appears to be bad but recoverable. For example, having an invalid timestamp in a commit signature or finding a `.gitattributes` file that can't actually be read. In these cases, we are currently faced with either reporting an error and failing in a situation that we could continue from (and Git often would), or ignoring the problem without giving the caller any notice that some questionable data was encountered (and in some cases, returning partially incorrect data as a result - e.g. we have no way of representing a bad signature timestamp, so we would just use a fallback value).\r\n\r\nThe driver for this is that we've been encountering some cases where libgit2 is more strict that Git for handling some data and I wanted a better option that just ignoring data problems when they are seen.\r\n\r\n**This is just as initial take.**  I expect that the API will need to change quite a bit before we accept it into libgit2. Currently, I wanted the warning callback to get a lot of data about what was happening, so I tried to shoehorn things into a generic API with a lot of parameters.\r\n\r\nThinking it over, I think there are a number of options to proceed:\r\n\r\n1. Use something like I have here with a generic warning signature (probably with fewer parameters than I've specified) and make the warning handler look at the message to decide what to do.\r\n2. Create warning specific structures that get filled in depending on the warning, along with adding the specific warning codes to `include/git2/sys/warning.h` so the callback can know what structure it is looking at.\r\n3. Add some sort of warning/error context stack where we could push data frames (e.g. top frame is a repository object, next frame down is a commit object being loaded, next frame down is the signature object being filled in, final frame is the string being parsed), and then the callback gets the stack of data frames with the option to report the error as desired. This could be useful for errors, too. Obviously this would have to be done with thread-local data.\r\n\r\nWe'll also have to decide where the warning callback should reside and how it should be set. I've opted to make it a single global value right now. I think that's probably the only good option. I've also opted to set it via `git_libgit2_opts` but I could easily add a setter API directly to `include/git2/sys/warning.h` which would restore type-safety (i.e. I should probably just do that).\r\n"},"comment":{"url":"https://api.github.com/repos/libgit2/libgit2/issues/comments/96717907","html_url":"https://github.com/libgit2/libgit2/pull/2101#issuecomment-96717907","issue_url":"https://api.github.com/repos/libgit2/libgit2/issues/2101","id":96717907,"user":{"login":"ethomson","id":1130014,"avatar_url":"https://avatars.githubusercontent.com/u/1130014?v=3","gravatar_id":"","url":"https://api.github.com/users/ethomson","html_url":"https://github.com/ethomson","followers_url":"https://api.github.com/users/ethomson/followers","following_url":"https://api.github.com/users/ethomson/following{/other_user}","gists_url":"https://api.github.com/users/ethomson/gists{/gist_id}","starred_url":"https://api.github.com/users/ethomson/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ethomson/subscriptions","organizations_url":"https://api.github.com/users/ethomson/orgs","repos_url":"https://api.github.com/users/ethomson/repos","events_url":"https://api.github.com/users/ethomson/events{/privacy}","received_events_url":"https://api.github.com/users/ethomson/received_events","type":"User","site_admin":false},"created_at":"2015-04-27T15:45:16Z","updated_at":"2015-04-27T15:45:16Z","body":"So we're trying to do a little housekeeping and clean up some of the pull requests that have been sitting.  This is one that I'm actively interested in.  I apologize for letting the conversation die out here, I think that this is a very worthy topic and I'm mostly :+1: on this implementation.\r\n\r\nMy only thought here is that wondering if we shouldn't make `git_warning_t` a little coarser and then bring some of that deeper information into the `struct git_warning` that is given to the caller.  Maybe something like:\r\n\r\n    typedef enum {\r\n        GIT_WARNING_NONE = 0,\r\n        GIT_WARNING_INVALID_COMMIT, /* default continue */\r\n    } git_warning_t;\r\n\r\nAnd then splitting out the more detailed warning information:\r\n\r\n    typedef enum {\r\n        GIT_WARNING_INVALID_COMMIT__SIGNATURE_TIMESTAMP, /* default continue */\r\n        GIT_WARNING_INVALID_COMMIT__SIGNATURE_TIMEZONE,  /* default continue */\r\n        GIT_WARNING_INVALID_COMMIT__SIGNATURE_EMAIL_MISSING, /* default error */\r\n        GIT_WARNING_INVALID_COMMIT__SIGNATURE_EMAIL_UNTERMINATED, /* error */\r\n    } git_warning_invalid_commit_t;\r\n\r\n    typedef struct {\r\n        git_warning base;\r\n        git_warning_invalid_commit_t type;\r\n        const char *invalid_data;\r\n        int invalid_data_len;\r\n    } git_warning_invalid_data;\r\n\r\nPart of my thinking here is that I wonder if it might be nice to be able to register a single warning type.  Maybe I care deeply about handling invalid data in commits, but I care very little about `core.safecrlf = warn`, and imagine that it's a little bit costly to marshal this data back and forth...  In a large tree, this could end up with lots of warnings getting fired (and marshalled into a .NET warning type just to know that I should ignore it.)\r\n\r\nI haven't actually measured this to know whether this is really problematic or not, this is just a hunch."}}