(cc-ing peff, who may remember this STORE_REF_ERROR_DF_CONFLICT case
from long ago)
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
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.
Functions that fail and lead callers to check errno, based on a quick
search with 'git grep -e 'errno *[!=]=':
fopen
lstat
builtin/apply.c::try_create_file (= mkdir, symlink, or open)
rmdir
open
mkdir
unlink
lock_any_ref_for_update
write_ref_sha1
strtol
kill
odb_pack_keep
opendir
fgets
read
poll
pread
strtoimax
strtoumax
git_parse_int
git_parse_int64
git_parse_ulong
write_in_full
credential-cache.c::send_request
fstat
run_command_v_opt
git.c::run_argv
readlink
resolve_ref_unsafe
hold_lock_file_for_update
unlink_or_warn
rename
execvp
waitpid
execv_git_cmd
execv_shell_cmd
sane_execvp
stat
read_object
git_mkstemp_mode
create_tmpfile
start_command
xwrite
iconv
fast_export_ls
fast_export_ls_rev
delete_ref
lock_any_ref_for_update has failure paths
* check_ref_format [!]
* lock_ref_sha1_basic
lock_ref_sha1_basic has failure paths
* remove_empty_directories
* resolve_ref_unsafe
* safe_create_leading_directories
* verify_lock
remove_empty_directories has one failure path
* remove_dir_recursively
but could be more explicit about the need to preserve errno.
errno from remove_dir_recursively is meaningful.
resolve_ref_unsafe has failure paths
* check_refname_format [!]
* too much recursion [!]
* lstat, readlink, open
* handle_missing_loose_ref
* read_in_full, but errno gets clobbered
* invalid ref [!]
* invalid symref [!]
verify_lock has failure paths
* read_ref_full, but errno gets clobbered
* old_sha1 didn't match [!]
write_ref_sha1 has failure paths
* !lock [!]
* missing object [!]
* non-commit object [!]
* write_in_full, close_ref, but errno gets clobbered
* log_ref_write
* commit_ref
log_ref_write has failure paths
* log_ref_setup
* write_in_full, close. errno gets clobbered
commit_ref just calls commit_lock_file.
log_ref_setup has failure paths
* safe_create_leading_directories, but errno gets clobbered
* remove_empty_directories, but errno gets clobbered
* open, but errno gets clobbered
safe_create_leading_directories doesn't set errno for SCLD_EXISTS
but otherwise its errno is useful.
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?
It's also tempting to teach vreportf and vwritef to save errno, which
would handle some of these cases automatically.
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;
+ }
for (;;) {
char path[PATH_MAX];
@@ -1342,8 +1344,10 @@ const char *resolve_ref_unsafe(const char *refname,
unsigned char *sha1, int rea
char *buf;
int fd;
- if (--depth < 0)
+ if (--depth < 0) {
+ errno = ELOOP;
return NULL;
+ }
git_snpath(path, sizeof(path), "%s", refname);
@@ -1405,9 +1409,13 @@ const char *resolve_ref_unsafe(const char *refname,
unsigned char *sha1, int rea
return NULL;
}
len = read_in_full(fd, buffer, sizeof(buffer)-1);
- close(fd);
- if (len < 0)
+ if (len < 0) {
+ int save_errno = errno;
+ close(fd);
+ errno = save_errno;
return NULL;
+ }
+ close(fd);
while (len && isspace(buffer[len-1]))
len--;
buffer[len] = '\0';
@@ -1424,6 +1432,7 @@ const char *resolve_ref_unsafe(const char *refname,
unsigned char *sha1, int rea
(buffer[40] != '\0' && !isspace(buffer[40]))) {
if (flag)
*flag |= REF_ISBROKEN;
+ errno = EINVAL;
return NULL;
}
return refname;
@@ -1436,6 +1445,7 @@ const char *resolve_ref_unsafe(const char *refname,
unsigned char *sha1, int rea
if (check_refname_format(buf, REFNAME_ALLOW_ONELEVEL)) {
if (flag)
*flag |= REF_ISBROKEN;
+ errno = EINVAL;
return NULL;
}
refname = strcpy(refname_buffer, buf);
@@ -1908,14 +1918,17 @@ static struct ref_lock *verify_lock(struct ref_lock
*lock,
const unsigned char *old_sha1, int mustexist)
{
if (read_ref_full(lock->ref_name, lock->old_sha1, mustexist, NULL)) {
+ int save_errno = errno;
error("Can't verify ref %s", lock->ref_name);
unlock_ref(lock);
+ errno = save_errno;
return NULL;
}
if (hashcmp(lock->old_sha1, old_sha1)) {
error("Ref %s is at %s but expected %s", lock->ref_name,
sha1_to_hex(lock->old_sha1), sha1_to_hex(old_sha1));
unlock_ref(lock);
+ errno = EBUSY;
return NULL;
}
return lock;
@@ -1928,15 +1941,17 @@ static int remove_empty_directories(const char *file)
* only empty directories), remove them.
*/
struct strbuf path;
- int result;
+ int result, save_errno;
strbuf_init(&path, 20);
strbuf_addstr(&path, file);
result = remove_dir_recursively(&path, REMOVE_DIR_EMPTY_ONLY);
+ save_errno = errno;
strbuf_release(&path);
+ errno = save_errno;
return result;
}
@@ -2137,8 +2152,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);
}
@@ -2734,9 +2751,12 @@ int log_ref_setup(const char *refname, char *logfile,
int bufsize)
starts_with(refname, "refs/remotes/") ||
starts_with(refname, "refs/notes/") ||
!strcmp(refname, "HEAD"))) {
- if (safe_create_leading_directories(logfile) < 0)
- return error("unable to create directory for %s",
- logfile);
+ if (safe_create_leading_directories(logfile) < 0) {
+ int save_errno = errno;
+ error("unable to create directory for %s", logfile);
+ errno = save_errno;
+ return -1;
+ }
oflags |= O_CREAT;
}
@@ -2747,15 +2767,21 @@ int log_ref_setup(const char *refname, char *logfile,
int bufsize)
if ((oflags & O_CREAT) && errno == EISDIR) {
if (remove_empty_directories(logfile)) {
- return error("There are still logs under '%s'",
- logfile);
+ int save_errno = errno;
+ error("There are still logs under '%s'",
logfile);
+ errno = save_errno;
+ return -1;
}
logfd = open(logfile, oflags, 0666);
}
- if (logfd < 0)
- return error("Unable to append to %s: %s",
+ if (logfd < 0) {
+ int save_errno = errno;
+ error("Unable to append to %s: %s",
logfile, strerror(errno));
+ errno = save_errno;
+ return -1;
+ }
}
adjust_shared_perm(logfile);
@@ -2795,8 +2821,19 @@ static int log_ref_write(const char *refname, const
unsigned char *old_sha1,
len += copy_msg(logrec + len - 1, msg) - 1;
written = len <= maxlen ? write_in_full(logfd, logrec, len) : -1;
free(logrec);
- if (close(logfd) != 0 || written != len)
- return error("Unable to append to %s", log_file);
+ if (written != len) {
+ int save_errno = errno;
+ close(logfd);
+ error("Unable to append to %s", log_file);
+ errno = save_errno;
+ return -1;
+ }
+ if (close(logfd)) {
+ int save_errno = errno;
+ error("Unable to append to %s", log_file);
+ errno = save_errno;
+ return -1;
+ }
return 0;
}
@@ -2811,8 +2848,10 @@ int write_ref_sha1(struct ref_lock *lock,
static char term = '\n';
struct object *o;
- if (!lock)
+ if (!lock) {
+ errno = EINVAL;
return -1;
+ }
if (!lock->force_write && !hashcmp(lock->old_sha1, sha1)) {
unlock_ref(lock);
return 0;
@@ -2822,26 +2861,32 @@ int write_ref_sha1(struct ref_lock *lock,
error("Trying to write ref %s with nonexistent object %s",
lock->ref_name, sha1_to_hex(sha1));
unlock_ref(lock);
+ errno = EINVAL;
return -1;
}
if (o->type != OBJ_COMMIT && is_branch(lock->ref_name)) {
error("Trying to write non-commit object %s to branch %s",
sha1_to_hex(sha1), lock->ref_name);
unlock_ref(lock);
+ errno = EINVAL;
return -1;
}
if (write_in_full(lock->lock_fd, sha1_to_hex(sha1), 40) != 40 ||
- write_in_full(lock->lock_fd, &term, 1) != 1
- || close_ref(lock) < 0) {
+ write_in_full(lock->lock_fd, &term, 1) != 1 ||
+ close_ref(lock) < 0) {
+ int save_errno = errno;
error("Couldn't write %s", lock->lk->filename);
unlock_ref(lock);
+ errno = save_errno;
return -1;
}
clear_loose_ref_cache(&ref_cache);
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) {
@@ -2866,8 +2911,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/sha1_file.c w/sha1_file.c
index 3e9f55f..a7bbba7 100644
--- i/sha1_file.c
+++ w/sha1_file.c
@@ -136,8 +136,10 @@ enum scld_error safe_create_leading_directories(char *path)
*slash = '\0';
if (!stat(path, &st)) {
/* path exists */
- if (!S_ISDIR(st.st_mode))
+ if (!S_ISDIR(st.st_mode)) {
+ errno = EEXIST;
ret = SCLD_EXISTS;
+ }
} else if (mkdir(path, 0777)) {
if (errno == EEXIST &&
!stat(path, &st) && S_ISDIR(st.st_mode))
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
