On Tue, Jul 22, 2014 at 11:19 AM, Ronnie Sahlberg <sahlberg@xxxxxxxxxx> wrote: > On Fri, Jul 18, 2014 at 3:31 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote: >> Ronnie Sahlberg <sahlberg@xxxxxxxxxx> 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> > I have a patch to make it possible to delete a broken ref that can not be resolved in : https://github.com/rsahlberg/git/commit/763ab16e1874d58a4fc5c37920abc1ea40ccd814 This patch is scheduled at the end of the next patch series (use transactions for all reflog updates) I plan to send out tomorrow. -- 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