Re: [topgit] tg update error

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 12, 2009, 21:01 UTC
Message-ID: <7veiy3l689.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <20090212125621.GB5397@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 5 quoted lines

> Junio, I think we should probably revert b229d18 (and loosen
> symbolic-ref's check to just "refs/"). Even if you want to argue that
> topgit should be changed to handle this differently, we are still
> breaking existing topgit installations, and who knows what other scripts
> which might have relied on doing something like this.

I'm Ok with the revert (and I agree it is absolutely the right thing to do at least for the short term).

But I still do agree with the reasoning for the change stated in its commit log message:

    commit b229d18a809c169314b7f0d048dc5a7632e8f916
    Author: Jeff King <peff@peff.net>
    Date:   Thu Jan 29 03:30:16 2009 -0500

        validate_headref: tighten ref-matching to just branches

        When we are trying to determine whether a directory contains
        a git repository, one of the tests we do is to check whether
        HEAD is either a symlink or a symref into the "refs/"
        hierarchy, or a detached HEAD.

        We can tighten this a little more, though: a non-detached
        HEAD should always point to a branch (since checking out
        anything else should result in detachment), so it is safe to
        check for "refs/heads/".

        Signed-off-by: Jeff King <peff@peff.net>
        Signed-off-by: Junio C Hamano <gitster@pobox.com>

It would be nice to hear TopGit people defend why setting HEAD to outside refs/heads/ is justified, why doing so should not break other things, and why it was needed.

The last one is particularly important to avoid this kind of issue in the future. Perhaps they _knew_ some things refuse to work on refs outside refs/heads/ and wanted to take advantage of that fact to protect their own refs from vanilla git tools, but if that really is the case, the rules they want have to be spelled out.

"git checkout" would refuse to switch to "refs/top-bases/frotz", because it currently considers HEAD pointing outside refs/heads/ is insane, for example. But the revert of the above commit *means* that it is not insane, and somebody may add an option to switch to any refs inside refs/ hierarchy. If the reason TopGit points HEAD outside refs/heads hierarchy were because they assume "git checkout" would never do so, such a change would break them again (I am not seriously suggesting to add such an option to "git checkout", but I am just using it to illustrate the point. We would not know what other assumption, warranted or unwarranted, it is making).