[PATCH 0/6] revision: fix order of revs for ^!

[PATCH 0/6] revision: fix order of revs for ^!

[René Scharfe]

According to gitrevisions(7), "R^!" is the same as "R ^R^1 [^R^2...]",
but handle_revision_arg_1() instead resolves it to "^R^1 [^R^2...] R".
This ordering affects git diff, which expects the child to be given
before its parents when asked to produce a combined diff of a merge.

This series reverses that order and makes "git diff R^!" consistent
with "git show R^!".  First an unrelated cleanup in the vicinity:

  revision: use strtol_i() for exclude_parent

Then dissolve add_parents_only() to gain the necessary flexibility.  I
may have overdone it:

  revision: factor out get_commit()
  revision: factor out add_parent()
  revision: factor out add_parents()
  revision: rename add_parents_only() to add_nth_parent()

Finally the actual change of order:

  revision: add parents after child for ^!

 revision.c               | 87 ++++++++++++++++++++++++++--------------
 t/t4038-diff-combined.sh | 10 +++++
 2 files changed, 68 insertions(+), 29 deletions(-)

--
2.37.3

[PATCH 1/6] revision: use strtol_i() for exclude_parent

[René Scharfe]

Avoid silent overflow of the int exclude_parent by using the appropriate
function, strtol_i(), to parse its value.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 revision.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/revision.c b/revision.c
index d5f4463cb6..0b8d48f94c 100644
--- a/revision.c
+++ b/revision.c
@@ -2119,9 +2119,8 @@ static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int fl
 		int exclude_parent = 1;

 		if (mark[2]) {
-			char *end;
-			exclude_parent = strtoul(mark + 2, &end, 10);
-			if (*end != '\0' || !exclude_parent)
+			if (strtol_i(mark + 2, 10, &exclude_parent) ||
+			    exclude_parent < 1)
 				return -1;
 		}

--
2.37.3

Re: [PATCH 1/6] revision: use strtol_i() for exclude_parent

[Chris Torek]

On Thu, Sep 15, 2022 at 7:51 AM René Scharfe <l.s.r@web.de> wrote:
> Avoid silent overflow of the int exclude_parent by using the appropriate
> function, strtol_i(), to parse its value.
>
> Signed-off-by: René Scharfe <l.s.r@web.de>

Nice (albeit small) improvement regardless of anything else,

Chris

[PATCH 2/6] revision: factor out get_commit()

[René Scharfe]

Split out the first half of add_parents_only() to obtain a function that
finds and returns the commit object.  It allows checking the validity of
the child separately from adding its parents.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 revision.c | 35 +++++++++++++++++++++++------------
 1 file changed, 23 insertions(+), 12 deletions(-)

diff --git a/revision.c b/revision.c
index 0b8d48f94c..4f896b4992 100644
--- a/revision.c
+++ b/revision.c
@@ -1822,35 +1822,46 @@ static void add_alternate_refs_to_pending(struct rev_info *revs,
 	for_each_alternate_ref(add_one_alternate_ref, &data);
 }

-static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,
-			    int exclude_parent)
+static struct commit *get_commit(struct rev_info *revs, const char *arg_)
 {
 	struct object_id oid;
 	struct object *it;
-	struct commit *commit;
-	struct commit_list *parents;
-	int parent_number;
 	const char *arg = arg_;

-	if (*arg == '^') {
-		flags ^= UNINTERESTING | BOTTOM;
+	if (*arg == '^')
 		arg++;
-	}
 	if (get_oid_committish(arg, &oid))
-		return 0;
+		return NULL;
 	while (1) {
 		it = get_reference(revs, arg, &oid, 0);
 		if (!it && revs->ignore_missing)
-			return 0;
+			return NULL;
 		if (it->type != OBJ_TAG)
 			break;
 		if (!((struct tag*)it)->tagged)
-			return 0;
+			return NULL;
 		oidcpy(&oid, &((struct tag*)it)->tagged->oid);
 	}
 	if (it->type != OBJ_COMMIT)
+		return NULL;
+	return (struct commit *)it;
+}
+
+static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,
+			    int exclude_parent)
+{
+	struct object *it;
+	struct commit *commit = get_commit(revs, arg_);
+	struct commit_list *parents;
+	int parent_number;
+	const char *arg = arg_;
+
+	if (*arg == '^') {
+		flags ^= UNINTERESTING | BOTTOM;
+		arg++;
+	}
+	if (!commit)
 		return 0;
-	commit = (struct commit *)it;
 	if (exclude_parent &&
 	    exclude_parent > commit_list_count(commit->parents))
 		return 0;
--
2.37.3

[PATCH 3/6] revision: factor out add_parent()

[René Scharfe]

Split out the addition of a single parent into a helper.  We'll use it
in the next commit.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 revision.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/revision.c b/revision.c
index 4f896b4992..14cb73e508 100644
--- a/revision.c
+++ b/revision.c
@@ -1847,19 +1847,27 @@ static struct commit *get_commit(struct rev_info *revs, const char *arg_)
 	return (struct commit *)it;
 }

