Merge branch 'jk/more-comments-on-textconv' into maint

The memory ownership rule of fill_textconv() API, which was a bit
tricky, has been documented a bit better.

* jk/more-comments-on-textconv:
diff: clarify textconv interface

diff --combined diff.c
index 2136b6970b3a9751a3ec420bac61e9baa08df9b0,a09b0b6ae3b894a8f16ba7b0df5c1c499a0561fe..a088e269b42d4af24a756c991bf60dca8eb4153d
--- 1/diff.c
--- 2/diff.c
+++ b/diff.c
@@@ -2,7 -2,6 +2,7 @@@
   * Copyright (C) 2005 Junio C Hamano
   */
  #include "cache.h"
 +#include "tempfile.h"
  #include "quote.h"
  #include "diff.h"
  #include "diffcore.h"
@@@ -13,7 -12,7 +13,7 @@@
  #include "run-command.h"
  #include "utf8.h"
  #include "userdiff.h"
 -#include "sigchain.h"
 +#include "submodule-config.h"
  #include "submodule.h"
  #include "ll-merge.h"
  #include "string-list.h"
@@@ -309,26 -308,11 +309,26 @@@ static const char *external_diff(void
        return external_diff_cmd;
  }
  
 +/*
 + * Keep track of files used for diffing. Sometimes such an entry
 + * refers to a temporary file, sometimes to an existing file, and
 + * sometimes to "/dev/null".
 + */
  static struct diff_tempfile {
 -      const char *name; /* filename external diff should read from */
 -      char hex[41];
 +      /*
 +       * filename external diff should read from, or NULL if this
 +       * entry is currently not in use:
 +       */
 +      const char *name;
 +
 +      char hex[GIT_SHA1_HEXSZ + 1];
        char mode[10];
 -      char tmp_path[PATH_MAX];
 +
 +      /*
 +       * If this diff_tempfile instance refers to a temporary file,
 +       * this tempfile object is used to manage its lifetime.
 +       */
 +      struct tempfile tempfile;
  } diff_temp[2];
  
  typedef unsigned long (*sane_truncate_fn)(char *line, unsigned long len);
