[PATCH 0/2] gitweb: shorten commitdiff index hashes with file modes

[PATCH 0/2] gitweb: shorten commitdiff index hashes with file modes

[Travor Liu]

From: Travor Liu <travor_lzh@outlook.com>

This series fixes gitweb rendering of commitdiff index lines that carry
a trailing file mode, such as:

    index <old>..<new> 100644

gitweb currently recognizes the mode before matching the object IDs,
which appends the file-type annotation first.  The later object-ID
matcher expects the ID range to end the line, so these common index
lines keep two full, unlinked object IDs.

Patch 1 moves the mode handling so the index object IDs can still be
shortened and linked.

Patch 2 adds a gitweb regression test for the common
"index <old>..<new> 100644" form.

Tested with:

    make NO_RUST=YesPlease
    cd t && prove -v t9502-gitweb-standalone-parse-output.sh

Travor Liu (2):
  gitweb: shorten index hashes with trailing file modes
  t9502: test gitweb index hash formatting with modes

 gitweb/gitweb.perl                        | 18 +++++++++++++-----
 t/t9502-gitweb-standalone-parse-output.sh | 14 ++++++++++++++
 2 files changed, 27 insertions(+), 5 deletions(-)

-- 
2.52.0

[PATCH 1/2] gitweb: shorten index hashes with trailing file modes

[Travor Liu]

From: Travor Liu <travor_lzh@outlook.com>

Diff index lines have included a trailing file mode since ec1fcc16af
(Show original and resulting blob object info in diff output,
2005-10-07) when the old and new file modes match:

    index <old>..<new> 100644

gitweb recognizes that trailing mode before it tries to shorten and
link the object IDs.  This appends the file-type annotation first, but
the object-ID matcher requires the ID range to end the line.  As a
result, this common form keeps both full object IDs as plain text.

That is inconsistent with other hash displays and makes commitdiff
output wider than necessary.  Recent gitweb changes have fixed mobile
overflow in log, commit, blob and diff views; leaving two full object
IDs in this header preserves an avoidable long line in the diff header.

Remove the trailing mode before matching the index IDs, then append it
again after the IDs have been shortened and linked.  This preserves the
mode display while letting ordinary and combined index lines use the
existing object-ID formatting paths.

Signed-off-by: Travor Liu <travor_lzh@outlook.com>
---
 gitweb/gitweb.perl | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index fde8045..8c2d9b8 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2339,12 +2339,14 @@ sub format_extended_diff_header_line {
 		$line .= $cgi->a({-href=>$to->{'href'}, -class=>"path"},
 		                 esc_path($to->{'file'}));
 	}