-static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,
-			    int exclude_parent)
+static void add_parent(struct rev_info *revs, struct object *parent,
+		       const char *arg_, int flags)
 {
-	struct object *it;
-	struct commit *commit = get_commit(revs, arg_);
-	struct commit_list *parents;
-	int parent_number;
 	const char *arg = arg_;

 	if (*arg == '^') {
 		flags ^= UNINTERESTING | BOTTOM;
 		arg++;
 	}
+	parent->flags |= flags;
+	add_rev_cmdline(revs, parent, arg_, REV_CMD_PARENTS_ONLY, flags);
+	add_pending_object(revs, parent, arg);
+}
+
+static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,
+			    int exclude_parent)
+{
+	struct commit *commit = get_commit(revs, arg_);
+	struct commit_list *parents;
+	int parent_number;
+
 	if (!commit)
 		return 0;
 	if (exclude_parent &&
@@ -1871,10 +1879,7 @@ static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,
 		if (exclude_parent && parent_number != exclude_parent)
 			continue;

-		it = &parents->item->object;
-		it->flags |= flags;
-		add_rev_cmdline(revs, it, arg_, REV_CMD_PARENTS_ONLY, flags);
-		add_pending_object(revs, it, arg);
+		add_parent(revs, &parents->item->object, arg_, flags);
 	}
 	return 1;
 }
--
2.37.3

[PATCH 4/6] revision: factor out add_parents()

[René Scharfe]

Add a function that registers all parents.  Use it in add_parents_only()
to handle the case of exclude_parent being unset early.

With that out of the way we can specialize the remaining loop can to
register only the specified parent.  And since we exit reporting success
once we got it we no longer need to check the total number of parents
beforehand.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 revision.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/revision.c b/revision.c
index 14cb73e508..284393a146 100644
--- a/revision.c
+++ b/revision.c
@@ -1861,6 +1861,13 @@ static void add_parent(struct rev_info *revs, struct object *parent,
 	add_pending_object(revs, parent, arg);
 }

+static void add_parents(struct rev_info *revs, struct commit_list *parents,
+			const char *arg, int flags)
+{
+	for (; parents; parents = parents->next)
+		add_parent(revs, &parents->item->object, arg, flags);
+}
+
 static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,
 			    int exclude_parent)
 {
@@ -1870,18 +1877,19 @@ static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,

 	if (!commit)
 		return 0;
-	if (exclude_parent &&
-	    exclude_parent > commit_list_count(commit->parents))
-		return 0;
+	if (!exclude_parent) {
+		add_parents(revs, commit->parents, arg_, flags);
+		return 1;
+	}
 	for (parents = commit->parents, parent_number = 1;
 	     parents;
 	     parents = parents->next, parent_number++) {
-		if (exclude_parent && parent_number != exclude_parent)
-			continue;
-
-		add_parent(revs, &parents->item->object, arg_, flags);
+		if (parent_number == exclude_parent) {
+			add_parent(revs, &parents->item->object, arg_, flags);
+			return 1;
+		}
 	}
-	return 1;
+	return 0;
 }

 void repo_init_revisions(struct repository *r,
--
2.37.3

[PATCH 5/6] revision: rename add_parents_only() to add_nth_parent()

[René Scharfe]

Move the handling of !exclude_parent to the two callers that pass zero.
This allows checking the validity of the child separately from adding
its parents, which we'll make use of in the next patch.

Rename the function to reflect its changed purpose, now that it
requires exclude_parent to be given and only adds at most one parent.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
Created with --inter-hunk-context=1 for easier review.

 revision.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/revision.c b/revision.c
index 284393a146..5e756b76aa 100644
--- a/revision.c
+++ b/revision.c
@@ -1868,19 +1868,15 @@ static void add_parents(struct rev_info *revs, struct commit_list *parents,
 		add_parent(revs, &parents->item->object, arg, flags);
 }

-static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,
-			    int exclude_parent)
+static int add_nth_parent(struct rev_info *revs, const char *arg_, int flags,
+			  int exclude_parent)
 {
 	struct commit *commit = get_commit(revs, arg_);
 	struct commit_list *parents;
 	int parent_number;

 	if (!commit)
 		return 0;
-	if (!exclude_parent) {
-		add_parents(revs, commit->parents, arg_, flags);
-		return 1;
-	}
 	for (parents = commit->parents, parent_number = 1;
 	     parents;
 	     parents = parents->next, parent_number++) {
@@ -2127,15 +2123,26 @@ static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int fl

 	mark = strstr(arg, "^@");
 	if (mark && !mark[2]) {
+		struct commit *commit;
+
 		*mark = 0;
-		if (add_parents_only(revs, arg, flags, 0))
+		commit = get_commit(revs, arg);
+		if (commit) {
+			add_parents(revs, commit->parents, arg, flags);
 			return 0;
+		}
 		*mark = '^';
 	}
 	mark = strstr(arg, "^!");
 	if (mark && !mark[2]) {
+		struct commit *commit;
+
 		*mark = 0;
-		if (!add_parents_only(revs, arg, flags ^ (UNINTERESTING | BOTTOM), 0))
+		commit = get_commit(revs, arg);
+		if (commit)
+			add_parents(revs, commit->parents, arg,
+				    flags ^ (UNINTERESTING | BOTTOM));
+		else
 			*mark = '^';
 	}
 	mark = strstr(arg, "^-");
@@ -2149,7 +2156,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_nth_parent(revs, arg, flags ^ (UNINTERESTING | BOTTOM), exclude_parent))
 			*mark = '^';
 	}

