[PATCH] diff: clarify textconv interface

[Jeff King <peff@peff.net>]

On Mon, Feb 22, 2016 at 01:06:45PM -0500, Jeff King wrote:

> I have a feeling you were confused by the fact that fill_textconv()
> does:

Looking over it, I agree this is a pretty confusing interface that grew
out of control over time.  But refactoring it is kind of tricky, because
we really do want to avoid extra allocations, or cross-module
assumptions (e.g., userdiff doesn't know about diff_filespec, but rather
the other way around, and we probably do not want to muck with the
internals of a diff_filespec when doing a textconv).

So I think the patch below is an improvement, but if somebody really
wants to dig into refactoring it, be my guest.

-- >8 --
Subject: [PATCH] diff: clarify textconv interface

The memory allocation scheme for the textconv interface is a
bit tricky, and not well documented. It was originally
designed as an internal part of diff.c (matching
fill_mmfile), but gradually was made public.

Refactoring it is difficult, but we can at least improve the
situation by documenting the intended flow and enforcing it
with an in-code assertion.

Signed-off-by: Jeff King <peff@peff.net>
---
 diff.c     |  5 ++++-
 diff.h     | 16 ++++++++++++++++
 userdiff.h |  4 ++++
 3 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/diff.c b/diff.c
index 2136b69..a088e26 100644
--- a/diff.c
+++ b/diff.c
@@ -5085,7 +5085,7 @@ size_t fill_textconv(struct userdiff_driver *driver,
 {
 	size_t size;
 
-	if (!driver || !driver->textconv) {
+	if (!driver) {
 		if (!DIFF_FILE_VALID(df)) {
 			*outbuf = "";
 			return 0;
@@ -5096,6 +5096,9 @@ size_t fill_textconv(struct userdiff_driver *driver,
 		return df->size;
 	}
 
+	if (!driver->textconv)
+		die("BUG: fill_textconv called with non-textconv driver");
+
 	if (driver->textconv_cache && df->sha1_valid) {
 		*outbuf = notes_cache_get(driver->textconv_cache, df->sha1,
 					  &size);
diff --git a/diff.h b/diff.h
index 70b2d70..4505b4d 100644
--- a/diff.h
+++ b/diff.h
@@ -349,10 +349,26 @@ extern void diff_no_index(struct rev_info *, int, const char **);
 
 extern int index_differs_from(const char *def, int diff_flags);
 
+/*
+ * Fill the contents of the filespec "df", respecting any textconv defined by
+ * its userdiff driver.  The "driver" parameter must come from a
+ * previous call to get_textconv(), and therefore should either be NULL or have
+ * textconv enabled.
+ *
+ * Note that the memory ownership of the resulting buffer depends on whether
+ * the driver field is NULL. If it is, then the memory belongs to the filespec
+ * struct. If it is non-NULL, then "outbuf" points to a newly allocated buffer
+ * that should be freed by the caller.
+ */
 extern size_t fill_textconv(struct userdiff_driver *driver,
 			    struct diff_filespec *df,
 			    char **outbuf);
 
+/*
+ * Look up the userdiff driver for the given filespec, and return it if
+ * and only if it has textconv enabled (otherwise return NULL). The result
+ * can be passed to fill_textconv().
+ */
 extern struct userdiff_driver *get_textconv(struct diff_filespec *one);
 
 extern int parse_rename_score(const char **cp_p);
diff --git a/userdiff.h b/userdiff.h
index 4a7e78f..2ef0ce5 100644
--- a/userdiff.h
+++ b/userdiff.h
@@ -23,6 +23,10 @@ int userdiff_config(const char *k, const char *v);
 struct userdiff_driver *userdiff_find_by_name(const char *name);
 struct userdiff_driver *userdiff_find_by_path(const char *path);
 
+/*
+ * Initialize any textconv-related fields in the driver and return it, or NULL
+ * if it does not have textconv enabled at all.
+ */
 struct userdiff_driver *userdiff_get_textconv(struct userdiff_driver *driver);
 
 #endif /* USERDIFF */
-- 
2.7.1.652.g2fdcad6