On Tue, Jul 8, 2014 at 8:02 AM, Michael Haggerty <mhagger@xxxxxxxxxxxx> wrote:
> 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 think the repair things are already busted since a while, so I don't
think this will make things worse,
but I will change it to make it better than this patch and better than
current master, please see below.
<current git>
$ cp .git/refs/heads/master .git/refs/heads/master.....@\*@\\.
$ git branch
fatal: Reference has invalid format: 'refs/heads/master.....@*@\.'
$ git branch -D master.....@\*@\\.
error: branch 'master.....@*@\.' not found.
git fsck does report that there is a problem with a broken ref
fatal: Reference has invalid format: 'refs/heads/master.....@*@\.'
But I don't think there is any way to fix this other than manually
deleting the file from the command line.
(this is because we need to do a resolve_ref_unsafe which will not
work and if we can not do a resolve_ref_unsafe we can not delete the
ref,
there is also the issue where we can not read the ref into ref-cache)
What I suggest doing here is to create a new patch towards the end of
the series that will :
* change the resolve_ref_unsafe(... , int reading, ...) argument to be
a bitmask of flags with
#define RESOLVE_REF_READING 0x01 (== current flag)
#define RESOLVE_REF_ALLOW_BAD_NAME 0x02 (== disable checking the
refname format during resolve)
* then provide the flag for RESOLVE_REF_ALLOW_BAD_NAME for the cases
where we try to resolve a ref in builtin/branch.c where we try to
delete a ref
* then also pass the same flag to lock_ref_sha1_basic when we are
deleting a ref from transaction_commit so we can disable the "check
refname" check there too.
I think that is all that is needed but I will see if there are
additional changes needed once I implement it.
This will mean that the semantics will become :
* you can not create or access a branch with a bad name since both
resolving it and locking it will fail.
* but you can always delete a branch regardless of whether the name is
good or bad.
(this will also mean that you will be able to rename a bad branch name
to a new good name)
which I think would be pretty sane semantics.
I will implement these changes as a separate patch.
Comments?
regards
ronnie sahlberg
> 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