[PATCH 2/3] diff.c: remove unnecessary typecast

[PATCH 2/3] diff.c: remove unnecessary typecast

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 diff.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/diff.c b/diff.c
index 8b3a3db..5bdc3c0 100644
--- a/diff.c
+++ b/diff.c
@@ -717,8 +717,8 @@ static void emit_rewrite_diff(const char *name_a,
 	ecbdata.opt = o;
 	if (ecbdata.ws_rule & WS_BLANK_AT_EOF) {
 		mmfile_t mf1, mf2;
-		mf1.ptr = (char *)data_one;
-		mf2.ptr = (char *)data_two;
+		mf1.ptr = data_one;
+		mf2.ptr = data_two;
 		mf1.size = size_one;
 		mf2.size = size_two;
 		check_blank_at_eof(&mf1, &mf2, &ecbdata);
-- 
2.7.1.532.gd9e3aaa

[PATCH 3/3] Correct conditions to free textconv result data

[Nguyễn Thái Ngọc Duy]

fill_textconv() have four code paths to return a new buffer. Two of
them, run_textconv() and notes_cache_get(), return a newly allocated
buffer. The other two return either a constant string or an existing
buffer (mmfile_t). We can only free the result buffer if it's allocated
by fill_textconv(). Correct all call sites.

Changes in combine-diff.c are not clear from diff output. We need to
force not running fill_textconv() unless the function returns a new
buffer so that it falls to the "else" case and allocates a new buffer
with no conversion.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Frankly I don't like the way fill_textconv() returns buffer at all.
 But I don't know much about textconv, or want to spend more time on
 it. This patch can be dropped if people come up with a better one.

 builtin/blame.c    | 2 +-
 combine-diff.c     | 4 ++--
 diff.c             | 8 ++++----
 diffcore-pickaxe.c | 5 +++--
 4 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index cb6f2fb..b5477ad 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -166,7 +166,7 @@ int textconv_object(const char *path,
 	df = alloc_filespec(path);
 	fill_filespec(df, sha1, sha1_valid, mode);
 	textconv = get_textconv(df);
-	if (!textconv) {
+	if (!textconv || !textconv->textconv) {
 		free_filespec(df);
 		return 0;
 	}
diff --git a/combine-diff.c b/combine-diff.c
index 5571304..c57cefd 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -299,7 +299,7 @@ static char *grab_blob(const struct object_id *oid, unsigned int mode,
 		/* deleted blob */
 		*size = 0;
 		return xcalloc(1, 1);
-	} else if (textconv) {
+	} else if (textconv && textconv->textconv) {
 		struct diff_filespec *df = alloc_filespec(path);
 		fill_filespec(df, oid->hash, 1, mode);
 		*size = fill_textconv(textconv, df, &blob);
@@ -1022,7 +1022,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 			else
 				result = grab_blob(&oid, elem->mode,
 						   &result_size, NULL, NULL);
-		} else if (textconv) {
+		} else if (textconv && textconv->textconv) {
 			struct diff_filespec *df = alloc_filespec(elem->path);
 			fill_filespec(df, null_sha1, 0, st.st_mode);
 			result_size = fill_textconv(textconv, df, &result);
diff --git a/diff.c b/diff.c
index 5bdc3c0..173ec5b 100644
--- a/diff.c
+++ b/diff.c
@@ -744,9 +744,9 @@ static void emit_rewrite_diff(const char *name_a,
 		emit_rewrite_lines(&ecbdata, '-', data_one, size_one);
 	if (lc_b)
 		emit_rewrite_lines(&ecbdata, '+', data_two, size_two);
-	if (textconv_one)
+	if (textconv_one && textconv_one->textconv)
 		free((char *)data_one);
-	if (textconv_two)
+	if (textconv_two && textconv_two->textconv)
 		free((char *)data_two);
 }
 
@@ -2454,9 +2454,9 @@ static void builtin_diff(const char *name_a,
 			die("unable to generate diff for %s", one->path);
 		if (o->word_diff)
 			free_diff_words_data(&ecbdata);
-		if (textconv_one)
+		if (textconv_one && textconv_one->textconv)
 			free(mf1.ptr);
-		if (textconv_two)
+		if (textconv_two && textconv_two->textconv)
 			free(mf2.ptr);
 		xdiff_clear_find_func(&xecfg);
 	}
diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index 7715c13..f89e106 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -7,6 +7,7 @@
 #include "diffcore.h"
 #include "xdiff-interface.h"
 #include "kwset.h"
+#include "userdiff.h"
 
 typedef int (*pickaxe_fn)(mmfile_t *one, mmfile_t *two,
 			  struct diff_options *o,
@@ -150,9 +151,9 @@ static int pickaxe_match(struct diff_filepair *p, struct diff_options *o,
 		 DIFF_FILE_VALID(p->two) ? &mf2 : NULL,
 		 o, regexp, kws);
 
-	if (textconv_one)
+	if (textconv_one && textconv_one->textconv)
 		free(mf1.ptr);
-	if (textconv_two)
+	if (textconv_two && textconv_two->textconv)
 		free(mf2.ptr);
 	diff_free_filespec_data(p->one);
 	diff_free_filespec_data(p->two);
-- 
2.7.1.532.gd9e3aaa

Re: [PATCH 3/3] Correct conditions to free textconv result data

[Duy Nguyen]

On Mon, Feb 22, 2016 at 7:52 PM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> fill_textconv() have four code paths to return a new buffer.

I forgot to add. Thanks to -Wwrite-strings pointing to '*outbuf = ""'
line in this function. I wouldn't notice otherwise.
-- 
Duy

Re: [PATCH 3/3] Correct conditions to free textconv result data

[Jeff King]

On Mon, Feb 22, 2016 at 07:52:25PM +0700, Nguyễn Thái Ngọc Duy wrote:

> fill_textconv() have four code paths to return a new buffer. Two of
> them, run_textconv() and notes_cache_get(), return a newly allocated
> buffer. The other two return either a constant string or an existing
> buffer (mmfile_t). We can only free the result buffer if it's allocated
> by fill_textconv(). Correct all call sites.

Right, and those two cases (allocated or not) should follow based on
whether you passed in a textconv-enabled driver.

> diff --git a/builtin/blame.c b/builtin/blame.c
> index cb6f2fb..b5477ad 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -166,7 +166,7 @@ int textconv_object(const char *path,
>  	df = alloc_filespec(path);
>  	fill_filespec(df, sha1, sha1_valid, mode);
>  	textconv = get_textconv(df);
> -	if (!textconv) {
> +	if (!textconv || !textconv->textconv) {
>  		free_filespec(df);
>  		return 0;
>  	}

This change (and the other similar ones) doesn't make any sense to me.
The point of get_textconv() is to return the userdiff driver if and only
if it has textconv enabled.

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

  if (!driver || !driver->textconv)

to decide whether to allocate the textconv buffer. The latter half of
that is really just defensive programming, and I think this would
probably be better as:

  if (driver)
	....

  assert(driver->textconv);

to make it more clear that we assume the parameter came from
get_textconv().

And if there's a case that triggers that assert(), then I think the bug
is in the caller, which should be fixed.

Is there a case I'm missing here where we actually leak memory or try to
free non-allocated memory? I didn't see it.

-Peff

Re: [PATCH 3/3] Correct conditions to free textconv result data

[Jeff King]

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

> Is there a case I'm missing here where we actually leak memory or try to
> free non-allocated memory? I didn't see it.

By the way, I saw only patches 2/3 and 3/3 on the list. So maybe there
is something interesting going on in 1/3, or in a cover letter that
didn't make it. :)

-Peff

Re: [PATCH 3/3] Correct conditions to free textconv result data

[Duy Nguyen]

On Tue, Feb 23, 2016 at 1:12 AM, Jeff King <peff@peff.net> wrote:
> On Mon, Feb 22, 2016 at 01:06:46PM -0500, Jeff King wrote:
>
>> Is there a case I'm missing here where we actually leak memory or try to
>> free non-allocated memory? I didn't see it.
>
> By the way, I saw only patches 2/3 and 3/3 on the list. So maybe there
> is something interesting going on in 1/3, or in a cover letter that
> didn't make it. :)

The only thing common in this series is it's the result of
-Wwrite-strings. 1/3 changes some "char *" to "const char *", you
don't miss anything.
-- 
Duy

Re: [PATCH 3/3] Correct conditions to free textconv result data

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Tue, Feb 23, 2016 at 1:12 AM, Jeff King <peff@peff.net> wrote:
>> On Mon, Feb 22, 2016 at 01:06:46PM -0500, Jeff King wrote:
>>
>>> Is there a case I'm missing here where we actually leak memory or try to
>>> free non-allocated memory? I didn't see it.
>>
>> By the way, I saw only patches 2/3 and 3/3 on the list. So maybe there
>> is something interesting going on in 1/3, or in a cover letter that
>> didn't make it. :)
>
> The only thing common in this series is it's the result of
> -Wwrite-strings. 1/3 changes some "char *" to "const char *", you
> don't miss anything.

While reading Peff's clarification patch, I did find the assignment
of "" to the *out pointer disturbing.  That part of your patch (I
presume that is what you mean by the above) may want to be revived.

Thanks.

Re: [PATCH 3/3] Correct conditions to free textconv result data

[Jeff King]

On Mon, Feb 22, 2016 at 03:08:01PM -0800, Junio C Hamano wrote:

> > The only thing common in this series is it's the result of
> > -Wwrite-strings. 1/3 changes some "char *" to "const char *", you
> > don't miss anything.
> 
> While reading Peff's clarification patch, I did find the assignment
> of "" to the *out pointer disturbing.  That part of your patch (I
> presume that is what you mean by the above) may want to be revived.

That part was inherited from diff_populate_filespec, though it casts
explicitly to "char *" since d2543b8 (Clean up diff.c, 2006-06-24).
I guess we could do the same here (not that it makes things any safer,
but at least tells everyone "yes, I _know_ this is weird".

-Peff

[PATCH] diff: clarify textconv interface

[Jeff King]

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

Re: [PATCH] diff: clarify textconv interface

[Duy Nguyen]

On Tue, Feb 23, 2016 at 1:28 AM, Jeff King <peff@peff.net> wrote:
> 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.

I almost went this way but I wasn't sure if driver->textconv can be
NULL on purpose. Definitely an improvement.
-- 
Duy