Ronnie Sahlberg [off-list ref] 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? Would it
want to report something when the ref to be overwritten was a broken
one?
quoted hunk
Reviewed-by: Jonathan Nieder <redacted>
Signed-off-by: Ronnie Sahlberg <redacted>
---
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))) {
On Fri, Jul 18, 2014 at 3:31 PM, Junio C Hamano [off-list ref] wrote:
Ronnie Sahlberg [off-list ref] writes:
quoted
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
quoted
Reviewed-by: Jonathan Nieder <redacted>
Signed-off-by: Ronnie Sahlberg <redacted>
---
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))) {
On Tue, Jul 22, 2014 at 11:19 AM, Ronnie Sahlberg [off-list ref] wrote:
On Fri, Jul 18, 2014 at 3:31 PM, Junio C Hamano [off-list ref] wrote:
quoted
Ronnie Sahlberg [off-list ref] writes:
quoted
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.
quoted
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.