{"thread":{"id":"3856","subject":"[PATCH] diff-options: add --stat","startedAt":"2006-04-13T01:02:10Z","lastAt":"2006-04-13T20:00:56Z","messageCount":13,"participants":["Johannes Schindelin","Junio C Hamano","Marco Costalba","Linus Torvalds"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"18594","messageId":"Pine.LNX.4.63.0604130301240.28688@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"3856","inReplyTo":null,"subject":"[PATCH] diff-options: add --stat","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2006-04-13T01:02:10Z","receivedAt":"2006-04-13T01:02:10Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"\nNow you can say \"git diff --stat\" (to get an idea how many changes are\nuncommitted), or \"git log --stat\" (to get an idea how many changes were\ncommitted).\n\nSigned-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>\n\n---\n\n\tProbably it would be nicer to pass down the diff_options to\n\tbuiltin_diff(), run_diff() and run_diff_cmd(), but I am *way*\n\ttoo tired to change that now.\n\n\tIt would have been nice to not just rip out and modify the diffstat\n\tcode from apply.c, but I did not want to fake patch structures.\n\tMaybe someone more intelligent than me wants to fix this...\n\n Documentation/diff-options.txt |    3 \n diff.c                         |  250 ++++++++++++++++++++++++++++++++++------\n diff.h                         |    3 \n git-diff.sh                    |    6 +\n 4 files changed, 222 insertions(+), 40 deletions(-)\n\nc706aa10bc5e2e2e22fb07aeaff1418f3d4caee0\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 338014c..447e522 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -7,6 +7,9 @@\n --patch-with-raw::\n \tGenerate patch but keep also the default raw diff output.\n \n+--stat::\n+\tGenerate a diffstat instead of a patch.\n+\n -z::\n \t\\0 line termination on output\n \ndiff --git a/diff.c b/diff.c\nindex a14e664..7f2b652 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -177,6 +177,7 @@ static int fill_mmfile(mmfile_t *mf, str\n \n struct emit_callback {\n \tconst char **label_path;\n+\tstruct diffstat_t *diffstat;\n };\n \n static int fn_out(void *priv, mmbuffer_t *mb, int nbuf)\n@@ -195,6 +196,150 @@ static int fn_out(void *priv, mmbuffer_t\n \treturn 0;\n }\n \n+struct diffstat_t {\n+\tint nr;\n+\tint alloc;\n+\tstruct diffstat_file {\n+\t\tchar *name;\n+\t\tunsigned int added, deleted;\n+\t} **files;\n+};\n+\n+static int fn_diffstat(void *priv, mmbuffer_t *mb, int nbuf)\n+{\n+\tint i;\n+\tstruct emit_callback *data = priv;\n+\tstruct diffstat_file *x;\n+\tstruct diffstat_t *diffstat = data->diffstat;\n+\n+\tif (data->label_path[0]) {\n+\t\tx = xcalloc(sizeof (*x), 1);\n+\t\tif (diffstat->nr == diffstat->alloc) {\n+\t\t\tdiffstat->alloc = alloc_nr(diffstat->alloc);\n+\t\t\tdiffstat->files = xrealloc(diffstat->files,\n+\t\t\t\t\tdiffstat->alloc * sizeof(x));\n+\t\t}\n+\t\tdiffstat->files[diffstat->nr++] = x;\n+\t\tx->name = strdup(data->label_path[0] + 2);\n+\t\tdata->label_path[0] = data->label_path[1] = NULL;\n+\t} else\n+\t\tx = diffstat->files[diffstat->nr - 1];\n+\n+\tfor (i = 0; i < nbuf; i++)\n+\t\tif (mb[i].ptr[0] == '+')\n+\t\t\tx->added++;\n+\t\telse if (mb[i].ptr[0] == '-')\n+\t\t\tx->deleted++;\n+\treturn 0;\n+}\n+\n+static void diffstat_binary(struct diffstat_t *diffstat, const char *name)\n+{\n+\tstruct diffstat_file *x = xcalloc(sizeof (*x), 1);\n+\tif (diffstat->nr == diffstat->alloc) {\n+\t\tdiffstat->alloc = alloc_nr(diffstat->alloc);\n+\t\tdiffstat->files = xrealloc(diffstat->files,\n+\t\t\t\tdiffstat->alloc * sizeof(x));\n+\t}\n+\tdiffstat->files[diffstat->nr++] = x;\n+\tx->name = strdup(name + 2);\n+\tx->added = -1;\n+}\n+\n+static const char pluses[] = \"++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\";\n+static const char minuses[]= \"----------------------------------------------------------------------\";\n+\n+static void show_stats(struct diffstat_t* data)\n+{\n+\tchar *prefix = \"\";\n+\tint i, len, add, del, total, adds = 0, dels = 0;\n+\tint max, max_change = 0, max_len = 0;\n+\tint total_files = data->nr;\n+\n+\tif (data->nr == 0)\n+\t\treturn;\n+\n+\tprintf(\"---\\n\");\n+\n+\tfor (i = 0; i < data->nr; i++) {\n+\t\tstruct diffstat_file *file = data->files[i];\n+\t\t\n+\t\tif (max_change < file->added + file->deleted)\n+\t\t\tmax_change = file->added + file->deleted;\n+\t\tlen = strlen(file->name);\n+\t\tif (max_len < len)\n+\t\t\tmax_len = len;\n+\t}\n+\n+\tfor (i = 0; i < data->nr; i++) {\n+\t\tchar *name = data->files[i]->name;\n+\t\tint added = data->files[i]->added;\n+\t\tint deleted = data->files[i]->deleted;\n+\n+\t\tif (0 < (len = quote_c_style(name, NULL, NULL, 0))) {\n+\t\t\tchar *qname = xmalloc(len + 1);\n+\t\t\tquote_c_style(name, qname, NULL, 0);\n+\t\t\tfree(name);\n+\t\t\tname = qname;\n+\t\t}\n+\n+\t\t/*\n+\t\t * \"scale\" the filename\n+\t\t */\n+\t\tlen = strlen(name);\n+\t\tmax = max_len;\n+\t\tif (max > 50)\n+\t\t\tmax = 50;\n+\t\tif (len > max) {\n+\t\t\tchar *slash;\n+\t\t\tprefix = \"...\";\n+\t\t\tmax -= 3;\n+\t\t\tname += len - max;\n+\t\t\tslash = strchr(name, '/');\n+\t\t\tif (slash)\n+\t\t\t\tname = slash;\n+\t\t}\n+\t\tlen = max;\n+\n+\t\t/*\n+\t\t * scale the add/delete\n+\t\t */\n+\t\tmax = max_change;\n+\t\tif (max + len > 70)\n+\t\t\tmax = 70 - len;\n+\n+\t\tif (added < 0) {\n+\t\t\t/* binary file */\n+\t\t\tprintf(\" %s%-*s |  Bin\\n\", prefix, len, name);\n+\t\t\tcontinue;\n+\t\t} else if (added + deleted == 0) {\n+\t\t\ttotal_files--;\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\tadd = added;\n+\t\tdel = deleted;\n+\t\ttotal = add + del;\n+\t\tadds += add;\n+\t\tdels += del;\n+\n+\t\tif (max_change > 0) {\n+\t\t\ttotal = (total * max + max_change / 2) / max_change;\n+\t\t\tadd = (add * max + max_change / 2) / max_change;\n+\t\t\tdel = total - add;\n+\t\t}\n+\t\t/* TODO: binary */\n+\t\tprintf(\" %s%-*s |%5d %.*s%.*s\\n\", prefix,\n+\t\t\t\tlen, name, added + deleted,\n+\t\t\t\tadd, pluses, del, minuses);\n+\t\tfree(name);\n+\t\tfree(data->files[i]);\n+\t}\n+\tfree(data->files);\n+\tprintf(\" %d files changed, %d insertions(+), %d deletions(-)\\n\",\n+\t\t\ttotal_files, adds, dels);\n+}\n+\n #define FIRST_FEW_BYTES 8000\n static int mmfile_is_binary(mmfile_t *mf)\n {\n@@ -211,7 +356,8 @@ static void builtin_diff(const char *nam\n \t\t\t struct diff_filespec *one,\n \t\t\t struct diff_filespec *two,\n \t\t\t const char *xfrm_msg,\n-\t\t\t int complete_rewrite)\n+\t\t\t int complete_rewrite,\n+\t\t\t struct diffstat_t* diffstat)\n {\n \tmmfile_t mf1, mf2;\n \tconst char *lbl[2];\n@@ -221,43 +367,49 @@ static void builtin_diff(const char *nam\n \tb_two = quote_two(\"b/\", name_b);\n \tlbl[0] = DIFF_FILE_VALID(one) ? a_one : \"/dev/null\";\n \tlbl[1] = DIFF_FILE_VALID(two) ? b_two : \"/dev/null\";\n-\tprintf(\"diff --git %s %s\\n\", a_one, b_two);\n-\tif (lbl[0][0] == '/') {\n-\t\t/* /dev/null */\n-\t\tprintf(\"new file mode %06o\\n\", two->mode);\n-\t\tif (xfrm_msg && xfrm_msg[0])\n-\t\t\tputs(xfrm_msg);\n-\t}\n-\telse if (lbl[1][0] == '/') {\n-\t\tprintf(\"deleted file mode %06o\\n\", one->mode);\n-\t\tif (xfrm_msg && xfrm_msg[0])\n-\t\t\tputs(xfrm_msg);\n-\t}\n-\telse {\n-\t\tif (one->mode != two->mode) {\n-\t\t\tprintf(\"old mode %06o\\n\", one->mode);\n-\t\t\tprintf(\"new mode %06o\\n\", two->mode);\n+\tif (!diffstat) {\n+\t\tprintf(\"diff --git %s %s\\n\", a_one, b_two);\n+\t\tif (lbl[0][0] == '/') {\n+\t\t\t/* /dev/null */\n+\t\t\tprintf(\"new file mode %06o\\n\", two->mode);\n+\t\t\tif (xfrm_msg && xfrm_msg[0])\n+\t\t\t\tputs(xfrm_msg);\n \t\t}\n-\t\tif (xfrm_msg && xfrm_msg[0])\n-\t\t\tputs(xfrm_msg);\n-\t\t/*\n-\t\t * we do not run diff between different kind\n-\t\t * of objects.\n-\t\t */\n-\t\tif ((one->mode ^ two->mode) & S_IFMT)\n-\t\t\tgoto free_ab_and_return;\n-\t\tif (complete_rewrite) {\n-\t\t\temit_rewrite_diff(name_a, name_b, one, two);\n-\t\t\tgoto free_ab_and_return;\n+\t\telse if (lbl[1][0] == '/') {\n+\t\t\tprintf(\"deleted file mode %06o\\n\", one->mode);\n+\t\t\tif (xfrm_msg && xfrm_msg[0])\n+\t\t\t\tputs(xfrm_msg);\n \t\t}\n+\t\telse {\n+\t\t\tif (one->mode != two->mode) {\n+\t\t\t\tprintf(\"old mode %06o\\n\", one->mode);\n+\t\t\t\tprintf(\"new mode %06o\\n\", two->mode);\n+\t\t\t}\n+\t\t\tif (xfrm_msg && xfrm_msg[0])\n+\t\t\t\tputs(xfrm_msg);\n+\t\t\t/*\n+\t\t\t * we do not run diff between different kind\n+\t\t\t * of objects.\n+\t\t\t */\n+\t\t\tif ((one->mode ^ two->mode) & S_IFMT)\n+\t\t\t\tgoto free_ab_and_return;\n+\t\t\tif (complete_rewrite) {\n+\t\t\t\temit_rewrite_diff(name_a, name_b, one, two);\n+\t\t\t\tgoto free_ab_and_return;\n+\t\t\t}\n+\t\t}\n \t}\n \n \tif (fill_mmfile(&mf1, one) < 0 || fill_mmfile(&mf2, two) < 0)\n \t\tdie(\"unable to read files to diff\");\n \n-\tif (mmfile_is_binary(&mf1) || mmfile_is_binary(&mf2))\n-\t\tprintf(\"Binary files %s and %s differ\\n\", lbl[0], lbl[1]);\n-\telse {\n+\tif (mmfile_is_binary(&mf1) || mmfile_is_binary(&mf2)) {\n+\t\tif (diffstat)\n+\t\t\tdiffstat_binary(diffstat, lbl[0]);\n+\t\telse\n+\t\t\tprintf(\"Binary files %s and %s differ\\n\",\n+\t\t\t\t\tlbl[0], lbl[1]);\n+\t} else {\n \t\t/* Crazy xdl interfaces.. */\n \t\tconst char *diffopts = getenv(\"GIT_DIFF_OPTS\");\n \t\txpparam_t xpp;\n@@ -266,6 +418,7 @@ static void builtin_diff(const char *nam\n \t\tstruct emit_callback ecbdata;\n \n \t\tecbdata.label_path = lbl;\n+\t\tecbdata.diffstat = diffstat;\n \t\txpp.flags = XDF_NEED_MINIMAL;\n \t\txecfg.ctxlen = 3;\n \t\txecfg.flags = XDL_EMIT_FUNCNAMES;\n@@ -275,7 +428,7 @@ static void builtin_diff(const char *nam\n \t\t\txecfg.ctxlen = strtoul(diffopts + 10, NULL, 10);\n \t\telse if (!strncmp(diffopts, \"-u\", 2))\n \t\t\txecfg.ctxlen = strtoul(diffopts + 2, NULL, 10);\n-\t\tecb.outf = fn_out;\n+\t\tecb.outf = diffstat ? fn_diffstat : fn_out;\n \t\tecb.priv = &ecbdata;\n \t\txdl_diff(&mf1, &mf2, &xpp, &xecfg, &ecb);\n \t}\n@@ -690,16 +843,19 @@ static void run_diff_cmd(const char *pgm\n \t\t\t struct diff_filespec *one,\n \t\t\t struct diff_filespec *two,\n \t\t\t const char *xfrm_msg,\n-\t\t\t int complete_rewrite)\n+\t\t\t int complete_rewrite,\n+\t\t\t struct diffstat_t *diffstat)\n {\n \tif (pgm) {\n+\t\tif (diffstat)\n+\t\t\tdie (\"Cannot use diffstat with external diff\");\n \t\trun_external_diff(pgm, name, other, one, two, xfrm_msg,\n \t\t\t\t  complete_rewrite);\n \t\treturn;\n \t}\n \tif (one && two)\n \t\tbuiltin_diff(name, other ? other : name,\n-\t\t\t     one, two, xfrm_msg, complete_rewrite);\n+\t\t\t     one, two, xfrm_msg, complete_rewrite, diffstat);\n \telse\n \t\tprintf(\"* Unmerged path %s\\n\", name);\n }\n@@ -733,7 +889,8 @@ static void run_diff(struct diff_filepai\n \n \tif (DIFF_PAIR_UNMERGED(p)) {\n \t\t/* unmerged */\n-\t\trun_diff_cmd(pgm, p->one->path, NULL, NULL, NULL, NULL, 0);\n+\t\trun_diff_cmd(pgm, p->one->path, NULL, NULL, NULL, NULL, 0,\n+\t\t\to->diffstat);\n \t\treturn;\n \t}\n \n@@ -805,15 +962,17 @@ static void run_diff(struct diff_filepai\n \t\t * needs to be split into deletion and creation.\n \t\t */\n \t\tstruct diff_filespec *null = alloc_filespec(two->path);\n-\t\trun_diff_cmd(NULL, name, other, one, null, xfrm_msg, 0);\n+\t\trun_diff_cmd(NULL, name, other, one, null, xfrm_msg, 0,\n+\t\t\to->diffstat);\n \t\tfree(null);\n \t\tnull = alloc_filespec(one->path);\n-\t\trun_diff_cmd(NULL, name, other, null, two, xfrm_msg, 0);\n+\t\trun_diff_cmd(NULL, name, other, null, two, xfrm_msg, 0,\n+\t\t\to->diffstat);\n \t\tfree(null);\n \t}\n \telse\n \t\trun_diff_cmd(pgm, name, other, one, two, xfrm_msg,\n-\t\t\t     complete_rewrite);\n+\t\t\t     complete_rewrite, o->diffstat);\n \n \tfree(name_munged);\n \tfree(other_munged);\n@@ -866,6 +1025,8 @@ int diff_opt_parse(struct diff_options *\n \t\toptions->output_format = DIFF_FORMAT_PATCH;\n \t\toptions->with_raw = 1;\n \t}\n+\telse if (!strcmp(arg, \"--stat\"))\n+\t\toptions->output_format = DIFF_FORMAT_DIFFSTAT;\n \telse if (!strcmp(arg, \"-z\"))\n \t\toptions->line_termination = 0;\n \telse if (!strncmp(arg, \"-l\", 2))\n@@ -1291,6 +1452,7 @@ static void flush_one_pair(struct diff_f\n \t\tbreak;\n \tdefault:\n \t\tswitch (diff_output_format) {\n+\t\tcase DIFF_FORMAT_DIFFSTAT:\n \t\tcase DIFF_FORMAT_PATCH:\n \t\t\tdiff_flush_patch(p, options);\n \t\t\tbreak;\n@@ -1316,6 +1478,12 @@ void diff_flush(struct diff_options *opt\n \tstruct diff_queue_struct *q = &diff_queued_diff;\n \tint i;\n \tint diff_output_format = options->output_format;\n+\tstruct diffstat_t *diffstat = NULL;\n+\n+\tif (diff_output_format == DIFF_FORMAT_DIFFSTAT) {\n+\t\tdiffstat = xcalloc(sizeof (struct diffstat_t), 1);\n+\t\toptions->diffstat = diffstat;\n+\t}\n \n \tif (options->with_raw) {\n \t\tfor (i = 0; i < q->nr; i++) {\n@@ -1329,6 +1497,12 @@ void diff_flush(struct diff_options *opt\n \t\tflush_one_pair(p, diff_output_format, options);\n \t\tdiff_free_filepair(p);\n \t}\n+\n+\tif (diffstat) {\n+\t\tshow_stats(diffstat);\n+\t\tfree(diffstat);\n+\t}\n+\n \tfree(q->queue);\n \tq->queue = NULL;\n \tq->nr = q->alloc = 0;\ndiff --git a/diff.h b/diff.h\nindex 236095f..5c1526c 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -44,6 +44,7 @@ struct diff_options {\n \tint *pathlens;\n \tchange_fn_t change;\n \tadd_remove_fn_t add_remove;\n+\tstruct diffstat_t *diffstat;\n };\n \n extern void diff_tree_setup_paths(const char **paths, struct diff_options *);\n@@ -119,6 +120,7 @@ #define COMMON_DIFF_OPTIONS_HELP \\\n \"  -u            synonym for -p.\\n\" \\\n \"  --patch-with-raw\\n\" \\\n \"                output both a patch and the diff-raw format.\\n\" \\\n+\"  --stat        show diffstat instead of patch.\\n\" \\\n \"  --name-only   show only names of changed files.\\n\" \\\n \"  --name-status show names and status of changed files.\\n\" \\\n \"  --full-index  show full object name on index lines.\\n\" \\\n@@ -142,6 +144,7 @@ #define DIFF_FORMAT_PATCH\t2\n #define DIFF_FORMAT_NO_OUTPUT\t3\n #define DIFF_FORMAT_NAME\t4\n #define DIFF_FORMAT_NAME_STATUS\t5\n+#define DIFF_FORMAT_DIFFSTAT\t6\n \n extern void diff_flush(struct diff_options*);\n \ndiff --git a/git-diff.sh b/git-diff.sh\nindex dc0dd31..0fe6770 100755\n--- a/git-diff.sh\n+++ b/git-diff.sh\n@@ -30,9 +30,11 @@ case \" $flags \" in\n \tcc_or_p=--cc ;;\n esac\n \n-# If we do not have --name-status, --name-only, -r, or -c default to --cc.\n+# If we do not have --name-status, --name-only, -r, -c or --stat,\n+# default to --cc.\n case \" $flags \" in\n-*\" '--name-status' \"* | *\" '--name-only' \"* | *\" '-r' \"* | *\" '-c' \"* )\n+*\" '--name-status' \"* | *\" '--name-only' \"* | *\" '-r' \"* | *\" '-c' \"* | \\\n+*\" '--stat' \"*)\n \t;;\n *)\n \tflags=\"$flags'$cc_or_p' \" ;;\n-- \n1.2.0.gd507-dirty\n"},{"id":"18595","messageId":"Pine.LNX.4.63.0604130308150.11360@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"3856","inReplyTo":"Pine.LNX.4.63.0604130301240.28688@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: [PATCH] diff-options: add --stat","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2006-04-13T01:15:51Z","receivedAt":"2006-04-13T01:15:51Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nI just realized that something is wrong with this patch: the first file \nof the patchset seems to be ignored. I'll fix it tomorrow.\n\nCiao,\nDscho\n"},{"id":"18596","messageId":"Pine.LNX.4.63.0604130331210.12456@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"3856","inReplyTo":"Pine.LNX.4.63.0604130308150.11360@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: [PATCH] diff-options: add --stat","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2006-04-13T01:32:20Z","receivedAt":"2006-04-13T01:32:20Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 13 Apr 2006, Johannes Schindelin wrote:\n\n> I just realized that something is wrong with this patch: the first file \n> of the patchset seems to be ignored. I'll fix it tomorrow.\n\nOkay, so I lied: I fixed it today.\n\ndiff --git a/diff-tree.c b/diff-tree.c\nindex 2b79dd0..536da8e 100644\n--- a/diff-tree.c\n+++ b/diff-tree.c\n@@ -117,7 +117,8 @@ int main(int argc, const char **argv)\n \tif (opt->dense_combined_merges)\n \t\topt->diffopt.output_format = DIFF_FORMAT_PATCH;\n \n-\tif (opt->diffopt.output_format == DIFF_FORMAT_PATCH)\n+\tif (opt->diffopt.output_format == DIFF_FORMAT_DIFFSTAT ||\n+\t\t\topt->diffopt.output_format == DIFF_FORMAT_PATCH)\n \t\topt->diffopt.recursive = 1;\n \n \tdiff_tree_setup_paths(get_pathspec(prefix, argv), opt);\ndiff --git a/git.c b/git.c\nindex 5cb0d32..e4fcf92 100644\n--- a/git.c\n+++ b/git.c\n@@ -344,7 +344,8 @@ static int cmd_log(int argc, const char \n \t\t\topt.ignore_merges = 0;\n \t\tif (opt.dense_combined_merges)\n \t\t\topt.diffopt.output_format = DIFF_FORMAT_PATCH;\n-\t\tif (opt.diffopt.output_format == DIFF_FORMAT_PATCH)\n+\t\tif (opt.diffopt.output_format == DIFF_FORMAT_DIFFSTAT ||\n+\t\t\t\topt.diffopt.output_format == DIFF_FORMAT_PATCH)\n \t\t\topt.diffopt.recursive = 1;\n \t\tif (!full_diff && rev.prune_data)\n \t\t\tdiff_tree_setup_paths(rev.prune_data, &opt.diffopt);\n"},{"id":"18598","messageId":"7vy7ya13e3.fsf@assigned-by-dhcp.cox.net","threadId":"3856","inReplyTo":"Pine.LNX.4.63.0604130301240.28688@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: [PATCH] diff-options: add --stat","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-04-13T04:54:44Z","receivedAt":"2006-04-13T04:54:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Interesting.\n\nI wonder if you can also make this an independent option that\nprepends diffstat in front of the patch, just like the way the\nnew flag --patch-with-raw flag prepends raw output in front of\nthe patch.\n\nBy the way, I've been wondering if anybody uses the\nGIT_EXTERNAL_DIFF interface.  Does anybody miss it if we did so?\n"},{"id":"18604","messageId":"7vek02ynif.fsf@assigned-by-dhcp.cox.net","threadId":"3856","inReplyTo":"7vy7ya13e3.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] diff-options: add --stat","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-04-13T06:53:44Z","receivedAt":"2006-04-13T06:53:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <junkio@cox.net> writes:\n\n> I wonder if you can also make this an independent option that\n> prepends diffstat in front of the patch, just like the way the\n> new flag --patch-with-raw flag prepends raw output in front of\n> the patch.\n\nClarification.\n\nTraditionally, we had diff-raw and diff-patch formats.\nWe can think of --name-status and --name-only variants of\ndiff-raw (just like different --abbrev settings give different\nvisuals for diff-raw).  Until very recently, these were either-or\noutput formats, but for Cogito we added an option to show both.\n\nWe could reorganize the output format options to:\n\n\t- diff-raw and its name variants\n\t- diff-stat\n        - diff-patch\n\nand have (internally) three bools to specify which ones to\noutput, in the above order.  The recent --patch-with-raw would\nflip bit #0 (show raw) and bit #2 (show patch) on.  It is very\nlikely that diff-stat followed by diff-patch would be a popular\nformat (that is what git-format-patch does), and it also is\nconceivable that diff-raw with diff-stat but without diff-patch\nmight turn out to be useful for some people.\n\nAlso, I forgot to mention, but would it be useful to have a\ndiffstat to show --cc?  It is unclear, without much thinking,\nwhat the numbers would mean, though...\n"},{"id":"18605","messageId":"Pine.LNX.4.63.0604130859080.13529@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"3856","inReplyTo":"7vy7ya13e3.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] diff-options: add --stat","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2006-04-13T06:59:47Z","receivedAt":"2006-04-13T06:59:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 12 Apr 2006, Junio C Hamano wrote:\n\n> By the way, I've been wondering if anybody uses the\n> GIT_EXTERNAL_DIFF interface.  Does anybody miss it if we did so?\n\nYes. I use it regularly with a patched wdiff (word-by-word diff).\n\nCiao,\nDscho\n"},{"id":"18608","messageId":"Pine.LNX.4.63.0604131023250.13970@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"3856","inReplyTo":"7vek02ynif.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] diff-options: add --stat","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2006-04-13T08:29:03Z","receivedAt":"2006-04-13T08:29:03Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 12 Apr 2006, Junio C Hamano wrote:\n\n> Junio C Hamano <junkio@cox.net> writes:\n> \n> > I wonder if you can also make this an independent option that\n> > prepends diffstat in front of the patch, just like the way the\n> > new flag --patch-with-raw flag prepends raw output in front of\n> > the patch.\n> \n> Clarification.\n> \n> Traditionally, we had diff-raw and diff-patch formats.\n> We can think of --name-status and --name-only variants of\n> diff-raw (just like different --abbrev settings give different\n> visuals for diff-raw).  Until very recently, these were either-or\n> output formats, but for Cogito we added an option to show both.\n> \n> We could reorganize the output format options to:\n> \n> \t- diff-raw and its name variants\n> \t- diff-stat\n>         - diff-patch\n> \n> and have (internally) three bools to specify which ones to\n> output, in the above order.  The recent --patch-with-raw would\n> flip bit #0 (show raw) and bit #2 (show patch) on.  It is very\n> likely that diff-stat followed by diff-patch would be a popular\n> format (that is what git-format-patch does), and it also is\n> conceivable that diff-raw with diff-stat but without diff-patch\n> might turn out to be useful for some people.\n\nThat sounds plausible. Note that if diff-stat and diff-patch are turned \non, the patch generating code will be called twice. I do not think it is \nsensible (or robust, for that matter) to cache the patch for this case.\n\n> Also, I forgot to mention, but would it be useful to have a\n> diffstat to show --cc?  It is unclear, without much thinking,\n> what the numbers would mean, though...\n\nI thought long and hard about that. But how would you display it? You can \nhave lines starting with \"+ \", \" +\", \"--\", etc. The only half-way \nreasonable approach I found was to display the diffstat against each \nparent individually. Since it would be a bit involved to implement that, I \nwanted to think about it a bit longer, if it really makes sense.\n\nCiao,\nDscho\n"},{"id":"18614","messageId":"e5bfff550604131047mbcc2f8j9a12b69a024673d9@mail.gmail.com","threadId":"3856","inReplyTo":"7vek02ynif.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] diff-options: add --stat","fromName":"Marco Costalba","fromEmail":"mcostalba@gmail.com","sentAt":"2006-04-13T17:47:07Z","receivedAt":"2006-04-13T17:47:07Z","isPatch":true,"sender":{"key":"mcostalba@gmail.com","avatar":null},"body":"On 4/13/06, Junio C Hamano <junkio@cox.net> wrote:\n> Junio C Hamano <junkio@cox.net> writes:\n>\n> It is very\n> likely that diff-stat followed by diff-patch would be a popular\n> format (that is what git-format-patch does),\n\nYes, it is very likely the new output format (stat+patch) will be the\ndefault in qgit diff window viewer ;-)\n\nMarco\n"},{"id":"18615","messageId":"7v64ld2uyv.fsf@assigned-by-dhcp.cox.net","threadId":"3856","inReplyTo":"Pine.LNX.4.63.0604130301240.28688@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: [PATCH] diff-options: add --stat","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-04-13T18:26:00Z","receivedAt":"2006-04-13T18:26:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> Now you can say \"git diff --stat\" (to get an idea how many changes are\n> uncommitted), or \"git log --stat\" (to get an idea how many changes were\n> committed).\n\nI like what this tries to do (as I already said), but there are\nissues with the way it does it; here are some comments.\n\n> +static int fn_diffstat(void *priv, mmbuffer_t *mb, int nbuf)\n> +{\n> +...\n> +\tfor (i = 0; i < nbuf; i++)\n> +\t\tif (mb[i].ptr[0] == '+')\n> +\t\t\tx->added++;\n> +\t\telse if (mb[i].ptr[0] == '-')\n> +\t\t\tx->deleted++;\n> +\treturn 0;\n> +}\n\nThis is broken if you have a hunk that adds, deletes or\nleaves a line that happens to begin with a plus or a minus.\n\nA demonstration.\n\n        : siamese; ./git-diff-files -p\n        diff --git a/Makefile b/Makefile\n        index 1130af4..389143e 100644\n        --- a/Makefile\n        +++ b/Makefile\n        @@ -1,3 +1,4 @@\n        +-this new line begins with a minus\n         # The default target of this Makefile is...\n         all:\n         \n        : siamese; ./git-diff-files --stat\n        ---\n         Makefile |    2 +-\n         1 files changed, 1 insertions(+), 1 deletions(-)\n\nFor an added line, xdl_emit_diffrec(rec, size, \" \", 1, ecb) is\ncalled, which gives mb[0].ptr = \" \" and mb[1].ptr = <the\ncontents of the line>; fn_diffstat() is called with (nbuf == 2).\nYou are counting the addition mark '+', but you are counting the\nminus at the beginning of '-this new line...'.\n\nA fragile workaround would be to make fn_diffstat() aware of\nwhat the caller does and look only at mb[0], with a note to the\npoor soul who needs to debug future breakage if xdiff side ever\nchanges.\n\nA less efficient but more futureproof alternative is to use\nxdiff-interface.c::xdiff_outf() to make sure your callback is\nfed one line at a time (an example of how this can be done is\nfound in combine-diff.c::combine_diff()).\n\nFor the original \"emit everything to stdout\" code Linus did,\nthis was not a problem, because that usage does not care about\nthe record boundary.  For this \"counting '+' and '-' at the\nbeginning of each hunk data\", you do.\n\n> @@ -221,43 +367,49 @@ static void builtin_diff(const char *nam\n>  \tb_two = quote_two(\"b/\", name_b);\n>  \tlbl[0] = DIFF_FILE_VALID(one) ? a_one : \"/dev/null\";\n>  \tlbl[1] = DIFF_FILE_VALID(two) ? b_two : \"/dev/null\";\n>...\n> +\tif (mmfile_is_binary(&mf1) || mmfile_is_binary(&mf2)) {\n> +\t\tif (diffstat)\n> +\t\t\tdiffstat_binary(diffstat, lbl[0]);\n> +\t\telse\n> +\t\t\tprintf(\"Binary files %s and %s differ\\n\",\n> +\t\t\t\t\tlbl[0], lbl[1]);\n> +\t} else {\n>...\n> @@ -690,16 +843,19 @@ static void run_diff_cmd(const char *pgm\n>  \t\t\t struct diff_filespec *one,\n>  \t\t\t struct diff_filespec *two,\n>  \t\t\t const char *xfrm_msg,\n> -\t\t\t int complete_rewrite)\n> +\t\t\t int complete_rewrite,\n> +\t\t\t struct diffstat_t *diffstat)\n>  {\n>  \tif (pgm) {\n> +\t\tif (diffstat)\n> +\t\t\tdie (\"Cannot use diffstat with external diff\");\n>  \t\trun_external_diff(pgm, name, other, one, two, xfrm_msg,\n>  \t\t\t\t  complete_rewrite);\n>  \t\treturn;\n>  \t}\n\nInstead of driving diffstat code from run_diff(),\nrun_diff_cmd(), and builtin_diff(), I think it would be much\ncleaner to define diff_flush_stat() as a sibling to\ndiff_flush_raw() and diff_flush_patch(), and bypass the\nrun_diff() chain.  There will be some stuff you can reuse from\nbuiltin_diff() (mostly how it interfaces with xdiff library), so\nsplit that out as a separate function, and call it from both\nbuiltin_diff() and diff_flush_stat().  Then you do not have to\nsay \"Cannot use diffstat with external diff\"; even when you\nusually use your wdiff based external diff, you would want an\noption to get diffstat, no?  This is even more true if we are to\nhave three bools to toggle which format to enable like I said in\nmy previous message with --with-raw --with-stat --with-patch\noptions.\n"},{"id":"18616","messageId":"Pine.LNX.4.64.0604131138080.14565@g5.osdl.org","threadId":"3856","inReplyTo":"7v64ld2uyv.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] diff-options: add --stat","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-04-13T18:39:22Z","receivedAt":"2006-04-13T18:39:22Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 13 Apr 2006, Junio C Hamano wrote:\n> \n> I like what this tries to do (as I already said), but there are\n> issues with the way it does it; here are some comments.\n> \n> > +static int fn_diffstat(void *priv, mmbuffer_t *mb, int nbuf)\n> > +{\n> > +...\n> > +\tfor (i = 0; i < nbuf; i++)\n> > +\t\tif (mb[i].ptr[0] == '+')\n> > +\t\t\tx->added++;\n> > +\t\telse if (mb[i].ptr[0] == '-')\n> > +\t\t\tx->deleted++;\n> > +\treturn 0;\n> > +}\n> \n> This is broken if you have a hunk that adds, deletes or\n> leaves a line that happens to begin with a plus or a minus.\n\nI think you can just depend on it being in mb[0].ptr[0], no?\n\nxdiff always gives full lines at a time, afaik, and mb[0] always ends up \nbeing available.\n\n\t\tLinus\n"},{"id":"18617","messageId":"7vwtdt1f10.fsf@assigned-by-dhcp.cox.net","threadId":"3856","inReplyTo":"Pine.LNX.4.64.0604131138080.14565@g5.osdl.org","subject":"Re: [PATCH] diff-options: add --stat","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-04-13T18:55:39Z","receivedAt":"2006-04-13T18:55:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@osdl.org> writes:\n\n> I think you can just depend on it being in mb[0].ptr[0], no?\n\nYes that happens to be the case _now_.  I just did not want to\nworry about future breakage, in case if Davide ever wants to\nchange how mb[] is prepared for whatever reason.\n"},{"id":"18619","messageId":"Pine.LNX.4.64.0604131216500.14565@g5.osdl.org","threadId":"3856","inReplyTo":"7vwtdt1f10.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] diff-options: add --stat","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-04-13T19:18:10Z","receivedAt":"2006-04-13T19:18:10Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 13 Apr 2006, Junio C Hamano wrote:\n> \n> Yes that happens to be the case _now_.  I just did not want to\n> worry about future breakage, in case if Davide ever wants to\n> change how mb[] is prepared for whatever reason.\n\nI'd be worried if we depended on an external version of xdiff, but as it \nis, we'd see all changes to our local xdiff implementation, so...\n\nIt's not like either of the statements\n - we always get whole lines\n - the first memory block is always non-empty\nis really very controversial ;)\n\n\t\tLinus\n"},{"id":"18620","messageId":"Pine.LNX.4.63.0604132151040.29391@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"3856","inReplyTo":"7v64ld2uyv.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] diff-options: add --stat","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2006-04-13T20:00:56Z","receivedAt":"2006-04-13T20:00:56Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 13 Apr 2006, Junio C Hamano wrote:\n\n> For an added line, xdl_emit_diffrec(rec, size, \" \", 1, ecb) is\n> called, which gives mb[0].ptr = \" \" and mb[1].ptr = <the\n> contents of the line>; fn_diffstat() is called with (nbuf == 2).\n\nSilly me. I did not check that code, but assumed that mb Just Contains \nWhole Lines...\n\n> Instead of driving diffstat code from run_diff(),\n> run_diff_cmd(), and builtin_diff(), I think it would be much\n> cleaner to define diff_flush_stat() as a sibling to\n> diff_flush_raw() and diff_flush_patch(), and bypass the\n> run_diff() chain.\n\nI guess you're right. It is also more work :-(\n\nThere is another bug: if a file is created, you see \"ev/null\" as filename. \nUgly.\n\nI'll try to fix it.\n\nCiao,\nDscho\n"}]}