Re: [PATCH v5 2/3] range-diff/format-patch: handle commit ranges other than A..B

From

Junio C Hamano <gitster@pobox.com>

Date

Feb 5, 2021, 01:32 UTC

Message-ID

<xmqqim772tal.fsf@gitster.c.googlers.com>

In-Reply-To

<xmqqmtwj2ugg.fsf@gitster.c.googlers.com>

Junio C Hamano <gitster@pobox.com> writes:

> Ah, I didn't think of adding this inside the same loop.  As long as
> the pending objects are independent, this should work, but it is
> unsafe, I think.

 * No need to count positive/negative; just seeing both exists is
   sufficient and there is no need to examine any later elements, if
   any.

 * Clearing commit marks needs to be done after we have seen enough,
   or it can clear the stuff we would want to inspect before we
   have a chance to.  Do it in a separate post-cleaning loop.

 * Dedent by judicious use of 'goto'.

 * The last parameter to setup_revisions() is a pointer, so spell it
   NULL not 0.

 * "rang" -> "range" typofix (it might be even better to look for
   "range:")

----- >8 ---------- >8 ---------- >8 ---------- >8 ---------- >8 ---------- >8 -----
Subject: [PATCH] fixup! range-diff/format-patch: handle commit ranges other than A..B

 range-diff.c          | 30 +++++++++++++++++-------------
 t/t3206-range-diff.sh |  2 +-
 2 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/range-diff.c b/range-diff.c
index c307bca9de..7a38dc8715 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -570,24 +570,28 @@ int is_range_diff_range(const char *arg)
 {
 	char *copy = xstrdup(arg); /* setup_revisions() modifies it */
 	const char *argv[] = { "", copy, "--", NULL };
-	int i, positive = 0, negative = 0;
+	int i;
 	struct rev_info revs;
+	unsigned npmask = 0;
 
 	init_revisions(&revs, NULL);
-	if (setup_revisions(3, argv, &revs, 0) == 1)
-		for (i = 0; i < revs.pending.nr; i++) {
-			struct object *obj = revs.pending.objects[i].item;
+	if (setup_revisions(3, argv, &revs, NULL) != 1)
+		goto cleanup;
 
-			if (obj->flags & UNINTERESTING)
-				negative++;
-			else
-				positive++;
-			if (obj->type == OBJ_COMMIT)
-				clear_commit_marks((struct commit *)obj,
-						   ALL_REV_FLAGS);
-		}
+	for (i = 0; i < revs.pending.nr && npmask != 3; i++) {
+		struct object *obj = revs.pending.objects[i].item;
+
+		npmask |= (obj->flags & UNINTERESTING) ? 01 : 02;
+	}
+
+	for (i = 0; i < revs.pending.nr; i++) {
+		struct object *obj = revs.pending.objects[i].item;
+		if (obj->type == OBJ_COMMIT)
+			clear_commit_marks((struct commit *)obj, ALL_REV_FLAGS);
+	}
 
+cleanup:
 	free(copy);
 	object_array_clear(&revs.pending);
-	return negative > 0 && positive > 0;
+	return npmask == 3;
 }
diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index 45f21ee215..2b518378d4 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -160,7 +160,7 @@ test_expect_success 'A^! and A^-<n> (unmodified)' '
 
 test_expect_success 'A^{/..} is not mistaken for a range' '
 	test_must_fail git range-diff topic^.. topic^{/..} 2>error &&
-	test_i18ngrep "not a commit rang" error
+	test_i18ngrep "not a commit range" error
 '
 
 test_expect_success 'trivial reordering' '

-- 
2.30.0-601-g9b6178ed87