{"thread":{"id":"24031","subject":"[PATCH v2 0/3] textconv support for blame","startedAt":"2010-06-07T14:41:50Z","lastAt":"2010-06-15T15:00:06Z","messageCount":17,"participants":["Axel Bonnet","Junio C Hamano","Jeff King","Diane Gasselin","Clément Poulain","bonneta","Matthieu Moy"],"isPatch":true,"patchVersion":2,"patchTotal":3},"messages":[{"id":"143163","messageId":"1275921713-3277-1-git-send-email-axel.bonnet@ensimag.imag.fr","threadId":"24031","inReplyTo":null,"subject":"[PATCH v2 0/3] textconv support for blame","fromName":"Axel Bonnet","fromEmail":"axel.bonnet@ensimag.imag.fr","sentAt":"2010-06-07T14:41:50Z","receivedAt":"2010-06-07T14:41:50Z","isPatch":true,"sender":{"key":"axel.bonnet@ensimag.imag.fr","avatar":null},"body":"This is a patch series to implement textconv support for git blame.\nAs textconv support has already been added to git diff, so we use textconv methods of diff.\nHere are the different changes:\n- make the diff textconv API public\n- add diff_options to blame (--textconv and --no-textconv)\n- perform textconv when we meet an object with textconv driver\n- t8006-blame-textconv.sh tests conversion works\n\nAxel Bonnet (3):\n textconv: make the API public\n textconv: support for blame\n t/t8006: test textconv support for blame\n\n builtin/blame.c | 82 +++++++++++++++++++++++++++++++++++++--------\n diff.c | 12 ++----\n diff.h | 8 ++++\n t/t8006-blame-textconv.sh | 80 +++++++++++++++++++++++++++++++++++++++++++\n 4 files changed, 160 insertions(+), 22 deletions(-)\n create mode 100755 t/t8006-blame-textconv.sh\n"},{"id":"143168","messageId":"1275924218-20154-1-git-send-email-axel.bonnet@ensimag.imag.fr","threadId":"24031","inReplyTo":"1275921713-3277-1-git-send-email-axel.bonnet@ensimag.imag.fr","subject":"[PATCH v2 1/3] textconv: make the API public","fromName":"Axel Bonnet","fromEmail":"axel.bonnet@ensimag.imag.fr","sentAt":"2010-06-07T15:23:36Z","receivedAt":"2010-06-07T15:23:36Z","isPatch":true,"sender":{"key":"axel.bonnet@ensimag.imag.fr","avatar":null},"body":"The textconv functionality allows one to convert a file into text before\nrunning diff. But this functionality can be useful to other features\nsuch as blame.\n\nSigned-off-by: Axel Bonnet \nSigned-off-by: Clément Poulain \nSigned-off-by: Diane Gasselin \n---\n diff.c | 12 ++++--------\n diff.h | 8 ++++++++\n 2 files changed, 12 insertions(+), 8 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 494f560..b4a830f 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -43,10 +43,6 @@ static char diff_colors[][COLOR_MAXLEN] = {\n \tGIT_COLOR_NORMAL,\t/* FUNCINFO */\n };\n \n-static void diff_filespec_load_driver(struct diff_filespec *one);\n-static size_t fill_textconv(struct userdiff_driver *driver,\n-\t\t\t struct diff_filespec *df, char **outbuf);\n-\n static int parse_diff_color_slot(const char *var, int ofs)\n {\n \tif (!strcasecmp(var+ofs, \"plain\"))\n@@ -1629,7 +1625,7 @@ void diff_set_mnemonic_prefix(struct diff_options *options, const char *a, const\n \t\toptions->b_prefix = b;\n }\n \n-static struct userdiff_driver *get_textconv(struct diff_filespec *one)\n+struct userdiff_driver *get_textconv(struct diff_filespec *one)\n {\n \tif (!DIFF_FILE_VALID(one))\n \t\treturn NULL;\n@@ -4002,9 +3998,9 @@ static char *run_textconv(const char *pgm, struct diff_filespec *spec,\n \treturn strbuf_detach(&buf, outsize);\n }\n \n-static size_t fill_textconv(struct userdiff_driver *driver,\n-\t\t\t struct diff_filespec *df,\n-\t\t\t char **outbuf)\n+size_t fill_textconv(struct userdiff_driver *driver,\n+\t\t struct diff_filespec *df,\n+\t\t char **outbuf)\n {\n \tsize_t size;\n \ndiff --git a/diff.h b/diff.h\nindex 9ace08c..2a0e36d 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -9,6 +9,8 @@\n struct rev_info;\n struct diff_options;\n struct diff_queue_struct;\n+struct diff_filespec;\n+struct userdiff_driver;\n \n typedef void (*change_fn_t)(struct diff_options *options,\n \t\t unsigned old_mode, unsigned new_mode,\n@@ -287,4 +289,10 @@ extern void diff_no_index(struct rev_info *, int, const char **, int, const char\n \n extern int index_differs_from(const char *def, int diff_flags);\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+extern struct userdiff_driver *get_textconv(struct diff_filespec *one);\n+\n #endif /* DIFF_H */\n-- \n1.6.6.7.ga5fe3\n"},{"id":"143166","messageId":"1275924218-20154-2-git-send-email-axel.bonnet@ensimag.imag.fr","threadId":"24031","inReplyTo":"1275924218-20154-1-git-send-email-axel.bonnet@ensimag.imag.fr","subject":"[PATCH v2 2/3] textconv: support for blame","fromName":"Axel Bonnet","fromEmail":"axel.bonnet@ensimag.imag.fr","sentAt":"2010-06-07T15:23:37Z","receivedAt":"2010-06-07T15:23:37Z","isPatch":true,"sender":{"key":"axel.bonnet@ensimag.imag.fr","avatar":null},"body":"This patches enables to perform textconv with blame if a textconv driver is\navailable for the file.\n\nThe main task is performed by the textconv_object function which prepares\ndiff_filespec and if possible converts the file using diff textconv API.\nOnly regular files are converted, so the mode of diff_filespec is faked.\n\nTextconv conversion is enabled by default (equivalent to the option\n--textconv), since blaming binary files is useless in most cases.\nThe option --no-textconv is used to disable textconv conversion.\n\nThe declarations of several functions are modified to give access to a\ndiff_options, in order to know whether the textconv option is activated or not.\n\nSigned-off-by: Axel Bonnet \nSigned-off-by: Clément Poulain \nSigned-off-by: Diane Gasselin \n---\n builtin/blame.c | 82 +++++++++++++++++++++++++++++++++++++++++++++---------\n 1 files changed, 68 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex fc15863..f831e3a 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -20,6 +20,7 @@\n #include \"mailmap.h\"\n #include \"parse-options.h\"\n #include \"utf8.h\"\n+#include \"userdiff.h\"\n \n static char blame_usage[] = \"git blame [options] [rev-opts] [rev] [--] file\";\n \n@@ -86,16 +87,49 @@ struct origin {\n };\n \n /*\n+ * Prepare diff_filespec and convert it using diff textconv API\n+ * if the textconv driver exists.\n+ * Return 1 if the conversion succeeds, 0 otherwise.\n+ */\n+static int textconv_object(const char *path,\n+\t\t\t const unsigned char *sha1,\n+\t\t\t char **buf,\n+\t\t\t size_t *buf_size)\n+{\n+\tstruct diff_filespec *df;\n+\tstruct userdiff_driver *textconv;\n+\n+\tdf = alloc_filespec(path);\n+\tfill_filespec(df, sha1, S_IFREG | 0664);\n+\ttextconv = get_textconv(df);\n+\tif (!textconv) {\n+\t\tfree_filespec(df);\n+\t\treturn 0;\n+\t}\n+\n+\t*buf_size = fill_textconv(textconv, df, buf);\n+\tfree_filespec(df);\n+\treturn 1;\n+}\n+\n+/*\n * Given an origin, prepare mmfile_t structure to be used by the\n * diff machinery\n */\n-static void fill_origin_blob(struct origin *o, mmfile_t *file)\n+static void fill_origin_blob(struct diff_options *opt,\n+\t\t\t struct origin *o, mmfile_t *file)\n {\n \tif (!o->file.ptr) {\n \t\tenum object_type type;\n \t\tnum_read_blob++;\n-\t\tfile->ptr = read_sha1_file(o->blob_sha1, &type,\n-\t\t\t\t\t (unsigned long *)(&(file->size)));\n+\n+\t\tif (DIFF_OPT_TST(opt, ALLOW_TEXTCONV) &&\n+\t\t textconv_object(o->path, o->blob_sha1, &file->ptr,\n+\t\t\t\t (size_t *) &file->size))\n+\t\t\t;\n+\t\telse\n+\t\t\tfile->ptr = read_sha1_file(o->blob_sha1, &type,\n+\t\t\t\t\t\t (unsigned long *)(&(file->size)));\n \t\tif (!file->ptr)\n \t\t\tdie(\"Cannot read blob %s for path %s\",\n \t\t\t sha1_to_hex(o->blob_sha1),\n@@ -282,7 +316,6 @@ static struct origin *get_origin(struct scoreboard *sb,\n static int fill_blob_sha1(struct origin *origin)\n {\n \tunsigned mode;\n-\n \tif (!is_null_sha1(origin->blob_sha1))\n \t\treturn 0;\n \tif (get_tree_entry(origin->commit->object.sha1,\n@@ -741,8 +774,8 @@ static int pass_blame_to_parent(struct scoreboard *sb,\n \tif (last_in_target < 0)\n \t\treturn 1; /* nothing remains for this target */\n \n-\tfill_origin_blob(parent, &file_p);\n-\tfill_origin_blob(target, &file_o);\n+\tfill_origin_blob(&sb->revs->diffopt, parent, &file_p);\n+\tfill_origin_blob(&sb->revs->diffopt, target, &file_o);\n \tnum_get_patch++;\n \n \tmemset(&xpp, 0, sizeof(xpp));\n@@ -922,7 +955,7 @@ static int find_move_in_parent(struct scoreboard *sb,\n \tif (last_in_target < 0)\n \t\treturn 1; /* nothing remains for this target */\n \n-\tfill_origin_blob(parent, &file_p);\n+\tfill_origin_blob(&sb->revs->diffopt, parent, &file_p);\n \tif (!file_p.ptr)\n \t\treturn 0;\n \n@@ -1063,7 +1096,7 @@ static int find_copy_in_parent(struct scoreboard *sb,\n \n \t\t\tnorigin = get_origin(sb, parent, p->one->path);\n \t\t\thashcpy(norigin->blob_sha1, p->one->sha1);\n-\t\t\tfill_origin_blob(norigin, &file_p);\n+\t\t\tfill_origin_blob(&sb->revs->diffopt, norigin, &file_p);\n \t\t\tif (!file_p.ptr)\n \t\t\t\tcontinue;\n \n@@ -1983,6 +2016,13 @@ static int git_blame_config(const char *var, const char *value, void *cb)\n \t\tblame_date_mode = parse_date_format(value);\n \t\treturn 0;\n \t}\n+\n+\tswitch (userdiff_config(var, value)) {\n+\t\tcase 0: break;\n+\t\tcase -1: return -1;\n+\t\tdefault: return 0;\n+\t}\n+\n \treturn git_default_config(var, value, cb);\n }\n \n@@ -1990,7 +2030,9 @@ static int git_blame_config(const char *var, const char *value, void *cb)\n * Prepare a dummy commit that represents the work tree (or staged) item.\n * Note that annotating work tree item never works in the reverse.\n */\n-static struct commit *fake_working_tree_commit(const char *path, const char *contents_from)\n+static struct commit *fake_working_tree_commit(struct diff_options *opt,\n+\t\t\t\t\t const char *path,\n+\t\t\t\t\t const char *contents_from)\n {\n \tstruct commit *commit;\n \tstruct origin *origin;\n@@ -2030,10 +2072,14 @@ static struct commit *fake_working_tree_commit(const char *path, const char *con\n \t\t\tread_from = path;\n \t\t}\n \t\tmode = canon_mode(st.st_mode);\n+\n \t\tswitch (st.st_mode & S_IFMT) {\n \t\tcase S_IFREG:\n-\t\t\tif (strbuf_read_file(&buf, read_from, st.st_size) != st.st_size)\n-\t\t\t\tdie_errno(\"cannot open or read '%s'\", read_from);\n+\t\t\tif (DIFF_OPT_TST(opt, ALLOW_TEXTCONV) &&\n+\t\t\t textconv_object(read_from, null_sha1, &buf.buf, &buf.len))\n+\t\t\t\t;\n+\t\t\telse if (strbuf_read_file(&buf, read_from, st.st_size) != st.st_size)\n+\t\t\t\t die_errno(\"cannot open or read '%s'\", read_from);\n \t\t\tbreak;\n \t\tcase S_IFLNK:\n \t\t\tif (strbuf_readlink(&buf, read_from, st.st_size) < 0)\n@@ -2248,6 +2294,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \tgit_config(git_blame_config, NULL);\n \tinit_revisions(&revs, NULL);\n \trevs.date_mode = blame_date_mode;\n+\tDIFF_OPT_SET(&revs.diffopt, ALLOW_TEXTCONV);\n \n \tsave_commit_buffer = 0;\n \tdashdash_pos = 0;\n@@ -2384,7 +2431,8 @@ parse_done:\n \t\t * or \"--contents\".\n \t\t */\n \t\tsetup_work_tree();\n-\t\tsb.final = fake_working_tree_commit(path, contents_from);\n+\t\tsb.final = fake_working_tree_commit(&sb.revs->diffopt,\n+\t\t\t\t\t\t path, contents_from);\n \t\tadd_pending_object(&revs, &(sb.final->object), \":\");\n \t}\n \telse if (contents_from)\n@@ -2411,8 +2459,14 @@ parse_done:\n \t\tif (fill_blob_sha1(o))\n \t\t\tdie(\"no such path %s in %s\", path, final_commit_name);\n \n-\t\tsb.final_buf = read_sha1_file(o->blob_sha1, &type,\n-\t\t\t\t\t &sb.final_buf_size);\n+\t\tif (DIFF_OPT_TST(&sb.revs->diffopt, ALLOW_TEXTCONV) &&\n+\t\t textconv_object(path, o->blob_sha1, (char **) &sb.final_buf,\n+\t\t\t\t (size_t *) &sb.final_buf_size))\n+\t\t\t;\n+\t\telse\n+\t\t\tsb.final_buf = read_sha1_file(o->blob_sha1, &type,\n+\t\t\t\t\t\t &sb.final_buf_size);\n+\n \t\tif (!sb.final_buf)\n \t\t\tdie(\"Cannot read blob %s for path %s\",\n \t\t\t sha1_to_hex(o->blob_sha1),\n-- \n1.6.6.7.ga5fe3\n"},{"id":"143167","messageId":"1275924218-20154-3-git-send-email-axel.bonnet@ensimag.imag.fr","threadId":"24031","inReplyTo":"1275924218-20154-2-git-send-email-axel.bonnet@ensimag.imag.fr","subject":"[PATCH v2 3/3] t/t8006: test textconv support for blame","fromName":"Axel Bonnet","fromEmail":"axel.bonnet@ensimag.imag.fr","sentAt":"2010-06-07T15:23:38Z","receivedAt":"2010-06-07T15:23:38Z","isPatch":true,"sender":{"key":"axel.bonnet@ensimag.imag.fr","avatar":null},"body":"Test the correct functionning of textconv with blame and blame HEAD^ .\nTest the case when no driver is specified.\n\nSigned-off-by: Axel Bonnet \nSigned-off-by: Clément Poulain \nSigned-off-by: Diane Gasselin \n---\n t/t8006-blame-textconv.sh | 80 +++++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 80 insertions(+), 0 deletions(-)\n create mode 100755 t/t8006-blame-textconv.sh\n\ndiff --git a/t/t8006-blame-textconv.sh b/t/t8006-blame-textconv.sh\nnew file mode 100755\nindex 0000000..db51d4c\n--- /dev/null\n+++ b/t/t8006-blame-textconv.sh\n@@ -0,0 +1,80 @@\n+#!/bin/sh\n+\n+test_description='git blame textconv support'\n+. ./test-lib.sh\n+\n+find_blame() {\n+\tsed -e 's/^.*(/(/g'\n+}\n+\n+cat >helper <<'EOF'\n+#!/bin/sh\n+sed 's/^/converted: /' \"$@\"\n+EOF\n+chmod +x helper\n+\n+test_expect_success 'setup ' '\n+\techo test 1 >one.bin &&\n+\techo test number 2 >two.bin &&\n+\tgit add . &&\n+\tGIT_AUTHOR_NAME=Number1 git commit -a -m First --date=\"2010-01-01 18:00:00\" &&\n+\techo test 1 version 2 >one.bin &&\n+\techo test number 2 version 2 >>two.bin &&\n+\tGIT_AUTHOR_NAME=Number2 git commit -a -m Second --date=\"2010-01-01 20:00:00\"\n+'\n+\n+cat >expected <blame &&\n+\tfind_blame Number2 result &&\n+\ttest_cmp expected result\n+'\n+\n+test_expect_success 'setup textconv filters' '\n+\techo \"*.bin diff=test\" >.gitattributes &&\n+\tgit config diff.test.textconv ./helper &&\n+\tgit config diff.test.cachetextconv false\n+'\n+\n+test_expect_success 'blame with --no-textconv' '\n+\tgit blame --no-textconv one.bin >blame &&\n+\tfind_blame result &&\n+\ttest_cmp expected result\n+'\n+\n+cat >expected <blame &&\n+\tfind_blame result &&\n+\ttest_cmp expected result\n+'\n+\n+cat >expected <blame &&\n+\tfind_blame result &&\n+\ttest_cmp expected result\n+'\n+\n+test_expect_success 'make a new commit' '\n+\techo \"test number 2 version 3\" >>two.bin &&\n+\tGIT_AUTHOR_NAME=Number3 git commit -a -m Third --date=\"2010-01-01 22:00:00\"\n+'\n+\n+test_expect_success 'blame from previous revision' '\n+\tgit blame HEAD^ two.bin >blame &&\n+\tfind_blame result &&\n+\ttest_cmp expected result\n+'\n+\n+test_done\n-- \n1.6.6.7.ga5fe3\n"},{"id":"143537","messageId":"7viq5pgma4.fsf@alter.siamese.dyndns.org","threadId":"24031","inReplyTo":"1275924218-20154-2-git-send-email-axel.bonnet@ensimag.imag.fr","subject":"Re: [PATCH v2 2/3] textconv: support for blame","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-06-11T23:52:19Z","receivedAt":"2010-06-11T23:52:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Axel Bonnet writes:\n\n> +\tswitch (userdiff_config(var, value)) {\n> +\t\tcase 0: break;\n> +\t\tcase -1: return -1;\n> +\t\tdefault: return 0;\n> +\t}\n\nStyle:\n\n\tswitch (userdiff_config(var, value)) {\n\tcase 0:\n\t\tbreak;\n\tcase -1:\n \treturn -1;\n\tdefault:\n \treturn 0;\n\t}\n"},{"id":"143538","messageId":"7vd3vxgm9x.fsf@alter.siamese.dyndns.org","threadId":"24031","inReplyTo":"1275924218-20154-3-git-send-email-axel.bonnet@ensimag.imag.fr","subject":"Re: [PATCH v2 3/3] t/t8006: test textconv support for blame","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-06-11T23:52:26Z","receivedAt":"2010-06-11T23:52:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Axel Bonnet writes:\n\n> Test the correct functionning of textconv with blame and blame HEAD^ .\n> Test the case when no driver is specified.\n\nGood to see tests for both positive and negative cases. Too many people\nforget the latter.\n\n> +find_blame() {\n> +\tsed -e 's/^.*(/(/g'\n> +}\n\nTwo issues:\n\n - No need for \"g\" as your pattern is anchored at the left;\n\n - As \".*\" is greedy, you will eat a lot more than what you expect when\n the line in the blamed contents happen to have '(' on it.\n\nI'd rewrite it as:\n\n sed -e 's/^[^(]*//'\n\nWill queue all three patches, with this fix and a style fix for 2/3; no\nneed to resend.\n\nThanks.\n"},{"id":"143545","messageId":"20100612041146.GB9419@coredump.intra.peff.net","threadId":"24031","inReplyTo":"7viq5pgma4.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 2/3] textconv: support for blame","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-06-12T04:11:46Z","receivedAt":"2010-06-12T04:11:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 11, 2010 at 04:52:19PM -0700, Junio C Hamano wrote:\n\n> Axel Bonnet writes:\n> \n> > +\tswitch (userdiff_config(var, value)) {\n> > +\t\tcase 0: break;\n> > +\t\tcase -1: return -1;\n> > +\t\tdefault: return 0;\n> > +\t}\n> \n> Style:\n> \n> \tswitch (userdiff_config(var, value)) {\n> \tcase 0:\n> \t\tbreak;\n> \tcase -1:\n> \treturn -1;\n> \tdefault:\n> \treturn 0;\n> \t}\n\nThis is cut-and-paste from some of my code in git_diff_basic_config. I\ndunno if it is worth style-fixing that one, too.\n\n-Peff\n"},{"id":"143644","messageId":"AANLkTikZCtymKUgD3uYj7kU3HDuo2Y-oJr2f10CKRbgU@mail.gmail.com","threadId":"24031","inReplyTo":"7vd3vxgm9x.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 3/3] t/t8006: test textconv support for blame","fromName":"Diane Gasselin","fromEmail":"diane.gasselin@ensimag.imag.fr","sentAt":"2010-06-14T07:52:48Z","receivedAt":"2010-06-14T07:52:48Z","isPatch":true,"sender":{"key":"diane.gasselin@ensimag.imag.fr","avatar":null},"body":"Le 12 juin 2010 01:52, Junio C Hamano a écrit :\n> Axel Bonnet writes:\n>\n>> Test the correct functionning of textconv with blame and blame HEAD^ .\n>> Test the case when no driver is specified.\n>\n> Good to see tests for both positive and negative cases.  Too many people\n> forget the latter.\n>\n>> +find_blame() {\n>> +     sed -e 's/^.*(/(/g'\n>> +}\n>\n> Two issues:\n>\n>  - No need for \"g\" as your pattern is anchored at the left;\n>\n>  - As \".*\" is greedy, you will eat a lot more than what you expect when\n>   the line in the blamed contents happen to have '(' on it.\n>\n> I'd rewrite it as:\n>\n>    sed -e 's/^[^(]*//'\n>\n> Will queue all three patches, with this fix and a style fix for 2/3; no\n> need to resend.\n>\n> Thanks.\n>\nThanks. And thanks for fixing.\n"},{"id":"143696","messageId":"7vfx0p9wlm.fsf@alter.siamese.dyndns.org","threadId":"24031","inReplyTo":"1275924218-20154-2-git-send-email-axel.bonnet@ensimag.imag.fr","subject":"Re: [PATCH v2 2/3] textconv: support for blame","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-06-14T20:40:21Z","receivedAt":"2010-06-14T20:40:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Axel Bonnet writes:\n\n> @@ -86,16 +87,49 @@ struct origin {\n> ...\n> +static void fill_origin_blob(struct diff_options *opt,\n> +\t\t\t struct origin *o, mmfile_t *file)\n> {\n> \tif (!o->file.ptr) {\n> \t\tenum object_type type;\n> \t\tnum_read_blob++;\n> -\t\tfile->ptr = read_sha1_file(o->blob_sha1, &type,\n> -\t\t\t\t\t (unsigned long *)(&(file->size)));\n> +\n> +\t\tif (DIFF_OPT_TST(opt, ALLOW_TEXTCONV) &&\n> +\t\t textconv_object(o->path, o->blob_sha1, &file->ptr,\n> +\t\t\t\t (size_t *) &file->size))\n\nThis cast is not correct, as there is no guarantee that your size_t and\ntypeof(mmfile_t.size) are compatible. Depending on the gcc version, you\nwould get \"dereferencing type-punned pointer will break strict-aliasing\nrules\" error.\n\nThe same issue exists in Clément's patch to builtin/cat-file.c.\n"},{"id":"143727","messageId":"0091febb4a3832a6680a0fbc2209f841@ensimag.fr","threadId":"24031","inReplyTo":"7vfx0p9wlm.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 2/3] textconv: support for blame","fromName":"Clément Poulain","fromEmail":"clement.poulain@ensimag.imag.fr","sentAt":"2010-06-15T09:29:57Z","receivedAt":"2010-06-15T09:29:57Z","isPatch":true,"sender":{"key":"clement.poulain@ensimag.imag.fr","avatar":null},"body":"On Mon, 14 Jun 2010 13:40:21 -0700, Junio C Hamano \nwrote:\n> Axel Bonnet writes:\n> \n>> @@ -86,16 +87,49 @@ struct origin {\n>> ...\n>> +static void fill_origin_blob(struct diff_options *opt,\n>> +\t\t\t struct origin *o, mmfile_t *file)\n>> {\n>> \tif (!o->file.ptr) {\n>> \t\tenum object_type type;\n>> \t\tnum_read_blob++;\n>> -\t\tfile->ptr = read_sha1_file(o->blob_sha1, &type,\n>> -\t\t\t\t\t (unsigned long *)(&(file->size)));\n>> +\n>> +\t\tif (DIFF_OPT_TST(opt, ALLOW_TEXTCONV) &&\n>> +\t\t textconv_object(o->path, o->blob_sha1, &file->ptr,\n>> +\t\t\t\t (size_t *) &file->size))\n> \n> This cast is not correct, as there is no guarantee that your size_t and\n> typeof(mmfile_t.size) are compatible. Depending on the gcc version, you\n> would get \"dereferencing type-punned pointer will break strict-aliasing\n> rules\" error.\n> \n> The same issue exists in Clément's patch to builtin/cat-file.c.\n\nWe did this way because we found a similar cast in prep_temp_blob(),\ndiff.c:\n\n\tif (convert_to_working_tree(path,\n\t\t\t(const char *)blob, (size_t)size, &buf)) {\n\nwhere size is an unsigned long.\nIs it the same issue ? Or is it different because it's not a pointer cast?\n\nOtherwise, we thought of reversing the conversion. That is to say, instead\nof casting \"long *\" in \"size_t *\" when calling textconv_object(), is it\nbetter to cast size_t in \"unsigned long\" in textconv_object():\n\n\t*buf_size = (unsigned long) fill_textconv(textconv, df, buf); ?\n"},{"id":"143728","messageId":"20100615095452.GA32624@sigill.intra.peff.net","threadId":"24031","inReplyTo":"0091febb4a3832a6680a0fbc2209f841@ensimag.fr","subject":"Re: [PATCH v2 2/3] textconv: support for blame","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-06-15T09:54:53Z","receivedAt":"2010-06-15T09:54:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 15, 2010 at 11:29:57AM +0200, Clément Poulain wrote:\n\n> > The same issue exists in Clément's patch to builtin/cat-file.c.\n> \n> We did this way because we found a similar cast in prep_temp_blob(),\n> diff.c:\n> \n> \tif (convert_to_working_tree(path,\n> \t\t\t(const char *)blob, (size_t)size, &buf)) {\n> \n> where size is an unsigned long.\n> Is it the same issue ? Or is it different because it's not a pointer cast?\n\nRight. The compiler will handle conversion between integer types during\nassignment itself, converting representations as necessary (in fact,\nthat cast looks useless to me, as implicit conversions are allowed in\nC). The only problem is dereferencing a pointer to X as something other\nthan X.\n\n> Otherwise, we thought of reversing the conversion. That is to say, instead\n> of casting \"long *\" in \"size_t *\" when calling textconv_object(), is it\n> better to cast size_t in \"unsigned long\" in textconv_object():\n> \n> \t*buf_size = (unsigned long) fill_textconv(textconv, df, buf); ?\n\nYou shouldn't even have to cast there, for the same reason as above.\nThat is why I wrote fill_textconv to return the size parameter, rather\nthan writing to a passed-in pointer. It avoids the annoying\nsize_t / unsigned long casting caused by different usage (in an ideal\nworld, all of our sizes would be the same type, but the strbuf and diff\ncode obviously differ).\n\n-Peff\n"},{"id":"143729","messageId":"192517e06785fed4fa799bee9a11ae28@ensimag.fr","threadId":"24031","inReplyTo":"20100615095452.GA32624@sigill.intra.peff.net","subject":"Re: [PATCH v2 2/3] textconv: support for blame","fromName":"bonneta","fromEmail":"bonneta@ensimag.fr","sentAt":"2010-06-15T10:32:56Z","receivedAt":"2010-06-15T10:32:56Z","isPatch":true,"sender":{"key":"bonneta@ensimag.fr","avatar":null},"body":"On Tue, 15 Jun 2010 05:54:53 -0400, Jeff King wrote:\n> On Tue, Jun 15, 2010 at 11:29:57AM +0200, Clément Poulain wrote:\n> \n>> > The same issue exists in Clément's patch to builtin/cat-file.c.\n>> \n>> We did this way because we found a similar cast in prep_temp_blob(),\n>> diff.c:\n>> \n>> \tif (convert_to_working_tree(path,\n>> \t\t\t(const char *)blob, (size_t)size, &buf)) {\n>> \n>> where size is an unsigned long.\n>> Is it the same issue ? Or is it different because it's not a pointer\n>> cast?\n> \n> Right. The compiler will handle conversion between integer types during\n> assignment itself, converting representations as necessary (in fact,\n> that cast looks useless to me, as implicit conversions are allowed in\n> C). The only problem is dereferencing a pointer to X as something other\n> than X.\n> \n>> Otherwise, we thought of reversing the conversion. That is to say,\n>> instead\n>> of casting \"long *\" in \"size_t *\" when calling textconv_object(), is it\n>> better to cast size_t in \"unsigned long\" in textconv_object():\n>> \n>> \t*buf_size = (unsigned long) fill_textconv(textconv, df, buf); ?\n> \n> You shouldn't even have to cast there, for the same reason as above.\n> That is why I wrote fill_textconv to return the size parameter, rather\n> than writing to a passed-in pointer. It avoids the annoying\n> size_t / unsigned long casting caused by different usage (in an ideal\n> world, all of our sizes would be the same type, but the strbuf and diff\n> code obviously differ).\n\nThanks for your answer.\n\nWe have changed the declaration of textconv_object() to:\n\nstatic int textconv_object(const char *path,\n const unsigned char *sha1,\n char **buf,\n unsigned long *buf_size)\n\nAnd now we can do:\n*buf_size = fill_textconv(textconv, df, buf);\nwithout any cast.\n\nBut we have to do:\ntextconv_object(read_from, null_sha1, &buf.buf, (unsigned long *)\n&buf.len))\nwhere buf.len is size_t.\n\nIs that ok?\nOur gcc doesn't report any strict-aliasing problem, so we don't know if it\nis better than the initial version or not...\n"},{"id":"143730","messageId":"vpqbpbc4lh3.fsf@bauges.imag.fr","threadId":"24031","inReplyTo":"192517e06785fed4fa799bee9a11ae28@ensimag.fr","subject":"Re: [PATCH v2 2/3] textconv: support for blame","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2010-06-15T10:51:52Z","receivedAt":"2010-06-15T10:51:52Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"bonneta writes:\n\n> But we have to do:\n> textconv_object(read_from, null_sha1, &buf.buf, (unsigned long *)\n> &buf.len))\n> where buf.len is size_t.\n>\n> Is that ok?\n\nI don't think it fixes the problem. You're assuming sizeof(unsigned\nlong) == sizeof(size_t), otherwise, textconv_object will write the\nincorrect number of bytes at the given adress.\n\nIf you have to use this pass-by-adress, you want\n\nsize_t buf_len; /* textconv_object needs a last parameter of type\n (size_t *) */\ntextconv_object(..., &buf_len); /* <-- no cast here */\nbuf.len = buf_len; /* This is a cast, but not a pointer cast. The\n compiler will do the actual conversion if\n needed (while pointer casts are just a matter of\n typing, the generate no code). */\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"143731","messageId":"20100615110710.GA1682@sigill.intra.peff.net","threadId":"24031","inReplyTo":"aad13a73928536f87879ef7284d6cc75@ensimag.fr","subject":"Re: [PATCH v2 2/3] textconv: support for blame","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-06-15T11:07:10Z","receivedAt":"2010-06-15T11:07:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"[resending to cc git@vger]\n\nOn Tue, Jun 15, 2010 at 12:29:43PM +0200, bonneta wrote:\n\n> We have changed the declaration of textconv_object() to:\n> \n> static int textconv_object(const char *path,\n> const unsigned char *sha1,\n> char **buf,\n> unsigned long *buf_size)\n> \n> And now we can do:\n> *buf_size = fill_textconv(textconv, df, buf);\n> without any cast.\n\nI assume you mean dropping the final buf_size parameter from that\ndeclaration, which is what your usage example has. I would return either\nan \"unsigned long\" or a size_t rather than an int. We are dealing with\npotential whole-file sizes, so it is better to use at least as large a\ndata type as other parts of the code (we still may run into truncation\nproblems, but at least you are not making things any worse).\n\n> But we have to do:\n> textconv_object(read_from, null_sha1, &buf.buf, (unsigned long *)\n> &buf.len))\n> where buf.len is size_t.\n> \n> Is that ok?\n\nNo, that has the same problem. Imagine a big endian machine with a\n32-bit unsigned long and a 64-bit size_t. You would write into the first\n32 bits of buf.len, which are the high bits, giving you a ridiculously\nlarge answer.\n\nThe only portable way in C to convert between types is by assignment. So\nyou have to do:\n\n unsigned long foo;\n textconv_object(read_from, null_sha1, &buf.buf, &foo);\n buf.len = foo;\n\nBut now I'm confused. That matches the declaration you gave in the first\npart of your email, but not the usage example.\n\n-Peff\n"},{"id":"143732","messageId":"d15348c8322f5f99aa1f330d32e0ddd4@ensimag.fr","threadId":"24031","inReplyTo":"20100615110710.GA1682@sigill.intra.peff.net","subject":"Re: [PATCH v2 2/3] textconv: support for blame","fromName":"bonneta","fromEmail":"bonneta@ensimag.fr","sentAt":"2010-06-15T12:13:58Z","receivedAt":"2010-06-15T12:13:58Z","isPatch":true,"sender":{"key":"bonneta@ensimag.fr","avatar":null},"body":"On Tue, 15 Jun 2010 07:07:10 -0400, Jeff King wrote:\n> [resending to cc git@vger]\n> \n> On Tue, Jun 15, 2010 at 12:29:43PM +0200, bonneta wrote:\n> \n>> We have changed the declaration of textconv_object() to:\n>> \n>> static int textconv_object(const char *path,\n>> const unsigned char *sha1,\n>> char **buf,\n>> unsigned long *buf_size)\n>> \n>> And now we can do:\n>> *buf_size = fill_textconv(textconv, df, buf);\n>> without any cast.\n> \n> I assume you mean dropping the final buf_size parameter from that\n> declaration, which is what your usage example has. I would return either\n> an \"unsigned long\" or a size_t rather than an int. We are dealing with\n> potential whole-file sizes, so it is better to use at least as large a\n> data type as other parts of the code (we still may run into truncation\n> problems, but at least you are not making things any worse).\n> \n>> But we have to do:\n>> textconv_object(read_from, null_sha1, &buf.buf, (unsigned long *)\n>> &buf.len))\n>> where buf.len is size_t.\n>> \n>> Is that ok?\n> \n> No, that has the same problem. Imagine a big endian machine with a\n> 32-bit unsigned long and a 64-bit size_t. You would write into the first\n> 32 bits of buf.len, which are the high bits, giving you a ridiculously\n> large answer.\n> \n> The only portable way in C to convert between types is by assignment. So\n> you have to do:\n> \n> unsigned long foo;\n> textconv_object(read_from, null_sha1, &buf.buf, &foo);\n> buf.len = foo;\n> \n> But now I'm confused. That matches the declaration you gave in the first\n> part of your email, but not the usage example.\n\nWe now understand what we have to do, thank you.\nWe are currently fixing this patch.\nDo we have to resend only this patch or the whole series?\n"},{"id":"143747","messageId":"1276610328-28532-1-git-send-email-axel.bonnet@ensimag.imag.fr","threadId":"24031","inReplyTo":"7vfx0p9wlm.fsf@alter.siamese.dyndns.org","subject":"[PATCH v2 2/3] textconv: support for blame","fromName":"Axel Bonnet","fromEmail":"axel.bonnet@ensimag.imag.fr","sentAt":"2010-06-15T13:58:48Z","receivedAt":"2010-06-15T13:58:48Z","isPatch":true,"sender":{"key":"axel.bonnet@ensimag.imag.fr","avatar":null},"body":"This patches enables to perform textconv with blame if a textconv driver is\navailable for the file.\n\nThe main task is performed by the textconv_object function which prepares\ndiff_filespec and if possible converts the file using diff textconv API.\nOnly regular files are converted, so the mode of diff_filespec is faked.\n\nTextconv conversion is enabled by default (equivalent to the option\n--textconv), since blaming binary files is useless in most cases.\nThe option --no-textconv is used to disable textconv conversion.\n\nThe declarations of several functions are modified to give access to a\ndiff_options, in order to know whether the textconv option is activated or not.\n\nSigned-off-by: Axel Bonnet \nSigned-off-by: Clément Poulain \nSigned-off-by: Diane Gasselin \n---\n\nThe problem with cast between size_t and unsigned long is fixed.\nThe style problem with the case is fixed.\n\n builtin/blame.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++---------\n 1 files changed, 72 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 8506286..5b61067 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -20,6 +20,7 @@\n #include \"mailmap.h\"\n #include \"parse-options.h\"\n #include \"utf8.h\"\n+#include \"userdiff.h\"\n \n static char blame_usage[] = \"git blame [options] [rev-opts] [rev] [--] file\";\n \n@@ -86,16 +87,49 @@ struct origin {\n };\n \n /*\n+ * Prepare diff_filespec and convert it using diff textconv API\n+ * if the textconv driver exists.\n+ * Return 1 if the conversion succeeds, 0 otherwise.\n+ */\n+static int textconv_object(const char *path,\n+\t\t\t const unsigned char *sha1,\n+\t\t\t char **buf,\n+\t\t\t unsigned long *buf_size)\n+{\n+\tstruct diff_filespec *df;\n+\tstruct userdiff_driver *textconv;\n+\n+\tdf = alloc_filespec(path);\n+\tfill_filespec(df, sha1, S_IFREG | 0664);\n+\ttextconv = get_textconv(df);\n+\tif (!textconv) {\n+\t\tfree_filespec(df);\n+\t\treturn 0;\n+\t}\n+\n+\t*buf_size = fill_textconv(textconv, df, buf);\n+\tfree_filespec(df);\n+\treturn 1;\n+}\n+\n+/*\n * Given an origin, prepare mmfile_t structure to be used by the\n * diff machinery\n */\n-static void fill_origin_blob(struct origin *o, mmfile_t *file)\n+static void fill_origin_blob(struct diff_options *opt,\n+\t\t\t struct origin *o, mmfile_t *file)\n {\n \tif (!o->file.ptr) {\n \t\tenum object_type type;\n \t\tnum_read_blob++;\n-\t\tfile->ptr = read_sha1_file(o->blob_sha1, &type,\n-\t\t\t\t\t (unsigned long *)(&(file->size)));\n+\n+\t\tif (DIFF_OPT_TST(opt, ALLOW_TEXTCONV) &&\n+\t\t textconv_object(o->path, o->blob_sha1, &file->ptr,\n+\t\t\t\t (unsigned long *)(&file->size)))\n+\t\t\t;\n+\t\telse\n+\t\t\tfile->ptr = read_sha1_file(o->blob_sha1, &type,\n+\t\t\t\t\t\t (unsigned long *)(&(file->size)));\n \t\tif (!file->ptr)\n \t\t\tdie(\"Cannot read blob %s for path %s\",\n \t\t\t sha1_to_hex(o->blob_sha1),\n@@ -282,7 +316,6 @@ static struct origin *get_origin(struct scoreboard *sb,\n static int fill_blob_sha1(struct origin *origin)\n {\n \tunsigned mode;\n-\n \tif (!is_null_sha1(origin->blob_sha1))\n \t\treturn 0;\n \tif (get_tree_entry(origin->commit->object.sha1,\n@@ -741,8 +774,8 @@ static int pass_blame_to_parent(struct scoreboard *sb,\n \tif (last_in_target < 0)\n \t\treturn 1; /* nothing remains for this target */\n \n-\tfill_origin_blob(parent, &file_p);\n-\tfill_origin_blob(target, &file_o);\n+\tfill_origin_blob(&sb->revs->diffopt, parent, &file_p);\n+\tfill_origin_blob(&sb->revs->diffopt, target, &file_o);\n \tnum_get_patch++;\n \n \tmemset(&xpp, 0, sizeof(xpp));\n@@ -922,7 +955,7 @@ static int find_move_in_parent(struct scoreboard *sb,\n \tif (last_in_target < 0)\n \t\treturn 1; /* nothing remains for this target */\n \n-\tfill_origin_blob(parent, &file_p);\n+\tfill_origin_blob(&sb->revs->diffopt, parent, &file_p);\n \tif (!file_p.ptr)\n \t\treturn 0;\n \n@@ -1063,7 +1096,7 @@ static int find_copy_in_parent(struct scoreboard *sb,\n \n \t\t\tnorigin = get_origin(sb, parent, p->one->path);\n \t\t\thashcpy(norigin->blob_sha1, p->one->sha1);\n-\t\t\tfill_origin_blob(norigin, &file_p);\n+\t\t\tfill_origin_blob(&sb->revs->diffopt, norigin, &file_p);\n \t\t\tif (!file_p.ptr)\n \t\t\t\tcontinue;\n \n@@ -1983,6 +2016,16 @@ static int git_blame_config(const char *var, const char *value, void *cb)\n \t\tblame_date_mode = parse_date_format(value);\n \t\treturn 0;\n \t}\n+\n+\tswitch (userdiff_config(var, value)) {\n+\tcase 0:\n+\t\tbreak;\n+\tcase -1:\n+\t\treturn -1;\n+\tdefault:\n+\t\treturn 0;\n+\t}\n+\n \treturn git_default_config(var, value, cb);\n }\n \n@@ -1990,7 +2033,9 @@ static int git_blame_config(const char *var, const char *value, void *cb)\n * Prepare a dummy commit that represents the work tree (or staged) item.\n * Note that annotating work tree item never works in the reverse.\n */\n-static struct commit *fake_working_tree_commit(const char *path, const char *contents_from)\n+static struct commit *fake_working_tree_commit(struct diff_options *opt,\n+\t\t\t\t\t const char *path,\n+\t\t\t\t\t const char *contents_from)\n {\n \tstruct commit *commit;\n \tstruct origin *origin;\n@@ -2018,6 +2063,7 @@ static struct commit *fake_working_tree_commit(const char *path, const char *con\n \tif (!contents_from || strcmp(\"-\", contents_from)) {\n \t\tstruct stat st;\n \t\tconst char *read_from;\n+\t\tunsigned long buf_len;\n \n \t\tif (contents_from) {\n \t\t\tif (stat(contents_from, &st) < 0)\n@@ -2030,10 +2076,14 @@ static struct commit *fake_working_tree_commit(const char *path, const char *con\n \t\t\tread_from = path;\n \t\t}\n \t\tmode = canon_mode(st.st_mode);\n+\n \t\tswitch (st.st_mode & S_IFMT) {\n \t\tcase S_IFREG:\n-\t\t\tif (strbuf_read_file(&buf, read_from, st.st_size) != st.st_size)\n-\t\t\t\tdie_errno(\"cannot open or read '%s'\", read_from);\n+\t\t\tif (DIFF_OPT_TST(opt, ALLOW_TEXTCONV) &&\n+\t\t\t textconv_object(read_from, null_sha1, &buf.buf, &buf_len))\n+\t\t\t\tbuf.len = buf_len;\n+\t\t\telse if (strbuf_read_file(&buf, read_from, st.st_size) != st.st_size)\n+\t\t\t\t die_errno(\"cannot open or read '%s'\", read_from);\n \t\t\tbreak;\n \t\tcase S_IFLNK:\n \t\t\tif (strbuf_readlink(&buf, read_from, st.st_size) < 0)\n@@ -2248,6 +2298,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \tgit_config(git_blame_config, NULL);\n \tinit_revisions(&revs, NULL);\n \trevs.date_mode = blame_date_mode;\n+\tDIFF_OPT_SET(&revs.diffopt, ALLOW_TEXTCONV);\n \n \tsave_commit_buffer = 0;\n \tdashdash_pos = 0;\n@@ -2384,7 +2435,8 @@ parse_done:\n \t\t * or \"--contents\".\n \t\t */\n \t\tsetup_work_tree();\n-\t\tsb.final = fake_working_tree_commit(path, contents_from);\n+\t\tsb.final = fake_working_tree_commit(&sb.revs->diffopt,\n+\t\t\t\t\t\t path, contents_from);\n \t\tadd_pending_object(&revs, &(sb.final->object), \":\");\n \t}\n \telse if (contents_from)\n@@ -2411,8 +2463,14 @@ parse_done:\n \t\tif (fill_blob_sha1(o))\n \t\t\tdie(\"no such path %s in %s\", path, final_commit_name);\n \n-\t\tsb.final_buf = read_sha1_file(o->blob_sha1, &type,\n-\t\t\t\t\t &sb.final_buf_size);\n+\t\tif (DIFF_OPT_TST(&sb.revs->diffopt, ALLOW_TEXTCONV) &&\n+\t\t textconv_object(path, o->blob_sha1, (char **) &sb.final_buf,\n+\t\t\t\t &sb.final_buf_size))\n+\t\t\t;\n+\t\telse\n+\t\t\tsb.final_buf = read_sha1_file(o->blob_sha1, &type,\n+\t\t\t\t\t\t &sb.final_buf_size);\n+\n \t\tif (!sb.final_buf)\n \t\t\tdie(\"Cannot read blob %s for path %s\",\n \t\t\t sha1_to_hex(o->blob_sha1),\n-- \n1.6.6.7.ga5fe3\n"},{"id":"143748","messageId":"7vfx0o8hop.fsf@alter.siamese.dyndns.org","threadId":"24031","inReplyTo":"0091febb4a3832a6680a0fbc2209f841@ensimag.fr","subject":"Re: [PATCH v2 2/3] textconv: support for blame","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-06-15T15:00:06Z","receivedAt":"2010-06-15T15:00:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Clément Poulain writes:\n\n> On Mon, 14 Jun 2010 13:40:21 -0700, Junio C Hamano \n> wrote:\n>> Axel Bonnet writes:\n>> \n>>> @@ -86,16 +87,49 @@ struct origin {\n>>> ...\n>>> +static void fill_origin_blob(struct diff_options *opt,\n>>> +\t\t\t struct origin *o, mmfile_t *file)\n>>> {\n>>> \tif (!o->file.ptr) {\n>>> \t\tenum object_type type;\n>>> \t\tnum_read_blob++;\n>>> -\t\tfile->ptr = read_sha1_file(o->blob_sha1, &type,\n>>> -\t\t\t\t\t (unsigned long *)(&(file->size)));\n>>> +\n>>> +\t\tif (DIFF_OPT_TST(opt, ALLOW_TEXTCONV) &&\n>>> +\t\t textconv_object(o->path, o->blob_sha1, &file->ptr,\n>>> +\t\t\t\t (size_t *) &file->size))\n>> \n>> This cast is not correct, as there is no guarantee that your size_t and\n>> typeof(mmfile_t.size) are compatible. Depending on the gcc version, you\n>> would get \"dereferencing type-punned pointer will break strict-aliasing\n>> rules\" error.\n>> \n>> The same issue exists in Clément's patch to builtin/cat-file.c.\n>\n> We did this way because we found a similar cast in prep_temp_blob(),\n> diff.c:\n>\n> \tif (convert_to_working_tree(path,\n> \t\t\t(const char *)blob, (size_t)size, &buf)) {\n>\n> where size is an unsigned long.\n\nThat is a completely different kind of cast that is sane. The function\ntakes a _value_ of type size_t.\n\nYour cast is \"This function wants to store a value to a _memory location_\nthat is supposed to hold a value of type size_t, and I know &(file->size)\nis not such a location (it is to hold a value of type 'unsigned long');\nplease pretend that these two distinct pointers are compatible\", which is\na big no-no.\n"}]}