* git diff ^! syntax stopped working for stashes in Git 2.28
@ 2022-09-12 9:57 Tim Jaacks
2022-09-12 11:16 ` René Scharfe
0 siblings, 1 reply; 6+ messages in thread
From: Tim Jaacks @ 2022-09-12 9:57 UTC (permalink / raw)
To: git@vger.kernel.org
Hello,
I noticed that the following syntax to show the changes of a stash stopped working in Git 2.28:
git diff stash@{0}^!
It still works on commits and HEAD, though:
git diff f27984d^!
git diff HEAD^!
And diffing against the stash's parent works as well:
git diff stash@{0}^1 stash@{0}
I assume this is a bug. Can anybody confirm this? I verified the behavior change trying different Git versions via docker:
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}^!
With v2.27.0 the above syntax works, with v2.28.0 and later it doesn't.
Kind regards,
Tim
--
Tim Jaacks
SOFTWARE DEVELOPER
SECO Northern Europe GmbH
Schlachthofstrasse 20
21079 Hamburg
Germany
T: +49 40 791899-183
E: tim.jaacks@seco.com
Register: Amtsgericht Hamburg, HRB 148893
Represented by: Dirk Finstel, Marc-Michael Braun, Massimo Mauri
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: git diff ^! syntax stopped working for stashes in Git 2.28 2022-09-12 9:57 git diff ^! syntax stopped working for stashes in Git 2.28 Tim Jaacks @ 2022-09-12 11:16 ` René Scharfe 2022-09-12 19:09 ` Chris Torek 0 siblings, 1 reply; 6+ messages in thread From: René Scharfe @ 2022-09-12 11:16 UTC (permalink / raw) To: Tim Jaacks, git@vger.kernel.org; +Cc: Chris Torek Am 12.09.22 um 11:57 schrieb Tim Jaacks: > Hello, > > I noticed that the following syntax to show the changes of a stash stopped working in Git 2.28: > > git diff stash@{0}^! > > It still works on commits and HEAD, though: > > git diff f27984d^! > git diff HEAD^! > > And diffing against the stash's parent works as well: > > git diff stash@{0}^1 stash@{0} > > I assume this is a bug. Can anybody confirm this? I verified the behavior change trying different Git versions via docker: > > 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}^! > > With v2.27.0 the above syntax works, with v2.28.0 and later it doesn't. Bisects to 8bfcb3a690 (git diff: improve range handling, 2020-06-12). Reverting it partially like in the patch below restores the behavior of "git diff stash@{0}^!". Tests still pass. A stash revision is a merge. With "stash@{0}^!" it ends up in ent.objects[2] and its parents (marked UNINTERESTING) in ent.objects[0] and ent.objects[1]. I don't know if the current behavior is expected or if the patch is the right fix, but in any case it would need a good explanation that I cannot come up with on the spot. Copying Chris, author of the mentioned patch. Old discussion thread: https://lore.kernel.org/git/pull.804.git.git.1591661021.gitgitgadget@gmail.com/ René diff --git a/builtin/diff.c b/builtin/diff.c index 54bb3de964..c34adf2695 100644 --- a/builtin/diff.c +++ b/builtin/diff.c @@ -588,6 +588,10 @@ int cmd_diff(int argc, const char **argv, const char *prefix) sdiff.left, sdiff.right, sdiff.base); result = builtin_diff_tree(&rev, argc, argv, &ent.objects[0], &ent.objects[1]); + } else if (ent.objects[0].item->flags & UNINTERESTING) { + result = builtin_diff_tree(&rev, argc, argv, + &ent.objects[0], + &ent.objects[ent.nr-1]); } else result = builtin_diff_combined(&rev, argc, argv, ent.objects, ent.nr); ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: git diff ^! syntax stopped working for stashes in Git 2.28 2022-09-12 11:16 ` René Scharfe @ 2022-09-12 19:09 ` Chris Torek 2022-09-14 15:57 ` René Scharfe 0 siblings, 1 reply; 6+ messages in thread From: Chris Torek @ 2022-09-12 19:09 UTC (permalink / raw) To: René Scharfe; +Cc: Tim Jaacks, git@vger.kernel.org On Mon, Sep 12, 2022 at 4:16 AM René Scharfe <l.s.r@web.de> wrote: > Am 12.09.22 um 11:57 schrieb Tim Jaacks: >> I noticed that the following syntax to show the changes of a stash stopped working in Git 2.28: >> >> git diff stash@{0}^! > Bisects to 8bfcb3a690 (git diff: improve range handling, 2020-06-12). It's not really clear to me what `git diff stash^!` (and similar) *should* do. That it worked before was at least in part an accident of implementation. > A stash revision is a merge. With "stash@{0}^!" it ends up in > ent.objects[2] and its parents (marked UNINTERESTING) in ent.objects[0] > and ent.objects[1]. Right: the ^! suffix produces a negated list of the children as additional revs (with the stash itself as the lone positive rev). Note that a stash made with `-u` will list *three* such revs rather than just two, since such a stash is a three-parent merge. You're advised (in the git stash documentation) to use `git stash show`, not `git diff`, to get a diff from the stash's parent commit to the stash's working-tree commit. It's certainly possible to detect a single positive rev and two or three negative ones as special cases here, but it's not clear that this is a good idea. Note that in commit bafa2d741e I made one of the tests stop using `git diff HEAD^!` on the grounds that it's not defined behavior. Chris ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: git diff ^! syntax stopped working for stashes in Git 2.28 2022-09-12 19:09 ` Chris Torek @ 2022-09-14 15:57 ` René Scharfe 2022-09-28 9:02 ` AW: " Tim Jaacks 0 siblings, 1 reply; 6+ messages in thread From: René Scharfe @ 2022-09-14 15:57 UTC (permalink / raw) To: Chris Torek; +Cc: Tim Jaacks, git@vger.kernel.org Am 12.09.22 um 21:09 schrieb Chris Torek: > On Mon, Sep 12, 2022 at 4:16 AM René Scharfe <l.s.r@web.de> wrote: >> Am 12.09.22 um 11:57 schrieb Tim Jaacks: >>> I noticed that the following syntax to show the changes of a stash stopped working in Git 2.28: >>> >>> git diff stash@{0}^! > >> Bisects to 8bfcb3a690 (git diff: improve range handling, 2020-06-12). > > It's not really clear to me what `git diff stash^!` (and similar) *should* do. > That it worked before was at least in part an accident of implementation. I get the same impression. git show stash@{0}` and `git show stash@{0}^!` both show a combined diff, which makes sense to me. `git diff stash@{0}^!` also results in a call of diff_tree_combined(), but with trees in a different order. `git diff stash@{0} stash@{0}^1 stash@{0}^2` gives me the same combined diff, but `git diff stash@{0}^!` gives nothing because it acts like `git diff stash@{0}^1 stash@{0}^2 stash@{0}` instead, and both parents have the same tree. That's because revision.c::handle_revision_arg_1() adds the parents before the child when handling "^!" and builtin/diff::builtin_diff_combined() expects the child first. Rotating the array there lets `git diff stash@{0}^!` show a combined diff, but breaks t4013.173, t4013.174 and t4057. Letting revision.c::handle_revision_arg_1() add the child first makes `git diff stash@{0}^!` consistent with `git show stash@{0}^!`. Tests still pass. gitrevisions(7) says <rev>^! is the "same as giving commit <rev> and then all its parents prefixed with ^ to exclude them", so this seems to be an actual fix. My demo patch below feels iffy, though. Hopefully this behavior can be implemented in a nicer way. >> A stash revision is a merge. With "stash@{0}^!" it ends up in >> ent.objects[2] and its parents (marked UNINTERESTING) in ent.objects[0] >> and ent.objects[1]. > > Right: the ^! suffix produces a negated list of the children as additional > revs (with the stash itself as the lone positive rev). Note that a stash > made with `-u` will list *three* such revs rather than just two, since such > a stash is a three-parent merge. > > You're advised (in the git stash documentation) to use `git stash show`, > not `git diff`, to get a diff from the stash's parent commit to the stash's > working-tree commit. So, Tim, does `git stash show -p stash@{0}` work for you? > It's certainly possible to detect a single positive rev and two or three > negative ones as special cases here, but it's not clear that this is a > good idea. Note that in commit bafa2d741e I made one > of the tests stop using `git diff HEAD^!` on the grounds that it's not > defined behavior. Letting `git diff X^!` mean the same as `git diff X ^X^` for a non-merge makes sense to me given the definition from gitrevisions(7) cited above. That in turn is the same as `git diff X^..X` and `git diff X^ X`. René --- revision.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/revision.c b/revision.c index c516415c48..ad4ead2c0a 100644 --- a/revision.c +++ b/revision.c @@ -1819,7 +1819,7 @@ static void add_alternate_refs_to_pending(struct rev_info *revs, } static int add_parents_only(struct rev_info *revs, const char *arg_, int flags, - int exclude_parent) + int exclude_parent, int dry_run) { struct object_id oid; struct object *it; @@ -1850,6 +1850,8 @@ static int add_parents_only(struct rev_info *revs, const char *arg_, int flags, if (exclude_parent && exclude_parent > commit_list_count(commit->parents)) return 0; + if (dry_run) + return 1; for (parents = commit->parents, parent_number = 1; parents; parents = parents->next, parent_number++) { @@ -2083,6 +2085,7 @@ static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int fl const char *arg = arg_; int cant_be_filename = revarg_opt & REVARG_CANNOT_BE_FILENAME; unsigned get_sha1_flags = GET_OID_RECORD_PATH; + int add_parents = 0; flags = flags & UNINTERESTING ? flags | BOTTOM : flags & ~BOTTOM; @@ -2100,14 +2103,16 @@ static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int fl mark = strstr(arg, "^@"); if (mark && !mark[2]) { *mark = 0; - if (add_parents_only(revs, arg, flags, 0)) + if (add_parents_only(revs, arg, flags, 0, 0)) return 0; *mark = '^'; } mark = strstr(arg, "^!"); if (mark && !mark[2]) { *mark = 0; - if (!add_parents_only(revs, arg, flags ^ (UNINTERESTING | BOTTOM), 0)) + if (add_parents_only(revs, arg, flags ^ (UNINTERESTING | BOTTOM), 0, 1)) + add_parents = 1; + else *mark = '^'; } mark = strstr(arg, "^-"); @@ -2122,7 +2127,7 @@ static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int fl } *mark = 0; - if (!add_parents_only(revs, arg, flags ^ (UNINTERESTING | BOTTOM), exclude_parent)) + if (!add_parents_only(revs, arg, flags ^ (UNINTERESTING | BOTTOM), exclude_parent, 0)) *mark = '^'; } @@ -2145,6 +2150,8 @@ static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int fl add_rev_cmdline(revs, object, arg_, REV_CMD_REV, flags ^ local_flags); add_pending_object_with_path(revs, object, arg, oc.mode, oc.path); free(oc.path); + if (add_parents) + add_parents_only(revs, arg_, flags ^ (UNINTERESTING | BOTTOM), 0, 0); return 0; } -- 2.37.3 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* AW: git diff ^! syntax stopped working for stashes in Git 2.28 2022-09-14 15:57 ` René Scharfe @ 2022-09-28 9:02 ` Tim Jaacks 2022-09-28 17:39 ` René Scharfe 0 siblings, 1 reply; 6+ messages in thread From: Tim Jaacks @ 2022-09-28 9:02 UTC (permalink / raw) To: René Scharfe, Chris Torek; +Cc: git@vger.kernel.org Hey Chris, hey René, thanks a lot for your replys. > So, Tim, does `git stash show -p stash@{0}` work for you? Yes, that works indeed. However, in my certain use-case I would prefer using `git diff`. > Letting `git diff X^!` mean the same as `git diff X ^X^` for a non-merge > makes sense to me given the definition from gitrevisions(7) cited above. > That in turn is the same as `git diff X^..X` and `git diff X^ X`. Yes, that is exactly how it worked before. And all the other syntaxes still work correctly. René, I saw you submitted some patches already, but these weren't approved obviously. So how does this continue now? Kind regards, Tim -- Tim Jaacks SOFTWARE DEVELOPER SECO Northern Europe GmbH Schlachthofstrasse 20 21079 Hamburg Germany T: +49 40 791899-183 E: tim.jaacks@seco.com Registergericht: Amtsgericht Hamburg, HRB 148893 Geschäftsführer: Stefan Heczko, Marc-Michael Braun, Massimo Mauri ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: git diff ^! syntax stopped working for stashes in Git 2.28 2022-09-28 9:02 ` AW: " Tim Jaacks @ 2022-09-28 17:39 ` René Scharfe 0 siblings, 0 replies; 6+ messages in thread From: René Scharfe @ 2022-09-28 17:39 UTC (permalink / raw) To: Tim Jaacks, Chris Torek; +Cc: git@vger.kernel.org Am 28.09.22 um 11:02 schrieb Tim Jaacks: > >> Letting `git diff X^!` mean the same as `git diff X ^X^` for a >> non-merge makes sense to me given the definition from >> gitrevisions(7) cited above. That in turn is the same as `git diff >> X^..X` and `git diff X^ X`. > > Yes, that is exactly how it worked before. And all the other syntaxes > still work correctly. `git diff X^!` still works for a non-merge commit as well. Only merge commits were affected by the change made in v2.28. And stashes are stored as merge commits internally, causing them to be affected. > René, I saw you submitted some patches already, but these weren't > approved obviously. So how does this continue now? It's RC time (release candidate) and only fixes for regressions added after v2.37 are accepted currently. I'll resend my patches once v2.38 is out, if necessary. René ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-09-28 17:39 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-09-12 9:57 git diff ^! syntax stopped working for stashes in Git 2.28 Tim Jaacks 2022-09-12 11:16 ` René Scharfe 2022-09-12 19:09 ` Chris Torek 2022-09-14 15:57 ` René Scharfe 2022-09-28 9:02 ` AW: " Tim Jaacks 2022-09-28 17:39 ` René Scharfe
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).