Re: [PATCH 06/12] refs.c: move the check for valid refname to lock_ref_sha1_basic

2014-07-18 Thread Junio C Hamano 
Ronnie Sahlberg sahlb...@google.com writes:

 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.

That might be sensible if our only goal were to remove
lock-any-ref-for-updates, but I wonder what the impact of this
change to other existing callers of lock-ref-sha1-basic.  I may be
recalling things incorrectly, but I suspect that it was deliberate
to keep the lowest-level internal helper function (i.e. _basic()) to
be lenient so that those who do not want the format checks can
choose to pass refnames that are not exactly kosher.

 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.

But fsck is about checking and never about recovering, isn't it?
Does it offer to remove misnamed refs?  Should it?



 Signed-off-by: Ronnie Sahlberg sahlb...@google.com
 ---
  refs.c | 7 +--
  1 file changed, 5 insertions(+), 2 deletions(-)

 diff --git a/refs.c b/refs.c
 index 0df6894..f29f18a 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);
  }
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 06/12] refs.c: move the check for valid refname to lock_ref_sha1_basic

2014-07-16 Thread Ronnie Sahlberg 
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
any codepaths that tries to open and repair badly named refs. The normal refs
API should not allow neither creating nor accessing refs with invalid names.
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.

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 refs.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/refs.c b/refs.c
index 0df6894..f29f18a 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);
 }
 
-- 
2.0.1.527.gc6b782e

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html