{"thread":{"id":"37239","subject":"[PATCH 0/2] improving \"git stash list -p\"","startedAt":"2014-07-29T11:53:34Z","lastAt":"2014-08-12T23:30:56Z","messageCount":20,"participants":["Jeff King","Junio C Hamano","Keller, Jacob E"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"246896","messageId":"20140729115334.GA8976@peff.net","threadId":"37239","inReplyTo":null,"subject":"[PATCH 0/2] improving \"git stash list -p\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-29T11:53:34Z","receivedAt":"2014-07-29T11:53:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Imagine you have a simple stash like:\n\n  git init\n  echo base >file && git add file && git commit -m file\n  echo new >file\n  git stash\n\nBefore this series:\n\n  $ git stash list -p\n  stash@{0}: WIP on master: 7a1fd22 file\n\nEr, what? It didn't give me a patch. Oh, that's because stashes are\nmerge commits, and we need to tell it how to handle merges.\n\n  $ git stash list -p --cc\n  stash@{0}: WIP on master: 7a1fd22 file\n\n  diff --cc file\n  index df967b9,df967b9..3e75765\n  --- a/file\n  +++ b/file\n  @@@ -1,1 -1,1 +1,1 @@@\n  --base\n  ++new\n\nBetter, though the combined diff is useless, since I didn't touch the\nindex at all. Here it is after this series:\n\n  $ git stash list -p\n  stash@{0}: WIP on master: 7a1fd22 file\n\n  diff --cc file\n  index df967b9..3e75765\n  --- a/file\n  +++ b/file\n  @@ -1,1 +1,1 @@\n  -base\n  +new\n\nAh, a nice readable diff with no hassle.\n\n  [1/2]: add --simplify-combined-diff option\n  [2/2]: stash: default listing to \"--cc --simplify-combined-diff\"\n\n-Peff\n"},{"id":"246897","messageId":"20140729120056.GA9175@peff.net","threadId":"37239","inReplyTo":"20140729115334.GA8976@peff.net","subject":"[PATCH 1/2] add --simplify-combined-diff option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-29T12:00:56Z","receivedAt":"2014-07-29T12:00:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When showing a combined diff, we are comparing two (or more)\nparents to a final state, and some of these states may be\nthe same.  Here's a table of the possible contents for a\ngiven path (for two parents, but we will generalize to more\nin a moment; we also omit isomorphic cases where the parents\nare swapped):\n\n  case | M | P1 | P2\n  -------------------\n  1    | A | A  | A\n  2    | A | A  | B\n  3    | A | B  | B\n  4    | A | B  | C\n\nIn case 1, the path was not relevant to the merge at all,\nand we omit it as uninteresting. In case 2, we did resolve\nthe path, but in favor of one side. We also consider this\nuninteresting and do not show the diff.\n\nIn case 4, we had a real content-level merge, and the\ncombined diff is interesting. We show it.\n\nThat leaves case 3, in which both parents are the same, but\nthe merge picks a new content. This should be rare in\nnormal merges, though it could happen if you updated an\nunrelated file due to a resolution elsewhere (i.e., an evil\nmerge that crosses a file boundary). But it happens\nfrequently in the fake merge commits we create for stashes,\nin which one parent is the base of the stash and the other\nis the index (in which case it simply means that the index\nentry for the path was not touched).\n\nRight now, we treat it the same as case 4, and show a normal\ncombined diff. However, the result is harder to read, and\nthe combined nature of the diff gives no extra information;\nevery marker in the combined diff will be identical for both\nparents.\n\nThis patch adds a new option, \"--simplify-combined-diff\",\nwhich converts this case into a normal, non-combined diff.\nIt would not be correct to simply omit it, because there\nreally is an interesting change going from B..A. It's just\nthat there are not two interesting changes, which the\ncombined diff would show.\n\nWhen generalizing this to more than two parents, we have two\noptions:\n\n  1. Either simply to a single parent content, or not at all.\n\n  2. Omit parents whose contents are duplicates of other\n     parents.\n\nFor a case like \"A B B C\", option (2) would still result in\na combined diff, but one with fewer sources. However, it\nwould also be ambiguous. The parents in a combined diff are\nmarked only by their position, so omitting a position means\nthat a reader can no longer tell which line goes with which\nparent.\n\nInstead, we choose option (1). Either you get the full\ncombined diff, or you get a normal non-combined diff.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThe implementation just cuts the number of parents down to 1, but\notherwise runs through the same combined-diff code.  The resulting\npairwise combined-diff differs from a normal diff in a few ways:\n\n  1. The header line is still \"diff --combined\" (or \"--cc\") instead of\n     \"diff --git\", and it mentions the filename only once.\n\n  2. The index lines do not contain the file mode; for combined diffs,\n     we generate a separate mode line (but only when it has something\n     interesting to show).\n\n  3. The hunk header for a single-line change says \"-1\" in the regular\n     code path, but \"-1,1\" in the combined code path.\n\nIs there any value in keeping this as a pseudo-combined diff (i.e., with\nthe combined header, but only a single parent)? It should not be too\nhard to just punt to the regular builtin_diff code path when\n\"num_parents == 1\".\n\n Documentation/rev-list-options.txt |   8 +++\n combine-diff.c                     |  30 +++++++--\n revision.c                         |   4 ++\n revision.h                         |   1 +\n t/t4038-diff-combined.sh           | 134 +++++++++++++++++++++++++++++++++++++\n 5 files changed, 173 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex deb8cca..7331bcc 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -805,6 +805,14 @@ options may be given. See linkgit:git-diff-files[1] for more options.\n \tin that case, the output represents the changes the merge\n \tbrought _into_ the then-current branch.\n \n+--simplify-combined-diff::\n+\tWhen showing a combined diff with `-c` or `--cc`, if a given\n+\tpath has identical content in all parents, show only a pairwise\n+\tdiff from that content, rather than a true combined diff. This\n+\tis easier for humans to read, though it may confuse an automatic\n+\tparser (as some paths in the output may be combined diffs, and\n+\tothers may not).\n+\n -r::\n \tShow recursive diffs.\n \ndiff --git a/combine-diff.c b/combine-diff.c\nindex f9975d2..60e54a7 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -955,6 +955,16 @@ static void show_combined_header(struct combine_diff_path *elem,\n \t\t\t\t line_prefix, c_meta, c_reset);\n }\n \n+static int simplify_parents(struct combine_diff_path *p, int nr)\n+{\n+\tint i;\n+\tfor (i = 1; i < nr; i++)\n+\t\tif (p->parent[i].mode != p->parent[i-1].mode ||\n+\t\t    hashcmp(p->parent[i].sha1, p->parent[i-1].sha1))\n+\t\t\treturn nr;\n+\treturn 1;\n+}\n+\n static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \t\t\t    int dense, int working_tree_file,\n \t\t\t    struct rev_info *rev)\n@@ -979,6 +989,9 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \tif (DIFF_OPT_TST(opt, ALLOW_TEXTCONV))\n \t\ttextconv = userdiff_get_textconv(userdiff);\n \n+\tif (rev->simplify_combined_diff)\n+\t\tnum_parent = simplify_parents(elem, num_parent);\n+\n \t/* Read the result of merge first */\n \tif (!working_tree_file)\n \t\tresult = grab_blob(elem->sha1, elem->mode, &result_size,\n@@ -1181,6 +1194,8 @@ static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct re\n \tif (rev->loginfo && !rev->no_commit_id)\n \t\tshow_log(rev);\n \n+\tif (rev->simplify_combined_diff)\n+\t\tnum_parent = simplify_parents(p, num_parent);\n \n \tif (opt->output_format & DIFF_FORMAT_RAW) {\n \t\tprintf(\"%s\", line_prefix);\n@@ -1247,12 +1262,16 @@ static void free_combined_pair(struct diff_filepair *pair)\n  * but currently nobody uses it, so this should suffice for now.\n  */\n static struct diff_filepair *combined_pair(struct combine_diff_path *p,\n-\t\t\t\t\t   int num_parent)\n+\t\t\t\t\t   int num_parent,\n+\t\t\t\t\t   struct rev_info *rev)\n {\n \tint i;\n \tstruct diff_filepair *pair;\n \tstruct diff_filespec *pool;\n \n+\tif (rev->simplify_combined_diff)\n+\t\tnum_parent = simplify_parents(p, num_parent);\n+\n \tpair = xmalloc(sizeof(*pair));\n \tpool = xcalloc(num_parent + 1, sizeof(struct diff_filespec));\n \tpair->one = pool + 1;\n@@ -1277,7 +1296,8 @@ static struct diff_filepair *combined_pair(struct combine_diff_path *p,\n static void handle_combined_callback(struct diff_options *opt,\n \t\t\t\t     struct combine_diff_path *paths,\n \t\t\t\t     int num_parent,\n-\t\t\t\t     int num_paths)\n+\t\t\t\t     int num_paths,\n+\t\t\t\t     struct rev_info *rev)\n {\n \tstruct combine_diff_path *p;\n \tstruct diff_queue_struct q;\n@@ -1287,7 +1307,8 @@ static void handle_combined_callback(struct diff_options *opt,\n \tq.alloc = num_paths;\n \tq.nr = num_paths;\n \tfor (i = 0, p = paths; p; p = p->next)\n-\t\tq.queue[i++] = combined_pair(p, num_parent);\n+\t\tq.queue[i++] = combined_pair(p, num_parent, rev);\n+\n \topt->format_callback(&q, opt, opt->format_callback_data);\n \tfor (i = 0; i < num_paths; i++)\n \t\tfree_combined_pair(q.queue[i]);\n@@ -1498,7 +1519,8 @@ void diff_tree_combined(const unsigned char *sha1,\n \t\t\t (DIFF_FORMAT_NUMSTAT|DIFF_FORMAT_DIFFSTAT))\n \t\t\tneedsep = 1;\n \t\telse if (opt->output_format & DIFF_FORMAT_CALLBACK)\n-\t\t\thandle_combined_callback(opt, paths, num_parent, num_paths);\n+\t\t\thandle_combined_callback(opt, paths, num_parent,\n+\t\t\t\t\t\t num_paths, rev);\n \n \t\tif (opt->output_format & DIFF_FORMAT_PATCH) {\n \t\t\tif (needsep)\ndiff --git a/revision.c b/revision.c\nindex 2571ada..a242485 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1820,6 +1820,10 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\trevs->diff = 1;\n \t\trevs->dense_combined_merges = 1;\n \t\trevs->combine_merges = 1;\n+\t} else if (!strcmp(arg, \"--simplify-combined-diff\")) {\n+\t\trevs->simplify_combined_diff = 1;\n+\t} else if (!strcmp(arg, \"--no-simplify-combined-diff\")) {\n+\t\trevs->simplify_combined_diff = 0;\n \t} else if (!strcmp(arg, \"-v\")) {\n \t\trevs->verbose_header = 1;\n \t} else if (!strcmp(arg, \"--pretty\")) {\ndiff --git a/revision.h b/revision.h\nindex a620530..48cb16e 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -122,6 +122,7 @@ struct rev_info {\n \t\t\tignore_merges:1,\n \t\t\tcombine_merges:1,\n \t\t\tdense_combined_merges:1,\n+\t\t\tsimplify_combined_diff:1,\n \t\t\talways_show_header:1;\n \n \t/* Format info */\ndiff --git a/t/t4038-diff-combined.sh b/t/t4038-diff-combined.sh\nindex 41913c3..d522474 100755\n--- a/t/t4038-diff-combined.sh\n+++ b/t/t4038-diff-combined.sh\n@@ -401,4 +401,138 @@ test_expect_success 'combine diff missing delete bug' '\n \tcompare_diff_patch expected actual\n '\n \n+# make a commit with contents \"$1\" and \"$2\" in the paths\n+# \"one\" and \"two\" respectively, and parents $3, $4, etc\n+#\n+# If the content is of the form \"mode:content\", then the\n+# mode for the given file is set (otherwise it defaults 100644).\n+mkcommit() {\n+\t(\n+\t\tGIT_INDEX_FILE=tmp.index &&\n+\t\texport GIT_INDEX_FILE &&\n+\t\tfor path in one two; do\n+\t\t\tcase \"$1\" in\n+\t\t\t*:*)\n+\t\t\t\tmode=$(echo \"$1\" | cut -d: -f1)\n+\t\t\t\tcontent=$(echo \"$1\" | cut -d: -f2)\n+\t\t\t\t;;\n+\t\t\t*)\n+\t\t\t\tmode=100644\n+\t\t\t\tcontent=$1\n+\t\t\tesac\n+\t\t\tblob=$(echo \"$content\" | git hash-object -w --stdin) &&\n+\t\t\tgit update-index --add --cacheinfo $mode,$blob,$path &&\n+\t\t\tshift || exit 1\n+\t\tdone\n+\t\ttree=$(git write-tree) &&\n+\t\tparents=$(for p in \"$@\"; do echo \"-p $p\"; done) &&\n+\t\tgit commit-tree $tree $parents </dev/null\n+\t)\n+}\n+\n+# we create a merge commit where path \"one\" can be simplified, but\n+# path \"two\" cannot\n+test_expect_success 'simplify combined --raw' '\n+\tside1=$(mkcommit base content1) &&\n+\tside2=$(mkcommit base content2) &&\n+\tmerge=$(mkcommit new new $side1 $side2) &&\n+\tcat >expect <<-\\EOF &&\n+\n+\t:100644 100644 df967b9... 3e75765... M\tone\n+\t::100644 100644 100644 ac3e272... 637f034... 3e75765... MM\ttwo\n+\tEOF\n+\n+\tgit diff-tree -c --simplify-combined-diff \\\n+\t\t--abbrev --pretty=format: $merge >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+# we do not use compare_diff_patch here because we want to\n+# make sure we get the headers right, too\n+test_expect_success 'simplify combined --patch' '\n+\tside1=$(mkcommit base content1) &&\n+\tside2=$(mkcommit base content2) &&\n+\tmerge=$(mkcommit new new $side1 $side2) &&\n+\tcat >expect <<-\\EOF &&\n+\n+\tdiff --combined one\n+\tindex df967b9..3e75765\n+\t--- a/one\n+\t+++ b/one\n+\t@@ -1,1 +1,1 @@\n+\t-base\n+\t+new\n+\tdiff --combined two\n+\tindex ac3e272,637f034..3e75765\n+\t--- a/two\n+\t+++ b/two\n+\t@@@ -1,1 -1,1 +1,1 @@@\n+\t- content1\n+\t -content2\n+\t++new\n+\tEOF\n+\n+\tgit diff-tree -c --simplify-combined-diff -p \\\n+\t\t--pretty=format: $merge >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'do not simplify unless all parents are identical' '\n+\tside1=$(mkcommit base content1) &&\n+\ttest_tick &&\n+\tside2=$(mkcommit base content1) &&\n+\tside3=$(mkcommit base content3) &&\n+\tmerge=$(mkcommit new new $side1 $side2 $side3) &&\n+\tcat >expect <<-\\EOF &&\n+\n+\tdiff --combined one\n+\tindex df967b9..3e75765\n+\t--- a/one\n+\t+++ b/one\n+\t@@ -1,1 +1,1 @@\n+\t-base\n+\t+new\n+\tdiff --combined two\n+\tindex ac3e272,ac3e272,27d10cc..3e75765\n+\t--- a/two\n+\t+++ b/two\n+\t@@@@ -1,1 -1,1 -1,1 +1,1 @@@@\n+\t-- content1\n+\t  -content3\n+\t+++new\n+\tEOF\n+\n+\tgit diff-tree -c --simplify-combined-diff -p \\\n+\t\t--pretty=format: $merge >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'do not simplify away mode changes' '\n+\tside1=$(mkcommit 100644:base 100644:base) &&\n+\tside2=$(mkcommit 100644:base 100755:base) &&\n+\tmerge=$(mkcommit new new $side1 $side2) &&\n+\tcat >expect <<-\\EOF &&\n+\n+\tdiff --combined one\n+\tindex df967b9..3e75765\n+\t--- a/one\n+\t+++ b/one\n+\t@@ -1,1 +1,1 @@\n+\t-base\n+\t+new\n+\tdiff --combined two\n+\tindex df967b9,df967b9..3e75765\n+\tmode 100644,100755..100644\n+\t--- a/two\n+\t+++ b/two\n+\t@@@ -1,1 -1,1 +1,1 @@@\n+\t--base\n+\t++new\n+\tEOF\n+\n+\tgit diff-tree -c --simplify-combined-diff -p \\\n+\t\t--pretty=format: $merge >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.1.0.rc0.286.g5c67d74\n"},{"id":"246898","messageId":"20140729120320.GB9175@peff.net","threadId":"37239","inReplyTo":"20140729115334.GA8976@peff.net","subject":"[PATCH 2/2] stash: default listing to \"--cc --simplify-combined-diff\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-29T12:03:20Z","receivedAt":"2014-07-29T12:03:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When you list stashes, you can provide arbitrary git-log\noptions to change the display. However, adding just \"-p\"\ndoes nothing, because each stash is actually a merge commit.\n\nThis implementation detail is easy to forget, leading to\nconfused users who think \"-p\" is not working. We can make\nthis easier by specifying \"--cc\" as a default ourselves\n(which does nothing if no diff format is requested by the\nuser).\n\nThe resulting diff would then be a combined diff between the\nbase commit and the stashed index state. In many cases,\nthough, the user did not stash anything in the index. We can\nfurther simplify this to a normal pairwise diff by using\n\"--simplify-combined-diff\".\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI don't think anybody should complain about these defaults changing,\ngiven how useless \"git stash list -p\" is now. You can negate\n--simplify-combined-diff if you want to, but I don't think there is a\nway to turn off \"--cc\" (though why would you want to?).\n\nAnyway, I don't think we need to worry too much about flexibility here.\nIf you really want to go wild, you can run \"git log -g refs/stash\"\nyourself.\n\n git-stash.sh     |  3 ++-\n t/t3903-stash.sh | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 65 insertions(+), 1 deletion(-)\n\ndiff --git a/git-stash.sh b/git-stash.sh\nindex bcc757b..a0246d5 100755\n--- a/git-stash.sh\n+++ b/git-stash.sh\n@@ -297,7 +297,8 @@ have_stash () {\n \n list_stash () {\n \thave_stash || return 0\n-\tgit log --format=\"%gd: %gs\" -g \"$@\" $ref_stash --\n+\tgit log --format=\"%gd: %gs\" -g --cc --simplify-combined-diff \\\n+\t\t\"$@\" $ref_stash --\n }\n \n show_stash () {\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex 5b79b21..54154d2 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -685,4 +685,67 @@ test_expect_success 'handle stash specification with spaces' '\n \tgrep pig file\n '\n \n+test_expect_success 'stash list implies --cc' '\n+\tgit stash clear &&\n+\tgit reset --hard &&\n+\techo index >file &&\n+\tgit add file &&\n+\techo working >file &&\n+\tgit stash &&\n+\tgit stash list -p >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tstash@{0}: WIP on master: b27a2bc subdir\n+\n+\tdiff --cc file\n+\tindex 257cc56,9015a7a..d26b33d\n+\t--- a/file\n+\t+++ b/file\n+\t@@@ -1,1 -1,1 +1,1 @@@\n+\t- foo\n+\t -index\n+\t++working\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'stash list implies --simplify-combined-diff' '\n+\tgit stash clear &&\n+\tgit reset --hard &&\n+\techo working >file &&\n+\tgit stash &&\n+\tgit stash list -p >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tstash@{0}: WIP on master: b27a2bc subdir\n+\n+\tdiff --cc file\n+\tindex 257cc56..d26b33d\n+\t--- a/file\n+\t+++ b/file\n+\t@@ -1,1 +1,1 @@\n+\t-foo\n+\t+working\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '--no-simplify-combined-diff overrides default' '\n+\tgit stash clear &&\n+\tgit reset --hard &&\n+\techo working >file &&\n+\tgit stash &&\n+\tgit stash list -p --no-simplify-combined-diff >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tstash@{0}: WIP on master: b27a2bc subdir\n+\n+\tdiff --cc file\n+\tindex 257cc56,257cc56..d26b33d\n+\t--- a/file\n+\t+++ b/file\n+\t@@@ -1,1 -1,1 +1,1 @@@\n+\t--foo\n+\t++working\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.1.0.rc0.286.g5c67d74\n"},{"id":"246899","messageId":"20140729120732.GA9302@peff.net","threadId":"37239","inReplyTo":"20140729115334.GA8976@peff.net","subject":"[RFC/PATCH 3/2] stash: show combined diff with \"stash show\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-29T12:07:33Z","receivedAt":"2014-07-29T12:07:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"A stash may store not only working tree changes, but also\nchanges to the index. However, \"git stash show\" will only\never show the working tree changes. We can instead show both\nas a combined diff, but use \"--simplify-combined-diff\" so\nthat we show a normal pairwise diff in the common case that\nthere were no index changes.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI also considered this on top of the other two, but it probably _isn't_\na good idea. People might be doing things like \"git stash show | git\napply\", and would want to ignore the index content (of course it could\nstill work in the cases where the index was unchanged, and maybe it is a\ngood thing that it would break when there are index changes that the\nuser might be forgetting about).\n\nAs you can see from the changes in the test scripts, though, the\nimplementation isn't quite there either. There's no way to convince \"git\nshow\" to produce _no_ pretty-printed output (the best we can do is an\nextra blank line). And of course the resulting diffs are single-parent\ncombined diffs, not \"real\" pairwise diffs.\n\nWe can fix those, but as I said, I'm not sure it is a good change even\nwithout those warts.\n\n git-stash.sh     |  3 ++-\n t/t3903-stash.sh | 42 ++++++++++++++++++++++++++++++++++--------\n 2 files changed, 36 insertions(+), 9 deletions(-)\n\ndiff --git a/git-stash.sh b/git-stash.sh\nindex a0246d5..9b13853 100755\n--- a/git-stash.sh\n+++ b/git-stash.sh\n@@ -304,7 +304,8 @@ list_stash () {\n show_stash () {\n \tassert_stash_like \"$@\"\n \n-\tgit diff ${FLAGS:---stat} $b_commit $w_commit\n+\tgit show --pretty=format: --simplify-combined-diff \\\n+\t\t${FLAGS:---stat} $w_commit\n }\n \n #\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex 54154d2..045db51 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -460,6 +460,7 @@ test_expect_success 'stash show format defaults to --stat' '\n \tSTASH_ID=$(git stash create) &&\n \tgit reset --hard &&\n \tcat >expected <<-EOF &&\n+\n \t file | 1 +\n \t 1 file changed, 1 insertion(+)\n \tEOF\n@@ -477,7 +478,7 @@ test_expect_success 'stash show - stashes on stack, stash-like argument' '\n \techo bar >> file &&\n \tSTASH_ID=$(git stash create) &&\n \tgit reset --hard &&\n-\techo \"1\t0\tfile\" >expected &&\n+\t{ echo; echo \"1\t0\tfile\"; } >expected &&\n \tgit stash show --numstat ${STASH_ID} >actual &&\n \ttest_cmp expected actual\n '\n@@ -493,11 +494,12 @@ test_expect_success 'stash show -p - stashes on stack, stash-like argument' '\n \tSTASH_ID=$(git stash create) &&\n \tgit reset --hard &&\n \tcat >expected <<-EOF &&\n-\tdiff --git a/file b/file\n-\tindex 7601807..935fbd3 100644\n+\n+\tdiff --cc file\n+\tindex 7601807..935fbd3\n \t--- a/file\n \t+++ b/file\n-\t@@ -1 +1,2 @@\n+\t@@ -1,1 +1,2 @@\n \t baz\n \t+bar\n \tEOF\n@@ -512,7 +514,7 @@ test_expect_success 'stash show - no stashes on stack, stash-like argument' '\n \techo foo >> file &&\n \tSTASH_ID=$(git stash create) &&\n \tgit reset --hard &&\n-\techo \"1\t0\tfile\" >expected &&\n+\t{ echo; echo \"1\t0\tfile\"; } >expected &&\n \tgit stash show --numstat ${STASH_ID} >actual &&\n \ttest_cmp expected actual\n '\n@@ -525,11 +527,12 @@ test_expect_success 'stash show -p - no stashes on stack, stash-like argument' '\n \tSTASH_ID=$(git stash create) &&\n \tgit reset --hard &&\n \tcat >expected <<-EOF &&\n-\tdiff --git a/file b/file\n-\tindex 7601807..71b52c4 100644\n+\n+\tdiff --cc file\n+\tindex 7601807..71b52c4\n \t--- a/file\n \t+++ b/file\n-\t@@ -1 +1,2 @@\n+\t@@ -1,1 +1,2 @@\n \t baz\n \t+foo\n \tEOF\n@@ -537,6 +540,29 @@ test_expect_success 'stash show -p - no stashes on stack, stash-like argument' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'stash show -p will show modified index' '\n+\tgit stash clear &&\n+\ttest_when_finished \"git reset --hard HEAD\" &&\n+\tgit reset --hard &&\n+\techo index >file &&\n+\tgit add file &&\n+\techo working >file &&\n+\tgit stash &&\n+\tcat >expect <<-\\EOF &&\n+\n+\tdiff --cc file\n+\tindex 7601807,9015a7a..d26b33d\n+\t--- a/file\n+\t+++ b/file\n+\t@@@ -1,1 -1,1 +1,1 @@@\n+\t- baz\n+\t -index\n+\t++working\n+\tEOF\n+\tgit stash show -p >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'stash drop - fail early if specified stash is not a stash reference' '\n \tgit stash clear &&\n \ttest_when_finished \"git reset --hard HEAD && git stash clear\" &&\n-- \n2.1.0.rc0.286.g5c67d74\n"},{"id":"246904","messageId":"20140729130026.GA29912@peff.net","threadId":"37239","inReplyTo":"20140729120056.GA9175@peff.net","subject":"Re: [PATCH 1/2] add --simplify-combined-diff option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-29T13:00:27Z","receivedAt":"2014-07-29T13:00:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 29, 2014 at 08:00:56AM -0400, Jeff King wrote:\n\n> The implementation just cuts the number of parents down to 1, but\n> otherwise runs through the same combined-diff code.  The resulting\n> pairwise combined-diff differs from a normal diff in a few ways:\n> \n>   1. The header line is still \"diff --combined\" (or \"--cc\") instead of\n>      \"diff --git\", and it mentions the filename only once.\n> \n>   2. The index lines do not contain the file mode; for combined diffs,\n>      we generate a separate mode line (but only when it has something\n>      interesting to show).\n> \n>   3. The hunk header for a single-line change says \"-1\" in the regular\n>      code path, but \"-1,1\" in the combined code path.\n> \n> Is there any value in keeping this as a pseudo-combined diff (i.e., with\n> the combined header, but only a single parent)? It should not be too\n> hard to just punt to the regular builtin_diff code path when\n> \"num_parents == 1\".\n\nIt's not too hard; mostly we just have to massage the data into a\ndiff_filepair. Patch is below (I'd squash it, and the further commits\nwill need fixups to their tests, too).\n\n---\n combine-diff.c           | 32 +++++++++++++++++++++++++++++---\n diff.c                   |  2 +-\n diff.h                   |  2 ++\n t/t4038-diff-combined.sh | 18 +++++++++---------\n 4 files changed, 41 insertions(+), 13 deletions(-)\n\ndiff --git a/combine-diff.c b/combine-diff.c\nindex 60e54a7..0588c86 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -965,6 +965,28 @@ static int simplify_parents(struct combine_diff_path *p, int nr)\n \treturn 1;\n }\n \n+static void show_single_parent_patch(struct combine_diff_path *elem,\n+\t\t\t\t     int working_tree_file,\n+\t\t\t\t     struct rev_info *rev)\n+{\n+\tstruct diff_filepair pair;\n+\n+\tmemset(&pair, 0, sizeof(pair));\n+\tpair.one = alloc_filespec(elem->path);\n+\tpair.two = alloc_filespec(elem->path);\n+\tpair.status = elem->parent[0].status;\n+\n+\tfill_filespec(pair.one, elem->parent[0].sha1, 1, elem->parent[0].mode);\n+\tif (working_tree_file)\n+\t\tfill_filespec(pair.two, null_sha1, 0, elem->mode);\n+\telse\n+\t\tfill_filespec(pair.two, elem->sha1, 1, elem->mode);\n+\n+\trun_diff(&pair, &rev->diffopt);\n+\tfree_filespec(pair.one);\n+\tfree_filespec(pair.two);\n+}\n+\n static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \t\t\t    int dense, int working_tree_file,\n \t\t\t    struct rev_info *rev)\n@@ -982,6 +1004,13 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \tint is_binary;\n \tconst char *line_prefix = diff_line_prefix(opt);\n \n+\tif (rev->simplify_combined_diff)\n+\t\tnum_parent = simplify_parents(elem, num_parent);\n+\tif (num_parent == 1) {\n+\t\tshow_single_parent_patch(elem, working_tree_file, rev);\n+\t\treturn;\n+\t}\n+\n \tcontext = opt->context;\n \tuserdiff = userdiff_find_by_path(elem->path);\n \tif (!userdiff)\n@@ -989,9 +1018,6 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \tif (DIFF_OPT_TST(opt, ALLOW_TEXTCONV))\n \t\ttextconv = userdiff_get_textconv(userdiff);\n \n-\tif (rev->simplify_combined_diff)\n-\t\tnum_parent = simplify_parents(elem, num_parent);\n-\n \t/* Read the result of merge first */\n \tif (!working_tree_file)\n \t\tresult = grab_blob(elem->sha1, elem->mode, &result_size,\ndiff --git a/diff.c b/diff.c\nindex 867f034..558a520 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3089,7 +3089,7 @@ static void strip_prefix(int prefix_length, const char **namep, const char **oth\n \t}\n }\n \n-static void run_diff(struct diff_filepair *p, struct diff_options *o)\n+void run_diff(struct diff_filepair *p, struct diff_options *o)\n {\n \tconst char *pgm = external_diff();\n \tstruct strbuf msg;\ndiff --git a/diff.h b/diff.h\nindex b4a624d..a669be0 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -356,4 +356,6 @@ extern int print_stat_summary(FILE *fp, int files,\n \t\t\t      int insertions, int deletions);\n extern void setup_diff_pager(struct diff_options *);\n \n+extern void run_diff(struct diff_filepair *p, struct diff_options *o);\n+\n #endif /* DIFF_H */\ndiff --git a/t/t4038-diff-combined.sh b/t/t4038-diff-combined.sh\nindex d522474..29cdb45 100755\n--- a/t/t4038-diff-combined.sh\n+++ b/t/t4038-diff-combined.sh\n@@ -455,11 +455,11 @@ test_expect_success 'simplify combined --patch' '\n \tmerge=$(mkcommit new new $side1 $side2) &&\n \tcat >expect <<-\\EOF &&\n \n-\tdiff --combined one\n-\tindex df967b9..3e75765\n+\tdiff --git a/one b/one\n+\tindex df967b9..3e75765 100644\n \t--- a/one\n \t+++ b/one\n-\t@@ -1,1 +1,1 @@\n+\t@@ -1 +1 @@\n \t-base\n \t+new\n \tdiff --combined two\n@@ -485,11 +485,11 @@ test_expect_success 'do not simplify unless all parents are identical' '\n \tmerge=$(mkcommit new new $side1 $side2 $side3) &&\n \tcat >expect <<-\\EOF &&\n \n-\tdiff --combined one\n-\tindex df967b9..3e75765\n+\tdiff --git a/one b/one\n+\tindex df967b9..3e75765 100644\n \t--- a/one\n \t+++ b/one\n-\t@@ -1,1 +1,1 @@\n+\t@@ -1 +1 @@\n \t-base\n \t+new\n \tdiff --combined two\n@@ -513,11 +513,11 @@ test_expect_success 'do not simplify away mode changes' '\n \tmerge=$(mkcommit new new $side1 $side2) &&\n \tcat >expect <<-\\EOF &&\n \n-\tdiff --combined one\n-\tindex df967b9..3e75765\n+\tdiff --git a/one b/one\n+\tindex df967b9..3e75765 100644\n \t--- a/one\n \t+++ b/one\n-\t@@ -1,1 +1,1 @@\n+\t@@ -1 +1 @@\n \t-base\n \t+new\n \tdiff --combined two\n-- \n2.1.0.rc0.286.g5c67d74\n"},{"id":"246927","messageId":"20140729175300.GA21536@peff.net","threadId":"37239","inReplyTo":"20140729115334.GA8976@peff.net","subject":"[PATCH v2 0/6] improving \"git stash list -p\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-29T17:53:00Z","receivedAt":"2014-07-29T17:53:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I spent a few more minutes cleaning up the rough edges here, and this is\nwhat I ended up with. This version uses run_diff to implement the actual\nsingle-parent diff, so that it matches a \"real\" diff exactly. It also\nintroduces a few new patches to let you really specify an empty --pretty\nuser-format.\n\nI do think this is an improvement over the current state. I'd also be OK\nwith just specifying \"--first-parent\" to \"git stash list\" by default, so\nthat \"git stash list -p\" does something sensible. I don't like that it\ndoesn't let you peek at the index, but nobody has complained about that\nin \"git stash show\", so perhaps nobody cares.  All things being equal, I\nprefer this solution, though.\n\n  [1/6]: revision: drop useless string offset when parsing \"--pretty\"\n  [2/6]: pretty: treat \"--format=\" as an empty userformat\n  [3/6]: pretty: make empty userformats truly empty\n  [4/6]: add --simplify-combined-diff option\n  [5/6]: stash: default listing to \"--cc --simplify-combined-diff\"\n  [6/6]: stash: show combined diff with \"stash show\"\n\n-Peff\n"},{"id":"246928","messageId":"20140729175339.GA31181@peff.net","threadId":"37239","inReplyTo":"20140729175300.GA21536@peff.net","subject":"[PATCH v2 1/6] revision: drop useless string offset when parsing \"--pretty\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-29T17:53:40Z","receivedAt":"2014-07-29T17:53:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Once upon a time, we parsed pretty options by looking for\n\"--pretty\" at the start of the string, and then feeding the\nrest (including an \"=\") to get_commit_format. Later, commit\n48ded91 (log --pretty: do not accept bogus \"--prettyshort\",\n2008-05-25) split this into a separate check for \"--pretty\"\nversus \"--pretty=\".\n\nHowever, when parsing \"--pretty\", we still passed \"arg+8\" to\nget_commit_format. This is useless, since it will always\npoint to the NUL terminator at the end of the string. We can\nsimply pass NULL instead; both parameters are treated the\nsame by get_commit_format.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n revision.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/revision.c b/revision.c\nindex 2571ada..615535c 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1825,7 +1825,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t} else if (!strcmp(arg, \"--pretty\")) {\n \t\trevs->verbose_header = 1;\n \t\trevs->pretty_given = 1;\n-\t\tget_commit_format(arg+8, revs);\n+\t\tget_commit_format(NULL, revs);\n \t} else if (starts_with(arg, \"--pretty=\") || starts_with(arg, \"--format=\")) {\n \t\t/*\n \t\t * Detached form (\"--pretty X\" as opposed to \"--pretty=X\")\n-- \n2.1.0.rc0.286.g5c67d74\n"},{"id":"246929","messageId":"20140729175446.GB31181@peff.net","threadId":"37239","inReplyTo":"20140729175300.GA21536@peff.net","subject":"[PATCH v2 2/6] pretty: treat \"--format=\" as an empty userformat","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-29T17:54:46Z","receivedAt":"2014-07-29T17:54:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Until now, we treated \"--pretty=\" or \"--format=\" as \"give me\nthe default format\". This was not planned nor documented,\nbut only what happened to work due to our parsing of\n\"--pretty\" (which should give the default format).\n\nLet's instead let these be an actual empty userformat.\nOtherwise one must write out the annoyingly long\n\"--pretty=tformat:\" to get the same behavior.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nIf we are really concerned with people expecting \"--pretty=\" to turn on\nthe default format, we could introduce a new mode \"none\" which does the\nsame thing behind the scenes. I don't think it's a problem, though.\n\n pretty.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex 3a1da6f..f97a762 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -146,7 +146,7 @@ void get_commit_format(const char *arg, struct rev_info *rev)\n \tstruct cmt_fmt_map *commit_format;\n \n \trev->use_terminator = 0;\n-\tif (!arg || !*arg) {\n+\tif (!arg) {\n \t\trev->commit_format = CMIT_FMT_DEFAULT;\n \t\treturn;\n \t}\n@@ -155,7 +155,7 @@ void get_commit_format(const char *arg, struct rev_info *rev)\n \t\treturn;\n \t}\n \n-\tif (strchr(arg, '%')) {\n+\tif (!*arg || strchr(arg, '%')) {\n \t\tsave_user_format(rev, arg, 1);\n \t\treturn;\n \t}\n-- \n2.1.0.rc0.286.g5c67d74\n"},{"id":"246930","messageId":"20140729175648.GC31181@peff.net","threadId":"37239","inReplyTo":"20140729175300.GA21536@peff.net","subject":"[PATCH v2 3/6] pretty: make empty userformats truly empty","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-29T17:56:48Z","receivedAt":"2014-07-29T17:56:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If the user provides an empty format with \"--format=\", we\nend up putting in extra whitespace that the user cannot\nprevent. This comes from two places:\n\n  1. If the format is missing a terminating newline, we add\n     one automatically. This makes sense for --format=%h, but\n     not for a truly empty format.\n\n  2. We add an extra newline between the pretty-printed\n     format and a diff or diffstat. If the format is empty,\n     there's no point in doing so if there's nothing to\n     separate.\n\nWith this patch, one can get a diff with no other cruft out\nof \"diff-tree --format= $commit\".\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI assume nobody really cared about including that extra whitespace with\na blank format (I have come across this literally dozens of times in the\npast few years, and everytime wanted the behavior that this patch gives,\nnot the other). But if somebody is depending on it, our alternate route\nwould be to introduce a new CMIT_FMT_NONE. I started on that, but it\nreally does end up hitting all of the same weird special-cases that that\nCMIT_FMT_USERFORMAT does. If we can do it this way, I think the code is\na lot cleaner.\n\n combine-diff.c | 3 ++-\n commit.h       | 1 +\n log-tree.c     | 5 +++--\n pretty.c       | 5 +++++\n 4 files changed, 11 insertions(+), 3 deletions(-)\n\ndiff --git a/combine-diff.c b/combine-diff.c\nindex f9975d2..1a1e659 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -1397,7 +1397,8 @@ void diff_tree_combined(const unsigned char *sha1,\n \t\tshow_log(rev);\n \n \t\tif (rev->verbose_header && opt->output_format &&\n-\t\t    opt->output_format != DIFF_FORMAT_NO_OUTPUT)\n+\t\t    opt->output_format != DIFF_FORMAT_NO_OUTPUT &&\n+\t\t    !commit_format_is_empty(rev->commit_format))\n \t\t\tprintf(\"%s%c\", diff_line_prefix(opt),\n \t\t\t       opt->line_termination);\n \t}\ndiff --git a/commit.h b/commit.h\nindex a8cbf52..aa8c3ca 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -159,6 +159,7 @@ extern void get_commit_format(const char *arg, struct rev_info *);\n extern const char *format_subject(struct strbuf *sb, const char *msg,\n \t\t\t\t  const char *line_separator);\n extern void userformat_find_requirements(const char *fmt, struct userformat_want *w);\n+extern int commit_format_is_empty(enum cmit_fmt);\n extern void format_commit_message(const struct commit *commit,\n \t\t\t\t  const char *format, struct strbuf *sb,\n \t\t\t\t  const struct pretty_print_context *context);\ndiff --git a/log-tree.c b/log-tree.c\nindex 0c53dc1..95e9b1d 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -649,7 +649,7 @@ void show_log(struct rev_info *opt)\n \t\tgraph_show_commit_msg(opt->graph, &msgbuf);\n \telse\n \t\tfwrite(msgbuf.buf, sizeof(char), msgbuf.len, stdout);\n-\tif (opt->use_terminator) {\n+\tif (opt->use_terminator && !commit_format_is_empty(opt->commit_format)) {\n \t\tif (!opt->missing_newline)\n \t\t\tgraph_show_padding(opt->graph);\n \t\tputchar(opt->diffopt.line_termination);\n@@ -676,7 +676,8 @@ int log_tree_diff_flush(struct rev_info *opt)\n \t\tshow_log(opt);\n \t\tif ((opt->diffopt.output_format & ~DIFF_FORMAT_NO_OUTPUT) &&\n \t\t    opt->verbose_header &&\n-\t\t    opt->commit_format != CMIT_FMT_ONELINE) {\n+\t\t    opt->commit_format != CMIT_FMT_ONELINE &&\n+\t\t    !commit_format_is_empty(opt->commit_format)) {\n \t\t\t/*\n \t\t\t * When showing a verbose header (i.e. log message),\n \t\t\t * and not in --pretty=oneline format, we would want\ndiff --git a/pretty.c b/pretty.c\nindex f97a762..31fc76b 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -24,6 +24,11 @@ static size_t commit_formats_len;\n static size_t commit_formats_alloc;\n static struct cmt_fmt_map *find_commit_format(const char *sought);\n \n+int commit_format_is_empty(enum cmit_fmt fmt)\n+{\n+\treturn fmt == CMIT_FMT_USERFORMAT && !*user_format;\n+}\n+\n static void save_user_format(struct rev_info *rev, const char *cp, int is_tformat)\n {\n \tfree(user_format);\n-- \n2.1.0.rc0.286.g5c67d74\n"},{"id":"246931","messageId":"20140729175712.GD31181@peff.net","threadId":"37239","inReplyTo":"20140729175300.GA21536@peff.net","subject":"[PATCH v2 4/6] add --simplify-combined-diff option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-29T17:57:12Z","receivedAt":"2014-07-29T17:57:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When showing a combined diff, we are comparing two (or more)\nparents to a final state, and some of these states may be\nthe same.  Here's a table of the possible contents for a\ngiven path (for two parents, but we will generalize to more\nin a moment; we also omit isomorphic cases where the parents\nare swapped):\n\n  case | M | P1 | P2\n  -------------------\n  1    | A | A  | A\n  2    | A | A  | B\n  3    | A | B  | B\n  4    | A | B  | C\n\nIn case 1, the path was not relevant to the merge at all,\nand we omit it as uninteresting. In case 2, we did resolve\nthe path, but in favor of one side. We also consider this\nuninteresting and do not show the diff.\n\nIn case 4, we had a real content-level merge, and the\ncombined diff is interesting. We show it.\n\nThat leaves case 3, in which both parents are the same, but\nthe merge picks a new content. This should be rare in\nnormal merges, though it could happen if you updated an\nunrelated file due to a resolution elsewhere (i.e., an evil\nmerge that crosses a file boundary). But it happens\nfrequently in the fake merge commits we create for stashes,\nin which one parent is the base of the stash and the other\nis the index (in which case it simply means that the index\nentry for the path was not touched).\n\nRight now, we treat it the same as case 4, and show a normal\ncombined diff. However, the result is harder to read, and\nthe combined nature of the diff gives no extra information;\nevery marker in the combined diff will be identical for both\nparents.\n\nThis patch adds a new option, \"--simplify-combined-diff\",\nwhich converts this case into a normal, non-combined diff.\nIt would not be correct to simply omit it, because there\nreally is an interesting change going from B..A. It's just\nthat there are not two interesting changes, which the\ncombined diff would show.\n\nWhen generalizing this to more than two parents, we have two\noptions:\n\n  1. Either simply to a single parent content, or not at all.\n\n  2. Omit parents whose contents are duplicates of other\n     parents.\n\nFor a case like \"A B B C\", option (2) would still result in\na combined diff, but one with fewer sources. However, it\nwould also be ambiguous. The parents in a combined diff are\nmarked only by their position, so omitting a position means\nthat a reader can no longer tell which line goes with which\nparent.\n\nInstead, we choose option (1). Either you get the full\ncombined diff, or you get a normal non-combined diff.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n Documentation/rev-list-options.txt |   8 +++\n combine-diff.c                     |  56 ++++++++++++++--\n diff.c                             |   2 +-\n diff.h                             |   2 +\n revision.c                         |   4 ++\n revision.h                         |   1 +\n t/t4038-diff-combined.sh           | 133 +++++++++++++++++++++++++++++++++++++\n 7 files changed, 201 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex deb8cca..7331bcc 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -805,6 +805,14 @@ options may be given. See linkgit:git-diff-files[1] for more options.\n \tin that case, the output represents the changes the merge\n \tbrought _into_ the then-current branch.\n \n+--simplify-combined-diff::\n+\tWhen showing a combined diff with `-c` or `--cc`, if a given\n+\tpath has identical content in all parents, show only a pairwise\n+\tdiff from that content, rather than a true combined diff. This\n+\tis easier for humans to read, though it may confuse an automatic\n+\tparser (as some paths in the output may be combined diffs, and\n+\tothers may not).\n+\n -r::\n \tShow recursive diffs.\n \ndiff --git a/combine-diff.c b/combine-diff.c\nindex 1a1e659..ac4bc5a 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -955,6 +955,38 @@ static void show_combined_header(struct combine_diff_path *elem,\n \t\t\t\t line_prefix, c_meta, c_reset);\n }\n \n+static int simplify_parents(struct combine_diff_path *p, int nr)\n+{\n+\tint i;\n+\tfor (i = 1; i < nr; i++)\n+\t\tif (p->parent[i].mode != p->parent[i-1].mode ||\n+\t\t    hashcmp(p->parent[i].sha1, p->parent[i-1].sha1))\n+\t\t\treturn nr;\n+\treturn 1;\n+}\n+\n+static void show_single_parent_patch(struct combine_diff_path *elem,\n+\t\t\t\t     int working_tree_file,\n+\t\t\t\t     struct rev_info *rev)\n+{\n+\tstruct diff_filepair pair;\n+\n+\tmemset(&pair, 0, sizeof(pair));\n+\tpair.one = alloc_filespec(elem->path);\n+\tpair.two = alloc_filespec(elem->path);\n+\tpair.status = elem->parent[0].status;\n+\n+\tfill_filespec(pair.one, elem->parent[0].sha1, 1, elem->parent[0].mode);\n+\tif (working_tree_file)\n+\t\tfill_filespec(pair.two, null_sha1, 0, elem->mode);\n+\telse\n+\t\tfill_filespec(pair.two, elem->sha1, 1, elem->mode);\n+\n+\trun_diff(&pair, &rev->diffopt);\n+\tfree_filespec(pair.one);\n+\tfree_filespec(pair.two);\n+}\n+\n static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \t\t\t    int dense, int working_tree_file,\n \t\t\t    struct rev_info *rev)\n@@ -972,6 +1004,13 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \tint is_binary;\n \tconst char *line_prefix = diff_line_prefix(opt);\n \n+\tif (rev->simplify_combined_diff)\n+\t\tnum_parent = simplify_parents(elem, num_parent);\n+\tif (num_parent == 1) {\n+\t\tshow_single_parent_patch(elem, working_tree_file, rev);\n+\t\treturn;\n+\t}\n+\n \tcontext = opt->context;\n \tuserdiff = userdiff_find_by_path(elem->path);\n \tif (!userdiff)\n@@ -1181,6 +1220,8 @@ static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct re\n \tif (rev->loginfo && !rev->no_commit_id)\n \t\tshow_log(rev);\n \n+\tif (rev->simplify_combined_diff)\n+\t\tnum_parent = simplify_parents(p, num_parent);\n \n \tif (opt->output_format & DIFF_FORMAT_RAW) {\n \t\tprintf(\"%s\", line_prefix);\n@@ -1247,12 +1288,16 @@ static void free_combined_pair(struct diff_filepair *pair)\n  * but currently nobody uses it, so this should suffice for now.\n  */\n static struct diff_filepair *combined_pair(struct combine_diff_path *p,\n-\t\t\t\t\t   int num_parent)\n+\t\t\t\t\t   int num_parent,\n+\t\t\t\t\t   struct rev_info *rev)\n {\n \tint i;\n \tstruct diff_filepair *pair;\n \tstruct diff_filespec *pool;\n \n+\tif (rev->simplify_combined_diff)\n+\t\tnum_parent = simplify_parents(p, num_parent);\n+\n \tpair = xmalloc(sizeof(*pair));\n \tpool = xcalloc(num_parent + 1, sizeof(struct diff_filespec));\n \tpair->one = pool + 1;\n@@ -1277,7 +1322,8 @@ static struct diff_filepair *combined_pair(struct combine_diff_path *p,\n static void handle_combined_callback(struct diff_options *opt,\n \t\t\t\t     struct combine_diff_path *paths,\n \t\t\t\t     int num_parent,\n-\t\t\t\t     int num_paths)\n+\t\t\t\t     int num_paths,\n+\t\t\t\t     struct rev_info *rev)\n {\n \tstruct combine_diff_path *p;\n \tstruct diff_queue_struct q;\n@@ -1287,7 +1333,8 @@ static void handle_combined_callback(struct diff_options *opt,\n \tq.alloc = num_paths;\n \tq.nr = num_paths;\n \tfor (i = 0, p = paths; p; p = p->next)\n-\t\tq.queue[i++] = combined_pair(p, num_parent);\n+\t\tq.queue[i++] = combined_pair(p, num_parent, rev);\n+\n \topt->format_callback(&q, opt, opt->format_callback_data);\n \tfor (i = 0; i < num_paths; i++)\n \t\tfree_combined_pair(q.queue[i]);\n@@ -1499,7 +1546,8 @@ void diff_tree_combined(const unsigned char *sha1,\n \t\t\t (DIFF_FORMAT_NUMSTAT|DIFF_FORMAT_DIFFSTAT))\n \t\t\tneedsep = 1;\n \t\telse if (opt->output_format & DIFF_FORMAT_CALLBACK)\n-\t\t\thandle_combined_callback(opt, paths, num_parent, num_paths);\n+\t\t\thandle_combined_callback(opt, paths, num_parent,\n+\t\t\t\t\t\t num_paths, rev);\n \n \t\tif (opt->output_format & DIFF_FORMAT_PATCH) {\n \t\t\tif (needsep)\ndiff --git a/diff.c b/diff.c\nindex 867f034..558a520 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3089,7 +3089,7 @@ static void strip_prefix(int prefix_length, const char **namep, const char **oth\n \t}\n }\n \n-static void run_diff(struct diff_filepair *p, struct diff_options *o)\n+void run_diff(struct diff_filepair *p, struct diff_options *o)\n {\n \tconst char *pgm = external_diff();\n \tstruct strbuf msg;\ndiff --git a/diff.h b/diff.h\nindex b4a624d..a669be0 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -356,4 +356,6 @@ extern int print_stat_summary(FILE *fp, int files,\n \t\t\t      int insertions, int deletions);\n extern void setup_diff_pager(struct diff_options *);\n \n+extern void run_diff(struct diff_filepair *p, struct diff_options *o);\n+\n #endif /* DIFF_H */\ndiff --git a/revision.c b/revision.c\nindex 615535c..0b8516b 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1820,6 +1820,10 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\trevs->diff = 1;\n \t\trevs->dense_combined_merges = 1;\n \t\trevs->combine_merges = 1;\n+\t} else if (!strcmp(arg, \"--simplify-combined-diff\")) {\n+\t\trevs->simplify_combined_diff = 1;\n+\t} else if (!strcmp(arg, \"--no-simplify-combined-diff\")) {\n+\t\trevs->simplify_combined_diff = 0;\n \t} else if (!strcmp(arg, \"-v\")) {\n \t\trevs->verbose_header = 1;\n \t} else if (!strcmp(arg, \"--pretty\")) {\ndiff --git a/revision.h b/revision.h\nindex a620530..48cb16e 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -122,6 +122,7 @@ struct rev_info {\n \t\t\tignore_merges:1,\n \t\t\tcombine_merges:1,\n \t\t\tdense_combined_merges:1,\n+\t\t\tsimplify_combined_diff:1,\n \t\t\talways_show_header:1;\n \n \t/* Format info */\ndiff --git a/t/t4038-diff-combined.sh b/t/t4038-diff-combined.sh\nindex 41913c3..5e141ad 100755\n--- a/t/t4038-diff-combined.sh\n+++ b/t/t4038-diff-combined.sh\n@@ -401,4 +401,137 @@ test_expect_success 'combine diff missing delete bug' '\n \tcompare_diff_patch expected actual\n '\n \n+# make a commit with contents \"$1\" and \"$2\" in the paths\n+# \"one\" and \"two\" respectively, and parents $3, $4, etc\n+#\n+# If the content is of the form \"mode:content\", then the\n+# mode for the given file is set (otherwise it defaults 100644).\n+mkcommit() {\n+\t(\n+\t\tGIT_INDEX_FILE=tmp.index &&\n+\t\texport GIT_INDEX_FILE &&\n+\t\tfor path in one two; do\n+\t\t\tcase \"$1\" in\n+\t\t\t*:*)\n+\t\t\t\tmode=$(echo \"$1\" | cut -d: -f1)\n+\t\t\t\tcontent=$(echo \"$1\" | cut -d: -f2)\n+\t\t\t\t;;\n+\t\t\t*)\n+\t\t\t\tmode=100644\n+\t\t\t\tcontent=$1\n+\t\t\tesac\n+\t\t\tblob=$(echo \"$content\" | git hash-object -w --stdin) &&\n+\t\t\tgit update-index --add --cacheinfo $mode,$blob,$path &&\n+\t\t\tshift || exit 1\n+\t\tdone\n+\t\ttree=$(git write-tree) &&\n+\t\tparents=$(for p in \"$@\"; do echo \"-p $p\"; done) &&\n+\t\tgit commit-tree $tree $parents </dev/null\n+\t)\n+}\n+\n+# we create a merge commit where path \"one\" can be simplified, but\n+# path \"two\" cannot\n+test_expect_success 'simplify combined --raw' '\n+\tside1=$(mkcommit base content1) &&\n+\tside2=$(mkcommit base content2) &&\n+\tmerge=$(mkcommit new new $side1 $side2) &&\n+\tcat >expect <<-\\EOF &&\n+\t:100644 100644 df967b9... 3e75765... M\tone\n+\t::100644 100644 100644 ac3e272... 637f034... 3e75765... MM\ttwo\n+\tEOF\n+\n+\tgit diff-tree -c --simplify-combined-diff \\\n+\t\t--abbrev --format= $merge >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+# we do not use compare_diff_patch here because we want to\n+# make sure we get the headers right, too\n+test_expect_success 'simplify combined --patch' '\n+\tside1=$(mkcommit base content1) &&\n+\tside2=$(mkcommit base content2) &&\n+\tmerge=$(mkcommit new new $side1 $side2) &&\n+\n+\tcat >expect <<-\\EOF &&\n+\tdiff --git a/one b/one\n+\tindex df967b9..3e75765 100644\n+\t--- a/one\n+\t+++ b/one\n+\t@@ -1 +1 @@\n+\t-base\n+\t+new\n+\tdiff --combined two\n+\tindex ac3e272,637f034..3e75765\n+\t--- a/two\n+\t+++ b/two\n+\t@@@ -1,1 -1,1 +1,1 @@@\n+\t- content1\n+\t -content2\n+\t++new\n+\tEOF\n+\n+\tgit diff-tree -c --simplify-combined-diff -p \\\n+\t\t--format= $merge >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'do not simplify unless all parents are identical' '\n+\tside1=$(mkcommit base content1) &&\n+\ttest_tick &&\n+\tside2=$(mkcommit base content1) &&\n+\tside3=$(mkcommit base content3) &&\n+\tmerge=$(mkcommit new new $side1 $side2 $side3) &&\n+\n+\tcat >expect <<-\\EOF &&\n+\tdiff --git a/one b/one\n+\tindex df967b9..3e75765 100644\n+\t--- a/one\n+\t+++ b/one\n+\t@@ -1 +1 @@\n+\t-base\n+\t+new\n+\tdiff --combined two\n+\tindex ac3e272,ac3e272,27d10cc..3e75765\n+\t--- a/two\n+\t+++ b/two\n+\t@@@@ -1,1 -1,1 -1,1 +1,1 @@@@\n+\t-- content1\n+\t  -content3\n+\t+++new\n+\tEOF\n+\n+\tgit diff-tree -c --simplify-combined-diff -p \\\n+\t\t--format= $merge >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'do not simplify away mode changes' '\n+\tside1=$(mkcommit 100644:base 100644:base) &&\n+\tside2=$(mkcommit 100644:base 100755:base) &&\n+\tmerge=$(mkcommit new new $side1 $side2) &&\n+\n+\tcat >expect <<-\\EOF &&\n+\tdiff --git a/one b/one\n+\tindex df967b9..3e75765 100644\n+\t--- a/one\n+\t+++ b/one\n+\t@@ -1 +1 @@\n+\t-base\n+\t+new\n+\tdiff --combined two\n+\tindex df967b9,df967b9..3e75765\n+\tmode 100644,100755..100644\n+\t--- a/two\n+\t+++ b/two\n+\t@@@ -1,1 -1,1 +1,1 @@@\n+\t--base\n+\t++new\n+\tEOF\n+\n+\tgit diff-tree -c --simplify-combined-diff -p \\\n+\t\t--format= $merge >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.1.0.rc0.286.g5c67d74\n"},{"id":"246932","messageId":"20140729175725.GE31181@peff.net","threadId":"37239","inReplyTo":"20140729175300.GA21536@peff.net","subject":"[PATCH v2 5/6] stash: default listing to \"--cc --simplify-combined-diff\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-29T17:57:25Z","receivedAt":"2014-07-29T17:57:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When you list stashes, you can provide arbitrary git-log\noptions to change the display. However, adding just \"-p\"\ndoes nothing, because each stash is actually a merge commit.\n\nThis implementation detail is easy to forget, leading to\nconfused users who think \"-p\" is not working. We can make\nthis easier by specifying \"--cc\" as a default ourselves\n(which does nothing if no diff format is requested by the\nuser).\n\nThe resulting diff would then be a combined diff between the\nbase commit and the stashed index state. In many cases,\nthough, the user did not stash anything in the index. We can\nfurther simplify this to a normal pairwise diff by using\n\"--simplify-combined-diff\".\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n git-stash.sh     |  3 ++-\n t/t3903-stash.sh | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 65 insertions(+), 1 deletion(-)\n\ndiff --git a/git-stash.sh b/git-stash.sh\nindex bcc757b..a0246d5 100755\n--- a/git-stash.sh\n+++ b/git-stash.sh\n@@ -297,7 +297,8 @@ have_stash () {\n \n list_stash () {\n \thave_stash || return 0\n-\tgit log --format=\"%gd: %gs\" -g \"$@\" $ref_stash --\n+\tgit log --format=\"%gd: %gs\" -g --cc --simplify-combined-diff \\\n+\t\t\"$@\" $ref_stash --\n }\n \n show_stash () {\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex 5b79b21..465f824 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -685,4 +685,67 @@ test_expect_success 'handle stash specification with spaces' '\n \tgrep pig file\n '\n \n+test_expect_success 'stash list implies --cc' '\n+\tgit stash clear &&\n+\tgit reset --hard &&\n+\techo index >file &&\n+\tgit add file &&\n+\techo working >file &&\n+\tgit stash &&\n+\tgit stash list -p >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tstash@{0}: WIP on master: b27a2bc subdir\n+\n+\tdiff --cc file\n+\tindex 257cc56,9015a7a..d26b33d\n+\t--- a/file\n+\t+++ b/file\n+\t@@@ -1,1 -1,1 +1,1 @@@\n+\t- foo\n+\t -index\n+\t++working\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'stash list implies --simplify-combined-diff' '\n+\tgit stash clear &&\n+\tgit reset --hard &&\n+\techo working >file &&\n+\tgit stash &&\n+\tgit stash list -p >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tstash@{0}: WIP on master: b27a2bc subdir\n+\n+\tdiff --git a/file b/file\n+\tindex 257cc56..d26b33d 100644\n+\t--- a/file\n+\t+++ b/file\n+\t@@ -1 +1 @@\n+\t-foo\n+\t+working\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '--no-simplify-combined-diff overrides default' '\n+\tgit stash clear &&\n+\tgit reset --hard &&\n+\techo working >file &&\n+\tgit stash &&\n+\tgit stash list -p --no-simplify-combined-diff >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tstash@{0}: WIP on master: b27a2bc subdir\n+\n+\tdiff --cc file\n+\tindex 257cc56,257cc56..d26b33d\n+\t--- a/file\n+\t+++ b/file\n+\t@@@ -1,1 -1,1 +1,1 @@@\n+\t--foo\n+\t++working\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.1.0.rc0.286.g5c67d74\n"},{"id":"246933","messageId":"20140729175820.GF31181@peff.net","threadId":"37239","inReplyTo":"20140729175300.GA21536@peff.net","subject":"[PATCH v2 6/6] stash: show combined diff with \"stash show\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-29T17:58:20Z","receivedAt":"2014-07-29T17:58:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"A stash may store not only working tree changes, but also\nchanges to the index. However, \"git stash show\" will only\never show the working tree changes. We can instead show both\nas a combined diff, but use \"--simplify-combined-diff\" so\nthat we show a normal pairwise diff in the common case that\nthere were no index changes.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI'm still a little iffy on this one in case somebody really doesn't want\nto see the saved index state. But with the earlier cleanups, you can at\nleast see that it doesn't even impact any existing tests.\n\n git-stash.sh     |  3 ++-\n t/t3903-stash.sh | 22 ++++++++++++++++++++++\n 2 files changed, 24 insertions(+), 1 deletion(-)\n\ndiff --git a/git-stash.sh b/git-stash.sh\nindex a0246d5..ae73ba4 100755\n--- a/git-stash.sh\n+++ b/git-stash.sh\n@@ -304,7 +304,8 @@ list_stash () {\n show_stash () {\n \tassert_stash_like \"$@\"\n \n-\tgit diff ${FLAGS:---stat} $b_commit $w_commit\n+\tgit show --format= --simplify-combined-diff \\\n+\t\t${FLAGS:---stat} $w_commit\n }\n \n #\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex 465f824..36b97a4 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -537,6 +537,28 @@ test_expect_success 'stash show -p - no stashes on stack, stash-like argument' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'stash show -p will show modified index' '\n+\tgit stash clear &&\n+\ttest_when_finished \"git reset --hard HEAD\" &&\n+\tgit reset --hard &&\n+\techo index >file &&\n+\tgit add file &&\n+\techo working >file &&\n+\tgit stash &&\n+\tcat >expect <<-\\EOF &&\n+\tdiff --cc file\n+\tindex 7601807,9015a7a..d26b33d\n+\t--- a/file\n+\t+++ b/file\n+\t@@@ -1,1 -1,1 +1,1 @@@\n+\t- baz\n+\t -index\n+\t++working\n+\tEOF\n+\tgit stash show -p >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'stash drop - fail early if specified stash is not a stash reference' '\n \tgit stash clear &&\n \ttest_when_finished \"git reset --hard HEAD && git stash clear\" &&\n-- \n2.1.0.rc0.286.g5c67d74\n"},{"id":"246934","messageId":"xmqq1tt43uem.fsf@gitster.dls.corp.google.com","threadId":"37239","inReplyTo":"20140729120732.GA9302@peff.net","subject":"Re: [RFC/PATCH 3/2] stash: show combined diff with \"stash show\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-29T18:13:37Z","receivedAt":"2014-07-29T18:13:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> ... People might be doing things like \"git stash show | git\n> apply\", and would want to ignore the index content ...\n\nFWIW, that is exactly how I use \"git stash show -p\" most of the time.\n"},{"id":"246936","messageId":"xmqqppgo2dqr.fsf@gitster.dls.corp.google.com","threadId":"37239","inReplyTo":"20140729175725.GE31181@peff.net","subject":"Re: [PATCH v2 5/6] stash: default listing to \"--cc --simplify-combined-diff\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-29T18:58:52Z","receivedAt":"2014-07-29T18:58:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> When you list stashes, you can provide arbitrary git-log\n> options to change the display. However, adding just \"-p\"\n> does nothing, because each stash is actually a merge commit.\n>\n> This implementation detail is easy to forget, leading to\n> confused users who think \"-p\" is not working. We can make\n> this easier by specifying \"--cc\" as a default ourselves\n> (which does nothing if no diff format is requested by the\n> user).\n\nSigh.\n\n\"git log --cc\" is one of the things I wanted for a long time to fix.\nWhen the user explicitly asks \"--cc\", we currently ignore it, but\nbecause we know the user wants to view combined diff, we should turn\n\"-p\" on automatically.  And the change this patch introduces will be\nbroken when we fix \"log --cc\" (\"stash list\" will end up always\nshowing the patch, without a way to disable it).\n\nCan you make this conditional?  Do this only when <options> are\ngiven to \"git stash list\" command and that includes \"-p\" or\nsomething?\n"},{"id":"246942","messageId":"xmqqd2co2asg.fsf@gitster.dls.corp.google.com","threadId":"37239","inReplyTo":"20140729175712.GD31181@peff.net","subject":"Re: [PATCH v2 4/6] add --simplify-combined-diff option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-29T20:02:39Z","receivedAt":"2014-07-29T20:02:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> When showing a combined diff, we are comparing two (or more)\n> parents to a final state, and some of these states may be\n> the same.  Here's a table of the possible contents for a\n> given path (for two parents, but we will generalize to more\n> in a moment; we also omit isomorphic cases where the parents\n> are swapped):\n>\n>   case | M | P1 | P2\n>   -------------------\n>   1    | A | A  | A\n>   2    | A | A  | B\n>   3    | A | B  | B\n>   4    | A | B  | C\n>\n> In case 1, the path was not relevant to the merge at all,\n> and we omit it as uninteresting. In case 2, we did resolve\n> the path, but in favor of one side. We also consider this\n> uninteresting and do not show the diff.\n>\n> In case 4, we had a real content-level merge, and the\n> combined diff is interesting. We show it.\n>\n> That leaves case 3, in which both parents are the same, but\n> the merge picks a new content. This should be rare in\n> normal merges, though it could happen if you updated an\n> unrelated file due to a resolution elsewhere (i.e., an evil\n> merge that crosses a file boundary). But it happens\n> frequently in the fake merge commits we create for stashes,\n> in which one parent is the base of the stash and the other\n> is the index (in which case it simply means that the index\n> entry for the path was not touched).\n>\n> Right now, we treat it the same as case 4, and show a normal\n> combined diff. However, the result is harder to read, and\n> the combined nature of the diff gives no extra information;\n> every marker in the combined diff will be identical for both\n> parents.\n>\n> This patch adds a new option, \"--simplify-combined-diff\",\n> which converts this case into a normal, non-combined diff.\n> It would not be correct to simply omit it, because there\n> really is an interesting change going from B..A. It's just\n> that there are not two interesting changes, which the\n> combined diff would show.\n>\n> When generalizing this to more than two parents, we have two\n> options:\n>\n>   1. Either simply to a single parent content, or not at all.\n>\n>   2. Omit parents whose contents are duplicates of other\n>      parents.\n>\n> For a case like \"A B B C\", option (2) would still result in\n> a combined diff, but one with fewer sources. However, it\n> would also be ambiguous. The parents in a combined diff are\n> marked only by their position, so omitting a position means\n> that a reader can no longer tell which line goes with which\n> parent.\n>\n> Instead, we choose option (1). Either you get the full\n> combined diff, or you get a normal non-combined diff.\n\nVery nicely analyzed.  The changes to the code looked also good from\na cursory read.\n\nThanks.\n"},{"id":"246989","messageId":"xmqqtx5yy6nm.fsf@gitster.dls.corp.google.com","threadId":"37239","inReplyTo":"xmqqppgo2dqr.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 5/6] stash: default listing to \"--cc --simplify-combined-diff\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-30T19:43:09Z","receivedAt":"2014-07-30T19:43:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Jeff King <peff@peff.net> writes:\n>\n>> When you list stashes, you can provide arbitrary git-log\n>> options to change the display. However, adding just \"-p\"\n>> does nothing, because each stash is actually a merge commit.\n>>\n>> This implementation detail is easy to forget, leading to\n>> confused users who think \"-p\" is not working. We can make\n>> this easier by specifying \"--cc\" as a default ourselves\n>> (which does nothing if no diff format is requested by the\n>> user).\n>\n> Sigh.\n>\n> \"git log --cc\" is one of the things I wanted for a long time to fix.\n> When the user explicitly asks \"--cc\", we currently ignore it, but\n> because we know the user wants to view combined diff, we should turn\n> \"-p\" on automatically.  And the change this patch introduces will be\n> broken when we fix \"log --cc\" (\"stash list\" will end up always\n> showing the patch, without a way to disable it).\n>\n> Can you make this conditional?  Do this only when <options> are\n> given to \"git stash list\" command and that includes \"-p\" or\n> something?\n\nPerhaps something along this line?\n\n git-stash.sh | 11 +++++++++--\n 1 file changed, 9 insertions(+), 2 deletions(-)\n\ndiff --git a/git-stash.sh b/git-stash.sh\nindex ae73ba4..0db1b19 100755\n--- a/git-stash.sh\n+++ b/git-stash.sh\n@@ -297,8 +297,15 @@ have_stash () {\n \n list_stash () {\n \thave_stash || return 0\n-\tgit log --format=\"%gd: %gs\" -g --cc --simplify-combined-diff \\\n-\t\t\"$@\" $ref_stash --\n+\tcase \" $* \" in\n+\t*' --cc '*)\n+\t\t;; # the user knows what she is doing\n+\t*' -p '* | *' -U'*)\n+\t\tset x \"--cc\" \"--simplify-combined-diff\" \"$@\"\n+\t\tshift\n+\t\t;;\n+\tesac\n+\tgit log --format=\"%gd: %gs\" -g \"$@\" $ref_stash --\n }\n \n show_stash () {\n"},{"id":"247006","messageId":"20140731000907.GA22297@peff.net","threadId":"37239","inReplyTo":"xmqqtx5yy6nm.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 5/6] stash: default listing to \"--cc --simplify-combined-diff\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-31T00:09:07Z","receivedAt":"2014-07-31T00:09:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 30, 2014 at 12:43:09PM -0700, Junio C Hamano wrote:\n\n> > \"git log --cc\" is one of the things I wanted for a long time to fix.\n> > When the user explicitly asks \"--cc\", we currently ignore it, but\n> > because we know the user wants to view combined diff, we should turn\n> > \"-p\" on automatically.  And the change this patch introduces will be\n> > broken when we fix \"log --cc\" (\"stash list\" will end up always\n> > showing the patch, without a way to disable it).\n> >\n> > Can you make this conditional?  Do this only when <options> are\n> > given to \"git stash list\" command and that includes \"-p\" or\n> > something?\n\nI'm definitely sympathetic. Using \"--cc\" with the diff family _does_\nimply \"-p\" already, so it would be nice to do the same for the log\nfamily.\n\n> Perhaps something along this line?\n> \n>  git-stash.sh | 11 +++++++++--\n>  1 file changed, 9 insertions(+), 2 deletions(-)\n> \n> diff --git a/git-stash.sh b/git-stash.sh\n> index ae73ba4..0db1b19 100755\n> --- a/git-stash.sh\n> +++ b/git-stash.sh\n> @@ -297,8 +297,15 @@ have_stash () {\n>  \n>  list_stash () {\n>  \thave_stash || return 0\n> -\tgit log --format=\"%gd: %gs\" -g --cc --simplify-combined-diff \\\n> -\t\t\"$@\" $ref_stash --\n> +\tcase \" $* \" in\n> +\t*' --cc '*)\n> +\t\t;; # the user knows what she is doing\n> +\t*' -p '* | *' -U'*)\n> +\t\tset x \"--cc\" \"--simplify-combined-diff\" \"$@\"\n> +\t\tshift\n> +\t\t;;\n> +\tesac\n> +\tgit log --format=\"%gd: %gs\" -g \"$@\" $ref_stash --\n\nUgh. I'd generally like to avoid this sort of ad-hoc command line\nparsing, as it is easy to get confused by option arguments or\neven non-options. At least we do not have to worry about pathspecs here,\nas we already do an explicit \"--\" (so we might be confused by them, but\nthey are broken anyway).\n\nStill, I wonder if a cleaner solution is to provide alternate \"default\nto this\" options for the revision.c option parser. We already have\n\"--default\" to pick the default starting point. Could we do something\nlike \"--default-merge-handling=cc\" or something?\n\nThere's a similar ad-hoc parsing case in \"stash show\", where we add\n\"--stat\" if no arguments are given, but we have no clue if a diff format\nwas given (so \"git stash show --color\" accidentally turns on patch\nformat!). Something like \"--default-diff-format=stat\" would be more\nrobust.\n\nI dunno. Maybe it is over-engineering. I just hate to see solutions that\nwork most of the time and crumble in weird corner cases, even if people\ndon't hit those corner cases very often.\n\n-Peff\n"},{"id":"247008","messageId":"20140731001746.GC22297@peff.net","threadId":"37239","inReplyTo":"xmqq1tt43uem.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC/PATCH 3/2] stash: show combined diff with \"stash show\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-31T00:17:46Z","receivedAt":"2014-07-31T00:17:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 29, 2014 at 11:13:37AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > ... People might be doing things like \"git stash show | git\n> > apply\", and would want to ignore the index content ...\n> \n> FWIW, that is exactly how I use \"git stash show -p\" most of the time.\n\nLike I said, I'm iffy on this part of the series for that reason. But\nI'm curious: what do you think should happen in such a use case when\nthere are staged contents in the index? Right now we completely ignore\nthem.\n\n-Peff\n"},{"id":"247010","messageId":"CAPc5daWuyELptgGgBUxD_etTM3H40tneKXTrtFM3NLaneiibBA@mail.gmail.com","threadId":"37239","inReplyTo":"20140731001746.GC22297@peff.net","subject":"Re: [RFC/PATCH 3/2] stash: show combined diff with \"stash show\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-31T00:28:18Z","receivedAt":"2014-07-31T00:28:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Wed, Jul 30, 2014 at 5:17 PM, Jeff King <peff@peff.net> wrote:\n>\n> Like I said, I'm iffy on this part of the series for that reason. But\n> I'm curious: what do you think should happen in such a use case when\n> there are staged contents in the index? Right now we completely ignore\n> them.\n\nI think ignoring is absolutely the right thing ;-)\n\nUnlike \"stash pop\", which is \"try to bring me back to exactly the same state\",\nit is a strong indication that the user wants to further tweak the previous work\nto grab a patch and apply to the working tree---and while you are working\ninside the working tree, you haven't started adding the index contents, so\n\"git diff\" after such an operation should show what you grabbed out of the\nstash, applied to the working tree, relative to what you had in the index.\n"},{"id":"247633","messageId":"1407886257.22389.13.camel@jekeller-desk1.amr.corp.intel.com","threadId":"37239","inReplyTo":"20140731000907.GA22297@peff.net","subject":"Re: [PATCH v2 5/6] stash: default listing to \"--cc --simplify-combined-diff\"","fromName":"Keller, Jacob E","fromEmail":"jacob.e.keller@intel.com","sentAt":"2014-08-12T23:30:56Z","receivedAt":"2014-08-12T23:30:56Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Wed, 2014-07-30 at 20:09 -0400, Jeff King wrote:\n> On Wed, Jul 30, 2014 at 12:43:09PM -0700, Junio C Hamano wrote:\n> \n> > > \"git log --cc\" is one of the things I wanted for a long time to fix.\n> > > When the user explicitly asks \"--cc\", we currently ignore it, but\n> > > because we know the user wants to view combined diff, we should turn\n> > > \"-p\" on automatically.  And the change this patch introduces will be\n> > > broken when we fix \"log --cc\" (\"stash list\" will end up always\n> > > showing the patch, without a way to disable it).\n> > >\n> > > Can you make this conditional?  Do this only when <options> are\n> > > given to \"git stash list\" command and that includes \"-p\" or\n> > > something?\n> \n> I'm definitely sympathetic. Using \"--cc\" with the diff family _does_\n> imply \"-p\" already, so it would be nice to do the same for the log\n> family.\n> \n> > Perhaps something along this line?\n> > \n> >  git-stash.sh | 11 +++++++++--\n> >  1 file changed, 9 insertions(+), 2 deletions(-)\n> > \n> > diff --git a/git-stash.sh b/git-stash.sh\n> > index ae73ba4..0db1b19 100755\n> > --- a/git-stash.sh\n> > +++ b/git-stash.sh\n> > @@ -297,8 +297,15 @@ have_stash () {\n> >  \n> >  list_stash () {\n> >  \thave_stash || return 0\n> > -\tgit log --format=\"%gd: %gs\" -g --cc --simplify-combined-diff \\\n> > -\t\t\"$@\" $ref_stash --\n> > +\tcase \" $* \" in\n> > +\t*' --cc '*)\n> > +\t\t;; # the user knows what she is doing\n> > +\t*' -p '* | *' -U'*)\n> > +\t\tset x \"--cc\" \"--simplify-combined-diff\" \"$@\"\n> > +\t\tshift\n> > +\t\t;;\n> > +\tesac\n> > +\tgit log --format=\"%gd: %gs\" -g \"$@\" $ref_stash --\n> \n> Ugh. I'd generally like to avoid this sort of ad-hoc command line\n> parsing, as it is easy to get confused by option arguments or\n> even non-options. At least we do not have to worry about pathspecs here,\n> as we already do an explicit \"--\" (so we might be confused by them, but\n> they are broken anyway).\n> \n> Still, I wonder if a cleaner solution is to provide alternate \"default\n> to this\" options for the revision.c option parser. We already have\n> \"--default\" to pick the default starting point. Could we do something\n> like \"--default-merge-handling=cc\" or something?\n> \n> There's a similar ad-hoc parsing case in \"stash show\", where we add\n> \"--stat\" if no arguments are given, but we have no clue if a diff format\n> was given (so \"git stash show --color\" accidentally turns on patch\n> format!). Something like \"--default-diff-format=stat\" would be more\n> robust.\n> \n> I dunno. Maybe it is over-engineering. I just hate to see solutions that\n> work most of the time and crumble in weird corner cases, even if people\n> don't hit those corner cases very often.\n> \n> -Peff\n\nI agree with you on this one. Those corner cases tend to bite the worst.\n\nThanks,\nJake\n\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n\n\n"}]}