{"thread":{"id":"41471","subject":"[PATCH 2/3] diff.c: remove unnecessary typecast","startedAt":"2016-02-22T12:52:24Z","lastAt":"2016-02-22T23:25:44Z","messageCount":10,"participants":["Nguyễn Thái Ngọc Duy","Duy Nguyen","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"278878","messageId":"1456145545-5374-2-git-send-email-pclouds@gmail.com","threadId":"41471","inReplyTo":"1456145545-5374-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 2/3] diff.c: remove unnecessary typecast","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2016-02-22T12:52:24Z","receivedAt":"2016-02-22T12:52:24Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Signed-off-by: Nguyễn Thái Ngọc Duy \n---\n diff.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 8b3a3db..5bdc3c0 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -717,8 +717,8 @@ static void emit_rewrite_diff(const char *name_a,\n \tecbdata.opt = o;\n \tif (ecbdata.ws_rule & WS_BLANK_AT_EOF) {\n \t\tmmfile_t mf1, mf2;\n-\t\tmf1.ptr = (char *)data_one;\n-\t\tmf2.ptr = (char *)data_two;\n+\t\tmf1.ptr = data_one;\n+\t\tmf2.ptr = data_two;\n \t\tmf1.size = size_one;\n \t\tmf2.size = size_two;\n \t\tcheck_blank_at_eof(&mf1, &mf2, &ecbdata);\n-- \n2.7.1.532.gd9e3aaa\n"},{"id":"278879","messageId":"1456145545-5374-3-git-send-email-pclouds@gmail.com","threadId":"41471","inReplyTo":"1456145545-5374-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 3/3] Correct conditions to free textconv result data","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2016-02-22T12:52:25Z","receivedAt":"2016-02-22T12:52:25Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"fill_textconv() have four code paths to return a new buffer. Two of\nthem, run_textconv() and notes_cache_get(), return a newly allocated\nbuffer. The other two return either a constant string or an existing\nbuffer (mmfile_t). We can only free the result buffer if it's allocated\nby fill_textconv(). Correct all call sites.\n\nChanges in combine-diff.c are not clear from diff output. We need to\nforce not running fill_textconv() unless the function returns a new\nbuffer so that it falls to the \"else\" case and allocates a new buffer\nwith no conversion.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n Frankly I don't like the way fill_textconv() returns buffer at all.\n But I don't know much about textconv, or want to spend more time on\n it. This patch can be dropped if people come up with a better one.\n\n builtin/blame.c | 2 +-\n combine-diff.c | 4 ++--\n diff.c | 8 ++++----\n diffcore-pickaxe.c | 5 +++--\n 4 files changed, 10 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex cb6f2fb..b5477ad 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -166,7 +166,7 @@ int textconv_object(const char *path,\n \tdf = alloc_filespec(path);\n \tfill_filespec(df, sha1, sha1_valid, mode);\n \ttextconv = get_textconv(df);\n-\tif (!textconv) {\n+\tif (!textconv || !textconv->textconv) {\n \t\tfree_filespec(df);\n \t\treturn 0;\n \t}\ndiff --git a/combine-diff.c b/combine-diff.c\nindex 5571304..c57cefd 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -299,7 +299,7 @@ static char *grab_blob(const struct object_id *oid, unsigned int mode,\n \t\t/* deleted blob */\n \t\t*size = 0;\n \t\treturn xcalloc(1, 1);\n-\t} else if (textconv) {\n+\t} else if (textconv && textconv->textconv) {\n \t\tstruct diff_filespec *df = alloc_filespec(path);\n \t\tfill_filespec(df, oid->hash, 1, mode);\n \t\t*size = fill_textconv(textconv, df, &blob);\n@@ -1022,7 +1022,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \t\t\telse\n \t\t\t\tresult = grab_blob(&oid, elem->mode,\n \t\t\t\t\t\t &result_size, NULL, NULL);\n-\t\t} else if (textconv) {\n+\t\t} else if (textconv && textconv->textconv) {\n \t\t\tstruct diff_filespec *df = alloc_filespec(elem->path);\n \t\t\tfill_filespec(df, null_sha1, 0, st.st_mode);\n \t\t\tresult_size = fill_textconv(textconv, df, &result);\ndiff --git a/diff.c b/diff.c\nindex 5bdc3c0..173ec5b 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -744,9 +744,9 @@ static void emit_rewrite_diff(const char *name_a,\n \t\temit_rewrite_lines(&ecbdata, '-', data_one, size_one);\n \tif (lc_b)\n \t\temit_rewrite_lines(&ecbdata, '+', data_two, size_two);\n-\tif (textconv_one)\n+\tif (textconv_one && textconv_one->textconv)\n \t\tfree((char *)data_one);\n-\tif (textconv_two)\n+\tif (textconv_two && textconv_two->textconv)\n \t\tfree((char *)data_two);\n }\n \n@@ -2454,9 +2454,9 @@ static void builtin_diff(const char *name_a,\n \t\t\tdie(\"unable to generate diff for %s\", one->path);\n \t\tif (o->word_diff)\n \t\t\tfree_diff_words_data(&ecbdata);\n-\t\tif (textconv_one)\n+\t\tif (textconv_one && textconv_one->textconv)\n \t\t\tfree(mf1.ptr);\n-\t\tif (textconv_two)\n+\t\tif (textconv_two && textconv_two->textconv)\n \t\t\tfree(mf2.ptr);\n \t\txdiff_clear_find_func(&xecfg);\n \t}\ndiff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c\nindex 7715c13..f89e106 100644\n--- a/diffcore-pickaxe.c\n+++ b/diffcore-pickaxe.c\n@@ -7,6 +7,7 @@\n #include \"diffcore.h\"\n #include \"xdiff-interface.h\"\n #include \"kwset.h\"\n+#include \"userdiff.h\"\n \n typedef int (*pickaxe_fn)(mmfile_t *one, mmfile_t *two,\n \t\t\t struct diff_options *o,\n@@ -150,9 +151,9 @@ static int pickaxe_match(struct diff_filepair *p, struct diff_options *o,\n \t\t DIFF_FILE_VALID(p->two) ? &mf2 : NULL,\n \t\t o, regexp, kws);\n \n-\tif (textconv_one)\n+\tif (textconv_one && textconv_one->textconv)\n \t\tfree(mf1.ptr);\n-\tif (textconv_two)\n+\tif (textconv_two && textconv_two->textconv)\n \t\tfree(mf2.ptr);\n \tdiff_free_filespec_data(p->one);\n \tdiff_free_filespec_data(p->two);\n-- \n2.7.1.532.gd9e3aaa\n"},{"id":"278880","messageId":"CACsJy8D-2kuBRhiMEVTeHOz2V=wE20pwd=jL5L=x0gHjA_OWMw@mail.gmail.com","threadId":"41471","inReplyTo":"1456145545-5374-3-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 3/3] Correct conditions to free textconv result data","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-02-22T13:00:18Z","receivedAt":"2016-02-22T13:00:18Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Feb 22, 2016 at 7:52 PM, Nguyễn Thái Ngọc Duy wrote:\n> fill_textconv() have four code paths to return a new buffer.\n\nI forgot to add. Thanks to -Wwrite-strings pointing to '*outbuf = \"\"'\nline in this function. I wouldn't notice otherwise.\n-- \nDuy\n"},{"id":"278890","messageId":"20160222180645.GB4587@sigill.intra.peff.net","threadId":"41471","inReplyTo":"1456145545-5374-3-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 3/3] Correct conditions to free textconv result data","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-22T18:06:46Z","receivedAt":"2016-02-22T18:06:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 22, 2016 at 07:52:25PM +0700, Nguyễn Thái Ngọc Duy wrote:\n\n> fill_textconv() have four code paths to return a new buffer. Two of\n> them, run_textconv() and notes_cache_get(), return a newly allocated\n> buffer. The other two return either a constant string or an existing\n> buffer (mmfile_t). We can only free the result buffer if it's allocated\n> by fill_textconv(). Correct all call sites.\n\nRight, and those two cases (allocated or not) should follow based on\nwhether you passed in a textconv-enabled driver.\n\n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index cb6f2fb..b5477ad 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -166,7 +166,7 @@ int textconv_object(const char *path,\n> \tdf = alloc_filespec(path);\n> \tfill_filespec(df, sha1, sha1_valid, mode);\n> \ttextconv = get_textconv(df);\n> -\tif (!textconv) {\n> +\tif (!textconv || !textconv->textconv) {\n> \t\tfree_filespec(df);\n> \t\treturn 0;\n> \t}\n\nThis change (and the other similar ones) doesn't make any sense to me.\nThe point of get_textconv() is to return the userdiff driver if and only\nif it has textconv enabled.\n\nI have a feeling you were confused by the fact that fill_textconv()\ndoes:\n\n if (!driver || !driver->textconv)\n\nto decide whether to allocate the textconv buffer. The latter half of\nthat is really just defensive programming, and I think this would\nprobably be better as:\n\n if (driver)\n\t....\n\n assert(driver->textconv);\n\nto make it more clear that we assume the parameter came from\nget_textconv().\n\nAnd if there's a case that triggers that assert(), then I think the bug\nis in the caller, which should be fixed.\n\nIs there a case I'm missing here where we actually leak memory or try to\nfree non-allocated memory? I didn't see it.\n\n-Peff\n"},{"id":"278891","messageId":"20160222181224.GA4917@sigill.intra.peff.net","threadId":"41471","inReplyTo":"20160222180645.GB4587@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] Correct conditions to free textconv result data","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-22T18:12:24Z","receivedAt":"2016-02-22T18:12:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 22, 2016 at 01:06:46PM -0500, Jeff King wrote:\n\n> Is there a case I'm missing here where we actually leak memory or try to\n> free non-allocated memory? I didn't see it.\n\nBy the way, I saw only patches 2/3 and 3/3 on the list. So maybe there\nis something interesting going on in 1/3, or in a cover letter that\ndidn't make it. :)\n\n-Peff\n"},{"id":"278892","messageId":"20160222182854.GA11732@sigill.intra.peff.net","threadId":"41471","inReplyTo":"20160222180645.GB4587@sigill.intra.peff.net","subject":"[PATCH] diff: clarify textconv interface","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-22T18:28:54Z","receivedAt":"2016-02-22T18:28:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 22, 2016 at 01:06:45PM -0500, Jeff King wrote:\n\n> I have a feeling you were confused by the fact that fill_textconv()\n> does:\n\nLooking over it, I agree this is a pretty confusing interface that grew\nout of control over time. But refactoring it is kind of tricky, because\nwe really do want to avoid extra allocations, or cross-module\nassumptions (e.g., userdiff doesn't know about diff_filespec, but rather\nthe other way around, and we probably do not want to muck with the\ninternals of a diff_filespec when doing a textconv).\n\nSo I think the patch below is an improvement, but if somebody really\nwants to dig into refactoring it, be my guest.\n\n-- >8 --\nSubject: [PATCH] diff: clarify textconv interface\n\nThe memory allocation scheme for the textconv interface is a\nbit tricky, and not well documented. It was originally\ndesigned as an internal part of diff.c (matching\nfill_mmfile), but gradually was made public.\n\nRefactoring it is difficult, but we can at least improve the\nsituation by documenting the intended flow and enforcing it\nwith an in-code assertion.\n\nSigned-off-by: Jeff King \n---\n diff.c | 5 ++++-\n diff.h | 16 ++++++++++++++++\n userdiff.h | 4 ++++\n 3 files changed, 24 insertions(+), 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex 2136b69..a088e26 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -5085,7 +5085,7 @@ size_t fill_textconv(struct userdiff_driver *driver,\n {\n \tsize_t size;\n \n-\tif (!driver || !driver->textconv) {\n+\tif (!driver) {\n \t\tif (!DIFF_FILE_VALID(df)) {\n \t\t\t*outbuf = \"\";\n \t\t\treturn 0;\n@@ -5096,6 +5096,9 @@ size_t fill_textconv(struct userdiff_driver *driver,\n \t\treturn df->size;\n \t}\n \n+\tif (!driver->textconv)\n+\t\tdie(\"BUG: fill_textconv called with non-textconv driver\");\n+\n \tif (driver->textconv_cache && df->sha1_valid) {\n \t\t*outbuf = notes_cache_get(driver->textconv_cache, df->sha1,\n \t\t\t\t\t &size);\ndiff --git a/diff.h b/diff.h\nindex 70b2d70..4505b4d 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -349,10 +349,26 @@ extern void diff_no_index(struct rev_info *, int, const char **);\n \n extern int index_differs_from(const char *def, int diff_flags);\n \n+/*\n+ * Fill the contents of the filespec \"df\", respecting any textconv defined by\n+ * its userdiff driver. The \"driver\" parameter must come from a\n+ * previous call to get_textconv(), and therefore should either be NULL or have\n+ * textconv enabled.\n+ *\n+ * Note that the memory ownership of the resulting buffer depends on whether\n+ * the driver field is NULL. If it is, then the memory belongs to the filespec\n+ * struct. If it is non-NULL, then \"outbuf\" points to a newly allocated buffer\n+ * that should be freed by the caller.\n+ */\n extern size_t fill_textconv(struct userdiff_driver *driver,\n \t\t\t struct diff_filespec *df,\n \t\t\t char **outbuf);\n \n+/*\n+ * Look up the userdiff driver for the given filespec, and return it if\n+ * and only if it has textconv enabled (otherwise return NULL). The result\n+ * can be passed to fill_textconv().\n+ */\n extern struct userdiff_driver *get_textconv(struct diff_filespec *one);\n \n extern int parse_rename_score(const char **cp_p);\ndiff --git a/userdiff.h b/userdiff.h\nindex 4a7e78f..2ef0ce5 100644\n--- a/userdiff.h\n+++ b/userdiff.h\n@@ -23,6 +23,10 @@ int userdiff_config(const char *k, const char *v);\n struct userdiff_driver *userdiff_find_by_name(const char *name);\n struct userdiff_driver *userdiff_find_by_path(const char *path);\n \n+/*\n+ * Initialize any textconv-related fields in the driver and return it, or NULL\n+ * if it does not have textconv enabled at all.\n+ */\n struct userdiff_driver *userdiff_get_textconv(struct userdiff_driver *driver);\n \n #endif /* USERDIFF */\n-- \n2.7.1.652.g2fdcad6\n"},{"id":"278946","messageId":"CACsJy8Cg4cgCEqwo=9ujdTV8aw5CO-irVxOCanzBG4n7Nm-0kg@mail.gmail.com","threadId":"41471","inReplyTo":"20160222182854.GA11732@sigill.intra.peff.net","subject":"Re: [PATCH] diff: clarify textconv interface","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-02-22T23:01:47Z","receivedAt":"2016-02-22T23:01:47Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Feb 23, 2016 at 1:28 AM, Jeff King wrote:\n> On Mon, Feb 22, 2016 at 01:06:45PM -0500, Jeff King wrote:\n>\n>> I have a feeling you were confused by the fact that fill_textconv()\n>> does:\n>\n> Looking over it, I agree this is a pretty confusing interface that grew\n> out of control over time. But refactoring it is kind of tricky, because\n> we really do want to avoid extra allocations, or cross-module\n> assumptions (e.g., userdiff doesn't know about diff_filespec, but rather\n> the other way around, and we probably do not want to muck with the\n> internals of a diff_filespec when doing a textconv).\n>\n> So I think the patch below is an improvement, but if somebody really\n> wants to dig into refactoring it, be my guest.\n\nI almost went this way but I wasn't sure if driver->textconv can be\nNULL on purpose. Definitely an improvement.\n-- \nDuy\n"},{"id":"278947","messageId":"CACsJy8BWgDFq_Dv=omkpDBa_pUF7zKr9cFftNL-KxtMcTkc5-w@mail.gmail.com","threadId":"41471","inReplyTo":"20160222181224.GA4917@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] Correct conditions to free textconv result data","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-02-22T23:03:59Z","receivedAt":"2016-02-22T23:03:59Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Feb 23, 2016 at 1:12 AM, Jeff King wrote:\n> On Mon, Feb 22, 2016 at 01:06:46PM -0500, Jeff King wrote:\n>\n>> Is there a case I'm missing here where we actually leak memory or try to\n>> free non-allocated memory? I didn't see it.\n>\n> By the way, I saw only patches 2/3 and 3/3 on the list. So maybe there\n> is something interesting going on in 1/3, or in a cover letter that\n> didn't make it. :)\n\nThe only thing common in this series is it's the result of\n-Wwrite-strings. 1/3 changes some \"char *\" to \"const char *\", you\ndon't miss anything.\n-- \nDuy\n"},{"id":"278949","messageId":"xmqqfuwk4a3y.fsf@gitster.mtv.corp.google.com","threadId":"41471","inReplyTo":"CACsJy8BWgDFq_Dv=omkpDBa_pUF7zKr9cFftNL-KxtMcTkc5-w@mail.gmail.com","subject":"Re: [PATCH 3/3] Correct conditions to free textconv result data","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-22T23:08:01Z","receivedAt":"2016-02-22T23:08:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> On Tue, Feb 23, 2016 at 1:12 AM, Jeff King wrote:\n>> On Mon, Feb 22, 2016 at 01:06:46PM -0500, Jeff King wrote:\n>>\n>>> Is there a case I'm missing here where we actually leak memory or try to\n>>> free non-allocated memory? I didn't see it.\n>>\n>> By the way, I saw only patches 2/3 and 3/3 on the list. So maybe there\n>> is something interesting going on in 1/3, or in a cover letter that\n>> didn't make it. :)\n>\n> The only thing common in this series is it's the result of\n> -Wwrite-strings. 1/3 changes some \"char *\" to \"const char *\", you\n> don't miss anything.\n\nWhile reading Peff's clarification patch, I did find the assignment\nof \"\" to the *out pointer disturbing. That part of your patch (I\npresume that is what you mean by the above) may want to be revived.\n\nThanks.\n"},{"id":"278952","messageId":"20160222232544.GA10362@sigill.intra.peff.net","threadId":"41471","inReplyTo":"xmqqfuwk4a3y.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/3] Correct conditions to free textconv result data","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-22T23:25:44Z","receivedAt":"2016-02-22T23:25:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 22, 2016 at 03:08:01PM -0800, Junio C Hamano wrote:\n\n> > The only thing common in this series is it's the result of\n> > -Wwrite-strings. 1/3 changes some \"char *\" to \"const char *\", you\n> > don't miss anything.\n> \n> While reading Peff's clarification patch, I did find the assignment\n> of \"\" to the *out pointer disturbing. That part of your patch (I\n> presume that is what you mean by the above) may want to be revived.\n\nThat part was inherited from diff_populate_filespec, though it casts\nexplicitly to \"char *\" since d2543b8 (Clean up diff.c, 2006-06-24).\nI guess we could do the same here (not that it makes things any safer,\nbut at least tells everyone \"yes, I _know_ this is weird\".\n\n-Peff\n"}]}