Re: [PATCH 05/12] refs.c: pass NULL as *flags to read_ref_full

> On Fri, Jul 18, 2014 at 3:31 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Ronnie Sahlberg <sahlberg@google.com> writes:
>>
>>> We call read_ref_full with a pointer to flags from rename_ref but since
>>> we never actually use the returned flags we can just pass NULL here instead.
>>
>> Sensible, at least for the current callers.  I had to wonder if
>> rename_ref() would never want to take advantage of the flags return
>> parameter in the future, though.  For example, would it want to act
>> differently when the given ref turns out to be a symref?
>
> I don't know.
>
> We have a check if the old refname was a symref or not since the old
> version did not have code for how to handle renaming the reflog.
> (That check is removed in a later series when we have enough
> transaction code and reflog api changes so that we no longer need to
> call rename() for the reflog handling.)
>
> I can not think of any reason right now why, but if we need it we can
> add the argument back when the need arises.
>
>> Would it
>> want to report something when the ref to be overwritten was a broken
>> one?
>
> Good point.
>
> There are two cases where the new ref could be broken.
> 1) It could either contain a broken SHA1, like if we do this :
> $ echo "Broken ref" > .git/refs/heads/foo-broken-1
> 2) or it could be broken due to having a bad/invalid name :
> $ cp .git.refs.heads.master .git/refs/heads/foo-broken-1-\*...
>
> For 2) I think this should not be allowed so the rename should just
> fail with something like :
> $ ./git branch -M foo foo-broken-1-\*...
> fatal: 'foo-broken-1-*...' is not a valid branch name.
>
> For 1)  if the new branch already exists but it has a broken SHA1, for
> that case I think we should allow rename_ref to overwrite the existing
> bad SHA1 with the new, good, SHA1 value.
> Currently this does not work in master :
> $ echo "Broken ref" > .git/refs/heads/foo-broken-1
> $ ./git branch -m foo foo-broken-1
> error: unable to resolve reference refs/heads/foo-broken-1: Invalid argument
> error: unable to lock refs/heads/foo-broken-1 for update
> fatal: Branch rename failed
>
>
> And the only way to recover is to first delete the branch as my other
> patch in this series now allows and then trying the rename again.
>
> For 1), since we are planning to overwrite the current branch with a
> new SHA1 value, I think that what makes most sense would be to treat
> the "branch exist but is broken" as if the branch did not exist at all
> and just allow overwriting it with the new good value.
>
>
>
> Currently this does not work in master :
>
> $ echo "Broken ref" > .git/refs/heads/foo-broken-1
> $ ./git branch -m foo foo-broken-1
> error: unable to resolve reference refs/heads/foo-broken-1: Invalid argument
> error: unable to lock refs/heads/foo-broken-1 for update
> fatal: Branch rename failed
> so since this is not a regression I will not update this particular
> patch series but instead I
> can add a new patch to the next patch series to allow this so that we can do :
> $ echo "Broken ref" > .git/refs/heads/foo-broken-1
> $ ./git branch -m foo foo-broken-1
> <success>
>