From: Giuseppe Bilotta <hidden> Date: 2016-06-15 22:45:12
This patch turns ref markers for tags and heads into links to
appropriate views for the ref name. Appropriate changes are made in the
CSS to prevent ref markers to be annoyingly blue and underlined.
For all git ref types it's assumed that the preferred view is named like
the ref type itself. For commits, we map the view to shortlog.
Signed-off-by: Giuseppe Bilotta <redacted>
---
This is a resend of version 2 of the patch, as it seems to have
dropped into oblivion without ACKs or NACKs.
gitweb/gitweb.css | 5 +++++
gitweb/gitweb.perl | 7 ++++++-
2 files changed, 11 insertions(+), 1 deletions(-)
@@ -1093,10 +1093,14 @@ sub format_log_line_html {subformat_ref_marker{my($refs,$id)=@_;my$markers='';+my%view=(+"commit"=>"shortlog",+);if(defined$refs->{$id}){foreachmy$ref(@{$refs->{$id}}){my($type,$name)=qw();+my$git_type=git_get_type($ref);# e.g. tags/v2.6.11 or heads/nextif($ref=~m!^(.*?)s?/(.*)$!){$type=$1;
@@ -1107,7 +1111,8 @@ sub format_ref_marker {}$markers.=" <span class=\"$type\" title=\"$ref\">".-esc_html($name)."</span>";+$cgi->a({-href=>href(action=>$view{$git_type}||$git_type,hash=>$name)},$name).+"</span>";}}
From: Jakub Narebski <hidden> Date: 2016-06-15 22:45:12
On Thu, 21 Aug 2008, Giuseppe Bilotta wrote:
This patch turns ref markers for tags and heads into links to
appropriate views for the ref name. Appropriate changes are made in the
CSS to prevent ref markers to be annoyingly blue and underlined.
For all git ref types it's assumed that the preferred view is named like
the ref type itself. For commits, we map the view to shortlog.
Possible improvement:
We would probably want to make this link discoverable, by adding
underline on :hover, like for other "hidden links" in gitweb (for
example in commitdiff view).
@@ -1093,10 +1093,14 @@ sub format_log_line_html {subformat_ref_marker{my($refs,$id)=@_;my$markers='';+my%view=(+"commit"=>"shortlog",+);if(defined$refs->{$id}){foreachmy$ref(@{$refs->{$id}}){my($type,$name)=qw();+my$git_type=git_get_type($ref);# e.g. tags/v2.6.11 or heads/nextif($ref=~m!^(.*?)s?/(.*)$!){$type=$1;
git_get_type calls 'git cat-file -t', so for each ref shown you make
*additional call* to git command (additional fork). Not good, especially
that you can get information if a ref is a tag (indirect reference)
or not one can get from within git_get_references; which in turn
uses "git show-refs --dereference" and used to use either
"git peek-remote ." or ".git/info/refs" file. If there is <name>^{},
then <name> is indirect reference: is a tag.
As we display ref markers only for log-like views, marker can be tag
or can be "lightweight reference" and be only a commit (in theory
we could show ref markers also for tree and blob items, but it is not
important now).
--
Jakub Narebski
Poland
Possible improvement:
We would probably want to make this link discoverable, by adding
underline on :hover, like for other "hidden links" in gitweb (for
example in commitdiff view).
Can do that.
quoted
my ($type, $name) = qw();
+ my $git_type = git_get_type($ref);
# e.g. tags/v2.6.11 or heads/next
if ($ref =~ m!^(.*?)s?/(.*)$!) {
$type = $1;
git_get_type calls 'git cat-file -t', so for each ref shown you make
*additional call* to git command (additional fork). Not good, especially
that you can get information if a ref is a tag (indirect reference)
or not one can get from within git_get_references; which in turn
uses "git show-refs --dereference" and used to use either
"git peek-remote ." or ".git/info/refs" file. If there is <name>^{},
then <name> is indirect reference: is a tag.
As we display ref markers only for log-like views, marker can be tag
or can be "lightweight reference" and be only a commit (in theory
we could show ref markers also for tree and blob items, but it is not
important now).
By looking at git_get_reference() what I see is basically the use of
the same field as $type in format_ref_marker(). I can probably use
that, although it means that any future extensions to ref marker
display will need to hack the routine too. (This would mean that the
patch would be more similar to my original patch
http://marc.info/?l=git&m=121769155017642&w=2 ).
If this is not what you're suggesting, then I'm afraid I don't fully
grasp your idea.
--
Giuseppe "Oblomov" Bilotta
From: Giuseppe Bilotta <hidden> Date: 2016-06-15 22:45:12
This patch turns ref markers into links to appropriate views for the ref
name, using the full ref name as hash. Appropriate changes are made in
the CSS to prevent ref markers to be annoyingly blue and underlined,
unless hovered on.
For all git ref types it's assumed that the preferred view is named like
the ref type itself. If the corresponding action is not defined,
shortlog is used.
Signed-off-by: Giuseppe Bilotta <redacted>
---
Following Jakub Narebski's suggestions, an underlined :hover is
added for the ref marker css, and we use $type instead of
$git_type, sparing ourselves a bunch of git calls.
An additional change wrt the previous version is that the full ref
name is used (to link to the correct object when e.g. a head and a
tag by the same name exist).
gitweb/gitweb.css | 9 +++++++++
gitweb/gitweb.perl | 3 ++-
2 files changed, 11 insertions(+), 1 deletions(-)
Possible improvement:
We would probably want to make this link discoverable, by adding
underline on :hover, like for other "hidden links" in gitweb (for
example in commitdiff view).
Can do that.
Additional idea: it would be nice to know if clicking on ref marker
would lead us to 'shortlog' view, or to 'tag' view; so perhaps we should
distinguish somehow indirect refs, for example using bold font-weight.
quoted
quoted
my ($type, $name) = qw();
+ my $git_type = git_get_type($ref);
# e.g. tags/v2.6.11 or heads/next
if ($ref =~ m!^(.*?)s?/(.*)$!) {
$type = $1;
git_get_type calls 'git cat-file -t', so for each ref shown you make
*additional call* to git command (additional fork). Not good, especially
that you can get information if a ref is a tag (indirect reference)
or not one can get from within git_get_references; which in turn
uses "git show-refs --dereference" and used to use either
"git peek-remote ." or ".git/info/refs" file. If there is <name>^{},
then <name> is indirect reference: is a tag.
As we display ref markers only for log-like views, marker can be tag
or can be "lightweight reference" and be only a commit (in theory
we could show ref markers also for tree and blob items, but it is not
important now).
By looking at git_get_reference() what I see is basically the use of
the same field as $type in format_ref_marker(). I can probably use
that, although it means that any future extensions to ref marker
display will need to hack the routine too. (This would mean that the
patch would be more similar to my original patch
http://marc.info/?l=git&m=121769155017642&w=2 ).
If this is not what you're suggesting, then I'm afraid I don't fully
grasp your idea.
No, that is not what I was suggesting.
What format_ref_marker() uses is not exactly 'type' of reference, but
more 'kind of' reference. It is based on reference namespace, not
on type of object the reference is at (points to). So code based
on this info (like your v3 patch) would fail on lightweight tag, i.e.
if there is ref in 'refs/tags' namespace which points directly to commit,
and not to tag object.
But 'git show-ref --dereference' _has_ information about whether
given reference points directly or indirectly to given object
($refs->{$id}), but currently we neither save it, nor use it.
For example we can have:
781c1834f5419bdf81bb7f3750170ccd6b809174 refs/heads/maint
...
124c62e8781a8f03ee0256bee78f7b392e3920af refs/stash
...
89e6fcde639d65823e8113c307067441701ac74f refs/tags/Attic/gitweb/parse_rev_list
b69a41a384d19fe253b9f4f34c9019ad96ca571d refs/tags/Attic/gitweb/patchset_body
781c1834f5419bdf81bb7f3750170ccd6b809174 refs/tags/TEMP
...
07cca3b30ee2b5d060e44e5b18d7c22929c63d1a refs/tags/v1.5.6.5
781c1834f5419bdf81bb7f3750170ccd6b809174 refs/tags/v1.5.6.5^{}
Now in this example we have three refs pointing to commit object
781c1834: refs/heads/maint, refs/tags/TEMP and refs/tags/v1.5.6.5.
From those only refs/tags/v1.5.6.5 is (via) tag, even though TEMP
is in tags namespace. Currently git_get_references() strips '^{}'
indirect reference marker from the output (from refname), and doesn't
make use of it. One solution would be to not stip it in
git_get_references(), but leave it, and strip it and make use of
it (if ref ends with '^{}' it must be tag object) in format_ref_marker().
But that is just a proposal...
--
Jakub Narebski
Poland
Since some of this thread seems to be about performance, you might just
make this a link to action => 'object' (and save the git_get_type call)
and let gitweb Do The Right Thing when the link is followed.
[Disclaimer: Haven't read the whole thread, and haven't checked if
action=object is actually doing the right thing here.]
-- Lea
Since some of this thread seems to be about performance, you might just
make this a link to action => 'object' (and save the git_get_type call)
and let gitweb Do The Right Thing when the link is followed.
[Disclaimer: Haven't read the whole thread, and haven't checked if
action=object is actually doing the right thing here.]
The object would do the right thing if we wanted 'commit' to be the
default action for commits, but we actually want shortlogs in that
case.
However, this remark of yours makes me think of a different way to
approach the problem: create an 'objectview' action that acts just
like object, but actually maps those objects to an appropriate default
view. I'd like to hear Petr's and Jakub's opinion, and if they think
it's a better approach than the latest version of my patch
http://marc.info/?l=git&m=121941177812828&w=2 (which has not received
any comments yet), I'll implemented it that way.
--
Giuseppe "Oblomov" Bilotta
Since some of this thread seems to be about performance, you might just
make this a link to action => 'object' (and save the git_get_type call)
and let gitweb Do The Right Thing when the link is followed.
[Disclaimer: Haven't read the whole thread, and haven't checked if
action=object is actually doing the right thing here.]
First, only the first patch (and perhaps second) called git_get_type;
v4 and v5 do not. Second, link to 'object' action would not do the
right thing; we want either 'shortlog' or 'tag' view, not 'commit'
or 'tag' view.
What this patch does is making ref markers in the log-like views, and
in the commit subject line headers in other view be "hidden links" to
either 'shortlog' (in the case of ref being head/branch, or lightweight
tag), or to 'tag' view in the case of annotated tag. We rely on the
fact that we know what type of object refs points to (currently it is
only 'commit', which might change, but the fact that we know type of
object for which we show marker would not change), and the fact that
tags point to given object only indirectly, and only tags can point
indirectly (^{} suffix in "git show-ref --dereference", and
"git ls-remote .", and $GIT_DIR/info/refs).
So you could have spared yourself this comment if you have read commit
and the rest of thread more carefully...
--
Jakub Narebski
Poland
Since some of this thread seems to be about performance, you might just
make this a link to action => 'object' (and save the git_get_type call)
and let gitweb Do The Right Thing when the link is followed.
[Disclaimer: Haven't read the whole thread, and haven't checked if
action=object is actually doing the right thing here.]
First, only the first patch (and perhaps second) called git_get_type;
v4 and v5 do not. Second, link to 'object' action would not do the
right thing; we want either 'shortlog' or 'tag' view, not 'commit'
or 'tag' view.
What this patch does is making ref markers in the log-like views, and
in the commit subject line headers in other view be "hidden links" to
either 'shortlog' (in the case of ref being head/branch, or lightweight
tag), or to 'tag' view in the case of annotated tag. We rely on the
fact that we know what type of object refs points to (currently it is
only 'commit', which might change, but the fact that we know type of
object for which we show marker would not change), and the fact that
tags point to given object only indirectly, and only tags can point
indirectly (^{} suffix in "git show-ref --dereference", and
"git ls-remote .", and $GIT_DIR/info/refs).
However, Lea's idea has its own merit. I hacked up a patch series that
implements a git_marker_view() function (or fucntion as the shortlog
says 8-P) and *that* one is used for ref markers, you can see it (3
patches) here: http://git.oblomov.eu/git/shortlog/heads/gitweb/shortlog..heads/gitweb/refmark
[it's on top of other stuff but it's actually independent from the
other stuff].
The advantage of this approach is that you actually it's more flexible
in case future expansions lead to other differences in object vs
marker view (currently the only difference is commit => shortlog):
such differences just need to be added to the appropriate %views hash.
The disadvantage is that we don't care about the difference between
lightweight and annotated tags, so there is no more visual difference
about it, something which patch v5 does. This could of course be
addressed separately as needed.
So, should I resend v5 with the small change about refs canonical
form, or is somebody else doing it? Or is the new idea as implemented
in the above mentioned changeset preferrable? Should I send that
patches to the mailing list?
Let me know, I've got plenty more stuff ready for gitweb and I'm eager
to see them accepted upstream 8-D
--
Giuseppe "Oblomov" Bilotta
Since some of this thread seems to be about performance, you might just
make this a link to action => 'object' (and save the git_get_type call)
and let gitweb Do The Right Thing when the link is followed.
[Disclaimer: Haven't read the whole thread, and haven't checked if
action=object is actually doing the right thing here.]
First, only the first patch (and perhaps second) called git_get_type;
v4 and v5 do not. Second, link to 'object' action would not do the
right thing; we want either 'shortlog' or 'tag' view, not 'commit'
or 'tag' view.
What this patch does is making ref markers in the log-like views, and
in the commit subject line headers in other view be "hidden links" to
either 'shortlog' (in the case of ref being head/branch, or lightweight
tag), or to 'tag' view in the case of annotated tag. We rely on the
fact that we know what type of object refs points to (currently it is
only 'commit', which might change, but the fact that we know type of
object for which we show marker would not change), and the fact that
tags point to given object only indirectly, and only tags can point
indirectly (^{} suffix in "git show-ref --dereference", and
"git ls-remote .", and $GIT_DIR/info/refs).
However, Lea's idea has its own merit. I hacked up a patch series that
implements a git_marker_view() function (or fucntion as the shortlog
says 8-P) and *that* one is used for ref markers, you can see it (3
patches) here: http://git.oblomov.eu/git/shortlog/heads/gitweb/shortlog..heads/gitweb/refmark
[it's on top of other stuff but it's actually independent from the
other stuff].
The advantage of this approach is that you actually it's more flexible
in case future expansions lead to other differences in object vs
marker view (currently the only difference is commit => shortlog):
such differences just need to be added to the appropriate %views hash.
The disadvantage is that we don't care about the difference between
lightweight and annotated tags, so there is no more visual difference
about it, something which patch v5 does. This could of course be
addressed separately as needed.
NAK. For me using 'objectview' action, i.e. deciding on action based
on the type of object, it is a bad idea. Let me explain in more detail.
First, I pretty much think that user would want to know if clicking
on ref marker would lead him/her to 'tag' view, or to 'shortlog' view.
So having visual difference is something that we want to have. And
if we do that when generating link, why not go one extra step and
in addition to visual difference also use different action in link?
Second, for me the whole idea of deciding on action based on the type
of object, either via 'object' view/action or via actionless gitweb URL
is either necessary evil or a tradeoff. We use it because of the
following issues:
* Calculating proper action during link generation via git_get_type()
is costly; it is additional fork (this can be avoided by using
'reuse connection' get type from Lea gitweb caching work), extra
CPU load, and extra I/O hit. If it *cannot be avoided* (which is
not the case of ref markers links) it is better to defer this cost
till user actually follows the link. This feature is used for
sha-1 committags (turning something that looks like sha-1 into
gitweb hyperlink) and for links to symbolic links targets in 'tree'
view (where link target can not exist - dangling symlink, or can
lead to file or directory).
(That is necessary evil part).
* Guessing action based on type of objects allows gitweb to be more
robust, support URL editing/mangling more easily, and aid the
creation of shortcuts to git repositories (e.g. in bugtracker)
more easily (see commit 7f9778b19b07601ae81). Currently it is
done in dispatch phase, and does not do redirection.
(That is tradeoff: more robust and extra feature for extra get type).
BTW. one thing that can be done is consolidation of "guessing action"
code: it is done by simply calculating what is to put in $action during
dispatch, or is done in 'object' view to calculate redirect URL with
proper action. I have tried to bring them together, but patches were
I think lost in the noise.
So, should I resend v5 with the small change about refs canonical
form, or is somebody else doing it? Or is the new idea as implemented
in the above mentioned changeset preferrable? Should I send that
patches to the mailing list?
IMHO v5 with small change making refs canonical (hash=>"refs/$ref")
is preferred way to do this. You can send v6 patch or I can send
it (I planned doing this today).
Let me know, I've got plenty more stuff ready for gitweb and I'm eager
to see them accepted upstream 8-D
First, the great problem with gitweb patches as of today is if Lea
Google Summer of Code 2008 work on gitweb caching would be accepted
(merged in) into git repository; I pretty much think that any gitweb
improvements would be "incompatibile" (read: causing conflicts) with
'gitweb caching' patch covering such large parts of code... but
I might be mistaken about that.
Second, I have planned on sending "gitweb TODO and wishlist", or
"what's cooking in gitweb", with links to threads on git mailing
list, and sometimes explanation why some feature should wait on
infrastructure improvements, such as consolidating log-like views
code.
Thank you for contributing to gitweb...
--
Jakub Narebski
Poland
From: Giuseppe Bilotta <hidden> Date: 2016-06-15 22:45:13
On Tue, Aug 26, 2008 at 10:15 AM, Jakub Narebski [off-list ref] wrote:
BTW. one thing that can be done is consolidation of "guessing action"
code: it is done by simply calculating what is to put in $action during
dispatch, or is done in 'object' view to calculate redirect URL with
proper action. I have tried to bring them together, but patches were
I think lost in the noise.
Ah, but the problem is that guessing the action based on the object
sometimes depends on the context: for some cases the 'commit' action
is the correct one for commits, in other cases shortlog is preferred.
This is why I introduced the %views hash in some of my patches.
IMHO v5 with small change making refs canonical (hash=>"refs/$ref")
is preferred way to do this. You can send v6 patch or I can send
it (I planned doing this today).
I can do it, no problem.
First, the great problem with gitweb patches as of today is if Lea
Google Summer of Code 2008 work on gitweb caching would be accepted
(merged in) into git repository; I pretty much think that any gitweb
improvements would be "incompatibile" (read: causing conflicts) with
'gitweb caching' patch covering such large parts of code... but
I might be mistaken about that.
That's ok, I can wait for Lea's code to get into the repo, so I can
work on the conflicts myself. I'm not sure what parts of the code she
touches though, so for some things it might be easy (the pathinfo
stuff, for example). Other features such as my allheads thing would
probably need to be reworked for caching.
From: Jakub Narebski <hidden> Date: 2016-06-15 22:45:13
On Tue, 26 Aug 2008 12:58, Giuseppe Bilotta wrote:
On Tue, Aug 26, 2008 at 10:15 AM, Jakub Narebski [off-list ref] wrote:
quoted
BTW. one thing that can be done is consolidation of "guessing action"
code: it is done by simply calculating what is to put in $action during
dispatch, or is done in 'object' view to calculate redirect URL with
proper action. I have tried to bring them together, but patches were
I think lost in the noise.
Ah, but the problem is that guessing the action based on the object
sometimes depends on the context: for some cases the 'commit' action
is the correct one for commits, in other cases shortlog is preferred.
This is why I introduced the %views hash in some of my patches.
By the way, this is argument *for* selecting action when generating
link, if it is possible without incurring unnecessary (if you don't
follow the link) performance penalty. For example in the case of
ref markers links, you can link to 'log' in log view, 'shortlog'
in 'shortlog' view, perhaps even 'history' in 'history' and 'tree'/'blob'
views; or just 'shortlog' in all other views (for "commit subject"
line heading).
Just my 2 eurocents.
--
Jakub Narebski
Poland
From: Giuseppe Bilotta <hidden> Date: 2016-06-15 22:45:13
On Tue, Aug 26, 2008 at 1:49 PM, Jakub Narebski [off-list ref] wrote:
By the way, this is argument *for* selecting action when generating
link, if it is possible without incurring unnecessary (if you don't
follow the link) performance penalty. For example in the case of
ref markers links, you can link to 'log' in log view, 'shortlog'
in 'shortlog' view, perhaps even 'history' in 'history' and 'tree'/'blob'
views; or just 'shortlog' in all other views (for "commit subject"
line heading).
An interesting idea. I'll start experimenting with this
context-sensitive action selection for ref-markers. (In the mean time,
I've sent v6 with the simpler 'shortlog or tags' view. I guess an
eventual context-sensitive action can be applied as a subsequent
patch).
--
Giuseppe "Oblomov" Bilotta
From: Giuseppe Bilotta <hidden> Date: 2016-06-15 22:45:14
On Tue, Aug 26, 2008 at 1:49 PM, Jakub Narebski [off-list ref] wrote:
On Tue, 26 Aug 2008 12:58, Giuseppe Bilotta wrote:
quoted
Ah, but the problem is that guessing the action based on the object
sometimes depends on the context: for some cases the 'commit' action
is the correct one for commits, in other cases shortlog is preferred.
This is why I introduced the %views hash in some of my patches.
By the way, this is argument *for* selecting action when generating
link, if it is possible without incurring unnecessary (if you don't
follow the link) performance penalty. For example in the case of
ref markers links, you can link to 'log' in log view, 'shortlog'
in 'shortlog' view, perhaps even 'history' in 'history' and 'tree'/'blob'
views; or just 'shortlog' in all other views (for "commit subject"
line heading).
So, I managed to get this feature into the latest version of the
refmark patch (v7). Hope it's really the final form 8-)
--
Giuseppe "Oblomov" Bilotta
By the way, this is argument *for* selecting action when generating
link, if it is possible without incurring unnecessary (if you don't
follow the link) performance penalty.
I agree that it's much cleaner to select the action when generating the
page, rather than having an 'objectview' action or so.
Worrying about performance seems like premature optimization though --
my guesstimate is that the performance penalty for looking up the object
type is not practically noticeable (read: relevant), and with my patch
applied (even without caching activated) it should move below the
measurable range. So don't complicate the code to gain another 0.01%
performance. ;-)
-- Lea
From: Giuseppe Bilotta <hidden> Date: 2016-06-15 22:45:15
On Thu, Aug 28, 2008 at 3:43 AM, Lea Wiemann [off-list ref] wrote:
Jakub Narebski wrote:
quoted
By the way, this is argument *for* selecting action when generating
link, if it is possible without incurring unnecessary (if you don't
follow the link) performance penalty.
I agree that it's much cleaner to select the action when generating the
page, rather than having an 'objectview' action or so.
In that case v7 of my patch is The Way (TM).
Worrying about performance seems like premature optimization though --
my guesstimate is that the performance penalty for looking up the object
type is not practically noticeable (read: relevant), and with my patch
applied (even without caching activated) it should move below the
measurable range. So don't complicate the code to gain another 0.01%
performance. ;-)
Well, considering that after Jakub's suggestion we just get the target
type from the presence and absence of ^{} in the output of show-refs,
we're not making the code overly complex :)
BTW, any ETA on your caching changes landing in git? I'm really
curious to see how many of my changes are not compatible with it 8-D
--
Giuseppe "Oblomov" Bilotta
From: Jakub Narebski <hidden> Date: 2016-06-15 22:45:15
Lea Wiemann wrote:
Jakub Narebski wrote:
quoted
By the way, this is argument *for* selecting action when generating
link, if it is possible without incurring unnecessary (if you don't
follow the link) performance penalty.
I agree that it's much cleaner to select the action when generating the
page, rather than having an 'objectview' action or so.
Worrying about performance seems like premature optimization though --
my guesstimate is that the performance penalty for looking up the object
type is not practically noticeable (read: relevant), and with my patch
applied (even without caching activated) it should move below the
measurable range. So don't complicate the code to gain another 0.01%
performance. ;-)
First, without your 'git cat-file --batch-check' reuse-connection trick
it wouldn't be _one_ additional fork; it is one fork per ref marker,
which might be quite a lot in tag-heavy, branch-heavy, and using for
example StGIT (with its refs) environment. Note that not all operating
systems have lightweight fork, and that even with "caching" it is IO hit,
and a bit of CPU hit.
Second, it isn't much more code than git_get_type solution, it is bit
larger change: leave ^{} alone, check if ^{} and strip it, as compared
to git_get_type.
--
Jakub Narebski
Poland