On Fri, May 16, 2014 at 11:33:48AM -0700, Jonathan Nieder wrote:
> (cc-ing peff, who may remember this STORE_REF_ERROR_DF_CONFLICT case
> from long ago)
No, but I have very good tools for searching the list archive. ;)
> > In s_update_ref there are two calls that when they fail we return an error
> > based on the errno value. In particular we want to return a specific error
> > if ENOTDIR happened. Both these functions do have failure modes where they
> > may return an error without updating errno
>
> That's a bug. Any function for which errno is supposed to be
> meaningful when it returns an error should always set errno.
> Otherwise errno may be set to ENOTDIR within the function by an
> unrelated system call.
I'd agree. It's OK for a function not to set errno if it is
_successful_. But if setting errno on error is part of the interface for
lock_any_ref_for_update, it should do so consistently.
We have not been very good about documenting which functions use errno,
or using it consistently (e.g., in my original patch, we are explicitly
converting errno into a numeric return value!), so I can definitely see
how it is confusing.
> That should cover the cases affecting the code path in fetch.c you
> mentioned (I haven't checked the others).
>
> How about something like this?
> [...]
Yeah, I think this is a better direction.
> diff --git i/refs.c w/refs.c
> index 82a8d4e..f98acf0 100644
> --- i/refs.c
> +++ w/refs.c
> @@ -1333,8 +1333,10 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea
> if (flag)
> *flag = 0;
>
> - if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))
> + if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
> + errno = EINVAL;
> return NULL;
> + }
check_refname_format comes up in a few places. I wonder if it should set
EINVAL itself.
Other than that, I just skimmed through your list. All looked
reasonable to me, though I was not thorough enough to be sure we got
call cases (OTOH, we can add them later on top).
-Peff
--
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