Re: [PATCH/RFC (take 2)] gitweb: New improved patchset view

Jakub Narebski [off-list ref] writes:

Few other questions, probably to be adressed in the future patches, and 
not added to this one.

0. git-ls-tree and git-diff-tree without -z does quote not only ...
also UTF-8 characters.

I've already mentioned this in an earlier message to you:

	Message-ID: [off-list ref]

Let's illustrate what I mean by an untested patch; this does:

 0. Use explicitly "unsigned char" so that (ch < ' ') does not
    catch bytes in 0x80- range.  The original meant to catch the
    control characters only so this is a bugfix;

 1. We still worry about control characters in 0x80-0x9f range;
    if there are some, that is not a valid UTF-8 string (or
    other encodings that is compatible with ASCII), and quoting
    only these bytes and not quoting 0xa0- range can result
    in letters chopped in the middle, so we would quote all
    bytes in 0xa0- range when we have them;

 2. Otherwise we do not quote bytes in 0xa0- range.

-- >8 --

diff --git a/quote.c b/quote.c
index ee7d62c..4f086fb 100644
--- a/quote.c
+++ b/quote.c

@@ -199,18 +199,32 @@ static int quote_c_style_counted(const c
 
 #define EMITQ() EMIT('\\')
 
-	const char *sp;
-	int ch, count = 0, needquote = 0;
+	const unsigned char *name_u = (const unsigned char *)name;
+	const unsigned char *sp;
+	int ch, count = 0, needquote = 0, has_high_ctrl = 0;
+
+	/* Check if we have control character in 0x80-0x9f range */
+	for (sp = name_u; sp < name_u + namelen; sp++) {
+		ch = *sp;
+		if (!ch)
+			break;
+		if ((ch < ' ') || (ch == '"') || (ch == '\\') ||
+		    (ch == 0177) || (ch == 0377))
+			needquote = 1;
+		else if (0x80 <= ch && ch <= 0x9f)
+			needquote = has_high_ctrl = 1;
+	}
 
 	if (!no_dq)
 		EMIT('"');
-	for (sp = name; sp < name + namelen; sp++) {
+
+	for (sp = name_u; sp < name_u + namelen; sp++) {
 		ch = *sp;
 		if (!ch)
 			break;
 		if ((ch < ' ') || (ch == '"') || (ch == '\\') ||
-		    (ch == 0177) || (ch == 0377)) {
-			needquote = 1;
+		    (ch == 0177) ||
+		    (has_high_ctrl && 0x80 <= ch)) {
 			switch (ch) {
 			case '\a': EMITQ(); ch = 'a'; break;
 			case '\b': EMITQ(); ch = 'b'; break;
-- 8< --

1. Current version doesn't display empty patches (i.e. pure rename and 
mode change combinations) and doesn't provide links to them from 
difftree. This is legacy of old /usr/bin/diff using code, which did not 
generated extended diff header, which is only output for "empty 
patches". Should we change this, or leave as is?

I think this needs to be fixed.

2. Schould we change syntax highlighting of chunk header line, namely 
changing slightly syntax coloring of "in which function are we" part of 
chunk header?

Probably matching "git diff --color" would be sensible; by
following what has already been done, you do not have to think
about what to color and how yourself.

3. Should we make from-range/to-range in chunk header hyperlink to the 
start of given bunch of lines in appropriate file? Or perhaps to the 
middle of the bunch of lines? Or to first changed line (omitting 
context)?

I do not see what usage pattern this link would help.  Care to
explain a bit better?

Junio C Hamano wrote:

Jakub Narebski [off-list ref] writes:

quoted

3. Should we make from-range/to-range in chunk header hyperlink to the 
start of given bunch of lines in appropriate file? Or perhaps to the 
middle of the bunch of lines? Or to first changed line (omitting 
context)?

I do not see what usage pattern this link would help.  Care to
explain a bit better?

For example for the following header

diff --git _a/gitweb/gitweb.perl_ _b/gitweb/gitweb.perl_

index _cbab3c9_.._a5a140c_ 100755

--- a/_gitweb/gitweb.perl_
+++ b/_gitweb/gitweb.perl_

@@ _-1255,9_ _+1255,12_ @@ sub parse_difftree_raw_line {

The '-1255,9' would be link to older version of gitweb/gitweb.perl file,
directly to the line #1255 (or to first changed line, or to the middle
of the chunk i.e. 1255+9/2 line).

The '+1255,12' would be link to newer version of file, to line 1255.
-- 
Jakub Narebski