Re: [PATCH v2 08/11] gitweb: display HEAD in heads list when detached

[Jakub Narebski <jnareb@gmail.com>]

On Thu, 13 Nov 2008, Giuseppe Bilotta wrote:

First, I think that _at least_ the first two patches dealing with
detached should be squashed.

Second, not this way!


But I think that support for detached HEAD (I am not sure if it should
have to be explicitly turned on using some %feature, or reusing some
existing feature like 'remote_heads') is a very good idea. Especially
for git-instaweb.

> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
> ---
>  gitweb/gitweb.perl |   21 +++++++++++++++++++++
>  1 files changed, 21 insertions(+), 0 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 09728cb..a168f6f 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -2672,6 +2672,27 @@ sub git_get_heads_list {
>  	my @refs = map { "refs/$_" } @class;
>  	my @headslist;
>  
> +	if (grep { $_ eq 'heads' } @class) {

First, IMHO layering violation. Resolving detached HEAD should not be
in my opinion left to git_get_heads_list, which is wrapped around 
git-for-each-ref, which for some reason (contrary for example to
"git ls-remote ." or "git show-ref -h") doesn't show HEAD even if it
is detached. Probably misfeature / a bug in git-for-each-ref. I guess
that we should resolve detached HEAD in caller.

But I am not sure about this decision. Maybe instead of showing
detached HEAD (if it is detached) for 'heads', we should show it if
there is 'HEAD' in @class (well, @refs would have to be corrected,
too)?

> +		my @x = (git_cmd(), 'branch');
> +		my @ret = split("\n", qx(@x));

                                      ^^^^^^^- bit strange

Especially compared to almost everywhere else using open ... "-|"

> +		if (grep { /^\* \(no branch\)$/ } @ret) { ;

                                                          ^
                            WTF? -------------------------|

Second, if we go the route of manually resolving detached HEAD, 
instead of adding [-h|--head] (from git-show-ref) option equivalent
to git-for-each-ref, which would work only for detached HEAD (fixing
kind of a bug), this is *not* the way to do it.

A. It should be done using encapsulation, adding is_HEAD_detached() or
git_is_head_detached() subroutine (squashing the next patch), or
simply using !defined($current_branch), where $current_branch would
be set using git_get_symbolic_ref('HEAD') or something.

B. Using porcelain, especially end-user porcelain such as git-branch,
which can change its output format (because they are porcelain). Use
equivalent plumbing, be if git-symbolic-ref ("git symbolic-ref -q HEAD"
to be more exact) to get current branch name[1], or just simply do
that in Perl: check if it is symlink, or if it starts with "ref: "
if it is regular file (IIRC HEAD, even detached HEAD, cannot get
packed into .git/packed-refs and deleted... at least I think so).

[1] This means that we have better way of detecting (and showing)
which branch is current one than comparing sha1 with resolved HEAD.
($head_hash).

> +			my %ref_item;
> +			@x = (git_cmd(), 'log', '-1', '--pretty=format:%H%n%ct%n%s');
> +			my ($hash, $epoch, $title) = split("\n", qx(@x), 3);

Errr... if we don't fix git-for-each-ref, and go that route, why not
simply use parse_commit subroutine, and extract relevant info from
there, instead of handcrafting git-log (why not git-show?) call?

You get more info than needed, but I think the cost of getting it is
almost the same, and you can reuse existing code.

And if we go --pretty=format:<...> or --pretty=tformat:<...> route for
git-log, git-rev-list or git-show, wouldn't it be possible to generate
the same output format as git-for-each-ref below?

> +
> +			$ref_item{'class'} = 'head';
> +			$ref_item{'name'} = 'HEAD';
> +			$ref_item{'id'} = $hash;
> +			$ref_item{'title'} = $title || '(no commit message)';
> +			if ($ref_item{'epoch'} = $epoch) {
> +				$ref_item{'age'} = age_string(time - $ref_item{'epoch'});

Hmmm...

> +			} else {
> +				$ref_item{'age'} = "unknown";
> +			}
> +			push @headslist, \%ref_item;
> +		}
> +	}
> +
>  	open my $fd, '-|', git_cmd(), 'for-each-ref',
>  		($limit ? '--count='.($limit+1) : ()), '--sort=-committerdate',
>  		'--format=%(objectname) %(refname) %(subject)%00%(committer)',
> -- 
> 1.5.6.5
> 
> 

-- 
Jakub Narebski
Poland