Re: [PATCH v2 09/11] gitweb: git_is_head_detached() function

From: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
Date: Nov 14, 2008, 08:52 UTC
Message-ID: <cb7bb73a0811140052h1b7aac6cp6b0b376fa59548a9@mail.gmail.com>
In-Reply-To: <7vk5b6dd3t.fsf@gitster.siamese.dyndns.org>

On Fri, Nov 14, 2008 at 7:40 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 7 quoted lines

> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
>
>> The function checks if the HEAD for the current project is detached by
>> checking if 'git branch' returns "* (no branch)"
>
> This one looks more like "oops, the way detached HEAD is detected in 08 is
> sucky, let's cover it up by introducing a function as an afterthought."

Half and half: it _is_ an afterthought, and although it's sucky that's not the reason why I decided to refactor it 8-)

> Have a patch that introduces git_is_head_detached() first, and then use
> that function to implement the feature.  I personally think the user (that
> is, 08/11) is small and isolated enough that these two can be a single
> patch.

Will do. I did some patch squashing while preparing this set, but I forgot to do this one.

Show 11 quoted lines

>> +# check if current HEAD is detached
>> +sub git_is_head_detached {
>> +     my @x = (git_cmd(), 'branch');
>> +     my @ret = split("\n", qx(@x));
>> +     return 0 + grep { /^\* \(no branch\)$/ } @ret;
>> +}
>
> Do not read from Porcelain in scripts.
>
> "git symbolic-ref HEAD" should error out when your HEAD is detached, and
> will return refs/heads/frotz when you are on frotz branch.

Eh, I was, like, 100% sure this was the wrong way to do it, but I didn't have any idea on how to do it using the plumbing. Thanks, I'll look into that.

> But realistically speaking, what does it mean to have a detached HEAD in a
> repository published via gitweb?  First of all these things are supposed
> to be bare and there would be no checkout.

You know what's funny, when I first started working on the 'show remote branches in gitweb' patchset, you had a similar objection, but as you yourself pointed out

> But obviously people use gitweb/instaweb as a way to view their
> own live repository, and I think it makes sense to show and
> support remotes/ in such a case.  It also would make sense to
> support detached HEAD there as well.

http://kerneltrap.org/mailarchive/git/2007/8/30/256411

and that's exactly the reason why I added the part about the detached head 8-)

I have been thinking about making this detached HEAD thing an additional option, but it _really_ seemed like overkill.

-- 
Giuseppe "Oblomov" Bilotta