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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 25, 2008, 18:31 UTC
Message-ID: <7v8wsca5ne.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <a96243124c555cbc4081f733b348252ac200bd53.1224939436.git.vmiklos@frugalware.org>

Miklos Vajna <vmiklos@frugalware.org> writes:

Show 12 quoted lines

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

Show 9 quoted lines

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

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

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.