Re: [PATCH v20 43/48] refs.c: move the check for valid refname to lock_ref_sha1_basic
From: Michael Haggerty <hidden>
Date: 2016-06-15 23:01:50
On 06/20/2014 04:43 PM, Ronnie Sahlberg wrote:
Move the check for check_refname_format from lock_any_ref_for_update to lock_ref_sha1_basic. At some later stage we will get rid of lock_any_ref_for_update completely. If lock_ref_sha1_basic fails the check_refname_format test, set errno to EINVAL before returning NULL. This to guarantee that we will not return an error without updating errno. This leaves lock_any_ref_for_updates as a no-op wrapper which could be removed. But this wrapper is also called from an external caller and we will soon make changes to the signature to lock_ref_sha1_basic that we do not want to expose to that caller. This changes semantics for lock_ref_sha1_basic slightly. With this change it is no longer possible to open a ref that has a badly name which breaks
s/badly name/bad name,/
any codepaths that tries to open and repair badly named refs. The normal refs
s/tries/try/
API should not allow neither creating nor accessing refs with invalid names.
s/not allow neither/allow neither/
If we need such recovery code we could add it as an option to git fsck and have git fsck be the only sanctioned way of bypassing the normal API and checks.
I like the sentiment, but in the real world I'm not sure we can take such a step based only on good intentions. Which callers would be affected? Where is this "git fsck" code that would be needed to help people rescue their repos? I can also imagine that we will tighten up the check_refname_format checks in the future; for example, I think it would be a good idea to prohibit reference names that start with '-' because it is almost impossible to work with them (their names look like command-line options). If we ever make a change like that, we will need some amount of tolerance in git versions around the transition. So...I like the idea of enforcing refname checks at the lowest level possible, but I think that the change you propose is too abrupt. I think it needs either more careful analysis showing that it won't hurt anybody, or some kind of tooling or non-strict mode that people can use to fix their repositories. Michael
quoted hunk ↗ jump to hunk
Signed-off-by: Ronnie Sahlberg <redacted> --- refs.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)diff --git a/refs.c b/refs.c index 389a55f..bccf8c3 100644 --- a/refs.c +++ b/refs.c@@ -2088,6 +2088,11 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname, int missing = 0; int attempts_remaining = 3; + if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) { + errno = EINVAL; + return NULL; + } + lock = xcalloc(1, sizeof(struct ref_lock)); lock->lock_fd = -1;@@ -2179,8 +2184,6 @@ 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)) - return NULL; return lock_ref_sha1_basic(refname, old_sha1, flags, type_p); }
-- Michael Haggerty mhagger@alum.mit.edu