summary |
shortlog |
log |
commit | commitdiff |
tree
raw |
patch |
inline | side by side (from parent 1:
df5e91f)
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>
else if (diff_populate_filespec(one, 0))
return -1;
else if (diff_populate_filespec(one, 0))
return -1;
- diff_filespec_load_driver(one);
- if (one->driver->textconv) {
- size_t size;
- mf->ptr = run_textconv(one->driver->textconv, one, &size);
- if (!mf->ptr)
- return -1;
- mf->size = size;
- }
- else {
- mf->ptr = one->data;
- mf->size = one->size;
- }
+ mf->ptr = one->data;
+ mf->size = one->size;
+static const char *get_textconv(struct diff_filespec *one)
+{
+ if (!DIFF_FILE_VALID(one))
+ return NULL;
+ diff_filespec_load_driver(one);
+ return one->driver->textconv;
+}
+
static void builtin_diff(const char *name_a,
const char *name_b,
struct diff_filespec *one,
static void builtin_diff(const char *name_a,
const char *name_b,
struct diff_filespec *one,
const char *set = diff_get_color_opt(o, DIFF_METAINFO);
const char *reset = diff_get_color_opt(o, DIFF_RESET);
const char *a_prefix, *b_prefix;
const char *set = diff_get_color_opt(o, DIFF_METAINFO);
const char *reset = diff_get_color_opt(o, DIFF_RESET);
const char *a_prefix, *b_prefix;
+ const char *textconv_one, *textconv_two;
diff_set_mnemonic_prefix(o, "a/", "b/");
if (DIFF_OPT_TST(o, REVERSE_DIFF)) {
diff_set_mnemonic_prefix(o, "a/", "b/");
if (DIFF_OPT_TST(o, REVERSE_DIFF)) {
if (fill_mmfile(&mf1, one) < 0 || fill_mmfile(&mf2, two) < 0)
die("unable to read files to diff");
if (fill_mmfile(&mf1, one) < 0 || fill_mmfile(&mf2, two) < 0)
die("unable to read files to diff");
+ textconv_one = get_textconv(one);
+ textconv_two = get_textconv(two);
+
if (!DIFF_OPT_TST(o, TEXT) &&
if (!DIFF_OPT_TST(o, TEXT) &&
- (diff_filespec_is_binary(one) || diff_filespec_is_binary(two))) {
+ ( (diff_filespec_is_binary(one) && !textconv_one) ||
+ (diff_filespec_is_binary(two) && !textconv_two) )) {
/* Quite common confusing case */
if (mf1.size == mf2.size &&
!memcmp(mf1.ptr, mf2.ptr, mf1.size))
/* Quite common confusing case */
if (mf1.size == mf2.size &&
!memcmp(mf1.ptr, mf2.ptr, mf1.size))
struct emit_callback ecbdata;
const struct userdiff_funcname *pe;
struct emit_callback ecbdata;
const struct userdiff_funcname *pe;
+ if (textconv_one) {
+ size_t size;
+ mf1.ptr = run_textconv(textconv_one, one, &size);
+ if (!mf1.ptr)
+ die("unable to read files to diff");
+ mf1.size = size;
+ }
+ if (textconv_two) {
+ size_t size;
+ mf2.ptr = run_textconv(textconv_two, two, &size);
+ if (!mf2.ptr)
+ die("unable to read files to diff");
+ mf2.size = size;
+ }
+
pe = diff_funcname_pattern(one);
if (!pe)
pe = diff_funcname_pattern(two);
pe = diff_funcname_pattern(one);
if (!pe)
pe = diff_funcname_pattern(two);
&xpp, &xecfg, &ecb);
if (DIFF_OPT_TST(o, COLOR_DIFF_WORDS))
free_diff_words_data(&ecbdata);
&xpp, &xecfg, &ecb);
if (DIFF_OPT_TST(o, COLOR_DIFF_WORDS))
free_diff_words_data(&ecbdata);
+ if (textconv_one)
+ free(mf1.ptr);
+ if (textconv_two)
+ free(mf2.ptr);
git config diff.fail.textconv false
'
git config diff.fail.textconv false
'
-test_expect_failure 'diff produces text' '
+test_expect_success 'diff produces text' '
git diff HEAD^ HEAD >diff &&
find_diff <diff >actual &&
test_cmp expect.text actual
git diff HEAD^ HEAD >diff &&
find_diff <diff >actual &&
test_cmp expect.text actual
test_cmp expect.binary actual
'
test_cmp expect.binary actual
'
-test_expect_failure 'log produces text' '
+test_expect_success 'log produces text' '
git log -1 -p >log &&
find_diff <log >actual &&
test_cmp expect.text actual
git log -1 -p >log &&
find_diff <log >actual &&
test_cmp expect.text actual
file | Bin 2 -> 4 bytes
1 files changed, 0 insertions(+), 0 deletions(-)
EOF
file | Bin 2 -> 4 bytes
1 files changed, 0 insertions(+), 0 deletions(-)
EOF
-test_expect_failure 'diffstat does not run textconv' '
+test_expect_success 'diffstat does not run textconv' '
echo file diff=fail >.gitattributes &&
git diff --stat HEAD^ HEAD >actual &&
test_cmp expect.stat actual
echo file diff=fail >.gitattributes &&
git diff --stat HEAD^ HEAD >actual &&
test_cmp expect.stat actual