Warnings API redux

[carlosmn]

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.

The 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.

[ethomson]

I like this - very simple and elegant.

One 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:

1. Things that really aren't all that important and git itself generally prints a message to the console and soldiers on.
2. Things that are really more like errors but perhaps we are able to continue on, perhaps in a diminished capacity.

An 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.

A user - if they want just CRLF warnings (for example) would be likely to build out a default handler like:

static int values_callback(git_warning *warning, void *payload)
{
    if (warning->type == GIT_WARNING_CRLF) {
        /* i'm interested in this warning! */
        printf("%s\n", warning->message);
        return 0;
    }

    /* i'm not interested in this warning, just return the default */
    return 0;
}

Which would - inadvertently - continue on in the should-be-error cases (which would be errors if there were no warning configured.)

This 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:

1. 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.
2. 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.
3. 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.

I 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.

[carlosmn]

Things that really aren't all that important and git itself generally prints a message to the console and soldiers on.

This is what I'm targeting with this. You may want to print a message, or present a neat table of how badly formatted your files are.

Things that are really more like errors but perhaps we are able to continue on, perhaps in a diminished capacity.

Recoverable errors feel like they're a different kind of beast, and I think separating the callbacks would help to hold it wrong less often.

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.

Would this be a different registration per warning type? I can see this not being too bad as we know the size of the array at compile time.

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.

We can have PASSTHROUGH behaviour here so the user can explicitly tell us they didn't make any decisions.

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.

The previous PR includes this, and I feel the same way :) We do however already provide something similar with the host certificate check, where we say whether we think it's valid, and then let the user decide (or not).

[mikeando]

Since an application may have more than one repository open at a given time and the callback is global, it might be sensible to pass the repository that caused the problem as an argument.

Or the callback could be a per-repository setting.

I guess the repository can probably be deduced from the filename - but that seems a little finicky.

[carlosmn]

There is no guarantee that we know what the repository is where we'd want to generate a warning, or that there even is a repository so it ends up being the same issue with figuring out where the issue comes from.

If you do use multiple repositories, you already have to handle concurrent callbacks for certain operations, so you can extend whatever mechanism you already have.

[ethomson]

One thing that makes this tricky is that generally you provide the payload during the method call. Here, there is no such mechanism.

Generally the warnings will get fired on the same thread that you started from, but that's not always true. It's hard for me to imagine how you would reconcile which warnings were fired from which operation if each of them fired off multiple threads. Unless we either arbitrate the warnings through the calling thread (which sounds crazy) or we stamp some sort of ID into the warning. Perhaps the thread ID of the originating thread. I dunno. This is a little icky, but I think that we need to give a little bit more context to the callers.

[carlosmn]

The only way to provide a per-call payload would be to carry it around anywhere, so I think we're realistically stuck with making the user provide a global payload.

Generally the warnings will get fired on the same thread that you started from, but that's not always true.

Do you have something particular operation in mind where we would raise warnings in a thread we created as opposed to the caller?

Unless we either arbitrate the warnings through the calling thread (which sounds crazy)

We have something similar for pack-objects progress (and I would expect a multi-threaded indexer or checkout would behave the same way) . The threads update the stats and it's the calling thread (which is otherwise sleeping) the one which calls the callback, since we do have the guarantee that we won't jump around threads (at least implicitly). As the places where we do perform multi-threaded operations on behalf of the user should be rather limited, and we would be performing status/progress updates by proxying through the calling thread anyhow. It doesn't seem like a big deal to also make sure warnings in these cases are raised by the calling thread.

or we stamp some sort of ID into the warning. Perhaps the thread ID of the originating thread

Originating as the worker thread that generated the warning when proxying? I suppose we could add this (though iirc thread IDs are tricky to get the right size). But the way I did expect the (presumably tiny amount of) callers which actually do multiple operations concurrently is to set either thread-local data to know which thread is dealing with which repository or attach some global hash table associating a task id (for thread-hopping workpool systems) with the repository.

I would like to provide some more context, if we can have it, but if we only have it sometimes, I'm not sure it'd help too much since a caller would have to handle the case without the information already.

[pks-t]

Shouldn't this rather be GIT_WARNING_NONE?

[pks-t]

git_warning_type would be a bit more obvious

[pks-t]

I bet this kind of stuff would come up quite a lot. So a function to build a warning with a type and a format string would be nice, as well as one cleaning up the warning

[pks-t]

Shouldn't you set a static value here to assert in the caller that the callback was in fact invoked?