[prev in list] [next in list] [prev in thread] [next in thread] 

List:       git
Subject:    Re: [PATCH v3 13/23] log_ref_setup(): pass the open file descriptor back to the caller
From:       Jeff King <peff () peff ! net>
Date:       2016-12-31 17:58:08
Message-ID: 20161231175808.cvm54nmk3x7zoipo () sigill ! intra ! peff ! net
[Download RAW message or body]

On Sat, Dec 31, 2016 at 08:58:43AM +0100, Michael Haggerty wrote:

> > The return value is always "0" or "-1". It seems like it would be
> > simpler to just return the descriptor instead of 0.
> > 
> > I guess that makes it hard to identify the case when we chose not to
> > create a descriptor. I wonder if more "normal" semantics would be:
> > 
> >   1. ret >= 0: file existed or was created, and ret is the descriptor
> > 
> >   2. ret < 0, err is empty: we chose not to create
> > 
> >   3. ret < 0, err is non-empty: a real error
> 
> I don't like making callers read err to find out whether the call was
> successful, and I think we've been able to avoid that pattern so far.

I guess my mental model is that case 2 _is_ a failure, because we didn't
open the reflog. It's just one that callers may want to distinguish from
case 3, because it's probably a silent failure, not one we want to
complain to the user about.

But whether that's accurate would depend on the callers. Looking at the
callers, I think the immediate callers would be happier with this, but
you probably would want to end up converting case 3 back to "return 0"
out of files_log_ref_write().

> > I dunno. This may just be bikeshedding, and I can live with it either
> > way (especially because you documented it!).
> 
> Let's see if anybody has a strong opinion about it; otherwise I'd rather
> leave it as is.

Sounds good.

-Peff
[prev in list] [next in list] [prev in thread] [next in thread]