RFC WIP: Define the three return states of HaveBlob as true/false/error

[mtrmac]

(WIP because this includes #202 and #203 just because I am lazy to do this several times.)

@runcom PTAL: I know in #63 you have explicitly asked for ErrBlobNotFound to be added, but it seems to me that just makes the uses of HaveBlob more complex. I can live with the current code if it is indeed is more idiomatic, or perhaps I am missing a cleaner way to use it.

The current interface can return (haveBlob, _, err):

• (true, nil): blob exists
• (false, ErrBlobNotFound): blob does not exist or unknown
• (false, other non-nil): unexpected error

i.e. haveBlob is equivalent to err == nil, which is both redundant and makes the interface unnecessarily difficult to use for the typical case where the caller wants to abort on an unexpected error, and then decide based on haveBlob:

if err != nil && err != types.ErrBlobNotFound {
   return err
}
if haveBlob { … }

Insted, define the interface to be

• (true, nil): blob exists
• (false, nil): blob does not exist or unknown
• (false, non-nil): unexpected error

which simplifies the “abort” conditional to a simple err != nil.

A possible alternative would be to eliminate the haveBlob bool instead, but that still requires two-clause checks for aborting, and semantically a blob not being present is not an error, it is a succesfully obtained return value.

[runcom]

As long as that interface is respected and (false, nil) always means not found, I'm fine. We need to fix up cri-o as well cause we're checking the typed error there instead.
(I won't hide I like the error and the 2 if check, since it's what I'm used to in golang, but I can live w/o that).

[mtrmac]

(I won't hide I like the error and the 2 if check, since it's what I'm used to in golang, but I can live w/o that).

Could you point at an example of such use? I can think of things like os.IsNotExist, but in that case we have os.Open, not os.DoesFileExist(…) (bool, error), i.e. IsNotExist is still a failure to open a file, not a “here’s the information you asked for” return value.

Alternatively, a HasBlob(…) error could work as well, and is perhaps even simpler, at the cost of ~forcing the user to use a specific order if they want the code simple:

err := …HasBlob()
if err == nil {
   // exists
} else if err == types.ErrBlobNotFound {
  // does not exist
} else {
 // unexpected error
}

(perhaps switching the last two clauses, but err == nil has to go first); and also HasBlob returning an error and not a bool is surprising.

The “error != nil only on an unexpected error” semantics works well with the “call function; if err != nil abort; process function return value” model, which seems common and idiomatic in Go (and also more consistent with how exception propagation works in other languages).

[runcom]

Could you point at an example of such use? I can think of things like os.IsNotExist, but in that case we have os.Open, not os.DoesFileExist(…) (bool, error), i.e. IsNotExist is still a failure to open a file, not a “here’s the information you asked for” return value.

yeah I was thinking at that, and yeah, it's not quite the same, I'm fine with this PR though

[mtrmac]

@runcom (Low-priority) PTAL eventually.

If you don’t like this, feel completely free to close it and leave the code as is, or tell me how to rewrite.

[runcom]

LGTM (@glestaris ping just in case you're using this and it breaks for you after this change)

[mtrmac]

👍

(@glestaris ping just in case you're using this and it breaks for you after this change)

(This drops types.ErrBlobNotFound, so it causes a very visible breakage at every call site.)