Re: [PATCH v11 06/10] convert: add 'working-tree-encoding' attribute

On 18 Mar 2018, at 08:24, Torsten Bögershausen [off-list ref] wrote:

Some comments inline

On Fri, Mar 09, 2018 at 06:35:32PM +0100, lars.schneider@autodesk.com wrote:

quoted

From: Lars Schneider <redacted>

Git recognizes files encoded with ASCII or one of its supersets (e.g.
UTF-8 or ISO-8859-1) as text files. All other encodings are usually
interpreted as binary and consequently built-in Git text processing
tools (e.g. 'git diff') as well as most Git web front ends do not
visualize the content.

Add an attribute to tell Git what encoding the user has defined for a
given file. If the content is added to the index, then Git converts the

Minor comment:
"Git converts the content"
Everywhere else (?) "encodes or reencodes" is used.
"Git reencodes the content" may be more consistent.

OK, will change.

quoted

+static const char *default_encoding = "UTF-8";
+
+static int encode_to_git(const char *path, const char *src, size_t src_len,
+			 struct strbuf *buf, const char *enc, int conv_flags)
+{
+	char *dst;
+	int dst_len;
+	int die_on_error = conv_flags & CONV_WRITE_OBJECT;
+
+	/*
+	 * No encoding is specified or there is nothing to encode.
+	 * Tell the caller that the content was not modified.
+	 */
+	if (!enc || (src && !src_len))
+		return 0;

(This may have been discussed before.
As we checked (enc != NULL) I think we can add here:)
	if (is_encoding_utf8(enc))
		return 0;

This should be covered in git_path_check_encoding(),
introduced in v12:

        /* Don't encode to the default encoding */
	if (same_encoding(value, default_encoding))
		return NULL;

In that function the encoding of a certain file is read from
the .gitattributes. If the encoding matches the compile-time
defined default encoding (= UTF-8), then the encoding is set
to NULL.

quoted

+
+static int encode_to_worktree(const char *path, const char *src, size_t src_len,
+			      struct strbuf *buf, const char *enc)
+{
+	char *dst;
+	int dst_len;
+
+	/*
+	 * No encoding is specified or there is nothing to encode.
+	 * Tell the caller that the content was not modified.
+	 */
+	if (!enc || (src && !src_len))
+		return 0;

Same as above:
	if (is_encoding_utf8(enc))
		return 0;

quoted

+
+	dst = reencode_string_len(src, src_len, enc, default_encoding,
+				  &dst_len);
+	if (!dst) {
+		error("failed to encode '%s' from %s to %s",
+			path, default_encoding, enc);
+		return 0;
+	}
+
+	strbuf_attach(buf, dst, dst_len, dst_len + 1);
+	return 1;
+}
+
static int crlf_to_git(const struct index_state *istate,
		       const char *path, const char *src, size_t len,
		       struct strbuf *buf,

@@ -978,6 +1051,25 @@ static int ident_to_worktree(const char *path, const char *src, size_t len,

	return 1;
}

+static const char *git_path_check_encoding(struct attr_check_item *check)
+{
+	const char *value = check->value;
+
+	if (ATTR_UNSET(value) || !strlen(value))
+		return NULL;
+

quoted

+	if (ATTR_TRUE(value) || ATTR_FALSE(value)) {
+		error(_("working-tree-encoding attribute requires a value"));
+		return NULL;
+	}

TRUE or false are values, but just wrong ones.
If this test is removed, the user will see "failed to encode "TRUE" to "UTF-8",
which should give enough information to fix it.

I see your point. However, I would like to stop the processing right
there for these invalid values. How about 

  error(_("true/false are no valid working-tree-encodings"));

I think that is the most straight forward/helpful error message
for the enduser (I consider the term "boolean" but dismissed it
as potentially confusing to folks not familiar with the term).

OK with you?

quoted

+
+	/* Don't encode to the default encoding */
+	if (!strcasecmp(value, default_encoding))
+		return NULL;

Same as above ?:
	if (is_encoding_utf8(value))
		return 0;

Yes, that was fixed in v12 as mentioned above :-)

- Lars