Re: [PATCH v6 1/7] refs.c: add err arguments to reflog functions

[David Turner <dturner@twopensource.com>]

On Mon, 2015-07-06 at 17:53 +0200, Michael Haggerty wrote:
> On 06/29/2015 10:17 PM, David Turner wrote:
> > Add an err argument to log_ref_setup that can explain the reason
> > for a failure. This then eliminates the need to manage errno through
> > this function since we can just add strerror(errno) to the err string
> > when meaningful. No callers relied on errno from this function for
> > anything else than the error message.
> > 
> > Also add err arguments to private functions write_ref_to_lockfile,
> > log_ref_write_1, commit_ref_update. This again eliminates the need to
> > manage errno in these functions.
> > 
> > Update of a patch by Ronnie Sahlberg.
> 
> First a general comment: we have a convention, not yet very well adhered
> to, that error messages should start with lower-case letters. I know
> that in many cases you are just carrying along pre-existing error
> messages that are capitalized. But at a minimum, please avoid adding new
> error messages that are capitalized. And if you are feeling ambitious,
> feel free to lower-case some existing ones :-)

I'll save lowercasing messages for other patches, but I'll try to take
care with new messages.

...

> Above, the call to close(logfd) could clobber errno. It would be better
> to exchange the calls.

Done, thanks.

> Since you are passing err into log_ref_write(), I assume that it would
> already have an error message written to it by the time you enter into
> this block. Yet in the block you append more text to it.
> 
> It seems to me that you either want to clear err and replace it with
> your own message, or create a new message that looks like
> 
>     Cannot update the ref '%s': %s
> 
> where the second "%s" is replaced with the error from log_ref_write().

Done, thanks.

> > @@ -3317,7 +3322,8 @@ static int commit_ref_update(struct ref_lock *lock,
> >  					      head_sha1, &head_flag);
> >  		if (head_ref && (head_flag & REF_ISSYMREF) &&
> >  		    !strcmp(head_ref, lock->ref_name))
> > -			log_ref_write("HEAD", lock->old_oid.hash, sha1, logmsg);
> > +			log_ref_write("HEAD", lock->old_oid.hash, sha1, logmsg,
> > +				      err);
> 
> Here you don't check for errors from log_ref_write(). So it might or
> might not have written something to err. If it has, is that error
> handled correctly?

That was the case before this patch too. But I guess I might as well
check.

> Previously, the error generated here was "cannot update the ref '%s'."
> But now that you are letting write_ref_to_lockfile() fill err, the error
> message will be something from that function. This might be OK or it
> might not. If you think it is, it would be worth a word or two of
> justification in the commit message.

Will adjust commit message.