Subject: [PATCH v5 0/2] add --combined-all-paths option to log and diff-tree

From: Elijah Newren <[email-masked]>
Date: Thu, 7 Feb 2019 17:12:45 -0800

Changes since v4:
  * Added a second patch that can be squashed in which will add
    'rename from' and 'copy from' extended headers.  I like it, but
    Junio sounded pessimistic about it.  See
    https://public-inbox.org/git/[email-masked]/
  * Micro fixes:
    * Renamed --combined-all-names to --combined-all-paths
    * Fixed formatting error (missed '+') in diff-generate-patch.txt
    * Marked tests which used tabs in filenames with FUNNYNAMES prereq
    * Added tests that didn't depend on FUNNYNAMES

Elijah Newren (2):
  log,diff-tree: add --combined-all-paths option
  squash! log,diff-tree: add --combined-all-paths option

 Documentation/diff-format.txt         | 20 +++++-
 Documentation/diff-generate-patch.txt | 20 ++++--
 Documentation/git-diff-tree.txt       | 11 +++-
 Documentation/rev-list-options.txt    |  7 +++
 builtin/diff-tree.c                   |  6 +-
 combine-diff.c                        | 91 +++++++++++++++++++++++----
 diff.h                                |  1 +
 revision.c                            |  6 ++
 revision.h                            |  1 +
 t/t4038-diff-combined.sh              | 88 ++++++++++++++++++++++++++
 10 files changed, 230 insertions(+), 21 deletions(-)