@@@ -494,59 -478,26 +494,59 @@@ static int new_blank_line_at_eof(struc
        return ws_blank_line(line, len, ecbdata->ws_rule);
  }
  
 -static void emit_add_line(const char *reset,
 -                        struct emit_callback *ecbdata,
 -                        const char *line, int len)
 +static void emit_line_checked(const char *reset,
 +                            struct emit_callback *ecbdata,
 +                            const char *line, int len,
 +                            enum color_diff color,
 +                            unsigned ws_error_highlight,
 +                            char sign)
  {
 -      const char *ws = diff_get_color(ecbdata->color_diff, DIFF_WHITESPACE);
 -      const char *set = diff_get_color(ecbdata->color_diff, DIFF_FILE_NEW);
 +      const char *set = diff_get_color(ecbdata->color_diff, color);
 +      const char *ws = NULL;
  
 -      if (!*ws)
 -              emit_line_0(ecbdata->opt, set, reset, '+', line, len);
 -      else if (new_blank_line_at_eof(ecbdata, line, len))
 +      if (ecbdata->opt->ws_error_highlight & ws_error_highlight) {
 +              ws = diff_get_color(ecbdata->color_diff, DIFF_WHITESPACE);
 +              if (!*ws)
 +                      ws = NULL;
 +      }
 +
 +      if (!ws)
 +              emit_line_0(ecbdata->opt, set, reset, sign, line, len);
 +      else if (sign == '+' && new_blank_line_at_eof(ecbdata, line, len))
                /* Blank line at EOF - paint '+' as well */
 -              emit_line_0(ecbdata->opt, ws, reset, '+', line, len);
 +              emit_line_0(ecbdata->opt, ws, reset, sign, line, len);
        else {
                /* Emit just the prefix, then the rest. */
 -              emit_line_0(ecbdata->opt, set, reset, '+', "", 0);
 +              emit_line_0(ecbdata->opt, set, reset, sign, "", 0);
                ws_check_emit(line, len, ecbdata->ws_rule,
                              ecbdata->opt->file, set, reset, ws);
        }
  }
  
 +static void emit_add_line(const char *reset,
 +                        struct emit_callback *ecbdata,
 +                        const char *line, int len)
 +{
 +      emit_line_checked(reset, ecbdata, line, len,
 +                        DIFF_FILE_NEW, WSEH_NEW, '+');
 +}
 +
 +static void emit_del_line(const char *reset,
 +                        struct emit_callback *ecbdata,
 +                        const char *line, int len)
 +{
 +      emit_line_checked(reset, ecbdata, line, len,
 +                        DIFF_FILE_OLD, WSEH_OLD, '-');
 +}
 +
 +static void emit_context_line(const char *reset,
 +                            struct emit_callback *ecbdata,
 +                            const char *line, int len)
 +{
 +      emit_line_checked(reset, ecbdata, line, len,
 +                        DIFF_CONTEXT, WSEH_CONTEXT, ' ');
 +}
 +
  static void emit_hunk_header(struct emit_callback *ecbdata,
                             const char *line, int len)
  {
@@@ -613,16 -564,25 +613,16 @@@ static struct diff_tempfile *claim_diff
        die("BUG: diff is failing to clean up its tempfiles");
  }
  
 -static int remove_tempfile_installed;
 -
  static void remove_tempfile(void)
  {
        int i;
        for (i = 0; i < ARRAY_SIZE(diff_temp); i++) {
 -              if (diff_temp[i].name == diff_temp[i].tmp_path)
 -                      unlink_or_warn(diff_temp[i].name);
 +              if (is_tempfile_active(&diff_temp[i].tempfile))
 +                      delete_tempfile(&diff_temp[i].tempfile);
                diff_temp[i].name = NULL;
        }
  }
  
 -static void remove_tempfile_on_signal(int signo)
 -{
 -      remove_tempfile();
 -      sigchain_pop(signo);
 -      raise(signo);
 -}
 -
  static void print_line_count(FILE *file, int count)
  {
        switch (count) {
@@@ -643,6 -603,7 +643,6 @@@ static void emit_rewrite_lines(struct e
  {
        const char *endp = NULL;
        static const char *nneof = " No newline at end of file\n";
 -      const char *old = diff_get_color(ecb->color_diff, DIFF_FILE_OLD);
        const char *reset = diff_get_color(ecb->color_diff, DIFF_RESET);
  
        while (0 < size) {
@@@ -652,7 -613,8 +652,7 @@@
                len = endp ? (endp - data + 1) : size;
                if (prefix != '+') {
                        ecb->lno_in_preimage++;
 -                      emit_line_0(ecb->opt, old, reset, '-',
 -                                  data, len);
 +                      emit_del_line(reset, ecb, data, len);
                } else {
                        ecb->lno_in_postimage++;
                        emit_add_line(reset, ecb, data, len);
@@@ -1289,27 -1251,17 +1289,27 @@@ static void fn_out_consume(void *priv, 
                return;
        }
  
 -      if (line[0] != '+') {
 -              const char *color =
 -                      diff_get_color(ecbdata->color_diff,
 -                                     line[0] == '-' ? DIFF_FILE_OLD : DIFF_CONTEXT);
 -              ecbdata->lno_in_preimage++;
 -              if (line[0] == ' ')
 -                      ecbdata->lno_in_postimage++;
 -              emit_line(ecbdata->opt, color, reset, line, len);
 -      } else {
 +      switch (line[0]) {
 +      case '+':
                ecbdata->lno_in_postimage++;
                emit_add_line(reset, ecbdata, line + 1, len - 1);
 +              break;
 +      case '-':
 +              ecbdata->lno_in_preimage++;
 +              emit_del_line(reset, ecbdata, line + 1, len - 1);
 +              break;
 +      case ' ':
 +              ecbdata->lno_in_postimage++;
 +              ecbdata->lno_in_preimage++;
 +              emit_context_line(reset, ecbdata, line + 1, len - 1);
 +              break;
 +      default:
 +              /* incomplete line at the end */
 +              ecbdata->lno_in_preimage++;
 +              emit_line(ecbdata->opt,
 +                        diff_get_color(ecbdata->color_diff, DIFF_CONTEXT),
 +                        reset, line, len);
 +              break;
        }
  }
  
@@@ -2869,7 -2821,8 +2869,7 @@@ static void prep_temp_blob(const char *
        strbuf_addstr(&template, "XXXXXX_");
        strbuf_addstr(&template, base);
  
 -      fd = git_mkstemps(temp->tmp_path, PATH_MAX, template.buf,
 -                      strlen(base) + 1);
 +      fd = mks_tempfile_ts(&temp->tempfile, template.buf, strlen(base) + 1);
        if (fd < 0)
                die_errno("unable to create temp-file");
        if (convert_to_working_tree(path,
@@@ -2879,10 -2832,11 +2879,10 @@@
        }
        if (write_in_full(fd, blob, size) != size)
                die_errno("unable to write temp-file");
 -      close(fd);
 -      temp->name = temp->tmp_path;
 -      strcpy(temp->hex, sha1_to_hex(sha1));
 -      temp->hex[40] = 0;
 -      sprintf(temp->mode, "%06o", mode);
 +      close_tempfile(&temp->tempfile);
 +      temp->name = get_tempfile_path(&temp->tempfile);
 +      sha1_to_hex_r(temp->hex, sha1);
 +      xsnprintf(temp->mode, sizeof(temp->mode), "%06o", mode);
        strbuf_release(&buf);
        strbuf_release(&template);
        free(path_dup);
@@@ -2899,11 -2853,17 +2899,11 @@@ static struct diff_tempfile *prepare_te
                 * a '+' entry produces this for file-1.
                 */
                temp->name = "/dev/null";
 -              strcpy(temp->hex, ".");
 -              strcpy(temp->mode, ".");
 +              xsnprintf(temp->hex, sizeof(temp->hex), ".");
 +              xsnprintf(temp->mode, sizeof(temp->mode), ".");
                return temp;
        }
  
 -      if (!remove_tempfile_installed) {
 -              atexit(remove_tempfile);
 -              sigchain_push_common(remove_tempfile_on_signal);
 -              remove_tempfile_installed = 1;
 -      }
 -
        if (!S_ISGITLINK(one->mode) &&
            (!one->sha1_valid ||
             reuse_worktree_file(name, one->sha1, 1))) {
@@@ -2928,16 -2888,16 +2928,16 @@@
                        /* we can borrow from the file in the work tree */
                        temp->name = name;
                        if (!one->sha1_valid)
 -                              strcpy(temp->hex, sha1_to_hex(null_sha1));
 +                              sha1_to_hex_r(temp->hex, null_sha1);
                        else
 -                              strcpy(temp->hex, sha1_to_hex(one->sha1));
 +                              sha1_to_hex_r(temp->hex, one->sha1);
                        /* Even though we may sometimes borrow the
                         * contents from the work tree, we always want
                         * one->mode.  mode is trustworthy even when
                         * !(one->sha1_valid), as long as
                         * DIFF_FILE_VALID(one).
                         */
 -                      sprintf(temp->mode, "%06o", one->mode);
 +                      xsnprintf(temp->mode, sizeof(temp->mode), "%06o", one->mode);
                }
                return temp;
        }
@@@ -3267,7 -3227,6 +3267,7 @@@ void diff_setup(struct diff_options *op
        options->rename_limit = -1;
        options->dirstat_permille = diff_dirstat_permille_default;
        options->context = diff_context_default;
 +      options->ws_error_highlight = WSEH_NEW;
        DIFF_OPT_SET(options, RENAME_EMPTY);
  
        /* pathchange left =NULL by default */
@@@ -3654,55 -3613,12 +3654,55 @@@ static void enable_patch_output(int *fm
        *fmt |= DIFF_FORMAT_PATCH;
  }
  
 -int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 +static int parse_one_token(const char **arg, const char *token)
 +{
 +      const char *rest;
 +      if (skip_prefix(*arg, token, &rest) && (!*rest || *rest == ',')) {
 +              *arg = rest;
 +              return 1;
 +      }
 +      return 0;
 +}
 +
 +static int parse_ws_error_highlight(struct diff_options *opt, const char *arg)
 +{
 +      const char *orig_arg = arg;
 +      unsigned val = 0;
 +      while (*arg) {
 +              if (parse_one_token(&arg, "none"))
 +                      val = 0;
 +              else if (parse_one_token(&arg, "default"))
 +                      val = WSEH_NEW;
 +              else if (parse_one_token(&arg, "all"))
 +                      val = WSEH_NEW | WSEH_OLD | WSEH_CONTEXT;
 +              else if (parse_one_token(&arg, "new"))
 +                      val |= WSEH_NEW;
 +              else if (parse_one_token(&arg, "old"))
 +                      val |= WSEH_OLD;
 +              else if (parse_one_token(&arg, "context"))
 +                      val |= WSEH_CONTEXT;
 +              else {
 +                      error("unknown value after ws-error-highlight=%.*s",
 +                            (int)(arg - orig_arg), orig_arg);
 +                      return 0;
 +              }
 +              if (*arg)
 +                      arg++;
 +      }
 +      opt->ws_error_highlight = val;
 +      return 1;
 +}
 +
 +int diff_opt_parse(struct diff_options *options,
 +                 const char **av, int ac, const char *prefix)
  {
        const char *arg = av[0];
        const char *optarg;
        int argcount;
  
 +      if (!prefix)
 +              prefix = "";
 +
        /* Output format options */
        if (!strcmp(arg, "-p") || !strcmp(arg, "-u") || !strcmp(arg, "--patch")
            || opt_arg(arg, 'U', "unified", &options->context))
@@@ -3827,10 -3743,9 +3827,10 @@@
                DIFF_OPT_SET(options, FIND_COPIES_HARDER);
        else if (!strcmp(arg, "--follow"))
                DIFF_OPT_SET(options, FOLLOW_RENAMES);
 -      else if (!strcmp(arg, "--no-follow"))
 +      else if (!strcmp(arg, "--no-follow")) {
                DIFF_OPT_CLR(options, FOLLOW_RENAMES);
 -      else if (!strcmp(arg, "--color"))
 +              DIFF_OPT_CLR(options, DEFAULT_FOLLOW_RENAMES);
 +      } else if (!strcmp(arg, "--color"))
                options->use_color = 1;
        else if (skip_prefix(arg, "--color=", &arg)) {
                int value = git_config_colorbool(NULL, arg);
@@@ -3895,8 -3810,6 +3895,8 @@@
                DIFF_OPT_SET(options, SUBMODULE_LOG);
        else if (skip_prefix(arg, "--submodule=", &arg))
                return parse_submodule_opt(options, arg);
 +      else if (skip_prefix(arg, "--ws-error-highlight=", &arg))
 +              return parse_ws_error_highlight(options, arg);
  
        /* misc options */
        else if (!strcmp(arg, "-z"))
@@@ -3919,8 -3832,7 +3919,8 @@@
        else if (!strcmp(arg, "--pickaxe-regex"))
                options->pickaxe_opts |= DIFF_PICKAXE_REGEX;
        else if ((argcount = short_opt('O', av, &optarg))) {
 -              options->orderfile = optarg;
 +              const char *path = prefix_filename(prefix, strlen(prefix), optarg);
 +              options->orderfile = xstrdup(path);
                return argcount;
        }
        else if ((argcount = parse_long_opt("diff-filter", av, &optarg))) {
@@@ -3959,10 -3871,9 +3959,10 @@@
        else if (!strcmp(arg, "--no-function-context"))
                DIFF_OPT_CLR(options, FUNCCONTEXT);
        else if ((argcount = parse_long_opt("output", av, &optarg))) {
 -              options->file = fopen(optarg, "w");
 +              const char *path = prefix_filename(prefix, strlen(prefix), optarg);
 +              options->file = fopen(path, "w");
                if (!options->file)
 -                      die_errno("Could not open '%s'", optarg);
 +                      die_errno("Could not open '%s'", path);
                options->close_file = 1;
                return argcount;
        } else
@@@ -4089,9 -4000,9 +4089,9 @@@ const char *diff_unique_abbrev(const un
        if (abblen < 37) {
                static char hex[41];
                if (len < abblen && abblen <= len + 2)
 -                      sprintf(hex, "%s%.*s", abbrev, len+3-abblen, "..");
 +                      xsnprintf(hex, sizeof(hex), "%s%.*s", abbrev, len+3-abblen, "..");
                else
 -                      sprintf(hex, "%s...", abbrev);
 +                      xsnprintf(hex, sizeof(hex), "%s...", abbrev);
                return hex;
        }
        return sha1_to_hex(sha1);
@@@ -5085,7 -4996,7 +5085,7 @@@ size_t fill_textconv(struct userdiff_dr
  {
        size_t size;
  
-       if (!driver || !driver->textconv) {
+       if (!driver) {
                if (!DIFF_FILE_VALID(df)) {
                        *outbuf = "";
                        return 0;
@@@ -5096,6 -5007,9 +5096,9 @@@
                return df->size;
        }
  
+       if (!driver->textconv)
+               die("BUG: fill_textconv called with non-textconv driver");
+ 
        if (driver->textconv_cache && df->sha1_valid) {
                *outbuf = notes_cache_get(driver->textconv_cache, df->sha1,
                                          &size);

diff --combined diff.h
index 70b2d70d64e1e47da617f51b8385870a22e92f40,65a5e78f64c6dedea2c6f06fd755d9731aaebcb9..4505b4d91dd0985b25f5ba8c087c5afe34166631
--- 1/diff.h
--- 2/diff.h
+++ b/diff.h
@@@ -6,7 -6,6 +6,7 @@@
  
  #include "tree-walk.h"
  #include "pathspec.h"
 +#include "object.h"
  
  struct rev_info;
  struct diff_options;
@@@ -91,7 -90,6 +91,7 @@@ typedef struct strbuf *(*diff_prefix_fn
  #define DIFF_OPT_DIRSTAT_BY_LINE     (1 << 28)
  #define DIFF_OPT_FUNCCONTEXT         (1 << 29)
  #define DIFF_OPT_PICKAXE_IGNORE_CASE (1 << 30)
 +#define DIFF_OPT_DEFAULT_FOLLOW_RENAMES (1U << 31)
  
  #define DIFF_OPT_TST(opts, flag)    ((opts)->flags & DIFF_OPT_##flag)
  #define DIFF_OPT_TOUCHED(opts, flag)    ((opts)->touched_flags & DIFF_OPT_##flag)
@@@ -139,11 -137,6 +139,11 @@@ struct diff_options 
        int dirstat_permille;
        int setup;
        int abbrev;
 +/* white-space error highlighting */
 +#define WSEH_NEW 1
 +#define WSEH_CONTEXT 2
 +#define WSEH_OLD 4
 +      unsigned ws_error_highlight;
        const char *prefix;
        int prefix_length;
        const char *stat_sep;
@@@ -214,11 -207,11 +214,11 @@@ struct combine_diff_path 
        struct combine_diff_path *next;
        char *path;
        unsigned int mode;
 -      unsigned char sha1[20];
 +      struct object_id oid;
        struct combine_diff_parent {
                char status;
                unsigned int mode;
 -              unsigned char sha1[20];
 +              struct object_id oid;
        } parent[FLEX_ARRAY];
  };
  #define combine_diff_path_size(n, l) \
@@@ -268,7 -261,7 +268,7 @@@ extern int parse_long_opt(const char *o
  extern int git_diff_basic_config(const char *var, const char *value, void *cb);
  extern int git_diff_ui_config(const char *var, const char *value, void *cb);
  extern void diff_setup(struct diff_options *);
 -extern int diff_opt_parse(struct diff_options *, const char **, int);
 +extern int diff_opt_parse(struct diff_options *, const char **, int, const char *);
  extern void diff_setup_done(struct diff_options *);
  
  #define DIFF_DETECT_RENAME    1
@@@ -345,14 -338,30 +345,30 @@@ extern int diff_flush_patch_id(struct d
  
  extern int diff_result_code(struct diff_options *, int);
  
 -extern void diff_no_index(struct rev_info *, int, const char **, const char *);
 +extern void diff_no_index(struct rev_info *, int, const char **);
  
  extern int index_differs_from(const char *def, int diff_flags);
  
+ /*
+  * Fill the contents of the filespec "df", respecting any textconv defined by
+  * its userdiff driver.  The "driver" parameter must come from a
+  * previous call to get_textconv(), and therefore should either be NULL or have
+  * textconv enabled.
+  *
+  * Note that the memory ownership of the resulting buffer depends on whether
+  * the driver field is NULL. If it is, then the memory belongs to the filespec
+  * struct. If it is non-NULL, then "outbuf" points to a newly allocated buffer
+  * that should be freed by the caller.
+  */
  extern size_t fill_textconv(struct userdiff_driver *driver,
                            struct diff_filespec *df,
                            char **outbuf);
  
+ /*
+  * Look up the userdiff driver for the given filespec, and return it if
+  * and only if it has textconv enabled (otherwise return NULL). The result
+  * can be passed to fill_textconv().
+  */
  extern struct userdiff_driver *get_textconv(struct diff_filespec *one);
  
  extern int parse_rename_score(const char **cp_p);