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

[Junio C Hamano <gitster@xxxxxxxxx>]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@xxxxxxxxx>
writes:

> +	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 (obj->flags & UNINTERESTING)
> +				negative++;
> +			else
> +				positive++;
> +			if (obj->type == OBJ_COMMIT)
> +				clear_commit_marks((struct commit *)obj,
> +						   ALL_REV_FLAGS);

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.  Imagine what happens if revs.pending[0].item can
reach the object revs.pending[1].item?  By the time the loop wants
to inspect the latter, its flags may have been cleared already
because we clear flags in objects that are reachable from the
former.

Let's do this as a post-processing for safety (or correctness).

	int negpos = 0;

	if (setup_revisions(3, argv, &revs, 0) != 1)
		goto cleanup;
	for (i = 0; i < revs.pending.nr && negpos != 3; i++) {
		struct object *obj = revs.pending.objects[i].item;
		negpos |= (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;
>  }

or something like that.

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

s/rang/range/, I would think.

Thanks.