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
> Signed-off-by: Ronnie Sahlberg <sahlberg@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html