Re: [PATCH v20 43/48] refs.c: move the check for valid refname to lock_ref_sha1_basic

2014-08-22 Thread Michael Haggerty
On 08/20/2014 11:47 PM, Ronnie Sahlberg wrote: [...] Since we already display broken/unresolvable refs, I think the most consistent path is to also allow showing the refs broken/illegal-names too in the list. (when DO_FOR_EACH_INCLUDE_BROKEN is specified) Of course, an end user could fix this

Re: [PATCH v20 43/48] refs.c: move the check for valid refname to lock_ref_sha1_basic

2014-08-21 Thread Ronnie Sahlberg
On Wed, Aug 20, 2014 at 11:34 AM, Michael Haggerty mhag...@alum.mit.edu wrote: On 08/20/2014 06:28 PM, Ronnie Sahlberg wrote: On Wed, Aug 20, 2014 at 7:52 AM, Michael Haggerty mhag...@alum.mit.edu wrote: I'm a little worried that abandoning *all* refname checks could open us up to somehow

Re: [PATCH v20 43/48] refs.c: move the check for valid refname to lock_ref_sha1_basic

2014-08-20 Thread Michael Haggerty
On 07/15/2014 10:58 PM, Ronnie Sahlberg wrote: On Tue, Jul 15, 2014 at 12:34 PM, Ronnie Sahlberg sahlb...@google.com wrote: On Tue, Jul 15, 2014 at 11:04 AM, Jonathan Nieder jrnie...@gmail.com wrote: Michael Haggerty wrote: So...I like the idea of enforcing refname checks at the lowest level

Re: [PATCH v20 43/48] refs.c: move the check for valid refname to lock_ref_sha1_basic

2014-08-20 Thread Ronnie Sahlberg
On Wed, Aug 20, 2014 at 7:52 AM, Michael Haggerty mhag...@alum.mit.edu wrote: On 07/15/2014 10:58 PM, Ronnie Sahlberg wrote: On Tue, Jul 15, 2014 at 12:34 PM, Ronnie Sahlberg sahlb...@google.com wrote: On Tue, Jul 15, 2014 at 11:04 AM, Jonathan Nieder jrnie...@gmail.com wrote: Michael

Re: [PATCH v20 43/48] refs.c: move the check for valid refname to lock_ref_sha1_basic

2014-08-20 Thread Jonathan Nieder
Hi, Ronnie Sahlberg wrote: On Wed, Aug 20, 2014 at 7:52 AM, Michael Haggerty mhag...@alum.mit.edu wrote: I'm a little worried that abandoning *all* refname checks could open us up to somehow trying to delete a reference with a name like ../../../../etc/passwd. Either such names have to be

Re: [PATCH v20 43/48] refs.c: move the check for valid refname to lock_ref_sha1_basic

2014-08-20 Thread Ronnie Sahlberg
On Wed, Aug 20, 2014 at 10:49 AM, Jonathan Nieder jrnie...@gmail.com wrote: Hi, Ronnie Sahlberg wrote: On Wed, Aug 20, 2014 at 7:52 AM, Michael Haggerty mhag...@alum.mit.edu wrote: I'm a little worried that abandoning *all* refname checks could open us up to somehow trying to delete a

Re: [PATCH v20 43/48] refs.c: move the check for valid refname to lock_ref_sha1_basic

2014-08-20 Thread Michael Haggerty
On 08/20/2014 06:28 PM, Ronnie Sahlberg wrote: On Wed, Aug 20, 2014 at 7:52 AM, Michael Haggerty mhag...@alum.mit.edu wrote: I'm a little worried that abandoning *all* refname checks could open us up to somehow trying to delete a reference with a name like ../../../../etc/passwd. Either

Re: [PATCH v20 43/48] refs.c: move the check for valid refname to lock_ref_sha1_basic

2014-08-20 Thread Junio C Hamano
Michael Haggerty mhag...@alum.mit.edu writes: I think we can get away with not including broken refnames when iterating. After all, the main goal of tolerating them is to let them be deleted, right? Or read from a ref whose name has retroactively made invalid, in order to create a similar

