On Tue, May 27, 2014 at 5:11 PM, Jonathan Nieder <jrnieder@xxxxxxxxx> wrote: > Hi, > > Comments from http://marc.info/?l=git&m=140079653930751&w=2: > > Ronnie Sahlberg wrote: > >> --- a/cache.h >> +++ b/cache.h >> @@ -559,6 +559,8 @@ struct lock_file { >> #define LOCK_DIE_ON_ERROR 1 >> #define LOCK_NODEREF 2 >> extern int unable_to_lock_error(const char *path, int err); >> +extern void unable_to_lock_strbuf(const char *path, int err, >> + struct strbuf *buf); > > "unable_to_lock_strbuf" could be called "unable_to_lock_message" (which > would make its behavior more obvious IMHO). Renamed. Thanks. > > [...] >> --- a/refs.c >> +++ b/refs.c >> @@ -2208,6 +2208,7 @@ int commit_packed_refs(void) >> struct packed_ref_cache *packed_ref_cache = >> get_packed_ref_cache(&ref_cache); >> int error = 0; >> + int save_errno = 0; > > This is about making errno meaningful when commit_packed_refs returns > an error. Probably its API documentation should say so to make it > obvious when people modify it in the future that they should preserve > that property or audit callers. > > [...] >> @@ -2444,6 +2448,11 @@ static int repack_without_refs(const char **refnames, int n) >> return 0; /* no refname exists in packed refs */ >> >> if (lock_packed_refs(0)) { >> + if (err) { >> + unable_to_lock_strbuf(git_path("packed-refs"), errno, >> + err); >> + return -1; >> + } >> unable_to_lock_error(git_path("packed-refs"), errno); > > Via the new call to unable_to_lock_..., repack_without_refs cares > about errno after a failed call to lock_packed_refs. lock_packed_refs > can only fail in hold_lock_file_for_update. hold_lock_file_for_update > is a thin wrapper around lockfile.c::lock_file. lock_file can error > out because > > strlen(path) >= max_path_len > adjust_shared_perm failed [calls error(), clobbers errno] > open failed > > So lock_file needs a similar kind of fix, and it's probably worth > updating API documentation for these calls to make it clear that their > errno is used (though that's not a new problem since > repack_without_refs already called unable_to_lock_error). Could be a > separate, earlier patch (or a TODO comment in this patch to be > addressed with a later patch) since it's fixing an existing bug. I will add it to my todo list. I think passing of errno around is too fragile and that we should avoid ad-hoc save_errno hacks and implement dedicated return codes to replace errno. We should only inspect errno immediately after a syscall has failed. > > Hope that helps, > Jonathan -- 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