{"thread":{"id":"58436","subject":"[PATCH 0/6] revision: fix order of revs for ^!","startedAt":"2022-09-15T14:49:10Z","lastAt":"2022-10-01T10:28:20Z","messageCount":16,"participants":["René Scharfe","Junio C Hamano","Chris Torek"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"463039","messageId":"ba6eea28-fb3a-b376-2529-351727c02f1a@web.de","threadId":"58436","inReplyTo":null,"subject":"[PATCH 0/6] revision: fix order of revs for ^!","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-09-15T14:48:56Z","receivedAt":"2022-09-15T14:49:10Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"According to gitrevisions(7), \"R^!\" is the same as \"R ^R^1 [^R^2...]\",\nbut handle_revision_arg_1() instead resolves it to \"^R^1 [^R^2...] R\".\nThis ordering affects git diff, which expects the child to be given\nbefore its parents when asked to produce a combined diff of a merge.\n\nThis series reverses that order and makes \"git diff R^!\" consistent\nwith \"git show R^!\". First an unrelated cleanup in the vicinity:\n\n revision: use strtol_i() for exclude_parent\n\nThen dissolve add_parents_only() to gain the necessary flexibility. I\nmay have overdone it:\n\n revision: factor out get_commit()\n revision: factor out add_parent()\n revision: factor out add_parents()\n revision: rename add_parents_only() to add_nth_parent()\n\nFinally the actual change of order:\n\n revision: add parents after child for ^!\n\n revision.c | 87 ++++++++++++++++++++++++++--------------\n t/t4038-diff-combined.sh | 10 +++++\n 2 files changed, 68 insertions(+), 29 deletions(-)\n\n--\n2.37.3\n"},{"id":"463040","messageId":"9baebb3d-8f2a-33eb-3f42-3e1755883d19@web.de","threadId":"58436","inReplyTo":"ba6eea28-fb3a-b376-2529-351727c02f1a@web.de","subject":"[PATCH 1/6] revision: use strtol_i() for exclude_parent","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-09-15T14:51:02Z","receivedAt":"2022-09-15T14:51:14Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Avoid silent overflow of the int exclude_parent by using the appropriate\nfunction, strtol_i(), to parse its value.\n\nSigned-off-by: René Scharfe \n---\n revision.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex d5f4463cb6..0b8d48f94c 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2119,9 +2119,8 @@ static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int fl\n \t\tint exclude_parent = 1;\n\n \t\tif (mark[2]) {\n-\t\t\tchar *end;\n-\t\t\texclude_parent = strtoul(mark + 2, &end, 10);\n-\t\t\tif (*end != '\\0' || !exclude_parent)\n+\t\t\tif (strtol_i(mark + 2, 10, &exclude_parent) ||\n+\t\t\t exclude_parent < 1)\n \t\t\t\treturn -1;\n \t\t}\n\n--\n2.37.3\n"},{"id":"463041","messageId":"90bdf408-9bad-6816-06c1-66ac64ea9c3b@web.de","threadId":"58436","inReplyTo":"ba6eea28-fb3a-b376-2529-351727c02f1a@web.de","subject":"[PATCH 2/6] revision: factor out get_commit()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-09-15T14:52:04Z","receivedAt":"2022-09-15T14:52:24Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Split out the first half of add_parents_only() to obtain a function that\nfinds and returns the commit object. It allows checking the validity of\nthe child separately from adding its parents.\n\nSigned-off-by: René Scharfe \n---\n revision.c | 35 +++++++++++++++++++++++------------\n 1 file changed, 23 insertions(+), 12 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 0b8d48f94c..4f896b4992 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1822,35 +1822,46 @@ static void add_alternate_refs_to_pending(struct rev_info *revs,\n \tfor_each_alternate_ref(add_one_alternate_ref, &data);\n }\n\n-static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,\n-\t\t\t int exclude_parent)\n+static struct commit *get_commit(struct rev_info *revs, const char *arg_)\n {\n \tstruct object_id oid;\n \tstruct object *it;\n-\tstruct commit *commit;\n-\tstruct commit_list *parents;\n-\tint parent_number;\n \tconst char *arg = arg_;\n\n-\tif (*arg == '^') {\n-\t\tflags ^= UNINTERESTING | BOTTOM;\n+\tif (*arg == '^')\n \t\targ++;\n-\t}\n \tif (get_oid_committish(arg, &oid))\n-\t\treturn 0;\n+\t\treturn NULL;\n \twhile (1) {\n \t\tit = get_reference(revs, arg, &oid, 0);\n \t\tif (!it && revs->ignore_missing)\n-\t\t\treturn 0;\n+\t\t\treturn NULL;\n \t\tif (it->type != OBJ_TAG)\n \t\t\tbreak;\n \t\tif (!((struct tag*)it)->tagged)\n-\t\t\treturn 0;\n+\t\t\treturn NULL;\n \t\toidcpy(&oid, &((struct tag*)it)->tagged->oid);\n \t}\n \tif (it->type != OBJ_COMMIT)\n+\t\treturn NULL;\n+\treturn (struct commit *)it;\n+}\n+\n+static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,\n+\t\t\t int exclude_parent)\n+{\n+\tstruct object *it;\n+\tstruct commit *commit = get_commit(revs, arg_);\n+\tstruct commit_list *parents;\n+\tint parent_number;\n+\tconst char *arg = arg_;\n+\n+\tif (*arg == '^') {\n+\t\tflags ^= UNINTERESTING | BOTTOM;\n+\t\targ++;\n+\t}\n+\tif (!commit)\n \t\treturn 0;\n-\tcommit = (struct commit *)it;\n \tif (exclude_parent &&\n \t exclude_parent > commit_list_count(commit->parents))\n \t\treturn 0;\n--\n2.37.3\n"},{"id":"463042","messageId":"b115f148-937b-5957-bfb7-930c02f446fb@web.de","threadId":"58436","inReplyTo":"ba6eea28-fb3a-b376-2529-351727c02f1a@web.de","subject":"[PATCH 3/6] revision: factor out add_parent()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-09-15T14:52:54Z","receivedAt":"2022-09-15T14:53:09Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Split out the addition of a single parent into a helper. We'll use it\nin the next commit.\n\nSigned-off-by: René Scharfe \n---\n revision.c | 25 +++++++++++++++----------\n 1 file changed, 15 insertions(+), 10 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 4f896b4992..14cb73e508 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1847,19 +1847,27 @@ static struct commit *get_commit(struct rev_info *revs, const char *arg_)\n \treturn (struct commit *)it;\n }\n\n-static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,\n-\t\t\t int exclude_parent)\n+static void add_parent(struct rev_info *revs, struct object *parent,\n+\t\t const char *arg_, int flags)\n {\n-\tstruct object *it;\n-\tstruct commit *commit = get_commit(revs, arg_);\n-\tstruct commit_list *parents;\n-\tint parent_number;\n \tconst char *arg = arg_;\n\n \tif (*arg == '^') {\n \t\tflags ^= UNINTERESTING | BOTTOM;\n \t\targ++;\n \t}\n+\tparent->flags |= flags;\n+\tadd_rev_cmdline(revs, parent, arg_, REV_CMD_PARENTS_ONLY, flags);\n+\tadd_pending_object(revs, parent, arg);\n+}\n+\n+static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,\n+\t\t\t int exclude_parent)\n+{\n+\tstruct commit *commit = get_commit(revs, arg_);\n+\tstruct commit_list *parents;\n+\tint parent_number;\n+\n \tif (!commit)\n \t\treturn 0;\n \tif (exclude_parent &&\n@@ -1871,10 +1879,7 @@ static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,\n \t\tif (exclude_parent && parent_number != exclude_parent)\n \t\t\tcontinue;\n\n-\t\tit = &parents->item->object;\n-\t\tit->flags |= flags;\n-\t\tadd_rev_cmdline(revs, it, arg_, REV_CMD_PARENTS_ONLY, flags);\n-\t\tadd_pending_object(revs, it, arg);\n+\t\tadd_parent(revs, &parents->item->object, arg_, flags);\n \t}\n \treturn 1;\n }\n--\n2.37.3\n"},{"id":"463043","messageId":"23a6914b-47e5-d3a9-ca95-01035b691bca@web.de","threadId":"58436","inReplyTo":"ba6eea28-fb3a-b376-2529-351727c02f1a@web.de","subject":"[PATCH 4/6] revision: factor out add_parents()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-09-15T14:53:40Z","receivedAt":"2022-09-15T14:53:49Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Add a function that registers all parents. Use it in add_parents_only()\nto handle the case of exclude_parent being unset early.\n\nWith that out of the way we can specialize the remaining loop can to\nregister only the specified parent. And since we exit reporting success\nonce we got it we no longer need to check the total number of parents\nbeforehand.\n\nSigned-off-by: René Scharfe \n---\n revision.c | 24 ++++++++++++++++--------\n 1 file changed, 16 insertions(+), 8 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 14cb73e508..284393a146 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1861,6 +1861,13 @@ static void add_parent(struct rev_info *revs, struct object *parent,\n \tadd_pending_object(revs, parent, arg);\n }\n\n+static void add_parents(struct rev_info *revs, struct commit_list *parents,\n+\t\t\tconst char *arg, int flags)\n+{\n+\tfor (; parents; parents = parents->next)\n+\t\tadd_parent(revs, &parents->item->object, arg, flags);\n+}\n+\n static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,\n \t\t\t int exclude_parent)\n {\n@@ -1870,18 +1877,19 @@ static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,\n\n \tif (!commit)\n \t\treturn 0;\n-\tif (exclude_parent &&\n-\t exclude_parent > commit_list_count(commit->parents))\n-\t\treturn 0;\n+\tif (!exclude_parent) {\n+\t\tadd_parents(revs, commit->parents, arg_, flags);\n+\t\treturn 1;\n+\t}\n \tfor (parents = commit->parents, parent_number = 1;\n \t parents;\n \t parents = parents->next, parent_number++) {\n-\t\tif (exclude_parent && parent_number != exclude_parent)\n-\t\t\tcontinue;\n-\n-\t\tadd_parent(revs, &parents->item->object, arg_, flags);\n+\t\tif (parent_number == exclude_parent) {\n+\t\t\tadd_parent(revs, &parents->item->object, arg_, flags);\n+\t\t\treturn 1;\n+\t\t}\n \t}\n-\treturn 1;\n+\treturn 0;\n }\n\n void repo_init_revisions(struct repository *r,\n--\n2.37.3\n"},{"id":"463044","messageId":"73e1e0e2-d6a4-3d1b-4303-f9e8984eaa79@web.de","threadId":"58436","inReplyTo":"ba6eea28-fb3a-b376-2529-351727c02f1a@web.de","subject":"[PATCH 5/6] revision: rename add_parents_only() to add_nth_parent()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-09-15T14:54:32Z","receivedAt":"2022-09-15T14:54:43Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Move the handling of !exclude_parent to the two callers that pass zero.\nThis allows checking the validity of the child separately from adding\nits parents, which we'll make use of in the next patch.\n\nRename the function to reflect its changed purpose, now that it\nrequires exclude_parent to be given and only adds at most one parent.\n\nSigned-off-by: René Scharfe \n---\nCreated with --inter-hunk-context=1 for easier review.\n\n revision.c | 25 ++++++++++++++++---------\n 1 file changed, 16 insertions(+), 9 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 284393a146..5e756b76aa 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1868,19 +1868,15 @@ static void add_parents(struct rev_info *revs, struct commit_list *parents,\n \t\tadd_parent(revs, &parents->item->object, arg, flags);\n }\n\n-static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,\n-\t\t\t int exclude_parent)\n+static int add_nth_parent(struct rev_info *revs, const char *arg_, int flags,\n+\t\t\t int exclude_parent)\n {\n \tstruct commit *commit = get_commit(revs, arg_);\n \tstruct commit_list *parents;\n \tint parent_number;\n\n \tif (!commit)\n \t\treturn 0;\n-\tif (!exclude_parent) {\n-\t\tadd_parents(revs, commit->parents, arg_, flags);\n-\t\treturn 1;\n-\t}\n \tfor (parents = commit->parents, parent_number = 1;\n \t parents;\n \t parents = parents->next, parent_number++) {\n@@ -2127,15 +2123,26 @@ static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int fl\n\n \tmark = strstr(arg, \"^@\");\n \tif (mark && !mark[2]) {\n+\t\tstruct commit *commit;\n+\n \t\t*mark = 0;\n-\t\tif (add_parents_only(revs, arg, flags, 0))\n+\t\tcommit = get_commit(revs, arg);\n+\t\tif (commit) {\n+\t\t\tadd_parents(revs, commit->parents, arg, flags);\n \t\t\treturn 0;\n+\t\t}\n \t\t*mark = '^';\n \t}\n \tmark = strstr(arg, \"^!\");\n \tif (mark && !mark[2]) {\n+\t\tstruct commit *commit;\n+\n \t\t*mark = 0;\n-\t\tif (!add_parents_only(revs, arg, flags ^ (UNINTERESTING | BOTTOM), 0))\n+\t\tcommit = get_commit(revs, arg);\n+\t\tif (commit)\n+\t\t\tadd_parents(revs, commit->parents, arg,\n+\t\t\t\t flags ^ (UNINTERESTING | BOTTOM));\n+\t\telse\n \t\t\t*mark = '^';\n \t}\n \tmark = strstr(arg, \"^-\");\n@@ -2149,7 +2156,7 @@ static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int fl\n \t\t}\n\n \t\t*mark = 0;\n-\t\tif (!add_parents_only(revs, arg, flags ^ (UNINTERESTING | BOTTOM), exclude_parent))\n+\t\tif (!add_nth_parent(revs, arg, flags ^ (UNINTERESTING | BOTTOM), exclude_parent))\n \t\t\t*mark = '^';\n \t}\n\n--\n2.37.3\n"},{"id":"463045","messageId":"84018532-169f-dc9b-f894-3d19bb7b4245@web.de","threadId":"58436","inReplyTo":"ba6eea28-fb3a-b376-2529-351727c02f1a@web.de","subject":"[PATCH 6/6] revision: add parents after child for ^!","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-09-15T14:55:12Z","receivedAt":"2022-09-15T14:55:26Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"gitrevisions(7) says: \"A suffix '^' followed by an exclamation mark is\nthe same as giving commit '' and then all its parents prefixed with\n'^' to exclude them (and their ancestors).\"\n\nhandle_revision_arg_1() however adds the negated parents first. This\nleads to unexpected results with git diff and merge commits, as that\ncommand expects the documented order.\n\nSplit up the handling of ^! by moving the actual addition of the parents\nafter the addition of the child.\n\nReported-by: Tim Jaacks \nSigned-off-by: René Scharfe \n---\n revision.c | 13 ++++++-------\n t/t4038-diff-combined.sh | 10 ++++++++++\n 2 files changed, 16 insertions(+), 7 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 5e756b76aa..8385127f1a 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2107,6 +2107,7 @@ static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int fl\n \tconst char *arg = arg_;\n \tint cant_be_filename = revarg_opt & REVARG_CANNOT_BE_FILENAME;\n \tunsigned get_sha1_flags = GET_OID_RECORD_PATH;\n+\tstruct commit *caret_bang_commit = NULL;\n\n \tflags = flags & UNINTERESTING ? flags | BOTTOM : flags & ~BOTTOM;\n\n@@ -2135,14 +2136,9 @@ static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int fl\n \t}\n \tmark = strstr(arg, \"^!\");\n \tif (mark && !mark[2]) {\n-\t\tstruct commit *commit;\n-\n \t\t*mark = 0;\n-\t\tcommit = get_commit(revs, arg);\n-\t\tif (commit)\n-\t\t\tadd_parents(revs, commit->parents, arg,\n-\t\t\t\t flags ^ (UNINTERESTING | BOTTOM));\n-\t\telse\n+\t\tcaret_bang_commit = get_commit(revs, arg);\n+\t\tif (!caret_bang_commit)\n \t\t\t*mark = '^';\n \t}\n \tmark = strstr(arg, \"^-\");\n@@ -2179,6 +2175,9 @@ static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int fl\n \tadd_rev_cmdline(revs, object, arg_, REV_CMD_REV, flags ^ local_flags);\n \tadd_pending_object_with_path(revs, object, arg, oc.mode, oc.path);\n \tfree(oc.path);\n+\tif (caret_bang_commit)\n+\t\tadd_parents(revs, caret_bang_commit->parents, arg,\n+\t\t\t flags ^ (UNINTERESTING | BOTTOM));\n \treturn 0;\n }\n\ndiff --git a/t/t4038-diff-combined.sh b/t/t4038-diff-combined.sh\nindex 9a292bac70..2ce26e585c 100755\n--- a/t/t4038-diff-combined.sh\n+++ b/t/t4038-diff-combined.sh\n@@ -80,11 +80,21 @@ test_expect_success 'check combined output (1)' '\n \tverify_helper sidewithone\n '\n\n+test_expect_success 'check combined output (1) with git diff ^!' '\n+\tgit diff sidewithone^! -- >sidewithone &&\n+\tverify_helper sidewithone\n+'\n+\n test_expect_success 'check combined output (2)' '\n \tgit show sidesansone -- >sidesansone &&\n \tverify_helper sidesansone\n '\n\n+test_expect_success 'check combined output (2) with git diff ^!' '\n+\tgit diff sidesansone^! -- >sidesansone &&\n+\tverify_helper sidesansone\n+'\n+\n test_expect_success 'diagnose truncated file' '\n \t>file &&\n \tgit add file &&\n--\n2.37.3\n"},{"id":"463055","messageId":"xmqq5yhoo76m.fsf@gitster.g","threadId":"58436","inReplyTo":"84018532-169f-dc9b-f894-3d19bb7b4245@web.de","subject":"Re: [PATCH 6/6] revision: add parents after child for ^!","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-15T17:53:37Z","receivedAt":"2022-09-15T17:53:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> gitrevisions(7) says: \"A suffix '^' followed by an exclamation mark is\n> the same as giving commit '' and then all its parents prefixed with\n> '^' to exclude them (and their ancestors).\"\n\nI did not mean to specify the order at all in that description when\nI wrote ca5ee2d1 (Enumerate revision range specifiers in the\ndocumentation, 2012-07-24) and I do not think it should be read as\nsuch.\n\n> handle_revision_arg_1() however adds the negated parents first.\n\nI suspect that this was deliberately done so to match how A..B is\nadded to the pending commit list in revisions.c::handle_dotdot_1()\nto tolerate \"git diff A..B\" as a synonym to \"git diff A B\", which\ndates back to cd2bdc53 (Common option parsing for \"git log --diff\"\nand friends, 2006-04-14).\n\n> Split up the handling of ^! by moving the actual addition of the\n> parents after the addition of the child.\n\nI do not offhand think of anything other than the \"diff\" frontend\nthat cares about the order of these commits from the command line, I\nam afraid that this might end up robbing Peter to pay paul. \n\nCan't we \"fix\" it at the consumer end, perhaps by checking where\nthese commits came from by looking at rev.cmdline?\n"},{"id":"463072","messageId":"29d50baa-1923-38e1-6ecb-73840376d28e@web.de","threadId":"58436","inReplyTo":"xmqq5yhoo76m.fsf@gitster.g","subject":"Re: [PATCH 6/6] revision: add parents after child for ^!","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-09-16T09:02:01Z","receivedAt":"2022-09-16T09:02:46Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 15.09.22 um 19:53 schrieb Junio C Hamano:\n> René Scharfe writes:\n>\n>> gitrevisions(7) says: \"A suffix '^' followed by an exclamation mark is\n>> the same as giving commit '' and then all its parents prefixed with\n>> '^' to exclude them (and their ancestors).\"\n>\n> I did not mean to specify the order at all in that description when\n> I wrote ca5ee2d1 (Enumerate revision range specifiers in the\n> documentation, 2012-07-24) and I do not think it should be read as\n> such.\n\nThen the \"then\" needs to go.\n\ngit rev-parse has its own parser for ^! etc. and puts parents last,\nconsistent with the \"then\".\n\n>> handle_revision_arg_1() however adds the negated parents first.\n>\n> I suspect that this was deliberately done so to match how A..B is\n> added to the pending commit list in revisions.c::handle_dotdot_1()\n> to tolerate \"git diff A..B\" as a synonym to \"git diff A B\", which\n> dates back to cd2bdc53 (Common option parsing for \"git log --diff\"\n> and friends, 2006-04-14).\n\n\"git diff A B\", \"git diff A..B\", \"git diff ^A B\" and \"git diff \"B ^A\"\nall produce the same output. So I suspect we could reverse the order\nfor A..B as well without ill effect if we wanted.\n\n>> Split up the handling of ^! by moving the actual addition of the\n>> parents after the addition of the child.\n>\n> I do not offhand think of anything other than the \"diff\" frontend\n> that cares about the order of these commits from the command line, I\n> am afraid that this might end up robbing Peter to pay paul.\n\nCan't think of anything, either, but it may well be possible that yet\nanother untested use case could depend on getting parents first.\n\"git diff R^!\" was untested as well and it took two years from release\nto Tim's bug report after all. And I'm not particularly proud of my\nrefactoring patches, so wouldn't mind dropping them.\n\n> Can't we \"fix\" it at the consumer end, perhaps by checking where\n> these commits came from by looking at rev.cmdline?\n\nWe could.\n\n--- >8 ---\nSubject: [PATCH] diff: support ^! for merges\n\nrevision.c::handle_revision_arg_1() resolves ^! by first adding the\nnegated parents and then itself. builtin_diff_combined() expects\nthe first tree to be the merge and the remaining ones to be the parents,\nthough. This mismatch results in bogus diff output.\n\nRemember the first tree that doesn't belong to a parent and use it\ninstead of blindly picking the first one. This makes \"git diff ^!\"\nconsistent with \"git show ^!\".\n\nReported-by: Tim Jaacks \nSuggested-by: Junio C Hamano \nSigned-off-by: René Scharfe \n---\nThe check \"i < rev.cmdline.nr\" is necessary to avoid segfaults e.g.\nin t0005. I wonder why. Shouldn't rev.pending.objects and\nrev.cmdline.rev always have the same number of entries?\n\n builtin/diff.c | 23 ++++++++++++++++++-----\n t/t4038-diff-combined.sh | 10 ++++++++++\n 2 files changed, 28 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 54bb3de964..04c6ba0862 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -209,7 +209,7 @@ static int builtin_diff_tree(struct rev_info *revs,\n static int builtin_diff_combined(struct rev_info *revs,\n \t\t\t\t int argc, const char **argv,\n \t\t\t\t struct object_array_entry *ent,\n-\t\t\t\t int ents)\n+\t\t\t\t int ents, int first_non_parent)\n {\n \tstruct oid_array parents = OID_ARRAY_INIT;\n \tint i;\n@@ -217,11 +217,18 @@ static int builtin_diff_combined(struct rev_info *revs,\n \tif (argc > 1)\n \t\tusage(builtin_diff_usage);\n\n+\tif (first_non_parent < 0)\n+\t\tdie(_(\"no merge given, only parents.\"));\n+\tif (first_non_parent >= ents)\n+\t\tBUG(\"first_non_parent out of range: %d\", first_non_parent);\n+\n \tdiff_merges_set_dense_combined_if_unset(revs);\n\n-\tfor (i = 1; i < ents; i++)\n-\t\toid_array_append(&parents, &ent[i].item->oid);\n-\tdiff_tree_combined(&ent[0].item->oid, &parents, revs);\n+\tfor (i = 0; i < ents; i++) {\n+\t\tif (i != first_non_parent)\n+\t\t\toid_array_append(&parents, &ent[i].item->oid);\n+\t}\n+\tdiff_tree_combined(&ent[first_non_parent].item->oid, &parents, revs);\n \toid_array_clear(&parents);\n \treturn 0;\n }\n@@ -385,6 +392,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \tint i;\n \tstruct rev_info rev;\n \tstruct object_array ent = OBJECT_ARRAY_INIT;\n+\tint first_non_parent = -1;\n \tint blobs = 0, paths = 0;\n \tstruct object_array_entry *blob[2];\n \tint nongit = 0, no_index = 0;\n@@ -541,6 +549,10 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\tif (obj->type == OBJ_TREE) {\n \t\t\tif (sdiff.skip && bitmap_get(sdiff.skip, i))\n \t\t\t\tcontinue;\n+\t\t\tif (first_non_parent < 0 &&\n+\t\t\t i < rev.cmdline.nr &&\n+\t\t\t rev.cmdline.rev[i].whence != REV_CMD_PARENTS_ONLY)\n+\t\t\t\tfirst_non_parent = ent.nr;\n \t\t\tobj->flags |= flags;\n \t\t\tadd_object_array(obj, name, &ent);\n \t\t} else if (obj->type == OBJ_BLOB) {\n@@ -590,7 +602,8 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\t\t\t\t &ent.objects[0], &ent.objects[1]);\n \t} else\n \t\tresult = builtin_diff_combined(&rev, argc, argv,\n-\t\t\t\t\t ent.objects, ent.nr);\n+\t\t\t\t\t ent.objects, ent.nr,\n+\t\t\t\t\t first_non_parent);\n \tresult = diff_result_code(&rev.diffopt, result);\n \tif (1 < rev.diffopt.skip_stat_unmatch)\n \t\trefresh_index_quietly();\ndiff --git a/t/t4038-diff-combined.sh b/t/t4038-diff-combined.sh\nindex 9a292bac70..2ce26e585c 100755\n--- a/t/t4038-diff-combined.sh\n+++ b/t/t4038-diff-combined.sh\n@@ -80,11 +80,21 @@ test_expect_success 'check combined output (1)' '\n \tverify_helper sidewithone\n '\n\n+test_expect_success 'check combined output (1) with git diff ^!' '\n+\tgit diff sidewithone^! -- >sidewithone &&\n+\tverify_helper sidewithone\n+'\n+\n test_expect_success 'check combined output (2)' '\n \tgit show sidesansone -- >sidesansone &&\n \tverify_helper sidesansone\n '\n\n+test_expect_success 'check combined output (2) with git diff ^!' '\n+\tgit diff sidesansone^! -- >sidesansone &&\n+\tverify_helper sidesansone\n+'\n+\n test_expect_success 'diagnose truncated file' '\n \t>file &&\n \tgit add file &&\n--\n2.37.3\n"},{"id":"463084","messageId":"xmqqh717jou6.fsf@gitster.g","threadId":"58436","inReplyTo":"29d50baa-1923-38e1-6ecb-73840376d28e@web.de","subject":"Re: [PATCH 6/6] revision: add parents after child for ^!","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-16T15:55:45Z","receivedAt":"2022-09-16T15:55:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> \"git diff A B\", \"git diff A..B\", \"git diff ^A B\" and \"git diff \"B ^A\"\n> all produce the same output. So I suspect we could reverse the order\n> for A..B as well without ill effect if we wanted.\n\nYup.\n\n>> Can't we \"fix\" it at the consumer end, perhaps by checking where\n>> these commits came from by looking at rev.cmdline?\n>\n> We could.\n\nYeah, doing so may be more in line with how two-tree comparison is\nparsed (your observation above on four combinations).\n\n> --- >8 ---\n> Subject: [PATCH] diff: support ^! for merges\n>\n> revision.c::handle_revision_arg_1() resolves ^! by first adding the\n> negated parents and then itself. builtin_diff_combined() expects\n> the first tree to be the merge and the remaining ones to be the parents,\n> though. This mismatch results in bogus diff output.\n>\n> Remember the first tree that doesn't belong to a parent and use it\n> instead of blindly picking the first one. This makes \"git diff ^!\"\n> consistent with \"git show ^!\".\n>\n> Reported-by: Tim Jaacks \n> Suggested-by: Junio C Hamano \n> Signed-off-by: René Scharfe \n> ---\n> The check \"i < rev.cmdline.nr\" is necessary to avoid segfaults e.g.\n> in t0005. I wonder why. Shouldn't rev.pending.objects and\n> rev.cmdline.rev always have the same number of entries?\n\nThings that did not come from command line can go into pending,\ncan't they? E.g. add_head_to_pending(), when lack of rev defaults\nto \"HEAD\", touches pending without touching cmdline?\n"},{"id":"463124","messageId":"CAPx1GvcdQFBMyyD2gX7rYSJk58eEm5rREhoTz4XwVac-Y=nK1Q@mail.gmail.com","threadId":"58436","inReplyTo":"9baebb3d-8f2a-33eb-3f42-3e1755883d19@web.de","subject":"Re: [PATCH 1/6] revision: use strtol_i() for exclude_parent","fromName":"Chris Torek","fromEmail":"chris.torek@gmail.com","sentAt":"2022-09-17T00:59:44Z","receivedAt":"2022-09-17T01:00:02Z","isPatch":true,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"On Thu, Sep 15, 2022 at 7:51 AM René Scharfe wrote:\n> Avoid silent overflow of the int exclude_parent by using the appropriate\n> function, strtol_i(), to parse its value.\n>\n> Signed-off-by: René Scharfe \n\nNice (albeit small) improvement regardless of anything else,\n\nChris\n"},{"id":"463150","messageId":"fd323d96-28fc-a362-2f23-d99ccc5ec76c@web.de","threadId":"58436","inReplyTo":"xmqqh717jou6.fsf@gitster.g","subject":"Re: [PATCH 6/6] revision: add parents after child for ^!","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-09-18T05:36:59Z","receivedAt":"2022-09-18T05:37:18Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 16.09.22 um 17:55 schrieb Junio C Hamano:\n> René Scharfe writes:\n>\n>> The check \"i < rev.cmdline.nr\" is necessary to avoid segfaults e.g.\n>> in t0005. I wonder why. Shouldn't rev.pending.objects and\n>> rev.cmdline.rev always have the same number of entries?\n>\n> Things that did not come from command line can go into pending,\n> can't they? E.g. add_head_to_pending(), when lack of rev defaults\n> to \"HEAD\", touches pending without touching cmdline?\n\nIndeed. In t0005 it's even only a fake HEAD with an empty_tree added by\ncmd_diff() a few lines up, for a diff of a new file in a fresh, empty\nrepository.\n\nRené\n"},{"id":"464035","messageId":"16c49d20-cafc-4b48-3c6b-e11c74c29abb@web.de","threadId":"58436","inReplyTo":"ba6eea28-fb3a-b376-2529-351727c02f1a@web.de","subject":"[PATCH v3 0/3] diff: support ^! for merges","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-10-01T10:23:42Z","receivedAt":"2022-10-01T10:23:55Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Changes between v1 [1] and v2 [2]:\n- Single patch, local fix, leave revision.c alone.\n\nChanges since v2 [2]:\n- Patch 1 (unrelated cleanup) is back, requested by Chris.\n- New patch 2 to remove a broken promise from documentation.\n- Move the first_non_parent tracking code to after add_object_array()\n and add a comment, both to (hopefully) improve readability.\n- Mention ^! in the documentation of git diff.\n\n revision: use strtol_i() for exclude_parent\n revisions.txt: unspecify order of resolved parts of ^!\n diff: support ^! for merges\n\n[1] https://lore.kernel.org/git/ba6eea28-fb3a-b376-2529-351727c02f1a@web.de/\n[2] https://lore.kernel.org/git/29d50baa-1923-38e1-6ecb-73840376d28e@web.de/\n\n Documentation/git-diff.txt | 8 ++++----\n Documentation/revisions.txt | 2 +-\n builtin/diff.c | 23 ++++++++++++++++++-----\n revision.c | 5 ++---\n t/t4038-diff-combined.sh | 10 ++++++++++\n 5 files changed, 35 insertions(+), 13 deletions(-)\n\n--\n2.37.3\n"},{"id":"464036","messageId":"4fc4cf3b-a658-3959-4541-b889534fbbb0@web.de","threadId":"58436","inReplyTo":"16c49d20-cafc-4b48-3c6b-e11c74c29abb@web.de","subject":"[PATCH v3 1/3] revision: use strtol_i() for exclude_parent","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-10-01T10:25:36Z","receivedAt":"2022-10-01T10:25:52Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Avoid silent overflow of the int exclude_parent by using the appropriate\nfunction, strtol_i(), to parse its value.\n\nSigned-off-by: René Scharfe \n---\n revision.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 36e31942ce..79b88604c7 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2120,9 +2120,8 @@ static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int fl\n \t\tint exclude_parent = 1;\n\n \t\tif (mark[2]) {\n-\t\t\tchar *end;\n-\t\t\texclude_parent = strtoul(mark + 2, &end, 10);\n-\t\t\tif (*end != '\\0' || !exclude_parent)\n+\t\t\tif (strtol_i(mark + 2, 10, &exclude_parent) ||\n+\t\t\t exclude_parent < 1)\n \t\t\t\treturn -1;\n \t\t}\n\n--\n2.37.3\n"},{"id":"464037","messageId":"970fd3b1-8792-cc59-c3ee-00656a0615d0@web.de","threadId":"58436","inReplyTo":"16c49d20-cafc-4b48-3c6b-e11c74c29abb@web.de","subject":"[PATCH v3 2/3] revisions.txt: unspecify order of resolved parts of ^!","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-10-01T10:26:34Z","receivedAt":"2022-10-01T10:26:45Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"gitrevisions(7) says that ^! resolves to and then all the\nparents of . revision.c::handle_revision_arg_1() actually adds\nall parents first, then . Change the documentation to leave the\norder unspecified, to avoid misleading readers.\n\nSigned-off-by: René Scharfe \n---\n Documentation/revisions.txt | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/Documentation/revisions.txt b/Documentation/revisions.txt\nindex e3e350126d..0d2e55d781 100644\n--- a/Documentation/revisions.txt\n+++ b/Documentation/revisions.txt\n@@ -363,7 +363,7 @@ Revision Range Summary\n\n '{caret}!', e.g. 'HEAD{caret}!'::\n A suffix '{caret}' followed by an exclamation mark is the same\n- as giving commit '' and then all its parents prefixed with\n+ as giving commit '' and all its parents prefixed with\n '{caret}' to exclude them (and their ancestors).\n\n '{caret}-', e.g. 'HEAD{caret}-, HEAD{caret}-2'::\n--\n2.37.3\n"},{"id":"464038","messageId":"0cf042fa-aa6f-4e3e-5982-3354c1cab677@web.de","threadId":"58436","inReplyTo":"16c49d20-cafc-4b48-3c6b-e11c74c29abb@web.de","subject":"[PATCH v3 3/3] diff: support ^! for merges","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-10-01T10:28:07Z","receivedAt":"2022-10-01T10:28:20Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"revision.c::handle_revision_arg_1() resolves ^! by first adding the\nnegated parents and then itself. builtin_diff_combined() expects\nthe first tree to be the merge and the remaining ones to be the parents,\nthough. This mismatch results in bogus diff output.\n\nRemember the first tree that doesn't belong to a parent and use it\ninstead of blindly picking the first one. This makes \"git diff ^!\"\nconsistent with \"git show ^!\".\n\nReported-by: Tim Jaacks \nSuggested-by: Junio C Hamano \nSigned-off-by: René Scharfe \n---\n Documentation/git-diff.txt | 8 ++++----\n builtin/diff.c | 23 ++++++++++++++++++-----\n t/t4038-diff-combined.sh | 10 ++++++++++\n 3 files changed, 32 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt\nindex 85ae6d6d08..52b679256c 100644\n--- a/Documentation/git-diff.txt\n+++ b/Documentation/git-diff.txt\n@@ -79,10 +79,10 @@ If --merge-base is given, use the merge base of the two commits for the\n\n \tThis form is to view the results of a merge commit. The first\n \tlisted must be the merge itself; the remaining two or\n-\tmore commits should be its parents. A convenient way to produce\n-\tthe desired set of revisions is to use the `^@` 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+\tmore commits should be its parents. Convenient ways to produce\n+\tthe desired set of revisions are to use the suffixes `^@` and\n+\t`^!`. If A is a merge commit, then `git diff A A^@`,\n+\t`git diff A^!` and `git show A` all give the same combined diff.\n\n 'git diff' [] .. [--] [...]::\n\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 54bb3de964..0e49919735 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -209,7 +209,7 @@ static int builtin_diff_tree(struct rev_info *revs,\n static int builtin_diff_combined(struct rev_info *revs,\n \t\t\t\t int argc, const char **argv,\n \t\t\t\t struct object_array_entry *ent,\n-\t\t\t\t int ents)\n+\t\t\t\t int ents, int first_non_parent)\n {\n \tstruct oid_array parents = OID_ARRAY_INIT;\n \tint i;\n@@ -217,11 +217,18 @@ static int builtin_diff_combined(struct rev_info *revs,\n \tif (argc > 1)\n \t\tusage(builtin_diff_usage);\n\n+\tif (first_non_parent < 0)\n+\t\tdie(_(\"no merge given, only parents.\"));\n+\tif (first_non_parent >= ents)\n+\t\tBUG(\"first_non_parent out of range: %d\", first_non_parent);\n+\n \tdiff_merges_set_dense_combined_if_unset(revs);\n\n-\tfor (i = 1; i < ents; i++)\n-\t\toid_array_append(&parents, &ent[i].item->oid);\n-\tdiff_tree_combined(&ent[0].item->oid, &parents, revs);\n+\tfor (i = 0; i < ents; i++) {\n+\t\tif (i != first_non_parent)\n+\t\t\toid_array_append(&parents, &ent[i].item->oid);\n+\t}\n+\tdiff_tree_combined(&ent[first_non_parent].item->oid, &parents, revs);\n \toid_array_clear(&parents);\n \treturn 0;\n }\n@@ -385,6 +392,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \tint i;\n \tstruct rev_info rev;\n \tstruct object_array ent = OBJECT_ARRAY_INIT;\n+\tint first_non_parent = -1;\n \tint blobs = 0, paths = 0;\n \tstruct object_array_entry *blob[2];\n \tint nongit = 0, no_index = 0;\n@@ -543,6 +551,10 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\t\t\tcontinue;\n \t\t\tobj->flags |= flags;\n \t\t\tadd_object_array(obj, name, &ent);\n+\t\t\tif (first_non_parent < 0 &&\n+\t\t\t (i >= rev.cmdline.nr || /* HEAD by hand. */\n+\t\t\t rev.cmdline.rev[i].whence != REV_CMD_PARENTS_ONLY))\n+\t\t\t\tfirst_non_parent = ent.nr - 1;\n \t\t} else if (obj->type == OBJ_BLOB) {\n \t\t\tif (2 <= blobs)\n \t\t\t\tdie(_(\"more than two blobs given: '%s'\"), name);\n@@ -590,7 +602,8 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\t\t\t\t &ent.objects[0], &ent.objects[1]);\n \t} else\n \t\tresult = builtin_diff_combined(&rev, argc, argv,\n-\t\t\t\t\t ent.objects, ent.nr);\n+\t\t\t\t\t ent.objects, ent.nr,\n+\t\t\t\t\t first_non_parent);\n \tresult = diff_result_code(&rev.diffopt, result);\n \tif (1 < rev.diffopt.skip_stat_unmatch)\n \t\trefresh_index_quietly();\ndiff --git a/t/t4038-diff-combined.sh b/t/t4038-diff-combined.sh\nindex 9a292bac70..2ce26e585c 100755\n--- a/t/t4038-diff-combined.sh\n+++ b/t/t4038-diff-combined.sh\n@@ -80,11 +80,21 @@ test_expect_success 'check combined output (1)' '\n \tverify_helper sidewithone\n '\n\n+test_expect_success 'check combined output (1) with git diff ^!' '\n+\tgit diff sidewithone^! -- >sidewithone &&\n+\tverify_helper sidewithone\n+'\n+\n test_expect_success 'check combined output (2)' '\n \tgit show sidesansone -- >sidesansone &&\n \tverify_helper sidesansone\n '\n\n+test_expect_success 'check combined output (2) with git diff ^!' '\n+\tgit diff sidesansone^! -- >sidesansone &&\n+\tverify_helper sidesansone\n+'\n+\n test_expect_success 'diagnose truncated file' '\n \t>file &&\n \tgit add file &&\n--\n2.37.3\n"}]}