Am 12.09.22 um 21:09 schrieb Chris Torek:

On Mon, Sep 12, 2022 at 4:16 AM René Scharfe [off-list ref] wrote:

quoted

Am 12.09.22 um 11:57 schrieb Tim Jaacks:

quoted

I noticed that the following syntax to show the changes of a stash stopped working in Git 2.28:

  git diff stash@{0}^!

quoted

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.

quoted

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