Junio C Hamano wrote: > Jakub Narebski <jnareb@xxxxxxxxx> writes: > >> Changes: >> * "gitweb diff header" which looked for example like below: >> file:_<sha1 before>_ -> file:_<sha1 after>_ >> where 'file' is file type and '<sha1>' is full sha1 of blob, is link >> and uses default link style is changed to >> diff --git a/<file before> b/<file after> >> where <file> is hidden link (i.e. underline on hover, only) >> to appropriate version of file. If file is added, a/<file> is not >> hyperlinked, if file is deleted, b/<file> is not hyperlinked. > > I do not have time to look at the code right now, but here are > quick comments on the output. Code is a bit horrible I think in trying to be smart and avoid code repetition. > I personally do not mind "hidden" but it might be more obvious > to make them normal links -- the filenames in the header are not > part of long text people need to "read". On the other hand, > it feels a bit wasteful to have these hidden links both on "diff > --git" line *and* ---/+++ lines (these three are very close to > each other). Good idea. We have lost visible links with the change. And easy to implement: just remove style override for .diff.header a.list from CSS (or what would be better but harder to code remove "list" class from <a> element in "git header"). Originally there was visible link with full sha1 of blob as text, and hidden link with filename as text; perhaps dropping the latter ---/+++ line link is better idea (here hyperlink should I think be hidden, if there is any, to match git-diff and GNU diff -u output). And this would simplify code somewhat... The hidden (but here I think we can change to not-hidden links) links in the "index" extended header line are to stay I think: this is the only place where we can have links to all the versions of file in the future combined commitdiff format. >> * there is added "extended diff header", with <path> and <hash> >> hyperlinked (and <hash> shortened to 7 characters), and <mode> >> explained: '<mode>' is extnded to '<mode>/<symbolic mode> (<file type>)'. > > It somehow feels that deviating from what "git diff" gives makes > it very distracting; I would feel better if "/-rw-r--r-- (file)" > were not there. Well, the old version had "(<file type>)" in gitweb diff header. Not all git/gitweb users are familiar with POSIX numeric modes; perhaps changing the style (color for example) of added stuff to mark it as added would be enough? > Also I think arguing over 7 or 8 hexdigits is pointless; if you > are reading this from "git diff", it is probably the easiest to > match what "git diff" gave you. One thing we _might_ want to do > in the future is to change "git diff" to use DEFAULT_ABBREV > hexdigits at the minimum but more if needed to disambiguate; I > think it currently does not do the "more if needed" part. I used 7 hexdigits because git-diff uses 7 by default. >> * <file> hyperlinking should work also when <file> is originally >> quoted. For now we present filename quoted. This needed changes to >> parse_difftree_raw_line subroutine. > > This feels Ok. I'm not sure if we shouldn't unquote filename always, or at least remove double quotes surrounding filename, or [ab]/filename. The decision to leave visible filename quoted was based on the troubles to code it correctly in "git diff" header where one of the filenames might not be hyperlinked: if file was added or deleted. >> * from-file/to-file two-line header lines have slightly darker color >> than removed/added lines. > > This visually feels right. What is left is fine-tuning the color. >> * chunk header has now delicate line above for easier finding chunk >> boundary, and top margin of 1px. > > This visually feels right. What is left is fine-tuning the line and vertical space. -- 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