Re: [PATCH v20 43/48] refs.c: move the check for valid refname to lock_ref_sha1_basic

2014-08-20 Thread Michael Haggerty
On 08/20/2014 09:45 PM, Junio C Hamano wrote: Michael Haggerty mhag...@alum.mit.edu writes: I think we can get away with not including broken refnames when iterating. After all, the main goal of tolerating them is to let them be deleted, right? Or read from a ref whose name has

Re: [PATCH v20 43/48] refs.c: move the check for valid refname to lock_ref_sha1_basic

2014-08-20 Thread Junio C Hamano
Michael Haggerty mhag...@alum.mit.edu writes: On 08/20/2014 09:45 PM, Junio C Hamano wrote: Michael Haggerty mhag...@alum.mit.edu writes: I think we can get away with not including broken refnames when iterating. After all, the main goal of tolerating them is to let them be deleted,

Re: [PATCH v20 43/48] refs.c: move the check for valid refname to lock_ref_sha1_basic

2014-08-20 Thread Ronnie Sahlberg
On Wed, Aug 20, 2014 at 1:11 PM, Michael Haggerty mhag...@alum.mit.edu wrote: On 08/20/2014 09:45 PM, Junio C Hamano wrote: Michael Haggerty mhag...@alum.mit.edu writes: I think we can get away with not including broken refnames when iterating. After all, the main goal of tolerating them is

Re: [PATCH v20 43/48] refs.c: move the check for valid refname to lock_ref_sha1_basic

2014-07-15 Thread Ronnie Sahlberg
On Tue, Jul 8, 2014 at 8:02 AM, Michael Haggerty mhag...@alum.mit.edu 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.

Re: [PATCH v20 43/48] refs.c: move the check for valid refname to lock_ref_sha1_basic

2014-07-15 Thread Jonathan Nieder
Michael Haggerty wrote: 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

Re: [PATCH v20 43/48] refs.c: move the check for valid refname to lock_ref_sha1_basic

2014-07-15 Thread Jonathan Nieder
Ronnie Sahlberg wrote: 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

Re: [PATCH v20 43/48] refs.c: move the check for valid refname to lock_ref_sha1_basic

2014-07-15 Thread Junio C Hamano
Jonathan Nieder jrnie...@gmail.com writes: How to take care of the recovery use case is another question. FWIW I also would prefer if git update-ref -d or git branch -D could be used to delete corrupt refs instead of having to use fsck (since a fsck run can take a while), but that's a

Re: [PATCH v20 43/48] refs.c: move the check for valid refname to lock_ref_sha1_basic

2014-07-15 Thread Ronnie Sahlberg
On Tue, Jul 15, 2014 at 11:04 AM, Jonathan Nieder jrnie...@gmail.com wrote: Michael Haggerty wrote: 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

Re: [PATCH v20 43/48] refs.c: move the check for valid refname to lock_ref_sha1_basic

2014-07-15 Thread Ronnie Sahlberg
On Tue, Jul 15, 2014 at 11:34 AM, Junio C Hamano gits...@pobox.com wrote: Jonathan Nieder jrnie...@gmail.com writes: How to take care of the recovery use case is another question. FWIW I also would prefer if git update-ref -d or git branch -D could be used to delete corrupt refs instead of

Re: [PATCH v20 43/48] refs.c: move the check for valid refname to lock_ref_sha1_basic

2014-07-15 Thread Ronnie Sahlberg
On Tue, Jul 15, 2014 at 12:34 PM, Ronnie Sahlberg sahlb...@google.com wrote: On Tue, Jul 15, 2014 at 11:04 AM, Jonathan Nieder jrnie...@gmail.com wrote: Michael Haggerty wrote: So...I like the idea of enforcing refname checks at the lowest level possible, but I think that the change you

Re: [PATCH v20 43/48] refs.c: move the check for valid refname to lock_ref_sha1_basic

2014-07-08 Thread Michael Haggerty
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