From: Jakub Narebski <hidden> Date: 2016-08-11 20:31:52
You should be able to check new commitdiff view at
http://roke_DOT_dyndns_DOT_info/cgi-bin/gitweb/gitweb.cgi
(URL mangling courtesy vger banned words list, aaaarrghhh)
if I didn't screwed something again with firewall, and when my machine
is up; should be for at least an hour. Check for example:
?p=git.git;a=commitdiff;h=origin
?p=git.git;a=commitdiff;h=161332a521fe10c41979bcd493d95e2ac562b7f
?p=git.git;a=commitdiff;h=e12c095aa69d8aca0326eb11960427d9bf9e2db7
?p=git.git;a=commitdiff;h=82560983997c961d9deafe0074b787c8484c2e1d
and compare to (for example)
http://repo.or.cz/w/git.git?a=commitdiff;h=82560983997c961d9deafe0074b787c8484c2e1d
or (even older gitweb)
http://www.kernel.org/git/?p=git/git.git;a=commitdiff;h=887a612fef942dd3e7dae452e2dc582738b0fb41
BTW. this gitweb has also my previous "Slight improvement" patch applied.
Do you like it? What should be changed (code, output, style)?
-- >8 --
Replace "gitweb diff header" with its full sha1 of blobs with
"git diff" header and extended diff header. Change also
highlighting of diffs.
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.
* 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>)'.
* <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.
* from-file/to-file two-line header lines have slightly darker color
than removed/added lines.
* chunk header has now delicate line above for easier finding chunk
boundary, and top margin of 1px.
Controversial ideas:
* All links in patch header are hidden
* Hashes are shortened to 7 characters
* Filenames are presented quoted
* Marking of chunk beginning
* No hyperlink for renamed from/to header (bug)
Signed-off-by: Jakub Narebski <redacted>
---
gitweb/gitweb.css | 46 ++++++++++++---
gitweb/gitweb.perl | 159 ++++++++++++++++++++++++++++++---------------------
2 files changed, 131 insertions(+), 74 deletions(-)
@@ -1255,9 +1255,12 @@ sub parse_difftree_raw_line {$res{'status'}=$5;$res{'similarity'}=$6;if($res{'status'}eq'R'||$res{'status'}eq'C'){# renamed or copied-($res{'from_file'},$res{'to_file'})=map{unquote($_)}split("\t",$7);+($res{'from_file_raw'},$res{'to_file_raw'})=split("\t",$7);+$res{'from_file'}=unquote($res{'from_file_raw'});+$res{'to_file'}=unquote($res{'to_file_raw'});}else{-$res{'file'}=unquote($7);+$res{'file_raw'}=$7;+$res{'file'}=unquote($res{'file_raw'});}}# 'c512b523472485aef4fff9e57b229d9d243c967f'
@@ -2024,6 +2027,7 @@ sub git_patchset_body {my$in_header=0;my$patch_found=0;my$diffinfo;+my(@from_subst,@to_subst);print"<div class=\"patchset\">\n";
@@ -2033,6 +2037,7 @@ sub git_patchset_body {if($patch_line=~m/^diff /){# "git diff" header# beginning of patch (in patchset)+if($patch_found){# close previous patchprint"</div>\n";# class="patch"
@@ -2042,11 +2047,59 @@ sub git_patchset_body {}print"<div class=\"patch\" id=\"patch".($patch_idx+1)."\">\n";+# read and prepare patch informationif(ref($difftree->[$patch_idx])eq"HASH"){+# pre-parsed (or generated by hand)$diffinfo=$difftree->[$patch_idx];}else{$diffinfo=parse_difftree_raw_line($difftree->[$patch_idx]);}+if($diffinfo->{'status'}ne"A"){# not new (added) file+my$quot='';+my$from_text;+my$file_raw=$diffinfo->{'from_file_raw'}||$diffinfo->{'file_raw'};+if($file_raw=~s/^"(.*)"$/\1/){+$from_text=qq("a/$file_raw");+$quot='"';+}else{+$from_text=qq(a/$file_raw);+}+my$file=$diffinfo->{'from_file'}||$diffinfo->{'file'};+my$from_link=+$cgi->a({-href=>href(action=>"blob",hash_base=>$hash_base,+hash=>$diffinfo->{'from_id'},file_name=>$file),+-class=>"list"},esc_html($file_raw));+my$hash_link=+$cgi->a({-href=>href(action=>"blob",hash_base=>$hash_base,+hash=>$diffinfo->{'to_id'},file_name=>$file),+-class=>"list"},substr($diffinfo->{'from_id'},0,7));+@from_subst=+($from_text,"${quot}a/$from_link${quot}",+$diffinfo->{'from_id'}.'\.\.',"$hash_link..");+}+if($diffinfo->{'status'}ne"D"){# not deleted file+my$quot='';+my$to_text;+my$file_raw=$diffinfo->{'to_file_raw'}||$diffinfo->{'file_raw'};+if($file_raw=~s/^"(.*)"$/\1/){+$to_text=qq("b/$file_raw");+$quot='"';+}else{+$to_text=qq(b/$file_raw);+}+my$file=$diffinfo->{'to_file'}||$diffinfo->{'file'};+my$to_link=+$cgi->a({-href=>href(action=>"blob",hash_base=>$hash_base,+hash=>$diffinfo->{'to_id'},file_name=>$file),+-class=>"list"},esc_html($file_raw));+my$hash_link=+$cgi->a({-href=>href(action=>"blob",hash_base=>$hash_base,+hash=>$diffinfo->{'to_id'},file_name=>$file),+-class=>"list"},substr($diffinfo->{'to_id'},0,7));+@to_subst=+($to_text,"${quot}b/$to_link${quot}",+'\.\.'.$diffinfo->{'to_id'},"..$hash_link");+}$patch_idx++;# for now we skip empty patches
@@ -2056,82 +2109,56 @@ sub git_patchset_body {nextLINE;}-if($diffinfo->{'status'}eq"A"){# added-print"<div class=\"diff_info\">".file_type($diffinfo->{'to_mode'}).":".-$cgi->a({-href=>href(action=>"blob",hash_base=>$hash,-hash=>$diffinfo->{'to_id'},file_name=>$diffinfo->{'file'})},-$diffinfo->{'to_id'})." (new)".-"</div>\n";# class="diff_info"--}elsif($diffinfo->{'status'}eq"D"){# deleted-print"<div class=\"diff_info\">".file_type($diffinfo->{'from_mode'}).":".-$cgi->a({-href=>href(action=>"blob",hash_base=>$hash_parent,-hash=>$diffinfo->{'from_id'},file_name=>$diffinfo->{'file'})},-$diffinfo->{'from_id'})." (deleted)".-"</div>\n";# class="diff_info"--}elsif($diffinfo->{'status'}eq"R"||# renamed-$diffinfo->{'status'}eq"C"||# copied-$diffinfo->{'status'}eq"2"){# with two filenames (from git_blobdiff)-print"<div class=\"diff_info\">".-file_type($diffinfo->{'from_mode'}).":".-$cgi->a({-href=>href(action=>"blob",hash_base=>$hash_parent,-hash=>$diffinfo->{'from_id'},file_name=>$diffinfo->{'from_file'})},-$diffinfo->{'from_id'}).-" -> ".-file_type($diffinfo->{'to_mode'}).":".-$cgi->a({-href=>href(action=>"blob",hash_base=>$hash,-hash=>$diffinfo->{'to_id'},file_name=>$diffinfo->{'to_file'})},-$diffinfo->{'to_id'});-print"</div>\n";# class="diff_info"--}else{# modified, mode changed, ...-print"<div class=\"diff_info\">".-file_type($diffinfo->{'from_mode'}).":".-$cgi->a({-href=>href(action=>"blob",hash_base=>$hash_parent,-hash=>$diffinfo->{'from_id'},file_name=>$diffinfo->{'file'})},-$diffinfo->{'from_id'}).-" -> ".-file_type($diffinfo->{'to_mode'}).":".-$cgi->a({-href=>href(action=>"blob",hash_base=>$hash,-hash=>$diffinfo->{'to_id'},file_name=>$diffinfo->{'file'})},-$diffinfo->{'to_id'});-print"</div>\n";# class="diff_info"-}+# print "git diff" header+$patch_line=~s/$from_subst[0]/$from_subst[1]/if@from_subst;+$patch_line=~s/$to_subst[0]/$to_subst[1]/if@to_subst;+print"<div class=\"diff header\">$patch_line</div>\n";-#print "<div class=\"diff extended_header\">\n";+print"<div class=\"diff extended_header\">\n";$in_header=1;nextLINE;}# start of patch in patchset+if($in_header){+if($patch_line!~m/^---/){+# match <path>+if($patch_line=~m|a/|){+$patch_line=~s/$from_subst[0]/$from_subst[1]/if@from_subst;+}+if($patch_line=~m|b/|){+$patch_line=~s/$to_subst[0]/$to_subst[1]/if@to_subst;+}+# match <mode>+if($patch_line=~m/\s(\d{6})$/){+$patch_line.='/'.mode_str($1).' ('.file_type($1).')';+}+# match <hash>+if($patch_line=~m/^index/){+$patch_line=~s/0{40}/'0' x 7/e;+$patch_line=~s/$from_subst[2]/$from_subst[3]/if@from_subst;+$patch_line=~s/$to_subst[2]/$to_subst[3]/if@to_subst;+}+print$patch_line."<br/>\n";-if($in_header&&$patch_line=~m/^---/){-#print "</div>\n"; # class="diff extended_header"-$in_header=0;+}else{+#$patch_line =~ m/^---/;+print"</div>\n";# class="diff extended_header"+$in_header=0;++$patch_line=~s/$from_subst[0]/$from_subst[1]/if@from_subst;+print"<div class=\"diff from_file\">$patch_line</div>\n";-my$file=$diffinfo->{'from_file'};-$file||=$diffinfo->{'file'};-$file=$cgi->a({-href=>href(action=>"blob",hash_base=>$hash_parent,-hash=>$diffinfo->{'from_id'},file_name=>$file),--class=>"list"},esc_html($file));-$patch_line=~s|a/.*$|a/$file|g;-print"<div class=\"diff from_file\">$patch_line</div>\n";+$patch_line=<$fd>;+chomp$patch_line;-$patch_line=<$fd>;-chomp$patch_line;+#$patch_line =~ m/^+++/;+$patch_line=~s/$to_subst[0]/$to_subst[1]/if@to_subst;+print"<div class=\"diff to_file\">$patch_line</div>\n";-#$patch_line =~ m/^+++/;-$file=$diffinfo->{'to_file'};-$file||=$diffinfo->{'file'};-$file=$cgi->a({-href=>href(action=>"blob",hash_base=>$hash,-hash=>$diffinfo->{'to_id'},file_name=>$file),--class=>"list"},esc_html($file));-$patch_line=~s|b/.*|b/$file|g;-print"<div class=\"diff to_file\">$patch_line</div>\n";+}nextLINE;}-nextLINEif$in_header;printformat_diff_line($patch_line);}
--
1.4.3.3
-------------------------------------------------------
--
Jakub Narebski
From: Edgar Toernig <hidden> Date: 2016-08-11 19:18:18
Jakub Narebski wrote:
I'm just saying that with HTML diffs, presented via gitweb in graphical
web browser, you have more possibilities, more formatting to use.
It would be nice though, when the gitweb output would be readable
on non css-capable browsers (i.e. w3m) too. At the moment, gitweb
is mostly usable - the only problematic case is code and diffs.
These are presented via div-tags so in a non-css browser, all spaces
are collapsed thereby removing all indentation. Couldn't code
fragments be presented via (styled) pre-tags for backward compatibility?
Pretty please :)
Btw, while the css version looks nice, Opera seems to have extreme
performance problems with gitweb's project page when there are a lot
of repositories. I.e. trying to view http://gitweb.freedesktop.org/
brings my system to its knees. Turning off style sheets cures it
but then diffs are unusable ...
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 like it and I like the "hidden" links.
* 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>)'.
I like that too, but would leave "100644/-rw-r--r-- (file)" out.
There is a bug in the code: the first index link is the same
as the second, while the first should upload the sha-7 it points to.
For example:
index 743f02b..c821e22
Both point to c821e22. Please fix that.
* <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.
I didn't see where you've quoted file names, but I'm a bit hesitant
to quote anything unnecessarily in the visual output.
* from-file/to-file two-line header lines have slightly darker color
than removed/added lines.
Good.
* chunk header has now delicate line above for easier finding chunk
boundary, and top margin of 1px.
Good.
Controversial ideas:
* All links in patch header are hidden
Love those hidden gems.
* Hashes are shortened to 7 characters
That's ok as long as the output is consisent with real git diff.
That is, a user using git can recognize this commitdiff output and a user
using this gitweb commitdiff output can recognize a git diff output.
* Filenames are presented quoted
I wouldn't do that.
* Marking of chunk beginning
I like the fine lines and their color as it is shown in your server.
* No hyperlink for renamed from/to header (bug)
I'm sure you'll fix that.
Overall I like this patch. Why? Because it makes gitweb commitdiff
output as similar as possible to git-diff output. This is always a good
thing, since both beginners and advanced git users can recognize one
or the other depending where they are coming from (gitweb or git).
Luben
@@ -1255,9 +1255,12 @@ sub parse_difftree_raw_line {$res{'status'}=$5;$res{'similarity'}=$6;if($res{'status'}eq'R'||$res{'status'}eq'C'){# renamed or copied-($res{'from_file'},$res{'to_file'})=map{unquote($_)}split("\t",$7);+($res{'from_file_raw'},$res{'to_file_raw'})=split("\t",$7);+$res{'from_file'}=unquote($res{'from_file_raw'});+$res{'to_file'}=unquote($res{'to_file_raw'});}else{-$res{'file'}=unquote($7);+$res{'file_raw'}=$7;+$res{'file'}=unquote($res{'file_raw'});}}# 'c512b523472485aef4fff9e57b229d9d243c967f'
@@ -2024,6 +2027,7 @@ sub git_patchset_body {my$in_header=0;my$patch_found=0;my$diffinfo;+my(@from_subst,@to_subst);print"<div class=\"patchset\">\n";
@@ -2033,6 +2037,7 @@ sub git_patchset_body {if($patch_line=~m/^diff /){# "git diff" header# beginning of patch (in patchset)+if($patch_found){# close previous patchprint"</div>\n";# class="patch"
@@ -2042,11 +2047,59 @@ sub git_patchset_body {}print"<div class=\"patch\" id=\"patch".($patch_idx+1)."\">\n";+# read and prepare patch informationif(ref($difftree->[$patch_idx])eq"HASH"){+# pre-parsed (or generated by hand)$diffinfo=$difftree->[$patch_idx];}else{$diffinfo=parse_difftree_raw_line($difftree->[$patch_idx]);}+if($diffinfo->{'status'}ne"A"){# not new (added) file+my$quot='';+my$from_text;+my$file_raw=$diffinfo->{'from_file_raw'}||$diffinfo->{'file_raw'};+if($file_raw=~s/^"(.*)"$/\1/){+$from_text=qq("a/$file_raw");+$quot='"';+}else{+$from_text=qq(a/$file_raw);+}+my$file=$diffinfo->{'from_file'}||$diffinfo->{'file'};+my$from_link=+$cgi->a({-href=>href(action=>"blob",hash_base=>$hash_base,+hash=>$diffinfo->{'from_id'},file_name=>$file),+-class=>"list"},esc_html($file_raw));+my$hash_link=+$cgi->a({-href=>href(action=>"blob",hash_base=>$hash_base,+hash=>$diffinfo->{'to_id'},file_name=>$file),+-class=>"list"},substr($diffinfo->{'from_id'},0,7));+@from_subst=+($from_text,"${quot}a/$from_link${quot}",+$diffinfo->{'from_id'}.'\.\.',"$hash_link..");+}+if($diffinfo->{'status'}ne"D"){# not deleted file+my$quot='';+my$to_text;+my$file_raw=$diffinfo->{'to_file_raw'}||$diffinfo->{'file_raw'};+if($file_raw=~s/^"(.*)"$/\1/){+$to_text=qq("b/$file_raw");+$quot='"';+}else{+$to_text=qq(b/$file_raw);+}+my$file=$diffinfo->{'to_file'}||$diffinfo->{'file'};+my$to_link=+$cgi->a({-href=>href(action=>"blob",hash_base=>$hash_base,+hash=>$diffinfo->{'to_id'},file_name=>$file),+-class=>"list"},esc_html($file_raw));+my$hash_link=+$cgi->a({-href=>href(action=>"blob",hash_base=>$hash_base,+hash=>$diffinfo->{'to_id'},file_name=>$file),+-class=>"list"},substr($diffinfo->{'to_id'},0,7));+@to_subst=+($to_text,"${quot}b/$to_link${quot}",+'\.\.'.$diffinfo->{'to_id'},"..$hash_link");+}$patch_idx++;# for now we skip empty patches
From: Jakub Narebski <hidden> Date: 2016-08-11 19:27:10
Luben Tuikov wrote:
quoted hunk
--- Jakub Narebski <jnareb@gmail.com> wrote:
quoted
quoted
Wouldn't this be confusing with the other fine lines?
I personally don't like this chunk separation. Chunk separation
already exists as is and we view it all the time elsewhere.
But not always the program displaying diff can display such line
separating chunks, for example on text terminal it can't.
What I meant is that since I stare at diffs exactly on text terminals,
my eyes have found other ways to discern chunk blocks.
I'm just saying that with HTML diffs, presented via gitweb in graphical
web browser, you have more possibilities, more formatting to use.
Why not make use of it?
quoted
But if you think that the dotted 1px #ffbbff line is too intrusive,
we can remove it (and perhaps increase vertical space a few pixels).
I'd like to have more opinions first.
No, I just think that it should be as close as possible to what
we see now and what we see on text terminals -- no extra vertical
space please. Between the two evils, I'd prefer the thin "dotted" line.
Well, I'll make it nearly invisible in the "take 3". BTW. some people
liked this line, some were indifferent.
quoted
BTW. you can easily override it in your CSS file.
Why should we allow something to go into gitweb and disrupt the current
default behavior only so that people have to change their own css file
to keep current default behaviour. Please don't shove this down our
throats. Please?
That was just to note that if you don't agree with default, you can change
it very easily. It is probably the time where people would disagree (for
example infamous "redundant links" debate) on the gitweb UI; the possibility
to tailor it easily to your own UI concepts and ideas is in my opinion
very important (and very nice).
--
Jakub Narebski
A couple of questions regarding new patchset/diff look for gitweb.
Currently patch starts with "git diff" header
diff --git a/file1 b/file2
then we have a couple of extended header lines
old|new|deleted file|new file mode <mode>
copy|rename from|to <path>
(dis)?similarity index <percent>
index <hash>..<hash> [<mode>]
then we have two-line from-file/to-file header
First revert back to monospace in the commit message.
As I mentioned in that other email, where the person
didn't CC you and thus so I didn't, see commit
4b7ce6e2d6ba088da50de1df38b040ea2c0b8f18.
Can you please hyperlink what you had intended to
quote? I.e. filenames which can lead the user browsing to a
state (pre-rename, post-rename, etc). Thanks.
--- a/file1
+++ b/file2
then patch itself.
1. Which parts to convert to hyperlinks, and which to do not? Which
links have visible and which hidden (underline on mouseover, the same
color as neighbouring text)?
I think the current state of your patch is good, sans the comments
received so far: monospace, that bug in the index links, etc.
Lets get that in, and then you can RFC another improvement on top of this.
I think that a/file1 in "git diff" header should be turned into
visible hyperlink unless file is created, and b/file2 should be link
unless file is deleted.
This is intuitive and makes sense.
Also both <hash>-es in "index" extended header lines should be turned
into links, as it is the only way to have hyperlink to all previou
Fix the bug though.
versions of the file in the case of "combined diff" format (to be
added later). The question is if those hyperlinks should be visible;
I personally love little hidden gems, but some people like everything
to be overly obvious to them. Hidden gems are part of the learning,
but I'm sure I'm not going to convince everyone.
Hidden.
I don't have compelling reason against. Should we use title attribute
to give filename perhaps, or is it unnecessary?
Currently file1 and file2 in "--- a/file1" and "+++ b/file2" are
turned into hidden links. Should we leave it, or should we remove
this link as we have similar link just above? If we decide to have
Leave it -- it's cool.
this link, should we also hyperlink <path> in "copy|rename" extended
header line?
If it makes sense.
2. Use quoted or unquoted filename, remove or leave surrounding quotes
in quoted filename? Should we unquote the not hyperlinked filename
Are you familiar with the term "legalism"?
in the case of creation/deletion? What should be span of link:
"a/_file1_", "_a/file1_", _"a/file1"_
"_file_", _"file"_
(where '_' marks beginning and end of link) for quoted filenames?
What should be span of link for unquoted filenames:
a/_file_, _a/file_
Currently gitweb uses a/_file_ in ---/+++ line.
Unquoted! Now that we've solved this "problem", lets move on to more
interesting things. ;-)
3. How (and if) to explain numerical mode: the currently used
Let's not explain it for now. Let's have your patch go in sans the
comments already posted. We can always debate on that later.
Thanks,
Luben
<mode>/<symbolic mode> (<file type>) e.g. 100755/-rwxr-xr-x (file).
Or <mode> (<file type>) should be enough? Should we mark the addition
compared to git-diff output? Or should we explain <mode> only on
mouseover, using for example:
<abbr title="100644/-rw-r--r-- (file)">100644</abbr>
or just
<abbr title="executable file">100755</abbr>?
--
Jakub Narebski
Poland
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Jakub Narebski <hidden> Date: 2016-08-11 19:31:21
Few other questions, probably to be adressed in the future patches, and
not added to this one.
0. git-ls-tree and git-diff-tree without -z does quote not only !isprint
characters like LF '\n' and non-space whitespace characters like TAB
'\t' (and of course quoting characters '\' and '"'), but for example
also UTF-8 characters. See for example git-ls-tree output for
gitweb/test directory. This screws somewhat idea how we should treat
filenames which are quoted.
[BTW. How git should deal with being deployed in the environment where
filesystem pathnames coding might not be UTF-8, and console (terminal)
coding might be not UTF-8?]
1. Current version doesn't display empty patches (i.e. pure rename and
mode change combinations) and doesn't provide links to them from
difftree. This is legacy of old /usr/bin/diff using code, which did not
generated extended diff header, which is only output for "empty
patches". Should we change this, or leave as is?
2. Schould we change syntax highlighting of chunk header line, namely
changing slightly syntax coloring of "in which function are we" part of
chunk header?
3. Should we make from-range/to-range in chunk header hyperlink to the
start of given bunch of lines in appropriate file? Or perhaps to the
middle of the bunch of lines? Or to first changed line (omitting
context)?
--
Jakub Narebski
From: Jakub Narebski <hidden> Date: 2016-08-11 19:33:32
Edgar Toernig wrote:
Jakub Narebski wrote:
quoted
I'm just saying that with HTML diffs, presented via gitweb in graphical
web browser, you have more possibilities, more formatting to use.
It would be nice though, when the gitweb output would be readable
on non css-capable browsers (i.e. w3m) too. At the moment, gitweb
is mostly usable - the only problematic case is code and diffs.
These are presented via div-tags so in a non-css browser, all spaces
are collapsed thereby removing all indentation. Couldn't code
fragments be presented via (styled) pre-tags for backward compatibility?
Pretty please :)
Well, we replaced using s/ / /g with .pre class woth white-space: pre.
Perhaps we can go halfway, and add <pre>...</pre> wrapping line.
Btw, while the css version looks nice, Opera seems to have extreme
performance problems with gitweb's project page when there are a lot
of repositories. I.e. trying to view http://gitweb.freedesktop.org/
brings my system to its knees. Turning off style sheets cures it
but then diffs are unusable ...
Strange. It's just a simple table. Could you and would you be able to
debug it further (e.g. by bisecting CSS)?
--
Jakub Narebski
From: Jakub Narebski <hidden> Date: 2016-08-11 19:36:20
Replace "gitweb diff header" with its full sha1 of blobs and replace
it by "git diff" header and extended diff header. Change also somewhat
highlighting of diffs.
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
changed to
diff --git _a/<file before>_ _b/<file after>_
In both cases links are visible and use default link style. If file
is added, a/<file> is not hyperlinked, if file is deleted, b/<file>
is not hyperlinked.
* there is added "extended diff header", with <path> and <hash>
hyperlinked (and <hash> shortened to 7 characters), and <mode>
explained: '<mode>' is extended to '<mode> (<file type>)'.
* <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. And doesn't work: perhaps
unquote is broken.
* from-file/to-file two-line header lines have slightly darker color
than removed/added lines.
* chunk header has now delicate line above for easier finding chunk
boundary, and top margin of 1px.
WORK IN PROGRESS: might not work (and actually doesn't work correctly)
for strange filenames, i.e. filenames contaning either metacharacters
or having TAB, LF, backslash or doublequote in them.
Code should be much more clean, by the way.
Signed-off-by: Jakub Narebski <redacted>
---
gitweb/gitweb.css | 46 +++++++++++---
gitweb/gitweb.perl | 178 +++++++++++++++++++++++++++++++++-------------------
2 files changed, 151 insertions(+), 73 deletions(-)
@@ -1255,9 +1255,12 @@ sub parse_difftree_raw_line {$res{'status'}=$5;$res{'similarity'}=$6;if($res{'status'}eq'R'||$res{'status'}eq'C'){# renamed or copied-($res{'from_file'},$res{'to_file'})=map{unquote($_)}split("\t",$7);+($res{'from_file_raw'},$res{'to_file_raw'})=split("\t",$7);+$res{'from_file'}=unquote($res{'from_file_raw'});+$res{'to_file'}=unquote($res{'to_file_raw'});}else{-$res{'file'}=unquote($7);+$res{'file_raw'}=$7;+$res{'file'}=unquote($res{'file_raw'});}}# 'c512b523472485aef4fff9e57b229d9d243c967f'
@@ -2023,7 +2026,9 @@ sub git_patchset_body {my$patch_idx=0;my$in_header=0;my$patch_found=0;+my$skip_patch=0;my$diffinfo;+my(%from,%to);print"<div class=\"patchset\">\n";
@@ -2033,6 +2038,8 @@ sub git_patchset_body {if($patch_line=~m/^diff /){# "git diff" header# beginning of patch (in patchset)+$skip_patch=0;+if($patch_found){# close previous patchprint"</div>\n";# class="patch"
@@ -2042,96 +2049,137 @@ sub git_patchset_body {}print"<div class=\"patch\" id=\"patch".($patch_idx+1)."\">\n";+# read and prepare patch informationif(ref($difftree->[$patch_idx])eq"HASH"){+# pre-parsed (or generated by hand)$diffinfo=$difftree->[$patch_idx];}else{$diffinfo=parse_difftree_raw_line($difftree->[$patch_idx]);}+if($diffinfo->{'status'}ne"A"){# not new (added) file+$from{'name'}=$diffinfo->{'from_file_raw'}||$diffinfo->{'file_raw'};+# because of "a/file" not a/"file"+$from{'quoted'}=($from{'name'}=~s/^"(.*)"$/$1/);++my$file=$diffinfo->{'from_file'}||$diffinfo->{'file'};+$from{'href'}=href(action=>"blob",hash_base=>$hash_parent,+hash=>$diffinfo->{'from_id'},file_name=>$file);+}+if($diffinfo->{'status'}ne"D"){# not deleted file+$to{'name'}=$diffinfo->{'to_file_raw'}||$diffinfo->{'file_raw'};+# because of "b/file" not b/"file"+$to{'quoted'}=($to{'name'}=~s/^"(.*)"$/$1/);++my$file=$diffinfo->{'to_file'}||$diffinfo->{'file'};+$to{'href'}=href(action=>"blob",hash_base=>$hash,+hash=>$diffinfo->{'to_id'},file_name=>$file);+}$patch_idx++;# for now we skip empty patchesif($diffinfo->{'from_id'}eq$diffinfo->{'to_id'}){# no change, empty patch$in_header=1;+$skip_patch=0;nextLINE;}-if($diffinfo->{'status'}eq"A"){# added-print"<div class=\"diff_info\">".file_type($diffinfo->{'to_mode'}).":".-$cgi->a({-href=>href(action=>"blob",hash_base=>$hash,-hash=>$diffinfo->{'to_id'},file_name=>$diffinfo->{'file'})},-$diffinfo->{'to_id'})." (new)".-"</div>\n";# class="diff_info"--}elsif($diffinfo->{'status'}eq"D"){# deleted-print"<div class=\"diff_info\">".file_type($diffinfo->{'from_mode'}).":".-$cgi->a({-href=>href(action=>"blob",hash_base=>$hash_parent,-hash=>$diffinfo->{'from_id'},file_name=>$diffinfo->{'file'})},-$diffinfo->{'from_id'})." (deleted)".-"</div>\n";# class="diff_info"--}elsif($diffinfo->{'status'}eq"R"||# renamed-$diffinfo->{'status'}eq"C"||# copied-$diffinfo->{'status'}eq"2"){# with two filenames (from git_blobdiff)-print"<div class=\"diff_info\">".-file_type($diffinfo->{'from_mode'}).":".-$cgi->a({-href=>href(action=>"blob",hash_base=>$hash_parent,-hash=>$diffinfo->{'from_id'},file_name=>$diffinfo->{'from_file'})},-$diffinfo->{'from_id'}).-" -> ".-file_type($diffinfo->{'to_mode'}).":".-$cgi->a({-href=>href(action=>"blob",hash_base=>$hash,-hash=>$diffinfo->{'to_id'},file_name=>$diffinfo->{'to_file'})},-$diffinfo->{'to_id'});-print"</div>\n";# class="diff_info"--}else{# modified, mode changed, ...-print"<div class=\"diff_info\">".-file_type($diffinfo->{'from_mode'}).":".-$cgi->a({-href=>href(action=>"blob",hash_base=>$hash_parent,-hash=>$diffinfo->{'from_id'},file_name=>$diffinfo->{'file'})},-$diffinfo->{'from_id'}).-" -> ".-file_type($diffinfo->{'to_mode'}).":".-$cgi->a({-href=>href(action=>"blob",hash_base=>$hash,-hash=>$diffinfo->{'to_id'},file_name=>$diffinfo->{'file'})},-$diffinfo->{'to_id'});-print"</div>\n";# class="diff_info"+# print "git diff" header+if($from{'name'}){+my$from_link=$cgi->a({-href=>$from{'href'},-class=>"path"},+'a/'.esc_html($from{'name'}));+my($q,$qq)=$from{'quoted'}?('"','"'):('','');+$patch_line=~s|${q}a/\Q$from{'name'}\E${q}|${qq}$from_link${qq}|;+}else{+# at least one of %from and %to must be set+$patch_line=~s|(["]?a/\Q$to{'name'}\E["]?)|esc_html($1)|e;+}+if($to{'name'}){+my$to_link=$cgi->a({-href=>$to{'href'},-class=>"path"},+'b/'.esc_html($to{'name'}));+my($q,$qq)=$to{'quoted'}?('"','"'):('','');+$patch_line=~s|${q}b/\Q$to{'name'}\E${q}$|${qq}$to_link${qq}|;+}else{+# at least one of %from and %to must be set+$patch_line=~s|(["]?b/\Q$from{'name'}\E["]?)$|esc_html($1)|e;}-#print "<div class=\"diff extended_header\">\n";+print"<div class=\"diff header\">$patch_line</div>\n";+print"<div class=\"diff extended_header\">\n";$in_header=1;nextLINE;+}else{+nextLINEif$skip_patch;}# start of patch in patchset+if($in_header){+if($patch_line!~m/^---/){+# match <path>+if($patch_line=~m!^(copy|rename) from !&&$from{'name'}){+my$qq=$from{'quoted'}?'"':'';+my$from_link=$cgi->a({-href=>$from{'href'},-class=>"list"},+esc_html($from{'name'}));+$patch_line=~s!from .*$!from $qq$from_link$qq!;+}+if($patch_line=~m!^(copy|rename) to !&&$to{'name'}){+my$qq=$to{'quoted'}?'"':'';+my$to_link=$cgi->a({-href=>$to{'href'},-class=>"list"},+esc_html($to{'name'}));+$patch_line=~s!to .*$!to $qq$to_link$qq!;+}+# match <mode>+if($patch_line=~m/\s(\d{6})$/){+$patch_line.='<span class="info"> ('.file_type($1).')</span>';+}+# match <hash>+if($patch_line=~m/^index/){+my($from_link,$to_link);+if($from{'href'}){+$from_link=$cgi->a({-href=>$from{'href'},-class=>"list"},+substr($diffinfo->{'from_id'},0,7));+}else{+$from_link='0'x7;+}+if($to{'href'}){+$to_link=$cgi->a({-href=>$to{'href'},-class=>"list"},+substr($diffinfo->{'to_id'},0,7));+}else{+$to_link='0'x7;+}+my($from_id,$to_id)=($diffinfo->{'from_id'},$diffinfo->{'to_id'});+$patch_line=~s!$from_id\.\.$to_id!$from_link..$to_link!;+}+print$patch_line."<br/>\n";-if($in_header&&$patch_line=~m/^---/){-#print "</div>\n"; # class="diff extended_header"-$in_header=0;+}else{+#$patch_line =~ m/^---/;+print"</div>\n";# class="diff extended_header"+$in_header=0;++if($from{'name'}){+my$qq=$from{'quoted'}?'"':'';+my$from_link=$cgi->a({-href=>$from{'href'},-class=>"list"},+esc_html($from{'name'}));+$patch_line=~s!["]?a/.*$!${qq}a/$from_link${qq}!;+}+print"<div class=\"diff from_file\">$patch_line</div>\n";-my$file=$diffinfo->{'from_file'};-$file||=$diffinfo->{'file'};-$file=$cgi->a({-href=>href(action=>"blob",hash_base=>$hash_parent,-hash=>$diffinfo->{'from_id'},file_name=>$file),--class=>"list"},esc_html($file));-$patch_line=~s|a/.*$|a/$file|g;-print"<div class=\"diff from_file\">$patch_line</div>\n";+$patch_line=<$fd>;+chomp$patch_line;-$patch_line=<$fd>;-chomp$patch_line;+#$patch_line =~ m/^+++/;+if($to{'name'}){+my$qq=$to{'quoted'}?'"':'';+my$from_link=$cgi->a({-href=>$to{'href'},-class=>"list"},+esc_html($to{'name'}));+$patch_line=~s!["]?b/.*$!${qq}b/$from_link${qq}!;+}+print"<div class=\"diff to_file\">$patch_line</div>\n";-#$patch_line =~ m/^+++/;-$file=$diffinfo->{'to_file'};-$file||=$diffinfo->{'file'};-$file=$cgi->a({-href=>href(action=>"blob",hash_base=>$hash,-hash=>$diffinfo->{'to_id'},file_name=>$file),--class=>"list"},esc_html($file));-$patch_line=~s|b/.*|b/$file|g;-print"<div class=\"diff to_file\">$patch_line</div>\n";+}nextLINE;}-nextLINEif$in_header;printformat_diff_line($patch_line);}
From: Jakub Narebski <hidden> Date: 2016-08-11 19:39:41
Luben Tuikov wrote:
quoted hunk
--- Jakub Narebski <jnareb@gmail.com> wrote:
quoted
Replace "gitweb diff header" with its full sha1 of blobs and replace
it by "git diff" header and extended diff header. Change also somewhat
highlighting of diffs.
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
changed to
diff --git _a/<file before>_ _b/<file after>_
In both cases links are visible and use default link style. If file
is added, a/<file> is not hyperlinked, if file is deleted, b/<file>
is not hyperlinked.
"Everything clickable underlined" isn't the best way to represent things.
Anyway, my 2 cents is that I don't like the overly explicit underlineing.
I liked it the way it was in take 1.
Thet is the only "obviously link" per patch. And I think there should be
at least one non-hidden link.
BTW. comments like this are the reason I've sent the patch as-is, without
resolving the strange filenames problem (it would be nice if somebody was
to send code; well Junio send patch to address core git filename quoting
issue).
quoted
* there is added "extended diff header", with <path> and <hash>
hyperlinked (and <hash> shortened to 7 characters), and <mode>
explained: '<mode>' is extended to '<mode> (<file type>)'.
* <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. And doesn't work: perhaps
unquote is broken.
In which case we shouldn't commit this. IOW, let's commit things
which we _know_ to work.
Why not resubmit your original patch with the bugfixes as few comments
as mentioned?
I'll do that, but for now quoting/unquoting filename is broken, both
in gitweb but also to lesser extent in git core (quoting perfectly valid
UTF-8 characters).
I'll try to adress that, but I wanted to send next RFC patch for review.
quoted
* from-file/to-file two-line header lines have slightly darker color
than removed/added lines.
* chunk header has now delicate line above for easier finding chunk
boundary, and top margin of 1px.
Wouldn't this be confusing with the other fine lines?
I personally don't like this chunk separation. Chunk separation
already exists as is and we view it all the time elsewhere.
But not always the program displaying diff can display such line
separating chunks, for example on text terminal it can't.
But if you think that the dotted 1px #ffbbff line is too intrusive,
we can remove it (and perhaps increase vertical space a few pixels).
I'd like to have more opinions first.
BTW. you can easily override it in your CSS file.
If you'd like to separate chunks, why not darken the background
of the section of line the chunk header is printed at? I.e.
anything between the @@ including the @@.
I'd rather have this one in a separate commit (this needs changes
to format_diff_line, not only to git_patchset_body).
--
Jakub Narebski
From: Edgar Toernig <hidden> Date: 2016-08-11 19:45:35
Jakub Narebski wrote:
Edgar Toernig wrote:
quoted
Btw, while the css version looks nice, Opera seems to have extreme
performance problems with gitweb's project page when there are a lot
of repositories. I.e. trying to view http://gitweb.freedesktop.org/
brings my system to its knees. Turning off style sheets cures it
but then diffs are unusable ...
Strange. It's just a simple table. Could you and would you be able to
debug it further (e.g. by bisecting CSS)?
It's the combination of tr.light/dark:hover and background-color.
Changing the foreground instead of the background color is fast.
Maybe it recalculates the complete table when the background of
a table cell changes.
I've reported the problem to Opera ...
I think your version looks a lot nicer but I believe that the commit
message area should be in a fixed-point font. In general it won't be a
problem but if anyone should choose to insert a diagram or something else
that assume a fixed-width display - it won't look good.
Anand
From: Jakub Narebski <hidden> Date: 2016-08-11 20:13:21
Luben Tuikov wrote:
quoted hunk
--- Jakub Narebski <jnareb@gmail.com> wrote:
quoted
A couple of questions regarding new patchset/diff look for gitweb.
Currently patch starts with "git diff" header
diff --git a/file1 b/file2
then we have a couple of extended header lines
old|new|deleted file|new file mode <mode>
copy|rename from|to <path>
(dis)?similarity index <percent>
index <hash>..<hash> [<mode>]
then we have two-line from-file/to-file header
First revert back to monospace in the commit message.
As I mentioned in that other email, where the person
didn't CC you and thus so I didn't, see commit
4b7ce6e2d6ba088da50de1df38b040ea2c0b8f18.
This regression was caused by the not-accepted (I think) patch
"gitweb: Slight visual improvements to commitdiff view",
and is not caused by _this_ patch.
I'll redo abovementioned patch correctly later.
Can you please hyperlink what you had intended to
quote? I.e. filenames which can lead the user browsing to a
state (pre-rename, post-rename, etc). Thanks.
I don't understand this comment. The above is pre-changes git-diff
patch output. The changes are/were mentioned below.
quoted
--- a/file1
+++ b/file2
then patch itself.
1. Which parts to convert to hyperlinks, and which to do not? Which
links have visible and which hidden (underline on mouseover, the same
color as neighbouring text)?
I think the current state of your patch is good, sans the comments
received so far: monospace, that bug in the index links, etc.
Lets get that in, and then you can RFC another improvement on top of this.
O.K. I send the corrected version (as "take 2") in a while.
--
Jakub Narebski
From: Jakub Narebski <hidden> Date: 2016-08-11 20:23:10
A couple of questions regarding new patchset/diff look for gitweb.
Currently patch starts with "git diff" header
diff --git a/file1 b/file2
then we have a couple of extended header lines
old|new|deleted file|new file mode <mode>
copy|rename from|to <path>
(dis)?similarity index <percent>
index <hash>..<hash> [<mode>]
then we have two-line from-file/to-file header
--- a/file1
+++ b/file2
then patch itself.
1. Which parts to convert to hyperlinks, and which to do not? Which
links have visible and which hidden (underline on mouseover, the same
color as neighbouring text)?
I think that a/file1 in "git diff" header should be turned into
visible hyperlink unless file is created, and b/file2 should be link
unless file is deleted.
Also both <hash>-es in "index" extended header lines should be turned
into links, as it is the only way to have hyperlink to all previous
versions of the file in the case of "combined diff" format (to be
added later). The question is if those hyperlinks should be visible;
I don't have compelling reason against. Should we use title attribute
to give filename perhaps, or is it unnecessary?
Currently file1 and file2 in "--- a/file1" and "+++ b/file2" are
turned into hidden links. Should we leave it, or should we remove
this link as we have similar link just above? If we decide to have
this link, should we also hyperlink <path> in "copy|rename" extended
header line?
2. Use quoted or unquoted filename, remove or leave surrounding quotes
in quoted filename? Should we unquote the not hyperlinked filename
in the case of creation/deletion? What should be span of link:
"a/_file1_", "_a/file1_", _"a/file1"_
"_file_", _"file"_
(where '_' marks beginning and end of link) for quoted filenames?
What should be span of link for unquoted filenames:
a/_file_, _a/file_
Currently gitweb uses a/_file_ in ---/+++ line.
3. How (and if) to explain numerical mode: the currently used
<mode>/<symbolic mode> (<file type>) e.g. 100755/-rwxr-xr-x (file).
Or <mode> (<file type>) should be enough? Should we mark the addition
compared to git-diff output? Or should we explain <mode> only on
mouseover, using for example:
<abbr title="100644/-rw-r--r-- (file)">100644</abbr>
or just
<abbr title="executable file">100755</abbr>?
--
Jakub Narebski
Replace "gitweb diff header" with its full sha1 of blobs and replace
it by "git diff" header and extended diff header. Change also somewhat
highlighting of diffs.
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
changed to
diff --git _a/<file before>_ _b/<file after>_
In both cases links are visible and use default link style. If file
is added, a/<file> is not hyperlinked, if file is deleted, b/<file>
is not hyperlinked.
"Everything clickable underlined" isn't the best way to represent things.
Anyway, my 2 cents is that I don't like the overly explicit underlineing.
I liked it the way it was in take 1.
* there is added "extended diff header", with <path> and <hash>
hyperlinked (and <hash> shortened to 7 characters), and <mode>
explained: '<mode>' is extended to '<mode> (<file type>)'.
* <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. And doesn't work: perhaps
unquote is broken.
In which case we shouldn't commit this. IOW, let's commit things
which we _know_ to work.
Why not resubmit your original patch with the bugfixes as few comments
as mentioned?
* from-file/to-file two-line header lines have slightly darker color
than removed/added lines.
* chunk header has now delicate line above for easier finding chunk
boundary, and top margin of 1px.
Wouldn't this be confusing with the other fine lines?
I personally don't like this chunk separation. Chunk separation
already exists as is and we view it all the time elsewhere.
If you'd like to separate chunks, why not darken the background
of the section of line the chunk header is printed at? I.e.
anything between the @@ including the @@.
Luben
I'm just saying that with HTML diffs, presented via gitweb in graphical
web browser, you have more possibilities, more formatting to use.
Why not make use of it?
That sounds fine.
The question is where one draws the line.
quoted
quoted
BTW. you can easily override it in your CSS file.
Why should we allow something to go into gitweb and disrupt the current
default behavior only so that people have to change their own css file
to keep current default behaviour. Please don't shove this down our
throats. Please?
That was just to note that if you don't agree with default, you can change
it very easily. It is probably the time where people would disagree (for
example infamous "redundant links" debate) on the gitweb UI; the possibility
to tailor it easily to your own UI concepts and ideas is in my opinion
very important (and very nice).
I would like to keep the visual default as stable as possible.
Luben
Wouldn't this be confusing with the other fine lines?
I personally don't like this chunk separation. Chunk separation
already exists as is and we view it all the time elsewhere.
But not always the program displaying diff can display such line
separating chunks, for example on text terminal it can't.
What I meant is that since I stare at diffs exactly on text terminals,
my eyes have found other ways to discern chunk blocks.
But if you think that the dotted 1px #ffbbff line is too intrusive,
we can remove it (and perhaps increase vertical space a few pixels).
I'd like to have more opinions first.
No, I just think that it should be as close as possible to what
we see now and what we see on text terminals -- no extra vertical
space please. Between the two evils, I'd prefer the thin "dotted" line.
BTW. you can easily override it in your CSS file.
Why should we allow something to go into gitweb and disrupt the current
default behavior only so that people have to change their own css file
to keep current default behaviour. Please don't shove this down our
throats. Please?
Luben
I think your version looks a lot nicer but I believe that the commit
message area should be in a fixed-point font. In general it won't be a
problem but if anyone should choose to insert a diagram or something else
that assume a fixed-width display - it won't look good.
Thanks for catching this!
Jakub, I believe I submitted a patch to change that to fixed point
and you can see the reasons in the commit message. See commit
4b7ce6e2d6ba088da50de1df38b040ea2c0b8f18.
Please change it to fixed point.
Luben