Merge branch 'jc/diff-unique-abbrev-comments'
authorJunio C Hamano <gitster@pobox.com>
Wed, 26 Oct 2016 20:14:42 +0000 (13:14 -0700)
committerJunio C Hamano <gitster@pobox.com>
Wed, 26 Oct 2016 20:14:42 +0000 (13:14 -0700)
A bit more comments in a tricky code.

* jc/diff-unique-abbrev-comments:
diff_unique_abbrev(): document its assumption and limitation

1  2 
diff.c
diff --combined diff.c
index 1d304e0550766edf4679f089aafac4de14258482,01e1507e74b02753aef74aac4e427a0b0804878b..f5d6d7e4f6700d86089ac84e930c7898ae01c6d8
--- 1/diff.c
--- 2/diff.c
+++ b/diff.c
@@@ -18,7 -18,6 +18,7 @@@
  #include "ll-merge.h"
  #include "string-list.h"
  #include "argv-array.h"
 +#include "graph.h"
  
  #ifdef NO_FAST_WORKING_DIRECTORY
  #define FAST_WORKING_DIRECTORY 0
@@@ -27,7 -26,6 +27,7 @@@
  #endif
  
  static int diff_detect_rename_default;
 +static int diff_indent_heuristic; /* experimental */
  static int diff_compaction_heuristic; /* experimental */
  static int diff_rename_limit_default = 400;
  static int diff_suppress_blank_empty;
@@@ -56,11 -54,6 +56,11 @@@ static char diff_colors[][COLOR_MAXLEN
        GIT_COLOR_NORMAL,       /* FUNCINFO */
  };
  
 +static NORETURN void die_want_option(const char *option_name)
 +{
 +      die(_("option '%s' requires a value"), option_name);
 +}
 +
  static int parse_diff_color_slot(const char *var)
  {
        if (!strcasecmp(var, "context") || !strcasecmp(var, "plain"))
@@@ -138,11 -131,9 +138,11 @@@ static int parse_dirstat_params(struct 
  static int parse_submodule_params(struct diff_options *options, const char *value)
  {
        if (!strcmp(value, "log"))
 -              DIFF_OPT_SET(options, SUBMODULE_LOG);
 +              options->submodule_format = DIFF_SUBMODULE_LOG;
        else if (!strcmp(value, "short"))
 -              DIFF_OPT_CLR(options, SUBMODULE_LOG);
 +              options->submodule_format = DIFF_SUBMODULE_SHORT;
 +      else if (!strcmp(value, "diff"))
 +              options->submodule_format = DIFF_SUBMODULE_INLINE_DIFF;
        else
                return -1;
        return 0;
@@@ -183,21 -174,6 +183,21 @@@ void init_diff_ui_defaults(void
        diff_detect_rename_default = 1;
  }
  
 +int git_diff_heuristic_config(const char *var, const char *value, void *cb)
 +{
 +      if (!strcmp(var, "diff.indentheuristic")) {
 +              diff_indent_heuristic = git_config_bool(var, value);
 +              if (diff_indent_heuristic)
 +                      diff_compaction_heuristic = 0;
 +      }
 +      if (!strcmp(var, "diff.compactionheuristic")) {
 +              diff_compaction_heuristic = git_config_bool(var, value);
 +              if (diff_compaction_heuristic)
 +                      diff_indent_heuristic = 0;
 +      }
 +      return 0;
 +}
 +
  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;
                return 0;
        }
  
 +      if (git_diff_heuristic_config(var, value, cb) < 0)
 +              return -1;
        if (git_color_config(var, value, cb) < 0)
                return -1;
  
@@@ -1638,7 -1616,7 +1638,7 @@@ static void show_stats(struct diffstat_
         */
  
        if (options->stat_width == -1)
 -              width = term_columns() - options->output_prefix_length;
 +              width = term_columns() - strlen(line_prefix);
        else
                width = options->stat_width ? options->stat_width : 80;
        number_width = decimal_width(max_change) > number_width ?
@@@ -2019,7 -1997,7 +2019,7 @@@ found_damage
                return;
  
        /* Show all directories with more than x% of the changes */
 -      qsort(dir.files, dir.nr, sizeof(dir.files[0]), dirstat_compare);
 +      QSORT(dir.files, dir.nr, dirstat_compare);
        gather_dirstat(options, &dir, changed, "", 0);
  }
  
@@@ -2063,7 -2041,7 +2063,7 @@@ static void show_dirstat_by_line(struc
                return;
  
        /* Show all directories with more than x% of the changes */
 -      qsort(dir.files, dir.nr, sizeof(dir.files[0]), dirstat_compare);
 +      QSORT(dir.files, dir.nr, dirstat_compare);
        gather_dirstat(options, &dir, changed, "", 0);
  }
  
@@@ -2312,37 -2290,17 +2312,37 @@@ static void builtin_diff(const char *na
        struct strbuf header = STRBUF_INIT;
        const char *line_prefix = diff_line_prefix(o);
  
 -      if (DIFF_OPT_TST(o, SUBMODULE_LOG) &&
 -                      (!one->mode || S_ISGITLINK(one->mode)) &&
 -                      (!two->mode || S_ISGITLINK(two->mode))) {
 +      diff_set_mnemonic_prefix(o, "a/", "b/");
 +      if (DIFF_OPT_TST(o, REVERSE_DIFF)) {
 +              a_prefix = o->b_prefix;
 +              b_prefix = o->a_prefix;
 +      } else {
 +              a_prefix = o->a_prefix;
 +              b_prefix = o->b_prefix;
 +      }
 +
 +      if (o->submodule_format == DIFF_SUBMODULE_LOG &&
 +          (!one->mode || S_ISGITLINK(one->mode)) &&
 +          (!two->mode || S_ISGITLINK(two->mode))) {
                const char *del = diff_get_color_opt(o, DIFF_FILE_OLD);
                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->oid.hash, two->oid.hash,
 +                              &one->oid, &two->oid,
                                two->dirty_submodule,
                                meta, del, add, reset);
                return;
 +      } else if (o->submodule_format == DIFF_SUBMODULE_INLINE_DIFF &&
 +                 (!one->mode || S_ISGITLINK(one->mode)) &&
 +                 (!two->mode || S_ISGITLINK(two->mode))) {
 +              const char *del = diff_get_color_opt(o, DIFF_FILE_OLD);
 +              const char *add = diff_get_color_opt(o, DIFF_FILE_NEW);
 +              show_submodule_inline_diff(o->file, one->path ? one->path : two->path,
 +                              line_prefix,
 +                              &one->oid, &two->oid,
 +                              two->dirty_submodule,
 +                              meta, del, add, reset, o);
 +              return;
        }
  
        if (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) {
                textconv_two = get_textconv(two);
        }
  
 -      diff_set_mnemonic_prefix(o, "a/", "b/");
 -      if (DIFF_OPT_TST(o, REVERSE_DIFF)) {
 -              a_prefix = o->b_prefix;
 -              b_prefix = o->a_prefix;
 -      } else {
 -              a_prefix = o->a_prefix;
 -              b_prefix = o->b_prefix;
 -      }
 -
        /* Never use a non-valid filename anywhere if at all possible */
        name_a = DIFF_FILE_VALID(one) ? name_a : name_b;
        name_b = DIFF_FILE_VALID(two) ? name_b : name_a;
@@@ -2723,7 -2690,7 +2723,7 @@@ static int reuse_worktree_file(const ch
         * This is not the sha1 we are looking for, or
         * unreusable because it is not a regular file.
         */
 -      if (hashcmp(sha1, ce->sha1) || !S_ISREG(ce->ce_mode))
 +      if (hashcmp(sha1, ce->oid.hash) || !S_ISREG(ce->ce_mode))
                return 0;
  
        /*
@@@ -3109,7 -3076,7 +3109,7 @@@ static void fill_metainfo(struct strbu
                }
                strbuf_addf(msg, "%s%sindex %s..", line_prefix, set,
                            find_unique_abbrev(one->oid.hash, abbrev));
 -              strbuf_addstr(msg, find_unique_abbrev(two->oid.hash, abbrev));
 +              strbuf_add_unique_abbrev(msg, two->oid.hash, abbrev);
                if (one->mode == two->mode)
                        strbuf_addf(msg, " %06o", one->mode);
                strbuf_addf(msg, "%s\n", reset);
@@@ -3316,9 -3283,7 +3316,9 @@@ 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)
 +      if (diff_indent_heuristic)
 +              DIFF_XDL_SET(options, INDENT_HEURISTIC);
 +      else if (diff_compaction_heuristic)
                DIFF_XDL_SET(options, COMPACTION_HEURISTIC);
  
        options->orderfile = diff_order_file_cfg;
@@@ -3347,7 -3312,7 +3347,7 @@@ void diff_setup_done(struct diff_option
        if (options->output_format & DIFF_FORMAT_NO_OUTPUT)
                count++;
        if (count > 1)
 -              die("--name-only, --name-status, --check and -s are mutually exclusive");
 +              die(_("--name-only, --name-status, --check and -s are mutually exclusive"));
  
        /*
         * Most of the time we can say "there are changes"
@@@ -3543,7 -3508,7 +3543,7 @@@ static int stat_opt(struct diff_option
                        if (*arg == '=')
                                width = strtoul(arg + 1, &end, 10);
                        else if (!*arg && !av[1])
 -                              die("Option '--stat-width' requires a value");
 +                              die_want_option("--stat-width");
                        else if (!*arg) {
                                width = strtoul(av[1], &end, 10);
                                argcount = 2;
                        if (*arg == '=')
                                name_width = strtoul(arg + 1, &end, 10);
                        else if (!*arg && !av[1])
 -                              die("Option '--stat-name-width' requires a value");
 +                              die_want_option("--stat-name-width");
                        else if (!*arg) {
                                name_width = strtoul(av[1], &end, 10);
                                argcount = 2;
                        if (*arg == '=')
                                graph_width = strtoul(arg + 1, &end, 10);
                        else if (!*arg && !av[1])
 -                              die("Option '--stat-graph-width' requires a value");
 +                              die_want_option("--stat-graph-width");
                        else if (!*arg) {
                                graph_width = strtoul(av[1], &end, 10);
                                argcount = 2;
                        if (*arg == '=')
                                count = strtoul(arg + 1, &end, 10);
                        else if (!*arg && !av[1])
 -                              die("Option '--stat-count' requires a value");
 +                              die_want_option("--stat-count");
                        else if (!*arg) {
                                count = strtoul(av[1], &end, 10);
                                argcount = 2;
@@@ -3840,15 -3805,9 +3840,15 @@@ 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"))
 +      else if (!strcmp(arg, "--indent-heuristic")) {
 +              DIFF_XDL_SET(options, INDENT_HEURISTIC);
 +              DIFF_XDL_CLR(options, COMPACTION_HEURISTIC);
 +      } else if (!strcmp(arg, "--no-indent-heuristic"))
 +              DIFF_XDL_CLR(options, INDENT_HEURISTIC);
 +      else if (!strcmp(arg, "--compaction-heuristic")) {
                DIFF_XDL_SET(options, COMPACTION_HEURISTIC);
 -      else if (!strcmp(arg, "--no-compaction-heuristic"))
 +              DIFF_XDL_CLR(options, INDENT_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);
                DIFF_OPT_SET(options, OVERRIDE_SUBMODULE_CONFIG);
                handle_ignore_submodules_arg(options, arg);
        } else if (!strcmp(arg, "--submodule"))
 -              DIFF_OPT_SET(options, SUBMODULE_LOG);
 +              options->submodule_format = DIFF_SUBMODULE_LOG;
        else if (skip_prefix(arg, "--submodule=", &arg))
                return parse_submodule_opt(options, arg);
        else if (skip_prefix(arg, "--ws-error-highlight=", &arg))
                options->a_prefix = optarg;
                return argcount;
        }
 +      else if ((argcount = parse_long_opt("line-prefix", av, &optarg))) {
 +              options->line_prefix = optarg;
 +              options->line_prefix_length = strlen(options->line_prefix);
 +              graph_setup_line_prefix(options);
 +              return argcount;
 +      }
        else if ((argcount = parse_long_opt("dst-prefix", av, &optarg))) {
                options->b_prefix = optarg;
                return argcount;
@@@ -4136,7 -4089,8 +4136,8 @@@ void diff_free_filepair(struct diff_fil
        free(p);
  }
  
- /* This is different from find_unique_abbrev() in that
+ /*
+  * This is different from find_unique_abbrev() in that
   * it stuffs the result with dots for alignment.
   */
  const char *diff_unique_abbrev(const unsigned char *sha1, int len)
  
        abbrev = find_unique_abbrev(sha1, len);
        abblen = strlen(abbrev);
+       /*
+        * In well-behaved cases, where the abbbreviated result is the
+        * same as the requested length, append three dots after the
+        * abbreviation (hence the whole logic is limited to the case
+        * where abblen < 37); when the actual abbreviated result is a
+        * bit longer than the requested length, we reduce the number
+        * of dots so that they match the well-behaved ones.  However,
+        * if the actual abbreviation is longer than the requested
+        * length by more than three, we give up on aligning, and add
+        * three dots anyway, to indicate that the output is not the
+        * full object name.  Yes, this may be suboptimal, but this
+        * appears only in "diff --raw --abbrev" output and it is not
+        * worth the effort to change it now.  Note that this would
+        * likely to work fine when the automatic sizing of default
+        * abbreviation length is used--we would be fed -1 in "len" in
+        * that case, and will end up always appending three-dots, but
+        * the automatic sizing is supposed to give abblen that ensures
+        * uniqueness across all objects (statistically speaking).
+        */
        if (abblen < 37) {
                static char hex[41];
                if (len < abblen && abblen <= len + 2)
@@@ -4923,7 -4897,7 +4944,7 @@@ static int diffnamecmp(const void *a_, 
  void diffcore_fix_diff_index(struct diff_options *options)
  {
        struct diff_queue_struct *q = &diff_queued_diff;
 -      qsort(q->queue, q->nr, sizeof(q->queue[0]), diffnamecmp);
 +      QSORT(q->queue, q->nr, diffnamecmp);
  }
  
  void diffcore_std(struct diff_options *options)