-	# match single <mode>
-	if ($line =~ m/\s(\d{6})$/) {
-		$line .= '<span class="info"> (' .
-		         file_type_long($1) .
-		         ')</span>';
+
+	# Temporarily remove a trailing <mode> so an index line ends with its
+	# object IDs and can be shortened below.
+	my $mode;
+	if ($line =~ s/\s(\d{6})$//) {
+		$mode = $1;
 	}
+
 	# match <hash>
 	if ($line =~ oid_nlen_prefix_infix_regex($sha1_len, "index ", ",") |
 	    $line =~ oid_nlen_prefix_infix_regex($sha256_len, "index ", ",")) {
@@ -2388,6 +2390,12 @@ sub format_extended_diff_header_line {
 		my ($from_id, $to_id) = ($diffinfo->{'from_id'}, $diffinfo->{'to_id'});
 		$line =~ s!$from_id\.\.$to_id!$from_link..$to_link!;
 	}
+	if (defined $mode) {
+		$line .= " $mode" .
+		         '<span class="info"> (' .
+		         file_type_long($mode) .
+		         ')</span>';
+	}
 
 	return $line . "<br/>\n";
 }
-- 
2.52.0

[PATCH 2/2] t9502: test gitweb index hash formatting with modes

[Travor Liu]

From: Travor Liu <travor_lzh@outlook.com>

gitweb should shorten and link the object IDs in commitdiff index lines
even when Git includes the trailing file mode:

    index <old>..<new> 100644

Add coverage for that common form by rendering a commitdiff for a
regular file modification.  Check that the visible index line contains
linked short blob IDs followed by the mode and file-type annotation,
and that the full unlinked form is not emitted.

Signed-off-by: Travor Liu <travor_lzh@outlook.com>
---
 t/t9502-gitweb-standalone-parse-output.sh | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/t/t9502-gitweb-standalone-parse-output.sh b/t/t9502-gitweb-standalone-parse-output.sh
index 81d5625..7f37e26 100755
--- a/t/t9502-gitweb-standalone-parse-output.sh
+++ b/t/t9502-gitweb-standalone-parse-output.sh
@@ -115,6 +115,20 @@ test_expect_success 'snapshot: hierarchical branch name (xx/test)' '
 '
 test_debug 'cat gitweb.headers'
 
+test_expect_success 'commitdiff: index line shortens hashes with mode' '
+	old_blob=$(git rev-parse HEAD:foo) &&
+	old_short=$(git rev-parse --short=7 HEAD:foo) &&
+	echo changed >foo &&
+	git commit -am "change foo" &&
+	new_blob=$(git rev-parse HEAD:foo) &&
+	new_short=$(git rev-parse --short=7 HEAD:foo) &&
+	gitweb_run "p=.git;a=commitdiff;h=HEAD" &&
+	grep ">${old_short}</a>\\.\\.<a [^>]*>${new_short}</a> 100644" \
+		gitweb.body >index_line &&
+	grep "<span class=\"info\"> (file)</span>" index_line &&
+	! grep "index ${old_blob}\\.\\.${new_blob} 100644" gitweb.body
+'
+
 # ----------------------------------------------------------------------
 # forks of projects
 
-- 
2.52.0

Re: [PATCH 2/2] t9502: test gitweb index hash formatting with modes

[Junio C Hamano]

Travor@web.codeaurora.org, "Liu <travor_lzh"@outlook.com writes:

> From: Travor Liu <travor_lzh@outlook.com>
>
> gitweb should shorten and link the object IDs in commitdiff index lines
> even when Git includes the trailing file mode:
>
>     index <old>..<new> 100644
>
> Add coverage for that common form by rendering a commitdiff for a
> regular file modification.  Check that the visible index line contains
> linked short blob IDs followed by the mode and file-type annotation,
> and that the full unlinked form is not emitted.
>
> Signed-off-by: Travor Liu <travor_lzh@outlook.com>
> ---
>  t/t9502-gitweb-standalone-parse-output.sh | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)

If the new test added by this patch validates the "fix" in
[PATCH 1/2], reviewing the change might be easier if the two
were squashed into a single patch.

> diff --git a/t/t9502-gitweb-standalone-parse-output.sh b/t/t9502-gitweb-standalone-parse-output.sh
> index 81d5625..7f37e26 100755
> --- a/t/t9502-gitweb-standalone-parse-output.sh
> +++ b/t/t9502-gitweb-standalone-parse-output.sh
> @@ -115,6 +115,20 @@ test_expect_success 'snapshot: hierarchical branch name (xx/test)' '
>  '
>  test_debug 'cat gitweb.headers'
>  
> +test_expect_success 'commitdiff: index line shortens hashes with mode' '
> +	old_blob=$(git rev-parse HEAD:foo) &&
> +	old_short=$(git rev-parse --short=7 HEAD:foo) &&
> +	echo changed >foo &&
> +	git commit -am "change foo" &&
> +	new_blob=$(git rev-parse HEAD:foo) &&
> +	new_short=$(git rev-parse --short=7 HEAD:foo) &&
> +	gitweb_run "p=.git;a=commitdiff;h=HEAD" &&
> +	grep ">${old_short}</a>\\.\\.<a [^>]*>${new_short}</a> 100644" \
> +		gitweb.body >index_line &&
> +	grep "<span class=\"info\"> (file)</span>" index_line &&
> +	! grep "index ${old_blob}\\.\\.${new_blob} 100644" gitweb.body
> +'

Can we use "test_grep" (for positive "this string must be there") and
"test_grep !" (for negative "it is an error if this string appears"
(note that exclamation point comes after test_grep))?  It would make
it easier to diagnose a failing test.

Also, there is a topic in flight that enforces the use of test_grep in
these test scripts, and use of raw grep like the above would break
under those stricter rules.

Thanks.