Re: [PATCH 1/2] Fix git branch -m for symrefs.
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:45:31
Possibly related (same subject, not in this thread)
- 2016-06-15 · [PATCH 1/2] Fix git branch -m for symrefs. · Miklos Vajna <hidden>
Miklos Vajna [off-list ref] 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?
quoted hunk
@@ -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.