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>
Comments/opinions?
regards
ronnie sahlberg
>
>> Reviewed-by: Jonathan Nieder <jrnieder@xxxxxxxxx>
>> Signed-off-by: Ronnie Sahlberg <sahlberg@xxxxxxxxxx>
>> ---
>> refs.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/refs.c b/refs.c
>> index 7d65253..0df6894 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -2666,7 +2666,7 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
>> goto rollback;
>> }
>>
>> - if (!read_ref_full(newrefname, sha1, 1, &flag) &&
>> + if (!read_ref_full(newrefname, sha1, 1, NULL) &&
>> delete_ref(newrefname, sha1, REF_NODEREF)) {
>> if (errno==EISDIR) {
>> if (remove_empty_directories(git_path("%s", newrefname))) {
--
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