{"action":"created","issue":{"url":"https://api.github.com/repos/libgit2/libgit2/issues/3722","repository_url":"https://api.github.com/repos/libgit2/libgit2","labels_url":"https://api.github.com/repos/libgit2/libgit2/issues/3722/labels{/name}","comments_url":"https://api.github.com/repos/libgit2/libgit2/issues/3722/comments","events_url":"https://api.github.com/repos/libgit2/libgit2/issues/3722/events","html_url":"https://github.com/libgit2/libgit2/pull/3722","id":145153058,"number":3722,"title":"Warnings API redux","user":{"login":"carlosmn","id":335443,"avatar_url":"https://avatars.githubusercontent.com/u/335443?v=3","gravatar_id":"","url":"https://api.github.com/users/carlosmn","html_url":"https://github.com/carlosmn","followers_url":"https://api.github.com/users/carlosmn/followers","following_url":"https://api.github.com/users/carlosmn/following{/other_user}","gists_url":"https://api.github.com/users/carlosmn/gists{/gist_id}","starred_url":"https://api.github.com/users/carlosmn/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/carlosmn/subscriptions","organizations_url":"https://api.github.com/users/carlosmn/orgs","repos_url":"https://api.github.com/users/carlosmn/repos","events_url":"https://api.github.com/users/carlosmn/events{/privacy}","received_events_url":"https://api.github.com/users/carlosmn/received_events","type":"User","site_admin":true},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":1,"created_at":"2016-04-01T11:19:39Z","updated_at":"2016-04-01T14:01:21Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/libgit2/libgit2/pulls/3722","html_url":"https://github.com/libgit2/libgit2/pull/3722","diff_url":"https://github.com/libgit2/libgit2/pull/3722.diff","patch_url":"https://github.com/libgit2/libgit2/pull/3722.patch"},"body":"This is another attempt at #2101 which incorporates the subclassing described in the comments. An application may choose to print to stdout/stderr or it may look into the subclass to show the warnings in a GUI or to provide its own format.\r\n\r\nThe commit signature parsing code has changed quite a bit since the original PR, so I've used CRLF as an example of how we'd use it."},"comment":{"url":"https://api.github.com/repos/libgit2/libgit2/issues/comments/204406998","html_url":"https://github.com/libgit2/libgit2/pull/3722#issuecomment-204406998","issue_url":"https://api.github.com/repos/libgit2/libgit2/issues/3722","id":204406998,"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":true},"created_at":"2016-04-01T14:01:21Z","updated_at":"2016-04-01T14:01:21Z","body":"I like this - very simple and elegant.\r\n\r\nOne concern that I have is that - as a consumer - I may have to carry a lot of state to figure out what I should do in the default cases.  With this implementation, I register a single warning handler that will get called for all types of warnings.  I think there are two types of warnings that we will end up adding:\r\n\r\n1. Things that really aren't all that important and git itself generally prints a message to the console and soldiers on.\r\n2. Things that are really more like _errors_ but perhaps we are able to continue on, perhaps in a diminished capacity.\r\n\r\nAn example of #2 is a malformed commit, which we _could_ fill with crazy default values (\"Unknown\", perhaps) but we would not want to in the default case.\r\n\r\nA user - if they want _just_ CRLF warnings (for example) would be likely to build out a default handler like:\r\n\r\n```\r\nstatic int values_callback(git_warning *warning, void *payload)\r\n{\r\n    if (warning->type == GIT_WARNING_CRLF) {\r\n        /* i'm interested in this warning! */\r\n        printf(\"%s\\n\", warning->message);\r\n        return 0;\r\n    }\r\n\r\n    /* i'm not interested in this warning, just return the default */\r\n    return 0;\r\n}\r\n```\r\n\r\nWhich would - inadvertently - continue on in the should-be-error cases (which would be errors if there were no warning configured.)\r\n\r\nThis is a sort of \"you're holding it wrong\" situation, but I think we're really _encouraging_ people to hold it wrong.  In this sort of situation, people would really need to know what the default should be for each type of warning.  There are a couple of ways to mitigate this:\r\n\r\n1. We could let people register each type of warning.  This was what I was thinking that we would do, but obviously this complicates _our_ implementation and I like the simplicity here.\r\n\r\n2. Make a return code of 0 mean \"do the default\", a negative return code a failure, and a positive return code a success.  This is sort of non-obvious, but has an interesting bit of simplicity.\r\n\r\n3. _Give_ them the default (pass / fail).  This is probably the easiest, and most sensible, but still feels a little bit weird, in a way I can't really explain.\r\n\r\nI lean a bit towards 1, but I'm pretty flexible.  I'm more interested in making sure that we don't trap the user here."}}