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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html