Merge branch 'jk/diff-do-not-reuse-wtf-needs-cleaning'
authorJunio C Hamano <gitster@pobox.com>
Wed, 3 Aug 2016 22:10:29 +0000 (15:10 -0700)
committerJunio C Hamano <gitster@pobox.com>
Wed, 3 Aug 2016 22:10:29 +0000 (15:10 -0700)
There is an optimization used in "git diff $treeA $treeB" to borrow
an already checked-out copy in the working tree when it is known to
be the same as the blob being compared, expecting that open/mmap of
such a file is faster than reading it from the object store, which
involves inflating and applying delta. This however kicked in even
when the checked-out copy needs to go through the convert-to-git
conversion (including the clean filter), which defeats the whole
point of the optimization. The optimization has been disabled when
the conversion is necessary.

* jk/diff-do-not-reuse-wtf-needs-cleaning:
diff: do not reuse worktree files that need "clean" conversion

1  2 
diff.c
diff --combined diff.c
index 7d0341988083dae44f8833875762b0b978bbafc0,918cedc4ad3f4ef21f5b8d04325bd7286ef83de7..b43d3dd2ecb7b2154ce6c445983e23ce89c69111
--- 1/diff.c
--- 2/diff.c
+++ b/diff.c
@@@ -26,7 -26,6 +26,7 @@@
  #endif
  
  static int diff_detect_rename_default;
 +static int diff_compaction_heuristic; /* experimental */
  static int diff_rename_limit_default = 400;
  static int diff_suppress_blank_empty;
  static int diff_use_color_default = -1;
