gitweb: Use git-diff-tree patch output for commitdiff
As the subject indicates (RFC), I'd like some comments on that patch.
This patch is the first patch in series which tries to remove external diff dependency in gitweb, and the need for temporary files.
Converting "blobdiff" and "blobdiff_plain" format would be much easier if git-diff-tree and friends had -L <label>/--label=<label> option, like GNU diff has.
Current patch preserves much of current output; the question is if for example generate if 'plain' format should generate patch which could be appplied by ordinary patch which do not understand git diff extensions (including renaming and copying), as it is done in current version, and if 'html' version should detect renames and copying.
Another question is if to have (as it is currently done) 'html' and 'plain' format generated by one subroutine, even though they don't share that much code, and how to divide this code (currently invoking git-commit-diff is in one part, generating header and commit message is in second part, generating patch/patchset is in third part).
Further patches are planned, including getting rid of git_commitdiff_plain, changing format_diff_line to include incomplete lines in output, and converting git_blobdiff and git_blobdiff_plain to use git-diff-tree.
This patch is based on 'next' (fbf19dd41bb51d5221fac739c5bdb48fd9012412)
P.S. Perhaps the below separator should be made standard and recognized by git tools?
-- >8 --
Get rid of git_diff_print invocation in git_commitdiff and therefore external diff (/usr/bin/diff) invocation, and use only git-diff-tree to generate patch.
git_commitdiff and git_commitdiff_plain are collapsed into one subroutine git_commitdiff, with format (currently 'html' which is default format corresponding to git_commitdiff, and 'plain' corresponding to git_commitdiff_plain) specified in argument.
Separate patch (diff) pretty-printing into git_patchset_body. It is used in git_commitdiff.
Separate patch (diff) line formatting from git_diff_print into format_diff_line function. It is used in git_patchset_body.
While at it, add $hash parameter to git_difftree_body, according to rule that inner functions should use parameter passing, and not global variables.
CHANGES TO OUTPUT:
* "commitdiff" now products patches with renaming and copying detection (git-diff-tree is invoked with -M and -C options). Empty patches (mode changes and pure renames and copying) are not written currently. Former version broke renaming and copying, and didn't notice mode changes, like this version.
* "commitdiff" output is now divided into several div elements of class "log", "patchset" and "patch".
* "commitdiff_plain" now only generates X-Git-Tag: line only if there is tag pointing to the current commit. Former version which wrote first tag following current commit was broken[*1*]; besides we are interested rather in tags _preceding_ the commit, and _heads_ following the commit. X-Git-Url: now is current URL; former version tried[*2*] to output URL to HTML version of commitdiff.
* "commitdiff_plain" is generated by git-diff-tree, and has therefore has git specific extensions to diff format: "git diff" header and optional extended header lines.
FOOTNOTES
[*1*] First it generated rev-list starting from HEAD even if hash_base parameter was set, second it wasn't corrected according to changes made in git_get_references (formerly read_info_ref) output, third even for older version of read_info_ref output it didn't work for multiple tags pointing to the current commit (rare).
[*2*] It wrote URL for commitdiff without hash_parent, which produces diff to first parent and is not the same as current diff if it is diff of merge commit to non-first parent.
--- gitweb/gitweb.perl | 323 ++++++++++++++++++++++++++++++++-------------------- 1 files changed, 201 insertions(+), 122 deletions(-)
gitweb/gitweb.perl +201 -122
@@ -524,6 +524,27 @@ sub format_subject_html { } } +sub format_diff_line { + my $line = shift; + my $char = substr($line, 0, 1); + my $diff_class = ""; + + chomp $line; + + if ($char eq '+') { + $diff_class = " add"; + } elsif ($char eq "-") { + $diff_class = " rem"; + } elsif ($char eq "@") { + $diff_class = " chunk_header"; + } elsif ($char eq "\\") { + # skip errors (incomplete lines) + return ""; + } + $line = untabify($line); + return "<div class=\"diff$diff_class\">" . esc_html($line) . "</div>\n"; +} + ## ---------------------------------------------------------------------- ## git utility subroutines, invoking git commands
@@ -1367,7 +1388,7 @@ ## ..................................... ## functions printing large fragments of HTML sub git_difftree_body { - my ($difftree, $parent) = @_; + my ($difftree, $hash, $parent) = @_; print "<div class=\"list_head\">\n"; if ($#{$difftree} > 10) {
@@ -1518,6 +1539,101 @@ sub git_difftree_body { print "</table>\n"; } +sub git_patchset_body { + my ($patchset, $difftree, $hash, $hash_parent) = @_; + + my $patch_idx = 0; + my $in_header = 0; + my $patch_found = 0; + my %diffinfo; + + print "<div class=\"patchset\">\n"; + + LINE: foreach my $patch_line (@$patchset) { + + if ($patch_line =~ m/^diff /) { # "git diff" header + # beginning of patch (in patchset) + if ($patch_found) { + # close previous patch + print "</div>\n"; # class="patch" + } else { + # first patch in patchset + $patch_found = 1; + } + print "<div class=\"patch\">\n"; + + %diffinfo = parse_difftree_raw_line($difftree->[$patch_idx++]); + + # for now, no extended header, hence we skip empty patches + # companion to next LINE if $in_header; + if ($diffinfo{'from_id'} eq $diffinfo{'to_id'}) { # no change + $in_header = 1; + next LINE; + } + + 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 + 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 "<div class=\"diff extended_header\">\n"; + $in_header = 1; + next LINE; + } # start of patch in patchset + + + if ($in_header && $patch_line =~ m/^---/) { + #print "</div>\n" + $in_header = 0; + } + next LINE if $in_header; + + print format_diff_line($patch_line); + } + print "</div>\n" if $patch_found; # class="patch" + + print "</div>\n"; # class="patchset" +} + +# . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . + sub git_shortlog_body { # uses global variable $project my ($revlist, $from, $to, $refs, $extra) = @_;
@@ -2556,7 +2672,7 @@ sub git_commit { git_print_log($co{'comment'}); print "</div>\n"; - git_difftree_body(\@difftree, $parent); + git_difftree_body(\@difftree, $hash, $parent); git_footer_html(); }
@@ -2600,7 +2716,7 @@ sub git_blobdiff_plain { } sub git_commitdiff { - mkdir($git_temp, 0700); + my $format = shift || 'html'; my %co = parse_commit($hash); if (!%co) { die_error(undef, "Unknown commit object");
@@ -2608,143 +2724,106 @@ sub git_commitdiff { if (!defined $hash_parent) { $hash_parent = $co{'parent'} || '--root'; } - open my $fd, "-|", $GIT, "diff-tree", '-r', $hash_parent, $hash - or die_error(undef, "Open git-diff-tree failed"); - my @difftree = map { chomp; $_ } <$fd>; - close $fd or die_error(undef, "Reading git-diff-tree failed"); + + # read commitdiff + my $fd; + my @difftree; + my @patchset; + if ($format eq 'html') { + open $fd, "-|", $GIT, "diff-tree", '-r', '-M', '-C', + "--patch-with-raw", "--full-index", $hash_parent, $hash + or die_error(undef, "Open git-diff-tree failed"); + + while (chomp(my $line = <$fd>)) { + # empty line ends raw part of diff-tree output + last unless $line; + push @difftree, $line; + } + @patchset = map { chomp; $_ } <$fd>; + + close $fd + or die_error(undef, "Reading git-diff-tree failed"); + } elsif ($format eq 'plain') { + open $fd, "-|", $GIT, "diff-tree", '-r', '-p', '-B', $hash_parent, $hash + or die_error(undef, "Open git-diff-tree failed"); + } else { + die_error(undef, "Unknown commitdiff format"); + } # non-textual hash id's can be cached my $expires; if ($hash =~ m/^[0-9a-fA-F]{40}$/) { $expires = "+1d"; } - my $refs = git_get_references(); - my $ref = format_ref_marker($refs, $co{'id'}); - my $formats_nav = - $cgi->a({-href => href(action=>"commitdiff_plain", hash=>$hash, hash_parent=>$hash_parent)}, - "plain"); - git_header_html(undef, $expires); - git_print_page_nav('commitdiff','', $hash,$co{'tree'},$hash, $formats_nav); - git_print_header_div('commit', esc_html($co{'title'}) . $ref, $hash); - print "<div class=\"page_body\">\n"; - git_print_simplified_log($co{'comment'}, 1); # skip title - print "<br/>\n"; - foreach my $line (@difftree) { - # ':100644 100644 03b218260e99b78c6df0ed378e59ed9205ccc96d 3b93d5e7cc7f7dd4ebed13a5cc1a4ad976fc94d8 M ls-files.c' - # ':100644 100644 7f9281985086971d3877aca27704f2aaf9c448ce bc190ebc71bbd923f2b728e505408f5e54bd073a M rev-tree.c' - if ($line !~ m/^:([0-7]{6}) ([0-7]{6}) ([0-9a-fA-F]{40}) ([0-9a-fA-F]{40}) (.)\t(.*)$/) { - next; - } - my $from_mode = $1; - my $to_mode = $2; - my $from_id = $3; - my $to_id = $4; - my $status = $5; - my $file = validate_input(unquote($6)); - if ($status eq "A") { - print "<div class=\"diff_info\">" . file_type($to_mode) . ":" . - $cgi->a({-href => href(action=>"blob", hash_base=>$hash, - hash=>$to_id, file_name=>$file)}, - $to_id) . "(new)" . - "</div>\n"; - git_diff_print(undef, "/dev/null", $to_id, "b/$file"); - } elsif ($status eq "D") { - print "<div class=\"diff_info\">" . file_type($from_mode) . ":" . - $cgi->a({-href => href(action=>"blob", hash_base=>$hash_parent, - hash=>$from_id, file_name=>$file)}, - $from_id) . "(deleted)" . - "</div>\n"; - git_diff_print($from_id, "a/$file", undef, "/dev/null"); - } elsif ($status eq "M") { - if ($from_id ne $to_id) { - print "<div class=\"diff_info\">" . - file_type($from_mode) . ":" . - $cgi->a({-href => href(action=>"blob", hash_base=>$hash_parent, - hash=>$from_id, file_name=>$file)}, - $from_id) . - " -> " . - file_type($to_mode) . ":" . - $cgi->a({-href => href(action=>"blob", hash_base=>$hash, - hash=>$to_id, file_name=>$file)}, - $to_id); - print "</div>\n"; - git_diff_print($from_id, "a/$file", $to_id, "b/$file"); - } - } - } - print "<br/>\n" . - "</div>"; - git_footer_html(); -} -sub git_commitdiff_plain { - mkdir($git_temp, 0700); - my %co = parse_commit($hash); - if (!%co) { - die_error(undef, "Unknown commit object"); - } - if (!defined $hash_parent) { - $hash_parent = $co{'parent'} || '--root'; - } - open my $fd, "-|", $GIT, "diff-tree", '-r', $hash_parent, $hash - or die_error(undef, "Open git-diff-tree failed"); - my @difftree = map { chomp; $_ } <$fd>; - close $fd or die_error(undef, "Reading diff-tree failed"); - - # try to figure out the next tag after this commit - my $tagname; - my $refs = git_get_references("tags"); - open $fd, "-|", $GIT, "rev-list", "HEAD"; - my @commits = map { chomp; $_ } <$fd>; - close $fd; - foreach my $commit (@commits) { - if (defined $refs->{$commit}) { - $tagname = $refs->{$commit} - } - if ($commit eq $hash) { - last; - } - } + # write commit message + if ($format eq 'html') { + my $refs = git_get_references(); + my $ref = format_ref_marker($refs, $co{'id'}); + my $formats_nav = + $cgi->a({-href => href(action=>"commitdiff_plain", + hash=>$hash, hash_parent=>$hash_parent)}, + "plain"); - print $cgi->header(-type => "text/plain", - -charset => 'utf-8', - -content_disposition => "inline; filename=\"git-$hash.patch\""); - my %ad = parse_date($co{'author_epoch'}, $co{'author_tz'}); - my $comment = $co{'comment'}; - print <<TEXT; + git_header_html(undef, $expires); + git_print_page_nav('commitdiff','', $hash,$co{'tree'},$hash, $formats_nav); + git_print_header_div('commit', esc_html($co{'title'}) . $ref, $hash); + print "<div class=\"page_body\">\n"; + print "<div class=\"log\">\n"; + git_print_simplified_log($co{'comment'}, 1); # skip title + print "</div>\n"; # class="log" + + } elsif ($format eq 'plain') { + my $refs = git_get_references("tags"); + my @tagnames; + if (exists $refs->{$hash}) { + @tagnames = map { s|^tags/|| } $refs->{$hash}; + } + my $filename = basename($project) . "-$hash.patch"; + + print $cgi->header( + -type => 'text/plain', + -charset => 'utf-8', + -expires => $expires, + -content_disposition => qq(inline; filename="$filename")); + my %ad = parse_date($co{'author_epoch'}, $co{'author_tz'}); + print <<TEXT; From: $co{'author'} Date: $ad{'rfc2822'} ($ad{'tz_local'}) Subject: $co{'title'} TEXT - if (defined $tagname) { - print "X-Git-Tag: $tagname\n"; + foreach my $tag (@tagnames) { + print "X-Git-Tag: $tag\n"; + } + print "X-Git-Url: " . $cgi->self_url() . "\n\n"; + foreach my $line (@{$co{'comment'}}) { + print "$line\n"; + } + print "---\n\n"; } - print "X-Git-Url: $my_url?p=$project;a=commitdiff;h=$hash\n" . - "\n"; - foreach my $line (@$comment) {; - print "$line\n"; - } - print "---\n\n"; + # write patch + if ($format eq 'html') { + #git_difftree_body(\@difftree, $hash, $hash_parent); + #print "<br/>\n"; - foreach my $line (@difftree) { - if ($line !~ m/^:([0-7]{6}) ([0-7]{6}) ([0-9a-fA-F]{40}) ([0-9a-fA-F]{40}) (.)\t(.*)$/) { - next; - } - my $from_id = $3; - my $to_id = $4; - my $status = $5; - my $file = $6; - if ($status eq "A") { - git_diff_print(undef, "/dev/null", $to_id, "b/$file", "plain"); - } elsif ($status eq "D") { - git_diff_print($from_id, "a/$file", undef, "/dev/null", "plain"); - } elsif ($status eq "M") { - git_diff_print($from_id, "a/$file", $to_id, "b/$file", "plain"); - } + git_patchset_body(\@patchset, \@difftree, $hash, $hash_parent); + + print "</div>\n"; # class="page_body" + git_footer_html(); + + } elsif ($format eq 'plain') { + local $/ = undef; + print <$fd>; + close $fd + or print "Reading git-diff-tree failed\n"; } } +sub git_commitdiff_plain { + git_commitdiff('plain'); +} + sub git_history { if (!defined $hash_base) { $hash_base = git_get_head_hash($project);
Signature · 1 line
1.4.1.1
PATCH 1/1 gitweb: Replace git_commitdiff_plain by anonymous subroutine
--- gitweb/gitweb.perl | 6 +----- 1 files changed, 1 insertions(+), 5 deletions(-)
gitweb/gitweb.perl +1 -5
@@ -234,7 +234,7 @@ my %actions = ( "blob" => \&git_blob, "blob_plain" => \&git_blob_plain, "commitdiff" => \&git_commitdiff, - "commitdiff_plain" => \&git_commitdiff_plain, + "commitdiff_plain" => sub { git_commitdiff('plain'); }, "commit" => \&git_commit, "heads" => \&git_heads, "history" => \&git_history,
@@ -2820,10 +2820,6 @@ TEXT } } -sub git_commitdiff_plain { - git_commitdiff('plain'); -} - sub git_history { if (!defined $hash_base) { $hash_base = git_get_head_hash($project);
Signature · 1 line
1.4.1.1
PATCH 1/1 gitweb: Show information about incomplete lines in commitdiff
In reply to Jakub NarebskiIn format_diff_line, instead of skipping errors/incomplete lines, for example
"\ No newline at end of file"
in HTML pretty-printing of diff, use "incomplete" class for div.
--- gitweb/gitweb.css | 4 ++++ gitweb/gitweb.perl | 2 +- 2 files changed, 5 insertions(+), 1 deletions(-)
gitweb/gitweb.css +4 -0
@@ -285,6 +285,10 @@ div.diff.chunk_header { color: #990099; } +div.diff.incomplete { + color: #cccccc; +} + div.diff_info { font-family: monospace; color: #000099;
gitweb/gitweb.perl +1 -1
@@ -539,7 +539,7 @@ sub format_diff_line { $diff_class = " chunk_header"; } elsif ($char eq "\\") { # skip errors (incomplete lines) - return ""; + $diff_class = " incomplete"; } $line = untabify($line); return "<div class=\"diff$diff_class\">" . esc_html($line) . "</div>\n";
Signature · 1 line
1.4.1.1
Re: [PATCH 3] gitweb: Show information about incomplete lines in commitdiff
Quoted 6 lines · Jakub Narebski
In
format_diff_line, instead of skipping errors/incomplete lines, for example
"\ No newline at end of file"
in HTML pretty-printing of diff, use "incomplete" class for div.Signed-off-by Jakub Narebski <[email protected]>
I think this makes sense, while I do not see much point in removal of git_commitdiff_plain.
Re: [PATCH 3] gitweb: Show information about incomplete lines in commitdiff
Quoted 2 lines · Junio C Hamano
I do not see much point in
removal ofgit_commitdiff_plain.
So should I split git_commitdiff into two subroutines, or should I left it as it is after
gitweb: Use git-diff-tree patch output for commitdiff
i.e. git_commitdiff('plain') for git_commitdiff_plain?
Signature · 3 lines
Jakub Narebski Warsaw, Poland ShadeHawk on #git
PATCH 1/1 gitweb: Remove invalid comment in format_diff_line
In reply to Jakub Narebski---
This patch and all subsequent patches assumes (although this should have no bearing) that patch
[PATCH 2] gitweb: Replace git_commitdiff_plain by anonymous subroutine
was not applied. (In my repository this commit had been simply reverted).
gitweb/gitweb.perl | 1 - 1 files changed, 0 insertions(+), 1 deletions(-)
gitweb/gitweb.perl +0 -1
@@ -538,7 +538,6 @@ sub format_diff_line { } elsif ($char eq "@") { $diff_class = " chunk_header"; } elsif ($char eq "\\") { - # skip errors (incomplete lines) $diff_class = " incomplete"; } $line = untabify($line);
Signature · 1 line
1.4.1.1
PATCH 1/1 gitweb: Streamify patch output in git_commitdiff
In reply to Jakub NarebskiChange output of patch(set) in git_commitdiff from slurping whole diff in @patchset array before processing, to passing file descriptor to git_patchset_body.
Advantages: faster, incremental output, smaller memory footprint.
Disadvantages: cannot react when there is error during closing file descriptor.
---
Quoted 3 lines · Junio C Hamano
I do not know we would want to slurp the entier diff in an array before processing. Is this easy to streamify by passing an pipe fd to the formatting sub?
This patch adresses that.
gitweb/gitweb.perl | 14 +++++++------- 1 files changed, 7 insertions(+), 7 deletions(-)
gitweb/gitweb.perl +7 -7
@@ -1539,7 +1539,7 @@ sub git_difftree_body { } sub git_patchset_body { - my ($patchset, $difftree, $hash, $hash_parent) = @_; + my ($fd, $difftree, $hash, $hash_parent) = @_; my $patch_idx = 0; my $in_header = 0;
@@ -1548,7 +1548,9 @@ sub git_patchset_body { print "<div class=\"patchset\">\n"; - LINE: foreach my $patch_line (@$patchset) { + LINE: + while (my $patch_line = <$fd>) { + chomp $patch_line; if ($patch_line =~ m/^diff /) { # "git diff" header # beginning of patch (in patchset)
@@ -2727,7 +2729,6 @@ sub git_commitdiff { # read commitdiff my $fd; my @difftree; - my @patchset; if ($format eq 'html') { open $fd, "-|", $GIT, "diff-tree", '-r', '-M', '-C', "--patch-with-raw", "--full-index", $hash_parent, $hash
@@ -2738,13 +2739,11 @@ sub git_commitdiff { last unless $line; push @difftree, $line; } - @patchset = map { chomp; $_ } <$fd>; - close $fd - or die_error(undef, "Reading git-diff-tree failed"); } elsif ($format eq 'plain') { open $fd, "-|", $GIT, "diff-tree", '-r', '-p', '-B', $hash_parent, $hash or die_error(undef, "Open git-diff-tree failed"); + } else { die_error(undef, "Unknown commitdiff format"); }
@@ -2806,7 +2805,8 @@ TEXT #git_difftree_body(\@difftree, $hash, $hash_parent); #print "<br/>\n"; - git_patchset_body(\@patchset, \@difftree, $hash, $hash_parent); + git_patchset_body($fd, \@difftree, $hash, $hash_parent); + close $fd; print "</div>\n"; # class="page_body" git_footer_html();
Signature · 1 line
1.4.1.1
PATCH 1/1 gitweb: Add git_get_{following,preceding}_references functions
In reply to Jakub NarebskiAdds git_get_following_references function, based on code which was used in git_commitdiff_plain to generate X-Git-Tag: header, and companion git_get_preceding_references function.
Both functions return array of all references of given type (as returned by git_get_references) following/preceding given commit in array (list) context, and last following/first preceding ref in scalar context.
Stripping ref (list of refs) of "$type/" (e.g. "tags/") is left to caller.
---
Quoted 9 lines · Junio C Hamano
Quoted 2 lines
* "
commitdiff_plain" now only generates X-Git-Tag: line only if there is tag pointing to the current commit.Hmph...
Quoted 3 lines
...; besides we are
interested rather in tags _preceding_ the commit, and _heads_ following the commit.
Quoted 7 lines
Interesting observation. When somebody says "feature X was introduced in such and such commit", people would want to know (1) the point release they are using has the feature -- which means you would want to know the earliest tag that comes after the commit, or
(2) if the branch they are working on already has that feature -- which again means if the head follows the commit. So I am not sure when preceding tag is interesting...
Actually, the two following patches adresses this, not this patch...
gitweb/gitweb.perl | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 52 insertions(+), 0 deletions(-)
gitweb/gitweb.perl +52 -0
@@ -749,6 +749,58 @@ sub git_get_references { return \%refs; } +sub git_get_following_references { + my $hash = shift || return undef; + my $type = shift; + my $base = shift || $hash_base || "HEAD"; + + my $refs = git_get_references($type); + open my $fd, "-|", $GIT, "rev-list", $base + or return undef; + my @commits = map { chomp; $_ } <$fd>; + close $fd + or return undef; + + my @reflist; + my $lastref; + + foreach my $commit (@commits) { + foreach my $ref (@{$refs->{$commit}}) { + $lastref = $ref; + push @reflist, $lastref; + } + if ($commit eq $hash) { + last; + } + } + + return wantarray ? @reflist : $lastref; +} + +sub git_get_preceding_references { + my $hash = shift || return undef; + my $type = shift; + + my $refs = git_get_references($type); + open my $fd, "-|", $GIT, "rev-list", $hash + or return undef; + my @commits = map { chomp; $_ } <$fd>; + close $fd + or return undef; + + my @reflist; + my $firstref; + + foreach my $commit (@commits) { + foreach my $ref (@{$refs->{$commit}}) { + $firstref = $ref unless $firstref; + push @reflist, $ref; + } + } + + return wantarray ? @reflist : $firstref; +} + ## ---------------------------------------------------------------------- ## parse to hash functions
Signature · 1 line
1.4.1.1
PATCH 1/1 gitweb: Faster return from git_get_preceding_references if possible
In reply to Jakub NarebskiReturn on first ref found when git_get_preceding_references is called in scalar context
---
Quoted 1 line · Jakub Narebski
Actually, the two following patches adresses this, not this patch...
The two following this patch...
gitweb/gitweb.perl | 5 ++--- 1 files changed, 2 insertions(+), 3 deletions(-)
gitweb/gitweb.perl +2 -3
@@ -789,16 +789,15 @@ sub git_get_preceding_references { or return undef; my @reflist; - my $firstref; foreach my $commit (@commits) { foreach my $ref (@{$refs->{$commit}}) { - $firstref = $ref unless $firstref; + return $ref unless wantarray; push @reflist, $ref; } } - return wantarray ? @reflist : $firstref; + return @reflist; } ## ----------------------------------------------------------------------
Signature · 1 line
1.4.1.1
PATCH 1/1 gitweb: Add git_get_rev_name_tags function
In reply to Jakub NarebskiAdd git_get_rev_name_tags function, for later use in git_commitdiff('plain') for X-Git-Tag: header.
This function, contrary to the call to
git_get_following_references($hash, "tags");
_does_ strip "tags/" and returns bare tag name.
---
Quoted 9 lines · Junio C Hamano
Quoted 2 lines
* "
commitdiff_plain" now only generates X-Git-Tag: line only if there is tag pointing to the current commit.Hmph...
Quoted 3 lines
...; besides we are
interested rather in tags _preceding_ the commit, and _heads_ following the commit.
Quoted 7 lines
Interesting observation. When somebody says "feature X was introduced in such and such commit", people would want to know (1) the point release they are using has the feature -- which means you would want to know the earliest tag that comes after the commit, or
(2) if the branch they are working on already has that feature -- which again means if the head follows the commit. So I am not sure when preceding tag is interesting...
This commit introduces function which does job that was intended I guess for X-Git-Tag: header... perhaps it should be renamed...
gitweb/gitweb.perl | 16 ++++++++++++++++ 1 files changed, 16 insertions(+), 0 deletions(-)
gitweb/gitweb.perl +16 -0
@@ -800,6 +800,22 @@ sub git_get_preceding_references { return @reflist; } +sub git_get_rev_name_tags { + my $hash = shift || return undef; + + open my $fd, "-|", $GIT, "name-rev", "--tags", $hash + or return; + my $name_rev = <$fd>; + close $fd; + + if ($name_rev =~ m|^$hash tags/(.*)$|) { + return $1; + } else { + # catches also '$hash undefined' output + return undef; + } +} + ## ---------------------------------------------------------------------- ## parse to hash functions
Signature · 1 line
1.4.1.1
PATCH 1/1 gitweb: Use git_get_name_rev_tags for commitdiff_plain X-Git-Tag: header
In reply to Jakub NarebskiUse git_get_rev_name_tags function for X-Git-Tag: header in git_commitdiff('plain'), i.e. for commitdiff_plain action.
---
For example, commitdiff_plain for 6c7f4cebdb40c0d95c63d59538fd235dcf978029 commit -- the following query string:
a=commitdiff_plain;p=git.git;h=6c7f4cebdb40c0d95c63d59538fd235dcf978029
outputs the following X-Git-Tag: header:
X-Git-Tag: v1.4.2^0~2
gitweb/gitweb.perl | 10 +++------- 1 files changed, 3 insertions(+), 7 deletions(-)
gitweb/gitweb.perl +3 -7
@@ -2840,10 +2840,7 @@ sub git_commitdiff { } elsif ($format eq 'plain') { my $refs = git_get_references("tags"); - my @tagnames; - if (exists $refs->{$hash}) { - @tagnames = map { s|^tags/|| } $refs->{$hash}; - } + my $tagname = git_get_rev_name_tags($hash); my $filename = basename($project) . "-$hash.patch"; print $cgi->header(
@@ -2857,10 +2854,9 @@ From: $co{'author'} Date: $ad{'rfc2822'} ($ad{'tz_local'}) Subject: $co{'title'} TEXT - foreach my $tag (@tagnames) { - print "X-Git-Tag: $tag\n"; - } + print "X-Git-Tag: $tagname\n" if $tagname; print "X-Git-Url: " . $cgi->self_url() . "\n\n"; + foreach my $line (@{$co{'comment'}}) { print "$line\n"; }
Signature · 1 line
1.4.1.1
PATCH 1/1 gitweb: Add support for hash_parent_base parameter for blobdiffs
In reply to Jakub NarebskiAdd support for hash_parent_base in input validation part and in href() function. Add proper hash_parent_base to all calls to blobdiff and blobdiff_plain action URLs.
To be used in future rewrite of git_blobdiff and git_blobdiff_plain.
While at it, move project before action in ordering CGI parameters in href().
---
Such an easy commitdiff_plain link wouldn't be possible without href() function, which filters out undefined parameters.
gitweb/gitweb.perl | 35 ++++++++++++++++++++++++++--------- 1 files changed, 26 insertions(+), 9 deletions(-)
gitweb/gitweb.perl +26 -9
@@ -211,6 +211,13 @@ if (defined $hash_base) { } } +our $hash_parent_base = $cgi->param('hpb'); +if (defined $hash_parent_base) { + if (!validate_input($hash_parent_base)) { + die_error(undef, "Invalid hash parent base parameter"); + } +} + our $page = $cgi->param('pg'); if (defined $page) { if ($page =~ m/[^0-9]$/) {
@@ -270,13 +277,14 @@ sub href(%) { my %params = @_; my @mapping = ( - action => "a", project => "p", + action => "a", file_name => "f", file_parent => "fp", hash => "h", hash_parent => "hp", hash_base => "hb", + hash_parent_base => "hpb", page => "pg", searchtext => "s", );
@@ -1543,8 +1551,10 @@ sub git_difftree_body { } print "<td>"; if ($diff{'to_id'} ne $diff{'from_id'}) { # modified - print $cgi->a({-href => href(action=>"blobdiff", hash=>$diff{'to_id'}, hash_parent=>$diff{'from_id'}, - hash_base=>$hash, file_name=>$diff{'file'}), + print $cgi->a({-href => href(action=>"blobdiff", + hash=>$diff{'to_id'}, hash_parent=>$diff{'from_id'}, + hash_base=>$hash, hash_parent_base=>$parent, + file_name=>$diff{'file'}), -class => "list"}, esc_html($diff{'file'})); } else { # only mode changed print $cgi->a({-href => href(action=>"blob", hash=>$diff{'to_id'},
@@ -1559,8 +1569,10 @@ sub git_difftree_body { "blob"); if ($diff{'to_id'} ne $diff{'from_id'}) { # modified print " | " . - $cgi->a({-href => href(action=>"blobdiff", hash=>$diff{'to_id'}, hash_parent=>$diff{'from_id'}, - hash_base=>$hash, file_name=>$diff{'file'})}, + $cgi->a({-href => href(action=>"blobdiff", + hash=>$diff{'to_id'}, hash_parent=>$diff{'from_id'}, + hash_base=>$hash, hash_parent_base=>$parent, + file_name=>$diff{'file'})}, "diff"); } print " | " .
@@ -1592,8 +1604,9 @@ sub git_difftree_body { "blob"); if ($diff{'to_id'} ne $diff{'from_id'}) { print " | " . - $cgi->a({-href => href(action=>"blobdiff", hash_base=>$hash, + $cgi->a({-href => href(action=>"blobdiff", hash=>$diff{'to_id'}, hash_parent=>$diff{'from_id'}, + hash_base=>$hash, hash_parent_base=>$parent, file_name=>$diff{'to_file'}, file_parent=>$diff{'from_file'})}, "diff"); }
@@ -1793,8 +1806,10 @@ sub git_history_body { if (defined $blob_current && defined $blob_parent && $blob_current ne $blob_parent) { print " | " . - $cgi->a({-href => href(action=>"blobdiff", hash=>$blob_current, hash_parent=>$blob_parent, - hash_base=>$commit, file_name=>$file_name)}, + $cgi->a({-href => href(action=>"blobdiff", + hash=>$blob_current, hash_parent=>$blob_parent, + hash_base=>$hash_base, hash_parent_base=>$commit, + file_name=>$file_name)}, "diff to current"); } }
@@ -2751,7 +2766,9 @@ sub git_blobdiff { if (defined $hash_base && (my %co = parse_commit($hash_base))) { my $formats_nav = $cgi->a({-href => href(action=>"blobdiff_plain", - hash=>$hash, hash_parent=>$hash_parent)}, + hash=>$hash, hash_parent=>$hash_parent, + hash_base=>$hash_base, hash_parent_base=>$hash_parent_base, + file_name=>$file_name, file_parent=>$file_parent)}, "plain"); git_print_page_nav('','', $hash_base,$co{'tree'},$hash_base, $formats_nav); git_print_header_div('commit', esc_html($co{'title'}), $hash_base);
Signature · 1 line
1.4.1.1
PATCH 1/1 gitweb: Add support for hash_parent_base parameter for blobdiffs
In reply to Jakub NarebskiAdd support for hash_parent_base in input validation part and in href() function. Add proper hash_parent_base to all calls to blobdiff and blobdiff_plain action URLs. Use hash_parent_base as hash_base for blobs of hash_parent.
To be used in future rewrite of git_blobdiff and git_blobdiff_plain.
While at it, move project before action in ordering CGI parameters in href().
--- gitweb/gitweb.perl | 37 +++++++++++++++++++++++++++---------- 1 files changed, 27 insertions(+), 10 deletions(-)
gitweb/gitweb.perl +27 -10
@@ -211,6 +211,13 @@ if (defined $hash_base) { } } +our $hash_parent_base = $cgi->param('hpb'); +if (defined $hash_parent_base) { + if (!validate_input($hash_parent_base)) { + die_error(undef, "Invalid hash parent base parameter"); + } +} + our $page = $cgi->param('pg'); if (defined $page) { if ($page =~ m/[^0-9]$/) {
@@ -270,13 +277,14 @@ sub href(%) { my %params = @_; my @mapping = ( - action => "a", project => "p", + action => "a", file_name => "f", file_parent => "fp", hash => "h", hash_parent => "hp", hash_base => "hb", + hash_parent_base => "hpb", page => "pg", searchtext => "s", );
@@ -1543,8 +1551,10 @@ sub git_difftree_body { } print "<td>"; if ($diff{'to_id'} ne $diff{'from_id'}) { # modified - print $cgi->a({-href => href(action=>"blobdiff", hash=>$diff{'to_id'}, hash_parent=>$diff{'from_id'}, - hash_base=>$hash, file_name=>$diff{'file'}), + print $cgi->a({-href => href(action=>"blobdiff", + hash=>$diff{'to_id'}, hash_parent=>$diff{'from_id'}, + hash_base=>$hash, hash_parent_base=>$parent, + file_name=>$diff{'file'}), -class => "list"}, esc_html($diff{'file'})); } else { # only mode changed print $cgi->a({-href => href(action=>"blob", hash=>$diff{'to_id'},
@@ -1559,8 +1569,10 @@ sub git_difftree_body { "blob"); if ($diff{'to_id'} ne $diff{'from_id'}) { # modified print " | " . - $cgi->a({-href => href(action=>"blobdiff", hash=>$diff{'to_id'}, hash_parent=>$diff{'from_id'}, - hash_base=>$hash, file_name=>$diff{'file'})}, + $cgi->a({-href => href(action=>"blobdiff", + hash=>$diff{'to_id'}, hash_parent=>$diff{'from_id'}, + hash_base=>$hash, hash_parent_base=>$parent, + file_name=>$diff{'file'})}, "diff"); } print " | " .
@@ -1592,8 +1604,9 @@ sub git_difftree_body { "blob"); if ($diff{'to_id'} ne $diff{'from_id'}) { print " | " . - $cgi->a({-href => href(action=>"blobdiff", hash_base=>$hash, + $cgi->a({-href => href(action=>"blobdiff", hash=>$diff{'to_id'}, hash_parent=>$diff{'from_id'}, + hash_base=>$hash, hash_parent_base=>$parent, file_name=>$diff{'to_file'}, file_parent=>$diff{'from_file'})}, "diff"); }
@@ -1793,8 +1806,10 @@ sub git_history_body { if (defined $blob_current && defined $blob_parent && $blob_current ne $blob_parent) { print " | " . - $cgi->a({-href => href(action=>"blobdiff", hash=>$blob_current, hash_parent=>$blob_parent, - hash_base=>$commit, file_name=>$file_name)}, + $cgi->a({-href => href(action=>"blobdiff", + hash=>$blob_current, hash_parent=>$blob_parent, + hash_base=>$hash_base, hash_parent_base=>$commit, + file_name=>$file_name)}, "diff to current"); } }
@@ -2751,7 +2766,9 @@ sub git_blobdiff { if (defined $hash_base && (my %co = parse_commit($hash_base))) { my $formats_nav = $cgi->a({-href => href(action=>"blobdiff_plain", - hash=>$hash, hash_parent=>$hash_parent)}, + hash=>$hash, hash_parent=>$hash_parent, + hash_base=>$hash_base, hash_parent_base=>$hash_parent_base, + file_name=>$file_name, file_parent=>$file_parent)}, "plain"); git_print_page_nav('','', $hash_base,$co{'tree'},$hash_base, $formats_nav); git_print_header_div('commit', esc_html($co{'title'}), $hash_base);
@@ -2765,7 +2782,7 @@ HTML print "<div class=\"page_body\">\n" . "<div class=\"diff_info\">blob:" . $cgi->a({-href => href(action=>"blob", hash=>$hash_parent, - hash_base=>$hash_base, file_name=>($file_parent || $file_name))}, + hash_base=>$hash_parent_base, file_name=>($file_parent || $file_name))}, $hash_parent) . " -> blob:" . $cgi->a({-href => href(action=>"blob", hash=>$hash,
Signature · 1 line
1.4.1.1
Quoted 3 lines · Jakub Narebski
Converting "blobdiff" and "
blobdiff_plain" format would be much easier if git-diff-tree and friends had -L <label>/--label=<label> option, like GNU diff has.
I am not sure how that would be useful, given that you would disect the header line-by-line to colorize anyway.
Quoted 5 lines
Current patch preserves much of current output; the question is if for example generate if 'plain' format should generate patch which could be appplied by ordinary patch which do not understand git diff extensions (including renaming and copying), as it is done in current version, and if 'html' version should detect renames and copying.
I would say html is definitely for human consumption; does anybody cut&paste html patch and expect to apply that? Plain format I am easy but probably enabling rename is fine. You can edit the header or tell patch to apply to which file anyway, and I think the value of being able to view the real changes outweigh that inconvenience.
Quoted 2 lines
* "commitdiff" now products patches with renaming and copying detection (git-diff-tree is invoked with -M and -C options).
You do not have to give -M and -C; a single -C is enough. I wonder if -B is also useful as a default (i.e. -B -C).
For a merge, I often would want to see --cc just like gitk does, but it is probably just me.
I do not know we would want to slurp the entier diff in an array before processing. Is this easy to streamify by passing an pipe fd to the formatting sub?
Quoted 2 lines
Empty patches (mode changes and pure renames and copying) are not written currently.
That's quite bad.
Quoted 5 lines
* "commitdiff" output is now divided into several div elements of class "log", "patchset" and "patch".
* "
commitdiff_plain" now only generates X-Git-Tag: line only if there is tag pointing to the current commit.
Hmph...
Quoted 3 lines
...; besides we are
interested rather in tags _preceding_ the commit, and _heads_ following the commit.
Interesting observation. When somebody says "feature X was introduced in such and such commit", people would want to know (1) the point release they are using has the feature -- which means you would want to know the earliest tag that comes after the commit, or (2) if the branch they are working on already has that feature -- which again means if the head follows the commit. So I am not sure when preceding tag is interesting...
Quoted 8 lines · Junio C Hamano
Quoted 3 lines · Jakub Narebski
Converting "blobdiff" and "
blobdiff_plain" format would be much easier if git-diff-tree and friends had -L <label>/--label=<label> option, like GNU diff has.I am not sure how that would be useful, given that you would disect the header line-by-line to colorize anyway.
gitweb could output patch directly (slurp-print) in blobdiff_plain, if there were -L <label>/--label=<label> option to git-diff. As it is now git_blobdiff_plain (or git_blobdiff('plain')) would have to process diff header, replacing hashes by file names.
Anyway to get full information you need commits to diff, not only blobs to diff, otherwise the mode change information is lost, I think. And then we would be able to reus git_patchset_body for git_blobdiff...
Quoted 6 lines
Quoted 5 lines
Current patch preserves much of current output; the question is if for example generate if 'plain' format should generate patch which could be appplied by ordinary patch which do not understand git diff extensions (including renaming and copying), as it is done in current version, and if 'html' version should detect renames and copying.
Quoted 6 lines
I would say html is definitely for human consumption; does anybody cut&paste html patch and expect to apply that? Plain format I am easy but probably enabling rename is fine. You can edit the header or tell patch to apply to which file anyway, and I think the value of being able to view the real changes outweigh that inconvenience.
Or I we can add another format/option, 'broken' to git_commitdiff and friends.
Quoted 5 lines
Quoted 2 lines
* "commitdiff" now products patches with renaming and copying detection (git-diff-tree is invoked with -M and -C options).
You do not have to give -M and -C; a single -C is enough. I wonder if -B is also useful as a default (i.e. -B -C).
So -C implies -M?
Quoted 2 lines
For a merge, I often would want to see --cc just like gitk does, but it is probably just me.
Planned. The problem is that raw format for --cc differs, and parser has to be improved (and similarity is lost)
Quoted 3 lines
I do not know we would want to slurp the entier diff in an array before processing. Is this easy to streamify by passing an pipe fd to the formatting sub?
That was one question I meant to ask: slurp entire diff (process then output rule) or streamify (streamify larger output for faster result)? Will do (quite easy).
Quoted 4 lines
Quoted 2 lines
Empty patches (mode changes and pure renames and copying) are not written currently.
That's quite bad.
This can be easily changed. Question: what format? Current "gitweb diff header" (<filetype>:<sha1 link> -> <filetype>:<sha1 link>) has no place for that. Can be easily changed.
Another question: do output difftree in commitdiff, like in commit view? Do write out extended header, perhaps except index line (repeats information in "gitweb diff header")?
Quoted 9 lines
Quoted 2 lines
* "
commitdiff_plain" now only generates X-Git-Tag: line only if there is tag pointing to the current commit.Hmph...
Quoted 3 lines
...; besides we are
interested rather in tags _preceding_ the commit, and _heads_ following the commit.
Quoted 7 lines
Interesting observation. When somebody says "feature X was introduced in such and such commit", people would want to know (1) the point release they are using has the feature -- which means you would want to know the earliest tag that comes after the commit, or (2) if the branch they are working on already has that feature -- which again means if the head follows the commit. So I am not sure when preceding tag is interesting...
Qgit and gitk both show "Follows:" listing followed = preceding tag(s). This answers question if given commit has given feature.
I have planned separating generation of first/all preceding/following tags/heads (from git_get_references) into subroutines anyway.
I will add 'X-Git-Tag-After:' then.
Signature · 3 lines
Jakub Narebski Warsaw, Poland ShadeHawk on #git
Quoted 8 lines · Jakub Narebski
Quoted 6 lines · Junio C Hamano
Quoted 3 lines · Jakub Narebski
Converting "blobdiff" and "
blobdiff_plain" format would be much easier if git-diff-tree and friends had -L <label>/--label=<label> option, like GNU diff has.
Quoted 7 lines
Quoted 2 lines
I am not sure how that would be useful, given that you would disect the header line-by-line to colorize anyway.
gitweb could output patch directly (slurp-print) in
blobdiff_plain, if there were -L <label>/--label=<label> option to git-diff. As it is nowgit_blobdiff_plain(orgit_blobdiff('plain')) would have to process diff header, replacing hashes by file names.
I do not think gitweb does diff between two arbitrary blobs; in other words, you only need "diff-tree treeA treeB -- path".
I think feeding object names _is_ what's causing you trouble.
Quoted 13 lines · Junio C Hamano
Quoted 11 lines · Jakub Narebski
Quoted 8 lines · Junio C Hamano
Quoted 3 lines · Jakub Narebski
Converting "blobdiff" and "
blobdiff_plain" format would be much easier if git-diff-tree and friends had -L <label>/--label=<label> option, like GNU diff has.I am not sure how that would be useful, given that you would disect the header line-by-line to colorize anyway.
Quoted 7 lines
Quoted 4 lines
gitweb could output patch directly (slurp-print) in
blobdiff_plain, if there were -L <label>/--label=<label> option to git-diff. As it is nowgit_blobdiff_plain(orgit_blobdiff('plain')) would have to process diff header, replacing hashes by file names.I do not think gitweb does diff between two arbitrary blobs; in other words, you only need "diff-tree treeA treeB -- path".
gitweb didn't pass parent commit (i.e. treeB) information for blobdiff, and IIRC passed only blob hashes for blobdiff_plain (and not passed filename aka path).
Quoted 1 line
I think feeding object names _is_ what's causing you trouble.
Quoted 3 lines · As I
Quoted 3 lines
Anyway to get full information you need commits to diff, not only blobs to diff, otherwise the mode change information is lost, I think. And then we would be able to reuse
git_patchset_bodyforgit_blobdiff...
Instead of using hash and hash_parent parameters for tree-ish and tree_parent-ish hashes (instead of blob hashes as it is now), I have used in patch (in this thread):
[PATCH 10] gitweb: Add support for hash_parent_base parameter for blobdiffs
hash_base and (newly introduced) hash_parent_base arguments/parameters.
Not used yet, but planned.
Signature · 3 lines
Jakub Narebski Warsaw, Poland ShadeHawk on #git
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
Empty patches (mode changes and pure renames and copying) are not written currently.
That's quite bad.
This can be easily changed. Question: what format? Current "gitweb diff
I don't know if has always been like this, but I don't see on gitweb
(http://www.kernel.org/git/?p=qgit/qgit.git;a=shortlog) the diff of the patch I've just pushed.
The patch is "Fix correct permissions on this excutable script", currently the HEAD of qgit repo. The content it's only a file mode change in helpgen.sh, so it' an 'empty' patch.
I've done something wrong or it's gitweb that does not show this info?
Thanks
Marco
Quoted 11 lines · Marco Costalba
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
Empty patches (mode changes and pure renames and copying) are not written currently.
That's quite bad.
This can be easily changed. Question: what format? Current "gitweb diff
I don't know if has always been like this, but I don't see on gitweb
(http://www.kernel.org/git/?p=qgit/qgit.git;a=shortlog) the diff of the patch I've just pushed.
Quoted 5 lines
The patch is "Fix correct permissions on this excutable script", currently the HEAD of qgit repo. The content it's only a file mode change in
helpgen.sh, so it' an 'empty' patch.I've done something wrong or it's gitweb that does not show this info?
It is visible in "commit" view, but is not visible in either "commitdiff", or "blobdiff".
Support for showing this kind of changes is planned... but of course you would have to wait for kernel.org to update their gitweb.
Signature · 3 lines
Jakub Narebski Warsaw, Poland ShadeHawk on #git
PATCH 11/19 gitweb: Allow for pre-parsed difftree info in git_patchset_body
In reply to Jakub NarebskiPreparation for converting git_blobdiff and git_blobdiff_plain to use git-diff-tree patch format to generate patches.
--- gitweb/gitweb.perl | 55 ++++++++++++++++++++++++++++------------------------ 1 files changed, 30 insertions(+), 25 deletions(-)
gitweb/gitweb.perl +30 -25
@@ -1624,7 +1624,7 @@ sub git_patchset_body { my $patch_idx = 0; my $in_header = 0; my $patch_found = 0; - my %diffinfo; + my $diffinfo; print "<div class=\"patchset\">\n";
@@ -1643,54 +1643,59 @@ sub git_patchset_body { } print "<div class=\"patch\">\n"; - %diffinfo = parse_difftree_raw_line($difftree->[$patch_idx++]); + if (ref($difftree->[$patch_idx]) eq "HASH") { + $diffinfo = $difftree->[$patch_idx]; + } else { + $diffinfo = parse_difftree_raw_line($difftree->[$patch_idx]); + } + $patch_idx++; # for now, no extended header, hence we skip empty patches # companion to next LINE if $in_header; - if ($diffinfo{'from_id'} eq $diffinfo{'to_id'}) { # no change + if ($diffinfo->{'from_id'} eq $diffinfo->{'to_id'}) { # no change $in_header = 1; next LINE; } - if ($diffinfo{'status'} eq "A") { # added - print "<div class=\"diff_info\">" . file_type($diffinfo{'to_mode'}) . ":" . + 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)" . + 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'}) . ":" . + } 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)" . + 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 + } elsif ($diffinfo->{'status'} eq "R" || # renamed + $diffinfo->{'status'} eq "C") { # copied print "<div class=\"diff_info\">" . - file_type($diffinfo{'from_mode'}) . ":" . + 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'}) . + hash=>$diffinfo->{'from_id'}, file_name=>$diffinfo->{'from_file'})}, + $diffinfo->{'from_id'}) . " -> " . - file_type($diffinfo{'to_mode'}) . ":" . + 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'}); + 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'}) . ":" . + 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'}) . + hash=>$diffinfo->{'from_id'}, file_name=>$diffinfo->{'file'})}, + $diffinfo->{'from_id'}) . " -> " . - file_type($diffinfo{'to_mode'}) . ":" . + 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'}); + hash=>$diffinfo->{'to_id'}, file_name=>$diffinfo->{'file'})}, + $diffinfo->{'to_id'}); print "</div>\n"; # class="diff_info" }
Signature · 1 line
1.4.1.1
PATCH 12/19 gitweb: Parse two-line from-file/to-file diff header in git_patchset_body
In reply to Jakub NarebskiParse two-line from-file/to-file unified diff header in git_patchset_body directly, instead of leaving pretty-printing to format_diff_line function. Hashes as from-file/to-file are replaced by proper from-file and to-file names (from $diffinfo); in the future we can put hyperlinks there. This makes possible to do blobdiff with only blobs hashes.
The lines in two-line unified diff header have now class "from_file" and "to_file"; the style is chosen to match previous output (classes "rem" and "add" because of '-' and '+' as first character of patch line).
--- gitweb/gitweb.css | 2 ++ gitweb/gitweb.perl | 18 +++++++++++++++++- 2 files changed, 19 insertions(+), 1 deletions(-)
gitweb/gitweb.css +2 -0
@@ -273,10 +273,12 @@ td.mode { font-family: monospace; } +div.diff.to_file, div.diff.add { color: #008800; } +div.diff.from_file, div.diff.rem { color: #cc0000; }
gitweb/gitweb.perl +17 -1
@@ -1706,8 +1706,24 @@ sub git_patchset_body { if ($in_header && $patch_line =~ m/^---/) { - #print "</div>\n" + #print "</div>\n"; # class="diff extended_header" $in_header = 0; + + my $file = $diffinfo->{'from_file'}; + $file ||= $diffinfo->{'file'}; + $patch_line =~ s|a/[0-9a-fA-F]{40}|a/$file|g; + print "<div class=\"diff from_file\">" . esc_html($patch_line) . "</div>\n"; + + $patch_line = <$fd>; + chomp $patch_line; + + #$patch_line =~ m/^+++/; + $file = $diffinfo->{'to_file'}; + $file ||= $diffinfo->{'file'}; + $patch_line =~ s|b/[0-9a-fA-F]{40}|b/$file|g; + print "<div class=\"diff to_file\">" . esc_html($patch_line) . "</div>\n"; + + next LINE; } next LINE if $in_header;
Signature · 1 line
1.4.1.1
PATCH 13/19 gitweb: Add invisible hyperlink to from-file/to-file diff header
In reply to Jakub NarebskiChange replacing hashes as from-file/to-file with filenames from difftree to adding invisible (except underlining on hover/mouseover) hyperlink to from-file/to-file blob. /dev/null as from-file or to-file is not changed (is not hyperlinked).
This makes two-file from-file/to-file unified diff header parsing in git_patchset_body more generic, and not only for legacy blobdiffs.
--- gitweb/gitweb.css | 10 ++++++++++ gitweb/gitweb.perl | 14 ++++++++++---- 2 files changed, 20 insertions(+), 4 deletions(-)
gitweb/gitweb.css +10 -0
@@ -273,11 +273,21 @@ td.mode { font-family: monospace; } +div.diff a.list { + text-decoration: none; +} + +div.diff a.list:hover { + text-decoration: underline; +} + +div.diff.to_file a.list, div.diff.to_file, div.diff.add { color: #008800; } +div.diff.from_file a.list, div.diff.from_file, div.diff.rem { color: #cc0000;
gitweb/gitweb.perl +10 -4
@@ -1711,8 +1711,11 @@ sub git_patchset_body { my $file = $diffinfo->{'from_file'}; $file ||= $diffinfo->{'file'}; - $patch_line =~ s|a/[0-9a-fA-F]{40}|a/$file|g; - print "<div class=\"diff from_file\">" . esc_html($patch_line) . "</div>\n"; + $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;
@@ -1720,8 +1723,11 @@ sub git_patchset_body { #$patch_line =~ m/^+++/; $file = $diffinfo->{'to_file'}; $file ||= $diffinfo->{'file'}; - $patch_line =~ s|b/[0-9a-fA-F]{40}|b/$file|g; - print "<div class=\"diff to_file\">" . esc_html($patch_line) . "</div>\n"; + $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"; next LINE; }
Signature · 1 line
1.4.1.1
Re: [PATCH 13/19] gitweb: Add invisible hyperlink to from-file/to-file diff header
Quoted 5 lines · Jakub Narebski
Change replacing hashes as from-file/to-file with filenames from difftree to adding invisible (except underlining on hover/mouseover) hyperlink to from-file/to-file blob.
/dev/nullas from-file or to-file is not changed (is not hyperlinked).
Wouldn't it be even better to have the hyperlink (or a new, separate one) point to the history for that file, too?
That way, you can go to the commit-diff, and when you see a diff for a file, you can easily just ask for the whole history for that file. As it is, you can get that, but only by going to the "commit" thing, not from the "commit-diff" thing.
Alternatively, maybe commit-diff should have a header with the files it changes (ie it would truly be a superset of the "commit" case)? That might be even nicer, since you'd not have to scroll through a potentially big diff for other files in order to get to the one you care about.
(If you do the "header with changed files", each file could have the same three buttons as in the "commit" view: "blob" (pointing to the blob), "diff" (which just points to _within_ the current window, so that you can get to the start of that particular file diff) and "history" (which obviously does what the "commit" case does too - generate a history page).
Linus
PATCH 14/19 gitweb: Always display link to blobdiff_plain in git_blobdiff
In reply to Jakub Narebski--- gitweb/gitweb.perl | 14 +++++++------- 1 files changed, 7 insertions(+), 7 deletions(-)
gitweb/gitweb.perl +7 -7
@@ -2790,18 +2790,18 @@ sub git_commit { sub git_blobdiff { mkdir($git_temp, 0700); git_header_html(); + my $formats_nav = + $cgi->a({-href => href(action=>"blobdiff_plain", + hash=>$hash, hash_parent=>$hash_parent, + hash_base=>$hash_base, hash_parent_base=>$hash_parent_base, + file_name=>$file_name, file_parent=>$file_parent)}, + "plain"); if (defined $hash_base && (my %co = parse_commit($hash_base))) { - my $formats_nav = - $cgi->a({-href => href(action=>"blobdiff_plain", - hash=>$hash, hash_parent=>$hash_parent, - hash_base=>$hash_base, hash_parent_base=>$hash_parent_base, - file_name=>$file_name, file_parent=>$file_parent)}, - "plain"); git_print_page_nav('','', $hash_base,$co{'tree'},$hash_base, $formats_nav); git_print_header_div('commit', esc_html($co{'title'}), $hash_base); } else { print <<HTML; -<div class="page_nav"><br/><br/></div> +<div class="page_nav"><br/>$formats_nav<br/></div> <div class="title">$hash vs $hash_parent</div> HTML }
Signature · 1 line
1.4.1.1
PATCH 15/19 gitweb: Change here-doc back for style consistency in git_blobdiff
In reply to Jakub Narebski---
Next will be patches which get rid of external diff dependency.
gitweb/gitweb.perl | 6 ++---- 1 files changed, 2 insertions(+), 4 deletions(-)
gitweb/gitweb.perl +2 -4
@@ -2800,10 +2800,8 @@ sub git_blobdiff { git_print_page_nav('','', $hash_base,$co{'tree'},$hash_base, $formats_nav); git_print_header_div('commit', esc_html($co{'title'}), $hash_base); } else { - print <<HTML; -<div class="page_nav"><br/>$formats_nav<br/></div> -<div class="title">$hash vs $hash_parent</div> -HTML + print "<div class=\"page_nav\"><br/>$formats_nav<br/></div>\n"; + print "<div class=\"title\">$hash vs $hash_parent</div>\n"; } git_print_page_path($file_name, "blob", $hash_base); print "<div class=\"page_body\">\n" .
Signature · 1 line
1.4.1.1
PATCH 16/19 gitweb: Use git-diff-tree or git-diff patch output for blobdiff
In reply to Jakub NarebskiThis is second part of removing gitweb dependency on external diff (used in git_diff_print).
Get rid of git_diff_print invocation in git_blobdiff, and use either git-diff-tree (when both hash_base and hash_parent_base are provided) patch format or git-diff patch format (when only hash and hash_parent are provided) for output.
Supported URI schemes, and output formats:
* New URI scheme: both hash_base and hash_parent_base (trees-ish containing blobs versions we want to compare) are provided. Also either filename is provided, or hash (of blob) is provided (we try to find filename then).
For this scheme we have copying and renames detection, mode changes, file types etc., and information extended diff header is correct.
* Old URI scheme: hash_parent_base is not provided, we use hash and hash_parent to directly compare blobs using git-diff. If no filename is given, blobs hashes are used in place of filenames.
This scheme has always "blob" as file type, it cannot detect mode changes, and we rely on CGI parameters to provide name of the file.
Added git_to_hash subroutine, which transforms symbolic name or list of symbolic name to hash or list of hashes using git-rev-parse.
To have "blob" instead of "unknown" (or "file" regardless of the type) in "gitweb diff header" for legacy scheme, file_type function now returns its argument if it is not octal string.
Added support for fake "2" status code in git_patchset_body. Such code is generated by git_blobdiff in legacy scheme case.
ATTENTION: The order of arguments (operands) to git-diff is reversed (sic!) to have correct diff in the legacy (no hash_parent_base) case. $hash_parent, $hash ordering is commented out, as it gives reversed patch (at least for git version 1.4.1.1) as compared to output in new scheme and output of older gitweb version.
---P.S. This is the place to mention that it would be nice to have in gitweb/README specification what minimal version of git is needed for some gitweb features; e.g. "history" relies on having --full-history option to git-rev-list, "blobdiff" (and later "blobdiff_plain") relies on git-diff which accepts blob hashes, and relies on bug in git-diff...
gitweb/gitweb.perl | 149 ++++++++++++++++++++++++++++++++++++++++++++++------ 1 files changed, 132 insertions(+), 17 deletions(-)
gitweb/gitweb.perl +132 -17
@@ -454,7 +454,13 @@ sub mode_str { # convert file mode in octal to file type string sub file_type { - my $mode = oct shift; + my $mode = shift; + + if ($mode !~ m/^[0-7]+$/) { + return $mode; + } else { + $mode = oct $mode; + } if (S_ISDIR($mode & S_IFMT)) { return "directory";
@@ -618,6 +624,26 @@ sub git_get_hash_by_path { return $3; } +# converts symbolic name to hash +sub git_to_hash { + my @params = @_; + return undef unless @params; + + open my $fd, "-|", $GIT, "rev-parse", @params + or return undef; + my @hashes = map { chomp; $_ } <$fd>; + close $fd; + + if (wantarray) { + return @hashes; + } elsif (scalar(@hashes) == 1) { + # single hash + return $hashes[0]; + } else { + return \@hashes; + } +} + ## ...................................................................... ## git utility functions, directly accessing git repository
@@ -1672,7 +1698,8 @@ sub git_patchset_body { "</div>\n"; # class="diff_info" } elsif ($diffinfo->{'status'} eq "R" || # renamed - $diffinfo->{'status'} eq "C") { # copied + $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,
@@ -2788,14 +2815,102 @@ sub git_commit { } sub git_blobdiff { - mkdir($git_temp, 0700); - git_header_html(); + my $fd; + my @difftree; + my %diffinfo; + + # preparing $fd and %diffinfo for git_patchset_body + # new style URI + if (defined $hash_base && defined $hash_parent_base) { + if (defined $file_name) { + # read raw output + open $fd, "-|", $GIT, "diff-tree", '-r', '-M', '-C', $hash_parent_base, $hash_base, + "--", $file_name + or die_error(undef, "Open git-diff-tree failed"); + @difftree = map { chomp; $_ } <$fd>; + close $fd + or die_error(undef, "Reading git-diff-tree failed"); + @difftree + or die_error('404 Not Found', "Blob diff not found"); + + } elsif (defined $hash) { # try to find filename from $hash + if ($hash !~ /[0-9a-fA-F]{40}/) { + $hash = git_to_hash($hash); + } + + # read filtered raw output + open $fd, "-|", $GIT, "diff-tree", '-r', '-M', '-C', $hash_parent_base, $hash_base + or die_error(undef, "Open git-diff-tree failed"); + @difftree = + # ':100644 100644 03b21826... 3b93d5e7... M ls-files.c' + # $hash == to_id + grep { /^:[0-7]{6} [0-7]{6} [0-9a-fA-F]{40} $hash/ } + map { chomp; $_ } <$fd>; + close $fd + or die_error(undef, "Reading git-diff-tree failed"); + @difftree + or die_error('404 Not Found', "Blob diff not found"); + + } else { + die_error('404 Not Found', "Missing one of the blob diff parameters"); + } + + if (@difftree > 1) { + die_error('404 Not Found', "Ambiguous blob diff specification"); + } + + %diffinfo = parse_difftree_raw_line($difftree[0]); + $file_parent ||= $diffinfo{'from_file'} || $file_name || $diffinfo{'file'}; + $file_name ||= $diffinfo{'to_file'} || $diffinfo{'file'}; + + $hash_parent ||= $diffinfo{'from_id'}; + $hash ||= $diffinfo{'to_id'}; + + # open patch output + open $fd, "-|", $GIT, "diff-tree", '-r', '-p', '-M', '-C', $hash_parent_base, $hash_base, + "--", $file_name + or die_error(undef, "Open git-diff-tree failed"); + } + + # old/legacy style URI + if (!%diffinfo && # if new style URI failed + defined $hash && defined $hash_parent) { + # fake git-diff-tree raw output + $diffinfo{'from_mode'} = $diffinfo{'to_mode'} = "blob"; + $diffinfo{'from_id'} = $hash_parent; + $diffinfo{'to_id'} = $hash; + if (defined $file_name) { + if (defined $file_parent) { + $diffinfo{'status'} = '2'; + $diffinfo{'from_file'} = $file_parent; + $diffinfo{'to_file'} = $file_name; + } else { # assume not renamed + $diffinfo{'status'} = '1'; + $diffinfo{'from_file'} = $file_name; + $diffinfo{'to_file'} = $file_name; + } + } else { # no filename given + $diffinfo{'status'} = '2'; + $diffinfo{'from_file'} = $hash_parent; + $diffinfo{'to_file'} = $hash; + } + + #open $fd, "-|", $GIT, "diff", '-p', $hash_parent, $hash + open $fd, "-|", $GIT, "diff", '-p', $hash, $hash_parent + or die_error(undef, "Open git-diff failed"); + } else { + die_error('404 Not Found', "Missing one of the blob diff parameters") + unless %diffinfo; + } + + # header my $formats_nav = $cgi->a({-href => href(action=>"blobdiff_plain", hash=>$hash, hash_parent=>$hash_parent, hash_base=>$hash_base, hash_parent_base=>$hash_parent_base, file_name=>$file_name, file_parent=>$file_parent)}, "plain"); + git_header_html(); if (defined $hash_base && (my %co = parse_commit($hash_base))) { git_print_page_nav('','', $hash_base,$co{'tree'},$hash_base, $formats_nav); git_print_header_div('commit', esc_html($co{'title'}), $hash_base);
@@ -2803,19 +2918,19 @@ sub git_blobdiff { print "<div class=\"page_nav\"><br/>$formats_nav<br/></div>\n"; print "<div class=\"title\">$hash vs $hash_parent</div>\n"; } - git_print_page_path($file_name, "blob", $hash_base); - print "<div class=\"page_body\">\n" . - "<div class=\"diff_info\">blob:" . - $cgi->a({-href => href(action=>"blob", hash=>$hash_parent, - hash_base=>$hash_parent_base, file_name=>($file_parent || $file_name))}, - $hash_parent) . - " -> blob:" . - $cgi->a({-href => href(action=>"blob", hash=>$hash, - hash_base=>$hash_base, file_name=>$file_name)}, - $hash) . - "</div>\n"; - git_diff_print($hash_parent, $file_name || $hash_parent, $hash, $file_name || $hash); - print "</div>"; # page_body + if (defined $file_name) { + git_print_page_path($file_name, "blob", $hash_base); + } else { + print "<div class=\"page_path\"></div>\n"; + } + + # patch + print "<div class=\"page_body\">\n"; + + git_patchset_body($fd, [ \%diffinfo ], $hash_base, $hash_parent_base); + close $fd; + + print "</div>\n"; # class="page_body" git_footer_html(); }
Signature · 1 line
1.4.1.1
PATCH 1/1 gitweb: Remove workaround for git-diff bug fixed in f82cd3c
Remove workaround in git_blobdiff for error in git-diff (showing reversed diff for diff of blobs), corrected in commit
f82cd3c Fix "git diff blob1 blob2" showing the diff in reverse.
which is post 1.4.2-rc2 commit.
--- gitweb/gitweb.perl | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-)
gitweb/gitweb.perl +1 -2
@@ -2841,8 +2841,7 @@ sub git_blobdiff { } # open patch output - #open $fd, "-|", $GIT, "diff", '-p', $hash_parent, $hash - open $fd, "-|", $GIT, "diff", '-p', $hash, $hash_parent + open $fd, "-|", $GIT, "diff", '-p', $hash_parent, $hash or die_error(undef, "Open git-diff failed"); } else { die_error('404 Not Found', "Missing one of the blob diff parameters")
Signature · 1 line
1.4.1.1
Re: [PATCH 16/19] gitweb: Use git-diff-tree or git-diff patch output for blobdiff
In reply to Jakub NarebskiQuoted 5 lines · Jakub Narebski
ATTENTION: The order of arguments (operands) to git-diff is reversed (sic!) to have correct diff in the legacy (no
hash_parent_base) case. $hash_parent, $hash ordering is commented out, as it gives reversed patch (at least for git version 1.4.1.1) as compared to output in new scheme and output of older gitweb version.
By the way, wa it corrected later? git version 1.4.1.1
1010:jnareb@roke:~/git> git diff-tree 599f8d63140^ 599f8d63140
:100644 100644 0bd517b2649af37d9980f85e784f9a00c3263922 8213ce240232a1dc8a0a498972323a33e8fcb7a0 M builtin-grep.c
1011:jnareb@roke:~/git> git diff-tree -p 599f8d63140^ 599f8d63140
builtin-grep.c +0 -3
@@ -293,9 +293,6 @@ static void compile_patterns(struct grep*/ p = opt->pattern_list; opt->pattern_expression =compile_pattern_expr (&p); -#if DEBUG -dump_pattern_exp (opt->pattern_expression,0 ); -#endif if (p)die ("incomplete pattern expression: %s" , p->pattern); }
1012:jnareb@roke:~/git> git diff 0bd517b2649af37d9980f85e784f9a00c3263922 8213ce240232a1dc8a0a498972323a33e8fcb7a0
0bd517b2649af37d9980f85e784f9a00c3263922 +3 -0
@@ -293,6 +293,9 @@ static void compile_patterns(struct grep */ p = opt->pattern_list; opt->pattern_expression = compile_pattern_expr(&p); +#if DEBUG + dump_pattern_exp(opt->pattern_expression, 0); +#endif if (p) die("incomplete pattern expression: %s", p->pattern); }
Signature · 3 lines
Jakub Narebski Warsaw, Poland ShadeHawk on #git
Re: [PATCH 16/19] gitweb: Use git-diff-tree or git-diff patch output for blobdiff
Quoted 9 lines · Jakub Narebski
Quoted 5 lines · Jakub Narebski
ATTENTION: The order of arguments (operands) to git-diff is reversed (sic!) to have correct diff in the legacy (no
hash_parent_base) case. $hash_parent, $hash ordering is commented out, as it gives reversed patch (at least for git version 1.4.1.1) as compared to output in new scheme and output of older gitweb version.By the way, wa it corrected later? git version 1.4.1.1
I think you were involved in the thread that resulted in the fix...
53dd8a9 Show both blob names from "git diff blob1 blob2" f82cd3c Fix "git diff blob1 blob2" showing the diff in reverse.
Re: [PATCH 16/19] gitweb: Use git-diff-tree or git-diff patch output for blobdiff
Quoted 10 lines · Junio C Hamano
Quoted 8 lines · Jakub Narebski
Quoted 5 lines · Jakub Narebski
ATTENTION: The order of arguments (operands) to git-diff is reversed (sic!) to have correct diff in the legacy (no
hash_parent_base) case. $hash_parent, $hash ordering is commented out, as it gives reversed patch (at least for git version 1.4.1.1) as compared to output in new scheme and output of older gitweb version.
Quoted 7 lines
Quoted 1 line
By the way, wa it corrected later? git version 1.4.1.1
I think you were involved in the thread that resulted in the fix...
53dd8a9 Show both blob names from "git diff blob1 blob2" f82cd3c Fix "git diff blob1 blob2" showing the diff in reverse.
Gaah. So I'd have to fix the gitweb, i.e. remove workaround (and update git to 1.4.2).
Signature · 3 lines
Jakub Narebski Warsaw, Poland ShadeHawk on #git
PATCH 17/19 gitweb: git_blobdiff_plain is git_blobdiff('plain')
In reply to Jakub Narebskigit_blobdiff and git_blobdiff_plain are now collapsed into one subroutine git_blobdiff, with format (currently 'html' which is default format corresponding to git_blobdiff, and 'plain' corresponding to git_blobdiff_plain) specified in argument.
blobdiff_plain format is now generated either by git-diff-tree or by git-diff. Added X-Git-Url: header. From-file and to-file name in header are corrected.
Note that for now commitdiff_plain does not detect renames and copying, while blobdiff_plain does.
While at it, set expires to "+1d" for non-textual hash ids.
---
gitweb/gitweb.perl | 95 ++++++++++++++++++++++++++++++++++++++-------------- 1 files changed, 69 insertions(+), 26 deletions(-)
gitweb/gitweb.perl +68 -25
@@ -2815,9 +2815,12 @@ sub git_commit { } sub git_blobdiff { + my $format = shift || 'html'; + my $fd; my @difftree; my %diffinfo; + my $expires; # preparing $fd and %diffinfo for git_patchset_body # new style URI
@@ -2866,6 +2869,12 @@ sub git_blobdiff { $hash_parent ||= $diffinfo{'from_id'}; $hash ||= $diffinfo{'to_id'}; + # non-textual hash id's can be cached + if ($hash_base =~ m/^[0-9a-fA-F]{40}$/ && + $hash_parent_base =~ m/^[0-9a-fA-F]{40}$/) { + $expires = '+1d'; + } + # open patch output open $fd, "-|", $GIT, "diff-tree", '-r', '-p', '-M', '-C', $hash_parent_base, $hash_base, "--", $file_name
@@ -2894,7 +2903,14 @@ sub git_blobdiff { $diffinfo{'from_file'} = $hash_parent; $diffinfo{'to_file'} = $hash; } + # non-textual hash id's can be cached + if ($hash =~ m/^[0-9a-fA-F]{40}$/ && + $hash_parent =~ m/^[0-9a-fA-F]{40}$/) { + $expires = '+1d'; + } + + # open patch output #open $fd, "-|", $GIT, "diff", '-p', $hash_parent, $hash open $fd, "-|", $GIT, "diff", '-p', $hash, $hash_parent or die_error(undef, "Open git-diff failed");
@@ -2904,40 +2920,67 @@ sub git_blobdiff { } # header - my $formats_nav = - $cgi->a({-href => href(action=>"blobdiff_plain", - hash=>$hash, hash_parent=>$hash_parent, - hash_base=>$hash_base, hash_parent_base=>$hash_parent_base, - file_name=>$file_name, file_parent=>$file_parent)}, - "plain"); - git_header_html(); - if (defined $hash_base && (my %co = parse_commit($hash_base))) { - git_print_page_nav('','', $hash_base,$co{'tree'},$hash_base, $formats_nav); - git_print_header_div('commit', esc_html($co{'title'}), $hash_base); - } else { - print "<div class=\"page_nav\"><br/>$formats_nav<br/></div>\n"; - print "<div class=\"title\">$hash vs $hash_parent</div>\n"; - } - if (defined $file_name) { - git_print_page_path($file_name, "blob", $hash_base); + if ($format eq 'html') { + my $formats_nav = + $cgi->a({-href => href(action=>"blobdiff_plain", + hash=>$hash, hash_parent=>$hash_parent, + hash_base=>$hash_base, hash_parent_base=>$hash_parent_base, + file_name=>$file_name, file_parent=>$file_parent)}, + "plain"); + git_header_html(undef, $expires); + if (defined $hash_base && (my %co = parse_commit($hash_base))) { + git_print_page_nav('','', $hash_base,$co{'tree'},$hash_base, $formats_nav); + git_print_header_div('commit', esc_html($co{'title'}), $hash_base); + } else { + print "<div class=\"page_nav\"><br/>$formats_nav<br/></div>\n"; + print "<div class=\"title\">$hash vs $hash_parent</div>\n"; + } + if (defined $file_name) { + git_print_page_path($file_name, "blob", $hash_base); + } else { + print "<div class=\"page_path\"></div>\n"; + } + + } elsif ($format eq 'plain') { + print $cgi->header( + -type => 'text/plain', + -charset => 'utf-8', + -expires => $expires, + -content_disposition => qq(inline; filename="${file_name}.patch")); + + print "X-Git-Url: " . $cgi->self_url() . "\n\n"; + } else { - print "<div class=\"page_path\"></div>\n"; + die_error(undef, "Unknown blobdiff format"); } # patch - print "<div class=\"page_body\">\n"; + if ($format eq 'html') { + print "<div class=\"page_body\">\n"; - git_patchset_body($fd, [ \%diffinfo ], $hash_base, $hash_parent_base); - close $fd; + git_patchset_body($fd, [ \%diffinfo ], $hash_base, $hash_parent_base); + close $fd; - print "</div>\n"; # class="page_body" - git_footer_html(); + print "</div>\n"; # class="page_body" + git_footer_html(); + + } else { + while (my $line = <$fd>) { + $line =~ s!a/($hash|$hash_parent)!a/$diffinfo{'from_file'}!g; + $line =~ s!b/($hash|$hash_parent)!b/$diffinfo{'to_file'}!g; + + print $line; + + last if $line =~ m!^\+\+\+!; + } + local $/ = undef; + print <$fd>; + close $fd; + } } sub git_blobdiff_plain { - mkdir($git_temp, 0700); - print $cgi->header(-type => "text/plain", -charset => 'utf-8'); - git_diff_print($hash_parent, $file_name || $hash_parent, $hash, $file_name || $hash, "plain"); + git_blobdiff('plain'); } sub git_commitdiff {
Signature · 1 line
1.4.1.1
PATCH 18/19 gitweb: Remove git_diff_print subroutine
In reply to Jakub NarebskiRemove git_diff_print subroutine, used to print diff in previous versions of "diff" actions, namely git_commitdiff, git_commitdiff_plain, git_blobdiff, git_blobdiff_plain.
--- gitweb/gitweb.perl | 71 ---------------------------------------------------- 1 files changed, 0 insertions(+), 71 deletions(-)
gitweb/gitweb.perl +0 -71
@@ -1975,77 +1975,6 @@ sub git_heads_body { print "</table>\n"; } -## ---------------------------------------------------------------------- -## functions printing large fragments, format as one of arguments - -sub git_diff_print { - my $from = shift; - my $from_name = shift; - my $to = shift; - my $to_name = shift; - my $format = shift || "html"; - - my $from_tmp = "/dev/null"; - my $to_tmp = "/dev/null"; - my $pid = $$; - - # create tmp from-file - if (defined $from) { - $from_tmp = "$git_temp/gitweb_" . $$ . "_from"; - open my $fd2, "> $from_tmp"; - open my $fd, "-|", $GIT, "cat-file", "blob", $from; - my @file = <$fd>; - print $fd2 @file; - close $fd2; - close $fd; - } - - # create tmp to-file - if (defined $to) { - $to_tmp = "$git_temp/gitweb_" . $$ . "_to"; - open my $fd2, "> $to_tmp"; - open my $fd, "-|", $GIT, "cat-file", "blob", $to; - my @file = <$fd>; - print $fd2 @file; - close $fd2; - close $fd; - } - - open my $fd, "-|", "/usr/bin/diff -u -p -L \'$from_name\' -L \'$to_name\' $from_tmp $to_tmp"; - if ($format eq "plain") { - undef $/; - print <$fd>; - $/ = "\n"; - } else { - while (my $line = <$fd>) { - chomp $line; - my $char = substr($line, 0, 1); - my $diff_class = ""; - if ($char eq '+') { - $diff_class = " add"; - } elsif ($char eq "-") { - $diff_class = " rem"; - } elsif ($char eq "@") { - $diff_class = " chunk_header"; - } elsif ($char eq "\\") { - # skip errors - next; - } - $line = untabify($line); - print "<div class=\"diff$diff_class\">" . esc_html($line) . "</div>\n"; - } - } - close $fd; - - if (defined $from) { - unlink($from_tmp); - } - if (defined $to) { - unlink($to_tmp); - } -} - - ## ====================================================================== ## ====================================================================== ## actions
Signature · 1 line
1.4.1.1
PATCH 19/19 gitweb: Remove creating directory for temporary files
In reply to Jakub NarebskiRemove $git_temp variable which held location for temporary files needed by git_diff_print, and removed creating $git_temp directory.
---
This is last patch in this (admittedly quite large) series, removing dependency on external diff, and the need for temporary files, from gitweb.
You can view new gitweb in work at
http://front.fuw.edu.pl/cgi-bin/jnareb/gitweb.cgi
Changes are in branch "gitweb/web" in repository available at
http://front.fuw.edu.pl/jnareb/scm/git.git/
Comments appreciated.
gitweb/gitweb.perl | 6 ------ 1 files changed, 0 insertions(+), 6 deletions(-)
gitweb/gitweb.perl +0 -6
@@ -31,9 +31,6 @@ # absolute fs-path which will be prepend #our $projectroot = "/pub/scm"; our $projectroot = "++GITWEB_PROJECTROOT++"; -# location for temporary files needed for diffs -our $git_temp = "/tmp/gitweb"; - # target of the home link on top of all pages our $home_link = $my_uri || "/";
@@ -144,9 +141,6 @@ # version of the core git binary our $git_version = qx($GIT --version) =~ m/git version (.*)$/ ? $1 : "unknown"; $projects_list ||= $projectroot; -if (! -d $git_temp) { - mkdir($git_temp, 0700) || die_error(undef, "Couldn't mkdir $git_temp"); -} # ====================================================================== # input validation and dispatch
Signature · 1 line
1.4.1.1
Re: [PATCH 19/19] gitweb: Remove creating directory for temporary files
Quoted 2 lines · you
You can view new gitweb in work at
http://front.fuw.edu.pl/cgi-bin/jnareb/gitweb.cgi
Hmm... history pages seem not to work.
Otherwise, looks fine.
Josef
Re: [PATCH 19/19] gitweb: Remove creating directory for temporary files
Quoted 6 lines · Josef Weidendorfer
Quoted 2 lines · you
You can view new gitweb in work at
http://front.fuw.edu.pl/cgi-bin/jnareb/gitweb.cgiHmm... history pages seem not to work.
Otherwise, looks fine.
Probably git core on this machine (version 1.3.0) is too old and doesn't support --full-history option to git-rev-list, needed by git_history.
Relevant changes were made to commitdiff, comitdiff_plain, blobdiff and blobdiff_plain views.
Signature · 3 lines
Jakub Narebski Warsaw, Poland ShadeHawk on #git
Re: [PATCH 19/19] gitweb: Remove creating directory for temporary files
In reply to Jakub NarebskiQuoted 2 lines · Jakub Narebski
Remove $git_temp variable which held location for temporary files needed by
git_diff_print, and removed creating $git_temp directory.
Very good. Not writing into the filesystem even in the temporary location is a very good thing.
Other things I noticed in this 19 series (note that I've applied them more or less intact already, expecting that any issues will be fixed in-tree):
* Overall
Your MUA configuration seems to have improved and stabilized quite a bit. I did not see any corruption this time around.
* 01/19 gitweb: Use git-diff-tree patch output for commitdiff
This was a bit large for me to review during work-day, and seemed a bit scary. It initially did not give me a good feeling to see that a large series that followed depended on something that was marked RFC, but it turned out mostly Ok.
* 02/19 gitweb: Replace git_commitdiff_plain by anonymous subroutine 03/19 gitweb: Show information about incomplete lines in commitdiff Revert "gitweb: Replace git_commitdiff_plain by anonymous subroutine"
I've commented on these.
* 06/19 gitweb: Add git_get_{following,preceding}_references functions 07/19 gitweb: Return on first ref found when git_get_preceding_references is called in scalar context
This looks *VERY* expensive. Does git_get_references() cache and reuse its result? How many times during a single invocation are these subs called?
Also I am not sure about the correctness of "get-following".
B------D------F
/ base
--A------C------E
hashYou read from "rev-list $base", stop when you see $hash, and grab all the refs that point at the rev you have seen before stopping as "following". But in the above picture, you will follow from F down to the very initial commit without stopping and there actually is _no_ rev that follows E so your result would contain B D A (if they are tagged) but none of them follows E. There is something wrong here.
At least you should read from "rev-list $hash..$base"; then traversal would go F D B and stop at A; you probably would want --boundary to force showing of A as well, but even then I am not sure how well the result would work.
"get-preceding" also wants to go down to the initial commit.
"get-following" is inherently a very expensive operation, so I would suggest not doing this. It seems that nobody uses these two subs yet, so probably it is better to yank them before they cause damages.
* 08/19 gitweb: Add git_get_rev_name_tags function 09/19 gitweb: Use git_get_name_rev_tags for commitdiff_plain
X-Git-Tag: header
I suspect these make the generation of the header extremely expensive. I'd suggest reverting them to the original.
* 13/19 gitweb: Add invisible hyperlink to from-file/to-file diff header
You seem to have forgotten esc_html() on the patch-line before sending it to the browser. Careful.
* 14/19 gitweb: Always display link to blobdiff_plain in git_blobdiff
Need justification why this change is needed (or why previous logic to avoid showing it in certain cases is wrong).
* 16/19 gitweb: Use git-diff-tree or git-diff patch output for blobdiff
Is git_to_hash sub always called with object names and nothing else? "git rev-parse no-such" would die with an error message, and "git rev-parse Makefile" in populated working tree would say "Makefile" without complaints. Perhaps you want --revs-only --no-flags here.
I think it is a bad style to return [] or $ in scalar context depending on the number of results. It forces the caller to do a conditional depending on the type of the stuff returned.
I would suggest just removing if (wantarray) and always return @hashes. A caller who is interested in a single element can say "($it) = your_sub(...)", a caller who wants the number of elements can say "$cnt = your_sub(...)", and a caller who wants to know all can say "(@them) = your_sub(...)".
I think that is the usual thing to do in Perl. Unless there is a compelling reason that is so important that it is worth to deviate from that norm and confusing the programmer, that is.
I think "# try to find filename from $hash" part would misbehave if $hash returned by git_to_hash($hash) becomes undef.
You seem to spell out '-M', '-C' everywhere. I suspect fixing them all to just '-C' (or perhaps '-B', '-C') would be tedious but probably is a good idea.
* 17/19 gitweb: git_blobdiff_plain is git_blobdiff('plain')
Needs justification why commitdiff and blobdiff plain needs to behave differently.
Re: [PATCH 19/19] gitweb: Remove creating directory for temporary files
Quoted 4 lines · Junio C Hamano
"get-following" is inherently a very expensive operation, so I would suggest not doing this. It seems that nobody uses these two subs yet, so probably it is better to yank them before they cause damages.
A bit of clarification. gitk has preceding/following but unlike gitweb it has three things that go in favor of having it.
- gitk can afford to use as much CPU as the user throw at it, since it runs locally.
- gitk finds preceding/following in the background so the user does not have to wait, and it is done while it gets the list of commits which it needs to do anyway.
- what gitk reads from rev-list persists while the user keeps it around. when the user walks around inspecting different commits, the cost for computing preceding/following is amortized. gitweb cannot do this unless it somehow caches this information, but you just spent significant effort to make it unnecessary for gitweb to write anything on the filesystem, so introducing caching is somewhat going backwards.
Re: [PATCH 19/19] gitweb: Remove creating directory for temporary files
Quoted 10 lines · Junio C Hamano
Quoted 4 lines · Junio C Hamano
"get-following" is inherently a very expensive operation, so I would suggest not doing this. It seems that nobody uses these two subs yet, so probably it is better to yank them before they cause damages.
A bit of clarification. gitk has preceding/following but unlike gitweb it has three things that go in favor of having it.
Quoted 6 lines
- gitk can afford to use as much CPU as the user throw at it, since it runs locally.
- gitk finds preceding/following in the background so the user does not have to wait, and it is done while it gets the list of commits which it needs to do anyway.
I wonder if we could AJAXize gitweb, and have _browser_ compute preceding/following tags using some JavaScript...
That would be client side, could be in background, and could be cached. :-)
Signature · 2 lines
Jakub Narebski ShadeHawk on #git
Re: [PATCH 19/19] gitweb: Remove creating directory for temporary files
In reply to Junio C HamanoQuoted 12 lines · Junio C Hamano
Quoted 2 lines · Jakub Narebski
Remove $git_temp variable which held location for temporary files needed by
git_diff_print, and removed creating $git_temp directory.Very good. Not writing into the filesystem even in the temporary location is a very good thing.
Other things I noticed in this 19 series (note that I've applied them more or less intact already, expecting that any issues will be fixed in-tree):
[...]
Quoted 29 lines
* 06/19 gitweb: Add git_get_{following,preceding}_references functions 07/19 gitweb: Return on first ref found when
git_get_preceding_referencesis called in scalar contextThis looks *VERY* expensive. Does
git_get_references()cache and reuse its result? How many times during a single invocation are these subs called?Also I am not sure about the correctness of "get-following".
B------D------F / base --A------C------E hashYou read from "rev-list $base", stop when you see $hash, and grab all the refs that point at the rev you have seen before stopping as "following". But in the above picture, you will follow from F down to the very initial commit without stopping and there actually is _no_ rev that follows E so your result would contain B D A (if they are tagged) but none of them follows E. There is something wrong here.
At least you should read from "rev-list $hash..$base"; then traversal would go F D B and stop at A; you probably would want --boundary to force showing of A as well, but even then I am not sure how well the result would work.
Quoted 6 lines
"get-preceding" also wants to go down to the initial commit.
"get-following" is inherently a very expensive operation, so I would suggest not doing this. It seems that nobody uses these two subs yet, so probably it is better to yank them before they cause damages.
I agree.
Quoted 6 lines
* 08/19 gitweb: Add
git_get_rev_name_tagsfunction 09/19 gitweb: Usegit_get_name_rev_tagsforcommitdiff_plain
X-Git-Tag: headerI suspect these make the generation of the header extremely expensive. I'd suggest reverting them to the original.
The git_get_following_references is copied almost verbatim from the original (i.e. before this series) git_commitdiff_plain implementation, modified only to allow for changed output of git_get_references (formerly read_info_ref), and with "HEAD" changed to $hash_base || "HEAD".
git_get_preceding_references was made to be companion to git_get_following_references, so of course it shares it's warts, errors and disadvantages.
First patch in series changed X-Git-Tag: header to show only the tag that points _directly_ to 'hash' commit, similarly to the ref marker in HTML output (in git_commitdiff for example).
There was mentioned in discussion that it is nice to know what version you need to have feature introduced by commitdiff. Hence writing code generating X-Git-Tag: header into subroutine, writing companion subroutine... then deciding that as there is native git command for that, namely 'git name-rev --tags', why not use it? The git_get_following_refs and git_get_preceding_refs didn't get used.
git name-rev is quite fast, perhaps up to half a second, or a second. And is called only once per commitdiff_plain
I guess that generating (properly!) X-Git-Follows:, X-Git-Branch: and
X-Git-Precedes: (similarly to what gitk and qgit do) should be made into feature, as it is time and resource consuming.
Quoted 4 lines
* 13/19 gitweb: Add invisible hyperlink to from-file/to-file diff header
You seem to have forgotten
esc_html()on the patch-line before sending it to the browser. Careful.
Cannot esc_html() line with HTML code, namely the hyperlink. I know that the line is "+++ a/<filename>" or "--- b/<filename>", and we replace <filename> with hyperlink, which has esc_html($filename) as contents. There is no need to escape "+++ a/" or "--- b/".
I guess I rely on git-diff/git-diff-tree to have "+++ /dev/null" and "--- /dev/null" (both HTML safe), if there is no "a/" and "b/" in from-file/to-file unified diff header.
Quoted 4 lines
* 14/19 gitweb: Always display link to
blobdiff_plainingit_blobdiffNeed justification why this change is needed (or why previous logic to avoid showing it in certain cases is wrong).
Why we didn't display it before? I though it was a bug (oversimplification in case we don't have $hash_base or it is not a commit). If we can display "blob" view, we can display "blob_plain" view...
Quoted 18 lines
* 16/19 gitweb: Use git-diff-tree or git-diff patch output for blobdiff
Is
git_to_hashsub always called with object names and nothing else? "git rev-parse no-such" would die with an error message, and "git rev-parseMakefile" in populated working tree would say "Makefile" without complaints. Perhaps you want --revs-only --no-flags here.I think it is a bad style to return [] or $ in scalar context depending on the number of results. It forces the caller to do a conditional depending on the type of the stuff returned.
I would suggest just removing if (wantarray) and always return @hashes. A caller who is interested in a single element can say "($it) =
your_sub(...)", a caller who wants the number of elements can say "$cnt =your_sub(...)", and a caller who wants to know all can say "(@them) =your_sub(...)".
Quoted 8 lines
I think that is the usual thing to do in Perl. Unless there is a compelling reason that is so important that it is worth to deviate from that norm and confusing the programmer, that is.
I think "# try to find filename from $hash" part would misbehave if $hash returned by
git_to_hash($hash)becomes undef.
Gaaah, perhaps we should stop to try to be too nice, and just barf or use legacy output if user didn't provide filename, and hash is not sha1 of blob (don't try too hard to find filename if it is not provided).
And remove git_to_hash function until the correct implementation is written.
Quoted 3 lines
You seem to spell out '-M', '-C' everywhere. I suspect fixing them all to just '-C' (or perhaps '-B', '-C') would be tedious but probably is a good idea.
Does '-C' imply '-M'?
Quoted 4 lines
* 17/19 gitweb:
git_blobdiff_plainisgit_blobdiff('plain')Needs justification why commitdiff and blobdiff plain needs to behave differently.
First, if we have blobdiff generated with renames/copying detection (and "commit" view uses it, and provides link to blobdiffs using it), there not always is single diff (blobdiff) without renames/copying detection. So to have blobdiff and blobdiff_plain equivalent, and blobdiff_plain without renames detection, then blobdiff_plain view would have sometimes _two_ patches.
This isn't the case with commitdiff. The diff with and without renames detection are equivalent for diff of whole commit.
Originally comittdiff and comitdiff_plain both were without renames detection, while blobdiff and blobdiff_plain were called from commit with renames detection, and from history with no need for renames detection.
The idea behind changing comittdiff (HTML version) to including rename detection was that it gives shorter and better to understand patches. The idea (perhaps wrong) behind leaving comitdiff_plain output without renames detection was that this output can be applied directly by non-git-aware tools. It can be easily changed to include renames/copying detection (put '-C' in one place).
The idea behind having both blobdiff and blobdiff_plain have renames detection was that commit view used rename detection, the two views should be equivalent and one exist always for the other, and that it was easier on implementation ;-)
P.S. I have most problems with having legacy blobdiff URL (without 'hpb' i.e. hash_parent_base parameter) working correctly without making use of external diff.
Signature · 3 lines
Jakub Narebski Warsaw, Poland ShadeHawk on #git
Re: [PATCH 19/19] gitweb: Remove creating directory for temporary files
Quoted 10 lines · Jakub Narebski
Quoted 6 lines · Junio C Hamano
Jakub Narebski <[email protected]> writes:
* 13/19 gitweb: Add invisible hyperlink to from-file/to-file diff header
You seem to have forgotten
esc_html()on the patch-line before sending it to the browser. Careful.Cannot
esc_html()line with HTML code, namely the hyperlink.
OK, I did not notice the lines are already HTMLified with links at that point.
Quoted 8 lines
Quoted 4 lines
* 14/19 gitweb: Always display link to
blobdiff_plainingit_blobdiffNeed justification why this change is needed (or why previous logic to avoid showing it in certain cases is wrong).
Why we didn't display it before? I though it was a bug (oversimplification in case we don't have $hash_base or it is not a commit). If we can display "blob" view, we can display "
blob_plain" view...
I take it that you mean that it avoids the case without $hash_base even though it can do all the necessary computation without it. If so please say that in the commit log message.
Quoted 5 lines
Quoted 3 lines
You seem to spell out '-M', '-C' everywhere. I suspect fixing them all to just '-C' (or perhaps '-B', '-C') would be tedious but probably is a good idea.
Does '-C' imply '-M'?
They are, in this order "-M" < "-C" < "--find-copies-harder -C", strict superset/subset of each other.
Having said that, I think just -M (or perhaps -B -M) might be a better default (or "only choice" if the UI does not give a choice), for a few reasons:
* -C tends to be far more expensive than -M. The cost of -M is proportional to (number of removed files) * (number of new files). The cost of -C is proportional to (number of changed files + number of removed files) * (number of new files). For -C with --find-copies-harder, the cost is still more expensive: (number of files in the original tree) * (number of new files).
* -C does not detect all copies. It considers only the pre-image of files changed in the same commit as candidates of copy source. To make it consider _any_ file that was in the original tree, you would need to give --find-copies-harder as well. In a way, -C is a cheaper (but still more expensive) compromise, middle ground between -M and -C --find-copies-harder.
Quoted 11 lines
Quoted 4 lines
* 17/19 gitweb:
git_blobdiff_plainisgit_blobdiff('plain')Needs justification why commitdiff and blobdiff plain needs to behave differently.
First, if we have blobdiff generated with renames/copying detection (and "commit" view uses it, and provides link to blobdiffs using it), there not always is single diff (blobdiff) without renames/copying detection. So to have blobdiff and
blobdiff_plainequivalent, andblobdiff_plainwithout renames detection, thenblobdiff_plainview would have sometimes _two_ patches.
Sorry, does not parse, but two paragraphs below I think you made it clear why.
Quoted 11 lines
The idea behind changing comittdiff (HTML version) to including rename detection was that it gives shorter and better to understand patches. The idea (perhaps wrong) behind leaving
comitdiff_plainoutput without renames detection was that this output can be applied directly by non-git-aware tools. It can be easily changed to include renames/copying detection (put '-C' in one place).The idea behind having both blobdiff and
blobdiff_plainhave renames detection was that commit view used rename detection, the two views should be equivalent and one exist always for the other, and that it was easier on implementation ;-)
I am not sure about this. People, especially the ones who do _not_ use git and are interested in one single fix, are the ones you are trying to help, because they can just be told about the change (e.g. "'Fix bar blah' patch in Linus tree last week might fix your problems"), go to gitweb and use it. But:
(0) If rename/copy is not involved there is no difference so I
will not discuss that case further;
(1) If rename/copy is involved, as you say, the patch is far easier to understand in git form; the person who downloaded the patch would have a hard time understanding it before applying the patch if you do not do -M/-C.
(2) The post-image file would not exist in the tree the person who downloaded the patch has for renamed/copied paths, so failure from "patch -p1" is immediately noticeable. The metainformation says what was renamed/copied where, and I do not think it is too much to expect for them to first rename/copy by hand then re-run "patch -p1".
(3) If we do not do -M, in situations where the patch has fuzz or conflict when applied to the tree the person who downloaded has, the post-image patch will be applied cleanly and the removal patch will have conflict or fuzz. The local change can easily be lost that way.
Quoted 3 lines
P.S. I have most problems with having legacy blobdiff URL (without 'hpb' i.e.hash_parent_baseparameter) working correctly without making use of external diff.
Are people bookmarking such URLs? How important is the legacy compatibility?
Re: [PATCH 19/19] gitweb: Remove creating directory for temporary files
In reply to Jakub NarebskiQuoted 4 lines
You can view new gitweb in work at
http://front.fuw.edu.pl/cgi-bin/jnareb/gitweb.cgi
Very nice job!
Just a couple of suggestion regarding blame.
- Instead of commit sha perhaps is more useful to show author name (linked to commit) and progressive number of revision.
- Original code lines, ie. imported at the beginning and never modified, perhaps it is better to view without commit number, this could obfuscate the view and in any case is not an accurate info because the line was not modified during initial patch. [Note] Also in qgit there was the annotation of the first initial commit in case of unmodified code lines, then Ingo Molnar suggest me to remove that line and leave it blank for the reasons I reported above. I think he was right.
Marco
Re: [PATCH 19/19] gitweb: Remove creating directory for temporary files
Quoted 9 lines · Marco Costalba
Quoted 2 lines
You can view new gitweb in work at
http://front.fuw.edu.pl/cgi-bin/jnareb/gitweb.cgiVery nice job!
Just a couple of suggestion regarding blame.
- Instead of commit sha perhaps is more useful to show author name (linked to commit) and progressive number of revision.
Remember git history is not linear so progressive number does not make much sense here. I agree author and commit date would be nice if they were readily available without cluttering. A pop-up when hovered, perhaps?
Quoted 4 lines
- Original code lines, ie. imported at the beginning and never modified, perhaps it is better to view without commit number, this could obfuscate the view and in any case is not an accurate info because the line was not modified during initial patch.
That holds only true for very young projects, or ones that were perfect from the beginning and did not see much action ;-).
Re: [PATCH 19/19] gitweb: Remove creating directory for temporary files
Quoted 8 lines
Quoted 4 lines
- Original code lines, ie. imported at the beginning and never modified, perhaps it is better to view without commit number, this could obfuscate the view and in any case is not an accurate info because the line was not modified during initial patch.
That holds only true for very young projects, or ones that were perfect from the beginning and did not see much action ;-).
Or also for projects like Linux ;-)
See blame output of something like kernel/dma.c or also kernel/exit.c, not exactly the most unknown files around.
Re: [PATCH 19/19] gitweb: Remove creating directory for temporary files
Quoted 5 lines · Marco Costalba
Quoted 5 lines
Quoted 4 lines
- Original code lines, ie. imported at the beginning and never modified, perhaps it is better to view without commit number, this could obfuscate the view and in any case is not an accurate info because the line was not modified during initial patch.
Quoted 7 lines
Quoted 2 lines
That holds only true for very young projects, or ones that were perfect from the beginning and did not see much action ;-).
Or also for projects like Linux ;-)
See blame output of something like
kernel/dma.cor alsokernel/exit.c, not exactly the most unknown files around.
Actually Linux git archive _is_ a special, odd-ball case. Their initial commit (2.6.12-rc2) contains all the fruits from their 10 years of evolution without git.
People with long history stored in some SCM tend to want to migrate their history and then switch, and I expect that to be the norm, especially with the progress Mozilla import team is making with the CVS interface.
Re: Mozilla import and large history
Quoted 4 lines · Junio C Hamano
People with long history stored in some SCM tend to want to migrate their history and then switch, and I expect that to be the norm, especially with the progress Mozilla import team is making with the CVS interface.
Not to hijack a thread or anything but we are definately making some progress here. Jon Smirl is able to import blob revisions, create trees, commits and tags. Import for Mozilla is down from approx. a week to around 4 hours but right now we are finding that the import isn't always correct.
Fortunately we've built a script to compare the result of the import into GIT with the result of the import into SVN, as the cvs2svn import is believed to be correct. The script is not however very fast, as it compares an SVN revision to the corresponding GIT tree by checking out the SVN revision to a working directory, loading the GIT tree into an index and using git-diff-files to compare them.
That simple script has caught a number of errors but has also confirmed a number of cases as working correctly. We're slowly working through the errors.
We may also do a CVS checkout based script to verify branch heads and labels all match, just as an extra check against what the native SVN import had given.
Signature · 1 line
Shawn.
Re: [PATCH 19/19] gitweb: Remove creating directory for temporary files
In reply to Junio C HamanoQuoted 1 line
Actually Linux git archive _is_ a special, odd-ball case.
It's also our main customer ;-)
Re: [PATCH 19/19] gitweb: Remove creating directory for temporary files
Quoted 4 lines · Marco Costalba
Quoted 1 line
Actually Linux git archive _is_ a special, odd-ball case.
It's also our main customer ;-)
More seriously, do you foreseen reasons for an SCM to split old histories (more then, say few years and some GB ago) in a separated repository?
Now in Linux we have the historical repo and is splitted because it was borne splitted, but do you think this could be also an acceptable policy for a big project after years of git use: to backup old stuff and keep recent one snappy?
With snappy I mean a long list of things: cloning, getting file histories / blaming, packing, fsck-ing, etc...
The net effect could be a better browsing/handling experience in the 99% of cases where you need to check recent work (say last 2 years) and in any case the possibility to do some archeologic research when needed.
Marco
Re: [PATCH 19/19] gitweb: Remove creating directory for temporary files
In reply to Marco CostalbaQuoted 7 lines · Marco Costalba
Quoted 3 lines
You can view new gitweb in work at
http://front.fuw.edu.pl/cgi-bin/jnareb/gitweb.cgiVery nice job!
Just a couple of suggestion regarding blame.
Well, blame code is not mine.
Luben Tuikov did some work on git_blame2 (using git-blame), original work was by Florian Forster (using git-annotate). At least it is what I found.
Signature · 3 lines
Jakub Narebski Warsaw, Poland ShadeHawk on #git
cover gitweb: Remove dependency on external diff and need for temporary files
In reply to Jakub NarebskiThis series of patches (now finished) removes dependency on external diff (/usr/bin/diff) to produce commitdiff and blobdiff views, and the need for temporary files.
Comments appreciated.
You can view new gitweb in work at
http://front.fuw.edu.pl/cgi-bin/jnareb/gitweb.cgi
--- gitweb/gitweb.css | 16 + gitweb/gitweb.perl | 734 +++++++++++++++++++++++++++++++++++----------------- 2 files changed, 515 insertions(+), 235 deletions(-)
Shortlog:
01/19 gitweb: Use git-diff-tree patch output for commitdiff 02/19 gitweb: Replace git_commitdiff_plain by anonymous subroutine 03/19 gitweb: Show information about incomplete lines in commitdiff
Revert "gitweb: Replace git_commitdiff_plain by anonymous
subroutine"
04/19 gitweb: Remove invalid comment in format_diff_line 05/19 gitweb: Streamify patch output in git_commitdiff 06/19 gitweb: Add git_get_{following,preceding}_references functions 07/19 gitweb: Return on first ref found when
git_get_preceding_references is called in scalar context
08/19 gitweb: Add git_get_rev_name_tags function
09/19 gitweb: Use git_get_name_rev_tags for commitdiff_plain
X-Git-Tag: header
10/19 gitweb: Add support for hash_parent_base parameter for blobdiffs 11/19 gitweb: Allow for pre-parsed difftree info in git_patchset_body 12/19 gitweb: Parse two-line from-file/to-file diff header
in git_patchset_body
13/19 gitweb: Add invisible hyperlink to from-file/to-file diff header 14/19 gitweb: Always display link to blobdiff_plain in git_blobdiff 15/19 gitweb: Change here-doc back for consistency in git_blobdiff 16/19 gitweb: Use git-diff-tree or git-diff patch output for blobdiff 17/19 gitweb: git_blobdiff_plain is git_blobdiff('plain') 18/19 gitweb: Remove git_diff_print subroutine
19/19 gitweb: Remove creating directory for temporary files
Signature · 3 lines
Jakub Narebski ShadeHawk on #git Poland
Re: [PATCH 00/19] gitweb: Remove dependency on external diff and need for temporary files
Quoted 4 lines · Jakub Narebski
This series of patches (now finished) removes dependency on external diff (
/usr/bin/diff) to produce commitdiff and blobdiff views, and the need for temporary files.
Ok, can we now please fix my final annouyance, which is that gitweb from the very beginning has apparently believed that the "Signed-off-by:" etc lines are not important, and they get stripped away when looking at the "commit-diff".
Also, "commit-diff" really should have some minimal authorship information. It's silly to have to go to "commit" and then separately ask for "diff" to see all these very basic things.
So I think that "commit-diff" should basically show the equivalent of "git show --pretty", ie author, date, commit message (including sign-offs) and then the diff.
No?
Linus
Re: [PATCH 00/19] gitweb: Remove dependency on external diff and need for temporary files
From: Linus Torvalds <[email protected]>
Date: Sat, 26 Aug 2006 20:30:49 -0700 (PDT)
Quoted 4 lines
Ok, can we now please fix my final annouyance, which is that gitweb from the very beginning has apparently believed that the "Signed-off-by:" etc lines are not important, and they get stripped away when looking at the "commit-diff".
Isn't this to keep the email address from being published on the web and thus harvested by spammers?
If it will obfuscate the email address, that's fine I guess.
Re: [PATCH 00/19] gitweb: Remove dependency on external diff and need for temporary files
Quoted 8 lines · David Miller
Quoted 5 lines
Ok, can we now please fix my final annouyance, which is that gitweb from the very beginning has apparently believed that the "Signed-off-by:" etc lines are not important, and they get stripped away when looking at the "commit-diff".
Isn't this to keep the email address from being published on the web and thus harvested by spammers?
Well, since gitweb already exposes all of those in the "commit" thing (in a different color, but they are there), I don't see why "commit-diff" wouldn't show the same data..
I would certainly _hope_ that every developer has a spam blocker. I don't think not showing email addresses is the answer to _that_ particular problem (and if somebody wants to harvest the email addresses for a git project that is exposed with gitweb, there are more efficient ways to do that ;)
Linus
Re: [PATCH 00/19] gitweb: Remove dependency on external diff and need for temporary files
In reply to Linus TorvaldsQuoted 10 lines · Linus Torvalds
Quoted 4 lines · Jakub Narebski
This series of patches (now finished) removes dependency on external diff (
/usr/bin/diff) to produce commitdiff and blobdiff views, and the need for temporary files.Ok, can we now please fix my final annoyance, which is that gitweb from the very beginning has apparently believed that the "Signed-off-by:" etc lines are not important, and they get stripped away when looking at the "commit-diff".
This can be easily fixed.
Quoted 3 lines
Also, "commit-diff" really should have some minimal authorship information. It's silly to have to go to "commit" and then separately ask for "diff" to see all these very basic things.
And this need some layout redesign for commitdiff and log views. Currently it is specially formatted subject/first line, simplified message (with empty lines collapsed), and without signoff.
Perhaps something similar to what "log" view uses?
Signature · 3 lines
Jakub Narebski Warsaw, Poland ShadeHawk on #git