* 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).