Re: git diff ^! syntax stopped working for stashes in Git 2.28
- From
René Scharfe <l.s.r@web.de>
- Date
- Sep 14, 2022, 15:57 UTC
- Message-ID
- <70e87095-d8be-f859-f5b4-a5e15904149c@web.de>
- In-Reply-To
- <CAPx1Gve=BZ3J7KH0FA_BbAnYMEFXyDyiD=tX1vBiKCfHS0JfFg@mail.gmail.com>
Am 12.09.22 um 21:09 schrieb Chris Torek:
Show 10 quoted lines
> 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.
Show 12 quoted lines
>> 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?Show 5 quoted lines
> 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