git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Ronnie Sahlberg <sahlberg@google.com>
Date
Jul 22, 2014, 18:19 UTC
Message-ID
<CAL=YDWnoCqEAN8+XPiVgPqUazAbzKG2oedLGBtEwPGCJMm_ctg@mail.gmail.com>
In-Reply-To
<xmqqd2d2l2o7.fsf@gitster.dls.corp.google.com>
On Fri, Jul 18, 2014 at 3:31 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 9 quoted lines
> 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>

Comments/opinions?

regards ronnie sahlberg

Show 20 quoted lines
>
>> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
>> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
>> ---
>>  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))) {
Previous: Junio C HamanoNext: Ronnie Sahlberg
Message 13 of 26 in “Use ref transactions part 3”
  1. 00/12 Use ref transactions part 3Ronnie Sahlberg, Jul 16, 2014
  2. 01/12 wrapper.c: simplify warn_if_unremovableRonnie Sahlberg, Jul 16, 2014
  3. Junio C HamanoJul 18, 2014
  4. 02/12 wrapper.c: add a new function unlink_or_msgRonnie Sahlberg, Jul 16, 2014
  5. Junio C HamanoJul 18, 2014
  6. Junio C HamanoJul 18, 2014
  7. Ronnie SahlbergJul 22, 2014
  8. Junio C HamanoJul 22, 2014
  9. 03/12 refs.c: add an err argument to delete_ref_looseRonnie Sahlberg, Jul 16, 2014
  10. 04/12 refs.c: pass the ref log message to _create/delete/update instead of _commitRonnie Sahlberg, Jul 16, 2014
  11. 05/12 refs.c: pass NULL as *flags to read_ref_fullRonnie Sahlberg, Jul 16, 2014
  12. Junio C HamanoJul 18, 2014
  13. Ronnie SahlbergJul 22, 2014
  14. Ronnie SahlbergJul 22, 2014
  15. 06/12 refs.c: move the check for valid refname to lock_ref_sha1_basicRonnie Sahlberg, Jul 16, 2014
  16. Junio C HamanoJul 18, 2014
  17. 07/12 refs.c: call lock_ref_sha1_basic directly from commitRonnie Sahlberg, Jul 16, 2014
  18. 08/12 refs.c: pass a skip list to name_conflict_fnRonnie Sahlberg, Jul 16, 2014
  19. 09/12 refs.c: propagate any errno==ENOTDIR from _commit back to the callersRonnie Sahlberg, Jul 16, 2014
  20. 10/12 fetch.c: change s_update_ref to use a ref transactionRonnie Sahlberg, Jul 16, 2014
  21. 11/12 refs.c: make write_ref_sha1 staticRonnie Sahlberg, Jul 16, 2014
  22. 12/12 refs.c: fix handling of badly named refsRonnie Sahlberg, Jul 16, 2014
  23. Junio C HamanoJul 22, 2014
  24. Ronnie SahlbergJul 22, 2014
  25. Ronnie SahlbergJul 22, 2014
  26. Junio C HamanoJul 22, 2014

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.