refs: cleanup errno sideband ref related functions
PATCH v8 6/7 refs: explicitly return failure_errno from parse_loose_ref_contents
In reply to Ævar Arnfjörð BjarmasonFrom: Han-Wen Nienhuys <[email protected]>
The EINVAL error from parse_loose_ref_contents is used in files-backend to create a custom error message.
In untangling this we discovered a tricky edge case. The refs_read_special_head() function was relying on parse_loose_ref_contents() setting EINVAL.
By converting it to use "saved_errno" we can migrate away from "errno" in this part of the code entirely, and do away with an existing "save_errno" pattern, its only purpose was to not clobber the "errno" we previously needed at the end of files_read_raw_ref().
Let's assert that we can do that by not having files_read_raw_ref() itself operate on *failure_errno in addition to passing it on. Instead we'll assert that if we return non-zero we actually do set errno, thus assuring ourselves and callers that they can trust the resulting "failure_errno".
--- refs.c | 8 +++++--- refs/files-backend.c | 30 +++++++++++++++++++----------- refs/refs-internal.h | 6 ++++-- 3 files changed, 28 insertions(+), 16 deletions(-)
refs.c +5 -3
@@ -1654,7 +1654,8 @@ int for_each_fullref_in_prefixes(const char *namespace,static int refs_read_special_head (struct ref_store *ref_store,const char *refname,struct object_id *oid, -struct strbuf *referent,unsigned int *type) +struct strbuf *referent,unsigned int *type, +int *failure_errno) {struct strbuf full_path =STRBUF_INIT ;struct strbuf content =STRBUF_INIT ;
@@ -1664,7 +1665,8 @@ static int refs_read_special_head(struct ref_store *ref_store,if (strbuf_read_file (&content, full_path.buf,0 ) <0 ) goto done; - result =parse_loose_ref_contents (content.buf, oid, referent, type); + result =parse_loose_ref_contents (content.buf, oid, referent, type, + failure_errno); done:strbuf_release (&full_path);
@@ -1679,7 +1681,7 @@ int refs_read_raw_ref(struct ref_store *ref_store, const char *refname,assert (failure_errno);if (!strcmp (refname,"FETCH_HEAD" ) || !strcmp (refname,"MERGE_HEAD" )) {return refs_read_special_head (ref_store, refname, oid, referent, - type); + type, failure_errno); } return ref_store->be->read_raw_ref(ref_store, refname, oid, referent,
refs/files-backend.c +19 -11
@@ -355,6 +355,7 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,int fd;int ret =-1 ;int remaining_retries =3 ; +int myerr =0 ; *type =0 ;strbuf_reset (&sb_path);
@@ -382,11 +383,13 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,if (lstat (path, &st) <0 ) {int ignore_errno; -if (errno !=ENOENT ) + myerr = errno; + errno =0 ; +if (myerr !=ENOENT ) goto out;if (refs_read_raw_ref (refs->packed_ref_store, refname, oid, referent, type, &ignore_errno)) { - errno =ENOENT ; + myerr =ENOENT ; goto out; } ret =0 ;
@@ -397,7 +400,9 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,if (S_ISLNK (st.st_mode)) {strbuf_reset (&sb_contents);if (strbuf_readlink (&sb_contents, path, st.st_size) <0 ) { -if (errno ==ENOENT || errno ==EINVAL ) + myerr = errno; + errno =0 ; +if (myerr ==ENOENT || myerr ==EINVAL )/* inconsistent with lstat; retry */ goto stat_ref;else
@@ -427,7 +432,7 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,*/ if (refs_read_raw_ref (refs->packed_ref_store, refname, oid, referent, type, &ignore_errno)) { - errno =EISDIR ; + myerr =EISDIR ; goto out; } ret =0 ;
@@ -440,7 +445,8 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,*/ fd =open (path,O_RDONLY );if (fd <0 ) { -if (errno ==ENOENT && !S_ISLNK (st.st_mode)) + myerr = errno; +if (myerr ==ENOENT && !S_ISLNK (st.st_mode))/* inconsistent with lstat; retry */ goto stat_ref;else
@@ -448,26 +454,28 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname, }strbuf_reset (&sb_contents);if (strbuf_read (&sb_contents, fd,256 ) <0 ) { -int save_errno = errno;close (fd); - errno = save_errno; goto out; }close (fd);strbuf_rtrim (&sb_contents); buf = sb_contents.buf; - ret =parse_loose_ref_contents (buf, oid, referent, type); + ret =parse_loose_ref_contents (buf, oid, referent, type, &myerr); out: - *failure_errno = errno; +if (ret && !myerr) +BUG ("returning non-zero %d, should have set myerr!" , ret); + *failure_errno = myerr; +strbuf_release (&sb_path);strbuf_release (&sb_contents);return ret; }int parse_loose_ref_contents (const char *buf,struct object_id *oid, -struct strbuf *referent,unsigned int *type) +struct strbuf *referent,unsigned int *type, +int *failure_errno) {const char *p;if (skip_prefix (buf,"ref:" , &buf)) {
@@ -486,7 +494,7 @@ int parse_loose_ref_contents(const char *buf, struct object_id *oid,if (parse_oid_hex (buf, oid, &p) || (*p !='\0' && !isspace (*p))) { *type |=REF_ISBROKEN ; - errno =EINVAL ; + *failure_errno =EINVAL ;return -1 ; }return 0 ;
refs/refs-internal.h +4 -2
@@ -689,10 +689,12 @@ struct ref_store { };/* -* Parse contents of a loose ref file. +* Parse contents of a loose ref file. *failure_errno maybe be set to EINVAL for +* invalid contents. */ int parse_loose_ref_contents (const char *buf,struct object_id *oid, -struct strbuf *referent,unsigned int *type); +struct strbuf *referent,unsigned int *type, +int *failure_errno);/* * Fill in the generic part of refs and add it to our collection of
Signature · 1 line
2.32.0.874.gfa1990a4f10
PATCH v8 7/7 refs: make errno output explicit for refs_resolve_ref_unsafe
In reply to Ævar Arnfjörð BjarmasonFrom: Han-Wen Nienhuys <[email protected]>
This introduces refs_resolve_ref_unsafe_with_errno(), which makes the API contract for the errno output explicit. The implementation still relies on the global errno variable to ensure no side effects of this refactoring.
lock_ref_oid_basic() in files-backend.c is the only caller of refs_resolve_ref() that needs error information to make logic decisions, so update that caller
--- refs.c | 2 +- refs.h | 11 +++++++++++ refs/files-backend.c | 12 +++++++----- 3 files changed, 19 insertions(+), 6 deletions(-)
refs.c +1 -1
@@ -1688,7 +1688,7 @@ int refs_read_raw_ref(struct ref_store *ref_store, const char *refname, type, failure_errno); } -static const char *refs_resolve_ref_unsafe_with_errno(struct ref_store *refs, +const char *refs_resolve_ref_unsafe_with_errno(struct ref_store *refs, const char *refname, int resolve_flags, struct object_id *oid,
refs.h +11 -0
@@ -68,6 +68,17 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,int resolve_flags, struct object_id *oid, int *flags); +/** +* refs_resolve_ref_unsafe_with_errno() is like +* refs_resolve_ref_unsafe(), but provide access to errno code that +* lead to a failure. We guarantee that errno is set to a meaningful +* value on non-zero return. +*/ +const char *refs_resolve_ref_unsafe_with_errno (struct ref_store *refs, +const char *refname, +int resolve_flags, +struct object_id *oid, +int *flags,int *failure_errno);const char *resolve_ref_unsafe (const char *refname,int resolve_flags,struct object_id *oid,int *flags);
refs/files-backend.c +7 -5
@@ -1001,6 +1001,7 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, {struct strbuf ref_file =STRBUF_INIT ;struct ref_lock *lock; +int resolve_errno =0 ;files_assert_main_repository (refs,"lock_ref_oid_basic" );assert (err);
@@ -1008,17 +1009,18 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,CALLOC_ARRAY (lock,1 );files_ref_path (refs, &ref_file, refname); -if (!refs_resolve_ref_unsafe (&refs->base, refname, -RESOLVE_REF_NO_RECURSE , - &lock->old_oid, type) && - (errno !=ENOTDIR || +if (!refs_resolve_ref_unsafe_with_errno (&refs->base, refname, +RESOLVE_REF_NO_RECURSE , + &lock->old_oid, type, + &resolve_errno) && + (resolve_errno !=ENOTDIR ||/* in case of D/F conflict, try to generate a better error * message. If that fails, fall back to strerror(ENOTDIR). */ !refs_verify_refname_available (&refs->base, refname,NULL ,NULL , err))) {strbuf_addf (err,"unable to resolve reference '%s': %s" , - refname,strerror (errno)); + refname,strerror (resolve_errno)); goto error_return; }
Signature · 1 line
2.32.0.874.gfa1990a4f10
v9 cover refs: cleanup errno sideband ref related functions
In reply to Ævar Arnfjörð BjarmasonA v9 re-roll of the v8, see
https://lore.kernel.org/git/[email protected]/
and more imporantly the real summary of what this is in v7 at
https://lore.kernel.org/git/[email protected]/
As noted there this topic builds on my just-re-rolled v3 removing of dead code + fixing a gc race in the refs API, at:
https://lore.kernel.org/git/[email protected]/
The changes in this v8 are merely to re-roll on top of that. There was a stray comment that didn't apply anymore with its new 12th patch. I also removed the "in case of D/F conflict..." comment the 3rd patch previously added, with the changes in the base topic I think this codepath has become easily understood enough to not need an new comment anymore (the initial version was more complex, see
https://lore.kernel.org/git/95025080c16f535599826ed4f013845d712b0e8d.1625684869.git.gitgitgadget@gmail.com/).
Han-Wen Nienhuys (6):
refs: remove EINVAL errno output from specification of read_raw_ref_fn refs/files-backend: stop setting errno from lock_ref_oid_basic
refs: make errno output explicit for read_raw_ref_fn
refs: add failure_errno to refs_read_raw_ref() signature
refs: explicitly return failure_errno from parse_loose_ref_contents
refs: make errno output explicit for refs_resolve_ref_unsafe
Ævar Arnfjörð Bjarmason (1):
refs file backend: move raceproof_create_file() here
cache.h | 43 ---------- object-file.c | 68 --------------- refs.c | 69 ++++++++++----- refs.h | 11 +++ refs/debug.c | 4 +- refs/files-backend.c | 195 +++++++++++++++++++++++++++++++++--------- refs/packed-backend.c | 15 ++-- refs/refs-internal.h | 32 ++++--- 8 files changed, 243 insertions(+), 194 deletions(-)
Range-diff against v8:
1: ce1ca2cf30f = 1: b7063c5af89 refs file backend: move raceproof_create_file() here
2: 2a69bbea821 = 2: 5a63b64f53f refs: remove EINVAL errno output from specification of read_raw_ref_fn
3: a3f80c6d2f7 ! 3: 0dd8a4c1209 refs/files-backend: stop setting errno from lock_ref_oid_basic
@@ Commit message
refs_reflog_exists() (which calls a function in a vtable that is not
documented to use and/or preserve errno)
- In the case of the "errno != ENOTDIR" case that originates in 5b2d8d6f218
- (lock_ref_sha1_basic(): improve diagnostics for ref D/F conflicts,
- 2015-05-11), there the "last_errno" was saved away to return it from
- lock_ref_oid_basic(), now that we're no longer doing that we can skip
- that entirely and use "errno" directly. A follow-up change will
- extract the specific errno we want earlier in this function.
-
Signed-off-by: Han-Wen Nienhuys <[email protected]>
Signed-off-by: Ævar Arnfjörð Bjarmason <[email protected]>
@@ refs/files-backend.c: static struct ref_lock *lock_ref_oid_basic(struct files_re
files_assert_main_repository(refs, "lock_ref_oid_basic");
assert(err);
@@ refs/files-backend.c: static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
- files_ref_path(refs, &ref_file, refname);
if (!refs_resolve_ref_unsafe(&refs->base, refname,
RESOLVE_REF_NO_RECURSE,
-- &lock->old_oid, type)) {
+ &lock->old_oid, type)) {
- last_errno = errno;
-- if (last_errno != ENOTDIR ||
-- !refs_verify_refname_available(&refs->base, refname,
-- NULL, NULL, err))
-- strbuf_addf(err, "unable to resolve reference '%s': %s",
+ if (!refs_verify_refname_available(&refs->base, refname,
+ NULL, NULL, err))
+ strbuf_addf(err, "unable to resolve reference '%s': %s",
- refname, strerror(last_errno));
--
-+ &lock->old_oid, type) &&
-+ (errno != ENOTDIR ||
-+ /* in case of D/F conflict, try to generate a better error
-+ * message. If that fails, fall back to strerror(ENOTDIR).
-+ */
-+ !refs_verify_refname_available(&refs->base, refname, NULL,
-+ NULL, err))) {
-+ strbuf_addf(err, "unable to resolve reference '%s': %s",
-+ refname, strerror(errno));
++ refname, strerror(errno));
+
goto error_return;
}
-
@@ refs/files-backend.c: static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
*/
if (is_null_oid(&lock->old_oid) &&
4: 147058c8c3d = 4: c54fb99714a refs: make errno output explicit for read_raw_ref_fn
5: b42a7474f18 = 5: 18825efce7d refs: add failure_errno to refs_read_raw_ref() signature
6: 93b770c8bea = 6: 57e3f246f4f refs: explicitly return failure_errno from parse_loose_ref_contents
7: cb32b5c0526 ! 7: 4b5e168b978 refs: make errno output explicit for refs_resolve_ref_unsafe
@@ refs/files-backend.c: static struct ref_lock *lock_ref_oid_basic(struct files_re
files_ref_path(refs, &ref_file, refname);
- if (!refs_resolve_ref_unsafe(&refs->base, refname,
- RESOLVE_REF_NO_RECURSE,
-- &lock->old_oid, type) &&
-- (errno != ENOTDIR ||
+- &lock->old_oid, type)) {
+ if (!refs_resolve_ref_unsafe_with_errno(&refs->base, refname,
+ RESOLVE_REF_NO_RECURSE,
+ &lock->old_oid, type,
-+ &resolve_errno) &&
-+ (resolve_errno != ENOTDIR ||
- /* in case of D/F conflict, try to generate a better error
- * message. If that fails, fall back to strerror(ENOTDIR).
- */
- !refs_verify_refname_available(&refs->base, refname, NULL,
- NULL, err))) {
- strbuf_addf(err, "unable to resolve reference '%s': %s",
-- refname, strerror(errno));
-+ refname, strerror(resolve_errno));
++ &resolve_errno)) {
+ if (!refs_verify_refname_available(&refs->base, refname,
+ NULL, NULL, err))
+ strbuf_addf(err, "unable to resolve reference '%s': %s",
+- refname, strerror(errno));
++ refname, strerror(resolve_errno));
+
goto error_return;
}
- Signature · 1 line
2.32.0.874.ge7a9d58bfcf
PATCH v9 1/7 refs file backend: move raceproof_create_file() here
Move the raceproof_create_file() API added to cache.h and object-file.c in 177978f56ad (raceproof_create_file(): new function, 2017-01-06) to its only user, refs/files-backend.c.
--- cache.h | 43 ----------------- object-file.c | 68 --------------------------- refs/files-backend.c | 109 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 109 insertions(+), 111 deletions(-)
cache.h +0 -43
@@ -1202,49 +1202,6 @@ enum scld_error safe_create_leading_directories(char *path);enum scld_error safe_create_leading_directories_const (const char *path);enum scld_error safe_create_leading_directories_no_share (char *path); -/* -* Callback function for raceproof_create_file(). This function is -* expected to do something that makes dirname(path) permanent despite -* the fact that other processes might be cleaning up empty -* directories at the same time. Usually it will create a file named -* path, but alternatively it could create another file in that -* directory, or even chdir() into that directory. The function should -* return 0 if the action was completed successfully. On error, it -* should return a nonzero result and set errno. -* raceproof_create_file() treats two errno values specially: -* -* - ENOENT -- dirname(path) does not exist. In this case, -* raceproof_create_file() tries creating dirname(path) -* (and any parent directories, if necessary) and calls -* the function again. -* -* - EISDIR -- the file already exists and is a directory. In this -* case, raceproof_create_file() removes the directory if -* it is empty (and recursively any empty directories that -* it contains) and calls the function again. -* -* Any other errno causes raceproof_create_file() to fail with the -* callback's return value and errno. -* -* Obviously, this function should be OK with being called again if it -* fails with ENOENT or EISDIR. In other scenarios it will not be -* called again. -*/ -typedef int create_file_fn (const char *path,void *cb); - -/* -* Create a file in dirname(path) by calling fn, creating leading -* directories if necessary. Retry a few times in case we are racing -* with another process that is trying to clean up the directory that -* contains path. See the documentation for create_file_fn for more -* details. -* -* Return the value and set the errno that resulted from the most -* recent call of fn. fn is always called at least once, and will be -* called more than once if it returns ENOENT or EISDIR. -*/ -int raceproof_create_file (const char *path,create_file_fn fn,void *cb); -int mkdir_in_gitdir (const char *path);char *expand_user_path (const char *path,int real_home);const char *enter_repo (const char *path,int strict);
object-file.c +0 -68
74 lines not on this page · raw message
refs/files-backend.c +109 -0
115 lines not on this page · raw message
Signature · 1 line
2.32.0.874.ge7a9d58bfcf
PATCH v9 2/7 refs: remove EINVAL errno output from specification of read_raw_ref_fn
In reply to Ævar Arnfjörð BjarmasonFrom: Han-Wen Nienhuys <[email protected]>
This commit does not change code; it documents the fact that an alternate ref backend does not need to return EINVAL from read_raw_ref_fn to function properly.
This is correct, because refs_read_raw_ref is only called from;
* resolve_ref_unsafe(), which does not care for the EINVAL errno result.
* refs_verify_refname_available(), which does not inspect errno.
* files-backend.c, where errno is overwritten on failure.
* packed-backend.c (is_packed_transaction_needed), which calls it for the packed ref backend, which never emits EINVAL.
A grep for EINVAL */*c reveals that no code checks errno against EINVAL after reading references. In addition, the refs.h file does not mention errno at all.
A grep over resolve_ref_unsafe() turned up the following callers that inspect errno:
* sequencer.c::print_commit_summary, which uses it for die_errno
* lock_ref_oid_basic(), which only treats EISDIR and ENOTDIR specially.
The files ref backend does use EINVAL. The files backend does not call into the generic API (refs_read_raw), but into the files-specific function (files_read_raw_ref), which we are not changing in this commit.
As the errno sideband is unintuitive and error-prone, remove EINVAL value, as a step towards getting rid of the errno sideband altogether.
Spotted by Ævar Arnfjörð Bjarmason <[email protected]>.
--- refs/refs-internal.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
refs/refs-internal.h +3 -3
@@ -622,9 +622,9 @@ typedef int reflog_expire_fn(struct ref_store *ref_store,* * Return 0 on success. If the ref doesn't exist, set errno to ENOENT * and return -1. If the ref exists but is neither a symbolic ref nor -* an object ID, it is broken; set REF_ISBROKEN in type, set errno to -* EINVAL, and return -1. If there is another error reading the ref, -* set errno appropriately and return -1. +* an object ID, it is broken; set REF_ISBROKEN in type, and return -1 +* (errno should not be ENOENT) If there is another error reading the +* ref, set errno appropriately and return -1. * * Backend-specific flags might be set in type as well, regardless of * outcome.
Signature · 1 line
2.32.0.874.ge7a9d58bfcf
PATCH v9 3/7 refs/files-backend: stop setting errno from lock_ref_oid_basic
In reply to Ævar Arnfjörð BjarmasonFrom: Han-Wen Nienhuys <[email protected]>
refs/files-backend.c::lock_ref_oid_basic() tries to signal how it failed to its callers using errno.
It is safe to stop setting errno here, because the callers of this file-scope static function are
* files_copy_or_rename_ref()
* files_create_symref()
* files_reflog_expire()
None of them looks at errno after seeing a negative return from lock_ref_oid_basic() to make any decision, and no caller of these three functions looks at errno after they signal a failure by returning a negative value. In particular,
* files_copy_or_rename_ref() - here, calls are followed by error() (which performs I/O) or write_ref_to_lockfile() (which calls parse_object() which may perform I/O)
* files_create_symref() - here, calls are followed by error() or create_symref_locked() (which performs I/O and does not inspect errno)
* files_reflog_expire() - here, calls are followed by error() or refs_reflog_exists() (which calls a function in a vtable that is not documented to use and/or preserve errno)
--- refs/files-backend.c | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-)
refs/files-backend.c +2 -9
@@ -982,7 +982,6 @@ static int create_reflock(const char *path, void *cb)/* * Locks a ref returning the lock on success and NULL on failure. -* On failure errno is set to something meaningful. */ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, const char *refname,
@@ -991,7 +990,6 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, {struct strbuf ref_file =STRBUF_INIT ;struct ref_lock *lock; -int last_errno =0 ;files_assert_main_repository (refs,"lock_ref_oid_basic" );assert (err);
@@ -1002,11 +1000,10 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,if (!refs_resolve_ref_unsafe (&refs->base, refname,RESOLVE_REF_NO_RECURSE , &lock->old_oid, type)) { - last_errno = errno;if (!refs_verify_refname_available (&refs->base, refname,NULL ,NULL , err))strbuf_addf (err,"unable to resolve reference '%s': %s" , - refname,strerror (last_errno)); + refname,strerror (errno)); goto error_return; }
@@ -1019,15 +1016,12 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,*/ if (is_null_oid (&lock->old_oid) &&refs_verify_refname_available (refs->packed_ref_store, refname, -NULL ,NULL , err)) { - last_errno =ENOTDIR ; +NULL ,NULL , err)) goto error_return; - } lock->ref_name =xstrdup (refname);if (raceproof_create_file (ref_file.buf, create_reflock, &lock->lk)) { - last_errno = errno;unable_to_lock_message (ref_file.buf, errno, err); goto error_return; }
@@ -1044,7 +1038,6 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, out:strbuf_release (&ref_file); - errno = last_errno;return lock; }
Signature · 1 line
2.32.0.874.ge7a9d58bfcf
PATCH v9 4/7 refs: make errno output explicit for read_raw_ref_fn
In reply to Ævar Arnfjörð BjarmasonFrom: Han-Wen Nienhuys <[email protected]>
This makes it explicit how alternative ref backends should report errors in read_raw_ref_fn.
read_raw_ref_fn needs to supply a credible errno for a number of cases. These are primarily:
1) The files backend calls read_raw_ref from lock_raw_ref, and uses the resulting error codes to create/remove directories as needed.
2) ENOENT should be translated in a zero OID, optionally with REF_ISBROKEN set, returning the last successfully resolved symref. This is necessary so read_raw_ref("HEAD") on an empty repo returns refs/heads/main (or the default branch du-jour), and we know on which branch to create the first commit.
Make this information flow explicit by adding a failure_errno to the signature of read_raw_ref. All errnos from the files backend are still propagated unchanged, even though inspection suggests only ENOTDIR, EISDIR and ENOENT are relevant.
--- refs.c | 2 +- refs/debug.c | 4 ++-- refs/files-backend.c | 29 +++++++++++++++-------------- refs/packed-backend.c | 8 ++++---- refs/refs-internal.h | 20 ++++++++++++-------- 5 files changed, 34 insertions(+), 29 deletions(-)
refs.c +1 -1
@@ -1682,7 +1682,7 @@ int refs_read_raw_ref(struct ref_store *ref_store, }return ref_store->be->read_raw_ref (ref_store, refname, oid, referent, - type); + type, &errno); }/* This function needs to return a meaningful errno on failure */
refs/debug.c +2 -2
@@ -238,7 +238,7 @@ debug_ref_iterator_begin(struct ref_store *ref_store, const char *prefix,static int debug_read_raw_ref (struct ref_store *ref_store,const char *refname,struct object_id *oid,struct strbuf *referent, -unsigned int *type) +unsigned int *type,int *failure_errno) {struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;int res =0 ;
@@ -246,7 +246,7 @@ static int debug_read_raw_ref(struct ref_store *ref_store, const char *refname,oidcpy (oid,null_oid ()); errno =0 ; res = drefs->refs->be->read_raw_ref (drefs->refs, refname, oid, referent, - type); + type, failure_errno);if (res ==0 ) { trace_printf_key(&trace_refs, "read_raw_ref: %s: %s (=> %s) type %x: %d\n",
refs/files-backend.c +15 -14
@@ -341,9 +341,9 @@ static struct ref_cache *get_loose_ref_cache(struct files_ref_store *refs)return refs->loose; } -static int files_read_raw_ref (struct ref_store *ref_store, -const char *refname,struct object_id *oid, -struct strbuf *referent,unsigned int *type) +static int files_read_raw_ref (struct ref_store *ref_store,const char *refname, +struct object_id *oid,struct strbuf *referent, +unsigned int *type,int *failure_errno) {struct files_ref_store *refs =files_downcast (ref_store,REF_STORE_READ ,"read_raw_ref" );
@@ -354,7 +354,6 @@ static int files_read_raw_ref(struct ref_store *ref_store,struct stat st;int fd;int ret =-1 ; -int save_errno;int remaining_retries =3 ; *type =0 ;
@@ -459,10 +458,9 @@ static int files_read_raw_ref(struct ref_store *ref_store, ret =parse_loose_ref_contents (buf, oid, referent, type); out: - save_errno = errno; + *failure_errno = errno;strbuf_release (&sb_path);strbuf_release (&sb_contents); - errno = save_errno;return ret; }
@@ -540,6 +538,7 @@ static int lock_raw_ref(struct files_ref_store *refs,struct strbuf ref_file =STRBUF_INIT ;int attempts_remaining =3 ;int ret =TRANSACTION_GENERIC_ERROR ; +int failure_errno;assert (err);files_assert_main_repository (refs,"lock_raw_ref" );
@@ -610,7 +609,9 @@ static int lock_raw_ref(struct files_ref_store *refs,if (hold_lock_file_for_update_timeout ( &lock->lk, ref_file.buf,LOCK_NO_DEREF ,get_files_ref_lock_timeout_ms ()) <0 ) { -if (errno ==ENOENT && --attempts_remaining >0 ) { +int myerr = errno; + errno =0 ; +if (myerr ==ENOENT && --attempts_remaining >0 ) {/* * Maybe somebody just deleted one of the * directories leading to ref_file. Try
@@ -618,7 +619,7 @@ static int lock_raw_ref(struct files_ref_store *refs,*/ goto retry; }else { -unable_to_lock_message (ref_file.buf, errno, err); +unable_to_lock_message (ref_file.buf, myerr, err); goto error_return; } }
@@ -628,9 +629,9 @@ static int lock_raw_ref(struct files_ref_store *refs,* fear that its value will change. */ -if (files_read_raw_ref (&refs->base, refname, - &lock->old_oid, referent, type)) { -if (errno ==ENOENT ) { +if (files_read_raw_ref (&refs->base, refname, &lock->old_oid, referent, + type, &failure_errno)) { +if (failure_errno ==ENOENT ) {if (mustexist) {/* Garden variety missing reference. */ strbuf_addf(err, "unable to resolve reference '%s'",
@@ -654,7 +655,7 @@ static int lock_raw_ref(struct files_ref_store *refs,* reference named "refs/foo/bar/baz". */ } - }else if (errno ==EISDIR ) { + }else if (failure_errno ==EISDIR ) {/* * There is a directory in the way. It might have * contained references that have been deleted. If
@@ -692,13 +693,13 @@ static int lock_raw_ref(struct files_ref_store *refs, goto error_return; } } - }else if (errno ==EINVAL && (*type &REF_ISBROKEN )) { + }else if (failure_errno ==EINVAL && (*type &REF_ISBROKEN )) {strbuf_addf (err,"unable to resolve reference '%s': " "reference broken" , refname); goto error_return; }else {strbuf_addf (err,"unable to resolve reference '%s': %s" , - refname,strerror (errno)); + refname,strerror (failure_errno)); goto error_return; }
refs/packed-backend.c +4 -4
@@ -724,9 +724,9 @@ static struct snapshot *get_snapshot(struct packed_ref_store *refs)return refs->snapshot; } -static int packed_read_raw_ref (struct ref_store *ref_store, -const char *refname,struct object_id *oid, -struct strbuf *referent,unsigned int *type) +static int packed_read_raw_ref (struct ref_store *ref_store,const char *refname, +struct object_id *oid,struct strbuf *referent, +unsigned int *type,int *failure_errno) {struct packed_ref_store *refs =packed_downcast (ref_store,REF_STORE_READ ,"read_raw_ref" );
@@ -739,7 +739,7 @@ static int packed_read_raw_ref(struct ref_store *ref_store,if (!rec) {/* refname is not a packed reference. */ - errno =ENOENT ; + *failure_errno =ENOENT ;return -1 ; }
refs/refs-internal.h +12 -8
@@ -620,11 +620,15 @@ typedef int reflog_expire_fn(struct ref_store *ref_store,* properly-formatted or even safe reference name. NEITHER INPUT NOR * OUTPUT REFERENCE NAMES ARE VALIDATED WITHIN THIS FUNCTION. * -* Return 0 on success. If the ref doesn't exist, set errno to ENOENT -* and return -1. If the ref exists but is neither a symbolic ref nor -* an object ID, it is broken; set REF_ISBROKEN in type, and return -1 -* (errno should not be ENOENT) If there is another error reading the -* ref, set errno appropriately and return -1. +* Return 0 on success, or -1 on failure. If the ref exists but is neither a +* symbolic ref nor an object ID, it is broken. In this case set REF_ISBROKEN in +* type, and return -1 (failure_errno should not be ENOENT) +* +* failure_errno provides errno codes that are interpreted beyond error +* reporting. The following error codes have special meaning: +* * ENOENT: the ref doesn't exist +* * EISDIR: ref name is a directory +* * ENOTDIR: ref prefix is not a directory * * Backend-specific flags might be set in type as well, regardless of * outcome.
@@ -638,9 +642,9 @@ typedef int reflog_expire_fn(struct ref_store *ref_store,* - in all other cases, referent will be untouched, and therefore * refname will still be valid and unchanged. */ -typedef int read_raw_ref_fn (struct ref_store *ref_store, -const char *refname,struct object_id *oid, -struct strbuf *referent,unsigned int *type); +typedef int read_raw_ref_fn (struct ref_store *ref_store,const char *refname, +struct object_id *oid,struct strbuf *referent, +unsigned int *type,int *failure_errno);struct ref_storage_be {struct ref_storage_be *next;
Signature · 1 line
2.32.0.874.ge7a9d58bfcf
Re: [PATCH v9 4/7] refs: make errno output explicit for read_raw_ref_fn
refs/debug.c debug_ref_iterator_begin(struct ref_store *ref_store, const char *prefix, · line 241
Quoted 8 lines · Ævar Arnfjörð Bjarmason
refs/debug.c +1
-1@@ -238,7 +238,7 @@ debug_ref_iterator_begin(struct ref_store *ref_store, const char *prefix,static int debug_read_raw_ref (struct ref_store *ref_store,const char *refname,struct object_id *oid,struct strbuf *referent, -unsigned int *type) +unsigned int *type,int *failure_errno)
the code in debug.c needs some tweaking:
refs/debug.c static int debug_read_raw_ref(struct ref_store *ref_store, const char *refname, · line 247
Quoted 6 lines
refs/debug.c +0
-0{struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;int res =0 ;@@ -246,7 +246,7 @@ static int debug_read_raw_ref(struct ref_store *ref_store, const char *refname,oidcpy (oid,null_oid ()); errno =0 ;
this line can go
refs/debug.c static int debug_read_raw_ref(struct ref_store *ref_store, const char *refname, · line 252
Quoted 6 lines
refs/debug.c +1
-1res = drefs->refs->be->read_raw_ref (drefs->refs, refname, oid, referent, - type); + type, failure_errno);if (res ==0 ) { trace_printf_key(&trace_refs, "read_raw_ref: %s: %s (=> %s) type %x: %d\n",
A bit below, failure_errno (rather than errno) should be printed.
Signature · 7 lines
Han-Wen Nienhuys - Google Munich I work 80%. Don't expect answers from me on Fridays. -- Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
PATCH v9 5/7 refs: add failure_errno to refs_read_raw_ref() signature
In reply to Ævar Arnfjörð BjarmasonFrom: Han-Wen Nienhuys <[email protected]>
This lets us use the explicit errno output parameter in refs_resolve_ref_unsafe.
Some of our callers explicitly do not care about the errno, rather than understanding NULL let's have them declare that they don't care by passing in an "ignore_errno". There's only three of them, and using that pattern will make it more obvious that they want to throw away data, let's also add a comment to one of the callers about why we'd like to ignore the errno.
Let's not extend that to refs_resolve_ref_unsafe() itself for now, it has a large set of legacy callers, so we're faking up the old "errno" behavior for it. We can convert those callers to refs_resolve_ref_unsafe_with_errno() later.
We are leaving out out the refs_read_special_head() in refs_read_raw_ref() for now, as noted in the next commit moving it to "failure_errno" will require some special consideration.
We're intentionally mis-indenting the argument list of the new refs_resolve_ref_unsafe_with_errno(), it will be non-static in a subsequent commit, doing it this way makes that diff smaller.
--- refs.c | 61 ++++++++++++++++++++++++++++++------------- refs/files-backend.c | 10 ++++--- refs/packed-backend.c | 7 ++--- refs/refs-internal.h | 6 ++--- 4 files changed, 56 insertions(+), 28 deletions(-)
refs.c +43 -18
@@ -1672,30 +1672,33 @@ static int refs_read_special_head(struct ref_store *ref_store,return result; } -int refs_read_raw_ref (struct ref_store *ref_store, -const char *refname,struct object_id *oid, -struct strbuf *referent,unsigned int *type) +int refs_read_raw_ref (struct ref_store *ref_store,const char *refname, +struct object_id *oid,struct strbuf *referent, +unsigned int *type,int *failure_errno) { +assert (failure_errno);if (!strcmp (refname,"FETCH_HEAD" ) || !strcmp (refname,"MERGE_HEAD" )) {return refs_read_special_head (ref_store, refname, oid, referent, type); }return ref_store->be->read_raw_ref (ref_store, refname, oid, referent, - type, &errno); + type, failure_errno); } -/* This function needs to return a meaningful errno on failure */ -const char *refs_resolve_ref_unsafe (struct ref_store *refs, -const char *refname, -int resolve_flags, -struct object_id *oid,int *flags) +static const char *refs_resolve_ref_unsafe_with_errno (struct ref_store *refs, +const char *refname, +int resolve_flags, +struct object_id *oid, +int *flags,int *failure_errno) {static struct strbuf sb_refname =STRBUF_INIT ;struct object_id unused_oid;int unused_flags;int symref_count; +assert (failure_errno); +if (!oid) oid = &unused_oid;if (!flags)
@@ -1706,7 +1709,7 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,if (check_refname_format (refname,REFNAME_ALLOW_ONELEVEL )) {if (!(resolve_flags &RESOLVE_REF_ALLOW_BAD_NAME ) || !refname_is_safe (refname)) { - errno =EINVAL ; + *failure_errno =EINVAL ;return NULL ; }
@@ -1724,8 +1727,8 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,for (symref_count =0 ; symref_count <SYMREF_MAXDEPTH ; symref_count++) {unsigned int read_flags =0 ; -if (refs_read_raw_ref (refs, refname, - oid, &sb_refname, &read_flags)) { +if (refs_read_raw_ref (refs, refname, oid, &sb_refname, + &read_flags, failure_errno)) { *flags |= read_flags;/* In reading mode, refs must eventually resolve */
@@ -1737,9 +1740,9 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,* may show errors besides ENOENT if there are * similarly-named refs. */ -if (errno !=ENOENT && - errno !=EISDIR && - errno !=ENOTDIR ) +if (*failure_errno !=ENOENT && + *failure_errno !=EISDIR && + *failure_errno !=ENOTDIR )return NULL ;oidclr (oid);
@@ -1766,7 +1769,7 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,if (check_refname_format (refname,REFNAME_ALLOW_ONELEVEL )) {if (!(resolve_flags &RESOLVE_REF_ALLOW_BAD_NAME ) || !refname_is_safe (refname)) { - errno =EINVAL ; + *failure_errno =EINVAL ;return NULL ; }
@@ -1774,10 +1777,24 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs, } } - errno =ELOOP ; + *failure_errno =ELOOP ;return NULL ; } +const char *refs_resolve_ref_unsafe (struct ref_store *refs,const char *refname, +int resolve_flags,struct object_id *oid, +int *flags) +{ +int failure_errno =0 ; +const char *refn; + refn =refs_resolve_ref_unsafe_with_errno (refs, refname, resolve_flags, + oid, flags, &failure_errno); +if (!refn) +/* For unmigrated legacy callers */ + errno = failure_errno; +return refn; +} +/* backend functions */ int refs_init_db (struct strbuf *err) {
@@ -2228,6 +2245,13 @@ int refs_verify_refname_available(struct ref_store *refs,strbuf_grow (&dirname,strlen (refname) +1 );for (slash =strchr (refname,'/' ); slash; slash =strchr (slash +1 ,'/' )) { +/* +* Just saying "Is a directory" when we e.g. can't +* lock some multi-level ref isn't very informative, +* the user won't be told *what* is a directory, so +* let's not use strerror() below. +*/ +int ignore_errno;/* Expand dirname to the new prefix, not including the trailing slash: */ strbuf_add (&dirname, refname + dirname.len, slash - refname - dirname.len);
@@ -2239,7 +2263,8 @@ int refs_verify_refname_available(struct ref_store *refs,if (skip &&string_list_has_string (skip, dirname.buf))continue ; -if (!refs_read_raw_ref (refs, dirname.buf, &oid, &referent, &type)) { +if (!refs_read_raw_ref (refs, dirname.buf, &oid, &referent, + &type, &ignore_errno)) {strbuf_addf (err,_ ("'%s' exists; cannot create '%s'" ), dirname.buf, refname); goto cleanup;
refs/files-backend.c +6 -4
@@ -381,10 +381,11 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname, goto out;if (lstat (path, &st) <0 ) { +int ignore_errno;if (errno !=ENOENT ) goto out; -if (refs_read_raw_ref (refs->packed_ref_store, refname, - oid, referent, type)) { +if (refs_read_raw_ref (refs->packed_ref_store, refname, oid, + referent, type, &ignore_errno)) { errno =ENOENT ; goto out; }
@@ -418,13 +419,14 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,/* Is it a directory? */ if (S_ISDIR (st.st_mode)) { +int ignore_errno;/* * Even though there is a directory where the loose * ref is supposed to be, there could still be a * packed ref: */ -if (refs_read_raw_ref (refs->packed_ref_store, refname, - oid, referent, type)) { +if (refs_read_raw_ref (refs->packed_ref_store, refname, oid, + referent, type, &ignore_errno)) { errno =EISDIR ; goto out; }
refs/packed-backend.c +4 -3
@@ -1347,6 +1347,7 @@ int is_packed_transaction_needed(struct ref_store *ref_store, ret =0 ;for (i =0 ; i < transaction->nr; i++) {struct ref_update *update = transaction->updates[i]; +int failure_errno;unsigned int type;struct object_id oid;
@@ -1357,9 +1358,9 @@ int is_packed_transaction_needed(struct ref_store *ref_store,*/ continue ; -if (!refs_read_raw_ref (ref_store, update->refname, - &oid, &referent, &type) || - errno !=ENOENT ) { +if (!refs_read_raw_ref (ref_store, update->refname, &oid, + &referent, &type, &failure_errno) || + failure_errno !=ENOENT ) {/* * We have to actually delete that reference * -> this transaction is needed.
refs/refs-internal.h +3 -3
@@ -149,9 +149,9 @@ struct ref_update {const char refname[FLEX_ARRAY ]; }; -int refs_read_raw_ref (struct ref_store *ref_store, -const char *refname,struct object_id *oid, -struct strbuf *referent,unsigned int *type); +int refs_read_raw_ref (struct ref_store *ref_store,const char *refname, +struct object_id *oid,struct strbuf *referent, +unsigned int *type,int *failure_errno);/* * Write an error to `err` and return a nonzero value iff the same
Signature · 1 line
2.32.0.874.ge7a9d58bfcf
PATCH v9 6/7 refs: explicitly return failure_errno from parse_loose_ref_contents
In reply to Ævar Arnfjörð BjarmasonFrom: Han-Wen Nienhuys <[email protected]>
The EINVAL error from parse_loose_ref_contents is used in files-backend to create a custom error message.
In untangling this we discovered a tricky edge case. The refs_read_special_head() function was relying on parse_loose_ref_contents() setting EINVAL.
By converting it to use "saved_errno" we can migrate away from "errno" in this part of the code entirely, and do away with an existing "save_errno" pattern, its only purpose was to not clobber the "errno" we previously needed at the end of files_read_raw_ref().
Let's assert that we can do that by not having files_read_raw_ref() itself operate on *failure_errno in addition to passing it on. Instead we'll assert that if we return non-zero we actually do set errno, thus assuring ourselves and callers that they can trust the resulting "failure_errno".
--- refs.c | 8 +++++--- refs/files-backend.c | 30 +++++++++++++++++++----------- refs/refs-internal.h | 6 ++++-- 3 files changed, 28 insertions(+), 16 deletions(-)
refs.c +5 -3
@@ -1654,7 +1654,8 @@ int for_each_fullref_in_prefixes(const char *namespace,static int refs_read_special_head (struct ref_store *ref_store,const char *refname,struct object_id *oid, -struct strbuf *referent,unsigned int *type) +struct strbuf *referent,unsigned int *type, +int *failure_errno) {struct strbuf full_path =STRBUF_INIT ;struct strbuf content =STRBUF_INIT ;
@@ -1664,7 +1665,8 @@ static int refs_read_special_head(struct ref_store *ref_store,if (strbuf_read_file (&content, full_path.buf,0 ) <0 ) goto done; - result =parse_loose_ref_contents (content.buf, oid, referent, type); + result =parse_loose_ref_contents (content.buf, oid, referent, type, + failure_errno); done:strbuf_release (&full_path);
@@ -1679,7 +1681,7 @@ int refs_read_raw_ref(struct ref_store *ref_store, const char *refname,assert (failure_errno);if (!strcmp (refname,"FETCH_HEAD" ) || !strcmp (refname,"MERGE_HEAD" )) {return refs_read_special_head (ref_store, refname, oid, referent, - type); + type, failure_errno); } return ref_store->be->read_raw_ref(ref_store, refname, oid, referent,
refs/files-backend.c +19 -11
@@ -355,6 +355,7 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,int fd;int ret =-1 ;int remaining_retries =3 ; +int myerr =0 ; *type =0 ;strbuf_reset (&sb_path);
@@ -382,11 +383,13 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,if (lstat (path, &st) <0 ) {int ignore_errno; -if (errno !=ENOENT ) + myerr = errno; + errno =0 ; +if (myerr !=ENOENT ) goto out;if (refs_read_raw_ref (refs->packed_ref_store, refname, oid, referent, type, &ignore_errno)) { - errno =ENOENT ; + myerr =ENOENT ; goto out; } ret =0 ;
@@ -397,7 +400,9 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,if (S_ISLNK (st.st_mode)) {strbuf_reset (&sb_contents);if (strbuf_readlink (&sb_contents, path, st.st_size) <0 ) { -if (errno ==ENOENT || errno ==EINVAL ) + myerr = errno; + errno =0 ; +if (myerr ==ENOENT || myerr ==EINVAL )/* inconsistent with lstat; retry */ goto stat_ref;else
@@ -427,7 +432,7 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,*/ if (refs_read_raw_ref (refs->packed_ref_store, refname, oid, referent, type, &ignore_errno)) { - errno =EISDIR ; + myerr =EISDIR ; goto out; } ret =0 ;
@@ -440,7 +445,8 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,*/ fd =open (path,O_RDONLY );if (fd <0 ) { -if (errno ==ENOENT && !S_ISLNK (st.st_mode)) + myerr = errno; +if (myerr ==ENOENT && !S_ISLNK (st.st_mode))/* inconsistent with lstat; retry */ goto stat_ref;else
@@ -448,26 +454,28 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname, }strbuf_reset (&sb_contents);if (strbuf_read (&sb_contents, fd,256 ) <0 ) { -int save_errno = errno;close (fd); - errno = save_errno; goto out; }close (fd);strbuf_rtrim (&sb_contents); buf = sb_contents.buf; - ret =parse_loose_ref_contents (buf, oid, referent, type); + ret =parse_loose_ref_contents (buf, oid, referent, type, &myerr); out: - *failure_errno = errno; +if (ret && !myerr) +BUG ("returning non-zero %d, should have set myerr!" , ret); + *failure_errno = myerr; +strbuf_release (&sb_path);strbuf_release (&sb_contents);return ret; }int parse_loose_ref_contents (const char *buf,struct object_id *oid, -struct strbuf *referent,unsigned int *type) +struct strbuf *referent,unsigned int *type, +int *failure_errno) {const char *p;if (skip_prefix (buf,"ref:" , &buf)) {
@@ -486,7 +494,7 @@ int parse_loose_ref_contents(const char *buf, struct object_id *oid,if (parse_oid_hex (buf, oid, &p) || (*p !='\0' && !isspace (*p))) { *type |=REF_ISBROKEN ; - errno =EINVAL ; + *failure_errno =EINVAL ;return -1 ; }return 0 ;
refs/refs-internal.h +4 -2
@@ -692,10 +692,12 @@ struct ref_store { };/* -* Parse contents of a loose ref file. +* Parse contents of a loose ref file. *failure_errno maybe be set to EINVAL for +* invalid contents. */ int parse_loose_ref_contents (const char *buf,struct object_id *oid, -struct strbuf *referent,unsigned int *type); +struct strbuf *referent,unsigned int *type, +int *failure_errno);/* * Fill in the generic part of refs and add it to our collection of
Signature · 1 line
2.32.0.874.ge7a9d58bfcf
Re: [PATCH v9 6/7] refs: explicitly return failure_errno from parse_loose_ref_contents
refs/files-backend.c static int files_read_raw_ref(struct ref_store *ref_store, const char *refname, · line 459
Quoted 9 lines
refs/files-backend.c +0
-2@@ -448,26 +454,28 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname, }strbuf_reset (&sb_contents);if (strbuf_read (&sb_contents, fd,256 ) <0 ) { -int save_errno = errno;close (fd); - errno = save_errno; goto out; }
[snip unrelated code]
refs/files-backend.c static int files_read_raw_ref(struct ref_store *ref_store, const char *refname, · line 468
Quoted 4 lines
refs/files-backend.c +2
-1out: - *failure_errno = errno; +if (ret && !myerr) +BUG ("returning non-zero %d, should have set myerr!" , ret);
At $DAYJOB, some people have observed this BUG triggering. Right now I don't have a consistent reproduction recipe, but we noticed that the block starting with "if (strbuf_read" quoted above does not set myerr upon an error, and instead immediately jumps to "out" with ret being -1.
PATCH v9 7/7 refs: make errno output explicit for refs_resolve_ref_unsafe
In reply to Ævar Arnfjörð BjarmasonFrom: Han-Wen Nienhuys <[email protected]>
This introduces refs_resolve_ref_unsafe_with_errno(), which makes the API contract for the errno output explicit. The implementation still relies on the global errno variable to ensure no side effects of this refactoring.
lock_ref_oid_basic() in files-backend.c is the only caller of refs_resolve_ref() that needs error information to make logic decisions, so update that caller
--- refs.c | 2 +- refs.h | 11 +++++++++++ refs/files-backend.c | 10 ++++++---- 3 files changed, 18 insertions(+), 5 deletions(-)
refs.c +1 -1
@@ -1688,7 +1688,7 @@ int refs_read_raw_ref(struct ref_store *ref_store, const char *refname, type, failure_errno); } -static const char *refs_resolve_ref_unsafe_with_errno(struct ref_store *refs, +const char *refs_resolve_ref_unsafe_with_errno(struct ref_store *refs, const char *refname, int resolve_flags, struct object_id *oid,
refs.h +11 -0
@@ -68,6 +68,17 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,int resolve_flags, struct object_id *oid, int *flags); +/** +* refs_resolve_ref_unsafe_with_errno() is like +* refs_resolve_ref_unsafe(), but provide access to errno code that +* lead to a failure. We guarantee that errno is set to a meaningful +* value on non-zero return. +*/ +const char *refs_resolve_ref_unsafe_with_errno (struct ref_store *refs, +const char *refname, +int resolve_flags, +struct object_id *oid, +int *flags,int *failure_errno);const char *resolve_ref_unsafe (const char *refname,int resolve_flags,struct object_id *oid,int *flags);
refs/files-backend.c +6 -4
@@ -1001,6 +1001,7 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, {struct strbuf ref_file =STRBUF_INIT ;struct ref_lock *lock; +int resolve_errno =0 ;files_assert_main_repository (refs,"lock_ref_oid_basic" );assert (err);
@@ -1008,13 +1009,14 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,CALLOC_ARRAY (lock,1 );files_ref_path (refs, &ref_file, refname); -if (!refs_resolve_ref_unsafe (&refs->base, refname, -RESOLVE_REF_NO_RECURSE , - &lock->old_oid, type)) { +if (!refs_resolve_ref_unsafe_with_errno (&refs->base, refname, +RESOLVE_REF_NO_RECURSE , + &lock->old_oid, type, + &resolve_errno)) {if (!refs_verify_refname_available (&refs->base, refname,NULL ,NULL , err))strbuf_addf (err,"unable to resolve reference '%s': %s" , - refname,strerror (errno)); + refname,strerror (resolve_errno)); goto error_return; }
Signature · 1 line
2.32.0.874.ge7a9d58bfcf
Quoted 4 lines · Ævar Arnfjörð Bjarmason
A v9 re-roll of the v8, see
https://lore.kernel.org/git/[email protected]/
and more imporantly the real summary of what this is in v7 at
https://lore.kernel.org/git/[email protected]/
FYI I did not re-roll a v10 of this on top of the just re-rolled base topic:
https://lore.kernel.org/git/[email protected]/#t
The range-diff I have locally between the two (this version, and my rebased "should I need a re-roll" version) shows no changes, so this still cleanly applies on top.
Quoted 7 lines · Ævar Arnfjörð Bjarmason
Quoted 4 lines · Ævar Arnfjörð Bjarmason
A v9 re-roll of the v8, see
https://lore.kernel.org/git/[email protected]/
and more imporantly the real summary of what this is in v7 at
https://lore.kernel.org/git/[email protected]/
Quoted 7 lines
FYI I did not re-roll a v10 of this on top of the just re-rolled base topic:
https://lore.kernel.org/git/[email protected]/#tThe range-diff I have locally between the two (this version, and my rebased "should I need a re-roll" version) shows no changes, so this still cleanly applies on top.
Thanks. I just rebased it on top of the base topic locally. No need to resend.
v10 cover refs: cleanup errno sideband ref related functions
In reply to Ævar Arnfjörð BjarmasonA v10 re-roll of the v9, see
https://lore.kernel.org/git/[email protected]/
and more imporantly the real summary of what this is in v7 at
https://lore.kernel.org/git/[email protected]/
This topic builds on my just-re-rolled v5 removing of dead code + fixing a gc race in the refs API, at:
https://lore.kernel.org/git/[email protected]/
The changes since v9 are:
* Rebased on v5 of its "base" topic, see:
https://lore.kernel.org/git/[email protected]/
* Incorporate the test + fix Han-Wen submitted as
https://lore.kernel.org/git/[email protected]/;
that's here as 6/8.
I fixed up the test code a bit, added a SYMLINKS prerequisite as suggested by Junio, used a "test_when_finished" instead of a "rm -rf" at the start, used test_cmp over "test", and other minor style changes. The actual BUG() fix is then squashed into 7/8.
* Fixed issues in refs/debug.c, noted in
https://lore.kernel.org/git/CAFQ2z_M0LNmZn2xW_GWdwZOCi20xc9t3EnMzMzHP8ZcmWrW9EA@mail.gmail.com/
Han-Wen Nienhuys (7):
refs: remove EINVAL errno output from specification of read_raw_ref_fn refs/files-backend: stop setting errno from lock_ref_oid_basic
refs: make errno output explicit for read_raw_ref_fn
refs: add failure_errno to refs_read_raw_ref() signature branch tests: test for errno propagating on failing read
refs: explicitly return failure_errno from parse_loose_ref_contents
refs: make errno output explicit for refs_resolve_ref_unsafe
Ævar Arnfjörð Bjarmason (1):
refs file backend: move raceproof_create_file() here
cache.h | 43 --------- object-file.c | 68 --------------- refs.c | 69 ++++++++++----- refs.h | 11 +++ refs/debug.c | 7 +- refs/files-backend.c | 196 +++++++++++++++++++++++++++++++++--------- refs/packed-backend.c | 15 ++-- refs/refs-internal.h | 32 ++++--- t/t3200-branch.sh | 21 +++++ 9 files changed, 266 insertions(+), 196 deletions(-)
Range-diff against v9:
1: b7063c5af89 = 1: f06b054e861 refs file backend: move raceproof_create_file() here 2: 5a63b64f53f = 2: ba0f5f5fb0a refs: remove EINVAL errno output from specification of read_raw_ref_fn 3: 0dd8a4c1209 ! 3: 2c4c30e8e06 refs/files-backend: stop setting errno from lock_ref_oid_basic
@@ refs/files-backend.c: static int create_reflock(const char *path, void *cb)
- * On failure errno is set to something meaningful.
*/
static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
- const char *refname,
+ const char *refname, int *type,
@@ refs/files-backend.c: static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
{
struct strbuf ref_file = STRBUF_INIT;
4: c54fb99714a ! 4: 05f3a346e3b refs: make errno output explicit for read_raw_ref_fn
@@ refs/debug.c: debug_ref_iterator_begin(struct ref_store *ref_store, const char *
{
struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
int res = 0;
-@@ refs/debug.c: static int debug_read_raw_ref(struct ref_store *ref_store, const char *refname,
+
oidcpy(oid, null_oid());
- errno = 0;
+- errno = 0;
res = drefs->refs->be->read_raw_ref(drefs->refs, refname, oid, referent,
- type);
+ type, failure_errno);
if (res == 0) {
trace_printf_key(&trace_refs, "read_raw_ref: %s: %s (=> %s) type %x: %d\n",
+@@ refs/debug.c: static int debug_read_raw_ref(struct ref_store *ref_store, const char *refname,
+ } else {
+ trace_printf_key(&trace_refs,
+ "read_raw_ref: %s: %d (errno %d)\n", refname,
+- res, errno);
++ res, *failure_errno);
+ }
+ return res;
+ }
## refs/files-backend.c ##
@@ refs/files-backend.c: static struct ref_cache *get_loose_ref_cache(struct files_ref_store *refs)
5: 18825efce7d = 5: fa9260f25fa refs: add failure_errno to refs_read_raw_ref() signature
-: ----------- > 6: 6dae8b643ad branch tests: test for errno propagating on failing read
6: 57e3f246f4f ! 7: 18bf4a0e97c refs: explicitly return failure_errno from parse_loose_ref_contents @@ refs/files-backend.c: static int files_read_raw_ref(struct ref_store *ref_store,
strbuf_reset(&sb_contents);
if (strbuf_read(&sb_contents, fd, 256) < 0) {
- int save_errno = errno;
++ myerr = errno;
close(fd);
- errno = save_errno;
goto out;
@@ refs/refs-internal.h: struct ref_store {
/*
* Fill in the generic part of refs and add it to our collection of
+
+ ## t/t3200-branch.sh ##
+@@ t/t3200-branch.sh: test_expect_success SYMLINKS 'git branch -m with symlinked .git/refs' '
+ ) &&
+ git --git-dir subdir/.git/ branch rename-src &&
+ git rev-parse rename-src >expect &&
++ # Tests a BUG() assertion in files_read_raw_ref()
+ git --git-dir subdir/.git/ branch -m rename-src rename-dest &&
+ git rev-parse rename-dest >actual &&
+ test_cmp expect actual &&
7: 4b5e168b978 = 8: 7d94a32af83 refs: make errno output explicit for refs_resolve_ref_unsafe
Signature · 1 line
2.33.0.662.g438caf9576d
PATCH v10 1/8 refs file backend: move raceproof_create_file() here
Move the raceproof_create_file() API added to cache.h and object-file.c in 177978f56ad (raceproof_create_file(): new function, 2017-01-06) to its only user, refs/files-backend.c.
--- cache.h | 43 ----------------- object-file.c | 68 --------------------------- refs/files-backend.c | 109 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 109 insertions(+), 111 deletions(-)
cache.h +0 -43
@@ -1211,49 +1211,6 @@ enum scld_error safe_create_leading_directories(char *path);enum scld_error safe_create_leading_directories_const (const char *path);enum scld_error safe_create_leading_directories_no_share (char *path); -/* -* Callback function for raceproof_create_file(). This function is -* expected to do something that makes dirname(path) permanent despite -* the fact that other processes might be cleaning up empty -* directories at the same time. Usually it will create a file named -* path, but alternatively it could create another file in that -* directory, or even chdir() into that directory. The function should -* return 0 if the action was completed successfully. On error, it -* should return a nonzero result and set errno. -* raceproof_create_file() treats two errno values specially: -* -* - ENOENT -- dirname(path) does not exist. In this case, -* raceproof_create_file() tries creating dirname(path) -* (and any parent directories, if necessary) and calls -* the function again. -* -* - EISDIR -- the file already exists and is a directory. In this -* case, raceproof_create_file() removes the directory if -* it is empty (and recursively any empty directories that -* it contains) and calls the function again. -* -* Any other errno causes raceproof_create_file() to fail with the -* callback's return value and errno. -* -* Obviously, this function should be OK with being called again if it -* fails with ENOENT or EISDIR. In other scenarios it will not be -* called again. -*/ -typedef int create_file_fn (const char *path,void *cb); - -/* -* Create a file in dirname(path) by calling fn, creating leading -* directories if necessary. Retry a few times in case we are racing -* with another process that is trying to clean up the directory that -* contains path. See the documentation for create_file_fn for more -* details. -* -* Return the value and set the errno that resulted from the most -* recent call of fn. fn is always called at least once, and will be -* called more than once if it returns ENOENT or EISDIR. -*/ -int raceproof_create_file (const char *path,create_file_fn fn,void *cb); -int mkdir_in_gitdir (const char *path);char *expand_user_path (const char *path,int real_home);const char *enter_repo (const char *path,int strict);
object-file.c +0 -68
74 lines not on this page · raw message
refs/files-backend.c +109 -0
115 lines not on this page · raw message
Signature · 1 line
2.33.0.662.g438caf9576d
PATCH v10 2/8 refs: remove EINVAL errno output from specification of read_raw_ref_fn
In reply to Ævar Arnfjörð BjarmasonFrom: Han-Wen Nienhuys <[email protected]>
This commit does not change code; it documents the fact that an alternate ref backend does not need to return EINVAL from read_raw_ref_fn to function properly.
This is correct, because refs_read_raw_ref is only called from;
* resolve_ref_unsafe(), which does not care for the EINVAL errno result.
* refs_verify_refname_available(), which does not inspect errno.
* files-backend.c, where errno is overwritten on failure.
* packed-backend.c (is_packed_transaction_needed), which calls it for the packed ref backend, which never emits EINVAL.
A grep for EINVAL */*c reveals that no code checks errno against EINVAL after reading references. In addition, the refs.h file does not mention errno at all.
A grep over resolve_ref_unsafe() turned up the following callers that inspect errno:
* sequencer.c::print_commit_summary, which uses it for die_errno
* lock_ref_oid_basic(), which only treats EISDIR and ENOTDIR specially.
The files ref backend does use EINVAL. The files backend does not call into the generic API (refs_read_raw), but into the files-specific function (files_read_raw_ref), which we are not changing in this commit.
As the errno sideband is unintuitive and error-prone, remove EINVAL value, as a step towards getting rid of the errno sideband altogether.
Spotted by Ævar Arnfjörð Bjarmason <[email protected]>.
--- refs/refs-internal.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
refs/refs-internal.h +3 -3
@@ -622,9 +622,9 @@ typedef int reflog_expire_fn(struct ref_store *ref_store,* * Return 0 on success. If the ref doesn't exist, set errno to ENOENT * and return -1. If the ref exists but is neither a symbolic ref nor -* an object ID, it is broken; set REF_ISBROKEN in type, set errno to -* EINVAL, and return -1. If there is another error reading the ref, -* set errno appropriately and return -1. +* an object ID, it is broken; set REF_ISBROKEN in type, and return -1 +* (errno should not be ENOENT) If there is another error reading the +* ref, set errno appropriately and return -1. * * Backend-specific flags might be set in type as well, regardless of * outcome.
Signature · 1 line
2.33.0.662.g438caf9576d
PATCH v10 3/8 refs/files-backend: stop setting errno from lock_ref_oid_basic
In reply to Ævar Arnfjörð BjarmasonFrom: Han-Wen Nienhuys <[email protected]>
refs/files-backend.c::lock_ref_oid_basic() tries to signal how it failed to its callers using errno.
It is safe to stop setting errno here, because the callers of this file-scope static function are
* files_copy_or_rename_ref()
* files_create_symref()
* files_reflog_expire()
None of them looks at errno after seeing a negative return from lock_ref_oid_basic() to make any decision, and no caller of these three functions looks at errno after they signal a failure by returning a negative value. In particular,
* files_copy_or_rename_ref() - here, calls are followed by error() (which performs I/O) or write_ref_to_lockfile() (which calls parse_object() which may perform I/O)
* files_create_symref() - here, calls are followed by error() or create_symref_locked() (which performs I/O and does not inspect errno)
* files_reflog_expire() - here, calls are followed by error() or refs_reflog_exists() (which calls a function in a vtable that is not documented to use and/or preserve errno)
--- refs/files-backend.c | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-)
refs/files-backend.c +2 -9
@@ -982,7 +982,6 @@ static int create_reflock(const char *path, void *cb)/* * Locks a ref returning the lock on success and NULL on failure. -* On failure errno is set to something meaningful. */ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, const char *refname, int *type,
@@ -990,7 +989,6 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, {struct strbuf ref_file =STRBUF_INIT ;struct ref_lock *lock; -int last_errno =0 ;files_assert_main_repository (refs,"lock_ref_oid_basic" );assert (err);
@@ -1001,11 +999,10 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,if (!refs_resolve_ref_unsafe (&refs->base, refname,RESOLVE_REF_NO_RECURSE , &lock->old_oid, type)) { - last_errno = errno;if (!refs_verify_refname_available (&refs->base, refname,NULL ,NULL , err))strbuf_addf (err,"unable to resolve reference '%s': %s" , - refname,strerror (last_errno)); + refname,strerror (errno)); goto error_return; }
@@ -1018,15 +1015,12 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,*/ if (is_null_oid (&lock->old_oid) &&refs_verify_refname_available (refs->packed_ref_store, refname, -NULL ,NULL , err)) { - last_errno =ENOTDIR ; +NULL ,NULL , err)) goto error_return; - } lock->ref_name =xstrdup (refname);if (raceproof_create_file (ref_file.buf, create_reflock, &lock->lk)) { - last_errno = errno;unable_to_lock_message (ref_file.buf, errno, err); goto error_return; }
@@ -1043,7 +1037,6 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, out:strbuf_release (&ref_file); - errno = last_errno;return lock; }
Signature · 1 line
2.33.0.662.g438caf9576d
PATCH v10 4/8 refs: make errno output explicit for read_raw_ref_fn
In reply to Ævar Arnfjörð BjarmasonFrom: Han-Wen Nienhuys <[email protected]>
This makes it explicit how alternative ref backends should report errors in read_raw_ref_fn.
read_raw_ref_fn needs to supply a credible errno for a number of cases. These are primarily:
1) The files backend calls read_raw_ref from lock_raw_ref, and uses the resulting error codes to create/remove directories as needed.
2) ENOENT should be translated in a zero OID, optionally with REF_ISBROKEN set, returning the last successfully resolved symref. This is necessary so read_raw_ref("HEAD") on an empty repo returns refs/heads/main (or the default branch du-jour), and we know on which branch to create the first commit.
Make this information flow explicit by adding a failure_errno to the signature of read_raw_ref. All errnos from the files backend are still propagated unchanged, even though inspection suggests only ENOTDIR, EISDIR and ENOENT are relevant.
--- refs.c | 2 +- refs/debug.c | 7 +++---- refs/files-backend.c | 29 +++++++++++++++-------------- refs/packed-backend.c | 8 ++++---- refs/refs-internal.h | 20 ++++++++++++-------- 5 files changed, 35 insertions(+), 31 deletions(-)
refs.c +1 -1
@@ -1682,7 +1682,7 @@ int refs_read_raw_ref(struct ref_store *ref_store, }return ref_store->be->read_raw_ref (ref_store, refname, oid, referent, - type); + type, &errno); }/* This function needs to return a meaningful errno on failure */
refs/debug.c +3 -4
@@ -239,15 +239,14 @@ debug_ref_iterator_begin(struct ref_store *ref_store, const char *prefix,static int debug_read_raw_ref (struct ref_store *ref_store,const char *refname,struct object_id *oid,struct strbuf *referent, -unsigned int *type) +unsigned int *type,int *failure_errno) {struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;int res =0 ;oidcpy (oid,null_oid ()); - errno =0 ; res = drefs->refs->be->read_raw_ref (drefs->refs, refname, oid, referent, - type); + type, failure_errno);if (res ==0 ) { trace_printf_key(&trace_refs, "read_raw_ref: %s: %s (=> %s) type %x: %d\n",
@@ -255,7 +254,7 @@ static int debug_read_raw_ref(struct ref_store *ref_store, const char *refname, }else {trace_printf_key (&trace_refs,"read_raw_ref: %s: %d (errno %d)\n" , refname, - res, errno); + res, *failure_errno); }return res; }
refs/files-backend.c +15 -14
@@ -341,9 +341,9 @@ static struct ref_cache *get_loose_ref_cache(struct files_ref_store *refs)return refs->loose; } -static int files_read_raw_ref (struct ref_store *ref_store, -const char *refname,struct object_id *oid, -struct strbuf *referent,unsigned int *type) +static int files_read_raw_ref (struct ref_store *ref_store,const char *refname, +struct object_id *oid,struct strbuf *referent, +unsigned int *type,int *failure_errno) {struct files_ref_store *refs =files_downcast (ref_store,REF_STORE_READ ,"read_raw_ref" );
@@ -354,7 +354,6 @@ static int files_read_raw_ref(struct ref_store *ref_store,struct stat st;int fd;int ret =-1 ; -int save_errno;int remaining_retries =3 ; *type =0 ;
@@ -459,10 +458,9 @@ static int files_read_raw_ref(struct ref_store *ref_store, ret =parse_loose_ref_contents (buf, oid, referent, type); out: - save_errno = errno; + *failure_errno = errno;strbuf_release (&sb_path);strbuf_release (&sb_contents); - errno = save_errno;return ret; }
@@ -540,6 +538,7 @@ static int lock_raw_ref(struct files_ref_store *refs,struct strbuf ref_file =STRBUF_INIT ;int attempts_remaining =3 ;int ret =TRANSACTION_GENERIC_ERROR ; +int failure_errno;assert (err);files_assert_main_repository (refs,"lock_raw_ref" );
@@ -610,7 +609,9 @@ static int lock_raw_ref(struct files_ref_store *refs,if (hold_lock_file_for_update_timeout ( &lock->lk, ref_file.buf,LOCK_NO_DEREF ,get_files_ref_lock_timeout_ms ()) <0 ) { -if (errno ==ENOENT && --attempts_remaining >0 ) { +int myerr = errno; + errno =0 ; +if (myerr ==ENOENT && --attempts_remaining >0 ) {/* * Maybe somebody just deleted one of the * directories leading to ref_file. Try
@@ -618,7 +619,7 @@ static int lock_raw_ref(struct files_ref_store *refs,*/ goto retry; }else { -unable_to_lock_message (ref_file.buf, errno, err); +unable_to_lock_message (ref_file.buf, myerr, err); goto error_return; } }
@@ -628,9 +629,9 @@ static int lock_raw_ref(struct files_ref_store *refs,* fear that its value will change. */ -if (files_read_raw_ref (&refs->base, refname, - &lock->old_oid, referent, type)) { -if (errno ==ENOENT ) { +if (files_read_raw_ref (&refs->base, refname, &lock->old_oid, referent, + type, &failure_errno)) { +if (failure_errno ==ENOENT ) {if (mustexist) {/* Garden variety missing reference. */ strbuf_addf(err, "unable to resolve reference '%s'",
@@ -654,7 +655,7 @@ static int lock_raw_ref(struct files_ref_store *refs,* reference named "refs/foo/bar/baz". */ } - }else if (errno ==EISDIR ) { + }else if (failure_errno ==EISDIR ) {/* * There is a directory in the way. It might have * contained references that have been deleted. If
@@ -692,13 +693,13 @@ static int lock_raw_ref(struct files_ref_store *refs, goto error_return; } } - }else if (errno ==EINVAL && (*type &REF_ISBROKEN )) { + }else if (failure_errno ==EINVAL && (*type &REF_ISBROKEN )) {strbuf_addf (err,"unable to resolve reference '%s': " "reference broken" , refname); goto error_return; }else {strbuf_addf (err,"unable to resolve reference '%s': %s" , - refname,strerror (errno)); + refname,strerror (failure_errno)); goto error_return; }
refs/packed-backend.c +4 -4
@@ -724,9 +724,9 @@ static struct snapshot *get_snapshot(struct packed_ref_store *refs)return refs->snapshot; } -static int packed_read_raw_ref (struct ref_store *ref_store, -const char *refname,struct object_id *oid, -struct strbuf *referent,unsigned int *type) +static int packed_read_raw_ref (struct ref_store *ref_store,const char *refname, +struct object_id *oid,struct strbuf *referent, +unsigned int *type,int *failure_errno) {struct packed_ref_store *refs =packed_downcast (ref_store,REF_STORE_READ ,"read_raw_ref" );
@@ -739,7 +739,7 @@ static int packed_read_raw_ref(struct ref_store *ref_store,if (!rec) {/* refname is not a packed reference. */ - errno =ENOENT ; + *failure_errno =ENOENT ;return -1 ; }
refs/refs-internal.h +12 -8
@@ -620,11 +620,15 @@ typedef int reflog_expire_fn(struct ref_store *ref_store,* properly-formatted or even safe reference name. NEITHER INPUT NOR * OUTPUT REFERENCE NAMES ARE VALIDATED WITHIN THIS FUNCTION. * -* Return 0 on success. If the ref doesn't exist, set errno to ENOENT -* and return -1. If the ref exists but is neither a symbolic ref nor -* an object ID, it is broken; set REF_ISBROKEN in type, and return -1 -* (errno should not be ENOENT) If there is another error reading the -* ref, set errno appropriately and return -1. +* Return 0 on success, or -1 on failure. If the ref exists but is neither a +* symbolic ref nor an object ID, it is broken. In this case set REF_ISBROKEN in +* type, and return -1 (failure_errno should not be ENOENT) +* +* failure_errno provides errno codes that are interpreted beyond error +* reporting. The following error codes have special meaning: +* * ENOENT: the ref doesn't exist +* * EISDIR: ref name is a directory +* * ENOTDIR: ref prefix is not a directory * * Backend-specific flags might be set in type as well, regardless of * outcome.
@@ -638,9 +642,9 @@ typedef int reflog_expire_fn(struct ref_store *ref_store,* - in all other cases, referent will be untouched, and therefore * refname will still be valid and unchanged. */ -typedef int read_raw_ref_fn (struct ref_store *ref_store, -const char *refname,struct object_id *oid, -struct strbuf *referent,unsigned int *type); +typedef int read_raw_ref_fn (struct ref_store *ref_store,const char *refname, +struct object_id *oid,struct strbuf *referent, +unsigned int *type,int *failure_errno);struct ref_storage_be {struct ref_storage_be *next;
Signature · 1 line
2.33.0.662.g438caf9576d
PATCH v10 5/8 refs: add failure_errno to refs_read_raw_ref() signature
In reply to Ævar Arnfjörð BjarmasonFrom: Han-Wen Nienhuys <[email protected]>
This lets us use the explicit errno output parameter in refs_resolve_ref_unsafe.
Some of our callers explicitly do not care about the errno, rather than understanding NULL let's have them declare that they don't care by passing in an "ignore_errno". There's only three of them, and using that pattern will make it more obvious that they want to throw away data, let's also add a comment to one of the callers about why we'd like to ignore the errno.
Let's not extend that to refs_resolve_ref_unsafe() itself for now, it has a large set of legacy callers, so we're faking up the old "errno" behavior for it. We can convert those callers to refs_resolve_ref_unsafe_with_errno() later.
We are leaving out out the refs_read_special_head() in refs_read_raw_ref() for now, as noted in the next commit moving it to "failure_errno" will require some special consideration.
We're intentionally mis-indenting the argument list of the new refs_resolve_ref_unsafe_with_errno(), it will be non-static in a subsequent commit, doing it this way makes that diff smaller.
--- refs.c | 61 ++++++++++++++++++++++++++++++------------- refs/files-backend.c | 10 ++++--- refs/packed-backend.c | 7 ++--- refs/refs-internal.h | 6 ++--- 4 files changed, 56 insertions(+), 28 deletions(-)
refs.c +43 -18
@@ -1672,30 +1672,33 @@ static int refs_read_special_head(struct ref_store *ref_store,return result; } -int refs_read_raw_ref (struct ref_store *ref_store, -const char *refname,struct object_id *oid, -struct strbuf *referent,unsigned int *type) +int refs_read_raw_ref (struct ref_store *ref_store,const char *refname, +struct object_id *oid,struct strbuf *referent, +unsigned int *type,int *failure_errno) { +assert (failure_errno);if (!strcmp (refname,"FETCH_HEAD" ) || !strcmp (refname,"MERGE_HEAD" )) {return refs_read_special_head (ref_store, refname, oid, referent, type); }return ref_store->be->read_raw_ref (ref_store, refname, oid, referent, - type, &errno); + type, failure_errno); } -/* This function needs to return a meaningful errno on failure */ -const char *refs_resolve_ref_unsafe (struct ref_store *refs, -const char *refname, -int resolve_flags, -struct object_id *oid,int *flags) +static const char *refs_resolve_ref_unsafe_with_errno (struct ref_store *refs, +const char *refname, +int resolve_flags, +struct object_id *oid, +int *flags,int *failure_errno) {static struct strbuf sb_refname =STRBUF_INIT ;struct object_id unused_oid;int unused_flags;int symref_count; +assert (failure_errno); +if (!oid) oid = &unused_oid;if (!flags)
@@ -1706,7 +1709,7 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,if (check_refname_format (refname,REFNAME_ALLOW_ONELEVEL )) {if (!(resolve_flags &RESOLVE_REF_ALLOW_BAD_NAME ) || !refname_is_safe (refname)) { - errno =EINVAL ; + *failure_errno =EINVAL ;return NULL ; }
@@ -1724,8 +1727,8 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,for (symref_count =0 ; symref_count <SYMREF_MAXDEPTH ; symref_count++) {unsigned int read_flags =0 ; -if (refs_read_raw_ref (refs, refname, - oid, &sb_refname, &read_flags)) { +if (refs_read_raw_ref (refs, refname, oid, &sb_refname, + &read_flags, failure_errno)) { *flags |= read_flags;/* In reading mode, refs must eventually resolve */
@@ -1737,9 +1740,9 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,* may show errors besides ENOENT if there are * similarly-named refs. */ -if (errno !=ENOENT && - errno !=EISDIR && - errno !=ENOTDIR ) +if (*failure_errno !=ENOENT && + *failure_errno !=EISDIR && + *failure_errno !=ENOTDIR )return NULL ;oidclr (oid);
@@ -1766,7 +1769,7 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,if (check_refname_format (refname,REFNAME_ALLOW_ONELEVEL )) {if (!(resolve_flags &RESOLVE_REF_ALLOW_BAD_NAME ) || !refname_is_safe (refname)) { - errno =EINVAL ; + *failure_errno =EINVAL ;return NULL ; }
@@ -1774,10 +1777,24 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs, } } - errno =ELOOP ; + *failure_errno =ELOOP ;return NULL ; } +const char *refs_resolve_ref_unsafe (struct ref_store *refs,const char *refname, +int resolve_flags,struct object_id *oid, +int *flags) +{ +int failure_errno =0 ; +const char *refn; + refn =refs_resolve_ref_unsafe_with_errno (refs, refname, resolve_flags, + oid, flags, &failure_errno); +if (!refn) +/* For unmigrated legacy callers */ + errno = failure_errno; +return refn; +} +/* backend functions */ int refs_init_db (struct strbuf *err) {
@@ -2228,6 +2245,13 @@ int refs_verify_refname_available(struct ref_store *refs,strbuf_grow (&dirname,strlen (refname) +1 );for (slash =strchr (refname,'/' ); slash; slash =strchr (slash +1 ,'/' )) { +/* +* Just saying "Is a directory" when we e.g. can't +* lock some multi-level ref isn't very informative, +* the user won't be told *what* is a directory, so +* let's not use strerror() below. +*/ +int ignore_errno;/* Expand dirname to the new prefix, not including the trailing slash: */ strbuf_add (&dirname, refname + dirname.len, slash - refname - dirname.len);
@@ -2239,7 +2263,8 @@ int refs_verify_refname_available(struct ref_store *refs,if (skip &&string_list_has_string (skip, dirname.buf))continue ; -if (!refs_read_raw_ref (refs, dirname.buf, &oid, &referent, &type)) { +if (!refs_read_raw_ref (refs, dirname.buf, &oid, &referent, + &type, &ignore_errno)) {strbuf_addf (err,_ ("'%s' exists; cannot create '%s'" ), dirname.buf, refname); goto cleanup;
refs/files-backend.c +6 -4
@@ -381,10 +381,11 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname, goto out;if (lstat (path, &st) <0 ) { +int ignore_errno;if (errno !=ENOENT ) goto out; -if (refs_read_raw_ref (refs->packed_ref_store, refname, - oid, referent, type)) { +if (refs_read_raw_ref (refs->packed_ref_store, refname, oid, + referent, type, &ignore_errno)) { errno =ENOENT ; goto out; }
@@ -418,13 +419,14 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,/* Is it a directory? */ if (S_ISDIR (st.st_mode)) { +int ignore_errno;/* * Even though there is a directory where the loose * ref is supposed to be, there could still be a * packed ref: */ -if (refs_read_raw_ref (refs->packed_ref_store, refname, - oid, referent, type)) { +if (refs_read_raw_ref (refs->packed_ref_store, refname, oid, + referent, type, &ignore_errno)) { errno =EISDIR ; goto out; }
refs/packed-backend.c +4 -3
@@ -1347,6 +1347,7 @@ int is_packed_transaction_needed(struct ref_store *ref_store, ret =0 ;for (i =0 ; i < transaction->nr; i++) {struct ref_update *update = transaction->updates[i]; +int failure_errno;unsigned int type;struct object_id oid;
@@ -1357,9 +1358,9 @@ int is_packed_transaction_needed(struct ref_store *ref_store,*/ continue ; -if (!refs_read_raw_ref (ref_store, update->refname, - &oid, &referent, &type) || - errno !=ENOENT ) { +if (!refs_read_raw_ref (ref_store, update->refname, &oid, + &referent, &type, &failure_errno) || + failure_errno !=ENOENT ) {/* * We have to actually delete that reference * -> this transaction is needed.
refs/refs-internal.h +3 -3
@@ -149,9 +149,9 @@ struct ref_update {const char refname[FLEX_ARRAY ]; }; -int refs_read_raw_ref (struct ref_store *ref_store, -const char *refname,struct object_id *oid, -struct strbuf *referent,unsigned int *type); +int refs_read_raw_ref (struct ref_store *ref_store,const char *refname, +struct object_id *oid,struct strbuf *referent, +unsigned int *type,int *failure_errno);/* * Write an error to `err` and return a nonzero value iff the same
Signature · 1 line
2.33.0.662.g438caf9576d
PATCH v10 6/8 branch tests: test for errno propagating on failing read
In reply to Ævar Arnfjörð BjarmasonFrom: Han-Wen Nienhuys <[email protected]>
Add a test for "git branch" to cover the case where .git/refs is symlinked. To check availability, refs_verify_refname_available() will run refs_read_raw_ref() on each prefix, leading to a read() from .git/refs (which is a directory).
It would probably be more robust to re-issue the lstat() as a normal stat(), in which case, we would fall back to the directory case, but for now let's just test for the existing behavior as-is. This test covers a regression in a commit that only ever made it to "next", see [1].
1. http://lore.kernel.org/git/[email protected]
--- t/t3200-branch.sh | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
t/t3200-branch.sh +20 -0
@@ -731,6 +731,26 @@ test_expect_success SYMLINKS 'git branch -m u v should fail when the reflog for test_must_fail git branch -m u v ' +test_expect_success SYMLINKS 'git branch -m with symlinked .git/refs' ' + test_when_finished "rm -rf subdir" && + git init subdir && + + ( + cd subdir && + for d in refs objects packed-refs + do + rm -rf .git/$d && + ln -s ../../.git/$d .git/$d + done + ) && + git --git-dir subdir/.git/ branch rename-src && + git rev-parse rename-src >expect && + git --git-dir subdir/.git/ branch -m rename-src rename-dest && + git rev-parse rename-dest >actual && + test_cmp expect actual && + git branch -D rename-dest +' + test_expect_success 'test tracking setup via --track' ' git config remote.local.url . && git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
Signature · 1 line
2.33.0.662.g438caf9576d
PATCH v10 7/8 refs: explicitly return failure_errno from parse_loose_ref_contents
In reply to Ævar Arnfjörð BjarmasonFrom: Han-Wen Nienhuys <[email protected]>
The EINVAL error from parse_loose_ref_contents is used in files-backend to create a custom error message.
In untangling this we discovered a tricky edge case. The refs_read_special_head() function was relying on parse_loose_ref_contents() setting EINVAL.
By converting it to use "saved_errno" we can migrate away from "errno" in this part of the code entirely, and do away with an existing "save_errno" pattern, its only purpose was to not clobber the "errno" we previously needed at the end of files_read_raw_ref().
Let's assert that we can do that by not having files_read_raw_ref() itself operate on *failure_errno in addition to passing it on. Instead we'll assert that if we return non-zero we actually do set errno, thus assuring ourselves and callers that they can trust the resulting "failure_errno".
--- refs.c | 8 +++++--- refs/files-backend.c | 31 ++++++++++++++++++++----------- refs/refs-internal.h | 6 ++++-- t/t3200-branch.sh | 1 + 4 files changed, 30 insertions(+), 16 deletions(-)
refs.c +5 -3
@@ -1654,7 +1654,8 @@ int for_each_fullref_in_prefixes(const char *namespace,static int refs_read_special_head (struct ref_store *ref_store,const char *refname,struct object_id *oid, -struct strbuf *referent,unsigned int *type) +struct strbuf *referent,unsigned int *type, +int *failure_errno) {struct strbuf full_path =STRBUF_INIT ;struct strbuf content =STRBUF_INIT ;
@@ -1664,7 +1665,8 @@ static int refs_read_special_head(struct ref_store *ref_store,if (strbuf_read_file (&content, full_path.buf,0 ) <0 ) goto done; - result =parse_loose_ref_contents (content.buf, oid, referent, type); + result =parse_loose_ref_contents (content.buf, oid, referent, type, + failure_errno); done:strbuf_release (&full_path);
@@ -1679,7 +1681,7 @@ int refs_read_raw_ref(struct ref_store *ref_store, const char *refname,assert (failure_errno);if (!strcmp (refname,"FETCH_HEAD" ) || !strcmp (refname,"MERGE_HEAD" )) {return refs_read_special_head (ref_store, refname, oid, referent, - type); + type, failure_errno); } return ref_store->be->read_raw_ref(ref_store, refname, oid, referent,
refs/files-backend.c +20 -11
@@ -355,6 +355,7 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,int fd;int ret =-1 ;int remaining_retries =3 ; +int myerr =0 ; *type =0 ;strbuf_reset (&sb_path);
@@ -382,11 +383,13 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,if (lstat (path, &st) <0 ) {int ignore_errno; -if (errno !=ENOENT ) + myerr = errno; + errno =0 ; +if (myerr !=ENOENT ) goto out;if (refs_read_raw_ref (refs->packed_ref_store, refname, oid, referent, type, &ignore_errno)) { - errno =ENOENT ; + myerr =ENOENT ; goto out; } ret =0 ;
@@ -397,7 +400,9 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,if (S_ISLNK (st.st_mode)) {strbuf_reset (&sb_contents);if (strbuf_readlink (&sb_contents, path, st.st_size) <0 ) { -if (errno ==ENOENT || errno ==EINVAL ) + myerr = errno; + errno =0 ; +if (myerr ==ENOENT || myerr ==EINVAL )/* inconsistent with lstat; retry */ goto stat_ref;else
@@ -427,7 +432,7 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,*/ if (refs_read_raw_ref (refs->packed_ref_store, refname, oid, referent, type, &ignore_errno)) { - errno =EISDIR ; + myerr =EISDIR ; goto out; } ret =0 ;
@@ -440,7 +445,8 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,*/ fd =open (path,O_RDONLY );if (fd <0 ) { -if (errno ==ENOENT && !S_ISLNK (st.st_mode)) + myerr = errno; +if (myerr ==ENOENT && !S_ISLNK (st.st_mode))/* inconsistent with lstat; retry */ goto stat_ref;else
@@ -448,26 +454,29 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname, }strbuf_reset (&sb_contents);if (strbuf_read (&sb_contents, fd,256 ) <0 ) { -int save_errno = errno; + myerr = errno;close (fd); - errno = save_errno; goto out; }close (fd);strbuf_rtrim (&sb_contents); buf = sb_contents.buf; - ret =parse_loose_ref_contents (buf, oid, referent, type); + ret =parse_loose_ref_contents (buf, oid, referent, type, &myerr); out: - *failure_errno = errno; +if (ret && !myerr) +BUG ("returning non-zero %d, should have set myerr!" , ret); + *failure_errno = myerr; +strbuf_release (&sb_path);strbuf_release (&sb_contents);return ret; }int parse_loose_ref_contents (const char *buf,struct object_id *oid, -struct strbuf *referent,unsigned int *type) +struct strbuf *referent,unsigned int *type, +int *failure_errno) {const char *p;if (skip_prefix (buf,"ref:" , &buf)) {
@@ -486,7 +495,7 @@ int parse_loose_ref_contents(const char *buf, struct object_id *oid,if (parse_oid_hex (buf, oid, &p) || (*p !='\0' && !isspace (*p))) { *type |=REF_ISBROKEN ; - errno =EINVAL ; + *failure_errno =EINVAL ;return -1 ; }return 0 ;
refs/refs-internal.h +4 -2
@@ -692,10 +692,12 @@ struct ref_store { };/* -* Parse contents of a loose ref file. +* Parse contents of a loose ref file. *failure_errno maybe be set to EINVAL for +* invalid contents. */ int parse_loose_ref_contents (const char *buf,struct object_id *oid, -struct strbuf *referent,unsigned int *type); +struct strbuf *referent,unsigned int *type, +int *failure_errno);/* * Fill in the generic part of refs and add it to our collection of
t/t3200-branch.sh +1 -0
@@ -745,6 +745,7 @@ test_expect_success SYMLINKS 'git branch -m with symlinked .git/refs' ' ) && git --git-dir subdir/.git/ branch rename-src && git rev-parse rename-src >expect && + # Tests a BUG() assertion in files_read_raw_ref() git --git-dir subdir/.git/ branch -m rename-src rename-dest && git rev-parse rename-dest >actual && test_cmp expect actual &&
Signature · 1 line
2.33.0.662.g438caf9576d
PATCH v10 8/8 refs: make errno output explicit for refs_resolve_ref_unsafe
In reply to Ævar Arnfjörð BjarmasonFrom: Han-Wen Nienhuys <[email protected]>
This introduces refs_resolve_ref_unsafe_with_errno(), which makes the API contract for the errno output explicit. The implementation still relies on the global errno variable to ensure no side effects of this refactoring.
lock_ref_oid_basic() in files-backend.c is the only caller of refs_resolve_ref() that needs error information to make logic decisions, so update that caller
--- refs.c | 2 +- refs.h | 11 +++++++++++ refs/files-backend.c | 10 ++++++---- 3 files changed, 18 insertions(+), 5 deletions(-)
refs.c +1 -1
@@ -1688,7 +1688,7 @@ int refs_read_raw_ref(struct ref_store *ref_store, const char *refname, type, failure_errno); } -static const char *refs_resolve_ref_unsafe_with_errno(struct ref_store *refs, +const char *refs_resolve_ref_unsafe_with_errno(struct ref_store *refs, const char *refname, int resolve_flags, struct object_id *oid,
refs.h +11 -0
@@ -68,6 +68,17 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,int resolve_flags, struct object_id *oid, int *flags); +/** +* refs_resolve_ref_unsafe_with_errno() is like +* refs_resolve_ref_unsafe(), but provide access to errno code that +* lead to a failure. We guarantee that errno is set to a meaningful +* value on non-zero return. +*/ +const char *refs_resolve_ref_unsafe_with_errno (struct ref_store *refs, +const char *refname, +int resolve_flags, +struct object_id *oid, +int *flags,int *failure_errno);const char *resolve_ref_unsafe (const char *refname,int resolve_flags,struct object_id *oid,int *flags);
refs/files-backend.c +6 -4
@@ -1001,6 +1001,7 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, {struct strbuf ref_file =STRBUF_INIT ;struct ref_lock *lock; +int resolve_errno =0 ;files_assert_main_repository (refs,"lock_ref_oid_basic" );assert (err);
@@ -1008,13 +1009,14 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,CALLOC_ARRAY (lock,1 );files_ref_path (refs, &ref_file, refname); -if (!refs_resolve_ref_unsafe (&refs->base, refname, -RESOLVE_REF_NO_RECURSE , - &lock->old_oid, type)) { +if (!refs_resolve_ref_unsafe_with_errno (&refs->base, refname, +RESOLVE_REF_NO_RECURSE , + &lock->old_oid, type, + &resolve_errno)) {if (!refs_verify_refname_available (&refs->base, refname,NULL ,NULL , err))strbuf_addf (err,"unable to resolve reference '%s': %s" , - refname,strerror (errno)); + refname,strerror (resolve_errno)); goto error_return; }
Signature · 1 line
2.33.0.662.g438caf9576d