Re: [PATCH 1/2] Fix git branch -m for symrefs.

[Junio C Hamano <gitster@pobox.com>]

Miklos Vajna <vmiklos@frugalware.org> writes:

> -int delete_ref(const char *refname, const unsigned char *sha1)
> +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;
>  
>  	lock = lock_ref_sha1_basic(refname, sha1, 0, &flag);
>  	if (!lock)
>  		return 1;
>  	if (!(flag & REF_ISPACKED)) {

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

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?

> @@ -964,10 +971,15 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)
>  	struct ref_lock *lock;
>  	struct stat loginfo;
>  	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".

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

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.

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.