gitweb: show no difference message

treediff supports comparing different trees. A tree can be specified either as hash or as base hash and filename.

gitweb/gitweb.perl +6 -0

+		if (defined $hash_parent_base && (my %co = parse_commit($hash_parent_base))) {
+	 		$formats_nav .= " | from: ".
+				$cgi->a({-href => href(action=>"commit",
+			        	               hash=>$hash_parent_base)},
+				        "commit")
+				." | ".

gitweb/gitweb.perl +6 -0

+
+	# read treediff
+	my $fd;
+	my @difftree;
+	if ($format eq 'html') {
+		open $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts,

gitweb/gitweb.perl +11 -0

+	}
+
+	# non-textual hash id's can be cached
+	my $expires;
+	if ($hash =~ m/^[0-9a-fA-F]{40}$/) {
+		$expires = "+1d";
+	}
+
+	# write header
+	if ($format eq 'html') {
+

Quoted 2 lines · Martin Koegler

treediff supports comparing different trees. A tree can be specified either as hash or as base hash and filename.

I'd use "treediff" view, or git_treediff subroutine. Just a minor nit.

gitweb/gitweb.perl sub git_commitdiff_plain { · line 4251

Quoted 4 lines

gitweb/gitweb.perl +4 -0

+sub git_treediff {
+	my $format = shift || 'html';
+	my $from = $file_parent || "";
+	my $to = $file_name || "";

I'd use $file_name || ''; here, and of course

(quoted diff) +1 -0

+	my $from = $file_parent || $file_name || '';

The unwritten rule is that we use 'fp' parameter (thet $file_parent variable is set) only

gitweb/gitweb.perl sub git_commitdiff_plain { · line 4255

Quoted 4 lines

gitweb/gitweb.perl +4 -0

+
+	if (!defined $hash) {
+		if (!defined $hash_base) {
+			die_error('','tree parameter missing');

This conflicts with the coding style used elsewhere in the gitweb (the informal coding convention / guideline for gitweb).

First, we either use undef and not '' to say: use default value of the first parameter (HTTP status) of die_error, or provide our own value in single quotes.

Second, the second parameter, error message, is a sentence (without final fullstop) describing error; it means starting it with capital letter. And we use double quotes for this parameter.

Examples:

	die_error(undef, "Couldn't find base commit");
	die_error(undef, "Unknown commit object");
	die_error(undef, "No file name defined");
	die_error(undef, "Open git-diff-tree failed");

	die_error('403 Permission denied', "Permission denied");
	die_error('404 Not Found', "File name not defined");
	die_error('404 Not Found', "Not enough information to find object");
	die_error('400 Bad Request', "Object is not a blob");

gitweb/gitweb.perl sub git_commitdiff_plain { · line 4266

Quoted 11 lines

gitweb/gitweb.perl +11 -0

+		}
+		$hash = $hash_base;
+		$hash .= ":".$file_name if (defined $file_name);
+	}
+
+	if (!defined $hash_parent) {
+		if (!defined $hash_parent_base) {
+			die_error('','tree parameter missing');
+		}
+		$hash_parent = $hash_parent_base;
+		$hash_parent .= ":".$file_parent if (defined $file_parent);

The same problem as above: we do not set 'fp' parameter (and $file_parent variable) if name does not change. So you should use instead:

Quoted 12 lines

gitweb/gitweb.perl +12 -0

+		if (defined $hash_parent_base && (my %co = parse_commit($hash_parent_base))) {
+	 		$formats_nav .= " | from: ".
+				$cgi->a({-href => href(action=>"commit",
+			        	               hash=>$hash_parent_base)},
+				        "commit")
+				." | ".
+				$cgi->a({-href => href(action=>"commitdiff",
+				                       hash=>$hash_parent_base)},
+				        "commitdiff")
+				." | ".
+				$cgi->a({-href => href(action=>"tree",
+						       hash=>$co{'tree'},

gitweb/gitweb.perl sub git_commitdiff_plain { · line 4294

Quoted 4 lines

gitweb/gitweb.perl +4 -0

+				                       hash_base=>$hash_parent_base)},
+				        "tree");
+		}
+	}

This depends if the similar change (feature) for "blobdiff" view (in git_blobdiff subroutine) is accepted. Perhaps this could be separated into separate patch?

Quoted 11 lines

gitweb/gitweb.perl +11 -0

+
+	# read treediff
+	my $fd;
+	my @difftree;
+	if ($format eq 'html') {
+		open $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts,
+			"--no-commit-id", "--patch-with-raw", "--full-index",
+			$hash_parent, $hash, "--"
+			or die_error(undef, "Open git-diff-tree failed");
+
+		while (my $line = <$fd>) {

gitweb/gitweb.perl sub git_commitdiff_plain { · line 4310

Quoted 5 lines

gitweb/gitweb.perl +5 -0

+			chomp $line;
+			# empty line ends raw part of diff-tree output
+			last unless $line;
+			push @difftree, $line;
+		}

This is also common with git_commitdiff. Should be it put into separate subroutine? Or do this refactoring in another patch?

gitweb/gitweb.perl sub git_commitdiff_plain { · line 4343

Quoted 4 lines

gitweb/gitweb.perl +4 -0

+
+	} elsif ($format eq 'plain') {
+		my $filename = basename($project) . "-diff.patch";
+

In "commitdiff_plain" view we use

		my $filename = basename($project) . "-$hash.patch";

Perhaps we should use the same: "-diff.patch" does not make much sense. Is it a typo?

Quoted 9 lines · Jakub Narebski

gitweb/gitweb.perl sub git_commitdiff_plain { · line 4251

Quoted 4 lines · Martin Koegler

gitweb/gitweb.perl +4 -0

+sub git_treediff {
+	my $format = shift || 'html';
+	my $from = $file_parent || "";
+	my $to = $file_name || "";

I'd use $file_name || ''; here, and of course

Quoted 4 lines

(quoted diff) +1 -0

+	my $from = $file_parent || $file_name || '';

The unwritten rule is that we use 'fp' parameter (thet $file_parent variable is set) only

How do I specifiy the root tree (=tree in commit) with hpb/fp, as fp can not be empty, but only undefined?

Quoted 6 lines

gitweb/gitweb.perl sub git_commitdiff_plain { · line 4255

Quoted 4 lines

gitweb/gitweb.perl +4 -0

+
+	if (!defined $hash) {
+		if (!defined $hash_base) {
+			die_error('','tree parameter missing');

This conflicts with the coding style used elsewhere in the gitweb

Quoted 2 lines

Examples:

	die_error(undef, "Couldn't find base commit");

I didn't know this. I'll change this.

Quoted 7 lines

gitweb/gitweb.perl sub git_commitdiff_plain { · line 4343

Quoted 4 lines

gitweb/gitweb.perl +4 -0

+
+	} elsif ($format eq 'plain') {
+		my $filename = basename($project) . "-diff.patch";
+

In "commitdiff_plain" view we use

Quoted 4 lines

		my $filename = basename($project) . "-$hash.patch";

Perhaps we should use the same: "-diff.patch" does not make much sense. Is it a typo?

No. What unique name do you propose? It needs to include $hash and $hase_parent. In this case, $hash could be $hash_base:$file_name, I'm not sure, how to escape $file_name.

git_treediff supports comparing subdirectories. As the output of git-difftree is missing the path to the compared directories, the links in the output would be wrong.

The patch adds two new parameters to add the missing path prefix.

gitweb/gitweb.perl +1 -1

@@ -2439,14 +2442,14 @@ sub git_patchset_body {
 			if ($diffinfo->{'status'} ne "A") { # not new (added) file
 				$from{'href'} = href(action=>"blob", hash_base=>$hash_parent,
 				                     hash=>$diffinfo->{'from_id'},
-				                     file_name=>$from{'file'});
+				                     file_name=>$file_parent.$from{'file'});
 			} else {

Quoted 5 lines · Martin Koegler

git_treediff supports comparing subdirectories. As the output of git-difftree is missing the path to the compared directories, the links in the output would be wrong.

The patch adds two new parameters to add the missing path prefix.

Wouldn't it be better to concatenate the two "path prefix" patches together? They are about the same thing.

gitweb/gitweb.perl sub git_difftree_body { · line 2378

Quoted 6 lines

sub git_patchset_body {

gitweb/gitweb.perl +1 -1

-	my ($fd, $difftree, $hash, $hash_parent) = @_;
+	my ($fd, $difftree, $hash, $hash_parent, $file_name, $file_parent) = @_;
 
 	my $patch_idx = 0;
 	my $patch_line;

I'd rather use $from_prefix, $to_prefix here, or $basedif_name, $basedir_parent, or $dir_name, $dir_parent (my preference is to $from_prefix, $to_prefix variables).

or something like that, or just modify $from{'file'} and $to{'file'}

	$from{'file'} = (!defined $from_prefix ? '' : $from_prefix.'/') . $from{'file'};
	$to{'file'}   = (!defined $to_prefix   ? '' : $to_prefix . '/') . $to{'file'};

just after setting $from{'file'} and $to{'file'}, although the second solution would additionally add prefix to the shown patch body itself.

Quoted 6 lines · Jakub Narebski

Quoted 4 lines · Martin Koegler

git_treediff supports comparing subdirectories. As the output of git-difftree is missing the path to the compared directories, the links in the output would be wrong.

Quoted 4 lines

Quoted 1 line

The patch adds two new parameters to add the missing path prefix.

Wouldn't it be better to concatenate the two "path prefix" patches together? They are about the same thing.

I thought, each patch would be more readable, I split them in logical separate units. Anyway, I'll combine them.

Quoted 10 lines

gitweb/gitweb.perl sub git_difftree_body { · line 2378

Quoted 6 lines

sub git_patchset_body {

gitweb/gitweb.perl +1 -1

-	my ($fd, $difftree, $hash, $hash_parent) = @_;
+	my ($fd, $difftree, $hash, $hash_parent, $file_name, $file_parent) = @_;
 
 	my $patch_idx = 0;
 	my $patch_line;

I'd rather use $from_prefix, $to_prefix here, or $basedif_name, $basedir_parent, or $dir_name, $dir_parent (my preference is to $from_prefix, $to_prefix variables).

I'll switch to $to_prefix and $from_prefix.

Review

Quoted 3 lines

(quoted diff) +3 -0

+	$from_prefix = !defined $from_prefix ? '' : $from_prefix.'/';
+	$to_prefix   = !defined $to_prefix   ? '' : $to_prefix . '/';
+	$to_prefix ||= $from_prefix;  # to allow to pass common prefix once

OK. But is not the 3 line useless, as $to_prefix is alway defined after the second line and you probable want $from_prefix ||= $to_prefix. This will cause problems, as I currently pass the root tree (=tree in commit object) as an missing file name parameter, as gitweb does not allow an empty file name.

With an propagation logic, comparing a root tree with an sub tree will only work in one direction.

So I prefer to do not implement any automatic propagation between the two prefixes.

Quoted 7 lines

or something like that, or just modify $from{'file'} and $to{'file'}

	$from{'file'} = (!defined $from_prefix ? '' : $from_prefix.'/') . $from{'file'};
	$to{'file'}   = (!defined $to_prefix   ? '' : $to_prefix . '/') . $to{'file'};

just after setting $from{'file'} and $to{'file'}, although the second solution would additionally add prefix to the shown patch body itself.

Modifing the paths before generating the links is a good idea. I'll look, where its useful.

It is done by adding $file_parent ('fp') to the path limiter, meaning that diff command becomes:

git diff-tree [options] hpb hb -- fp f

instead of finding hash of a blob using git_get_hash_by_path subroutine or using extended SHA-1 syntax:

git diff [options] hpb:fp hp:f

Quoted 10 lines · Jakub Narebski

It is done by adding $file_parent ('fp') to the path limiter, meaning that diff command becomes:

git diff-tree [options] hpb hb -- fp f

instead of finding hash of a blob using git_get_hash_by_path subroutine or using extended SHA-1 syntax:

git diff [options] hpb:fp hp:f

As far as I tested, this only works for renames, not for comparing different objects, eg:

git-diff -r -p 08727ea8bba8c81678e5cf15124ada23ad097bc3:grep.h bb95e19c5f1e470d2efe1c0e4e04c291019e4b25:refs.h shows differences

git-diff-tree -r 08727ea8bba8c81678e5cf15124ada23ad097bc3 bb95e19c5f1e470d2efe1c0e4e04c291019e4b25 -- grep.h refs.h shows no difference

As I want gitweb to be able to even do such compares (not just renames), I'll still need a solution for this.

My idea is, that if I got hb:f and hpb:fp, the user exactly specified the blobs to be compared. Then I don't want any guessing logic.

Quoted 2 lines · Martin Koegler

My idea is, that if I got hb:f and hpb:fp, the user exactly specified the blobs to be compared. Then I don't want any guessing logic.

I'd rather you reuse the "no hash_parent" code, which also hand-crafts diffinfo. Perhaps removing "git-diff-tree hpb hb -- f" code entirely. Besides, code dealing with "blobdiff" coming from "commit", "commitdiff" and "history" views are tested to work as expected, not so with arbitrary diffs.

By the way, if you call git_get_hash_by_path (which is expensive, as it calls git command), you can use resulting hash in place of hash_base:filename as an argument to git-diff.

Quoted 9 lines · Jakub Narebski

Quoted 2 lines · Martin Koegler

My idea is, that if I got hb:f and hpb:fp, the user exactly specified the blobs to be compared. Then I don't want any guessing logic.

I'd rather you reuse the "no hash_parent" code, which also hand-crafts diffinfo. Perhaps removing "git-diff-tree hpb hb -- f" code entirely. Besides, code dealing with "blobdiff" coming from "commit", "commitdiff" and "history" views are tested to work as expected, not so with arbitrary diffs.

I don't like the whole rename detection code, so I offer to simplify git_blobdiff. For all calls to git_blobdiff (except those from git_history), I'm sure, that I can assume $file_parent ||= $file_name.

If you think, its safe, I can simplify git_blobdiff. I propose doing the following way (pseudo-code):

$file_parent ||= $file_name;

$hash=git_get_hash_by_path($hash_base,$file_name);

$hash_parent=git_get_hash_by_path($hash_parent_base,$file_parent);

open $fd, "-|", git_cmd(), "diff", '-p', @diff_opts,

    $hash_parent, $hash
    or die_error(undef, "Open git-diff failed");

Else I will keep a reworked version of my patch.

Quoted 3 lines

By the way, if you call git_get_hash_by_path (which is expensive, as it calls git command), you can use resulting hash in place of hash_base:filename as an argument to git-diff.

I must check, if we need to resolve $hash ($hash_parent) by git_get_hash_by_path, if we construct it out of $hash_base and $file_name. Maybe we can avoid this call.

Quoted 5 lines · Martin Koegler

On Tue, Mar 27, 2007 at 01:56:24AM +0100, Jakub Narebski wrote:
If you think, its safe, I can simplify git_blobdiff. I propose doing the following way (pseudo-code):

$file_parent ||= $file_name;

[...]

Quoted 1 line

$hash=git_get_hash_by_path($hash_base,$file_name);

[...]

Quoted 1 line

$hash_parent=git_get_hash_by_path($hash_parent_base,$file_parent);

[...]

Quoted 3 lines

open $fd, "-|", git_cmd(), "diff", '-p', @diff_opts,

    $hash_parent, $hash
    or die_error(undef, "Open git-diff failed");

[...]

Quoted 1 line

Else I will keep a reworked version of my patch.

The trouble with this is that we may lose mode change (symlink to ordinary file etc.) because we hand-generate %diffinfo.

x deleted +0 -1

deleted file mode 100644 (file)
index 190a180..873fb8d
@@ -1 +0,0 @@
-123

b/ new file +0 -0

new file mode 120000 (symlink)
index 190a180..873fb8d

Quoted 7 lines

Quoted 3 lines

By the way, if you call git_get_hash_by_path (which is expensive, as it calls git command), you can use resulting hash in place of hash_base:filename as an argument to git-diff.

I must check, if we need to resolve $hash ($hash_parent) by git_get_hash_by_path, if we construct it out of $hash_base and $file_name. Maybe we can avoid this call.

We can use "$hash_base:$file_name" as second parameter to git-diff etc., but I don't think we want to create links with "$hash_base:$file_name" instead of sha-1 id of a blob as 'h' parameter.

It can be first implementation, thought, and later we can try to use "index <hash>..<hash> <mode>" lines from extended header to get $hash and $hash_parent (with exception of pure rename, but then we need only one invocation of git_get_hash_by_path subroutine).

But I think it is better left for later patch.

Quoted 12 lines · Jakub Narebski

Quoted 6 lines · Martin Koegler

On Tue, Mar 27, 2007 at 01:56:24AM +0100, Jakub Narebski wrote:

If you think, its safe, I can simplify git_blobdiff. I propose doing the following way (pseudo-code):

$file_parent ||= $file_name;

[...]

Quoted 1 line

$hash=git_get_hash_by_path($hash_base,$file_name);

[...]

Quoted 1 line

$hash_parent=git_get_hash_by_path($hash_parent_base,$file_parent);

[...]

Quoted 8 lines

Quoted 3 lines

open $fd, "-|", git_cmd(), "diff", '-p', @diff_opts,

    $hash_parent, $hash
    or die_error(undef, "Open git-diff failed");

[...]

Quoted 1 line

Else I will keep a reworked version of my patch.

The trouble with this is that we may lose mode change (symlink to ordinary file etc.) because we hand-generate %diffinfo.

If we need the correct mode in %diffinfo, this is not difficult:

$from_mode="blob";
if (defined $hash_base && defined $file_name) ($to_mode,$hash)=git_get_hash_by_path($hash_base,$file_name);

Then we set to_mode and from_mode in %diffinfo to $to_mode and $from_mode.

git_get_hash_by_path_mode is like git_get_hash_by_path, only that it returns an array with the mode and the hash.

Quoted 12 lines

Quoted 7 lines

Quoted 3 lines

By the way, if you call git_get_hash_by_path (which is expensive, as it calls git command), you can use resulting hash in place of hash_base:filename as an argument to git-diff.

I must check, if we need to resolve $hash ($hash_parent) by git_get_hash_by_path, if we construct it out of $hash_base and $file_name. Maybe we can avoid this call.

We can use "$hash_base:$file_name" as second parameter to git-diff etc., but I don't think we want to create links with "$hash_base:$file_name" instead of SHA-1 id (hash) of a blob as 'h' parameter.

Quoted 4 lines

It can be first implementation, thought, and later we can try to use "index <hash>..<hash> <mode>" lines from extended header to get $hash and $hash_parent (with exception of pure rename, but then we need only one invocation of git_get_hash_by_path subroutine).

The <mode> must be discarded, as it is wrong for anything which as not a mode of 100644, as we specify a two blob as parameter to git-diff.

Quoted 1 line

But I think it is better left for later patch.

As git_patchset_body requires an information about the compared file as parameter, a new formating function will be needed. I'm not sure, if the overhead of git_get_hash_by_path is this worth. Additionally, if we have the hash passed by parameter in most cases, there is no need to call it in these cases.

Blobdiff (html output) in its current version can not handle symlinks:

Quoted 13 lines

x deleted +0 -1

deleted file mode 100644 (file)
index 190a180..873fb8d
@@ -1 +0,0 @@
-123

b/ new file +0 -0

new file mode 120000 (symlink)
index 190a180..873fb8d

b/ line 1

Quoted 2 lines

b/ new file +1 -0

@@ -0,0 +1 @@
+file3
\ No newline at end of file

This was generated by "diff to current" in the history view of a file, which was changed between symlink and normal file.

Additionally, to_mode and from_mode of %diffinfo seem to be ignored by git_patchset_body.

mfg Martin Kögler
PS:
I created a blob with a "strange" filename: &()=!"§$%[<>]*#+_;.
In the result of the blob view, the " is not escaped in the filename in the header and a strange content type is returned:

$ telnet localhost 80
Trying 127.0.0.1...
Connected to localhost.
Escape character is '^]'.
GET /gitweb/gitweb.cgi?p=t/.git;a=blob;f=%26()%3D%21%22%A7%24%25%5B%3C%3E%5D%2A%23%2B_%3B.;hb=7bfed2588bee66b33db544830606fa6606478fd9 HTTP/1.0

HTTP/1.1 200 OK
Date: Wed, 28 Mar 2007 19:55:36 GMT
Server: Apache
Content-disposition: inline; filename="&()=!"§$%[<>]*#+_;."
Expires: Thu, 29 Mar 2007 19:55:39 GMT
Connection: close
Content-Type: application/vnd.mif

xx

I created a blob with a "strange" filename: &()=!"§$%[<>]*#+_;.
In the result of the blob view, the " is not escaped in the filename in the header and a strage content type is returned:

$ telnet localhost 80
Trying 127.0.0.1...
Connected to localhost.
Escape character is '^]'.
GET /gitweb/gitweb.cgi?p=t/.git;a=blob;f=%26()%3D%21%22%A7%24%25%5B%3C%3E%5D%2A%23%2B_%3B.;hb=7bfed2588bee66b33db544830606fa6606478fd9 HTTP/1.0

HTTP/1.1 200 OK
Date: Wed, 28 Mar 2007 19:55:36 GMT
Server: Apache
Content-disposition: inline; filename="&()=!"§$%[<>]*#+_;."
Expires: Thu, 29 Mar 2007 19:55:39 GMT
Connection: close
Content-Type: application/vnd.mif

xx

Second is detecting file as binary (for text files we always return HTML), and content type set. Gitweb checks mimetype of a file (based on extension, mainly) by default using /etc/mime.magic, and if it is neither text/* nor HTML images (png, jpg, gif) it uses "blobdiff_plain" view. Perhaps instead of just calling git_blob_plain($mimetype) we should do a redirect (but then how to pass detected mimetype?). It looks like there is an error with mimetype detection: could you tell me the line with "application/vnd.mif" in your /etc/mime.types?

Quoted 18 lines · Martin Koegler

I created a blob with a "strange" filename: &()=!"§$%[<>]*#+_;.
In the result of the blob view, the " is not escaped in the filename in the header and a strange content type is returned:

$ telnet localhost 80
Trying 127.0.0.1...
Connected to localhost.
Escape character is '^]'.
GET /gitweb/gitweb.cgi?p=t/.git;a=blob;f=%26()%3D%21%22%A7%24%25%5B%3C%3E%5D%2A%23%2B_%3B.;hb=7bfed2588bee66b33db544830606fa6606478fd9 HTTP/1.0

HTTP/1.1 200 OK
Date: Wed, 28 Mar 2007 19:55:36 GMT
Server: Apache
Content-disposition: inline; filename="&()=!"§$%[<>]*#+_;."
Expires: Thu, 29 Mar 2007 19:55:39 GMT
Connection: close
Content-Type: application/vnd.mif

Quoted 1 line

xx

There are two separate things.

First is not escaped filename in HTTP header. There was some discussion about this, and even patch by Luben Tuikov which added to_qtext subroutine to deal with escaping in HTTP (which has diferent rules than escaping in HTML, or in HTML attributes)
* gitweb: using quotemeta
http://thread.gmane.org/gmane.comp.version-control.git/28050/
* [PATCH] gitweb: Convert Content-Disposition filenames into qtext
http://thread.gmane.org/gmane.comp.version-control.git/28437
But the patch was newer accepted; either lost in the noise, or in lack of summary to the discussion.

Second is detecting file as binary (for text files we always return HTML), and content type set. Gitweb checks mimetype of a file (based on extension, mainly) by default using /etc/mime.magic, and if it is neither text/* nor HTML images (png, jpg, gif) it uses "blobdiff_plain" view. Perhaps instead of just calling git_blob_plain($mimetype) we should do a redirect (but then how to pass detected mimetype?). It looks like there is an error with mimetype detection: could you tell me the line with "application/vnd.mif" in your /etc/mime.types?

Quoted 11 lines · Jakub Narebski

Quoted 10 lines

First is not escaped filename in HTTP header. There was some discussion about this, and even patch by Luben Tuikov which added to_qtext subroutine to deal with escaping in HTTP (which has diferent rules than escaping in HTML, or in HTML attributes)
* gitweb: using quotemeta
http://thread.gmane.org/gmane.comp.version-control.git/28050/
* [PATCH] gitweb: Convert Content-Disposition filenames into qtext
http://thread.gmane.org/gmane.comp.version-control.git/28437
But the patch was newer accepted; either lost in the noise, or in lack of summary to the discussion.

Quoted 1 line

Junio, do you remember by chance why this patch was dropped?

No, but I suspect that was because the noisiness of the thread around them suggested they were not ready to be applied. I do not remember if people submitted the patch and commented on reached a consensus.

Quoted 6 lines · Junio C Hamano

Quoted 4 lines · Jakub Narebski

Quoted 3 lines

First is not escaped filename in HTTP header. There was some discussion about this, and even patch by Luben Tuikov which added to_qtext subroutine to deal with escaping in HTTP [...]

Quoted 6 lines

Quoted 1 line

Junio, do you remember by chance why this patch was dropped?

No, but I suspect that was because the noisiness of the thread around them suggested they were not ready to be applied. I do not remember if people submitted the patch and commented on reached a consensus.

Probably not. Here is alternative proposal. It does not implement
RFC2184: MIME Parameter Value and Encoded Word Extensions
but I'm not sure if 1) it is needed for _HTTP_ Content-Disposition header filename, 2) all browsers implement it.

P.P.S. Here is an example of RFC2184 encoded header:

   Content-Type: application/x-stuff
    title*1*=us-ascii'en'This%20is%20even%20more%20
    title*2*=%2A%2A%2Afun%2A%2A%2A%20
    title*3="isn't it!"

As to browsers: Mozilla 1.7.12 implements RFC2183 correctly, although for example shows %0A / \n as a strange symbol in "save as" dialog, created file has embedded newline in filename, as it should. But both Lynx 2.8.5, and ELinks 0.10.3 do not implement it fully and without errors.

So that is why we have:

# It not worth potential problems to try to carry newlines # in the header; it is just _suggested_ filename $filename =~ s/[[:cntrl:]\n\r]/_/g;

Blobdiff (html output) in its current version can not handle symlinks:

Quoted 10 lines

x deleted +0 -1

deleted file mode 100644 (file)
index 190a180..873fb8d
@@ -1 +0,0 @@
-123

(quoted diff) new file +0 -0

new file mode 120000 (symlink)
index 190a180..873fb8d

b/ line 1

Quoted 5 lines

b/ new file +1 -0

@@ -0,0 +1 @@
+file3
\ No newline at end of file

This was generated by "diff to current" in the history view of a file, which was changed between symlink and normal file.

Currently, gitweb shows only header and footer, if no differences are found. This patch adds a "No differences are found" message for the html output.

gitweb/gitweb.perl +2 -0

index 5214050..fbadab4 100755
@@ -2376,6 +2376,7 @@ sub git_patchset_body {
 
 	my $patch_idx = 0;
 	my $patch_line;
+	my $empty = 0;
 	my $diffinfo;
 	my (%from, %to);
 

@@ -2396,6 +2397,7 @@ sub git_patchset_body {
 		# git diff header
 		#assert($patch_line =~ m/^diff /) if DEBUG;
 		#assert($patch_line !~ m!$/$!) if DEBUG; # is chomp-ed
+		$empty++;
 		push @diff_header, $patch_line;
 
 		# extended diff header

Quoted 20 lines · Martin Koegler

gitweb/gitweb.perl +2 -0

index 5214050..fbadab4 100755
@@ -2376,6 +2376,7 @@ sub git_patchset_body {
 
 	my $patch_idx = 0;
 	my $patch_line;
+	my $empty = 0;
 	my $diffinfo;
 	my (%from, %to);
 

@@ -2396,6 +2397,7 @@ sub git_patchset_body {
 		# git diff header
 		#assert($patch_line =~ m/^diff /) if DEBUG;
 		#assert($patch_line !~ m!$/$!) if DEBUG; # is chomp-ed
+		$empty++;
 		push @diff_header, $patch_line;
 
 		# extended diff header

First, programming style. You named the variable $empty, when it evaluates to true when the patch is _not_ empty, i.e. when some differences are found.