@matheusbsbs / matheus-bagliano / commits / 04427ac848

Fork 0

refactor userdiff textconv code

The original implementation of textconv put the conversion into fill_mmfile. This was a bad idea for a number of reasons: - it made the semantics of fill_mmfile unclear. In some cases, it was allocating data (if a text conversion occurred), and in some cases not (if we could use the data directly from the filespec). But the caller had no idea which had happened, and so didn't know whether the memory should be freed - similarly, the caller had no idea if a text conversion had occurred, and so didn't know whether the contents should be treated as binary or not. This meant that we incorrectly guessed that text-converted content was binary and didn't actually show it (unless the user overrode us with "diff.foo.binary = false", which then created problems in plumbing where the text conversion did _not_ occur) - not all callers of fill_mmfile want the text contents. In particular, we don't really want diffstat, whitespace checks, patch id generation, etc, to look at the converted contents. This patch pulls the conversion code directly into builtin_diff, so that we only see the conversion when generating an actual patch. We also then know whether we are doing a conversion, so we can check the binary-ness and free the data from the mmfile appropriately (the previous version leaked quite badly when text conversion was used) Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Oct 26, 2008 at 00:44 UTC 04427ac8483f61dcb01a48c78a821f5042c88195

2 files changed +38 -16

diff.c

+35 -13

@@ -294,18 +294,8 @@ static int fill_mmfile(mmfile_t *mf, struct diff_filespec *one)

294 else if (diff_populate_filespec(one, 0))

295 return -1;

296

297 - diff_filespec_load_driver(one);

298 - if (one->driver->textconv) {

299 - size_t size;

300 - mf->ptr = run_textconv(one->driver->textconv, one, &size);

301 - if (!mf->ptr)

302 - return -1;

303 - mf->size = size;

304 - }

305 - else {

306 - mf->ptr = one->data;

307 - mf->size = one->size;

308 - }

297 + mf->ptr = one->data;

298 + mf->size = one->size;

299 return 0;

300 }

301

@@ -1323,6 +1313,14 @@ void diff_set_mnemonic_prefix(struct diff_options *options, const char *a, const

1313 options->b_prefix = b;

1314 }

1315

1316 +static const char *get_textconv(struct diff_filespec *one)

1317 +{

1318 + if (!DIFF_FILE_VALID(one))

1319 + return NULL;

1320 + diff_filespec_load_driver(one);

1321 + return one->driver->textconv;

1322 +}

1323 +

1324 static void builtin_diff(const char *name_a,

1325 const char *name_b,

1326 struct diff_filespec *one,

@@ -1337,6 +1335,7 @@ static void builtin_diff(const char *name_a,

1335 const char *set = diff_get_color_opt(o, DIFF_METAINFO);

1336 const char *reset = diff_get_color_opt(o, DIFF_RESET);

1337 const char *a_prefix, *b_prefix;

1338 + const char *textconv_one, *textconv_two;

1339

1340 diff_set_mnemonic_prefix(o, "a/", "b/");

1341 if (DIFF_OPT_TST(o, REVERSE_DIFF)) {

@@ -1390,8 +1389,12 @@ static void builtin_diff(const char *name_a,

1389 if (fill_mmfile(&mf1, one) < 0 || fill_mmfile(&mf2, two) < 0)

1390 die("unable to read files to diff");

1391

1392 + textconv_one = get_textconv(one);

1393 + textconv_two = get_textconv(two);

1394 +

1395 if (!DIFF_OPT_TST(o, TEXT) &&

1394 - (diff_filespec_is_binary(one) || diff_filespec_is_binary(two))) {

1396 + ( (diff_filespec_is_binary(one) && !textconv_one) ||

1397 + (diff_filespec_is_binary(two) && !textconv_two) )) {

1398 /* Quite common confusing case */

1399 if (mf1.size == mf2.size &&

1400 !memcmp(mf1.ptr, mf2.ptr, mf1.size))

@@ -1412,6 +1415,21 @@ static void builtin_diff(const char *name_a,

1415 struct emit_callback ecbdata;

1416 const struct userdiff_funcname *pe;

1417

1418 + if (textconv_one) {

1419 + size_t size;

1420 + mf1.ptr = run_textconv(textconv_one, one, &size);

1421 + if (!mf1.ptr)

1422 + die("unable to read files to diff");

1423 + mf1.size = size;

1424 + }

1425 + if (textconv_two) {

1426 + size_t size;

1427 + mf2.ptr = run_textconv(textconv_two, two, &size);

1428 + if (!mf2.ptr)

1429 + die("unable to read files to diff");

1430 + mf2.size = size;

1431 + }

1432 +

1433 pe = diff_funcname_pattern(one);

1434 if (!pe)

1435 pe = diff_funcname_pattern(two);

@@ -1443,6 +1461,10 @@ static void builtin_diff(const char *name_a,

1461 &xpp, &xecfg, &ecb);

1462 if (DIFF_OPT_TST(o, COLOR_DIFF_WORDS))

1463 free_diff_words_data(&ecbdata);

1464 + if (textconv_one)

1465 + free(mf1.ptr);

1466 + if (textconv_two)

1467 + free(mf2.ptr);

1468 }

1469

1470 free_ab_and_return:

t/t4030-diff-textconv.sh

+3 -3

@@ -52,7 +52,7 @@ test_expect_success 'setup textconv filters' '

52 git config diff.fail.textconv false

53 '

55 -test_expect_failure 'diff produces text' '

55 +test_expect_success 'diff produces text' '

56 git diff HEAD^ HEAD >diff &&

57 find_diff <diff >actual &&

58 test_cmp expect.text actual

@@ -64,7 +64,7 @@ test_expect_success 'diff-tree produces binary' '

64 test_cmp expect.binary actual

65 '

67 -test_expect_failure 'log produces text' '

67 +test_expect_success 'log produces text' '

68 git log -1 -p >log &&

69 find_diff <log >actual &&

70 test_cmp expect.text actual

@@ -80,7 +80,7 @@ cat >expect.stat <<'EOF'

80 file | Bin 2 -> 4 bytes

81 1 files changed, 0 insertions(+), 0 deletions(-)

82 EOF

83 -test_expect_failure 'diffstat does not run textconv' '

83 +test_expect_success 'diffstat does not run textconv' '

84 echo file diff=fail >.gitattributes &&

85 git diff --stat HEAD^ HEAD >actual &&

86 test_cmp expect.stat actual