errno is hairy :-(
You probably also want something like this :
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 4603cb6..55e7dd8 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -426,7 +426,7 @@ static int update_local_ref(struct ref *ref,
_("! %-*s %-*s -> %s (can't fetch in current branch
TRANSPORT_SUMMARY(_("[rejected]")),
REFCOL_WIDTH, remote, pretty_ref);
- return 1;
+ return STORE_REF_ERROR_OTHER;
}
if (!is_null_sha1(ref->old_sha1) &&
@@ -513,7 +513,7 @@ static int update_local_ref(struct ref *ref,
TRANSPORT_SUMMARY(_("[rejected]")),
REFCOL_WIDTH, remote, pretty_ref,
_("(non-fast-forward)"));
- return 1;
+ return STORE_REF_ERROR_OTHER;
}
}
To make it more clear that this function returns a specific error and
not just a generic not-zero.
On Fri, May 16, 2014 at 11:33 AM, Jonathan Nieder <jrnieder@xxxxxxxxx> wrote:
> (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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html