{"thread":{"id":"53639","subject":"[PATCH 0/3] improve git-diff documentation and A...B handling","startedAt":"2020-06-09T00:03:51Z","lastAt":"2020-06-12T19:25:56Z","messageCount":26,"participants":["Chris Torek via GitGitGadget","Junio C Hamano","Chris Torek","Philip Oakley"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"399331","messageId":"pull.804.git.git.1591661021.gitgitgadget@gmail.com","threadId":"53639","inReplyTo":null,"subject":"[PATCH 0/3] improve git-diff documentation and A...B handling","fromName":"Chris Torek via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-06-09T00:03:38Z","receivedAt":"2020-06-09T00:03:51Z","isPatch":true,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"git diff -h help is succinct, but perhaps too much so.\n\nThe symmetric-diff syntax, git diff A...B, is defined by the documentation\nto compare the merge base of A and B to commit B. It does so just fine when\nthere is a merge base. It compares A and B directly if there is no merge\nbase, and it is overly forgiving of bad arguments after which it can produce\nnonsensical diffs.\n\nThe first patch simply adjusts a test that will fail if the second patch is\naccepted. The second patch adds special handling for the symmetric diff\nsyntax so that the option parsing works, plus a small test suite. The third\npatch just updates the SYNOPSIS section of the documentation and makes the\nhelp output more verbose (to match the SYNOPSIS and provide common diff\noptions like git-diff-files, for instance).\n\nChris Torek (3):\n t/t3430: avoid undocumented git diff behavior\n git diff: improve A...B merge-base handling\n Documentation: tweak git diff help slightly\n\n Documentation/git-diff.txt | 2 +\n builtin/diff.c | 138 ++++++++++++++++++++++++++++++++-----\n t/t3430-rebase-merges.sh | 2 +-\n t/t4068-diff-symmetric.sh | 81 ++++++++++++++++++++++\n 4 files changed, 206 insertions(+), 17 deletions(-)\n create mode 100755 t/t4068-diff-symmetric.sh\n\n\nbase-commit: 20514004ddf1a3528de8933bc32f284e175e1012\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-804%2Fchris3torek%2Fcleanup-diff-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-804/chris3torek/cleanup-diff-v1\nPull-Request: https://github.com/git/git/pull/804\n-- \ngitgitgadget\n"},{"id":"399332","messageId":"f7c8f094e02406a7d0cb0c61f880e5b01fa413c4.1591661021.git.gitgitgadget@gmail.com","threadId":"53639","inReplyTo":"pull.804.git.git.1591661021.gitgitgadget@gmail.com","subject":"[PATCH 2/3] git diff: improve A...B merge-base handling","fromName":"Chris Torek via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-06-09T00:03:40Z","receivedAt":"2020-06-09T00:03:54Z","isPatch":true,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"From: Chris Torek \n\nWhen git diff is given a symmetric difference A...B, it chooses\nsome merge base from the two specified commits (as documented).\n\nThis fails, however, if there is *no* merge base: instead, you\nsee the differences between A and B, which is certainly not what\nis expected.\n\nMoreover, if additional revisions are specified on the command\nline (\"git diff A...B C\"), the results get a bit weird:\n\n * If there is a symmetric difference merge base, this is used\n as the left side of the diff. The last final ref is used as\n the right side.\n * If there is no merge base, the symmetric status is completely\n lost. We will produce a combined diff instead.\n\nSimilar weirdness occurs if you use, e.g., \"git diff C A...B D\".\n\nTo avoid all this, add a routine to catch the A...B case and verify that\nthere is at least one merge base, and that the arguments make sense.\nAs a side effect, produce a warning showing *which* merge base is being\nused when there are multiple choices; die if there is no merge base.\n\nSigned-off-by: Chris Torek \n---\n builtin/diff.c | 129 +++++++++++++++++++++++++++++++++-----\n t/t4068-diff-symmetric.sh | 81 ++++++++++++++++++++++++\n 2 files changed, 195 insertions(+), 15 deletions(-)\n create mode 100755 t/t4068-diff-symmetric.sh\n\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 8537b17bd5e..8b8b95ec97e 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -6,6 +6,7 @@\n #define USE_THE_INDEX_COMPATIBILITY_MACROS\n #include \"cache.h\"\n #include \"config.h\"\n+#include \"ewah/ewok.h\"\n #include \"lockfile.h\"\n #include \"color.h\"\n #include \"commit.h\"\n@@ -254,6 +255,103 @@ static int builtin_diff_files(struct rev_info *revs, int argc, const char **argv\n \treturn run_diff_files(revs, options);\n }\n \n+struct symdiff {\n+\tstruct bitmap *skip;\t/* bitmap of commit indices to skip, or NULL */\n+\tint warn;\t\t/* true if there were multiple merge bases */\n+\tint base, left, right;\t/* index of chosen merge base and left&right */\n+};\n+\n+/*\n+ * Check for symmetric-difference arguments, and if present, arrange\n+ * everything we need to know to handle them correctly.\n+ *\n+ * For an actual symmetric diff, *symdiff is set this way:\n+ *\n+ * - its skip is non-NULL and marks *all* rev->pending.objects[i]\n+ * indices that the caller should ignore (extra merge bases, of\n+ * which there might be many, and A in A...B). Note that the\n+ * chosen merge base and right side are NOT marked.\n+ * - warn is set if there are multiple merge bases.\n+ * - base, left, and right hold the merge base and left and\n+ * right side indices, for warnings or errors.\n+ *\n+ * If there is no symmetric diff argument, sym->skip is NULL and\n+ * sym->warn is cleared. The remaining fields are not set.\n+ *\n+ * If the user provides a symmetric diff with no merge base, or\n+ * more than one range, we do a usage-exit.\n+ */\n+static void builtin_diff_symdiff(struct rev_info *rev, struct symdiff *sym)\n+{\n+\tint i, lcount = 0, rcount = 0, basecount = 0;\n+\tint lpos = -1, rpos = -1, basepos = -1;\n+\tstruct bitmap *map = NULL;\n+\n+\t/*\n+\t * Use the whence fields to find merge bases and left and\n+\t * right parts of symmetric difference, so that we do not\n+\t * depend on the order that revisions are parsed. If there\n+\t * are any revs that aren't from these sources, we have a\n+\t * \"git diff C A...B\" or \"git diff A...B C\" case. Or we\n+\t * could even get \"git diff A...B C...E\", for instance.\n+\t *\n+\t * If we don't have just one merge base, we pick one\n+\t * at random.\n+\t *\n+\t * NB: REV_CMD_LEFT, REV_CMD_RIGHT are also used for A..B,\n+\t * so we must check for SYMMETRIC_LEFT too. The two arrays\n+\t * rev->pending.objects and rev->cmdline.rev are parallel.\n+\t */\n+\tfor (i = 0; i < rev->cmdline.nr; i++) {\n+\t\tstruct object *obj = rev->pending.objects[i].item;\n+\t\tswitch (rev->cmdline.rev[i].whence) {\n+\t\tcase REV_CMD_MERGE_BASE:\n+\t\t\tif (basepos < 0)\n+\t\t\t\tbasepos = i;\n+\t\t\tbasecount++;\n+\t\t\tbreak;\t\t/* do mark all bases */\n+\t\tcase REV_CMD_LEFT:\n+\t\t\tif (obj->flags & SYMMETRIC_LEFT) {\n+\t\t\t\tlpos = i;\n+\t\t\t\tlcount++;\n+\t\t\t\tbreak;\t/* do mark A */\n+\t\t\t}\n+\t\t\tcontinue;\n+\t\tcase REV_CMD_RIGHT:\n+\t\t\trpos = i;\n+\t\t\trcount++;\n+\t\t\tcontinue;\t/* don't mark B */\n+\t\tdefault:\n+\t\t\tcontinue;\n+\t\t}\n+\t\tif (map == NULL)\n+\t\t\tmap = bitmap_new();\n+\t\tbitmap_set(map, i);\n+\t}\n+\n+\tif (lcount == 0) {\t/* not a symmetric difference */\n+\t\tbitmap_free(map);\n+\t\tsym->warn = 0;\n+\t\tsym->skip = NULL;\n+\t\treturn;\n+\t}\n+\n+\tif (lcount != 1)\n+\t\tdie(_(\"cannot use more than one symmetric difference\"));\n+\n+\tif (basecount == 0) {\n+\t\tconst char *lname = rev->pending.objects[lpos].name;\n+\t\tconst char *rname = rev->pending.objects[rpos].name;\n+\t\tdie(_(\"%s...%s: no merge base\"), lname, rname);\n+\t}\n+\tbitmap_unset(map, basepos);\t/* unmark the base we want */\n+\tsym->base = basepos;\n+\tsym->left = lpos;\n+\tsym->right = rpos;\n+\tsym->warn = basecount > 1;\n+\tsym->skip = map;\n+}\n+\n int cmd_diff(int argc, const char **argv, const char *prefix)\n {\n \tint i;\n@@ -263,6 +361,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \tstruct object_array_entry *blob[2];\n \tint nongit = 0, no_index = 0;\n \tint result = 0;\n+\tstruct symdiff sdiff;\n \n \t/*\n \t * We could get N tree-ish in the rev.pending_objects list.\n@@ -382,6 +481,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n+\tbuiltin_diff_symdiff(&rev, &sdiff);\n \tfor (i = 0; i < rev.pending.nr; i++) {\n \t\tstruct object_array_entry *entry = &rev.pending.objects[i];\n \t\tstruct object *obj = entry->item;\n@@ -394,8 +494,9 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\t\tdie(_(\"invalid object '%s' given.\"), name);\n \t\tif (obj->type == OBJ_COMMIT)\n \t\t\tobj = &get_commit_tree(((struct commit *)obj))->object;\n-\n \t\tif (obj->type == OBJ_TREE) {\n+\t\t\tif (sdiff.skip && bitmap_get(sdiff.skip, i))\n+\t\t\t\tcontinue;\n \t\t\tobj->flags |= flags;\n \t\t\tadd_object_array(obj, name, &ent);\n \t\t} else if (obj->type == OBJ_BLOB) {\n@@ -437,24 +538,22 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\tusage(builtin_diff_usage);\n \telse if (ent.nr == 1)\n \t\tresult = builtin_diff_index(&rev, argc, argv);\n-\telse if (ent.nr == 2)\n+\telse if (ent.nr == 2) {\n+\t\tif (sdiff.warn) {\n+\t\t\tconst char *lname = rev.pending.objects[sdiff.left].name;\n+\t\t\tconst char *rname = rev.pending.objects[sdiff.right].name;\n+\t\t\tconst char *basename = rev.pending.objects[sdiff.base].name;\n+\t\t\twarning(_(\"%s...%s: multiple merge bases, using %s\"),\n+\t\t\t\tlname, rname, basename);\n+\t\t}\n \t\tresult = builtin_diff_tree(&rev, argc, argv,\n \t\t\t\t\t &ent.objects[0], &ent.objects[1]);\n-\telse if (ent.objects[0].item->flags & UNINTERESTING) {\n-\t\t/*\n-\t\t * diff A...B where there is at least one merge base\n-\t\t * between A and B. We have ent.objects[0] ==\n-\t\t * merge-base, ent.objects[ents-2] == A, and\n-\t\t * ent.objects[ents-1] == B. Show diff between the\n-\t\t * base and B. Note that we pick one merge base at\n-\t\t * random if there are more than one.\n-\t\t */\n-\t\tresult = builtin_diff_tree(&rev, argc, argv,\n-\t\t\t\t\t &ent.objects[0],\n-\t\t\t\t\t &ent.objects[ent.nr-1]);\n-\t} else\n+\t} else {\n+\t\tif (sdiff.skip)\n+\t\t\tusage(builtin_diff_usage);\n \t\tresult = builtin_diff_combined(&rev, argc, argv,\n \t\t\t\t\t ent.objects, ent.nr);\n+\t}\n \tresult = diff_result_code(&rev.diffopt, result);\n \tif (1 < rev.diffopt.skip_stat_unmatch)\n \t\trefresh_index_quietly();\ndiff --git a/t/t4068-diff-symmetric.sh b/t/t4068-diff-symmetric.sh\nnew file mode 100755\nindex 00000000000..7b5988933da\n--- /dev/null\n+++ b/t/t4068-diff-symmetric.sh\n@@ -0,0 +1,81 @@\n+#!/bin/sh\n+\n+test_description='behavior of diff with symmetric-diff setups'\n+\n+. ./test-lib.sh\n+\n+# build these situations:\n+# - normal merge with one merge base (b1...b2);\n+# - criss-cross merge ie 2 merge bases (b1...master);\n+# - disjoint subgraph (orphan branch, b3...master).\n+#\n+# B---E <-- master\n+# / \\ /\n+# A X\n+# \\ / \\\n+# C---D--G <-- br1\n+# \\ /\n+# ---F <-- br2\n+#\n+# H <-- br3\n+#\n+# We put files into a few commits so that we can verify the\n+# output as well.\n+\n+test_expect_success setup '\n+\tgit commit --allow-empty -m A &&\n+\techo b >b &&\n+\tgit add b &&\n+\tgit commit -m B &&\n+\tgit checkout -b br1 HEAD^ &&\n+\techo c >c &&\n+\tgit add c &&\n+\tgit commit -m C &&\n+\tgit tag commit-C &&\n+\tgit merge -m D master &&\n+\tgit tag commit-D &&\n+\tgit checkout master &&\n+\tgit merge -m E commit-C &&\n+\tgit checkout -b br2 commit-C &&\n+\techo f >f &&\n+\tgit add f &&\n+\tgit commit -m F &&\n+\tgit checkout br1 &&\n+\tgit merge -m G br2 &&\n+\tgit checkout --orphan br3 &&\n+\tgit commit -m H\n+'\n+\n+test_expect_success 'diff with one merge base' '\n+\tgit diff commit-D...br1 >tmp &&\n+\ttail -1 tmp >actual &&\n+\techo +f >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+# The output (in tmp) can have +b or +c depending\n+# on which merge base (commit B or C) is picked.\n+# It should have one of those two, which comes out\n+# to seven lines.\n+test_expect_success 'diff with two merge bases' '\n+\tgit diff br1...master >tmp 2>err &&\n+\ttest_line_count = 7 tmp &&\n+\ttest_line_count = 1 err\n+'\n+\n+test_expect_success 'diff with no merge bases' '\n+\ttest_must_fail git diff br2...br3 >tmp 2>err &&\n+\ttest_i18ngrep \"fatal: br2...br3: no merge base\" err\n+'\n+\n+test_expect_success 'diff with too many symmetric differences' '\n+\ttest_must_fail git diff br1...master br2...br3 >tmp 2>err &&\n+\ttest_i18ngrep \"fatal: cannot use more than one symmetric difference\" err\n+'\n+\n+test_expect_success 'diff with symmetric difference and extraneous arg' '\n+\ttest_must_fail git diff master br1...master >tmp 2>err &&\n+\ttest_i18ngrep \"usage\" err\n+'\n+\n+test_done\n-- \ngitgitgadget\n\n"},{"id":"399333","messageId":"414163bbc3cbdda241bedc7bc4dfb8b493071dcb.1591661021.git.gitgitgadget@gmail.com","threadId":"53639","inReplyTo":"pull.804.git.git.1591661021.gitgitgadget@gmail.com","subject":"[PATCH 1/3] t/t3430: avoid undocumented git diff behavior","fromName":"Chris Torek via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-06-09T00:03:39Z","receivedAt":"2020-06-09T00:04:03Z","isPatch":true,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"From: Chris Torek \n\nAccording to the documentation, \"git diff\" takes at most two commit-ish,\nor an A..B style range, or an A...B style symmetric difference range.\nThe autosquash-and-exec test relied on \"git diff HEAD^!\", which works\nfine for ordinary commits as the revision parse produces two commit-ish,\nnamely ^HEAD^ and HEAD.\n\nFor merge commits, however, this test makes use of an undocumented\nfeature: the resulting revision parse has all the parents as UNINTERESTING\nfollowed by the HEAD commit. This looks identical to a symmetric\ndiff parse, which lists the merge bases as UNINTERESTING, followed by\nthe A (UNINTERESTING) and B revs. So the diff winds up treating it\nas one, using the first oid (i.e., HEAD^) and the last (i.e., HEAD).\nThe documentation, however, says nothing about this usage.\n\nSince diff actually just uses HEAD^ and HEAD, call for these directly\nhere. That makes it possible to improve the diff code's handling of\nsymmetric difference arguments.\n\nSigned-off-by: Chris Torek \n---\n t/t3430-rebase-merges.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh\nindex a1bc3e20016..b454f400ebd 100755\n--- a/t/t3430-rebase-merges.sh\n+++ b/t/t3430-rebase-merges.sh\n@@ -420,7 +420,7 @@ test_expect_success 'with --autosquash and --exec' '\n \tgit commit --fixup B B.t &&\n \twrite_script show.sh <<-\\EOF &&\n \tsubject=\"$(git show -s --format=%s HEAD)\"\n-\tcontent=\"$(git diff HEAD^! | tail -n 1)\"\n+\tcontent=\"$(git diff HEAD^ HEAD | tail -n 1)\"\n \techo \"$subject: $content\"\n \tEOF\n \ttest_tick &&\n-- \ngitgitgadget\n\n"},{"id":"399334","messageId":"9318365915cfe1898b2942c735d675656ce7b5e5.1591661021.git.gitgitgadget@gmail.com","threadId":"53639","inReplyTo":"pull.804.git.git.1591661021.gitgitgadget@gmail.com","subject":"[PATCH 3/3] Documentation: tweak git diff help slightly","fromName":"Chris Torek via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-06-09T00:03:41Z","receivedAt":"2020-06-09T00:04:13Z","isPatch":true,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"From: Chris Torek \n\nUpdate the manual page synopsis to include the two and three\ndot notation.\n\nMake \"git diff -h\" print the same usage summary as the manual\npage synopsis.\n\nSigned-off-by: Chris Torek \n---\n Documentation/git-diff.txt | 2 ++\n builtin/diff.c | 9 ++++++++-\n 2 files changed, 10 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt\nindex 37781cf1755..c6a201abd72 100644\n--- a/Documentation/git-diff.txt\n+++ b/Documentation/git-diff.txt\n@@ -12,6 +12,8 @@ SYNOPSIS\n 'git diff' [] [] [--] [...]\n 'git diff' [] --cached [] [--] [...]\n 'git diff' [] [--] [...]\n+'git diff' [] .. [--] [...]\n+'git diff' [] ... [--] [...]\n 'git diff' [] \n 'git diff' [] --no-index [--] \n \ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 8b8b95ec97e..365f9e9a908 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -24,7 +24,14 @@\n #define DIFF_NO_INDEX_IMPLICIT 2\n \n static const char builtin_diff_usage[] =\n-\"git diff [] [ []] [--] [...]\";\n+\"git diff [] [] [--] [...]\\n\"\n+\" or: git diff [] --cached [] [--] [...]\\n\"\n+\" or: git diff [] ] [--] [...]\\n\"\n+\" or: git diff [] ..] [--] [...]\\n\"\n+\" or: git diff [] ...] [--] [...]\\n\"\n+\" or: git diff [] ]\\n\"\n+\" or: git diff [] --no-index [--] ]\\n\"\n+COMMON_DIFF_OPTIONS_HELP;\n \n static const char *blob_path(struct object_array_entry *entry)\n {\n-- \ngitgitgadget\n"},{"id":"399340","messageId":"xmqqr1uoizqc.fsf@gitster.c.googlers.com","threadId":"53639","inReplyTo":"414163bbc3cbdda241bedc7bc4dfb8b493071dcb.1591661021.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/3] t/t3430: avoid undocumented git diff behavior","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-09T05:18:19Z","receivedAt":"2020-06-09T05:18:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Chris Torek via GitGitGadget\" writes:\n\n> From: Chris Torek \n>\n> According to the documentation, \"git diff\" takes at most two commit-ish,\n> or an A..B style range, or an A...B style symmetric difference range.\n> The autosquash-and-exec test relied on \"git diff HEAD^!\", which works\n> fine for ordinary commits as the revision parse produces two commit-ish,\n> namely ^HEAD^ and HEAD.\n>\n> For merge commits, however, this test makes use of an undocumented\n> feature:\n\ns/undocumented feature/undefined behaviour/;\n\nThe show.sh scripts wants to compute the diff against first parent,\nand it uses a range notation HEAD^! which happens to mean\nHEAD^..HEAD for a single parent commit, but it forgets that the\ncommit it may get fed could be a merge. What the code happens to do\nwhen given \"git diff ^HEAD^2 HEAD^..HEAD\" is undefined behaviour and\ndoes not even ...\n\n> the resulting revision parse has all the parents as UNINTERESTING\n> followed by the HEAD commit. This looks identical to a symmetric\n> diff parse, which lists the merge bases as UNINTERESTING, followed by\n> the A (UNINTERESTING) and B revs. So the diff winds up treating it\n> as one, using the first oid (i.e., HEAD^) and the last (i.e., HEAD).\n> The documentation, however, says nothing about this usage.\n\n...deserve to be explained in a paragraph like this, I would think.\n\n> Since diff actually just uses HEAD^ and HEAD, call for these directly\n> here. That makes it possible to improve the diff code's handling of\n> symmetric difference arguments.\n\nYes, the resulting code expresses the intent much better.\n\n\n>\n> Signed-off-by: Chris Torek \n> ---\n> t/t3430-rebase-merges.sh | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh\n> index a1bc3e20016..b454f400ebd 100755\n> --- a/t/t3430-rebase-merges.sh\n> +++ b/t/t3430-rebase-merges.sh\n> @@ -420,7 +420,7 @@ test_expect_success 'with --autosquash and --exec' '\n> \tgit commit --fixup B B.t &&\n> \twrite_script show.sh <<-\\EOF &&\n> \tsubject=\"$(git show -s --format=%s HEAD)\"\n> -\tcontent=\"$(git diff HEAD^! | tail -n 1)\"\n> +\tcontent=\"$(git diff HEAD^ HEAD | tail -n 1)\"\n> \techo \"$subject: $content\"\n> \tEOF\n> \ttest_tick &&\n"},{"id":"399341","messageId":"xmqqmu5ciyom.fsf@gitster.c.googlers.com","threadId":"53639","inReplyTo":"f7c8f094e02406a7d0cb0c61f880e5b01fa413c4.1591661021.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/3] git diff: improve A...B merge-base handling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-09T05:40:57Z","receivedAt":"2020-06-09T05:41:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Chris Torek via GitGitGadget\" writes:\n\n> +struct symdiff {\n> +\tstruct bitmap *skip;\t/* bitmap of commit indices to skip, or NULL */\n> +\tint warn;\t\t/* true if there were multiple merge bases */\n> +\tint base, left, right;\t/* index of chosen merge base and left&right */\n> +};\n> +\n> +/*\n> + * Check for symmetric-difference arguments, and if present, arrange\n> + * everything we need to know to handle them correctly.\n> + *\n> + * For an actual symmetric diff, *symdiff is set this way:\n> + *\n> + * - its skip is non-NULL and marks *all* rev->pending.objects[i]\n> + * indices that the caller should ignore (extra merge bases, of\n> + * which there might be many, and A in A...B). Note that the\n> + * chosen merge base and right side are NOT marked.\n> + * - warn is set if there are multiple merge bases.\n> + * - base, left, and right hold the merge base and left and\n> + * right side indices, for warnings or errors.\n> + *\n> + * If there is no symmetric diff argument, sym->skip is NULL and\n> + * sym->warn is cleared. The remaining fields are not set.\n> + *\n> + * If the user provides a symmetric diff with no merge base, or\n> + * more than one range, we do a usage-exit.\n> + */\n> +static void builtin_diff_symdiff(struct rev_info *rev, struct symdiff *sym)\n\nThe function name feels quite suboptimal. At least I thought that\nby the time a call to this function returns, we would have already\nproduced a symmetric diff output from its name, but apparently that\nis not what is being done. Calling it symdiff_prepare() may be a\nvast improvement, perhaps.\n\n> +{\n> +\tint i, lcount = 0, rcount = 0, basecount = 0;\n> +\tint lpos = -1, rpos = -1, basepos = -1;\n> +\tstruct bitmap *map = NULL;\n> +\n> +\t/*\n> +\t * Use the whence fields to find merge bases and left and\n> +\t * right parts of symmetric difference, so that we do not\n> +\t * depend on the order that revisions are parsed. If there\n> +\t * are any revs that aren't from these sources, we have a\n> +\t * \"git diff C A...B\" or \"git diff A...B C\" case. Or we\n> +\t * could even get \"git diff A...B C...E\", for instance.\n> +\t *\n> +\t * If we don't have just one merge base, we pick one\n> +\t * at random.\n> +\t *\n> +\t * NB: REV_CMD_LEFT, REV_CMD_RIGHT are also used for A..B,\n> +\t * so we must check for SYMMETRIC_LEFT too. The two arrays\n> +\t * rev->pending.objects and rev->cmdline.rev are parallel.\n> +\t */\n> +\tfor (i = 0; i < rev->cmdline.nr; i++) {\n> +\t\tstruct object *obj = rev->pending.objects[i].item;\n> +\t\tswitch (rev->cmdline.rev[i].whence) {\n> +\t\tcase REV_CMD_MERGE_BASE:\n> +\t\t\tif (basepos < 0)\n> +\t\t\t\tbasepos = i;\n> +\t\t\tbasecount++;\n> +\t\t\tbreak;\t\t/* do mark all bases */\n> +\t\tcase REV_CMD_LEFT:\n> +\t\t\tif (obj->flags & SYMMETRIC_LEFT) {\n> +\t\t\t\tlpos = i;\n> +\t\t\t\tlcount++;\n> +\t\t\t\tbreak;\t/* do mark A */\n> +\t\t\t}\n> +\t\t\tcontinue;\n> +\t\tcase REV_CMD_RIGHT:\n> +\t\t\trpos = i;\n> +\t\t\trcount++;\n\nEven though, unlike lcount, you allow arbitrary number of rcount,\nand rpos uses \"the last one wins\" semantics. Can we describe in the\ncomment above what use case benefits from this looseness (as opposed\nto erroring out when rcount is NOT 1, like done for lcount)?\n\n> +\t\t\tcontinue;\t/* don't mark B */\n> +\t\tdefault:\n> +\t\t\tcontinue;\n> +\t\t}\n> +\t\tif (map == NULL)\n> +\t\t\tmap = bitmap_new();\n> +\t\tbitmap_set(map, i);\n> +\t}\n> +\n> +\tif (lcount == 0) {\t/* not a symmetric difference */\n> +\t\tbitmap_free(map);\n> +\t\tsym->warn = 0;\n> +\t\tsym->skip = NULL;\n> +\t\treturn;\n> +\t}\n> +\n> +\tif (lcount != 1)\n> +\t\tdie(_(\"cannot use more than one symmetric difference\"));\n> +\n> +\tif (basecount == 0) {\n> +\t\tconst char *lname = rev->pending.objects[lpos].name;\n> +\t\tconst char *rname = rev->pending.objects[rpos].name;\n> +\t\tdie(_(\"%s...%s: no merge base\"), lname, rname);\n> +\t}\n> +\tbitmap_unset(map, basepos);\t/* unmark the base we want */\n> +\tsym->base = basepos;\n> +\tsym->left = lpos;\n> +\tsym->right = rpos;\n> +\tsym->warn = basecount > 1;\n> +\tsym->skip = map;\n> +}\n> +\n> int cmd_diff(int argc, const char **argv, const char *prefix)\n> {\n> \tint i;\n> @@ -263,6 +361,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n> \tstruct object_array_entry *blob[2];\n> \tint nongit = 0, no_index = 0;\n> \tint result = 0;\n> +\tstruct symdiff sdiff;\n> \n> \t/*\n> \t * We could get N tree-ish in the rev.pending_objects list.\n> @@ -382,6 +481,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n> \t\t}\n> \t}\n> \n> +\tbuiltin_diff_symdiff(&rev, &sdiff);\n> \tfor (i = 0; i < rev.pending.nr; i++) {\n> \t\tstruct object_array_entry *entry = &rev.pending.objects[i];\n> \t\tstruct object *obj = entry->item;\n> @@ -394,8 +494,9 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n> \t\t\tdie(_(\"invalid object '%s' given.\"), name);\n> \t\tif (obj->type == OBJ_COMMIT)\n> \t\t\tobj = &get_commit_tree(((struct commit *)obj))->object;\n> -\n> \t\tif (obj->type == OBJ_TREE) {\n> +\t\t\tif (sdiff.skip && bitmap_get(sdiff.skip, i))\n> +\t\t\t\tcontinue;\n> \t\t\tobj->flags |= flags;\n> \t\t\tadd_object_array(obj, name, &ent);\n> \t\t} else if (obj->type == OBJ_BLOB) {\n> @@ -437,24 +538,22 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n> \t\tusage(builtin_diff_usage);\n> \telse if (ent.nr == 1)\n> \t\tresult = builtin_diff_index(&rev, argc, argv);\n> -\telse if (ent.nr == 2)\n> +\telse if (ent.nr == 2) {\n> +\t\tif (sdiff.warn) {\n> +\t\t\tconst char *lname = rev.pending.objects[sdiff.left].name;\n> +\t\t\tconst char *rname = rev.pending.objects[sdiff.right].name;\n> +\t\t\tconst char *basename = rev.pending.objects[sdiff.base].name;\n> +\t\t\twarning(_(\"%s...%s: multiple merge bases, using %s\"),\n> +\t\t\t\tlname, rname, basename);\n> +\t\t}\n> \t\tresult = builtin_diff_tree(&rev, argc, argv,\n> \t\t\t\t\t &ent.objects[0], &ent.objects[1]);\n> -\telse if (ent.objects[0].item->flags & UNINTERESTING) {\n> -\t\t/*\n> -\t\t * diff A...B where there is at least one merge base\n> -\t\t * between A and B. We have ent.objects[0] ==\n> -\t\t * merge-base, ent.objects[ents-2] == A, and\n> -\t\t * ent.objects[ents-1] == B. Show diff between the\n> -\t\t * base and B. Note that we pick one merge base at\n> -\t\t * random if there are more than one.\n> -\t\t */\n> -\t\tresult = builtin_diff_tree(&rev, argc, argv,\n> -\t\t\t\t\t &ent.objects[0],\n> -\t\t\t\t\t &ent.objects[ent.nr-1]);\n> -\t} else\n> +\t} else {\n> +\t\tif (sdiff.skip)\n> +\t\t\tusage(builtin_diff_usage);\n\nsdiff.skip being non-NULL means symdiff_prepare() saw one A...B that\nproduced two ents and the fact that we have more than two ents mean\nthat the command line gave us other tree-ishes, e.g. \"git diff A...B C\"\nand it is rejected here. OK.\n\n> \t\tresult = builtin_diff_combined(&rev, argc, argv,\n> \t\t\t\t\t ent.objects, ent.nr);\n> +\t}\n"},{"id":"399342","messageId":"xmqqimg0iyg9.fsf@gitster.c.googlers.com","threadId":"53639","inReplyTo":"9318365915cfe1898b2942c735d675656ce7b5e5.1591661021.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/3] Documentation: tweak git diff help slightly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-09T05:45:58Z","receivedAt":"2020-06-09T05:46:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Chris Torek via GitGitGadget\" writes:\n\n> 'git diff' [] [] [--] [...]\n> 'git diff' [] --cached [] [--] [...]\n> 'git diff' [] [--] [...]\n> +'git diff' [] .. [--] [...]\n> +'git diff' [] ... [--] [...]\n> 'git diff' [] \n> 'git diff' [] --no-index [--] \n\nWe actually are trying to wean users off of saying \"diff A..B\" which\nis a nonsense notation, so I'd rather not to see it added here.\nDescribing \"diff A...B\" is a good idea, though.\n\nWhile we have our attention on this part of the documentation, would\nit make sense to also add description on invoking the combined diff\nas well?\n"},{"id":"399356","messageId":"pull.804.v2.git.git.1591729224.gitgitgadget@gmail.com","threadId":"53639","inReplyTo":"pull.804.git.git.1591661021.gitgitgadget@gmail.com","subject":"[PATCH v2 0/3] improve git-diff documentation and A...B handling","fromName":"Chris Torek via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-06-09T19:00:20Z","receivedAt":"2020-06-09T19:00:33Z","isPatch":true,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"git diff -h help is succinct, but perhaps too much so.\n\nThe symmetric-diff syntax, git diff A...B, is defined by the documentation\nto compare the merge base of A and B to commit B. It does so just fine when\nthere is a merge base. It compares A and B directly if there is no merge\nbase, and it is overly forgiving of bad arguments after which it can produce\nnonsensical diffs.\n\nThe first patch simply adjusts a test that will fail if the second patch is\naccepted. The second patch adds special handling for the symmetric diff\nsyntax so that the option parsing works, plus a small test suite. The third\npatch updates the documentation, including adding a section for combined\ncommits, and makes the help output more verbose (to match the SYNOPSIS and\nprovide common diff options like git-diff-files, for instance).\n\nChanges since v1: \n\n * shortened first commit's message \n * renamed prepare function \n * removed A..B syntax from usage (and fixed typo) \n * added combined diff syntax to main documentation \n\nNote: I looked into adding special handling for rev^! syntax and\nit seems a bit messy. prepare_symdiff() could do this with its\nother analysis, and slide the decoded revisions around. Perhaps\nbetter, revision.c could insert the parent refs after the child,\nunder control of a flag in the diff flags section of a rev_info.\n\nChris Torek (3):\n t/t3430: avoid undefined git diff behavior\n git diff: improve A...B merge-base handling\n Documentation: tweak git diff help slightly\n\n Documentation/git-diff.txt | 21 ++++--\n builtin/diff.c | 137 ++++++++++++++++++++++++++++++++-----\n t/t3430-rebase-merges.sh | 2 +-\n t/t4068-diff-symmetric.sh | 81 ++++++++++++++++++++++\n 4 files changed, 220 insertions(+), 21 deletions(-)\n create mode 100755 t/t4068-diff-symmetric.sh\n\n\nbase-commit: 20514004ddf1a3528de8933bc32f284e175e1012\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-804%2Fchris3torek%2Fcleanup-diff-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-804/chris3torek/cleanup-diff-v2\nPull-Request: https://github.com/git/git/pull/804\n\nRange-diff vs v1:\n\n 1: 414163bbc3c ! 1: 2ccaad645ff t/t3430: avoid undocumented git diff behavior\n @@ Metadata\n Author: Chris Torek \n \n ## Commit message ##\n - t/t3430: avoid undocumented git diff behavior\n + t/t3430: avoid undefined git diff behavior\n \n - According to the documentation, \"git diff\" takes at most two commit-ish,\n - or an A..B style range, or an A...B style symmetric difference range.\n - The autosquash-and-exec test relied on \"git diff HEAD^!\", which works\n - fine for ordinary commits as the revision parse produces two commit-ish,\n - namely ^HEAD^ and HEAD.\n -\n - For merge commits, however, this test makes use of an undocumented\n - feature: the resulting revision parse has all the parents as UNINTERESTING\n - followed by the HEAD commit. This looks identical to a symmetric\n - diff parse, which lists the merge bases as UNINTERESTING, followed by\n - the A (UNINTERESTING) and B revs. So the diff winds up treating it\n - as one, using the first oid (i.e., HEAD^) and the last (i.e., HEAD).\n - The documentation, however, says nothing about this usage.\n -\n - Since diff actually just uses HEAD^ and HEAD, call for these directly\n - here. That makes it possible to improve the diff code's handling of\n - symmetric difference arguments.\n + The autosquash-and-exec test used \"git diff HEAD^!\" to mean\n + \"git diff HEAD^ HEAD\". Use these directly instead of relying\n + on the undefined but actual-current behavior of \"HEAD^!\".\n \n Signed-off-by: Chris Torek \n \n 2: f7c8f094e02 ! 2: 100fa403477 git diff: improve A...B merge-base handling\n @@ builtin/diff.c: static int builtin_diff_files(struct rev_info *revs, int argc, c\n + * If the user provides a symmetric diff with no merge base, or\n + * more than one range, we do a usage-exit.\n + */\n -+static void builtin_diff_symdiff(struct rev_info *rev, struct symdiff *sym)\n ++static void symdiff_prepare(struct rev_info *rev, struct symdiff *sym)\n +{\n +\tint i, lcount = 0, rcount = 0, basecount = 0;\n +\tint lpos = -1, rpos = -1, basepos = -1;\n @@ builtin/diff.c: int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n -+\tbuiltin_diff_symdiff(&rev, &sdiff);\n ++\tsymdiff_prepare(&rev, &sdiff);\n \tfor (i = 0; i < rev.pending.nr; i++) {\n \t\tstruct object_array_entry *entry = &rev.pending.objects[i];\n \t\tstruct object *obj = entry->item;\n 3: 9318365915c ! 3: b9b4c6f113d Documentation: tweak git diff help slightly\n @@ Metadata\n ## Commit message ##\n Documentation: tweak git diff help slightly\n \n - Update the manual page synopsis to include the two and three\n - dot notation.\n + Update the manual page synopsis to include the three-dot notation\n + and the combined-diff option\n \n Make \"git diff -h\" print the same usage summary as the manual\n - page synopsis.\n + page synopsis, minus the \"A..B\" form, which is now discouraged.\n +\n + Document the usage for producing combined commits.\n \n Signed-off-by: Chris Torek \n \n ## Documentation/git-diff.txt ##\n @@ Documentation/git-diff.txt: SYNOPSIS\n + [verse]\n 'git diff' [] [] [--] [...]\n 'git diff' [] --cached [] [--] [...]\n - 'git diff' [] [--] [...]\n -+'git diff' [] .. [--] [...]\n +-'git diff' [] [--] [...]\n ++'git diff' [] [...] [--] [...]\n +'git diff' [] ... [--] [...]\n 'git diff' [] \n 'git diff' [] --no-index [--] \n \n + DESCRIPTION\n + -----------\n + Show changes between the working tree and the index or a tree, changes\n +-between the index and a tree, changes between two trees, changes between\n +-two blob objects, or changes between two files on disk.\n ++between the index and a tree, changes between two trees, changes resulting\n ++from a merge, changes between two blob objects, or changes between two\n ++files on disk.\n + \n + 'git diff' [] [--] [...]::\n + \n +@@ Documentation/git-diff.txt: two blob objects, or changes between two files on disk.\n + \tone side is omitted, it will have the same effect as\n + \tusing HEAD instead.\n + \n ++'git diff' [] [...] [--] [...]::\n ++\n ++\tThis form is to view the results of a merge commit. The first\n ++\tlisted must be the merge itself; the remaining two or\n ++\tmore commits should be its parents. A convenient way to produce\n ++\tthe desired set of revisions is to use the {caret}@ suffix, i.e.,\n ++\t\"git diff master master^@\". This is equivalent to running \"git\n ++\tshow --format=\" on the merge commit, e.g., \"git show --format=\n ++\tmaster\".\n ++\n + 'git diff' [] \\... [--] [...]::\n + \n + \tThis form is to view the changes on the branch containing\n +@@ Documentation/git-diff.txt: linkgit:git-difftool[1],\n + linkgit:git-log[1],\n + linkgit:gitdiffcore[7],\n + linkgit:git-format-patch[1],\n +-linkgit:git-apply[1]\n ++linkgit:git-apply[1],\n ++linkgit:git-show[1]\n + \n + GIT\n + ---\n \n ## builtin/diff.c ##\n @@\n @@ builtin/diff.c\n -\"git diff [] [ []] [--] [...]\";\n +\"git diff [] [] [--] [...]\\n\"\n +\" or: git diff [] --cached [] [--] [...]\\n\"\n -+\" or: git diff [] ] [--] [...]\\n\"\n -+\" or: git diff [] ..] [--] [...]\\n\"\n ++\" or: git diff [] [...] [--] [...]\\n\"\n +\" or: git diff [] ...] [--] [...]\\n\"\n +\" or: git diff [] ]\\n\"\n +\" or: git diff [] --no-index [--] ]\\n\"\n\n-- \ngitgitgadget\n"},{"id":"399357","messageId":"2ccaad645ff01b786e76dc63210d75da633389a6.1591729224.git.gitgitgadget@gmail.com","threadId":"53639","inReplyTo":"pull.804.v2.git.git.1591729224.gitgitgadget@gmail.com","subject":"[PATCH v2 1/3] t/t3430: avoid undefined git diff behavior","fromName":"Chris Torek via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-06-09T19:00:21Z","receivedAt":"2020-06-09T19:00:36Z","isPatch":true,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"From: Chris Torek \n\nThe autosquash-and-exec test used \"git diff HEAD^!\" to mean\n\"git diff HEAD^ HEAD\". Use these directly instead of relying\non the undefined but actual-current behavior of \"HEAD^!\".\n\nSigned-off-by: Chris Torek \n---\n t/t3430-rebase-merges.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh\nindex a1bc3e20016..b454f400ebd 100755\n--- a/t/t3430-rebase-merges.sh\n+++ b/t/t3430-rebase-merges.sh\n@@ -420,7 +420,7 @@ test_expect_success 'with --autosquash and --exec' '\n \tgit commit --fixup B B.t &&\n \twrite_script show.sh <<-\\EOF &&\n \tsubject=\"$(git show -s --format=%s HEAD)\"\n-\tcontent=\"$(git diff HEAD^! | tail -n 1)\"\n+\tcontent=\"$(git diff HEAD^ HEAD | tail -n 1)\"\n \techo \"$subject: $content\"\n \tEOF\n \ttest_tick &&\n-- \ngitgitgadget\n\n"},{"id":"399358","messageId":"100fa4034771e58b65cdac2f3dfb48531c07b735.1591729224.git.gitgitgadget@gmail.com","threadId":"53639","inReplyTo":"pull.804.v2.git.git.1591729224.gitgitgadget@gmail.com","subject":"[PATCH v2 2/3] git diff: improve A...B merge-base handling","fromName":"Chris Torek via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-06-09T19:00:22Z","receivedAt":"2020-06-09T19:00:36Z","isPatch":true,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"From: Chris Torek \n\nWhen git diff is given a symmetric difference A...B, it chooses\nsome merge base from the two specified commits (as documented).\n\nThis fails, however, if there is *no* merge base: instead, you\nsee the differences between A and B, which is certainly not what\nis expected.\n\nMoreover, if additional revisions are specified on the command\nline (\"git diff A...B C\"), the results get a bit weird:\n\n * If there is a symmetric difference merge base, this is used\n as the left side of the diff. The last final ref is used as\n the right side.\n * If there is no merge base, the symmetric status is completely\n lost. We will produce a combined diff instead.\n\nSimilar weirdness occurs if you use, e.g., \"git diff C A...B D\".\n\nTo avoid all this, add a routine to catch the A...B case and verify that\nthere is at least one merge base, and that the arguments make sense.\nAs a side effect, produce a warning showing *which* merge base is being\nused when there are multiple choices; die if there is no merge base.\n\nSigned-off-by: Chris Torek \n---\n builtin/diff.c | 129 +++++++++++++++++++++++++++++++++-----\n t/t4068-diff-symmetric.sh | 81 ++++++++++++++++++++++++\n 2 files changed, 195 insertions(+), 15 deletions(-)\n create mode 100755 t/t4068-diff-symmetric.sh\n\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 8537b17bd5e..0b6e63dbd02 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -6,6 +6,7 @@\n #define USE_THE_INDEX_COMPATIBILITY_MACROS\n #include \"cache.h\"\n #include \"config.h\"\n+#include \"ewah/ewok.h\"\n #include \"lockfile.h\"\n #include \"color.h\"\n #include \"commit.h\"\n@@ -254,6 +255,103 @@ static int builtin_diff_files(struct rev_info *revs, int argc, const char **argv\n \treturn run_diff_files(revs, options);\n }\n \n+struct symdiff {\n+\tstruct bitmap *skip;\t/* bitmap of commit indices to skip, or NULL */\n+\tint warn;\t\t/* true if there were multiple merge bases */\n+\tint base, left, right;\t/* index of chosen merge base and left&right */\n+};\n+\n+/*\n+ * Check for symmetric-difference arguments, and if present, arrange\n+ * everything we need to know to handle them correctly.\n+ *\n+ * For an actual symmetric diff, *symdiff is set this way:\n+ *\n+ * - its skip is non-NULL and marks *all* rev->pending.objects[i]\n+ * indices that the caller should ignore (extra merge bases, of\n+ * which there might be many, and A in A...B). Note that the\n+ * chosen merge base and right side are NOT marked.\n+ * - warn is set if there are multiple merge bases.\n+ * - base, left, and right hold the merge base and left and\n+ * right side indices, for warnings or errors.\n+ *\n+ * If there is no symmetric diff argument, sym->skip is NULL and\n+ * sym->warn is cleared. The remaining fields are not set.\n+ *\n+ * If the user provides a symmetric diff with no merge base, or\n+ * more than one range, we do a usage-exit.\n+ */\n+static void symdiff_prepare(struct rev_info *rev, struct symdiff *sym)\n+{\n+\tint i, lcount = 0, rcount = 0, basecount = 0;\n+\tint lpos = -1, rpos = -1, basepos = -1;\n+\tstruct bitmap *map = NULL;\n+\n+\t/*\n+\t * Use the whence fields to find merge bases and left and\n+\t * right parts of symmetric difference, so that we do not\n+\t * depend on the order that revisions are parsed. If there\n+\t * are any revs that aren't from these sources, we have a\n+\t * \"git diff C A...B\" or \"git diff A...B C\" case. Or we\n+\t * could even get \"git diff A...B C...E\", for instance.\n+\t *\n+\t * If we don't have just one merge base, we pick one\n+\t * at random.\n+\t *\n+\t * NB: REV_CMD_LEFT, REV_CMD_RIGHT are also used for A..B,\n+\t * so we must check for SYMMETRIC_LEFT too. The two arrays\n+\t * rev->pending.objects and rev->cmdline.rev are parallel.\n+\t */\n+\tfor (i = 0; i < rev->cmdline.nr; i++) {\n+\t\tstruct object *obj = rev->pending.objects[i].item;\n+\t\tswitch (rev->cmdline.rev[i].whence) {\n+\t\tcase REV_CMD_MERGE_BASE:\n+\t\t\tif (basepos < 0)\n+\t\t\t\tbasepos = i;\n+\t\t\tbasecount++;\n+\t\t\tbreak;\t\t/* do mark all bases */\n+\t\tcase REV_CMD_LEFT:\n+\t\t\tif (obj->flags & SYMMETRIC_LEFT) {\n+\t\t\t\tlpos = i;\n+\t\t\t\tlcount++;\n+\t\t\t\tbreak;\t/* do mark A */\n+\t\t\t}\n+\t\t\tcontinue;\n+\t\tcase REV_CMD_RIGHT:\n+\t\t\trpos = i;\n+\t\t\trcount++;\n+\t\t\tcontinue;\t/* don't mark B */\n+\t\tdefault:\n+\t\t\tcontinue;\n+\t\t}\n+\t\tif (map == NULL)\n+\t\t\tmap = bitmap_new();\n+\t\tbitmap_set(map, i);\n+\t}\n+\n+\tif (lcount == 0) {\t/* not a symmetric difference */\n+\t\tbitmap_free(map);\n+\t\tsym->warn = 0;\n+\t\tsym->skip = NULL;\n+\t\treturn;\n+\t}\n+\n+\tif (lcount != 1)\n+\t\tdie(_(\"cannot use more than one symmetric difference\"));\n+\n+\tif (basecount == 0) {\n+\t\tconst char *lname = rev->pending.objects[lpos].name;\n+\t\tconst char *rname = rev->pending.objects[rpos].name;\n+\t\tdie(_(\"%s...%s: no merge base\"), lname, rname);\n+\t}\n+\tbitmap_unset(map, basepos);\t/* unmark the base we want */\n+\tsym->base = basepos;\n+\tsym->left = lpos;\n+\tsym->right = rpos;\n+\tsym->warn = basecount > 1;\n+\tsym->skip = map;\n+}\n+\n int cmd_diff(int argc, const char **argv, const char *prefix)\n {\n \tint i;\n@@ -263,6 +361,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \tstruct object_array_entry *blob[2];\n \tint nongit = 0, no_index = 0;\n \tint result = 0;\n+\tstruct symdiff sdiff;\n \n \t/*\n \t * We could get N tree-ish in the rev.pending_objects list.\n@@ -382,6 +481,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n+\tsymdiff_prepare(&rev, &sdiff);\n \tfor (i = 0; i < rev.pending.nr; i++) {\n \t\tstruct object_array_entry *entry = &rev.pending.objects[i];\n \t\tstruct object *obj = entry->item;\n@@ -394,8 +494,9 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\t\tdie(_(\"invalid object '%s' given.\"), name);\n \t\tif (obj->type == OBJ_COMMIT)\n \t\t\tobj = &get_commit_tree(((struct commit *)obj))->object;\n-\n \t\tif (obj->type == OBJ_TREE) {\n+\t\t\tif (sdiff.skip && bitmap_get(sdiff.skip, i))\n+\t\t\t\tcontinue;\n \t\t\tobj->flags |= flags;\n \t\t\tadd_object_array(obj, name, &ent);\n \t\t} else if (obj->type == OBJ_BLOB) {\n@@ -437,24 +538,22 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\tusage(builtin_diff_usage);\n \telse if (ent.nr == 1)\n \t\tresult = builtin_diff_index(&rev, argc, argv);\n-\telse if (ent.nr == 2)\n+\telse if (ent.nr == 2) {\n+\t\tif (sdiff.warn) {\n+\t\t\tconst char *lname = rev.pending.objects[sdiff.left].name;\n+\t\t\tconst char *rname = rev.pending.objects[sdiff.right].name;\n+\t\t\tconst char *basename = rev.pending.objects[sdiff.base].name;\n+\t\t\twarning(_(\"%s...%s: multiple merge bases, using %s\"),\n+\t\t\t\tlname, rname, basename);\n+\t\t}\n \t\tresult = builtin_diff_tree(&rev, argc, argv,\n \t\t\t\t\t &ent.objects[0], &ent.objects[1]);\n-\telse if (ent.objects[0].item->flags & UNINTERESTING) {\n-\t\t/*\n-\t\t * diff A...B where there is at least one merge base\n-\t\t * between A and B. We have ent.objects[0] ==\n-\t\t * merge-base, ent.objects[ents-2] == A, and\n-\t\t * ent.objects[ents-1] == B. Show diff between the\n-\t\t * base and B. Note that we pick one merge base at\n-\t\t * random if there are more than one.\n-\t\t */\n-\t\tresult = builtin_diff_tree(&rev, argc, argv,\n-\t\t\t\t\t &ent.objects[0],\n-\t\t\t\t\t &ent.objects[ent.nr-1]);\n-\t} else\n+\t} else {\n+\t\tif (sdiff.skip)\n+\t\t\tusage(builtin_diff_usage);\n \t\tresult = builtin_diff_combined(&rev, argc, argv,\n \t\t\t\t\t ent.objects, ent.nr);\n+\t}\n \tresult = diff_result_code(&rev.diffopt, result);\n \tif (1 < rev.diffopt.skip_stat_unmatch)\n \t\trefresh_index_quietly();\ndiff --git a/t/t4068-diff-symmetric.sh b/t/t4068-diff-symmetric.sh\nnew file mode 100755\nindex 00000000000..7b5988933da\n--- /dev/null\n+++ b/t/t4068-diff-symmetric.sh\n@@ -0,0 +1,81 @@\n+#!/bin/sh\n+\n+test_description='behavior of diff with symmetric-diff setups'\n+\n+. ./test-lib.sh\n+\n+# build these situations:\n+# - normal merge with one merge base (b1...b2);\n+# - criss-cross merge ie 2 merge bases (b1...master);\n+# - disjoint subgraph (orphan branch, b3...master).\n+#\n+# B---E <-- master\n+# / \\ /\n+# A X\n+# \\ / \\\n+# C---D--G <-- br1\n+# \\ /\n+# ---F <-- br2\n+#\n+# H <-- br3\n+#\n+# We put files into a few commits so that we can verify the\n+# output as well.\n+\n+test_expect_success setup '\n+\tgit commit --allow-empty -m A &&\n+\techo b >b &&\n+\tgit add b &&\n+\tgit commit -m B &&\n+\tgit checkout -b br1 HEAD^ &&\n+\techo c >c &&\n+\tgit add c &&\n+\tgit commit -m C &&\n+\tgit tag commit-C &&\n+\tgit merge -m D master &&\n+\tgit tag commit-D &&\n+\tgit checkout master &&\n+\tgit merge -m E commit-C &&\n+\tgit checkout -b br2 commit-C &&\n+\techo f >f &&\n+\tgit add f &&\n+\tgit commit -m F &&\n+\tgit checkout br1 &&\n+\tgit merge -m G br2 &&\n+\tgit checkout --orphan br3 &&\n+\tgit commit -m H\n+'\n+\n+test_expect_success 'diff with one merge base' '\n+\tgit diff commit-D...br1 >tmp &&\n+\ttail -1 tmp >actual &&\n+\techo +f >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+# The output (in tmp) can have +b or +c depending\n+# on which merge base (commit B or C) is picked.\n+# It should have one of those two, which comes out\n+# to seven lines.\n+test_expect_success 'diff with two merge bases' '\n+\tgit diff br1...master >tmp 2>err &&\n+\ttest_line_count = 7 tmp &&\n+\ttest_line_count = 1 err\n+'\n+\n+test_expect_success 'diff with no merge bases' '\n+\ttest_must_fail git diff br2...br3 >tmp 2>err &&\n+\ttest_i18ngrep \"fatal: br2...br3: no merge base\" err\n+'\n+\n+test_expect_success 'diff with too many symmetric differences' '\n+\ttest_must_fail git diff br1...master br2...br3 >tmp 2>err &&\n+\ttest_i18ngrep \"fatal: cannot use more than one symmetric difference\" err\n+'\n+\n+test_expect_success 'diff with symmetric difference and extraneous arg' '\n+\ttest_must_fail git diff master br1...master >tmp 2>err &&\n+\ttest_i18ngrep \"usage\" err\n+'\n+\n+test_done\n-- \ngitgitgadget\n\n"},{"id":"399359","messageId":"b9b4c6f113dfb03268b391cc62abfd38bd7632e4.1591729224.git.gitgitgadget@gmail.com","threadId":"53639","inReplyTo":"pull.804.v2.git.git.1591729224.gitgitgadget@gmail.com","subject":"[PATCH v2 3/3] Documentation: tweak git diff help slightly","fromName":"Chris Torek via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-06-09T19:00:23Z","receivedAt":"2020-06-09T19:00:40Z","isPatch":true,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"From: Chris Torek \n\nUpdate the manual page synopsis to include the three-dot notation\nand the combined-diff option\n\nMake \"git diff -h\" print the same usage summary as the manual\npage synopsis, minus the \"A..B\" form, which is now discouraged.\n\nDocument the usage for producing combined commits.\n\nSigned-off-by: Chris Torek \n---\n Documentation/git-diff.txt | 21 +++++++++++++++++----\n builtin/diff.c | 8 +++++++-\n 2 files changed, 24 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt\nindex 37781cf1755..0bce278652a 100644\n--- a/Documentation/git-diff.txt\n+++ b/Documentation/git-diff.txt\n@@ -11,15 +11,17 @@ SYNOPSIS\n [verse]\n 'git diff' [] [] [--] [...]\n 'git diff' [] --cached [] [--] [...]\n-'git diff' [] [--] [...]\n+'git diff' [] [...] [--] [...]\n+'git diff' [] ... [--] [...]\n 'git diff' [] \n 'git diff' [] --no-index [--] \n \n DESCRIPTION\n -----------\n Show changes between the working tree and the index or a tree, changes\n-between the index and a tree, changes between two trees, changes between\n-two blob objects, or changes between two files on disk.\n+between the index and a tree, changes between two trees, changes resulting\n+from a merge, changes between two blob objects, or changes between two\n+files on disk.\n \n 'git diff' [] [--] [...]::\n \n@@ -67,6 +69,16 @@ two blob objects, or changes between two files on disk.\n \tone side is omitted, it will have the same effect as\n \tusing HEAD instead.\n \n+'git diff' [] [...] [--] [...]::\n+\n+\tThis form is to view the results of a merge commit. The first\n+\tlisted must be the merge itself; the remaining two or\n+\tmore commits should be its parents. A convenient way to produce\n+\tthe desired set of revisions is to use the {caret}@ suffix, i.e.,\n+\t\"git diff master master^@\". This is equivalent to running \"git\n+\tshow --format=\" on the merge commit, e.g., \"git show --format=\n+\tmaster\".\n+\n 'git diff' [] \\... [--] [...]::\n \n \tThis form is to view the changes on the branch containing\n@@ -196,7 +208,8 @@ linkgit:git-difftool[1],\n linkgit:git-log[1],\n linkgit:gitdiffcore[7],\n linkgit:git-format-patch[1],\n-linkgit:git-apply[1]\n+linkgit:git-apply[1],\n+linkgit:git-show[1]\n \n GIT\n ---\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 0b6e63dbd02..b333640082b 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -24,7 +24,13 @@\n #define DIFF_NO_INDEX_IMPLICIT 2\n \n static const char builtin_diff_usage[] =\n-\"git diff [] [ []] [--] [...]\";\n+\"git diff [] [] [--] [...]\\n\"\n+\" or: git diff [] --cached [] [--] [...]\\n\"\n+\" or: git diff [] [...] [--] [...]\\n\"\n+\" or: git diff [] ...] [--] [...]\\n\"\n+\" or: git diff [] ]\\n\"\n+\" or: git diff [] --no-index [--] ]\\n\"\n+COMMON_DIFF_OPTIONS_HELP;\n \n static const char *blob_path(struct object_array_entry *entry)\n {\n-- \ngitgitgadget\n"},{"id":"399373","messageId":"xmqqftb3hmx6.fsf@gitster.c.googlers.com","threadId":"53639","inReplyTo":"100fa4034771e58b65cdac2f3dfb48531c07b735.1591729224.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/3] git diff: improve A...B merge-base handling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-09T22:52:37Z","receivedAt":"2020-06-09T22:52:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Chris Torek via GitGitGadget\" writes:\n\n> +static void symdiff_prepare(struct rev_info *rev, struct symdiff *sym)\n> +{\n> +\tint i, lcount = 0, rcount = 0, basecount = 0;\n> +\tint lpos = -1, rpos = -1, basepos = -1;\n> +\tstruct bitmap *map = NULL;\n\nThe logic around rcount and rpos in this function still smells\nfishy. For example, rcount is counted up from 0 but its value is\nnever consulted, so we should be able to get rid of it.\n\nFor that matter, lcount and lpos look somewhat redundant. lpos\nbegins with -1 to signal \"have not seen any left end of symmetric\nrange yet\", and we won't allow more than two symmetric ranges\nanyway, so we should be able to get rid of lcount and base the error\ncondition purely on lpos by\n\n * when we see SYMMETRIC_LEFT, check if lpos is already non-negative,\n and die otherwise right there. We don't need \"if lcount != 1, die\"\n after the loop.\n\n * after the loop, if lpos is still -1, we know we didn't see\n symmetric difference.\n\n> +\t/*\n> +\t * Use the whence fields to find merge bases and left and\n> +\t * right parts of symmetric difference, so that we do not\n> +\t * depend on the order that revisions are parsed. If there\n> +\t * are any revs that aren't from these sources, we have a\n> +\t * \"git diff C A...B\" or \"git diff A...B C\" case. Or we\n> +\t * could even get \"git diff A...B C...E\", for instance.\n> +\t *\n> +\t * If we don't have just one merge base, we pick one\n> +\t * at random.\n> +\t *\n> +\t * NB: REV_CMD_LEFT, REV_CMD_RIGHT are also used for A..B,\n> +\t * so we must check for SYMMETRIC_LEFT too. The two arrays\n> +\t * rev->pending.objects and rev->cmdline.rev are parallel.\n> +\t */\n> +\tfor (i = 0; i < rev->cmdline.nr; i++) {\n> +\t\tstruct object *obj = rev->pending.objects[i].item;\n> +\t\tswitch (rev->cmdline.rev[i].whence) {\n> +\t\tcase REV_CMD_MERGE_BASE:\n> +\t\t\tif (basepos < 0)\n> +\t\t\t\tbasepos = i;\n> +\t\t\tbasecount++;\n> +\t\t\tbreak;\t\t/* do mark all bases */\n> +\t\tcase REV_CMD_LEFT:\n> +\t\t\tif (obj->flags & SYMMETRIC_LEFT) {\n> +\t\t\t\tlpos = i;\n> +\t\t\t\tlcount++;\n> +\t\t\t\tbreak;\t/* do mark A */\n> +\t\t\t}\n> +\t\t\tcontinue;\n> +\t\tcase REV_CMD_RIGHT:\n> +\t\t\trpos = i;\n> +\t\t\trcount++;\n> +\t\t\tcontinue;\t/* don't mark B */\n\nIt is unclear if we want to allow \"git diff A..B C..D\" (or\nalternatively \"git diff A...B C..D\") and if so why. \n\nIt appears that you are allowing both, but I am not sure if that is\na good idea. Read a bit further below.\n\n> +\t\tdefault:\n> +\t\t\tcontinue;\n> +\t\t}\n> +\t\tif (map == NULL)\n> +\t\t\tmap = bitmap_new();\n> +\t\tbitmap_set(map, i);\n> +\t}\n> +\n> +\tif (lcount == 0) {\t/* not a symmetric difference */\n> +\t\tbitmap_free(map);\n> +\t\tsym->warn = 0;\n> +\t\tsym->skip = NULL;\n> +\t\treturn;\n> +\t}\n> +\n> +\tif (lcount != 1)\n> +\t\tdie(_(\"cannot use more than one symmetric difference\"));\n> +\n> +\tif (basecount == 0) {\n> +\t\tconst char *lname = rev->pending.objects[lpos].name;\n> +\t\tconst char *rname = rev->pending.objects[rpos].name;\n> +\t\tdie(_(\"%s...%s: no merge base\"), lname, rname);\n\nWhen \"git diff A...B C..D\" is given, what do we want to do? A is\nthe only element marked with REV_CMD_LEFT, which is pointed at by\nlpos and lcount gets incremented to become one. When we see B, we\nmake rpos point at it, but then later, when we see D, wouldn't rpos\nget updated to point at it? What error message would we give when\nwe see no merge base then? We would want to say the symdiff between\nA and B has no merge bases, but wouldn't we end up mentioning D\ninstead of B?\n\n> +\t}\n> +\tbitmap_unset(map, basepos);\t/* unmark the base we want */\n> +\tsym->base = basepos;\n> +\tsym->left = lpos;\n> +\tsym->right = rpos;\n> +\tsym->warn = basecount > 1;\n> +\tsym->skip = map;\n> +}\n\n> @@ -394,8 +494,9 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n> \t\t\tdie(_(\"invalid object '%s' given.\"), name);\n> \t\tif (obj->type == OBJ_COMMIT)\n> \t\t\tobj = &get_commit_tree(((struct commit *)obj))->object;\n> -\n\nDo not lose this blank line. It does not make a difference to the\ncompiler, but it is semantically significant to human readers.\n\n> \t\tif (obj->type == OBJ_TREE) {\n> +\t\t\tif (sdiff.skip && bitmap_get(sdiff.skip, i))\n> +\t\t\t\tcontinue;\n> \t\t\tobj->flags |= flags;\n> \t\t\tadd_object_array(obj, name, &ent);\n> \t\t} else if (obj->type == OBJ_BLOB) {\n\n> diff --git a/t/t4068-diff-symmetric.sh b/t/t4068-diff-symmetric.sh\n> new file mode 100755\n> index 00000000000..7b5988933da\n> --- /dev/null\n> +++ b/t/t4068-diff-symmetric.sh\n> @@ -0,0 +1,81 @@\n> +#!/bin/sh\n> +...\n> +test_expect_success setup '\n> +\tgit commit --allow-empty -m A &&\n> +\techo b >b &&\n> +\tgit add b &&\n> +\tgit commit -m B &&\n> +\tgit checkout -b br1 HEAD^ &&\n> +\techo c >c &&\n> +\tgit add c &&\n> +\tgit commit -m C &&\n> +\tgit tag commit-C &&\n> +\tgit merge -m D master &&\n> +\tgit tag commit-D &&\n> +\tgit checkout master &&\n> +\tgit merge -m E commit-C &&\n> +\tgit checkout -b br2 commit-C &&\n> +\techo f >f &&\n> +\tgit add f &&\n> +\tgit commit -m F &&\n> +\tgit checkout br1 &&\n> +\tgit merge -m G br2 &&\n> +\tgit checkout --orphan br3 &&\n> +\tgit commit -m H\n> +'\n> +\n> +test_expect_success 'diff with one merge base' '\n> +\tgit diff commit-D...br1 >tmp &&\n> +\ttail -1 tmp >actual &&\n\nLet's make sure we spell \"tail -n 1\" (same for \"head -1\" -> \"head -n 1\").\nI know there are a handful of existing offenders, but that does not mean\nit is OK to make things worse.\n\n> +\techo +f >expect &&\n> +\ttest_cmp expect actual\n"},{"id":"399374","messageId":"xmqqbllrhmie.fsf@gitster.c.googlers.com","threadId":"53639","inReplyTo":"b9b4c6f113dfb03268b391cc62abfd38bd7632e4.1591729224.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 3/3] Documentation: tweak git diff help slightly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-09T23:01:29Z","receivedAt":"2020-06-09T23:01:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Chris Torek via GitGitGadget\" writes:\n\n> From: Chris Torek \n>\n> Update the manual page synopsis to include the three-dot notation\n> and the combined-diff option\n\nSurely. That is \"tweak ... slightly\". Full-stop is missing here,\nby the way.\n\n> Make \"git diff -h\" print the same usage summary as the manual\n> page synopsis, minus the \"A..B\" form, which is now discouraged.\n\nGood.\n\n> Document the usage for producing combined commits.\n\nYup, that is \"while we are at it\". The new text reads well, but it\nappears that it is the more significant part of the change in this\npatch now ;-)\n\n> diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt\n> index 37781cf1755..0bce278652a 100644\n> --- a/Documentation/git-diff.txt\n> +++ b/Documentation/git-diff.txt\n> @@ -11,15 +11,17 @@ SYNOPSIS\n> [verse]\n> 'git diff' [] [] [--] [...]\n> 'git diff' [] --cached [] [--] [...]\n> -'git diff' [] [--] [...]\n> +'git diff' [] [...] [--] [...]\n> +'git diff' [] ... [--] [...]\n> 'git diff' [] \n> 'git diff' [] --no-index [--] \n> \n> DESCRIPTION\n> -----------\n> Show changes between the working tree and the index or a tree, changes\n> -between the index and a tree, changes between two trees, changes between\n> -two blob objects, or changes between two files on disk.\n> +between the index and a tree, changes between two trees, changes resulting\n> +from a merge, changes between two blob objects, or changes between two\n> +files on disk.\n> \n> 'git diff' [] [--] [...]::\n> \n> @@ -67,6 +69,16 @@ two blob objects, or changes between two files on disk.\n> \tone side is omitted, it will have the same effect as\n> \tusing HEAD instead.\n> \n> +'git diff' [] [...] [--] [...]::\n> +\n> +\tThis form is to view the results of a merge commit. The first\n> +\tlisted must be the merge itself; the remaining two or\n> +\tmore commits should be its parents. A convenient way to produce\n> +\tthe desired set of revisions is to use the {caret}@ suffix, i.e.,\n> +\t\"git diff master master^@\". This is equivalent to running \"git\n\nDon't we usually use `git diff master master^@` to mark up literal\nexamples with tt, instead of \"git diff...\" with double-quotes?\n\n> +\tshow --format=\" on the merge commit, e.g., \"git show --format=\n> +\tmaster\".\n\nLikewise.\n\nBut more importantly, I think giving the exact equivalent is much\nless important than keeping the explanation concise, simple and\nclear, and the \"empty format to omit the log part\" is distracting\n(after all, teaching how to squelch the log message part in the\n\"show\" command is not the topic of this manpage).\n\n For a merge commit `master`, this gives the same combined diff\n as `git show master` does.\n\nperhaps?\n\nThanks.\n"},{"id":"399543","messageId":"pull.804.v3.git.git.1591888511.gitgitgadget@gmail.com","threadId":"53639","inReplyTo":"pull.804.v2.git.git.1591729224.gitgitgadget@gmail.com","subject":"[PATCH v3 0/3] improve git-diff documentation and A...B handling","fromName":"Chris Torek via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-06-11T15:15:07Z","receivedAt":"2020-06-11T15:15:16Z","isPatch":true,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"git diff -h help is succinct, but perhaps too much so.\n\nThe symmetric-diff syntax, git diff A...B, is defined by the documentation\nto compare the merge base of A and B to commit B. It does so just fine when\nthere is a merge base. It compares A and B directly if there is no merge\nbase, and it is overly forgiving of bad arguments after which it can produce\nnonsensical diffs. It also behaves badly with other odd/incorrect usages,\nsuch as git diff A..B C..D.\n\nThe first patch simply adjusts a test that will fail if the second patch is\naccepted. The second patch adds special handling for the symmetric diff\nsyntax so that the option parsing works, plus a small test suite. The third\npatch updates the documentation, including adding a section for combined\ncommits, and makes the help output more verbose (to match the SYNOPSIS and\nprovide common diff options like git-diff-files, for instance).\n\nChanges since v1:\n\n * updated commit messages\n * rewrote prepare function\n * added tests to reject bad two-dot usage as well\n * removed A..B syntax from usage (and fixed typo)\n * fixed up three-dot synopsis text\n\nChris Torek (3):\n t/t3430: avoid undefined git diff behavior\n git diff: improve range handling\n Documentation: usage for diff combined commits\n\n Documentation/git-diff.txt | 20 ++++--\n builtin/diff.c | 132 +++++++++++++++++++++++++++++++++----\n t/t3430-rebase-merges.sh | 2 +-\n t/t4068-diff-symmetric.sh | 91 +++++++++++++++++++++++++\n 4 files changed, 226 insertions(+), 19 deletions(-)\n create mode 100755 t/t4068-diff-symmetric.sh\n\n\nbase-commit: 20514004ddf1a3528de8933bc32f284e175e1012\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-804%2Fchris3torek%2Fcleanup-diff-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-804/chris3torek/cleanup-diff-v3\nPull-Request: https://github.com/git/git/pull/804\n\nRange-diff vs v2:\n\n 1: 2ccaad645ff = 1: 2ccaad645ff t/t3430: avoid undefined git diff behavior\n 2: 100fa403477 ! 2: 60aed3f9d65 git diff: improve A...B merge-base handling\n @@ Metadata\n Author: Chris Torek \n \n ## Commit message ##\n - git diff: improve A...B merge-base handling\n + git diff: improve range handling\n \n When git diff is given a symmetric difference A...B, it chooses\n some merge base from the two specified commits (as documented).\n @@ Commit message\n lost. We will produce a combined diff instead.\n \n Similar weirdness occurs if you use, e.g., \"git diff C A...B D\".\n + Likewise, using multiple two-dot ranges, or tossing extra\n + revision specifiers into the command line with two-dot ranges,\n + or mixing two and three dot ranges, all produce nonsense.\n \n - To avoid all this, add a routine to catch the A...B case and verify that\n - there is at least one merge base, and that the arguments make sense.\n - As a side effect, produce a warning showing *which* merge base is being\n - used when there are multiple choices; die if there is no merge base.\n + To avoid all this, add a routine to catch the range cases and\n + verify that that the arguments make sense. As a side effect,\n + produce a warning showing *which* merge base is being used when\n + there are multiple choices; die if there is no merge base.\n \n Signed-off-by: Chris Torek \n \n @@ builtin/diff.c: static int builtin_diff_files(struct rev_info *revs, int argc, c\n }\n \n +struct symdiff {\n -+\tstruct bitmap *skip;\t/* bitmap of commit indices to skip, or NULL */\n -+\tint warn;\t\t/* true if there were multiple merge bases */\n -+\tint base, left, right;\t/* index of chosen merge base and left&right */\n ++\tstruct bitmap *skip;\n ++\tint warn;\n ++\tconst char *base, *left, *right;\n +};\n +\n +/*\n + * Check for symmetric-difference arguments, and if present, arrange\n -+ * everything we need to know to handle them correctly.\n ++ * everything we need to know to handle them correctly. As a bonus,\n ++ * weed out all bogus range-based revision specifications, e.g.,\n ++ * \"git diff A..B C..D\" or \"git diff A..B C\" get rejected.\n + *\n + * For an actual symmetric diff, *symdiff is set this way:\n + *\n @@ builtin/diff.c: static int builtin_diff_files(struct rev_info *revs, int argc, c\n + * which there might be many, and A in A...B). Note that the\n + * chosen merge base and right side are NOT marked.\n + * - warn is set if there are multiple merge bases.\n -+ * - base, left, and right hold the merge base and left and\n -+ * right side indices, for warnings or errors.\n ++ * - base, left, and right point to the names to use in a\n ++ * warning about multiple merge bases.\n + *\n + * If there is no symmetric diff argument, sym->skip is NULL and\n + * sym->warn is cleared. The remaining fields are not set.\n -+ *\n -+ * If the user provides a symmetric diff with no merge base, or\n -+ * more than one range, we do a usage-exit.\n + */\n +static void symdiff_prepare(struct rev_info *rev, struct symdiff *sym)\n +{\n -+\tint i, lcount = 0, rcount = 0, basecount = 0;\n ++\tint i, is_symdiff = 0, basecount = 0, othercount = 0;\n +\tint lpos = -1, rpos = -1, basepos = -1;\n +\tstruct bitmap *map = NULL;\n +\n @@ builtin/diff.c: static int builtin_diff_files(struct rev_info *revs, int argc, c\n +\t\t\tbasecount++;\n +\t\t\tbreak;\t\t/* do mark all bases */\n +\t\tcase REV_CMD_LEFT:\n ++\t\t\tif (lpos > 0)\n ++\t\t\t\tusage(builtin_diff_usage);\n ++\t\t\tlpos = i;\n +\t\t\tif (obj->flags & SYMMETRIC_LEFT) {\n -+\t\t\t\tlpos = i;\n -+\t\t\t\tlcount++;\n ++\t\t\t\tis_symdiff = 1;\n +\t\t\t\tbreak;\t/* do mark A */\n +\t\t\t}\n +\t\t\tcontinue;\n +\t\tcase REV_CMD_RIGHT:\n ++\t\t\tif (rpos > 0)\n ++\t\t\t\tusage(builtin_diff_usage);\n +\t\t\trpos = i;\n -+\t\t\trcount++;\n +\t\t\tcontinue;\t/* don't mark B */\n -+\t\tdefault:\n ++\t\tcase REV_CMD_PARENTS_ONLY:\n ++\t\tcase REV_CMD_REF:\n ++\t\tcase REV_CMD_REV:\n ++\t\t\tothercount++;\n +\t\t\tcontinue;\n +\t\t}\n +\t\tif (map == NULL)\n @@ builtin/diff.c: static int builtin_diff_files(struct rev_info *revs, int argc, c\n +\t\tbitmap_set(map, i);\n +\t}\n +\n -+\tif (lcount == 0) {\t/* not a symmetric difference */\n ++\t/*\n ++\t * Forbid any additional revs for both A...B and A..B.\n ++\t */\n ++\tif (lpos >= 0 && othercount > 0)\n ++\t\tusage(builtin_diff_usage);\n ++\n ++\tif (!is_symdiff) {\n +\t\tbitmap_free(map);\n +\t\tsym->warn = 0;\n +\t\tsym->skip = NULL;\n +\t\treturn;\n +\t}\n +\n -+\tif (lcount != 1)\n -+\t\tdie(_(\"cannot use more than one symmetric difference\"));\n -+\n -+\tif (basecount == 0) {\n -+\t\tconst char *lname = rev->pending.objects[lpos].name;\n -+\t\tconst char *rname = rev->pending.objects[rpos].name;\n -+\t\tdie(_(\"%s...%s: no merge base\"), lname, rname);\n -+\t}\n ++\tsym->left = rev->pending.objects[lpos].name;\n ++\tsym->right = rev->pending.objects[rpos].name;\n ++\tsym->base = rev->pending.objects[basepos].name;\n ++\tif (basecount == 0)\n ++\t\tdie(_(\"%s...%s: no merge base\"), sym->left, sym->right);\n +\tbitmap_unset(map, basepos);\t/* unmark the base we want */\n -+\tsym->base = basepos;\n -+\tsym->left = lpos;\n -+\tsym->right = rpos;\n +\tsym->warn = basecount > 1;\n +\tsym->skip = map;\n +}\n @@ builtin/diff.c: int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\tstruct object_array_entry *entry = &rev.pending.objects[i];\n \t\tstruct object *obj = entry->item;\n @@ builtin/diff.c: int cmd_diff(int argc, const char **argv, const char *prefix)\n - \t\t\tdie(_(\"invalid object '%s' given.\"), name);\n - \t\tif (obj->type == OBJ_COMMIT)\n \t\t\tobj = &get_commit_tree(((struct commit *)obj))->object;\n --\n + \n \t\tif (obj->type == OBJ_TREE) {\n +\t\t\tif (sdiff.skip && bitmap_get(sdiff.skip, i))\n +\t\t\t\tcontinue;\n @@ builtin/diff.c: int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\tresult = builtin_diff_index(&rev, argc, argv);\n -\telse if (ent.nr == 2)\n +\telse if (ent.nr == 2) {\n -+\t\tif (sdiff.warn) {\n -+\t\t\tconst char *lname = rev.pending.objects[sdiff.left].name;\n -+\t\t\tconst char *rname = rev.pending.objects[sdiff.right].name;\n -+\t\t\tconst char *basename = rev.pending.objects[sdiff.base].name;\n ++\t\tif (sdiff.warn)\n +\t\t\twarning(_(\"%s...%s: multiple merge bases, using %s\"),\n -+\t\t\t\tlname, rname, basename);\n -+\t\t}\n ++\t\t\t\tsdiff.left, sdiff.right, sdiff.base);\n \t\tresult = builtin_diff_tree(&rev, argc, argv,\n \t\t\t\t\t &ent.objects[0], &ent.objects[1]);\n -\telse if (ent.objects[0].item->flags & UNINTERESTING) {\n @@ builtin/diff.c: int cmd_diff(int argc, const char **argv, const char *prefix)\n -\t\tresult = builtin_diff_tree(&rev, argc, argv,\n -\t\t\t\t\t &ent.objects[0],\n -\t\t\t\t\t &ent.objects[ent.nr-1]);\n --\t} else\n -+\t} else {\n -+\t\tif (sdiff.skip)\n -+\t\t\tusage(builtin_diff_usage);\n + \t} else\n \t\tresult = builtin_diff_combined(&rev, argc, argv,\n \t\t\t\t\t ent.objects, ent.nr);\n -+\t}\n - \tresult = diff_result_code(&rev.diffopt, result);\n - \tif (1 < rev.diffopt.skip_stat_unmatch)\n - \t\trefresh_index_quietly();\n \n ## t/t4068-diff-symmetric.sh (new) ##\n @@\n @@ t/t4068-diff-symmetric.sh (new)\n +\n +test_expect_success 'diff with one merge base' '\n +\tgit diff commit-D...br1 >tmp &&\n -+\ttail -1 tmp >actual &&\n ++\ttail -n 1 tmp >actual &&\n +\techo +f >expect &&\n +\ttest_cmp expect actual\n +'\n @@ t/t4068-diff-symmetric.sh (new)\n +\n +test_expect_success 'diff with too many symmetric differences' '\n +\ttest_must_fail git diff br1...master br2...br3 >tmp 2>err &&\n -+\ttest_i18ngrep \"fatal: cannot use more than one symmetric difference\" err\n ++\ttest_i18ngrep \"usage\" err\n +'\n +\n +test_expect_success 'diff with symmetric difference and extraneous arg' '\n @@ t/t4068-diff-symmetric.sh (new)\n +\ttest_i18ngrep \"usage\" err\n +'\n +\n ++test_expect_success 'diff with two ranges' '\n ++\ttest_must_fail git diff master br1..master br2..br3 >tmp 2>err &&\n ++\ttest_i18ngrep \"usage\" err\n ++'\n ++\n ++test_expect_success 'diff with ranges and extra arg' '\n ++\ttest_must_fail git diff master br1..master commit-D >tmp 2>err &&\n ++\ttest_i18ngrep \"usage\" err\n ++'\n ++\n +test_done\n 3: b9b4c6f113d ! 3: a7da92cd635 Documentation: tweak git diff help slightly\n @@ Metadata\n Author: Chris Torek \n \n ## Commit message ##\n - Documentation: tweak git diff help slightly\n + Documentation: usage for diff combined commits\n \n - Update the manual page synopsis to include the three-dot notation\n - and the combined-diff option\n + Document the usage for producing combined commits with \"git diff\".\n + This includes updating the synopsis section.\n +\n + While here, add the three-dot notation to the synopsis.\n \n Make \"git diff -h\" print the same usage summary as the manual\n page synopsis, minus the \"A..B\" form, which is now discouraged.\n \n - Document the usage for producing combined commits.\n -\n Signed-off-by: Chris Torek \n \n ## Documentation/git-diff.txt ##\n @@ Documentation/git-diff.txt: two blob objects, or changes between two files on di\n +\tThis form is to view the results of a merge commit. The first\n +\tlisted must be the merge itself; the remaining two or\n +\tmore commits should be its parents. A convenient way to produce\n -+\tthe desired set of revisions is to use the {caret}@ suffix, i.e.,\n -+\t\"git diff master master^@\". This is equivalent to running \"git\n -+\tshow --format=\" on the merge commit, e.g., \"git show --format=\n -+\tmaster\".\n ++\tthe desired set of revisions is to use the {caret}@ suffix.\n ++\tFor instance, if `master` names a merge commit, `git diff master\n ++\tmaster^@` gives the same combined diff as `git show master`.\n +\n 'git diff' [] \\... [--] [...]::\n \n\n-- \ngitgitgadget\n"},{"id":"399544","messageId":"2ccaad645ff01b786e76dc63210d75da633389a6.1591888511.git.gitgitgadget@gmail.com","threadId":"53639","inReplyTo":"pull.804.v3.git.git.1591888511.gitgitgadget@gmail.com","subject":"[PATCH v3 1/3] t/t3430: avoid undefined git diff behavior","fromName":"Chris Torek via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-06-11T15:15:08Z","receivedAt":"2020-06-11T15:15:19Z","isPatch":true,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"From: Chris Torek \n\nThe autosquash-and-exec test used \"git diff HEAD^!\" to mean\n\"git diff HEAD^ HEAD\". Use these directly instead of relying\non the undefined but actual-current behavior of \"HEAD^!\".\n\nSigned-off-by: Chris Torek \n---\n t/t3430-rebase-merges.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh\nindex a1bc3e20016..b454f400ebd 100755\n--- a/t/t3430-rebase-merges.sh\n+++ b/t/t3430-rebase-merges.sh\n@@ -420,7 +420,7 @@ test_expect_success 'with --autosquash and --exec' '\n \tgit commit --fixup B B.t &&\n \twrite_script show.sh <<-\\EOF &&\n \tsubject=\"$(git show -s --format=%s HEAD)\"\n-\tcontent=\"$(git diff HEAD^! | tail -n 1)\"\n+\tcontent=\"$(git diff HEAD^ HEAD | tail -n 1)\"\n \techo \"$subject: $content\"\n \tEOF\n \ttest_tick &&\n-- \ngitgitgadget\n\n"},{"id":"399545","messageId":"60aed3f9d6543a5f66ff75a50c86cf626ec04ed4.1591888511.git.gitgitgadget@gmail.com","threadId":"53639","inReplyTo":"pull.804.v3.git.git.1591888511.gitgitgadget@gmail.com","subject":"[PATCH v3 2/3] git diff: improve range handling","fromName":"Chris Torek via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-06-11T15:15:09Z","receivedAt":"2020-06-11T15:15:20Z","isPatch":true,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"From: Chris Torek \n\nWhen git diff is given a symmetric difference A...B, it chooses\nsome merge base from the two specified commits (as documented).\n\nThis fails, however, if there is *no* merge base: instead, you\nsee the differences between A and B, which is certainly not what\nis expected.\n\nMoreover, if additional revisions are specified on the command\nline (\"git diff A...B C\"), the results get a bit weird:\n\n * If there is a symmetric difference merge base, this is used\n as the left side of the diff. The last final ref is used as\n the right side.\n * If there is no merge base, the symmetric status is completely\n lost. We will produce a combined diff instead.\n\nSimilar weirdness occurs if you use, e.g., \"git diff C A...B D\".\nLikewise, using multiple two-dot ranges, or tossing extra\nrevision specifiers into the command line with two-dot ranges,\nor mixing two and three dot ranges, all produce nonsense.\n\nTo avoid all this, add a routine to catch the range cases and\nverify that that the arguments make sense. As a side effect,\nproduce a warning showing *which* merge base is being used when\nthere are multiple choices; die if there is no merge base.\n\nSigned-off-by: Chris Torek \n---\n builtin/diff.c | 124 ++++++++++++++++++++++++++++++++++----\n t/t4068-diff-symmetric.sh | 91 ++++++++++++++++++++++++++++\n 2 files changed, 202 insertions(+), 13 deletions(-)\n create mode 100755 t/t4068-diff-symmetric.sh\n\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 8537b17bd5e..3d33a9231ae 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -6,6 +6,7 @@\n #define USE_THE_INDEX_COMPATIBILITY_MACROS\n #include \"cache.h\"\n #include \"config.h\"\n+#include \"ewah/ewok.h\"\n #include \"lockfile.h\"\n #include \"color.h\"\n #include \"commit.h\"\n@@ -254,6 +255,108 @@ static int builtin_diff_files(struct rev_info *revs, int argc, const char **argv\n \treturn run_diff_files(revs, options);\n }\n \n+struct symdiff {\n+\tstruct bitmap *skip;\n+\tint warn;\n+\tconst char *base, *left, *right;\n+};\n+\n+/*\n+ * Check for symmetric-difference arguments, and if present, arrange\n+ * everything we need to know to handle them correctly. As a bonus,\n+ * weed out all bogus range-based revision specifications, e.g.,\n+ * \"git diff A..B C..D\" or \"git diff A..B C\" get rejected.\n+ *\n+ * For an actual symmetric diff, *symdiff is set this way:\n+ *\n+ * - its skip is non-NULL and marks *all* rev->pending.objects[i]\n+ * indices that the caller should ignore (extra merge bases, of\n+ * which there might be many, and A in A...B). Note that the\n+ * chosen merge base and right side are NOT marked.\n+ * - warn is set if there are multiple merge bases.\n+ * - base, left, and right point to the names to use in a\n+ * warning about multiple merge bases.\n+ *\n+ * If there is no symmetric diff argument, sym->skip is NULL and\n+ * sym->warn is cleared. The remaining fields are not set.\n+ */\n+static void symdiff_prepare(struct rev_info *rev, struct symdiff *sym)\n+{\n+\tint i, is_symdiff = 0, basecount = 0, othercount = 0;\n+\tint lpos = -1, rpos = -1, basepos = -1;\n+\tstruct bitmap *map = NULL;\n+\n+\t/*\n+\t * Use the whence fields to find merge bases and left and\n+\t * right parts of symmetric difference, so that we do not\n+\t * depend on the order that revisions are parsed. If there\n+\t * are any revs that aren't from these sources, we have a\n+\t * \"git diff C A...B\" or \"git diff A...B C\" case. Or we\n+\t * could even get \"git diff A...B C...E\", for instance.\n+\t *\n+\t * If we don't have just one merge base, we pick one\n+\t * at random.\n+\t *\n+\t * NB: REV_CMD_LEFT, REV_CMD_RIGHT are also used for A..B,\n+\t * so we must check for SYMMETRIC_LEFT too. The two arrays\n+\t * rev->pending.objects and rev->cmdline.rev are parallel.\n+\t */\n+\tfor (i = 0; i < rev->cmdline.nr; i++) {\n+\t\tstruct object *obj = rev->pending.objects[i].item;\n+\t\tswitch (rev->cmdline.rev[i].whence) {\n+\t\tcase REV_CMD_MERGE_BASE:\n+\t\t\tif (basepos < 0)\n+\t\t\t\tbasepos = i;\n+\t\t\tbasecount++;\n+\t\t\tbreak;\t\t/* do mark all bases */\n+\t\tcase REV_CMD_LEFT:\n+\t\t\tif (lpos > 0)\n+\t\t\t\tusage(builtin_diff_usage);\n+\t\t\tlpos = i;\n+\t\t\tif (obj->flags & SYMMETRIC_LEFT) {\n+\t\t\t\tis_symdiff = 1;\n+\t\t\t\tbreak;\t/* do mark A */\n+\t\t\t}\n+\t\t\tcontinue;\n+\t\tcase REV_CMD_RIGHT:\n+\t\t\tif (rpos > 0)\n+\t\t\t\tusage(builtin_diff_usage);\n+\t\t\trpos = i;\n+\t\t\tcontinue;\t/* don't mark B */\n+\t\tcase REV_CMD_PARENTS_ONLY:\n+\t\tcase REV_CMD_REF:\n+\t\tcase REV_CMD_REV:\n+\t\t\tothercount++;\n+\t\t\tcontinue;\n+\t\t}\n+\t\tif (map == NULL)\n+\t\t\tmap = bitmap_new();\n+\t\tbitmap_set(map, i);\n+\t}\n+\n+\t/*\n+\t * Forbid any additional revs for both A...B and A..B.\n+\t */\n+\tif (lpos >= 0 && othercount > 0)\n+\t\tusage(builtin_diff_usage);\n+\n+\tif (!is_symdiff) {\n+\t\tbitmap_free(map);\n+\t\tsym->warn = 0;\n+\t\tsym->skip = NULL;\n+\t\treturn;\n+\t}\n+\n+\tsym->left = rev->pending.objects[lpos].name;\n+\tsym->right = rev->pending.objects[rpos].name;\n+\tsym->base = rev->pending.objects[basepos].name;\n+\tif (basecount == 0)\n+\t\tdie(_(\"%s...%s: no merge base\"), sym->left, sym->right);\n+\tbitmap_unset(map, basepos);\t/* unmark the base we want */\n+\tsym->warn = basecount > 1;\n+\tsym->skip = map;\n+}\n+\n int cmd_diff(int argc, const char **argv, const char *prefix)\n {\n \tint i;\n@@ -263,6 +366,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \tstruct object_array_entry *blob[2];\n \tint nongit = 0, no_index = 0;\n \tint result = 0;\n+\tstruct symdiff sdiff;\n \n \t/*\n \t * We could get N tree-ish in the rev.pending_objects list.\n@@ -382,6 +486,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n+\tsymdiff_prepare(&rev, &sdiff);\n \tfor (i = 0; i < rev.pending.nr; i++) {\n \t\tstruct object_array_entry *entry = &rev.pending.objects[i];\n \t\tstruct object *obj = entry->item;\n@@ -396,6 +501,8 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\t\tobj = &get_commit_tree(((struct commit *)obj))->object;\n \n \t\tif (obj->type == OBJ_TREE) {\n+\t\t\tif (sdiff.skip && bitmap_get(sdiff.skip, i))\n+\t\t\t\tcontinue;\n \t\t\tobj->flags |= flags;\n \t\t\tadd_object_array(obj, name, &ent);\n \t\t} else if (obj->type == OBJ_BLOB) {\n@@ -437,21 +544,12 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\tusage(builtin_diff_usage);\n \telse if (ent.nr == 1)\n \t\tresult = builtin_diff_index(&rev, argc, argv);\n-\telse if (ent.nr == 2)\n+\telse if (ent.nr == 2) {\n+\t\tif (sdiff.warn)\n+\t\t\twarning(_(\"%s...%s: multiple merge bases, using %s\"),\n+\t\t\t\tsdiff.left, sdiff.right, sdiff.base);\n \t\tresult = builtin_diff_tree(&rev, argc, argv,\n \t\t\t\t\t &ent.objects[0], &ent.objects[1]);\n-\telse if (ent.objects[0].item->flags & UNINTERESTING) {\n-\t\t/*\n-\t\t * diff A...B where there is at least one merge base\n-\t\t * between A and B. We have ent.objects[0] ==\n-\t\t * merge-base, ent.objects[ents-2] == A, and\n-\t\t * ent.objects[ents-1] == B. Show diff between the\n-\t\t * base and B. Note that we pick one merge base at\n-\t\t * random if there are more than one.\n-\t\t */\n-\t\tresult = builtin_diff_tree(&rev, argc, argv,\n-\t\t\t\t\t &ent.objects[0],\n-\t\t\t\t\t &ent.objects[ent.nr-1]);\n \t} else\n \t\tresult = builtin_diff_combined(&rev, argc, argv,\n \t\t\t\t\t ent.objects, ent.nr);\ndiff --git a/t/t4068-diff-symmetric.sh b/t/t4068-diff-symmetric.sh\nnew file mode 100755\nindex 00000000000..dd60da0294e\n--- /dev/null\n+++ b/t/t4068-diff-symmetric.sh\n@@ -0,0 +1,91 @@\n+#!/bin/sh\n+\n+test_description='behavior of diff with symmetric-diff setups'\n+\n+. ./test-lib.sh\n+\n+# build these situations:\n+# - normal merge with one merge base (b1...b2);\n+# - criss-cross merge ie 2 merge bases (b1...master);\n+# - disjoint subgraph (orphan branch, b3...master).\n+#\n+# B---E <-- master\n+# / \\ /\n+# A X\n+# \\ / \\\n+# C---D--G <-- br1\n+# \\ /\n+# ---F <-- br2\n+#\n+# H <-- br3\n+#\n+# We put files into a few commits so that we can verify the\n+# output as well.\n+\n+test_expect_success setup '\n+\tgit commit --allow-empty -m A &&\n+\techo b >b &&\n+\tgit add b &&\n+\tgit commit -m B &&\n+\tgit checkout -b br1 HEAD^ &&\n+\techo c >c &&\n+\tgit add c &&\n+\tgit commit -m C &&\n+\tgit tag commit-C &&\n+\tgit merge -m D master &&\n+\tgit tag commit-D &&\n+\tgit checkout master &&\n+\tgit merge -m E commit-C &&\n+\tgit checkout -b br2 commit-C &&\n+\techo f >f &&\n+\tgit add f &&\n+\tgit commit -m F &&\n+\tgit checkout br1 &&\n+\tgit merge -m G br2 &&\n+\tgit checkout --orphan br3 &&\n+\tgit commit -m H\n+'\n+\n+test_expect_success 'diff with one merge base' '\n+\tgit diff commit-D...br1 >tmp &&\n+\ttail -n 1 tmp >actual &&\n+\techo +f >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+# The output (in tmp) can have +b or +c depending\n+# on which merge base (commit B or C) is picked.\n+# It should have one of those two, which comes out\n+# to seven lines.\n+test_expect_success 'diff with two merge bases' '\n+\tgit diff br1...master >tmp 2>err &&\n+\ttest_line_count = 7 tmp &&\n+\ttest_line_count = 1 err\n+'\n+\n+test_expect_success 'diff with no merge bases' '\n+\ttest_must_fail git diff br2...br3 >tmp 2>err &&\n+\ttest_i18ngrep \"fatal: br2...br3: no merge base\" err\n+'\n+\n+test_expect_success 'diff with too many symmetric differences' '\n+\ttest_must_fail git diff br1...master br2...br3 >tmp 2>err &&\n+\ttest_i18ngrep \"usage\" err\n+'\n+\n+test_expect_success 'diff with symmetric difference and extraneous arg' '\n+\ttest_must_fail git diff master br1...master >tmp 2>err &&\n+\ttest_i18ngrep \"usage\" err\n+'\n+\n+test_expect_success 'diff with two ranges' '\n+\ttest_must_fail git diff master br1..master br2..br3 >tmp 2>err &&\n+\ttest_i18ngrep \"usage\" err\n+'\n+\n+test_expect_success 'diff with ranges and extra arg' '\n+\ttest_must_fail git diff master br1..master commit-D >tmp 2>err &&\n+\ttest_i18ngrep \"usage\" err\n+'\n+\n+test_done\n-- \ngitgitgadget\n\n"},{"id":"399546","messageId":"a7da92cd63582ec9405a529ee4ecbe245f18a4e1.1591888511.git.gitgitgadget@gmail.com","threadId":"53639","inReplyTo":"pull.804.v3.git.git.1591888511.gitgitgadget@gmail.com","subject":"[PATCH v3 3/3] Documentation: usage for diff combined commits","fromName":"Chris Torek via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-06-11T15:15:10Z","receivedAt":"2020-06-11T15:15:21Z","isPatch":true,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"From: Chris Torek \n\nDocument the usage for producing combined commits with \"git diff\".\nThis includes updating the synopsis section.\n\nWhile here, add the three-dot notation to the synopsis.\n\nMake \"git diff -h\" print the same usage summary as the manual\npage synopsis, minus the \"A..B\" form, which is now discouraged.\n\nSigned-off-by: Chris Torek \n---\n Documentation/git-diff.txt | 20 ++++++++++++++++----\n builtin/diff.c | 8 +++++++-\n 2 files changed, 23 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt\nindex 37781cf1755..1018110ddc2 100644\n--- a/Documentation/git-diff.txt\n+++ b/Documentation/git-diff.txt\n@@ -11,15 +11,17 @@ SYNOPSIS\n [verse]\n 'git diff' [] [] [--] [...]\n 'git diff' [] --cached [] [--] [...]\n-'git diff' [] [--] [...]\n+'git diff' [] [...] [--] [...]\n+'git diff' [] ... [--] [...]\n 'git diff' [] \n 'git diff' [] --no-index [--] \n \n DESCRIPTION\n -----------\n Show changes between the working tree and the index or a tree, changes\n-between the index and a tree, changes between two trees, changes between\n-two blob objects, or changes between two files on disk.\n+between the index and a tree, changes between two trees, changes resulting\n+from a merge, changes between two blob objects, or changes between two\n+files on disk.\n \n 'git diff' [] [--] [...]::\n \n@@ -67,6 +69,15 @@ two blob objects, or changes between two files on disk.\n \tone side is omitted, it will have the same effect as\n \tusing HEAD instead.\n \n+'git diff' [] [...] [--] [...]::\n+\n+\tThis form is to view the results of a merge commit. The first\n+\tlisted must be the merge itself; the remaining two or\n+\tmore commits should be its parents. A convenient way to produce\n+\tthe desired set of revisions is to use the {caret}@ suffix.\n+\tFor instance, if `master` names a merge commit, `git diff master\n+\tmaster^@` gives the same combined diff as `git show master`.\n+\n 'git diff' [] \\... [--] [...]::\n \n \tThis form is to view the changes on the branch containing\n@@ -196,7 +207,8 @@ linkgit:git-difftool[1],\n linkgit:git-log[1],\n linkgit:gitdiffcore[7],\n linkgit:git-format-patch[1],\n-linkgit:git-apply[1]\n+linkgit:git-apply[1],\n+linkgit:git-show[1]\n \n GIT\n ---\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 3d33a9231ae..c8a999f8089 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -24,7 +24,13 @@\n #define DIFF_NO_INDEX_IMPLICIT 2\n \n static const char builtin_diff_usage[] =\n-\"git diff [] [ []] [--] [...]\";\n+\"git diff [] [] [--] [...]\\n\"\n+\" or: git diff [] --cached [] [--] [...]\\n\"\n+\" or: git diff [] [...] [--] [...]\\n\"\n+\" or: git diff [] ...] [--] [...]\\n\"\n+\" or: git diff [] ]\\n\"\n+\" or: git diff [] --no-index [--] ]\\n\"\n+COMMON_DIFF_OPTIONS_HELP;\n \n static const char *blob_path(struct object_array_entry *entry)\n {\n-- \ngitgitgadget\n"},{"id":"399549","messageId":"CAPx1GvcvDCoOHrSOgybDpKawMTPSHs2FUq6-sWVOmwAS_GRKzA@mail.gmail.com","threadId":"53639","inReplyTo":"60aed3f9d6543a5f66ff75a50c86cf626ec04ed4.1591888511.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 2/3] git diff: improve range handling","fromName":"Chris Torek","fromEmail":"chris.torek@gmail.com","sentAt":"2020-06-11T15:51:05Z","receivedAt":"2020-06-11T15:51:19Z","isPatch":true,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"On Thu, Jun 11, 2020 at 8:15 AM Chris Torek via GitGitGadget\n wrote:\n > + if (lpos > 0)\n\nUgh, this and the rpos test are supposed to be >= not >. Will fix these for v4.\nOtherwise seems review-able.\n\nAlso, I forgot to update the GitGitGadget \"changes since\",\nIt's changes since v2, not since v1, of course.\n\nChris\n"},{"id":"399587","messageId":"6eadaa89-fde7-4224-dcb9-ceef315942f2@iee.email","threadId":"53639","inReplyTo":"f7c8f094e02406a7d0cb0c61f880e5b01fa413c4.1591661021.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/3] git diff: improve A...B merge-base handling","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.email","sentAt":"2020-06-12T13:38:43Z","receivedAt":"2020-06-12T13:38:47Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"On 09/06/2020 01:03, Chris Torek via GitGitGadget wrote:\n[snip]\n> +test_description='behavior of diff with symmetric-diff setups'\n> +\n> +. ./test-lib.sh\n> +\n> +# build these situations:\n> +# - normal merge with one merge base (b1...b2);\n> +# - criss-cross merge ie 2 merge bases (b1...master);\n> +# - disjoint subgraph (orphan branch, b3...master).\n\nnit:\nUse of b1, b2, b3 here, but br1, br2, br3 below\n> +#\n> +# B---E <-- master\n> +# / \\ /\n> +# A X\n> +# \\ / \\\n> +# C---D--G <-- br1\n> +# \\ /\n> +# ---F <-- br2\n> +#\n> +# H <-- br3\n> +#\nPhilip\n"},{"id":"399603","messageId":"2ccaad645ff01b786e76dc63210d75da633389a6.1591978801.git.gitgitgadget@gmail.com","threadId":"53639","inReplyTo":"pull.804.v4.git.git.1591978801.gitgitgadget@gmail.com","subject":"[PATCH v4 1/3] t/t3430: avoid undefined git diff behavior","fromName":"Chris Torek via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-06-12T16:19:58Z","receivedAt":"2020-06-12T16:20:07Z","isPatch":true,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"From: Chris Torek \n\nThe autosquash-and-exec test used \"git diff HEAD^!\" to mean\n\"git diff HEAD^ HEAD\". Use these directly instead of relying\non the undefined but actual-current behavior of \"HEAD^!\".\n\nSigned-off-by: Chris Torek \n---\n t/t3430-rebase-merges.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh\nindex a1bc3e20016..b454f400ebd 100755\n--- a/t/t3430-rebase-merges.sh\n+++ b/t/t3430-rebase-merges.sh\n@@ -420,7 +420,7 @@ test_expect_success 'with --autosquash and --exec' '\n \tgit commit --fixup B B.t &&\n \twrite_script show.sh <<-\\EOF &&\n \tsubject=\"$(git show -s --format=%s HEAD)\"\n-\tcontent=\"$(git diff HEAD^! | tail -n 1)\"\n+\tcontent=\"$(git diff HEAD^ HEAD | tail -n 1)\"\n \techo \"$subject: $content\"\n \tEOF\n \ttest_tick &&\n-- \ngitgitgadget\n\n"},{"id":"399604","messageId":"pull.804.v4.git.git.1591978801.gitgitgadget@gmail.com","threadId":"53639","inReplyTo":"pull.804.v3.git.git.1591888511.gitgitgadget@gmail.com","subject":"[PATCH v4 0/3] improve git-diff documentation and A...B handling","fromName":"Chris Torek via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-06-12T16:19:57Z","receivedAt":"2020-06-12T16:20:08Z","isPatch":true,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"git diff -h help is succinct, but perhaps too much so.\n\nThe symmetric-diff syntax, git diff A...B, is defined by the documentation\nto compare the merge base of A and B to commit B. It does so just fine when\nthere is a merge base. It compares A and B directly if there is no merge\nbase, and it is overly forgiving of bad arguments after which it can produce\nnonsensical diffs. It also behaves badly with other odd/incorrect usages,\nsuch as git diff A...B C..D.\n\nThe first patch simply adjusts a test that will fail if the second patch is\naccepted. The second patch adds special handling for the symmetric and range\ndiff syntax so that the option parsing works, plus a small test suite. The\nthird patch updates the documentation, including adding a section for\ncombined commits, and makes the help output more verbose (to match the\nSYNOPSIS and provide common diff options like git-diff-files, for instance).\n\nChanges since v3:\n\n * correct > / >= goof\n * fix test nit per Philip Oakley\n\nChris Torek (3):\n t/t3430: avoid undefined git diff behavior\n git diff: improve range handling\n Documentation: usage for diff combined commits\n\n Documentation/git-diff.txt | 20 ++++--\n builtin/diff.c | 132 +++++++++++++++++++++++++++++++++----\n t/t3430-rebase-merges.sh | 2 +-\n t/t4068-diff-symmetric.sh | 91 +++++++++++++++++++++++++\n 4 files changed, 226 insertions(+), 19 deletions(-)\n create mode 100755 t/t4068-diff-symmetric.sh\n\n\nbase-commit: 20514004ddf1a3528de8933bc32f284e175e1012\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-804%2Fchris3torek%2Fcleanup-diff-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-804/chris3torek/cleanup-diff-v4\nPull-Request: https://github.com/git/git/pull/804\n\nRange-diff vs v3:\n\n 1: 2ccaad645ff = 1: 2ccaad645ff t/t3430: avoid undefined git diff behavior\n 2: 60aed3f9d65 ! 2: 4fa6fba33b3 git diff: improve range handling\n @@ builtin/diff.c: static int builtin_diff_files(struct rev_info *revs, int argc, c\n +\t\t\tbasecount++;\n +\t\t\tbreak;\t\t/* do mark all bases */\n +\t\tcase REV_CMD_LEFT:\n -+\t\t\tif (lpos > 0)\n ++\t\t\tif (lpos >= 0)\n +\t\t\t\tusage(builtin_diff_usage);\n +\t\t\tlpos = i;\n +\t\t\tif (obj->flags & SYMMETRIC_LEFT) {\n @@ builtin/diff.c: static int builtin_diff_files(struct rev_info *revs, int argc, c\n +\t\t\t}\n +\t\t\tcontinue;\n +\t\tcase REV_CMD_RIGHT:\n -+\t\t\tif (rpos > 0)\n ++\t\t\tif (rpos >= 0)\n +\t\t\t\tusage(builtin_diff_usage);\n +\t\t\trpos = i;\n +\t\t\tcontinue;\t/* don't mark B */\n @@ t/t4068-diff-symmetric.sh (new)\n +. ./test-lib.sh\n +\n +# build these situations:\n -+# - normal merge with one merge base (b1...b2);\n -+# - criss-cross merge ie 2 merge bases (b1...master);\n -+# - disjoint subgraph (orphan branch, b3...master).\n ++# - normal merge with one merge base (br1...b2r);\n ++# - criss-cross merge ie 2 merge bases (br1...master);\n ++# - disjoint subgraph (orphan branch, br3...master).\n +#\n +# B---E <-- master\n +# / \\ /\n 3: a7da92cd635 = 3: d7bc9aca44b Documentation: usage for diff combined commits\n\n-- \ngitgitgadget\n"},{"id":"399605","messageId":"4fa6fba33b329ce85f68470fb2545adf1bb06900.1591978801.git.gitgitgadget@gmail.com","threadId":"53639","inReplyTo":"pull.804.v4.git.git.1591978801.gitgitgadget@gmail.com","subject":"[PATCH v4 2/3] git diff: improve range handling","fromName":"Chris Torek via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-06-12T16:19:59Z","receivedAt":"2020-06-12T16:20:10Z","isPatch":true,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"From: Chris Torek \n\nWhen git diff is given a symmetric difference A...B, it chooses\nsome merge base from the two specified commits (as documented).\n\nThis fails, however, if there is *no* merge base: instead, you\nsee the differences between A and B, which is certainly not what\nis expected.\n\nMoreover, if additional revisions are specified on the command\nline (\"git diff A...B C\"), the results get a bit weird:\n\n * If there is a symmetric difference merge base, this is used\n as the left side of the diff. The last final ref is used as\n the right side.\n * If there is no merge base, the symmetric status is completely\n lost. We will produce a combined diff instead.\n\nSimilar weirdness occurs if you use, e.g., \"git diff C A...B D\".\nLikewise, using multiple two-dot ranges, or tossing extra\nrevision specifiers into the command line with two-dot ranges,\nor mixing two and three dot ranges, all produce nonsense.\n\nTo avoid all this, add a routine to catch the range cases and\nverify that that the arguments make sense. As a side effect,\nproduce a warning showing *which* merge base is being used when\nthere are multiple choices; die if there is no merge base.\n\nSigned-off-by: Chris Torek \n---\n builtin/diff.c | 124 ++++++++++++++++++++++++++++++++++----\n t/t4068-diff-symmetric.sh | 91 ++++++++++++++++++++++++++++\n 2 files changed, 202 insertions(+), 13 deletions(-)\n create mode 100755 t/t4068-diff-symmetric.sh\n\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 8537b17bd5e..0f48b0d3e71 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -6,6 +6,7 @@\n #define USE_THE_INDEX_COMPATIBILITY_MACROS\n #include \"cache.h\"\n #include \"config.h\"\n+#include \"ewah/ewok.h\"\n #include \"lockfile.h\"\n #include \"color.h\"\n #include \"commit.h\"\n@@ -254,6 +255,108 @@ static int builtin_diff_files(struct rev_info *revs, int argc, const char **argv\n \treturn run_diff_files(revs, options);\n }\n \n+struct symdiff {\n+\tstruct bitmap *skip;\n+\tint warn;\n+\tconst char *base, *left, *right;\n+};\n+\n+/*\n+ * Check for symmetric-difference arguments, and if present, arrange\n+ * everything we need to know to handle them correctly. As a bonus,\n+ * weed out all bogus range-based revision specifications, e.g.,\n+ * \"git diff A..B C..D\" or \"git diff A..B C\" get rejected.\n+ *\n+ * For an actual symmetric diff, *symdiff is set this way:\n+ *\n+ * - its skip is non-NULL and marks *all* rev->pending.objects[i]\n+ * indices that the caller should ignore (extra merge bases, of\n+ * which there might be many, and A in A...B). Note that the\n+ * chosen merge base and right side are NOT marked.\n+ * - warn is set if there are multiple merge bases.\n+ * - base, left, and right point to the names to use in a\n+ * warning about multiple merge bases.\n+ *\n+ * If there is no symmetric diff argument, sym->skip is NULL and\n+ * sym->warn is cleared. The remaining fields are not set.\n+ */\n+static void symdiff_prepare(struct rev_info *rev, struct symdiff *sym)\n+{\n+\tint i, is_symdiff = 0, basecount = 0, othercount = 0;\n+\tint lpos = -1, rpos = -1, basepos = -1;\n+\tstruct bitmap *map = NULL;\n+\n+\t/*\n+\t * Use the whence fields to find merge bases and left and\n+\t * right parts of symmetric difference, so that we do not\n+\t * depend on the order that revisions are parsed. If there\n+\t * are any revs that aren't from these sources, we have a\n+\t * \"git diff C A...B\" or \"git diff A...B C\" case. Or we\n+\t * could even get \"git diff A...B C...E\", for instance.\n+\t *\n+\t * If we don't have just one merge base, we pick one\n+\t * at random.\n+\t *\n+\t * NB: REV_CMD_LEFT, REV_CMD_RIGHT are also used for A..B,\n+\t * so we must check for SYMMETRIC_LEFT too. The two arrays\n+\t * rev->pending.objects and rev->cmdline.rev are parallel.\n+\t */\n+\tfor (i = 0; i < rev->cmdline.nr; i++) {\n+\t\tstruct object *obj = rev->pending.objects[i].item;\n+\t\tswitch (rev->cmdline.rev[i].whence) {\n+\t\tcase REV_CMD_MERGE_BASE:\n+\t\t\tif (basepos < 0)\n+\t\t\t\tbasepos = i;\n+\t\t\tbasecount++;\n+\t\t\tbreak;\t\t/* do mark all bases */\n+\t\tcase REV_CMD_LEFT:\n+\t\t\tif (lpos >= 0)\n+\t\t\t\tusage(builtin_diff_usage);\n+\t\t\tlpos = i;\n+\t\t\tif (obj->flags & SYMMETRIC_LEFT) {\n+\t\t\t\tis_symdiff = 1;\n+\t\t\t\tbreak;\t/* do mark A */\n+\t\t\t}\n+\t\t\tcontinue;\n+\t\tcase REV_CMD_RIGHT:\n+\t\t\tif (rpos >= 0)\n+\t\t\t\tusage(builtin_diff_usage);\n+\t\t\trpos = i;\n+\t\t\tcontinue;\t/* don't mark B */\n+\t\tcase REV_CMD_PARENTS_ONLY:\n+\t\tcase REV_CMD_REF:\n+\t\tcase REV_CMD_REV:\n+\t\t\tothercount++;\n+\t\t\tcontinue;\n+\t\t}\n+\t\tif (map == NULL)\n+\t\t\tmap = bitmap_new();\n+\t\tbitmap_set(map, i);\n+\t}\n+\n+\t/*\n+\t * Forbid any additional revs for both A...B and A..B.\n+\t */\n+\tif (lpos >= 0 && othercount > 0)\n+\t\tusage(builtin_diff_usage);\n+\n+\tif (!is_symdiff) {\n+\t\tbitmap_free(map);\n+\t\tsym->warn = 0;\n+\t\tsym->skip = NULL;\n+\t\treturn;\n+\t}\n+\n+\tsym->left = rev->pending.objects[lpos].name;\n+\tsym->right = rev->pending.objects[rpos].name;\n+\tsym->base = rev->pending.objects[basepos].name;\n+\tif (basecount == 0)\n+\t\tdie(_(\"%s...%s: no merge base\"), sym->left, sym->right);\n+\tbitmap_unset(map, basepos);\t/* unmark the base we want */\n+\tsym->warn = basecount > 1;\n+\tsym->skip = map;\n+}\n+\n int cmd_diff(int argc, const char **argv, const char *prefix)\n {\n \tint i;\n@@ -263,6 +366,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \tstruct object_array_entry *blob[2];\n \tint nongit = 0, no_index = 0;\n \tint result = 0;\n+\tstruct symdiff sdiff;\n \n \t/*\n \t * We could get N tree-ish in the rev.pending_objects list.\n@@ -382,6 +486,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n+\tsymdiff_prepare(&rev, &sdiff);\n \tfor (i = 0; i < rev.pending.nr; i++) {\n \t\tstruct object_array_entry *entry = &rev.pending.objects[i];\n \t\tstruct object *obj = entry->item;\n@@ -396,6 +501,8 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\t\tobj = &get_commit_tree(((struct commit *)obj))->object;\n \n \t\tif (obj->type == OBJ_TREE) {\n+\t\t\tif (sdiff.skip && bitmap_get(sdiff.skip, i))\n+\t\t\t\tcontinue;\n \t\t\tobj->flags |= flags;\n \t\t\tadd_object_array(obj, name, &ent);\n \t\t} else if (obj->type == OBJ_BLOB) {\n@@ -437,21 +544,12 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\tusage(builtin_diff_usage);\n \telse if (ent.nr == 1)\n \t\tresult = builtin_diff_index(&rev, argc, argv);\n-\telse if (ent.nr == 2)\n+\telse if (ent.nr == 2) {\n+\t\tif (sdiff.warn)\n+\t\t\twarning(_(\"%s...%s: multiple merge bases, using %s\"),\n+\t\t\t\tsdiff.left, sdiff.right, sdiff.base);\n \t\tresult = builtin_diff_tree(&rev, argc, argv,\n \t\t\t\t\t &ent.objects[0], &ent.objects[1]);\n-\telse if (ent.objects[0].item->flags & UNINTERESTING) {\n-\t\t/*\n-\t\t * diff A...B where there is at least one merge base\n-\t\t * between A and B. We have ent.objects[0] ==\n-\t\t * merge-base, ent.objects[ents-2] == A, and\n-\t\t * ent.objects[ents-1] == B. Show diff between the\n-\t\t * base and B. Note that we pick one merge base at\n-\t\t * random if there are more than one.\n-\t\t */\n-\t\tresult = builtin_diff_tree(&rev, argc, argv,\n-\t\t\t\t\t &ent.objects[0],\n-\t\t\t\t\t &ent.objects[ent.nr-1]);\n \t} else\n \t\tresult = builtin_diff_combined(&rev, argc, argv,\n \t\t\t\t\t ent.objects, ent.nr);\ndiff --git a/t/t4068-diff-symmetric.sh b/t/t4068-diff-symmetric.sh\nnew file mode 100755\nindex 00000000000..31d17a5af02\n--- /dev/null\n+++ b/t/t4068-diff-symmetric.sh\n@@ -0,0 +1,91 @@\n+#!/bin/sh\n+\n+test_description='behavior of diff with symmetric-diff setups'\n+\n+. ./test-lib.sh\n+\n+# build these situations:\n+# - normal merge with one merge base (br1...b2r);\n+# - criss-cross merge ie 2 merge bases (br1...master);\n+# - disjoint subgraph (orphan branch, br3...master).\n+#\n+# B---E <-- master\n+# / \\ /\n+# A X\n+# \\ / \\\n+# C---D--G <-- br1\n+# \\ /\n+# ---F <-- br2\n+#\n+# H <-- br3\n+#\n+# We put files into a few commits so that we can verify the\n+# output as well.\n+\n+test_expect_success setup '\n+\tgit commit --allow-empty -m A &&\n+\techo b >b &&\n+\tgit add b &&\n+\tgit commit -m B &&\n+\tgit checkout -b br1 HEAD^ &&\n+\techo c >c &&\n+\tgit add c &&\n+\tgit commit -m C &&\n+\tgit tag commit-C &&\n+\tgit merge -m D master &&\n+\tgit tag commit-D &&\n+\tgit checkout master &&\n+\tgit merge -m E commit-C &&\n+\tgit checkout -b br2 commit-C &&\n+\techo f >f &&\n+\tgit add f &&\n+\tgit commit -m F &&\n+\tgit checkout br1 &&\n+\tgit merge -m G br2 &&\n+\tgit checkout --orphan br3 &&\n+\tgit commit -m H\n+'\n+\n+test_expect_success 'diff with one merge base' '\n+\tgit diff commit-D...br1 >tmp &&\n+\ttail -n 1 tmp >actual &&\n+\techo +f >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+# The output (in tmp) can have +b or +c depending\n+# on which merge base (commit B or C) is picked.\n+# It should have one of those two, which comes out\n+# to seven lines.\n+test_expect_success 'diff with two merge bases' '\n+\tgit diff br1...master >tmp 2>err &&\n+\ttest_line_count = 7 tmp &&\n+\ttest_line_count = 1 err\n+'\n+\n+test_expect_success 'diff with no merge bases' '\n+\ttest_must_fail git diff br2...br3 >tmp 2>err &&\n+\ttest_i18ngrep \"fatal: br2...br3: no merge base\" err\n+'\n+\n+test_expect_success 'diff with too many symmetric differences' '\n+\ttest_must_fail git diff br1...master br2...br3 >tmp 2>err &&\n+\ttest_i18ngrep \"usage\" err\n+'\n+\n+test_expect_success 'diff with symmetric difference and extraneous arg' '\n+\ttest_must_fail git diff master br1...master >tmp 2>err &&\n+\ttest_i18ngrep \"usage\" err\n+'\n+\n+test_expect_success 'diff with two ranges' '\n+\ttest_must_fail git diff master br1..master br2..br3 >tmp 2>err &&\n+\ttest_i18ngrep \"usage\" err\n+'\n+\n+test_expect_success 'diff with ranges and extra arg' '\n+\ttest_must_fail git diff master br1..master commit-D >tmp 2>err &&\n+\ttest_i18ngrep \"usage\" err\n+'\n+\n+test_done\n-- \ngitgitgadget\n\n"},{"id":"399606","messageId":"d7bc9aca44bdff18ec2cee4f45d34c0965dc2002.1591978801.git.gitgitgadget@gmail.com","threadId":"53639","inReplyTo":"pull.804.v4.git.git.1591978801.gitgitgadget@gmail.com","subject":"[PATCH v4 3/3] Documentation: usage for diff combined commits","fromName":"Chris Torek via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-06-12T16:20:00Z","receivedAt":"2020-06-12T16:20:11Z","isPatch":true,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"From: Chris Torek \n\nDocument the usage for producing combined commits with \"git diff\".\nThis includes updating the synopsis section.\n\nWhile here, add the three-dot notation to the synopsis.\n\nMake \"git diff -h\" print the same usage summary as the manual\npage synopsis, minus the \"A..B\" form, which is now discouraged.\n\nSigned-off-by: Chris Torek \n---\n Documentation/git-diff.txt | 20 ++++++++++++++++----\n builtin/diff.c | 8 +++++++-\n 2 files changed, 23 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt\nindex 37781cf1755..1018110ddc2 100644\n--- a/Documentation/git-diff.txt\n+++ b/Documentation/git-diff.txt\n@@ -11,15 +11,17 @@ SYNOPSIS\n [verse]\n 'git diff' [] [] [--] [...]\n 'git diff' [] --cached [] [--] [...]\n-'git diff' [] [--] [...]\n+'git diff' [] [...] [--] [...]\n+'git diff' [] ... [--] [...]\n 'git diff' [] \n 'git diff' [] --no-index [--] \n \n DESCRIPTION\n -----------\n Show changes between the working tree and the index or a tree, changes\n-between the index and a tree, changes between two trees, changes between\n-two blob objects, or changes between two files on disk.\n+between the index and a tree, changes between two trees, changes resulting\n+from a merge, changes between two blob objects, or changes between two\n+files on disk.\n \n 'git diff' [] [--] [...]::\n \n@@ -67,6 +69,15 @@ two blob objects, or changes between two files on disk.\n \tone side is omitted, it will have the same effect as\n \tusing HEAD instead.\n \n+'git diff' [] [...] [--] [...]::\n+\n+\tThis form is to view the results of a merge commit. The first\n+\tlisted must be the merge itself; the remaining two or\n+\tmore commits should be its parents. A convenient way to produce\n+\tthe desired set of revisions is to use the {caret}@ suffix.\n+\tFor instance, if `master` names a merge commit, `git diff master\n+\tmaster^@` gives the same combined diff as `git show master`.\n+\n 'git diff' [] \\... [--] [...]::\n \n \tThis form is to view the changes on the branch containing\n@@ -196,7 +207,8 @@ linkgit:git-difftool[1],\n linkgit:git-log[1],\n linkgit:gitdiffcore[7],\n linkgit:git-format-patch[1],\n-linkgit:git-apply[1]\n+linkgit:git-apply[1],\n+linkgit:git-show[1]\n \n GIT\n ---\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 0f48b0d3e71..b3d17340ee3 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -24,7 +24,13 @@\n #define DIFF_NO_INDEX_IMPLICIT 2\n \n static const char builtin_diff_usage[] =\n-\"git diff [] [ []] [--] [...]\";\n+\"git diff [] [] [--] [...]\\n\"\n+\" or: git diff [] --cached [] [--] [...]\\n\"\n+\" or: git diff [] [...] [--] [...]\\n\"\n+\" or: git diff [] ...] [--] [...]\\n\"\n+\" or: git diff [] ]\\n\"\n+\" or: git diff [] --no-index [--] ]\\n\"\n+COMMON_DIFF_OPTIONS_HELP;\n \n static const char *blob_path(struct object_array_entry *entry)\n {\n-- \ngitgitgadget\n"},{"id":"399610","messageId":"xmqq8sgs2ozk.fsf@gitster.c.googlers.com","threadId":"53639","inReplyTo":"6eadaa89-fde7-4224-dcb9-ceef315942f2@iee.email","subject":"Re: [PATCH 2/3] git diff: improve A...B merge-base handling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-12T17:06:07Z","receivedAt":"2020-06-12T17:06:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Philip Oakley writes:\n\n> On 09/06/2020 01:03, Chris Torek via GitGitGadget wrote:\n> [snip]\n>> +test_description='behavior of diff with symmetric-diff setups'\n>> +\n>> +. ./test-lib.sh\n>> +\n>> +# build these situations:\n>> +# - normal merge with one merge base (b1...b2);\n>> +# - criss-cross merge ie 2 merge bases (b1...master);\n>> +# - disjoint subgraph (orphan branch, b3...master).\n>\n> nit:\n> Use of b1, b2, b3 here, but br1, br2, br3 below\n>> +#\n>> +# B---E <-- master\n>> +# / \\ /\n>> +# A X\n>> +# \\ / \\\n>> +# C---D--G <-- br1\n>> +# \\ /\n>> +# ---F <-- br2\n>> +#\n>> +# H <-- br3\n>> +#\n\n\nTrue. Which one is to be recommended? The shorter and sweeter b1,\nb2 and b3?\n\nIn any case, that must match the topology used by the tests.\n\nThanks.\n"},{"id":"399621","messageId":"xmqq36702l1d.fsf@gitster.c.googlers.com","threadId":"53639","inReplyTo":"4fa6fba33b329ce85f68470fb2545adf1bb06900.1591978801.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 2/3] git diff: improve range handling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-12T18:31:26Z","receivedAt":"2020-06-12T18:31:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Chris Torek via GitGitGadget\" writes:\n\n> +struct symdiff {\n> +\tstruct bitmap *skip;\n> +\tint warn;\n> +\tconst char *base, *left, *right;\n> +};\n> +\n> +/*\n> + * Check for symmetric-difference arguments, and if present, arrange\n> + * everything we need to know to handle them correctly. As a bonus,\n> + * weed out all bogus range-based revision specifications, e.g.,\n> + * \"git diff A..B C..D\" or \"git diff A..B C\" get rejected.\n> + *\n> + * For an actual symmetric diff, *symdiff is set this way:\n> + *\n> + * - its skip is non-NULL and marks *all* rev->pending.objects[i]\n> + * indices that the caller should ignore (extra merge bases, of\n> + * which there might be many, and A in A...B). Note that the\n> + * chosen merge base and right side are NOT marked.\n> + * - warn is set if there are multiple merge bases.\n> + * - base, left, and right point to the names to use in a\n> + * warning about multiple merge bases.\n> + *\n> + * If there is no symmetric diff argument, sym->skip is NULL and\n> + * sym->warn is cleared. The remaining fields are not set.\n> + */\n\nOK.\n\n> +static void symdiff_prepare(struct rev_info *rev, struct symdiff *sym)\n> +{\n> +\tint i, is_symdiff = 0, basecount = 0, othercount = 0;\n> +\tint lpos = -1, rpos = -1, basepos = -1;\n> +\tstruct bitmap *map = NULL;\n> +\n> +\t/*\n> +\t * Use the whence fields to find merge bases and left and\n> +\t * right parts of symmetric difference, so that we do not\n> +\t * depend on the order that revisions are parsed. If there\n> +\t * are any revs that aren't from these sources, we have a\n> +\t * \"git diff C A...B\" or \"git diff A...B C\" case. Or we\n> +\t * could even get \"git diff A...B C...E\", for instance.\n> +\t *\n> +\t * If we don't have just one merge base, we pick one\n> +\t * at random.\n> +\t *\n> +\t * NB: REV_CMD_LEFT, REV_CMD_RIGHT are also used for A..B,\n> +\t * so we must check for SYMMETRIC_LEFT too. The two arrays\n> +\t * rev->pending.objects and rev->cmdline.rev are parallel.\n> +\t */\n> +\tfor (i = 0; i < rev->cmdline.nr; i++) {\n> +\t\tstruct object *obj = rev->pending.objects[i].item;\n> +\t\tswitch (rev->cmdline.rev[i].whence) {\n> +\t\tcase REV_CMD_MERGE_BASE:\n> +\t\t\tif (basepos < 0)\n> +\t\t\t\tbasepos = i;\n> +\t\t\tbasecount++;\n> +\t\t\tbreak;\t\t/* do mark all bases */\n\nWe find and use the first found merge base (i.e. \"pick at random\" as\npromised in the comment before the function), but for warning, keep\ntrack of how many merge bases there are.\n\n> +\t\tcase REV_CMD_LEFT:\n> +\t\t\tif (lpos >= 0)\n> +\t\t\t\tusage(builtin_diff_usage);\n\nA range (either A..B or A...B) has already been seen, and we have\nanother, which is now rejected.\n\n> +\t\t\tlpos = i;\n> +\t\t\tif (obj->flags & SYMMETRIC_LEFT) {\n> +\t\t\t\tis_symdiff = 1;\n> +\t\t\t\tbreak;\t/* do mark A */\n\nInside this switch statement, \"continue\" is a sign that the caller\nshould use the rev, and \"break\" is a sign that the rev is to be\nignored. We obviously do not ignore \"A\" in ...\n\n> +\t\t\t}\n> +\t\t\tcontinue;\n\n... \"A..B\" notation, so we \"continue\" here.\n\n> +\t\tcase REV_CMD_RIGHT:\n> +\t\t\tif (rpos >= 0)\n> +\t\t\t\tusage(builtin_diff_usage);\n\nHere is the same \"we reject having two or more ranges\". \n\nI actually suspect that this usage() would become dead code---we\nwould already have died when we saw the matching left end of the\nsecond range (so this could become BUG(), even though usage() does\nnot hurt).\n\n> +\t\t\trpos = i;\n> +\t\t\tcontinue;\t/* don't mark B */\n\nAnd of course, whether \"A..B\" or \"A...B\", B will be used as the\n\"result\" side of the diff, so won't be marked for skipping.\n\n> +\t\tcase REV_CMD_PARENTS_ONLY:\n> +\t\tcase REV_CMD_REF:\n> +\t\tcase REV_CMD_REV:\n> +\t\t\tothercount++;\n> +\t\t\tcontinue;\n\nI wonder if we want to use \"default\" instead of these three\nindividual cases. Pros and cons?\n\n - If we forgot to list a whence REV_CMD_* here, it will be silently\n marked to be skipped with this code. With \"default\", it will be\n counted to be diffed (which may trigger \"giving too many revs to\n be diffed\" error from the diff machinery, which is good).\n\n - With \"default\", when we add new type of whence to REV_CMD_* and\n forget to adjust this code, it will be counted to be diffed.\n With the current code, it will be skipped.\n\nWe probably could get the best of the both words by keeping the\nabove three for \"counted in othercount and kept), and then add a\ndefault arm to the switch() that just says \n\n\t\tdefault:\n\t\t\tBUG(\"forgot to handle %d\",\n\t\t\t rev->cmdline.rev[i].whence);\n\nThat way, every time we add a new type of whence, we would be forced\nto think what should be done to them.\n\n> +\t\t}\n> +\t\tif (map == NULL)\n> +\t\t\tmap = bitmap_new();\n> +\t\tbitmap_set(map, i);\n> +\t}\n> +\n> +\t/*\n> +\t * Forbid any additional revs for both A...B and A..B.\n> +\t */\n> +\tif (lpos >= 0 && othercount > 0)\n> +\t\tusage(builtin_diff_usage);\n\nMeaning \"git diff A..B C\" is bad. Reasonable.\n\n> +\tif (!is_symdiff) {\n> +\t\tbitmap_free(map);\n\nIt is not wrong per-se to free it unconditionally, but wouldn't it\nbe a bug if (map != NULL) at this point in the flow?\n\nThe merge bases would only be stuffed in the revs when A...B is\ngiven, and we are not skipping anything involved in A..B or\nnon-range revs.\n\n> +\t\tsym->warn = 0;\n> +\t\tsym->skip = NULL;\n\nClearing these two fields are as promised to the callers in the\ncomment above, which is good.\n\n> +\t\treturn;\n> +\t}\n> +\n> +\tsym->left = rev->pending.objects[lpos].name;\n> +\tsym->right = rev->pending.objects[rpos].name;\n> +\tsym->base = rev->pending.objects[basepos].name;\n> +\tif (basecount == 0)\n> +\t\tdie(_(\"%s...%s: no merge base\"), sym->left, sym->right);\n\nGood.\n\n> +\tbitmap_unset(map, basepos);\t/* unmark the base we want */\n\nHmph. You could\n\n\tcase REV_CMD_MERGE_BASE:\n\t\tbasecount++;\n\t\tif (basepos < 0) {\n\t\t\tbasepos = i;\n\t\t\tcontinue; /* keep this one */\n\t\t}\n\t\tbreak; /* skip all others */\n\nand lose this unset(). I do not think it makes too much of a\ndifference, but it probably is easier to follow if we avoided this\n\"do something and then come back to correct\" pattern.\n\n> +\tsym->warn = basecount > 1;\n> +\tsym->skip = map;\n> +}\n> +\n> int cmd_diff(int argc, const char **argv, const char *prefix)\n> {\n> \tint i;\n> @@ -263,6 +366,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n> \tstruct object_array_entry *blob[2];\n> \tint nongit = 0, no_index = 0;\n> \tint result = 0;\n> +\tstruct symdiff sdiff;\n> \n> \t/*\n> \t * We could get N tree-ish in the rev.pending_objects list.\n> @@ -382,6 +486,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n> \t\t}\n> \t}\n> \n> +\tsymdiff_prepare(&rev, &sdiff);\n> \tfor (i = 0; i < rev.pending.nr; i++) {\n> \t\tstruct object_array_entry *entry = &rev.pending.objects[i];\n> \t\tstruct object *obj = entry->item;\n> @@ -396,6 +501,8 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n> \t\t\tobj = &get_commit_tree(((struct commit *)obj))->object;\n> \n> \t\tif (obj->type == OBJ_TREE) {\n> +\t\t\tif (sdiff.skip && bitmap_get(sdiff.skip, i))\n> +\t\t\t\tcontinue;\n\nBy the way, I cannot shake this feeling that, given that\nrev.pending/cmdline.nr will not be an unreasonably large number, if\nit is overkill to use the bitmap here. If I were writing this code,\nI would have made symdiff_prepare() to fill a separate object array\nby copying the elements to be used in the final \"diff\" out of the\nrev.pending array and updated this loop to iterate over that array.\n\nI am not saying that such an approach is better than the use of\nbitmap code here. It just was a bit unexpected to see the bitmap\ncode used for set of objects that is typically less than a dozen.\n\nThanks.\n\n"},{"id":"399628","messageId":"CAPx1Gvd8Ggt3OwxAcQY3NsxY=ypOk4S=8TdhjzsohKbbyWNGyQ@mail.gmail.com","threadId":"53639","inReplyTo":"xmqq36702l1d.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v4 2/3] git diff: improve range handling","fromName":"Chris Torek","fromEmail":"chris.torek@gmail.com","sentAt":"2020-06-12T19:25:21Z","receivedAt":"2020-06-12T19:25:56Z","isPatch":true,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"Ugh, forgot to tweak gmail reply to go to the mailing list. Also\nI typed in a wrong word (\"commit\" should be \"comment\").\n\nCorrected reply:\n\nOn Fri, Jun 12, 2020 at 11:31 AM Junio C Hamano wrote:\n>\n> \"Chris Torek via GitGitGadget\" writes:\n>\n> > +struct symdiff {\n> > + struct bitmap *skip;\n> > + int warn;\n> > + const char *base, *left, *right;\n> > +};\n> > +\n> > +/*\n> > + * Check for symmetric-difference arguments, and if present, arrange\n> > + * everything we need to know to handle them correctly. As a bonus,\n> > + * weed out all bogus range-based revision specifications, e.g.,\n> > + * \"git diff A..B C..D\" or \"git diff A..B C\" get rejected.\n> > + *\n> > + * For an actual symmetric diff, *symdiff is set this way:\n> > + *\n> > + * - its skip is non-NULL and marks *all* rev->pending.objects[i]\n> > + * indices that the caller should ignore (extra merge bases, of\n> > + * which there might be many, and A in A...B). Note that the\n> > + * chosen merge base and right side are NOT marked.\n> > + * - warn is set if there are multiple merge bases.\n> > + * - base, left, and right point to the names to use in a\n> > + * warning about multiple merge bases.\n> > + *\n> > + * If there is no symmetric diff argument, sym->skip is NULL and\n> > + * sym->warn is cleared. The remaining fields are not set.\n> > + */\n>\n> OK.\n>\n> > +static void symdiff_prepare(struct rev_info *rev, struct symdiff *sym)\n> > +{\n> > + int i, is_symdiff = 0, basecount = 0, othercount = 0;\n> > + int lpos = -1, rpos = -1, basepos = -1;\n> > + struct bitmap *map = NULL;\n> > +\n> > + /*\n> > + * Use the whence fields to find merge bases and left and\n> > + * right parts of symmetric difference, so that we do not\n> > + * depend on the order that revisions are parsed. If there\n> > + * are any revs that aren't from these sources, we have a\n> > + * \"git diff C A...B\" or \"git diff A...B C\" case. Or we\n> > + * could even get \"git diff A...B C...E\", for instance.\n> > + *\n> > + * If we don't have just one merge base, we pick one\n> > + * at random.\n> > + *\n> > + * NB: REV_CMD_LEFT, REV_CMD_RIGHT are also used for A..B,\n> > + * so we must check for SYMMETRIC_LEFT too. The two arrays\n> > + * rev->pending.objects and rev->cmdline.rev are parallel.\n> > + */\n> > + for (i = 0; i < rev->cmdline.nr; i++) {\n> > + struct object *obj = rev->pending.objects[i].item;\n> > + switch (rev->cmdline.rev[i].whence) {\n> > + case REV_CMD_MERGE_BASE:\n> > + if (basepos < 0)\n> > + basepos = i;\n> > + basecount++;\n> > + break; /* do mark all bases */\n>\n> We find and use the first found merge base (i.e. \"pick at random\" as\n> promised in the comment before the function), but for warning, keep\n> track of how many merge bases there are.\n>\n> > + case REV_CMD_LEFT:\n> > + if (lpos >= 0)\n> > + usage(builtin_diff_usage);\n>\n> A range (either A..B or A...B) has already been seen, and we have\n> another, which is now rejected.\n>\n> > + lpos = i;\n> > + if (obj->flags & SYMMETRIC_LEFT) {\n> > + is_symdiff = 1;\n> > + break; /* do mark A */\n>\n> Inside this switch statement, \"continue\" is a sign that the caller\n> should use the rev, and \"break\" is a sign that the rev is to be\n> ignored. We obviously do not ignore \"A\" in ...\n>\n> > + }\n> > + continue;\n>\n> ... \"A..B\" notation, so we \"continue\" here.\n>\n> > + case REV_CMD_RIGHT:\n> > + if (rpos >= 0)\n> > + usage(builtin_diff_usage);\n>\n> Here is the same \"we reject having two or more ranges\".\n>\n> I actually suspect that this usage() would become dead code---we\n> would already have died when we saw the matching left end of the\n> second range (so this could become BUG(), even though usage() does\n> not hurt).\n\nRight - I considered it both ways and figured a second usage() was\nsimpler and straightforward, but I'm fine with either method.\n\n> > + rpos = i;\n> > + continue; /* don't mark B */\n>\n> And of course, whether \"A..B\" or \"A...B\", B will be used as the\n> \"result\" side of the diff, so won't be marked for skipping.\n>\n> > + case REV_CMD_PARENTS_ONLY:\n> > + case REV_CMD_REF:\n> > + case REV_CMD_REV:\n> > + othercount++;\n> > + continue;\n>\n> I wonder if we want to use \"default\" instead of these three\n> individual cases. Pros and cons?\n>\n> - If we forgot to list a whence REV_CMD_* here, it will be silently\n> marked to be skipped with this code. With \"default\", it will be\n> counted to be diffed (which may trigger \"giving too many revs to\n> be diffed\" error from the diff machinery, which is good).\n>\n> - With \"default\", when we add new type of whence to REV_CMD_* and\n> forget to adjust this code, it will be counted to be diffed.\n> With the current code, it will be skipped.\n>\n> We probably could get the best of the both words by keeping the\n> above three for \"counted in othercount and kept), and then add a\n> default arm to the switch() that just says\n>\n> default:\n> BUG(\"forgot to handle %d\",\n> rev->cmdline.rev[i].whence);\n\nI'm fine with this as well. I did it the way I did because at least some\ncompilers give a warning or error if you forgot an enum. Using default\n(or adding one) defeats this, but the BUG method is reasonable.\n\n> That way, every time we add a new type of whence, we would be forced\n> to think what should be done to them.\n>\n> > + }\n> > + if (map == NULL)\n> > + map = bitmap_new();\n> > + bitmap_set(map, i);\n> > + }\n> > +\n> > + /*\n> > + * Forbid any additional revs for both A...B and A..B.\n> > + */\n> > + if (lpos >= 0 && othercount > 0)\n> > + usage(builtin_diff_usage);\n>\n> Meaning \"git diff A..B C\" is bad. Reasonable.\n>\n> > + if (!is_symdiff) {\n> > + bitmap_free(map);\n>\n> It is not wrong per-se to free it unconditionally, but wouldn't it\n> be a bug if (map != NULL) at this point in the flow?\n\nYes, because the A in A...B will have been marked. It didn't\nseem worth a BUG() call though.\n\n> The merge bases would only be stuffed in the revs when A...B is\n> given, and we are not skipping anything involved in A..B or\n> non-range revs.\n>\n> > + sym->warn = 0;\n> > + sym->skip = NULL;\n>\n> Clearing these two fields are as promised to the callers in the\n> comment above, which is good.\n>\n> > + return;\n> > + }\n> > +\n> > + sym->left = rev->pending.objects[lpos].name;\n> > + sym->right = rev->pending.objects[rpos].name;\n> > + sym->base = rev->pending.objects[basepos].name;\n> > + if (basecount == 0)\n> > + die(_(\"%s...%s: no merge base\"), sym->left, sym->right);\n>\n> Good.\n>\n> > + bitmap_unset(map, basepos); /* unmark the base we want */\n>\n> Hmph. You could\n>\n> case REV_CMD_MERGE_BASE:\n> basecount++;\n> if (basepos < 0) {\n> basepos = i;\n> continue; /* keep this one */\n> }\n> break; /* skip all others */\n>\n> and lose this unset(). I do not think it makes too much of a\n> difference, but it probably is easier to follow if we avoided this\n> \"do something and then come back to correct\" pattern.\n\nOK, I'll change it to do this in the re-roll.\n\n> > + sym->warn = basecount > 1;\n> > + sym->skip = map;\n> > +}\n> > +\n> > int cmd_diff(int argc, const char **argv, const char *prefix)\n> > {\n> > int i;\n> > @@ -263,6 +366,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n> > struct object_array_entry *blob[2];\n> > int nongit = 0, no_index = 0;\n> > int result = 0;\n> > + struct symdiff sdiff;\n> >\n> > /*\n> > * We could get N tree-ish in the rev.pending_objects list.\n> > @@ -382,6 +486,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n> > }\n> > }\n> >\n> > + symdiff_prepare(&rev, &sdiff);\n> > for (i = 0; i < rev.pending.nr; i++) {\n> > struct object_array_entry *entry = &rev.pending.objects[i];\n> > struct object *obj = entry->item;\n> > @@ -396,6 +501,8 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n> > obj = &get_commit_tree(((struct commit *)obj))->object;\n> >\n> > if (obj->type == OBJ_TREE) {\n> > + if (sdiff.skip && bitmap_get(sdiff.skip, i))\n> > + continue;\n>\n> By the way, I cannot shake this feeling that, given that\n> rev.pending/cmdline.nr will not be an unreasonably large number, if\n> it is overkill to use the bitmap here. If I were writing this code,\n> I would have made symdiff_prepare() to fill a separate object array\n> by copying the elements to be used in the final \"diff\" out of the\n> rev.pending array and updated this loop to iterate over that array.\n>\n> I am not saying that such an approach is better than the use of\n> bitmap code here. It just was a bit unexpected to see the bitmap\n> code used for set of objects that is typically less than a dozen.\n\nI am sure it is overkill -- but at the same time, it's already coded\nand cheap enough to use that rolling our own separate array felt\nworse. I can add a comment (NOT COMMIT) if you like.\n\nThanks,\n\nChris\n"}]}