--
2.37.3

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

[René Scharfe]

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

handle_revision_arg_1() however adds the negated parents first.  This
leads to unexpected results with git diff and merge commits, as that
command expects the documented order.

Split up the handling of ^! by moving the actual addition of the parents
after the addition of the child.

Reported-by: Tim Jaacks <tim.jaacks@garz-fricke.com>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
 revision.c               | 13 ++++++-------
 t/t4038-diff-combined.sh | 10 ++++++++++
 2 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/revision.c b/revision.c
index 5e756b76aa..8385127f1a 100644
--- a/revision.c
+++ b/revision.c
@@ -2107,6 +2107,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;
+	struct commit *caret_bang_commit = NULL;

 	flags = flags & UNINTERESTING ? flags | BOTTOM : flags & ~BOTTOM;

@@ -2135,14 +2136,9 @@ static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int fl
 	}
 	mark = strstr(arg, "^!");
 	if (mark && !mark[2]) {
-		struct commit *commit;
-
 		*mark = 0;
-		commit = get_commit(revs, arg);
-		if (commit)
-			add_parents(revs, commit->parents, arg,
-				    flags ^ (UNINTERESTING | BOTTOM));
-		else
+		caret_bang_commit = get_commit(revs, arg);
+		if (!caret_bang_commit)
 			*mark = '^';
 	}
 	mark = strstr(arg, "^-");
@@ -2179,6 +2175,9 @@ 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 (caret_bang_commit)
+		add_parents(revs, caret_bang_commit->parents, arg,
+			    flags ^ (UNINTERESTING | BOTTOM));
 	return 0;
 }

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

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

