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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Jakub Narebski <jnareb@xxxxxxxxx> 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: <7v3b972bzq.fsf@xxxxxxxxxxxxxxxxxxxxxxxx>

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?

-
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

[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]