[PATCH 0/3] symref rename/delete fixes

From: Miklos Vajna <vmiklos@frugalware.org>
Date: Oct 26, 2008, 02:33 UTC
Message-ID: <cover.1224987944.git.vmiklos@frugalware.org>
In-Reply-To: <7v8wsca5ne.fsf@gitster.siamese.dyndns.org>

On Sat, Oct 25, 2008 at 11:31:01AM -0700, Junio C Hamano <gitster@pobox.com> wrote:

Show 7 quoted lines

> > +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.

Show 12 quoted lines

> >     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.

Show 10 quoted lines

> > +   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.

Show 7 quoted lines

> 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].

Show 5 quoted lines

> 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'.