[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.

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

> 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 we "fix" it at the consumer end, perhaps by checking where
these commits came from by looking at rev.cmdline?

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

[René Scharfe]

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

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

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> "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.

Yup.

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

Yeah, doing so may be more in line with how two-tree comparison is
parsed (your observation above on four combinations).

> --- >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?

Things that did not come from command line can go into pending,
can't they?  E.g. add_head_to_pending(), when lack of rev defaults
to "HEAD", touches pending without touching cmdline?

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

[René Scharfe]

Am 16.09.22 um 17:55 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>
>> 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?
>
> Things that did not come from command line can go into pending,
> can't they?  E.g. add_head_to_pending(), when lack of rev defaults
> to "HEAD", touches pending without touching cmdline?

Indeed.  In t0005 it's even only a fake HEAD with an empty_tree added by
cmd_diff() a few lines up, for a diff of a new file in a fresh, empty
repository.

René

[PATCH v3 0/3] diff: support ^! for merges

[René Scharfe]

Changes between v1 [1] and v2 [2]:
- Single patch, local fix, leave revision.c alone.

Changes since v2 [2]:
- Patch 1 (unrelated cleanup) is back, requested by Chris.
- New patch 2 to remove a broken promise from documentation.
- Move the first_non_parent tracking code to after add_object_array()
  and add a comment, both to (hopefully) improve readability.
- Mention ^! in the documentation of git diff.

  revision: use strtol_i() for exclude_parent
  revisions.txt: unspecify order of resolved parts of ^!
  diff: support ^! for merges

[1] https://lore.kernel.org/git/ba6eea28-fb3a-b376-2529-351727c02f1a@web.de/
[2] https://lore.kernel.org/git/29d50baa-1923-38e1-6ecb-73840376d28e@web.de/

 Documentation/git-diff.txt  |  8 ++++----
 Documentation/revisions.txt |  2 +-
 builtin/diff.c              | 23 ++++++++++++++++++-----
 revision.c                  |  5 ++---
 t/t4038-diff-combined.sh    | 10 ++++++++++
 5 files changed, 35 insertions(+), 13 deletions(-)

--
2.37.3

[PATCH v3 1/3] revision: use strtol_i() for exclude_parent

[René Scharfe]

Avoid silent overflow of the int exclude_parent by using the appropriate
function, strtol_i(), to parse its value.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 revision.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/revision.c b/revision.c
index 36e31942ce..79b88604c7 100644
--- a/revision.c
+++ b/revision.c
@@ -2120,9 +2120,8 @@ static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int fl
 		int exclude_parent = 1;

 		if (mark[2]) {
-			char *end;
-			exclude_parent = strtoul(mark + 2, &end, 10);
-			if (*end != '\0' || !exclude_parent)
+			if (strtol_i(mark + 2, 10, &exclude_parent) ||
+			    exclude_parent < 1)
 				return -1;
 		}

--
2.37.3

[PATCH v3 2/3] revisions.txt: unspecify order of resolved parts of ^!

[René Scharfe]

gitrevisions(7) says that <rev>^! resolves to <rev> and then all the
parents of <rev>.  revision.c::handle_revision_arg_1() actually adds
all parents first, then <rev>.  Change the documentation to leave the
order unspecified, to avoid misleading readers.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 Documentation/revisions.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt
index e3e350126d..0d2e55d781 100644
--- a/Documentation/revisions.txt
+++ b/Documentation/revisions.txt
@@ -363,7 +363,7 @@ Revision Range Summary

 '<rev>{caret}!', e.g. 'HEAD{caret}!'::
   A suffix '{caret}' followed by an exclamation mark is the same
-  as giving commit '<rev>' and then all its parents prefixed with
+  as giving commit '<rev>' and all its parents prefixed with
   '{caret}' to exclude them (and their ancestors).

 '<rev>{caret}-<n>', e.g. 'HEAD{caret}-, HEAD{caret}-2'::
--
2.37.3

[PATCH v3 3/3] diff: support ^! for merges

[René Scharfe]

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>
---
 Documentation/git-diff.txt |  8 ++++----
 builtin/diff.c             | 23 ++++++++++++++++++-----
 t/t4038-diff-combined.sh   | 10 ++++++++++
 3 files changed, 32 insertions(+), 9 deletions(-)

diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt
index 85ae6d6d08..52b679256c 100644
--- a/Documentation/git-diff.txt
+++ b/Documentation/git-diff.txt
@@ -79,10 +79,10 @@ If --merge-base is given, use the merge base of the two commits for the

 	This form is to view the results of a merge commit.  The first
 	listed <commit> must be the merge itself; the remaining two or
-	more commits should be its parents.  A convenient way to produce
-	the desired set of revisions is to use the `^@` suffix.
-	For instance, if `master` names a merge commit, `git diff master
-	master^@` gives the same combined diff as `git show master`.
+	more commits should be its parents.  Convenient ways to produce
+	the desired set of revisions are to use the suffixes `^@` and
+	`^!`.  If A is a merge commit, then `git diff A A^@`,
+	`git diff A^!` and `git show A` all give the same combined diff.

 'git diff' [<options>] <commit>..<commit> [--] [<path>...]::

diff --git a/builtin/diff.c b/builtin/diff.c
index 54bb3de964..0e49919735 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;
@@ -543,6 +551,10 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 				continue;
 			obj->flags |= flags;
 			add_object_array(obj, name, &ent);
+			if (first_non_parent < 0 &&
+			    (i >= rev.cmdline.nr || /* HEAD by hand. */
+			     rev.cmdline.rev[i].whence != REV_CMD_PARENTS_ONLY))
+				first_non_parent = ent.nr - 1;
 		} else if (obj->type == OBJ_BLOB) {
 			if (2 <= blobs)
 				die(_("more than two blobs given: '%s'"), name);
@@ -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