[PATCH 0/7] Rework diff options

[PATCH 0/7] Rework diff options

[Timo Hirvonen]

This patch series cleans up diff output format options.

This makes it possible to use any combination of --raw, -p, --stat and
--summary options and they work as you would expect.

These patches passed all test and are for the next branch. Patches 6 and
7 are optional.

 b/builtin-diff-files.c  |   10 --
 b/builtin-diff-index.c  |    3 
 b/builtin-diff-stages.c |    3 
 b/builtin-diff-tree.c   |    3 
 b/builtin-diff.c        |   62 +++----------
 b/builtin-log.c         |   13 +-
 b/combine-diff.c        |   55 +++++------
 b/diff.c                |  221 +++++++++++++++++++++++-------------------------
 b/diff.h                |   27 +++--
 b/log-tree.c            |   10 +-
 b/revision.c            |    4 
 11 files changed, 189 insertions(+), 222 deletions(-)

-- 
http://onion.dynserv.net/~timo/

[PATCH 1/7] Clean up diff.c

[Timo Hirvonen]

Signed-off-by: Timo Hirvonen <tihirvon@gmail.com>
---
 diff.c |   18 ++++++------------
 1 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/diff.c b/diff.c
index ad77543..f358546 100644
--- a/diff.c
+++ b/diff.c
@@ -203,7 +203,7 @@ static void emit_rewrite_diff(const char
 static int fill_mmfile(mmfile_t *mf, struct diff_filespec *one)
 {
 	if (!DIFF_FILE_VALID(one)) {
-		mf->ptr = ""; /* does not matter */
+		mf->ptr = (char *)""; /* does not matter */
 		mf->size = 0;
 		return 0;
 	}
@@ -395,7 +395,7 @@ static void show_stats(struct diffstat_t
 	}
 
 	for (i = 0; i < data->nr; i++) {
-		char *prefix = "";
+		const char *prefix = "";
 		char *name = data->files[i]->name;
 		int added = data->files[i]->added;
 		int deleted = data->files[i]->deleted;
@@ -918,7 +918,7 @@ int diff_populate_filespec(struct diff_f
 			err_empty:
 				err = -1;
 			empty:
-				s->data = "";
+				s->data = (char *)"";
 				s->size = 0;
 				return err;
 			}
@@ -1409,7 +1409,7 @@ int diff_setup_done(struct diff_options 
 	return 0;
 }
 
-int opt_arg(const char *arg, int arg_short, const char *arg_long, int *val)
+static int opt_arg(const char *arg, int arg_short, const char *arg_long, int *val)
 {
 	char c, *eq;
 	int len;
@@ -1725,16 +1725,12 @@ static void diff_flush_raw(struct diff_f
 		free((void*)path_two);
 }
 
-static void diff_flush_name(struct diff_filepair *p,
-			    int inter_name_termination,
-			    int line_termination)
+static void diff_flush_name(struct diff_filepair *p, int line_termination)
 {
 	char *path = p->two->path;
 
 	if (line_termination)
 		path = quote_one(p->two->path);
-	else
-		path = p->two->path;
 	printf("%s%c", path, line_termination);
 	if (p->two->path != path)
 		free(path);
@@ -1955,9 +1951,7 @@ static void flush_one_pair(struct diff_f
 				       options, diff_output_format);
 			break;
 		case DIFF_FORMAT_NAME:
-			diff_flush_name(p,
-					inter_name_termination,
-					line_termination);
+			diff_flush_name(p, line_termination);
 			break;
 		case DIFF_FORMAT_NO_OUTPUT:
 			break;
-- 
1.4.1.rc1.g8637

[PATCH 2/7] Merge with_raw, with_stat and summary variables to output_format

[Timo Hirvonen]

DIFF_FORMAT_* are now bit-flags instead of enumerated values.

Signed-off-by: Timo Hirvonen <tihirvon@gmail.com>
---
 builtin-diff.c |    2 -
 builtin-log.c  |    4 -
 combine-diff.c |   55 +++++++----------
 diff.c         |  183 +++++++++++++++++++++++++++-----------------------------
 diff.h         |   27 +++++---
 log-tree.c     |    9 ++-
 6 files changed, 136 insertions(+), 144 deletions(-)

diff --git a/builtin-diff.c b/builtin-diff.c
index 99a2f76..3b44296 100644
--- a/builtin-diff.c
+++ b/builtin-diff.c
@@ -56,7 +56,7 @@ static int builtin_diff_files(struct rev
 	    3 < revs->max_count)
 		usage(builtin_diff_usage);
 	if (revs->max_count < 0 &&
-	    (revs->diffopt.output_format == DIFF_FORMAT_PATCH))
+	    (revs->diffopt.output_format & DIFF_FORMAT_PATCH))
 		revs->combine_merges = revs->dense_combined_merges = 1;
 	/*
 	 * Backward compatibility wart - "diff-files -s" used to
diff --git a/builtin-log.c b/builtin-log.c
index 5a8a50b..5c656bc 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -176,11 +176,9 @@ int cmd_format_patch(int argc, const cha
 	rev.commit_format = CMIT_FMT_EMAIL;
 	rev.verbose_header = 1;
 	rev.diff = 1;
-	rev.diffopt.with_raw = 0;
-	rev.diffopt.with_stat = 1;
 	rev.combine_merges = 0;
 	rev.ignore_merges = 1;
-	rev.diffopt.output_format = DIFF_FORMAT_PATCH;
+	rev.diffopt.output_format = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_PATCH;
 
 	git_config(git_format_config);
 	rev.extra_headers = extra_headers;
diff --git a/combine-diff.c b/combine-diff.c
index 64b20cc..3daa8cb 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -771,7 +771,7 @@ static void show_raw_diff(struct combine
 	if (rev->loginfo)
 		show_log(rev, rev->loginfo, "\n");
 
-	if (opt->output_format == DIFF_FORMAT_RAW) {
+	if (opt->output_format & DIFF_FORMAT_RAW) {
 		offset = strlen(COLONS) - num_parent;
 		if (offset < 0)
 			offset = 0;
@@ -791,8 +791,7 @@ static void show_raw_diff(struct combine
 		printf(" %s ", diff_unique_abbrev(p->sha1, opt->abbrev));
 	}
 
-	if (opt->output_format == DIFF_FORMAT_RAW ||
-	    opt->output_format == DIFF_FORMAT_NAME_STATUS) {
+	if (opt->output_format & (DIFF_FORMAT_RAW | DIFF_FORMAT_NAME_STATUS)) {
 		for (i = 0; i < num_parent; i++)
 			putchar(p->parent[i].status);
 		putchar(inter_name_termination);
@@ -818,17 +817,12 @@ void show_combined_diff(struct combine_d
 	struct diff_options *opt = &rev->diffopt;
 	if (!p->len)
 		return;
-	switch (opt->output_format) {
-	case DIFF_FORMAT_RAW:
-	case DIFF_FORMAT_NAME_STATUS:
-	case DIFF_FORMAT_NAME:
+	if (opt->output_format & (DIFF_FORMAT_RAW |
+				  DIFF_FORMAT_NAME |
+				  DIFF_FORMAT_NAME_STATUS)) {
 		show_raw_diff(p, num_parent, rev);
-		return;
-	case DIFF_FORMAT_PATCH:
+	} else if (opt->output_format & DIFF_FORMAT_PATCH) {
 		show_patch_diff(p, num_parent, dense, rev);
-		return;
-	default:
-		return;
 	}
 }
 
@@ -842,13 +836,9 @@ void diff_tree_combined(const unsigned c
 	struct diff_options diffopts;
 	struct combine_diff_path *p, *paths = NULL;
 	int i, num_paths;
-	int do_diffstat;
 
-	do_diffstat = (opt->output_format == DIFF_FORMAT_DIFFSTAT ||
-		       opt->with_stat);
 	diffopts = *opt;
-	diffopts.with_raw = 0;
-	diffopts.with_stat = 0;
+	diffopts.output_format &= ~(DIFF_FORMAT_RAW | DIFF_FORMAT_DIFFSTAT);
 	diffopts.recursive = 1;
 
 	/* find set of paths that everybody touches */
@@ -856,19 +846,18 @@ void diff_tree_combined(const unsigned c
 		/* show stat against the first parent even
 		 * when doing combined diff.
 		 */
-		if (i == 0 && do_diffstat)
-			diffopts.output_format = DIFF_FORMAT_DIFFSTAT;
+		if (i == 0 && opt->output_format & DIFF_FORMAT_DIFFSTAT)
+			diffopts.output_format |= DIFF_FORMAT_DIFFSTAT;
 		else
-			diffopts.output_format = DIFF_FORMAT_NO_OUTPUT;
+			diffopts.output_format |= DIFF_FORMAT_NO_OUTPUT;
 		diff_tree_sha1(parent[i], sha1, "", &diffopts);
 		diffcore_std(&diffopts);
 		paths = intersect_paths(paths, i, num_parent);
 
-		if (do_diffstat && rev->loginfo)
-			show_log(rev, rev->loginfo,
-				 opt->with_stat ? "---\n" : "\n");
+		if (opt->output_format & DIFF_FORMAT_DIFFSTAT && rev->loginfo)
+			show_log(rev, rev->loginfo, "---\n");
 		diff_flush(&diffopts);
-		if (opt->with_stat)
+		if (opt->output_format & DIFF_FORMAT_DIFFSTAT)
 			putchar('\n');
 	}
 
@@ -878,17 +867,21 @@ void diff_tree_combined(const unsigned c
 			num_paths++;
 	}
 	if (num_paths) {
-		if (opt->with_raw) {
-			int saved_format = opt->output_format;
-			opt->output_format = DIFF_FORMAT_RAW;
+		if (opt->output_format & (DIFF_FORMAT_RAW |
+					  DIFF_FORMAT_NAME |
+					  DIFF_FORMAT_NAME_STATUS)) {
 			for (p = paths; p; p = p->next) {
-				show_combined_diff(p, num_parent, dense, rev);
+				if (p->len)
+					show_raw_diff(p, num_parent, rev);
 			}
-			opt->output_format = saved_format;
 			putchar(opt->line_termination);
 		}
-		for (p = paths; p; p = p->next) {
-			show_combined_diff(p, num_parent, dense, rev);
+		if (opt->output_format & DIFF_FORMAT_PATCH) {
+			for (p = paths; p; p = p->next) {
+				if (p->len)
+					show_patch_diff(p, num_parent, dense,
+							rev);
+			}
 		}
 	}
 
diff --git a/diff.c b/diff.c
index f358546..bfed79c 100644
--- a/diff.c
+++ b/diff.c
@@ -1372,23 +1372,27 @@ int diff_setup_done(struct diff_options 
 	    (0 <= options->rename_limit && !options->detect_rename))
 		return -1;
 
+	if (options->output_format & DIFF_FORMAT_NO_OUTPUT)
+		options->output_format = 0;
+
+	if (options->output_format & (DIFF_FORMAT_NAME |
+				      DIFF_FORMAT_NAME_STATUS |
+				      DIFF_FORMAT_CHECKDIFF |
+				      DIFF_FORMAT_NO_OUTPUT))
+		options->output_format &= ~(DIFF_FORMAT_RAW |
+					    DIFF_FORMAT_DIFFSTAT |
+					    DIFF_FORMAT_SUMMARY |
+					    DIFF_FORMAT_PATCH);
+
 	/*
 	 * These cases always need recursive; we do not drop caller-supplied
 	 * recursive bits for other formats here.
 	 */
-	if ((options->output_format == DIFF_FORMAT_PATCH) ||
-	    (options->output_format == DIFF_FORMAT_DIFFSTAT) ||
-	    (options->output_format == DIFF_FORMAT_CHECKDIFF))
+	if (options->output_format & (DIFF_FORMAT_PATCH |
+				      DIFF_FORMAT_DIFFSTAT |
+				      DIFF_FORMAT_CHECKDIFF))
 		options->recursive = 1;
 
-	/*
-	 * These combinations do not make sense.
-	 */
-	if (options->output_format == DIFF_FORMAT_RAW)
-		options->with_raw = 0;
-	if (options->output_format == DIFF_FORMAT_DIFFSTAT)
-		options->with_stat  = 0;
-
 	if (options->detect_rename && options->rename_limit < 0)
 		options->rename_limit = diff_rename_limit_default;
 	if (options->setup & DIFF_SETUP_USE_CACHE) {
@@ -1460,22 +1464,20 @@ int diff_opt_parse(struct diff_options *
 {
 	const char *arg = av[0];
 	if (!strcmp(arg, "-p") || !strcmp(arg, "-u"))
-		options->output_format = DIFF_FORMAT_PATCH;
+		options->output_format |= DIFF_FORMAT_PATCH;
 	else if (opt_arg(arg, 'U', "unified", &options->context))
-		options->output_format = DIFF_FORMAT_PATCH;
+		options->output_format |= DIFF_FORMAT_PATCH;
 	else if (!strcmp(arg, "--patch-with-raw")) {
-		options->output_format = DIFF_FORMAT_PATCH;
-		options->with_raw = 1;
+		options->output_format |= DIFF_FORMAT_PATCH | DIFF_FORMAT_RAW;
 	}
 	else if (!strcmp(arg, "--stat"))
-		options->output_format = DIFF_FORMAT_DIFFSTAT;
+		options->output_format |= DIFF_FORMAT_DIFFSTAT;
 	else if (!strcmp(arg, "--check"))
-		options->output_format = DIFF_FORMAT_CHECKDIFF;
+		options->output_format |= DIFF_FORMAT_CHECKDIFF;
 	else if (!strcmp(arg, "--summary"))
-		options->summary = 1;
+		options->output_format |= DIFF_FORMAT_SUMMARY;
 	else if (!strcmp(arg, "--patch-with-stat")) {
-		options->output_format = DIFF_FORMAT_PATCH;
-		options->with_stat = 1;
+		options->output_format |= DIFF_FORMAT_PATCH | DIFF_FORMAT_DIFFSTAT;
 	}
 	else if (!strcmp(arg, "-z"))
 		options->line_termination = 0;
@@ -1484,19 +1486,20 @@ int diff_opt_parse(struct diff_options *
 	else if (!strcmp(arg, "--full-index"))
 		options->full_index = 1;
 	else if (!strcmp(arg, "--binary")) {
-		options->output_format = DIFF_FORMAT_PATCH;
+		options->output_format |= DIFF_FORMAT_PATCH;
 		options->full_index = options->binary = 1;
 	}
 	else if (!strcmp(arg, "--name-only"))
-		options->output_format = DIFF_FORMAT_NAME;
+		options->output_format |= DIFF_FORMAT_NAME;
 	else if (!strcmp(arg, "--name-status"))
-		options->output_format = DIFF_FORMAT_NAME_STATUS;
+		options->output_format |= DIFF_FORMAT_NAME_STATUS;
 	else if (!strcmp(arg, "-R"))
 		options->reverse_diff = 1;
 	else if (!strncmp(arg, "-S", 2))
 		options->pickaxe = arg + 2;
-	else if (!strcmp(arg, "-s"))
-		options->output_format = DIFF_FORMAT_NO_OUTPUT;
+	else if (!strcmp(arg, "-s")) {
+		options->output_format |= DIFF_FORMAT_NO_OUTPUT;
+	}
 	else if (!strncmp(arg, "-O", 2))
 		options->orderfile = arg + 2;
 	else if (!strncmp(arg, "--diff-filter=", 14))
@@ -1671,15 +1674,17 @@ const char *diff_unique_abbrev(const uns
 }
 
 static void diff_flush_raw(struct diff_filepair *p,
-			   int line_termination,
-			   int inter_name_termination,
-			   struct diff_options *options,
-			   int output_format)
+			   struct diff_options *options)
 {
 	int two_paths;
 	char status[10];
 	int abbrev = options->abbrev;
 	const char *path_one, *path_two;
+	int inter_name_termination = '\t';
+	int line_termination = options->line_termination;
+
+	if (!line_termination)
+		inter_name_termination = 0;
 
 	path_one = p->one->path;
 	path_two = p->two->path;
@@ -1708,7 +1713,7 @@ static void diff_flush_raw(struct diff_f
 		two_paths = 0;
 		break;
 	}
-	if (output_format != DIFF_FORMAT_NAME_STATUS) {
+	if (!(options->output_format & DIFF_FORMAT_NAME_STATUS)) {
 		printf(":%06o %06o %s ",
 		       p->one->mode, p->two->mode,
 		       diff_unique_abbrev(p->one->sha1, abbrev));
@@ -1917,48 +1922,30 @@ static void diff_resolve_rename_copy(voi
 	diff_debug_queue("resolve-rename-copy done", q);
 }
 
-static void flush_one_pair(struct diff_filepair *p,
-			   int diff_output_format,
-			   struct diff_options *options,
-			   struct diffstat_t *diffstat)
+static int check_pair_status(struct diff_filepair *p)
 {
-	int inter_name_termination = '\t';
-	int line_termination = options->line_termination;
-	if (!line_termination)
-		inter_name_termination = 0;
-
 	switch (p->status) {
 	case DIFF_STATUS_UNKNOWN:
-		break;
+		return 0;
 	case 0:
 		die("internal error in diff-resolve-rename-copy");
-		break;
 	default:
-		switch (diff_output_format) {
-		case DIFF_FORMAT_DIFFSTAT:
-			diff_flush_stat(p, options, diffstat);
-			break;
-		case DIFF_FORMAT_CHECKDIFF:
-			diff_flush_checkdiff(p, options);
-			break;
-		case DIFF_FORMAT_PATCH:
-			diff_flush_patch(p, options);
-			break;
-		case DIFF_FORMAT_RAW:
-		case DIFF_FORMAT_NAME_STATUS:
-			diff_flush_raw(p, line_termination,
-				       inter_name_termination,
-				       options, diff_output_format);
-			break;
-		case DIFF_FORMAT_NAME:
-			diff_flush_name(p, line_termination);
-			break;
-		case DIFF_FORMAT_NO_OUTPUT:
-			break;
-		}
+		return 1;
 	}
 }
 
+static void flush_one_pair(struct diff_filepair *p, struct diff_options *opt)
+{
+	int fmt = opt->output_format;
+
+	if (fmt & DIFF_FORMAT_CHECKDIFF)
+		diff_flush_checkdiff(p, opt);
+	else if (fmt & (DIFF_FORMAT_RAW | DIFF_FORMAT_NAME_STATUS))
+		diff_flush_raw(p, opt);
+	else if (fmt & DIFF_FORMAT_NAME)
+		diff_flush_name(p, opt->line_termination);
+}
+
 static void show_file_mode_name(const char *newdelete, struct diff_filespec *fs)
 {
 	if (fs->mode)
@@ -2041,55 +2028,61 @@ static void diff_summary(struct diff_fil
 void diff_flush(struct diff_options *options)
 {
 	struct diff_queue_struct *q = &diff_queued_diff;
-	int i;
-	int diff_output_format = options->output_format;
-	struct diffstat_t *diffstat = NULL;
+	int i, output_format = options->output_format;
 
-	if (diff_output_format == DIFF_FORMAT_DIFFSTAT || options->with_stat) {
-		diffstat = xcalloc(sizeof (struct diffstat_t), 1);
-		diffstat->xm.consume = diffstat_consume;
-	}
+	/*
+	 * Order: raw, stat, summary, patch
+	 * or:    name/name-status/checkdiff (other bits clear)
+	 */
 
-	if (options->with_raw) {
+	if (output_format & (DIFF_FORMAT_RAW |
+			     DIFF_FORMAT_NAME |
+			     DIFF_FORMAT_NAME_STATUS |
+			     DIFF_FORMAT_CHECKDIFF)) {
 		for (i = 0; i < q->nr; i++) {
 			struct diff_filepair *p = q->queue[i];
-			flush_one_pair(p, DIFF_FORMAT_RAW, options, NULL);
+			if (check_pair_status(p))
+				flush_one_pair(p, options);
 		}
-		putchar(options->line_termination);
 	}
-	if (options->with_stat) {
+
+	if (output_format & DIFF_FORMAT_DIFFSTAT) {
+		struct diffstat_t *diffstat;
+
+		diffstat = xcalloc(sizeof (struct diffstat_t), 1);
+		diffstat->xm.consume = diffstat_consume;
 		for (i = 0; i < q->nr; i++) {
 			struct diff_filepair *p = q->queue[i];
-			flush_one_pair(p, DIFF_FORMAT_DIFFSTAT, options,
-				       diffstat);
+			if (check_pair_status(p))
+				diff_flush_stat(p, options, diffstat);
 		}
 		show_stats(diffstat);
 		free(diffstat);
-		diffstat = NULL;
-		if (options->summary)
-			for (i = 0; i < q->nr; i++)
-				diff_summary(q->queue[i]);
-		if (options->stat_sep)
-			fputs(options->stat_sep, stdout);
-		else
-			putchar(options->line_termination);
-	}
-	for (i = 0; i < q->nr; i++) {
-		struct diff_filepair *p = q->queue[i];
-		flush_one_pair(p, diff_output_format, options, diffstat);
 	}
 
-	if (diffstat) {
-		show_stats(diffstat);
-		free(diffstat);
+	if (output_format & DIFF_FORMAT_SUMMARY) {
+		for (i = 0; i < q->nr; i++)
+			diff_summary(q->queue[i]);
 	}
 
-	for (i = 0; i < q->nr; i++) {
-		if (diffstat && options->summary)
-			diff_summary(q->queue[i]);
-		diff_free_filepair(q->queue[i]);
+	if (output_format & DIFF_FORMAT_PATCH) {
+		if (output_format & (DIFF_FORMAT_DIFFSTAT |
+				     DIFF_FORMAT_SUMMARY)) {
+			if (options->stat_sep)
+				fputs(options->stat_sep, stdout);
+			else
+				putchar(options->line_termination);
+		}
+
+		for (i = 0; i < q->nr; i++) {
+			struct diff_filepair *p = q->queue[i];
+			if (check_pair_status(p))
+				diff_flush_patch(p, options);
+		}
 	}
 
+	for (i = 0; i < q->nr; i++)
+		diff_free_filepair(q->queue[i]);
 	free(q->queue);
 	q->queue = NULL;
 	q->nr = q->alloc = 0;
diff --git a/diff.h b/diff.h
index b61fdc8..2b6dc0c 100644
--- a/diff.h
+++ b/diff.h
@@ -20,19 +20,31 @@ typedef void (*add_remove_fn_t)(struct d
 		    const unsigned char *sha1,
 		    const char *base, const char *path);
 
+#define DIFF_FORMAT_RAW		0x0001
+#define DIFF_FORMAT_DIFFSTAT	0x0002
+#define DIFF_FORMAT_SUMMARY	0x0004
+#define DIFF_FORMAT_PATCH	0x0008
+
+/* These override all above */
+#define DIFF_FORMAT_NAME	0x0010
+#define DIFF_FORMAT_NAME_STATUS	0x0020
+#define DIFF_FORMAT_CHECKDIFF	0x0040
+
+/* Same as output_format = 0 but we know that -s flag was given
+ * and we should not give default value to output_format.
+ */
+#define DIFF_FORMAT_NO_OUTPUT	0x0080
+
 struct diff_options {
 	const char *filter;
 	const char *orderfile;
 	const char *pickaxe;
 	unsigned recursive:1,
-		 with_raw:1,
-		 with_stat:1,
 		 tree_in_recursive:1,
 		 binary:1,
 		 full_index:1,
 		 silent_on_remove:1,
 		 find_copies_harder:1,
-		 summary:1,
 		 color_diff:1;
 	int context;
 	int break_opt;
@@ -151,15 +163,6 @@ #define COMMON_DIFF_OPTIONS_HELP \
 "                show all files diff when -S is used and hit is found.\n"
 
 extern int diff_queue_is_empty(void);
-
-#define DIFF_FORMAT_RAW		1
-#define DIFF_FORMAT_PATCH	2
-#define DIFF_FORMAT_NO_OUTPUT	3
-#define DIFF_FORMAT_NAME	4
-#define DIFF_FORMAT_NAME_STATUS	5
-#define DIFF_FORMAT_DIFFSTAT	6
-#define DIFF_FORMAT_CHECKDIFF	7
-
 extern void diff_flush(struct diff_options*);
 
 /* diff-raw status letters */
diff --git a/log-tree.c b/log-tree.c
index ebb49f2..7d4c51f 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -163,8 +163,13 @@ int log_tree_diff_flush(struct rev_info 
 		return 0;
 	}
 
-	if (opt->loginfo && !opt->no_commit_id)
-		show_log(opt, opt->loginfo, opt->diffopt.with_stat ? "---\n" : "\n");
+	if (opt->loginfo && !opt->no_commit_id) {
+		if (opt->diffopt.output_format & DIFF_FORMAT_DIFFSTAT) {
+			show_log(opt, opt->loginfo,  "---\n");
+		} else {
+			show_log(opt, opt->loginfo,  "\n");
+		}
+	}
 	diff_flush(&opt->diffopt);
 	return 1;
 }
-- 
1.4.1.rc1.g8637

[PATCH 3/7] Make --raw option available for all diff commands

[Timo Hirvonen]

Signed-off-by: Timo Hirvonen <tihirvon@gmail.com>
---
 builtin-diff.c |   48 ++++++++++++------------------------------------
 diff.c         |    2 ++
 2 files changed, 14 insertions(+), 36 deletions(-)

diff --git a/builtin-diff.c b/builtin-diff.c
index 3b44296..91235a1 100644
--- a/builtin-diff.c
+++ b/builtin-diff.c
@@ -39,8 +39,6 @@ static int builtin_diff_files(struct rev
 			revs->max_count = 3;
 		else if (!strcmp(arg, "-q"))
 			silent = 1;
-		else if (!strcmp(arg, "--raw"))
-			revs->diffopt.output_format = DIFF_FORMAT_RAW;
 		else
 			usage(builtin_diff_usage);
 		argv++; argc--;
@@ -107,14 +105,9 @@ static int builtin_diff_b_f(struct rev_i
 	/* Blob vs file in the working tree*/
 	struct stat st;
 
-	while (1 < argc) {
-		const char *arg = argv[1];
-		if (!strcmp(arg, "--raw"))
-			revs->diffopt.output_format = DIFF_FORMAT_RAW;
-		else
-			usage(builtin_diff_usage);
-		argv++; argc--;
-	}
+	if (argc > 1)
+		usage(builtin_diff_usage);
+
 	if (lstat(path, &st))
 		die("'%s': %s", path, strerror(errno));
 	if (!(S_ISREG(st.st_mode) || S_ISLNK(st.st_mode)))
@@ -137,14 +130,9 @@ static int builtin_diff_blobs(struct rev
 	 */
 	unsigned mode = canon_mode(S_IFREG | 0644);
 
-	while (1 < argc) {
-		const char *arg = argv[1];
-		if (!strcmp(arg, "--raw"))
-			revs->diffopt.output_format = DIFF_FORMAT_RAW;
-		else
-			usage(builtin_diff_usage);
-		argv++; argc--;
-	}
+	if (argc > 1)
+		usage(builtin_diff_usage);
+
 	stuff_change(&revs->diffopt,
 		     mode, mode,
 		     blob[1].sha1, blob[0].sha1,
@@ -162,8 +150,6 @@ static int builtin_diff_index(struct rev
 		const char *arg = argv[1];
 		if (!strcmp(arg, "--cached"))
 			cached = 1;
-		else if (!strcmp(arg, "--raw"))
-			revs->diffopt.output_format = DIFF_FORMAT_RAW;
 		else
 			usage(builtin_diff_usage);
 		argv++; argc--;
@@ -185,14 +171,9 @@ static int builtin_diff_tree(struct rev_
 {
 	const unsigned char *(sha1[2]);
 	int swap = 0;
-	while (1 < argc) {
-		const char *arg = argv[1];
-		if (!strcmp(arg, "--raw"))
-			revs->diffopt.output_format = DIFF_FORMAT_RAW;
-		else
-			usage(builtin_diff_usage);
-		argv++; argc--;
-	}
+
+	if (argc > 1)
+		usage(builtin_diff_usage);
 
 	/* We saw two trees, ent[0] and ent[1].
 	 * if ent[1] is unintesting, they are swapped
@@ -214,14 +195,9 @@ static int builtin_diff_combined(struct 
 	const unsigned char (*parent)[20];
 	int i;
 
-	while (1 < argc) {
-		const char *arg = argv[1];
-		if (!strcmp(arg, "--raw"))
-			revs->diffopt.output_format = DIFF_FORMAT_RAW;
-		else
-			usage(builtin_diff_usage);
-		argv++; argc--;
-	}
+	if (argc > 1)
+		usage(builtin_diff_usage);
+
 	if (!revs->dense_combined_merges && !revs->combine_merges)
 		revs->dense_combined_merges = revs->combine_merges = 1;
 	parent = xmalloc(ents * sizeof(*parent));
diff --git a/diff.c b/diff.c
index bfed79c..6e5ae77 100644
--- a/diff.c
+++ b/diff.c
@@ -1467,6 +1467,8 @@ int diff_opt_parse(struct diff_options *
 		options->output_format |= DIFF_FORMAT_PATCH;
 	else if (opt_arg(arg, 'U', "unified", &options->context))
 		options->output_format |= DIFF_FORMAT_PATCH;
+	else if (!strcmp(arg, "--raw"))
+		options->output_format |= DIFF_FORMAT_RAW;
 	else if (!strcmp(arg, "--patch-with-raw")) {
 		options->output_format |= DIFF_FORMAT_PATCH | DIFF_FORMAT_RAW;
 	}
-- 
1.4.1.rc1.g8637

[PATCH 4/7] Set default diff output format after parsing command line

[Timo Hirvonen]

Initialize output_format to 0 instead of DIFF_FORMAT_RAW so that we can see
later if any command line options changed it.  Default value is set only if
output format was not specified.

Signed-off-by: Timo Hirvonen <tihirvon@gmail.com>
---
 builtin-diff-files.c  |    3 +++
 builtin-diff-index.c  |    3 +++
 builtin-diff-stages.c |    3 +++
 builtin-diff-tree.c   |    3 +++
 builtin-diff.c        |    4 +++-
 builtin-log.c         |    4 +++-
 diff.c                |    1 -
 7 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/builtin-diff-files.c b/builtin-diff-files.c
index 5afc1d7..a655eea 100644
--- a/builtin-diff-files.c
+++ b/builtin-diff-files.c
@@ -36,6 +36,9 @@ int cmd_diff_files(int argc, const char 
 			usage(diff_files_usage);
 		argv++; argc--;
 	}
+	if (!rev.diffopt.output_format)
+		rev.diffopt.output_format = DIFF_FORMAT_RAW;
+
 	/*
 	 * Make sure there are NO revision (i.e. pending object) parameter,
 	 * rev.max_count is reasonable (0 <= n <= 3),
diff --git a/builtin-diff-index.c b/builtin-diff-index.c
index c42ef9a..b37c9e8 100644
--- a/builtin-diff-index.c
+++ b/builtin-diff-index.c
@@ -28,6 +28,9 @@ int cmd_diff_index(int argc, const char 
 		else
 			usage(diff_cache_usage);
 	}
+	if (!rev.diffopt.output_format)
+		rev.diffopt.output_format = DIFF_FORMAT_RAW;
+
 	/*
 	 * Make sure there is one revision (i.e. pending object),
 	 * and there is no revision filtering parameters.
diff --git a/builtin-diff-stages.c b/builtin-diff-stages.c
index 7c157ca..30931fe 100644
--- a/builtin-diff-stages.c
+++ b/builtin-diff-stages.c
@@ -85,6 +85,9 @@ int cmd_diff_stages(int ac, const char *
 		ac--; av++;
 	}
 
+	if (!diff_options.output_format)
+		diff_options.output_format = DIFF_FORMAT_RAW;
+
 	if (ac < 3 ||
 	    sscanf(av[1], "%d", &stage1) != 1 ||
 	    ! (0 <= stage1 && stage1 <= 3) ||
diff --git a/builtin-diff-tree.c b/builtin-diff-tree.c
index 3409a39..ae1cde9 100644
--- a/builtin-diff-tree.c
+++ b/builtin-diff-tree.c
@@ -84,6 +84,9 @@ int cmd_diff_tree(int argc, const char *
 		usage(diff_tree_usage);
 	}
 
+	if (!opt->diffopt.output_format)
+		opt->diffopt.output_format = DIFF_FORMAT_RAW;
+
 	/*
 	 * NOTE! We expect "a ^b" to be equal to "a..b", so we
 	 * reverse the order of the objects if the second one
diff --git a/builtin-diff.c b/builtin-diff.c
index 91235a1..47e0a37 100644
--- a/builtin-diff.c
+++ b/builtin-diff.c
@@ -252,9 +252,11 @@ int cmd_diff(int argc, const char **argv
 
 	git_config(git_diff_config);
 	init_revisions(&rev);
-	rev.diffopt.output_format = DIFF_FORMAT_PATCH;
 
 	argc = setup_revisions(argc, argv, &rev, NULL);
+	if (!rev.diffopt.output_format)
+		rev.diffopt.output_format = DIFF_FORMAT_PATCH;
+
 	/* Do we have --cached and not have a pending object, then
 	 * default to HEAD by hand.  Eek.
 	 */
diff --git a/builtin-log.c b/builtin-log.c
index 5c656bc..65f9527 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -178,7 +178,6 @@ int cmd_format_patch(int argc, const cha
 	rev.diff = 1;
 	rev.combine_merges = 0;
 	rev.ignore_merges = 1;
-	rev.diffopt.output_format = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_PATCH;
 
 	git_config(git_format_config);
 	rev.extra_headers = extra_headers;
@@ -247,6 +246,9 @@ int cmd_format_patch(int argc, const cha
 	if (argc > 1)
 		die ("unrecognized argument: %s", argv[1]);
 
+	if (!rev.diffopt.output_format)
+		rev.diffopt.output_format = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_PATCH;
+
 	if (output_directory) {
 		if (use_stdout)
 			die("standard output, or directory, which one?");
diff --git a/diff.c b/diff.c
index 6e5ae77..6be31e7 100644
--- a/diff.c
+++ b/diff.c
@@ -1355,7 +1355,6 @@ static void run_checkdiff(struct diff_fi
 void diff_setup(struct diff_options *options)
 {
 	memset(options, 0, sizeof(*options));
-	options->output_format = DIFF_FORMAT_RAW;
 	options->line_termination = '\n';
 	options->break_opt = -1;
 	options->rename_limit = -1;
-- 
1.4.1.rc1.g8637

[PATCH 5/7] DIFF_FORMAT_RAW is not default anymore

[Timo Hirvonen]

diff_setup() used to initialize output_format to DIFF_FORMAT_RAW.  Now
the default is 0 (no output) so don't compare against DIFF_FORMAT_RAW to
see if any diff format command line flags were given.

Signed-off-by: Timo Hirvonen <tihirvon@gmail.com>
---
 builtin-log.c |    5 +----
 revision.c    |    3 +--
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/builtin-log.c b/builtin-log.c
index 65f9527..f173070 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -24,11 +24,8 @@ static int cmd_log_wc(int argc, const ch
 	rev->verbose_header = 1;
 	argc = setup_revisions(argc, argv, rev, "HEAD");
 	if (rev->always_show_header) {
-		if (rev->diffopt.pickaxe || rev->diffopt.filter) {
+		if (rev->diffopt.pickaxe || rev->diffopt.filter)
 			rev->always_show_header = 0;
-			if (rev->diffopt.output_format == DIFF_FORMAT_RAW)
-				rev->diffopt.output_format = DIFF_FORMAT_NO_OUTPUT;
-		}
 	}
 
 	if (argc > 1)
diff --git a/revision.c b/revision.c
index b963f2a..ae4ca82 100644
--- a/revision.c
+++ b/revision.c
@@ -851,8 +851,7 @@ int setup_revisions(int argc, const char
 	}
 	if (revs->combine_merges) {
 		revs->ignore_merges = 0;
-		if (revs->dense_combined_merges &&
-		    (revs->diffopt.output_format != DIFF_FORMAT_DIFFSTAT))
+		if (revs->dense_combined_merges && !revs->diffopt.output_format)
 			revs->diffopt.output_format = DIFF_FORMAT_PATCH;
 	}
 	revs->diffopt.abbrev = revs->abbrev;
-- 
1.4.1.rc1.g8637

[PATCH 6/7] --name-only, --name-status, --check and -s are mutually exclusive

[Timo Hirvonen]

Signed-off-by: Timo Hirvonen <tihirvon@gmail.com>
---
 diff.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/diff.c b/diff.c
index 6be31e7..4b1b4eb 100644
--- a/diff.c
+++ b/diff.c
@@ -1366,6 +1366,19 @@ void diff_setup(struct diff_options *opt
 
 int diff_setup_done(struct diff_options *options)
 {
+	int count = 0;
+
+	if (options->output_format & DIFF_FORMAT_NAME)
+		count++;
+	if (options->output_format & DIFF_FORMAT_NAME_STATUS)
+		count++;
+	if (options->output_format & DIFF_FORMAT_CHECKDIFF)
+		count++;
+	if (options->output_format & DIFF_FORMAT_NO_OUTPUT)
+		count++;
+	if (count > 1)
+		die("--name-only, --name-status, --check and -s are mutually exclusive");
+
 	if ((options->find_copies_harder &&
 	     options->detect_rename != DIFF_DETECT_COPY) ||
 	    (0 <= options->rename_limit && !options->detect_rename))
-- 
1.4.1.rc1.g8637

[PATCH 7/7] Remove awkward compatibility warts

[Timo Hirvonen]

Signed-off-by: Timo Hirvonen <tihirvon@gmail.com>
---
 builtin-diff-files.c |    7 -------
 builtin-diff.c       |    7 -------
 2 files changed, 0 insertions(+), 14 deletions(-)

diff --git a/builtin-diff-files.c b/builtin-diff-files.c
index a655eea..3361898 100644
--- a/builtin-diff-files.c
+++ b/builtin-diff-files.c
@@ -47,12 +47,5 @@ int cmd_diff_files(int argc, const char 
 	if (rev.pending.nr ||
 	    rev.min_age != -1 || rev.max_age != -1)
 		usage(diff_files_usage);
-	/*
-	 * Backward compatibility wart - "diff-files -s" used to
-	 * defeat the common diff option "-s" which asked for
-	 * DIFF_FORMAT_NO_OUTPUT.
-	 */
-	if (rev.diffopt.output_format == DIFF_FORMAT_NO_OUTPUT)
-		rev.diffopt.output_format = DIFF_FORMAT_RAW;
 	return run_diff_files(&rev, silent);
 }
diff --git a/builtin-diff.c b/builtin-diff.c
index 47e0a37..076eb09 100644
--- a/builtin-diff.c
+++ b/builtin-diff.c
@@ -56,13 +56,6 @@ static int builtin_diff_files(struct rev
 	if (revs->max_count < 0 &&
 	    (revs->diffopt.output_format & DIFF_FORMAT_PATCH))
 		revs->combine_merges = revs->dense_combined_merges = 1;
-	/*
-	 * Backward compatibility wart - "diff-files -s" used to
-	 * defeat the common diff option "-s" which asked for
-	 * DIFF_FORMAT_NO_OUTPUT.
-	 */
-	if (revs->diffopt.output_format == DIFF_FORMAT_NO_OUTPUT)
-		revs->diffopt.output_format = DIFF_FORMAT_RAW;
 	return run_diff_files(revs, silent);
 }
 
-- 
1.4.1.rc1.g8637

Re: [PATCH 2/7] Merge with_raw, with_stat and summary variables to output_format

[Johannes Schindelin]

Hi,

thank you very much for doing the extra step and using the original 
constant names. I appreciate that.

On Sat, 24 Jun 2006, Timo Hirvonen wrote:

> @@ -818,17 +817,12 @@ void show_combined_diff(struct combine_d
>  	struct diff_options *opt = &rev->diffopt;
>  	if (!p->len)
>  		return;
> -	switch (opt->output_format) {
> -	case DIFF_FORMAT_RAW:
> -	case DIFF_FORMAT_NAME_STATUS:
> -	case DIFF_FORMAT_NAME:
> +	if (opt->output_format & (DIFF_FORMAT_RAW |
> +				  DIFF_FORMAT_NAME |
> +				  DIFF_FORMAT_NAME_STATUS)) {
>  		show_raw_diff(p, num_parent, rev);
> -		return;
> -	case DIFF_FORMAT_PATCH:
> +	} else if (opt->output_format & DIFF_FORMAT_PATCH) {

Not that it matters, but this "else" could go. (Otherwise,  "--raw -p" 
would be the same as "--raw", right?)

>  		show_patch_diff(p, num_parent, dense, rev);
> -		return;
> -	default:
> -		return;
>  	}
>  }

> @@ -856,19 +846,18 @@ void diff_tree_combined(const unsigned c
> [...]
>  
> -		if (do_diffstat && rev->loginfo)
> -			show_log(rev, rev->loginfo,
> -				 opt->with_stat ? "---\n" : "\n");
> +		if (opt->output_format & DIFF_FORMAT_DIFFSTAT && rev->loginfo)
> +			show_log(rev, rev->loginfo, "---\n");
>  		diff_flush(&diffopts);
> -		if (opt->with_stat)
> +		if (opt->output_format & DIFF_FORMAT_DIFFSTAT)
>  			putchar('\n');
>  	}

Just a remark: this hunk actually changes behaviour. "with_stat" meant 
that the stat was prepended before something like a patch, and therefore a 
separator was needed. If you pass only "--stat", the separator will be 
printed anyway now.

> diff --git a/diff.c b/diff.c
> index f358546..bfed79c 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -1372,23 +1372,27 @@ int diff_setup_done(struct diff_options 
>  	    (0 <= options->rename_limit && !options->detect_rename))
>  		return -1;
>  
> +	if (options->output_format & DIFF_FORMAT_NO_OUTPUT)
> +		options->output_format = 0;
> +
> +	if (options->output_format & (DIFF_FORMAT_NAME |
> +				      DIFF_FORMAT_NAME_STATUS |
> +				      DIFF_FORMAT_CHECKDIFF |
> +				      DIFF_FORMAT_NO_OUTPUT))

The DIFF_FORMAT_NO_OUTPUT here makes no sense (if it was set, you unset it 
above).

> @@ -1671,15 +1674,17 @@ const char *diff_unique_abbrev(const uns
> [...]
>  
>  static void diff_flush_raw(struct diff_filepair *p,
> -			   int line_termination,
> -			   int inter_name_termination,
> -			   struct diff_options *options,
> -			   int output_format)
> +			   struct diff_options *options)
>  {
>  	int two_paths;
>  	char status[10];
>  	int abbrev = options->abbrev;
>  	const char *path_one, *path_two;
> +	int inter_name_termination = '\t';
> +	int line_termination = options->line_termination;
> +
> +	if (!line_termination)
> +		inter_name_termination = 0;

<nit type=minor>
	This should be part of patch 1/7.
</nit>

> @@ -2041,55 +2028,61 @@ static void diff_summary(struct diff_fil
> [...]
>  
> -	if (options->with_raw) {
> +	if (output_format & (DIFF_FORMAT_RAW |
> +			     DIFF_FORMAT_NAME |
> +			     DIFF_FORMAT_NAME_STATUS |
> +			     DIFF_FORMAT_CHECKDIFF)) {
>  		for (i = 0; i < q->nr; i++) {
>  			struct diff_filepair *p = q->queue[i];
> -			flush_one_pair(p, DIFF_FORMAT_RAW, options, NULL);
> +			if (check_pair_status(p))
> +				flush_one_pair(p, options);

This is a very nice cleanup.

>  	}
> -	if (options->with_stat) {
> +
> +	if (output_format & DIFF_FORMAT_DIFFSTAT) {
> +		struct diffstat_t *diffstat;
> +
> +		diffstat = xcalloc(sizeof (struct diffstat_t), 1);
> +		diffstat->xm.consume = diffstat_consume;
>  		for (i = 0; i < q->nr; i++) {
>  			struct diff_filepair *p = q->queue[i];
> -			flush_one_pair(p, DIFF_FORMAT_DIFFSTAT, options,
> -				       diffstat);
> +			if (check_pair_status(p))
> +				diff_flush_stat(p, options, diffstat);

Again, very nice.

>  		}
>  		show_stats(diffstat);
>  		free(diffstat);

Why not go the full nine yards, and make diffstat not a pointer, but the 
struct itself? You would avoid calloc()ing and free()ing. (Of course, 
instead of calloc()ing you have to memset() it to 0.)

> +	if (output_format & DIFF_FORMAT_PATCH) {
> +		if (output_format & (DIFF_FORMAT_DIFFSTAT |
> +				     DIFF_FORMAT_SUMMARY)) {
> +			if (options->stat_sep)
> +				fputs(options->stat_sep, stdout);
> +			else
> +				putchar(options->line_termination);

Are we sure we do not want something like

	if (output_format / DIFF_FORMAT_DIFFSTAT > 1)
		/* output separator */

after each format (this example being after the diffstat), the condition 
being: if there is still an output format to come, add the separator?

All in all, I like this patch.

Ciao,
Dscho

Re: [PATCH 2/7] Merge with_raw, with_stat and summary variables to output_format

[Timo Hirvonen]

Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

> Hi,
> 
> thank you very much for doing the extra step and using the original 
> constant names. I appreciate that.
> 
> On Sat, 24 Jun 2006, Timo Hirvonen wrote:
> 
> > @@ -818,17 +817,12 @@ void show_combined_diff(struct combine_d
> >  	struct diff_options *opt = &rev->diffopt;
> >  	if (!p->len)
> >  		return;
> > -	switch (opt->output_format) {
> > -	case DIFF_FORMAT_RAW:
> > -	case DIFF_FORMAT_NAME_STATUS:
> > -	case DIFF_FORMAT_NAME:
> > +	if (opt->output_format & (DIFF_FORMAT_RAW |
> > +				  DIFF_FORMAT_NAME |
> > +				  DIFF_FORMAT_NAME_STATUS)) {
> >  		show_raw_diff(p, num_parent, rev);
> > -		return;
> > -	case DIFF_FORMAT_PATCH:
> > +	} else if (opt->output_format & DIFF_FORMAT_PATCH) {
> 
> Not that it matters, but this "else" could go. (Otherwise,  "--raw -p" 
> would be the same as "--raw", right?)

Just tested, ./git log -p --raw displays both raw and patch.  I think it
works because I changed diff_tree_combined() to use show_raw_diff() and
show_patch_diff() directly.

It feels 'wrong' to check flags and then call a function which checks
the flags again.  This combined diff stuff is confusing.

> > @@ -856,19 +846,18 @@ void diff_tree_combined(const unsigned c
> > [...]
> >  
> > -		if (do_diffstat && rev->loginfo)
> > -			show_log(rev, rev->loginfo,
> > -				 opt->with_stat ? "---\n" : "\n");
> > +		if (opt->output_format & DIFF_FORMAT_DIFFSTAT && rev->loginfo)
> > +			show_log(rev, rev->loginfo, "---\n");
> >  		diff_flush(&diffopts);
> > -		if (opt->with_stat)
> > +		if (opt->output_format & DIFF_FORMAT_DIFFSTAT)
> >  			putchar('\n');
> >  	}
> 
> Just a remark: this hunk actually changes behaviour. "with_stat" meant 
> that the stat was prepended before something like a patch, and therefore a 
> separator was needed. If you pass only "--stat", the separator will be 
> printed anyway now.

You are right, it now prints --- when it should print empty line.

> > +	int inter_name_termination = '\t';
> > +	int line_termination = options->line_termination;
> > +
> > +	if (!line_termination)
> > +		inter_name_termination = 0;
> 
> <nit type=minor>
> 	This should be part of patch 1/7.
> </nit>

That clean up was possible only after I made other changes to the code,
I think.  At least it wasn't obvious when I wrote 1/7.

> >  		show_stats(diffstat);
> >  		free(diffstat);
> 
> Why not go the full nine yards, and make diffstat not a pointer, but the 
> struct itself? You would avoid calloc()ing and free()ing. (Of course, 
> instead of calloc()ing you have to memset() it to 0.)

I was blind :)

> > +	if (output_format & DIFF_FORMAT_PATCH) {
> > +		if (output_format & (DIFF_FORMAT_DIFFSTAT |
> > +				     DIFF_FORMAT_SUMMARY)) {
> > +			if (options->stat_sep)
> > +				fputs(options->stat_sep, stdout);
> > +			else
> > +				putchar(options->line_termination);
> 
> Are we sure we do not want something like
> 
> 	if (output_format / DIFF_FORMAT_DIFFSTAT > 1)
> 		/* output separator */
> 
> after each format (this example being after the diffstat), the condition 
> being: if there is still an output format to come, add the separator?

I'm not sure what you mean.

It outputs separator between (diffstat and/or summary) and patch.
There's no separator between diffstat and summary or raw and diffstat.
Should there be one?


Thanks for your comments.  Should I patch the patch or send a fixed one?
I'm currently too tired to write any code.

-- 
http://onion.dynserv.net/~timo/

Re: [PATCH 2/7] Merge with_raw, with_stat and summary variables to output_format

[Johannes Schindelin]

Hi,

On Sun, 25 Jun 2006, Timo Hirvonen wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> 
> > On Sat, 24 Jun 2006, Timo Hirvonen wrote:
> > 
> > > @@ -818,17 +817,12 @@ void show_combined_diff(struct combine_d
> > >  	struct diff_options *opt = &rev->diffopt;
> > >  	if (!p->len)
> > >  		return;
> > > -	switch (opt->output_format) {
> > > -	case DIFF_FORMAT_RAW:
> > > -	case DIFF_FORMAT_NAME_STATUS:
> > > -	case DIFF_FORMAT_NAME:
> > > +	if (opt->output_format & (DIFF_FORMAT_RAW |
> > > +				  DIFF_FORMAT_NAME |
> > > +				  DIFF_FORMAT_NAME_STATUS)) {
> > >  		show_raw_diff(p, num_parent, rev);
> > > -		return;
> > > -	case DIFF_FORMAT_PATCH:
> > > +	} else if (opt->output_format & DIFF_FORMAT_PATCH) {
> > 
> > Not that it matters, but this "else" could go. (Otherwise,  "--raw -p" 
> > would be the same as "--raw", right?)
> 
> Just tested, ./git log -p --raw displays both raw and patch.  I think it
> works because I changed diff_tree_combined() to use show_raw_diff() and
> show_patch_diff() directly.
> 
> It feels 'wrong' to check flags and then call a function which checks
> the flags again.  This combined diff stuff is confusing.

Sorry for not checking the result, but just the patch. I also find this 
behaviour confusing. Junio?

> > > +	int inter_name_termination = '\t';
> > > +	int line_termination = options->line_termination;
> > > +
> > > +	if (!line_termination)
> > > +		inter_name_termination = 0;
> > 
> > <nit type=minor>
> > 	This should be part of patch 1/7.
> > </nit>
> 
> That clean up was possible only after I made other changes to the code,
> I think.  At least it wasn't obvious when I wrote 1/7.

It is just a minor nit, I do not think it is necessary to change the 
patch.

> > >  		show_stats(diffstat);
> > >  		free(diffstat);
> > 
> > Why not go the full nine yards, and make diffstat not a pointer, but the 
> > struct itself? You would avoid calloc()ing and free()ing. (Of course, 
> > instead of calloc()ing you have to memset() it to 0.)
> 
> I was blind :)

;-) In my experience, after staring at the code too long, you turn blind. 
This is why I like a second pair of eyeballs so much.

> > > +	if (output_format & DIFF_FORMAT_PATCH) {
> > > +		if (output_format & (DIFF_FORMAT_DIFFSTAT |
> > > +				     DIFF_FORMAT_SUMMARY)) {
> > > +			if (options->stat_sep)
> > > +				fputs(options->stat_sep, stdout);
> > > +			else
> > > +				putchar(options->line_termination);
> > 
> > Are we sure we do not want something like
> > 
> > 	if (output_format / DIFF_FORMAT_DIFFSTAT > 1)
> > 		/* output separator */
> > 
> > after each format (this example being after the diffstat), the condition 
> > being: if there is still an output format to come, add the separator?
> 
> I'm not sure what you mean.
> 
> It outputs separator between (diffstat and/or summary) and patch.
> There's no separator between diffstat and summary or raw and diffstat.
> Should there be one?

IMHO there should be one.

> Thanks for your comments.  Should I patch the patch or send a fixed one?

I cannot speak for Junio, but I think an additional patch to clean things 
up would be the way to go.

Ciao,
Dscho

Re: [PATCH 0/7] Rework diff options

[Junio C Hamano]

Timo Hirvonen <tihirvon@gmail.com> writes:

> This patch series cleans up diff output format options.
>
> This makes it possible to use any combination of --raw, -p, --stat and
> --summary options and they work as you would expect.
>
> These patches passed all test and are for the next branch. Patches 6 and
> 7 are optional.

Thanks, very nicely done.  Tentatively placed all of them in
"pu"; the first "clean-up" is in "master".

Here are a few problems I have seen:

 - "git show --stat HEAD" gives '---' marker as Johannes and you
   have already discussed (I do not mind this that much though);

 - "--cc" seems to be quite broken.  "git show v1.0.0" nor "git
   diff-tree --pretty --cc v1.0.0" does not give the log
   message, and gives something quite confused instead.  I think
   it is showing "-m -p" followed by "--cc".

We may find more minor breakages, in addition to these, but I am
reasonably sure we should be able to fix them in-tree.

[PATCH] Add msg_sep to diff_options

[Timo Hirvonen]

Add msg_sep variable to struct diff_options.  msg_sep is printed after
commit message.  Default is "\n", format-patch sets it to "---\n".

This also removes the second argument from show_log() because all
callers derived it from the first argument:

    show_log(rev, rev->loginfo, ...

Signed-off-by: Timo Hirvonen <tihirvon@gmail.com>
---

  I'm not 100% sure if format-patch is the only one wanting "---\n".
  But I think "\n" should be used for every command that doesn't create
  patches.

 builtin-log.c  |    1 +
 combine-diff.c |    7 ++++---
 diff.c         |    1 +
 diff.h         |    1 +
 log-tree.c     |   15 ++++++---------
 log-tree.h     |    2 +-
 6 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/builtin-log.c b/builtin-log.c
index f173070..5b3fadc 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -175,6 +175,7 @@ int cmd_format_patch(int argc, const cha
 	rev.diff = 1;
 	rev.combine_merges = 0;
 	rev.ignore_merges = 1;
+	rev.diffopt.msg_sep = "---\n";
 
 	git_config(git_format_config);
 	rev.extra_headers = extra_headers;
diff --git a/combine-diff.c b/combine-diff.c
index 3daa8cb..39fb10c 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -701,7 +701,7 @@ static int show_patch_diff(struct combin
 		const char *abb;
 
 		if (rev->loginfo)
-			show_log(rev, rev->loginfo, "\n");
+			show_log(rev, opt->msg_sep);
 		dump_quoted_path(dense ? "diff --cc " : "diff --combined ", elem->path);
 		printf("index ");
 		for (i = 0; i < num_parent; i++) {
@@ -769,7 +769,7 @@ static void show_raw_diff(struct combine
 		inter_name_termination = 0;
 
 	if (rev->loginfo)
-		show_log(rev, rev->loginfo, "\n");
+		show_log(rev, opt->msg_sep);
 
 	if (opt->output_format & DIFF_FORMAT_RAW) {
 		offset = strlen(COLONS) - num_parent;
@@ -855,7 +855,8 @@ void diff_tree_combined(const unsigned c
 		paths = intersect_paths(paths, i, num_parent);
 
 		if (opt->output_format & DIFF_FORMAT_DIFFSTAT && rev->loginfo)
-			show_log(rev, rev->loginfo, "---\n");
+			show_log(rev, opt->msg_sep);
+
 		diff_flush(&diffopts);
 		if (opt->output_format & DIFF_FORMAT_DIFFSTAT)
 			putchar('\n');
diff --git a/diff.c b/diff.c
index 4b1b4eb..cc2af30 100644
--- a/diff.c
+++ b/diff.c
@@ -1359,6 +1359,7 @@ void diff_setup(struct diff_options *opt
 	options->break_opt = -1;
 	options->rename_limit = -1;
 	options->context = 3;
+	options->msg_sep = "\n";
 
 	options->change = diff_change;
 	options->add_remove = diff_addremove;
diff --git a/diff.h b/diff.h
index 2b6dc0c..729cd02 100644
--- a/diff.h
+++ b/diff.h
@@ -57,6 +57,7 @@ struct diff_options {
 	int rename_limit;
 	int setup;
 	int abbrev;
+	const char *msg_sep;
 	const char *stat_sep;
 	long xdl_opts;
 
diff --git a/log-tree.c b/log-tree.c
index 7d4c51f..ab6b682 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -43,9 +43,10 @@ static int append_signoff(char *buf, int
 	return at;
 }
 
-void show_log(struct rev_info *opt, struct log_info *log, const char *sep)
+void show_log(struct rev_info *opt, const char *sep)
 {
 	static char this_header[16384];
+	struct log_info *log = opt->loginfo;
 	struct commit *commit = log->commit, *parent = log->parent;
 	int abbrev = opt->diffopt.abbrev;
 	int abbrev_commit = opt->abbrev_commit ? opt->abbrev : 40;
@@ -163,13 +164,9 @@ int log_tree_diff_flush(struct rev_info 
 		return 0;
 	}
 
-	if (opt->loginfo && !opt->no_commit_id) {
-		if (opt->diffopt.output_format & DIFF_FORMAT_DIFFSTAT) {
-			show_log(opt, opt->loginfo,  "---\n");
-		} else {
-			show_log(opt, opt->loginfo,  "\n");
-		}
-	}
+	if (opt->loginfo && !opt->no_commit_id)
+		show_log(opt, opt->diffopt.msg_sep);
+
 	diff_flush(&opt->diffopt);
 	return 1;
 }
@@ -266,7 +263,7 @@ int log_tree_commit(struct rev_info *opt
 	shown = log_tree_diff(opt, commit, &log);
 	if (!shown && opt->loginfo && opt->always_show_header) {
 		log.parent = NULL;
-		show_log(opt, opt->loginfo, "");
+		show_log(opt, "");
 		shown = 1;
 	}
 	opt->loginfo = NULL;
diff --git a/log-tree.h b/log-tree.h
index a26e484..e82b56a 100644
--- a/log-tree.h
+++ b/log-tree.h
@@ -11,6 +11,6 @@ void init_log_tree_opt(struct rev_info *
 int log_tree_diff_flush(struct rev_info *);
 int log_tree_commit(struct rev_info *, struct commit *);
 int log_tree_opt_parse(struct rev_info *, const char **, int);
-void show_log(struct rev_info *opt, struct log_info *log, const char *sep);
+void show_log(struct rev_info *opt, const char *sep);
 
 #endif
-- 
1.4.1.rc1.g35ee-dirty

[PATCH] whatchanged: Default to DIFF_FORMAT_RAW

[Timo Hirvonen]

Signed-off-by: Timo Hirvonen <tihirvon@gmail.com>
---
 builtin-log.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/builtin-log.c b/builtin-log.c
index 5b3fadc..8a39770 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -28,6 +28,15 @@ static int cmd_log_wc(int argc, const ch
 			rev->always_show_header = 0;
 	}
 
+	if (!rev->diffopt.output_format && !rev->simplify_history) {
+		/* Ugly hack!
+		 *
+		 * rev->simplify_history == 0 -> whatchanged
+		 * Can't do this before setup_revisions()
+		 */
+		rev->diffopt.output_format = DIFF_FORMAT_RAW;
+	}
+
 	if (argc > 1)
 		die("unrecognized argument: %s", argv[1]);
 
-- 
1.4.1.rc1.g6e272-dirty

Re: [PATCH 0/7] Rework diff options

[Timo Hirvonen]

Junio C Hamano <junkio@cox.net> wrote:

> Here are a few problems I have seen:
> 
>  - "git show --stat HEAD" gives '---' marker as Johannes and you
>    have already discussed (I do not mind this that much though);

The patch I sent as a reply to 2/7 should fix this.

>  - "--cc" seems to be quite broken.  "git show v1.0.0" nor "git
>    diff-tree --pretty --cc v1.0.0" does not give the log
>    message, and gives something quite confused instead.  I think
>    it is showing "-m -p" followed by "--cc".

Sorry about that.  I don't understand the --cc stuff very well but I try
to fix the bug.

-- 
http://onion.dynserv.net/~timo/

[PATCH] Don't xcalloc() struct diffstat_t

[Timo Hirvonen]

Signed-off-by: Timo Hirvonen <tihirvon@gmail.com>
---
 diff.c |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/diff.c b/diff.c
index cc2af30..8880150 100644
--- a/diff.c
+++ b/diff.c
@@ -2062,17 +2062,16 @@ void diff_flush(struct diff_options *opt
 	}
 
 	if (output_format & DIFF_FORMAT_DIFFSTAT) {
-		struct diffstat_t *diffstat;
+		struct diffstat_t diffstat;
 
-		diffstat = xcalloc(sizeof (struct diffstat_t), 1);
-		diffstat->xm.consume = diffstat_consume;
+		memset(&diffstat, 0, sizeof(struct diffstat_t));
+		diffstat.xm.consume = diffstat_consume;
 		for (i = 0; i < q->nr; i++) {
 			struct diff_filepair *p = q->queue[i];
 			if (check_pair_status(p))
-				diff_flush_stat(p, options, diffstat);
+				diff_flush_stat(p, options, &diffstat);
 		}
-		show_stats(diffstat);
-		free(diffstat);
+		show_stats(&diffstat);
 	}
 
 	if (output_format & DIFF_FORMAT_SUMMARY) {
-- 
1.4.1.rc1.g5472-dirty

Re: [PATCH] Add msg_sep to diff_options

[Junio C Hamano]

Timo Hirvonen <tihirvon@gmail.com> writes:

> Add msg_sep variable to struct diff_options.  msg_sep is printed after
> commit message.  Default is "\n", format-patch sets it to "---\n".
>
> This also removes the second argument from show_log() because all
> callers derived it from the first argument:
>
>     show_log(rev, rev->loginfo, ...

Good catch.  Thanks.

> Signed-off-by: Timo Hirvonen <tihirvon@gmail.com>
> ---

I often wonder if the separator should be "\n---\n" instead when
I see something like the above, but do not change it yet please.

>   I'm not 100% sure if format-patch is the only one wanting "---\n".

git log --patch-with-stat should also show "---\n".

>   But I think "\n" should be used for every command that doesn't create
>   patches.

This sounds good.

We probably would want to have an output format testsuite to
catch regression.

Re: [PATCH] whatchanged: Default to DIFF_FORMAT_RAW

[Junio C Hamano]

Timo Hirvonen <tihirvon@gmail.com> writes:

> Signed-off-by: Timo Hirvonen <tihirvon@gmail.com>
> ---
>  builtin-log.c |    9 +++++++++
>  1 files changed, 9 insertions(+), 0 deletions(-)
>
> diff --git a/builtin-log.c b/builtin-log.c
> index 5b3fadc..8a39770 100644
> --- a/builtin-log.c
> +++ b/builtin-log.c
> @@ -28,6 +28,15 @@ static int cmd_log_wc(int argc, const ch
>  			rev->always_show_header = 0;
>  	}
>  
> +	if (!rev->diffopt.output_format && !rev->simplify_history) {
> +		/* Ugly hack!
> +		 *
> +		 * rev->simplify_history == 0 -> whatchanged
> +		 * Can't do this before setup_revisions()
> +		 */

Indeed it is ugly.  Might it be a cleaner option to signal _wc
function what command its caller is, by adding an extra
parameter (or check argv -- ugh)?

[PATCH] whatchanged: Default to DIFF_FORMAT_RAW

[Timo Hirvonen]

Split cmd_log_wc() to cmd_log_init() and cmd_log_walk() and set default
diff output format for whatchanged to DIFF_FORMAT_RAW.

Signed-off-by: Timo Hirvonen <tihirvon@gmail.com>
---

  Forget the previous patch, this is cleaner.

 builtin-log.c |   27 ++++++++++++++++-----------
 1 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/builtin-log.c b/builtin-log.c
index 5b3fadc..28cd8bf 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -14,22 +14,22 @@ #include "builtin.h"
 /* this is in builtin-diff.c */
 void add_head(struct rev_info *revs);
 
-static int cmd_log_wc(int argc, const char **argv, char **envp,
+static void cmd_log_init(int argc, const char **argv, char **envp,
 		      struct rev_info *rev)
 {
-	struct commit *commit;
-
 	rev->abbrev = DEFAULT_ABBREV;
 	rev->commit_format = CMIT_FMT_DEFAULT;
 	rev->verbose_header = 1;
 	argc = setup_revisions(argc, argv, rev, "HEAD");
-	if (rev->always_show_header) {
-		if (rev->diffopt.pickaxe || rev->diffopt.filter)
-			rev->always_show_header = 0;
-	}
-
+	if (rev->diffopt.pickaxe || rev->diffopt.filter)
+		rev->always_show_header = 0;
 	if (argc > 1)
 		die("unrecognized argument: %s", argv[1]);
+}
+
+static int cmd_log_walk(struct rev_info *rev)
+{
+	struct commit *commit;
 
 	prepare_revision_walk(rev);
 	setup_pager();
@@ -51,7 +51,10 @@ int cmd_whatchanged(int argc, const char
 	rev.diff = 1;
 	rev.diffopt.recursive = 1;
 	rev.simplify_history = 0;
-	return cmd_log_wc(argc, argv, envp, &rev);
+	cmd_log_init(argc, argv, envp, &rev);
+	if (!rev.diffopt.output_format)
+		rev.diffopt.output_format = DIFF_FORMAT_RAW;
+	return cmd_log_walk(&rev);
 }
 
 int cmd_show(int argc, const char **argv, char **envp)
@@ -66,7 +69,8 @@ int cmd_show(int argc, const char **argv
 	rev.always_show_header = 1;
 	rev.ignore_merges = 0;
 	rev.no_walk = 1;
-	return cmd_log_wc(argc, argv, envp, &rev);
+	cmd_log_init(argc, argv, envp, &rev);
+	return cmd_log_walk(&rev);
 }
 
 int cmd_log(int argc, const char **argv, char **envp)
@@ -76,7 +80,8 @@ int cmd_log(int argc, const char **argv,
 	init_revisions(&rev);
 	rev.always_show_header = 1;
 	rev.diffopt.recursive = 1;
-	return cmd_log_wc(argc, argv, envp, &rev);
+	cmd_log_init(argc, argv, envp, &rev);
+	return cmd_log_walk(&rev);
 }
 
 static int istitlechar(char c)
-- 
1.4.1.rc1.g39849-dirty

Re: [PATCH 0/7] Rework diff options

[Junio C Hamano]

Junio C Hamano <junkio@cox.net> writes:

> Here are a few problems I have seen:
>
>  - "git show --stat HEAD" gives '---' marker as Johannes and you
>    have already discussed (I do not mind this that much though);
>
>  - "--cc" seems to be quite broken.  "git show v1.0.0" nor "git
>    diff-tree --pretty --cc v1.0.0" does not give the log
>    message, and gives something quite confused instead.  I think
>    it is showing "-m -p" followed by "--cc".
>
> We may find more minor breakages, in addition to these, but I am
> reasonably sure we should be able to fix them in-tree.

Further impressions, while with a clean index and working tree.

First the good ones (improvements).

 - "git diff-index --patch-with-raw HEAD" gives empty result;
   the traditional one shows one empty line.

 - "git diff-tree -p --stat" and "git diff-tree --stat -p"
   works, as you planned.

 - "git diff-tree --root --patch-with-raw --summary" works; the
   traditional one misses --summary.

 - "git show --name-only HEAD" works; the traditional one always
   does --cc -p; the same for "git show -s HEAD".

Regressions, most of the minor.

 - "git diff-index -p --stat HEAD" gives one empty line; the
   traditional one gives empty.

 - "git diff-tree --patch-with-raw HEAD" for a non-merge commit
   misses the empty line between raw and patch.

 - "git diff-tree --cc HEAD" for an evil merge (a merge whose
   result does not match either parents, e.g. v1.0.0) shows extra
   two-tree diffs (presumably HEAD^1..HEAD and HEAD^2..HEAD)
   before showing what is expected.  The same for "git show". 

 - "git show --name-only HEAD" for an evil merge similarly shows
   extra two-tree diffs in --name-only format before showing
   what is expected.  Presumably the same bug as the above.

 - "git diff-tree -c HEAD" for an evil merge shows extra newline
   after the output.

 - Neither "git diff-tree -m -s HEAD" for a merge, "git diff-tree -s
   HEAD" for a non-merge does not squelch the output; same for
   "git whatchanged".

 - "git log --raw HEAD" descends into subdirectories.  It
   instead should show the top-level tree differences.

 - "git diff-tree --pretty --patch-with-stat HEAD" for a
   non-merge misses "---\n" before stat (I think you are aware
   of this).

 - "git show --cc HEAD" for a merge should do "---\n", followed
   by a stat for diff between HEAD^1..HEAD, followed by dense
   combined-diff for HEAD.