On Sat, Oct 25, 2008 at 11:31:01AM -0700, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> > +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.
> > 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.
> > + 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'.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
