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@xxxxxxxxx>
> ---
> 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
--
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
