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

On 07/15/2014 10:58 PM, Ronnie Sahlberg wrote:

On Tue, Jul 15, 2014 at 12:34 PM, Ronnie Sahlberg [off-list ref] wrote:

quoted

On Tue, Jul 15, 2014 at 11:04 AM, Jonathan Nieder [off-list ref] wrote:

quoted

Michael Haggerty wrote:

quoted

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.

The recovery code has been broken for a while, so I don't see harm
in this change.

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 question for a later series.

In an ideal world, the low-level functions would allow *reading* and
*deleting* poorly named refs (even without any special flag) but not
creating them.  Is that doable?

That should be doable. Ill add these repairs as 3-4 new patches at the
end of the current patch series.

quoted

The main complication I can see is
iteration: would iteration skip poorly named refs and warn, or would
something more complicated be needed?

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?  Then as long as iteration is not needed to implement
the command "git update-ref -d", it seems to me that it is OK if
iterating over a reference with a broken name causes a die().

quoted

Right now,  "git branch"  will error and abort right away when it
finds the first bad ref. I haven't checked the exact spot it does this
yet but I suspect it is when building the ref-cache.
I will solve the cases for create and delete for now.


What/how to handle iterables will require more thought.
Right now, since these refs will be filtered out and never end up in
ref-cache, either from loose refs or from packed refs
it does mean that anyone that uses an iterator is guaranteed to only
get refs with valid names passed back to them.
We would need to audit all code that uses iterators and make sure it
can handle the case where the callback is suddenly
invoked with a bad refname.

quoted

Thanks,
Jonathan

The following seems to do the trick to allow deleting a bad ref. We
would need something for the iterator too.
Since this touches the same areas that my ~100 other ref transaction
patches that are queued up do, I
would like to wait applying this patch until once the next few series
are finished and merged.
(to avoid having to do a lot of rebases and fix legio of merge
conflicts that this would introduce in my branches).


Treat this as an approximation on what the fix to repair git branch -D
will look like once the time comes.

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 prohibited
somehow, or we have to be very sure that they can only come from trusted
sources.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu