Merge branch 'lt/diff-stat-show-0-lines'
authorJunio C Hamano <gitster@pobox.com>
Thu, 29 Nov 2012 20:53:54 +0000 (12:53 -0800)
committerJunio C Hamano <gitster@pobox.com>
Thu, 29 Nov 2012 20:53:54 +0000 (12:53 -0800)
"git diff --stat" miscounted the total number of changed lines when
binary files were involved and hidden beyond --stat-count. It also
miscounted the total number of changed files when there were
unmerged paths.

* lt/diff-stat-show-0-lines:
t4049: refocus tests
diff --shortstat: do not count "unmerged" entries
diff --stat: do not count "unmerged" entries
diff --stat: move the "total count" logic to the last loop
diff --stat: use "file" temporary variable to refer to data->files[i]
diff --stat: status of unmodified pair in diff-q is not zero
test: add failing tests for "diff --stat" to t4049

1  2 
diff.c
diff --combined diff.c
index cb1fd9afcc2ece8c5a4123a451a0cb091baae1e3,374b2354f3eb51e7055eb2014cc84661aac13e14..732d4c227595873bb94224da618ce5612a7415af
--- 1/diff.c
--- 2/diff.c
+++ b/diff.c
@@@ -15,7 -15,6 +15,7 @@@
  #include "sigchain.h"
  #include "submodule.h"
  #include "ll-merge.h"
 +#include "string-list.h"
  
  #ifdef NO_FAST_WORKING_DIRECTORY
  #define FAST_WORKING_DIRECTORY 0
@@@ -27,7 -26,6 +27,7 @@@ static int diff_detect_rename_default
  static int diff_rename_limit_default = 400;
  static int diff_suppress_blank_empty;
  static int diff_use_color_default = -1;
 +static int diff_context_default = 3;
  static const char *diff_word_regex_cfg;
  static const char *external_diff_cmd_cfg;
  int diff_auto_refresh_index = 1;
@@@ -70,30 -68,26 +70,30 @@@ static int parse_diff_color_slot(const 
        return -1;
  }
  
 -static int parse_dirstat_params(struct diff_options *options, const char *params,
 +static int parse_dirstat_params(struct diff_options *options, const char *params_string,
                                struct strbuf *errmsg)
  {
 -      const char *p = params;
 -      int p_len, ret = 0;
 +      char *params_copy = xstrdup(params_string);
 +      struct string_list params = STRING_LIST_INIT_NODUP;
 +      int ret = 0;
 +      int i;
  
 -      while (*p) {
 -              p_len = strchrnul(p, ',') - p;
 -              if (!memcmp(p, "changes", p_len)) {
 +      if (*params_copy)
 +              string_list_split_in_place(&params, params_copy, ',', -1);
 +      for (i = 0; i < params.nr; i++) {
 +              const char *p = params.items[i].string;
 +              if (!strcmp(p, "changes")) {
                        DIFF_OPT_CLR(options, DIRSTAT_BY_LINE);
                        DIFF_OPT_CLR(options, DIRSTAT_BY_FILE);
 -              } else if (!memcmp(p, "lines", p_len)) {
 +              } else if (!strcmp(p, "lines")) {
                        DIFF_OPT_SET(options, DIRSTAT_BY_LINE);
                        DIFF_OPT_CLR(options, DIRSTAT_BY_FILE);
 -              } else if (!memcmp(p, "files", p_len)) {
 +              } else if (!strcmp(p, "files")) {
                        DIFF_OPT_CLR(options, DIRSTAT_BY_LINE);
                        DIFF_OPT_SET(options, DIRSTAT_BY_FILE);
 -              } else if (!memcmp(p, "noncumulative", p_len)) {
 +              } else if (!strcmp(p, "noncumulative")) {
                        DIFF_OPT_CLR(options, DIRSTAT_CUMULATIVE);
 -              } else if (!memcmp(p, "cumulative", p_len)) {
 +              } else if (!strcmp(p, "cumulative")) {
                        DIFF_OPT_SET(options, DIRSTAT_CUMULATIVE);
                } else if (isdigit(*p)) {
                        char *end;
                                while (isdigit(*++end))
                                        ; /* nothing */
                        }
 -                      if (end - p == p_len)
 +                      if (!*end)
                                options->dirstat_permille = permille;
                        else {
 -                              strbuf_addf(errmsg, _("  Failed to parse dirstat cut-off percentage '%.*s'\n"),
 -                                          p_len, p);
 +                              strbuf_addf(errmsg, _("  Failed to parse dirstat cut-off percentage '%s'\n"),
 +                                          p);
                                ret++;
                        }
                } else {
 -                      strbuf_addf(errmsg, _("  Unknown dirstat parameter '%.*s'\n"),
 -                                  p_len, p);
 +                      strbuf_addf(errmsg, _("  Unknown dirstat parameter '%s'\n"), p);
                        ret++;
                }
  
 -              p += p_len;
 -
 -              if (*p)
 -                      p++; /* more parameters, swallow separator */
        }
 +      string_list_clear(&params, 0);
 +      free(params_copy);
        return ret;
  }
  
 +static int parse_submodule_params(struct diff_options *options, const char *value)
 +{
 +      if (!strcmp(value, "log"))
 +              DIFF_OPT_SET(options, SUBMODULE_LOG);
 +      else if (!strcmp(value, "short"))
 +              DIFF_OPT_CLR(options, SUBMODULE_LOG);
 +      else
 +              return -1;
 +      return 0;
 +}
 +
  static int git_config_rename(const char *var, const char *value)
  {
        if (!value)
@@@ -155,12 -141,6 +155,12 @@@ int git_diff_ui_config(const char *var
                diff_use_color_default = git_config_colorbool(var, value);
                return 0;
        }
 +      if (!strcmp(var, "diff.context")) {
 +              diff_context_default = git_config_int(var, value);
 +              if (diff_context_default < 0)
 +                      return -1;
 +              return 0;
 +      }
        if (!strcmp(var, "diff.renames")) {
                diff_detect_rename_default = git_config_rename(var, value);
                return 0;
        if (!strcmp(var, "diff.ignoresubmodules"))
                handle_ignore_submodules_arg(&default_diff_options, value);
  
 +      if (!strcmp(var, "diff.submodule")) {
 +              if (parse_submodule_params(&default_diff_options, value))
 +                      warning(_("Unknown value for 'diff.submodule' config variable: '%s'"),
 +                              value);
 +              return 0;
 +      }
 +
        if (git_color_config(var, value, cb) < 0)
                return -1;
  
@@@ -1497,8 -1470,8 +1497,8 @@@ static void show_stats(struct diffstat_
        for (i = 0; (i < count) && (i < data->nr); i++) {
                struct diffstat_file *file = data->files[i];
                uintmax_t change = file->added + file->deleted;
-               if (!data->files[i]->is_interesting &&
-                        (change == 0)) {
+               if (!file->is_interesting && (change == 0)) {
                        count++; /* not shown == room for one more */
                        continue;
                }
                if (max_change < change)
                        max_change = change;
        }
-       count = i; /* min(count, data->nr) */
+       count = i; /* where we can stop scanning in data->files[] */
  
        /*
         * We have width = stat_width or term_columns() columns total.
         */
        for (i = 0; i < count; i++) {
                const char *prefix = "";
-               char *name = data->files[i]->print_name;
-               uintmax_t added = data->files[i]->added;
-               uintmax_t deleted = data->files[i]->deleted;
+               struct diffstat_file *file = data->files[i];
+               char *name = file->print_name;
+               uintmax_t added = file->added;
+               uintmax_t deleted = file->deleted;
                int name_len;
  
-               if (!data->files[i]->is_interesting &&
-                        (added + deleted == 0)) {
-                       total_files--;
+               if (!file->is_interesting && (added + deleted == 0))
                        continue;
-               }
                /*
                 * "scale" the filename
                 */
                                name = slash;
                }
  
-               if (data->files[i]->is_binary) {
+               if (file->is_binary) {
                        fprintf(options->file, "%s", line_prefix);
                        show_name(options->file, prefix, name, len);
                        fprintf(options->file, " %*s", number_width, "Bin");
                        fprintf(options->file, "\n");
                        continue;
                }
-               else if (data->files[i]->is_unmerged) {
+               else if (file->is_unmerged) {
                        fprintf(options->file, "%s", line_prefix);
                        show_name(options->file, prefix, name, len);
                        fprintf(options->file, " Unmerged\n");
                 */
                add = added;
                del = deleted;
-               adds += add;
-               dels += del;
  
                if (graph_width <= max_change) {
                        int total = add + del;
                show_graph(options->file, '-', del, del_c, reset);
                fprintf(options->file, "\n");
        }
-       for (i = count; i < data->nr; i++) {
-               uintmax_t added = data->files[i]->added;
-               uintmax_t deleted = data->files[i]->deleted;
-               if (!data->files[i]->is_interesting &&
-                        (added + deleted == 0)) {
+       for (i = 0; i < data->nr; i++) {
+               struct diffstat_file *file = data->files[i];
+               uintmax_t added = file->added;
+               uintmax_t deleted = file->deleted;
+               if (file->is_unmerged ||
+                   (!file->is_interesting && (added + deleted == 0))) {
                        total_files--;
                        continue;
                }
-               adds += added;
-               dels += deleted;
+               if (!file->is_binary) {
+                       adds += added;
+                       dels += deleted;
+               }
+               if (i < count)
+                       continue;
                if (!extra_shown)
                        fprintf(options->file, "%s ...\n", line_prefix);
                extra_shown = 1;
@@@ -1723,9 -1701,8 +1728,8 @@@ static void show_shortstats(struct diff
                int added = data->files[i]->added;
                int deleted= data->files[i]->deleted;
  
-               if (data->files[i]->is_unmerged)
-                       continue;
-               if (!data->files[i]->is_interesting && (added + deleted == 0)) {
+               if (data->files[i]->is_unmerged ||
+                   (!data->files[i]->is_interesting && (added + deleted == 0))) {
                        total_files--;
                } else if (!data->files[i]->is_binary) { /* don't count bytes */
                        adds += added;
@@@ -2241,7 -2218,7 +2245,7 @@@ static void builtin_diff(const char *na
        mmfile_t mf1, mf2;
        const char *lbl[2];
        char *a_one, *b_two;
 -      const char *set = diff_get_color_opt(o, DIFF_METAINFO);
 +      const char *meta = diff_get_color_opt(o, DIFF_METAINFO);
        const char *reset = diff_get_color_opt(o, DIFF_RESET);
        const char *a_prefix, *b_prefix;
        struct userdiff_driver *textconv_one = NULL;
                const char *add = diff_get_color_opt(o, DIFF_FILE_NEW);
                show_submodule_summary(o->file, one ? one->path : two->path,
                                one->sha1, two->sha1, two->dirty_submodule,
 -                              del, add, reset);
 +                              meta, del, add, reset);
                return;
        }
  
        b_two = quote_two(b_prefix, name_b + (*name_b == '/'));
        lbl[0] = DIFF_FILE_VALID(one) ? a_one : "/dev/null";
        lbl[1] = DIFF_FILE_VALID(two) ? b_two : "/dev/null";
 -      strbuf_addf(&header, "%s%sdiff --git %s %s%s\n", line_prefix, set, a_one, b_two, reset);
 +      strbuf_addf(&header, "%s%sdiff --git %s %s%s\n", line_prefix, meta, a_one, b_two, reset);
        if (lbl[0][0] == '/') {
                /* /dev/null */
 -              strbuf_addf(&header, "%s%snew file mode %06o%s\n", line_prefix, set, two->mode, reset);
 +              strbuf_addf(&header, "%s%snew file mode %06o%s\n", line_prefix, meta, two->mode, reset);
                if (xfrm_msg)
                        strbuf_addstr(&header, xfrm_msg);
                must_show_header = 1;
        }
        else if (lbl[1][0] == '/') {
 -              strbuf_addf(&header, "%s%sdeleted file mode %06o%s\n", line_prefix, set, one->mode, reset);
 +              strbuf_addf(&header, "%s%sdeleted file mode %06o%s\n", line_prefix, meta, one->mode, reset);
                if (xfrm_msg)
                        strbuf_addstr(&header, xfrm_msg);
                must_show_header = 1;
        }
        else {
                if (one->mode != two->mode) {
 -                      strbuf_addf(&header, "%s%sold mode %06o%s\n", line_prefix, set, one->mode, reset);
 -                      strbuf_addf(&header, "%s%snew mode %06o%s\n", line_prefix, set, two->mode, reset);
 +                      strbuf_addf(&header, "%s%sold mode %06o%s\n", line_prefix, meta, one->mode, reset);
 +                      strbuf_addf(&header, "%s%snew mode %06o%s\n", line_prefix, meta, two->mode, reset);
                        must_show_header = 1;
                }
                if (xfrm_msg)
@@@ -2438,7 -2415,7 +2442,7 @@@ static void builtin_diffstat(const cha
        }
  
        data = diffstat_add(diffstat, name_a, name_b);
-       data->is_interesting = p->status != 0;
+       data->is_interesting = p->status != DIFF_STATUS_UNKNOWN;
  
        if (!one || !two) {
                data->is_unmerged = 1;
@@@ -3202,7 -3179,7 +3206,7 @@@ void diff_setup(struct diff_options *op
        options->break_opt = -1;
        options->rename_limit = -1;
        options->dirstat_permille = diff_dirstat_permille_default;
 -      options->context = 3;
 +      options->context = diff_context_default;
        DIFF_OPT_SET(options, RENAME_EMPTY);
  
        options->change = diff_change;
@@@ -3498,14 -3475,6 +3502,14 @@@ static int parse_dirstat_opt(struct dif
        return 1;
  }
  
 +static int parse_submodule_opt(struct diff_options *options, const char *value)
 +{
 +      if (parse_submodule_params(options, value))
 +              die(_("Failed to parse --submodule option parameter: '%s'"),
 +                      value);
 +      return 1;
 +}
 +
  int diff_opt_parse(struct diff_options *options, const char **av, int ac)
  {
        const char *arg = av[0];
                handle_ignore_submodules_arg(options, arg + 20);
        } else if (!strcmp(arg, "--submodule"))
                DIFF_OPT_SET(options, SUBMODULE_LOG);
 -      else if (!prefixcmp(arg, "--submodule=")) {
 -              if (!strcmp(arg + 12, "log"))
 -                      DIFF_OPT_SET(options, SUBMODULE_LOG);
 -      }
 +      else if (!prefixcmp(arg, "--submodule="))
 +              return parse_submodule_opt(options, arg + 12);
  
        /* misc options */
        else if (!strcmp(arg, "-z"))
@@@ -4909,19 -4880,3 +4913,19 @@@ size_t fill_textconv(struct userdiff_dr
  
        return size;
  }
 +
 +void setup_diff_pager(struct diff_options *opt)
 +{
 +      /*
 +       * If the user asked for our exit code, then either they want --quiet
 +       * or --exit-code. We should definitely not bother with a pager in the
 +       * former case, as we will generate no output. Since we still properly
 +       * report our exit code even when a pager is run, we _could_ run a
 +       * pager with --exit-code. But since we have not done so historically,
 +       * and because it is easy to find people oneline advising "git diff
 +       * --exit-code" in hooks and other scripts, we do not do so.
 +       */
 +      if (!DIFF_OPT_TST(opt, EXIT_WITH_STATUS) &&
 +          check_pager_config("diff") != 0)
 +              setup_pager();
 +}