On Sat, May 17, 2014 at 7:56 AM, Michael Haggerty <mhagger@xxxxxxxxxxxx> wrote: > On 05/16/2014 07:37 PM, Ronnie Sahlberg wrote: >> 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, in which case a previous and >> unrelated ENOTDIR may cause us to return the wrong error. Clear errno before >> calling any functions if we check errno afterwards. > > If I understand correctly, this is a workaround for some other broken > functions that don't handle errno correctly. Long-term, wouldn't it be > better to fix the other functions? In other words, they should change > errno if an only if they return an error status, no? Yeah, but this patch is gone now. Longer term I think we should get rid of passing errno around. errno is best to only be checked immediately after a failed system call and never else. (otherwise you end up with a hairy forest of save_errno variables.) > > Of course you are under no obligation to fix the universe, so this > change may be an expedient workaround anyway. But if you go this route, > then I think a comment would be helpful to explain the unusual clearing > of errno. > > Michael > >> >> Also skip initializing a static variable to 0. Statics live in .bss and >> are all automatically initialized to 0. >> >> Signed-off-by: Ronnie Sahlberg <sahlberg@xxxxxxxxxx> >> --- >> builtin/fetch.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/builtin/fetch.c b/builtin/fetch.c >> index 55f457c..a93c893 100644 >> --- a/builtin/fetch.c >> +++ b/builtin/fetch.c >> @@ -44,7 +44,7 @@ static struct transport *gtransport; >> static struct transport *gsecondary; >> static const char *submodule_prefix = ""; >> static const char *recurse_submodules_default; >> -static int shown_url = 0; >> +static int shown_url; >> >> static int option_parse_recurse_submodules(const struct option *opt, >> const char *arg, int unset) >> @@ -382,6 +382,8 @@ static int s_update_ref(const char *action, >> if (!rla) >> rla = default_rla.buf; >> snprintf(msg, sizeof(msg), "%s: %s", rla, action); >> + >> + errno = 0; >> lock = lock_any_ref_for_update(ref->name, >> check_old ? ref->old_sha1 : NULL, >> 0, NULL); >> > > > -- > Michael Haggerty > mhagger@xxxxxxxxxxxx > http://softwareswirl.blogspot.com/ -- 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