From 04427ac8483f61dcb01a48c78a821f5042c88195 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sun, 26 Oct 2008 00:44:53 -0400 Subject: 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 Signed-off-by: Junio C Hamano --- diff.c | 48 +++++++++++++++++++++++++++++++++++------------- t/t4030-diff-textconv.sh | 6 +++--- 2 files changed, 38 insertions(+), 16 deletions(-) diff --git a/diff.c b/diff.c index d1fd594ba..6f01595ec 100644 --- a/diff.c +++ b/diff.c @@ -294,18 +294,8 @@ static int fill_mmfile(mmfile_t *mf, struct diff_filespec *one) 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; return 0; } @@ -1323,6 +1313,14 @@ void diff_set_mnemonic_prefix(struct diff_options *options, const char *a, const options->b_prefix = b; } +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, @@ -1337,6 +1335,7 @@ static void builtin_diff(const char *name_a, 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)) { @@ -1390,8 +1389,12 @@ static void builtin_diff(const char *name_a, 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) && - (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)) @@ -1412,6 +1415,21 @@ static void builtin_diff(const char *name_a, 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); @@ -1443,6 +1461,10 @@ static void builtin_diff(const char *name_a, &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); } free_ab_and_return: diff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh index 1b0964843..090a21d0b 100755 --- a/t/t4030-diff-textconv.sh +++ b/t/t4030-diff-textconv.sh @@ -52,7 +52,7 @@ test_expect_success 'setup textconv filters' ' 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 actual && test_cmp expect.text actual @@ -64,7 +64,7 @@ test_expect_success 'diff-tree produces binary' ' test_cmp expect.binary actual ' -test_expect_failure 'log produces text' ' +test_expect_success 'log produces text' ' git log -1 -p >log && find_diff actual && test_cmp expect.text actual @@ -80,7 +80,7 @@ cat >expect.stat <<'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 -- cgit v1.2.1 From c7534ef4a12bb44806d522fc8e3961e390f9169b Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sun, 26 Oct 2008 00:45:55 -0400 Subject: userdiff: require explicitly allowing textconv Diffs that have been produced with textconv almost certainly cannot be applied, so we want to be careful not to generate them in things like format-patch. This introduces a new diff options, ALLOW_TEXTCONV, which controls this behavior. It is off by default, but is explicitly turned on for the "log" family of commands, as well as the "diff" porcelain (but not diff-* plumbing). Because both text conversion and external diffing are controlled by these diff options, we can get rid of the "plumbing versus porcelain" distinction when reading the config. This was an attempt to control the same thing, but suffered from being too coarse-grained. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin-diff.c | 1 + builtin-log.c | 1 + diff.c | 26 +++++++++++--------------- diff.h | 1 + t/t4030-diff-textconv.sh | 2 +- userdiff.c | 10 +--------- userdiff.h | 3 +-- 7 files changed, 17 insertions(+), 27 deletions(-) diff --git a/builtin-diff.c b/builtin-diff.c index 9c8c29573..2de5834c1 100644 --- a/builtin-diff.c +++ b/builtin-diff.c @@ -300,6 +300,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix) } DIFF_OPT_SET(&rev.diffopt, ALLOW_EXTERNAL); DIFF_OPT_SET(&rev.diffopt, RECURSIVE); + DIFF_OPT_SET(&rev.diffopt, ALLOW_TEXTCONV); /* * If the user asked for our exit code then don't start a diff --git a/builtin-log.c b/builtin-log.c index a0944f70a..75d698f0c 100644 --- a/builtin-log.c +++ b/builtin-log.c @@ -59,6 +59,7 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix, } else die("unrecognized argument: %s", arg); } + DIFF_OPT_SET(&rev->diffopt, ALLOW_TEXTCONV); } /* diff --git a/diff.c b/diff.c index 6f01595ec..608223ab5 100644 --- a/diff.c +++ b/diff.c @@ -93,12 +93,6 @@ int git_diff_ui_config(const char *var, const char *value, void *cb) if (!strcmp(var, "diff.external")) return git_config_string(&external_diff_cmd_cfg, var, value); - switch (userdiff_config_porcelain(var, value)) { - case 0: break; - case -1: return -1; - default: return 0; - } - return git_diff_basic_config(var, value, cb); } @@ -109,6 +103,12 @@ int git_diff_basic_config(const char *var, const char *value, void *cb) return 0; } + switch (userdiff_config(var, value)) { + case 0: break; + case -1: return -1; + default: return 0; + } + if (!prefixcmp(var, "diff.color.") || !prefixcmp(var, "color.diff.")) { int slot = parse_diff_color_slot(var, 11); if (!value) @@ -123,12 +123,6 @@ int git_diff_basic_config(const char *var, const char *value, void *cb) return 0; } - switch (userdiff_config_basic(var, value)) { - case 0: break; - case -1: return -1; - default: return 0; - } - return git_color_default_config(var, value, cb); } @@ -1335,7 +1329,7 @@ static void builtin_diff(const char *name_a, 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; + const char *textconv_one = NULL, *textconv_two = NULL; diff_set_mnemonic_prefix(o, "a/", "b/"); if (DIFF_OPT_TST(o, REVERSE_DIFF)) { @@ -1389,8 +1383,10 @@ static void builtin_diff(const char *name_a, 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, ALLOW_TEXTCONV)) { + textconv_one = get_textconv(one); + textconv_two = get_textconv(two); + } if (!DIFF_OPT_TST(o, TEXT) && ( (diff_filespec_is_binary(one) && !textconv_one) || diff --git a/diff.h b/diff.h index a49d865bd..42582edee 100644 --- a/diff.h +++ b/diff.h @@ -65,6 +65,7 @@ typedef void (*diff_format_fn_t)(struct diff_queue_struct *q, #define DIFF_OPT_IGNORE_SUBMODULES (1 << 18) #define DIFF_OPT_DIRSTAT_CUMULATIVE (1 << 19) #define DIFF_OPT_DIRSTAT_BY_FILE (1 << 20) +#define DIFF_OPT_ALLOW_TEXTCONV (1 << 21) #define DIFF_OPT_TST(opts, flag) ((opts)->flags & DIFF_OPT_##flag) #define DIFF_OPT_SET(opts, flag) ((opts)->flags |= DIFF_OPT_##flag) #define DIFF_OPT_CLR(opts, flag) ((opts)->flags &= ~DIFF_OPT_##flag) diff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh index 090a21d0b..1df48ae12 100755 --- a/t/t4030-diff-textconv.sh +++ b/t/t4030-diff-textconv.sh @@ -70,7 +70,7 @@ test_expect_success 'log produces text' ' test_cmp expect.text actual ' -test_expect_failure 'format-patch produces binary' ' +test_expect_success 'format-patch produces binary' ' git format-patch --no-binary --stdout HEAD^ >patch && find_diff actual && test_cmp expect.binary actual diff --git a/userdiff.c b/userdiff.c index d95257ab3..3681062eb 100644 --- a/userdiff.c +++ b/userdiff.c @@ -120,7 +120,7 @@ static int parse_tristate(int *b, const char *k, const char *v) return 1; } -int userdiff_config_basic(const char *k, const char *v) +int userdiff_config(const char *k, const char *v) { struct userdiff_driver *drv; @@ -130,14 +130,6 @@ int userdiff_config_basic(const char *k, const char *v) return parse_funcname(&drv->funcname, k, v, REG_EXTENDED); if ((drv = parse_driver(k, v, "binary"))) return parse_tristate(&drv->binary, k, v); - - return 0; -} - -int userdiff_config_porcelain(const char *k, const char *v) -{ - struct userdiff_driver *drv; - if ((drv = parse_driver(k, v, "command"))) return parse_string(&drv->external, k, v); if ((drv = parse_driver(k, v, "textconv"))) diff --git a/userdiff.h b/userdiff.h index f29c18ffb..ba2945770 100644 --- a/userdiff.h +++ b/userdiff.h @@ -14,8 +14,7 @@ struct userdiff_driver { const char *textconv; }; -int userdiff_config_basic(const char *k, const char *v); -int userdiff_config_porcelain(const char *k, const char *v); +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); -- cgit v1.2.1 From 2675773af893ae81f9b09f18c1f2ec86ca2158e7 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sun, 26 Oct 2008 00:46:21 -0400 Subject: only textconv regular files We treat symlinks as text containing the results of the symlink, so it doesn't make much sense to text-convert them. Similarly gitlink components just end up as the text "Subproject commit $sha1", which we should leave intact. Note that a typechange may be broken into two parts: the removal of the old part and the addition of the new. In that case, we _do_ show the textconv for any part which is the addition or removal of a file we would ordinarily textconv, since it is purely acting on the file contents. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- diff.c | 2 ++ t/t4030-diff-textconv.sh | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/diff.c b/diff.c index 608223ab5..23d454e71 100644 --- a/diff.c +++ b/diff.c @@ -1311,6 +1311,8 @@ static const char *get_textconv(struct diff_filespec *one) { if (!DIFF_FILE_VALID(one)) return NULL; + if (!S_ISREG(one->mode)) + return NULL; diff_filespec_load_driver(one); return one->driver->textconv; } diff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh index 1df48ae12..3945731e9 100755 --- a/t/t4030-diff-textconv.sh +++ b/t/t4030-diff-textconv.sh @@ -104,7 +104,7 @@ index ad8b3d2..67be421 \ No newline at end of file EOF # make a symlink the hard way that works on symlink-challenged file systems -test_expect_failure 'textconv does not act on symlinks' ' +test_expect_success 'textconv does not act on symlinks' ' echo -n frotz > file && git add file && git ls-files -s | sed -e s/100644/120000/ | -- cgit v1.2.1 From 4f672ad6c77bfe73cc5dd4b240fd42303d101d2a Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sun, 26 Oct 2008 00:49:35 -0400 Subject: wt-status: load diff ui config When "git status -v" shows a diff, we did not respect the user's usual diff preferences at all. Loading just git_diff_basic_config would give us things like rename limits and diff drivers. But it makes even more sense to load git_diff_ui_config, which gives us colorization if the user has requested it. Note that we need to take special care to cancel colorization when writing to the commit template file, as described in the code comments. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t7502-commit.sh | 8 ++++++++ wt-status.c | 10 +++++++++- 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh index 3eb9faedc..ad42c78d7 100755 --- a/t/t7502-commit.sh +++ b/t/t7502-commit.sh @@ -89,6 +89,14 @@ test_expect_success 'verbose' ' ' +test_expect_success 'verbose respects diff config' ' + + git config color.diff always && + git status -v >actual && + grep "\[1mdiff --git" actual && + git config --unset color.diff +' + test_expect_success 'cleanup commit messages (verbatim,-t)' ' echo >>negative && diff --git a/wt-status.c b/wt-status.c index c3a9cab89..54d2f5869 100644 --- a/wt-status.c +++ b/wt-status.c @@ -303,6 +303,14 @@ static void wt_status_print_verbose(struct wt_status *s) rev.diffopt.detect_rename = 1; rev.diffopt.file = s->fp; rev.diffopt.close_file = 0; + /* + * If we're not going to stdout, then we definitely don't + * want color, since we are going to the commit message + * file (and even the "auto" setting won't work, since it + * will have checked isatty on stdout). + */ + if (s->fp != stdout) + DIFF_OPT_CLR(&rev.diffopt, COLOR_DIFF); run_diff_index(&rev, 1); } @@ -422,5 +430,5 @@ int git_status_config(const char *k, const char *v, void *cb) return error("Invalid untracked files mode '%s'", v); return 0; } - return git_color_default_config(k, v, cb); + return git_diff_ui_config(k, v, cb); } -- cgit v1.2.1 From a79b8b6623288f7d5409ad749cc6553976a4f0e8 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sun, 26 Oct 2008 00:50:02 -0400 Subject: enable textconv for diff in verbose status/commit This diff is meant for human consumption, so it makes sense to apply text conversion here, as we would for the regular diff porcelain. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t4030-diff-textconv.sh | 8 ++++++++ wt-status.c | 1 + 2 files changed, 9 insertions(+) diff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh index 3945731e9..09967f566 100755 --- a/t/t4030-diff-textconv.sh +++ b/t/t4030-diff-textconv.sh @@ -76,6 +76,14 @@ test_expect_success 'format-patch produces binary' ' test_cmp expect.binary actual ' +test_expect_success 'status -v produces text' ' + git reset --soft HEAD^ && + git status -v >diff && + find_diff actual && + test_cmp expect.text actual && + git reset --soft HEAD@{1} +' + cat >expect.stat <<'EOF' file | Bin 2 -> 4 bytes 1 files changed, 0 insertions(+), 0 deletions(-) diff --git a/wt-status.c b/wt-status.c index 54d2f5869..6a7645ed8 100644 --- a/wt-status.c +++ b/wt-status.c @@ -301,6 +301,7 @@ static void wt_status_print_verbose(struct wt_status *s) setup_revisions(0, NULL, &rev, s->reference); rev.diffopt.output_format |= DIFF_FORMAT_PATCH; rev.diffopt.detect_rename = 1; + DIFF_OPT_SET(&rev.diffopt, ALLOW_TEXTCONV); rev.diffopt.file = s->fp; rev.diffopt.close_file = 0; /* -- cgit v1.2.1