@@@ -169,11 -168,6 +169,11 @@@ long parse_algorithm_value(const char *
   * never be affected by the setting of diff.renames
   * the user happens to have in the configuration file.
   */
 +void init_diff_ui_defaults(void)
 +{
 +      diff_detect_rename_default = 1;
 +}
 +
  int git_diff_ui_config(const char *var, const char *value, void *cb)
  {
        if (!strcmp(var, "diff.color") || !strcmp(var, "color.diff")) {
                diff_detect_rename_default = git_config_rename(var, value);
                return 0;
        }
 +      if (!strcmp(var, "diff.compactionheuristic")) {
 +              diff_compaction_heuristic = git_config_bool(var, value);
 +              return 0;
 +      }
        if (!strcmp(var, "diff.autorefreshindex")) {
                diff_auto_refresh_index = git_config_bool(var, value);
                return 0;
@@@ -1933,8 -1923,8 +1933,8 @@@ static void show_dirstat(struct diff_op
  
                name = p->two->path ? p->two->path : p->one->path;
  
 -              if (p->one->sha1_valid && p->two->sha1_valid)
 -                      content_changed = hashcmp(p->one->sha1, p->two->sha1);
 +              if (p->one->oid_valid && p->two->oid_valid)
 +                      content_changed = oidcmp(&p->one->oid, &p->two->oid);
                else
                        content_changed = 1;
  
@@@ -2306,8 -2296,7 +2306,8 @@@ static void builtin_diff(const char *na
                const char *add = diff_get_color_opt(o, DIFF_FILE_NEW);
                show_submodule_summary(o->file, one->path ? one->path : two->path,
                                line_prefix,
 -                              one->sha1, two->sha1, two->dirty_submodule,
 +                              one->oid.hash, two->oid.hash,
 +                              two->dirty_submodule,
                                meta, del, add, reset);
                return;
        }
                if (!one->data && !two->data &&
                    S_ISREG(one->mode) && S_ISREG(two->mode) &&
                    !DIFF_OPT_TST(o, BINARY)) {
 -                      if (!hashcmp(one->sha1, two->sha1)) {
 +                      if (!oidcmp(&one->oid, &two->oid)) {
                                if (must_show_header)
                                        fprintf(o->file, "%s", header.buf);
                                goto free_ab_and_return;
@@@ -2506,7 -2495,7 +2506,7 @@@ static void builtin_diffstat(const cha
                return;
        }
  
 -      same_contents = !hashcmp(one->sha1, two->sha1);
 +      same_contents = !oidcmp(&one->oid, &two->oid);
  
        if (diff_filespec_is_binary(one) || diff_filespec_is_binary(two)) {
                data->is_binary = 1;
@@@ -2639,8 -2628,8 +2639,8 @@@ void fill_filespec(struct diff_filespe
  {
        if (mode) {
                spec->mode = canon_mode(mode);
 -              hashcpy(spec->sha1, sha1);
 -              spec->sha1_valid = sha1_valid;
 +              hashcpy(spec->oid.hash, sha1);
 +              spec->oid_valid = sha1_valid;
        }
  }
  
@@@ -2683,6 -2672,13 +2683,13 @@@ static int reuse_worktree_file(const ch
        if (!FAST_WORKING_DIRECTORY && !want_file && has_sha1_pack(sha1))
                return 0;
  
+       /*
+        * Similarly, if we'd have to convert the file contents anyway, that
+        * makes the optimization not worthwhile.
+        */
+       if (!want_file && would_convert_to_git(name))
+               return 0;
        len = strlen(name);
        pos = cache_name_pos(name, len);
        if (pos < 0)
@@@ -2722,8 -2718,7 +2729,8 @@@ static int diff_populate_gitlink(struc
        if (s->dirty_submodule)
                dirty = "-dirty";
  
 -      strbuf_addf(&buf, "Subproject commit %s%s\n", sha1_to_hex(s->sha1), dirty);
 +      strbuf_addf(&buf, "Subproject commit %s%s\n",
 +                  oid_to_hex(&s->oid), dirty);
        s->size = buf.len;
        if (size_only) {
                s->data = NULL;
@@@ -2766,8 -2761,8 +2773,8 @@@ int diff_populate_filespec(struct diff_
        if (S_ISGITLINK(s->mode))
                return diff_populate_gitlink(s, size_only);
  
 -      if (!s->sha1_valid ||
 -          reuse_worktree_file(s->path, s->sha1, 0)) {
 +      if (!s->oid_valid ||
 +          reuse_worktree_file(s->path, s->oid.hash, 0)) {
                struct strbuf buf = STRBUF_INIT;
                struct stat st;
                int fd;
        else {
                enum object_type type;
                if (size_only || (flags & CHECK_BINARY)) {
 -                      type = sha1_object_info(s->sha1, &s->size);
 +                      type = sha1_object_info(s->oid.hash, &s->size);
                        if (type < 0)
 -                              die("unable to read %s", sha1_to_hex(s->sha1));
 +                              die("unable to read %s",
 +                                  oid_to_hex(&s->oid));
                        if (size_only)
                                return 0;
                        if (s->size > big_file_threshold && s->is_binary == -1) {
                                return 0;
                        }
                }
 -              s->data = read_sha1_file(s->sha1, &type, &s->size);
 +              s->data = read_sha1_file(s->oid.hash, &type, &s->size);
                if (!s->data)
 -                      die("unable to read %s", sha1_to_hex(s->sha1));
 +                      die("unable to read %s", oid_to_hex(&s->oid));
                s->should_free = 1;
        }
        return 0;
@@@ -2866,7 -2860,7 +2873,7 @@@ void diff_free_filespec_data(struct dif
  static void prep_temp_blob(const char *path, struct diff_tempfile *temp,
                           void *blob,
                           unsigned long size,
 -                         const unsigned char *sha1,
 +                         const struct object_id *oid,
                           int mode)
  {
        int fd;
                die_errno("unable to write temp-file");
        close_tempfile(&temp->tempfile);
        temp->name = get_tempfile_path(&temp->tempfile);
 -      sha1_to_hex_r(temp->hex, sha1);
 +      oid_to_hex_r(temp->hex, oid);
        xsnprintf(temp->mode, sizeof(temp->mode), "%06o", mode);
        strbuf_release(&buf);
        strbuf_release(&template);
@@@ -2915,8 -2909,8 +2922,8 @@@ static struct diff_tempfile *prepare_te
        }
  
        if (!S_ISGITLINK(one->mode) &&
 -          (!one->sha1_valid ||
 -           reuse_worktree_file(name, one->sha1, 1))) {
 +          (!one->oid_valid ||
 +           reuse_worktree_file(name, one->oid.hash, 1))) {
                struct stat st;
                if (lstat(name, &st) < 0) {
                        if (errno == ENOENT)
                        if (strbuf_readlink(&sb, name, st.st_size) < 0)
                                die_errno("readlink(%s)", name);
                        prep_temp_blob(name, temp, sb.buf, sb.len,
 -                                     (one->sha1_valid ?
 -                                      one->sha1 : null_sha1),
 -                                     (one->sha1_valid ?
 +                                     (one->oid_valid ?
 +                                      &one->oid : &null_oid),
 +                                     (one->oid_valid ?
                                        one->mode : S_IFLNK));
                        strbuf_release(&sb);
                }
                else {
                        /* we can borrow from the file in the work tree */
                        temp->name = name;
 -                      if (!one->sha1_valid)
 +                      if (!one->oid_valid)
                                sha1_to_hex_r(temp->hex, null_sha1);
                        else
 -                              sha1_to_hex_r(temp->hex, one->sha1);
 +                              sha1_to_hex_r(temp->hex, one->oid.hash);
                        /* Even though we may sometimes borrow the
                         * contents from the work tree, we always want
                         * one->mode.  mode is trustworthy even when
                if (diff_populate_filespec(one, 0))
                        die("cannot read data blob for %s", one->path);
                prep_temp_blob(name, temp, one->data, one->size,
 -                             one->sha1, one->mode);
 +                             &one->oid, one->mode);
        }
        return temp;
  }
@@@ -3068,7 -3062,7 +3075,7 @@@ static void fill_metainfo(struct strbu
        default:
                *must_show_header = 0;
        }
 -      if (one && two && hashcmp(one->sha1, two->sha1)) {
 +      if (one && two && oidcmp(&one->oid, &two->oid)) {
                int abbrev = DIFF_OPT_TST(o, FULL_INDEX) ? 40 : DEFAULT_ABBREV;
  
                if (DIFF_OPT_TST(o, BINARY)) {
                                abbrev = 40;
                }
                strbuf_addf(msg, "%s%sindex %s..", line_prefix, set,
 -                          find_unique_abbrev(one->sha1, abbrev));
 -              strbuf_addstr(msg, find_unique_abbrev(two->sha1, abbrev));
 +                          find_unique_abbrev(one->oid.hash, abbrev));
 +              strbuf_addstr(msg, find_unique_abbrev(two->oid.hash, abbrev));
                if (one->mode == two->mode)
                        strbuf_addf(msg, " %06o", one->mode);
                strbuf_addf(msg, "%s\n", reset);
@@@ -3134,20 -3128,20 +3141,20 @@@ static void run_diff_cmd(const char *pg
  static void diff_fill_sha1_info(struct diff_filespec *one)
  {
        if (DIFF_FILE_VALID(one)) {
 -              if (!one->sha1_valid) {
 +              if (!one->oid_valid) {
                        struct stat st;
                        if (one->is_stdin) {
 -                              hashcpy(one->sha1, null_sha1);
 +                              oidclr(&one->oid);
                                return;
                        }
                        if (lstat(one->path, &st) < 0)
                                die_errno("stat '%s'", one->path);
 -                      if (index_path(one->sha1, one->path, &st, 0))
 +                      if (index_path(one->oid.hash, one->path, &st, 0))
                                die("cannot hash %s", one->path);
                }
        }
        else
 -              hashclr(one->sha1);
 +              oidclr(&one->oid);
  }
  
  static void strip_prefix(int prefix_length, const char **namep, const char **otherp)
@@@ -3286,8 -3280,6 +3293,8 @@@ void diff_setup(struct diff_options *op
        options->use_color = diff_use_color_default;
        options->detect_rename = diff_detect_rename_default;
        options->xdl_opts |= diff_algorithm;
 +      if (diff_compaction_heuristic)
 +              DIFF_XDL_SET(options, COMPACTION_HEURISTIC);
  
        options->orderfile = diff_order_file_cfg;
  
@@@ -3808,10 -3800,6 +3815,10 @@@ int diff_opt_parse(struct diff_options 
                DIFF_XDL_SET(options, IGNORE_WHITESPACE_AT_EOL);
        else if (!strcmp(arg, "--ignore-blank-lines"))
                DIFF_XDL_SET(options, IGNORE_BLANK_LINES);
 +      else if (!strcmp(arg, "--compaction-heuristic"))
 +              DIFF_XDL_SET(options, COMPACTION_HEURISTIC);
 +      else if (!strcmp(arg, "--no-compaction-heuristic"))
 +              DIFF_XDL_CLR(options, COMPACTION_HEURISTIC);
        else if (!strcmp(arg, "--patience"))
                options->xdl_opts = DIFF_WITH_ALG(options, PATIENCE_DIFF);
        else if (!strcmp(arg, "--histogram"))
                if (!options->file)
                        die_errno("Could not open '%s'", path);
                options->close_file = 1;
 +              if (options->use_color != GIT_COLOR_ALWAYS)
 +                      options->use_color = GIT_COLOR_NEVER;
                return argcount;
        } else
                return 0;
@@@ -4123,9 -4109,8 +4130,9 @@@ static void diff_flush_raw(struct diff_
        fprintf(opt->file, "%s", diff_line_prefix(opt));
        if (!(opt->output_format & DIFF_FORMAT_NAME_STATUS)) {
                fprintf(opt->file, ":%06o %06o %s ", p->one->mode, p->two->mode,
 -                      diff_unique_abbrev(p->one->sha1, opt->abbrev));
 -              fprintf(opt->file, "%s ", diff_unique_abbrev(p->two->sha1, opt->abbrev));
 +                      diff_unique_abbrev(p->one->oid.hash, opt->abbrev));
 +              fprintf(opt->file, "%s ",
 +                      diff_unique_abbrev(p->two->oid.hash, opt->abbrev));
        }
        if (p->score) {
                fprintf(opt->file, "%c%03d%c", p->status, similarity_index(p),
@@@ -4174,11 -4159,11 +4181,11 @@@ int diff_unmodified_pair(struct diff_fi
        /* both are valid and point at the same path.  that is, we are
         * dealing with a change.
         */
 -      if (one->sha1_valid && two->sha1_valid &&
 -          !hashcmp(one->sha1, two->sha1) &&
 +      if (one->oid_valid && two->oid_valid &&
 +          !oidcmp(&one->oid, &two->oid) &&
            !one->dirty_submodule && !two->dirty_submodule)
                return 1; /* no change */
 -      if (!one->sha1_valid && !two->sha1_valid)
 +      if (!one->oid_valid && !two->oid_valid)
                return 1; /* both look at the same file on the filesystem. */
        return 0;
  }
@@@ -4239,7 -4224,7 +4246,7 @@@ void diff_debug_filespec(struct diff_fi
                s->path,
                DIFF_FILE_VALID(s) ? "valid" : "invalid",
                s->mode,
 -              s->sha1_valid ? sha1_to_hex(s->sha1) : "");
 +              s->oid_valid ? oid_to_hex(&s->oid) : "");
        fprintf(stderr, "queue[%d] %s size %lu\n",
                x, one ? one : "",
                s->size);
@@@ -4309,11 -4294,11 +4316,11 @@@ static void diff_resolve_rename_copy(vo
                        else
                                p->status = DIFF_STATUS_RENAMED;
                }
 -              else if (hashcmp(p->one->sha1, p->two->sha1) ||
 +              else if (oidcmp(&p->one->oid, &p->two->oid) ||
                         p->one->mode != p->two->mode ||
                         p->one->dirty_submodule ||
                         p->two->dirty_submodule ||
 -                       is_null_sha1(p->one->sha1))
 +                       is_null_oid(&p->one->oid))
                        p->status = DIFF_STATUS_MODIFIED;
                else {
                        /* This is a "no-change" entry and should not
@@@ -4529,10 -4514,8 +4536,10 @@@ static int diff_get_patch_id(struct dif
  
                if (diff_filespec_is_binary(p->one) ||
                    diff_filespec_is_binary(p->two)) {
 -                      git_SHA1_Update(&ctx, sha1_to_hex(p->one->sha1), 40);
 -                      git_SHA1_Update(&ctx, sha1_to_hex(p->two->sha1), 40);
 +                      git_SHA1_Update(&ctx, oid_to_hex(&p->one->oid),
 +                                      40);
 +                      git_SHA1_Update(&ctx, oid_to_hex(&p->two->oid),
 +                                      40);
                        continue;
                }
  
@@@ -4824,7 -4807,7 +4831,7 @@@ static int diff_filespec_check_stat_unm
         */
        if (!DIFF_FILE_VALID(p->one) || /* (1) */
            !DIFF_FILE_VALID(p->two) ||
 -          (p->one->sha1_valid && p->two->sha1_valid) ||
 +          (p->one->oid_valid && p->two->oid_valid) ||
            (p->one->mode != p->two->mode) ||
            diff_populate_filespec(p->one, CHECK_SIZE_ONLY) ||
            diff_populate_filespec(p->two, CHECK_SIZE_ONLY) ||
@@@ -5120,9 -5103,8 +5127,9 @@@ size_t fill_textconv(struct userdiff_dr
        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,
 +      if (driver->textconv_cache && df->oid_valid) {
 +              *outbuf = notes_cache_get(driver->textconv_cache,
 +                                        df->oid.hash,
                                          &size);
                if (*outbuf)
                        return size;
        if (!*outbuf)
                die("unable to read files to diff");
  
 -      if (driver->textconv_cache && df->sha1_valid) {
 +      if (driver->textconv_cache && df->oid_valid) {
                /* ignore errors, as we might be in a readonly repository */
 -              notes_cache_put(driver->textconv_cache, df->sha1, *outbuf,
 +              notes_cache_put(driver->textconv_cache, df->oid.hash, *outbuf,
                                size);
                /*
                 * we could save up changes and flush them all at the end,