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


> 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.


> 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 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

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

Reply via email to