{"thread":{"id":"54193","subject":"[PATCH 0/4] builtin/diff: learn --merge-base","startedAt":"2020-09-05T19:08:41Z","lastAt":"2020-09-29T19:53:29Z","messageCount":58,"participants":["Denton Liu","Junio C Hamano","Jeff King","Martin Ågren"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"405074","messageId":"cover.1599332861.git.liu.denton@gmail.com","threadId":"54193","inReplyTo":null,"subject":"[PATCH 0/4] builtin/diff: learn --merge-base","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-09-05T19:08:17Z","receivedAt":"2020-09-05T19:08:41Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The range-notation in `git diff` has been cited as a mistake since diff\ncompares two endpoints, not whole ranges.[0] In fact, the ranges seem\nto take on the opposite meanings when compared to range notation in\n`git log`.\n\nIn an effort to reduce the use of range notation as much as possible,\nintroduce the `--merge-base` flag, slightly modified from a suggestion\nby Jonathan Nieder.[1] This flag allows us to replace the first commit\ngiven on the command-line with its merge base between the first and\nsecond commits. This allows us to gently deprecate the `...` form\nentirely, although that is left as an exercise to the reader ;)\n\nOne additional bonus is that this flag allows the \"after\" side to be not\njust constrained to a commit (like with `...` notation). It can now be\nthe working tree or the index as well.\n\n[0]: https://lore.kernel.org/git/xmqqy2v26hu0.fsf@gitster-ct.c.googlers.com/\n[1]: https://lore.kernel.org/git/20191223215928.GB38316@google.com/\n\nDenton Liu (4):\n t4068: remove unnecessary >tmp\n git-diff.txt: backtick quote command text\n builtin/diff: parse --no-index using parse_options()\n builtin/diff: learn --merge-base\n\n Documentation/git-diff.txt | 40 ++++++++++++-----\n builtin/diff.c | 92 +++++++++++++++++++++++++++++++++-----\n diff-no-index.c | 15 ++-----\n t/t4068-diff-symmetric.sh | 89 +++++++++++++++++++++++++++++++++---\n 4 files changed, 197 insertions(+), 39 deletions(-)\n\n-- \n2.28.0.rc0.135.gc7877b767d\n\n"},{"id":"405075","messageId":"80e9066a59524ededbb0d0191a93e6e3154c5623.1599332861.git.liu.denton@gmail.com","threadId":"54193","inReplyTo":"cover.1599332861.git.liu.denton@gmail.com","subject":"[PATCH 1/4] t4068: remove unnecessary >tmp","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-09-05T19:08:18Z","receivedAt":"2020-09-05T19:08:44Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The many `git diff` invocations have a `>tmp` redirection even though\nthe file is not being used afterwards. Remove these unnecessary\nredirections.\n\nSigned-off-by: Denton Liu \n---\n t/t4068-diff-symmetric.sh | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t4068-diff-symmetric.sh b/t/t4068-diff-symmetric.sh\nindex 31d17a5af0..60c506c2b2 100755\n--- a/t/t4068-diff-symmetric.sh\n+++ b/t/t4068-diff-symmetric.sh\n@@ -64,27 +64,27 @@ test_expect_success 'diff with two merge bases' '\n '\n \n test_expect_success 'diff with no merge bases' '\n-\ttest_must_fail git diff br2...br3 >tmp 2>err &&\n+\ttest_must_fail git diff br2...br3 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_must_fail git diff br1...master br2...br3 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_must_fail git diff master br1...master 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_must_fail git diff master br1..master br2..br3 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_must_fail git diff master br1..master commit-D 2>err &&\n \ttest_i18ngrep \"usage\" err\n '\n \n-- \n2.28.0.rc0.135.gc7877b767d\n\n"},{"id":"405076","messageId":"ea6717e7b3a8b89fc3750505678321803cde78dc.1599332861.git.liu.denton@gmail.com","threadId":"54193","inReplyTo":"cover.1599332861.git.liu.denton@gmail.com","subject":"[PATCH 3/4] builtin/diff: parse --no-index using parse_options()","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-09-05T19:08:20Z","receivedAt":"2020-09-05T19:08:47Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Instead of parsing the `--no-index` option with a plain strcmp, use\nparse_options() to parse options. This allows us to easily add more\noptions in a future commit.\n\nAs a result of this change, `--no-index` is removed from `argv` so, as a\nresult, we no longer need to handle it in diff_no_index().\n\nSigned-off-by: Denton Liu \n---\n builtin/diff.c | 33 ++++++++++++++++++++++-----------\n diff-no-index.c | 15 +++------------\n 2 files changed, 25 insertions(+), 23 deletions(-)\n\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex cb98811c21..0e086ed7c4 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -373,6 +373,13 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \tint nongit = 0, no_index = 0;\n \tint result = 0;\n \tstruct symdiff sdiff;\n+\tstruct option options[] = {\n+\t\tOPT_SET_INT_F(0, \"no-index\", &no_index,\n+\t\t\t N_(\"compare the given paths on the filesystem\"),\n+\t\t\t DIFF_NO_INDEX_EXPLICIT,\n+\t\t\t PARSE_OPT_NONEG),\n+\t\tOPT_END(),\n+\t};\n \n \t/*\n \t * We could get N tree-ish in the rev.pending_objects list.\n@@ -406,21 +413,25 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t * Other cases are errors.\n \t */\n \n-\t/* Were we asked to do --no-index explicitly? */\n-\tfor (i = 1; i < argc; i++) {\n-\t\tif (!strcmp(argv[i], \"--\")) {\n-\t\t\ti++;\n-\t\t\tbreak;\n-\t\t}\n-\t\tif (!strcmp(argv[i], \"--no-index\"))\n-\t\t\tno_index = DIFF_NO_INDEX_EXPLICIT;\n-\t\tif (argv[i][0] != '-')\n-\t\t\tbreak;\n-\t}\n+\targc = parse_options(argc, argv, prefix, options, NULL,\n+\t\t\t PARSE_OPT_KEEP_DASHDASH |\n+\t\t\t PARSE_OPT_KEEP_ARGV0 |\n+\t\t\t PARSE_OPT_KEEP_UNKNOWN |\n+\t\t\t PARSE_OPT_NO_INTERNAL_HELP);\n \n \tprefix = setup_git_directory_gently(&nongit);\n \n \tif (!no_index) {\n+\t\tint i;\n+\t\tfor (i = 1; i < argc; i++) {\n+\t\t\tif (!strcmp(argv[i], \"--\")) {\n+\t\t\t\ti++;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t\tif (argv[i][0] != '-')\n+\t\t\t\tbreak;\n+\t\t}\n+\n \t\t/*\n \t\t * Treat git diff with at least one path outside of the\n \t\t * repo the same as if the command would have been executed\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex 7814eabfe0..da82e2d090 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -243,28 +243,19 @@ int diff_no_index(struct rev_info *revs,\n \t\t int implicit_no_index,\n \t\t int argc, const char **argv)\n {\n-\tint i, no_index;\n+\tint i;\n \tconst char *paths[2];\n \tstruct strbuf replacement = STRBUF_INIT;\n \tconst char *prefix = revs->prefix;\n-\tstruct option no_index_options[] = {\n-\t\tOPT_BOOL_F(0, \"no-index\", &no_index, \"\",\n-\t\t\t PARSE_OPT_NONEG | PARSE_OPT_HIDDEN),\n-\t\tOPT_END(),\n-\t};\n-\tstruct option *options;\n \n-\toptions = parse_options_concat(no_index_options,\n-\t\t\t\t revs->diffopt.parseopts);\n-\targc = parse_options(argc, argv, revs->prefix, options,\n+\targc = parse_options(argc, argv, revs->prefix, revs->diffopt.parseopts,\n \t\t\t diff_no_index_usage, 0);\n \tif (argc != 2) {\n \t\tif (implicit_no_index)\n \t\t\twarning(_(\"Not a git repository. Use --no-index to \"\n \t\t\t\t \"compare two paths outside a working tree\"));\n-\t\tusage_with_options(diff_no_index_usage, options);\n+\t\tusage_with_options(diff_no_index_usage, revs->diffopt.parseopts);\n \t}\n-\tFREE_AND_NULL(options);\n \tfor (i = 0; i < 2; i++) {\n \t\tconst char *p = argv[argc - 2 + i];\n \t\tif (!strcmp(p, \"-\"))\n-- \n2.28.0.rc0.135.gc7877b767d\n\n"},{"id":"405077","messageId":"8e72bd8fea276d40729705eb8dcc63bcc9faef2c.1599332861.git.liu.denton@gmail.com","threadId":"54193","inReplyTo":"cover.1599332861.git.liu.denton@gmail.com","subject":"[PATCH 2/4] git-diff.txt: backtick quote command text","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-09-05T19:08:19Z","receivedAt":"2020-09-05T19:08:52Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The modern way to quote commands in the documentation is to use\nbackticks instead of double-quotes as this renders the text with the\ncode style. Convert double-quoted command text to backtick-quoted\ncommands. While we're at it, quote one instance of `^@`.\n\nSigned-off-by: Denton Liu \n---\n Documentation/git-diff.txt | 18 +++++++++---------\n 1 file changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt\nindex 727f24d16e..8f7b4ed3ca 100644\n--- a/Documentation/git-diff.txt\n+++ b/Documentation/git-diff.txt\n@@ -68,13 +68,13 @@ files on disk.\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+\tthe desired set of revisions is to use the `^@` 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 is synonymous to the earlier form (without the \"..\") for\n+\tThis is synonymous to the earlier form (without the `..`) for\n \tviewing the changes between two arbitrary . If on\n \tone side is omitted, it will have the same effect as\n \tusing HEAD instead.\n@@ -83,20 +83,20 @@ files on disk.\n \n \tThis form is to view the changes on the branch containing\n \tand up to the second , starting at a common ancestor\n-\tof both . \"git diff A\\...B\" is equivalent to\n-\t\"git diff $(git merge-base A B) B\". You can omit any one\n+\tof both . `git diff A...B` is equivalent to\n+\t`git diff $(git merge-base A B) B`. You can omit any one\n \tof , which has the same effect as using HEAD instead.\n \n Just in case you are doing something exotic, it should be\n noted that all of the in the above description, except\n-in the last two forms that use \"..\" notations, can be any\n+in the last two forms that use `..` notations, can be any\n .\n \n For a more complete list of ways to spell , see\n \"SPECIFYING REVISIONS\" section in linkgit:gitrevisions[7].\n However, \"diff\" is about comparing two _endpoints_, not ranges,\n-and the range notations (\"..\" and\n-\"\\...\") do not mean a range as defined in the\n+and the range notations (`..` and\n+`...`) do not mean a range as defined in the\n \"SPECIFYING RANGES\" section in linkgit:gitrevisions[7].\n \n 'git diff' [] ::\n@@ -144,9 +144,9 @@ $ git diff HEAD <3>\n +\n <1> Changes in the working tree not yet staged for the next commit.\n <2> Changes between the index and your last commit; what you\n- would be committing if you run \"git commit\" without \"-a\" option.\n+ would be committing if you run `git commit` without `-a` option.\n <3> Changes in the working tree since your last commit; what you\n- would be committing if you run \"git commit -a\"\n+ would be committing if you run `git commit -a`\n \n Comparing with arbitrary commits::\n +\n-- \n2.28.0.rc0.135.gc7877b767d\n\n"},{"id":"405078","messageId":"231ba3f661cc4aa7a55c44e339e187c6d70c5507.1599332861.git.liu.denton@gmail.com","threadId":"54193","inReplyTo":"cover.1599332861.git.liu.denton@gmail.com","subject":"[PATCH 4/4] builtin/diff: learn --merge-base","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-09-05T19:08:21Z","receivedAt":"2020-09-05T19:09:06Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In order to get the diff between a commit and its merge base, the\ncurrently preferred method is to use `git diff A...B`. However, the\nrange-notation with diff has, time and time again, been noted as a point\nof confusion and thus, it should be avoided. Although we have a\nsubstitute for the double-dot notation, we don't have any replacement\nfor the triple-dot notation.\n\nIntroduce the `--merge-base` flag as a replacement for triple-dot\nnotation. Thus, we would be able to write the above as\n`git diff --merge-base A B`, allowing us to gently deprecate\nrange-notation completely.\n\nSuggested-by: Jonathan Nieder \nSigned-off-by: Denton Liu \n---\n Documentation/git-diff.txt | 24 ++++++++++--\n builtin/diff.c | 59 ++++++++++++++++++++++++++++\n t/t4068-diff-symmetric.sh | 79 ++++++++++++++++++++++++++++++++++++++\n 3 files changed, 159 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt\nindex 8f7b4ed3ca..0031729794 100644\n--- a/Documentation/git-diff.txt\n+++ b/Documentation/git-diff.txt\n@@ -12,7 +12,7 @@ SYNOPSIS\n 'git diff' [] [] [--] [...]\n 'git diff' [] --cached [] [--] [...]\n 'git diff' [] [...] [--] [...]\n-'git diff' [] ... [--] [...]\n+'git diff' [] --merge-base [--cached] [ []] [--] [...]\n 'git diff' [] \n 'git diff' [] --no-index [--] \n \n@@ -63,6 +63,24 @@ files on disk.\n \tThis is to view the changes between two arbitrary\n \t.\n \n+'git diff' [] --merge-base [--cached] [ []] [--] [...]::\n+\n+\tIn this form, the \"before\" side will be the merge base of the\n+\ttwo given commits. If either commit is omitted, it will default\n+\tto HEAD.\n++\n+In the case where two commits are given, a diff is displayed between the\n+merge base and the second commit. `git diff --merge-base A B` is\n+equivalent to `git diff $(git merge-base A B) B`.\n++\n+In the case where one commit is given, a diff is displayed between the\n+merge base and the working tree or the index if `--cached` is given.\n+`git diff --merge-base A` is equivalent to `git diff $(git merge-base A\n+HEAD)`.\n++\n+In the case where no commits are given, this form behaves identically to\n+as if no `--merge-base` were supplied.\n+\n 'git diff' [] ... [--] [...]::\n \n \tThis form is to view the results of a merge commit. The first\n@@ -89,8 +107,8 @@ files on disk.\n \n Just in case you are doing something exotic, it should be\n noted that all of the in the above description, except\n-in the last two forms that use `..` notations, can be any\n-.\n+in the `--merge-base` case and the last two forms that use `..`\n+notations, can be any .\n \n For a more complete list of ways to spell , see\n \"SPECIFYING REVISIONS\" section in linkgit:gitrevisions[7].\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 0e086ed7c4..0af5a6c8c9 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -19,6 +19,7 @@\n #include \"builtin.h\"\n #include \"submodule.h\"\n #include \"oid-array.h\"\n+#include \"commit-reach.h\"\n \n #define DIFF_NO_INDEX_EXPLICIT 1\n #define DIFF_NO_INDEX_IMPLICIT 2\n@@ -371,6 +372,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \tint blobs = 0, paths = 0;\n \tstruct object_array_entry *blob[2];\n \tint nongit = 0, no_index = 0;\n+\tint merge_base = 0;\n \tint result = 0;\n \tstruct symdiff sdiff;\n \tstruct option options[] = {\n@@ -378,6 +380,8 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"compare the given paths on the filesystem\"),\n \t\t\t DIFF_NO_INDEX_EXPLICIT,\n \t\t\t PARSE_OPT_NONEG),\n+\t\tOPT_BOOL(0, \"merge-base\", &merge_base,\n+\t\t\t N_(\"use the merge base between the two commits as the diff base\")),\n \t\tOPT_END(),\n \t};\n \n@@ -457,6 +461,9 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \trev.diffopt.flags.allow_external = 1;\n \trev.diffopt.flags.allow_textconv = 1;\n \n+\tif (no_index && merge_base)\n+\t\tdie(_(\"--no-index and --merge-base are mutually exclusive\"));\n+\n \t/* If this is a no-index diff, just run it and exit there. */\n \tif (no_index)\n \t\texit(diff_no_index(&rev, no_index == DIFF_NO_INDEX_IMPLICIT,\n@@ -513,6 +520,58 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t}\n \n \tsymdiff_prepare(&rev, &sdiff);\n+\n+\tif (merge_base && rev.pending.nr) {\n+\t\tint i;\n+\t\tstruct commit *mb_child[2] = {0};\n+\t\tstruct commit_list *merge_bases;\n+\t\tint old_nr;\n+\n+\t\tfor (i = 0; i < rev.pending.nr; i++) {\n+\t\t\tstruct object *obj = rev.pending.objects[i].item;\n+\t\t\tif (obj->flags)\n+\t\t\t\tdie(_(\"--merge-base does not work with ranges\"));\n+\t\t\tif (obj->type != OBJ_COMMIT)\n+\t\t\t\tdie(_(\"--merge-base only works with commits\"));\n+\t\t}\n+\n+\t\t/*\n+\t\t * This check must go after the for loop above because A...B\n+\t\t * ranges produce three pending commits, resulting in a\n+\t\t * misleading error message.\n+\t\t */\n+\t\tif (rev.pending.nr > ARRAY_SIZE(mb_child))\n+\t\t\tdie(_(\"--merge-base does not work with more than two commits\"));\n+\n+\t\tfor (i = 0; i < rev.pending.nr; i++)\n+\t\t\tmb_child[i] = lookup_commit_reference(the_repository, &rev.pending.objects[i].item->oid);\n+\t\tif (rev.pending.nr < ARRAY_SIZE(mb_child)) {\n+\t\t\tstruct object_id oid;\n+\n+\t\t\tif (rev.pending.nr != 1)\n+\t\t\t\tBUG(\"unexpected rev.pending.nr: %d\", rev.pending.nr);\n+\n+\t\t\tif (get_oid(\"HEAD\", &oid))\n+\t\t\t\tdie(_(\"unable to get HEAD\"));\n+\n+\t\t\tmb_child[1] = lookup_commit_reference(the_repository, &oid);\n+\t\t}\n+\n+\t\tmerge_bases = repo_get_merge_bases(the_repository, mb_child[0], mb_child[1]);\n+\t\tif (!merge_bases)\n+\t\t\tdie(_(\"no merge base found\"));\n+\t\tif (merge_bases->next)\n+\t\t\tdie(_(\"multiple merge bases found\"));\n+\n+\t\told_nr = rev.pending.nr;\n+\t\trev.pending.nr = 1;\n+\t\tobject_array_pop(&rev.pending);\n+\t\tadd_object_array(&merge_bases->item->object, oid_to_hex(&merge_bases->item->object.oid), &rev.pending);\n+\t\trev.pending.nr = old_nr;\n+\n+\t\tfree_commit_list(merge_bases);\n+\t}\n+\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;\ndiff --git a/t/t4068-diff-symmetric.sh b/t/t4068-diff-symmetric.sh\nindex 60c506c2b2..0e43ed7660 100755\n--- a/t/t4068-diff-symmetric.sh\n+++ b/t/t4068-diff-symmetric.sh\n@@ -88,4 +88,83 @@ test_expect_success 'diff with ranges and extra arg' '\n \ttest_i18ngrep \"usage\" err\n '\n \n+test_expect_success 'diff --merge-base with two commits' '\n+\tgit diff commit-C master >expect &&\n+\tgit diff --merge-base br2 master >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'diff --merge-base with no commits' '\n+\tgit diff --merge-base >actual &&\n+\ttest_must_be_empty actual\n+'\n+\n+test_expect_success 'diff --merge-base with one commit' '\n+\tgit checkout master &&\n+\tgit diff commit-C >expect &&\n+\tgit diff --merge-base br2 >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'diff --merge-base with one commit and unstaged changes' '\n+\tgit checkout master &&\n+\ttest_when_finished git reset --hard &&\n+\techo unstaged >>c &&\n+\tgit diff commit-C >expect &&\n+\tgit diff --merge-base br2 >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'diff --merge-base with one commit and staged and unstaged changes' '\n+\tgit checkout master &&\n+\ttest_when_finished git reset --hard &&\n+\techo staged >>c &&\n+\tgit add c &&\n+\techo unstaged >>c &&\n+\tgit diff commit-C >expect &&\n+\tgit diff --merge-base br2 >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'diff --merge-base --cached with one commit and staged and unstaged changes' '\n+\tgit checkout master &&\n+\ttest_when_finished git reset --hard &&\n+\techo staged >>c &&\n+\tgit add c &&\n+\techo unstaged >>c &&\n+\tgit diff --cached commit-C >expect &&\n+\tgit diff --cached --merge-base br2 >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'diff --merge-base with --no-index' '\n+\ttest_must_fail git diff --merge-base --no-index expect actual 2>err &&\n+\ttest_i18ngrep \"fatal: --no-index and --merge-base are mutually exclusive\" err\n+'\n+\n+test_expect_success 'diff --merge-base with range' '\n+\ttest_must_fail git diff --merge-base br2..br3 2>err &&\n+\ttest_i18ngrep \"fatal: --merge-base does not work with ranges\" err\n+'\n+\n+test_expect_success 'diff --merge-base non-commit' '\n+\ttest_must_fail git diff --merge-base master^{tree} 2>err &&\n+\ttest_i18ngrep \"fatal: --merge-base only works with commits\" err\n+'\n+\n+test_expect_success 'diff --merge-base with three commits' '\n+\ttest_must_fail git diff --merge-base br1 br2 master 2>err &&\n+\ttest_i18ngrep \"fatal: --merge-base does not work with more than two commits\" err\n+'\n+\n+test_expect_success 'diff --merge-base with no merge bases' '\n+\ttest_must_fail git diff --merge-base br2 br3 2>err &&\n+\ttest_i18ngrep \"fatal: no merge base found\" err\n+'\n+\n+test_expect_success 'diff --merge-base with multiple merge bases' '\n+\ttest_must_fail git diff --merge-base master br1 2>err &&\n+\ttest_i18ngrep \"fatal: multiple merge bases found\" err\n+'\n+\n test_done\n-- \n2.28.0.rc0.135.gc7877b767d\n\n"},{"id":"405103","messageId":"xmqqsgbu36vn.fsf@gitster.c.googlers.com","threadId":"54193","inReplyTo":"231ba3f661cc4aa7a55c44e339e187c6d70c5507.1599332861.git.liu.denton@gmail.com","subject":"Re: [PATCH 4/4] builtin/diff: learn --merge-base","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-06T21:58:04Z","receivedAt":"2020-09-06T21:58:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu writes:\n\n> +In the case where one commit is given, a diff is displayed between the\n> +merge base and the working tree or the index if `--cached` is given.\n\ns/merge base/& between the commit and the HEAD/\n\nAs \"merge base\" in the context of talking about \"one commit\" does\nnot make sense, such a clarification would be necessary.\n\nJonathan suggested \"--fork-point A B\"; this replaces it with\n\"--merge-base A B\". Neither is quite satisfying, but other\npeople might be able to come up with a better names.\n\nIn any case, \"git diff $(git merge-base MERGE_HEAD HEAD)\" while\nmerging to see how the changes in the working tree look like is\nsomething I've always felt cumbersome to spell, and this is a good\naddition.\n\n> +\t\tOPT_BOOL(0, \"merge-base\", &merge_base,\n> +\t\t\t N_(\"use the merge base between the two commits as the diff base\"))\n\nDefine \"diff base\". Eh, that was rhetoric---do not use such an\nundefined word. \"compare with the merge base between two commits\"\nmay work better.\n"},{"id":"405316","messageId":"cover.1599723087.git.liu.denton@gmail.com","threadId":"54193","inReplyTo":"cover.1599332861.git.liu.denton@gmail.com","subject":"[PATCH v2 0/4] builtin/diff: learn --merge-base","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-09-10T07:32:16Z","receivedAt":"2020-09-10T07:33:06Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The range-notation in `git diff` has been cited as a mistake since diff\ncompares two endpoints, not whole ranges.[0] In fact, the ranges seem\nto take on the opposite meanings when compared to range notation in\n`git log`.\n\nIn an effort to reduce the use of range notation as much as possible,\nintroduce the `--merge-base` flag, slightly modified from a suggestion\nby Jonathan Nieder.[1] This flag allows us to replace the first commit\ngiven on the command-line with its merge base between the first and\nsecond commits. This allows us to gently deprecate the `...` form\nentirely, although that is left as an exercise to the reader ;)\n\nOne additional bonus is that this flag allows the \"after\" side to be not\njust constrained to a commit (like with `...` notation). It can now be\nthe working tree or the index as well.\n\nThe `--merge-base` name isn't very satisfying. If anyone has any better\nsuggestions, please let me know.\n\nChanges since v1:\n\n* Implement Junio's documentation suggestions\n\n* Update git diff's usage to include this option\n\n[0]: https://lore.kernel.org/git/xmqqy2v26hu0.fsf@gitster-ct.c.googlers.com/\n[1]: https://lore.kernel.org/git/20191223215928.GB38316@google.com/\n\nDenton Liu (4):\n t4068: remove unnecessary >tmp\n git-diff.txt: backtick quote command text\n builtin/diff: parse --no-index using parse_options()\n builtin/diff: learn --merge-base\n\n Documentation/git-diff.txt | 40 +++++++++++-----\n builtin/diff.c | 94 +++++++++++++++++++++++++++++++++-----\n diff-no-index.c | 15 ++----\n t/t4068-diff-symmetric.sh | 89 ++++++++++++++++++++++++++++++++++--\n 4 files changed, 198 insertions(+), 40 deletions(-)\n\nRange-diff against v1:\n1: 231ba3f661 ! 1: 4f219cf0d1 builtin/diff: learn --merge-base\n @@ Commit message\n \n Suggested-by: Jonathan Nieder \n \n +\n + ## Notes ##\n + The `--merge-base` name isn't very satisfying. If anyone has any\n + suggestions for alternative names, please let me know.\n +\n ## Documentation/git-diff.txt ##\n @@ Documentation/git-diff.txt: SYNOPSIS\n 'git diff' [] [] [--] [...]\n @@ Documentation/git-diff.txt: files on disk.\n +equivalent to `git diff $(git merge-base A B) B`.\n ++\n +In the case where one commit is given, a diff is displayed between the\n -+merge base and the working tree or the index if `--cached` is given.\n -+`git diff --merge-base A` is equivalent to `git diff $(git merge-base A\n -+HEAD)`.\n ++merge base of the commit and the HEAD and the working tree or the index\n ++if `--cached` is given. `git diff --merge-base A` is equivalent to `git\n ++diff $(git merge-base A HEAD)`.\n ++\n +In the case where no commits are given, this form behaves identically to\n +as if no `--merge-base` were supplied.\n @@ builtin/diff.c\n \n #define DIFF_NO_INDEX_EXPLICIT 1\n #define DIFF_NO_INDEX_IMPLICIT 2\n +@@ builtin/diff.c: static const char builtin_diff_usage[] =\n + \"git diff [] [] [--] [...]\\n\"\n + \" or: git diff [] --cached [] [--] [...]\\n\"\n + \" or: git diff [] [...] [--] [...]\\n\"\n +-\" or: git diff [] ...] [--] [...]\\n\"\n ++\" or: git diff [] --merge-base [ []] [--] [...]\\n\"\n + \" or: git diff [] ]\\n\"\n + \" or: git diff [] --no-index [--] ]\\n\"\n + COMMON_DIFF_OPTIONS_HELP;\n @@ builtin/diff.c: int cmd_diff(int argc, const char **argv, const char *prefix)\n \tint blobs = 0, paths = 0;\n \tstruct object_array_entry *blob[2];\n @@ builtin/diff.c: int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\t\t DIFF_NO_INDEX_EXPLICIT,\n \t\t\t PARSE_OPT_NONEG),\n +\t\tOPT_BOOL(0, \"merge-base\", &merge_base,\n -+\t\t\t N_(\"use the merge base between the two commits as the diff base\")),\n ++\t\t\t N_(\"compare with the merge base between two commits\")),\n \t\tOPT_END(),\n \t};\n \n-- \n2.28.0.618.gf4bc123cb7\n\n"},{"id":"405317","messageId":"80e9066a59524ededbb0d0191a93e6e3154c5623.1599723087.git.liu.denton@gmail.com","threadId":"54193","inReplyTo":"cover.1599723087.git.liu.denton@gmail.com","subject":"[PATCH v2 1/4] t4068: remove unnecessary >tmp","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-09-10T07:32:17Z","receivedAt":"2020-09-10T07:33:07Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The many `git diff` invocations have a `>tmp` redirection even though\nthe file is not being used afterwards. Remove these unnecessary\nredirections.\n\nSigned-off-by: Denton Liu \n---\n t/t4068-diff-symmetric.sh | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t4068-diff-symmetric.sh b/t/t4068-diff-symmetric.sh\nindex 31d17a5af0..60c506c2b2 100755\n--- a/t/t4068-diff-symmetric.sh\n+++ b/t/t4068-diff-symmetric.sh\n@@ -64,27 +64,27 @@ test_expect_success 'diff with two merge bases' '\n '\n \n test_expect_success 'diff with no merge bases' '\n-\ttest_must_fail git diff br2...br3 >tmp 2>err &&\n+\ttest_must_fail git diff br2...br3 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_must_fail git diff br1...master br2...br3 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_must_fail git diff master br1...master 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_must_fail git diff master br1..master br2..br3 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_must_fail git diff master br1..master commit-D 2>err &&\n \ttest_i18ngrep \"usage\" err\n '\n \n-- \n2.28.0.618.gf4bc123cb7\n\n"},{"id":"405318","messageId":"8e72bd8fea276d40729705eb8dcc63bcc9faef2c.1599723087.git.liu.denton@gmail.com","threadId":"54193","inReplyTo":"cover.1599723087.git.liu.denton@gmail.com","subject":"[PATCH v2 2/4] git-diff.txt: backtick quote command text","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-09-10T07:32:18Z","receivedAt":"2020-09-10T07:33:08Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The modern way to quote commands in the documentation is to use\nbackticks instead of double-quotes as this renders the text with the\ncode style. Convert double-quoted command text to backtick-quoted\ncommands. While we're at it, quote one instance of `^@`.\n\nSigned-off-by: Denton Liu \n---\n Documentation/git-diff.txt | 18 +++++++++---------\n 1 file changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt\nindex 727f24d16e..8f7b4ed3ca 100644\n--- a/Documentation/git-diff.txt\n+++ b/Documentation/git-diff.txt\n@@ -68,13 +68,13 @@ files on disk.\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+\tthe desired set of revisions is to use the `^@` 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 is synonymous to the earlier form (without the \"..\") for\n+\tThis is synonymous to the earlier form (without the `..`) for\n \tviewing the changes between two arbitrary . If on\n \tone side is omitted, it will have the same effect as\n \tusing HEAD instead.\n@@ -83,20 +83,20 @@ files on disk.\n \n \tThis form is to view the changes on the branch containing\n \tand up to the second , starting at a common ancestor\n-\tof both . \"git diff A\\...B\" is equivalent to\n-\t\"git diff $(git merge-base A B) B\". You can omit any one\n+\tof both . `git diff A...B` is equivalent to\n+\t`git diff $(git merge-base A B) B`. You can omit any one\n \tof , which has the same effect as using HEAD instead.\n \n Just in case you are doing something exotic, it should be\n noted that all of the in the above description, except\n-in the last two forms that use \"..\" notations, can be any\n+in the last two forms that use `..` notations, can be any\n .\n \n For a more complete list of ways to spell , see\n \"SPECIFYING REVISIONS\" section in linkgit:gitrevisions[7].\n However, \"diff\" is about comparing two _endpoints_, not ranges,\n-and the range notations (\"..\" and\n-\"\\...\") do not mean a range as defined in the\n+and the range notations (`..` and\n+`...`) do not mean a range as defined in the\n \"SPECIFYING RANGES\" section in linkgit:gitrevisions[7].\n \n 'git diff' [] ::\n@@ -144,9 +144,9 @@ $ git diff HEAD <3>\n +\n <1> Changes in the working tree not yet staged for the next commit.\n <2> Changes between the index and your last commit; what you\n- would be committing if you run \"git commit\" without \"-a\" option.\n+ would be committing if you run `git commit` without `-a` option.\n <3> Changes in the working tree since your last commit; what you\n- would be committing if you run \"git commit -a\"\n+ would be committing if you run `git commit -a`\n \n Comparing with arbitrary commits::\n +\n-- \n2.28.0.618.gf4bc123cb7\n\n"},{"id":"405319","messageId":"ea6717e7b3a8b89fc3750505678321803cde78dc.1599723087.git.liu.denton@gmail.com","threadId":"54193","inReplyTo":"cover.1599723087.git.liu.denton@gmail.com","subject":"[PATCH v2 3/4] builtin/diff: parse --no-index using parse_options()","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-09-10T07:32:19Z","receivedAt":"2020-09-10T07:33:31Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Instead of parsing the `--no-index` option with a plain strcmp, use\nparse_options() to parse options. This allows us to easily add more\noptions in a future commit.\n\nAs a result of this change, `--no-index` is removed from `argv` so, as a\nresult, we no longer need to handle it in diff_no_index().\n\nSigned-off-by: Denton Liu \n---\n builtin/diff.c | 33 ++++++++++++++++++++++-----------\n diff-no-index.c | 15 +++------------\n 2 files changed, 25 insertions(+), 23 deletions(-)\n\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex cb98811c21..0e086ed7c4 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -373,6 +373,13 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \tint nongit = 0, no_index = 0;\n \tint result = 0;\n \tstruct symdiff sdiff;\n+\tstruct option options[] = {\n+\t\tOPT_SET_INT_F(0, \"no-index\", &no_index,\n+\t\t\t N_(\"compare the given paths on the filesystem\"),\n+\t\t\t DIFF_NO_INDEX_EXPLICIT,\n+\t\t\t PARSE_OPT_NONEG),\n+\t\tOPT_END(),\n+\t};\n \n \t/*\n \t * We could get N tree-ish in the rev.pending_objects list.\n@@ -406,21 +413,25 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t * Other cases are errors.\n \t */\n \n-\t/* Were we asked to do --no-index explicitly? */\n-\tfor (i = 1; i < argc; i++) {\n-\t\tif (!strcmp(argv[i], \"--\")) {\n-\t\t\ti++;\n-\t\t\tbreak;\n-\t\t}\n-\t\tif (!strcmp(argv[i], \"--no-index\"))\n-\t\t\tno_index = DIFF_NO_INDEX_EXPLICIT;\n-\t\tif (argv[i][0] != '-')\n-\t\t\tbreak;\n-\t}\n+\targc = parse_options(argc, argv, prefix, options, NULL,\n+\t\t\t PARSE_OPT_KEEP_DASHDASH |\n+\t\t\t PARSE_OPT_KEEP_ARGV0 |\n+\t\t\t PARSE_OPT_KEEP_UNKNOWN |\n+\t\t\t PARSE_OPT_NO_INTERNAL_HELP);\n \n \tprefix = setup_git_directory_gently(&nongit);\n \n \tif (!no_index) {\n+\t\tint i;\n+\t\tfor (i = 1; i < argc; i++) {\n+\t\t\tif (!strcmp(argv[i], \"--\")) {\n+\t\t\t\ti++;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t\tif (argv[i][0] != '-')\n+\t\t\t\tbreak;\n+\t\t}\n+\n \t\t/*\n \t\t * Treat git diff with at least one path outside of the\n \t\t * repo the same as if the command would have been executed\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex 7814eabfe0..da82e2d090 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -243,28 +243,19 @@ int diff_no_index(struct rev_info *revs,\n \t\t int implicit_no_index,\n \t\t int argc, const char **argv)\n {\n-\tint i, no_index;\n+\tint i;\n \tconst char *paths[2];\n \tstruct strbuf replacement = STRBUF_INIT;\n \tconst char *prefix = revs->prefix;\n-\tstruct option no_index_options[] = {\n-\t\tOPT_BOOL_F(0, \"no-index\", &no_index, \"\",\n-\t\t\t PARSE_OPT_NONEG | PARSE_OPT_HIDDEN),\n-\t\tOPT_END(),\n-\t};\n-\tstruct option *options;\n \n-\toptions = parse_options_concat(no_index_options,\n-\t\t\t\t revs->diffopt.parseopts);\n-\targc = parse_options(argc, argv, revs->prefix, options,\n+\targc = parse_options(argc, argv, revs->prefix, revs->diffopt.parseopts,\n \t\t\t diff_no_index_usage, 0);\n \tif (argc != 2) {\n \t\tif (implicit_no_index)\n \t\t\twarning(_(\"Not a git repository. Use --no-index to \"\n \t\t\t\t \"compare two paths outside a working tree\"));\n-\t\tusage_with_options(diff_no_index_usage, options);\n+\t\tusage_with_options(diff_no_index_usage, revs->diffopt.parseopts);\n \t}\n-\tFREE_AND_NULL(options);\n \tfor (i = 0; i < 2; i++) {\n \t\tconst char *p = argv[argc - 2 + i];\n \t\tif (!strcmp(p, \"-\"))\n-- \n2.28.0.618.gf4bc123cb7\n\n"},{"id":"405320","messageId":"4f219cf0d1df1b92d2bd49e3449a4c387056a6cd.1599723087.git.liu.denton@gmail.com","threadId":"54193","inReplyTo":"cover.1599723087.git.liu.denton@gmail.com","subject":"[PATCH v2 4/4] builtin/diff: learn --merge-base","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-09-10T07:32:20Z","receivedAt":"2020-09-10T07:33:33Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In order to get the diff between a commit and its merge base, the\ncurrently preferred method is to use `git diff A...B`. However, the\nrange-notation with diff has, time and time again, been noted as a point\nof confusion and thus, it should be avoided. Although we have a\nsubstitute for the double-dot notation, we don't have any replacement\nfor the triple-dot notation.\n\nIntroduce the `--merge-base` flag as a replacement for triple-dot\nnotation. Thus, we would be able to write the above as\n`git diff --merge-base A B`, allowing us to gently deprecate\nrange-notation completely.\n\nSuggested-by: Jonathan Nieder \nSigned-off-by: Denton Liu \n---\n\nNotes:\n The `--merge-base` name isn't very satisfying. If anyone has any\n suggestions for alternative names, please let me know.\n\n Documentation/git-diff.txt | 24 ++++++++++--\n builtin/diff.c | 61 ++++++++++++++++++++++++++++-\n t/t4068-diff-symmetric.sh | 79 ++++++++++++++++++++++++++++++++++++++\n 3 files changed, 160 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt\nindex 8f7b4ed3ca..2cab6eabe1 100644\n--- a/Documentation/git-diff.txt\n+++ b/Documentation/git-diff.txt\n@@ -12,7 +12,7 @@ SYNOPSIS\n 'git diff' [] [] [--] [...]\n 'git diff' [] --cached [] [--] [...]\n 'git diff' [] [...] [--] [...]\n-'git diff' [] ... [--] [...]\n+'git diff' [] --merge-base [--cached] [ []] [--] [...]\n 'git diff' [] \n 'git diff' [] --no-index [--] \n \n@@ -63,6 +63,24 @@ files on disk.\n \tThis is to view the changes between two arbitrary\n \t.\n \n+'git diff' [] --merge-base [--cached] [ []] [--] [...]::\n+\n+\tIn this form, the \"before\" side will be the merge base of the\n+\ttwo given commits. If either commit is omitted, it will default\n+\tto HEAD.\n++\n+In the case where two commits are given, a diff is displayed between the\n+merge base and the second commit. `git diff --merge-base A B` is\n+equivalent to `git diff $(git merge-base A B) B`.\n++\n+In the case where one commit is given, a diff is displayed between the\n+merge base of the commit and the HEAD and the working tree or the index\n+if `--cached` is given. `git diff --merge-base A` is equivalent to `git\n+diff $(git merge-base A HEAD)`.\n++\n+In the case where no commits are given, this form behaves identically to\n+as if no `--merge-base` were supplied.\n+\n 'git diff' [] ... [--] [...]::\n \n \tThis form is to view the results of a merge commit. The first\n@@ -89,8 +107,8 @@ files on disk.\n \n Just in case you are doing something exotic, it should be\n noted that all of the in the above description, except\n-in the last two forms that use `..` notations, can be any\n-.\n+in the `--merge-base` case and the last two forms that use `..`\n+notations, can be any .\n \n For a more complete list of ways to spell , see\n \"SPECIFYING REVISIONS\" section in linkgit:gitrevisions[7].\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 0e086ed7c4..448d2dd69a 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -19,6 +19,7 @@\n #include \"builtin.h\"\n #include \"submodule.h\"\n #include \"oid-array.h\"\n+#include \"commit-reach.h\"\n \n #define DIFF_NO_INDEX_EXPLICIT 1\n #define DIFF_NO_INDEX_IMPLICIT 2\n@@ -27,7 +28,7 @@ static const char builtin_diff_usage[] =\n \"git diff [] [] [--] [...]\\n\"\n \" or: git diff [] --cached [] [--] [...]\\n\"\n \" or: git diff [] [...] [--] [...]\\n\"\n-\" or: git diff [] ...] [--] [...]\\n\"\n+\" or: git diff [] --merge-base [ []] [--] [...]\\n\"\n \" or: git diff [] ]\\n\"\n \" or: git diff [] --no-index [--] ]\\n\"\n COMMON_DIFF_OPTIONS_HELP;\n@@ -371,6 +372,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \tint blobs = 0, paths = 0;\n \tstruct object_array_entry *blob[2];\n \tint nongit = 0, no_index = 0;\n+\tint merge_base = 0;\n \tint result = 0;\n \tstruct symdiff sdiff;\n \tstruct option options[] = {\n@@ -378,6 +380,8 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"compare the given paths on the filesystem\"),\n \t\t\t DIFF_NO_INDEX_EXPLICIT,\n \t\t\t PARSE_OPT_NONEG),\n+\t\tOPT_BOOL(0, \"merge-base\", &merge_base,\n+\t\t\t N_(\"compare with the merge base between two commits\")),\n \t\tOPT_END(),\n \t};\n \n@@ -457,6 +461,9 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \trev.diffopt.flags.allow_external = 1;\n \trev.diffopt.flags.allow_textconv = 1;\n \n+\tif (no_index && merge_base)\n+\t\tdie(_(\"--no-index and --merge-base are mutually exclusive\"));\n+\n \t/* If this is a no-index diff, just run it and exit there. */\n \tif (no_index)\n \t\texit(diff_no_index(&rev, no_index == DIFF_NO_INDEX_IMPLICIT,\n@@ -513,6 +520,58 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t}\n \n \tsymdiff_prepare(&rev, &sdiff);\n+\n+\tif (merge_base && rev.pending.nr) {\n+\t\tint i;\n+\t\tstruct commit *mb_child[2] = {0};\n+\t\tstruct commit_list *merge_bases;\n+\t\tint old_nr;\n+\n+\t\tfor (i = 0; i < rev.pending.nr; i++) {\n+\t\t\tstruct object *obj = rev.pending.objects[i].item;\n+\t\t\tif (obj->flags)\n+\t\t\t\tdie(_(\"--merge-base does not work with ranges\"));\n+\t\t\tif (obj->type != OBJ_COMMIT)\n+\t\t\t\tdie(_(\"--merge-base only works with commits\"));\n+\t\t}\n+\n+\t\t/*\n+\t\t * This check must go after the for loop above because A...B\n+\t\t * ranges produce three pending commits, resulting in a\n+\t\t * misleading error message.\n+\t\t */\n+\t\tif (rev.pending.nr > ARRAY_SIZE(mb_child))\n+\t\t\tdie(_(\"--merge-base does not work with more than two commits\"));\n+\n+\t\tfor (i = 0; i < rev.pending.nr; i++)\n+\t\t\tmb_child[i] = lookup_commit_reference(the_repository, &rev.pending.objects[i].item->oid);\n+\t\tif (rev.pending.nr < ARRAY_SIZE(mb_child)) {\n+\t\t\tstruct object_id oid;\n+\n+\t\t\tif (rev.pending.nr != 1)\n+\t\t\t\tBUG(\"unexpected rev.pending.nr: %d\", rev.pending.nr);\n+\n+\t\t\tif (get_oid(\"HEAD\", &oid))\n+\t\t\t\tdie(_(\"unable to get HEAD\"));\n+\n+\t\t\tmb_child[1] = lookup_commit_reference(the_repository, &oid);\n+\t\t}\n+\n+\t\tmerge_bases = repo_get_merge_bases(the_repository, mb_child[0], mb_child[1]);\n+\t\tif (!merge_bases)\n+\t\t\tdie(_(\"no merge base found\"));\n+\t\tif (merge_bases->next)\n+\t\t\tdie(_(\"multiple merge bases found\"));\n+\n+\t\told_nr = rev.pending.nr;\n+\t\trev.pending.nr = 1;\n+\t\tobject_array_pop(&rev.pending);\n+\t\tadd_object_array(&merge_bases->item->object, oid_to_hex(&merge_bases->item->object.oid), &rev.pending);\n+\t\trev.pending.nr = old_nr;\n+\n+\t\tfree_commit_list(merge_bases);\n+\t}\n+\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;\ndiff --git a/t/t4068-diff-symmetric.sh b/t/t4068-diff-symmetric.sh\nindex 60c506c2b2..0e43ed7660 100755\n--- a/t/t4068-diff-symmetric.sh\n+++ b/t/t4068-diff-symmetric.sh\n@@ -88,4 +88,83 @@ test_expect_success 'diff with ranges and extra arg' '\n \ttest_i18ngrep \"usage\" err\n '\n \n+test_expect_success 'diff --merge-base with two commits' '\n+\tgit diff commit-C master >expect &&\n+\tgit diff --merge-base br2 master >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'diff --merge-base with no commits' '\n+\tgit diff --merge-base >actual &&\n+\ttest_must_be_empty actual\n+'\n+\n+test_expect_success 'diff --merge-base with one commit' '\n+\tgit checkout master &&\n+\tgit diff commit-C >expect &&\n+\tgit diff --merge-base br2 >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'diff --merge-base with one commit and unstaged changes' '\n+\tgit checkout master &&\n+\ttest_when_finished git reset --hard &&\n+\techo unstaged >>c &&\n+\tgit diff commit-C >expect &&\n+\tgit diff --merge-base br2 >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'diff --merge-base with one commit and staged and unstaged changes' '\n+\tgit checkout master &&\n+\ttest_when_finished git reset --hard &&\n+\techo staged >>c &&\n+\tgit add c &&\n+\techo unstaged >>c &&\n+\tgit diff commit-C >expect &&\n+\tgit diff --merge-base br2 >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'diff --merge-base --cached with one commit and staged and unstaged changes' '\n+\tgit checkout master &&\n+\ttest_when_finished git reset --hard &&\n+\techo staged >>c &&\n+\tgit add c &&\n+\techo unstaged >>c &&\n+\tgit diff --cached commit-C >expect &&\n+\tgit diff --cached --merge-base br2 >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'diff --merge-base with --no-index' '\n+\ttest_must_fail git diff --merge-base --no-index expect actual 2>err &&\n+\ttest_i18ngrep \"fatal: --no-index and --merge-base are mutually exclusive\" err\n+'\n+\n+test_expect_success 'diff --merge-base with range' '\n+\ttest_must_fail git diff --merge-base br2..br3 2>err &&\n+\ttest_i18ngrep \"fatal: --merge-base does not work with ranges\" err\n+'\n+\n+test_expect_success 'diff --merge-base non-commit' '\n+\ttest_must_fail git diff --merge-base master^{tree} 2>err &&\n+\ttest_i18ngrep \"fatal: --merge-base only works with commits\" err\n+'\n+\n+test_expect_success 'diff --merge-base with three commits' '\n+\ttest_must_fail git diff --merge-base br1 br2 master 2>err &&\n+\ttest_i18ngrep \"fatal: --merge-base does not work with more than two commits\" err\n+'\n+\n+test_expect_success 'diff --merge-base with no merge bases' '\n+\ttest_must_fail git diff --merge-base br2 br3 2>err &&\n+\ttest_i18ngrep \"fatal: no merge base found\" err\n+'\n+\n+test_expect_success 'diff --merge-base with multiple merge bases' '\n+\ttest_must_fail git diff --merge-base master br1 2>err &&\n+\ttest_i18ngrep \"fatal: multiple merge bases found\" err\n+'\n+\n test_done\n-- \n2.28.0.618.gf4bc123cb7\n\n"},{"id":"405349","messageId":"xmqqo8mdpji9.fsf@gitster.c.googlers.com","threadId":"54193","inReplyTo":"ea6717e7b3a8b89fc3750505678321803cde78dc.1599723087.git.liu.denton@gmail.com","subject":"Re: [PATCH v2 3/4] builtin/diff: parse --no-index using parse_options()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-10T18:35:42Z","receivedAt":"2020-09-10T18:36:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu writes:\n\n> Instead of parsing the `--no-index` option with a plain strcmp, use\n> parse_options() to parse options. This allows us to easily add more\n> options in a future commit.\n>\n> As a result of this change, `--no-index` is removed from `argv` so, as a\n> result, we no longer need to handle it in diff_no_index().\n>\n> Signed-off-by: Denton Liu \n> ---\n> builtin/diff.c | 33 ++++++++++++++++++++++-----------\n> diff-no-index.c | 15 +++------------\n> 2 files changed, 25 insertions(+), 23 deletions(-)\n>\n> diff --git a/builtin/diff.c b/builtin/diff.c\n> index cb98811c21..0e086ed7c4 100644\n> --- a/builtin/diff.c\n> +++ b/builtin/diff.c\n> @@ -373,6 +373,13 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n> \tint nongit = 0, no_index = 0;\n> \tint result = 0;\n> \tstruct symdiff sdiff;\n> +\tstruct option options[] = {\n> +\t\tOPT_SET_INT_F(0, \"no-index\", &no_index,\n> +\t\t\t N_(\"compare the given paths on the filesystem\"),\n> +\t\t\t DIFF_NO_INDEX_EXPLICIT,\n> +\t\t\t PARSE_OPT_NONEG),\n> +\t\tOPT_END(),\n> +\t};\n\nWhat's the reasoning behind teaching the \"--merge-base\" option only\nto \"diff\" and not allowing \"diff-index\" and \"diff-tree\" to also\nbenefit from the new support? It should be taught to be handled by\ndiff.c::diff_opt_parse() instead, like all other \"diff\" options. I\nsimply do not see what makes \"--merge-base\" so special compared to\nother options like \"-R\", \"--color-words\", etc.\n\nAnd with that, this step will become totally unnecessary, because\nthe options[] array will still have only the \"no-index\" option and\nnothing else.\n\nUNLESS we are planning to use this array to parse all diff options\nhere, whether it is for the no-index mode or the normal invocation\nof diff, but that is unlikely to happen---we'd still share many\noptions between the plumbing \"diff-*\" family and Porcelain \"diff\"\ncommands.\n\nThanks.\n"},{"id":"405487","messageId":"20200913083122.GA189023@generichostname","threadId":"54193","inReplyTo":"xmqqo8mdpji9.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 3/4] builtin/diff: parse --no-index using parse_options()","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-09-13T08:31:22Z","receivedAt":"2020-09-13T08:31:30Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Junio,\n\nOn Thu, Sep 10, 2020 at 11:35:42AM -0700, Junio C Hamano wrote:\n> Denton Liu writes:\n> \n> > Instead of parsing the `--no-index` option with a plain strcmp, use\n> > parse_options() to parse options. This allows us to easily add more\n> > options in a future commit.\n> >\n> > As a result of this change, `--no-index` is removed from `argv` so, as a\n> > result, we no longer need to handle it in diff_no_index().\n> >\n> > Signed-off-by: Denton Liu \n> > ---\n> > builtin/diff.c | 33 ++++++++++++++++++++++-----------\n> > diff-no-index.c | 15 +++------------\n> > 2 files changed, 25 insertions(+), 23 deletions(-)\n> >\n> > diff --git a/builtin/diff.c b/builtin/diff.c\n> > index cb98811c21..0e086ed7c4 100644\n> > --- a/builtin/diff.c\n> > +++ b/builtin/diff.c\n> > @@ -373,6 +373,13 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n> > \tint nongit = 0, no_index = 0;\n> > \tint result = 0;\n> > \tstruct symdiff sdiff;\n> > +\tstruct option options[] = {\n> > +\t\tOPT_SET_INT_F(0, \"no-index\", &no_index,\n> > +\t\t\t N_(\"compare the given paths on the filesystem\"),\n> > +\t\t\t DIFF_NO_INDEX_EXPLICIT,\n> > +\t\t\t PARSE_OPT_NONEG),\n> > +\t\tOPT_END(),\n> > +\t};\n> \n> What's the reasoning behind teaching the \"--merge-base\" option only\n> to \"diff\" and not allowing \"diff-index\" and \"diff-tree\" to also\n> benefit from the new support?\n\nThis is a good idea. Initially, I was hesitant because the manpages for\ndiff-index and diff-tree both list the argument as but I think\nit makes sense that these commands should accept --merge-base too.\n\n> It should be taught to be handled by\n> diff.c::diff_opt_parse() instead, like all other \"diff\" options. I\n> simply do not see what makes \"--merge-base\" so special compared to\n> other options like \"-R\", \"--color-words\", etc.\n\nUnfortunately, despite the above, I don't think we'll be able to handle\nthis in diff_opt_parse(). --merge-base won't be common to all diff\nmodes. It'll only work with diff-index and diff-tree, so it'll have to\nbe handled in those modes specifically.\n\nThanks,\nDenton\n"},{"id":"405511","messageId":"xmqqsgblmjuv.fsf@gitster.c.googlers.com","threadId":"54193","inReplyTo":"20200913083122.GA189023@generichostname","subject":"Re: [PATCH v2 3/4] builtin/diff: parse --no-index using parse_options()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-13T21:45:28Z","receivedAt":"2020-09-13T21:45:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu writes:\n\n>> It should be taught to be handled by\n>> diff.c::diff_opt_parse() instead, like all other \"diff\" options. I\n>> simply do not see what makes \"--merge-base\" so special compared to\n>> other options like \"-R\", \"--color-words\", etc.\n>\n> Unfortunately, despite the above, I don't think we'll be able to handle\n> this in diff_opt_parse(). --merge-base won't be common to all diff\n> modes. It'll only work with diff-index and diff-tree, so it'll have to\n> be handled in those modes specifically.\n\nYes, I think we can follow precedences, like the \"--cached\" option,\n\"builtin/diff-index.c::cmd_diff_index()\" knows about it and it is\nalso handled by \"builtin/diff.c::builtin_diff_index()\".\n\nThanks.\n\n\n"},{"id":"405718","messageId":"cover.1600328335.git.liu.denton@gmail.com","threadId":"54193","inReplyTo":"cover.1599723087.git.liu.denton@gmail.com","subject":"[PATCH v3 00/10] builtin/diff: learn --merge-base","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-09-17T07:44:04Z","receivedAt":"2020-09-17T07:44:49Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The range-notation in `git diff` has been cited as a mistake since diff\ncompares two endpoints, not whole ranges.[0] In fact, the ranges seem\nto take on the opposite meanings when compared to range notation in\n`git log`.\n\nIn an effort to reduce the use of range notation as much as possible,\nintroduce the `--merge-base` flag, slightly modified from a suggestion\nby Jonathan Nieder.[1] This flag allows us to replace the first commit\ngiven on the command-line with its merge base between the first and\nsecond commits. This allows us to gently deprecate the `...` form\nentirely, although that is left as an exercise to the reader ;)\n\nOne additional bonus is that this flag allows the \"after\" side to be not\njust constrained to a commit (like with `...` notation). It can now be\nthe working tree or the index as well.\n\nThe `--merge-base` name isn't very satisfying. If anyone has any better\nsuggestions, please let me know.\n\nChanges since v1:\n\n* Implement Junio's documentation suggestions\n\n* Update git diff's usage to include this option\n\nChanges since v2:\n\n* Teach diff-index and diff-tree the --merge-base option as well\n\n[0]: https://lore.kernel.org/git/xmqqy2v26hu0.fsf@gitster-ct.c.googlers.com/\n[1]: https://lore.kernel.org/git/20191223215928.GB38316@google.com/\n\nDenton Liu (10):\n t4068: remove unnecessary >tmp\n git-diff-index.txt: make --cached description a proper sentence\n git-diff.txt: backtick quote command text\n contrib/completion: extract common diff/difftool options\n diff-lib: accept option flags in run_diff_index()\n diff-lib: define diff_get_merge_base()\n t4068: add --merge-base tests\n builtin/diff-index: learn --merge-base\n builtin/diff-tree: learn --merge-base\n contrib/completion: complete `git diff --merge-base`\n\n Documentation/git-diff-index.txt | 9 +-\n Documentation/git-diff-tree.txt | 7 +-\n Documentation/git-diff.txt | 37 +++--\n builtin/diff-index.c | 10 +-\n builtin/diff-tree.c | 18 +++\n builtin/diff.c | 50 ++++---\n contrib/completion/git-completion.bash | 15 +-\n diff-lib.c | 65 ++++++++-\n diff.h | 6 +-\n t/t4068-diff-symmetric-merge-base.sh | 193 +++++++++++++++++++++++++\n t/t4068-diff-symmetric.sh | 91 ------------\n 11 files changed, 358 insertions(+), 143 deletions(-)\n create mode 100755 t/t4068-diff-symmetric-merge-base.sh\n delete mode 100755 t/t4068-diff-symmetric.sh\n\nRange-diff against v2:\n 1: 80e9066a59 = 1: 80e9066a59 t4068: remove unnecessary >tmp\n -: ---------- > 2: 21b20281e6 git-diff-index.txt: make --cached description a proper sentence\n 2: 8e72bd8fea = 3: ca9568c2ea git-diff.txt: backtick quote command text\n 3: ea6717e7b3 < -: ---------- builtin/diff: parse --no-index using parse_options()\n 4: 4f219cf0d1 < -: ---------- builtin/diff: learn --merge-base\n -: ---------- > 4: 1ac8459541 contrib/completion: extract common diff/difftool options\n -: ---------- > 5: 496908ac10 diff-lib: accept option flags in run_diff_index()\n -: ---------- > 6: 6aac57ca02 diff-lib: define diff_get_merge_base()\n -: ---------- > 7: c9225a0440 t4068: add --merge-base tests\n -: ---------- > 8: 1e4f805e57 builtin/diff-index: learn --merge-base\n -: ---------- > 9: c0d27b125e builtin/diff-tree: learn --merge-base\n -: ---------- > 10: 42a8c9b665 contrib/completion: complete `git diff --merge-base`\n-- \n2.28.0.618.gf4bc123cb7\n\n"},{"id":"405719","messageId":"80e9066a59524ededbb0d0191a93e6e3154c5623.1600328335.git.liu.denton@gmail.com","threadId":"54193","inReplyTo":"cover.1600328335.git.liu.denton@gmail.com","subject":"[PATCH v3 01/10] t4068: remove unnecessary >tmp","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-09-17T07:44:05Z","receivedAt":"2020-09-17T07:44:53Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The many `git diff` invocations have a `>tmp` redirection even though\nthe file is not being used afterwards. Remove these unnecessary\nredirections.\n\nSigned-off-by: Denton Liu \n---\n t/t4068-diff-symmetric.sh | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t4068-diff-symmetric.sh b/t/t4068-diff-symmetric.sh\nindex 31d17a5af0..60c506c2b2 100755\n--- a/t/t4068-diff-symmetric.sh\n+++ b/t/t4068-diff-symmetric.sh\n@@ -64,27 +64,27 @@ test_expect_success 'diff with two merge bases' '\n '\n \n test_expect_success 'diff with no merge bases' '\n-\ttest_must_fail git diff br2...br3 >tmp 2>err &&\n+\ttest_must_fail git diff br2...br3 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_must_fail git diff br1...master br2...br3 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_must_fail git diff master br1...master 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_must_fail git diff master br1..master br2..br3 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_must_fail git diff master br1..master commit-D 2>err &&\n \ttest_i18ngrep \"usage\" err\n '\n \n-- \n2.28.0.618.gf4bc123cb7\n\n"},{"id":"405720","messageId":"21b20281e6a1fa265d4853fa04cc71ba7e843267.1600328335.git.liu.denton@gmail.com","threadId":"54193","inReplyTo":"cover.1600328335.git.liu.denton@gmail.com","subject":"[PATCH v3 02/10] git-diff-index.txt: make --cached description a proper sentence","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-09-17T07:44:06Z","receivedAt":"2020-09-17T07:45:03Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Signed-off-by: Denton Liu \n---\n Documentation/git-diff-index.txt | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-diff-index.txt b/Documentation/git-diff-index.txt\nindex f4bd8155c0..25fe165f00 100644\n--- a/Documentation/git-diff-index.txt\n+++ b/Documentation/git-diff-index.txt\n@@ -27,7 +27,7 @@ include::diff-options.txt[]\n \tThe id of a tree object to diff against.\n \n --cached::\n-\tdo not consider the on-disk file at all\n+\tDo not consider the on-disk file at all.\n \n -m::\n \tBy default, files recorded in the index but not checked\n-- \n2.28.0.618.gf4bc123cb7\n\n"},{"id":"405721","messageId":"42a8c9b665c63d5e8135546538db69a366cd70da.1600328336.git.liu.denton@gmail.com","threadId":"54193","inReplyTo":"cover.1600328335.git.liu.denton@gmail.com","subject":"[PATCH v3 10/10] contrib/completion: complete `git diff --merge-base`","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-09-17T07:44:14Z","receivedAt":"2020-09-17T07:45:10Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Signed-off-by: Denton Liu \n---\n contrib/completion/git-completion.bash | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex f68c8e0646..679d1ec8a8 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -1692,7 +1692,7 @@ __git_diff_common_options=\"--stat --numstat --shortstat --summary\n \"\n \n __git_diff_difftool_options=\"--cached --staged --pickaxe-all --pickaxe-regex\n-\t\t\t--base --ours --theirs --no-index --relative\n+\t\t\t--base --ours --theirs --no-index --relative --merge-base\n \t\t\t$__git_diff_common_options\"\n \n _git_diff ()\n-- \n2.28.0.618.gf4bc123cb7\n\n"},{"id":"405722","messageId":"c0d27b125e969e13c52b0fa806a8e3caa8c20ac6.1600328336.git.liu.denton@gmail.com","threadId":"54193","inReplyTo":"cover.1600328335.git.liu.denton@gmail.com","subject":"[PATCH v3 09/10] builtin/diff-tree: learn --merge-base","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-09-17T07:44:13Z","receivedAt":"2020-09-17T07:45:20Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In order to get the diff between a commit and its merge base, the\ncurrently preferred method is to use `git diff A...B`. However, the\nrange-notation with diff has, time and time again, been noted as a point\nof confusion and thus, it should be avoided. Although we have a\nsubstitute for the double-dot notation, we don't have any replacement\nfor the triple-dot notation.\n\nIntroduce the --merge-base flag as a replacement for triple-dot\nnotation. Thus, we would be able to write the above as\n`git diff --merge-base A B`, allowing us to gently deprecate\nrange-notation completely.\n\nSigned-off-by: Denton Liu \n---\n Documentation/git-diff-tree.txt | 7 ++++-\n Documentation/git-diff.txt | 9 ++++---\n builtin/diff-tree.c | 18 +++++++++++++\n builtin/diff.c | 40 +++++++++++++++++++---------\n t/t4068-diff-symmetric-merge-base.sh | 34 +++++++++++++++++++++++\n 5 files changed, 91 insertions(+), 17 deletions(-)\n\ndiff --git a/Documentation/git-diff-tree.txt b/Documentation/git-diff-tree.txt\nindex 5c8a2a5e97..2fc24c542f 100644\n--- a/Documentation/git-diff-tree.txt\n+++ b/Documentation/git-diff-tree.txt\n@@ -10,7 +10,7 @@ SYNOPSIS\n --------\n [verse]\n 'git diff-tree' [--stdin] [-m] [-s] [-v] [--no-commit-id] [--pretty]\n-\t [-t] [-r] [-c | --cc] [--combined-all-paths] [--root]\n+\t [-t] [-r] [-c | --cc] [--combined-all-paths] [--root] [--merge-base]\n \t [] [] [...]\n \n DESCRIPTION\n@@ -43,6 +43,11 @@ include::diff-options.txt[]\n \tWhen `--root` is specified the initial commit will be shown as a big\n \tcreation event. This is equivalent to a diff against the NULL tree.\n \n+--merge-base::\n+\tInstead of comparing the s directly, use the merge\n+\tbase between the two s as the \"before\" side. There\n+\tmust be two s given and they must both be commits.\n+\n --stdin::\n \tWhen `--stdin` is specified, the command does not take\n \t arguments from the command line. Instead, it\ndiff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt\nindex 762ee6d074..d3b526e00a 100644\n--- a/Documentation/git-diff.txt\n+++ b/Documentation/git-diff.txt\n@@ -11,8 +11,7 @@ SYNOPSIS\n [verse]\n 'git diff' [] [] [--] [...]\n 'git diff' [] --cached [--merge-base] [] [--] [...]\n-'git diff' [] [...] [--] [...]\n-'git diff' [] ... [--] [...]\n+'git diff' [] [--merge-base] [...] [--] [...]\n 'git diff' [] \n 'git diff' [] --no-index [--] \n \n@@ -62,10 +61,14 @@ of and HEAD. `git diff --merge-base A` is equivalent to\n \tbranch name to compare with the tip of a different\n \tbranch.\n \n-'git diff' [] [--] [...]::\n+'git diff' [] [--merge-base] [--] [...]::\n \n \tThis is to view the changes between two arbitrary\n \t.\n++\n+If --merge-base is given, use the merge base of the two commits for the\n+\"before\" side. `git diff --merge-base A B` is equivalent to\n+`git diff $(git merge-base A B) B`.\n \n 'git diff' [] ... [--] [...]::\n \ndiff --git a/builtin/diff-tree.c b/builtin/diff-tree.c\nindex 802363d0a2..823d6678e5 100644\n--- a/builtin/diff-tree.c\n+++ b/builtin/diff-tree.c\n@@ -111,6 +111,7 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)\n \tstruct setup_revision_opt s_r_opt;\n \tstruct userformat_want w;\n \tint read_stdin = 0;\n+\tint merge_base = 0;\n \n \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n \t\tusage(diff_tree_usage);\n@@ -143,9 +144,26 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)\n \t\t\tread_stdin = 1;\n \t\t\tcontinue;\n \t\t}\n+\t\tif (!strcmp(arg, \"--merge-base\")) {\n+\t\t\tmerge_base = 1;\n+\t\t\tcontinue;\n+\t\t}\n \t\tusage(diff_tree_usage);\n \t}\n \n+\tif (read_stdin && merge_base)\n+\t\tdie(_(\"--stdin and --merge-base are mutually exclusive\"));\n+\n+\tif (merge_base) {\n+\t\tstruct object_id oid;\n+\n+\t\tif (opt->pending.nr != 2)\n+\t\t\tdie(_(\"--merge-base only works with two commits\"));\n+\n+\t\tdiff_get_merge_base(opt, &oid);\n+\t\topt->pending.objects[0].item = lookup_object(the_repository, &oid);\n+\t}\n+\n \t/*\n \t * NOTE! We expect \"a..b\" to expand to \"^a b\" but it is\n \t * perfectly valid for revision range parser to yield \"b ^a\",\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 1baea18ae0..ad78bc89b3 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -26,8 +26,7 @@\n static const char builtin_diff_usage[] =\n \"git diff [] [] [--] [...]\\n\"\n \" or: git diff [] --cached [] [--] [...]\\n\"\n-\" or: git diff [] [...] [--] [...]\\n\"\n-\" or: git diff [] ...] [--] [...]\\n\"\n+\" or: git diff [] [--merge-base] [...] [--] [...]\\n\"\n \" or: git diff [] ]\\n\"\n \" or: git diff [] --no-index [--] ]\\n\"\n COMMON_DIFF_OPTIONS_HELP;\n@@ -172,19 +171,34 @@ static int builtin_diff_tree(struct rev_info *revs,\n \t\t\t struct object_array_entry *ent1)\n {\n \tconst struct object_id *(oid[2]);\n-\tint swap = 0;\n+\tstruct object_id mb_oid;\n+\tint merge_base = 0;\n \n-\tif (argc > 1)\n-\t\tusage(builtin_diff_usage);\n+\twhile (1 < argc) {\n+\t\tconst char *arg = argv[1];\n+\t\tif (!strcmp(arg, \"--merge-base\"))\n+\t\t\tmerge_base = 1;\n+\t\telse\n+\t\t\tusage(builtin_diff_usage);\n+\t\targv++; argc--;\n+\t}\n \n-\t/*\n-\t * We saw two trees, ent0 and ent1. If ent1 is uninteresting,\n-\t * swap them.\n-\t */\n-\tif (ent1->item->flags & UNINTERESTING)\n-\t\tswap = 1;\n-\toid[swap] = &ent0->item->oid;\n-\toid[1 - swap] = &ent1->item->oid;\n+\tif (merge_base) {\n+\t\tdiff_get_merge_base(revs, &mb_oid);\n+\t\toid[0] = &mb_oid;\n+\t\toid[1] = &revs->pending.objects[1].item->oid;\n+\t} else {\n+\t\tint swap = 0;\n+\n+\t\t/*\n+\t\t * We saw two trees, ent0 and ent1. If ent1 is uninteresting,\n+\t\t * swap them.\n+\t\t */\n+\t\tif (ent1->item->flags & UNINTERESTING)\n+\t\t\tswap = 1;\n+\t\toid[swap] = &ent0->item->oid;\n+\t\toid[1 - swap] = &ent1->item->oid;\n+\t}\n \tdiff_tree_oid(oid[0], oid[1], \"\", &revs->diffopt);\n \tlog_tree_diff_flush(revs);\n \treturn 0;\ndiff --git a/t/t4068-diff-symmetric-merge-base.sh b/t/t4068-diff-symmetric-merge-base.sh\nindex 49432379cb..03487cc945 100755\n--- a/t/t4068-diff-symmetric-merge-base.sh\n+++ b/t/t4068-diff-symmetric-merge-base.sh\n@@ -156,4 +156,38 @@ do\n \t'\n done\n \n+for cmd in diff-tree diff\n+do\n+\ttest_expect_success \"$cmd --merge-base with two commits\" '\n+\t\tgit $cmd commit-C master >expect &&\n+\t\tgit $cmd --merge-base br2 master >actual &&\n+\t\ttest_cmp expect actual\n+\t'\n+\n+\ttest_expect_success \"$cmd --merge-base commit and non-commit\" '\n+\t\ttest_must_fail git $cmd --merge-base br2 master^{tree} 2>err &&\n+\t\ttest_i18ngrep \"fatal: --merge-base only works with commits\" err\n+\t'\n+\n+\ttest_expect_success \"$cmd --merge-base with no merge bases and two commits\" '\n+\t\ttest_must_fail git $cmd --merge-base br2 br3 2>err &&\n+\t\ttest_i18ngrep \"fatal: no merge base found\" err\n+\t'\n+\n+\ttest_expect_success \"$cmd --merge-base with multiple merge bases and two commits\" '\n+\t\ttest_must_fail git $cmd --merge-base master br1 2>err &&\n+\t\ttest_i18ngrep \"fatal: multiple merge bases found\" err\n+\t'\n+done\n+\n+test_expect_success 'diff-tree --merge-base with one commit' '\n+\ttest_must_fail git diff-tree --merge-base master 2>err &&\n+\ttest_i18ngrep \"fatal: --merge-base only works with two commits\" err\n+'\n+\n+test_expect_success 'diff --merge-base with range' '\n+\ttest_must_fail git diff --merge-base br2..br3 2>err &&\n+\ttest_i18ngrep \"fatal: --merge-base does not work with ranges\" err\n+'\n+\n test_done\n-- \n2.28.0.618.gf4bc123cb7\n\n"},{"id":"405723","messageId":"1e4f805e570b8caad26c43b4c8293413e9128d5f.1600328335.git.liu.denton@gmail.com","threadId":"54193","inReplyTo":"cover.1600328335.git.liu.denton@gmail.com","subject":"[PATCH v3 08/10] builtin/diff-index: learn --merge-base","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-09-17T07:44:12Z","receivedAt":"2020-09-17T07:45:29Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"There is currently no easy way to take the diff between the working tree\nor index and the merge base between an arbitrary commit and HEAD. Even\ndiff's `...` notation doesn't allow this because it only works between\ncommits. However, the ability to do this would be desirable to a user\nwho would like to see all the changes they've made on a branch plus\nuncommitted changes without taking into account changes made in the\nupstream branch.\n\nTeach diff-index and diff (with one commit) the --merge-base option\nwhich allows a user to use the merge base of a commit and HEAD as the\n\"before\" side.\n\nSigned-off-by: Denton Liu \n---\n Documentation/git-diff-index.txt | 7 +++-\n Documentation/git-diff.txt | 12 ++++--\n builtin/diff-index.c | 2 +\n builtin/diff.c | 2 +\n diff-lib.c | 13 +++++-\n diff.h | 1 +\n t/t4068-diff-symmetric-merge-base.sh | 59 ++++++++++++++++++++++++++++\n 7 files changed, 90 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-diff-index.txt b/Documentation/git-diff-index.txt\nindex 25fe165f00..27acb31cbf 100644\n--- a/Documentation/git-diff-index.txt\n+++ b/Documentation/git-diff-index.txt\n@@ -9,7 +9,7 @@ git-diff-index - Compare a tree to the working tree or index\n SYNOPSIS\n --------\n [verse]\n-'git diff-index' [-m] [--cached] [] [...]\n+'git diff-index' [-m] [--cached] [--merge-base] [] [...]\n \n DESCRIPTION\n -----------\n@@ -29,6 +29,11 @@ include::diff-options.txt[]\n --cached::\n \tDo not consider the on-disk file at all.\n \n+--merge-base::\n+\tInstead of comparing directly, use the merge base\n+\tbetween and HEAD instead. must be a\n+\tcommit.\n+\n -m::\n \tBy default, files recorded in the index but not checked\n \tout are reported as deleted. This flag makes\ndiff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt\nindex 8f7b4ed3ca..762ee6d074 100644\n--- a/Documentation/git-diff.txt\n+++ b/Documentation/git-diff.txt\n@@ -10,7 +10,7 @@ SYNOPSIS\n --------\n [verse]\n 'git diff' [] [] [--] [...]\n-'git diff' [] --cached [] [--] [...]\n+'git diff' [] --cached [--merge-base] [] [--] [...]\n 'git diff' [] [...] [--] [...]\n 'git diff' [] ... [--] [...]\n 'git diff' [] \n@@ -40,7 +40,7 @@ files on disk.\n \tor when running the command outside a working tree\n \tcontrolled by Git. This form implies `--exit-code`.\n \n-'git diff' [] --cached [] [--] [...]::\n+'git diff' [] --cached [--merge-base] [] [--] [...]::\n \n \tThis form is to view the changes you staged for the next\n \tcommit relative to the named . Typically you\n@@ -49,6 +49,10 @@ files on disk.\n \tIf HEAD does not exist (e.g. unborn branches) and\n \t is not given, it shows all staged changes.\n \t--staged is a synonym of --cached.\n++\n+If --merge-base is given, instead of using , use the merge base\n+of and HEAD. `git diff --merge-base A` is equivalent to\n+`git diff $(git merge-base A HEAD)`.\n \n 'git diff' [] [--] [...]::\n \n@@ -89,8 +93,8 @@ files on disk.\n \n Just in case you are doing something exotic, it should be\n noted that all of the in the above description, except\n-in the last two forms that use `..` notations, can be any\n-.\n+in the `--merge-base` case and in the last two forms that use `..`\n+notations, can be any .\n \n For a more complete list of ways to spell , see\n \"SPECIFYING REVISIONS\" section in linkgit:gitrevisions[7].\ndiff --git a/builtin/diff-index.c b/builtin/diff-index.c\nindex c3878f7ad6..7f5281c461 100644\n--- a/builtin/diff-index.c\n+++ b/builtin/diff-index.c\n@@ -33,6 +33,8 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)\n \n \t\tif (!strcmp(arg, \"--cached\"))\n \t\t\toption |= DIFF_INDEX_CACHED;\n+\t\telse if (!strcmp(arg, \"--merge-base\"))\n+\t\t\toption |= DIFF_INDEX_MERGE_BASE;\n \t\telse\n \t\t\tusage(diff_cache_usage);\n \t}\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex e45e19e37e..1baea18ae0 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -139,6 +139,8 @@ static int builtin_diff_index(struct rev_info *revs,\n \t\tconst char *arg = argv[1];\n \t\tif (!strcmp(arg, \"--cached\") || !strcmp(arg, \"--staged\"))\n \t\t\toption |= DIFF_INDEX_CACHED;\n+\t\telse if (!strcmp(arg, \"--merge-base\"))\n+\t\t\toption |= DIFF_INDEX_MERGE_BASE;\n \t\telse\n \t\t\tusage(builtin_diff_usage);\n \t\targv++; argc--;\ndiff --git a/diff-lib.c b/diff-lib.c\nindex e01c3f0612..68bf86f289 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -569,13 +569,24 @@ void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb)\n int run_diff_index(struct rev_info *revs, unsigned int option)\n {\n \tstruct object_array_entry *ent;\n+\tstruct object_id oid;\n+\tconst char *name;\n \n \tif (revs->pending.nr != 1)\n \t\tBUG(\"run_diff_index must be passed exactly one tree\");\n \n \ttrace_performance_enter();\n \tent = revs->pending.objects;\n-\tif (diff_cache(revs, &ent->item->oid, ent->name, !!(option & DIFF_INDEX_CACHED)))\n+\n+\tif (option & DIFF_INDEX_MERGE_BASE) {\n+\t\tdiff_get_merge_base(revs, &oid);\n+\t\tname = xstrdup(oid_to_hex(&oid));\n+\t} else {\n+\t\toidcpy(&oid, &ent->item->oid);\n+\t\tname = ent->name;\n+\t}\n+\n+\tif (diff_cache(revs, &oid, name, !!(option & DIFF_INDEX_CACHED)))\n \t\texit(128);\n \n \tdiff_set_mnemonic_prefix(&revs->diffopt, \"c/\", (option & DIFF_INDEX_CACHED) ? \"i/\" : \"w/\");\ndiff --git a/diff.h b/diff.h\nindex ae2bb7001a..0485786b68 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -588,6 +588,7 @@ void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb);\n #define DIFF_RACY_IS_MODIFIED 02\n int run_diff_files(struct rev_info *revs, unsigned int option);\n #define DIFF_INDEX_CACHED 01\n+#define DIFF_INDEX_MERGE_BASE 02\n int run_diff_index(struct rev_info *revs, unsigned int option);\n \n int do_diff_cache(const struct object_id *, struct diff_options *);\ndiff --git a/t/t4068-diff-symmetric-merge-base.sh b/t/t4068-diff-symmetric-merge-base.sh\nindex bd4cf254d9..49432379cb 100755\n--- a/t/t4068-diff-symmetric-merge-base.sh\n+++ b/t/t4068-diff-symmetric-merge-base.sh\n@@ -97,4 +97,63 @@ test_expect_success 'diff --merge-base with three commits' '\n \ttest_i18ngrep \"usage\" err\n '\n \n+for cmd in diff-index diff\n+do\n+\ttest_expect_success \"$cmd --merge-base with one commit\" '\n+\t\tgit checkout master &&\n+\t\tgit $cmd commit-C >expect &&\n+\t\tgit $cmd --merge-base br2 >actual &&\n+\t\ttest_cmp expect actual\n+\t'\n+\n+\ttest_expect_success \"$cmd --merge-base with one commit and unstaged changes\" '\n+\t\tgit checkout master &&\n+\t\ttest_when_finished git reset --hard &&\n+\t\techo unstaged >>c &&\n+\t\tgit $cmd commit-C >expect &&\n+\t\tgit $cmd --merge-base br2 >actual &&\n+\t\ttest_cmp expect actual\n+\t'\n+\n+\ttest_expect_success \"$cmd --merge-base with one commit and staged and unstaged changes\" '\n+\t\tgit checkout master &&\n+\t\ttest_when_finished git reset --hard &&\n+\t\techo staged >>c &&\n+\t\tgit add c &&\n+\t\techo unstaged >>c &&\n+\t\tgit $cmd commit-C >expect &&\n+\t\tgit $cmd --merge-base br2 >actual &&\n+\t\ttest_cmp expect actual\n+\t'\n+\n+\ttest_expect_success \"$cmd --merge-base --cached with one commit and staged and unstaged changes\" '\n+\t\tgit checkout master &&\n+\t\ttest_when_finished git reset --hard &&\n+\t\techo staged >>c &&\n+\t\tgit add c &&\n+\t\techo unstaged >>c &&\n+\t\tgit $cmd --cached commit-C >expect &&\n+\t\tgit $cmd --cached --merge-base br2 >actual &&\n+\t\ttest_cmp expect actual\n+\t'\n+\n+\ttest_expect_success \"$cmd --merge-base with non-commit\" '\n+\t\tgit checkout master &&\n+\t\ttest_must_fail git $cmd --merge-base master^{tree} 2>err &&\n+\t\ttest_i18ngrep \"fatal: --merge-base only works with commits\" err\n+\t'\n+\n+\ttest_expect_success \"$cmd --merge-base with no merge bases and one commit\" '\n+\t\tgit checkout master &&\n+\t\ttest_must_fail git $cmd --merge-base br3 2>err &&\n+\t\ttest_i18ngrep \"fatal: no merge base found\" err\n+\t'\n+\n+\ttest_expect_success \"$cmd --merge-base with multiple merge bases and one commit\" '\n+\t\tgit checkout master &&\n+\t\ttest_must_fail git $cmd --merge-base br1 2>err &&\n+\t\ttest_i18ngrep \"fatal: multiple merge bases found\" err\n+\t'\n+done\n+\n test_done\n-- \n2.28.0.618.gf4bc123cb7\n\n"},{"id":"405724","messageId":"496908ac10152edb9cbcfdb75c43b1d00e163e3c.1600328335.git.liu.denton@gmail.com","threadId":"54193","inReplyTo":"cover.1600328335.git.liu.denton@gmail.com","subject":"[PATCH v3 05/10] diff-lib: accept option flags in run_diff_index()","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-09-17T07:44:09Z","receivedAt":"2020-09-17T07:45:34Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In a future commit, we will teach run_diff_index() to accept more\noptions via flag bits. For now, change `cached` into a flag in the\n`option` bitfield. The behaviour should remain exactly the same.\n\nSigned-off-by: Denton Liu \n---\n builtin/diff-index.c | 8 ++++----\n builtin/diff.c | 8 ++++----\n diff-lib.c | 6 +++---\n diff.h | 3 ++-\n 4 files changed, 13 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/diff-index.c b/builtin/diff-index.c\nindex 93ec642423..c3878f7ad6 100644\n--- a/builtin/diff-index.c\n+++ b/builtin/diff-index.c\n@@ -15,7 +15,7 @@ COMMON_DIFF_OPTIONS_HELP;\n int cmd_diff_index(int argc, const char **argv, const char *prefix)\n {\n \tstruct rev_info rev;\n-\tint cached = 0;\n+\tunsigned int option = 0;\n \tint i;\n \tint result;\n \n@@ -32,7 +32,7 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)\n \t\tconst char *arg = argv[i];\n \n \t\tif (!strcmp(arg, \"--cached\"))\n-\t\t\tcached = 1;\n+\t\t\toption |= DIFF_INDEX_CACHED;\n \t\telse\n \t\t\tusage(diff_cache_usage);\n \t}\n@@ -46,7 +46,7 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)\n \tif (rev.pending.nr != 1 ||\n \t rev.max_count != -1 || rev.min_age != -1 || rev.max_age != -1)\n \t\tusage(diff_cache_usage);\n-\tif (!cached) {\n+\tif (!(option & DIFF_INDEX_CACHED)) {\n \t\tsetup_work_tree();\n \t\tif (read_cache_preload(&rev.diffopt.pathspec) < 0) {\n \t\t\tperror(\"read_cache_preload\");\n@@ -56,7 +56,7 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)\n \t\tperror(\"read_cache\");\n \t\treturn -1;\n \t}\n-\tresult = run_diff_index(&rev, cached);\n+\tresult = run_diff_index(&rev, option);\n \tUNLEAK(rev);\n \treturn diff_result_code(&rev.diffopt, result);\n }\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex cb98811c21..e45e19e37e 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -134,11 +134,11 @@ static int builtin_diff_blobs(struct rev_info *revs,\n static int builtin_diff_index(struct rev_info *revs,\n \t\t\t int argc, const char **argv)\n {\n-\tint cached = 0;\n+\tunsigned int option = 0;\n \twhile (1 < argc) {\n \t\tconst char *arg = argv[1];\n \t\tif (!strcmp(arg, \"--cached\") || !strcmp(arg, \"--staged\"))\n-\t\t\tcached = 1;\n+\t\t\toption |= DIFF_INDEX_CACHED;\n \t\telse\n \t\t\tusage(builtin_diff_usage);\n \t\targv++; argc--;\n@@ -151,7 +151,7 @@ static int builtin_diff_index(struct rev_info *revs,\n \t revs->max_count != -1 || revs->min_age != -1 ||\n \t revs->max_age != -1)\n \t\tusage(builtin_diff_usage);\n-\tif (!cached) {\n+\tif (!(option & DIFF_INDEX_CACHED)) {\n \t\tsetup_work_tree();\n \t\tif (read_cache_preload(&revs->diffopt.pathspec) < 0) {\n \t\t\tperror(\"read_cache_preload\");\n@@ -161,7 +161,7 @@ static int builtin_diff_index(struct rev_info *revs,\n \t\tperror(\"read_cache\");\n \t\treturn -1;\n \t}\n-\treturn run_diff_index(revs, cached);\n+\treturn run_diff_index(revs, option);\n }\n \n static int builtin_diff_tree(struct rev_info *revs,\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 50521e2093..0a0e69113c 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -518,7 +518,7 @@ static int diff_cache(struct rev_info *revs,\n \treturn unpack_trees(1, &t, &opts);\n }\n \n-int run_diff_index(struct rev_info *revs, int cached)\n+int run_diff_index(struct rev_info *revs, unsigned int option)\n {\n \tstruct object_array_entry *ent;\n \n@@ -527,10 +527,10 @@ int run_diff_index(struct rev_info *revs, int cached)\n \n \ttrace_performance_enter();\n \tent = revs->pending.objects;\n-\tif (diff_cache(revs, &ent->item->oid, ent->name, cached))\n+\tif (diff_cache(revs, &ent->item->oid, ent->name, !!(option & DIFF_INDEX_CACHED)))\n \t\texit(128);\n \n-\tdiff_set_mnemonic_prefix(&revs->diffopt, \"c/\", cached ? \"i/\" : \"w/\");\n+\tdiff_set_mnemonic_prefix(&revs->diffopt, \"c/\", (option & DIFF_INDEX_CACHED) ? \"i/\" : \"w/\");\n \tdiffcore_fix_diff_index();\n \tdiffcore_std(&revs->diffopt);\n \tdiff_flush(&revs->diffopt);\ndiff --git a/diff.h b/diff.h\nindex e0c0af6286..0cc874f2d5 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -585,7 +585,8 @@ const char *diff_aligned_abbrev(const struct object_id *sha1, int);\n /* report racily-clean paths as modified */\n #define DIFF_RACY_IS_MODIFIED 02\n int run_diff_files(struct rev_info *revs, unsigned int option);\n-int run_diff_index(struct rev_info *revs, int cached);\n+#define DIFF_INDEX_CACHED 01\n+int run_diff_index(struct rev_info *revs, unsigned int option);\n \n int do_diff_cache(const struct object_id *, struct diff_options *);\n int diff_flush_patch_id(struct diff_options *, struct object_id *, int, int);\n-- \n2.28.0.618.gf4bc123cb7\n\n"},{"id":"405725","messageId":"1ac84595419d8d1ecd8129395c3261ebca643bc2.1600328335.git.liu.denton@gmail.com","threadId":"54193","inReplyTo":"cover.1600328335.git.liu.denton@gmail.com","subject":"[PATCH v3 04/10] contrib/completion: extract common diff/difftool options","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-09-17T07:44:08Z","receivedAt":"2020-09-17T07:45:43Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"difftool parses its own options and then passes the remaining options\nonto diff. As a result, they share common command-line options. Instead\nof duplicating the list, use a shared $__git_diff_difftool_options list.\n\nThe completion for diff is missing --relative and the completion for\ndifftool is missing --no-index. Add both of these to the common list.\n\nSigned-off-by: Denton Liu \n---\n contrib/completion/git-completion.bash | 15 ++++++---------\n 1 file changed, 6 insertions(+), 9 deletions(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 9147fba3d5..f68c8e0646 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -1691,6 +1691,10 @@ __git_diff_common_options=\"--stat --numstat --shortstat --summary\n \t\t\t--textconv --no-textconv\n \"\n \n+__git_diff_difftool_options=\"--cached --staged --pickaxe-all --pickaxe-regex\n+\t\t\t--base --ours --theirs --no-index --relative\n+\t\t\t$__git_diff_common_options\"\n+\n _git_diff ()\n {\n \t__git_has_doubledash && return\n@@ -1713,10 +1717,7 @@ _git_diff ()\n \t\treturn\n \t\t;;\n \t--*)\n-\t\t__gitcomp \"--cached --staged --pickaxe-all --pickaxe-regex\n-\t\t\t--base --ours --theirs --no-index\n-\t\t\t$__git_diff_common_options\n-\t\t\t\"\n+\t\t__gitcomp \"$__git_diff_difftool_options\"\n \t\treturn\n \t\t;;\n \tesac\n@@ -1738,11 +1739,7 @@ _git_difftool ()\n \t\treturn\n \t\t;;\n \t--*)\n-\t\t__gitcomp_builtin difftool \"$__git_diff_common_options\n-\t\t\t\t\t--base --cached --ours --theirs\n-\t\t\t\t\t--pickaxe-all --pickaxe-regex\n-\t\t\t\t\t--relative --staged\n-\t\t\t\t\t\"\n+\t\t__gitcomp_builtin difftool \"$__git_diff_difftool_options\"\n \t\treturn\n \t\t;;\n \tesac\n-- \n2.28.0.618.gf4bc123cb7\n\n"},{"id":"405726","messageId":"6aac57ca022963fb41d93905e41dff36dccd5969.1600328335.git.liu.denton@gmail.com","threadId":"54193","inReplyTo":"cover.1600328335.git.liu.denton@gmail.com","subject":"[PATCH v3 06/10] diff-lib: define diff_get_merge_base()","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-09-17T07:44:10Z","receivedAt":"2020-09-17T07:45:50Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In a future commit, we will be using this function to implement\n--merge-base functionality in various diff commands.\n\nSigned-off-by: Denton Liu \n---\n diff-lib.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++\n diff.h | 2 ++\n 2 files changed, 50 insertions(+)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 0a0e69113c..e01c3f0612 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -13,6 +13,7 @@\n #include \"submodule.h\"\n #include \"dir.h\"\n #include \"fsmonitor.h\"\n+#include \"commit-reach.h\"\n \n /*\n * diff-files\n@@ -518,6 +519,53 @@ static int diff_cache(struct rev_info *revs,\n \treturn unpack_trees(1, &t, &opts);\n }\n \n+void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb)\n+{\n+\tint i;\n+\tstruct commit *mb_child[2] = {0};\n+\tstruct commit_list *merge_bases;\n+\n+\tfor (i = 0; i < revs->pending.nr; i++) {\n+\t\tstruct object *obj = revs->pending.objects[i].item;\n+\t\tif (obj->flags)\n+\t\t\tdie(_(\"--merge-base does not work with ranges\"));\n+\t\tif (obj->type != OBJ_COMMIT)\n+\t\t\tdie(_(\"--merge-base only works with commits\"));\n+\t}\n+\n+\t/*\n+\t * This check must go after the for loop above because A...B\n+\t * ranges produce three pending commits, resulting in a\n+\t * misleading error message.\n+\t */\n+\tif (revs->pending.nr > ARRAY_SIZE(mb_child))\n+\t\tdie(_(\"--merge-base does not work with more than two commits\"));\n+\n+\tfor (i = 0; i < revs->pending.nr; i++)\n+\t\tmb_child[i] = lookup_commit_reference(the_repository, &revs->pending.objects[i].item->oid);\n+\tif (revs->pending.nr < ARRAY_SIZE(mb_child)) {\n+\t\tstruct object_id oid;\n+\n+\t\tif (revs->pending.nr != 1)\n+\t\t\tBUG(\"unexpected revs->pending.nr: %d\", revs->pending.nr);\n+\n+\t\tif (get_oid(\"HEAD\", &oid))\n+\t\t\tdie(_(\"unable to get HEAD\"));\n+\n+\t\tmb_child[1] = lookup_commit_reference(the_repository, &oid);\n+\t}\n+\n+\tmerge_bases = repo_get_merge_bases(the_repository, mb_child[0], mb_child[1]);\n+\tif (!merge_bases)\n+\t\tdie(_(\"no merge base found\"));\n+\tif (merge_bases->next)\n+\t\tdie(_(\"multiple merge bases found\"));\n+\n+\toidcpy(mb, &merge_bases->item->object.oid);\n+\n+\tfree_commit_list(merge_bases);\n+}\n+\n int run_diff_index(struct rev_info *revs, unsigned int option)\n {\n \tstruct object_array_entry *ent;\ndiff --git a/diff.h b/diff.h\nindex 0cc874f2d5..ae2bb7001a 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -580,6 +580,8 @@ void diff_warn_rename_limit(const char *varname, int needed, int degraded_cc);\n */\n const char *diff_aligned_abbrev(const struct object_id *sha1, int);\n \n+void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb);\n+\n /* do not report anything on removed paths */\n #define DIFF_SILENT_ON_REMOVED 01\n /* report racily-clean paths as modified */\n-- \n2.28.0.618.gf4bc123cb7\n\n"},{"id":"405727","messageId":"ca9568c2ea7d28605633576609de53a2d79df22e.1600328335.git.liu.denton@gmail.com","threadId":"54193","inReplyTo":"cover.1600328335.git.liu.denton@gmail.com","subject":"[PATCH v3 03/10] git-diff.txt: backtick quote command text","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-09-17T07:44:07Z","receivedAt":"2020-09-17T07:45:53Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The modern way to quote commands in the documentation is to use\nbackticks instead of double-quotes as this renders the text with the\ncode style. Convert double-quoted command text to backtick-quoted\ncommands. While we're at it, quote one instance of `^@`.\n\nSigned-off-by: Denton Liu \n---\n Documentation/git-diff.txt | 18 +++++++++---------\n 1 file changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt\nindex 727f24d16e..8f7b4ed3ca 100644\n--- a/Documentation/git-diff.txt\n+++ b/Documentation/git-diff.txt\n@@ -68,13 +68,13 @@ files on disk.\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+\tthe desired set of revisions is to use the `^@` 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 is synonymous to the earlier form (without the \"..\") for\n+\tThis is synonymous to the earlier form (without the `..`) for\n \tviewing the changes between two arbitrary . If on\n \tone side is omitted, it will have the same effect as\n \tusing HEAD instead.\n@@ -83,20 +83,20 @@ files on disk.\n \n \tThis form is to view the changes on the branch containing\n \tand up to the second , starting at a common ancestor\n-\tof both . \"git diff A\\...B\" is equivalent to\n-\t\"git diff $(git merge-base A B) B\". You can omit any one\n+\tof both . `git diff A...B` is equivalent to\n+\t`git diff $(git merge-base A B) B`. You can omit any one\n \tof , which has the same effect as using HEAD instead.\n \n Just in case you are doing something exotic, it should be\n noted that all of the in the above description, except\n-in the last two forms that use \"..\" notations, can be any\n+in the last two forms that use `..` notations, can be any\n .\n \n For a more complete list of ways to spell , see\n \"SPECIFYING REVISIONS\" section in linkgit:gitrevisions[7].\n However, \"diff\" is about comparing two _endpoints_, not ranges,\n-and the range notations (\"..\" and\n-\"\\...\") do not mean a range as defined in the\n+and the range notations (`..` and\n+`...`) do not mean a range as defined in the\n \"SPECIFYING RANGES\" section in linkgit:gitrevisions[7].\n \n 'git diff' [] ::\n@@ -144,9 +144,9 @@ $ git diff HEAD <3>\n +\n <1> Changes in the working tree not yet staged for the next commit.\n <2> Changes between the index and your last commit; what you\n- would be committing if you run \"git commit\" without \"-a\" option.\n+ would be committing if you run `git commit` without `-a` option.\n <3> Changes in the working tree since your last commit; what you\n- would be committing if you run \"git commit -a\"\n+ would be committing if you run `git commit -a`\n \n Comparing with arbitrary commits::\n +\n-- \n2.28.0.618.gf4bc123cb7\n\n"},{"id":"405728","messageId":"c9225a04404f1e1aa0b5146bddef77ecca69ebe3.1600328335.git.liu.denton@gmail.com","threadId":"54193","inReplyTo":"cover.1600328335.git.liu.denton@gmail.com","subject":"[PATCH v3 07/10] t4068: add --merge-base tests","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-09-17T07:44:11Z","receivedAt":"2020-09-17T07:45:54Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In the future, we will be adding more --merge-base tests to this test\nscript. To prepare for that, rename the script accordingly and update\nits description. Also, add two basic --merge-base tests that don't\nrequire any functionality to be implemented yet.\n\nSigned-off-by: Denton Liu \n---\n ...ymmetric.sh => t4068-diff-symmetric-merge-base.sh} | 11 ++++++++++-\n 1 file changed, 10 insertions(+), 1 deletion(-)\n rename t/{t4068-diff-symmetric.sh => t4068-diff-symmetric-merge-base.sh} (86%)\n\ndiff --git a/t/t4068-diff-symmetric.sh b/t/t4068-diff-symmetric-merge-base.sh\nsimilarity index 86%\nrename from t/t4068-diff-symmetric.sh\nrename to t/t4068-diff-symmetric-merge-base.sh\nindex 60c506c2b2..bd4cf254d9 100755\n--- a/t/t4068-diff-symmetric.sh\n+++ b/t/t4068-diff-symmetric-merge-base.sh\n@@ -1,6 +1,6 @@\n #!/bin/sh\n \n-test_description='behavior of diff with symmetric-diff setups'\n+test_description='behavior of diff with symmetric-diff setups and --merge-base'\n \n . ./test-lib.sh\n \n@@ -88,4 +88,13 @@ test_expect_success 'diff with ranges and extra arg' '\n \ttest_i18ngrep \"usage\" err\n '\n \n+test_expect_success 'diff --merge-base with no commits' '\n+\ttest_must_fail git diff --merge-base\n+'\n+\n+test_expect_success 'diff --merge-base with three commits' '\n+\ttest_must_fail git diff --merge-base br1 br2 master 2>err &&\n+\ttest_i18ngrep \"usage\" err\n+'\n+\n test_done\n-- \n2.28.0.618.gf4bc123cb7\n\n"},{"id":"405771","messageId":"xmqqh7rws5hm.fsf@gitster.c.googlers.com","threadId":"54193","inReplyTo":"496908ac10152edb9cbcfdb75c43b1d00e163e3c.1600328335.git.liu.denton@gmail.com","subject":"Re: [PATCH v3 05/10] diff-lib: accept option flags in run_diff_index()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-17T17:00:37Z","receivedAt":"2020-09-17T17:01:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu writes:\n\n> diff --git a/diff-lib.c b/diff-lib.c\n> index 50521e2093..0a0e69113c 100644\n> --- a/diff-lib.c\n> +++ b/diff-lib.c\n> @@ -518,7 +518,7 @@ static int diff_cache(struct rev_info *revs,\n> \treturn unpack_trees(1, &t, &opts);\n> }\n> \n> -int run_diff_index(struct rev_info *revs, int cached)\n> +int run_diff_index(struct rev_info *revs, unsigned int option)\n> {\n> \tstruct object_array_entry *ent;\n\nIf we introduce\n\n\tint cached = !!(option & DIFF_INDEX_CACHED);\n\nwe do not have to touch the remainder of the function, and it makes\nit easier to read the place(s) where 'cached' is used. At that\npoint, the fact that we were instructed by a bit in the option flag\nword is much much less important than we were instructed to compare\nthe tree-ish with the index and not the working tree files.\n\nThe same technique is used with DIFF_RACY_IS_MODIFIED flag in\nrun_diff_files().\n\n> diff --git a/diff.h b/diff.h\n> index e0c0af6286..0cc874f2d5 100644\n> --- a/diff.h\n> +++ b/diff.h\n> @@ -585,7 +585,8 @@ const char *diff_aligned_abbrev(const struct object_id *sha1, int);\n> /* report racily-clean paths as modified */\n> #define DIFF_RACY_IS_MODIFIED 02\n> int run_diff_files(struct rev_info *revs, unsigned int option);\n> -int run_diff_index(struct rev_info *revs, int cached);\n> +#define DIFF_INDEX_CACHED 01\n> +int run_diff_index(struct rev_info *revs, unsigned int option);\n\nIt is unclear from the above if the option word for run_diff_files()\nand run_diff_index() share the same bit assignment. After reading\nthe series through to the end, I know they are independent set of\nbits and never shared, but I wonder if we can make it more obvious\nhere.\n\nPerhaps an extra blank line before this new #define is sufficient to\nmake it clear?\n"},{"id":"405773","messageId":"xmqqd02ks4qk.fsf@gitster.c.googlers.com","threadId":"54193","inReplyTo":"6aac57ca022963fb41d93905e41dff36dccd5969.1600328335.git.liu.denton@gmail.com","subject":"Re: [PATCH v3 06/10] diff-lib: define diff_get_merge_base()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-17T17:16:51Z","receivedAt":"2020-09-17T17:18:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu writes:\n\n> +void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb)\n> +{\n> +\tint i;\n> +\tstruct commit *mb_child[2] = {0};\n> +\tstruct commit_list *merge_bases;\n> +\n> +\tfor (i = 0; i < revs->pending.nr; i++) {\n> +\t\tstruct object *obj = revs->pending.objects[i].item;\n> +\t\tif (obj->flags)\n> +\t\t\tdie(_(\"--merge-base does not work with ranges\"));\n> +\t\tif (obj->type != OBJ_COMMIT)\n> +\t\t\tdie(_(\"--merge-base only works with commits\"));\n> +\t}\n\nThis is the first use of die() in this file, that is designed to\nkeep a set of reusable library functions so that the caller(s) can\ndo their own die(). They may want to become\n\n\treturn error(_(...));\n\nThe same comment applies to all die()s the patch adds.\n\n> +\t/*\n> +\t * This check must go after the for loop above because A...B\n> +\t * ranges produce three pending commits, resulting in a\n> +\t * misleading error message.\n> +\t */\n\nShould \"git diff --merge-base A...B\" be forbidden, or does it just\nask the same thing twice and is not a die-worthy offence?\n\n> +\tif (revs->pending.nr > ARRAY_SIZE(mb_child))\n> +\t\tdie(_(\"--merge-base does not work with more than two commits\"));\n\nCompare with hardcoded '2' in the condition. The requirement for\nmb_child[] is that it can hold at least 2 (changes in the future may\nfind it convenient if its size gets increased to 3 to hold NULL sentinel\nto signal end-of-list, for example). Comparison with ARRAY_SIZE() may\nbe appropriate in different situations but not here where the code knows\nit wants to reject more than two no matter how big mb_child[] is.\n\n> +\tfor (i = 0; i < revs->pending.nr; i++)\n> +\t\tmb_child[i] = lookup_commit_reference(the_repository, &revs->pending.objects[i].item->oid);\n> +\tif (revs->pending.nr < ARRAY_SIZE(mb_child)) {\n> +\t\tstruct object_id oid;\n> +\n> +\t\tif (revs->pending.nr != 1)\n> +\t\t\tBUG(\"unexpected revs->pending.nr: %d\", revs->pending.nr);\n\nThis is an obviously impossible condition as we will not take more\nthan 2.\n\n> +\t\tif (get_oid(\"HEAD\", &oid))\n> +\t\t\tdie(_(\"unable to get HEAD\"));\n> +\t\tmb_child[1] = lookup_commit_reference(the_repository, &oid);\n> +\t}\n> +\n> +\tmerge_bases = repo_get_merge_bases(the_repository, mb_child[0], mb_child[1]);\n> +\tif (!merge_bases)\n> +\t\tdie(_(\"no merge base found\"));\n> +\tif (merge_bases->next)\n> +\t\tdie(_(\"multiple merge bases found\"));\n> +\n> +\toidcpy(mb, &merge_bases->item->object.oid);\n> +\n> +\tfree_commit_list(merge_bases);\n> +}\n\n\nOK.\n\n> int run_diff_index(struct rev_info *revs, unsigned int option)\n> {\n> \tstruct object_array_entry *ent;\n> diff --git a/diff.h b/diff.h\n> index 0cc874f2d5..ae2bb7001a 100644\n> --- a/diff.h\n> +++ b/diff.h\n> @@ -580,6 +580,8 @@ void diff_warn_rename_limit(const char *varname, int needed, int degraded_cc);\n> */\n> const char *diff_aligned_abbrev(const struct object_id *sha1, int);\n> \n> +void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb);\n\nWithout an actual caller, we cannot judge if this interface is\ndesigned right, but we'll see soon in the next steps ;-)\n\nLooking good so far.\n\nThanks.\n\n> +\n> /* do not report anything on removed paths */\n> #define DIFF_SILENT_ON_REMOVED 01\n> /* report racily-clean paths as modified */\n"},{"id":"405774","messageId":"xmqq8sd8s46i.fsf@gitster.c.googlers.com","threadId":"54193","inReplyTo":"1e4f805e570b8caad26c43b4c8293413e9128d5f.1600328335.git.liu.denton@gmail.com","subject":"Re: [PATCH v3 08/10] builtin/diff-index: learn --merge-base","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-17T17:28:53Z","receivedAt":"2020-09-17T17:30:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu writes:\n\n> diff --git a/builtin/diff-index.c b/builtin/diff-index.c\n> index c3878f7ad6..7f5281c461 100644\n> --- a/builtin/diff-index.c\n> +++ b/builtin/diff-index.c\n> @@ -33,6 +33,8 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)\n> \n> \t\tif (!strcmp(arg, \"--cached\"))\n> \t\t\toption |= DIFF_INDEX_CACHED;\n> +\t\telse if (!strcmp(arg, \"--merge-base\"))\n> +\t\t\toption |= DIFF_INDEX_MERGE_BASE;\n> \t\telse\n> \t\t\tusage(diff_cache_usage);\n> \t}\n> diff --git a/builtin/diff.c b/builtin/diff.c\n> index e45e19e37e..1baea18ae0 100644\n> --- a/builtin/diff.c\n> +++ b/builtin/diff.c\n> @@ -139,6 +139,8 @@ static int builtin_diff_index(struct rev_info *revs,\n> \t\tconst char *arg = argv[1];\n> \t\tif (!strcmp(arg, \"--cached\") || !strcmp(arg, \"--staged\"))\n> \t\t\toption |= DIFF_INDEX_CACHED;\n> +\t\telse if (!strcmp(arg, \"--merge-base\"))\n> +\t\t\toption |= DIFF_INDEX_MERGE_BASE;\n> \t\telse\n> \t\t\tusage(builtin_diff_usage);\n> \t\targv++; argc--;\n\nOK.\n\n> diff --git a/diff-lib.c b/diff-lib.c\n> index e01c3f0612..68bf86f289 100644\n> --- a/diff-lib.c\n> +++ b/diff-lib.c\n> @@ -569,13 +569,24 @@ void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb)\n> int run_diff_index(struct rev_info *revs, unsigned int option)\n> {\n> \tstruct object_array_entry *ent;\n> +\tstruct object_id oid;\n> +\tconst char *name;\n\nLet's do the same\n\n\tint compare_with_merge_base = !!(option & DIFF_INDEX_MERGE_BASE);\n\nhere to keep the result easier to follow.\n\n> \tif (revs->pending.nr != 1)\n> \t\tBUG(\"run_diff_index must be passed exactly one tree\");\n> \n> \ttrace_performance_enter();\n> \tent = revs->pending.objects;\n> -\tif (diff_cache(revs, &ent->item->oid, ent->name, !!(option & DIFF_INDEX_CACHED)))\n> +\n> +\tif (option & DIFF_INDEX_MERGE_BASE) {\n> +\t\tdiff_get_merge_base(revs, &oid);\n> +\t\tname = xstrdup(oid_to_hex(&oid));\n\nLeak?\n\n> +\t} else {\n> +\t\toidcpy(&oid, &ent->item->oid);\n> +\t\tname = ent->name;\n> +\t}\n> +\n> +\tif (diff_cache(revs, &oid, name, !!(option & DIFF_INDEX_CACHED)))\n> \t\texit(128);\n> \n> \tdiff_set_mnemonic_prefix(&revs->diffopt, \"c/\", (option & DIFF_INDEX_CACHED) ? \"i/\" : \"w/\");\n\n> +for cmd in diff-index diff\n> +do\n> +\ttest_expect_success \"$cmd --merge-base with one commit\" '\n> +\t\tgit checkout master &&\n> +\t\tgit $cmd commit-C >expect &&\n> +\t\tgit $cmd --merge-base br2 >actual &&\n> +\t\ttest_cmp expect actual\n> +\t'\n\nOK, the same command, when comparing with commit-C and with\n\"--merge-base br2\" that should compute the same commit-C, should\ngive the same answer. Good testing strategy.\n\nThanks.\n"},{"id":"405782","messageId":"20200917181303.GA108156@coredump.intra.peff.net","threadId":"54193","inReplyTo":"xmqq8sd8s46i.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3 08/10] builtin/diff-index: learn --merge-base","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-09-17T18:13:03Z","receivedAt":"2020-09-17T18:15:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 17, 2020 at 10:28:53AM -0700, Junio C Hamano wrote:\n\n> > -\tif (diff_cache(revs, &ent->item->oid, ent->name, !!(option & DIFF_INDEX_CACHED)))\n> > +\n> > +\tif (option & DIFF_INDEX_MERGE_BASE) {\n> > +\t\tdiff_get_merge_base(revs, &oid);\n> > +\t\tname = xstrdup(oid_to_hex(&oid));\n> \n> Leak?\n\nUsing oid_to_hex_r() avoids an extra copy, and the leak goes away too:\n\n char merge_base_hex[GIT_MAX_HEXSZ + 1];\n ...\n name = oid_to_hex_r(merge_base_hex, &oid);\n\n-Peff\n"},{"id":"405789","messageId":"xmqq363gs1mt.fsf@gitster.c.googlers.com","threadId":"54193","inReplyTo":"c0d27b125e969e13c52b0fa806a8e3caa8c20ac6.1600328336.git.liu.denton@gmail.com","subject":"Re: [PATCH v3 09/10] builtin/diff-tree: learn --merge-base","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-17T18:23:54Z","receivedAt":"2020-09-17T18:24:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu writes:\n\n> +\tif (read_stdin && merge_base)\n> +\t\tdie(_(\"--stdin and --merge-base are mutually exclusive\"));\n> +\n> +\tif (merge_base) {\n> +\t\tstruct object_id oid;\n> +\n> +\t\tif (opt->pending.nr != 2)\n> +\t\t\tdie(_(\"--merge-base only works with two commits\"));\n> +\n> +\t\tdiff_get_merge_base(opt, &oid);\n> +\t\topt->pending.objects[0].item = lookup_object(the_repository, &oid);\n> +\t}\n> +\n\nThis looks quite straight-forward.\n\n> -\t/*\n> -\t * We saw two trees, ent0 and ent1. If ent1 is uninteresting,\n> -\t * swap them.\n> -\t */\n> -\tif (ent1->item->flags & UNINTERESTING)\n> -\t\tswap = 1;\n> -\toid[swap] = &ent0->item->oid;\n> -\toid[1 - swap] = &ent1->item->oid;\n> +\tif (merge_base) {\n> +\t\tdiff_get_merge_base(revs, &mb_oid);\n> +\t\toid[0] = &mb_oid;\n> +\t\toid[1] = &revs->pending.objects[1].item->oid;\n> +\t} else {\n> +\t\tint swap = 0;\n> +\n> +\t\t/*\n> +\t\t * We saw two trees, ent0 and ent1. If ent1 is uninteresting,\n> +\t\t * swap them.\n> +\t\t */\n> +\t\tif (ent1->item->flags & UNINTERESTING)\n> +\t\t\tswap = 1;\n> +\t\toid[swap] = &ent0->item->oid;\n> +\t\toid[1 - swap] = &ent1->item->oid;\n> +\t}\n\nIt is not entirely clear why the original has to become an [else]\nclause here, unlike the change we saw earlier in cmd_diff_tree().\nIt feels quite inconsistent.\n\nThanks.\n"},{"id":"405839","messageId":"xmqqk0wroehv.fsf@gitster.c.googlers.com","threadId":"54193","inReplyTo":"20200917181303.GA108156@coredump.intra.peff.net","subject":"Re: [PATCH v3 08/10] builtin/diff-index: learn --merge-base","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-18T05:11:56Z","receivedAt":"2020-09-18T05:12:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Sep 17, 2020 at 10:28:53AM -0700, Junio C Hamano wrote:\n>\n>> > -\tif (diff_cache(revs, &ent->item->oid, ent->name, !!(option & DIFF_INDEX_CACHED)))\n>> > +\n>> > +\tif (option & DIFF_INDEX_MERGE_BASE) {\n>> > +\t\tdiff_get_merge_base(revs, &oid);\n>> > +\t\tname = xstrdup(oid_to_hex(&oid));\n>> \n>> Leak?\n>\n> Using oid_to_hex_r() avoids an extra copy, and the leak goes away too:\n>\n> char merge_base_hex[GIT_MAX_HEXSZ + 1];\n> ...\n> name = oid_to_hex_r(merge_base_hex, &oid);\n>\n> -Peff\n\nYes, I was debating myself if I should mention it or trust/assume\nthat the contributor can easily figure it out.\n"},{"id":"405844","messageId":"20200918103400.GA1874074@generichostname","threadId":"54193","inReplyTo":"xmqqd02ks4qk.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3 06/10] diff-lib: define diff_get_merge_base()","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-09-18T10:34:00Z","receivedAt":"2020-09-18T10:34:06Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Junio,\n\nOn Thu, Sep 17, 2020 at 10:16:51AM -0700, Junio C Hamano wrote:\n> Denton Liu writes:\n> \n> > +void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb)\n> > +{\n> > +\tint i;\n> > +\tstruct commit *mb_child[2] = {0};\n> > +\tstruct commit_list *merge_bases;\n> > +\n> > +\tfor (i = 0; i < revs->pending.nr; i++) {\n> > +\t\tstruct object *obj = revs->pending.objects[i].item;\n> > +\t\tif (obj->flags)\n> > +\t\t\tdie(_(\"--merge-base does not work with ranges\"));\n> > +\t\tif (obj->type != OBJ_COMMIT)\n> > +\t\t\tdie(_(\"--merge-base only works with commits\"));\n> > +\t}\n> \n> This is the first use of die() in this file, that is designed to\n> keep a set of reusable library functions so that the caller(s) can\n> do their own die(). They may want to become\n\nAlthough this is the first instance of die(), run_diff_index() has an\nexit(128), which is basically a die() in disguise.\n\n> \treturn error(_(...));\n> \n> The same comment applies to all die()s the patch adds.\n\nI applied this change but then each callsite of diff_get_merge_base()\nbecame something like\n\n\tif (diff_get_merge_base(revs, &oid))\n\t\texit(128);\n\nso I do agree with the spirit of the change but in reality, it just\ncreates more busywork for the callers.\n\n> > +\t/*\n> > +\t * This check must go after the for loop above because A...B\n> > +\t * ranges produce three pending commits, resulting in a\n> > +\t * misleading error message.\n> > +\t */\n> \n> Should \"git diff --merge-base A...B\" be forbidden, or does it just\n> ask the same thing twice and is not a die-worthy offence?\n\nI think that it should be die-worthy because it's a logic error for a\nuser to do this. I can't think of any situation where it wouldn't be\nmore desirable error early to correct a user's thinking. Plus, we're\ntrying to move away from the `...` notation anyway ;)\n\n> > +\tfor (i = 0; i < revs->pending.nr; i++)\n> > +\t\tmb_child[i] = lookup_commit_reference(the_repository, &revs->pending.objects[i].item->oid);\n> > +\tif (revs->pending.nr < ARRAY_SIZE(mb_child)) {\n> > +\t\tstruct object_id oid;\n> > +\n> > +\t\tif (revs->pending.nr != 1)\n> > +\t\t\tBUG(\"unexpected revs->pending.nr: %d\", revs->pending.nr);\n> \n> This is an obviously impossible condition as we will not take more\n> than 2.\n\nWe also want to ensure that revs->pending.nr isn't 0 here. That being\nsaid, I can explicitly check earlier in the function that the number of\npending is 1 or 2 so that it's more clearly written.\n\nThanks,\nDenton\n"},{"id":"405845","messageId":"20200918104833.GB1874074@generichostname","threadId":"54193","inReplyTo":"xmqq363gs1mt.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3 09/10] builtin/diff-tree: learn --merge-base","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-09-18T10:48:33Z","receivedAt":"2020-09-18T10:48:39Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"On Thu, Sep 17, 2020 at 11:23:54AM -0700, Junio C Hamano wrote:\n> Denton Liu writes:\n> \n> > +\tif (read_stdin && merge_base)\n> > +\t\tdie(_(\"--stdin and --merge-base are mutually exclusive\"));\n> > +\n> > +\tif (merge_base) {\n> > +\t\tstruct object_id oid;\n> > +\n> > +\t\tif (opt->pending.nr != 2)\n> > +\t\t\tdie(_(\"--merge-base only works with two commits\"));\n> > +\n> > +\t\tdiff_get_merge_base(opt, &oid);\n> > +\t\topt->pending.objects[0].item = lookup_object(the_repository, &oid);\n> > +\t}\n> > +\n> \n> This looks quite straight-forward.\n> \n> > -\t/*\n> > -\t * We saw two trees, ent0 and ent1. If ent1 is uninteresting,\n> > -\t * swap them.\n> > -\t */\n> > -\tif (ent1->item->flags & UNINTERESTING)\n> > -\t\tswap = 1;\n> > -\toid[swap] = &ent0->item->oid;\n> > -\toid[1 - swap] = &ent1->item->oid;\n> > +\tif (merge_base) {\n> > +\t\tdiff_get_merge_base(revs, &mb_oid);\n> > +\t\toid[0] = &mb_oid;\n> > +\t\toid[1] = &revs->pending.objects[1].item->oid;\n> > +\t} else {\n> > +\t\tint swap = 0;\n> > +\n> > +\t\t/*\n> > +\t\t * We saw two trees, ent0 and ent1. If ent1 is uninteresting,\n> > +\t\t * swap them.\n> > +\t\t */\n> > +\t\tif (ent1->item->flags & UNINTERESTING)\n> > +\t\t\tswap = 1;\n> > +\t\toid[swap] = &ent0->item->oid;\n> > +\t\toid[1 - swap] = &ent1->item->oid;\n> > +\t}\n> \n> It is not entirely clear why the original has to become an [else]\n> clause here, unlike the change we saw earlier in cmd_diff_tree().\n> It feels quite inconsistent.\n\nSince we're only interested in the oids, I thought that it would be\npossible to save a lookup_object() and just use the oids directly. If\nit's clearer, this can be written as something like this but the lookup\nfeels unnecessary:\n\n\t/*\n\t * We saw two trees, ent0 and ent1. If ent1 is uninteresting,\n\t * swap them.\n\t */\n\tif (ent1->item->flags & UNINTERESTING)\n\t\tswap = 1;\n\n\tif (merge_base) {\n\t\tstruct object_id mb_oid;\n\t\tif (swap)\n\t\t\tBUG(\"swap is unexpectedly set\");\n\t\tif (diff_get_merge_base(revs, &mb_oid))\n\t\t\texit(128);\n\t\tent0->item = lookup_object(the_repository, &mb_oid);\n\t}\n\n\n\toid[swap] = &ent0->item->oid;\n\toid[1 - swap] = &ent1->item->oid;\n\nThanks,\nDenton\n"},{"id":"405881","messageId":"xmqqy2l7m3hk.fsf@gitster.c.googlers.com","threadId":"54193","inReplyTo":"20200918104833.GB1874074@generichostname","subject":"Re: [PATCH v3 09/10] builtin/diff-tree: learn --merge-base","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-18T16:52:39Z","receivedAt":"2020-09-18T16:52:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu writes:\n\n> Since we're only interested in the oids, I thought that it would be\n> possible to save a lookup_object() and just use the oids directly. If\n> it's clearer, this can be written as something like this but the lookup\n> feels unnecessary:\n\nWhen running the tree diff, we'd need the object anyway, and the\nresult of the look-up made here is cached, right?\n\nThat is why I expected it would just be an insertion before the\nexisting code, like the other side.\n\nBut the existing \"if we got either ^A B or B ^A, treat it as A..B\"\nlogic is just like \"if we got '--merge-base A B', treat it as\nsomething else\" we are adding, and they (and any future such special\nsyntax) should not interact with each other. So in that sense, the\ncode structure you have in the originally posted patch (not the code\nsnippet in your message I am responding to) that does\n\n ...\n if (using merge-base feature) {\n\tdo the merge base thing to populate oid[]\n } else if (user used A..B) {\n\tensure \"^A B\" and \"B ^A\" both have A in oid[0] and B in oid[1]\n }\n ...\n call diff-tree between oid[0] and oid[1]\n\nmakes a lot more sense than anything else we discussed so far.\n\nI wonder if turning the builtin/diff-tree.c to match that structure\nmake the result easier to understand (and I'll be perfectly happy if\nthe answer to this question turns out to be \"no, the result of the\nposted patch is the easiest to follow\").\n\nThanks.\n\n"},{"id":"405904","messageId":"20200918181217.GB187490@coredump.intra.peff.net","threadId":"54193","inReplyTo":"xmqqk0wroehv.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3 08/10] builtin/diff-index: learn --merge-base","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-09-18T18:12:17Z","receivedAt":"2020-09-18T18:12:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 17, 2020 at 10:11:56PM -0700, Junio C Hamano wrote:\n\n> > Using oid_to_hex_r() avoids an extra copy, and the leak goes away too:\n> >\n> > char merge_base_hex[GIT_MAX_HEXSZ + 1];\n> > ...\n> > name = oid_to_hex_r(merge_base_hex, &oid);\n> >\n> > -Peff\n> \n> Yes, I was debating myself if I should mention it or trust/assume\n> that the contributor can easily figure it out.\n\nI was tempted to call \"xstrdup(oid_to_hex())\" an anti-pattern, but\nlooking at most of the other calls, they really do want a new string\nthat lasts longer than a stack variable.\n\nAnd even the stack ones are a bit ugly in that you have to know to size\nthings correctly.\n\nSo I lost my enthusiasm to crusade against it. ;)\n\n-Peff\n"},{"id":"405923","messageId":"xmqq5z8ali5d.fsf@gitster.c.googlers.com","threadId":"54193","inReplyTo":"20200918103400.GA1874074@generichostname","subject":"Re: [PATCH v3 06/10] diff-lib: define diff_get_merge_base()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-19T00:33:34Z","receivedAt":"2020-09-19T00:33:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu writes:\n\n> became something like\n>\n> \tif (diff_get_merge_base(revs, &oid))\n> \t\texit(128);\n>\n> so I do agree with the spirit of the change but in reality, it just\n> creates more busywork for the callers.\n\nOK, then lets keep them as die()s.\n\n> I think that it should be die-worthy because it's a logic error for a\n> user to do this. I can't think of any situation where it wouldn't be\n> more desirable error early to correct a user's thinking. Plus, we're\n> trying to move away from the `...` notation anyway ;)\n\nI do not think so. We are *NOT* trying to move away from A...B;\nwhat is mistake is A..B and that is what we want to move away from.\nLuckily, there is no need to introduce a new option there, because\nthe user can just stop typing .. and instead type SP.\n\nThe primary value of the new option you are adding is that it allows\nus to compare the index and the working tree with the merge base.\nThe current A...B notation can only be used to compare two trees.\n"},{"id":"405978","messageId":"20200920110148.GA227771@generichostname","threadId":"54193","inReplyTo":"xmqqy2l7m3hk.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3 09/10] builtin/diff-tree: learn --merge-base","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-09-20T11:01:48Z","receivedAt":"2020-09-20T11:03:34Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Junio,\n\nOn Fri, Sep 18, 2020 at 09:52:39AM -0700, Junio C Hamano wrote:\n> I wonder if turning the builtin/diff-tree.c to match that structure\n> make the result easier to understand (and I'll be perfectly happy if\n> the answer to this question turns out to be \"no, the result of the\n> posted patch is the easiest to follow\").\n\ngit diff-tree does not even recognise ranges so as a result, the else\ncase does not even need to exist there, unlike in git diff.\n"},{"id":"405979","messageId":"80e9066a59524ededbb0d0191a93e6e3154c5623.1600600823.git.liu.denton@gmail.com","threadId":"54193","inReplyTo":"cover.1600600823.git.liu.denton@gmail.com","subject":"[PATCH v4 01/10] t4068: remove unnecessary >tmp","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-09-20T11:22:18Z","receivedAt":"2020-09-20T11:22:39Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The many `git diff` invocations have a `>tmp` redirection even though\nthe file is not being used afterwards. Remove these unnecessary\nredirections.\n\nSigned-off-by: Denton Liu \n---\n t/t4068-diff-symmetric.sh | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t4068-diff-symmetric.sh b/t/t4068-diff-symmetric.sh\nindex 31d17a5af0..60c506c2b2 100755\n--- a/t/t4068-diff-symmetric.sh\n+++ b/t/t4068-diff-symmetric.sh\n@@ -64,27 +64,27 @@ test_expect_success 'diff with two merge bases' '\n '\n \n test_expect_success 'diff with no merge bases' '\n-\ttest_must_fail git diff br2...br3 >tmp 2>err &&\n+\ttest_must_fail git diff br2...br3 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_must_fail git diff br1...master br2...br3 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_must_fail git diff master br1...master 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_must_fail git diff master br1..master br2..br3 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_must_fail git diff master br1..master commit-D 2>err &&\n \ttest_i18ngrep \"usage\" err\n '\n \n-- \n2.28.0.760.g8d73e04208\n\n"},{"id":"405980","messageId":"21b20281e6a1fa265d4853fa04cc71ba7e843267.1600600823.git.liu.denton@gmail.com","threadId":"54193","inReplyTo":"cover.1600600823.git.liu.denton@gmail.com","subject":"[PATCH v4 02/10] git-diff-index.txt: make --cached description a proper sentence","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-09-20T11:22:19Z","receivedAt":"2020-09-20T11:22:40Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Signed-off-by: Denton Liu \n---\n Documentation/git-diff-index.txt | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-diff-index.txt b/Documentation/git-diff-index.txt\nindex f4bd8155c0..25fe165f00 100644\n--- a/Documentation/git-diff-index.txt\n+++ b/Documentation/git-diff-index.txt\n@@ -27,7 +27,7 @@ include::diff-options.txt[]\n \tThe id of a tree object to diff against.\n \n --cached::\n-\tdo not consider the on-disk file at all\n+\tDo not consider the on-disk file at all.\n \n -m::\n \tBy default, files recorded in the index but not checked\n-- \n2.28.0.760.g8d73e04208\n\n"},{"id":"405981","messageId":"cover.1600600823.git.liu.denton@gmail.com","threadId":"54193","inReplyTo":"cover.1600328335.git.liu.denton@gmail.com","subject":"[PATCH v4 00/10] builtin/diff: learn --merge-base","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-09-20T11:22:17Z","receivedAt":"2020-09-20T11:22:42Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The range-notation in `git diff` has been cited as a mistake since diff\ncompares two endpoints, not whole ranges.[0] In fact, the ranges seem\nto take on the opposite meanings when compared to range notation in\n`git log`.\n\nIn an effort to reduce the use of range notation as much as possible,\nintroduce the `--merge-base` flag, slightly modified from a suggestion\nby Jonathan Nieder.[1] This flag allows us to replace the first commit\ngiven on the command-line with its merge base between the first and\nsecond commits.\n\nOne additional bonus is that this flag allows the \"after\" side to be not\njust constrained to a commit (like with `...` notation). It can now be\nthe working tree or the index as well.\n\nThe `--merge-base` name isn't very satisfying. If anyone has any better\nsuggestions, please let me know.\n\nChanges since v1:\n\n* Implement Junio's documentation suggestions\n\n* Update git diff's usage to include this option\n\nChanges since v2:\n\n* Teach diff-index and diff-tree the --merge-base option as well\n\nChanges since v3:\n\n* Don't use bitfields directly; extract them to an intermediate variable\n\n* Use explicit literals in diff_get_merge_base() for clarity\n\n* Fix xstrdup() leak\n\n* I misunderstood and thought we wanted to deprecate `...` notation;\n this is not the case so remove all references to \"gentle deprecation\"\n and don't remove usage text that references it\n\n[0]: https://lore.kernel.org/git/xmqqy2v26hu0.fsf@gitster-ct.c.googlers.com/\n[1]: https://lore.kernel.org/git/20191223215928.GB38316@google.com/\n\nDenton Liu (10):\n t4068: remove unnecessary >tmp\n git-diff-index.txt: make --cached description a proper sentence\n git-diff.txt: backtick quote command text\n contrib/completion: extract common diff/difftool options\n diff-lib: accept option flags in run_diff_index()\n diff-lib: define diff_get_merge_base()\n t4068: add --merge-base tests\n builtin/diff-index: learn --merge-base\n builtin/diff-tree: learn --merge-base\n contrib/completion: complete `git diff --merge-base`\n\n Documentation/git-diff-index.txt | 9 +-\n Documentation/git-diff-tree.txt | 7 +-\n Documentation/git-diff.txt | 36 +++--\n builtin/diff-index.c | 10 +-\n builtin/diff-tree.c | 18 +++\n builtin/diff.c | 49 +++++--\n contrib/completion/git-completion.bash | 15 +-\n diff-lib.c | 63 +++++++-\n diff.h | 7 +-\n t/t4068-diff-symmetric-merge-base.sh | 193 +++++++++++++++++++++++++\n t/t4068-diff-symmetric.sh | 91 ------------\n 11 files changed, 358 insertions(+), 140 deletions(-)\n create mode 100755 t/t4068-diff-symmetric-merge-base.sh\n delete mode 100755 t/t4068-diff-symmetric.sh\n\nRange-diff against v3:\n 1: 80e9066a59 = 1: 80e9066a59 t4068: remove unnecessary >tmp\n 2: 21b20281e6 = 2: 21b20281e6 git-diff-index.txt: make --cached description a proper sentence\n 3: ca9568c2ea = 3: ca9568c2ea git-diff.txt: backtick quote command text\n 4: 1ac8459541 = 4: 1ac8459541 contrib/completion: extract common diff/difftool options\n 5: 496908ac10 ! 5: 99d8b51585 diff-lib: accept option flags in run_diff_index()\n @@ diff-lib.c: static int diff_cache(struct rev_info *revs,\n +int run_diff_index(struct rev_info *revs, unsigned int option)\n {\n \tstruct object_array_entry *ent;\n ++\tint cached = !!(option & DIFF_INDEX_CACHED);\n \n -@@ diff-lib.c: int run_diff_index(struct rev_info *revs, int cached)\n - \n - \ttrace_performance_enter();\n - \tent = revs->pending.objects;\n --\tif (diff_cache(revs, &ent->item->oid, ent->name, cached))\n -+\tif (diff_cache(revs, &ent->item->oid, ent->name, !!(option & DIFF_INDEX_CACHED)))\n - \t\texit(128);\n - \n --\tdiff_set_mnemonic_prefix(&revs->diffopt, \"c/\", cached ? \"i/\" : \"w/\");\n -+\tdiff_set_mnemonic_prefix(&revs->diffopt, \"c/\", (option & DIFF_INDEX_CACHED) ? \"i/\" : \"w/\");\n - \tdiffcore_fix_diff_index();\n - \tdiffcore_std(&revs->diffopt);\n - \tdiff_flush(&revs->diffopt);\n + \tif (revs->pending.nr != 1)\n + \t\tBUG(\"run_diff_index must be passed exactly one tree\");\n \n ## diff.h ##\n @@ diff.h: const char *diff_aligned_abbrev(const struct object_id *sha1, int);\n @@ diff.h: const char *diff_aligned_abbrev(const struct object_id *sha1, int);\n #define DIFF_RACY_IS_MODIFIED 02\n int run_diff_files(struct rev_info *revs, unsigned int option);\n -int run_diff_index(struct rev_info *revs, int cached);\n ++\n +#define DIFF_INDEX_CACHED 01\n +int run_diff_index(struct rev_info *revs, unsigned int option);\n \n 6: 6aac57ca02 ! 6: 3287e649f1 diff-lib: define diff_get_merge_base()\n @@ diff-lib.c: static int diff_cache(struct rev_info *revs,\n +\t * ranges produce three pending commits, resulting in a\n +\t * misleading error message.\n +\t */\n -+\tif (revs->pending.nr > ARRAY_SIZE(mb_child))\n -+\t\tdie(_(\"--merge-base does not work with more than two commits\"));\n ++\tif (revs->pending.nr < 1 || revs->pending.nr > 2)\n ++\t\tBUG(\"unexpected revs->pending.nr: %d\", revs->pending.nr);\n +\n +\tfor (i = 0; i < revs->pending.nr; i++)\n +\t\tmb_child[i] = lookup_commit_reference(the_repository, &revs->pending.objects[i].item->oid);\n -+\tif (revs->pending.nr < ARRAY_SIZE(mb_child)) {\n ++\tif (revs->pending.nr == 1) {\n +\t\tstruct object_id oid;\n +\n -+\t\tif (revs->pending.nr != 1)\n -+\t\t\tBUG(\"unexpected revs->pending.nr: %d\", revs->pending.nr);\n -+\n +\t\tif (get_oid(\"HEAD\", &oid))\n +\t\t\tdie(_(\"unable to get HEAD\"));\n +\n 7: c9225a0440 = 7: 27930d4476 t4068: add --merge-base tests\n 8: 1e4f805e57 ! 8: f54baa4ecd builtin/diff-index: learn --merge-base\n @@ builtin/diff.c: static int builtin_diff_index(struct rev_info *revs,\n \t\targv++; argc--;\n \n ## diff-lib.c ##\n -@@ diff-lib.c: void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb)\n - int run_diff_index(struct rev_info *revs, unsigned int option)\n +@@ diff-lib.c: int run_diff_index(struct rev_info *revs, unsigned int option)\n {\n \tstruct object_array_entry *ent;\n + \tint cached = !!(option & DIFF_INDEX_CACHED);\n ++\tint merge_base = !!(option & DIFF_INDEX_MERGE_BASE);\n +\tstruct object_id oid;\n +\tconst char *name;\n ++\tchar merge_base_hex[GIT_MAX_HEXSZ + 1];\n \n \tif (revs->pending.nr != 1)\n \t\tBUG(\"run_diff_index must be passed exactly one tree\");\n \n \ttrace_performance_enter();\n \tent = revs->pending.objects;\n --\tif (diff_cache(revs, &ent->item->oid, ent->name, !!(option & DIFF_INDEX_CACHED)))\n +-\tif (diff_cache(revs, &ent->item->oid, ent->name, cached))\n +\n -+\tif (option & DIFF_INDEX_MERGE_BASE) {\n ++\tif (merge_base) {\n +\t\tdiff_get_merge_base(revs, &oid);\n -+\t\tname = xstrdup(oid_to_hex(&oid));\n ++\t\tname = oid_to_hex_r(merge_base_hex, &oid);\n +\t} else {\n +\t\toidcpy(&oid, &ent->item->oid);\n +\t\tname = ent->name;\n +\t}\n +\n -+\tif (diff_cache(revs, &oid, name, !!(option & DIFF_INDEX_CACHED)))\n ++\tif (diff_cache(revs, &oid, name, cached))\n \t\texit(128);\n \n - \tdiff_set_mnemonic_prefix(&revs->diffopt, \"c/\", (option & DIFF_INDEX_CACHED) ? \"i/\" : \"w/\");\n + \tdiff_set_mnemonic_prefix(&revs->diffopt, \"c/\", cached ? \"i/\" : \"w/\");\n \n ## diff.h ##\n @@ diff.h: void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb);\n - #define DIFF_RACY_IS_MODIFIED 02\n int run_diff_files(struct rev_info *revs, unsigned int option);\n + \n #define DIFF_INDEX_CACHED 01\n +#define DIFF_INDEX_MERGE_BASE 02\n int run_diff_index(struct rev_info *revs, unsigned int option);\n 9: c0d27b125e ! 9: 4880d21119 builtin/diff-tree: learn --merge-base\n @@ Metadata\n ## Commit message ##\n builtin/diff-tree: learn --merge-base\n \n - In order to get the diff between a commit and its merge base, the\n - currently preferred method is to use `git diff A...B`. However, the\n - range-notation with diff has, time and time again, been noted as a point\n - of confusion and thus, it should be avoided. Although we have a\n - substitute for the double-dot notation, we don't have any replacement\n - for the triple-dot notation.\n + The previous commit introduced ---merge-base a way to take the diff\n + between the working tree or index and the merge base between an arbitrary\n + commit and HEAD. It makes sense to extend this option to support the\n + case where two commits are given too and behave in a manner identical to\n + `git diff A...B`.\n \n - Introduce the --merge-base flag as a replacement for triple-dot\n + Introduce the --merge-base flag as an alternative to triple-dot\n notation. Thus, we would be able to write the above as\n - `git diff --merge-base A B`, allowing us to gently deprecate\n - range-notation completely.\n + `git diff --merge-base A B`.\n \n ## Documentation/git-diff-tree.txt ##\n @@ Documentation/git-diff-tree.txt: SYNOPSIS\n @@ Documentation/git-diff.txt: SYNOPSIS\n 'git diff' [] [] [--] [...]\n 'git diff' [] --cached [--merge-base] [] [--] [...]\n -'git diff' [] [...] [--] [...]\n --'git diff' [] ... [--] [...]\n +'git diff' [] [--merge-base] [...] [--] [...]\n + 'git diff' [] ... [--] [...]\n 'git diff' [] \n 'git diff' [] --no-index [--] \n - \n @@ Documentation/git-diff.txt: of and HEAD. `git diff --merge-base A` is equivalent to\n \tbranch name to compare with the tip of a different\n \tbranch.\n @@ builtin/diff.c\n \"git diff [] [] [--] [...]\\n\"\n \" or: git diff [] --cached [] [--] [...]\\n\"\n -\" or: git diff [] [...] [--] [...]\\n\"\n --\" or: git diff [] ...] [--] [...]\\n\"\n +\" or: git diff [] [--merge-base] [...] [--] [...]\\n\"\n + \" or: git diff [] ...] [--] [...]\\n\"\n \" or: git diff [] ]\\n\"\n \" or: git diff [] --no-index [--] ]\\n\"\n - COMMON_DIFF_OPTIONS_HELP;\n @@ builtin/diff.c: static int builtin_diff_tree(struct rev_info *revs,\n \t\t\t struct object_array_entry *ent1)\n {\n10: 42a8c9b665 = 10: 32e3e52b5f contrib/completion: complete `git diff --merge-base`\n-- \n2.28.0.760.g8d73e04208\n\n"},{"id":"405982","messageId":"3287e649f15fe8ca2dca924a29f36132bbce96f8.1600600823.git.liu.denton@gmail.com","threadId":"54193","inReplyTo":"cover.1600600823.git.liu.denton@gmail.com","subject":"[PATCH v4 06/10] diff-lib: define diff_get_merge_base()","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-09-20T11:22:23Z","receivedAt":"2020-09-20T11:22:47Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In a future commit, we will be using this function to implement\n--merge-base functionality in various diff commands.\n\nSigned-off-by: Denton Liu \n---\n diff-lib.c | 45 +++++++++++++++++++++++++++++++++++++++++++++\n diff.h | 2 ++\n 2 files changed, 47 insertions(+)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex d5b2c8af56..fa64e64bbe 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -13,6 +13,7 @@\n #include \"submodule.h\"\n #include \"dir.h\"\n #include \"fsmonitor.h\"\n+#include \"commit-reach.h\"\n \n /*\n * diff-files\n@@ -518,6 +519,50 @@ static int diff_cache(struct rev_info *revs,\n \treturn unpack_trees(1, &t, &opts);\n }\n \n+void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb)\n+{\n+\tint i;\n+\tstruct commit *mb_child[2] = {0};\n+\tstruct commit_list *merge_bases;\n+\n+\tfor (i = 0; i < revs->pending.nr; i++) {\n+\t\tstruct object *obj = revs->pending.objects[i].item;\n+\t\tif (obj->flags)\n+\t\t\tdie(_(\"--merge-base does not work with ranges\"));\n+\t\tif (obj->type != OBJ_COMMIT)\n+\t\t\tdie(_(\"--merge-base only works with commits\"));\n+\t}\n+\n+\t/*\n+\t * This check must go after the for loop above because A...B\n+\t * ranges produce three pending commits, resulting in a\n+\t * misleading error message.\n+\t */\n+\tif (revs->pending.nr < 1 || revs->pending.nr > 2)\n+\t\tBUG(\"unexpected revs->pending.nr: %d\", revs->pending.nr);\n+\n+\tfor (i = 0; i < revs->pending.nr; i++)\n+\t\tmb_child[i] = lookup_commit_reference(the_repository, &revs->pending.objects[i].item->oid);\n+\tif (revs->pending.nr == 1) {\n+\t\tstruct object_id oid;\n+\n+\t\tif (get_oid(\"HEAD\", &oid))\n+\t\t\tdie(_(\"unable to get HEAD\"));\n+\n+\t\tmb_child[1] = lookup_commit_reference(the_repository, &oid);\n+\t}\n+\n+\tmerge_bases = repo_get_merge_bases(the_repository, mb_child[0], mb_child[1]);\n+\tif (!merge_bases)\n+\t\tdie(_(\"no merge base found\"));\n+\tif (merge_bases->next)\n+\t\tdie(_(\"multiple merge bases found\"));\n+\n+\toidcpy(mb, &merge_bases->item->object.oid);\n+\n+\tfree_commit_list(merge_bases);\n+}\n+\n int run_diff_index(struct rev_info *revs, unsigned int option)\n {\n \tstruct object_array_entry *ent;\ndiff --git a/diff.h b/diff.h\nindex aea0d5b96b..fedfeab7a2 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -580,6 +580,8 @@ void diff_warn_rename_limit(const char *varname, int needed, int degraded_cc);\n */\n const char *diff_aligned_abbrev(const struct object_id *sha1, int);\n \n+void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb);\n+\n /* do not report anything on removed paths */\n #define DIFF_SILENT_ON_REMOVED 01\n /* report racily-clean paths as modified */\n-- \n2.28.0.760.g8d73e04208\n\n"},{"id":"405983","messageId":"99d8b51585b0b6831f3feaefb83de8390689b683.1600600823.git.liu.denton@gmail.com","threadId":"54193","inReplyTo":"cover.1600600823.git.liu.denton@gmail.com","subject":"[PATCH v4 05/10] diff-lib: accept option flags in run_diff_index()","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-09-20T11:22:22Z","receivedAt":"2020-09-20T11:22:51Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In a future commit, we will teach run_diff_index() to accept more\noptions via flag bits. For now, change `cached` into a flag in the\n`option` bitfield. The behaviour should remain exactly the same.\n\nSigned-off-by: Denton Liu \n---\n builtin/diff-index.c | 8 ++++----\n builtin/diff.c | 8 ++++----\n diff-lib.c | 3 ++-\n diff.h | 4 +++-\n 4 files changed, 13 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/diff-index.c b/builtin/diff-index.c\nindex 93ec642423..c3878f7ad6 100644\n--- a/builtin/diff-index.c\n+++ b/builtin/diff-index.c\n@@ -15,7 +15,7 @@ COMMON_DIFF_OPTIONS_HELP;\n int cmd_diff_index(int argc, const char **argv, const char *prefix)\n {\n \tstruct rev_info rev;\n-\tint cached = 0;\n+\tunsigned int option = 0;\n \tint i;\n \tint result;\n \n@@ -32,7 +32,7 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)\n \t\tconst char *arg = argv[i];\n \n \t\tif (!strcmp(arg, \"--cached\"))\n-\t\t\tcached = 1;\n+\t\t\toption |= DIFF_INDEX_CACHED;\n \t\telse\n \t\t\tusage(diff_cache_usage);\n \t}\n@@ -46,7 +46,7 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)\n \tif (rev.pending.nr != 1 ||\n \t rev.max_count != -1 || rev.min_age != -1 || rev.max_age != -1)\n \t\tusage(diff_cache_usage);\n-\tif (!cached) {\n+\tif (!(option & DIFF_INDEX_CACHED)) {\n \t\tsetup_work_tree();\n \t\tif (read_cache_preload(&rev.diffopt.pathspec) < 0) {\n \t\t\tperror(\"read_cache_preload\");\n@@ -56,7 +56,7 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)\n \t\tperror(\"read_cache\");\n \t\treturn -1;\n \t}\n-\tresult = run_diff_index(&rev, cached);\n+\tresult = run_diff_index(&rev, option);\n \tUNLEAK(rev);\n \treturn diff_result_code(&rev.diffopt, result);\n }\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex cb98811c21..e45e19e37e 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -134,11 +134,11 @@ static int builtin_diff_blobs(struct rev_info *revs,\n static int builtin_diff_index(struct rev_info *revs,\n \t\t\t int argc, const char **argv)\n {\n-\tint cached = 0;\n+\tunsigned int option = 0;\n \twhile (1 < argc) {\n \t\tconst char *arg = argv[1];\n \t\tif (!strcmp(arg, \"--cached\") || !strcmp(arg, \"--staged\"))\n-\t\t\tcached = 1;\n+\t\t\toption |= DIFF_INDEX_CACHED;\n \t\telse\n \t\t\tusage(builtin_diff_usage);\n \t\targv++; argc--;\n@@ -151,7 +151,7 @@ static int builtin_diff_index(struct rev_info *revs,\n \t revs->max_count != -1 || revs->min_age != -1 ||\n \t revs->max_age != -1)\n \t\tusage(builtin_diff_usage);\n-\tif (!cached) {\n+\tif (!(option & DIFF_INDEX_CACHED)) {\n \t\tsetup_work_tree();\n \t\tif (read_cache_preload(&revs->diffopt.pathspec) < 0) {\n \t\t\tperror(\"read_cache_preload\");\n@@ -161,7 +161,7 @@ static int builtin_diff_index(struct rev_info *revs,\n \t\tperror(\"read_cache\");\n \t\treturn -1;\n \t}\n-\treturn run_diff_index(revs, cached);\n+\treturn run_diff_index(revs, option);\n }\n \n static int builtin_diff_tree(struct rev_info *revs,\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 50521e2093..d5b2c8af56 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -518,9 +518,10 @@ static int diff_cache(struct rev_info *revs,\n \treturn unpack_trees(1, &t, &opts);\n }\n \n-int run_diff_index(struct rev_info *revs, int cached)\n+int run_diff_index(struct rev_info *revs, unsigned int option)\n {\n \tstruct object_array_entry *ent;\n+\tint cached = !!(option & DIFF_INDEX_CACHED);\n \n \tif (revs->pending.nr != 1)\n \t\tBUG(\"run_diff_index must be passed exactly one tree\");\ndiff --git a/diff.h b/diff.h\nindex e0c0af6286..aea0d5b96b 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -585,7 +585,9 @@ const char *diff_aligned_abbrev(const struct object_id *sha1, int);\n /* report racily-clean paths as modified */\n #define DIFF_RACY_IS_MODIFIED 02\n int run_diff_files(struct rev_info *revs, unsigned int option);\n-int run_diff_index(struct rev_info *revs, int cached);\n+\n+#define DIFF_INDEX_CACHED 01\n+int run_diff_index(struct rev_info *revs, unsigned int option);\n \n int do_diff_cache(const struct object_id *, struct diff_options *);\n int diff_flush_patch_id(struct diff_options *, struct object_id *, int, int);\n-- \n2.28.0.760.g8d73e04208\n\n"},{"id":"405984","messageId":"ca9568c2ea7d28605633576609de53a2d79df22e.1600600823.git.liu.denton@gmail.com","threadId":"54193","inReplyTo":"cover.1600600823.git.liu.denton@gmail.com","subject":"[PATCH v4 03/10] git-diff.txt: backtick quote command text","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-09-20T11:22:20Z","receivedAt":"2020-09-20T11:22:51Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The modern way to quote commands in the documentation is to use\nbackticks instead of double-quotes as this renders the text with the\ncode style. Convert double-quoted command text to backtick-quoted\ncommands. While we're at it, quote one instance of `^@`.\n\nSigned-off-by: Denton Liu \n---\n Documentation/git-diff.txt | 18 +++++++++---------\n 1 file changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt\nindex 727f24d16e..8f7b4ed3ca 100644\n--- a/Documentation/git-diff.txt\n+++ b/Documentation/git-diff.txt\n@@ -68,13 +68,13 @@ files on disk.\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+\tthe desired set of revisions is to use the `^@` 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 is synonymous to the earlier form (without the \"..\") for\n+\tThis is synonymous to the earlier form (without the `..`) for\n \tviewing the changes between two arbitrary . If on\n \tone side is omitted, it will have the same effect as\n \tusing HEAD instead.\n@@ -83,20 +83,20 @@ files on disk.\n \n \tThis form is to view the changes on the branch containing\n \tand up to the second , starting at a common ancestor\n-\tof both . \"git diff A\\...B\" is equivalent to\n-\t\"git diff $(git merge-base A B) B\". You can omit any one\n+\tof both . `git diff A...B` is equivalent to\n+\t`git diff $(git merge-base A B) B`. You can omit any one\n \tof , which has the same effect as using HEAD instead.\n \n Just in case you are doing something exotic, it should be\n noted that all of the in the above description, except\n-in the last two forms that use \"..\" notations, can be any\n+in the last two forms that use `..` notations, can be any\n .\n \n For a more complete list of ways to spell , see\n \"SPECIFYING REVISIONS\" section in linkgit:gitrevisions[7].\n However, \"diff\" is about comparing two _endpoints_, not ranges,\n-and the range notations (\"..\" and\n-\"\\...\") do not mean a range as defined in the\n+and the range notations (`..` and\n+`...`) do not mean a range as defined in the\n \"SPECIFYING RANGES\" section in linkgit:gitrevisions[7].\n \n 'git diff' [] ::\n@@ -144,9 +144,9 @@ $ git diff HEAD <3>\n +\n <1> Changes in the working tree not yet staged for the next commit.\n <2> Changes between the index and your last commit; what you\n- would be committing if you run \"git commit\" without \"-a\" option.\n+ would be committing if you run `git commit` without `-a` option.\n <3> Changes in the working tree since your last commit; what you\n- would be committing if you run \"git commit -a\"\n+ would be committing if you run `git commit -a`\n \n Comparing with arbitrary commits::\n +\n-- \n2.28.0.760.g8d73e04208\n\n"},{"id":"405985","messageId":"27930d44769503103533c8c38212d7eaa2856a8e.1600600823.git.liu.denton@gmail.com","threadId":"54193","inReplyTo":"cover.1600600823.git.liu.denton@gmail.com","subject":"[PATCH v4 07/10] t4068: add --merge-base tests","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-09-20T11:22:24Z","receivedAt":"2020-09-20T11:22:51Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In the future, we will be adding more --merge-base tests to this test\nscript. To prepare for that, rename the script accordingly and update\nits description. Also, add two basic --merge-base tests that don't\nrequire any functionality to be implemented yet.\n\nSigned-off-by: Denton Liu \n---\n ...ymmetric.sh => t4068-diff-symmetric-merge-base.sh} | 11 ++++++++++-\n 1 file changed, 10 insertions(+), 1 deletion(-)\n rename t/{t4068-diff-symmetric.sh => t4068-diff-symmetric-merge-base.sh} (86%)\n\ndiff --git a/t/t4068-diff-symmetric.sh b/t/t4068-diff-symmetric-merge-base.sh\nsimilarity index 86%\nrename from t/t4068-diff-symmetric.sh\nrename to t/t4068-diff-symmetric-merge-base.sh\nindex 60c506c2b2..bd4cf254d9 100755\n--- a/t/t4068-diff-symmetric.sh\n+++ b/t/t4068-diff-symmetric-merge-base.sh\n@@ -1,6 +1,6 @@\n #!/bin/sh\n \n-test_description='behavior of diff with symmetric-diff setups'\n+test_description='behavior of diff with symmetric-diff setups and --merge-base'\n \n . ./test-lib.sh\n \n@@ -88,4 +88,13 @@ test_expect_success 'diff with ranges and extra arg' '\n \ttest_i18ngrep \"usage\" err\n '\n \n+test_expect_success 'diff --merge-base with no commits' '\n+\ttest_must_fail git diff --merge-base\n+'\n+\n+test_expect_success 'diff --merge-base with three commits' '\n+\ttest_must_fail git diff --merge-base br1 br2 master 2>err &&\n+\ttest_i18ngrep \"usage\" err\n+'\n+\n test_done\n-- \n2.28.0.760.g8d73e04208\n\n"},{"id":"405986","messageId":"f54baa4ecd495026c2fce1772b560b7bbbeecc04.1600600823.git.liu.denton@gmail.com","threadId":"54193","inReplyTo":"cover.1600600823.git.liu.denton@gmail.com","subject":"[PATCH v4 08/10] builtin/diff-index: learn --merge-base","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-09-20T11:22:25Z","receivedAt":"2020-09-20T11:22:52Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"There is currently no easy way to take the diff between the working tree\nor index and the merge base between an arbitrary commit and HEAD. Even\ndiff's `...` notation doesn't allow this because it only works between\ncommits. However, the ability to do this would be desirable to a user\nwho would like to see all the changes they've made on a branch plus\nuncommitted changes without taking into account changes made in the\nupstream branch.\n\nTeach diff-index and diff (with one commit) the --merge-base option\nwhich allows a user to use the merge base of a commit and HEAD as the\n\"before\" side.\n\nSigned-off-by: Denton Liu \n---\n Documentation/git-diff-index.txt | 7 +++-\n Documentation/git-diff.txt | 12 ++++--\n builtin/diff-index.c | 2 +\n builtin/diff.c | 2 +\n diff-lib.c | 15 ++++++-\n diff.h | 1 +\n t/t4068-diff-symmetric-merge-base.sh | 59 ++++++++++++++++++++++++++++\n 7 files changed, 92 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-diff-index.txt b/Documentation/git-diff-index.txt\nindex 25fe165f00..27acb31cbf 100644\n--- a/Documentation/git-diff-index.txt\n+++ b/Documentation/git-diff-index.txt\n@@ -9,7 +9,7 @@ git-diff-index - Compare a tree to the working tree or index\n SYNOPSIS\n --------\n [verse]\n-'git diff-index' [-m] [--cached] [] [...]\n+'git diff-index' [-m] [--cached] [--merge-base] [] [...]\n \n DESCRIPTION\n -----------\n@@ -29,6 +29,11 @@ include::diff-options.txt[]\n --cached::\n \tDo not consider the on-disk file at all.\n \n+--merge-base::\n+\tInstead of comparing directly, use the merge base\n+\tbetween and HEAD instead. must be a\n+\tcommit.\n+\n -m::\n \tBy default, files recorded in the index but not checked\n \tout are reported as deleted. This flag makes\ndiff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt\nindex 8f7b4ed3ca..762ee6d074 100644\n--- a/Documentation/git-diff.txt\n+++ b/Documentation/git-diff.txt\n@@ -10,7 +10,7 @@ SYNOPSIS\n --------\n [verse]\n 'git diff' [] [] [--] [...]\n-'git diff' [] --cached [] [--] [...]\n+'git diff' [] --cached [--merge-base] [] [--] [...]\n 'git diff' [] [...] [--] [...]\n 'git diff' [] ... [--] [...]\n 'git diff' [] \n@@ -40,7 +40,7 @@ files on disk.\n \tor when running the command outside a working tree\n \tcontrolled by Git. This form implies `--exit-code`.\n \n-'git diff' [] --cached [] [--] [...]::\n+'git diff' [] --cached [--merge-base] [] [--] [...]::\n \n \tThis form is to view the changes you staged for the next\n \tcommit relative to the named . Typically you\n@@ -49,6 +49,10 @@ files on disk.\n \tIf HEAD does not exist (e.g. unborn branches) and\n \t is not given, it shows all staged changes.\n \t--staged is a synonym of --cached.\n++\n+If --merge-base is given, instead of using , use the merge base\n+of and HEAD. `git diff --merge-base A` is equivalent to\n+`git diff $(git merge-base A HEAD)`.\n \n 'git diff' [] [--] [...]::\n \n@@ -89,8 +93,8 @@ files on disk.\n \n Just in case you are doing something exotic, it should be\n noted that all of the in the above description, except\n-in the last two forms that use `..` notations, can be any\n-.\n+in the `--merge-base` case and in the last two forms that use `..`\n+notations, can be any .\n \n For a more complete list of ways to spell , see\n \"SPECIFYING REVISIONS\" section in linkgit:gitrevisions[7].\ndiff --git a/builtin/diff-index.c b/builtin/diff-index.c\nindex c3878f7ad6..7f5281c461 100644\n--- a/builtin/diff-index.c\n+++ b/builtin/diff-index.c\n@@ -33,6 +33,8 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)\n \n \t\tif (!strcmp(arg, \"--cached\"))\n \t\t\toption |= DIFF_INDEX_CACHED;\n+\t\telse if (!strcmp(arg, \"--merge-base\"))\n+\t\t\toption |= DIFF_INDEX_MERGE_BASE;\n \t\telse\n \t\t\tusage(diff_cache_usage);\n \t}\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex e45e19e37e..1baea18ae0 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -139,6 +139,8 @@ static int builtin_diff_index(struct rev_info *revs,\n \t\tconst char *arg = argv[1];\n \t\tif (!strcmp(arg, \"--cached\") || !strcmp(arg, \"--staged\"))\n \t\t\toption |= DIFF_INDEX_CACHED;\n+\t\telse if (!strcmp(arg, \"--merge-base\"))\n+\t\t\toption |= DIFF_INDEX_MERGE_BASE;\n \t\telse\n \t\t\tusage(builtin_diff_usage);\n \t\targv++; argc--;\ndiff --git a/diff-lib.c b/diff-lib.c\nindex fa64e64bbe..79defdc6b8 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -567,13 +567,26 @@ int run_diff_index(struct rev_info *revs, unsigned int option)\n {\n \tstruct object_array_entry *ent;\n \tint cached = !!(option & DIFF_INDEX_CACHED);\n+\tint merge_base = !!(option & DIFF_INDEX_MERGE_BASE);\n+\tstruct object_id oid;\n+\tconst char *name;\n+\tchar merge_base_hex[GIT_MAX_HEXSZ + 1];\n \n \tif (revs->pending.nr != 1)\n \t\tBUG(\"run_diff_index must be passed exactly one tree\");\n \n \ttrace_performance_enter();\n \tent = revs->pending.objects;\n-\tif (diff_cache(revs, &ent->item->oid, ent->name, cached))\n+\n+\tif (merge_base) {\n+\t\tdiff_get_merge_base(revs, &oid);\n+\t\tname = oid_to_hex_r(merge_base_hex, &oid);\n+\t} else {\n+\t\toidcpy(&oid, &ent->item->oid);\n+\t\tname = ent->name;\n+\t}\n+\n+\tif (diff_cache(revs, &oid, name, cached))\n \t\texit(128);\n \n \tdiff_set_mnemonic_prefix(&revs->diffopt, \"c/\", cached ? \"i/\" : \"w/\");\ndiff --git a/diff.h b/diff.h\nindex fedfeab7a2..6c2efa16fd 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -589,6 +589,7 @@ void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb);\n int run_diff_files(struct rev_info *revs, unsigned int option);\n \n #define DIFF_INDEX_CACHED 01\n+#define DIFF_INDEX_MERGE_BASE 02\n int run_diff_index(struct rev_info *revs, unsigned int option);\n \n int do_diff_cache(const struct object_id *, struct diff_options *);\ndiff --git a/t/t4068-diff-symmetric-merge-base.sh b/t/t4068-diff-symmetric-merge-base.sh\nindex bd4cf254d9..49432379cb 100755\n--- a/t/t4068-diff-symmetric-merge-base.sh\n+++ b/t/t4068-diff-symmetric-merge-base.sh\n@@ -97,4 +97,63 @@ test_expect_success 'diff --merge-base with three commits' '\n \ttest_i18ngrep \"usage\" err\n '\n \n+for cmd in diff-index diff\n+do\n+\ttest_expect_success \"$cmd --merge-base with one commit\" '\n+\t\tgit checkout master &&\n+\t\tgit $cmd commit-C >expect &&\n+\t\tgit $cmd --merge-base br2 >actual &&\n+\t\ttest_cmp expect actual\n+\t'\n+\n+\ttest_expect_success \"$cmd --merge-base with one commit and unstaged changes\" '\n+\t\tgit checkout master &&\n+\t\ttest_when_finished git reset --hard &&\n+\t\techo unstaged >>c &&\n+\t\tgit $cmd commit-C >expect &&\n+\t\tgit $cmd --merge-base br2 >actual &&\n+\t\ttest_cmp expect actual\n+\t'\n+\n+\ttest_expect_success \"$cmd --merge-base with one commit and staged and unstaged changes\" '\n+\t\tgit checkout master &&\n+\t\ttest_when_finished git reset --hard &&\n+\t\techo staged >>c &&\n+\t\tgit add c &&\n+\t\techo unstaged >>c &&\n+\t\tgit $cmd commit-C >expect &&\n+\t\tgit $cmd --merge-base br2 >actual &&\n+\t\ttest_cmp expect actual\n+\t'\n+\n+\ttest_expect_success \"$cmd --merge-base --cached with one commit and staged and unstaged changes\" '\n+\t\tgit checkout master &&\n+\t\ttest_when_finished git reset --hard &&\n+\t\techo staged >>c &&\n+\t\tgit add c &&\n+\t\techo unstaged >>c &&\n+\t\tgit $cmd --cached commit-C >expect &&\n+\t\tgit $cmd --cached --merge-base br2 >actual &&\n+\t\ttest_cmp expect actual\n+\t'\n+\n+\ttest_expect_success \"$cmd --merge-base with non-commit\" '\n+\t\tgit checkout master &&\n+\t\ttest_must_fail git $cmd --merge-base master^{tree} 2>err &&\n+\t\ttest_i18ngrep \"fatal: --merge-base only works with commits\" err\n+\t'\n+\n+\ttest_expect_success \"$cmd --merge-base with no merge bases and one commit\" '\n+\t\tgit checkout master &&\n+\t\ttest_must_fail git $cmd --merge-base br3 2>err &&\n+\t\ttest_i18ngrep \"fatal: no merge base found\" err\n+\t'\n+\n+\ttest_expect_success \"$cmd --merge-base with multiple merge bases and one commit\" '\n+\t\tgit checkout master &&\n+\t\ttest_must_fail git $cmd --merge-base br1 2>err &&\n+\t\ttest_i18ngrep \"fatal: multiple merge bases found\" err\n+\t'\n+done\n+\n test_done\n-- \n2.28.0.760.g8d73e04208\n\n"},{"id":"405987","messageId":"1ac84595419d8d1ecd8129395c3261ebca643bc2.1600600823.git.liu.denton@gmail.com","threadId":"54193","inReplyTo":"cover.1600600823.git.liu.denton@gmail.com","subject":"[PATCH v4 04/10] contrib/completion: extract common diff/difftool options","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-09-20T11:22:21Z","receivedAt":"2020-09-20T11:22:54Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"difftool parses its own options and then passes the remaining options\nonto diff. As a result, they share common command-line options. Instead\nof duplicating the list, use a shared $__git_diff_difftool_options list.\n\nThe completion for diff is missing --relative and the completion for\ndifftool is missing --no-index. Add both of these to the common list.\n\nSigned-off-by: Denton Liu \n---\n contrib/completion/git-completion.bash | 15 ++++++---------\n 1 file changed, 6 insertions(+), 9 deletions(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 9147fba3d5..f68c8e0646 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -1691,6 +1691,10 @@ __git_diff_common_options=\"--stat --numstat --shortstat --summary\n \t\t\t--textconv --no-textconv\n \"\n \n+__git_diff_difftool_options=\"--cached --staged --pickaxe-all --pickaxe-regex\n+\t\t\t--base --ours --theirs --no-index --relative\n+\t\t\t$__git_diff_common_options\"\n+\n _git_diff ()\n {\n \t__git_has_doubledash && return\n@@ -1713,10 +1717,7 @@ _git_diff ()\n \t\treturn\n \t\t;;\n \t--*)\n-\t\t__gitcomp \"--cached --staged --pickaxe-all --pickaxe-regex\n-\t\t\t--base --ours --theirs --no-index\n-\t\t\t$__git_diff_common_options\n-\t\t\t\"\n+\t\t__gitcomp \"$__git_diff_difftool_options\"\n \t\treturn\n \t\t;;\n \tesac\n@@ -1738,11 +1739,7 @@ _git_difftool ()\n \t\treturn\n \t\t;;\n \t--*)\n-\t\t__gitcomp_builtin difftool \"$__git_diff_common_options\n-\t\t\t\t\t--base --cached --ours --theirs\n-\t\t\t\t\t--pickaxe-all --pickaxe-regex\n-\t\t\t\t\t--relative --staged\n-\t\t\t\t\t\"\n+\t\t__gitcomp_builtin difftool \"$__git_diff_difftool_options\"\n \t\treturn\n \t\t;;\n \tesac\n-- \n2.28.0.760.g8d73e04208\n\n"},{"id":"405988","messageId":"4880d21119c8b67ab53db8ce82196855f6e8680f.1600600823.git.liu.denton@gmail.com","threadId":"54193","inReplyTo":"cover.1600600823.git.liu.denton@gmail.com","subject":"[PATCH v4 09/10] builtin/diff-tree: learn --merge-base","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-09-20T11:22:26Z","receivedAt":"2020-09-20T11:22:55Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The previous commit introduced ---merge-base a way to take the diff\nbetween the working tree or index and the merge base between an arbitrary\ncommit and HEAD. It makes sense to extend this option to support the\ncase where two commits are given too and behave in a manner identical to\n`git diff A...B`.\n\nIntroduce the --merge-base flag as an alternative to triple-dot\nnotation. Thus, we would be able to write the above as\n`git diff --merge-base A B`.\n\nSigned-off-by: Denton Liu \n---\n Documentation/git-diff-tree.txt | 7 ++++-\n Documentation/git-diff.txt | 8 ++++--\n builtin/diff-tree.c | 18 +++++++++++++\n builtin/diff.c | 39 +++++++++++++++++++---------\n t/t4068-diff-symmetric-merge-base.sh | 34 ++++++++++++++++++++++++\n 5 files changed, 91 insertions(+), 15 deletions(-)\n\ndiff --git a/Documentation/git-diff-tree.txt b/Documentation/git-diff-tree.txt\nindex 5c8a2a5e97..2fc24c542f 100644\n--- a/Documentation/git-diff-tree.txt\n+++ b/Documentation/git-diff-tree.txt\n@@ -10,7 +10,7 @@ SYNOPSIS\n --------\n [verse]\n 'git diff-tree' [--stdin] [-m] [-s] [-v] [--no-commit-id] [--pretty]\n-\t [-t] [-r] [-c | --cc] [--combined-all-paths] [--root]\n+\t [-t] [-r] [-c | --cc] [--combined-all-paths] [--root] [--merge-base]\n \t [] [] [...]\n \n DESCRIPTION\n@@ -43,6 +43,11 @@ include::diff-options.txt[]\n \tWhen `--root` is specified the initial commit will be shown as a big\n \tcreation event. This is equivalent to a diff against the NULL tree.\n \n+--merge-base::\n+\tInstead of comparing the s directly, use the merge\n+\tbase between the two s as the \"before\" side. There\n+\tmust be two s given and they must both be commits.\n+\n --stdin::\n \tWhen `--stdin` is specified, the command does not take\n \t arguments from the command line. Instead, it\ndiff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt\nindex 762ee6d074..7f4c8a8ce7 100644\n--- a/Documentation/git-diff.txt\n+++ b/Documentation/git-diff.txt\n@@ -11,7 +11,7 @@ SYNOPSIS\n [verse]\n 'git diff' [] [] [--] [...]\n 'git diff' [] --cached [--merge-base] [] [--] [...]\n-'git diff' [] [...] [--] [...]\n+'git diff' [] [--merge-base] [...] [--] [...]\n 'git diff' [] ... [--] [...]\n 'git diff' [] \n 'git diff' [] --no-index [--] \n@@ -62,10 +62,14 @@ of and HEAD. `git diff --merge-base A` is equivalent to\n \tbranch name to compare with the tip of a different\n \tbranch.\n \n-'git diff' [] [--] [...]::\n+'git diff' [] [--merge-base] [--] [...]::\n \n \tThis is to view the changes between two arbitrary\n \t.\n++\n+If --merge-base is given, use the merge base of the two commits for the\n+\"before\" side. `git diff --merge-base A B` is equivalent to\n+`git diff $(git merge-base A B) B`.\n \n 'git diff' [] ... [--] [...]::\n \ndiff --git a/builtin/diff-tree.c b/builtin/diff-tree.c\nindex 802363d0a2..823d6678e5 100644\n--- a/builtin/diff-tree.c\n+++ b/builtin/diff-tree.c\n@@ -111,6 +111,7 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)\n \tstruct setup_revision_opt s_r_opt;\n \tstruct userformat_want w;\n \tint read_stdin = 0;\n+\tint merge_base = 0;\n \n \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n \t\tusage(diff_tree_usage);\n@@ -143,9 +144,26 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)\n \t\t\tread_stdin = 1;\n \t\t\tcontinue;\n \t\t}\n+\t\tif (!strcmp(arg, \"--merge-base\")) {\n+\t\t\tmerge_base = 1;\n+\t\t\tcontinue;\n+\t\t}\n \t\tusage(diff_tree_usage);\n \t}\n \n+\tif (read_stdin && merge_base)\n+\t\tdie(_(\"--stdin and --merge-base are mutually exclusive\"));\n+\n+\tif (merge_base) {\n+\t\tstruct object_id oid;\n+\n+\t\tif (opt->pending.nr != 2)\n+\t\t\tdie(_(\"--merge-base only works with two commits\"));\n+\n+\t\tdiff_get_merge_base(opt, &oid);\n+\t\topt->pending.objects[0].item = lookup_object(the_repository, &oid);\n+\t}\n+\n \t/*\n \t * NOTE! We expect \"a..b\" to expand to \"^a b\" but it is\n \t * perfectly valid for revision range parser to yield \"b ^a\",\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 1baea18ae0..b50fc68c2a 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -26,7 +26,7 @@\n static const char builtin_diff_usage[] =\n \"git diff [] [] [--] [...]\\n\"\n \" or: git diff [] --cached [] [--] [...]\\n\"\n-\" or: git diff [] [...] [--] [...]\\n\"\n+\" or: git diff [] [--merge-base] [...] [--] [...]\\n\"\n \" or: git diff [] ...] [--] [...]\\n\"\n \" or: git diff [] ]\\n\"\n \" or: git diff [] --no-index [--] ]\\n\"\n@@ -172,19 +172,34 @@ static int builtin_diff_tree(struct rev_info *revs,\n \t\t\t struct object_array_entry *ent1)\n {\n \tconst struct object_id *(oid[2]);\n-\tint swap = 0;\n+\tstruct object_id mb_oid;\n+\tint merge_base = 0;\n \n-\tif (argc > 1)\n-\t\tusage(builtin_diff_usage);\n+\twhile (1 < argc) {\n+\t\tconst char *arg = argv[1];\n+\t\tif (!strcmp(arg, \"--merge-base\"))\n+\t\t\tmerge_base = 1;\n+\t\telse\n+\t\t\tusage(builtin_diff_usage);\n+\t\targv++; argc--;\n+\t}\n \n-\t/*\n-\t * We saw two trees, ent0 and ent1. If ent1 is uninteresting,\n-\t * swap them.\n-\t */\n-\tif (ent1->item->flags & UNINTERESTING)\n-\t\tswap = 1;\n-\toid[swap] = &ent0->item->oid;\n-\toid[1 - swap] = &ent1->item->oid;\n+\tif (merge_base) {\n+\t\tdiff_get_merge_base(revs, &mb_oid);\n+\t\toid[0] = &mb_oid;\n+\t\toid[1] = &revs->pending.objects[1].item->oid;\n+\t} else {\n+\t\tint swap = 0;\n+\n+\t\t/*\n+\t\t * We saw two trees, ent0 and ent1. If ent1 is uninteresting,\n+\t\t * swap them.\n+\t\t */\n+\t\tif (ent1->item->flags & UNINTERESTING)\n+\t\t\tswap = 1;\n+\t\toid[swap] = &ent0->item->oid;\n+\t\toid[1 - swap] = &ent1->item->oid;\n+\t}\n \tdiff_tree_oid(oid[0], oid[1], \"\", &revs->diffopt);\n \tlog_tree_diff_flush(revs);\n \treturn 0;\ndiff --git a/t/t4068-diff-symmetric-merge-base.sh b/t/t4068-diff-symmetric-merge-base.sh\nindex 49432379cb..03487cc945 100755\n--- a/t/t4068-diff-symmetric-merge-base.sh\n+++ b/t/t4068-diff-symmetric-merge-base.sh\n@@ -156,4 +156,38 @@ do\n \t'\n done\n \n+for cmd in diff-tree diff\n+do\n+\ttest_expect_success \"$cmd --merge-base with two commits\" '\n+\t\tgit $cmd commit-C master >expect &&\n+\t\tgit $cmd --merge-base br2 master >actual &&\n+\t\ttest_cmp expect actual\n+\t'\n+\n+\ttest_expect_success \"$cmd --merge-base commit and non-commit\" '\n+\t\ttest_must_fail git $cmd --merge-base br2 master^{tree} 2>err &&\n+\t\ttest_i18ngrep \"fatal: --merge-base only works with commits\" err\n+\t'\n+\n+\ttest_expect_success \"$cmd --merge-base with no merge bases and two commits\" '\n+\t\ttest_must_fail git $cmd --merge-base br2 br3 2>err &&\n+\t\ttest_i18ngrep \"fatal: no merge base found\" err\n+\t'\n+\n+\ttest_expect_success \"$cmd --merge-base with multiple merge bases and two commits\" '\n+\t\ttest_must_fail git $cmd --merge-base master br1 2>err &&\n+\t\ttest_i18ngrep \"fatal: multiple merge bases found\" err\n+\t'\n+done\n+\n+test_expect_success 'diff-tree --merge-base with one commit' '\n+\ttest_must_fail git diff-tree --merge-base master 2>err &&\n+\ttest_i18ngrep \"fatal: --merge-base only works with two commits\" err\n+'\n+\n+test_expect_success 'diff --merge-base with range' '\n+\ttest_must_fail git diff --merge-base br2..br3 2>err &&\n+\ttest_i18ngrep \"fatal: --merge-base does not work with ranges\" err\n+'\n+\n test_done\n-- \n2.28.0.760.g8d73e04208\n\n"},{"id":"405989","messageId":"32e3e52b5f4dbbb3050fe9fcff1a3c1f1d2df123.1600600823.git.liu.denton@gmail.com","threadId":"54193","inReplyTo":"cover.1600600823.git.liu.denton@gmail.com","subject":"[PATCH v4 10/10] contrib/completion: complete `git diff --merge-base`","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-09-20T11:22:27Z","receivedAt":"2020-09-20T11:22:57Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Signed-off-by: Denton Liu \n---\n contrib/completion/git-completion.bash | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex f68c8e0646..679d1ec8a8 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -1692,7 +1692,7 @@ __git_diff_common_options=\"--stat --numstat --shortstat --summary\n \"\n \n __git_diff_difftool_options=\"--cached --staged --pickaxe-all --pickaxe-regex\n-\t\t\t--base --ours --theirs --no-index --relative\n+\t\t\t--base --ours --theirs --no-index --relative --merge-base\n \t\t\t$__git_diff_common_options\"\n \n _git_diff ()\n-- \n2.28.0.760.g8d73e04208\n\n"},{"id":"406051","messageId":"xmqqzh5jf73t.fsf@gitster.c.googlers.com","threadId":"54193","inReplyTo":"20200920110148.GA227771@generichostname","subject":"Re: [PATCH v3 09/10] builtin/diff-tree: learn --merge-base","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-21T16:05:26Z","receivedAt":"2020-09-21T16:05:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu writes:\n\n> Hi Junio,\n>\n> On Fri, Sep 18, 2020 at 09:52:39AM -0700, Junio C Hamano wrote:\n>> I wonder if turning the builtin/diff-tree.c to match that structure\n>> make the result easier to understand (and I'll be perfectly happy if\n>> the answer to this question turns out to be \"no, the result of the\n>> posted patch is the easiest to follow\").\n>\n> git diff-tree does not even recognise ranges so as a result, the else\n> case does not even need to exist there, unlike in git diff.\n\n(caution: before morning caffeine so what I say may be totally off)\n\nDo you mean \"git diff-tree HEAD^..HEAD\" would fail, or something\nelse?\n\nThanks.\n"},{"id":"406058","messageId":"20200921172740.GA946178@generichostname","threadId":"54193","inReplyTo":"xmqqzh5jf73t.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3 09/10] builtin/diff-tree: learn --merge-base","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-09-21T17:27:40Z","receivedAt":"2020-09-21T17:27:45Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Junio,\n\nOn Mon, Sep 21, 2020 at 09:05:26AM -0700, Junio C Hamano wrote:\n> Denton Liu writes:\n> \n> > Hi Junio,\n> >\n> > On Fri, Sep 18, 2020 at 09:52:39AM -0700, Junio C Hamano wrote:\n> >> I wonder if turning the builtin/diff-tree.c to match that structure\n> >> make the result easier to understand (and I'll be perfectly happy if\n> >> the answer to this question turns out to be \"no, the result of the\n> >> posted patch is the easiest to follow\").\n> >\n> > git diff-tree does not even recognise ranges so as a result, the else\n> > case does not even need to exist there, unlike in git diff.\n> \n> (caution: before morning caffeine so what I say may be totally off)\n> \n> Do you mean \"git diff-tree HEAD^..HEAD\" would fail, or something\n> else?\n\nYes, that is what I meant but I can see that what I wrote is totally\nwrong. I was reading git-diff-tree.txt and I assumed that ranges were\nnot supported at all.\n\nAnyway, now that I've realised my mistake, I've rewritten the diff-tree\npart so that the structure matches what was written in diff and it\nshould be easier to follow.\n\n-- >8 --\n\nFrom: Denton Liu \nDate: Mon, 14 Sep 2020 11:36:52 -0700\nSubject: [PATCH] builtin/diff-tree: learn --merge-base\n\nThe previous commit introduced ---merge-base a way to take the diff\nbetween the working tree or index and the merge base between an arbitrary\ncommit and HEAD. It makes sense to extend this option to support the\ncase where two commits are given too and behave in a manner identical to\n`git diff A...B`.\n\nIntroduce the --merge-base flag as an alternative to triple-dot\nnotation. Thus, we would be able to write the above as\n`git diff --merge-base A B`.\n\nSigned-off-by: Denton Liu \n---\n Documentation/git-diff-tree.txt | 7 ++++-\n Documentation/git-diff.txt | 8 ++++--\n builtin/diff-tree.c | 17 +++++++++++-\n builtin/diff.c | 39 +++++++++++++++++++---------\n t/t4068-diff-symmetric-merge-base.sh | 34 ++++++++++++++++++++++++\n 5 files changed, 89 insertions(+), 16 deletions(-)\n\ndiff --git a/Documentation/git-diff-tree.txt b/Documentation/git-diff-tree.txt\nindex 5c8a2a5e97..2fc24c542f 100644\n--- a/Documentation/git-diff-tree.txt\n+++ b/Documentation/git-diff-tree.txt\n@@ -10,7 +10,7 @@ SYNOPSIS\n --------\n [verse]\n 'git diff-tree' [--stdin] [-m] [-s] [-v] [--no-commit-id] [--pretty]\n-\t [-t] [-r] [-c | --cc] [--combined-all-paths] [--root]\n+\t [-t] [-r] [-c | --cc] [--combined-all-paths] [--root] [--merge-base]\n \t [] [] [...]\n \n DESCRIPTION\n@@ -43,6 +43,11 @@ include::diff-options.txt[]\n \tWhen `--root` is specified the initial commit will be shown as a big\n \tcreation event. This is equivalent to a diff against the NULL tree.\n \n+--merge-base::\n+\tInstead of comparing the s directly, use the merge\n+\tbase between the two s as the \"before\" side. There\n+\tmust be two s given and they must both be commits.\n+\n --stdin::\n \tWhen `--stdin` is specified, the command does not take\n \t arguments from the command line. Instead, it\ndiff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt\nindex 762ee6d074..7f4c8a8ce7 100644\n--- a/Documentation/git-diff.txt\n+++ b/Documentation/git-diff.txt\n@@ -11,7 +11,7 @@ SYNOPSIS\n [verse]\n 'git diff' [] [] [--] [...]\n 'git diff' [] --cached [--merge-base] [] [--] [...]\n-'git diff' [] [...] [--] [...]\n+'git diff' [] [--merge-base] [...] [--] [...]\n 'git diff' [] ... [--] [...]\n 'git diff' [] \n 'git diff' [] --no-index [--] \n@@ -62,10 +62,14 @@ of and HEAD. `git diff --merge-base A` is equivalent to\n \tbranch name to compare with the tip of a different\n \tbranch.\n \n-'git diff' [] [--] [...]::\n+'git diff' [] [--merge-base] [--] [...]::\n \n \tThis is to view the changes between two arbitrary\n \t.\n++\n+If --merge-base is given, use the merge base of the two commits for the\n+\"before\" side. `git diff --merge-base A B` is equivalent to\n+`git diff $(git merge-base A B) B`.\n \n 'git diff' [] ... [--] [...]::\n \ndiff --git a/builtin/diff-tree.c b/builtin/diff-tree.c\nindex 802363d0a2..9fc95e959f 100644\n--- a/builtin/diff-tree.c\n+++ b/builtin/diff-tree.c\n@@ -111,6 +111,7 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)\n \tstruct setup_revision_opt s_r_opt;\n \tstruct userformat_want w;\n \tint read_stdin = 0;\n+\tint merge_base = 0;\n \n \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n \t\tusage(diff_tree_usage);\n@@ -143,9 +144,18 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)\n \t\t\tread_stdin = 1;\n \t\t\tcontinue;\n \t\t}\n+\t\tif (!strcmp(arg, \"--merge-base\")) {\n+\t\t\tmerge_base = 1;\n+\t\t\tcontinue;\n+\t\t}\n \t\tusage(diff_tree_usage);\n \t}\n \n+\tif (read_stdin && merge_base)\n+\t\tdie(_(\"--stdin and --merge-base are mutually exclusive\"));\n+\tif (merge_base && opt->pending.nr != 2)\n+\t\tdie(_(\"--merge-base only works with two commits\"));\n+\n \t/*\n \t * NOTE! We expect \"a..b\" to expand to \"^a b\" but it is\n \t * perfectly valid for revision range parser to yield \"b ^a\",\n@@ -165,7 +175,12 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)\n \tcase 2:\n \t\ttree1 = opt->pending.objects[0].item;\n \t\ttree2 = opt->pending.objects[1].item;\n-\t\tif (tree2->flags & UNINTERESTING) {\n+\t\tif (merge_base) {\n+\t\t\tstruct object_id oid;\n+\n+\t\t\tdiff_get_merge_base(opt, &oid);\n+\t\t\ttree1 = lookup_object(the_repository, &oid);\n+\t\t} else if (tree2->flags & UNINTERESTING) {\n \t\t\tSWAP(tree2, tree1);\n \t\t}\n \t\tdiff_tree_oid(&tree1->oid, &tree2->oid, \"\", &opt->diffopt);\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 1baea18ae0..b50fc68c2a 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -26,7 +26,7 @@\n static const char builtin_diff_usage[] =\n \"git diff [] [] [--] [...]\\n\"\n \" or: git diff [] --cached [] [--] [...]\\n\"\n-\" or: git diff [] [...] [--] [...]\\n\"\n+\" or: git diff [] [--merge-base] [...] [--] [...]\\n\"\n \" or: git diff [] ...] [--] [...]\\n\"\n \" or: git diff [] ]\\n\"\n \" or: git diff [] --no-index [--] ]\\n\"\n@@ -172,19 +172,34 @@ static int builtin_diff_tree(struct rev_info *revs,\n \t\t\t struct object_array_entry *ent1)\n {\n \tconst struct object_id *(oid[2]);\n-\tint swap = 0;\n+\tstruct object_id mb_oid;\n+\tint merge_base = 0;\n \n-\tif (argc > 1)\n-\t\tusage(builtin_diff_usage);\n+\twhile (1 < argc) {\n+\t\tconst char *arg = argv[1];\n+\t\tif (!strcmp(arg, \"--merge-base\"))\n+\t\t\tmerge_base = 1;\n+\t\telse\n+\t\t\tusage(builtin_diff_usage);\n+\t\targv++; argc--;\n+\t}\n \n-\t/*\n-\t * We saw two trees, ent0 and ent1. If ent1 is uninteresting,\n-\t * swap them.\n-\t */\n-\tif (ent1->item->flags & UNINTERESTING)\n-\t\tswap = 1;\n-\toid[swap] = &ent0->item->oid;\n-\toid[1 - swap] = &ent1->item->oid;\n+\tif (merge_base) {\n+\t\tdiff_get_merge_base(revs, &mb_oid);\n+\t\toid[0] = &mb_oid;\n+\t\toid[1] = &revs->pending.objects[1].item->oid;\n+\t} else {\n+\t\tint swap = 0;\n+\n+\t\t/*\n+\t\t * We saw two trees, ent0 and ent1. If ent1 is uninteresting,\n+\t\t * swap them.\n+\t\t */\n+\t\tif (ent1->item->flags & UNINTERESTING)\n+\t\t\tswap = 1;\n+\t\toid[swap] = &ent0->item->oid;\n+\t\toid[1 - swap] = &ent1->item->oid;\n+\t}\n \tdiff_tree_oid(oid[0], oid[1], \"\", &revs->diffopt);\n \tlog_tree_diff_flush(revs);\n \treturn 0;\ndiff --git a/t/t4068-diff-symmetric-merge-base.sh b/t/t4068-diff-symmetric-merge-base.sh\nindex 49432379cb..03487cc945 100755\n--- a/t/t4068-diff-symmetric-merge-base.sh\n+++ b/t/t4068-diff-symmetric-merge-base.sh\n@@ -156,4 +156,38 @@ do\n \t'\n done\n \n+for cmd in diff-tree diff\n+do\n+\ttest_expect_success \"$cmd --merge-base with two commits\" '\n+\t\tgit $cmd commit-C master >expect &&\n+\t\tgit $cmd --merge-base br2 master >actual &&\n+\t\ttest_cmp expect actual\n+\t'\n+\n+\ttest_expect_success \"$cmd --merge-base commit and non-commit\" '\n+\t\ttest_must_fail git $cmd --merge-base br2 master^{tree} 2>err &&\n+\t\ttest_i18ngrep \"fatal: --merge-base only works with commits\" err\n+\t'\n+\n+\ttest_expect_success \"$cmd --merge-base with no merge bases and two commits\" '\n+\t\ttest_must_fail git $cmd --merge-base br2 br3 2>err &&\n+\t\ttest_i18ngrep \"fatal: no merge base found\" err\n+\t'\n+\n+\ttest_expect_success \"$cmd --merge-base with multiple merge bases and two commits\" '\n+\t\ttest_must_fail git $cmd --merge-base master br1 2>err &&\n+\t\ttest_i18ngrep \"fatal: multiple merge bases found\" err\n+\t'\n+done\n+\n+test_expect_success 'diff-tree --merge-base with one commit' '\n+\ttest_must_fail git diff-tree --merge-base master 2>err &&\n+\ttest_i18ngrep \"fatal: --merge-base only works with two commits\" err\n+'\n+\n+test_expect_success 'diff --merge-base with range' '\n+\ttest_must_fail git diff --merge-base br2..br3 2>err &&\n+\ttest_i18ngrep \"fatal: --merge-base does not work with ranges\" err\n+'\n+\n test_done\n-- \n2.28.0.760.g8d73e04208\n\n"},{"id":"406074","messageId":"xmqqwo0met17.fsf@gitster.c.googlers.com","threadId":"54193","inReplyTo":"20200921172740.GA946178@generichostname","subject":"Re: [PATCH v3 09/10] builtin/diff-tree: learn --merge-base","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-21T21:09:24Z","receivedAt":"2020-09-21T21:09:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu writes:\n\n> @@ -165,7 +175,12 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)\n> \tcase 2:\n> \t\ttree1 = opt->pending.objects[0].item;\n> \t\ttree2 = opt->pending.objects[1].item;\n> -\t\tif (tree2->flags & UNINTERESTING) {\n> +\t\tif (merge_base) {\n> +\t\t\tstruct object_id oid;\n> +\n> +\t\t\tdiff_get_merge_base(opt, &oid);\n> +\t\t\ttree1 = lookup_object(the_repository, &oid);\n> +\t\t} else if (tree2->flags & UNINTERESTING) {\n> \t\t\tSWAP(tree2, tree1);\n> \t\t}\n> \t\tdiff_tree_oid(&tree1->oid, &tree2->oid, \"\", &opt->diffopt);\n\nOK. Handling this in that \"case 2\" does make sense.\n\nHowever.\n\nThe above code as-is will allow something like\n\n git diff --merge-base A..B\n\nand it will be taken the same as\n\n git diff --merge-base A B\n\nBut let's step back and think why we bother with SWAP() in the\nnormal case. This is due to the possibility that A..B, which\ncurrently is left in the pending.objects[] array as ^A B, might\nsomeday be stored as B ^A. If we leave that code to protect us from\nthe possibility, shouldn't we be protecting us from the same\n\"someday\" for the new code, too? \n\nThat is \"git diff --merge-base A..B\", when the control reaches this\npart of the code, may have tree1=B tree2=^A\n\nWhich suggests that a consistently written code would look like so:\n\n\ttree1 = opt->pending.objects[0].item;\n\ttree2 = opt->pending.objects[1].item;\n\n\tif (tree2->flags & UNINTERESTING)\n\t\t/* \n * A..B currently becomes ^A B but it is perfectly\n\t\t * ok for revision parser to leave us B ^A; detect\n\t\t * and swap them in the original order.\n\t\t */\n\t\tSWAP(tree2, tree1);\n\n\tif (merge_base) {\n\t\tstruct object_id oid;\n\n\t\tdiff_get_merge_base(opt, &oid);\n\t\ttree1 = lookup_object(the_repository, &oid);\n\t}\n\tdiff_tree_oid(&tree1->oid, &tree2->oid, \"\", &opt->diffopt);\n\tlog_tree_diff_flush(opt);\n\nAnother possibility is to error out when \"--merge-base A..B\" is\ngiven, which might be simpler. Then the code would look more like\n\n\n\ttree1 = ...\n\ttree2 = ...\n\n\tif (merge_base) {\n\t\tif ((tree1->flags | tree2->flags) & UNINTERESTING)\n\t\t\tdie(_(\"use of --merge-base with A..B forbidden\"));\n\t\t... get merge base and assign it to tree1 ...\n\t} else if (tree2->flags & UNINTERESTING) {\n\t\tSWAP();\n\t}\n\nWhile we are at it, what happens when \"--merge-base A...B\" is given?\n\nIn the original code without \"--merge-base\", \"git diff-tree A...B\"\nplaces the merge base between A and B in pending.objects[0] and B in\npending.objects[1], I think. \"git diff-tree --merge-base A...B\"\nwould further compute the merge base between these two objects, but\nluckily $(git merge-base $(merge-base A B) B) is the same as $(git\nmerge-base A B), so you won't get an incorrect answer from such a\nrequest. Is this something we want to diagnose as an error? I am\ninclined to say we should allow it (and if it hurts the user can\nstop doing so) as there is no harm done.\n\nThanks.\n\n\n"},{"id":"406076","messageId":"xmqqo8lyesk2.fsf@gitster.c.googlers.com","threadId":"54193","inReplyTo":"xmqqwo0met17.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3 09/10] builtin/diff-tree: learn --merge-base","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-21T21:19:41Z","receivedAt":"2020-09-21T21:19:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Another possibility is to error out when \"--merge-base A..B\" is\n> given, which might be simpler. Then the code would look more like\n> ...\n>\n> While we are at it, what happens when \"--merge-base A...B\" is given?\n>\n> ... Is this something we want to diagnose as an error? I am\n> inclined to say we should allow it (and if it hurts the user can\n> stop doing so) as there is no harm done.\n\nMy recommendation is to allow both \"git --merge-base A..B\" and \"git\n--merge-base A...B\". The discussion about A..B and SWAP() would\nequally apply to builtin/diff part of the patch. The posted patch\nignores the swap logic when --merge-base is given, but we should\napply the swap logic first and then make sure the merge_base logic\nwill have the oid[0] and oid[1] in the correct order.\n"},{"id":"406079","messageId":"20200921215409.GA1018675@generichostname","threadId":"54193","inReplyTo":"xmqqwo0met17.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3 09/10] builtin/diff-tree: learn --merge-base","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-09-21T21:54:09Z","receivedAt":"2020-09-21T21:54:14Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Junio,\n\nOn Mon, Sep 21, 2020 at 02:09:24PM -0700, Junio C Hamano wrote:\n> Denton Liu writes:\n> \n> > @@ -165,7 +175,12 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)\n> > \tcase 2:\n> > \t\ttree1 = opt->pending.objects[0].item;\n> > \t\ttree2 = opt->pending.objects[1].item;\n> > -\t\tif (tree2->flags & UNINTERESTING) {\n> > +\t\tif (merge_base) {\n> > +\t\t\tstruct object_id oid;\n> > +\n> > +\t\t\tdiff_get_merge_base(opt, &oid);\n> > +\t\t\ttree1 = lookup_object(the_repository, &oid);\n> > +\t\t} else if (tree2->flags & UNINTERESTING) {\n> > \t\t\tSWAP(tree2, tree1);\n> > \t\t}\n> > \t\tdiff_tree_oid(&tree1->oid, &tree2->oid, \"\", &opt->diffopt);\n> \n> OK. Handling this in that \"case 2\" does make sense.\n> \n> However.\n> \n> The above code as-is will allow something like\n> \n> git diff --merge-base A..B\n> \n> and it will be taken the same as\n> \n> git diff --merge-base A B\n\nThis does not happen because at the top of diff_get_merge_base(), we\nhave\n\n\tfor (i = 0; i < revs->pending.nr; i++) {\n\t\tstruct object *obj = revs->pending.objects[i].item;\n\t\tif (obj->flags)\n\t\t\tdie(_(\"--merge-base does not work with ranges\"));\n\t\tif (obj->type != OBJ_COMMIT)\n\t\t\tdie(_(\"--merge-base only works with commits\"));\n\t}\n\nwhich ensures that we don't accept any ranges at all. This is why I\nconsidered the SWAP and merge_base cases to be mutually exclusive.\n\n> Another possibility is to error out when \"--merge-base A..B\" is\n> given, which might be simpler. Then the code would look more like\n> \n> \n> \ttree1 = ...\n> \ttree2 = ...\n> \n> \tif (merge_base) {\n> \t\tif ((tree1->flags | tree2->flags) & UNINTERESTING)\n> \t\t\tdie(_(\"use of --merge-base with A..B forbidden\"));\n> \t\t... get merge base and assign it to tree1 ...\n> \t} else if (tree2->flags & UNINTERESTING) {\n> \t\tSWAP();\n> \t}\n\nThis is the route I picked, although the logic for this is in\ndiff_get_merge_base().\n\n> While we are at it, what happens when \"--merge-base A...B\" is given?\n> \n> In the original code without \"--merge-base\", \"git diff-tree A...B\"\n> places the merge base between A and B in pending.objects[0] and B in\n> pending.objects[1], I think. \"git diff-tree --merge-base A...B\"\n> would further compute the merge base between these two objects, but\n> luckily $(git merge-base $(merge-base A B) B) is the same as $(git\n> merge-base A B), so you won't get an incorrect answer from such a\n> request. Is this something we want to diagnose as an error? I am\n> inclined to say we should allow it (and if it hurts the user can\n> stop doing so) as there is no harm done.\n\nI think that we should error out for all ranges because this option\nsemantically only really makes sense on two endpoints, not a range of\ncommits. Since the check is cheap to protect users from themselves, we\nmight as well actually do it.\n\nWorst case, if someone has a legimitate use case for --merge-base and\nranges, we can allow it in the future, which would be easier than\nremoving this feature.\n\nThanks,\nDenton\n"},{"id":"406088","messageId":"xmqqblhyepup.fsf@gitster.c.googlers.com","threadId":"54193","inReplyTo":"20200921215409.GA1018675@generichostname","subject":"Re: [PATCH v3 09/10] builtin/diff-tree: learn --merge-base","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-21T22:18:06Z","receivedAt":"2020-09-21T22:18:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu writes:\n\n> This does not happen because at the top of diff_get_merge_base(), we\n> have\n>\n> \tfor (i = 0; i < revs->pending.nr; i++) {\n> \t\tstruct object *obj = revs->pending.objects[i].item;\n> \t\tif (obj->flags)\n> \t\t\tdie(_(\"--merge-base does not work with ranges\"));\n> \t\tif (obj->type != OBJ_COMMIT)\n> \t\t\tdie(_(\"--merge-base only works with commits\"));\n> \t}\n>\n> which ensures that we don't accept any ranges at all.\n\nI think we should lose that loop, or at least the first test.\n\nIf we are not removing the support for \"A..B\" notation and still\naccept \"diff A..B\" happily, not accepting \"diff --merge-base A..B\"\nwould appear inconsistent to the users. \n\nThe same applies to \"A...B\".\n\nThanks.\n\n"},{"id":"406185","messageId":"20200923094716.GA1309694@generichostname","threadId":"54193","inReplyTo":"xmqqblhyepup.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3 09/10] builtin/diff-tree: learn --merge-base","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-09-23T09:47:16Z","receivedAt":"2020-09-23T09:47:20Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Junio,\n\nOn Mon, Sep 21, 2020 at 03:18:06PM -0700, Junio C Hamano wrote:\n> Denton Liu writes:\n> \n> > This does not happen because at the top of diff_get_merge_base(), we\n> > have\n> >\n> > \tfor (i = 0; i < revs->pending.nr; i++) {\n> > \t\tstruct object *obj = revs->pending.objects[i].item;\n> > \t\tif (obj->flags)\n> > \t\t\tdie(_(\"--merge-base does not work with ranges\"));\n> > \t\tif (obj->type != OBJ_COMMIT)\n> > \t\t\tdie(_(\"--merge-base only works with commits\"));\n> > \t}\n> >\n> > which ensures that we don't accept any ranges at all.\n> \n> I think we should lose that loop, or at least the first test.\n> \n> If we are not removing the support for \"A..B\" notation and still\n> accept \"diff A..B\" happily, not accepting \"diff --merge-base A..B\"\n> would appear inconsistent to the users. \n\nI disagree, in the documentation, it clearly states that this option is\nonly available to the diff modes that accept endpoints, not\nranges:\n\n\t'git diff' [] --cached [--merge-base] [] [--] [...]::\n\nand\n\n\t'git diff' [] [--merge-base] [--] [...]::\n\nso it seems perfectly consistent to me. The documentation gives the\nimpression that the range notations are their own separate mode anyway.\n\nAnd worst case scenario, if we receive user reports that they believe\nthe feature is inconsistent, it's 100x easier to change it to allow\nranges than attempting to remove support for ranges in the future.\n\nThanks,\nDenton\n"},{"id":"406397","messageId":"xmqqh7rlk1t8.fsf@gitster.c.googlers.com","threadId":"54193","inReplyTo":"20200923094716.GA1309694@generichostname","subject":"Re: [PATCH v3 09/10] builtin/diff-tree: learn --merge-base","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-25T21:02:11Z","receivedAt":"2020-09-25T21:02:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu writes:\n\n> And worst case scenario, if we receive user reports that they believe\n> the feature is inconsistent, it's 100x easier to change it to allow\n> ranges than attempting to remove support for ranges in the future.\n\nIf we allow ranges from day one, we do not even have to worry about\nit, no?\n"},{"id":"406412","messageId":"20200926015256.GA136085@generichostname","threadId":"54193","inReplyTo":"xmqqh7rlk1t8.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3 09/10] builtin/diff-tree: learn --merge-base","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-09-26T01:52:56Z","receivedAt":"2020-09-26T01:53:01Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Junio,\n\nOn Fri, Sep 25, 2020 at 02:02:11PM -0700, Junio C Hamano wrote:\n> Denton Liu writes:\n> \n> > And worst case scenario, if we receive user reports that they believe\n> > the feature is inconsistent, it's 100x easier to change it to allow\n> > ranges than attempting to remove support for ranges in the future.\n> \n> If we allow ranges from day one, we do not even have to worry about\n> it, no?\n\nYes, but I'm worried that being able to mix --merge-base with ranges\nmight cause more confusion for users since, in my opinion, it only\nreally makes sense for endpoints. That's why I restricted it in the\nfirst place.\n\nI think that since we're in disagreement, it makes more sense to take\nthe safer option where we can implement functionality later whereas if\nwe implement it and we want to remove it later, it'll be a much harder\ntime.\n\nThanks,\nDenton\n"},{"id":"406641","messageId":"CAN0heSqr4_orRR5BD53hoaykd26ZBrfZgQ8PR2WdyZTOwNh6GA@mail.gmail.com","threadId":"54193","inReplyTo":"f54baa4ecd495026c2fce1772b560b7bbbeecc04.1600600823.git.liu.denton@gmail.com","subject":"Re: [PATCH v4 08/10] builtin/diff-index: learn --merge-base","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-09-29T19:53:14Z","receivedAt":"2020-09-29T19:53:29Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Hi Denton,\n\nOn Sun, 20 Sep 2020 at 13:24, Denton Liu wrote:\n> --- a/Documentation/git-diff-index.txt\n> +++ b/Documentation/git-diff-index.txt\n\n> +--merge-base::\n> + Instead of comparing directly, use the merge base\n> + between and HEAD instead. must be a\n> + commit.\n\nIf you end up rerolling this patch series for other reasons, you might\nwant to consider using `backticks` around `` and `HEAD` so\nthey get typeset as monospace.\n\n> If HEAD does not exist (e.g. unborn branches) and\n> is not given, it shows all staged changes.\n> --staged is a synonym of --cached.\n> ++\n> +If --merge-base is given, instead of using , use the merge base\n> +of and HEAD. `git diff --merge-base A` is equivalent to\n> +`git diff $(git merge-base A HEAD)`.\n\nSimilarly here. (I realize there are existing offenders. I think it's\nfine or even preferable to leave them as they are so that you don't get\nlost in a huge while-at-it.) This also applies to the next patch which\ntouches git-diff-tree.txt.\n\n\nMartin\n"}]}