Those are additional comments, most of which I have come about when
rewriting this series and testing it.
There are to complement existing comments in other post(s).
Kato Kazuyoshi <kato.kazuyoshi@xxxxxxxxx> writes:
> ---
> gitweb/gitweb.perl | 81 +++++++++++++++++++++++++++++++++++++++++----
> gitweb/static/gitweb.css | 15 ++++++++
> 2 files changed, 88 insertions(+), 8 deletions(-)
No tests.
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 095adda..dca09dc 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -757,6 +757,7 @@ our @cgi_param_mapping = (
> extra_options => "opt",
> search_use_regexp => "sr",
> ctag => "by_tag",
> + diff_style => "ds",
> # this must be last entry (for manipulation from JavaScript)
> javascript => "js"
> );
Alternate solution would be to use 'extra_options' ("opt") for
that... though current use of it in gitweb seems to suggest that it is
more about passing extra options to underlying git commands; and "git
diff" doesn't support '--side-by-side' like GNU diff does, (yet?).
So currently I favor neither.
> @@ -1072,6 +1073,8 @@ sub evaluate_and_validate_params {
> }
> $search_regexp = $search_use_regexp ? $searchtext : quotemeta $searchtext;
> }
> +
> + $input_params{diff_style} ||= 'inline';
> }
Hmmm... similar option 'order' ("o") had default value set in action
subroutine. I wonder if it wouldn't be better to do the same also in
this situation.
> @@ -2276,7 +2279,7 @@ sub format_diff_line {
> }
> $line = "<span class=\"chunk_info\">@@ $from_text $to_text @@</span>" .
> "<span class=\"section\">" . esc_html($section, -nbsp=>1) .
> "</span>";
> - return "$div_open$line</div>\n";
> + return $diff_class, "$div_open$line</div>\n";
> } elsif ($from && $to && $line =~ m/^\@{3}/) {
> my ($prefix, $ranges, $section) = $line =~ m/^(\@+) (.*?) \@+(.*)$/;
> my (@from_text, @from_start, @from_nlines, $to_text, $to_start, $to_nlines);
This subroutine no longer *format* line to be printed, isn't it?
> @@ -4828,8 +4831,32 @@ sub git_difftree_body {
> print "</table>\n";
> }
>
> +sub format_diff_chunk {
Name: it is not about diff chunk (or hunk), but about a block of lines
in a chunk; in this case block of change lines (rem / add). Also, it
is not about generic diff, only about sidebyside one.
BTW. I think it would be better if this subroutine also managed
context lines.
> + my @chunk = @_;
> +
> + my $first_class = $chunk[0]->[0];
Style: You can use simply $chunk[0][0] here. perlref(1) says:
"The arrow is optional between brackets subscripts, [...]"
> + my @partial = map { $_->[1] } grep { $_->[0] eq $first_class } @chunk;
> +
> + if (scalar @partial < scalar @chunk) {
Style: you can write simply
+ if (@partial < @chunk) {
> + return join '', ("<div class='chunk'><div class='old'>",
> + @partial,
> + "</div>",
> + "<div class='new'>",
> + (map {
> + $_->[1];
> + } @chunk[scalar @partial..scalar @chunk-1]),
> + "</div></div>");
> + } else {
> + return join '', ("<div class='chunk'><div class='",
> + ($first_class eq 'add' ? 'new' : 'old'),
> + "'>",
> + @partial,
> + "</div></div>");
> + }
> +}
Anyway this code is not very clear. You rely on the fact that if
there are two classes, then they are "rem" first, and "add" second.
Also, it is I think overly complicated.
> +
> sub git_patchset_body {
> - my ($fd, $difftree, $hash, @hash_parents) = @_;
> + my ($fd, $is_inline, $difftree, $hash, @hash_parents) = @_;
Rather than passing $is_inline, I think it would be better to pass
$diff_style (with default value filled in)
> my ($hash_parent) = $hash_parents[0];
>
> my $is_combined = (@hash_parents > 1);
> @@ -4940,12 +4967,31 @@ sub git_patchset_body {
>
> # the patch itself
> LINE:
> + my @chunk;
> while ($patch_line = <$fd>) {
> chomp $patch_line;
>
> next PATCH if ($patch_line =~ m/^diff /);
Here is a bug. If patchset consists of more than one patch, if
not-last patches have change that does not have trailing context lines
(changed, added or removed lines at the end of file), then the last
block will be lost (@chunk can be non-empty here).
>
> - print format_diff_line($patch_line, \%from, \%to);
> + my ($class, $line) = format_diff_line($patch_line, \%from, \%to);
> + if ($is_inline) {
That is wrong to test for. You should test if you can use
side-by-side diff, not if you use default output.
Especially that diff can be combined diff of a merge commit, which
cannot be represented as 2-sided side-by-side diff; for such diff
gitweb needs to use inline diff.
> + print $line;
> + } elsif ($class eq 'add' || $class eq 'rem') {
> + push @chunk, [ $class, $line ];
> + } else {
> + if (@chunk) {
> + print format_diff_chunk(@chunk);
> + @chunk = ();
> + } elsif ($class eq 'chunk_header') {
> + print $line;
> + } else {
> + print '<div class="chunk"><div class="old">',
> + $line,
> + '</div><div class="new">',
> + $line,
> + '</div></div>';
All of this should in my opinion be done in format_diff_chunk(), not
in caller. This also introduces a bit of inconsistency in that
added/removed lines are in single block and context lines are each in
its own block.
Additionally you forgot about incomplete lines here, which can apply
either to added lines, removed lines, both of added and removed lines,
and to context lines. Your code generates incorrect info in the case
if incomplete line is either removed line only, or added line only.
[Nb. I have to check my code yet again.]
> +sub diff_nav {
> + my ($style) = @_;
> +
> + my %pairs = (inline => 'inline', 'sidebyside' => 'side by side');
> + join '', ($cgi->start_form({ method => 'get' }),
> + $cgi->hidden('p'),
> + $cgi->hidden('a'),
> + $cgi->hidden('h'),
> + $cgi->hidden('hp'),
> + $cgi->hidden('hb'),
> + $cgi->hidden('hpb'),
> + $cgi->popup_menu('ds', [keys %pairs], $style, \%pairs),
> + $cgi->submit('change'),
> + $cgi->end_form);
> +}
What about 'f' and 'fp' for "blobdiff" view?
> diff --git a/gitweb/static/gitweb.css b/gitweb/static/gitweb.css
> index 7d88509..dc84db2 100644
> --- a/gitweb/static/gitweb.css
> +++ b/gitweb/static/gitweb.css
> @@ -618,6 +618,21 @@ div.remote {
> cursor: pointer;
> }
>
> +/* side-by-side diff */
> +div.chunk {
> + overflow: hidden;
> +}
> +
> +div.chunk div.old {
> + float: left;
> + width: 50%;
> + overflow: hidden;
> +}
> +
> +div.chunk div.new {
> + margin-left: 50%;
> + width: 50%;
> +}
Nice trick of composing CSS layout... though I wonder if there is
perhaps a better solution.
Anyway, I think this addition should be put near style for div.diff
etc.
--
Jakub Narębski
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html