{"thread":{"id":"58419","subject":"git diff ^! syntax stopped working for stashes in Git 2.28","startedAt":"2022-09-12T10:02:23Z","lastAt":"2022-09-28T17:39:44Z","messageCount":6,"participants":["Tim Jaacks","René Scharfe","Chris Torek"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"462928","messageId":"6f729c8c43c4409faf6a11a69791f45a@garz-fricke.com","threadId":"58419","inReplyTo":null,"subject":"git diff ^! syntax stopped working for stashes in Git 2.28","fromName":"Tim Jaacks","fromEmail":"tim.jaacks@garz-fricke.com","sentAt":"2022-09-12T09:57:13Z","receivedAt":"2022-09-12T10:02:23Z","isPatch":false,"sender":{"key":"tim.jaacks@garz-fricke.com","avatar":null},"body":"Hello,\n\nI noticed that the following syntax to show the changes of a stash stopped working in Git 2.28:\n\n  git diff stash@{0}^!\n\nIt still works on commits and HEAD, though:\n\n  git diff f27984d^!\n  git diff HEAD^!\n\nAnd diffing against the stash's parent works as well:\n\n  git diff stash@{0}^1 stash@{0}\n\nI assume this is a bug. Can anybody confirm this? I verified the behavior change trying different Git versions via docker:\n\n  docker run -it --rm --user $(id -u):$(id -g) -v $HOME:$HOME:rw -v /etc/passwd:/etc/passwd:ro -v /etc/group:/etc/group:ro -v $PWD:$PWD:rw -w $PWD bitnami/git:2.27.0 diff stash@{0}^!\n\nWith v2.27.0 the above syntax works, with v2.28.0 and later it doesn't.\n\nKind regards,\nTim\n\n-- \nTim Jaacks\nSOFTWARE DEVELOPER\nSECO Northern Europe GmbH\n\nSchlachthofstrasse 20\n21079 Hamburg\nGermany\nT: +49 40 791899-183\nE: tim.jaacks@seco.com\n\nRegister: Amtsgericht Hamburg, HRB 148893\nRepresented by: Dirk Finstel, Marc-Michael Braun, Massimo Mauri\n\n"},{"id":"462929","messageId":"9e0c1a2c-542b-156f-4c70-371d3a9b452d@web.de","threadId":"58419","inReplyTo":"6f729c8c43c4409faf6a11a69791f45a@garz-fricke.com","subject":"Re: git diff ^! syntax stopped working for stashes in Git 2.28","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-09-12T11:16:26Z","receivedAt":"2022-09-12T11:16:38Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 12.09.22 um 11:57 schrieb Tim Jaacks:\n> Hello,\n>\n> I noticed that the following syntax to show the changes of a stash stopped working in Git 2.28:\n>\n>   git diff stash@{0}^!\n>\n> It still works on commits and HEAD, though:\n>\n>   git diff f27984d^!\n>   git diff HEAD^!\n>\n> And diffing against the stash's parent works as well:\n>\n>   git diff stash@{0}^1 stash@{0}\n>\n> I assume this is a bug. Can anybody confirm this? I verified the behavior change trying different Git versions via docker:\n>\n>   docker run -it --rm --user $(id -u):$(id -g) -v $HOME:$HOME:rw -v /etc/passwd:/etc/passwd:ro -v /etc/group:/etc/group:ro -v $PWD:$PWD:rw -w $PWD bitnami/git:2.27.0 diff stash@{0}^!\n>\n> With v2.27.0 the above syntax works, with v2.28.0 and later it doesn't.\n\nBisects to 8bfcb3a690 (git diff: improve range handling, 2020-06-12).\n\nReverting it partially like in the patch below restores the behavior of\n\"git diff stash@{0}^!\".  Tests still pass.\n\nA stash revision is a merge.  With \"stash@{0}^!\" it ends up in\nent.objects[2] and its parents (marked UNINTERESTING) in ent.objects[0]\nand ent.objects[1].\n\nI don't know if the current behavior is expected or if the patch is the\nright fix, but in any case it would need a good explanation that I\ncannot come up with on the spot.\n\nCopying Chris, author of the mentioned patch.  Old discussion thread:\nhttps://lore.kernel.org/git/pull.804.git.git.1591661021.gitgitgadget@gmail.com/\n\nRené\n\n\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 54bb3de964..c34adf2695 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -588,6 +588,10 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\t\t\tsdiff.left, sdiff.right, sdiff.base);\n \t\tresult = builtin_diff_tree(&rev, argc, argv,\n \t\t\t\t\t   &ent.objects[0], &ent.objects[1]);\n+\t} else if (ent.objects[0].item->flags & UNINTERESTING) {\n+\t\tresult = builtin_diff_tree(&rev, argc, argv,\n+\t\t\t\t\t   &ent.objects[0],\n+\t\t\t\t\t   &ent.objects[ent.nr-1]);\n \t} else\n \t\tresult = builtin_diff_combined(&rev, argc, argv,\n \t\t\t\t\t       ent.objects, ent.nr);\n\n"},{"id":"462944","messageId":"CAPx1Gve=BZ3J7KH0FA_BbAnYMEFXyDyiD=tX1vBiKCfHS0JfFg@mail.gmail.com","threadId":"58419","inReplyTo":"9e0c1a2c-542b-156f-4c70-371d3a9b452d@web.de","subject":"Re: git diff ^! syntax stopped working for stashes in Git 2.28","fromName":"Chris Torek","fromEmail":"chris.torek@gmail.com","sentAt":"2022-09-12T19:09:07Z","receivedAt":"2022-09-12T19:09:24Z","isPatch":false,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"On Mon, Sep 12, 2022 at 4:16 AM René Scharfe <l.s.r@web.de> wrote:\n> Am 12.09.22 um 11:57 schrieb Tim Jaacks:\n>> I noticed that the following syntax to show the changes of a stash stopped working in Git 2.28:\n>>\n>>   git diff stash@{0}^!\n\n> Bisects to 8bfcb3a690 (git diff: improve range handling, 2020-06-12).\n\nIt's not really clear to me what `git diff stash^!` (and similar) *should* do.\nThat it worked before was at least in part an accident of implementation.\n\n> A stash revision is a merge.  With \"stash@{0}^!\" it ends up in\n> ent.objects[2] and its parents (marked UNINTERESTING) in ent.objects[0]\n> and ent.objects[1].\n\nRight: the ^! suffix produces a negated list of the children as additional\nrevs (with the stash itself as the lone positive rev).  Note that a stash\nmade with `-u` will list *three* such revs rather than just two, since such\na stash is a three-parent merge.\n\nYou're advised (in the git stash documentation) to use `git stash show`,\nnot `git diff`, to get a diff from the stash's parent commit to the stash's\nworking-tree commit.\n\nIt's certainly possible to detect a single positive rev and two or three\nnegative ones as special cases here, but it's not clear that this is a\ngood idea.  Note that in commit bafa2d741e I made one\nof the tests stop using `git diff HEAD^!` on the grounds that it's not\ndefined behavior.\n\nChris\n"},{"id":"463019","messageId":"70e87095-d8be-f859-f5b4-a5e15904149c@web.de","threadId":"58419","inReplyTo":"CAPx1Gve=BZ3J7KH0FA_BbAnYMEFXyDyiD=tX1vBiKCfHS0JfFg@mail.gmail.com","subject":"Re: git diff ^! syntax stopped working for stashes in Git 2.28","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-09-14T15:57:37Z","receivedAt":"2022-09-14T15:57:46Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 12.09.22 um 21:09 schrieb Chris Torek:\n> On Mon, Sep 12, 2022 at 4:16 AM René Scharfe <l.s.r@web.de> wrote:\n>> Am 12.09.22 um 11:57 schrieb Tim Jaacks:\n>>> I noticed that the following syntax to show the changes of a stash stopped working in Git 2.28:\n>>>\n>>>   git diff stash@{0}^!\n>\n>> Bisects to 8bfcb3a690 (git diff: improve range handling, 2020-06-12).\n>\n> It's not really clear to me what `git diff stash^!` (and similar) *should* do.\n> That it worked before was at least in part an accident of implementation.\n\nI get the same impression.\n\ngit show stash@{0}` and `git show stash@{0}^!` both show a combined\ndiff, which makes sense to me.  `git diff stash@{0}^!` also results in a\ncall of diff_tree_combined(), but with trees  in a different order.\n`git diff stash@{0} stash@{0}^1 stash@{0}^2` gives me the same combined\ndiff, but `git diff stash@{0}^!` gives nothing because it acts like\n`git diff stash@{0}^1 stash@{0}^2 stash@{0}` instead, and both parents\nhave the same tree.\n\nThat's because revision.c::handle_revision_arg_1() adds the parents\nbefore the child when handling \"^!\" and\nbuiltin/diff::builtin_diff_combined() expects the child first.\nRotating the array there lets `git diff stash@{0}^!` show a combined\ndiff, but breaks t4013.173, t4013.174 and t4057.\n\nLetting revision.c::handle_revision_arg_1() add the child first makes\n`git diff stash@{0}^!` consistent with `git show stash@{0}^!`.  Tests\nstill pass.  gitrevisions(7) says <rev>^! is the \"same as giving commit\n<rev> and then all its parents prefixed with ^ to exclude them\", so\nthis seems to be an actual fix.\n\nMy demo patch below feels iffy, though.  Hopefully this behavior can\nbe implemented in a nicer way.\n\n>> A stash revision is a merge.  With \"stash@{0}^!\" it ends up in\n>> ent.objects[2] and its parents (marked UNINTERESTING) in ent.objects[0]\n>> and ent.objects[1].\n>\n> Right: the ^! suffix produces a negated list of the children as additional\n> revs (with the stash itself as the lone positive rev).  Note that a stash\n> made with `-u` will list *three* such revs rather than just two, since such\n> a stash is a three-parent merge.\n>\n> You're advised (in the git stash documentation) to use `git stash show`,\n> not `git diff`, to get a diff from the stash's parent commit to the stash's\n> working-tree commit.\n\nSo, Tim, does `git stash show -p stash@{0}` work for you?\n\n> It's certainly possible to detect a single positive rev and two or three\n> negative ones as special cases here, but it's not clear that this is a\n> good idea.  Note that in commit bafa2d741e I made one\n> of the tests stop using `git diff HEAD^!` on the grounds that it's not\n> defined behavior.\n\nLetting `git diff X^!` mean the same as `git diff X ^X^` for a non-merge\nmakes sense to me given the definition from gitrevisions(7) cited above.\nThat in turn is the same as `git diff X^..X` and `git diff X^ X`.\n\nRené\n\n\n---\n revision.c | 15 +++++++++++----\n 1 file changed, 11 insertions(+), 4 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex c516415c48..ad4ead2c0a 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1819,7 +1819,7 @@ static void add_alternate_refs_to_pending(struct rev_info *revs,\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+\t\t\t    int exclude_parent, int dry_run)\n {\n \tstruct object_id oid;\n \tstruct object *it;\n@@ -1850,6 +1850,8 @@ static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,\n \tif (exclude_parent &&\n \t    exclude_parent > commit_list_count(commit->parents))\n \t\treturn 0;\n+\tif (dry_run)\n+\t\treturn 1;\n \tfor (parents = commit->parents, parent_number = 1;\n \t     parents;\n \t     parents = parents->next, parent_number++) {\n@@ -2083,6 +2085,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+\tint add_parents = 0;\n\n \tflags = flags & UNINTERESTING ? flags | BOTTOM : flags & ~BOTTOM;\n\n@@ -2100,14 +2103,16 @@ static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int fl\n \tmark = strstr(arg, \"^@\");\n \tif (mark && !mark[2]) {\n \t\t*mark = 0;\n-\t\tif (add_parents_only(revs, arg, flags, 0))\n+\t\tif (add_parents_only(revs, arg, flags, 0, 0))\n \t\t\treturn 0;\n \t\t*mark = '^';\n \t}\n \tmark = strstr(arg, \"^!\");\n \tif (mark && !mark[2]) {\n \t\t*mark = 0;\n-\t\tif (!add_parents_only(revs, arg, flags ^ (UNINTERESTING | BOTTOM), 0))\n+\t\tif (add_parents_only(revs, arg, flags ^ (UNINTERESTING | BOTTOM), 0, 1))\n+\t\t\tadd_parents = 1;\n+\t\telse\n \t\t\t*mark = '^';\n \t}\n \tmark = strstr(arg, \"^-\");\n@@ -2122,7 +2127,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_parents_only(revs, arg, flags ^ (UNINTERESTING | BOTTOM), exclude_parent, 0))\n \t\t\t*mark = '^';\n \t}\n\n@@ -2145,6 +2150,8 @@ 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 (add_parents)\n+\t\tadd_parents_only(revs, arg_, flags ^ (UNINTERESTING | BOTTOM), 0, 0);\n \treturn 0;\n }\n\n--\n2.37.3\n"},{"id":"463838","messageId":"15f30d59650b4de2b14d89f6e8cb21ce@garz-fricke.com","threadId":"58419","inReplyTo":"70e87095-d8be-f859-f5b4-a5e15904149c@web.de","subject":"AW: git diff ^! syntax stopped working for stashes in Git 2.28","fromName":"Tim Jaacks","fromEmail":"tim.jaacks@garz-fricke.com","sentAt":"2022-09-28T09:02:42Z","receivedAt":"2022-09-28T09:02:52Z","isPatch":false,"sender":{"key":"tim.jaacks@garz-fricke.com","avatar":null},"body":"Hey Chris, hey René,\n\nthanks a lot for your replys.\n\n> So, Tim, does `git stash show -p stash@{0}` work for you?\n\nYes, that works indeed. However, in my certain use-case I would prefer using `git diff`.\n \n> Letting `git diff X^!` mean the same as `git diff X ^X^` for a non-merge\n> makes sense to me given the definition from gitrevisions(7) cited above.\n> That in turn is the same as `git diff X^..X` and `git diff X^ X`.\n\nYes, that is exactly how it worked before. And all the other syntaxes still work correctly.\n\nRené, I saw you submitted some patches already, but these weren't approved obviously. So how does this continue now?\n\nKind regards,\nTim\n\n-- \nTim Jaacks\nSOFTWARE DEVELOPER\nSECO Northern Europe GmbH\n\nSchlachthofstrasse 20\n21079 Hamburg\nGermany\nT: +49  40 791899-183\nE: tim.jaacks@seco.com\n\nRegistergericht:  Amtsgericht Hamburg, HRB 148893\nGeschäftsführer: Stefan Heczko, Marc-Michael Braun, Massimo Mauri     \n\n"},{"id":"463861","messageId":"bbcb9a3f-c338-50ea-1d63-1a6939ecd735@web.de","threadId":"58419","inReplyTo":"15f30d59650b4de2b14d89f6e8cb21ce@garz-fricke.com","subject":"Re: git diff ^! syntax stopped working for stashes in Git 2.28","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-09-28T17:39:27Z","receivedAt":"2022-09-28T17:39:44Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 28.09.22 um 11:02 schrieb Tim Jaacks:\n>\n>> Letting `git diff X^!` mean the same as `git diff X ^X^` for a\n>> non-merge makes sense to me given the definition from\n>> gitrevisions(7) cited above. That in turn is the same as `git diff\n>> X^..X` and `git diff X^ X`.\n>\n> Yes, that is exactly how it worked before. And all the other syntaxes\n> still work correctly.\n\n`git diff X^!` still works for a non-merge commit as well.  Only\nmerge commits were affected by the change made in v2.28.  And stashes\nare stored as merge commits internally, causing them to be affected.\n\n> René, I saw you submitted some patches already, but these weren't\n> approved obviously. So how does this continue now?\n\nIt's RC time (release candidate) and only fixes for regressions added\nafter v2.37 are accepted currently.  I'll resend my patches once v2.38\nis out, if necessary.\n\nRené\n"}]}