[PATCH 0/2] improving "git stash list -p"

[PATCH 0/2] improving "git stash list -p"

[Jeff King]

Imagine you have a simple stash like:

  git init
  echo base >file && git add file && git commit -m file
  echo new >file
  git stash

Before this series:

  $ git stash list -p
  stash@{0}: WIP on master: 7a1fd22 file

Er, what? It didn't give me a patch. Oh, that's because stashes are
merge commits, and we need to tell it how to handle merges.

  $ git stash list -p --cc
  stash@{0}: WIP on master: 7a1fd22 file

  diff --cc file
  index df967b9,df967b9..3e75765
  --- a/file
  +++ b/file
  @@@ -1,1 -1,1 +1,1 @@@
  --base
  ++new

Better, though the combined diff is useless, since I didn't touch the
index at all. Here it is after this series:

  $ git stash list -p
  stash@{0}: WIP on master: 7a1fd22 file

  diff --cc file
  index df967b9..3e75765
  --- a/file
  +++ b/file
  @@ -1,1 +1,1 @@
  -base
  +new

Ah, a nice readable diff with no hassle.

  [1/2]: add --simplify-combined-diff option
  [2/2]: stash: default listing to "--cc --simplify-combined-diff"

-Peff

[PATCH 1/2] add --simplify-combined-diff option

[Jeff King]

When showing a combined diff, we are comparing two (or more)
parents to a final state, and some of these states may be
the same.  Here's a table of the possible contents for a
given path (for two parents, but we will generalize to more
in a moment; we also omit isomorphic cases where the parents
are swapped):

  case | M | P1 | P2
  -------------------
  1    | A | A  | A
  2    | A | A  | B
  3    | A | B  | B
  4    | A | B  | C

In case 1, the path was not relevant to the merge at all,
and we omit it as uninteresting. In case 2, we did resolve
the path, but in favor of one side. We also consider this
uninteresting and do not show the diff.

In case 4, we had a real content-level merge, and the
combined diff is interesting. We show it.

That leaves case 3, in which both parents are the same, but
the merge picks a new content. This should be rare in
normal merges, though it could happen if you updated an
unrelated file due to a resolution elsewhere (i.e., an evil
merge that crosses a file boundary). But it happens
frequently in the fake merge commits we create for stashes,
in which one parent is the base of the stash and the other
is the index (in which case it simply means that the index
entry for the path was not touched).

Right now, we treat it the same as case 4, and show a normal
combined diff. However, the result is harder to read, and
the combined nature of the diff gives no extra information;
every marker in the combined diff will be identical for both
parents.

This patch adds a new option, "--simplify-combined-diff",
which converts this case into a normal, non-combined diff.
It would not be correct to simply omit it, because there
really is an interesting change going from B..A. It's just
that there are not two interesting changes, which the
combined diff would show.

When generalizing this to more than two parents, we have two
options:

  1. Either simply to a single parent content, or not at all.

  2. Omit parents whose contents are duplicates of other
     parents.

For a case like "A B B C", option (2) would still result in
a combined diff, but one with fewer sources. However, it
would also be ambiguous. The parents in a combined diff are
marked only by their position, so omitting a position means
that a reader can no longer tell which line goes with which
parent.

Instead, we choose option (1). Either you get the full
combined diff, or you get a normal non-combined diff.

Signed-off-by: Jeff King <peff@peff.net>
---
The implementation just cuts the number of parents down to 1, but
otherwise runs through the same combined-diff code.  The resulting
pairwise combined-diff differs from a normal diff in a few ways:

  1. The header line is still "diff --combined" (or "--cc") instead of
     "diff --git", and it mentions the filename only once.

  2. The index lines do not contain the file mode; for combined diffs,
     we generate a separate mode line (but only when it has something
     interesting to show).

  3. The hunk header for a single-line change says "-1" in the regular
     code path, but "-1,1" in the combined code path.

Is there any value in keeping this as a pseudo-combined diff (i.e., with
the combined header, but only a single parent)? It should not be too
hard to just punt to the regular builtin_diff code path when
"num_parents == 1".

 Documentation/rev-list-options.txt |   8 +++
 combine-diff.c                     |  30 +++++++--
 revision.c                         |   4 ++
 revision.h                         |   1 +
 t/t4038-diff-combined.sh           | 134 +++++++++++++++++++++++++++++++++++++
 5 files changed, 173 insertions(+), 4 deletions(-)

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index deb8cca..7331bcc 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -805,6 +805,14 @@ options may be given. See linkgit:git-diff-files[1] for more options.
 	in that case, the output represents the changes the merge
 	brought _into_ the then-current branch.
 
+--simplify-combined-diff::
+	When showing a combined diff with `-c` or `--cc`, if a given
+	path has identical content in all parents, show only a pairwise
+	diff from that content, rather than a true combined diff. This
+	is easier for humans to read, though it may confuse an automatic
+	parser (as some paths in the output may be combined diffs, and
+	others may not).
+
 -r::
 	Show recursive diffs.
 
diff --git a/combine-diff.c b/combine-diff.c
index f9975d2..60e54a7 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -955,6 +955,16 @@ static void show_combined_header(struct combine_diff_path *elem,
 				 line_prefix, c_meta, c_reset);
 }
 
+static int simplify_parents(struct combine_diff_path *p, int nr)
+{
+	int i;
+	for (i = 1; i < nr; i++)
+		if (p->parent[i].mode != p->parent[i-1].mode ||
+		    hashcmp(p->parent[i].sha1, p->parent[i-1].sha1))
+			return nr;
+	return 1;
+}
+
 static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 			    int dense, int working_tree_file,
 			    struct rev_info *rev)
@@ -979,6 +989,9 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 	if (DIFF_OPT_TST(opt, ALLOW_TEXTCONV))
 		textconv = userdiff_get_textconv(userdiff);
 
