Ronnie Sahlberg wrote:
> Update repack_without_refs to take an err argument and update it if there
> is a failure. Pass the err variable from ref_transaction_commit to this
> function so that callers can print a meaningful error message if _commit
> fails due to a problem in repack_without_refs.
>
> Add a new function unable_to_lock_message that takes a strbuf argument and
> fills in the reason for the failure.
>
> In commit_packed_refs, make sure that we propagate any errno that
> commit_lock_file might have set back to our caller.
>
> Make sure both commit_packed_refs and lock_file has errno set to a meaningful
> value on error. Update a whole bunch of other places to keep errno from
> beeing clobbered.
This patch is doing several (all nice) things. Are they connected?
Would splitting some of them out into separate patches make sense or
would it be too much fuss to be worth the trouble?
[...]
> --- 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_message(const char *path, int err,
> + struct strbuf *buf);
Introducing a new unable_to_lock_message helper, which has nicer
semantics than unable_to_lock_error and cleans up lockfile.c a little.
[...]
> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -121,7 +121,7 @@ static char *resolve_symlink(char *p, size_t s)
> return p;
> }
>
> -
> +/* Make sure errno contains a meaningful value on error */
> static int lock_file(struct lock_file *lk, const char *path, int flags)
Making errno when returning from lock_file() meaningful, which should
fix
* an existing almost-bug in lock_ref_sha1_basic where it assumes
errno==ENOENT is meaningful and could waste some work on retries
* an existing bug in repack_without_refs where it prints
strerror(errno) and picks advice based on errno, despite errno
potentially being zero and potentially having been clobbered by
that point
To make sure we don't lose these fixes, it would be helpful to also
mention that errno is set in the API documentation for the relevant
functions:
* hold_lock_file_for_update (declared in cache.h)
* lock_packed_refs (declared in refs.h)
[...]
> --- a/refs.c
> +++ b/refs.c
> @@ -1333,8 +1333,10 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea
Making errno when returning from resolve_ref_unsafe() meaningful,
which should fix
* a bug in lock_ref_sha1_basic, where it assumes EISDIR
means it failed due to a directory being in the way
To make sure we don't lose this fix, it would be helpful to change
Michael's understated comment (why wasn't it marked with NEEDSWORK,
btw?)
* errno is sometimes set on errors, but not always.
to describe the new guarantee.
[...]
> @@ -1908,14 +1918,17 @@ static struct ref_lock *verify_lock(struct ref_lock *lock,
Making errno when returning from verify_lock() meaningful, which
should almost but not completely fix
* a bug in "git fetch"'s s_update_ref, which trusts the result of an
errno == ENOTDIR check to detect D/F conflicts
To make sure we don't lose this fix, it would be helpful to also
mention that errno is set in the API documentation for the relevant
functions:
* lock_any_ref_for_update (declared in refs.h), after handling the
check_refname_format case
* lock_ref_sha1_basic
ENOTDIR makes sense as a sign that a file was in the way of a
directory we wanted to create. Should "git fetch" also look for
ENOTEMPTY or EEXIST to catch cases where a directory was in the way
of a file to be created?
> @@ -1928,13 +1941,15 @@ static int remove_empty_directories(const char *file)
Making errno when returning from remove_empty_directories() more
obviously meaningful, which should provide some peace of mind for
people auditing lock_ref_sha1_basic.
[...]
> @@ -2203,11 +2218,16 @@ int lock_packed_refs(int flags)
[...]
> int commit_packed_refs(void)
Making errno when returning from commit_packed_refs() meaningful,
which should fix
* a bug in "git clone" where it prints strerror(errno) based on
errno, despite errno possibly being zero and potentially having
been clobbered by that point
* the same kind of bug in "git pack-refs"
and prepares for repack_without_refs() to get a meaningful
error message when commit_packed_refs() fails without falling into
the same bug.
To make sure we don't lose this fix, it would be helpful to also
mention that errno is set in the API documentation for
commit_packed_refs in refs.h.
[...]
> @@ -2427,12 +2450,12 @@ static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data)
> return 0;
> }
>
> -static int repack_without_refs(const char **refnames, int n)
> +static int repack_without_refs(const char **refnames, int n, struct strbuf *err)
Adding an 'err' argument to repack_without_refs and writing to it on
error.
[...]
> @@ -2723,9 +2755,12 @@ int log_ref_setup(const char *refname, char *logfile, int bufsize)
Making errno when returning from log_ref_setup() meaningful,
presumably as preparation for making errno from log_ref_write()
and write_ref_sha1() meaningful.
To make sure we don't break it by mistake, it would be helpful to also
mention that errno is set in the API documentation for log_ref_setup
in refs.h (or to make that function static --- why is it public?).
[...]
> @@ -2784,8 +2826,19 @@ static int log_ref_write(const char *refname, const unsigned char *old_sha1,
Making errno from log_ref_write() meaningful.
[...]
> @@ -2800,8 +2853,10 @@ int write_ref_sha1(struct ref_lock *lock,
Making errno from write_ref_sha1() meaningful, which should fix
* a bug in "git checkout -b" where it prints strerror(errno)
despite errno possibly being zero or clobbered
* a bug in "git fetch"'s s_update_ref, which trusts the result of an
errno == ENOTDIR check to detect D/F conflicts
To make sure we don't lose these fixes, it would be helpful to also
mention that errno is set in the API documentation for write_ref_sha1
in refs.h.
So this could potentially be multiple patches:
1. Set errno in lock_packed_refs, fixing a bug in repack_without_refs
2. Set errno in resolve_ref_unsafe, fixing a longstanding TODO
3. Set errno in lock_any_ref_for_update, tightening half of the D/F check
4. Set errno in commit_packed_refs, fixing a bug in clone, packed-refs
5. Set errno in log_ref_setup, log_ref_write, and write_ref_sha1,
fixing a bug in 'checkout -b' and the other half of the D/F check
6. Introduce unable_to_lock_message helper
7. Add an 'err' argument to repack_without_refs and write to it.
diff --git i/cache.h w/cache.h
index 8b12aa8..0b844c3 100644
--- i/cache.h
+++ w/cache.h
@@ -562,8 +562,10 @@ extern int unable_to_lock_error(const char *path, int err);
extern void unable_to_lock_message(const char *path, int err,
struct strbuf *buf);
extern NORETURN void unable_to_lock_index_die(const char *path, int err);
+/* Sets errno on error. */
extern int hold_lock_file_for_update(struct lock_file *, const char *path, int);
extern int hold_lock_file_for_append(struct lock_file *, const char *path, int);
+/* Sets errno on error. */
extern int commit_lock_file(struct lock_file *);
extern void update_index_if_able(struct index_state *, struct lock_file *);
@@ -785,6 +787,11 @@ enum sharedrepo {
PERM_EVERYBODY = 0664
};
int git_config_perm(const char *var, const char *value);
+
+/*
+ * Widens permissions on 'path' to respect core.sharedrepository.
+ * Sets errno on error.
+ */
int adjust_shared_perm(const char *path);
/*
@@ -979,7 +986,7 @@ extern int read_ref(const char *refname, unsigned char *sha1);
* NULL. If more than MAXDEPTH recursive symbolic lookups are needed,
* give up and return NULL.
*
- * errno is sometimes set on errors, but not always.
+ * On error, sets errno to indicate what went wrong.
*/
extern const char *resolve_ref_unsafe(const char *ref, unsigned char *sha1, int reading, int *flag);
extern char *resolve_refdup(const char *ref, unsigned char *sha1, int reading, int *flag);
diff --git i/dir.c w/dir.c
index eb6f581..daab7b0 100644
--- i/dir.c
+++ w/dir.c
@@ -1527,7 +1527,8 @@ static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up)
{
DIR *dir;
struct dirent *e;
- int ret = 0, original_len = path->len, len, kept_down = 0;
+ int ret = 0, save_errno = 0;
+ int original_len = path->len, len, kept_down = 0;
int only_empty = (flag & REMOVE_DIR_EMPTY_ONLY);
int keep_toplevel = (flag & REMOVE_DIR_KEEP_TOPLEVEL);
unsigned char submodule_head[20];
@@ -1582,20 +1583,23 @@ static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up)
}
/* path too long, stat fails, or non-directory still exists */
+ save_errno = errno;
ret = -1;
break;
}
closedir(dir);
strbuf_setlen(path, original_len);
- if (!ret && !keep_toplevel && !kept_down)
- ret = (!rmdir(path->buf) || errno == ENOENT) ? 0 : -1;
- else if (kept_up)
+ if (!ret && !keep_toplevel && !kept_down) {
+ if (rmdir(path->buf) && errno != ENOENT)
+ return -1;
+ } else if (kept_up)
/*
* report the uplevel that it is not an error that we
* did not rmdir() our directory.
*/
*kept_up = !ret;
+ errno = save_errno;
return ret;
}
diff --git i/dir.h w/dir.h
index 55e5345..c44afbb 100644
--- i/dir.h
+++ w/dir.h
@@ -185,6 +185,7 @@ extern int is_empty_dir(const char *dir);
extern void setup_standard_excludes(struct dir_struct *dir);
+/* sets errno on error */
#define REMOVE_DIR_EMPTY_ONLY 01
#define REMOVE_DIR_KEEP_NESTED_GIT 02
#define REMOVE_DIR_KEEP_TOPLEVEL 04
diff --git i/refs.c w/refs.c
index 7ec1f32..53d8bc7 100644
--- i/refs.c
+++ w/refs.c
@@ -1295,6 +1295,8 @@ static struct ref_entry *get_packed_ref(const char *refname)
/*
* A loose ref file doesn't exist; check for a packed ref. The
* options are forwarded from resolve_safe_unsafe().
+ *
+ * Sets errno on error.
*/
static const char *handle_missing_loose_ref(const char *refname,
unsigned char *sha1,
@@ -1316,6 +1318,7 @@ static const char *handle_missing_loose_ref(const char *refname,
}
/* The reference is not a packed reference, either. */
if (reading) {
+ errno = ENOENT;
return NULL;
} else {
hashclr(sha1);
@@ -1413,7 +1416,7 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea
int save_errno = errno;
close(fd);
errno = save_errno;
- return NULL;
+ return NULL;
}
close(fd);
while (len && isspace(buffer[len-1]))
@@ -1949,9 +1952,9 @@ static int remove_empty_directories(const char *file)
result = remove_dir_recursively(&path, REMOVE_DIR_EMPTY_ONLY);
save_errno = errno;
- errno = save_errno;
strbuf_release(&path);
+ errno = save_errno;
return result;
}
@@ -2039,6 +2042,7 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log)
return logs_found;
}
+/* Sets errno on error. */
static struct ref_lock *lock_ref_sha1_basic(const char *refname,
const unsigned char *old_sha1,
int flags, int *type_p)
@@ -2152,8 +2156,10 @@ struct ref_lock *lock_any_ref_for_update(const char *refname,
const unsigned char *old_sha1,
int flags, int *type_p)
{
- if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))
+ if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
+ errno = EINVAL;
return NULL;
+ }
return lock_ref_sha1_basic(refname, old_sha1, flags, type_p);
}
@@ -2218,10 +2224,6 @@ int lock_packed_refs(int flags)
return 0;
}
-/*
- * Commit the packed refs changes.
- * On error we must make sure that errno contains a meaningful value.
- */
int commit_packed_refs(void)
{
struct packed_ref_cache *packed_ref_cache =
@@ -2776,7 +2778,7 @@ int log_ref_setup(const char *refname, char *logfile, int bufsize)
logfile);
errno = save_errno;
return -1;
- }
+ }
logfd = open(logfile, oflags, 0666);
}
@@ -2794,6 +2796,7 @@ int log_ref_setup(const char *refname, char *logfile, int bufsize)
return 0;
}
+/* Sets errno on error. */
static int log_ref_write(const char *refname, const unsigned char *old_sha1,
const unsigned char *new_sha1, const char *msg)
{
@@ -2824,7 +2827,9 @@ static int log_ref_write(const char *refname, const unsigned char *old_sha1,
committer);
if (msglen)
len += copy_msg(logrec + len - 1, msg) - 1;
- written = len <= maxlen ? write_in_full(logfd, logrec, len) : -1;
+ if (len > maxlen)
+ die("BUG: log_ref_write buffer not large enough?");
+ written = write_in_full(logfd, logrec, len);
free(logrec);
if (written != len) {
int save_errno = errno;
@@ -2867,7 +2872,7 @@ int write_ref_sha1(struct ref_lock *lock,
lock->ref_name, sha1_to_hex(sha1));
unlock_ref(lock);
errno = EINVAL;
- return -1;
+ return -1;
}
if (o->type != OBJ_COMMIT && is_branch(lock->ref_name)) {
error("Trying to write non-commit object %s to branch %s",
@@ -2889,7 +2894,9 @@ int write_ref_sha1(struct ref_lock *lock,
if (log_ref_write(lock->ref_name, lock->old_sha1, sha1, logmsg) < 0 ||
(strcmp(lock->ref_name, lock->orig_ref_name) &&
log_ref_write(lock->orig_ref_name, lock->old_sha1, sha1, logmsg) < 0)) {
+ int save_errno = errno;
unlock_ref(lock);
+ errno = save_errno;
return -1;
}
if (strcmp(lock->orig_ref_name, "HEAD") != 0) {
@@ -2914,8 +2921,10 @@ int write_ref_sha1(struct ref_lock *lock,
log_ref_write("HEAD", lock->old_sha1, sha1, logmsg);
}
if (commit_ref(lock)) {
+ int save_errno = errno;
error("Couldn't set %s", lock->ref_name);
unlock_ref(lock);
+ errno = save_errno;
return -1;
}
unlock_ref(lock);
diff --git i/refs.h w/refs.h
index 94d4cd4..c734944 100644
--- i/refs.h
+++ w/refs.h
@@ -81,6 +81,7 @@ extern void warn_dangling_symref(FILE *fp, const char *msg_fmt, const char *refn
/*
* Lock the packed-refs file for writing. Flags is passed to
* hold_lock_file_for_update(). Return 0 on success.
+ * On error, sets errno to indicate what went wrong.
*/
extern int lock_packed_refs(int flags);
@@ -96,6 +97,7 @@ extern void add_packed_ref(const char *refname, const unsigned char *sha1);
* Write the current version of the packed refs cache from memory to
* disk. The packed-refs file must already be locked for writing (see
* lock_packed_refs()). Return zero on success.
+ * On error, sets errno to indicate what went wrong.
*/
extern int commit_packed_refs(void);
@@ -135,7 +137,10 @@ extern int peel_ref(const char *refname, unsigned char *sha1);
/** Locks a "refs/" ref returning the lock on success and NULL on failure. **/
extern struct ref_lock *lock_ref_sha1(const char *refname, const unsigned char *old_sha1);
-/** Locks any ref (for 'HEAD' type refs). */
+/**
+ * Locks any ref (for 'HEAD' type refs).
+ * On error, returns NULL and sets errno to describe the error.
+ */
#define REF_NODEREF 0x01
extern struct ref_lock *lock_any_ref_for_update(const char *refname,
const unsigned char *old_sha1,
@@ -144,16 +149,25 @@ extern struct ref_lock *lock_any_ref_for_update(const char *refname,
/** Close the file descriptor owned by a lock and return the status */
extern int close_ref(struct ref_lock *lock);
-/** Close and commit the ref locked by the lock */
+/**
+ * Close and commit the ref locked by the lock.
+ * Sets errno on error.
+ */
extern int commit_ref(struct ref_lock *lock);
/** Release any lock taken but not written. **/
extern void unlock_ref(struct ref_lock *lock);
-/** Writes sha1 into the ref specified by the lock. **/
+/**
+ * Writes sha1 into the ref specified by the lock.
+ * Sets errno on error.
+ */
extern int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1, const char *msg);
-/** Setup reflog before using. **/
+/**
+ * Setup reflog before using.
+ * Sets errno on error.
+ */
int log_ref_setup(const char *refname, char *logfile, int bufsize);
/** Reads log for the value of ref during at_time. **/
--
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