Re: [PATCH 6/6] revision: add parents after child for ^!

["René Scharfe" <l.s.r@web.de>]

Am 15.09.22 um 19:53 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>
>> gitrevisions(7) says: "A suffix '^' followed by an exclamation mark is
>> the same as giving commit '<rev>' and then all its parents prefixed with
>> '^' to exclude them (and their ancestors)."
>
> I did not mean to specify the order at all in that description when
> I wrote ca5ee2d1 (Enumerate revision range specifiers in the
> documentation, 2012-07-24) and I do not think it should be read as
> such.

Then the "then" needs to go.

git rev-parse has its own parser for ^! etc. and puts parents last,
consistent with the "then".

>> handle_revision_arg_1() however adds the negated parents first.
>
> I suspect that this was deliberately done so to match how A..B is
> added to the pending commit list in revisions.c::handle_dotdot_1()
> to tolerate "git diff A..B" as a synonym to "git diff A B", which
> dates back to cd2bdc53 (Common option parsing for "git log --diff"
> and friends, 2006-04-14).

"git diff A B", "git diff A..B", "git diff ^A B" and "git diff "B ^A"
all produce the same output.  So I suspect we could reverse the order
for A..B as well without ill effect if we wanted.

>> Split up the handling of ^! by moving the actual addition of the
>> parents after the addition of the child.
>
> I do not offhand think of anything other than the "diff" frontend
> that cares about the order of these commits from the command line, I
> am afraid that this might end up robbing Peter to pay paul.

Can't think of anything, either, but it may well be possible that yet
another untested use case could depend on getting parents first.
"git diff R^!" was untested as well and it took two years from release
to Tim's bug report after all.  And I'm not particularly proud of my
refactoring patches, so wouldn't mind dropping them.

> Can't we "fix" it at the consumer end, perhaps by checking where
> these commits came from by looking at rev.cmdline?

We could.

--- >8 ---
Subject: [PATCH] diff: support ^! for merges

revision.c::handle_revision_arg_1() resolves <rev>^! by first adding the
negated parents and then <rev> itself.  builtin_diff_combined() expects
the first tree to be the merge and the remaining ones to be the parents,
though.  This mismatch results in bogus diff output.

Remember the first tree that doesn't belong to a parent and use it
instead of blindly picking the first one.  This makes "git diff <rev>^!"
consistent with "git show <rev>^!".

Reported-by: Tim Jaacks <tim.jaacks@garz-fricke.com>
Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
The check "i < rev.cmdline.nr" is necessary to avoid segfaults e.g.
in t0005.  I wonder why.  Shouldn't rev.pending.objects and
rev.cmdline.rev always have the same number of entries?

 builtin/diff.c           | 23 ++++++++++++++++++-----
 t/t4038-diff-combined.sh | 10 ++++++++++
 2 files changed, 28 insertions(+), 5 deletions(-)

diff --git a/builtin/diff.c b/builtin/diff.c
index 54bb3de964..04c6ba0862 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -209,7 +209,7 @@ static int builtin_diff_tree(struct rev_info *revs,
 static int builtin_diff_combined(struct rev_info *revs,
 				 int argc, const char **argv,
 				 struct object_array_entry *ent,
-				 int ents)
+				 int ents, int first_non_parent)
 {
 	struct oid_array parents = OID_ARRAY_INIT;
 	int i;
@@ -217,11 +217,18 @@ static int builtin_diff_combined(struct rev_info *revs,
 	if (argc > 1)
 		usage(builtin_diff_usage);

+	if (first_non_parent < 0)
+		die(_("no merge given, only parents."));
+	if (first_non_parent >= ents)
+		BUG("first_non_parent out of range: %d", first_non_parent);
+
 	diff_merges_set_dense_combined_if_unset(revs);

-	for (i = 1; i < ents; i++)
-		oid_array_append(&parents, &ent[i].item->oid);
-	diff_tree_combined(&ent[0].item->oid, &parents, revs);
+	for (i = 0; i < ents; i++) {
+		if (i != first_non_parent)
+			oid_array_append(&parents, &ent[i].item->oid);
+	}
+	diff_tree_combined(&ent[first_non_parent].item->oid, &parents, revs);
 	oid_array_clear(&parents);
 	return 0;
 }
@@ -385,6 +392,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 	int i;
 	struct rev_info rev;
 	struct object_array ent = OBJECT_ARRAY_INIT;
+	int first_non_parent = -1;
 	int blobs = 0, paths = 0;
 	struct object_array_entry *blob[2];
 	int nongit = 0, no_index = 0;
@@ -541,6 +549,10 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 		if (obj->type == OBJ_TREE) {
 			if (sdiff.skip && bitmap_get(sdiff.skip, i))
 				continue;
+			if (first_non_parent < 0 &&
+			    i < rev.cmdline.nr &&
+			    rev.cmdline.rev[i].whence != REV_CMD_PARENTS_ONLY)
+				first_non_parent = ent.nr;
 			obj->flags |= flags;
 			add_object_array(obj, name, &ent);
 		} else if (obj->type == OBJ_BLOB) {
@@ -590,7 +602,8 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 					   &ent.objects[0], &ent.objects[1]);
 	} else
 		result = builtin_diff_combined(&rev, argc, argv,
-					       ent.objects, ent.nr);
+					       ent.objects, ent.nr,
+					       first_non_parent);
 	result = diff_result_code(&rev.diffopt, result);
 	if (1 < rev.diffopt.skip_stat_unmatch)
 		refresh_index_quietly();
diff --git a/t/t4038-diff-combined.sh b/t/t4038-diff-combined.sh
index 9a292bac70..2ce26e585c 100755
--- a/t/t4038-diff-combined.sh
+++ b/t/t4038-diff-combined.sh
@@ -80,11 +80,21 @@ test_expect_success 'check combined output (1)' '
 	verify_helper sidewithone
 '

+test_expect_success 'check combined output (1) with git diff <rev>^!' '
+	git diff sidewithone^! -- >sidewithone &&
+	verify_helper sidewithone
+'
+
 test_expect_success 'check combined output (2)' '
 	git show sidesansone -- >sidesansone &&
 	verify_helper sidesansone
 '

+test_expect_success 'check combined output (2) with git diff <rev>^!' '
+	git diff sidesansone^! -- >sidesansone &&
+	verify_helper sidesansone
+'
+
 test_expect_success 'diagnose truncated file' '
 	>file &&
 	git add file &&
--
2.37.3