Re: [PATCH] gitweb: Better symbolic link support in "tree" view

[Jakub Narebski <jnareb@gmail.com>]

Junio C Hamano wrote:

> Jakub Narebski <jnareb@gmail.com> writes:
> 
>> Junio C Hamano wrote:
>>> ...
>>> I think " -> link_target" is fine, but I do not know if it is
>>> useful (while I do not think it is wrong) to make the value that
>>> would have been returned from readlink() into an href, even when
>>> it points at something inside the same revision.
>>
>> I have added this bit (making symbolic link target symlink) because 
>> otherwise there is no way, besides hand-munging the URL, to go to the 
>> link target.
> 
> I can read what you wrote it does.
> 
> For one thing, the user is tracking the symbolic link itself,
> not the contents of the file or directory the link points at.
> For that "tracked symlink", where it points at is the important
> content, not what the file that is pointed at happens to contain
> in the same revision.
> 
> If you have to open an extra object while drawing the list, I do
> not think it is worth doing it.
> 
> In order to show " -> link_target", you have to read the
> contents of the blob.  I think that overhead to read one extra
> blob is probably an acceptable tradeoff for convenience.

Especially that it is done _only_ if there exist symbolic link
entry in a tree.
 
> But if you want to make it a link into the same tree, you would
> need to check if link_target path exists and if it is a blob or
> tree to produce an appropriate tree_view/blob_view link (I
> haven't read your code but that is the natural thing to do).
> 
> That would involve in reading a few more tree objects (depending
> on how deep the target is in the tree), and I do not think it is
> worth doing it while drawing a list.  After you prepared dozens
> of such links, the user would click at most one of them and 
> leaves the page; your cycles to draw those unclicked links were
> wasted.

Actually this requires only one call to git-ls-tree

  $ git ls-tree $hash_base -- $target_path

Internally this mean reading a few more tree objects, but in gitweb
the cost of fork is what (I think) dominates.

> If you wanted to do this, a better way would be to have a new
> view that takes a commit/tree object and a path from the top of
> the repository, and shows either "no such path in that tree" or
> "here is the view for that object, by the way it was a blob."
> page.  Then your list drawing would still need to open each
> symlink blob to show " -> link_target", and need to check if it
> goes outside the repository (I would assume you are handling
> relative links as well),

I handle _only_ relative links. There is no way to treat absolute links 
leading within repository (well, there is, but absoulte links depends 
on position of repository in the filesystem, and that is usually bad 
idea... unless absolute link is not to file within repository). The 
link is "normalized" to path from the top of the tree/top of repository 
tree (dealing with /./, /../, and // in the way).

>                          but you do not need to do expensive 
> ls-tree step one per symlink on the page.  The href attr of the
> A element " -> link_target" would point at that "universal
> object view" with the link_target pathname (that is, the blob
> contents) and the commit/tree object name (h or hb I do not know
> which) and you will spend cycles to run ls-tree only when the
> user actually asks to follow that link.
> 
> In other words, I think trying to be lazy is extremely important
> while drawing a big list.

Well, that is certainly another solution. I'm not sure if distinction 
between checking if link target exists (and getting target type while 
at it) and providing link to perhaps "no such patch in that tree" page 
is worth %feature... well, I guess it is not.

I'll split the patch into two: first to read link target and show it in 
"tree" view _without_ hyperlink, and later perhaps either your or mine 
solution (most probably yours), depending on feedback.

-- 
Jakub Narebski