Re: git diff ^! syntax stopped working for stashes in Git 2.28
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