+	if (rev->simplify_combined_diff)
+		num_parent = simplify_parents(elem, num_parent);
+
 	/* Read the result of merge first */
 	if (!working_tree_file)
 		result = grab_blob(elem->sha1, elem->mode, &result_size,
@@ -1181,6 +1194,8 @@ static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct re
 	if (rev->loginfo && !rev->no_commit_id)
 		show_log(rev);
 
+	if (rev->simplify_combined_diff)
+		num_parent = simplify_parents(p, num_parent);
 
 	if (opt->output_format & DIFF_FORMAT_RAW) {
 		printf("%s", line_prefix);
@@ -1247,12 +1262,16 @@ static void free_combined_pair(struct diff_filepair *pair)
  * but currently nobody uses it, so this should suffice for now.
  */
 static struct diff_filepair *combined_pair(struct combine_diff_path *p,
-					   int num_parent)
+					   int num_parent,
+					   struct rev_info *rev)
 {
 	int i;
 	struct diff_filepair *pair;
 	struct diff_filespec *pool;
 
+	if (rev->simplify_combined_diff)
+		num_parent = simplify_parents(p, num_parent);
+
 	pair = xmalloc(sizeof(*pair));
 	pool = xcalloc(num_parent + 1, sizeof(struct diff_filespec));
 	pair->one = pool + 1;
@@ -1277,7 +1296,8 @@ static struct diff_filepair *combined_pair(struct combine_diff_path *p,
 static void handle_combined_callback(struct diff_options *opt,
 				     struct combine_diff_path *paths,
 				     int num_parent,
-				     int num_paths)
+				     int num_paths,
+				     struct rev_info *rev)
 {
 	struct combine_diff_path *p;
 	struct diff_queue_struct q;
@@ -1287,7 +1307,8 @@ static void handle_combined_callback(struct diff_options *opt,
 	q.alloc = num_paths;
 	q.nr = num_paths;
 	for (i = 0, p = paths; p; p = p->next)
-		q.queue[i++] = combined_pair(p, num_parent);
+		q.queue[i++] = combined_pair(p, num_parent, rev);
+
 	opt->format_callback(&q, opt, opt->format_callback_data);
 	for (i = 0; i < num_paths; i++)
 		free_combined_pair(q.queue[i]);
@@ -1498,7 +1519,8 @@ void diff_tree_combined(const unsigned char *sha1,
 			 (DIFF_FORMAT_NUMSTAT|DIFF_FORMAT_DIFFSTAT))
 			needsep = 1;
 		else if (opt->output_format & DIFF_FORMAT_CALLBACK)
-			handle_combined_callback(opt, paths, num_parent, num_paths);
+			handle_combined_callback(opt, paths, num_parent,
+						 num_paths, rev);
 
 		if (opt->output_format & DIFF_FORMAT_PATCH) {
 			if (needsep)
diff --git a/revision.c b/revision.c
index 2571ada..a242485 100644
--- a/revision.c
+++ b/revision.c
@@ -1820,6 +1820,10 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->diff = 1;
 		revs->dense_combined_merges = 1;
 		revs->combine_merges = 1;
+	} else if (!strcmp(arg, "--simplify-combined-diff")) {
+		revs->simplify_combined_diff = 1;
+	} else if (!strcmp(arg, "--no-simplify-combined-diff")) {
+		revs->simplify_combined_diff = 0;
 	} else if (!strcmp(arg, "-v")) {
 		revs->verbose_header = 1;
 	} else if (!strcmp(arg, "--pretty")) {
diff --git a/revision.h b/revision.h
index a620530..48cb16e 100644
--- a/revision.h
+++ b/revision.h
@@ -122,6 +122,7 @@ struct rev_info {
 			ignore_merges:1,
 			combine_merges:1,
 			dense_combined_merges:1,
+			simplify_combined_diff:1,
 			always_show_header:1;
 
 	/* Format info */
diff --git a/t/t4038-diff-combined.sh b/t/t4038-diff-combined.sh
index 41913c3..d522474 100755
--- a/t/t4038-diff-combined.sh
+++ b/t/t4038-diff-combined.sh
@@ -401,4 +401,138 @@ test_expect_success 'combine diff missing delete bug' '
 	compare_diff_patch expected actual
 '
 
+# make a commit with contents "$1" and "$2" in the paths
+# "one" and "two" respectively, and parents $3, $4, etc
+#
+# If the content is of the form "mode:content", then the
+# mode for the given file is set (otherwise it defaults 100644).
+mkcommit() {
+	(
+		GIT_INDEX_FILE=tmp.index &&
+		export GIT_INDEX_FILE &&
+		for path in one two; do
+			case "$1" in
+			*:*)
+				mode=$(echo "$1" | cut -d: -f1)
+				content=$(echo "$1" | cut -d: -f2)
+				;;
+			*)
+				mode=100644
+				content=$1
+			esac
+			blob=$(echo "$content" | git hash-object -w --stdin) &&
+			git update-index --add --cacheinfo $mode,$blob,$path &&
+			shift || exit 1
+		done
+		tree=$(git write-tree) &&
+		parents=$(for p in "$@"; do echo "-p $p"; done) &&
+		git commit-tree $tree $parents </dev/null
+	)
+}
+
+# we create a merge commit where path "one" can be simplified, but
+# path "two" cannot
+test_expect_success 'simplify combined --raw' '
+	side1=$(mkcommit base content1) &&
+	side2=$(mkcommit base content2) &&
+	merge=$(mkcommit new new $side1 $side2) &&
+	cat >expect <<-\EOF &&
+
+	:100644 100644 df967b9... 3e75765... M	one
+	::100644 100644 100644 ac3e272... 637f034... 3e75765... MM	two
+	EOF
+
+	git diff-tree -c --simplify-combined-diff \
+		--abbrev --pretty=format: $merge >actual &&
+	test_cmp expect actual
+'
+
+# we do not use compare_diff_patch here because we want to
+# make sure we get the headers right, too
+test_expect_success 'simplify combined --patch' '
+	side1=$(mkcommit base content1) &&
+	side2=$(mkcommit base content2) &&
+	merge=$(mkcommit new new $side1 $side2) &&
+	cat >expect <<-\EOF &&
+
+	diff --combined one
+	index df967b9..3e75765
+	--- a/one
+	+++ b/one
+	@@ -1,1 +1,1 @@
+	-base
+	+new
+	diff --combined two
+	index ac3e272,637f034..3e75765
+	--- a/two
+	+++ b/two
+	@@@ -1,1 -1,1 +1,1 @@@
+	- content1
+	 -content2
+	++new
+	EOF
+
+	git diff-tree -c --simplify-combined-diff -p \
+		--pretty=format: $merge >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'do not simplify unless all parents are identical' '
+	side1=$(mkcommit base content1) &&
+	test_tick &&
+	side2=$(mkcommit base content1) &&
+	side3=$(mkcommit base content3) &&
+	merge=$(mkcommit new new $side1 $side2 $side3) &&
+	cat >expect <<-\EOF &&
+
+	diff --combined one
+	index df967b9..3e75765
+	--- a/one
+	+++ b/one
+	@@ -1,1 +1,1 @@
+	-base
+	+new
+	diff --combined two
+	index ac3e272,ac3e272,27d10cc..3e75765
+	--- a/two
+	+++ b/two
+	@@@@ -1,1 -1,1 -1,1 +1,1 @@@@
+	-- content1
+	  -content3
+	+++new
+	EOF
+
+	git diff-tree -c --simplify-combined-diff -p \
+		--pretty=format: $merge >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'do not simplify away mode changes' '
+	side1=$(mkcommit 100644:base 100644:base) &&
+	side2=$(mkcommit 100644:base 100755:base) &&
+	merge=$(mkcommit new new $side1 $side2) &&
+	cat >expect <<-\EOF &&
+
+	diff --combined one
+	index df967b9..3e75765
+	--- a/one
+	+++ b/one
+	@@ -1,1 +1,1 @@
+	-base
+	+new
+	diff --combined two
+	index df967b9,df967b9..3e75765
+	mode 100644,100755..100644
+	--- a/two
+	+++ b/two
+	@@@ -1,1 -1,1 +1,1 @@@
+	--base
+	++new
+	EOF
+
+	git diff-tree -c --simplify-combined-diff -p \
+		--pretty=format: $merge >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.1.0.rc0.286.g5c67d74

[PATCH 2/2] stash: default listing to "--cc --simplify-combined-diff"

[Jeff King]

When you list stashes, you can provide arbitrary git-log
options to change the display. However, adding just "-p"
does nothing, because each stash is actually a merge commit.

This implementation detail is easy to forget, leading to
confused users who think "-p" is not working. We can make
this easier by specifying "--cc" as a default ourselves
(which does nothing if no diff format is requested by the
user).

The resulting diff would then be a combined diff between the
base commit and the stashed index state. In many cases,
though, the user did not stash anything in the index. We can
further simplify this to a normal pairwise diff by using
"--simplify-combined-diff".

Signed-off-by: Jeff King <peff@peff.net>
---
I don't think anybody should complain about these defaults changing,
given how useless "git stash list -p" is now. You can negate
--simplify-combined-diff if you want to, but I don't think there is a
way to turn off "--cc" (though why would you want to?).

Anyway, I don't think we need to worry too much about flexibility here.
If you really want to go wild, you can run "git log -g refs/stash"
yourself.

 git-stash.sh     |  3 ++-
 t/t3903-stash.sh | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 65 insertions(+), 1 deletion(-)

diff --git a/git-stash.sh b/git-stash.sh
index bcc757b..a0246d5 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -297,7 +297,8 @@ have_stash () {
 
 list_stash () {
 	have_stash || return 0
-	git log --format="%gd: %gs" -g "$@" $ref_stash --
+	git log --format="%gd: %gs" -g --cc --simplify-combined-diff \
+		"$@" $ref_stash --
 }
 
 show_stash () {
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 5b79b21..54154d2 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -685,4 +685,67 @@ test_expect_success 'handle stash specification with spaces' '
 	grep pig file
 '
 
+test_expect_success 'stash list implies --cc' '
+	git stash clear &&
+	git reset --hard &&
+	echo index >file &&
+	git add file &&
+	echo working >file &&
+	git stash &&
+	git stash list -p >actual &&
+	cat >expect <<-\EOF &&
+	stash@{0}: WIP on master: b27a2bc subdir
+
+	diff --cc file
+	index 257cc56,9015a7a..d26b33d
+	--- a/file
+	+++ b/file
+	@@@ -1,1 -1,1 +1,1 @@@
+	- foo
+	 -index
+	++working
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'stash list implies --simplify-combined-diff' '
+	git stash clear &&
+	git reset --hard &&
+	echo working >file &&
+	git stash &&
+	git stash list -p >actual &&
+	cat >expect <<-\EOF &&
+	stash@{0}: WIP on master: b27a2bc subdir
+
+	diff --cc file
+	index 257cc56..d26b33d
+	--- a/file
+	+++ b/file
+	@@ -1,1 +1,1 @@
+	-foo
+	+working
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success '--no-simplify-combined-diff overrides default' '
+	git stash clear &&
+	git reset --hard &&
+	echo working >file &&
+	git stash &&
+	git stash list -p --no-simplify-combined-diff >actual &&
+	cat >expect <<-\EOF &&
+	stash@{0}: WIP on master: b27a2bc subdir
+
+	diff --cc file
+	index 257cc56,257cc56..d26b33d
+	--- a/file
+	+++ b/file
+	@@@ -1,1 -1,1 +1,1 @@@
+	--foo
+	++working
+	EOF
+	test_cmp expect actual
+'
+
 test_done
-- 
2.1.0.rc0.286.g5c67d74

[RFC/PATCH 3/2] stash: show combined diff with "stash show"

[Jeff King]

A stash may store not only working tree changes, but also
changes to the index. However, "git stash show" will only
ever show the working tree changes. We can instead show both
as a combined diff, but use "--simplify-combined-diff" so
that we show a normal pairwise diff in the common case that
there were no index changes.

Signed-off-by: Jeff King <peff@peff.net>
---
I also considered this on top of the other two, but it probably _isn't_
a good idea. People might be doing things like "git stash show | git
apply", and would want to ignore the index content (of course it could
still work in the cases where the index was unchanged, and maybe it is a
good thing that it would break when there are index changes that the
user might be forgetting about).

As you can see from the changes in the test scripts, though, the
implementation isn't quite there either. There's no way to convince "git
show" to produce _no_ pretty-printed output (the best we can do is an
extra blank line). And of course the resulting diffs are single-parent
combined diffs, not "real" pairwise diffs.

We can fix those, but as I said, I'm not sure it is a good change even
without those warts.

 git-stash.sh     |  3 ++-
 t/t3903-stash.sh | 42 ++++++++++++++++++++++++++++++++++--------
 2 files changed, 36 insertions(+), 9 deletions(-)

diff --git a/git-stash.sh b/git-stash.sh
index a0246d5..9b13853 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -304,7 +304,8 @@ list_stash () {
 show_stash () {
 	assert_stash_like "$@"
 
-	git diff ${FLAGS:---stat} $b_commit $w_commit
+	git show --pretty=format: --simplify-combined-diff \
+		${FLAGS:---stat} $w_commit
 }
 
 #
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 54154d2..045db51 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -460,6 +460,7 @@ test_expect_success 'stash show format defaults to --stat' '
 	STASH_ID=$(git stash create) &&
 	git reset --hard &&
 	cat >expected <<-EOF &&
+
 	 file | 1 +
 	 1 file changed, 1 insertion(+)
 	EOF
@@ -477,7 +478,7 @@ test_expect_success 'stash show - stashes on stack, stash-like argument' '
 	echo bar >> file &&
 	STASH_ID=$(git stash create) &&
 	git reset --hard &&
-	echo "1	0	file" >expected &&
+	{ echo; echo "1	0	file"; } >expected &&
 	git stash show --numstat ${STASH_ID} >actual &&
 	test_cmp expected actual
 '
@@ -493,11 +494,12 @@ test_expect_success 'stash show -p - stashes on stack, stash-like argument' '
 	STASH_ID=$(git stash create) &&
 	git reset --hard &&
 	cat >expected <<-EOF &&
-	diff --git a/file b/file
-	index 7601807..935fbd3 100644
+
+	diff --cc file
+	index 7601807..935fbd3
 	--- a/file
 	+++ b/file
-	@@ -1 +1,2 @@
+	@@ -1,1 +1,2 @@
 	 baz
 	+bar
 	EOF
@@ -512,7 +514,7 @@ test_expect_success 'stash show - no stashes on stack, stash-like argument' '
 	echo foo >> file &&
 	STASH_ID=$(git stash create) &&
 	git reset --hard &&
-	echo "1	0	file" >expected &&
+	{ echo; echo "1	0	file"; } >expected &&
 	git stash show --numstat ${STASH_ID} >actual &&
 	test_cmp expected actual
 '
@@ -525,11 +527,12 @@ test_expect_success 'stash show -p - no stashes on stack, stash-like argument' '
 	STASH_ID=$(git stash create) &&
 	git reset --hard &&
 	cat >expected <<-EOF &&
-	diff --git a/file b/file
-	index 7601807..71b52c4 100644
+
+	diff --cc file
+	index 7601807..71b52c4
 	--- a/file
 	+++ b/file
-	@@ -1 +1,2 @@
+	@@ -1,1 +1,2 @@
 	 baz
 	+foo
 	EOF
@@ -537,6 +540,29 @@ test_expect_success 'stash show -p - no stashes on stack, stash-like argument' '
 	test_cmp expected actual
 '
 
+test_expect_success 'stash show -p will show modified index' '
+	git stash clear &&
+	test_when_finished "git reset --hard HEAD" &&
+	git reset --hard &&
+	echo index >file &&
+	git add file &&
+	echo working >file &&
+	git stash &&
+	cat >expect <<-\EOF &&
+
+	diff --cc file
+	index 7601807,9015a7a..d26b33d
+	--- a/file
+	+++ b/file
+	@@@ -1,1 -1,1 +1,1 @@@
+	- baz
+	 -index
+	++working
+	EOF
+	git stash show -p >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'stash drop - fail early if specified stash is not a stash reference' '
 	git stash clear &&
 	test_when_finished "git reset --hard HEAD && git stash clear" &&
-- 
2.1.0.rc0.286.g5c67d74

Re: [PATCH 1/2] add --simplify-combined-diff option

[Jeff King]

On Tue, Jul 29, 2014 at 08:00:56AM -0400, Jeff King wrote:

> The implementation just cuts the number of parents down to 1, but
> otherwise runs through the same combined-diff code.  The resulting
> pairwise combined-diff differs from a normal diff in a few ways:
> 
>   1. The header line is still "diff --combined" (or "--cc") instead of
>      "diff --git", and it mentions the filename only once.
> 
>   2. The index lines do not contain the file mode; for combined diffs,
>      we generate a separate mode line (but only when it has something
>      interesting to show).
> 
>   3. The hunk header for a single-line change says "-1" in the regular
>      code path, but "-1,1" in the combined code path.
> 
> Is there any value in keeping this as a pseudo-combined diff (i.e., with
> the combined header, but only a single parent)? It should not be too
> hard to just punt to the regular builtin_diff code path when
> "num_parents == 1".

It's not too hard; mostly we just have to massage the data into a
diff_filepair. Patch is below (I'd squash it, and the further commits
will need fixups to their tests, too).

---
 combine-diff.c           | 32 +++++++++++++++++++++++++++++---
 diff.c                   |  2 +-
 diff.h                   |  2 ++
 t/t4038-diff-combined.sh | 18 +++++++++---------
 4 files changed, 41 insertions(+), 13 deletions(-)

diff --git a/combine-diff.c b/combine-diff.c
index 60e54a7..0588c86 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -965,6 +965,28 @@ static int simplify_parents(struct combine_diff_path *p, int nr)
 	return 1;
 }
 
+static void show_single_parent_patch(struct combine_diff_path *elem,
+				     int working_tree_file,
+				     struct rev_info *rev)
+{
+	struct diff_filepair pair;
+
+	memset(&pair, 0, sizeof(pair));
+	pair.one = alloc_filespec(elem->path);
+	pair.two = alloc_filespec(elem->path);
+	pair.status = elem->parent[0].status;
+
+	fill_filespec(pair.one, elem->parent[0].sha1, 1, elem->parent[0].mode);
+	if (working_tree_file)
+		fill_filespec(pair.two, null_sha1, 0, elem->mode);
+	else
+		fill_filespec(pair.two, elem->sha1, 1, elem->mode);
+
+	run_diff(&pair, &rev->diffopt);
+	free_filespec(pair.one);
+	free_filespec(pair.two);
+}
+
 static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 			    int dense, int working_tree_file,
 			    struct rev_info *rev)
@@ -982,6 +1004,13 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 	int is_binary;
 	const char *line_prefix = diff_line_prefix(opt);
 
+	if (rev->simplify_combined_diff)
+		num_parent = simplify_parents(elem, num_parent);
+	if (num_parent == 1) {
+		show_single_parent_patch(elem, working_tree_file, rev);
+		return;
+	}
+
 	context = opt->context;
 	userdiff = userdiff_find_by_path(elem->path);
 	if (!userdiff)
@@ -989,9 +1018,6 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 	if (DIFF_OPT_TST(opt, ALLOW_TEXTCONV))
 		textconv = userdiff_get_textconv(userdiff);
 
-	if (rev->simplify_combined_diff)
-		num_parent = simplify_parents(elem, num_parent);
-
 	/* Read the result of merge first */
 	if (!working_tree_file)
 		result = grab_blob(elem->sha1, elem->mode, &result_size,
diff --git a/diff.c b/diff.c
index 867f034..558a520 100644
--- a/diff.c
+++ b/diff.c
@@ -3089,7 +3089,7 @@ static void strip_prefix(int prefix_length, const char **namep, const char **oth
 	}
 }
 
-static void run_diff(struct diff_filepair *p, struct diff_options *o)
+void run_diff(struct diff_filepair *p, struct diff_options *o)
 {
 	const char *pgm = external_diff();
 	struct strbuf msg;
diff --git a/diff.h b/diff.h
index b4a624d..a669be0 100644
--- a/diff.h
+++ b/diff.h
@@ -356,4 +356,6 @@ extern int print_stat_summary(FILE *fp, int files,
 			      int insertions, int deletions);
 extern void setup_diff_pager(struct diff_options *);
 
+extern void run_diff(struct diff_filepair *p, struct diff_options *o);
+
 #endif /* DIFF_H */
diff --git a/t/t4038-diff-combined.sh b/t/t4038-diff-combined.sh
index d522474..29cdb45 100755
--- a/t/t4038-diff-combined.sh
+++ b/t/t4038-diff-combined.sh
@@ -455,11 +455,11 @@ test_expect_success 'simplify combined --patch' '
 	merge=$(mkcommit new new $side1 $side2) &&
 	cat >expect <<-\EOF &&
 
-	diff --combined one
-	index df967b9..3e75765
+	diff --git a/one b/one
+	index df967b9..3e75765 100644
 	--- a/one
 	+++ b/one
-	@@ -1,1 +1,1 @@
+	@@ -1 +1 @@
 	-base
 	+new
 	diff --combined two
@@ -485,11 +485,11 @@ test_expect_success 'do not simplify unless all parents are identical' '
 	merge=$(mkcommit new new $side1 $side2 $side3) &&
 	cat >expect <<-\EOF &&
 
-	diff --combined one
-	index df967b9..3e75765
+	diff --git a/one b/one
+	index df967b9..3e75765 100644
 	--- a/one
 	+++ b/one
-	@@ -1,1 +1,1 @@
+	@@ -1 +1 @@
 	-base
 	+new
 	diff --combined two
@@ -513,11 +513,11 @@ test_expect_success 'do not simplify away mode changes' '
 	merge=$(mkcommit new new $side1 $side2) &&
 	cat >expect <<-\EOF &&
 
-	diff --combined one
-	index df967b9..3e75765
+	diff --git a/one b/one
+	index df967b9..3e75765 100644
 	--- a/one
 	+++ b/one
-	@@ -1,1 +1,1 @@
+	@@ -1 +1 @@
 	-base
 	+new
 	diff --combined two
-- 
2.1.0.rc0.286.g5c67d74

[PATCH v2 0/6] improving "git stash list -p"

[Jeff King]

I spent a few more minutes cleaning up the rough edges here, and this is
what I ended up with. This version uses run_diff to implement the actual
single-parent diff, so that it matches a "real" diff exactly. It also
introduces a few new patches to let you really specify an empty --pretty
user-format.

I do think this is an improvement over the current state. I'd also be OK
with just specifying "--first-parent" to "git stash list" by default, so
that "git stash list -p" does something sensible. I don't like that it
doesn't let you peek at the index, but nobody has complained about that
in "git stash show", so perhaps nobody cares.  All things being equal, I
prefer this solution, though.

  [1/6]: revision: drop useless string offset when parsing "--pretty"
  [2/6]: pretty: treat "--format=" as an empty userformat
  [3/6]: pretty: make empty userformats truly empty
  [4/6]: add --simplify-combined-diff option
  [5/6]: stash: default listing to "--cc --simplify-combined-diff"
  [6/6]: stash: show combined diff with "stash show"

-Peff

[PATCH v2 1/6] revision: drop useless string offset when parsing "--pretty"

[Jeff King]

Once upon a time, we parsed pretty options by looking for
"--pretty" at the start of the string, and then feeding the
rest (including an "=") to get_commit_format. Later, commit
48ded91 (log --pretty: do not accept bogus "--prettyshort",
2008-05-25) split this into a separate check for "--pretty"
versus "--pretty=".

However, when parsing "--pretty", we still passed "arg+8" to
get_commit_format. This is useless, since it will always
point to the NUL terminator at the end of the string. We can
simply pass NULL instead; both parameters are treated the
same by get_commit_format.

Signed-off-by: Jeff King <peff@peff.net>
---
 revision.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/revision.c b/revision.c
index 2571ada..615535c 100644
--- a/revision.c
+++ b/revision.c
@@ -1825,7 +1825,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 	} else if (!strcmp(arg, "--pretty")) {
 		revs->verbose_header = 1;
 		revs->pretty_given = 1;
-		get_commit_format(arg+8, revs);
+		get_commit_format(NULL, revs);
 	} else if (starts_with(arg, "--pretty=") || starts_with(arg, "--format=")) {
 		/*
 		 * Detached form ("--pretty X" as opposed to "--pretty=X")
-- 
2.1.0.rc0.286.g5c67d74

[PATCH v2 2/6] pretty: treat "--format=" as an empty userformat

[Jeff King]

Until now, we treated "--pretty=" or "--format=" as "give me
the default format". This was not planned nor documented,
but only what happened to work due to our parsing of
"--pretty" (which should give the default format).

Let's instead let these be an actual empty userformat.
Otherwise one must write out the annoyingly long
"--pretty=tformat:" to get the same behavior.

Signed-off-by: Jeff King <peff@peff.net>
---
If we are really concerned with people expecting "--pretty=" to turn on
the default format, we could introduce a new mode "none" which does the
same thing behind the scenes. I don't think it's a problem, though.

 pretty.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/pretty.c b/pretty.c
index 3a1da6f..f97a762 100644
--- a/pretty.c
+++ b/pretty.c
@@ -146,7 +146,7 @@ void get_commit_format(const char *arg, struct rev_info *rev)
 	struct cmt_fmt_map *commit_format;
 
 	rev->use_terminator = 0;
-	if (!arg || !*arg) {
+	if (!arg) {
 		rev->commit_format = CMIT_FMT_DEFAULT;
 		return;
 	}
@@ -155,7 +155,7 @@ void get_commit_format(const char *arg, struct rev_info *rev)
 		return;
 	}
 
-	if (strchr(arg, '%')) {
+	if (!*arg || strchr(arg, '%')) {
 		save_user_format(rev, arg, 1);
 		return;
 	}
-- 
2.1.0.rc0.286.g5c67d74

[PATCH v2 3/6] pretty: make empty userformats truly empty

[Jeff King]

If the user provides an empty format with "--format=", we
end up putting in extra whitespace that the user cannot
prevent. This comes from two places:

  1. If the format is missing a terminating newline, we add
     one automatically. This makes sense for --format=%h, but
     not for a truly empty format.

  2. We add an extra newline between the pretty-printed
     format and a diff or diffstat. If the format is empty,
     there's no point in doing so if there's nothing to
     separate.

With this patch, one can get a diff with no other cruft out
of "diff-tree --format= $commit".

Signed-off-by: Jeff King <peff@peff.net>
---
I assume nobody really cared about including that extra whitespace with
a blank format (I have come across this literally dozens of times in the
past few years, and everytime wanted the behavior that this patch gives,
not the other). But if somebody is depending on it, our alternate route
would be to introduce a new CMIT_FMT_NONE. I started on that, but it
really does end up hitting all of the same weird special-cases that that
CMIT_FMT_USERFORMAT does. If we can do it this way, I think the code is
a lot cleaner.

 combine-diff.c | 3 ++-
 commit.h       | 1 +
 log-tree.c     | 5 +++--
 pretty.c       | 5 +++++
 4 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/combine-diff.c b/combine-diff.c
index f9975d2..1a1e659 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -1397,7 +1397,8 @@ void diff_tree_combined(const unsigned char *sha1,
 		show_log(rev);
 
 		if (rev->verbose_header && opt->output_format &&
-		    opt->output_format != DIFF_FORMAT_NO_OUTPUT)
+		    opt->output_format != DIFF_FORMAT_NO_OUTPUT &&
+		    !commit_format_is_empty(rev->commit_format))
 			printf("%s%c", diff_line_prefix(opt),
 			       opt->line_termination);
 	}
diff --git a/commit.h b/commit.h
index a8cbf52..aa8c3ca 100644
--- a/commit.h
+++ b/commit.h
@@ -159,6 +159,7 @@ extern void get_commit_format(const char *arg, struct rev_info *);
 extern const char *format_subject(struct strbuf *sb, const char *msg,
 				  const char *line_separator);
 extern void userformat_find_requirements(const char *fmt, struct userformat_want *w);
+extern int commit_format_is_empty(enum cmit_fmt);
 extern void format_commit_message(const struct commit *commit,
 				  const char *format, struct strbuf *sb,
 				  const struct pretty_print_context *context);
diff --git a/log-tree.c b/log-tree.c
index 0c53dc1..95e9b1d 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -649,7 +649,7 @@ void show_log(struct rev_info *opt)
 		graph_show_commit_msg(opt->graph, &msgbuf);
 	else
 		fwrite(msgbuf.buf, sizeof(char), msgbuf.len, stdout);
-	if (opt->use_terminator) {
+	if (opt->use_terminator && !commit_format_is_empty(opt->commit_format)) {
 		if (!opt->missing_newline)
 			graph_show_padding(opt->graph);
 		putchar(opt->diffopt.line_termination);
@@ -676,7 +676,8 @@ int log_tree_diff_flush(struct rev_info *opt)
 		show_log(opt);
 		if ((opt->diffopt.output_format & ~DIFF_FORMAT_NO_OUTPUT) &&
 		    opt->verbose_header &&
-		    opt->commit_format != CMIT_FMT_ONELINE) {
+		    opt->commit_format != CMIT_FMT_ONELINE &&
+		    !commit_format_is_empty(opt->commit_format)) {
 			/*
 			 * When showing a verbose header (i.e. log message),
 			 * and not in --pretty=oneline format, we would want
diff --git a/pretty.c b/pretty.c
index f97a762..31fc76b 100644
--- a/pretty.c
+++ b/pretty.c
@@ -24,6 +24,11 @@ static size_t commit_formats_len;
 static size_t commit_formats_alloc;
 static struct cmt_fmt_map *find_commit_format(const char *sought);
 
+int commit_format_is_empty(enum cmit_fmt fmt)
+{
+	return fmt == CMIT_FMT_USERFORMAT && !*user_format;
+}
+
 static void save_user_format(struct rev_info *rev, const char *cp, int is_tformat)
 {
 	free(user_format);
-- 
2.1.0.rc0.286.g5c67d74

[PATCH v2 4/6] add --simplify-combined-diff option

[Jeff King]

When showing a combined diff, we are comparing two (or more)
parents to a final state, and some of these states may be
the same.  Here's a table of the possible contents for a
given path (for two parents, but we will generalize to more
in a moment; we also omit isomorphic cases where the parents
are swapped):

  case | M | P1 | P2
  -------------------
  1    | A | A  | A
  2    | A | A  | B
  3    | A | B  | B
  4    | A | B  | C

In case 1, the path was not relevant to the merge at all,
and we omit it as uninteresting. In case 2, we did resolve
the path, but in favor of one side. We also consider this
uninteresting and do not show the diff.

In case 4, we had a real content-level merge, and the
combined diff is interesting. We show it.

That leaves case 3, in which both parents are the same, but
the merge picks a new content. This should be rare in
normal merges, though it could happen if you updated an
unrelated file due to a resolution elsewhere (i.e., an evil
merge that crosses a file boundary). But it happens
frequently in the fake merge commits we create for stashes,
in which one parent is the base of the stash and the other
is the index (in which case it simply means that the index
entry for the path was not touched).

Right now, we treat it the same as case 4, and show a normal
combined diff. However, the result is harder to read, and
the combined nature of the diff gives no extra information;
every marker in the combined diff will be identical for both
parents.

This patch adds a new option, "--simplify-combined-diff",
which converts this case into a normal, non-combined diff.
It would not be correct to simply omit it, because there
really is an interesting change going from B..A. It's just
that there are not two interesting changes, which the
combined diff would show.

When generalizing this to more than two parents, we have two
options:

  1. Either simply to a single parent content, or not at all.

  2. Omit parents whose contents are duplicates of other
     parents.

For a case like "A B B C", option (2) would still result in
a combined diff, but one with fewer sources. However, it
would also be ambiguous. The parents in a combined diff are
marked only by their position, so omitting a position means
that a reader can no longer tell which line goes with which
parent.

Instead, we choose option (1). Either you get the full
combined diff, or you get a normal non-combined diff.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/rev-list-options.txt |   8 +++
 combine-diff.c                     |  56 ++++++++++++++--
 diff.c                             |   2 +-
 diff.h                             |   2 +
 revision.c                         |   4 ++
 revision.h                         |   1 +
 t/t4038-diff-combined.sh           | 133 +++++++++++++++++++++++++++++++++++++
 7 files changed, 201 insertions(+), 5 deletions(-)

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index deb8cca..7331bcc 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -805,6 +805,14 @@ options may be given. See linkgit:git-diff-files[1] for more options.
 	in that case, the output represents the changes the merge
 	brought _into_ the then-current branch.
 
+--simplify-combined-diff::
+	When showing a combined diff with `-c` or `--cc`, if a given
+	path has identical content in all parents, show only a pairwise
+	diff from that content, rather than a true combined diff. This
+	is easier for humans to read, though it may confuse an automatic
+	parser (as some paths in the output may be combined diffs, and
+	others may not).
+
 -r::
 	Show recursive diffs.
 
diff --git a/combine-diff.c b/combine-diff.c
index 1a1e659..ac4bc5a 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -955,6 +955,38 @@ static void show_combined_header(struct combine_diff_path *elem,
 				 line_prefix, c_meta, c_reset);
 }
 
+static int simplify_parents(struct combine_diff_path *p, int nr)
+{
+	int i;
+	for (i = 1; i < nr; i++)
+		if (p->parent[i].mode != p->parent[i-1].mode ||
+		    hashcmp(p->parent[i].sha1, p->parent[i-1].sha1))
+			return nr;
+	return 1;
+}
+
+static void show_single_parent_patch(struct combine_diff_path *elem,
+				     int working_tree_file,
+				     struct rev_info *rev)
+{
+	struct diff_filepair pair;
+
+	memset(&pair, 0, sizeof(pair));
+	pair.one = alloc_filespec(elem->path);
+	pair.two = alloc_filespec(elem->path);
+	pair.status = elem->parent[0].status;
+
+	fill_filespec(pair.one, elem->parent[0].sha1, 1, elem->parent[0].mode);
+	if (working_tree_file)
+		fill_filespec(pair.two, null_sha1, 0, elem->mode);
+	else
+		fill_filespec(pair.two, elem->sha1, 1, elem->mode);
+
+	run_diff(&pair, &rev->diffopt);
+	free_filespec(pair.one);
+	free_filespec(pair.two);
+}
+
 static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 			    int dense, int working_tree_file,
 			    struct rev_info *rev)
@@ -972,6 +1004,13 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 	int is_binary;
 	const char *line_prefix = diff_line_prefix(opt);
 
+	if (rev->simplify_combined_diff)
+		num_parent = simplify_parents(elem, num_parent);
+	if (num_parent == 1) {
+		show_single_parent_patch(elem, working_tree_file, rev);
+		return;
+	}
+
 	context = opt->context;
 	userdiff = userdiff_find_by_path(elem->path);
 	if (!userdiff)
@@ -1181,6 +1220,8 @@ static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct re
 	if (rev->loginfo && !rev->no_commit_id)
 		show_log(rev);
 
+	if (rev->simplify_combined_diff)
+		num_parent = simplify_parents(p, num_parent);
 
 	if (opt->output_format & DIFF_FORMAT_RAW) {
 		printf("%s", line_prefix);
@@ -1247,12 +1288,16 @@ static void free_combined_pair(struct diff_filepair *pair)
  * but currently nobody uses it, so this should suffice for now.
  */
 static struct diff_filepair *combined_pair(struct combine_diff_path *p,
-					   int num_parent)
+					   int num_parent,
+					   struct rev_info *rev)
 {
 	int i;
 	struct diff_filepair *pair;
 	struct diff_filespec *pool;
 
+	if (rev->simplify_combined_diff)
+		num_parent = simplify_parents(p, num_parent);
+
 	pair = xmalloc(sizeof(*pair));
 	pool = xcalloc(num_parent + 1, sizeof(struct diff_filespec));
 	pair->one = pool + 1;
@@ -1277,7 +1322,8 @@ static struct diff_filepair *combined_pair(struct combine_diff_path *p,
 static void handle_combined_callback(struct diff_options *opt,
 				     struct combine_diff_path *paths,
 				     int num_parent,
-				     int num_paths)
+				     int num_paths,
+				     struct rev_info *rev)
 {
 	struct combine_diff_path *p;
 	struct diff_queue_struct q;
@@ -1287,7 +1333,8 @@ static void handle_combined_callback(struct diff_options *opt,
 	q.alloc = num_paths;
 	q.nr = num_paths;
 	for (i = 0, p = paths; p; p = p->next)
-		q.queue[i++] = combined_pair(p, num_parent);
+		q.queue[i++] = combined_pair(p, num_parent, rev);
+
 	opt->format_callback(&q, opt, opt->format_callback_data);
 	for (i = 0; i < num_paths; i++)
 		free_combined_pair(q.queue[i]);
@@ -1499,7 +1546,8 @@ void diff_tree_combined(const unsigned char *sha1,
 			 (DIFF_FORMAT_NUMSTAT|DIFF_FORMAT_DIFFSTAT))
 			needsep = 1;
 		else if (opt->output_format & DIFF_FORMAT_CALLBACK)
-			handle_combined_callback(opt, paths, num_parent, num_paths);
+			handle_combined_callback(opt, paths, num_parent,
+						 num_paths, rev);
 
 		if (opt->output_format & DIFF_FORMAT_PATCH) {
 			if (needsep)
diff --git a/diff.c b/diff.c
index 867f034..558a520 100644
--- a/diff.c
+++ b/diff.c
@@ -3089,7 +3089,7 @@ static void strip_prefix(int prefix_length, const char **namep, const char **oth
 	}
 }
 
-static void run_diff(struct diff_filepair *p, struct diff_options *o)
+void run_diff(struct diff_filepair *p, struct diff_options *o)
 {
 	const char *pgm = external_diff();
 	struct strbuf msg;
diff --git a/diff.h b/diff.h
index b4a624d..a669be0 100644
--- a/diff.h
+++ b/diff.h
@@ -356,4 +356,6 @@ extern int print_stat_summary(FILE *fp, int files,
 			      int insertions, int deletions);
 extern void setup_diff_pager(struct diff_options *);
 
+extern void run_diff(struct diff_filepair *p, struct diff_options *o);
+
 #endif /* DIFF_H */
diff --git a/revision.c b/revision.c
index 615535c..0b8516b 100644
--- a/revision.c
+++ b/revision.c
@@ -1820,6 +1820,10 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->diff = 1;
 		revs->dense_combined_merges = 1;
 		revs->combine_merges = 1;
+	} else if (!strcmp(arg, "--simplify-combined-diff")) {
+		revs->simplify_combined_diff = 1;
+	} else if (!strcmp(arg, "--no-simplify-combined-diff")) {
+		revs->simplify_combined_diff = 0;
 	} else if (!strcmp(arg, "-v")) {
 		revs->verbose_header = 1;
 	} else if (!strcmp(arg, "--pretty")) {
diff --git a/revision.h b/revision.h
index a620530..48cb16e 100644
--- a/revision.h
+++ b/revision.h
@@ -122,6 +122,7 @@ struct rev_info {
 			ignore_merges:1,
 			combine_merges:1,
 			dense_combined_merges:1,
+			simplify_combined_diff:1,
 			always_show_header:1;
 
 	/* Format info */
diff --git a/t/t4038-diff-combined.sh b/t/t4038-diff-combined.sh
index 41913c3..5e141ad 100755
--- a/t/t4038-diff-combined.sh
+++ b/t/t4038-diff-combined.sh
@@ -401,4 +401,137 @@ test_expect_success 'combine diff missing delete bug' '
 	compare_diff_patch expected actual
 '
 
+# make a commit with contents "$1" and "$2" in the paths
+# "one" and "two" respectively, and parents $3, $4, etc
+#
+# If the content is of the form "mode:content", then the
+# mode for the given file is set (otherwise it defaults 100644).
+mkcommit() {
+	(
+		GIT_INDEX_FILE=tmp.index &&
+		export GIT_INDEX_FILE &&
+		for path in one two; do
+			case "$1" in
+			*:*)
+				mode=$(echo "$1" | cut -d: -f1)
+				content=$(echo "$1" | cut -d: -f2)
+				;;
+			*)
+				mode=100644
+				content=$1
+			esac
+			blob=$(echo "$content" | git hash-object -w --stdin) &&
+			git update-index --add --cacheinfo $mode,$blob,$path &&
+			shift || exit 1
+		done
+		tree=$(git write-tree) &&
+		parents=$(for p in "$@"; do echo "-p $p"; done) &&
+		git commit-tree $tree $parents </dev/null
+	)
+}
+
+# we create a merge commit where path "one" can be simplified, but
+# path "two" cannot
+test_expect_success 'simplify combined --raw' '
+	side1=$(mkcommit base content1) &&
+	side2=$(mkcommit base content2) &&
+	merge=$(mkcommit new new $side1 $side2) &&
+	cat >expect <<-\EOF &&
+	:100644 100644 df967b9... 3e75765... M	one
+	::100644 100644 100644 ac3e272... 637f034... 3e75765... MM	two
+	EOF
+
+	git diff-tree -c --simplify-combined-diff \
+		--abbrev --format= $merge >actual &&
+	test_cmp expect actual
+'
+
+# we do not use compare_diff_patch here because we want to
+# make sure we get the headers right, too
+test_expect_success 'simplify combined --patch' '
+	side1=$(mkcommit base content1) &&
+	side2=$(mkcommit base content2) &&
+	merge=$(mkcommit new new $side1 $side2) &&
+
+	cat >expect <<-\EOF &&
+	diff --git a/one b/one
+	index df967b9..3e75765 100644
+	--- a/one
+	+++ b/one
+	@@ -1 +1 @@
+	-base
+	+new
+	diff --combined two
+	index ac3e272,637f034..3e75765
+	--- a/two
+	+++ b/two
+	@@@ -1,1 -1,1 +1,1 @@@
+	- content1
+	 -content2
+	++new
+	EOF
+
+	git diff-tree -c --simplify-combined-diff -p \
+		--format= $merge >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'do not simplify unless all parents are identical' '
+	side1=$(mkcommit base content1) &&
+	test_tick &&
+	side2=$(mkcommit base content1) &&
+	side3=$(mkcommit base content3) &&
+	merge=$(mkcommit new new $side1 $side2 $side3) &&
+
+	cat >expect <<-\EOF &&
+	diff --git a/one b/one
+	index df967b9..3e75765 100644
+	--- a/one
+	+++ b/one
+	@@ -1 +1 @@
+	-base
+	+new
+	diff --combined two
+	index ac3e272,ac3e272,27d10cc..3e75765
+	--- a/two
+	+++ b/two
+	@@@@ -1,1 -1,1 -1,1 +1,1 @@@@
+	-- content1
+	  -content3
+	+++new
+	EOF
+
+	git diff-tree -c --simplify-combined-diff -p \
+		--format= $merge >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'do not simplify away mode changes' '
+	side1=$(mkcommit 100644:base 100644:base) &&
+	side2=$(mkcommit 100644:base 100755:base) &&
+	merge=$(mkcommit new new $side1 $side2) &&
+
+	cat >expect <<-\EOF &&
+	diff --git a/one b/one
+	index df967b9..3e75765 100644
+	--- a/one
+	+++ b/one
+	@@ -1 +1 @@
+	-base
+	+new
+	diff --combined two
+	index df967b9,df967b9..3e75765
+	mode 100644,100755..100644
+	--- a/two
+	+++ b/two
+	@@@ -1,1 -1,1 +1,1 @@@
+	--base
+	++new
+	EOF
+
+	git diff-tree -c --simplify-combined-diff -p \
+		--format= $merge >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.1.0.rc0.286.g5c67d74

[PATCH v2 5/6] stash: default listing to "--cc --simplify-combined-diff"

[Jeff King]

When you list stashes, you can provide arbitrary git-log
options to change the display. However, adding just "-p"
does nothing, because each stash is actually a merge commit.

This implementation detail is easy to forget, leading to
confused users who think "-p" is not working. We can make
this easier by specifying "--cc" as a default ourselves
(which does nothing if no diff format is requested by the
user).

The resulting diff would then be a combined diff between the
base commit and the stashed index state. In many cases,
though, the user did not stash anything in the index. We can
further simplify this to a normal pairwise diff by using
"--simplify-combined-diff".

Signed-off-by: Jeff King <peff@peff.net>
---
 git-stash.sh     |  3 ++-
 t/t3903-stash.sh | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 65 insertions(+), 1 deletion(-)

diff --git a/git-stash.sh b/git-stash.sh
index bcc757b..a0246d5 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -297,7 +297,8 @@ have_stash () {
 
 list_stash () {
 	have_stash || return 0
-	git log --format="%gd: %gs" -g "$@" $ref_stash --
+	git log --format="%gd: %gs" -g --cc --simplify-combined-diff \
+		"$@" $ref_stash --
 }
 
 show_stash () {
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 5b79b21..465f824 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -685,4 +685,67 @@ test_expect_success 'handle stash specification with spaces' '
 	grep pig file
 '
 
+test_expect_success 'stash list implies --cc' '
+	git stash clear &&
+	git reset --hard &&
+	echo index >file &&
+	git add file &&
+	echo working >file &&
+	git stash &&
+	git stash list -p >actual &&
+	cat >expect <<-\EOF &&
+	stash@{0}: WIP on master: b27a2bc subdir
+
+	diff --cc file
+	index 257cc56,9015a7a..d26b33d
+	--- a/file
+	+++ b/file
+	@@@ -1,1 -1,1 +1,1 @@@
+	- foo
+	 -index
+	++working
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'stash list implies --simplify-combined-diff' '
+	git stash clear &&
+	git reset --hard &&
+	echo working >file &&
+	git stash &&
+	git stash list -p >actual &&
+	cat >expect <<-\EOF &&
+	stash@{0}: WIP on master: b27a2bc subdir
+
+	diff --git a/file b/file
+	index 257cc56..d26b33d 100644
+	--- a/file
+	+++ b/file
+	@@ -1 +1 @@
+	-foo
+	+working
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success '--no-simplify-combined-diff overrides default' '
+	git stash clear &&
+	git reset --hard &&
+	echo working >file &&
+	git stash &&
+	git stash list -p --no-simplify-combined-diff >actual &&
+	cat >expect <<-\EOF &&
+	stash@{0}: WIP on master: b27a2bc subdir
+
+	diff --cc file
+	index 257cc56,257cc56..d26b33d
+	--- a/file
+	+++ b/file
+	@@@ -1,1 -1,1 +1,1 @@@
+	--foo
+	++working
+	EOF
+	test_cmp expect actual
+'
+
 test_done
-- 
2.1.0.rc0.286.g5c67d74

[PATCH v2 6/6] stash: show combined diff with "stash show"

[Jeff King]

A stash may store not only working tree changes, but also
changes to the index. However, "git stash show" will only
ever show the working tree changes. We can instead show both
as a combined diff, but use "--simplify-combined-diff" so
that we show a normal pairwise diff in the common case that
there were no index changes.

Signed-off-by: Jeff King <peff@peff.net>
---
I'm still a little iffy on this one in case somebody really doesn't want
to see the saved index state. But with the earlier cleanups, you can at
least see that it doesn't even impact any existing tests.

 git-stash.sh     |  3 ++-
 t/t3903-stash.sh | 22 ++++++++++++++++++++++
 2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/git-stash.sh b/git-stash.sh
index a0246d5..ae73ba4 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -304,7 +304,8 @@ list_stash () {
 show_stash () {
 	assert_stash_like "$@"
 
-	git diff ${FLAGS:---stat} $b_commit $w_commit
+	git show --format= --simplify-combined-diff \
+		${FLAGS:---stat} $w_commit
 }
 
 #
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 465f824..36b97a4 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -537,6 +537,28 @@ test_expect_success 'stash show -p - no stashes on stack, stash-like argument' '
 	test_cmp expected actual
 '
 
+test_expect_success 'stash show -p will show modified index' '
+	git stash clear &&
+	test_when_finished "git reset --hard HEAD" &&
+	git reset --hard &&
+	echo index >file &&
+	git add file &&
+	echo working >file &&
+	git stash &&
+	cat >expect <<-\EOF &&
+	diff --cc file
+	index 7601807,9015a7a..d26b33d
+	--- a/file
+	+++ b/file
+	@@@ -1,1 -1,1 +1,1 @@@
+	- baz
+	 -index
+	++working
+	EOF
+	git stash show -p >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'stash drop - fail early if specified stash is not a stash reference' '
 	git stash clear &&
 	test_when_finished "git reset --hard HEAD && git stash clear" &&
-- 
2.1.0.rc0.286.g5c67d74

Re: [RFC/PATCH 3/2] stash: show combined diff with "stash show"

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> ... People might be doing things like "git stash show | git
> apply", and would want to ignore the index content ...

FWIW, that is exactly how I use "git stash show -p" most of the time.

Re: [PATCH v2 5/6] stash: default listing to "--cc --simplify-combined-diff"

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> When you list stashes, you can provide arbitrary git-log
> options to change the display. However, adding just "-p"
> does nothing, because each stash is actually a merge commit.
>
> This implementation detail is easy to forget, leading to
> confused users who think "-p" is not working. We can make
> this easier by specifying "--cc" as a default ourselves
> (which does nothing if no diff format is requested by the
> user).

Sigh.

"git log --cc" is one of the things I wanted for a long time to fix.
When the user explicitly asks "--cc", we currently ignore it, but
because we know the user wants to view combined diff, we should turn
"-p" on automatically.  And the change this patch introduces will be
broken when we fix "log --cc" ("stash list" will end up always
showing the patch, without a way to disable it).

Can you make this conditional?  Do this only when <options> are
given to "git stash list" command and that includes "-p" or
something?

Re: [PATCH v2 4/6] add --simplify-combined-diff option

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> When showing a combined diff, we are comparing two (or more)
> parents to a final state, and some of these states may be
> the same.  Here's a table of the possible contents for a
> given path (for two parents, but we will generalize to more
> in a moment; we also omit isomorphic cases where the parents
> are swapped):
>
>   case | M | P1 | P2
>   -------------------
>   1    | A | A  | A
>   2    | A | A  | B
>   3    | A | B  | B
>   4    | A | B  | C
>
> In case 1, the path was not relevant to the merge at all,
> and we omit it as uninteresting. In case 2, we did resolve
> the path, but in favor of one side. We also consider this
> uninteresting and do not show the diff.
>
> In case 4, we had a real content-level merge, and the
> combined diff is interesting. We show it.
>
> That leaves case 3, in which both parents are the same, but
> the merge picks a new content. This should be rare in
> normal merges, though it could happen if you updated an
> unrelated file due to a resolution elsewhere (i.e., an evil
> merge that crosses a file boundary). But it happens
> frequently in the fake merge commits we create for stashes,
> in which one parent is the base of the stash and the other
> is the index (in which case it simply means that the index
> entry for the path was not touched).
>
> Right now, we treat it the same as case 4, and show a normal
> combined diff. However, the result is harder to read, and
> the combined nature of the diff gives no extra information;
> every marker in the combined diff will be identical for both
> parents.
>
> This patch adds a new option, "--simplify-combined-diff",
> which converts this case into a normal, non-combined diff.
> It would not be correct to simply omit it, because there
> really is an interesting change going from B..A. It's just
> that there are not two interesting changes, which the
> combined diff would show.
>
> When generalizing this to more than two parents, we have two
> options:
>
>   1. Either simply to a single parent content, or not at all.
>
>   2. Omit parents whose contents are duplicates of other
>      parents.
>
> For a case like "A B B C", option (2) would still result in
> a combined diff, but one with fewer sources. However, it
> would also be ambiguous. The parents in a combined diff are
> marked only by their position, so omitting a position means
> that a reader can no longer tell which line goes with which
> parent.
>
> Instead, we choose option (1). Either you get the full
> combined diff, or you get a normal non-combined diff.

Very nicely analyzed.  The changes to the code looked also good from
a cursory read.

Thanks.

Re: [PATCH v2 5/6] stash: default listing to "--cc --simplify-combined-diff"

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Jeff King <peff@peff.net> writes:
>
>> When you list stashes, you can provide arbitrary git-log
>> options to change the display. However, adding just "-p"
>> does nothing, because each stash is actually a merge commit.
>>
>> This implementation detail is easy to forget, leading to
>> confused users who think "-p" is not working. We can make
>> this easier by specifying "--cc" as a default ourselves
>> (which does nothing if no diff format is requested by the
>> user).
>
> Sigh.
>
> "git log --cc" is one of the things I wanted for a long time to fix.
> When the user explicitly asks "--cc", we currently ignore it, but
> because we know the user wants to view combined diff, we should turn
> "-p" on automatically.  And the change this patch introduces will be
> broken when we fix "log --cc" ("stash list" will end up always
> showing the patch, without a way to disable it).
>
> Can you make this conditional?  Do this only when <options> are
> given to "git stash list" command and that includes "-p" or
> something?

Perhaps something along this line?

 git-stash.sh | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/git-stash.sh b/git-stash.sh
index ae73ba4..0db1b19 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -297,8 +297,15 @@ have_stash () {
 
 list_stash () {
 	have_stash || return 0
-	git log --format="%gd: %gs" -g --cc --simplify-combined-diff \
-		"$@" $ref_stash --
+	case " $* " in
+	*' --cc '*)
+		;; # the user knows what she is doing
+	*' -p '* | *' -U'*)
+		set x "--cc" "--simplify-combined-diff" "$@"
+		shift
+		;;
+	esac
+	git log --format="%gd: %gs" -g "$@" $ref_stash --
 }
 
 show_stash () {

Re: [PATCH v2 5/6] stash: default listing to "--cc --simplify-combined-diff"

[Jeff King]

On Wed, Jul 30, 2014 at 12:43:09PM -0700, Junio C Hamano wrote:

> > "git log --cc" is one of the things I wanted for a long time to fix.
> > When the user explicitly asks "--cc", we currently ignore it, but
> > because we know the user wants to view combined diff, we should turn
> > "-p" on automatically.  And the change this patch introduces will be
> > broken when we fix "log --cc" ("stash list" will end up always
> > showing the patch, without a way to disable it).
> >
> > Can you make this conditional?  Do this only when <options> are
> > given to "git stash list" command and that includes "-p" or
> > something?

I'm definitely sympathetic. Using "--cc" with the diff family _does_
imply "-p" already, so it would be nice to do the same for the log
family.

> Perhaps something along this line?
> 
>  git-stash.sh | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/git-stash.sh b/git-stash.sh
> index ae73ba4..0db1b19 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -297,8 +297,15 @@ have_stash () {
>  
>  list_stash () {
>  	have_stash || return 0
> -	git log --format="%gd: %gs" -g --cc --simplify-combined-diff \
> -		"$@" $ref_stash --
> +	case " $* " in
> +	*' --cc '*)
> +		;; # the user knows what she is doing
> +	*' -p '* | *' -U'*)
> +		set x "--cc" "--simplify-combined-diff" "$@"
> +		shift
> +		;;
> +	esac
> +	git log --format="%gd: %gs" -g "$@" $ref_stash --

Ugh. I'd generally like to avoid this sort of ad-hoc command line
parsing, as it is easy to get confused by option arguments or
even non-options. At least we do not have to worry about pathspecs here,
as we already do an explicit "--" (so we might be confused by them, but
they are broken anyway).

Still, I wonder if a cleaner solution is to provide alternate "default
to this" options for the revision.c option parser. We already have
"--default" to pick the default starting point. Could we do something
like "--default-merge-handling=cc" or something?

There's a similar ad-hoc parsing case in "stash show", where we add
"--stat" if no arguments are given, but we have no clue if a diff format
was given (so "git stash show --color" accidentally turns on patch
format!). Something like "--default-diff-format=stat" would be more
robust.

I dunno. Maybe it is over-engineering. I just hate to see solutions that
work most of the time and crumble in weird corner cases, even if people
don't hit those corner cases very often.

-Peff

Re: [RFC/PATCH 3/2] stash: show combined diff with "stash show"

[Jeff King]

On Tue, Jul 29, 2014 at 11:13:37AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > ... People might be doing things like "git stash show | git
> > apply", and would want to ignore the index content ...
> 
> FWIW, that is exactly how I use "git stash show -p" most of the time.

Like I said, I'm iffy on this part of the series for that reason. But
I'm curious: what do you think should happen in such a use case when
there are staged contents in the index? Right now we completely ignore
them.

-Peff

Re: [RFC/PATCH 3/2] stash: show combined diff with "stash show"

[Junio C Hamano]

On Wed, Jul 30, 2014 at 5:17 PM, Jeff King <peff@peff.net> wrote:
>
> Like I said, I'm iffy on this part of the series for that reason. But
> I'm curious: what do you think should happen in such a use case when
> there are staged contents in the index? Right now we completely ignore
> them.

I think ignoring is absolutely the right thing ;-)

Unlike "stash pop", which is "try to bring me back to exactly the same state",
it is a strong indication that the user wants to further tweak the previous work
to grab a patch and apply to the working tree---and while you are working
inside the working tree, you haven't started adding the index contents, so
"git diff" after such an operation should show what you grabbed out of the
stash, applied to the working tree, relative to what you had in the index.

Re: [PATCH v2 5/6] stash: default listing to "--cc --simplify-combined-diff"

[Keller, Jacob E]

On Wed, 2014-07-30 at 20:09 -0400, Jeff King wrote:
> On Wed, Jul 30, 2014 at 12:43:09PM -0700, Junio C Hamano wrote:
> 
> > > "git log --cc" is one of the things I wanted for a long time to fix.
> > > When the user explicitly asks "--cc", we currently ignore it, but
> > > because we know the user wants to view combined diff, we should turn
> > > "-p" on automatically.  And the change this patch introduces will be
> > > broken when we fix "log --cc" ("stash list" will end up always
> > > showing the patch, without a way to disable it).
> > >
> > > Can you make this conditional?  Do this only when <options> are
> > > given to "git stash list" command and that includes "-p" or
> > > something?
> 
> I'm definitely sympathetic. Using "--cc" with the diff family _does_
> imply "-p" already, so it would be nice to do the same for the log
> family.
> 
> > Perhaps something along this line?
> > 
> >  git-stash.sh | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/git-stash.sh b/git-stash.sh
> > index ae73ba4..0db1b19 100755
> > --- a/git-stash.sh
> > +++ b/git-stash.sh
> > @@ -297,8 +297,15 @@ have_stash () {
> >  
> >  list_stash () {
> >  	have_stash || return 0
> > -	git log --format="%gd: %gs" -g --cc --simplify-combined-diff \
> > -		"$@" $ref_stash --
> > +	case " $* " in
> > +	*' --cc '*)
> > +		;; # the user knows what she is doing
> > +	*' -p '* | *' -U'*)
> > +		set x "--cc" "--simplify-combined-diff" "$@"
> > +		shift
> > +		;;
> > +	esac
> > +	git log --format="%gd: %gs" -g "$@" $ref_stash --
> 
> Ugh. I'd generally like to avoid this sort of ad-hoc command line
> parsing, as it is easy to get confused by option arguments or
> even non-options. At least we do not have to worry about pathspecs here,
> as we already do an explicit "--" (so we might be confused by them, but
> they are broken anyway).
> 
> Still, I wonder if a cleaner solution is to provide alternate "default
> to this" options for the revision.c option parser. We already have
> "--default" to pick the default starting point. Could we do something
> like "--default-merge-handling=cc" or something?
> 
> There's a similar ad-hoc parsing case in "stash show", where we add
> "--stat" if no arguments are given, but we have no clue if a diff format
> was given (so "git stash show --color" accidentally turns on patch
> format!). Something like "--default-diff-format=stat" would be more
> robust.
> 
> I dunno. Maybe it is over-engineering. I just hate to see solutions that
> work most of the time and crumble in weird corner cases, even if people
> don't hit those corner cases very often.
> 
> -Peff

I agree with you on this one. Those corner cases tend to bite the worst.

Thanks,
Jake

> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html