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

Junio C Hamano [off-list ref] 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.

Here is what I added on top of the 3-patch series for tonight's
pushout.

 * 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

Hi Junio,

On Thu, 4 Feb 2021, Junio C Hamano wrote:

Junio C Hamano [off-list ref] writes:

quoted

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.

Here is what I added on top of the 3-patch series for tonight's
pushout.

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

Your code is substantially harder to read. I really dislike that `npmask`
idea.

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

I implemented that.

 * Dedent by judicious use of 'goto'.

Not necessary: the code is short and narrow enough.

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

I had missed that in your review. Fixed, too.

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

Technically, it is not necessary, as we're only verifying that the
intended error message is shown, and that "e" does not make any
difference.

But it _was_ a typo, so I fixed it, too.

Next iteration is coming soon,
Dscho