Range-diff:
1:  26c64cee8a ! 1:  2205640429 log,diff-tree: add --combined-all-names option
    @@ -1,6 +1,6 @@
     Author: Elijah Newren <[email-masked]>
     
    -    log,diff-tree: add --combined-all-names option
    +    log,diff-tree: add --combined-all-paths option
     
         The combined diff format for merges will only list one filename, even if
         rename or copy detection is active.  For example, with raw format one
    @@ -15,7 +15,7 @@
         of phooey.c were in either of the parents.  In contrast, for non-merge
         commits, raw format does provide original filenames (and a rename score
         to boot).  In order to also provide original filenames for merge
    -    commits, add a --combined-all-names option (which must be used with
    +    commits, add a --combined-all-paths option (which must be used with
         either -c or --cc, and is likely only useful with rename or copy
         detection active) so that we can print tab-separated filenames when
         renames are involved.  This transforms the above output to:
    @@ -38,8 +38,6 @@
           +++ b/phooey.c
     
         Signed-off-by: Elijah Newren <[email-masked]>
    -    Signed-off-by: Junio C Hamano <[email-masked]>
    -    Message-Id: <[email-masked]>
     
      diff --git a/Documentation/diff-format.txt b/Documentation/diff-format.txt
      --- a/Documentation/diff-format.txt
    @@ -54,17 +52,17 @@
     -Example:
     +For `-c` and `--cc`, only the destination or final path is shown even
     +if the file was renamed on any side of history.  With
    -+`--combined-all-names`, the name of the path in each parent is shown
    ++`--combined-all-paths`, the name of the path in each parent is shown
     +followed by the name of the path in the merge commit.
     +
    -+Examples for `-c` and `-cc` without `--combined-all-names`:
    ++Examples for `-c` and `-cc` without `--combined-all-paths`:
     +------------------------------------------------
     +::100644 100644 100644 fabadb8 cc95eb0 4866510 MM	desc.c
     +::100755 100755 100755 52b7a2d 6d1ac04 d2ac7d7 RM	bar.sh
     +::100644 100644 100644 e07d6c5 9042e82 ee91881 RR	phooey.c
     +------------------------------------------------
     +
    -+Examples when `--combined-all-names` added to either `-c` or `--cc`:
    ++Examples when `--combined-all-paths` added to either `-c` or `--cc`:
      
      ------------------------------------------------
     -::100644 100644 100644 fabadb8 cc95eb0 4866510 MM	describe.c
    @@ -79,9 +77,10 @@
      --- a/Documentation/diff-generate-patch.txt
      +++ b/Documentation/diff-generate-patch.txt
     @@
    + Similar to two-line header for traditional 'unified' diff
      format, `/dev/null` is used to signal created or deleted
      files.
    - 
    +++
     +However, if the --combined-all-paths option is provided, instead of a
     +two-line from-file/to-file you get a N+1 line from-file/to-file header,
     +where N is the number of parents in the merge commit
    @@ -94,10 +93,9 @@
     +This extended format can be useful if rename or copy detection is
     +active, to allow you to see the original name of the file in different
     +parents.
    -+
    + 
      4.   Chunk header format is modified to prevent people from
           accidentally feeding it to `patch -p1`. Combined diff format
    -      was created for review of merge commit changes, and was not
     
      diff --git a/Documentation/git-diff-tree.txt b/Documentation/git-diff-tree.txt
      --- a/Documentation/git-diff-tree.txt
    @@ -108,7 +106,7 @@
      'git diff-tree' [--stdin] [-m] [-s] [-v] [--no-commit-id] [--pretty]
     -	      [-t] [-r] [-c | --cc] [--root] [<common diff options>]
     -	      <tree-ish> [<tree-ish>] [<path>...]
    -+	      [-t] [-r] [-c | --cc] [--combined-all-names] [--root]
    ++	      [-t] [-r] [-c | --cc] [--combined-all-paths] [--root]
     +	      [<common diff options>] <tree-ish> [<tree-ish>] [<path>...]
      
      DESCRIPTION
    @@ -117,7 +115,7 @@
      	itself and the commit log message is not shown, just like in any other
      	"empty diff" case.
      
    -+--combined-all-names::
    ++--combined-all-paths::
     +	This flag causes combined diffs (used for merge commits) to
     +	list the name of the file from all parents.  It thus only has
     +	effect when -c or --cc are specified, and is likely only
    @@ -135,7 +133,7 @@
      	the parents have only two variants and the merge result picks
      	one of them without modification.
      
    -+--combined-all-names::
    ++--combined-all-paths::
     +	This flag causes combined diffs (used for merge commits) to
     +	list the name of the file from all parents.  It thus only has
     +	effect when -c or --cc are specified, and is likely only
    @@ -159,7 +157,7 @@
      "  -r            diff recursively\n"
     +"  -c            show combined diff for merge commits\n"
     +"  --cc          show combined diff for merge commits removing uninteresting hunks\n"
    -+"  --combined-all-names\n"
    ++"  --combined-all-paths\n"
     +"                show name of file in all parents for combined diffs\n"
      "  --root        include the initial commit as diff against /dev/null\n"
      COMMON_DIFF_OPTIONS_HELP;
    @@ -182,7 +180,7 @@
     +	struct combine_diff_path *curr,
     +	int n,
     +	int num_parent,
    -+	int combined_all_names)
    ++	int combined_all_paths)
      {
      	struct diff_queue_struct *q = &diff_queued_diff;
      	struct combine_diff_path *p, **tail = &curr;
    @@ -196,7 +194,7 @@
      			p->parent[n].mode = q->queue[i]->one->mode;
      			p->parent[n].status = q->queue[i]->status;
     +
    -+			if (combined_all_names &&
    ++			if (combined_all_paths &&
     +			    filename_changed(p->parent[n].status)) {
     +				strbuf_init(&p->parent[n].path, 0);
     +				strbuf_addstr(&p->parent[n].path,
    @@ -210,7 +208,7 @@
      			/* p->path not in q->queue[]; drop it */
      			*tail = p->next;
     +			for (j = 0; j < num_parent; j++)
    -+				if (combined_all_names &&
    ++				if (combined_all_paths &&
     +				    filename_changed(p->parent[j].status))
     +					strbuf_release(&p->parent[j].path);
      			free(p);
    @@ -220,7 +218,7 @@
      		oidcpy(&p->parent[n].oid, &q->queue[i]->one->oid);
      		p->parent[n].mode = q->queue[i]->one->mode;
      		p->parent[n].status = q->queue[i]->status;
    -+		if (combined_all_names &&
    ++		if (combined_all_paths &&
     +		    filename_changed(p->parent[n].status))
     +			strbuf_addstr(&p->parent[n].path,
     +				      q->queue[i]->one->path);
    @@ -237,7 +235,7 @@
     -	else
     -		dump_quoted_path("--- ", a_prefix, elem->path,
     -				 line_prefix, c_meta, c_reset);
    -+	if (rev->combined_all_names) {
    ++	if (rev->combined_all_paths) {
     +		for (i = 0; i < num_parent; i++) {
     +			char *path = filename_changed(elem->parent[i].status)
     +				? elem->parent[i].path.buf : elem->path;
    @@ -264,7 +262,7 @@
      	}
      
     +	for (i = 0; i < num_parent; i++)
    -+		if (rev->combined_all_names) {
    ++		if (rev->combined_all_paths) {
     +			if (filename_changed(p->parent[i].status))
     +				write_name_quoted(p->parent[i].path.buf, stdout,
     +						  inter_name_termination);
    @@ -282,7 +280,7 @@
     -	const struct oid_array *parents, struct diff_options *opt)
     +	const struct oid_array *parents,
     +	struct diff_options *opt,
    -+	int combined_all_names)
    ++	int combined_all_paths)
      {
      	struct combine_diff_path *paths = NULL;
      	int i, num_parent = parents->nr;
    @@ -292,7 +290,7 @@
      		diffcore_std(opt);
     -		paths = intersect_paths(paths, i, num_parent);
     +		paths = intersect_paths(paths, i, num_parent,
    -+					combined_all_names);
    ++					combined_all_paths);
      
      		/* if showing diff, show it in requested order */
      		if (opt->output_format != DIFF_FORMAT_NO_OUTPUT &&
    @@ -302,7 +300,7 @@
      		 */
     -		paths = find_paths_generic(oid, parents, &diffopts);
     +		paths = find_paths_generic(oid, parents, &diffopts,
    -+					   rev->combined_all_names);
    ++					   rev->combined_all_paths);
      	}
      	else {
      		int stat_opt;
    @@ -311,7 +309,7 @@
      		struct combine_diff_path *tmp = paths;
      		paths = paths->next;
     +		for (i = 0; i < num_parent; i++)
    -+			if (rev->combined_all_names &&
    ++			if (rev->combined_all_paths &&
     +			    filename_changed(tmp->parent[i].status))
     +				strbuf_release(&tmp->parent[i].path);
      		free(tmp);
    @@ -337,9 +335,9 @@
      		revs->diff = 1;
      		revs->dense_combined_merges = 0;
      		revs->combine_merges = 1;
    -+	} else if (!strcmp(arg, "--combined-all-names")) {
    ++	} else if (!strcmp(arg, "--combined-all-paths")) {
     +		revs->diff = 1;
    -+		revs->combined_all_names = 1;
    ++		revs->combined_all_paths = 1;
      	} else if (!strcmp(arg, "--cc")) {
      		revs->diff = 1;
      		revs->dense_combined_merges = 1;
    @@ -347,8 +345,8 @@
      	}
      	if (revs->combine_merges)
      		revs->ignore_merges = 0;
    -+	if (revs->combined_all_names && !revs->combine_merges)
    -+		die("--combined-all-names makes no sense without -c or --cc");
    ++	if (revs->combined_all_paths && !revs->combine_merges)
    ++		die("--combined-all-paths makes no sense without -c or --cc");
     +
      	revs->diffopt.abbrev = revs->abbrev;
      
    @@ -361,7 +359,7 @@
      			verbose_header:1,
      			ignore_merges:1,
      			combine_merges:1,
    -+			combined_all_names:1,
    ++			combined_all_paths:1,
      			dense_combined_merges:1,
      			always_show_header:1;
      
    @@ -373,20 +371,61 @@
      	test_cmp expect actual
      '
      
    -+test_expect_success 'setup for --combined-with-paths' '
    ++test_expect_success 'setup for --combined-all-paths' '
     +	git branch side1c &&
     +	git branch side2c &&
     +	git checkout side1c &&
    ++	test_seq 1 10 >filename-side1c &&
    ++	git add filename-side1c &&
    ++	git commit -m with &&
    ++	git checkout side2c &&
    ++	test_seq 1 9 >filename-side2c &&
    ++	echo ten >>filename-side2c &&
    ++	git add filename-side2c &&
    ++	git commit -m iam &&
    ++	git checkout -b mergery side1c &&
    ++	git merge --no-commit side2c &&
    ++	git rm filename-side1c &&
    ++	echo eleven >>filename-side2c &&
    ++	git mv filename-side2c filename-merged &&
    ++	git add filename-merged &&
    ++	git commit
    ++'
    ++
    ++test_expect_success '--combined-all-paths and --raw' '
    ++	cat <<-\EOF >expect &&
    ++	::100644 100644 100644 f00c965d8307308469e537302baa73048488f162 088bd5d92c2a8e0203ca8e7e4c2a5c692f6ae3f7 333b9c62519f285e1854830ade0fe1ef1d40ee1b RR	filename-side1c	filename-side2c	filename-merged
    ++	EOF
    ++	git diff-tree -c -M --raw --combined-all-paths HEAD >actual.tmp &&
    ++	sed 1d <actual.tmp >actual &&
    ++	test_cmp expect actual
    ++'
    ++
    ++test_expect_success '--combined-all-paths and --cc' '
    ++	cat <<-\EOF >expect &&
    ++	--- a/filename-side1c
    ++	--- a/filename-side2c
    ++	+++ b/filename-merged
    ++	EOF
    ++	git diff-tree --cc -M --combined-all-paths HEAD >actual.tmp &&
    ++	grep ^[-+][-+][-+] <actual.tmp >actual &&
    ++	test_cmp expect actual
    ++'
    ++
    ++test_expect_success FUNNYNAMES 'setup for --combined-all-paths with funny names' '
    ++	git branch side1d &&
    ++	git branch side2d &&
    ++	git checkout side1d &&
     +	test_seq 1 10 >$(printf "file\twith\ttabs") &&
     +	git add file* &&
     +	git commit -m with &&
    -+	git checkout side2c &&
    ++	git checkout side2d &&
     +	test_seq 1 9 >$(printf "i\tam\ttabbed") &&
     +	echo ten >>$(printf "i\tam\ttabbed") &&
     +	git add *tabbed &&
     +	git commit -m iam &&
    -+	git checkout -b mergery side1c &&
    -+	git merge --no-commit side2c &&
    ++	git checkout -b funny-names-mergery side1d &&
    ++	git merge --no-commit side2d &&
     +	git rm *tabs &&
     +	echo eleven >>$(printf "i\tam\ttabbed") &&
     +	git mv "$(printf "i\tam\ttabbed")" "$(printf "fickle\tnaming")" &&
    @@ -394,28 +433,28 @@
     +	git commit
     +'
     +
    -+test_expect_success '--combined-all-names and --raw' '
    ++test_expect_success FUNNYNAMES '--combined-all-paths and --raw and funny names' '
     +	cat <<-\EOF >expect &&
     +	::100644 100644 100644 f00c965d8307308469e537302baa73048488f162 088bd5d92c2a8e0203ca8e7e4c2a5c692f6ae3f7 333b9c62519f285e1854830ade0fe1ef1d40ee1b RR	"file\twith\ttabs"	"i\tam\ttabbed"	"fickle\tnaming"
     +	EOF
    -+	git diff-tree -c -M --raw --combined-all-names HEAD >actual.tmp &&
    ++	git diff-tree -c -M --raw --combined-all-paths HEAD >actual.tmp &&
     +	sed 1d <actual.tmp >actual &&
     +	test_cmp expect actual
     +'
     +
    -+test_expect_success '--combined-all-names and --raw -and -z' '
    -+	printf "0f9645804ebb04cc3eef91f799eb7fb54d70cefb\0::100644 100644 100644 f00c965d8307308469e537302baa73048488f162 088bd5d92c2a8e0203ca8e7e4c2a5c692f6ae3f7 333b9c62519f285e1854830ade0fe1ef1d40ee1b RR\0file\twith\ttabs\0i\tam\ttabbed\0fickle\tnaming\0" >expect &&
    -+	git diff-tree -c -M --raw --combined-all-names -z HEAD >actual &&
    ++test_expect_success FUNNYNAMES '--combined-all-paths and --raw -and -z and funny names' '
    ++	printf "aaf8087c3cbd4db8e185a2d074cf27c53cfb75d7\0::100644 100644 100644 f00c965d8307308469e537302baa73048488f162 088bd5d92c2a8e0203ca8e7e4c2a5c692f6ae3f7 333b9c62519f285e1854830ade0fe1ef1d40ee1b RR\0file\twith\ttabs\0i\tam\ttabbed\0fickle\tnaming\0" >expect &&
    ++	git diff-tree -c -M --raw --combined-all-paths -z HEAD >actual &&
     +	test_cmp -a expect actual
     +'
     +
    -+test_expect_success '--combined-all-names and --cc' '
    ++test_expect_success FUNNYNAMES '--combined-all-paths and --cc and funny names' '
     +	cat <<-\EOF >expect &&
     +	--- "a/file\twith\ttabs"
     +	--- "a/i\tam\ttabbed"
     +	+++ "b/fickle\tnaming"
     +	EOF
    -+	git diff-tree --cc -M --combined-all-names HEAD >actual.tmp &&
    ++	git diff-tree --cc -M --combined-all-paths HEAD >actual.tmp &&
     +	grep ^[-+][-+][-+] <actual.tmp >actual &&
     +	test_cmp expect actual
     +'
2:  d93e6c5fee < -:  ---------- SQUASH??? fix mark-up
-:  ---------- > 2:  b8408a6075 squash! log,diff-tree: add --combined-all-paths option
-- 
2.20.1.311.gb8408a6075