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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Junio,

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

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

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