Re: [PATCH v11 06/41] refs.c: add an err argument to repack_without_refs
From: Jonathan Nieder <hidden>
Date: 2016-06-15 23:01:24
Hi, Comments from http://marc.info/?l=git&m=140079653930751&w=2: Ronnie Sahlberg wrote:
quoted hunk ↗ jump to hunk
--- 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). [...]
quoted hunk ↗ jump to hunk
--- 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. [...]
quoted hunk ↗ jump to hunk
@@ -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. Hope that helps, Jonathan