[PATCH 0/3] symref rename/delete fixes

On Sat, Oct 25, 2008 at 11:31:01AM -0700, Junio C Hamano [off-list ref] wrote:

quoted

+int delete_ref(const char *refname, const unsigned char *sha1, int
flags)
 {
    struct ref_lock *lock;
-   int err, i, ret = 0, flag = 0;
+   int err, i = 0, ret = 0, flag = 0;
+   char *path;

Two variables flag vs flags is a bit confusing, isn't it?  How about
naming the new one "delopt" or something?

Renamed.

The new variable "char *path" at the toplevel can be confined in the
scope
of this if () {} block and probably can become "const char *", right?

Yes - moved.

quoted

    int log = !lstat(git_path("logs/%s", oldref), &loginfo);
+   const char *symref = NULL;
+   int is_symref = 0;

    if (S_ISLNK(loginfo.st_mode))
            return error("reflog for %s is a symlink", oldref);

Possible bug in the context.  When there is no reflog for the ref
being
renamed, lstat would fail; it doesn't feel right to have this
S_ISLNK()
before checking the result of the lstat which is in "log".

OK, this went to a separate patch.

quoted

+   symref = resolve_ref(oldref, orig_sha1, 0, &flag);
+   if (flag & REF_ISSYMREF)
+           is_symref = 1;
    if (!resolve_ref(oldref, orig_sha1, 1, &flag))
            return error("refname %s not found", oldref);

Do we really need two calls to resolve_ref()?  Your new call calls it
without must-exist bit --- why?  Immediately after that, the existing
call
will barf if it does not exist anyway.

Just having

        if (!symref)
                return error("refname %s not found", oldref);

first looks weird, given that the error message is not "refname %s is
not a symref", but you are right, changed.

I agree it is good to have symref aware delete_ref(), but I am not
sure
supporting symref in rename_ref() is either needed or necessarily a
good
idea.  You also need to worry about a symref pointing at a branch yet
to
be born.

That is currently not supported and the error message of 'git branch -m'
is (in case foo points to refs/heads/bar and bar is not yet born):

error: refname refs/heads/foo not found
fatal: Branch rename failed

which is quite acceptable, IMHO[1].

In the meantime, I think we should just check (flag & REF_ISSYMREF)
after
the existing resolve_ref() we can see in the context above, and error
out
saying you cannot rename a symref, and do nothing else.

A symref-aware rename_ref() is needed by git remove rename, since it
typically does origin/HEAD -> upstream/HEAD symref renames there.

Of course you can say that this should be handled by git-remote itself,
without using rename_ref() but that not seem to be a good solution to
me. (Workaround in the wrong layer, instead of a solution in a good
one.)

[1] I mean, I have a real-world scenario (git remove rename) for "why
renaming a symref is a good idea", but I don't think renaming a ref
pointing to a yet-to-be-born ref has any real world users.

Miklos Vajna (3):
  Fix git branch -m for symrefs.
  rename_ref(): handle the case when the reflog of a ref does not exist
  Fix git update-ref --no-deref -d.

 builtin-branch.c       |    2 +-
 builtin-receive-pack.c |    2 +-
 builtin-remote.c       |    4 +-
 builtin-reset.c        |    2 +-
 builtin-send-pack.c    |    2 +-
 builtin-tag.c          |    2 +-
 builtin-update-ref.c   |    8 ++++--
 cache.h                |    2 +-
 refs.c                 |   61 +++++++++++++++++++++++++++++------------------
 t/t1400-update-ref.sh  |    7 +++++
 t/t3200-branch.sh      |    9 +++++++
 11 files changed, 67 insertions(+), 34 deletions(-)

Also available in the 'symref-mv' branch of 'git://repo.or.cz/git/vmiklos.git'.